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
@@ -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 ;;
|
||||
|
||||
Reference in New Issue
Block a user