Skip to content

fix(fabrika): prove push containment on the force path, never on the lease alone - #5289

Merged
usirin merged 1 commit into
mainfrom
usirin/5222-fabrika-push-containment-867F92CE
Aug 10, 2026
Merged

fix(fabrika): prove push containment on the force path, never on the lease alone#5289
usirin merged 1 commit into
mainfrom
usirin/5222-fabrika-push-containment-867F92CE

Conversation

@usirin

@usirin usirin commented Aug 10, 2026

Copy link
Copy Markdown
Member

Fixes #5222

fabrika build push guarded its ancestry test with if (before.value !== null && !options.forceWithLease)
— so the one path that force-pushes, the repair path, was the one path with no containment
evidence at all. The documented repair invocation is fabrika build push --force-with-lease,
and on it the verb pushed whatever the lane's head happened to be, then declared success by
comparing the remote against that same head. A head that had dropped the PR's published
commits therefore reported PUSH-VERDICT: MOVED.

--force-with-lease does not close this and cannot. A lease compares the remote against what
this clone last saw of it: it defends the ref against another writer, never against this
lane's own head having dropped the remote's commits. A bare lease is also, per git push's own
documentation, "trivially defeated" by anything that implicitly runs git fetch on the remote
being pushed to — which the repair path does before it rebases.

What changed

  • The containment test runs on both paths. Whenever the target ref already exists, the local
    head must contain the SHA a live git ls-remote just read off it. On the plain path that is
    the fast-forward test and its failure stays 19; on the force path its failure is the new
    proven code 23.
  • --drop-remote-commits is the explicit escape. A deliberate history rewrite says so and
    publishes; the push then records on stderr that it dropped the published head. Silence is no
    longer a way to do it.
  • An unreadable remote head is UNKNOWN (11), never "not contained". git merge-base --is-ancestor needs both commits locally, and a repair lane's published head can be a commit
    this clone has never held. The verb probes for it, fetches <remote>/<ref> once if absent, and
    refuses on 11 if it still cannot hold it — a proven code is reserved for a fact proven
    about two commits it has.
  • The read is live. Containment is compared against ls-remote, not a remote-tracking ref a
    preceding fetch in the same lane may already have refreshed (the trigger recorded on fabrika build: repair path skips stale-base protections; no verb asserts diff surface pre-push #5222 from
    Repair rebase can silently revert a commit pushed by explicit refspec #5263).

23 rather than widening 19: the remedies differ. 19 says "pass the lease"; 23 says
"rebase onto the published head, or say you mean the rewrite". 22 is deliberately skipped — it
is allocated by the in-flight #5229 lane, and a gap costs nothing while a collision costs a
silent double meaning.

Regression coverage

push-verb.unit.test.ts gains four cases, three of which fail against the pre-fix code
(measured: restoring the && !options.forceWithLease guard and re-running gives 3 failed / 11
passed of 14):

  • a --force-with-lease push whose head does not contain the remote head → exit 23, empty
    stdout, no git push spawned, and the refusal names the dropped commits;
  • the same push with --drop-remote-commits → publishes, and says on stderr that it dropped the
    remote head;
  • a force push whose remote head is neither local nor fetchable → exit 11, nothing pushed;
  • a force push that does contain the remote head → still PUSH-VERDICT: MOVED (the guard adds
    no false refusal to the normal repair round).

Contract

claude-plugins/fabrika/skills/build/contract.md carries 23 in the shared exit matrix and in
build push's own exit + error tables, the --drop-remote-commits input row, the containment
paragraph (including why the lease cannot substitute and why an absent object is 11), and the
grounding entries. claude-plugins/fabrika/skills/build/SKILL.md's repair step now tells a lane
what a 23 means: re-build branch --resume, never --drop-remote-commits.

Verification

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

Deviations

Class 3 — acceptance criteria 3, 4 and one third of criterion 5 are deliberately not built
here.

No ADR was authored. The push verb's containment rule is a contract-level fix inside an existing
decision, not a new one; the pre-assigned 0260 is unused.

@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. (76071be)
  • web — Stage pr-5289 torn down.

@usirin

usirin commented Aug 10, 2026

Copy link
Copy Markdown
Member Author

review-code: PASS @ 23c94e9

Gate: review-code · PR #5289 · issue #5222 · head 23c94e9f06a906ed27e7c6536a22a19176274091.
Class probe (pipeline-cli class-probe classify --namespaces, live §CLASS) → 2 namespaces: review-code, review-skill. No review-design (no apps/web/src/** path).

§CP — not control plane, on both axes, verified live. pipeline-cli cp-classify classify against the CONTROL_PLANE_RE re-resolved from origin/main returned not-control-plane [path-clear-no-content-source]: no changed path matched the live boundary (claude-plugins/kampus-pipeline/** is covered; claude-plugins/fabrika/** and packages/fabrika-cli/** are not), and no .decisions/** file is present, so ADR 0164's content clause has nothing to decide. This verdict therefore BINDS — it is the non-blocking auto-merge signal ship-it asserts, not an advisory awaiting a human approval.

Acceptance criteria

  • [PASS] build push verifies containment on the force path. packages/fabrika-cli/src/build/push-verb.ts now runs the ancestry test under a bare if (before.value !== null); the old !options.forceWithLease conjunct is gone. before is a live ls-remote read (remoteSha in packages/fabrika-cli/src/build/git.ts), not a remote-tracking ref — the Repair rebase can silently revert a commit pushed by explicit refspec #5263 trigger carried onto this issue. Containment failure on the force path refuses with the new proven code 23 (HEAD_DROPS_REMOTE, packages/fabrika-cli/src/build/codes.ts), and the refusal names the dropped commits via commitsDropped (a log <local>..<remoteHead> read, capped at 10 with a truncation marker). The deliberate-rewrite escape is a separate, explicitly-named flag, --drop-remote-commits.
  • [PASS] The refusal is a real refusal, reachable on the documented repair invocation. It is a refuse(...) return (non-zero exit, empty stdout, report on stderr), taken before the push is ever attempted, and the covering test drives exactly --force-with-lease, the invocation claude-plugins/fabrika/skills/build/SKILL.md mandates for a repair round. Verified by run: the test asserts no push subprocess was spawned.
  • [N/A] build check's surface classification stops being silently blind to unlisted file classes. Split out of this issue's scope by the intake desk's scope-boundary comment on fabrika build: repair path skips stale-base protections; no verb asserts diff surface pre-push #5222 (2026-08-10T00:37:06Z), which assigns packages/fabrika-cli/src/build/check-verb.ts to fabrika build check passes green on diffs its two regexes never classify #5229 and states section 2 of the body "stays in this body as context, not scope". Read first-party. fabrika build check passes green on diffs its two regexes never classify #5229 is open, type:bug / status:triaged / p1. Ruling: the narrowing is legitimate — different file, different verb, independently fixable, and building it here would double-implement an in-flight lane.
  • [N/A] --surface code no longer refuses a workflow-only diff. Same split, same evidence.
  • [PASS] Unit tests cover a force-with-lease push whose local head does not contain the remote head (refused). push-verb.unit.test.ts gains four cases: the 23 refusal naming the dropped commits; the --drop-remote-commits publish plus stderr record; the unfetchable remote head yielding 11; the contained force push still MOVED. The two build check thirds of this criterion are N/A under the fabrika build check passes green on diffs its two regexes never classify #5229 split, as above.
  • [PASS] The contract section for the changed verb states the new rule, including the exit code. claude-plugins/fabrika/skills/build/contract.md carries 23 in the shared exit matrix, in build push's own exit table and errors table, the --drop-remote-commits input row, the containment paragraph (why the lease cannot substitute, why an absent object is 11), and the fabrika build: repair path skips stale-base protections; no verb asserts diff surface pre-push #5222/Repair rebase can silently revert a commit pushed by explicit refspec #5263 grounding entries.

Regression test — independently reproduced, not taken on the author's word

At head, in an isolated throwaway worktree cut from the PR ref: vitest run push-verb.unit.test.ts14 passed (14). I then restored the pre-fix guard surgically — putting the !options.forceWithLease conjunct back on that one if — and re-ran the same head test file: 3 failed, 11 passed, of 14, exactly the author's reported figure. The three failures are the 23 refusal, the --drop-remote-commits publish record, and the 11 UNKNOWN case (which regressed to 17 — the push went out and the ref did not move). The file was restored from the PR ref afterwards; nothing was committed or published.

Proven-vs-unknown split — checked in both directions

  • A remote head this clone cannot hold is 11 (PRECONDITION_UNKNOWN), never 23. ensureCommitPresent probes with rev-parse --verify --quiet <sha>^{commit}, fetches <remote>/<ref> once if absent, re-probes, and on a still-absent object push-verb.ts returns PRECONDITION_UNKNOWN with cannot prove containmentbefore isAncestor is reached. So a failed read can never become a proven containment verdict.
  • The reverse also holds: a genuine drop can only degrade to 11 (a refusal) if the object read fails, never to a permissive push. Both directions refuse; neither publishes.
  • PRECONDITION_UNKNOWN = 11 and WRITE_UNKNOWN = 8 confirmed against packages/fabrika-cli/src/report/codes.ts. 23 is unique in packages/fabrika-cli/src/build/codes.ts, with 22 deliberately skipped and the skip documented in both codes.ts and the contract.

--drop-remote-commits — scrutinised as an override on a data-loss guard

  • Cannot be defaulted on. Flag.boolean("drop-remote-commits") in packages/fabrika-cli/src/build/command.ts, with no withDefault(true) anywhere. PushOptions declares it a required readonly boolean, so no call site can omit it.
  • Cannot be triggered accidentally. Repo-wide, the string appears in exactly five places: push-verb.ts, command.ts, push-verb.unit.test.ts, and the two skill docs. No skill step, script, or crew prompt invokes itSKILL.md names it only to say never use it on a 23.
  • Cannot bypass the lease. The non-fast-forward-without-lease refusal (19) is evaluated first, so --drop-remote-commits alone still refuses. A destructive publish needs both flags typed explicitly.
  • Not silent. The publish path records --drop-remote-commits given — publishing a head that drops <remote>/<ref> (<sha>) on stderr.
  • Documented. contract.md carries it in the invocation line, the inputs table and the containment paragraph. SKILL.md's repair step tells a lane that a 23 means build branch --resume, never --drop-remote-commits.

Adversarial hunt for remaining silent-drop paths

I looked for any path where a push can drop remote commits and still report success. None found that this PR leaves open. Checked and cleared:

  • before.value === null (remote ref absent) skips the containment test, but the ls-remote refs/heads/<ref> read and the HEAD:refs/heads/<ref> publish use the same namespace and the same <ref>, so a miss on the read means a create on the write — nothing to drop.
  • The publish target is the tracked upstream, and local is rev-parse HEAD while the publish is HEAD:refs/heads/<ref> — the compared SHA and the published SHA are the same object.
  • Success is still after.value === local.value against a second live ls-remote, but it is now reached only after containment is proven or explicitly overridden.

Two residual observations, neither blocking and neither introduced by this PR:

  1. isAncestor folds any subprocess failure into false. merge-base --is-ancestor exiting non-1 (e.g. 128) reads as "not contained" and lands the proven code 23. The direction is safe — it refuses, it never publishes — and both commits are proven resolvable immediately beforehand, so the case needs a corrupt object database. Noted, not blocking.
  2. The containment read and the publish are not atomic. A concurrent writer advancing the ref between the ls-remote read and the publish is still covered only by the bare lease, and ensureCommitPresent's fetch can refresh that lease — though only when the object was absent, and only to the head just read. That is the other-writer hazard the lease exists for, not the this-lane-drops-its-own-work hazard fabrika build: repair path skips stale-base protections; no verb asserts diff surface pre-push #5222 names. Pre-existing, and strictly narrower than before this PR.

Deviation disclosure (§DEV)

  • [PASS] The body carries a literal ## Deviations heading and it is true. The entry is labelled class 3 and is substantively a class 1 scope narrowing. Per §DEV the label is a routing hint and a gate matches the substance, which here is complete: Said / Did / Why / Disposition, naming fabrika build check passes green on diffs its two regexes never classify #5229 and the 2026-08-10T00:37Z scope-boundary comment I read first-party. It is not a falsified None., and the follow-up owner (fabrika build check passes green on diffs its two regexes never classify #5229) is open and in flight. The body separately discloses that no ADR was authored and the pre-assigned 0260 is unused.
  • Walked the other six classes against the diff: no guard bypassed, no pre-existing assertion weakened or deleted (the test diff only adds fixture rows and cases), no out-of-scope file.

Verification run at head (isolated throwaway worktree from the PR ref)

  • vitest run over packages/fabrika-cli195 files, 2812 tests, all passing.
  • tsc --noEmit over packages/fabrika-cli — clean.
  • biome check over the five changed TypeScript files — clean, no fixes applied.
  • CI at this head: every check run success or skipped, zero failing or neutral.
  • Run-evidence: the produce run-evidence bundle job completed success at this head.

Verdict

PASS. Every in-scope acceptance criterion is met with evidence, the regression coverage is independently proven to fail against pre-fix code, the proven/unknown split holds in both directions, and the escape hatch is explicit, double-gated and documented. Not control plane on either axis, so this verdict binds. ship-it owns the merge; this gate does not.

@usirin

usirin commented Aug 10, 2026

Copy link
Copy Markdown
Member Author

review-skill: PASS @ 23c94e9

Gate: review-skill · PR #5289 · issue #5222 · head 23c94e9f06a906ed27e7c6536a22a19176274091.
Skills class of a two-namespace diff (review-code + review-skill, per pipeline-cli class-probe classify --namespaces at this head). Posted as its own comment so the marker sits on this comment's literal first line — the review-code verdict is a separate comment.

§CP — not control plane, on both axes, verified live. pipeline-cli cp-classify classify against the CONTROL_PLANE_RE re-resolved from origin/main: not-control-plane [path-clear-no-content-source]. The live boundary covers claude-plugins/kampus-pipeline/skills/, not claude-plugins/fabrika/**, and the diff carries no .decisions/** file, so ADR 0164's content clause has no input. This is the binding non-blocking marker, not a §CP advisory.

Skill artifacts under review

Acceptance criteria (skills half)

  • [PASS] The contract section for the changed verb states the new rule, including the exit code for the containment refusal. Verified line by line against the implementation at head, not against the PR's prose: 23 in the group exit matrix, in build push's exit table (proven: the local head does not contain the published remote head), in the errors table with the exact refusal string the verb emits, and 11 restated as "the lane's claim could not be read, or the remote head could not be made readable so containment is UNKNOWN". --drop-remote-commits carries a false default in the inputs table. The containment paragraph states why a lease cannot substitute and why an absent object is 11 rather than 23.
  • [N/A] The build check criteria (3, 4 and one third of 5) are out of scope under the intake desk's 2026-08-10T00:37:06Z scope-boundary comment on fabrika build: repair path skips stale-base protections; no verb asserts diff surface pre-push #5222, which assigns them to fabrika build check passes green on diffs its two regexes never classify #5229 (open, type:bug / status:triaged / p1). Read first-party; the narrowing is legitimate.

Skill-rigor checklist

  • [PASS] 1. Behavioral correctness. Traced the changed repair step as an executing agent would. SKILL.md's repair round already prescribes fabrika build push --force-with-lease; the new sentence gives that step the missing exit branch — a 23 routes back to build branch --resume, and explicitly not to --drop-remote-commits. That is the correct remedy: --resume re-fetches the PR's remote head branch and refuses if the fetch fails, so rebuilding on it is what makes the head contain the published head. No control-flow gap: 23 previously had no described handling at all, which is the uncovered-procedure-path the fix closes.
  • [PASS] 2. Trigger / description quality. Neither file's frontmatter description changed, so the trigger surface is untouched and no shadowing is introduced.
  • [PASS] 3. Cross-skill conflict / shadowing. --drop-remote-commits appears in exactly five places repo-wide (push-verb.ts, command.ts, push-verb.unit.test.ts, contract.md, SKILL.md). No other fabrika skill (build-ui, build-epic) or script invokes build push with a flag this changes, and none restates its exit table, so no reader was left un-updated. The exit-code seam is honoured on both sides: 23 is added to codes.ts and to the contract in the same diff, with 22's reservation for the in-flight fabrika build check passes green on diffs its two regexes never classify #5229 lane documented in both — the collision that would matter is prevented, not merely noted.
  • [PASS] 4. Gate-invariant preservation. No §CP path in the diff, so no gate invariant is in reach. Direction of travel is strengthening: the change adds a refusal on a previously unguarded destructive path and adds a distinct code rather than widening 19. The new --drop-remote-commits escape is the only relaxation, and it is opt-in, off by default, unreachable without also passing --force-with-lease, invoked by no skill step, and documented as never-use in the repair round.
  • [PASS] 5. Contract implementation ticket (ADR 0248). In literal scope — the diff changes claude-plugins/fabrika/skills/build/contract.md. Discharged on substance: ADR 0248 exists to stop a contract landing on main "whose verbs do not exist", and this contract change specifies no unbuilt verb. It documents 23, --drop-remote-commits and the containment rule that ship implemented in the same diff (push-verb.ts, git.ts, codes.ts, command.ts, with 14 covering unit tests). There is no authoring-session handoff to ticket. Recording the reasoning rather than a bare PASS, since the check's literal scope test is "a contract.md changed" and this PR meets it.

Specialist fan-out (unreachable-step · contradictory-instruction · uncovered-procedure-path)

No in-scope finding. The one procedural path worth naming — what a lane does when build push refuses 11 (cannot prove containment) — is stated in contract.md but not echoed in SKILL.md's repair step, which covers only 23. It is a documentation nicety on a refusal that already names its own cause on stderr, it does not trace to this issue's stated goal, and it blocks nothing. Recorded here rather than appended as an acceptance criterion.

Doc hygiene

  • Every path cited in both files is repo-relative and resolves. No home, absolute, vault or sibling-repo path in either changed file.
  • The added contract prose matches the shipped strings exactly (I diffed the errors table against the refuse(...) messages in push-verb.ts).
  • contract.md's "the last stdout line is always exactly one of PUSH-VERDICT: MOVED on exit 0" claim survives the new code: 23 and 11 are refusals with empty stdout, so the documented tail -1-on-exit-0 idiom is unchanged.

Deviation disclosure (§DEV)

  • [PASS] The body carries a literal ## Deviations heading and it is true — a complete Said / Did / Why / Disposition entry naming fabrika build check passes green on diffs its two regexes never classify #5229 and the 2026-08-10T00:37Z scope-boundary comment, which I read first-party before ruling. Labelled class 3, substantively class 1; §DEV matches on substance, not the label. Not a falsified None.. No governing-ADR departure in the diff, no guard bypassed, no pre-existing assertion weakened.

Verdict

PASS. Conjunctive across the in-scope acceptance criteria and all five rigor checks. The skill text now tells a repair lane exactly what a 23 means and steers it away from the destructive escape, and the contract states the rule the code enforces, with the exit codes matching the implementation. Not control plane on either axis, so this verdict binds. ship-it owns the merge; this gate does not.

…lease alone

`build push` ran its ancestry test only when `--force-with-lease` was absent, so the
repair path — which mandates the lease — got no containment check at all. The lease
defends the ref against another writer, not against this lane's own head having
dropped the remote's commits, and the verb's success test (remote equals local)
reported the drop as MOVED.

Containment now runs on both paths against a live `ls-remote` read. The force path's
failure is its own proven code `23`, with `--drop-remote-commits` as the explicit
escape for a rewrite a lane actually means. An unavailable remote head object is
UNKNOWN (`11`), never "not contained".

Refs #5222
@usirin
usirin force-pushed the usirin/5222-fabrika-push-containment-867F92CE branch from 23c94e9 to 76071be Compare August 10, 2026 05:57
@usirin

usirin commented Aug 10, 2026

Copy link
Copy Markdown
Member Author

review-code: PASS @ 76071be

Gate: review-code · PR #5289 · issue #5222 · head 76071be8771b42e4851a036aa4d5f30db1ea5a38.

Re-review at a NEW head. The prior verdicts (review-code PASS, review-skill PASS) are bound to 23c94e9f06a906ed27e7c6536a22a19176274091 and are stale by design (ADR 0058). They were read first-party for context; every claim below is re-derived at 76071be8, in an isolated throwaway worktree cut from the PR ref. No session tree was ever switched to the head.

Why the head moved: the shipper refused on mergeable_state: dirty. Sibling PR #5287 (issue #5229) landed on main at 05:37Z touching two files this PR also touches — packages/fabrika-cli/src/build/codes.ts and claude-plugins/fabrika/skills/build/contract.md. The rebase carried two conflict resolutions. The specific risk re-reviewed here is a resolution that silently drops one lane's text while still merging clean. It did not happen; proof below.

Class probe (pipeline-cli class-probe classify --namespaces, live §CLASS) at this head → 7 changed files → 2 namespaces: review-code, review-skill. No review-design (no apps/web/src/** path). Same set as the prior head — re-derived, not reused.

§CP — not control plane, both axes, verified live at this head. pipeline-cli cp-classify classify against the CONTROL_PLANE_RE re-resolved from origin/main: not-control-plane [path-clear-no-content-source] — no changed path matched the live boundary (claude-plugins/kampus-pipeline/** is covered; claude-plugins/fabrika/** and packages/fabrika-cli/** are not), and no .decisions/** file is present, so ADR 0164's content clause has nothing to decide. This verdict therefore BINDS. A PASS here auto-ships with no human approval — stated explicitly, as it should be.

Conflict resolution — verified file by file, not taken on the repairer's word

The method is a diff of the head against current origin/main. Anything #5287 landed that survived the resolution is absent from that diff; anything clobbered would appear as a deletion. There are exactly three deleted lines across both conflicted files, and all three are this lane's own edits.

packages/fabrika-cli/src/build/codes.ts — clean append, nothing clobbered

The whole delta against origin/main is a pure append of the 23 block. Not one landed value is renumbered, and the 22 docblock is byte-identical to main's. Full numbering read off the head:

12 NOT_A_WORKTREE · 13 DIRTY_TREE · 14 WRONG_LANE · 15 CLAIM_NOT_MINE · 16 BLOCKED · 17 REF_NOT_MOVED · 18 VALIDATION_RED · 19 UNSAFE_PUSH · 20 OUT_OF_FOCUS · 21 AUDIENCE_NOT_AGENT · 22 UNCLASSIFIED_DIFF (from main, #5287) · 23 HEAD_DROPS_REMOTE (this lane)

No duplicate, no gap, no collision, and the nine 311 seats are still re-exported from ../report/codes.ts rather than re-typed. The repairer's claimed order is exactly what is there.

claude-plugins/fabrika/skills/build/contract.md — BOTH lanes' prose survives

Read first-party at the head. #5287's contributions, each confirmed present:

  • the **Amended 2026-08-10** header note for fabrika build check passes green on diffs its two regexes never classify #5229 (line 7), alongside the pre-existing **Amended 2026-08-09** note;
  • its 22 row in the shared group exit matrix (line 252);
  • its build check verb block — the --surface inputs row (985), 22 in check's own exit table (1033), the 10 mismatch error row (1042), and the #5229 grounding entry (1062).

This lane's contributions, each confirmed present:

  • 23 in the shared exit matrix, immediately after 22;
  • the build push invocation line carrying [--drop-remote-commits] and its inputs-table row;
  • the three containment paragraphs (why the lease cannot substitute, why an absent object is 11 and not 23, why the read must be a live remote read);
  • 23 and the amended 11 in build push's exit table, plus both new error-table rows;
  • the #5222 and #5263 grounding entries.

"It merged without markers" was not accepted as evidence; the surviving text was read.

The one deliberate deletion — ruled CORRECT

The pre-rebase head carried a paragraph stating 22 was "deliberately skipped: allocated on a lane in flight (#5229)". The repairer removed it. I rule the deletion correct, and it was the only defensible resolution. Reasoning, since this was a judgment call rather than a mechanical merge:

  • Once 22 is literally defined on the line above, the sentence is not stale-but-harmless — it is false, and it is false in an agent-read contract. contract.md is the artifact a fabrika lane reads to decide what an exit code means; a line saying a live, allocated, shipped code is "skipped" is a wrong instruction, not a dated footnote. The asymmetry is decisive: a missing note costs a reader nothing, a false one costs a reader a wrong action.
  • The note's entire job was collision-avoidance between two in-flight lanes. fabrika build check passes green on diffs its two regexes never classify #5229 is now closed and its code landed. The job is done; the note has no remaining referent.
  • Deleting it loses no information a reader needs — 22's own docblock in codes.ts and its two contract rows carry the whole meaning.

Verified the removal is complete and consistent: no 22-is-skipped residue remains in contract.md, codes.ts, or SKILL.md.

Acceptance criteria of #5222 — the rebase changed the answer on three of them

The prior gate ruled AC 3, 4 and the build check thirds of AC 5 [N/A] under the intake desk's 2026-08-10T00:37:06Z scope-boundary split to #5229. #5229 is now closed and #5287 landed on main, which is this PR's base. So those criteria are no longer deferred — they are satisfied at this PR's merge target, verified at this head:

  • [PASS] build push verifies containment on the force path. push-verb.ts runs the ancestry test under a bare if (before.value !== null); the !options.forceWithLease conjunct is gone. before is a live remote read (remoteSha, git.ts), not a remote-tracking ref — the Repair rebase can silently revert a commit pushed by explicit refspec #5263 trigger. Containment failure on the force path refuses with the proven code 23, and the refusal names the dropped commits via commitsDropped, capped at 10 with a truncation marker. The escape is a separate, explicitly-named flag.
  • [PASS] The refusal is a real refusal, reachable on the documented repair invocation. A refuse(...) return — non-zero exit, empty stdout, report on stderr — taken before any push. The covering test drives exactly --force-with-lease, the invocation SKILL.md mandates for a repair round, and asserts no push subprocess was spawned.
  • [PASS-on-target] build check's surface classification stops being silently blind to unlisted file classes. Verified at this head: check-verb.ts calls unvalidatableDiff(files) and refuses on UNCLASSIFIED_DIFF (22), and a partly-unvalidatable green now discloses the files it did not cover. Delivered by fix(fabrika): refuse a build check diff no surface validates, never green it #5287, present in this PR's base. Not this PR's code — stated plainly — but the criterion is met at the commit that would merge.
  • [PASS-on-target] --surface code no longer refuses a workflow-only diff on the grounds that it "changes no code file". Met in substance at head: a workflow-only diff under --surface code now refuses on 22 "naming the honest reason" (check-verb.unit.test.ts:220) rather than on the false changes no code file ground. It still refuses — correctly, there is nothing to run — but on an honest, distinct code. That is fabrika build check passes green on diffs its two regexes never classify #5229's ruled design.
  • [PASS] Unit tests cover a force-with-lease push whose local head does not contain the remote head (refused). push-verb.unit.test.ts gains four cases: the 23 refusal naming dropped commits; the --drop-remote-commits publish plus its stderr record; the unfetchable remote head yielding 11; the contained force push still MOVED. The build check thirds are covered at head by check-verb.unit.test.ts (three 22 cases at 206/215/220, two disclosure cases at 231/244).
  • [PASS] The contract section for the changed verb states the new rule, including the exit code. Confirmed line by line above.

Regression guard — INDEPENDENTLY re-proved at the new head

A rebase is exactly when a guard silently stops guarding, so this was re-derived, not carried over. At 76071be8, in the throwaway worktree, I restored the pre-fix guard surgically — putting && !options.forceWithLease back on that one if — and re-ran the head's own test file:

× refuses on 23 when the force-path head does not contain the remote head, and pushes nothing
× publishes the dropping head only when --drop-remote-commits says so, and says it did
× refuses on 11 when the remote head is unreadable locally — UNKNOWN, never 'not contained'
AssertionError: expected 17 to be 23
AssertionError: expected false to be true
AssertionError: expected 17 to be 11
Tests  3 failed | 11 passed (14)

The first failure is the data-loss defect itself, reproduced. expected 17 to be 23 means that with the guard removed the verb did not refuse — it spawned the push, the destructive publish went out, and the run only failed later on 17 (the remote ref did not move) because the fake remote never advanced. That is the live defect #5222 names: the drop happens, and on a real remote the verb's own success test would then have reported PUSH-VERDICT: MOVED. The guard at this head prevents it. The file was restored from the PR ref afterwards; the worktree is clean, nothing was committed or published.

Verification run at head (isolated throwaway worktree from the PR ref)

  • Full vitest run over packages/fabrika-cli195 files, 2819 tests, all passing. Exactly the predicted count: 2812 + 7 from fix(fabrika): refuse a build check diff no surface validates, never green it #5287's landed build check tests. No unexplained delta.
  • tsc --noEmit over packages/fabrika-cli — clean, exit 0.
  • biome check over the five changed TypeScript files — clean, no fixes applied.
  • Leak scan over the whole diff — no home, absolute, vault or sibling-repo path.
  • CI at this head: zero failing or neutral check runs; 39 success, 4 skipped, unit + client tests still in_progress at the time of this verdict. ship-it is the terminal green gate and enforces this independently.

The two disclosed residuals — confirmed NOT widened by the rebase

Not re-litigated, per the prior gate's disclosure; only checked that the rebase left them where they were.

  1. isAncestor folds any subprocess failure into false. Re-read at head: it is still return r.ok, byte-identical to main's — the rebase touched neither the function nor its call site's ordering. Unchanged, still safe-direction (it refuses, never publishes), and both commits are still proven resolvable immediately beforehand by ensureCommitPresent.
  2. The containment read and the publish are not atomic. Re-read at head: the read → ancestry → publish ordering in push-verb.ts is unchanged by the rebase, and no new read was inserted between them. Still the other-writer hazard the lease exists for, not the this-lane-drops-its-own-work hazard fabrika build: repair path skips stale-base protections; no verb asserts diff surface pre-push #5222 closes. Not widened.

Non-blocking observations

  • codes.ts's top docblock still says the group "adds its own 12-21", now that 22 and 23 exist. This staleness is inherited from main — verified against origin/main, where fix(fabrika): refuse a build check diff no surface validates, never green it #5287 added 22 without touching it — so it is not introduced by this PR and is not this lane's to fix. Worth a follow-up on the group's own file, not a bounce here.
  • The PR body's narrative still says 22 "is deliberately skipped — it is allocated by the in-flight fabrika build check passes green on diffs its two regexes never classify #5229 lane", and still reports 2812 tests. Both are pre-rebase statements. Non-blocking, and deliberately not a bounce: the body is a historical record of the change, not an agent-read contract, and the false line was correctly removed from the artifact that is one. Costing a fresh head and a full re-review cycle to edit prose that ships no behavior is the wrong trade. The ## Deviations section — the part §DEV governs — is untouched by this and still true.

Deviation disclosure (§DEV)

  • [PASS] The body carries a literal ## Deviations heading and it is true at this head. The entry is labelled class 3 and is substantively a class 1 scope narrowing; §DEV matches on substance, and the substance is complete — Said / Did / Why / Disposition, naming fabrika build check passes green on diffs its two regexes never classify #5229 and the 2026-08-10T00:37Z scope-boundary comment, which I read first-party. Not a falsified None..
  • One honest note on it: the disclosure says fabrika build check passes green on diffs its two regexes never classify #5229 is "open and in flight". At this head it is closed, and its work is in this PR's base. That makes the disclosed narrowing narrower than disclosed, never wider — the deviation shrank rather than grew, which is the safe direction and not a §DEV defect.
  • Walked the other six classes against the diff: no guard bypassed, no pre-existing assertion weakened or deleted (the test diff only adds cases and fixture rows), no out-of-scope file, no governing-ADR departure.
  • No unresolved review threads on this PR (unresolved-threads-read.sh, exit 0 — the read ran).

Verdict

PASS @ 76071be8771b42e4851a036aa4d5f30db1ea5a38. The rebase is clean on both conflicted files, with both lanes' text proven present rather than assumed; the one deliberate deletion is correct and complete; every acceptance criterion of #5222 is met at the merge target; and the data-loss guard was independently re-proved to fail against pre-fix code at this exact head, with the destructive publish reproduced. Not control plane on either axis, so this verdict binds and this PR auto-ships on it. ship-it owns the merge and the terminal CI-green check; this gate does not merge.

Verdict-written: 2026-08-10T06:09:10Z

@usirin

usirin commented Aug 10, 2026

Copy link
Copy Markdown
Member Author

review-skill: PASS @ 76071be

Gate: review-skill · PR #5289 · issue #5222 · head 76071be8771b42e4851a036aa4d5f30db1ea5a38.

Skills class of a two-namespace diff (review-code + review-skill, per pipeline-cli class-probe classify --namespaces re-run at this head — 7 files, has-code + has-skills, no review-design). Posted as its own comment so the marker sits on this comment's literal first line; the review-code verdict is a separate comment.

Re-review at a NEW head. The prior review-skill PASS is bound to 23c94e9f and is stale by design (ADR 0058). It was read first-party for context; every claim here is re-derived at 76071be8 from an isolated throwaway worktree cut from the PR ref.

§CP — not control plane, both axes, verified live at this head. pipeline-cli cp-classify classify against the CONTROL_PLANE_RE re-resolved from origin/main: not-control-plane [path-clear-no-content-source]. The live boundary covers claude-plugins/kampus-pipeline/skills/, not claude-plugins/fabrika/**, and the diff carries no .decisions/** file, so ADR 0164's content clause has no input. This is the binding non-blocking marker, not a §CP advisory — a PASS auto-ships with no human approval.

Skill artifacts under review

  • claude-plugins/fabrika/skills/build/contract.md — one of the two files the rebase had to resolve.
  • claude-plugins/fabrika/skills/build/SKILL.md — not a conflicted file; its repair step gained the 23 branch.

The rebase resolution in contract.md — both lanes' text confirmed present

This is the risk the re-review exists for: two independent fixes appending to the same exit matrix, where a careless resolution drops one lane's prose and still merges clean. Verified by diffing the head against current origin/main — anything #5287 landed that survived is absent from that diff, anything clobbered would show as a deletion. Exactly one line is deleted from contract.md, and it is this lane's own 11 row being amended.

#5287's text, each item read at the head: the **Amended 2026-08-10** header note for #5229 (line 7), sitting alongside the earlier **Amended 2026-08-09** note; its 22 row in the shared exit matrix (252); and its whole build check block — the --surface inputs row (985), 22 in check's own exit table (1033), the 10 mismatch error row (1042), the #5229 grounding entry (1062).

This lane's text, each item read at the head: the 23 matrix row; [--drop-remote-commits] in the build push invocation line plus its inputs row; the three containment paragraphs; 23 and the amended 11 in build push's exit table; both new error-table rows quoting the verb's exact refusal strings; the #5222 and #5263 grounding entries.

The one deliberate deletion — ruled CORRECT. The pre-rebase head carried a note that 22 was "deliberately skipped: allocated on a lane in flight (#5229)". Once 22 is literally defined on the line above, that sentence is not stale-but-harmless — it is false in an agent-read contract, which is the artifact a fabrika lane consults to decide what an exit code means. A missing note costs a reader nothing; a false one costs a reader a wrong action. Its only job was collision-avoidance between two in-flight lanes, and #5229 is now closed with its code landed, so it has no remaining referent. Removing it is right, and the removal is complete: no 22-is-skipped residue in contract.md, codes.ts or SKILL.md.

Acceptance criteria (skills half)

  • [PASS] The contract section for the changed verb states the new rule, including the exit code for the containment refusal. Re-verified line by line against the implementation at head, not against the PR's prose: 23 in the group matrix, in build push's exit table ("proven: the local head does not contain the published remote head"), and in the errors table carrying the exact refusal string the verb emits. 11 is restated as "the lane's claim could not be read, or the remote head could not be made readable so containment is UNKNOWN". --drop-remote-commits carries a false default in the inputs table. The containment paragraphs state why a lease cannot substitute and why an absent object is 11 rather than 23.
  • [PASS-on-target] The build check criteria (3, 4 and one third of 5) were [N/A] at the prior head under the intake desk's 2026-08-10T00:37:06Z scope split to fabrika build check passes green on diffs its two regexes never classify #5229. fabrika build check passes green on diffs its two regexes never classify #5229 is now closed and fix(fabrika): refuse a build check diff no surface validates, never green it #5287 is in this PR's base, so the contract's build check block at this head already documents the third file class. They are met at the merge target rather than deferred.

Skill-rigor checklist — re-run at this head

  • [PASS] 1. Behavioral correctness. Traced the changed repair step as an executing agent would. SKILL.md's repair round prescribes fabrika build push --force-with-lease; the added sentence gives that step the exit branch it lacked — a 23 routes back to build branch --resume, explicitly not to --drop-remote-commits. That is the correct remedy: --resume re-fetches the PR's remote head branch and refuses if that fetch fails, so rebuilding on it is what makes the head contain the published head. 23 previously had no described handling at all; the uncovered procedure path is closed.
  • [PASS] 2. Trigger / description quality. Neither file's frontmatter changed in the diff against main. The trigger surface is untouched and no shadowing is introduced.
  • [PASS] 3. Cross-skill conflict / shadowing. --drop-remote-commits appears in exactly five places repo-wide (push-verb.ts, command.ts, push-verb.unit.test.ts, contract.md, SKILL.md). No fabrika skill or script invokes build push with a flag this changes, and none restates its exit table, so no reader is left un-updated. The exit-code seam is honoured on both sides at this head: 23 is in codes.ts and in the contract in the same diff, and 22 — the sibling lane's code — is intact in both, from main.
  • [PASS] 4. Gate-invariant preservation. No §CP path in the diff, so no gate invariant is in reach. Direction of travel is strengthening: a refusal is added on a previously unguarded destructive path, with a distinct code rather than a widened 19. The --drop-remote-commits escape is the only relaxation, and it is opt-in, off by default, unreachable without also passing --force-with-lease (the 19 refusal is evaluated first), invoked by no skill step, records the drop on stderr, and is documented as never-use in the repair round. Re-checked at this head, unchanged by the rebase.
  • [PASS] 5. Contract implementation ticket (ADR 0248). In literal scope — the diff changes contract.md. Discharged on substance: ADR 0248 exists to stop a contract landing whose verbs do not exist, and this change specifies no unbuilt verb. It documents 23, --drop-remote-commits and the containment rule that ship in the same diff (push-verb.ts, git.ts, codes.ts, command.ts, with 14 covering unit tests, all green at this head). There is no authoring-session handoff to ticket.

Doc hygiene

  • Every path cited in both files is repo-relative and resolves. No home, absolute, vault or sibling-repo path anywhere in the diff (scanned).
  • The added contract prose still matches the shipped strings exactly — the errors table was re-diffed against the refuse(...) messages in push-verb.ts at this head, post-rebase.
  • contract.md's "the last stdout line is always exactly one of PUSH-VERDICT: MOVED on exit 0" claim survives both lanes: 23 and 11 are refusals with empty stdout, and fix(fabrika): refuse a build check diff no surface validates, never green it #5287's 22 is a refusal too, so the documented tail -1-on-exit-0 idiom is unchanged.

Specialist fan-out (unreachable-step · contradictory-instruction · uncovered-procedure-path)

No in-scope finding. The one procedural path worth naming is unchanged from the prior head: what a lane does when build push refuses 11 ("cannot prove containment") is stated in contract.md but not echoed in SKILL.md's repair step, which covers only 23. It is a documentation nicety on a refusal that already names its own cause on stderr, it does not trace to this issue's stated goal, and it blocks nothing. Recorded here rather than appended as an acceptance criterion.

Deviation disclosure (§DEV)

  • [PASS] The body carries a literal ## Deviations heading and it is true at this head — a complete Said / Did / Why / Disposition entry naming fabrika build check passes green on diffs its two regexes never classify #5229 and the 2026-08-10T00:37Z scope-boundary comment, which I read first-party. Labelled class 3, substantively class 1; §DEV matches on substance, not the label. Not a falsified None..
  • Honest note: the disclosure calls fabrika build check passes green on diffs its two regexes never classify #5229 "open and in flight"; at this head it is closed and its work is in this PR's base. The disclosed narrowing therefore shrank rather than grew — the safe direction, and not a §DEV defect.
  • No governing-ADR departure in the diff, no guard bypassed, no pre-existing assertion weakened. No unresolved review threads on this PR.

Verdict

PASS @ 76071be8771b42e4851a036aa4d5f30db1ea5a38. The rebase preserved both lanes' contract prose — proven by reading the surviving text, not by the absence of conflict markers — the one deliberate deletion removed a statement the rebase had made false, and the skill text still tells a repair lane exactly what a 23 means and steers it away from the destructive escape. Conjunctive across the in-scope criteria and all five rigor checks. Not control plane on either axis, so this verdict binds. ship-it owns the merge; this gate does not.

Verdict-written: 2026-08-10T06:11:13Z

@usirin
usirin added this pull request to the merge queue Aug 10, 2026
Merged via the queue into main with commit 2d78b05 Aug 10, 2026
46 checks passed
@usirin
usirin deleted the usirin/5222-fabrika-push-containment-867F92CE branch August 10, 2026 06:20
@github-actions github-actions Bot mentioned this pull request Aug 10, 2026
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: repair path skips stale-base protections; no verb asserts diff surface pre-push

1 participant