fix(ce-code-review): close LFG fixer gap and reconcile walkthrough

Address PR #685 review feedback:

- Step 3 fixer queue contract previously defined behavior for
  gated_auto/manual WITH suggested_fix and manual WITHOUT, but left
  gated_auto WITHOUT suggested_fix undefined. Extend the no-fix branch
  to cover both gated_auto and manual so they route to failed instead
  of being silently skipped, preserving the apply-or-fail contract.

- walkthrough.md still documented LFG-the-rest -> Proceed/Cancel and
  end-of-loop dispatch semantics from the removed bulk-preview path.
  Reconcile: scope end-of-walk-through dispatch to the run-to-completion
  path and explicitly point to the LFG-the-rest bullet for that path's
  single-dispatch semantics. Remove stale Proceed/Cancel framings and
  the bulk-preview reference in the no-sink adaptation.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Trevin Chow
2026-04-25 10:24:09 -07:00
parent f1e36e990b
commit 530560a3eb
2 changed files with 7 additions and 5 deletions
@@ -782,7 +782,7 @@ The fixer accepts two queue shapes depending on which caller invoked it:
- **Homogeneous queue (autofix, headless, walk-through Apply set):** every item is `safe_auto -> review-fixer` (autofix, headless), or every item carries a concrete `suggested_fix` (walk-through Apply set, where the user picked Apply on each finding). The fixer applies each item.
- **Heterogeneous queue (LFG path — interactive option B and walk-through `LFG the rest`):** the queue mixes `gated_auto`, `manual`, and `advisory` findings. Each item carries: `autofix_class`, `severity`, `file:line`, `title`, `suggested_fix` (may be null), `why_it_matters`, and `evidence`. The fixer routes each item by class:
- **`safe_auto` / `gated_auto` / `manual` with `suggested_fix`:** light evidence-match check (verify the cited code at `file:line` still resembles the persona's evidence — concretely: at least one identifier or distinctive token from the evidence appears at the cited location, and the line has not been deleted). If the check passes, attempt to apply the fix. On clean apply, route to `applied`. On fix-application failure (line moved, conflicting edit, syntax issue), route to `failed` with a concrete reason such as `fix did not apply cleanly: <error>`.
- **`manual` without `suggested_fix`:** route to `failed` with reason `no fix proposed by reviewer`. This signal indicates the persona judged the finding to need cross-team input or context outside the review.
- **`gated_auto` or `manual` without `suggested_fix`:** route to `failed` with reason `no fix proposed by reviewer`. For `manual` this signal indicates the persona judged the finding to need cross-team input or context outside the review. For `gated_auto` this is a defensive case (the persona shouldn't normally produce `gated_auto` without a concrete fix) — surface it in `failed` rather than skipping it, to preserve the apply-or-fail contract.
- **Advisory items (`autofix_class: advisory`):** no-op. Route to `advisory` (recorded as acknowledged).
- **Evidence-match check fails:** route to `failed` with reason `evidence no longer matches code at <file:line>`. This is the false-positive case — the finding cited something that has since changed or was already handled.
@@ -141,7 +141,7 @@ When reviewers disagreed or content context cuts against the default, still mark
- **Advisory-only finding:** when the finding's `autofix_class` is `advisory` (no actionable fix), option A is replaced with `Acknowledge — mark as reviewed`. The other three options remain. The advisory variant is the only case where `Acknowledge` appears in the menu.
- **N=1 (exactly one pending finding):** the terminal block's heading omits `Finding N of M` and renders as `## {severity} {plain-English title}`. The stem's first line drops the position counter, becoming `{severity} {short handle}.` Option D (`LFG the rest`) is suppressed because no subsequent findings exist — the menu shows three options: Apply / Defer / Skip (or Acknowledge, for advisory).
- **No sink (Defer option unavailable):** when the tracker-detection tuple reports `any_sink_available: false` (every tier in the fallback chain — named tracker and GitHub Issues via `gh` — is unreachable), option B (`Defer`) is omitted. The stem appends one line explaining why (e.g., `Defer unavailable on this platform — no durable tracker sink detected.`). The menu shows three options: Apply / Skip / LFG the rest (and Acknowledge in place of Apply for advisory-only findings). **Before rendering the options, remap any per-finding `Defer` recommendation produced by Stage 5 step 7b to `Skip`** so the `(recommended)` marker always lands on an option that is actually in the menu. When the remap fires, surface it on the R15 conflict context line (e.g., `Stage 5 recommended Defer; downgraded to Skip — no tracker sink available.`). This is a render-time runtime step mirroring the Defer→Skip downgrade that `bulk-preview.md` performs for LFG previews; Stage 5 step 7b has no knowledge of sink availability and only orders conflicting reviewer recommendations.
- **No sink (Defer option unavailable):** when the tracker-detection tuple reports `any_sink_available: false` (every tier in the fallback chain — named tracker and GitHub Issues via `gh` — is unreachable), option B (`Defer`) is omitted. The stem appends one line explaining why (e.g., `Defer unavailable on this platform — no durable tracker sink detected.`). The menu shows three options: Apply / Skip / LFG the rest (and Acknowledge in place of Apply for advisory-only findings). **Before rendering the options, remap any per-finding `Defer` recommendation produced by Stage 5 step 7b to `Skip`** so the `(recommended)` marker always lands on an option that is actually in the menu. When the remap fires, surface it on the R15 conflict context line (e.g., `Stage 5 recommended Defer; downgraded to Skip — no tracker sink available.`). This is a render-time runtime step; Stage 5 step 7b has no knowledge of sink availability and only orders conflicting reviewer recommendations.
- **Combined N=1 + no sink:** the menu shows two options: Apply / Skip (or Acknowledge / Skip).
Only when `ToolSearch` explicitly returns no match or the tool call errors — or on a platform with no blocking question tool — fall back to presenting the options as a numbered list and waiting for the user's next reply.
@@ -182,13 +182,15 @@ Formal cross-session resumption is out of scope for v1.
## End-of-walk-through dispatch
After the loop terminates — either every finding has been answered, or the user took `LFG the rest → Proceed` the walk-through hands off to the dispatch phase:
This section covers the run-to-completion path only — every finding has been answered Apply / Defer / Skip / Acknowledge and the loop ended naturally. The `LFG the rest` path exits the walk-through earlier and dispatches its own fixer pass on the union of (accumulated Apply set remaining undecided findings); see the `LFG the rest` bullet under "Per-finding routing" above. There is no second dispatch in that branch.
When the loop runs to completion, the walk-through hands off to the dispatch phase:
1. **Apply set:** spawn one fixer subagent for the full accumulated Apply set. The fixer receives the set as its input queue and applies all changes in one pass against the current working tree. This preserves the existing "one fixer, consistent tree" mechanic and gives the fixer the full set at once to handle inter-fix dependencies (two Applies touching overlapping regions). The existing Step 3 fixer prompt needs a small update to acknowledge this queue may be heterogeneous (`gated_auto` and `manual` mix, not just `safe_auto`) — authored alongside this reference.
2. **Defer set:** already executed inline during the walk-through. Nothing to dispatch here.
3. **Skip / Acknowledge:** no-op.
After dispatch completes (or after `LFG the rest → Cancel` followed by the user working through remaining findings one at a time, or after the loop runs to completion), emit the unified completion report described below.
After dispatch completes, emit the unified completion report described below.
---
@@ -197,7 +199,7 @@ After dispatch completes (or after `LFG the rest → Cancel` followed by the use
Every terminal path of Interactive mode emits the same completion report structure. This covers:
- Walk-through completed (all findings answered)
- Walk-through bailed via `LFG the rest → Proceed`
- Walk-through bailed via `LFG the rest`
- Top-level LFG (routing option B) completed
- Top-level File tickets (routing option C) completed
- Zero findings after `safe_auto` (routing question was skipped — the completion summary is a one-line degenerate case of this structure)