fix(629): close six false-opens in the H10 verdict grammar, and give it tests #636

Merged

timothy merged 6 commits from fix/629-verdict-classifier-falseopens into main 2026-07-26 03:02:32 +02:00

Conversation 1 Commits 6 Files Changed 6

+792 -84

timothy commented 2026-07-26 00:31:39 +02:00

Owner

Copy Link

Copy Source

fixes #629

The H10 verdict classification lived inline in pretooluse-merge-consent.sh with no tests. Protections the release.review-verdict-gate record described were never actually implemented — and each one made an unreviewed head read as approved.

What was broken

Every row reproduced before being fixed, and the "before" column is measured, not inferred:

input	before	now
Review-verdict: MERGEABLE-LATER @ <head>	positive	unknown
a verdict inside a ``` fence	positive	absent
Review-verdict: MERGEABLE [x](https://e/@0123456)	positive	no-sha
a verdict inside a ~~~ fence	positive	absent
Review-verdict: MERGEABLE @ <head>f / …ZZZ	positive	no-sha
body containing \x01BODY-BOUNDARY\x01 inside an open fence	positive	absent

The first three were in the code as it has stood for months. The last three I introduced while fixing the first three, and a cross-family review caught them — each is the same fix done half-way (fences stripped for only one marker; a length range with no right boundary, so a malformed token was silently truncated into a passing one; an in-band separator between comment bodies, which is forgeable by anyone who can comment on the PR).

The read path had to fail closed too

A second review round found the reader itself could fail open:

• Reading bodies through a here-document makes bash materialise a temp file. When that fails, the loop reads nothing and the classifier returned absent — discarding a real BLOCKED verdict. Reproduced with TMPDIR=/nonexistent: absent before, negative now. Both loops are off here-docs.
• A malformed payload (object-valued .body) degraded to absent rather than an input error. Shape is now asserted in jq, so it exits 2 and callers fail closed.
• Pre-existing, found while probing: bash strips NULs in command substitution, so Review<NUL>-verdict: MERGEABLE @ <head> arrived at the matcher as a valid verdict — text that is not a verdict becoming one in transit. NUL-bearing bodies are now rejected.

Why it happened

The grammar existed in two places: described precisely in the decision record, implemented loosely in the hook, with nothing comparing them. The record even asserted the URL protection as working; it never existed.

Worse, the test that was supposed to catch drift (test_post_review_verdict.py) re-implemented the shell regexes in Python and asserted the shell source still contained them — validating a copy against a copy. That mirror is deleted, not updated; the cross-check now runs the real classifier.

The grammar now lives in one tested place, scripts/check-review-verdict.sh, called by the hook and by post-review-verdict.sh's cross-check.

Verification

• 161 tests (44 on the classifier). Each grammar fix is mutation-verified: reverting it fails exactly the corresponding test, with a green control.
• The out-of-band-bodies fix has no equivalent mutation (structural, not a regex), so its evidence is direct reproduction against the prior commit.
• Live integration: PR #613's real history — six verdicts across four shas — classifies positive at its true head, negative at 4c320e6f (which carries both MERGEABLE and BLOCKED — negative wins), stale at an unrelated sha.
• shellcheck clean, ruff/pyright clean, decisions-validate: OK, catalog regenerated.

Scope note

#629 listed four items. The fourth is not a defect and is not claimed as a fix: a later @ <head> on a BLOCKED line is a verdict for the sha in its own field, so stale is correct — and was correct before this change. Kept as a characterization test.

Two previously-green tests changed and both were right to: an over-long token now classifies no-sha rather than stale (the old fixture was 45 hex chars, so it had been exercising the length guard while claiming to test the prefix rule), and jq -e exits 4 on no output, so an empty comment list had started reading as a malformed payload — turning "no comments yet" into an input error on a gate whose callers fail closed.

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.

🤖 Generated with Claude Code

fixes #629 The H10 verdict classification lived inline in `pretooluse-merge-consent.sh` with **no tests**. Protections the `release.review-verdict-gate` record described were never actually implemented — and each one made an unreviewed head read as approved. ## What was broken Every row reproduced before being fixed, and the "before" column is measured, not inferred: | input | before | now | |---|---|---| | `Review-verdict: MERGEABLE-LATER @ <head>` | `positive` | `unknown` | | a verdict inside a ` ``` ` fence | `positive` | `absent` | | `Review-verdict: MERGEABLE [x](https://e/@0123456)` | `positive` | `no-sha` | | a verdict inside a `~~~` fence | `positive` | `absent` | | `Review-verdict: MERGEABLE @ <head>f` / `…ZZZ` | `positive` | `no-sha` | | body containing `\x01BODY-BOUNDARY\x01` inside an open fence | `positive` | `absent` | The first three were in the code as it has stood for months. The last three I introduced *while fixing the first three*, and a cross-family review caught them — each is the same fix done half-way (fences stripped for only one marker; a length range with no right boundary, so a malformed token was silently **truncated** into a passing one; an in-band separator between comment bodies, which is forgeable by anyone who can comment on the PR). ## The read path had to fail closed too A second review round found the reader itself could fail *open*: - Reading bodies through a **here-document** makes bash materialise a temp file. When that fails, the loop reads nothing and the classifier returned `absent` — discarding a real BLOCKED verdict. Reproduced with `TMPDIR=/nonexistent`: `absent` before, `negative` now. Both loops are off here-docs. - A malformed payload (object-valued `.body`) degraded to `absent` rather than an input error. Shape is now asserted in jq, so it exits 2 and callers fail closed. - Pre-existing, found while probing: bash strips NULs in command substitution, so `Review<NUL>-verdict: MERGEABLE @ <head>` arrived at the matcher as a valid verdict — text that is not a verdict becoming one in transit. NUL-bearing bodies are now rejected. ## Why it happened The grammar existed in **two places**: described precisely in the decision record, implemented loosely in the hook, with nothing comparing them. The record even asserted the URL protection as working; it never existed. Worse, the test that was supposed to catch drift (`test_post_review_verdict.py`) re-implemented the shell regexes **in Python** and asserted the shell source still contained them — validating a copy against a copy. That mirror is deleted, not updated; the cross-check now runs the real classifier. The grammar now lives in one tested place, `scripts/check-review-verdict.sh`, called by the hook and by `post-review-verdict.sh`'s cross-check. ## Verification - **161 tests** (44 on the classifier). Each grammar fix is **mutation-verified**: reverting it fails exactly the corresponding test, with a green control. - The out-of-band-bodies fix has no equivalent mutation (structural, not a regex), so its evidence is direct reproduction against the prior commit. - Live integration: PR #613's real history — six verdicts across four shas — classifies `positive` at its true head, `negative` at `4c320e6f` (which carries both MERGEABLE and BLOCKED — negative wins), `stale` at an unrelated sha. - shellcheck clean, ruff/pyright clean, `decisions-validate: OK`, catalog regenerated. ## Scope note #629 listed four items. **The fourth is not a defect** and is not claimed as a fix: a later `@ <head>` on a BLOCKED line is a verdict for the sha in *its own field*, so `stale` is correct — and was correct before this change. Kept as a characterization test. Two previously-green tests changed and both were right to: an over-long token now classifies `no-sha` rather than `stale` (the old fixture was 45 hex chars, so it had been exercising the length guard while claiming to test the prefix rule), and `jq -e` exits **4** on no output, so an empty comment list had started reading as a malformed payload — turning "no comments yet" into an input error on a gate whose callers fail closed. 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. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

timothy added 3 commits 2026-07-26 00:31:40 +02:00

fix(629): close three false-opens in the H10 verdict grammar, and give it tests ... 38a96f47cc

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

fix(629): review fixes — tilde fences, an unbounded sha field, and a forgeable comment boundary ... 8d0df55398

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

fix(629): re-review fixes — the READ path must fail closed too ... 3bc3c39948

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>

timothy referenced this pull request 2026-07-26 00:34:12 +02:00

merge gate: false-opens in the H10 verdict classifier — an undefined token, code blocks (fenced/indented/HTML), a URL-borne sha, and a read path that failed open #629

timothy added 1 commit 2026-07-26 00:43:09 +02:00

fix(629): round-3 fixes — fence LENGTH semantics, and stop masking reader failures ... 3ec15e9f17

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

timothy force-pushed fix/629-verdict-classifier-falseopens from 3ec15e9f17 to 299e7b27af 2026-07-26 00:48:17 +02:00 Compare

timothy added 1 commit 2026-07-26 00:52:16 +02:00

fix(629): round-4 — require the marker at COLUMN 0, narrowing the grammar instead of patching again ... 28a9d0dbfd

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

timothy added 1 commit 2026-07-26 01:15:02 +02:00

fix(629): strip raw HTML blocks, and state where the hardening stops ... 50bcd7b0c7

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

timothy commented 2026-07-26 02:47:18 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ 50bcd7b

Five cross-family review rounds (codex exec, GPT-5.4 high). Every round found real false-opens; rounds 2-5 each found a bug introduced while fixing the round before, always the same fix applied half-way.

Fixed and each mutation-verified (reverting the fix fails exactly the matching test, control green): prefix-matched verdict token; ``` fences; ~~~ fences; fence-LENGTH semantics (a longer fence legitimately contains a shorter marker); indented code blocks (closed by requiring the marker at COLUMN 0, which removes the whole indentation class rather than adding a second stripper); raw HTML (

//comments); sha taken from a URL; an unbounded sha field that TRUNCATED a malformed token into a passing one; a forgeable in-band comment separator; and three read-path fail-opens (here-doc temp-file failure, malformed shapes, NUL-bearing bodies) that returned absent and would have discarded a real BLOCKED verdict.

178 tests. Real-corpus regression: 7 PRs / 15 posted verdict markers classify identically after the column-0 tightening, so its accepted cost has never been paid in this repo's history.
DISCLOSED, so this verdict is not read as stronger than it is: round 5's HTML finding was fixed WITHOUT an independent round 6. Hardening was stopped by an explicit decision, recorded in release.review-verdict-gate: this scanner is a best-effort heuristic, not a markdown parser, and stopping is safe because the comment is not the load-bearing gate — review-verdict/h10 is written only by this script from explicit arguments and a comment cannot forge it. A residual false-open means the hook does not object; it does not mean a merge happens.
Also rejected with measurement rather than deferred: a 'verdict must be the first line' rule would kill every code-block form at once, but 14 of 18 verdict markers ever posted here are not on the first line.

Review-verdict: MERGEABLE @ 50bcd7b Five cross-family review rounds (codex exec, GPT-5.4 high). Every round found real false-opens; rounds 2-5 each found a bug introduced while fixing the round before, always the same fix applied half-way. Fixed and each mutation-verified (reverting the fix fails exactly the matching test, control green): prefix-matched verdict token; ``` fences; `~~~` fences; fence-LENGTH semantics (a longer fence legitimately contains a shorter marker); indented code blocks (closed by requiring the marker at COLUMN 0, which removes the whole indentation class rather than adding a second stripper); raw HTML (<pre>/<code>/comments); sha taken from a URL; an unbounded sha field that TRUNCATED a malformed token into a passing one; a forgeable in-band comment separator; and three read-path fail-opens (here-doc temp-file failure, malformed shapes, NUL-bearing bodies) that returned `absent` and would have discarded a real BLOCKED verdict. 178 tests. Real-corpus regression: 7 PRs / 15 posted verdict markers classify identically after the column-0 tightening, so its accepted cost has never been paid in this repo's history. DISCLOSED, so this verdict is not read as stronger than it is: round 5's HTML finding was fixed WITHOUT an independent round 6. Hardening was stopped by an explicit decision, recorded in `release.review-verdict-gate`: this scanner is a best-effort heuristic, not a markdown parser, and stopping is safe because the comment is not the load-bearing gate — `review-verdict/h10` is written only by this script from explicit arguments and a comment cannot forge it. A residual false-open means the hook does not object; it does not mean a merge happens. Also rejected with measurement rather than deferred: a 'verdict must be the first line' rule would kill every code-block form at once, but 14 of 18 verdict markers ever posted here are not on the first line.

timothy merged commit caf3ca72c1 into main 2026-07-26 03:02:32 +02:00

timothy referenced this issue from a commit 2026-07-26 03:02:34 +02:00

Merge pull request 'fix(629): close six false-opens in the H10 verdict grammar, and give it tests' (#636) from fix/629-verdict-classifier-falseopens into main

timothy referenced this pull request 2026-07-26 03:03:26 +02:00

merge gate: false-opens in the H10 verdict classifier — an undefined token, code blocks (fenced/indented/HTML), a URL-borne sha, and a read path that failed open #629

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

ad-hoc

One-off / ad-hoc work not tracked by a dedicated issue

api

REST API / HTTP endpoints

bug

Something isn't working

ci-cd

Build, test, deploy pipeline

content

Channel content / schedules / playlists

dependencies

Dependency updates (Renovate)

enhancement

New feature or improvement

frontend

ChicoryTV React SPA frontend

in-progress

Claimed by an active session — do not pick up

jellyfin

Jellyfin tuner / IPTV integration

parked

Excluded from automatic queue pickup; work only when explicitly selected

priority: high

priority: low

priority: medium

review

Adversarial review finding

security

Security / vulnerability fix

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate timothy (Timothy)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: timothy/ersatztv#636