diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 53f19f96f..d5a2834c4 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -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 `:` 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: @ ` comment references the PR's current head sha (short-sha prefix match against the verdict's OWN `@ ` 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: @ ` comment references the PR's current head sha (short-sha prefix match against the verdict's OWN `@ ` 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 `" 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) | diff --git a/docs/decisions/records/release/review-verdict-gate.md b/docs/decisions/records/release/review-verdict-gate.md index 223db2528..36d0ef15f 100644 --- a/docs/decisions/records/release/review-verdict-gate.md +++ b/docs/decisions/records/release/review-verdict-gate.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: @ ` comment references the PR''s current head sha (short-sha prefix match against the verdict''s OWN `@ ` 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: @ ` comment references the PR''s current head sha (short-sha prefix match against the verdict''s OWN `@ ` 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 diff --git a/scripts/check-review-verdict.sh b/scripts/check-review-verdict.sh index 6cf2e97e6..79c3cf4e7 100755 --- a/scripts/check-review-verdict.sh +++ b/scripts/check-review-verdict.sh @@ -21,10 +21,15 @@ # # THE GRAMMAR (deliberately strict — every relaxation here has been a false-open): # -# ^[space]* review-verdict: [space]* [space]* @ [space]* <7-40 hex> +# ^review-verdict: [space]* [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>` 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 diff --git a/scripts/tests/test_check_review_verdict.py b/scripts/tests/test_check_review_verdict.py index b75da0e93..d8b90f77e 100644 --- a/scripts/tests/test_check_review_verdict.py +++ b/scripts/tests/test_check_review_verdict.py @@ -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():