--- name: fix description: Verify and fix BLOCKING/HIGH code review findings from a prior review session via category-dispatch, then run project verification. argument-hint: " [--include-medium] [--include-tentative] [--include-justified] [--dry-run | --apply] [--category-only ] [--skip-verification]" --- # 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**. 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 ```json 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 | |---|---|---| | `` (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 ` | 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 `/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 or was deferred past budget) | 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 ` 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. | | `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 `` and search for `` (or a normalized prefix when the snippet is multi-line) within `±3` of ``. 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. ```bash # Drift check (only runs when code_snippet is non-empty) grep -Fn "" "" | \ awk -F: '$1 >= && $1 <= ' | 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 | |---|---| | `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 `/skills/fix/templates/.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: ```markdown ## 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: ```markdown ## 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 `/fix_result.json` so non-interactive callers (`run-loop.sh`, future automation) can act on the outcome without parsing stdout: ```json { "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": , "duration_seconds": } ``` `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: ```bash "$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 `/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.