fix(fabrika): scope build check's unvalidated list to the surface that ran (#5288)

[usirin]

fabrika build check --surface code returned green over a diff like ["a.ts", "README.md"] while listing nothing as skipped. The markdown was classified, so it never tripped the unclassified-diff refusal, but no code runner reads markdown — lint:worktree filters .md out by extension — and an empty unvalidated: [] affirmatively reads as "nothing uncovered". --surface plan did the same to code files. A green now lists every changed file this surface's validators did not read, so an empty list means the whole diff was checked and nothing weaker.

Fixes #5288

The fork, and which side this takes

#5287's disclosure named two candidate directions — scan both classes, or list the unscanned class — and this takes the second: disclose, do not widen.

--surface is an anchor, not a classifier. Running the markdown validators under --surface code would make the surface guess at file classes, which the anchor exists to refuse, and it has no coherent mirror: covering the symmetric --surface plan case that way would mean build check --surface plan spawning pnpm typecheck. Acceptance criterion 2 asks for one mechanism across both cases, and per-surface disclosure is the only shape that gives one. Acceptance criterion 1 already permits either ("validated or explicitly named as not covered").

This is also what the field was documented to be. contract.md said unvalidated "lists the changed files this verdict does not cover" and then narrowed it to the third class; the implementation followed the narrowing. Scoping the computation to the surface makes the code match the sentence — no new key, no new exit code, no third vocabulary.

What changed

• notCoveredBy(surface, files) reports the changed files this surface's validators do not open, off a small COVERS map. It is a superset of the third class, and the extra members are markdown under code and code under plan — one rule, both cases. packages/fabrika-cli/src/build/check-verb.ts.
• The green's unvalidated and the stderr scope note both read from it. The note names the surface now (--surface code does not validate — NOT covered by this verdict: README.md), because "no surface validates" is no longer true of everything in the list.
• DiffClasses.unvalidated is renamed unvalidatable. The tree-level fact (no surface covers this file, which drives exit 22) and the per-verdict one shared a word, and that collision is how a markdown file sat outside the disclosure while the field's own docs said otherwise.
• contract.md is amended to match: the scoped meaning, the disclosure-is-not-validation rule and its remedy (a second run, not a wider surface), the new scope-note row, an example with both classes, and the build check --surface code greens a diff whose markdown it never scans #5288 grounding line.

Exit codes are untouched — no refusal is added, and 22 keeps its exact trigger.

Regression coverage

Three runCheck tests pin the defect, and they were proven to fail against the pre-fix computation by restoring it (const unvalidated = classifyDiff(files).unvalidatable;) and re-running: all three failed with expected [] to deeply equal [...], which is the defect's exact signature.

• --surface code over ["apps/web/src/App.tsx", "README.md"] → green with unvalidated: ["README.md"] and the stderr line asserted verbatim.
• --surface plan over ["apps/web/src/App.tsx", "plans/epic.md"] → green with unvalidated: ["apps/web/src/App.tsx"], and pnpm typecheck --force asserted not spawned, so the mirrored case is disclosed rather than validated.
• A --surface code run stays green over markdown carrying a machine-local path — the prose validators would red it, which is what proves the file was named but not scanned.

Plus five notCoveredBy unit tests, including the empty-only-when-fully-read case and diff-order reporting.

Verification

• pnpm typecheck — 31/31 tasks, 0 cached, resolved against this worktree.
• pnpm lint:worktree — clean.
• pnpm vitest run in packages/fabrika-cli — 196 files, 2833 tests, all passing.

Deviations

• Reviewer-suggested alternative declined — Said: triage's read on build check --surface code greens a diff whose markdown it never scans #5288 was that disclose-only "leaves mixed diffs permanently unscannable for leaks and dead links, so it is honest but not sufficient on its own." Did: shipped disclose-only anyway. Why: the gap triage names is real but it is a different defect — surfaceMismatch refuses --surface prose whenever the diff holds any code file, so a mixed diff has no runnable prose surface at all. That is a routing rule, not the disclosure bug this issue scopes, and fixing it here would change which surfaces a diff admits, which no acceptance criterion asks for. Acceptance criterion 2's "same mechanism" requirement also rules out the validate-both shape, since its mirror would have --surface plan running pnpm typecheck. Disposition: follow-up build check has no runnable surface that scans a mixed code+markdown diff's prose #5301 filed for the unscannable-mixed-diff routing; for the reviewer to judge whether it belongs in this PR instead.

• Widened the fix shape — Said: the issue asks that the skipped class be named. Did: also renamed the DiffClasses bucket unvalidated → unvalidatable, touching the exported interface fix(fabrika): refuse a build check diff no surface validates, never green it #5287 landed hours ago. Why: leaving both the structural bucket and the per-verdict list called unvalidated in one file reproduces the exact confusion that produced this bug — a reader greps classifyDiff().unvalidated and concludes the JSON field means the same thing. Disposition: no action needed; contained to check-verb.ts and its test, no other consumer in the repo.

• Known defect left unfixed — Said: packages/fabrika-cli/src/build/codes.ts:7 still documents the group as adding its own 12-21, which is stale now that 22 and 23 are allocated; fix(fabrika): refuse a build check diff no surface validates, never green it #5287 left it for the next lane touching that file. Did: left it. Why: this fix allocates no exit code, so codes.ts is not in the diff, and opening it solely for a comment would widen the reviewed surface past the issue. Disposition: for the reviewer to judge — still owned by the next PR that edits codes.ts.

• No ADR — Said: ADR 0265 was pre-assigned for this lane. Did: wrote none, leaving 0265 unused. Why: the change makes an existing documented contract true rather than deciding anything new; contract.md carries the reasoning at the surface it governs. Disposition: no action needed — 0265 is free for another lane.

[github-actions]

No preview deploy

• No preview deploy for this PR — its diff touches no deploy-relevant path, so no preview stack was minted and e2e is not applicable. _(efd6fae)

• web — Stage pr-5300 torn down.

[usirin]

review-code: PASS @ efd6fae — merge-ready

Verified PR #5300 against the acceptance criteria of #5288, one at a time:

• [PASS] AC1 — --surface code over a markdown-carrying diff no longer greens omitting the markdown, on BOTH channels. notCoveredBy() (check-verb.ts:106-113) feeds the JSON unvalidated key (:288, :311, :354) and the stderr scope note (:290-295). AC1 permits "validated or explicitly named as not covered"; this PR takes the second branch, which the criterion authorizes on its face.
• [PASS] AC2 — the symmetric --surface plan-over-code case runs through the same function, not a parallel one: one COVERS map (:88-92) plus one notCoveredBy serves all three surfaces. The test asserts pnpm typecheck --force is NOT spawned on the plan path (check-verb.unit.test.ts:296-313), pinning "disclosed, not validated" as the mirrored behavior.
• [PASS] AC3 — both mixed-diff cases pinned: --surface code over ["apps/web/src/App.tsx","README.md"] (:279-294, JSON + verbatim stderr) and --surface plan over ["apps/web/src/App.tsx","plans/epic.md"] (:296-313). Plus a third proving disclosure is not scanning (:315-331) and five notCoveredBy unit tests (:87-107).
• [PASS] AC4 — no exit code allocated or altered. codes.ts is not in the diff; the imported set (OFF_VOCABULARY, PRECONDITION_UNKNOWN, UNCLASSIFIED_DIFF, VALIDATION_RED, ZERO_SCOPE) is unchanged and 22 keeps its exact trigger (unvalidatableDiff, :119-127). No third vocabulary.
• [PASS] AC5 — built on merged fix(fabrika): refuse a build check diff no surface validates, never green it #5287: the PR's parent 2d78b051 is an ancestor of origin/main (verified by git merge-base --is-ancestor) and already carries classifyDiff / UNCLASSIFIED_DIFF = 22.

Independent verification — re-run, not taken on the body's word

• Regression proof reproduced. Restoring const unvalidated = classifyDiff(files).unvalidatable; at :288 in a throwaway tree at this head fails exactly 3 tests with 28 still passing — expected [] to deeply equal [ 'README.md' ], [ 'apps/web/src/App.tsx' ], [ 'docs/guide.md' ]. The reported signature is exact, and the failure set is targeted rather than blanket.
• tsgo -p tsconfig.json --noEmit run directly and uncached in a tree I confirmed at efd6fae0 and git status --porcelain-clean — exit 0. Run outside the turbo path deliberately, per the review-gate head handle is session-keyed, not per-PR/agent — concurrent reviewers can gate on a foreign worktree and report clean #4881/review-code's in-worktree typecheck reads green off a turbo cache hit produced by a different worktree #4887 cross-worktree cached-pass class.
• biome check over both changed dirs — 47 files, clean.
• Full packages/fabrika-cli suite — 196 files, 2833 tests passing. Matches the body's claim exactly.

Ruling on the fork, and on the disagreement with the triaged body

• Fork (disclose-only over validate-both): upheld. AC1 authorizes either on its face, so AC2 is the discriminator — and the author's argument holds. Validate-both has no coherent mirror: covering --surface plan over a code diff the same way means build check --surface plan spawning pnpm typecheck, which dissolves --surface into a no-op. That is not a rationale invented to shrink the job — it cites the file's own pre-existing docblock ("--surface is an anchor, not a second classifier", :12-16), which predates this ticket. And contract.md's headline sentence already read "lists the changed files this verdict does not cover" before an em-dash narrowed it to the third class. So this makes the code match a sentence that had already shipped, rather than inventing new semantics.
• Disagreement with the triaged body: legitimate and correctly disclosed — not ticket-narrowing. The routing-vs-disclosure distinction is sound at source: the unscannability triage names is produced by surfaceMismatch refusing --surface prose whenever any code file is present (:131-133), plus leakDefects running only under prose (:325). No change to the unvalidated computation can reach it. Two further things decide it. First, the sentence the author departs from sits in triage's discussion ("Triage's read: ..."), not in the acceptance criteria — and AC1, written by the same triage with that discussion in view, is a deliberate disjunction. A read the criteria themselves decline to encode is guidance, not a contract term. Second, the test for narrowing is whether the remainder was hidden or defined away; here it was quoted verbatim, filed as build check has no runnable surface that scans a mixed code+markdown diff's prose #5301, and handed to the gate with an explicit invitation to overrule. That is the deviation protocol working, not being evaded.
• I do not overrule. Bundling a routing change into a disclosure fix would change which surfaces a diff admits — a behavior change no criterion requests and one that needs its own reasoning. But triage's substantive point is not discharged by this PR: a mixed code+markdown diff still has no surface that leak-scans its prose. build check has no runnable surface that scans a mixed code+markdown diff's prose #5301 should be triaged as real work, not filed-and-forgotten.

Invariant check — is unvalidated: [] TRUE whenever emitted?

Within the diff's reach, yes: an empty list now requires that every changed file matched a class this surface's validators open. Two residual paths where a green can still under-report — both verified pre-existing at the parent commit, neither introduced nor worsened here, both outside the ACs:

1. :324 — if (!text.ok) continue; skips a covered markdown file whose read failed, and the catch is catchTag("PlatformError"), i.e. every platform error, not just not-found. The comment asserts deletion is the only cause; the code never checks that. A permission or IO failure therefore yields a green with unvalidated: [] over a file nothing opened — exactly the "green means I could not tell" shape.
2. COVERS models coverage per file class, but prose and plan both claim the markdown class while running disjoint validators (:325). So --surface plan over a .md runs only the Dependencies grammar, never the leak/link scan, and still reports unvalidated: []. Literally consistent with the contract's new sentence (the file was read by a validator that passed), but it is the next instance of the same shape.

Neither blocks this PR. Both deserve a follow-up — #5301 is the natural home or a sibling to it.

Rename (the root-cause half) — complete. DiffClasses.unvalidated → unvalidatable (:78, :84), unvalidatableDiff destructuring updated (:120), both test assertions updated. A tree-wide grep for classifyDiff / DiffClasses / unvalidatable at this head finds zero consumers outside check-verb.ts and its test, so the containment claim is true. The two surviving unvalidated identifiers are the public JSON key (documented in contract.md; renaming it would be a breaking output change) and the local now sourced from notCoveredBy. No call site conflates the tree-level fact with the per-verdict one.

Deviations audit — literal ## Deviations heading present, 4 entries, each verified true at source:

1. Triage disagreement — quoted build check --surface code greens a diff whose markdown it never scans #5288 sentence is verbatim; build check has no runnable surface that scans a mixed code+markdown diff's prose #5301 exists and is open.
2. Rename containment — confirmed by the tree-wide grep above.
3. codes.ts:7 stale header — confirmed stale: the docblock says the group "adds its own 12-21" while UNCLASSIFIED_DIFF = 22 (:92) and HEAD_DROPS_REMOTE = 23 (:105) are allocated. Leaving it is correct — the authorisation to fix was conditional on the file already being edited, and this PR allocates no exit code, so codes.ts is legitimately absent from the diff. Opening it for a comment alone would widen the reviewed surface past the issue.
4. No ADR / 0265 unused — .decisions/ on main ends at 0252, so 0265 is unclaimed; a fix that makes an existing documented contract true decides nothing new.

Run-evidence bundle: PENDING for head efd6fae0 — the run-evidence producer exists but has no run at this head yet (queried head_sha=efd6fae0, 0 run-evidence run(s) at head). PENDING is not ABSENT: the producer has not published for this head yet. — verified from diff + worktree run

Read the PR head (§HEAD): all files under review sourced from efd6fae0eafe61d695eed2f2b8bd28f312a15a4d via a per-run ref plus an isolated throwaway worktree, never the launched checkout's working copy.

§CP: not-control-plane, proven ordinary on both axes (cp-classify classify → path-clear-no-content-source; no CODEOWNERS row covers packages/fabrika-cli/** or claude-plugins/fabrika/**, and no .decisions/** file is present). This verdict therefore binds — no human approval gates the merge.

All criteria pass. This PR is merge-ready. review-code does not merge — ship-it is the authorized merge step; merging will auto-close #5288 via Fixes #5288.

Verdict-written: 2026-08-10T06:45:05Z

[usirin]

review-skill: PASS @ efd6fae — merge-ready

Reviewed-head: @ efd6fae

Skill-class gate over claude-plugins/fabrika/skills/build/contract.md — the behavioral artifact in this diff. The code half is verdicted separately in the review-code namespace (comment 5236799751); this comment covers the contract doc only. Class set at this head from class-probe classify --namespaces: has-code, has-skills → review-code + review-skill. Not UI-affecting, so no review-design.

Acceptance criteria (of #5288, as they land on the contract surface)

• [PASS] AC1 — the contract now states the scoped meaning: unvalidated "computed against this surface's validators", naming markdown-under-code and its mirror code-under-plan (contract.md:991-995). Both channels are documented — the JSON key and the stderr scope note, the latter as a new exit-matrix row (:1054).
• [PASS] AC2 — the doc describes one rule, not two: the closing grounding entry says scoping the list "closes it, and the mirrored --surface plan case, with one rule" (:1075-1078). No parallel mechanism is introduced in prose.
• [PASS] AC4 — no exit code is documented as added. The new matrix row carries exit 0 (a scope note beside a green), and 22 keeps its existing row and trigger verbatim. No third vocabulary term appears.

Skill rigor

• [PASS] Behavioral correctness — the prose matches the implementation I read at this head, line for line. The doc's new invariant sentence, "unvalidated: [] therefore means every changed file was read by a validator that passed, and nothing weaker" (:1000-1001), is exactly what notCoveredBy computes (check-verb.ts:106-113). The disclosure-is-not-validation rule and its stated remedy ("a second run, not a wider surface", :996-1000) match the code's actual behavior, and are pinned by a test that keeps a --surface code run green over markdown the prose validators would red (check-verb.unit.test.ts:315-331). The updated example (:1066) shows both classes in one list, which is the behavior.
• [PASS] Trigger / description quality — unchanged; this PR amends contract body text only and adds no trigger, front-matter, or description surface.
• [PASS] Cross-skill conflict / shadowing — none. The change is confined to one section of one fabrika contract; no other skill documents build check's unvalidated field, and the fabrika plugin calls pipeline-cli nowhere (ADR 0238), so no pipeline-side contract is shadowed.
• [PASS] Gate-invariant preservation — strengthened, not weakened. The doc's governing invariant is that a green never over-claims; this amendment widens what a green must disclose (from the third class to every class this surface did not read) and removes no refusal. Exit 22's trigger is documented unchanged, and the zero-scope 7 rule is untouched.
• [PASS] Contract implementation ticket exists and is named — build check --surface code greens a diff whose markdown it never scans #5288, named in the grounding list (:1075) and closed by this PR via Fixes #5288. This is an amendment to a shipped contract whose verbs already exist, not a fresh authoring handoff, so no new implementation ticket is owed.

Advisory — non-blocking, not a condition of this PASS

contract.md keeps a dated amendment log in its header (:5, :7), and every prior contract-semantic change got a line there. This change is contract-semantic — it narrows then rescopes what a documented field means — and adds no line. Worse, the existing 2026-08-10 line now understates current behavior: it says a green "carries the files it did not cover", when a green now carries the files this surface did not cover, a strictly wider set.

I am not failing on it. The substance is documented twice inside the same file (the body text and a dedicated #5288 grounding entry), so the contract itself is correct and a reader of the body is not misled — only a reader who stops at the header. That is the same disposition the stale codes.ts:7 header got and should be treated consistently: the next lane touching contract.md should add the missing **Amended 2026-08-10** — ... (#5288) line and correct the existing one.

Fork and triage-disagreement ruling — both are ruled in full on the review-code verdict (comment 5236799751) and are not restated here. In summary: the disclose-only fork is upheld (AC1 permits either; AC2's one-mechanism requirement rules out validate-both, whose mirror would have --surface plan spawning pnpm typecheck), and the departure from triage's "not sufficient on its own" read is legitimate disagreement correctly disclosed, not ticket-narrowing — the gap triage names lives in surfaceMismatch routing, was quoted verbatim, filed as #5301, and handed to the gate. I do not overrule, but #5301 carries real, undischarged work.

Deviations — literal ## Deviations heading present, 4 entries, each verified true at source (full audit on the review-code verdict).

Read the PR head (§HEAD): all files under review sourced from efd6fae0eafe61d695eed2f2b8bd28f312a15a4d via a per-run ref plus an isolated throwaway worktree, never the launched checkout's working copy.

§CP: not-control-plane, proven ordinary on both axes — cp-classify classify returns path-clear-no-content-source, and no CODEOWNERS row covers claude-plugins/fabrika/** (the claude-plugins/ rows are scoped to kampus-pipeline only). This verdict therefore binds, and a PASS auto-ships with no human approval.

All criteria and rigor checks pass. This PR is merge-ready. review-skill does not merge — ship-it is the authorized merge step.

Verdict-written: 2026-08-10T06:46:07Z