fix(629): close three false-opens in the H10 verdict grammar, and give it tests
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 `@<hex>` 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
`@ <sha>` 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 `@ <head>`
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) <noreply@anthropic.com>
Decisions-Edit: yes
This commit is contained in:
Claude Opus 5
Executable
+119
@@ -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 <sha> < 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 `@ <sha>` 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]* <TOKEN> [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 `@ <sha>` field** — the one immediately after the token —
|
||||
# never "the first `@<hex>` 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 <sha> 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 `@ <sha>`. 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 `@ <sha>` 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 <<VERDICTS
|
||||
$verdicts
|
||||
VERDICTS
|
||||
|
||||
if [ "$head_neg" = 1 ]; then printf 'negative\n'; exit 0; fi
|
||||
if [ "$head_pos" = 1 ]; then printf 'positive\n'; exit 0; fi
|
||||
if [ "$stale" = 1 ]; then printf 'stale\n'; exit 0; fi
|
||||
if [ "$unknown" = 1 ]; then printf 'unknown\n'; exit 0; fi
|
||||
printf 'no-sha\n'
|
||||
Reference in New Issue
Block a user