From df032d84e1e604b98a710ea7f778bb173a9f9d33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20S=C3=A4nger?= <20968534+dsnger@users.noreply.github.com> Date: Mon, 20 Jul 2026 10:59:53 +0200 Subject: [PATCH 1/4] Gate findings go to a file, not the MCP response (0.5.1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Long finding lists came back cut off on effectively every substantial Gate A pass in the field, and a cut landing between findings is indistinguishable from a short list — so dropped findings read as a clean review. Both gate prompts now have Codex write the full list to .context/codex-reviews/.md, terminate it with END OF FINDINGS ( total), and reply with one line per branch. A pass is accepted only when the file is readable, the terminator exact, the body nothing but finding lines, and — for a Gate-B reviewType:full pass — both branch files valid. Gate B needs one file per reviewer branch: reviewType:full runs spec and quality in parallel from one additionalContext, so a shared path lets the second writer overwrite the first and still pass every check. Recovery is one attempt per pass, shared with the existing timeout retry rather than added beside it, deleting exactly what the re-run rewrites. Stated 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 an incomplete pass either way — including a failed review, since the pinned mcp-codex-dev returns its own timeouts and aborts as ordinary results — so the rule is to discount every incomplete pass regardless of what the counter says. Fingerprint truncated-tool-output-read-as-complete, 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. Gate A: 6 passes, clean on the sixth. Gate B: 3 passes, both branches clean. --- CLAUDE.md | 98 +++++- README.md | 10 + docs/hardening-log.md | 1 + docs/hardening-taxonomy.md | 23 ++ .../2026-07-20-codex-file-first-output.md | 319 ++++++++++++++++++ .../dev-workflow/.claude-plugin/plugin.json | 2 +- plugins/dev-workflow/CHANGELOG.md | 28 ++ .../dev-workflow/commands/workflow-init.md | 96 +++++- 8 files changed, 570 insertions(+), 7 deletions(-) create mode 100644 docs/superpowers/specs/2026-07-20-codex-file-first-output.md 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..2b82c52 --- /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: +> +> ``` +> | 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: +> +> ``` +> 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..4fed3da 100644 --- a/plugins/dev-workflow/CHANGELOG.md +++ b/plugins/dev-workflow/CHANGELOG.md @@ -22,6 +22,34 @@ 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`, which + fires only after a *successful* call, so a call that returns and then fails validation + still increments the counter you are discounting. +- 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. --- From 775fb38fda478137c3ed42e3e13630ca22ffbcf9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20S=C3=A4nger?= <20968534+dsnger@users.noreply.github.com> Date: Mon, 20 Jul 2026 14:52:50 +0200 Subject: [PATCH 2/4] =?UTF-8?q?docs(todos):=20park=20the=20failed-call-cou?= =?UTF-8?q?nts-as-a-pass=20false=20=E2=9C=93,=20with=20its=20derivation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- todos.md | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/todos.md b/todos.md index 51d2951..0900f20 100644 --- a/todos.md +++ b/todos.md @@ -24,6 +24,38 @@ 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**, + and bumps both counters; 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. This is a false ✓, the direction invariant 2 calls + dangerous, and unlike the counter-overstatement the 0.5.1 prompts document, **no + instruction discounts it**: the reader never sees a finding list to judge, because + the call died. + *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 From ef449c7549168fbac23508fac3e019087ec086c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20S=C3=A4nger?= <20968534+dsnger@users.noreply.github.com> Date: Mon, 20 Jul 2026 14:59:02 +0200 Subject: [PATCH 3/4] docs: fix placeholder collision and complete the CHANGELOG counter note MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From PR #9 review. Greptile P2: the spec's slot table used for the pass number while the reply-line format uses for the finding count — and the spec itself says

is "the same decimal pass number used in the slot name", so the table contradicted its own prose. Both shipped prompts already use

; the spec was the outlier. Renamed in the table. Greptile P2: the CHANGELOG bullet said PostToolUse "fires only after a successful call", which reads as though a failed review does not increment. It does — the pinned server returns its own errors as ordinary results — and that was stated in CLAUDE.md and workflow-init.md but not here. CodeRabbit MD040: labelled the two protocol fences `text`. Note the stated reason does not hold — this repo runs no markdownlint, in CI or locally — so this is applied on its own minor merit, not to satisfy a check. Gate B skipped per CLAUDE.md §5 (trivial changes only): a placeholder rename, two fence labels, and one clarifying clause. No behaviour, no protocol change. --- .../specs/2026-07-20-codex-file-first-output.md | 10 +++++----- plugins/dev-workflow/CHANGELOG.md | 8 +++++--- 2 files changed, 10 insertions(+), 8 deletions(-) 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 index 2b82c52..238c789 100644 --- a/docs/superpowers/specs/2026-07-20-codex-file-first-output.md +++ b/docs/superpowers/specs/2026-07-20-codex-file-first-output.md @@ -58,9 +58,9 @@ terminator the reader verifies. > > | Gate | `` | > |---|---| -> | Gate A, spec run | `gate-a-spec-pass-` | -> | Gate A, plan run | `gate-a-plan-pass-` | -> | Gate B | `gate-b--pass-` | +> | 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`): @@ -79,7 +79,7 @@ terminator the reader verifies. > 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 | | > ``` @@ -89,7 +89,7 @@ terminator the reader verifies. > > 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 > ``` diff --git a/plugins/dev-workflow/CHANGELOG.md b/plugins/dev-workflow/CHANGELOG.md index 4fed3da..057cf66 100644 --- a/plugins/dev-workflow/CHANGELOG.md +++ b/plugins/dev-workflow/CHANGELOG.md @@ -43,9 +43,11 @@ AGENTS.md invariant 12 carries the complete list. `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`, which - fires only after a *successful* call, so a call that returns and then fails validation - still increments the counter you are discounting. + 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. From e4ac35ef5629f8ce750d203ddc1972238dfee5bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20S=C3=A4nger?= <20968534+dsnger@users.noreply.github.com> Date: Mon, 20 Jul 2026 15:03:36 +0200 Subject: [PATCH 4/4] docs(todos): narrow the parked false-pass claim to what is actually true MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both from Gate B pass 4 on PR #9. The row said the review branch 'bumps both counters'. Only the cycle counter is unconditional; the fresh-streak counter is set to 0, 1, or prior+1 by fingerprint availability and equality. A future fixer reading the old wording would treat the streak as cumulative. The row also said no instruction discounts a timed-out call. The 0.5.1 prompts shipped in this same PR classify timeout and abort as incomplete and require discounting regardless of the counter — I wrote that bullet in this PR and then contradicted it. The residual defect is narrower: the mitigation is instruction-backed and depends on the agent obeying it, while the hook's recorded state is wrong either way. Gate-B N/A per CLAUDE.md §5: todos.md is root-level prose, no prompt path. --- todos.md | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/todos.md b/todos.md index 0900f20..c9335cb 100644 --- a/todos.md +++ b/todos.md @@ -34,14 +34,23 @@ driven by recurrence rather than by enthusiasm. 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**, - and bumps both counters; for `$exec_tool` it bumps `countA` unconditionally. + 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. This is a false ✓, the direction invariant 2 calls - dangerous, and unlike the counter-overstatement the 0.5.1 prompts document, **no - instruction discounts it**: the reader never sees a finding list to judge, because - the call died. + 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