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

scripts/tests/test_check_review_verdict.py

@@ -77,6 +77,49 @@ def test_fence_state_does_not_leak_between_comments():
     assert classify(["Example:\n```\nnot a verdict", verdict("MERGEABLE", HEAD)]) == ("positive", 0)
 
 
+# --- found by cross-family review of the first fix (all three reproduced before fixing) ----------
+
+
+def test_falseopen_tilde_fence_is_also_stripped():
+    """Markdown accepts `~~~` as well as ```; stripping only backticks left the hole half-open."""
+    body = f"~~~\n{verdict('MERGEABLE', HEAD)}\n~~~"
+    assert classify([body]) == ("absent", 0)
+
+
+@pytest.mark.parametrize(
+    "suffix",
+    [
+        "f",  # a 41st hex char -> over-long, must not truncate to a valid 40
+        "9999",  # a longer wrong sha
+        "ZZZ",  # a non-hex suffix welded to a valid sha
+    ],
+)
+def test_falseopen_sha_field_needs_a_right_boundary(suffix):
+    """Matching `{7,40}` with no right boundary silently TRUNCATED a malformed token into a match.
+
+    `@ <40-hex-head><anything>` matched its first 40 characters and graded as a verdict for head.
+    """
+    assert classify([verdict("MERGEABLE", HEAD + suffix)]) == ("no-sha", 0)
+
+
+def test_falseopen_a_body_cannot_forge_a_comment_boundary():
+    """The separator between comments must be out-of-band.
+
+    An earlier version joined bodies with a literal `\\x01BODY-BOUNDARY\\x01` line. A comment
+    containing that line could reset fence state mid-body and expose a verdict still inside an
+    unclosed fence — an in-band delimiter is forgeable by whoever writes the data, and here that is
+    anyone who can comment on the PR.
+    """
+    sep = "\x01BODY-BOUNDARY\x01"
+    body = f"```\n{sep}\n{verdict('MERGEABLE', HEAD)}"
+    assert classify([body]) == ("absent", 0)
+
+
+def test_a_valid_verdict_may_carry_trailing_prose():
+    """The right boundary must not reject the ordinary `@ <sha> (note)` shape."""
+    assert classify([f"{verdict('MERGEABLE', HEAD)} (all findings resolved)"]) == ("positive", 0)
+
+
 # --- the happy paths -------------------------------------------------------------------------
 
 
@@ -131,7 +174,15 @@ def test_verdict_only_for_an_older_commit_is_stale():
 
 
 def test_old_sha_containing_the_head_prefix_does_not_match():
-    contains_head_prefix = "abcdef" + SHORT + "1234567890abcdef1234567890abcdef"
+    """A VALID 40-char sha that contains the head prefix but does not start with it is stale.
+
+    The fixture is deliberately a well-formed sha: an over-long hex run is now rejected as malformed
+    (`no-sha`) rather than compared, so a 45-char fixture would have tested the length guard instead
+    of the prefix rule it is named for.
+    """
+    contains_head_prefix = ("abcdef" + SHORT + "0" * 40)[:40]
+    assert len(contains_head_prefix) == 40
+    assert SHORT in contains_head_prefix and not contains_head_prefix.startswith(SHORT)
     assert classify([verdict("MERGEABLE", contains_head_prefix)]) == ("stale", 0)