mirror of
https://github.com/EveryInc/compound-engineering-plugin.git
synced 2026-06-26 12:23:01 +02:00
Address PR review feedback (#590)
- Align Stage 5 tie-break cross-reference in bulk-preview.md with SKILL.md's canonical `Skip > Defer > Apply > Acknowledge` ordering (was missing `Acknowledge`). - Reword the zero-remaining completion summary so it doesn't imply advisory/pre-existing findings are cleared. Unqualified `All findings resolved` now applies only when no advisory/pre-existing remain; otherwise use the qualified `All actionable findings resolved ... (K advisory, J pre-existing remain in the report.)` form. Updated in SKILL.md Step 2 Interactive and walkthrough.md's zero-findings degenerate case. - Scope the Step 2 `Verify question-tool pre-load (checklist)` bullet to Claude Code only. Codex (request_user_input) and Gemini (ask_user) don't need a ToolSearch pre-load step; the rephrased bullet says so explicitly so converted targets aren't blocked. - Add an explicit no-sink Defer→Skip remap in walkthrough.md. When `any_sink_available: false` removes Defer from the menu, Stage 5 step 7b can still produce a Defer recommendation; the remap ensures the `(recommended)` marker always lands on a visible option. The downgrade is surfaced on the R15 conflict context line. Mirrors the shape bulk-preview.md already uses for LFG previews.
This commit is contained in:
Trevin Chow
@@ -623,9 +623,9 @@ After presenting findings and verdict (Stage 6), route the next steps by mode. R
|
||||
**Interactive mode**
|
||||
|
||||
- Apply `safe_auto -> review-fixer` findings automatically without asking. These are safe by definition.
|
||||
- **Zero-remaining case:** if no `gated_auto` or `manual` findings remain after the `safe_auto` pass, skip the routing question entirely. Emit a one-line completion summary (e.g., `All findings resolved — N safe_auto fixes applied.`) followed by the existing end-of-review verdict, then proceed to Step 5 per the gating rule there.
|
||||
- **Zero-remaining case:** if no `gated_auto` or `manual` findings remain after the `safe_auto` pass, skip the routing question entirely. Emit a one-line completion summary phrased so advisory and pre-existing findings (which are not handled by this flow) are not implied to be cleared. When no advisory or pre-existing findings remain in the report, `All findings resolved — N safe_auto fixes applied.` is accurate. When advisory and/or pre-existing findings do remain, use the qualified form `All actionable findings resolved — N safe_auto fixes applied. (K advisory, J pre-existing findings remain in the report.)`, omitting any zero-count clause. Follow the summary with the existing end-of-review verdict, then proceed to Step 5 per the gating rule there.
|
||||
- **Tracker pre-detection:** before rendering the routing question, consult `references/tracker-defer.md` for the session's tracker tuple `{ tracker_name, confidence, named_sink_available, any_sink_available }`. The probe runs at most once per session and is cached for the rest of the run. `named_sink_available` drives the option C label (inline tracker name only when the named sink can actually be invoked). `any_sink_available` drives whether option C is offered at all (it can still be offered when the named tracker is unreachable but `gh` or the harness task primitive works).
|
||||
- **Verify question-tool pre-load (checklist).** Before firing the routing question, confirm `AskUserQuestion` is loaded (per Interactive mode rules at the top of this skill). If not yet loaded this session, call `ToolSearch` with query `select:AskUserQuestion` now. Do not proceed to the routing question without this verification. Rendering the question as narrative text is a bug, not a valid fallback.
|
||||
- **Verify question-tool pre-load (checklist, Claude Code only).** Before firing the routing question in Claude Code, confirm `AskUserQuestion` is loaded (per Interactive mode rules at the top of this skill). If not yet loaded this session, call `ToolSearch` with query `select:AskUserQuestion` now. Do not proceed to the routing question without this verification. Rendering the question as narrative text is a bug, not a valid fallback. On Codex (`request_user_input`) and Gemini (`ask_user`) this checklist does not apply — the platform-native question tool is loaded by default and there is no `ToolSearch` preload step to perform.
|
||||
- **Routing question.** Ask using the platform's blocking question tool (`AskUserQuestion` in Claude Code, `request_user_input` in Codex, `ask_user` in Gemini). Stem: `What should the agent do with the remaining N findings?` — third-person voice per `plugins/compound-engineering/AGENTS.md:127`. Options:
|
||||
|
||||
```
|
||||
|
||||
@@ -128,5 +128,5 @@ Failure during `Proceed` (e.g., ticket creation fails for one finding during a b
|
||||
- **Zero findings in a bucket:** omit the bucket header. A preview with only Apply and Skip does not show an empty `Filing tickets (0):` or `Acknowledging (0):` line.
|
||||
- **All findings in one bucket:** preview still shows the bucket header; Proceed / Cancel still offered. This is the common case for routing option C (every finding under `Filing tickets`).
|
||||
- **N=1 preview (only one finding in scope):** the preview still uses the grouped format, just with a single-line bucket. `Proceed` / `Cancel` still apply.
|
||||
- **No tracker available:** option C is not offered upstream (see `tracker-defer.md` no-sink handling). LFG (option B) and walk-through `LFG the rest` can still run — they may contain per-finding Defer recommendations from Stage 5. Before rendering any LFG-shaped preview, downgrade every Defer recommendation to Skip when the session's cached `any_sink_available` is false, and surface the downgrade on the preview itself (e.g., a `Skipping — defer sink unavailable (N):` bucket, or a note in the header: `N Defer recommendations downgraded to Skip — no tracker sink`). This is a preview-time runtime step, not Stage 5 tie-breaking — step 7b only orders conflicting reviewer recommendations (Skip > Defer > Apply) and has no knowledge of sink availability.
|
||||
- **No tracker available:** option C is not offered upstream (see `tracker-defer.md` no-sink handling). LFG (option B) and walk-through `LFG the rest` can still run — they may contain per-finding Defer recommendations from Stage 5. Before rendering any LFG-shaped preview, downgrade every Defer recommendation to Skip when the session's cached `any_sink_available` is false, and surface the downgrade on the preview itself (e.g., a `Skipping — defer sink unavailable (N):` bucket, or a note in the header: `N Defer recommendations downgraded to Skip — no tracker sink`). This is a preview-time runtime step, not Stage 5 tie-breaking — step 7b only orders conflicting reviewer recommendations (`Skip > Defer > Apply > Acknowledge`, as defined in `SKILL.md` Stage 5 step 7b) and has no knowledge of sink availability.
|
||||
- **Walk-through `LFG the rest` with zero remaining findings:** the walk-through's own logic suppresses `LFG the rest` as an option when N=1 and otherwise, so the preview should never be invoked with zero remaining findings. If it is, render `LFG plan — 0 remaining findings` and fall through to Proceed with no-op.
|
||||
|
||||
@@ -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, GitHub Issues, harness primitive — is unreachable), option B (`Defer`) is omitted. The stem appends one line explaining why (e.g., `Defer unavailable on this platform — no tracker or task-tracking primitive detected.`). The menu shows three options: Apply / Skip / LFG the rest (and Acknowledge in place of Apply for advisory-only findings).
|
||||
- **No-sink (Defer option unavailable):** when the tracker-detection tuple reports `any_sink_available: false` (every tier in the fallback chain — named tracker, GitHub Issues, harness primitive — is unreachable), option B (`Defer`) is omitted. The stem appends one line explaining why (e.g., `Defer unavailable on this platform — no tracker or task-tracking primitive 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.
|
||||
- **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.
|
||||
@@ -221,7 +221,9 @@ The report appears after all execution completes. Ordering inside the report: fa
|
||||
|
||||
### Zero-findings degenerate case
|
||||
|
||||
When the routing question was skipped because no `gated_auto` / `manual` findings remained after `safe_auto`, the completion report collapses to its summary-counts + verdict form with one added line — the count of `safe_auto` fixes applied. Example:
|
||||
When the routing question was skipped because no `gated_auto` / `manual` findings remained after `safe_auto`, the completion report collapses to its summary-counts + verdict form with one added line — the count of `safe_auto` fixes applied. The summary wording mirrors `SKILL.md` Step 2 Interactive mode's zero-remaining case: the unqualified `All findings resolved` form is only accurate when no advisory or pre-existing findings remain. When advisory and/or pre-existing findings remain in the report, use the qualified form that names what was cleared and names what still remains. Examples:
|
||||
|
||||
No remaining advisory or pre-existing findings:
|
||||
|
||||
```
|
||||
All findings resolved — 3 safe_auto fixes applied.
|
||||
@@ -229,6 +231,14 @@ All findings resolved — 3 safe_auto fixes applied.
|
||||
Verdict: Ready with fixes.
|
||||
```
|
||||
|
||||
Advisory and/or pre-existing findings remain in the report:
|
||||
|
||||
```
|
||||
All actionable findings resolved — 3 safe_auto fixes applied. (2 advisory, 1 pre-existing findings remain in the report.)
|
||||
|
||||
Verdict: Ready with fixes.
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Execution posture
|
||||
|
||||
Reference in New Issue
Block a user