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
parent 299e7b27af
commit 28a9d0dbfd
4 changed files with 48 additions and 11 deletions
@@ -111,7 +111,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera
 | `release.migration-rehearsal-prodcopy` | Before promoting a migration-bearing release, rehearse the new image's migrations against a throwaway copy of the latest prod backup (`scripts/migration-smoke.sh`), gating PASS on the migrator's completion log line rather than HTTP readiness alone. | 2026-07-12 | [link](records/release/migration-rehearsal-prodcopy.md) |
 | `release.prepush-clean-worktree-guard` | A fail-open pre-push hook blocks a push when any file in the branch's diff vs `origin/main` also has uncommitted working-tree or index changes, since a stale-index commit (e.g. `git reset --soft` + `git add` over an edited-but-unstaged fix) can silently push, CI-test, and get reviewed a different tree than the one on disk. Scope is precise to pushed-diff files; escape hatch `ETV_ALLOW_DIRTY_PUSH=1`. | 2026-07-17 | [link](records/release/prepush-clean-worktree-guard.md) |
 | `release.promotion-floating-prod` | Prod tracks the floating `:prod` image reference; a tag build's immutable `:<version>` image is scanned first, then promotion happens via a separate manual `DeployStack`, with daily auto-update only as a fallback — tag with enough runway before 03:00 to avoid an unscanned promotion. | 2026-07-13 | [link](records/release/promotion-floating-prod.md) |
-| `release.review-verdict-gate` | A PR may not merge until a `Review-verdict: <MERGEABLE\|APPROVED\|BLOCKED\|NOT-MERGEABLE> @ <head-sha>` comment references the PR's current head sha (short-sha prefix match against the verdict's OWN `@ <sha>` field, line-start marker only, whole-word verdict token, fenced code blocks stripped, negative wins over positive on the same head); folds into the H6 merge-consent hook as condition (c). The grammar lives in ONE tested place, `scripts/check-review-verdict.sh` — #629 found three false-opens that survived because it was implemented inline and untested while this record described stricter behaviour than the code had. | 2026-07-12 | [link](records/release/review-verdict-gate.md) |
+| `release.review-verdict-gate` | A PR may not merge until a `Review-verdict: <MERGEABLE\|APPROVED\|BLOCKED\|NOT-MERGEABLE> @ <head-sha>` comment references the PR's current head sha (short-sha prefix match against the verdict's OWN `@ <sha>` field, marker at COLUMN 0 (no indent, so indented code blocks cannot self-approve), whole-word verdict token, fenced code blocks stripped with markdown fence-length semantics, negative wins over positive on the same head); folds into the H6 merge-consent hook as condition (c). The grammar lives in ONE tested place, `scripts/check-review-verdict.sh` — #629 found three false-opens that survived because it was implemented inline and untested while this record described stricter behaviour than the code had. | 2026-07-12 | [link](records/release/review-verdict-gate.md) |
 | `release.verdict-status-check` | The H10 review verdict is written as a `review-verdict/h10` Gitea **commit status** on the exact reviewed sha by `scripts/post-review-verdict.sh`, and that context is a REQUIRED status check on `main`. Because a status belongs to one sha, a later commit cannot inherit it, so Gitea's own `merge_when_checks_succeed` refuses to merge a head no one reviewed. The PreToolUse hook additionally refuses to SCHEDULE an auto-merge unless that status is already green on head. A `pull_request` workflow auto-passes the two exempt classes (Renovate-authored, docs-only) unless the PR touches a protected path (`.claude/`, `.gitea/`, `.husky/`, `scripts/`, `docker/ci/`). This extends — does not supersede — `release.review-verdict-gate` (#303 H10), whose comment convention remains the human-readable artifact and the hook's condition (c). | 2026-07-25 | [link](records/release/verdict-status-check.md) |
 | `rulebuilder.relative-date-macros` | The visual rule builder's `inLast`/`notInLast` date operators compile to/parse from the pre-existing `CustomMultiFieldQueryParser` macros `released_inthelast`/`released_notinthelast` and `added_inthelast`/`added_notinthelast`, value form `"<n> day\|week\|month\|year"`; there is no backend change. | 2026-07-23 | [link](records/rulebuilder/relative-date-macros.md) |
 | `scan.collections-scan-status` | `GET /api/v1/media-sources/collections-scan-status` reports a family-global (not per-source), boolean-only active-scan set read from `IEntityLocker`; the SPA reconciles authoritatively against it (with a grace-tick helper) instead of a fixed client-side timeout. | 2026-07-12 | [link](records/scan/collections-scan-status.md) |
@@ -5,7 +5,7 @@ status: active
 since: '2026-07-12'
 supersedes: none
 superseded-by: none
-rule: 'A PR may not merge until a `Review-verdict: <MERGEABLE|APPROVED|BLOCKED|NOT-MERGEABLE> @ <head-sha>` comment references the PR''s current head sha (short-sha prefix match against the verdict''s OWN `@ <sha>` field, line-start marker only, whole-word verdict token, fenced code blocks stripped, negative wins over positive on the same head); folds into the H6 merge-consent hook as condition (c). The grammar lives in ONE tested place, `scripts/check-review-verdict.sh` — #629 found three false-opens that survived because it was implemented inline and untested while this record described stricter behaviour than the code had.'
+rule: 'A PR may not merge until a `Review-verdict: <MERGEABLE|APPROVED|BLOCKED|NOT-MERGEABLE> @ <head-sha>` comment references the PR''s current head sha (short-sha prefix match against the verdict''s OWN `@ <sha>` field, marker at COLUMN 0 (no indent, so indented code blocks cannot self-approve), whole-word verdict token, fenced code blocks stripped with markdown fence-length semantics, negative wins over positive on the same head); folds into the H6 merge-consent hook as condition (c). The grammar lives in ONE tested place, `scripts/check-review-verdict.sh` — #629 found three false-opens that survived because it was implemented inline and untested while this record described stricter behaviour than the code had.'
 signals: 'review-verdict, head-sha match, stale-review prevention, verdict false-open, MERGEABLE-LATER, fenced code block verdict, sha from a URL, unknown verdict token · paths: `scripts/check-review-verdict.sh`, `.claude/hooks/pretooluse-merge-consent.sh`, `.claude/settings.json` · issues: #303 (H10), #242, #629'
 mechanics: '`scripts/check-review-verdict.sh` (the grammar, + `scripts/tests/test_check_review_verdict.py`); `pretooluse-merge-consent.sh` (maps a class onto allow/deny/ask); `scripts/post-review-verdict.sh`; CLAUDE.md → Task Completion Protocol (H10 convention)'
 ---
@@ -62,6 +62,21 @@ recording because each is the same fix done half-way:
  exposed a verdict still inside an unclosed fence. Bodies are now carried out-of-band (one
  JSON-encoded string per line), which removes the class rather than escaping the sentinel.

+A fourth round found the last one, and it is why the grammar was **narrowed** rather than patched
+again: markdown has a second code-block form — *indented* blocks (4 spaces or a tab) — which the fence
+stripper does not cover, so a pasted indented example still self-approved. Rather than add a second
+stripper (the same instance-not-class move that produced the previous three rounds), **the marker must
+now sit at COLUMN 0**. That removes every indentation-based ambiguity at once. The cost is that a
+verdict indented under a list item is ignored and classifies `absent` — which asks a human, the safe
+direction for a gate. Fence detection keeps its leading-whitespace tolerance, because stripping *more*
+is always safe.
+
+Four review rounds, four sets of real findings, and rounds 2–4 each found a bug introduced while
+fixing the round before — every one the same fix applied half-way (one fence marker but not the other;
+a bounded left side but not the right; state reset per comment but via a delimiter the writer
+controls). For attacker-writable text, budget several rounds and prefer eliminating a class over
+enumerating its members.
+
 The lesson is about where a grammar lives, not about any one regex: this record described the intended
 behaviour accurately, the code did something looser, and nothing compared them. The grammar now lives
 in `scripts/check-review-verdict.sh` — one implementation, called by the hook and by
@@ -21,10 +21,15 @@
 #
 # THE GRAMMAR (deliberately strict — every relaxation here has been a false-open):
 #
-#   ^[space]* review-verdict: [space]* <TOKEN> [space]* @ [space]* <7-40 hex>
+#   ^review-verdict: [space]* <TOKEN> [space]* @ [space]* <7-40 hex>     (marker at COLUMN 0)
 #
-# - **Line-start marker.** Rejects a comment that quotes the template mid-sentence ("please post:
-#   Review-verdict: MERGEABLE @ …"), which would otherwise self-approve.
+# - **Marker at COLUMN 0** — no leading whitespace (#629). This rejects a mid-sentence quote
+#   ("please post: Review-verdict: MERGEABLE @ …"), and it also rejects markdown's *indented* code
+#   blocks (4 spaces or a tab) and anything nested in a list. Indented code is a second code-block
+#   form the fence stripper does not cover, and patching each form in turn is how three rounds of
+#   false-opens happened; requiring column 0 removes the ambiguity rather than enumerating it.
+#   The cost is that a verdict indented under a list item is ignored — it classifies `absent`, which
+#   asks a human. Erring toward ignoring is the safe direction for a gate.
 # - **Fenced code blocks are stripped first** (#629). The line-start anchor alone does not save us:
 #   inside a ``` block the marker IS at line start, so documentation showing the convention counted
 #   as a real verdict. Fence state is tracked PER COMMENT BODY so an unclosed fence in one comment
@@ -124,7 +129,7 @@ verdicts=$(printf '%s\n' "$encoded" | while IFS= read -r encoded_body; do
  # `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:')
+  found=$(printf '%s\n' "$outside" | grep -iE '^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
@@ -144,9 +149,9 @@ fi
 # its length is checked separately. Matching `{7,40}` directly had no right boundary, so an over-long
 # or malformed token was silently TRUNCATED into a valid-looking one: `@ <40-hex-head><more hex>` and
 # `@ <40-hex-head>ZZZ` both matched their first 40 chars and graded as a verdict for head.
-FIELD_RE='^[[:space:]]*review-verdict:[[:space:]]*[A-Za-z][A-Za-z-]*[[:space:]]*@[[:space:]]*[0-9a-fA-F]+([^0-9a-zA-Z]|$)'
-POS_RE='^[[:space:]]*review-verdict:[[:space:]]*(mergeable|approved|lgtm)([[:space:]@]|$)'
-NEG_RE='^[[:space:]]*review-verdict:[[:space:]]*(blocked|not-mergeable)([[:space:]@]|$)'
+FIELD_RE='^review-verdict:[[:space:]]*[A-Za-z][A-Za-z-]*[[:space:]]*@[[:space:]]*[0-9a-fA-F]+([^0-9a-zA-Z]|$)'
+POS_RE='^review-verdict:[[:space:]]*(mergeable|approved|lgtm)([[:space:]@]|$)'
+NEG_RE='^review-verdict:[[:space:]]*(blocked|not-mergeable)([[:space:]@]|$)'

 head_pos=0; head_neg=0; stale=0; unknown=0
 while IFS= read -r line; do
@@ -197,8 +197,25 @@ def test_short_sha_prefix_is_accepted():
    assert classify([verdict("MERGEABLE", SHORT)]) == ("positive", 0)


-def test_leading_indent_is_tolerated():
-    assert classify([f"   {verdict('MERGEABLE', HEAD)}"]) == ("positive", 0)
+@pytest.mark.parametrize(
+    "prefix",
+    [
+        "    ",  # markdown indented code block (4 spaces)
+        "\t",  # indented code block (tab)
+        "  ",  # any indent at all
+        "      ",  # nested under a list item
+    ],
+)
+def test_falseopen_an_indented_verdict_is_not_a_verdict(prefix):
+    """Markdown's *indented* code block is a second code-block form the fence stripper misses.
+
+    Allowing leading whitespace let a pasted, indented example self-approve. The marker must sit at
+    COLUMN 0. This is a deliberate tightening — a verdict indented under a list item now classifies
+    `absent` and asks a human, which is the safe direction — and it removes the whole indentation
+    ambiguity rather than adding a second stripper for each code-block form.
+    """
+    body = f"Example:\n\n{prefix}{verdict('MERGEABLE', HEAD)}\n"
+    assert classify([body]) == ("absent", 0)


 def test_verdict_among_ordinary_prose_in_the_same_comment():