fix(fabrika): prove push containment on the force path, never on the lease alone - #5289
Conversation
No preview deploy
|
|
review-code: PASS @ 23c94e9 Gate: §CP — not control plane, on both axes, verified live. Acceptance criteria
Regression test — independently reproduced, not taken on the author's wordAt head, in an isolated throwaway worktree cut from the PR ref: Proven-vs-unknown split — checked in both directions
|
|
review-skill: PASS @ 23c94e9 Gate: §CP — not control plane, on both axes, verified live. Skill artifacts under review
Acceptance criteria (skills half)
Skill-rigor checklist
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 Doc hygiene
Deviation disclosure (§DEV)
VerdictPASS. Conjunctive across the in-scope acceptance criteria and all five rigor checks. The skill text now tells a repair lane exactly what a |
…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
23c94e9 to
76071be
Compare
|
review-code: PASS @ 76071be Gate: Re-review at a NEW head. The prior verdicts ( Why the head moved: the shipper refused on Class probe ( §CP — not control plane, both axes, verified live at this head. Conflict resolution — verified file by file, not taken on the repairer's wordThe method is a diff of the head against current
|
|
review-skill: PASS @ 76071be Gate: Skills class of a two-namespace diff ( Re-review at a NEW head. The prior §CP — not control plane, both axes, verified live at this head. Skill artifacts under review
The rebase resolution in
|
Fixes #5222
fabrika build pushguarded its ancestry test withif (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-leasedoes not close this and cannot. A lease compares the remote against whatthis 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 owndocumentation, "trivially defeated" by anything that implicitly runs
git fetchon the remotebeing pushed to — which the repair path does before it rebases.
What changed
head must contain the SHA a live
git ls-remotejust read off it. On the plain path that isthe fast-forward test and its failure stays
19; on the force path its failure is the newproven code
23.--drop-remote-commitsis the explicit escape. A deliberate history rewrite says so andpublishes; the push then records on stderr that it dropped the published head. Silence is no
longer a way to do it.
11), never "not contained".git merge-base --is-ancestorneeds both commits locally, and a repair lane's published head can be a committhis clone has never held. The verb probes for it, fetches
<remote>/<ref>once if absent, andrefuses on
11if it still cannot hold it — a proven code is reserved for a fact provenabout two commits it has.
ls-remote, not a remote-tracking ref apreceding 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).
23rather than widening19: the remedies differ.19says "pass the lease";23says"rebase onto the published head, or say you mean the rewrite".
22is deliberately skipped — itis 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.tsgains four cases, three of which fail against the pre-fix code(measured: restoring the
&& !options.forceWithLeaseguard and re-running gives 3 failed / 11passed of 14):
--force-with-leasepush whose head does not contain the remote head → exit23, emptystdout, no
git pushspawned, and the refusal names the dropped commits;--drop-remote-commits→ publishes, and says on stderr that it dropped theremote head;
11, nothing pushed;PUSH-VERDICT: MOVED(the guard addsno false refusal to the normal repair round).
Contract
claude-plugins/fabrika/skills/build/contract.mdcarries23in the shared exit matrix and inbuild push's own exit + error tables, the--drop-remote-commitsinput row, the containmentparagraph (including why the lease cannot substitute and why an absent object is
11), and thegrounding entries.
claude-plugins/fabrika/skills/build/SKILL.md's repair step now tells a lanewhat a
23means: re-build branch --resume, never--drop-remote-commits.Verification
pnpm typecheck— 31/31 tasks green.pnpm lint:worktree— clean.pnpm vitest runinpackages/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.
build check's surface classification("stops being silently blind to unlisted file classes", "
--surface codeno longer refuses aworkflow-only diff") and the two matching unit tests.
packages/fabrika-cli/src/build/push-verb.tsand itsdependencies.
check-verb.tsis untouched.of that body out to fabrika build check passes green on diffs its two regexes never classify #5229 and states it "stays in this body as context, not scope" —
explicitly because a coder could otherwise read it as work to do. fabrika build check passes green on diffs its two regexes never classify #5229 is open and in flight.
Building it here would double-implement it.
record, not the follow-up filing.
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
0260is unused.