fix(632): bind a review verdict to its BASE branch, not only to its head sha #667

Merged

timothy merged 4 commits from fix/632-verdict-base-ref into main 2026-07-26 23:58:50 +02:00

Conversation 1 Commits 4 Files Changed 6

+427 -6

timothy commented 2026-07-26 23:39:35 +02:00

Owner

Copy Link

Copy Source

fixes #632

#622 made review-verdict/h10 a per-sha required status, so a new commit cannot inherit an old verdict — a new commit is a new sha, and the required context is simply absent. 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.

What changed

• scripts/post-review-verdict.sh records the base branch in the status description as a trailing (base: <ref>), and refuses to write a status 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.
• .claude/hooks/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, stated 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. Docs-only PRs also 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 there is a missing warning on a merge a human is already confirming, not a silent merge. The decision record says so explicitly rather than implying the deny is unconditional.

Accepted exposure: base changes are rare, manual, and this is a two-account repo. It now fails loud in the place that evaluates consent instead of living only in a doc.

Two fail-opens found during review — one mine, one one level deeper

Both are worth reading, because each looked like a completed fix:

1. || true on the re-read. Folding the head and base re-reads into one prjson_now=$(api_get … || true) swallowed a guard that had only ever been 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 guards see an empty string, both no-op, and the status is written having confirmed nothing. Confirmed empirically rather than by reading; now explicit and pinned. Nothing had asserted it, which is exactly why a refactor could drop it silently.

2. Validating only the top-level .statuses array. "Every unreadable input asks" was false: {"statuses":[1]} passed the guard, then .context on a number errored, 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 as (1), surviving one level deeper. The validation domain now matches the consumption domain.

Both guards there are load-bearing for different shapes, established by mutating them together and separately rather than assuming the pair redundant: a non-string .description is caught only by the row validation; a scalar row is caught by either.

"Could not check" is a third outcome, deliberately not folded into "no base recorded": an unreadable status response or a PR with no resolvable .base.ref falls through to a human ask. Only one silent case remains — a verdict predating this change, which could not have carried the field. That window closes on its own, since verdicts are per-head and short-lived.

Verification

Mutation, ten mutants, each killed by its intended test: drop the hook's deny; compare base.sha; drop graceful adoption; stop recording the base; drop the TOCTOU guard; accept a PR with no resolvable base; swallow the re-read failure; skip on a missing live base; revert to top-level-only validation; restore || true on the extraction.

The positive controls matter more than usual here: the test PR is deliberately non-docs (a docs-only PR short-circuits the gate and would never reach the base check) and the rest of the gate is unstubbed, so "the hook denied" alone proves nothing.

Review

Two rounds of cold cross-family review (Codex), the second re-reviewing the fix commits. Round 1 independently found fail-open (1), which had already been fixed; round 2 found (2) and confirmed the docs-only severity characterisation by tracing that the carve-out calls decide allow and never decide grant.

fixes #632 #622 made `review-verdict/h10` a per-sha required status, so a new commit cannot inherit an old verdict — a new commit is a new sha, and the required context is simply absent. **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. ## What changed - `scripts/post-review-verdict.sh` records the base branch in the status description as a trailing `(base: <ref>)`, and refuses to write a status 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. - `.claude/hooks/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, stated 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. **Docs-only PRs also 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 there is a missing warning on a merge a human is already confirming, not a silent merge. The decision record says so explicitly rather than implying the deny is unconditional. Accepted exposure: base changes are rare, manual, and this is a two-account repo. It now fails loud in the place that evaluates consent instead of living only in a doc. ## Two fail-opens found during review — one mine, one one level deeper Both are worth reading, because each *looked* like a completed fix: 1. **`|| true` on the re-read.** Folding the head and base re-reads into one `prjson_now=$(api_get … || true)` swallowed a guard that had only ever been **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 guards see an empty string, both no-op, and the status is written having confirmed nothing. Confirmed empirically rather than by reading; now explicit and pinned. Nothing had asserted it, which is exactly why a refactor could drop it silently. 2. **Validating only the top-level `.statuses` array.** "Every unreadable input asks" was false: `{"statuses":[1]}` passed the guard, then `.context` on a number errored, 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 as (1), surviving one level deeper. The validation domain now matches the consumption domain. Both guards there are load-bearing for **different** shapes, established by mutating them together and separately rather than assuming the pair redundant: a non-string `.description` is caught only by the row validation; a scalar row is caught by either. **"Could not check" is a third outcome**, deliberately not folded into "no base recorded": an unreadable status response or a PR with no resolvable `.base.ref` falls through to a human `ask`. Only one silent case remains — a verdict predating this change, which could not have carried the field. That window closes on its own, since verdicts are per-head and short-lived. ## Verification Mutation, ten mutants, each killed by its intended test: drop the hook's deny; compare `base.sha`; drop graceful adoption; stop recording the base; drop the TOCTOU guard; accept a PR with no resolvable base; swallow the re-read failure; skip on a missing live base; revert to top-level-only validation; restore `|| true` on the extraction. The positive controls matter more than usual here: the test PR is deliberately **non-docs** (a docs-only PR short-circuits the gate and would never reach the base check) and the rest of the gate is unstubbed, so "the hook denied" alone proves nothing. ## Review Two rounds of cold cross-family review (Codex), the second re-reviewing the fix commits. Round 1 independently found fail-open (1), which had already been fixed; round 2 found (2) and confirmed the docs-only severity characterisation by tracing that the carve-out calls `decide allow` and never `decide grant`.

timothy commented 2026-07-26 23:47:49 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ ed8de77

Three rounds of cold cross-family review (Codex).

Round 1 independently found the '|| true' fail-open on the head/base re-read, which had already been found and fixed by self-review. Round 2 found the same swallow-the-error shape surviving ONE LEVEL DOWN: validating only that .statuses is an array left {"statuses":[1]} passing, after which .context on a number errored and the extraction's '|| true' turned that into an empty description - straight back onto the graceful-adoption path. Fixed by matching the validation domain to the consumption domain. Round 3 confirmed the layering and found only a Low outside the documented API contract.

Accepted, not fixed: duplicate well-typed review-verdict/h10 rows would let array order decide whether a mismatch is denied. Gitea's combined-status endpoint returns latest-per-context, so duplicates are outside its contract; hardening 'first' against a shape the API does not produce is speculative.

Ten mutants killed, each by its intended test. Scope is stated rather than overclaimed: detection on the hook path only, docs-only PRs exit before the check (that carve-out calls decide allow, never decide grant, so it cannot auto-grant), and the comparator is base.ref rather than base.sha because a tip comparison would deadlock every open PR whenever main advances.

Review-verdict: MERGEABLE @ ed8de77 Three rounds of cold cross-family review (Codex). Round 1 independently found the '|| true' fail-open on the head/base re-read, which had already been found and fixed by self-review. Round 2 found the same swallow-the-error shape surviving ONE LEVEL DOWN: validating only that .statuses is an array left {"statuses":[1]} passing, after which .context on a number errored and the extraction's '|| true' turned that into an empty description - straight back onto the graceful-adoption path. Fixed by matching the validation domain to the consumption domain. Round 3 confirmed the layering and found only a Low outside the documented API contract. Accepted, not fixed: duplicate well-typed review-verdict/h10 rows would let array order decide whether a mismatch is denied. Gitea's combined-status endpoint returns latest-per-context, so duplicates are outside its contract; hardening 'first' against a shape the API does not produce is speculative. Ten mutants killed, each by its intended test. Scope is stated rather than overclaimed: detection on the hook path only, docs-only PRs exit before the check (that carve-out calls decide allow, never decide grant, so it cannot auto-grant), and the comparator is base.ref rather than base.sha because a tip comparison would deadlock every open PR whenever main advances.

timothy changed target branch from main to scratch/632-alternate-base 2026-07-26 23:48:06 +02:00

timothy changed target branch from scratch/632-alternate-base to main 2026-07-26 23:48:11 +02:00

timothy added 4 commits 2026-07-26 23:48:11 +02:00

fix(632): bind a review verdict to its BASE branch, not only to its head sha ... 00e623c066

#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: <ref>)`, 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

fix(632): fail closed when the head/base re-read itself fails ... f0f8708a6e

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

fix(632): "could not check" is a third outcome, not a quiet synonym for "nothing to check" ... d51255a8ef

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

fix(632): validate status ROWS, not just the top-level array — the same swallow one level down ... ed8de77e10

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

timothy referenced this pull request 2026-07-26 23:48:44 +02:00

A PR base change leaves the head sha — and so the `review-verdict/h10` status — unchanged #632

timothy merged commit 8dcd4f3602 into main 2026-07-26 23:58:50 +02:00

timothy deleted branch fix/632-verdict-base-ref 2026-07-26 23:58:51 +02:00

timothy referenced this issue from a commit 2026-07-26 23:58:52 +02:00

Merge pull request 'fix(632): bind a review verdict to its BASE branch, not only to its head sha' (#667) from fix/632-verdict-base-ref into main

timothy referenced this pull request 2026-07-26 23:59:24 +02:00

A PR base change leaves the head sha — and so the `review-verdict/h10` status — unchanged #632

timothy referenced this pull request 2026-07-27 00:26:35 +02:00

test(649): cover the review-verdict status read and the bot-path guards #673

timothy referenced this pull request 2026-07-27 07:44:52 +02:00

docs: permit subagents explicitly, and make claiming an issue a check rather than a label #682

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

ad-hoc

One-off / ad-hoc work not tracked by a dedicated issue

api

REST API / HTTP endpoints

bug

Something isn't working

ci-cd

Build, test, deploy pipeline

content

Channel content / schedules / playlists

dependencies

Dependency updates (Renovate)

enhancement

New feature or improvement

frontend

ChicoryTV React SPA frontend

in-progress

Claimed by an active session — do not pick up

jellyfin

Jellyfin tuner / IPTV integration

parked

Excluded from automatic queue pickup; work only when explicitly selected

priority: high

priority: low

priority: medium

review

Adversarial review finding

security

Security / vulnerability fix

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate timothy (Timothy)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: timothy/ersatztv#667