From 0f83b54334fc317cbe0fbd28404f58418b5f6dd0 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 25 Jul 2026 23:45:53 +0200 Subject: [PATCH 1/6] fix(629): close three false-opens in the H10 verdict grammar, and give it tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The H10 classification lived inline in `pretooluse-merge-consent.sh` with no tests. Three protections the `release.review-verdict-gate` record described were never actually implemented, and each graded an unreviewed head as approved. All three reproduced first: 1 MERGEABLE-LATER -> positive the token was prefix-matched, so any word STARTING with mergeable/approved/lgtm passed 2 fenced code block -> positive the line-start anchor is satisfied inside ```, so documentation showing the convention was a verdict 3 URL-borne sha -> positive the sha came from the first `@` ANYWHERE on the line, so a markdown link could supply it Fixes: whole-word token matching, with a token in neither vocabulary classified `unknown` (never positive, and not guessed into a block either — it goes to a human); fenced blocks stripped with fence state reset per comment body; the sha read from the verdict's OWN `@ ` field, which also makes multi-`@` lines unambiguous. The grammar moves to `scripts/check-review-verdict.sh` so it can be tested at all — 38 tests, and each fix mutation-verified: restoring the old regex/extraction makes exactly the corresponding test fail, control green. #629's fourth reported item is NOT a defect and is not claimed as a fix. A later `@ ` on a BLOCKED line was reported as "masking a negative"; under the documented grammar that line is a verdict for the sha in its own field, so `stale` is correct — and was correct before this change too. Kept as a characterization test. `test_post_review_verdict.py`'s cross-check re-implemented the hook's regexes in Python and asserted the shell still contained them. That mirror is removed: it is the same duplication that let these three survive, and a Python copy would keep passing while the shell drifted. It now runs the real classifier. The decision record is corrected — it asserted the URL protection this commit actually adds. Note: the active corpus is 5637 lines against a 5600 budget, so the validator emits its consolidation warning (non-blocking). That is #620's subject, not regressed here. fixes #629 Co-Authored-By: Claude Opus 5 (1M context) Decisions-Edit: yes --- .claude/hooks/pretooluse-merge-consent.sh | 81 +++---- docs/decisions/README.md | 2 +- .../records/release/review-verdict-gate.md | 39 +++- scripts/check-review-verdict.sh | 119 ++++++++++ scripts/tests/test_check_review_verdict.py | 216 ++++++++++++++++++ scripts/tests/test_post_review_verdict.py | 72 +++--- 6 files changed, 445 insertions(+), 84 deletions(-) create mode 100755 scripts/check-review-verdict.sh create mode 100644 scripts/tests/test_check_review_verdict.py diff --git a/.claude/hooks/pretooluse-merge-consent.sh b/.claude/hooks/pretooluse-merge-consent.sh index cba4cd552..b0c03c9ac 100755 --- a/.claude/hooks/pretooluse-merge-consent.sh +++ b/.claude/hooks/pretooluse-merge-consent.sh @@ -233,44 +233,42 @@ comments=$(gq "repos/$owner/$repo/issues/$pr/comments?limit=100") if [ -z "$comments" ]; then decide ask "H10 merge gate: could not fetch PR #$pr comments to verify a head-referencing review verdict ($short). Confirm the adversarial/Codex review covered the latest commit before merging." fi -# Verdict lines across all comment bodies: a real verdict line STARTS with the marker (after optional -# leading whitespace). Anchoring to line-start is deliberate — it rejects a comment that merely QUOTES -# the positive template mid-sentence (an instruction "please post: Review-verdict: MERGEABLE @ ", -# or the gate's own suggestion text echoed back), which would otherwise self-approve the merge. -verdicts=$(printf '%s' "$comments" | jq -r '.[].body // empty' 2>/dev/null | grep -iE '^[[:space:]]*review-verdict:' || true) -if [ -z "$verdicts" ]; then - decide ask "H10 merge gate: no 'Review-verdict:' comment found on PR #$pr referencing head $short. Post the adversarial/Codex verdict (e.g. 'Review-verdict: MERGEABLE @ $short'), or confirm the review covered the latest commit and approve." +# Classification is delegated to `scripts/check-review-verdict.sh` — the single source of truth for +# the H10 grammar, extracted in #629 so it could be TESTED. While it lived here it had none, and three +# false-opens survived in it: a prefix-matched token (`MERGEABLE-LATER` graded positive), a verdict +# inside a fenced code block (documentation showing the convention counted as a real verdict), and a +# sha taken from the first `@` anywhere on the line (a markdown link could supply it). Every +# decision the classifier makes is documented there; this file only maps a class onto a hook decision. +verdict_script="${CLAUDE_PROJECT_DIR:-.}/scripts/check-review-verdict.sh" +if [ ! -x "$verdict_script" ]; then + decide ask "H10 merge gate: verdict classifier not found at $verdict_script, so the review state can't be derived. Confirm the review covered the latest commit before merging." +fi +# An input error (exit 2) is NOT a classification — fall through to a human rather than guessing. +if ! class=$(printf '%s' "$comments" | "$verdict_script" --head "$sha" 2>/dev/null); then + decide ask "H10 merge gate: could not classify the review verdicts on PR #$pr (malformed comments payload or unreadable head). Confirm the review covered the latest commit ($short) before merging." fi -# Classify each verdict line by the sha it references (its "@ " field) and its verdict word. -# A line references the CURRENT head iff head BEGINS WITH that sha token AND the token is >=7 chars -# (git short-sha prefix semantics) — NOT a loose substring test: an older sha that merely contains -# the head prefix, or the head prefix appearing in an unrelated URL on the line, must NOT count -# (adversarial false-opens). The verdict token must sit right after the marker on the same line. -head_pos=0; head_neg=0; stale=0 -while IFS= read -r line; do - [ -n "$line" ] || continue - # The sha the line references: the hex token in its "@ " field (>=7 chars), lowercased. - ref=$(printf '%s' "$line" | grep -ioE '@[[:space:]]*[0-9a-f]{7,40}' | head -1 \ - | grep -oiE '[0-9a-f]{7,40}' | tr 'A-F' 'a-f' || true) - is_pos=0 - # Positive iff the line's OWN leading verdict word (right after the line-start marker) is positive — - # anchored so a second, later `review-verdict: mergeable` substring on a BLOCKED line can't flip it. - if printf '%s' "$line" | grep -iqE '^[[:space:]]*review-verdict:[[:space:]]*(mergeable|approved|lgtm)'; then is_pos=1; fi - [ -z "$ref" ] && continue # marker present but no @ -> falls through to the final ask - case "$sha" in - "$ref"*) if [ "$is_pos" = 1 ]; then head_pos=1; else head_neg=1; fi ;; - *) stale=1 ;; - esac -done < ask (don't mislabel as a stale older-commit review). + decide ask "H10 merge gate: a 'Review-verdict:' comment on PR #$pr references no commit sha in its own '@ ' field. Post one referencing the current head ($short) — e.g. 'Review-verdict: MERGEABLE @ $short' — or confirm the review covered the latest commit and approve." ;; + absent) + decide ask "H10 merge gate: no 'Review-verdict:' comment found on PR #$pr referencing head $short. Post the adversarial/Codex verdict (e.g. 'Review-verdict: MERGEABLE @ $short'), or confirm the review covered the latest commit and approve." ;; + positive) : ;; + *) + decide ask "H10 merge gate: unrecognized verdict classification '$class' for PR #$pr. Confirm the review covered the latest commit ($short) before merging." ;; +esac + +if [ "$class" = "positive" ]; then # (a) CI + (b) all Done-when ticked + (c) positive verdict @ current head -> SATISFIED. Auto-grant. # The reason string must not claim more than was actually checked: on the merge_when_checks_succeed # path this hook never read the CI status at all (it is delegated to Gitea), so saying "CI green" @@ -280,11 +278,8 @@ if [ "$head_pos" = 1 ]; then fi decide grant "H6/H10 merge gate: satisfied — CI green, all Done-when boxes ticked, and a positive Review-verdict references the current head ($short). Auto-granted (no separate confirmation needed)." fi -if [ "$stale" = 1 ]; then - decide deny "H10 merge gate: BLOCKED — a review-verdict comment references an older commit, not the current head ($short). The latest commit(s) are unreviewed (ersatztv#242: re-review the fix commit, not just the initial diff). Re-review the head and post 'Review-verdict: MERGEABLE @ $short'." -fi -# Marker(s) exist but reference no sha at all -> ask (don't mislabel as a stale older-commit review). -decide ask "H10 merge gate: a 'Review-verdict:' comment on PR #$pr references no commit sha. Post one referencing the current head ($short) — e.g. 'Review-verdict: MERGEABLE @ $short' — or confirm the review covered the latest commit and approve." -# All derivable and satisfied -> auto-grant (defensive: the head_pos branch above already exits here). -decide grant "H6/H10 merge gate: satisfied — auto-granted." +# Unreachable: the `case` above exits on every class, and `positive` exits in the block above. Kept as +# a fail-safe so a future class added to the classifier without a branch here cannot fall off the end +# of the script (which would exit 0 = silent passthrough, the one outcome a gate must never produce). +decide ask "H10 merge gate: verdict classification for PR #$pr produced no decision. Confirm the review covered the latest commit ($short) before merging." diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 9d77bfabb..53f19f96f 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -111,7 +111,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `release.migration-rehearsal-prodcopy` | Before promoting a migration-bearing release, rehearse the new image's migrations against a throwaway copy of the latest prod backup (`scripts/migration-smoke.sh`), gating PASS on the migrator's completion log line rather than HTTP readiness alone. | 2026-07-12 | [link](records/release/migration-rehearsal-prodcopy.md) | | `release.prepush-clean-worktree-guard` | A fail-open pre-push hook blocks a push when any file in the branch's diff vs `origin/main` also has uncommitted working-tree or index changes, since a stale-index commit (e.g. `git reset --soft` + `git add` over an edited-but-unstaged fix) can silently push, CI-test, and get reviewed a different tree than the one on disk. Scope is precise to pushed-diff files; escape hatch `ETV_ALLOW_DIRTY_PUSH=1`. | 2026-07-17 | [link](records/release/prepush-clean-worktree-guard.md) | | `release.promotion-floating-prod` | Prod tracks the floating `:prod` image reference; a tag build's immutable `:` image is scanned first, then promotion happens via a separate manual `DeployStack`, with daily auto-update only as a fallback — tag with enough runway before 03:00 to avoid an unscanned promotion. | 2026-07-13 | [link](records/release/promotion-floating-prod.md) | -| `release.review-verdict-gate` | A PR may not merge until a `Review-verdict: @ ` comment references the PR's current head sha (short-sha prefix match, line-start marker only, negative wins over positive on the same head); folds into the H6 merge-consent hook as condition (c). | 2026-07-12 | [link](records/release/review-verdict-gate.md) | +| `release.review-verdict-gate` | A PR may not merge until a `Review-verdict: @ ` comment references the PR's current head sha (short-sha prefix match against the verdict's OWN `@ ` field, line-start marker only, whole-word verdict token, fenced code blocks stripped, negative wins over positive on the same head); folds into the H6 merge-consent hook as condition (c). The grammar lives in ONE tested place, `scripts/check-review-verdict.sh` — #629 found three false-opens that survived because it was implemented inline and untested while this record described stricter behaviour than the code had. | 2026-07-12 | [link](records/release/review-verdict-gate.md) | | `release.verdict-status-check` | The H10 review verdict is written as a `review-verdict/h10` Gitea **commit status** on the exact reviewed sha by `scripts/post-review-verdict.sh`, and that context is a REQUIRED status check on `main`. Because a status belongs to one sha, a later commit cannot inherit it, so Gitea's own `merge_when_checks_succeed` refuses to merge a head no one reviewed. The PreToolUse hook additionally refuses to SCHEDULE an auto-merge unless that status is already green on head. A `pull_request` workflow auto-passes the two exempt classes (Renovate-authored, docs-only) unless the PR touches a protected path (`.claude/`, `.gitea/`, `.husky/`, `scripts/`, `docker/ci/`). This extends — does not supersede — `release.review-verdict-gate` (#303 H10), whose comment convention remains the human-readable artifact and the hook's condition (c). | 2026-07-25 | [link](records/release/verdict-status-check.md) | | `rulebuilder.relative-date-macros` | The visual rule builder's `inLast`/`notInLast` date operators compile to/parse from the pre-existing `CustomMultiFieldQueryParser` macros `released_inthelast`/`released_notinthelast` and `added_inthelast`/`added_notinthelast`, value form `" day\|week\|month\|year"`; there is no backend change. | 2026-07-23 | [link](records/rulebuilder/relative-date-macros.md) | | `scan.collections-scan-status` | `GET /api/v1/media-sources/collections-scan-status` reports a family-global (not per-source), boolean-only active-scan set read from `IEntityLocker`; the SPA reconciles authoritatively against it (with a grace-tick helper) instead of a fixed client-side timeout. | 2026-07-12 | [link](records/scan/collections-scan-status.md) | diff --git a/docs/decisions/records/release/review-verdict-gate.md b/docs/decisions/records/release/review-verdict-gate.md index e421c7591..fc4aea04d 100644 --- a/docs/decisions/records/release/review-verdict-gate.md +++ b/docs/decisions/records/release/review-verdict-gate.md @@ -5,9 +5,9 @@ status: active since: '2026-07-12' supersedes: none superseded-by: none -rule: 'A PR may not merge until a `Review-verdict: @ ` comment references the PR''s current head sha (short-sha prefix match, line-start marker only, negative wins over positive on the same head); folds into the H6 merge-consent hook as condition (c).' -signals: 'review-verdict, head-sha match, stale-review prevention · paths: `.claude/settings.json` · issues: #303 (H10), #242' -mechanics: '`pretooluse-merge-consent.sh`; CLAUDE.md → Task Completion Protocol (H10 convention)' +rule: 'A PR may not merge until a `Review-verdict: @ ` comment references the PR''s current head sha (short-sha prefix match against the verdict''s OWN `@ ` field, line-start marker only, whole-word verdict token, fenced code blocks stripped, negative wins over positive on the same head); folds into the H6 merge-consent hook as condition (c). The grammar lives in ONE tested place, `scripts/check-review-verdict.sh` — #629 found three false-opens that survived because it was implemented inline and untested while this record described stricter behaviour than the code had.' +signals: 'review-verdict, head-sha match, stale-review prevention, verdict false-open, MERGEABLE-LATER, fenced code block verdict, sha from a URL, unknown verdict token · paths: `scripts/check-review-verdict.sh`, `.claude/hooks/pretooluse-merge-consent.sh`, `.claude/settings.json` · issues: #303 (H10), #242, #629' +mechanics: '`scripts/check-review-verdict.sh` (the grammar, + `scripts/tests/test_check_review_verdict.py`); `pretooluse-merge-consent.sh` (maps a class onto allow/deny/ask); `scripts/post-review-verdict.sh`; CLAUDE.md → Task Completion Protocol (H10 convention)' --- **A PR may not merge until a `Review-verdict:` comment on it references the PR's CURRENT head sha** — @@ -26,7 +26,31 @@ MERGEABLE @ …", or the gate's own suggestion text echoed back) does **not** se (adversarial re-review false-open, folded pre-merge). It then classifies each verdict line by the sha in its `@ ` field, matched to the head by **git short-sha prefix semantics** (head *begins with* the token, token ≥7 chars) — NOT a loose substring test, so an older sha that merely contains the head -prefix, or the head prefix appearing in an unrelated URL on the line, does not count: +prefix does not count. + +**#629 — three of the protections described here were asserted but not implemented.** The grammar +lived inline in the hook with no tests, and each of these graded as a positive verdict until #629 +(every one reproduced, then fixed, then mutation-verified): + +- the verdict token was **prefix-matched**, so `MERGEABLE-LATER`, `APPROVED-PENDING-QA` and `LGTMish` + all read as positive. A token is now matched as a whole word, and one in neither vocabulary is + classified `unknown` — never positive, and never guessed into a block either; +- a verdict inside a **fenced code block** counted, because the line-start anchor is satisfied inside + a fence. So documentation showing the convention was itself a verdict. Fenced blocks are now + stripped, with fence state reset per comment body (blockquotes never needed handling — a `>` prefix + already fails the anchor); +- **"the head prefix appearing in an unrelated URL on the line does not count"** — this paragraph's + own earlier claim — was false. The implementation took the first `@` *anywhere* on the line, so + `Review-verdict: MERGEABLE [x](https://e/@0123456)` was graded against the link. The sha is now read + from the verdict's **own** `@ ` field, the one immediately following the token, which also + makes multi-`@` lines unambiguous. + +The lesson is about where a grammar lives, not about any one regex: this record described the intended +behaviour accurately, the code did something looser, and nothing compared them. The grammar now lives +in `scripts/check-review-verdict.sh` — one implementation, called by the hook and by +`post-review-verdict.sh`'s cross-check, covered by `scripts/tests/test_check_review_verdict.py`. + +The classifications: - a MERGEABLE/APPROVED/LGTM verdict whose `@ ` is the current head → **allow**; - a **negative** verdict (BLOCKED/NOT-MERGEABLE) *on the head* → **deny**, and it *wins over* a positive one on the same head (a later BLOCKED retracts an earlier MERGEABLE; to retract, re-review head and @@ -36,8 +60,11 @@ prefix, or the head prefix appearing in an unrelated URL on the line, does not c `MERGEABLE @ head` (the normal flow). So a genuine block must reference head, per the convention; - verdict comment(s) exist but reference only *older* commits → **deny** — the stale-review case #242 targets; -- a `Review-verdict:` marker with **no `@ `** at all → **ask** (a lazy/quoted marker; not - mislabelled as stale); +- a verdict line whose **token is in neither vocabulary** (`MERGEABLE-LATER`, `SHIP-IT`, …) → + **ask** (#629). Deliberately not read as approval, and deliberately not read as a block either — + an unrecognized token means the reviewer's intent is unknown, so it goes to a human; +- a `Review-verdict:` marker with **no `@ `** in its own field → **ask** (a lazy/quoted marker; + not mislabelled as stale); - no `Review-verdict:` comment at all → **ask** (graceful adoption, mirrors H6's "no Done-when → ask": surface, don't hard-block a PR that hasn't adopted the convention yet); - comments unfetchable / head sha unresolvable → **ask**. diff --git a/scripts/check-review-verdict.sh b/scripts/check-review-verdict.sh new file mode 100755 index 000000000..920cb8b54 --- /dev/null +++ b/scripts/check-review-verdict.sh @@ -0,0 +1,119 @@ +#!/usr/bin/env bash +# Classify a PR's `Review-verdict:` comments against its CURRENT head sha (ersatztv#303 H10, #629). +# +# Extracted from `.claude/hooks/pretooluse-merge-consent.sh` so the H10 grammar can be TESTED. While it +# lived inline it had no tests, and four false-opens survived in it — each of which made an unreviewed +# or explicitly-blocked head read as approved (#629). +# +# Usage: check-review-verdict.sh --head < comments.json +# stdin : the Gitea `issues/{index}/comments` JSON array (objects carrying `.body`). +# stdout : exactly one classification word (below). +# exit : 0 on a successful classification; 2 on a usage/input error (callers MUST fail closed — +# an unreadable input is never a pass). +# +# Classifications, in the order they are decided: +# negative a verdict on the CURRENT head is BLOCKED/NOT-MERGEABLE -> block +# positive a verdict on the CURRENT head is MERGEABLE/APPROVED/LGTM -> allow +# stale verdict(s) exist but reference only OLDER commits -> block (the #242 case) +# unknown a verdict line uses a token in neither vocabulary -> undecidable, surface it +# no-sha a `Review-verdict:` marker carries no `@ ` field -> undecidable +# absent no `Review-verdict:` marker anywhere -> undecidable (not adopted) +# +# THE GRAMMAR (deliberately strict — every relaxation here has been a false-open): +# +# ^[space]* review-verdict: [space]* [space]* @ [space]* <7-40 hex> +# +# - **Line-start marker.** Rejects a comment that quotes the template mid-sentence ("please post: +# Review-verdict: MERGEABLE @ …"), which would otherwise self-approve. +# - **Fenced code blocks are stripped first** (#629). The line-start anchor alone does not save us: +# inside a ``` block the marker IS at line start, so documentation showing the convention counted +# as a real verdict. Fence state is tracked PER COMMENT BODY so an unclosed fence in one comment +# cannot swallow or expose another. (Blockquotes need no special handling — a `>` prefix already +# fails the anchor.) +# - **The token must be a whole word** (#629). The old test prefix-matched, so `MERGEABLE-LATER`, +# `APPROVED-PENDING-QA` and `LGTMish` all graded as positive. A token in neither vocabulary is +# `unknown`, NOT positive and NOT a block — it is surfaced for a human rather than guessed at. +# - **The sha is read from the verdict's OWN `@ ` field** — the one immediately after the token — +# never "the first `@` anywhere on the line" (#629). That older rule let a markdown link supply +# the sha: `Review-verdict: MERGEABLE [x](https://e/@0123456)` graded against the URL. Anchoring the +# field also makes multi-`@` lines unambiguous, so a verdict naming one sha cannot be re-read as a +# verdict for another. +# - **Prefix, not substring**: head must BEGIN WITH the token, token >= 7 chars (git short-sha +# semantics), so an older sha that merely contains the head prefix does not match. +# +# "negative wins over positive on the same head" is deliberate: a later BLOCKED retracts an earlier +# MERGEABLE. Staleness is symmetric on purpose — a negative for an OLDER commit is stale exactly like +# a positive for one, and must not override a fresh head-positive, or a pre-fix `BLOCKED @ oldsha` +# would block forever even after the fix changes the sha and earns a fresh verdict. +set -uo pipefail + +head="" +while [ $# -gt 0 ]; do + case "$1" in + --head) head="${2:-}"; shift 2 ;; + *) printf 'check-review-verdict: unknown argument: %s\n' "$1" >&2; exit 2 ;; + esac +done +[ -n "$head" ] || { printf 'check-review-verdict: --head is required\n' >&2; exit 2; } + +head=$(printf '%s' "$head" | tr 'A-F' 'a-f') +case "$head" in + *[!0-9a-f]*|"") printf 'check-review-verdict: --head is not a hex sha: %s\n' "$head" >&2; exit 2 ;; +esac + +comments=$(cat) +[ -n "$comments" ] || { printf 'check-review-verdict: empty comments payload on stdin\n' >&2; exit 2; } + +# `jq -e` so malformed JSON is an input error (exit 2), never a silent "absent" — which would read as +# "convention not adopted" and downgrade a hard block into an ask. +# A sentinel line after each body lets the fence stripper reset state per comment. +SEP=$'\001BODY-BOUNDARY\001' +bodies=$(printf '%s' "$comments" | jq -re --arg sep "$SEP" '[.[] | (.body // ""), $sep] | flatten | join("\n")' 2>/dev/null) || { + printf 'check-review-verdict: stdin is not a JSON array of comment objects\n' >&2; exit 2; } + +# Strip fenced code blocks, resetting fence state at each comment boundary. +stripped=$(printf '%s\n' "$bodies" | awk -v sep="$SEP" ' + $0 == sep { fence = 0; next } + /^[[:space:]]*```/ { fence = !fence; next } + !fence { print } +') + +verdicts=$(printf '%s\n' "$stripped" | grep -iE '^[[:space:]]*review-verdict:' || true) +[ -n "$(printf '%s' "$verdicts" | tr -d '[:space:]')" ] || { printf 'absent\n'; exit 0; } + +# The verdict field, anchored: token then its own `@ `. Two greps rather than a capture group, +# because BSD/macOS grep has no -P and `sed -E` backreference portability is worse than this. +FIELD_RE='^[[:space:]]*review-verdict:[[:space:]]*[A-Za-z][A-Za-z-]*[[:space:]]*@[[:space:]]*[0-9a-fA-F]{7,40}' +POS_RE='^[[:space:]]*review-verdict:[[:space:]]*(mergeable|approved|lgtm)([[:space:]@]|$)' +NEG_RE='^[[:space:]]*review-verdict:[[:space:]]*(blocked|not-mergeable)([[:space:]@]|$)' + +head_pos=0; head_neg=0; stale=0; unknown=0 +while IFS= read -r line; do + [ -n "$line" ] || continue + + is_pos=0; is_neg=0 + printf '%s' "$line" | grep -iqE "$POS_RE" && is_pos=1 + printf '%s' "$line" | grep -iqE "$NEG_RE" && is_neg=1 + if [ "$is_pos" = 0 ] && [ "$is_neg" = 0 ]; then + unknown=1 # a verdict-shaped line whose token is in neither vocabulary — never guess + continue + fi + + # The sha from THIS line's own field. Take the anchored field, then the trailing hex of it. + field=$(printf '%s' "$line" | grep -ioE "$FIELD_RE" | head -1 || true) + ref=$(printf '%s' "$field" | grep -oiE '[0-9a-fA-F]{7,40}$' | tr 'A-F' 'a-f' || true) + [ -n "$ref" ] || continue # token recognized but no `@ ` field -> falls through to `no-sha` + + case "$head" in + "$ref"*) if [ "$is_pos" = 1 ]; then head_pos=1; else head_neg=1; fi ;; + *) stale=1 ;; + esac +done <` ANYWHERE on the line, so a link supplied it.""" + line = f"Review-verdict: MERGEABLE [x](https://e.invalid/@{HEAD[:12]})" + assert classify([line]) == ("no-sha", 0) + + +def test_a_later_at_token_does_not_retarget_the_verdict(): + """CHARACTERIZATION, not a regression test — #629's fourth reported item was NOT a false-open. + + The report framed a later `@ ` on a BLOCKED line as "masking a negative". It isn't: under + the documented grammar the line is a verdict for `deadbeef1234`, so `stale` is correct, and it was + correct before #629 too (the old first-`@`-anywhere rule picked the same token here). No false-open + exists — the line never grants on head either way. + + Kept because it pins the grammar the anchored field now guarantees: the sha is the verdict's OWN + field, and trailing `@` tokens cannot re-target it. + """ + line = f"Review-verdict: BLOCKED @ deadbeef1234 correction @ {HEAD}" + assert classify([line]) == ("stale", 0) + + +def test_fence_state_does_not_leak_between_comments(): + """An unclosed fence in one comment must not swallow a real verdict in the next.""" + assert classify(["Example:\n```\nnot a verdict", verdict("MERGEABLE", HEAD)]) == ("positive", 0) + + +# --- the happy paths ------------------------------------------------------------------------- + + +@pytest.mark.parametrize("word", ["MERGEABLE", "APPROVED", "LGTM", "mergeable", "Approved"]) +def test_positive_verdict_on_head(word): + assert classify([verdict(word, HEAD)]) == ("positive", 0) + + +@pytest.mark.parametrize("word", ["BLOCKED", "NOT-MERGEABLE", "blocked", "not-mergeable"]) +def test_negative_verdict_on_head(word): + assert classify([verdict(word, HEAD)]) == ("negative", 0) + + +def test_short_sha_prefix_is_accepted(): + assert classify([verdict("MERGEABLE", SHORT)]) == ("positive", 0) + + +def test_leading_indent_is_tolerated(): + assert classify([f" {verdict('MERGEABLE', HEAD)}"]) == ("positive", 0) + + +def test_verdict_among_ordinary_prose_in_the_same_comment(): + body = f"Reviewed the fix commit; findings resolved.\n\n{verdict('MERGEABLE', HEAD)}\n" + assert classify([body]) == ("positive", 0) + + +# --- precedence ------------------------------------------------------------------------------ + + +def test_negative_wins_over_positive_on_the_same_head(): + assert classify([verdict("MERGEABLE", HEAD), verdict("BLOCKED", HEAD)]) == ("negative", 0) + assert classify([verdict("BLOCKED", HEAD), verdict("MERGEABLE", HEAD)]) == ("negative", 0) + + +def test_staleness_is_symmetric_old_negative_does_not_block_a_fresh_positive(): + """A pre-fix `BLOCKED @ oldsha` must not block forever once the fix changes the sha.""" + assert classify([verdict("BLOCKED", OTHER), verdict("MERGEABLE", HEAD)]) == ("positive", 0) + + +def test_a_real_verdict_outranks_an_unknown_token(): + assert classify([verdict("MERGEABLE-LATER", HEAD), verdict("MERGEABLE", HEAD)]) == ( + "positive", + 0, + ) + + +# --- the stale-review case #242 targets ------------------------------------------------------ + + +def test_verdict_only_for_an_older_commit_is_stale(): + assert classify([verdict("MERGEABLE", OTHER)]) == ("stale", 0) + + +def test_old_sha_containing_the_head_prefix_does_not_match(): + contains_head_prefix = "abcdef" + SHORT + "1234567890abcdef1234567890abcdef" + assert classify([verdict("MERGEABLE", contains_head_prefix)]) == ("stale", 0) + + +def test_a_mid_string_substring_of_head_is_not_a_prefix_match(): + """The discriminating case for prefix-vs-substring matching. + + `interior` is a genuine substring of head but NOT a prefix, so `*"$ref"*` instead of `"$ref"*` + would wrongly match. The sibling test above cannot catch that: its fixture is LONGER than a + 40-char sha, so it fails a substring test for the wrong reason. + """ + interior = HEAD[5:15] + assert interior in HEAD and not HEAD.startswith(interior) + assert classify([verdict("MERGEABLE", interior)]) == ("stale", 0) + + +def test_trailing_url_does_not_change_a_valid_verdicts_target(): + line = f"Review-verdict: MERGEABLE @ {OTHER} (see http://ci.example/build/{HEAD})" + assert classify([line]) == ("stale", 0) + + +# --- false-open guards ----------------------------------------------------------------------- + + +def test_quoted_template_mid_sentence_is_not_a_verdict(): + assert classify([f"Please post: {verdict('MERGEABLE', HEAD)} when you are done"]) == ( + "absent", + 0, + ) + + +def test_blockquoted_verdict_is_not_a_verdict(): + assert classify([f"> {verdict('MERGEABLE', HEAD)}"]) == ("absent", 0) + + +def test_trailing_mergeable_substring_cannot_flip_a_blocked_line(): + line = f"Review-verdict: BLOCKED @ {HEAD} — do not post review-verdict: mergeable until fixed" + assert classify([line]) == ("negative", 0) + + +def test_marker_without_a_sha_is_undecidable(): + assert classify(["Review-verdict: MERGEABLE"]) == ("no-sha", 0) + + +def test_too_short_a_sha_is_not_a_reference(): + assert classify([verdict("MERGEABLE", HEAD[:6])]) == ("no-sha", 0) + + +def test_no_marker_at_all_is_absent(): + assert classify(["LGTM, nice work", "ship it"]) == ("absent", 0) + + +def test_empty_comment_list_is_absent(): + assert classify([]) == ("absent", 0) + + +# --- fail-closed on bad input ---------------------------------------------------------------- + + +def test_malformed_json_is_an_input_error_not_absent(): + """Must NOT degrade to `absent` — that reads as "not adopted" and downgrades a block to an ask.""" + p = subprocess.run(["bash", str(SCRIPT), "--head", HEAD], input="{not json", capture_output=True, text=True) + assert p.returncode == 2, p.stdout + + +def test_empty_stdin_is_an_input_error(): + p = subprocess.run(["bash", str(SCRIPT), "--head", HEAD], input="", capture_output=True, text=True) + assert p.returncode == 2 + + +def test_missing_head_argument_is_an_input_error(): + p = subprocess.run(["bash", str(SCRIPT)], input="[]", capture_output=True, text=True) + assert p.returncode == 2 + + +def test_non_hex_head_is_an_input_error(): + p = subprocess.run( + ["bash", str(SCRIPT), "--head", "refs/heads/main"], + input="[]", + capture_output=True, + text=True, + ) + assert p.returncode == 2 diff --git a/scripts/tests/test_post_review_verdict.py b/scripts/tests/test_post_review_verdict.py index 92d588dde..5d4f96100 100644 --- a/scripts/tests/test_post_review_verdict.py +++ b/scripts/tests/test_post_review_verdict.py @@ -9,17 +9,21 @@ after posting the comment and must NOT write a success status if a commit arrive Without that, a status written for the parent would be presented as covering the child — which is ersatztv#622 itself, just at a smaller time scale. -`test_comment_line_matches_the_hook_parser` is a cross-check rather than a unit test: it runs the -comment body this script produces through the *same* regexes `.claude/hooks/pretooluse-merge-consent.sh` -uses for condition (c). The two are separate implementations of one convention, and a drift between -them would be invisible until a merge mysteriously stalled. +`test_comment_matches_the_hook_parser` is a cross-check rather than a unit test: it runs the comment +body this script produces through the *actual* H10 classifier the hook uses for condition (c), +`scripts/check-review-verdict.sh`. A drift between the two would be invisible until a merge +mysteriously stalled. + +It originally re-implemented the hook's regexes in Python and asserted the shell source still +contained them. #629 removed that mirror: three false-opens had survived precisely because the +grammar existed in two places, and a Python copy would have kept passing while the shell drifted. +The grammar now lives in one tested script, so this calls it. """ from __future__ import annotations import json import os -import re import subprocess from pathlib import Path @@ -28,6 +32,7 @@ import pytest REPO_ROOT = Path(__file__).resolve().parents[2] SCRIPT = REPO_ROOT / "scripts" / "post-review-verdict.sh" HOOK = REPO_ROOT / ".claude" / "hooks" / "pretooluse-merge-consent.sh" +CLASSIFIER = REPO_ROOT / "scripts" / "check-review-verdict.sh" SHA_A = "fba5233c1111111111111111111111111111aaaa" SHA_B = "52786a542222222222222222222222222222bbbb" @@ -208,47 +213,46 @@ def test_unreachable_pr_is_an_error_not_a_silent_success(gitea): # --- Cross-checks against the hook's own condition-(c) parser ----------------------------------- -def _hook_regexes(): - """Pull the marker and positive-verdict regexes out of the hook, so drift shows up here.""" - source = HOOK.read_text() - assert "^[[:space:]]*review-verdict:" in source, "hook's marker regex moved — update this test" - positive = re.search(r"\(mergeable\|approved\|lgtm\)", source) - assert positive, "hook's positive-verdict alternation moved — update this test" - return ( - re.compile(r"^[ \t]*review-verdict:", re.IGNORECASE), - re.compile(r"^[ \t]*review-verdict:[ \t]*(mergeable|approved|lgtm)", re.IGNORECASE), +def _classify(body: str, head: str) -> str: + """Run the REAL H10 classifier over a comment body — no Python mirror of the grammar. + + This used to scrape the hook's regexes and re-implement them here (#622). Since #629 the grammar + lives in one tested place, `scripts/check-review-verdict.sh`, and the hook calls it — so the + cross-check can execute the actual thing. That matters: a Python copy of a shell regex is exactly + the duplication that let three false-opens survive in the first place, and it would have kept + passing here while the shell drifted. + """ + payload = json.dumps([{"body": body}]) + p = subprocess.run( + ["bash", str(CLASSIFIER), "--head", head], input=payload, capture_output=True, text=True ) + assert p.returncode == 0, f"classifier errored: {p.stderr}" + return p.stdout.strip() -def test_comment_line_matches_the_hook_parser(gitea): - marker, positive = _hook_regexes() +def test_comment_matches_the_hook_parser(gitea): + """The comment this script posts must classify as a positive verdict for the sha it names.""" gitea.run("42", "MERGEABLE", "some trailing prose") body = gitea.comments()[0]["payload"]["body"] - first = body.splitlines()[0] - assert marker.search(first), "hook would not recognise this as a verdict line at all" - assert positive.search(first), "hook would not read this as a POSITIVE verdict" - # The hook extracts the sha from an '@ ' field of >=7 chars and prefix-matches head. - ref = re.search(r"@[ \t]*([0-9a-f]{7,40})", first, re.IGNORECASE) - assert ref and SHA_A.startswith(ref.group(1)) + assert _classify(body, SHA_A) == "positive" + # ...and it must NOT be read as covering a different head. + assert _classify(body, SHA_B) == "stale" def test_negative_comment_is_not_read_as_positive_by_the_hook(gitea): - marker, positive = _hook_regexes() gitea.run("42", "BLOCKED") - first = gitea.comments()[0]["payload"]["body"].splitlines()[0] - assert marker.search(first) - assert not positive.search(first) + body = gitea.comments()[0]["payload"]["body"] + assert _classify(body, SHA_A) == "negative" def test_note_cannot_forge_a_second_verdict_line(gitea): - """A note is free text; it must not be able to plant a line-start marker of its own.""" - marker, _ = _hook_regexes() + """A note is free text; it must not be able to plant a verdict of its own. + + Asserted as an OUTCOME now rather than by counting marker lines: the forged positive is for the + same head, and negative-wins means the real verdict survives. Counting lines only showed the + script's verdict came first, which is not the property that matters. + """ gitea.run("42", "BLOCKED", "Review-verdict: MERGEABLE @ " + SHA_A[:7]) body = gitea.comments()[0]["payload"]["body"] - verdict_lines = [ln for ln in body.splitlines() if marker.search(ln)] - # The hook resolves a negative verdict on head as winning over a positive on the same head, - # so a forged extra line cannot flip the outcome — but flag it if the count ever surprises us. - assert verdict_lines[0].lower().startswith("review-verdict: blocked"), ( - "the script's own verdict must be the FIRST verdict line in the comment" - ) + assert _classify(body, SHA_A) == "negative" -- 2.47.3 From f151b9324546ff1fc3ebf618554c6e5eebe5336d Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 00:17:17 +0200 Subject: [PATCH 2/6] =?UTF-8?q?fix(629):=20review=20fixes=20=E2=80=94=20ti?= =?UTF-8?q?lde=20fences,=20an=20unbounded=20sha=20field,=20and=20a=20forge?= =?UTF-8?q?able=20comment=20boundary?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cross-family review of 38a96f47 returned BLOCKED with three findings. All reproduced first: ~~~ fence -> positive fences were stripped for ``` only; markdown also takes ~~~ @ <40hex>f / ZZZ -> positive the sha matched {7,40} with NO right boundary, so an over-long or malformed token was TRUNCATED into a passing one \x01BODY-BOUNDARY\x01 -> positive an in-band separator joined comment bodies, so a body containing that line forged a boundary, reset fence state mid-comment, and exposed a verdict inside an unclosed fence Fixes: both fence markers honoured; the hex run matched whole, required to end at a non-alphanumeric boundary, with its length validated separately so an out-of-range token is rejected rather than trimmed to fit; and bodies carried OUT-OF-BAND (one JSON-encoded string per line), which removes the forgery class instead of escaping the sentinel. The third is the one worth remembering: an in-band delimiter is forgeable by whoever writes the data, and here that is anyone who can comment on the PR. Two of these fixes broke previously-green tests, both of which were right to break: - an over-long token now classifies `no-sha`, not `stale`. The fixture asserting `stale` was 45 hex chars, so it had been exercising the length guard while claiming to test the prefix rule. Rebuilt as a well-formed 40-char sha that contains the head prefix without starting with it. - `jq -e` exits 4 when a filter produces NO output, which is the legitimate empty-comment-list case. Treating it as an error turned "no comments yet" into an input error — and callers fail closed on those, so a new PR would have read as unclassifiable. Exit 4 is now accepted. 44 classifier tests, 155 total. `~~~` and the boundary fixes are each mutation-verified; the out-of-band fix has no equivalent mutation (it is structural, not a regex) so its evidence is the direct reproduction against the previous commit. refs #629 Co-Authored-By: Claude Opus 5 (1M context) Decisions-Edit: yes --- .../records/release/review-verdict-gate.md | 20 +++++++ scripts/check-review-verdict.sh | 60 ++++++++++++++----- scripts/tests/test_check_review_verdict.py | 53 +++++++++++++++- 3 files changed, 116 insertions(+), 17 deletions(-) diff --git a/docs/decisions/records/release/review-verdict-gate.md b/docs/decisions/records/release/review-verdict-gate.md index fc4aea04d..1d302fba5 100644 --- a/docs/decisions/records/release/review-verdict-gate.md +++ b/docs/decisions/records/release/review-verdict-gate.md @@ -45,11 +45,31 @@ lived inline in the hook with no tests, and each of these graded as a positive v from the verdict's **own** `@ ` field, the one immediately following the token, which also makes multi-`@` lines unambiguous. +A cross-family review of that first fix found **three more**, all reproduced before fixing — worth +recording because each is the same fix done half-way: + +- fences were stripped for ` ``` ` only, but markdown also accepts **`~~~`**; +- the sha field matched `{7,40}` with **no right boundary**, so an over-long or malformed token was + silently *truncated* into a passing one: `@ <40-hex-head>f` and `@ <40-hex-head>ZZZ` both graded as + a verdict for head. The hex run is now matched whole, must end at a non-alphanumeric boundary, and + its length is validated separately — an out-of-range token is rejected, never trimmed to fit; +- comment bodies were joined with a literal `\x01BODY-BOUNDARY\x01` line so fence state could reset + per comment. **An in-band delimiter is forgeable by whoever writes the data** — and here that is + anyone who can comment on the PR: a body containing that line reset the fence mid-comment and + exposed a verdict still inside an unclosed fence. Bodies are now carried out-of-band (one + JSON-encoded string per line), which removes the class rather than escaping the sentinel. + The lesson is about where a grammar lives, not about any one regex: this record described the intended behaviour accurately, the code did something looser, and nothing compared them. The grammar now lives in `scripts/check-review-verdict.sh` — one implementation, called by the hook and by `post-review-verdict.sh`'s cross-check, covered by `scripts/tests/test_check_review_verdict.py`. +Two of those fixes also broke previously-green tests, which is the useful part: an over-long token +became `no-sha` where a fixture expected `stale` (the fixture was 45 hex chars, so it had been +testing the length guard while claiming to test the prefix rule), and `jq -e` exits **4** on no +output, so an empty comment list started reading as a malformed payload — turning "no comments yet" +into an input error on a gate whose callers fail closed. + The classifications: - a MERGEABLE/APPROVED/LGTM verdict whose `@ ` is the current head → **allow**; - a **negative** verdict (BLOCKED/NOT-MERGEABLE) *on the head* → **deny**, and it *wins over* a positive diff --git a/scripts/check-review-verdict.sh b/scripts/check-review-verdict.sh index 920cb8b54..4cca325de 100755 --- a/scripts/check-review-verdict.sh +++ b/scripts/check-review-verdict.sh @@ -66,24 +66,45 @@ comments=$(cat) # `jq -e` so malformed JSON is an input error (exit 2), never a silent "absent" — which would read as # "convention not adopted" and downgrade a hard block into an ask. -# A sentinel line after each body lets the fence stripper reset state per comment. -SEP=$'\001BODY-BOUNDARY\001' -bodies=$(printf '%s' "$comments" | jq -re --arg sep "$SEP" '[.[] | (.body // ""), $sep] | flatten | join("\n")' 2>/dev/null) || { - printf 'check-review-verdict: stdin is not a JSON array of comment objects\n' >&2; exit 2; } +# +# Each body is emitted as a JSON STRING on its own line (newlines escaped by JSON), so comment +# boundaries are carried out-of-band. An earlier version joined bodies with a literal sentinel line; +# a comment containing that sentinel could forge a boundary, reset fence state mid-body, and expose a +# verdict that was still inside an unclosed fence. In-band delimiters are forgeable by whoever writes +# the data — and here that is anyone who can comment on the PR. +encoded=$(printf '%s' "$comments" | jq -ce '.[] | (.body // "")' 2>/dev/null) +jq_rc=$? +# `jq -e` exits 4 when a filter produced NO output — which is exactly the legitimate empty-comment-list +# case, not a malformed payload. Treating it as an error turned "no comments yet" into an input error, +# and callers fail closed on those, so an unremarkable new PR would have read as unclassifiable. +if [ "$jq_rc" -ne 0 ] && [ "$jq_rc" -ne 4 ]; then + printf 'check-review-verdict: stdin is not a JSON array of comment objects\n' >&2; exit 2 +fi -# Strip fenced code blocks, resetting fence state at each comment boundary. -stripped=$(printf '%s\n' "$bodies" | awk -v sep="$SEP" ' - $0 == sep { fence = 0; next } - /^[[:space:]]*```/ { fence = !fence; next } - !fence { print } -') - -verdicts=$(printf '%s\n' "$stripped" | grep -iE '^[[:space:]]*review-verdict:' || true) +# Strip fenced code blocks per body, so fence state cannot leak between comments. Both fence markers +# markdown accepts are honoured: ``` and ~~~ (a verdict inside a `~~~` block was still counted). +verdicts="" +while IFS= read -r encoded_body; do + [ -n "$encoded_body" ] || continue + body=$(printf '%s' "$encoded_body" | jq -r '.' 2>/dev/null) || continue + found=$(printf '%s\n' "$body" | awk ' + /^[[:space:]]*(```|~~~)/ { fence = !fence; next } + !fence { print } + ' | grep -iE '^[[:space:]]*review-verdict:' || true) + [ -n "$found" ] && verdicts="${verdicts}${found} +" +done <`. Two greps rather than a capture group, # because BSD/macOS grep has no -P and `sed -E` backreference portability is worse than this. -FIELD_RE='^[[:space:]]*review-verdict:[[:space:]]*[A-Za-z][A-Za-z-]*[[:space:]]*@[[:space:]]*[0-9a-fA-F]{7,40}' +# The hex run is matched WHOLE (`+`) and must end at a non-alphanumeric boundary or end-of-line, then +# its length is checked separately. Matching `{7,40}` directly had no right boundary, so an over-long +# or malformed token was silently TRUNCATED into a valid-looking one: `@ <40-hex-head>` and +# `@ <40-hex-head>ZZZ` both matched their first 40 chars and graded as a verdict for head. +FIELD_RE='^[[:space:]]*review-verdict:[[:space:]]*[A-Za-z][A-Za-z-]*[[:space:]]*@[[:space:]]*[0-9a-fA-F]+([^0-9a-zA-Z]|$)' POS_RE='^[[:space:]]*review-verdict:[[:space:]]*(mergeable|approved|lgtm)([[:space:]@]|$)' NEG_RE='^[[:space:]]*review-verdict:[[:space:]]*(blocked|not-mergeable)([[:space:]@]|$)' @@ -99,10 +120,17 @@ while IFS= read -r line; do continue fi - # The sha from THIS line's own field. Take the anchored field, then the trailing hex of it. + # The sha from THIS line's own field. Take the anchored field, then its LAST hex run — the sha sits + # after the token, and the field ends at the boundary, so the last run is always the candidate. field=$(printf '%s' "$line" | grep -ioE "$FIELD_RE" | head -1 || true) - ref=$(printf '%s' "$field" | grep -oiE '[0-9a-fA-F]{7,40}$' | tr 'A-F' 'a-f' || true) - [ -n "$ref" ] || continue # token recognized but no `@ ` field -> falls through to `no-sha` + ref=$(printf '%s' "$field" | grep -oE '[0-9a-fA-F]+' | tail -1 | tr 'A-F' 'a-f' || true) + # Length is validated HERE rather than in the regex, so an out-of-range token is rejected outright + # instead of being truncated to a passing prefix. + case "${#ref}" in + 7|8|9|1[0-9]|2[0-9]|3[0-9]|40) : ;; + *) ref="" ;; + esac + [ -n "$ref" ] || continue # token recognized but no valid `@ ` field -> falls through to `no-sha` case "$head" in "$ref"*) if [ "$is_pos" = 1 ]; then head_pos=1; else head_neg=1; fi ;; diff --git a/scripts/tests/test_check_review_verdict.py b/scripts/tests/test_check_review_verdict.py index 9ada0990f..e7557ed35 100644 --- a/scripts/tests/test_check_review_verdict.py +++ b/scripts/tests/test_check_review_verdict.py @@ -77,6 +77,49 @@ def test_fence_state_does_not_leak_between_comments(): assert classify(["Example:\n```\nnot a verdict", verdict("MERGEABLE", HEAD)]) == ("positive", 0) +# --- found by cross-family review of the first fix (all three reproduced before fixing) ---------- + + +def test_falseopen_tilde_fence_is_also_stripped(): + """Markdown accepts `~~~` as well as ```; stripping only backticks left the hole half-open.""" + body = f"~~~\n{verdict('MERGEABLE', HEAD)}\n~~~" + assert classify([body]) == ("absent", 0) + + +@pytest.mark.parametrize( + "suffix", + [ + "f", # a 41st hex char -> over-long, must not truncate to a valid 40 + "9999", # a longer wrong sha + "ZZZ", # a non-hex suffix welded to a valid sha + ], +) +def test_falseopen_sha_field_needs_a_right_boundary(suffix): + """Matching `{7,40}` with no right boundary silently TRUNCATED a malformed token into a match. + + `@ <40-hex-head>` matched its first 40 characters and graded as a verdict for head. + """ + assert classify([verdict("MERGEABLE", HEAD + suffix)]) == ("no-sha", 0) + + +def test_falseopen_a_body_cannot_forge_a_comment_boundary(): + """The separator between comments must be out-of-band. + + An earlier version joined bodies with a literal `\\x01BODY-BOUNDARY\\x01` line. A comment + containing that line could reset fence state mid-body and expose a verdict still inside an + unclosed fence — an in-band delimiter is forgeable by whoever writes the data, and here that is + anyone who can comment on the PR. + """ + sep = "\x01BODY-BOUNDARY\x01" + body = f"```\n{sep}\n{verdict('MERGEABLE', HEAD)}" + assert classify([body]) == ("absent", 0) + + +def test_a_valid_verdict_may_carry_trailing_prose(): + """The right boundary must not reject the ordinary `@ (note)` shape.""" + assert classify([f"{verdict('MERGEABLE', HEAD)} (all findings resolved)"]) == ("positive", 0) + + # --- the happy paths ------------------------------------------------------------------------- @@ -131,7 +174,15 @@ def test_verdict_only_for_an_older_commit_is_stale(): def test_old_sha_containing_the_head_prefix_does_not_match(): - contains_head_prefix = "abcdef" + SHORT + "1234567890abcdef1234567890abcdef" + """A VALID 40-char sha that contains the head prefix but does not start with it is stale. + + The fixture is deliberately a well-formed sha: an over-long hex run is now rejected as malformed + (`no-sha`) rather than compared, so a 45-char fixture would have tested the length guard instead + of the prefix rule it is named for. + """ + contains_head_prefix = ("abcdef" + SHORT + "0" * 40)[:40] + assert len(contains_head_prefix) == 40 + assert SHORT in contains_head_prefix and not contains_head_prefix.startswith(SHORT) assert classify([verdict("MERGEABLE", contains_head_prefix)]) == ("stale", 0) -- 2.47.3 From 3bfd925baff8bf86bf24a4d4d439a2fd0ceee78d Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 00:31:10 +0200 Subject: [PATCH 3/6] =?UTF-8?q?fix(629):=20re-review=20fixes=20=E2=80=94?= =?UTF-8?q?=20the=20READ=20path=20must=20fail=20closed=20too?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review of 8d0df553 returned BLOCKED. Both findings are the same class as the bugs this issue is about, one level down: the reader, not the grammar. 1. A here-document makes bash materialise a temp file. When that fails (read-only or restricted TMPDIR) the loop reads NOTHING, and the classifier returned `absent` — silently discarding a real BLOCKED verdict. Reproduced with TMPDIR=/nonexistent: `absent` before, `negative` now. Both loops are off here-docs (a pipe for the reader, process substitution for the classifier so its flags stay in the current shell). "The environment could not supply a temp file" is not evidence that a PR was approved. 2. A malformed payload degraded to `absent` instead of an input error: an object-valued `.body` reached the scanner and simply matched nothing. Shape is now asserted in jq — must be an array, bodies must be strings — so it exits 2 and callers fail closed. Also closed a pre-existing false-open the review found while probing (present before #629, so not a regression, but cheap to fix here): bash strips NULs in command substitution, so a body of `Review-verdict: MERGEABLE @ ` arrived at the matcher as a valid verdict line — text that is not a verdict becoming one in transit. Bodies containing NUL are now rejected. 161 tests. Verified: 80k body still classifies; empty array, null body still `absent`; malformed JSON, non-array, array-of-non-objects, object body, NUL body all exit 2; a hostile TMPDIR still returns `negative`. refs #629 Co-Authored-By: Claude Opus 5 (1M context) --- scripts/check-review-verdict.sh | 48 +++++++++++++------- scripts/tests/test_check_review_verdict.py | 51 ++++++++++++++++++++++ 2 files changed, 84 insertions(+), 15 deletions(-) diff --git a/scripts/check-review-verdict.sh b/scripts/check-review-verdict.sh index 4cca325de..690109297 100755 --- a/scripts/check-review-verdict.sh +++ b/scripts/check-review-verdict.sh @@ -72,30 +72,50 @@ comments=$(cat) # a comment containing that sentinel could forge a boundary, reset fence state mid-body, and expose a # verdict that was still inside an unclosed fence. In-band delimiters are forgeable by whoever writes # the data — and here that is anyone who can comment on the PR. -encoded=$(printf '%s' "$comments" | jq -ce '.[] | (.body // "")' 2>/dev/null) +# The shape is asserted IN jq so a payload that isn't an array of comment objects with STRING bodies +# is an input error (exit 2), not a silent `absent`. An object-valued `.body` used to reach the +# scanner and simply match nothing — a malformed payload reading as "no verdict posted" is a +# fail-OPEN on a gate whose whole job is to withhold approval. +# A body containing a NUL is rejected outright: bash strips NULs in command substitution, so +# `Review-verdict: MERGEABLE @ ` would arrive at the matcher as a valid verdict line — +# text that is not a verdict silently becoming one. +encoded=$(printf '%s' "$comments" | jq -ce ' + if type != "array" then error("not an array") else .[] end + | (.body // "") + | if type != "string" then error("non-string body") + elif contains("\u0000") then error("NUL in body") + else . end' 2>/dev/null) jq_rc=$? # `jq -e` exits 4 when a filter produced NO output — which is exactly the legitimate empty-comment-list # case, not a malformed payload. Treating it as an error turned "no comments yet" into an input error, # and callers fail closed on those, so an unremarkable new PR would have read as unclassifiable. if [ "$jq_rc" -ne 0 ] && [ "$jq_rc" -ne 4 ]; then - printf 'check-review-verdict: stdin is not a JSON array of comment objects\n' >&2; exit 2 + printf 'check-review-verdict: stdin is not a JSON array of comment objects with string bodies\n' >&2 + exit 2 fi # Strip fenced code blocks per body, so fence state cannot leak between comments. Both fence markers # markdown accepts are honoured: ``` and ~~~ (a verdict inside a `~~~` block was still counted). -verdicts="" -while IFS= read -r encoded_body; do +# +# Fed by a PIPE, not a here-document. A here-doc makes bash materialise a temp file, and when that +# fails (read-only or restricted TMPDIR) the loop silently reads nothing — the classifier returns +# `absent` and a real BLOCKED verdict disappears. Gate failures must never land on the permissive +# side, and "the environment could not supply a temp file" is not evidence that a PR was approved. +# The loop runs in a subshell, so its result is captured through stdout rather than a variable. +verdicts=$(printf '%s\n' "$encoded" | while IFS= read -r encoded_body; do [ -n "$encoded_body" ] || continue - body=$(printf '%s' "$encoded_body" | jq -r '.' 2>/dev/null) || continue - found=$(printf '%s\n' "$body" | awk ' + # A body that fails to decode is fatal, not skippable — skipping one could drop the only BLOCKED + # verdict on the PR. 3 is distinct from the exit-2 paths above so the caller sees where it broke. + body=$(printf '%s' "$encoded_body" | jq -r '.' 2>/dev/null) || exit 3 + printf '%s\n' "$body" | awk ' /^[[:space:]]*(```|~~~)/ { fence = !fence; next } !fence { print } - ' | grep -iE '^[[:space:]]*review-verdict:' || true) - [ -n "$found" ] && verdicts="${verdicts}${found} -" -done <&2; exit 2 +fi [ -n "$(printf '%s' "$verdicts" | tr -d '[:space:]')" ] || { printf 'absent\n'; exit 0; } # The verdict field, anchored: token then its own `@ `. Two greps rather than a capture group, @@ -136,9 +156,7 @@ while IFS= read -r line; do "$ref"*) if [ "$is_pos" = 1 ]; then head_pos=1; else head_neg=1; fi ;; *) stale=1 ;; esac -done <-verdict:` would arrive as a verdict. + + Text that is not a verdict must not become one on the way through the reader. + """ + payload = json.dumps([{"body": "Review" + chr(0) + "-verdict: MERGEABLE @ " + HEAD}]) + p = subprocess.run(["bash", str(SCRIPT), "--head", HEAD], input=payload, capture_output=True, text=True) + assert p.returncode == 2, f"got rc={p.returncode} out={p.stdout!r}" + + def test_non_hex_head_is_an_input_error(): p = subprocess.run( ["bash", str(SCRIPT), "--head", "refs/heads/main"], -- 2.47.3 From 299e7b27afc7c8604b161c60a9f61e023c920b2b Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 00:42:57 +0200 Subject: [PATCH 4/6] =?UTF-8?q?fix(629):=20round-3=20fixes=20=E2=80=94=20f?= =?UTF-8?q?ence=20LENGTH=20semantics,=20and=20stop=20masking=20reader=20fa?= =?UTF-8?q?ilures?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third review round, third set of real findings. Both reproduced before fixing. 1. High: markdown closes a fence only with N-or-more of the SAME marker it was opened with, so a ```` block legitimately CONTAINS a ``` line as content. Toggling on any 3+ marker left the fence at that inner line and graded the verdict below it as a real approval: ````text / ``` / Review-verdict: MERGEABLE @ / ```` -> positive Now tracks the opening marker's character and length; a shorter or different marker while a fence is open is content, so it neither closes the fence nor escapes it. 2. Medium: `awk ... | grep ... || true` flattened "no match" (grep rc 1, normal) together with a real tool failure (rc >= 2). A failing reader produced no verdict lines at all — `absent` — silently discarding a real BLOCKED verdict. awk and grep are now checked separately, and only "no match" is tolerated. Also fixed while writing (2): `[ rc = 0 ] && printf` as the loop body's LAST command would leave the subshell exiting 1 whenever the newest comment carried no verdict, which the rc check would then report as a failure to read comment bodies — an ordinary PR reading as broken. Uses an `if`. 169 tests. Both findings mutation-verified: restoring the naive fence toggle fails all four longer-fence cases, restoring `|| true` fails the failing-grep case, control green. Verified the ordinary shapes still work: plain ``` and ~~~ fences and lang-tagged fences still stripped, an unclosed fence still swallows, a real verdict beside a fenced example still counts, and a fenced positive alongside a real BLOCKED still classifies negative. refs #629 Co-Authored-By: Claude Opus 5 (1M context) Decisions-Edit: yes --- .../records/release/review-verdict-gate.md | 3 + scripts/check-review-verdict.sh | 28 +++++++-- scripts/tests/test_check_review_verdict.py | 59 +++++++++++++++++++ 3 files changed, 86 insertions(+), 4 deletions(-) diff --git a/docs/decisions/records/release/review-verdict-gate.md b/docs/decisions/records/release/review-verdict-gate.md index 1d302fba5..223db2528 100644 --- a/docs/decisions/records/release/review-verdict-gate.md +++ b/docs/decisions/records/release/review-verdict-gate.md @@ -53,6 +53,9 @@ recording because each is the same fix done half-way: silently *truncated* into a passing one: `@ <40-hex-head>f` and `@ <40-hex-head>ZZZ` both graded as a verdict for head. The hex run is now matched whole, must end at a non-alphanumeric boundary, and its length is validated separately — an out-of-range token is rejected, never trimmed to fit; +- fence stripping toggled on **any** line of three-or-more markers. Markdown closes a fence only + with N-or-more of the *same* marker it was opened with, so a ```` block legitimately contains a + ``` line — the stripper left the fence early and graded the verdict below it as real; - comment bodies were joined with a literal `\x01BODY-BOUNDARY\x01` line so fence state could reset per comment. **An in-band delimiter is forgeable by whoever writes the data** — and here that is anyone who can comment on the PR: a body containing that line reset the fence mid-comment and diff --git a/scripts/check-review-verdict.sh b/scripts/check-review-verdict.sh index 690109297..6cf2e97e6 100755 --- a/scripts/check-review-verdict.sh +++ b/scripts/check-review-verdict.sh @@ -107,10 +107,30 @@ verdicts=$(printf '%s\n' "$encoded" | while IFS= read -r encoded_body; do # A body that fails to decode is fatal, not skippable — skipping one could drop the only BLOCKED # verdict on the PR. 3 is distinct from the exit-2 paths above so the caller sees where it broke. body=$(printf '%s' "$encoded_body" | jq -r '.' 2>/dev/null) || exit 3 - printf '%s\n' "$body" | awk ' - /^[[:space:]]*(```|~~~)/ { fence = !fence; next } - !fence { print } - ' | grep -iE '^[[:space:]]*review-verdict:' || true + # Fence tracking follows markdown: a fence opened with N markers is closed only by N-or-more of the + # SAME character. A naive "any line starting with 3 toggles" exits a ```` block at the first ``` + # LINE INSIDE IT — which is legitimate content — and the verdict below it then counts as real. + # A shorter or different marker while a fence is open is content, so it neither closes nor prints. + outside=$(printf '%s\n' "$body" | awk ' + { + if (match($0, /^[[:space:]]*(`{3,}|~{3,})/)) { + m = substr($0, RSTART, RLENGTH); gsub(/[[:space:]]/, "", m) + ch = substr(m, 1, 1); len = length(m) + if (!fence) { fence = 1; fch = ch; flen = len; next } + else if (ch == fch && len >= flen) { fence = 0; next } + } + if (!fence) print + }') || exit 4 + # `grep` exits 1 for "no match" (normal) and >=2 for a real error. `|| true` flattened both into + # success, so a failing reader silently produced no verdicts — `absent` — and dropped a real + # BLOCKED verdict. Only "no match" may be tolerated. + found=$(printf '%s\n' "$outside" | grep -iE '^[[:space:]]*review-verdict:') + grep_rc=$? + [ "$grep_rc" -le 1 ] || exit 5 + # An `if`, not `[ ... ] && printf`: the latter is the loop body's last command, so a final comment + # with no verdict would leave the SUBSHELL exiting 1 and the rc check below would report a reader + # failure on a perfectly ordinary PR. + if [ "$grep_rc" -eq 0 ]; then printf '%s\n' "$found"; fi done) verdict_rc=$? if [ "$verdict_rc" -ne 0 ]; then diff --git a/scripts/tests/test_check_review_verdict.py b/scripts/tests/test_check_review_verdict.py index 8cc68435e..b75da0e93 100644 --- a/scripts/tests/test_check_review_verdict.py +++ b/scripts/tests/test_check_review_verdict.py @@ -121,6 +121,65 @@ def test_a_valid_verdict_may_carry_trailing_prose(): assert classify([f"{verdict('MERGEABLE', HEAD)} (all findings resolved)"]) == ("positive", 0) +# --- found by a third review round: fence LENGTH, and masked reader failures --------------------- + + +@pytest.mark.parametrize( + "body", + [ + "````text\n```\n{v}\n````\n", # a 4-fence legitimately contains a ``` line + "~~~~text\n~~~\n{v}\n~~~~\n", # same for tildes + "- item\n ````text\n ```\n {v}\n ````\n", # indented inside a list + "`````\n```\n{v}\n`````\n", # 5 markers + ], +) +def test_falseopen_a_longer_fence_may_contain_a_shorter_marker(body): + """Markdown closes a fence only with N-or-more of the SAME marker it was opened with. + + Toggling on any 3+ marker exited a ```` block at the first ``` line inside it — which is + ordinary content — and graded the verdict below it as real. + """ + assert classify([body.format(v=verdict("MERGEABLE", HEAD))]) == ("absent", 0) + + +@pytest.mark.parametrize("tool", ["awk", "grep"]) +def test_a_failing_reader_tool_is_an_error_not_absent(tool, tmp_path): + """`grep` exits 1 for "no match" and >=2 for a real error; `|| true` flattened both. + + A failing reader then produced no verdict lines at all — `absent` — silently discarding a real + BLOCKED verdict. Only "no match" may be tolerated. + """ + shim = tmp_path / tool + shim.write_text("#!/bin/sh\nexit 91\n") + shim.chmod(0o755) + env = {**os.environ, "PATH": f"{tmp_path}{os.pathsep}{os.environ['PATH']}"} + payload = json.dumps([{"body": verdict("BLOCKED", HEAD)}]) + p = subprocess.run( + ["bash", str(SCRIPT), "--head", HEAD], + input=payload, + capture_output=True, + text=True, + env=env, + ) + assert p.returncode != 0, f"failed {tool} produced rc=0 out={p.stdout!r}" + assert p.stdout.strip() != "absent" + + +def test_a_final_comment_without_a_verdict_does_not_look_like_a_reader_failure(): + """Guards the rc plumbing: the loop's last command must not leak "no match" as a subshell error. + + Written as `[ rc = 0 ] && printf`, an ordinary PR whose newest comment carries no verdict would + have exited the subshell 1 and been reported as a failure to read comment bodies. + """ + assert classify([verdict("MERGEABLE", HEAD), "thanks!"]) == ("positive", 0) + + +def test_a_fenced_example_alongside_a_real_blocked_verdict_still_blocks(): + """The stripper must not eat a genuine verdict in a different comment.""" + fenced = f"```\n{verdict('MERGEABLE', HEAD)}\n```" + assert classify([fenced, verdict("BLOCKED", HEAD)]) == ("negative", 0) + + # --- the happy paths ------------------------------------------------------------------------- -- 2.47.3 From 28a9d0dbfd3bc3c3d1aed39881c38dc34d048c73 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 00:52:11 +0200 Subject: [PATCH 5/6] =?UTF-8?q?fix(629):=20round-4=20=E2=80=94=20require?= =?UTF-8?q?=20the=20marker=20at=20COLUMN=200,=20narrowing=20the=20grammar?= =?UTF-8?q?=20instead=20of=20patching=20again?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 4 found the last false-open: markdown has a SECOND code-block form the fence stripper does not cover — indented blocks (4 spaces or a tab). A pasted indented example still self-approved: Example: Review-verdict: MERGEABLE @ -> positive Adding an indented-code stripper would be the same move that produced rounds 2, 3 and 4: fixing the instance, not the class. So the grammar is narrowed instead — the marker must sit at COLUMN 0. That kills every indentation-based ambiguity at once (4-space, tab, list-nested, arbitrary indent). Cost, accepted deliberately: a verdict indented under a list item is now ignored and classifies `absent`, which asks a human. For a gate, erring toward ignoring is the safe direction. Fence detection KEEPS its leading-whitespace tolerance, because stripping more is always safe. `test_leading_indent_is_tolerated` asserted the old behaviour and is replaced by `test_falseopen_an_indented_verdict_is_not_a_verdict`, parameterised over four indent shapes and mutation-verified: restoring `^[[:space:]]*` fails all four, control green. 172 tests. Round 4 verified clean by execution: the rc plumbing fails closed for a forced failure in the inner jq, awk, grep AND the pipeline producer (rc 3/4/5/93 -> exit 2); fence-length semantics, mismatched markers, CRLF fences, 10-marker fences, blockquote fences; emoji, CRLF, a 120k line, 200 comments; NUL rejection with no JSON-encoding bypass. refs #629 Co-Authored-By: Claude Opus 5 (1M context) Decisions-Edit: yes --- docs/decisions/README.md | 2 +- .../records/release/review-verdict-gate.md | 17 ++++++++++++++- scripts/check-review-verdict.sh | 19 ++++++++++------- scripts/tests/test_check_review_verdict.py | 21 +++++++++++++++++-- 4 files changed, 48 insertions(+), 11 deletions(-) diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 53f19f96f..d5a2834c4 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -111,7 +111,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `release.migration-rehearsal-prodcopy` | Before promoting a migration-bearing release, rehearse the new image's migrations against a throwaway copy of the latest prod backup (`scripts/migration-smoke.sh`), gating PASS on the migrator's completion log line rather than HTTP readiness alone. | 2026-07-12 | [link](records/release/migration-rehearsal-prodcopy.md) | | `release.prepush-clean-worktree-guard` | A fail-open pre-push hook blocks a push when any file in the branch's diff vs `origin/main` also has uncommitted working-tree or index changes, since a stale-index commit (e.g. `git reset --soft` + `git add` over an edited-but-unstaged fix) can silently push, CI-test, and get reviewed a different tree than the one on disk. Scope is precise to pushed-diff files; escape hatch `ETV_ALLOW_DIRTY_PUSH=1`. | 2026-07-17 | [link](records/release/prepush-clean-worktree-guard.md) | | `release.promotion-floating-prod` | Prod tracks the floating `:prod` image reference; a tag build's immutable `:` image is scanned first, then promotion happens via a separate manual `DeployStack`, with daily auto-update only as a fallback — tag with enough runway before 03:00 to avoid an unscanned promotion. | 2026-07-13 | [link](records/release/promotion-floating-prod.md) | -| `release.review-verdict-gate` | A PR may not merge until a `Review-verdict: @ ` comment references the PR's current head sha (short-sha prefix match against the verdict's OWN `@ ` field, line-start marker only, whole-word verdict token, fenced code blocks stripped, negative wins over positive on the same head); folds into the H6 merge-consent hook as condition (c). The grammar lives in ONE tested place, `scripts/check-review-verdict.sh` — #629 found three false-opens that survived because it was implemented inline and untested while this record described stricter behaviour than the code had. | 2026-07-12 | [link](records/release/review-verdict-gate.md) | +| `release.review-verdict-gate` | A PR may not merge until a `Review-verdict: @ ` comment references the PR's current head sha (short-sha prefix match against the verdict's OWN `@ ` field, marker at COLUMN 0 (no indent, so indented code blocks cannot self-approve), whole-word verdict token, fenced code blocks stripped with markdown fence-length semantics, negative wins over positive on the same head); folds into the H6 merge-consent hook as condition (c). The grammar lives in ONE tested place, `scripts/check-review-verdict.sh` — #629 found three false-opens that survived because it was implemented inline and untested while this record described stricter behaviour than the code had. | 2026-07-12 | [link](records/release/review-verdict-gate.md) | | `release.verdict-status-check` | The H10 review verdict is written as a `review-verdict/h10` Gitea **commit status** on the exact reviewed sha by `scripts/post-review-verdict.sh`, and that context is a REQUIRED status check on `main`. Because a status belongs to one sha, a later commit cannot inherit it, so Gitea's own `merge_when_checks_succeed` refuses to merge a head no one reviewed. The PreToolUse hook additionally refuses to SCHEDULE an auto-merge unless that status is already green on head. A `pull_request` workflow auto-passes the two exempt classes (Renovate-authored, docs-only) unless the PR touches a protected path (`.claude/`, `.gitea/`, `.husky/`, `scripts/`, `docker/ci/`). This extends — does not supersede — `release.review-verdict-gate` (#303 H10), whose comment convention remains the human-readable artifact and the hook's condition (c). | 2026-07-25 | [link](records/release/verdict-status-check.md) | | `rulebuilder.relative-date-macros` | The visual rule builder's `inLast`/`notInLast` date operators compile to/parse from the pre-existing `CustomMultiFieldQueryParser` macros `released_inthelast`/`released_notinthelast` and `added_inthelast`/`added_notinthelast`, value form `" day\|week\|month\|year"`; there is no backend change. | 2026-07-23 | [link](records/rulebuilder/relative-date-macros.md) | | `scan.collections-scan-status` | `GET /api/v1/media-sources/collections-scan-status` reports a family-global (not per-source), boolean-only active-scan set read from `IEntityLocker`; the SPA reconciles authoritatively against it (with a grace-tick helper) instead of a fixed client-side timeout. | 2026-07-12 | [link](records/scan/collections-scan-status.md) | diff --git a/docs/decisions/records/release/review-verdict-gate.md b/docs/decisions/records/release/review-verdict-gate.md index 223db2528..36d0ef15f 100644 --- a/docs/decisions/records/release/review-verdict-gate.md +++ b/docs/decisions/records/release/review-verdict-gate.md @@ -5,7 +5,7 @@ status: active since: '2026-07-12' supersedes: none superseded-by: none -rule: 'A PR may not merge until a `Review-verdict: @ ` comment references the PR''s current head sha (short-sha prefix match against the verdict''s OWN `@ ` field, line-start marker only, whole-word verdict token, fenced code blocks stripped, negative wins over positive on the same head); folds into the H6 merge-consent hook as condition (c). The grammar lives in ONE tested place, `scripts/check-review-verdict.sh` — #629 found three false-opens that survived because it was implemented inline and untested while this record described stricter behaviour than the code had.' +rule: 'A PR may not merge until a `Review-verdict: @ ` comment references the PR''s current head sha (short-sha prefix match against the verdict''s OWN `@ ` field, marker at COLUMN 0 (no indent, so indented code blocks cannot self-approve), whole-word verdict token, fenced code blocks stripped with markdown fence-length semantics, negative wins over positive on the same head); folds into the H6 merge-consent hook as condition (c). The grammar lives in ONE tested place, `scripts/check-review-verdict.sh` — #629 found three false-opens that survived because it was implemented inline and untested while this record described stricter behaviour than the code had.' signals: 'review-verdict, head-sha match, stale-review prevention, verdict false-open, MERGEABLE-LATER, fenced code block verdict, sha from a URL, unknown verdict token · paths: `scripts/check-review-verdict.sh`, `.claude/hooks/pretooluse-merge-consent.sh`, `.claude/settings.json` · issues: #303 (H10), #242, #629' mechanics: '`scripts/check-review-verdict.sh` (the grammar, + `scripts/tests/test_check_review_verdict.py`); `pretooluse-merge-consent.sh` (maps a class onto allow/deny/ask); `scripts/post-review-verdict.sh`; CLAUDE.md → Task Completion Protocol (H10 convention)' --- @@ -62,6 +62,21 @@ recording because each is the same fix done half-way: exposed a verdict still inside an unclosed fence. Bodies are now carried out-of-band (one JSON-encoded string per line), which removes the class rather than escaping the sentinel. +A fourth round found the last one, and it is why the grammar was **narrowed** rather than patched +again: markdown has a second code-block form — *indented* blocks (4 spaces or a tab) — which the fence +stripper does not cover, so a pasted indented example still self-approved. Rather than add a second +stripper (the same instance-not-class move that produced the previous three rounds), **the marker must +now sit at COLUMN 0**. That removes every indentation-based ambiguity at once. The cost is that a +verdict indented under a list item is ignored and classifies `absent` — which asks a human, the safe +direction for a gate. Fence detection keeps its leading-whitespace tolerance, because stripping *more* +is always safe. + +Four review rounds, four sets of real findings, and rounds 2–4 each found a bug introduced while +fixing the round before — every one the same fix applied half-way (one fence marker but not the other; +a bounded left side but not the right; state reset per comment but via a delimiter the writer +controls). For attacker-writable text, budget several rounds and prefer eliminating a class over +enumerating its members. + The lesson is about where a grammar lives, not about any one regex: this record described the intended behaviour accurately, the code did something looser, and nothing compared them. The grammar now lives in `scripts/check-review-verdict.sh` — one implementation, called by the hook and by diff --git a/scripts/check-review-verdict.sh b/scripts/check-review-verdict.sh index 6cf2e97e6..79c3cf4e7 100755 --- a/scripts/check-review-verdict.sh +++ b/scripts/check-review-verdict.sh @@ -21,10 +21,15 @@ # # THE GRAMMAR (deliberately strict — every relaxation here has been a false-open): # -# ^[space]* review-verdict: [space]* [space]* @ [space]* <7-40 hex> +# ^review-verdict: [space]* [space]* @ [space]* <7-40 hex> (marker at COLUMN 0) # -# - **Line-start marker.** Rejects a comment that quotes the template mid-sentence ("please post: -# Review-verdict: MERGEABLE @ …"), which would otherwise self-approve. +# - **Marker at COLUMN 0** — no leading whitespace (#629). This rejects a mid-sentence quote +# ("please post: Review-verdict: MERGEABLE @ …"), and it also rejects markdown's *indented* code +# blocks (4 spaces or a tab) and anything nested in a list. Indented code is a second code-block +# form the fence stripper does not cover, and patching each form in turn is how three rounds of +# false-opens happened; requiring column 0 removes the ambiguity rather than enumerating it. +# The cost is that a verdict indented under a list item is ignored — it classifies `absent`, which +# asks a human. Erring toward ignoring is the safe direction for a gate. # - **Fenced code blocks are stripped first** (#629). The line-start anchor alone does not save us: # inside a ``` block the marker IS at line start, so documentation showing the convention counted # as a real verdict. Fence state is tracked PER COMMENT BODY so an unclosed fence in one comment @@ -124,7 +129,7 @@ verdicts=$(printf '%s\n' "$encoded" | while IFS= read -r encoded_body; do # `grep` exits 1 for "no match" (normal) and >=2 for a real error. `|| true` flattened both into # success, so a failing reader silently produced no verdicts — `absent` — and dropped a real # BLOCKED verdict. Only "no match" may be tolerated. - found=$(printf '%s\n' "$outside" | grep -iE '^[[:space:]]*review-verdict:') + found=$(printf '%s\n' "$outside" | grep -iE '^review-verdict:') grep_rc=$? [ "$grep_rc" -le 1 ] || exit 5 # An `if`, not `[ ... ] && printf`: the latter is the loop body's last command, so a final comment @@ -144,9 +149,9 @@ fi # its length is checked separately. Matching `{7,40}` directly had no right boundary, so an over-long # or malformed token was silently TRUNCATED into a valid-looking one: `@ <40-hex-head>` and # `@ <40-hex-head>ZZZ` both matched their first 40 chars and graded as a verdict for head. -FIELD_RE='^[[:space:]]*review-verdict:[[:space:]]*[A-Za-z][A-Za-z-]*[[:space:]]*@[[:space:]]*[0-9a-fA-F]+([^0-9a-zA-Z]|$)' -POS_RE='^[[:space:]]*review-verdict:[[:space:]]*(mergeable|approved|lgtm)([[:space:]@]|$)' -NEG_RE='^[[:space:]]*review-verdict:[[:space:]]*(blocked|not-mergeable)([[:space:]@]|$)' +FIELD_RE='^review-verdict:[[:space:]]*[A-Za-z][A-Za-z-]*[[:space:]]*@[[:space:]]*[0-9a-fA-F]+([^0-9a-zA-Z]|$)' +POS_RE='^review-verdict:[[:space:]]*(mergeable|approved|lgtm)([[:space:]@]|$)' +NEG_RE='^review-verdict:[[:space:]]*(blocked|not-mergeable)([[:space:]@]|$)' head_pos=0; head_neg=0; stale=0; unknown=0 while IFS= read -r line; do diff --git a/scripts/tests/test_check_review_verdict.py b/scripts/tests/test_check_review_verdict.py index b75da0e93..d8b90f77e 100644 --- a/scripts/tests/test_check_review_verdict.py +++ b/scripts/tests/test_check_review_verdict.py @@ -197,8 +197,25 @@ def test_short_sha_prefix_is_accepted(): assert classify([verdict("MERGEABLE", SHORT)]) == ("positive", 0) -def test_leading_indent_is_tolerated(): - assert classify([f" {verdict('MERGEABLE', HEAD)}"]) == ("positive", 0) +@pytest.mark.parametrize( + "prefix", + [ + " ", # markdown indented code block (4 spaces) + "\t", # indented code block (tab) + " ", # any indent at all + " ", # nested under a list item + ], +) +def test_falseopen_an_indented_verdict_is_not_a_verdict(prefix): + """Markdown's *indented* code block is a second code-block form the fence stripper misses. + + Allowing leading whitespace let a pasted, indented example self-approve. The marker must sit at + COLUMN 0. This is a deliberate tightening — a verdict indented under a list item now classifies + `absent` and asks a human, which is the safe direction — and it removes the whole indentation + ambiguity rather than adding a second stripper for each code-block form. + """ + body = f"Example:\n\n{prefix}{verdict('MERGEABLE', HEAD)}\n" + assert classify([body]) == ("absent", 0) def test_verdict_among_ordinary_prose_in_the_same_comment(): -- 2.47.3 From 50bcd7b0c7a0403e9d36896c66843faf6e55c9f4 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 01:14:59 +0200 Subject: [PATCH 6/6] fix(629): strip raw HTML blocks, and state where the hardening stops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 5: raw HTML is the third code-block form. `
`, `` and HTML comments all render their
contents literally, so a verdict inside one is an example, not an approval:

  
 /  /   containing a verdict  ->  positive

Now stripped, tracked as a marker count rather than parsed — the direction of error is to strip MORE,
which can only ever withhold approval. Mutation-verified: removing the stripper fails all five cases.

AND THE HARDENING STOPS HERE, deliberately. The record now says so, because otherwise the next
session re-derives it: this is a best-effort heuristic, not a markdown parser. It covers the three
code-block forms markdown has (fenced; indented, via the column-0 rule; raw HTML) and is not proof
against every way to render text as non-prose.

Stopping is safe because the comment is NOT the load-bearing gate. Since #622 the authoritative
signal is the `review-verdict/h10` commit status, written only by post-review-verdict.sh from
explicit arguments — a comment cannot forge it. This classifier is condition (c) of the PreToolUse
hook: defense in depth on an agent's merge call. A residual false-open means the hook does not
object; it does not mean a merge happens.

Five rounds found five code-block forms, four of them introduced while fixing the previous round.
The generalisable rule, now in the record: when a heuristic keeps failing at the edges, check whether
it is actually the thing enforcing the invariant before spending another round on it.

Also measured, against the real corpus: a "verdict must be the first line" rule would have killed
every code-block form at once, but 14 of 18 verdict markers ever posted in this repo are NOT on the
first line — so it was rejected as a retroactive break, not deferred.

178 tests. refs #629

Co-Authored-By: Claude Opus 5 (1M context) 

Decisions-Edit: yes
---
 .../records/release/review-verdict-gate.md    | 19 +++++++++++++
 scripts/check-review-verdict.sh               | 14 ++++++++++
 scripts/tests/test_check_review_verdict.py    | 27 +++++++++++++++++++
 3 files changed, 60 insertions(+)

diff --git a/docs/decisions/records/release/review-verdict-gate.md b/docs/decisions/records/release/review-verdict-gate.md
index 36d0ef15f..89c20d7f3 100644
--- a/docs/decisions/records/release/review-verdict-gate.md
+++ b/docs/decisions/records/release/review-verdict-gate.md
@@ -77,6 +77,25 @@ a bounded left side but not the right; state reset per comment but via a delimit
 controls). For attacker-writable text, budget several rounds and prefer eliminating a class over
 enumerating its members.
 
+A fifth round found raw HTML — `
`, ``, HTML comments — the third code-block form. It is
+stripped too, and **hardening stopped there, deliberately.**
+
+**Scope limit, stated so nobody re-derives it: the comment scanner is a best-effort heuristic, not a
+markdown parser.** It handles the three code-block forms markdown actually has (fenced, indented via
+the column-0 rule, raw HTML) and errs toward stripping more, because every error in that direction can
+only *withhold* approval. It is not proof against every conceivable way to render text as non-prose,
+and chasing that was demonstrably not converging: five review rounds, five code-block forms, four of
+them introduced while fixing the previous one.
+
+Stopping is safe because **the comment is not the load-bearing gate.** Since #622 the authoritative
+signal is the `review-verdict/h10` commit status, written only by `scripts/post-review-verdict.sh`
+from explicit arguments — **a comment cannot forge it**, whatever it contains. The comment classifier
+is condition (c) of the PreToolUse hook, i.e. defense in depth on an agent's merge call. A residual
+false-open there means the hook does not object; it does not mean a merge happens.
+
+The rule that generalises: *when a heuristic keeps failing at the edges, check whether it is actually
+the thing enforcing the invariant before spending another round on it.*
+
 The lesson is about where a grammar lives, not about any one regex: this record described the intended
 behaviour accurately, the code did something looser, and nothing compared them. The grammar now lives
 in `scripts/check-review-verdict.sh` — one implementation, called by the hook and by
diff --git a/scripts/check-review-verdict.sh b/scripts/check-review-verdict.sh
index 79c3cf4e7..1513e9a2d 100755
--- a/scripts/check-review-verdict.sh
+++ b/scripts/check-review-verdict.sh
@@ -118,6 +118,20 @@ verdicts=$(printf '%s\n' "$encoded" | while IFS= read -r encoded_body; do
   # A shorter or different marker while a fence is open is content, so it neither closes nor prints.
   outside=$(printf '%s\n' "$body" | awk '
     {
+      # Raw HTML blocks are the third code-block form (#629 round 5): 
,  and HTML
+      # comments all render their contents literally, so a verdict inside one is an example, not an
+      # approval. Tracked as a simple depth/marker count rather than parsed — the direction of error
+      # is to strip MORE, which can only ever withhold approval.
+      low = tolower($0)
+      if (low ~ //)                 { closed = 1 }
+        if (low ~ /<\/(pre|code)>/)      { closed = 1 }
+        if (closed) { html = 0 }
+        next
+      }
       if (match($0, /^[[:space:]]*(`{3,}|~{3,})/)) {
         m = substr($0, RSTART, RLENGTH); gsub(/[[:space:]]/, "", m)
         ch = substr(m, 1, 1); len = length(m)
diff --git a/scripts/tests/test_check_review_verdict.py b/scripts/tests/test_check_review_verdict.py
index d8b90f77e..5670c947e 100644
--- a/scripts/tests/test_check_review_verdict.py
+++ b/scripts/tests/test_check_review_verdict.py
@@ -180,6 +180,33 @@ def test_a_fenced_example_alongside_a_real_blocked_verdict_still_blocks():
     assert classify([fenced, verdict("BLOCKED", HEAD)]) == ("negative", 0)
 
 
+# --- found by a fifth review round: raw HTML is the third code-block form ------------------------
+
+
+@pytest.mark.parametrize(
+    "wrapper",
+    [
+        "
\n{v}\n
",
+        "\n{v}\n",
+        "",
+        '
\n{v}\n
',
+        "
\n{v}\n
",
+    ],
+)
+def test_falseopen_raw_html_blocks_are_not_verdicts(wrapper):
+    """`
`, `` and HTML comments render their contents literally.
+
+    A verdict inside one is an example, not an approval. Markdown's three code-block forms — fenced,
+    indented, and raw HTML — each had to be handled; indentation was closed by the column-0 rule.
+    """
+    assert classify([wrapper.format(v=verdict("MERGEABLE", HEAD))]) == ("absent", 0)
+
+
+def test_an_html_example_does_not_eat_a_real_verdict_in_another_comment():
+    html = f"
\n{verdict('MERGEABLE', HEAD)}\n
"
+    assert classify([html, verdict("BLOCKED", HEAD)]) == ("negative", 0)
+
+
 # --- the happy paths -------------------------------------------------------------------------
 
 
-- 
2.47.3