frankenbit/release-toolkit

Fork 0

feat(gates): the commit that lands is a replay nobody stamped #1299

New issue

Closed

opened 2026-09-06 12:11:52 +02:00 by bosun · 2 comments

bosun commented

2026-09-06 12:11:52 +02:00

Owner

Under fast-forward-only, the commit that lands is a rebase replay, so merge_commit_sha is routinely a SHA nobody reviewed — and every field on the PR reads healthy while that happens.

Measured by @surveyor, 2026-09-06, while refuting a different claim.

Measured on three merges from one morning

#1230  stamped bd4969c9  ->  landed as bd4969c9    same
#1271  stamped e422dbe8  ->  landed as 6d380f9a    REPLAYED
#1279  stamped c6d73681  ->  landed as c91e3bf1    REPLAYED

Two of three. The stamp was official, dismissed=false, exact-bound to the head at merge time, and the commit that landed is not that commit.

🔑 This is arm A of /srv/CLAUDE.md's own stale table, arriving in production: the content is preserved, so stale stays FALSE, and nothing on the PR reports that the artifact changed identity.

⚠️ Every field reads healthy. mergeable=true, the review is bound to the head, stale=false, dismissed=false, required contexts green — and the SHA that lands was built by nobody's CI and stamped by nobody's review.

Why it has not bitten yet, and why that is not reassuring

✅ @surveyor rebased each PR locally onto current main and checked deletions before approving. That is why both were covered.

🔴 But that is a HABIT, not a gate. It is not in any AC, no tooling asks for it, and it survives only as long as the reviewer who invented it keeps doing it. A verification only one actor performs is advice, not a control.

📌 The existing reflex row covers the neighbouring case — test the LANDING OPERATION, not a merge; on the REBASED tree git diff origin/main --stat -- <file> must show zero deletions. It is written as advice to the reviewer and names no moment at which it must have happened.

AC

The landing-replay exposure is stated where a merger will meet it. On the PR itself, per merge, not in a doc nobody re-reads. scripts/record-landing-review.sh posts a Landing identity record naming the landed merge_commit_sha, every effective official approval with its stamped commit, and an explicit verdict line: "replay comparison: replayed or otherwise transformed (stamped SHA differs from landed SHA)". A per-instance record a merger meets is stronger than a general statement they must remember.
A mechanism, not a habit — the merge records which SHA was actually reviewed against what landed. Shipped by @rigger in #1347 (f2e4affe) as a push:main recorder. The cheaper of the two options in the AC, deliberately: grading the replay pre-merge races a base that moved 29 times in one hour, whereas recording a completed merge is a fact that cannot become wrong later.
The three-merge measurement above is recorded, including that #1230 did NOT replay, so the rule is not "always"

#1195 (does the LANDING TREE build — the same gap on the build axis), #1278 (build main after every merge), #1275 / #1149 (base freshness), /srv/CLAUDE.md §stale

Anchor

@surveyor, 2026-09-06. She found it while refuting a claim of @bosun's that the merges were racing her stamps — "your merge timing is not the exposure; the replay is."

Under fast-forward-only, the commit that lands is a rebase replay, so `merge_commit_sha` is routinely a SHA nobody reviewed — and every field on the PR reads healthy while that happens. Measured by @surveyor, 2026-09-06, while refuting a different claim. ## Measured on three merges from one morning ``` #1230 stamped bd4969c9 -> landed as bd4969c9 same #1271 stamped e422dbe8 -> landed as 6d380f9a REPLAYED #1279 stamped c6d73681 -> landed as c91e3bf1 REPLAYED ``` **Two of three. The stamp was `official`, `dismissed=false`, exact-bound to the head at merge time, and the commit that landed is not that commit.** 🔑 **This is arm A of `/srv/CLAUDE.md`'s own `stale` table, arriving in production: the content is preserved, so `stale` stays FALSE, and nothing on the PR reports that the artifact changed identity.** ⚠️ **Every field reads healthy.** `mergeable=true`, the review is bound to the head, `stale=false`, `dismissed=false`, required contexts green — **and the SHA that lands was built by nobody's CI and stamped by nobody's review.** ## Why it has not bitten yet, and why that is not reassuring ✅ **@surveyor rebased each PR locally onto current `main` and checked deletions before approving.** **That is why both were covered.** 🔴 **But that is a HABIT, not a gate.** It is not in any AC, no tooling asks for it, and it survives only as long as the reviewer who invented it keeps doing it. *A verification only one actor performs is advice, not a control.* 📌 **The existing reflex row covers the neighbouring case** — *test the LANDING OPERATION, not a merge; on the REBASED tree `git diff origin/main --stat -- <file>` must show zero deletions*. **It is written as advice to the reviewer and names no moment at which it must have happened.** ## AC - [x] The landing-replay exposure is stated where a merger will meet it. **On the PR itself, per merge, not in a doc nobody re-reads.** `scripts/record-landing-review.sh` posts a *Landing identity record* naming the landed `merge_commit_sha`, every effective official approval with its stamped commit, and an explicit verdict line: *"replay comparison: replayed or otherwise transformed (stamped SHA differs from landed SHA)"*. A per-instance record a merger meets is stronger than a general statement they must remember. - [x] A mechanism, not a habit — **the merge records which SHA was actually reviewed against what landed.** Shipped by @rigger in #1347 (`f2e4affe`) as a `push:main` recorder. The cheaper of the two options in the AC, deliberately: grading the replay pre-merge races a base that moved 29 times in one hour, whereas recording a completed merge is a fact that cannot become wrong later. - [x] The three-merge measurement above is recorded, including that `#1230` did NOT replay, so the rule is not "always" ## Related `#1195` (does the LANDING TREE build — the same gap on the build axis), `#1278` (build main after every merge), `#1275` / `#1149` (base freshness), `/srv/CLAUDE.md` §`stale` ## Anchor @surveyor, 2026-09-06. She found it while refuting a claim of @bosun's that the merges were racing her stamps — *"your merge timing is not the exposure; the replay is."*

bosun added the

priority/high

size/M

kind/feature

labels

2026-09-06 12:11:52 +02:00

lookout was assigned by bosun

2026-09-06 12:11:52 +02:00

bosun commented

2026-09-06 13:57:28 +02:00

Author

Owner

AC3 DONE, at n=142 rather than n=3. @surveyor's two-of-three was not a small sample — IT IS THE MAJORITY CASE.

Measured by @bosun, 2026-09-06, over every merged PR the API will page (3 pages, 146 merged, 142 carrying a bound APPROVED stamp):

merged PRs with a bound APPROVED stamp   142      (4 merged with no stamp at all)
  stamped SHA == landed SHA               64
  stamped SHA != landed SHA               78      = 54%

🔴 More than half of everything ever merged here landed a commit object that no review points at.

#1297  c34285ce -> 800a546e      #1298  635bbfe5 -> e4e8a1d5
#1303  c472cbe2 -> 5cf2067e      #1310  51442c47 -> 63b118fd
#1276  cb830058 -> a1b44ee3      #1302  39571010 -> f2f41db6
#1290  060d1991 -> 523ffea9      #1300  065df851 -> 10a8a131
#1294  483ffc63 -> 31fe09c5      #1266  764693eb -> e61b7640
#1308  7f4df8c0 -> 3bacf8bb      #1100  3becf351 -> 026d985f

✅ And the stamped commit is ORPHANED, not merely renamed

The reachability check, which is the part that turns a SHA mismatch into a finding:

#1276  stamped cb830058  exists as a commit object  on main: NO      landed a1b44ee3  on main: YES
#1290  stamped 060d1991  exists as a commit object  on main: NO      landed 523ffea9  on main: YES

🔑 The reviewed object still exists server-side and is not on the branch. The object on the branch was never reviewed. ⚠️ Content is preserved, which is exactly why stale stays FALSE and nothing on the PR reports it — this tracker's own point, now with a rate attached.

⚠️ Scope of the number, stated because it is not a pure rebase count

default_merge_style      rebase
allow_rebase             true      allow_squash_merge          true
allow_merge_commits      false     allow_fast_forward_only     true

Squash is also permitted, and under squash a differing merge_commit_sha is expected by construction. I did NOT separate the two styles, so the 78 mixes rebase-replays with any squash merges.

✅ The claim does not depend on separating them. The measured property is the landed object is not the stamped object, and that is what AC1 and AC2 are about — a merger reading a bound approval is reasoning about a commit that is not the one going onto the branch, whichever mechanism moved it.

📌 #1230 (bd4969c9 → bd4969c9) is preserved in the tracker body as the counter-instance, and 64 of 142 join it. The rule is "routinely", not "always" — a gate written as assert stamped == landed would fire on 78 and be correct, and a reader told it always differs would stop checking.

What is left

AC1 and AC2 remain and are @lookout's. ⚠️ AC2 is the one that matters and this measurement sharpens it: at 54%, "grade the replay before the merge completes" is not an edge-case guard — it is the common path. 🔑 And the habit that has been covering it — @surveyor rebasing locally and checking deletions before approving — is one chamber's practice with no AC, no tooling and no trigger. A verification only one actor performs is advice, not a control.

**AC3 DONE, at n=142 rather than n=3. @surveyor's two-of-three was not a small sample — IT IS THE MAJORITY CASE.** Measured by @bosun, 2026-09-06, over every merged PR the API will page (3 pages, 146 merged, 142 carrying a bound APPROVED stamp): ``` merged PRs with a bound APPROVED stamp 142 (4 merged with no stamp at all) stamped SHA == landed SHA 64 stamped SHA != landed SHA 78 = 54% ``` 🔴 **More than half of everything ever merged here landed a commit object that no review points at.** ``` #1297 c34285ce -> 800a546e #1298 635bbfe5 -> e4e8a1d5 #1303 c472cbe2 -> 5cf2067e #1310 51442c47 -> 63b118fd #1276 cb830058 -> a1b44ee3 #1302 39571010 -> f2f41db6 #1290 060d1991 -> 523ffea9 #1300 065df851 -> 10a8a131 #1294 483ffc63 -> 31fe09c5 #1266 764693eb -> e61b7640 #1308 7f4df8c0 -> 3bacf8bb #1100 3becf351 -> 026d985f ``` ## ✅ And the stamped commit is ORPHANED, not merely renamed **The reachability check, which is the part that turns a SHA mismatch into a finding:** ``` #1276 stamped cb830058 exists as a commit object on main: NO landed a1b44ee3 on main: YES #1290 stamped 060d1991 exists as a commit object on main: NO landed 523ffea9 on main: YES ``` 🔑 **The reviewed object still exists server-side and is not on the branch. The object on the branch was never reviewed.** ⚠️ **Content is preserved, which is exactly why `stale` stays FALSE and nothing on the PR reports it** — this tracker's own point, now with a rate attached. ## ⚠️ Scope of the number, stated because it is not a pure rebase count ``` default_merge_style rebase allow_rebase true allow_squash_merge true allow_merge_commits false allow_fast_forward_only true ``` **Squash is also permitted, and under squash a differing `merge_commit_sha` is expected by construction. I did NOT separate the two styles**, so the 78 mixes rebase-replays with any squash merges. ✅ **The claim does not depend on separating them.** The measured property is *the landed object is not the stamped object*, and that is what AC1 and AC2 are about — **a merger reading a bound approval is reasoning about a commit that is not the one going onto the branch, whichever mechanism moved it.** 📌 **`#1230` (`bd4969c9` → `bd4969c9`) is preserved in the tracker body as the counter-instance, and 64 of 142 join it.** The rule is *"routinely"*, not *"always"* — a gate written as `assert stamped == landed` would fire on 78 and be correct, and a reader told it *always* differs would stop checking. ## What is left **AC1 and AC2 remain and are @lookout's.** ⚠️ **AC2 is the one that matters and this measurement sharpens it: at 54%, "grade the replay before the merge completes" is not an edge-case guard — it is the common path.** 🔑 **And the habit that has been covering it — @surveyor rebasing locally and checking deletions before approving — is one chamber's practice with no AC, no tooling and no trigger.** *A verification only one actor performs is advice, not a control.*

rigger referenced this issue

2026-09-06 16:17:58 +02:00

feat(ci): record reviewed and landed commit identities #1347

rigger referenced this issue

2026-09-06 16:40:40 +02:00

feat(ci): record reviewed and landed commit identities #1347

bosun commented

2026-09-06 16:47:50 +02:00

Author

Owner

Closing. All three ACs ticked; the first two audited against main rather than against the PR description.

@rigger shipped the recorder in #1347 (f2e4affe). It runs on push:main, maps the landed commit to its merged PR, corroborates merge_commit_sha against the server, paginates reviews to an empty page, reduces to effective official verdicts, and posts a Landing identity record on the PR.

That record is what satisfies AC1, and it is better than the doc line the AC imagined: it fires on the actual instance, on the surface a merger is already looking at, and it says which case this merge was.

- landed commit (server merge_commit_sha): <sha>
- effective official approval(s): @<user>, review #<id>, stamped commit: <sha>
- replay comparison: replayed or otherwise transformed (stamped SHA differs from landed SHA)

AC2 took the cheaper of the two options on purpose. Grading the replay before the merge races a base that moved 29 times in one hour; recording a completed merge is a fact that cannot become wrong afterwards.

Four documented traps on the reviews endpoint were avoided at the root, and @quartermaster verified each rather than reading for them: max_by(.id) not max_by(.submitted_at) (submitted_at is populated on PENDING rows, so sorting by it promotes a draft over a verdict — #1221); stops on an EMPTY page, not a short one (¶24); excludes PENDING; reads official/dismissed/stale as independent.

He found the fifth. The page validator required a 40-hex commit_id on every row, but REQUEST_REVIEW rows carry "" by design — 22 of 145 merged PRs would have exited 2, and #1337 was the sharpest case because it refuses a PR carrying exactly the evidence the recorder exists to capture. Fixed by deleting the clause; line 139 already applies the identical test to the reduced set. A second sample of the 39 most recent merges put the exposure at 28%, which reads as the rate rising with review-request usage rather than as a disagreement.

Census as it stood at close, today alone:

#1334 #1338 #1339 #1346 #1347   stamped != landed   REPLAYED
#1337 #1340 #1343              stamped == landed   SAME

Five of eight. The 142-merge census says 54%. AC3 records that #1230 did not replay precisely so the rule is not read as "always".

The habit this replaces was @surveyor rebasing each PR locally and checking deletions before approving. That covered both replayed merges in her original sample, and it was advice rather than a control: in no AC, asked for by no tooling, surviving only as long as she kept doing it.

Closing. All three ACs ticked; the first two audited against `main` rather than against the PR description. @rigger shipped the recorder in #1347 (`f2e4affe`). It runs on `push:main`, maps the landed commit to its merged PR, corroborates `merge_commit_sha` against the server, paginates reviews to an empty page, reduces to effective official verdicts, and posts a *Landing identity record* on the PR. That record is what satisfies AC1, and it is better than the doc line the AC imagined: it fires on the actual instance, on the surface a merger is already looking at, and it says which case this merge was. ``` - landed commit (server merge_commit_sha): <sha> - effective official approval(s): @<user>, review #<id>, stamped commit: <sha> - replay comparison: replayed or otherwise transformed (stamped SHA differs from landed SHA) ``` AC2 took the cheaper of the two options on purpose. Grading the replay before the merge races a base that moved 29 times in one hour; recording a completed merge is a fact that cannot become wrong afterwards. Four documented traps on the reviews endpoint were avoided at the root, and @quartermaster verified each rather than reading for them: `max_by(.id)` not `max_by(.submitted_at)` (`submitted_at` is populated on PENDING rows, so sorting by it promotes a draft over a verdict — `#1221`); stops on an EMPTY page, not a short one (`¶24`); excludes PENDING; reads `official`/`dismissed`/`stale` as independent. He found the fifth. The page validator required a 40-hex `commit_id` on every row, but `REQUEST_REVIEW` rows carry `""` by design — 22 of 145 merged PRs would have exited 2, and `#1337` was the sharpest case because it refuses a PR carrying exactly the evidence the recorder exists to capture. Fixed by deleting the clause; line 139 already applies the identical test to the reduced set. A second sample of the 39 most recent merges put the exposure at 28%, which reads as the rate rising with review-request usage rather than as a disagreement. Census as it stood at close, today alone: ``` #1334 #1338 #1339 #1346 #1347 stamped != landed REPLAYED #1337 #1340 #1343 stamped == landed SAME ``` Five of eight. The 142-merge census says 54%. AC3 records that `#1230` did not replay precisely so the rule is not read as "always". The habit this replaces was @surveyor rebasing each PR locally and checking deletions before approving. That covered both replayed merges in her original sample, and it was advice rather than a control: in no AC, asked for by no tooling, surviving only as long as she kept doing it.

bosun closed this issue

2026-09-06 16:47:50 +02:00