From 4c45a7b97bc85a3559370bc283738248ea1a8004 Mon Sep 17 00:00:00 2001 From: Surbhi <65139581+hellosurbhi@users.noreply.github.com> Date: Sun, 2 Aug 2026 21:59:44 -0400 Subject: [PATCH 01/13] docs(plan-codex): mandatory mid-call failure handling for dead audits Pre-existing uncommitted change dated 2026-07-28: a Codex call that dies mid-audit is not an approval; retry once, then stop and declare the plan UNREVIEWED out loud. Committed alone before the iteration changes so each lands separately. Verified: read the paragraph, matches the cross-review rule already recorded in global CLAUDE.md Co-Authored-By: Claude Fable 5 --- skills/plan-codex/SKILL.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/skills/plan-codex/SKILL.md b/skills/plan-codex/SKILL.md index afda551..bcc6cde 100644 --- a/skills/plan-codex/SKILL.md +++ b/skills/plan-codex/SKILL.md @@ -48,6 +48,8 @@ Save the returned plan to `.claude/plan/.md`. **MANDATORY Codex availability check**: `mcp__codex__codex` MUST be listed in the available tools (either in the tool list or in ``). Do NOT skip or bypass this phase. If the tool is genuinely absent from both locations, **stop and tell the user**: "Codex MCP is not available. This skill requires Codex for plan audit. Please add the Codex MCP server." Do not proceed without Codex — the audit loop is this skill's core value. +**MANDATORY mid-call failure handling**: a Codex call that errors, times out, or is aborted mid-audit (e.g. "sent no response or progress", MCP idle timeout, task failed) is NOT an approval and NOT a skippable step. Retry the call exactly once — via `codex exec --sandbox read-only` in Bash if the MCP transport is the thing failing (the per-session MCP connection does not pick up config changes until restart). If the retry also dies, **stop and tell the user**: "Codex audit incomplete — the review did not run. The plan is UNREVIEWED." Never proceed to Phase 3 or to implementation on a dead audit; an unreviewed plan must be labeled as such, out loud, every time. (Added 2026-07-28 after an xhigh audit was silently killed by a 1800s MCP idle timeout and work continued without it.) + Read `codex-analyzer-role.md` from this skill directory and inject as `developer-instructions`. **Call `mcp__codex__codex`** (iteration 1): From a13961734ff53a11c7721b8748a2a4a41793944a Mon Sep 17 00:00:00 2001 From: Surbhi <65139581+hellosurbhi@users.noreply.github.com> Date: Sun, 2 Aug 2026 22:00:08 -0400 Subject: [PATCH 02/13] feat(plan-codex): single audit pass, re-audit only on BLOCKED The audit loop ran up to 3 xhigh Codex rounds and only exited early on APPROVED, so a harmless WARNING verdict bought another 10-15 minute round. Measured 2026-08-02: three rounds, zero BLOCKED findings, ~45 minutes before any code. Now WARNING means incorporate the accepted findings and proceed with no further call; only BLOCKED (CRITICAL findings) triggers a re-audit, still capped at 3 iterations. The critical-evaluation and discussion-reply protocol is unchanged. Verified: read the revised Phase 2 end to end; verdict grammar matches codex-analyzer-role.md (VERDICT: APPROVED | WARNING | BLOCKED) Co-Authored-By: Claude Fable 5 --- skills/plan-codex/SKILL.md | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/skills/plan-codex/SKILL.md b/skills/plan-codex/SKILL.md index bcc6cde..5017d22 100644 --- a/skills/plan-codex/SKILL.md +++ b/skills/plan-codex/SKILL.md @@ -44,7 +44,7 @@ If requirements are ambiguous before launching the planner, ask clarifying quest Save the returned plan to `.claude/plan/.md`. -### Phase 2: Codex Audit Loop (max 3 iterations) +### Phase 2: Codex Audit (single pass; re-audit only on BLOCKED, max 3 iterations) **MANDATORY Codex availability check**: `mcp__codex__codex` MUST be listed in the available tools (either in the tool list or in ``). Do NOT skip or bypass this phase. If the tool is genuinely absent from both locations, **stop and tell the user**: "Codex MCP is not available. This skill requires Codex for plan audit. Please add the Codex MCP server." Do not proceed without Codex — the audit loop is this skill's core value. @@ -60,9 +60,10 @@ Read `codex-analyzer-role.md` from this skill directory and inject as `developer Save the returned `threadId`. -**Parse response**: -- Contains "APPROVED" → update `.claude/plan/.md` with final version, go to Phase 3 -- Contains issues → critically evaluate before addressing (see below) +**Parse the VERDICT line** (the analyzer role emits `VERDICT: APPROVED | WARNING | BLOCKED`): +- **APPROVED** → update `.claude/plan/.md` with final version, go to Phase 3. +- **WARNING** (HIGH/MEDIUM findings, nothing CRITICAL) → critically evaluate the findings (see below), fold the accepted ones into the plan, note them in the Phase 3 delivery, and go to Phase 3 with **no further Codex call**. Log: "First-pass WARNING, findings incorporated, skipping re-audit." A WARNING plan is a reviewed plan; re-auditing it buys a second 10-15 minute xhigh call to confirm formatting changes. (Measured 2026-08-02: three audit rounds, zero BLOCKED findings, ~45 minutes before any code.) +- **BLOCKED** (CRITICAL findings) → critically evaluate before addressing (see below), then revise and re-audit. **Critical Evaluation of Audit Findings** @@ -81,11 +82,12 @@ Before addressing any issues, critically evaluate each finding: **If findings are accepted:** address every accepted issue, revise plan, update `.claude/plan/.md`. -**Call `mcp__codex__codex-reply`** (iterations 2-3): +**Call `mcp__codex__codex-reply`** (iterations 2-3, **BLOCKED verdicts only**): - threadId: {saved threadId} - prompt: "The plan has been revised to address your feedback. Re-read the plan file at `.claude/plan/.md` and audit it again for correctness, completeness, security, and edge cases. Reply APPROVED if solid, or list specific issues to fix." +- A re-audit that comes back WARNING follows the WARNING rule above: incorporate and proceed, no further call. -After 3 iterations without APPROVED, stop and ask user for direction. +After 3 iterations still BLOCKED, stop and ask user for direction. ### Phase 3: Deliver From fb80cfa2fce254f12b6606b74a81caee815880b6 Mon Sep 17 00:00:00 2001 From: Surbhi <65139581+hellosurbhi@users.noreply.github.com> Date: Sun, 2 Aug 2026 22:02:20 -0400 Subject: [PATCH 03/13] feat(skills): re-review only on BLOCKED across all review loops claude-codex, execute-codex and both tdd variants re-reviewed on any non-APPROVED verdict, so a WARNING (HIGH-only) verdict bought another full xhigh Codex round. Session forensics showed a 6:1 blocked-to- approved ratio with most rounds churning on non-critical findings. Now: WARNING means fix the accepted HIGHs in one batch and proceed with no further call (the pre-push gate and CodeRabbit still review the final code); only BLOCKED/CRITICAL verdicts trigger a re-review; iteration caps are unchanged. Critical-evaluation and discussion-reply protocols untouched. Verified: grepped all four skills for stale 'without APPROVED' / 'WARNING -> re-review' phrasing, none remain Co-Authored-By: Claude Fable 5 --- skills/claude-codex/SKILL.md | 8 ++++---- skills/execute-codex/SKILL.md | 11 ++++++----- skills/tdd-claude-codex/SKILL.md | 15 ++++++++------- skills/tdd-execute-codex/SKILL.md | 18 ++++++++++-------- 4 files changed, 28 insertions(+), 24 deletions(-) diff --git a/skills/claude-codex/SKILL.md b/skills/claude-codex/SKILL.md index 244eed3..2fffaa4 100644 --- a/skills/claude-codex/SKILL.md +++ b/skills/claude-codex/SKILL.md @@ -93,7 +93,7 @@ Save the returned `threadId` for follow-up replies. Classify the verdict: - **APPROVED** — no CRITICAL or HIGH → go to Phase 4 -- **WARNING** — HIGH issues only → fix all, increment iteration, re-review +- **WARNING** — HIGH issues only → critically evaluate (R1.5), fix all accepted HIGHs in one batch, then go to Phase 4 with **no re-review call**. A WARNING diff is a reviewed diff; the pre-push gate and CodeRabbit still see the final code. (Re-reviewing WARNINGs is what turned 1 review into 3+ xhigh calls per task.) - **BLOCKED** — CRITICAL issues → fix all, increment iteration, re-review **Step R1.5 — Critical Evaluation of Review Findings** @@ -113,7 +113,7 @@ Before fixing any issues, critically evaluate each CRITICAL/HIGH finding: **If all findings appear correct:** proceed directly to Step R2. -**Step R2 — Fix and Re-review** +**Step R2 — Fix and Re-review (BLOCKED verdicts only)** Address ALL CRITICAL and HIGH issues before re-reviewing: - Collect every CRITICAL/HIGH finding from the last review @@ -127,9 +127,9 @@ Run `git diff HEAD` again to see the updated changes after fixes, then re-review Return ONLY the structured verdict in the same format. ``` -One review per iteration, not one review per fix. Stop after 3 iterations without APPROVED. +One review per iteration, not one review per fix. Only BLOCKED verdicts trigger a re-review; a re-review that comes back WARNING follows the WARNING rule above (fix accepted HIGHs, proceed, no further call). -After 3 iterations without APPROVED, stop and report remaining issues to user. +After 3 iterations still BLOCKED, stop and report remaining issues to user. **Step R3 — MEDIUM / LOW Issues** diff --git a/skills/execute-codex/SKILL.md b/skills/execute-codex/SKILL.md index fffc61b..1cc7976 100644 --- a/skills/execute-codex/SKILL.md +++ b/skills/execute-codex/SKILL.md @@ -84,7 +84,7 @@ After all tasks complete (either path): - **Check context**: Does the reviewer have full context, or is it flagging intentional design? - **Verify applicability**: Does it improve correctness/security, or is it a style preference / false positive? - **If a finding seems incorrect**: Do NOT fix it or count it as an iteration. Launch a new code-reviewer agent with the original diff, the contested findings, and your technical reasoning. Ask it to re-evaluate only the contested items. Accept confirmed findings; drop withdrawn ones. Discussion rounds do NOT count toward the iteration cap. -4. If reviewer finds CRITICAL/HIGH issues (after evaluation): fix directly with Edit/Write and re-review (max 2 rounds) +4. After evaluation: CRITICAL findings → fix directly with Edit/Write and re-review (max 2 rounds). HIGH-only findings → fix them in one batch and proceed with **no re-review call**; the pre-push gate and CodeRabbit still see the final code. 5. Go to Phase 3 --- @@ -175,11 +175,12 @@ Parse reviewer response: - **Verify applicability**: Does it improve correctness/security, or is it a style preference / false positive? - **If a finding seems incorrect**: Do NOT fix it or count it as an iteration. Launch a new code-reviewer agent with the original diff, the contested findings, and your technical reasoning. Ask it to re-evaluate only the contested items. Accept confirmed findings; drop withdrawn ones. Discussion rounds do NOT count toward the iteration cap. -After evaluation: -- Has CRITICAL/HIGH (3+ task path) → dispatch a Claude subagent per affected task with the accepted reviewer feedback verbatim; there is no shared `threadId` to reply to, so do not call `mcp__codex__codex-reply`. After fixes, re-run `code-reviewer` (max 3 total iterations). -- Has CRITICAL/HIGH (single Codex session path) → call `mcp__codex__codex-reply` with accepted reviewer feedback verbatim, increment iteration count (max 3 total iterations). +After evaluation (re-review rounds trigger on CRITICAL findings only; HIGH-only rounds fix and proceed without another call, since the pre-push gate and CodeRabbit still see the final code): +- Has CRITICAL (3+ task path) → dispatch a Claude subagent per affected task with the accepted reviewer feedback verbatim; there is no shared `threadId` to reply to, so do not call `mcp__codex__codex-reply`. After fixes, re-run `code-reviewer` (max 3 total iterations). +- Has CRITICAL (single Codex session path) → call `mcp__codex__codex-reply` with accepted reviewer feedback verbatim, increment iteration count (max 3 total iterations). +- HIGH only → fix all accepted HIGHs in one batch, then go to Phase 3. -After 3 iterations without approval, stop and report status to user. +After 3 iterations still finding CRITICALs, stop and report status to user. --- diff --git a/skills/tdd-claude-codex/SKILL.md b/skills/tdd-claude-codex/SKILL.md index cefd7d3..6fed507 100644 --- a/skills/tdd-claude-codex/SKILL.md +++ b/skills/tdd-claude-codex/SKILL.md @@ -113,7 +113,8 @@ Save the returned `threadId` (separate from the implementation review threadId). **Parse response**: - **APPROVED** → proceed to Phase 4 -- **WARNING/BLOCKED** → critically evaluate findings before fixing (see below) +- **WARNING** (no CRITICAL) → critically evaluate findings (see below), fix the accepted ones in the test files, re-verify RED, then proceed with **no further Codex call** +- **BLOCKED** → critically evaluate findings before fixing (see below), then fix and re-audit **Critical Evaluation of Test Audit Findings** @@ -130,9 +131,9 @@ Before fixing any issues, critically evaluate each CRITICAL/HIGH finding: - If the reviewer concedes or cannot justify further, drop the finding. - Discussion replies do NOT increment the iteration counter — only fix-and-re-review cycles count. -**If findings are accepted:** Claude fixes all accepted CRITICAL/HIGH issues in the test files, re-verifies RED, then re-calls via `mcp__codex__codex-reply` (max 2 iterations). +**If findings are accepted:** Claude fixes all accepted CRITICAL/HIGH issues in the test files and re-verifies RED. Only a BLOCKED verdict re-calls via `mcp__codex__codex-reply` (max 2 iterations); a WARNING proceeds after the fixes with no further call. -After 2 iterations without APPROVED, stop and ask user for direction. +After 2 iterations still BLOCKED, stop and ask user for direction. ### Phase 4: Claude Implements (GREEN) @@ -192,7 +193,7 @@ Save the returned `threadId` for follow-up replies. Classify the verdict: - **APPROVED** — no CRITICAL or HIGH → go to Phase 7 -- **WARNING** — HIGH issues only → fix all, increment iteration, re-review +- **WARNING** — HIGH issues only → critically evaluate (R1.5), fix all accepted HIGHs in one batch, then go to Phase 7 with **no re-review call**; the pre-push gate and CodeRabbit still see the final code - **BLOCKED** — CRITICAL issues → fix all, increment iteration, re-review **Step R1.5 — Critical Evaluation of Review Findings** @@ -212,7 +213,7 @@ Before fixing any issues, critically evaluate each CRITICAL/HIGH finding: **If all findings appear correct:** proceed directly to Step R2. -**Step R2 — Fix and Re-review** +**Step R2 — Fix and Re-review (BLOCKED verdicts only)** Address ALL accepted CRITICAL and HIGH issues before re-reviewing: - Collect every CRITICAL/HIGH finding from the last review @@ -226,8 +227,8 @@ Run `git diff $START_SHA` again to see the updated changes, then re-review. Return ONLY the structured verdict in the same format. ``` -One review per iteration, not one review per fix. Stop after 3 iterations without APPROVED. -After 3 iterations without APPROVED, stop and report remaining issues to user. +One review per iteration, not one review per fix. Only BLOCKED verdicts trigger a re-review; a re-review that comes back WARNING follows the WARNING rule above (fix accepted HIGHs, proceed, no further call). +After 3 iterations still BLOCKED, stop and report remaining issues to user. **Step R3 — MEDIUM / LOW Issues** diff --git a/skills/tdd-execute-codex/SKILL.md b/skills/tdd-execute-codex/SKILL.md index c27f51a..b08fe1b 100644 --- a/skills/tdd-execute-codex/SKILL.md +++ b/skills/tdd-execute-codex/SKILL.md @@ -111,7 +111,8 @@ Save the returned `threadId` (separate from the implementation review threadId). **Parse response**: - **APPROVED** → proceed to Phase 4 -- **WARNING/BLOCKED** → critically evaluate findings before fixing (see below) +- **WARNING** (no CRITICAL) → critically evaluate findings (see below), fix the accepted ones in the test files, re-verify RED, then proceed with **no further Codex call** +- **BLOCKED** → critically evaluate findings before fixing (see below), then fix and re-audit **Critical Evaluation of Test Audit Findings** @@ -128,9 +129,9 @@ Before fixing any issues, critically evaluate each CRITICAL/HIGH finding: - If the reviewer concedes or cannot justify further, drop the finding. - Discussion replies do NOT increment the iteration counter — only fix-and-re-review cycles count. -**If findings are accepted:** Claude fixes all accepted CRITICAL/HIGH issues in the test files, re-verifies RED, then re-calls via `mcp__codex__codex-reply` (max 2 iterations). +**If findings are accepted:** Claude fixes all accepted CRITICAL/HIGH issues in the test files and re-verifies RED. Only a BLOCKED verdict re-calls via `mcp__codex__codex-reply` (max 2 iterations); a WARNING proceeds after the fixes with no further call. -After 2 iterations without APPROVED, stop and ask user for direction. +After 2 iterations still BLOCKED, stop and ask user for direction. ### Phase 4: Route by Change Size @@ -175,7 +176,7 @@ After all tasks complete (either path): - **Check context**: Does the reviewer have full context, or is it flagging intentional design? - **Verify applicability**: Does it improve correctness/security, or is it a style preference / false positive? - **If a finding seems incorrect**: Do NOT fix it or count it as an iteration. Launch a new code-reviewer agent with the original diff, the contested findings, and your technical reasoning. Ask it to re-evaluate only the contested items. Accept confirmed findings; drop withdrawn ones. Discussion rounds do NOT count toward the iteration cap. -9. If accepted CRITICAL/HIGH issues remain: fix with Edit/Write, re-review (max 2 rounds) +9. Accepted CRITICAL issues → fix with Edit/Write and re-review (max 2 rounds). Accepted HIGH-only issues → fix with Edit/Write and proceed with no re-review call; the pre-push gate and CodeRabbit still see the final code. 10. After CRITICAL/HIGH resolved, collect MEDIUM/LOW issues and ask user: > "Reviewer flagged N MEDIUM/LOW issue(s) that were not fixed: > - [list issues] @@ -294,11 +295,12 @@ Parse reviewer response: - **Verify applicability**: Does it improve correctness/security, or is it a style preference / false positive? - **If a finding seems incorrect**: Do NOT fix it or count it as an iteration. Launch a new code-reviewer agent with the original diff, the contested findings, and your technical reasoning. Ask it to re-evaluate only the contested items. Accept confirmed findings; drop withdrawn ones. Discussion rounds do NOT count toward the iteration cap. -After evaluation: -- Has CRITICAL/HIGH (3+ task path) → dispatch a Claude subagent per affected task with the accepted reviewer feedback verbatim; there is no shared `threadId` to reply to, so do not call `mcp__codex__codex-reply`. After fixes, re-run `code-reviewer` (max 3 total iterations). -- Has CRITICAL/HIGH (single Codex session path) → call `mcp__codex__codex-reply` with accepted reviewer feedback verbatim, increment iteration (max 3 total iterations). +After evaluation (re-review rounds trigger on CRITICAL findings only; HIGH-only rounds fix and proceed without another call, since the pre-push gate and CodeRabbit still see the final code): +- Has CRITICAL (3+ task path) → dispatch a Claude subagent per affected task with the accepted reviewer feedback verbatim; there is no shared `threadId` to reply to, so do not call `mcp__codex__codex-reply`. After fixes, re-run `code-reviewer` (max 3 total iterations). +- Has CRITICAL (single Codex session path) → call `mcp__codex__codex-reply` with accepted reviewer feedback verbatim, increment iteration (max 3 total iterations). +- HIGH only → fix all accepted HIGHs in one batch, then proceed. -After 3 iterations without approval, stop and report remaining issues to user. +After 3 iterations still finding CRITICALs, stop and report remaining issues to user. --- From 621026175333c591d7add12a94657b1d803041e3 Mon Sep 17 00:00:00 2001 From: Surbhi <65139581+hellosurbhi@users.noreply.github.com> Date: Sun, 2 Aug 2026 22:10:03 -0400 Subject: [PATCH 04/13] fix(skills): round-1 review findings on the WARNING path The justification for skipping WARNING re-reviews cited a pre-push gate and CodeRabbit, which are operator-environment infrastructure this repo cannot assume; the justification now stands on its own (HIGHs are fixed and verified, only the paid re-verification round is skipped). WARNING paths now run post-fix verification (tests/lint, GREEN suite) and route MEDIUM/LOW handling instead of jumping straight to delivery. Route B HIGH-only branches name their path-specific fix mechanisms (subagents for the 3+ task path, codex-reply for the single-session path) so the no-Edit/Write rule cannot be violated. plan-codex WARNING description aligned to the analyzer-role grammar (at least one HIGH). Verified: grepped skills/ for 'pre-push gate and CodeRabbit', zero remain; re-read all four After-evaluation blocks for mechanism coverage Co-Authored-By: Claude Fable 5 --- skills/claude-codex/SKILL.md | 2 +- skills/execute-codex/SKILL.md | 6 +++--- skills/plan-codex/SKILL.md | 2 +- skills/tdd-claude-codex/SKILL.md | 2 +- skills/tdd-execute-codex/SKILL.md | 6 +++--- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/skills/claude-codex/SKILL.md b/skills/claude-codex/SKILL.md index 2fffaa4..d4cbeeb 100644 --- a/skills/claude-codex/SKILL.md +++ b/skills/claude-codex/SKILL.md @@ -93,7 +93,7 @@ Save the returned `threadId` for follow-up replies. Classify the verdict: - **APPROVED** — no CRITICAL or HIGH → go to Phase 4 -- **WARNING** — HIGH issues only → critically evaluate (R1.5), fix all accepted HIGHs in one batch, then go to Phase 4 with **no re-review call**. A WARNING diff is a reviewed diff; the pre-push gate and CodeRabbit still see the final code. (Re-reviewing WARNINGs is what turned 1 review into 3+ xhigh calls per task.) +- **WARNING** — HIGH issues only → critically evaluate (R1.5), fix all accepted HIGHs in one batch, run available tests/lint on the fixes, handle MEDIUM/LOW per Step R3, then go to Phase 4 with **no re-review call**. The HIGHs are fixed and verified; only the paid re-verification round is skipped. (Re-reviewing WARNINGs is what turned 1 review into 3+ xhigh calls per task.) - **BLOCKED** — CRITICAL issues → fix all, increment iteration, re-review **Step R1.5 — Critical Evaluation of Review Findings** diff --git a/skills/execute-codex/SKILL.md b/skills/execute-codex/SKILL.md index 1cc7976..899cae8 100644 --- a/skills/execute-codex/SKILL.md +++ b/skills/execute-codex/SKILL.md @@ -84,7 +84,7 @@ After all tasks complete (either path): - **Check context**: Does the reviewer have full context, or is it flagging intentional design? - **Verify applicability**: Does it improve correctness/security, or is it a style preference / false positive? - **If a finding seems incorrect**: Do NOT fix it or count it as an iteration. Launch a new code-reviewer agent with the original diff, the contested findings, and your technical reasoning. Ask it to re-evaluate only the contested items. Accept confirmed findings; drop withdrawn ones. Discussion rounds do NOT count toward the iteration cap. -4. After evaluation: CRITICAL findings → fix directly with Edit/Write and re-review (max 2 rounds). HIGH-only findings → fix them in one batch and proceed with **no re-review call**; the pre-push gate and CodeRabbit still see the final code. +4. After evaluation: CRITICAL findings → fix directly with Edit/Write and re-review (max 2 rounds). HIGH-only findings → fix them in one batch with Edit/Write, re-run the self-verification from step 1 (lint / typecheck / tests), then proceed with **no re-review call**. The HIGHs are fixed and verified; only the paid re-verification round is skipped. 5. Go to Phase 3 --- @@ -175,10 +175,10 @@ Parse reviewer response: - **Verify applicability**: Does it improve correctness/security, or is it a style preference / false positive? - **If a finding seems incorrect**: Do NOT fix it or count it as an iteration. Launch a new code-reviewer agent with the original diff, the contested findings, and your technical reasoning. Ask it to re-evaluate only the contested items. Accept confirmed findings; drop withdrawn ones. Discussion rounds do NOT count toward the iteration cap. -After evaluation (re-review rounds trigger on CRITICAL findings only; HIGH-only rounds fix and proceed without another call, since the pre-push gate and CodeRabbit still see the final code): +After evaluation (re-review rounds trigger on CRITICAL findings only; HIGH-only rounds apply fixes through the same path-specific mechanisms below and skip only the re-review verdict): - Has CRITICAL (3+ task path) → dispatch a Claude subagent per affected task with the accepted reviewer feedback verbatim; there is no shared `threadId` to reply to, so do not call `mcp__codex__codex-reply`. After fixes, re-run `code-reviewer` (max 3 total iterations). - Has CRITICAL (single Codex session path) → call `mcp__codex__codex-reply` with accepted reviewer feedback verbatim, increment iteration count (max 3 total iterations). -- HIGH only → fix all accepted HIGHs in one batch, then go to Phase 3. +- HIGH only (3+ task path) → dispatch a Claude subagent per affected task with the accepted feedback verbatim; (single Codex session path) → call `mcp__codex__codex-reply` with the accepted feedback, instructing it to apply the fixes and reply with a change summary, no re-review verdict. Route B's no-Edit/Write rule holds either way. Verify with scoped lint/tests, then go to Phase 3. After 3 iterations still finding CRITICALs, stop and report status to user. diff --git a/skills/plan-codex/SKILL.md b/skills/plan-codex/SKILL.md index 5017d22..25e2187 100644 --- a/skills/plan-codex/SKILL.md +++ b/skills/plan-codex/SKILL.md @@ -62,7 +62,7 @@ Save the returned `threadId`. **Parse the VERDICT line** (the analyzer role emits `VERDICT: APPROVED | WARNING | BLOCKED`): - **APPROVED** → update `.claude/plan/.md` with final version, go to Phase 3. -- **WARNING** (HIGH/MEDIUM findings, nothing CRITICAL) → critically evaluate the findings (see below), fold the accepted ones into the plan, note them in the Phase 3 delivery, and go to Phase 3 with **no further Codex call**. Log: "First-pass WARNING, findings incorporated, skipping re-audit." A WARNING plan is a reviewed plan; re-auditing it buys a second 10-15 minute xhigh call to confirm formatting changes. (Measured 2026-08-02: three audit rounds, zero BLOCKED findings, ~45 minutes before any code.) +- **WARNING** (at least one HIGH finding, nothing CRITICAL) → critically evaluate the findings (see below), fold the accepted ones into the plan, note them in the Phase 3 delivery, and go to Phase 3 with **no further Codex call**. Log: "First-pass WARNING, findings incorporated, skipping re-audit." A WARNING plan is a reviewed plan; re-auditing it buys a second 10-15 minute xhigh call to confirm formatting changes. (Measured 2026-08-02: three audit rounds, zero BLOCKED findings, ~45 minutes before any code.) - **BLOCKED** (CRITICAL findings) → critically evaluate before addressing (see below), then revise and re-audit. **Critical Evaluation of Audit Findings** diff --git a/skills/tdd-claude-codex/SKILL.md b/skills/tdd-claude-codex/SKILL.md index 6fed507..1847d57 100644 --- a/skills/tdd-claude-codex/SKILL.md +++ b/skills/tdd-claude-codex/SKILL.md @@ -193,7 +193,7 @@ Save the returned `threadId` for follow-up replies. Classify the verdict: - **APPROVED** — no CRITICAL or HIGH → go to Phase 7 -- **WARNING** — HIGH issues only → critically evaluate (R1.5), fix all accepted HIGHs in one batch, then go to Phase 7 with **no re-review call**; the pre-push gate and CodeRabbit still see the final code +- **WARNING** — HIGH issues only → critically evaluate (R1.5), fix all accepted HIGHs in one batch, re-run the test suite (all tests must stay GREEN), handle MEDIUM/LOW per Step R3, then go to Phase 7 with **no re-review call**. The HIGHs are fixed and verified; only the paid re-verification round is skipped - **BLOCKED** — CRITICAL issues → fix all, increment iteration, re-review **Step R1.5 — Critical Evaluation of Review Findings** diff --git a/skills/tdd-execute-codex/SKILL.md b/skills/tdd-execute-codex/SKILL.md index b08fe1b..285d049 100644 --- a/skills/tdd-execute-codex/SKILL.md +++ b/skills/tdd-execute-codex/SKILL.md @@ -176,7 +176,7 @@ After all tasks complete (either path): - **Check context**: Does the reviewer have full context, or is it flagging intentional design? - **Verify applicability**: Does it improve correctness/security, or is it a style preference / false positive? - **If a finding seems incorrect**: Do NOT fix it or count it as an iteration. Launch a new code-reviewer agent with the original diff, the contested findings, and your technical reasoning. Ask it to re-evaluate only the contested items. Accept confirmed findings; drop withdrawn ones. Discussion rounds do NOT count toward the iteration cap. -9. Accepted CRITICAL issues → fix with Edit/Write and re-review (max 2 rounds). Accepted HIGH-only issues → fix with Edit/Write and proceed with no re-review call; the pre-push gate and CodeRabbit still see the final code. +9. Accepted CRITICAL issues → fix with Edit/Write and re-review (max 2 rounds). Accepted HIGH-only issues → fix with Edit/Write, re-run the full test suite (must stay GREEN), then proceed with no re-review call. The HIGHs are fixed and verified; only the paid re-verification round is skipped. 10. After CRITICAL/HIGH resolved, collect MEDIUM/LOW issues and ask user: > "Reviewer flagged N MEDIUM/LOW issue(s) that were not fixed: > - [list issues] @@ -295,10 +295,10 @@ Parse reviewer response: - **Verify applicability**: Does it improve correctness/security, or is it a style preference / false positive? - **If a finding seems incorrect**: Do NOT fix it or count it as an iteration. Launch a new code-reviewer agent with the original diff, the contested findings, and your technical reasoning. Ask it to re-evaluate only the contested items. Accept confirmed findings; drop withdrawn ones. Discussion rounds do NOT count toward the iteration cap. -After evaluation (re-review rounds trigger on CRITICAL findings only; HIGH-only rounds fix and proceed without another call, since the pre-push gate and CodeRabbit still see the final code): +After evaluation (re-review rounds trigger on CRITICAL findings only; HIGH-only rounds apply fixes through the same path-specific mechanisms below and skip only the re-review verdict): - Has CRITICAL (3+ task path) → dispatch a Claude subagent per affected task with the accepted reviewer feedback verbatim; there is no shared `threadId` to reply to, so do not call `mcp__codex__codex-reply`. After fixes, re-run `code-reviewer` (max 3 total iterations). - Has CRITICAL (single Codex session path) → call `mcp__codex__codex-reply` with accepted reviewer feedback verbatim, increment iteration (max 3 total iterations). -- HIGH only → fix all accepted HIGHs in one batch, then proceed. +- HIGH only (3+ task path) → dispatch a Claude subagent per affected task with the accepted feedback verbatim; (single Codex session path) → call `mcp__codex__codex-reply` with the accepted feedback, instructing it to apply the fixes and reply with a change summary, no re-review verdict. Route B's no-Edit/Write rule holds either way. Verify with the test suite (must stay GREEN), then proceed. After 3 iterations still finding CRITICALs, stop and report remaining issues to user. From b9da863c716c102c407bab1e04aab6ec95169175 Mon Sep 17 00:00:00 2001 From: Surbhi <65139581+hellosurbhi@users.noreply.github.com> Date: Sun, 2 Aug 2026 22:11:40 -0400 Subject: [PATCH 05/13] fix(commands,evals): align compatibility entrypoints and evals with BLOCKED-only re-review The commands/*.md compatibility entrypoints and both eval contracts still mandated re-review on WARNING and re-audit until APPROVED, so the policy shipped in the skills did not apply across all supported review loops and the repo's own evals would enforce the removed behavior. All entrypoints now match the skills: WARNING fixes accepted HIGHs, verifies, and proceeds with no further call; only BLOCKED iterates. BUGS.md drops the four HIGH findings fixed by this commit and the previous one; the two MEDIUMs stay queued. Verified: grep across commands/ and evals for 'without APPROVED' and WARNING-re-review phrasing returns only BLOCKED-scoped lines Co-Authored-By: Claude Fable 5 --- BUGS.md | 6 ++++++ commands/claude-codex.md | 8 ++++---- commands/execute-codex.md | 5 +++-- commands/plan-codex.md | 5 +++-- commands/tdd-claude-codex.md | 13 +++++++------ commands/tdd-execute-codex.md | 10 ++++++---- skills/claude-codex/evals/evals.json | 2 +- skills/plan-codex/evals/evals.json | 6 +++--- 8 files changed, 33 insertions(+), 22 deletions(-) create mode 100644 BUGS.md diff --git a/BUGS.md b/BUGS.md new file mode 100644 index 0000000..aea81ed --- /dev/null +++ b/BUGS.md @@ -0,0 +1,6 @@ + +## Medium priority (auto-fix pending) + +### Codex (2026-08-03T02:06Z) +- [ ] MEDIUM: `skills/plan-codex/SKILL.md:51` gives `codex exec --sandbox read-only` as the mandatory fallback without supplying a prompt or analyzer instructions. The CLI reads stdin when no prompt is supplied, so this retry can hang or return output that does not follow the required verdict grammar. + - New copy throughout the diff uses em dashes despite the repository copy-voice mandate prohibiting them. diff --git a/commands/claude-codex.md b/commands/claude-codex.md index bb36c85..f5f1625 100644 --- a/commands/claude-codex.md +++ b/commands/claude-codex.md @@ -81,10 +81,10 @@ Save the returned `threadId` for follow-up replies. Classify the verdict: - **APPROVED** — no CRITICAL or HIGH → go to Phase 4 -- **WARNING** — HIGH issues only → fix all, increment iteration, re-review +- **WARNING** — HIGH issues only → fix all accepted HIGHs in one batch, run available tests/lint, handle MEDIUM/LOW per Step R3, then go to Phase 4 with no re-review call - **BLOCKED** — CRITICAL issues → fix all, increment iteration, re-review -**Step R2 — Fix and Re-review** +**Step R2 — Fix and Re-review (BLOCKED verdicts only)** Address ALL CRITICAL and HIGH issues before re-reviewing: - Collect every CRITICAL/HIGH finding from the last review @@ -98,9 +98,9 @@ Run `git diff HEAD` again to see the updated changes after fixes, then re-review Return ONLY the structured verdict in the same format. ``` -One review per iteration, not one review per fix. Stop after 3 iterations without APPROVED. +One review per iteration, not one review per fix. Only BLOCKED verdicts trigger a re-review. -After 3 iterations without APPROVED, stop and report remaining issues to user. +After 3 iterations still BLOCKED, stop and report remaining issues to user. **Step R3 — MEDIUM / LOW Issues** diff --git a/commands/execute-codex.md b/commands/execute-codex.md index 4aedd55..0c8c5ac 100644 --- a/commands/execute-codex.md +++ b/commands/execute-codex.md @@ -69,7 +69,7 @@ After all tasks complete (either path): 2. Launch Task agent (subagent_type: "feature-dev:code-reviewer") with: - `git diff HEAD` - Original task requirements -3. If reviewer finds CRITICAL/HIGH issues: fix directly with Edit/Write and re-review (max 2 rounds) +3. CRITICAL issues → fix directly with Edit/Write and re-review (max 2 rounds). HIGH-only issues → fix with Edit/Write, re-run self-verification, proceed with no re-review call. 4. Go to Phase 3 --- @@ -132,7 +132,8 @@ Launch Task agent (subagent_type: "feature-dev:code-reviewer") with: Parse reviewer response: - No CRITICAL/HIGH issues → approved, go to Phase 3 - Has CRITICAL/HIGH (3+ task path) → dispatch a Claude subagent per affected task with the reviewer feedback verbatim; there is no shared `threadId` to reply to, so do not call `mcp__codex__codex-reply`. After fixes, re-run `code-reviewer` (max 3 total iterations). -- Has CRITICAL/HIGH (single Codex session path) → call `mcp__codex__codex-reply` with reviewer feedback verbatim, increment iteration count (max 3 total iterations). +- Has CRITICAL (single Codex session path) → call `mcp__codex__codex-reply` with reviewer feedback verbatim, increment iteration count (max 3 total iterations). +- HIGH only → apply fixes via the same path-specific mechanism (subagents for the 3+ task path, `mcp__codex__codex-reply` asking for fixes with no re-review verdict for the single-session path), verify with scoped lint/tests, proceed. After 3 iterations without approval, stop and report status to user. diff --git a/commands/plan-codex.md b/commands/plan-codex.md index ec9357f..37cb79d 100644 --- a/commands/plan-codex.md +++ b/commands/plan-codex.md @@ -49,13 +49,14 @@ Save the returned `threadId`. **Parse response**: - Contains "APPROVED" → update `.claude/plan/.md` with final version, go to Phase 3 -- Contains issues → address every issue, revise plan, update `.claude/plan/.md` +- WARNING (no CRITICAL) → fold the accepted findings into the plan, update `.claude/plan/.md`, go to Phase 3 with no further Codex call +- BLOCKED (CRITICAL findings) → address every CRITICAL, revise plan, update `.claude/plan/.md`, re-audit **Call `mcp__codex__codex-reply`** (iterations 2-3): - threadId: {saved threadId} - prompt: "The plan has been revised to address your feedback. Re-read the plan file at `.claude/plan/.md` and audit it again for correctness, completeness, security, and edge cases. Reply APPROVED if solid, or list specific issues to fix." -After 3 iterations without APPROVED, stop and ask user for direction. +After 3 iterations still BLOCKED, stop and ask user for direction. ### Phase 3: Deliver diff --git a/commands/tdd-claude-codex.md b/commands/tdd-claude-codex.md index 0bd3881..20774ee 100644 --- a/commands/tdd-claude-codex.md +++ b/commands/tdd-claude-codex.md @@ -99,9 +99,10 @@ Save the returned `threadId` (separate from the implementation review threadId). **Parse response**: - **APPROVED** → proceed to Phase 4 -- **WARNING/BLOCKED** → Claude fixes all CRITICAL/HIGH issues in the test files, re-verifies RED, then re-calls via `mcp__codex__codex-reply` (max 2 iterations) +- **WARNING** (no CRITICAL) → Claude fixes the accepted issues in the test files, re-verifies RED, proceeds with no further Codex call +- **BLOCKED** → Claude fixes all CRITICAL/HIGH issues in the test files, re-verifies RED, then re-calls via `mcp__codex__codex-reply` (max 2 iterations) -After 2 iterations without APPROVED, stop and ask user for direction. +After 2 iterations still BLOCKED, stop and ask user for direction. ### Phase 4: Claude Implements (GREEN) @@ -159,10 +160,10 @@ Save the returned `threadId` for follow-up replies. Classify the verdict: - **APPROVED** — no CRITICAL or HIGH → go to Phase 7 -- **WARNING** — HIGH issues only → fix all, increment iteration, re-review +- **WARNING** — HIGH issues only → fix all accepted HIGHs in one batch, re-run the test suite (must stay GREEN), handle MEDIUM/LOW per Step R3, then go to Phase 7 with no re-review call - **BLOCKED** — CRITICAL issues → fix all, increment iteration, re-review -**Step R2 — Fix and Re-review** +**Step R2 — Fix and Re-review (BLOCKED verdicts only)** Address ALL CRITICAL and HIGH issues before re-reviewing: - Collect every CRITICAL/HIGH finding from the last review @@ -176,8 +177,8 @@ Run `git diff $START_SHA` again to see the updated changes, then re-review. Return ONLY the structured verdict in the same format. ``` -One review per iteration, not one review per fix. Stop after 3 iterations without APPROVED. -After 3 iterations without APPROVED, stop and report remaining issues to user. +One review per iteration, not one review per fix. Only BLOCKED verdicts trigger a re-review. +After 3 iterations still BLOCKED, stop and report remaining issues to user. **Step R3 — MEDIUM / LOW Issues** diff --git a/commands/tdd-execute-codex.md b/commands/tdd-execute-codex.md index 8c387ea..2cbee64 100644 --- a/commands/tdd-execute-codex.md +++ b/commands/tdd-execute-codex.md @@ -101,9 +101,10 @@ Save the returned `threadId` (separate from the implementation review threadId). **Parse response**: - **APPROVED** → proceed to Phase 4 -- **WARNING/BLOCKED** → Claude fixes all CRITICAL/HIGH issues in the test files, re-verifies RED, then re-calls via `mcp__codex__codex-reply` (max 2 iterations) +- **WARNING** (no CRITICAL) → Claude fixes the accepted issues in the test files, re-verifies RED, proceeds with no further Codex call +- **BLOCKED** → Claude fixes all CRITICAL/HIGH issues in the test files, re-verifies RED, then re-calls via `mcp__codex__codex-reply` (max 2 iterations) -After 2 iterations without APPROVED, stop and ask user for direction. +After 2 iterations still BLOCKED, stop and ask user for direction. ### Phase 4: Route by Change Size @@ -143,7 +144,7 @@ After all tasks complete (either path): 7. Launch Task agent (subagent_type: "feature-dev:code-reviewer") with: - `git diff $START_SHA` output - Original task requirements -8. If CRITICAL/HIGH issues: fix with Edit/Write, re-review (max 2 rounds) +8. CRITICAL issues → fix with Edit/Write, re-review (max 2 rounds). HIGH-only issues → fix with Edit/Write, re-run the test suite (must stay GREEN), proceed with no re-review call. 9. After CRITICAL/HIGH resolved, collect MEDIUM/LOW issues and ask user: > "Reviewer flagged N MEDIUM/LOW issue(s) that were not fixed: > - [list issues] @@ -230,7 +231,8 @@ Launch Task agent (subagent_type: "feature-dev:code-reviewer") with: Parse reviewer response: - No CRITICAL/HIGH → collect MEDIUM/LOW, ask user (same prompt as Route A step 9), then Phase 5 - Has CRITICAL/HIGH (3+ task path) → dispatch a Claude subagent per affected task with the reviewer feedback verbatim; there is no shared `threadId` to reply to, so do not call `mcp__codex__codex-reply`. After fixes, re-run `code-reviewer` (max 3 total iterations). -- Has CRITICAL/HIGH (single Codex session path) → call `mcp__codex__codex-reply` with reviewer feedback verbatim, increment iteration (max 3 total iterations). +- Has CRITICAL (single Codex session path) → call `mcp__codex__codex-reply` with reviewer feedback verbatim, increment iteration (max 3 total iterations). +- HIGH only → apply fixes via the same path-specific mechanism (subagents for the 3+ task path, `mcp__codex__codex-reply` asking for fixes with no re-review verdict for the single-session path), verify with the test suite, proceed. After 3 iterations without approval, stop and report remaining issues to user. diff --git a/skills/claude-codex/evals/evals.json b/skills/claude-codex/evals/evals.json index 725365b..979f7b2 100644 --- a/skills/claude-codex/evals/evals.json +++ b/skills/claude-codex/evals/evals.json @@ -25,7 +25,7 @@ "Claude does NOT attempt to read a plan file (uses task description directly)", "Claude calls mcp__codex__codex for review after implementation", "The Codex prompt includes 'git diff HEAD' instruction", - "If Codex returns WARNING or BLOCKED, Claude fixes CRITICAL/HIGH issues before re-reviewing", + "If Codex returns BLOCKED, Claude fixes the CRITICAL issues and re-reviews; if Codex returns WARNING, Claude fixes accepted HIGHs, runs tests/lint, and proceeds with no re-review call", "If MEDIUM/LOW issues remain after APPROVED, Claude asks user whether to fix them", "Delivery includes a changes table and review result" ] diff --git a/skills/plan-codex/evals/evals.json b/skills/plan-codex/evals/evals.json index dd1b412..e342cbb 100644 --- a/skills/plan-codex/evals/evals.json +++ b/skills/plan-codex/evals/evals.json @@ -20,15 +20,15 @@ { "id": 2, "prompt": "Plan migrating our environment variable configuration from process.env usage scattered across files to a typed config module with validation.", - "expected_output": "Claude creates plan, Codex audits with issues, Claude revises and re-submits via codex-reply, eventually APPROVED.", + "expected_output": "Claude creates plan; a BLOCKED audit is revised and re-submitted via codex-reply until no CRITICALs remain; a WARNING audit is folded into the plan with no further call.", "files": [], "expectations": [ "Claude saves the plan to .claude/plan/ before calling Codex", "Claude calls mcp__codex__codex with approval-policy 'never' (read-only audit)", - "If Codex returns issues, Claude revises the plan file before re-auditing", + "If Codex returns BLOCKED, Claude revises the plan file before re-auditing; if Codex returns WARNING, Claude folds the findings into the plan and does not re-audit", "Claude uses mcp__codex__codex-reply (not a new mcp__codex__codex call) for iterations 2-3", "Claude reuses the saved threadId for follow-up audit calls", - "After 3 iterations without APPROVED, Claude stops and asks user for direction" + "After 3 iterations still BLOCKED, Claude stops and asks user for direction" ] }, { From f3d407bd76458fd2afc56925877632a9c8e5c48f Mon Sep 17 00:00:00 2001 From: Surbhi <65139581+hellosurbhi@users.noreply.github.com> Date: Sun, 2 Aug 2026 22:30:57 -0400 Subject: [PATCH 06/13] chore(backlog): file HIGH/MEDIUM findings from the PR #1 round-2 review Verified: read the BUGS.md diff, auto-routed findings only, no code touched Co-Authored-By: Claude Fable 5 --- BUGS.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/BUGS.md b/BUGS.md index aea81ed..c6c38ad 100644 --- a/BUGS.md +++ b/BUGS.md @@ -4,3 +4,14 @@ ### Codex (2026-08-03T02:06Z) - [ ] MEDIUM: `skills/plan-codex/SKILL.md:51` gives `codex exec --sandbox read-only` as the mandatory fallback without supplying a prompt or analyzer instructions. The CLI reads stdin when no prompt is supplied, so this retry can hang or return output that does not follow the required verdict grammar. - New copy throughout the diff uses em dashes despite the repository copy-voice mandate prohibiting them. + +## High priority (from push reviews, fix first) + +### Codex (2026-08-03T02:17Z) +- [ ] HIGH: `commands/execute-codex.md:134` and `commands/tdd-execute-codex.md:233` still route CRITICAL/HIGH findings on the 3+ task path through re-review, contradicting their HIGH-only branches and the BLOCKED-only policy. Lines 138 and 237 also retain the approval-based iteration stop. +- [ ] HIGH: `skills/plan-codex/evals/evals.json:7` still expects iteration until APPROVED. `skills/plan-codex/SKILL.md:3` and `commands/plan-codex.md:2` repeat that obsolete contract even though WARNING now exits without approval. +- [ ] HIGH: `skills/tdd-execute-codex/SKILL.md:301` and `commands/tdd-execute-codex.md:235` proceed after fixing HIGHs without collecting and routing remaining MEDIUM/LOW findings as the no-HIGH branch does. + +### Codex (2026-08-03T02:17Z) +- [ ] MEDIUM: New copy still violates the repository’s em-dash prohibition, including `commands/claude-codex.md:84`, `commands/tdd-claude-codex.md:163`, `skills/claude-codex/SKILL.md:96` and `skills/tdd-claude-codex/SKILL.md:196`. + From 658dbeeb640785abb9d8192b0700206331ca697a Mon Sep 17 00:00:00 2001 From: Surbhi <65139581+hellosurbhi@users.noreply.github.com> Date: Sun, 2 Aug 2026 22:43:32 -0400 Subject: [PATCH 07/13] fix(skills): close the three HIGH findings from the backlog-push reviews The 3+ task re-review branches in commands/execute-codex.md and commands/tdd-execute-codex.md now trigger on CRITICAL only, matching the BLOCKED-only policy, and their iteration stops key on CRITICALs instead of approval. The obsolete until-APPROVED contract is removed from the plan-codex skill description, command description, eval expectation and all three README references. HIGH-only branches now carry MEDIUM/LOW findings forward the same way their approved branches do (delivery report for execute-codex, user prompt for the TDD variants). BUGS.md drops the fixed HIGH entries and duplicates; the blank-line LOW is fixed by the rewrite and its entry removed with it. Verified: grep across skills, commands and README for 'until approved' and CRITICAL/HIGH re-review phrasing returns only BLOCKED or CRITICAL scoped lines Co-Authored-By: Claude Fable 5 --- BUGS.md | 20 +++++--------------- README.md | 6 +++--- commands/execute-codex.md | 6 +++--- commands/plan-codex.md | 2 +- commands/tdd-execute-codex.md | 6 +++--- skills/execute-codex/SKILL.md | 4 ++-- skills/plan-codex/SKILL.md | 2 +- skills/plan-codex/evals/evals.json | 2 +- skills/tdd-execute-codex/SKILL.md | 2 +- 9 files changed, 20 insertions(+), 30 deletions(-) diff --git a/BUGS.md b/BUGS.md index c6c38ad..8850a27 100644 --- a/BUGS.md +++ b/BUGS.md @@ -1,17 +1,7 @@ - ## Medium priority (auto-fix pending) -### Codex (2026-08-03T02:06Z) -- [ ] MEDIUM: `skills/plan-codex/SKILL.md:51` gives `codex exec --sandbox read-only` as the mandatory fallback without supplying a prompt or analyzer instructions. The CLI reads stdin when no prompt is supplied, so this retry can hang or return output that does not follow the required verdict grammar. - - New copy throughout the diff uses em dashes despite the repository copy-voice mandate prohibiting them. - -## High priority (from push reviews, fix first) - -### Codex (2026-08-03T02:17Z) -- [ ] HIGH: `commands/execute-codex.md:134` and `commands/tdd-execute-codex.md:233` still route CRITICAL/HIGH findings on the 3+ task path through re-review, contradicting their HIGH-only branches and the BLOCKED-only policy. Lines 138 and 237 also retain the approval-based iteration stop. -- [ ] HIGH: `skills/plan-codex/evals/evals.json:7` still expects iteration until APPROVED. `skills/plan-codex/SKILL.md:3` and `commands/plan-codex.md:2` repeat that obsolete contract even though WARNING now exits without approval. -- [ ] HIGH: `skills/tdd-execute-codex/SKILL.md:301` and `commands/tdd-execute-codex.md:235` proceed after fixing HIGHs without collecting and routing remaining MEDIUM/LOW findings as the no-HIGH branch does. - -### Codex (2026-08-03T02:17Z) -- [ ] MEDIUM: New copy still violates the repository’s em-dash prohibition, including `commands/claude-codex.md:84`, `commands/tdd-claude-codex.md:163`, `skills/claude-codex/SKILL.md:96` and `skills/tdd-claude-codex/SKILL.md:196`. - +### Codex (2026-08-03T02:38Z) +- [ ] MEDIUM: `skills/plan-codex/SKILL.md:51` invokes `codex exec --sandbox read-only` without a prompt or analyzer instructions. The CLI reads stdin when the prompt is omitted, so the mandatory fallback can wait, fail or produce an invalid audit. + - `commands/claude-codex.md:84`, `commands/plan-codex.md:52` and both TDD command audit paths refer to accepted findings without defining the critical-evaluation process present in their skill counterparts. + - The execute and TDD eval suites add no WARNING-path cases, leaving the changed no-re-review behavior unenforced. + - Added copy uses prohibited em dashes and Oxford commas throughout the changed Markdown. diff --git a/README.md b/README.md index e6088a3..9f28637 100644 --- a/README.md +++ b/README.md @@ -8,7 +8,7 @@ Skills are the primary interface. They support eval-based testing via skill-crea | Skill | Trigger | Description | |-------|---------|-------------| -| `plan-codex` | `/plan-codex ` | Claude plans with Opus, Codex audits for correctness/completeness/security until approved | +| `plan-codex` | `/plan-codex ` | Claude plans with Opus, Codex audits once for correctness/completeness/security; re-audit only on BLOCKED | | `claude-codex` | `/claude-codex ` | Claude implements, Codex reviews with APPROVED/WARNING/BLOCKED verdicts | | `execute-codex` | `/execute-codex ` | Smart size-based routing: Claude (small) or Codex (large) implements, code-reviewer reviews | | `tdd-claude-codex` | `/tdd-claude-codex ` | TDD: Claude writes tests, Codex audits tests, Claude implements, Codex reviews implementation | @@ -32,7 +32,7 @@ Commands (`commands/`) are retained for backward compatibility — they support /claude-codex Claude implements (any model) → Edit/Write + self-verify Codex reviews uncommitted changes → returns structured verdict (APPROVED/WARNING/BLOCKED) - Claude fixes CRITICAL/HIGH issues → re-reviews (max 3 rounds) + Claude fixes findings → re-review only on BLOCKED (max 3 rounds) MEDIUM/LOW issues → user decides before delivery ``` @@ -146,7 +146,7 @@ Or for a direct task: Claude implements using the active model (default: Sonnet; override with `/model opus` for heavier tasks). Codex reviews the uncommitted diff via MCP, returning a structured verdict (APPROVED / WARNING / BLOCKED): - **BLOCKED** (CRITICAL issues) — Claude fixes, Codex re-reviews (max 3 rounds) -- **WARNING** (HIGH issues) — Claude fixes, Codex re-reviews (max 3 rounds) +- **WARNING** (HIGH issues) — Claude fixes them, verifies with tests/lint, and proceeds; no re-review call - **MEDIUM/LOW issues** — surfaced to user; user decides whether to fix before delivery ### Model Override diff --git a/commands/execute-codex.md b/commands/execute-codex.md index 0c8c5ac..a004a80 100644 --- a/commands/execute-codex.md +++ b/commands/execute-codex.md @@ -131,11 +131,11 @@ Launch Task agent (subagent_type: "feature-dev:code-reviewer") with: Parse reviewer response: - No CRITICAL/HIGH issues → approved, go to Phase 3 -- Has CRITICAL/HIGH (3+ task path) → dispatch a Claude subagent per affected task with the reviewer feedback verbatim; there is no shared `threadId` to reply to, so do not call `mcp__codex__codex-reply`. After fixes, re-run `code-reviewer` (max 3 total iterations). +- Has CRITICAL (3+ task path) → dispatch a Claude subagent per affected task with the reviewer feedback verbatim; there is no shared `threadId` to reply to, so do not call `mcp__codex__codex-reply`. After fixes, re-run `code-reviewer` (max 3 total iterations). - Has CRITICAL (single Codex session path) → call `mcp__codex__codex-reply` with reviewer feedback verbatim, increment iteration count (max 3 total iterations). -- HIGH only → apply fixes via the same path-specific mechanism (subagents for the 3+ task path, `mcp__codex__codex-reply` asking for fixes with no re-review verdict for the single-session path), verify with scoped lint/tests, proceed. +- HIGH only → apply fixes via the same path-specific mechanism (subagents for the 3+ task path, `mcp__codex__codex-reply` asking for fixes with no re-review verdict for the single-session path), verify with scoped lint/tests, proceed, carrying any MEDIUM/LOW findings into the delivery report. -After 3 iterations without approval, stop and report status to user. +After 3 iterations still finding CRITICALs, stop and report status to user. --- diff --git a/commands/plan-codex.md b/commands/plan-codex.md index 37cb79d..a58c3bc 100644 --- a/commands/plan-codex.md +++ b/commands/plan-codex.md @@ -1,5 +1,5 @@ --- -description: "Claude plans with Opus, Codex audits, loop until approved" +description: "Claude plans with Opus, Codex audits once, re-audit only on BLOCKED" argument-hint: "" model: claude-opus-4-6 allowed-tools: ["AskUserQuestion", "mcp__codex__codex", "mcp__codex__codex-reply", "Task", "Read", "Glob", "Grep", "Write", "Bash"] diff --git a/commands/tdd-execute-codex.md b/commands/tdd-execute-codex.md index 2cbee64..d13eaa3 100644 --- a/commands/tdd-execute-codex.md +++ b/commands/tdd-execute-codex.md @@ -230,11 +230,11 @@ Launch Task agent (subagent_type: "feature-dev:code-reviewer") with: Parse reviewer response: - No CRITICAL/HIGH → collect MEDIUM/LOW, ask user (same prompt as Route A step 9), then Phase 5 -- Has CRITICAL/HIGH (3+ task path) → dispatch a Claude subagent per affected task with the reviewer feedback verbatim; there is no shared `threadId` to reply to, so do not call `mcp__codex__codex-reply`. After fixes, re-run `code-reviewer` (max 3 total iterations). +- Has CRITICAL (3+ task path) → dispatch a Claude subagent per affected task with the reviewer feedback verbatim; there is no shared `threadId` to reply to, so do not call `mcp__codex__codex-reply`. After fixes, re-run `code-reviewer` (max 3 total iterations). - Has CRITICAL (single Codex session path) → call `mcp__codex__codex-reply` with reviewer feedback verbatim, increment iteration (max 3 total iterations). -- HIGH only → apply fixes via the same path-specific mechanism (subagents for the 3+ task path, `mcp__codex__codex-reply` asking for fixes with no re-review verdict for the single-session path), verify with the test suite, proceed. +- HIGH only → apply fixes via the same path-specific mechanism (subagents for the 3+ task path, `mcp__codex__codex-reply` asking for fixes with no re-review verdict for the single-session path), verify with the test suite, then collect MEDIUM/LOW and ask the user (same prompt as the no-CRITICAL/HIGH branch) before Phase 5. -After 3 iterations without approval, stop and report remaining issues to user. +After 3 iterations still finding CRITICALs, stop and report remaining issues to user. --- diff --git a/skills/execute-codex/SKILL.md b/skills/execute-codex/SKILL.md index 899cae8..1fbf4af 100644 --- a/skills/execute-codex/SKILL.md +++ b/skills/execute-codex/SKILL.md @@ -167,7 +167,7 @@ Launch Task agent (subagent_type: "feature-dev:code-reviewer") with: - Original task requirements Parse reviewer response: -- No CRITICAL/HIGH issues → approved, go to Phase 3 +- No CRITICAL/HIGH issues → approved, go to Phase 3, carrying any MEDIUM/LOW findings into the delivery report (user decides whether to fix) **Critical Evaluation** (before fixing): Critically evaluate each CRITICAL/HIGH finding: - **Assess correctness**: Is it technically accurate? Check actual code and project conventions. @@ -178,7 +178,7 @@ Parse reviewer response: After evaluation (re-review rounds trigger on CRITICAL findings only; HIGH-only rounds apply fixes through the same path-specific mechanisms below and skip only the re-review verdict): - Has CRITICAL (3+ task path) → dispatch a Claude subagent per affected task with the accepted reviewer feedback verbatim; there is no shared `threadId` to reply to, so do not call `mcp__codex__codex-reply`. After fixes, re-run `code-reviewer` (max 3 total iterations). - Has CRITICAL (single Codex session path) → call `mcp__codex__codex-reply` with accepted reviewer feedback verbatim, increment iteration count (max 3 total iterations). -- HIGH only (3+ task path) → dispatch a Claude subagent per affected task with the accepted feedback verbatim; (single Codex session path) → call `mcp__codex__codex-reply` with the accepted feedback, instructing it to apply the fixes and reply with a change summary, no re-review verdict. Route B's no-Edit/Write rule holds either way. Verify with scoped lint/tests, then go to Phase 3. +- HIGH only (3+ task path) → dispatch a Claude subagent per affected task with the accepted feedback verbatim; (single Codex session path) → call `mcp__codex__codex-reply` with the accepted feedback, instructing it to apply the fixes and reply with a change summary, no re-review verdict. Route B's no-Edit/Write rule holds either way. Verify with scoped lint/tests, then go to Phase 3, carrying any MEDIUM/LOW findings into the delivery report exactly as the approved branch does. After 3 iterations still finding CRITICALs, stop and report status to user. diff --git a/skills/plan-codex/SKILL.md b/skills/plan-codex/SKILL.md index 25e2187..513be46 100644 --- a/skills/plan-codex/SKILL.md +++ b/skills/plan-codex/SKILL.md @@ -1,6 +1,6 @@ --- name: plan-codex -description: "Create a detailed implementation plan with Opus-level reasoning, then Codex audits it for correctness, completeness, and security in a loop until approved. Best for: planning before coding — never modifies production code. Triggers on: /plan-codex, plan a feature, create implementation plan, design a system, architect this." +description: "Create a detailed implementation plan with Opus-level reasoning, then Codex audits it once for correctness, completeness, and security; only a BLOCKED verdict triggers re-audit. Best for: planning before coding — never modifies production code. Triggers on: /plan-codex, plan a feature, create implementation plan, design a system, architect this." --- # Plan-Codex — Claude Plans, Codex Audits diff --git a/skills/plan-codex/evals/evals.json b/skills/plan-codex/evals/evals.json index e342cbb..eac6ab5 100644 --- a/skills/plan-codex/evals/evals.json +++ b/skills/plan-codex/evals/evals.json @@ -4,7 +4,7 @@ { "id": 1, "prompt": "Plan adding JWT authentication to the API with refresh tokens, token rotation, and logout support.", - "expected_output": "Claude launches a Plan agent, saves plan to .claude/plan/, calls Codex with read-only sandbox for audit, iterates until APPROVED, presents plan and stops.", + "expected_output": "Claude launches a Plan agent, saves plan to .claude/plan/, calls Codex with read-only sandbox for a single audit (re-auditing only on BLOCKED), presents plan and stops.", "files": [], "expectations": [ "Claude launches a Task agent with subagent_type 'Plan' for plan creation", diff --git a/skills/tdd-execute-codex/SKILL.md b/skills/tdd-execute-codex/SKILL.md index 285d049..e15d55b 100644 --- a/skills/tdd-execute-codex/SKILL.md +++ b/skills/tdd-execute-codex/SKILL.md @@ -298,7 +298,7 @@ Parse reviewer response: After evaluation (re-review rounds trigger on CRITICAL findings only; HIGH-only rounds apply fixes through the same path-specific mechanisms below and skip only the re-review verdict): - Has CRITICAL (3+ task path) → dispatch a Claude subagent per affected task with the accepted reviewer feedback verbatim; there is no shared `threadId` to reply to, so do not call `mcp__codex__codex-reply`. After fixes, re-run `code-reviewer` (max 3 total iterations). - Has CRITICAL (single Codex session path) → call `mcp__codex__codex-reply` with accepted reviewer feedback verbatim, increment iteration (max 3 total iterations). -- HIGH only (3+ task path) → dispatch a Claude subagent per affected task with the accepted feedback verbatim; (single Codex session path) → call `mcp__codex__codex-reply` with the accepted feedback, instructing it to apply the fixes and reply with a change summary, no re-review verdict. Route B's no-Edit/Write rule holds either way. Verify with the test suite (must stay GREEN), then proceed. +- HIGH only (3+ task path) → dispatch a Claude subagent per affected task with the accepted feedback verbatim; (single Codex session path) → call `mcp__codex__codex-reply` with the accepted feedback, instructing it to apply the fixes and reply with a change summary, no re-review verdict. Route B's no-Edit/Write rule holds either way. Verify with the test suite (must stay GREEN), then collect remaining MEDIUM/LOW findings and ask the user, exactly as the no-CRITICAL/HIGH branch does, before proceeding. After 3 iterations still finding CRITICALs, stop and report remaining issues to user. From 68de8b70b124ffb613572e59bac5ab758d9311fa Mon Sep 17 00:00:00 2001 From: Surbhi <65139581+hellosurbhi@users.noreply.github.com> Date: Mon, 3 Aug 2026 00:05:34 -0400 Subject: [PATCH 08/13] chore(backlog): file MEDIUMs auto-routed by the final skills-push review Verified: read the BUGS.md diff, auto-routed MEDIUM entries only, no code touched Co-Authored-By: Claude Fable 5 --- BUGS.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/BUGS.md b/BUGS.md index 8850a27..02e3de7 100644 --- a/BUGS.md +++ b/BUGS.md @@ -5,3 +5,8 @@ - `commands/claude-codex.md:84`, `commands/plan-codex.md:52` and both TDD command audit paths refer to accepted findings without defining the critical-evaluation process present in their skill counterparts. - The execute and TDD eval suites add no WARNING-path cases, leaving the changed no-re-review behavior unenforced. - Added copy uses prohibited em dashes and Oxford commas throughout the changed Markdown. +### Codex (2026-08-03T03:20Z) +- [ ] MEDIUM: `commands/execute-codex.md:133` can still drop MEDIUM/LOW findings on the no-CRITICAL/HIGH path. Line 136 preserves them only for HIGH-only reviews and the Phase 3 template has no field for them. + - `README.md:35` says Claude fixes all “findings,” conflicting with line 36 where users decide on MEDIUM/LOW issues. `README.md:117` still calls the result “approved” although WARNING now exits without approval. + - `README.md:149` and `skills/plan-codex/SKILL.md:3` retain prohibited em dashes. The skill description also retains an Oxford comma. + From 181aa8be3622db60f35d21c040ecd552ef41fd21 Mon Sep 17 00:00:00 2001 From: Surbhi <65139581+hellosurbhi@users.noreply.github.com> Date: Mon, 3 Aug 2026 00:05:58 -0400 Subject: [PATCH 09/13] docs(readme): install-instruction fixes harvested from upstream PR #1 Adapted from ching-kuo/claude-codex#1 by Stephen-Kimoi, taken into this fork so the upstream PR can be left alone: codex login as the primary auth path with the API key as alternative, the corrected plugin install command (claude plugin install feature-dev@claude-plugins-official; the old claude plugin add syntax no longer exists), mkdir -p for the target dirs before the cp lines, the TOML note that hide_agent_reasoning is a top-level key that must sit above any table header, and the MCP model example updated to this setup's actual pin (gpt-5.6-sol at xhigh; the bare gpt-5.6 alias is API-only and rejected on ChatGPT-account auth). Verified: read the full rendered README install section top to bottom; every command matches this machine's working configuration Co-Authored-By: Claude Fable 5 --- README.md | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 9f28637..630bf5d 100644 --- a/README.md +++ b/README.md @@ -48,7 +48,13 @@ Install from [claude.ai/code](https://claude.ai/code). npm install -g @openai/codex ``` -Authenticate with your OpenAI API key: +Authenticate with your ChatGPT account (recommended): + +```bash +codex login +``` + +Or with your OpenAI API key: ```bash export OPENAI_API_KEY=your-key-here @@ -61,7 +67,7 @@ This setup uses the `code-reviewer` agent from [claude-plugins-official](https:/ Install the plugin in Claude Code: ``` -claude plugin add claude-plugins-official/feature-dev +claude plugin install feature-dev@claude-plugins-official ``` ## Installation @@ -72,6 +78,8 @@ claude plugin add claude-plugins-official/feature-dev git clone https://github.com//claude-codex cd claude-codex +mkdir -p ~/.claude/skills ~/.claude/prompts ~/.claude/commands + cp -r skills/* ~/.claude/skills/ cp -r prompts/* ~/.claude/prompts/ cp -r commands/* ~/.claude/commands/ # optional: for model pinning / tool restrictions @@ -85,13 +93,15 @@ Suppress reasoning tokens by adding the following to `~/.codex/config.toml`: hide_agent_reasoning = true ``` +Note: this is a top-level key. If your `config.toml` already has `[table]` sections, add it above the first one (TOML keys appended after a table header belong to that table). + ### 3. Add Codex as an MCP server ```bash -claude mcp add codex -s user -- codex -c model=gpt-5.3-codex -c model_reasoning_effort=high mcp-server +claude mcp add codex -s user -- codex -c model=gpt-5.6-sol -c model_reasoning_effort=xhigh mcp-server ``` -Adjust `model` and `model_reasoning_effort` (`low`/`medium`/`high`/`xhigh`) to your preference. +Adjust `model` and `model_reasoning_effort` (`low`/`medium`/`high`/`xhigh`) to your preference. Available models depend on your Codex plan, account type and CLI version: on ChatGPT-account auth the bare `gpt-5.6` alias is API-only and gets rejected, use `gpt-5.6-sol`. If Codex calls fail with an `invalid_request_error`, re-run with a model your account supports. ### 4. Restart Claude Code From 2460d271bd867997aeb213c996c7191a3395ed2d Mon Sep 17 00:00:00 2001 From: Surbhi <65139581+hellosurbhi@users.noreply.github.com> Date: Mon, 3 Aug 2026 00:21:56 -0400 Subject: [PATCH 10/13] fix(plan-codex-command): verdict grammar wired, dead prompt path replaced, BLOCKED covers HIGHs The command branched on WARNING and BLOCKED while its injected analyzer prompt (~/.claude/prompts/codex/analyzer.md) defined no verdicts at all and did not even exist in this install; non-approved audits could fall through unparsed. The command now injects the skill's codex-analyzer-role.md, which defines VERDICT: APPROVED | WARNING | BLOCKED, both audit prompts request the structured verdict, and the BLOCKED branch addresses accompanying HIGH findings alongside CRITICALs. ~/.claude/prompts is now symlinked to this repo's prompts/ so the architect.md references in the execute command variants resolve as well. BUGS.md drops the fixed HIGH. Verified: grep confirms codex-analyzer-role.md defines the verdict grammar while prompts/codex/analyzer.md contains zero VERDICT mentions; ls ~/.claude/prompts/codex resolves through the new symlink Co-Authored-By: Claude Fable 5 --- BUGS.md | 8 ++++++++ commands/plan-codex.md | 10 +++++----- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/BUGS.md b/BUGS.md index 02e3de7..2784e00 100644 --- a/BUGS.md +++ b/BUGS.md @@ -10,3 +10,11 @@ - `README.md:35` says Claude fixes all “findings,” conflicting with line 36 where users decide on MEDIUM/LOW issues. `README.md:117` still calls the result “approved” although WARNING now exits without approval. - `README.md:149` and `skills/plan-codex/SKILL.md:3` retain prohibited em dashes. The skill description also retains an Oxford comma. +### Codex (2026-08-03T04:15Z) +- [ ] MEDIUM: `skills/plan-codex/SKILL.md:51` invokes `codex exec` without a prompt, plan path or analyzer instructions. Local CLI help confirms an omitted prompt reads stdin, so the mandatory fallback cannot reliably perform the audit. + 2. `commands/claude-codex.md:84`, `commands/plan-codex.md:52` and both TDD command audit paths refer to “accepted” findings without defining the critical-evaluation process. + 3. `commands/execute-codex.md:133-155` can silently drop MEDIUM and LOW findings on the no-CRITICAL/HIGH path because its delivery template has no field for them. + 4. The execute and TDD eval suites contain no WARNING-path cases. `skills/claude-codex/evals/evals.json:29` also covers remaining MEDIUM/LOW findings only after APPROVED. + 5. `README.md:35-36` says Claude fixes all findings while users decide MEDIUM/LOW findings. `README.md:127` calls WARNING outcomes approved. + 6. Changed Markdown retains prohibited em dashes and Oxford commas, contrary to the repository copy mandate. + diff --git a/commands/plan-codex.md b/commands/plan-codex.md index a58c3bc..1d21508 100644 --- a/commands/plan-codex.md +++ b/commands/plan-codex.md @@ -37,24 +37,24 @@ Save the returned plan to `.claude/plan/.md`. ### Phase 2: Codex Audit Loop (max 3 iterations) -Read `~/.claude/prompts/codex/analyzer.md` and inject as `developer-instructions`. +Read `~/.claude/skills/plan-codex/codex-analyzer-role.md` and inject as `developer-instructions`. (That file defines the `VERDICT: APPROVED | WARNING | BLOCKED` grammar the parse step below branches on; the old `~/.claude/prompts/codex/analyzer.md` path does not exist in this install.) **Call `mcp__codex__codex`** (iteration 1): -- prompt: "Read the plan file at `.claude/plan/.md` and audit it for correctness, completeness, security, and edge cases. Reply APPROVED if solid, or list specific issues to fix." +- prompt: "Read the plan file at `.claude/plan/.md` and audit it for correctness, completeness, security, and edge cases. Return the structured verdict (VERDICT: APPROVED | WARNING | BLOCKED) with severity-prefixed findings per your instructions." - sandbox: "read-only" - approval-policy: "never" -- developer-instructions: {content of ~/.claude/prompts/codex/analyzer.md} + "\nBe concise. Output result only, no reasoning process." +- developer-instructions: {content of ~/.claude/skills/plan-codex/codex-analyzer-role.md} + "\nBe concise. Output result only, no reasoning process." Save the returned `threadId`. **Parse response**: - Contains "APPROVED" → update `.claude/plan/.md` with final version, go to Phase 3 - WARNING (no CRITICAL) → fold the accepted findings into the plan, update `.claude/plan/.md`, go to Phase 3 with no further Codex call -- BLOCKED (CRITICAL findings) → address every CRITICAL, revise plan, update `.claude/plan/.md`, re-audit +- BLOCKED (CRITICAL findings) → address every CRITICAL and any accompanying HIGH findings, revise plan, update `.claude/plan/.md`, re-audit **Call `mcp__codex__codex-reply`** (iterations 2-3): - threadId: {saved threadId} -- prompt: "The plan has been revised to address your feedback. Re-read the plan file at `.claude/plan/.md` and audit it again for correctness, completeness, security, and edge cases. Reply APPROVED if solid, or list specific issues to fix." +- prompt: "The plan has been revised to address your feedback. Re-read the plan file at `.claude/plan/.md` and audit it again for correctness, completeness, security, and edge cases. Return the structured verdict (VERDICT: APPROVED | WARNING | BLOCKED) with severity-prefixed findings per your instructions." After 3 iterations still BLOCKED, stop and ask user for direction. From 9f8e9f91574b0a671eac7ca4d02684d3c38f4de6 Mon Sep 17 00:00:00 2001 From: Surbhi <65139581+hellosurbhi@users.noreply.github.com> Date: Mon, 3 Aug 2026 00:33:29 -0400 Subject: [PATCH 11/13] fix(plan-codex-command): line-anchored verdict validation, APPROVED findings preserved The parse step matched the word APPROVED anywhere in the response, so a finding merely containing it could trigger delivery, and missing or multiple verdict lines fell through unparsed. Parsing is now anchored to exactly one VERDICT: line with an explicit malformed path (retry once, else declare the plan UNREVIEWED). MEDIUM/LOW findings that accompany APPROVED are folded into the plan notes per the analyzer contract instead of being discarded. The analyzer-path note no longer claims the legacy prompt file does not exist; it exists but predates the verdict grammar, which is the actual reason it must not be used. BUGS.md consolidates three overlapping MEDIUM composites into the one canonical copy and drops the entries fixed here. Verified: read the revised parse block against codex-analyzer-role.md's grammar; BUGS.md reread, each open item appears once Co-Authored-By: Claude Fable 5 --- BUGS.md | 10 ---------- commands/plan-codex.md | 10 +++++----- 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/BUGS.md b/BUGS.md index 2784e00..284ab51 100644 --- a/BUGS.md +++ b/BUGS.md @@ -1,15 +1,5 @@ ## Medium priority (auto-fix pending) -### Codex (2026-08-03T02:38Z) -- [ ] MEDIUM: `skills/plan-codex/SKILL.md:51` invokes `codex exec --sandbox read-only` without a prompt or analyzer instructions. The CLI reads stdin when the prompt is omitted, so the mandatory fallback can wait, fail or produce an invalid audit. - - `commands/claude-codex.md:84`, `commands/plan-codex.md:52` and both TDD command audit paths refer to accepted findings without defining the critical-evaluation process present in their skill counterparts. - - The execute and TDD eval suites add no WARNING-path cases, leaving the changed no-re-review behavior unenforced. - - Added copy uses prohibited em dashes and Oxford commas throughout the changed Markdown. -### Codex (2026-08-03T03:20Z) -- [ ] MEDIUM: `commands/execute-codex.md:133` can still drop MEDIUM/LOW findings on the no-CRITICAL/HIGH path. Line 136 preserves them only for HIGH-only reviews and the Phase 3 template has no field for them. - - `README.md:35` says Claude fixes all “findings,” conflicting with line 36 where users decide on MEDIUM/LOW issues. `README.md:117` still calls the result “approved” although WARNING now exits without approval. - - `README.md:149` and `skills/plan-codex/SKILL.md:3` retain prohibited em dashes. The skill description also retains an Oxford comma. - ### Codex (2026-08-03T04:15Z) - [ ] MEDIUM: `skills/plan-codex/SKILL.md:51` invokes `codex exec` without a prompt, plan path or analyzer instructions. Local CLI help confirms an omitted prompt reads stdin, so the mandatory fallback cannot reliably perform the audit. 2. `commands/claude-codex.md:84`, `commands/plan-codex.md:52` and both TDD command audit paths refer to “accepted” findings without defining the critical-evaluation process. diff --git a/commands/plan-codex.md b/commands/plan-codex.md index 1d21508..15f4e5c 100644 --- a/commands/plan-codex.md +++ b/commands/plan-codex.md @@ -37,7 +37,7 @@ Save the returned plan to `.claude/plan/.md`. ### Phase 2: Codex Audit Loop (max 3 iterations) -Read `~/.claude/skills/plan-codex/codex-analyzer-role.md` and inject as `developer-instructions`. (That file defines the `VERDICT: APPROVED | WARNING | BLOCKED` grammar the parse step below branches on; the old `~/.claude/prompts/codex/analyzer.md` path does not exist in this install.) +Read `~/.claude/skills/plan-codex/codex-analyzer-role.md` and inject as `developer-instructions`. (That file defines the `VERDICT: APPROVED | WARNING | BLOCKED` grammar the parse step below branches on; `prompts/codex/analyzer.md` also exists but predates the verdict contract and defines no grammar, so it must not be used here.) **Call `mcp__codex__codex`** (iteration 1): - prompt: "Read the plan file at `.claude/plan/.md` and audit it for correctness, completeness, security, and edge cases. Return the structured verdict (VERDICT: APPROVED | WARNING | BLOCKED) with severity-prefixed findings per your instructions." @@ -47,10 +47,10 @@ Read `~/.claude/skills/plan-codex/codex-analyzer-role.md` and inject as `develop Save the returned `threadId`. -**Parse response**: -- Contains "APPROVED" → update `.claude/plan/.md` with final version, go to Phase 3 -- WARNING (no CRITICAL) → fold the accepted findings into the plan, update `.claude/plan/.md`, go to Phase 3 with no further Codex call -- BLOCKED (CRITICAL findings) → address every CRITICAL and any accompanying HIGH findings, revise plan, update `.claude/plan/.md`, re-audit +**Parse the exact `VERDICT:` line** (line-anchored; exactly one must be present). The word APPROVED appearing inside prose or a finding is NOT approval. A response with no verdict line, multiple verdict lines, or a value other than APPROVED, WARNING or BLOCKED is malformed: retry the call once, and if the retry is also malformed, stop and tell the user the plan is UNREVIEWED. +- `VERDICT: APPROVED` → fold any accompanying MEDIUM/LOW findings into the plan's notes (the analyzer role explicitly permits them), update `.claude/plan/.md` with final version, go to Phase 3 +- `VERDICT: WARNING` (no CRITICAL) → fold the accepted findings into the plan, update `.claude/plan/.md`, go to Phase 3 with no further Codex call +- `VERDICT: BLOCKED` (CRITICAL findings) → address every CRITICAL and any accompanying HIGH findings, revise plan, update `.claude/plan/.md`, re-audit **Call `mcp__codex__codex-reply`** (iterations 2-3): - threadId: {saved threadId} From 29fad2c8217630b9bc60aa50872d07faad54a402 Mon Sep 17 00:00:00 2001 From: Surbhi <65139581+hellosurbhi@users.noreply.github.com> Date: Mon, 3 Aug 2026 01:52:55 -0400 Subject: [PATCH 12/13] fix(review-loops): fail closed at the cap, critical-evaluation gate in commands, working CLI fallback Resolves the CodeRabbit threads on the mainline-adoption PR. Every capped review route now states explicitly that surviving accepted CRITICALs mean STOP and report, never deliver: a capped-out review is not an approval. Both compatibility commands define the critical-evaluation gate (accuracy, context, applicability; contested findings go through codex-reply and do not count as iterations) that their skill counterparts already had. The plan-codex CLI retry carries the analyzer role and audit prompt inline, since a bare codex exec reads stdin and hangs. The verdict state machine thread was already resolved by the earlier line-anchored VERDICT parsing commit on this branch. reviews/ is gitignored: review-pipeline artifacts must never be tracked (an earlier attempt of this commit swept the cache in via git add -A and was reset before pushing). BUGS.md drops the two queued items these fixes close; ENHANCEMENTS.md carries the routed LOWs. Verified: git show --stat contains only intended files; grep confirms every capped route has a fail-closed stop clause Co-Authored-By: Claude Fable 5 --- .gitignore | 1 + BUGS.md | 29 +++++++++++++++++++++++------ ENHANCEMENTS.md | 11 +++++++++++ commands/claude-codex.md | 4 +++- commands/execute-codex.md | 2 +- commands/plan-codex.md | 1 + commands/tdd-execute-codex.md | 2 +- skills/execute-codex/SKILL.md | 3 ++- skills/plan-codex/SKILL.md | 2 +- skills/tdd-execute-codex/SKILL.md | 2 +- 10 files changed, 45 insertions(+), 12 deletions(-) create mode 100644 ENHANCEMENTS.md diff --git a/.gitignore b/.gitignore index 6ea56da..cc86153 100644 --- a/.gitignore +++ b/.gitignore @@ -21,3 +21,4 @@ src/ package.json package-lock.json node_modules/ +reviews/ diff --git a/BUGS.md b/BUGS.md index 284ab51..c579d16 100644 --- a/BUGS.md +++ b/BUGS.md @@ -1,10 +1,27 @@ ## Medium priority (auto-fix pending) ### Codex (2026-08-03T04:15Z) -- [ ] MEDIUM: `skills/plan-codex/SKILL.md:51` invokes `codex exec` without a prompt, plan path or analyzer instructions. Local CLI help confirms an omitted prompt reads stdin, so the mandatory fallback cannot reliably perform the audit. - 2. `commands/claude-codex.md:84`, `commands/plan-codex.md:52` and both TDD command audit paths refer to “accepted” findings without defining the critical-evaluation process. - 3. `commands/execute-codex.md:133-155` can silently drop MEDIUM and LOW findings on the no-CRITICAL/HIGH path because its delivery template has no field for them. - 4. The execute and TDD eval suites contain no WARNING-path cases. `skills/claude-codex/evals/evals.json:29` also covers remaining MEDIUM/LOW findings only after APPROVED. - 5. `README.md:35-36` says Claude fixes all findings while users decide MEDIUM/LOW findings. `README.md:127` calls WARNING outcomes approved. - 6. Changed Markdown retains prohibited em dashes and Oxford commas, contrary to the repository copy mandate. +- [ ] MEDIUM: 1. `commands/execute-codex.md:133-155` can silently drop MEDIUM and LOW findings on the no-CRITICAL/HIGH path because its delivery template has no field for them. 2. The execute and TDD eval suites contain no WARNING-path cases. `skills/claude-codex/evals/evals.json:29` also covers remaining MEDIUM/LOW findings only after APPROVED. 3. `README.md:35-36` says Claude fixes all findings while users decide MEDIUM/LOW findings. `README.md:127` calls WARNING outcomes approved. 4. Changed Markdown retains prohibited em dashes and Oxford commas, contrary to the repository copy mandate. + + +## High priority (from push reviews, fix first) + +### Codex (2026-08-03T04:51Z) +- [ ] HIGH: `commands/plan-codex.md:50-53` validates the verdict line but not its consistency with severity findings. `VERDICT: APPROVED` accompanied by `CRITICAL:` or `HIGH:` still reaches Phase 3 and silently drops those findings. Treat contradictory severity and verdict combinations as malformed. + +### Codex (2026-08-03T04:51Z) +- [ ] MEDIUM: `commands/plan-codex.md:48-55` does not specify whether a malformed retry uses the saved thread or starts a new one. A new call can return a replacement `threadId`, yet later re-audits may use the original thread. +- [ ] MEDIUM: `commands/plan-codex.md:50` adds the Oxford comma in “no verdict line, multiple verdict lines, or a value,” violating the repository copy mandate. + +### Codex (2026-08-03T05:50Z) +- [ ] HIGH: `skills/plan-codex/SKILL.md:51,61,76-87`: The CLI fallback provides no MCP `threadId`. A BLOCKED result or contested finding therefore reaches `mcp__codex__codex-reply` with no valid saved thread. Define a CLI follow-up path for the full audit loop. + - `commands/plan-codex.md:50-54`: Verdict validation ignores severity consistency. `VERDICT: APPROVED` with CRITICAL or HIGH findings still reaches Phase 3 and drops them. + - `commands/claude-codex.md:82,91-93` and `commands/plan-codex.md:51,54`: The new gate says rejected findings are not accepted, but later instructions require fixing every CRITICAL and HIGH finding. This defeats the gate on BLOCKED reviews. + - `SCOPE-CREEP`: `commands/claude-codex.md:82` expands the counterpart’s CRITICAL/HIGH evaluation gate to “ANY finding.” This can discard MEDIUM/LOW findings before the user decision in Step R3 without support in the stated intent. + +### Codex (2026-08-03T05:50Z) +- [ ] MEDIUM: `commands/plan-codex.md:48-58`: The malformed-response retry does not specify whether it reuses the thread or replaces the saved `threadId`. + - `UNASKED-CHANGE`: More than 2,000 lines of derived state under `reviews/.cache`, `reviews/.routed` and `reviews/.verdicts` were committed without a trace to the stated intent. No repository code references these artifacts. + - `ENHANCEMENTS.md:5,8`: Both new entries are stale. Current `BUGS.md:12` is a heading, while current `commands/plan-codex.md:40` already says the old analyzer exists. + - Changed Markdown introduces prohibited em dashes and Oxford commas, including `commands/claude-codex.md:82,87`, `commands/plan-codex.md:51` and `skills/plan-codex/SKILL.md:51`. diff --git a/ENHANCEMENTS.md b/ENHANCEMENTS.md new file mode 100644 index 0000000..5183cd7 --- /dev/null +++ b/ENHANCEMENTS.md @@ -0,0 +1,11 @@ + +## Low priority enhancements + +### Codex (2026-08-03T04:15Z) +- LOW: `BUGS.md:12` adds a trailing blank line flagged by `git diff --check`. + +### Codex (2026-08-03T04:31Z) +- LOW: `commands/plan-codex.md:40` falsely says the old analyzer path does not exist. `prompts/codex/analyzer.md` is tracked, `README.md:84` installs it and the reviewed local symlink resolves it. + +### Codex (2026-08-03T05:50Z) +- LOW: The range fails `git diff --check`: `ENHANCEMENTS.md:9` adds a blank line at EOF and the committed review-cache diffs contain extensive trailing whitespace. diff --git a/commands/claude-codex.md b/commands/claude-codex.md index f5f1625..8039cd7 100644 --- a/commands/claude-codex.md +++ b/commands/claude-codex.md @@ -79,10 +79,12 @@ LOW: Save the returned `threadId` for follow-up replies. +**Critical evaluation gate** (before acting on ANY finding): assess whether it is technically accurate against the actual code, whether the reviewer had full context for intentional decisions, and whether it improves correctness/security or is a preference/false positive. Contest doubtful findings via `codex-reply` with specific reasoning; discussion replies do not count as iterations. Only findings that survive this gate are "accepted" below. + Classify the verdict: - **APPROVED** — no CRITICAL or HIGH → go to Phase 4 - **WARNING** — HIGH issues only → fix all accepted HIGHs in one batch, run available tests/lint, handle MEDIUM/LOW per Step R3, then go to Phase 4 with no re-review call -- **BLOCKED** — CRITICAL issues → fix all, increment iteration, re-review +- **BLOCKED** — CRITICAL issues → fix all accepted ones, increment iteration, re-review **Step R2 — Fix and Re-review (BLOCKED verdicts only)** diff --git a/commands/execute-codex.md b/commands/execute-codex.md index a004a80..73e0fd1 100644 --- a/commands/execute-codex.md +++ b/commands/execute-codex.md @@ -69,7 +69,7 @@ After all tasks complete (either path): 2. Launch Task agent (subagent_type: "feature-dev:code-reviewer") with: - `git diff HEAD` - Original task requirements -3. CRITICAL issues → fix directly with Edit/Write and re-review (max 2 rounds). HIGH-only issues → fix with Edit/Write, re-run self-verification, proceed with no re-review call. +3. CRITICAL issues → fix directly with Edit/Write and re-review (max 2 rounds). HIGH-only issues → fix with Edit/Write, re-run self-verification, proceed with no re-review call. If CRITICAL issues remain after the final round, STOP and report them; do not deliver. 4. Go to Phase 3 --- diff --git a/commands/plan-codex.md b/commands/plan-codex.md index 15f4e5c..d5f18f7 100644 --- a/commands/plan-codex.md +++ b/commands/plan-codex.md @@ -48,6 +48,7 @@ Read `~/.claude/skills/plan-codex/codex-analyzer-role.md` and inject as `develop Save the returned `threadId`. **Parse the exact `VERDICT:` line** (line-anchored; exactly one must be present). The word APPROVED appearing inside prose or a finding is NOT approval. A response with no verdict line, multiple verdict lines, or a value other than APPROVED, WARNING or BLOCKED is malformed: retry the call once, and if the retry is also malformed, stop and tell the user the plan is UNREVIEWED. +**Critical evaluation gate** (before acting on ANY finding): assess whether it is technically accurate against the actual codebase, whether the reviewer had full context for intentional decisions, and whether it improves the plan or is a preference/false positive. Contest doubtful findings via `codex-reply` with specific reasoning; discussion replies do not count as audit iterations. Only findings that survive this gate are "accepted" below. - `VERDICT: APPROVED` → fold any accompanying MEDIUM/LOW findings into the plan's notes (the analyzer role explicitly permits them), update `.claude/plan/.md` with final version, go to Phase 3 - `VERDICT: WARNING` (no CRITICAL) → fold the accepted findings into the plan, update `.claude/plan/.md`, go to Phase 3 with no further Codex call - `VERDICT: BLOCKED` (CRITICAL findings) → address every CRITICAL and any accompanying HIGH findings, revise plan, update `.claude/plan/.md`, re-audit diff --git a/commands/tdd-execute-codex.md b/commands/tdd-execute-codex.md index d13eaa3..94cde59 100644 --- a/commands/tdd-execute-codex.md +++ b/commands/tdd-execute-codex.md @@ -144,7 +144,7 @@ After all tasks complete (either path): 7. Launch Task agent (subagent_type: "feature-dev:code-reviewer") with: - `git diff $START_SHA` output - Original task requirements -8. CRITICAL issues → fix with Edit/Write, re-review (max 2 rounds). HIGH-only issues → fix with Edit/Write, re-run the test suite (must stay GREEN), proceed with no re-review call. +8. CRITICAL issues → fix with Edit/Write, re-review (max 2 rounds). HIGH-only issues → fix with Edit/Write, re-run the test suite (must stay GREEN), proceed with no re-review call. If CRITICAL issues remain after the final round, STOP and report them; do not deliver. 9. After CRITICAL/HIGH resolved, collect MEDIUM/LOW issues and ask user: > "Reviewer flagged N MEDIUM/LOW issue(s) that were not fixed: > - [list issues] diff --git a/skills/execute-codex/SKILL.md b/skills/execute-codex/SKILL.md index 1fbf4af..5cc7c56 100644 --- a/skills/execute-codex/SKILL.md +++ b/skills/execute-codex/SKILL.md @@ -85,7 +85,8 @@ After all tasks complete (either path): - **Verify applicability**: Does it improve correctness/security, or is it a style preference / false positive? - **If a finding seems incorrect**: Do NOT fix it or count it as an iteration. Launch a new code-reviewer agent with the original diff, the contested findings, and your technical reasoning. Ask it to re-evaluate only the contested items. Accept confirmed findings; drop withdrawn ones. Discussion rounds do NOT count toward the iteration cap. 4. After evaluation: CRITICAL findings → fix directly with Edit/Write and re-review (max 2 rounds). HIGH-only findings → fix them in one batch with Edit/Write, re-run the self-verification from step 1 (lint / typecheck / tests), then proceed with **no re-review call**. The HIGHs are fixed and verified; only the paid re-verification round is skipped. -5. Go to Phase 3 +5. **Fail closed at the cap**: if accepted CRITICAL findings remain after the final round, STOP. Report the unresolved CRITICALs to the user and do not deliver; a capped-out review is not an approval. +6. Go to Phase 3 --- diff --git a/skills/plan-codex/SKILL.md b/skills/plan-codex/SKILL.md index 513be46..533ddb9 100644 --- a/skills/plan-codex/SKILL.md +++ b/skills/plan-codex/SKILL.md @@ -48,7 +48,7 @@ Save the returned plan to `.claude/plan/.md`. **MANDATORY Codex availability check**: `mcp__codex__codex` MUST be listed in the available tools (either in the tool list or in ``). Do NOT skip or bypass this phase. If the tool is genuinely absent from both locations, **stop and tell the user**: "Codex MCP is not available. This skill requires Codex for plan audit. Please add the Codex MCP server." Do not proceed without Codex — the audit loop is this skill's core value. -**MANDATORY mid-call failure handling**: a Codex call that errors, times out, or is aborted mid-audit (e.g. "sent no response or progress", MCP idle timeout, task failed) is NOT an approval and NOT a skippable step. Retry the call exactly once — via `codex exec --sandbox read-only` in Bash if the MCP transport is the thing failing (the per-session MCP connection does not pick up config changes until restart). If the retry also dies, **stop and tell the user**: "Codex audit incomplete — the review did not run. The plan is UNREVIEWED." Never proceed to Phase 3 or to implementation on a dead audit; an unreviewed plan must be labeled as such, out loud, every time. (Added 2026-07-28 after an xhigh audit was silently killed by a 1800s MCP idle timeout and work continued without it.) +**MANDATORY mid-call failure handling**: a Codex call that errors, times out, or is aborted mid-audit (e.g. "sent no response or progress", MCP idle timeout, task failed) is NOT an approval and NOT a skippable step. Retry the call exactly once — if the MCP transport is the thing failing (the per-session MCP connection does not pick up config changes until restart), retry via the CLI with the full audit context supplied explicitly, since a bare `codex exec` with no prompt hangs reading stdin: `codex exec --sandbox read-only "$(cat ~/.claude/skills/plan-codex/codex-analyzer-role.md)"$'\n\n'"Read the plan file at .claude/plan/.md and audit it for correctness, completeness, security, and edge cases. Return the structured verdict."` and validate the output against the same VERDICT grammar. If the retry also dies, **stop and tell the user**: "Codex audit incomplete — the review did not run. The plan is UNREVIEWED." Never proceed to Phase 3 or to implementation on a dead audit; an unreviewed plan must be labeled as such, out loud, every time. (Added 2026-07-28 after an xhigh audit was silently killed by a 1800s MCP idle timeout and work continued without it.) Read `codex-analyzer-role.md` from this skill directory and inject as `developer-instructions`. diff --git a/skills/tdd-execute-codex/SKILL.md b/skills/tdd-execute-codex/SKILL.md index e15d55b..837e210 100644 --- a/skills/tdd-execute-codex/SKILL.md +++ b/skills/tdd-execute-codex/SKILL.md @@ -176,7 +176,7 @@ After all tasks complete (either path): - **Check context**: Does the reviewer have full context, or is it flagging intentional design? - **Verify applicability**: Does it improve correctness/security, or is it a style preference / false positive? - **If a finding seems incorrect**: Do NOT fix it or count it as an iteration. Launch a new code-reviewer agent with the original diff, the contested findings, and your technical reasoning. Ask it to re-evaluate only the contested items. Accept confirmed findings; drop withdrawn ones. Discussion rounds do NOT count toward the iteration cap. -9. Accepted CRITICAL issues → fix with Edit/Write and re-review (max 2 rounds). Accepted HIGH-only issues → fix with Edit/Write, re-run the full test suite (must stay GREEN), then proceed with no re-review call. The HIGHs are fixed and verified; only the paid re-verification round is skipped. +9. Accepted CRITICAL issues → fix with Edit/Write and re-review (max 2 rounds). Accepted HIGH-only issues → fix with Edit/Write, re-run the full test suite (must stay GREEN), then proceed with no re-review call. The HIGHs are fixed and verified; only the paid re-verification round is skipped. If accepted CRITICALs remain after the final round, STOP and report them; do not deliver. 10. After CRITICAL/HIGH resolved, collect MEDIUM/LOW issues and ask user: > "Reviewer flagged N MEDIUM/LOW issue(s) that were not fixed: > - [list issues] From cfd3f91f0ee8a358670de61bfe9f23c851ee7abe Mon Sep 17 00:00:00 2001 From: Claude Auto-Resolve Date: Mon, 3 Aug 2026 03:15:43 -0400 Subject: [PATCH 13/13] fix: auto-resolve PR #4 review findings Addressed CodeRabbit/CodeQL findings (9 thread(s) in this pass): - Files changed: BUGS.md ENHANCEMENTS.md commands/claude-codex.md commands/execute-codex.md commands/plan-codex.md commands/tdd-claude-codex.md commands/tdd-execute-codex.md skills/claude-codex/SKILL.md skills/claude-codex/evals/evals.json skills/execute-codex/SKILL.md skills/plan-codex/SKILL.md skills/tdd-claude-codex/SKILL.md skills/tdd-execute-codex/SKILL.md Verified: Claude read each flagged file and made targeted fixes per the review comments. No unrelated changes staged. --- BUGS.md | 6 +----- ENHANCEMENTS.md | 2 +- commands/claude-codex.md | 2 +- commands/execute-codex.md | 11 ++++++----- commands/plan-codex.md | 4 ++++ commands/tdd-claude-codex.md | 2 +- commands/tdd-execute-codex.md | 16 +++++++++++----- skills/claude-codex/SKILL.md | 2 +- skills/claude-codex/evals/evals.json | 2 +- skills/execute-codex/SKILL.md | 7 ++++--- skills/plan-codex/SKILL.md | 2 +- skills/tdd-claude-codex/SKILL.md | 2 +- skills/tdd-execute-codex/SKILL.md | 2 +- 13 files changed, 34 insertions(+), 26 deletions(-) diff --git a/BUGS.md b/BUGS.md index c579d16..e792e71 100644 --- a/BUGS.md +++ b/BUGS.md @@ -1,15 +1,13 @@ ## Medium priority (auto-fix pending) ### Codex (2026-08-03T04:15Z) -- [ ] MEDIUM: 1. `commands/execute-codex.md:133-155` can silently drop MEDIUM and LOW findings on the no-CRITICAL/HIGH path because its delivery template has no field for them. 2. The execute and TDD eval suites contain no WARNING-path cases. `skills/claude-codex/evals/evals.json:29` also covers remaining MEDIUM/LOW findings only after APPROVED. 3. `README.md:35-36` says Claude fixes all findings while users decide MEDIUM/LOW findings. `README.md:127` calls WARNING outcomes approved. 4. Changed Markdown retains prohibited em dashes and Oxford commas, contrary to the repository copy mandate. +- [ ] MEDIUM: 1. The execute and TDD eval suites contain no WARNING-path cases. `skills/claude-codex/evals/evals.json:29` also covers remaining MEDIUM/LOW findings only after APPROVED. 2. `README.md:35-36` says Claude fixes all findings while users decide MEDIUM/LOW findings. `README.md:127` calls WARNING outcomes approved. 3. Changed Markdown retains prohibited em dashes and Oxford commas, contrary to the repository copy mandate. ## High priority (from push reviews, fix first) ### Codex (2026-08-03T04:51Z) - [ ] HIGH: `commands/plan-codex.md:50-53` validates the verdict line but not its consistency with severity findings. `VERDICT: APPROVED` accompanied by `CRITICAL:` or `HIGH:` still reaches Phase 3 and silently drops those findings. Treat contradictory severity and verdict combinations as malformed. - -### Codex (2026-08-03T04:51Z) - [ ] MEDIUM: `commands/plan-codex.md:48-55` does not specify whether a malformed retry uses the saved thread or starts a new one. A new call can return a replacement `threadId`, yet later re-audits may use the original thread. - [ ] MEDIUM: `commands/plan-codex.md:50` adds the Oxford comma in “no verdict line, multiple verdict lines, or a value,” violating the repository copy mandate. @@ -18,8 +16,6 @@ - `commands/plan-codex.md:50-54`: Verdict validation ignores severity consistency. `VERDICT: APPROVED` with CRITICAL or HIGH findings still reaches Phase 3 and drops them. - `commands/claude-codex.md:82,91-93` and `commands/plan-codex.md:51,54`: The new gate says rejected findings are not accepted, but later instructions require fixing every CRITICAL and HIGH finding. This defeats the gate on BLOCKED reviews. - `SCOPE-CREEP`: `commands/claude-codex.md:82` expands the counterpart’s CRITICAL/HIGH evaluation gate to “ANY finding.” This can discard MEDIUM/LOW findings before the user decision in Step R3 without support in the stated intent. - -### Codex (2026-08-03T05:50Z) - [ ] MEDIUM: `commands/plan-codex.md:48-58`: The malformed-response retry does not specify whether it reuses the thread or replaces the saved `threadId`. - `UNASKED-CHANGE`: More than 2,000 lines of derived state under `reviews/.cache`, `reviews/.routed` and `reviews/.verdicts` were committed without a trace to the stated intent. No repository code references these artifacts. - `ENHANCEMENTS.md:5,8`: Both new entries are stale. Current `BUGS.md:12` is a heading, while current `commands/plan-codex.md:40` already says the old analyzer exists. diff --git a/ENHANCEMENTS.md b/ENHANCEMENTS.md index 5183cd7..b4bc960 100644 --- a/ENHANCEMENTS.md +++ b/ENHANCEMENTS.md @@ -1,5 +1,5 @@ -## Low priority enhancements +## Low-priority enhancements ### Codex (2026-08-03T04:15Z) - LOW: `BUGS.md:12` adds a trailing blank line flagged by `git diff --check`. diff --git a/commands/claude-codex.md b/commands/claude-codex.md index 8039cd7..7100fac 100644 --- a/commands/claude-codex.md +++ b/commands/claude-codex.md @@ -82,7 +82,7 @@ Save the returned `threadId` for follow-up replies. **Critical evaluation gate** (before acting on ANY finding): assess whether it is technically accurate against the actual code, whether the reviewer had full context for intentional decisions, and whether it improves correctness/security or is a preference/false positive. Contest doubtful findings via `codex-reply` with specific reasoning; discussion replies do not count as iterations. Only findings that survive this gate are "accepted" below. Classify the verdict: -- **APPROVED** — no CRITICAL or HIGH → go to Phase 4 +- **APPROVED** — no CRITICAL or HIGH → handle any MEDIUM/LOW findings per Step R3, then go to Phase 4 - **WARNING** — HIGH issues only → fix all accepted HIGHs in one batch, run available tests/lint, handle MEDIUM/LOW per Step R3, then go to Phase 4 with no re-review call - **BLOCKED** — CRITICAL issues → fix all accepted ones, increment iteration, re-review diff --git a/commands/execute-codex.md b/commands/execute-codex.md index 73e0fd1..45e320d 100644 --- a/commands/execute-codex.md +++ b/commands/execute-codex.md @@ -69,8 +69,8 @@ After all tasks complete (either path): 2. Launch Task agent (subagent_type: "feature-dev:code-reviewer") with: - `git diff HEAD` - Original task requirements -3. CRITICAL issues → fix directly with Edit/Write and re-review (max 2 rounds). HIGH-only issues → fix with Edit/Write, re-run self-verification, proceed with no re-review call. If CRITICAL issues remain after the final round, STOP and report them; do not deliver. -4. Go to Phase 3 +3. CRITICAL issues → fix every CRITICAL and HIGH issue together with Edit/Write and re-review (max 2 rounds). HIGH-only issues → fix with Edit/Write, re-run self-verification, proceed with no re-review call. If CRITICAL issues remain after the final round, STOP and report them; do not deliver. +4. Go to Phase 3, carrying any remaining MEDIUM/LOW findings into the delivery report (user decides whether to fix) --- @@ -130,9 +130,9 @@ Launch Task agent (subagent_type: "feature-dev:code-reviewer") with: - Original task requirements Parse reviewer response: -- No CRITICAL/HIGH issues → approved, go to Phase 3 -- Has CRITICAL (3+ task path) → dispatch a Claude subagent per affected task with the reviewer feedback verbatim; there is no shared `threadId` to reply to, so do not call `mcp__codex__codex-reply`. After fixes, re-run `code-reviewer` (max 3 total iterations). -- Has CRITICAL (single Codex session path) → call `mcp__codex__codex-reply` with reviewer feedback verbatim, increment iteration count (max 3 total iterations). +- No CRITICAL/HIGH issues → approved, go to Phase 3, carrying any MEDIUM/LOW findings into the delivery report (user decides whether to fix) +- Has CRITICAL (3+ task path) → dispatch a Claude subagent per affected task with the reviewer feedback verbatim, covering every CRITICAL and HIGH finding together; there is no shared `threadId` to reply to, so do not call `mcp__codex__codex-reply`. After fixes, re-run `code-reviewer` (max 3 total iterations). +- Has CRITICAL (single Codex session path) → call `mcp__codex__codex-reply` with reviewer feedback verbatim, covering every CRITICAL and HIGH finding together, increment iteration count (max 3 total iterations). After the fixes, re-run `code-reviewer` before proceeding. - HIGH only → apply fixes via the same path-specific mechanism (subagents for the 3+ task path, `mcp__codex__codex-reply` asking for fixes with no re-review verdict for the single-session path), verify with scoped lint/tests, proceed, carrying any MEDIUM/LOW findings into the delivery report. After 3 iterations still finding CRITICALs, stop and report status to user. @@ -152,6 +152,7 @@ After 3 iterations still finding CRITICALs, stop and report status to user. ### Review Result - Route: Small (Claude) / Large (Codex, N/3 iterations) - Code review: Passed / N issues resolved +- Remaining MEDIUM/LOW findings: N (listed above, user decides) / none ### Recommended Next Steps 1. [ ] diff --git a/commands/plan-codex.md b/commands/plan-codex.md index d5f18f7..be07608 100644 --- a/commands/plan-codex.md +++ b/commands/plan-codex.md @@ -39,6 +39,10 @@ Save the returned plan to `.claude/plan/.md`. Read `~/.claude/skills/plan-codex/codex-analyzer-role.md` and inject as `developer-instructions`. (That file defines the `VERDICT: APPROVED | WARNING | BLOCKED` grammar the parse step below branches on; `prompts/codex/analyzer.md` also exists but predates the verdict contract and defines no grammar, so it must not be used here.) +**MANDATORY Codex availability check**: `mcp__codex__codex` MUST be listed in the available tools (either in the tool list or in ``). Do NOT skip or bypass this phase. If the tool is genuinely absent from both locations, **stop and tell the user**: "Codex MCP is not available. This command requires Codex for plan audit. Please add the Codex MCP server." Do not proceed without Codex. + +**MANDATORY mid-call failure handling**: a Codex call that errors, times out or is aborted mid-audit is NOT an approval and NOT a skippable step. Retry the call exactly once. If the MCP transport itself is failing, retry via the CLI with the full audit context supplied explicitly (a bare `codex exec` with no prompt hangs reading stdin): `codex exec --sandbox read-only "$(cat ~/.claude/skills/plan-codex/codex-analyzer-role.md)"$'\n\n'"Read the plan file at .claude/plan/.md and audit it for correctness, completeness, security, and edge cases. Return the structured verdict."` and validate the output against the same `VERDICT:` grammar below. If the retry also dies, **stop and tell the user**: "Codex audit incomplete. The review did not run. The plan is UNREVIEWED." Never proceed to Phase 3 on a dead audit. + **Call `mcp__codex__codex`** (iteration 1): - prompt: "Read the plan file at `.claude/plan/.md` and audit it for correctness, completeness, security, and edge cases. Return the structured verdict (VERDICT: APPROVED | WARNING | BLOCKED) with severity-prefixed findings per your instructions." - sandbox: "read-only" diff --git a/commands/tdd-claude-codex.md b/commands/tdd-claude-codex.md index 20774ee..44778e6 100644 --- a/commands/tdd-claude-codex.md +++ b/commands/tdd-claude-codex.md @@ -159,7 +159,7 @@ LOW: Save the returned `threadId` for follow-up replies. Classify the verdict: -- **APPROVED** — no CRITICAL or HIGH → go to Phase 7 +- **APPROVED** — no CRITICAL or HIGH → handle any MEDIUM/LOW findings per Step R3, then go to Phase 7 - **WARNING** — HIGH issues only → fix all accepted HIGHs in one batch, re-run the test suite (must stay GREEN), handle MEDIUM/LOW per Step R3, then go to Phase 7 with no re-review call - **BLOCKED** — CRITICAL issues → fix all, increment iteration, re-review diff --git a/commands/tdd-execute-codex.md b/commands/tdd-execute-codex.md index 94cde59..a6563ff 100644 --- a/commands/tdd-execute-codex.md +++ b/commands/tdd-execute-codex.md @@ -99,10 +99,12 @@ LOW: Save the returned `threadId` (separate from the implementation review threadId). +**Critical evaluation gate** (before acting on ANY finding): assess whether it is technically accurate against the test code and project test conventions, whether the reviewer had full context on the testing strategy and requirements and whether it improves test quality or is a preference / false positive. Contest doubtful findings via `mcp__codex__codex-reply` (reuse threadId) with specific reasoning; discussion replies do not count as iterations. Only findings that survive this gate are "accepted" below. + **Parse response**: - **APPROVED** → proceed to Phase 4 - **WARNING** (no CRITICAL) → Claude fixes the accepted issues in the test files, re-verifies RED, proceeds with no further Codex call -- **BLOCKED** → Claude fixes all CRITICAL/HIGH issues in the test files, re-verifies RED, then re-calls via `mcp__codex__codex-reply` (max 2 iterations) +- **BLOCKED** → Claude fixes all accepted CRITICAL/HIGH issues in the test files, re-verifies RED, then re-calls via `mcp__codex__codex-reply` (max 2 iterations) After 2 iterations still BLOCKED, stop and ask user for direction. @@ -144,7 +146,7 @@ After all tasks complete (either path): 7. Launch Task agent (subagent_type: "feature-dev:code-reviewer") with: - `git diff $START_SHA` output - Original task requirements -8. CRITICAL issues → fix with Edit/Write, re-review (max 2 rounds). HIGH-only issues → fix with Edit/Write, re-run the test suite (must stay GREEN), proceed with no re-review call. If CRITICAL issues remain after the final round, STOP and report them; do not deliver. +8. **Critical evaluation gate** (before acting on ANY finding): assess whether it is technically accurate against the actual code and project conventions, whether the reviewer had full context for intentional decisions and whether it improves correctness/security or is a preference / false positive. If a finding seems incorrect, launch a new code-reviewer agent with the original diff, the contested findings and your technical reasoning; accept confirmed findings, drop withdrawn ones. Discussion rounds do NOT count toward the iteration cap. Then: accepted CRITICAL issues → fix with Edit/Write, re-review (max 2 rounds). Accepted HIGH-only issues → fix with Edit/Write, re-run the test suite (must stay GREEN), proceed with no re-review call. If accepted CRITICAL issues remain after the final round, STOP and report them; do not deliver. 9. After CRITICAL/HIGH resolved, collect MEDIUM/LOW issues and ask user: > "Reviewer flagged N MEDIUM/LOW issue(s) that were not fixed: > - [list issues] @@ -230,9 +232,13 @@ Launch Task agent (subagent_type: "feature-dev:code-reviewer") with: Parse reviewer response: - No CRITICAL/HIGH → collect MEDIUM/LOW, ask user (same prompt as Route A step 9), then Phase 5 -- Has CRITICAL (3+ task path) → dispatch a Claude subagent per affected task with the reviewer feedback verbatim; there is no shared `threadId` to reply to, so do not call `mcp__codex__codex-reply`. After fixes, re-run `code-reviewer` (max 3 total iterations). -- Has CRITICAL (single Codex session path) → call `mcp__codex__codex-reply` with reviewer feedback verbatim, increment iteration (max 3 total iterations). -- HIGH only → apply fixes via the same path-specific mechanism (subagents for the 3+ task path, `mcp__codex__codex-reply` asking for fixes with no re-review verdict for the single-session path), verify with the test suite, then collect MEDIUM/LOW and ask the user (same prompt as the no-CRITICAL/HIGH branch) before Phase 5. + +**Critical evaluation gate** (before acting on ANY finding): assess whether it is technically accurate against the actual code and project conventions, whether the reviewer had full context for intentional decisions and whether it improves correctness/security or is a preference / false positive. If a finding seems incorrect, launch a new code-reviewer agent with the original diff, the contested findings and your technical reasoning; accept confirmed findings, drop withdrawn ones. Discussion rounds do NOT count toward the iteration cap. + +After evaluation: +- Has CRITICAL (3+ task path) → dispatch a Claude subagent per affected task with the accepted reviewer feedback verbatim; there is no shared `threadId` to reply to, so do not call `mcp__codex__codex-reply`. After fixes, re-run `code-reviewer` (max 3 total iterations). +- Has CRITICAL (single Codex session path) → call `mcp__codex__codex-reply` with accepted reviewer feedback verbatim, increment iteration (max 3 total iterations). After the fixes, re-run `code-reviewer` before proceeding. +- HIGH only → apply accepted fixes via the same path-specific mechanism (subagents for the 3+ task path, `mcp__codex__codex-reply` asking for fixes with no re-review verdict for the single-session path), verify with the test suite, then collect MEDIUM/LOW and ask the user (same prompt as the no-CRITICAL/HIGH branch) before Phase 5. After 3 iterations still finding CRITICALs, stop and report remaining issues to user. diff --git a/skills/claude-codex/SKILL.md b/skills/claude-codex/SKILL.md index d4cbeeb..5648532 100644 --- a/skills/claude-codex/SKILL.md +++ b/skills/claude-codex/SKILL.md @@ -92,7 +92,7 @@ LOW: Save the returned `threadId` for follow-up replies. Classify the verdict: -- **APPROVED** — no CRITICAL or HIGH → go to Phase 4 +- **APPROVED** — no CRITICAL or HIGH → handle any MEDIUM/LOW findings per Step R3, then go to Phase 4 - **WARNING** — HIGH issues only → critically evaluate (R1.5), fix all accepted HIGHs in one batch, run available tests/lint on the fixes, handle MEDIUM/LOW per Step R3, then go to Phase 4 with **no re-review call**. The HIGHs are fixed and verified; only the paid re-verification round is skipped. (Re-reviewing WARNINGs is what turned 1 review into 3+ xhigh calls per task.) - **BLOCKED** — CRITICAL issues → fix all, increment iteration, re-review diff --git a/skills/claude-codex/evals/evals.json b/skills/claude-codex/evals/evals.json index 979f7b2..4381b39 100644 --- a/skills/claude-codex/evals/evals.json +++ b/skills/claude-codex/evals/evals.json @@ -25,7 +25,7 @@ "Claude does NOT attempt to read a plan file (uses task description directly)", "Claude calls mcp__codex__codex for review after implementation", "The Codex prompt includes 'git diff HEAD' instruction", - "If Codex returns BLOCKED, Claude fixes the CRITICAL issues and re-reviews; if Codex returns WARNING, Claude fixes accepted HIGHs, runs tests/lint, and proceeds with no re-review call", + "If Codex returns BLOCKED, Claude fixes all accepted CRITICAL and HIGH issues together before re-reviewing; if Codex returns WARNING, Claude fixes accepted HIGHs, runs tests/lint, and proceeds with no re-review call", "If MEDIUM/LOW issues remain after APPROVED, Claude asks user whether to fix them", "Delivery includes a changes table and review result" ] diff --git a/skills/execute-codex/SKILL.md b/skills/execute-codex/SKILL.md index 5cc7c56..6bf3cfe 100644 --- a/skills/execute-codex/SKILL.md +++ b/skills/execute-codex/SKILL.md @@ -84,9 +84,9 @@ After all tasks complete (either path): - **Check context**: Does the reviewer have full context, or is it flagging intentional design? - **Verify applicability**: Does it improve correctness/security, or is it a style preference / false positive? - **If a finding seems incorrect**: Do NOT fix it or count it as an iteration. Launch a new code-reviewer agent with the original diff, the contested findings, and your technical reasoning. Ask it to re-evaluate only the contested items. Accept confirmed findings; drop withdrawn ones. Discussion rounds do NOT count toward the iteration cap. -4. After evaluation: CRITICAL findings → fix directly with Edit/Write and re-review (max 2 rounds). HIGH-only findings → fix them in one batch with Edit/Write, re-run the self-verification from step 1 (lint / typecheck / tests), then proceed with **no re-review call**. The HIGHs are fixed and verified; only the paid re-verification round is skipped. +4. After evaluation: CRITICAL findings → fix them together with every accepted HIGH finding using Edit/Write and re-review (max 2 rounds). HIGH-only findings → fix them in one batch with Edit/Write, re-run the self-verification from step 1 (lint / typecheck / tests), then proceed with **no re-review call**. The HIGHs are fixed and verified; only the paid re-verification round is skipped. 5. **Fail closed at the cap**: if accepted CRITICAL findings remain after the final round, STOP. Report the unresolved CRITICALs to the user and do not deliver; a capped-out review is not an approval. -6. Go to Phase 3 +6. Go to Phase 3, carrying any remaining MEDIUM/LOW findings into the delivery report (user decides whether to fix) --- @@ -178,7 +178,7 @@ Parse reviewer response: After evaluation (re-review rounds trigger on CRITICAL findings only; HIGH-only rounds apply fixes through the same path-specific mechanisms below and skip only the re-review verdict): - Has CRITICAL (3+ task path) → dispatch a Claude subagent per affected task with the accepted reviewer feedback verbatim; there is no shared `threadId` to reply to, so do not call `mcp__codex__codex-reply`. After fixes, re-run `code-reviewer` (max 3 total iterations). -- Has CRITICAL (single Codex session path) → call `mcp__codex__codex-reply` with accepted reviewer feedback verbatim, increment iteration count (max 3 total iterations). +- Has CRITICAL (single Codex session path) → call `mcp__codex__codex-reply` with accepted reviewer feedback verbatim, increment iteration count (max 3 total iterations). After the fixes, re-run `code-reviewer` before proceeding. - HIGH only (3+ task path) → dispatch a Claude subagent per affected task with the accepted feedback verbatim; (single Codex session path) → call `mcp__codex__codex-reply` with the accepted feedback, instructing it to apply the fixes and reply with a change summary, no re-review verdict. Route B's no-Edit/Write rule holds either way. Verify with scoped lint/tests, then go to Phase 3, carrying any MEDIUM/LOW findings into the delivery report exactly as the approved branch does. After 3 iterations still finding CRITICALs, stop and report status to user. @@ -198,6 +198,7 @@ After 3 iterations still finding CRITICALs, stop and report status to user. ### Review Result - Route: Small (Claude) / Large (Codex, N/3 iterations) - Code review: Passed / N issues resolved +- Remaining MEDIUM/LOW findings: N (listed above, user decides) / none ### Recommended Next Steps 1. [ ] diff --git a/skills/plan-codex/SKILL.md b/skills/plan-codex/SKILL.md index 533ddb9..fff601d 100644 --- a/skills/plan-codex/SKILL.md +++ b/skills/plan-codex/SKILL.md @@ -61,7 +61,7 @@ Read `codex-analyzer-role.md` from this skill directory and inject as `developer Save the returned `threadId`. **Parse the VERDICT line** (the analyzer role emits `VERDICT: APPROVED | WARNING | BLOCKED`): -- **APPROVED** → update `.claude/plan/.md` with final version, go to Phase 3. +- **APPROVED** → fold any accompanying MEDIUM/LOW findings into the plan's notes, update `.claude/plan/.md` with final version, go to Phase 3. - **WARNING** (at least one HIGH finding, nothing CRITICAL) → critically evaluate the findings (see below), fold the accepted ones into the plan, note them in the Phase 3 delivery, and go to Phase 3 with **no further Codex call**. Log: "First-pass WARNING, findings incorporated, skipping re-audit." A WARNING plan is a reviewed plan; re-auditing it buys a second 10-15 minute xhigh call to confirm formatting changes. (Measured 2026-08-02: three audit rounds, zero BLOCKED findings, ~45 minutes before any code.) - **BLOCKED** (CRITICAL findings) → critically evaluate before addressing (see below), then revise and re-audit. diff --git a/skills/tdd-claude-codex/SKILL.md b/skills/tdd-claude-codex/SKILL.md index 1847d57..a526017 100644 --- a/skills/tdd-claude-codex/SKILL.md +++ b/skills/tdd-claude-codex/SKILL.md @@ -192,7 +192,7 @@ LOW: Save the returned `threadId` for follow-up replies. Classify the verdict: -- **APPROVED** — no CRITICAL or HIGH → go to Phase 7 +- **APPROVED** — no CRITICAL or HIGH → handle any MEDIUM/LOW findings per Step R3, then go to Phase 7 - **WARNING** — HIGH issues only → critically evaluate (R1.5), fix all accepted HIGHs in one batch, re-run the test suite (all tests must stay GREEN), handle MEDIUM/LOW per Step R3, then go to Phase 7 with **no re-review call**. The HIGHs are fixed and verified; only the paid re-verification round is skipped - **BLOCKED** — CRITICAL issues → fix all, increment iteration, re-review diff --git a/skills/tdd-execute-codex/SKILL.md b/skills/tdd-execute-codex/SKILL.md index 837e210..acfa7f8 100644 --- a/skills/tdd-execute-codex/SKILL.md +++ b/skills/tdd-execute-codex/SKILL.md @@ -297,7 +297,7 @@ Parse reviewer response: After evaluation (re-review rounds trigger on CRITICAL findings only; HIGH-only rounds apply fixes through the same path-specific mechanisms below and skip only the re-review verdict): - Has CRITICAL (3+ task path) → dispatch a Claude subagent per affected task with the accepted reviewer feedback verbatim; there is no shared `threadId` to reply to, so do not call `mcp__codex__codex-reply`. After fixes, re-run `code-reviewer` (max 3 total iterations). -- Has CRITICAL (single Codex session path) → call `mcp__codex__codex-reply` with accepted reviewer feedback verbatim, increment iteration (max 3 total iterations). +- Has CRITICAL (single Codex session path) → call `mcp__codex__codex-reply` with accepted reviewer feedback verbatim, increment iteration (max 3 total iterations). After the fixes, re-run `code-reviewer` before proceeding. - HIGH only (3+ task path) → dispatch a Claude subagent per affected task with the accepted feedback verbatim; (single Codex session path) → call `mcp__codex__codex-reply` with the accepted feedback, instructing it to apply the fixes and reply with a change summary, no re-review verdict. Route B's no-Edit/Write rule holds either way. Verify with the test suite (must stay GREEN), then collect remaining MEDIUM/LOW findings and ask the user, exactly as the no-CRITICAL/HIGH branch does, before proceeding. After 3 iterations still finding CRITICALs, stop and report remaining issues to user.