From dd9cd113c3d74e892b0586b264a0f53f32933681 Mon Sep 17 00:00:00 2001 From: Timothy Date: Wed, 26 Aug 2026 20:04:46 +0200 Subject: [PATCH 1/6] fix(788): one declarative H10 verdict vocabulary, derived by both sides MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The verdict words lived in two hand-written shell copies — the `case` arms of `post-review-verdict.sh` (write: word -> commit-status state) and the `POS_RE`/`NEG_RE` regexes of `check-review-verdict.sh` (read: comment -> classification for the merge-consent hook). Nothing held them together but a comment, and that comment had already gone stale once. A word the writer treats as positive but the classifier does not sends `review-verdict/h10` green while the hook still denies. #774 tried to hold them together with a parity test that scraped both vocabularies out of shell source with regexes. Six cold-review rounds each found another construction that escaped the extractor (`SHIP*)`; an unquoted `SHIP-IT) state=success`) or reddened it on a correct tree (a `) state=` in a heredoc; a column-zero `esac` in a string truncating the scoped match). It was withdrawn rather than patched a seventh time. So this removes the duplication instead of detecting it. `scripts/lib/review-verdict-vocabulary.sh` declares the two word arrays once and exposes three derived views: a membership test for the writer, an alternation body for the reader, and the operator-facing list for the "unknown verdict" message and the usage banner (both of which were themselves third copies). Neither script enumerates a word any more. Only the WORD SET moved. The grammar stays in `check-review-verdict.sh` — the column-0 anchor, the whole-word boundary, the anchored `@ ` field. Every #629 false-open was in the grammar, and none of it belongs in a word list. Validation is the security property, not hygiene. The words are interpolated into an ERE, so a `.*` in the positive list would make `POS_RE` match every verdict-shaped line and classify an explicit BLOCKED as `positive`. Words are restricted to `[a-z][a-z-]*`, and a vocabulary that is missing, unreadable, duplicated across polarities or empty is fatal — exit 2 for the reader (callers fail closed on it), exit 1 for the writer, having posted nothing. The arrays are arrays rather than whitespace-separated strings because splitting a string needs an unquoted expansion, and an unquoted expansion also globs: the first draft, handed `.*`, reported on `.claude`. That makes validation depend on the cwd and fails open — a `*` expands to the filenames beside it, and `scripts`/`docs` both match `[a-z][a-z-]*` and would have entered the vocabulary as real verdict words. Caught by the negative control, not by review. The proof is behavioural, never textual: add a word to a sandbox copy of the one declaration and require it to reach both sides. Graded MUTATION, with the clause declared in `mutation_manifest.py` — the harness restores the pre-#788 hardcoded `POS_RE` on every run and requires the proof to go red, so a refactor that quietly reintroduces a second copy cannot leave the suite green. Known residual, deliberately not fixed: `pretooluse-merge-consent.sh` names the words in one operator-facing message string. It classifies nothing (it delegates to `check-review-verdict.sh`), so it is a prose copy, not a second implementation. Verification: 980 passed / 2 skipped (`pytest scripts/tests`), ruff clean, shellcheck clean, catalog regenerated. Refs: #788 Decisions-Edit: yes Co-authored-by: Timothy --- docs/ci-cd.md | 18 ++ docs/decisions/README.md | 1 + .../release/verdict-vocabulary-shared.md | 58 ++++ docs/guard-inventory.md | 3 +- docs/remote-state-inventory.md | 1 + scripts/check-review-verdict.sh | 54 +++- scripts/lib/review-verdict-vocabulary.sh | 166 ++++++++++ scripts/post-review-verdict.sh | 62 ++-- scripts/tests/mutation_manifest.py | 16 + .../tests/test_review_verdict_vocabulary.py | 306 ++++++++++++++++++ 10 files changed, 660 insertions(+), 25 deletions(-) create mode 100644 docs/decisions/records/release/verdict-vocabulary-shared.md create mode 100644 scripts/lib/review-verdict-vocabulary.sh create mode 100644 scripts/tests/test_review_verdict_vocabulary.py diff --git a/docs/ci-cd.md b/docs/ci-cd.md index cf1b21cc0..a805f611b 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -1237,6 +1237,24 @@ hook's condition (c)) and the `review-verdict/h10` status on the same sha. `BLOC a commit landed mid-flight it writes **no** status and exits non-zero rather than retargeting your verdict at a commit you never read. +**The verdict words are declared once** in `scripts/lib/review-verdict-vocabulary.sh`, and both +sides derive from it: `post-review-verdict.sh` classifies through `etv_verdict_class`, and +`check-review-verdict.sh` builds its `POS_RE`/`NEG_RE` from `etv_verdict_alternation`. Adding a word +is a one-line edit to that file and it reaches both sides by construction (ersatztv#788, +`release.verdict-vocabulary-shared`). Only the WORD SET is shared — the grammar the classifier +applies around it (the column-0 anchor, the whole-word boundary, the anchored `@ ` field) stays +in `check-review-verdict.sh`, where every #629 false-open actually lived. Run +`scripts/post-review-verdict.sh` with no arguments to print the live vocabulary. + +Two things follow that are easy to get wrong. **Do not reintroduce a parity test** that compares the +two scripts' sources: #774 shipped one and withdrew it after six review rounds, each finding another +shell construction that escaped the extractor or reddened it on a correct tree. And a vocabulary +that is missing, unreadable or invalid is **fatal on both sides** — the reader exits 2 (which +callers treat as an unreadable input and fail closed on), the writer exits 1 having posted nothing. +That is deliberate: words are interpolated into a regex, so a `.*` slipping into the positive list +would classify an explicit `BLOCKED` as `positive`. Validation restricts every word to +`[a-z][a-z-]*`, which is why it is a hard exit rather than a warning. + The status description also records the base branch — `Review-verdict: MERGEABLE @ abc1234 (base: main)` — and the merge-consent hook denies when that no longer matches the PR's live `base.ref` (ersatztv#632). Retargeting a PR changes the effective diff without moving the head sha, so the diff --git a/docs/decisions/README.md b/docs/decisions/README.md index d44ace7e4..2021f3851 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -141,6 +141,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `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, 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_target` workflow auto-passes the two exempt classes (Renovate-authored, docs-only) unless the PR touches a protected path (`.claude/`, `.codex/`, `.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) | +| `release.verdict-vocabulary-shared` | The H10 verdict words live in exactly one place, `scripts/lib/review-verdict-vocabulary.sh`, as two arrays. `scripts/post-review-verdict.sh` (write: word to commit-status state) classifies through `etv_verdict_class`; `scripts/check-review-verdict.sh` (read: comment to classification) builds `POS_RE`/`NEG_RE` from `etv_verdict_alternation`. Neither script may restate a word, and neither may enumerate the vocabulary in a usage banner or an error string. Every word is validated to `[a-z][a-z-]*` before it reaches a regex, and a vocabulary that is missing, unreadable or invalid is FATAL on both sides — exit 1 for the writer, exit 2 for the reader. Do NOT reintroduce a text-comparison parity test alongside this. | 2026-08-26 | [link](records/release/verdict-vocabulary-shared.md) | | `release.verdict-writes-status-before-comment` | `scripts/post-review-verdict.sh` writes the sha-bound `review-verdict/h10` commit status FIRST and the human-readable `Review-verdict:` comment SECOND. Every refusal path still refuses (fail-closed, unchanged) and exits non-zero, and none of them may leave a verdict comment behind. An orphaned comment is therefore PREVENTED rather than tolerated. If the comment write fails after the status was written, that is an error too, but it degrades to an `ask` at the merge gate rather than to an apparent grant. | 2026-08-22 | [link](records/release/verdict-writes-status-before-comment.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/verdict-vocabulary-shared.md b/docs/decisions/records/release/verdict-vocabulary-shared.md new file mode 100644 index 000000000..e13e50836 --- /dev/null +++ b/docs/decisions/records/release/verdict-vocabulary-shared.md @@ -0,0 +1,58 @@ +--- +key: release.verdict-vocabulary-shared +title: '2026-08-26 — the H10 verdict vocabulary is DECLARED ONCE and both sides derive from it, rather than being held together by a detector (#788)' +status: active +since: '2026-08-26' +supersedes: none +superseded-by: none +rule: 'The H10 verdict words live in exactly one place, `scripts/lib/review-verdict-vocabulary.sh`, as two arrays. `scripts/post-review-verdict.sh` (write: word to commit-status state) classifies through `etv_verdict_class`; `scripts/check-review-verdict.sh` (read: comment to classification) builds `POS_RE`/`NEG_RE` from `etv_verdict_alternation`. Neither script may restate a word, and neither may enumerate the vocabulary in a usage banner or an error string. Every word is validated to `[a-z][a-z-]*` before it reaches a regex, and a vocabulary that is missing, unreadable or invalid is FATAL on both sides — exit 1 for the writer, exit 2 for the reader. Do NOT reintroduce a text-comparison parity test alongside this.' +signals: 'verdict vocabulary drift, POS_RE NEG_RE, two shell copies, parity test withdrawn, add a verdict word, shared vocabulary, MERGEABLE not recognised, unexplained merge-gate deny · paths: `scripts/lib/review-verdict-vocabulary.sh`, `scripts/post-review-verdict.sh`, `scripts/check-review-verdict.sh`, `scripts/tests/test_review_verdict_vocabulary.py` · issues: #788, #774, #629, #622' +mechanics: '`scripts/tests/test_review_verdict_vocabulary.py::test_a_word_added_to_the_shared_source_reaches_BOTH_sides` (and the NEGATIVE-list twin), graded MUTATION in `docs/guard-inventory.md` with its clause declared in `scripts/tests/mutation_manifest.py` — the harness restores the pre-#788 hardcoded `POS_RE` on every run and requires that proof to go red' +--- + +The vocabulary had two hand-written copies: the `case` arms of the write side and the `POS_RE`/ +`NEG_RE` regexes of the read side. A word the writer treats as positive but the classifier does not +sends `review-verdict/h10` green while the merge-consent hook still denies — an unexplained deny on +the one gate whose job is to be explicable. + +**The detector was tried first and does not converge.** #774 shipped a parity test that extracted +both vocabularies from their own shell source with regexes and compared them. Six cold-review rounds +each found another shell construction that either escaped the extractor (`SHIP*)`, a glob in the arm +label; an unquoted `SHIP-IT) state=success`) or reddened it on a correct tree (a `) state=` +inside a heredoc; a column-zero `esac` in a string truncating the scoped match). Each fix was locally +correct and the sequence converged on nothing, because a regex over shell source is not a shell +parser. It was withdrawn rather than patched a seventh time. That is why the rule above bans +reintroducing one: the ban is the conclusion of the six rounds, not caution. + +So the duplication is REMOVED rather than detected. One declaration, two derived views — a +membership test for the writer, an alternation body for the reader. The proof is correspondingly +behavioural: add a word to the sandbox copy of the declaration and require it to reach both sides, +which is only possible if both derive. + +**The split between WORD SET and GRAMMAR is deliberate and load-bearing.** Only the word list moved. +Every false-open in the classifier's history was in the grammar — the column-0 anchor, the +whole-word boundary, the anchored `@ ` field (#629) — and none of that belongs in a word list. +`etv_verdict_alternation` emits an alternation body and nothing else; the anchors and the +`([[:space:]@]|$)` boundary stay in `check-review-verdict.sh`, unchanged. + +**Validation is not hygiene, it is the security property.** The list is interpolated into an +extended regular expression, so it is an injection surface, and the failure direction is the +dangerous one: a `.*` in the positive list would make `POS_RE` match every verdict-shaped line, so +an explicit `BLOCKED` would classify `positive` and the gate would grant consent nobody gave. +Restricting words to `[a-z][a-z-]*` makes that unreachable as a property of the character class +rather than of anyone remembering to escape. A word appearing in both lists, an empty list and an +empty word are refused for the same reason: each would make the classification of some real verdict +depend on which arm ran first. + +**The arrays are arrays, not whitespace-separated strings.** A string list has to be split by an +UNQUOTED expansion, and an unquoted expansion also performs PATHNAME EXPANSION. That is not +theoretical: the first draft of the validator, handed `.*`, reported on `.claude` — a file in the +repo root. It makes validation depend on the working directory, and it fails OPEN in the case that +matters, since a `*` entry expands to the filenames beside it and any matching `[a-z][a-z-]*` +(`scripts`, `docs`) would validate cleanly and enter the vocabulary as a real verdict word. + +**What is deliberately NOT shared.** `.claude/hooks/pretooluse-merge-consent.sh` names the words in +one operator-facing message string. It performs no classification of its own — it delegates to +`check-review-verdict.sh` — so it is a prose copy, not a second implementation, and sourcing a shell +library into a Claude hook to fix an error message would buy nothing for the risk. It is a known +residual, not an oversight. diff --git a/docs/guard-inventory.md b/docs/guard-inventory.md index a4e510827..16d7ae765 100644 --- a/docs/guard-inventory.md +++ b/docs/guard-inventory.md @@ -218,6 +218,7 @@ recorded as unexamined rather than as cleared. | `scripts/tests/test_prepush_rebase_check_tag_exemption.py` | the `script-tests` job | PROOF | NONE | — | | `scripts/tests/test_optional_request_members.py` | the `script-tests` job, on an OpenAPI request schema that can silently drop a member with no stated disposition | GUARD | MUTATION | `test_optional_request_members.py::test_every_droppable_request_schema_has_a_stated_disposition` | | `scripts/tests/test_prove_fix.py` | the `script-tests` job | PROOF | NONE | — | +| `scripts/tests/test_review_verdict_vocabulary.py` | the `script-tests` job, when the H10 verdict vocabulary stops being shared — a word added to the one declaration that fails to reach the write side or the read side | GUARD | MUTATION | `test_review_verdict_vocabulary.py::test_a_word_added_to_the_shared_source_reaches_BOTH_sides` | | `scripts/tests/test_remote_state_inventory.py` | the `script-tests` job, on an executable that talks to a remote service with no row in `docs/remote-state-inventory.md` | GUARD | MUTATION | `test_remote_state_inventory.py::test_every_in_scope_file_has_a_row_and_every_row_names_a_real_file` | | `scripts/tests/test_worktree_ownership_guard.py` | the `script-tests` job | PROOF | NONE | — | @@ -241,7 +242,7 @@ clause-provable and the entry is regraded. ## What the numbers say -38 guards, 6 tooling scripts, 20 proof files. **16 guards carry a mutation proof; 6 are +39 guards, 6 tooling scripts, 20 proof files. **17 guards carry a mutation proof; 6 are behaviour-only; 16 have none.** These figures are asserted against the table by `test_the_summary_counts_match_the_table` — they were wrong in the first draft (28/4/6/3/19 against a table holding 27/5/6/3/18), because a hand-maintained summary of a table is a second copy of it, diff --git a/docs/remote-state-inventory.md b/docs/remote-state-inventory.md index ca95a36c9..482e21a47 100644 --- a/docs/remote-state-inventory.md +++ b/docs/remote-state-inventory.md @@ -109,6 +109,7 @@ classifications differ; otherwise the strictest applies and the Note names the e | `scripts/ci-detect-docs-only.sh` — `git fetch origin "$base"`, then diffs the fetched tip against HEAD to emit `docs_only`, which gates whether the required test/migrations jobs run their real steps | `UNSAFE-KNOWN` | Read-then-act with no re-check between the fetch and the emitted value, and the decision genuinely gates required CI work. Accepted because the script is deliberately asymmetric: every ambiguous, undeterminable or shallow-checkout case resolves to `docs_only=false` (run everything), and only an exact unanimous all-docs diff yields `true` — so a stale or racing base read can at worst cause an unnecessary full run, never a skipped one (#416). | | `scripts/refresh-shared-checkout.sh` — `git fetch origin main`, then `git merge --ff-only origin/main` and a conditional `npm ci` | `UNSAFE-KNOWN` | Fetch-then-act with no re-check between the fetch and the merge. Accepted because every action is self-refusing or reversible: `--ff-only` fails harmlessly rather than diverging if the ref moved on, the script refuses outright when the tree is not clean `main` or is ahead or mid-rebase, and this is a developer-convenience checkout rather than a release or merge-authorization path — a stale read costs one extra fetch next run, never lost work. | +| `scripts/lib/review-verdict-vocabulary.sh` | `N/A` | Reads nothing at all — a sourced declaration of the H10 verdict word list plus pure functions over it; it makes no call, local or remote. Listed because it is in this inventory's scope (`scripts/**/*.sh`), not because it has a remote-state shape. | | `scripts/check-review-verdict.sh` | `N/A` | Reads no live remote state itself — classifies a comments JSON payload supplied on stdin; the fetch belongs to the caller's row. | | `scripts/decisions_validate.py` | `N/A` | Reads no live remote state — its `git log`/`show`/`ls-tree`/`merge-base` calls operate on refs the caller already checked out or passed via `--base`/`--head`, never a fetch. | | `scripts/prove-fix.sh` | `N/A` | Reads no live remote state — `git worktree add`/`rev-parse`/`diff-tree` operate on the local repository's own objects. | diff --git a/scripts/check-review-verdict.sh b/scripts/check-review-verdict.sh index 2b930afb1..bc062fad3 100755 --- a/scripts/check-review-verdict.sh +++ b/scripts/check-review-verdict.sh @@ -12,8 +12,10 @@ # an unreadable input is never a pass). # # Classifications, in the order they are decided: -# negative a verdict on the CURRENT head is BLOCKED/NOT-MERGEABLE -> block -# positive a verdict on the CURRENT head is MERGEABLE/APPROVED/LGTM -> allow +# negative a verdict on the CURRENT head is a NEGATIVE word -> block +# positive a verdict on the CURRENT head is a POSITIVE word -> allow +# (the words themselves: scripts/lib/review-verdict-vocabulary.sh, shared with the +# write side so the two cannot drift — ersatztv#788) # stale verdict(s) exist but reference only OLDER commits -> block (the #242 case) # unknown a verdict line uses a token in neither vocabulary -> undecidable, surface it # no-sha a `Review-verdict:` marker carries no `@ ` field -> undecidable @@ -66,6 +68,35 @@ case "$head" in *[!0-9a-f]*|"") printf 'check-review-verdict: --head is not a hex sha: %s\n' "$head" >&2; exit 2 ;; esac +# --- The shared verdict vocabulary (ersatztv#788). ---------------------------------------------- +# This script (the READ side) and scripts/post-review-verdict.sh (the WRITE side) used to carry two +# hand-written copies of the word list — `POS_RE`/`NEG_RE` here, `case` arms there — held together +# by nothing but a comment. They now derive from one declaration, so a word added on one side +# cannot fail to reach the other. +# +# Resolved relative to THIS FILE, not the caller's cwd: the merge-consent hook invokes this script +# by absolute path (`${CLAUDE_PROJECT_DIR:-.}/scripts/check-review-verdict.sh`) from whatever +# directory the tool call happened in, so a cwd-relative lookup would find the vocabulary only by +# luck. +# +# Every failure below is exit 2, the script's declared input-error code, because callers MUST fail +# closed on it (see the header). A classifier that cannot load its vocabulary does not know what a +# verdict word is; the one thing it must never do is fall through to `absent`, which the hook reads +# as "convention not adopted" and downgrades into an `ask`. +_here=$(CDPATH='' cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd) || { + printf 'check-review-verdict: could not resolve this script own directory to locate the shared verdict vocabulary\n' >&2; exit 2; } +VOCABULARY_LIB="$_here/lib/review-verdict-vocabulary.sh" +if [ ! -r "$VOCABULARY_LIB" ]; then + printf 'check-review-verdict: the shared verdict vocabulary is missing or unreadable at %s\n' "$VOCABULARY_LIB" >&2; exit 2 +fi +# shellcheck source=lib/review-verdict-vocabulary.sh +if ! . "$VOCABULARY_LIB"; then + printf 'check-review-verdict: the shared verdict vocabulary at %s could not be sourced\n' "$VOCABULARY_LIB" >&2; exit 2 +fi +if ! etv_verdict_vocabulary_validate; then + printf 'check-review-verdict: the shared verdict vocabulary at %s did not validate (see above)\n' "$VOCABULARY_LIB" >&2; exit 2 +fi + comments=$(cat) [ -n "$comments" ] || { printf 'check-review-verdict: empty comments payload on stdin\n' >&2; exit 2; } @@ -181,8 +212,23 @@ fi # 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='^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:]@]|$)' + +# The WORD SET comes from the shared vocabulary; the GRAMMAR around it stays here (ersatztv#788). +# That split is the point: every false-open in this file's history was in the grammar — the column-0 +# anchor, the whole-word boundary, the anchored `@ ` field — and none of it belongs in a word +# list. `etv_verdict_alternation` emits only the alternation BODY (`word|word|word`), and the +# `([[:space:]@]|$)` boundary below are unchanged. +# +# Interpolating into an ERE is safe because `etv_verdict_vocabulary_validate` (run above, fatal) +# restricts every word to `[a-z][a-z-]*`, so no character reaching this regex can be a metacharacter. +# Without that check a `.*` in the positive list would make every verdict-shaped line classify +# `positive`, including an explicit BLOCKED — which is why the validation is a hard exit, not a warn. +POS_ALTERNATION=$(etv_verdict_alternation positive) || { + printf 'check-review-verdict: could not build the positive verdict pattern from %s\n' "$VOCABULARY_LIB" >&2; exit 2; } +NEG_ALTERNATION=$(etv_verdict_alternation negative) || { + printf 'check-review-verdict: could not build the negative verdict pattern from %s\n' "$VOCABULARY_LIB" >&2; exit 2; } +POS_RE='^review-verdict:[[:space:]]*('"$POS_ALTERNATION"')([[:space:]@]|$)' +NEG_RE='^review-verdict:[[:space:]]*('"$NEG_ALTERNATION"')([[:space:]@]|$)' head_pos=0; head_neg=0; stale=0; unknown=0 while IFS= read -r line; do diff --git a/scripts/lib/review-verdict-vocabulary.sh b/scripts/lib/review-verdict-vocabulary.sh new file mode 100644 index 000000000..4cf879d1c --- /dev/null +++ b/scripts/lib/review-verdict-vocabulary.sh @@ -0,0 +1,166 @@ +# shellcheck shell=bash +# review-verdict-vocabulary.sh — THE H10 verdict vocabulary, declared ONCE (ersatztv#788). +# +# SOURCED, NEVER EXECUTED. It defines data plus pure functions and takes no action of its own, so it +# is deliberately not executable and carries no shebang. +# +# WHY THIS FILE EXISTS. The vocabulary used to live in two hand-written shell copies: the `case` +# arms of `scripts/post-review-verdict.sh` (the WRITE side, which turns a word into a commit-status +# state) and the `POS_RE`/`NEG_RE` regexes of `scripts/check-review-verdict.sh` (the READ side, +# which classifies a `Review-verdict:` comment for the merge-consent hook). A word the write side +# treats as positive but the read side does not sends the server-side status green while the hook +# still denies — an unexplained deny on a gate whose whole job is to be explicable. +# +# WHY IT IS NOT A TEST. ersatztv#774 tried to hold the two copies together with a parity test that +# extracted both vocabularies from their own shell source with regexes and compared them. Six cold +# review rounds each found another shell construction that either escaped the extractor (`SHIP*)`, a +# glob in the arm label; an unquoted `SHIP-IT) state=success`) or reddened it on a correct tree (a +# `) state=` inside a heredoc; a column-zero `esac` in a string truncating the scoped match). +# Each fix was locally correct and the sequence converged on nothing, because a regex over shell +# source is not a shell parser. It was withdrawn rather than patched a seventh time — see +# `docs/decisions/records/testing/guard-derives-population-from-source.md`, which carries it as the +# worked example of "a weak detector is itself the symptom-keyed mistake". +# +# So this file removes the duplication rather than detecting it. There is one list; both sides +# DERIVE from it. Do not reintroduce a text-comparison test alongside — the lesson of those six +# rounds is that the shape does not converge, and dedup by construction is what closes it. +# +# ADDING A WORD is a one-line edit to the list below, and it reaches both sides by construction. +# `scripts/tests/test_review_verdict_vocabulary.py` proves exactly that, by adding a word to a +# sandbox copy of THIS file and driving both real scripts. + +# --- The vocabulary. Whitespace-separated, lower case. ------------------------------------------- +# Positive: the merge may proceed -> commit status `success`. +# Negative: the merge stays blocked -> commit status `failure`. +# A word in NEITHER list is `unknown` on the read side and a hard refusal on the write side. That is +# deliberate and must stay: a token nobody recognises is surfaced for a human, never guessed at. +# +# DECLARED AS ARRAYS, not as whitespace-separated strings, and every expansion below is quoted +# (`"${arr[@]}"`). A string list has to be split by an UNQUOTED expansion, and an unquoted expansion +# also performs PATHNAME EXPANSION: with `.*` in the list the validator below reported on `.claude`, +# a file in the repo root, rather than on the word actually written. That makes validation depend on +# the working directory, and it fails OPEN in the case that matters — a `*` entry expands to the +# filenames around it, and any of those matching `[a-z][a-z-]*` (`scripts`, `docs`) would validate +# cleanly and enter the vocabulary as a real verdict word. Arrays remove the expansion rather than +# guarding it. +ETV_VERDICT_POSITIVE_WORDS=(mergeable approved lgtm) +ETV_VERDICT_NEGATIVE_WORDS=(blocked not-mergeable) + +# --- Validation. ------------------------------------------------------------------------------- +# The read side interpolates these words into an EXTENDED REGULAR EXPRESSION. That makes the list an +# injection surface, and the failure direction is the dangerous one: a stray `.*` in the positive +# list would make POS_RE match every verdict-shaped line, so an explicit `BLOCKED` would classify as +# `positive` and the merge gate would grant consent it was never given. Restricting words to +# `[a-z][a-z-]*` means no character reaching the regex can be a metacharacter, which is a property of +# the character class rather than of anyone remembering to escape. +# +# Returns 0 when the vocabulary is usable, 1 otherwise (message on stderr). It never exits: the two +# consumers fail closed with DIFFERENT codes (`post` dies 1, `check` exits 2 so its callers treat it +# as an unreadable input), so the exit semantics belong to them, not here. +etv_verdict_vocabulary_validate() { + local word seen=' ' + + # `${#arr[@]}` is safe on an unset/empty array even under `set -u`, which `"${arr[@]}"` is not on + # bash 3.2 — so the count is checked BEFORE anything expands the arrays. + if [ "${#ETV_VERDICT_POSITIVE_WORDS[@]}" -eq 0 ] || [ "${#ETV_VERDICT_NEGATIVE_WORDS[@]}" -eq 0 ]; then + printf 'review-verdict vocabulary: the positive and negative lists must both be non-empty\n' >&2 + return 1 + fi + + for word in "${ETV_VERDICT_POSITIVE_WORDS[@]}" "${ETV_VERDICT_NEGATIVE_WORDS[@]}"; do + # Rejects upper case, digits, whitespace and every regex metacharacter, plus a leading or + # trailing hyphen (which would build an alternation branch that reads oddly and matches nothing + # useful). `-` is last in the bracket expression, where it is literal. + case "$word" in + '' | *[!a-z-]* | -* | *-) + printf 'review-verdict vocabulary: %s is not a usable verdict word (allowed: [a-z] and internal -)\n' "$word" >&2 + return 1 + ;; + esac + # A word in BOTH lists would classify as whichever side is consulted first — an ambiguity that + # would silently pick a side. Reject it here instead. + case "$seen" in + *" $word "*) + printf 'review-verdict vocabulary: %s appears twice; a word must be positive or negative, not both\n' "$word" >&2 + return 1 + ;; + esac + seen="$seen$word " + done + + return 0 +} + +# --- Derived view 1: classify one token (the WRITE side). --------------------------------------- +# Prints `positive` or `negative` and returns 0; returns 1 for a word in neither list, printing +# nothing. Case-insensitive, so the caller may pass the operator's raw argument. +etv_verdict_class() { + local token word + token=$(printf '%s' "${1-}" | tr '[:upper:]' '[:lower:]') + + for word in ${ETV_VERDICT_POSITIVE_WORDS[@]+"${ETV_VERDICT_POSITIVE_WORDS[@]}"}; do + if [ "$word" = "$token" ]; then + printf 'positive\n' + return 0 + fi + done + for word in ${ETV_VERDICT_NEGATIVE_WORDS[@]+"${ETV_VERDICT_NEGATIVE_WORDS[@]}"}; do + if [ "$word" = "$token" ]; then + printf 'negative\n' + return 0 + fi + done + + return 1 +} + +# --- Derived view 2: an ERE alternation body (the READ side). ----------------------------------- +# `etv_verdict_alternation positive` -> `mergeable|approved|lgtm`. The caller wraps it in its own +# anchors and boundary, so this deliberately emits no parentheses: the grammar around the token +# belongs to the classifier, only the WORD SET belongs here. +etv_verdict_alternation() { + local alternation='' word + local words=() + + case "${1-}" in + positive) words=(${ETV_VERDICT_POSITIVE_WORDS[@]+"${ETV_VERDICT_POSITIVE_WORDS[@]}"}) ;; + negative) words=(${ETV_VERDICT_NEGATIVE_WORDS[@]+"${ETV_VERDICT_NEGATIVE_WORDS[@]}"}) ;; + *) + printf 'review-verdict vocabulary: etv_verdict_alternation needs positive|negative, got %s\n' "${1-}" >&2 + return 1 + ;; + esac + + for word in ${words[@]+"${words[@]}"}; do + if [ -z "$alternation" ]; then alternation="$word"; else alternation="$alternation|$word"; fi + done + + [ -n "$alternation" ] || return 1 + printf '%s\n' "$alternation" +} + +# --- Derived view 3: the operator-facing list. -------------------------------------------------- +# `MERGEABLE, APPROVED, LGTM, BLOCKED or NOT-MERGEABLE`. Derived rather than written out, because +# the "unknown verdict" error message naming a stale set is how an operator learns the wrong +# vocabulary — the same drift this file exists to remove, one layer out. +etv_verdict_words_display() { + local word index=0 total out='' + + set -- ${ETV_VERDICT_POSITIVE_WORDS[@]+"${ETV_VERDICT_POSITIVE_WORDS[@]}"} \ + ${ETV_VERDICT_NEGATIVE_WORDS[@]+"${ETV_VERDICT_NEGATIVE_WORDS[@]}"} + total=$# + + for word in "$@"; do + index=$((index + 1)) + word=$(printf '%s' "$word" | tr '[:lower:]' '[:upper:]') + if [ "$index" -eq 1 ]; then + out="$word" + elif [ "$index" -eq "$total" ]; then + out="$out or $word" + else + out="$out, $word" + fi + done + + printf '%s\n' "$out" +} diff --git a/scripts/post-review-verdict.sh b/scripts/post-review-verdict.sh index d6cc0ddea..86115a8a3 100755 --- a/scripts/post-review-verdict.sh +++ b/scripts/post-review-verdict.sh @@ -27,8 +27,11 @@ # Usage: # scripts/post-review-verdict.sh [note ...] # -# MERGEABLE | APPROVED | LGTM -> commit status `success` (merge may proceed) -# BLOCKED | NOT-MERGEABLE -> commit status `failure` (merge stays blocked) +# a POSITIVE word -> commit status `success` (merge may proceed) +# a NEGATIVE word -> commit status `failure` (merge stays blocked) +# The words are declared ONCE in scripts/lib/review-verdict-vocabulary.sh and are +# printed live by running this script with no arguments — deliberately not restated +# here, because a hand-kept list in a usage banner is the same drift, one layer out. # [note] optional free text appended to the comment under the verdict line. # # Env: ETV_GITEA_TOKEN (token) or ETV_GITEA_BASICAUTH (user:pass) — required. @@ -40,8 +43,33 @@ STATUS_CONTEXT="review-verdict/h10" die() { printf 'post-review-verdict: %s\n' "$*" >&2; exit 1; } +# --- The shared verdict vocabulary (ersatztv#788). ---------------------------------------------- +# This script (the WRITE side) and scripts/check-review-verdict.sh (the READ side) used to carry two +# hand-written copies of the word list, held together by nothing but a comment. They now derive from +# one declaration. Resolved relative to THIS FILE, never to the caller's cwd: the merge-consent hook +# invokes the read side by absolute path from an arbitrary directory, and both scripts must behave +# the same way about where their vocabulary comes from. +# +# Fails CLOSED on every branch. A vocabulary that cannot be loaded or does not validate means this +# script cannot know whether a word is positive, and posting a `success` it cannot justify is the +# one outcome the H10 gate exists to prevent. +_here=$(CDPATH='' cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd) \ + || die "could not resolve this script's own directory, so the shared verdict vocabulary cannot be located" +VOCABULARY_LIB="$_here/lib/review-verdict-vocabulary.sh" +[ -r "$VOCABULARY_LIB" ] \ + || die "the shared verdict vocabulary is missing or unreadable at $VOCABULARY_LIB — no status was written" +# shellcheck source=lib/review-verdict-vocabulary.sh +. "$VOCABULARY_LIB" \ + || die "the shared verdict vocabulary at $VOCABULARY_LIB could not be sourced — no status was written" +etv_verdict_vocabulary_validate \ + || die "the shared verdict vocabulary at $VOCABULARY_LIB did not validate (see above) — no status was written" + usage() { sed -n '/^# Usage:/,/^# ETV_GITEA_REPO/p' "$0" | sed 's/^# \{0,1\}//' + printf '\nVerdict words (from %s):\n positive -> success : %s\n negative -> failure : %s\n' \ + "lib/review-verdict-vocabulary.sh" \ + "$(etv_verdict_alternation positive | tr '|' ' ')" \ + "$(etv_verdict_alternation negative | tr '|' ' ')" exit 2 } @@ -51,26 +79,20 @@ note="$*" printf '%s' "$pr" | grep -qE '^[0-9]+$' || die "pull-request number must be numeric, got '$pr'" -# Normalize to upper-case and classify. The positive set MUST stay in sync with the READ side's -# POS_RE/NEG_RE in scripts/check-review-verdict.sh (mergeable|approved|lgtm) — a word this script -# treats as positive but the classifier does not would let the server-side status go green while -# the merge-consent hook still denies, which reads as an unexplained deny. +# Normalize to upper-case for display, and classify against the SHARED vocabulary sourced above. +# There is no word list here any more: `etv_verdict_class` reads the same declaration that builds +# the read side's POS_RE/NEG_RE, so a word this script accepts as positive is positive over there by +# construction rather than by two people editing two files in step (ersatztv#788). # -# That sync is NOT asserted anywhere, and you should not assume otherwise. ersatztv#774 tried: a -# test extracted both vocabularies from their own shell source and compared them. Six cold-review -# rounds each found another shell construction that either escaped the extractor or made it fail on -# a correct tree, so it was withdrawn rather than patched a seventh time. Writing a shell parser as -# a regex does not converge. -# -# The fix is to stop having two copies — one declarative vocabulary both scripts read — tracked in -# ersatztv#788. Until that lands, THIS COMMENT IS THE ONLY THING holding the two lists together, and -# comments drift: this one already had, naming .claude/hooks/pretooluse-merge-consent.sh as the home -# of the regex. The hook carries no copy; it delegates to check-review-verdict.sh. +# The unknown-word message is derived too. A hand-written "use , , ..." list is how an +# operator learns a vocabulary that has moved on — the same drift, displaced into an error string. verdict=$(printf '%s' "$verdict_raw" | tr '[:lower:]' '[:upper:]') -case "$verdict" in - MERGEABLE|APPROVED|LGTM) state="success" ;; - BLOCKED|NOT-MERGEABLE) state="failure" ;; - *) die "unknown verdict '$verdict_raw' — use MERGEABLE, APPROVED, LGTM, BLOCKED or NOT-MERGEABLE" ;; +verdict_class=$(etv_verdict_class "$verdict") \ + || die "unknown verdict '$verdict_raw' — use $(etv_verdict_words_display)" +case "$verdict_class" in + positive) state="success" ;; + negative) state="failure" ;; + *) die "the shared vocabulary classified '$verdict' as '$verdict_class', which is neither positive nor negative — refusing to guess a commit-status state" ;; esac base_url="${ETV_GITEA_URL:-http://192.168.1.95:3000}/api/v1" diff --git a/scripts/tests/mutation_manifest.py b/scripts/tests/mutation_manifest.py index 31c2b0eb4..0c5509980 100644 --- a/scripts/tests/mutation_manifest.py +++ b/scripts/tests/mutation_manifest.py @@ -246,6 +246,22 @@ MUTATIONS: tuple[Mutation, ...] = ( "real outage down the could-not-verify path, which sends an operator to the registry's " "health rather than to the rebuild that fixes it.", ), + Mutation( + guard="scripts/tests/test_review_verdict_vocabulary.py", + target="scripts/check-review-verdict.sh", + clause="""POS_RE='^review-verdict:[[:space:]]*('"$POS_ALTERNATION"')([[:space:]@]|$)'""", + replacement="""POS_RE='^review-verdict:[[:space:]]*(mergeable|approved|lgtm)([[:space:]@]|$)'""", + proof="test_review_verdict_vocabulary.py::test_a_word_added_to_the_shared_source_reaches_BOTH_sides", + granularity=CLAUSE, + expect="restating the word list rather than deriving it", + why="THE GUARD IS A TEST, so the mutation goes into the guarded ARTIFACT — the read side " + "whose derivation it watches — rather than into the checker, per the checker-guard " + "exception. The replacement is not an invented mutant: it is the LITERAL pre-#788 line, the " + "second hand-written copy this change removed, so the proof is taken against the real " + "predecessor. Mutating the shared vocabulary instead would be the trap that looks " + "identical: emptying or corrupting the word list reddens the proof through the fail-closed " + "validator, which says nothing about whether the read side still DERIVES from it.", + ), ) diff --git a/scripts/tests/test_review_verdict_vocabulary.py b/scripts/tests/test_review_verdict_vocabulary.py new file mode 100644 index 000000000..b923583fe --- /dev/null +++ b/scripts/tests/test_review_verdict_vocabulary.py @@ -0,0 +1,306 @@ +"""The H10 verdict vocabulary is declared ONCE and both sides derive from it (ersatztv#788). + +`scripts/post-review-verdict.sh` (WRITE: word -> commit-status state) and +`scripts/check-review-verdict.sh` (READ: comment -> classification for the merge-consent hook) used +to carry two hand-written copies of the word list. A word the write side treats as positive but the +read side does not sends `review-verdict/h10` green while the hook still denies. + +WHAT THIS FILE DELIBERATELY DOES NOT DO. It does not compare the two scripts' sources, and nothing +here scrapes a vocabulary out of shell text. ersatztv#774 shipped exactly that and withdrew it after +six cold-review rounds, each finding another shell construction that escaped the extractor or +reddened it on a correct tree; a regex over shell source is not a shell parser and the sequence +converged on nothing. See `docs/decisions/records/testing/guard-derives-population-from-source.md`. + +So the proof here is BEHAVIOURAL and runs against the real scripts: edit the one declaration in a +sandbox copy and require the new word to reach both sides, which is only possible if both derive. +The negative half is declared in `scripts/tests/mutation_manifest.py` and executed by the clause +mutation harness on every run: it puts the pre-#788 hardcoded `POS_RE` back into the read side and +requires `test_a_word_added_to_the_shared_source_reaches_BOTH_sides` to go RED. So a future refactor +that quietly reintroduces a second copy cannot leave this suite green. +""" + +from __future__ import annotations + +import json +import os +import shutil +import subprocess +from pathlib import Path + +import pytest + +from scripts.tests.test_post_review_verdict import CURL_SHIM, SHA_A + +REPO_ROOT = Path(__file__).resolve().parents[2] +VOCABULARY = REPO_ROOT / "scripts" / "lib" / "review-verdict-vocabulary.sh" + +# The declaration line the sandbox rewrites. Named once here so a rename of the array is a single +# edit and an obvious red, rather than four silently-not-matching `str.replace` calls. +POSITIVE_DECL = "ETV_VERDICT_POSITIVE_WORDS=(mergeable approved lgtm)" +NEGATIVE_DECL = "ETV_VERDICT_NEGATIVE_WORDS=(blocked not-mergeable)" + + +class Sandbox: + """A writable copy of `scripts/`, so a proof may edit the vocabulary without touching the repo. + + The whole directory is copied rather than the three files of interest, because both scripts + resolve the vocabulary RELATIVE TO THEMSELVES (`dirname "${BASH_SOURCE[0]}"/lib/...`). Copying + them individually would leave them sourcing the repo's real vocabulary and every mutation below + would silently prove nothing. + """ + + def __init__(self, root: Path): + self.scripts = root / "scripts" + shutil.copytree(REPO_ROOT / "scripts", self.scripts) + self.vocabulary = self.scripts / "lib" / "review-verdict-vocabulary.sh" + self.reader = self.scripts / "check-review-verdict.sh" + self.writer = self.scripts / "post-review-verdict.sh" + + bindir = root / "bin" + bindir.mkdir() + shim = bindir / "curl" + shim.write_text(CURL_SHIM) + shim.chmod(0o755) + + self.state = root / "state" + self.state.mkdir() + (self.state / "pr_shas").write_text(SHA_A) + (self.state / "pr_bases").write_text("main") + (self.state / "pr_state").write_text("open") + + self.env = dict(os.environ) + self.env["PATH"] = f"{bindir}{os.pathsep}{self.env['PATH']}" + self.env["STUB_DIR"] = str(self.state) + self.env["ETV_GITEA_TOKEN"] = "stub-token" # noqa: S105 - fake; real creds come from env + self.env["ETV_GITEA_URL"] = "http://gitea.example" + self.env["ETV_GITEA_REPO"] = "timothy/ersatztv" + self.env.pop("ETV_GITEA_BASICAUTH", None) + + # --- editing the one declaration ----------------------------------------------------------- + def edit_vocabulary(self, old: str, new: str) -> None: + text = self.vocabulary.read_text() + assert text.count(old) == 1, f"expected exactly one {old!r} in the shared vocabulary" + self.vocabulary.write_text(text.replace(old, new)) + + # --- driving the two real scripts ---------------------------------------------------------- + def read_side(self, token: str, head: str = SHA_A) -> subprocess.CompletedProcess[str]: + payload = json.dumps([{"body": f"Review-verdict: {token} @ {head}"}]) + return subprocess.run( + ["bash", str(self.reader), "--head", head], + input=payload, + env=self.env, + capture_output=True, + text=True, + ) + + def write_side(self, token: str) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["bash", str(self.writer), "42", token], + env=self.env, + capture_output=True, + text=True, + ) + + def posted_status_state(self) -> str | None: + log = self.state / "posts.jsonl" + if not log.exists(): + return None + for line in log.read_text().splitlines(): + if not line.strip(): + continue + post = json.loads(line) + if "/statuses/" in post["url"]: + return post["payload"]["state"] + return None + + +@pytest.fixture +def sandbox(tmp_path): + return Sandbox(tmp_path) + + +# --- The load-bearing proof: one declaration, both sides. --------------------------------------- + + +def test_a_word_added_to_the_shared_source_reaches_BOTH_sides(sandbox): + """The Done-when of #788, asserted as behaviour rather than as textual agreement. + + `shipit` is in neither list to begin with, so the two `before` assertions are a real negative + control: they establish that the word is genuinely unknown to both sides, which is what makes + the `after` assertions evidence of derivation rather than of a word that already worked. + """ + assert sandbox.read_side("SHIPIT").stdout.strip() == "unknown" + before = sandbox.write_side("SHIPIT") + assert before.returncode != 0, "an unknown word must be refused by the write side" + assert "unknown verdict" in before.stderr + + # ONE line, in ONE file. + sandbox.edit_vocabulary(POSITIVE_DECL, POSITIVE_DECL[:-1] + " shipit)") + + assert sandbox.read_side("SHIPIT").stdout.strip() == "positive", ( + "the read side did not pick up a word added to the shared vocabulary — it is restating the " + "word list rather than deriving it" + ) + after = sandbox.write_side("SHIPIT") + assert after.returncode == 0, f"the write side still refuses the new word: {after.stderr}" + assert sandbox.posted_status_state() == "success", ( + "the write side did not map the newly-added POSITIVE word onto a success status" + ) + + +def test_a_word_added_to_the_NEGATIVE_list_reaches_both_sides_as_a_block(sandbox): + """The positive list proving derivation would not prove the negative list does too. + + Both are separate arrays and separate call sites; #685 is on record here for two guards on one + condition where deleting either left the suite green. + """ + assert sandbox.read_side("VETOED").stdout.strip() == "unknown" + + sandbox.edit_vocabulary(NEGATIVE_DECL, NEGATIVE_DECL[:-1] + " vetoed)") + + assert sandbox.read_side("VETOED").stdout.strip() == "negative" + assert sandbox.write_side("VETOED").returncode == 0 + assert sandbox.posted_status_state() == "failure", ( + "a newly-added NEGATIVE word must block, not merely be recognised" + ) + + +# --- Fail-closed: a vocabulary that cannot be loaded is never a pass. --------------------------- + + +def test_a_MISSING_vocabulary_makes_the_read_side_fail_closed(sandbox): + sandbox.vocabulary.unlink() + result = sandbox.read_side("MERGEABLE") + assert result.returncode == 2, "callers fail closed on exit 2; anything else is a silent pass" + assert result.stdout.strip() != "positive" + assert "vocabulary" in result.stderr.lower() + + +def test_a_MISSING_vocabulary_makes_the_write_side_refuse_to_post(sandbox): + sandbox.vocabulary.unlink() + result = sandbox.write_side("MERGEABLE") + assert result.returncode != 0 + assert sandbox.posted_status_state() is None, "no status may be written without a vocabulary" + + +def test_an_UNREADABLE_vocabulary_is_refused_rather_than_skipped(sandbox): + sandbox.vocabulary.chmod(0o000) + try: + assert sandbox.read_side("MERGEABLE").returncode == 2 + assert sandbox.write_side("MERGEABLE").returncode != 0 + finally: + sandbox.vocabulary.chmod(0o644) + + +# --- The list is interpolated into an ERE, so it is an injection surface. ----------------------- + + +def test_a_REGEX_METACHARACTER_in_the_word_list_is_refused_not_interpolated(sandbox): + """`.*` in the positive list would make POS_RE match every verdict-shaped line. + + The failure direction is the dangerous one: an explicit BLOCKED would classify `positive` and + the gate would grant consent nobody gave. The validator must refuse the vocabulary outright + rather than build a pattern from it. + """ + sandbox.edit_vocabulary(POSITIVE_DECL, "ETV_VERDICT_POSITIVE_WORDS=(mergeable '.*')") + + result = sandbox.read_side("BLOCKED") + assert result.returncode == 2, "a vocabulary that cannot be safely interpolated must be fatal" + assert result.stdout.strip() != "positive", ( + "an explicit BLOCKED verdict classified as positive — the word list reached the regex unvalidated" + ) + + +def test_a_GLOB_in_the_word_list_is_not_expanded_against_the_filesystem(sandbox): + """A `*` entry must be reported as `*`, not as whatever files happen to sit alongside. + + An unquoted expansion of a string word-list did exactly that: with `.*` declared, the validator + reported on `.claude`. Pathname expansion makes the vocabulary depend on the working directory, + and a `*` expanding to lowercase filenames (`scripts`, `docs`) would VALIDATE and enter the + vocabulary as real verdict words. + + Run with cwd=REPO_ROOT deliberately: that directory contains `scripts`, `docs` and `.claude`, + all of which match `[a-z][a-z-]*` and would therefore pass validation if they ever reached it. + """ + sandbox.edit_vocabulary(POSITIVE_DECL, "ETV_VERDICT_POSITIVE_WORDS=(mergeable '*')") + + result = subprocess.run( + ["bash", str(sandbox.reader), "--head", SHA_A], + input=json.dumps([{"body": f"Review-verdict: MERGEABLE @ {SHA_A}"}]), + env=sandbox.env, + capture_output=True, + text=True, + cwd=str(REPO_ROOT), + ) + assert result.returncode == 2 + # POSITIVE assertion first: the diagnostic must name the literal word that was written. Without + # this the test would pass on any exit-2 for any reason, which is the vacuous shape. + assert "* is not a usable verdict word" in result.stderr, ( + f"the validator did not report on the literal '*': {result.stderr!r}" + ) + for leaked in ("scripts", "docs", ".claude"): + assert f"{leaked} is not a usable" not in result.stderr, ( + f"the glob was expanded against the filesystem and reported on {leaked!r}" + ) + + +def test_the_SAME_word_in_both_lists_is_refused_rather_than_silently_picking_a_side(sandbox): + sandbox.edit_vocabulary(NEGATIVE_DECL, "ETV_VERDICT_NEGATIVE_WORDS=(blocked not-mergeable lgtm)") + result = sandbox.read_side("LGTM") + assert result.returncode == 2 + assert "appears twice" in result.stderr + + +def test_an_EMPTY_list_is_refused_rather_than_matching_nothing(sandbox): + """An empty positive list would build a pattern no verdict matches — every PR reads `unknown`. + + That direction is safe but unexplainable, and the empty NEGATIVE list is not safe at all: a + BLOCKED verdict would stop classifying as negative. + """ + sandbox.edit_vocabulary(NEGATIVE_DECL, "ETV_VERDICT_NEGATIVE_WORDS=()") + result = sandbox.read_side("BLOCKED") + assert result.returncode == 2 + assert result.stdout.strip() != "positive" + + +# --- The unchanged grammar still holds, now that the words arrive from elsewhere. --------------- + + +@pytest.mark.parametrize( + ("token", "expected"), + [ + ("MERGEABLE", "positive"), + ("mergeable", "positive"), + ("Approved", "positive"), + ("LGTM", "positive"), + ("BLOCKED", "negative"), + ("NOT-MERGEABLE", "negative"), + ("MERGEABLE-LATER", "unknown"), + ("LGTMish", "unknown"), + ("SHIPIT", "unknown"), + ], +) +def test_the_shipped_vocabulary_classifies_as_before(sandbox, token, expected): + """#788 is a refactor: the classification of every shipped word is unchanged. + + Whole-word matching (`MERGEABLE-LATER`, `LGTMish`) is the #629 false-open and is asserted here + because the boundary now sits next to an interpolated alternation rather than a literal one. + """ + assert sandbox.read_side(token).stdout.strip() == expected + + +@pytest.mark.parametrize( + ("token", "state"), + [ + ("MERGEABLE", "success"), + ("APPROVED", "success"), + ("LGTM", "success"), + ("BLOCKED", "failure"), + ("NOT-MERGEABLE", "failure"), + ], +) +def test_the_write_side_maps_the_shipped_words_onto_the_same_states(sandbox, token, state): + """Every shipped word, both polarities. A presence test cannot see POLARITY: a positive-only + matrix stays green if the negative arm maps onto `success` too.""" + assert sandbox.write_side(token).returncode == 0 + assert sandbox.posted_status_state() == state -- 2.47.3 From 99caae76158cf55f972b4af235201705fc11dcfd Mon Sep 17 00:00:00 2001 From: Timothy Date: Wed, 26 Aug 2026 20:37:38 +0200 Subject: [PATCH 2/6] fix(788): gate the vocabulary on a sentinel, not on a skippable `if` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cold adversarial review found a fail-open in the first commit, reproduced here before fixing: with `.*` in the positive list and the negative list written as a scalar, `check-review-verdict.sh` classified an explicit `BLOCKED @ ` as `positive`, exit 0. The merge-consent hook auto-grants on exactly that, and it discards the classifier's stderr, so the only signal was invisible. Root cause, measured rather than reasoned: * `${#arr[@]}` is nounset-safe only for a DECLARED-EMPTY array. On an unset name or a scalar it is a fatal unbound-variable error. * Under `set -u` that error inside a FUNCTION aborts the function, not the script — and in the `if ! validate; then exit 2; fi` form NEITHER branch is then taken, so the gate is skipped and execution continues. * `check-review-verdict.sh` deliberately runs without `set -e`, so the words reached the regex unvalidated. The write side has `set -e` and did die, so the asymmetry left the hole on precisely the side that authorizes merges. The single-fault case needed no metacharacter and is the realistic wrong edit: writing the list as a string degraded every negative verdict from `negative` to `unknown` — DENY silently downgraded to ASK — with no non-zero exit anywhere. A control-flow gate can be skipped by an abort; a value that was never assigned cannot be read. So enforcement is now a DATA dependency: validation sets `ETV_VERDICT_VOCABULARY_OK=1` as the last statement of its successful path, and `etv_verdict_class`/`etv_verdict_alternation`/`etv_verdict_words_display` refuse without it. The sentinel is reset to 0 at load time, so an inherited environment variable cannot forge it. Array-ness is asserted with `declare -p` BEFORE any `${#arr[@]}`, so the abort cannot occur in the first place — kept alongside the sentinel for the accurate message, not as a second gate. Separately, a top-level `exit` in a sourced library terminates the sourcing script at the `source` line, so no check written after it can run: the reader returned exit 0 with EMPTY stdout, breaking its "exactly one classification word" contract. Both consumers now probe-source the library in a subshell and require an end-of-file marker before sourcing it for real; that also catches a syntax error and a truncated file. All six failure modes now fail closed (reader exit 2, writer exit 1 having posted nothing), and the shipped words classify exactly as before. Four of the six new tests were witnessed RED against the unfixed predecessor 2d389a5bf; the other two (unset array, truncated library) pin behaviour that already held and are labelled as such rather than presented as fixes. Also from the review: prose in the decision record, `docs/ci-cd.md` and two in-script comments claimed validation was "run above, fatal" — the false premise that shipped the hole. Corrected to name the sentinel as what enforces it. The unresolved-symlink location dependency is recorded as a known limitation (fail-closed today). De-flaked the unreadable-file test for a root runner, and fixed a stale `check-review-verdict.sh:212` line reference plus a comment still describing #788 as future work. Not done, deliberately: no third guard on the both-patterns-match case. The validator rejects a word in both lists, and test_check_review_verdict.py already pins the dangerous direction; a third overlapping guard is the shape where deleting any one leaves the suite green. Verification: 987 passed / 2 skipped, ruff clean, shellcheck clean, catalog regenerated, decisions-validate OK. Refs: #788 Decisions-Edit: yes Co-authored-by: Timothy --- docs/ci-cd.md | 19 ++- docs/decisions/README.md | 2 +- .../release/verdict-vocabulary-shared.md | 42 +++++- scripts/check-review-verdict.sh | 34 ++++- scripts/lib/review-verdict-vocabulary.sh | 73 +++++++++- scripts/post-review-verdict.sh | 9 ++ scripts/tests/test_check_review_verdict.py | 6 +- .../tests/test_review_verdict_vocabulary.py | 126 ++++++++++++++++++ 8 files changed, 288 insertions(+), 23 deletions(-) diff --git a/docs/ci-cd.md b/docs/ci-cd.md index a805f611b..66155d615 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -1249,11 +1249,20 @@ in `check-review-verdict.sh`, where every #629 false-open actually lived. Run Two things follow that are easy to get wrong. **Do not reintroduce a parity test** that compares the two scripts' sources: #774 shipped one and withdrew it after six review rounds, each finding another shell construction that escaped the extractor or reddened it on a correct tree. And a vocabulary -that is missing, unreadable or invalid is **fatal on both sides** — the reader exits 2 (which -callers treat as an unreadable input and fail closed on), the writer exits 1 having posted nothing. -That is deliberate: words are interpolated into a regex, so a `.*` slipping into the positive list -would classify an explicit `BLOCKED` as `positive`. Validation restricts every word to -`[a-z][a-z-]*`, which is why it is a hard exit rather than a warning. +that cannot be loaded or does not validate yields **no words on either side** — the reader exits 2 +(which callers treat as an unreadable input and fail closed on), the writer exits 1 having posted +nothing. Words are interpolated into a regex, so a `.*` in the positive list would classify an +explicit `BLOCKED` as `positive`; validation restricts every word to `[a-z][a-z-]*`. + +**What enforces that is a data dependency, not the `if` around the validator** — worth knowing +before editing either script, because the obvious reading is wrong and the first version of #788 +shipped the hole. Under `set -u`, an unbound-variable error inside a function aborts the function +but not the script, and in an `if ! validate` form neither branch runs, so the `exit 2` is skipped. +The reader has no `set -e`, so unvalidated words reached the regex and an explicit `BLOCKED @ head` +classified `positive`, exit 0. Validation now sets a sentinel on its last line and the derived views +refuse without it. Likewise a top-level `exit` in a sourced library terminates the sourcing script +where it stands, so both consumers probe-source the library in a subshell and require an +end-of-file marker before trusting it. The status description also records the base branch — `Review-verdict: MERGEABLE @ abc1234 (base: main)` — and the merge-consent hook denies when that no longer matches the PR's live `base.ref` diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 2021f3851..51ae1a9ed 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -141,7 +141,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `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, 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_target` workflow auto-passes the two exempt classes (Renovate-authored, docs-only) unless the PR touches a protected path (`.claude/`, `.codex/`, `.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) | -| `release.verdict-vocabulary-shared` | The H10 verdict words live in exactly one place, `scripts/lib/review-verdict-vocabulary.sh`, as two arrays. `scripts/post-review-verdict.sh` (write: word to commit-status state) classifies through `etv_verdict_class`; `scripts/check-review-verdict.sh` (read: comment to classification) builds `POS_RE`/`NEG_RE` from `etv_verdict_alternation`. Neither script may restate a word, and neither may enumerate the vocabulary in a usage banner or an error string. Every word is validated to `[a-z][a-z-]*` before it reaches a regex, and a vocabulary that is missing, unreadable or invalid is FATAL on both sides — exit 1 for the writer, exit 2 for the reader. Do NOT reintroduce a text-comparison parity test alongside this. | 2026-08-26 | [link](records/release/verdict-vocabulary-shared.md) | +| `release.verdict-vocabulary-shared` | The H10 verdict words live in exactly one place, `scripts/lib/review-verdict-vocabulary.sh`, as two arrays. `scripts/post-review-verdict.sh` (write: word to commit-status state) classifies through `etv_verdict_class`; `scripts/check-review-verdict.sh` (read: comment to classification) builds `POS_RE`/`NEG_RE` from `etv_verdict_alternation`. Neither script may restate a word, and neither may enumerate the vocabulary in a usage banner or an error string. Every word is validated to `[a-z][a-z-]*` before it reaches a regex. Validation is enforced by a DATA dependency, not a control-flow check: `etv_verdict_vocabulary_validate` sets `ETV_VERDICT_VOCABULARY_OK=1` on its final line, and the derived views refuse to hand out words without it. A vocabulary that is missing, unreadable, truncated, exits at top level, is declared as a scalar, or fails validation yields NO words on either side — exit 1 for the writer (nothing posted), exit 2 for the reader (callers fail closed). Do NOT reintroduce a text-comparison parity test alongside this. | 2026-08-26 | [link](records/release/verdict-vocabulary-shared.md) | | `release.verdict-writes-status-before-comment` | `scripts/post-review-verdict.sh` writes the sha-bound `review-verdict/h10` commit status FIRST and the human-readable `Review-verdict:` comment SECOND. Every refusal path still refuses (fail-closed, unchanged) and exits non-zero, and none of them may leave a verdict comment behind. An orphaned comment is therefore PREVENTED rather than tolerated. If the comment write fails after the status was written, that is an error too, but it degrades to an `ask` at the merge gate rather than to an apparent grant. | 2026-08-22 | [link](records/release/verdict-writes-status-before-comment.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/verdict-vocabulary-shared.md b/docs/decisions/records/release/verdict-vocabulary-shared.md index e13e50836..3d56f3474 100644 --- a/docs/decisions/records/release/verdict-vocabulary-shared.md +++ b/docs/decisions/records/release/verdict-vocabulary-shared.md @@ -5,7 +5,7 @@ status: active since: '2026-08-26' supersedes: none superseded-by: none -rule: 'The H10 verdict words live in exactly one place, `scripts/lib/review-verdict-vocabulary.sh`, as two arrays. `scripts/post-review-verdict.sh` (write: word to commit-status state) classifies through `etv_verdict_class`; `scripts/check-review-verdict.sh` (read: comment to classification) builds `POS_RE`/`NEG_RE` from `etv_verdict_alternation`. Neither script may restate a word, and neither may enumerate the vocabulary in a usage banner or an error string. Every word is validated to `[a-z][a-z-]*` before it reaches a regex, and a vocabulary that is missing, unreadable or invalid is FATAL on both sides — exit 1 for the writer, exit 2 for the reader. Do NOT reintroduce a text-comparison parity test alongside this.' +rule: 'The H10 verdict words live in exactly one place, `scripts/lib/review-verdict-vocabulary.sh`, as two arrays. `scripts/post-review-verdict.sh` (write: word to commit-status state) classifies through `etv_verdict_class`; `scripts/check-review-verdict.sh` (read: comment to classification) builds `POS_RE`/`NEG_RE` from `etv_verdict_alternation`. Neither script may restate a word, and neither may enumerate the vocabulary in a usage banner or an error string. Every word is validated to `[a-z][a-z-]*` before it reaches a regex. Validation is enforced by a DATA dependency, not a control-flow check: `etv_verdict_vocabulary_validate` sets `ETV_VERDICT_VOCABULARY_OK=1` on its final line, and the derived views refuse to hand out words without it. A vocabulary that is missing, unreadable, truncated, exits at top level, is declared as a scalar, or fails validation yields NO words on either side — exit 1 for the writer (nothing posted), exit 2 for the reader (callers fail closed). Do NOT reintroduce a text-comparison parity test alongside this.' signals: 'verdict vocabulary drift, POS_RE NEG_RE, two shell copies, parity test withdrawn, add a verdict word, shared vocabulary, MERGEABLE not recognised, unexplained merge-gate deny · paths: `scripts/lib/review-verdict-vocabulary.sh`, `scripts/post-review-verdict.sh`, `scripts/check-review-verdict.sh`, `scripts/tests/test_review_verdict_vocabulary.py` · issues: #788, #774, #629, #622' mechanics: '`scripts/tests/test_review_verdict_vocabulary.py::test_a_word_added_to_the_shared_source_reaches_BOTH_sides` (and the NEGATIVE-list twin), graded MUTATION in `docs/guard-inventory.md` with its clause declared in `scripts/tests/mutation_manifest.py` — the harness restores the pre-#788 hardcoded `POS_RE` on every run and requires that proof to go red' --- @@ -37,12 +37,34 @@ whole-word boundary, the anchored `@ ` field (#629) — and none of that be **Validation is not hygiene, it is the security property.** The list is interpolated into an extended regular expression, so it is an injection surface, and the failure direction is the -dangerous one: a `.*` in the positive list would make `POS_RE` match every verdict-shaped line, so -an explicit `BLOCKED` would classify `positive` and the gate would grant consent nobody gave. -Restricting words to `[a-z][a-z-]*` makes that unreachable as a property of the character class -rather than of anyone remembering to escape. A word appearing in both lists, an empty list and an -empty word are refused for the same reason: each would make the classification of some real verdict -depend on which arm ran first. +dangerous one: a `.*` in the positive list makes `POS_RE` match every verdict-shaped line, so an +explicit `BLOCKED` classifies `positive` and the gate grants consent nobody gave. A word appearing +in both lists, an empty list and an empty word are refused for the same reason: each would make the +classification of some real verdict depend on which arm ran first. + +**A control-flow gate was not enough, and the first version of this change shipped the hole.** The +validator was called as `if ! etv_verdict_vocabulary_validate; then exit 2; fi`. Under `set -u` an +unbound-variable error inside a FUNCTION aborts the function but not the script, and in that form +NEITHER branch is then taken — execution continues straight past the gate. `${#arr[@]}` triggers +exactly that abort: it is nounset-safe only on a *declared-empty* array, and fatal on an unset name +or a scalar. Since the reader deliberately runs without `set -e`, the words reached the regex +unvalidated. Measured, with `.*` in the positive list and the negative list written as a scalar: an +explicit `BLOCKED @ ` classified **`positive`, exit 0**, with the only signal on a stderr the +calling hook discards. Cold review found it; it was not caught by the suite. + +So enforcement is now a DATA dependency. Validation sets `ETV_VERDICT_VOCABULARY_OK=1` as the last +statement of its successful path, and `etv_verdict_class` / `etv_verdict_alternation` refuse without +it. A control-flow gate can be skipped by an abort; a value that was never assigned cannot be read. +The sentinel is reset to 0 at load time so an inherited environment variable cannot forge it, and +array-ness is asserted with `declare -p` *before* any `${#arr[@]}` so the abort cannot happen in the +first place. Both are kept deliberately — the second gives an accurate message, the first is what +holds. + +**A sourced library that `exit`s cannot be caught after the fact**, because the `exit` terminates +the sourcing script at the `source` line — the reader returned exit 0 with empty stdout, breaking +its "exactly one classification word" contract. Both consumers therefore probe-source the library in +a SUBSHELL and require an end-of-file marker on stdout before sourcing it for real. That also covers +a syntax error and a truncated file, neither of which reaches the marker. **The arrays are arrays, not whitespace-separated strings.** A string list has to be split by an UNQUOTED expansion, and an unquoted expansion also performs PATHNAME EXPANSION. That is not @@ -51,6 +73,12 @@ repo root. It makes validation depend on the working directory, and it fails OPE matters, since a `*` entry expands to the filenames beside it and any matching `[a-z][a-z-]*` (`scripts`, `docs`) would validate cleanly and enter the vocabulary as a real verdict word. +**Known limitation, stated rather than implied.** The library is located relative to +`${BASH_SOURCE[0]}` without resolving symlinks, so a script reached through a symlink looks for its +vocabulary beside the *link*. Every normal invocation is unaffected (absolute path, relative, via +`PATH`, a path containing spaces — all measured), and the case fails CLOSED when no library sits +there. It is recorded because this repo does symlink files across repositories. + **What is deliberately NOT shared.** `.claude/hooks/pretooluse-merge-consent.sh` names the words in one operator-facing message string. It performs no classification of its own — it delegates to `check-review-verdict.sh` — so it is a prose copy, not a second implementation, and sourcing a shell diff --git a/scripts/check-review-verdict.sh b/scripts/check-review-verdict.sh index bc062fad3..459994ebd 100755 --- a/scripts/check-review-verdict.sh +++ b/scripts/check-review-verdict.sh @@ -83,16 +83,37 @@ esac # closed on it (see the header). A classifier that cannot load its vocabulary does not know what a # verdict word is; the one thing it must never do is fall through to `absent`, which the hook reads # as "convention not adopted" and downgrades into an `ask`. +# +# THE GATE IS THE SENTINEL, NOT THIS `if`. Stated because the obvious reading is wrong: under +# `set -u` an unbound-variable error inside a function aborts the function and NOT the script, and +# in an `if ! validate` form neither branch is then taken, so the `exit 2` is skipped and execution +# continues. That is a measured fail-open, not a hypothetical — see the sentinel comment in the +# library. `etv_verdict_alternation` refusing without the sentinel is what actually holds. _here=$(CDPATH='' cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd) || { printf 'check-review-verdict: could not resolve this script own directory to locate the shared verdict vocabulary\n' >&2; exit 2; } VOCABULARY_LIB="$_here/lib/review-verdict-vocabulary.sh" if [ ! -r "$VOCABULARY_LIB" ]; then printf 'check-review-verdict: the shared verdict vocabulary is missing or unreadable at %s\n' "$VOCABULARY_LIB" >&2; exit 2 fi +# PROBE-SOURCE IN A SUBSHELL FIRST, and require the library's end-of-file marker on stdout. A +# top-level `exit` in a sourced file terminates THIS script where the `source` stands, so no check +# written after it can run: the reader returned exit 0 with empty stdout, which violates its +# "exactly one classification word" contract and would read as success to any caller that checks +# only the exit code. The probe moves that failure into a subshell, where it is observable. It also +# covers a syntax error and a truncated file, since neither reaches the marker. +# shellcheck source=lib/review-verdict-vocabulary.sh +vocabulary_probe=$( . "$VOCABULARY_LIB" >/dev/null 2>&1 && etv_verdict_vocabulary_loaded 2>/dev/null ) || vocabulary_probe='' +if [ "$vocabulary_probe" != 'etv-verdict-vocabulary-loaded' ]; then + printf 'check-review-verdict: the shared verdict vocabulary at %s did not load to completion (syntax error, truncated, or it exits at top level)\n' "$VOCABULARY_LIB" >&2; exit 2 +fi # shellcheck source=lib/review-verdict-vocabulary.sh if ! . "$VOCABULARY_LIB"; then printf 'check-review-verdict: the shared verdict vocabulary at %s could not be sourced\n' "$VOCABULARY_LIB" >&2; exit 2 fi +# The return code is checked, but it is NOT what makes this safe — a `set -u` abort inside the +# function would skip both branches of this `if` and carry straight on. What makes it safe is that +# validation sets a sentinel on its last line and `etv_verdict_alternation` refuses without it, so a +# skipped validation cannot yield a pattern. Both are kept: this gives the accurate diagnostic. if ! etv_verdict_vocabulary_validate; then printf 'check-review-verdict: the shared verdict vocabulary at %s did not validate (see above)\n' "$VOCABULARY_LIB" >&2; exit 2 fi @@ -219,10 +240,15 @@ FIELD_RE='^review-verdict:[[:space:]]*[A-Za-z][A-Za-z-]*[[:space:]]*@[[:space:]] # list. `etv_verdict_alternation` emits only the alternation BODY (`word|word|word`), and the # `([[:space:]@]|$)` boundary below are unchanged. # -# Interpolating into an ERE is safe because `etv_verdict_vocabulary_validate` (run above, fatal) -# restricts every word to `[a-z][a-z-]*`, so no character reaching this regex can be a metacharacter. -# Without that check a `.*` in the positive list would make every verdict-shaped line classify -# `positive`, including an explicit BLOCKED — which is why the validation is a hard exit, not a warn. +# Interpolating into an ERE is safe because every word is restricted to `[a-z][a-z-]*`, so no +# character reaching this regex can be a metacharacter. Without that a `.*` in the positive list +# would make every verdict-shaped line classify `positive`, including an explicit BLOCKED. +# +# WHAT ENFORCES IT is `etv_verdict_alternation` refusing unless validation set its sentinel — NOT +# the `if ! etv_verdict_vocabulary_validate` above. Saying "validated above, therefore safe" was the +# false premise that shipped the hole: a `set -u` abort inside that function skips both branches of +# the `if`, and the words then reached this line unvalidated. The call above is kept for its +# diagnostic; the refusal below is the gate. POS_ALTERNATION=$(etv_verdict_alternation positive) || { printf 'check-review-verdict: could not build the positive verdict pattern from %s\n' "$VOCABULARY_LIB" >&2; exit 2; } NEG_ALTERNATION=$(etv_verdict_alternation negative) || { diff --git a/scripts/lib/review-verdict-vocabulary.sh b/scripts/lib/review-verdict-vocabulary.sh index 4cf879d1c..dc12aa2d0 100644 --- a/scripts/lib/review-verdict-vocabulary.sh +++ b/scripts/lib/review-verdict-vocabulary.sh @@ -46,6 +46,24 @@ ETV_VERDICT_POSITIVE_WORDS=(mergeable approved lgtm) ETV_VERDICT_NEGATIVE_WORDS=(blocked not-mergeable) +# --- The validation sentinel. ------------------------------------------------------------------- +# Reset to 0 HERE, at load time, for two reasons. It defeats an inherited `ETV_VERDICT_VOCABULARY_OK=1` +# from the environment, and — the reason it exists — it makes "validated" a fact the DERIVED VIEWS +# can require, rather than something a caller is trusted to have checked. +# +# WHY A SENTINEL AND NOT A RETURN CODE. `etv_verdict_vocabulary_validate` used to be gated as +# `if ! etv_verdict_vocabulary_validate; then exit 2; fi`, and that is bypassable: under `set -u` an +# unbound-variable error inside a FUNCTION aborts the function but not the script, and in the +# `if ! f` form NEITHER branch is then taken — execution simply continues past the gate. Measured: +# with the negative list written as a scalar (`ETV_VERDICT_NEGATIVE_WORDS="blocked"`) and `.*` in the +# positive list, `check-review-verdict.sh` classified an explicit `BLOCKED @ ` as `positive`, +# exit 0, with only a stderr line the calling hook discards. That is the exact fail-open this whole +# file exists to make impossible. +# +# A control-flow gate can be skipped by an abort. A DATA dependency cannot: the words are now +# unobtainable unless validation ran all the way to its final line. +ETV_VERDICT_VOCABULARY_OK=0 + # --- Validation. ------------------------------------------------------------------------------- # The read side interpolates these words into an EXTENDED REGULAR EXPRESSION. That makes the list an # injection surface, and the failure direction is the dangerous one: a stray `.*` in the positive @@ -57,11 +75,33 @@ ETV_VERDICT_NEGATIVE_WORDS=(blocked not-mergeable) # Returns 0 when the vocabulary is usable, 1 otherwise (message on stderr). It never exits: the two # consumers fail closed with DIFFERENT codes (`post` dies 1, `check` exits 2 so its callers treat it # as an unreadable input), so the exit semantics belong to them, not here. -etv_verdict_vocabulary_validate() { - local word seen=' ' +# True when $1 names a set variable carrying the ARRAY attribute. Checked before any `${#name[@]}`, +# because that expansion is nounset-safe ONLY for a declared-empty array: on an UNSET name or on a +# SCALAR it is a fatal unbound-variable error, which is precisely the abort that used to skip +# validation entirely. `declare -p` answers without expanding anything. +etv_verdict__is_array() { + local declaration flags + declaration=$(declare -p "$1" 2>/dev/null) || return 1 + flags=${declaration#declare -} + flags=${flags%% *} + case "$flags" in + *a*) return 0 ;; + *) return 1 ;; + esac +} + +etv_verdict_vocabulary_validate() { + local word seen=' ' name + + ETV_VERDICT_VOCABULARY_OK=0 + + for name in ETV_VERDICT_POSITIVE_WORDS ETV_VERDICT_NEGATIVE_WORDS; do + if ! etv_verdict__is_array "$name"; then + printf 'review-verdict vocabulary: %s is not an array — declare it as %s=(word word), not as a string\n' "$name" "$name" >&2 + return 1 + fi + done - # `${#arr[@]}` is safe on an unset/empty array even under `set -u`, which `"${arr[@]}"` is not on - # bash 3.2 — so the count is checked BEFORE anything expands the arrays. if [ "${#ETV_VERDICT_POSITIVE_WORDS[@]}" -eq 0 ] || [ "${#ETV_VERDICT_NEGATIVE_WORDS[@]}" -eq 0 ]; then printf 'review-verdict vocabulary: the positive and negative lists must both be non-empty\n' >&2 return 1 @@ -88,6 +128,9 @@ etv_verdict_vocabulary_validate() { seen="$seen$word " done + # The ONLY assignment to 1 in this file, and it is the last statement of the successful path. Any + # earlier `return 1`, and any abort part-way through, leaves the sentinel at 0. + ETV_VERDICT_VOCABULARY_OK=1 return 0 } @@ -96,6 +139,11 @@ etv_verdict_vocabulary_validate() { # nothing. Case-insensitive, so the caller may pass the operator's raw argument. etv_verdict_class() { local token word + if [ "${ETV_VERDICT_VOCABULARY_OK:-0}" != 1 ]; then + printf 'review-verdict vocabulary: not validated — refusing to hand out verdict words\n' >&2 + return 1 + fi + token=$(printf '%s' "${1-}" | tr '[:upper:]' '[:lower:]') for word in ${ETV_VERDICT_POSITIVE_WORDS[@]+"${ETV_VERDICT_POSITIVE_WORDS[@]}"}; do @@ -121,6 +169,10 @@ etv_verdict_class() { etv_verdict_alternation() { local alternation='' word local words=() + if [ "${ETV_VERDICT_VOCABULARY_OK:-0}" != 1 ]; then + printf 'review-verdict vocabulary: not validated — refusing to hand out verdict words\n' >&2 + return 1 + fi case "${1-}" in positive) words=(${ETV_VERDICT_POSITIVE_WORDS[@]+"${ETV_VERDICT_POSITIVE_WORDS[@]}"}) ;; @@ -145,6 +197,10 @@ etv_verdict_alternation() { # vocabulary — the same drift this file exists to remove, one layer out. etv_verdict_words_display() { local word index=0 total out='' + if [ "${ETV_VERDICT_VOCABULARY_OK:-0}" != 1 ]; then + printf 'review-verdict vocabulary: not validated — refusing to hand out verdict words\n' >&2 + return 1 + fi set -- ${ETV_VERDICT_POSITIVE_WORDS[@]+"${ETV_VERDICT_POSITIVE_WORDS[@]}"} \ ${ETV_VERDICT_NEGATIVE_WORDS[@]+"${ETV_VERDICT_NEGATIVE_WORDS[@]}"} @@ -164,3 +220,12 @@ etv_verdict_words_display() { printf '%s\n' "$out" } + +# --- Reached-the-end marker. -------------------------------------------------------------------- +# A top-level `exit` in a SOURCED file terminates the sourcing script where it stands, so no check +# placed after the `source` can ever run — the reader returned exit 0 with empty stdout, violating +# its own "exactly one classification word" contract. Both consumers therefore source this file in a +# SUBSHELL first and require the marker below on stdout; a file that exits early never prints it. +# Defining a function is not enough (a truncated file can define every function and still stop +# short), so this is the LAST line and it must stay last. +etv_verdict_vocabulary_loaded() { printf 'etv-verdict-vocabulary-loaded\n'; } diff --git a/scripts/post-review-verdict.sh b/scripts/post-review-verdict.sh index 86115a8a3..ca1ef602d 100755 --- a/scripts/post-review-verdict.sh +++ b/scripts/post-review-verdict.sh @@ -58,9 +58,18 @@ _here=$(CDPATH='' cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd) \ VOCABULARY_LIB="$_here/lib/review-verdict-vocabulary.sh" [ -r "$VOCABULARY_LIB" ] \ || die "the shared verdict vocabulary is missing or unreadable at $VOCABULARY_LIB — no status was written" +# Probe-source in a subshell and require the end-of-file marker before trusting the file: a +# top-level `exit` in a sourced library terminates THIS script at the `source` line, which exited 0 +# having posted neither a status nor a comment. See the read side for the full reasoning. +# shellcheck source=lib/review-verdict-vocabulary.sh +vocabulary_probe=$( . "$VOCABULARY_LIB" >/dev/null 2>&1 && etv_verdict_vocabulary_loaded 2>/dev/null ) || vocabulary_probe='' +[ "$vocabulary_probe" = 'etv-verdict-vocabulary-loaded' ] \ + || die "the shared verdict vocabulary at $VOCABULARY_LIB did not load to completion (syntax error, truncated, or it exits at top level) — no status was written" # shellcheck source=lib/review-verdict-vocabulary.sh . "$VOCABULARY_LIB" \ || die "the shared verdict vocabulary at $VOCABULARY_LIB could not be sourced — no status was written" +# `set -e` makes this `die` reliable here, unlike on the read side — but the sentinel the library +# sets on validation's last line is what actually gates the words, on both sides. etv_verdict_vocabulary_validate \ || die "the shared verdict vocabulary at $VOCABULARY_LIB did not validate (see above) — no status was written" diff --git a/scripts/tests/test_check_review_verdict.py b/scripts/tests/test_check_review_verdict.py index 2c7c6f4ed..743e23640 100644 --- a/scripts/tests/test_check_review_verdict.py +++ b/scripts/tests/test_check_review_verdict.py @@ -429,7 +429,9 @@ def test_non_hex_head_is_an_input_error(): # completeness claim: the property below is PER-MEMBER — "each of these words classifies as exactly # one thing" — so a word missing from this list is simply an untested word, not a defect the list # conceals. It is emphatically NOT a claim that these are the only words the scripts accept. Proving -# THAT needs one shared vocabulary both scripts read, which is ersatztv#788. +# THAT is what the shared vocabulary in `scripts/lib/review-verdict-vocabulary.sh` supplies +# (ersatztv#788, landed); `test_review_verdict_vocabulary.py` makes the completeness claim against +# that one declaration, which is why this list may stay a per-member literal. POSITIVE_WORDS = ["MERGEABLE", "APPROVED", "LGTM"] NEGATIVE_WORDS = ["BLOCKED", "NOT-MERGEABLE"] @@ -463,7 +465,7 @@ def test_each_verdict_word_retains_its_established_polarity(word, expected): both flags and precedence decides. WHICH DIRECTION THIS CATCHES, MEASURED RATHER THAN REASONED — the first draft of this docstring - asserted it and was checked afterwards, which is the wrong order. `check-review-verdict.sh:212` + asserted it and was checked afterwards, which is the wrong order. `check-review-verdict.sh:279` reads `if [ "$is_pos" = 1 ]; then head_pos=1; else head_neg=1; fi`, so `is_pos` wins per line and an overlapping word resolves POSITIVE. Both mutations were run against this test: diff --git a/scripts/tests/test_review_verdict_vocabulary.py b/scripts/tests/test_review_verdict_vocabulary.py index b923583fe..63700e1cb 100644 --- a/scripts/tests/test_review_verdict_vocabulary.py +++ b/scripts/tests/test_review_verdict_vocabulary.py @@ -183,6 +183,10 @@ def test_a_MISSING_vocabulary_makes_the_write_side_refuse_to_post(sandbox): assert sandbox.posted_status_state() is None, "no status may be written without a vocabulary" +@pytest.mark.skipif( + hasattr(os, "geteuid") and os.geteuid() == 0, + reason="mode 000 does not make a file unreadable to root, and script-tests runs in a container", +) def test_an_UNREADABLE_vocabulary_is_refused_rather_than_skipped(sandbox): sandbox.vocabulary.chmod(0o000) try: @@ -304,3 +308,125 @@ def test_the_write_side_maps_the_shipped_words_onto_the_same_states(sandbox, tok matrix stays green if the negative arm maps onto `success` too.""" assert sandbox.write_side(token).returncode == 0 assert sandbox.posted_status_state() == state + + +# --- A control-flow gate can be SKIPPED; the sentinel is what actually holds. -------------------- +# +# Under `set -u`, an unbound-variable error inside a FUNCTION aborts the function but not the +# script, and in an `if ! validate` form NEITHER branch is then taken — so the `exit 2` never runs +# and execution continues past the gate. `check-review-verdict.sh` runs without `set -e`, so the +# words reached the regex unvalidated: with `.*` in the positive list and the negative list written +# as a scalar, an explicit `BLOCKED @ ` classified `positive`, exit 0. The calling hook +# discards stderr, so the only signal was invisible. +# +# The fix is a DATA dependency rather than a control-flow one: validation sets +# `ETV_VERDICT_VOCABULARY_OK=1` on its last line, and the derived views refuse without it. These +# tests pin the malformation classes that produce the abort. + + +def test_a_SCALAR_word_list_cannot_reach_the_regex_unvalidated(sandbox): + """The originally-measured fail-open, pinned end to end. + + Two faults at once, because that is the shape that produced `positive`: a metacharacter in the + positive list (which only matters if validation is skipped) and a scalar negative list (which is + what skips it). + """ + sandbox.edit_vocabulary(POSITIVE_DECL, 'ETV_VERDICT_POSITIVE_WORDS=(mergeable ".*")') + sandbox.edit_vocabulary(NEGATIVE_DECL, 'ETV_VERDICT_NEGATIVE_WORDS="blocked"') + + result = sandbox.read_side("BLOCKED") + assert result.stdout.strip() != "positive", ( + "an explicit BLOCKED verdict classified as positive — the word list reached the regex " + "without validation completing" + ) + assert result.returncode == 2 + + +def test_a_SCALAR_word_list_is_refused_even_with_no_metacharacter(sandbox): + """The single-fault case, which is the realistic wrong edit. + + Writing the list as a string is the natural mistake, and on its own it silently degraded every + negative verdict from `negative` to `unknown` — DENY downgraded to ASK — with no non-zero exit. + """ + sandbox.edit_vocabulary(NEGATIVE_DECL, 'ETV_VERDICT_NEGATIVE_WORDS="blocked not-mergeable"') + + result = sandbox.read_side("BLOCKED") + assert result.returncode == 2 + assert result.stdout.strip() != "negative", "expected a refusal, not a lucky classification" + assert "is not an array" in result.stderr + + +def test_an_UNSET_word_list_is_refused(sandbox): + sandbox.edit_vocabulary(NEGATIVE_DECL, "unset ETV_VERDICT_NEGATIVE_WORDS") + result = sandbox.read_side("BLOCKED") + assert result.returncode == 2 + assert result.stdout.strip() != "positive" + + +def test_a_library_that_EXITS_at_top_level_does_not_return_exit_0_with_no_verdict(sandbox): + """A top-level `exit` in a SOURCED file terminates the sourcing script where it stands. + + No check written after the `source` can run, so the reader returned exit 0 with EMPTY stdout — + violating its own "exactly one classification word" contract. A caller testing only the exit + code would read that as a successful classification. The subshell probe moves the failure + somewhere observable. + """ + sandbox.vocabulary.write_text("exit 0\n") + + result = sandbox.read_side("BLOCKED") + assert result.returncode == 2, f"exit {result.returncode} with stdout {result.stdout!r}" + assert result.stdout.strip() == "" + + write = sandbox.write_side("MERGEABLE") + assert write.returncode != 0 + assert sandbox.posted_status_state() is None + + +def test_a_TRUNCATED_library_is_refused_rather_than_half_used(sandbox): + """A file cut short can define every function it got to and still be incomplete. + + So the marker the probe requires is the LAST line of the library, not merely a function it + defines somewhere. + """ + text = sandbox.vocabulary.read_text() + marker = "etv_verdict_vocabulary_loaded()" + assert marker in text, "the end-of-file marker moved; this test no longer truncates anything" + # Cut exactly at the marker: everything before it is a file that defines every function it got + # to and is still incomplete. A fixed line count would drift as the library grows. + sandbox.vocabulary.write_text(text[: text.index(marker)]) + + result = sandbox.read_side("BLOCKED") + assert result.returncode == 2 + assert result.stdout.strip() != "positive" + + +def test_the_sentinel_cannot_be_PRE_SET_from_the_environment(sandbox): + """Otherwise the data dependency would be forgeable by whoever sets the environment.""" + env = dict(sandbox.env) + env["ETV_VERDICT_VOCABULARY_OK"] = "1" + sandbox.edit_vocabulary(NEGATIVE_DECL, 'ETV_VERDICT_NEGATIVE_WORDS="blocked"') + + result = subprocess.run( + ["bash", str(sandbox.reader), "--head", SHA_A], + input=json.dumps([{"body": f"Review-verdict: BLOCKED @ {SHA_A}"}]), + env=env, + capture_output=True, + text=True, + ) + assert result.returncode == 2, "an inherited sentinel must not stand in for validation" + assert result.stdout.strip() != "positive" + + +def test_a_VALID_vocabulary_still_sets_the_sentinel_and_classifies(sandbox): + """The positive control for every refusal above: the guard must not simply always refuse.""" + env = dict(sandbox.env) + env["ETV_VERDICT_VOCABULARY_OK"] = "1" + result = subprocess.run( + ["bash", str(sandbox.reader), "--head", SHA_A], + input=json.dumps([{"body": f"Review-verdict: BLOCKED @ {SHA_A}"}]), + env=env, + capture_output=True, + text=True, + ) + assert result.returncode == 0 + assert result.stdout.strip() == "negative" -- 2.47.3 From 16070d40d30fd1e46e09fb05c9d7e33b1159eb34 Mon Sep 17 00:00:00 2001 From: Timothy Date: Wed, 26 Aug 2026 20:56:06 +0200 Subject: [PATCH 3/6] fix(788): prove the sentinel is load-bearing, and correct three claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second cold review could not break the fix — no fail-open under sentinel forgery, `declare -p` evasion across 17 declaration forms on bash 5.3 and 3.2, probe-source attacks, or a 19-case grammar regression battery. What it broke was the EVIDENCE. **The sentinel was proved by nothing.** Deleting all three `ETV_VERDICT_VOCABULARY_OK` guards left 150 tests green. The reason is the other half of the same fix: `etv_verdict__is_array` removes the `set -u` abort, so validation now returns 1 cleanly and the ordinary `if ! validate; then exit 2; fi` fires — meaning every refusal test passes without the sentinel ever executing. Two guards on one condition, where deleting either leaves the suite green, is #685 exactly, cited in this test file's own docstring. So the sentinel is now proved by disarming the OTHER guard first. With the array assertion removed the abort route reopens, the `if` is skipped, and the sentinel is the only thing between a malformed word list and the regex: * with the array assertion disarmed, the sentinel alone still refuses (exit 2, and the assertion requires the sentinel's OWN diagnostic, not merely some refusal) * removing BOTH reproduces the original fail-open — `positive`, exit 0 — which is the control that stops the first test from proving only "it refuses" * an inherited `ETV_VERDICT_VOCABULARY_OK=1` still cannot carry a malformed vocabulary through, tested with the abort route open Witnessed: deleting all three guards now turns those three tests RED. Three claims corrected, all measured: * `check-review-verdict.sh:212` -> the previous commit "fixed" this to 279, which is also wrong; the line is 284. Found by grep this time rather than by counting, in a docstring specifically about measuring rather than reasoning. * The record said the merge-consent hook names the words in ONE message string. There are two — and the one it excused had ALREADY drifted, omitting LGTM from the vocabulary this record exists to unify. The hook comment is corrected and the residual is restated as two, with the honest note that prose copies drift too and are tolerated only because they are inert. * Two test names claimed more than the tests observed (`..._cannot_be_PRE_SET_from_the_environment` passed with the sentinel deleted; `..._still_sets_the_sentinel_...` cannot see the sentinel). Both replaced by tests that isolate what they name. Also: the usage banner printed the vocabulary lower-case while the unknown-word error printed it upper-case, so an operator saw two spellings of one list. Rebased onto origin/main (40a3232d9) — #841 landed from a parallel session while this branch was in review, and the pre-rebase diff showed that PR's files as deletions. Verification: 989 passed / 2 skipped, ruff clean, shellcheck clean (the two SC1090s on the hook are pre-existing on main), catalog regenerated, decisions-validate OK. Refs: #788 Decisions-Edit: yes Co-authored-by: Timothy --- .claude/hooks/pretooluse-merge-consent.sh | 2 +- .../release/verdict-vocabulary-shared.md | 14 ++- scripts/post-review-verdict.sh | 6 +- scripts/tests/test_check_review_verdict.py | 2 +- .../tests/test_review_verdict_vocabulary.py | 104 +++++++++++++++--- 5 files changed, 105 insertions(+), 23 deletions(-) diff --git a/.claude/hooks/pretooluse-merge-consent.sh b/.claude/hooks/pretooluse-merge-consent.sh index d811139ef..330e6626c 100755 --- a/.claude/hooks/pretooluse-merge-consent.sh +++ b/.claude/hooks/pretooluse-merge-consent.sh @@ -22,7 +22,7 @@ # 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: @ `. +# comment carrying a line `Review-verdict: @ `. # # Decision policy — a CONSENT gate, so it does NOT fail silently open: # - state derivable and satisfied -> grant (auto-approve: permissionDecision "allow", diff --git a/docs/decisions/records/release/verdict-vocabulary-shared.md b/docs/decisions/records/release/verdict-vocabulary-shared.md index 3d56f3474..644ab2b66 100644 --- a/docs/decisions/records/release/verdict-vocabulary-shared.md +++ b/docs/decisions/records/release/verdict-vocabulary-shared.md @@ -80,7 +80,13 @@ vocabulary beside the *link*. Every normal invocation is unaffected (absolute pa there. It is recorded because this repo does symlink files across repositories. **What is deliberately NOT shared.** `.claude/hooks/pretooluse-merge-consent.sh` names the words in -one operator-facing message string. It performs no classification of its own — it delegates to -`check-review-verdict.sh` — so it is a prose copy, not a second implementation, and sourcing a shell -library into a Claude hook to fix an error message would buy nothing for the risk. It is a known -residual, not an oversight. +**two** operator-facing strings — a header comment and the unrecognized-token message. It performs +no classification of its own (it delegates to `check-review-verdict.sh`), so these are prose copies, +not a second implementation, and sourcing a shell library into a Claude hook to fix an error message +would buy nothing for the risk. + +They are a known residual rather than an oversight — but one of the two had **already drifted**: the +header comment omitted `LGTM`, so the copy this record excuses was wrong on the day it was written. +It is corrected here. That is the argument for the residual and against it at the same time, and the +honest statement is that prose copies drift too; these are tolerated because they are inert, not +because they stay right. diff --git a/scripts/post-review-verdict.sh b/scripts/post-review-verdict.sh index ca1ef602d..2a7070ad3 100755 --- a/scripts/post-review-verdict.sh +++ b/scripts/post-review-verdict.sh @@ -75,10 +75,12 @@ etv_verdict_vocabulary_validate \ usage() { sed -n '/^# Usage:/,/^# ETV_GITEA_REPO/p' "$0" | sed 's/^# \{0,1\}//' + # Upper-cased to match `etv_verdict_words_display`, which the unknown-verdict error uses: an + # operator who has just been shown "use MERGEABLE, ..." should not then read "mergeable" here. printf '\nVerdict words (from %s):\n positive -> success : %s\n negative -> failure : %s\n' \ "lib/review-verdict-vocabulary.sh" \ - "$(etv_verdict_alternation positive | tr '|' ' ')" \ - "$(etv_verdict_alternation negative | tr '|' ' ')" + "$(etv_verdict_alternation positive | tr '|' ' ' | tr '[:lower:]' '[:upper:]')" \ + "$(etv_verdict_alternation negative | tr '|' ' ' | tr '[:lower:]' '[:upper:]')" exit 2 } diff --git a/scripts/tests/test_check_review_verdict.py b/scripts/tests/test_check_review_verdict.py index 743e23640..077ccd5d1 100644 --- a/scripts/tests/test_check_review_verdict.py +++ b/scripts/tests/test_check_review_verdict.py @@ -465,7 +465,7 @@ def test_each_verdict_word_retains_its_established_polarity(word, expected): both flags and precedence decides. WHICH DIRECTION THIS CATCHES, MEASURED RATHER THAN REASONED — the first draft of this docstring - asserted it and was checked afterwards, which is the wrong order. `check-review-verdict.sh:279` + asserted it and was checked afterwards, which is the wrong order. `check-review-verdict.sh:284` reads `if [ "$is_pos" = 1 ]; then head_pos=1; else head_neg=1; fi`, so `is_pos` wins per line and an overlapping word resolves POSITIVE. Both mutations were run against this test: diff --git a/scripts/tests/test_review_verdict_vocabulary.py b/scripts/tests/test_review_verdict_vocabulary.py index 63700e1cb..bfaeefb3e 100644 --- a/scripts/tests/test_review_verdict_vocabulary.py +++ b/scripts/tests/test_review_verdict_vocabulary.py @@ -400,12 +400,93 @@ def test_a_TRUNCATED_library_is_refused_rather_than_half_used(sandbox): assert result.stdout.strip() != "positive" -def test_the_sentinel_cannot_be_PRE_SET_from_the_environment(sandbox): - """Otherwise the data dependency would be forgeable by whoever sets the environment.""" - env = dict(sandbox.env) - env["ETV_VERDICT_VOCABULARY_OK"] = "1" +# The array assertion and the sentinel are NOT redundant, and this is where that is proved. +# +# `etv_verdict__is_array` removes the `set -u` abort, so with it in place validation returns 1 +# cleanly and the ordinary `if ! validate; then exit 2; fi` fires — which means every other refusal +# test in this file stays green even with the sentinel deleted. Cold review measured exactly that: +# all three sentinel guards removed, 150 tests still passing. +# +# So the sentinel is proved by DISARMING THE OTHER GUARD FIRST. With the array assertion gone the +# abort route reopens, the `if` is skipped, and the sentinel is the only thing left standing between +# a malformed word list and the regex. That is the #685 shape handled the way #685 says to handle +# it: delete one gate ALONE and require a red. +ARRAY_ASSERTION = ( + " for name in ETV_VERDICT_POSITIVE_WORDS ETV_VERDICT_NEGATIVE_WORDS; do\n" + ' if ! etv_verdict__is_array "$name"; then\n' + " printf 'review-verdict vocabulary: %s is not an array — declare it as " + '%s=(word word), not as a string\\n\' "$name" "$name" >&2\n' + " return 1\n" + " fi\n" + " done\n" +) + +SENTINEL_GUARD = """ if [ "${ETV_VERDICT_VOCABULARY_OK:-0}" != 1 ]; then + printf 'review-verdict vocabulary: not validated — refusing to hand out verdict words\\n' >&2 + return 1 + fi +""" + + +def _disarm_array_assertion(sandbox): + """Reopen the `set -u` abort route by removing the assertion that prevents it.""" + text = sandbox.vocabulary.read_text() + assert ARRAY_ASSERTION in text, "the array assertion moved; this proof no longer disarms it" + sandbox.vocabulary.write_text(text.replace(ARRAY_ASSERTION, "")) + + +def test_MUTATION_with_the_array_assertion_disarmed_the_SENTINEL_still_refuses(sandbox): + """The sentinel is load-bearing, not decoration. + + Disarm the array assertion, then declare the negative list as a scalar: `${#arr[@]}` aborts the + validator mid-way, `if ! validate` takes neither branch, and the `exit 2` never runs. Only the + sentinel — never assigned, because validation did not reach its last line — stops the words. + """ + _disarm_array_assertion(sandbox) sandbox.edit_vocabulary(NEGATIVE_DECL, 'ETV_VERDICT_NEGATIVE_WORDS="blocked"') + result = sandbox.read_side("BLOCKED") + assert result.stdout.strip() != "positive" + assert result.returncode == 2 + assert "not validated" in result.stderr, ( + "expected the SENTINEL's refusal specifically; a refusal from some other clause would not " + f"prove the sentinel does anything. stderr={result.stderr!r}" + ) + + +def test_MUTATION_removing_BOTH_guards_reopens_the_original_fail_open(sandbox): + """The negative control for the test above — without it, that test proves only 'it refuses'. + + With the array assertion AND the sentinel both gone, this is the tree as first committed, and + the measured fail-open comes back: an explicit BLOCKED classifies `positive`, exit 0. Pinning it + here means the pair cannot both be deleted without a red. + """ + _disarm_array_assertion(sandbox) + text = sandbox.vocabulary.read_text() + assert text.count(SENTINEL_GUARD) == 3, f"expected 3 sentinel guards, found {text.count(SENTINEL_GUARD)}" + sandbox.vocabulary.write_text(text.replace(SENTINEL_GUARD, "")) + + sandbox.edit_vocabulary(POSITIVE_DECL, 'ETV_VERDICT_POSITIVE_WORDS=(mergeable ".*")') + sandbox.edit_vocabulary(NEGATIVE_DECL, 'ETV_VERDICT_NEGATIVE_WORDS="blocked"') + + result = sandbox.read_side("BLOCKED") + assert (result.returncode, result.stdout.strip()) == (0, "positive"), ( + "the historical fail-open no longer reproduces with both guards removed — this control is " + f"no longer measuring what it claims. got rc={result.returncode} out={result.stdout!r}" + ) + + +def test_an_inherited_sentinel_cannot_stand_in_for_validation(sandbox): + """Reset-at-load is what makes the sentinel unforgeable from the environment. + + Tested with the array assertion disarmed, so the abort route is open and the inherited value is + the ONLY thing that could carry a malformed vocabulary through. + """ + _disarm_array_assertion(sandbox) + sandbox.edit_vocabulary(NEGATIVE_DECL, 'ETV_VERDICT_NEGATIVE_WORDS="blocked"') + + env = dict(sandbox.env) + env["ETV_VERDICT_VOCABULARY_OK"] = "1" result = subprocess.run( ["bash", str(sandbox.reader), "--head", SHA_A], input=json.dumps([{"body": f"Review-verdict: BLOCKED @ {SHA_A}"}]), @@ -415,18 +496,11 @@ def test_the_sentinel_cannot_be_PRE_SET_from_the_environment(sandbox): ) assert result.returncode == 2, "an inherited sentinel must not stand in for validation" assert result.stdout.strip() != "positive" + assert "not validated" in result.stderr -def test_a_VALID_vocabulary_still_sets_the_sentinel_and_classifies(sandbox): - """The positive control for every refusal above: the guard must not simply always refuse.""" - env = dict(sandbox.env) - env["ETV_VERDICT_VOCABULARY_OK"] = "1" - result = subprocess.run( - ["bash", str(sandbox.reader), "--head", SHA_A], - input=json.dumps([{"body": f"Review-verdict: BLOCKED @ {SHA_A}"}]), - env=env, - capture_output=True, - text=True, - ) +def test_a_VALID_vocabulary_still_classifies(sandbox): + """The positive control for every refusal above: the guards must not simply always refuse.""" + result = sandbox.read_side("BLOCKED") assert result.returncode == 0 assert result.stdout.strip() == "negative" -- 2.47.3 From 4dd15523c93c13750ffa3d6321cf92b2ee83c7b1 Mon Sep 17 00:00:00 2001 From: Timothy Date: Wed, 26 Aug 2026 21:35:04 +0200 Subject: [PATCH 4/6] fix(788): delete the count instead of correcting it a fourth time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-3 review found no fail-open — it attacked the core hard (inherited sentinel, env override, missing/truncated/exiting library, regex injection, scalar list, both bash 5.3 and 3.2, a 2612-payload differential fuzz against origin/main with 0 diffs, a 12-mode write-side matrix with 0 posts on failure) and everything failed closed. What it broke was accuracy, again. **The count of prose vocabulary copies was wrong for the third time.** One draft said one string, the next said two, the third said two while a third string sat four lines away in the same file. Each reported the copies someone had noticed. The sweeps behind them anchored their pattern on `MERGEABLE`, so an enumeration of the negative words alone — `(BLOCKED/NOT-MERGEABLE)` — was invisible to every one of them. So the count is gone rather than corrected. A count is a second copy of the list and rots exactly like one, which is the argument this whole record is built on; writing a fourth number would be making the record's own mistake inside the record. The residual paragraph now carries the derivation instead — a sweep matching a run of any two adjacent vocabulary words in either polarity, which is what the earlier patterns could not see. Corrected with it: `CLAUDE.md` and `release.review-verdict-gate` (twice, so the catalog too) both enumerated the vocabulary without `LGTM`, a word the code has accepted since #629. The record a reader consults for this gate was teaching a vocabulary missing a live word. **The sentinel proof also overclaimed.** The comment said "delete one gate ALONE and require a red", and that is true of exactly one of the three sentinel guards. Measured: deleting the READ side's (`etv_verdict_alternation`) reddens the three behavioural tests; deleting either write-side copy reddens only the `count == 3` tripwire. The write side's two are defence-in-depth — `post-review-verdict.sh` has `set -e`, so the abort that defeats the reader kills the writer outright (rc=1, nothing posted). They are kept, because the writer should not depend on `set -e` staying, but their proof is a COUNT and the comment now says so. Correcting forward rather than rewriting history: the previous commit's message claims "the two SC1090s on the hook are pre-existing". There is ONE, at `pretooluse-merge-consent.sh:51`, and only one dynamic-source construct exists in the file — the earlier count came from grepping shellcheck output, which repeats the code on its "For more information" line. Rebased onto ba6a4b08a; #842 and #843 landed from parallel sessions during review. Verification: 989 passed / 2 skipped, ruff clean, shellcheck clean, catalog regenerated, decisions-validate OK, and the documented sweep command executed. Refs: #788 Decisions-Edit: yes Co-authored-by: Timothy --- CLAUDE.md | 2 +- docs/decisions/README.md | 2 +- .../records/release/review-verdict-gate.md | 4 +- .../release/verdict-vocabulary-shared.md | 37 ++++++++++++++----- .../tests/test_review_verdict_vocabulary.py | 12 +++++- 5 files changed, 41 insertions(+), 16 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 5d476bf6c..b53a5fa3a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -83,7 +83,7 @@ main in) and re-run the local gate whenever the fetch shows movement. 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 + 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) — **auto-grants** a merge (emits `permissionDecision: allow`, so **no** redundant mechanical prompt fires) 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). On the auto-grant (satisfied) path the derived state **is** the consent — do not also ask conversationally to merge; a separate human confirmation is warranted only when the gate **asks** (ersatztv#314). **The H10 review-verdict convention**: after an adversarial/Codex review of a PR (or its latest fix commit), run **`scripts/post-review-verdict.sh [note]`** — it posts both the `Review-verdict: … @ ` comment and the sha-bound `review-verdict/h10` commit status, proving the *latest* commit was reviewed rather than a stale earlier diff (ersatztv#242). Do not hand-write the comment: the **status** is the required check branch protection enforces, and a comment alone leaves it absent. +- `pretooluse-merge-consent.sh` (Claude PreToolUse on the Gitea merge tool) — **auto-grants** a merge (emits `permissionDecision: allow`, so **no** redundant mechanical prompt fires) 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). On the auto-grant (satisfied) path the derived state **is** the consent — do not also ask conversationally to merge; a separate human confirmation is warranted only when the gate **asks** (ersatztv#314). **The H10 review-verdict convention**: after an adversarial/Codex review of a PR (or its latest fix commit), run **`scripts/post-review-verdict.sh [note]`** — it posts both the `Review-verdict: … @ ` comment and the sha-bound `review-verdict/h10` commit status, proving the *latest* commit was reviewed rather than a stale earlier diff (ersatztv#242). Do not hand-write the comment: the **status** is the required check branch protection enforces, and a comment alone leaves it absent. - **The gate is enforced server-side, per sha (ersatztv#622).** `review-verdict/h10` is a required status check on `main`. Because a commit status belongs to one sha, a commit pushed *after* an auto-merge is scheduled clears it and blocks the merge — closing the hole where `merge_when_checks_succeed` froze consent at scheduling time and Gitea later merged an unreviewed head. Renovate-authored and docs-only PRs are auto-passed by `.gitea/workflows/review-verdict.yml`, **except** when they touch `.claude/`, `.codex/`, `.gitea/`, `.husky/`, `scripts/` or `docker/ci/`. See `docs/ci-cd.md` → Review-verdict gate. - `.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. **Since ersatztv#743 that push can no longer happen at all** (see below), so this hook is now belt-and-braces for a path the server refuses. diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 51ae1a9ed..d144276be 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -139,7 +139,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, 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.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_target` workflow auto-passes the two exempt classes (Renovate-authored, docs-only) unless the PR touches a protected path (`.claude/`, `.codex/`, `.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) | | `release.verdict-vocabulary-shared` | The H10 verdict words live in exactly one place, `scripts/lib/review-verdict-vocabulary.sh`, as two arrays. `scripts/post-review-verdict.sh` (write: word to commit-status state) classifies through `etv_verdict_class`; `scripts/check-review-verdict.sh` (read: comment to classification) builds `POS_RE`/`NEG_RE` from `etv_verdict_alternation`. Neither script may restate a word, and neither may enumerate the vocabulary in a usage banner or an error string. Every word is validated to `[a-z][a-z-]*` before it reaches a regex. Validation is enforced by a DATA dependency, not a control-flow check: `etv_verdict_vocabulary_validate` sets `ETV_VERDICT_VOCABULARY_OK=1` on its final line, and the derived views refuse to hand out words without it. A vocabulary that is missing, unreadable, truncated, exits at top level, is declared as a scalar, or fails validation yields NO words on either side — exit 1 for the writer (nothing posted), exit 2 for the reader (callers fail closed). Do NOT reintroduce a text-comparison parity test alongside this. | 2026-08-26 | [link](records/release/verdict-vocabulary-shared.md) | | `release.verdict-writes-status-before-comment` | `scripts/post-review-verdict.sh` writes the sha-bound `review-verdict/h10` commit status FIRST and the human-readable `Review-verdict:` comment SECOND. Every refusal path still refuses (fail-closed, unchanged) and exits non-zero, and none of them may leave a verdict comment behind. An orphaned comment is therefore PREVENTED rather than tolerated. If the comment write fails after the status was written, that is an error too, but it degrades to an `ask` at the merge gate rather than to an apparent grant. | 2026-08-22 | [link](records/release/verdict-writes-status-before-comment.md) | diff --git a/docs/decisions/records/release/review-verdict-gate.md b/docs/decisions/records/release/review-verdict-gate.md index 89c20d7f3..e078c4807 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, 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.' +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)' --- @@ -19,7 +19,7 @@ Gitea-auth-from-env (no second hook → no detection drift, per the #303 methodo 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). +`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 diff --git a/docs/decisions/records/release/verdict-vocabulary-shared.md b/docs/decisions/records/release/verdict-vocabulary-shared.md index 644ab2b66..5128a6f4f 100644 --- a/docs/decisions/records/release/verdict-vocabulary-shared.md +++ b/docs/decisions/records/release/verdict-vocabulary-shared.md @@ -79,14 +79,31 @@ vocabulary beside the *link*. Every normal invocation is unaffected (absolute pa `PATH`, a path containing spaces — all measured), and the case fails CLOSED when no library sits there. It is recorded because this repo does symlink files across repositories. -**What is deliberately NOT shared.** `.claude/hooks/pretooluse-merge-consent.sh` names the words in -**two** operator-facing strings — a header comment and the unrecognized-token message. It performs -no classification of its own (it delegates to `check-review-verdict.sh`), so these are prose copies, -not a second implementation, and sourcing a shell library into a Claude hook to fix an error message -would buy nothing for the risk. +**What is deliberately NOT shared — and why this paragraph names no number.** Nothing outside +`review-verdict-vocabulary.sh` CLASSIFIES a verdict word; that part is closed. But the vocabulary is +also *written out* in prose, in the merge-consent hook's operator-facing strings, in `CLAUDE.md`, +and in `release.review-verdict-gate` (whence the catalog). Those are inert — none of them decides +anything — and sourcing a shell library into a Claude hook or a Markdown file to fix an error +message would buy nothing for the risk. So they stay. -They are a known residual rather than an oversight — but one of the two had **already drifted**: the -header comment omitted `LGTM`, so the copy this record excuses was wrong on the day it was written. -It is corrected here. That is the argument for the residual and against it at the same time, and the -honest statement is that prose copies drift too; these are tolerated because they are inert, not -because they stay right. +Several of them had **already drifted**, omitting `LGTM`, a word the code has accepted since #629 — +including the record a reader consults for this gate. They are corrected here. + +The count is deliberately absent. Three successive drafts of this paragraph gave one, and all three +were wrong: "one string", then "two", then "two" again while a third sat four lines away in the same +file. Each draft reported the copies someone had noticed. A count is a second copy of the list and +rots exactly like one, which is the argument this whole record is built on — so it does not get +written down here. Re-derive it instead: + +```bash +git ls-files | grep -v '^docs/superpowers/' | while read -r f; do + [ -f "$f" ] && grep -noE \ + '\b(MERGEABLE|APPROVED|LGTM|BLOCKED|NOT-MERGEABLE)([|/](MERGEABLE|APPROVED|LGTM|BLOCKED|NOT-MERGEABLE))+' \ + "$f" | sed "s|^|$f:|" +done +``` + +The earlier sweeps missed sites because their pattern anchored on `MERGEABLE`, so an enumeration of +the negative words alone was invisible to it. The pattern above matches a run of any two adjacent +vocabulary words, in either polarity. `docs/superpowers/**` is excluded as historical session +artifacts (#812 is deciding whether the docs rules reach them at all). diff --git a/scripts/tests/test_review_verdict_vocabulary.py b/scripts/tests/test_review_verdict_vocabulary.py index bfaeefb3e..8c05b562e 100644 --- a/scripts/tests/test_review_verdict_vocabulary.py +++ b/scripts/tests/test_review_verdict_vocabulary.py @@ -409,8 +409,16 @@ def test_a_TRUNCATED_library_is_refused_rather_than_half_used(sandbox): # # So the sentinel is proved by DISARMING THE OTHER GUARD FIRST. With the array assertion gone the # abort route reopens, the `if` is skipped, and the sentinel is the only thing left standing between -# a malformed word list and the regex. That is the #685 shape handled the way #685 says to handle -# it: delete one gate ALONE and require a red. +# a malformed word list and the regex. +# +# EXACTLY WHICH COPY THAT PROVES, stated because the obvious reading overclaims. There are three +# sentinel guards, one per derived view, and only the READ side's (`etv_verdict_alternation`) has a +# behavioural alone-deletion red — measured: deleting it alone reddens the three tests below, +# deleting either of the other two reddens only the `count == 3` tripwire further down. The write +# side's two are defence-in-depth: `post-review-verdict.sh` runs under `set -e`, so the abort that +# defeats the read side kills the writer outright (measured: rc=1, nothing posted). They are kept +# because the writer should not depend on `set -e` staying, but their current proof is a COUNT, not +# a behaviour, and saying otherwise would be the round-2 defect one level down. ARRAY_ASSERTION = ( " for name in ETV_VERDICT_POSITIVE_WORDS ETV_VERDICT_NEGATIVE_WORDS; do\n" ' if ! etv_verdict__is_array "$name"; then\n' -- 2.47.3 From 87d2bd8450803c872b5b11b5764c03afe7cf0f1e Mon Sep 17 00:00:00 2001 From: Timothy Date: Wed, 26 Aug 2026 22:06:41 +0200 Subject: [PATCH 5/6] fix(788): withdraw the completeness claim instead of widening the pattern again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-4 review found no fail-open — 7560-payload differential fuzz against origin/main with 0 diffs, 28 hostile-vocabulary cases, environment poisoning, and a 34-case write-side matrix, all fail-closed — and verified every other claim in the previous commit exactly. It found one thing, and it is the same thing for the fourth time. The derivation that replaced the rotting count could not see the generated catalog. `docs/decisions/README.md` is a Markdown table, so it escapes its pipes (`MERGEABLE\|APPROVED\|...`), and the pattern required the separator to follow the word directly. That site is one this branch had just repaired, and the sweep reported it clean — while the sentence beneath asserted the pattern "matches a run of any two adjacent vocabulary words, in either polarity". Widening the separator to `\\?[|/]` finds it, and that is done. But widening alone would be the fifth attempt to assert a completeness nobody has established: three counts, then a pattern, now a wider pattern. The comma-separated list inside `etv_verdict_words_display` is still invisible to it, and will stay invisible, because a string predicate over prose does not converge — the same finding #774 recorded for a regex over shell source (`testing.guard-derives-population-from-source`), which is the record this one cites. So the completeness claim is withdrawn rather than re-asserted. The record now says what the sweep actually does — finds separator-joined enumerations, is better than a number because it can be re-run, and is NOT evidence that no other copy exists — and names the catalog omission as the worked example of why. The safety property never rested on it: nothing outside `review-verdict-vocabulary.sh` classifies a word, which is what makes the prose copies inert. Also: the sweep matched its own record's text, so two of its ten hits were the tool looking at itself while a real site was missing. The record now excludes itself, and the command was run verbatim as a reader would — it returns nine sites, all real, including the catalog. Plus a grammar slip in a comment added two commits ago (`check-review-verdict.sh`): the sentence lists the anchors and the boundary, but read as a singular subject with a plural verb. Verification: 989 passed / 2 skipped, ruff clean, shellcheck clean, catalog regenerated, decisions-validate OK, 0 commits behind origin/main. Refs: #788 Decisions-Edit: yes Co-authored-by: Timothy --- .../release/verdict-vocabulary-shared.md | 26 ++++++++++++++----- scripts/check-review-verdict.sh | 4 +-- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/docs/decisions/records/release/verdict-vocabulary-shared.md b/docs/decisions/records/release/verdict-vocabulary-shared.md index 5128a6f4f..211ad9aa9 100644 --- a/docs/decisions/records/release/verdict-vocabulary-shared.md +++ b/docs/decisions/records/release/verdict-vocabulary-shared.md @@ -96,14 +96,28 @@ rots exactly like one, which is the argument this whole record is built on — s written down here. Re-derive it instead: ```bash -git ls-files | grep -v '^docs/superpowers/' | while read -r f; do +git ls-files | grep -vE '^docs/superpowers/|verdict-vocabulary-shared\.md$' | while read -r f; do [ -f "$f" ] && grep -noE \ - '\b(MERGEABLE|APPROVED|LGTM|BLOCKED|NOT-MERGEABLE)([|/](MERGEABLE|APPROVED|LGTM|BLOCKED|NOT-MERGEABLE))+' \ + '\b(MERGEABLE|APPROVED|LGTM|BLOCKED|NOT-MERGEABLE)((\\?[|/])(MERGEABLE|APPROVED|LGTM|BLOCKED|NOT-MERGEABLE))+' \ "$f" | sed "s|^|$f:|" done ``` -The earlier sweeps missed sites because their pattern anchored on `MERGEABLE`, so an enumeration of -the negative words alone was invisible to it. The pattern above matches a run of any two adjacent -vocabulary words, in either polarity. `docs/superpowers/**` is excluded as historical session -artifacts (#812 is deciding whether the docs rules reach them at all). +**This sweep is a starting point, not a completeness proof, and the difference is the whole lesson +of this record.** It finds vocabulary words joined by a bare or markdown-escaped separator (`|`, +`/`, `\|`). It does NOT find an enumeration written any other way — the comma-separated list inside +`etv_verdict_words_display` is invisible to it, and so was the generated catalog until the `\\?` was +added, because a Markdown table escapes its pipes. That omission was not hypothetical: the catalog +is a site this record had just repaired, and the sweep reported it as clean. + +Each earlier attempt at this — three counts, then a pattern — failed the same way, by asserting a +completeness its author had not established. A string predicate over prose does not converge, which +is the same finding #774 recorded for a regex over shell source (see +`testing.guard-derives-population-from-source`). So the honest statement is the narrow one: this +finds separator-joined enumerations, it is better than a number because it can be re-run, and it is +not evidence that no other copy exists. The safety property does not rest on it — nothing outside +`review-verdict-vocabulary.sh` classifies a word, and that is what makes these copies inert. + +`docs/superpowers/**` is excluded as historical session artifacts (#812 is deciding whether the docs +rules reach them at all), and this record excludes itself, since the pattern above matches its own +text. diff --git a/scripts/check-review-verdict.sh b/scripts/check-review-verdict.sh index 459994ebd..4893b84da 100755 --- a/scripts/check-review-verdict.sh +++ b/scripts/check-review-verdict.sh @@ -237,8 +237,8 @@ FIELD_RE='^review-verdict:[[:space:]]*[A-Za-z][A-Za-z-]*[[:space:]]*@[[:space:]] # The WORD SET comes from the shared vocabulary; the GRAMMAR around it stays here (ersatztv#788). # That split is the point: every false-open in this file's history was in the grammar — the column-0 # anchor, the whole-word boundary, the anchored `@ ` field — and none of it belongs in a word -# list. `etv_verdict_alternation` emits only the alternation BODY (`word|word|word`), and the -# `([[:space:]@]|$)` boundary below are unchanged. +# list. `etv_verdict_alternation` emits only the alternation BODY (`word|word|word`); the anchors +# and the `([[:space:]@]|$)` boundary below are unchanged. # # Interpolating into an ERE is safe because every word is restricted to `[a-z][a-z-]*`, so no # character reaching this regex can be a metacharacter. Without that a `.*` in the positive list -- 2.47.3 From e66196fd248ca710882a1489ea926d790d69cb51 Mon Sep 17 00:00:00 2001 From: Timothy Date: Wed, 26 Aug 2026 22:27:12 +0200 Subject: [PATCH 6/6] docs(788): two precision fixes in the vocabulary record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 5 returned MERGEABLE with no must-fix items. These are its two nits, taken because this branch's failure mode has been imprecise prose and leaving a known imprecision to save a round would be the same trade that produced the previous five. * The comma-separated enumeration the sweep cannot see is in the doc COMMENT above `etv_verdict_words_display`, not inside it — the body builds its list from the data. Verified: the comment is at review-verdict-vocabulary.sh:195, the function opens at :198. * The separator parenthetical omitted `\/`, which `(\\?[|/])` also accepts. That was an understatement rather than an overclaim, and it is the one direction this record can afford, but it is still not what the pattern does. Verified by feeding `MERGEABLE\/APPROVED` to the pattern: it matches. No executable line changed. The sweep still runs verbatim and still returns nine real sites including the catalog; catalog regenerated; decisions-validate OK; 989 passed / 2 skipped. Refs: #788 Decisions-Edit: yes Co-authored-by: Timothy --- .../records/release/verdict-vocabulary-shared.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/decisions/records/release/verdict-vocabulary-shared.md b/docs/decisions/records/release/verdict-vocabulary-shared.md index 211ad9aa9..68012363d 100644 --- a/docs/decisions/records/release/verdict-vocabulary-shared.md +++ b/docs/decisions/records/release/verdict-vocabulary-shared.md @@ -104,11 +104,11 @@ done ``` **This sweep is a starting point, not a completeness proof, and the difference is the whole lesson -of this record.** It finds vocabulary words joined by a bare or markdown-escaped separator (`|`, -`/`, `\|`). It does NOT find an enumeration written any other way — the comma-separated list inside -`etv_verdict_words_display` is invisible to it, and so was the generated catalog until the `\\?` was -added, because a Markdown table escapes its pipes. That omission was not hypothetical: the catalog -is a site this record had just repaired, and the sweep reported it as clean. +of this record.** It finds vocabulary words joined by a bare or markdown-escaped separator (`|`, `/`, +`\|`, `\/`). It does NOT find an enumeration written any other way — the comma-separated list in the +comment above `etv_verdict_words_display` is invisible to it, and so was the generated catalog until +the `\\?` was added, because a Markdown table escapes its pipes. That omission was not hypothetical: +the catalog is a site this record had just repaired, and the sweep reported it as clean. Each earlier attempt at this — three counts, then a pattern — failed the same way, by asserting a completeness its author had not established. A string predicate over prose does not converge, which -- 2.47.3