Skip to content

Commit eadebed

Browse files
committed
fix(pm): address codex review findings on prompt robustness
- Add INCOMPLETE assessment status for when review sources fail/timeout - Require evidence citations (plan_ref, spec_ref, evidence) in codex plan reviewer findings - Require codex to verify file paths via read-only shell inspection - Deduplicate by issue fingerprint not line proximity - Calibrate gate semantics: only CRITICAL or 3+ IMPORTANT shift recommendation, MINOR/taste stays advisory - Explicit source status tracking (ok/empty/failed/skipped)
1 parent cda95c9 commit eadebed

4 files changed

Lines changed: 83 additions & 27 deletions

File tree

‎pm/SKILL.md‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -994,11 +994,15 @@ which codex >/dev/null 2>&1 && echo "CODEX_AVAILABLE" || echo "CODEX_SKIP"
994994
- `[CURRENT_HEAD]` → output of `git rev-parse HEAD`
995995

996996
4. Handle the synthesizer's Assessment:
997-
- **CLEAN:** No findings. Proceed to Present & Publish with clean recommendation.
997+
- **CLEAN:** No findings, both sources completed. Proceed to Present & Publish with clean recommendation.
998998
- **HAS_FIXES:** Dispatch a fix subagent (model: sonnet) with the Auto-fixable list.
999999
After fixes commit, proceed to Present & Publish with clean recommendation.
1000-
- **HAS_CONCERNS:** Carry the "Surface to User" list forward. Shift the recommendation
1001-
in Present & Publish away from "Publish" toward "Review the code first."
1000+
- **HAS_CONCERNS:** Carry the "Surface to User" list forward. Only shift the
1001+
recommendation away from "Publish" if CRITICAL items exist or there are 3+
1002+
IMPORTANT items. Otherwise include findings as advisory in the summary.
1003+
- **INCOMPLETE:** One or both review sources failed/timed out. Include whatever
1004+
findings are available and note in the summary that the review was partial.
1005+
Recommend the user review manually before publishing.
10021006

10031007
**If `CODEX_SKIP`:**
10041008

@@ -1032,7 +1036,7 @@ Present the completed work to the user.
10321036
- C) Changes needed — tell me what to adjust
10331037
- D) Run /qa first (recommend if there's a testable URL you haven't checked)
10341038

1035-
RECOMMENDATION: A if cross-model review returned CLEAN or HAS_FIXES (all fixed). B if cross-model review returned HAS_CONCERNS with unfixed items — the user should see the findings before publishing. D if the feature has a web-testable component and hasn't been tested in a browser yet.
1039+
RECOMMENDATION: A if cross-model review returned CLEAN or HAS_FIXES (all fixed). B if cross-model review returned HAS_CONCERNS with CRITICAL items, 3+ IMPORTANT items, or INCOMPLETE — the user should see the findings before publishing. D if the feature has a web-testable component and hasn't been tested in a browser yet.
10361040

10371041
**If the user picks A:** Run `/publish`. It handles versioning, CHANGELOG, test verification,
10381042
PR creation, and everything else. Don't do it manually.

‎pm/SKILL.md.tmpl‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -647,11 +647,15 @@ which codex >/dev/null 2>&1 && echo "CODEX_AVAILABLE" || echo "CODEX_SKIP"
647647
- `[CURRENT_HEAD]` → output of `git rev-parse HEAD`
648648

649649
4. Handle the synthesizer's Assessment:
650-
- **CLEAN:** No findings. Proceed to Present & Publish with clean recommendation.
650+
- **CLEAN:** No findings, both sources completed. Proceed to Present & Publish with clean recommendation.
651651
- **HAS_FIXES:** Dispatch a fix subagent (model: sonnet) with the Auto-fixable list.
652652
After fixes commit, proceed to Present & Publish with clean recommendation.
653-
- **HAS_CONCERNS:** Carry the "Surface to User" list forward. Shift the recommendation
654-
in Present & Publish away from "Publish" toward "Review the code first."
653+
- **HAS_CONCERNS:** Carry the "Surface to User" list forward. Only shift the
654+
recommendation away from "Publish" if CRITICAL items exist or there are 3+
655+
IMPORTANT items. Otherwise include findings as advisory in the summary.
656+
- **INCOMPLETE:** One or both review sources failed/timed out. Include whatever
657+
findings are available and note in the summary that the review was partial.
658+
Recommend the user review manually before publishing.
655659

656660
**If `CODEX_SKIP`:**
657661

@@ -685,7 +689,7 @@ Present the completed work to the user.
685689
- C) Changes needed — tell me what to adjust
686690
- D) Run /qa first (recommend if there's a testable URL you haven't checked)
687691

688-
RECOMMENDATION: A if cross-model review returned CLEAN or HAS_FIXES (all fixed). B if cross-model review returned HAS_CONCERNS with unfixed items — the user should see the findings before publishing. D if the feature has a web-testable component and hasn't been tested in a browser yet.
692+
RECOMMENDATION: A if cross-model review returned CLEAN or HAS_FIXES (all fixed). B if cross-model review returned HAS_CONCERNS with CRITICAL items, 3+ IMPORTANT items, or INCOMPLETE — the user should see the findings before publishing. D if the feature has a web-testable component and hasn't been tested in a browser yet.
689693

690694
**If the user picks A:** Run `/publish`. It handles versioning, CHANGELOG, test verification,
691695
PR creation, and everything else. Don't do it manually.

‎pm/codex-plan-reviewer-prompt.md‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,12 @@ You are reviewing an implementation plan for a software feature.
2525
Read the plan at: [PLAN_FILE_PATH]
2626
Read the approved spec at: [SPEC_FILE_PATH]
2727
28-
Review the plan against the spec and check:
28+
Review the plan against the spec. For each check, use read-only shell commands to
29+
verify claims in the plan (e.g., check that referenced file paths exist, inspect
30+
the repo structure). If you cannot verify a claim, say "not verified" rather than
31+
assuming it's correct.
32+
33+
Check:
2934
3035
1. **Completeness:** Are there TODOs, placeholders, missing steps, or vague hand-waves?
3136
Every task should be actionable without guessing.
@@ -34,8 +39,8 @@ Review the plan against the spec and check:
3439
scope creep (tasks that go beyond what the spec asks for)?
3540
3641
3. **Feasibility:** Could an engineer follow this plan without getting stuck? Are file
37-
paths real? Are dependencies between tasks correct? Would the batch ordering actually
38-
work?
42+
paths real (verify by listing them)? Are dependencies between tasks correct? Would
43+
the batch ordering actually work?
3944
4045
4. **Batch groupings:** Are risky/complex tasks isolated in solo batches? Are independent
4146
tasks correctly marked for parallel execution?
@@ -49,13 +54,20 @@ Tag each finding with a severity:
4954
- [P2] — Minor issue or suggestion. Won't block implementation but worth noting.
5055
Examples: could be more specific, batch grouping could be tighter, naming suggestion.
5156
52-
Output format:
57+
Every finding MUST include evidence. Use this format:
5358
5459
PLAN REVIEW:
5560
- [P1] [specific finding] — [why it matters]
61+
plan_ref: [which task/step in the plan]
62+
spec_ref: [which section of the spec, or "n/a"]
63+
evidence: [what you saw when you checked — command output, file listing, or "not verified"]
64+
suggested_change: [concrete fix]
65+
5666
- [P2] [specific finding] — [suggestion]
67+
plan_ref: [task/step]
68+
evidence: [what you checked]
5769
58-
If the plan looks solid, output:
70+
If the plan looks solid after all checks, output:
5971
PLAN REVIEW: No issues found.
6072
```
6173

‎pm/cross-model-reviewer-prompt.md‎

Lines changed: 50 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,20 @@ This is the output from OpenAI's Codex CLI reviewing the same diff:
3131
3232
[CODEX_OUTPUT]
3333
34-
(If this says "codex not available": skip all cross-model logic. Just pass through
35-
the Claude /review findings with all items marked [SINGLE SOURCE: review].)
34+
(If this says "codex not available" or is empty/error output: skip all cross-model logic.
35+
Just pass through the Claude /review findings with all items marked [SINGLE SOURCE: review].
36+
Set source_status for codex to "skipped" or "failed".)
37+
38+
## Source Status
39+
40+
Before synthesizing, classify each source's output:
41+
- **ok**: Source returned structured findings (even if "no issues found")
42+
- **empty**: Source returned but with no parseable findings
43+
- **failed**: Source timed out, errored, or returned garbage
44+
- **skipped**: Source was not run (e.g., codex not installed)
45+
46+
You MUST note source status in your output. NEVER output Assessment: CLEAN unless
47+
both sources have status "ok" — if either source failed/skipped, use INCOMPLETE instead.
3648
3749
## Diff Context
3850
@@ -44,9 +56,13 @@ need to verify a finding.
4456
4557
## Your Task
4658
47-
1. **Deduplicate by location.** If both sources flag the same file:line (or clearly
48-
the same issue within 5 lines), merge into one finding. Keep the more specific
49-
description. Mark as [HIGH CONFIDENCE] — two independent models agree.
59+
1. **Deduplicate by issue fingerprint.** Two findings are duplicates if they describe
60+
the same root cause and fix, even if they reference different line numbers (e.g.,
61+
a caller and callee). Merge duplicates into one finding. Keep the more specific
62+
description and the most precise location. Mark as [HIGH CONFIDENCE] — but only
63+
if you verified the issue exists by checking the actual diff.
64+
65+
Two unrelated findings on nearby lines are NOT duplicates — don't merge by proximity alone.
5066
5167
2. **Categorize each finding** using this taxonomy:
5268
- **auto-decidable** — Style, formatting, trivial fix, obvious bug with clear fix.
@@ -67,22 +83,39 @@ need to verify a finding.
6783
- IMPORTANT: Missing tests, performance issues, pattern violations
6884
- MINOR: Style, naming, documentation
6985
70-
5. **Assess overall status:**
71-
- CLEAN: No findings from either source
72-
- HAS_FIXES: Only auto-fixable items found
73-
- HAS_CONCERNS: Items that need user attention exist
86+
5. **Assess overall status.** Output EXACTLY ONE of these values on the Assessment line:
87+
- CLEAN — No findings from either source AND both sources completed successfully (status: ok)
88+
- HAS_FIXES — Only auto-fixable items found, no items requiring user attention
89+
- HAS_CONCERNS — CRITICAL or IMPORTANT items that need user attention exist
90+
- INCOMPLETE — One or both sources failed/skipped/empty. Pass through whatever
91+
findings are available, but flag that the review is partial.
92+
93+
## Gate Calibration
94+
95+
Not every concern should block publishing:
96+
- CRITICAL (security, data loss, broken functionality) → always surface, shift away from publish
97+
- IMPORTANT → surface in summary, but don't shift recommendation unless there are 3+ items
98+
- MINOR and taste items → include in summary as advisory, never shift recommendation
7499
75100
## Output Format
76101
102+
Output this exact structure. Do not add extra sections or commentary.
103+
77104
## Cross-Model Review Synthesis
78105
106+
**Source Status:** review: [ok|empty|failed|skipped], codex: [ok|empty|failed|skipped]
107+
79108
### Auto-fixable
80109
- [file:line] [issue] — [specific fix] [HIGH CONFIDENCE|SINGLE SOURCE: review|SINGLE SOURCE: codex]
81110
111+
(If none: "No auto-fixable items.")
112+
82113
### Surface to User
83114
- [SEVERITY] [file:line] [issue] — [recommendation] [HIGH CONFIDENCE|SINGLE SOURCE: review|SINGLE SOURCE: codex]
84115
85-
### Assessment: CLEAN | HAS_FIXES | HAS_CONCERNS
116+
(If none: "No items requiring attention.")
117+
118+
### Assessment: [exactly one of: CLEAN | HAS_FIXES | HAS_CONCERNS | INCOMPLETE]
86119
87120
If Assessment is HAS_CONCERNS, add:
88121
### Recommendation
@@ -93,11 +126,14 @@ If Assessment is HAS_CONCERNS, add:
93126

94127
## After synthesizer responds
95128

96-
Parse the Assessment line:
129+
Parse the Assessment line (look for the word after "Assessment:"):
97130

98131
- **CLEAN:** Proceed to Phase 6 with clean recommendation (Publish).
99132
- **HAS_FIXES:** Dispatch a fix subagent (model: sonnet) with the Auto-fixable list.
100133
After fixes commit, proceed to Phase 6 with clean recommendation.
101-
- **HAS_CONCERNS:** Carry the "Surface to User" list into Phase 6. Shift the
102-
recommendation away from "Publish" toward "Review the code first." Include the
103-
synthesizer's Recommendation line in the Phase 6 summary.
134+
- **HAS_CONCERNS:** Carry the "Surface to User" list into Phase 6. Only shift
135+
the recommendation away from "Publish" if CRITICAL items exist or there are 3+
136+
IMPORTANT items. Otherwise include findings as advisory in the summary.
137+
- **INCOMPLETE:** Proceed to Phase 6 but note in the summary that the review was
138+
partial. Include whatever findings are available. Recommend the user do a manual
139+
review before publishing.

0 commit comments

Comments
 (0)