fix(fabrika): refuse a build check diff no surface validates, never green it - #5287
Conversation
…reen it (#5229) `build check` sorted changed files with two extension patterns and read nothing off what matched neither, so "unclassifiable" was an absence rather than a state. A workflow-/shell-only diff cleared the zero-scope guard, produced an empty markdown list, ran zero validators and returned verdict green having opened no file. Name the third class. classifyDiff splits code / markdown / unvalidated, a wholly unvalidatable diff refuses on a new proven code 22 under every surface, and a green over a partly-unvalidatable diff carries the files it did not cover on stdout (unvalidated) and on stderr. Green now means checked-and-passed; it never means could-not-tell. Widening the code pattern to swallow .yml/.sh was the other fork and is rejected in the contract: it would claim pnpm typecheck validated a shell script.
No preview deploy
|
|
review-code: FAIL @ 5a6936a — not merge-ready Verified PR #5287 against the acceptance criteria of #5229, one at a time. All seven acceptance criteria PASS. The single blocking finding is the Read the PR head (§HEAD): every file under review sourced from Artifact class fan: §CP classification:
Acceptance criteria
The regression test genuinely pins the defect — proven by execution, not by the author's claimI did not take "it fails against the old code" on trust. In an isolated throwaway worktree at
That is a real regression test, not one that passes both before and after.
|
|
review-skill: FAIL @ 5a6936a — not merge-ready The skills-class artifact in this PR is Read the PR head (§HEAD): the contract text was read from Artifact class fan: Step 0 — blocking or non-blocking: NON-BLOCKING (
|
|
review-code: PASS @ 5a6936a — merge-ready Re-gate of the body-only repair round. The prior The head did not move. Artifact class fan, re-run at head (not inherited): §CP:
|
|
review-skill: PASS @ 5a6936a — merge-ready Re-gate of the body-only repair round. The prior The head did not move. Artifact class fan, re-run at head (not inherited): Step 0 — blocking or non-blocking: NON-BLOCKING (
|
Fixes #5229
fabrika build checksorted the changed-file list with two extension patterns and readnothing off what matched neither, so "unclassifiable" was an absence rather than a
state — and an absence cannot be refused. A
.github/workflows/*.yml+*.shdiff clearedthe zero-scope guard, produced an empty markdown list, ran the validator loop zero times and
returned
{"verdict":"green","surface":"prose","ran":["markdown link + leak scan"]}havingopened no file. The mirror direction refused the same diff under
--surface codewith"the diff changes no code file", which points at the branch that greens.
The design question, settled: refuse, do not widen
The issue left the fork open. Resolution: make "matched neither" a named class and refuse —
do not widen the code pattern.
Widening
CODE_REto swallow.yml/.shwould make the verb claimpnpm typecheckandpnpm lint:worktreevalidated a shell script. They do not:lint:worktreefilters tobiome-handled extensions, so a widened pattern buys a green that is false in a new way rather
than silent in the old way. It also makes
--surfaceguess at file classes, whichcontract.mdexplicitly declined ("an anchor, not a second classifier").The honest answer is that no validator covers those files, so no verdict can. The remedy for a
workflow-only lane is to split the diff or extend a validator — not to rename the surface.
A new proven code (
22), notPRECONDITION_UNKNOWN(11).11means a read or a runnerfailed and nothing is proven. Here the diff read succeeded and the classification is complete:
the fact is about the tree, so it belongs with the group's other proven refusals (
20/21),not with the unknowns.
What changed
classifyDiffsplits the changed files three ways —code/markdown/unvalidated— and every file lands in exactly one bucket.
surfaceMismatchnow reads from it instead ofre-filtering, so there is one classifier, not two.
22under every surface, checked before thesurface-mismatch test so
--surface codealso gets the honest reason instead of"changes no code file".
unvalidated: [...](the files this verdict does not cover), and the same line goes tostderr. That is the live PR fix(ci): run every repo-wide gate on push to main, not just on PRs (#5085) #5187 / fix(pipeline): teardown-head stops reporting success on the runs it leaks #5205 shape — 25 workflow files and 8 shell files whose
ranfield was true and misleading at the same time.Green now means "the validators ran and passed". It never means "I could not tell".
Regression coverage
check-verb.unit.test.tsgains the exact diff that used to read green: a.github/workflows/ship.yml+*.shdiff under--surface prosenow asserts exit22andempty stdout. It fails against the old code, which returned
0with a green verdict. Plus:the same diff under
planandcode(asserting the old "no code file" wording is gone and norunner was spawned), both partial-green disclosure cases, and the bucket-partition property.
Contract
claude-plugins/fabrika/skills/build/contract.mdis amended so the contract and the sourceagree:
22in the shared exit matrix and inbuild check's own exit + errors tables, theunvalidatedkey in the output shape and the example, the three-file-classes paragraph underthe surface anchor (including why widening was rejected), and the scope note.
packages/fabrika-cli/src/build/push-verb.tsis untouched — that is #5222's scope.Verification
pnpm typecheck— 31/31 tasks green.pnpm lint:worktree— clean.pnpm vitest runinpackages/fabrika-cli— 2815 tests across 195 files, all passing.Deviations
Known defect left unfixed — Said: fabrika build check passes green on diffs its two regexes never classify #5229 asks that a green verdict never imply a file was
validated when it was not. Did: only the wholly-unvalidatable diff is refused, and the
partial disclosure covers only the third bucket.
--surface codeover a diff that also containsmarkdown (the pre-existing
["a.ts","README.md"]case) still greens: the markdown lands in themarkdownbucket, so it is neither scanned by the code runners nor listed inunvalidated.Why: it is a different shape from this ticket's — those files have a validator, just not the
one this surface runs, so the fix is a design call (scan both classes, or list the unscanned
class) that fabrika build check passes green on diffs its two regexes never classify #5229 did not scope. Widening it here would have changed behavior no acceptance
criterion asked for. Disposition: follow-up build check --surface code greens a diff whose markdown it never scans #5288 filed; disclosed here because §DEV class 3
is owed whether or not a follow-up exists.
Known defect left unfixed (repair round 1) — Said: a comment should describe the code it
sits on. Did:
packages/fabrika-cli/src/build/codes.ts:7still documents the group as addingits own
12-21; this PR adds22, so that range is now stale, and the line is left as-is.Why: the reviewer recorded it as a non-blocking observation requiring no repair, and this
repair round is body-only at head
5a6936ae— touching the comment would move the head andinvalidate the verified acceptance-criteria and run-evidence work already done against it.
Disposition: for the reviewer to judge — fold into a follow-up, or into the next PR that
touches this file.