Gate findings go to a file, not the MCP response (0.5.1)

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/<slot>.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). `<slot>` is
+> `gate-a-spec-pass-<p>`, `gate-a-plan-pass-<p>`, or `gate-b-<spec|quality>-pass-<p>`.
+>
+> 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 (<n> total)`, `<n>` 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 — `<gate> | pass <p> | <n> findings | <path>`
+> — or `INCOMPLETE | <cause> | <path>` 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 (<n> total)`; it contains exactly `<n>` finding lines *and nothing
+else* (or the single line `NO FINDINGS` when `<n>` 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.
 
 ---
 

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

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/<slot>.md` ending `END OF FINDINGS (<n> 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 |

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