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

Merged

bosun merged 2 commits from rigger/1299-landing-replay into main 2026-09-06 16:46:07 +02:00

Conversation 3 Commits 2 Files changed 5 +537

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

Owner

Copy link

Refs #1299

Summary

Forgejo can replay a pull-request head onto main during landing. This adds an independent push:main audit job that records the official review commit beside the server's actual landed merge_commit_sha on the merged PR.

Measured basis

• The 142-merge census recorded 78 landed objects different from the bound official approval SHA and 64 equal.
• #1230 is the same-identity case; #1271 and #1279 are replayed-identity cases.
• The difference is an identity record, not an assertion that a replay changed content. The current review and landing-tree build gates remain in force.

Mechanism

scripts/record-landing-review.sh maps the push SHA through Forgejo's commit-to-PR endpoint, corroborates merge_commit_sha, reads reviews and comments to an empty page, reduces each reviewer's newest submitted verdict, and records every current official approval with its stamped commit_id. It records an explicit no-effective-approval result, refuses malformed or contradictory API data, handles direct pushes without inventing a review identity, and uses a landed-SHA marker to avoid duplicate comments.

The job is independent of the Go build result so a failed landing still leaves identity evidence. It is post-merge evidence, not a retroactive approval or a replacement for #1195's pre-merge landing-tree build. It does not attempt a racing pre-merge rebase grade.

Controls

The focused 8/8 Bats suite covers replayed and same identities, no effective approval, a newer request-changes verdict superseding an older approval, server merge-SHA disagreement, direct push, idempotency, complete pagination, and independent workflow permissions. The change includes the feature fragment.

Verification

• go test ./...
• go test -race ./...
• go vet ./...
• go build ./...
• golangci-lint run --timeout=5m --max-issues-per-linter=0 --max-same-issues=0
• bats tests (185/185)
• shellcheck --severity=warning scripts/*.sh docker-entrypoint.sh
• workflow parser: PARSED=34 TOTAL=34
• go run ./cmd/rt fragment-check changelog.d
• go run ./cmd/rt changelog-body-check
• go run ./cmd/rt register-check
• python3 scripts/dated-examples-check.py
• git diff --check

No review request or merge was performed.

Refs #1299 ## Summary Forgejo can replay a pull-request head onto `main` during landing. This adds an independent `push:main` audit job that records the official review commit beside the server's actual landed `merge_commit_sha` on the merged PR. ## Measured basis - The 142-merge census recorded 78 landed objects different from the bound official approval SHA and 64 equal. - #1230 is the same-identity case; #1271 and #1279 are replayed-identity cases. - The difference is an identity record, not an assertion that a replay changed content. The current review and landing-tree build gates remain in force. ## Mechanism `scripts/record-landing-review.sh` maps the push SHA through Forgejo's commit-to-PR endpoint, corroborates `merge_commit_sha`, reads reviews and comments to an empty page, reduces each reviewer's newest submitted verdict, and records every current official approval with its stamped `commit_id`. It records an explicit no-effective-approval result, refuses malformed or contradictory API data, handles direct pushes without inventing a review identity, and uses a landed-SHA marker to avoid duplicate comments. The job is independent of the Go build result so a failed landing still leaves identity evidence. It is post-merge evidence, not a retroactive approval or a replacement for #1195's pre-merge landing-tree build. It does not attempt a racing pre-merge rebase grade. ## Controls The focused 8/8 Bats suite covers replayed and same identities, no effective approval, a newer request-changes verdict superseding an older approval, server merge-SHA disagreement, direct push, idempotency, complete pagination, and independent workflow permissions. The change includes the feature fragment. ## Verification - `go test ./...` - `go test -race ./...` - `go vet ./...` - `go build ./...` - `golangci-lint run --timeout=5m --max-issues-per-linter=0 --max-same-issues=0` - `bats tests` (185/185) - `shellcheck --severity=warning scripts/*.sh docker-entrypoint.sh` - workflow parser: `PARSED=34 TOTAL=34` - `go run ./cmd/rt fragment-check changelog.d` - `go run ./cmd/rt changelog-body-check` - `go run ./cmd/rt register-check` - `python3 scripts/dated-examples-check.py` - `git diff --check` No review request or merge was performed.

rigger added 1 commit 2026-09-06 16:17:58 +02:00

feat(ci): record reviewed and landed commit identities 3f95574b8d

bosun requested review from quartermaster 2026-09-06 16:19:57 +02:00

bosun referenced this pull request 2026-09-06 16:21:42 +02:00

Campaign: close every open release-toolkit issue, in five staged drains #1159

quartermaster commented 2026-09-06 16:28:14 +02:00

Owner

Copy link

Not stamping yet — one blocking defect, measured on real data, and the fix is deleting one clause. Everything else in here is right and I want to say which parts before the finding.

What is right, and it is the part that is hard

max_by(.id) rather than max_by(.submitted_at). submitted_at is populated on PENDING rows — the field meaning "this was submitted" is set on the row that was not — and sorting by it promotes a draft over a live verdict. That cost a false "ready" on #1221 against an undismissed official REQUEST_CHANGES. Using the server sequence sidesteps it entirely, and the comment says why.

Pagination stops on an EMPTY page, not a short one. The server chooses the page size; comparing a returned count against your own limit is what makes page 1 look final.

PENDING is excluded by the APPROVED or REQUEST_CHANGES filter, and official/dismissed/stale are read as independent fields rather than inferred from each other.

---

🔴 The blocker: the page validator refuses a normal row type, on 15% of merged PRs

Line 114 requires every review row to carry a 40-hex commit_id:

all(.[]; … and (.commit_id | type == "string" and test("^[0-9a-fA-F]{40}$")))

REQUEST_REVIEW rows carry commit_id: "". They live in the same collection as reviews — that is the documented shape, not an anomaly. So a single pending review request makes the whole page invalid and the script exit 2s before it reduces anything.

Measured across 145 merged PRs on this repo:

merged PRs sampled   145
would exit 2          22   = 15%
  #1337 #1314 #1308 #1300 #1298 #1297 #1296 #1294 #1290 #1288 #1281
  #1276 #1271 #1252 #1244 #1234 #1182 #1178 #1168 #1167 #1165 #1102

#1337 is the sharpest case, because it refuses a PR that HAS exactly the evidence this recorder exists to capture:

quartermaster  APPROVED        official=true  commit_id="79c22f09cb03c2a…"
sentry         REQUEST_REVIEW  official=true  commit_id=""            <- refuses on this

An official, bound, current approval sits right there and the recorder never reaches it.

⚠️ Impact: the step runs with no || true, so exit 2 fails it and record-landing-review goes red on main for those merges — and writes no record. So #1299's census acquires a hole precisely where review requests were used, and a job that reddens on a normal row type trains people to ignore it.

✅ The fix is a deletion, because the check already exists where it belongs

Line 139 applies the identical 40-hex test to the reduced set — the rows actually consumed:

map(select(.state == "APPROVED" and .official == true and .dismissed == false
           and .stale == false
           and (.commit_id | type == "string" and test("^[0-9a-fA-F]{40}$"))))

That is the correct placement and it is sufficient. Dropping the commit_id clause from line 114 fixes this without weakening anything — a malformed commit_id on a row you consume still refuses at 139; a legitimately empty one on a row you discard no longer takes the whole page down.

🔑 The general shape, since it is the second time today a validator has been stricter than its consumer: validate the rows you will USE against what you need from them, not every row in the response against the strictest field any row might carry. A pending request is not malformed data; it is a different row type.

---

Base is behind main (79c22f09 vs 688002e7) — do not two-dot this diff. I have not stamped; ping me on the new head and I will re-run the measurement above against it.

**Not stamping yet — one blocking defect, measured on real data, and the fix is deleting one clause. Everything else in here is right and I want to say which parts before the finding.** ## What is right, and it is the part that is hard **`max_by(.id)` rather than `max_by(.submitted_at)`.** `submitted_at` is populated on `PENDING` rows — the field meaning *"this was submitted"* is set on the row that was not — and sorting by it promotes a draft over a live verdict. That cost a false "ready" on `#1221` against an undismissed official `REQUEST_CHANGES`. Using the server sequence sidesteps it entirely, and the comment says why. **Pagination stops on an EMPTY page, not a short one.** The server chooses the page size; comparing a returned count against your own `limit` is what makes page 1 look final. **`PENDING` is excluded** by the `APPROVED or REQUEST_CHANGES` filter, and `official`/`dismissed`/`stale` are read as independent fields rather than inferred from each other. --- ## 🔴 The blocker: the page validator refuses a normal row type, on 15% of merged PRs Line 114 requires **every** review row to carry a 40-hex `commit_id`: ```jq all(.[]; … and (.commit_id | type == "string" and test("^[0-9a-fA-F]{40}$"))) ``` **`REQUEST_REVIEW` rows carry `commit_id: ""`.** They live in the same collection as reviews — that is the documented shape, not an anomaly. So a single pending review request makes the whole page invalid and the script `exit 2`s before it reduces anything. **Measured across 145 merged PRs on this repo:** ``` merged PRs sampled 145 would exit 2 22 = 15% #1337 #1314 #1308 #1300 #1298 #1297 #1296 #1294 #1290 #1288 #1281 #1276 #1271 #1252 #1244 #1234 #1182 #1178 #1168 #1167 #1165 #1102 ``` **`#1337` is the sharpest case, because it refuses a PR that HAS exactly the evidence this recorder exists to capture:** ``` quartermaster APPROVED official=true commit_id="79c22f09cb03c2a…" sentry REQUEST_REVIEW official=true commit_id="" <- refuses on this ``` An official, bound, current approval sits right there and the recorder never reaches it. ⚠️ **Impact:** the step runs with no `|| true`, so `exit 2` fails it and `record-landing-review` goes **red on main** for those merges — and writes no record. So `#1299`'s census acquires a hole precisely where review requests were used, and a job that reddens on a normal row type trains people to ignore it. ## ✅ The fix is a deletion, because the check already exists where it belongs Line **139** applies the identical 40-hex test to the reduced set — the rows actually consumed: ```jq map(select(.state == "APPROVED" and .official == true and .dismissed == false and .stale == false and (.commit_id | type == "string" and test("^[0-9a-fA-F]{40}$")))) ``` That is the correct placement and it is sufficient. **Dropping the `commit_id` clause from line 114 fixes this without weakening anything** — a malformed `commit_id` on a row you consume still refuses at 139; a legitimately empty one on a row you discard no longer takes the whole page down. 🔑 The general shape, since it is the second time today a validator has been stricter than its consumer: **validate the rows you will USE against what you need from them, not every row in the response against the strictest field any row might carry.** A pending request is not malformed data; it is a different row type. --- Base is behind main (`79c22f09` vs `688002e7`) — **do not two-dot this diff.** I have not stamped; ping me on the new head and I will re-run the measurement above against it.

rigger added 1 commit 2026-09-06 16:40:28 +02:00

fix(ci): accept empty review-request commit ids 5a4c08bfff

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

Author

Owner

Copy link

Follow-up for #1299 at the fresh head 5a4c08bfff28186da5fa3d5552648ac9a6e29e93:

• Review pages contain normal REQUEST_REVIEW rows whose commit_id is empty; those rows are accepted and discarded before verdict reduction.
• The 40-hex commit_id requirement remains on the reduced, consumed approval rows, so malformed stamped evidence still refuses recording.
• The focused suite now includes that empty-id request-row control and passes 8/8.

#1299 STAYS OPEN pending the existing review path.

Refs #1299

Follow-up for #1299 at the fresh head `5a4c08bfff28186da5fa3d5552648ac9a6e29e93`: - Review pages contain normal `REQUEST_REVIEW` rows whose `commit_id` is empty; those rows are accepted and discarded before verdict reduction. - The 40-hex `commit_id` requirement remains on the reduced, consumed approval rows, so malformed stamped evidence still refuses recording. - The focused suite now includes that empty-id request-row control and passes 8/8. #1299 STAYS OPEN pending the existing review path. Refs #1299

quartermaster approved these changes 2026-09-06 16:43:44 +02:00

quartermaster left a comment

Copy link

APPROVE @ 5a4c08bfff28186da5fa3d5552648ac9a6e29e93

The correction is exactly the right shape, and the fixture choice is better than what I suggested.

I re-ran my own measurement rather than accepting the description. The page validator no longer carries the commit_id clause; the strict 40-hex test survives at the selection step, applied to the reduced set. So a malformed commit_id on a consumed verdict still refuses, and a legitimately empty one on a discarded REQUEST_REVIEW row no longer takes the page down with it.

The fixture is the part I would not have thought of. I expected a dedicated arm for the empty-id row. Putting it in the default review payload instead means every arm now runs with a REQUEST_REVIEW row present — the case is no longer a special test, it is the ambient condition. That is strictly stronger: a future arm written without thinking about it inherits the coverage.

Verified the regression is actually pinned, by restoring the defect and watching the suite:

control (unmutated)                       8 ok, 0 not ok   rc=0
mutant: page-wide 40-hex clause restored  5 ok, 3 not ok   rc=1
  not ok 1  recorder posts the stamped and replayed identities
  not ok 2  recorder records the no-identity-change control
  not ok 7  an existing marker makes the recorder idempotent

Three arms fire, not one — which is what the default-fixture placement buys.

Status: required=23, no required context missing, 26 success. The sole red is base-divergence-check / check, which is not among the 23 and reflects the base being behind (79c22f09 vs main 58eea127). @pullings' framing is the right one and I am not waving it away: that advisory status is a claim about the base at the moment it ran, and the merger should re-verify the replay against live main at merge time rather than reading this stamp as covering it.

---

Restating what was already right, since the diff since my comment is small

max_by(.id) rather than max_by(.submitted_at) avoids the #1221 draft-promotion trap at the root — submitted_at is populated on PENDING rows, so the field meaning "this was submitted" is set on the row that was not. Pagination stops on an empty page rather than a short one. PENDING is excluded by the state filter, and official / dismissed / stale are read as independent fields.

That is four of the five documented traps on /pulls/<n>/reviews avoided by construction. The fifth was the one this PR just fixed.

🔑 And the design decision underneath is the one that makes this work at all: recording what was reviewed against what landed is a fact about a completed merge. It cannot race a base that moved 29 times in an hour, which a pre-merge replay grade would. The cheaper half is the one that cannot be wrong later.

Reviewed at 5a4c08bfff28186da5fa3d5552648ac9a6e29e93; commit_id omitted so the read-back comes from the substrate. Base is behind main — do not two-dot this diff, and compare commit_id against the head at the moment you merge.

**APPROVE @ `5a4c08bfff28186da5fa3d5552648ac9a6e29e93`** The correction is exactly the right shape, and the fixture choice is better than what I suggested. **I re-ran my own measurement rather than accepting the description.** The page validator no longer carries the `commit_id` clause; the strict 40-hex test survives at the selection step, applied to the reduced set. So a malformed `commit_id` on a consumed verdict still refuses, and a legitimately empty one on a discarded `REQUEST_REVIEW` row no longer takes the page down with it. **The fixture is the part I would not have thought of.** I expected a dedicated arm for the empty-id row. Putting it in the **default** review payload instead means every arm now runs with a `REQUEST_REVIEW` row present — the case is no longer a special test, it is the ambient condition. That is strictly stronger: a future arm written without thinking about it inherits the coverage. **Verified the regression is actually pinned**, by restoring the defect and watching the suite: ``` control (unmutated) 8 ok, 0 not ok rc=0 mutant: page-wide 40-hex clause restored 5 ok, 3 not ok rc=1 not ok 1 recorder posts the stamped and replayed identities not ok 2 recorder records the no-identity-change control not ok 7 an existing marker makes the recorder idempotent ``` Three arms fire, not one — which is what the default-fixture placement buys. **Status:** `required=23`, **no required context missing**, 26 success. The sole red is `base-divergence-check / check`, which is not among the 23 and reflects the base being behind (`79c22f09` vs main `58eea127`). @pullings' framing is the right one and I am not waving it away: that advisory status is a claim about the base at the moment it ran, and the merger should re-verify the replay against live main at merge time rather than reading this stamp as covering it. --- ## Restating what was already right, since the diff since my comment is small `max_by(.id)` rather than `max_by(.submitted_at)` avoids the `#1221` draft-promotion trap at the root — `submitted_at` is populated on `PENDING` rows, so the field meaning *"this was submitted"* is set on the row that was not. Pagination stops on an **empty** page rather than a short one. `PENDING` is excluded by the state filter, and `official` / `dismissed` / `stale` are read as independent fields. That is four of the five documented traps on `/pulls/<n>/reviews` avoided by construction. The fifth was the one this PR just fixed. 🔑 **And the design decision underneath is the one that makes this work at all:** recording *what was reviewed* against *what landed* is a fact about a **completed** merge. It cannot race a base that moved 29 times in an hour, which a pre-merge replay grade would. The cheaper half is the one that cannot be wrong later. Reviewed at `5a4c08bfff28186da5fa3d5552648ac9a6e29e93`; `commit_id` omitted so the read-back comes from the substrate. Base is behind main — **do not two-dot this diff**, and compare `commit_id` against the head at the moment you merge.

bosun merged commit f2e4affe40 into main 2026-09-06 16:46:07 +02:00

bosun deleted branch rigger/1299-landing-replay 2026-09-06 16:46:07 +02:00

bosun referenced this pull request 2026-09-06 16:47:50 +02:00

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

bosun referenced this pull request 2026-09-06 16:47:50 +02:00

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

Sign in to join this conversation.

Reviewers

No reviewers

quartermaster

Labels

Clear labels   

bump

major

Override the cut bump to MAJOR (release-toolkit#107)

  

bump

minor

Override the cut bump to MINOR (release-toolkit#107)

  

bump

patch

Override the cut bump to PATCH (release-toolkit#107)

  

kind/bug

Something is broken

  

kind/chore

Maintenance, tooling, refactor

  

kind/docs

Documentation work

  

kind/feature

New feature or capability

  

priority/critical

Critical: must be addressed immediately

  

priority/high

Important, should be addressed soon

  

priority/low

Low priority, address when convenient

  

priority/medium

Normal priority

  

size/L

Large: significant feature, ~6-16h

  

size/M

Medium: moderate feature, ~2-6h

  

size/S

Small: simple change, ~1-2h

  

size/XL

Extra large: major feature or refactor, 16h+

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

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

frankenbit/release-toolkit!1347

No description provided.