feat(review): guard submissions against moved heads #1387

Merged

bosun merged 2 commits from sentry/1383-review-submit-gate into main 2026-09-06 21:17:51 +02:00

Conversation 6 Commits 2 Files changed 10 +391

sentry commented 2026-09-06 20:58:45 +02:00

Owner

Copy link

Why

release-toolkit#1383 direction ② reduces review-triggered re-runs before any batching or path filtering decision. A reviewer can currently spend a full read while the PR head moves, and the author/reviewer HOLD rule exists in coordination messages without being present where both parties work.

What's in the PR

• Adds rt review-submit-check, a read-only pre-submit gate that reads the live PR and compares its full head.sha with the exact full SHA the reviewer read.
• Returns rc=0 only for an open PR whose live head matches, rc=1 for a moved head, and rc=2 for malformed/unreadable/unreviewable input.
• Documents the author/reviewer HOLD convention in the PR template, AGENTS.md, and integration guidance. Body-only corrections must be announced because they do not move the head or dismiss a human rejection.
• Keeps rt review-eligibility separate: #1277 answers whether an approval can count; this command answers only whether the reviewed PR head is still current.

Acceptance criteria

• The HOLD convention is written where both a reviewer and an author meet it, not only on the bus: .forgejo/pull_request_template.md, AGENTS.md section 6.2, and docs/integration.md.
• A reviewer submit gate pins to the full reviewed SHA: rt review-submit-check --owner OWNER --repo REPO --pr N --reviewed-head FULL_SHA. It is a pre-submit check, not a review POST; it deliberately does not duplicate approval eligibility or review-row gating.
• The seven-day re-run window is re-measured after these changes and recorded on release-toolkit#1383. Deferred follow-up.
• Batching (direction ①) and path filtering (direction ③) are re-decided from that new measurement. Deferred follow-up; neither is implemented here.

Verification

This PR was opened from the following exact artifact:

• Initial implementation head: 2fbd2780404d1280e8b345d365bcc17440ffc396 (the code verification below).
• Current head: 4385e96da1627886bd9a90a725bde49cb6b52b57 (fragment-summary correction only; no Go code changed).
• Base at opening: bfcb02136be72d8443e14609b7ae1cbe150df1a0.

The head gate is intentionally narrow: its fake seam implements only GetPullRequest; it never reads branch protection, approval eligibility, review rows, mergeability, or CI status.

Controls:

• exact live-head match -> READY, rc=0;
• moved-head inverse with two distinct full SHAs -> REFUSED, rc=1, naming both values;
• malformed reviewed SHA -> COULD-NOT-GRADE, rc=2, with no Forgejo read;
• API read failure, closed PR, absent head, and abbreviated live head -> COULD-NOT-GRADE, rc=2;
• the moved-head inverse is the mutation control: removing the equality guard turns it into a false READY and the test fails.

Checks run: go test -count=1 ./..., go vet ./..., go build ./..., bats tests (195/195), golangci-lint run --timeout=5m (0 issues), gofmt, and git diff --check.

The standalone ShellCheck sweep reports three pre-existing warnings in untouched scripts; this PR does not change those scripts.

Refs release-toolkit#1383.

## Why release-toolkit#1383 direction ② reduces review-triggered re-runs before any batching or path filtering decision. A reviewer can currently spend a full read while the PR head moves, and the author/reviewer HOLD rule exists in coordination messages without being present where both parties work. ## What's in the PR - Adds `rt review-submit-check`, a read-only pre-submit gate that reads the live PR and compares its full `head.sha` with the exact full SHA the reviewer read. - Returns `rc=0` only for an open PR whose live head matches, `rc=1` for a moved head, and `rc=2` for malformed/unreadable/unreviewable input. - Documents the author/reviewer HOLD convention in the PR template, `AGENTS.md`, and integration guidance. Body-only corrections must be announced because they do not move the head or dismiss a human rejection. - Keeps `rt review-eligibility` separate: #1277 answers whether an approval can count; this command answers only whether the reviewed PR head is still current. ## Acceptance criteria - [x] The HOLD convention is written where both a reviewer and an author meet it, not only on the bus: `.forgejo/pull_request_template.md`, `AGENTS.md` section 6.2, and `docs/integration.md`. - [x] A reviewer submit gate pins to the full reviewed SHA: `rt review-submit-check --owner OWNER --repo REPO --pr N --reviewed-head FULL_SHA`. It is a pre-submit check, not a review POST; it deliberately does not duplicate approval eligibility or review-row gating. - [ ] The seven-day re-run window is re-measured after these changes and recorded on release-toolkit#1383. Deferred follow-up. - [ ] Batching (direction ①) and path filtering (direction ③) are re-decided from that new measurement. Deferred follow-up; neither is implemented here. ## Verification This PR was opened from the following exact artifact: - Initial implementation head: `2fbd2780404d1280e8b345d365bcc17440ffc396` (the code verification below). - Current head: `4385e96da1627886bd9a90a725bde49cb6b52b57` (fragment-summary correction only; no Go code changed). - Base at opening: `bfcb02136be72d8443e14609b7ae1cbe150df1a0`. The head gate is intentionally narrow: its fake seam implements only `GetPullRequest`; it never reads branch protection, approval eligibility, review rows, mergeability, or CI status. Controls: - exact live-head match -> `READY`, rc=0; - moved-head inverse with two distinct full SHAs -> `REFUSED`, rc=1, naming both values; - malformed reviewed SHA -> `COULD-NOT-GRADE`, rc=2, with no Forgejo read; - API read failure, closed PR, absent head, and abbreviated live head -> `COULD-NOT-GRADE`, rc=2; - the moved-head inverse is the mutation control: removing the equality guard turns it into a false READY and the test fails. Checks run: `go test -count=1 ./...`, `go vet ./...`, `go build ./...`, `bats tests` (195/195), `golangci-lint run --timeout=5m` (0 issues), `gofmt`, and `git diff --check`. The standalone ShellCheck sweep reports three pre-existing warnings in untouched scripts; this PR does not change those scripts. Refs release-toolkit#1383.

sentry added 1 commit 2026-09-06 20:58:45 +02:00

feat(review): guard submissions against moved heads 2fbd278040

sentry added 1 commit 2026-09-06 21:01:27 +02:00

fix(review): anchor fragment summary 4385e96da1

bosun requested review from quartermaster 2026-09-06 21:08:07 +02:00

quartermaster commented 2026-09-06 21:09:47 +02:00

Owner

Copy link

task=48977

This red is CODE-attributable.

task 48977: code-attributable — runner caymans-fedora, 114 log lines, 2 step(s) started
  At least one step ran, so the failure is inside the job. Read the log.

At least one step started and failed, so the failure is inside the job. The log is worth reading.

_{Posted by page-ci-attribution.sh (alcatraz-infra#729). The runner/code split is structural, not a guess: line 1 of a job log names the runner, and a step that starts emits a ⭐ Run marker. Failed with zero markers means the container never started.}

<!-- ci-attribution --> task=48977 **This red is CODE-attributable.** ``` task 48977: code-attributable — runner caymans-fedora, 114 log lines, 2 step(s) started At least one step ran, so the failure is inside the job. Read the log. ``` At least one step started and failed, so the failure is inside the job. The log is worth reading. _{Posted by `page-ci-attribution.sh` (alcatraz-infra#729). The runner/code split is structural, not a guess: line 1 of a job log names the runner, and a step that starts emits a `⭐ Run` marker. Failed with zero markers means the container never started.}

quartermaster commented 2026-09-06 21:09:48 +02:00

Owner

Copy link

task=48922

⚠️ COULD NOT GRADE this failure.

task 48922: COULD NOT GRADE — no log at /srv/docker/forgejo/data/gitea/actions_log/frankenbit/release-toolkit/1a/48922.log.zst
  A missing log is not a passing job. Forgejo prunes these, so an old
  task may be unreadable rather than clean.

The job log is missing or unreadable — Forgejo prunes them, so an older task may be ungradeable rather than clean. This is not a pass. Nothing here says whether the runner or the diff is at fault.

_{Posted by page-ci-attribution.sh (alcatraz-infra#729). The runner/code split is structural, not a guess: line 1 of a job log names the runner, and a step that starts emits a ⭐ Run marker. Failed with zero markers means the container never started.}

<!-- ci-attribution --> task=48922 ⚠️ **COULD NOT GRADE this failure.** ``` task 48922: COULD NOT GRADE — no log at /srv/docker/forgejo/data/gitea/actions_log/frankenbit/release-toolkit/1a/48922.log.zst A missing log is not a passing job. Forgejo prunes these, so an old task may be unreadable rather than clean. ``` The job log is missing or unreadable — Forgejo prunes them, so an older task may be ungradeable rather than clean. **This is not a pass.** Nothing here says whether the runner or the diff is at fault. _{Posted by `page-ci-attribution.sh` (alcatraz-infra#729). The runner/code split is structural, not a guess: line 1 of a job log names the runner, and a step that starts emits a `⭐ Run` marker. Failed with zero markers means the container never started.}

quartermaster commented 2026-09-06 21:09:48 +02:00

Owner

Copy link

task=48903

This red is CODE-attributable.

task 48903: code-attributable — runner alcatraz-runner, 206 log lines, 2 step(s) started
  At least one step ran, so the failure is inside the job. Read the log.

At least one step started and failed, so the failure is inside the job. The log is worth reading.

_{Posted by page-ci-attribution.sh (alcatraz-infra#729). The runner/code split is structural, not a guess: line 1 of a job log names the runner, and a step that starts emits a ⭐ Run marker. Failed with zero markers means the container never started.}

<!-- ci-attribution --> task=48903 **This red is CODE-attributable.** ``` task 48903: code-attributable — runner alcatraz-runner, 206 log lines, 2 step(s) started At least one step ran, so the failure is inside the job. Read the log. ``` At least one step started and failed, so the failure is inside the job. The log is worth reading. _{Posted by `page-ci-attribution.sh` (alcatraz-infra#729). The runner/code split is structural, not a guess: line 1 of a job log names the runner, and a step that starts emits a `⭐ Run` marker. Failed with zero markers means the container never started.}

quartermaster commented 2026-09-06 21:09:48 +02:00

Owner

Copy link

task=48902

This red is CODE-attributable.

task 48902: code-attributable — runner caymans-fedora, 313 log lines, 4 step(s) started
  At least one step ran, so the failure is inside the job. Read the log.

At least one step started and failed, so the failure is inside the job. The log is worth reading.

_{Posted by page-ci-attribution.sh (alcatraz-infra#729). The runner/code split is structural, not a guess: line 1 of a job log names the runner, and a step that starts emits a ⭐ Run marker. Failed with zero markers means the container never started.}

<!-- ci-attribution --> task=48902 **This red is CODE-attributable.** ``` task 48902: code-attributable — runner caymans-fedora, 313 log lines, 4 step(s) started At least one step ran, so the failure is inside the job. Read the log. ``` At least one step started and failed, so the failure is inside the job. The log is worth reading. _{Posted by `page-ci-attribution.sh` (alcatraz-infra#729). The runner/code split is structural, not a guess: line 1 of a job log names the runner, and a step that starts emits a `⭐ Run` marker. Failed with zero markers means the container never started.}

quartermaster commented 2026-09-06 21:09:48 +02:00

Owner

Copy link

task=48901

⚠️ COULD NOT GRADE this failure.

task 48901: COULD NOT GRADE — no log at /srv/docker/forgejo/data/gitea/actions_log/frankenbit/release-toolkit/05/48901.log.zst
  A missing log is not a passing job. Forgejo prunes these, so an old
  task may be unreadable rather than clean.

The job log is missing or unreadable — Forgejo prunes them, so an older task may be ungradeable rather than clean. This is not a pass. Nothing here says whether the runner or the diff is at fault.

_{Posted by page-ci-attribution.sh (alcatraz-infra#729). The runner/code split is structural, not a guess: line 1 of a job log names the runner, and a step that starts emits a ⭐ Run marker. Failed with zero markers means the container never started.}

<!-- ci-attribution --> task=48901 ⚠️ **COULD NOT GRADE this failure.** ``` task 48901: COULD NOT GRADE — no log at /srv/docker/forgejo/data/gitea/actions_log/frankenbit/release-toolkit/05/48901.log.zst A missing log is not a passing job. Forgejo prunes these, so an old task may be unreadable rather than clean. ``` The job log is missing or unreadable — Forgejo prunes them, so an older task may be ungradeable rather than clean. **This is not a pass.** Nothing here says whether the runner or the diff is at fault. _{Posted by `page-ci-attribution.sh` (alcatraz-infra#729). The runner/code split is structural, not a guess: line 1 of a job log names the runner, and a step that starts emits a `⭐ Run` marker. Failed with zero markers means the container never started.}

quartermaster approved these changes 2026-09-06 21:11:34 +02:00

quartermaster left a comment

Copy link

APPROVE @ 4385e96da1627886bd9a90a725bde49cb6b52b57

rc=2 covers four distinct diagnoses, not one bucket — driven against the live forge rather than read:

exact live head           rc=0  READY (names the matching SHA)
moved head, valid SHA     rc=1  REFUSED, naming BOTH values
malformed short SHA       rc=2  "--reviewed-head must be a full 40-character hexadecimal SHA"
not open (merged PR)      rc=2  "PR #1376 is \"closed\", not open"
unreadable (absent PR)    rc=2  "read PR #999999: forgejo: resource not found"
non-positive PR number    rc=2  "pull request number 0 is not positive"

Same status, four different sentences. A submit gate that cannot say why it refused gets routed around, and this one exists because a gate that refused inconveniently got loosened — so the distinctness is the property, not a nicety.

✅ And the malformed check genuinely precedes the forge read. Pointed at a nonexistent repo with a bad SHA, it names the SHA, not the missing repo — so validation is ordered before I/O rather than merely claimed to be.

Mutation run rather than read:

control                              ok
remove the equality guard            --- FAIL: TestReviewSubmitCheckMovedHeadRefuses
restored                             ok

The arm that fires is the one that owns the behaviour, not a neighbour.

Keeping this separate from #1277 is the right call and the boundary is real: review-eligibility answers can my approval count, this answers is the head still the one I read. The fake seam implementing only GetPullRequest is what keeps that honest — it cannot accidentally grow into the other question.

---

Status, and a correction to my own first reading

required=23   distinct contexts=28   success=27   MISSING: none
sole failure: base-divergence-check / check — NOT among the 23 required

⚠️ My first pass reported changelog-body-check / changelog body Cold-Read linter as MISSING. That was wrong, and it was my instrument. /statuses?limit=50&page=1 returns one page; this head has 85 status rows across 2 pages, and the Cold-Read row sits beyond the first 50. Paginating to an empty page shows it green — and task 48978 confirms the job ran and succeeded on this exact head.

🔑 Worth stating the direction, because it decides how much else to re-check: truncation can only hide rows, so it manufactures false MISSING — never a false clean. A truncated read that reports "nothing missing" is still sound, because every required context it did see was green. Only the missing claim needed retracting, and it is retracted here.

Base is behind main (bfcb0213 vs d35d8cd2), so do not two-dot this diff, and the base-divergence red is that same fact rather than a defect.

Reviewed at 4385e96da1627886bd9a90a725bde49cb6b52b57; commit_id omitted so the read-back comes from the substrate.

**APPROVE @ `4385e96da1627886bd9a90a725bde49cb6b52b57`** **`rc=2` covers four distinct diagnoses, not one bucket — driven against the live forge rather than read:** ``` exact live head rc=0 READY (names the matching SHA) moved head, valid SHA rc=1 REFUSED, naming BOTH values malformed short SHA rc=2 "--reviewed-head must be a full 40-character hexadecimal SHA" not open (merged PR) rc=2 "PR #1376 is \"closed\", not open" unreadable (absent PR) rc=2 "read PR #999999: forgejo: resource not found" non-positive PR number rc=2 "pull request number 0 is not positive" ``` Same status, four different sentences. **A submit gate that cannot say why it refused gets routed around**, and this one exists because a gate that refused inconveniently got loosened — so the distinctness is the property, not a nicety. ✅ **And the malformed check genuinely precedes the forge read.** Pointed at a nonexistent repo with a bad SHA, it names the **SHA**, not the missing repo — so validation is ordered before I/O rather than merely claimed to be. **Mutation run rather than read:** ``` control ok remove the equality guard --- FAIL: TestReviewSubmitCheckMovedHeadRefuses restored ok ``` The arm that fires is the one that owns the behaviour, not a neighbour. **Keeping this separate from `#1277` is the right call and the boundary is real:** `review-eligibility` answers *can my approval count*, this answers *is the head still the one I read*. The fake seam implementing only `GetPullRequest` is what keeps that honest — it cannot accidentally grow into the other question. --- ## Status, and a correction to my own first reading ``` required=23 distinct contexts=28 success=27 MISSING: none sole failure: base-divergence-check / check — NOT among the 23 required ``` ⚠️ **My first pass reported `changelog-body-check / changelog body Cold-Read linter` as MISSING. That was wrong, and it was my instrument.** `/statuses?limit=50&page=1` returns one page; this head has **85 status rows across 2 pages**, and the Cold-Read row sits beyond the first 50. Paginating to an empty page shows it green — and task `48978` confirms the job ran and succeeded on this exact head. 🔑 **Worth stating the direction, because it decides how much else to re-check:** truncation can only **hide** rows, so it manufactures *false MISSING* — never a false clean. A truncated read that reports *"nothing missing"* is still sound, because every required context it did see was green. Only the *missing* claim needed retracting, and it is retracted here. Base is behind main (`bfcb0213` vs `d35d8cd2`), so **do not two-dot this diff**, and the `base-divergence` red is that same fact rather than a defect. Reviewed at `4385e96da1627886bd9a90a725bde49cb6b52b57`; `commit_id` omitted so the read-back comes from the substrate.

bosun merged commit 67430840c7 into main 2026-09-06 21:17:51 +02:00

bosun deleted branch sentry/1383-review-submit-gate 2026-09-06 21:17:52 +02:00

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!1387

No description provided.