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 new file mode 100644 index 0000000..e792e71 --- /dev/null +++ b/BUGS.md @@ -0,0 +1,23 @@ +## Medium priority (auto-fix pending) + +### Codex (2026-08-03T04:15Z) +- [ ] 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. +- [ ] 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. +- [ ] 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..b4bc960 --- /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/README.md b/README.md index e6088a3..630bf5d 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 ``` @@ -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 @@ -146,7 +156,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/claude-codex.md b/commands/claude-codex.md index bb36c85..7100fac 100644 --- a/commands/claude-codex.md +++ b/commands/claude-codex.md @@ -79,12 +79,14 @@ 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, increment iteration, re-review -- **BLOCKED** — CRITICAL issues → fix all, increment iteration, re-review +- **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 -**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 +100,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..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. If reviewer finds CRITICAL/HIGH issues: fix directly with Edit/Write and re-review (max 2 rounds) -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,11 +130,12 @@ 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/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). +- 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 without approval, stop and report status to user. +After 3 iterations still finding CRITICALs, stop and report status to user. --- @@ -151,6 +152,7 @@ After 3 iterations without approval, 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 ec9357f..be07608 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"] @@ -37,25 +37,31 @@ 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; `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. 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 -- Contains issues → address every issue, revise plan, update `.claude/plan/.md` +**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 **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 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..44778e6 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) @@ -158,11 +159,11 @@ LOW: 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 +- **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 -**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..a6563ff 100644 --- a/commands/tdd-execute-codex.md +++ b/commands/tdd-execute-codex.md @@ -99,11 +99,14 @@ 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/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 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 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 +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. If CRITICAL/HIGH issues: fix with Edit/Write, re-review (max 2 rounds) +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] @@ -229,10 +232,15 @@ 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). -After 3 iterations without approval, stop and report remaining issues to user. +**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 244eed3..5648532 100644 --- a/skills/claude-codex/SKILL.md +++ b/skills/claude-codex/SKILL.md @@ -92,8 +92,8 @@ LOW: 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 +- **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 **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/claude-codex/evals/evals.json b/skills/claude-codex/evals/evals.json index 725365b..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 WARNING or BLOCKED, Claude fixes CRITICAL/HIGH issues before re-reviewing", + "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 fffc61b..6bf3cfe 100644 --- a/skills/execute-codex/SKILL.md +++ b/skills/execute-codex/SKILL.md @@ -84,8 +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. If reviewer finds CRITICAL/HIGH issues (after evaluation): fix directly with Edit/Write and re-review (max 2 rounds) -5. Go to Phase 3 +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, carrying any remaining MEDIUM/LOW findings into the delivery report (user decides whether to fix) --- @@ -167,7 +168,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. @@ -175,11 +176,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 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). 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 without approval, stop and report status to user. +After 3 iterations still finding CRITICALs, stop and report status to user. --- @@ -196,6 +198,7 @@ After 3 iterations without approval, 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 afda551..fff601d 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 @@ -44,10 +44,12 @@ 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. +**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`. **Call `mcp__codex__codex`** (iteration 1): @@ -58,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** → 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. **Critical Evaluation of Audit Findings** @@ -79,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 diff --git a/skills/plan-codex/evals/evals.json b/skills/plan-codex/evals/evals.json index dd1b412..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", @@ -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" ] }, { diff --git a/skills/tdd-claude-codex/SKILL.md b/skills/tdd-claude-codex/SKILL.md index cefd7d3..a526017 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) @@ -191,8 +192,8 @@ LOW: 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 +- **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 **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..acfa7f8 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, 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] @@ -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 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). 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 without approval, stop and report remaining issues to user. +After 3 iterations still finding CRITICALs, stop and report remaining issues to user. ---