Skip to content

fix(review): gate headless fanout fix behind explicit --yes opt-in (#435)#638

Merged
kyle-sexton merged 2 commits into
mainfrom
fix/435-review-fanout-headless-confirmation-gate
Jul 20, 2026
Merged

fix(review): gate headless fanout fix behind explicit --yes opt-in (#435)#638
kyle-sexton merged 2 commits into
mainfrom
fix/435-review-fanout-headless-confirmation-gate

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Summary

The review plugin's fanout fix action is the only fanout mode that mutates the working tree, and its Step-3 confirmation gate self-downgraded in non-interactive sessions — fix-pass-mode.md read "interactive sessions; non-interactive sessions proceed without the gate." A headless /review:fanout fix therefore applied correctness- and cleanup-class fixes with no confirmation at all: the fix argument alone became the entire gate in exactly the unattended context where a human check matters most. Low severity (local, reversible working-tree write — not an outward artifact), but it violates the work-readiness sweep criterion that every side-effect path be explicitly gated rather than a gate that self-downgrades unattended.

Fix

Option chosen: explicit --yes / -y headless opt-in flag (not a hard refusal).

This dispatch's stated default was "Option 1 — refuse fix mode non-interactively (mirror wayfind)." I chose a different option because it is the operator-accepted decision already recorded on #435, which supersedes the dispatch default (the dispatch licensed exactly this — "unless you find during investigation that this repo's actual conventions clearly point the other way"):

  • Operator decision, 2026-07-19"ACCEPTED — the brief's RECOMMENDED option verbatim: explicit headless opt-in flag. Headless fix without --yes prints the classification plan and STOPs; with --yes it proceeds and writes the applied plan to the findings directory. Start strict; loosening later is additive."
  • A subsequent T2 implementation spec on the issue set the acceptance criteria this PR implements.

A hard refusal (dispatch Option 1) would have deleted the autonomous-lane capability the repo is actively building toward; the flag keeps it behind informed opt-in. The flag mirrors the ai-briefing:generate precedent verbatim (--yes / -y, "Skip the pre-execution confirmation gate. Required for headless runs."). Behavior:

Session --yes Behavior
Interactive absent Emit plan, confirm, apply — unchanged
Interactive present Emit plan, skip prompt, apply
Non-interactive (CLAUDE_CODE_REMOTE, claude -p, autonomous loop) absent Emit plan, STOP, mutate nothing (plan is the report)
Non-interactive present Apply, then write a durable applied-plan record

Non-interactive detection reuses wayfind's existing convention (CLAUDE_CODE_REMOTE, claude -p, an autonomous loop) — no new mechanism invented. The applied-plan record lands in the branch findings directory as <UTC-timestamp>-fix-pass-applied.md with type: fix-pass-record frontmatter — deliberately not review-findings, so the fix-pass locator (Step 1) skips it and never re-consumes it as findings, reusing the same frontmatter fence that already skips quality-gate reports.

Two implementation choices the T2 spec flagged vetoable-before-merge (implemented as spec'd; a maintainer may override):

  • The -y alias (the operator wrote only --yes; -y matches ai-briefing verbatim).
  • The applied-plan record filename/shape (<UTC-timestamp>-fix-pass-applied.md, type: fix-pass-record) — inferred from the accepted "log the applied plan for after-the-fact review."

Changes: fanout/SKILL.md (argument-hint, new ## Arguments flag table, Step-0 flag parsing), fanout/context/fix-pass-mode.md (Step-3 gate table replacing the silent waiver; Step-5 applied-plan record), two new evals, version bump 0.14.30.14.4, CHANGELOG entry.

Verification

  • JSON + eval integrity: node -e over the edited evals.json — 22 evals, ids 1..22 contiguous, zero duplicates; plugin.json version resolves to 0.14.4.
  • Contract validators: scripts/validate-plugin-contracts.mjs"Plugin contracts validated: 33 setup skills and 1761 plugin files checked." scripts/validate-plugins.sh"All plugin manifests and the catalog validated." (all 33 plugins + catalog ✔ Validation passed).
  • Silent-downgrade language removed: grep for "without the gate" / "non-interactive sessions proceed" across plugins/review/ returns only the CHANGELOG, which quotes the old wording as the fixed before-state. The runtime Step-3 text no longer contains it.
  • Behavior trace against the edited Step-3 gate table — the four rows are the exhaustive interactive/headless × flag/no-flag matrix:
    • Interactive, no --yes → confirm before applying (row 1) = criterion 1, unchanged.
    • Non-interactive, no --yes → "STOP after the plan — mutate nothing" (row 3) = criterion 2.
    • Non-interactive, --yes → "Apply, then write the applied-plan record (Step 5)" (row 4) = criterion 3; Step 5 writes type: fix-pass-record which Step 1's locator (type: review-findings only) skips.
  • Router trace: Step 0 parses --yes/-y out before the mode match, so /review:fanout fix --yes routes to fix mode and does not trip the Unknown action diagnostic.

Note: these evals are LLM-graded prompt specs, not executable tests — there is no eval-prompt runner in this repo to quote pass/fail output from, so verification above is the contract-validator output plus a behavior trace of the edited text. No pass/fail output is fabricated.

Closes #435

Related

🤖 Generated with a Claude Code implementation subagent (issue #435)

)

The fanout `fix` action's Step-3 confirmation gate self-downgraded in
non-interactive sessions ("non-interactive sessions proceed without the
gate"), so a headless `/review:fanout fix` mutated the working tree with
no confirmation — the `fix` argument alone became the entire gate in
exactly the unattended context where a human check matters most.

Replace the silent waiver with an explicit `--yes` / `-y` opt-in flag,
mirroring the `ai-briefing:generate` precedent verbatim. Interactive
`fix` is unchanged. Headless `fix` without the flag emits the
classification plan and STOPs (plan is the report); headless `fix` with
the flag applies, then writes a durable `type: fix-pass-record` applied-
plan record into the branch findings directory for after-the-fact review
(the non-`review-findings` type keeps the locator from re-consuming it).

Implements the operator-accepted direction on #435. Adds two evals for
the headless paths; the interactive-confirmation eval is unchanged.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011V31qpAHP3jfs76B9d5Rfo
@kyle-sexton kyle-sexton added the automated Opened by automation. label Jul 20, 2026
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 16s —— View job


PR Review — fix(review): gate headless fanout fix behind explicit --yes opt-in

  • Gather context
  • Read and diff all changed files
  • Review SKILL.md changes
  • Review fix-pass-mode.md changes
  • Review evals.json changes
  • Review plugin.json + CHANGELOG.md
  • Compile and post review findings

Summary

This is a well-scoped, correctness-critical fix. The root issue — the Step-3 gate's silent self-downgrade in non-interactive sessions — is cleanly excised and replaced with a proper 4-row gate table. The implementation is internally consistent (SKILL.md ## Arguments, Step 0 routing, fix-pass-mode.md Step 3, two new evals all agree), and the applied-plan record design is sound. The version bump (patch: 0.14.3 → 0.14.4) is correct semver for a bug fix.

No blocking defects. Three items to call out for maintainer awareness:


Finding 1 — Missing eval for interactive + --yes path (low severity)

Location: evals/evals.json

The gate table introduced in fix-pass-mode.md Step 3 has four rows but only three of them have eval coverage:

Row Coverage
Interactive, no --yes → confirm Eval 18 (existing)
Interactive, --yes → skip prompt, apply ❌ No eval
Non-interactive, no --yes → STOP Eval 21 (new)
Non-interactive, --yes → apply + record Eval 22 (new)

The uncovered path (interactive session + explicit --yes) is the simplest of the four, but its absence means the table isn't exhaustively exercised. The PR description acknowledges the two new evals — it doesn't mention the interactive---yes gap. Not a blocker (the behavior is "skip the prompt and apply," which is low-risk), but worth adding for completeness. Fix this →


Finding 2 — -y alias is vetoable-before-merge (flagged by PR itself)

Location: SKILL.md line 37, fix-pass-mode.md line 37

The PR's own description explicitly flags this as "vetoable-before-merge": the operator's decision on #435 specified only --yes; the implementation author added -y to match the ai-briefing:generate precedent verbatim. Both references are consistent across all files (argument-hint, ## Arguments table, Step 0, Step 3 gate table), so if the alias is kept, no inconsistency exists. If a maintainer wants to drop it, three locations would need updating: argument-hint, the ## Arguments table, and the Step 3 gate prose in fix-pass-mode.md. Calling it out explicitly so no merge-time veto is forgotten.


Finding 3 — Step 5 header doesn't signal the new record-writing branch (cosmetic)

Location: fix-pass-mode.md line 69

## Step 5: Report now has two distinct behaviors depending on the session path: emit counts (always) and persist an applied-plan record (headless apply only). The ## Step 5: Report heading gives no signal that record-writing happens here; a reader scanning headers sees "Report" and might not locate the record-writing spec. The subsection ### Applied-plan record (headless apply only) partially addresses this, but the section header itself could be ## Step 5: Report + applied-plan record to match the scan-level readability of the other steps. Strictly cosmetic — the body is correct as written.


Correctness verification

  • Silent-downgrade language removed: Confirmed. The old "interactive sessions; non-interactive sessions proceed without the gate" is gone from Step 3; the only remaining mention is in the CHANGELOG quoting the before-state.
  • Flag strip before mode match: Correctly specified in Step 0. fix --yes hits the fix route, not the unknown-action diagnostic. Eval 22 expectation ci: onboard to the CI platform (ci-workflows + standards) #4 explicitly tests this.
  • type: fix-pass-recordreview-findings: The Step 1 locator only picks up type: review-findings files. The applied-plan record's deliberate type mismatch prevents Step 1 from ever re-consuming it as findings. Correct by construction.
  • Version bump: 0.14.3 → 0.14.4 (patch) for a behavioral bug fix. Correct semver.
  • Eval contiguity: IDs 1–22, no duplicates. Confirmed from the file.

LGTM on the core fix. The two noteworthy follow-ups are the interactive---yes eval gap (minor, additive) and the maintainer -y veto window.

Address two non-blocking review findings on #638:

- Add eval 23 (fix-pass-interactive-yes-skips-prompt) covering the fourth
  gate-table cell — interactive session + explicit --yes skips the
  confirmation prompt, applies, and writes no applied-plan record (that
  record is headless-apply-only).
- Rename Step 5 header to "Report + applied-plan record" so the header
  signals the record write that now lives in that step.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011V31qpAHP3jfs76B9d5Rfo
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude Code is working…

I'll analyze this and get back to you.

View job run

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Review findings classified and addressed (commit 13d3469):

  • Finding 1 (eval gap) — FIXED. Added eval id 23 fix-pass-interactive-yes-skips-prompt, covering the previously-uncovered fourth gate-table cell (interactive + --yes): asserts no confirmation prompt is shown, fixes are applied, and no applied-plan record is written (that record is headless-apply-only). The gate table's 4 cells now have 4 evals (18 interactive/no-flag, 21 headless/no-flag, 22 headless/--yes, 23 interactive/--yes).
  • Finding 2 (cosmetic) — FIXED. Renamed ## Step 5: Report## Step 5: Report + applied-plan record so the header signals the record write that now lives in that step.
  • Finding 3 (-y alias) — ACKNOWLEDGED, no action. The alias is applied consistently across all locations per the ai-briefing:generate precedent; accepted as-is, nothing to change.

@kyle-sexton
kyle-sexton marked this pull request as ready for review July 20, 2026 07:38
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 48s —— View job


PR Review — fix(review): gate headless fanout fix behind explicit --yes opt-in

  • Gather context and prior review findings
  • Read current state of all changed files
  • Verify Finding 1 fix (eval 23 added correctly)
  • Verify Finding 3 fix (Step 5 header rename)
  • Check internal consistency across SKILL.md, fix-pass-mode.md, evals.json
  • Final review post

Follow-up Review — addressing findings from prior review

The two findings fixed in commit 13d3469 are correctly implemented. Summary of each:

Finding 1 — eval gap (interactive + --yes) ✅ Resolved

Eval 23 (fix-pass-interactive-yes-skips-prompt) covers the previously uncovered fourth gate-table cell exactly as needed. All three expectations are correct and internally consistent with fix-pass-mode.md Step 5:

  • (1) No confirmation prompt shown — matches Step 3 row "Interactive / present → Skip the confirmation prompt and apply"
  • (2) Fixes are applied — correct
  • (3) No applied-plan record written — matches Step 5's "When the apply ran under --yes in a non-interactive session" qualifier and the explicit call-out "Interactive and headless-stop paths write no record"

The gate table's 4 cells now map 1:1 to evals 18, 21, 22, 23. ✅

Finding 3 — Step 5 header rename ✅ Resolved

## Step 5: Report + applied-plan record now signals record-writing at scan level. ✅

Finding 2 — -y alias ✅ Accepted as-is

Alias remains. All three locations (argument-hint, ## Arguments table, Step 3 gate prose) are consistent. No inconsistency to resolve. ✅


Correctness re-verification at HEAD

Check Result
Silent-downgrade language absent from Step 3 ✅ Replaced by explicit 4-row table
Flag strip before mode match (SKILL.md Step 0) ✅ "fix --yes from tripping the diagnostic"
type: fix-pass-recordreview-findings → Step 1 skip ✅ Correct by construction, covered by eval 22 expectation 3
Eval IDs 1–23 contiguous, no duplicates
Version 0.14.4 in plugin.json
CHANGELOG entry accurately describes the change
Step 5 applied-plan record scoped to non-interactive --yes only ✅ Eval 23 expectation 3 confirms interactive writes no record

No new findings. LGTM — all prior findings resolved, no regressions observed. Ready to merge.

@kyle-sexton
kyle-sexton merged commit d6accb3 into main Jul 20, 2026
17 of 18 checks passed
@kyle-sexton
kyle-sexton deleted the fix/435-review-fanout-headless-confirmation-gate branch July 20, 2026 08:36
kyle-sexton added a commit that referenced this pull request Jul 20, 2026
…) (#643)

## Summary

`/work-items:work` selection had no rule excluding an issue that already
has an open linked PR from the pickable frontier. An issue keeps
`status: ready` for its entire window with an open PR (from open through
merge/auto-close), so a picker had to manually cross-check `gh pr list`
to avoid re-picking work already in flight and starting a duplicate
branch. This ships the ratified in-repo open-linked-PR selection filter
(operator decision, 2026-07-19): an issue with an open linked PR is not
pickable.

## Fix

- **Selection-time frontier filter (`skills/work/SKILL.md`, Step 1).**
After `list-frontier` derives the frontier, tiers 2–3 candidates are
additionally filtered to drop any number that has an open PR targeting
it for closure — so an in-flight item leaves the pickable set instead of
being re-picked. The **closing-keyword linkage** is authoritative (the
same `Closes #N` / native-closing-keyword signal `pr-issue-linkage`
enforces); an intentional `Refs #N` opt-out does not exclude its issue.
The filter **fails open** when the bound provider exposes no PR host
(offline `local-markdown` is never a coordination surface and touches no
network tool).
- **New GitHub adapter mechanic
(`tools/work-item-tracker/adapters/github/README.md`, "Open linked
PRs").** The provider mechanics stay in the adapter per this repo's
seam/adapter separation — the skill core inlines no `gh`. It uses `gh pr
list --state open --search "<N> in:body"` (truncation-safe per item, no
page-size race) and a closing-keyword `jq` test with an exact-number
boundary so `#463` does not match `#4630` / `#1463`.
- **Retires the interim heuristic.** The prior "Already-in-flight
(interim, retire on `#463`)" bullet in the execute-step staleness
pre-check is removed — the frontier itself now excludes in-flight items,
which is exactly the retirement that bullet anticipated. Scope-narrowed
to the closing-keyword signal (the interim branch-pattern-only match is
dropped): a standard-flow PR always carries the gate-enforced `Closes
#N`, so keyword-matching is faithful to the ratified "references it as a
closing target" scope.
- **No seam-contract change.** `CONTRACT.md` and `list-frontier` are
untouched; the addition is a README-documented adapter mechanic, not a
new seam verb. The durable seam-level in-review state is explicitly NOT
built here (see Related).

## Verification

The adapter mechanic was exercised against live repo data (read-only).
`<N>` substituted per row:

```
issue #435 open-closing-PR: true    (open, unassigned, status: ready; open PR #638 closes it)
issue #487 open-closing-PR: true    (open, unassigned, status: ready; open PR #629 closes it)
issue #534 open-closing-PR: true    (open, unassigned, status: ready; open PR #634 closes it)
issue #463 open-closing-PR: false   (no open PR closes it → stays pickable)
issue #4630 open-closing-PR: false  (exact-number boundary: does not false-match on #463's PR)
```

Each of #435 / #487 / #534 is a genuine frontier-eligible candidate
(open, unassigned, `status: ready`) that under the old logic stayed
pickable despite an open PR already in flight; the filter returns `true`
for exactly those and excludes them, while #463 (no open closing PR)
returns `false` and remains pickable. `list-frontier` itself was not run
end-to-end here because the `work-items` plugin repo binds no tracker
(`.work-item-tracker.json` is a consuming-repo artifact) — the
per-number filter, which is the added logic, is what is demonstrated
above.

Closes #463

## Related

- #463 — this issue (open-linked-PR selection filter).
- #416 — planning routes through the tracker seam; argues the durable
in-review state belongs at the seam, not a GitHub-only label. This PR
does NOT implement that durable state.
- #498 — seam read-verb coverage; where a durable in-review /
container-scoped read would live. This PR does NOT implement that
durable state.

Note: PR #641 concurrently bumps `work-items` to `0.14.0` (minor, for
#478) and also edits `plugin.json` + `CHANGELOG.md`. This PR is a patch
bump to `0.13.1` from the current `origin/main` base (`0.13.0`) — a
different version slot, not a race — but whichever merges second will
need a mechanical rebase on those two files (the same-plugin concurrency
#464 treats as an awareness note, not a block).

🤖 Generated with a Claude Code implementation subagent (issue #463)

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automated Opened by automation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

review: headless /review:fanout fix mutates the working tree without the confirmation gate

1 participant