← back to Cli Printing Press

fix(skills): stop phase 4.95 code review from being skipped (#1655)

5cb39a697616696dd4ea163b10298d0296726cbe · 2026-05-19 08:30:33 -0700 · Trevin Chow

* fix(skills): stop phase 4.95 code review from being skipped

Phase 4.95's instruction told Claude Code to invoke `/review` for an
in-place code review. But `/review` is PR-shaped — it fetches an open
GitHub PR and comments back via `gh`. At Phase 4.95 there is no PR
yet, so the agent hit the wrong-shape mismatch, then rationalized
skipping the entire phase under the harness-exemption clause ("no
built-in code review"). Code review was silently dropping out of the
pipeline on Claude Code.

Rewrite the section goal-shaped (intent: local review *before* PR
open; CI-time review is the wrong fix window). Tool selection becomes
a survey-and-pick step with `compound-engineering:ce-code-review` and
`/codex:review` as candidate skills and direct reviewer-subagent
dispatch via the Agent tool named as the universal fallback. Explicit
anti-pattern callout on `/review`. Narrow the harness exemption: skip
is only legitimate when neither a review skill nor subagent capability
exists, and "first tool name didn't fit", "no PR yet", and "PR-time CI
will catch it" are explicitly non-acceptable rationales.

Also slim the findings artifact. Fixed-in-place items collapse to a
one-line summary + commit hashes — the commits are already the record.
Full enumeration is reserved for the three non-fix buckets
(template-shape retro candidates, out-of-scope retro candidates,
surface-to-user findings) plus convergence outcome and review-path
chosen.

* fix(skills): clarify phase 4.95 round semantics and trim redundancy

Resolves Greptile P2 findings on #1655:

1. Direct-subagent-dispatch path was ambiguous about what a "round"
   means when 3-5 reviewers run in parallel. An agent could fix
   correctness findings, re-run only that subagent, see it clear,
   and declare convergence with security/maintainability findings
   still open. Add: a round means re-running ALL spawned reviewers
   in parallel, findings merge into a single set before autofix,
   convergence is the merged set being empty.

2. The Autofix policy block restated "cheapest fix window" verbatim
   from the opening paragraph. Drop the second occurrence.

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

Files touched

• M skills/printing-press/SKILL.md

Diff

commit 5cb39a697616696dd4ea163b10298d0296726cbe
Author: Trevin Chow <trevin@trevinchow.com>
Date:   Tue May 19 08:30:33 2026 -0700

    fix(skills): stop phase 4.95 code review from being skipped (#1655)
    
    * fix(skills): stop phase 4.95 code review from being skipped
    
    Phase 4.95's instruction told Claude Code to invoke `/review` for an
    in-place code review. But `/review` is PR-shaped — it fetches an open
    GitHub PR and comments back via `gh`. At Phase 4.95 there is no PR
    yet, so the agent hit the wrong-shape mismatch, then rationalized
    skipping the entire phase under the harness-exemption clause ("no
    built-in code review"). Code review was silently dropping out of the
    pipeline on Claude Code.
    
    Rewrite the section goal-shaped (intent: local review *before* PR
    open; CI-time review is the wrong fix window). Tool selection becomes
    a survey-and-pick step with `compound-engineering:ce-code-review` and
    `/codex:review` as candidate skills and direct reviewer-subagent
    dispatch via the Agent tool named as the universal fallback. Explicit
    anti-pattern callout on `/review`. Narrow the harness exemption: skip
    is only legitimate when neither a review skill nor subagent capability
    exists, and "first tool name didn't fit", "no PR yet", and "PR-time CI
    will catch it" are explicitly non-acceptable rationales.
    
    Also slim the findings artifact. Fixed-in-place items collapse to a
    one-line summary + commit hashes — the commits are already the record.
    Full enumeration is reserved for the three non-fix buckets
    (template-shape retro candidates, out-of-scope retro candidates,
    surface-to-user findings) plus convergence outcome and review-path
    chosen.
    
    * fix(skills): clarify phase 4.95 round semantics and trim redundancy
    
    Resolves Greptile P2 findings on #1655:
    
    1. Direct-subagent-dispatch path was ambiguous about what a "round"
       means when 3-5 reviewers run in parallel. An agent could fix
       correctness findings, re-run only that subagent, see it clear,
       and declare convergence with security/maintainability findings
       still open. Add: a round means re-running ALL spawned reviewers
       in parallel, findings merge into a single set before autofix,
       convergence is the merged set being empty.
    
    2. The Autofix policy block restated "cheapest fix window" verbatim
       from the opening paragraph. Drop the second occurrence.
    
    ---------
    
    Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
---
 skills/printing-press/SKILL.md | 41 +++++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/skills/printing-press/SKILL.md b/skills/printing-press/SKILL.md
index eb412bc9..fc28897e 100644
--- a/skills/printing-press/SKILL.md
+++ b/skills/printing-press/SKILL.md
@@ -2818,18 +2818,21 @@ The sub-skill carries `context: fork` so the reviewer agent's diagnostic chatter
 
 **Wave B rollout policy:** all findings surface as **warnings**, not blockers. Shipcheck does not fail on Phase 4.85 findings. Log the findings to `manuscripts/<api>/<run>/proofs/phase-4.85-findings.md` and surface them to the user. The user decides case by case whether to fix before shipping. Wave B calibrates false-positive rates before Wave C flips errors to blocking.
 
-## Phase 4.95: Native Code Review
+## Phase 4.95: Local Code Review
 
-**Runs after Phase 4.85, before Phase 5.** Reviews the printed CLI source for security and correctness issues using the harness's built-in code review.
+**Runs after Phase 4.85, before Phase 5.** Reviews the printed CLI source for security and correctness issues *before* any PR exists. This is the cheapest fix window in the pipeline — session context is hot, no PR feedback round-trip, no CI comments to chase. Catching issues here means they never become PR-time review comments, which is the wrong fix window for the same problems.
 
 **Target.** The generated CLI and MCP source under `$CLI_WORK_DIR`. In scope: `internal/cli/`, `internal/mcp/` (excluding `cobratree/`), `internal/store/`, `internal/client/`, and `cmd/`. **Out of scope:** `internal/cliutil/` and `internal/mcp/cobratree/` — these are generator-reserved packages. Any finding there is a machine bug; route to retro, do not patch in place.
 
-Use the harness-native code review path:
-- Claude Code: invoke `/review [target]` against `$CLI_WORK_DIR`.
-- Codex: review the current diff/target directly in built-in code-review mode.
-- Other harnesses: use their native review command or equivalent.
+**Tool selection — pick what's installed, do not name-match.** This phase needs *a* code review, not a specific named command. Survey the review-shaped capabilities the current harness has and pick the best fit. Plausible candidates (names drift across harnesses and plugin sets; treat this as an example list, not a closed set):
 
-**Autofix policy.** This is the cheapest fix-window in the entire pipeline — session context is hot, no PR feedback round-trip, no publish decision in flight. The default is fix. Surfacing to the user is the exception, not the rule. Severity is informational, not gating: a low-severity nil-deref is a 30-second fix; close it the same as a high-severity one.
+- A standalone, working-dir-shaped code review skill that runs against `git diff` and a file list without needing an open PR (e.g., `compound-engineering:ce-code-review`, or similar).
+- Codex's built-in code-review mode (`/codex:review`), which reviews the current diff or target directly.
+- **Direct reviewer-subagent dispatch via the Agent tool.** Spawn `correctness`, `security`, and `maintainability` reviewers (always-on) plus any conditional reviewers warranted by the diff (`api-contract`, `data-migrations`, `reliability`, `performance`) against the in-scope paths. This is the universal fallback: any harness that runs the press skill has the Agent tool, so this path is always available. When dispatching multiple reviewers, a "round" (per the autofix loop below) means re-running *all* spawned reviewers in parallel and merging their findings into a single set before autofix; convergence is the merged set being empty, not any individual reviewer clearing. Do not re-run only the reviewer whose prior findings were touched — every round must include every reviewer so cascading or newly-introduced issues surface.
+
+**Do not invoke Claude Code's `/review` for this phase.** `/review` is PR-shaped — it fetches an open GitHub PR and comments back via `gh`. There is no PR yet at Phase 4.95; the CLI is in a working dir that has not been promoted or published. Reaching for `/review`, bouncing off its shape, and claiming "harness has no code review" is the failure mode this section is written to prevent.
+
+**Autofix policy.** Session context is hot, no PR feedback round-trip, no publish decision in flight. The default is fix. Surfacing to the user is the exception, not the rule. Severity is informational, not gating: a low-severity nil-deref is a 30-second fix; close it the same as a high-severity one.
 
 Fix without asking when:
 - The fix is mechanical (parameterized query, input validation, error wrapping, missing nil check, dead code removal, obvious refactor).
@@ -2844,17 +2847,31 @@ Surface to the user only when the fix requires a real tradeoff they have to make
 
 Treat agent judgment as sufficient here — these categories are distinguishable on inspection. Conservatism is the failure mode, not over-fixing. Drafting an AskUserQuestion because "the user might want to know" is premature; fix the issue and note it in the shipcheck report.
 
-Re-run the native review after each autofix round until findings clear. Cap at 3 rounds; if findings persist after round 3, stop and surface — autofix is not converging. Findings in out-of-scope paths (`internal/cliutil/`, `internal/mcp/cobratree/`) file as retro-candidates and do not count toward the convergence check or the 3-round cap; the convergence check applies only to in-scope findings.
+Re-run the review after each autofix round until findings clear. Cap at 3 rounds; if findings persist after round 3, stop and surface — autofix is not converging. Findings in out-of-scope paths (`internal/cliutil/`, `internal/mcp/cobratree/`) file as retro-candidates and do not count toward the convergence check or the 3-round cap; the convergence check applies only to in-scope findings.
+
+**Findings artifact.** Log to `manuscripts/<api>/<run>/proofs/phase-4.95-findings.md`. Skip the per-finding enumeration for fixed-in-place items — the commits and diffs are already the authoritative record. Specifically:
+- **Autofix summary (one line).** "N findings autofixed in-place across M rounds; see commits `<hash>`, `<hash>`, …" Do not enumerate the fixed findings.
+- **Template-shape retro candidates (full detail).** Each finding's file:line, severity, the template path it appears to come from, and why it was filed instead of fixed. Not fixed in-place, so the log is the only record.
+- **Out-of-scope retro candidates (full detail).** Findings in `internal/cliutil/` or `internal/mcp/cobratree/`. Same shape as template-shape entries.
+- **Surface-to-user findings (full detail).** Each finding's file:line, severity, the real-tradeoff category it falls into, and the user's decision once they make one. Pending between turns; the log is what carries them.
+- **Convergence outcome (one line).** "Findings cleared at round N" or "stopped at round 3 with N findings outstanding — see surface-to-user list."
+- **Review path chosen (one line).** Skill name + invocation form, or "direct subagent dispatch" with the persona list. Lets a retro audit tool-selection drift across runs.
 
-**Findings artifact.** Log all review activity to `manuscripts/<api>/<run>/proofs/phase-4.95-findings.md`: each finding's file:line, severity, autofix outcome (fixed in-place / surfaced to user / filed as retro-candidate), and the corresponding fix commit or rationale. The shipcheck report references this file for the autofix summary; the retro skill scans it for template-shape candidates worth filing against the machine.
+The retro skill scans the template-shape and out-of-scope sections for candidates worth filing against the machine.
 
-**Rollout posture.** Unlike Phase 4.85, this phase starts without a warnings-only calibration period. The native review tools (`/review` in Claude Code, Codex's built-in review) are mature, well-understood surfaces — calibration risk is low. The 3-round autofix cap is the safety net for runaway findings, and the template-shape escape hatch routes systemic issues to retro instead of patching in place.
+**Rollout posture.** Unlike Phase 4.85, this phase starts without a warnings-only calibration period. Local code review is a well-understood surface — calibration risk is low. The 3-round autofix cap is the safety net for runaway findings, and the template-shape escape hatch routes systemic issues to retro instead of patching in place.
 
 **Template-shape escape hatch.** Even if a finding lives in an in-scope path, if it appears to come from a generator template (recurs across files in identical shape, sits in a path matched by `internal/generator/templates/`'s emit set, or duplicates a known prior template bug), file as retro-candidate and surface to the user rather than autofixing. Patching the printed CLI hides the machine bug from the next CLI.
 
-**Post-fix simplification (Claude Code only).** After the review + autofix loop converges, the printed CLI has fresh edits from the autofix passes — typically defensive guards, sanitization helpers, and near-duplicate fixes across sibling files. Run `/simplify` scoped to the same in-scope paths to consolidate duplication, remove dead code, and tighten the autofix output before dogfood. `/simplify` is Claude Code-only; skip on Codex and other harnesses (they have no built-in equivalent, and the press skill explicitly avoids custom simplification logic — same rule as the native code review above).
+**Post-fix simplification (Claude Code only).** After the review + autofix loop converges, the printed CLI has fresh edits from the autofix passes — typically defensive guards, sanitization helpers, and near-duplicate fixes across sibling files. Run `/simplify` scoped to the same in-scope paths to consolidate duplication, remove dead code, and tighten the autofix output before dogfood. `/simplify` is Claude Code-only; skip on Codex and other harnesses (they have no built-in equivalent, and the press skill explicitly avoids custom simplification logic — same rule as the review path above).
+
+**Harness exemption — narrow.** Skipping this phase is legitimate only when the current harness has *neither* a working-dir-shaped review skill *nor* the Agent/subagent capability needed for the direct-dispatch fallback. In practice this is almost never true — any harness that runs the press skill has access to subagents. The following rationales are **not** acceptable for skipping:
+
+- "The first tool name I tried (e.g., `/review`, `code-review:code-review`) didn't fit, so the harness must have no review path." Survey the catalog before claiming exemption; if no skill fits, dispatch reviewer subagents directly via the Agent tool.
+- "There's no PR yet, so code review can't run here." Pre-PR is the *point* of this phase. CI-time PR review is too late.
+- "PR-time CI review will catch it." That defeats the purpose of running review in the cheapest fix window.
 
-**Harness exemption.** If the current harness has no built-in code review, skip this phase with "Native code review unavailable in this harness; skipping" in the shipcheck report.
+If a skip is genuinely warranted, the shipcheck report must state which review-shaped capabilities were searched and why none fit — not just "harness exemption."
 
 ## Phase 5: Dogfood Testing
 

← 2c3271fc fix(cli): detect path params in browser-sniff URL grouper (#  ·  back to Cli Printing Press  ·  fix(cli): make live-dogfood runner enum-aware and resilient cda9b43b →