From 00e623c066ff80a52c1a6df0bdacd07d97103bfc Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 23:13:50 +0200 Subject: [PATCH 1/4] fix(632): bind a review verdict to its BASE branch, not only to its head sha MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #622 made `review-verdict/h10` a per-sha required status, so a new commit cannot inherit an old verdict — the required context is simply absent on the new head. Retargeting a PR's base reaches the same end from the opposite direction: the head sha and the status both hold still while the merge-base, and therefore the effective diff the verdict was formed against, changes underneath them. #622's record claimed the invariant holds "by construction"; this was the documented exception, and an unrecorded exception is how a guarantee degrades into a habit. `post-review-verdict.sh` now records the base branch in the status description as a trailing `(base: )`, and refuses to write a status at all if the base moved between reading the PR and posting — the same TOCTOU window the head check already covers, which the head check cannot see because retargeting does not move the head. `pretooluse-merge-consent.sh` reads the field back and denies when it no longer matches the PR's live `base.ref`. Two choices are load-bearing, and each is pinned by a test rather than left to a comment: - The comparator is `base.ref`, NOT `base.sha`. `base.sha` tracks the base branch's tip, which moves whenever anything merges to `main` — comparing it would invalidate every open verdict on every unrelated merge, converting a rare-event guard into a permanent merge deadlock. A base that merely advances is out of scope by design: rebasing onto it moves the head sha, which the per-sha binding already covers. - The field goes in the status DESCRIPTION, not the verdict comment. The comment body is parsed by `scripts/check-review-verdict.sh`, whose grammar had three false-opens in its history (#629); nothing parses the description, so this adds a field without reopening that surface. Scope is stated honestly rather than overclaimed: this is DETECTION on the hook path only. A commit status carries no base of its own, so the server-side required check cannot see a retarget, and a merge driven through the Gitea UI or API is unaffected. That is the accepted exposure — base changes are rare, manual, and this is a two-account repo — but it now fails loud in the one place that evaluates consent, instead of living only in a doc. Verdicts posted before this change carry no `(base: …)` and get NO opinion rather than a deny; denying would block every in-flight PR the day it lands, and the window closes on its own since verdicts are per-head and short-lived. Verified by mutation, six mutants, each killed by its intended test: remove the hook's deny; compare base.sha instead of base.ref; drop graceful adoption; stop recording the base; drop the TOCTOU guard; accept a PR with no resolvable base. The positive controls matter more than usual here — the test PR is deliberately non-docs (a docs-only PR short-circuits the whole gate and would never reach the base check) and the rest of the gate is unstubbed, so "the hook denied" alone proves nothing. Refs #632 Decisions-Edit: yes --- .claude/hooks/pretooluse-merge-consent.sh | 28 ++++ docs/ci-cd.md | 8 + .../records/release/verdict-status-check.md | 23 ++- scripts/post-review-verdict.sh | 32 +++- .../tests/test_merge_consent_base_change.py | 150 ++++++++++++++++++ scripts/tests/test_post_review_verdict.py | 62 +++++++- 6 files changed, 297 insertions(+), 6 deletions(-) create mode 100644 scripts/tests/test_merge_consent_base_change.py diff --git a/.claude/hooks/pretooluse-merge-consent.sh b/.claude/hooks/pretooluse-merge-consent.sh index 2b41c37e0..b2851a664 100755 --- a/.claude/hooks/pretooluse-merge-consent.sh +++ b/.claude/hooks/pretooluse-merge-consent.sh @@ -120,6 +120,34 @@ if [ "$files_complete" = yes ] && [ -n "$files" ] && ! printf '%s\n' "$files" | decide allow "" # passthrough (exit 0 → normal prompt), NOT grant fi +# --- Base-change detection: a verdict is bound to a head AND to a base (ersatztv#632). --- +# `review-verdict/h10` is per-sha, which makes "the head moved under a fixed verdict" impossible by +# construction. Retargeting a PR's base is the mirror case and slips through: it changes neither the +# head sha nor the status, so a verdict formed while the PR targeted `main` still reads green after +# the PR is pointed at a branch with a very different merge-base. The diff moves while the verdict +# and the head both hold still. +# +# DETECTION, NOT PREVENTION, and only on this path. A commit status carries no base, so the +# server-side required check cannot see this; a merge driven through the Gitea UI or API is +# unaffected. That is the accepted exposure — base changes are rare, manual, and this is a +# two-account repo — but it is now recorded in a place that fails LOUD rather than only in a doc. +# +# GRACEFUL ADOPTION, mirroring (b) and (c): a description with no `(base: …)` field is a verdict +# posted before ersatztv#632 and gets NO opinion, rather than denying every in-flight PR the day +# this lands. The window closes on its own — verdicts are per-head and short-lived, so every verdict +# posted after this carries the field. +live_base=$(printf '%s' "$prjson" | jq -r '.base.ref // ""' 2>/dev/null || true) +if [ -n "$sha" ] && [ -n "$live_base" ]; then + vdesc=$(gq "repos/$owner/$repo/commits/$sha/status?limit=100" \ + | jq -r '[.statuses[]? | select(.context == "review-verdict/h10")] | first | .description // ""' \ + 2>/dev/null || true) + # The field is written by scripts/post-review-verdict.sh as a trailing `(base: )`. + recorded_base=$(printf '%s' "$vdesc" | sed -n 's/.*(base: \(.*\))$/\1/p') + if [ -n "$recorded_base" ] && [ "$recorded_base" != "$live_base" ]; then + decide deny "H10 merge gate: BLOCKED — the review verdict on head ${sha:0:7} was formed while PR #$pr targeted '$recorded_base', but it now targets '$live_base'. Retargeting a base does not move the head sha, so the per-sha verdict status still reads green even though the effective diff has changed (ersatztv#632). Re-review against the new base and run: scripts/post-review-verdict.sh $pr MERGEABLE" + fi +fi + # --- Linked issue: Gitea auto-close keywords in the PR body. --- issues=$(printf '%s' "$body" | grep -ioE '(close[sd]?|fix(e[sd])?|resolve[sd]?) +#[0-9]+' | grep -oE '[0-9]+' | sort -u || true) [ -n "$issues" ] || decide ask "H6 merge gate: PR #$pr has no linked issue (no 'fixes #N' / 'closes #N' in its body), so there is no Done-when checklist to derive consent from. Confirm the work is complete + reviewed, then approve." diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 2b27b03fb..33b8bd37c 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -785,6 +785,14 @@ hook's condition (c)) and the `review-verdict/h10` status on the same sha. `BLOC a commit landed mid-flight it writes **no** status and exits non-zero rather than retargeting your verdict at a commit you never read. +The status description also records the base branch — `Review-verdict: MERGEABLE @ abc1234 (base: +main)` — and the merge-consent hook denies when that no longer matches the PR's live `base.ref` +(ersatztv#632). Retargeting a PR changes the effective diff without moving the head sha, so the +per-sha binding alone cannot see it. This is **detection on the hook path only**: a commit status +carries no base of its own, so a merge driven through the Gitea UI or API is unaffected. The +comparator is the base *branch*, never its tip sha — a base that merely advances is ordinary churn, +and comparing tips would invalidate every open verdict on every unrelated merge to `main`. + **Exemptions** are handled by `review-verdict.yml` on every `pull_request` event, which posts the status as `success` for **Renovate-authored** PRs (it uses `platformAutomerge: true`, so a required verdict with no exemption would stall every dependency bump) and for **docs-only** PRs, and as diff --git a/docs/decisions/records/release/verdict-status-check.md b/docs/decisions/records/release/verdict-status-check.md index 614d6706b..83ed38a27 100644 --- a/docs/decisions/records/release/verdict-status-check.md +++ b/docs/decisions/records/release/verdict-status-check.md @@ -142,8 +142,27 @@ described as one: `GET /commits/{sha}/status`, which returns latest-per-context, and the workflow refuses to post anything at all when that read fails or is unparseable, rather than treating it as "no verdict yet". 3. **Changing a PR's base does not change its head sha**, so a verdict status keeps applying to a diff - that has materially changed. Not currently handled; low exposure here because base changes are rare - and manual. + that has materially changed. **Detected, not prevented** (#632): `post-review-verdict.sh` records + the base branch in the status description as a trailing `(base: )`, and the merge-consent hook + reads it back and denies when it no longer matches the PR's live `base.ref`. That covers the hook + path only — a commit status carries no base of its own, so the server-side required check cannot + see this, and a merge driven through the Gitea UI or API is unaffected. Accepted: base changes are + rare, manual, and this is a two-account repo. + + Two details are load-bearing and each was chosen against a plausible alternative: + + - **The comparator is `base.ref`, not `base.sha`.** `base.sha` tracks the base branch's *tip*, + which moves whenever anything merges to `main`; comparing it would invalidate every open verdict + on every unrelated merge — a rare-event guard turned into a permanent merge deadlock. A base + branch that merely *advances* is deliberately out of scope: rebasing onto it moves the head sha, + which the per-sha binding already covers. + - **The field goes in the status description, not the verdict comment.** The comment body is parsed + by `scripts/check-review-verdict.sh`, whose grammar had three false-opens in its history (#629); + nothing parses the description, so this adds a field without reopening that surface. + + Verdicts posted before #632 carry no `(base: …)` and get **no opinion** rather than a deny — the + alternative would block every in-flight PR the day it lands, and the window closes on its own since + verdicts are per-head and short-lived. 4. **A PR that edits `review-verdict.yml` is judged by its own edited copy.** Gitea runs `pull_request` workflows from the PR **head**, not the base — confirmed on the very PR that introduced this workflow (#630): `review-verdict.yml` does not exist on `main`, yet its job ran diff --git a/scripts/post-review-verdict.sh b/scripts/post-review-verdict.sh index cd0093dac..a7458f376 100755 --- a/scripts/post-review-verdict.sh +++ b/scripts/post-review-verdict.sh @@ -92,6 +92,22 @@ pr_url=$(printf '%s' "$prjson" | jq -r '.html_url // ""') [ "$pr_state" = "open" ] || die "PR #$pr is '$pr_state', not open — refusing to post a verdict" short=${sha:0:7} +# --- Record the BASE BRANCH the verdict was formed against (ersatztv#632). ---------------------- +# The sha binding closes "the head moved under a fixed verdict". It does not close the mirror case: +# RETARGETING a PR's base changes neither the head sha nor the status, yet changes the effective +# diff — so a verdict written while the PR targeted `main` still reads green after it is pointed at +# a branch with a very different merge-base. Consent outliving what it was granted for, reached from +# the other direction. +# +# The comparator is `base.ref` (the BRANCH NAME), deliberately NOT `base.sha`. `base.sha` tracks the +# base branch's tip, which moves every time anything merges to `main` — comparing it would invalidate +# every open verdict on every unrelated merge, i.e. a self-inflicted merge deadlock. `base.ref` +# changes exactly when someone retargets the PR, which is the event being guarded. A base branch that +# merely ADVANCES is out of scope by design: that is ordinary churn, and rebasing onto it changes the +# head sha, which the existing per-sha binding already catches. +base_ref=$(printf '%s' "$prjson" | jq -r '.base.ref // ""') +[ -n "$base_ref" ] || die "PR #$pr has no resolvable base branch (.base.ref) — refusing to post a verdict that cannot record what it was formed against" + # --- The comment (human-readable artifact + the hook's condition-(c) input). -------------------- # The verdict line MUST start the line: the hook anchors its parser to line-start precisely so a # comment that merely QUOTES the template mid-sentence cannot self-approve a merge. @@ -107,14 +123,26 @@ printf 'posted comment: Review-verdict: %s @ %s\n' "$verdict" "$short" # a verdict written for its parent — reintroducing ersatztv#622 at a smaller time scale. We do NOT # retry against the new head: the new commit is genuinely unreviewed, and silently re-targeting the # verdict at it is exactly the failure this script exists to prevent. -sha_now=$(api_get "repos/$owner/$repo/pulls/$pr" | jq -r '.head.sha // ""') +prjson_now=$(api_get "repos/$owner/$repo/pulls/$pr" || true) +sha_now=$(printf '%s' "$prjson_now" | jq -r '.head.sha // ""') if [ -n "$sha_now" ] && [ "$sha_now" != "$sha" ]; then die "head moved from $short to ${sha_now:0:7} while posting — that commit is UNREVIEWED, so no status was written. Re-review the new head and run this again." fi +# The same TOCTOU window applies to the base (ersatztv#632): a retarget between the read above and +# the status write below would bind the verdict to a base that is no longer the PR's, and the head +# sha check would not notice because retargeting does not move the head. +base_now=$(printf '%s' "$prjson_now" | jq -r '.base.ref // ""') +if [ -n "$base_now" ] && [ "$base_now" != "$base_ref" ]; then + die "base branch changed from '$base_ref' to '$base_now' while posting — the diff you reviewed is not the diff this PR now merges, so no status was written. Re-review against the new base and run this again." +fi +# The base branch goes in the status DESCRIPTION, not in the comment. The comment body is parsed by +# `scripts/check-review-verdict.sh`, whose grammar had three false-opens in its history; nothing +# parses the description today, so this adds a field without reopening that surface. The hook reads +# it back and compares (ersatztv#632). status_payload=$(jq -n \ --arg s "$state" --arg c "$STATUS_CONTEXT" --arg u "$pr_url" \ - --arg d "Review-verdict: $verdict @ $short" \ + --arg d "Review-verdict: $verdict @ $short (base: $base_ref)" \ '{state:$s, context:$c, description:$d, target_url:$u}') api_post "repos/$owner/$repo/statuses/$sha" "$status_payload" >/dev/null \ || die "failed to post the '$STATUS_CONTEXT' commit status on $short" diff --git a/scripts/tests/test_merge_consent_base_change.py b/scripts/tests/test_merge_consent_base_change.py new file mode 100644 index 000000000..8e4b294ce --- /dev/null +++ b/scripts/tests/test_merge_consent_base_change.py @@ -0,0 +1,150 @@ +"""Tests for the base-change detection in `.claude/hooks/pretooluse-merge-consent.sh` (#632). + +`review-verdict/h10` is a per-sha commit status, which makes "a new commit inherits an old verdict" +impossible by construction (#622). Retargeting a PR's base reaches the same end by the opposite +route: the head sha does not move, so the status stays green, while the merge-base — and therefore +the effective diff the verdict was formed against — changes underneath it. + +What is asserted here is DETECTION on the hook path only, and the tests are written to keep that +claim narrow: + + * a status carries no base field of its own, so the server-side required check cannot see this at + all; a merge driven through the Gitea UI or API is unaffected. No test here implies otherwise. + * a verdict posted before #632 has no `(base: …)` in its description and must get NO opinion, + rather than denying every in-flight PR the day this lands. + +Observable contract: the hook exits 0 with EMPTY stdout when it has no opinion (passthrough to +normal permissioning), and emits a JSON `permissionDecision` otherwise. +""" + +from __future__ import annotations + +import json +import os +import subprocess +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[2] +HOOK = REPO_ROOT / ".claude" / "hooks" / "pretooluse-merge-consent.sh" + +SHA = "a9e3e23abf337980ca4c05854f5b1e210099d08b" + +# The PR is deliberately NOT docs-only: the docs-only exemption short-circuits the whole gate, so a +# docs PR would never reach the base check and the tests would pass without exercising it. +CURL_SHIM = r'''#!/usr/bin/env python3 +import json, os, sys, pathlib, urllib.parse + +state = pathlib.Path(os.environ["STUB_DIR"]) +args = sys.argv[1:] +url = [a for a in args if a.startswith("http")][-1] + +if "/pulls/" in url and "/files" in url: + q = urllib.parse.parse_qs(urllib.parse.urlparse(url).query) + page = int(q.get("page", ["1"])[0]) + if page == 1: + print(json.dumps([{"filename": "ErsatzTV/Program.cs", "status": "modified"}])) + else: + print("[]") + sys.exit(0) + +if "/status" in url: + desc = (state / "verdict_desc").read_text() + rows = [] if desc == "NONE" else [ + {"context": "review-verdict/h10", "status": "success", "description": desc}] + print(json.dumps({"state": "success", "statuses": rows})) + sys.exit(0) + +if "/pulls/" in url: + print(json.dumps({ + "head": {"sha": os.environ["STUB_SHA"]}, + "base": {"ref": (state / "live_base").read_text().strip()}, + "body": "fixes #1", + })) + sys.exit(0) + +print("{}") +''' + + +@pytest.fixture +def hook(tmp_path): + bindir = tmp_path / "bin"; bindir.mkdir() + curl = bindir / "curl"; curl.write_text(CURL_SHIM); curl.chmod(0o755) + state = tmp_path / "state"; state.mkdir() + (state / "live_base").write_text("main") + (state / "verdict_desc").write_text("Review-verdict: MERGEABLE @ a9e3e23 (base: main)") + + env = dict(os.environ) + env["PATH"] = f"{bindir}{os.pathsep}{env['PATH']}" + env["STUB_DIR"] = str(state) + env["STUB_SHA"] = SHA + env["ETV_GITEA_TOKEN"] = "stub" + env["ETV_GITEA_URL"] = "http://gitea.example" + env.pop("ETV_GITEA_BASICAUTH", None) + + class Handle: + def set_live_base(self, ref): + (state / "live_base").write_text(ref) + + def set_verdict_description(self, desc): + """'NONE' serves a head with no review-verdict/h10 status at all.""" + (state / "verdict_desc").write_text(desc) + + def decision(self): + payload = {"tool_input": {"method": "merge", "owner": "timothy", + "repo": "ersatztv", "pull_number": 42}} + r = subprocess.run(["bash", str(HOOK)], input=json.dumps(payload), + env=env, capture_output=True, text=True) + assert r.returncode == 0, r.stderr + if not r.stdout.strip(): + return None + return json.loads(r.stdout) + + def reason(self): + d = self.decision() + return "" if d is None else json.dumps(d) + + return Handle() + + +def test_a_retargeted_base_denies_a_verdict_formed_against_the_old_one(hook): + hook.set_live_base("release/26.4") + reason = hook.reason() + assert "deny" in reason, "a verdict formed against a different base was allowed to stand" + assert "release/26.4" in reason and "main" in reason, ( + "the deny must name both bases; a reader cannot act on 'the base changed'") + + +def test_positive_control_an_unchanged_base_does_not_trigger_the_base_deny(hook): + """Without this, the test above could pass because the hook denies on every path — which it + very nearly does, since this PR is non-docs and the rest of the gate is unstubbed.""" + reason = hook.reason() + assert "ersatztv#632" not in reason, ( + "the base check fired on a PR whose base never moved") + + +@pytest.mark.parametrize("desc", [ + "Review-verdict: MERGEABLE @ a9e3e23", # posted before #632 + "NONE", # no verdict status on this head at all +]) +def test_a_verdict_with_no_recorded_base_gets_no_opinion(hook, desc): + """Graceful adoption. Denying here would block every in-flight PR the day this lands, and the + window closes on its own: verdicts are per-head and short-lived, so every verdict posted after + #632 carries the field.""" + hook.set_live_base("release/26.4") + hook.set_verdict_description(desc) + assert "ersatztv#632" not in hook.reason(), ( + "a pre-#632 verdict was denied for a field it could not have carried") + + +def test_the_comparator_is_the_base_REF_not_its_tip_sha(): + """The design decision this test exists to freeze. `base.sha` tracks the base branch's TIP, + which moves every time anything merges to `main` — comparing that would invalidate every open + verdict on every unrelated merge, turning a rare-event guard into a permanent merge deadlock. + A base branch that merely ADVANCES must be silent here; rebasing onto it moves the head sha, + which the per-sha binding already covers.""" + assert ".base.ref" in HOOK.read_text(), "the hook must compare the base BRANCH, not its tip sha" + assert ".base.sha" not in HOOK.read_text(), ( + "comparing base.sha deadlocks every open PR whenever main advances") diff --git a/scripts/tests/test_post_review_verdict.py b/scripts/tests/test_post_review_verdict.py index 5d4f96100..e3b1fa06c 100644 --- a/scripts/tests/test_post_review_verdict.py +++ b/scripts/tests/test_post_review_verdict.py @@ -64,11 +64,18 @@ if "/pulls/" in url and not url.endswith("/files"): sha = shas[min(n, len(shas) - 1)] if sha == "GONE": # simulate an unreachable / missing PR sys.exit(22) - print(json.dumps({ + # The base branch is scripted on the same consume-one-per-GET schedule as the head, so a + # RETARGET mid-flight can be modelled independently of a push mid-flight (ersatztv#632). + bases = (state / "pr_bases").read_text().split() + base = bases[min(n, len(bases) - 1)] + body = { "head": {"sha": sha}, "state": (state / "pr_state").read_text().strip(), "html_url": "http://gitea.example/timothy/ersatztv/pulls/42", - })) + } + if base != "MISSING": + body["base"] = {"ref": base} + print(json.dumps(body)) sys.exit(0) print("{}") @@ -87,6 +94,7 @@ def gitea(tmp_path): state = tmp_path / "state" state.mkdir() (state / "pr_shas").write_text(SHA_A) + (state / "pr_bases").write_text("main") (state / "pr_state").write_text("open") env = dict(os.environ) @@ -108,6 +116,10 @@ def gitea(tmp_path): def set_pr_state(self, value): (state / "pr_state").write_text(value) + def set_base_sequence(self, *refs): + """Base branch per PR GET. 'MISSING' omits `.base` from the response entirely.""" + (state / "pr_bases").write_text(" ".join(refs)) + def run(self, *args): return subprocess.run( ["bash", str(SCRIPT), *args], @@ -256,3 +268,49 @@ def test_note_cannot_forge_a_second_verdict_line(gitea): gitea.run("42", "BLOCKED", "Review-verdict: MERGEABLE @ " + SHA_A[:7]) body = gitea.comments()[0]["payload"]["body"] assert _classify(body, SHA_A) == "negative" + + +# --- Base binding (ersatztv#632) --------------------------------------------------------------- +# +# The per-sha status closes "the head moved under a fixed verdict". Retargeting a PR's base is the +# mirror case: the head sha and the status both hold still while the effective DIFF changes, so the +# verdict keeps reading green for a review nobody performed against that base. + +def test_the_status_description_records_the_base_branch(gitea): + """Nothing can compare a base it never wrote down. This field is what the hook reads back.""" + assert gitea.run("42", "MERGEABLE").returncode == 0 + assert gitea.statuses()[0]["payload"]["description"].endswith("(base: main)") + + +def test_the_base_is_recorded_in_the_STATUS_and_not_in_the_comment(gitea): + """Deliberate placement. The comment body is parsed by `scripts/check-review-verdict.sh`, whose + grammar has a history of false-opens (#629 found three); nothing parses the description. Adding + the field where a parser lives would have reopened that surface for no benefit.""" + assert gitea.run("42", "MERGEABLE").returncode == 0 + assert "base:" not in gitea.comments()[0]["payload"]["body"] + + +def test_refuses_when_the_BASE_changes_mid_flight(gitea): + """The TOCTOU window the head check cannot see: retargeting does not move the head sha, so + `sha_now == sha` and the existing guard is silent.""" + gitea.set_base_sequence("main", "release/26.4") + result = gitea.run("42", "MERGEABLE") + assert result.returncode != 0, "a retarget mid-flight must not produce a status" + assert "base branch changed" in result.stderr + assert gitea.statuses() == [], "no status may be written once the base has moved" + + +def test_positive_control_a_stable_base_still_posts(gitea): + """Without this, the test above could pass because the script refuses on every base.""" + gitea.set_base_sequence("main", "main") + assert gitea.run("42", "MERGEABLE").returncode == 0 + assert len(gitea.statuses()) == 1 + + +def test_refuses_when_the_pr_has_no_resolvable_base(gitea): + """A verdict that cannot record what it was formed against is not a verdict this gate can + later re-check, so it fails closed rather than posting an unbindable success.""" + gitea.set_base_sequence("MISSING") + result = gitea.run("42", "MERGEABLE") + assert result.returncode != 0 + assert gitea.statuses() == [] From f0f8708a6ea7240e323ce5dd1a8ade2c6df18521 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 23:16:35 +0200 Subject: [PATCH 2/4] fix(632): fail closed when the head/base re-read itself fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review of the previous commit. Folding the head and base re-reads into one `prjson_now=$(api_get ... || true)` swallowed a guard that used to be implicit: the old `sha_now=$(api_get ... | jq ...)` aborted under `set -e` + `pipefail` when the GET failed, before any status was written. With `|| true`, both `sha_now` and `base_now` come back empty, both `[ -n ... ]` guards no-op, and the status is written having confirmed nothing about either the head or the base — a fail-open regression introduced by the refactor itself. Confirmed the old behaviour empirically rather than by reading it: a failed piped command substitution under `set -euo pipefail` exits with curl's status. The refusal is now explicit, and pinned by a test — nothing asserted it before, which is exactly why the refactor could drop it silently. Mutation-verified: restoring `|| true` reddens that test alone. Refs #632 --- scripts/post-review-verdict.sh | 9 ++++++++- scripts/tests/test_post_review_verdict.py | 16 ++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/scripts/post-review-verdict.sh b/scripts/post-review-verdict.sh index a7458f376..649623279 100755 --- a/scripts/post-review-verdict.sh +++ b/scripts/post-review-verdict.sh @@ -123,7 +123,14 @@ printf 'posted comment: Review-verdict: %s @ %s\n' "$verdict" "$short" # a verdict written for its parent — reintroducing ersatztv#622 at a smaller time scale. We do NOT # retry against the new head: the new commit is genuinely unreviewed, and silently re-targeting the # verdict at it is exactly the failure this script exists to prevent. -prjson_now=$(api_get "repos/$owner/$repo/pulls/$pr" || true) +# Fail CLOSED if the re-read itself fails. This used to be `sha_now=$(api_get ... | jq ...)`, where +# `set -e` + `pipefail` aborted the script on a failed GET — implicitly, but before any status was +# written. Folding the two reads into one variable with `|| true` would have swallowed that: both +# `sha_now` and `base_now` come back empty, both `[ -n … ]` guards become no-ops, and the status is +# written having confirmed NOTHING about the head or the base. That is a fail-open regression +# introduced by the refactor, so the refusal is now explicit rather than a side effect of `set -e`. +prjson_now=$(api_get "repos/$owner/$repo/pulls/$pr") \ + || die "could not re-read PR #$pr to confirm the head and base had not moved while posting — no status was written. Re-run once Gitea is reachable." sha_now=$(printf '%s' "$prjson_now" | jq -r '.head.sha // ""') if [ -n "$sha_now" ] && [ "$sha_now" != "$sha" ]; then die "head moved from $short to ${sha_now:0:7} while posting — that commit is UNREVIEWED, so no status was written. Re-review the new head and run this again." diff --git a/scripts/tests/test_post_review_verdict.py b/scripts/tests/test_post_review_verdict.py index e3b1fa06c..f573d82d5 100644 --- a/scripts/tests/test_post_review_verdict.py +++ b/scripts/tests/test_post_review_verdict.py @@ -314,3 +314,19 @@ def test_refuses_when_the_pr_has_no_resolvable_base(gitea): result = gitea.run("42", "MERGEABLE") assert result.returncode != 0 assert gitea.statuses() == [] + + +def test_a_failed_HEAD_RECHECK_writes_no_status(gitea): + """Fail-closed on the re-read itself, not just on a moved head. + + This guard was previously implicit: `sha_now=$(api_get ... | jq ...)` aborted under `set -e` + + `pipefail` when the GET failed. Nothing asserted it, so folding the head and base re-reads into + one `$(... || true)` variable silently converted it to fail-OPEN — both guards see an empty + string, both no-op, and the status is written having confirmed nothing. Asserted now so the + behaviour is a contract rather than a side effect of a shell option. + """ + gitea.set_head_sequence(SHA_A, "GONE") + result = gitea.run("42", "MERGEABLE") + assert result.returncode != 0 + assert gitea.statuses() == [], ( + "a status was written even though the head/base re-read failed — nothing was confirmed") From d51255a8ef0b168c6a993fe7e680f7c4e4864956 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 23:27:22 +0200 Subject: [PATCH 3/4] fix(632): "could not check" is a third outcome, not a quiet synonym for "nothing to check" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cold review's substantive finding. The first draft collapsed an unreadable status response into the graceful-adoption path: `vdesc` came back empty, so `recorded_base` was empty, so the comparison was skipped IN SILENCE — and a later, successful status read could then auto-grant, emitting "merge gate: satisfied" for a comparison that never happened. A transient Gitea hiccup is not evidence that the base is unchanged. The unreadable status response and a PR with no resolvable `.base.ref` now both fall through to a human `ask`, leaving exactly one benign silent case: a verdict that predates #632 and could not have carried the field. The emptiness check is done in SHELL before jq sees it, same jq-1.6 rule as the rest of this file. Also from review: the graceful-adoption test asserted only that the decision lacked the issue tag, so it would have passed for a base-specific ask or deny whose wording omitted it — the failure mode most likely to appear when someone edits these messages. It now asserts on the word "base". Recorded rather than fixed, because fixing it would be worse: docs-only PRs exit before this check, since that carve-out short-circuits the gate earlier. It does not auto-grant — it passes through to an ordinary permission prompt — so the exposure is a missing warning on a merge a human is already confirming, not a silent merge. The record now says so instead of implying the deny is unconditional. Mutation-verified: collapsing the unreadable case back into graceful adoption, skipping the check on a missing live base, and dropping the mismatch deny each redden their own test and nothing else. Refs #632 --- .claude/hooks/pretooluse-merge-consent.sh | 26 ++++++++-- .../records/release/verdict-status-check.md | 11 ++++- .../tests/test_merge_consent_base_change.py | 49 ++++++++++++++++--- 3 files changed, 73 insertions(+), 13 deletions(-) diff --git a/.claude/hooks/pretooluse-merge-consent.sh b/.claude/hooks/pretooluse-merge-consent.sh index b2851a664..251da314a 100755 --- a/.claude/hooks/pretooluse-merge-consent.sh +++ b/.claude/hooks/pretooluse-merge-consent.sh @@ -136,12 +136,30 @@ fi # posted before ersatztv#632 and gets NO opinion, rather than denying every in-flight PR the day # this lands. The window closes on its own — verdicts are per-head and short-lived, so every verdict # posted after this carries the field. +# "Could not check" is a THIRD outcome, distinct from both "matches" and "no base recorded". Cold +# review found the first draft collapsing it into the latter: an unreadable status response yielded +# an empty `recorded_base`, which took the graceful-adoption path and skipped validation silently — +# after which a later, successful status read could still auto-grant. A transient failure would then +# have produced a "merge gate: satisfied" message for a comparison that never happened. Every +# unreadable input here therefore falls through to a human (`ask`), never to silence. live_base=$(printf '%s' "$prjson" | jq -r '.base.ref // ""' 2>/dev/null || true) -if [ -n "$sha" ] && [ -n "$live_base" ]; then - vdesc=$(gq "repos/$owner/$repo/commits/$sha/status?limit=100" \ - | jq -r '[.statuses[]? | select(.context == "review-verdict/h10")] | first | .description // ""' \ +if [ -z "$live_base" ]; then + decide ask "H10 merge gate: PR #$pr reports no base branch (.base.ref), so the verdict cannot be checked against the branch it was formed for (ersatztv#632). Confirm the PR still targets the branch it was reviewed against before merging." +fi +if [ -n "$sha" ]; then + vjson_base=$(gq "repos/$owner/$repo/commits/$sha/status?limit=100") + # Same jq-1.6 rule as everywhere else in this file: check emptiness in SHELL first, never via + # `jq -e`'s exit status over empty input. + if [ -z "${vjson_base//[[:space:]]/}" ] || ! printf '%s' "$vjson_base" | jq -e '.statuses | type == "array"' >/dev/null 2>&1; then + decide ask "H10 merge gate: could not read the commit statuses for PR #$pr head ${sha:0:7}, so the verdict could not be checked against the PR's base branch (ersatztv#632). Confirm the review covered the branch this PR currently targets ('$live_base') before merging." + fi + vdesc=$(printf '%s' "$vjson_base" \ + | jq -r '[.statuses[] | select(.context == "review-verdict/h10")] | first | .description // ""' \ 2>/dev/null || true) - # The field is written by scripts/post-review-verdict.sh as a trailing `(base: )`. + # The field is written by scripts/post-review-verdict.sh as a trailing `(base: )`. Its + # ABSENCE is the one benign case: a verdict posted before ersatztv#632 could not have carried it, + # and denying those would block every in-flight PR the day this lands. The window closes on its + # own, since verdicts are per-head and short-lived. recorded_base=$(printf '%s' "$vdesc" | sed -n 's/.*(base: \(.*\))$/\1/p') if [ -n "$recorded_base" ] && [ "$recorded_base" != "$live_base" ]; then decide deny "H10 merge gate: BLOCKED — the review verdict on head ${sha:0:7} was formed while PR #$pr targeted '$recorded_base', but it now targets '$live_base'. Retargeting a base does not move the head sha, so the per-sha verdict status still reads green even though the effective diff has changed (ersatztv#632). Re-review against the new base and run: scripts/post-review-verdict.sh $pr MERGEABLE" diff --git a/docs/decisions/records/release/verdict-status-check.md b/docs/decisions/records/release/verdict-status-check.md index 83ed38a27..9175d1f3b 100644 --- a/docs/decisions/records/release/verdict-status-check.md +++ b/docs/decisions/records/release/verdict-status-check.md @@ -162,7 +162,16 @@ described as one: Verdicts posted before #632 carry no `(base: …)` and get **no opinion** rather than a deny — the alternative would block every in-flight PR the day it lands, and the window closes on its own since - verdicts are per-head and short-lived. + verdicts are per-head and short-lived. **"Could not check" is a third outcome**, deliberately not + folded into that one: an unreadable status response or a PR with no resolvable `.base.ref` falls + through to a human `ask`. The first draft collapsed them, so a transient Gitea hiccup skipped the + comparison in silence and a later successful read could still emit "merge gate: satisfied" for a + check that never ran. + + **Docs-only PRs exit before this check**, because the docs-only carve-out short-circuits the whole + gate earlier in the hook. That carve-out does not auto-grant — it passes through to an ordinary + permission prompt — so the exposure is a missing warning on a merge a human is already confirming, + not a silent merge. Worth knowing before reading "the hook denies on a retarget" as unconditional. 4. **A PR that edits `review-verdict.yml` is judged by its own edited copy.** Gitea runs `pull_request` workflows from the PR **head**, not the base — confirmed on the very PR that introduced this workflow (#630): `review-verdict.yml` does not exist on `main`, yet its job ran diff --git a/scripts/tests/test_merge_consent_base_change.py b/scripts/tests/test_merge_consent_base_change.py index 8e4b294ce..5018599b2 100644 --- a/scripts/tests/test_merge_consent_base_change.py +++ b/scripts/tests/test_merge_consent_base_change.py @@ -51,17 +51,21 @@ if "/pulls/" in url and "/files" in url: if "/status" in url: desc = (state / "verdict_desc").read_text() + if desc == "TRANSPORT-ERROR": + sys.exit(22) + if desc == "GARBAGE": + print('{"message":"internal error"}'); sys.exit(0) rows = [] if desc == "NONE" else [ {"context": "review-verdict/h10", "status": "success", "description": desc}] print(json.dumps({"state": "success", "statuses": rows})) sys.exit(0) if "/pulls/" in url: - print(json.dumps({ - "head": {"sha": os.environ["STUB_SHA"]}, - "base": {"ref": (state / "live_base").read_text().strip()}, - "body": "fixes #1", - })) + body = {"head": {"sha": os.environ["STUB_SHA"]}, "body": "fixes #1"} + live = (state / "live_base").read_text().strip() + if live != "MISSING": + body["base"] = {"ref": live} + print(json.dumps(body)) sys.exit(0) print("{}") @@ -132,11 +136,40 @@ def test_positive_control_an_unchanged_base_does_not_trigger_the_base_deny(hook) def test_a_verdict_with_no_recorded_base_gets_no_opinion(hook, desc): """Graceful adoption. Denying here would block every in-flight PR the day this lands, and the window closes on its own: verdicts are per-head and short-lived, so every verdict posted after - #632 carries the field.""" + #632 carries the field. + + Asserting on the word "base" rather than on the issue tag, per cold review: the tag-only check + would have passed for a base-specific ask or deny whose wording happened to omit it, which is + the failure mode most likely to appear when someone edits these messages. + """ hook.set_live_base("release/26.4") hook.set_verdict_description(desc) - assert "ersatztv#632" not in hook.reason(), ( - "a pre-#632 verdict was denied for a field it could not have carried") + assert "base" not in hook.reason(), ( + "a pre-#632 verdict drew a base-related decision for a field it could not have carried") + + +@pytest.mark.parametrize("failure", ["TRANSPORT-ERROR", "GARBAGE"]) +def test_an_UNREADABLE_status_response_asks_rather_than_skipping_the_check(hook, failure): + """"Could not check" is a third outcome, not a quiet synonym for "no base recorded". + + The first draft collapsed the two: an unreadable status response produced an empty + `recorded_base`, took the graceful-adoption path, and skipped validation in silence — after + which a later successful status read could still auto-grant, emitting "merge gate: satisfied" + for a comparison that never happened. A transient Gitea hiccup is not evidence that the base is + unchanged. + """ + hook.set_live_base("release/26.4") + hook.set_verdict_description(failure) + reason = hook.reason() + assert "ask" in reason, "an unreadable status response silently skipped the base check" + assert "base" in reason, "the ask must name what could not be checked" + + +def test_a_pr_with_no_resolvable_base_asks(hook): + """A null/absent `.base.ref` is also 'could not check', not 'nothing to check'.""" + hook.set_live_base("MISSING") + reason = hook.reason() + assert "ask" in reason and "base" in reason def test_the_comparator_is_the_base_REF_not_its_tip_sha(): From ed8de77e10a61303a16c1b6aca108eedbe0893a5 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 23:38:14 +0200 Subject: [PATCH 4/4] =?UTF-8?q?fix(632):=20validate=20status=20ROWS,=20not?= =?UTF-8?q?=20just=20the=20top-level=20array=20=E2=80=94=20the=20same=20sw?= =?UTF-8?q?allow=20one=20level=20down?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review caught my previous fix claiming more than it delivered. "Every unreadable input asks" was false: validating only that `.statuses` is an array left `{"statuses":[1]}` passing the guard, after which `.context` on a number errors and the `|| true` on the extraction turned that error into an empty description — straight back onto the graceful-adoption path the guard exists to distinguish from. The identical swallow-the-error shape I had just fixed a few lines up, surviving one level deeper. The validation domain now matches the CONSUMPTION domain: every row must be an object with a string `.context` and a `.description` that is absent or a string. The extraction drops its `|| true` and asks explicitly instead, since a swallowed error there is indistinguishable from a benign "no base recorded". Both guards are load-bearing, for DIFFERENT shapes — established by mutating them together and separately rather than assuming the pair was redundant: - a non-string `.description` is caught ONLY by the row validation (jq -r renders the object as JSON, the sed finds no `(base: …)`, and it silently reads as a legacy verdict); - a scalar row is caught by EITHER, so with the validation weakened the extraction guard is what still asks. Also noted rather than changed: this is the third read of the same status endpoint in a worst-case hook run. Sharing one snapshot would close a narrow same-run disagreement window, but the other two branches derive different decisions from a failed read, so threading a shared response through them changes pre-existing logic rather than #632's. Recorded in place so it is not rediscovered as an oversight — every `decide` exits immediately, so the reads cannot produce one self-contradictory message. Refs #632 --- .claude/hooks/pretooluse-merge-consent.sh | 32 ++++++++++++++++--- .../tests/test_merge_consent_base_change.py | 23 +++++++++++++ 2 files changed, 51 insertions(+), 4 deletions(-) diff --git a/.claude/hooks/pretooluse-merge-consent.sh b/.claude/hooks/pretooluse-merge-consent.sh index 251da314a..5271dc7ea 100755 --- a/.claude/hooks/pretooluse-merge-consent.sh +++ b/.claude/hooks/pretooluse-merge-consent.sh @@ -147,15 +147,39 @@ if [ -z "$live_base" ]; then decide ask "H10 merge gate: PR #$pr reports no base branch (.base.ref), so the verdict cannot be checked against the branch it was formed for (ersatztv#632). Confirm the PR still targets the branch it was reviewed against before merging." fi if [ -n "$sha" ]; then + # This is the THIRD read of this endpoint in a worst-case hook run (the ordinary-CI branch and the + # scheduled-auto-merge branch each do their own). Sharing one snapshot would close a narrow + # same-run window where two reads disagree, but the later branches derive different decisions from + # a failed read than this one does, so threading a shared response through them is a change to + # pre-existing logic rather than to ersatztv#632's. Left deliberately, noted so it is not + # rediscovered as an oversight: every `decide` exits immediately, so the reads cannot produce a + # single self-contradictory message — only a later decision made on a fresher snapshot. vjson_base=$(gq "repos/$owner/$repo/commits/$sha/status?limit=100") # Same jq-1.6 rule as everywhere else in this file: check emptiness in SHELL first, never via # `jq -e`'s exit status over empty input. - if [ -z "${vjson_base//[[:space:]]/}" ] || ! printf '%s' "$vjson_base" | jq -e '.statuses | type == "array"' >/dev/null 2>&1; then + # VALIDATE EVERY FIELD THE EXTRACTION CONSUMES, on EVERY row — the same rule the file-enumeration + # guard learned the hard way. Checking only that `.statuses` is an array left a hole one level + # down: `{"statuses":[1]}` passes a top-level type check, then `.context` on a number errors, and + # a `|| true` on the extraction turned that error into an empty `vdesc` — i.e. straight back onto + # the graceful-adoption path this block exists to distinguish from. That is the identical + # swallow-the-error shape fixed a few lines up, surviving one level deeper. + if [ -z "${vjson_base//[[:space:]]/}" ] \ + || ! printf '%s' "$vjson_base" \ + | jq -e '.statuses | type == "array" + and all(.[]; type == "object" + and (.context | type == "string") + and (.description == null or (.description | type == "string")))' \ + >/dev/null 2>&1; then decide ask "H10 merge gate: could not read the commit statuses for PR #$pr head ${sha:0:7}, so the verdict could not be checked against the PR's base branch (ersatztv#632). Confirm the review covered the branch this PR currently targets ('$live_base') before merging." fi - vdesc=$(printf '%s' "$vjson_base" \ - | jq -r '[.statuses[] | select(.context == "review-verdict/h10")] | first | .description // ""' \ - 2>/dev/null || true) + # No `|| true` here. The validation above makes an error unreachable, but a swallowed error would + # be indistinguishable from "no base recorded" — the exact confusion this block removes — so the + # failure is handled explicitly rather than left to a fallback that reads as a benign result. + if ! vdesc=$(printf '%s' "$vjson_base" \ + | jq -r '[.statuses[] | select(.context == "review-verdict/h10")] | first | .description // ""' \ + 2>/dev/null); then + decide ask "H10 merge gate: the commit statuses for PR #$pr head ${sha:0:7} could not be parsed to find the review verdict, so it could not be checked against the PR's base branch (ersatztv#632). Confirm the review covered the branch this PR currently targets ('$live_base') before merging." + fi # The field is written by scripts/post-review-verdict.sh as a trailing `(base: )`. Its # ABSENCE is the one benign case: a verdict posted before ersatztv#632 could not have carried it, # and denying those would block every in-flight PR the day this lands. The window closes on its diff --git a/scripts/tests/test_merge_consent_base_change.py b/scripts/tests/test_merge_consent_base_change.py index 5018599b2..f7343ac3f 100644 --- a/scripts/tests/test_merge_consent_base_change.py +++ b/scripts/tests/test_merge_consent_base_change.py @@ -55,6 +55,12 @@ if "/status" in url: sys.exit(22) if desc == "GARBAGE": print('{"message":"internal error"}'); sys.exit(0) + if desc == "SCALAR-ROW": + print('{"state":"success","statuses":[1]}'); sys.exit(0) + if desc == "NONSTRING-DESC": + print(json.dumps({"state": "success", "statuses": [ + {"context": "review-verdict/h10", "status": "success", "description": {"x": 1}}]})) + sys.exit(0) rows = [] if desc == "NONE" else [ {"context": "review-verdict/h10", "status": "success", "description": desc}] print(json.dumps({"state": "success", "statuses": rows})) @@ -148,6 +154,23 @@ def test_a_verdict_with_no_recorded_base_gets_no_opinion(hook, desc): "a pre-#632 verdict drew a base-related decision for a field it could not have carried") +@pytest.mark.parametrize("failure", ["SCALAR-ROW", "NONSTRING-DESC"]) +def test_a_malformed_status_MEMBER_asks_too(hook, failure): + """One level below the previous fix, and it survived it. + + Validating only that `.statuses` is an array left `{"statuses":[1]}` passing the guard, after + which `.context` on a number errors and a `|| true` on the extraction turned that error into an + empty description — straight back onto the graceful-adoption path, which is precisely the + outcome the guard exists to distinguish from. Same swallow-the-error shape as the bug one level + up, which is why the validation domain must match the CONSUMPTION domain rather than stopping at + the top-level type. + """ + hook.set_live_base("release/26.4") + hook.set_verdict_description(failure) + reason = hook.reason() + assert "ask" in reason and "base" in reason + + @pytest.mark.parametrize("failure", ["TRANSPORT-ERROR", "GARBAGE"]) def test_an_UNREADABLE_status_response_asks_rather_than_skipping_the_check(hook, failure): """"Could not check" is a third outcome, not a quiet synonym for "no base recorded".