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
@@ -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 @ <sha>",
|
||||
# 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 `@<hex>` 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 "@ <sha>" 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 "@ <sha>" 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 @<sha> -> 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 <<VERDICTS
|
||||
$verdicts
|
||||
VERDICTS
|
||||
|
||||
# A negative verdict on head wins over a positive one (a later BLOCKED retracts an earlier MERGEABLE
|
||||
# on the SAME head; and if the head were fixed the sha would change, so this can't wrongly block).
|
||||
if [ "$head_neg" = 1 ]; then
|
||||
decide deny "H10 merge gate: BLOCKED — a review verdict for the current head ($short) is negative (BLOCKED/NOT-MERGEABLE). Resolve the findings and post a fresh 'Review-verdict: MERGEABLE @ $short' before merging PR #$pr."
|
||||
fi
|
||||
if [ "$head_pos" = 1 ]; then
|
||||
case "$class" in
|
||||
negative)
|
||||
# A negative verdict on head wins over a positive one (a later BLOCKED retracts an earlier
|
||||
# MERGEABLE on the SAME head; if the head were fixed the sha would change, so this can't
|
||||
# wrongly block).
|
||||
decide deny "H10 merge gate: BLOCKED — a review verdict for the current head ($short) is negative (BLOCKED/NOT-MERGEABLE). Resolve the findings and post a fresh 'Review-verdict: MERGEABLE @ $short' before merging PR #$pr." ;;
|
||||
stale)
|
||||
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'." ;;
|
||||
unknown)
|
||||
decide ask "H10 merge gate: a 'Review-verdict:' comment on PR #$pr uses an unrecognized verdict token (not MERGEABLE/APPROVED/LGTM/BLOCKED/NOT-MERGEABLE). It is deliberately NOT read as approval. Post a verdict using the documented vocabulary — e.g. 'Review-verdict: MERGEABLE @ $short'." ;;
|
||||
no-sha)
|
||||
# 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 in its own '@ <sha>' 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."
|
||||
|
||||
@@ -110,7 +110,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 `:<version>` 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: <MERGEABLE\|APPROVED\|BLOCKED\|NOT-MERGEABLE> @ <head-sha>` 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: <MERGEABLE\|APPROVED\|BLOCKED\|NOT-MERGEABLE> @ <head-sha>` comment references the PR's current head sha (short-sha prefix match against the verdict's OWN `@ <sha>` 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 `"<n> 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) |
|
||||
|
||||
@@ -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: <MERGEABLE|APPROVED|BLOCKED|NOT-MERGEABLE> @ <head-sha>` 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: <MERGEABLE|APPROVED|BLOCKED|NOT-MERGEABLE> @ <head-sha>` comment references the PR''s current head sha (short-sha prefix match against the verdict''s OWN `@ <sha>` 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 `@ <sha>` 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 `@<hex>` *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** `@ <sha>` 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 `@ <sha>` 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 `@ <sha>`** 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 `@ <sha>`** 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**.
|
||||
|
||||
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'
|
||||
@@ -0,0 +1,216 @@
|
||||
"""H10 verdict-classification grammar (`scripts/check-review-verdict.sh`).
|
||||
|
||||
Before #629 this logic lived inline in `pretooluse-merge-consent.sh` with NO tests, and three
|
||||
false-opens survived in it — each making an unreviewed head read as approved. Every
|
||||
`test_falseopen_*` below fails against the pre-#629 grammar; that is what makes them regression
|
||||
tests rather than descriptions.
|
||||
|
||||
#629 reported a fourth item. It turned out not to be a defect — see
|
||||
`test_a_later_at_token_does_not_retarget_the_verdict`, kept as characterization.
|
||||
"""
|
||||
|
||||
import json
|
||||
import subprocess
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
SCRIPT = Path(__file__).resolve().parents[1] / "check-review-verdict.sh"
|
||||
|
||||
HEAD = "02c82b35eafd715380ddd6b8ba03cb63f0966a3f"
|
||||
SHORT = HEAD[:7]
|
||||
OTHER = "fba5233caf44f41bf6601352f1c1b9f46e829262"
|
||||
|
||||
|
||||
def classify(bodies, head=HEAD):
|
||||
"""Run the classifier over comment bodies; return (stdout_word, returncode)."""
|
||||
payload = json.dumps([{"body": b} for b in bodies])
|
||||
p = subprocess.run(["bash", str(SCRIPT), "--head", head], input=payload, capture_output=True, text=True)
|
||||
return p.stdout.strip(), p.returncode
|
||||
|
||||
|
||||
def verdict(word, sha):
|
||||
return f"Review-verdict: {word} @ {sha}"
|
||||
|
||||
|
||||
# --- the three #629 false-opens ---------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.parametrize("token", ["MERGEABLE-LATER", "APPROVED-PENDING-QA", "LGTMish", "MERGEABLEISH"])
|
||||
def test_falseopen_token_must_be_a_whole_word(token):
|
||||
"""#629(1): the old test prefix-matched, so any token STARTING with a positive word passed.
|
||||
|
||||
An unrecognized token is `unknown` — never positive, and never guessed into a block either.
|
||||
"""
|
||||
assert classify([verdict(token, HEAD)]) == ("unknown", 0)
|
||||
|
||||
|
||||
def test_falseopen_fenced_code_block_is_not_a_verdict():
|
||||
"""#629(2): inside a fence the marker IS at line start, so docs showing the convention counted."""
|
||||
body = f"Post it like this:\n\n```\n{verdict('MERGEABLE', HEAD)}\n```\n"
|
||||
assert classify([body]) == ("absent", 0)
|
||||
|
||||
|
||||
def test_falseopen_sha_may_not_come_from_a_url():
|
||||
"""#629(3): the old rule took the first `@<hex>` 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 `@ <head>` 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
|
||||
@@ -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 '@ <hex>' 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"
|
||||
|
||||
Reference in New Issue
Block a user