Skip to content

fix(fabrika): refuse a build check diff no surface validates, never green it - #5287

Merged
usirin merged 1 commit into
mainfrom
usirin/5229-fabrika-check-unclassifiable-diff-6C6BF922
Aug 10, 2026
Merged

fix(fabrika): refuse a build check diff no surface validates, never green it#5287
usirin merged 1 commit into
mainfrom
usirin/5229-fabrika-check-unclassifiable-diff-6C6BF922

Conversation

@usirin

@usirin usirin commented Aug 10, 2026

Copy link
Copy Markdown
Member

Fixes #5229

fabrika build check sorted the changed-file list with two extension patterns and read
nothing off what matched neither, so "unclassifiable" was an absence rather than a
state — and an absence cannot be refused. A .github/workflows/*.yml + *.sh diff cleared
the 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"]} having
opened no file. The mirror direction refused the same diff under --surface code with
"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_RE to swallow .yml/.sh would make the verb claim pnpm typecheck and
pnpm lint:worktree validated a shell script. They do not: lint:worktree filters to
biome-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 --surface guess at file classes, which
contract.md explicitly 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), not PRECONDITION_UNKNOWN (11). 11 means a read or a runner
failed 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

  • classifyDiff splits the changed files three ways — code / markdown / unvalidated
    — and every file lands in exactly one bucket. surfaceMismatch now reads from it instead of
    re-filtering, so there is one classifier, not two.
  • A wholly unvalidatable diff refuses on 22 under every surface, checked before the
    surface-mismatch test so --surface code also gets the honest reason instead of
    "changes no code file".
  • A partly unvalidatable diff still greens, but never bare. The green JSON always carries
    unvalidated: [...] (the files this verdict does not cover), and the same line goes to
    stderr. 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
    ran field 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.ts gains the exact diff that used to read green: a
.github/workflows/ship.yml + *.sh diff under --surface prose now asserts exit 22 and
empty stdout. It fails against the old code, which returned 0 with a green verdict. Plus:
the same diff under plan and code (asserting the old "no code file" wording is gone and no
runner was spawned), both partial-green disclosure cases, and the bucket-partition property.

Contract

claude-plugins/fabrika/skills/build/contract.md is amended so the contract and the source
agree: 22 in the shared exit matrix and in build check's own exit + errors tables, the
unvalidated key in the output shape and the example, the three-file-classes paragraph under
the surface anchor (including why widening was rejected), and the scope note.

packages/fabrika-cli/src/build/push-verb.ts is untouched — that is #5222's scope.

Verification

  • pnpm typecheck — 31/31 tasks green.
  • pnpm lint:worktree — clean.
  • pnpm vitest run in packages/fabrika-cli — 2815 tests across 195 files, all passing.

Deviations

  • Known defect left unfixedSaid: 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 code over a diff that also contains
    markdown (the pre-existing ["a.ts","README.md"] case) still greens: the markdown lands in the
    markdown bucket, so it is neither scanned by the code runners nor listed in unvalidated.
    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:7 still documents the group as adding
    its own 12-21; this PR adds 22, 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 and
    invalidate 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.

…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.
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown

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. (5a6936a)
  • web — Stage pr-5287 torn down.

@usirin

usirin commented Aug 10, 2026

Copy link
Copy Markdown
Member Author

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 ## Deviations section (§DEV) — a body edit, no code change.

Read the PR head (§HEAD): every file under review sourced from 5a6936ae via a per-run ref + throwaway worktree, never the launched checkout's working copy.

Artifact class fan: class-probe classify --namespaces at this head → review-code, review-skill (2 namespaces; no apps/web/src/** path, so no review-design). This is the review-code marker; the review-skill marker is posted as its own separate comment.

§CP classification: not-control-plane [path-clear-no-content-source] — settled on both axes, against the author's own note. The write-code progress comment on #5229 says "the diff touches claude-plugins/**, so it classifies control-plane by path." That is wrong, and correcting it is load-bearing here:

  • Path axis (the deciding axis). The live CONTROL_PLANE_RE, re-resolved from origin/main, covers ^claude-plugins/kampus-pipeline/skills/, .../lib/, .../agents/, .../hooks(/|\.json$)kampus-pipeline only. The one claude-plugins file here is claude-plugins/fabrika/skills/build/contract.md, which matches no branch. The three packages/fabrika-cli/** files match no packages/ branch either (^packages/ci-required/, ^packages/pipeline-cli/src/[^/]+$, the enumerated pipeline-cli/src/tools/* dirs).
  • CODEOWNERS axis. Read live from .github/CODEOWNERS on main: no row covers claude-plugins/fabrika/** or packages/fabrika-cli/**. Not owned by @kamp-us/control-plane.
  • ADR-0164 content axis. Probed rather than assumed: the diff carries zero .decisions/** files, so the content clause has no input.

pipeline-cli cp-classify classify returns not-control-plane [path-clear-no-content-source] — no path matched the live CONTROL_PLANE_RE and no .decisions/** file is present, so the ADR-0164 content clause has nothing to decide. Proven ordinary. This matches sibling PR #5261's classification on the same basis. So this verdict is a binding, non-blocking marker, not a §CP advisory.

Acceptance criteria

  • [PASS] Matched-neither is a representable state, not an absenceclassifyDiff in packages/fabrika-cli/src/build/check-verb.ts returns DiffClasses {code, markdown, unvalidated}, with unvalidated the files matching neither pattern. The two patterns are disjoint, so the three buckets are a true partition — pinned by the "puts every file in exactly one bucket" property test over .tsx .mjs .json .md .mdx .sql .css LICENSE. surfaceMismatch now reads from classifyDiff instead of re-filtering, so there is one classifier, not two.
  • [PASS] A wholly-unclassifiable diff can no longer reach verdict: green under any --surfaceunvalidatableDiff(files) returns non-null exactly when code.length === 0 && markdown.length === 0, checked before surfaceMismatch and after the ZERO_SCOPE guard, so it refuses on UNCLASSIFIED_DIFF (22) on all three surfaces. I traced every remaining answer(...) return: the code branch is reachable only past surfaceMismatch, which requires code.length > 0; the prose/plan branch requires markdown.length > 0 (prose because unvalidatableDiff already excluded the both-empty case, plan because surfaceMismatch refuses on an empty markdown list). No green is reachable on an unclassified input.
  • [PASS] The resolution is stated in the PR body with reasoning, and the refusal code is in the contract exit table — the body's "The design question, settled: refuse, do not widen" section carries the reasoning; claude-plugins/fabrika/skills/build/contract.md carries 22 in the shared exit matrix, in build check's own exit table, and in its errors table, with the message string byte-identical to the source.
  • [PASS] The mirror direction is honest — the resolution keeps those files unclassifiable, so --surface code over a workflow-/shell-only diff refuses with the honest reason instead of "the diff changes no code file". Pinned: that case asserts the new message, asserts the old wording is gone, and asserts no runner was ever spawned.
  • [PASS] A partially-unclassifiable diff never returns a bare green — every green JSON carries unvalidated: [...], and the same fact goes to stderr as "N changed file(s) no surface validates — NOT covered by this verdict". Both channels tested, on the prose path (the fix(ci): run every repo-wide gate on push to main, not just on PRs (#5085) #5187 shape) and the code path.
  • [PASS] Unit coverage for the wholly-unclassifiable diff under each of the three surfaces, and the mixed case — present: three surface cases, two partial-disclosure cases, and the bucket-partition property.
  • [PASS] push-verb.ts untouched — the changed-file list is exactly four files; packages/fabrika-cli/src/build/push-verb.ts is not among them.

The regression test genuinely pins the defect — proven by execution, not by the author's claim

I did not take "it fails against the old code" on trust. In an isolated throwaway worktree at 5a6936ae I put back the merge-base (fb2044a9) copy of check-verb.ts, appended a pure classifyDiff shim so the new test file's imports resolve without touching the old decision logic, kept the new codes.ts (purely additive) and the new test file, and ran the suite:

  • At head: 23/23 pass.
  • Against the old runCheck logic: 6 of 23 fail, including the headline regression — "refuses a wholly-unvalidatable diff on 22 under --surface prose — the false green" fails with expected +0 to be 22. The old code returned exit 0 on the exact .github/workflows/ship.yml + *.sh diff under --surface prose. The other five: the same diff under plan (old 10) and code (old 10), and the three unvalidated-disclosure assertions (expected undefined to deeply equal [...]).

That is a real regression test, not one that passes both before and after.

22 is a proven refusal, correctly kept distinct from PRECONDITION_UNKNOWN

The proven/unknown split the whole build code table rests on is preserved, not collapsed:

  • UNCLASSIFIED_DIFF = 22 is its own constant in packages/fabrika-cli/src/build/codes.ts, documented as a proven refusal alongside 20/21, explicitly not a borrowed PRECONDITION_UNKNOWN.
  • The distinction holds at the call site: a diff read that fails still refuses on PRECONDITION_UNKNOWN with "the verdict is UNKNOWN, never green"; 22 is only reachable after that read succeeded and classifyDiff completed. The fact established is about the tree, not about a read.
  • The remedy differs, which is what earns the separate seat: 11 says retry or investigate the read, 22 says split the diff or extend a validator.

Sub-gates

  • [PASS] comment-discipline — the added notes are load-bearing under the deslop-comments KEEP carve-out: the module docblock states the green-means invariant and points at classifyDiff; the DiffClasses and unvalidatableDiff docblocks each state a local invariant at its enforcement site (why the third bucket exists; why the check must precede surfaceMismatch); the inline note above noted carries the forcing constraint for the disclosure; the UNCLASSIFIED_DIFF docblock carries the proven-vs-unknown rationale. No narration, no name-restating, no ADR re-derivation.
  • [PASS] unresolved-threads — the review-thread read ran and found no unresolved thread.
  • glossary-freshness: not applicable — no new feature folder under apps/web/worker/features/* and no new public package; the new exports are module-internal to packages/fabrika-cli/src/build/.
  • session-caching gate: not applicable — no new session-caching path.
  • flag-gating: not applicablefabrika build check passes green on diffs its two regexes never classify #5229 carries no **Containment:** marker.
  • [FAIL] deviation-disclosure (§DEV) — below. This is the only blocking row.

[FAIL] deviation-disclosure — a falsified None.

The body's ## Deviations heading is present (Tier-M scan: section present, 0 suppression/skip lines, 0 removed-assertion lines) and says None. But a §DEV class 3 — Known defect left unfixed is in this PR, and the author named it themselves in the write-code progress comment on #5229:

--surface code over a diff containing markdown still greens without scanning that markdown (the pre-existing ["a.ts","README.md"] case, asserted in the old tests). Different shape from this ticket's, left alone deliberately — worth a look separately.

Confirmed in source: with .md files in a --surface code diff, the markdown list is non-empty so unvalidatableDiff returns null, the code runners run, and the markdown lands in the markdown bucket — so it is neither scanned nor listed in unvalidated. A green that says "the validators ran and passed" over a file nothing opened, disclosed nowhere on the PR. Filing the follow-up (#5288) does not discharge the obligation: §DEV class 3 is owed "whether or not you filed a follow-up", and the disposition field is precisely where "follow-up #5288 filed" belongs.

Per §DEV, a gate finding a class-N deviation in a body that says None. is blocking on two counts — the undisclosed deviation, and that the section can no longer be trusted on this PR. Sibling PR #5282 failed both namespaces tonight for the weaker version of this (disclosure content present, but under a prose heading rather than ## Deviations); this body carries the content nowhere and affirmatively asserts None.

Remedy — one body edit, no code change: replace None. with an entry naming what the spec said, what the implementation did instead, why, and the disposition — Said: #5229 asks that a green never imply files were validated that were not. Did: --surface code over a diff containing markdown still greens without scanning that markdown, and those files are not listed in unvalidated either. Why: a different shape from this ticket's — those files have a validator, just not the one this surface runs, and the fix needs its own design call. Disposition: follow-up #5288 filed.

Non-blocking observations (not FAIL rows; no repair required)

  1. packages/fabrika-cli/src/build/codes.ts:7 still reads "adds its own 12-21" — this PR added 22, so that range is now stale. Not a [FAIL]: it is not an added or changed comment line, so it sits outside the comment-discipline gate's scope. Worth folding into the same body-edit repair round if one happens anyway.
  2. unvalidated slightly over-reports for .css / .jsonc / .graphql. scripts/biome-worktree.mjs:31 sets BIOME_EXT = /\.(ts|tsx|js|jsx|mjs|cjs|json|jsonc|css|graphql)$/, so lint:worktree does validate those three while CODE_RE does not include them — they land in unvalidated. This errs conservative (it under-claims coverage, never over-claims), so it produces no false green. A candidate follow-up, not a defect here.
  3. A prose diff whose only markdown file was deleted still greens with ran: ["markdown link + leak scan"] having opened no file (the if (!text.ok) continue; branch, unchanged by this PR). Same family as fabrika build check passes green on diffs its two regexes never classify #5229, different cause; outside this issue's acceptance criteria.

Verification run at this head

  • Full packages/fabrika-cli suite in the review worktree: 2815/2815 passed, 195/195 files.
  • tsgo -p tsconfig.json for packages/fabrika-cli: clean, exit 0.
  • Run-evidence bundle: PRESENT for head 5a6936ae — producer run 31357386615, artifact 9051199817, manifest.commit == head, schemaVersion 1; checks 2/2 pass; tests 2424/2424 passed, 0 failed, 0 skipped.

Rulings on the two design calls the PR asked for

Refuse-don't-widen is RIGHT — and the author's grounding claim checks out at source. The root package.json runs lint:worktree as node scripts/biome-worktree.mjs check, and that file filters the changed set through BIOME_EXT = /\.(ts|tsx|js|jsx|mjs|cjs|json|jsonc|css|graphql)$/ (line 31), which excludes .yml and .sh outright; tsgo reads neither. Widening CODE_RE would emit a green whose ran: ["pnpm typecheck --force", "pnpm lint:worktree"] is a false claim about files neither command opened — trading a silent false green for a loud one, which is strictly worse in a verb whose entire defect class is "asserts success on the path it failed". It would also turn --surface into the file-class guesser contract.md explicitly declined ("an anchor, not a second classifier"). Refusing keeps the honest invariant.

Leaving the --surface code-over-markdown case is ACCEPTABLE — it does not make this fix incomplete. It is a genuinely different shape (a file class that has a validator, just not the one this surface runs, versus a class no validator covers), its fix needs a design call this ticket did not scope, and follow-up #5288 is open. Neither acceptance criterion reaches it: AC 2 bars only the wholly-unclassifiable diff, AC 5 covers only the markdown-plus-ignored case. My acceptance is conditional on one thing only — that it be disclosed in the body, which is the FAIL above.


deviation-disclosure is the only failing criterion; every acceptance criterion and every other sub-gate passes. The PR stays open and unmerged; #5229 stays open and assigned. Re-request review once the ## Deviations entry lands — no code change is needed and nothing needs re-running. review-code does not mergeship-it is the authorized merge step.

Verdict-written: 2026-08-10T05:18:18Z

@usirin

usirin commented Aug 10, 2026

Copy link
Copy Markdown
Member Author

review-skill: FAIL @ 5a6936a — not merge-ready

The skills-class artifact in this PR is claude-plugins/fabrika/skills/build/contract.md. Every acceptance criterion that reaches it PASSES and every rigor check PASSES. The single blocking finding is the ## Deviations section (§DEV) — the same row the review-code verdict carries, since one falsified None. fails both namespaces.

Read the PR head (§HEAD): the contract text was read from 5a6936ae via a per-run ref, never the launched checkout's working copy. Config-pin held: this gate ran on the base review-skill, not the PR's.

Artifact class fan: class-probe classify --namespaces at this head → review-code, review-skill. This is the review-skill marker; the review-code marker is a separate comment on this PR.

Step 0 — blocking or non-blocking: NON-BLOCKING (not-control-plane), settling the author's own claim

The write-code progress comment on #5229 states "the diff touches claude-plugins/**, so it classifies control-plane by path." That is wrong. Settled here on both axes, read live rather than inherited:

  • Path axis (the deciding axis). The live CONTROL_PLANE_RE re-resolved from origin/main covers ^claude-plugins/kampus-pipeline/skills/, .../lib/, .../agents/, .../hooks(/|\.json$)kampus-pipeline only. This PR's only claude-plugins file is claude-plugins/fabrika/skills/build/contract.md; fabrika is not kampus-pipeline, so it matches no branch. The three packages/fabrika-cli/** files match no packages/ branch either.
  • CODEOWNERS axis. .github/CODEOWNERS on main carries no row for claude-plugins/fabrika/** or packages/fabrika-cli/**, so @kamp-us/control-plane does not own any changed path.
  • ADR-0164 content axis. Run, not assumed: the diff carries zero .decisions/** files, so the content clause has no input.

pipeline-cli cp-classify classifynot-control-plane [path-clear-no-content-source], matching sibling PR #5261's classification on exactly this basis. So this is a binding, non-blocking marker, not a §CP advisory, and no control-plane approval is required.

Acceptance criteria touching the skills class

  • [PASS] The chosen resolution is stated in the PR body with reasoning, and the refusal exit code is added to the contract's exit tablecontract.md gains 22 in the shared exit matrix, in build check's own exit table, and in its errors table. The errors-table message is byte-identical to what the source emits, so the contract and the source agree rather than merely both mentioning 22.
  • [PASS] The mirror direction is documented honestly — the new "Three file classes, because two cannot express unvalidatable" paragraph under the surface anchor states that a wholly-third-class diff refuses on 22 under every surface, names the remedy (split the diff or extend a validator), and records that widening the code pattern was considered and rejected. That matches the implementation exactly.

Skill-rigor checklist

  • [PASS] Behavioral correctness — I traced the amended contract as an agent would execute it against the shipped verb. The output-shape line, the example, the unvalidated paragraph, the three-file-classes paragraph, the two exit tables and the errors table all describe behavior the source actually produces at this head: unvalidated is present on every green (the code-branch answer and the prose/plan answer both carry it), the 22 refusal fires before the surface-mismatch test, and the disclosure line goes to stderr as well as stdout. No step of the contract points an agent at behavior the verb does not have.
  • [PASS] Trigger / description quality — no frontmatter or trigger surface changed; contract.md is a reference document, not a trigger surface.
  • [PASS] Cross-skill conflict / shadowing — the amendment stays inside the build group's own exit table. 22 is unused in that table before this PR, and the sibling groups that overlap 20/21 (plan/codes.ts, ledger/codes.ts) are explicitly scoped to their own tables and are untouched. No shared seam, marker namespace, or routing rule is changed.
  • [PASS] Gate-invariant preservation — nothing in §CP is in this diff's reach. No pipeline gate's refusal, matcher, SHA-binding, conjunctive bar, or ACL is touched. The check ran and had nothing to weaken. Worth noting the direction of the change is the opposite of a weakening: it converts a reachable false green into a proven refusal, which strengthens the verb's own zero-scope posture (ADR 0092) one step further in.
  • [PASS] Contract implementation ticket (ADR 0248) — the diff does change a claude-plugins/fabrika/skills/*/contract.md, so the check is in scope by its file-list selector. It is discharged by the diff itself: ADR 0248's question is "does an open ticket exist that routes to these verbs", and its stated concern is "a merged contract with no ticket, because that is the state that reads as done and is not." Here the specified behavior is not unbuilt — 22, UNCLASSIFIED_DIFF and the unvalidated key ship in check-verb.ts and codes.ts in this same PR, with unit coverage, so there is no unbuilt spec to hand off and no authoring-session lane end. This is a fix PR that moves code and contract in lockstep, not an authoring session's spec-only handoff. PASS with that evidence.

Step 4b — specialist fan-out

  • unreachable-step — none. The new contract paragraphs are declarative reference text, not procedure; no branch or guard is introduced.
  • contradictory-instruction — none found. I checked the new "refuse, never rename the surface" guidance against the pre-existing "an anchor, not a second classifier" line and the 10 surface-mismatch row: they agree, and the new paragraph is the stronger form of the same rule.
  • uncovered-procedure-path — none in scope. The one gap a reader could ask about (what a workflow-only lane should do) is answered in the new paragraph: split the diff or extend a validator.

No findings to route.

[FAIL] deviation-disclosure (§DEV)

The body's ## Deviations heading is present (Tier-M scan: section present, 0 suppression/skip lines, 0 removed-assertion lines) and says None. But a §DEV class 3 — Known defect left unfixed exists in this PR, named by the author in the write-code progress comment on #5229:

--surface code over a diff containing markdown still greens without scanning that markdown (the pre-existing ["a.ts","README.md"] case, asserted in the old tests). Different shape from this ticket's, left alone deliberately — worth a look separately.

Confirmed in source, and it also touches this class directly: contract.md now asserts that a green's unvalidated list "is what keeps the partial case honest" and that green means the validators ran and passed — while --surface code over a diff containing markdown still greens with the markdown neither scanned nor listed in unvalidated. The contract therefore slightly over-states the guarantee, and the PR body discloses nothing.

Filing the follow-up (#5288) does not discharge the obligation: §DEV class 3 is owed "whether or not you filed a follow-up", and the disposition field is exactly where "follow-up #5288 filed" belongs. Per §DEV a gate finding a class-N deviation in a body that says None. is blocking on two counts — the undisclosed deviation, and that the section can no longer be trusted on this PR. Sibling PR #5282 failed both namespaces tonight for the weaker version (content present, wrong heading); here the content is absent from the body and None. is affirmatively asserted.

Remedy — one body edit, no code change. Replace None. with an entry naming what the spec said, what the implementation did instead, why, and the disposition (follow-up #5288 filed). If the repair round is opening the body anyway, two cheap extras are worth folding in: packages/fabrika-cli/src/build/codes.ts:7 still says the group "adds its own 12-21", now stale at 22; and the contract's partial-green paragraph could name the --surface code-over-markdown carve-out so the doc does not over-state its own guarantee.

Ruling: refuse-don't-widen is right, and the contract records it correctly

Verified the grounding at source rather than taking the PR body's word: scripts/biome-worktree.mjs:31 sets BIOME_EXT = /\.(ts|tsx|js|jsx|mjs|cjs|json|jsonc|css|graphql)$/, so lint:worktree never opens a .yml or .sh, and tsgo does not either. A widened code pattern would make the contract promise that pnpm typecheck and pnpm lint:worktree validated a shell script — a documented false claim, worse than the silent one being fixed, and it would make --surface the file-class guesser this same document declines two paragraphs earlier. The contract's amendment states the rejection and its reason, which is the right record.


deviation-disclosure is the only failing row; every acceptance criterion and every rigor check passes. The PR stays open and unmerged; #5229 stays open and assigned. Re-request review once the ## Deviations entry lands. review-skill does not mergeship-it is the authorized merge step.

Verdict-written: 2026-08-10T05:18:29Z

@usirin

usirin commented Aug 10, 2026

Copy link
Copy Markdown
Member Author

review-code: PASS @ 5a6936a — merge-ready

Re-gate of the body-only repair round. The prior review-code: FAIL (comment 5236191670) carried exactly one blocking row — deviation-disclosure (§DEV) — against a ## Deviations section that said None. while a class-3 known defect existed. That row now passes. No other row changed, and the earlier FAIL is left standing as the record of round 1.

The head did not move. 5a6936ae49b4632c422b66e914799b2ff03d59bd before and after; the repair was a PR-body edit only, no commit, no push, no repo file touched. Verified by fetching pull/5287/head into a per-run ref and asserting it resolves to the pinned SHA (§HEAD) — every file below was read from that ref, never from a working copy.

Artifact class fan, re-run at head (not inherited): class-probe classify --namespaces over the 4 changed files → has-code, has-skillsreview-code, review-skill. Exit 0, 2 namespaces. No apps/web/src/** path, and the live UI_RE='^apps/web/src/' re-resolved from origin/main matches nothing here, so review-design is neither required nor dispatched. This is the review-code marker; review-skill is posted as its own separate comment.

§CP: not-control-plane — re-derived, and this verdict BINDS

Re-run rather than carried over. pipeline-cli cp-classify classify against the live CONTROL_PLANE_RE re-resolved from origin/main:

not-control-plane [path-clear-no-content-source] — no path matched the live CONTROL_PLANE_RE and no .decisions/** file is present, so the ADR-0164 content clause has nothing to decide. Proven ordinary.

  • Path axis (deciding). CONTROL_PLANE_RE covers claude-plugins/kampus-pipeline/ (skills/, lib/, agents/, hooks) only. This PR's one claude-plugins file is claude-plugins/fabrika/skills/build/contract.mdfabrika, not kampus-pipeline — so it matches no branch, and the three packages/fabrika-cli/** files match no packages/ branch.
  • ADR-0164 content axis. Zero .decisions/** files in the diff, so the content clause has no input.

So this is a binding, non-blocking marker, not an advisory — a PASS here makes the PR eligible to auto-ship with no human approval. Stated plainly because that is the consequence: I am the last gate on this namespace.

What I re-verified vs. what I inherited

The head is byte-identical to the one round 1 graded, so re-running the 2815-test suite, the typecheck and the run-evidence check could not produce a different answer, and I did not manufacture churn by re-running them. What I did not inherit is the judgment the repair turns on — I re-derived the §CP classification, re-ran the class fan, re-ran the canonical Tier-M scan, and independently traced the disclosed defect through the source at head.

Carried forward from round 1, unchanged and not re-litigated: 7/7 acceptance criteria PASS, all sub-gates PASS (comment-discipline, unresolved-threads; glossary-freshness / session-caching / flag-gating N/A), 2815/2815 tests, clean tsgo, run-evidence bundle present and manifest.commit == head.

[PASS] deviation-disclosure (§DEV) — the disclosure is true, specific, and not softened

Canonical Tier-M scan at head: ## Deviations section present; 0 suppression/skip lines; 0 removed-assertion lines. None. is gone, replaced by two class-3 entries, each carrying all four fields (Said / Did / Why / Disposition).

Replacing None. is the correct shape, not a violation of "repair appends, never replaces." That rule protects prior entries; round 1's section held no entries, only the explicit-empty marker, and leaving a falsified None. standing beside real entries would make the section assert nothing and something at once. The second entry is tagged (repair round 1) per the convention.

Entry 1 — the --surface code-over-markdown case. I checked the claim against source rather than accepting it as present. Traced ["a.ts", "README.md"] under --surface code through runCheck at head:

  1. classifyDiffcode: ["a.ts"], markdown: ["README.md"], unvalidated: [].
  2. unvalidatableDiff returns null (code.length > 0) — no refusal.
  3. surfaceMismatch("code", …) returns null (code.length === 1, not 0) — no refusal.
  4. noted takes the unvalidated.length === 0 branch — no disclosure line on stderr.
  5. The code branch runs pnpm typecheck --force and pnpm lint:worktree. scripts/biome-worktree.mjs:31 filters to BIOME_EXT = /\.(ts|tsx|js|jsx|mjs|cjs|json|jsonc|css|graphql)$/, which excludes .md; tsgo does not read it either. The markdown scan loop is in the other branch and never runs.
  6. Returns {"verdict":"green", …, "unvalidated": []}.

So README.md is neither scanned nor listed — and unvalidated: [] affirmatively reads as "nothing uncovered." The entry's wording ("the markdown lands in the markdown bucket, so it is neither scanned by the code runners nor listed in unvalidated") describes the real mechanism precisely. Not a softened version: it names the exact bucket, both failure modes, and concedes the gap against #5229's general intent rather than hiding behind "no acceptance criterion required it" — which it could have, and which would have been true (AC 2 is scoped to the wholly-unvalidatable diff; AC 5 to markdown plus ignored files, a shape this case does not have). Conceding more than the minimum is the opposite of softening. This confirms round 1's 7/7 stands: the case is a genuine §DEV item, not a missed criterion.

Judged on §DEV's three questions:

  • Authorized? Yes — round 1 ruled the deferral acceptable with standing, conditional on disclosure. This entry is what discharges that condition.
  • Needs an ADR? No — class 3, not a class-2 governing-ADR departure.
  • Needs a follow-up issue? Yes, and one exists: build check --surface code greens a diff whose markdown it never scans #5288, open, type:bug, status:triaged, p1, titled "build check --surface code greens a diff whose markdown it never scans" — the same defect, not an adjacent one.

All three answered. PASS.

Ruling on entry 2 — the stale codes.ts:7 comment: disclosing without fixing is ACCEPTABLE here

The entry's disposition is for the reviewer to judge, one of §DEV's four sanctioned disposition values, so the repairer routed it correctly rather than dodging. Judging it:

The characterization is accurate, and worth sharpening on one point. The codes.ts hunk is purely additive (@@ -81,3 +81,12 @@); line 7 is untouched context. But this PR did not inherit that staleness — it created it: "adds its own 12-21" was true before this commit and false after it. That is a slightly worse framing than "left unfixed," and it is the framing I judged against.

Ruling: non-blocking, no action needed for this PR. Reasons, in order of weight:

  1. The authoritative list is correct and co-located. Every constant carries its own accurate docblock, and UNCLASSIFIED_DIFF = 22 is the last line of the same file. The realistic collision path — a future author reads "12-21" and allocates 22 — is closed by the fact that they must scroll past = 22 to append their own constant. The wrong sentence is a summary; the source of truth is three screens below it and right.
  2. The fix worth making is not a bump. Correcting 12-21 to 12-22 restores truth for exactly one commit and re-arms the same trap at code 23. The durable fix is to de-enumerate it — "adds its own codes from 12 up" — a small editorial call about the docblock, not a mechanical correction, and it belongs with the next change to this file rather than bolted onto a fix PR at a verified head.
  3. The cost is real and one-sided. Touching it moves the head and forces a full re-verification round (2815 tests, typecheck, a fresh run-evidence bundle, a third gate pass) to correct one token in a comment whose error cannot produce a wrong code, a wrong exit, or a wrong verdict. That trade does not clear the bar for blocking a merge.

§DEV's "a class-3 defect with no filed issue is a [FAIL]" does not fire here, because the rule's operative test is the question it parenthesizes — does it need a follow-up issue? — and I judge that it does not. A one-line docblock range whose authoritative list sits in the same file does not need a tracked ticket; requiring one would make the sanctioned no action needed disposition unreachable for class 3, and would trade a third gate round for a board entry nobody would action. This verdict is the durable record of that judgment, which is exactly what for the reviewer to judge contemplates.

Recorded for whoever next touches packages/fabrika-cli/src/build/codes.ts: de-enumerate the range in the module docblock rather than bumping it. #5288's fix lands in this same build group and is the natural place.

Non-blocking observations (carried from round 1, unchanged — no repair required)

  1. unvalidated slightly over-reports for .css / .jsonc / .graphql: lint:worktree does validate those three while CODE_RE does not include them. This errs conservative — it under-claims coverage and can never produce a false green.
  2. A prose diff whose only markdown file was deleted still greens with ran: ["markdown link + leak scan"] having opened no file (the if (!text.ok) continue; branch, unchanged by this PR). Same family as fabrika build check passes green on diffs its two regexes never classify #5229, different cause, outside its acceptance criteria. Round 1 ruled this out of scope and I hold that bar rather than moving it between rounds.

Neither is disclosed in ## Deviations, and neither is owed: round 1 ruled both candidate follow-ups rather than defects of this change, and a gate that raises its own bar at an unchanged head after the author repaired to the stated one is the churn §DEV exists to avoid.


Every acceptance criterion, every sub-gate and every rigor check passes; the single blocking row from round 1 is discharged. Note the §DEV PASS means "nothing undisclosed that this gate could see" — class 3 is Tier D, so it rests on the author's honesty, corroborated here by tracing the disclosed defect to source.

The PR is merge-ready at 5a6936ae. review-code does not mergeship-it is the authorized merge step.

Verdict-written: re-gate round 2, prior FAIL 5236191670 left intact.

@usirin

usirin commented Aug 10, 2026

Copy link
Copy Markdown
Member Author

review-skill: PASS @ 5a6936a — merge-ready

Re-gate of the body-only repair round. The prior review-skill: FAIL (comment 5236192852) carried exactly one blocking row — deviation-disclosure (§DEV), the same falsified None. that failed the sibling namespace. That row now passes. Nothing else changed, and the round-1 FAIL is left standing as the record.

The head did not move. 5a6936ae49b4632c422b66e914799b2ff03d59bd before and after; the repair was a PR-body edit only — no commit, no push, no repo file touched, so claude-plugins/fabrika/skills/build/contract.md is byte-identical to the text round 1 graded. Read via a per-run ref fetched from pull/5287/head and asserted against the pinned SHA (§HEAD), never from a working copy. Config-pin held: this gate ran on the base review-skill, not the PR's.

Artifact class fan, re-run at head (not inherited): class-probe classify --namespaces over the 4 changed files → has-code, has-skillsreview-code, review-skill. Exit 0, 2 namespaces; review-design not required (live UI_RE='^apps/web/src/' matches nothing here). This is the review-skill marker; review-code is a separate comment on this PR.

Step 0 — blocking or non-blocking: NON-BLOCKING (not-control-plane), re-derived

Re-run rather than carried over. pipeline-cli cp-classify classify against the live CONTROL_PLANE_RE re-resolved from origin/main returns not-control-plane [path-clear-no-content-source]. CONTROL_PLANE_RE covers claude-plugins/kampus-pipeline/ (skills/, lib/, agents/, hooks) only — this PR's one claude-plugins file is claude-plugins/fabrika/skills/build/contract.md, and fabrika is not kampus-pipeline, so it matches no branch; the three packages/fabrika-cli/** files match no packages/ branch. Zero .decisions/** files, so ADR 0164's content clause has no input.

This verdict therefore BINDS: a non-blocking marker, not a §CP advisory. No control-plane approval gates this PR, so a PASS here makes it eligible to auto-ship with no human in the loop. Worth stating outright — the fabrika plugin looks control-plane at a glance and is not, and that mis-read is the one thing that would wrongly downgrade this verdict to advisory.

Carried forward, unchanged and not re-litigated

The contract text is identical to the head round 1 graded, so re-deriving its rows could not change an answer. Standing: both acceptance criteria that reach this class PASS (22 in the shared exit matrix, build check's own exit table and its errors table, with the message string byte-identical to source; the three-file-classes paragraph documenting the mirror direction honestly, including the rejected widening). All five rigor checks PASS — behavioral correctness, trigger/description quality (N/A surface, unchanged), cross-skill conflict, gate-invariant preservation, and the ADR 0248 contract-implementation-ticket check (discharged by the diff itself: 22, UNCLASSIFIED_DIFF and unvalidated ship in this same PR). Specialist fan-out found nothing to route.

[PASS] deviation-disclosure (§DEV)

Canonical Tier-M scan at head: ## Deviations present; 0 suppression/skip lines; 0 removed-assertion lines. None. is replaced by two class-3 entries, each carrying Said / Did / Why / Disposition. Replacing None. rather than appending under it is correct — "repair appends, never replaces" protects prior entries, and round 1's section held none, only the explicit-empty marker.

Entry 1 is true at source, not merely present. I traced ["a.ts", "README.md"] under --surface code through runCheck at head: classifyDiff puts README.md in the markdown bucket, so unvalidatableDiff returns null, surfaceMismatch returns null (code.length === 1), noted takes the unvalidated.length === 0 branch so no stderr disclosure fires, the code branch runs only pnpm typecheck --force and pnpm lint:worktree (whose BIOME_EXT at scripts/biome-worktree.mjs:31 excludes .md), and the answer carries unvalidated: []. Neither scanned nor listed, with an empty unvalidated reading as "nothing uncovered." The entry describes exactly that, naming the bucket and both failure modes — not a softened paraphrase.

This matters more in this namespace than in the sibling. Round 1's finding here was not only that the body was silent — it was that contract.md now asserts unvalidated "is what keeps the partial case honest," which slightly over-states the guarantee while this hole is open. The body entry is what makes that over-statement a disclosed, ticketed gap rather than a documented promise the verb does not keep. That is the specific condition round 1 attached, and it is now met.

Judged on §DEV's three questions: authorized (round 1 ruled the deferral acceptable with standing, conditional on exactly this disclosure); no ADR needed (class 3, not a class-2 governing-ADR departure); follow-up filed#5288, open, type:bug, status:triaged, p1, titled "build check --surface code greens a diff whose markdown it never scans". All three answered. PASS.

Entry 2 — the stale packages/fabrika-cli/src/build/codes.ts:7 range comment — ruled non-blocking, no action needed for this PR. Its disposition was for the reviewer to judge (a sanctioned §DEV value), so I judge it: the sentence is a summary, the authoritative per-constant list sits in the same file and is correct with UNCLASSIFIED_DIFF = 22 as its last line, and a bump would restore truth for one commit before going stale again at 23 — the durable fix is to de-enumerate the range, which is an editorial call for the next PR touching that file. Blocking on it would cost a full re-verification round and a third gate pass to correct one token in a comment that cannot produce a wrong code, exit, or verdict. Full reasoning is in the review-code verdict; recorded here so both namespaces carry the same ruling. Nothing in this item touches the skills-class artifact — contract.md's own tables are correct and complete at 22.

One standing note for the contract, not a repair item

contract.md's partial-green paragraph could eventually name the --surface code-over-markdown carve-out so the document does not over-state its own guarantee. Round 1 raised this as a "cheap extra if the body is open anyway," not as a required row, and I am deliberately not promoting it to blocking at an unchanged head after the author repaired to the stated bar — that is exactly the moving-target churn that makes a gate untrustworthy. It belongs with #5288's fix, which will have to touch this paragraph anyway once the hole closes.


Both acceptance criteria reaching the skills class pass, all five rigor checks pass, and the single blocking row from round 1 is discharged. The §DEV PASS means "nothing undisclosed that this gate could see" — class 3 is Tier D and rests on the author's honesty, corroborated here by tracing the disclosed defect to source.

Merge-ready at 5a6936ae. review-skill does not mergeship-it is the authorized merge step.

Verdict-written: re-gate round 2, prior FAIL 5236192852 left intact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fabrika build check passes green on diffs its two regexes never classify

1 participant