diff --git a/.claude/hooks/pretooluse-merge-consent.sh b/.claude/hooks/pretooluse-merge-consent.sh index 5b8b6457e..54c08bf16 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,62 @@ 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: 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 +# 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 +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 + # 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 ;; + *) 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/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..955f66b73 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,44 @@ 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** (issue-style, not a +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 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; 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 + 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**. + +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).