diff --git a/.planning/ROADMAP.md b/.planning/ROADMAP.md new file mode 100644 index 0000000..2183355 --- /dev/null +++ b/.planning/ROADMAP.md @@ -0,0 +1,519 @@ +# Flagship Roadmap + +Status: **active** +Spec: `.planning/SPEC.md` +Operating rule: advance only when the current gate is explicitly PASS. + +## How To Use This Roadmap + +This file is an executable state machine, not a wishlist. + +At the start of a session: + +1. find the first phase whose gate is not `PASS`, `KILL`, or permanently `BLOCKED` +2. fix blockers inside that phase before starting anything later +3. make the smallest change needed to answer the phase question +4. run the listed verification +5. record `PASS`, `FIX`, `KILL`, or `BLOCKED` +6. only then move to the next phase + +New ideas go to **Backlog** unless they materially change the current gate. + +--- + +# Current State + +## Phase 0 — Review-context v1 functional contract + +Status: **IN PROGRESS** + +Current implementation PR: `#128 feat(review): add bounded review-context packets` + +Question: + +> Can the public project reliably compile a bounded, versioned review-context packet from an exact committed git change without an LLM? + +### Deliverables + +- [x] review-context core packet builder +- [x] executable `codebase-context-review` package binary +- [x] exact base/head commit identity +- [x] exact diff fingerprint +- [x] clean-worktree / checked-out-head invariants +- [x] rename and binary handling +- [x] NUL-safe filename parsing +- [x] bounded identifier extraction +- [x] bounded related-context retrieval +- [x] convention/pattern attachment +- [x] warnings / quality metadata +- [x] unit tests for the packet core +- [x] executable-wrapper smoke test +- [x] public review-context documentation +- [x] autonomous product spec and gated roadmap +- [ ] CI quality gate green +- [ ] CI functional test gate green +- [ ] squash merge to `master` + +### Gate G0 — Functional publishability + +PASS only if all are true: + +- [ ] `pnpm lint` has zero errors +- [ ] `pnpm format:check` passes +- [ ] `pnpm type-check` passes +- [ ] `pnpm build` passes +- [ ] full test suite passes +- [ ] executable wrapper smoke passes +- [ ] PR remains mergeable +- [ ] docs describe actual behavior and limitations +- [ ] no performance/reviewer-quality claim has been added + +On PASS: + +- squash merge PR #128 +- evidence level becomes **E1** for the review-context surface +- move immediately to Phase 1 + +On FAIL: + +- fix only the failing G0 requirement +- do not add features + +--- + +# Phase 1 — Freeze the first honest review-context evaluation + +Status: **BLOCKED BY G0** + +Question: + +> What frozen public task set can distinguish useful review context from plausible-looking retrieval noise? + +This phase freezes evidence **before** retrieval tuning. + +## Required task set + +Minimum target: + +- 20 public tasks +- at least 5 public repositories +- at least 3 language ecosystems +- mix of small and non-trivial changes +- real bug-fix / review-relevant changes, not only synthetic retrieval queries +- exact repository + base/head commit identity +- public reproducibility + +Prefer tasks where ground truth can identify: + +- changed files +- causally relevant non-changed files +- relevant symbols/spans when defensible +- actual bug/finding when the task supports reviewer evaluation + +## Freeze artifacts + +Before looking at treatment results, commit: + +- [ ] task manifest +- [ ] repo/commit identities +- [ ] task inclusion/exclusion rationale +- [ ] ground-truth format +- [ ] scorer +- [ ] retrieval limits +- [ ] setup/index accounting policy +- [ ] failure-row policy +- [ ] baseline definitions +- [ ] report schema + +## Required retrieval baselines + +At minimum: + +1. changed files/path signals only +2. repository-native/basic text navigation +3. current `review-context-v1` + +If an additional competitor is cheap and reproducible, add it **before** running treatment results. + +## Gate G1 — Eval freeze integrity + +PASS only if: + +- [ ] all tasks are frozen before treatment inspection +- [ ] task set meets diversity minimums +- [ ] scorer does not depend on treatment output +- [ ] baseline budgets are explicit +- [ ] failures stay in denominator +- [ ] no private/employer data is used +- [ ] another engineer could materialize the set from public information + +On PASS: + +- tag/fingerprint the frozen manifest +- move to Phase 2 + +On FAIL: + +- repair the evaluation design before running any claim-bearing treatment + +--- + +# Phase 2 — Prove or kill repository-context retrieval value + +Status: **BLOCKED BY G1** + +Question: + +> Does the review-context retrieval lane find materially more relevant context than simple baselines at acceptable precision and cost? + +No reviewer LLM is needed yet. First isolate retrieval quality. + +## Metrics + +Record separately: + +- relevant-file recall@k +- relevant-span recall@k where ground truth permits +- precision@k +- characters/tokens returned +- setup/index time +- retrieval wall time +- failures / abstentions +- peak memory where practical + +## Comparison discipline + +- same task set +- same frozen commits +- same k/budget where comparable +- setup/index costs reported, not hidden +- no best-of-N +- no per-task hand tuning + +## Gate G2 — Retrieval value + +### PASS + +Pass if the evidence shows a useful and reasonably general trade-off versus simple baselines. + +A PASS does not require winning every task. It requires a defensible aggregate improvement that is not explained by massively larger context budgets. + +Then: + +- evidence level becomes **E2** +- publish honest result table + failure analysis +- move to Phase 3 + +### FIX + +One bounded repair cycle is allowed if: + +- failure mode is coherent and general +- fix can be justified without task-specific branching +- frozen tasks/ground truth remain untouched + +After that repair, rerun the full frozen set once. + +### KILL + +Kill or substantially simplify this retrieval lane if, after the bounded repair: + +- simple baselines match/beat it at lower complexity/cost +- precision is too low +- gains are isolated to development repos/frameworks +- setup/index cost overwhelms value + +If killed: + +- publish the negative result honestly +- retain the deterministic git/review packet pieces that still have value +- redesign from the simplest winning baseline, not from sunk cost + +--- + +# Phase 3 — Measure actual reviewer impact + +Status: **BLOCKED BY G2 PASS** + +Question: + +> Does the bounded context packet improve real review findings when reasoning is held constant? + +## Frozen experimental arms + +### Control + +Same reviewer model + prompt + budget with raw diff/repository-native tools only. + +### Treatment + +Same reviewer model + prompt + budget plus `review-context-v1`. + +Do not give the treatment hidden extra turns/tokens unless that cost difference is explicitly part of the reported trade-off. + +## Reviewer harness requirements + +- [ ] model/provider/version recorded +- [ ] prompt recorded +- [ ] tool set recorded +- [ ] token/turn budget recorded +- [ ] timeout recorded +- [ ] repeated runs where nondeterminism matters +- [ ] append-only attempt records +- [ ] invalid output / tool failure is terminal evidence +- [ ] no best-of-N selection + +## Metrics + +Report separately: + +- real finding recall +- precision / false positives +- evidence correctness +- unsupported claims +- token/context usage +- wall time +- tool/setup failure rate + +## Gate G3 — Reviewer value + +### PASS + +Pass only if reviewer quality improves enough to justify the added context cost and the effect is not confined to one repo/language. + +Then: + +- evidence level becomes **E3** +- publish methodology + failure cases +- move to Phase 4 + +### FIX + +One bounded context-compilation repair cycle is allowed if the failure analysis clearly implicates retrieval/context rather than reviewer reasoning. + +Do not tune the reviewer prompt per task to rescue the treatment. + +### KILL + +Kill the claim that this context strategy helps review if the treatment does not produce a meaningful trade-off after the bounded repair cycle. + +Do not respond by adding agent orchestration, more models, or a dashboard. + +--- + +# Phase 4 — External reproduction and usefulness + +Status: **BLOCKED BY G3 PASS** + +Question: + +> Can someone outside the development loop reproduce the value and use the product without bespoke help? + +## Deliverables + +- [ ] stable npm release containing the review command +- [ ] concise README entrypoint +- [ ] one public end-to-end example +- [ ] reproducible evaluation command +- [ ] machine-readable result artifact +- [ ] install/runtime limitations documented +- [ ] external feedback channel + +Seek at least one of: + +- external engineer reproduces evaluation +- maintainer/user runs it on another repo +- useful issue/PR from an external user +- independent adoption/integration signal + +## Gate G4 — External evidence + +PASS when there is credible external reproduction/use rather than only owner/agent self-evaluation. + +Then evidence level becomes **E4**. + +If external users cannot successfully run or understand it, fix usability before expanding scope. + +--- + +# Phase 5 — Real PR workflow integration + +Status: **BLOCKED BY G3 PASS; PREFER G4** + +Question: + +> Is the proven context capability useful enough to integrate into real pull-request workflows? + +Only now consider: + +- GitHub Actions integration +- GitHub App +- Azure DevOps adapter +- model-agnostic reviewer consumer +- comment drafting +- review result persistence + +## Mandatory safety boundary + +The first integration must default to **read-only / draft output**. + +No autonomous public review comments until: + +- reviewer precision is measured +- false-positive policy exists +- explicit opt-in exists +- dry-run evidence exists + +## Gate G5 — Operational usefulness + +PASS requires: + +- real PR input +- reproducible context packet +- reviewer output trace +- explicit failure state +- no silent posting +- measurable latency/cost + +--- + +# Phase 6 — Optional platform/full-stack expansion + +Status: **BLOCKED BY PRODUCT EVIDENCE** + +This phase exists for product utility and engineering breadth, not portfolio theater. + +Promote only if real usage creates a need for shared state, concurrency, observability, or hosted exploration. + +Possible components: + +- TypeScript/Node API boundary +- async worker/queue for evaluations +- persistent run metadata +- Next.js benchmark/result explorer +- optional authentication +- observability +- one minimal cloud deployment path + +The default local-first CLI/MCP path must remain usable without cloud infrastructure. + +## Gate G6 — Platform justification + +Do not build this phase unless at least one is true: + +- external users need shared runs/results +- evaluations require durable async execution +- a real integration needs a service boundary +- operating evidence manually is now a material bottleneck + +A desire to demonstrate AWS/Next.js is not sufficient product justification by itself. + +--- + +# Autonomous Routing Rules + +When new information arrives, route it as follows. + +## CI or test failure + +→ current phase blocker +→ fix before all other work + +## Bug in published contract + +→ hotfix current stable surface +→ add regression test +→ return to current roadmap gate + +## Benchmark result + +→ update current evidence gate +→ choose PASS/FIX/KILL/BLOCKED +→ do not brainstorm around an unfavorable result + +## New paper/tool/competitor + +Ask: + +1. does it change a frozen baseline or invalidate current methodology? +2. does it expose a clear general failure mode? +3. is it cheap enough to add before treatment observation? + +If no → Backlog. + +## External user/maintainer interest + +→ high-priority opportunity +→ fix adoption blockers ahead of speculative features + +## New project idea + +→ default Backlog/reject +→ it must beat finishing the current gate, not merely sound interesting + +--- + +# Work-In-Progress Limits + +At any time: + +- maximum 1 active flagship product PR +- maximum 1 active claim-bearing evaluation +- maximum 1 bounded repair cycle per failed evidence gate before a kill/rethink decision + +Do not maintain parallel speculative implementations of the same hypothesis. + +--- + +# Merge Rules + +For roadmap product work: + +1. branch from current `master` +2. one primary scope per PR +3. CI must be green +4. relevant evidence gate must be satisfied +5. squash merge unless history itself is useful evidence +6. update this roadmap when a gate changes state + +Never merge because the code "looks right" while the required gate is red. + +--- + +# Backlog — Not Active + +These are intentionally **not** roadmap commitments. + +- AST-aware diff signal extraction +- richer graph expansion +- hosted dashboard +- autonomous PR commenter +- Azure DevOps review adapter +- GitHub App +- queue/worker infrastructure +- Next.js result explorer +- AWS deployment +- multi-user auth +- paid hosted embeddings +- framework-specific reviewer rules +- standalone replacement for existing reviewer products + +Promotion rule: + +> A backlog item enters Track A only when evidence or external pull shows it is the smallest next step for the current product bottleneck. + +--- + +# Immediate Next Action + +**Do not start Phase 1 yet.** + +Finish G0 on PR #128: + +1. make `src/review-context.ts` exactly Prettier-compliant +2. get quality checks green +3. get full functional tests green, including the executable wrapper smoke +4. remove any temporary verification-only workflow/artifact before merge +5. squash merge PR #128 +6. mark G0 PASS +7. only then freeze the Phase 1 evaluation diff --git a/.planning/SPEC.md b/.planning/SPEC.md new file mode 100644 index 0000000..ee6a294 --- /dev/null +++ b/.planning/SPEC.md @@ -0,0 +1,493 @@ +# Flagship Product & Autonomous Operating Spec + +Status: **active** +Owner: PatrickSys +Scope: `codebase-context` as a measurable repository-context system for AI-assisted code review and coding agents +Primary execution plan: `.planning/ROADMAP.md` + +## 1. Mission + +Build a public, local-first, vendor-neutral context system that can answer one hard question with evidence: + +> Does bounded repository context materially improve an AI reviewer's ability to understand a change and find real issues, at acceptable precision, latency, and context cost? + +The project is not successful because it has many analyzers, tools, workflows, or agent integrations. It is successful when an external engineer can reproduce the system, inspect its decisions, run the evidence harness, and see honest results. + +## 2. Product Wedge + +The flagship wedge is **review context compilation**. + +Given a clean committed git change, compile a bounded packet containing: + +1. exact base/head identity and diff fingerprint +2. changed files and change statistics +3. deterministic signals extracted from changed lines +4. bounded related repository context +5. relevant repository conventions/patterns +6. retrieval/preflight quality metadata +7. explicit warnings and limits + +That packet can then be consumed by any reviewer model or human evaluation harness. + +This separates: + +- **context compilation** from +- **review reasoning** from +- **evaluation** + +That separation is the core architectural principle for the flagship. + +## 3. Current Product Contract + +The current public review surface is `review-context-v1`. + +### Inputs + +- local git repository +- clean checked-out `HEAD` +- base git ref +- optional explicit head ref that must resolve to checked-out `HEAD` +- existing or refreshable `codebase-context` index +- explicit query/result/identifier limits + +### Outputs + +A versioned JSON packet containing: + +- exact resolved base/head SHAs +- exact git diff SHA-256 +- changed file descriptors +- addition/deletion counts +- rename/binary metadata +- bounded identifiers derived from changed lines +- bounded retrieval queries +- bounded related search results +- search quality / edit preflight metadata when available +- convention/pattern snapshot +- warnings +- all operative limits + +### Invariants + +The review path MUST: + +- use exact resolved commits for diff generation +- fail closed if the working tree is dirty +- fail closed if requested head is not checked-out `HEAD` +- parse git paths safely, including unusual valid filenames +- never silently truncate the diff used for fingerprinting +- bound queries, results, identifiers, and snippets +- never call an LLM during context compilation +- never post review comments +- never represent retrieval confidence as code correctness +- preserve failures/warnings rather than papering over them + +## 4. What This Product Is Not + +Until evidence passes the relevant gates, this project is **not**: + +- a proven AI reviewer +- a replacement for CodeRabbit, Greptile, Copilot review, or similar products +- a claim that semantic/vector retrieval improves review quality +- a cloud platform +- an autonomous pull-request commenter +- a dashboard product +- a framework-specific reviewer +- a place to port abandoned private experiments + +`context-kit` is not an authority, benchmark, source of claims, or required dependency. Its historical existence does not validate any design here. + +## 5. Evidence Levels + +Every public statement must map to one of these levels. + +### E0 — Code exists + +Evidence: + +- implementation present +- unit tests present + +Allowed claim: + +- "The project implements X." + +Not allowed: + +- "X works better" +- "X improves review" +- "X is efficient" + +### E1 — Reproducible functional behavior + +Evidence: + +- build passes +- full tests pass +- executable smoke passes +- deterministic envelope invariants verified +- public reproduction instructions exist + +Allowed claim: + +- "The project can compile a bounded review-context packet from a committed git range." + +### E2 — Retrieval value + +Evidence: + +- frozen public task set before algorithm tuning +- multiple repositories +- multiple languages/frameworks +- fixed limits +- baseline and treatment run under identical scoring +- relevant-file/span retrieval metrics reported with failures + +Allowed claim: + +- bounded claims about retrieval performance on the frozen evaluation only + +### E3 — Reviewer impact + +Evidence: + +- same reviewer model/harness +- same prompt/budget/timeout +- raw-diff/tool baseline vs review-context treatment +- repeated runs where model nondeterminism matters +- finding recall, precision/false positives, evidence quality, token/context cost, wall time reported separately +- no best-of-N selection + +Allowed claim: + +- bounded claim that review-context changed measured reviewer performance on the frozen evaluation + +### E4 — External usefulness + +Evidence can include: + +- external users +- independent reproduction +- maintainer adoption +- accepted integrations +- third-party benchmark replication + +Only at E4 should broad product-positioning claims become credible. + +## 6. Core Hypotheses + +### H1 — Changed-line signals can cheaply focus retrieval + +A small deterministic signal extractor can produce useful queries from a diff without an LLM. + +Kill/fix trigger: + +- relevant-file/span coverage is materially worse than simpler path/symbol baselines + +### H2 — Bounded repository context can outperform raw diff/repository navigation alone + +Adding a bounded `review-context-v1` packet improves review outcomes without unacceptable false positives or context cost. + +Kill/fix trigger: + +- no meaningful improvement across a frozen multi-repo review set +- improvement is explained only by substantially larger token budgets +- gains disappear outside the development repositories + +### H3 — The existing `codebase-context` retrieval stack is good enough to support the wedge + +The current search/indexing system can supply relevant review context reliably enough to justify reuse. + +Kill/fix trigger: + +- retrieval remains the dominant failure mode after a frozen diagnostic evaluation +- simpler baselines match or beat it at lower setup/runtime cost + +If H3 fails, do not build more reviewer orchestration. Fix, simplify, or replace the retrieval lane first. + +## 7. Architecture + +```text +Git repository + | + v +Exact committed change envelope +(base/head SHAs, clean tree, exact diff hash) + | + v +Deterministic change-signal extraction +(files, stats, identifiers, path fallbacks) + | + v +Bounded query planner +(max queries / identifiers) + | + v +Repository retrieval +(codebase-context search + quality/preflight metadata) + | + +--> conventions / patterns + | + v +review-context-v1 packet + | + +--> human inspection + +--> frozen evaluation harness + +--> model-agnostic reviewer consumer (only after gate) +``` + +### Architectural boundaries + +- Git/diff mechanics must remain independent of model vendors. +- Review-context packet construction must remain usable without an LLM. +- Evaluation must be able to swap reviewer models without rewriting context compilation. +- Framework-specific logic stays in analyzers. +- Core/shared types stay framework-neutral. +- No cloud infrastructure is required for the default product path. + +## 8. Metrics + +Do not collapse metrics into one vanity score. + +### Retrieval metrics + +At minimum: + +- relevant file recall@k +- relevant span recall@k when ground truth supports it +- precision@k +- context characters/tokens returned +- setup/index time +- retrieval wall time +- failure/abstention rate + +### Review metrics + +At minimum: + +- real finding recall +- false positives / precision +- evidence correctness +- unsupported-claim rate +- reviewer token/context usage +- wall time +- tool/runtime failure rate + +### Operational metrics + +Track when meaningful: + +- cold install size +- cold index time +- incremental refresh time +- peak memory +- packet size + +Performance claims require recorded measurements, not intuition. + +## 9. Evaluation Integrity + +The rules in root `AGENTS.md` remain binding. This spec adds the following review-specific requirements. + +### Freeze before tuning + +Before changing retrieval/ranking for a claim-bearing evaluation: + +1. freeze task IDs +2. freeze repositories and commits +3. freeze ground truth / scorer +4. freeze budgets and limits +5. commit the manifest +6. only then inspect treatment results + +### Required baselines + +Use the cheapest meaningful baselines first: + +1. raw diff only +2. raw diff + repository-native/basic text tools +3. simple changed-file/path/symbol context +4. current `codebase-context` retrieval packet + +Do not claim value against an intentionally weak baseline. + +### Failure accounting + +A setup failure, index failure, timeout, invalid output, missing evidence, or judge failure remains a failure row. It is never silently removed from the denominator. + +## 10. Autonomous Agent Operating Model + +An agent may manage routine progress without asking for permission for every implementation detail, but it MUST follow this state machine. + +### State A — Observe + +Read: + +1. `AGENTS.md` +2. `.planning/SPEC.md` +3. `.planning/ROADMAP.md` +4. relevant code/tests/docs +5. current PR/CI state + +Do not code before identifying the current open gate. + +### State B — Classify new information + +Every new input is one of: + +- **blocker** — CI/test/security/IP/reproducibility failure +- **evidence** — benchmark or external usage result +- **bug** — shipped behavior violates contract +- **hypothesis signal** — suggests a current hypothesis may be right/wrong +- **opportunity** — integration/user/recruiter/maintainer pull +- **idea** — unvalidated feature suggestion +- **noise** — does not materially affect current gate + +Only blockers, evidence, bugs, and strong external opportunities can pre-empt the current milestone. + +### State C — Route + +Priority order: + +1. protect privacy/IP/security/reproducibility +2. restore green build/tests +3. satisfy the current roadmap gate +4. collect evidence +5. package/document proven behavior +6. only then expand capability + +A new idea never outranks a failing gate. + +### State D — Change + +Rules: + +- one primary hypothesis or defect per PR +- smallest change capable of crossing the current gate +- prefer deleting/simplifying over layering heuristics +- no hidden benchmark-specific branches +- no fixture edits to make output pass +- no broad refactors while evidence is blocked + +### State E — Verify + +Minimum before merge: + +- lint/format +- type-check +- build +- full relevant test suite +- executable smoke for new public binaries +- docs match behavior +- no new unsupported public claim + +Evidence-changing work also requires the relevant frozen evaluation. + +### State F — Decide + +For each milestone, the agent must choose exactly one: + +- **PASS** — gate met; advance +- **FIX** — hypothesis still plausible; one bounded repair cycle justified +- **KILL** — evidence says this lane is not worth further work +- **BLOCKED** — external dependency prevents honest conclusion + +Do not use "mostly done" as a state. + +### State G — Record + +After a gate decision: + +- update `.planning/ROADMAP.md` +- update claim-bearing docs only if evidence level changed +- record material limitations +- preserve benchmark artifacts/manifests needed for reproduction + +## 11. Agent Authority + +### May do autonomously + +- create branches and PRs +- fix CI failures in current scope +- add tests for current contract +- harden parsing/error handling/reproducibility +- simplify implementation +- add frozen evaluation fixtures before implementation +- run public benchmark/eval infrastructure +- update docs to become more conservative/accurate +- revert its own failing changes +- close/kill a roadmap lane when predefined kill criteria are met + +### Must not do autonomously + +- publish employer/private code or data +- import private `context-kit` history as validation +- publish broad performance/superiority claims without evidence gate +- add paid/cloud dependencies as required defaults +- weaken frozen ground truth after seeing results +- silently increase treatment budgets relative to baseline +- add an autonomous PR-commenting bot before reviewer-impact evidence passes +- create a dashboard merely to make the project look productized +- change project licensing or ownership policy +- spend money or enable paid external services without explicit owner approval + +### Escalate to owner when + +- evidence is genuinely ambiguous after one bounded repair cycle +- a decision requires money or credentials +- a privacy/IP boundary is unclear +- two viable architectures have materially different long-term product directions +- external opportunity requires a commitment or public statement + +## 12. Scope Control + +### Track A — Current + +Only work needed to pass the next open gate in `.planning/ROADMAP.md`. + +### Track B — Backlog + +Potential future work that cannot pre-empt Track A without new evidence. + +Examples: + +- hosted dashboard +- GitHub App +- Azure DevOps adapter +- autonomous reviewer comments +- queue/worker cloud architecture +- large framework-specific rule packs +- general code-review agent orchestration + +Track B items become Track A only when the roadmap explicitly promotes them. + +## 13. Product Kill Rules + +Stop expanding the review-context wedge if any of these holds after a fair frozen evaluation and one bounded repair cycle: + +- simple baselines equal or beat it at materially lower complexity/cost +- relevant-context precision is too low to be useful +- reviewer false positives rise enough to negate recall gains +- benefits require dramatically larger token/context budgets +- gains do not generalize across repositories/languages +- setup/index cost makes realistic use unattractive + +A killed hypothesis is a successful engineering result if the evidence is credible and published honestly. + +## 14. Definition of Publishable + +A milestone is publishable only when a stranger can: + +1. understand the problem from public docs +2. install/build the project +3. run the relevant command +4. inspect the versioned output contract +5. run the evidence harness or reproduce the stated functional behavior +6. see limitations and failures +7. distinguish shipped capability from experimental hypothesis + +## 15. Current Strategic Decision + +Do **not** build a second overlapping project. + +Use `codebase-context` as the public flagship substrate. Prove or kill the review-context wedge inside it. If the existing retrieval engine fails the evidence gates, simplify or replace that layer rather than inventing another repository. diff --git a/docs/review-context.md b/docs/review-context.md new file mode 100644 index 0000000..e35df6a --- /dev/null +++ b/docs/review-context.md @@ -0,0 +1,125 @@ +# Review context + +`codebase-context-review` turns a committed git diff into a bounded review-context packet. + +It is deliberately **not** an AI reviewer. It does not call an LLM, decide whether code is correct, or post comments. Its job is narrower and testable: compile the changed surface, stable diff signals, related codebase context, and current conventions into a versioned input for a reviewer or evaluation harness. + +## Why this exists + +A PR reviewer needs more than the patch, but dumping an entire repository into a model is expensive and hard to reproduce. The review-context path creates a clean boundary between: + +1. **context compilation** — git + local repository analysis +2. **review reasoning** — any model or human reviewer consuming the packet +3. **evaluation** — measuring whether the context actually helps find the right files/spans/issues + +Keeping those layers separate makes failures diagnosable. A bad review can be attributed to missing context, bad reasoning, or both instead of hiding every variable behind one agent loop. + +## Usage + +Build and run from the repository: + +```bash +pnpm build +node dist/review-bin.js --base origin/main --head HEAD +``` + +After package publication, the package also exposes: + +```bash +npx codebase-context-review --base origin/main --head HEAD +``` + +Use `--json` for the complete machine-readable packet: + +```bash +npx codebase-context-review \ + --base origin/main \ + --head HEAD \ + --max-queries 8 \ + --max-results 3 \ + --json > review-context.json +``` + +The command uses merge-base diff semantics (`base...head`), which matches the normal pull-request question: what changed on this branch since it diverged from the base branch? + +`review-context-v1` requires `--head` to resolve to the checked-out `HEAD` and requires a clean working tree. The diff itself is then generated from the already-resolved commit SHAs rather than mutable symbolic refs. + +## Packet contract + +The current schema is `review-context-v1`. + +The packet contains: + +- exact resolved base/head commit SHAs +- SHA-256 fingerprint of Git's exact raw diff output +- changed file status, additions/deletions, rename source, and binary flag +- NUL-safe filename parsing for unusual valid git paths +- identifiers extracted deterministically from changed lines +- bounded search queries derived from those diff signals +- bounded related-context results from the existing `search_codebase` engine +- edit-preflight/search-quality metadata when the search engine provides it +- current team-pattern/convention output +- explicit warnings when context could not be produced + +Absolute local repository paths are not part of the packet contract. + +The git envelope, identifier extraction, bounds, and query derivation are deterministic. Related-context ranking is only reproducible under the same `codebase-context` version, index contents, embedding/reranking configuration, and runtime dependencies; benchmark manifests should freeze those inputs rather than pretending the entire retrieval stack is environment-independent. + +## Bounds + +Defaults: + +| Bound | Default | +| --- | ---: | +| Changed-file search queries | 8 | +| Related results per query | 3 | +| Identifier candidates per file | 10 | +| Snippet characters per related result | 1,200 | + +These are explicit because an unbounded context compiler is not useful evidence. Increasing them is allowed, but benchmark runs should record the values and compare like-for-like. + +## Index behavior + +By default, the command creates an index when one is missing and runs an incremental refresh when one already exists. This keeps normal use aligned with the checked-out clean `HEAD`. + +Pass `--no-index` to prohibit both creation and refresh. It fails if no index exists. This is useful in controlled benchmark runs where setup/index work is captured separately; the caller is then responsible for proving that the supplied index matches the frozen source state. + +The command is local-first. It invokes git and the existing local index/search pipeline; it does not introduce an LLM or external review API. + +## What this proves + +Shipping this command proves only that the project can compile a bounded, versioned review-oriented context packet from an exact committed git range. + +It **does not** prove that the packet improves review quality, catches more bugs, reduces false positives, or beats another context strategy. Those are benchmark claims and remain blocked until measured. + +## Evaluation path + +The intended evaluation is an ablation, not a marketing benchmark: + +1. freeze a set of public PR/bug-fix tasks before observing lane outputs +2. run the same reviewer/model with **raw diff only** +3. run the same reviewer/model with **raw diff + `review-context-v1` packet** +4. keep model, prompt, turn/token budget, timeout, and scoring fixed +5. score bug/finding recall and precision, evidence quality, false positives, token cost, and wall time separately +6. report setup/index failures as failures, not as competitor losses +7. do not publish a superiority claim from a one-task pilot + +The existing ContextBench protocol already follows the same evidence discipline for repository-context retrieval. Review-specific claims should meet at least the same standard rather than creating a weaker parallel benchmark. + +## Current limitations + +- Only committed git refs are supported in v1; working-tree/staged review is intentionally deferred. +- `--head` must be the checked-out commit and the worktree must be clean. +- Query generation is lexical and deterministic. It extracts identifiers from changed lines and falls back to path signals; it is not AST-aware yet. +- `--no-index` deliberately skips freshness work, so benchmark callers must attest the index/source match themselves. +- Large diffs are bounded by the CLI git-buffer limit and fail rather than silently truncating the raw fingerprint input. +- Binary files are recorded but do not generate identifier-based queries. +- The packet is context, not a verdict. A consumer should never turn `preflight.ready` into "the change is correct." + +## Next gate + +Do not add reviewer-agent features until a small frozen public evaluation can answer this first question: + +> Does `review-context-v1` improve relevant-file/span coverage at an acceptable precision and context cost compared with the same reviewer using the raw diff/repository tools alone? + +If the answer is no, fix or kill this lane before adding more orchestration. diff --git a/package.json b/package.json index a1fb5a0..2b75047 100644 --- a/package.json +++ b/package.json @@ -44,7 +44,8 @@ } }, "bin": { - "codebase-context": "./dist/index.js" + "codebase-context": "./dist/index.js", + "codebase-context-review": "./dist/review-bin.js" }, "files": [ "dist", @@ -54,6 +55,7 @@ "docs/cli.md", "docs/capabilities.md", "docs/client-setup.md", + "docs/review-context.md", "templates" ], "packageManager": "pnpm@10.27.0", diff --git a/src/review-bin.ts b/src/review-bin.ts new file mode 100644 index 0000000..1591998 --- /dev/null +++ b/src/review-bin.ts @@ -0,0 +1,8 @@ +#!/usr/bin/env node + +import { runReviewCli } from './review-cli.js'; + +runReviewCli(process.argv.slice(2)).catch((error) => { + console.error(error instanceof Error ? error.message : String(error)); + process.exitCode = 1; +}); diff --git a/src/review-cli.ts b/src/review-cli.ts new file mode 100644 index 0000000..e4e08f7 --- /dev/null +++ b/src/review-cli.ts @@ -0,0 +1,401 @@ +#!/usr/bin/env node + +import { execFileSync } from 'node:child_process'; +import { promises as fs } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { AngularAnalyzer } from './analyzers/angular/index.js'; +import { GenericAnalyzer } from './analyzers/generic/index.js'; +import { NestJsAnalyzer } from './analyzers/nestjs/index.js'; +import { NextJsAnalyzer } from './analyzers/nextjs/index.js'; +import { ReactAnalyzer } from './analyzers/react/index.js'; +import { + CODEBASE_CONTEXT_DIRNAME, + HEALTH_FILENAME, + INTELLIGENCE_FILENAME, + KEYWORD_INDEX_FILENAME, + MEMORY_FILENAME, + VECTOR_DB_DIRNAME +} from './constants/codebase-context.js'; +import { analyzerRegistry } from './core/analyzer-registry.js'; +import { CodebaseIndexer } from './core/indexer.js'; +import { buildReviewContextPacket, parseNameStatus } from './review-context.js'; +import { loadProjectConfig } from './server/config.js'; +import { dispatchTool } from './tools/index.js'; +import type { PatternResponse, SearchResponse, ToolContext, ToolResponse } from './tools/types.js'; + +analyzerRegistry.register(new AngularAnalyzer()); +analyzerRegistry.register(new NextJsAnalyzer()); +analyzerRegistry.register(new NestJsAnalyzer()); +analyzerRegistry.register(new ReactAnalyzer()); +analyzerRegistry.register(new GenericAnalyzer()); + +interface ReviewCliOptions { + base: string; + head: string; + rootPath: string; + maxQueries: number; + maxResults: number; + maxIdentifiers: number; + json: boolean; + noIndex: boolean; +} + +const DEFAULT_MAX_QUERIES = 8; +const DEFAULT_MAX_RESULTS = 3; +const DEFAULT_MAX_IDENTIFIERS = 10; +const MAX_GIT_BUFFER = 16 * 1024 * 1024; + +function printUsage(): void { + console.log(`codebase-context-review --base [options] + +Build a bounded review-context packet from a committed git diff. +It does not call an LLM and it does not post review comments. + +Required: + --base PR/base ref. Compared with merge-base semantics. + +Options: + --head Head ref; must resolve to checked-out HEAD (default: HEAD) + --root Repository root (default: CODEBASE_ROOT or cwd) + --max-queries Maximum changed-file search queries (default: 8) + --max-results Maximum related results per query (default: 3) + --max-identifiers Identifier candidates retained per file (default: 10) + --no-index Do not create/refresh the index; fail if it is missing + --json Emit the complete JSON packet + --help Show this help + +The v1 command requires a clean working tree so repository search and the git diff +refer to the same committed source state. + +Example: + codebase-context-review --base origin/main --head HEAD --json +`); +} + +function parsePositiveInt(value: string | undefined, flag: string, fallback: number): number { + if (value === undefined) return fallback; + const parsed = Number(value); + if (!Number.isInteger(parsed) || parsed <= 0) { + throw new Error(`${flag} must be a positive integer`); + } + return parsed; +} + +function parseArgs(argv: string[]): ReviewCliOptions | undefined { + if (argv.includes('--help') || argv.includes('-h')) { + printUsage(); + return undefined; + } + + const values = new Map(); + const booleans = new Set(); + const valueFlags = new Set([ + '--base', + '--head', + '--root', + '--max-queries', + '--max-results', + '--max-identifiers' + ]); + const booleanFlags = new Set(['--json', '--no-index']); + + for (let index = 0; index < argv.length; index += 1) { + const arg = argv[index]; + if (booleanFlags.has(arg)) { + booleans.add(arg); + continue; + } + if (!valueFlags.has(arg)) { + throw new Error(`Unknown argument: ${arg}`); + } + const value = argv[index + 1]; + if (!value || value.startsWith('--')) { + throw new Error(`${arg} requires a value`); + } + values.set(arg, value); + index += 1; + } + + const base = values.get('--base'); + if (!base) throw new Error('--base is required'); + + return { + base, + head: values.get('--head') ?? 'HEAD', + rootPath: path.resolve(values.get('--root') ?? process.env.CODEBASE_ROOT ?? process.cwd()), + maxQueries: parsePositiveInt(values.get('--max-queries'), '--max-queries', DEFAULT_MAX_QUERIES), + maxResults: parsePositiveInt(values.get('--max-results'), '--max-results', DEFAULT_MAX_RESULTS), + maxIdentifiers: parsePositiveInt( + values.get('--max-identifiers'), + '--max-identifiers', + DEFAULT_MAX_IDENTIFIERS + ), + json: booleans.has('--json'), + noIndex: booleans.has('--no-index') + }; +} + +function runGit(rootPath: string, args: string[], maxBuffer = MAX_GIT_BUFFER): string { + try { + return execFileSync('git', ['-c', 'core.quotepath=false', ...args], { + cwd: rootPath, + encoding: 'utf8', + input: '', + stdio: ['pipe', 'pipe', 'pipe'], + maxBuffer + }); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + throw new Error(`git ${args.join(' ')} failed: ${message}`); + } +} + +function resolveCommit(rootPath: string, ref: string): string { + const commit = runGit(rootPath, ['rev-parse', '--verify', `${ref}^{commit}`]).trim(); + if (!/^[0-9a-f]{40}$/i.test(commit)) { + throw new Error(`Could not resolve ${ref} to a commit`); + } + return commit; +} + +function assertReviewSourceState(rootPath: string, headCommit: string): void { + const checkoutHead = resolveCommit(rootPath, 'HEAD'); + if (checkoutHead !== headCommit) { + throw new Error( + `--head must resolve to the checked-out HEAD in review-context-v1 (${checkoutHead.slice(0, 12)} != ${headCommit.slice(0, 12)})` + ); + } + + const dirty = runGit(rootPath, ['status', '--porcelain=v1', '--untracked-files=all']); + if (dirty.length > 0) { + throw new Error( + 'review-context-v1 requires a clean working tree so the repository index and diff describe the same committed source state' + ); + } +} + +async function createToolContext(rootPath: string, noIndex: boolean): Promise { + const projectConfig = await loadProjectConfig(rootPath); + const paths = { + baseDir: path.join(rootPath, CODEBASE_CONTEXT_DIRNAME), + memory: path.join(rootPath, CODEBASE_CONTEXT_DIRNAME, MEMORY_FILENAME), + intelligence: path.join(rootPath, CODEBASE_CONTEXT_DIRNAME, INTELLIGENCE_FILENAME), + health: path.join(rootPath, CODEBASE_CONTEXT_DIRNAME, HEALTH_FILENAME), + keywordIndex: path.join(rootPath, CODEBASE_CONTEXT_DIRNAME, KEYWORD_INDEX_FILENAME), + vectorDb: path.join(rootPath, CODEBASE_CONTEXT_DIRNAME, VECTOR_DB_DIRNAME) + }; + + let indexExists = false; + try { + await fs.access(paths.keywordIndex); + indexExists = true; + } catch { + indexExists = false; + } + + const indexState: ToolContext['indexState'] = { + status: indexExists ? 'ready' : 'idle' + }; + + const performIndexing = async (incrementalOnly?: boolean, reason?: string): Promise => { + indexState.status = 'indexing'; + console.error( + `Indexing review target (${incrementalOnly ? 'incremental' : 'full'})${reason ? ` — ${reason}` : ''}` + ); + + try { + const indexer = new CodebaseIndexer({ + rootPath, + ...(projectConfig?.parsing ? { config: { parsing: projectConfig.parsing } } : {}), + incrementalOnly + }); + indexState.indexer = indexer; + const stats = await indexer.index(); + indexState.status = 'ready'; + indexState.lastIndexed = new Date(); + indexState.stats = stats; + } catch (error) { + indexState.status = 'error'; + indexState.error = error instanceof Error ? error.message : String(error); + throw error; + } + }; + + const context: ToolContext = { indexState, paths, rootPath, performIndexing }; + + if (!indexExists) { + if (noIndex) { + throw new Error( + 'No codebase-context index exists. Run `codebase-context reindex` or omit --no-index.' + ); + } + await performIndexing(false, 'review-context'); + } else if (!noIndex) { + await performIndexing(true, 'review-context-refresh'); + } + + return context; +} + +function parseToolJson(response: ToolResponse, operation: string): T { + const text = response.content?.find((item) => item.type === 'text')?.text; + if (!text) throw new Error(`${operation} returned no JSON payload`); + + let parsed: unknown; + try { + parsed = JSON.parse(text); + } catch { + throw new Error(`${operation} returned invalid JSON`); + } + + if (response.isError) { + const message = + typeof parsed === 'object' && parsed !== null && 'error' in parsed + ? String((parsed as { error: unknown }).error) + : `${operation} failed`; + throw new Error(message); + } + return parsed as T; +} + +function printHuman(packet: Awaited>): void { + console.log(`Review context: ${packet.refs.base}...${packet.refs.head}`); + console.log( + `Commits: ${packet.refs.baseCommit.slice(0, 12)} -> ${packet.refs.headCommit.slice(0, 12)}` + ); + console.log(`Diff: sha256:${packet.diffSha256}`); + console.log( + `Changed: ${packet.summary.filesChanged} files, +${packet.summary.additions}/-${packet.summary.deletions}` + ); + console.log( + `Searches: ${packet.summary.queryCount}, related results: ${packet.summary.relatedResultCount}` + ); + console.log(''); + + for (const file of packet.changedFiles) { + const rename = file.previousPath ? ` (from ${file.previousPath})` : ''; + const identifiers = file.identifiers.slice(0, 5).join(', '); + console.log( + `${file.rawStatus.padEnd(4)} ${file.path}${rename} +${file.additions}/-${file.deletions}${file.binary ? ' [binary]' : ''}` + ); + if (identifiers) console.log(` signals: ${identifiers}`); + } + + if (packet.searches.length > 0) { + console.log('\nRelated context'); + for (const search of packet.searches) { + console.log(`- ${search.query} <- ${search.sourceFiles.join(', ')}`); + if (search.error) { + console.log(` error: ${search.error}`); + continue; + } + for (const result of search.results) { + console.log(` ${result.file} score=${result.score.toFixed(3)} ${result.summary}`); + } + } + } + + if (packet.warnings.length > 0) { + console.log('\nWarnings'); + for (const warning of packet.warnings) console.log(`- ${warning}`); + } + + console.log('\nUse --json for the machine-readable packet including conventions and snippets.'); +} + +export async function runReviewCli(argv: string[]): Promise { + const options = parseArgs(argv); + if (!options) return 0; + + const gitRoot = runGit(options.rootPath, ['rev-parse', '--show-toplevel']).trim(); + const rootPath = path.resolve(gitRoot); + const baseCommit = resolveCommit(rootPath, options.base); + const headCommit = resolveCommit(rootPath, options.head); + assertReviewSourceState(rootPath, headCommit); + + const range = `${baseCommit}...${headCommit}`; + const nameStatus = runGit(rootPath, [ + 'diff', + '--name-status', + '-z', + '--find-renames', + range, + '--' + ]); + const changedFiles = parseNameStatus(nameStatus); + const rawDiff = runGit(rootPath, [ + 'diff', + '--no-ext-diff', + '--binary', + '--no-color', + '--find-renames', + range, + '--' + ]); + + const patchesByPath = new Map(); + for (const file of changedFiles) { + const patch = runGit(rootPath, [ + 'diff', + '--no-ext-diff', + '--unified=0', + '--no-color', + '--find-renames', + range, + '--', + file.path + ]); + patchesByPath.set(file.path, patch); + } + + const toolContext = await createToolContext(rootPath, options.noIndex); + const packet = await buildReviewContextPacket({ + base: options.base, + head: options.head, + baseCommit, + headCommit, + rawDiff, + changedFiles, + patchesByPath, + maxQueries: options.maxQueries, + maxResultsPerQuery: options.maxResults, + maxIdentifiersPerFile: options.maxIdentifiers, + search: async (query, limit) => + parseToolJson( + await dispatchTool( + 'search_codebase', + { query, includeSnippets: true, intent: 'edit', limit }, + toolContext + ), + `search_codebase(${query})` + ), + loadConventions: async () => + parseToolJson( + await dispatchTool('get_team_patterns', { category: 'all' }, toolContext), + 'get_team_patterns' + ) + }); + + if (options.json) console.log(JSON.stringify(packet, null, 2)); + else printHuman(packet); + + return 0; +} + +const isDirectRun = (() => { + const entry = process.argv[1]; + if (!entry) return false; + try { + return path.resolve(entry) === path.resolve(fileURLToPath(import.meta.url)); + } catch { + return entry.replace(/\\/g, '/').endsWith('/review-cli.js'); + } +})(); + +if (isDirectRun) { + runReviewCli(process.argv.slice(2)).catch((error) => { + console.error(error instanceof Error ? error.message : String(error)); + process.exitCode = 1; + }); +} diff --git a/src/review-context.ts b/src/review-context.ts new file mode 100644 index 0000000..f3b76a7 --- /dev/null +++ b/src/review-context.ts @@ -0,0 +1,452 @@ +import { createHash } from 'node:crypto'; +import path from 'node:path'; +import type { + PatternResponse, + SearchQuality, + SearchResponse, + SearchResultItem +} from './tools/types.js'; + +const IDENTIFIER_RE = /[A-Za-z_$][A-Za-z0-9_$]*/g; + +const IDENTIFIER_STOPWORDS = new Set([ + 'abstract', + 'any', + 'array', + 'async', + 'await', + 'boolean', + 'break', + 'case', + 'catch', + 'class', + 'const', + 'continue', + 'debugger', + 'declare', + 'default', + 'delete', + 'else', + 'enum', + 'export', + 'extends', + 'false', + 'finally', + 'for', + 'from', + 'func', + 'function', + 'implements', + 'import', + 'interface', + 'let', + 'map', + 'namespace', + 'never', + 'new', + 'null', + 'number', + 'object', + 'package', + 'private', + 'protected', + 'public', + 'readonly', + 'return', + 'self', + 'static', + 'string', + 'struct', + 'super', + 'switch', + 'this', + 'throw', + 'true', + 'try', + 'type', + 'typeof', + 'undefined', + 'unknown', + 'using', + 'var', + 'void', + 'while', + 'with', + 'yield' +]); + +export interface ChangedFileDescriptor { + path: string; + previousPath?: string; + status: string; + rawStatus: string; +} + +export interface ChangedFileContext extends ChangedFileDescriptor { + additions: number; + deletions: number; + binary: boolean; + identifiers: string[]; +} + +export interface ReviewQuery { + query: string; + sourceFiles: string[]; + identifiers: string[]; +} + +export interface ReviewRelatedResult { + file: string; + summary: string; + score: number; + relevanceReason?: string; + type?: string; + trend?: 'Rising' | 'Declining'; + patternWarning?: string; + symbol?: string; + scope?: string; + signaturePreview?: string; + snippet?: string; + importedByCount?: number; + hasTests?: boolean; +} + +export interface ReviewSearchResult { + query: string; + sourceFiles: string[]; + searchQuality?: SearchQuality; + preflight?: SearchResponse['preflight']; + results: ReviewRelatedResult[]; + error?: string; +} + +export interface ReviewContextPacket { + schemaVersion: 'review-context-v1'; + refs: { + base: string; + head: string; + baseCommit: string; + headCommit: string; + range: string; + }; + diffSha256: string; + limits: { + maxQueries: number; + maxResultsPerQuery: number; + maxIdentifiersPerFile: number; + }; + summary: { + filesChanged: number; + additions: number; + deletions: number; + queryCount: number; + relatedResultCount: number; + }; + changedFiles: ChangedFileContext[]; + searches: ReviewSearchResult[]; + conventions?: PatternResponse; + warnings: string[]; +} + +export interface PatchStats { + additions: number; + deletions: number; + binary: boolean; +} + +export interface BuildReviewContextOptions { + base: string; + head: string; + baseCommit: string; + headCommit: string; + rawDiff: string; + changedFiles: ChangedFileDescriptor[]; + patchesByPath: ReadonlyMap; + maxQueries?: number; + maxResultsPerQuery?: number; + maxIdentifiersPerFile?: number; + search: (query: string, limit: number) => Promise; + loadConventions?: () => Promise; +} + +function descriptorFromStatus(rawStatus: string, filePath: string): ChangedFileDescriptor { + return { + path: filePath, + status: rawStatus.charAt(0), + rawStatus + }; +} + +export function parseNameStatus(output: string): ChangedFileDescriptor[] { + if (output.includes('\0')) { + const fields = output.split('\0'); + const files: ChangedFileDescriptor[] = []; + let index = 0; + + while (index < fields.length) { + const rawStatus = fields[index++]; + if (!rawStatus) continue; + + const status = rawStatus.charAt(0); + if (status === 'R' || status === 'C') { + const previousPath = fields[index++]; + const currentPath = fields[index++]; + if (previousPath && currentPath) { + files.push({ path: currentPath, previousPath, status, rawStatus }); + } + continue; + } + + const filePath = fields[index++]; + if (filePath) files.push(descriptorFromStatus(rawStatus, filePath)); + } + + return files; + } + + const files: ChangedFileDescriptor[] = []; + for (const rawLine of output.split(/\r?\n/)) { + const line = rawLine.trim(); + if (!line) continue; + + const parts = line.split('\t'); + const rawStatus = parts[0] ?? ''; + const status = rawStatus.charAt(0); + if (!status) continue; + + if ((status === 'R' || status === 'C') && parts.length >= 3) { + const previousPath = parts[1]; + const currentPath = parts[2]; + if (previousPath && currentPath) { + files.push({ path: currentPath, previousPath, status, rawStatus }); + } + continue; + } + + const filePath = parts[1]; + if (filePath) files.push(descriptorFromStatus(rawStatus, filePath)); + } + + return files; +} + +export function inspectPatch(patch: string): PatchStats { + let additions = 0; + let deletions = 0; + let binary = false; + + for (const line of patch.split(/\r?\n/)) { + if (line.startsWith('Binary files ') || line === 'GIT binary patch') { + binary = true; + continue; + } + if (line.startsWith('+++') || line.startsWith('---')) continue; + if (line.startsWith('+')) additions += 1; + else if (line.startsWith('-')) deletions += 1; + } + + return { additions, deletions, binary }; +} + +export function extractChangedIdentifiers(patch: string, limit = 10): string[] { + const scores = new Map(); + + for (const line of patch.split(/\r?\n/)) { + const isAddition = line.startsWith('+') && !line.startsWith('+++'); + const isDeletion = line.startsWith('-') && !line.startsWith('---'); + if (!isAddition && !isDeletion) continue; + + for (const match of line.slice(1).matchAll(IDENTIFIER_RE)) { + const identifier = match[0]; + if (identifier.length < 3) continue; + if (IDENTIFIER_STOPWORDS.has(identifier.toLowerCase())) continue; + + let score = isAddition ? 3 : 2; + if (/^[A-Z][A-Za-z0-9_$]+$/.test(identifier)) score += 2; + if (/[a-z][A-Z]/.test(identifier)) score += 1; + if (identifier.length >= 8) score += 1; + + scores.set(identifier, (scores.get(identifier) ?? 0) + score); + } + } + + return Array.from(scores.entries()) + .sort(([nameA, scoreA], [nameB, scoreB]) => scoreB - scoreA || nameA.localeCompare(nameB)) + .slice(0, Math.max(0, limit)) + .map(([identifier]) => identifier); +} + +function fallbackIdentifiersForPath(filePath: string): string[] { + const withoutExtension = filePath.replace(/\.[^./]+$/, ''); + const basename = path.basename(withoutExtension); + const parentSegments = path + .dirname(withoutExtension) + .split(/[\\/._-]+/) + .filter(Boolean) + .reverse(); + const ordered = [basename, ...parentSegments]; + const seen = new Set(); + + return ordered.filter((part) => { + const normalized = part.toLowerCase(); + if (part.length < 3 || IDENTIFIER_STOPWORDS.has(normalized) || seen.has(normalized)) { + return false; + } + seen.add(normalized); + return true; + }); +} + +export function buildReviewQueries(files: ChangedFileContext[], maxQueries = 8): ReviewQuery[] { + const selected = [...files] + .sort((a, b) => { + const identifierDelta = b.identifiers.length - a.identifiers.length; + if (identifierDelta !== 0) return identifierDelta; + + const changeDelta = b.additions + b.deletions - (a.additions + a.deletions); + if (changeDelta !== 0) return changeDelta; + + return a.path.localeCompare(b.path); + }) + .slice(0, Math.max(0, maxQueries)); + + const queries: ReviewQuery[] = []; + const seen = new Set(); + + for (const file of selected) { + const identifiers = ( + file.identifiers.length > 0 ? file.identifiers : fallbackIdentifiersForPath(file.path) + ).slice(0, 3); + if (identifiers.length === 0) continue; + + const query = identifiers.join(' '); + const key = query.toLowerCase(); + const existing = queries.find((entry) => entry.query.toLowerCase() === key); + if (existing) { + if (!existing.sourceFiles.includes(file.path)) existing.sourceFiles.push(file.path); + continue; + } + if (seen.has(key)) continue; + + seen.add(key); + queries.push({ query, sourceFiles: [file.path], identifiers }); + } + + return queries; +} + +export function fingerprintDiff(rawDiff: string): string { + return createHash('sha256').update(rawDiff, 'utf8').digest('hex'); +} + +function compactSearchResult(result: SearchResultItem): ReviewRelatedResult { + const importedByCount = result.relationships?.importedByCount ?? result.importedByCount; + + return { + file: result.file, + summary: result.summary, + score: result.score, + ...(result.relevanceReason ? { relevanceReason: result.relevanceReason } : {}), + ...(result.type ? { type: result.type } : {}), + ...(result.trend ? { trend: result.trend } : {}), + ...(result.patternWarning ? { patternWarning: result.patternWarning } : {}), + ...(result.symbol ? { symbol: result.symbol } : {}), + ...(result.scope ? { scope: result.scope } : {}), + ...(result.signaturePreview ? { signaturePreview: result.signaturePreview } : {}), + ...(result.snippet ? { snippet: result.snippet.slice(0, 1200) } : {}), + ...(importedByCount != null ? { importedByCount } : {}), + ...(result.relationships?.hasTests != null ? { hasTests: result.relationships.hasTests } : {}) + }; +} + +export async function buildReviewContextPacket( + options: BuildReviewContextOptions +): Promise { + const maxQueries = Math.max(1, Math.floor(options.maxQueries ?? 8)); + const maxResultsPerQuery = Math.max(1, Math.floor(options.maxResultsPerQuery ?? 3)); + const maxIdentifiersPerFile = Math.max(1, Math.floor(options.maxIdentifiersPerFile ?? 10)); + const warnings: string[] = []; + + const changedFiles = options.changedFiles.map((file) => { + const patch = options.patchesByPath.get(file.path) ?? ''; + if (!patch) warnings.push(`No textual patch captured for ${file.path}`); + + const stats = inspectPatch(patch); + return { + ...file, + ...stats, + identifiers: stats.binary ? [] : extractChangedIdentifiers(patch, maxIdentifiersPerFile) + }; + }); + + const queries = buildReviewQueries(changedFiles, maxQueries); + if (queries.length === 0 && changedFiles.length > 0) { + warnings.push('No review search queries could be derived from the changed diff'); + } + if (changedFiles.length > maxQueries) { + warnings.push( + `Search was bounded to ${maxQueries} changed-file queries for ${changedFiles.length} changed files` + ); + } + + const searches: ReviewSearchResult[] = []; + for (const reviewQuery of queries) { + try { + const response = await options.search(reviewQuery.query, maxResultsPerQuery); + searches.push({ + query: reviewQuery.query, + sourceFiles: reviewQuery.sourceFiles, + searchQuality: response.searchQuality, + ...(response.preflight ? { preflight: response.preflight } : {}), + results: response.results.slice(0, maxResultsPerQuery).map(compactSearchResult) + }); + } catch (error) { + searches.push({ + query: reviewQuery.query, + sourceFiles: reviewQuery.sourceFiles, + results: [], + error: error instanceof Error ? error.message : String(error) + }); + warnings.push(`Search failed for query: ${reviewQuery.query}`); + } + } + + let conventions: PatternResponse | undefined; + if (options.loadConventions) { + try { + conventions = await options.loadConventions(); + } catch (error) { + warnings.push( + `Convention lookup failed: ${error instanceof Error ? error.message : String(error)}` + ); + } + } + + const additions = changedFiles.reduce((sum, file) => sum + file.additions, 0); + const deletions = changedFiles.reduce((sum, file) => sum + file.deletions, 0); + const relatedResultCount = searches.reduce((sum, search) => sum + search.results.length, 0); + + return { + schemaVersion: 'review-context-v1', + refs: { + base: options.base, + head: options.head, + baseCommit: options.baseCommit, + headCommit: options.headCommit, + range: `${options.base}...${options.head}` + }, + diffSha256: fingerprintDiff(options.rawDiff), + limits: { maxQueries, maxResultsPerQuery, maxIdentifiersPerFile }, + summary: { + filesChanged: changedFiles.length, + additions, + deletions, + queryCount: searches.length, + relatedResultCount + }, + changedFiles, + searches, + ...(conventions ? { conventions } : {}), + warnings + }; +} diff --git a/tests/review-cli-entrypoint-runtime.test.ts b/tests/review-cli-entrypoint-runtime.test.ts new file mode 100644 index 0000000..1e3c131 --- /dev/null +++ b/tests/review-cli-entrypoint-runtime.test.ts @@ -0,0 +1,21 @@ +import { spawnSync } from 'node:child_process'; +import { resolve } from 'node:path'; +import { describe, expect, it } from 'vitest'; + +const root = resolve(import.meta.dirname, '..'); +const entrypoint = resolve(root, 'src', 'review-bin.ts'); + +describe('review CLI entrypoint runtime', () => { + it('executes the package wrapper and prints help without loading a review target', () => { + const result = spawnSync(process.execPath, ['--import', 'tsx', entrypoint, '--help'], { + cwd: root, + encoding: 'utf8', + timeout: 30_000 + }); + + expect(result.status).toBe(0); + expect(result.stderr).toBe(''); + expect(result.stdout).toContain('codebase-context-review --base '); + expect(result.stdout).toContain('It does not call an LLM'); + }, 30_000); +}); diff --git a/tests/review-context.test.ts b/tests/review-context.test.ts new file mode 100644 index 0000000..52dca41 --- /dev/null +++ b/tests/review-context.test.ts @@ -0,0 +1,174 @@ +import { describe, expect, it } from 'vitest'; +import { + buildReviewContextPacket, + buildReviewQueries, + extractChangedIdentifiers, + fingerprintDiff, + inspectPatch, + parseNameStatus, + type ChangedFileContext +} from '../src/review-context.js'; +import type { SearchResponse } from '../src/tools/types.js'; + +describe('review context', () => { + it('parses modified, added, deleted and renamed files', () => { + expect( + parseNameStatus(['M\tsrc/a.ts', 'A\tsrc/b.ts', 'D\tsrc/c.ts', 'R100\tsrc/old.ts\tsrc/new.ts'].join('\n')) + ).toEqual([ + { path: 'src/a.ts', status: 'M', rawStatus: 'M' }, + { path: 'src/b.ts', status: 'A', rawStatus: 'A' }, + { path: 'src/c.ts', status: 'D', rawStatus: 'D' }, + { + path: 'src/new.ts', + previousPath: 'src/old.ts', + status: 'R', + rawStatus: 'R100' + } + ]); + }); + + it('parses NUL-delimited git output without corrupting unusual paths', () => { + expect(parseNameStatus('M\0src/tab\tname.ts\0R100\0old\nname.ts\0new name.ts\0')).toEqual([ + { path: 'src/tab\tname.ts', status: 'M', rawStatus: 'M' }, + { + path: 'new name.ts', + previousPath: 'old\nname.ts', + status: 'R', + rawStatus: 'R100' + } + ]); + }); + + it('counts textual changes without treating diff headers as source', () => { + const patch = [ + 'diff --git a/src/auth.ts b/src/auth.ts', + '--- a/src/auth.ts', + '+++ b/src/auth.ts', + '@@ -1,2 +1,3 @@', + '-const oldToken = loadToken();', + '+const refreshToken = loadRefreshToken();', + '+return refreshToken;' + ].join('\n'); + + expect(inspectPatch(patch)).toEqual({ additions: 2, deletions: 1, binary: false }); + }); + + it('detects binary patches', () => { + expect(inspectPatch('diff --git a/a.png b/a.png\nBinary files a/a.png and b/a.png differ')).toEqual({ + additions: 0, + deletions: 0, + binary: true + }); + }); + + it('ranks semantic identifiers from changed lines and drops language noise', () => { + const patch = [ + '-const token = auth.load();', + '+const refreshToken = sessionService.loadRefreshToken();', + '+return sessionService.rotateRefreshToken(refreshToken);', + '+const SessionService = createSessionService();' + ].join('\n'); + + const identifiers = extractChangedIdentifiers(patch, 8); + + expect(identifiers).toContain('sessionService'); + expect(identifiers).toContain('refreshToken'); + expect(identifiers).toContain('rotateRefreshToken'); + expect(identifiers).not.toContain('const'); + expect(identifiers).not.toContain('return'); + }); + + it('bounds review queries and falls back to file path signals', () => { + const files: ChangedFileContext[] = [ + { + path: 'src/auth/session-service.ts', + status: 'M', + rawStatus: 'M', + additions: 10, + deletions: 2, + binary: false, + identifiers: ['SessionService', 'refreshToken', 'rotateToken'] + }, + { + path: 'src/http/request-guard.ts', + status: 'M', + rawStatus: 'M', + additions: 2, + deletions: 1, + binary: false, + identifiers: [] + } + ]; + + expect(buildReviewQueries(files, 1)).toEqual([ + { + query: 'SessionService refreshToken rotateToken', + sourceFiles: ['src/auth/session-service.ts'], + identifiers: ['SessionService', 'refreshToken', 'rotateToken'] + } + ]); + + expect(buildReviewQueries(files, 2)[1]).toEqual({ + query: 'request-guard http src', + sourceFiles: ['src/http/request-guard.ts'], + identifiers: ['request-guard', 'http', 'src'] + }); + }); + + it('fingerprints exact diff bytes, including the final newline', () => { + expect(fingerprintDiff('diff body\n')).not.toBe(fingerprintDiff('diff body')); + }); + + it('builds a bounded packet and keeps failed searches explicit', async () => { + const rawDiff = [ + 'diff --git a/src/auth.ts b/src/auth.ts', + '--- a/src/auth.ts', + '+++ b/src/auth.ts', + '@@ -1 +1 @@', + '-oldSession();', + '+rotateSessionToken();' + ].join('\n'); + + const searchCalls: string[] = []; + const search = async (query: string): Promise => { + searchCalls.push(query); + return { + status: 'ok', + searchQuality: { status: 'ok', confidence: 0.91 }, + results: [ + { + file: 'src/session/store.ts:10-24', + summary: 'Session token storage', + score: 0.88, + snippet: 'export function rotateSessionToken() { return true; }' + } + ] + }; + }; + + const packet = await buildReviewContextPacket({ + base: 'origin/main', + head: 'HEAD', + baseCommit: 'a'.repeat(40), + headCommit: 'b'.repeat(40), + rawDiff, + changedFiles: [{ path: 'src/auth.ts', status: 'M', rawStatus: 'M' }], + patchesByPath: new Map([['src/auth.ts', rawDiff]]), + maxQueries: 2, + maxResultsPerQuery: 1, + search + }); + + expect(packet.schemaVersion).toBe('review-context-v1'); + expect(packet.diffSha256).toBe(fingerprintDiff(rawDiff)); + expect(packet.summary).toEqual({ + filesChanged: 1, + additions: 1, + deletions: 1, + queryCount: 1, + relatedResultCount: 1 + }); + expect(searchCalls).toEqual(['rotateSessionToken oldSession']); + expect(packet.searches[0]?.results[0]?.file).toBe('src/session/store.ts:10-24'); + }); +});