Gate findings go to a file, not the MCP response (0.5.1)#9
Conversation
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/<slot>.md, terminate it with END OF FINDINGS (<n> 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 <n> 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.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughCodex Gate A and Gate B guidance now stores complete findings in validated per-slot files, uses separate files for full Gate-B branches, and defines deletion, validation, retry, and incomplete-pass handling. Documentation, taxonomy records, backlog notes, and plugin release metadata are updated accordingly. ChangesCodex gate hardening
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR hardens the Codex review gates against silent truncation of long finding lists by moving all findings out of the MCP response and into a verifiable file with a mandatory terminator and count. Both gate prompts (in
Confidence Score: 4/5All changes are documentation and prompt text; no executable machinery was modified. The protocol logic is sound and internally consistent across CLAUDE.md and workflow-init.md. The two operational prompts (CLAUDE.md and workflow-init.md) are mutually consistent and correctly handle all documented edge cases. Two minor documentation gaps exist: the spec document uses the same placeholder variable name for both the pass number and the findings count, which could confuse a reader comparing the slot table to the reply-line format; and the CHANGELOG summary omits that failed reviews (where the server returns success:false as a normal result) also increment the pass counter, a nuance the prompts themselves cover correctly. Neither affects runtime behavior. docs/superpowers/specs/2026-07-20-codex-file-first-output.md (variable naming collision in slot table) and plugins/dev-workflow/CHANGELOG.md (incomplete PostToolUse counter description). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Start gate pass] --> B[Delete target file\nconfirm gone]
B --> C{Delete\nsucceeded?}
C -- No --> D[STOP — surface cause\npath/permissions/directory/other writer]
C -- Yes --> E[Call mcp__codex__exec\nwith workingDirectory = repo root]
E --> F{MCP reply\nform?}
F -- INCOMPLETE reply --> G[Incomplete pass]
F -- Summary line --> H[Read .context/codex-reviews/slot.md]
H --> I{All checks pass?\n1 file exists and readable\n2 terminator exact\n3 count matches\n4 Gate-B full both branches}
I -- Yes --> J[Pass accepted\nread findings]
I -- No --> G
G --> K{Recovery\nattempt spent?}
K -- No, attempt remaining --> L[Delete targets\nfresh re-run]
L --> E
K -- Yes, spent --> M[STOP — surface\nwhich check failed]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Start gate pass] --> B[Delete target file\nconfirm gone]
B --> C{Delete\nsucceeded?}
C -- No --> D[STOP — surface cause\npath/permissions/directory/other writer]
C -- Yes --> E[Call mcp__codex__exec\nwith workingDirectory = repo root]
E --> F{MCP reply\nform?}
F -- INCOMPLETE reply --> G[Incomplete pass]
F -- Summary line --> H[Read .context/codex-reviews/slot.md]
H --> I{All checks pass?\n1 file exists and readable\n2 terminator exact\n3 count matches\n4 Gate-B full both branches}
I -- Yes --> J[Pass accepted\nread findings]
I -- No --> G
G --> K{Recovery\nattempt spent?}
K -- No, attempt remaining --> L[Delete targets\nfresh re-run]
L --> E
K -- Yes, spent --> M[STOP — surface\nwhich check failed]
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
docs/superpowers/specs/2026-07-20-codex-file-first-output.md:244-273
**`<n>` used for two different things in the same document**
In the slot-name table (lines ~244–250), `<n>` stands for the pass number (e.g. `gate-a-spec-pass-<n>`). Twelve lines later, in the reply-line format, `<n>` means the *findings count* (`<gate> | pass <p> | <n> findings | <path>`). A reader cross-referencing the table and the reply line with fresh eyes will interpret both `<n>` fields as the same quantity. The operational prompts (`CLAUDE.md`, `workflow-init.md`) already use `<p>` consistently for pass number throughout; aligning the spec table to `<p>` as well removes the collision.
### Issue 2 of 2
plugins/dev-workflow/CHANGELOG.md:44-48
**CHANGELOG omits that a failed review also increments the counter**
The bullet says "`PostToolUse` fires only after a *successful* call, so a call that returns and then fails validation still increments." A reader who only reads the CHANGELOG could infer that a *failed review* (server returning `{success: false}`) does not increment — since it sounds like "not a successful call." The crucial detail — that the pinned `mcp-codex-dev@1.0.1` swallows its own errors and returns `{success: false}` as a normal result, so Claude Code sees a successful tool invocation and `PostToolUse` fires regardless — is fully stated in `CLAUDE.md` and `workflow-init.md` but is absent here. A brief parenthetical like "(the pinned server returns failures as normal results, so the tool call itself succeeds)" would close the gap for changelog readers.
Reviews (1): Last reviewed commit: "Gate findings go to a file, not the MCP ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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 |
There was a problem hiding this comment.
<n> used for two different things in the same document
In the slot-name table (lines ~244–250), <n> stands for the pass number (e.g. gate-a-spec-pass-<n>). Twelve lines later, in the reply-line format, <n> means the findings count (<gate> | pass <p> | <n> findings | <path>). A reader cross-referencing the table and the reply line with fresh eyes will interpret both <n> fields as the same quantity. The operational prompts (CLAUDE.md, workflow-init.md) already use <p> consistently for pass number throughout; aligning the spec table to <p> as well removes the collision.
Prompt To Fix With AI
This is a comment left during a code review.
Path: docs/superpowers/specs/2026-07-20-codex-file-first-output.md
Line: 244-273
Comment:
**`<n>` used for two different things in the same document**
In the slot-name table (lines ~244–250), `<n>` stands for the pass number (e.g. `gate-a-spec-pass-<n>`). Twelve lines later, in the reply-line format, `<n>` means the *findings count* (`<gate> | pass <p> | <n> findings | <path>`). A reader cross-referencing the table and the reply line with fresh eyes will interpret both `<n>` fields as the same quantity. The operational prompts (`CLAUDE.md`, `workflow-init.md`) already use `<p>` consistently for pass number throughout; aligning the spec table to `<p>` as well removes the collision.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Accepted and fixed in ef449c7.
Validated by hand rather than by subagent: this PR edits CLAUDE.md and plugins/**, so /dev-workflow:process-pr-review's instruction-path precheck skips delegated triage — a subagent would load the very instruction files under review.
It was worse than a naming collision. Twelve lines below the table the spec says <p> is "the same decimal pass number used in the slot name" — while the slot name said <n>. So the document contradicted itself, and CLAUDE.md and workflow-init.md already used <p> throughout, leaving the spec the only outlier. Table now reads <p>.
| - **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. |
There was a problem hiding this comment.
The bullet says "PostToolUse fires only after a successful call, so a call that returns and then fails validation still increments." A reader who only reads the CHANGELOG could infer that a failed review (server returning {success: false}) does not increment — since it sounds like "not a successful call." The crucial detail — that the pinned mcp-codex-dev@1.0.1 swallows its own errors and returns {success: false} as a normal result, so Claude Code sees a successful tool invocation and PostToolUse fires regardless — is fully stated in CLAUDE.md and workflow-init.md but is absent here. A brief parenthetical like "(the pinned server returns failures as normal results, so the tool call itself succeeds)" would close the gap for changelog readers.
Prompt To Fix With AI
This is a comment left during a code review.
Path: plugins/dev-workflow/CHANGELOG.md
Line: 44-48
Comment:
**CHANGELOG omits that a failed review also increments the counter**
The bullet says "`PostToolUse` fires only after a *successful* call, so a call that returns and then fails validation still increments." A reader who only reads the CHANGELOG could infer that a *failed review* (server returning `{success: false}`) does not increment — since it sounds like "not a successful call." The crucial detail — that the pinned `mcp-codex-dev@1.0.1` swallows its own errors and returns `{success: false}` as a normal result, so Claude Code sees a successful tool invocation and `PostToolUse` fires regardless — is fully stated in `CLAUDE.md` and `workflow-init.md` but is absent here. A brief parenthetical like "(the pinned server returns failures as normal results, so the tool call itself succeeds)" would close the gap for changelog readers.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Accepted and fixed in ef449c7. Validated by hand, not by subagent — this PR edits CLAUDE.md and plugins/**, so the instruction-path precheck skips delegated triage.
This one is well spotted, because the omission was a smaller version of the mistake this PR exists to correct. An earlier revision claimed outright that errors and timeouts "increment nothing", inferred from the documented PostToolUse/PostToolUseFailure split without reading the server. Gate B caught it: mcp-codex-dev@1.0.1 catches its own exceptions and returns {success: false} rather than throwing, so the tool call succeeds and the counter moves. That correction went into the prompts — and the CHANGELOG kept the phrasing that invites the same wrong inference.
Now reads: the hook counts on PostToolUse, so a call that returns and then fails validation increments — and so does a failed review, because the pinned server returns its own errors and timeouts as ordinary results. Ends on the rule that avoids the inference entirely: discount every incomplete pass whatever the counter says.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/specs/2026-07-20-codex-file-first-output.md`:
- Around line 82-92: Label both protocol example code fences in the
specification with text (or another appropriate language identifier), including
the fence containing the gate and incomplete formats, so Markdown validation no
longer reports unlabeled fences.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a7d3538-8fdf-4ee6-adda-b556acad2c6e
📒 Files selected for processing (8)
CLAUDE.mdREADME.mddocs/hardening-log.mddocs/hardening-taxonomy.mddocs/superpowers/specs/2026-07-20-codex-file-first-output.mdplugins/dev-workflow/.claude-plugin/plugin.jsonplugins/dev-workflow/CHANGELOG.mdplugins/dev-workflow/commands/workflow-init.md
From PR #9 review. Greptile P2: the spec's slot table used <n> for the pass number while the reply-line format uses <n> for the finding count — and the spec itself says <p> is "the same decimal pass number used in the slot name", so the table contradicted its own prose. Both shipped prompts already use <p>; 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.
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.
1 participant
The finding
Field report,
manual/major:The hardening — rung P
Both gate prompts move findings out of the MCP response and into a file with a verifiable terminator. Codex writes the full list to
.context/codex-reviews/<slot>.md, ends it withEND OF FINDINGS (<n> total), and replies with one line per branch. A pass is accepted only when the file is readable, the terminator exact, the body nothing but<n>finding lines, and — for a Gate-BreviewType: fullpass — both branch files valid. Anything else is an INCOMPLETE pass: not acted on, not counted, never read as clean.Gate B needs one file per reviewer branch.
reviewType: fullruns spec and quality in parallel from oneadditionalContext; 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. Caught at Gate A pass 1, as a Blocker.Recovery is one attempt per pass, shared with the existing timeout retry rather than added beside it (two budgets let a pass alternate between them indefinitely), deleting exactly what the re-run rewrites.
What it does not do
Stated in the prompts, not just here:
mcp-codex-dev@1.0.1catches its own errors, executor timeouts and aborts included, and returns{success: false}as an ordinary result rather than throwing. So the rule avoids event-routing inference entirely: discount every incomplete pass whatever the counter says.Secondary mitigation
MAX_MCP_OUTPUT_TOKENSverified against current docs (25,000 default, warning above 10,000). Documented in the README as secondary only: it moves the ceiling rather than removing it, and does not apply to tools declaringanthropic/maxResultSizeChars— whethermcp-codex-devdeclares one is not established.Which documented mechanism caused the field loss — the token limit or the separate character-based persist-to-disk threshold — was not established. The protocol is correct under either, since the response stops carrying findings at all.
Fingerprint
Minted
truncated-tool-output-read-as-completerather than filing underverification-masks-failure. That would make this a recurrence on a1 proserow, and the skill's recurrence rule then demands a mechanical rung or sharpenedAGENTS.mdwording — neither of which is P. Three Gate-A passes each rejected a different attempt to narrate that gap, which was the class being bent to fit the ladder rather than the defect.Gates
Gate A: 6 passes, clean on the sixth (12 → 10 → 7 → 5 → 2 → 0), dogfooding the protocol from pass 1.
Gate B: 3 passes, both branches clean on the third.
Two Gate-B findings changed shipped text: a claim that errors and timeouts "increment nothing" (inferred from the documented event split without reading the server — wrong, and wrong toward false confidence), and a stale
Timeout retrybullet saying an aborted call "loses nothing". The spec moved in the same commit both times, per §5.Quality battery green: shellcheck ×6, hook tests, both invariant checkers and their suites, version-bump check,
claude plugin validate --strict.Summary by CodeRabbit