fix(fd3,code-review): 2026-09-25 run tuning and headless code review in the implement workflow

[grixu]

Summary

Fixes from an audit of the 2026-09-25 fd3 chain (build-spec → split-to-tasks ×2 → implement) and wires the code-review plugin into the implement workflow through new headless skills.

fd3 — spec stage

• build-spec re-enters at validation when handed a finished spec, re-validates every edit made after the final verdict, places the spec in the repository's layout (never the scratchpad), and relays write-spec questions through AskUserQuestion.
• validate-spec probes run in a scratch worktree without touching dependencies; non-blocking findings get their own check result form; the verdict line records the commit each repository was validated at.
• grill-topic re-checks sized recommendations when an answer widens scope.

fd3 — split stage

• The split never writes a verdict; an unvalidated spec goes back to build-spec.
• The split stops when origin changed files the spec cites since validation.
• A commit sequence the spec binds inside one branch is serialised with edges.
• Branch reuse is asked when the spec names the branch itself; the report is written after the coverage re-run with bare-slug depends-on.

fd3 — implement stage

• A root branch is measured against origin/<default> (diffBase), not the parked branch it starts from (startRef) — the cause of the empty review diff.
• Each branch is reviewed with code-review's headless lenses, then fixes are re-reviewed; each repair's own commits are reviewed and findings go to the human. An empty diff or skipped lens is no-verdict, not clean; a branch with open findings stays merged.
• Workflow agents are pinned to their own step, the baseline worktree is cut detached, CI verdicts come from each command's exit status, behaviour-changing CI failures go to the human, and each repair decision is committed separately.

code-review

• New headless skills cr-prepare, cr-scan, cr-merge for workflow-driven reviews (no questions, no edits).
• Scanner/merge contracts, review setup and fix risk classes moved to shared references; get_changes.py gains -C.

Test plan

• fd3 evals: 8/8 pass, including new split-stale-origin and build-spec-reentry
• code-review eval-20 (headless pipeline) passes
• claude plugin validate on both plugins; workflow scripts parse; get_changes.py tests pass
• implement-run / repair-run review stage exercised against a stubbed agent() (empty diff, delta findings, clean path, review: false)

Not covered: a split-commit-sequence eval (no fixture has two independent elements on one branch).

[pullfrog]

Important

Findings meant for a human can still reach the automatic fixer, or get dropped, in implement-run.js. Two finding classes skip the human gate that the README, CHANGELOG and implement-tasks promise: spec wrong-implementations and boy-scout findings. A third gap lets a branch reach done while a finding the fixer skipped is still open.

Reviewed changes

This is the initial review of the full PR: code-review's headless pipeline, fd3's spec/split fixes, and the review stage wired into both implementation workflows.

• Shared code-review references — /start-cr Steps 2–5 move into review-setup.md, scanner-contract.md and merge-contract.md. The last adds a Fix risk section that maps the apply buckets to safe | structural | report-only.
• Headless skills — cr-prepare, cr-scan and cr-merge split the review into steps a workflow drives through a context directory. None of them asks questions, and an empty change or a missing lens comes back as a status.
• get_changes.py -C — runs as if started in another checkout, with a test.
• fd3 spec stage — build-spec re-enters at validation for a finished spec, validate-spec probes run in a detached scratch worktree, a new non-blocking — Result form is added, and a Checked at: line goes under the verdict.
• fd3 split stage — the split stops when origin/<default> has changed a cited path, never writes a verdict, and turns a commit sequence the spec binds into edges.
• fd3 workflows — defaultRef splits into startRef and diffBase. reviewSkills becomes a boolean review that drives cr-prepare, then one cr-scan per lens, then cr-merge, plus one delta review of the fixes. Repairs are reviewed from their own fromSha. Every prompt gets a step guard, and CI now reads each command's own exit status.
• Evals — code-review eval-20 (headless track on a git sandbox), plus fd3 split-stale-origin (with a new SETUP.sh hook) and build-spec-reentry.

ℹ️ fd3's review stage depends on a code-review release that ships the cr-* skills

implement-run and repair-run call code-review:cr-prepare, cr-scan and cr-merge by name, and the two plugins release separately. If fd3 is released first, every user whose code-review predates this PR gets no-verdict on every branch with review on, so every branch stays merged. The step-2 wording handles this safely, but it would be a bad first experience.

Technical details

# Release order for the cross-plugin dependency

## Affected sites
- plugins/fd3/skills/implement-tasks/SKILL.md:113-121 — recommends review when `code-review:cr-scan` is listed
- plugins/fd3/README.md:52-56 — says review needs the code-review plugin, but not which version

## Required outcome
- code-review is released (`./scripts/release.sh code-review minor`) before or together with fd3
- fd3's README names the first code-review version that ships the headless skills

## Open questions for the human
- Should fd3's README or CHANGELOG pin a minimum code-review version?

ℹ️ Nitpicks

• plugins/code-review/references/merge-contract.md:231 and plugins/code-review/references/scanner-contract.md:139 cite references/severity.md, but both files open by saying every path in them is relative to references/. That makes the path severity.md, as merge-contract.md:70 already writes it.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏}

[pullfrog]

ℹ️ No critical issues. All three routing fixes are correct; the prose that lists what reaches the user just needs to catch up.

Reviewed changes

This run covers the four commits since the prior review at 7718000, each answering one prior thread.

• Kept spec findings out of the fixer. sortFindings excludes family === 'spec' from applied, so a headless structural wrong-implementation now goes to forHuman.
• Routed boy-scout findings to the human. Both CR_MERGE_RESULT schemas gain boyScout, both merge wrappers set it from the boy-scout token, and applied excludes it.
• Held a branch on findings the fixer left unfixed. FIX_RESULT gains skipped, which the prompt asks for only on code-review fixes. Exact matches join forHuman. A line the fixer paraphrased still becomes its own review HIL item and sets reviewHeld, so a bad copy fails safe. reviewHeld now ORs in, so the earlier hold survives.
• Counted check 9's form as the fifth in validation-report.md:65.

ℹ️ Nitpicks

• plugins/fd3/workflows/implement-run.js:1007 stores summary.review.applied / forHuman before d39ba0b moves the fixer's skipped findings into sorted.forHuman (L1015). So the run summary counts a declined finding as applied. Moving the Object.assign below the fix block (or recomputing the two counts there) would keep it accurate.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏}

[pullfrog]

sortFindings now also sends two more kinds of finding to the human: boy-scout findings (98edd97) and findings the fixer left unfixed (d39ba0b). This list still names only the original four. The skill relays this text to the user literally, so it should name every kind of review item it can hand over. The same enumeration appears at README.md:55 and CHANGELOG.md:25-26.

Suggested change

	A `review` item is a finding the workflow would not fix unasked — a `spec` finding, a `security`
	fix, a report-only one, or anything the delta review found in the fixes it did apply. Its branch
	A `review` item is a finding the workflow would not fix unasked — a `spec` finding, a `security`
	fix, a boy-scout finding on code the branch never touched, a report-only one, a finding the fixer
	left unfixed, or anything the delta review found in the fixes it did apply. Its branch

[pullfrog]

Same drift as implement-tasks/SKILL.md:209: boy-scout findings and fixer-skipped findings now reach the user as review items too.

Suggested change

	`safe` and `structural` findings are fixed and the fixes reviewed again, while `spec` findings,
	`security` fixes and report-only findings go to the user as `review` items. `repair-run`
	`safe` and `structural` findings are fixed and the fixes reviewed again, while `spec` findings,
	`security` fixes, boy-scout findings, report-only findings and any finding the fixer left
	unfixed go to the user as `review` items. `repair-run`

[pullfrog]

Same drift: add the two new human-bound classes here too.

Suggested change

	one agent each. Mechanical and structural findings are fixed and the fixes reviewed again;
	`spec` findings, `security` fixes and anything report-only go to the user. A branch whose review
	one agent each. Mechanical and structural findings are fixed and the fixes reviewed again;
	`spec` findings, `security` fixes, boy-scout findings on untouched code, anything report-only and
	anything the fixer left unfixed go to the user. A branch whose review

[pullfrog]

ℹ️ The new commit adds no issues. The review stays non-approving only because the three prose threads from the previous review are still open. The lists of review items in implement-tasks/SKILL.md, README.md and CHANGELOG.md still leave out boy-scout findings and findings the fixer left unfixed.

Reviewed changes

This run covers the one commit since the prior review at 787d24b.

• Counted a coded worker heading as closing defect 5. In validate-defective-spec.mjs, the uncoded "Delivery retry worker" check now passes in either of two cases. The first is a report finding that names retry-worker and element code. The second is a sandbox spec where a ### PREFIX-n … worker heading has replaced the bare ### Delivery retry worker. This fits validate-spec/SKILL.md:269-270, which says a finding with one obvious correction should be corrected, not just reported. I also ran the predicate offline against the fixture. The unchanged spec evaluates to false: its existing coded headings (DB-1, API-1, OBSERVABILITY-1) don't contain "worker" on the heading line, so a run that neither reports nor fixes the defect still fails.

  ^{｜ Fix it ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏}

[pullfrog]

ℹ️ The new commit adds no issues. This review is non-approving only because three prose threads from an earlier review are still open. The lists of review items in implement-tasks/SKILL.md, README.md and CHANGELOG.md still leave out boy-scout findings and findings the fixer left unfixed.

Reviewed changes

This run covers the one commit since the prior review at a2af1d5.

• Told the five validate-* evals that no one answers questions. Each prompt now tells the forked validate-spec skill not to hand up questions. Every user-held fact and every repair choice goes into the report as blocked, naming the question it would have asked. This matches validate-spec/SKILL.md:297-300, which turns a claim that can't be settled before reporting into blocked. It also means the final message is the full report, not a step-4 hand-up. The asserts can still fail:
  • validate-ownerless-gap.mjs:19-20 still requires not ready when the claim is blocked, and still rejects a deferred claim whose owner nobody gave.
  • validate-clean-spec and validate-phased-verdict still fail if a spurious question becomes a blocked claim, because that drops the verdict.
  • One side effect: no validate eval now exercises the step-4 hand-up batch or the route where an answer turns a claim deferred.

  ^{｜ Fix it ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏}