From 299e7b27afc7c8604b161c60a9f61e023c920b2b Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 00:42:57 +0200 Subject: [PATCH] =?UTF-8?q?fix(629):=20round-3=20fixes=20=E2=80=94=20fence?= =?UTF-8?q?=20LENGTH=20semantics,=20and=20stop=20masking=20reader=20failur?= =?UTF-8?q?es?= 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 -------------------------------------------------------------------------