diff --git a/plugins/code-review/CHANGELOG.md b/plugins/code-review/CHANGELOG.md index f486851..c13737a 100644 --- a/plugins/code-review/CHANGELOG.md +++ b/plugins/code-review/CHANGELOG.md @@ -7,6 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Headless skills for workflow callers — `cr-prepare` (scope, conventions, standards and the + active lens set, written to a context directory), `cr-scan` (one lens) and `cr-merge` (one + report, every finding returned with its fix-risk class). Model-only, they ask nothing and edit + nothing in the checkout, and an empty change or a missing lens returns a status, never a clean + review +- `get_changes.py -C ` reviews another checkout without changing directory +- Eval-20 runs the headless pipeline against a git sandbox + ### Changed - Author metadata now reads `Mateusz Gostański ` in `plugin.json` and the marketplace entry. diff --git a/plugins/code-review/README.md b/plugins/code-review/README.md index f942264..afe9b13 100644 --- a/plugins/code-review/README.md +++ b/plugins/code-review/README.md @@ -35,6 +35,22 @@ partial review, invoke `/comment-review` or `/quality-review` directly; both sta independently available and share the same rule text as the command. The three added lenses have no standalone skill. +### Headless skills for workflows + +A workflow agent cannot answer questions or dispatch agents of its own, so `/start-cr` +cannot run there. Three model-only skills (hidden from the `/` menu) split the same +pipeline into steps a caller orchestrates, all eight lenses included: + +| Skill | Arguments | Writes | +|---|---|---| +| `code-review:cr-prepare` | `--base --out [-C ] [--spec ]` | `scope.json`, `conventions.md`, `standards.md` | +| `code-review:cr-scan` | `--lens --context ` | `.md` — run one per active lens, each as its own agent | +| `code-review:cr-merge` | `--context ` | `report.md`, and returns every finding with its fix-risk class | + +None of them edits the checkout or asks anything. An empty change, a lens that did not +report or a missing lens file comes back as a status, never as a clean review. +`fd3`'s implementation workflows are the first caller. + The report groups by **file**, with the two vocabularies side by side — comment verdicts (`R1`–`R12` · KEEP/REMOVE/REWRITE/MOVE/ADD) and quality findings (`` `family` · rule · severity `` across eleven families: `readability`, `tests`, diff --git a/plugins/code-review/commands/start-cr.md b/plugins/code-review/commands/start-cr.md index e23d764..b36d1c7 100644 --- a/plugins/code-review/commands/start-cr.md +++ b/plugins/code-review/commands/start-cr.md @@ -110,62 +110,16 @@ list. Then classify every surviving file by that file's `## File kinds` section ## Step 2 — Read project conventions and standards (once) -`${CLAUDE_PLUGIN_ROOT}/references/scope.md` also carries the **mechanical convention -read** (the exact paths, root first) and the **language-applicability** rules for -families or rules that have no counterpart in the language under review. That read -now opens with the **standards pair** at the repository root — `CODING_STANDARDS.md`, -then `CODING_STANDARDS.local.md` — which LAYER: both apply, and where two statements -conflict the `.local` one wins. Work it there, then: - -- capture what you learned in one short **conventions note**, and **pass it to every - Scanner** so a documented convention never surfaces as a finding; the note also - records a **tracked `.local` file** (`git check-ignore` fails on it) and any - **conflict between two project files** (resolved by scope.md's precedence order), - both of which reach the report's `Conventions` line; -- **one note, byte-identical in every brief, and it may only suppress.** Write it once and paste - the same text into all N briefs: a per-Lens note is a per-Lens instruction, and the Scanner - reads whatever it finds there as what you want it to look for. So the slot holds nothing but - documented conventions, each **quoted verbatim with its file** — never your own threat - hypotheses or "where to focus", never an "established facts — do not raise" list, never a - paraphrase of a rule (one run's paraphrase said a legacy pattern "is documented as accepted" - where the rule said to migrate off it, and buried the very finding the user later asked for). - Anything you want checked belongs in the Lens's own rules file, not here. A note that grows - past a screen is the wrong shape: cut it to the rules that actually suppress something; -- name any family or rule the language makes **N/A** in that note, so its owning - Scanner clears it in one line instead of inventing findings to fit; -- keep the standards text **out of the note**: it travels in the brief's own - `` slot because, unlike everything else the read picks up, it - **generates** findings. A Scanner raises `` `standards` · · `` only for - an explicit, quotable rule inside its own Lens's subject, citing the file and - section; vague prose ("write clean code") never generates; unsettled fit goes to - `CANDIDATES`; a rule the `.local` file relaxes is suppressed; and a - formatting/whitespace/import-order/quote rule is skipped when a formatter or linter - config exists at the root (scope.md lists the presence check). When the pair is long, - **pre-slice it per Lens** so each Scanner receives only the rules in its subject; a - short pair goes to every Scanner whole. The rest of the conventions — `CLAUDE.md`, - `AGENTS.md`, `CONTRIBUTING.md`, `.cursor/rules`, `.claude/rules` — stay - **suppress-only**: they remove findings, never create them. +Settle the conventions note and the standards slot by the first half of +`${CLAUDE_PLUGIN_ROOT}/references/review-setup.md` — read it now; the mechanical read it works +from is in `${CLAUDE_PLUGIN_ROOT}/references/scope.md`. One note, written once, byte-identical in +every brief. ## Step 2b — Resolve the active lens set -Not every Lens runs on every change. Decide the set here, once, from the input — -never from a preference: - -- the five craft Lenses (`comments`, `readability & tests`, `naming & module`, - `objects & patterns`, `simplicity & types`) and **`security`** are **always - active** — six on any change, however small; -- **`performance`** is active iff the `source`-kind subset of the resolved list, - **minus `.sh` files**, is non-empty — a tests-only, IaC-only, or shell-only change - skips it; -- **`spec`** is active iff a spec resolved to a readable local file in Step 1 — from - `--spec`, or from the offer the user accepted when the change carried its own spec. - -Record **N**, the number of active Lenses, and for each one its own ``: -`performance` gets the source subset it was gated on; every other Lens gets the full -resolved list. Record every **inactive** Lens with its reason (`performance — no -executable code`, `spec — no spec named`); the Tally prints them in Step 5. From here on -**N** means this count: N Scanners dispatched, N `` blocks awaited, N outputs -merged. +Decide the set once, from the input and never from a preference, by the second half of +`${CLAUDE_PLUGIN_ROOT}/references/review-setup.md`. Record **N**, each active Lens's own +``, and every inactive Lens with its reason. ## Step 3 — Dispatch N Scanners in parallel @@ -179,9 +133,10 @@ synchronous. Let them background; that is the working path. **Without the `Agent` tool there is no review to run.** In some contexts — inside another agent, inside a workflow step — it is simply absent, and a single pass by one reader is not this command however carefully it reads. Say so in your first sentence, name the lenses that will not -run, and let the caller decide between an announced single-pass reading and invoking -`/quality-review`, `/comment-review` and `/security-review` as their own agents. Never discover -this silently halfway and report the result as a review. +run, and let the caller decide between an announced single-pass reading and the headless route: +`code-review:cr-prepare` once, `code-review:cr-scan` once per active lens as its own agent, then +`code-review:cr-merge` — the same eight lenses, one agent each, with no questions asked. Never +discover this silently halfway and report the result as a review. **Never pass `name:` to a Scanner call.** Naming routes the Scanner into the agent-teams mailbox, where its findings come back only if you ask for them and it answers — a channel @@ -238,417 +193,32 @@ problem with no rule to land on, never compressed into the one-line list. ### The Scanner brief -Send each Scanner a brief in this shape, filling every slot: - -``` - - comments | readability & tests | naming & module | objects & patterns | simplicity & types | security | performance | spec - ${CLAUDE_PLUGIN_ROOT}/references/rules/.md - - - - tracked → `git diff -- ` - untracked → read the file directly; every line is added - - - - - - primary = the problem is in code this change added or modified, or structure - the change introduced or made worse. - boy-scout = a problem in untouched code noticed only while reading for context — - optional, kept strictly separate, never mixed into the primary findings. - A fully added file (status `A`) has no boy-scout findings: the whole file is code - the change introduced, so every finding in it is primary. - - - -``` - -Read the rules file **completely first**, then judge only the families that belong to -that Lens. A Scanner **returns findings/verdicts only**: it does not render a report, -does not re-grade centrally, and **writes nothing into the tree** — not the files under -review, and not a scratch or probe file to test a hypothesis against. - -A Scanner is **one agent, one pass, one output**. It **dispatches no agent of its own** — a -sub-agent puts a second hop between the finding and the merge, and the Scanner that tried it -had its own report overwritten by the follow-up, losing a handoff outright. It does not wait in -the background, poll, or schedule anything; it reads, judges, and returns. Its **final message -is its whole output**: if something has to change after it has already written its findings, -it re-sends the complete list, never an "amendment" or a delta — anything the last message -leaves out never reaches the merge. It is reading the -user's working copy, so it settles a doubt by reading the type, the signature, or the call -site, and marks the rest `(verify)`. Read the whole changed file for context, and target -what the change touched. The `naming & module` Scanner alone adds the **one-hop -cross-file protocol** on top of that: search the importers of each changed module and the -imports of each module it newly imports — with the `Grep` tool, or `git grep` from `Bash` in a -session where that tool is not handed to sub-agents — open those files at the matched lines only — -no transitive crawl, no repo listing, no `find`; a fact beyond the hop is `(verify)`; -it still writes nothing. - -### The eight Lenses - -1. **comments** → `${CLAUDE_PLUGIN_ROOT}/references/rules/comments.md` - Returns per-comment **VERDICTS**, one per comment: - `` `comments` · R# · KEEP/REMOVE/REWRITE/MOVE/ADD · `path:line` · "verbatim comment" — one-line reason → concrete suggested fix ``. - Run the deletion test on every comment first. Surface **R9 - (contradicts-the-code) findings first**. The **test-file bar is higher (R11)**: - default to REMOVE when unsure in tests. **`ADD` is the one verdict with no - existing comment to quote** — an R2 *missing WHY* at genuinely non-obvious code - (a magic constant, a workaround, a specific timeout/retry/batch size, a silent - catch); it drops the verbatim-comment slot for a site description: - `` `comments` · R2 · ADD · `path:line` — → ``. - Raise `ADD` only where you can state the reason concretely — never a guess - dressed as a WHY. Every suggested fix obeys the comment rules itself: no spec-id - fragments (`(R2)`, `F1:`, `§4.1`), no new file/doc cross-references (R4), no - banners (R5). For MOVE, name the destination and give the exact text to place - there, plus "delete from the declaration". - -2. **readability & tests** → `${CLAUDE_PLUGIN_ROOT}/references/rules/readability-tests.md` - Judges the `readability` and `tests` families. - -3. **naming & module** → `${CLAUDE_PLUGIN_ROOT}/references/rules/naming-module.md` - Judges the `naming` and `module` families. - -4. **objects & patterns** → `${CLAUDE_PLUGIN_ROOT}/references/rules/objects-patterns.md` - Judges the `objects` and `patterns` families. - -5. **simplicity & types** → `${CLAUDE_PLUGIN_ROOT}/references/rules/simplicity-types.md` - Judges the `simplicity` family. - -6. **security** → `${CLAUDE_PLUGIN_ROOT}/references/rules/security.md` - Judges the `security` family; always active. A finding names **both** `path:line` - of the **source** (where untrusted data enters) and of the **sink**; a pattern alone - (`req.body`, a string containing `SELECT`) is never a finding; `L` lists both - ends, source first, and the clause says which is which. When either end sits - outside the files in view the Scanner reads it — it can `Read` any file and search with - `Grep` or `git grep` — and marks only what it still cannot confirm `(verify)`. `CANDIDATES` is reserved for a - confirmed source→sink pair whose *mitigation* is the doubt; a cleared look-alike is - one prose line for `Not flagged`. Severity is `high` or `medium`, **never `nit`**. - It never runs the code, an audit tool, or a network command; `.env`, YAML, JSON and - manifests stay skipped, and the report's Skipped line sends those to - `/security-review`. - -7. **performance** → `${CLAUDE_PLUGIN_ROOT}/references/rules/performance.md` - Judges the `performance` family; active only over the `source` subset from Step 2b. - A finding names **four things** — the multiplier (the loop's collection or the - endpoint, and where its size comes from), the call inside it, the bound that is - missing, and the batch/limit API that exists — or it is a `CANDIDATE`. "Could be - slow", "may impact performance", and any estimate not derived from a line in the - diff are forbidden; it never runs or profiles code. - -8. **spec** → `${CLAUDE_PLUGIN_ROOT}/references/rules/spec.md` - Judges the `spec` family; active only with `--spec`. It enumerates the requirements - in the `` slot and maps each to the diff. **Every finding quotes the spec line - verbatim.** A `wrong-implementation`, `partial-requirement`, or `scope-creep` sits - under the code file it points at; a `missing-requirement` has no code site, so it - sits under a `### ` header with the **spec's own `L`**. The - requirements met come back as **one prose count line**, never as findings. - `scope_split` is **N/A** for this Lens — a spec finding is neither primary nor - boy-scout, so it returns one list. Runtime claims are `(verify)`; a PARTIAL-vs-WRONG - doubt is a candidate; a craft problem noticed on the way is a `HANDOFF`. - -For the finding-shaped Lenses (2–8) the Scanner returns **FINDINGS**, split into primary -and boy-scout (the `spec` Lens excepted), each in this exact shape: - -``` -`family` · rule · severity · L — → -``` - -A **`standards` finding** — any Lens may raise one, from the `` slot only — -puts the quoted rule and its source where the loss goes: - -``` -`standards` · · · L — "" (CODING_STANDARDS.md ›
) → -``` - -with a short kebab-case slug from the rule's wording and the severity from the keyword -mapping in `references/severity.md`. - -**Severity is exactly one of `high`, `medium`, or `nit`** — never `low`, never a -number, never a paraphrase. A Scanner whose own rules file happens to list only one -of the three still uses the full vocabulary. Tell each Scanner that **severity is a -first pass** — you re-grade every quality finding centrally in Step 4, so it grades -honestly against its rules without agonizing over the boundary. - -**The FINDINGS section holds findings only.** Anything a Scanner checked and cleared -belongs in one prose line, never in the finding shape — a "none found" or "is **not** -a finding" bullet with a dash where the severity goes reads as a finding to everything -downstream. - -**A duplication finding sweeps the whole file.** "Target what the change touched" holds -for most rules, but duplication is the exception: when you flag repeated code (an -`over-complex` duplication, a copy-pasted predicate), scan the **rest of the file** for -every other copy of the same pattern and list all the call sites in the one finding — -including copies in code the change didn't touch. A finding that names two of three -copies makes the extraction fix leave a straggler behind. The one **cross-file** -exception is `module` · canonical-helper: a new helper duplicating an exported helper -elsewhere in the repo is found by the one-hop Grep, bounded to the helper's name and its -distinctive expression — never a repo-wide sweep, and inconclusive means `(verify)`. - -### Three side-channels, three distinct meanings - -Report what you find and let the merge filter it. Each Scanner judges against its -rules, then against each rule's own calibration paragraph — the look-alike that is -*not* a violation. Calibration clearing a site makes it a non-finding. Anything left -unsettled travels in one of three channels, and these are **not** interchangeable: - -| channel | means | -|---------|-------| -| `(verify)` | the **fact** is unconfirmable here — runtime behaviour, or a file outside the review scope | -| `HANDOFF` | confirmed, but **another Lens's family** owns it | -| `CANDIDATES` | confirmed and mine, but the **rule fit or its calibration** is a judgment call | - -- **`(verify)` marker** — a Scanner that doubts a finding **resolves it itself first**: - it has `Read`, so it opens the type, the signature, or the call site and confirms or - drops it (a `needless-cast` is the common case — check what the value's type actually - is before claiming the cast is redundant). It appends `(verify)` only when confirming - would take something it does not have. You resolve those in Step 4. -- **`CANDIDATES` block** — a site that survives the deletion of doubt about the *facts* - but that the Scanner cannot settle against the rule's calibration. It belongs here - rather than in the bin: you decide it with the whole review in view, and a candidate - you reject costs one line in `Not flagged`, while one the Scanner never reported costs - the finding outright. - - ``` - ## CANDIDATES (rule fit or calibration uncertain — orchestrator decides) - - `family` · rule · `path:line` — → - ``` - -- **`HANDOFF` block** — a real problem that belongs to another Lens's family, in a - separate block at the end of the output, never mixed into the Scanner's own findings - and never buried in prose: - - ``` - ## HANDOFF (out-of-my-family — noticed but not mine to grade) - - `` · · `path:line` — → - ``` - -One terse line each. Omit a block when it is empty. +The brief each Scanner receives, the eight Lenses and their output contracts, and the three +side-channels are in `${CLAUDE_PLUGIN_ROOT}/references/scanner-contract.md` — read it completely +before the dispatch and fill every slot of the brief it defines. Its paths are relative to +`${CLAUDE_PLUGIN_ROOT}/references/`: a brief carries the absolute +`${CLAUDE_PLUGIN_ROOT}/references/rules/.md` in its `` slot. ## Step 4 — Merge and re-grade -- **Collect** all N Scanners' outputs — every active lens's `` actually in hand - per Step 3, not merely a notification that fired; a lens you could not collect is a - labelled degradation you already surfaced to the user, never a silent gap in the merge. -- **Dedup overlaps**: when two findings point at the same code — including across - different lenses, and across **all eleven families**, craft and `security` / - `performance` / `spec` / `standards` alike — keep the **most-specific** one and drop - the rest. When the overlap - spans two severities (a `high` symptom folding into a lower-severity root cause, or the - reverse), the surviving bullet keeps the **highest** severity of the overlap — deduping - must never quietly demote a `high` under a `medium`. -- **Count the lenses that converged.** Independent Scanners landing on the same code - is the strongest signal this review produces — they read the file separately and had - no way to coordinate. Treat a finding several lenses reached (directly or via - `HANDOFF`) as **confirmed**: it leads its file, and it is a candidate for the - headline. Convergence raises confidence and ordering, **never severity** — that stays - verbatim from the table. -- **Route every `HANDOFF` and every `CANDIDATES` entry to a visible home.** Assign a - `HANDOFF` its correct family and rule; decide a candidate against its rule's - calibration. Either way it lands in exactly one of two places: a graded bullet in the - per-file report (on its own, or merged into a converging finding), or a `Not flagged` - line with its one-line reason. **The entry no primary finding corroborates is the one - that slips, so reconcile by an itemized check, not by assertion.** Before rendering, - write the check out: enumerate every `HANDOFF` and every candidate you received, and - against each name its home — the report bullet (`path:line`) it became, the converging - finding it merged into, or the `Not flagged` line that clears it. An entry with no home - on that list is a bug: route it before you render. -- **A primary finding is reconciled too.** The channels are not the only thing that goes - missing: a Scanner's own `FINDINGS` entry can fall out of the merge between collecting and - rendering, and nothing downstream notices. Count what you received per Scanner, and give every - primary finding that does not reach a report bullet — deduped into another, demoted, or - rejected — its own `Not flagged` entry with the reason. Dedup is the one silent case allowed, - and only because the surviving bullet carries it. -- **Publish that check as one counted line above the report** — `Reconciliation: N - handoffs + M candidates → A merged · B own bullet · C boy-scout · D Not flagged; P primary - dropped` — where `A + B + C + D` equals `N + M`, and `P` counts the primary findings that got - no bullet. The arithmetic is what makes the check real: a - run that states "every handoff routed" without it has asserted rather than reconciled, - and loses the entry nothing else corroborates. When the sums disagree, an entry is - unrouted — find it, never adjust a number to close the gap. -- **Each count names the block it is counted in**, so the line can be checked against the report - rather than believed: `merged` is an entry folded into another finding's bullet and visible in - its text, `own bullet` one that became its own graded bullet under a file, `boy-scout` one - rendered in the `Boy-scout` block, `Not flagged` one rendered as its own entry in `Not - flagged`. Runs whose arithmetic was right have still printed `0 boy-scout` over a Boy-scout - block holding three routed handoffs, and counted six entries as `merged` into a bullet that - was never rendered. Before publishing, count the rendered blocks: `C` equals the Boy-scout - entries that came from a channel, and `D + P` equals the entries in `Not flagged`. A count - that does not match the block it names is the bug, not the block. -- **Resolve every `(verify)` finding**: read the code and confirm or refute it. A - confirmed finding drops the marker and proceeds; a refuted one is a **Scanner false - positive** — drop it and note it under `Not flagged`. An unresolved `(verify)` finding - never reaches an apply batch. Most runs will have none — the Scanners resolve their own - doubts. When no Scanner emitted one, say nothing about `(verify)` anywhere: do not - claim to have resolved an empty list, and do not relabel some other mechanism as a - `(verify)` — a routed `HANDOFF`, a decided candidate, or a refuted scanner doubt is - resolved under its own name. -- **Re-grade every quality finding's severity yourself** against the master table in - `${CLAUDE_PLUGIN_ROOT}/references/severity.md` — read it now if you have not. It - carries the 44 rows, what each severity means, the anti-anchoring rule, and the - **`standards` keyword mapping** (MUST / MUST NOT / NEVER / ALWAYS → high, SHOULD → - medium, MAY / prefer / consider → nit, no keyword → medium). A `standards` finding has - no fixed row: re-grade it against that mapping by re-reading the rule it quotes, not - the Scanner's guess. A single-lens Scanner is the one most prone to the anchoring that - table forbids, so its severity is a first pass and yours is the one that ships. -- **Judge the fix, not only the finding.** A finding can be right and its fix wrong, and Step 6 - is too late to notice: by then the user has approved it. For every fix that could reach a - bucket, check three things against the code you already read: - - **Does it keep behaviour?** Moving a guard onto a DTO turns a 400 into a 422; splitting a - shared client drops the double-submit guard that shared instance provided; deleting an unused - export removes what a later stage of the same spec consumes. A fix that changes what callers - observe is not mechanical, whatever its rule says. - - **Does it contradict another finding?** One review's headline fix bounded a payload *before* - the redaction walk, which would have truncated secrets under the redactor's minimum length — - a security hole introduced by a performance fix. Read the fixes as a set, not one at a time. - - **Does it create the next finding?** An extraction that takes five positional parameters, a - helper that duplicates one two files away — fix the fix before offering it. - - A fix that fails any of the three is re-routed: to the structural walk with the behaviour - change named in its option, or to report-only with one line on why. Say which in the report's - bullet rather than silently dropping the finding. -- **Comment verdicts are not re-graded** and are **not** mapped to severities. The - two vocabularies stay side by side; there is no severity↔verdict mapping - anywhere in this command. +Merge the N outputs by the **Merge and re-grade** half of +`${CLAUDE_PLUGIN_ROOT}/references/merge-contract.md` — read it now if you have not. Every rule +there binds this step: dedup, convergence, routing every `HANDOFF` and candidate, the published +`Reconciliation` line, `(verify)` resolution, re-grading against the severity table, and judging +each fix before it can reach Step 6. ## Step 5 — Report (one per-file skeleton, two vocabularies side by side) -Group by **file**, not by Scanner. Under each file, list quality findings and -comment verdicts **together**. Render with **exactly this template**, in this -order — keep the structure identical between runs: - -```markdown -Reconciliation: handoffs + candidates → merged · own bullet · boy-scout · Not flagged;

primary dropped - -## Code review — - -**Conventions:** -**Headline:** - -### -- `family` · rule · severity · L — → -- `comments` · R# · KEEP/REMOVE/REWRITE/MOVE/ADD · L — → - -### -- `family` · rule · severity · L — <…> - -**Not flagged:** - -**Boy-scout (untouched code, optional):** -- `family` · rule · :L — - -**Tally:** N quality findings (H high · M medium · K nit) · C comments (X remove · Y rewrite · Z move · V add · W keep) · F files. Lenses: L of 8 (skipped: — ). Spec: R of T requirements met. Skipped: . -``` - -A filled-in report reads like this: - - -Reconciliation: 4 handoffs + 2 candidates → 3 merged · 1 own bullet · 0 boy-scout · 2 Not flagged; 0 primary dropped - -## Code review — committed (base → HEAD), 3 files - -**Conventions:** repo `CLAUDE.md` documents barrel exports as the public-API style, so `module` · barrel is not flagged here. -**Headline:** `checkout/total.ts` concatenates the request's coupon code into a raw SQL string at L72. - -### src/checkout/total.ts -- `security` · injection-sink · high · L70, L72 — `couponCode` read from `req.query` at L70 reaches the raw `WHERE` string at L72 by concatenation → bind it as a query parameter -- `simplicity` · over-complex · high · L18, L34, L51 — three copies of the tier-discount branch drift independently → collapse into `discountFor(tier)` and call it at each site -- `readability` · magic-literal · medium · L22 — `0.1` carries the gold-tier rate with nothing naming it → name `GOLD_DISCOUNT_RATE` -- `comments` · R1 · REMOVE · L17 — "// multiply by the rate" restates the line beneath it → delete these lines - -### src/checkout/receipt.ts -- `naming` · role-name · nit · L9 — `receiptArray` names the type instead of the role → `receipts` -- `comments` · R2 · ADD · L44 — the 250 ms retry gap is a gateway constraint no reader can infer → "// 250 ms — the gateway rejects retries closer than its own debounce window" - -### docs/checkout-spec.md -- `spec` · missing-requirement · high · L14 — "A receipt lists the discount applied per line item" has no implementation in the diff → add the per-line discount to `Receipt` - -**Not flagged:** `JSON.parse(raw) as Config` at L7 (boundary narrowing, not `needless-cast`); the exhaustive `default:` throw at L61 (defensive assertion, not `dead-code`). - -**Tally:** 5 quality findings (3 high · 1 medium · 1 nit) · 8 comments (1 remove · 0 rewrite · 0 move · 1 add · 6 keep) · 3 files. Lenses: 8 of 8. Spec: 4 of 5 requirements met. Skipped: pnpm-lock.yaml (lockfile). - - -**The skeleton is the whole report.** It has no other sections: no `### Findings` -header, no numbered or bolded finding entries, no `---` rules between findings, no -per-finding code block, no closing summary. The `###` headers are **file paths** — one -per reviewed file, plus the **spec's own path** when `--spec` was given and a -`missing-requirement` needs a home — and each finding is a single markdown bullet -beneath its file. Do -not paste the code under review, the rewritten body, or a before/after block: a finding -that seems to need a code block is one whose fix is not yet stated as a clause, so state -it as a clause. Every report opens with `Conventions` and `Headline`, and closes with -`Tally`. The `Reconciliation` line is the only thing that precedes `## Code review` — it -belongs to Step 4's check rather than to the report, which is why it carries counts and -not prose. - -Rules for filling it in: - -- **Two vocabularies, side by side.** Quality findings use `` `family` · rule · - severity ``, with the family **backticked** — one of the eleven fixed labels - `readability`, `tests`, `naming`, `module`, `objects`, `patterns`, `simplicity`, - `security`, `performance`, `spec`, `standards` — and rule and severity verbatim from - `references/severity.md` (a `standards` rule is its slug, graded by the keyword - mapping). Comment verdicts use `` `comments` · R# · KEEP/REMOVE/REWRITE/MOVE/ADD ``. - **No severity↔verdict mapping** — keep them distinct. -- **Findings are markdown bullets** under a `###` file header (not inside a ``` - fence) so every `path:line` stays clickable. A `spec` · missing-requirement bullet - sits under `### ` with the spec's own lines; every other spec finding sits - under the code file it points at. -- **Order files** by their highest-severity quality finding; a REMOVE/REWRITE/MOVE/ADD - comment weighs like a medium for ordering. Within a file: a `security` high first, - then any **R9 (contradicts-the-code)** comment verdict, then high → medium → nit, - then by line. -- **Collapse repeats**: one `family` · rule breaking in several spots is a single - bullet with the lines listed together (`L20, L34, L51`). -- **The fix is a clause, not code.** "extract - `transitionOrReportConflict(...)` and early-return at each site", "drop the `as - User` cast", "name `SECONDS_PER_DAY`". Keep a rewritten body or a before/after - block out of the report. For a comment REWRITE the fix is the exact replacement - text; for MOVE, name the destination. -- **Quote comments verbatim.** Every comment verdict carries the verbatim comment - text and its `path:line`. -- **`Not flagged`** lists the look-alikes deliberately passed on, plus every candidate, - `HANDOFF` and dropped primary finding the merge cleared — one line when they are all genuine - non-findings, a short bullet each when one of them is a *real* problem that merely has no rule - to land on. **Its entries stay countable**: separated by `;` on the one-line form, one bullet - each otherwise, because the `Reconciliation` line's last two numbers are checked against them. A real problem keeps its own bullet rather than being compressed into a - subordinate clause; that compression is how something worth acting on disappears. Drop - the block if empty. -- **`Boy-scout`** holds only findings in code the change did not touch; omit the - whole block when there are none. -- **Resolved findings only.** The body lists confirmed findings; a refuted one goes in - `Not flagged` as a Scanner false positive. -- **The headline may not contradict the combined tally.** If there is any quality - `high` or `medium` finding, **or** any comment REMOVE / REWRITE / MOVE / ADD, the - headline names the worst one — it must not call the change "clean", - "well-structured", or "only cosmetic nits". A confirmed **`security`** finding is the - headline over any craft finding, whatever their severities — and so is a confirmed - **exposure that no rule names**, which leads the report from its own `Not flagged` - bullet rather than being demoted for want of a tag; a `spec` · - missing-requirement or wrong-implementation forbids the clean headline outright. - Reserve the clean verdict for a tally that is genuinely nits-only-and-all-KEEP (or - empty). -- **The `Tally` names the lenses.** `Lenses: L of 8` always, with each skipped Lens - and its Step 2b reason in the parenthesis (`skipped: performance — no executable - code; spec — no spec named`); drop the parenthesis when all eight ran. When a spec was - given, add the `spec` Scanner's met-requirements count as `Spec: R of T requirements - met`; omit that clause otherwise. - -Collapse the whole report to the title line plus a one-sentence verdict and the -tally **only when the change reads cleanly** — the quality tally is empty or -nits-only and every comment is KEEP, and no `spec` · missing-requirement or -wrong-implementation stands. Match the report to what you found: neither pad -a clean one to look thorough, nor collapse one carrying a medium-or-higher finding, a -spec gap, or a REMOVE/REWRITE/MOVE/ADD to look clean. +Render the review with **exactly** the skeleton in the **Report** half of +`${CLAUDE_PLUGIN_ROOT}/references/merge-contract.md`, and by every rule for filling it in that +follows the skeleton there — same structure between runs, nothing added. **`Tally` ends the report text, not the turn.** Go straight into Step 6's `AskUserQuestion` — same turn, no pause, nothing between it and the tally. A turn that ends on the report leaves the run stalled with the findings unactionable until the user prods it, and the report then costs a second render to get back on screen. The closure -cues above (`closes with Tally`, `the skeleton is the whole report`) bound the report's -*shape*; they do not license ending the turn. +cues in the skeleton's rules (`closes with Tally`, `the skeleton is the whole report`) bound +the report's *shape*; they do not license ending the turn. ## Step 6 — Apply menu (single AskUserQuestion, multiSelect; never edit during review) @@ -658,51 +228,12 @@ origin**. Only offer a category when you actually have findings that fall into i accepts at most four options** — the four canonical risk buckets below are the whole menu; never add a fifth. `Report only` is always offered: -- **Safe fixes** — mechanical, easy to eyeball: quality `openness`, - `explaining-variable`, `magic-literal`, `role-name`, `guard-clause`, - verified-redundant `needless-cast`, trivial `over-complex`, and `dead-code` that is an - unread binding or an always-true/false guard; **plus** comment - **REMOVE** and **REWRITE**, and a comment **ADD** whose rationale the review - actually confirmed — locate the code site by content and insert the comment - above it. An `ADD` whose WHY you could only guess is **report-only**: hand the - author the suggested text, since only they know the real reason. -- **Walk the structural ones (one at a time)** — riskier, they move or remove code: - `ordering`, `composed-method` extraction, `command-query` splits, `style-mix` / - `full-construction` / `leaky-collection` reshaping, the `patterns` refactors - (`composition`, `polymorphism`, `execute-around`), large `over-complex` - unifications, `test-structure` restructuring, and `dead-code` removal of a branch that - looks reachable; the cross-file `module` and `objects` rules (`dependency-direction`, - `misplaced-logic`, `canonical-helper`, `pass-through`, `feature-envy`, `data-clump`, - `message-chain`); every **`performance`** fix; every **`security`** fix; **plus** - comment **MOVE**. -- **Boy-scout extras** — apply the untouched-code findings, or skip them. **Risk sorts this - bucket too.** Only the mechanical ones — the same edits Safe fixes accepts — travel as a batch; - a boy-scout finding whose fix moves, removes or restructures code, or touches `security`, joins - the structural walk and is applied one at a time with its own yes. Untouched code is where the - review understands the least, so a structural edit there is riskier than the same edit inside - the diff, not safer: one run bundled a client split into this bucket, silently broke a - double-submit guard, dragged an unrelated page into the pull request, and the user discarded - the work. -- **Report only** — change nothing. - -**Route any unlisted rule by the fix's risk, not its family:** a mechanical, eyeball-able -edit (a rename, a named constant, deleting an unread binding) → Safe fixes; anything that -moves or restructures code, or removes a branch that looks reachable → structural. A -`standards` finding is an unlisted rule and routes the same way. - -**Security is never a Safe fix.** However small the edit looks — a bound parameter, a -removed literal — it changes behaviour at a boundary, so a `security` finding always -walks structurally, one at a time. When a canonical bucket is empty, `security` may take -the freed slot as its own option, **Security fixes (walk one at a time)**, so the user -can pick it apart from the craft restructuring. A `secret-in-source` fix removes the -literal from the file and nothing more: the wrap-up states that **rotating the exposed -secret is the user's step** — the review cannot do it and must not imply it did. - -**`spec` findings are report-only.** A missing or partial requirement is work to do, -not an edit to apply, and never enters a bucket. The one exception is a -`wrong-implementation` the review **verified** in Step 4 whose fix is a **single edit**: -that one is offer-able through the escape hatch below for a confirmed correctness -problem. +- **Safe fixes**, **Walk the structural ones (one at a time)**, **Boy-scout extras** and + **Report only** — what each holds, how an unlisted or `standards` rule routes, why `security` + is never safe and `spec` is report-only are the **Fix risk** section of + `${CLAUDE_PLUGIN_ROOT}/references/merge-contract.md`; read it before composing the menu. When a + canonical bucket is empty, `security` may take the freed slot as its own option, **Security + fixes (walk one at a time)**, so the user can pick it apart from the craft restructuring. **Degenerate and edge menus.** The four buckets are a ceiling, not a quota, and the menu must stay honest when findings don't spread across them: diff --git a/plugins/code-review/evals/README.md b/plugins/code-review/evals/README.md index 081a4a8..f61df6b 100644 --- a/plugins/code-review/evals/README.md +++ b/plugins/code-review/evals/README.md @@ -23,8 +23,15 @@ evals/ prompts/standards.txt # quality trigger with the standards fixture dir as repo root fixtures/ # inputs; fixtures/spec/ and fixtures/standards/ are multi-file scope-mix/ # eval-19 input, kept out of fixtures/ so its paths classify by kind + prompts/headless.txt # cr-prepare → cr-scan × N → cr-merge, in that order — headless track + checks/ # deterministic asserts that read files on disk (headless track) + reset-sandbox.sh # rebuilds .sandbox/headless, the git checkout eval-20 reviews ``` +The headless skills review committed changes only, so eval-20 runs against a git +checkout rather than a fixture path; `scripts/run-evals.sh code-review` rebuilds it +before every run. + Node dev deps (`@anthropic-ai/claude-agent-sdk` + `promptfoo`) and the run scripts live at the **repo root** (`package.json`, single shared `node_modules`), not per-plugin. diff --git a/plugins/code-review/evals/checks/headless-pipeline.mjs b/plugins/code-review/evals/checks/headless-pipeline.mjs new file mode 100644 index 0000000..8d8f32a --- /dev/null +++ b/plugins/code-review/evals/checks/headless-pipeline.mjs @@ -0,0 +1,46 @@ +import { execFileSync } from 'node:child_process'; +import fs from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const LENSES = ['comments', 'readability-tests', 'naming-module', 'objects-patterns', 'simplicity-types', 'security', 'performance', 'spec']; +const FINDING = /^- (high|medium|nit) · [\w-]+ · [\w-]+ · \S+:L\d+(?:-\d+)? · (safe|structural|report-only)\b.* — /m; + +export default (output, context) => { + const { checkout, context: dir } = context.vars; + const root = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', '..', '..', '..'); + const ctx = path.resolve(root, dir); + const failures = []; + const check = (ok, msg) => { + if (!ok) failures.push(msg); + return ok; + }; + + const asked = (context.providerResponse?.metadata?.toolCalls || []).filter((c) => c.name === 'AskUserQuestion'); + check(asked.length === 0, `a headless skill asked ${asked.length} question(s)`); + + const scopePath = path.join(ctx, 'scope.json'); + if (check(fs.existsSync(scopePath), 'cr-prepare wrote no scope.json')) { + const scope = JSON.parse(fs.readFileSync(scopePath, 'utf8')); + const active = scope.lenses.active.map((l) => l.lens); + const named = [...active, ...scope.lenses.inactive.map((l) => l.lens)].sort(); + check(JSON.stringify(named) === JSON.stringify([...LENSES].sort()), `scope.json does not account for all eight lenses once: ${named.join(', ')}`); + check(!active.includes('spec'), 'the spec lens is active with no spec named'); + check(scope.files.map((f) => f.path).sort().join() === 'src/quality-recall.ts,src/security-recall.ts', `judged files are not the committed change: ${scope.files.map((f) => f.path).join(', ')}`); + for (const lens of active) { + const p = path.join(ctx, `${lens}.md`); + check(fs.existsSync(p) && /^Lens: /m.test(fs.readFileSync(p, 'utf8')), `cr-scan left no ${lens}.md with its Lens header`); + } + } + + const report = path.join(ctx, 'report.md'); + check(fs.existsSync(report) && /Reconciliation/.test(fs.readFileSync(report, 'utf8')), 'cr-merge wrote no report.md with a Reconciliation line'); + check(/status: merged/.test(output), 'cr-merge did not return status: merged'); + check(FINDING.test(output), 'no finding line in the fixed `severity · family · rule · path:L · class — fix` shape'); + check(/^- high · security · /m.test(output), 'the planted security finding did not come through the merge'); + + const dirty = execFileSync('git', ['-C', path.resolve(root, checkout), 'status', '--porcelain'], { encoding: 'utf8' }).trim(); + check(dirty === '', `the review edited the checkout: ${dirty}`); + + return { pass: failures.length === 0, score: failures.length === 0 ? 1 : 0, reason: failures.join('; ') || 'all checks passed' }; +}; diff --git a/plugins/code-review/evals/promptfooconfig.yaml b/plugins/code-review/evals/promptfooconfig.yaml index f2cd5e6..f83e5a3 100644 --- a/plugins/code-review/evals/promptfooconfig.yaml +++ b/plugins/code-review/evals/promptfooconfig.yaml @@ -24,6 +24,10 @@ prompts: # for the CODING_STANDARDS pair. - id: file://prompts/standards.txt label: standards-track + # Headless track: the three model-only skills a workflow drives, run in sequence against a + # committed change in a git sandbox that reset-sandbox.sh rebuilds. + - id: file://prompts/headless.txt + label: headless-track providers: - id: anthropic:claude-agent-sdk # alias: anthropic:claude-code @@ -805,3 +809,14 @@ tests: - type: regex value: '[Ss]kipped' + - description: 'eval-20 headless-pipeline (cr-prepare → cr-scan × N → cr-merge)' + prompts: [headless-track] + vars: + checkout: plugins/code-review/evals/.sandbox/headless + context: plugins/code-review/evals/.sandbox/headless-ctx + # Eight lenses in one agent's context: the default cap would abort mid-run. The skills write + # their context files; without Write and Bash the agent narrates a review it never ran. + options: { max_budget_usd: 12, allow_all_tools: true } + assert: + - type: javascript + value: file://checks/headless-pipeline.mjs diff --git a/plugins/code-review/evals/prompts/headless.txt b/plugins/code-review/evals/prompts/headless.txt new file mode 100644 index 0000000..03d5e8f --- /dev/null +++ b/plugins/code-review/evals/prompts/headless.txt @@ -0,0 +1,10 @@ +Review the last commit of the git checkout {{checkout}} with the code-review plugin's headless +skills, exactly in this order, through the Skill tool: + +1. `code-review:cr-prepare` with `--base HEAD~1 --out {{context}} -C {{checkout}}`. +2. `code-review:cr-scan` with `--lens --context {{context}}`, once for every lens its + closing block lists as active. +3. `code-review:cr-merge` with `--context {{context}}`. + +Then reply with the three skills' closing blocks, verbatim, in that order — the cr-scan blocks +one after another — and nothing else. diff --git a/plugins/code-review/evals/reset-sandbox.sh b/plugins/code-review/evals/reset-sandbox.sh new file mode 100755 index 0000000..d632f19 --- /dev/null +++ b/plugins/code-review/evals/reset-sandbox.sh @@ -0,0 +1,22 @@ +#!/usr/bin/env bash +# Rebuild the git checkout the headless-track eval reviews: a base commit, then one commit that adds +# two fixtures with planted findings. The headless skills review committed changes only, so the +# file-path fixtures the other tracks read cannot serve them. scripts/run-evals.sh runs this first. +set -euo pipefail +cd "$(dirname "$0")" + +rm -rf .sandbox +repo=.sandbox/headless +mkdir -p "$repo/src" +git -C "$repo" init -q -b main +printf '# orders service\n' > "$repo/README.md" +git -C "$repo" add -A +git -C "$repo" -c user.name='cr-evals' -c user.email='cr-evals@localhost' commit -q -m 'base' --no-gpg-sign + +# Under src/, not fixtures/: scope.md classes anything below a fixtures/ directory as test code. +cp fixtures/security-recall.ts "$repo/src/security-recall.ts" +cp fixtures/quality-recall.ts "$repo/src/quality-recall.ts" +git -C "$repo" add -A +git -C "$repo" -c user.name='cr-evals' -c user.email='cr-evals@localhost' commit -q -m 'feat: add order lookup and pricing' --no-gpg-sign + +echo "code-review sandbox reset: $(pwd)/$repo" diff --git a/plugins/code-review/references/merge-contract.md b/plugins/code-review/references/merge-contract.md new file mode 100644 index 0000000..0bc21e4 --- /dev/null +++ b/plugins/code-review/references/merge-contract.md @@ -0,0 +1,283 @@ +# Merge and report contract + +How the Scanners' outputs become one review: the merge, which dedups, re-grades and reconciles, +then the report it renders. Two callers use it: `/start-cr` Steps 4–5, and the `cr-merge` skill. The +scope resolution, the conventions read and the lens set it refers to come from whoever prepared the +review — `/start-cr` Steps 1–2b, or `cr-prepare`. + +Every path in this file is relative to the directory it sits in, the plugin's `references/`. + +## Merge and re-grade + + +- **Collect** all N Scanners' outputs — every active lens's `` actually in hand + as the dispatcher collected it, not merely a notification that fired; a lens you could not collect is a + labelled degradation you already surfaced to the user, never a silent gap in the merge. +- **Dedup overlaps**: when two findings point at the same code — including across + different lenses, and across **all eleven families**, craft and `security` / + `performance` / `spec` / `standards` alike — keep the **most-specific** one and drop + the rest. When the overlap + spans two severities (a `high` symptom folding into a lower-severity root cause, or the + reverse), the surviving bullet keeps the **highest** severity of the overlap — deduping + must never quietly demote a `high` under a `medium`. +- **Count the lenses that converged.** Independent Scanners landing on the same code + is the strongest signal this review produces — they read the file separately and had + no way to coordinate. Treat a finding several lenses reached (directly or via + `HANDOFF`) as **confirmed**: it leads its file, and it is a candidate for the + headline. Convergence raises confidence and ordering, **never severity** — that stays + verbatim from the table. +- **Route every `HANDOFF` and every `CANDIDATES` entry to a visible home.** Assign a + `HANDOFF` its correct family and rule; decide a candidate against its rule's + calibration. Either way it lands in exactly one of two places: a graded bullet in the + per-file report (on its own, or merged into a converging finding), or a `Not flagged` + line with its one-line reason. **The entry no primary finding corroborates is the one + that slips, so reconcile by an itemized check, not by assertion.** Before rendering, + write the check out: enumerate every `HANDOFF` and every candidate you received, and + against each name its home — the report bullet (`path:line`) it became, the converging + finding it merged into, or the `Not flagged` line that clears it. An entry with no home + on that list is a bug: route it before you render. +- **A primary finding is reconciled too.** The channels are not the only thing that goes + missing: a Scanner's own `FINDINGS` entry can fall out of the merge between collecting and + rendering, and nothing downstream notices. Count what you received per Scanner, and give every + primary finding that does not reach a report bullet — deduped into another, demoted, or + rejected — its own `Not flagged` entry with the reason. Dedup is the one silent case allowed, + and only because the surviving bullet carries it. +- **Publish that check as one counted line above the report** — `Reconciliation: N + handoffs + M candidates → A merged · B own bullet · C boy-scout · D Not flagged; P primary + dropped` — where `A + B + C + D` equals `N + M`, and `P` counts the primary findings that got + no bullet. The arithmetic is what makes the check real: a + run that states "every handoff routed" without it has asserted rather than reconciled, + and loses the entry nothing else corroborates. When the sums disagree, an entry is + unrouted — find it, never adjust a number to close the gap. +- **Each count names the block it is counted in**, so the line can be checked against the report + rather than believed: `merged` is an entry folded into another finding's bullet and visible in + its text, `own bullet` one that became its own graded bullet under a file, `boy-scout` one + rendered in the `Boy-scout` block, `Not flagged` one rendered as its own entry in `Not + flagged`. Runs whose arithmetic was right have still printed `0 boy-scout` over a Boy-scout + block holding three routed handoffs, and counted six entries as `merged` into a bullet that + was never rendered. Before publishing, count the rendered blocks: `C` equals the Boy-scout + entries that came from a channel, and `D + P` equals the entries in `Not flagged`. A count + that does not match the block it names is the bug, not the block. +- **Resolve every `(verify)` finding**: read the code and confirm or refute it. A + confirmed finding drops the marker and proceeds; a refuted one is a **Scanner false + positive** — drop it and note it under `Not flagged`. An unresolved `(verify)` finding + never reaches an apply batch. Most runs will have none — the Scanners resolve their own + doubts. When no Scanner emitted one, say nothing about `(verify)` anywhere: do not + claim to have resolved an empty list, and do not relabel some other mechanism as a + `(verify)` — a routed `HANDOFF`, a decided candidate, or a refuted scanner doubt is + resolved under its own name. +- **Re-grade every quality finding's severity yourself** against the master table in + `severity.md` — read it now if you have not. It + carries the 44 rows, what each severity means, the anti-anchoring rule, and the + **`standards` keyword mapping** (MUST / MUST NOT / NEVER / ALWAYS → high, SHOULD → + medium, MAY / prefer / consider → nit, no keyword → medium). A `standards` finding has + no fixed row: re-grade it against that mapping by re-reading the rule it quotes, not + the Scanner's guess. A single-lens Scanner is the one most prone to the anchoring that + table forbids, so its severity is a first pass and yours is the one that ships. +- **Judge the fix, not only the finding.** A finding can be right and its fix wrong, and the apply + phase is too late to notice: by then the user has approved it. For every fix that could reach a + bucket, check three things against the code you already read: + - **Does it keep behaviour?** Moving a guard onto a DTO turns a 400 into a 422; splitting a + shared client drops the double-submit guard that shared instance provided; deleting an unused + export removes what a later stage of the same spec consumes. A fix that changes what callers + observe is not mechanical, whatever its rule says. + - **Does it contradict another finding?** One review's headline fix bounded a payload *before* + the redaction walk, which would have truncated secrets under the redactor's minimum length — + a security hole introduced by a performance fix. Read the fixes as a set, not one at a time. + - **Does it create the next finding?** An extraction that takes five positional parameters, a + helper that duplicates one two files away — fix the fix before offering it. + + A fix that fails any of the three is re-routed: to the structural walk with the behaviour + change named in its option, or to report-only with one line on why. Say which in the report's + bullet rather than silently dropping the finding. +- **Comment verdicts are not re-graded** and are **not** mapped to severities. The + two vocabularies stay side by side; there is no severity↔verdict mapping + anywhere in a review. + +## Fix risk + +Every finding the merge keeps carries one risk class for its fix, and whoever applies fixes cuts by +it — `/start-cr`'s apply menu offers the classes as its buckets, and a headless caller reads the +class from the `cr-merge` return. The buckets below map to the classes: **Safe fixes** → `safe`, +**Walk the structural ones** → `structural`, **Report only** and every `spec` finding → +`report-only`; a boy-scout finding takes the class its fix would have inside the diff, raised to +`structural` where the text below says so. A fix that failed any of the three checks under *Judge +the fix* is `report-only` too, with its one-line reason. + +- **Safe fixes** — mechanical, easy to eyeball: quality `openness`, + `explaining-variable`, `magic-literal`, `role-name`, `guard-clause`, + verified-redundant `needless-cast`, trivial `over-complex`, and `dead-code` that is an + unread binding or an always-true/false guard; **plus** comment + **REMOVE** and **REWRITE**, and a comment **ADD** whose rationale the review + actually confirmed — locate the code site by content and insert the comment + above it. An `ADD` whose WHY you could only guess is **report-only**: hand the + author the suggested text, since only they know the real reason. +- **Walk the structural ones (one at a time)** — riskier, they move or remove code: + `ordering`, `composed-method` extraction, `command-query` splits, `style-mix` / + `full-construction` / `leaky-collection` reshaping, the `patterns` refactors + (`composition`, `polymorphism`, `execute-around`), large `over-complex` + unifications, `test-structure` restructuring, and `dead-code` removal of a branch that + looks reachable; the cross-file `module` and `objects` rules (`dependency-direction`, + `misplaced-logic`, `canonical-helper`, `pass-through`, `feature-envy`, `data-clump`, + `message-chain`); every **`performance`** fix; every **`security`** fix; **plus** + comment **MOVE**. +- **Boy-scout extras** — apply the untouched-code findings, or skip them. **Risk sorts this + bucket too.** Only the mechanical ones — the same edits Safe fixes accepts — travel as a batch; + a boy-scout finding whose fix moves, removes or restructures code, or touches `security`, joins + the structural walk and is applied one at a time with its own yes. Untouched code is where the + review understands the least, so a structural edit there is riskier than the same edit inside + the diff, not safer: one run bundled a client split into this bucket, silently broke a + double-submit guard, dragged an unrelated page into the pull request, and the user discarded + the work. +- **Report only** — change nothing. + +**Route any unlisted rule by the fix's risk, not its family:** a mechanical, eyeball-able +edit (a rename, a named constant, deleting an unread binding) → Safe fixes; anything that +moves or restructures code, or removes a branch that looks reachable → structural. A +`standards` finding is an unlisted rule and routes the same way. + +**Security is never a Safe fix.** However small the edit looks — a bound parameter, a +removed literal — it changes behaviour at a boundary, so a `security` finding always +walks structurally, one at a time. When a canonical bucket is empty, `security` may take +the freed slot as its own option, **Security fixes (walk one at a time)**, so the user +can pick it apart from the craft restructuring. A `secret-in-source` fix removes the +literal from the file and nothing more: the wrap-up states that **rotating the exposed +secret is the user's step** — the review cannot do it and must not imply it did. + +**`spec` findings are report-only.** A missing or partial requirement is work to do, +not an edit to apply, and never enters a bucket. The one exception is a +`wrong-implementation` the review **verified** in the merge whose fix is a **single edit**: +that one is offer-able through the apply menu's escape hatch for a confirmed +correctness problem; headless, it classes as `structural`. + +## Report — one per-file skeleton, two vocabularies side by side + + +Group by **file**, not by Scanner. Under each file, list quality findings and +comment verdicts **together**. Render with **exactly this template**, in this +order — keep the structure identical between runs: + +```markdown +Reconciliation: handoffs + candidates → merged · own bullet · boy-scout · Not flagged;

primary dropped + +## Code review — + +**Conventions:** +**Headline:** + +### +- `family` · rule · severity · L — → +- `comments` · R# · KEEP/REMOVE/REWRITE/MOVE/ADD · L — → + +### +- `family` · rule · severity · L — <…> + +**Not flagged:** + +**Boy-scout (untouched code, optional):** +- `family` · rule · :L — + +**Tally:** N quality findings (H high · M medium · K nit) · C comments (X remove · Y rewrite · Z move · V add · W keep) · F files. Lenses: L of 8 (skipped: — ). Spec: R of T requirements met. Skipped: . +``` + +A filled-in report reads like this: + + +Reconciliation: 4 handoffs + 2 candidates → 3 merged · 1 own bullet · 0 boy-scout · 2 Not flagged; 0 primary dropped + +## Code review — committed (base → HEAD), 3 files + +**Conventions:** repo `CLAUDE.md` documents barrel exports as the public-API style, so `module` · barrel is not flagged here. +**Headline:** `checkout/total.ts` concatenates the request's coupon code into a raw SQL string at L72. + +### src/checkout/total.ts +- `security` · injection-sink · high · L70, L72 — `couponCode` read from `req.query` at L70 reaches the raw `WHERE` string at L72 by concatenation → bind it as a query parameter +- `simplicity` · over-complex · high · L18, L34, L51 — three copies of the tier-discount branch drift independently → collapse into `discountFor(tier)` and call it at each site +- `readability` · magic-literal · medium · L22 — `0.1` carries the gold-tier rate with nothing naming it → name `GOLD_DISCOUNT_RATE` +- `comments` · R1 · REMOVE · L17 — "// multiply by the rate" restates the line beneath it → delete these lines + +### src/checkout/receipt.ts +- `naming` · role-name · nit · L9 — `receiptArray` names the type instead of the role → `receipts` +- `comments` · R2 · ADD · L44 — the 250 ms retry gap is a gateway constraint no reader can infer → "// 250 ms — the gateway rejects retries closer than its own debounce window" + +### docs/checkout-spec.md +- `spec` · missing-requirement · high · L14 — "A receipt lists the discount applied per line item" has no implementation in the diff → add the per-line discount to `Receipt` + +**Not flagged:** `JSON.parse(raw) as Config` at L7 (boundary narrowing, not `needless-cast`); the exhaustive `default:` throw at L61 (defensive assertion, not `dead-code`). + +**Tally:** 5 quality findings (3 high · 1 medium · 1 nit) · 8 comments (1 remove · 0 rewrite · 0 move · 1 add · 6 keep) · 3 files. Lenses: 8 of 8. Spec: 4 of 5 requirements met. Skipped: pnpm-lock.yaml (lockfile). + + +**The skeleton is the whole report.** It has no other sections: no `### Findings` +header, no numbered or bolded finding entries, no `---` rules between findings, no +per-finding code block, no closing summary. The `###` headers are **file paths** — one +per reviewed file, plus the **spec's own path** when `--spec` was given and a +`missing-requirement` needs a home — and each finding is a single markdown bullet +beneath its file. Do +not paste the code under review, the rewritten body, or a before/after block: a finding +that seems to need a code block is one whose fix is not yet stated as a clause, so state +it as a clause. Every report opens with `Conventions` and `Headline`, and closes with +`Tally`. The `Reconciliation` line is the only thing that precedes `## Code review` — it +belongs to the merge's check rather than to the report, which is why it carries counts and +not prose. + +Rules for filling it in: + +- **Two vocabularies, side by side.** Quality findings use `` `family` · rule · + severity ``, with the family **backticked** — one of the eleven fixed labels + `readability`, `tests`, `naming`, `module`, `objects`, `patterns`, `simplicity`, + `security`, `performance`, `spec`, `standards` — and rule and severity verbatim from + `references/severity.md` (a `standards` rule is its slug, graded by the keyword + mapping). Comment verdicts use `` `comments` · R# · KEEP/REMOVE/REWRITE/MOVE/ADD ``. + **No severity↔verdict mapping** — keep them distinct. +- **Findings are markdown bullets** under a `###` file header (not inside a ``` + fence) so every `path:line` stays clickable. A `spec` · missing-requirement bullet + sits under `### ` with the spec's own lines; every other spec finding sits + under the code file it points at. +- **Order files** by their highest-severity quality finding; a REMOVE/REWRITE/MOVE/ADD + comment weighs like a medium for ordering. Within a file: a `security` high first, + then any **R9 (contradicts-the-code)** comment verdict, then high → medium → nit, + then by line. +- **Collapse repeats**: one `family` · rule breaking in several spots is a single + bullet with the lines listed together (`L20, L34, L51`). +- **The fix is a clause, not code.** "extract + `transitionOrReportConflict(...)` and early-return at each site", "drop the `as + User` cast", "name `SECONDS_PER_DAY`". Keep a rewritten body or a before/after + block out of the report. For a comment REWRITE the fix is the exact replacement + text; for MOVE, name the destination. +- **Quote comments verbatim.** Every comment verdict carries the verbatim comment + text and its `path:line`. +- **`Not flagged`** lists the look-alikes deliberately passed on, plus every candidate, + `HANDOFF` and dropped primary finding the merge cleared — one line when they are all genuine + non-findings, a short bullet each when one of them is a *real* problem that merely has no rule + to land on. **Its entries stay countable**: separated by `;` on the one-line form, one bullet + each otherwise, because the `Reconciliation` line's last two numbers are checked against them. A real problem keeps its own bullet rather than being compressed into a + subordinate clause; that compression is how something worth acting on disappears. Drop + the block if empty. +- **`Boy-scout`** holds only findings in code the change did not touch; omit the + whole block when there are none. +- **Resolved findings only.** The body lists confirmed findings; a refuted one goes in + `Not flagged` as a Scanner false positive. +- **The headline may not contradict the combined tally.** If there is any quality + `high` or `medium` finding, **or** any comment REMOVE / REWRITE / MOVE / ADD, the + headline names the worst one — it must not call the change "clean", + "well-structured", or "only cosmetic nits". A confirmed **`security`** finding is the + headline over any craft finding, whatever their severities — and so is a confirmed + **exposure that no rule names**, which leads the report from its own `Not flagged` + bullet rather than being demoted for want of a tag; a `spec` · + missing-requirement or wrong-implementation forbids the clean headline outright. + Reserve the clean verdict for a tally that is genuinely nits-only-and-all-KEEP (or + empty). +- **The `Tally` names the lenses.** `Lenses: L of 8` always, with each skipped Lens + and its lens-set reason in the parenthesis (`skipped: performance — no executable + code; spec — no spec named`); drop the parenthesis when all eight ran. When a spec was + given, add the `spec` Scanner's met-requirements count as `Spec: R of T requirements + met`; omit that clause otherwise. + +Collapse the whole report to the title line plus a one-sentence verdict and the +tally **only when the change reads cleanly** — the quality tally is empty or +nits-only and every comment is KEEP, and no `spec` · missing-requirement or +wrong-implementation stands. Match the report to what you found: neither pad +a clean one to look thorough, nor collapse one carrying a medium-or-higher finding, a +spec gap, or a REMOVE/REWRITE/MOVE/ADD to look clean. diff --git a/plugins/code-review/references/review-setup.md b/plugins/code-review/references/review-setup.md new file mode 100644 index 0000000..79c528e --- /dev/null +++ b/plugins/code-review/references/review-setup.md @@ -0,0 +1,63 @@ +# Review setup — the conventions note, the standards slot, the active lens set + +What a review settles once, after its file list is resolved and before any Scanner runs. Two +callers use it: `/start-cr` Steps 2 and 2b, and the `cr-prepare` skill. Every path in this file is +relative to the directory it sits in, the plugin's `references/`. + +## The conventions note and the standards slot + +`scope.md` carries the **mechanical convention +read** (the exact paths, root first) and the **language-applicability** rules for +families or rules that have no counterpart in the language under review. That read +now opens with the **standards pair** at the repository root — `CODING_STANDARDS.md`, +then `CODING_STANDARDS.local.md` — which LAYER: both apply, and where two statements +conflict the `.local` one wins. Work it there, then: + +- capture what you learned in one short **conventions note**, and **pass it to every + Scanner** so a documented convention never surfaces as a finding; the note also + records a **tracked `.local` file** (`git check-ignore` fails on it) and any + **conflict between two project files** (resolved by scope.md's precedence order), + both of which reach the report's `Conventions` line; +- **one note, byte-identical in every brief, and it may only suppress.** Write it once and paste + the same text into all N briefs: a per-Lens note is a per-Lens instruction, and the Scanner + reads whatever it finds there as what you want it to look for. So the slot holds nothing but + documented conventions, each **quoted verbatim with its file** — never your own threat + hypotheses or "where to focus", never an "established facts — do not raise" list, never a + paraphrase of a rule (one run's paraphrase said a legacy pattern "is documented as accepted" + where the rule said to migrate off it, and buried the very finding the user later asked for). + Anything you want checked belongs in the Lens's own rules file, not here. A note that grows + past a screen is the wrong shape: cut it to the rules that actually suppress something; +- name any family or rule the language makes **N/A** in that note, so its owning + Scanner clears it in one line instead of inventing findings to fit; +- keep the standards text **out of the note**: it travels in the brief's own + `` slot because, unlike everything else the read picks up, it + **generates** findings. A Scanner raises `` `standards` · · `` only for + an explicit, quotable rule inside its own Lens's subject, citing the file and + section; vague prose ("write clean code") never generates; unsettled fit goes to + `CANDIDATES`; a rule the `.local` file relaxes is suppressed; and a + formatting/whitespace/import-order/quote rule is skipped when a formatter or linter + config exists at the root (scope.md lists the presence check). When the pair is long, + **pre-slice it per Lens** so each Scanner receives only the rules in its subject; a + short pair goes to every Scanner whole. The rest of the conventions — `CLAUDE.md`, + `AGENTS.md`, `CONTRIBUTING.md`, `.cursor/rules`, `.claude/rules` — stay + **suppress-only**: they remove findings, never create them. + +## The active lens set + +Not every Lens runs on every change. Decide the set here, once, from the input — +never from a preference: + +- the five craft Lenses (`comments`, `readability & tests`, `naming & module`, + `objects & patterns`, `simplicity & types`) and **`security`** are **always + active** — six on any change, however small; +- **`performance`** is active iff the `source`-kind subset of the resolved list, + **minus `.sh` files**, is non-empty — a tests-only, IaC-only, or shell-only change + skips it; +- **`spec`** is active iff a spec resolved to a readable local file during the scope resolution — from + `--spec`, or from the offer the user accepted when the change carried its own spec. + +Record **N**, the number of active Lenses, and for each one its own ``: +`performance` gets the source subset it was gated on; every other Lens gets the full +resolved list. Record every **inactive** Lens with its reason (`performance — no +executable code`, `spec — no spec named`); the Tally prints them. From here on +**N** means this count: N Scanners run, N outputs awaited, N outputs merged. diff --git a/plugins/code-review/references/scanner-contract.md b/plugins/code-review/references/scanner-contract.md new file mode 100644 index 0000000..dd6ad6c --- /dev/null +++ b/plugins/code-review/references/scanner-contract.md @@ -0,0 +1,200 @@ +# Scanner contract + +The brief a Scanner receives and the output it returns, for one Lens. Two callers use it: +`/start-cr` dispatches one Scanner per active Lens as a sub-agent, and the `cr-scan` skill runs one +Lens in its own context. Either way the scope resolution (the file list and its `diff_args`), the +conventions note, the standards text and the lens set come from whoever prepared the review — +`/start-cr` Steps 1–2b, or `cr-prepare` — and the merge (`/start-cr` Step 4, or `cr-merge`) is where +severity is re-graded and every side-channel entry finds its home. + +Every path in this file is relative to the directory it sits in, the plugin's `references/`. + +## The Scanner brief + +Send each Scanner a brief in this shape, filling every slot: + +``` + + comments | readability & tests | naming & module | objects & patterns | simplicity & types | security | performance | spec + rules/.md + + + + tracked → `git diff -- ` + untracked → read the file directly; every line is added + + + + + + primary = the problem is in code this change added or modified, or structure + the change introduced or made worse. + boy-scout = a problem in untouched code noticed only while reading for context — + optional, kept strictly separate, never mixed into the primary findings. + A fully added file (status `A`) has no boy-scout findings: the whole file is code + the change introduced, so every finding in it is primary. + + + +``` + +Read the rules file **completely first**, then judge only the families that belong to +that Lens. A Scanner **returns findings/verdicts only**: it does not render a report, +does not re-grade centrally, and **writes nothing into the tree** — not the files under +review, and not a scratch or probe file to test a hypothesis against. + +A Scanner is **one agent, one pass, one output**. It **dispatches no agent of its own** — a +sub-agent puts a second hop between the finding and the merge, and the Scanner that tried it +had its own report overwritten by the follow-up, losing a handoff outright. It does not wait in +the background, poll, or schedule anything; it reads, judges, and returns. Its **final message +is its whole output**: if something has to change after it has already written its findings, +it re-sends the complete list, never an "amendment" or a delta — anything the last message +leaves out never reaches the merge. It is reading the +user's working copy, so it settles a doubt by reading the type, the signature, or the call +site, and marks the rest `(verify)`. Read the whole changed file for context, and target +what the change touched. The `naming & module` Scanner alone adds the **one-hop +cross-file protocol** on top of that: search the importers of each changed module and the +imports of each module it newly imports — with the `Grep` tool, or `git grep` from `Bash` in a +session where that tool is not handed to sub-agents — open those files at the matched lines only — +no transitive crawl, no repo listing, no `find`; a fact beyond the hop is `(verify)`; +it still writes nothing. + +## The eight Lenses + +1. **comments** → `rules/comments.md` + Returns per-comment **VERDICTS**, one per comment: + `` `comments` · R# · KEEP/REMOVE/REWRITE/MOVE/ADD · `path:line` · "verbatim comment" — one-line reason → concrete suggested fix ``. + Run the deletion test on every comment first. Surface **R9 + (contradicts-the-code) findings first**. The **test-file bar is higher (R11)**: + default to REMOVE when unsure in tests. **`ADD` is the one verdict with no + existing comment to quote** — an R2 *missing WHY* at genuinely non-obvious code + (a magic constant, a workaround, a specific timeout/retry/batch size, a silent + catch); it drops the verbatim-comment slot for a site description: + `` `comments` · R2 · ADD · `path:line` — → ``. + Raise `ADD` only where you can state the reason concretely — never a guess + dressed as a WHY. Every suggested fix obeys the comment rules itself: no spec-id + fragments (`(R2)`, `F1:`, `§4.1`), no new file/doc cross-references (R4), no + banners (R5). For MOVE, name the destination and give the exact text to place + there, plus "delete from the declaration". + +2. **readability & tests** → `rules/readability-tests.md` + Judges the `readability` and `tests` families. + +3. **naming & module** → `rules/naming-module.md` + Judges the `naming` and `module` families. + +4. **objects & patterns** → `rules/objects-patterns.md` + Judges the `objects` and `patterns` families. + +5. **simplicity & types** → `rules/simplicity-types.md` + Judges the `simplicity` family. + +6. **security** → `rules/security.md` + Judges the `security` family; always active. A finding names **both** `path:line` + of the **source** (where untrusted data enters) and of the **sink**; a pattern alone + (`req.body`, a string containing `SELECT`) is never a finding; `L` lists both + ends, source first, and the clause says which is which. When either end sits + outside the files in view the Scanner reads it — it can `Read` any file and search with + `Grep` or `git grep` — and marks only what it still cannot confirm `(verify)`. `CANDIDATES` is reserved for a + confirmed source→sink pair whose *mitigation* is the doubt; a cleared look-alike is + one prose line for `Not flagged`. Severity is `high` or `medium`, **never `nit`**. + It never runs the code, an audit tool, or a network command; `.env`, YAML, JSON and + manifests stay skipped, and the report's Skipped line sends those to + `/security-review`. + +7. **performance** → `rules/performance.md` + Judges the `performance` family; active only over the `source` subset the lens set gated it on. + A finding names **four things** — the multiplier (the loop's collection or the + endpoint, and where its size comes from), the call inside it, the bound that is + missing, and the batch/limit API that exists — or it is a `CANDIDATE`. "Could be + slow", "may impact performance", and any estimate not derived from a line in the + diff are forbidden; it never runs or profiles code. + +8. **spec** → `rules/spec.md` + Judges the `spec` family; active only with `--spec`. It enumerates the requirements + in the `` slot and maps each to the diff. **Every finding quotes the spec line + verbatim.** A `wrong-implementation`, `partial-requirement`, or `scope-creep` sits + under the code file it points at; a `missing-requirement` has no code site, so it + sits under a `### ` header with the **spec's own `L`**. The + requirements met come back as **one prose count line**, never as findings. + `scope_split` is **N/A** for this Lens — a spec finding is neither primary nor + boy-scout, so it returns one list. Runtime claims are `(verify)`; a PARTIAL-vs-WRONG + doubt is a candidate; a craft problem noticed on the way is a `HANDOFF`. + +For the finding-shaped Lenses (2–8) the Scanner returns **FINDINGS**, split into primary +and boy-scout (the `spec` Lens excepted), each in this exact shape: + +``` +`family` · rule · severity · L — → +``` + +A **`standards` finding** — any Lens may raise one, from the `` slot only — +puts the quoted rule and its source where the loss goes: + +``` +`standards` · · · L — "" (CODING_STANDARDS.md ›

) → +``` + +with a short kebab-case slug from the rule's wording and the severity from the keyword +mapping in `references/severity.md`. + +**Severity is exactly one of `high`, `medium`, or `nit`** — never `low`, never a +number, never a paraphrase. A Scanner whose own rules file happens to list only one +of the three still uses the full vocabulary. Tell each Scanner that **severity is a +first pass** — you re-grade every quality finding centrally in the merge, so it grades +honestly against its rules without agonizing over the boundary. + +**The FINDINGS section holds findings only.** Anything a Scanner checked and cleared +belongs in one prose line, never in the finding shape — a "none found" or "is **not** +a finding" bullet with a dash where the severity goes reads as a finding to everything +downstream. + +**A duplication finding sweeps the whole file.** "Target what the change touched" holds +for most rules, but duplication is the exception: when you flag repeated code (an +`over-complex` duplication, a copy-pasted predicate), scan the **rest of the file** for +every other copy of the same pattern and list all the call sites in the one finding — +including copies in code the change didn't touch. A finding that names two of three +copies makes the extraction fix leave a straggler behind. The one **cross-file** +exception is `module` · canonical-helper: a new helper duplicating an exported helper +elsewhere in the repo is found by the one-hop Grep, bounded to the helper's name and its +distinctive expression — never a repo-wide sweep, and inconclusive means `(verify)`. + +## Three side-channels, three distinct meanings + +Report what you find and let the merge filter it. Each Scanner judges against its +rules, then against each rule's own calibration paragraph — the look-alike that is +*not* a violation. Calibration clearing a site makes it a non-finding. Anything left +unsettled travels in one of three channels, and these are **not** interchangeable: + +| channel | means | +|---------|-------| +| `(verify)` | the **fact** is unconfirmable here — runtime behaviour, or a file outside the review scope | +| `HANDOFF` | confirmed, but **another Lens's family** owns it | +| `CANDIDATES` | confirmed and mine, but the **rule fit or its calibration** is a judgment call | + +- **`(verify)` marker** — a Scanner that doubts a finding **resolves it itself first**: + it has `Read`, so it opens the type, the signature, or the call site and confirms or + drops it (a `needless-cast` is the common case — check what the value's type actually + is before claiming the cast is redundant). It appends `(verify)` only when confirming + would take something it does not have. The merge resolves those. +- **`CANDIDATES` block** — a site that survives the deletion of doubt about the *facts* + but that the Scanner cannot settle against the rule's calibration. It belongs here + rather than in the bin: you decide it with the whole review in view, and a candidate + you reject costs one line in `Not flagged`, while one the Scanner never reported costs + the finding outright. + + ``` + ## CANDIDATES (rule fit or calibration uncertain — orchestrator decides) + - `family` · rule · `path:line` — → + ``` + +- **`HANDOFF` block** — a real problem that belongs to another Lens's family, in a + separate block at the end of the output, never mixed into the Scanner's own findings + and never buried in prose: + + ``` + ## HANDOFF (out-of-my-family — noticed but not mine to grade) + - `` · · `path:line` — → + ``` + +One terse line each. Omit a block when it is empty. diff --git a/plugins/code-review/scripts/get_changes.py b/plugins/code-review/scripts/get_changes.py index 26ba215..6ba7bdc 100755 --- a/plugins/code-review/scripts/get_changes.py +++ b/plugins/code-review/scripts/get_changes.py @@ -2,7 +2,7 @@ """Enumerate changed files for a review scope. Usage: - get_changes.py --scope {uncommitted|committed|both} [--base REF] + get_changes.py --scope {uncommitted|committed|both} [--base REF] [-C PATH] Scopes: uncommitted working tree + index vs HEAD (git diff HEAD) @@ -44,6 +44,7 @@ import argparse import json +import os import subprocess import sys from typing import Optional @@ -179,7 +180,10 @@ def main() -> int: p = argparse.ArgumentParser() p.add_argument("--scope", required=True, choices=["uncommitted", "committed", "both"]) p.add_argument("--base", default=None, help="explicit base ref (default: auto-detect)") + p.add_argument("-C", dest="repo", default=None, help="run as if started in PATH, like git -C") args = p.parse_args() + if args.repo: + os.chdir(args.repo) inside = _run(["git", "rev-parse", "--is-inside-work-tree"], check=False).strip() if inside != "true": diff --git a/plugins/code-review/scripts/tests/test_get_changes.py b/plugins/code-review/scripts/tests/test_get_changes.py index e0e5930..28137d2 100644 --- a/plugins/code-review/scripts/tests/test_get_changes.py +++ b/plugins/code-review/scripts/tests/test_get_changes.py @@ -85,3 +85,16 @@ def test_explicit_base_never_gets_an_alternate(origin_repo: Path): assert out["count"] == 0 assert "alternate" not in out + + +def test_dash_c_reviews_another_checkout(origin_repo: Path, tmp_path: Path): + elsewhere = tmp_path / "elsewhere" + elsewhere.mkdir() + + res = subprocess.run( + [sys.executable, str(SCRIPT), "-C", str(origin_repo), "--scope", "committed", + "--base", "origin/main"], + cwd=elsewhere, capture_output=True, text=True, check=True, + ) + + assert [f["path"] for f in json.loads(res.stdout)["files"]] == ["feature.ts"] diff --git a/plugins/code-review/skills/cr-merge/SKILL.md b/plugins/code-review/skills/cr-merge/SKILL.md new file mode 100644 index 0000000..bdf18cf --- /dev/null +++ b/plugins/code-review/skills/cr-merge/SKILL.md @@ -0,0 +1,60 @@ +--- +name: cr-merge +description: >- + Headless last step of a code review that another workflow drives — merges the lens outputs + cr-scan wrote into one re-graded report, writes it into the context directory, and returns every + finding with its fix and risk class in a fixed line shape. Asks nothing and applies nothing. Not + for interactive use: a person reviews with /start-cr. +argument-hint: "--context " +user-invocable: false +allowed-tools: Read, Bash, Grep, Glob, Write +--- + +# cr-merge — one report from N lenses + +Arguments: `$ARGUMENTS` + +You do `/start-cr`'s Steps 4–5 for a caller that applies fixes itself: nobody reads a question you +ask, and nothing you do edits the checkout. + +A missing `--context`, or a directory without `scope.json`, ends the run with `status: error` and +the reason. + +## 1. Check that every lens delivered + +`scope.json`'s `lenses.active` names the lenses that had to run. Each needs `.md` in the +context directory, opening with its `Lens:` header line. A file that is missing, has no header, or +presents itself as an amendment or a partial list is a lens that has **not** reported: do not merge +around it. Return `status: incomplete` with those lenses named and write no report — the caller +runs them again. A review missing a lens is never passed off as whole. + +## 2. Merge and render + +Read `${CLAUDE_PLUGIN_ROOT}/references/merge-contract.md` completely and follow it: the **Merge and +re-grade** half over the lens files, then **Fix risk** to give every kept finding its class, then +the **Report** half's skeleton. Pre-read the change the way `/start-cr` does while its Scanners +run — `git -C diff -- ` per file in `scope.json` — so `(verify)` +resolution and the fix checks read the code, not the lens's account of it; the file-growth check +runs here too. Its `Conventions` line comes from `conventions.md`, and the `Tally`'s lens count and +skipped reasons from `scope.json`. + +Write the rendered report, `Reconciliation` line first, to `/report.md` with the `Write` +tool. That and nothing else is what you write. + +## 3. Return + +End this skill with this block and nothing after it. The block closes this skill, not the turn: when the prompt that invoked it names further steps, carry them out after it. One line per finding the report keeps, boy-scout +included, in the report's order: + +``` +status: merged | incomplete | error +report: /report.md +lenses: of 8 (skipped: ) +findings: +- · · · :L · — +- comments · R · · :L · — +``` + +A `spec` · missing-requirement anchors to the spec's own path and lines. A boy-scout finding says +`boy-scout` after its class. With nothing to report, write `findings: none` — the report still +exists, and the tally in it is the evidence that the review ran. diff --git a/plugins/code-review/skills/cr-prepare/SKILL.md b/plugins/code-review/skills/cr-prepare/SKILL.md new file mode 100644 index 0000000..990a67f --- /dev/null +++ b/plugins/code-review/skills/cr-prepare/SKILL.md @@ -0,0 +1,100 @@ +--- +name: cr-prepare +description: >- + Headless first step of a code review that another workflow drives — resolves the committed + change against a base, reads the project's conventions and standards once, decides the active + lens set, and writes all of it to a context directory that cr-scan and cr-merge read. Asks + nothing and edits nothing in the checkout. Not for interactive use: a person reviews with + /start-cr. +argument-hint: "--base --out [-C ] [--spec ]" +user-invocable: false +allowed-tools: Read, Bash, Grep, Glob, Write +--- + +# cr-prepare — settle what the review covers, once + +Arguments: `$ARGUMENTS` + +You do `/start-cr`'s Steps 1–2b for a caller that cannot answer questions: nobody reads a question +you ask, so every decision below is taken from the input or reported back, never put to anyone. + +- `--base ` — required. The ref the change is measured from; a commit SHA is fine. +- `--out ` — required. The context directory; create it. It is the only place you write. +- `-C ` — the checkout under review. Defaults to the current directory. +- `--spec ` — optional. A spec the `spec` Lens reviews against. + +A missing `--base` or `--out`, or a `-C` that is not a git checkout, ends the run with +`status: error` and the reason. Guess nothing. + +## 1. Resolve the scope + +The review covers what the checkout's commits changed since the base — never its uncommitted +work, which belongs to whoever has the checkout open: + +```bash +python3 ${CLAUDE_PLUGIN_ROOT}/scripts/get_changes.py -C --scope committed --base +``` + +A `count` of zero is never a clean review. With an `alternate` in the output, return +`status: empty` and name the alternate ref and its count; without one, return `status: empty` +alone. The caller decides what an empty change means — you do not call it "nothing to review". + +Apply `${CLAUDE_PLUGIN_ROOT}/references/scope.md` to the list: which files are judged, which are +skipped and why, and each judged file's kind (`source`, `test`, `iac`) from its `## File kinds` +section. + +`--spec` counts only when it names a readable local file. Anything else — a URL, a ticket id, a +path that does not open — leaves the `spec` Lens inactive with that reason; never substitute a file +you think was meant. + +## 2. Conventions, standards, lens set + +Settle the conventions note, the standards slot and the active lens set by +`${CLAUDE_PLUGIN_ROOT}/references/review-setup.md`, working the mechanical read `scope.md` +describes inside the checkout. A lens is named by its rules file's basename: `comments`, +`readability-tests`, `naming-module`, `objects-patterns`, `simplicity-types`, `security`, +`performance`, `spec`. + +## 3. Write the context directory + +Write each file with the `Write` tool: + +- `conventions.md` — the note exactly as every Scanner will receive it. One text for all lenses. +- `standards.md` — the standards text, or one `## ` section per lens when you pre-sliced it; + the single word `none` when the root has neither file. +- `scope.json`: + +```json +{ + "checkout": "", + "base": "", + "diff_args": [""], + "files": [{ "path": "src/a.ts", "status": "M", "kind": "source", "untracked": false }], + "skipped": [{ "path": "pnpm-lock.yaml", "reason": "lockfile" }], + "spec": "", + "lenses": { + "active": [{ "lens": "performance", "files": ["src/a.ts"] }], + "inactive": [{ "lens": "spec", "reason": "no spec named" }] + } +} +``` + +`files` holds the judged files only. Every lens appears once, active or inactive; an active lens's +`files` is its own list — the `source` subset for `performance`, the whole judged list for every +other lens. On `status: empty`, write `scope.json` with empty lists and the `alternate` object +the script returned, so the caller can read what you saw. + +## 4. Return + +End this skill with this block and nothing after it. The block closes this skill, not the turn: when the prompt that invoked it names further steps, carry them out after it. + +``` +status: ready | empty | error +context: +base: · diff_args: <…> +files: judged · skipped +active: +inactive: +``` + +On `empty`, add `alternate: — files`; on `error`, `reason: `. diff --git a/plugins/code-review/skills/cr-scan/SKILL.md b/plugins/code-review/skills/cr-scan/SKILL.md new file mode 100644 index 0000000..c1a000d --- /dev/null +++ b/plugins/code-review/skills/cr-scan/SKILL.md @@ -0,0 +1,66 @@ +--- +name: cr-scan +description: >- + Headless code-review Scanner for one lens — reads the context directory cr-prepare wrote, + judges the change against that lens's rules, and writes its findings into the directory for + cr-merge. Asks nothing and edits nothing. Not for interactive use: a person reviews with + /start-cr. +argument-hint: "--lens --context " +user-invocable: false +allowed-tools: Read, Bash, Grep, Glob, Write +--- + +# cr-scan — one lens, one pass, one output + +Arguments: `$ARGUMENTS` + +You are the Scanner that `${CLAUDE_PLUGIN_ROOT}/references/scanner-contract.md` describes, for the +one lens named by `--lens`. Read that contract completely before anything else: it is the whole +definition of what you judge, how you grade, and the shape you return. + +A missing `--lens` or `--context`, or a context directory without `scope.json`, ends the run with +`status: error` and the reason; write nothing. + +## Your brief + +The contract's brief arrives as files, not as a message. Assemble it slot by slot: + +| Slot | Where it is | +|---|---| +| `` | `--lens` — the rules file's basename (`readability-tests` is the contract's `readability & tests`) | +| `` | `${CLAUDE_PLUGIN_ROOT}/references/rules/.md` — read it completely | +| `` | this lens's `files` under `lenses.active` in `scope.json` | +| `` | `diff_args` in `scope.json` | +| `` | tracked → `git -C diff -- `; untracked → read `/` | +| `` | `conventions.md`, verbatim | +| `` | your lens's section of `standards.md`, or all of it when it has no sections | +| `` | `spec` lens only: the file `scope.json` names, read in full | + +`` is `scope.json`'s `checkout`; every path in `files` is relative to it. A lens listed +under `lenses.inactive` is not judged: write its output file with the header alone, ending +`inactive — `, and return that. + +## Output + +Write your complete output to `/.md` with the `Write` tool, in the contract's shape +for your lens, under one header line: + +``` +Lens: · files judged: of +``` + +A file counts as judged once you have read its change. That file is the only thing you write: +nothing in the checkout, no probe or scratch file anywhere. You dispatch no agent. If something +changes after you have written it, rewrite the whole file — the merge reads that file, never this +conversation. + +End this skill with this block and nothing after it. The block closes this skill, not the turn: when the prompt that invoked it names further steps, carry them out after it. + +``` +status: scanned | inactive | error +lens: +output: /.md +files judged: of +findings: high · medium · nit — or, for comments: verdicts ( to act on) +candidates: · handoffs: +``` diff --git a/plugins/fd3/CHANGELOG.md b/plugins/fd3/CHANGELOG.md index 64e1b3a..3af8e88 100644 --- a/plugins/fd3/CHANGELOG.md +++ b/plugins/fd3/CHANGELOG.md @@ -20,6 +20,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- `implement-tasks` reviews each branch with the `code-review` plugin's headless skills — + `cr-prepare`, one `cr-scan` agent per active lens, `cr-merge` — after its scoped CI passes; + `safe` and `structural` findings are fixed and the fixes reviewed again, while `spec` findings, + `security` fixes, boy-scout findings, report-only findings and any finding the fixer left + unfixed go to the user as `review` items. `repair-run` + reviews each repair's own commits the same way, and a repair reopens a `done` branch +- `/fd3:build-spec ` re-enters at validation for a finished spec — no grilling — and + validates every edit made after the final verdict, including a move or a header rewrite +- The validation verdict line is followed by `Checked at:` with each repository's commit, and + `split-to-tasks` stops when `origin/` has since changed a file the spec cites +- `split-to-tasks` turns a commit sequence the spec binds inside a branch into `depends-on` edges +- Evals — `split-stale-origin` and `build-spec-reentry`, with a fixture `SETUP.sh` hook in the + sandbox reset for scenarios that need a remote - `grill-topic` writes what the conversation established before the command into the research directory before round 1, and keeps a question ledger file — round numbers, answers and carry-overs no longer live only in a context that gets compacted @@ -39,6 +52,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `split-to-tasks` never writes a verdict line or an evidence block, even when asked, and routes + an unvalidated or stale spec to `/fd3:build-spec` — a hand-written pass is not a validation - A gap the spec already declares with an owner and a placement is `deferred` on sight — it never reaches the user as a question, and it never lowers a verdict or a phase row - An operational task exists only for hand-run steps no repository carries; a phase's own @@ -82,9 +97,28 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 what actually holds: edit them at their absolute paths, commit nothing, touch nothing else - `implement-tasks` says that a pre-launch commit of the spec and tasks directory must carry the repository's regenerated indexes, or say it did not — a stale one fails every branch at once -- `implement-tasks` step 2 offers only review skills that review inline, and expands a fan-out - orchestrator the user names (`code-review:start-cr`) into its single-lens skills — inside a - workflow agent there is no `Agent` tool, so the orchestrator silently degraded to one pass +- The in-workflow review no longer passes an empty diff as clean: a root branch is measured + against `origin/` (`diffBase`) rather than the parked branch it was cut from + (`startRef`), and a review with an empty change, a lens that judged nothing or a skill that was + never invoked is `no-verdict` — the branch stays `merged` +- Every workflow agent prompt opens with a step guard, so an agent never re-runs the skill the + relayed user request names +- The baseline worktree is cut detached, so a parked base branch no longer refuses it +- CI runners read each command's own exit status from a log beside the worktree — a pipe into + `tail` no longer turns a failing suite into a pass +- A CI failure a fix can only clear by changing behaviour goes to the human as a caveat, not into + the fixer's commit +- `repair-run` commits each human decision separately +- `validate-spec` probes run in a detached scratch worktree and never install, link or modify + dependencies in the user's checkout +- A check whose only findings are non-blocking reads so in its row, and a regrade between passes + is recorded +- `build-spec` places the spec in the repository's own layout, never the scratchpad, and relays + `write-spec`'s questions through `AskUserQuestion` with numbered sub-parts +- `grill-topic` re-checks every answered question whose cost quoted a size when a later answer + widens the scope +- `split-to-tasks` asks to reuse the branch the spec itself names even when it carries only the + spec, writes its report after the coverage re-run, and never abbreviates `depends-on` ## [0.1.0] - 2026-09-04 diff --git a/plugins/fd3/README.md b/plugins/fd3/README.md index b70f6d1..12f7d9f 100644 --- a/plugins/fd3/README.md +++ b/plugins/fd3/README.md @@ -46,8 +46,15 @@ Both are dispatched by the skills, never by the user. `implement-tasks` drives two dynamic-workflow scripts: `implement-run.js` (waves, merges, then CI and code review per target branch) and `repair-run.js` (applies human decisions to existing -branches and re-validates with CI only). The skill owns the conversation; the workflows own -everything between launch and report. +branches, re-validates them with CI and reviews what the repair changed). The skill owns the +conversation; the workflows own everything between launch and report. + +Code review is optional and needs the [`code-review`](../code-review) plugin from this marketplace: +the workflows drive its headless `cr-prepare`, `cr-scan` and `cr-merge` skills — all eight lenses, +one agent each. Mechanical and structural findings are fixed and the fixes reviewed again; +`spec` findings, `security` fixes, boy-scout findings on untouched code, anything report-only and +anything the fixer left unfixed go to the user. A branch whose review +did not run, or left findings for the user, stays `merged` rather than `done`. ## Usage diff --git a/plugins/fd3/commands/build-spec.md b/plugins/fd3/commands/build-spec.md index a3a6912..47fa3c5 100644 --- a/plugins/fd3/commands/build-spec.md +++ b/plugins/fd3/commands/build-spec.md @@ -5,8 +5,12 @@ argument-hint: " "$teammate/repo-a/services/ledger/src/api/entries.ts" <<'TS' +export interface LedgerEntry { + orderId: string; + amountMinor: number; + direction: "debit" | "credit"; + currency: string; +} + +export async function listEntries(orderId: string, limit = 50): Promise { + void orderId; + void limit; + return []; +} +TS +git -C "$teammate/repo-a" -c user.name='teammate' -c user.email='teammate@localhost' \ + commit -q -am 'feat(ledger): add currency and a page limit to entries' --no-gpg-sign +git -C "$teammate/repo-a" push -q origin main +rm -rf "$teammate" diff --git a/plugins/fd3/evals/fixtures/stale-origin-rollout-spec/repo-a/README.md b/plugins/fd3/evals/fixtures/stale-origin-rollout-spec/repo-a/README.md new file mode 100644 index 0000000..54e96bf --- /dev/null +++ b/plugins/fd3/evals/fixtures/stale-origin-rollout-spec/repo-a/README.md @@ -0,0 +1,9 @@ +# commerce-core (repo-a) + +Monorepo. Two services, two owning teams: + +- `services/checkout/` — owned by team-checkout +- `services/ledger/` — owned by team-ledger + +Pull requests must be scoped to one service's subtree; CODEOWNERS requires the owning team's +approval per subtree. Branches follow `feat/-`. diff --git a/plugins/fd3/evals/fixtures/stale-origin-rollout-spec/repo-a/services/checkout/src/api/charge.ts b/plugins/fd3/evals/fixtures/stale-origin-rollout-spec/repo-a/services/checkout/src/api/charge.ts new file mode 100644 index 0000000..cd4b9c7 --- /dev/null +++ b/plugins/fd3/evals/fixtures/stale-origin-rollout-spec/repo-a/services/checkout/src/api/charge.ts @@ -0,0 +1,8 @@ +export interface ChargeBody { + orderId: string; + amountMinor: number; +} + +export async function postCharge(body: ChargeBody) { + return { status: "accepted", orderId: body.orderId }; +} diff --git a/plugins/fd3/evals/fixtures/stale-origin-rollout-spec/repo-a/services/checkout/src/config.ts b/plugins/fd3/evals/fixtures/stale-origin-rollout-spec/repo-a/services/checkout/src/config.ts new file mode 100644 index 0000000..96692a0 --- /dev/null +++ b/plugins/fd3/evals/fixtures/stale-origin-rollout-spec/repo-a/services/checkout/src/config.ts @@ -0,0 +1,3 @@ +export const flags = { + asyncSettlement: false, +}; diff --git a/plugins/fd3/evals/fixtures/stale-origin-rollout-spec/repo-a/services/ledger/migrations/README.md b/plugins/fd3/evals/fixtures/stale-origin-rollout-spec/repo-a/services/ledger/migrations/README.md new file mode 100644 index 0000000..4594af4 --- /dev/null +++ b/plugins/fd3/evals/fixtures/stale-origin-rollout-spec/repo-a/services/ledger/migrations/README.md @@ -0,0 +1,5 @@ +# Ledger migrations + +SQL files in this directory are applied by CI in filename order (`NNNN_description.sql`) on merge +to `main`. A migration is irreversible once applied to the shared staging database — expand-only +changes land here; contracting changes wait for their cleanup gate. diff --git a/plugins/fd3/evals/fixtures/stale-origin-rollout-spec/repo-a/services/ledger/src/api/entries.ts b/plugins/fd3/evals/fixtures/stale-origin-rollout-spec/repo-a/services/ledger/src/api/entries.ts new file mode 100644 index 0000000..3312e13 --- /dev/null +++ b/plugins/fd3/evals/fixtures/stale-origin-rollout-spec/repo-a/services/ledger/src/api/entries.ts @@ -0,0 +1,10 @@ +export interface LedgerEntry { + orderId: string; + amountMinor: number; + direction: "debit" | "credit"; +} + +export async function listEntries(orderId: string): Promise { + void orderId; + return []; +} diff --git a/plugins/fd3/evals/fixtures/stale-origin-rollout-spec/repo-b/README.md b/plugins/fd3/evals/fixtures/stale-origin-rollout-spec/repo-b/README.md new file mode 100644 index 0000000..ca87206 --- /dev/null +++ b/plugins/fd3/evals/fixtures/stale-origin-rollout-spec/repo-b/README.md @@ -0,0 +1,3 @@ +# merchant-dashboard (repo-b) + +Merchant-facing web app, owned by team-web. Branches follow `feat/`. diff --git a/plugins/fd3/evals/fixtures/stale-origin-rollout-spec/repo-b/src/components/PaymentStatus.tsx b/plugins/fd3/evals/fixtures/stale-origin-rollout-spec/repo-b/src/components/PaymentStatus.tsx new file mode 100644 index 0000000..155abe8 --- /dev/null +++ b/plugins/fd3/evals/fixtures/stale-origin-rollout-spec/repo-b/src/components/PaymentStatus.tsx @@ -0,0 +1,8 @@ +export interface PaymentStatusProps { + orderId: string; +} + +export function PaymentStatus(props: PaymentStatusProps) { + void props; + return null; +} diff --git a/plugins/fd3/evals/fixtures/stale-origin-rollout-spec/spec/rollout-spec.md b/plugins/fd3/evals/fixtures/stale-origin-rollout-spec/spec/rollout-spec.md new file mode 100644 index 0000000..7d60bd1 --- /dev/null +++ b/plugins/fd3/evals/fixtures/stale-origin-rollout-spec/spec/rollout-spec.md @@ -0,0 +1,217 @@ +# Asynchronous settlement with a merchant-visible ledger — SPEC + +**What changes:** checkout emits settlement events into a new ledger table, the ledger service +exposes them, and the merchant dashboard shows payment status — built dark in phase 1, switched on +in phase 2. + +- Epic: LED-100 +- Status: validated +- Date: 2026-07-28 + +This spec supersedes nothing; there are no companion documents. + +## 2. Problem and goal + +Settlement today is implicit: checkout accepts a charge (`repo-a/services/checkout/src/api/charge.ts:6`) +and nothing records the resulting ledger movement, so merchants cannot see payment status anywhere. +The ledger service has an entries endpoint stub that returns nothing +(`repo-a/services/ledger/src/api/entries.ts:7`), and the dashboard has an unrouted placeholder +component (`repo-b/src/components/PaymentStatus.tsx:5`). + +Goal: every accepted charge produces a ledger entry a merchant can see in the dashboard, switched +on per the rollout, with no behaviour change until phase 2. + +## 3. Design decisions + +| # | Decision | Rationale | +|---|---|---| +| D1 | **Ledger entries live in a new `ledger_entries` table owned by the ledger service** | The ledger service already owns the read path (`repo-a/services/ledger/src/api/entries.ts:7`); giving checkout its own copy would fork the source of truth. Cost accepted: checkout depends on the ledger schema landing first. | +| D2 | **Checkout emits settlement writes synchronously behind the `asyncSettlement` flag, default off** | The flag exists (`repo-a/services/checkout/src/config.ts:2`) and default-off keeps phase 1 dark; a queue would add a broker no current volume justifies. Cost accepted: a ledger write failure surfaces on the charge path once the flag is on. | +| D3 | **The dashboard reads through the ledger's `GET /ledger/entries` endpoint, never the database** | The dashboard is in another repository and team-web owns no database credentials; the endpoint is the contract. Cost accepted: a second network hop for status data. | +| D4 | **Phase 1 builds everything dark; phase 2 switches behaviour** | Both repositories can land and deploy independently with no user-visible change, then the switch is two small, reversible changes. Cost accepted: two deploys instead of one. | + +## 4. Target architecture + +### DB-1 — `ledger_entries` table (migration) + +New migration `repo-a/services/ledger/migrations/0001_create_ledger_entries.sql`: + +- Columns: `id BIGSERIAL PRIMARY KEY`, `order_id TEXT NOT NULL`, + `amount_minor BIGINT NOT NULL CHECK (amount_minor >= 0)`, + `direction TEXT NOT NULL CHECK (direction IN ('debit','credit'))`, + `created_at TIMESTAMPTZ NOT NULL DEFAULT now()`. +- Index on `(order_id, created_at)`. +- Errors: none at runtime — this element is schema only. +- Auth: applied by CI with the migration role (see the migrations README convention). +- Limits: expand-only; no column drops or renames in this spec. +- Migrations are applied by CI in filename order and are irreversible once applied to the shared + staging database, so this element must land on `main` before any code that writes to it. + +### API-2 — ledger entries endpoints (ledger service) + +`GET /ledger/entries?orderId=` replaces the stub at `repo-a/services/ledger/src/api/entries.ts:7`, and `POST /ledger/entries` is added beside it. + +- Request: `GET` takes an `orderId` query parameter, required, non-empty string; `POST` takes a JSON body `{ orderId: string, amountMinor: number, direction: "debit" | "credit" }`. +- Response: `GET` returns a JSON array of `{ orderId: string, amountMinor: number, direction: "debit" | "credit", createdAt: string }`; `POST` returns 201 with the stored entry. +- Errors: 400 on a missing or empty `orderId` (both methods) or a negative `amountMinor`; `GET` returns 200 with `[]` when no entries exist. +- Auth: the existing internal service token middleware; the dashboard's token is already accepted. +- Limits: `GET` responses capped at 500 entries, newest first; `POST` writes exactly one row. + +### API-1 — settlement write from checkout + +`postCharge` (`repo-a/services/checkout/src/api/charge.ts:6`) gains a settlement write: when +`flags.asyncSettlement` is true, an accepted charge writes one `credit` entry through API-2's +`POST /ledger/entries`. + +- Fields: `orderId`, `amountMinor` from the charge body; `direction` fixed to `credit`. +- Errors: a ledger write failure fails the charge with HTTP 502 (flag on); flag off, no write + happens and behaviour is byte-identical to today. +- Auth: the existing internal service token. +- Limits: one entry per accepted charge; no retries — the caller may retry the charge. + +### UI-1 — payment status panel (dashboard) + +`PaymentStatus` (`repo-b/src/components/PaymentStatus.tsx:5`) renders the entries for an order. + +- Fields: renders `amountMinor`, `direction`, `createdAt` per entry; empty state for `[]`. +- Errors: an API error renders the existing dashboard error banner. +- Auth: the dashboard's existing session; the panel adds no new auth surface. +- Limits: phase 1 renders from a local mock module only and stays unrouted — dark by D4. + +### CONFIG-1 — the settlement switch (checkout) + +`flags.asyncSettlement` (`repo-a/services/checkout/src/config.ts:2`) flips to `true`. + +- Fields: one boolean flag. +- Errors: none — the flag is read at module load. +- Auth: none — a code change through the normal review path. +- Limits: phase 2 only, after DB-1, API-1 and API-2 are deployed. + +### INTEGRATION-1 — dashboard wired to the live endpoint (dashboard) + +The panel swaps its mock module for the live `GET /ledger/entries` call and gets routed into the +order detail page. + +- Fields: same rendering contract as UI-1; the data source changes. +- Errors: same error banner path as UI-1. +- Auth: the dashboard's existing internal service token toward the ledger. +- Limits: phase 2 only, after API-2 is deployed and UI-1 has landed. + +### Prerequisites + +| Prerequisite | Status | +|---|---| +| CI applies ledger migrations on merge | met — the convention is documented in `repo-a/services/ledger/migrations/README.md` and CI already runs it for the existing schema | +| Internal service token shared between the three services | met — checkout and the dashboard already call the ledger with it today | + +## 5. Ownership + +| Repository / component | Owns | Apply mechanism | +|---|---|---| +| repo-a `services/checkout/` | API-1, CONFIG-1 | pull request; CODEOWNERS requires team-checkout approval; CI deploys on merge | +| repo-a `services/ledger/` | DB-1, API-2 | pull request; CODEOWNERS requires team-ledger approval; CI deploys on merge and applies migrations | +| repo-b | UI-1, INTEGRATION-1 | pull request; team-web approval; CI deploys on merge | + +repo-a is a monorepo with per-subtree CODEOWNERS: a pull request touching both `services/checkout/` +and `services/ledger/` needs both teams' approval, so changes are scoped to one subtree per pull +request. + +## 6. The change, per repository + +### repo-a — `services/ledger/` (team-ledger) + +1. **DB-1** — new: migration `0001_create_ledger_entries.sql` in + `repo-a/services/ledger/migrations/`. +2. **API-2** — changed: replace the stub in `repo-a/services/ledger/src/api/entries.ts:7-10` with + the real query and the 400 guard, and add the `POST` handler beside it. + +### repo-a — `services/checkout/` (team-checkout) + +3. **API-1** — changed: settlement write in `repo-a/services/checkout/src/api/charge.ts:6-8`, + guarded by the flag. +4. **CONFIG-1** — changed: flip `asyncSettlement` to `true` in + `repo-a/services/checkout/src/config.ts:2`. + +### repo-b (team-web) + +5. **UI-1** — changed: real rendering plus a mock data module, component stays unrouted + (`repo-b/src/components/PaymentStatus.tsx:5-8`). +6. **INTEGRATION-1** — changed: swap the mock for the live endpoint call and route the panel into + the order detail page. + +## 7. Rollout + +| # | Phase | Where | Switches anything? | +|---|---|---|---| +| 1 | DB-1, API-2, API-1 (flag off), UI-1 (unrouted) land and deploy | repo-a, repo-b | no — everything is dark | +| 2 | CONFIG-1 flips the flag; INTEGRATION-1 routes the panel onto live data | repo-a, repo-b | yes — settlement writes begin and merchants see status | + +Build order within phase 1: DB-1 first (the migration must be applied before any writer or reader +ships), then API-2, then API-1; UI-1 is independent of all three. Phase 2 starts only after every +phase-1 item is deployed; within phase 2, CONFIG-1 and INTEGRATION-1 are independent of each +other. + +Single environment per repository; each phase is one deploy per repository, checkout after ledger. +Waiting period between phases: none — phase 2 starts as soon as every phase-1 item is deployed and +its verification rows pass. Phase 1 switches nothing, so there is nothing to observe between the +phases and no gate outside this spec's own verification. + +Hard dependencies: none outside this spec. + +Rollback: phase 2 — flip the flag back and un-route the panel; the reversal is complete when no +new `ledger_entries` rows appear and the panel is unreachable. Phase 1 — revert the code merges; +the migration stays behind, unused (expand-only; removal is out of scope, LED-109). + +## 8. Verification + +- **DB-1** — probe: `psql "$LEDGER_DATABASE_URL" -c "\d ledger_entries"` lists the five columns + and the `(order_id, created_at)` index. Before the change: `did not find any relation`. +- **API-2** — probe: `curl -s "ledger.internal/ledger/entries?orderId=o_1"` returns `[]` with 200; + omitting `orderId` returns 400; a valid `POST` returns 201. Before the change both `GET`s return the stub's empty 200 and the `POST` 404s. +- **API-1** — triggered: with the flag on in a test environment, post a charge; one `credit` row + for the order appears in `ledger_entries`. +- **UI-1** — triggered: render the panel in the dashboard's component preview against the mock + module; entries and the empty state both render. +- **CONFIG-1** — probe: `rg "asyncSettlement" repo-a/services/checkout/src/config.ts` shows + `true`. Before phase 2 it shows `false`. +- **INTEGRATION-1** — triggered: open an order with entries in the dashboard; the panel shows the + rows returned by API-2. + +Phase 1 is verified by the DB-1, API-2 probes plus the API-1 and UI-1 triggered checks; phase 2 by +the CONFIG-1 probe and the INTEGRATION-1 triggered check. + +## 9. Cleanup + +The subject has no cleanup in this spec: the migration is expand-only, and removing the mock data +module happens inside INTEGRATION-1's pull request. + +## 10. Out of scope + +- **Refunds in the ledger (a `refund` direction)** — owner: team-ledger, placement: ticket LED-108. +- **Dropping the mock-era fixtures from the dashboard test suite** — owner: team-web, placement: + ticket LED-109. + +## 11. Tickets + +LED-100 (epic), LED-108 and LED-109 exist in the tracker. No new tickets are needed; each task's +pull request cites LED-100. + +## 12. Appendix — the evidence record + +| Claim | How it was verified | +|---|---| +| Checkout accepts charges with no settlement record | `repo-a/services/checkout/src/api/charge.ts:6-8` — `postCharge` returns `accepted`, no write | +| The ledger entries endpoint is a stub | `repo-a/services/ledger/src/api/entries.ts:7-10` — `listEntries` returns `[]` unconditionally | +| The settlement flag exists and is off | `repo-a/services/checkout/src/config.ts:2` — `asyncSettlement: false` | +| The dashboard panel exists and is unrouted | `repo-b/src/components/PaymentStatus.tsx:5` — component returns `null`; no route references it | +| Migrations are applied by CI in filename order and are irreversible on staging | `repo-a/services/ledger/migrations/README.md` — the convention paragraph | + +### Validation pass — 2026-07-30 + +Verdict: ready — claims: 1 verified / 0 deferred / 0 blocked — spec 217 lines at this verdict +Checked at: repo-a @ @@SHA:repo-a@@, repo-b @ @@SHA:repo-b@@ + +| Claim | How it was verified | +|---|---| +| All 12 spec-level checks pass | `fd3:validate-spec` run of 2026-07-30 — every check row `pass`, no blocking findings | +| Verdict | phase 1: yes; phase 2: yes — spec is ready to split | diff --git a/plugins/fd3/evals/lib/checks/build-spec-reentry.mjs b/plugins/fd3/evals/lib/checks/build-spec-reentry.mjs new file mode 100644 index 0000000..37bcf44 --- /dev/null +++ b/plugins/fd3/evals/lib/checks/build-spec-reentry.mjs @@ -0,0 +1,31 @@ +import * as h from '../helpers.mjs'; + +const SCENARIO = 'build-spec-reentry'; +const SPEC = 'spec/rollout-spec.md'; + +const verdictLines = (text) => (text.match(/^Verdict: /gm) || []).length; + +// A finished spec handed to build-spec is a re-validation, never a new grilling: the decisions in it +// are the user's, already made. +export default () => { + const c = h.checker(); + const diff = h.diffSandbox(SCENARIO, 'rollout-spec'); + + const ledger = diff.added.filter((f) => /question-ledger\.md$/.test(f)); + c.check(ledger.length === 0, `a grilling round started on a finished spec: ${ledger.join(', ')}`); + + const before = verdictLines(h.readFixtureFile('rollout-spec', SPEC)); + const spec = h.readSandboxFile(SCENARIO, SPEC); + c.check( + spec !== null && verdictLines(spec) > before, + `no validation pass appended a verdict line (found ${spec ? verdictLines(spec) : 0}, the fixture holds ${before})`, + ); + + const newSpecs = diff.added.filter((f) => f.endsWith('.md') && !/(^|\/)(notes|research|evidence)\//.test(f)); + c.check(newSpecs.length === 0, `a second spec was written instead of re-validating the given one: ${newSpecs.join(', ')}`); + + const code = diff.modified.filter((f) => f !== SPEC); + c.check(code.length === 0, `validation touched files other than the spec: ${code.join(', ')}`); + + return c.verdict(); +}; diff --git a/plugins/fd3/evals/lib/checks/split-stale-origin.mjs b/plugins/fd3/evals/lib/checks/split-stale-origin.mjs new file mode 100644 index 0000000..f7b4769 --- /dev/null +++ b/plugins/fd3/evals/lib/checks/split-stale-origin.mjs @@ -0,0 +1,41 @@ +import { execFileSync } from 'node:child_process'; +import path from 'node:path'; +import * as h from '../helpers.mjs'; + +const SCENARIO = 'split-stale-origin'; +const FIXTURE = 'stale-origin-rollout-spec'; +const SPEC = 'spec/rollout-spec.md'; + +// The reset writes each repo's SHA over its placeholder, so the only legitimate difference between +// the sandbox spec and the fixture is that one line. +const normalise = (text) => text.replace(/^Checked at: .*$/m, 'Checked at: '); + +export default (output) => { + const c = h.checker(); + + c.check(h.readTasks(SCENARIO).length === 0, 'task files were written although origin changed a file the spec cites'); + + const diff = h.diffSandbox(SCENARIO, FIXTURE); + c.check(diff.added.length === 0, `files created on a stale spec: ${diff.added.join(', ')}`); + c.check(diff.removed.length === 0, `fixture files removed: ${diff.removed.join(', ')}`); + const drifted = diff.modified.filter((f) => f !== SPEC); + c.check(drifted.length === 0, `fixture files modified: ${drifted.join(', ')}`); + + const spec = h.readSandboxFile(SCENARIO, SPEC); + c.check( + spec !== null && normalise(spec) === normalise(h.readFixtureFile(FIXTURE, SPEC)), + 'the spec was edited — correcting it to follow origin is build-spec\'s work, not the split\'s', + ); + + // Rebasing the branch onto origin is the improvisation the stop exists to prevent. + const recorded = /repo-a @ ([0-9a-f]{7,})/.exec(spec || ''); + if (c.check(recorded !== null, 'the Checked at line lost its repo-a SHA')) { + const head = execFileSync('git', ['-C', path.join(h.sandboxDir(SCENARIO), 'repo-a'), 'rev-parse', '--short', 'main'], { encoding: 'utf8' }).trim(); + c.check(head.startsWith(recorded[1]) || recorded[1].startsWith(head), `repo-a main moved from the validated ${recorded[1]} to ${head}`); + } + + c.check(/entries\.ts/.test(output), 'the stop does not name the file origin changed (entries.ts)'); + c.check(/\/?fd3:build-spec/.test(output), 'the stop does not name the /fd3:build-spec route'); + + return c.verdict(); +}; diff --git a/plugins/fd3/evals/lib/checks/validate-defective-spec.mjs b/plugins/fd3/evals/lib/checks/validate-defective-spec.mjs index b45c60e..42c365b 100644 --- a/plugins/fd3/evals/lib/checks/validate-defective-spec.mjs +++ b/plugins/fd3/evals/lib/checks/validate-defective-spec.mjs @@ -12,7 +12,10 @@ export default (output) => { c.check(/D2[\s\S]{0,400}D5|D5[\s\S]{0,400}D2/.test(output), 'D2/D5 contradiction not surfaced'); c.check(/redis/i.test(output) && /postgres/i.test(output), 'undecided Redis-or-Postgres either/or not surfaced'); c.check(/invoice pdf/i.test(output), 'ownerless out-of-scope item (invoice PDF rendering) not surfaced'); - c.check(/retry[-\s]worker/i.test(output) && /element[-\s]code/i.test(output), 'uncoded "Delivery retry worker" element not surfaced'); + // Giving the worker a code is the one obvious correction, so a spec that now carries it counts as + // surfacing the defect just as a report finding does. + const workerCoded = spec !== null && /^###\s+[A-Z][A-Z0-9]*-\d+\b.*worker/im.test(spec) && !/^###\s+Delivery retry worker\s*$/im.test(spec); + c.check(workerCoded || (/retry[-\s]worker/i.test(output) && /element[-\s]code/i.test(output)), 'uncoded "Delivery retry worker" element neither surfaced nor coded in the spec'); c.check(/section 3/i.test(output), 'no finding anchored to section 3'); c.check(/section 10/i.test(output), 'no finding anchored to section 10'); c.check(/section 4/i.test(output), 'no finding anchored to section 4'); diff --git a/plugins/fd3/evals/lib/helpers.mjs b/plugins/fd3/evals/lib/helpers.mjs index f63a0a7..4990909 100644 --- a/plugins/fd3/evals/lib/helpers.mjs +++ b/plugins/fd3/evals/lib/helpers.mjs @@ -6,8 +6,8 @@ export const EVALS_DIR = path.resolve(path.dirname(fileURLToPath(import.meta.url // The one central place that skips `.git/`: the reset script initialises repos inside // sandboxes that the pristine fixtures do not have, so a naive recursive diff always fires. -// DEFECTS.md is fixture documentation and is excluded from the sandbox copy. -const SKIP = new Set(['.git', 'DEFECTS.md']); +// DEFECTS.md is fixture documentation and SETUP.sh fixture tooling; neither reaches the sandbox. +const SKIP = new Set(['.git', 'DEFECTS.md', 'SETUP.sh']); export function sandboxDir(scenario) { return path.join(EVALS_DIR, '.sandbox', scenario); diff --git a/plugins/fd3/evals/promptfooconfig.yaml b/plugins/fd3/evals/promptfooconfig.yaml index d7ab7ab..b2babe5 100644 --- a/plugins/fd3/evals/promptfooconfig.yaml +++ b/plugins/fd3/evals/promptfooconfig.yaml @@ -95,6 +95,13 @@ tests: two was chosen — a stated choice counts on its own, no question needed in the text; (d) it does not split the spec into task files. + - description: split-stale-origin + vars: { query: file://prompts/split-stale-origin.txt } + options: { working_dir: .sandbox/split-stale-origin, max_budget_usd: 5.0 } + assert: + - type: javascript + value: file://lib/checks/split-stale-origin.mjs + - description: split-orphan-element vars: { query: file://prompts/split-orphan-element.txt } options: { working_dir: .sandbox/split-orphan-element, max_budget_usd: 5.0 } @@ -196,3 +203,10 @@ tests: assert: - type: javascript value: file://lib/checks/build-spec-gate.mjs + + - description: build-spec-reentry + vars: { query: file://prompts/build-spec-reentry.txt } + options: { working_dir: .sandbox/build-spec-reentry, max_budget_usd: 8.0 } + assert: + - type: javascript + value: file://lib/checks/build-spec-reentry.mjs diff --git a/plugins/fd3/evals/prompts/build-spec-reentry.txt b/plugins/fd3/evals/prompts/build-spec-reentry.txt new file mode 100644 index 0000000..40893ad --- /dev/null +++ b/plugins/fd3/evals/prompts/build-spec-reentry.txt @@ -0,0 +1 @@ +/fd3:build-spec spec/rollout-spec.md \ No newline at end of file diff --git a/plugins/fd3/evals/prompts/split-stale-origin.txt b/plugins/fd3/evals/prompts/split-stale-origin.txt new file mode 100644 index 0000000..abc8047 --- /dev/null +++ b/plugins/fd3/evals/prompts/split-stale-origin.txt @@ -0,0 +1 @@ +/fd3:split-to-tasks spec/rollout-spec.md \ No newline at end of file diff --git a/plugins/fd3/evals/prompts/validate-clean-spec.txt b/plugins/fd3/evals/prompts/validate-clean-spec.txt index 17a13f2..5dc112a 100644 --- a/plugins/fd3/evals/prompts/validate-clean-spec.txt +++ b/plugins/fd3/evals/prompts/validate-clean-spec.txt @@ -1,3 +1,7 @@ Invoke the fd3:validate-spec skill on this spec: spec/clean-spec.md +Pass the skill this with the path: nobody can answer questions during this run, so it must not hand +any up or wait for answers. Every fact only the user holds and every repair choice goes into the +report as blocked, naming the question it would have asked, and the skill returns its full report. + When it returns, print its report verbatim and unedited as your entire reply — no summary, no reordering, no commentary of your own. diff --git a/plugins/fd3/evals/prompts/validate-declared-gap.txt b/plugins/fd3/evals/prompts/validate-declared-gap.txt index 5d6e0dc..1103a23 100644 --- a/plugins/fd3/evals/prompts/validate-declared-gap.txt +++ b/plugins/fd3/evals/prompts/validate-declared-gap.txt @@ -1,3 +1,7 @@ Invoke the fd3:validate-spec skill on this spec: spec/gap-spec.md +Pass the skill this with the path: nobody can answer questions during this run, so it must not hand +any up or wait for answers. Every fact only the user holds and every repair choice goes into the +report as blocked, naming the question it would have asked, and the skill returns its full report. + When it returns, print its report verbatim and unedited as your entire reply — no summary, no reordering, no commentary of your own. diff --git a/plugins/fd3/evals/prompts/validate-defective-spec.txt b/plugins/fd3/evals/prompts/validate-defective-spec.txt index 995c0d6..b31bd83 100644 --- a/plugins/fd3/evals/prompts/validate-defective-spec.txt +++ b/plugins/fd3/evals/prompts/validate-defective-spec.txt @@ -1,3 +1,7 @@ Invoke the fd3:validate-spec skill on this spec: spec/payments-spec.md +Pass the skill this with the path: nobody can answer questions during this run, so it must not hand +any up or wait for answers. Every fact only the user holds and every repair choice goes into the +report as blocked, naming the question it would have asked, and the skill returns its full report. + When it returns, print its report verbatim and unedited as your entire reply — no summary, no reordering, no commentary of your own. diff --git a/plugins/fd3/evals/prompts/validate-ownerless-gap.txt b/plugins/fd3/evals/prompts/validate-ownerless-gap.txt index 00a4207..bfbf9f5 100644 --- a/plugins/fd3/evals/prompts/validate-ownerless-gap.txt +++ b/plugins/fd3/evals/prompts/validate-ownerless-gap.txt @@ -1,3 +1,7 @@ Invoke the fd3:validate-spec skill on this spec: spec/ownerless-gap-spec.md +Pass the skill this with the path: nobody can answer questions during this run, so it must not hand +any up or wait for answers. Every fact only the user holds and every repair choice goes into the +report as blocked, naming the question it would have asked, and the skill returns its full report. + When it returns, print its report verbatim and unedited as your entire reply — no summary, no reordering, no commentary of your own. diff --git a/plugins/fd3/evals/prompts/validate-phased-verdict.txt b/plugins/fd3/evals/prompts/validate-phased-verdict.txt index 98d1d99..6da5c82 100644 --- a/plugins/fd3/evals/prompts/validate-phased-verdict.txt +++ b/plugins/fd3/evals/prompts/validate-phased-verdict.txt @@ -1,3 +1,7 @@ Invoke the fd3:validate-spec skill on this spec: spec/phased-spec.md +Pass the skill this with the path: nobody can answer questions during this run, so it must not hand +any up or wait for answers. Every fact only the user holds and every repair choice goes into the +report as blocked, naming the question it would have asked, and the skill returns its full report. + When it returns, print its report verbatim and unedited as your entire reply — no summary, no reordering, no commentary of your own. diff --git a/plugins/fd3/evals/reset-sandboxes.sh b/plugins/fd3/evals/reset-sandboxes.sh index ffe8c2f..9c319bb 100755 --- a/plugins/fd3/evals/reset-sandboxes.sh +++ b/plugins/fd3/evals/reset-sandboxes.sh @@ -4,7 +4,9 @@ # # sandbox:fixture:git-roots — git-roots is a space-separated list of directories # (relative to the sandbox) that get a fresh git repo; "." is the sandbox root, -# empty means no repo, "-" as fixture means an empty sandbox. +# empty means no repo, "-" as fixture means an empty sandbox. A fixture's SETUP.sh, when present, +# runs inside the sandbox after the repos exist, with ORIGINS naming a directory outside every +# sandbox for the remotes it creates — a remote inside the sandbox would show up in its diff. set -euo pipefail cd "$(dirname "$0")" @@ -20,6 +22,7 @@ MAPPINGS=( "split-unvalidated-precondition:unvalidated-rollout-spec:repo-a repo-b" "split-orphan-element:orphan-rollout-spec:repo-a repo-b" "split-english-artifacts:rollout-spec:repo-a repo-b" + "split-stale-origin:stale-origin-rollout-spec:repo-a repo-b" "write-missing-input-stop:empty-project:" "write-template-conformance:grilling-summary:." "write-no-invented-decisions:no-rollout-order:." @@ -28,6 +31,7 @@ MAPPINGS=( "grill-numbered-questions:retry-topic:." "grill-session-files:retry-topic:." "build-spec-gate:retry-topic:." + "build-spec-reentry:rollout-spec:repo-a repo-b" "e2e-chain:grilling-summary:." "researcher-output-contract:-:" "researcher-multiple-questions:-:" @@ -47,7 +51,7 @@ for entry in "${MAPPINGS[@]}"; do mkdir -p "$dest" if [ "$fixture" != "-" ]; then # DEFECTS.md is fixture documentation, never part of the scenario's fake project. - rsync -a --exclude 'DEFECTS.md' "fixtures/${fixture}/" "$dest/" + rsync -a --exclude 'DEFECTS.md' --exclude 'SETUP.sh' "fixtures/${fixture}/" "$dest/" fi if [ -n "$git_roots" ]; then @@ -60,6 +64,12 @@ for entry in "${MAPPINGS[@]}"; do commit -q -m 'fixture baseline' --no-gpg-sign done fi + + if [ -f "fixtures/${fixture}/SETUP.sh" ]; then + origins="$(pwd)/.sandbox/.origins/${sandbox}" + mkdir -p "$origins" + (cd "$dest" && ORIGINS="$origins" bash "../../fixtures/${fixture}/SETUP.sh") + fi done echo "fd3 sandboxes reset: ${#MAPPINGS[@]} scenario dirs under $(pwd)/.sandbox/" diff --git a/plugins/fd3/references/task-template.md b/plugins/fd3/references/task-template.md index 012014e..3b0ae02 100644 --- a/plugins/fd3/references/task-template.md +++ b/plugins/fd3/references/task-template.md @@ -51,7 +51,8 @@ implementation flow: `implemented` means the code exists on the task's own branc that branch has reached the target branch, which is now waiting on validation — batched per target branch (build, lint, tests, then code review — once, never per task, so parallel tasks never race the same tooling); `blocked` means only a human can move it; `done` comes only after that batch -passes its final gate. Which of the two a file records is a report of what happened, never the +passes its final gate with nothing left for a human — a review that did not run, or a finding +waiting on the user, keeps the branch `merged`. Which of the two a file records is a report of what happened, never the authority on it: whether a branch reached its target is git's knowledge, and an interrupted run re-derives it. `done` is skipped on resume; `implemented` and `merged` both re-enter the merge round, where an already-merged branch no-ops. diff --git a/plugins/fd3/references/validation-report.md b/plugins/fd3/references/validation-report.md index 4b8e0f9..bb83176 100644 --- a/plugins/fd3/references/validation-report.md +++ b/plugins/fd3/references/validation-report.md @@ -2,7 +2,7 @@ The shape `validate-spec` returns its verdict and its status in. Read this file before composing a return, and read it again after a compaction — a return composed from memory is where the fixed -rows and the four `Result` forms go missing. +rows and the five `Result` forms go missing. ```markdown ## Run @@ -57,11 +57,12 @@ one. The short names are fixed too: A finding about an element cites the element's code; a finding about the document anchors to a section, on the terms `spec-rules.md` sets. -A `Result` cell reads `pass (unchanged)`, `pass (was fail — )`, or `fail — -
, `. It is the only thing by which the caller can tell that an iteration +A `Result` cell reads `pass (unchanged)`, `pass (was fail — )`, `fail — +
, `, or `non-blocking —
, ` for a check whose only +open findings do not block. It is the only thing by which the caller can tell that an iteration moved, so a check whose answer changed says so where it changed. Each cell is one line. -Check 9 takes a fourth form and needs it: its evidence lives outside the spec, so `unchanged` there +Check 9 takes a fifth form and needs it: its evidence lives outside the spec, so `unchanged` there reports the document, not the lookup. A check 9 that ran and holds reads `pass (verified — )`; `pass (unchanged)` on that row says the lookup never happened. diff --git a/plugins/fd3/skills/grill-topic/SKILL.md b/plugins/fd3/skills/grill-topic/SKILL.md index 21bedca..fad9174 100644 --- a/plugins/fd3/skills/grill-topic/SKILL.md +++ b/plugins/fd3/skills/grill-topic/SKILL.md @@ -89,6 +89,8 @@ If a late fact contradicts a question you already asked, re-ask that numbered qu When an answer collides with a cost you yourself wrote, say so in the acknowledgement before recording it, and name the fact that would settle the collision. Writing the answer down and carrying the contradiction into the summary makes you the author of a conflict the user never saw. +An answer that widens the scope collides with costs you wrote earlier, too. When one does, re-read every answered question whose recommendation quoted a size — files, lines, pull requests, hours — against the new scope; one whose number no longer holds is voided and re-asked, the same way a returning fact voids a question. + ## Closing The session is done when the frontier is empty: every branch of the design tree visited, nothing left silently assumed. diff --git a/plugins/fd3/skills/implement-tasks/SKILL.md b/plugins/fd3/skills/implement-tasks/SKILL.md index 595f837..2315b26 100644 --- a/plugins/fd3/skills/implement-tasks/SKILL.md +++ b/plugins/fd3/skills/implement-tasks/SKILL.md @@ -73,14 +73,14 @@ Then make the graph launchable: the step-2 batch. Only when the field is absent (tasks split before it existed), derive it: order the repository's distinct `branch:` values by their tasks' rollout phase (the numeric prefix of `phase:`; `cleanup` sorts after every numbered phase), the first stacking on the - repository's base ref (`baseBranch: null` — resolved to the `defaultRef` confirmed in step 2), + repository's base ref (`baseBranch: null` — resolved to the `startRef` confirmed in step 2), each later one on the previous unit's branch. A derived base is a guess about a decision the split made — the report says which bases were read and which derived. The workflow starts worktrees and the pull-request chain from these. - **Normalise the root's base.** A `branch-base` no task in that repository builds and naming either the repository's default branch or the branch its checkout is parked on is that repository's root rather than a stack link: it goes to step 3 as `baseBranch: null`, which - resolves to whichever of the two step 2 settles as `defaultRef`. Both are step-1 facts, and each + resolves to whichever of the two step 2 settles as `startRef`. Both are step-1 facts, and each is matched by branch name — the split roots on the default branch for an ordinary run and writes `main` where the ref is `origin/main`, but roots on the parked branch where it found the checkout carrying the spec's commits, and which of the two this run wants is step 2's to answer. @@ -110,17 +110,15 @@ Then make the graph launchable: One batch, following `${CLAUDE_SKILL_DIR}/../../references/question-batching.md`: -- which code-review skills to run during validation — offer only names present in this session's - skill listing, never one recalled from memory; the lens costs roughly a quarter to two fifths - of the run, and a review bot on the pull request finds different things, not the same ones — - `none` is a valid answer but a real trade. **Offer only skills that review inline.** A review - agent in the workflow has no `Agent` tool, so a skill or command that fans out into scanners of - its own — `code-review:start-cr` is the one to watch for — cannot do what its name promises - there: it quietly reviews everything itself in one pass, which is the single perspective the - fan-out exists to avoid. When the user names one anyway, expand it into the single-lens skills - it orchestrates (for `start-cr`: `code-review:quality-review`, `code-review:comment-review`, - `code-review:security-review`), pass those as `reviewSkills`, and say that is what you did — - each becomes its own review agent, which is the fan-out the workflow can actually run; +- whether to review each branch. Review is the `code-review` plugin's headless lenses — all + eight, one agent each, after the branch's scoped CI passes — then a delta review of whatever + the automatic fixes changed. It costs roughly a quarter to two fifths of the run, and a review + bot on the pull request finds different things, not the same ones: `no` is a valid answer but a + real trade. Recommend it when `code-review:cr-scan` is in this session's skill listing; when it + is not, ask anyway and say the plugin must be installed — a listing can be withheld or cut + short, and a review whose skill is missing comes back as no verdict, never as a clean pass. Take + no other skill in its place: a skill that asks questions or fans out into agents of its own + cannot run inside a workflow agent; - the spec path, when the `spec:` pointers did not resolve to an existing file in step 1; - any unresolved repository paths, `branch-base:` disagreements and stale `in-progress` calls from step 1; @@ -157,9 +155,10 @@ Workflow({ args: { specPath: "", tasks: [{ slug, file, name, repository, branch, baseBranch, phase, dependsOn, status }, ...], - repos: { "": { defaultRef: "", + repos: { "": { startRef: "", + diffBase: "origin/", parkedBranch: "" }, ... }, - reviewSkills: [], + review: , maxFixRounds: 3, reportPath: ` path — omit on a first launch> } @@ -168,8 +167,11 @@ Workflow({ `file` and `repository` are absolute paths (`repository: "none"` for operational tasks); `dependsOn` carries bare slugs; `baseBranch` is the stack base from step 1, `null` for the -repository's `defaultRef` — the ref the user confirmed in step 2, fetched fresh in step 1. -Worktrees and target branches are cut from that ref (or the task's stack base). When step 2 +repository's `startRef` — the ref the user confirmed in step 2, fetched fresh in step 1. +`diffBase` is always `origin/`: it is what a root branch's diff, its scoped CI and its +review are measured against. On a run that builds on the parked branch, `startRef` names that +branch, and measuring against it would give every review an empty diff. +Worktrees and target branches are cut from `startRef` (or the task's stack base). When step 2 established that a target branch is the branch the repository itself is parked on, say so via `parkedBranch` — git refuses a second worktree for it, and the workflow must know to use the main checkout rather than discover the refusal. Merges and fixes for that branch then happen in @@ -188,7 +190,8 @@ dispatch implementation agents yourself. The completion notification truncates the result — read the full report from the notification's `` path before relaying anything. The workflow returns per-task statuses, -per-branch validation outcomes (with each branch's review findings), agent `caveats`, the HIL +per-branch validation outcomes (with each branch's review findings and the path of its review +`report.md`), agent `caveats`, the HIL list, the tasks left unreachable behind blockers, and its `toolchain` and `baseline` knowledge. Relay it faithfully — a failed CI stays failed in the telling, and any totals you state are the report's own `tasks[]` tally, never hand-counted — with one distinction the report already @@ -203,6 +206,14 @@ that base implied did not happen. Relay each as a diagnostic saying which it was asks for is a corrected task file before the next split or relaunch, never a decision that moves those tasks, so neither is one of the items step 4 puts to the user below. +A `review` item is a finding the workflow would not fix unasked — a `spec` finding, a `security` +fix, a boy-scout finding on code the branch never touched, a report-only one, a finding the fixer +left unfixed, or anything the delta review found in the fixes it did apply. Its branch +passed CI and stays `merged` until the item is settled: a finding the user wants fixed goes to a +repair; one the user dismisses needs no work. When every `review` item of a branch is dismissed, +set that branch's tasks to `done` yourself — CI already passed on the commit they sit on. Give +the `report.md` path with the findings: the one-line form in the HIL list drops the evidence. + Caveats are triaged, not relayed wholesale: one that names a decision the agent took, a risk, an as-built deviation or a commit no review saw goes to the user; one that reports compliance with its own prompt, or restates what the task file already records, does not. Write the full @@ -230,17 +241,25 @@ composed from the plausible one costs a full round to disprove. The answers spli repairs: [{ repo, branch, worktree, base, instructions: [], taskFiles: [] }, ...], repos: , + specPath: , + review: , reportPath: ` path>, maxFixRounds: 3 } }) ``` - `base` is the branch's stack base (or the repo's `defaultRef`). `reportPath` is the previous + `base` is the branch's stack base (or the repo's `diffBase`). `reportPath` is the previous report's output file; the workflow reads its toolchain and baseline knowledge itself, which is what spares a full re-scout plus a full baseline pipeline on every repository in the run. Pass the path, never the knowledge. Repair agents receive the decision as their sole - authority and never read the spec. Repair validation is CI only — no code review. + authority and never read the spec. + + A repair reopens its branch. Before launch, set every `done` task on a repaired branch back to + `merged` and list it in that branch's `taskFiles` — the tasks were marked done on a tree the + repair is about to change, and only the repair's own final gate may mark them again. With + review on, the workflow reviews the repair's own commits after its CI passes; those findings + come back as `review` items for the user, never to a fixer. An `instructions` line says what to change, never asks for validation. "Then run the tests and confirm they pass", "verify the build is green" — the workflow runs CI itself, after the agent diff --git a/plugins/fd3/skills/split-to-tasks/SKILL.md b/plugins/fd3/skills/split-to-tasks/SKILL.md index 71f32bb..e5aa8c1 100644 --- a/plugins/fd3/skills/split-to-tasks/SKILL.md +++ b/plugins/fd3/skills/split-to-tasks/SKILL.md @@ -52,10 +52,18 @@ blocks from being *written* draws a `depends-on` edge onto it (step 4's authorsh Anything short of that — a blocked claim, a count that does not match, a dated block with no verdict line, no pass anywhere — is a stop before step 1. A dated heading over verified rows is not -a verdict. Validating is not this skill's work, and no command is named for it: on *validate first* -the split ends with nothing written. What lifts the stop is the user's answer, never your own — say -what the record holds, then ask, once, whether to validate first or split as-is. The message that -ends the run says what the record held and which way the user answered. +a verdict. Validating is not this skill's work: on *validate first* the split ends with nothing +written and names the route — `/fd3:build-spec `, which takes a finished spec straight to +validation. What lifts the stop is the user's answer, never your own — say what the record holds, +then ask, once, whether to validate first or split as-is, following +`${CLAUDE_SKILL_DIR}/../../references/question-batching.md` with *validate first* as the +recommendation. The message that ends the run says what the record held and which way the user +answered. + +**Never write a verdict.** An evidence block, a verdict line or a validation status in the spec is +written by `fd3:validate-spec` alone — not by this skill, and not when the user asks it to, because a +verdict nothing validated is exactly what the precondition above would then trust. A spec that needs +repairing goes through the route above, and the split stops. ## Workflow @@ -92,6 +100,14 @@ current branch is behind it. The base you write into `branch-base:` is the ref t stage cuts worktrees from; a base derived from a week-old local ref is a worktree that starts from the wrong commit, and nothing downstream re-derives it. +The last evidence block records the commit each repository was validated at, on its `Checked at:` +line; a block older than that line is measured from the commit that last changed the spec file. Where `origin/` +has moved past it, the spec may describe code that is no longer there: intersect `git diff --name-only +..origin/` with the paths the spec cites. No hit, and the split goes on. A +hit is a stop before anything is written — name the paths the new commits changed and the route +`/fd3:build-spec `; rebasing a branch or correcting the spec to follow is that route's +work, never this skill's. + Record each repository's absolute root path and write that path into `repository:`. A remote slug is not a location: the next stage resolves it by guessing among the user's checkouts, and a feature worked on in a second worktree is exactly where the guess goes wrong. @@ -190,7 +206,15 @@ migration one writes and the other reads. Name it in the dependent task's `## No report's table — the frontmatter field stays a bare slug list, because prose in a machine-read field breaks the reader. An edge you cannot name that way is sequencing by intuition: drop it. It buys nothing on a shared branch and costs the implementation stage a serialisation, since tasks -with no edge between them are implemented concurrently. Never draw an edge onto +with no edge between them are implemented concurrently. + +**A commit sequence the spec binds is an edge too.** Where the spec orders the commits inside one +branch — one commit per module in a stated order, a gate that every commit from some point on must +pass — tasks that follow that order land in it only if the graph says so: concurrent tasks merge in +whatever order they finish, and a per-commit gate is then never checked in sequence. Give each such +task a `depends-on` edge onto its predecessor in the spec's order, and let its `## Note` quote the +spec's ordering sentence in place of a shared file or symbol; the drop rule above does not apply to +these edges. Never draw an edge onto an operational task when the spec lets the code land before that gate — an edge there strands implementable work behind human hands, and a whole extra run pays for it; a dependency that only gates *verification* belongs in the task's Done-when, not in the graph. A gate that blocks @@ -202,7 +226,9 @@ following its repository's visible convention — existing branches show it; the group, not the task. One exception joins the step-6 batch: when the checkout already sits on a branch carrying implementation commits for this spec, whether the first landing unit reuses that branch or cuts fresh by the convention is the user's call — a user mid-feature may have chosen it -deliberately. A branch that carries only the spec file itself is not that case. +deliberately. A branch that carries only the spec file itself is not that case — unless the spec +names that branch as where its work lands, which makes reuse the spec's own answer: then the +question joins the batch with reuse as the recommendation. When the edge onto an operational task is real, carry it up to the branch: a landing unit that mixes a gate-blocked task with implementable ones cannot reach a complete state in one run. Cut @@ -289,8 +315,9 @@ over the written files — coverage is the one check a fan-out cannot perform on of five files or fewer is faster written here. Write the report to `.split.md` beside the spec — never inside `tasks/`, where -a task-file glob trips over it. It carries one table (slug, repository, branch, phase, -depends-on, elements), the branch creation order and stack chain per repository, where the +a task-file glob trips over it — after that re-run, since its coverage statement is the re-run's +result. It carries one table (slug, repository, branch, phase, depends-on as the bare slugs the +frontmatter holds, elements), the branch creation order and stack chain per repository, where the files went, the coverage statement from step 5, every work item split across tasks with its seam, any size-check warning, the verdict line this split was taken against quoted verbatim, and anything the user still owes an answer. In the conversation give the path and the same diff --git a/plugins/fd3/skills/validate-spec/SKILL.md b/plugins/fd3/skills/validate-spec/SKILL.md index 4c1b23c..b809973 100644 --- a/plugins/fd3/skills/validate-spec/SKILL.md +++ b/plugins/fd3/skills/validate-spec/SKILL.md @@ -14,6 +14,12 @@ need, then end your turn. Do not guess it and do not go looking for it. **That spec file is the only file you may edit.** Everything else you read is read-only, no matter what you find in it. +A probe — a command you run to settle a claim — runs in a detached worktree under your scratchpad +(`git worktree add --detach / `), never in the user's checkout. It never +installs, links or rebuilds dependencies, anywhere: a package manager repoints shared links and +leaves the user's tree broken. A claim that needs more than that to settle stays unverified, and the +report says what the probe would have needed. + **Every edit traces to a finding of this pass.** The dated evidence block is appended to a spec of any quality; everything else you write must be the repair of something you recorded as a finding, in the section that finding names. A spec whose checks all pass leaves this skill byte-identical @@ -120,7 +126,8 @@ report**, whether it passed or not. A row is `pass` only when its prose names no unresolved finding. Where a finding genuinely does not block — the Terms say when — it is still a finding: give it its own row in the findings list and say why it does not block. Calling it non-blocking inside a passing row hides it from the verdict, and -from whoever splits this into tasks. +from whoever splits this into tasks. The row itself reads `non-blocking — `: never `fail` +over a `ready` verdict, and never `pass (unchanged)` while that finding stays open. 1. Design decisions do not contradict one another within the authoritative set. Where the spec declares precedence over another document, that declaration settles the disagreement; what to look for instead is @@ -167,7 +174,9 @@ record. Spot-check its rows and append to it under a dated sub-heading, so the s stays distinguishable from this run's. When the spec has none, add one at the end. Once the verdict is known, open the dated block with one line — `Verdict: — claims: N verified / N deferred / N blocked — spec N lines at this verdict` — so a later reader can tell a clean pass from -a qualified one without hunting for the session that produced it. Fill the line count in last: write +a qualified one without hunting for the session that produced it. The line under it names the +commit each repository was checked against in step 0 — `Checked at: @ , …` +— which is how the split tells whether the code has moved on since. Fill the line count in last: write the dated block through to its final line, then `wc -l`, then put that number in the verdict line — replacing it changes no line count, so the number counts itself. A file at `/evidence/
.md` is for overflow only: a probe transcript or a command output too long to diff --git a/plugins/fd3/workflows/implement-run.js b/plugins/fd3/workflows/implement-run.js index c68d36d..f1a4627 100644 --- a/plugins/fd3/workflows/implement-run.js +++ b/plugins/fd3/workflows/implement-run.js @@ -5,7 +5,7 @@ export const meta = { phases: [ { title: 'Recon', detail: 'toolchain detection and a baseline run per repository' }, { title: 'Implement', detail: 'parallel waves gated by the depends-on graph, each wave merged into its target branch' }, - { title: 'Validate', detail: 'CI then code review, one branch at a time' }, + { title: 'Validate', detail: 'CI, then a code-review pass and a delta review of its fixes, one branch at a time' }, ], } @@ -16,12 +16,15 @@ export const meta = { // the branch this task's target branch stacks on, or null for the repository's // base ref; status is one of todo | implemented | merged | blocked | done (stale // in-progress is reset by the skill before launch) -// repos { [repository path]: { defaultRef, parkedBranch } } — defaultRef is the ref new -// branches are cut from (e.g. "origin/main"); the skill fetches before launch, so -// origin/* is fresh. parkedBranch (optional) names the target branch checked out -// in the main repository itself — git refuses a second worktree for it, so the -// main checkout serves as that branch's worktree -// reviewSkills code-review skill names to run during validation, may be empty +// repos { [repository path]: { startRef, diffBase, parkedBranch } } — startRef is the ref +// new branches are cut from (e.g. "origin/main", or the branch the user parked the +// checkout on); diffBase is what a root branch's diff is measured against, always +// origin/; the skill fetches before launch, so origin/* is fresh. +// parkedBranch (optional) names the target branch checked out in the main +// repository itself — git refuses a second worktree for it, so the main checkout +// serves as that branch's worktree +// review true to run the code-review plugin's headless lenses on every branch after its +// scoped CI passes; false skips review // maxFixRounds CI fix attempts per branch before giving up // reportPath (optional) absolute path of a previous run's report file; one cheap agent reads // its toolchain and baseline knowledge, so a relaunch skips the re-scout and the @@ -33,7 +36,8 @@ export const meta = { // args can arrive JSON-encoded depending on the caller; normalize before destructuring const input = typeof args === 'string' ? JSON.parse(args) : args -const { specPath, tasks, repos, reviewSkills } = input +const { specPath, tasks, repos } = input +const review = input.review === true // Undefined would make every `fixRounds < maxFixRounds` false and silently skip the fix rounds // the run exists to perform, reporting failures it was built to repair. const maxFixRounds = input.maxFixRounds ?? 3 @@ -55,8 +59,11 @@ const worktreePath = (repo, name) => `${repo}.worktrees/${name.replace(/\//g, '- // Every validation label names the unit, never just the repository: one repository carries many // target branches, and without the branch a fix loop and a fan-out look identical in the run view. const unitTag = (unit) => `${unit.repo.split('/').pop()}:${unit.branch.replace(/\//g, '-')}` -const lensTag = (skill) => skill.replace(/:/g, '/') // a review skill's own name carries a colon -const repoDefault = (repo) => (repos && repos[repo] && repos[repo].defaultRef) || "the repository's default branch" +// Where work starts and what it is measured against part ways when the run builds on a branch the +// user parked the checkout on: worktrees start from that branch, and a diff against it is empty. +const refOf = (repo, key) => (repos && repos[repo] && repos[repo][key]) || "the repository's default branch" +const startRef = (repo) => refOf(repo, 'startRef') +const diffBase = (repo) => refOf(repo, 'diffBase') const byRepo = (list) => { const groups = new Map() for (const t of list) { @@ -65,10 +72,15 @@ const byRepo = (list) => { } return groups } +// The harness relays the user's request to every agent, and a cheap model reads it as its own task +// and re-runs the skill that launched this workflow. +const STEP_GUARD = + 'Do only the step this prompt describes. Invoke no skill or slash command it does not name, ' + + 'whatever the relayed user request says: that request belongs to the session that launched this workflow.\n\n' // One retry rides out transient API failures (529s, brief limit blips). A second null is a real // no-verdict — absence of evidence that must never be reported as a validation verdict. const tryTwice = async (prompt, opts) => - (await agent(prompt, opts)) ?? agent(prompt, { ...opts, label: `${opts.label}:retry` }) + (await agent(STEP_GUARD + prompt, opts)) ?? agent(STEP_GUARD + prompt, { ...opts, label: `${opts.label}:retry` }) // ---- Recon: detect each repository's validation toolchain, then baseline it on the clean base // (both kept in memory for this run and returned in the report for a later repair-run) @@ -171,8 +183,10 @@ const baselinePrompt = (repo) => [ `Establish the validation baseline of the repository ${repo} on its clean base.`, ``, - `1. Create a worktree at ${worktreePath(repo, 'baseline')} from ${repoDefault(repo)}`, - ` (git worktree add ) unless it already exists — then reuse it as is.`, + `1. Create a worktree at ${worktreePath(repo, 'baseline')} from ${startRef(repo)}`, + ` (git worktree add --detach ) unless it already exists — then reuse it as is.`, + ` Detached, because the ref may be a branch already checked out elsewhere, which git refuses`, + ` a second worktree for.`, `2. Run every runnable validation command from the toolchain report below, in the reported`, ` order, sequentially — never in parallel. Each command's cwd in the report is relative to`, ` the repository root: resolve it inside that worktree, never against ${repo}. Skip what the`, @@ -215,7 +229,7 @@ const baselineText = (repo) => { ? `- ${c.command}: passed` : `- ${c.command}: FAILED on the clean base:\n` + (c.failures || []).map((f) => ` ${f}`).join('\n'), ) - return `Baseline on the clean base (${repoDefault(repo)}):\n${lines.join('\n')}` + return `Baseline on the clean base (${startRef(repo)}):\n${lines.join('\n')}` } // ---- Implement: waves of parallel tasks gated by depends-on, each wave merged before the next @@ -282,7 +296,7 @@ const implementPrompt = (task) => { `3. In the repository ${task.repository}, create that worktree on a new branch ${taskBranch(task)}`, ` (git worktree add -b ). The start-point is the first of these`, ` refs that exists — an early wave can run before the later ones are created:`, - ` ${[task.branch, task.baseBranch, repoDefault(task.repository)].filter(Boolean).join(', then ')}.`, + ` ${[task.branch, task.baseBranch, startRef(task.repository)].filter(Boolean).join(', then ')}.`, ` Everything this task depends on is already merged into whichever you start from. If the`, ` worktree already exists from an interrupted attempt, continue in it instead of recreating`, ` anything.`, @@ -359,7 +373,7 @@ const mergePrompt = (repo, repoTasks) => { const plan = [...targets.entries()] .map( ([branch, g]) => - `- target ${branch} (stacks on ${g.base || repoDefault(repo)}, worktree ${branch === parked(repo) ? `${repo} — the main checkout` : worktreePath(repo, branch)}):\n` + + `- target ${branch} (stacks on ${g.base || startRef(repo)}, worktree ${branch === parked(repo) ? `${repo} — the main checkout` : worktreePath(repo, branch)}):\n` + g.tasks.map((t) => ` - ${taskBranch(t)} (task ${t.slug}, file ${t.file})`).join('\n'), ) .join('\n') @@ -399,7 +413,7 @@ const recordUnit = (repo, b, slugs) => { let unit = units.find((u) => u.repo === repo && u.branch === b.branch) if (!unit) { const sample = tasks.find((t) => t.repository === repo && t.branch === b.branch) - unit = { repo, branch: b.branch, worktree: b.worktree, base: (sample && sample.baseBranch) || repoDefault(repo), tasks: [] } + unit = { repo, branch: b.branch, worktree: b.worktree, base: (sample && sample.baseBranch) || diffBase(repo), tasks: [] } units.push(unit) } for (const slug of slugs) if (!unit.tasks.includes(slug)) unit.tasks.push(slug) @@ -580,15 +594,62 @@ const FIX_RESULT = { required: ['summary'], properties: { summary: { type: 'string' }, + fromSha: { type: 'string', description: '`git rev-parse HEAD` in the worktree before the first edit' }, caveats: { type: 'array', items: { type: 'string' }, description: 'problems skipped with the reason, judgment calls that went beyond the listed problems, and any change that touches a spec decision' }, + skipped: { type: 'array', items: { type: 'string' }, description: 'every listed problem left unfixed, copied verbatim from the list' }, }, } -const CR_RESULT = { +const PREP_RESULT = { type: 'object', - required: ['findings'], + required: ['status', 'invoked'], properties: { - findings: { type: 'array', items: { type: 'string' }, description: 'one entry per finding worth fixing: file, problem, why' }, + invoked: { type: 'boolean', description: 'the code-review:cr-prepare skill was invoked through the Skill tool' }, + status: { enum: ['ready', 'empty', 'error'] }, + files: { type: 'number', description: 'judged files' }, + active: { type: 'array', items: { type: 'string' }, description: 'active lenses, by name' }, + inactive: { type: 'array', items: { type: 'string' }, description: 'inactive lenses, each with its reason' }, + alternate: { type: 'string', description: 'the alternate base and its file count; only when status is empty and the skill named one' }, + reason: { type: 'string', description: 'only when status is error' }, + }, +} + +const SCAN_RESULT = { + type: 'object', + required: ['status', 'invoked', 'filesJudged', 'filesTotal'], + properties: { + invoked: { type: 'boolean', description: 'the code-review:cr-scan skill was invoked through the Skill tool' }, + status: { enum: ['scanned', 'inactive', 'error'] }, + filesJudged: { type: 'number' }, + filesTotal: { type: 'number' }, + }, +} + +const CR_MERGE_RESULT = { + type: 'object', + required: ['status', 'invoked', 'findings'], + properties: { + invoked: { type: 'boolean', description: 'the code-review:cr-merge skill was invoked through the Skill tool' }, + status: { enum: ['merged', 'incomplete', 'error'] }, + report: { type: 'string', description: 'absolute path of report.md' }, + missing: { type: 'array', items: { type: 'string' }, description: 'lenses that did not report; only when status is incomplete' }, + findings: { + type: 'array', + items: { + type: 'object', + required: ['severity', 'family', 'rule', 'location', 'risk', 'fix'], + properties: { + severity: { type: 'string', description: 'high | medium | nit; `comment` for a comment verdict' }, + family: { type: 'string' }, + rule: { type: 'string', description: 'the rule, or for a comment verdict R and its verdict' }, + location: { type: 'string', description: ':L' }, + risk: { enum: ['safe', 'structural', 'report-only'] }, + fix: { type: 'string' }, + reserved: { type: 'boolean', description: 'a direct consequence of the open work the prompt lists' }, + boyScout: { type: 'boolean', description: 'the finding line carries the `boy-scout` token: it is about code the change did not touch' }, + }, + }, + }, }, } @@ -600,22 +661,31 @@ const CR_RESULT = { const validationTree = (unit) => unit.worktree === unit.repo ? `${unit.repo}.worktrees/${unit.branch.replace(/\//g, '-')}-validate` : unit.worktree +const treeSetup = (unit) => { + const tree = validationTree(unit) + return tree === unit.worktree + ? [] + : [ + `That worktree is this branch's validation checkout, detached at its commit. Create it`, + `with \`git worktree add --detach ${tree} ${unit.branch}\` if it is not there; if it is,`, + `bring it to the branch's current commit with \`git -C ${tree} checkout --detach`, + `${unit.branch}\`. Never \`git clean\` it — installed dependencies live there untracked.`, + ``, + ] +} + const ciPrompt = (unit, mode, markFiles) => { const tree = validationTree(unit) return [ `Run the validation commands for the repository ${unit.repo}, branch ${unit.branch},`, `in the worktree ${tree}. Run them in the reported order, sequentially — never in`, - `parallel.`, + `parallel. Run each one as \` > ${tree}.ci.log 2>&1; echo "exit $?"\` — the log sits`, + `beside the worktree, never inside it — read pass or fail from that exit line alone, and read`, + `the log only for the lines a failure needs. Never pipe a command into \`tail\`, \`head\` or`, + `\`grep\`: the pipe's status replaces the command's, and a failing suite then reads as whatever`, + `its output happens to show.`, ``, - ...(tree === unit.worktree - ? [] - : [ - `That worktree is this branch's validation checkout, detached at its commit. Create it`, - `with \`git worktree add --detach ${tree} ${unit.branch}\` if it is not there; if it is,`, - `bring it to the branch's current commit with \`git -C ${tree} checkout --detach`, - `${unit.branch}\`. Never \`git clean\` it — installed dependencies live there untracked.`, - ``, - ]), + ...treeSetup(unit), `\`cd ${tree}\` before anything else, and confirm what you are about to grade: \`git rev-parse`, `HEAD\` there must equal \`git -C ${unit.repo} rev-parse ${unit.branch}\`. When they match,`, `return branch "${unit.branch}". When they do not, run nothing: return the branch you actually`, @@ -680,44 +750,132 @@ const fixPrompt = (unit, problems, source) => `take or approve — anything that touches production, anything irreversible, any secret or`, `credential, generating a migration, and whatever this repository's own rules reserve — is a`, `blocker, not a fix: skip it and record it under caveats. Never guess your way past it.`, + `A failure the fix can only clear by changing behaviour — an exception carved out of a rule,`, + `a narrowed decision, an error that stops meaning what it meant — is a design call, not a fix:`, + `skip it and record under caveats what the choice is.`, ``, `Toolchain report for this repository — when a fix changes something a listed command`, `derives an artifact from, regenerate that artifact the way the report says:`, ``, toolchain.get(unit.repo), ``, + `Before your first edit, run \`git rev-parse HEAD\` in the worktree and return it as fromSha.`, `Commit the fixes with a conventional-commit message. To verify a fix you may re-run the`, - `exact commands that failed — never the full validation suite; it is rerun after you.`, + `exact commands that failed, scoped to the files they failed on — never a whole suite; the`, + `full validation runs after you.`, ``, `Return a two-sentence summary. Under caveats, return every problem you skipped with the`, `reason, any judgment call that went beyond the listed problems, and any change that touches`, `a decision recorded in the spec.`, `A caveat says something the parent could not otherwise know. Following this prompt is not a`, `caveat.`, + ...(source === 'code-review' + ? [`Under skipped, also copy verbatim every listed problem you left unfixed, whatever the reason.`] + : []), ].join('\n') -const reviewPrompt = (unit, skillName) => - [ - `In the worktree ${unit.worktree} (repository ${unit.repo}), review the branch ${unit.branch}:`, - `invoke the \`${skillName}\` skill via the Skill tool on the diff between this branch and`, - `${unit.base}. Report, do not fix.`, - ``, - `\`git diff ${unit.base}...HEAD\` is the whole territory: the files it touches and the direct`, - `call sites of what they change. The rest of the repository, the architecture and the`, - `validation suite are out of scope. The narrowing is of territory, not of severity.`, +// The lens skills read a context directory, never this prompt, so the run's own facts — which tree, +// which base, what is deliberately unfinished — reach them only through the wrapper agents. +const reviewDir = (unit, pass) => `${unit.repo}.worktrees/.review/${unit.branch.replace(/\//g, '-')}-${pass}` + +const prepPrompt = (unit, base, dir) => { + const tree = validationTree(unit) + return [ + `Prepare a code review of the branch ${unit.branch} (repository ${unit.repo}) in the checkout`, + `${tree}, measured from ${base}.`, ``, - baselineText(unit.repo), + ...treeSetup(unit), + `\`git -C ${tree} rev-parse HEAD\` must equal \`git -C ${unit.repo} rev-parse ${unit.branch}\`;`, + `when it does not, invoke nothing and return status "error" with the mismatch as reason.`, ``, - ...(reservations ? [reservations, ``] : []), - `Report only findings this branch's diff introduces. An issue that exists identically on the`, - `clean base — including everything in the baseline above — is pre-existing: leave it out. So`, - `is the open work listed above and its direct consequences: a gap a blocked or human-owned`, - `task deliberately leaves is not a finding.`, + `Remove ${dir} if it exists — lens files left there by an earlier run would be read as this`, + `run's. Then invoke the \`code-review:cr-prepare\` skill through the Skill tool with:`, + `\`--base ${base} --out ${dir} -C ${tree} --spec ${specPath}\``, ``, - `Return only the findings worth fixing, one entry each: file, the problem, and why it matters.`, - `An empty findings array is a valid result.`, + `Return what its closing block says: status, judged file count, active and inactive lenses,`, + `the alternate on empty, the reason on error — and invoked=true. When the Skill tool is not`, + `available to you, return invoked=false and status "error"; never do the skill's work by hand.`, + ].join('\n') +} + +const scanPrompt = (lens, dir) => + [ + `Invoke the \`code-review:cr-scan\` skill through the Skill tool with`, + `\`--lens ${lens} --context ${dir}\`, and return what its closing block says — status, files`, + `judged of the total — with invoked=true. When the Skill tool is not available to you, return`, + `invoked=false, status "error" and zero counts; never judge the change by hand.`, ].join('\n') +const crMergePrompt = (dir) => + [ + `Invoke the \`code-review:cr-merge\` skill through the Skill tool with \`--context ${dir}\`,`, + `and return what its closing block says: status, the report path, the lenses that did not`, + `report on incomplete, and every finding line as one entry — severity, family, rule, location,`, + `risk class and the fix. A comment verdict's severity is \`comment\`; a line carrying the`, + `\`boy-scout\` token sets boyScout=true. Return invoked=true; when`, + `the Skill tool is not available to you, return invoked=false, status "error" and no findings.`, + ...(reservations + ? [ + ``, + reservations, + ``, + `Set reserved=true on a finding that is this open work or its direct consequence — a gap a`, + `blocked or human-owned task deliberately leaves. Change nothing else the skill returned.`, + ] + : []), + ].join('\n') + +// A review that did not run is absence of evidence: an empty change, a lens that judged nothing or a +// skill the agent never invoked all come back as a reason, never as an empty findings list. +const runReview = async (unit, base, pass, tag) => { + const dir = reviewDir(unit, pass) + const prep = await tryTwice(prepPrompt(unit, base, dir), { label: `cr-prep:${tag}:${pass}`, phase: 'Validate', schema: PREP_RESULT }) + if (!prep || !prep.invoked || prep.status === 'error') { + return { dir, dead: `cr-prepare ${prep ? `failed: ${prep.reason || 'the skill was not invoked'}` : 'returned no result after a retry'}` } + } + if (prep.status === 'empty') { + return { dir, empty: true, dead: `the change against ${base} is empty${prep.alternate ? ` (the skill names ${prep.alternate})` : ''}` } + } + const lenses = prep.active || [] + const scans = await parallel( + lenses.map((lens) => () => + tryTwice(scanPrompt(lens, dir), { label: `cr:${tag}:${pass}:${lens}`, phase: 'Validate', schema: SCAN_RESULT }), + ), + ) + const deadLenses = lenses.filter((_, i) => { + const r = scans[i] + return !r || !r.invoked || r.status === 'error' || (r.status === 'scanned' && r.filesTotal > 0 && r.filesJudged === 0) + }) + if (deadLenses.length > 0) return { dir, dead: `lens ${deadLenses.join(', ')} did not report` } + const merged = await tryTwice(crMergePrompt(dir), { label: `cr-merge:${tag}:${pass}`, phase: 'Validate', schema: CR_MERGE_RESULT }) + if (!merged || !merged.invoked || merged.status !== 'merged') { + const why = !merged + ? 'returned no result after a retry' + : merged.status === 'incomplete' + ? `found lens ${(merged.missing || []).join(', ')} missing` + : 'failed' + return { dir, dead: `cr-merge ${why}` } + } + return { dir, report: merged.report, files: prep.files || 0, lenses: lenses.length, findings: merged.findings } +} + +// Security fixes change behaviour at a boundary, spec findings are work, not edits, and a boy-scout +// fix edits code the task never touched — all three wait for a human; nits and the remaining +// comment verdicts are reported, never applied unasked. +const serious = (f) => f.severity === 'high' || f.severity === 'medium' +const sortFindings = (findings) => { + const live = findings.filter((f) => !f.reserved) + const applied = live.filter( + (f) => + (serious(f) && f.risk !== 'report-only' && f.family !== 'security' && f.family !== 'spec' && !f.boyScout) || + (f.severity === 'comment' && f.risk === 'safe'), + ) + const forHuman = live.filter((f) => !applied.includes(f) && serious(f)) + const reported = live.filter((f) => !applied.includes(f) && !forHuman.includes(f)) + return { applied, forHuman, reported, reserved: findings.length - live.length } +} +const findingLine = (f) => `${f.location} — ${f.family} · ${f.rule} (${f.severity}): ${f.fix}` + const mechanical = { model: 'haiku', effort: 'high' } // CI runners interpret command output; they design nothing // A CI verdict is a statement about one tree at one commit. A runner that stayed in the @@ -835,33 +993,63 @@ for (const unit of units) { } summary.ci = 'passed' - // Review skills are read-only lenses; they can run in parallel — unlike CI they hog no cores. - let deadLenses = [] - if (reviewSkills.length > 0) { - const reviews = await parallel( - reviewSkills.map((skillName) => () => - tryTwice(reviewPrompt(unit, skillName), { label: `cr:${tag}:${lensTag(skillName)}`, phase: 'Validate', schema: CR_RESULT }), - ), - ) - deadLenses = reviewSkills.filter((_, i) => !reviews[i]) - const findings = reviews.filter(Boolean).flatMap((r) => r.findings) - summary.reviewFindings = findings.length - summary.findings = findings - if (findings.length > 0) { - const fix = await tryTwice(fixPrompt(unit, findings, 'code-review'), { label: `fix-cr:${tag}`, phase: 'Validate', schema: FIX_RESULT }) - if (fix && fix.caveats) caveats.push(...fix.caveats.map((c) => `${unit.branch} fix-cr: ${c}`)) + let reviewDead = null // why the review has no verdict for this branch + let reviewHeld = false // findings wait for a human + if (review) { + const first = await runReview(unit, unit.base, 'review', tag) + summary.review = { context: first.dir, report: first.report || null } + if (first.dead) { + reviewDead = `the review did not run: ${first.dead}` + } else { + const sorted = sortFindings(first.findings) + summary.reviewFindings = first.findings.length + summary.findings = [...sorted.applied, ...sorted.forHuman, ...sorted.reported].map(findingLine) + Object.assign(summary.review, { applied: sorted.applied.length, forHuman: sorted.forHuman.length, reported: sorted.reported.length, reserved: sorted.reserved }) + if (sorted.applied.length > 0) { + const fix = await tryTwice(fixPrompt(unit, sorted.applied.map(findingLine), 'code-review'), { label: `fix-cr:${tag}`, phase: 'Validate', schema: FIX_RESULT }) + if (fix && fix.caveats) caveats.push(...fix.caveats.map((c) => `${unit.branch} fix-cr: ${c}`)) + // A finding the fixer declined is still open, and the delta review cannot see it: it only + // reads what the fixer changed. + const declined = (fix && fix.skipped) || [] + const unfixed = sorted.applied.filter((f) => declined.includes(findingLine(f))) + sorted.forHuman.push(...unfixed) + const unmatched = declined.filter((line) => !unfixed.some((f) => findingLine(f) === line)) + for (const line of unmatched) { + hil.push({ slug: null, kind: 'review', reason: `${unit.repo} ${unit.branch}: ${line} — the fixer left it unfixed; the branch keeps status merged until a repair settles it` }) + } + if (unmatched.length > 0) reviewHeld = true + // The fixes are new code nobody has reviewed; one delta pass, whose findings go to a human. + if (!fix || !fix.fromSha) { + reviewDead = `the review fixes have no verdict: the fix agent ${fix ? 'did not report its starting commit' : 'returned no result after a retry'}` + } else { + const delta = await runReview(unit, fix.fromSha, 'delta', tag) + summary.review.delta = { context: delta.dir, report: delta.report || null } + if (delta.dead && !delta.empty) { + reviewDead = `the delta review of the fixes did not run: ${delta.dead}` + } else if (!delta.empty) { + const d = sortFindings(delta.findings) + const open = [...d.applied, ...d.forHuman] + summary.review.delta.findings = open.length + sorted.forHuman.push(...open) + } + } + } + for (const f of sorted.forHuman) { + hil.push({ slug: null, kind: 'review', reason: `${unit.repo} ${unit.branch}: ${findingLine(f)} — the branch keeps status merged until a repair settles it` }) + } + reviewHeld = reviewHeld || sorted.forHuman.length > 0 } } // The full command list is the branch's final gate — always, review fixes or not. - let { ci: finalCi, fault: finalFault } = await runCi(unit, 'full', deadLenses.length === 0, `ci:${tag}:final`) + let { ci: finalCi, fault: finalFault } = await runCi(unit, 'full', !reviewDead && !reviewHeld, `ci:${tag}:final`) if (finalCi && !finalFault && !finalCi.passed) { // One fix round here: a final-gate failure is often mechanical — a derived artifact the // review fixes invalidated — and only what survives the round deserves a human. summary.fixRounds += 1 const fix = await tryTwice(fixPrompt(unit, finalCi.failures, 'final-gate CI'), { label: `fix-final:${tag}`, phase: 'Validate', schema: FIX_RESULT }) if (fix && fix.caveats) caveats.push(...fix.caveats.map((c) => `${unit.branch} fix-final: ${c}`)) - ;({ ci: finalCi, fault: finalFault } = await runCi(unit, 'full', deadLenses.length === 0, `ci:${tag}:final#2`)) + ;({ ci: finalCi, fault: finalFault } = await runCi(unit, 'full', !reviewDead && !reviewHeld, `ci:${tag}:final#2`)) } if (!finalCi || finalFault) { summary.ci = 'no-verdict' @@ -886,16 +1074,17 @@ for (const unit of units) { continue } - // A dead lens is not an empty findings list: the branch stays merged until reviewed. - if (deadLenses.length > 0) { + // An unreviewed branch, or one with findings a human must settle, is not done: it stays merged. + if (reviewDead) { hil.push({ slug: null, kind: 'no-verdict', stage: 'review', - reason: `${unit.repo} ${unit.branch}: review lens ${deadLenses.join(', ')} returned no result after a retry; CI passed, but the branch keeps status merged until the lens has run.`, + reason: `${unit.repo} ${unit.branch}: CI passed, but ${reviewDead}; the branch keeps status merged until a relaunch reviews it.`, }) continue } + if (reviewHeld) continue // The final gate marks the files itself: it is already in this unit holding the verdict, where // a separate agent per branch spent its whole budget booting to edit one frontmatter line. diff --git a/plugins/fd3/workflows/repair-run.js b/plugins/fd3/workflows/repair-run.js index 2ad2720..9874cfe 100644 --- a/plugins/fd3/workflows/repair-run.js +++ b/plugins/fd3/workflows/repair-run.js @@ -1,31 +1,35 @@ export const meta = { name: 'repair-run', - description: 'Apply human HIL decisions to existing task branches, then re-validate each branch — no code review', + description: 'Apply human HIL decisions to existing task branches, then re-validate each branch and review what the repair changed', whenToUse: 'Launched by the fd3:implement-tasks skill after the user has decided the HIL items of an implement-run report; not meant to be invoked bare.', phases: [ { title: 'Recon', detail: 'only for repositories whose toolchain or baseline knowledge did not arrive in args' }, { title: 'Repair', detail: 'one agent per branch, the HIL decision applied verbatim' }, - { title: 'Validate', detail: 'scoped CI with fix rounds, then the full gate, one branch at a time' }, + { title: 'Validate', detail: 'scoped CI with fix rounds, a review of the repair delta, then the full gate, one branch at a time' }, ], } // args, provided by the fd3:implement-tasks skill (the script has no filesystem access): // repairs [{ repo, branch, worktree, base, instructions, taskFiles }] // repo and worktree are absolute paths; base is the ref the branch's diff is -// measured against (its stack base, or the repo's defaultRef); instructions carry +// measured against (its stack base, or the repo's diffBase); instructions carry // the user's HIL decisions verbatim; taskFiles are the task files to flip to done // when the branch passes, may be empty -// repos { [repository path]: { defaultRef } } +// repos { [repository path]: { startRef, diffBase } } — as implement-run takes them // reportPath (optional) absolute path of the previous run's report file; one cheap agent reads // its toolchain and baseline knowledge, sparing a re-scout and a re-baseline // toolchain (optional) { [repository path]: } — an alternative to reportPath; // repositories missing from both are re-scouted // baseline (optional) { [repository path]: { commands: [...] } } — likewise // maxFixRounds CI fix attempts per branch before giving up +// review true to review each repaired branch's delta with the code-review plugin's headless +// lenses; its findings go to the human, never to a fixer +// specPath (optional) absolute path of the spec, for the review's spec lens // args can arrive JSON-encoded depending on the caller; normalize before destructuring const input = typeof args === 'string' ? JSON.parse(args) : args -const { repairs, repos } = input +const { repairs, repos, specPath } = input +const review = input.review === true // Undefined would make every `fixRounds < maxFixRounds` false and silently skip the fix rounds // the run exists to perform, reporting failures it was built to repair. const maxFixRounds = input.maxFixRounds ?? 3 @@ -38,11 +42,20 @@ const worktreePath = (repo, name) => `${repo}.worktrees/${name.replace(/\//g, '- // Every label names the unit, never just the repository: one repository carries many branches, and // without the branch a fix loop and a fan-out look identical in the run view. const unitTag = (unit) => `${unit.repo.split('/').pop()}:${unit.branch.replace(/\//g, '-')}` -const repoDefault = (repo) => (repos && repos[repo] && repos[repo].defaultRef) || "the repository's default branch" +// Where work starts and what it is measured against part ways when the run builds on a branch the +// user parked the checkout on: worktrees start from that branch, and a diff against it is empty. +const refOf = (repo, key) => (repos && repos[repo] && repos[repo][key]) || "the repository's default branch" +const startRef = (repo) => refOf(repo, 'startRef') +const diffBase = (repo) => refOf(repo, 'diffBase') +// The harness relays the user's request to every agent, and a cheap model reads it as its own task +// and re-runs the skill that launched this workflow. +const STEP_GUARD = + 'Do only the step this prompt describes. Invoke no skill or slash command it does not name, ' + + 'whatever the relayed user request says: that request belongs to the session that launched this workflow.\n\n' // One retry rides out transient API failures (529s, brief limit blips). A second null is a real // no-verdict — absence of evidence that must never be reported as a validation verdict. const tryTwice = async (prompt, opts) => - (await agent(prompt, opts)) ?? agent(prompt, { ...opts, label: `${opts.label}:retry` }) + (await agent(STEP_GUARD + prompt, opts)) ?? agent(STEP_GUARD + prompt, { ...opts, label: `${opts.label}:retry` }) const repositories = [...new Set(repairs.map((r) => r.repo))] @@ -144,8 +157,10 @@ const baselinePrompt = (repo) => [ `Establish the validation baseline of the repository ${repo} on its clean base.`, ``, - `1. Create a worktree at ${worktreePath(repo, 'baseline')} from ${repoDefault(repo)}`, - ` (git worktree add ) unless it already exists — then reuse it as is.`, + `1. Create a worktree at ${worktreePath(repo, 'baseline')} from ${startRef(repo)}`, + ` (git worktree add --detach ) unless it already exists — then reuse it as is.`, + ` Detached, because the ref may be a branch already checked out elsewhere, which git refuses`, + ` a second worktree for.`, `2. Run every runnable validation command from the toolchain report below, in the reported`, ` order, sequentially — never in parallel. Each command's cwd in the report is relative to`, ` the repository root: resolve it inside that worktree, never against ${repo}. Skip what the`, @@ -187,7 +202,7 @@ const baselineText = (repo) => { ? `- ${c.command}: passed` : `- ${c.command}: FAILED on the clean base:\n` + (c.failures || []).map((f) => ` ${f}`).join('\n'), ) - return `Baseline on the clean base (${repoDefault(repo)}):\n${lines.join('\n')}` + return `Baseline on the clean base (${startRef(repo)}):\n${lines.join('\n')}` } // ---- Repair: the decision is the authority; agents apply it, they do not re-design @@ -201,6 +216,7 @@ const REPAIR_RESULT = { outcome: { enum: ['repaired', 'blocked'] }, summary: { type: 'string' }, reason: { type: 'string', description: 'why the decision could not be applied; only when outcome is blocked' }, + fromSha: { type: 'string', description: '`git rev-parse HEAD` in the worktree before the first edit' }, caveats: { type: 'array', items: { type: 'string' }, description: 'side effects of applying the decision literally that the human should see — a degraded type, a narrower behavior than the decision may have intended' }, }, } @@ -216,8 +232,10 @@ const repairPrompt = (r) => `your own reading of the design. Do not read the spec; do not re-derive the design; apply`, `the decision exactly as stated.`, ``, + `Before your first edit, run \`git rev-parse HEAD\` in the worktree and return it as fromSha.`, `Work only inside the worktree. Do not run linters, test suites or builds — validation runs`, - `after you. Commit with a conventional-commit message describing the repair.`, + `after you. Make one commit per decision quoted above, each with a conventional-commit message`, + `describing that decision's change — a reviewer reverts or questions one decision, never a bundle.`, ``, `Return outcome "repaired" with a two-sentence summary of what changed, or outcome "blocked"`, `with the reason when the decision cannot be applied as stated. Never guess past an ambiguity.`, @@ -241,7 +259,7 @@ repairs.forEach((r, i) => { const result = repairResults[i] if (result && result.caveats) caveats.push(...result.caveats.map((c) => `${r.branch} repair: ${c}`)) if (result && result.outcome === 'repaired') { - units.push(r) + units.push({ ...r, fromSha: result.fromSha || null }) } else { hil.push({ slug: null, @@ -279,22 +297,31 @@ const CI_RESULT = { const validationTree = (unit) => unit.worktree === unit.repo ? `${unit.repo}.worktrees/${unit.branch.replace(/\//g, '-')}-validate` : unit.worktree +const treeSetup = (unit) => { + const tree = validationTree(unit) + return tree === unit.worktree + ? [] + : [ + `That worktree is this branch's validation checkout, detached at its commit. Create it`, + `with \`git worktree add --detach ${tree} ${unit.branch}\` if it is not there; if it is,`, + `bring it to the branch's current commit with \`git -C ${tree} checkout --detach`, + `${unit.branch}\`. Never \`git clean\` it — installed dependencies live there untracked.`, + ``, + ] +} + const ciPrompt = (unit, mode, markFiles) => { const tree = validationTree(unit) return [ `Run the validation commands for the repository ${unit.repo}, branch ${unit.branch},`, `in the worktree ${tree}. Run them in the reported order, sequentially — never in`, - `parallel.`, + `parallel. Run each one as \` > ${tree}.ci.log 2>&1; echo "exit $?"\` — the log sits`, + `beside the worktree, never inside it — read pass or fail from that exit line alone, and read`, + `the log only for the lines a failure needs. Never pipe a command into \`tail\`, \`head\` or`, + `\`grep\`: the pipe's status replaces the command's, and a failing suite then reads as whatever`, + `its output happens to show.`, ``, - ...(tree === unit.worktree - ? [] - : [ - `That worktree is this branch's validation checkout, detached at its commit. Create it`, - `with \`git worktree add --detach ${tree} ${unit.branch}\` if it is not there; if it is,`, - `bring it to the branch's current commit with \`git -C ${tree} checkout --detach`, - `${unit.branch}\`. Never \`git clean\` it — installed dependencies live there untracked.`, - ``, - ]), + ...treeSetup(unit), `\`cd ${tree}\` before anything else, and confirm what you are about to grade: \`git rev-parse`, `HEAD\` there must equal \`git -C ${unit.repo} rev-parse ${unit.branch}\`. When they match,`, `return branch "${unit.branch}". When they do not, run nothing: return the branch you actually`, @@ -311,7 +338,7 @@ const ciPrompt = (unit, mode, markFiles) => { ``, mode === 'scoped' ? `Scope the run to this branch's changes: list them with` + - `\n\`git diff --name-only ${unit.base || repoDefault(unit.repo)}...HEAD\` — that ref is the` + + `\n\`git diff --name-only ${unit.base || diffBase(unit.repo)}...HEAD\` — that ref is the` + `\nbase, never diff the branch against itself — and use each command's scoped form from` + `\nthe report on those paths, quoting every path you pass to a shell (unquoted brackets` + `\nand globs break zsh); run a command in full only when the report marks it not scopeable.` @@ -366,6 +393,9 @@ const fixPrompt = (unit, problems) => `must take or approve — anything that touches production, anything irreversible, any secret`, `or credential, generating a migration, and whatever this repository's own rules reserve —`, `is a blocker, not a fix: skip it and record it under caveats. Never guess your way past it.`, + `A failure the fix can only clear by changing behaviour — an exception carved out of a rule,`, + `a narrowed decision, an error that stops meaning what it meant — is a design call, not a fix:`, + `skip it and record under caveats what the choice is.`, ``, `Toolchain report for this repository — when a fix changes something a listed command`, `derives an artifact from, regenerate that artifact the way the report says:`, @@ -373,13 +403,146 @@ const fixPrompt = (unit, problems) => toolchain.get(unit.repo), ``, `Commit the fixes with a conventional-commit message. To verify a fix you may re-run the`, - `exact commands that failed — never the full validation suite; it is rerun after you.`, + `exact commands that failed, scoped to the files they failed on — never a whole suite; the`, + `full validation runs after you.`, ``, `Return a two-sentence summary. Under caveats, return every problem you skipped with the`, `reason, any judgment call that went beyond the listed problems, and any change that touches`, `a decision recorded in the spec.`, ].join('\n') +const PREP_RESULT = { + type: 'object', + required: ['status', 'invoked'], + properties: { + invoked: { type: 'boolean', description: 'the code-review:cr-prepare skill was invoked through the Skill tool' }, + status: { enum: ['ready', 'empty', 'error'] }, + files: { type: 'number', description: 'judged files' }, + active: { type: 'array', items: { type: 'string' }, description: 'active lenses, by name' }, + inactive: { type: 'array', items: { type: 'string' }, description: 'inactive lenses, each with its reason' }, + alternate: { type: 'string', description: 'the alternate base and its file count; only when status is empty and the skill named one' }, + reason: { type: 'string', description: 'only when status is error' }, + }, +} + +const SCAN_RESULT = { + type: 'object', + required: ['status', 'invoked', 'filesJudged', 'filesTotal'], + properties: { + invoked: { type: 'boolean', description: 'the code-review:cr-scan skill was invoked through the Skill tool' }, + status: { enum: ['scanned', 'inactive', 'error'] }, + filesJudged: { type: 'number' }, + filesTotal: { type: 'number' }, + }, +} + +const CR_MERGE_RESULT = { + type: 'object', + required: ['status', 'invoked', 'findings'], + properties: { + invoked: { type: 'boolean', description: 'the code-review:cr-merge skill was invoked through the Skill tool' }, + status: { enum: ['merged', 'incomplete', 'error'] }, + report: { type: 'string', description: 'absolute path of report.md' }, + missing: { type: 'array', items: { type: 'string' }, description: 'lenses that did not report; only when status is incomplete' }, + findings: { + type: 'array', + items: { + type: 'object', + required: ['severity', 'family', 'rule', 'location', 'risk', 'fix'], + properties: { + severity: { type: 'string', description: 'high | medium | nit; `comment` for a comment verdict' }, + family: { type: 'string' }, + rule: { type: 'string', description: 'the rule, or for a comment verdict R and its verdict' }, + location: { type: 'string', description: ':L' }, + risk: { enum: ['safe', 'structural', 'report-only'] }, + fix: { type: 'string' }, + reserved: { type: 'boolean', description: 'a direct consequence of the open work the prompt lists' }, + boyScout: { type: 'boolean', description: 'the finding line carries the `boy-scout` token: it is about code the change did not touch' }, + }, + }, + }, + }, +} + +// The lens skills read a context directory, never this prompt, so the run's own facts — which tree, +// which base, what is deliberately unfinished — reach them only through the wrapper agents. +const reviewDir = (unit, pass) => `${unit.repo}.worktrees/.review/${unit.branch.replace(/\//g, '-')}-${pass}` + +const prepPrompt = (unit, base, dir) => { + const tree = validationTree(unit) + return [ + `Prepare a code review of the branch ${unit.branch} (repository ${unit.repo}) in the checkout`, + `${tree}, measured from ${base}.`, + ``, + ...treeSetup(unit), + `\`git -C ${tree} rev-parse HEAD\` must equal \`git -C ${unit.repo} rev-parse ${unit.branch}\`;`, + `when it does not, invoke nothing and return status "error" with the mismatch as reason.`, + ``, + `Remove ${dir} if it exists — lens files left there by an earlier run would be read as this`, + `run's. Then invoke the \`code-review:cr-prepare\` skill through the Skill tool with:`, + `\`--base ${base} --out ${dir} -C ${tree}${specPath ? ` --spec ${specPath}` : ''}\``, + ``, + `Return what its closing block says: status, judged file count, active and inactive lenses,`, + `the alternate on empty, the reason on error — and invoked=true. When the Skill tool is not`, + `available to you, return invoked=false and status "error"; never do the skill's work by hand.`, + ].join('\n') +} + +const scanPrompt = (lens, dir) => + [ + `Invoke the \`code-review:cr-scan\` skill through the Skill tool with`, + `\`--lens ${lens} --context ${dir}\`, and return what its closing block says — status, files`, + `judged of the total — with invoked=true. When the Skill tool is not available to you, return`, + `invoked=false, status "error" and zero counts; never judge the change by hand.`, + ].join('\n') + +const crMergePrompt = (dir) => + [ + `Invoke the \`code-review:cr-merge\` skill through the Skill tool with \`--context ${dir}\`,`, + `and return what its closing block says: status, the report path, the lenses that did not`, + `report on incomplete, and every finding line as one entry — severity, family, rule, location,`, + `risk class and the fix. A comment verdict's severity is \`comment\`; a line carrying the`, + `\`boy-scout\` token sets boyScout=true. Return invoked=true; when`, + `the Skill tool is not available to you, return invoked=false, status "error" and no findings.`, + ].join('\n') + +// A review that did not run is absence of evidence: an empty change, a lens that judged nothing or a +// skill the agent never invoked all come back as a reason, never as an empty findings list. +const runReview = async (unit, base, pass, tag) => { + const dir = reviewDir(unit, pass) + const prep = await tryTwice(prepPrompt(unit, base, dir), { label: `cr-prep:${tag}:${pass}`, phase: 'Validate', schema: PREP_RESULT }) + if (!prep || !prep.invoked || prep.status === 'error') { + return { dir, dead: `cr-prepare ${prep ? `failed: ${prep.reason || 'the skill was not invoked'}` : 'returned no result after a retry'}` } + } + if (prep.status === 'empty') { + return { dir, empty: true, dead: `the change against ${base} is empty${prep.alternate ? ` (the skill names ${prep.alternate})` : ''}` } + } + const lenses = prep.active || [] + const scans = await parallel( + lenses.map((lens) => () => + tryTwice(scanPrompt(lens, dir), { label: `cr:${tag}:${pass}:${lens}`, phase: 'Validate', schema: SCAN_RESULT }), + ), + ) + const deadLenses = lenses.filter((_, i) => { + const r = scans[i] + return !r || !r.invoked || r.status === 'error' || (r.status === 'scanned' && r.filesTotal > 0 && r.filesJudged === 0) + }) + if (deadLenses.length > 0) return { dir, dead: `lens ${deadLenses.join(', ')} did not report` } + const merged = await tryTwice(crMergePrompt(dir), { label: `cr-merge:${tag}:${pass}`, phase: 'Validate', schema: CR_MERGE_RESULT }) + if (!merged || !merged.invoked || merged.status !== 'merged') { + const why = !merged + ? 'returned no result after a retry' + : merged.status === 'incomplete' + ? `found lens ${(merged.missing || []).join(', ')} missing` + : 'failed' + return { dir, dead: `cr-merge ${why}` } + } + return { dir, report: merged.report, files: prep.files || 0, lenses: lenses.length, findings: merged.findings } +} + +const serious = (f) => f.severity === 'high' || f.severity === 'medium' +const findingLine = (f) => `${f.location} — ${f.family} · ${f.rule} (${f.severity}): ${f.fix}` + const mechanical = { model: 'haiku', effort: 'high' } // CI runners interpret command output; they design nothing // A CI verdict is a statement about one tree at one commit. A runner that stayed in the @@ -458,8 +621,32 @@ for (const unit of units) { continue } + // A repair is new code written to a human's one-line decision; CI proves it builds, never that it + // did only what was decided — the review reads the repair's own commits, and a human reads that. + let reviewDead = null + let reviewHeld = false + if (review) { + if (!unit.fromSha) { + reviewDead = 'the repair agent did not report its starting commit, so the repair delta is unknown' + } else { + const delta = await runReview(unit, unit.fromSha, 'repair', tag) + summary.review = { context: delta.dir, report: delta.report || null } + if (delta.dead && !delta.empty) { + reviewDead = `the review of the repair did not run: ${delta.dead}` + } else if (!delta.empty) { + const open = delta.findings.filter(serious) + summary.review.findings = delta.findings.map(findingLine) + for (const f of open) { + hil.push({ slug: null, kind: 'review', reason: `${unit.repo} ${unit.branch}: ${findingLine(f)} — the task files keep their status until a repair settles it` }) + } + reviewHeld = open.length > 0 + } + } + } + // The full command list is the branch's final gate — repairs go out only fully validated. - const { ci: finalCi, fault: finalFault } = await runCi(unit, 'full', !!(unit.taskFiles && unit.taskFiles.length > 0), `ci:${tag}:final`) + const markFiles = !!(unit.taskFiles && unit.taskFiles.length > 0) && !reviewDead && !reviewHeld + const { ci: finalCi, fault: finalFault } = await runCi(unit, 'full', markFiles, `ci:${tag}:final`) if (!finalCi || finalFault) { summary.ci = 'no-verdict' hil.push({ @@ -486,7 +673,16 @@ for (const unit of units) { // The final gate marks the files itself: it is already in this unit holding the verdict, where // a separate agent per branch spent its whole budget booting to edit one frontmatter line. - if (unit.taskFiles && unit.taskFiles.length > 0 && !finalCi.marked) { + if (reviewDead) { + hil.push({ + slug: null, + kind: 'no-verdict', + stage: 'review', + reason: `${unit.repo} ${unit.branch}: CI passed, but ${reviewDead}; the task files keep their status until a relaunch reviews it.`, + }) + continue + } + if (markFiles && !finalCi.marked) { // The files are the state store; in-memory state must never outrun them. hil.push({ slug: null,