feat(review): vendor the review-row binding view so it travels with the checkout #1015

Closed
herald wants to merge 1 commit from i/953-vendor-review-binding into main
Owner

Closes #953 by 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#617 deploys the script to /usr/local/bin (world-executable, root:root); ai#657 added the per-user roster. Both landed today — the roster deployed at 16:07. Running it from inside an rt checkout, no alcatraz-infra clone:

review-gate-binding.sh frankenbit release-toolkit 1011
  rows (newest per user)
      sentry  APPROVED  official=true  BOUND  cid=df1e307c

⚠️ 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/bin on her PATH at position 5, command -v resolves, exit 0, correct output. n=2 seats. Her stated bound: two, not all.

AC3/AC4 hold by construct — ((.commit_id // "") | length) >= 7 closes both the abbreviated-stored direction and startswith("") on an empty commit_id; head_is_full() requires exactly 40 hex and die()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

option verdict
PATH utility already done, sufficient here, does not survive leaving the host
thin wrapper still has to find what it wraps — inherits the same dependency one layer up
reimplement in Go 🔴 rejected outright — the ordering predicate exists in ONE place with ONE set of convict arms. A second implementation is the exact duplication ai#592 was 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-test is offline, needs no token and no network, and rt CI now runs it — placed beside the #926 step 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  string-sort ordering (the ai#592 defect)   REDDENS 2 arms
      "DST: mixed offsets, later row is +01:00   real=1 (want 0)"
      "DST: same pair, polarity reversed        real=0 (want 1)"
M2  drop the >= 7 commit_id guard               REDDENS, exit 1

🔴 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

  • It does not remove the PATH copy. Both exist; the repo copy is the portable one.
  • It cannot detect upstream drift directly — only a predicate edit that breaks the arms. A cosmetic upstream change would go unnoticed here.
  • The self-test covers the PREDICATE, not the fetch path — it says so itself: "does NOT test the fetch path, the token, the API's field semantics."

Closes #953

Intended-targets: #953

Closes `#953` by 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#617` deploys the script to `/usr/local/bin` (world-executable, `root:root`); `ai#657` added the per-user roster. **Both landed today** — the roster deployed at 16:07. Running it from inside an rt checkout, no alcatraz-infra clone: ``` review-gate-binding.sh frankenbit release-toolkit 1011 rows (newest per user) sentry APPROVED official=true BOUND cid=df1e307c ``` ⚠️ **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/bin` on her PATH at position 5, `command -v` resolves, exit 0, correct output. **n=2 seats.** Her stated bound: two, not all. AC3/AC4 hold by construct — `((.commit_id // "") | length) >= 7` closes both the abbreviated-stored direction *and* `startswith("")` on an empty `commit_id`; `head_is_full()` requires exactly 40 hex and `die()`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 | option | verdict | |---|---| | PATH utility | already done, sufficient here, **does not survive leaving the host** | | thin wrapper | still has to find what it wraps — inherits the same dependency one layer up | | **reimplement in Go** | 🔴 **rejected outright** — the ordering predicate exists in ONE place with ONE set of convict arms. A second implementation is the exact duplication `ai#592` was 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-test` is offline, needs no token and no network, and rt CI now runs it — placed beside the `#926` step 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 string-sort ordering (the ai#592 defect) REDDENS 2 arms "DST: mixed offsets, later row is +01:00 real=1 (want 0)" "DST: same pair, polarity reversed real=0 (want 1)" M2 drop the >= 7 commit_id guard REDDENS, exit 1 ``` 🔴 **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 - **It does not remove the PATH copy.** Both exist; the repo copy is the portable one. - **It cannot detect upstream drift directly** — only a predicate edit that breaks the arms. A cosmetic upstream change would go unnoticed here. - **The self-test covers the PREDICATE, not the fetch path** — it says so itself: *"does NOT test the fetch path, the token, the API's field semantics."* Closes #953 Intended-targets: #953
feat(review): vendor the review-row binding view so it travels with the checkout
Some checks failed
fork-pr-approval-notice / explain fork workflow approval (pull_request_target) Successful in 3s
ac-closure-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 5s
ac-closure-check / ac-closure check (pull_request) Failing after 6s
ac-closure-check / check (pull_request) Failing after 0s
changelog-body-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 4s
changelog-body-check / changelog body Cold-Read linter (pull_request) Successful in 6s
changelog-body-check / check (pull_request) Successful in 0s
check-self-bootstrap / check (pull_request) Successful in 5s
fragment-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 9s
fragment-check / changelog fragment-kind (pull_request) Successful in 7s
fragment-check / check (pull_request) Successful in 0s
go-ci / lint + build + test (pull_request) Successful in 27s
manifest-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 4s
manifest-check / manifest-vs-tag consistency (pull_request) Successful in 7s
manifest-check / check (pull_request) Successful in 0s
register-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 4s
register-check / register-drift check (pull_request) Successful in 7s
register-check / check (pull_request) Successful in 0s
tests / workflow-schema (pull_request) Successful in 3s
tests / bats (pull_request) Successful in 13s
tests / dated-examples (pull_request) Successful in 4s
tests / shellcheck (pull_request) Successful in 5s
workflow-parse-check / toolkit-self parse guard and controls (pull_request) Successful in 3s
workflow-parse-check / workflow parse and schema (pull_request) Successful in 4s
workflow-parse-check / check (pull_request) Successful in 0s
d818dc25d0
rt#953. The script answers the one question a reviewer cannot answer by eye —
does any live approval BIND to the head about to merge — by taking the newest
row per user, then state, official, and commit_id against that head.

WHAT WAS ALREADY TRUE, stated because it decides the scope:

AC1 and AC2 are already met ON THIS HOST. ai#617 deploys the script to
/usr/local/bin world-executable, and ai#657 added the per-user roster; both
landed today and the roster deployed at 16:07. Verified from TWO independent
seats — mine and @surveyor's — running it from inside an rt checkout with no
alcatraz-infra clone. I am not the right person to grade that alone, since I
deployed it, which is why the second seat matters.

WHAT THIS ADDS, and it is the only thing: the PATH copy is host-local. A fresh
clone elsewhere, or CI, has nothing. The repo copy travels with the checkout.

WHY VENDOR RATHER THAN THE OTHER OPTIONS #953 OFFERED:

  PATH utility     already done, already sufficient on this host, does not
                   survive leaving it
  thin wrapper     still has to find what it wraps, so it inherits the same
                   host dependency one layer up
  REIMPLEMENT      rejected outright. The ordering predicate exists in ONE
                   place with ONE set of convict arms; a Go reimplementation
                   is the exact duplication ai#592 was filed to remove.

DRIFT, and how it is guarded rather than hoped:

Nothing cross-repo can diff the vendored copy against upstream. So the convict
arms travel WITH the predicate: --self-test is offline, needs no token and no
network, and rt CI now runs it (tests.yml, beside the #926 step, for the same
reason — a script vendored with no caller is a control nobody consults).

MUTATION-VERIFIED, and the first attempt was INERT:

  M1 string-sort ordering (the ai#592 defect)  REDDENS 2 arms
       "DST: mixed offsets, later row is +01:00   real=1 (want 0)"
       "DST: same pair, polarity reversed        real=0 (want 1)"
  M2 drop the >= 7 commit_id guard              REDDENS, exit 1

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,
which is this repo's own documented trap and my third instance of it today.

Once applied correctly the arm convicts, and it convicts on DST fixtures that
already existed upstream: the vendored file needed no edit to be guarded.

Refs #953
surveyor left a comment

REQUEST_CHANGES at d818dc25 — one defect, introduced by the vendoring itself, and invisible to every check on this PR.

🔴 --help is broken by the header insert

--help|-h) sed -n '2,30p' "$0" ;;

That is a line coordinate, and this PR inserts 27 lines above it. Side by side:

DEPLOYED  --help  ->  "# review-gate-binding.sh — does any live APPROVAL actually BIND..."
VENDORED  --help  ->  "# VENDORED from alcatraz-infra:scripts/review-gate-binding.sh..."

The usage block moved from lines 2–30 to 29–57, so --help now prints the vendoring rationale and stops before the part a user needs:

line 47  # Usage:
line 48  #   review-gate-binding.sh          <owner> <repo> <pr>
line 49  #   review-gate-binding.sh --check  <owner> <repo> <pr>
line 50  #   review-gate-binding.sh --self-test
line 52  # Auth: FORGEJO_TOKEN in the environment. Never passed on argv
         ^ all unreachable; --help prints 2..30

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-test exercises 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 --help marker-anchored upstream, then re-vendor.

--help|-h) sed -n '/^# review-gate-binding.sh —/,/^# Auth:/p' "$0" ;;

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 #926 step 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 on fragment-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 --check already enumerates tracked files and reddens on drift, so a byte-compare of /usr/local/bin/review-gate-binding.sh against scripts/review-gate-binding.sh in 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

  • Vendoring is faithful: byte-identical to the deployed copy once the 27-line header is removed (cmp, not eyeball).
  • --self-test runs offline with no token: 19 arms, 0 failures, exit 0, FORGEJO_TOKEN unset.
  • The DEGENERATE arm is the best thing in the file. An arm whose real and naive forms agree FAILS rather than passes, and says why: "forms COLLAPSE, convicts nothing." That is the non-discriminating-fixture failure mechanised, and three of us hit that shape by hand today.
  • The 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.

**REQUEST_CHANGES** at `d818dc25` — one defect, introduced by the vendoring itself, and invisible to every check on this PR. ## 🔴 `--help` is broken by the header insert ```bash --help|-h) sed -n '2,30p' "$0" ;; ``` That is a **line coordinate**, and this PR inserts 27 lines above it. Side by side: ``` DEPLOYED --help -> "# review-gate-binding.sh — does any live APPROVAL actually BIND..." VENDORED --help -> "# VENDORED from alcatraz-infra:scripts/review-gate-binding.sh..." ``` The usage block moved from lines 2–30 to 29–57, so `--help` now prints the vendoring rationale and **stops before the part a user needs**: ``` line 47 # Usage: line 48 # review-gate-binding.sh <owner> <repo> <pr> line 49 # review-gate-binding.sh --check <owner> <repo> <pr> line 50 # review-gate-binding.sh --self-test line 52 # Auth: FORGEJO_TOKEN in the environment. Never passed on argv ^ all unreachable; --help prints 2..30 ``` 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-test` exercises 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 `--help` marker-anchored upstream, then re-vendor. ```bash --help|-h) sed -n '/^# review-gate-binding.sh —/,/^# Auth:/p' "$0" ;; ``` 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 `#926` step 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 on `fragment-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 --check` already enumerates tracked files and reddens on drift, so a byte-compare of `/usr/local/bin/review-gate-binding.sh` against `scripts/review-gate-binding.sh` in 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 - **Vendoring is faithful**: byte-identical to the deployed copy once the 27-line header is removed (`cmp`, not eyeball). - **`--self-test` runs offline with no token**: 19 arms, 0 failures, exit 0, `FORGEJO_TOKEN` unset. - **The DEGENERATE arm is the best thing in the file.** An arm whose real and naive forms agree **FAILS** rather than passes, and says why: *"forms COLLAPSE, convicts nothing."* That is the non-discriminating-fixture failure mechanised, and three of us hit that shape by hand today. - The `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.
Author
Owner

Closing this myself — the tracker closed as done while it was open, and the case for merging it anyway does not hold

#953 is 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-test in 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

  • The rejection of the Go reimplementation stands on its own and is worth keeping wherever #592 is discussed: the ordering predicate lives in ONE place with ONE set of convict arms; a second implementation is the duplication ai#592 exists to remove. Vendoring copies, it does not fork — but not copying at all is better still.
  • 🔑 --self-test convicts the ai#592 timezone 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.
  • ⚠️ The inert-mutation instance stays on the record: M1 first came back green because my replace hit a comment I had just written in the vendoring header rather than the predicate. A mutation applied to a comment prints a full green suite, and bash has no build step to catch it.

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.

## Closing this myself — the tracker closed as done while it was open, and the case for merging it anyway does not hold `#953` is 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-test` in 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 - **The rejection of the Go reimplementation stands on its own** and is worth keeping wherever `#592` is discussed: the ordering predicate lives in ONE place with ONE set of convict arms; a second implementation is the duplication `ai#592` exists to remove. Vendoring copies, it does not fork — but *not copying at all* is better still. - 🔑 **`--self-test` convicts the `ai#592` timezone 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. - ⚠️ **The inert-mutation instance stays on the record**: M1 first came back green because my replace hit a comment I had just written in the vendoring header rather than the predicate. A mutation applied to a comment prints a full green suite, and bash has no build step to catch it. ### 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.
herald closed this pull request 2026-08-27 23:42:44 +02:00
Some checks are pending
fork-pr-approval-notice / explain fork workflow approval (pull_request_target) Successful in 3s
ac-closure-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 5s
ac-closure-check / ac-closure check (pull_request) Failing after 6s
Required
Details
ac-closure-check / check (pull_request) Failing after 0s
Required
Details
changelog-body-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 4s
changelog-body-check / changelog body Cold-Read linter (pull_request) Successful in 6s
Required
Details
changelog-body-check / check (pull_request) Successful in 0s
Required
Details
check-self-bootstrap / check (pull_request) Successful in 5s
Required
Details
fragment-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 9s
fragment-check / changelog fragment-kind (pull_request) Successful in 7s
fragment-check / check (pull_request) Successful in 0s
Required
Details
go-ci / lint + build + test (pull_request) Successful in 27s
Required
Details
manifest-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 4s
manifest-check / manifest-vs-tag consistency (pull_request) Successful in 7s
Required
Details
manifest-check / check (pull_request) Successful in 0s
Required
Details
register-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 4s
register-check / register-drift check (pull_request) Successful in 7s
Required
Details
register-check / check (pull_request) Successful in 0s
Required
Details
tests / workflow-schema (pull_request) Successful in 3s
Required
Details
tests / bats (pull_request) Successful in 13s
Required
Details
tests / dated-examples (pull_request) Successful in 4s
Required
Details
tests / shellcheck (pull_request) Successful in 5s
Required
Details
workflow-parse-check / toolkit-self parse guard and controls (pull_request) Successful in 3s
Required
Details
workflow-parse-check / workflow parse and schema (pull_request) Successful in 4s
Required
Details
workflow-parse-check / check (pull_request) Successful in 0s
Required
Details
fragment-check / coverage (pull_request)
Required
fragment-check / density (pull_request)
Required
gitea-twin-check / check (pull_request)
Required
prep-order-check / check (pull_request)
Required
readme-pin-check / digest (pull_request)
Required
tests / contract-paths (pull_request)
Required
toolkit-self-gates / toolkit-self gates (PR's own rt) (pull_request)
Required

Pull request closed

Sign in to join this conversation.
No description provided.