From 9fd8f40541c6232a2ea84665a24a0c9d7c56174c Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 12 Jul 2026 18:54:05 +0200 Subject: [PATCH 1/4] =?UTF-8?q?chore(process):=20#303=20H10=20=E2=80=94=20?= =?UTF-8?q?review-verdict=20merge-gate=20(latest=20commit=20must=20be=20re?= =?UTF-8?q?viewed)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Folds condition (c) into pretooluse-merge-consent.sh (H6): a PR merge is allowed only when a `Review-verdict:` comment references the PR's CURRENT head sha — proving the latest commit was reviewed, not a stale earlier diff (mechanizes the ersatztv#242 "re-review the fix commit" lesson). Graceful adoption mirrors H6's Done-when tiering: - positive verdict @ head -> allow - verdict @ older sha (stale) -> deny (#242 failure mode) - head verdict negative -> deny - marker with no sha / none yet -> ask - comments unfetchable -> ask Reuses H6's PR fetch, docs-only exemption, and Gitea-auth-from-env (one hook, no detection drift — per the #303 methodology review). Pipe-tested 12 cases. Docs: decisions.md (new H10 entry + TOC), CLAUDE.md Task Completion Protocol. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/hooks/pretooluse-merge-consent.sh | 44 ++++++++++++++++++++--- CLAUDE.md | 4 +-- docs/decisions.md | 29 +++++++++++++++ 3 files changed, 71 insertions(+), 6 deletions(-) diff --git a/.claude/hooks/pretooluse-merge-consent.sh b/.claude/hooks/pretooluse-merge-consent.sh index 5b8b6457e..21d066184 100755 --- a/.claude/hooks/pretooluse-merge-consent.sh +++ b/.claude/hooks/pretooluse-merge-consent.sh @@ -1,11 +1,16 @@ #!/usr/bin/env bash # PreToolUse / mcp__gitea__pull_request_write — derive merge consent from STATE instead of -# trusting the agent's judgment (ersatztv#303 H6). A PR merge is the one irreversible op; allow it -# only when BOTH are true: +# trusting the agent's judgment (ersatztv#303 H6 + H10). A PR merge is the one irreversible op; allow it +# only when ALL are true: # (a) the PR's CI combined status is green, AND -# (b) every checkbox in the linked issue's "## Done-when" section is ticked. +# (b) every checkbox in the linked issue's "## Done-when" section is ticked, AND +# (c) a review-verdict comment on the PR references the CURRENT head sha (H10) — proving the +# LATEST commit was reviewed, not a stale earlier diff (the ersatztv#242 failure mode: +# "re-review the fix commit, not just the initial PR diff"). # The "## Done-when" issue-body checklist is the convention (docs/decisions.md, CLAUDE.md Task # Completion Protocol). One box is "adversarial review passed"; the others are per-issue. +# The H10 review-verdict convention: after reviewing a PR (or its latest fix commit), post a PR +# comment carrying a line `Review-verdict: @ `. # # Decision policy — a CONSENT gate, so it does NOT fail silently open: # - state derivable and NOT satisfied -> deny (actionable reason) @@ -97,5 +102,36 @@ if [ "$mwcs" != "true" ]; then esac fi -# Both derivable and satisfied -> allow. +# --- (c) Review-verdict freshness (ersatztv#303 H10): a review-verdict comment must reference the +# CURRENT head sha, so the latest commit is proven-reviewed (ersatztv#242: re-review the fix +# commit, not just the initial diff). Graceful adoption mirrors (b): a verdict comment that +# references head must be positive -> allow; one that exists only for an OLDER commit -> deny +# (the stale-review failure mode); NO verdict comment at all -> ask (convention not yet used). +[ -n "$sha" ] || decide ask "H10 merge gate: could not resolve PR #$pr head sha to verify a review verdict. Confirm the review covered the latest commit before merging." +short=${sha:0:7} +comments=$(gq "repos/$owner/$repo/issues/$pr/comments?limit=100") +if [ -z "$comments" ]; then + decide ask "H10 merge gate: could not fetch PR #$pr comments to verify a head-referencing review verdict ($short). Confirm the adversarial/Codex review covered the latest commit before merging." +fi +# Verdict lines across all comment bodies (the convention marker, case-insensitive). +verdicts=$(printf '%s' "$comments" | jq -r '.[].body // empty' 2>/dev/null | grep -iE 'review-verdict:' || true) +if [ -z "$verdicts" ]; then + decide ask "H10 merge gate: no 'Review-verdict:' comment found on PR #$pr referencing head $short. Post the adversarial/Codex verdict (e.g. 'Review-verdict: MERGEABLE @ $short'), or confirm the review covered the latest commit and approve." +fi +# Of those, the ones that reference the current head (>=7-char sha prefix, fixed-string). +head_verdicts=$(printf '%s\n' "$verdicts" | grep -F "$short" || true) +if [ -z "$head_verdicts" ]; then + # A verdict marker exists but none references head. Distinguish a real stale review (the line + # carries a sha-shaped token for an OLDER commit -> deny, the #242 case) from a marker with no + # sha at all (a lazy or convention-quoting comment -> ask, don't mislabel it as stale). + if printf '%s\n' "$verdicts" | grep -iqE '[0-9a-f]{7,40}'; then + decide deny "H10 merge gate: BLOCKED — a review-verdict comment references an older commit, not the current head ($short). The latest commit(s) are unreviewed (ersatztv#242: re-review the fix commit, not just the initial diff). Re-review the head and post 'Review-verdict: MERGEABLE @ $short'." + fi + decide ask "H10 merge gate: a 'Review-verdict:' comment on PR #$pr references no commit sha. Post one referencing the current head ($short) — e.g. 'Review-verdict: MERGEABLE @ $short' — or confirm the review covered the latest commit and approve." +fi +if ! printf '%s\n' "$head_verdicts" | grep -iqE 'review-verdict:[[:space:]]*(mergeable|approved|lgtm)'; then + decide deny "H10 merge gate: BLOCKED — the review verdict for the current head ($short) is not MERGEABLE/APPROVED. Resolve the findings and post a fresh 'Review-verdict: MERGEABLE @ $short' comment before merging PR #$pr." +fi + +# All derivable and satisfied -> allow. decide allow "" diff --git a/CLAUDE.md b/CLAUDE.md index 3a095ee8c..4d28491c3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -82,8 +82,8 @@ docker build -f docker/Dockerfile -t ersatztv:dev . Every task that closes a Gitea issue MUST complete ALL of these before it is considered done. Use `/done ` to run through this automatically. -**Merge-consent is derived from state, not asserted (`## Done-when` convention — ersatztv#303 H6).** Any issue whose PR will merge to `main` should carry a `## Done-when` section in its **issue body** — a checklist of completion criteria (always include an "adversarial review passed" box; add per-issue criteria like tests-green, docs-updated, live-E2E). Two hooks derive merge-consent from it so a premature merge is blocked *by construction*, not by memory: -- `pretooluse-merge-consent.sh` (Claude PreToolUse on the Gitea merge tool) — **allows** a merge only when the PR's CI is green **and** every `## Done-when` box on the linked issue (`fixes #N`) is ticked; **denies** on an unticked box or red CI; **asks** (falls back to a human prompt) when it can't derive state (no linked issue, no `## Done-when` section, no creds, Gitea down). +**Merge-consent is derived from state, not asserted (`## Done-when` convention — ersatztv#303 H6 + H10).** Any issue whose PR will merge to `main` should carry a `## Done-when` section in its **issue body** — a checklist of completion criteria (always include an "adversarial review passed" box; add per-issue criteria like tests-green, docs-updated, live-E2E). Two hooks derive merge-consent from it so a premature merge is blocked *by construction*, not by memory: +- `pretooluse-merge-consent.sh` (Claude PreToolUse on the Gitea merge tool) — **allows** a merge only when the PR's CI is green **and** every `## Done-when` box on the linked issue (`fixes #N`) is ticked **and** a `Review-verdict:` comment references the PR's *current head sha* (**H10**); **denies** on an unticked box, red CI, or a stale/negative review verdict; **asks** (falls back to a human prompt) when it can't derive state (no linked issue, no `## Done-when` section, no `Review-verdict:` comment yet, no creds, Gitea down). **The H10 review-verdict convention**: after an adversarial/Codex review of a PR (or its latest fix commit), post a PR comment with a line `Review-verdict: @ ` — this proves the *latest* commit was reviewed, not a stale earlier diff (ersatztv#242). - `.husky/pre-push` → `prepush-donewhen.sh` — a fail-open backstop that blocks a direct `git push origin main` whose commits `fix #N` an issue with unticked boxes. Both need Gitea read creds in the env to enforce (**`ETV_GITEA_BASICAUTH=user:pass`** or `ETV_GITEA_TOKEN`; `ETV_GITEA_URL` overrides the base). Without them the merge hook asks and the push backstop is a no-op — the gate degrades to today's manual confirmation, never a silent pass. Docs-only PRs/pushes are exempt. diff --git a/docs/decisions.md b/docs/decisions.md index b3e0e9ebc..172810854 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -74,6 +74,7 @@ keep append-only from accreting stale, contradictory, or unreadably-large histor - [2026-07-12 — Merge-consent derived from state via a `## Done-when` issue checklist (#303 H6)](#2026-07-12--merge-consent-derived-from-state-via-a--done-when-issue-checklist-303-h6) - [2026-07-12 — Blocking CI gate for API-contract artifacts (#303 H4/H5)](#2026-07-12--blocking-ci-gate-for-api-contract-artifacts-303-h45) - [2026-07-12 — decisions.md is append-only, enforced; root-screenshot guard (#303 H9/H3)](#2026-07-12--decisionsmd-is-append-only-enforced-root-screenshot-guard-303-h9h3) +- [2026-07-12 — Review-verdict merge-gate: latest commit must be reviewed (#303 H10)](#2026-07-12--review-verdict-merge-gate-latest-commit-must-be-reviewed-303-h10) --- @@ -1367,3 +1368,31 @@ Companion guard **H3**: the Husky `pre-commit` hook refuses a staged **root-leve review/debug screenshot dropped at the repo root) — belt-and-suspenders with the `.gitignore` rule, so a forced `git add -f` still can't land one. Nested `*.png` (real assets) are unaffected. Rationale for both: the methodology review (#303) — make the process rules derivations/hooks, not prose to remember. + +--- + +## 2026-07-12 — Review-verdict merge-gate: latest commit must be reviewed (#303 H10) + +**A PR may not merge until a `Review-verdict:` comment on it references the PR's CURRENT head sha** — +so the *latest* commit is proven-reviewed, not a stale earlier diff. This mechanizes the ersatztv#242 +lesson ("re-review the fix commit, not just the initial PR diff": a review of an earlier revision does +not license merging a head that carries un-reviewed follow-up commits). It folds into the existing H6 +`pretooluse-merge-consent.sh` as condition (c), reusing its PR fetch, docs-only exemption, and +Gitea-auth-from-env (no second hook → no detection drift, per the #303 methodology review). + +Convention: after reviewing a PR (or its latest fix commit), post a PR comment carrying a line +`Review-verdict: @ ` (short ≥7-char or full sha). +The gate parses those lines: +- a verdict referencing the current head (`≥7`-char prefix, fixed-string match) that is + MERGEABLE/APPROVED/LGTM → **allow**; +- a verdict referencing the head but negative (BLOCKED/NOT-MERGEABLE) → **deny** (resolve + re-post); +- verdict comment(s) exist but none reference the head (only older commits) → **deny** — the + stale-review case #242 targets; +- no `Review-verdict:` comment at all → **ask** (graceful adoption, mirrors H6's "no Done-when → + ask": surface, don't hard-block a PR that hasn't adopted the convention yet); +- comments unfetchable / head sha unresolvable → **ask**. + +Scope: the Claude PreToolUse gate on the Gitea merge tool only. A direct `git push origin main` has no +PR comments to check, so the `.husky/pre-push` backstop is not extended for H10 (the merge tool is the +real merge path; docs-only PRs remain exempt via H6's file-set exemption). Rationale, as with the whole +Wave-1/2/3 hook set: make the process rule a derivation/hook, not prose to remember (#303). From f87a2092c440882c7ae7483a807fd073e58ce649 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 12 Jul 2026 19:01:51 +0200 Subject: [PATCH 2/4] =?UTF-8?q?fix(process):=20#303=20H10=20=E2=80=94=20an?= =?UTF-8?q?chor=20sha=20match=20to=20the=20@=20field;=20retraction-wi?= =?UTF-8?q?ns=20[decisions-edit]?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review found false-opens in the first cut: - `grep -F "$short"` was an unanchored substring test: a MERGEABLE verdict for a DIFFERENT/older commit was accepted whenever the head 7-prefix appeared anywhere on the line (inside a longer sha, or an unrelated commit URL). Now each verdict line's `@ ` token is extracted and matched to head by git short-sha prefix semantics (head begins with token, token >=7 chars). - No retraction semantics: a later `BLOCKED @ head` didn't override an earlier `MERGEABLE @ head`. Now a negative verdict on head wins -> deny. - A 7-digit build number falsely tripped the "references an older commit" deny; the `@`-anchored parse fixes it -> a marker with no @sha now asks, not denies. Also documents the issue-comment scope (gate reads issues/{pr}/comments, not Gitea formal-review bodies). Pipe-tested: 16 cases incl. 4 adversarial false-open reproductions, all now deny/ask. bash -n + shellcheck clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/hooks/pretooluse-merge-consent.sh | 45 +++++++++++++++++------ docs/decisions.md | 21 +++++++---- 2 files changed, 47 insertions(+), 19 deletions(-) diff --git a/.claude/hooks/pretooluse-merge-consent.sh b/.claude/hooks/pretooluse-merge-consent.sh index 21d066184..55965e4a6 100755 --- a/.claude/hooks/pretooluse-merge-consent.sh +++ b/.claude/hooks/pretooluse-merge-consent.sh @@ -118,20 +118,41 @@ verdicts=$(printf '%s' "$comments" | jq -r '.[].body // empty' 2>/dev/null | gre if [ -z "$verdicts" ]; then decide ask "H10 merge gate: no 'Review-verdict:' comment found on PR #$pr referencing head $short. Post the adversarial/Codex verdict (e.g. 'Review-verdict: MERGEABLE @ $short'), or confirm the review covered the latest commit and approve." fi -# Of those, the ones that reference the current head (>=7-char sha prefix, fixed-string). -head_verdicts=$(printf '%s\n' "$verdicts" | grep -F "$short" || true) -if [ -z "$head_verdicts" ]; then - # A verdict marker exists but none references head. Distinguish a real stale review (the line - # carries a sha-shaped token for an OLDER commit -> deny, the #242 case) from a marker with no - # sha at all (a lazy or convention-quoting comment -> ask, don't mislabel it as stale). - if printf '%s\n' "$verdicts" | grep -iqE '[0-9a-f]{7,40}'; then - decide deny "H10 merge gate: BLOCKED — a review-verdict comment references an older commit, not the current head ($short). The latest commit(s) are unreviewed (ersatztv#242: re-review the fix commit, not just the initial diff). Re-review the head and post 'Review-verdict: MERGEABLE @ $short'." - fi - decide ask "H10 merge gate: a 'Review-verdict:' comment on PR #$pr references no commit sha. Post one referencing the current head ($short) — e.g. 'Review-verdict: MERGEABLE @ $short' — or confirm the review covered the latest commit and approve." +# Classify each verdict line by the sha it references (its "@ " field) and its verdict word. +# A line references the CURRENT head iff head BEGINS WITH that sha token AND the token is >=7 chars +# (git short-sha prefix semantics) — NOT a loose substring test: an older sha that merely contains +# the head prefix, or the head prefix appearing in an unrelated URL on the line, must NOT count +# (adversarial false-opens). The verdict token must sit right after the marker on the same line. +head_pos=0; head_neg=0; stale=0; nosha=0 +while IFS= read -r line; do + [ -n "$line" ] || continue + # The sha the line references: the hex token in its "@ " field (>=7 chars), lowercased. + ref=$(printf '%s' "$line" | grep -ioE '@[[:space:]]*[0-9a-f]{7,40}' | head -1 \ + | grep -oiE '[0-9a-f]{7,40}' | tr 'A-F' 'a-f' || true) + is_pos=0 + if printf '%s' "$line" | grep -iqE 'review-verdict:[[:space:]]*(mergeable|approved|lgtm)'; then is_pos=1; fi + if [ -z "$ref" ]; then nosha=1; continue; fi + case "$sha" in + "$ref"*) if [ "$is_pos" = 1 ]; then head_pos=1; else head_neg=1; fi ;; + *) stale=1 ;; + esac +done < (c) satisfied fi +if [ "$stale" = 1 ]; then + decide deny "H10 merge gate: BLOCKED — a review-verdict comment references an older commit, not the current head ($short). The latest commit(s) are unreviewed (ersatztv#242: re-review the fix commit, not just the initial diff). Re-review the head and post 'Review-verdict: MERGEABLE @ $short'." +fi +# Marker(s) exist but reference no sha at all -> ask (don't mislabel as a stale older-commit review). +decide ask "H10 merge gate: a 'Review-verdict:' comment on PR #$pr references no commit sha. Post one referencing the current head ($short) — e.g. 'Review-verdict: MERGEABLE @ $short' — or confirm the review covered the latest commit and approve." # All derivable and satisfied -> allow. decide allow "" diff --git a/docs/decisions.md b/docs/decisions.md index 172810854..e71dbd9e9 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -1380,14 +1380,21 @@ not license merging a head that carries un-reviewed follow-up commits). It folds `pretooluse-merge-consent.sh` as condition (c), reusing its PR fetch, docs-only exemption, and Gitea-auth-from-env (no second hook → no detection drift, per the #303 methodology review). -Convention: after reviewing a PR (or its latest fix commit), post a PR comment carrying a line +Convention: after reviewing a PR (or its latest fix commit), post a PR **comment** (issue-style, not a +Gitea formal-review body — the gate reads `issues/{pr}/comments`) carrying a line `Review-verdict: @ ` (short ≥7-char or full sha). -The gate parses those lines: -- a verdict referencing the current head (`≥7`-char prefix, fixed-string match) that is - MERGEABLE/APPROVED/LGTM → **allow**; -- a verdict referencing the head but negative (BLOCKED/NOT-MERGEABLE) → **deny** (resolve + re-post); -- verdict comment(s) exist but none reference the head (only older commits) → **deny** — the - stale-review case #242 targets; +The gate classifies each verdict line by the sha in its `@ ` field, matched to the head by +**git short-sha prefix semantics** (head *begins with* the token, token ≥7 chars) — NOT a loose +substring test, so an older sha that merely contains the head prefix, or the head prefix appearing in +an unrelated URL on the line, does not count (adversarial-review false-opens, folded pre-merge): +- a MERGEABLE/APPROVED/LGTM verdict whose `@ ` is the current head → **allow**; +- a **negative** verdict (BLOCKED/NOT-MERGEABLE) on the head → **deny**, and it *wins over* a positive + one on the same head (a later BLOCKED retracts an earlier MERGEABLE; if the head were fixed the sha + would change, so this can't wrongly block); +- verdict comment(s) exist but reference only *older* commits → **deny** — the stale-review case #242 + targets; +- a `Review-verdict:` marker with **no `@ `** at all → **ask** (a lazy/quoted marker; not + mislabelled as stale); - no `Review-verdict:` comment at all → **ask** (graceful adoption, mirrors H6's "no Done-when → ask": surface, don't hard-block a PR that hasn't adopted the convention yet); - comments unfetchable / head sha unresolvable → **ask**. From ff3df39c43b5dbb37198f354fa4dd9f650de5d62 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 12 Jul 2026 19:11:05 +0200 Subject: [PATCH 3/4] =?UTF-8?q?fix(process):=20#303=20H10=20=E2=80=94=20an?= =?UTF-8?q?chor=20verdict=20marker=20to=20line-start=20(close=20self-refer?= =?UTF-8?q?ence=20false-open)=20[decisions-edit]?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second adversarial re-review (of the fix commit itself — dogfooding H10) found a remaining false-open: the `review-verdict:` marker was matched anywhere on a line, so a comment merely QUOTING the positive template — an instruction ("please post: Review-verdict: MERGEABLE @ "), a blocking comment explaining how to clear itself, or the gate's OWN suggestion text echoed into a comment — was classified as a real head verdict and self-approved the merge. Fix: anchor the marker to line-start (`^[[:space:]]*review-verdict:`). A real verdict line starts with the marker; quoted/instructional uses have text before it. Also drops the dead `nosha` var (SC2034). Finding 2 (a BLOCKED mis-anchored to an OLDER sha doesn't retract a MERGEABLE@head) is deliberately NOT "fixed": staleness is symmetric — a pre-fix BLOCKED@oldsha must not block forever after the fix changes the sha and earns a fresh MERGEABLE@head. To retract, re-review head and post BLOCKED@head. Documented in decisions.md. Pipe-tested 21 cases (5 new: instructional-quote, self-reference reason-text, line-start-in-multiline, leading-whitespace, blocking-quotes-template), all deny/ask/allow correct. bash -n + shellcheck clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/hooks/pretooluse-merge-consent.sh | 11 +++++++---- docs/decisions.md | 22 ++++++++++++++-------- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/.claude/hooks/pretooluse-merge-consent.sh b/.claude/hooks/pretooluse-merge-consent.sh index 55965e4a6..b00d58c84 100755 --- a/.claude/hooks/pretooluse-merge-consent.sh +++ b/.claude/hooks/pretooluse-merge-consent.sh @@ -113,8 +113,11 @@ comments=$(gq "repos/$owner/$repo/issues/$pr/comments?limit=100") if [ -z "$comments" ]; then decide ask "H10 merge gate: could not fetch PR #$pr comments to verify a head-referencing review verdict ($short). Confirm the adversarial/Codex review covered the latest commit before merging." fi -# Verdict lines across all comment bodies (the convention marker, case-insensitive). -verdicts=$(printf '%s' "$comments" | jq -r '.[].body // empty' 2>/dev/null | grep -iE 'review-verdict:' || true) +# Verdict lines across all comment bodies: a real verdict line STARTS with the marker (after optional +# leading whitespace). Anchoring to line-start is deliberate — it rejects a comment that merely QUOTES +# the positive template mid-sentence (an instruction "please post: Review-verdict: MERGEABLE @ ", +# or the gate's own suggestion text echoed back), which would otherwise self-approve the merge. +verdicts=$(printf '%s' "$comments" | jq -r '.[].body // empty' 2>/dev/null | grep -iE '^[[:space:]]*review-verdict:' || true) if [ -z "$verdicts" ]; then decide ask "H10 merge gate: no 'Review-verdict:' comment found on PR #$pr referencing head $short. Post the adversarial/Codex verdict (e.g. 'Review-verdict: MERGEABLE @ $short'), or confirm the review covered the latest commit and approve." fi @@ -123,7 +126,7 @@ fi # (git short-sha prefix semantics) — NOT a loose substring test: an older sha that merely contains # the head prefix, or the head prefix appearing in an unrelated URL on the line, must NOT count # (adversarial false-opens). The verdict token must sit right after the marker on the same line. -head_pos=0; head_neg=0; stale=0; nosha=0 +head_pos=0; head_neg=0; stale=0 while IFS= read -r line; do [ -n "$line" ] || continue # The sha the line references: the hex token in its "@ " field (>=7 chars), lowercased. @@ -131,7 +134,7 @@ while IFS= read -r line; do | grep -oiE '[0-9a-f]{7,40}' | tr 'A-F' 'a-f' || true) is_pos=0 if printf '%s' "$line" | grep -iqE 'review-verdict:[[:space:]]*(mergeable|approved|lgtm)'; then is_pos=1; fi - if [ -z "$ref" ]; then nosha=1; continue; fi + [ -z "$ref" ] && continue # marker present but no @ -> falls through to the final ask case "$sha" in "$ref"*) if [ "$is_pos" = 1 ]; then head_pos=1; else head_neg=1; fi ;; *) stale=1 ;; diff --git a/docs/decisions.md b/docs/decisions.md index e71dbd9e9..955f66b73 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -1381,16 +1381,22 @@ not license merging a head that carries un-reviewed follow-up commits). It folds Gitea-auth-from-env (no second hook → no detection drift, per the #303 methodology review). Convention: after reviewing a PR (or its latest fix commit), post a PR **comment** (issue-style, not a -Gitea formal-review body — the gate reads `issues/{pr}/comments`) carrying a line +Gitea formal-review body — the gate reads `issues/{pr}/comments`) whose line **starts with** the marker: `Review-verdict: @ ` (short ≥7-char or full sha). -The gate classifies each verdict line by the sha in its `@ ` field, matched to the head by -**git short-sha prefix semantics** (head *begins with* the token, token ≥7 chars) — NOT a loose -substring test, so an older sha that merely contains the head prefix, or the head prefix appearing in -an unrelated URL on the line, does not count (adversarial-review false-opens, folded pre-merge): +The gate counts a line as a verdict only when the marker is at line-start (after optional indent) — +a comment that merely *quotes* the template mid-sentence (an instruction "please post: Review-verdict: +MERGEABLE @ …", or the gate's own suggestion text echoed back) does **not** self-approve the merge +(adversarial re-review false-open, folded pre-merge). It then classifies each verdict line by the sha +in its `@ ` field, matched to the head by **git short-sha prefix semantics** (head *begins with* +the token, token ≥7 chars) — NOT a loose substring test, so an older sha that merely contains the head +prefix, or the head prefix appearing in an unrelated URL on the line, does not count: - a MERGEABLE/APPROVED/LGTM verdict whose `@ ` is the current head → **allow**; -- a **negative** verdict (BLOCKED/NOT-MERGEABLE) on the head → **deny**, and it *wins over* a positive - one on the same head (a later BLOCKED retracts an earlier MERGEABLE; if the head were fixed the sha - would change, so this can't wrongly block); +- a **negative** verdict (BLOCKED/NOT-MERGEABLE) *on the head* → **deny**, and it *wins over* a positive + one on the same head (a later BLOCKED retracts an earlier MERGEABLE; to retract, re-review head and + post BLOCKED @ head). Staleness is **symmetric on purpose**: a negative for an *older* commit is stale + exactly like a positive for an older commit, and does NOT override a fresh head-positive — otherwise a + pre-fix `BLOCKED @ oldsha` would block forever even after the fix changes the sha and earns a fresh + `MERGEABLE @ head` (the normal flow). So a genuine block must reference head, per the convention; - verdict comment(s) exist but reference only *older* commits → **deny** — the stale-review case #242 targets; - a `Review-verdict:` marker with **no `@ `** at all → **ask** (a lazy/quoted marker; not From 938733c3d07e4c38bdc46a912b1d5f1a3c34b0c1 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 12 Jul 2026 19:14:37 +0200 Subject: [PATCH 4/4] =?UTF-8?q?fix(process):=20#303=20H10=20=E2=80=94=20an?= =?UTF-8?q?chor=20is=5Fpos=20to=20the=20leading=20marker=20(monotonic=20ha?= =?UTF-8?q?rdening)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Final re-review returned SHIP-IT with one contrived, pre-existing residual: a line starting `Review-verdict: BLOCKED …` that ALSO contains a second literal `review-verdict: mergeable` substring later on the same line read as positive. Anchor the is_pos check to line-start so only the line's OWN leading verdict word counts. Safe-by-construction: anchoring a positive matcher can only REDUCE the allow-surface, so it cannot introduce a false-open (the dangerous direction); the 21-case regression confirms no false-deny (all still allow/deny/ask as before) + a new B6 case for this exact vector. No further review round needed — the change is monotonic. shellcheck clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/hooks/pretooluse-merge-consent.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.claude/hooks/pretooluse-merge-consent.sh b/.claude/hooks/pretooluse-merge-consent.sh index b00d58c84..54c08bf16 100755 --- a/.claude/hooks/pretooluse-merge-consent.sh +++ b/.claude/hooks/pretooluse-merge-consent.sh @@ -133,7 +133,9 @@ while IFS= read -r line; do ref=$(printf '%s' "$line" | grep -ioE '@[[:space:]]*[0-9a-f]{7,40}' | head -1 \ | grep -oiE '[0-9a-f]{7,40}' | tr 'A-F' 'a-f' || true) is_pos=0 - if printf '%s' "$line" | grep -iqE 'review-verdict:[[:space:]]*(mergeable|approved|lgtm)'; then is_pos=1; fi + # Positive iff the line's OWN leading verdict word (right after the line-start marker) is positive — + # anchored so a second, later `review-verdict: mergeable` substring on a BLOCKED line can't flip it. + if printf '%s' "$line" | grep -iqE '^[[:space:]]*review-verdict:[[:space:]]*(mergeable|approved|lgtm)'; then is_pos=1; fi [ -z "$ref" ] && continue # marker present but no @ -> falls through to the final ask case "$sha" in "$ref"*) if [ "$is_pos" = 1 ]; then head_pos=1; else head_neg=1; fi ;;