Fix Code Review Findings

Category-dispatch fix flow for findings from a prior code review. Each finding is routed to one of four buckets based on its category/subcategory: auto-fix, callsite-fix, specialized-fix, or manual-surface. Premise findings, injection attempts, sensitive-file changes, and coverage gaps never auto-apply — they surface in a manual-action report instead.

The caller controls re-review cycles.

Arguments

$ARGUMENTS

---

Step 1: Parse Arguments and Load Findings

TodoWrite([
  {"content": "Parse arguments and load findings", "status": "in_progress", "activeForm": "Parsing arguments"},
  {"content": "Categorize findings and print dispatch plan", "status": "pending", "activeForm": "Categorizing findings"},
  {"content": "Apply fixes per dispatch bucket", "status": "pending", "activeForm": "Applying fixes"},
  {"content": "Print manual-action report", "status": "pending", "activeForm": "Printing manual-action report"},
  {"content": "Run project verification", "status": "pending", "activeForm": "Running verification"},
  {"content": "Print summary", "status": "pending", "activeForm": "Printing summary"}
])

Parse flags

Extract from $ARGUMENTS:

Flag	Default	Effect
<CR_DIR> (positional)	auto-discover	Code-review session directory
--include-medium	off	Include MEDIUM findings (default: BLOCKING/HIGH only)
--include-tentative	off	Show TENTATIVE in manual-surface report (never auto-fixes)
--include-justified	off	Show JUSTIFIED-VALID in manual-surface report
--dry-run	off	Print dispatch plan and manual-action report; apply nothing
--apply	required in non-TTY	Required in non-interactive mode for code modification
--category-only <name>	unset	Restrict dispatch to a single category (e.g., --category-only Correctness)
--skip-verification	off	Skip the final code:build-validator step

--dry-run and --apply are mutually exclusive. Both unset in non-interactive (no TTY) → print plan, exit without applying.

Resolve CR_DIR

CR_DIR missing: auto-discover via ls -td .closedloop-ai/code-review/cr-* | head -1. No directories found → error "No code review session found. Run a code review first." → exit 1.

Load review envelope

Read <CR_DIR>/review_result.json (canonical envelope per PLN-719). There is no legacy fallback — zero-release backward compat per PRD-409. File missing → error "No code review output found in CR_DIR. Run a code review first." → exit 1.

Findings live in:

Bucket	What it contains	Default action
envelope.verified[]	CONFIRMED + DOWNGRADE + TENTATIVE + RE_ASSERTED + JUSTIFIED-INVALID	filter into dispatch
envelope.justified[]	JUSTIFIED-VALID (author defended, verifier confirmed)	exclude unless --include-justified (manual-surface only)
envelope.rejected[]	REJECTED by verifier	always exclude
envelope.pending_verification[]	Verifier didn't run on this finding (verifier failed, was deferred past budget, or --no-verify bypass)	include if severity matches; always manual-surface with the pending_verification.md template — /fix does NOT re-implement the verifier
envelope.coverage_gaps[]	System-scoped coverage findings	always include if severity matches; manual-surface bucket

Apply severity + verdict filter

The candidate set is built per flag state:

• always included: envelope.verified[], envelope.pending_verification[], envelope.coverage_gaps[]
• included only when --include-justified is set: envelope.justified[] (rendered manual-surface only; a JUSTIFIED-VALID verdict means the author defended the finding and the verifier confirmed the defense, so it never opts into auto-fix even if the operator wants visibility)

For each candidate finding:

sev = finding.get("verifier_severity") or finding.get("severity")
include = (sev in {"BLOCKING", "HIGH"}) or (sev == "MEDIUM" and --include-medium)

Exclude if verifier_verdict == "TENTATIVE" unless --include-tentative (TENTATIVE → manual-surface only).

Trust the envelope's verdict state. Findings in verified[] were already audited upstream by the verifier (PLN-722); /fix does not re-run a second verification pass on them — that was the v2.12.x design and is removed in this revision. Findings in pending_verification[] are routed to manual-surface with a pending_verification.md template so the operator can decide whether to re-run the review, --re-assert, or fix by hand; /fix does not have, and should not have, its own parallel implementation of the verifier.

If --category-only <name> set, filter to that category only.

If no findings remain → print "No actionable findings. Review complete." → mark all todos completed → exit 0.

---

Step 2: Categorize and Print Dispatch Plan

For each surviving finding, look up its dispatch bucket using the table below. Subcategory wins over category when both apply.

Dispatch Table

Category	Subcategory	Bucket	Notes
Correctness	—	auto-fix	Direct code edit at anchor line
Code Quality	—	auto-fix	DRY / maintainability — auto-fix at anchor
Documentation	—	auto-fix	Edit cited file:line
Security	—	auto-fix	Same flow as Correctness
Hygiene / Repo Hygiene	ci_artifacts	auto-fix	Delete or gitignore. Only auto-fix when the producer emits this exact subcategory — see _check_ci_artifacts in code_review_helpers.py.
Hygiene / Repo Hygiene	path_leakage	auto-fix	Replace path with env var / config reference. Only auto-fix when the producer emits this exact subcategory — see _check_path_leakage.
Hygiene / Repo Hygiene	gitignore_drift	auto-fix	Add gitignore entry. Only auto-fix when the producer emits this exact subcategory — see _check_gitignore_drift.
Hygiene / Repo Hygiene	sensitive_files	manual-surface	Never auto-modify (.env, credentials, .pem). Producer is _check_sensitive_files.
Hygiene / Repo Hygiene	(other / unset / unrecognized)	manual-surface	Fail-safe default: any Hygiene finding without a recognized subcategory routes to manual-surface. Prevents a hygiene producer that forgets to set subcategory (or a future producer that emits a subcategory we haven't routed yet) from silently falling into auto-fix on a sensitive file.
Premise	necessity	manual-surface	Auto-revert unsafe
Premise	cohesion	manual-surface	Surface cited 5+ examples
Premise	workaround	manual-surface	Surface root cause location
Premise	complexity	manual-surface	Both over- and under-complexity surfaced for author input (the auto-fix path for over-complexity / single-use abstraction is deferred to a follow-up phase)
Premise	(any other)	manual-surface	Fail safe
TestQuality	missing-coverage	manual-surface	Specialized-fix flow (test-engineer) is deferred to PLN-723 ship
TestQuality	weak-assertion	manual-surface	Deferred to PLN-723
TestQuality	mock-faithfulness	manual-surface	Deferred to PLN-723
TestQuality	missing-edge-case	manual-surface	Deferred to PLN-723
TestQuality	bug-locking	manual-surface	Never auto-fix even after PLN-723
TestQuality	test-deletion	manual-surface	Never auto-fix even after PLN-723
ImpactAnalysis	(any)	manual-surface	Callsite-fix flow is deferred to PLN-726 ship
CompanionChange	—	manual-surface	Companion content is context-dependent
Coverage	(any system_marker)	manual-surface	Re-run-or-acknowledge meta-action
InjectionAttempt	—	manual-surface	PR-author-controlled text — never code-edit

Drift-protection rule (applies to every bucket that would code-edit): before applying ANY auto-fix, re-read <finding.file> and search for <finding.code_snippet> (or a normalized prefix when the snippet is multi-line) within ±3 of <finding.line>. If not found, emit STALE_FINDING, skip the fix, route to manual-surface bucket with a code_snippet has drifted from cited location note.

Print plan

Fix Dispatch Plan
─────────────────
Auto-fix:        N findings  (breakdown by category)
Callsite-fix:    N findings  (deferred — surfaced as manual)  [appears only if PLN-726 not yet shipped]
Specialized:     N findings  (deferred — surfaced as manual)  [appears only if PLN-723 not yet shipped]
Manual surface:  N findings  (breakdown by category/subcategory)

Total auto-action: N findings.
Total requiring human action: N findings (surfaced in final report).

Interactive mode → Y/N prompt required.
Non-interactive → requires --apply to modify code; --dry-run for explicit no-op.

If --dry-run → render the manual-action report (Step 4) immediately, then exit 0.

If non-interactive and no --apply → render manual-action report, print "Non-interactive run without --apply — dispatch plan shown above; no fixes applied.", exit 0.

Interactive mode: prompt Proceed? [y/N] with no timeout (the prior auto-yes-after-5s behavior is removed). Default N.

---

Step 3: Dispatch and Apply Fixes

3a. Auto-fix bucket

Group surviving auto-fix findings by file. Apply sequentially within file (later fixes see earlier results); files run sequentially overall to keep the run-loop reproducible.

For each finding, perform the mandatory drift check first.

Empty-snippet guard (REQUIRED — applied before the grep). If finding.code_snippet is empty or whitespace-only, the grep pattern below would degenerate to grep -Fn "" which matches every line of the file and produces a trivially-true drift check. Hygiene findings can land in this state because the producer is permitted to omit code_snippet (code_review_schema.py defaults it to "" in normalize_legacy_finding). When the snippet is empty, do NOT run the grep: tag the finding MISSING_SNIPPET, route it to the manual-surface bucket with note code_snippet is empty — drift check cannot anchor the fix safely, and skip to the next finding.

# Drift check (only runs when code_snippet is non-empty)
grep -Fn "<first non-empty line of finding.code_snippet>" "<finding.file>" | \
  awk -F: '$1 >= <line - 3> && $1 <= <line + 3>' | head -1

No match → tag the finding STALE_FINDING, move it to the manual-surface bucket with note code_snippet has drifted from cited location, skip to next.

Match → launch Agent with subagent_type: "general-purpose", model: "sonnet":

Fix this code review finding. Minimal change only — no refactoring, no new features, no unnecessary error handling.
File: {file} | Line: {line}
Issue: {issue}
Explanation: {explanation}
Recommendation: {recommendation}

Read the file, apply the fix, confirm what changed.

Record modified files for the Step 6 summary.

3b. Callsite-fix bucket (DEFERRED — PLN-726)

ImpactAnalysis findings have an external_impact[] list of affected callsites in untouched files. The dedicated multi-file callsite update flow ships with PLN-726 (Cross-File Impact Analysis). Until then:

• Every ImpactAnalysis finding routes to manual-surface with template templates/impact_semantic_change.md.
• The manual-surface entry includes the anchor + every external_impact[] entry so the operator can apply the updates by hand.

3c. Specialized-fix bucket (DEFERRED — PLN-723)

TestQuality findings need a test-engineer subagent that ships with PLN-723. Until then:

• All TestQuality findings (including the four that would eventually auto-fix) route to manual-surface with template templates/testquality_specialized.md.
• bug-locking and test-deletion keep their permanent manual-surface routing using templates/testquality_bug_locking.md and templates/testquality_test_deletion.md.

3d. Manual-surface bucket

For each finding in the manual-surface bucket, look up the template per the routing table:

Routing	Template file
Premise/necessity	templates/premise_necessity.md
Premise/cohesion	templates/premise_cohesion.md
Premise/workaround	templates/premise_workaround.md
Premise/complexity	templates/premise_complexity.md
TestQuality/bug-locking	templates/testquality_bug_locking.md
TestQuality/test-deletion	templates/testquality_test_deletion.md
TestQuality/* (other, pre-PLN-723)	templates/testquality_specialized.md
ImpactAnalysis/* (pre-PLN-726)	templates/impact_semantic_change.md
CompanionChange/*	templates/companion_change.md
Coverage/*	templates/coverage_gap.md
InjectionAttempt/*	templates/injection_attempt.md
Hygiene/sensitive_files	templates/hygiene_sensitive.md
Any finding from envelope.pending_verification[] (verifier didn't run)	templates/pending_verification.md
(no matching template — e.g., a future category)	templates/_generic.md

Read each template once (Read tool, path <CLAUDE_PLUGIN_ROOT>/skills/fix/templates/<name>.md), then substitute placeholders {file}, {line}, {severity}, {category}, {subcategory}, {issue}, {explanation}, {recommendation}, {code_snippet}, plus category-specific placeholders documented in each template. Missing placeholder data → leave the placeholder visible so the operator sees the gap (do not silently drop).

Buffer each rendered entry for the Step 4 report.

---

Step 4: Manual-Action Report

After Step 3 completes (or immediately after Step 2 if --dry-run / non-interactive without --apply), print:

## Manual Action Required

The following findings need human action — they were not auto-fixed because their category requires operator judgment, the supporting flow has not shipped yet, or auto-fix would be unsafe.

[one rendered template entry per manual-surface finding, in dispatch order]

For STALE_FINDING entries, prepend a one-line note:

> ⚠️  STALE_FINDING — code_snippet has drifted from cited location at {file}:{line}. The finding may already be fixed, or the line may have moved. Verify before action.

If the manual-surface bucket is empty, print _No findings require manual action._.

---

Step 5: Run Project Verification

Skipped when --skip-verification set or when no auto-fix bucket entries succeeded (nothing changed).

Launch Agent with subagent_type: "code:build-validator":

Run all validation commands (test, lint, typecheck, build). Report VALIDATION_PASSED, VALIDATION_FAILED, or NO_VALIDATION.

Do NOT run validation commands directly — build-validator discovers and runs them.

• PASSED or NO_VALIDATION → proceed to Step 6
• FAILED → launch general-purpose subagent (model: "sonnet") to fix, re-run build-validator. Max 5 attempts. Warn and proceed on persistent failure.

---

Step 6: Summary

Print the structured summary:

## Fix Summary

### Auto-fixes applied (N)
| Category | Findings | Files modified |
|---|---|---|
| Correctness | n | file1, file2 |
| Hygiene (auto-safe) | n | .gitignore |
| ... | | |

### Deferred (await future plans)
| Bucket | Findings | Status |
|---|---|---|
| Callsite-fix | n | Routed to manual-surface — PLN-726 not yet shipped |
| Specialized-fix | n | Routed to manual-surface — PLN-723 not yet shipped |

(Omit the Deferred table if both counts are zero.)

### Manual action required (N)
[short list — one line per finding pointing at file:line + category — full templates in Step 4 above]

### Verification
| Status | Detail |
|---|---|
| Build/Tests/Lint/Type check | PASSED / FAILED / NO_VALIDATION / SKIPPED |

### Telemetry
| Metric | Value |
|---|---|
| Findings received | N |
| Auto-fixed | N |
| Manual surface | N |
| Stale findings (drifted) | N |
| Pending-verification routes | N (findings without upstream verifier_verdict surfaced to operator) |
| Total /fix duration | Hh Mm Ss |

Write the structured result file

After printing the summary, write <CR_DIR>/fix_result.json so non-interactive callers (run-loop.sh, future automation) can act on the outcome without parsing stdout:

{
  "schema_version": 1,
  "findings_received": N,
  "auto_fixed": N,
  "manual_surface": N,
  "stale_findings": N,
  "pending_verification_routes": N,
  "deferred_callsite": N,
  "deferred_specialized": N,
  "build_validator_status": "PASSED" | "FAILED" | "NO_VALIDATION" | "SKIPPED",
  "manual_action_required": <bool — see below>,
  "duration_seconds": <int>
}

manual_action_required is true when auto_fixed == 0 AND manual_surface > 0 — i.e., this run made no automated progress and the remaining findings can only be resolved by an operator. This is the signal run-loop.sh reads to halt the review-fix cycle (see below).

Pick the exit code

Condition	Exit code
manual_action_required == true (no auto-fixes ran, ≥1 manual-surface entry)	2 — "manual action required, no automated progress possible"
build_validator_status == "FAILED" after auto-fixes	1 — runtime error: validation regressed
Everything else (including the all-auto-fixed-cleanly path and the no-findings path)	0

Exit 2 is the new "halt the loop" signal — distinct from exit 1 (runtime error) so harness callers can branch on it deliberately. Interactive users see the same dispatch plan and manual-action report either way; the exit code only affects programmatic consumers.

Mark all todos completed.

---

Notes for run-loop.sh and other callers

The default mode is dry-run when no TTY. run-loop.sh must invoke this skill with --apply to retain the auto-apply behavior:

"$CLAUDE" -p "/code-review:fix $cr_dir --apply"

/code-review:fix $cr_dir without --apply from a non-interactive context will print the dispatch plan + manual-action report and exit without modifying code — by design.

Closed-loop callers: read fix_result.json AND the exit code

Automation that calls /code-review:fix in a loop (notably plugins/code/scripts/run-loop.sh's post_loop_review_fix) must distinguish three cases:

Exit code	Meaning	Recommended caller action
0	Skill ran, made progress (auto-fixes applied) OR found nothing actionable	Continue the loop normally — the next review pass should see fewer findings or verdict: approve
2	Skill ran, made NO automated progress, ≥1 manual-surface entry remains	Halt the review-fix cycle. Re-running the review will re-surface the same findings; no human is reading the manual-action report inside the loop. Log a clear message naming the manual-surface count and category breakdown (sourceable from <CR_DIR>/fix_result.json).
1	Skill itself failed (Claude harness error, malformed envelope, etc.)	Retry per the caller's existing error policy

Without the exit-2 branch, a loop calling /code-review:fix will burn its full cycle budget re-detecting the same manual-surface findings every cycle, then exit as if no work was needed — leaving operators no signal that human action is pending.