skills/everyinc__compound-engineering-plugin
1
0
Fork 0
mirror of https://github.com/EveryInc/compound-engineering-plugin.git synced 2026-09-19 01:09:55 +08:00
Code Issues Packages Projects Releases Wiki Activity
Files
tmchow/https-github.com-EveryInc-compound-engineering-plugin-issues-867
everyinc__compound-engineer…/docs/plans/2026-04-25-001-fix-ce-code-review-lfg-defer-bias-plan.md
T
Trevin Chow 9ba41a14ca fix(ce-code-review): replace LFG with best-judgment auto-resolve (#685) 
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-25 18:12:09 -07:00

38 KiB
Raw Permalink Blame History

title, type, status, date
title type status date
fix: Remove LFG defer-bias and bulk-preview gate in ce-code-review fix active 2026-04-25

fix: Remove LFG defer-bias and bulk-preview gate in ce-code-review

Overview

Fix the LFG path's defer-bias in plugins/compound-engineering/skills/ce-code-review/ so that picking LFG actually applies fixes the agent can defensibly propose, instead of routing them to ticket-filing. Replace the bulk-preview approval gate with a "just go" execution model: dispatch the fixer immediately on LFG selection, summarize what got applied versus what could not be resolved, then ask one targeted question about the leftovers.

The fix has two parts that work together:

  1. Push personas to commit to a suggested_fix more aggressively when one is reachable from diff and surrounding code, so findings come into synthesis already classified correctly. Only manual findings the persona honestly cannot propose for stay un-fixed.
  2. Drop both gates on the LFG path — the Stage 5b validator pre-pass and the bulk-preview approval prompt. The fixer's success/failure is the validation; the diff is the audit surface; the user reviews via diff after the fact, not via preview before the fact.

This is consistent for option B (top-level LFG) and the walk-through's LFG the rest. Walk-through option A automatically inherits the persona-side improvement: manual + suggested_fix recommends Apply, so per-finding recommendations stop punting to Defer.

Problem Frame

A real /ce-code-review run on a 13-finding branch routed 8 findings to "file tickets" when LFG was selected. The user pushed back ("can we not decide how to fix them all?"), and the agent then produced concrete, defensible design decisions for all 8 in one pass — without external context the agent did not already have. The rescue prompt revealed that the skill should be doing this work by default rather than waiting for the user to override.

Two structural causes:

  1. autofix_class: manual is overloaded. Personas use manual for two semantically different cases — "needs design judgment but I can propose one" and "genuinely needs cross-team alignment." Today's subagent template tells personas suggested_fix is optional ("a bad suggestion is worse than none"), and personas default to manual when they don't want to commit to a fix shape. Synthesis inherits that punt — Stage 5 step 6b's tie-break order (Skip > Defer > Apply > Acknowledge) routes both meanings to Defer.
  2. The LFG path performs research before approval, then asks for approval. Stage 5b dispatches a validator subagent per surviving finding, then bulk-preview renders a plan, then waits for Proceed/Cancel. On uncommitted local edits — which is what LFG runs on — this inverts the cost calculus. Reverting an applied edit is cheaper than closing 8 GitHub issues that should not have been filed.

The current references/bulk-preview.md, walk-through LFG the rest, and Stage 5b validation gate were all built to mitigate "agent might apply a wrong fix in bulk." They mitigate it by adding research and approval overhead. The user's framing inverts the bias: trust the agent on uncommitted edits, audit via diff, file tickets only for items the agent honestly cannot resolve.

Requirements Trace

  • R1. Personas attempt to propose suggested_fix whenever a defensible fix is reachable from the diff and surrounding code. manual without suggested_fix is reserved for findings that genuinely need cross-team input, business context, or research the reviewer cannot do during this review.
  • R2. Stage 5 action-derivation rule maps manual + suggested_fix to recommended-action Apply. manual without suggested_fix continues to map to Defer. Step 6b's cross-reviewer tie-break order is unchanged.
  • R3. The LFG path — both interactive routing option B (top-level) and the walk-through's LFG the rest sub-decision within option A — does not run Stage 5b validation. Other paths (autofix, headless, walk-through option A's tracker-defer handoff for individual Defer choices, file-tickets option C) still run Stage 5b.
  • R4. The LFG path does not render a bulk-preview approval prompt. Selecting LFG dispatches the fixer immediately on the full pending action set (gated_auto + manual + advisory).
  • R5. The fixer subagent handles a heterogeneous queue: items with concrete suggested_fix get applied; advisory items are no-op (recorded as acknowledged); items where the fix cannot be applied cleanly, where the cited evidence no longer matches the code at the cited location, or that lack a suggested_fix entirely are routed to a failed bucket with a one-line reason. False-positive recognition flows through the same failed bucket — they are not silently filtered. The user retains agency over whether to file/walk/ignore them via the post-run question.
  • R6. After the fixer returns, the unified completion report is emitted as today. When the failed bucket is non-empty, one post-run question fires using the platform's blocking question tool with three options: file tickets for the leftovers (when a tracker sink is available), walk through them one by one, or ignore.
  • R7. Walk-through option A inherits R2's action-derivation change automatically — manual + suggested_fix findings recommend Apply with the proposed fix shown in the terminal block. No separate logic added in references/walkthrough.md.
  • R8. references/bulk-preview.md is narrowed to option C (file-tickets) only. The walk-through LFG the rest Cancel-from-preview semantics are removed (no preview to cancel from). Option C's preview behavior is unchanged.
  • R9. Tests in tests/review-skill-contract.test.ts and tests/pipeline-review-contract.test.ts are updated to match the new flow contract, not to preserve assertions against the removed gates.

Scope Boundaries

  • The safe_auto -> review-fixer automatic-fix pass before the routing question is unchanged. LFG only governs gated_auto / manual / advisory items remaining after that pass.
  • File-tickets routing (option C) is unchanged — the bulk-preview is still its approval surface, and Stage 5b still runs on every pending finding before tickets fire.
  • Autofix and headless modes are unchanged. They never ran the bulk-preview gate, and they retain Stage 5b.
  • Report-only mode is unchanged.
  • The 5-anchor confidence rubric, the >= 75 filter threshold (with P0 escape at 50+), cross-reviewer promotion, and mode-aware demotion (step 6c) are unchanged. This plan only touches the recommended-action derivation and the LFG dispatch path.
  • No P0-special-case carve-out. P0 findings flow through the same LFG path as other severities. Reverts are cheap on uncommitted edits regardless of severity, and a P0 carve-out would break the "just go" promise.
  • The manual vs gated_auto distinction in autofix_class is preserved in finding output. This plan does not collapse them — synthesis still records what the persona thought. The change is in how the recommended action is derived, not in the underlying classification.

Context & Research

Relevant Code and Patterns

  • plugins/compound-engineering/skills/ce-code-review/SKILL.md — Stage 5 step 6b (action-derivation tie-break), Stage 5b conditional table, Step 2 Interactive mode option B (LFG dispatch), Step 3 (fixer subagent), Step 4 (artifacts).
  • plugins/compound-engineering/skills/ce-code-review/references/walkthrough.md — per-finding presentation, action options, LFG the rest exit path, end-of-walk-through dispatch, unified completion report.
  • plugins/compound-engineering/skills/ce-code-review/references/bulk-preview.md — three call sites today (B / C / walk-through LFG the rest); narrowing to one (C only).
  • plugins/compound-engineering/skills/ce-code-review/references/subagent-template.md — persona output contract; suggested_fix guidance is currently permissive ("a bad suggestion is worse than none"); autofix_class decision guide at lines 137-142 documents manual as "actionable work that requires design decisions or cross-cutting changes."
  • plugins/compound-engineering/skills/ce-code-review/references/tracker-defer.md — Interactive and Non-interactive execution modes; the post-run failure-handling question reuses the existing Interactive flow on the leftover set.
  • plugins/compound-engineering/skills/ce-code-review/references/findings-schema.json — suggested_fix: ["string", "null"] with description "Concrete minimal fix. Omit or null if no good fix is obvious — a bad suggestion is worse than none."

Institutional Learnings

  • docs/plans/2026-04-21-002-refactor-ce-code-review-precision-and-validation-plan.md — added Stage 5b as a per-finding validation gate before externalization. R3 of that plan scoped Stage 5b to externalizing modes only; this plan further narrows by removing it from the LFG path specifically. Stage 5b's intent (validate before externalizing to a tracker) still holds for option C.
  • docs/plans/2026-04-17-002-feat-ce-review-interactive-judgment-plan.md — introduced the Interactive mode routing question, walk-through, and bulk-preview design.
  • docs/solutions/skill-design/confidence-anchored-scoring-2026-04-21.md — anchored 0/25/50/75/100 rubric; this plan does not change the rubric.

External References

None — this is an internal skill behavior change.

Key Technical Decisions

  • Push personas instead of bolting on a synthesis-time proposal pass. Architecturally simpler. The persona has the diff and evidence loaded; second-passing a sub-agent to re-derive what the persona could have committed to is wasted work. Synthesis stays a routing function rather than a reasoning function.
  • suggested_fix becomes the authoritative signal for "agent can fix this," not autofix_class. Today's logic infers the recommended action from autofix_class alone (which is the persona's recommendation about handling, not about fix-availability). Going forward, presence of suggested_fix on a manual finding upgrades the recommended action to Apply; absence keeps it at Defer. autofix_class itself is not collapsed — the report still surfaces what the persona thought.
  • Drop Stage 5b on LFG, not "fold it into the fixer." The fixer naturally re-checks each finding when applying the fix (or proposing one for manual). Running a separate validator subagent per finding before the fixer dispatches is duplicate research. Failure to apply or false-positive recognition surfaces during the fix attempt itself.
  • No P0 carve-out for "preview-before-apply." LFG is opt-in to "agent's best judgment." Reverting a wrong P0 fix on uncommitted edits is cheaper than the cognitive overhead of re-routing some severities through preview and others not. Consistency is more valuable than the marginal safety floor.
  • Single fixer pass. Today's max_rounds: 2 re-review loop is for the safe_auto queue. LFG runs once, summarizes, exits. Re-review is heavy and the user can re-invoke /ce-code-review if a follow-up pass is wanted.
  • Post-run failure-handling question scoped to the failed set only. When the failed bucket is empty (everything got applied or acknowledged), no question fires — the unified completion report is the terminal output. When non-empty, options are: file tickets for leftovers (when sink available), walk through them, or ignore. Mirrors tracker-defer.md's sink-availability rules.

Open Questions

Resolved During Planning

  • Should walk-through option A also flip to Apply for manual + suggested_fix? Yes — the user confirmed flipping option A consistently. R2's action-derivation change is the single source of truth; option A reads the recommended action from synthesis, so the flip happens automatically without separate walkthrough.md logic.
  • Should the fixer be expanded to "propose then apply" for manual-without-suggested_fix items? No. Push the proposal upstream to the persona instead. If the persona could not commit to a fix, the finding stays in Defer territory. The user explicitly chose this architecture over a synthesis-time second pass.
  • Should P0 findings always preview, even under LFG? No. The user agreed dropping the carve-out. Consistency over marginal safety; revert is the audit surface.
  • Should Stage 5b stay on the LFG path? No. The user chose to drop it on LFG specifically. Stage 5b stays on autofix, headless, file-tickets (option C), and the walk-through tracker-defer handoff.

Deferred to Implementation

  • Exact prompt wording in the subagent template for the stronger suggested_fix expectation. The implementer should write language that pushes for a fix without inflating false-positive risk — the existing false-positive catalog (lines 115-126 of subagent-template.md) stays as the suppression backstop. Calibration will need iteration during testing.
  • The fixer's failure-reason taxonomy. R5 says the fixer reports a one-line reason per failed item. The exact phrasing (e.g., "fix did not apply cleanly", "needs cross-team input", "finding looks false on closer review") affects how the post-run question framing reads. Pick during implementation; the user already noted these reasons should be specific enough that the post-run prompt is meaningful.
  • Whether to preserve the old LFG the rest Cancel-to-current-finding semantic without the bulk-preview. The walk-through LFG the rest previously cancelled back to the current finding from the bulk-preview. Without a preview, "LFG the rest" just dispatches. The implementer should confirm with the user whether LFG the rest itself should now have a soft "are you sure?" gate, or whether it should fully match option B's no-gate model. Default to the latter (full match) unless review surfaces a real concern.
  • Operational definition of "evidence no longer matches code" in U4's intent re-check. Options: (a) all identifiers cited in evidence strings still appear at the cited file:line; (b) line-content hash comparison within an edit-distance threshold; (c) light AST-level comparison; (d) leaver-it-to-the-fixer's-judgment with a clear prompt. Pick during implementation. The vagueness is acceptable here because the fixer is an LLM agent making a judgment call — over-specifying the rule risks brittleness — but the implementer should commit to a clear operational definition (and a reason phrase taxonomy) before writing the prompt.

Implementation Units

  • U1. Strengthen persona suggested_fix expectation in subagent template

Goal: Push reviewer personas to attempt a suggested_fix whenever defensible from the diff and surrounding code, while preserving the existing false-positive backstop. Reserve manual without suggested_fix for findings that genuinely require cross-team input, business context, or research outside the review.

Requirements: R1.

Dependencies: None.

Files:

  • Modify: plugins/compound-engineering/skills/ce-code-review/references/subagent-template.md
  • Modify: plugins/compound-engineering/skills/ce-code-review/references/findings-schema.json (description tightening on suggested_fix; field stays ["string", "null"])

Approach:

  • Replace the current "suggested_fix is optional. Only include it when the fix is obvious and correct. A bad suggestion is worse than none." line with stronger guidance: when a fix is reachable from the diff and surrounding code, propose one. Only omit when the finding genuinely needs cross-team alignment, business context, or research outside this review.
  • Update the autofix_class decision guide to note that manual should be paired with suggested_fix whenever the persona can defend a fix from context, and that omitting suggested_fix on manual is a signal the finding genuinely needs handoff.
  • Tighten the schema's suggested_fix description to match: emphasize that omission means "I genuinely cannot propose a fix from review context," not "I'd rather not commit."
  • Preserve the existing false-positive catalog (lines 115-126 of subagent-template.md) unchanged. The catalog is the suppression backstop; this change only affects how non-suppressed manual findings present.

Patterns to follow: The existing decision guide format in subagent-template.md lines 137-142. Match the imperative voice and concrete-example style.

Test scenarios:

  • Happy path: A persona reviewing a diff with a clear ownership-check gap produces autofix_class: manual with a concrete suggested_fix referencing the existing pattern in a sibling controller.
  • Edge case: A persona reviewing a diff that adds an unbounded query produces autofix_class: manual with NO suggested_fix because pagination strategy depends on call-site knowledge the reviewer does not have.
  • Edge case: The false-positive catalog still suppresses lint-comment-suppressed findings even when a fix would be reachable.
  • Integration: Updated subagent template is loaded by Stage 4 spawning logic and the change reaches the persona prompts (no breakage in template variable substitution).

Verification: Spot-check the updated template against the existing persona files in plugins/compound-engineering/agents/ to confirm none rely on the old suggested_fix is optional phrasing in a way that contradicts the strengthened guidance. The schema validation in tests should still pass with the description-only change.

  • U2. Update Stage 5 action-derivation rule for manual + suggested_fix

Goal: Make the recommended-action mapping read suggested_fix as the authoritative signal for "agent can apply this." manual + suggested_fix recommends Apply; manual without suggested_fix recommends Defer. Walk-through option A and the LFG path both inherit this through the existing recommended-action surface.

Requirements: R2, R7.

Dependencies: None (parallel with U1; U1 is what makes the Apply path actually fire often).

Files:

  • Modify: plugins/compound-engineering/skills/ce-code-review/SKILL.md (Stage 5 step 6b, and any other location that documents the recommended-action mapping)

Approach:

  • Update step 6b to specify the recommended-action mapping explicitly:
    • safe_auto -> Apply (already auto-applied before the routing question; not surfaced to LFG)
    • gated_auto with suggested_fix -> Apply
    • manual with suggested_fix -> Apply (was Defer)
    • manual without suggested_fix -> Defer
    • advisory -> Acknowledge
  • The cross-reviewer tie-break order (Skip > Defer > Apply > Acknowledge) is unchanged. It only fires when reviewers disagree on action; the per-finding mapping above is the single-reviewer default.
  • Update any prose elsewhere in SKILL.md that asserts "manual implies Defer" without qualifying for suggested_fix presence.

Patterns to follow: The existing step 6b structure. Keep the same mapping-table/prose hybrid format.

Test scenarios:

  • Happy path: A manual finding with suggested_fix populated maps to recommended Apply in the synthesized findings table.
  • Happy path: A manual finding with suggested_fix: null maps to recommended Defer.
  • Edge case: A manual finding with suggested_fix: "" (empty string) is treated as "no suggestion" and maps to Defer.
  • Integration: Walk-through option A's terminal block correctly renders the proposed fix when the upgraded recommendation fires.

Verification: Trace a sample synthesized finding set through the Stage 5/6 flow on paper — manual + suggested_fix items should appear in the Apply-recommended bucket in the Stage 6 findings table, and the walk-through stem should phrase as "Apply the …?" rather than "Defer …?".

  • U3. Drop Stage 5b and bulk-preview gates on the LFG path

Goal: Remove the validator-pre-pass and approval-prompt gates from interactive option B and walk-through LFG the rest. Other paths (autofix, headless, option C, walk-through tracker-defer handoff) keep Stage 5b.

Requirements: R3, R4, R8.

Dependencies: U1, U2 (the gate removal is only safe once manual findings come in already classified by suggested_fix).

Files:

  • Modify: plugins/compound-engineering/skills/ce-code-review/SKILL.md (Stage 5b conditional table; Step 2 Interactive mode option B handler)
  • Modify: plugins/compound-engineering/skills/ce-code-review/references/bulk-preview.md (narrow scope to option C only)
  • Modify: plugins/compound-engineering/skills/ce-code-review/references/walkthrough.md (remove the LFG the rest -> bulk-preview path; update Cancel semantics)

Approach:

  • Stage 5b conditional table: change interactive, LFG routing (option B) and interactive, walk-through routing (option A) — LFG-the-rest handoff rows from "Yes" to "No". Keep all other rows unchanged.
  • Step 2 Interactive option B handler: rewrite to dispatch the fixer immediately on the full pending action set (gated_auto + manual + advisory). Remove the "first run Stage 5b validation" preamble and the "load references/bulk-preview.md" instruction.
  • bulk-preview.md: narrow the "When the preview fires" section to option C only. Remove call sites 1 and 3 (routing option B and walk-through LFG the rest). Adjust the Scope summary wording, Cancel semantics, and edge cases sections to match the single remaining caller.
  • walkthrough.md LFG the rest flow: replace the bulk-preview dispatch with direct fixer dispatch on the (current finding + remaining undecided) set. Remove the "Cancel returns to current finding" semantic — there is no preview to cancel from. Update the unified completion report description if it referenced bulk-preview-specific behavior.

Patterns to follow: The existing autofix and headless dispatch patterns in Step 2 — they already dispatch the fixer without preview. Mirror that shape for option B.

Test scenarios:

  • Happy path: User selects option B with 8 pending findings; the fixer is dispatched immediately, no Proceed/Cancel prompt fires.
  • Edge case: User is in walk-through option A with 3 findings answered, picks LFG the rest with 5 remaining; the fixer is dispatched on those 5 plus the current finding, no preview fires.
  • Edge case: User selects option C with 8 pending findings; Stage 5b runs as before, bulk-preview renders, Proceed/Cancel fires (option C unchanged).
  • Integration: Stage 5b conditional table parsing in tests still resolves correctly for non-LFG paths.

Verification: Read through Step 2 option B and walk-through LFG the rest in the updated SKILL.md and walkthrough.md as if running them — the path from "user picks LFG" to "fixer dispatches" should have zero blocking prompts. Option C should still show a preview.

  • U4. Expand fixer subagent contract for the LFG heterogeneous queue

Goal: The fixer accepts a queue containing gated_auto (with suggested_fix), manual (with suggested_fix), and advisory items. It applies items with concrete fixes, no-ops on advisory, and routes items where the fix cannot be applied cleanly to a failed bucket with a one-line reason. Returns the partition {applied, failed, advisory} to the orchestrator.

Requirements: R5, R6.

Dependencies: U3 (the fixer is only invoked through this expanded contract on the LFG path; option C still files tickets without invoking the fixer).

Files:

  • Modify: plugins/compound-engineering/skills/ce-code-review/SKILL.md (Step 3 fixer subagent description, including the queue contract and return shape)

Approach:

  • Document the heterogeneous queue contract in Step 3: each item carries its autofix_class, suggested_fix, severity, file:line, title, why_it_matters, and evidence. The fixer iterates and:
    • For items with suggested_fix: attempt to apply the fix. On clean application, add to applied. On failure (line moved, conflicting edit, syntax issue, or any other apply-time failure), add to failed with a reason.
    • For advisory: no-op; add to advisory (recorded as acknowledged).
    • For items without suggested_fix that somehow reach the queue: route to failed with reason "no fix proposed by reviewer." This should be rare under the new flow but is a safety net.
  • The fixer also performs a light intent re-check before applying — confirm the cited code at the cited file:line still resembles the persona's evidence. If it has substantially changed (the diff already moved on, the line was deleted, etc.), route to failed with reason "evidence no longer matches code."
  • Return shape: structured object listing each finding with its outcome and reason. Orchestrator assembles the unified completion report from this and computes whether the post-run question fires (R6).
  • Single pass — no max_rounds: 2 re-review loop on the LFG path. After the fixer returns, emit the report.
  • requires_verification: true items still trigger targeted verification after apply, same as today's safe_auto fixer.

Patterns to follow: The existing Step 3 contract for the safe_auto fixer. Extend rather than rewrite.

Test scenarios:

  • Happy path: Queue of 5 gated_auto and 3 manual items, all with suggested_fix; fixer applies all 8, returns applied: 8, failed: 0, advisory: 0.
  • Edge case: Queue includes 1 advisory item; fixer no-ops, returns it under advisory, not failed.
  • Edge case: One item's suggested_fix cannot apply cleanly because the cited line was already changed by an earlier fix in the same pass; fixer routes that item to failed with reason "evidence no longer matches code" or "fix did not apply cleanly."
  • Edge case: Queue contains a manual finding without suggested_fix (defensive case); fixer routes to failed with reason "no fix proposed by reviewer."
  • Error path: Fixer subagent dies mid-pass (timeout, dispatch error); orchestrator captures partial state — items marked applied stay applied, everything else routes to failed with reason "fixer dispatch failed."
  • Integration: requires_verification: true items run their targeted verification after apply; failure to verify routes the item to failed with reason naming the failed verification.

Verification: Trace a synthetic queue through the new contract on paper. Check that the returned partition cleanly drives the unified completion report and that the post-run question fires only when failed is non-empty.

  • U5. Add post-run failure-handling question on LFG path

Goal: After the fixer returns, when the failed bucket is non-empty, fire one question via the platform's blocking question tool with three options: file tickets for the leftovers, walk through them one at a time, or ignore. Sink-availability rules from tracker-defer.md govern whether the file-tickets option appears.

Requirements: R6.

Dependencies: U4 (the question is gated on the fixer's failed return).

Files:

  • Modify: plugins/compound-engineering/skills/ce-code-review/SKILL.md (Step 2 Interactive mode option B handler, after the fixer return)

Approach:

  • After the fixer returns and the unified completion report has been assembled, check whether failed is non-empty. If empty, emit the report and fall through to Step 5 per the existing fixes_applied_count gating rule.
  • If non-empty, fire one question via the platform's blocking question tool (AskUserQuestion in Claude Code with the existing pre-load step; request_user_input in Codex; ask_user in Gemini; ask_user in Pi via pi-ask-user). Stem: N findings could not be auto-resolved. What should the agent do with them?
  • Options:
    • File tickets for these (when any_sink_available: true from cached tracker-detection tuple)
    • Walk through these one at a time
    • Ignore — leave them in the report
  • Sink-omission: when any_sink_available: false, omit the file-tickets option and append one line to the stem explaining why (mirrors the routing-question stem behavior at top of Step 2 today).
  • Dispatch on selection:
    • File tickets: route the failed set through tracker-defer.md Interactive mode (the existing flow used by option C). No bulk-preview here — failures already happened, this is just durable filing.
    • Walk through: re-enter the walk-through loop scoped to the failed set. Each finding's recommended action is recomputed via the standard rule from U2:
      • Items that have a suggested_fix (the fix existed but did not apply cleanly, or evidence-match check failed): recommend Apply. If the user picks Apply, the finding joins the in-memory Apply set and the standard end-of-walk-through fixer dispatch handles it. This is a focused fixer pass on a small Apply set — distinct from the original single-pass LFG dispatch and not a violation of LFG's single-pass guarantee.
      • Items without a suggested_fix (no fix proposed, or filtered as false positive): recommend Defer. Apply is not offered for these; the menu is Defer / Skip / LFG the rest. This re-entry path uses the existing per-finding question logic in walkthrough.md — no new walk-through behavior, just a different scope set.
    • Ignore: emit the unified completion report including the failed list under a "Could not resolve" section.
  • Numbered-list fallback when the harness lacks a blocking tool, following the same conventions as the rest of the skill.

Patterns to follow: The existing routing-question handler at the top of Step 2 Interactive mode. Use the same option-label discipline (third-person voice, self-contained labels, label-by-letter dispatch when label varies by tracker confidence).

Test scenarios:

  • Happy path: Failed set has 3 items, tracker available; question fires with 3 options including "File tickets for these"; user picks "File tickets," tracker-defer fires for each.
  • Edge case: Failed set is empty; no question fires; report emitted, Step 5 gating logic handles the rest.
  • Edge case: Failed set has 5 items, no tracker sink available; question fires with only "Walk through" and "Ignore"; stem includes "no tracker sink detected."
  • Edge case: User picks "Walk through" with 5 failed items; walk-through re-enters scoped to those 5, each presented with the standard per-finding flow.
  • Integration: When the user picks "File tickets," Stage 5b does not re-run on the failed set — the fixer already attempted them and their state is the input to ticket composition.
  • Cross-platform: On Codex without request_user_input available, the numbered-list fallback fires correctly.

Verification: Walk the new question's three branches end-to-end on paper. Confirm sink-omission behavior matches tracker-defer.md's existing Interactive-mode contract. Confirm the question never fires when the failed set is empty.

  • U6. Update tests and fixtures to match the new LFG flow contract

Goal: Update tests/review-skill-contract.test.ts and tests/pipeline-review-contract.test.ts so they assert the new LFG behavior — fixer dispatch on selection, no bulk-preview for option B, no Stage 5b on LFG, post-run question on failed set — and remove assertions that pinned the removed gates.

Requirements: R9.

Dependencies: U3, U4, U5 (tests cover the contract these units define).

Files:

  • Modify: tests/review-skill-contract.test.ts
  • Modify: tests/pipeline-review-contract.test.ts
  • Modify: tests/fixtures/ (any fixture asserting old LFG flow output)

Approach:

  • Identify failing assertions: based on grep results, current asserts include (B) LFG.label format (line 83),references/bulk-preview.mdreference (line 89),Stage 5b validation pass dispatches conditionally(line 278),(B) LFG.*first run Stage 5b validation (line 304), LFG the rest shape in walkthrough.md (line 552 of pipeline-review-contract).
  • For each, decide: rewrite to assert the new contract, or delete because the underlying mechanism is gone.
    • Option B label asserts → keep, with updated label text if the label changes.
    • bulk-preview.md reference under option B → remove; assert that bulk-preview is referenced by option C only.
    • Stage 5b conditional table parsing → keep but update expected rows.
    • (B) ... first run Stage 5b validation → delete and replace with assert that option B does NOT mention Stage 5b.
    • Walkthrough LFG the rest shape → update to assert the new no-preview path.
  • Add new assertions for the post-run failure-handling question (presence of the stem and three options in SKILL.md).
  • Add assertions that references/bulk-preview.md is scoped to option C only after narrowing.
  • Update fixtures if any sample finding sets or expected reports are pinned to the old flow.

Patterns to follow: The existing assertion style in review-skill-contract.test.ts — file content reads, regex matchers for structural elements, named tests describing the contract.

Test scenarios:

  • Happy path: Updated tests pass against the modified SKILL.md, walkthrough.md, and bulk-preview.md.
  • Regression check: Tests for option C, autofix, headless, and walk-through option A continue to pass without modification (only LFG-specific tests change).
  • Integration: bun test passes on the full suite.

Verification: Run bun test tests/review-skill-contract.test.ts tests/pipeline-review-contract.test.ts and confirm the updated suite passes. Run the full bun test suite to confirm no incidental breakage.

System-Wide Impact

  • Interaction graph: The LFG path's dispatch shape changes. Callers that introspect SKILL.md option B handler text (the lfg skill, slfg orchestrator) may have assertions to update. Per tests/review-skill-contract.test.ts line 562-563, lfg references tracker-defer.md directly and explicitly does NOT reference ce-code-review/references/bulk-preview.md — so the bulk-preview narrowing should not break lfg. Verify during U6.
  • Error propagation: New failed bucket from the fixer becomes the trigger for the post-run question. Failures during the new question's tracker-defer dispatch (when the user picks "File tickets") follow the existing Interactive failure path in tracker-defer.md (Retry / Fall back / Convert to Skip).
  • State lifecycle risks: The fixer applies edits without preview gating. On uncommitted local edits the user can revert via git diff; on PR-mode runs (where fixes accumulate locally on the PR branch), the same revert applies until the user pushes. No new persistent state.
  • API surface parity: Other interactive-mode options (A walk-through, C file-tickets, D report-only) are unchanged in dispatch behavior. The bulk-preview reference remains valid for option C. Tracker-defer remains valid for both option C and the new post-run question.
  • Integration coverage: Tests covering the heterogeneous fixer queue, the post-run question gating on failed non-empty, and sink-availability behavior on the post-run question are net-new and need real coverage in U6.
  • Unchanged invariants: The 5-anchor confidence rubric, Stage 5 dedup/cross-reviewer-promotion/mode-aware demotion, the protected-artifact rules, the safe_auto auto-apply pre-pass before the routing question, autofix and headless mode behavior, report-only mode behavior, and Step 5 push/PR options are all unchanged.

Risks & Dependencies

Risk Mitigation
Persona-side strengthening (U1) overshoots and personas start proposing weak fixes that the fixer applies. Keep the false-positive catalog unchanged — it's the suppression backstop. The fixer's intent re-check (U4) is a second backstop. Calibrate prompt wording with real review runs after rollout.
Heterogeneous fixer queue (U4) has subtle ordering issues — applying one fix invalidates another's cited line. The fixer's "evidence no longer matches code" check (U4) routes invalidated items to failed rather than mis-applying. Single-pass execution avoids re-cascading edits. Order findings by file + line descending (already today's behavior) so earlier fixes do not shift line numbers for later ones in the same file.
Test churn from removing Stage 5b on LFG creates incidental breakage in lfg/slfg callers. U6 explicitly verifies lfg skill content is unchanged and that pipeline-review-contract tests are updated alongside. The lfg skill already uses tracker-defer.md directly per existing tests.
Without preview, a wrong fix lands silently and the user does not notice during a busy review. The unified completion report enumerates every applied fix with file:line. The diff is the audit surface. The user is expected to skim the report and git diff after LFG. This is the explicit trade the user is making.
Walk-through LFG the rest users lose the "I changed my mind" Cancel path that bulk-preview offered. Stop-the-loop semantics still exist — the user can interrupt the agent. The Cancel path was a soft escape that only fired in a narrow window. If user feedback shows the loss matters, a future iteration can add a one-line confirmation before LFG-the-rest dispatches. Open question carried in "Deferred to Implementation."

Documentation / Operational Notes

  • Update plugins/compound-engineering/README.md only if a user-visible feature description references the bulk-preview gate or the Stage 5b behavior under LFG. Most plugin README content is at the agent/skill-name level and is unaffected.
  • No release-version bumps in this plan — release-please owns versioning. Per plugins/compound-engineering/AGENTS.md, do not hand-edit .claude-plugin/plugin.json or marketplace files.
  • Run bun run release:validate after implementation to confirm no parity drift across Claude/Cursor/Codex manifests.
  • Capture a docs/solutions/skill-design/ entry post-merge if this fix surfaces a learning about persona-side-vs-synthesis-side responsibility for fix-shape commitment. Likely worth documenting as it shaped the architectural choice for U1.

Sources & References

  • Origin context: this conversation's exchange between user and agent on 2026-04-25 covering the LFG defer-bias diagnosis and the "just go, summarize, ask about leftovers" redesign.
  • Prior plan: docs/plans/2026-04-21-002-refactor-ce-code-review-precision-and-validation-plan.md (introduced Stage 5b).
  • Prior plan: docs/plans/2026-04-17-002-feat-ce-review-interactive-judgment-plan.md (introduced the routing question, walk-through, and bulk-preview).
  • Skill files: plugins/compound-engineering/skills/ce-code-review/SKILL.md, references/walkthrough.md, references/bulk-preview.md, references/subagent-template.md, references/findings-schema.json, references/tracker-defer.md.
  • Test files: tests/review-skill-contract.test.ts, tests/pipeline-review-contract.test.ts.
Reference in New Issue View Git Blame Copy Permalink