7 Commits

Include renames

Author	SHA1	Message	Date
timothyandClaude Opus 5	7265fba36d	fix(647): the H10 verdict classifier was inert on jq 1.6 — the runner's version ... 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("<NUL>")` 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) <noreply@anthropic.com>	2026-07-26 13:24:20 +02:00
timothyandClaude Opus 5	50bcd7b0c7	fix(629): strip raw HTML blocks, and state where the hardening stops ... Round 5: raw HTML is the third code-block form. `<pre>`, `<code>` and HTML comments all render their contents literally, so a verdict inside one is an example, not an approval: <pre> / <code> / <!-- ... --> containing a verdict -> positive Now stripped, tracked as a marker count rather than parsed — the direction of error is to strip MORE, which can only ever withhold approval. Mutation-verified: removing the stripper fails all five cases. AND THE HARDENING STOPS HERE, deliberately. The record now says so, because otherwise the next session re-derives it: this is a best-effort heuristic, not a markdown parser. It covers the three code-block forms markdown has (fenced; indented, via the column-0 rule; raw HTML) and is not proof against every way to render text as non-prose. Stopping is safe because the comment is NOT the load-bearing gate. Since #622 the authoritative signal is the `review-verdict/h10` commit status, written only by post-review-verdict.sh from explicit arguments — a comment cannot forge it. This classifier is condition (c) of the PreToolUse hook: defense in depth on an agent's merge call. A residual false-open means the hook does not object; it does not mean a merge happens. Five rounds found five code-block forms, four of them introduced while fixing the previous round. The generalisable rule, now in the record: when a heuristic keeps failing at the edges, check whether it is actually the thing enforcing the invariant before spending another round on it. Also measured, against the real corpus: a "verdict must be the first line" rule would have killed every code-block form at once, but 14 of 18 verdict markers ever posted in this repo are NOT on the first line — so it was rejected as a retroactive break, not deferred. 178 tests. refs #629 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Decisions-Edit: yes	2026-07-26 01:14:59 +02:00
timothyandClaude Opus 5	28a9d0dbfd	fix(629): round-4 — require the marker at COLUMN 0, narrowing the grammar instead of patching again ... Round 4 found the last false-open: markdown has a SECOND code-block form the fence stripper does not cover — indented blocks (4 spaces or a tab). A pasted indented example still self-approved: Example: Review-verdict: MERGEABLE @ <head> -> positive Adding an indented-code stripper would be the same move that produced rounds 2, 3 and 4: fixing the instance, not the class. So the grammar is narrowed instead — the marker must sit at COLUMN 0. That kills every indentation-based ambiguity at once (4-space, tab, list-nested, arbitrary indent). Cost, accepted deliberately: a verdict indented under a list item is now ignored and classifies `absent`, which asks a human. For a gate, erring toward ignoring is the safe direction. Fence detection KEEPS its leading-whitespace tolerance, because stripping more is always safe. `test_leading_indent_is_tolerated` asserted the old behaviour and is replaced by `test_falseopen_an_indented_verdict_is_not_a_verdict`, parameterised over four indent shapes and mutation-verified: restoring `^[[:space:]]*` fails all four, control green. 172 tests. Round 4 verified clean by execution: the rc plumbing fails closed for a forced failure in the inner jq, awk, grep AND the pipeline producer (rc 3/4/5/93 -> exit 2); fence-length semantics, mismatched markers, CRLF fences, 10-marker fences, blockquote fences; emoji, CRLF, a 120k line, 200 comments; NUL rejection with no JSON-encoding bypass. refs #629 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Decisions-Edit: yes	2026-07-26 00:52:11 +02:00
timothyandClaude Opus 5	299e7b27af	fix(629): round-3 fixes — fence LENGTH semantics, and stop masking reader failures ... 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 @ <head> / ```` -> 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) <noreply@anthropic.com> Decisions-Edit: yes	2026-07-26 00:47:40 +02:00
timothyandClaude Opus 5	3bfd925baf	fix(629): re-review fixes — the READ path must fail closed too ... Re-review of 8d0df553 returned BLOCKED. Both findings are the same class as the bugs this issue is about, one level down: the reader, not the grammar. 1. A here-document makes bash materialise a temp file. When that fails (read-only or restricted TMPDIR) the loop reads NOTHING, and the classifier returned `absent` — silently discarding a real BLOCKED verdict. Reproduced with TMPDIR=/nonexistent: `absent` before, `negative` now. Both loops are off here-docs (a pipe for the reader, process substitution for the classifier so its flags stay in the current shell). "The environment could not supply a temp file" is not evidence that a PR was approved. 2. A malformed payload degraded to `absent` instead of an input error: an object-valued `.body` reached the scanner and simply matched nothing. Shape is now asserted in jq — must be an array, bodies must be strings — so it exits 2 and callers fail closed. Also closed a pre-existing false-open the review found while probing (present before #629, so not a regression, but cheap to fix here): bash strips NULs in command substitution, so a body of `Review<NUL>-verdict: MERGEABLE @ <head>` arrived at the matcher as a valid verdict line — text that is not a verdict becoming one in transit. Bodies containing NUL are now rejected. 161 tests. Verified: 80k body still classifies; empty array, null body still `absent`; malformed JSON, non-array, array-of-non-objects, object body, NUL body all exit 2; a hostile TMPDIR still returns `negative`. refs #629 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-26 00:47:40 +02:00
timothyandClaude Opus 5	f151b93245	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	2026-07-26 00:47:40 +02:00
timothyandClaude Opus 5	0f83b54334	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	2026-07-26 00:47:40 +02:00