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/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/ci-cd.md b/docs/ci-cd.md index cf1b21cc0..66155d615 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -1237,6 +1237,33 @@ 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 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` (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..d144276be 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -139,8 +139,9 @@ 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) | | `rulebuilder.relative-date-macros` | The visual rule builder's `inLast`/`notInLast` date operators compile to/parse from the pre-existing `CustomMultiFieldQueryParser` macros `released_inthelast`/`released_notinthelast` and `added_inthelast`/`added_notinthelast`, value form `" day\|week\|month\|year"`; there is no backend change. | 2026-07-23 | [link](records/rulebuilder/relative-date-macros.md) | | `scan.collections-scan-status` | `GET /api/v1/media-sources/collections-scan-status` reports a family-global (not per-source), boolean-only active-scan set read from `IEntityLocker`; the SPA reconciles authoritatively against it (with a grace-tick helper) instead of a fixed client-side timeout. | 2026-07-12 | [link](records/scan/collections-scan-status.md) | diff --git a/docs/decisions/records/release/review-verdict-gate.md b/docs/decisions/records/release/review-verdict-gate.md index 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 new file mode 100644 index 000000000..68012363d --- /dev/null +++ b/docs/decisions/records/release/verdict-vocabulary-shared.md @@ -0,0 +1,123 @@ +--- +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. 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' +--- + +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 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 +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. + +**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 — 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. + +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 -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))+' \ + "$f" | sed "s|^|$f:|" +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 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 +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/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..4893b84da 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,56 @@ 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`. +# +# 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 + comments=$(cat) [ -n "$comments" ] || { printf 'check-review-verdict: empty comments payload on stdin\n' >&2; exit 2; } @@ -181,8 +233,28 @@ 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`); 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 +# 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) || { + 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..dc12aa2d0 --- /dev/null +++ b/scripts/lib/review-verdict-vocabulary.sh @@ -0,0 +1,231 @@ +# 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) + +# --- 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 +# 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. +# 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 + + 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 + + # 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 +} + +# --- 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 + 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 + 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=() + 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[@]}"}) ;; + 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='' + 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[@]}"} + 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" +} + +# --- 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 d6cc0ddea..2a7070ad3 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,44 @@ 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" +# 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" + 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 '|' ' ' | tr '[:lower:]' '[:upper:]')" \ + "$(etv_verdict_alternation negative | tr '|' ' ' | tr '[:lower:]' '[:upper:]')" exit 2 } @@ -51,26 +90,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_check_review_verdict.py b/scripts/tests/test_check_review_verdict.py index 2c7c6f4ed..077ccd5d1 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: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 new file mode 100644 index 000000000..8c05b562e --- /dev/null +++ b/scripts/tests/test_review_verdict_vocabulary.py @@ -0,0 +1,514 @@ +"""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" + + +@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: + 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 + + +# --- 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" + + +# 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. +# +# 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' + " 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}"}]), + 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" + assert "not validated" in result.stderr + + +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"