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

4 Commits

Author	SHA1	Message	Date
timothy	ed8de77e10	fix(632): validate status ROWS, not just the top-level array — the same swallow one level down ... 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	2026-07-26 23:38:14 +02:00
timothy	d51255a8ef	fix(632): "could not check" is a third outcome, not a quiet synonym for "nothing to check" ... 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	2026-07-26 23:27:22 +02:00
timothy	f0f8708a6e	fix(632): fail closed when the head/base re-read itself fails ... 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	2026-07-26 23:16:35 +02:00
timothy	00e623c066	fix(632): bind a review verdict to its BASE branch, not only to its head sha ... #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	2026-07-26 23:13:50 +02:00