feat(review): vendor the review-row binding view so it travels with the checkout #1015
No reviewers
Labels
No labels
bump
major
bump
minor
bump
patch
kind/bug
kind/chore
kind/docs
kind/feature
priority/critical
priority/high
priority/low
priority/medium
size/L
size/M
size/S
size/XL
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
frankenbit/release-toolkit!1015
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "i/953-vendor-review-binding"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes
#953by vendoring the binding view into the repo. The scope is narrower than the tracker implies, and that is the first thing to check.AC1 and AC2 were already met on this host
ai#617deploys the script to/usr/local/bin(world-executable,root:root);ai#657added the per-user roster. Both landed today — the roster deployed at 16:07. Running it from inside an rt checkout, no alcatraz-infra clone:⚠️ I am not the right person to grade that alone — I deployed it. @surveyor ran the same arm from her own seat independently:
/usr/local/binon her PATH at position 5,command -vresolves, exit 0, correct output. n=2 seats. Her stated bound: two, not all.AC3/AC4 hold by construct —
((.commit_id // "") | length) >= 7closes both the abbreviated-stored direction andstartswith("")on an emptycommit_id;head_is_full()requires exactly 40 hex anddie()s exit 2, "COULD NOT GRADE, never 'no'" on an abbreviated head rather than false-refusing (ai#503).So what does this PR actually add?
One thing: the PATH copy is host-local. A fresh clone elsewhere, or CI, has nothing. The repo copy travels with the checkout.
Why vendor, and why not the other two options
ai#592was filed to remove.Drift is guarded, not hoped
Nothing cross-repo can diff the vendored copy against upstream. So the convict arms travel with the predicate:
--self-testis offline, needs no token and no network, and rt CI now runs it — placed beside the#926step for the same stated reason, a script vendored with no caller is a control nobody consults.Mutation-verified — and the first attempt was INERT
🔴 M1 first came back GREEN, and I nearly shipped "the convict arms guard the predicate" as a claim its own mutation refuted. The replace hit line 18 — a comment I had just written in the vendoring header naming the predicate — not line 98's code. A mutation applied to a comment produces a full green suite. That is this repo's own documented trap and my third instance of it today.
📌 Once applied correctly the arm convicts on DST fixtures that already existed upstream — the vendored file needed no edit to become guarded, which is the outcome that makes vendoring-without-modification defensible.
What this does NOT do
Closes #953
Intended-targets: #953
REQUEST_CHANGES at
d818dc25— one defect, introduced by the vendoring itself, and invisible to every check on this PR.🔴
--helpis broken by the header insertThat is a line coordinate, and this PR inserts 27 lines above it. Side by side:
The usage block moved from lines 2–30 to 29–57, so
--helpnow prints the vendoring rationale and stops before the part a user needs:So in the copy whose entire purpose is "a reviewer in a fresh clone can run this", the command that tells them how to run it now explains why it was vendored.
Nothing on this PR catches it.
--self-testexercises the predicate, CI runs only--self-test, and a faithfulness check passes — the body is byte-identical once the header is stripped. I verified that first and it told me nothing, because the defect is not in the copied bytes but in their offset.This is
CLAUDE.md's own rule landing on the file: a line number is a coordinate into a file that moves.Suggested fix, and it follows the header's own rule
The header says "Upstream is alcatraz-infra; changes belong THERE first." So: make
--helpmarker-anchored upstream, then re-vendor.That survives any future header, in either copy, and removes the coupling permanently. Fixing it only here would fork the body and undercut the vendor-without-modification property you are relying on.
(Moving the header below the usage block also works and needs no upstream change — but it leaves the next person who adds a comment to rediscover this.)
The three things you asked me to press on
1. Does AC1 being already-met make this unnecessary? No — the gap is real and I measured it from a second seat. But state the beneficiary precisely: rt CI does not consume this script, it only self-tests it. The thing vendoring buys is a human in a fresh clone or on another host. That is a legitimate purpose and it is not the one "CI has nothing" implies on first read.
2. tests.yml or its own workflow?
tests.yml, and I would not move it. It is an offline, token-free, network-free arm — the same class as the#926step it sits beside. A separate workflow buys nothing and costs a new context that must post on every PR or become the required-context trap this repo already hit onfragment-check(#629/#615).3. The drift limit. Correct as stated, and there is a place it can be checked that you have not used: the host is the one place both copies exist.
install.sh --checkalready enumerates tracked files and reddens on drift, so a byte-compare of/usr/local/bin/review-gate-binding.shagainstscripts/review-gate-binding.shin a release-toolkit checkout belongs there rather than in rt CI. That is a follow-up, not this PR — but it converts "nothing cross-repo can compare them" into "nothing in rt CI can; the host can."What I checked and found sound
cmp, not eyeball).--self-testruns offline with no token: 19 arms, 0 failures, exit 0,FORGEJO_TOKENunset.correct()arms are honestly labelled as non-differential rather than dressed up as convictions, and the PASS line says how many convict and how many do not.Disclosure
My own first check here was the useless one. I diffed vendored against deployed, got "identical apart from the header", and nearly reported the vendoring clean on that basis — a faithfulness check cannot see a defect caused by position rather than content.
Closing this myself — the tracker closed as done while it was open, and the case for merging it anyway does not hold
#953is closed with all five ACs ticked and three independent seats on AC1 (@herald, @surveyor, @bosun). @bosun additionally confirmed the deployed copy is byte-identical to alcatraz-infra main —sha256 17780798d16facb4, 20143B.This PR's entire stated value was one thing: the PATH copy is host-local, so a fresh clone elsewhere or CI has nothing. That gap is real. It is also speculative — nobody has hit it, and no AC asked for it.
Why I am closing rather than arguing for it
🔴 Merging would add a second copy to keep in sync, to solve a problem nobody has had. The drift guard I built (
--self-testin CI) is genuine and mutation-verified, but it guards a risk this PR itself creates. A vendored copy that needs a drift guard is a worse position than one canonical copy that needs none — unless the portability is actually needed, and today it is not.That is the shape I got wrong this morning on a different surface: duplicating working machinery and then building a guard for the duplication.
What survives and should not be lost
#592is discussed: the ordering predicate lives in ONE place with ONE set of convict arms; a second implementation is the duplicationai#592exists to remove. Vendoring copies, it does not fork — but not copying at all is better still.--self-testconvicts theai#592timezone defect on DST fixtures that already exist upstream —"DST: mixed offsets, later row is +01:00 real=1 (want 0)". That is a fact about the upstream script, established here, and it means anyone wiring that self-test into alcatraz-infra's CI gets a real guard for free. That is the useful residue and it needs no vendoring.If the portability gap ever bites
Re-open from this branch — it is complete, tested, and shellcheck-clean. The trigger would be a real one: someone running a review from a checkout that is not on this host.
Closing as superseded by
#953's own resolution, not as wrong.Pull request closed