diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index bb7b234..1eb004c 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -117,7 +117,7 @@ { "name": "code-review", "source": "./plugins/code-review", - "description": "Unified code review that fans out parallel scanners over a change — one per active lens, six to eight per run — and merges them into one per-file report. Eight lenses: comment quality, four quality/craft lenses (readability & tests, naming & module, objects & patterns, simplicity & types), an always-on narrow security lens (secrets, injection, access checks, boundary validation, insecure settings), a performance lens for executable source (N+1, unbounded fetch, blocking-in-async, wasted renders), and a spec lens via --spec . Explicit rules in a root CODING_STANDARDS.md (plus a gitignored .local.md overlay) generate standards findings. The standalone comment-review and quality-review skills stay invocable for a single-lens pass. Reviews the current branch diff by default (or explicit paths / --base). Quality findings are tagged family · rule · severity; comment verdicts are R1–R12 · KEEP/REMOVE/REWRITE/MOVE/ADD, shown side by side and applied through a single risk-cut menu. Successor to the comment-review and quality-review plugins.", + "description": "Unified code review that fans out parallel scanners over a change — one per active lens, six to eight per run — and merges them into one per-file report. Eight lenses: comment quality, four quality/craft lenses (readability & tests, naming & module, objects & patterns, simplicity & types), an always-on narrow security lens (secrets, injection, access checks, boundary validation, insecure settings, IaC exposure, access widening), a performance lens for executable source (N+1, unbounded fetch, blocking-in-async, wasted renders), and a spec lens via --spec or a spec file the diff carries. Explicit rules in a root CODING_STANDARDS.md (plus a gitignored .local.md overlay) generate standards findings. The standalone comment-review and quality-review skills stay invocable for a single-lens pass. Reviews the current branch diff by default (or explicit paths / --base). Quality findings are tagged family · rule · severity; comment verdicts are R1–R12 · KEEP/REMOVE/REWRITE/MOVE/ADD, shown side by side and applied through a single risk-cut menu. Successor to the comment-review and quality-review plugins.", "version": "0.3.0", "author": { "name": "Mateusz Gostański", diff --git a/.github/workflows/code-review-scripts.yml b/.github/workflows/code-review-scripts.yml new file mode 100644 index 0000000..264dd03 --- /dev/null +++ b/.github/workflows/code-review-scripts.yml @@ -0,0 +1,21 @@ +name: code-review scripts + +on: + pull_request: + paths: + - 'plugins/code-review/scripts/**' + - '.github/workflows/code-review-scripts.yml' + +jobs: + pytest: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: '3.12' + - name: Install pytest + run: python -m pip install pytest + # The suite builds real git repositories and sets their identity itself, so no git config is needed here. + - name: Run tests + run: python -m pytest plugins/code-review/scripts/tests diff --git a/.github/workflows/fd3-evals.yml b/.github/workflows/fd3-evals.yml index a6627a7..bbcc46d 100644 --- a/.github/workflows/fd3-evals.yml +++ b/.github/workflows/fd3-evals.yml @@ -17,7 +17,8 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 60 env: - ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} + # Runs on a subscription OAuth token (`claude setup-token`), not a metered API key. + CLAUDE_CODE_OAUTH_TOKEN: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} steps: - uses: actions/checkout@v4 - uses: pnpm/action-setup@v4 diff --git a/.gitignore b/.gitignore index 61e22c1..f3a628a 100644 --- a/.gitignore +++ b/.gitignore @@ -15,3 +15,7 @@ plugins/*/evals/.results/ # the target app's own runtime store, written on every boot plugins/tester/evals/fixtures/target-app/state.json + +# Python bytecode +__pycache__/ +*.pyc diff --git a/plugins/code-review/.claude-plugin/plugin.json b/plugins/code-review/.claude-plugin/plugin.json index bb33425..eb94ba8 100644 --- a/plugins/code-review/.claude-plugin/plugin.json +++ b/plugins/code-review/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "code-review", "version": "0.3.0", - "description": "Unified code review that fans out parallel scanners over a change — one per active lens, six to eight per run — and merges them into one per-file report. Eight lenses: comment quality, four quality/craft lenses (readability & tests, naming & module, objects & patterns, simplicity & types), an always-on narrow security lens (secrets, injection, access checks, boundary validation, insecure settings), a performance lens for executable source (N+1, unbounded fetch, blocking-in-async, wasted renders), and a spec lens via --spec . Explicit rules in a root CODING_STANDARDS.md (plus a gitignored .local.md overlay) generate standards findings. The standalone comment-review and quality-review skills stay invocable for a single-lens pass. Reviews the current branch diff by default (or explicit paths / --base). Quality findings are tagged family · rule · severity; comment verdicts are R1–R12 · KEEP/REMOVE/REWRITE/MOVE/ADD, shown side by side and applied through a single risk-cut menu. Successor to the comment-review and quality-review plugins.", + "description": "Unified code review that fans out parallel scanners over a change — one per active lens, six to eight per run — and merges them into one per-file report. Eight lenses: comment quality, four quality/craft lenses (readability & tests, naming & module, objects & patterns, simplicity & types), an always-on narrow security lens (secrets, injection, access checks, boundary validation, insecure settings, IaC exposure, access widening), a performance lens for executable source (N+1, unbounded fetch, blocking-in-async, wasted renders), and a spec lens via --spec or a spec file the diff carries. Explicit rules in a root CODING_STANDARDS.md (plus a gitignored .local.md overlay) generate standards findings. The standalone comment-review and quality-review skills stay invocable for a single-lens pass. Reviews the current branch diff by default (or explicit paths / --base). Quality findings are tagged family · rule · severity; comment verdicts are R1–R12 · KEEP/REMOVE/REWRITE/MOVE/ADD, shown side by side and applied through a single risk-cut menu. Successor to the comment-review and quality-review plugins.", "author": { "name": "Mateusz Gostański", "email": "mg@grixu.dev" diff --git a/plugins/code-review/CHANGELOG.md b/plugins/code-review/CHANGELOG.md index dfb29b0..f486851 100644 --- a/plugins/code-review/CHANGELOG.md +++ b/plugins/code-review/CHANGELOG.md @@ -10,6 +10,76 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Author metadata now reads `Mateusz Gostański ` in `plugin.json` and the marketplace entry. +- Step 3 spells out that waiting for the Scanners means ending the turn: no `sleep`, `ListAgents` + polling, transcript `stat`s, placeholder calls or `Monitor`/`until` loops, and never `TaskStop` + on a Scanner — elapsed time is not a state the Orchestrator can observe +- The conventions note is one byte-identical, suppress-only text in every brief, with each rule + quoted verbatim from its file: no per-Lens threat hypotheses, no "do not raise" lists, no + paraphrases +- A Scanner dispatches no agent of its own, waits in no background, and its final message is its + whole output; a `` presented as an amendment or a partial list counts as truncated and + the Lens is re-dispatched +- The unavailable-lens question offers exactly proceed-without-it or abort — reviewing that lens + inline is not an option on it +- Step 3 states what to do where the `Agent` tool is absent (inside another agent or a workflow + step): say so up front and hand the caller the choice, never discover it halfway and report a + single pass as an eight-lens review +- Scanner search is described tool-neutrally (`Grep`, or `git grep` where sub-agents have no + `Grep` tool) +- Step 4 now judges the fix as well as the finding — behaviour preserved, no contradiction with + another fix, no new smell — and re-routes a fix that fails any of the three to the structural + walk (with the behaviour change named) or to report-only +- Boy-scout extras are sorted by risk: a structural or `security` boy-scout fix walks one at a + time instead of riding the batch; a `Not flagged` item reaches the menu only as its own named + option; and the slot order puts `security`, then verified rule-less correctness problems, ahead + of boy-scout extras +- The `Reconciliation` line gained a `P primary dropped` term and each of its counts now names + the rendered block it is checked against (`C` against `Boy-scout`, `D + P` against `Not + flagged`); `Not flagged` entries stay countable so the check can be verified from the report +- Apply-phase discipline: edits go through the `Edit` tool (no `sed`/heredoc rewrites), a + formatter runs only on the files the review edited, an approved fix that cannot be applied as + approved goes back to the user instead of being substituted, and the wrap-up lists every fix + skipped, substituted or extended +- The rendered report and the user's selection are written to the session scratchpad before the + apply walk, so a compaction mid-walk no longer costs the approved list +- A confirmed exposure that no rule names still leads the report from its own `Not flagged` + bullet + +- `get_changes.py` reports an `alternate` base when the resolved one saw no committed + change and another (`origin/main`, …) holds commits — a branch pushed to its own remote + counterpart no longer reads as "nothing to review"; `start-cr`, `comment-review` and + `quality-review` offer the re-run instead of stopping +- A `security` scanner's `CANDIDATES` block is a heading with bullets, never a fenced code block +- Scope: `.mjs`/`.cjs`/`.mts`/`.cts` are reviewed as source, a directory whose name + contains `e2e` (or ends in `-tests`) classifies as `test`, `.txt` is skipped, and a CI + workflow file is skipped with a sentence naming its triggers, permissions and secret + handling as `/security-review` territory +- The "substance of the change" sentence now covers a docs, spec/ADR or release-notes + branch, not only a dependency manifest +- When no `--spec` was passed and the diff carries a spec-shaped file (`specs/`, + `docs/adr/`, `tasks/`, `*SPEC*.md`, …), `start-cr` offers to review the change against + it instead of silently leaving the `spec` lens off +- `comments` · R4 resolves a spec-id-shaped token against the code before stripping it — a + token bound to an identifier or a string literal is a code value, not a document pointer, + and the verdict line says which of the two it was +- `Not flagged` is required whenever a look-alike was cleared, and a clean file is the case + that needs it most: without the line a reader cannot tell a review that cleared candidates + from one that never looked + +### Added + +- `security` · **`iac-exposure`** (high) — infrastructure code that materializes a secret into + state or an unmarked output, or grants trust wider than the identity it names (an OIDC + condition matching beyond the intended workflow, a wildcard principal, anonymous access) +- `security` · **`access-widening`** (high) — a change that relaxes an authorization boundary + that existed: a weaker permission, a removed guard, a dropped owner predicate, a widened + allowlist +- `missing-access-check` calibration now routes a test that would stay green if the guard + regressed to `tests` · test-fidelity, instead of grading a test gap as a security high +- Evals — a unified-diff scanner track plus four scenarios covering the defects this round + fixed: `iac-exposure` recall, `access-widening` on a diff, a guard whose weak tests must route + to `tests` · test-fidelity rather than a security high, and scope classification over a mixed + tree (`.mjs`/`.cjs`, an `e2e` directory, `.txt`, a CI workflow) ## [0.3.0] - 2026-09-02 diff --git a/plugins/code-review/CONTEXT.md b/plugins/code-review/CONTEXT.md index 128232d..df1fd2c 100644 --- a/plugins/code-review/CONTEXT.md +++ b/plugins/code-review/CONTEXT.md @@ -15,7 +15,7 @@ One parallel review subagent running exactly one Lens. _Avoid_: role, reviewer, worker **Lens**: -One of the eight rule clusters: comments (`R1`–`R12`), readability & tests, naming & module, objects & patterns, simplicity & types, security, performance, spec. Three sit beyond the craft five: security (always on), and two gated ones — performance (executable source files in scope), spec (`--spec ` given). Comments is a Lens like any other, not a special case; the three added Lenses have no standalone skill. +One of the eight rule clusters: comments (`R1`–`R12`), readability & tests, naming & module, objects & patterns, simplicity & types, security, performance, spec. Three sit beyond the craft five: security (always on), and two gated ones — performance (executable source files in scope), spec (a spec file named — `--spec `, or the one the diff carries, accepted by the user). Comments is a Lens like any other, not a special case; the three added Lenses have no standalone skill. _Avoid_: theme, dimension **Rules file**: @@ -25,7 +25,7 @@ The single source of truth for one Lens's rule text: `references/rules/.md One of the eleven stable top-level labels in the quality vocabulary: `readability`, `tests`, `naming`, `module`, `objects`, `patterns`, `simplicity`, `security`, `performance`, `spec`, and `standards`. Ten are fixed by the plugin; `standards` is repo-defined (its rules come from the Standards file). **Rule**: -A specific sub-tag under a Family — one of the 42 fixed rules across the ten plugin-defined Families, or a repo-defined `standards` rule whose slug derives from the quoted rule — or one of the comment rules `R1`–`R12`. +A specific sub-tag under a Family — one of the 44 fixed rules across the ten plugin-defined Families, or a repo-defined `standards` rule whose slug derives from the quoted rule — or one of the comment rules `R1`–`R12`. **Finding**: The quality-side unit of output: `family` · rule · severity · lines → fix. @@ -42,7 +42,7 @@ The root pair `CODING_STANDARDS.md` + `CODING_STANDARDS.local.md`, read together _Avoid_: style guide, conventions file (a conventions file — `CLAUDE.md`, `AGENTS.md`, `CONTRIBUTING.md`, `.claude/rules`, `.cursor/rules` — only suppresses) **Active lens set**: -The N Lenses (6 to 8) that Step 2b of **start-cr** resolves for one run: the five craft Lenses and security always, performance when the source-kind subset of the resolved files (minus `.sh`) is non-empty, spec when `--spec` was given. The report's `Lenses: L of 8` line records it, naming every inactive Lens with its reason. +The N Lenses (6 to 8) that Step 2b of **start-cr** resolves for one run: the five craft Lenses and security always, performance when the source-kind subset of the resolved files (minus `.sh`) is non-empty, spec when a spec file was named. The report's `Lenses: L of 8` line records it, naming every inactive Lens with its reason. _Avoid_: lens selection (the user never picks), enabled lenses ## Relationships @@ -60,7 +60,7 @@ _Avoid_: lens selection (the user never picks), enabled lenses ## Example dialogue > **Dev:** "Can I run just the comment **Scanner**?" -> **Domain expert:** "Invoke the `comment-review` skill directly — **start-cr** always runs its whole **Active lens set**; a **Scanner** is its internal unit of fan-out, not a user-facing switch. The set is decided by the change and the `--spec` flag, never by picking lenses." +> **Domain expert:** "Invoke the `comment-review` skill directly — **start-cr** always runs its whole **Active lens set**; a **Scanner** is its internal unit of fan-out, not a user-facing switch. The set is decided by the change and by whether a spec file is named, never by picking lenses." ## Flagged ambiguities diff --git a/plugins/code-review/README.md b/plugins/code-review/README.md index 49258b6..f942264 100644 --- a/plugins/code-review/README.md +++ b/plugins/code-review/README.md @@ -28,7 +28,8 @@ From the `grixu/cc-toolkit` marketplace: `/start-cr` has no lens switch; the change decides which lenses run. The five craft lenses and `security` run every time; `performance` runs when the change -touches executable source; `spec` runs only when you pass `--spec `. The +touches executable source; `spec` runs when a spec file is named — by `--spec `, +or by accepting the one the review offers when the diff itself carries a spec. The report's `Lenses: L of 8` line names every lens that sat out and why. For a partial review, invoke `/comment-review` or `/quality-review` directly; both stay independently available and share the same rule text as the command. The three @@ -79,14 +80,17 @@ Three more sit beyond the craft five — one always on, two gated — and the re says which ran: - **security** — always on. Secrets in source, injection sinks, missing access - checks, unvalidated boundaries, and insecure settings in source files. A - finding names both the source and the sink; a pattern alone is never a finding. + checks, unvalidated boundaries, insecure settings, infrastructure code that + exposes a secret or trusts too widely, and a change that relaxes an existing + authorization boundary. A finding names both the source and the sink; a pattern + alone is never a finding. - **performance** — only when the change touches executable source (not tests, not infrastructure-as-code, not `.sh`). N+1 calls, unbounded fetches, blocking calls on an async path, wasted React renders. Every finding names the multiplier, the call inside it, the missing bound, and the batch/limit API that exists; "could be slow" is not a finding. -- **spec** — only with `--spec ` (a local file). A spec line nothing +- **spec** — only with a named spec file (`--spec `, or the one the review + offers from the diff). A spec line nothing implements, one implemented against its wording, one only partly met, and scope creep the spec never asked for. Every finding quotes the spec line. @@ -123,7 +127,7 @@ Tests, infrastructure-as-code, and `.sh` files are reviewed by the craft lenses **This is a craft review plus a narrow security lens, not a security audit.** The security lens looks for secrets, injection, access checks, boundary validation, -and insecure settings in source files; the performance lens raises diff-level +insecure settings, infrastructure exposure, and widened access in source files; the performance lens raises diff-level hypotheses it can point at a line. Neither is a dependency, config, or data-flow audit: `.env` files, manifests, and lockfiles are not scanned, and a vulnerability outside those shapes will surface only by accident. Do not read a diff --git a/plugins/code-review/commands/start-cr.md b/plugins/code-review/commands/start-cr.md index e4bb9be..e23d764 100644 --- a/plugins/code-review/commands/start-cr.md +++ b/plugins/code-review/commands/start-cr.md @@ -5,7 +5,8 @@ description: >- security, performance, spec) in parallel over a change and merges them into one per-file report. Three lenses are gated by the input, never by the user: security is always on, performance runs only when executable source files are in scope, - spec only with `--spec `. Manual only — never auto-triggered. It resolves + spec only when a spec file is named — by `--spec `, or by the user accepting + the one the diff itself carries. Manual only — never auto-triggered. It resolves scope once, dispatches one scanner subagent per active lens, re-grades severity centrally, and offers a single apply menu. It never edits code during the review. allowed-tools: Read, Bash, Grep, Glob, Agent, AskUserQuestion, Edit, Write @@ -23,7 +24,7 @@ and only for what the user picks. This command is **explicit invocation only**; it is never auto-triggered. There is no lens selection — which Lenses run is decided by the input in Step 2b, never by user choice: the five craft Lenses and `security` always run, `performance` runs -when executable source is in scope, `spec` when `--spec` names a file. For a partial +when executable source is in scope, `spec` when a spec file is named. For a partial review the user invokes `/comment-review` or `/quality-review` directly. Arguments: `$ARGUMENTS` @@ -59,7 +60,13 @@ Scanner's `` is cut from this one list in Step 2b, and all of them get th (append `--base ` to both when the user passed one.) Read the `count` of each: - - both zero → tell the user there is nothing to review and **stop**; + - both zero → before concluding, look for an `alternate` object in the `committed` + output: the script adds it when the resolved base saw nothing but another base + (usually `origin/main`) holds real commits, which is what a freshly pushed branch + tracking its own remote counterpart looks like. When it is there, say which base was + used, which one differs and by how many files, and offer to re-run with + `--base ` — do not report "nothing to review" over it. With no + `alternate`, tell the user there is nothing to review and **stop**; - exactly one non-zero → use that scope automatically; - both non-zero → ask with **one** `AskUserQuestion` which to review — **Uncommitted** (working tree vs HEAD), **Committed** (HEAD vs base), or @@ -82,6 +89,17 @@ Scanner's `` is cut from this one list in Step 2b, and all of them get th offer to review uncommitted changes only or to pass `--base ` — **never guess silently**. +**When the change carries its own spec, offer the Lens.** If `--spec` was not passed +and the resolved list contains a specification-shaped file — a path under `specs/`, +`spec/`, `docs/adr/`, `tasks/`, or a name matching `*SPEC*.md`, `*ADR*.md`, `*.spec.md`, +`*-plan.md` — say so in one line and offer that path with a single `AskUserQuestion`: +review the change against it, or continue without the `spec` Lens. Offer the one file +that best fits (the most recently changed, or the one the other files sit under); more +than two options is a menu, not an offer. On acceptance, treat it exactly as a passed +`--spec` — Read it now — and note in the Tally that the spec Lens was activated from the +diff rather than from the flag. Do not make this offer twice, and never activate the +Lens without the user saying yes. + **Which files get judged** — the in-scope extensions, the skip list, and the rule about a skipped dependency manifest that is the substance of the change — is in `${CLAUDE_PLUGIN_ROOT}/references/scope.md`. Read it and apply it to the resolved @@ -104,6 +122,15 @@ conflict the `.local` one wins. Work it there, then: 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 @@ -130,13 +157,13 @@ never from a preference: - **`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 `--spec` was given and resolved to a readable local file in - Step 1. +- **`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`); the Tally prints them in Step 5. From here on +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. @@ -149,6 +176,13 @@ return inline at once, so the harness backgrounds them — this holds **even if `run_in_background: false`**, because the flag cannot make a concurrent fan-out 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. + **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 that has failed outright in practice, leaving an orchestrator with every Scanner signalling @@ -167,14 +201,28 @@ its findings verbatim inside `` — that is the delivery, and it arrives control straight back, so "ask and block on the reply" is not a thing the tool can do. Chasing a Scanner that is merely slow makes it regenerate its whole output, which can land after you have already merged. + + **Waiting is ending your turn.** Once the pre-reading below is done, say "standing by" and + end the turn: each `` wakes you, and a turn you never end is the only way + to *not* receive them promptly. Never `sleep`, never poll `ListAgents`, never `stat` a + Scanner's transcript, never emit a placeholder tool call to stay alive, and never set up a + `Monitor` or an `until` loop over any of these — a run that polled its way through the wait + burned 70% of its turns and two thirds of its context on `echo ok`, and the leftover timers + then fired into the report and the apply phase. And **never `TaskStop` a Scanner**: elapsed + time is not a state you can observe, the "stalled" one was mid-`Read` with 27 tool calls + behind it, and killing it cost the review its whole security lens. 2. **Fail closed on an empty ``, not on silence.** The failure to catch is a notification whose `` is missing, empty, or truncated mid-block — that Scanner - has **not** reported. Re-dispatch that one Lens as a fresh **unnamed** `Agent` and + has **not** reported. A `` that presents itself as an **amendment, a correction, or + a partial list** counts as truncated too, whatever it contains: the Scanner's own full + findings are somewhere you cannot see, so re-dispatch that Lens rather than merge the + fragment. Re-dispatch that one Lens as a fresh **unnamed** `Agent` and collect its `` the same way — this holds for every active Lens, `security`, `performance` and `spec` included. Never quietly review that lens yourself and pass the result off as a full N-lens review. If the re-dispatch also comes back empty, **tell the user that lens is unavailable** and ask whether to proceed without it or - abort. A single-pass or missing-lens review is a **labelled, user-acknowledged + abort — those two are the whole menu, and "I read that lens inline myself" is not on it, + however reasonable it looks as the recommended option. A single-pass or missing-lens review is a **labelled, user-acknowledged degradation**, never the silent default — that silent fallback is exactly how a single perspective's false positive reaches the report unchecked. 3. **Merge only once all N have delivered a ``.** Merging early loses findings. @@ -220,12 +268,21 @@ Send each Scanner a brief in this shape, filling every slot: 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. It is reading the +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: Grep the importers of each changed module and the -imports of each module it newly imports, open those files at the matched lines only — +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. @@ -264,8 +321,8 @@ it still writes nothing. 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 has `Read` and `Grep` — and marks - only what it still cannot confirm `(verify)`. `CANDIDATES` is reserved for a + 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 @@ -397,12 +454,28 @@ One terse line each. Omit a block when it is empty. 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` — - where `A + B + C + D` equals `N + M`. The arithmetic is what makes the check real: a + 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 @@ -413,12 +486,28 @@ One terse line each. Omit a block when it is empty. 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 42 rows, what each severity means, the anti-anchoring rule, and the + 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. @@ -430,7 +519,7 @@ 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 +Reconciliation: handoffs + candidates → merged · own bullet · boy-scout · Not flagged;

primary dropped ## Code review — @@ -456,7 +545,7 @@ each when one is a real problem with no rule to land on; omit when empty> A filled-in report reads like this: -Reconciliation: 4 handoffs + 2 candidates → 3 merged · 1 own bullet · 0 boy-scout · 2 Not flagged +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 @@ -520,10 +609,11 @@ Rules for filling it in: 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 - and `HANDOFF` 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. A real problem keeps its own bullet rather than being compressed into a +- **`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 @@ -534,13 +624,15 @@ Rules for filling it in: `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; a `spec` · + 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`); drop the parenthesis when all eight ran. When a spec was + 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. @@ -583,7 +675,14 @@ menu; never add a fifth. `Report only` is always offered: `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. +- **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 @@ -621,16 +720,46 @@ must stay honest when findings don't spread across them: `Not flagged` or spans untouched code, yet the review actually verified — is offer-able as its own apply bucket; so is a verified `spec` · wrong-implementation with a one-edit fix. The review's most valuable output belongs in the menu, not buried in `Report - only` or `Boy-scout extras` because it lacks a rule tag. + only` or `Boy-scout extras` because it lacks a rule tag. It is the **only** way a `Not + flagged` item enters the menu: it gets its own option, named for the problem, never folded + into `Safe fixes` or `Boy-scout extras` where the user approves it without seeing it. +- **When there are more candidates than slots**, the order is: a confirmed `security` problem + first, then a verified correctness problem with no rule, then the canonical buckets by risk, + and `Boy-scout extras` last — it is the one whose loss costs the change nothing. A run that + gave its last slot to a boy-scout nit while a verified backend gap waited had the priority + backwards. - A before/after **preview** diff belongs in an `AskUserQuestion` option, never in the report body — Step 5 stays clause-only. +**Put the review on disk before the apply phase starts.** The apply walk is the longest stretch +of the run and the one most likely to be compacted; when that happens mid-walk, the report and +the user's answer are gone, and a run that had to reconstruct its approved list by parsing its +own transcript spent that effort for nothing. Write the rendered report to a file in the session +scratchpad before the menu, and the user's selection — each approved finding with its file, site +and exact fix — under it as soon as the answer arrives. Read it back rather than recalling it, +and say where it is in the wrap-up. + Apply with `Edit` only what the user selects; **auto-apply nothing structural without an explicit yes**. Only findings confirmed in Step 4 enter an apply batch. -**`Write` creates a file that does not exist yet, and nothing else.** The one case is -a new file the user picked from the menu — the missing spec a correctness bucket -offered, say. Every change to a file already on disk goes through `Edit`, so a +**`Edit` means the tool, not "an edit".** No `sed -i`, no Python or heredoc rewrite, no `awk`, +however convenient the shell looks for a repeated change: `Edit` fails loudly when the text it +expects is not there, and a shell rewrite silently hits every look-alike in the file — one run's +blanket strip took out the project's own documented comment prefix, which its conventions note +had just said to leave alone. A formatter runs on the files you edited, never across the package +or the repository: three runs reflowed snapshots, fixtures and a protected `tsconfig` that way, +then had to revert them and explain them to the user as "not mine". + +**An approved fix that cannot be applied as approved goes back to the user.** A hook blocks it, +the site turns out ambiguous, the edit needs a companion change nobody approved — say which fix, +what stopped it, and what you would do instead; never substitute a different edit (one run +deleted a test where the approved fix was to fold it into another) and mention it in passing +afterwards. The wrap-up lists every approved fix that was skipped, substituted or extended, with +its reason, and claims nothing the tree does not carry. + +**`Write` creates a file that does not exist yet, and nothing else.** Two cases: a new file +the user picked from the menu — the missing spec a correctness bucket offered, say — and the +review's own scratchpad file above, which lives outside the repository. Every change to a file already on disk goes through `Edit`, so a targeted fix can never turn into a wholesale rewrite of a file the review only read in part. This is the Orchestrator's alone: a Scanner still writes nothing at all. diff --git a/plugins/code-review/docs/adr/0002-active-lens-set-and-standards.md b/plugins/code-review/docs/adr/0002-active-lens-set-and-standards.md index 73faaea..6ad8a1e 100644 --- a/plugins/code-review/docs/adr/0002-active-lens-set-and-standards.md +++ b/plugins/code-review/docs/adr/0002-active-lens-set-and-standards.md @@ -8,12 +8,14 @@ dispatch: the five craft lenses (comments, readability & tests, naming & module, objects & patterns, simplicity & types) plus `security` are always active; `performance` is active only when the resolved files contain executable source (the `source` file kind — not tests, not infrastructure-as-code, not `.sh`); -`spec` is active only when the user passed `--spec `. The -orchestrator dispatches N scanners (6 to 8), waits for N `` blocks, +`spec` is active only when a spec resolves to a local file — from `--spec `, or from the spec-shaped file in the diff that `start-cr` offers and the +user accepts. The orchestrator dispatches N scanners (6 to 8), waits for N +`` blocks, applies fail-closed re-dispatch to every active lens, merges once all N have delivered, and records `Lenses: L of 8` in the tally with every inactive lens -and its reason. The user still picks no lens: the change and the `--spec` flag -decide. +and its reason. The user still picks no lens: the change, the `--spec` flag, +and the answer to that offer decide. Three lenses get their own rules files (`security.md`, `performance.md`, `spec.md`), read by `start-cr` only. Ten further rules from the same proposal — @@ -67,8 +69,10 @@ keeps its suppress-only role. ## Consequences - "Five" is no longer an invariant anywhere: every count in the command, the - skills, the references, and the docs is N, 8, 11, or 42, and a future lens - adds a gate to Step 2b rather than a new number to hunt down. + skills, the references, and the docs is N, 8, 11, or the master table's row + count, and a future lens adds a gate to Step 2b rather than a new number to hunt + down. The row count was 42 when this decision was taken; run analysis has since + added `security` · `iac-exposure` and `access-widening`, making it 44. - The gated lenses have no eval surface except the scanner-track prompts — no standalone skill means no skill-level fixture, so their recall and noise gates emulate the brief directly. diff --git a/plugins/code-review/evals/README.md b/plugins/code-review/evals/README.md index 0245b4e..081a4a8 100644 --- a/plugins/code-review/evals/README.md +++ b/plugins/code-review/evals/README.md @@ -22,6 +22,7 @@ evals/ prompts/spec.txt # scanner brief — spec lens ({{spec}} carries the spec path) 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 ``` Node dev deps (`@anthropic-ai/claude-agent-sdk` + `promptfoo`) and the run diff --git a/plugins/code-review/evals/fixtures/access-widening.diff b/plugins/code-review/evals/fixtures/access-widening.diff new file mode 100644 index 0000000..01f3023 --- /dev/null +++ b/plugins/code-review/evals/fixtures/access-widening.diff @@ -0,0 +1,59 @@ +diff --git a/src/api/contracts.router.ts b/src/api/contracts.router.ts +index 8a1c4f2..b77e910 100644 +--- a/src/api/contracts.router.ts ++++ b/src/api/contracts.router.ts +@@ -1,24 +1,23 @@ + import { Router } from 'express' + import { requireAuth } from '../auth/middleware' + import { requireScope } from '../auth/scopes' + import { ContractStore } from '../store/contracts' + + export const contracts = Router() + const store = new ContractStore() + +-contracts.get('/contracts/:id', requireAuth, requireScope('contract:read:own'), async (req, res) => { +- const contract = await store.byId(req.params.id, { ownerId: req.user.id }) ++contracts.get('/contracts/:id', requireAuth, requireScope('org:read'), async (req, res) => { ++ const contract = await store.byId(req.params.id) + if (!contract) return res.status(404).end() + res.json(contract) + }) + +-contracts.get('/contracts/:id/export', requireAuth, async (req, res) => { ++contracts.get('/contracts/:id/export', async (req, res) => { + const rows = await store.exportRows(req.params.id) + res.type('text/csv').send(toCsv(rows)) + }) + +-contracts.post('/contracts/:id/sign', requireAuth, requireScope('contract:sign'), async (req, res) => { ++contracts.post('/contracts/:id/sign', requireAuth, requireScope('contract:sign'), requireOwner, async (req, res) => { + const signed = await store.sign(req.params.id, req.user.id) + res.status(201).json(signed) + }) +diff --git a/src/store/webhooks.ts b/src/store/webhooks.ts +index 3c0e1aa..5f2b881 100644 +--- a/src/store/webhooks.ts ++++ b/src/store/webhooks.ts +@@ -3,11 +3,11 @@ import { db } from '../db' + export class WebhookStore { + async listForTenant(tenantId: string, status: string) { + const rows = await db.query( +- 'SELECT id, url, status FROM webhooks WHERE tenant_id = $1 AND status = $2', +- [tenantId, status], ++ `SELECT id, url, status FROM webhooks WHERE status = '${status}'`, ++ [], + ) + return rows + } + } +diff --git a/config/cors.ts b/config/cors.ts +index 11a9b02..2d4e775 100644 +--- a/config/cors.ts ++++ b/config/cors.ts +@@ -1,8 +1,8 @@ + export const corsOptions = { +- origin: ['https://app.acme.com', 'https://admin.acme.com', 'https://staging.acme.com'], ++ origin: ['https://app.acme.com', 'https://admin.acme.com'], + credentials: true, + maxAge: 600, + } diff --git a/plugins/code-review/evals/fixtures/iac-exposure.tf b/plugins/code-review/evals/fixtures/iac-exposure.tf new file mode 100644 index 0000000..7c506a1 --- /dev/null +++ b/plugins/code-review/evals/fixtures/iac-exposure.tf @@ -0,0 +1,104 @@ +terraform { + required_version = ">= 1.6" +} + +variable "environment" { + type = string + default = "prod" +} + +variable "db_admin_password" { + type = string + default = "Sup3rS3cret-prod!" +} + +resource "tls_private_key" "deploy" { + algorithm = "RSA" + rsa_bits = 4096 +} + +resource "local_file" "deploy_key" { + content = tls_private_key.deploy.private_key_pem + filename = "${path.module}/keys/deploy_id_rsa" +} + +output "database_admin_password" { + value = var.db_admin_password +} + +output "cluster_endpoint" { + value = google_container_cluster.primary.endpoint +} + +data "google_secret_manager_secret_version" "stripe" { + secret = "stripe-webhook-signing-key" +} + +resource "google_cloud_run_service_iam_member" "admin_invoker" { + service = google_cloud_run_service.internal_admin.name + role = "roles/run.invoker" + member = "allUsers" +} + +resource "google_storage_bucket_iam_member" "public_assets" { + bucket = google_storage_bucket.marketing_assets.name + role = "roles/storage.objectViewer" + member = "allUsers" +} + +resource "google_storage_bucket" "marketing_assets" { + name = "acme-marketing-assets" + location = "EU" +} + +resource "google_compute_firewall" "postgres" { + name = "allow-postgres" + network = google_compute_network.main.name + + allow { + protocol = "tcp" + ports = ["5432"] + } + + source_ranges = ["0.0.0.0/0"] +} + +resource "google_compute_firewall" "https" { + name = "allow-https" + network = google_compute_network.main.name + + allow { + protocol = "tcp" + ports = ["443"] + } + + source_ranges = ["0.0.0.0/0"] +} + +resource "google_iam_workload_identity_pool_provider" "github" { + workload_identity_pool_provider_id = "github" + + attribute_condition = "attribute.repository == 'acme/payments'" + + oidc { + issuer_uri = "https://token.actions.githubusercontent.com" + } +} + +resource "google_cloud_run_service" "internal_admin" { + name = "internal-admin" + location = "europe-west1" + + template { + spec { + containers { + image = "eu.gcr.io/acme/webhooks:1.4.2" + + env { + name = "STRIPE_SIGNING_KEY" + value = data.google_secret_manager_secret_version.stripe.secret_data + } + } + } + } +} diff --git a/plugins/code-review/evals/fixtures/quality-calibration-2.ts b/plugins/code-review/evals/fixtures/quality-calibration-2.ts index 78fc163..b5d21e6 100644 --- a/plugins/code-review/evals/fixtures/quality-calibration-2.ts +++ b/plugins/code-review/evals/fixtures/quality-calibration-2.ts @@ -11,6 +11,8 @@ export type Order = { }; const PAGE_SIZE = 20; +const WEB_CHANNEL = "channel = 'web'"; +const PAID_STATUS = "status = 'paid'"; export class OrderDtoMapper { toDto(order: Order): OrderDto { @@ -65,7 +67,7 @@ export class OrdersController { } async list(_req: Request, res: Response): Promise { - const sql = new OrderQuery().where("channel = 'web'").where("status = 'paid'").limit(PAGE_SIZE).build(); + const sql = new OrderQuery().where(WEB_CHANNEL).where(PAID_STATUS).limit(PAGE_SIZE).build(); const orders = await this.repo.query(sql, []); res.json(orders.map((order) => this.mapper.toDto(order))); } diff --git a/plugins/code-review/evals/fixtures/tenant-guard.test.ts b/plugins/code-review/evals/fixtures/tenant-guard.test.ts new file mode 100644 index 0000000..986f702 --- /dev/null +++ b/plugins/code-review/evals/fixtures/tenant-guard.test.ts @@ -0,0 +1,34 @@ +import { describe, it, expect, vi } from 'vitest' +import { requireTenantMember, getInvoice } from './tenant-guard' + +vi.mock('./db', () => ({ + db: { query: vi.fn(async () => [{ id: 'inv_1', total_cents: 2500, status: 'open' }]) }, +})) + +const res = () => { + const r: any = {} + r.status = vi.fn(() => r) + r.json = vi.fn(() => r) + return r +} + +describe('requireTenantMember', () => { + it('rejects a caller from another tenant', () => { + const r = res() + const next = vi.fn() + + requireTenantMember({ session: { tenantId: 't1' }, params: { tenantId: 't1' } } as any, r, next) + + expect(next).toHaveBeenCalled() + }) +}) + +describe('getInvoice', () => { + it('does not leak an invoice belonging to another tenant', async () => { + const r = res() + + await getInvoice({ session: { tenantId: 't1' }, params: { invoiceId: 'inv_1' } } as any, r) + + expect(r.json).toHaveBeenCalled() + }) +}) diff --git a/plugins/code-review/evals/fixtures/tenant-guard.ts b/plugins/code-review/evals/fixtures/tenant-guard.ts new file mode 100644 index 0000000..87eff42 --- /dev/null +++ b/plugins/code-review/evals/fixtures/tenant-guard.ts @@ -0,0 +1,36 @@ +import { Router, type Request, type Response } from 'express' +import { db } from './db' + +export type Session = { userId: string; tenantId: string; roles: string[] } + +export function requireTenantMember(req: Request, res: Response, next: () => void) { + const session = req.session as Session | undefined + if (!session) return res.status(401).json({ error: 'unauthenticated' }) + if (session.tenantId !== req.params.tenantId) { + return res.status(403).json({ error: 'forbidden' }) + } + next() +} + +export async function listInvoices(req: Request, res: Response) { + const session = req.session as Session + const rows = await db.query( + 'SELECT id, total_cents, status FROM invoices WHERE tenant_id = $1 ORDER BY issued_at DESC LIMIT 100', + [session.tenantId], + ) + res.json(rows) +} + +export async function getInvoice(req: Request, res: Response) { + const session = req.session as Session + const [row] = await db.query( + 'SELECT id, total_cents, status FROM invoices WHERE id = $1 AND tenant_id = $2', + [req.params.invoiceId, session.tenantId], + ) + if (!row) return res.status(404).json({ error: 'not found' }) + res.json(row) +} + +export const router = Router() +router.get('/tenants/:tenantId/invoices', requireTenantMember, listInvoices) +router.get('/tenants/:tenantId/invoices/:invoiceId', requireTenantMember, getInvoice) diff --git a/plugins/code-review/evals/promptfooconfig.yaml b/plugins/code-review/evals/promptfooconfig.yaml index f326bdb..f2cd5e6 100644 --- a/plugins/code-review/evals/promptfooconfig.yaml +++ b/plugins/code-review/evals/promptfooconfig.yaml @@ -16,6 +16,8 @@ prompts: label: security-track - id: file://prompts/performance.txt label: performance-track + - id: file://prompts/security-diff.txt + label: security-diff-track - id: file://prompts/spec.txt label: spec-track # Standards track: quality-review with the fixture dir declared as the repository root @@ -687,3 +689,119 @@ tests: `` `standards` · · · L — "" ( ›

) → ``: a kebab-case slug, a verbatim quoted rule, and a file › section citation. No `standards` bullet lacks the quoted rule. + + # ============================================================================ + # Regression guards — the rules and the scope boundaries added from run analysis + # ============================================================================ + + - description: 'eval-16 iac-exposure-recall (iac-exposure.tf)' + prompts: [security-track] + vars: + file: plugins/code-review/evals/fixtures/iac-exposure.tf + assert: + - type: regex + value: 'iac-exposure' + - type: llm-rubric + value: >- + Flags the generated `tls_private_key.deploy` — written to remote state and to + `local_file.deploy_key` on disk — as `security` · iac-exposure · high. + - type: llm-rubric + value: >- + Flags BOTH credential leaks in the declaration: the plaintext `default` on the + `db_admin_password` variable and the `database_admin_password` output that + carries it without `sensitive`. + - type: llm-rubric + value: >- + Flags the `allUsers` invoker binding on the `internal_admin` Cloud Run service + and the `0.0.0.0/0` source range reaching port 5432 as over-wide grants + (`iac-exposure`, high). + - type: llm-rubric + value: >- + Flags the GitHub OIDC provider whose `attribute_condition` pins only + `attribute.repository` — any workflow or ref of that repository satisfies it. + - type: llm-rubric + value: >- + Does NOT flag as findings, and names on the `Not flagged` line: the `allUsers` + objectViewer binding on the marketing-assets bucket (public by design), the + `0.0.0.0/0` range on port 443 (a public port), the Stripe key read from the + secret-manager data source, and the `cluster_endpoint` output (not a + credential). + - type: llm-rubric + value: >- + Every finding is tagged `security` with a rule from the master table and + severity `high` or `medium` — no craft family (readability, naming, module, + objects, patterns, simplicity) appears at all. + + - description: 'eval-17 access-widening-diff (access-widening.diff)' + prompts: [security-diff-track] + vars: + file: plugins/code-review/evals/fixtures/access-widening.diff + assert: + - type: regex + value: 'access-widening' + - type: llm-rubric + value: >- + Flags the `/contracts/:id` route as `security` · access-widening · high: the + required scope went from `contract:read:own` to `org:read` AND the + `{ ownerId: req.user.id }` predicate was dropped from the store call. + - type: llm-rubric + value: >- + Flags the `/contracts/:id/export` route, which lost `requireAuth` entirely. + - type: llm-rubric + value: >- + Flags the webhook query losing its `tenant_id` predicate as `access-widening` + (an `injection-sink` finding for the interpolated `status` alongside it is + correct, not a substitute for it). + - type: llm-rubric + value: >- + Does NOT flag the two hunks that tighten: `requireOwner` added to the sign + route, and the CORS origin list losing `https://staging.acme.com`. Both are + named on the `Not flagged` line. + + - description: 'eval-18 guard-test-fidelity-routing (tenant-guard.ts + tenant-guard.test.ts)' + prompts: [security-track] + vars: + file: plugins/code-review/evals/fixtures/tenant-guard.ts plugins/code-review/evals/fixtures/tenant-guard.test.ts + assert: + - type: llm-rubric + value: >- + Raises NO `missing-access-check` finding against `tenant-guard.ts`: the + middleware compares the session tenant with the route tenant, and both queries + carry a `tenant_id` predicate bound as a parameter. + - type: llm-rubric + value: >- + Identifies that both tests claim a boundary their assertions never exercise + (each passes the SAME tenant and asserts only that the happy path ran), and + routes it to the `tests` family as test-fidelity — in a HANDOFF block or named + as belonging to the tests lens — rather than grading it as a `security` finding + of any severity. + - type: llm-rubric + value: >- + No finding in the output carries the `security` family with severity `high`. + + - description: 'eval-19 scope-classification (scope-mix/)' + prompts: [quality-track] + vars: + # Outside fixtures/: scope.md makes everything under a fixtures/ directory `test` kind, + # which would hide the source-vs-test classification this eval checks. + file: plugins/code-review/evals/scope-mix + assert: + - type: llm-rubric + value: >- + `build.mjs` and `legacy-report.cjs` were reviewed as source files — the report + judges their content (findings against them, or an explicit clean verdict naming + them). Neither is listed as skipped or dismissed as tooling. + - type: llm-rubric + value: >- + `tests-e2e/checkout.spec.ts` is treated as a test file — it is reviewed, and any + finding against it comes from the `tests` family (for example the interleaved + act/assert steps as test-structure), never dismissed for sitting outside a + directory named `test`. + - type: llm-rubric + value: >- + The `Skipped` line names `.github/workflows/ci.yml` and says its triggers, + permissions and secret handling are not line-graded here, pointing the reader at + `/security-review`. `notes.txt` is listed as skipped too. + - type: regex + value: '[Ss]kipped' + diff --git a/plugins/code-review/evals/prompts/security-diff.txt b/plugins/code-review/evals/prompts/security-diff.txt new file mode 100644 index 0000000..2d9658c --- /dev/null +++ b/plugins/code-review/evals/prompts/security-diff.txt @@ -0,0 +1,18 @@ +Act as the `security` scanner of the code-review plugin. Read +`plugins/code-review/references/rules/security.md` and +`plugins/code-review/references/severity.md` completely, then read {{file}} in full — +it is a **unified diff** of the change under review, so the `-` side is what stood +before and the `+` side is what the change introduces — and judge it against the +`security` family only. The conventions note is "none" and the standards slot is +"none". The files the diff touches are not on disk; the diff is the whole evidence. + +Return findings only: no report skeleton, no headline, no tally, no edits, and nothing +written to disk. State each finding as its own markdown bullet in exactly this shape: + +`security` · rule · severity · L — → + +`L` names the diff's own file and line region (`src/api/contracts.router.ts` +around the changed hunk); a pattern alone is never a finding. After the findings add +one `Not flagged:` prose line naming each look-alike you cleared and the mitigation +that clears it — a hunk that *tightens* a boundary belongs there, never among the +findings. diff --git a/plugins/code-review/evals/prompts/security.txt b/plugins/code-review/evals/prompts/security.txt index fd926f6..2e61a3e 100644 --- a/plugins/code-review/evals/prompts/security.txt +++ b/plugins/code-review/evals/prompts/security.txt @@ -1,8 +1,9 @@ Act as the `security` scanner of the code-review plugin. Read `plugins/code-review/references/rules/security.md` and -`plugins/code-review/references/severity.md` completely, then read {{file}} in full — -the whole file counts as added code — and judge it against the `security` family only. -The conventions note is "none" and the standards slot is "none". +`plugins/code-review/references/severity.md` completely, then read every file in +{{file}} in full — each path is a separate file, and the whole of each counts as added +code — and judge them against the `security` family only. The conventions note is +"none" and the standards slot is "none". Return findings only: no report skeleton, no headline, no tally, no edits, and nothing written to disk — not the file under review and not a scratch file. State each finding @@ -14,4 +15,5 @@ as its own markdown bullet in exactly this shape: and which is the sink; a pattern alone is never a finding. After the findings add one `Not flagged:` prose line naming each look-alike you cleared and the mitigation that clears it, and a `CANDIDATES` block only for a confirmed pair whose mitigation is in -doubt. +doubt. A real problem that belongs to another lens's family goes in a `HANDOFF` block at +the end, naming that family and rule — never graded as a `security` finding to keep it. diff --git a/plugins/code-review/evals/scope-mix/.github/workflows/ci.yml b/plugins/code-review/evals/scope-mix/.github/workflows/ci.yml new file mode 100644 index 0000000..4a017fd --- /dev/null +++ b/plugins/code-review/evals/scope-mix/.github/workflows/ci.yml @@ -0,0 +1,21 @@ +name: ci + +on: + pull_request_target: + branches: [main] + +permissions: write-all + +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.sha }} + - uses: actions/setup-node@v4 + with: + node-version: 20 + - run: npm ci && npm test + env: + NPM_TOKEN: ${{ secrets.NPM_TOKEN }} diff --git a/plugins/code-review/evals/scope-mix/build.mjs b/plugins/code-review/evals/scope-mix/build.mjs new file mode 100644 index 0000000..2b48401 --- /dev/null +++ b/plugins/code-review/evals/scope-mix/build.mjs @@ -0,0 +1,28 @@ +import { readFile, writeFile, readdir } from 'node:fs/promises' +import { join, extname } from 'node:path' + +export async function buildManifest(sourceDir, outFile) { + const entries = await readdir(sourceDir, { withFileTypes: true }) + const manifest = [] + + for (const entry of entries) { + if (!entry.isFile()) continue + if (extname(entry.name) !== '.json') continue + + const raw = await readFile(join(sourceDir, entry.name), 'utf8') + const parsed = JSON.parse(raw) + + if (parsed.expiresAt && Date.now() - parsed.expiresAt > 604800000) continue + + manifest.push({ + id: parsed.id, + title: parsed.title, + slug: parsed.title.toLowerCase().replace(/[^a-z0-9]+/g, '-').replace(/^-|-$/g, ''), + tags: (parsed.tags ?? []).map((t) => t.trim().toLowerCase()).filter(Boolean), + }) + } + + manifest.sort((a, b) => a.slug.localeCompare(b.slug)) + await writeFile(outFile, JSON.stringify({ generatedAt: Date.now(), entries: manifest }, null, 2)) + return manifest.length +} diff --git a/plugins/code-review/evals/scope-mix/legacy-report.cjs b/plugins/code-review/evals/scope-mix/legacy-report.cjs new file mode 100644 index 0000000..9131af5 --- /dev/null +++ b/plugins/code-review/evals/scope-mix/legacy-report.cjs @@ -0,0 +1,12 @@ +const { createWriteStream } = require('node:fs') + +function proc(rows, out) { + const s = createWriteStream(out) + s.write('id,title,slug,tags\n') + for (const r of rows) { + s.write([r.id, JSON.stringify(r.title), r.title.toLowerCase().replace(/[^a-z0-9]+/g, '-'), r.tags.join(' ')].join(',') + '\n') + } + s.end() +} + +module.exports = { proc } diff --git a/plugins/code-review/evals/scope-mix/notes.txt b/plugins/code-review/evals/scope-mix/notes.txt new file mode 100644 index 0000000..26b6d35 --- /dev/null +++ b/plugins/code-review/evals/scope-mix/notes.txt @@ -0,0 +1,5 @@ +Release notes draft for the manifest builder change. + +- the manifest now skips entries whose expiry is more than a week old +- slugs are generated from the title rather than taken from the payload +- the legacy CSV reporter keeps its own copy of the slug rule for now diff --git a/plugins/code-review/evals/scope-mix/tests-e2e/checkout.spec.ts b/plugins/code-review/evals/scope-mix/tests-e2e/checkout.spec.ts new file mode 100644 index 0000000..c37a690 --- /dev/null +++ b/plugins/code-review/evals/scope-mix/tests-e2e/checkout.spec.ts @@ -0,0 +1,12 @@ +import { test, expect } from '@playwright/test' + +test('a signed-in shopper can pay for a basket', async ({ page }) => { + await page.goto('/basket') + await page.getByRole('button', { name: 'Checkout' }).click() + expect(await page.getByTestId('step').textContent()).toBe('payment') + await page.getByLabel('Card number').fill('4242424242424242') + await page.getByLabel('Expiry').fill('12/30') + expect(await page.getByRole('button', { name: 'Pay' }).isEnabled()).toBe(true) + await page.getByRole('button', { name: 'Pay' }).click() + await expect(page.getByTestId('receipt')).toBeVisible() +}) diff --git a/plugins/code-review/references/rules/comments.md b/plugins/code-review/references/rules/comments.md index 7d15dfb..53708e8 100644 --- a/plugins/code-review/references/rules/comments.md +++ b/plugins/code-review/references/rules/comments.md @@ -125,6 +125,15 @@ hides coupling: the reader has to leave the code to understand the code. ``` The fragment is the leak: it is fine to keep the real constraint, but the `(R2)`, the `F1:`, the `§4.1` must not ride along into the kept comment. +- **Resolve the token against the code before you strip it, and say how it + resolved.** A letter+number reads like a spec-id and can just as easily be a + value the code uses — a region (`R2`), a tier, an enum member, a column name. + `Grep` the token in the file and its neighbours: bound to an identifier or a + string literal, it is a code value and the comment naming it is not an R4 + finding at all; found nowhere in the code, it points into a document and the + strip applies. The verdict line says which of the two it was, in a clause — + a strip whose reasoning is "it looks like a spec-id" is the one way this rule + deletes a fact the reader needed. - **No "provenance" loophole.** A doc/file ref glued onto an otherwise self-contained sentence still goes (`// that hard-stop is intentional (DD_PLAN.md T4.1)` → `// that hard-stop is intentional`). The test is simple: diff --git a/plugins/code-review/references/rules/security.md b/plugins/code-review/references/rules/security.md index 7f128ac..a336c3c 100644 --- a/plugins/code-review/references/rules/security.md +++ b/plugins/code-review/references/rules/security.md @@ -25,7 +25,7 @@ first pass. ## Contents - `security` — secret-in-source, injection-sink, missing-access-check, - unvalidated-boundary, insecure-setting + unvalidated-boundary, insecure-setting, iac-exposure, access-widening Every rule below carries its **Flag** conditions, a **Suggested fix**, and a **Calibration** paragraph naming the look-alike that is *not* a violation. @@ -37,6 +37,8 @@ Every rule below carries its **Flag** conditions, a **Suggested fix**, and a | `security` | missing-access-check | handler reading/mutating a resource with no authn/authz guard, or request-supplied id with no ownership/tenant predicate | high | | `security` | unvalidated-boundary | HTTP/CLI/env/queue/third-party payload used in logic or persistence with no parse/validate at entry | medium | | `security` | insecure-setting | a literal disabling a protection (`rejectUnauthorized:false`, `verify=False`, unsafe `yaml.load`, `Math.random` for tokens, CORS `*`+credentials) | high | +| `security` | iac-exposure | infrastructure code storing a secret where others can read it, or admitting an identity wider than the one it names | high | +| `security` | access-widening | the change relaxes an authorization boundary that existed — a weaker permission, a dropped guard or owner predicate, an allowlist opened up | high | ### Confirm the sink — the discipline for the whole lens @@ -50,7 +52,8 @@ Every rule below carries its **Flag** conditions, a **Suggested fix**, and a builder parameterizing, does this decorator check ownership? Name the pair; a candidate with no named pair is a cleared prose line. Cleared look-alikes go to `Not flagged`, one prose line naming the pair and the mitigation — never the finding - shape. + shape. The block is a heading and markdown bullets, never a fenced code block: a fence + makes the orchestrator's merge read it as source rather than as findings. - **Never run the code; never run `npm audit`, a secret scanner, or any network command.** The evidence is the lines in view and what Read/Grep return. - **`.env`, yaml, JSON, manifests, and lockfiles are out of this lens** — skipped by @@ -146,7 +149,10 @@ Authentication says who is calling; authorization says whether *this* caller may registration (read it — an `app.use(auth)` above the route clears the whole group); a deliberately public endpoint (health, login, signup, a signature-verified webhook); a query already scoped to the session's own tenant one layer up; code with no - request-facing caller. This rule almost always needs the registration read; when it + request-facing caller — **a test that would stay green if the guard regressed is + `tests` · test-fidelity, not this rule**: the missing assertion is a test defect, and + grading it here turns a medium into a high and puts the whole review under a security + headline it has not earned. This rule almost always needs the registration read; when it is out of reach, the finding is `(verify)`, never an assertion. #### `unvalidated-boundary` — parse at the edge, then trust @@ -195,3 +201,56 @@ checks, safe parsing, unpredictable tokens, origin isolation. elsewhere — say so); `Math.random` for a non-security value (jitter, sampling); CORS `*` with no credentials on a public read-only API. A gate you cannot read is `(verify)`. + +#### `iac-exposure` — infrastructure that stores a secret readably, or trusts too widely + +Infrastructure code is in scope for this lens (`.tf`/HCL and the declarative surfaces +`scope.md` classifies as `iac`), and it fails differently from application code: nothing +is executed, so the harm sits in what a declaration *stores* and *admits*. + +- **Flag** when: + - a secret ends up somewhere the declaration does not control — a generated key or + password materialized as a resource attribute, so it lands in remote state + (`tls_private_key`, a service-account or access-key resource, a `local_file` of a + key); an output carrying a credential without `sensitive`; a plaintext `default` on + a credential variable; + - a trust or access grant is wider than the identity it names — a federation or OIDC + condition that matches beyond the branch, environment or workflow intended (a `sub` + pinned to `refs/heads/main` still matches a workflow that runs on + `pull_request_target`); a wildcard principal (`allUsers`, `AWS: "*"`, a project-wide + binding where one service account was meant); a bucket, topic or dataset opened to + anonymous access; `0.0.0.0/0` reaching a non-public port. + Ends: the declaration line and where the value becomes readable, or the identity the + grant admits — name it (`the prod state bucket`, `any workflow run of any fork`). +- **Suggested fix**: name the mechanism the stack already has — reference the secret + manager instead of materializing the value, mark the output `sensitive` and keep the + key out of state, tighten the condition to the full ref *and* the workflow, name the + exact principal, or replace the open CIDR with the peer range. +- **Calibration → not a finding**: a value read from a secret-manager data source; a + resource public by design (an assets bucket behind a CDN, a load balancer's public + address); a wildcard inside a scope the provider narrows by another condition you + have read; a fixture in a test or sandbox module; a breadth the Step 2 conventions + note documents. State cannot be read from the file, so a claim about *who* can read + the state bucket is `(verify)` unless the configuration in view says so. + +#### `access-widening` — the change relaxes a boundary that was there + +The diff is the evidence here: a permission, guard or predicate that stood on the `-` +side and is weaker or gone on the `+` side hands data to callers who could not reach it +yesterday, and nothing in the code looks wrong afterwards. + +- **Flag** when the change, on an existing path: swaps a required permission, role or + scope for a broader one (`admin:contract:read` → an org-wide read); removes or + loosens a guard, decorator or middleware; drops an owner or tenant predicate from a + query that had one; moves a route out of an authenticated group; widens an allowlist, + origin list or audience to a wildcard; lowers a validation that gated who may write. + Ends: the removed or weakened line (quote the `-` side) and the resource it now + admits. +- **Suggested fix**: name the boundary that was there and what would restore it, or the + narrower predicate that covers the new caller. +- **Calibration → not a finding**: a widening the `--spec` text or the Step 2 note + explicitly asks for (clear it in one prose line naming where it is written); a rename + of the same permission; a boundary moved rather than removed — the guard now sits one + layer up and you have read it; a new endpoint with no previous boundary, which is + `missing-access-check` territory if anything. When the diff does not show the previous + boundary, this rule does not apply. diff --git a/plugins/code-review/references/scope.md b/plugins/code-review/references/scope.md index 25c7d3c..7d536b2 100644 --- a/plugins/code-review/references/scope.md +++ b/plugins/code-review/references/scope.md @@ -9,29 +9,44 @@ findings. ## In scope -Source files that carry human-authored code and comments: `.ts .tsx .js .jsx .py .go -.rs .java .kt .swift .c .cpp .h .rb .php .vue .scala .cs .sh`, plus +Source files that carry human-authored code and comments: `.ts .tsx .mts .cts .js .jsx +.mjs .cjs .py .go .rs .java .kt .swift .c .cpp .h .rb .php .vue .scala .cs .sh`, plus **infrastructure-as-code** (`.tf`/HCL and similar declarative surfaces that still carry -comments and structure worth reviewing). +comments and structure worth reviewing). The ES-module and CommonJS extensions carry the +same hand-written code as `.js` — a build script or a config-as-code module under +`.mjs`/`.cjs` is reviewed, not skipped as tooling. ## Skip JSON, lockfiles, generated or minified files (a generator's `.d.ts`, `*_pb.*`, anything under `dist/`, `build/`, `node_modules/`), `.md` and docs (in a comment review the prose -*is* the content), **static config data** (`.yaml`/`.toml`/`.ini` settings, `.env`), and -license/SPDX headers. +*is* the content), plain text (`.txt`), **static config data** (`.yaml`/`.toml`/`.ini` +settings, `.env`), and license/SPDX headers. Note every skipped file in one line, so coverage stays honest. +**CI workflow definitions are skipped with a security sentence.** A changed +`.github/workflows/*.yml`, `.gitlab-ci.yml`, or equivalent pipeline file is static config +by these rules, but it is the one skipped kind that routinely carries a real exposure — a +`pull_request_target` job checking out and running untrusted code, a secret passed into a +step that echoes it, a third-party action pinned to a moving tag. Say on the `Skipped` +line that the pipeline files were not line-graded and that their permissions, triggers and +secret handling belong to `/security-review`. If the `security` Scanner nonetheless reads +one and finds a confirmed exposure, that finding stands — `security` · `iac-exposure` +covers it. + **A skipped file that is the substance of the change gets its own sentence.** A dependency manifest (`package.json`, `composer.json`, …) on a dependency-bump or upgrade -branch is the whole point of that diff. Say so explicitly rather than burying it in the -skip list: its dependency changes aren't line-graded, and the reader should read that as -a deliberate scope boundary rather than an oversight. The same sentence carries a second -boundary: a changed `.env*`, dependency manifest, or lockfile is also **not secret- or -dependency-scanned** by the `security` lens, which reads source files only. Say that on -the `Skipped` line and point the reader to `/security-review` for the dependency and -configuration audit this review does not do. +branch is the whole point of that diff, and so is the prose on a documentation branch, a +spec/ADR change, or a release-notes commit — when the skipped files *are* the change, +name that in a sentence of its own and say what the review therefore covers (often only +the handful of source files that came along for the ride). Say so explicitly rather than +burying it in the skip list: the skipped content isn't line-graded, and the reader should +read that as a deliberate scope boundary rather than an oversight. The same sentence +carries a second boundary: a changed `.env*`, dependency manifest, or lockfile is also +**not secret- or dependency-scanned** by the `security` lens, which reads source files +only. Say that on the `Skipped` line and point the reader to `/security-review` for the +dependency and configuration audit this review does not do. ## File kinds @@ -40,7 +55,9 @@ eyeballing the content: - **`test`** — any file under one of these directories: `__tests__/`, `test/`, `tests/`, `spec/`, `e2e/`, `cypress/`, `fixtures/`, `__mocks__/`, `__snapshots__/`, - `testdata/`; or whose name matches one of: `*.test.*`, `*.spec.*`, `*_test.go`, + `testdata/`; under any directory whose name *contains* `e2e` (`tests-e2e/`, + `e2e-tests/`, `apps/web-e2e/`) or ends in `-tests`/`-test`; or whose name matches one + of: `*.test.*`, `*.spec.*`, `*_test.go`, `test_*.py`, `*_test.py`, `conftest.py`, `*Test.php`, `*Test.java`, `*Test.kt`, `*Tests.cs`, `*_spec.rb`, `*.feature`, `*.stories.*`, `setupTests.*`. - **`iac`** — `.tf`/HCL and the other declarative infrastructure-as-code surfaces named diff --git a/plugins/code-review/references/severity.md b/plugins/code-review/references/severity.md index 4d3f8b9..7440c5c 100644 --- a/plugins/code-review/references/severity.md +++ b/plugins/code-review/references/severity.md @@ -4,7 +4,7 @@ Every finding that reports as `family` · rule · severity carries one **family* **rule**, and one **severity**, all three **verbatim** from this table — never a code number, never a paraphrase invented this run. Two reviews of the same code name the same `family` · rule every time. Eleven families report in that shape: the ten below -with **42 fixed rows**, plus `standards`, whose rules are the project's own and are +with **44 fixed rows**, plus `standards`, whose rules are the project's own and are graded by the mapping at the end of this file. Severity is exactly one of `high`, `medium`, or `nit`. There is no `low`, no @@ -46,6 +46,8 @@ Severity is exactly one of `high`, `medium`, or `nit`. There is no `low`, no | `security` | missing-access-check | handler reading/mutating a resource with no authn/authz guard, or request-supplied id with no ownership/tenant predicate | high | | `security` | unvalidated-boundary | HTTP/CLI/env/queue/third-party payload used in logic or persistence with no parse/validate at entry | medium | | `security` | insecure-setting | a literal disabling a protection (`rejectUnauthorized:false`, `verify=False`, unsafe `yaml.load`, `Math.random` for tokens, CORS `*`+credentials) | high | +| `security` | iac-exposure | infrastructure code storing a secret where others can read it (state, an unmarked output), or a trust/access grant wider than the identity it names | high | +| `security` | access-widening | the change relaxes an authorization boundary that existed — a weaker permission, a removed guard or owner predicate, an allowlist opened up | high | | `performance` | n-plus-one | per-item DB/HTTP/IO call inside a loop over an unbounded collection where a batch form exists | high | | `performance` | unbounded-fetch | a list read with no limit/pagination over data that grows (incl. list endpoints) | medium | | `performance` | blocking-in-async | sync blocking call on a request-serving/event-loop path (N/A outside Node & Python asyncio) | medium | @@ -66,10 +68,12 @@ Severity is exactly one of `high`, `medium`, or `nit`. There is no `low`, no cycle or inverts the layering (`dependency-direction`), a helper the repo already exports (`canonical-helper`), a per-item call that multiplies with the data (`n-plus-one`), a spec line left unimplemented or implemented against its wording - (`missing-requirement`, `wrong-implementation`), and the four security rules that + (`missing-requirement`, `wrong-implementation`), and the six security rules that name a confirmed exposure — a literal credential (`secret-in-source`), untrusted data reaching a sink (`injection-sink`), an unguarded resource (`missing-access-check`), - and a protection switched off (`insecure-setting`). These cost the most to live with. + a protection switched off (`insecure-setting`), a secret or over-wide grant declared + into infrastructure (`iac-exposure`), and a boundary the change relaxes + (`access-widening`). These cost the most to live with. - **medium** — readability friction a reader feels every time, or a latent gap that matters: `ordering`, `test-structure` interleaving, a `test-fidelity` name/fixture that claims more than its assertions check, `guard-clause` nesting, an unexplained diff --git a/plugins/code-review/scripts/get_changes.py b/plugins/code-review/scripts/get_changes.py index d6c16f0..26ba215 100755 --- a/plugins/code-review/scripts/get_changes.py +++ b/plugins/code-review/scripts/get_changes.py @@ -23,7 +23,13 @@ "files": [ {"path": "src/foo.ts", "status": "M", "binary": false} ], - "count": 1 + "count": 1, + "alternate": { // only when the run found nothing + "ref": "origin/main", // and another base would have + "base": "def5678", + "diff_args": ["def5678..HEAD"], + "count": 7 + } } Status codes follow `git diff --name-status`: @@ -58,25 +64,52 @@ def _ref_exists(ref: str) -> bool: return res.returncode == 0 -def _resolve_base(explicit: Optional[str]) -> str: +BASE_CANDIDATES = ["@{upstream}", "origin/main", "origin/master", "main", "master"] + + +def _merge_base(ref: str) -> Optional[str]: + if not _ref_exists(ref): + return None + return _run(["git", "merge-base", "HEAD", ref], check=False).strip() or None + + +def _resolve_base(explicit: Optional[str]) -> tuple[str, str]: if explicit: if not _ref_exists(explicit): raise SystemExit(f"--base ref does not exist: {explicit}") merge_base = _run(["git", "merge-base", "HEAD", explicit]).strip() - return merge_base or explicit - - candidates = ["@{upstream}", "origin/main", "origin/master", "main", "master"] - for ref in candidates: - if _ref_exists(ref): - mb = _run(["git", "merge-base", "HEAD", ref], check=False).strip() - if mb: - return mb + return merge_base or explicit, explicit + + for ref in BASE_CANDIDATES: + mb = _merge_base(ref) + if mb: + return mb, ref raise SystemExit( "could not resolve a base ref — set upstream, push to origin/main, " "or pass --base " ) +def _alternate(scope: str, base: str, used_ref: str) -> Optional[dict]: + """A second base worth reporting when the resolved one saw no change. + + A branch whose upstream is its own remote counterpart diffs to nothing the + moment it is pushed, which reads as "no changes" while the whole branch is + still unreviewed against the trunk. + """ + for ref in BASE_CANDIDATES[1:]: + if ref == used_ref: + continue + mb = _merge_base(ref) + if not mb or mb == base: + continue + ref_args = [mb] if scope == "both" else [f"{mb}..HEAD"] + files = _list_files(ref_args) + if files: + return {"ref": ref, "base": mb, "diff_args": ref_args, "count": len(files)} + return None + + def _is_binary(path: str, ref_args: list[str]) -> bool: """Detect binary files via git diff --numstat ('-' for binary).""" out = _run(["git", "diff", "--numstat", *ref_args, "--", path], check=False) @@ -153,15 +186,16 @@ def main() -> int: raise SystemExit("not inside a git repository") include_untracked = False + used_ref = None if args.scope == "uncommitted": ref_args = ["HEAD"] base = None include_untracked = True elif args.scope == "committed": - base = _resolve_base(args.base) + base, used_ref = _resolve_base(args.base) ref_args = [f"{base}..HEAD"] else: # both - base = _resolve_base(args.base) + base, used_ref = _resolve_base(args.base) ref_args = [base] include_untracked = True @@ -171,17 +205,18 @@ def main() -> int: for u in _list_untracked(): if u["path"] not in tracked_paths: files.append(u) - json.dump( - { - "scope": args.scope, - "base": base, - "diff_args": ref_args, - "files": files, - "count": len(files), - }, - sys.stdout, - indent=2, - ) + payload = { + "scope": args.scope, + "base": base, + "diff_args": ref_args, + "files": files, + "count": len(files), + } + if not files and base and not args.base: + alternate = _alternate(args.scope, base, used_ref) + if alternate: + payload["alternate"] = alternate + json.dump(payload, sys.stdout, indent=2) sys.stdout.write("\n") return 0 diff --git a/plugins/code-review/scripts/tests/test_get_changes.py b/plugins/code-review/scripts/tests/test_get_changes.py new file mode 100644 index 0000000..e0e5930 --- /dev/null +++ b/plugins/code-review/scripts/tests/test_get_changes.py @@ -0,0 +1,87 @@ +"""get_changes.py against real git repositories built per test.""" +from __future__ import annotations + +import json +import subprocess +import sys +from pathlib import Path + +import pytest + +SCRIPT = Path(__file__).resolve().parents[1] / "get_changes.py" + + +def git(repo: Path, *args: str) -> str: + res = subprocess.run( + ["git", *args], cwd=repo, capture_output=True, text=True, check=True + ) + return res.stdout + + +def commit(repo: Path, name: str, body: str) -> None: + (repo / name).write_text(body) + git(repo, "add", name) + git(repo, "commit", "-m", f"add {name}") + + +def run_script(repo: Path, *args: str) -> dict: + res = subprocess.run( + [sys.executable, str(SCRIPT), *args], + cwd=repo, capture_output=True, text=True, check=True, + ) + return json.loads(res.stdout) + + +@pytest.fixture +def origin_repo(tmp_path: Path) -> Path: + """A clone whose branch tracks its own pushed counterpart on origin.""" + upstream = tmp_path / "origin.git" + seed = tmp_path / "seed" + seed.mkdir() + git(seed, "init", "-b", "main") + git(seed, "config", "user.email", "t@example.com") + git(seed, "config", "user.name", "T") + commit(seed, "base.ts", "export const a = 1\n") + git(seed, "clone", "--bare", str(seed), str(upstream)) + + work = tmp_path / "work" + subprocess.run(["git", "clone", str(upstream), str(work)], check=True, + capture_output=True) + git(work, "config", "user.email", "t@example.com") + git(work, "config", "user.name", "T") + git(work, "checkout", "-b", "feature") + commit(work, "feature.ts", "export const b = 2\n") + git(work, "push", "-u", "origin", "feature") + return work + + +def test_committed_reports_alternate_base_when_upstream_sees_nothing(origin_repo: Path): + out = run_script(origin_repo, "--scope", "committed") + + assert out["count"] == 0 + alt = out["alternate"] + assert alt["ref"] == "origin/main" + assert alt["count"] == 1 + files = run_script(origin_repo, "--scope", "committed", "--base", alt["ref"])["files"] + assert [f["path"] for f in files] == ["feature.ts"] + + +def test_alternate_appears_until_the_resolved_base_sees_the_change(origin_repo: Path): + git(origin_repo, "commit", "--allow-empty", "-m", "unpushed") + + out = run_script(origin_repo, "--scope", "committed") + + assert out["count"] == 0 # the empty commit touches no file + assert "alternate" in out # …but origin/main still holds feature.ts + + commit(origin_repo, "later.ts", "export const c = 3\n") + out = run_script(origin_repo, "--scope", "committed") + assert [f["path"] for f in out["files"]] == ["later.ts"] + assert "alternate" not in out + + +def test_explicit_base_never_gets_an_alternate(origin_repo: Path): + out = run_script(origin_repo, "--scope", "committed", "--base", "HEAD") + + assert out["count"] == 0 + assert "alternate" not in out diff --git a/plugins/code-review/skills/comment-review/SKILL.md b/plugins/code-review/skills/comment-review/SKILL.md index 8bdffdc..6c270e1 100644 --- a/plugins/code-review/skills/comment-review/SKILL.md +++ b/plugins/code-review/skills/comment-review/SKILL.md @@ -94,7 +94,13 @@ Parse the invocation arguments: (append `--base ` to both when the user passed one.) Read the `count` of each: - - both zero → tell the user there is nothing to review and stop; + - both zero → before concluding, look for an `alternate` object in the `committed` + output: the script adds it when the resolved base saw nothing but another base + (usually `origin/main`) holds real commits, which is what a freshly pushed branch + tracking its own remote counterpart looks like. When it is there, say which base was + used, which one differs and by how many files, and offer to re-run with + `--base ` — do not report "nothing to review" over it. With no + `alternate`, tell the user there is nothing to review and stop; - exactly one non-zero → use that scope automatically; - both non-zero → ask with `AskUserQuestion` which to review — **Uncommitted** (working tree vs HEAD), **Committed** (HEAD vs base), or **Both** (base → working diff --git a/plugins/code-review/skills/quality-review/SKILL.md b/plugins/code-review/skills/quality-review/SKILL.md index 6ab41a6..82c35c2 100644 --- a/plugins/code-review/skills/quality-review/SKILL.md +++ b/plugins/code-review/skills/quality-review/SKILL.md @@ -113,7 +113,13 @@ Parse the invocation arguments: (append `--base ` to both when the user passed one.) Read the `count` of each: - - both zero → tell the user there is nothing to review and stop; + - both zero → before concluding, look for an `alternate` object in the `committed` + output: the script adds it when the resolved base saw nothing but another base + (usually `origin/main`) holds real commits, which is what a freshly pushed branch + tracking its own remote counterpart looks like. When it is there, say which base was + used, which one differs and by how many files, and offer to re-run with + `--base ` — do not report "nothing to review" over it. With no + `alternate`, tell the user there is nothing to review and stop; - exactly one non-zero → use that scope automatically; - both non-zero → ask with `AskUserQuestion` which to review — **Uncommitted** (working tree vs HEAD), **Committed** (HEAD vs base), or **Both** (base → working @@ -226,7 +232,7 @@ anything: | `simplicity` | `${CLAUDE_PLUGIN_ROOT}/references/rules/simplicity-types.md` | **Severity comes from `${CLAUDE_PLUGIN_ROOT}/references/severity.md`** — the master -table of all 42 fixed rules, what `high` / `medium` / `nit` each mean, the keyword +table of all 44 fixed rules, what `high` / `medium` / `nit` each mean, the keyword mapping for `standards` findings, and the anti-anchoring rule. Read it and grade every finding against its own row there (a `standards` finding against its keyword). The family, the rule, and the severity are all used **verbatim**, so a reader (and a diff @@ -253,7 +259,7 @@ kind of noise. So render the report with **exactly this template**, in this orde ### - `family` · rule · severity · L — <…> -**Not flagged:** +**Not flagged:** **Boy-scout (untouched code, optional):** - `family` · rule · :L — @@ -318,6 +324,11 @@ Rules for filling it in: the wall-of-text this format exists to kill. The full refactor belongs in Step 4 (apply time) or when the user asks to see it. If a fix genuinely cannot be named without a few tokens of code, inline at most a short expression. +- **`Not flagged` is not optional when you cleared something.** A clean file is the case + that most needs it: with no findings to read, the line is the only evidence that the + look-alikes were considered rather than missed, and a reader cannot tell a review that + cleared six candidates from one that never looked. Name them in the line, never only in + the prose of your own reasoning. - **`Not flagged`** is **one line** — a comma-separated list of the look-alikes you considered and passed on, not a paragraph per item. The exception is an entry that is a *real* problem with no rule to land on: that one keeps its own bullet, since diff --git a/plugins/fd3/CHANGELOG.md b/plugins/fd3/CHANGELOG.md index 688a27f..64e1b3a 100644 --- a/plugins/fd3/CHANGELOG.md +++ b/plugins/fd3/CHANGELOG.md @@ -7,6 +7,85 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- `split-to-tasks` turns each declared gap — a `deferred` claim with an owner and a placement — + into an operational task naming its owner, instead of stopping the split; a `blocked` claim, + which nothing owns, still stops it +- `split-to-tasks` shows the full six-column table in its reply, `elements` included +- `split-to-tasks` and `implement-tasks` make their first tool call in the same reply as the opening + checklist — a reply that only announced the checklist ended a headless run with nothing done +- `grill-topic` posts a round's unblocked questions while a lookup runs, instead of holding the + whole round and ending the turn on "waiting" + +### Added + +- `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 +- The spec template caps every table cell at two sentences and sends longer evidence to + `evidence/
.md`, with a per-pass overflow file for a validation block that outgrows the + appendix +- `split-to-tasks` cuts a protected path — one guarded by `CODEOWNERS`, branch protection, or a + required review — into a delivery task on its own branch, so one external approval no longer + holds a whole landing unit +- A `depends-on` edge between two tasks on the same branch must name the file or symbol they + share, in the dependent task's `## Note` and in the report; an edge that cannot be named is + dropped, because it only serialises the implementation stage +- Evals — four scenarios covering the defects this round fixed: a split over a spec whose + `ready` verdict carries a declared gap, a split over a `CODEOWNERS`-protected path, a + validation of an ownerless gap, and a grilling run whose two bookkeeping files must land in + `notes/` and `research/` + +### Fixed + +- 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 + verification rows are run by the repositories' checks and raise no task +- A `ready` verdict no longer hides an ownerless gap: `validate-spec` returns `ready` only when + every claim is `verified` or `deferred`, and asking the user for an owner is the last move + before a claim is recorded `blocked` +- `build-spec` re-invokes validation when the previous pass edited the spec, not only when the + verdict was not ready — a `ready` verdict on a document that same pass rewrote judged the + version before those edits — and each re-invocation carries a focus list of open findings and + edited sections +- The validation return prints all twelve check rows with their fixed numbering and short names, + and the report reference says to re-read it after a compaction +- A pass that hands questions up sends its report with them — the twelve rows, the findings it + already holds and a `not ready` verdict, with the handed-up items under *Still open* — instead + of promising the table once answers land that may never come +- `validate-spec` edits only what a finding of that pass names: on a spec whose checks all pass it + leaves the file byte-identical apart from the appended evidence block +- A HIL CI failure is diagnosed by running the failing check in the branch's worktree, not by + grepping the source for what the message suggests +- A repair `instructions` line says what to change and never asks the agent to validate — the + workflow runs CI itself, and a second pipeline on the machine is exactly what validation cannot + tolerate +- The closing proposal names each branch's worktree path, whatever the user decides about pushing +- The split reads the spec at its absolute path and never lets the spec's own commit location + decide a branch base — a spec committed on a feature or docs branch no longer roots the stack + there + +- CI verdicts now describe the branch they claim to: the toolchain scout reports each command's + `cwd` relative to the repository root, the CI prompt `cd`s into the worktree and reads + `git branch --show-current`, and `implement-run` / `repair-run` discard a verdict whose branch + is not the unit's — previously an absolute `cwd` sent every command into the repository's main + checkout, so stacked branches were marked done on another branch's code +- A CI runner that edits its way to green no longer produces a pass: regenerating a derived + artifact counts as fixing, the runner returns `git status --porcelain`, and a verdict from a + tree carrying uncommitted changes beyond the task files is discarded as `no-verdict` +- A target branch that is the repository's own checkout is now validated in a detached worktree + at the branch's commit, so the user's uncommitted work and the tasks directory are no longer + part of the tree under test; merges and fixes still happen in the checkout +- The merge and CI prompts no longer claim the task files live outside the repository — they say + 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 + ## [0.1.0] - 2026-09-04 ### Added diff --git a/plugins/fd3/agents/toolchain-scout.md b/plugins/fd3/agents/toolchain-scout.md index 9a90b2a..a8abed6 100644 --- a/plugins/fd3/agents/toolchain-scout.md +++ b/plugins/fd3/agents/toolchain-scout.md @@ -74,7 +74,7 @@ Package manager: — — Validation commands (in order): -1. — cwd: — — source: +1. — cwd: — — source: scoped form: ` or `` placeholder — or "not scopeable: "> 2. ... @@ -86,6 +86,11 @@ Doubts: - ``` +Every `cwd` is **relative to the repository root** — `.`, `backend`, `packages/api`, never the +absolute path of the checkout you inspected. The caller runs your commands in git worktrees of +this repository, so an absolute path sends every command into the checkout you happened to read +and the branch under validation is never exercised. + Order the commands as CI orders them; where CI is silent, install → build → typecheck → lint → unit tests. Always include the install command, marked "required in a fresh worktree": the caller runs these commands in git worktrees, which share nothing installed — `node_modules` and diff --git a/plugins/fd3/commands/build-spec.md b/plugins/fd3/commands/build-spec.md index 613e536..a3a6912 100644 --- a/plugins/fd3/commands/build-spec.md +++ b/plugins/fd3/commands/build-spec.md @@ -7,6 +7,6 @@ Three stages, in order. The grilling runs here, in the main thread — it needs 1. Invoke the `fd3:grill-topic` skill with `$ARGUMENTS`, forwarded verbatim. Work its rounds to the end. While its lookups run, speak only when there is something to decide — a round ready to post, a returning fact that voids a question already asked, or a command the user must run; a lookup that came back and changed nothing gets one line. 2. Once the user confirms the closing summary — and not before — invoke the `fd3:write-spec` skill with the path of the closing-notes file the grilling wrote, the session's research directory, and where the spec goes. The paths, never a description of where things are. It runs in its own context; do not wrap it in a sub-agent of your own. Relay its report: where the spec went, the counts, what is not yet settled in it. If it stops to ask instead, put its questions to the user here, batched per `${CLAUDE_PLUGIN_ROOT}/references/question-batching.md`, and send the answers back with `SendMessage` — it resumes where it stopped. -3. Then invoke the `fd3:validate-spec` skill on the written spec. It runs in its own context and returns its verdict and status. Validation is also where the spec's defects get repaired: the skill edits the spec wherever a fact settles one, so its report lists spec edits alongside findings. A verdict on a spec that had findings and lists no edits is a pass that only counted them — say so when you relay it. While the verdict is not ready and a further pass could close what remains, invoke it again on the same spec, passing back the status it returned — at most three passes in all, and say plainly what is still open if the third ends short. A pass that stopped to ask is not finished: resume it with `SendMessage`, never with a fresh invocation. A pass that ended its own turn with a verdict is finished, and the next invocation is a fresh reading of a document the previous pass edited — the only thing that ever re-checks those edits, so do not skip it because the verdict looks close. If it stops to ask instead, put its questions to the user here, batched per `${CLAUDE_PLUGIN_ROOT}/references/question-batching.md`, and send the answers back with `SendMessage` — it resumes where it stopped. Relay the final verdict, and relay every finding the status leaves standing inside a passing check. +3. Then invoke the `fd3:validate-spec` skill on the written spec. It runs in its own context and returns its verdict and status. Validation is also where the spec's defects get repaired: the skill edits the spec wherever a fact settles one, so its report lists spec edits alongside findings. A verdict on a spec that had findings and lists no edits is a pass that only counted them — say so when you relay it. Invoke it again on the same spec, passing back the status it returned, while either holds: the verdict is not ready and a further pass could close what remains, or the pass applied spec edits — a `ready` verdict on a document the same pass rewrote is a verdict on the version before those edits, and nothing but the next pass ever reads them. At most three passes in all, and say plainly what is still open if the third ends short. Each re-invocation carries a **focus list** alongside the status: the findings the previous pass left open and the sections it edited. That is what the next pass spends itself on; everything the status records as verified it inherits. A pass that stopped to ask is not finished: resume it with `SendMessage`, never with a fresh invocation. A pass that ended its own turn with a verdict is finished, and the next invocation is a fresh reading of a document the previous pass edited — the only thing that ever re-checks those edits, so do not skip it because the verdict looks close. If it stops to ask instead, put its questions to the user here, batched per `${CLAUDE_PLUGIN_ROOT}/references/question-batching.md`, and send the answers back with `SendMessage` — it resumes where it stopped. Relay the final verdict, and relay every finding the status leaves standing inside a passing check. Confirmation of shared understanding is the gate between stages 1 and 2. If the user ends the session without confirming, stop after the first stage and say what is still open. diff --git a/plugins/fd3/evals/fixtures/clean-payments-spec/DEFECTS.md b/plugins/fd3/evals/fixtures/clean-payments-spec/DEFECTS.md index a0e4fe6..49ee0fc 100644 --- a/plugins/fd3/evals/fixtures/clean-payments-spec/DEFECTS.md +++ b/plugins/fd3/evals/fixtures/clean-payments-spec/DEFECTS.md @@ -40,5 +40,9 @@ Load-bearing facts: - No prerequisite is marked "met" on something outside the tree. The deploy manifest and `DATABASE_URL` do not exist here, so that prerequisite is an assertion with an owner and a placement. +- Each section 8 row says **where** its check runs — DB-1 in the deployed environment after the + phase 1 deploy, OBSERVABILITY-1 on the branch. Without that, DB-1's `psql "$DATABASE_URL"` probe + reads as a branch-level criterion no branch can pass here, and a validation pass legitimately + repairs the row — which breaks the append-only assertion below. - The only spec edits the eval accepts are appended evidence rows under a dated sub-heading: the assertion checks the fixture content is a prefix of the sandbox content. diff --git a/plugins/fd3/evals/fixtures/clean-payments-spec/spec/clean-spec.md b/plugins/fd3/evals/fixtures/clean-payments-spec/spec/clean-spec.md index ba79718..b49c3da 100644 --- a/plugins/fd3/evals/fixtures/clean-payments-spec/spec/clean-spec.md +++ b/plugins/fd3/evals/fixtures/clean-payments-spec/spec/clean-spec.md @@ -122,10 +122,13 @@ and nothing in this spec drops it — section 9 says so, and rollback does not c ## 8. Verification -- **DB-1** — probe: `psql "$DATABASE_URL" -c "\d idempotency_keys"` lists the four columns. Before +- **DB-1** — probe, run in the single deployed environment after the phase 1 deploy (section 7) + and not on the branch, since this repository has no database and reads no environment variable: + `psql "$DATABASE_URL" -c "\d idempotency_keys"` lists the four columns. Before the change the same command errors with `did not find any relation`. -- **OBSERVABILITY-1** — triggered, through `npm test` (`vitest run`, the script `package.json` - defines): call `deliver` on two events, then assert `deliveryMetrics()` returns `delivered: 2`. +- **OBSERVABILITY-1** — triggered, on the branch, through `npm test` (`vitest run`, the script + `package.json` defines): call `deliver` on two events, then assert `deliveryMetrics()` returns + `delivered: 2`. Before the change the export does not exist. The `failed` count is not exercised — nothing here can make a delivery fail (section 10). diff --git a/plugins/fd3/evals/fixtures/defective-payments-spec/.github/workflows/deploy.yml b/plugins/fd3/evals/fixtures/defective-payments-spec/.github/workflows/deploy.yml new file mode 100644 index 0000000..4de4536 --- /dev/null +++ b/plugins/fd3/evals/fixtures/defective-payments-spec/.github/workflows/deploy.yml @@ -0,0 +1,16 @@ +name: deploy + +on: + push: + branches: [main] + +jobs: + deploy: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - run: npm ci && npm test + - run: npx node-pg-migrate up + env: + DATABASE_URL: ${{ secrets.DATABASE_URL }} + - run: kubectl apply -f deploy/manifest.yaml diff --git a/plugins/fd3/evals/fixtures/defective-payments-spec/DEFECTS.md b/plugins/fd3/evals/fixtures/defective-payments-spec/DEFECTS.md index f7bfa9a..9f81177 100644 --- a/plugins/fd3/evals/fixtures/defective-payments-spec/DEFECTS.md +++ b/plugins/fd3/evals/fixtures/defective-payments-spec/DEFECTS.md @@ -33,6 +33,19 @@ without re-checking this list breaks the eval. - `src/store/idempotency.ts` — `new Map` at line 3; functions at lines 5 and 9. - `src/webhooks/enqueue.ts` — `enqueueWebhook` at line 8. +## The repository backs every claim the spec does not plant as a defect + +A run that finds nothing wrong with the prerequisites, the apply mechanism or the delivery path +must be right to do so, or the five defects drown in real findings. So the repository carries: +`DATABASE_URL`, the merchant key and the webhook URL in `deploy/manifest.yaml`, plus its scrape +annotations for `/metrics`; the orders schema (`migrations/0001_create_orders.sql`, +`src/orders/repository.ts`) and the `pg` client; `src/server.ts` serving `POST /charges` behind +`src/http/merchantAuth.ts` (402 on decline) and `GET /metrics`; `src/queue/poller.ts` draining +the queue into `deliver`; a `post` that really calls the merchant endpoint, so a failing merchant is +reachable; `fixtures/charge.json` for the API-1 probe; and `.github/workflows/deploy.yml`, which +migrates and deploys on merge to `main`. Removing any of these turns a clean claim into a sixth +finding. + Everything else in the spec is deliberately clean: all other citations resolve, the decision table is otherwise consistent, every other element carries a code, the evidence table exists and its other rows are true. Section 3 carries a **risks accepted** table and section 7's rollout table diff --git a/plugins/fd3/evals/fixtures/defective-payments-spec/README.md b/plugins/fd3/evals/fixtures/defective-payments-spec/README.md index 1fe4ce4..c903487 100644 --- a/plugins/fd3/evals/fixtures/defective-payments-spec/README.md +++ b/plugins/fd3/evals/fixtures/defective-payments-spec/README.md @@ -2,7 +2,10 @@ Charges cards and delivers `charge.settled` webhooks to merchants. +- `src/server.ts` — HTTP entry point: `POST /charges` behind the merchant API key, `GET /metrics` - `src/billing/` — charge entry point and idempotency handling - `src/webhooks/` — event enqueueing -- `src/queue/` — the delivery worker +- `src/queue/` — the delivery worker and the poller that drains the queue into it - `src/store/` — idempotency key storage (in-memory today) +- `src/orders/`, `src/db.ts`, `migrations/` — the orders schema in Postgres +- `deploy/manifest.yaml`, `.github/workflows/deploy.yml` — CI migrates and deploys on merge to `main` diff --git a/plugins/fd3/evals/fixtures/defective-payments-spec/deploy/manifest.yaml b/plugins/fd3/evals/fixtures/defective-payments-spec/deploy/manifest.yaml new file mode 100644 index 0000000..92ae0fd --- /dev/null +++ b/plugins/fd3/evals/fixtures/defective-payments-spec/deploy/manifest.yaml @@ -0,0 +1,33 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: payments-service +spec: + replicas: 2 + selector: + matchLabels: + app: payments-service + template: + metadata: + labels: + app: payments-service + annotations: + prometheus.io/scrape: "true" + prometheus.io/path: /metrics + prometheus.io/port: "3000" + spec: + containers: + - name: payments-service + image: registry.internal/payments-service:latest + ports: + - containerPort: 3000 + env: + - name: DATABASE_URL + valueFrom: + secretKeyRef: { name: payments-db, key: url } + - name: MERCHANT_API_KEY + valueFrom: + secretKeyRef: { name: payments-merchant, key: api-key } + - name: MERCHANT_WEBHOOK_URL + valueFrom: + configMapKeyRef: { name: payments-config, key: merchant-webhook-url } diff --git a/plugins/fd3/evals/fixtures/defective-payments-spec/fixtures/charge.json b/plugins/fd3/evals/fixtures/defective-payments-spec/fixtures/charge.json new file mode 100644 index 0000000..dc413dc --- /dev/null +++ b/plugins/fd3/evals/fixtures/defective-payments-spec/fixtures/charge.json @@ -0,0 +1 @@ +{ "orderId": "o_1", "amountMinor": 1200, "currency": "EUR" } diff --git a/plugins/fd3/evals/fixtures/defective-payments-spec/migrations/0001_create_orders.sql b/plugins/fd3/evals/fixtures/defective-payments-spec/migrations/0001_create_orders.sql new file mode 100644 index 0000000..33cb239 --- /dev/null +++ b/plugins/fd3/evals/fixtures/defective-payments-spec/migrations/0001_create_orders.sql @@ -0,0 +1,5 @@ +CREATE TABLE orders ( + id TEXT PRIMARY KEY, + amount_minor BIGINT NOT NULL, + created_at TIMESTAMPTZ NOT NULL DEFAULT now() +); diff --git a/plugins/fd3/evals/fixtures/defective-payments-spec/package.json b/plugins/fd3/evals/fixtures/defective-payments-spec/package.json index 6f439aa..6bec7fb 100644 --- a/plugins/fd3/evals/fixtures/defective-payments-spec/package.json +++ b/plugins/fd3/evals/fixtures/defective-payments-spec/package.json @@ -3,6 +3,14 @@ "version": "1.4.2", "private": true, "scripts": { + "start": "node dist/server.js", "test": "vitest run" + }, + "dependencies": { + "pg": "^8.12.0" + }, + "devDependencies": { + "node-pg-migrate": "^7.6.0", + "vitest": "^2.1.0" } } diff --git a/plugins/fd3/evals/fixtures/defective-payments-spec/spec/payments-spec.md b/plugins/fd3/evals/fixtures/defective-payments-spec/spec/payments-spec.md index 1a7f4ad..681386a 100644 --- a/plugins/fd3/evals/fixtures/defective-payments-spec/spec/payments-spec.md +++ b/plugins/fd3/evals/fixtures/defective-payments-spec/spec/payments-spec.md @@ -127,7 +127,7 @@ dropping it is cleanup (section 9), not rollback. - **DB-1** — probe: `psql "$DATABASE_URL" -c "\d idempotency_keys"` lists the four columns. Before the change the same command errors with `did not find any relation`. -- **API-1** — probe: `curl -s -X POST localhost:3000/charges -d @fixtures/charge.json | jq .deliveryStatus` +- **API-1** — probe: `curl -s -X POST -H "x-api-key: $MERCHANT_API_KEY" localhost:3000/charges -d @fixtures/charge.json | jq .deliveryStatus` prints `"queued"`. Before the change it prints `null`. - **Delivery retry worker** — triggered: post a charge with the mock merchant endpoint returning 500; the outcome table gains 5 rows for the event, `delivered = false` on each. diff --git a/plugins/fd3/evals/fixtures/defective-payments-spec/src/db.ts b/plugins/fd3/evals/fixtures/defective-payments-spec/src/db.ts new file mode 100644 index 0000000..7446fda --- /dev/null +++ b/plugins/fd3/evals/fixtures/defective-payments-spec/src/db.ts @@ -0,0 +1,3 @@ +import { Pool } from "pg"; + +export const pool = new Pool({ connectionString: process.env.DATABASE_URL }); diff --git a/plugins/fd3/evals/fixtures/defective-payments-spec/src/http/merchantAuth.ts b/plugins/fd3/evals/fixtures/defective-payments-spec/src/http/merchantAuth.ts new file mode 100644 index 0000000..caa3e1b --- /dev/null +++ b/plugins/fd3/evals/fixtures/defective-payments-spec/src/http/merchantAuth.ts @@ -0,0 +1,6 @@ +import type { IncomingMessage } from "node:http"; + +export function isMerchantAuthorized(req: IncomingMessage): boolean { + const key = req.headers["x-api-key"]; + return typeof key === "string" && key === process.env.MERCHANT_API_KEY; +} diff --git a/plugins/fd3/evals/fixtures/defective-payments-spec/src/metrics.ts b/plugins/fd3/evals/fixtures/defective-payments-spec/src/metrics.ts new file mode 100644 index 0000000..63e73a3 --- /dev/null +++ b/plugins/fd3/evals/fixtures/defective-payments-spec/src/metrics.ts @@ -0,0 +1,9 @@ +let chargesTotal = 0; + +export function countCharge(): void { + chargesTotal += 1; +} + +export function renderMetrics(): string { + return `# TYPE charges_total counter\ncharges_total ${chargesTotal}\n`; +} diff --git a/plugins/fd3/evals/fixtures/defective-payments-spec/src/orders/repository.ts b/plugins/fd3/evals/fixtures/defective-payments-spec/src/orders/repository.ts new file mode 100644 index 0000000..4737d9c --- /dev/null +++ b/plugins/fd3/evals/fixtures/defective-payments-spec/src/orders/repository.ts @@ -0,0 +1,6 @@ +import { pool } from "../db"; + +export async function findOrder(orderId: string) { + const { rows } = await pool.query("SELECT id, amount_minor FROM orders WHERE id = $1", [orderId]); + return rows[0]; +} diff --git a/plugins/fd3/evals/fixtures/defective-payments-spec/src/queue/poller.ts b/plugins/fd3/evals/fixtures/defective-payments-spec/src/queue/poller.ts new file mode 100644 index 0000000..a9798a5 --- /dev/null +++ b/plugins/fd3/evals/fixtures/defective-payments-spec/src/queue/poller.ts @@ -0,0 +1,12 @@ +import { drainQueue } from "../webhooks/enqueue"; +import { deliver } from "./worker"; + +const POLL_INTERVAL_MS = 1000; + +export function startDeliveryPoller(): NodeJS.Timeout { + return setInterval(async () => { + for (const event of drainQueue()) { + await deliver(event); + } + }, POLL_INTERVAL_MS); +} diff --git a/plugins/fd3/evals/fixtures/defective-payments-spec/src/queue/worker.ts b/plugins/fd3/evals/fixtures/defective-payments-spec/src/queue/worker.ts index fe0fb22..42efc3b 100644 --- a/plugins/fd3/evals/fixtures/defective-payments-spec/src/queue/worker.ts +++ b/plugins/fd3/evals/fixtures/defective-payments-spec/src/queue/worker.ts @@ -11,6 +11,11 @@ export async function deliver(event: WebhookEvent): Promise { } } -async function post(_event: WebhookEvent): Promise { - return true; +async function post(event: WebhookEvent): Promise { + const res = await fetch(process.env.MERCHANT_WEBHOOK_URL ?? "", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(event), + }); + return res.ok; } diff --git a/plugins/fd3/evals/fixtures/defective-payments-spec/src/server.ts b/plugins/fd3/evals/fixtures/defective-payments-spec/src/server.ts new file mode 100644 index 0000000..b9505f7 --- /dev/null +++ b/plugins/fd3/evals/fixtures/defective-payments-spec/src/server.ts @@ -0,0 +1,29 @@ +import { createServer } from "node:http"; +import { charge, type ChargeRequest } from "./billing/charge"; +import { isMerchantAuthorized } from "./http/merchantAuth"; +import { countCharge, renderMetrics } from "./metrics"; +import { startDeliveryPoller } from "./queue/poller"; + +const server = createServer(async (req, res) => { + if (req.method === "GET" && req.url === "/metrics") { + res.writeHead(200, { "content-type": "text/plain" }).end(renderMetrics()); + return; + } + if (req.method === "POST" && req.url === "/charges") { + if (!isMerchantAuthorized(req)) { + res.writeHead(401).end(); + return; + } + let body = ""; + for await (const chunk of req) body += chunk; + const result = await charge(JSON.parse(body) as ChargeRequest); + countCharge(); + res.writeHead(result.status === "declined" ? 402 : 200, { "content-type": "application/json" }); + res.end(JSON.stringify(result)); + return; + } + res.writeHead(404).end(); +}); + +startDeliveryPoller(); +server.listen(Number(process.env.PORT ?? 3000)); diff --git a/plugins/fd3/evals/fixtures/defective-payments-spec/src/webhooks/enqueue.ts b/plugins/fd3/evals/fixtures/defective-payments-spec/src/webhooks/enqueue.ts index 2bf2336..fd2d828 100644 --- a/plugins/fd3/evals/fixtures/defective-payments-spec/src/webhooks/enqueue.ts +++ b/plugins/fd3/evals/fixtures/defective-payments-spec/src/webhooks/enqueue.ts @@ -8,3 +8,7 @@ const queue: WebhookEvent[] = []; export async function enqueueWebhook(type: string, payload: unknown): Promise { queue.push({ type, payload }); } + +export function drainQueue(): WebhookEvent[] { + return queue.splice(0); +} diff --git a/plugins/fd3/evals/fixtures/gap-rollout-spec/DEFECTS.md b/plugins/fd3/evals/fixtures/gap-rollout-spec/DEFECTS.md new file mode 100644 index 0000000..caa0871 --- /dev/null +++ b/plugins/fd3/evals/fixtures/gap-rollout-spec/DEFECTS.md @@ -0,0 +1,41 @@ +# gap-rollout-spec — fixture contract + +This file is fixture documentation only. `reset-sandboxes.sh` excludes it from the sandbox copy. +`rollout-spec` with one change: its last verdict line carries a deferred claim — a gap the spec +itself declares with an owner and a placement. It serves split-declared-gap. + +## The declared gap + +Section 7 names the unmeasured ledger write ceiling, its owner (**the platform team**) and its +placement (**a gate before phase 2**). Section 12's `### Validation pass — 2026-07-30` block counts +it, so the verdict line reads: + +`Verdict: ready — claims: 1 verified / 1 deferred / 0 blocked — spec 224 lines at this verdict` + +All three halves are load-bearing. `ready` with a deferred claim is what the precondition must +accept and turn into an operational task; the owner and the placement are what make it a declared +gap rather than a stop; and `224` equals `wc -l` on the spec, so any edit to the file must be +followed by rewriting the number. +Removing the owner from section 7 turns this fixture into a stop-before-step-1 case and breaks the +scenario — that case has its own fixture, `ownerless-gap-payments-spec`, on the validate side. + +## The 7-task split + +The six delivery tasks of `rollout-spec` (see that fixture's DEFECTS.md for the frozen cut, the +element→owner map and the sentinel strings, which are unchanged here) **plus one operational task** +for the gap: `repository: none`, no branch, no element code, and a `## Note` naming the platform +team and the phase-2 gate. Seven task files, exactly one of them operational. + +## Precondition material + +Unlike `rollout-spec`, the spec is read-only in this scenario: the split writes task files and the +split report beside the spec (`spec/gap-rollout-spec.split.md`) and modifies nothing. + +## Load-bearing line numbers + +Identical to `rollout-spec`: + +- `repo-a/services/checkout/src/api/charge.ts:6` — `postCharge` +- `repo-a/services/checkout/src/config.ts:2` — `asyncSettlement: false` +- `repo-a/services/ledger/src/api/entries.ts:7` — `listEntries` +- `repo-b/src/components/PaymentStatus.tsx:5` — `PaymentStatus` diff --git a/plugins/fd3/evals/fixtures/gap-rollout-spec/repo-a/README.md b/plugins/fd3/evals/fixtures/gap-rollout-spec/repo-a/README.md new file mode 100644 index 0000000..54e96bf --- /dev/null +++ b/plugins/fd3/evals/fixtures/gap-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/gap-rollout-spec/repo-a/services/checkout/src/api/charge.ts b/plugins/fd3/evals/fixtures/gap-rollout-spec/repo-a/services/checkout/src/api/charge.ts new file mode 100644 index 0000000..cd4b9c7 --- /dev/null +++ b/plugins/fd3/evals/fixtures/gap-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/gap-rollout-spec/repo-a/services/checkout/src/config.ts b/plugins/fd3/evals/fixtures/gap-rollout-spec/repo-a/services/checkout/src/config.ts new file mode 100644 index 0000000..96692a0 --- /dev/null +++ b/plugins/fd3/evals/fixtures/gap-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/gap-rollout-spec/repo-a/services/ledger/migrations/README.md b/plugins/fd3/evals/fixtures/gap-rollout-spec/repo-a/services/ledger/migrations/README.md new file mode 100644 index 0000000..4594af4 --- /dev/null +++ b/plugins/fd3/evals/fixtures/gap-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/gap-rollout-spec/repo-a/services/ledger/src/api/entries.ts b/plugins/fd3/evals/fixtures/gap-rollout-spec/repo-a/services/ledger/src/api/entries.ts new file mode 100644 index 0000000..3312e13 --- /dev/null +++ b/plugins/fd3/evals/fixtures/gap-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/gap-rollout-spec/repo-b/README.md b/plugins/fd3/evals/fixtures/gap-rollout-spec/repo-b/README.md new file mode 100644 index 0000000..ca87206 --- /dev/null +++ b/plugins/fd3/evals/fixtures/gap-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/gap-rollout-spec/repo-b/src/components/PaymentStatus.tsx b/plugins/fd3/evals/fixtures/gap-rollout-spec/repo-b/src/components/PaymentStatus.tsx new file mode 100644 index 0000000..155abe8 --- /dev/null +++ b/plugins/fd3/evals/fixtures/gap-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/gap-rollout-spec/spec/gap-rollout-spec.md b/plugins/fd3/evals/fixtures/gap-rollout-spec/spec/gap-rollout-spec.md new file mode 100644 index 0000000..30a8fc9 --- /dev/null +++ b/plugins/fd3/evals/fixtures/gap-rollout-spec/spec/gap-rollout-spec.md @@ -0,0 +1,224 @@ +# 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. + +**Declared gap — the production write ceiling of the ledger database is unconfirmed.** Nobody in +this session could establish how many settlement writes per second the shared staging instance +sustains before the ledger's connection pool saturates, and no measurement exists. Owner: the +platform team. Placement: a gate before phase 2 — the flag flip in CONFIG-1 is what puts real +write volume on the table, so the ceiling must be measured and recorded before that phase starts. +The phase-1 elements are unaffected: they land dark and write nothing. + +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 / 1 deferred / 0 blocked — spec 224 lines at this verdict + +| 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 | +| The ledger write ceiling is unmeasured | no measurement exists; declared as a gap in section 7 with the platform team as owner and a gate before phase 2 as placement | +| Verdict | phase 1: yes; phase 2: yes, behind the declared gap's gate — spec is ready to split | diff --git a/plugins/fd3/evals/fixtures/orphan-rollout-spec/spec/rollout-spec.md b/plugins/fd3/evals/fixtures/orphan-rollout-spec/spec/rollout-spec.md index 0ad40be..9625432 100644 --- a/plugins/fd3/evals/fixtures/orphan-rollout-spec/spec/rollout-spec.md +++ b/plugins/fd3/evals/fixtures/orphan-rollout-spec/spec/rollout-spec.md @@ -47,15 +47,15 @@ New migration `repo-a/services/ledger/migrations/0001_create_ledger_entries.sql` - 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 endpoint (ledger service) +### API-2 — ledger entries endpoints (ledger service) -`GET /ledger/entries?orderId=` replaces the stub at `repo-a/services/ledger/src/api/entries.ts:7`. +`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: `orderId` query parameter, required, non-empty string. -- Response: JSON array of `{ orderId: string, amountMinor: number, direction: "debit" | "credit", createdAt: string }`. -- Errors: 400 on a missing or empty `orderId`; 200 with `[]` when no entries exist. +- 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: response capped at 500 entries, newest first. +- Limits: `GET` responses capped at 500 entries, newest first; `POST` writes exactly one row. ### API-3 — ledger entry export (ledger service) @@ -70,8 +70,8 @@ New migration `repo-a/services/ledger/migrations/0001_create_ledger_entries.sql` ### 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 via the ledger -service's internal write endpoint. +`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 @@ -133,7 +133,7 @@ request. 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. + the real query and the 400 guard, and add the `POST` handler beside it. ### repo-a — `services/checkout/` (team-checkout) @@ -177,7 +177,7 @@ the migration stays behind, unused (expand-only; removal is out of scope, LED-10 - **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. Before the change both return the stub's empty 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 diff --git a/plugins/fd3/evals/fixtures/ownerless-gap-payments-spec/DEFECTS.md b/plugins/fd3/evals/fixtures/ownerless-gap-payments-spec/DEFECTS.md new file mode 100644 index 0000000..51345ef --- /dev/null +++ b/plugins/fd3/evals/fixtures/ownerless-gap-payments-spec/DEFECTS.md @@ -0,0 +1,36 @@ +# ownerless-gap-payments-spec — fixture contract + +This file is fixture documentation only. `reset-sandboxes.sh` excludes it from the sandbox copy. +`gap-payments-spec` with the gap's owner and placement removed. It serves validate-ownerless-gap. + +## The unowned gap + +The same fact is unconfirmed — the platform egress rate-limit ceiling behind D5, CONFIG-1 and the +phase-2 gate — but the spec places it nowhere and names nobody: + +- section 4's sub-heading reads **`### Open question`**, not `### Declared gap`, and its paragraph + says nobody could name who sets the ceiling or who would confirm it; +- section 7's `Gate after?` cell and hard-dependency line say phase 2 waits on *a confirmation + nobody owns*; +- the section 12 evidence row marks it the same way. + +Restore an owner or a placement in any of the three and the claim becomes `deferred` on sight, which +is the other fixture's scenario (`gap-payments-spec`), not this one. + +The validate-ownerless-gap assertions require: + +- the claim reaches the report — under `## Blocked`, or under `## Deferred` only with an owner and a + placement that an answer supplied, never a stand-in like "nobody" or "TBD"; +- a `## Blocked` claim lowers the verdict to `not ready`; +- the owned out-of-scope items of section 10 (refund webhooks, invoice PDF rendering, the wiring) + never turn up as blocked — they name a team and a ticket. + +With `ask_user_question: first_option` step 4's ownership question is auto-answered, so both +outcomes are within contract; what is not is a deferred entry with no owner behind it. + +## Everything else + +Unchanged from `gap-payments-spec` — read that fixture's DEFECTS.md for the tree's facts the spec +must keep declaring (no caller for `deliver`, `post` returns `true` unconditionally, no HTTP surface, +no manifest), the risks-accepted table, the five-column rollout table and the `Limits:` bullet that +keeps section 9 consistent. Source files are byte-identical to `defective-payments-spec`'s. diff --git a/plugins/fd3/evals/fixtures/ownerless-gap-payments-spec/README.md b/plugins/fd3/evals/fixtures/ownerless-gap-payments-spec/README.md new file mode 100644 index 0000000..1fe4ce4 --- /dev/null +++ b/plugins/fd3/evals/fixtures/ownerless-gap-payments-spec/README.md @@ -0,0 +1,8 @@ +# payments-service + +Charges cards and delivers `charge.settled` webhooks to merchants. + +- `src/billing/` — charge entry point and idempotency handling +- `src/webhooks/` — event enqueueing +- `src/queue/` — the delivery worker +- `src/store/` — idempotency key storage (in-memory today) diff --git a/plugins/fd3/evals/fixtures/ownerless-gap-payments-spec/package.json b/plugins/fd3/evals/fixtures/ownerless-gap-payments-spec/package.json new file mode 100644 index 0000000..6f439aa --- /dev/null +++ b/plugins/fd3/evals/fixtures/ownerless-gap-payments-spec/package.json @@ -0,0 +1,8 @@ +{ + "name": "payments-service", + "version": "1.4.2", + "private": true, + "scripts": { + "test": "vitest run" + } +} diff --git a/plugins/fd3/evals/fixtures/ownerless-gap-payments-spec/spec/ownerless-gap-spec.md b/plugins/fd3/evals/fixtures/ownerless-gap-payments-spec/spec/ownerless-gap-spec.md new file mode 100644 index 0000000..6554828 --- /dev/null +++ b/plugins/fd3/evals/fixtures/ownerless-gap-payments-spec/spec/ownerless-gap-spec.md @@ -0,0 +1,208 @@ +# Webhook delivery hardening — SPEC + +**What changes:** charge processing becomes durably idempotent, `charge.settled` webhook delivery +gains outcome metrics, and the rate at which the process issues delivery attempts gains a +configurable cap that is raised once the platform egress ceiling allows. + +- Ticket: PAY-231 +- Status: ready for validation +- Date: 2026-07-25 + +This spec supersedes nothing; there are no companion documents. + +## 2. Problem and goal + +Idempotency keys live in process memory (`src/store/idempotency.ts:3`), so a restart forgets every +processed order and a redelivered request charges the card twice. Webhook delivery retries exist +(`src/queue/worker.ts:3`) but nothing records delivery outcomes, so a failed delivery is invisible. +The retry loop also issues its attempts back to back with no delay (`src/queue/worker.ts:6`), and +nothing coordinates concurrent `deliver` calls, so nothing in the module bounds the attempts it +sends. The module is not wired in yet either — `deliver` has no caller and nothing imports +`src/queue/worker.ts` — so the ceiling has to be in place before it is, not after (wiring it is out +of scope; section 10). + +Goal: a redelivered charge request never charges twice across restarts, every delivery the worker +attempts is counted, and the attempts it issues stay under a ceiling that can be raised without a +code change. + +## 3. Design decisions + +| # | Decision | Rationale | +|---|---|---| +| D1 | **Charge processing stays idempotent per `orderId`** | The lookup-before-charge shape already exists (`src/billing/charge.ts:16`); this spec only makes the store durable. Cost accepted: one storage dependency where today there is none. | +| D2 | **Idempotency keys are stored in Postgres** | The service already holds a `DATABASE_URL` and the orders schema lives there; Redis would add a second stateful dependency for the same guarantee. Cost accepted: key reads join the existing database's load. | +| D3 | **Delivery retries stay capped at 5 attempts** | The cap already exists (`src/queue/worker.ts:3`) and no incident has needed more. Raising it would only delay surfacing a dead merchant endpoint. | +| D4 | **Webhook processing stays queued, off the request path** | Delivery lives in its own module (`src/queue/worker.ts:6`) and `charge` only enqueues (`src/billing/charge.ts:21`); moving delivery into the request path would put merchant endpoint latency on the charge response. Cost accepted: the merchant learns the outcome asynchronously. | +| D5 | **One process-wide cap on attempts per second, set by config and raised in phase 2** | The retry loop applies no delay (`src/queue/worker.ts:6`) and concurrent `deliver` calls do not see each other, so a per-event delay would not bound what the process sends; the cap has to be shared module state. A config value moves it without a deploy of new code. `8` per second is the largest value that stays safely under the lowest egress ceiling anyone has quoted, pending a confirmation nobody owns (the open question in section 4). Cost accepted: `1` is slower than the module's uncapped behaviour, and the whole cap reaches only the test suite until the worker is wired in (section 10). | +| D6 | **Delivery outcomes are counted in the process, not scraped** | The repository holds four modules and no HTTP surface, so a scraped endpoint would be a second change with its own contract; an exported counter is verifiable by the test runner `package.json` already names. Cost accepted: the counts are per process and are lost on restart. | +| D7 | **The cap lands before the worker is wired in** | `deliver` has no caller today, so a cap added afterwards would ship a window in which delivery runs uncapped. Cost accepted: both phases are exercised by the test suite rather than by production traffic, so phase 2 opens on a confirmation nobody owns and not on a reading. | + +**Risks accepted** + +| Risk | What it costs if it lands | Mitigation | +|---|---|---| +| `idempotency_keys` grows without bound — nothing in this spec deletes rows, and section 9 adds no cleanup. | Table size grows with order volume, and index maintenance cost rises with it. | One row per order and the primary key as the only index; a retention policy is a later change, not a blocker for this spec. | +| A provider charge that succeeds and whose key write then fails leaves the card charged with no key stored, so a redelivery charges twice. D1 keeps the existing lookup-before-charge order (`src/billing/charge.ts:16`). | One duplicate charge per occurrence, refunded by hand. | The write is retried once and then rethrown as `IdempotencyWriteFailed` rather than swallowed, so the window is visible when it opens. Closing it entirely needs reserve-before-charge, which this spec does not do. | +| Phase 1 caps the module at `1` attempt per second, below its uncapped behaviour, and phase 2 cannot start until the ceiling is confirmed — and the spec names nobody who would confirm it (the open question in section 4). | Whenever the worker is wired in (section 10), a burst takes longer to deliver for as long as the cap sits at `1`; merchants learn outcomes later. | Retries stay capped at 5 attempts (D3), so a slow burst still terminates, and phase 2 raises the cap as soon as the confirmation lands — which is a configuration change, not a release. | +| OBSERVABILITY-1's counts live in process memory (D6) and reset on every restart. | A restart during an incident loses the delivery history up to that point. | The counts are a rate signal, not a ledger: what phase 2's criterion reads is the slope over seconds, which survives any restart that is not mid-burst. | + +## 4. Target architecture + +### DB-1 — durable idempotency key store + +Replaces the in-memory `Map` in `src/store/idempotency.ts`. Contract: + +- Table `idempotency_keys` with columns `order_id TEXT PRIMARY KEY`, `charge_id TEXT NOT NULL`, + `status TEXT NOT NULL`, `created_at TIMESTAMPTZ NOT NULL DEFAULT now()`. +- Reads and writes go through the existing `getIdempotencyKey` / `saveIdempotencyKey` functions; + their signatures do not change. +- Errors: a store read failure throws `IdempotencyStoreUnavailable` out of `getIdempotencyKey`; a + write failure after a successful provider charge is retried once, then logged and rethrown as + `IdempotencyWriteFailed`. Neither is caught inside `charge` (`src/billing/charge.ts:15`), which + has no error path today and gains none here. +- Auth: the service's existing database credentials; no new principal. +- Dependencies: the `pg` client and `node-pg-migrate` for the migration, both new — + `package.json` declares no dependencies today. +- Limits: none beyond the primary key. Rows accumulate; nothing in this spec deletes them. + +### OBSERVABILITY-1 — delivery metrics + +Counter `webhook_delivery_attempts_total`, held in module scope in `src/queue/worker.ts`, +incremented by `deliver` (`src/queue/worker.ts:5`) after each attempt under the outcome that +attempt had, and read through a new exported `deliveryMetrics()`. + +- Fields: two counts, `delivered` and `failed`, returned as one object. +- Errors: incrementing cannot fail; it is an in-process integer add. +- Auth: none — in-process, no new principal. +- Limits: the counts are per process and start at zero on every restart (D6). Only `delivered` is + exercisable here — `post` (`src/queue/worker.ts:14`) returns `true` unconditionally, so nothing in + this repository can produce a failed attempt; a transport that can fail is out of scope + (section 10). + +### CONFIG-1 — delivery attempt rate cap + +New config value `DELIVERY_RATE_LIMIT`: the maximum delivery attempts per second the whole process +issues. One token bucket in module scope in `src/queue/worker.ts`, refilled at that rate and shared +by every `deliver` call; each attempt in the retry loop (`src/queue/worker.ts:6`) takes a token +first and waits when the bucket is empty. `DELIVERY_RATE_LIMIT` is read from `process.env` when the +module loads and held for the life of the process. + +- Fields: one integer environment variable, attempts per second. Default `1`, the phase-1 value. + The bucket holds one token, so there is no burst allowance above the rate. +- Errors: the same module-load read throws `DeliveryRateLimitInvalid` on a value that is not an + integer, is below `1`, or is above `8` — never a silent fallback and never a clamp. The error + carries the rejected value and the bound it broke. +- Auth: none — the environment the service runs under. +- Limits: `8` is the ceiling the read enforces, and phase 2 is what sets the value to it. + +### Open question + +**The platform egress rate-limit ceiling is unconfirmed.** The value `8` in D5 and CONFIG-1 rests +on the lowest quoted ceiling, not on a confirmed number. Nobody could say who sets that ceiling or +who would confirm it, and the spec places the question nowhere. + +### Prerequisites + +| Prerequisite | Status | +|---|---| +| Postgres reachable from the service, with a `DATABASE_URL` in the deploy manifest | asserted by the payments team; not verifiable from this repository, which holds no manifest — owner: payments team, placement: confirmed before the phase-1 deploy | +| Somewhere to set `DELIVERY_RATE_LIMIT` — the service's environment configuration lives outside this repository, which holds no manifest and no CI configuration | asserted by the payments team — owner: payments team, placement: confirmed before the phase-1 deploy, since phase 2 is a change to that configuration and nothing else | + +## 5. Ownership + +| Repository / component | Owns | Apply mechanism | +|---|---|---| +| payments-service (this repository) | everything in this spec | pull request, CI deploy on merge to `main` | + +All paths in this spec are owned by the payments team; review is one approval from that team, and +CI applies the deploy — no human runs anything by hand. + +## 6. The change, per repository + +### payments-service + +1. **DB-1** — new: migration adding `idempotency_keys`, plus rewiring `getIdempotencyKey` / + `saveIdempotencyKey` (`src/store/idempotency.ts:5`, `src/store/idempotency.ts:9`) to the table. +2. **OBSERVABILITY-1** — new: module-scope counts in `src/queue/worker.ts`, incremented inside the + retry loop (`src/queue/worker.ts:6`) once per attempt, plus the exported `deliveryMetrics()`. +3. **CONFIG-1** — new: the module-scope token bucket in `src/queue/worker.ts`, read from + `process.env` at module load, with the retry loop (`src/queue/worker.ts:6`) taking a token per + attempt. +4. **Dependencies** — `package.json` declares none today: `pg` and `node-pg-migrate` for DB-1, and + `vitest` as a dev dependency for the checks in section 8, which the existing `test` script + already invokes. + +Items 1–3 are independent — none reads anything another introduces — so they may land in any order +within the phase; item 4 lands with or before whichever of them needs it. + +## 7. Rollout + +| # | Phase | Where | Switches anything? | Gate after? | +|---|---|---|---|---| +| 1 | All four work items land and deploy together, `DELIVERY_RATE_LIMIT=1` | payments-service | yes — the store becomes durable, the counts start, and the process is capped at 1 attempt per second | yes — phase 2 waits on a ceiling confirmation nobody owns, so phase 1 closes its landing unit: one branch, one pull request | +| 2 | Set `DELIVERY_RATE_LIMIT=8` in the service's environment configuration, which lives outside this repository (section 4 prerequisites) | payments-service | yes — the cap rises to 8 attempts per second | yes — the final phase, so it closes its unit | + +Single environment; each phase is one deploy. Phase 2 opens on a confirmation nobody owns and +nothing else: no production reading gates it, because until the worker is wired in (section 10) the +counts move only under the test suite. + +Hard dependency: **an unowned confirmation of the egress rate-limit ceiling gates phase +2** (the open question in section 4). The gate is a confirmation, not a merge or a deploy. + +Rollback: phase 1 — revert the merge commit and redeploy; the `idempotency_keys` table stays +behind, unused. Phase 2 — set `DELIVERY_RATE_LIMIT` back to `1` and redeploy; the reversal is complete when that +deploy is live. There is no runtime reading to wait for: nothing calls `deliver` in the deployed +process until the worker is wired in (section 10), so the counts stay at zero either way. + +## 8. Verification + +- **DB-1** — probe: `psql "$DATABASE_URL" -c "\d idempotency_keys"` lists the four columns. Before + the change the same command errors with `did not find any relation`. +- **OBSERVABILITY-1** — triggered, through `npm test` (`vitest run`, the script `package.json` + defines): call `deliver` on two events, then assert `deliveryMetrics()` returns `delivered: 2`. + Before the change the export does not exist. The `failed` count is not exercised — nothing here + can make a delivery fail (section 10). +- **CONFIG-1** — triggered, through the same test run: importing the module with + `DELIVERY_RATE_LIMIT=abc` throws `DeliveryRateLimitInvalid`; with `DELIVERY_RATE_LIMIT=1`, three events + delivered concurrently take at least two seconds for their three attempts, which is what a + per-event delay would not produce — each of those events costs one attempt, so only a shared + bucket can space them. + +Phase 1 is verified when the DB-1 probe and the two triggered checks above pass; phase 2 by the same +suite delivering 100 events at up to 8 attempts per second, where phase 1 held the same burst to 1. + +## 9. Cleanup + +The subject has no cleanup: nothing is deleted, and the only irreversible artifact (the +`idempotency_keys` table) is additive. + +## 10. Out of scope + +- **Wiring the worker into the service** — `deliver` has no caller and nothing imports + `src/queue/worker.ts`; owner: payments team, placement: ticket PAY-262. +- **A delivery transport that can fail** — `post` (`src/queue/worker.ts:14`) returns `true` + unconditionally, so no delivery can fail today; owner: payments team, placement: ticket PAY-262, + alongside the wiring that makes it reachable. +- **Refund webhooks** — owner: payments team, placement: ticket PAY-244. +- **Invoice PDF rendering** — owner: billing team, placement: ticket PAY-251. + +## 11. Tickets + +PAY-231 exists and tracks this spec. PAY-244, PAY-251 and PAY-262 exist and hold the four +exclusions. No new tickets are needed. + +## 12. Appendix — the evidence record + +| Claim | How it was verified | +|---|---| +| Idempotency keys are in-memory today | `src/store/idempotency.ts:3` — `const keys = new Map<...>()` | +| Charge processing checks the key before charging | `src/billing/charge.ts:16` — `getIdempotencyKey` called before `callProvider` | +| Delivery retries cap at 5 attempts | `src/queue/worker.ts:3` — `MAX_DELIVERY_ATTEMPTS = 5` | +| The retry loop applies no delay between attempts, and nothing bounds concurrent calls | `src/queue/worker.ts:6` — the `for` loop awaits `post` and retries immediately; no timer, no backoff and no module-scope state in the file | +| The repository holds no HTTP surface, no metrics endpoint and no environment read | `git ls-files` returns 7 files; no server, route, `/metrics` handler or `process.env` access anywhere under `src/` | +| `package.json` declares no dependencies | `package.json` — `scripts.test` only; no `dependencies` and no `devDependencies` block | +| Nothing calls `deliver` and nothing imports the worker module | No `import` of `src/queue/worker` anywhere under `src/`; `deliver` (`src/queue/worker.ts:5`) is exported and unreferenced | +| No delivery can fail today | `src/queue/worker.ts:14` — `post` ignores its argument and returns `true`; it is module-private with no injection seam | +| Webhook enqueueing already exists | `src/webhooks/enqueue.ts:8` — `enqueueWebhook` | +| The store functions are the only key readers/writers | `grep -rn "getIdempotencyKey\|saveIdempotencyKey" src/` — hits only in `src/store/idempotency.ts` and `src/billing/charge.ts` | +| The egress rate-limit ceiling supports 8 attempts per second | Unconfirmed — no documentation states the ceiling, and nobody could be named who would confirm it | diff --git a/plugins/fd3/evals/fixtures/ownerless-gap-payments-spec/src/billing/charge.ts b/plugins/fd3/evals/fixtures/ownerless-gap-payments-spec/src/billing/charge.ts new file mode 100644 index 0000000..ba0bbfe --- /dev/null +++ b/plugins/fd3/evals/fixtures/ownerless-gap-payments-spec/src/billing/charge.ts @@ -0,0 +1,30 @@ +import { getIdempotencyKey, saveIdempotencyKey } from "../store/idempotency"; +import { enqueueWebhook } from "../webhooks/enqueue"; + +export interface ChargeRequest { + orderId: string; + amountMinor: number; + currency: string; +} + +export interface ChargeResult { + chargeId: string; + status: "succeeded" | "declined"; +} + +export async function charge(req: ChargeRequest): Promise { + const existing = await getIdempotencyKey(req.orderId); + if (existing) { + return existing.result; + } + const result = await callProvider(req); + await saveIdempotencyKey(req.orderId, result); + await enqueueWebhook("charge.settled", result); + return result; +} + +async function callProvider(req: ChargeRequest): Promise { + const providerTimeoutMs = 8000; + void providerTimeoutMs; + return { chargeId: `ch_${req.orderId}`, status: "succeeded" }; +} diff --git a/plugins/fd3/evals/fixtures/ownerless-gap-payments-spec/src/queue/worker.ts b/plugins/fd3/evals/fixtures/ownerless-gap-payments-spec/src/queue/worker.ts new file mode 100644 index 0000000..fe0fb22 --- /dev/null +++ b/plugins/fd3/evals/fixtures/ownerless-gap-payments-spec/src/queue/worker.ts @@ -0,0 +1,16 @@ +import type { WebhookEvent } from "../webhooks/enqueue"; + +const MAX_DELIVERY_ATTEMPTS = 5; + +export async function deliver(event: WebhookEvent): Promise { + for (let attempt = 1; attempt <= MAX_DELIVERY_ATTEMPTS; attempt += 1) { + const delivered = await post(event); + if (delivered) { + return; + } + } +} + +async function post(_event: WebhookEvent): Promise { + return true; +} diff --git a/plugins/fd3/evals/fixtures/ownerless-gap-payments-spec/src/store/idempotency.ts b/plugins/fd3/evals/fixtures/ownerless-gap-payments-spec/src/store/idempotency.ts new file mode 100644 index 0000000..bdff893 --- /dev/null +++ b/plugins/fd3/evals/fixtures/ownerless-gap-payments-spec/src/store/idempotency.ts @@ -0,0 +1,11 @@ +import type { ChargeResult } from "../billing/charge"; + +const keys = new Map(); + +export async function getIdempotencyKey(orderId: string) { + return keys.get(orderId); +} + +export async function saveIdempotencyKey(orderId: string, result: ChargeResult) { + keys.set(orderId, { result }); +} diff --git a/plugins/fd3/evals/fixtures/ownerless-gap-payments-spec/src/webhooks/enqueue.ts b/plugins/fd3/evals/fixtures/ownerless-gap-payments-spec/src/webhooks/enqueue.ts new file mode 100644 index 0000000..2bf2336 --- /dev/null +++ b/plugins/fd3/evals/fixtures/ownerless-gap-payments-spec/src/webhooks/enqueue.ts @@ -0,0 +1,10 @@ +export interface WebhookEvent { + type: string; + payload: unknown; +} + +const queue: WebhookEvent[] = []; + +export async function enqueueWebhook(type: string, payload: unknown): Promise { + queue.push({ type, payload }); +} diff --git a/plugins/fd3/evals/fixtures/protected-path-rollout-spec/DEFECTS.md b/plugins/fd3/evals/fixtures/protected-path-rollout-spec/DEFECTS.md new file mode 100644 index 0000000..99ad02c --- /dev/null +++ b/plugins/fd3/evals/fixtures/protected-path-rollout-spec/DEFECTS.md @@ -0,0 +1,53 @@ +# protected-path-rollout-spec — fixture contract + +This file is fixture documentation only. `reset-sandboxes.sh` excludes it from the sandbox copy. +`rollout-spec` plus a seventh element whose path is owned by a team none of the others can stand +in for. It serves split-protected-path. + +## The protected path + +`repo-a/CODEOWNERS` gives `/.github/workflows/` to `@acme/release-team`, and section 5's ownership +table repeats it with the clause that makes it load-bearing: *which no other team can give*. The +new element **CI-1 — migration step in the deploy workflow** edits +`repo-a/.github/workflows/deploy.yml`, which that line covers. + +Delete either the CODEOWNERS line or the ownership row and the cut has nothing to read: the split +then legitimately folds CI-1 into a phase-1 task with the ledger work, and the assert fails on a +split that is within contract. + +## The 7-task split + +The six delivery tasks of `rollout-spec` (see that fixture's DEFECTS.md for the frozen cut, the +element→owner map and the sentinel strings, which are unchanged here) **plus CI-1**, which must: + +- carry CI-1 and nothing else; +- be a delivery task — `repository: repo-a`, never `repository: none`; +- sit in phase 1, where the migration it runs lands; +- come **before DB-1**: section 7 opens phase 1 with "CI-1 first (the migration step must exist + before a migration relies on it)", so the migration task carries exactly one `depends-on` edge + and it points at CI-1. This is the one place this fixture departs from `rollout-spec`, where + DB-1 is the root — hence `checkBoundaries(..., { precedesDb: 'CI-1' })`; +- hold its branch **alone**: no other task may name the same branch, so release-team's approval + gates one pull request rather than the whole phase-1 landing unit. + +Seven task files, no operational task. + +## Precondition material + +Section 12's `### Validation pass — 2026-07-30` block opens with + +`Verdict: ready — claims: 1 verified / 0 deferred / 0 blocked — spec 237 lines at this verdict` + +`237` equals `wc -l` on the spec; any edit to the file must be followed by rewriting the number. +The spec is read-only in this scenario: the split writes task files and the split report beside the +spec (`spec/protected-path-spec.split.md`) and modifies nothing. + +## Load-bearing line numbers + +`rollout-spec`'s four, plus the workflow CI-1 edits: + +- `repo-a/services/checkout/src/api/charge.ts:6` — `postCharge` +- `repo-a/services/checkout/src/config.ts:2` — `asyncSettlement: false` +- `repo-a/services/ledger/src/api/entries.ts:7` — `listEntries` +- `repo-b/src/components/PaymentStatus.tsx:5` — `PaymentStatus` +- `repo-a/.github/workflows/deploy.yml` — checkout then deploy, with no step between them diff --git a/plugins/fd3/evals/fixtures/protected-path-rollout-spec/repo-a/.github/workflows/deploy.yml b/plugins/fd3/evals/fixtures/protected-path-rollout-spec/repo-a/.github/workflows/deploy.yml new file mode 100644 index 0000000..1017121 --- /dev/null +++ b/plugins/fd3/evals/fixtures/protected-path-rollout-spec/repo-a/.github/workflows/deploy.yml @@ -0,0 +1,13 @@ +name: deploy + +on: + push: + branches: [main] + +jobs: + deploy: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: Deploy services + run: ./scripts/deploy.sh diff --git a/plugins/fd3/evals/fixtures/protected-path-rollout-spec/repo-a/CODEOWNERS b/plugins/fd3/evals/fixtures/protected-path-rollout-spec/repo-a/CODEOWNERS new file mode 100644 index 0000000..6b237a9 --- /dev/null +++ b/plugins/fd3/evals/fixtures/protected-path-rollout-spec/repo-a/CODEOWNERS @@ -0,0 +1,3 @@ +/services/ledger/ @acme/team-ledger +/services/checkout/ @acme/team-checkout +/.github/workflows/ @acme/release-team diff --git a/plugins/fd3/evals/fixtures/protected-path-rollout-spec/repo-a/README.md b/plugins/fd3/evals/fixtures/protected-path-rollout-spec/repo-a/README.md new file mode 100644 index 0000000..54e96bf --- /dev/null +++ b/plugins/fd3/evals/fixtures/protected-path-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/protected-path-rollout-spec/repo-a/services/checkout/src/api/charge.ts b/plugins/fd3/evals/fixtures/protected-path-rollout-spec/repo-a/services/checkout/src/api/charge.ts new file mode 100644 index 0000000..cd4b9c7 --- /dev/null +++ b/plugins/fd3/evals/fixtures/protected-path-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/protected-path-rollout-spec/repo-a/services/checkout/src/config.ts b/plugins/fd3/evals/fixtures/protected-path-rollout-spec/repo-a/services/checkout/src/config.ts new file mode 100644 index 0000000..96692a0 --- /dev/null +++ b/plugins/fd3/evals/fixtures/protected-path-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/protected-path-rollout-spec/repo-a/services/ledger/migrations/README.md b/plugins/fd3/evals/fixtures/protected-path-rollout-spec/repo-a/services/ledger/migrations/README.md new file mode 100644 index 0000000..4594af4 --- /dev/null +++ b/plugins/fd3/evals/fixtures/protected-path-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/protected-path-rollout-spec/repo-a/services/ledger/src/api/entries.ts b/plugins/fd3/evals/fixtures/protected-path-rollout-spec/repo-a/services/ledger/src/api/entries.ts new file mode 100644 index 0000000..3312e13 --- /dev/null +++ b/plugins/fd3/evals/fixtures/protected-path-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/protected-path-rollout-spec/repo-b/README.md b/plugins/fd3/evals/fixtures/protected-path-rollout-spec/repo-b/README.md new file mode 100644 index 0000000..ca87206 --- /dev/null +++ b/plugins/fd3/evals/fixtures/protected-path-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/protected-path-rollout-spec/repo-b/src/components/PaymentStatus.tsx b/plugins/fd3/evals/fixtures/protected-path-rollout-spec/repo-b/src/components/PaymentStatus.tsx new file mode 100644 index 0000000..155abe8 --- /dev/null +++ b/plugins/fd3/evals/fixtures/protected-path-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/protected-path-rollout-spec/spec/protected-path-spec.md b/plugins/fd3/evals/fixtures/protected-path-rollout-spec/spec/protected-path-spec.md new file mode 100644 index 0000000..71bd016 --- /dev/null +++ b/plugins/fd3/evals/fixtures/protected-path-rollout-spec/spec/protected-path-spec.md @@ -0,0 +1,237 @@ +# 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. + +### CI-1 — migration step in the deploy workflow + +`repo-a/.github/workflows/deploy.yml` gains a step that applies pending ledger migrations before +the service deploy step, so DB-1 reaches staging by the documented CI convention rather than by +hand. + +- Request/response: none — this element is a workflow definition. +- Errors: a failing migration step fails the deploy job, and no service is deployed. +- Auth: the workflow's existing deploy credentials; no new secret is introduced. +- Limits: the step runs only on `main`, in filename order, and never rolls back. + +### 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-a `.github/workflows/` | CI-1 | pull request; CODEOWNERS requires release-team approval, which no other team can give | +| 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 — `.github/workflows/` (release-team) + +3. **CI-1** — changed: add the migration step to `repo-a/.github/workflows/deploy.yml` ahead of the + service deploy step. + +### repo-a — `services/checkout/` (team-checkout) + +4. **API-1** — changed: settlement write in `repo-a/services/checkout/src/api/charge.ts:6-8`, + guarded by the flag. +5. **CONFIG-1** — changed: flip `asyncSettlement` to `true` in + `repo-a/services/checkout/src/config.ts:2`. + +### repo-b (team-web) + +6. **UI-1** — changed: real rendering plus a mock data module, component stays unrouted + (`repo-b/src/components/PaymentStatus.tsx:5-8`). +7. **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 | CI-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: CI-1 first (the migration step must exist before a migration relies on +it), then DB-1 (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. +- **CI-1** — probe: `rg "migrate" repo-a/.github/workflows/deploy.yml` shows the migration step + above the deploy step. Before the change the file has no migration step. +- **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 deploy workflow has no migration step, and `.github/workflows/` is release-team's under CODEOWNERS | `repo-a/.github/workflows/deploy.yml` — checkout then deploy, nothing between; `repo-a/CODEOWNERS` — the `/.github/workflows/` line | +| 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 237 lines at this verdict + +| 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/fixtures/rollout-spec/DEFECTS.md b/plugins/fd3/evals/fixtures/rollout-spec/DEFECTS.md index 39f615b..0895735 100644 --- a/plugins/fd3/evals/fixtures/rollout-spec/DEFECTS.md +++ b/plugins/fd3/evals/fixtures/rollout-spec/DEFECTS.md @@ -13,7 +13,9 @@ Exactly six tasks, exercising all four cut boundaries at once: (irreversibility cuts, moves to the front). 2. **API-2** — repo-a / `services/ledger`, phase 1, depends on the DB-1 task. 3. **API-1** — repo-a / `services/checkout`, phase 1, depends on the API-2 task (build order: - DB-1 → API-2 → API-1; monorepo ownership cuts API-1 away from API-2). + DB-1 → API-2 → API-1; monorepo ownership cuts API-1 away from API-2). API-1 writes through + API-2's `POST /ledger/entries`, and that endpoint must stay in API-2's contract and work + item: without it the write path has no builder, and the split stops on a coverage gap. 4. **UI-1** — repo-b, phase 1, no depends-on (repository cuts). 5. **CONFIG-1** — repo-a / `services/checkout`, phase 2 (phase cuts CONFIG-1 away from API-1). 6. **INTEGRATION-1** — repo-b, phase 2 (phase cuts INTEGRATION-1 away from UI-1). diff --git a/plugins/fd3/evals/fixtures/rollout-spec/spec/rollout-spec.md b/plugins/fd3/evals/fixtures/rollout-spec/spec/rollout-spec.md index 16cc498..5a126fd 100644 --- a/plugins/fd3/evals/fixtures/rollout-spec/spec/rollout-spec.md +++ b/plugins/fd3/evals/fixtures/rollout-spec/spec/rollout-spec.md @@ -47,21 +47,21 @@ New migration `repo-a/services/ledger/migrations/0001_create_ledger_entries.sql` - 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 endpoint (ledger service) +### API-2 — ledger entries endpoints (ledger service) -`GET /ledger/entries?orderId=` replaces the stub at `repo-a/services/ledger/src/api/entries.ts:7`. +`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: `orderId` query parameter, required, non-empty string. -- Response: JSON array of `{ orderId: string, amountMinor: number, direction: "debit" | "credit", createdAt: string }`. -- Errors: 400 on a missing or empty `orderId`; 200 with `[]` when no entries exist. +- 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: response capped at 500 entries, newest first. +- 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 via the ledger -service's internal write endpoint. +`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 @@ -123,7 +123,7 @@ request. 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. + the real query and the 400 guard, and add the `POST` handler beside it. ### repo-a — `services/checkout/` (team-checkout) @@ -167,7 +167,7 @@ the migration stays behind, unused (expand-only; removal is out of scope, LED-10 - **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. Before the change both return the stub's empty 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 diff --git a/plugins/fd3/evals/fixtures/unvalidated-rollout-spec/spec/rollout-spec.md b/plugins/fd3/evals/fixtures/unvalidated-rollout-spec/spec/rollout-spec.md index 5718a9b..06d707b 100644 --- a/plugins/fd3/evals/fixtures/unvalidated-rollout-spec/spec/rollout-spec.md +++ b/plugins/fd3/evals/fixtures/unvalidated-rollout-spec/spec/rollout-spec.md @@ -47,21 +47,21 @@ New migration `repo-a/services/ledger/migrations/0001_create_ledger_entries.sql` - 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 endpoint (ledger service) +### API-2 — ledger entries endpoints (ledger service) -`GET /ledger/entries?orderId=` replaces the stub at `repo-a/services/ledger/src/api/entries.ts:7`. +`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: `orderId` query parameter, required, non-empty string. -- Response: JSON array of `{ orderId: string, amountMinor: number, direction: "debit" | "credit", createdAt: string }`. -- Errors: 400 on a missing or empty `orderId`; 200 with `[]` when no entries exist. +- 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: response capped at 500 entries, newest first. +- 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 via the ledger -service's internal write endpoint. +`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 @@ -123,7 +123,7 @@ request. 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. + the real query and the 400 guard, and add the `POST` handler beside it. ### repo-a — `services/checkout/` (team-checkout) @@ -167,7 +167,7 @@ the migration stays behind, unused (expand-only; removal is out of scope, LED-10 - **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. Before the change both return the stub's empty 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 diff --git a/plugins/fd3/evals/lib/checks/build-spec-gate.mjs b/plugins/fd3/evals/lib/checks/build-spec-gate.mjs index 61099cb..74f3246 100644 --- a/plugins/fd3/evals/lib/checks/build-spec-gate.mjs +++ b/plugins/fd3/evals/lib/checks/build-spec-gate.mjs @@ -1,10 +1,12 @@ import * as h from '../helpers.mjs'; -export default (output) => { +const NUMBERED = /^\s{0,3}(?:#{1,4}\s+)?(?:\*\*)?Q?\d+[.)]\s/m; + +export default (output, context) => { const c = h.checker(); - const numbered = output.match(/^\s{0,3}(?:#{1,4}\s+)?(?:\*\*)?Q?\d+[.)]\s/gm) || []; - c.check(numbered.length >= 1, 'no numbered round of questions — the grilling half never ran'); + // Numbering is grill-numbered-questions' concern; here the round only has to have gone out. + c.check(NUMBERED.test(output) || h.askedThroughTool(context), 'no round of questions — the grilling half never ran'); // The gate: without a confirmed closing summary the write-spec half must not start. const diff = h.diffSandbox('build-spec-gate', 'retry-topic'); diff --git a/plugins/fd3/evals/lib/checks/grill-session-files.mjs b/plugins/fd3/evals/lib/checks/grill-session-files.mjs new file mode 100644 index 0000000..4e0a091 --- /dev/null +++ b/plugins/fd3/evals/lib/checks/grill-session-files.mjs @@ -0,0 +1,36 @@ +import * as h from '../helpers.mjs'; + +// The grilling half keeps two bookkeeping files, and both have a pinned home: the question +// ledger under notes/, the prior-conversation record under research/. Loose in the working +// tree they land in the user's repository and outlive the session. +export default (output, context) => { + const c = h.checker(); + + const numbered = /^\s{0,3}(?:#{1,4}\s+)?(?:\*\*)?Q?\d+[.)]\s/m.test(output); + c.check(numbered || h.askedThroughTool(context), 'no round of questions — the grilling half never ran'); + + const diff = h.diffSandbox('grill-session-files', 'retry-topic'); + + // The session scratchpad sits wherever the skill puts it, so match the trailing directory. + const ledger = diff.added.filter((f) => /question-ledger\.md$/.test(f)); + if (c.check(ledger.length > 0, 'no question ledger was kept')) { + c.check( + ledger.every((f) => /(^|\/)notes\/question-ledger\.md$/.test(f)), + `the question ledger is not in a notes/ directory: ${ledger.join(', ')}`, + ); + } + + // Placement only: the prompt is the bare command, so nothing was established before it and + // writing no prior-conversation record is correct here. + const prior = diff.added.filter((f) => /prior-conversation\.md$/.test(f)); + c.check( + prior.every((f) => /(^|\/)research\/prior-conversation\.md$/.test(f)), + `the prior-conversation record is not in a research/ directory: ${prior.join(', ')}`, + ); + + const stray = diff.added.filter((f) => !/(^|\/)(notes|research)\//.test(f)); + c.check(stray.length === 0, `session files written outside notes/ and research/: ${stray.join(', ')}`); + c.check(diff.modified.length === 0, `fixture files modified: ${diff.modified.join(', ')}`); + + return c.verdict(); +}; diff --git a/plugins/fd3/evals/lib/checks/split-declared-gap.mjs b/plugins/fd3/evals/lib/checks/split-declared-gap.mjs new file mode 100644 index 0000000..b11d6e9 --- /dev/null +++ b/plugins/fd3/evals/lib/checks/split-declared-gap.mjs @@ -0,0 +1,37 @@ +import * as h from '../helpers.mjs'; +import * as s from './split-shared.mjs'; + +// The spec's verdict line carries one deferred claim — a gap the spec itself declares with an +// owner and a placement. The split proceeds and tracks the gap as an operational task. +export default (output) => { + const c = h.checker(); + const tasks = h.readTasks('split-declared-gap'); + + c.check(tasks.length === 7, `expected 7 task files (6 delivery + the declared gap), found ${tasks.length}`); + s.checkTaskStructure(c, tasks); + s.checkCoverage(c, tasks); + s.checkBoundaries(c, tasks); + s.checkIndexCardRule(c, tasks); + + const operational = tasks.filter((t) => t.fm && t.fm.repository === 'none'); + if (c.check(operational.length === 1, `expected exactly 1 operational task, found ${operational.length}`)) { + const gap = operational[0]; + c.check(s.elementsOf(gap).length === 0, `${gap.file}: the gap task carries an element code it does not build`); + const note = h.section(gap.body, 'Note') || ''; + c.check(/platform team/i.test(note), `${gap.file}: the ## Note does not name the gap's owner (platform team)`); + c.check(/phase 2|ceiling|rate[-\s]?limit/i.test(note), `${gap.file}: the ## Note does not say what the gap is or where it lands`); + } + + // The run did not stop on the deferred claim: the files and the report exist. + const SPLIT_REPORT = 'spec/gap-rollout-spec.split.md'; + const diff = h.diffSandbox('split-declared-gap', 'gap-rollout-spec'); + c.check(diff.added.includes(SPLIT_REPORT), `the split report ${SPLIT_REPORT} was not written beside the spec`); + c.check(diff.modified.length === 0, `fixture files modified (spec is read-only here): ${diff.modified.join(', ')}`); + const stray = diff.added.filter((f) => !f.startsWith('spec/tasks/') && f !== SPLIT_REPORT); + c.check(stray.length === 0, `files created outside spec/tasks/: ${stray.join(', ')}`); + + const report = h.readSandboxFile('split-declared-gap', SPLIT_REPORT) || ''; + c.check(/1 deferred/.test(report) || /1 deferred/.test(output), 'neither the report nor the reply quotes the verdict line the split was taken against'); + + return c.verdict(); +}; diff --git a/plugins/fd3/evals/lib/checks/split-protected-path.mjs b/plugins/fd3/evals/lib/checks/split-protected-path.mjs new file mode 100644 index 0000000..7bdd663 --- /dev/null +++ b/plugins/fd3/evals/lib/checks/split-protected-path.mjs @@ -0,0 +1,34 @@ +import * as h from '../helpers.mjs'; +import * as s from './split-shared.mjs'; + +// CI-1 edits `.github/workflows/`, which repo-a's CODEOWNERS gives to release-team. It is a +// delivery task like any other, on a branch of its own, so one external approval cannot hold +// the rest of the phase-1 landing unit. +export default (output) => { + const c = h.checker(); + const tasks = h.readTasks('split-protected-path'); + const CODES = [...s.ELEMENT_CODES, 'CI-1']; + + c.check(tasks.length === 7, `expected 7 task files (the six elements plus CI-1), found ${tasks.length}`); + s.checkTaskStructure(c, tasks); + s.checkCoverage(c, tasks, CODES); + s.checkBoundaries(c, tasks, { precedesDb: 'CI-1' }); + s.checkIndexCardRule(c, tasks); + + const ci = tasks.find((t) => s.elementsOf(t).includes('CI-1')); + if (c.check(ci !== undefined, 'no task carries CI-1')) { + c.check(s.elementsOf(ci).length === 1, `${ci.file}: CI-1 shares its task with another element`); + c.check(ci.fm.repository !== 'none', `${ci.file}: CI-1 is a delivery task, not an operational one`); + c.check(/1/.test(String(ci.fm.phase)), `${ci.file}: CI-1 is not in phase 1 (${ci.fm.phase})`); + + const sharing = tasks.filter((t) => t !== ci && t.fm && t.fm.branch && t.fm.branch === ci.fm.branch); + c.check(sharing.length === 0, `${ci.file}: the protected-path task shares its branch with ${sharing.map((t) => t.file).join(', ')}`); + } + + const SPLIT_REPORT = 'spec/protected-path-spec.split.md'; + const diff = h.diffSandbox('split-protected-path', 'protected-path-rollout-spec'); + c.check(diff.added.includes(SPLIT_REPORT), `the split report ${SPLIT_REPORT} was not written beside the spec`); + c.check(diff.modified.length === 0, `fixture files modified (spec is read-only here): ${diff.modified.join(', ')}`); + + return c.verdict(); +}; diff --git a/plugins/fd3/evals/lib/checks/split-shared.mjs b/plugins/fd3/evals/lib/checks/split-shared.mjs index 44c8a71..8e3c7bf 100644 --- a/plugins/fd3/evals/lib/checks/split-shared.mjs +++ b/plugins/fd3/evals/lib/checks/split-shared.mjs @@ -73,7 +73,9 @@ export function checkCoverage(c, tasks, codes = ELEMENT_CODES) { } } -export function checkBoundaries(c, tasks) { +// `precedesDb` names the one element a fixture's build order puts ahead of the migration; without +// it DB-1 is the root and any edge onto it is one the data does not require. +export function checkBoundaries(c, tasks, { precedesDb = null } = {}) { for (const t of tasks) { const els = elementsOf(t); const groupsHit = OWNER_GROUPS.filter((g) => els.some((e) => g.includes(e))).length; @@ -82,8 +84,18 @@ export function checkBoundaries(c, tasks) { const migration = tasks.find((t) => elementsOf(t).includes('DB-1')); if (c.check(migration !== undefined, 'no task carries DB-1')) { c.check(elementsOf(migration).length === 1, 'the DB-1 migration does not have its own task'); - const deps = migration.fm['depends-on']; - c.check(!deps || deps.length === 0, 'the DB-1 migration task has a depends-on edge its data does not require'); + const deps = migration.fm['depends-on'] || []; + if (precedesDb === null) { + c.check(deps.length === 0, 'the DB-1 migration task has a depends-on edge its data does not require'); + } else { + const predecessor = tasks.find((t) => elementsOf(t).includes(precedesDb)); + const allowed = predecessor ? [predecessor.slug, predecessor.fm && predecessor.fm.name] : []; + c.check(deps.length === 1, `the DB-1 migration task carries ${deps.length} edges; the build order puts only ${precedesDb} ahead of it`); + c.check( + deps.every((d) => allowed.includes(String(d))), + `the DB-1 migration task depends on ${deps.join(', ')} instead of the ${precedesDb} task`, + ); + } } const slugs = new Set(tasks.flatMap((t) => [t.slug, t.file.replace(/\.md$/, ''), t.fm && t.fm.name].filter(Boolean))); for (const t of tasks) { diff --git a/plugins/fd3/evals/lib/checks/validate-ownerless-gap.mjs b/plugins/fd3/evals/lib/checks/validate-ownerless-gap.mjs new file mode 100644 index 0000000..199fa9c --- /dev/null +++ b/plugins/fd3/evals/lib/checks/validate-ownerless-gap.mjs @@ -0,0 +1,41 @@ +import * as h from '../helpers.mjs'; + +// Same gap as validate-declared-gap, with the owner and the placement stripped. Step 4 asks who +// owns it: an answer makes it deferred with both named, and no answer makes it blocked, which +// lowers the verdict. What must never happen is the third way — deferred on an owner nobody gave. +const CLAIM = /ceiling|rate[-\s]?limit|DELIVERY_RATE_LIMIT/i; + +export default (output) => { + const c = h.checker(); + + c.check(h.checksTableComplete(output), 'Checks table is missing rows (needs all 12)'); + + const verdict = h.section(output, 'Verdict') || ''; + const blocked = h.section(output, 'Blocked') || ''; + const deferred = h.section(output, 'Deferred') || ''; + + c.check(CLAIM.test(blocked) || CLAIM.test(deferred), 'the unowned ceiling claim reaches neither ## Blocked nor ## Deferred'); + + if (CLAIM.test(blocked)) { + c.check(/\bnot ready\b/i.test(verdict), 'a blocked claim did not lower the verdict to "not ready"'); + } else { + const entry = deferred.split('\n').find((l) => CLAIM.test(l)) || ''; + c.check(/owner:/i.test(entry), 'the ceiling claim was deferred without an owner — the spec names none, so an answer has to'); + c.check(/placement:/i.test(entry), 'the ceiling claim was deferred without a placement'); + c.check(!/nobody|unowned|unknown|tbd|n\/a/i.test(entry), `the deferred entry stands in for an owner instead of naming one: ${entry.trim()}`); + } + + // The out-of-scope items do name owners and tickets; they are not what this spec leaves open. + c.check(!/invoice pdf/i.test(blocked), 'an out-of-scope item that names an owner and a ticket was graded as blocked'); + + const SPEC = 'spec/ownerless-gap-spec.md'; + const diff = h.diffSandbox('validate-ownerless-gap', 'ownerless-gap-payments-spec'); + c.check( + diff.modified.every((f) => f === SPEC), + `modified outside the spec: ${diff.modified.filter((f) => f !== SPEC).join(', ')}`, + ); + c.check(diff.removed.length === 0, `fixture files removed: ${diff.removed.join(', ')}`); + c.check(diff.added.every((f) => f.startsWith('spec/')), `files created outside spec/: ${diff.added.filter((f) => !f.startsWith('spec/')).join(', ')}`); + + return c.verdict(); +}; diff --git a/plugins/fd3/evals/lib/helpers.mjs b/plugins/fd3/evals/lib/helpers.mjs index 3c8c1de..f63a0a7 100644 --- a/plugins/fd3/evals/lib/helpers.mjs +++ b/plugins/fd3/evals/lib/helpers.mjs @@ -160,3 +160,9 @@ export function section(output, heading) { const next = /^##\s+/m.exec(rest); return next ? rest.slice(0, next.index) : rest; } + +// A round that went out through AskUserQuestion is auto-answered under `first_option`, so the +// final message can hold no numbered question even though the grilling ran. +export function askedThroughTool(context) { + return (context?.providerResponse?.metadata?.toolCalls || []).some((call) => call.name === 'AskUserQuestion'); +} diff --git a/plugins/fd3/evals/promptfooconfig.yaml b/plugins/fd3/evals/promptfooconfig.yaml index cf0f57e..d7ab7ab 100644 --- a/plugins/fd3/evals/promptfooconfig.yaml +++ b/plugins/fd3/evals/promptfooconfig.yaml @@ -65,6 +65,13 @@ tests: - type: javascript value: file://lib/checks/validate-phased-verdict.mjs + - description: validate-ownerless-gap + vars: { query: file://prompts/validate-ownerless-gap.txt } + options: { working_dir: .sandbox/validate-ownerless-gap, max_budget_usd: 5.0 } + assert: + - type: javascript + value: file://lib/checks/validate-ownerless-gap.mjs + # ---- split-to-tasks (Priority 1; same budget override) ---- - description: split-baseline vars: { query: file://prompts/split-baseline.txt } @@ -102,6 +109,20 @@ tests: - type: javascript value: file://lib/checks/split-english-artifacts.mjs + - description: split-declared-gap + vars: { query: file://prompts/split-declared-gap.txt } + options: { working_dir: .sandbox/split-declared-gap, max_budget_usd: 5.0 } + assert: + - type: javascript + value: file://lib/checks/split-declared-gap.mjs + + - description: split-protected-path + vars: { query: file://prompts/split-protected-path.txt } + options: { working_dir: .sandbox/split-protected-path, max_budget_usd: 5.0 } + assert: + - type: javascript + value: file://lib/checks/split-protected-path.mjs + # ---- write-spec (Priority 2) ---- - description: write-missing-input-stop vars: { query: file://prompts/write-missing-input-stop.txt } @@ -159,6 +180,13 @@ tests: - type: javascript value: file://lib/checks/grill-numbered-questions.mjs + - description: grill-session-files + vars: { query: file://prompts/grill-session-files.txt } + options: { working_dir: .sandbox/grill-session-files, max_budget_usd: 8.0 } + assert: + - type: javascript + value: file://lib/checks/grill-session-files.mjs + # ---- build-spec command (gate smoke) ---- # 5.0 like the P1 group: first_option auto-answers whole grilling rounds, so this run # is long by design and a 2.0 cut-off kills the SDK process mid-flight (hard exit 1). diff --git a/plugins/fd3/evals/prompts/grill-session-files.txt b/plugins/fd3/evals/prompts/grill-session-files.txt new file mode 100644 index 0000000..bf0dca1 --- /dev/null +++ b/plugins/fd3/evals/prompts/grill-session-files.txt @@ -0,0 +1 @@ +/fd3:grill-topic notes/topic.md \ No newline at end of file diff --git a/plugins/fd3/evals/prompts/split-declared-gap.txt b/plugins/fd3/evals/prompts/split-declared-gap.txt new file mode 100644 index 0000000..4fa0ff1 --- /dev/null +++ b/plugins/fd3/evals/prompts/split-declared-gap.txt @@ -0,0 +1 @@ +/fd3:split-to-tasks spec/gap-rollout-spec.md \ No newline at end of file diff --git a/plugins/fd3/evals/prompts/split-protected-path.txt b/plugins/fd3/evals/prompts/split-protected-path.txt new file mode 100644 index 0000000..f289e2f --- /dev/null +++ b/plugins/fd3/evals/prompts/split-protected-path.txt @@ -0,0 +1 @@ +/fd3:split-to-tasks spec/protected-path-spec.md \ No newline at end of file diff --git a/plugins/fd3/evals/prompts/validate-ownerless-gap.txt b/plugins/fd3/evals/prompts/validate-ownerless-gap.txt new file mode 100644 index 0000000..00a4207 --- /dev/null +++ b/plugins/fd3/evals/prompts/validate-ownerless-gap.txt @@ -0,0 +1,3 @@ +Invoke the fd3:validate-spec skill on this spec: spec/ownerless-gap-spec.md + +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 6244123..ffe8c2f 100755 --- a/plugins/fd3/evals/reset-sandboxes.sh +++ b/plugins/fd3/evals/reset-sandboxes.sh @@ -13,7 +13,10 @@ MAPPINGS=( "validate-clean-spec:clean-payments-spec:." "validate-declared-gap:gap-payments-spec:." "validate-phased-verdict:phased-payments-spec:." + "validate-ownerless-gap:ownerless-gap-payments-spec:." "split-baseline:rollout-spec:repo-a repo-b" + "split-declared-gap:gap-rollout-spec:repo-a repo-b" + "split-protected-path:protected-path-rollout-spec:repo-a repo-b" "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" @@ -23,6 +26,7 @@ MAPPINGS=( "grill-round-shape:retry-topic:." "grill-no-topic:retry-topic:." "grill-numbered-questions:retry-topic:." + "grill-session-files:retry-topic:." "build-spec-gate:retry-topic:." "e2e-chain:grilling-summary:." "researcher-output-contract:-:" diff --git a/plugins/fd3/references/spec-template.md b/plugins/fd3/references/spec-template.md index 540f597..d9879f1 100644 --- a/plugins/fd3/references/spec-template.md +++ b/plugins/fd3/references/spec-template.md @@ -227,3 +227,13 @@ A table. This is the spec's proof of work, and it is what a validation pass spot A claim that rests on inference says so — "no documentation states the negative explicitly; treat as strong inference, confirmed empirically at stage before prod" is honest and actionable. Softening it into a confirmation is the one thing this table exists to prevent. + +**A cell is a line, not a paragraph.** Two sentences at most in any cell of any table in this +document. A verification that needs a transcript, a query plan, a long quote or a list of hits puts +it in `evidence/
.md` beside the spec and cites that file in the cell. Prose that fills a +cell is unreadable at the width a reviewer scans, and it is what pushes a spec past the size at +which anyone re-reads it. + +Each validation pass appends its own dated block here. A pass whose rows outgrow the appendix writes +them to `evidence/-pass-.md` and leaves the dated block its verdict line plus one line per +claim pointing at that file — the record stays complete, and the spec stays a document. diff --git a/plugins/fd3/references/validation-report.md b/plugins/fd3/references/validation-report.md index cb66632..4b8e0f9 100644 --- a/plugins/fd3/references/validation-report.md +++ b/plugins/fd3/references/validation-report.md @@ -1,6 +1,8 @@ # Validation report -The shape `validate-spec` returns its verdict and its status in. +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. ```markdown ## Run @@ -16,10 +18,25 @@ repositories: |---|---|---| ## Checks + +Twelve rows, always all twelve, numbered and ordered as the skill numbers them — a return that +prints only the checks that moved leaves the caller unable to tell an unrun check from a passing +one. The short names are fixed too: + | # | Check | Result | |---|---|---| | 1 | decisions do not contradict | pass (unchanged) | | 2 | scope covers every decision | fail — section 10, | +| 3 | every element described and coded | | +| 4 | dependencies exist, planned or deferred | | +| 5 | external contracts confirmed | | +| 6 | referenced documents open | | +| 7 | element contracts complete | | +| 8 | build order stated and holds | | +| 9 | achievable in this project | | +| 10 | splittable into tasks | | +| 11 | every element has a check | | +| 12 | no vague verb, no undecided either/or | | ## Still open - —
— — blocking | non-blocking diff --git a/plugins/fd3/skills/grill-topic/SKILL.md b/plugins/fd3/skills/grill-topic/SKILL.md index 221455b..21bedca 100644 --- a/plugins/fd3/skills/grill-topic/SKILL.md +++ b/plugins/fd3/skills/grill-topic/SKILL.md @@ -16,6 +16,8 @@ Establish the facts the topic asserts, before asking anything. A topic document This is the highest-value work in the whole session. A round asked against the document gets answers about the document; a round asked against reality gets answers you can build on. +**The conversation before the command counts as input.** Whatever was established before this skill was invoked — a constraint the user stated, an option they already ruled out, a number they gave, a file they pointed at — goes into `prior-conversation.md` in the session's research directory before round 1, one line each, naming who established it. It is the only input no lookup can re-derive: it does not survive a compaction, and every downstream skill reads files rather than this conversation. Round 1 then treats those lines as facts under test like any other, not as settled ground. + Two things come before the first dispatch. `git fetch` the repository and say if the tree is behind the branch the topic describes — facts cited from a stale clone drift on exactly the files the session will argue from. And read the repository's own prior specs, ADRs and decision records (`docs/specs/`, `requirements/`, wherever they live): a question one of them already settles is not a question, and a lookup dispatched without them re-researches a decision the repository has already made. For every library, service or tool the topic names, establish three versions: the one the lockfile resolves — never the manifest range — the current release, and the one whose API the discussion will quote. Any difference between them is a round-1 finding: an API argued from the wrong version becomes pseudocode nobody can run. A table when there are more than two, a line each otherwise. @@ -40,6 +42,8 @@ Open every round after the first with one line naming the numbers still unanswer A blocked question keeps its number and stays out of the round's numbered items; name it on the line that opens the round. A number printed inside the round is a number the user will answer. +Keep a **question ledger** — `notes/question-ledger.md` in the session scratchpad, beside where the closing notes land — with one row per number: the question in a line, the round it went out in, the option chosen or `open`, and `carried-over` where it has been re-asked. Write the row when the question goes out, and update it when the answer arrives — before composing the next round, which is read from the ledger and not from memory. Numbers tracked in your head are the first thing a compaction takes, and what comes back after one is a reassigned number, a question that quietly vanished, or a carried-over question compressed into a summary of itself. + Track which numbered questions came back answered. A question the user skipped is still open: re-put it in the next round under its original number, labelled as carried over, with its options and costs written out in full — a carried-over question compressed to a list of recommendations is not a question, and a user who answers one is ratifying a menu they cannot see. Numbers are never reassigned, so the summary can cite one decision by one name. Silence is not assent, and there is no round count after which it becomes assent. A defect the user admits to scope is work admitted, not work decided: each admitted defect gets its own numbered question with fix options, or an explicit deferral with an owner. A batch yes/no that sweeps a dozen defects into scope leaves every one of them undesigned. @@ -73,7 +77,7 @@ Route every lookup by where the fact lives — the routes and dispatch rules are The session's research directory is `research/` in the session scratchpad. Dispatch prompts name it, agents write their full reports there, and what enters this conversation is each report's condensed answers and its file path. When a round argues from a report's findings, cite the file — the user can open the evidence. -A question you dispatched a lookup for is **blocked by that lookup** — no exceptions. Do not predict what the lookup will return, or which questions it will turn out to touch: whether a fact changes a question is knowable only once you hold the fact. Questions you sent nobody to answer are not blocked — ask those now; a running lookup is an unsettled prerequisite for its own question only. +A question you dispatched a lookup for is **blocked by that lookup** — no exceptions. Do not predict what the lookup will return, or which questions it will turn out to touch: whether a fact changes a question is knowable only once you hold the fact. Questions you sent nobody to answer are not blocked — ask those now; a running lookup is an unsettled prerequisite for its own question only. Never hold a round back to keep it whole: post the unblocked questions and name the blocked numbers on the round's opening line. A turn that ends on "waiting for the lookup" leaves the user nothing to answer. A recommendation is never conditional. If you would write "recommended, provided the check confirms it", the question is blocked by that check and stays out of the round — a conditional recommendation gets answered as an unconditional one. The same bar holds inside an option's cost, its preamble and the recommendation itself: any admission that something outside this conversation is unchecked — a source unread, a contradiction unresolved, a behaviour unobserved — is a conditional recommendation wearing a cost's clothes, and the question is blocked by that lookup. If you find yourself writing the hedge, you have found the dispatch. diff --git a/plugins/fd3/skills/implement-tasks/SKILL.md b/plugins/fd3/skills/implement-tasks/SKILL.md index 3b2fad0..595f837 100644 --- a/plugins/fd3/skills/implement-tasks/SKILL.md +++ b/plugins/fd3/skills/implement-tasks/SKILL.md @@ -33,10 +33,11 @@ to have. ## Workflow -Post this checklist as your first message in the run, before any tool call — a run that then stops -on an unresolvable path has cost one message. Post it again in full — marks updated, never -compressed to a line and never summarised — before every user interaction (the question batch, each -report round) and at the close: +Open your first reply with this checklist, before any tool call — a run that then stops on an +unresolvable path has cost one message — and make the first tool call in that same reply. A reply +that only announces the checklist, or only posts it, ends the turn with nothing done. Post it again +in full — marks updated, never compressed to a line and never summarised — before every user +interaction (the question batch, each report round) and at the close: ``` - [ ] 1. Read the graph: parse task frontmatter, resolve repositories, check integrity @@ -110,9 +111,16 @@ 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 is roughly 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; + 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; - 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; @@ -133,6 +141,12 @@ One batch, following `${CLAUDE_SKILL_DIR}/../../references/question-batching.md` Everything else — wave composition, branch names, merge order — the task files already decided; report it, do not ask. +Committing the spec and the tasks directory before launch is the user's call, and it is a change +to the repository like any other: whatever that repository derives from the tree you touched — +a docs index, a manifest, a generated list — regenerate it in the same commit, or say plainly +that you did not. A stale generated file fails validation on every branch of the run at once, +and reads there as the branches' own defect. + ### 3. Launch Launch the dynamic workflow and let it run in the background: @@ -158,7 +172,10 @@ repository's `defaultRef` — the ref the user confirmed in step 2, fetched fres Worktrees and target branches are cut from that ref (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. On a relaunch, pass `reportPath` — the `` +main checkout rather than discover the refusal. Merges and fixes for that branch then happen in +the checkout, but its validation does not: the workflow grades it in a detached worktree beside +the repository, so the verdict describes the branch's commit rather than whatever else the user +has open in that tree. On a relaunch, pass `reportPath` — the `` path from the previous run's completion notification. The workflow reads that file's toolchain and baseline knowledge with one cheap agent, so the run skips a re-scout and a re-baseline of every repository it already knows. Never transcribe that knowledge into the call yourself: it is tens of @@ -196,7 +213,10 @@ the task file's steps as a script to follow; mark `done` only when they confirm) conflict needs their call on how to proceed. A CI failure on the list may be diagnosed first — read-only, in the branch's worktree — so the question puts analyzed options before the user instead of raw output; the diagnosis then travels verbatim in the repair `instructions`, sparing -the repair agent a re-investigation. The answers split into two lanes: +the repair agent a re-investigation. Diagnose by **running the failing check** in that worktree and +reading what it says. Grepping the source for what the report's message suggests names a plausible +cause, not the cause: the check is the only thing that knows which of them is true, and a repair +composed from the plausible one costs a full round to disprove. The answers split into two lanes: - **Decisions that unblock tasks** — update the affected task files and relaunch `implement-run` the same way; statuses make the rerun skip everything finished. @@ -222,6 +242,10 @@ the repair agent a re-investigation. The answers split into two lanes: 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. + 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 + returns, and an agent that runs it too puts a second pipeline on a machine that tolerates one. + One carve-out from the second lane: a purely mechanical git operation — merging an existing task branch into its target, reverting a named commit — may be done by this skill directly when the decision deliberately leaves the branch incomplete, because a repair-run would fail its own @@ -241,8 +265,8 @@ live. A pause that survives only in this conversation is state lost. ### 5. Propose, never push When every repository-bearing task is `done`: one table — repository, branch, its stack base, -tasks on it, the element codes those tasks carry, proposed pull-request title citing the -tickets — with the still-open operational tasks listed alongside; they need the branches landed +its worktree path, tasks on it, the element codes those tasks carry, proposed pull-request title +citing the tickets — with the still-open operational tasks listed alongside; they need the branches landed first, so they never gate this proposal. Stacked branches make a pull-request chain: each pull request's base is its branch's stack base, and after one lands its successor is retargeted onto the default branch — but only when the predecessor landed as a merge commit. After a squash @@ -253,6 +277,8 @@ after explicit consent: push, `gh pr create` per branch (`--base` set to the sta description naming the tasks, the spec and the branch's element codes. Offer cleanup — remove the `.worktrees` directories and delete the merged `task/` branches — as its own question, never coupled to the push: declining to publish while wanting a clean repository is a -normal combination. If push consent does not come, leave everything local and say where it -lives — and when the tasks directory is untracked, say that too: it is the only copy of the +normal combination. If push consent does not come, leave everything local. The worktree paths are +in the table whatever the user decides: a branch whose worktree nobody can name is a branch the +user cannot open, and the run's own directories are not guessable. When the tasks directory is +untracked, say that too: it is the only copy of the run's state store, one `git clean -fd` away from gone. diff --git a/plugins/fd3/skills/split-to-tasks/SKILL.md b/plugins/fd3/skills/split-to-tasks/SKILL.md index afb11fd..71f32bb 100644 --- a/plugins/fd3/skills/split-to-tasks/SKILL.md +++ b/plugins/fd3/skills/split-to-tasks/SKILL.md @@ -26,6 +26,11 @@ that cites nothing — is a reason to stop and report it, never something to fix split. Its frontmatter says `repository: none` and leaves `branch` empty — these fields are machine-read, so prose in them breaks the reader — and its body closes with a `## Note` saying why no pull request exists. + A declared gap the spec carries into the split is the second thing this shape holds (see + *Precondition*): the missing fact has an owner, and the task is how the split tracks it. + Both cases need hand-run steps that no repository carries. A phase's own verification rows are + not that — the repositories' checks run them — so a phase boundary gets no operational task + unless the spec names a gate outside its own verification. - **The index card rule** — a task file carries pointers, never copies. The spec stays the single source of truth. Contract prose copied into a task is a second source of truth that rots silently, because nothing detects that the spec moved on. @@ -35,11 +40,18 @@ that cites nothing — is a reason to stop and report it, never something to fix Splitting propagates the spec's defects into every task. A validation verdict in this conversation settles the question. Otherwise the spec must carry all three: read its evidence record **from the bottom** — the last verdict line in the file is the current one, position -decides and not the date — that line records no blocked claims, and its count equals `wc -l` on -the spec. - -Anything short of that — blocked claims, 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 +decides and not the date — that line reads `ready`, it counts no blocked claim, and its count +equals `wc -l` on the spec. + +**A declared gap is work, not a stop.** Validation records a gap the spec declares with an owner +and a placement as a `deferred` claim, and a `ready` verdict may carry any number of them: the +fact is unresolved and the spec names who resolves it. Each such gap gets an **operational task** +of its own, whose `## Note` says what has to come back and from whom, and every task the gap +blocks from being *written* draws a `depends-on` edge onto it (step 4's authorship rule). A +`blocked` claim is a gap nothing owns — it is not a declared gap, and it stops the split. + +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 @@ -47,10 +59,11 @@ ends the run says what the record held and which way the user answered. ## Workflow -Post this checklist as your first message in the run, before any tool call — a run that then stops -on an unresolvable path has cost one message. Post it again in full — marks updated, never -compressed to a line and never summarised — before every user interaction (the question batch, the -report) and at the close: +Open your first reply with this checklist, before any tool call — a run that then stops on an +unresolvable path has cost one message — and make the first tool call in that same reply. A reply +that only announces the checklist, or only posts it, ends the turn with nothing done. Post it again +in full — marks updated, never compressed to a line and never summarised — before every user +interaction (the question batch, the report) and at the close: ``` - [ ] 1. Enumerate: work items, element codes, phases and gates, ownership, tickets @@ -83,6 +96,11 @@ Record each repository's absolute root path and write that path into `repository 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. +The spec is an input read at its **absolute path**, and where it happens to be committed decides +nothing. A spec written on a docs branch, on a feature branch, or in a repository the work never +touches still cuts its branches from each repository's default branch: a base is derived from the +rollout and the stack, never from `git log` on the spec file. + ### 2. Cut The raw material is the work items. Every task is a subset of them, and every item lands in @@ -105,6 +123,13 @@ reason, and the reason decides the edge cases: section and becomes its own late task, behind the gate the spec names, carrying `phase: cleanup` in its frontmatter — cleanup is not a rollout phase, and a section reference in a machine-read field breaks the reader. +5. **A protected path cuts, and keeps its own branch.** An edit to a path the repository guards — + a `CODEOWNERS` entry naming another team, a branch-protection or required-review rule, a + release manifest, the CI workflow definitions — waits on an approval the rest of the unit does + not. It is a delivery task like any other, with its own element and done criterion, on a branch + of its own, so one external approval never holds the whole landing unit. Read the repository's + `CODEOWNERS` and protection settings in step 1 to know which paths these are; where the + repository declares none, the rule finds nothing and costs nothing. Within what survives the boundaries, prefer the smallest task that makes one verification row pass. Where one repository owns a whole behaviour, that yields a vertical slice — schema, endpoint @@ -156,7 +181,16 @@ is policy of this skill and never appears in the spec. ### 4. Order Dependencies come from the spec's build order and its phase table. Record each as a `depends-on` -edge between task slugs: an edge means the other task must land first. Never draw an edge onto +edge between task slugs: an edge means the other task must land first. + +**An edge between two tasks on the same branch has to name what they share.** Same-branch tasks +land together, so an edge there is a claim that one must be *written* before the other, and only a +concrete overlap makes that true: a file both touch, a symbol one defines and the other calls, a +migration one writes and the other reads. Name it in the dependent task's `## Note` and in the +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 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 @@ -166,8 +200,9 @@ operational task exists for that gate, that is a coverage failure in step 5, not record the gate in prose. Propose each branch's name following its repository's visible convention — existing branches show it; the name belongs to the group, not the task. One exception joins the step-6 batch: when the checkout already sits on a -branch carrying the spec's commits, 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. +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. 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 @@ -206,7 +241,7 @@ Before writing anything, check — and say in the report — that: workflow's merge planning relies on that; - the `branch-base` chain is rooted, acyclic, single-parent and identical on every task of a branch. Its one root is the repository's default branch — or, where step 4 found the checkout - already sitting on a branch that carries the spec's commits, that branch: the root is then + already sitting on a branch that carries implementation commits for this spec, that branch: the root is then whatever the step-6 answer settles, so a chain rooted there is a question still pending, never a coverage failure. Stopping on it would abort a split the user was never asked about. @@ -258,6 +293,6 @@ a task-file glob trips over it. It carries one table (slug, repository, branch, depends-on, 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 table, -not the file. Anything in the report that binds one task's work also goes into that task's +and anything the user still owes an answer. In the conversation give the path and the same +table — all six columns, `elements` included — not the file. Anything in the report that binds one task's work also goes into that task's `## Note`, in the imperative — the conversation ends before implementation starts. diff --git a/plugins/fd3/skills/validate-spec/SKILL.md b/plugins/fd3/skills/validate-spec/SKILL.md index 1b7bc5a..4c1b23c 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. +**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 +except for the appended block — rewording a section you merely read, tidying a table, or improving +prose nobody flagged rewrites a document the user validated on the strength of its own wording. + ## Goal Decide whether the spec can be implemented, or split into tasks, as written. It can when every @@ -62,6 +68,10 @@ broken one — so a check inherits its reasoning, never its result. What you do evidence work behind a claim the status records as `verified`, unless an edit since then touched the section it rests on. Spend the pass on what the status leaves open. +The invocation may carry a **focus list** with that status — the findings still open and the sections +the previous pass edited. It says where to spend the pass, never what to skip: the twelve checks are +re-derived either way, and an edited section is read as new text, not as a section already cleared. + The spec's evidence record may hold dated blocks that no handed-down status accounts for — passes from earlier runs. Their identity is their date; pass numbers count this run's passes only. They are available when a claim's history bears on what you are deciding. @@ -257,6 +267,12 @@ repair choices alike — numbered, each with your recommended answer first, then answers arrive as a message and you continue from where you stopped, with everything this pass established still in front of you. +**The batch carries this pass's report with it, and so does the turn that ends.** Handing up is not +an alternative to reporting: the twelve check rows, the findings you already hold and a verdict of +`not ready` go out in the same message, with the handed-up items under *Still open*. A turn that +ends on "the full table comes once the answers land" leaves the caller with nothing to relay and +nothing to act on, and the answers may never come. + One batch per pass. Nothing may still be outstanding when you send it: a dispatch that has not returned is a dispatch whose answer changes what you would ask, and a second message sent while the first is being answered tells the user the first was incomplete. Steps 3–5 may bring you back here, @@ -274,6 +290,19 @@ names no owner. A `blocked` claim goes into the report; do not put it to the use ends with a claim `open`: a claim you cannot settle before reporting becomes `blocked`, with the reason it could not be settled stated in the report. +**`ready` and `blocked`.** A `deferred` claim never lowers a verdict: it bounds the phase it gates, +that phase's row still reads `yes`, and the document is still `ready`. The verdict is `ready` only +when every claim is `verified` or `deferred` — +a declared gap with a named owner and a placement is what a downstream stage can act on, because +`split-to-tasks` turns it into an operational task carrying that owner. An ownerless `blocked` claim +leaves it nothing to write, so it makes the verdict `not ready`, however small the gap looks. The +last move before recording a claim `blocked` is therefore step 4: ask the user who owns it and where +it lands. An owner and a placement make it `deferred`, and the spec records both. + +This reaches the user only for a claim **nothing in the spec owns**. A gap the spec already declares +with an owner and a placement is `deferred` on sight — it is not a finding, it does not go into the +batch, and a pass that asks about it has turned a settled document into a question. + ### 6. Report End the pass by returning the verdict and this pass's status — nothing is written to a file. The diff --git a/plugins/fd3/skills/write-spec/SKILL.md b/plugins/fd3/skills/write-spec/SKILL.md index 00f2f3a..8f7584c 100644 --- a/plugins/fd3/skills/write-spec/SKILL.md +++ b/plugins/fd3/skills/write-spec/SKILL.md @@ -101,6 +101,10 @@ worth more than any prose you could write instead. Two rules: `${CLAUDE_SKILL_DIR}/../../references/fact-routes.md`. If it stays unsettled, it goes into the document as a declared gap with an owner and a placement, never as a bare statement. +Keep every cell of this table — and of every other table in the spec — to two sentences. Evidence +that needs more room goes to `evidence/
.md` beside the spec, cited from the cell; the +template says so, and a spec whose tables read as prose is one nobody re-reads. + A number you chose while writing — a bake period, a waiting window, a threshold, a version — is a claim like any other: it gets an evidence row stating its basis, or it becomes a declared gap. A plausible reason attached to a number nobody agreed is still a number nobody agreed. diff --git a/plugins/fd3/workflows/implement-run.js b/plugins/fd3/workflows/implement-run.js index c0df7d6..c68d36d 100644 --- a/plugins/fd3/workflows/implement-run.js +++ b/plugins/fd3/workflows/implement-run.js @@ -174,8 +174,10 @@ const baselinePrompt = (repo) => `1. Create a worktree at ${worktreePath(repo, 'baseline')} from ${repoDefault(repo)}`, ` (git worktree add ) unless it already exists — then reuse it as is.`, `2. Run every runnable validation command from the toolchain report below, in the reported`, - ` order, sequentially — never in parallel. Skip what the report lists as not runnable here,`, - ` recording each skip under skipped with its reason — a skip is never recorded as passed.`, + ` 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`, + ` report lists as not runnable here, recording each skip under skipped with its reason — a`, + ` skip is never recorded as passed.`, `3. Fix nothing, change nothing. Run each command once, as \` 2>&1; echo "exit $?"\``, ` — that one run gives both the output and the exit status. Record, per command, whether it`, ` exited 0, and for each failure the output lines that matter.`, @@ -377,9 +379,9 @@ const mergePrompt = (repo, repoTasks) => { ` "Already up to date" is expected on a resumed run — count its task as merged.`, `4. For every task now merged in, including one that was already up to date, set`, ` \`status: merged\` in the task file listed beside it, changing nothing else in that file.`, - ` The task files are the run's state store, they live outside the repository, and they are`, - ` never committed — a status left at implemented after the merge is the one thing a later`, - ` reader cannot tell from a merge that never happened.`, + ` The task files are the run's state store and you never commit them, wherever they live —`, + ` a status left at implemented after the merge is the one thing a later reader cannot tell`, + ` from a merge that never happened.`, ``, `Resolve a merge conflict only when the two sides are clearly compatible and the resolution`, `is mechanical; commit the resolution and record it under resolved — the task's slug and one`, @@ -561,10 +563,12 @@ await baselineReady const CI_RESULT = { type: 'object', - required: ['passed', 'failures'], + required: ['passed', 'failures', 'branch', 'dirty'], properties: { passed: { type: 'boolean', description: 'true when nothing fails beyond the baseline' }, failures: { type: 'array', items: { type: 'string' }, description: 'one entry per newly failing command, with the load-bearing output lines' }, + branch: { type: 'string', description: '`git branch --show-current` in the worktree, read before the first command; `detached` when HEAD is detached' }, + dirty: { type: 'string', description: '`git status --porcelain` in the worktree after the last command, verbatim; an empty string when the tree is clean' }, preExisting: { type: 'array', items: { type: 'string' }, description: 'failures that match the baseline of the clean base — informational, never fixed on this branch' }, skipped: { type: 'array', items: { type: 'string' }, description: 'commands not run, each with the reason — a skip is never reported as passed' }, marked: { type: 'boolean', description: 'the task files were set to done; asked for on a final gate only' }, @@ -588,11 +592,39 @@ const CR_RESULT = { }, } -const ciPrompt = (unit, mode, markFiles) => - [ +// A branch that is checked out in the repository itself has no worktree of its own, and that +// checkout is the user's: their uncommitted work, and often the tasks directory, sit in the tree +// the commands would grade. Validate it in a detached worktree at the branch's commit instead — +// `git worktree add --detach` is allowed for a branch checked out elsewhere, and what it holds is +// exactly what the branch holds. Fixes still land in the checkout; the next run refreshes this one. +const validationTree = (unit) => + unit.worktree === unit.repo ? `${unit.repo}.worktrees/${unit.branch.replace(/\//g, '-')}-validate` : unit.worktree + +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 ${unit.worktree}. Run them in the reported order, sequentially — never in`, - `parallel. Toolchain report for this repository:`, + `in the worktree ${tree}. Run them in the reported order, sequentially — never in`, + `parallel.`, + ``, + ...(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.`, + ``, + ]), + `\`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`, + `found (or the short HEAD sha when detached) as branch, with passed=false and the mismatch in`, + `failures. Every command runs from that worktree: each command's cwd in the report is relative`, + `to the repository root, so resolve it there — never against ${unit.repo}, which is a`, + `different checkout on a different branch.`, + ``, + `Toolchain report for this repository:`, ``, toolchain.get(unit.repo), ``, @@ -608,9 +640,14 @@ const ciPrompt = (unit, mode, markFiles) => ``, `Skip everything the report lists as not runnable here, and skip a command the baseline`, `shows failing before it produces a verdict — re-proving a baseline failure is wasted time.`, - `Every skip goes under skipped with its reason; a skip is never reported as passed. Do not`, - `fix anything. A failure whose location and message match the baseline is pre-existing:`, - `return it under preExisting, never under failures, and do not count it against the branch.`, + `Every skip goes under skipped with its reason; a skip is never reported as passed. A failure`, + `whose location and message match the baseline is pre-existing: return it under preExisting,`, + `never under failures, and do not count it against the branch.`, + ``, + `Do not fix anything. Editing a source file, applying a formatter, and regenerating a derived`, + `artifact a command compares against — an index, a schema, a lockfile — are all fixing: report`, + `the failure and leave it. A verdict is only worth what the tree it ran on was, so when the`, + `last command has run, \`git status --porcelain\` and return its output verbatim as dirty.`, `Return passed=true only when every runnable command exits 0 or fails only on baseline`, `entries; otherwise return each newly failing command with the output lines that matter.`, ...(markFiles @@ -620,12 +657,14 @@ const ciPrompt = (unit, mode, markFiles) => `\`status: done\` in the frontmatter of these task files, changing nothing else in them,`, `and return marked=true:`, ...unit.tasks.map((slug) => `- ${tasks.find((t) => t.slug === slug).file}`), - `They are this run's state store: they live outside the repository, they are never`, - `committed, and the no-fixing rule above is about the code, not about them. On any`, - `failure leave them untouched and return marked=false.`, + `They are this run's state store, and the no-fixing rule above is about the code, not`, + `about them: edit them at the absolute paths listed, commit nothing, and if they happen`, + `to sit inside a checkout of this repository, leave that checkout's other files alone.`, + `On any failure leave them untouched and return marked=false.`, ] : []), ].join('\n') +} const fixPrompt = (unit, problems, source) => [ @@ -681,6 +720,43 @@ const reviewPrompt = (unit, skillName) => 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 +// repository's main checkout graded another branch's code, and one that edited its way to green +// graded a state no commit holds — both are absence of evidence, never a pass. +const taskFilePaths = new Set(tasks.map((t) => t.file)) +const porcelainPath = (line) => { + const p = line.length > 3 ? line.slice(3) : '' + const renamed = p.indexOf(' -> ') + return (renamed === -1 ? p : p.slice(renamed + 4)).replace(/^"|"$/g, '') +} +// The task files are the run's state store and the CI agent itself flips them to done, so their +// own dirtiness is expected; anything else in the tree is the runner's edit. +const strayChanges = (dirty, worktree) => + (dirty || '') + .split('\n') + .filter((line) => line.trim()) + .map(porcelainPath) + .filter((p) => p && !taskFilePaths.has(`${worktree}/${p}`)) + +const ciFault = (ci, unit) => { + const ran = (ci.branch || '').trim() + if (ran && ran !== unit.branch) return `ran in a checkout on ${ran} instead of ${unit.branch}` + if (!ran) return `could not name the branch it ran on` + const stray = strayChanges(ci.dirty, validationTree(unit)) + if (stray.length > 0) { + const shown = stray.slice(0, 5).join(', ') + return `left ${stray.length} uncommitted change(s) in the worktree (${shown}${stray.length > 5 ? ', …' : ''}), so its verdict describes a tree no commit holds` + } + return null +} + +const runCi = async (unit, mode, markFiles, label) => { + const ci = await tryTwice(ciPrompt(unit, mode, markFiles), { label, phase: 'Validate', schema: CI_RESULT, ...mechanical }) + if (!ci) return { ci: null, fault: null } + const fault = ciFault(ci, unit) + return { ci, fault } +} + const validation = [] // per-branch summary for the final report const REFRESH_RESULT = { @@ -728,20 +804,22 @@ for (const unit of units) { } } - let ci = await tryTwice(ciPrompt(unit, 'scoped', false), { label: `ci:${tag}`, phase: 'Validate', schema: CI_RESULT, ...mechanical }) - while (ci && !ci.passed && summary.fixRounds < maxFixRounds) { + let { ci, fault } = await runCi(unit, 'scoped', false, `ci:${tag}`) + while (ci && !fault && !ci.passed && summary.fixRounds < maxFixRounds) { summary.fixRounds += 1 const fix = await tryTwice(fixPrompt(unit, ci.failures, 'CI'), { label: `fix-ci:${tag}#${summary.fixRounds}`, phase: 'Validate', schema: FIX_RESULT }) if (fix && fix.caveats) caveats.push(...fix.caveats.map((c) => `${unit.branch} fix-ci: ${c}`)) - ci = await tryTwice(ciPrompt(unit, 'scoped', false), { label: `ci:${tag}#${summary.fixRounds + 1}`, phase: 'Validate', schema: CI_RESULT, ...mechanical }) + ;({ ci, fault } = await runCi(unit, 'scoped', false, `ci:${tag}#${summary.fixRounds + 1}`)) } - if (!ci) { + if (!ci || fault) { summary.ci = 'no-verdict' hil.push({ slug: null, kind: 'no-verdict', stage: 'ci', - reason: `${unit.repo} ${unit.branch}: the CI agent returned no result after a retry (transient API failure); the branch has no verdict after ${summary.fixRounds} fix rounds — absence of evidence, not a failure.`, + reason: fault + ? `${unit.repo} ${unit.branch}: the CI agent ${fault}; its verdict was discarded after ${summary.fixRounds} fix rounds — the branch is unvalidated, not failing.` + : `${unit.repo} ${unit.branch}: the CI agent returned no result after a retry (transient API failure); the branch has no verdict after ${summary.fixRounds} fix rounds — absence of evidence, not a failure.`, }) continue } @@ -776,22 +854,24 @@ for (const unit of units) { } // The full command list is the branch's final gate — always, review fixes or not. - let finalCi = await tryTwice(ciPrompt(unit, 'full', deadLenses.length === 0), { label: `ci:${tag}:final`, phase: 'Validate', schema: CI_RESULT, ...mechanical }) - if (finalCi && !finalCi.passed) { + let { ci: finalCi, fault: finalFault } = await runCi(unit, 'full', deadLenses.length === 0, `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}`)) - finalCi = await tryTwice(ciPrompt(unit, 'full', deadLenses.length === 0), { label: `ci:${tag}:final#2`, phase: 'Validate', schema: CI_RESULT, ...mechanical }) + ;({ ci: finalCi, fault: finalFault } = await runCi(unit, 'full', deadLenses.length === 0, `ci:${tag}:final#2`)) } - if (!finalCi) { + if (!finalCi || finalFault) { summary.ci = 'no-verdict' hil.push({ slug: null, kind: 'no-verdict', stage: 'ci-final', - reason: `${unit.repo} ${unit.branch}: scoped CI passed but the full-gate agent returned no result after a retry; the branch has no final verdict.`, + reason: finalFault + ? `${unit.repo} ${unit.branch}: scoped CI passed but the full-gate agent ${finalFault}; its verdict was discarded and the branch has no final verdict.` + : `${unit.repo} ${unit.branch}: scoped CI passed but the full-gate agent returned no result after a retry; the branch has no final verdict.`, }) continue } diff --git a/plugins/fd3/workflows/repair-run.js b/plugins/fd3/workflows/repair-run.js index cd9e92f..2ad2720 100644 --- a/plugins/fd3/workflows/repair-run.js +++ b/plugins/fd3/workflows/repair-run.js @@ -147,8 +147,10 @@ const baselinePrompt = (repo) => `1. Create a worktree at ${worktreePath(repo, 'baseline')} from ${repoDefault(repo)}`, ` (git worktree add ) unless it already exists — then reuse it as is.`, `2. Run every runnable validation command from the toolchain report below, in the reported`, - ` order, sequentially — never in parallel. Skip what the report lists as not runnable here,`, - ` recording each skip under skipped with its reason — a skip is never recorded as passed.`, + ` 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`, + ` report lists as not runnable here, recording each skip under skipped with its reason — a`, + ` skip is never recorded as passed.`, `3. Fix nothing, change nothing. Run each command once, as \` 2>&1; echo "exit $?"\``, ` — that one run gives both the output and the exit status. Record, per command, whether it`, ` exited 0, and for each failure the output lines that matter.`, @@ -260,20 +262,48 @@ await baselineReady const CI_RESULT = { type: 'object', - required: ['passed', 'failures'], + required: ['passed', 'failures', 'branch', 'dirty'], properties: { passed: { type: 'boolean', description: 'true when nothing fails beyond the baseline' }, failures: { type: 'array', items: { type: 'string' }, description: 'one entry per newly failing command, with the load-bearing output lines' }, + branch: { type: 'string', description: '`git branch --show-current` in the worktree, read before the first command; `detached` when HEAD is detached' }, + dirty: { type: 'string', description: '`git status --porcelain` in the worktree after the last command, verbatim; an empty string when the tree is clean' }, preExisting: { type: 'array', items: { type: 'string' }, description: 'failures that match the baseline of the clean base — informational, never fixed on this branch' }, marked: { type: 'boolean', description: 'the task files were set to done; asked for on a final gate only' }, }, } -const ciPrompt = (unit, mode, markFiles) => - [ +// A branch checked out in the repository itself has no worktree of its own, and that checkout is +// the user's: their uncommitted work sits in the tree the commands would grade. Validate it in a +// detached worktree at the branch's commit — repairs still land in the checkout. +const validationTree = (unit) => + unit.worktree === unit.repo ? `${unit.repo}.worktrees/${unit.branch.replace(/\//g, '-')}-validate` : unit.worktree + +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 ${unit.worktree}. Run them in the reported order, sequentially — never in`, - `parallel. Toolchain report for this repository:`, + `in the worktree ${tree}. Run them in the reported order, sequentially — never in`, + `parallel.`, + ``, + ...(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.`, + ``, + ]), + `\`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`, + `found (or the short HEAD sha when detached) as branch, with passed=false and the mismatch in`, + `failures. Every command runs from that worktree: each command's cwd in the report is relative`, + `to the repository root, so resolve it there — never against ${unit.repo}, which is a`, + `different checkout on a different branch.`, + ``, + `Toolchain report for this repository:`, ``, toolchain.get(unit.repo), ``, @@ -289,9 +319,14 @@ const ciPrompt = (unit, mode, markFiles) => ``, `Skip everything the report lists as not runnable here, and skip a command the baseline`, `shows failing before it produces a verdict — re-proving a baseline failure is wasted time.`, - `Every skip goes under skipped with its reason; a skip is never reported as passed. Do not`, - `fix anything. A failure whose location and message match the baseline is pre-existing:`, - `return it under preExisting, never under failures, and do not count it against the branch.`, + `Every skip goes under skipped with its reason; a skip is never reported as passed. A failure`, + `whose location and message match the baseline is pre-existing: return it under preExisting,`, + `never under failures, and do not count it against the branch.`, + ``, + `Do not fix anything. Editing a source file, applying a formatter, and regenerating a derived`, + `artifact a command compares against — an index, a schema, a lockfile — are all fixing: report`, + `the failure and leave it. A verdict is only worth what the tree it ran on was, so when the`, + `last command has run, \`git status --porcelain\` and return its output verbatim as dirty.`, `Return passed=true only when every runnable command exits 0 or fails only on baseline`, `entries; otherwise return each newly failing command with the output lines that matter.`, ...(markFiles @@ -301,12 +336,14 @@ const ciPrompt = (unit, mode, markFiles) => `\`status: done\` in the frontmatter of these task files, changing nothing else in them,`, `and return marked=true:`, ...unit.taskFiles.map((f) => `- ${f}`), - `They are the run's state store: they live outside the repository, they are never`, - `committed, and the no-fixing rule above is about the code, not about them. On any`, - `failure leave them untouched and return marked=false.`, + `They are the run's state store, and the no-fixing rule above is about the code, not`, + `about them: edit them at the absolute paths listed, commit nothing, and if they happen`, + `to sit inside a checkout of this repository, leave that checkout's other files alone.`, + `On any failure leave them untouched and return marked=false.`, ] : []), ].join('\n') +} const FIX_RESULT = { type: 'object', @@ -345,6 +382,40 @@ const fixPrompt = (unit, problems) => 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 +// repository's main checkout graded another branch's code, and one that edited its way to green +// graded a state no commit holds — both are absence of evidence, never a pass. +const porcelainPath = (line) => { + const p = line.length > 3 ? line.slice(3) : '' + const renamed = p.indexOf(' -> ') + return (renamed === -1 ? p : p.slice(renamed + 4)).replace(/^"|"$/g, '') +} +const ciFault = (ci, unit) => { + const ran = (ci.branch || '').trim() + if (ran && ran !== unit.branch) return `ran in a checkout on ${ran} instead of ${unit.branch}` + if (!ran) return `could not name the branch it ran on` + // The final gate flips this branch's task files to done itself, so their own dirtiness is + // expected wherever the tasks directory happens to live; anything else is the runner's edit. + const own = new Set(unit.taskFiles || []) + const tree = validationTree(unit) + const stray = (ci.dirty || '') + .split('\n') + .filter((line) => line.trim()) + .map(porcelainPath) + .filter((p) => p && !own.has(`${tree}/${p}`)) + if (stray.length > 0) { + const shown = stray.slice(0, 5).join(', ') + return `left ${stray.length} uncommitted change(s) in the worktree (${shown}${stray.length > 5 ? ', …' : ''}), so its verdict describes a tree no commit holds` + } + return null +} + +const runCi = async (unit, mode, markFiles, label) => { + const ci = await tryTwice(ciPrompt(unit, mode, markFiles), { label, phase: 'Validate', schema: CI_RESULT, ...mechanical }) + if (!ci) return { ci: null, fault: null } + return { ci, fault: ciFault(ci, unit) } +} + const validation = [] // per-branch summary for the final report for (const unit of units) { @@ -357,20 +428,22 @@ for (const unit of units) { continue } - let ci = await tryTwice(ciPrompt(unit, 'scoped', false), { label: `ci:${tag}`, phase: 'Validate', schema: CI_RESULT, ...mechanical }) - while (ci && !ci.passed && summary.fixRounds < maxFixRounds) { + let { ci, fault } = await runCi(unit, 'scoped', false, `ci:${tag}`) + while (ci && !fault && !ci.passed && summary.fixRounds < maxFixRounds) { summary.fixRounds += 1 const fix = await tryTwice(fixPrompt(unit, ci.failures), { label: `fix-ci:${tag}#${summary.fixRounds}`, phase: 'Validate', schema: FIX_RESULT }) if (fix && fix.caveats) caveats.push(...fix.caveats.map((c) => `${unit.branch} fix-ci: ${c}`)) - ci = await tryTwice(ciPrompt(unit, 'scoped', false), { label: `ci:${tag}#${summary.fixRounds + 1}`, phase: 'Validate', schema: CI_RESULT, ...mechanical }) + ;({ ci, fault } = await runCi(unit, 'scoped', false, `ci:${tag}#${summary.fixRounds + 1}`)) } - if (!ci) { + if (!ci || fault) { summary.ci = 'no-verdict' hil.push({ slug: null, kind: 'no-verdict', stage: 'ci', - reason: `${unit.repo} ${unit.branch}: the CI agent returned no result after a retry (transient API failure); the branch has no verdict after ${summary.fixRounds} fix rounds — absence of evidence, not a failure.`, + reason: fault + ? `${unit.repo} ${unit.branch}: the CI agent ${fault}; its verdict was discarded after ${summary.fixRounds} fix rounds — the branch is unvalidated, not failing.` + : `${unit.repo} ${unit.branch}: the CI agent returned no result after a retry (transient API failure); the branch has no verdict after ${summary.fixRounds} fix rounds — absence of evidence, not a failure.`, }) continue } @@ -386,14 +459,16 @@ for (const unit of units) { } // The full command list is the branch's final gate — repairs go out only fully validated. - const finalCi = await tryTwice(ciPrompt(unit, 'full', !!(unit.taskFiles && unit.taskFiles.length > 0)), { label: `ci:${tag}:final`, phase: 'Validate', schema: CI_RESULT, ...mechanical }) - if (!finalCi) { + const { ci: finalCi, fault: finalFault } = await runCi(unit, 'full', !!(unit.taskFiles && unit.taskFiles.length > 0), `ci:${tag}:final`) + if (!finalCi || finalFault) { summary.ci = 'no-verdict' hil.push({ slug: null, kind: 'no-verdict', stage: 'ci-final', - reason: `${unit.repo} ${unit.branch}: scoped CI passed but the full-gate agent returned no result after a retry; the branch has no final verdict.`, + reason: finalFault + ? `${unit.repo} ${unit.branch}: scoped CI passed but the full-gate agent ${finalFault}; its verdict was discarded and the branch has no final verdict.` + : `${unit.repo} ${unit.branch}: scoped CI passed but the full-gate agent returned no result after a retry; the branch has no final verdict.`, }) continue }