fix(629): review fixes — tilde fences, an unbounded sha field, and a forgeable comment boundary
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) <noreply@anthropic.com>
Decisions-Edit: yes
This commit is contained in:
Claude Opus 5
@@ -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** `@ <sha>` 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 `@ <sha>` is the current head → **allow**;
|
||||
- a **negative** verdict (BLOCKED/NOT-MERGEABLE) *on the head* → **deny**, and it *wins over* a positive
|
||||
|
||||
@@ -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 <<ENCODED
|
||||
$encoded
|
||||
ENCODED
|
||||
[ -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}'
|
||||
# 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><more hex>` 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 `@ <sha>` 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 `@ <sha>` field -> falls through to `no-sha`
|
||||
|
||||
case "$head" in
|
||||
"$ref"*) if [ "$is_pos" = 1 ]; then head_pos=1; else head_neg=1; fi ;;
|
||||
|
||||
@@ -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><anything>` 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 `@ <sha> (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)
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user