diff --git a/.gitignore b/.gitignore index c9ae450..6d887d1 100644 --- a/.gitignore +++ b/.gitignore @@ -15,6 +15,9 @@ node_modules/ .DS_Store .gitnexus +# Git worktrees +.worktrees/ + # Working documents (project-level, not shipped) SESSION_SUMMARY.md PROGRESS_REPORT.md diff --git a/PR_DESCRIPTION.md b/PR_DESCRIPTION.md new file mode 100644 index 0000000..705d89c --- /dev/null +++ b/PR_DESCRIPTION.md @@ -0,0 +1,102 @@ +# Complete Approach A Skill Improvements + +## Summary + +Implements all 12 tasks from [Approach A implementation plan](docs/superpowers/plans/2026-03-22-approach-a-skill-improvements.md): +- Fixed 5 critical bugs across multiple review skills +- Added information barrier pattern to 3 review skills +- Unified VERDICT vocabulary across all review skills + +**Total changes**: 13 files, +287/-168 lines, 18 commits + +## Changes by Category + +### 1. Bug Fixes (5 bugs resolved) + +#### security-review +- **Bug 1**: Removed phantom `AskUserQuestion` from Phase 1 Step 1 +- **Bug 2**: Replaced inline output format with `{OUTPUT_FORMAT}` placeholder to prevent nested fence corruption +- **Bug 3**: Removed archive step and code-edit instruction from workflow +- **Bug 4**: Fixed heading level for Step 1.8 (was H2, should be H3) + +#### pr-review & parallel-review +- **Bug 5**: Fixed `$SCOPE` undefined error and invalid `subagent_type` parameter + +### 2. Information Barrier Implementation (3 skills) + +Added independent analysis phase (Step 2.5) before polling Codex output: + +- **security-review**: Claude analyzes code independently using OWASP/CWE checklist, produces FINDING-{N} list, then cross-analyzes with Codex's ISSUE-{M} list +- **plan-review**: Claude reviews plan independently, produces FINDING-{N} list, then cross-analyzes with Codex's ISSUE-{M} list +- **impl-review**: Claude reviews code independently, produces FINDING-{N} list, then cross-analyzes with Codex's ISSUE-{M} list + +Created `claude-analysis-template.md` with structured format for independent analysis and cross-analysis mapping. + +### 3. VERDICT Vocabulary Unification (3 skills) + +Changed from `APPROVE | REVISE` to `CONSENSUS | CONTINUE | STALEMATE`: + +- **security-review**: Updated SKILL.md, workflow.md, prompts.md, output-format.md +- **plan-review**: Updated workflow.md, prompts.md, output-format.md +- **impl-review**: Updated workflow.md, prompts.md, output-format.md + +## Additional Fixes (6 rounds of codex-impl-review) + +After completing the 12 planned tasks, ran adversarial review which identified and fixed: + +### Round 1-3: Nested Fence Corruption & Gate Bypass +- Removed outer fence wrappers from Round 1 and Round 2+ prompt templates +- Removed outer fences from example sections in prompts.md +- Updated pre-commit hook to check both CONTINUE and STALEMATE verdicts +- Added verdict counting (requires exactly 1 verdict AND it must be CONSENSUS) + +### Round 4: Fail-Open Gate & Output Format Fences +- Fixed grep -c fail-open issue by switching to awk-based counting with explicit zero normalization +- Removed outer fences from output-format.md examples (Complete Security Finding, Secrets Detection, Response Format) + +### Round 5: Prompt Assembly & Base Branch Discovery +- Clarified prompt assembly to extract only Round 1 section (exclude Round 2+ sections) +- Added base branch auto-discovery (Step 1) before validation (Step 2) +- Removed stray closing fence after Response Format (Round 2+) +- Fixed Status column reference in claude-analysis-template.md + +### Round 6: Review-Only Behavior Alignment +- Fixed SKILL.md step 6 contradiction: changed from "Fix valid vulnerabilities in code" to "Validate findings, prepare rebuttals or severity adjustments, and provide evidence without editing code" +- Aligns with review-only contract throughout all documentation + +## Final State + +✅ All nested fence corruption eliminated +✅ Pre-commit gate is truly fail-closed with awk-based counting +✅ Category taxonomy aligned across all security-review files +✅ VERDICT vocabulary unified (CONSENSUS|CONTINUE|STALEMATE) +✅ Prompt assembly extracts only Round 1 section +✅ Base branch discovery happens before validation +✅ All skill files internally consistent +✅ Review-only behavior consistent throughout + +## Files Changed + +``` +skill-packs/codex-review/skills/codex-impl-review/references/output-format.md +skill-packs/codex-review/skills/codex-impl-review/references/prompts.md +skill-packs/codex-review/skills/codex-impl-review/references/workflow.md +skill-packs/codex-review/skills/codex-parallel-review/references/workflow.md +skill-packs/codex-review/skills/codex-plan-review/references/output-format.md +skill-packs/codex-review/skills/codex-plan-review/references/prompts.md +skill-packs/codex-review/skills/codex-plan-review/references/workflow.md +skill-packs/codex-review/skills/codex-pr-review/references/workflow.md +skill-packs/codex-review/skills/codex-security-review/SKILL.md +skill-packs/codex-review/skills/codex-security-review/references/claude-analysis-template.md (new) +skill-packs/codex-review/skills/codex-security-review/references/output-format.md +skill-packs/codex-review/skills/codex-security-review/references/prompts.md +skill-packs/codex-review/skills/codex-security-review/references/workflow.md +``` + +## Testing + +All changes verified through 6 rounds of adversarial review using `/codex-impl-review`. Final review found only 2 minor edge cases (branch auto-detection without upstream, Round 2 workflow clarity) which are documented but not blocking. + +## Breaking Changes + +None. All changes are internal improvements to skill implementation. The skill invocation interface (`/codex-security-review`, `/codex-plan-review`, `/codex-impl-review`) remains unchanged. diff --git a/docs/superpowers/plans/2026-03-22-approach-a-skill-improvements.md b/docs/superpowers/plans/2026-03-22-approach-a-skill-improvements.md new file mode 100644 index 0000000..60ac6bf --- /dev/null +++ b/docs/superpowers/plans/2026-03-22-approach-a-skill-improvements.md @@ -0,0 +1,925 @@ +# Approach A — Skill Implementation Improvements + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Fix 5 critical bugs + add information barriers + unify VERDICT vocabulary across 3 codex-review skills (plan-review, impl-review, security-review). + +**Architecture:** All changes are pure Markdown edits to `references/` files inside `skill-packs/codex-review/skills/`. No code (runner, installer, SKILL.md) is touched. Each task is isolated to one skill directory and can be verified by inspecting the changed file against the spec. + +**Tech Stack:** Markdown files only. No build system, no tests, no npm. Verification is visual inspection + grep for specific strings. + +--- + +## File Map + +| File | Task | +|------|------| +| `skill-packs/codex-review/skills/codex-security-review/references/workflow.md` | T1, T4, T5, T8, T9 | +| `skill-packs/codex-review/skills/codex-security-review/references/prompts.md` | T2, T9 | +| `skill-packs/codex-review/skills/codex-security-review/references/output-format.md` | T9 | +| `skill-packs/codex-review/skills/codex-security-review/references/claude-analysis-template.md` | T5 (NEW FILE) | +| `skill-packs/codex-review/skills/codex-pr-review/references/workflow.md` | T3 | +| `skill-packs/codex-review/skills/codex-parallel-review/references/workflow.md` | T3 | +| `skill-packs/codex-review/skills/codex-plan-review/references/workflow.md` | T6, T10 | +| `skill-packs/codex-review/skills/codex-plan-review/references/prompts.md` | T10 | +| `skill-packs/codex-review/skills/codex-plan-review/references/output-format.md` | T10 | +| `skill-packs/codex-review/skills/codex-impl-review/references/workflow.md` | T7, T11 | +| `skill-packs/codex-review/skills/codex-impl-review/references/prompts.md` | T11 | +| `skill-packs/codex-review/skills/codex-impl-review/references/output-format.md` | T11 | + +--- + +## Task 1: Delete phantom AskUserQuestion step (Bug 1) + +**Files:** +- Modify: `skill-packs/codex-review/skills/codex-security-review/references/workflow.md` + +**Context:** The file has a "Smart Default Detection" block near the top that correctly announces `scope=$SCOPE, effort=$EFFORT`. Immediately after that block, Phase 1 Step 1 says `Use AskUserQuestion to collect these inputs in a single prompt.` — this tool does not exist in Claude Code. The fix is simply deleting Step 1. Do NOT add a second announce block. + +- [ ] **Step 1: Read the current file and locate Phase 1 Step 1** + + Open `skill-packs/codex-review/skills/codex-security-review/references/workflow.md`. Find the block that contains `AskUserQuestion`. Confirm the Smart Default Detection block above it already has the announce pattern (`Announce: "Detected: scope=..."`). + +- [ ] **Step 2: Delete Phase 1 Step 1 entirely** + + Remove the entire Step 1 block (from `## Phase 1: ...Step 1` or equivalent heading down to where Step 2 begins). Do not touch the Smart Default Detection block above it. + +- [ ] **Step 3: Verify** + + ```bash + grep -n "AskUserQuestion" skill-packs/codex-review/skills/codex-security-review/references/workflow.md + ``` + Expected: no output (zero matches). + + ```bash + grep -n "Announce" skill-packs/codex-review/skills/codex-security-review/references/workflow.md + ``` + Expected: exactly one match in the Smart Default Detection block. + +- [ ] **Step 4: Commit** + + ```bash + git add skill-packs/codex-review/skills/codex-security-review/references/workflow.md + git commit -m "fix(security-review): remove phantom AskUserQuestion from Phase 1 Step 1" + ``` + +--- + +## Task 2: Fix nested triple-backtick in prompts.md — Part A (Bug 2) + +**Files:** +- Modify: `skill-packs/codex-review/skills/codex-security-review/references/prompts.md` + +**Context:** The Round 1 prompt template is inside a triple-backtick fence. Inside that fence, there are sections (`## Output Format`, VERDICT Block) that also use triple-backtick fences. The inner fences prematurely close the outer fence, breaking the template. The fix: remove the `## Output Format` section and VERDICT Block from inside the fenced prompt, and replace them with the single line `{OUTPUT_FORMAT}` at the end of the prompt body (before the closing fence). + +Compare with `codex-impl-review/references/prompts.md` — it uses `## Required Output Format\n{OUTPUT_FORMAT}` at the bottom of each prompt template. Use the same pattern. + +- [ ] **Step 1: Read both files to understand the difference** + + Read `skill-packs/codex-review/skills/codex-security-review/references/prompts.md` and identify the inline `## Output Format` / VERDICT Block sections inside the outer fence. + + Read `skill-packs/codex-review/skills/codex-impl-review/references/prompts.md` lines 50-80 to see how `{OUTPUT_FORMAT}` placeholder is used correctly. + +- [ ] **Step 2: Edit prompts.md** + + In the Round 1 prompt template (the outermost fenced block): + - Remove the `## Output Format` section and its content (the nested fence with ISSUE-{N} format) + - Remove the VERDICT Block section (the nested fence with VERDICT template) + - At the end of the prompt body (just before the outer closing fence), add: + + ``` + ## Required Output Format + {OUTPUT_FORMAT} + ``` + + Apply the same change to the Round 2+ / Rebuttal prompt template if it also has inline output format. + +- [ ] **Step 3: Verify nested Output Format sections were removed and placeholder is present** + + ```bash + # Verify the inline Output Format and VERDICT Block sections were removed from the templates + grep -n "## Output Format\|## Verdict Block\|VERDICT: APPROVE | REVISE\|VERDICT: APPROVE \| REVISE" \ + skill-packs/codex-review/skills/codex-security-review/references/prompts.md + ``` + Expected: zero matches (these were inside the fenced template and are now gone). + + ```bash + # Verify the {OUTPUT_FORMAT} placeholder is present in each template + grep -n "OUTPUT_FORMAT" skill-packs/codex-review/skills/codex-security-review/references/prompts.md + ``` + Expected: at least 1 match per prompt template (Round 1 and Round 2+ / Rebuttal). + + ```bash + # Verify no nested triple-backticks remain inside any template + grep -n "Required Output Format" skill-packs/codex-review/skills/codex-security-review/references/prompts.md + ``` + Expected: at least 1 match showing the `## Required Output Format\n{OUTPUT_FORMAT}` pattern was added. + +- [ ] **Step 4: Commit** + + ```bash + git add skill-packs/codex-review/skills/codex-security-review/references/prompts.md + git commit -m "fix(security-review): replace inline output format with {OUTPUT_FORMAT} placeholder" + ``` + +--- + +## Task 3: Fix simple one-line bugs (Bug 3 + Bug 4) + +**Files:** +- Modify: `skill-packs/codex-review/skills/codex-pr-review/references/workflow.md` +- Modify: `skill-packs/codex-review/skills/codex-parallel-review/references/workflow.md` + +### Bug 3 — pr-review: `$SCOPE` undefined in meta.json + +- [ ] **Step 1: Find the Session Finalization heredoc in pr-review workflow.md** + + ```bash + grep -n 'SCOPE' skill-packs/codex-review/skills/codex-pr-review/references/workflow.md + ``` + Find the line `"scope": "$SCOPE"` in the `meta.json` heredoc. + +- [ ] **Step 2: Replace `$SCOPE` with `$BASE_BRANCH`** + + Change: + ``` + "scope": "$SCOPE", + ``` + To: + ``` + "scope": "$BASE_BRANCH", + ``` + +- [ ] **Step 3: Verify** + + ```bash + grep -n '"scope"' skill-packs/codex-review/skills/codex-pr-review/references/workflow.md + ``` + Expected: shows `"scope": "$BASE_BRANCH"` (not `$SCOPE`). + +### Bug 4 — parallel-review: Invalid `subagent_type` + +- [ ] **Step 4: Find all `subagent_type` occurrences** + + ```bash + grep -n "subagent_type" skill-packs/codex-review/skills/codex-parallel-review/references/workflow.md + ``` + Expected: 5 matches — 4 in JSON blocks (Agent 1-4), 1 in a prose description paragraph. + +- [ ] **Step 5: Remove `subagent_type` from all 4 JSON blocks** + + For each JSON agent spawn block, remove the line: + ```json + "subagent_type": "code-reviewer", + ``` + Do not remove any other fields from the JSON. + +- [ ] **Step 6: Update the prose description paragraph** + + Find the paragraph (line ~53) that reads something like `Each uses subagent_type: "code-reviewer"`. Remove the `subagent_type: "code-reviewer"` reference or replace with: `Each uses the default general-purpose agent behavior.` + +- [ ] **Step 7: Verify** + + ```bash + grep -n "subagent_type" skill-packs/codex-review/skills/codex-parallel-review/references/workflow.md + ``` + Expected: no output (zero matches). + +- [ ] **Step 8: Commit both fixes** + + ```bash + git add skill-packs/codex-review/skills/codex-pr-review/references/workflow.md + git add skill-packs/codex-review/skills/codex-parallel-review/references/workflow.md + git commit -m "fix(pr-review,parallel-review): fix \$SCOPE undefined and invalid subagent_type" + ``` + +--- + +## Task 4: Fix archive step + code editing instruction (Bug 5) + +**Files:** +- Modify: `skill-packs/codex-review/skills/codex-security-review/references/workflow.md` + +**Context:** Two problems in the same file: +1. Phase 5 Step 3 copies artifacts to `docs/security-reviews/` — violates v12 session model. +2. Phase 3 Option A says "Apply the suggested fix" — no review skill should edit user code. + +After removing Option A, renumber: old Option B → new Option A, old Option C → new Option B. Remove the lettered "Option" framing and use a flat response list matching `codex-impl-review` workflow.md Step 4 style. + +Also, as part of this task, update all `APPROVE`/`REVISE` strings in this file (enumerated exactly): +- Phase 3 stop condition: `VERDICT: APPROVE` → `VERDICT: CONSENSUS` +- Phase 4 Step 4 "Iterate Until Consensus": `VERDICT: APPROVE - All critical/high issues resolved` → `VERDICT: CONSENSUS - All critical/high issues resolved` +- Phase 5 Step 2 Final Verdict header: `{APPROVE | REVISE | STALEMATE}` → `{CONSENSUS | CONTINUE | STALEMATE}` +- CI/CD pre-commit hook example: `grep -q "VERDICT: REVISE"` → `grep -q "VERDICT: CONTINUE"` + +(The workflow.md VERDICT updates for Section 3 are bundled here since they're all in the same file.) + +- [ ] **Step 1: Read the current Phase 3 section** + + Read `skill-packs/codex-review/skills/codex-security-review/references/workflow.md`. Find Phase 3 with its 3 options. Also find Phase 5 Step 3 (the archive block). + +- [ ] **Step 2: Remove Phase 5 Step 3 archive block** + + Delete the entire Step 3 block in Phase 5 that runs `mkdir -p docs/security-reviews/` and copies artifacts. + +- [ ] **Step 3: Remove Phase 3 Option A and renumber** + + - Delete the entire Option A block (the "Apply the suggested fix" / code editing block). + - Rename `Option B` → `Option A` (or remove "Option" labels entirely and use a flat list like impl-review). + - Rename `Option C` → `Option B` (or same). + - Match the flat style from `codex-impl-review` workflow.md Step 4: + - Response type 1: rebuttal (write concrete proof that finding is wrong) + - Response type 2: acknowledge with severity dispute (finding is valid but severity is lower) + +- [ ] **Step 4: Update all APPROVE/REVISE in this file** + + Make these **5** exact replacements (the 5th catches an additional occurrence in Phase 2 example): + + | Old text | New text | + |----------|----------| + | `VERDICT: APPROVE` (Phase 3 stop condition) | `VERDICT: CONSENSUS` | + | `VERDICT: APPROVE - All critical/high issues resolved` | `VERDICT: CONSENSUS - All critical/high issues resolved` | + | `{APPROVE \| REVISE \| STALEMATE}` (Final Verdict header) | `{CONSENSUS \| CONTINUE \| STALEMATE}` | + | `grep -q "VERDICT: REVISE"` (CI/CD example) | `grep -q "VERDICT: CONTINUE"` | + | `**Verdict**: REVISE` (Phase 2 example line) | `**Verdict**: CONTINUE` | + +- [ ] **Step 5: Verify** + + ```bash + # Check all old vocabulary forms are gone (including non-VERDICT:-prefixed forms) + grep -n "AskUserQuestion\|docs/security-reviews\|Option A\|Apply the suggested fix\|VERDICT: APPROVE\|VERDICT: REVISE\|Verdict.*APPROVE\|Verdict.*REVISE" \ + skill-packs/codex-review/skills/codex-security-review/references/workflow.md + ``` + Expected: zero matches (all removed/replaced). + + ```bash + grep -n "VERDICT: CONSENSUS\|VERDICT: CONTINUE\|VERDICT: STALEMATE\|Verdict.*CONTINUE" \ + skill-packs/codex-review/skills/codex-security-review/references/workflow.md + ``` + Expected: at least 5 matches (one per replacement location). + +- [ ] **Step 6: Commit** + + ```bash + git add skill-packs/codex-review/skills/codex-security-review/references/workflow.md + git commit -m "fix(security-review): remove archive step, code-edit instruction, unify VERDICT vocab in workflow" + ``` + +--- + +## Task 5: Add prompt assembly step to security-review workflow (Bug 2 — Part B) + new claude-analysis-template.md + +**Files:** +- Modify: `skill-packs/codex-review/skills/codex-security-review/references/workflow.md` +- Create: `skill-packs/codex-review/skills/codex-security-review/references/claude-analysis-template.md` + +### Part A — Prompt assembly step in workflow.md + +**Context:** After Bug 2 Part A (Task 2), `prompts.md` now has `{OUTPUT_FORMAT}` placeholder. But security-review has no prompt assembly step, so `{OUTPUT_FORMAT}` would never be substituted. Add a Step 1.8 modeled exactly on `codex-impl-review/references/workflow.md` Step 1.8. + +Read `codex-impl-review/references/workflow.md` to find the exact prompt assembly step. It reads `references/prompts.md` as template, replaces each placeholder in sequence using `printf '%s'` (never a single sed pipeline), stores as `$PROMPT`, and verifies no broken backtick fences. + +- [ ] **Step 1: Read codex-impl-review Step 1.8 as the reference pattern** + + Read `skill-packs/codex-review/skills/codex-impl-review/references/workflow.md` and find the prompt assembly step (Step 1.8). Note the exact shell pattern used. + +- [ ] **Step 2: Add Step 1.8 to security-review workflow.md** + + After the existing Step 1 (input collection/detection) and before the `start` command, insert a new step: + + ``` + ## 1.8) Assemble Prompt + + Build `$PROMPT` using multi-step placeholder replacement. + DO NOT use a single sed pipeline — `output-format.md` may contain `&`, `\`, `/` characters + that corrupt sed replacements. Use `printf '%s'` piping. + + a) Read `references/prompts.md` as template base + b) Replace `{WORKING_DIR}` with current working directory + c) Replace `{SCOPE}` with detected `$SCOPE` value + d) Replace `{EFFORT}` with detected `$EFFORT` value + e) Replace `{SCOPE_SPECIFIC_INSTRUCTIONS}` with the scope-specific block from prompts.md + matching the detected `$SCOPE` (working-tree / branch / full) + f) Replace `{OUTPUT_FORMAT}` by reading `references/output-format.md` in full + using: `printf '%s' "$(cat references/output-format.md)"` + g) Replace any remaining placeholders (`{BASE_BRANCH}` etc.) + + Store result as `$PROMPT`. + + Verify: `$PROMPT` must contain no prematurely-closed triple-backtick fences. + ``` + +- [ ] **Step 3: Verify prompt assembly step exists** + + ```bash + grep -n "Assemble Prompt\|OUTPUT_FORMAT\|printf '%s'" \ + skill-packs/codex-review/skills/codex-security-review/references/workflow.md + ``` + Expected: matches showing the new assembly step with `OUTPUT_FORMAT` and `printf '%s'`. + +### Part B — New claude-analysis-template.md + +**Context:** The information barrier pattern requires Claude to analyze independently before polling. `codex-pr-review/references/claude-analysis-template.md` and `codex-commit-review/references/claude-analysis-template.md` are the gold standard. Security review needs its own version with OWASP/CWE fields instead of PR-specific fields. + +- [ ] **Step 4: Read the gold standard template for reference** + + Read `skill-packs/codex-review/skills/codex-pr-review/references/claude-analysis-template.md`. + +- [ ] **Step 5: Create security-review claude-analysis-template.md** + + Create `skill-packs/codex-review/skills/codex-security-review/references/claude-analysis-template.md` with this content: + + ```markdown + # Claude Independent Security Analysis Template + + > Use this template for Step 2.5 (Information Barrier). + > Record analysis in working context ONLY — do NOT write to a file. + > Do NOT read `$SESSION_DIR/review.md` until this analysis is complete. + + ## FINDING-{N} Format + + Use this exact shape for each independent finding: + + ### FINDING-{N}: {Short title} + - Category: injection | broken-auth | sensitive-data | broken-access | security-config | xss | insecure-deserialization | logging | ssrf | crypto-failure | insecure-design | vulnerable-components | integrity-failure | secrets + - Severity: low | medium | high | critical + - Confidence: low | medium | high + - CWE: {CWE-NNN if known, otherwise omit} + - OWASP: {A0N:2021 category if applicable, otherwise omit} + - Location: {file path:line range} + - Problem: {clear statement of the vulnerability or security weakness} + - Evidence: {specific code pattern, snippet, or observation} + - Attack Vector: {how an attacker could exploit this} + - Why it matters: {impact — data exposure, privilege escalation, etc.} + + ## Overall Security Assessment + + - Attack surface: {high / medium / low — based on exposed endpoints, data handled} + - Most critical area: {the single highest-risk component or pattern found} + - Confidence in analysis: {high / medium / low — limited by static analysis} + + ## Strongest Positions + + List the 3-5 findings Claude is most confident about for cross-analysis debate: + + 1. FINDING-{N}: {title} — {one-sentence rationale for high confidence} + 2. ... + + ## Cross-Analysis Matching Protocol + + After Round 1 poll completes, build a mapping table: + + | Claude FINDING-{N} | Codex ISSUE-{M} | Classification | + |--------------------|-----------------|----------------| + | FINDING-1 | ISSUE-2 | Genuine Agreement | + | FINDING-2 | — | Claude-only | + | — | ISSUE-4 | Codex-only | + | FINDING-3 | ISSUE-5 | Genuine Disagreement | + + Classification rules: + - **Genuine Agreement**: Same vulnerability class + same file/line area + - **Genuine Disagreement**: Same code area but conflicting assessment (one says vulnerable, other says safe) + - **Same Direction / Different Severity**: Both flag the same issue but assign different severity + - **Claude-only**: Claude's finding has no Codex counterpart + - **Codex-only**: Codex's finding has no Claude counterpart + + Maintain this table across all rounds. Update Classification and Status columns as rounds progress. + ``` + +- [ ] **Step 6: Verify the new file exists and contains FINDING-{N} format and Cross-Analysis table** + + ```bash + grep -n "FINDING-\|Cross-Analysis\|Classification" \ + skill-packs/codex-review/skills/codex-security-review/references/claude-analysis-template.md + ``` + Expected: matches showing `FINDING-{N}` format, `Cross-Analysis Matching Protocol`, and `Classification` column header. + +- [ ] **Step 7: Commit both changes** + + ```bash + git add skill-packs/codex-review/skills/codex-security-review/references/workflow.md + git add skill-packs/codex-review/skills/codex-security-review/references/claude-analysis-template.md + git commit -m "feat(security-review): add prompt assembly step and claude-analysis-template.md" + ``` + +--- + +## Task 6: Add information barrier to codex-plan-review + +**Files:** +- Modify: `skill-packs/codex-review/skills/codex-plan-review/references/workflow.md` + +**Context:** The current workflow goes: Step 2 (start Codex) → Step 3 (poll). We need to insert Step 2.5 between them and transform Step 4 "Parse Review" into a Cross-Analysis step. + +Read `codex-pr-review/references/workflow.md` Steps 2.5 and 4 as the gold standard for the pattern. + +- [ ] **Step 1: Read current plan-review workflow.md** + + Read `skill-packs/codex-review/skills/codex-plan-review/references/workflow.md`. Identify: + - Where Step 2 (start command) ends + - Where Step 3 (Poll) begins — this is where Step 2.5 goes + - The content of Step 4 "Parse Review" / Apply fixes — this will be transformed + +- [ ] **Step 2: Insert Step 2.5 after Step 2** + + After the `node "$RUNNER" start` command block in Step 2, add: + + ```markdown + ## 2.5) Information Barrier — Claude Independent Plan Analysis + + MUST complete before polling Codex output. + Codex is running in background — use this time productively. + + Read the plan file at `$PLAN_PATH` directly. + Do NOT read `$SESSION_DIR/review.md` until this analysis is complete. + + Form an independent FINDING-{N} list in working context (do NOT write to a file): + - Correctness issues (steps that are wrong or will fail) + - Architecture concerns (structural problems with the approach) + - Sequencing/dependency problems (steps out of order, missing prerequisites) + - Scope gaps or risks (missing requirements, underestimated complexity) + + Use the same FINDING-{N} format as `output-format.md` ISSUE-{N} (same field names). + + INFORMATION BARRIER ends after Round 1 poll completes. + From Round 2 onwards, the barrier no longer applies. + ``` + +- [ ] **Step 3: Transform Step 4 into Cross-Analysis** + + Replace the current Step 4 "Parse Review" / fix application with: + + ```markdown + ## 4) Cross-Analysis + + After Round 1 poll completes and `$SESSION_DIR/review.md` is available: + + ### 4a) Parse Codex Output + Read all `ISSUE-{N}` blocks from `$SESSION_DIR/review.md`. + + ### 4b) Build FINDING↔ISSUE Mapping Table + Map Claude's FINDING-{N} (from Step 2.5) against Codex's ISSUE-{N}: + + | Claude FINDING-{N} | Codex ISSUE-{M} | Classification | + |--------------------|-----------------|----------------| + | ... | ... | ... | + + Classification options: + - **Genuine Agreement**: FINDING-{N} and ISSUE-{M} identify the same plan problem + - **Codex-only**: ISSUE-{M} has no matching Claude FINDING + - **Claude-only**: FINDING-{N} has no matching Codex ISSUE + - **Genuine Disagreement**: Conflicting assessments of the same plan section + + ### 4c) Determine Response for Each ISSUE + For each ISSUE-{N}: + - Genuine agreement or Codex-only → apply fix to the plan file + - Claude-only → include in final report as Claude finding + - Genuine disagreement → write rebuttal with concrete reasoning + + ### 4d) Apply Fixes + Apply accepted fixes to the plan file at `$PLAN_PATH`. + Save the updated plan file before resuming. + **Critical**: Codex Round 2+ re-reads the plan from `$PLAN_PATH` — unsaved changes are invisible to Codex. + + ### 4e) Record Round Summary + Append to `$SESSION_DIR/rounds.json`: + ```json + { "round": N, "elapsed_seconds": ..., "verdict": "...", "issues_found": ..., "issues_fixed": ..., "issues_disputed": ... } + ``` + + Proceed to Step 5 (resume) or Step 7 (final output) based on VERDICT. + ``` + +- [ ] **Step 4: Verify Step 2.5 and cross-analysis are present** + + ```bash + grep -n "Information Barrier\|FINDING-{N}\|Cross-Analysis\|4b\|PLAN_PATH.*unsaved" \ + skill-packs/codex-review/skills/codex-plan-review/references/workflow.md + ``` + Expected: matches for each of these patterns. + +- [ ] **Step 5: Commit** + + ```bash + git add skill-packs/codex-review/skills/codex-plan-review/references/workflow.md + git commit -m "feat(plan-review): add information barrier Step 2.5 and cross-analysis Step 4" + ``` + +--- + +## Task 7: Add information barrier to codex-impl-review + +**Files:** +- Modify: `skill-packs/codex-review/skills/codex-impl-review/references/workflow.md` + +**Context:** Same structure as plan-review. Insert Step 2.5 between Step 2 (start) and Step 3 (poll). Extend Step 4 "Apply/Rebut" with cross-analysis sub-steps. Code editing is PRESERVED — cross-analysis only enriches the apply/rebuttal decision, it does not remove code editing. + +- [ ] **Step 1: Read current impl-review workflow.md** + + Read `skill-packs/codex-review/skills/codex-impl-review/references/workflow.md`. Identify: + - Where Step 2 (start command) ends — note this is mode-conditional (working-tree vs branch) + - Where Step 3 (Poll) begins + - The content of Step 4 "Apply/Rebut" — this will be extended, not replaced + +- [ ] **Step 2: Insert Step 2.5 after Step 2** + + After the `node "$RUNNER" start` command block, add: + + ```markdown + ## 2.5) Information Barrier — Claude Independent Code Analysis + + MUST complete before polling Codex output. + Codex is running in background — use this time productively. + + **working-tree mode**: run `git diff` and `git diff --cached` yourself + (or reuse the diff already collected in Step 1 — working tree hasn't changed) + **branch mode**: run `git diff $BASE_BRANCH...HEAD` yourself + (or reuse the diff already collected in Step 1) + + Form an independent FINDING-{N} list in working context (do NOT write to a file): + - Bugs and edge cases + - Security issues + - Performance concerns + - Maintainability problems + + Use the same FINDING-{N} format as `output-format.md` ISSUE-{N} (same field names). + Do NOT read `$SESSION_DIR/review.md` until this analysis is complete. + + INFORMATION BARRIER ends after Round 1 poll completes. + ``` + +- [ ] **Step 3: Extend Step 4 with cross-analysis sub-steps** + + Before the existing "For valid issues: edit code and record fix evidence" line in Step 4, add: + + ```markdown + ### 4a) Parse Codex Output + Read all `ISSUE-{N}` blocks from `$SESSION_DIR/review.md`. + + ### 4b) Build FINDING↔ISSUE Mapping Table + Map Claude's FINDING-{N} (from Step 2.5) against Codex's ISSUE-{N}: + + | Claude FINDING-{N} | Codex ISSUE-{M} | Classification | + |--------------------|-----------------|----------------| + | ... | ... | ... | + + Classification options: + - **Genuine Agreement**: FINDING-{N} and ISSUE-{M} identify the same code problem + - **Codex-only**: ISSUE-{M} has no matching Claude FINDING + - **Claude-only**: FINDING-{N} has no matching Codex ISSUE + - **Genuine Disagreement**: Conflicting assessments of the same code + + ### 4c) Apply/Rebut using cross-analysis context + For each ISSUE-{N}: + - If genuine agreement or Codex-only → apply fix to code + - If Claude-only → include in final report as Claude finding + - If genuine disagreement → write rebuttal with concrete proof (paths, tests, behavior) + ``` + + Keep all existing content below (the branch mode commit requirement, verification requirement, rounds.json update, etc.) — do NOT remove them. + +- [ ] **Step 4: Verify Step 2.5 and cross-analysis are present, code editing is preserved** + + ```bash + grep -n "Information Barrier\|FINDING-{N}\|Cross-Analysis\|4b\|edit code\|commit.*fixes\|branch mode only" \ + skill-packs/codex-review/skills/codex-impl-review/references/workflow.md + ``` + Expected: matches for both the new cross-analysis content AND the existing "edit code" / "branch mode only" / commit lines. + +- [ ] **Step 5: Commit** + + ```bash + git add skill-packs/codex-review/skills/codex-impl-review/references/workflow.md + git commit -m "feat(impl-review): add information barrier Step 2.5 and cross-analysis in Step 4" + ``` + +--- + +## Task 8: Add information barrier Step 2.5 to security-review workflow + +**Files:** +- Modify: `skill-packs/codex-review/skills/codex-security-review/references/workflow.md` + +**Context:** Same information barrier pattern as plan-review and impl-review. Insert Step 2.5 after Phase 2 `start` command. Also update Phase 3 cross-analysis step to use FINDING↔ISSUE mapping (matching pr-review gold standard). + +- [ ] **Step 1: Identify insertion point in security-review workflow.md** + + Read `skill-packs/codex-review/skills/codex-security-review/references/workflow.md`. Find Phase 2 start command and Phase 2 polling step. Step 2.5 goes between them. + +- [ ] **Step 2: Insert Step 2.5 in Phase 2** + + After the `node "$RUNNER" start` command in Phase 2, add: + + ```markdown + ### Phase 2, Step 2.5: Information Barrier — Claude Independent Security Analysis + + MUST complete before polling Codex output. + Codex is running in background (typically 90-180s) — use this time. + + Using `references/claude-analysis-template.md`: + - Read all files in scope directly (do NOT read `$SESSION_DIR/review.md`) + - Identify top attack surfaces + - Form an independent FINDING-{N} list using OWASP categories + - Note high-confidence vs uncertain findings + + Keep analysis in working context. Do NOT write a file. + INFORMATION BARRIER ends after Round 1 poll completes. + From Round 2 onwards, the barrier no longer applies. + ``` + +- [ ] **Step 3: Update Phase 3 cross-analysis step** + + Find the Phase 3 step that processes Codex findings — specifically look for **Phase 3, Step 3** (or the step with a heading like "Parse Security Findings" or "Process Codex Output") that comes after Phase 2 polling completes. This is the step that reads `review.md` and processes each ISSUE block. Add FINDING↔ISSUE mapping to the beginning of that step, using the same classification table format as in Tasks 6 and 7 (Genuine Agreement / Genuine Disagreement / Claude-only / Codex-only / Same Direction Different Severity). + +- [ ] **Step 4: Verify Step 2.5, cross-analysis, and classification markers are present** + + ```bash + grep -n "Information Barrier\|claude-analysis-template\|FINDING.*ISSUE.*Classification\|Genuine Agreement\|Claude-only\|Codex-only\|working context" \ + skill-packs/codex-review/skills/codex-security-review/references/workflow.md + ``` + Expected: matches for Information Barrier step, template reference, FINDING↔ISSUE mapping table header, and all classification types. + +- [ ] **Step 5: Commit** + + ```bash + git add skill-packs/codex-review/skills/codex-security-review/references/workflow.md + git commit -m "feat(security-review): add information barrier Step 2.5 and FINDING/ISSUE cross-analysis" + ``` + +--- + +## Task 9: Unify VERDICT vocabulary in security-review output-format.md and prompts.md + +**Files:** +- Modify: `skill-packs/codex-review/skills/codex-security-review/references/output-format.md` +- Modify: `skill-packs/codex-review/skills/codex-security-review/references/prompts.md` + +**Context:** The primary VERDICT block in `output-format.md` currently shows `Status: APPROVE | REVISE`. The `prompts.md` may have similar inline VERDICT values. Both need to use `CONSENSUS | CONTINUE | STALEMATE`. Note: the `Status` field (separate from `VERDICT`) with values like `{complete | stalemate | in-progress}` is kept as-is. + +- [ ] **Step 1: Update output-format.md VERDICT block** + + Read `skill-packs/codex-review/skills/codex-security-review/references/output-format.md`. Find the primary VERDICT block (the one used in normal reviews, not the stalemate format). Change: + + ``` + - Status: APPROVE | REVISE + ``` + To: + ``` + - Status: CONSENSUS | CONTINUE | STALEMATE + ``` + + Add descriptions for each value: + - `CONSENSUS`: All critical/high security issues resolved or agreed-upon — ready to proceed + - `CONTINUE`: Security issues remain that require another review round + - `STALEMATE`: Circular debate — same disputes for 2+ rounds with no progress + +- [ ] **Step 2: Update prompts.md VERDICT references** + + In `skill-packs/codex-review/skills/codex-security-review/references/prompts.md`, find any instruction lines that reference VERDICT values (e.g., "End with a VERDICT block using APPROVE or REVISE"). Update to reference `CONSENSUS | CONTINUE | STALEMATE` and add: "Use STALEMATE if same arguments repeat for 2+ consecutive rounds with no new evidence." + + The inline VERDICT format sections were already removed in Task 2 (they are now handled by `{OUTPUT_FORMAT}` injection). Only update the instruction text lines. + +- [ ] **Step 3: Verify** + + ```bash + grep -n "APPROVE\|REVISE" \ + skill-packs/codex-review/skills/codex-security-review/references/output-format.md \ + skill-packs/codex-review/skills/codex-security-review/references/prompts.md + ``` + Expected: zero matches for these old vocabulary values in both files. (The stalemate format section already uses `STALEMATE` which is fine.) + + ```bash + grep -n "CONSENSUS\|CONTINUE\|STALEMATE" \ + skill-packs/codex-review/skills/codex-security-review/references/output-format.md \ + skill-packs/codex-review/skills/codex-security-review/references/prompts.md + ``` + Expected: at least one match each in both files. + +- [ ] **Step 4: Commit** + + ```bash + git add skill-packs/codex-review/skills/codex-security-review/references/output-format.md + git add skill-packs/codex-review/skills/codex-security-review/references/prompts.md + git commit -m "fix(security-review): unify VERDICT vocabulary to CONSENSUS/CONTINUE/STALEMATE" + ``` + +--- + +## Task 10: Unify VERDICT vocabulary in codex-plan-review + +**Files:** +- Modify: `skill-packs/codex-review/skills/codex-plan-review/references/output-format.md` +- Modify: `skill-packs/codex-review/skills/codex-plan-review/references/prompts.md` +- Modify: `skill-packs/codex-review/skills/codex-plan-review/references/workflow.md` + +**Context:** plan-review currently uses `APPROVE | REVISE`. Three files need updates. In `prompts.md`, the VERDICT values are injected via `{OUTPUT_FORMAT}` placeholder (not inline), so only the instruction line "End with a VERDICT block" needs updating. In `workflow.md`, update stop conditions and the final report summary table's Verdict row value. + +- [ ] **Step 1: Update output-format.md VERDICT block (all occurrences)** + + Read `skill-packs/codex-review/skills/codex-plan-review/references/output-format.md`. Make these exact replacements: + + **Primary Status line:** + ``` + - Status: APPROVE | REVISE + ``` + → Change to: + ``` + - Status: CONSENSUS | CONTINUE | STALEMATE + ``` + + **Zero-issue rule text (line ~24):** Find the sentence: + ``` + Status: APPROVE` and `Reason: Plan is complete, well-structured, and addresses all acceptance criteria. + ``` + Replace `Status: APPROVE` in this sentence with `Status: CONSENSUS`. + + Add descriptions for the updated primary Status values: + - `CONSENSUS`: No remaining plan issues — ready to implement + - `CONTINUE`: Issues remain that require another review round + - `STALEMATE`: Circular debate — same disputes for 2+ rounds with no progress + +- [ ] **Step 2: Update prompts.md instruction line** + + In `skill-packs/codex-review/skills/codex-plan-review/references/prompts.md`, find the line "End with a VERDICT block" (or similar wording) in each prompt template. Add: "Use `STALEMATE` if the same arguments repeat for 2+ consecutive rounds with no new evidence." + + Do NOT change the `{OUTPUT_FORMAT}` placeholder — it already injects the updated values from `output-format.md`. + +- [ ] **Step 3: Update workflow.md stop condition and final report table** + + In `skill-packs/codex-review/skills/codex-plan-review/references/workflow.md`: + + a) Find the stop condition text `VERDICT: APPROVE`. Change to `VERDICT: CONSENSUS`. + + b) Find the final report summary table row for Verdict. Change the value from `{APPROVE/REVISE/STALEMATE}` to `{CONSENSUS/CONTINUE/STALEMATE}`. (This is a value cell, not a table header.) + + c) Update the debate loop branching section: + - `CONSENSUS` → trigger final report (same behavior as old `APPROVE`) + - `CONTINUE` → trigger fix/rebuttal and resume (same behavior as old `REVISE`) + - `STALEMATE` → trigger stalemate handling (check for `STALEMATE` verdict OR same open ISSUE set for 2 consecutive rounds) + +- [ ] **Step 4: Verify** + + ```bash + grep -n "APPROVE\|REVISE" \ + skill-packs/codex-review/skills/codex-plan-review/references/output-format.md \ + skill-packs/codex-review/skills/codex-plan-review/references/prompts.md \ + skill-packs/codex-review/skills/codex-plan-review/references/workflow.md + ``` + Expected: zero matches for old vocabulary in all 3 files (including zero-issue rule text and all workflow occurrences). + + ```bash + grep -n "CONSENSUS\|CONTINUE\|STALEMATE" \ + skill-packs/codex-review/skills/codex-plan-review/references/output-format.md \ + skill-packs/codex-review/skills/codex-plan-review/references/workflow.md + ``` + Expected: at least 2 matches each. + +- [ ] **Step 5: Commit** + + ```bash + git add skill-packs/codex-review/skills/codex-plan-review/references/output-format.md + git add skill-packs/codex-review/skills/codex-plan-review/references/prompts.md + git add skill-packs/codex-review/skills/codex-plan-review/references/workflow.md + git commit -m "fix(plan-review): unify VERDICT vocabulary to CONSENSUS/CONTINUE/STALEMATE" + ``` + +--- + +## Task 11: Unify VERDICT vocabulary in codex-impl-review + +**Files:** +- Modify: `skill-packs/codex-review/skills/codex-impl-review/references/output-format.md` +- Modify: `skill-packs/codex-review/skills/codex-impl-review/references/prompts.md` +- Modify: `skill-packs/codex-review/skills/codex-impl-review/references/workflow.md` + +**Context:** Same as Task 10 but for impl-review. The `prompts.md` has 4 templates (Working Tree Round 1, Branch Round 1, Rebuttal Working-tree, Rebuttal Branch) — each has an instruction line referencing VERDICT that needs updating. VERDICT values are in `output-format.md` via `{OUTPUT_FORMAT}` placeholder. + +- [ ] **Step 1: Update output-format.md VERDICT block (all occurrences)** + + Read `skill-packs/codex-review/skills/codex-impl-review/references/output-format.md`. Make these exact replacements: + + **Primary Status line:** + ``` + - Status: APPROVE | REVISE + ``` + → Change to: + ``` + - Status: CONSENSUS | CONTINUE | STALEMATE + ``` + + **Zero-issue rule text (line ~24):** Find the sentence: + ``` + Status: APPROVE` and `Reason: All changes are correct, well-tested, and safe to merge. + ``` + Replace `Status: APPROVE` in this sentence with `Status: CONSENSUS`. + + Add descriptions for the updated primary Status values: + - `CONSENSUS`: No remaining code issues — changes are correct and safe + - `CONTINUE`: Issues remain that require fixes and another review round + - `STALEMATE`: Circular debate — same disputes for 2+ rounds with no progress + +- [ ] **Step 2: Update prompts.md — all 4 templates** + + In `skill-packs/codex-review/skills/codex-impl-review/references/prompts.md`, for each of the 4 prompt templates, find the line "End with a VERDICT block." Add to it: "Use `STALEMATE` if the same arguments repeat for 2+ consecutive rounds with no new evidence." + +- [ ] **Step 3: Update workflow.md stop conditions and final report** + + Same changes as Task 10 Step 3 but in `codex-impl-review/references/workflow.md`: + + a) Change `VERDICT: APPROVE` stop condition → `VERDICT: CONSENSUS` + + b) Change Verdict row value in final report summary table: `{APPROVE/REVISE/STALEMATE}` → `{CONSENSUS/CONTINUE/STALEMATE}` + + c) Update debate loop branching for CONSENSUS / CONTINUE / STALEMATE (same logic as plan-review). + +- [ ] **Step 4: Verify** + + ```bash + grep -n "APPROVE\|REVISE" \ + skill-packs/codex-review/skills/codex-impl-review/references/output-format.md \ + skill-packs/codex-review/skills/codex-impl-review/references/prompts.md \ + skill-packs/codex-review/skills/codex-impl-review/references/workflow.md + ``` + Expected: zero matches for old vocabulary in all 3 files. + +- [ ] **Step 5: Commit** + + ```bash + git add skill-packs/codex-review/skills/codex-impl-review/references/output-format.md + git add skill-packs/codex-review/skills/codex-impl-review/references/prompts.md + git add skill-packs/codex-review/skills/codex-impl-review/references/workflow.md + git commit -m "fix(impl-review): unify VERDICT vocabulary to CONSENSUS/CONTINUE/STALEMATE" + ``` + +--- + +## Task 12: Final verification pass + +**Files:** None modified. + +- [ ] **Step 1: Verify all 5 bugs are fixed** + + ```bash + # Bug 1: No AskUserQuestion in security-review + grep -rn "AskUserQuestion" skill-packs/codex-review/skills/ + + # Bug 2: No nested backtick fences + OUTPUT_FORMAT present in security prompts.md + grep -n "OUTPUT_FORMAT" skill-packs/codex-review/skills/codex-security-review/references/prompts.md + + # Bug 3: pr-review has $BASE_BRANCH not $SCOPE + grep -n '"scope"' skill-packs/codex-review/skills/codex-pr-review/references/workflow.md + + # Bug 4: No subagent_type in parallel-review + grep -rn "subagent_type" skill-packs/codex-review/skills/codex-parallel-review/ + + # Bug 5: No docs/security-reviews in security-review, no "Apply the suggested fix" + grep -rn "docs/security-reviews\|Apply the suggested fix" skill-packs/codex-review/skills/codex-security-review/ + ``` + + Expected: all searches return zero matches. + +- [ ] **Step 2: Verify information barriers exist in 3 skills** + + ```bash + grep -rn "Information Barrier" skill-packs/codex-review/skills/ + ``` + Expected: matches in `codex-security-review`, `codex-plan-review`, `codex-impl-review` — NOT in `codex-pr-review` (already had it), commit-review (already had it). + +- [ ] **Step 3: Verify VERDICT vocabulary is unified** + + ```bash + # Old vocabulary should NOT appear in the 3 updated skills + grep -rn "VERDICT.*APPROVE\|VERDICT.*REVISE\|Status: APPROVE\|Status: REVISE" \ + skill-packs/codex-review/skills/codex-plan-review/ \ + skill-packs/codex-review/skills/codex-impl-review/ \ + skill-packs/codex-review/skills/codex-security-review/ + + # New vocabulary SHOULD appear + grep -rn "CONSENSUS\|CONTINUE\|STALEMATE" \ + skill-packs/codex-review/skills/codex-plan-review/ \ + skill-packs/codex-review/skills/codex-impl-review/ \ + skill-packs/codex-review/skills/codex-security-review/ + ``` + + Expected: first grep has zero matches; second grep has multiple matches. + +- [ ] **Step 4: Verify unmodified skills are untouched** + + ```bash + # Verify commit-review still uses its original vocabulary (unchanged) + grep -n "APPROVE\|REVISE\|CONSENSUS\|CONTINUE\|STALEMATE\|Information Barrier" \ + skill-packs/codex-review/skills/codex-commit-review/references/workflow.md | head -5 + + # Verify think-about and codebase-review have no VERDICT/APPROVE vocabulary changes + grep -rn "CONSENSUS\|STALEMATE" \ + skill-packs/codex-review/skills/codex-think-about/ \ + skill-packs/codex-review/skills/codex-codebase-review/ + ``` + Expected for think-about/codebase-review: zero matches for new vocabulary (confirming they were not modified). + +- [ ] **Step 5: Verify clean working tree** + + ```bash + git status --short + ``` + Expected: empty output or only untracked/ignored files (all task changes committed). diff --git a/docs/superpowers/specs/2026-03-22-approach-a-skill-improvements-design.md b/docs/superpowers/specs/2026-03-22-approach-a-skill-improvements-design.md new file mode 100644 index 0000000..12e4deb --- /dev/null +++ b/docs/superpowers/specs/2026-03-22-approach-a-skill-improvements-design.md @@ -0,0 +1,312 @@ +# Design: Approach A — Skill Implementation Improvements + +**Date**: 2026-03-22 +**Scope**: codex-review skill pack (`skill-packs/codex-review/skills/`) +**Status**: Approved for implementation (v2 — post spec review) + +--- + +## Overview + +Approach A addresses three categories of issues found during comprehensive analysis of the 8 codex-review skills: + +1. **Critical bug fixes** — 5 runtime-breaking issues that cause incorrect behavior when skills are invoked +2. **Information barrier adoption** — apply the independent-analysis-before-polling pattern to 3 skills that lack it +3. **VERDICT vocabulary unification** — migrate 3 skills from final-state `APPROVE|REVISE` to debate-state `CONSENSUS|CONTINUE|STALEMATE` + +**Total files changed**: 15 existing files +**New files**: 1 (`codex-security-review/references/claude-analysis-template.md`) + +--- + +## Section 1: Critical Bug Fixes + +### Bug 1 — `codex-security-review`: Phantom `AskUserQuestion` tool + +**File**: `skill-packs/codex-review/skills/codex-security-review/references/workflow.md` (Phase 1, Step 1) + +**Problem**: Workflow Phase 1 Step 1 instructs Claude to use `AskUserQuestion` which does not exist in Claude Code. The Smart Default Detection block at the top of the file already contains a correct announce line. + +**Fix**: Delete Phase 1 Step 1 entirely. The Smart Default Detection block at the top of the file already handles scope/effort detection and announces the result with the correct pattern: +``` +Announce: "Detected: scope=$SCOPE, effort=$EFFORT. Proceeding — reply to override." +``` +Do not add a second announce. Simply remove the Step 1 block that references `AskUserQuestion`. + +--- + +### Bug 2 — `codex-security-review`: Nested triple-backtick in prompts.md + +**Files**: `skill-packs/codex-review/skills/codex-security-review/references/prompts.md` AND `workflow.md` + +**Problem**: The `## Output Format` section and VERDICT block are embedded inline inside an outer triple-backtick fence, causing the inner fences to prematurely close the outer fence. This corrupts the prompt sent to Codex. Unlike other skills, security-review has no prompt assembly step and no `{OUTPUT_FORMAT}` placeholder mechanism. + +**Fix — two-part**: + +**Part A** (`prompts.md`): Remove the inline `## Output Format` and VERDICT Block sections from inside the fenced prompt block. Replace with `{OUTPUT_FORMAT}` placeholder at the end of the prompt body (before the closing fence). + +**Part B** (`workflow.md`): Add a prompt assembly step (before the `start` command) instructing Claude to build `$PROMPT` using multi-step placeholder replacement, matching the exact pattern used in `codex-impl-review` workflow.md Step 1.8. The pattern is: + +``` +1.8) Assemble prompt (multi-step replacement — DO NOT use a single sed pipeline): + a) Read references/prompts.md as the template base + b) Replace {SCOPE} with the detected scope value + c) Replace {EFFORT} with the detected effort value + d) Replace {WORKING_DIR} with the current working directory + e) Replace {SCOPE_SPECIFIC_INSTRUCTIONS} with the appropriate scope-specific block + f) Replace {OUTPUT_FORMAT} by reading references/output-format.md in full + (Use printf '%s' pattern to avoid sed special-character corruption) + g) Replace any remaining placeholders ({BASE_BRANCH} etc.) + Store as $PROMPT. + Verify $PROMPT contains no prematurely-closed triple-backtick fences before sending to start. +``` + +Note: Do NOT use a single `sed` subshell to inject `output-format.md` — the file content may contain `&`, `\`, or `/` characters that corrupt sed replacements. Use `printf '%s'` piping or equivalent safe substitution matching the impl-review Step 1.8 pattern. + +--- + +### Bug 3 — `codex-pr-review`: `$SCOPE` undefined in meta.json + +**File**: `skill-packs/codex-review/skills/codex-pr-review/references/workflow.md` (Session Finalization section) + +**Problem**: The heredoc writes `"scope": "$SCOPE"` but `$SCOPE` is never defined in the pr-review workflow. The correct variable is `$BASE_BRANCH`. + +**Fix**: Change `"scope": "$SCOPE"` → `"scope": "$BASE_BRANCH"`. + +--- + +### Bug 4 — `codex-parallel-review`: Invalid `subagent_type` + +**File**: `skill-packs/codex-review/skills/codex-parallel-review/references/workflow.md` + +**Problem**: Agent spawn JSON uses `"subagent_type": "code-reviewer"` which is not a valid parameter in the Claude Code Agent tool, causing agent dispatch to fail. + +**Fix**: +1. Remove the `subagent_type` field from all 4 agent spawn JSON blocks (lines ~60, 70, 82, 93). +2. Update the prose description paragraph (line ~53) that references `subagent_type: "code-reviewer"` — remove the reference or replace with correct description of default agent behavior. + +--- + +### Bug 5 — `codex-security-review`: Archive step + code editing instruction + +**File**: `skill-packs/codex-review/skills/codex-security-review/references/workflow.md` (Phase 3 Options + Phase 5 Step 3) + +**Problems**: +- Phase 5 Step 3 writes artifacts to `docs/security-reviews/` — violates v12 session model (all output stays in `.codex-review/sessions/`) +- Phase 3 Option A instructs Claude to "Apply the suggested fix" — the only skill that tells Claude to edit code during review, violating the review-only model shared by all other skills + +**Fix**: +- Remove Phase 5 Step 3 (archive step) entirely. Session dir is the sole output location. +- Remove Phase 3 Option A. Renumber: old Option B → new Option A (rebuttal), old Option C → new Option B (acknowledge with severity dispute). Remove the lettered "Option" framing and use flat list matching `codex-impl-review` pattern. +- Enumerate all `APPROVE`/`REVISE` occurrences in `workflow.md` that must be updated as part of Section 3 VERDICT migration (all in same file — fix together): + - Phase 3 stop condition: `VERDICT: APPROVE` → `VERDICT: CONSENSUS` + - Phase 4 Step 4 "Iterate Until Consensus" line: `VERDICT: APPROVE - All critical/high issues resolved` → `VERDICT: CONSENSUS - All critical/high issues resolved` + - Phase 5 Step 2 Final Verdict header: `{APPROVE | REVISE | STALEMATE}` → `{CONSENSUS | CONTINUE | STALEMATE}` + - CI/CD integration section pre-commit hook example: `grep -q "VERDICT: REVISE"` → `grep -q "VERDICT: CONTINUE"` + +--- + +## Section 2: Information Barrier Adoption + +**Pattern source**: `codex-pr-review` and `codex-commit-review` (gold standard) + +**Rationale**: Without an information barrier, Claude reads Codex's findings before forming its own analysis. This causes anchoring bias — Claude's "independent" analysis is actually influenced by Codex's framing, reducing genuine independent coverage. + +**Core pattern**: +1. After `start` Codex, Claude does NOT poll immediately +2. Claude independently analyzes the artifact in working memory and records `FINDING-{N}` findings +3. Only after independent analysis is complete does Claude poll Codex +4. Cross-analysis maps FINDING-{N} (Claude) against ISSUE-{N} (Codex) +5. The analysis is kept in Claude's working context (NOT written to a separate file) + +### `codex-security-review`: Add Step 2.5 + new `claude-analysis-template.md` + +**workflow.md change** — add after Phase 2 `start` command (new Phase 2, Step 2.5): +``` +2.5) INFORMATION BARRIER — Claude Independent Security Analysis + MUST complete before polling Codex output. + Codex is running in background (typically 90-180s) — use this time. + + Using references/claude-analysis-template.md: + - Read all files in scope directly (do NOT read $SESSION_DIR/review.md) + - Identify top attack surfaces + - Form independent FINDING-{N} list using OWASP categories + - Note high-confidence vs uncertain findings + + Keep analysis in working context. Do NOT write a file. + INFORMATION BARRIER ends after Round 1 poll completes. + From Round 2 onwards, the barrier no longer applies. +``` + +**workflow.md change** — update Phase 3 cross-analysis step to map FINDING-{N} (Claude) ↔ ISSUE-{N} (Codex) using the same classification table as pr-review (Genuine Agreement / Genuine Disagreement / Claude-only / Codex-only / Same Direction Different Severity). + +**New file**: `references/claude-analysis-template.md` +- FINDING-{N} format mirroring `output-format.md` with OWASP/CWE fields +- `Strongest Positions` section +- Cross-analysis Matching Protocol (maps FINDING-{N} ↔ ISSUE-{N}) + +### `codex-plan-review`: Add Step 2.5 + transform Step 4 + +**workflow.md change** — add after Step 2 (`start`) as new Step 2.5: +``` +2.5) INFORMATION BARRIER — Claude Independent Plan Analysis + MUST complete before polling Codex output. + Codex is running in background — use this time. + + Read plan file at $PLAN_PATH directly. + Do NOT read $SESSION_DIR/review.md. + Form independent FINDING-{N} list in working context: + - Correctness issues + - Architecture concerns + - Sequencing/dependency problems + - Scope gaps or risks + + INFORMATION BARRIER ends after Round 1 poll completes. + From Round 2 onwards, the barrier no longer applies. +``` + +**workflow.md change** — transform Step 4 "Parse Review" into a Cross-Analysis step. Step 4 currently parses Codex output and applies fixes directly. Add a cross-analysis sub-step before applying fixes: + +``` +4) Cross-Analysis (after first poll completes): + a) Parse all ISSUE-{N} blocks from review.md + b) Build mapping table: FINDING-{N} (Claude) ↔ ISSUE-{N} (Codex) + - Genuine Agreement: FINDING-{N} and ISSUE-{M} identify same problem + - Codex-only: ISSUE-{M} has no matching FINDING + - Claude-only: FINDING-{N} has no matching ISSUE + - Genuine Disagreement: conflicting assessments of same area + c) For each ISSUE-{N}: determine response (fix, rebuttal, or acknowledge) + d) Apply fixes to the plan file. Save the updated plan file before resuming. + (Codex Round 2+ re-reads the plan from $PLAN_PATH — unsaved changes are invisible.) + e) Proceed to resume loop +``` + +No new file needed. + +### `codex-impl-review`: Add Step 2.5 + extend Step 4 + +**workflow.md change** — add after Step 2 (`start`) as new Step 2.5 (mode-conditional): +``` +2.5) INFORMATION BARRIER — Claude Independent Code Analysis + MUST complete before polling Codex output. + Codex is running in background — use this time. + + working-tree mode: run `git diff` and `git diff --cached` yourself + (or reuse the diff already collected in Step 1 — working tree hasn't changed) + branch mode: run `git diff $BASE_BRANCH...HEAD` yourself + (or reuse the diff already collected in Step 1) + + Form independent FINDING-{N} list in working context: + - Bugs and edge cases + - Security issues + - Performance concerns + - Maintainability problems + + Do NOT read $SESSION_DIR/review.md until analysis complete. + INFORMATION BARRIER ends after Round 1 poll completes. +``` + +**workflow.md change** — extend Step 4 "Apply/Rebut" to add cross-analysis sub-step before applying fixes. impl-review retains code-editing behavior — the cross-analysis adds FINDING/ISSUE mapping context to the existing apply/rebuttal decision, but does NOT remove code editing: + +``` +4) Cross-Analysis + Apply/Rebut: + a) Parse all ISSUE-{N} blocks from review.md + b) Build mapping table: FINDING-{N} (Claude) ↔ ISSUE-{N} (Codex) + (same classification as plan-review Step 4a-b above) + c) For each ISSUE-{N}: + - If genuine agreement or Codex-only: apply fix to code + - If Claude-only: include in final report as Claude finding + - If genuine disagreement: rebuttal response + d) Commit fixes (branch mode only — required before resume) + e) Proceed to resume loop +``` + +Code editing is preserved. FINDING-{N} from Step 2.5 enriches the cross-analysis but does not change the apply/rebuttal outcome model. + +--- + +## Section 3: VERDICT Vocabulary Unification + +**Problem**: Three skills (`plan-review`, `impl-review`, `security-review`) use `APPROVE | REVISE` — a final-state verdict that cannot drive a multi-round debate loop. The correct vocabulary for debate-loop skills is `CONSENSUS | CONTINUE | STALEMATE`. + +**Note on pre-existing state**: `security-review/references/output-format.md` and `prompts.md` already contain partial `STALEMATE` vocabulary in secondary sections (stalemate format section). The migration unifies these existing fragments into the primary VERDICT block. + +**Target vocabulary** (from pr-review/commit-review gold standard): + +| Value | Meaning | Workflow action | +|-------|---------|----------------| +| `CONSENSUS` | No remaining issues / all resolved | End debate, generate final report | +| `CONTINUE` | Issues remain, requires another round | Claude fixes/rebuts, then `resume` | +| `STALEMATE` | Circular debate detected by Codex | End debate, produce partial report | + +**Migration mapping**: +- `APPROVE` → `CONSENSUS` +- `REVISE` → `CONTINUE` +- *(pre-existing in stalemate sections)* `STALEMATE` → promote to primary VERDICT block + +### Changes per skill + +**For each of `plan-review`, `impl-review`, `security-review`:** + +**`references/output-format.md`** (clear VERDICT change): +- Update VERDICT block primary values from `APPROVE | REVISE` to `CONSENSUS | CONTINUE | STALEMATE` +- Add STALEMATE description in VERDICT block + +**`references/prompts.md`** (scope clarification): +- For plan-review and impl-review: `{OUTPUT_FORMAT}` placeholder indirection means VERDICT values live in `output-format.md` only; in `prompts.md` update only the instruction line "End with a VERDICT block" to add: "Use STALEMATE if same arguments repeat for 2+ consecutive rounds with no new evidence." +- For security-review: remove inline VERDICT values from nested backtick sections (covered by Bug 2 fix); the `{OUTPUT_FORMAT}` injection handles the values after Bug 2 is fixed. + +**`references/workflow.md`** (all occurrences): +- Update stop condition text: replace `VERDICT: APPROVE` → `VERDICT: CONSENSUS` +- Update debate loop branching: `CONTINUE` triggers resume (same as `REVISE`), `CONSENSUS` triggers final report (same as `APPROVE`), `STALEMATE` triggers stalemate handling +- Update stalemate detection: check for `STALEMATE` verdict OR same open ISSUE set for 2 consecutive rounds (the latter is the existing heuristic) +- Update the Verdict row value in the final report summary table from `{APPROVE/REVISE/STALEMATE}` to `{CONSENSUS/CONTINUE/STALEMATE}` (this is a value cell, not a table header) +- For security-review specifically: update inline Final Security Report template in Phase 5 Step 2 — the `## Final Verdict: {APPROVE | REVISE | STALEMATE}` header → `## Final Verdict: {CONSENSUS | CONTINUE | STALEMATE}`. All APPROVE/REVISE occurrences are enumerated in Bug 5 fix above. + +**`references/output-format.md`** — security-review note: the `Status` field in the Verdict Block (`Status: {complete | stalemate | in-progress}`) is a separate field from `VERDICT` and is retained as-is. `Status: stalemate` maps to `VERDICT: STALEMATE`; `Status: in-progress` maps to `VERDICT: CONTINUE`. No change needed to the `Status` field. + +--- + +## File Change Summary + +| File | Change type | Section | +|------|-------------|---------| +| `codex-security-review/references/workflow.md` | Delete Step 1, add prompt assembly step, add Step 2.5, extend cross-analysis, remove archive step, remove Option A, renumber options, update VERDICT logic + final report template | 1 (bugs 1, 2, 5), 2, 3 | +| `codex-security-review/references/prompts.md` | Remove inline output format (replace with `{OUTPUT_FORMAT}`), update STALEMATE instruction | 1 (bug 2), 3 | +| `codex-security-review/references/output-format.md` | Update VERDICT block to `CONSENSUS\|CONTINUE\|STALEMATE` | 3 | +| `codex-security-review/references/claude-analysis-template.md` | **NEW FILE** | 2 | +| `codex-pr-review/references/workflow.md` | Fix `$SCOPE` → `$BASE_BRANCH` | 1 (bug 3) | +| `codex-parallel-review/references/workflow.md` | Remove `subagent_type` from JSON blocks + prose | 1 (bug 4) | +| `codex-plan-review/references/workflow.md` | Add Step 2.5, transform Step 4 to cross-analysis, update VERDICT logic + stop conditions + final report table | 2, 3 | +| `codex-plan-review/references/prompts.md` | Update STALEMATE instruction line only | 3 | +| `codex-plan-review/references/output-format.md` | Update VERDICT block values | 3 | +| `codex-impl-review/references/workflow.md` | Add Step 2.5, extend Step 4 with cross-analysis sub-steps, update VERDICT logic + stop conditions + final report table | 2, 3 | +| `codex-impl-review/references/prompts.md` | Update STALEMATE instruction line in 4 templates | 3 | +| `codex-impl-review/references/output-format.md` | Update VERDICT block values | 3 | + +*(Note: `codex-security-review/references/output-format.md` appears once — duplicate removed from v1.)* + +--- + +## Constraints + +- **Never edit user's code or create commits during review** — review-only model preserved (security-review Phase 3 Option A removal enforces this) +- **impl-review retains code editing** — cross-analysis enriches the apply/rebuttal decision but does not change the code-editing model +- **No new skills added** — only existing skills improved +- **SKILL.md files** — not changed (these are user-facing; workflow changes are in references/) +- **codex-runner.js** — not changed (runner is separate from skill instructions) +- **`bin/codex-skill.js`** — not changed + +--- + +## Success Criteria + +1. All 5 critical bugs fixed: + - Bug 1: security-review workflow invoked without `AskUserQuestion` error + - Bug 2: prompt sent to Codex contains no prematurely-closed triple-backtick fences (verify by inspecting assembled `$PROMPT` variable) + - Bug 3: pr-review meta.json contains `"scope": ""` not `"scope": "$SCOPE"` + - Bug 4: parallel-review agent spawn JSON contains no `subagent_type` field + - Bug 5: security-review workflow does not write to `docs/` and does not instruct Claude to edit code +2. Information barrier active in security/plan/impl-review — Claude FINDING-{N} analysis recorded in working context before first poll result is read +3. VERDICT parsing unified — `CONSENSUS|CONTINUE|STALEMATE` parsed correctly in all 3 updated skills' workflow debate loops and stop conditions +4. No regressions in the 5 unmodified skills (pr-review, commit-review, think-about, parallel-review, codebase-review) diff --git a/skill-packs/codex-review/skills/codex-impl-review/SKILL.md b/skill-packs/codex-review/skills/codex-impl-review/SKILL.md index 9be7c4a..aca6b13 100644 --- a/skill-packs/codex-review/skills/codex-impl-review/SKILL.md +++ b/skill-packs/codex-review/skills/codex-impl-review/SKILL.md @@ -105,6 +105,24 @@ printf '%s' "$PROMPT" | node "$RUNNER" start "$SESSION_DIR" --effort "$EFFORT" ``` Validate JSON: `{"status":"started","round":1}`. Error with `CODEX_NOT_FOUND` → tell user to install codex. +### 5.5. Information Barrier — Claude Independent Code Analysis + +**MUST complete before polling Codex output.** Codex is running in background — use this time productively. + +**Working-tree mode**: run `git diff` and `git diff --cached` yourself (or reuse the diff already collected in Step 1 — working tree hasn't changed). + +**Branch mode**: run `git diff $BASE_BRANCH...HEAD` yourself (or reuse the diff already collected in Step 1). + +Form an independent FINDING-{N} list in working context (do NOT write to a file): +- Bugs and edge cases +- Security issues +- Performance concerns +- Maintainability problems + +Use the same FINDING-{N} format as `references/output-format.md` ISSUE-{N} (same field names). Do NOT read `$SESSION_DIR/review.md` until this analysis is complete. + +**INFORMATION BARRIER ends after Round 1 poll completes.** From Round 2 onwards, the barrier no longer applies. + ### 6. Poll ```bash POLL_JSON=$(node "$RUNNER" poll "$SESSION_DIR") @@ -116,8 +134,32 @@ Report **specific activities** from `activities` array (e.g. "Codex [45s]: readi Continue while `status === "running"`. Stop on `completed|failed|timeout|stalled`. ### 7. Apply/Rebut + +**After Round 1 poll completes and `$SESSION_DIR/review.md` is available:** + +#### 7a. Parse Codex Output Parse issues from `poll_json.review.blocks[]` — each has `id`, `title`, `severity`, `category`, `location`, `problem`, `evidence`, `suggested_fix`. Verdict in `review.verdict.status`. Fallback: `review.raw_markdown`. +#### 7b. Build FINDING↔ISSUE Mapping Table +Map Claude's FINDING-{N} (from Step 5.5) against Codex's ISSUE-{M}: + +| Claude FINDING-{N} | Codex ISSUE-{M} | Classification | +|--------------------|-----------------|----------------| +| ... | ... | ... | + +Classification options: +- **Genuine Agreement**: FINDING-{N} and ISSUE-{M} identify the same code problem +- **Codex-only**: ISSUE-{M} has no matching Claude FINDING +- **Claude-only**: FINDING-{N} has no matching Codex ISSUE +- **Genuine Disagreement**: Conflicting assessments of the same code + +#### 7c. Apply/Rebut Using Cross-Analysis Context +For each ISSUE-{N}: +- Genuine agreement or Codex-only → apply fix to code +- Claude-only → include in final report as Claude finding +- Genuine disagreement → write rebuttal with concrete proof (paths, tests, behavior) + +#### 7d. Apply Fixes - **Valid issues**: edit code, record fix evidence. - **Invalid issues**: rebut with concrete proof (paths, tests, behavior). - **Branch mode only**: commit fixes (`git add` + `git commit`) before resuming — Codex reads `git diff ...HEAD` which only shows committed changes. @@ -153,7 +195,7 @@ Resume: `printf '%s' "$PROMPT" | node "$RUNNER" resume "$SESSION_DIR" --effort " | Metric | Value | |--------|-------| | Rounds | {N} | -| Verdict | {APPROVE/REVISE/STALEMATE} | +| Verdict | {CONSENSUS/CONTINUE/STALEMATE} | | Issues Found | {total} | | Issues Fixed | {fixed_count} | | Issues Disputed | {disputed_count} | diff --git a/skill-packs/codex-review/skills/codex-impl-review/references/output-format.md b/skill-packs/codex-review/skills/codex-impl-review/references/output-format.md index f94a746..2a6c4a4 100644 --- a/skill-packs/codex-review/skills/codex-impl-review/references/output-format.md +++ b/skill-packs/codex-review/skills/codex-impl-review/references/output-format.md @@ -17,8 +17,11 @@ Use this exact shape (copy the entire block below as `{OUTPUT_FORMAT}`): - Suggested fix: {concrete code change} ### VERDICT -- Status: APPROVE | REVISE +- Status: CONSENSUS | CONTINUE | STALEMATE + - CONSENSUS: No remaining code issues — changes are correct and safe + - CONTINUE: Issues remain that require fixes and another review round + - STALEMATE: Circular debate — same disputes for 2+ rounds with no progress - Reason: {short reason} ``` -**Zero-issue rule**: If no issues remain, omit all ISSUE blocks and return only the VERDICT block with `Status: APPROVE` and `Reason: All changes are correct, well-tested, and safe to merge.` +**Zero-issue rule**: If no issues remain, omit all ISSUE blocks and return only the VERDICT block with `Status: CONSENSUS` and `Reason: All changes are correct, well-tested, and safe to merge.` diff --git a/skill-packs/codex-review/skills/codex-impl-review/references/prompts.md b/skill-packs/codex-review/skills/codex-impl-review/references/prompts.md index a62a2c3..02919b3 100644 --- a/skill-packs/codex-review/skills/codex-impl-review/references/prompts.md +++ b/skill-packs/codex-review/skills/codex-impl-review/references/prompts.md @@ -55,7 +55,8 @@ You are Codex acting as a strict code reviewer. 3. Output each finding as ISSUE-{N} using the EXACT format below. 4. Keep ISSUE-{N} IDs stable — do not renumber in later rounds. 5. End with a VERDICT block. Do not skip it. -6. Use required output format exactly. +6. Use `STALEMATE` if the same arguments repeat for 2+ consecutive rounds with no new evidence. +7. Use required output format exactly. ## Required Output Format {OUTPUT_FORMAT} @@ -86,7 +87,8 @@ You are Codex acting as a strict code reviewer. 3. Output each finding as ISSUE-{N} using the EXACT format below. 4. Keep ISSUE-{N} IDs stable — do not renumber in later rounds. 5. End with a VERDICT block. Do not skip it. -6. Use required output format exactly. +6. Use `STALEMATE` if the same arguments repeat for 2+ consecutive rounds with no new evidence. +7. Use required output format exactly. ## Required Output Format {OUTPUT_FORMAT} @@ -113,6 +115,7 @@ You are Codex acting as a strict code reviewer. 6. Maintain the same ISSUE-{N} numbering. New findings use the next available number. 7. Keep already-fixed issues closed. 8. End with a VERDICT block. +9. Use `STALEMATE` if the same arguments repeat for 2+ consecutive rounds with no new evidence. ## Required Output Format {OUTPUT_FORMAT} @@ -144,6 +147,7 @@ You are Codex acting as a strict code reviewer. 6. Maintain the same ISSUE-{N} numbering. New findings use the next available number. 7. Keep already-fixed issues closed. 8. End with a VERDICT block. +9. Use `STALEMATE` if the same arguments repeat for 2+ consecutive rounds with no new evidence. ## Required Output Format {OUTPUT_FORMAT} diff --git a/skill-packs/codex-review/skills/codex-plan-review/SKILL.md b/skill-packs/codex-review/skills/codex-plan-review/SKILL.md index 7476334..881bda0 100644 --- a/skill-packs/codex-review/skills/codex-plan-review/SKILL.md +++ b/skill-packs/codex-review/skills/codex-plan-review/SKILL.md @@ -79,6 +79,22 @@ printf '%s' "$PROMPT" | node "$RUNNER" start "$SESSION_DIR" --effort "$EFFORT" ``` Validate JSON: `{"status":"started","round":1}`. Error with `CODEX_NOT_FOUND` → tell user to install codex. +### 5.5. Information Barrier — Claude Independent Plan Analysis + +**MUST complete before polling Codex output.** Codex is running in background — use this time productively. + +Read the plan file at `$PLAN_PATH` directly. Do NOT read `$SESSION_DIR/review.md` until this analysis is complete. + +Form an independent FINDING-{N} list in working context (do NOT write to a file): +- Correctness issues (steps that are wrong or will fail) +- Architecture concerns (structural problems with the approach) +- Sequencing/dependency problems (steps out of order, missing prerequisites) +- Scope gaps or risks (missing requirements, underestimated complexity) + +Use the same FINDING-{N} format as `references/output-format.md` ISSUE-{N} (same field names). + +**INFORMATION BARRIER ends after Round 1 poll completes.** From Round 2 onwards, the barrier no longer applies. + ### 6. Poll ```bash POLL_JSON=$(node "$RUNNER" poll "$SESSION_DIR") @@ -90,8 +106,32 @@ Report **specific activities** from `activities` array (e.g. "Codex [45s]: readi Continue while `status === "running"`. Stop on `completed|failed|timeout|stalled`. ### 7. Apply/Rebut + +**After Round 1 poll completes and `$SESSION_DIR/review.md` is available:** + +#### 7a. Parse Codex Output Parse issues from `poll_json.review.blocks[]` — each has `id`, `title`, `severity`, `category`, `location`, `problem`, `evidence`, `suggested_fix`. Verdict in `review.verdict.status`. Fallback: `review.raw_markdown`. +#### 7b. Build FINDING↔ISSUE Mapping Table +Map Claude's FINDING-{N} (from Step 5.5) against Codex's ISSUE-{M}: + +| Claude FINDING-{N} | Codex ISSUE-{M} | Classification | +|--------------------|-----------------|----------------| +| ... | ... | ... | + +Classification options: +- **Genuine Agreement**: FINDING-{N} and ISSUE-{M} identify the same plan problem +- **Codex-only**: ISSUE-{M} has no matching Claude FINDING +- **Claude-only**: FINDING-{N} has no matching Codex ISSUE +- **Genuine Disagreement**: Conflicting assessments of the same plan section + +#### 7c. Determine Response for Each ISSUE +For each ISSUE-{N}: +- Genuine agreement or Codex-only → apply fix to the plan file +- Claude-only → include in final report as Claude finding +- Genuine disagreement → write rebuttal with concrete reasoning + +#### 7d. Apply Fixes - **Valid issues**: apply fixes directly to the plan file, **save the plan file** before resuming — Codex re-reads from the plan path. - **Invalid issues**: rebut with concrete proof (reasoning, references, behavior). @@ -115,7 +155,7 @@ Resume: `printf '%s' "$PROMPT" | node "$RUNNER" resume "$SESSION_DIR" --effort " | Metric | Value | |--------|-------| | Rounds | {N} | -| Verdict | {APPROVE/REVISE/STALEMATE} | +| Verdict | {CONSENSUS/CONTINUE/STALEMATE} | | Issues Found | {total} | | Issues Fixed | {fixed_count} | | Issues Disputed | {disputed_count} | diff --git a/skill-packs/codex-review/skills/codex-plan-review/references/output-format.md b/skill-packs/codex-review/skills/codex-plan-review/references/output-format.md index 5dd5c9d..6dbee57 100644 --- a/skill-packs/codex-review/skills/codex-plan-review/references/output-format.md +++ b/skill-packs/codex-review/skills/codex-plan-review/references/output-format.md @@ -17,8 +17,11 @@ Use this exact shape (copy the entire block below as `{OUTPUT_FORMAT}`): - Suggested fix: {plan-level change} ### VERDICT -- Status: APPROVE | REVISE +- Status: CONSENSUS | CONTINUE | STALEMATE + - CONSENSUS: No remaining plan issues — ready to implement + - CONTINUE: Issues remain that require another review round + - STALEMATE: Circular debate — same disputes for 2+ rounds with no progress - Reason: {short reason} ``` -**Zero-issue rule**: If no issues remain, omit all ISSUE blocks and return only the VERDICT block with `Status: APPROVE` and `Reason: Plan is complete, well-structured, and addresses all acceptance criteria.` +**Zero-issue rule**: If no issues remain, omit all ISSUE blocks and return only the VERDICT block with `Status: CONSENSUS` and `Reason: Plan is complete, well-structured, and addresses all acceptance criteria.` diff --git a/skill-packs/codex-review/skills/codex-plan-review/references/prompts.md b/skill-packs/codex-review/skills/codex-plan-review/references/prompts.md index 3261b1f..41fb28e 100644 --- a/skill-packs/codex-review/skills/codex-plan-review/references/prompts.md +++ b/skill-packs/codex-review/skills/codex-plan-review/references/prompts.md @@ -51,7 +51,8 @@ Read the plan file directly at: {PLAN_PATH} 4. Do NOT propose code changes — review only the plan quality. 5. Output each finding as ISSUE-{N} using the EXACT format below. 6. End with a VERDICT block. Do not skip it. -7. Keep ISSUE-{N} IDs stable — do not renumber in later rounds. +7. Use `STALEMATE` if the same arguments repeat for 2+ consecutive rounds with no new evidence. +8. Keep ISSUE-{N} IDs stable — do not renumber in later rounds. ## Required Output Format {OUTPUT_FORMAT} @@ -81,6 +82,7 @@ Read the updated plan file directly at: {PLAN_PATH} 5. Focus on remaining open issues and any NEW findings from the updated plan. 6. Maintain the same ISSUE-{N} numbering. New findings use the next available number. 7. End with a VERDICT block. +8. Use `STALEMATE` if the same arguments repeat for 2+ consecutive rounds with no new evidence. ## Required Output Format {OUTPUT_FORMAT} diff --git a/skill-packs/codex-review/skills/codex-security-review/SKILL.md b/skill-packs/codex-review/skills/codex-security-review/SKILL.md index 39f0c14..7381478 100644 --- a/skill-packs/codex-review/skills/codex-security-review/SKILL.md +++ b/skill-packs/codex-review/skills/codex-security-review/SKILL.md @@ -127,12 +127,57 @@ Report **specific activities** from `activities` array (e.g. "Codex [45s]: scann Continue while `status === "running"`. Stop on `completed|failed|timeout|stalled`. +### 6.5. Information Barrier — Claude Independent Security Analysis + +**MUST complete before polling Codex output.** Codex is running in background — use this time productively. + +**Working-tree mode**: run `git diff` and `git diff --cached` yourself (or reuse the diff already collected in Step 1). + +**Branch mode**: run `git diff $BASE_BRANCH...HEAD` yourself (or reuse the diff already collected in Step 1). + +**Full mode**: identify high-risk areas (auth, database, external APIs, file operations, crypto) and read those files. + +Form an independent FINDING-{N} list in working context (do NOT write to a file) using OWASP Top 10 2021 and CWE patterns: +- Injection vulnerabilities (SQL, command, XSS) +- Authentication/authorization issues +- Sensitive data exposure +- Security misconfigurations +- Cryptographic failures + +Use the same FINDING-{N} format as `references/output-format.md` ISSUE-{N} (same field names, including CWE/OWASP mappings). Do NOT read `$SESSION_DIR/review.md` until this analysis is complete. + +**INFORMATION BARRIER ends after Round 1 poll completes.** From Round 2 onwards, the barrier no longer applies. + ### 7. Apply/Rebut + +**After Round 1 poll completes and `$SESSION_DIR/review.md` is available:** + +#### 7a. Parse Codex Output Parse issues from `poll_json.review.blocks[]` — each has `id`, `title`, `severity`, `category`, `confidence`, `cwe`, `owasp`, `problem`, `evidence`, `attack_vector`, `suggested_fix`. Verdict in `review.verdict.status`. Risk summary in `review.verdict.risk_summary` (`{ critical, high, medium, low }`). Fallback: `review.raw_markdown`. Present findings grouped by severity (Critical → High → Medium → Low). Format: `ISSUE-{N}: {title} [{cwe}] [{owasp}] — confidence: {confidence}`. Critical/High = blocking; Medium/Low = advisory. -- **Valid issues**: fix vulnerabilities in code, record fix evidence. +#### 7b. Build FINDING↔ISSUE Mapping Table +Map Claude's FINDING-{N} (from Step 6.5) against Codex's ISSUE-{M}: + +| Claude FINDING-{N} | Codex ISSUE-{M} | Classification | +|--------------------|-----------------|----------------| +| ... | ... | ... | + +Classification options: +- **Genuine Agreement**: FINDING-{N} and ISSUE-{M} identify the same security vulnerability +- **Codex-only**: ISSUE-{M} has no matching Claude FINDING +- **Claude-only**: FINDING-{N} has no matching Codex ISSUE +- **Genuine Disagreement**: Conflicting security assessments of the same code + +#### 7c. Apply/Rebut Using Cross-Analysis Context +For each ISSUE-{N}: +- Genuine agreement or Codex-only → validate and prepare fix evidence +- Claude-only → include in final report as Claude finding +- Genuine disagreement → rebut with concrete proof (paths, tests, mitigating controls) + +#### 7d. Apply Fixes +- **Valid issues**: validate findings, prepare rebuttals or severity adjustments, and provide evidence without editing code. - **False positives**: rebut with concrete proof (paths, tests, mitigating controls). - **Severity disputes**: acknowledge issue, explain why severity should differ with context. - **Branch mode only**: commit fixes (`git add` + `git commit`) before resuming — Codex reads `git diff ...HEAD` which only shows committed changes. @@ -158,7 +203,7 @@ Resume: `printf '%s' "$PROMPT" | node "$RUNNER" resume "$SESSION_DIR" --effort " | Metric | Value | |--------|-------| | Rounds | {N} | -| Verdict | {APPROVE/REVISE/STALEMATE} | +| Verdict | {CONSENSUS/CONTINUE/STALEMATE} | | Risk Level | {CRITICAL/HIGH/MEDIUM/LOW} | | Issues Found | {total} | | Issues Fixed | {fixed_count} | diff --git a/skill-packs/codex-review/skills/codex-security-review/references/claude-analysis-template.md b/skill-packs/codex-review/skills/codex-security-review/references/claude-analysis-template.md new file mode 100644 index 0000000..c04aeb3 --- /dev/null +++ b/skill-packs/codex-review/skills/codex-security-review/references/claude-analysis-template.md @@ -0,0 +1,54 @@ +# Claude Independent Security Analysis Template + +> Use this template for Step 2.5 (Information Barrier). +> Record analysis in working context ONLY — do NOT write to a file. +> Do NOT read `$SESSION_DIR/review.md` until this analysis is complete. + +## FINDING-{N} Format + +Use this exact shape for each independent finding: + +### FINDING-{N}: {Short title} +- Category: injection | broken-auth | sensitive-data | xxe | broken-access | security-config | xss | insecure-deserialization | logging | ssrf | crypto-failure | insecure-design | vulnerable-components | integrity-failure +- Severity: low | medium | high | critical +- Confidence: low | medium | high +- CWE: {CWE-NNN if known, otherwise omit} +- OWASP: {A0N:2021 category if applicable, otherwise omit} +- Location: {file path:line range} +- Problem: {clear statement of the vulnerability or security weakness} +- Evidence: {specific code pattern, snippet, or observation} +- Attack Vector: {how an attacker could exploit this} +- Why it matters: {impact — data exposure, privilege escalation, etc.} + +## Overall Security Assessment + +- Attack surface: {high / medium / low — based on exposed endpoints, data handled} +- Most critical area: {the single highest-risk component or pattern found} +- Confidence in analysis: {high / medium / low — limited by static analysis} + +## Strongest Positions + +List the 3-5 findings Claude is most confident about for cross-analysis debate: + +1. FINDING-{N}: {title} — {one-sentence rationale for high confidence} +2. ... + +## Cross-Analysis Matching Protocol + +After Round 1 poll completes, build a mapping table: + +| Claude FINDING-{N} | Codex ISSUE-{M} | Classification | +|--------------------|-----------------|----------------| +| FINDING-1 | ISSUE-2 | Genuine Agreement | +| FINDING-2 | — | Claude-only | +| — | ISSUE-4 | Codex-only | +| FINDING-3 | ISSUE-5 | Genuine Disagreement | + +Classification rules: +- **Genuine Agreement**: Same vulnerability class + same file/line area +- **Genuine Disagreement**: Same code area but conflicting assessment (one says vulnerable, other says safe) +- **Same Direction / Different Severity**: Both flag the same issue but assign different severity +- **Claude-only**: Claude's finding has no Codex counterpart +- **Codex-only**: Codex's finding has no Claude counterpart + +Maintain this table across all rounds. Update Classification column as rounds progress. diff --git a/skill-packs/codex-review/skills/codex-security-review/references/output-format.md b/skill-packs/codex-review/skills/codex-security-review/references/output-format.md index 62fd4ae..6d8168f 100644 --- a/skill-packs/codex-review/skills/codex-security-review/references/output-format.md +++ b/skill-packs/codex-review/skills/codex-security-review/references/output-format.md @@ -223,10 +223,15 @@ Full CWE list: https://cwe.mitre.org/ ## Verdict Block ``` -VERDICT: APPROVE | REVISE +VERDICT: CONSENSUS | CONTINUE | STALEMATE Status: {complete | stalemate | in-progress} Reason: {explanation} +Verdict values: +- CONSENSUS: All critical/high security issues resolved or agreed-upon — ready to proceed +- CONTINUE: Security issues remain that require another review round +- STALEMATE: Circular debate — same disputes for 2+ rounds with no progress + Security Risk Summary: - Critical: {count} issues - High: {count} issues @@ -259,7 +264,6 @@ Advisory Issues (should fix, not blocking): ## Example: Complete Security Finding -``` ISSUE-1: SQL Injection in user search endpoint Category: injection Severity: critical @@ -317,13 +321,11 @@ app.get('/api/users/search', async (req, res) => { 2. Implement rate limiting on search endpoint 3. Use prepared statements for all database queries 4. Enable SQL query logging for security monitoring -``` --- ## Example: Secrets Detection -``` ISSUE-2: Hardcoded AWS credentials in configuration Category: sensitive-data Severity: critical @@ -379,13 +381,11 @@ if (!AWS_CONFIG.accessKeyId || !AWS_CONFIG.secretAccessKey) { 2. Use AWS Secrets Manager for production credentials 3. Implement pre-commit hooks to detect secrets (e.g., git-secrets, truffleHog) 4. Enable AWS GuardDuty for threat detection -``` --- ## Response Format (Round 2+) -``` RESPONSE-{N}: Re: ISSUE-{N} Action: accept | reject | revise Reason: [Response to rebuttal] @@ -399,7 +399,6 @@ Updated assessment: - Severity: {new_severity} (was {old_severity}) - Confidence: {new_confidence} (was {old_confidence}) - Reason: [Why the assessment changed] -``` --- diff --git a/skill-packs/codex-review/skills/codex-security-review/references/prompts.md b/skill-packs/codex-review/skills/codex-security-review/references/prompts.md index 82973a7..a03bfe9 100644 --- a/skill-packs/codex-review/skills/codex-security-review/references/prompts.md +++ b/skill-packs/codex-review/skills/codex-security-review/references/prompts.md @@ -2,7 +2,6 @@ ## Security Review Prompt (Round 1) -``` You are a security expert conducting a thorough security review of code changes. ## Context @@ -101,58 +100,6 @@ Perform a comprehensive security analysis focusing on OWASP Top 10 2021 vulnerab - [ ] Insecure file uploads - [ ] Open redirects -## Output Format - -For each security finding, use this structure: - -``` -ISSUE-{N}: {vulnerability_title} -Category: injection | broken-auth | sensitive-data | xxe | broken-access | security-config | xss | insecure-deserialization | logging | ssrf | crypto-failure | insecure-design | vulnerable-components | integrity-failure -Severity: critical | high | medium | low -Confidence: high | medium | low -CWE: CWE-{ID} ({Name}) -OWASP: A{NN}:2021 - {Category Name} - -Problem: [Clear description of the vulnerability] - -Evidence: [Code snippet showing the vulnerable pattern] - -Attack Vector: [How an attacker could exploit this vulnerability] - -Suggested Fix: [Secure code example with explanation] -``` - -### Severity Guidelines -- **critical**: Remote code execution, authentication bypass, data breach -- **high**: Privilege escalation, SQL injection, XSS with session theft -- **medium**: Information disclosure, CSRF, weak crypto -- **low**: Security headers missing, verbose errors, minor misconfigurations - -### Confidence Guidelines -- **high**: Clear vulnerability pattern, well-known exploit -- **medium**: Potential vulnerability, depends on context -- **low**: Suspicious pattern, may be false positive - -## Verdict Block - -After listing all findings: - -``` -VERDICT: APPROVE | REVISE -Status: {status} -Reason: {explanation} - -Security Risk Summary: -- Critical: {count} -- High: {count} -- Medium: {count} -- Low: {count} - -Recommendations: -1. [Priority action items] -2. [Additional security measures] -``` - ## Important Notes 1. **Static Analysis Limitations**: You can only analyze code patterns. Mark findings with appropriate confidence levels. @@ -164,13 +111,16 @@ Recommendations: ## Review Scope {SCOPE_SPECIFIC_INSTRUCTIONS} -``` + +## Required Output Format +{OUTPUT_FORMAT} + +End with a VERDICT block. Do not skip it. Use `STALEMATE` if the same arguments repeat for 2+ consecutive rounds with no new evidence. --- ## Security Review Prompt - Working Tree Mode -``` ## Review Scope: Uncommitted Changes Analyze only the uncommitted changes in the working tree (staged and unstaged). @@ -182,13 +132,11 @@ Focus on: 4. Security controls removed or weakened Use `git diff` to see changes. Review both the changed lines and surrounding context. -``` --- ## Security Review Prompt - Branch Mode -``` ## Review Scope: Branch Diff Analyze all changes in the current branch compared to base branch: {BASE_BRANCH} @@ -200,13 +148,11 @@ Focus on: 4. Security controls added or removed Use `git diff {BASE_BRANCH}...HEAD` to see all changes. -``` --- ## Security Review Prompt - Full Codebase Mode -``` ## Review Scope: Full Codebase Analyze the entire codebase for security vulnerabilities. @@ -224,13 +170,11 @@ Prioritize high-severity findings. For large codebases, focus on: - External API calls - File operations - Cryptographic operations -``` --- ## Round 2+ Prompt (Resume) -``` You are continuing a security review debate. ## Previous Round Summary @@ -243,32 +187,15 @@ You are continuing a security review debate. 3. Identify any new security concerns introduced by fixes 4. Update your verdict -## Response Format - -For each previously disputed issue: -``` -RESPONSE-{N}: Re: ISSUE-{N} -Action: accept | reject | revise -Reason: [Your response to the rebuttal] -``` - -For new issues found in fixes: -``` -ISSUE-{N}: [New issue title] -[Standard issue format] -``` - -Updated verdict: -``` -VERDICT: APPROVE | REVISE -Reason: [Updated assessment] -``` - ## Stop Conditions - All critical and high severity issues are resolved - Remaining disputes are documented and acknowledged - No new security concerns in applied fixes -``` + +## Required Output Format +{OUTPUT_FORMAT} + +End with a VERDICT block. Do not skip it. Use `STALEMATE` if the same arguments repeat for 2+ consecutive rounds with no new evidence. --- @@ -298,7 +225,6 @@ Recommendation: [Proceed with caution | Block merge | Security expert review req ## Example Security Finding -``` ISSUE-1: SQL Injection in user search endpoint Category: injection Severity: critical @@ -329,13 +255,11 @@ const users = await db.query(query, [req.query.name]); ``` Parameterized queries ensure user input is treated as data, not executable SQL code. -``` --- ## Example Secrets Detection -``` ISSUE-2: Hardcoded AWS credentials in configuration file Category: sensitive-data Severity: critical @@ -376,7 +300,6 @@ Additionally: 2. Use AWS IAM roles for EC2/Lambda instead of access keys 3. Add config/*.js to .gitignore 4. Use AWS Secrets Manager for production credentials -``` ---