diff --git a/CLAUDE.md b/CLAUDE.md index debd298..663ce62 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -79,6 +79,93 @@ clean or clearly stuck → then STOP and surface to the user. The only early exi below 3 is a pass with **zero** findings; don't manufacture findings to pad. Codex is advisory — validate before applying; dismissed finding → one-line why. +**Findings go to a FILE, not the response — both gates.** In the field, long finding +lists came back cut off on effectively every substantial Gate A pass, and a cut that +lands between findings is indistinguishable from a short list: silently dropped +findings, the dangerous direction. Claude Code both limits MCP tool output (25,000 +tokens by default, `MAX_MCP_OUTPUT_TOKENS`) and persists over-threshold results to disk +behind a file reference; which one produced the field loss is not established, and the +protocol is correct either way, because the response stops carrying the findings at all. +Append to the gate prompt: + +> Pass the reviewed repo root as `workingDirectory`. Write the FULL findings list to +> `.context/codex-reviews/.md` (create the directory if needed; the path is +> relative to that root — Codex resolves writes against its working directory, so +> without this a valid file can land in a different checkout). `` is +> `gate-a-spec-pass-

`, `gate-a-plan-pass-

`, or `gate-b--pass-

`. +> +> One finding per line in the format above; escape a literal pipe inside a field as +> `\|`. Every line before the terminator is exactly one finding line — no blank lines, +> headings, prose or wrapped continuations. End the file with a final line reading +> exactly `END OF FINDINGS ( total)`, `` being the number of finding lines. A +> clean pass is the single body line `NO FINDINGS` with `END OF FINDINGS (0 total)`. +> +> Then reply with ONLY one line per branch — ` | pass

| findings | ` +> — or `INCOMPLETE | | ` if you could not write the file. An unwritten +> file behind a normal-looking reply is the one outcome the reader cannot diagnose. + +**Gate B takes one file per branch** because `reviewType: full` runs the spec and +quality reviewers in parallel from one `additionalContext`. Aimed at a single path they +race, and the second writer leaves a correctly terminated, correctly counted file +holding half the findings — with every check still passing. + +**Before each call, delete every target file and confirm it is gone.** A call that dies +part-way leaves the prior attempt's valid file behind, and no terminator can tell that +from a fresh one. If a target survives deletion, stop and name the cause — path resolved +against the wrong root, permissions, a *directory* at the target, or a file reappearing +(another writer) each need a different fix, and "delete failed" alone sends you retrying +the delete. Run one pass at a time: the slot name has no invocation-unique component, so +two concurrent calls on one slot race. Passes are sequential by construction, so that is +a stated limitation, not a guarded one. + +**Accept a pass only when** the file exists and is readable; its last line is exactly +`END OF FINDINGS ( total)`; it contains exactly `` finding lines *and nothing +else* (or the single line `NO FINDINGS` when `` is 0 — "n valid lines somewhere in +the file" would accept a truncated file padded with fragments); and, for a `full` Gate-B +pass, both branch files satisfy all of that. Anything else — missing, unreadable or +empty file, wrong path, malformed terminator, count mismatch, extra lines, one branch +file, an `INCOMPLETE` reply — is an **INCOMPLETE pass**, which is not a review: don't +act on the partial list, don't count it toward the 3-pass floor, and don't read "no +Blocker/Major visible" as clean. + +**Recovery: one attempt per pass**, shared across timeout, an `INCOMPLETE` reply and +failed validation — the Mechanics timeout-retry rule widened, not a second budget beside +it, since two budgets let a pass alternate between them indefinitely. The attempt is a +fresh re-run, deleting exactly what it will rewrite: both branch files for a full +re-run, only the failed branch for a single-branch resume — deleting both and recreating +one makes the both-files check fail by construction, spending the attempt on a path that +cannot succeed. Prefer a resume only when the reply shows the review ran and just the +write failed: pass the `sessionId` from the original tool result back, and for a Gate-B +branch pass its `reviewType` alongside (`spec` with `specSessionId`, `quality` with +`qualitySessionId`) — the tool defaults to `full`, and a resume that omits it can run the +other reviewer and write the wrong slot, which no check detects, because the file is +well-formed and merely from the wrong branch. Whether a resumed session re-executes the +write or just returns its prior summary is not established; if it returns the summary, +that was the attempt. Spent and still incomplete → STOP and surface, naming which check +failed. + +**What this does not do.** The hook counts on `PostToolUse`, keyed on tool name, and +never sees the file. Claude Code fires `PostToolUse` after a *successful* call and routes +a failed one to `PostToolUseFailure`, which the plugin registers no handler for — but do +not infer from that which failures escape counting: the pinned `mcp-codex-dev` catches +its own errors, executor timeouts and aborts included, and returns them as a normal +result carrying `success: false` rather than throwing or setting `isError` +(`dist/tools/codex-review.js`). A failed review therefore looks like a successful tool +call and increments the counter. So does a call that returns and then fails validation. +The rule that follows is the simple one: **discount every incomplete pass regardless of +what the counter says** — a "satisfied" count can overstate the passes you actually hold, +and reasoning about which failure took which event path will get it wrong. Nothing checks the terminator mechanically; this is +instruction-backed by design, and a recurring truncation incident is the trigger to build +the checker, not a reason to build it now. Detection is conditional: it catches an absent +or malformed terminator, a count mismatch and a missing branch file *in the artifact you +actually read*; it does not catch a model that writes a wrong count with a matching +number of lines, nor a stale file if you skip the delete. + +**Why `.context/codex-reviews/`:** with the current `tree_hash()`, changes confined to +`.context/` move none of its three fingerprint inputs, so review artifacts cannot +invalidate the review they document. Ignore `/.context/codex-reviews/` specifically — +not all of `.context/`, which would strip the committed `codex-gate.on` adoption marker. + - **Gate A — Spec, then plan (TWO runs, each its own 3-pass loop).** Run on the **spec** right after brainstorming (before `writing-plans`), then on the **plan** before `executing-plans`/`subagent-driven-development` — catching a @@ -162,9 +249,14 @@ advisory — validate before applying; dismissed finding → one-line why. follow-up commit for two reasons: a `WIP: …` commit left in history defeats the naming convention it exists for, and a follow-up commit has nothing to commit when the review produced no fixes. -- **Timeout retry:** a codex call that dies at the MCP tool-call timeout is retried - once before surfacing to the user — pass state persists in `.context/`, so an - aborted call loses nothing. +- **Timeout / abort:** a codex call that dies at the MCP tool-call timeout is retried + once before surfacing to the user, and that retry *is* the single shared recovery + attempt above — not a second one. An abort is an incomplete pass, so treat it as one: + it may already have moved the hook's counter (the pinned server returns its own + timeouts as ordinary results), and it may have left a partial or stale target file, so + delete the targets and confirm them gone before retrying, then validate the result like + any other pass. Counter and workspace state persist in `.context/`; the *pass* does + not. --- diff --git a/README.md b/README.md index 4796dac..184dbc3 100644 --- a/README.md +++ b/README.md @@ -61,6 +61,16 @@ old version until you restart it or run `/reload-plugins`. `codex mcp-server` is a *different* server exposing a single `codex` tool, which can't be attributed to Gate A (reviews text) or Gate B (reviews a diff): with it connected, no pass ever counts and Gate B reports "not run" forever (the hook says so, once). + + Both gates have Codex write its findings to a file under `.context/codex-reviews/` and + reply with a one-line summary, because a long finding list returned inline can come + back cut off — and a cut between findings looks exactly like a short list. Claude Code + limits MCP tool output to 25,000 tokens by default; `MAX_MCP_OUTPUT_TOKENS` raises + that, which is worth trying as a *secondary* mitigation but is not the fix. It moves + the ceiling rather than removing it, and it applies only to tools that don't declare + their own text limit via `anthropic/maxResultSizeChars` — whether `mcp-codex-dev` + declares one is not established here. The file protocol is what actually keeps + findings out of the response. - **`gh`** — optional; only `/dev-workflow:process-pr-review` uses it. **3. Run `/dev-workflow:workflow-init` in each project.** It verifies the rest and tells diff --git a/docs/hardening-log.md b/docs/hardening-log.md index d478033..058a469 100644 --- a/docs/hardening-log.md +++ b/docs/hardening-log.md @@ -23,3 +23,4 @@ escape `\|`, one line), `source` (gate-a|gate-b|bot|manual), | 2026-07-19 | false-negative-gate | both Gate-B hash components described the worktree while `git commit` commits the index, so staging a change and reverting the file on disk reported satisfied on unreviewed content | gate-b | blocker | 4 test | plugins/dev-workflow/hooks/codex-gate.test.sh sections 24-30 + AGENTS.md invariant 3 (amended to name all three components). What the tests DO cover: the index component exists and moves on divergence, the failure marker never self-matches, and the ambient-alternate-index and three-way-`.context` states. What they do NOT: seven of `tree_hash`'s failure seams have no targeted test — `mktemp -d`, the non-symbolic unresolvable-HEAD branch, the throwaway-index `git rm --cached`, each `write-tree` separately, `git add -A`, and the buffered-stream redirect (enumerated in todos.md by derivation after three attempts understated it from memory); every divergence shape (they pin the mechanism, not exhaustive enumeration); mutation after the PreToolUse event, which is the parked compound-command row; command-local retargeting (`GIT_INDEX_FILE=`/`-C`/`--git-dir`), split to the 2026-07-19 command-retargeting-guard story; and a `git add`/`write-tree` failure inside the throwaway index, which stays parked | | 2026-07-19 | unverified-enforcement-claim | across four Gate-B rounds on the index-tree documentation task, prose repeatedly claimed more than the hook guarantees, and each fix introduced a subtler version of the same overclaim — "the tree-hash proves what was reviewed", then "what is being committed is what was reviewed", then "what a commit would actually carry", then "everything a commit could carry"; every round searched for the previous PHRASE rather than the CLAIM, so a synonym survived each time | gate-b | major | 1 prose | AGENTS.md Don'ts, "Never describe what a gate proves without checking what it actually compares" — a rule plus a by-meaning grep recipe, sibling to the existing manifest-claims rule. Deliberately NOT another banned phrase: adding one phrase per round is the same rung a fifth time. What it does NOT do, in the rule text as well as here: nothing runs it in CI (a human runs it), and an overclaim phrased without those totality words escapes it. It raises the floor; it does not close the class | | 2026-07-20 | verification-masks-failure | the plan's mutation-verification steps read `sh …codex-gate.test.sh 2>&1 \| grep -cE '^FAIL'`, which reports the GREP's status, not the runner's — a suite that died part-way, or exited non-zero without printing `FAIL`, would read as 0 and be recorded as green. Found by CodeRabbit on PR #8, in the very steps used to prove this branch's guards were load-bearing | bot | major | 1 prose | plan Task 1 Step 3 + the two later mutation steps now capture `exit=$?` separately into a variable and RETURN success only when the status and the FAIL count are both zero (per-run `mktemp` file, not a shared path), with the reason stated inline. An earlier revision of this row said they "assert both" when they only printed both and still returned grep's status — caught by Gate B, in the row recording this very class. Scope of the fix, plainly: it corrects the three sites in this plan and the rationale a future plan author reads — nothing checks new plans for the same shape, so a plan written tomorrow can reintroduce it | +| 2026-07-20 | truncated-tool-output-read-as-complete | Codex Gate A responses arrived cut off on long finding lists — on effectively every substantial pass in real project use — and a cut landing between findings is indistinguishable from a short list, so dropped findings read as a clean review; field agents were improvising a write-to-file workaround per session, which means the failure was handled only when someone happened to notice | manual | major | P std | CLAUDE.md §5 "Findings go to a FILE, not the response" + the same block in the workflow-init inline template: Codex writes the full list to `.context/codex-reviews/.md` ending `END OF FINDINGS ( total)`, replies with one line, and the reader accepts only on terminator-present AND count-matching AND nothing-but-finding-lines AND (for Gate-B `full`) both branch files. Gate B needs one file per branch because `reviewType: full` runs two reviewers in parallel off one additionalContext — a shared path lets the second overwrite the first and still pass every check. What it does NOT do: nothing checks the terminator mechanically (instruction-backed by design; a recurrence is the trigger to build the checker); the hook counts on `PostToolUse` and never sees the file, so an incomplete pass still increments the counter — including a FAILED review, because the pinned `mcp-codex-dev@1.0.1` catches its own errors, executor timeouts and aborts included, and returns `{success: false}` as a normal result rather than throwing, so Claude Code reads it as a successful tool call (an earlier draft of this row inferred from the documented `PostToolUseFailure` split that errors increment nothing — Gate B caught it by reading the server; the rule is now stated without reference to event routing: discount every incomplete pass whatever the counter says); and detection misses a model writing a wrong count with matching lines, or a stale file if the pre-call delete is skipped. Secondary mitigation only, in README: `MAX_MCP_OUTPUT_TOKENS` (25,000 default) moves the ceiling, does not remove it, and does not apply to tools declaring `anthropic/maxResultSizeChars`. Which documented mechanism (token limit vs persist-to-disk threshold) caused the field loss was NOT established; the protocol is correct under either. Fingerprint minted rather than filed under `verification-masks-failure`: that would make this a recurrence on a `1 prose` row, and the skill's recurrence rule then demands a mechanical rung or sharpened AGENTS.md wording, neither of which is P — three Gate-A passes each rejected a different attempt to narrate that gap, which is the class being bent to fit the ladder rather than the defect | diff --git a/docs/hardening-taxonomy.md b/docs/hardening-taxonomy.md index ee98a57..00bba1c 100644 --- a/docs/hardening-taxonomy.md +++ b/docs/hardening-taxonomy.md @@ -49,6 +49,29 @@ the synonyms a future reader might search for instead. fix is to force the version forward. Grep this one when the sentence is "nobody got the change"; grep the other when it is "we don't know what we got". +- `truncated-tool-output-read-as-complete` — a tool result that arrived incomplete is + consumed as if whole, because nothing in it distinguishes the two. Aliases: cut-off + response, output limit hit, silent drop, partial list read as the full list, "the + reviewer only found three things", findings lost in transport, response persisted to + disk and never re-read. + + **Not the same as `verification-masks-failure`**, the nearest class, though the + surfaces rhyme — both end with a bad signal accepted as a good one. The difference is + origin and fix. There, an author wrote a check that reports the wrong thing (a pipe + returning grep's exit status instead of the runner's), and the fix is to make the + check report the real status. Here the author's reasoning is sound and the *delivery* + drops data underneath it, so the fix is to make incompleteness detectable — a + terminator, a declared count, a re-read. Grep this one when the sentence is "we never + saw all of it"; grep the other when it is "we looked at the wrong thing". + +- `verification-masks-failure` — a check reports success because of how it was wired, + not because the thing it checks succeeded. Aliases: exit status swallowed, pipeline + status masked, green that proves nothing, the check that cannot fail, asserting on the + wrong command's result. + + (Defined here retroactively: the 2026-07-20 ledger row used this class before any + definition existed. Recorded now so the recurrence grep has something to land on.) + **Promotion candidate.** These classes are stack-neutral, not project vocabulary, so they belong in the `harden-finding` base list rather than here. They live here because the skill says to mint into this file (the plugin ships the base classes, the project diff --git a/docs/superpowers/specs/2026-07-20-codex-file-first-output.md b/docs/superpowers/specs/2026-07-20-codex-file-first-output.md new file mode 100644 index 0000000..238c789 --- /dev/null +++ b/docs/superpowers/specs/2026-07-20-codex-file-first-output.md @@ -0,0 +1,319 @@ +# Spec — file-first output protocol for the Codex review gates + +Date: 2026-07-20 · Source: field finding (manual, major) · Hardening rung: **P std** + +## The finding + +**Observed, in the field:** `mcp__codex__exec` responses arrive cut off on long finding +lists — on effectively every substantial Gate A pass in real project use. Field agents +already work around it ad hoc by having Codex write output to a file. An improvised +workaround per session means the failure mode is handled only when someone notices — and +a cut that lands between findings is indistinguishable from a complete list: silently +dropped findings, the dangerous direction. + +**Documented, in current Claude Code docs** (https://code.claude.com/docs/en/mcp.md, +read 2026-07-20) — two separate mechanisms, neither of which the docs describe as +truncation: + +- a **token limit**: a warning above 10,000 tokens, and output "limited to 25,000 tokens + by default", raisable via `MAX_MCP_OUTPUT_TOKENS`. The docs state the limit; they do + not state what happens to output past it. +- a **persist-to-disk threshold** (character-based, separate from the token limit): + "results that exceed the default threshold are persisted to disk and replaced with a + file reference in the conversation", unless the tool declares + `_meta["anthropic/maxResultSizeChars"]` (hard ceiling 500,000 characters). + +**Not established:** which mechanism produced the field loss, or on which Claude Code +version — and nothing here examines the MCP transport protocol itself, so the loss is +not attributed to it. Stating this rather than asserting "the transport truncates" +matters because the wrong attribution sends the next reader to harden the wrong layer. + +**The hardening does not depend on the answer.** Keeping findings out of the response +entirely is correct whether an over-limit response is truncated in place, persisted to +disk behind a file reference, or dropped some third way: under this protocol each Codex +branch replies with one short line, so the response stays orders of magnitude below any +of these thresholds. "One short line" is the per-branch contract, not the shape of every +aggregate response — a Gate-B `reviewType: full` call returns the server's own merged +wrapper (its `Spec Compliance Review` / `Code Quality Review` headings and separator) +around the two branch lines, and the MCP layer serializes that into a JSON result. Still +bounded and small; just not literally one line, and the reader validates the two files +rather than parsing that wrapper. + +## Decision + +Both gate prompts move the findings **out of the MCP response and into a file**, with a +terminator the reader verifies. + +### The protocol (appended to the Gate A and Gate B prompts) + +> Write the FULL findings list to `.context/codex-reviews/.md` — create the +> directory if needed; the path is relative to the repo root. `` is: +> +> (Every gate call and every branch resume passes the reviewed repo root as +> `workingDirectory`, and the reader resolves the pre-call delete and the post-call read +> against that same absolute root. Codex resolves writes against its working directory, +> so without this a compliant model can write a perfectly valid file into a different +> checkout — the reader then sees a missing target and burns its one recovery attempt on +> a configuration mismatch no retry fixes.) +> +> | Gate | `` | +> |---|---| +> | Gate A, spec run | `gate-a-spec-pass-

` | +> | Gate A, plan run | `gate-a-plan-pass-

` | +> | Gate B | `gate-b--pass-

` | +> +> Write one finding per line, exactly in the format given above +> (`SEVERITY | confidence | location | what is wrong | why it matters | suggested fix`): +> one line per finding, no wrapped continuations, no blank lines, no headings, no other +> prose. Six fields, so a literal pipe inside any field is escaped `\|` — the same +> convention `docs/hardening-log.md` uses — otherwise a field containing a pipe reads as +> a seventh field and a well-formed finding looks malformed. End the file with a final +> line reading exactly `END OF FINDINGS ( total)`, where `` is the number of +> finding lines you wrote. For a clean pass the body is the single line `NO FINDINGS` +> and the terminator reads `END OF FINDINGS (0 total)`. +> +> Every line before the terminator is exactly one finding line — or, for a clean pass, +> the single line `NO FINDINGS`. No blank lines anywhere in the file. +> +> Then reply with ONLY one line — one line per Codex branch; a `full` Gate-B call wraps +> the two branch lines in the server's own merged-review formatting, which is expected — +> in one of these two forms: +> +> ```text +> | pass

| findings | +> INCOMPLETE | | +> ``` +> +> where `

` is the same decimal pass number used in the slot name, and `` +> carries no literal pipe (escape one as `\|`): +> +> for example: +> +> ```text +> gate-a-spec | pass 2 | 10 findings | .context/codex-reviews/gate-a-spec-pass-2.md +> INCOMPLETE | permission denied creating .context/codex-reviews/ | .context/codex-reviews/gate-b-spec-pass-1.md +> ``` +> +> Do not repeat the findings in the reply. Use the `INCOMPLETE` form whenever the file +> was not written in full — an unwritten file behind a normal-looking reply is the one +> outcome the reader cannot diagnose. + +**Gate B takes one file per reviewer branch** because `reviewType: full` runs the spec +and quality reviewers in parallel from one `additionalContext` (it returns +`specSessionId` and `qualitySessionId` separately). Pointed at a single shared path they +race, and the second writer leaves a correctly terminated, correctly counted file holding +only its own branch — half the findings gone, with every check still passing. With +`reviewType: spec` or `quality`, only that branch's file appears. + +**Delete every target file before each call, and confirm it is gone.** A call that dies +part-way leaves the previous attempt's valid file in place, and a terminator cannot +distinguish a stale complete file from a fresh one — so an undeleted target would +reinstate exactly the silent failure the delete exists to close. If a target still +exists after the delete, stop and surface it; do not call Codex, because from that point +on a valid-looking file proves nothing about this pass. Name the cause when you surface +it — the four an agent can tell apart are: the path resolved against the wrong directory +(check it against the repo root you passed as `workingDirectory`), a permissions failure +on the file or on `.context/codex-reviews/`, a *directory* sitting at the target path, +and a file that reappears after deletion (another writer — see the one-pass-at-a-time +limitation). Each takes a different fix, and "delete failed" alone sends the reader +retrying the delete. + +**One pass at a time.** The slot name has no invocation-unique component, so two +concurrent calls sharing a gate, review type and pass number would race for one path. +Passes are sequential by construction — you fix findings between them — so this is a +stated limitation, not a guarded one. + +### The reader's obligation + +The reader (Claude) accepts a pass only when **all** hold: + +1. the file exists and is readable; +2. its last line is exactly `END OF FINDINGS ( total)`; +3. every line before the terminator is exactly one finding line in the format above, and + there are exactly `` of them — or, when `` is `0`, the single body line + `NO FINDINGS`. No blank lines, headings, prose, or wrapped continuations: a file may + contain findings *and nothing else*, because "n valid lines somewhere in the file" + would accept a truncated file padded with fragments; +4. for a `reviewType: full` Gate-B pass, **both** branch files satisfy 1–3. + +Anything else — missing, unreadable, or empty file; wrong path; absent or malformed +terminator; count mismatch; extra lines; only one of two Gate-B branch files; an +`INCOMPLETE` reply — is an **INCOMPLETE pass**. An incomplete pass is not a review: do +not act on the partial list, do not count it toward the 3-pass floor, and do not read +"no Blocker/Major visible" as clean. + +**Recovery: one attempt per pass, covering every cause together** — timeout, an +`INCOMPLETE` reply, and any failure of checks 1–4 draw on the same single budget, so a +timeout that is retried and then returns a bad file has spent it. This is the existing +§5 Mechanics timeout-retry rule, widened to the new failure modes rather than added +alongside it; two separate budgets would let a pass alternate between them indefinitely. + +That Mechanics bullet is rewritten in the same change, because its old wording ("an +aborted call loses nothing") contradicts all of this: an abort is an incomplete pass, it +may already have moved the counter, and it may have left a partial target file. What +persists in `.context/` is counter and workspace state — not the pass. + +The recovery attempt is **a fresh re-run** of the call. Delete exactly what the re-run +will rewrite: a fresh `full` Gate-B re-run rewrites both branch files, so delete both; a +single-branch resume rewrites only that branch, so delete only that branch's invalid +file and **keep the sibling that already validated** — deleting both and recreating one +guarantees check 4 fails, spending the only attempt on a path that cannot succeed. + +Resuming the session is worth preferring in one case only: the reply shows the review +itself ran and only the write failed. Then pass the `sessionId` from the original tool +result back to `mcp__codex__exec` / `mcp__codex__review`. For a `full` Gate-B pass the +two branches resume separately and **the branch's `reviewType` must be passed with its +session id** — `reviewType: spec` with `specSessionId`, `reviewType: quality` with +`qualitySessionId`. The tool defaults `reviewType` to `full`, so a resume that omits it +can run a different reviewer than the branch you meant to recover and write the wrong +slot, which check 4 cannot detect: the file would be well-formed, just from the wrong +reviewer. + +State plainly what is not established: whether a resumed session re-executes the +file-writing instruction or merely returns its prior summary. If it returns the summary, +that is the spent attempt — which is why a fresh re-run is the default. +`mcp__codex__session_list` is a debugging aid for a human hunting a lost session, not +part of this path; it defaults to `status: active` while a session that returned +normally is already `completed`, and it offers no way to tell which of several completed +sessions was the one you want. + +Attempt spent and still incomplete → STOP and surface to the user, naming which of +checks 1–4 failed. + +### What this does NOT do (stated so nobody mistakes it for a guard) + +- **The hook still counts an incomplete pass — specifically the incomplete passes this + protocol is about.** Counters increment in the hook's `PostToolUse` branch + (`codex-gate.sh:361`), keyed on tool name; the hook never sees the response, let alone + the file. Claude Code fires `PostToolUse` after a tool call *succeeds* and routes a + failed call to `PostToolUseFailure`, for which `hooks/hooks.json` registers no handler. + **That does not mean failed reviews escape counting.** Verified in the pinned server's + source (`mcp-codex-dev@1.0.1`, `dist/tools/codex-review.js` and `codex-exec.js`): it + catches its own exceptions — executor timeouts and aborts among them — and *returns* + `{success: false, …}` as a normal result, without throwing and without setting + `isError`. Claude Code therefore sees a successful tool call, `PostToolUse` fires, and + the counter moves. A call that returns and then fails validation increments too. + + So the rule is stated without reference to event routing: **discount every incomplete + pass regardless of what the counter says.** A "satisfied" count can overstate the + passes you actually hold; discounting is yours to apply — the same instruction-backed + gap §5 already names for Gate A. An earlier revision of this spec claimed errors and + timeouts "increment nothing", inferred from the documented event split without reading + the server; Gate B caught it. The lesson is this repo's own Don't: a mechanism + documented one layer up does not tell you what the layer beneath actually does. +- **Nothing checks the terminator mechanically.** No script, no hook, no CI step. The + protocol is instruction-backed by design (scope guard on this hardening: prompts and + templates only). Whether that suffices is measured by whether truncation incidents + recur — a recurrence is the trigger to escalate a rung, not a reason to build the + checker now. +- **Detection is conditional, not total.** Checks 1–4 catch an absent or malformed + terminator, a count mismatch, and a missing branch file *in the artifact the reader + actually reads*. They do not catch a model that writes a wrong count with a matching + number of lines, and they do not catch a stale prior-attempt file if the pre-call + delete is skipped. The observed failure — output cut off mid-list — is inside that + coverage; model dishonesty is not. + +### Why `.context/codex-reviews/` + +With the current `tree_hash()` (0.5.0, `plugins/dev-workflow/hooks/codex-gate.sh`), +changes confined to `.context/` change none of its three fingerprint inputs — verified +in the source: `git diff HEAD -- . ':(exclude).context'`, +`git rm -rfq --cached … -- .context` before the index-tree `write-tree`, and +`git add -A -- . ':(exclude).context'` before the worktree-tree `write-tree`. So under +that version, writing review artifacts there does not move the Gate-B fingerprint. The +claim is about those three inputs at that version — nothing more. + +Ignore `.context/codex-reviews/` specifically, **not** all of `.context/`: +`/workflow-init` writes `.context/codex-gate.on` as the adoption marker and says to +commit it, so a blanket ignore would strip adoption from fresh clones. The exact entry +is the root-anchored `/.context/codex-reviews/`. + +`/workflow-init` writes no `.gitignore` today, and this change does not add that step — +scaffolding a new file is more than a prompt edit, and an unignored review directory +costs a noisy `git status`, nothing more. The template therefore *recommends* the entry +in prose. Should a later change scaffold it, invariant 9 applies: merge additively, +never overwrite. The hook's `.context/` exclusion above is independent of `.gitignore` +either way, which is why Gate-B validity does not depend on this paragraph. + +## Secondary mitigation (not the fix) + +README, Codex prerequisites: name `MAX_MCP_OUTPUT_TOKENS` and the 25,000-token default. +Raising it moves the ceiling; it does not remove it — and it moves it only for tools +that do not declare their own text limit. A tool setting +`_meta["anthropic/maxResultSizeChars"]` uses that value for text content regardless of +the environment variable (image output stays subject to it). Whether the pinned +`mcp-codex-dev@1.0.1` declares the annotation is **not established here**, so the README +must not promise the variable will help — it is a knob worth trying, and the file +protocol is the fix. + +## Fingerprint + +**Minted: `truncated-tool-output-read-as-complete`** — a tool result that arrived +incomplete is consumed as if whole, because nothing in it distinguishes the two. + +Reusing `verification-masks-failure` was tried first and abandoned. It is the nearest +existing class, and the surface shapes rhyme — a signal read as a pass that cannot +distinguish success from failure. But the two differ in origin and in fix: there, an +author wrote a check that reported the wrong thing (a pipe returning grep's status), and +the fix is to make the check report the real status; here, the author's reasoning is +sound and the *delivery* silently drops data, so the fix is to make incompleteness +detectable. That difference is what the taxonomy's existing "not the same as" paragraphs +exist to record. + +The deciding evidence was mechanical: forcing this into `verification-masks-failure` +makes it a recurrence on a `1 prose` row, and the skill's recurrence rule then demands +either a mechanical rung (out of scope here) or sharpened `AGENTS.md` wording with the +absence of a deterministic rung recorded. Rung P is neither. Three review passes each +rejected a different attempt to narrate that gap — the class was being bent to fit the +ladder rather than the defect. + +As a new class, no recurrence rule applies and **rung P is simply the fitting rung**: +the defect is in prompt artifacts (the gate prompts and the scaffolded template), which +is what P is for. A mechanical rung would be stronger, and is deferred with a named +trigger rather than dismissed: deciding whether a tool response was complete needs a +checker, which this hardening's scope guard excludes — if incidents recur under this +protocol, that checker is the escalation. + +Bookkeeping while in the taxonomy file: `verification-masks-failure` is used by the +2026-07-20 ledger row but was never defined there, and the new class's "not the same as" +paragraph points at it, so this change adds its definition too. (`false-negative-gate`, +used by the 2026-07-19 row, is undefined as well — noted, not fixed here.) + +## Change list + +| # | File | Change | +|---|---|---| +| 1 | `CLAUDE.md` §5 | protocol + reader obligation + does-NOT-do, in Gate A and Gate B | +| 2 | `plugins/dev-workflow/commands/workflow-init.md` (inline §5 template) | the same, phrased for a scaffolded project | +| 3 | `README.md` | `MAX_MCP_OUTPUT_TOKENS` as secondary mitigation | +| 4 | `docs/hardening-taxonomy.md` | define the minted `truncated-tool-output-read-as-complete` (aliases + the "not the same as" distinction), **and** the referenced-but-undefined `verification-masks-failure` | +| 5 | `docs/hardening-log.md` | one row | +| 6 | `plugins/dev-workflow/.claude-plugin/plugin.json` | 0.5.0 → 0.5.1 (invariant 12; the §5 template ships) | +| 7 | `plugins/dev-workflow/CHANGELOG.md` | 0.5.1 entry | + +**The scope guard, stated precisely.** "Prompt/template changes only" bounds the +*mechanism*, not the bookkeeping: no executable machinery (no hook change, no new +script, no CI checker, no terminator-checking tool) and no newly scaffolded project +files. In scope alongside the two prompt sites (1, 2), each for its own reason rather +than one blanket one: + +- **6, 7** (manifest version, changelog) — invariant 12 and release policy *require* + these of any shipped-plugin change. +- **4, 5** (taxonomy, ledger) — required by the `harden-finding` flow being followed, + which mints the class and appends the row in the same change. +- **3** (README) — neither: a design choice, the explicitly in-scope secondary + mitigation. + +Read literally as "only files that are prompts", the guard would forbid the version bump +invariant 12 mandates, which is not what it means. + +## Invariants touched + +- **11** (prompt changes pass `docs/prompt-standards.md`) — items 1–12 on the new text; + items 3 (stop conditions), 4 (output format with an example), 10 (diagnostic states + name their causes) and 11 (enforcement claims name their mechanism) are the ones this + protocol most directly exercises. +- **12** (plugin change requires a version bump) — 0.5.1. +- **8** (`/workflow-init` templates stay inline) — the change is inside the inline body. +- **3** (Gate-B validity is content-derived) — relied upon, not modified; the + `.context/` exclusion is what makes the file location safe. diff --git a/plugins/dev-workflow/.claude-plugin/plugin.json b/plugins/dev-workflow/.claude-plugin/plugin.json index a10195d..9ac4a2a 100644 --- a/plugins/dev-workflow/.claude-plugin/plugin.json +++ b/plugins/dev-workflow/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "dev-workflow", "displayName": "Cross-Model Review Workflow", - "version": "0.5.0", + "version": "0.5.1", "description": "Spec-driven workflow with two independent cross-model review gates, an append-only hardening ledger with an escalation ladder, and repo-enforced quality. Requires the superpowers plugin.", "author": { "name": "Daniel Sänger", diff --git a/plugins/dev-workflow/CHANGELOG.md b/plugins/dev-workflow/CHANGELOG.md index 4f4a23c..057cf66 100644 --- a/plugins/dev-workflow/CHANGELOG.md +++ b/plugins/dev-workflow/CHANGELOG.md @@ -22,6 +22,36 @@ unambiguously, still fails. Deleting only a plugin's *manifest* while the direct keeps shipping fails too. AGENTS.md invariant 12 carries the complete list. +## 0.5.1 + +- **Gate findings go to a file, not the MCP response.** Long finding lists came back cut + off on effectively every substantial Gate A pass in the field, and a cut landing + between findings looks exactly like a short list — so dropped findings read as a clean + review. Both gate prompts in the `/workflow-init` CLAUDE.md template now have Codex + write the full list to `.context/codex-reviews/.md`, end it with + `END OF FINDINGS ( total)`, and reply with one line. The reader accepts a pass only + when the terminator is present, the count matches, the file holds nothing but finding + lines, and — for a Gate-B `full` review — both branch files pass. +- **Gate B writes one file per reviewer branch.** `reviewType: full` runs the spec and + quality reviewers in parallel from a single `additionalContext`. Aimed at one path they + race, and the second writer leaves a correctly terminated, correctly counted file + holding half the findings, with every check still passing. +- **Bounded recovery.** An incomplete pass gets one attempt, shared with the existing + timeout retry rather than added beside it, then STOP — two budgets let a pass alternate + between them indefinitely. Delete exactly what the re-run rewrites; a resumed Gate-B + branch must carry its `reviewType` alongside its session id, since the tool defaults to + `full` and would otherwise run the other reviewer into the wrong slot. +- **What this does not do**, stated in the template rather than implied: nothing checks + the terminator mechanically — the protocol is instruction-backed by design, and a + recurrence is the trigger to build the checker. The hook counts on `PostToolUse`, so a + call that returns and then fails validation still increments the counter you are + discounting — and so does a *failed* review, because the pinned `mcp-codex-dev` returns + its own errors and timeouts as ordinary results rather than throwing, which makes the + tool call itself succeed. Discount every incomplete pass whatever the counter says. +- No hook, script, CI checker or other executable machinery changed. The behaviour of + both gates *does* change — in this repo the prompts are the product — and that change + is instruction-backed, living in prompts and templates only. + ## 0.5.0 - **Gate-B fingerprint covers the index.** `git commit` commits the index, but both hash diff --git a/plugins/dev-workflow/commands/workflow-init.md b/plugins/dev-workflow/commands/workflow-init.md index a2f02b4..e43ffdf 100644 --- a/plugins/dev-workflow/commands/workflow-init.md +++ b/plugins/dev-workflow/commands/workflow-init.md @@ -262,6 +262,91 @@ clean or clearly stuck → then STOP and surface to the user. The only early exi below 3 is a pass with **zero** findings; don't manufacture findings to pad. Codex is advisory — validate before applying; dismissed finding → one-line why. +**Findings go to a FILE, not the response — both gates.** Long finding lists come back +cut off, and a cut that lands between findings is indistinguishable from a short list: +silently dropped findings, the dangerous direction. Claude Code both limits MCP tool +output (25,000 tokens by default, `MAX_MCP_OUTPUT_TOKENS`) and persists over-threshold +results to disk behind a file reference; the protocol below is correct under either, +because the response stops carrying the findings at all. Append to the gate prompt: + +> Pass the reviewed repo root as `workingDirectory`. Write the FULL findings list to +> `.context/codex-reviews/.md` (create the directory if needed; the path is +> relative to that root — Codex resolves writes against its working directory, so +> without this a valid file can land in a different checkout). `` is +> `gate-a-spec-pass-

`, `gate-a-plan-pass-

`, or `gate-b--pass-

`. +> +> One finding per line in the format above; escape a literal pipe inside a field as +> `\|`. Every line before the terminator is exactly one finding line — no blank lines, +> headings, prose or wrapped continuations. End the file with a final line reading +> exactly `END OF FINDINGS ( total)`, `` being the number of finding lines. A +> clean pass is the single body line `NO FINDINGS` with `END OF FINDINGS (0 total)`. +> +> Then reply with ONLY one line per branch — ` | pass

| findings | ` +> — or `INCOMPLETE | | ` if you could not write the file. An unwritten +> file behind a normal-looking reply is the one outcome the reader cannot diagnose. + +**Gate B takes one file per branch** because `reviewType: full` runs the spec and +quality reviewers in parallel from one `additionalContext`. Aimed at a single path they +race, and the second writer leaves a correctly terminated, correctly counted file +holding half the findings — with every check still passing. + +**Before each call, delete every target file and confirm it is gone.** A call that dies +part-way leaves the prior attempt's valid file behind, and no terminator can tell that +from a fresh one. If a target survives deletion, stop and name the cause — path resolved +against the wrong root, permissions, a *directory* at the target, or a file reappearing +(another writer) each need a different fix, and "delete failed" alone sends you retrying +the delete. Run one pass at a time: the slot name has no invocation-unique component, so +two concurrent calls on one slot race. Passes are sequential by construction, so that is +a stated limitation, not a guarded one. + +**Accept a pass only when** the file exists and is readable; its last line is exactly +`END OF FINDINGS ( total)`; it contains exactly `` finding lines *and nothing +else* (or the single line `NO FINDINGS` when `` is 0 — "n valid lines somewhere in +the file" would accept a truncated file padded with fragments); and, for a `full` Gate-B +pass, both branch files satisfy all of that. Anything else — missing, unreadable or +empty file, wrong path, malformed terminator, count mismatch, extra lines, one branch +file, an `INCOMPLETE` reply — is an **INCOMPLETE pass**, which is not a review: don't +act on the partial list, don't count it toward the 3-pass floor, and don't read "no +Blocker/Major visible" as clean. + +**Recovery: one attempt per pass**, shared across timeout, an `INCOMPLETE` reply and +failed validation — the Mechanics timeout-retry rule widened, not a second budget beside +it, since two budgets let a pass alternate between them indefinitely. The attempt is a +fresh re-run, deleting exactly what it will rewrite: both branch files for a full +re-run, only the failed branch for a single-branch resume — deleting both and recreating +one makes the both-files check fail by construction, spending the attempt on a path that +cannot succeed. Prefer a resume only when the reply shows the review ran and just the +write failed: pass the `sessionId` from the original tool result back, and for a Gate-B +branch pass its `reviewType` alongside (`spec` with `specSessionId`, `quality` with +`qualitySessionId`) — the tool defaults to `full`, and a resume that omits it can run the +other reviewer and write the wrong slot, which no check detects, because the file is +well-formed and merely from the wrong branch. Whether a resumed session re-executes the +write or just returns its prior summary is not established; if it returns the summary, +that was the attempt. Spent and still incomplete → STOP and surface, naming which check +failed. + +**What this does not do.** The hook counts on `PostToolUse`, keyed on tool name, and +never sees the file. Claude Code fires `PostToolUse` after a *successful* call and routes +a failed one to `PostToolUseFailure`, which the plugin registers no handler for — but do +not infer from that which failures escape counting: the pinned `mcp-codex-dev` catches +its own errors, executor timeouts and aborts included, and returns them as a normal +result carrying `success: false` rather than throwing or setting `isError`. A failed +review therefore looks like a successful tool call and increments the counter. So does a +call that returns and then fails validation. The rule that follows is the simple one: +**discount every incomplete pass regardless of what the counter says** — a "satisfied" +count can overstate the passes you actually hold, and reasoning about which failure took +which event path will get it wrong. Nothing checks the terminator mechanically; this is +instruction-backed by design, and a recurring truncation incident is the trigger to build +the checker, not a reason to build it now. Detection is conditional: it catches an absent +or malformed terminator, a count mismatch and a missing branch file *in the artifact you +actually read*; it does not catch a model that writes a wrong count with a matching +number of lines, nor a stale file if you skip the delete. + +**Why `.context/codex-reviews/`:** the hook excludes `.context/` from all three of its +fingerprint components, so review artifacts cannot invalidate the review they document. +Add `/.context/codex-reviews/` to `.gitignore` — that entry specifically, not all of +`.context/`, which would strip the committed `codex-gate.on` adoption marker. + - **Gate A — Spec, then plan (TWO runs, each its own 3-pass loop).** Run on the **spec** right after brainstorming (before `writing-plans`), then on the **plan** before `executing-plans`/`subagent-driven-development` — catching a @@ -341,9 +426,14 @@ advisory — validate before applying; dismissed finding → one-line why. follow-up commit for two reasons: a `WIP: …` commit left in history defeats the naming convention it exists for, and a follow-up commit has nothing to commit when the review produced no fixes. -- **Timeout retry:** a codex call that dies at the MCP tool-call timeout is retried - once before surfacing to the user — pass state persists in `.context/`, so an - aborted call loses nothing. +- **Timeout / abort:** a codex call that dies at the MCP tool-call timeout is retried + once before surfacing to the user, and that retry *is* the single shared recovery + attempt above — not a second one. An abort is an incomplete pass, so treat it as one: + it may already have moved the hook's counter (the pinned server returns its own + timeouts as ordinary results), and it may have left a partial or stale target file, so + delete the targets and confirm them gone before retrying, then validate the result like + any other pass. Counter and workspace state persist in `.context/`; the *pass* does + not. --- diff --git a/todos.md b/todos.md index 51d2951..c9335cb 100644 --- a/todos.md +++ b/todos.md @@ -24,6 +24,47 @@ driven by recurrence rather than by enthusiasm. ### Parked (trigger-gated) +- [ ] **A failed Codex call counts as a pass — false ✓ in the firing direction.** + Derived while writing the 0.5.1 file-first protocol (PR #9), from a Gate-B finding + that corrected the opposite belief. The chain, each link checked against source + rather than inferred: the pinned `mcp-codex-dev@1.0.1` **catches** its own + exceptions — executor timeouts and aborts included — and *returns* + `{success: false, …}` as an ordinary result, without throwing and without setting + `isError` (`dist/tools/codex-review.js`, `codex-exec.js`). Claude Code therefore + classifies it as a **successful** tool call, so `PostToolUse` fires rather than + `PostToolUseFailure`. The hook's `PostToolUse` branch inspects nothing about the + result: for `$review_tool` it computes `tree_hash`, **stores that fingerprint**, + bumps the cycle counter unconditionally, and sets the fresh-streak counter to 0 + (fingerprint unavailable), 1 (fingerprint changed) or its prior value plus one + (fingerprint unchanged) — the streak is not a second cumulative counter; for + `$exec_tool` it bumps `countA` unconditionally. + Consequence: three timed-out Gate-A calls satisfy the Gate-A floor, and one + timed-out Gate-B call stores a current-content fingerprint for a review that read + nothing — the satisfied message then reports a fresh pass covering exactly the + content nobody reviewed. That is a false ✓ in the hook's recorded state, the + direction invariant 2 calls dangerous. + *What the shipped 0.5.1 prompts already do about it, stated so nobody over-scopes + the fix:* they classify a timeout or abort as an incomplete pass, require every + incomplete pass to be discounted **regardless of what the counter says**, and allow + one recovery attempt. So the residual defect is not "no mitigation exists" — an + earlier draft of this row claimed that and contradicted text shipped in the same + PR — it is that the mitigation is instruction-backed and depends on the agent + noticing and obeying the failed result, while the hook's own state is wrong either + way and stays wrong for anyone reading it later. + *Candidate fix, explicitly unverified:* skip the bump and the fingerprint store + when the result reports failure. The `PostToolUse` payload is documented to carry + `tool_response`, but **what it actually contains for an MCP tool on this server is + not established** — the hook has no `tool_response` reader at all today + (`input_field` parses only `.tool_input`), and the one place the hook reasons about + `tool_response` records that Bash's shape carries no exit status, which is why the + commit-reset deliberately ignores success. Verify the real payload for + `mcp__codex__*` before writing any matcher; a matcher built on an assumed shape + fails silently and in the same dangerous direction. Note also that failing closed + here is the *safe* direction for once — not counting a real pass costs a re-run, + while counting a dead one is the false ✓. + *Trigger: this session's discovery — already fired.* Deliberately not fixed in + PR #9, whose scope guard is prompts and templates only; this needs hook code and + regression tests. - [ ] **jq-free parser stops at an escaped JSON quote.** A payload containing `echo \"quoted\" && git commit -m x` decodes to nothing, so no reminder fires — wrong direction under invariant 2, and only on machines without `jq`. Needs