fix(622): bind H10 merge consent to the reviewed sha via a required commit status #630
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: timothy/ersatztv#630
Reference in New Issue
Block a user
fixes #622
The hole
pretooluse-merge-consent.shproves all three consent conditions at the moment the merge tool is called. Withmerge_when_checks_succeed=true, condition (a) is delegated to Gitea — which then merges whatever head is green at its merge time, while (b) Done-when and (c) the head-referencing verdict were proven against the head at scheduling time. Commits pushed in between merge unreviewed. The gate isn't bypassed; it's satisfied against a snapshot that stops being true.Correction to the issue's premise
#622's motivating anecdote is wrong, and this PR corrects it rather than repeating it. PR #619 does carry
Review-verdict: MERGEABLE @ 02c82b35, posted at 20:36:02 — six seconds before the 20:36:08 merge — explicitly re-reviewing the two follow-up commits. The issue was filed off a lagging API read. Caught by the independent reviewer, after I had already propagated the claim into a commit message, a decision record and two docs.The hole is real anyway, and structural: nothing forced that re-review inside the 45-minute window Gitea would have merged in. It held by diligence. This makes it hold by construction.
Demonstrated, not asserted
Controlled A/B on throwaway branches (
main's protection never touched), with a CI check left pending so Gitea genuinely waits:ci/fakeci/fake,review-verdict/h10Also verified against live Gitea 1.25.4: a missing required context blocks (an unrelated status event forced a real re-evaluation and it still refused), and
.status— not.state— is the field the status-list endpoint populates.The fix
A commit status belongs to one sha, so a status written for a parent cannot be inherited by a child that didn't exist yet.
review-verdict/h10becomes a required check: push a new head and the context is simply absent. No re-evaluation hook, no polling, and it covers merge paths the PreToolUse hook never sees (UI, raw API, another session).scripts/post-review-verdict.sh— writes comment + status together; re-reads head afterwards and writes no status if a commit slipped in, rather than retargeting the verdict at unread code..gitea/workflows/review-verdict.yml— auto-passes Renovate-authored and docs-only PRs (Renovate usesplatformAutomerge, so it would otherwise stall);pendingotherwise. Exemptions void for.claude/,.gitea/,.husky/,scripts/,docker/ci/.Two silent false negatives, found by verifying rather than reasoning
Both fixed at both call sites — the new workflow and the hook's pre-existing docs-only carve-out:
limit. PR #619 has 194 changed files;?limit=100returns 50. A single page saw zero protected paths there where full enumeration finds ten. Both now page to exhaustion and withhold the exemption if they can't complete.filenameis the destination, source only inprevious_filename. Verified live:.gitea/workflows/renovate.yml→docs/innocuous-note.mdpresented as docs-only with no protected path visible. Both now read both sides. (An initial check against #619 suggested Gitea exposed no rename data — a false negative, since that PR contained no renames. A constructed positive case settled it.)Limits, stated rather than papered over
Base changes leave the head sha (and so the verdict) unchanged; and a PR editing
review-verdict.ymlis judged by its own edited copy if Gitea runspull_requestworkflows from the head — soPROTECTEDis a guardrail against accident, not a tamper-proof control. Fine for a two-account repo; documented in the decision record.Verification
95 script tests green, shellcheck clean, decisions validator + catalog
--check+ kickoff guard OK. New suite mutation-proven: removing the TOCTOU guard, flippingBLOCKED→success, and posting on a short sha each fail a test. Tests cross-check the emitted comment against the hook's own condition-(c) regexes so the two implementations can't drift.review-verdict/h10is not yet a required context — I add it to branch protection after this merges, so this PR isn't gated by a check that doesn't exist onmainyet.Cross-family re-review returned BLOCKED on the fix commit, correctly. Two of the three findings were on the FAILURE path, which is why every happy-path run and the whole first test suite looked fine. 1. (High/Medium) Both paging loops treated a failed or non-array page as a legitimate short final page: empty output counts as zero rows, zero rows reads as "end of list", and the enumeration completed from a PARTIAL list — failing OPEN at exactly the point the guard exists to fail closed, while the decision record claimed the opposite. Each page is now validated as a JSON array before its rows are counted; anything else withholds the exemption. 2. (High) The workflow's "is there already a verdict" pre-check read `/statuses/{sha}`, which returns one row per status POST rather than per context and pages at 50 — so a head with a few CI reruns can push a real verdict off the first page (this PR's head already carries 15 rows for 11 contexts). Missing it there is not benign: the job would post `pending`, or an exemption `success`, OVER a human verdict. Both the workflow and the hook now read `/commits/{sha}/status` (latest-per-context), and the workflow refuses to post anything when that read fails rather than inferring "no verdict yet". Credit: raised independently by the parallel session on this issue. 3. (Low) The record claimed both callers withhold the exemption when enumeration cannot complete. True only after (1); the claim and the code now agree. Adds scripts/tests/test_merge_consent_exemption.py — 9 tests over the hook's exemption path via a curl shim, asserting behaviour when a page ERRORS or returns a non-array body, not merely when it returns data. Mutation-proven: reverting the array validation fails both failure-path tests; dropping `previous_filename` fails the rename test. One test I wrote was wrong and the run caught it: I asserted a `.gitea/... -> docs/...` rename must break the hook's exemption, but that hook deliberately exempts `.claude/`/`.gitea/`/`.husky/` too (to a human PROMPT, never an auto-grant), so both sides legitimately match. The rename guard matters there for a genuinely non-exempt source; the stricter PROTECTED semantics belong to the workflow. Both cases are now pinned, including a positive control that a docs->docs rename stays exempt. Decisions-Edit: yesSecond BLOCKED verdict, also correct. My previous fix validated only the TOP-LEVEL type of each page, which left the same hole one level down: `[{}]` is a well-formed JSON array whose rows carry no `filename`, so it contributes zero paths, looks like a short final page, and completes the enumeration from a PARTIAL list — the exact failure the guard exists to prevent, just wearing a valid-looking wrapper. An array of scalars additionally tripped `set -e` during extraction without producing a decision. Both call sites now require every row to carry a non-empty string `filename`. An empty array remains valid, since that is what a real end-of-pagination is. Predicate checked against nine shapes before adopting: `[]` and well-formed rows valid; `[{}]`, `[{"filename":""}]`, `[{"filename":null}]`, `["x"]`, `[1,2]`, a bare error object, and a mixed good/bad array all rejected. The reviewer also caught that my `test_first_page_failure_*` was VACUOUS: with page 1 failing the path list is empty, and the hook independently withholds the exemption on an empty list, so it passed with the guard removed. It is kept as a smoke case with that limitation stated in its docstring, and three tests that actually pin the behaviour were added — malformed rows and scalar rows after a FULL page 1 (so the path list is non-empty and the exemption would really fire), plus MAX_PAGES exhaustion. Added a positive control too: an empty final page must still exempt. Without it, an over-strict guard would make every "withholds the exemption" test pass for the wrong reason — the suite would be asserting "never exempt", which is not the contract. Verified by mutation: top-level-only validation fails 2 tests, an over-strict guard fails the positive control, and widening MAX_PAGES fails the exhaustion test. Decisions-Edit: yesThird BLOCKED verdict, third legitimate finding. The per-row guard validated the DESTINATION only, so `{"filename":"docs/x.md","status":"renamed"}` with no `previous_filename` passed while its source silently vanished — precisely the path-hiding that collecting both sides exists to prevent. A rename row must now carry a non-empty `previous_filename`. Required for rename rows ONLY. Demanding it globally would reject every ordinary modified/added row and make the gate refuse all exemptions — which every "withholds the exemption" test would happily pass through, so that direction gets its own positive control. Predicate checked against ten shapes before adopting; mutation-verified in both directions (dropping the clause fails 2 tests, applying it globally fails 3 including the controls). Also fixes a wart this PR introduced. `review-verdict/h10` is itself folded into the COMBINED status, so a PR awaiting its verdict reports combined 'pending' and the hook's condition (a) reported it as a CI problem — sending a reader to build logs when the missing thing is the review, and exiting before the H10 branch that would have said so. The message now names the outstanding contexts, and says plainly when the verdict is the only one left. Scope boundary, stated in the record: real Gitea populates `previous_filename` on renames and returns well-formed pages. Everything past this point defends against shapes with no evidence of existing, so the guard's claim stays "any page we cannot fully classify withholds the exemption" rather than growing to cover unobserved responses. Decisions-Edit: yesReview-verdict: MERGEABLE @
9f5317bFifth cross-family pass, scoped to the condition-(a) diff: no new defects. Confirms the
skipped-is-green carve-out is safe (a genuinely failed upstream job still surfaces its own failure/error context), that both jq filters share one predicate sovonlycan never disagree withnongreen, that the single-fetch refactor still routes an unreadable response toask, and that the change is strictly message-only with no allow/grant path made more permissive.Prior rounds (all BLOCKED, all fixed): paging failed OPEN on a mid-pagination error; top-level-only validation missed
[{}]rows; rename rows validated only the destination; the non-green filter missed Gitea'serrorstate and mis-worded a negative verdict. Plus the corrected #619 narrative and thestatuses/{sha}truncation raised by the parallel session.Posted with scripts/post-review-verdict.sh — its own first live use.