From 7265fba36dc24f7c0af0df002ae707f1dbfa3927 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 12:17:46 +0200 Subject: [PATCH] =?UTF-8?q?fix(647):=20the=20H10=20verdict=20classifier=20?= =?UTF-8?q?was=20inert=20on=20jq=201.6=20=E2=80=94=20the=20runner's=20vers?= =?UTF-8?q?ion?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Turning on the scripts/tests suite in CI immediately paid for itself: measured on origin/main, 61 of 178 tests FAIL under jq 1.6, which is what the CI runner ships. They pass on a dev Mac's jq 1.8.2, which is why this was invisible — and the suite has never run anywhere else, which is exactly #631's thesis. Two defects in scripts/check-review-verdict.sh (from #629, the single source of truth for H10 verdict classification): 1. `contains("")` is TRUE FOR EVERY STRING on jq 1.6 — the escape truncates the literal to the empty string, and every string contains "". So the body guard errored "NUL in body" on every comment and the H10 grammar was entirely inert on the runner. Verified against both binaries: 1.6 says true for "hello", 1.7+ says false. Replaced with `(explode | index(0)) != null`, which involves no regex engine and agrees on both. 2. A parse error was indistinguishable from "no output". The script used jq's exit code to separate malformed input from a legitimately empty comment list, treating 4 as benign — but jq >= 1.7 exits 5 on a parse error while 1.6 exits 4, the same code both use for "filter produced no output". On 1.6 a garbage API response therefore returned `absent` instead of an input error. Fixed with an explicit `jq empty` pre-check, which is non-zero iff the input does not parse regardless of output volume. Severity: fail-closed, not exploitable. The classifier is only invoked from the merge-consent hook, which runs on the dev machine (jq 1.8.2), so the live gate is unaffected. The cost is that #629's hardening was inert on the runner and would have stayed invisible. 198 tests now pass under BOTH jq 1.8.2 and jq 1.6 (was 138/60 split under 1.6). This is the third distinct jq-1.6 divergence found in this codebase today (the first was #643's `jq -e` on empty input). The rule: a shell gate's behaviour is a function of its interpreter's version — test against the version CI actually runs, or pin it. Refs #647, #631 Co-Authored-By: Claude Opus 5 (1M context) --- scripts/check-review-verdict.sh | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/scripts/check-review-verdict.sh b/scripts/check-review-verdict.sh index 1513e9a2d..2b930afb1 100755 --- a/scripts/check-review-verdict.sh +++ b/scripts/check-review-verdict.sh @@ -82,13 +82,30 @@ comments=$(cat) # scanner and simply match nothing — a malformed payload reading as "no verdict posted" is a # fail-OPEN on a gate whose whole job is to withhold approval. # A body containing a NUL is rejected outright: bash strips NULs in command substitution, so +# NOTE the NUL test is `explode | index(0)`, NOT `contains("\u0000")` (ersatztv#647). On jq 1.6 the +# escape truncates the literal to the EMPTY string, and every string contains "" — so that form +# returns true for ALL input, making this guard reject every comment body as malformed. Verified +# against both binaries: 1.6 says true for "hello", 1.7+ says false. The CI runner ships jq 1.6, so +# the whole verdict classifier was inert there. `explode | index(0)` agrees on both. +# # `Review-verdict: MERGEABLE @ ` would arrive at the matcher as a valid verdict line — # text that is not a verdict silently becoming one. +# PARSE CHECK FIRST, separately, because jq's exit codes are not portable enough to distinguish +# "malformed input" from "valid input, no output" (ersatztv#647): jq >= 1.7 exits 5 on a parse error +# while jq 1.6 exits 4 — the SAME code both versions use for "filter produced no output", which is +# the legitimate empty-comment-list case. So on jq 1.6 the check below could not tell a garbage API +# response from "no comments yet", and silently returned `absent` where it should have raised an +# input error. `jq empty` separates the two on every version: non-zero iff the input does not parse, +# regardless of how much output the filter would produce. +if ! printf '%s' "$comments" | jq empty >/dev/null 2>&1; then + printf 'check-review-verdict: stdin is not valid JSON\n' >&2 + exit 2 +fi encoded=$(printf '%s' "$comments" | jq -ce ' if type != "array" then error("not an array") else .[] end | (.body // "") | if type != "string" then error("non-string body") - elif contains("\u0000") then error("NUL in body") + elif (explode | index(0)) != null then error("NUL in body") else . end' 2>/dev/null) jq_rc=$? # `jq -e` exits 4 when a filter produced NO output — which is exactly the legitimate empty-comment-list