From afe61e80aae47af4df64842225e7cc2e5130341f Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 29 Aug 2026 23:02:24 +0200 Subject: [PATCH 01/17] fix(845): the verdict writer checks that the gate will honour what it just posted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `.gitea/workflows/review-verdict.yml` inherits an existing `review-verdict/h10=success` only from a status whose `.creator.login` is on its `H10_REVIEWERS` allow-list (#742). `scripts/post-review-verdict.sh` writes those verdicts with whatever account owns `ETV_GITEA_TOKEN`/`ETV_GITEA_BASICAUTH` and never asked whose it was. The two values were coupled with nothing asserting the coupling, and the failure mode was the silent one: the status really is written, the tool really does report success, and then the next `pull_request_target` event re-derives it and posts over it. And again. The PR deadlocks and the only diagnostic is a `::warning::` inside a workflow run nobody is reading. The script now READS THE STATUS BACK after posting it, identifies its own write by state and description, and refuses — before the comment, so the surviving half-state is the documented `ask` one — unless the recorded creator is on the allow-list. Measured after the write rather than probed before it: a pre-flight `GET /user` tests what the credential claims to be, this tests what Gitea recorded as the author of this status, which is the value the gate reads. It also needs no scope beyond the repo access the POST already required. The allow-list is not restated. `scripts/lib/h10-reviewers.sh` derives it from the workflow's own literal, so there is ONE declaration rather than two held together by a parity test — the shape `testing.guard-derives-population-from-source` records six failed rounds against. It is a parse rather than a shared declaration both sides source, and that is not a preference: the gate runs against a checkout of the PR's BASE sha, so a PR whose base predates a new `scripts/lib/` file would not have it, and a missing `source` under `set -euo pipefail` kills the job — which posts no `review-verdict/h10` at all and blocks every merge to `main` including the PR that would repair it, with no admin override (#743). A fallback list for that case reintroduces the second copy. So the derivation runs on the side that can fail safely and the gate's body is untouched. Fail-closed on every branch of the derivation: 0 or >1 anchored assignments, an entry that is not a plain login, an empty list, or an unreadable file all refuse BEFORE any network call. The membership query answers member / not-a-member / cannot-tell as three distinct codes rather than two, so a caller cannot fold "cannot tell" into either. The safety DIRECTION is unchanged and the commit does not claim otherwise: a non-allow-listed verdict was always a stall, never a fail-open. What changes is where the signal lands — at the terminal that made the mistake, instead of one PR event later. Residual, stated rather than fixed: the check runs after the POST, so a refused verdict leaves a non-inheritable status standing with no comment beside it. That is the `ask` half-state `release.verdict-writes-status-before-comment` already designates as the safe one, and the gate re-derives it on the next PR event. A second corrective write is the sticky-sentinel mechanism #849 is separately designing; inventing a parallel one here would be two mechanisms for one invariant. `scripts/post-review-verdict.sh` moves BEHAVIOUR-ONLY -> MUTATION in the guard inventory, which the manifest's own UNDECLARED note called "the most valuable upgrade on this list". The declared clause is in the GATE, not the script: rewriting `H10_REVIEWERS` while the posting account stays fixed reddens the accept path only if the writer reads the list live AND the comparison gates the outcome. Disarming the comparison instead would prove the second and say nothing about the first, which is the half that was missing. Verification: full `scripts/tests` suite green (1239 passed, 2 skipped); the declared mutation executes and reddens its named proof with the declared diagnostic; four clauses disarmed by hand and each confirmed to redden its own test. fixes #845 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW --- CLAUDE.md | 2 +- docs/ci-cd.md | 25 +- docs/decisions/README.md | 2 +- .../records/ci/exemption-provenance.md | 57 ++-- docs/guard-inventory.md | 4 +- docs/remote-state-inventory.md | 3 +- scripts/lib/h10-reviewers.sh | 189 ++++++++++++++ scripts/post-review-verdict.sh | 94 ++++++- scripts/tests/mutation_manifest.py | 19 +- scripts/tests/test_post_review_verdict.py | 247 ++++++++++++++++++ .../tests/test_review_verdict_vocabulary.py | 8 + 11 files changed, 616 insertions(+), 34 deletions(-) create mode 100644 scripts/lib/h10-reviewers.sh diff --git a/CLAUDE.md b/CLAUDE.md index bc8ae1f44..866b68140 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. **The credential you post with must be an account on `H10_REVIEWERS` in `.gitea/workflows/review-verdict.yml`** (`timothy` today) — since ersatztv#742 the gate inherits an existing `success` only from an allow-listed creator (an existing `failure` is left alone on a weaker attributability test, so an attributable rejection VISIBLE AT THE FIRST READ is not re-derived into a green — a rejection landing later, inside a run's own write window, is a separate and still-open route, ersatztv#849), so a verdict posted with any other account is written, reported as success by the script, and then silently re-derived on the next PR event (ersatztv#845). +- `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 credential you post with must be an account on `H10_REVIEWERS` in `.gitea/workflows/review-verdict.yml`** (`timothy` today) — since ersatztv#742 the gate inherits an existing `success` only from an allow-listed creator (an existing `failure` is left alone on a weaker attributability test, so an attributable rejection VISIBLE AT THE FIRST READ is not re-derived into a green — a rejection landing later, inside a run's own write window, is a separate and still-open route, ersatztv#849), and since ersatztv#845 the script ENFORCES that coupling rather than assuming it: it reads its own status back and refuses, before writing the verdict comment, unless the recorded `.creator.login` is on that allow-list — so a verdict posted with any other account fails loudly at your terminal instead of being written, reported as success, and then silently re-derived on the next PR event. The allow-list is derived from the workflow by `scripts/lib/h10-reviewers.sh`; it is never restated. - **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 1bce581f5..f9429a157 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -1612,12 +1612,25 @@ account to `H10_REVIEWERS` in `.gitea/workflows/review-verdict.yml`. (A separate annotation carrying the same three values was tried and withdrawn: its only delta was a severity whose rendering could not be probed, and it needed a guard of its own to stop contradicting the line above it.) `post-review-verdict.sh` posts -with whatever account owns `ETV_GITEA_TOKEN`/`ETV_GITEA_BASICAUTH` and does **not** check that it is an -allow-listed one (ersatztv#845). A changed writer credential does not fail loudly — it reports success, -and each *subsequent* PR event re-derives the status: to `pending` on a PR that needs a verdict, or to -an exemption **`success`** on a docs-only or Renovate-manifest one, which is the quieter and worse -half. A verdict posted after the last event stands until the next one, so the symptom is a PR that -keeps losing its verdict rather than an immediate refusal. +with whatever account owns `ETV_GITEA_TOKEN`/`ETV_GITEA_BASICAUTH`, and since ersatztv#845 it +**checks that it is an allow-listed one** — at the terminal that made the mistake, not one PR event +later. It reads the status back after posting, identifies its own write by state and description, and +refuses **before writing the comment** unless `.creator.login` is a member; a `creator: null` +(Actions-token) status gets its own message, since there is no account to name. The allow-list is +DERIVED from the `H10_REVIEWERS` literal in `review-verdict.yml` by `scripts/lib/h10-reviewers.sh`, +not restated beside it, so there is one declaration rather than two plus a parity test. + +What that changes is the *diagnostic*, not the safety direction: a non-allow-listed verdict was +always a stall, never a fail-open. Before, a changed writer credential reported success and each +*subsequent* PR event re-derived the status — to `pending` on a PR that needs a verdict, or to an +exemption **`success`** on a docs-only or Renovate-manifest one, which is the quieter and worse +half — so the symptom was a PR that kept losing its verdict rather than an immediate refusal. Now it +is an immediate refusal naming the account and the allow-list. **The residual, stated:** the check +runs after the POST, so a refused verdict leaves a non-inheritable status standing on the head with +no comment beside it. That is the `ask` half-state `release.verdict-writes-status-before-comment` +already designates as the safe one, and the gate re-derives the status itself on the next PR event; +it is deliberately not repaired here, because a second corrective write is the sticky-sentinel +mechanism ersatztv#849 is separately designing. This remains a *provenance* check, not an authentication one — an **allow-listed** collaborator's own write-scoped token can still POST a verdict they did not form, branch protection binds the context diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 5feb5f1a4..a4cad78f5 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -46,7 +46,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.decisions-lifecycle-flake` | When `decisions lifecycle` is the **only** red job, do not investigate and do not create a new run to clear it — no rebase, no `--amend`, no no-op push; the operator reruns that single job from the Gitea UI. | 2026-07-21 | [link](records/ci/decisions-lifecycle-flake.md) | | `ci.docs-only-detect-shallow-safe` | The docs-only detect script must diff against `FETCH_HEAD` (always resolves after `git fetch`, even shallow) using a two-dot tree diff — not `origin/` with three-dot — because a `fetch-depth: 1` shallow clone has no remote-tracking ref and no merge-base, which silently fails the original detect into `docs_only=false` (full matrix, no functional error). A CI-behavior change must be verified by measuring the effect (job durations), not just a green check. | 2026-07-17 | [link](records/ci/docs-only-detect-shallow-safe.md) | | `ci.docs-only-skip-steps` | A docs-only change must still run every required job (`test`, `migrations`) so their commit-status contexts always report; each heavy job runs `scripts/ci-detect-docs-only.sh` first and gates its real STEPS on `if: steps.detect.outputs.docs_only != 'true'`, never `if:`-skips the whole job (an `if:`-skipped job reports `skipped`, not `success`, which branch protection may never unblock on). Detection biases toward running more on any doubt. | 2026-07-17 | [link](records/ci/docs-only-skip-steps.md) | -| `ci.exemption-provenance` | The three inputs the exemption decision rests on must each be bound to something the judged PR cannot mutate. (1) BASE — `scripts/pr-changed-files.sh` takes the expected base BRANCH as a REQUIRED 5th argument and re-reads it before and after paging, because `/pulls/{n}/files` diffs against the PR's live base and retargeting moves the answer without moving the head sha; the workflow passes `github.event.pull_request.base.ref` from the `pull_request_target` payload, which a retarget cannot rewrite. (2) BOT EXEMPTION — an author match is necessary but never sufficient: `pull_request.user.login` is the PR's immutable CREATOR while its head is not, so the exemption additionally requires EVERY changed path to be a dependency manifest (`Directory.Packages.props` or `.config/dotnet-tools.json`, and ONLY those — the npm manifests are excluded because `package.json` `scripts` are executed by CI). (3) INHERITED SUCCESS — the never-overwrite short-circuit fires for a `success` only when it is POSITIVELY identified as an APPROVED REVIEWER's verdict for THIS base, meaning a `.creator.login` that is a member of the workflow's `H10_REVIEWERS` allow-list (an existing `failure` takes the weaker ATTRIBUTABLE test and needs no membership, because inheriting a rejection can only withhold an exemption while re-deriving one can turn it green on an exempt PR) — non-null was too weak, since any account's credential satisfies it and `RENOVATE_TOKEN` is one that cannot be scoped down (#742) — AND a `Review-verdict:` description AND, when that description records a base (`(base: …)`, `release.verdict-status-check`), a base matching the PR's — tested by requiring the description to END with the exact literal `(base: )` and to contain exactly ONE such marker, never by extracting a value (see below); a present-but-different base is rejected, an absent one is not, since verdicts predating that convention carry none; every other shape, including any unrecognised one, is re-derived rather than trusted. The bot and docs-only exemptions are evaluated as INDEPENDENT predicates and the decision made afterwards, never as an `elif` chain. `edited` is in the workflow's `types:` so a retarget reclassifies — which gives DETECTION, not atomicity: status writes are not serialized, so a stale run can still post over a fresher one. That residual is now FENCED rather than merely tracked (NARROWED, not resolved — a retarget between the fence's final count and the POST still leaves a PERMANENT forged green, #849) — the job refuses to write at all if the PR's timeline retarget COUNT moved while it was classifying (`ci.verdict-write-retarget-fence`, #706) — leaving TWO windows, not one: the sub-round-trip window that no API without compare-and-set can close, AND the post-final-count/pre-POST window, which is not sub-round-trip and yields a PERMANENT green because the successor can consume the `edited` event and exit before the stale run posts last (#849). The PROTECTED path list additionally covers `CLAUDE.md` and `AGENTS.md` (#751) — they are not prose but the documents DEFINING the completion protocol, the merge-consent convention and the H10 rule, so protecting `.claude/` while the file specifying what it enforces stayed docs-only-exempt was the same self-exemption one directory over; driving the real classify body with a lone `CLAUDE.md` change produced an exemption `success`. `README.md` is deliberately not listed. It also covers `.codex/` (#711), which mirrors `.claude/hooks/` byte for byte including the merge-consent hook — latent while that directory is untracked, live the moment it is tracked; the list stays ENUMERATIVE rather than derived, because a derived rule would have to be evaluated against the very file list being classified. Reading the CURRENT status for input (3) must tolerate `statuses: null`: `GET /commits/{sha}/status` serialises a nil slice as `null`, not `[]`, on a head with no statuses yet, and an `array`-only gate made `read_existing_verdict` `exit 1` and post nothing at all (#751, `ci.workflow-run-body-no-expressions`) — `null` is accepted only when `total_count` is 0, so a body that merely lost its array is still refused. Path predicates are evaluated by COUNTING with `grep -c`, never `\| grep -q` (SIGPIPE inversion) and never a here-string (temp-space failure) — see `ci.grep-q-pipefail-inversion`. | 2026-07-29 | [link](records/ci/exemption-provenance.md) | +| `ci.exemption-provenance` | The three inputs the exemption decision rests on must each be bound to something the judged PR cannot mutate. (1) BASE — `scripts/pr-changed-files.sh` takes the expected base BRANCH as a REQUIRED 5th argument and re-reads it before and after paging, because `/pulls/{n}/files` diffs against the PR's live base and retargeting moves the answer without moving the head sha; the workflow passes `github.event.pull_request.base.ref` from the `pull_request_target` payload, which a retarget cannot rewrite. (2) BOT EXEMPTION — an author match is necessary but never sufficient: `pull_request.user.login` is the PR's immutable CREATOR while its head is not, so the exemption additionally requires EVERY changed path to be a dependency manifest (`Directory.Packages.props` or `.config/dotnet-tools.json`, and ONLY those — the npm manifests are excluded because `package.json` `scripts` are executed by CI). (3) INHERITED SUCCESS — the never-overwrite short-circuit fires for a `success` only when it is POSITIVELY identified as an APPROVED REVIEWER's verdict for THIS base, meaning a `.creator.login` that is a member of the workflow's `H10_REVIEWERS` allow-list (an existing `failure` takes the weaker ATTRIBUTABLE test and needs no membership, because inheriting a rejection can only withhold an exemption while re-deriving one can turn it green on an exempt PR) — non-null was too weak, since any account's credential satisfies it and `RENOVATE_TOKEN` is one that cannot be scoped down (#742) — AND a `Review-verdict:` description AND, when that description records a base (`(base: …)`, `release.verdict-status-check`), a base matching the PR's — tested by requiring the description to END with the exact literal `(base: )` and to contain exactly ONE such marker, never by extracting a value (see below); a present-but-different base is rejected, an absent one is not, since verdicts predating that convention carry none; every other shape, including any unrecognised one, is re-derived rather than trusted. The bot and docs-only exemptions are evaluated as INDEPENDENT predicates and the decision made afterwards, never as an `elif` chain. `edited` is in the workflow's `types:` so a retarget reclassifies — which gives DETECTION, not atomicity: status writes are not serialized, so a stale run can still post over a fresher one. That residual is now FENCED rather than merely tracked (NARROWED, not resolved — a retarget between the fence's final count and the POST still leaves a PERMANENT forged green, #849) — the job refuses to write at all if the PR's timeline retarget COUNT moved while it was classifying (`ci.verdict-write-retarget-fence`, #706) — leaving TWO windows, not one: the sub-round-trip window that no API without compare-and-set can close, AND the post-final-count/pre-POST window, which is not sub-round-trip and yields a PERMANENT green because the successor can consume the `edited` event and exit before the stale run posts last (#849). The PROTECTED path list additionally covers `CLAUDE.md` and `AGENTS.md` (#751) — they are not prose but the documents DEFINING the completion protocol, the merge-consent convention and the H10 rule, so protecting `.claude/` while the file specifying what it enforces stayed docs-only-exempt was the same self-exemption one directory over; driving the real classify body with a lone `CLAUDE.md` change produced an exemption `success`. `README.md` is deliberately not listed. It also covers `.codex/` (#711), which mirrors `.claude/hooks/` byte for byte including the merge-consent hook — latent while that directory is untracked, live the moment it is tracked; the list stays ENUMERATIVE rather than derived, because a derived rule would have to be evaluated against the very file list being classified. Reading the CURRENT status for input (3) must tolerate `statuses: null`: `GET /commits/{sha}/status` serialises a nil slice as `null`, not `[]`, on a head with no statuses yet, and an `array`-only gate made `read_existing_verdict` `exit 1` and post nothing at all (#751, `ci.workflow-run-body-no-expressions`) — `null` is accepted only when `total_count` is 0, so a body that merely lost its array is still refused. Path predicates are evaluated by COUNTING with `grep -c`, never `\| grep -q` (SIGPIPE inversion) and never a here-string (temp-space failure) — see `ci.grep-q-pipefail-inversion`. The WRITER is bound to the same allow-list rather than trusted to match it (#845): `scripts/post-review-verdict.sh` reads its own status back, identifies that write by state and description, and refuses before writing the comment unless `.creator.login` is a member — so a writer credential that is not a reviewer fails at the terminal that made the mistake instead of stalling the PR one event later. The list is DERIVED from the literal in this workflow by `scripts/lib/h10-reviewers.sh`, never restated: one declaration, not two plus a parity test. | 2026-07-29 | [link](records/ci/exemption-provenance.md) | | `ci.fetch-depth-never-grafts-a-complete-clone` | Any `git fetch` in CI tooling passes `--depth` only when `git rev-parse --is-shallow-repository` already answers `true`, and passes none on any other answer including an unreadable one. `git fetch --depth=N` GRAFTS a complete clone shallow: it writes `.git/shallow` and cuts history at N even though every object is already present, so a depth chosen for one consumer silently breaks every other consumer that checks out `fetch-depth: 0`. A shared script whose consumers disagree on checkout depth must ask the repository rather than assume. And whatever the fetch feeds — a version, a diff, a base revision — must be ASSERTED rather than defaulted: a fallback that cannot fail converts the graft into a plausible wrong answer that never reddens. | 2026-08-29 | [link](records/ci/fetch-depth-never-grafts-a-complete-clone.md) | | `ci.format-gate-folder-mode` | The blocking `format` CI job (and matching pre-commit hook) runs `dotnet format whitespace . --folder --include ` instead of loading the full MSBuild/Roslyn solution, cutting the gate from ~480s to ~0.5s with unchanged whitespace/charset coverage. | 2026-07-19 | [link](records/ci/format-gate-folder-mode.md) | | `ci.functional-e2e-harness` | The `functional-e2e` CI job boots the PR's own code from source via `dotnet run` (`scripts/e2e-local.sh`) and runs deterministic assertions (`scripts/e2e-functional.sh`) as an advisory (non-blocking) job, not a `build` dependency or required check. Originally curl-only; since #445 the same job carries a second, headless-browser step for the contracts curl cannot express — see `ci.ui-e2e-harness`. | 2026-07-16 | [link](records/ci/functional-e2e-harness.md) | diff --git a/docs/decisions/records/ci/exemption-provenance.md b/docs/decisions/records/ci/exemption-provenance.md index c63afbc18..516f65bc1 100644 --- a/docs/decisions/records/ci/exemption-provenance.md +++ b/docs/decisions/records/ci/exemption-provenance.md @@ -5,9 +5,9 @@ status: active since: '2026-07-29' supersedes: none superseded-by: none -rule: 'The three inputs the exemption decision rests on must each be bound to something the judged PR cannot mutate. (1) BASE — `scripts/pr-changed-files.sh` takes the expected base BRANCH as a REQUIRED 5th argument and re-reads it before and after paging, because `/pulls/{n}/files` diffs against the PR''s live base and retargeting moves the answer without moving the head sha; the workflow passes `github.event.pull_request.base.ref` from the `pull_request_target` payload, which a retarget cannot rewrite. (2) BOT EXEMPTION — an author match is necessary but never sufficient: `pull_request.user.login` is the PR''s immutable CREATOR while its head is not, so the exemption additionally requires EVERY changed path to be a dependency manifest (`Directory.Packages.props` or `.config/dotnet-tools.json`, and ONLY those — the npm manifests are excluded because `package.json` `scripts` are executed by CI). (3) INHERITED SUCCESS — the never-overwrite short-circuit fires for a `success` only when it is POSITIVELY identified as an APPROVED REVIEWER''s verdict for THIS base, meaning a `.creator.login` that is a member of the workflow''s `H10_REVIEWERS` allow-list (an existing `failure` takes the weaker ATTRIBUTABLE test and needs no membership, because inheriting a rejection can only withhold an exemption while re-deriving one can turn it green on an exempt PR) — non-null was too weak, since any account''s credential satisfies it and `RENOVATE_TOKEN` is one that cannot be scoped down (#742) — AND a `Review-verdict:` description AND, when that description records a base (`(base: …)`, `release.verdict-status-check`), a base matching the PR''s — tested by requiring the description to END with the exact literal `(base: )` and to contain exactly ONE such marker, never by extracting a value (see below); a present-but-different base is rejected, an absent one is not, since verdicts predating that convention carry none; every other shape, including any unrecognised one, is re-derived rather than trusted. The bot and docs-only exemptions are evaluated as INDEPENDENT predicates and the decision made afterwards, never as an `elif` chain. `edited` is in the workflow''s `types:` so a retarget reclassifies — which gives DETECTION, not atomicity: status writes are not serialized, so a stale run can still post over a fresher one. That residual is now FENCED rather than merely tracked (NARROWED, not resolved — a retarget between the fence''s final count and the POST still leaves a PERMANENT forged green, #849) — the job refuses to write at all if the PR''s timeline retarget COUNT moved while it was classifying (`ci.verdict-write-retarget-fence`, #706) — leaving TWO windows, not one: the sub-round-trip window that no API without compare-and-set can close, AND the post-final-count/pre-POST window, which is not sub-round-trip and yields a PERMANENT green because the successor can consume the `edited` event and exit before the stale run posts last (#849). The PROTECTED path list additionally covers `CLAUDE.md` and `AGENTS.md` (#751) — they are not prose but the documents DEFINING the completion protocol, the merge-consent convention and the H10 rule, so protecting `.claude/` while the file specifying what it enforces stayed docs-only-exempt was the same self-exemption one directory over; driving the real classify body with a lone `CLAUDE.md` change produced an exemption `success`. `README.md` is deliberately not listed. It also covers `.codex/` (#711), which mirrors `.claude/hooks/` byte for byte including the merge-consent hook — latent while that directory is untracked, live the moment it is tracked; the list stays ENUMERATIVE rather than derived, because a derived rule would have to be evaluated against the very file list being classified. Reading the CURRENT status for input (3) must tolerate `statuses: null`: `GET /commits/{sha}/status` serialises a nil slice as `null`, not `[]`, on a head with no statuses yet, and an `array`-only gate made `read_existing_verdict` `exit 1` and post nothing at all (#751, `ci.workflow-run-body-no-expressions`) — `null` is accepted only when `total_count` is 0, so a body that merely lost its array is still refused. Path predicates are evaluated by COUNTING with `grep -c`, never `| grep -q` (SIGPIPE inversion) and never a here-string (temp-space failure) — see `ci.grep-q-pipefail-inversion`.' -signals: 'verdict inherited from a bot token, RENOVATE_TOKEN posts a review verdict, who may post an h10 verdict, H10_REVIEWERS allow-list, forged review-verdict exemption, retarget race against the docs-only classifier, PR base changed mid-run, hijacked Renovate branch, bot exemption on a code change, machine-written success inherited as a verdict, status creator null vs user, never overwrite a human verdict, exemption chain skips docs-only for bots, why is my Renovate PR asking for a verdict, base ref binding on pr-changed-files.sh · paths: `.gitea/workflows/review-verdict.yml`, `scripts/pr-changed-files.sh`, `.claude/hooks/pretooluse-merge-consent.sh`, `scripts/tests/test_pr_changed_files.py` · issues: #849, #845, #742, #698, #697, #672, #663, #649, #632' -mechanics: '`scripts/pr-changed-files.sh ` (5 args; a 4-arg call exits 2); workflow env `BASE_REF: ${{ github.event.pull_request.base.ref }}`; `BOT_MANIFESTS` anchored allow-list; `H10_REVIEWERS="timothy"` declared as a literal in the classify body (a repo `vars:` entry or secret is settable without a PR); membership by exact POSIX `[ = ]` comparison over word-split entries, never an UNQUOTED right-hand side — `[[ $rv == $ex_creator ]]` makes it a pattern, while a quoted RHS under `[[ ]]` is literal (probed, bash 5.3); POSIX `[ = ]` cannot glob at all, which is why it is used and never `grep -xF` (an API-supplied creator containing a newline reads as several patterns); `read_existing_verdict` sets `ex_attributable` (creator non-null AND `^Review-verdict:` AND base OK) and derives `ex_human` from it by membership; the INHERITANCE short-circuit reads `ex_human`, the LAST-MOMENT RE-READ reads `ex_attributable` plus a change against the state/creator/description triple snapshotted at the first read, and the post-write raced `jq` stays broad on `creator != null`; `types: [opened, reopened, synchronize, ready_for_review, edited]`' +rule: 'The three inputs the exemption decision rests on must each be bound to something the judged PR cannot mutate. (1) BASE — `scripts/pr-changed-files.sh` takes the expected base BRANCH as a REQUIRED 5th argument and re-reads it before and after paging, because `/pulls/{n}/files` diffs against the PR''s live base and retargeting moves the answer without moving the head sha; the workflow passes `github.event.pull_request.base.ref` from the `pull_request_target` payload, which a retarget cannot rewrite. (2) BOT EXEMPTION — an author match is necessary but never sufficient: `pull_request.user.login` is the PR''s immutable CREATOR while its head is not, so the exemption additionally requires EVERY changed path to be a dependency manifest (`Directory.Packages.props` or `.config/dotnet-tools.json`, and ONLY those — the npm manifests are excluded because `package.json` `scripts` are executed by CI). (3) INHERITED SUCCESS — the never-overwrite short-circuit fires for a `success` only when it is POSITIVELY identified as an APPROVED REVIEWER''s verdict for THIS base, meaning a `.creator.login` that is a member of the workflow''s `H10_REVIEWERS` allow-list (an existing `failure` takes the weaker ATTRIBUTABLE test and needs no membership, because inheriting a rejection can only withhold an exemption while re-deriving one can turn it green on an exempt PR) — non-null was too weak, since any account''s credential satisfies it and `RENOVATE_TOKEN` is one that cannot be scoped down (#742) — AND a `Review-verdict:` description AND, when that description records a base (`(base: …)`, `release.verdict-status-check`), a base matching the PR''s — tested by requiring the description to END with the exact literal `(base: )` and to contain exactly ONE such marker, never by extracting a value (see below); a present-but-different base is rejected, an absent one is not, since verdicts predating that convention carry none; every other shape, including any unrecognised one, is re-derived rather than trusted. The bot and docs-only exemptions are evaluated as INDEPENDENT predicates and the decision made afterwards, never as an `elif` chain. `edited` is in the workflow''s `types:` so a retarget reclassifies — which gives DETECTION, not atomicity: status writes are not serialized, so a stale run can still post over a fresher one. That residual is now FENCED rather than merely tracked (NARROWED, not resolved — a retarget between the fence''s final count and the POST still leaves a PERMANENT forged green, #849) — the job refuses to write at all if the PR''s timeline retarget COUNT moved while it was classifying (`ci.verdict-write-retarget-fence`, #706) — leaving TWO windows, not one: the sub-round-trip window that no API without compare-and-set can close, AND the post-final-count/pre-POST window, which is not sub-round-trip and yields a PERMANENT green because the successor can consume the `edited` event and exit before the stale run posts last (#849). The PROTECTED path list additionally covers `CLAUDE.md` and `AGENTS.md` (#751) — they are not prose but the documents DEFINING the completion protocol, the merge-consent convention and the H10 rule, so protecting `.claude/` while the file specifying what it enforces stayed docs-only-exempt was the same self-exemption one directory over; driving the real classify body with a lone `CLAUDE.md` change produced an exemption `success`. `README.md` is deliberately not listed. It also covers `.codex/` (#711), which mirrors `.claude/hooks/` byte for byte including the merge-consent hook — latent while that directory is untracked, live the moment it is tracked; the list stays ENUMERATIVE rather than derived, because a derived rule would have to be evaluated against the very file list being classified. Reading the CURRENT status for input (3) must tolerate `statuses: null`: `GET /commits/{sha}/status` serialises a nil slice as `null`, not `[]`, on a head with no statuses yet, and an `array`-only gate made `read_existing_verdict` `exit 1` and post nothing at all (#751, `ci.workflow-run-body-no-expressions`) — `null` is accepted only when `total_count` is 0, so a body that merely lost its array is still refused. Path predicates are evaluated by COUNTING with `grep -c`, never `| grep -q` (SIGPIPE inversion) and never a here-string (temp-space failure) — see `ci.grep-q-pipefail-inversion`. The WRITER is bound to the same allow-list rather than trusted to match it (#845): `scripts/post-review-verdict.sh` reads its own status back, identifies that write by state and description, and refuses before writing the comment unless `.creator.login` is a member — so a writer credential that is not a reviewer fails at the terminal that made the mistake instead of stalling the PR one event later. The list is DERIVED from the literal in this workflow by `scripts/lib/h10-reviewers.sh`, never restated: one declaration, not two plus a parity test.' +signals: 'verdict inherited from a bot token, RENOVATE_TOKEN posts a review verdict, who may post an h10 verdict, H10_REVIEWERS allow-list, forged review-verdict exemption, retarget race against the docs-only classifier, PR base changed mid-run, hijacked Renovate branch, bot exemption on a code change, machine-written success inherited as a verdict, status creator null vs user, never overwrite a human verdict, exemption chain skips docs-only for bots, why is my Renovate PR asking for a verdict, base ref binding on pr-changed-files.sh · paths: `.gitea/workflows/review-verdict.yml`, `scripts/pr-changed-files.sh`, `.claude/hooks/pretooluse-merge-consent.sh`, `scripts/tests/test_pr_changed_files.py`, `scripts/post-review-verdict.sh`, `scripts/lib/h10-reviewers.sh` · issues: #849, #845, #742, #698, #697, #672, #663, #649, #632' +mechanics: '`scripts/pr-changed-files.sh ` (5 args; a 4-arg call exits 2); workflow env `BASE_REF: ${{ github.event.pull_request.base.ref }}`; `BOT_MANIFESTS` anchored allow-list; `H10_REVIEWERS="timothy"` declared as a literal in the classify body (a repo `vars:` entry or secret is settable without a PR); `scripts/lib/h10-reviewers.sh` derives that same literal for the write side by an anchored single-assignment parse and refuses on 0 or >1 matches, on a non-plain-login entry, or on an empty list; membership by exact POSIX `[ = ]` comparison over word-split entries, never an UNQUOTED right-hand side — `[[ $rv == $ex_creator ]]` makes it a pattern, while a quoted RHS under `[[ ]]` is literal (probed, bash 5.3); POSIX `[ = ]` cannot glob at all, which is why it is used and never `grep -xF` (an API-supplied creator containing a newline reads as several patterns); `read_existing_verdict` sets `ex_attributable` (creator non-null AND `^Review-verdict:` AND base OK) and derives `ex_human` from it by membership; the INHERITANCE short-circuit reads `ex_human`, the LAST-MOMENT RE-READ reads `ex_attributable` plus a change against the state/creator/description triple snapshotted at the first read, and the post-write raced `jq` stays broad on `creator != null`; `types: [opened, reopened, synchronize, ready_for_review, edited]`' --- `ci.gate-trigger-base-resolved` stopped a PR supplying the gate's own *definition*. This closes the @@ -219,22 +219,41 @@ verified when a mark exists, a sticky sentinel when it does not, and a CHAINED t real output into run N+1. Two lines inside a provenance change was the wrong size for it — the attempt is recorded because the next reader will otherwise re-attempt it the same way. -**A coupling this record does NOT assert, tracked as #845 instead.** `post-review-verdict.sh` posts with -whatever account owns `ETV_GITEA_TOKEN`/`ETV_GITEA_BASICAUTH` and never checks whose it is. If that -ever stops being an allow-listed account, every verdict silently stops being inheritable and each -later event re-derives it — to `pending` on a PR that needs a verdict, or to an exemption `success` on -a docs-only or Renovate-manifest one, which is the quieter and worse half. Either way the verdict is -gone, and the only diagnostic is a line in a job log. -Correct today — verified live 2026-08-26, reproduce with `GET /api/v1/user` (login `timothy`) and -`GET /api/v1/admin/users?limit=50` (exactly `renovate`, `timothy`) — but nothing keeps it correct -(#845). The job DOES name the creator and the allow-list whenever it declines to inherit — but in the -WORKFLOW RUN, while the mistake is made at the terminal running `post-review-verdict.sh`, which -reports success either way. That gap is what #845 is about, and it is why a louder annotation was not -the answer: an earlier draft added a `::warning::` alongside that line, and it was withdrawn after -producing three defects in three rounds (a message that contradicted itself for an allow-listed -creator, an unescaped API value interpolated into a workflow command, and a test asserting its absence -on a path that exits two guards earlier and so could never fail). The signal has to reach the WRITER, -not be repeated more loudly to the reader. +**The writer/gate coupling, ASSERTED at the writer since #845.** `post-review-verdict.sh` posts with +whatever account owns `ETV_GITEA_TOKEN`/`ETV_GITEA_BASICAUTH`. It used to never ask whose that was, and +the two values are coupled: the day the writer credential stops being an allow-listed account, every +verdict silently stops being inheritable and each later event re-derives it — to `pending` on a PR that +needs a verdict, or to an exemption `success` on a docs-only or Renovate-manifest one, which is the +quieter and worse half. Either way the verdict is gone. It now READS THE STATUS BACK after posting it, +identifies its own write by state and description, and refuses — before the comment, so the surviving +half-state is the `ask` one — unless `.creator.login` is on the allow-list. A `creator: null` +(Actions-token) status is refused with its own message, because there is no account to name and "add +them to the allow-list" would be the wrong advice. + +The allow-list is not restated on the writer side. `scripts/lib/h10-reviewers.sh` DERIVES it from the +`H10_REVIEWERS` literal in this workflow, so there is one declaration rather than two held together by +a parity test — the shape `testing.guard-derives-population-from-source` records six failed rounds +against. It is a parse rather than a shared declaration both sides source, because the gate runs +against a checkout of the PR BASE sha: a PR whose base predates such a file would not have it, and a +missing `source` under `set -euo pipefail` kills the job, which posts no `review-verdict/h10` at all +and blocks every merge including the repair (`block_admin_merge_override: true`). A fallback list for +that case would reintroduce the second copy. So the derivation runs on the side that can fail safely +and the body of this workflow is untouched. The parse admits exactly one anchored assignment or +refuses; `test_pr_changed_files.py::test_the_H10_REVIEWERS_list_is_glob_free_and_non_empty` reads the +same literal with the same anchors, so a shape change reddens CI on the PR that makes it. + +The allow-list was `timothy` and the instance held exactly `renovate` and `timothy` when this was +measured on 2026-08-26 (`GET /api/v1/user`, `GET /api/v1/admin/users?limit=50`). That measurement is +no longer what keeps the coupling correct, which is the point of the change. + +**Why the signal had to reach the WRITER.** The job already names the creator and the allow-list +whenever it declines to inherit — but in the WORKFLOW RUN, while the mistake is made at the terminal +running `post-review-verdict.sh`, which reported success either way. An earlier draft tried to fix +that by shouting louder at the reader: a `::warning::` alongside that line, withdrawn after producing +three defects in three rounds (a message that contradicted itself for an allow-listed creator, an +unescaped API value interpolated into a workflow command, and a test asserting its absence on a path +that exits two guards earlier and so could never fail). Recorded so it is not re-attempted: more +volume in the run was never the missing piece. **What #742 does NOT close, restated because the allow-list invites the wrong reading.** An allow-listed collaborator's own write-scoped token can still POST a verdict nobody formed. Branch diff --git a/docs/guard-inventory.md b/docs/guard-inventory.md index f626d90aa..85abde7c7 100644 --- a/docs/guard-inventory.md +++ b/docs/guard-inventory.md @@ -227,7 +227,7 @@ recorded as unexamined rather than as cleared. | `scripts/e2e-ui.sh` | nothing (drives the Playwright flows) | TOOLING | NONE | — | | `scripts/hook-fire-log.sh` | nothing (records that each hook fired, and reports it) | TOOLING | NONE | — | | `scripts/jq-preflight.sh` | the `script-tests` job, on a jq version change | GUARD | BEHAVIOUR-ONLY | `test_jq_preflight.py::test_below_the_floor_is_LOUD` | -| `scripts/post-review-verdict.sh` | nothing (writes the verdict status) | GUARD | BEHAVIOUR-ONLY | `test_post_review_verdict.py::test_never_retargets_the_verdict_at_the_new_head` | +| `scripts/post-review-verdict.sh` | a verdict the gate will not inherit being reported as posted | GUARD | MUTATION | `test_post_review_verdict.py::test_a_verdict_posted_by_an_ALLOWLISTED_account_is_accepted` | | `scripts/pr-changed-files.sh` | the verdict exemption, on an incomplete enumeration | GUARD | BEHAVIOUR-ONLY | `test_pr_changed_files.py::test_a_SHORT_page_does_not_end_the_enumeration` | | `scripts/prove-fix.sh` | the `prove-fix` job, on a commit whose `Proves:` trailer names a test that passes without the fix | GUARD | MUTATION | `test_prove_fix.py::test_MUTATION_disarming_the_UNPROVEN_clause_reddens_the_refusal_test` | | `scripts/update-openapi.sh` | nothing (regenerates the spec) | TOOLING | NONE | — | @@ -375,7 +375,7 @@ clause-provable and the entry is regraded. ## What the numbers say -45 guards, 6 tooling scripts, 23 proof files. **24 guards carry a mutation proof; 6 are +45 guards, 6 tooling scripts, 23 proof files. **25 guards carry a mutation proof; 5 are behaviour-only; 15 have none.** These figures are asserted against the table by `test_the_summary_counts_match_the_table`, because a hand-maintained summary of a table is a second copy of it — the duplication family this change argues against, and one that has already drifted diff --git a/docs/remote-state-inventory.md b/docs/remote-state-inventory.md index 00cd13728..137a843e7 100644 --- a/docs/remote-state-inventory.md +++ b/docs/remote-state-inventory.md @@ -96,7 +96,7 @@ classifications differ; otherwise the strictest applies and the Note names the e | Site | Class | Note | |---|---|---| -| `scripts/post-review-verdict.sh` — commit-status write | `PINNED` | Re-reads the PR and compares **both** `.head.sha` and `.base.ref` immediately before the POST, and `die`s (exit 1, no status written) on a mismatch **or on a field it cannot read**. That last clause is load-bearing: guarding both comparisons with `[ -n "$x" ] &&` alone makes a well-formed 2xx body that merely omits the field a no-op, and the status is posted having confirmed nothing. Regression-tested against the real predecessor (#778), since the redundant-looking `-z` arm alone mutates green. Closes #706 and #632 for this path by read-compare-refuse, not by CAS: Gitea's status API offers no conditional write. Residual closed on the write ORDER since #792: the status is written first and the comment second, so a refusal can no longer leave a verdict comment with no status behind it — the only reachable half-state is a status with no comment, which the merge hook resolves as `ask` (`release.verdict-writes-status-before-comment`). | +| `scripts/post-review-verdict.sh` — commit-status write | `PINNED` | Re-reads the PR and compares **both** `.head.sha` and `.base.ref` immediately before the POST, and `die`s (exit 1, no status written) on a mismatch **or on a field it cannot read**. That last clause is load-bearing: guarding both comparisons with `[ -n "$x" ] &&` alone makes a well-formed 2xx body that merely omits the field a no-op, and the status is posted having confirmed nothing. Regression-tested against the real predecessor (#778), since the redundant-looking `-z` arm alone mutates green. Closes #706 and #632 for this path by read-compare-refuse, not by CAS: Gitea's status API offers no conditional write. Residual closed on the write ORDER since #792: the status is written first and the comment second, so a refusal can no longer leave a verdict comment with no status behind it — the only reachable half-state is a status with no comment, which the merge hook resolves as `ask` (`release.verdict-writes-status-before-comment`). Since #845 the status is also READ BACK after the POST and refused unless it is the write just made AND its `.creator.login` is on the allow-list derived from the gate — the same read-compare-refuse shape, applied to WHOSE verdict it is rather than to which commit it covers. That read is check-then-act too and is graded with this row rather than separately: it can only withhold this tool's success report, never authorize anything, and the residual it leaves — a non-inheritable status standing with no comment beside it — is the `ask` half-state above. | | `scripts/pr-changed-files.sh` — paged file enumeration | `UNSAFE-KNOWN` | #707's fix: `.base.ref`, `.base.sha` and `.head.sha` are captured before paging and re-checked after, and any *observed* movement fails the whole enumeration closed rather than emitting a short list. But before-and-after equality is **ABA-vulnerable** — a `main → scratch → main` retarget during paging can return the same ref and, if nothing merged meanwhile, the same base sha, while the pages in between were diffed against the scratch base. The script's own comment says it narrows rather than erases — "any movement fails" would be stronger than the code. Accepted here because the enumerator cannot close it alone, but be exact about what the caller-side fence does and does not cover: `ci.verdict-write-retarget-fence` counts `change_target_branch` events, so it catches the BASE alias and **nothing else**. A HEAD alias — a force-push `H1 -> H2 -> H1` during pagination — leaves the final `.head.sha` comparison equal while the middle pages were enumerated against `H2`, and NOTHING inside this script can see it. **Fenced at the enforced caller since #803** (2026-08-28): the same timeline walk now also counts `pull_push`, so a head that moved at all during classification withholds the write — subject to that walk's own terminator, which is defeatable by a page of filtered rows (**#870**, pre-existing and equally affecting the base axis). The grade stays `UNSAFE-KNOWN` for the SCRIPT, because the script's own guarantee is unchanged and one-way — a caller that does not run the fence inherits the alias. Enumeration bound to an immutable tree remains unavailable: re-probed at 1.27.1 on 2026-08-28, `compare/{base}...{head}` still returns no `files` key. | | `scripts/select-queue.sh` — issue list, then per-issue `/dependencies` | `UNSAFE-KNOWN` | The open-issue list (labels, milestone, priority) is snapshotted once; per-candidate dependency reads happen seconds later and never re-read the issue's own labels, so an issue claimed `in-progress` in that gap still appears on the shortlist. Accepted: the script authorizes **no write**. The real gate is the four-way claim check in `process.parallel-session-claim`, which runs after selection and re-reads live state by construction. Tightening this would move a check that must be adversarial into a tool that is advisory. | | `scripts/ci-detect-already-validated.sh` — prior-head combined status | `UNSAFE-KNOWN` | Reads the PR head's status and emits `skip=true`, with nothing re-checking before the consuming job runs. Accepted and narrow: the skip elides only **re-running** test/migrations on a tree already validated; the `build` job still builds and pushes unconditionally, so no image ever ships from unvalidated source. | @@ -111,6 +111,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/lib/h10-reviewers.sh` | `N/A` | Reads no live remote state — it derives the gate's `H10_REVIEWERS` allow-list by parsing one anchored assignment out of the LOCAL `.gitea/workflows/review-verdict.yml`, and makes no call of its own. The read-then-act it feeds belongs to its caller's row above: `post-review-verdict.sh` compares the derived list against a creator it reads back from the server. Deliberately no env-var override of the file it reads — an env var choosing which tree answers a security question is an unsound input (#858). | | `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/check-required-contexts.sh` | `N/A` | Reads no live remote state itself — classifies a `branch_protections` payload supplied on stdin; the fetch belongs to the caller's row (`.claude/hooks/pretooluse-merge-consent.sh` — guard-scope freshness). | | `scripts/lib/branch-rule-classifier.jq` | `N/A` | Reads nothing at all — a jq program over a `branch_protections` payload its caller supplies, resolving which protection rule governs a branch. Loaded by both `pretooluse-merge-consent.sh` and `check-required-contexts.sh`; every fetch belongs to those callers' rows. | diff --git a/scripts/lib/h10-reviewers.sh b/scripts/lib/h10-reviewers.sh new file mode 100644 index 000000000..9b7a9dec5 --- /dev/null +++ b/scripts/lib/h10-reviewers.sh @@ -0,0 +1,189 @@ +# shellcheck shell=bash +# h10-reviewers.sh — WHOSE verdicts the H10 gate will inherit, DERIVED from the gate itself +# (ersatztv#845). +# +# SOURCED, NEVER EXECUTED. It defines pure functions over one file and takes no action of its own, +# so it is deliberately not executable and carries no shebang. +# +# WHY THIS FILE EXISTS. `.gitea/workflows/review-verdict.yml` inherits an existing +# `review-verdict/h10=success` only when the status's `.creator.login` is a member of its +# `H10_REVIEWERS` allow-list (ersatztv#742). `scripts/post-review-verdict.sh` is the tool that WRITES +# those verdicts, with whatever account owns `ETV_GITEA_TOKEN` / `ETV_GITEA_BASICAUTH`. The two +# values are coupled and nothing asserted the coupling: the day the writer credential stops being an +# allow-listed account — a service token, a second machine, a renamed account, a bot minted for +# automation — every verdict is still WRITTEN, the tool still reports success, and the gate silently +# re-derives it on the next PR event. A repo-wide merge stall whose only diagnostic was a +# `::warning::` in a workflow run nobody reads. +# +# THERE IS NO SECOND COPY OF THE LIST HERE, on purpose. The workflow literal is the ONE declaration +# and this file DERIVES from it, the way `review-verdict-vocabulary.sh` made one word list serve both +# the read and the write side (ersatztv#788). A hand-copied allow-list held together by a parity test +# is the shape that record spent six rounds failing to converge on. +# +# WHY A PARSE RATHER THAN A SHARED DECLARATION BOTH SIDES SOURCE. That would be the better shape and +# it is not available. The gate runs under `pull_request_target` against a checkout of the PR BASE +# sha, so a PR whose base predates the introduction of a new `scripts/lib/` file would not have it — +# the same condition the `jq preflight` step already handles by skipping. Under `set -euo pipefail` a +# missing source is a dead job, and a dead job posts no `review-verdict/h10` at all, which blocks +# EVERY merge to `main` including the PR that would repair it (`block_admin_merge_override: true`, +# ersatztv#743). Writing a fallback list for that case reintroduces the second copy. So the +# derivation runs on the side that can fail safely — this one — and the gate body is untouched. +# +# WHAT THE PARSE IS ALLOWED TO BE. Exactly one anchored assignment line, or a refusal. That is not a +# shell parser and does not try to be: `testing.guard-derives-population-from-source` records why a +# regex over shell source does not converge when the target is an open-ended construct. The target +# here is closed — `test_pr_changed_files.py::test_the_H10_REVIEWERS_list_is_glob_free_and_non_empty` +# reads the same literal with the same anchors and already fails the suite if the workflow stops +# declaring it as a single double-quoted assignment, so a shape change reddens CI on the PR that +# makes it rather than going stale here. +# +# SNAPSHOT BOUNDARY, stated because it is a real limit and not a defect: this reads the workflow in +# the LOCAL checkout, while the gate that actually runs is the one at `origin/main` (base-resolved). +# On a branch that edits `H10_REVIEWERS` the two differ until it merges. That is the correct +# approximation — the alternative is a live remote read on the merge-consent write path — and CI +# holds them consistent, because `.gitea/` cannot ride a docs-only exemption. +# +# NO ENVIRONMENT VARIABLE SELECTS THE SOURCE FILE. The path is derived from this file own location, +# and a caller may pass one explicitly; there is deliberately no `ETV_*_WORKFLOW` override, because +# an env var that chooses which tree answers a security question is an unsound input (ersatztv#858 +# is the same shape one file over). + +# --- The default source: the shipped gate definition. -------------------------------------------- +# Resolved relative to THIS FILE, never to the caller cwd — `post-review-verdict.sh` is run from +# whatever directory an operator happens to be in. +ETV_H10_REVIEWERS_WORKFLOW='' +if _etv_h10_root=$(CDPATH='' cd -- "$(dirname -- "${BASH_SOURCE[0]}")/../.." 2>/dev/null && pwd); then + ETV_H10_REVIEWERS_WORKFLOW="$_etv_h10_root/.gitea/workflows/review-verdict.yml" +fi +unset _etv_h10_root + +# --- The validation sentinel. -------------------------------------------------------------------- +# Reset to 0 HERE, at load time, for the two reasons `review-verdict-vocabulary.sh` sets out at +# length: it defeats an inherited `ETV_H10_REVIEWERS_OK=1` from the environment, and it makes +# "derived successfully" a DATA dependency the query function can require rather than a control-flow +# gate a caller is trusted to have taken. 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. A data dependency cannot be skipped that way. +ETV_H10_REVIEWERS_OK=0 +ETV_H10_REVIEWERS='' + +# --- The split idiom, stated once. --------------------------------------------------------------- +# Both functions below turn the space-separated list into the positional parameters with PATHNAME +# EXPANSION OFF, then restore `-f` to whatever the caller had. It is written out at each site rather +# than factored into a helper because a helper cannot set its caller positional parameters, and +# returning the words through a string would reintroduce a split — of a value that may contain a +# newline — at the other end. +# +# Getting this wrong is the documented fail-open, not a style point: `for x in $list` and +# `$(printf %s "$list")` both glob, so a `*` entry expands to the FILENAMES around it, and any of +# those matching the plain-login class below would validate cleanly and enter the allow-list as a +# real reviewer. That is the defect `review-verdict-vocabulary.sh` records against its own word list, +# reached one file over through the allow-list. + +# --- The derivation. ----------------------------------------------------------------------------- +# Sets `ETV_H10_REVIEWERS` and the sentinel. Returns 0 when the allow-list is usable, 1 otherwise +# (message on stderr). It never exits: the consumer decides its own exit semantics. +# +# $1 — the workflow file to read; defaults to the shipped one resolved above. +etv_h10_reviewers_load() { + local src="${1:-${ETV_H10_REVIEWERS_WORKFLOW:-}}" + local count value entry n=0 glob_was_off=0 + + ETV_H10_REVIEWERS_OK=0 + ETV_H10_REVIEWERS='' + + if [ -z "$src" ]; then + printf 'h10 reviewers: could not resolve the gate definition from the location of this library, and no path was passed\n' >&2 + return 1 + fi + if [ ! -r "$src" ]; then + printf 'h10 reviewers: %s is missing or unreadable, so who the gate accepts cannot be derived\n' "$src" >&2 + return 1 + fi + + # COUNT the anchored assignments, never `grep -q`. Two separate reasons, both recorded because each + # has cost this repo a round: `grep -q` closes the pipe early and inverts under `pipefail` on large + # input (`ci.grep-q-pipefail-inversion`), and — the reason that matters here — a boolean cannot + # tell ONE assignment from TWO. The shell runs the LAST assignment executed, so a second or + # conditional one would leave this deriving a value the gate does not use. `grep -c` reads a FILE + # argument, so there is no pipe to invert in the first place. + count=$(grep -cE '^[[:space:]]*H10_REVIEWERS="[^"]*"[[:space:]]*$' "$src") || count=0 + if [ "$count" != 1 ]; then + printf 'h10 reviewers: %s carries %s anchored H10_REVIEWERS assignments, not exactly 1 — refusing to guess which one the gate runs with\n' "$src" "$count" >&2 + return 1 + fi + + value=$(sed -n 's/^[[:space:]]*H10_REVIEWERS="\([^"]*\)"[[:space:]]*$/\1/p' "$src") || value='' + + # Validate every entry against the same character class the workflow own lint enforces. It is not + # cosmetic on either side: the gate membership loop word-splits the list UNQUOTED, so `*`, `?` or + # `[` there would be matched against the runner cwd and could silently change who the gate accepts. + case "$-" in *f*) glob_was_off=1 ;; esac + set -f + # shellcheck disable=SC2086 # deliberate word-split of the space-separated list, globbing off + set -- $value + [ "$glob_was_off" = 1 ] || set +f + for entry in "$@"; do + case "$entry" in + *[!A-Za-z0-9._-]*) + printf 'h10 reviewers: %s is not a plain login; * ? and [ are glob metacharacters in the membership loop of the gate\n' "$entry" >&2 + return 1 + ;; + esac + n=$((n + 1)) + done + + # Emptiness is checked on the SPLIT COUNT, not on the raw string. A value of " " is non-empty as + # a string and splits to nothing, and an emptied list is not a harmless default: the gate loop + # `for rv in ""` iterates zero times, so every verdict from every account would stop being + # inherited and `pending` would be re-posted over real human verdicts forever. + if [ "$n" -eq 0 ]; then + printf 'h10 reviewers: %s declares an EMPTY H10_REVIEWERS — no verdict from any account could be inherited\n' "$src" >&2 + return 1 + fi + + ETV_H10_REVIEWERS="$value" + # 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_H10_REVIEWERS_OK=1 + return 0 +} + +# --- Derived view: is this login one whose verdict the gate would inherit? ----------------------- +# THREE outcomes, deliberately, because two would conflate them: 0 = a member, 1 = NOT a member, +# 2 = CANNOT ANSWER (the derivation never ran, or failed). A caller that treats "cannot answer" as +# "not a member" turns a broken checkout into a refusal, which is survivable; one that treats it as +# "member" turns it into a silent pass, which is the failure this file exists to prevent. Both are +# wrong to guess at, so the third code makes the caller say which it meant. +etv_h10_reviewers_contains() { + local login="${1:-}" entry glob_was_off=0 + + if [ "${ETV_H10_REVIEWERS_OK:-0}" != 1 ]; then + printf 'h10 reviewers: the allow-list was never derived (call etv_h10_reviewers_load first)\n' >&2 + return 2 + fi + + case "$-" in *f*) glob_was_off=1 ;; esac + set -f + # shellcheck disable=SC2086 # deliberate word-split of the space-separated list, globbing off + set -- $ETV_H10_REVIEWERS + [ "$glob_was_off" = 1 ] || set +f + + for entry in "$@"; do + # POSIX `[ = ]`, exactly as the gate compares. Never `[[ $entry == $login ]]` — an unquoted + # right-hand side under `[[ ]]` is a PATTERN, not a literal — and never `grep -xF`, because an + # API-supplied creator containing a newline would read as several patterns. + if [ "$entry" = "$login" ]; then + return 0 + fi + done + return 1 +} + +# --- 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 consumer therefore sources this file in a SUBSHELL +# first and requires 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_h10_reviewers_loaded() { printf 'etv-h10-reviewers-loaded\n'; } diff --git a/scripts/post-review-verdict.sh b/scripts/post-review-verdict.sh index 2a7070ad3..79755d72c 100755 --- a/scripts/post-review-verdict.sh +++ b/scripts/post-review-verdict.sh @@ -106,6 +106,32 @@ case "$verdict_class" in *) die "the shared vocabulary classified '$verdict' as '$verdict_class', which is neither positive nor negative — refusing to guess a commit-status state" ;; esac +# --- WHOSE verdict the gate will inherit (ersatztv#845). ---------------------------------------- +# Loaded HERE: after the arguments validate, so a bare `post-review-verdict.sh` still prints its +# usage, and before the first network call, so a checkout that cannot answer the question refuses +# without having touched the server. +# +# The list is NOT declared here. `.gitea/workflows/review-verdict.yml` owns it and this derives from +# that file; see `scripts/lib/h10-reviewers.sh` for why the derivation is a parse rather than a +# declaration both sides source. Fails CLOSED on every branch, exactly like the vocabulary above: a +# writer that cannot find out whether its own verdicts are inheritable must not post one and report +# success, because that combination is the ersatztv#845 stall itself. +H10_REVIEWERS_LIB="$_here/lib/h10-reviewers.sh" +[ -r "$H10_REVIEWERS_LIB" ] \ + || die "the reviewer allow-list derivation is missing or unreadable at $H10_REVIEWERS_LIB — no status was written" +# Probe-source in a subshell and require the end-of-file marker before trusting the file, for the +# same reason the vocabulary does: a top-level `exit` in a sourced library terminates THIS script at +# the `source` line, having posted neither a status nor a comment, with exit 0. +# shellcheck source=lib/h10-reviewers.sh +h10_probe=$( . "$H10_REVIEWERS_LIB" >/dev/null 2>&1 && etv_h10_reviewers_loaded 2>/dev/null ) || h10_probe='' +[ "$h10_probe" = 'etv-h10-reviewers-loaded' ] \ + || die "the reviewer allow-list derivation at $H10_REVIEWERS_LIB did not load to completion (syntax error, truncated, or it exits at top level) — no status was written" +# shellcheck source=lib/h10-reviewers.sh +. "$H10_REVIEWERS_LIB" \ + || die "the reviewer allow-list derivation at $H10_REVIEWERS_LIB could not be sourced — no status was written" +etv_h10_reviewers_load \ + || die "could not derive the gate's H10_REVIEWERS allow-list (see above) — no status was written. Until it can be read, this tool cannot tell whether the verdict it posts would be honoured or silently re-derived." + base_url="${ETV_GITEA_URL:-http://192.168.1.95:3000}/api/v1" repo_slug="${ETV_GITEA_REPO:-timothy/ersatztv}" owner="${repo_slug%%/*}" @@ -211,14 +237,80 @@ fi # `scripts/check-review-verdict.sh`, whose grammar had three false-opens in its history; nothing # parses the description today, so this adds a field without reopening that surface. The hook reads # it back and compares (ersatztv#632). +# Held in a variable rather than inlined, because the read-back below identifies OUR write by it. +status_desc="Review-verdict: $verdict @ $short (base: $base_ref)" status_payload=$(jq -n \ --arg s "$state" --arg c "$STATUS_CONTEXT" --arg u "$pr_url" \ - --arg d "Review-verdict: $verdict @ $short (base: $base_ref)" \ + --arg d "$status_desc" \ '{state:$s, context:$c, description:$d, target_url:$u}') api_post "repos/$owner/$repo/statuses/$sha" "$status_payload" >/dev/null \ || die "failed to post the '$STATUS_CONTEXT' commit status on $short" printf 'posted status: %s = %s on %s\n' "$STATUS_CONTEXT" "$state" "$short" +# --- Is the verdict we just wrote one the GATE would honour? (ersatztv#845) --------------------- +# The gate inherits an existing `review-verdict/h10=success` only from a status whose +# `.creator.login` is on its `H10_REVIEWERS` allow-list (ersatztv#742). This script posts with +# whatever account owns the credential in the environment and used to never ask whose it was, so the +# two coupled values could drift apart with no diagnostic anywhere: the status really is written, +# this tool really did report success, and then the next `pull_request_target` event re-derives it +# and posts `pending` over it. Again on the next event. The PR deadlocks and the only trace is a +# `::warning::` inside a workflow run nobody is reading. +# +# MEASURED AFTER THE WRITE, NOT BEFORE IT. A pre-flight `GET /user` would test what the credential +# CLAIMS to be; this tests what Gitea actually recorded as the author of this status, which is the +# value the gate reads. It also needs no scope beyond the repo access the POST already required — +# a `/user` probe would add a read:user dependency that a narrowly-scoped but perfectly valid +# reviewer token might not carry, turning a hardening step into a new way to block every verdict. +# +# THE RESIDUAL, stated: on refusal a non-inheritable status is left standing on the head, because +# the check runs after the POST. That is the half-state `release.verdict-writes-status-before-comment` +# already designates as the safe one — a status with no comment leaves the merge hook at condition +# (c) with nothing to classify, which is an `ask`, not a grant — and the gate re-derives the status +# itself on the next PR event. It is not repaired here on purpose: a second corrective write is the +# sticky-sentinel mechanism ersatztv#849 is separately designing, and inventing a parallel one on +# this path would be two mechanisms for one invariant. +status_readback=$(api_get "repos/$owner/$repo/commits/$sha/status") \ + || die "the '$STATUS_CONTEXT' status was posted on $short, but it could not be read back to confirm the gate will honour it — no comment was written. Re-run once Gitea is reachable." +# `.statuses` serialises as `null` rather than `[]` on a head with no statuses (ersatztv#751), so +# the array TYPE is tested rather than assumed; a body that merely lost its array yields no entry +# and is refused below, exactly like one that carries no verdict. +readback_entry=$(printf '%s' "$status_readback" | jq -c --arg c "$STATUS_CONTEXT" \ + 'if (.statuses | type) == "array" then ((.statuses | map(select(.context == $c)))[0] // null) else null end') \ + || die "the '$STATUS_CONTEXT' status was posted on $short, but the read-back body could not be parsed — no comment was written." +if [ -z "$readback_entry" ] || [ "$readback_entry" = "null" ]; then + die "the '$STATUS_CONTEXT' status was posted on $short but is not present when read back, so it is UNPROVEN that the verdict landed — no comment was written. Re-run and check the PR's checks." +fi +readback_state=$(printf '%s' "$readback_entry" | jq -r '.state // ""') +readback_desc=$(printf '%s' "$readback_entry" | jq -r '.description // ""') +readback_creator=$(printf '%s' "$readback_entry" | jq -r '.creator.login // ""') +# Identify OUR write before judging its author. `/commits/{sha}/status` returns the LATEST status per +# context, so a run of the gate that overwrote ours between the POST and this read would otherwise +# have its own author judged here — an Actions-token write carries `creator: null` and would be +# reported as a wrong credential, which is a false and very confusing accusation. +if [ "$readback_state" != "$state" ] || [ "$readback_desc" != "$status_desc" ]; then + die "the '$STATUS_CONTEXT' status standing on $short is not the one just written (state '$readback_state', description '$readback_desc') — something overwrote it, so this verdict is not in force. No comment was written. Re-review the current head and run this again." +fi +if [ -z "$readback_creator" ]; then + die "the '$STATUS_CONTEXT' status on $short reads back with no creator, which is how a status posted by an ACTIONS token appears. The gate never inherits one of those as a reviewer verdict (ersatztv#742), so this verdict would be silently re-derived. No comment was written. Post it with a reviewer's own credential." +fi +# Explicit capture rather than `if ! etv_h10_reviewers_contains ...`, so the THIRD outcome cannot be +# folded into the second. The function answers 0 member / 1 not-a-member / 2 cannot-tell, and an +# abort inside it lands here as a non-zero code rather than skipping both arms of an `if !`. +set +e +etv_h10_reviewers_contains "$readback_creator" +h10_membership=$? +set -e +case "$h10_membership" in + 0) ;; + 1) + die "the '$STATUS_CONTEXT' status on $short was posted by '$readback_creator', which is NOT on the gate's H10_REVIEWERS allow-list ('$ETV_H10_REVIEWERS'). The status is written but the gate will not inherit it: the next PR event re-derives it and posts over it, and the PR deadlocks with no visible cause. No comment was written. Either post with an allow-listed reviewer's credential, or add '$readback_creator' to H10_REVIEWERS in .gitea/workflows/review-verdict.yml." + ;; + *) + die "could not determine whether '$readback_creator' is on the gate's H10_REVIEWERS allow-list (membership check returned $h10_membership). Refusing to report a verdict as posted when it is unknown whether the gate will honour it. No comment was written." + ;; +esac +printf 'verdict author: %s (on the gate allow-list: %s)\n' "$readback_creator" "$ETV_H10_REVIEWERS" + # --- The comment (human-readable artifact + the hook's condition-(c) input). -------------------- # Written LAST, after the gating status exists (ersatztv#792). The verdict line MUST start the line: # the hook anchors its parser to line-start precisely so a comment that merely QUOTES the template diff --git a/scripts/tests/mutation_manifest.py b/scripts/tests/mutation_manifest.py index 3e6796d07..d09fde233 100644 --- a/scripts/tests/mutation_manifest.py +++ b/scripts/tests/mutation_manifest.py @@ -387,6 +387,22 @@ MUTATIONS: tuple[Mutation, ...] = ( "#807's silent weight reset live again. The row is named in full rather than by the bare " "disposition line, which occurs three times and would identify none of them.", ), + Mutation( + guard="scripts/post-review-verdict.sh", + target=".gitea/workflows/review-verdict.yml", + clause='H10_REVIEWERS="timothy"', + replacement='H10_REVIEWERS="somebody-who-is-not-a-reviewer"', + proof="test_post_review_verdict.py::test_a_verdict_posted_by_an_ALLOWLISTED_account_is_accepted", + granularity=CLAUSE, + expect="the verdict writer REFUSED an allow-listed account", + why="THE COUPLING, NOT THE COMPARISON. The guard is the writer; the clause lives in the GATE, " + "because what ersatztv#845 is about is the two drifting apart. Mutating the gate's own " + "allow-list while the posting account stays fixed reddens the accept path only if BOTH hold: " + "the writer reads the list LIVE from the workflow (a hand-copied list would not move), and " + "the membership comparison actually gates the outcome (a no-op comparison would not care " + "that it moved). Disarming the comparison inside the script instead would prove the second " + "and say nothing about the first, which is the half that was missing.", + ), ) @@ -477,9 +493,6 @@ UNDECLARED: dict[str, str] = { "harness's.", "scripts/jq-preflight.sh": "BEHAVIOUR-ONLY. Its named test drives the real script below the " "version floor; no clause has been disarmed to show the floor comparison is what refuses.", - "scripts/post-review-verdict.sh": "BEHAVIOUR-ONLY, and the most valuable upgrade on this list: " - "it writes the required `review-verdict/h10` status. Its re-read-and-compare has several arms " - "and naming one as THE clause needs the judgement #790 declines to make blind.", "scripts/pr-changed-files.sh": "BEHAVIOUR-ONLY. Its named test feeds a short page to the real " "enumeration; the pagination clause has not been disarmed.", "scripts/tests/test_ci_release_path_scan_job.py": "A GUARD that is a test, so the mutation would " diff --git a/scripts/tests/test_post_review_verdict.py b/scripts/tests/test_post_review_verdict.py index c90ee94a9..14b90d2f6 100644 --- a/scripts/tests/test_post_review_verdict.py +++ b/scripts/tests/test_post_review_verdict.py @@ -24,6 +24,8 @@ from __future__ import annotations import json import os +import re +import shutil import subprocess from pathlib import Path @@ -87,6 +89,36 @@ if "/pulls/" in url and not url.endswith("/files"): print(json.dumps(body)) sys.exit(0) +# GET /repos///commits//status -> the COMBINED status (latest per context). +# It REPLAYS the status that was actually POSTed, so the script's identify-our-own-write comparison +# is satisfied by construction and tests only have to vary the one field under test: the CREATOR +# Gitea recorded (ersatztv#845). `status_body` overrides the whole response for the shapes a replay +# cannot express — an overwrite, a `statuses: null` body, a malformed one. +if "/commits/" in url and url.split("?")[0].endswith("/status"): + if (state / "status_fail").exists(): + sys.exit(22) + raw = state / "status_body" + if raw.exists(): + print(raw.read_text()) + sys.exit(0) + entry = None + log = state / "posts.jsonl" + if log.exists(): + for line in log.read_text().splitlines(): + if not line.strip(): + continue + post = json.loads(line) + if "/statuses/" in post["url"]: + entry = dict(post["payload"]) # last write wins, as the combined endpoint does + if entry is None: + print(json.dumps({"total_count": 0, "statuses": None})) + sys.exit(0) + who = (state / "status_creator").read_text().strip() if (state / "status_creator").exists() else "timothy" + # 'NULL' is how a status POSTed by an ACTIONS token reads back — the case the gate re-derives. + entry["creator"] = None if who == "NULL" else {"login": who} + print(json.dumps({"total_count": 1, "statuses": [entry]})) + sys.exit(0) + print("{}") ''' @@ -133,6 +165,19 @@ def gitea(tmp_path): """Base branch per PR GET. 'MISSING' omits `.base` from the response entirely.""" (state / "pr_bases").write_text(" ".join(refs)) + def set_status_creator(self, login): + """Who Gitea records as the author of the read-back status. 'NULL' models an Actions + token, whose statuses carry `creator: null` (ersatztv#742/#845).""" + (state / "status_creator").write_text(login) + + def fail_status_readback(self): + """Make the combined-status GET fail, as curl -f does on a 4xx/5xx.""" + (state / "status_fail").write_text("1") + + def set_status_body(self, body): + """Replace the whole combined-status response with this literal JSON text.""" + (state / "status_body").write_text(body) + def run(self, *args): return subprocess.run( ["bash", str(SCRIPT), *args], @@ -530,3 +575,205 @@ def test_a_failed_COMMENT_after_a_written_status_is_still_an_error(gitea): assert gitea.comments() == [] assert "COMMENT could not be posted" in result.stderr assert "ask" in result.stderr.lower(), "it must say what the gate will do, not just that a call failed" + + +# ================================================================================================== +# Is the verdict this tool writes one the GATE will actually inherit? (ersatztv#845) +# ================================================================================================== +# +# `.gitea/workflows/review-verdict.yml` inherits an existing `review-verdict/h10=success` only from a +# status whose `.creator.login` is on its `H10_REVIEWERS` allow-list (ersatztv#742). This script +# posts with whatever account owns the credential in the environment. Those two values were coupled +# with nothing asserting the coupling, and the failure was silent in the worst way: the status is +# written, THIS TOOL REPORTS SUCCESS, and the next PR event re-derives it and posts over it. Forever. +# +# The account below is written as the literal `timothy` on purpose, and it is the same deliberate +# hardcode `test_pr_changed_files.py::test_the_allowlisted_reviewer_in_the_TESTS_is_the_one_the_ +# WORKFLOW_ships` exists to pin. Deriving it here would make these tests vacuous: the mutation proof +# works precisely BECAUSE changing the workflow literal moves the allow-list out from under a fixed +# creator. Derive both and they would move together and nothing would ever redden. +ALLOWLISTED = "timothy" +OFF_LIST = "renovate" + + +def test_a_verdict_posted_by_an_ALLOWLISTED_account_is_accepted(gitea): + """The accept path — and the mutation proof for the whole derivation. + + This is the test `mutation_manifest.py` reddens by rewriting `H10_REVIEWERS` in the SHIPPED + workflow. That single mutation proves two things at once, which is why it is the declared one: + the script must be reading the allow-list LIVE from the gate (a hand-copied list would not + move), and the membership comparison must actually gate the outcome (a no-op comparison would + not care that the list moved). + """ + gitea.set_status_creator(ALLOWLISTED) + result = gitea.run("42", "MERGEABLE") + + assert result.returncode == 0, f"the verdict writer REFUSED an allow-listed account: {result.stderr}" + assert len(gitea.statuses()) == 1 + assert len(gitea.comments()) == 1, "an accepted verdict must still post its comment" + + +def test_a_verdict_posted_by_an_account_OFF_the_allowlist_is_refused_before_the_comment(gitea): + """The whole point of ersatztv#845: this used to report success and deadlock the PR later.""" + gitea.set_status_creator(OFF_LIST) + result = gitea.run("42", "MERGEABLE") + + assert result.returncode != 0, "a verdict the gate will not inherit must not report success" + assert "H10_REVIEWERS" in result.stderr, ( + "the diagnostic must name the allow-list, because the repair is to edit it or to re-post " + "with another credential — an operator cannot act on 'refused'" + ) + assert OFF_LIST in result.stderr, "the diagnostic must name the account that actually posted" + assert gitea.comments() == [], ( + "the refusal must land BEFORE the comment: a verdict comment with no honoured status is the " + "half-state ersatztv#792 removed, and it reads to a human as consent that was never granted" + ) + # Stated rather than fixed. The status IS written by the time the author can be measured, and it + # is left standing deliberately — see the residual note in the script. + assert len(gitea.statuses()) == 1 + + +def test_a_status_that_reads_back_with_NO_creator_is_refused(gitea): + """`creator: null` is how a status POSTed by an ACTIONS token appears, and the gate never + inherits one of those as a reviewer verdict. Distinct from the off-list case: there is no + account to name, so 'add them to the allow-list' would be the wrong advice.""" + gitea.set_status_creator("NULL") + result = gitea.run("42", "MERGEABLE") + + assert result.returncode != 0 + assert "ACTIONS token" in result.stderr + assert gitea.comments() == [] + + +def test_a_status_OVERWRITTEN_between_the_write_and_the_read_is_not_blamed_on_its_author(gitea): + """`/commits/{sha}/status` returns the LATEST status per context. If the gate overwrote ours + between the POST and the read-back, judging THAT status's author would accuse the wrong party — + and an Actions write carries no creator, so it would surface as a credential problem that does + not exist. Identify our own write first, then judge it.""" + gitea.set_status_body( + json.dumps( + { + "total_count": 1, + "statuses": [ + { + "context": "review-verdict/h10", + "state": "pending", + "description": "Awaiting review verdict", + "creator": None, + } + ], + } + ) + ) + result = gitea.run("42", "MERGEABLE") + + assert result.returncode != 0 + assert "overwrote it" in result.stderr + assert "ACTIONS token" not in result.stderr, ( + "an overwritten status must not be reported as a wrong-credential problem" + ) + assert gitea.comments() == [] + + +def test_an_unreadable_readback_refuses_rather_than_reporting_the_verdict_posted(gitea): + """UNKNOWN must fail. A read that cannot be performed leaves it unproven that the verdict is in + force, and reporting success on unproven is exactly the ersatztv#845 shape.""" + gitea.fail_status_readback() + result = gitea.run("42", "MERGEABLE") + + assert result.returncode != 0 + assert gitea.comments() == [] + + +def test_a_readback_whose_statuses_array_is_NULL_is_refused(gitea): + """Gitea serialises a nil slice as `null`, not `[]` (ersatztv#751). The script tests the array + TYPE rather than assuming one; a body that merely lost its array must refuse, not be read as + 'no verdict, carry on'.""" + gitea.set_status_body(json.dumps({"total_count": 0, "statuses": None})) + result = gitea.run("42", "MERGEABLE") + + assert result.returncode != 0 + assert gitea.comments() == [] + + +# --- The allow-list is DERIVED from the gate, not restated beside it. ---------------------------- + + +def _sandbox(tmp_path, reviewers): + """A throwaway tree carrying the real `scripts/` and the real gate workflow, with the workflow's + `H10_REVIEWERS` literal rewritten. Returns a runner over the SANDBOX copy of the script. + + The whole `scripts/` directory is copied rather than the three files in play, because the script + resolves both of its libraries relative to `${BASH_SOURCE[0]}` — copying selectively would leave + it sourcing the real repo's copies and the rewrite would prove nothing. + """ + root = tmp_path / "sandbox" + shutil.copytree(REPO_ROOT / "scripts", root / "scripts") + workflow = root / ".gitea" / "workflows" / "review-verdict.yml" + workflow.parent.mkdir(parents=True) + source = (REPO_ROOT / ".gitea" / "workflows" / "review-verdict.yml").read_text() + rewritten, n = re.subn( + r'^(\s*)H10_REVIEWERS="[^"]*"$', + lambda m: f'{m.group(1)}H10_REVIEWERS="{reviewers}"', + source, + flags=re.MULTILINE, + ) + assert n == 1, f"expected exactly one H10_REVIEWERS assignment to rewrite, substituted {n}" + workflow.write_text(rewritten) + return root / "scripts" / "post-review-verdict.sh" + + +def test_the_allowlist_MOVES_when_the_workflow_moves(gitea, tmp_path): + """The single-declaration property, measured end to end rather than argued. + + Rewrite the gate's own literal to an account this repo does not have, drive the REAL script, and + the accounts swap roles: the one the shipped workflow accepts is refused, and the invented one is + accepted. A copy of the list living beside the script could not produce that. + """ + invented = "not-a-real-account" + script = _sandbox(tmp_path, invented) + + def run_with(creator): + gitea.set_status_creator(creator) + (gitea.state / "posts.jsonl").unlink(missing_ok=True) + (gitea.state / "pr_get_count").unlink(missing_ok=True) + return subprocess.run( + ["bash", str(script), "42", "MERGEABLE"], + env=gitea.env, + capture_output=True, + text=True, + ) + + accepted = run_with(invented) + assert accepted.returncode == 0, f"the rewritten allow-list was not the one used: {accepted.stderr}" + + refused = run_with(ALLOWLISTED) + assert refused.returncode != 0, ( + f"{ALLOWLISTED} was accepted against a workflow that no longer lists them, so the script is " + "not deriving the allow-list from the gate" + ) + assert invented in refused.stderr + + +def test_a_workflow_whose_allowlist_cannot_be_DERIVED_refuses_before_any_write(gitea, tmp_path): + """Fail-closed, and early. Two assignments means the shell would run the last one executed, so + which value the gate uses is unknowable from here — and a writer that cannot tell whether its + verdict is inheritable must not post one. Nothing is written at all, so there is no half-state + to clean up. + """ + script = _sandbox(tmp_path, ALLOWLISTED) + text = script.parent.parent.joinpath(".gitea/workflows/review-verdict.yml") + body = text.read_text() + marker = f'H10_REVIEWERS="{ALLOWLISTED}"' + assert body.count(marker) == 1 + text.write_text(body.replace(marker, f'{marker}\n H10_REVIEWERS="someone-else"')) + + result = subprocess.run( + ["bash", str(script), "42", "MERGEABLE"], + env=gitea.env, + capture_output=True, + text=True, + ) + assert result.returncode != 0 + assert gitea.statuses() == [], "a derivation it cannot trust must refuse before writing anything" + assert gitea.comments() == [] diff --git a/scripts/tests/test_review_verdict_vocabulary.py b/scripts/tests/test_review_verdict_vocabulary.py index 8c05b562e..731cfe598 100644 --- a/scripts/tests/test_review_verdict_vocabulary.py +++ b/scripts/tests/test_review_verdict_vocabulary.py @@ -52,6 +52,14 @@ class Sandbox: def __init__(self, root: Path): self.scripts = root / "scripts" shutil.copytree(REPO_ROOT / "scripts", self.scripts) + # The gate workflow comes too. `post-review-verdict.sh` derives the H10 reviewer allow-list + # from it (ersatztv#845), resolving it relative to its own location — so a sandbox holding + # only `scripts/` would leave the writer unable to answer whether its verdict is inheritable, + # and every write-side assertion below would fail for a reason that has nothing to do with + # the vocabulary under test. + workflow = root / ".gitea" / "workflows" / "review-verdict.yml" + workflow.parent.mkdir(parents=True) + shutil.copy2(REPO_ROOT / ".gitea" / "workflows" / "review-verdict.yml", workflow) 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" -- 2.47.3 From 8c580cbfcd887e7b07f734b04def6fa169fc7e70 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 29 Aug 2026 23:08:23 +0200 Subject: [PATCH 02/17] =?UTF-8?q?fix(845):=20read=20`.status`,=20not=20`.s?= =?UTF-8?q?tate`=20=E2=80=94=20the=20fixture=20was=20mirroring=20my=20POST?= =?UTF-8?q?=20body,=20not=20Gitea?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review defect, caught by probing the live instance rather than re-reading the code. A row inside `.statuses[]` serialises its state under the key `status`. `state` exists only as the AGGREGATE at the top level of the combined body. Measured on Gitea 1.27.1 (2026-08-29) against a real `review-verdict/h10` row on PR #882's head. Reading `.state` per row therefore yields "" for every well-formed response, which compares unequal to the state just posted — so the previous commit would have refused EVERY verdict with "something overwrote it". Since no PR can merge to `main` without a `review-verdict/h10` success, that is a repo-wide deadlock, shipped green. WHY THE TESTS DID NOT CATCH IT, which is the more useful half. The `curl` shim replays the status POST payload as the read-back body. A POST body legitimately uses `state` — that is the field the write API takes — so the fixture agreed with the parser BY CONSTRUCTION and the pair was self-consistent and both wrong. A fixture derived from the code under test cannot disagree with it. The shim now renames the field on replay, and a separate test feeds a VERBATIM live-Gitea body so the real shape is pinned independently of the shim. Measured, both directions: with the fixture fixed and `.state` restored, 25 tests redden; before the fixture fix, zero did. The gate reads the same endpoint and the same key (`review-verdict.yml` line 520, `ex_state=$(... jq -r '.status // ""')`), so the writer and the gate are now wrong or right together rather than independently. `.state` is kept as a fallback so a server spelling it the other way degrades to working rather than to a deadlock; it is not a second source of truth, since the value still has to equal the state this run posted. A row carrying NEITHER key now gets its own message. It is a response-SHAPE problem, not a race, and "something overwrote it" would state a cause that did not happen — the #859 defect class. fixes #845 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW --- scripts/post-review-verdict.sh | 17 ++++- scripts/tests/test_post_review_verdict.py | 82 ++++++++++++++++++++++- 2 files changed, 96 insertions(+), 3 deletions(-) diff --git a/scripts/post-review-verdict.sh b/scripts/post-review-verdict.sh index 79755d72c..d5b7de72a 100755 --- a/scripts/post-review-verdict.sh +++ b/scripts/post-review-verdict.sh @@ -280,9 +280,24 @@ readback_entry=$(printf '%s' "$status_readback" | jq -c --arg c "$STATUS_CONTEXT if [ -z "$readback_entry" ] || [ "$readback_entry" = "null" ]; then die "the '$STATUS_CONTEXT' status was posted on $short but is not present when read back, so it is UNPROVEN that the verdict landed — no comment was written. Re-run and check the PR's checks." fi -readback_state=$(printf '%s' "$readback_entry" | jq -r '.state // ""') +# `.status`, NOT `.state`. A row inside `.statuses[]` serialises its state under the key `status`; +# `state` exists only as the AGGREGATE at the top level of the combined body. Measured on Gitea +# 1.27.1, 2026-08-29, against a real `review-verdict/h10` row. Reading `.state` here yields the empty +# string for every well-formed response, which compares unequal to the state just posted and would +# have refused EVERY verdict — a repo-wide deadlock, since no PR can merge without one. The gate +# reads the same endpoint and the same key (`review-verdict.yml`, `ex_state=$(... jq -r '.status')`), +# so the two stay wrong or right together. `.state` is accepted as a fallback only so a server that +# spells it the other way degrades to working rather than to a deadlock; it is never a second source +# of truth, because the value still has to EQUAL the state this run posted. +readback_state=$(printf '%s' "$readback_entry" | jq -r '(.status // .state) // ""') readback_desc=$(printf '%s' "$readback_entry" | jq -r '.description // ""') readback_creator=$(printf '%s' "$readback_entry" | jq -r '.creator.login // ""') +# A row carrying NEITHER key is a SHAPE problem, not an overwrite. Kept separate because a refusal +# that states a cause which did not happen is its own defect class in this repo (#859): "something +# overwrote it" would send the reader looking for a race that never occurred. +if [ -z "$readback_state" ]; then + die "the '$STATUS_CONTEXT' status on $short read back with no state field, so it is UNPROVEN that the verdict just written is the one standing — no comment was written. This is a response-SHAPE problem, not a race: check what \`.statuses[]\` rows look like on this Gitea version." +fi # Identify OUR write before judging its author. `/commits/{sha}/status` returns the LATEST status per # context, so a run of the gate that overwrote ours between the POST and this read would otherwise # have its own author judged here — an Actions-token write carries `creator: null` and would be diff --git a/scripts/tests/test_post_review_verdict.py b/scripts/tests/test_post_review_verdict.py index 14b90d2f6..ec99384fd 100644 --- a/scripts/tests/test_post_review_verdict.py +++ b/scripts/tests/test_post_review_verdict.py @@ -116,7 +116,13 @@ if "/commits/" in url and url.split("?")[0].endswith("/status"): who = (state / "status_creator").read_text().strip() if (state / "status_creator").exists() else "timothy" # 'NULL' is how a status POSTed by an ACTIONS token reads back — the case the gate re-derives. entry["creator"] = None if who == "NULL" else {"login": who} - print(json.dumps({"total_count": 1, "statuses": [entry]})) + # RENAME state -> status. This is the one place the replay must NOT mirror the POST body: a row + # inside `.statuses[]` serialises its state under `status`, while `state` is only the aggregate + # at the top level. Replaying the POST payload verbatim would make this shim agree with the + # script's parser by construction and test nothing — the shape below is what Gitea 1.27.1 + # actually returns, measured 2026-08-29. + entry["status"] = entry.pop("state") + print(json.dumps({"state": entry["status"], "total_count": 1, "statuses": [entry]})) sys.exit(0) print("{}") @@ -657,7 +663,7 @@ def test_a_status_OVERWRITTEN_between_the_write_and_the_read_is_not_blamed_on_it "statuses": [ { "context": "review-verdict/h10", - "state": "pending", + "status": "pending", "description": "Awaiting review verdict", "creator": None, } @@ -777,3 +783,75 @@ def test_a_workflow_whose_allowlist_cannot_be_DERIVED_refuses_before_any_write(g assert result.returncode != 0 assert gitea.statuses() == [], "a derivation it cannot trust must refuse before writing anything" assert gitea.comments() == [] + + +def test_a_VERBATIM_live_gitea_status_body_is_accepted(gitea): + """Pins the real response shape, independently of the shim that serves it. + + The shim replays what was POSTed, so it can only ever disagree with the parser about a field it + is told to rename — which is a fixture agreeing with the code by construction. This body is the + shape `GET /repos/{o}/{r}/commits/{sha}/status` actually returned on Gitea 1.27.1 (measured + 2026-08-29 against a real `review-verdict/h10` row), trimmed of fields nothing reads. The key + that matters is `status`: a row inside `.statuses[]` carries its state under that name, while + `state` is only the top-level AGGREGATE. Parsing `.state` per row yields "" for every + well-formed response, refuses every verdict, and deadlocks the repo — no PR merges without one. + """ + short = SHA_A[:7] + gitea.set_status_body( + json.dumps( + { + "sha": SHA_A, + "state": "success", # the AGGREGATE, deliberately present and deliberately not read + "total_count": 2, + "statuses": [ + { + "id": 32, + "status": "success", + "description": f"Review-verdict: MERGEABLE @ {short} (base: main)", + "context": "review-verdict/h10", + "creator": {"id": 1, "login": ALLOWLISTED, "username": ALLOWLISTED}, + "created_at": "2026-08-29T21:40:22+02:00", + }, + { + "id": 31, + "status": "success", + "description": "Build & test", + "context": "ci/build", + "creator": None, + }, + ], + } + ) + ) + result = gitea.run("42", "MERGEABLE") + assert result.returncode == 0, f"the verdict writer REFUSED a verbatim live Gitea response: {result.stderr}" + assert len(gitea.comments()) == 1 + + +def test_a_status_row_carrying_NO_state_field_says_so_instead_of_blaming_a_race(gitea): + """A shape it cannot read is not an overwrite. Stating a cause that did not happen is its own + defect class here (#859) — 'something overwrote it' would send the reader hunting a race that + never occurred, on what is really a Gitea-version problem.""" + short = SHA_A[:7] + gitea.set_status_body( + json.dumps( + { + "total_count": 1, + "statuses": [ + { + "context": "review-verdict/h10", + "description": f"Review-verdict: MERGEABLE @ {short} (base: main)", + "creator": {"login": ALLOWLISTED}, + } + ], + } + ) + ) + result = gitea.run("42", "MERGEABLE") + + assert result.returncode != 0 + assert "no state field" in result.stderr + assert "overwrote it" not in result.stderr, ( + "an unreadable response shape must not be reported as a race that did not happen" + ) + assert gitea.comments() == [] -- 2.47.3 From 785edbe6e6db7ca576d78c667ab27a148703431c Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 29 Aug 2026 23:10:20 +0200 Subject: [PATCH 03/17] fix(845): the read-back asks for a page big enough, and stops naming a cause it cannot know MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second self-review defect, again found by probing rather than reading. `GET /commits/{sha}/status` PAGES, and it pages undetectably: measured on Gitea 1.27.1 (2026-08-29), `?limit=2` against a head carrying 15 contexts returned 2 rows AND `total_count: 2`. The count reports the PAGE, not the total, so no field in the body distinguishes a truncated response from a complete one. A head whose context count grew past the server's default page size would silently stop containing the `review-verdict/h10` row, and the read-back would refuse a verdict that is actually in force — the deadlock direction, on the one path that must not deadlock. The call now asks for `?limit=100` against the ~15 contexts this repo produces. The not-found refusal no longer states a cause. Two causes reach it — the status is gone, or the list was truncated — and the response cannot tell them apart, so asserting either would be the #859 defect: a message that sends the reader hunting something that did not happen. It now reports how many rows it actually read and names neither, which also makes a future paging overflow legible instead of mysterious. Recorded in `docs/remote-state-inventory.md` on the writer's own row, since it is a property of that read rather than of the tool. fixes #845 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW --- docs/remote-state-inventory.md | 2 +- scripts/post-review-verdict.sh | 16 ++++++++++++-- scripts/tests/test_post_review_verdict.py | 26 +++++++++++++++++++++++ 3 files changed, 41 insertions(+), 3 deletions(-) diff --git a/docs/remote-state-inventory.md b/docs/remote-state-inventory.md index 137a843e7..812a96b03 100644 --- a/docs/remote-state-inventory.md +++ b/docs/remote-state-inventory.md @@ -96,7 +96,7 @@ classifications differ; otherwise the strictest applies and the Note names the e | Site | Class | Note | |---|---|---| -| `scripts/post-review-verdict.sh` — commit-status write | `PINNED` | Re-reads the PR and compares **both** `.head.sha` and `.base.ref` immediately before the POST, and `die`s (exit 1, no status written) on a mismatch **or on a field it cannot read**. That last clause is load-bearing: guarding both comparisons with `[ -n "$x" ] &&` alone makes a well-formed 2xx body that merely omits the field a no-op, and the status is posted having confirmed nothing. Regression-tested against the real predecessor (#778), since the redundant-looking `-z` arm alone mutates green. Closes #706 and #632 for this path by read-compare-refuse, not by CAS: Gitea's status API offers no conditional write. Residual closed on the write ORDER since #792: the status is written first and the comment second, so a refusal can no longer leave a verdict comment with no status behind it — the only reachable half-state is a status with no comment, which the merge hook resolves as `ask` (`release.verdict-writes-status-before-comment`). Since #845 the status is also READ BACK after the POST and refused unless it is the write just made AND its `.creator.login` is on the allow-list derived from the gate — the same read-compare-refuse shape, applied to WHOSE verdict it is rather than to which commit it covers. That read is check-then-act too and is graded with this row rather than separately: it can only withhold this tool's success report, never authorize anything, and the residual it leaves — a non-inheritable status standing with no comment beside it — is the `ask` half-state above. | +| `scripts/post-review-verdict.sh` — commit-status write | `PINNED` | Re-reads the PR and compares **both** `.head.sha` and `.base.ref` immediately before the POST, and `die`s (exit 1, no status written) on a mismatch **or on a field it cannot read**. That last clause is load-bearing: guarding both comparisons with `[ -n "$x" ] &&` alone makes a well-formed 2xx body that merely omits the field a no-op, and the status is posted having confirmed nothing. Regression-tested against the real predecessor (#778), since the redundant-looking `-z` arm alone mutates green. Closes #706 and #632 for this path by read-compare-refuse, not by CAS: Gitea's status API offers no conditional write. Residual closed on the write ORDER since #792: the status is written first and the comment second, so a refusal can no longer leave a verdict comment with no status behind it — the only reachable half-state is a status with no comment, which the merge hook resolves as `ask` (`release.verdict-writes-status-before-comment`). Since #845 the status is also READ BACK after the POST and refused unless it is the write just made AND its `.creator.login` is on the allow-list derived from the gate — the same read-compare-refuse shape, applied to WHOSE verdict it is rather than to which commit it covers. That read is check-then-act too and is graded with this row rather than separately: it can only withhold this tool's success report, never authorize anything, and the residual it leaves — a non-inheritable status standing with no comment beside it — is the `ask` half-state above. One property of that read is worth recording because no field in the response carries it: `GET /commits/{sha}/status` PAGES, and its `total_count` reports the PAGE rather than the total (measured Gitea 1.27.1, 2026-08-29 — `?limit=2` on a 15-context head returned 2 rows and `total_count: 2`), so a truncated body is indistinguishable from a complete one. The call asks for `?limit=100` against the ~15 contexts this repo produces, and a row it cannot find is refused with the row count it read and NO stated cause, since absence and truncation are not separable here. | | `scripts/pr-changed-files.sh` — paged file enumeration | `UNSAFE-KNOWN` | #707's fix: `.base.ref`, `.base.sha` and `.head.sha` are captured before paging and re-checked after, and any *observed* movement fails the whole enumeration closed rather than emitting a short list. But before-and-after equality is **ABA-vulnerable** — a `main → scratch → main` retarget during paging can return the same ref and, if nothing merged meanwhile, the same base sha, while the pages in between were diffed against the scratch base. The script's own comment says it narrows rather than erases — "any movement fails" would be stronger than the code. Accepted here because the enumerator cannot close it alone, but be exact about what the caller-side fence does and does not cover: `ci.verdict-write-retarget-fence` counts `change_target_branch` events, so it catches the BASE alias and **nothing else**. A HEAD alias — a force-push `H1 -> H2 -> H1` during pagination — leaves the final `.head.sha` comparison equal while the middle pages were enumerated against `H2`, and NOTHING inside this script can see it. **Fenced at the enforced caller since #803** (2026-08-28): the same timeline walk now also counts `pull_push`, so a head that moved at all during classification withholds the write — subject to that walk's own terminator, which is defeatable by a page of filtered rows (**#870**, pre-existing and equally affecting the base axis). The grade stays `UNSAFE-KNOWN` for the SCRIPT, because the script's own guarantee is unchanged and one-way — a caller that does not run the fence inherits the alias. Enumeration bound to an immutable tree remains unavailable: re-probed at 1.27.1 on 2026-08-28, `compare/{base}...{head}` still returns no `files` key. | | `scripts/select-queue.sh` — issue list, then per-issue `/dependencies` | `UNSAFE-KNOWN` | The open-issue list (labels, milestone, priority) is snapshotted once; per-candidate dependency reads happen seconds later and never re-read the issue's own labels, so an issue claimed `in-progress` in that gap still appears on the shortlist. Accepted: the script authorizes **no write**. The real gate is the four-way claim check in `process.parallel-session-claim`, which runs after selection and re-reads live state by construction. Tightening this would move a check that must be adversarial into a tool that is advisory. | | `scripts/ci-detect-already-validated.sh` — prior-head combined status | `UNSAFE-KNOWN` | Reads the PR head's status and emits `skip=true`, with nothing re-checking before the consuming job runs. Accepted and narrow: the skip elides only **re-running** test/migrations on a tree already validated; the `build` job still builds and pushes unconditionally, so no image ever ships from unvalidated source. | diff --git a/scripts/post-review-verdict.sh b/scripts/post-review-verdict.sh index d5b7de72a..5d02a471b 100755 --- a/scripts/post-review-verdict.sh +++ b/scripts/post-review-verdict.sh @@ -269,7 +269,15 @@ printf 'posted status: %s = %s on %s\n' "$STATUS_CONTEXT" "$state" "$short" # itself on the next PR event. It is not repaired here on purpose: a second corrective write is the # sticky-sentinel mechanism ersatztv#849 is separately designing, and inventing a parallel one on # this path would be two mechanisms for one invariant. -status_readback=$(api_get "repos/$owner/$repo/commits/$sha/status") \ +# `?limit=100` is not decoration. This endpoint PAGES, and it pages in a way that cannot be detected +# from the response: measured on Gitea 1.27.1 (2026-08-29), `?limit=2` on a head carrying 15 contexts +# returned 2 rows AND `total_count: 2` — the count reports the PAGE, not the total, so a truncated +# body is indistinguishable from a complete one by any field in it. A head whose context count grows +# past the server default page size would silently stop containing our row, and the refusal below +# would fire on a verdict that is in force. 100 is far above the 15 contexts this repo produces, and +# the diagnostic states the row count it actually read so a future overflow is legible rather than +# mysterious. +status_readback=$(api_get "repos/$owner/$repo/commits/$sha/status?limit=100") \ || die "the '$STATUS_CONTEXT' status was posted on $short, but it could not be read back to confirm the gate will honour it — no comment was written. Re-run once Gitea is reachable." # `.statuses` serialises as `null` rather than `[]` on a head with no statuses (ersatztv#751), so # the array TYPE is tested rather than assumed; a body that merely lost its array yields no entry @@ -278,7 +286,11 @@ readback_entry=$(printf '%s' "$status_readback" | jq -c --arg c "$STATUS_CONTEXT 'if (.statuses | type) == "array" then ((.statuses | map(select(.context == $c)))[0] // null) else null end') \ || die "the '$STATUS_CONTEXT' status was posted on $short, but the read-back body could not be parsed — no comment was written." if [ -z "$readback_entry" ] || [ "$readback_entry" = "null" ]; then - die "the '$STATUS_CONTEXT' status was posted on $short but is not present when read back, so it is UNPROVEN that the verdict landed — no comment was written. Re-run and check the PR's checks." + # Deliberately does NOT assert why. Two causes reach here and the response cannot tell them apart: + # the status genuinely is not there, or the row list was truncated by paging. Naming one would be + # stating a cause that may not have happened (#859). + readback_rows=$(printf '%s' "$status_readback" | jq -r 'if (.statuses | type) == "array" then (.statuses | length) else "unreadable" end' 2>/dev/null) || readback_rows='unreadable' + die "the '$STATUS_CONTEXT' status was posted on $short but is not among the $readback_rows status rows read back, so it is UNPROVEN that the verdict landed — no comment was written. Either it was removed, or this head now carries more status contexts than one page returns. Check the PR's checks before re-running." fi # `.status`, NOT `.state`. A row inside `.statuses[]` serialises its state under the key `status`; # `state` exists only as the AGGREGATE at the top level of the combined body. Measured on Gitea diff --git a/scripts/tests/test_post_review_verdict.py b/scripts/tests/test_post_review_verdict.py index ec99384fd..c2e45b950 100644 --- a/scripts/tests/test_post_review_verdict.py +++ b/scripts/tests/test_post_review_verdict.py @@ -855,3 +855,29 @@ def test_a_status_row_carrying_NO_state_field_says_so_instead_of_blaming_a_race( "an unreadable response shape must not be reported as a race that did not happen" ) assert gitea.comments() == [] + + +def test_a_readback_MISSING_our_context_reports_the_row_count_and_names_no_cause(gitea): + """Two causes reach this branch and the response cannot tell them apart: the status is genuinely + gone, or the row list was truncated by paging. Measured on Gitea 1.27.1 (2026-08-29), the + combined endpoint's `total_count` reports the PAGE and not the total (`?limit=2` on a 15-context + head returned 2 rows and `total_count: 2`), so no field in the body distinguishes them. The + diagnostic therefore reports how many rows it read and names neither cause as the one.""" + gitea.set_status_body( + json.dumps( + { + "total_count": 2, + "statuses": [ + {"context": "ci/build", "status": "success", "creator": None}, + {"context": "ci/lint", "status": "success", "creator": None}, + ], + } + ) + ) + result = gitea.run("42", "MERGEABLE") + + assert result.returncode != 0 + assert "not among the 2 status rows" in result.stderr, ( + "the row count it actually read must be in the message, or a paging overflow is unreadable" + ) + assert gitea.comments() == [] -- 2.47.3 From 09dcfe76fb994d048060cdee3fb8193ec7791ab4 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 29 Aug 2026 23:11:32 +0200 Subject: [PATCH 04/17] fix(845): each read-back extraction dies with its own message, and the no-creator refusal stops asserting a cause MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Probed, not reasoned: a `creator` arriving as a STRING rather than an object makes jq exit 5 ("Cannot index string with string"). That already failed closed — the abort lands before the comment — but with only jq's stderr, which never says a status was left standing on the head. Each extraction now carries its own message. The no-creator refusal said the status "is how a status posted by an ACTIONS token appears". Three shapes reach that branch (`creator` absent, null, or an empty login) and only one of them is an Actions token, so the message now states the fact first and offers the cause as the likely one rather than the established one. fixes #845 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW --- scripts/post-review-verdict.sh | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/scripts/post-review-verdict.sh b/scripts/post-review-verdict.sh index 5d02a471b..28ecdfa84 100755 --- a/scripts/post-review-verdict.sh +++ b/scripts/post-review-verdict.sh @@ -301,9 +301,16 @@ fi # so the two stay wrong or right together. `.state` is accepted as a fallback only so a server that # spells it the other way degrades to working rather than to a deadlock; it is never a second source # of truth, because the value still has to EQUAL the state this run posted. -readback_state=$(printf '%s' "$readback_entry" | jq -r '(.status // .state) // ""') -readback_desc=$(printf '%s' "$readback_entry" | jq -r '.description // ""') -readback_creator=$(printf '%s' "$readback_entry" | jq -r '.creator.login // ""') +# Each extraction carries its own `|| die`. Without one they still fail closed — a jq error under +# `set -e` aborts the script before the comment — but with only jq's own stderr, which does not say +# that a status was left standing on the head. `.creator` arriving as a STRING rather than an object +# is the shape that reaches this (probed: jq exits 5, "Cannot index string with string"). +readback_state=$(printf '%s' "$readback_entry" | jq -r '(.status // .state) // ""') \ + || die "the '$STATUS_CONTEXT' status was posted on $short but its state could not be read from the response — no comment was written." +readback_desc=$(printf '%s' "$readback_entry" | jq -r '.description // ""') \ + || die "the '$STATUS_CONTEXT' status was posted on $short but its description could not be read from the response — no comment was written." +readback_creator=$(printf '%s' "$readback_entry" | jq -r '.creator.login // ""') \ + || die "the '$STATUS_CONTEXT' status was posted on $short but its creator could not be read from the response — no comment was written. The verdict is UNPROVEN, not necessarily wrong." # A row carrying NEITHER key is a SHAPE problem, not an overwrite. Kept separate because a refusal # that states a cause which did not happen is its own defect class in this repo (#859): "something # overwrote it" would send the reader looking for a race that never occurred. @@ -318,7 +325,10 @@ if [ "$readback_state" != "$state" ] || [ "$readback_desc" != "$status_desc" ]; die "the '$STATUS_CONTEXT' status standing on $short is not the one just written (state '$readback_state', description '$readback_desc') — something overwrote it, so this verdict is not in force. No comment was written. Re-review the current head and run this again." fi if [ -z "$readback_creator" ]; then - die "the '$STATUS_CONTEXT' status on $short reads back with no creator, which is how a status posted by an ACTIONS token appears. The gate never inherits one of those as a reviewer verdict (ersatztv#742), so this verdict would be silently re-derived. No comment was written. Post it with a reviewer's own credential." + # States the fact first and the likely cause second. `creator` absent, null, or carrying an empty + # login all land here; an ACTIONS-token status is the one that actually occurs, but saying it IS + # one would assert a cause this response cannot establish. + die "the '$STATUS_CONTEXT' status on $short reads back with no attributable creator (absent, null, or an empty login). A status POSTed by an ACTIONS token reads exactly this way, and the gate never inherits one as a reviewer verdict (ersatztv#742), so this verdict would be silently re-derived. No comment was written. Post it with a reviewer's own credential." fi # Explicit capture rather than `if ! etv_h10_reviewers_contains ...`, so the THIRD outcome cannot be # folded into the second. The function answers 0 member / 1 not-a-member / 2 cannot-tell, and an -- 2.47.3 From ca38f285d570228319ac6b6dc66c634c58ed4349 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 29 Aug 2026 23:13:16 +0200 Subject: [PATCH 05/17] docs(845): cite the measurement behind the latest-per-context claim instead of asserting it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment stated that `/commits/{sha}/status` returns the latest row per context as a bare fact. It was measured — a head carrying two `review-verdict/h10` rows in the `/statuses` list returned only the newer one from the combined endpoint — so the comment now says so. An unattributed claim about an API is the kind that quietly stops being true and talks the next reader out of re-checking it. refs #845 Co-Authored-By: Claude Opus 5 --- scripts/post-review-verdict.sh | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/scripts/post-review-verdict.sh b/scripts/post-review-verdict.sh index 28ecdfa84..36285849a 100755 --- a/scripts/post-review-verdict.sh +++ b/scripts/post-review-verdict.sh @@ -317,10 +317,12 @@ readback_creator=$(printf '%s' "$readback_entry" | jq -r '.creator.login // ""') if [ -z "$readback_state" ]; then die "the '$STATUS_CONTEXT' status on $short read back with no state field, so it is UNPROVEN that the verdict just written is the one standing — no comment was written. This is a response-SHAPE problem, not a race: check what \`.statuses[]\` rows look like on this Gitea version." fi -# Identify OUR write before judging its author. `/commits/{sha}/status` returns the LATEST status per -# context, so a run of the gate that overwrote ours between the POST and this read would otherwise -# have its own author judged here — an Actions-token write carries `creator: null` and would be -# reported as a wrong credential, which is a false and very confusing accusation. +# Identify OUR write before judging its author. `/commits/{sha}/status` returns the LATEST row per +# context — measured, not assumed: on 2026-08-29 a head carrying two `review-verdict/h10` rows in the +# `/statuses` list (an Actions `pending`, then a reviewer `success`) returned only the newer one here. +# So a run of the gate that overwrote ours between the POST and this read would otherwise have ITS +# author judged below, and an Actions write carries `creator: null` — which would surface as a +# wrong-credential accusation against a credential that is fine. if [ "$readback_state" != "$state" ] || [ "$readback_desc" != "$status_desc" ]; then die "the '$STATUS_CONTEXT' status standing on $short is not the one just written (state '$readback_state', description '$readback_desc') — something overwrote it, so this verdict is not in force. No comment was written. Re-review the current head and run this again." fi -- 2.47.3 From 5f93930a1877d8ddaea783d15990528cdb92c07d Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 29 Aug 2026 23:24:32 +0200 Subject: [PATCH 06/17] =?UTF-8?q?test(845):=20the=20library's=20own=20clau?= =?UTF-8?q?ses=20were=20untested=20=E2=80=94=20four=20of=20them,=20includi?= =?UTF-8?q?ng=20the=20glob=20fail-open?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by disarming the LIBRARY's clauses rather than the script's. The end-to-end tests only ever drive the real workflow, which is well-formed, so four clauses could be removed with the suite still green: - the plain-login validation that rejects a glob entry - the sentinel gate that makes `contains` answer CANNOT-TELL instead of NOT-A-MEMBER - the empty-list check - the restoration of the caller's `-f` setting after the split The first is the one that matters. `for x in $list` performs PATHNAME EXPANSION, so a `*` entry expands to the FILENAMES around it, and any of those matching the plain-login class would validate cleanly and enter the allow-list as a real reviewer. `review-verdict-vocabulary.sh` records exactly this happening to its own word list; the new test stocks the working directory with plausible login names so an expanding implementation loads a list instead of refusing. The sentinel one is a wrong-cause defect rather than a fail-open: without it a broken checkout answers NOT-A-MEMBER, and the script then tells the operator to fix a credential that is fine. A note on the measurement, because the first attempt was wrong. The initial disarm of the plain-login class widened it to `[!A-Za-z0-9._!@#$%^-]`, which still excludes `*` — so it mutated nothing and the "no test covers this" reading was an artifact of an inert mutation. The real disarm has to ADMIT `*` (`[!A-Za-z0-9._*-]`), and the probe now asserts that the mutant class accepts `*` before drawing any conclusion from the run. All four confirmed to redden their own named test. fixes #845 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW --- scripts/tests/test_post_review_verdict.py | 103 ++++++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/scripts/tests/test_post_review_verdict.py b/scripts/tests/test_post_review_verdict.py index c2e45b950..da6436335 100644 --- a/scripts/tests/test_post_review_verdict.py +++ b/scripts/tests/test_post_review_verdict.py @@ -881,3 +881,106 @@ def test_a_readback_MISSING_our_context_reports_the_row_count_and_names_no_cause "the row count it actually read must be in the message, or a paging overflow is unreadable" ) assert gitea.comments() == [] + + +# --- The derivation library itself. -------------------------------------------------------------- +# +# Driven directly rather than through the script, because three of its clauses cannot be reached from +# the script at all: the script only ever runs against the real workflow, which is well-formed. Each +# of these was measured to survive a disarm of its clause when only the end-to-end tests existed. + +LIB = REPO_ROOT / "scripts" / "lib" / "h10-reviewers.sh" + + +def _lib(snippet, cwd, workflow_body=None): + """Source the real library and run `snippet`. Returns the CompletedProcess. + + `cwd` matters: pathname expansion is resolved against it, which is the whole point of the glob + test below. A workflow body, when given, is written to `cwd/wf.yml`. + """ + if workflow_body is not None: + (cwd / "wf.yml").write_text(workflow_body) + script = f'set -euo pipefail\n. "{LIB}"\n{snippet}\n' + return subprocess.run(["bash", "-c", script], cwd=str(cwd), capture_output=True, text=True) + + +def test_a_GLOB_entry_is_rejected_rather_than_expanded_against_the_working_directory(tmp_path): + """The documented fail-open, reached one file over from where it was first found. + + `for x in $list` and `$(printf %s "$list")` both perform PATHNAME EXPANSION, so a `*` entry + expands to the FILENAMES around it — and any of those matching the plain-login class would + validate cleanly and enter the allow-list as a real reviewer. `review-verdict-vocabulary.sh` + records this happening to its own word list. The cwd below is stocked with names that would + validate, so an expanding implementation loads a non-empty list instead of refusing. + """ + for name in ("timothy", "renovate", "attacker"): + (tmp_path / name).write_text("") + result = _lib( + 'etv_h10_reviewers_load "$PWD/wf.yml" && echo "LOADED=[$ETV_H10_REVIEWERS]"', + tmp_path, + ' H10_REVIEWERS="*"\n', + ) + assert result.returncode != 0, ( + f"a `*` entry was accepted; it expanded to {result.stdout.strip()!r} in a directory " + "containing plausible login names" + ) + assert "LOADED=" not in result.stdout + assert "glob metacharacters" in result.stderr + + +def test_an_EMPTY_or_whitespace_only_allowlist_is_rejected(tmp_path): + """`" "` is a non-empty STRING that splits to nothing, so emptiness is checked on the split + count. An empty allow-list is not a harmless default at either end: the gate's `for rv in ""` + iterates zero times, and here every account would be judged not-a-member.""" + for body in (' H10_REVIEWERS=""\n', ' H10_REVIEWERS=" "\n'): + result = _lib('etv_h10_reviewers_load "$PWD/wf.yml"', tmp_path, body) + assert result.returncode != 0, f"accepted an empty allow-list from {body!r}" + assert "EMPTY" in result.stderr + + +def test_membership_asked_before_a_successful_load_answers_CANNOT_TELL_not_NOT_A_MEMBER(tmp_path): + """Three outcomes, not two. Folding "cannot tell" into "not a member" would make a broken + checkout indistinguishable from a wrong credential, and the script would then tell the operator + to fix their credential — a cause that did not happen. Exit 2 is what keeps them apart.""" + result = _lib('set +e; etv_h10_reviewers_contains timothy; echo "RC=$?"', tmp_path) + assert "RC=2" in result.stdout, f"expected cannot-tell (2), got {result.stdout.strip()!r}" + + # And after a FAILED load, not merely before any load at all. + result = _lib( + 'set +e; etv_h10_reviewers_load "$PWD/nonexistent.yml"; etv_h10_reviewers_contains timothy; echo "RC=$?"', + tmp_path, + ) + assert "RC=2" in result.stdout, f"a failed load must not leave the list queryable: {result.stdout!r}" + + +def test_a_successful_load_answers_member_and_non_member_apart(tmp_path): + """The positive control for the three-outcome contract above: 0 and 1 must both be reachable, + or the exit-2 assertion could be passing because the function only ever returns 2.""" + result = _lib( + 'etv_h10_reviewers_load "$PWD/wf.yml"; set +e; ' + 'etv_h10_reviewers_contains alice; echo "ALICE=$?"; ' + 'etv_h10_reviewers_contains carol; echo "CAROL=$?"', + tmp_path, + ' H10_REVIEWERS="alice bob"\n', + ) + assert "ALICE=0" in result.stdout, result.stdout + assert "CAROL=1" in result.stdout, result.stdout + + +def test_sourcing_the_library_does_not_disturb_the_callers_globbing_setting(tmp_path): + """The membership split turns pathname expansion off and must put it back exactly as found — + `post-review-verdict.sh` runs under `set -euo pipefail` with globbing ON and interpolates API + values into messages afterwards.""" + (tmp_path / "wf.yml").write_text(' H10_REVIEWERS="alice"\n') + on = _lib( + 'etv_h10_reviewers_load "$PWD/wf.yml"; set +e; etv_h10_reviewers_contains alice; ' + 'case "$-" in *f*) echo "GLOB=off" ;; *) echo "GLOB=on" ;; esac', + tmp_path, + ) + assert "GLOB=on" in on.stdout, on.stdout + off = _lib( + 'set -f; etv_h10_reviewers_load "$PWD/wf.yml"; set +e; etv_h10_reviewers_contains alice; ' + 'case "$-" in *f*) echo "GLOB=off" ;; *) echo "GLOB=on" ;; esac', + tmp_path, + ) + assert "GLOB=off" in off.stdout, off.stdout -- 2.47.3 From e3a19227c144cc7a7d1c00fafbc2f5eec7210d3e Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 29 Aug 2026 23:48:15 +0200 Subject: [PATCH 07/17] =?UTF-8?q?fix(845):=20two=20independent=20cold=20re?= =?UTF-8?q?views=20=E2=80=94=20the=20guard=20did=20not=20walk=20the=20same?= =?UTF-8?q?=20tree,=20and=20CLAUDE.md=20said=20the=20status=20is=20not=20w?= =?UTF-8?q?ritten?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two cold reviewers, independently briefed and worktree-isolated. One returned BLOCKED on the first two below; the other returned MERGEABLE having not examined them. No arbitration was needed — both are verifiable, and I reproduced each before acting. BLOCKING 1 — the CI guard parsed a different population than the library. `test_the_H10_REVIEWERS_list_is_glob_free_and_non_empty` read `_classify_step()["run"]`, one step's body. `scripts/lib/h10-reviewers.sh` greps the WHOLE YAML and refuses on anything but exactly one match. So a second anchored `H10_REVIEWERS=` in ANY other `run:` block was invisible to the guard while making the writer refuse every verdict — and `main` is PR-only with `review-verdict/h10` required, so that is a repo-wide merge outage. Reproduced: injecting one into the last step left the guard green and reddened 31 writer tests whose diagnostics said nothing about a second assignment. The guard now reads the whole file, so it walks the same tree as its subject; the header and the decision record no longer claim coverage that did not exist. BLOCKING 2 — `CLAUDE.md` said an off-list verdict "fails loudly at your terminal instead of being written". The status IS written; only the comment is withheld. The script, `ci-cd.md` and the record all said so correctly, and the most-read doc contradicted them. It now also says what the other docs did not: the green left standing satisfies the server-side required check, because branch protection binds the context NAME and not its issuer. Four clauses were shipped UNWITNESSED, found by the reviewer's own disarm matrix and each now reddening a named test: the `.statuses` array-TYPE test, the read-back GET's explicit `|| die`, `?limit=100`, and the `set -f` restoration. The first two passed only because a downstream check produced the same non-zero exit — the shape this very file already documents at the `-z`/`-n` arms. They now assert their own diagnostic. `?limit=100` was unobservable because the shim discarded the query string; the shim now models the measured server (default 30, clamped to 50, `total_count` reporting the page). Also fixed, all reproduced first: - `[0]` over same-context rows had no uniqueness guard. Unreachable today, but the alternative is judging one row's author while another is the one standing. - The row count in the not-found refusal came out BLANK on a 200 with an empty body: jq exits 0 with empty output, so the `||` fallback was dead exactly where the count matters. Tests the value now, not jq's exit. - The split pinned globbing but not IFS. A caller with `IFS=-` made `renovate` read as a member of `renovate-bot`. Unreachable from this script (bash resets IFS), pinned because the header invites reuse. - The `set -u` rationale was inherited and is WRONG. Both reviewers measured it: the shell EXITS, in both the `if ! f` and the `f; rc=$?` shapes, on bash 5.3 and 3.2. Nothing here depended on it; the sentinel now rests on the two grounds that were measured. - The `?limit=100` margin was overstated. This instance clamps to MAX_RESPONSE_ITEMS = 50, so the headroom is ~3x over today's 8-15 contexts, not 6x, and it shrinks as checks are added. - The snapshot boundary now names its DIRECTION in all three places: it is a false ACCEPT, and the likely edit (ADDING a reviewer) is exactly the one it mis-handles. - The overwrite refusal told the operator to re-review when a gate re-derive needs no review. A note on the measurements, since two of my own disarms were inert and briefly read as "no test covers this": a mutation must be checked to actually mutate. The glob-class disarm still excluded `*`, and the IFS disarm matched two sites and applied to neither. fixes #845 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW --- .gitea/workflows/pr-checks.yml | 13 +- CLAUDE.md | 2 +- docs/ci-cd.md | 26 +++- .../records/ci/exemption-provenance.md | 16 ++- scripts/lib/h10-reviewers.sh | 66 +++++++-- scripts/post-review-verdict.sh | 47 +++++-- scripts/tests/test_post_review_verdict.py | 125 +++++++++++++++++- scripts/tests/test_pr_changed_files.py | 12 +- 8 files changed, 267 insertions(+), 40 deletions(-) diff --git a/.gitea/workflows/pr-checks.yml b/.gitea/workflows/pr-checks.yml index 9c2626dee..f53996261 100644 --- a/.gitea/workflows/pr-checks.yml +++ b/.gitea/workflows/pr-checks.yml @@ -301,11 +301,14 @@ jobs: # close. A distinct job name keeps a real failure unambiguous. # # Runs UNCONDITIONALLY on every PR rather than behind a `scripts/**` path filter. The suite's - # corpus tests are fixture/tmp-repo based, but test_post_review_verdict.py and - # test_merge_consent_exemption.py execute the REAL `scripts/post-review-verdict.sh` and - # `.claude/hooks/pretooluse-merge-consent.sh`, so its true input set spans at least two top-level - # directories. A `scripts/**` filter would silently miss a `.claude/hooks/**` edit — and at ~10s a - # filter buys nothing but drift. + # corpus tests are fixture/tmp-repo based, but several execute REAL artifacts from other top-level + # directories: test_post_review_verdict.py runs `scripts/post-review-verdict.sh`, + # test_merge_consent_exemption.py runs `.claude/hooks/pretooluse-merge-consent.sh`, and since + # ersatztv#845 test_post_review_verdict.py ALSO reads `.gitea/workflows/review-verdict.yml` — + # the writer derives the H10 allow-list from it, so editing that literal changes the suite's + # outcome. Its true input set therefore spans at least three top-level directories, and this + # enumeration is the kind that goes stale: a `scripts/**` filter would silently miss a + # `.claude/hooks/**` or `.gitea/workflows/**` edit. At ~10s a filter buys nothing but drift. prove-fix: name: "Fix proofs (Proves trailers)" runs-on: small diff --git a/CLAUDE.md b/CLAUDE.md index 866b68140..6308ab8c6 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. **The credential you post with must be an account on `H10_REVIEWERS` in `.gitea/workflows/review-verdict.yml`** (`timothy` today) — since ersatztv#742 the gate inherits an existing `success` only from an allow-listed creator (an existing `failure` is left alone on a weaker attributability test, so an attributable rejection VISIBLE AT THE FIRST READ is not re-derived into a green — a rejection landing later, inside a run's own write window, is a separate and still-open route, ersatztv#849), and since ersatztv#845 the script ENFORCES that coupling rather than assuming it: it reads its own status back and refuses, before writing the verdict comment, unless the recorded `.creator.login` is on that allow-list — so a verdict posted with any other account fails loudly at your terminal instead of being written, reported as success, and then silently re-derived on the next PR event. The allow-list is derived from the workflow by `scripts/lib/h10-reviewers.sh`; it is never restated. +- `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 credential you post with must be an account on `H10_REVIEWERS` in `.gitea/workflows/review-verdict.yml`** (`timothy` today) — since ersatztv#742 the gate inherits an existing `success` only from an allow-listed creator (an existing `failure` is left alone on a weaker attributability test, so an attributable rejection VISIBLE AT THE FIRST READ is not re-derived into a green — a rejection landing later, inside a run's own write window, is a separate and still-open route, ersatztv#849), and since ersatztv#845 the script ENFORCES that coupling rather than assuming it: it reads its own status back and refuses, before writing the verdict comment, unless the recorded `.creator.login` is on that allow-list — so a verdict posted with any other account fails loudly at your terminal instead of being reported as success and silently re-derived on the next PR event. **The status is still written** — the check runs after the POST, because it measures the creator Gitea recorded rather than what the credential claims — and what is withheld is the verdict COMMENT, which leaves the merge hook at condition (c) with nothing to classify, i.e. an `ask`. So a refusal leaves a green `review-verdict/h10` standing on that head that the gate itself will not inherit; branch protection binds the context NAME and not its issuer, so do not read that green as consent. The allow-list is derived from the workflow by `scripts/lib/h10-reviewers.sh`; it is never restated. - **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 f9429a157..e650aec02 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -1625,12 +1625,26 @@ always a stall, never a fail-open. Before, a changed writer credential reported *subsequent* PR event re-derived the status — to `pending` on a PR that needs a verdict, or to an exemption **`success`** on a docs-only or Renovate-manifest one, which is the quieter and worse half — so the symptom was a PR that kept losing its verdict rather than an immediate refusal. Now it -is an immediate refusal naming the account and the allow-list. **The residual, stated:** the check -runs after the POST, so a refused verdict leaves a non-inheritable status standing on the head with -no comment beside it. That is the `ask` half-state `release.verdict-writes-status-before-comment` -already designates as the safe one, and the gate re-derives the status itself on the next PR event; -it is deliberately not repaired here, because a second corrective write is the sticky-sentinel -mechanism ersatztv#849 is separately designing. +is an immediate refusal naming the account and the allow-list. + +**The residual, stated precisely, because "the `ask` half-state" describes only half of it.** The +check runs after the POST, so a refused verdict leaves a `review-verdict/h10` **standing on the +head** with no comment beside it. On the *hook* path that is the safe half-state +`release.verdict-writes-status-before-comment` designates — nothing for condition (c) to classify, +so it asks rather than granting. On the *server* path it is weaker: branch protection binds the +context NAME and not its issuer, so a `success` posted by any account satisfies the required check +until the gate re-derives it on the next PR event. **A refusal from this tool is not a reason to +trust the green it leaves behind.** That is not a regression — it was equally true before #845, when +the tool reported success and said nothing — and it is deliberately not repaired here, because a +second corrective write is the sticky-sentinel mechanism ersatztv#849 is separately designing. + +**One false ACCEPT, named because the direction matters.** The writer reads the workflow in the local +checkout while the gate runs the base-resolved one, so on a branch that ADDS a reviewer the writer +accepts an account the live gate will not yet inherit from — the #845 stall, reached through the +check meant to catch it, on the one branch where someone is editing the allow-list. Accepted: +closing it means a live remote read on the merge-consent write path, whose own read failure would +have to resolve either to refusing every verdict or to passing. The window is one PR long and the +residual is a stall, never an unreviewed merge. This remains a *provenance* check, not an authentication one — an **allow-listed** collaborator's own write-scoped token can still POST a verdict they did not form, branch protection binds the context diff --git a/docs/decisions/records/ci/exemption-provenance.md b/docs/decisions/records/ci/exemption-provenance.md index 516f65bc1..65dca07bd 100644 --- a/docs/decisions/records/ci/exemption-provenance.md +++ b/docs/decisions/records/ci/exemption-provenance.md @@ -240,7 +240,21 @@ and blocks every merge including the repair (`block_admin_merge_override: true`) that case would reintroduce the second copy. So the derivation runs on the side that can fail safely and the body of this workflow is untouched. The parse admits exactly one anchored assignment or refuses; `test_pr_changed_files.py::test_the_H10_REVIEWERS_list_is_glob_free_and_non_empty` reads the -same literal with the same anchors, so a shape change reddens CI on the PR that makes it. +same literal with the same anchors OVER THE SAME POPULATION — the whole file — so a shape change +reddens CI on the PR that makes it. The population is part of the rule, not an implementation +detail: that guard used to scope itself to the classify step's `run:` body, and a second anchored +assignment in any other `run:` block then left it green while making the writer refuse every +verdict, which is a repo-wide merge outage on a PR-only `main`. Measured before it was aligned: +green guard, 31 red writer tests whose diagnostics named no second assignment. + +**The one false ACCEPT this leaves, named with its direction.** The writer reads the workflow in the +LOCAL checkout; the gate runs the base-resolved one. On a branch that ADDS a reviewer — the likely +edit — the writer accepts that account while the live gate will not yet inherit from it, which is +the #845 stall reached through the check meant to catch it. Accepted rather than closed: a live +remote read of the base workflow puts a network dependency on the merge-consent write path, where a +read failure must resolve either to a refusal (a new way to block every verdict) or to a pass (the +hole, reopened). The window is one PR long, the person making the edit is the one who knows the list +is in flux, and the residual is a STALL, never a merge of unreviewed code. The allow-list was `timothy` and the instance held exactly `renovate` and `timothy` when this was measured on 2026-08-26 (`GET /api/v1/user`, `GET /api/v1/admin/users?limit=50`). That measurement is diff --git a/scripts/lib/h10-reviewers.sh b/scripts/lib/h10-reviewers.sh index 9b7a9dec5..c1ac29a4e 100644 --- a/scripts/lib/h10-reviewers.sh +++ b/scripts/lib/h10-reviewers.sh @@ -33,15 +33,30 @@ # shell parser and does not try to be: `testing.guard-derives-population-from-source` records why a # regex over shell source does not converge when the target is an open-ended construct. The target # here is closed — `test_pr_changed_files.py::test_the_H10_REVIEWERS_list_is_glob_free_and_non_empty` -# reads the same literal with the same anchors and already fails the suite if the workflow stops -# declaring it as a single double-quoted assignment, so a shape change reddens CI on the PR that -# makes it rather than going stale here. +# reads the same literal, over the SAME POPULATION (the whole file), with the same anchors, and +# already fails the suite if the workflow stops declaring it as exactly one double-quoted +# assignment. So a shape change reddens CI on the PR that makes it rather than going stale here. # -# SNAPSHOT BOUNDARY, stated because it is a real limit and not a defect: this reads the workflow in -# the LOCAL checkout, while the gate that actually runs is the one at `origin/main` (base-resolved). -# On a branch that edits `H10_REVIEWERS` the two differ until it merges. That is the correct -# approximation — the alternative is a live remote read on the merge-consent write path — and CI -# holds them consistent, because `.gitea/` cannot ride a docs-only exemption. +# That guard used to scope itself to the classify step's `run:` body, which is NOT the same tree +# this file walks: a second anchored assignment in any other `run:` block left it green while making +# this refuse every verdict — a repo-wide merge outage, `main` being PR-only with +# `review-verdict/h10` required. Measured before the fix: green guard, 31 red writer tests whose +# diagnostics said nothing about a second assignment. The populations are now the same one, which is +# the property to preserve if either side is ever rescoped. +# +# SNAPSHOT BOUNDARY, and its DIRECTION, because the direction is the part that matters. This reads +# the workflow in the LOCAL checkout, while the gate that actually runs is the one at `origin/main` +# (base-resolved). On a branch that edits `H10_REVIEWERS` the two differ until it merges, and the +# likely edit is ADDING a reviewer — for which this check ACCEPTS an account the live gate will not +# yet inherit from. That is a false accept: the #845 stall, reached through the very check meant to +# catch it, on the one branch where someone is thinking about the allow-list. +# +# It is accepted rather than closed, with the reasons stated. A live remote read of the base +# workflow would close it and puts a network dependency on the merge-consent write path, where a +# read failure has to resolve either to a refusal (a new way to block every verdict) or to a pass +# (the hole, reopened). The window is one PR long, the operator making the edit is the one person +# who knows the list is in flux, and the residual is a STALL, never a merge of unreviewed code. The +# direction is what is written down; a future reader deciding whether to close it should start here. # # NO ENVIRONMENT VARIABLE SELECTS THE SOURCE FILE. The path is derived from this file own location, # and a caller may pass one explicitly; there is deliberately no `ETV_*_WORKFLOW` override, because @@ -58,12 +73,19 @@ fi unset _etv_h10_root # --- The validation sentinel. -------------------------------------------------------------------- -# Reset to 0 HERE, at load time, for the two reasons `review-verdict-vocabulary.sh` sets out at -# length: it defeats an inherited `ETV_H10_REVIEWERS_OK=1` from the environment, and it makes -# "derived successfully" a DATA dependency the query function can require rather than a control-flow -# gate a caller is trusted to have taken. 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. A data dependency cannot be skipped that way. +# Reset to 0 HERE, at load time, for two reasons. It defeats an inherited `ETV_H10_REVIEWERS_OK=1` +# from the environment — measured: exporting `ETV_H10_REVIEWERS_OK=1` with a hostile +# `ETV_H10_REVIEWERS` does not get a verdict past the writer. And it makes "derived successfully" a +# DATA dependency the query function requires, rather than a control-flow gate a caller is trusted +# to have taken. +# +# WHAT THIS DOES NOT REST ON, stated because the claim was inherited and is wrong. The vocabulary +# library argues that under `set -u` an unbound-variable error inside a function "aborts the function +# but not the script", so an `if ! f` gate takes NEITHER branch. Measured here (bash 5.3 and bash +# 3.2, `set -euo pipefail`): the shell EXITS outright, in both the `if ! f` shape and the +# `f; rc=$?` shape. So that particular skip-past-the-gate route does not exist, and no reasoning +# below depends on it. The sentinel earns its place on the environment-defeating and data-dependency +# grounds above, which are the two that were measured. ETV_H10_REVIEWERS_OK=0 ETV_H10_REVIEWERS='' @@ -79,6 +101,18 @@ ETV_H10_REVIEWERS='' # those matching the plain-login class below would validate cleanly and enter the allow-list as a # real reviewer. That is the defect `review-verdict-vocabulary.sh` records against its own word list, # reached one file over through the allow-list. +# +# WHICH SITE CARRIES THAT RISK: the one in `etv_h10_reviewers_load`, which splits a value NOTHING has +# validated yet. By the time `etv_h10_reviewers_contains` runs, the plain-login class has already +# excluded every metacharacter, so its `set -f` is defence in depth against a future edit that +# reorders or weakens the validation — not the primary guard. Said plainly because "this is the +# fail-open" attached to both sites would misdirect the next reader about which line to keep. +# +# `IFS` IS PINNED TOO. Globbing is only half of what an unquoted expansion depends on; the other +# half is the ambient `IFS`, and a caller that had set `IFS=-` turned `renovate-bot` into two +# entries, making `renovate` read as a member. Not reachable from `post-review-verdict.sh` today +# (bash resets `IFS` at startup, so the environment route is closed, and the script never assigns +# it), but this file invites reuse and the cost is one word per function. # --- The derivation. ----------------------------------------------------------------------------- # Sets `ETV_H10_REVIEWERS` and the sentinel. Returns 0 when the allow-list is usable, 1 otherwise @@ -88,6 +122,8 @@ ETV_H10_REVIEWERS='' etv_h10_reviewers_load() { local src="${1:-${ETV_H10_REVIEWERS_WORKFLOW:-}}" local count value entry n=0 glob_was_off=0 + # Pin the split: the caller's IFS is not this function's business to inherit. + local IFS=$' \t\n' ETV_H10_REVIEWERS_OK=0 ETV_H10_REVIEWERS='' @@ -157,6 +193,8 @@ etv_h10_reviewers_load() { # wrong to guess at, so the third code makes the caller say which it meant. etv_h10_reviewers_contains() { local login="${1:-}" entry glob_was_off=0 + # Pin the split: the caller's IFS is not this function's business to inherit. + local IFS=$' \t\n' if [ "${ETV_H10_REVIEWERS_OK:-0}" != 1 ]; then printf 'h10 reviewers: the allow-list was never derived (call etv_h10_reviewers_load first)\n' >&2 diff --git a/scripts/post-review-verdict.sh b/scripts/post-review-verdict.sh index 36285849a..d33355083 100755 --- a/scripts/post-review-verdict.sh +++ b/scripts/post-review-verdict.sh @@ -273,25 +273,54 @@ printf 'posted status: %s = %s on %s\n' "$STATUS_CONTEXT" "$state" "$short" # from the response: measured on Gitea 1.27.1 (2026-08-29), `?limit=2` on a head carrying 15 contexts # returned 2 rows AND `total_count: 2` — the count reports the PAGE, not the total, so a truncated # body is indistinguishable from a complete one by any field in it. A head whose context count grows -# past the server default page size would silently stop containing our row, and the refusal below -# would fire on a verdict that is in force. 100 is far above the 15 contexts this repo produces, and -# the diagnostic states the row count it actually read so a future overflow is legible rather than -# mysterious. +# past the page size would silently stop containing our row, and the refusal below would fire on a +# verdict that is in force. +# +# BE EXACT ABOUT THE HEADROOM, because the obvious reading of `100` is wrong: this instance clamps +# `limit` to `MAX_RESPONSE_ITEMS`, measured at 50 (the same cap `pr-changed-files.sh` and the gate +# page against). So the real ceiling is 50 rows, not 100 — against the 8-15 status contexts a head +# here carries. Asking for 100 costs nothing and takes whatever the server will give; the margin is +# roughly 3x, not 6x, and it SHRINKS as required checks are added. The diagnostic below states the +# row count it actually read, so an overflow is legible rather than mysterious. status_readback=$(api_get "repos/$owner/$repo/commits/$sha/status?limit=100") \ || die "the '$STATUS_CONTEXT' status was posted on $short, but it could not be read back to confirm the gate will honour it — no comment was written. Re-run once Gitea is reachable." # `.statuses` serialises as `null` rather than `[]` on a head with no statuses (ersatztv#751), so # the array TYPE is tested rather than assumed; a body that merely lost its array yields no entry # and is refused below, exactly like one that carries no verdict. -readback_entry=$(printf '%s' "$status_readback" | jq -c --arg c "$STATUS_CONTEXT" \ - 'if (.statuses | type) == "array" then ((.statuses | map(select(.context == $c)))[0] // null) else null end') \ +# Three outcomes are kept apart below, each with its own message, because they call for different +# actions and an earlier draft collapsed them into one: a body whose `.statuses` is not an array at +# all, a well-formed body that does not carry our context, and a body carrying MORE THAN ONE row for +# it. Collapsing them also made the array-TYPE test unwitnessable — the absent-entry check caught +# every case on its own, so disarming the type test left the suite green. +readback_matches=$(printf '%s' "$status_readback" | jq -c --arg c "$STATUS_CONTEXT" \ + 'if (.statuses | type) == "array" then (.statuses | map(select(.context == $c))) else null end') \ || die "the '$STATUS_CONTEXT' status was posted on $short, but the read-back body could not be parsed — no comment was written." -if [ -z "$readback_entry" ] || [ "$readback_entry" = "null" ]; then +if [ -z "$readback_matches" ] || [ "$readback_matches" = "null" ]; then + die "the '$STATUS_CONTEXT' status was posted on $short, but the read-back body carries no \`.statuses\` ARRAY (it is missing, null, or some other type), so nothing about the verdict can be confirmed — no comment was written. This is a response-SHAPE problem; check what the combined-status endpoint returns on this Gitea version." +fi +readback_count=$(printf '%s' "$readback_matches" | jq -r 'length') \ + || die "the '$STATUS_CONTEXT' status was posted on $short, but the matching rows could not be counted — no comment was written." +if [ "$readback_count" = 0 ]; then # Deliberately does NOT assert why. Two causes reach here and the response cannot tell them apart: # the status genuinely is not there, or the row list was truncated by paging. Naming one would be # stating a cause that may not have happened (#859). - readback_rows=$(printf '%s' "$status_readback" | jq -r 'if (.statuses | type) == "array" then (.statuses | length) else "unreadable" end' 2>/dev/null) || readback_rows='unreadable' + # + # `[ -n ]` on the VALUE, not `||` on jq's exit. jq exits 0 with EMPTY OUTPUT on empty input, so an + # exit-status fallback is dead exactly where it is needed — a 200 with an empty body printed "not + # among the status rows", with the count this branch exists to report left blank. + readback_rows=$(printf '%s' "$status_readback" | jq -r '.statuses | length' 2>/dev/null) || readback_rows='' + [ -n "$readback_rows" ] || readback_rows='an unreadable number of' die "the '$STATUS_CONTEXT' status was posted on $short but is not among the $readback_rows status rows read back, so it is UNPROVEN that the verdict landed — no comment was written. Either it was removed, or this head now carries more status contexts than one page returns. Check the PR's checks before re-running." fi +if [ "$readback_count" != 1 ]; then + # Not reachable against Gitea as measured (the combined endpoint returns the latest row PER + # context), so this is a guard against that property silently ceasing to hold rather than against + # an observed shape. It is here because the alternative is `[0]`, which would pick a row and + # judge its author while a DIFFERENT row for the same context is the one standing. + die "the '$STATUS_CONTEXT' status on $short reads back as $readback_count separate rows, so which one is in force is ambiguous — no comment was written. The combined-status endpoint is expected to return one row per context; if that has changed, this check needs rewriting before verdicts can be trusted." +fi +readback_entry=$(printf '%s' "$readback_matches" | jq -c '.[0]') \ + || die "the '$STATUS_CONTEXT' status was posted on $short, but its row could not be read — no comment was written." # `.status`, NOT `.state`. A row inside `.statuses[]` serialises its state under the key `status`; # `state` exists only as the AGGREGATE at the top level of the combined body. Measured on Gitea # 1.27.1, 2026-08-29, against a real `review-verdict/h10` row. Reading `.state` here yields the empty @@ -324,7 +353,7 @@ fi # author judged below, and an Actions write carries `creator: null` — which would surface as a # wrong-credential accusation against a credential that is fine. if [ "$readback_state" != "$state" ] || [ "$readback_desc" != "$status_desc" ]; then - die "the '$STATUS_CONTEXT' status standing on $short is not the one just written (state '$readback_state', description '$readback_desc') — something overwrote it, so this verdict is not in force. No comment was written. Re-review the current head and run this again." + die "the '$STATUS_CONTEXT' status standing on $short is not the one just written (state '$readback_state', description '$readback_desc') — it was overwritten, so this verdict is not in force. No comment was written. If the head is unchanged the gate simply re-derived the status and re-running this is enough; re-review only if the head moved." fi if [ -z "$readback_creator" ]; then # States the fact first and the likely cause second. `creator` absent, null, or carrying an empty diff --git a/scripts/tests/test_post_review_verdict.py b/scripts/tests/test_post_review_verdict.py index da6436335..f6834341a 100644 --- a/scripts/tests/test_post_review_verdict.py +++ b/scripts/tests/test_post_review_verdict.py @@ -41,7 +41,7 @@ SHA_B = "52786a542222222222222222222222222222bbbb" CURL_SHIM = r'''#!/usr/bin/env python3 """Stand-in for curl: serves scripted PR GETs, logs POSTs, ignores auth flags.""" -import json, os, sys, pathlib +import json, os, sys, pathlib, urllib.parse state = pathlib.Path(os.environ["STUB_DIR"]) args = sys.argv[1:] @@ -122,7 +122,21 @@ if "/commits/" in url and url.split("?")[0].endswith("/status"): # script's parser by construction and test nothing — the shape below is what Gitea 1.27.1 # actually returns, measured 2026-08-29. entry["status"] = entry.pop("state") - print(json.dumps({"state": entry["status"], "total_count": 1, "statuses": [entry]})) + # PAGING, modelled from the measured server rather than assumed away. Gitea applies + # `DEFAULT_PAGING_NUM` (30) when no `limit` is given and clamps to `MAX_RESPONSE_ITEMS` (50), + # and `total_count` reports the PAGE rather than the total. Without this the `?limit=100` the + # script sends is invisible to every test — the shim used to discard the query string, so + # deleting the parameter reddened nothing. + filler = int((state / "status_filler").read_text()) if (state / "status_filler").exists() else 0 + rows = [ + {"context": "ci/filler-%d" % i, "status": "success", "description": "f", "creator": None} + for i in range(filler) + ] + rows.append(entry) + query = urllib.parse.parse_qs(urllib.parse.urlparse(url).query) + limit = min(int(query.get("limit", ["30"])[0]), 50) + rows = rows[:limit] + print(json.dumps({"state": entry["status"], "total_count": len(rows), "statuses": rows})) sys.exit(0) print("{}") @@ -180,6 +194,10 @@ def gitea(tmp_path): """Make the combined-status GET fail, as curl -f does on a 4xx/5xx.""" (state / "status_fail").write_text("1") + def set_status_filler(self, n): + """Prepend N unrelated status contexts, so the read-back has to page past them.""" + (state / "status_filler").write_text(str(n)) + def set_status_body(self, body): """Replace the whole combined-status response with this literal JSON text.""" (state / "status_body").write_text(body) @@ -674,7 +692,7 @@ def test_a_status_OVERWRITTEN_between_the_write_and_the_read_is_not_blamed_on_it result = gitea.run("42", "MERGEABLE") assert result.returncode != 0 - assert "overwrote it" in result.stderr + assert "it was overwritten" in result.stderr assert "ACTIONS token" not in result.stderr, ( "an overwritten status must not be reported as a wrong-credential problem" ) @@ -688,6 +706,9 @@ def test_an_unreadable_readback_refuses_rather_than_reporting_the_verdict_posted result = gitea.run("42", "MERGEABLE") assert result.returncode != 0 + # Same reason as above: `set -e` plus the absent-entry check already refuse, so without naming + # the message this test passed with the explicit `|| die` removed. + assert "could not be read back" in result.stderr assert gitea.comments() == [] @@ -699,6 +720,10 @@ def test_a_readback_whose_statuses_array_is_NULL_is_refused(gitea): result = gitea.run("42", "MERGEABLE") assert result.returncode != 0 + # The DIAGNOSTIC, not merely a non-zero exit. Asserting only the exit code let this pass on the + # downstream absent-entry check, so the array-TYPE clause it names could be disarmed with the + # suite still green — an outcome the script produces anyway is not evidence about a clause. + assert "carries no `.statuses` ARRAY" in result.stderr assert gitea.comments() == [] @@ -984,3 +1009,97 @@ def test_sourcing_the_library_does_not_disturb_the_callers_globbing_setting(tmp_ tmp_path, ) assert "GLOB=off" in off.stdout, off.stdout + + +def test_the_readback_asks_for_a_page_that_reaches_past_the_other_contexts(gitea): + """Witnesses `?limit=100`, which was previously unobservable. + + Gitea applies `DEFAULT_PAGING_NUM` (30) when no `limit` is given and clamps to + `MAX_RESPONSE_ITEMS` (50) — so a head carrying more status contexts than one default page holds + would stop containing the verdict row, and the read-back would refuse a verdict that IS in + force. That is the deadlock direction on the one path that must not deadlock. With 40 unrelated + contexts ahead of it, the row is reachable only because the request asks for more than 30. + """ + gitea.set_status_filler(40) + result = gitea.run("42", "MERGEABLE") + + assert result.returncode == 0, ( + f"the verdict writer REFUSED a verdict that is in force, behind 40 other contexts — the " + f"read-back is not asking for a big enough page: {result.stderr}" + ) + assert len(gitea.comments()) == 1 + + +def test_a_head_carrying_more_contexts_than_the_server_cap_refuses_and_says_the_row_count(gitea): + """The residual, pinned rather than described. 50 is the server's own ceiling, so past it the + row genuinely cannot be fetched in one page and the tool refuses — reporting how many rows it + read, so the cause is legible instead of looking like a vanished status.""" + gitea.set_status_filler(60) + result = gitea.run("42", "MERGEABLE") + + assert result.returncode != 0 + assert "not among the 50 status rows" in result.stderr, result.stderr + assert gitea.comments() == [] + + +def test_TWO_rows_for_our_context_are_ambiguous_rather_than_resolved_by_taking_the_first(gitea): + """`[0]` would pick a row and judge ITS author while a different row for the same context is the + one actually standing. Unreachable against Gitea as measured (the combined endpoint returns the + latest row per context), so this guards the property rather than an observed shape — which is + why the message says so instead of blaming a race.""" + short = SHA_A[:7] + desc = f"Review-verdict: MERGEABLE @ {short} (base: main)" + gitea.set_status_body( + json.dumps( + { + "total_count": 2, + "statuses": [ + { + "context": "review-verdict/h10", + "status": "success", + "description": desc, + "creator": {"login": ALLOWLISTED}, + }, + { + "context": "review-verdict/h10", + "status": "success", + "description": desc, + "creator": {"login": OFF_LIST}, + }, + ], + } + ) + ) + result = gitea.run("42", "MERGEABLE") + + assert result.returncode != 0, "an allow-listed FIRST row let a second row with an off-list creator through" + assert "2 separate rows" in result.stderr + assert gitea.comments() == [] + + +def test_an_EMPTY_body_still_reports_a_row_count_rather_than_a_blank(gitea): + """jq exits 0 with EMPTY OUTPUT on empty input, so an exit-status fallback for the row count is + dead exactly where it is needed: the message read "not among the status rows". The count exists + to make a paging overflow legible, so the one shape where it goes blank is the one to pin.""" + gitea.set_status_body("") + result = gitea.run("42", "MERGEABLE") + + assert result.returncode != 0 + assert "among the status" not in result.stderr, f"the row count came out blank: {result.stderr}" + assert gitea.comments() == [] + + +def test_the_split_pins_IFS_so_a_callers_separator_cannot_merge_or_split_entries(tmp_path): + """Globbing is only half of what an unquoted expansion depends on; the other half is the ambient + `IFS`. With `IFS=-` a caller turns `renovate-bot` into two entries and `renovate` reads as a + member. Not reachable from the script today (bash resets IFS at startup and the script never + assigns it), so this pins the hardening the library's own header invites by inviting reuse.""" + (tmp_path / "wf.yml").write_text(' H10_REVIEWERS="renovate-bot"\n') + result = _lib( + 'IFS="-"; etv_h10_reviewers_load "$PWD/wf.yml"; set +e; ' + 'etv_h10_reviewers_contains renovate; echo "SPLIT=$?"; ' + 'etv_h10_reviewers_contains renovate-bot; echo "WHOLE=$?"', + tmp_path, + ) + assert "SPLIT=1" in result.stdout, f"`renovate` read as a member under IFS=-: {result.stdout!r}" + assert "WHOLE=0" in result.stdout, result.stdout diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index 38bc809d5..00534e685 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -2382,7 +2382,17 @@ def test_an_UNATTRIBUTABLE_failure_is_still_RE_DERIVED(tmp_path, shape, creator, def _h10_reviewers_literal(): """The allow-list as the workflow actually declares it — read from the shipped file, never restated here, so a test cannot pass against a value the gate does not use.""" - body = _classify_step()["run"] + # THE WHOLE FILE, not `_classify_step()["run"]` — and the difference is load-bearing, not + # tidiness. `scripts/lib/h10-reviewers.sh` derives the same literal for the WRITE side by + # `grep -c` over the whole YAML, and refuses unless it finds exactly one. A guard scoped to the + # classify step therefore does not walk the same tree as its subject: a second anchored + # assignment in ANY OTHER `run:` block is invisible here while making the writer refuse every + # verdict, which — `main` being PR-only with `review-verdict/h10` required — is a repo-wide merge + # outage. Measured on this file: injecting one into the last step left this test green and + # reddened 31 writer tests, whose diagnostics say nothing about a second assignment. + # See `a-check-must-walk-the-same-tree`; the classify step remains what the GATE runs, and + # `_classify_step()` is still the right scope for every other assertion about that body. + body = WORKFLOW.read_text() # findall + a SINGLE-ASSIGNMENT assertion, not `search`. `search` returns the FIRST assignment # while the shell runs the LAST one executed, so a later or conditional reassignment would leave # this guard validating a value the gate does not use — the same extractor-vs-classifier gap -- 2.47.3 From 04a375243270c88447874c883f6f4ef9516b0f36 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 30 Aug 2026 00:02:06 +0200 Subject: [PATCH 08/17] test(845): the two identify-our-own-write arms masked each other; grammar in the library header MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Caught while a cross-family review was probing the same spot, and confirmed before fixing: `test_a_status_OVERWRITTEN_...` used a fixture differing in BOTH state and description, so either arm of `[ state != ] || [ desc != ]` could be deleted with the suite still green. Each arm was individually disarmed and the suite stayed at 69 passed both times — #685's duplicate-guards-mask-each-other shape. Each arm now has a fixture differing in exactly one field, and both were confirmed to redden their own named test. The creator in each is deliberately ALLOW-LISTED, so the refusal has to come from identifying the row as not-ours rather than from its author. The script now also states WHICH arm carries the safety, matching how this file already handles its overlapping `-z`/`-n` arms: the DESCRIPTION is the identifier (verdict word, short sha, base — only this run writes it), and the STATE is defence in depth, since our own writer cannot produce our description with a different state. Also: apostrophes restored in four library header sentences that read as typos. fixes #845 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW --- scripts/lib/h10-reviewers.sh | 14 +++---- scripts/post-review-verdict.sh | 6 +++ scripts/tests/test_post_review_verdict.py | 47 +++++++++++++++++++++++ 3 files changed, 60 insertions(+), 7 deletions(-) diff --git a/scripts/lib/h10-reviewers.sh b/scripts/lib/h10-reviewers.sh index c1ac29a4e..fce9a3026 100644 --- a/scripts/lib/h10-reviewers.sh +++ b/scripts/lib/h10-reviewers.sh @@ -58,13 +58,13 @@ # who knows the list is in flux, and the residual is a STALL, never a merge of unreviewed code. The # direction is what is written down; a future reader deciding whether to close it should start here. # -# NO ENVIRONMENT VARIABLE SELECTS THE SOURCE FILE. The path is derived from this file own location, -# and a caller may pass one explicitly; there is deliberately no `ETV_*_WORKFLOW` override, because -# an env var that chooses which tree answers a security question is an unsound input (ersatztv#858 -# is the same shape one file over). +# NO ENVIRONMENT VARIABLE SELECTS THE SOURCE FILE. The path is derived from this file's own +# location, and a caller may pass one explicitly; there is deliberately no `ETV_*_WORKFLOW` +# override, because an env var that chooses which tree answers a security question is an unsound +# input (ersatztv#858 is the same shape one file over). # --- The default source: the shipped gate definition. -------------------------------------------- -# Resolved relative to THIS FILE, never to the caller cwd — `post-review-verdict.sh` is run from +# Resolved relative to THIS FILE, never to the caller's cwd — `post-review-verdict.sh` is run from # whatever directory an operator happens to be in. ETV_H10_REVIEWERS_WORKFLOW='' if _etv_h10_root=$(CDPATH='' cd -- "$(dirname -- "${BASH_SOURCE[0]}")/../.." 2>/dev/null && pwd); then @@ -92,7 +92,7 @@ ETV_H10_REVIEWERS='' # --- The split idiom, stated once. --------------------------------------------------------------- # Both functions below turn the space-separated list into the positional parameters with PATHNAME # EXPANSION OFF, then restore `-f` to whatever the caller had. It is written out at each site rather -# than factored into a helper because a helper cannot set its caller positional parameters, and +# than factored into a helper because a helper cannot set its caller's positional parameters, and # returning the words through a string would reintroduce a split — of a value that may contain a # newline — at the other end. # @@ -151,7 +151,7 @@ etv_h10_reviewers_load() { value=$(sed -n 's/^[[:space:]]*H10_REVIEWERS="\([^"]*\)"[[:space:]]*$/\1/p' "$src") || value='' - # Validate every entry against the same character class the workflow own lint enforces. It is not + # Validate every entry against the same character class the workflow's own lint enforces. It is not # cosmetic on either side: the gate membership loop word-splits the list UNQUOTED, so `*`, `?` or # `[` there would be matched against the runner cwd and could silently change who the gate accepts. case "$-" in *f*) glob_was_off=1 ;; esac diff --git a/scripts/post-review-verdict.sh b/scripts/post-review-verdict.sh index d33355083..d14e4afcf 100755 --- a/scripts/post-review-verdict.sh +++ b/scripts/post-review-verdict.sh @@ -352,6 +352,12 @@ fi # So a run of the gate that overwrote ours between the POST and this read would otherwise have ITS # author judged below, and an Actions write carries `creator: null` — which would surface as a # wrong-credential accusation against a credential that is fine. +# WHICH ARM CARRIES THE SAFETY, stated because the two OVERLAP and a fixture that changes both +# leaves either one disarmable with the suite still green (#685's shape). The DESCRIPTION is the +# identifier: it carries the verdict word, the short sha and the base, so only this run writes it. +# The STATE is defence in depth — our own writer cannot produce our description with a different +# state, since the verdict word determines it. Both are pinned by their own single-field fixture +# below rather than by one fixture that differs in both. if [ "$readback_state" != "$state" ] || [ "$readback_desc" != "$status_desc" ]; then die "the '$STATUS_CONTEXT' status standing on $short is not the one just written (state '$readback_state', description '$readback_desc') — it was overwritten, so this verdict is not in force. No comment was written. If the head is unchanged the gate simply re-derived the status and re-running this is enough; re-review only if the head moved." fi diff --git a/scripts/tests/test_post_review_verdict.py b/scripts/tests/test_post_review_verdict.py index f6834341a..e0484db15 100644 --- a/scripts/tests/test_post_review_verdict.py +++ b/scripts/tests/test_post_review_verdict.py @@ -1103,3 +1103,50 @@ def test_the_split_pins_IFS_so_a_callers_separator_cannot_merge_or_split_entries ) assert "SPLIT=1" in result.stdout, f"`renovate` read as a member under IFS=-: {result.stdout!r}" assert "WHOLE=0" in result.stdout, result.stdout + + +def _standing_status(gitea, *, state, description): + """A read-back carrying one `review-verdict/h10` row with exactly these two fields.""" + gitea.set_status_body( + json.dumps( + { + "total_count": 1, + "statuses": [ + { + "context": "review-verdict/h10", + "status": state, + "description": description, + "creator": {"login": ALLOWLISTED}, + } + ], + } + ) + ) + + +def test_a_row_differing_ONLY_in_state_is_refused(gitea): + """Witnesses the STATE arm on its own. + + `test_a_status_OVERWRITTEN_...` changes state AND description, so either arm could be deleted + with the suite green — the two overlap and mask each other (#685). Each arm therefore gets a + fixture that differs in exactly one field. The creator here is ALLOW-LISTED on purpose: the + refusal must come from identifying the row as not-ours, not from its author. + """ + short = SHA_A[:7] + _standing_status(gitea, state="pending", description=f"Review-verdict: MERGEABLE @ {short} (base: main)") + result = gitea.run("42", "MERGEABLE") + + assert result.returncode != 0, "a row whose STATE is not the one just written was accepted" + assert "it was overwritten" in result.stderr + assert gitea.comments() == [] + + +def test_a_row_differing_ONLY_in_description_is_refused(gitea): + """Witnesses the DESCRIPTION arm on its own — the one that actually identifies our write, since + it carries the verdict word, the short sha and the base.""" + _standing_status(gitea, state="success", description="Review-verdict: MERGEABLE @ deadbee (base: main)") + result = gitea.run("42", "MERGEABLE") + + assert result.returncode != 0, "a row whose DESCRIPTION names a different head was accepted as this run's write" + assert "it was overwritten" in result.stderr + assert gitea.comments() == [] -- 2.47.3 From a55f47023ea506c57dea90a536282e540b20af60 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 30 Aug 2026 00:33:26 +0200 Subject: [PATCH 09/17] =?UTF-8?q?fix(845):=20a=20cross-family=20review=20a?= =?UTF-8?q?nd=20a=20fix-commit=20re-review=20=E2=80=94=20the=20.state=20fa?= =?UTF-8?q?llback=20recreated=20#845,=20and=20the=20paging=20model=20was?= =?UTF-8?q?=20backwards?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more independent cold reviews: GPT-5.6 via Codex (cross-family) and a worktree-isolated Opus re-review of the previous fix commit. They converged on the paging defect from different directions. Each finding below was reproduced before acting. HIGH — the `(.status // .state)` fallback recreated the bug it was added to prevent. The gate reads `.status` and nothing else. On a response carrying only `.state`, the fallback made THIS tool accept and report success while the gate saw no recognisable verdict and re-derived it — which is #845 exactly, reached through the check meant to stop it. My comment claimed it "degrades to working"; it degrades to the bug. Removed: the writer now reads exactly what the gate reads, and a shape neither understands is refused loudly. MEDIUM — the allow-list was applied to `failure` too, which the gate deliberately does not do. `review-verdict.yml` inherits a `success` only from a member but a `failure` from any ATTRIBUTABLE account, and says so at length: inheriting a rejection can only withhold an exemption, while re-deriving one can turn it green. So refusing an off-list `BLOCKED` refused a verdict the gate honours, told the reviewer their rejection would be re-derived when it would not, and — because the comment is withheld on refusal — left a real reviewer no supported way to record a rejection at all. Membership is now required for `success` only. MEDIUM — the paging model was backwards, and so was the reasoning built on it. Gitea selects each context's MAX row id, orders those DESCENDING, and paginates that. Measured: a head with ids [17,19,...,41,43] returns 41 and 43 at `?limit=2`, and on another head the h10 row appears at `limit=9` but not `limit=8`, exactly its rank by recency. The status this script has just POSTed is the newest on the head, so it sorts FIRST and page 1 always holds it. The shim appended it LAST and sliced from the front, which manufactured a truncation the server cannot produce — so `test_a_head_carrying_more_contexts_than_the_server_cap_refuses` asserted a fiction, and the `?limit=100` "witness" was circular: observable only because the fixture was wrong in precisely the direction that made it observable. The shim now orders newest-first, that test is deleted, and `?limit=100` is kept as declared-unwitnessed insurance rather than dressed up as a tested guard. Also corrected, each verified first: - The `readback_rows` fallback was unreachable and its test never reached the branch it named. The fallback is gone; the test now asserts the message an empty body actually produces. The blank-count bug was real and was fixed by the restructure, not by the clause I credited. - "A repo-wide merge outage" overstated the guard's role. Such an edit landing WOULD be one, but `script-tests` is ADVISORY — measured, the required contexts are `Build & test`, `EF migration integrity` and `review-verdict/h10` — so the writer suite already reddens the PR. What alignment buys is a diagnostic naming the cause instead of 31 unrelated failures. Restated in all four places, and connected to the snapshot boundary, which is the route by which the outage is actually reachable. - The IFS test's docstring blamed the wrong split. `load` stores the RAW string, so its pin affects validation only; membership is the `contains` split, and only that one is witnessed. - The two `|| die` guards on already-constrained jq output are declared unreachable rather than counted as protection, and a multi-document body now refuses instead of printing "0 0 separate rows". - Why the writer may hardcode a limit where the gate deliberately refused to: the gate must prove a verdict ABSENT, which no single page can establish; this only confirms a row it just wrote is PRESENT. Presence needs no completeness argument. CORRECTION to the previous commit message: it claimed four clauses were made witnessed, including the `set -f` restoration. Measured on that predecessor, `set -f` restoration already reddened its named test at both sites, and the one genuinely unwitnessed `set -f` — the `contains`-side pathname guard — still is, correctly, since the validator excludes every metacharacter before it runs. Three, not four. fixes #845 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW --- CLAUDE.md | 2 +- docs/ci-cd.md | 10 +- .../records/ci/exemption-provenance.md | 20 ++- docs/remote-state-inventory.md | 2 +- scripts/lib/h10-reviewers.sh | 21 ++- scripts/post-review-verdict.sh | 141 ++++++++++------ scripts/tests/test_post_review_verdict.py | 152 +++++++++++++----- scripts/tests/test_pr_changed_files.py | 9 +- 8 files changed, 251 insertions(+), 106 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 6308ab8c6..1d677a614 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. **The credential you post with must be an account on `H10_REVIEWERS` in `.gitea/workflows/review-verdict.yml`** (`timothy` today) — since ersatztv#742 the gate inherits an existing `success` only from an allow-listed creator (an existing `failure` is left alone on a weaker attributability test, so an attributable rejection VISIBLE AT THE FIRST READ is not re-derived into a green — a rejection landing later, inside a run's own write window, is a separate and still-open route, ersatztv#849), and since ersatztv#845 the script ENFORCES that coupling rather than assuming it: it reads its own status back and refuses, before writing the verdict comment, unless the recorded `.creator.login` is on that allow-list — so a verdict posted with any other account fails loudly at your terminal instead of being reported as success and silently re-derived on the next PR event. **The status is still written** — the check runs after the POST, because it measures the creator Gitea recorded rather than what the credential claims — and what is withheld is the verdict COMMENT, which leaves the merge hook at condition (c) with nothing to classify, i.e. an `ask`. So a refusal leaves a green `review-verdict/h10` standing on that head that the gate itself will not inherit; branch protection binds the context NAME and not its issuer, so do not read that green as consent. The allow-list is derived from the workflow by `scripts/lib/h10-reviewers.sh`; it is never restated. +- `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 credential you post with must be an account on `H10_REVIEWERS` in `.gitea/workflows/review-verdict.yml`** (`timothy` today) — since ersatztv#742 the gate inherits an existing `success` only from an allow-listed creator (an existing `failure` is left alone on a weaker attributability test, so an attributable rejection VISIBLE AT THE FIRST READ is not re-derived into a green — a rejection landing later, inside a run's own write window, is a separate and still-open route, ersatztv#849), and since ersatztv#845 the script ENFORCES that coupling rather than assuming it: it reads its own status back and refuses, before writing the verdict comment, unless the recorded `.creator.login` is on that allow-list — so a POSITIVE verdict posted with any other account fails loudly at your terminal instead of being reported as success. The gate still re-derives such a status on the next PR event — that part is unchanged; what the check removes is the tool telling you it worked. **The membership requirement is `success`-only**, mirroring the gate: a `BLOCKED` verdict is honoured from ANY attributable account, so an off-list reviewer can still record a rejection. **The status is still written** — the check runs after the POST, because it measures the creator Gitea recorded rather than what the credential claims — and what is withheld is the verdict COMMENT, which leaves the merge hook at condition (c) with nothing to classify, i.e. an `ask`. So a refused positive verdict leaves a green `review-verdict/h10` standing on that head that the gate itself will not inherit; branch protection binds the context NAME and not its issuer, so do not read that green as consent. The allow-list is derived from the workflow by `scripts/lib/h10-reviewers.sh`; it is never restated. - **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 e650aec02..d8e9804f9 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -1620,8 +1620,14 @@ refuses **before writing the comment** unless `.creator.login` is a member; a `c DERIVED from the `H10_REVIEWERS` literal in `review-verdict.yml` by `scripts/lib/h10-reviewers.sh`, not restated beside it, so there is one declaration rather than two plus a parity test. -What that changes is the *diagnostic*, not the safety direction: a non-allow-listed verdict was -always a stall, never a fail-open. Before, a changed writer credential reported success and each +**The membership requirement is `success`-only, mirroring the gate's own asymmetry.** An existing +`failure` is inherited on ATTRIBUTABILITY alone, whichever account recorded it, so requiring +membership for a `BLOCKED` verdict would refuse one the gate honours perfectly well — and would +leave an off-list reviewer no supported way to record a rejection, since the comment would be +withheld too. A negative verdict from any attributable account is written and commented normally. + +What that changes is the *diagnostic*, not the safety direction: a non-allow-listed POSITIVE verdict +was always a stall, never a fail-open. Before, a changed writer credential reported success and each *subsequent* PR event re-derived the status — to `pending` on a PR that needs a verdict, or to an exemption **`success`** on a docs-only or Renovate-manifest one, which is the quieter and worse half — so the symptom was a PR that kept losing its verdict rather than an immediate refusal. Now it diff --git a/docs/decisions/records/ci/exemption-provenance.md b/docs/decisions/records/ci/exemption-provenance.md index 65dca07bd..342bf5522 100644 --- a/docs/decisions/records/ci/exemption-provenance.md +++ b/docs/decisions/records/ci/exemption-provenance.md @@ -230,6 +230,14 @@ half-state is the `ask` one — unless `.creator.login` is on the allow-list. A (Actions-token) status is refused with its own message, because there is no account to name and "add them to the allow-list" would be the wrong advice. +The writer mirrors the state asymmetry above rather than simplifying it: membership is required for a +`success` only. A `failure` is written and commented normally from any attributable account, because +that is exactly what this record already says the gate does — requiring membership there would refuse +a verdict the gate honours, and withhold the comment that is the only supported way to record a +rejection. It also reads `.status` and NOT `.state`, with no fallback, because the gate reads +`.status`: tolerating a shape the gate cannot read would make the writer report success on a verdict +that gets re-derived, which is #845 recreated by the check meant to prevent it. + The allow-list is not restated on the writer side. `scripts/lib/h10-reviewers.sh` DERIVES it from the `H10_REVIEWERS` literal in this workflow, so there is one declaration rather than two held together by a parity test — the shape `testing.guard-derives-population-from-source` records six failed rounds @@ -243,9 +251,15 @@ refuses; `test_pr_changed_files.py::test_the_H10_REVIEWERS_list_is_glob_free_and same literal with the same anchors OVER THE SAME POPULATION — the whole file — so a shape change reddens CI on the PR that makes it. The population is part of the rule, not an implementation detail: that guard used to scope itself to the classify step's `run:` body, and a second anchored -assignment in any other `run:` block then left it green while making the writer refuse every -verdict, which is a repo-wide merge outage on a PR-only `main`. Measured before it was aligned: -green guard, 31 red writer tests whose diagnostics named no second assignment. +assignment in any other `run:` block then left it green while making the writer refuse every verdict. +Measured before it was aligned: green guard, 31 red writer tests whose diagnostics named no second +assignment. Such an edit reaching `main` WOULD be a repo-wide merge outage — no verdict could be +posted and `review-verdict/h10` is required — but the guard is not the only thing standing in the +way, and saying so would overstate it: the writer suite reddens too, in the ADVISORY `script-tests` +job (the required contexts, measured 2026-08-30, are `Build & test`, `EF migration integrity` and +`review-verdict/h10`). What alignment buys is a diagnostic naming the cause instead of 31 unrelated +failures. The route by which the outage is actually reachable is the snapshot boundary below — a +verdict posted from a checkout predating the edit — which is why the two paragraphs belong together. **The one false ACCEPT this leaves, named with its direction.** The writer reads the workflow in the LOCAL checkout; the gate runs the base-resolved one. On a branch that ADDS a reviewer — the likely diff --git a/docs/remote-state-inventory.md b/docs/remote-state-inventory.md index 812a96b03..71c6f1c31 100644 --- a/docs/remote-state-inventory.md +++ b/docs/remote-state-inventory.md @@ -96,7 +96,7 @@ classifications differ; otherwise the strictest applies and the Note names the e | Site | Class | Note | |---|---|---| -| `scripts/post-review-verdict.sh` — commit-status write | `PINNED` | Re-reads the PR and compares **both** `.head.sha` and `.base.ref` immediately before the POST, and `die`s (exit 1, no status written) on a mismatch **or on a field it cannot read**. That last clause is load-bearing: guarding both comparisons with `[ -n "$x" ] &&` alone makes a well-formed 2xx body that merely omits the field a no-op, and the status is posted having confirmed nothing. Regression-tested against the real predecessor (#778), since the redundant-looking `-z` arm alone mutates green. Closes #706 and #632 for this path by read-compare-refuse, not by CAS: Gitea's status API offers no conditional write. Residual closed on the write ORDER since #792: the status is written first and the comment second, so a refusal can no longer leave a verdict comment with no status behind it — the only reachable half-state is a status with no comment, which the merge hook resolves as `ask` (`release.verdict-writes-status-before-comment`). Since #845 the status is also READ BACK after the POST and refused unless it is the write just made AND its `.creator.login` is on the allow-list derived from the gate — the same read-compare-refuse shape, applied to WHOSE verdict it is rather than to which commit it covers. That read is check-then-act too and is graded with this row rather than separately: it can only withhold this tool's success report, never authorize anything, and the residual it leaves — a non-inheritable status standing with no comment beside it — is the `ask` half-state above. One property of that read is worth recording because no field in the response carries it: `GET /commits/{sha}/status` PAGES, and its `total_count` reports the PAGE rather than the total (measured Gitea 1.27.1, 2026-08-29 — `?limit=2` on a 15-context head returned 2 rows and `total_count: 2`), so a truncated body is indistinguishable from a complete one. The call asks for `?limit=100` against the ~15 contexts this repo produces, and a row it cannot find is refused with the row count it read and NO stated cause, since absence and truncation are not separable here. | +| `scripts/post-review-verdict.sh` — commit-status write | `PINNED` | Re-reads the PR and compares **both** `.head.sha` and `.base.ref` immediately before the POST, and `die`s (exit 1, no status written) on a mismatch **or on a field it cannot read**. That last clause is load-bearing: guarding both comparisons with `[ -n "$x" ] &&` alone makes a well-formed 2xx body that merely omits the field a no-op, and the status is posted having confirmed nothing. Regression-tested against the real predecessor (#778), since the redundant-looking `-z` arm alone mutates green. Closes #706 and #632 for this path by read-compare-refuse, not by CAS: Gitea's status API offers no conditional write. Residual closed on the write ORDER since #792: the status is written first and the comment second, so a refusal can no longer leave a verdict comment with no status behind it — the only reachable half-state is a status with no comment, which the merge hook resolves as `ask` (`release.verdict-writes-status-before-comment`). Since #845 the status is also READ BACK after the POST and refused unless it is the write just made AND its `.creator.login` is on the allow-list derived from the gate — the same read-compare-refuse shape, applied to WHOSE verdict it is rather than to which commit it covers. That read is check-then-act too and is graded with this row rather than separately: it can only withhold this tool's success report, never authorize anything, and the residual it leaves — a non-inheritable status standing with no comment beside it — is the `ask` half-state above. Two properties of that read are recorded because neither is visible in the response. `GET /commits/{sha}/status` PAGES and its `total_count` reports the PAGE rather than the total (measured Gitea 1.27.1, 2026-08-29 — `?limit=2` on a 15-context head returned 2 rows and `total_count: 2`), so a truncated body is indistinguishable from a complete one. That looked like a risk of the verdict row falling off a page and is NOT, because of the second property: the endpoint selects each context's MAX row id and orders those DESCENDING before paginating (same measurement — ids `[17,19,...,41,43]`, and `?limit=2` returns 41 and 43, the two highest). The row this tool has just POSTed is the newest on the head, so it sorts first and page 1 holds it at any page size. `?limit=100` (the instance clamps `limit` to `MAX_RESPONSE_ITEMS`, measured at 50) is kept as insurance against a burst of concurrent status writes and against that ordering changing, and has no observable effect otherwise — stated so it is not mistaken for an untested guard. A row it cannot find is therefore refused with the row count it read and no stated cause; an earlier version of this row named truncation as the candidate cause, before the ordering was measured. | | `scripts/pr-changed-files.sh` — paged file enumeration | `UNSAFE-KNOWN` | #707's fix: `.base.ref`, `.base.sha` and `.head.sha` are captured before paging and re-checked after, and any *observed* movement fails the whole enumeration closed rather than emitting a short list. But before-and-after equality is **ABA-vulnerable** — a `main → scratch → main` retarget during paging can return the same ref and, if nothing merged meanwhile, the same base sha, while the pages in between were diffed against the scratch base. The script's own comment says it narrows rather than erases — "any movement fails" would be stronger than the code. Accepted here because the enumerator cannot close it alone, but be exact about what the caller-side fence does and does not cover: `ci.verdict-write-retarget-fence` counts `change_target_branch` events, so it catches the BASE alias and **nothing else**. A HEAD alias — a force-push `H1 -> H2 -> H1` during pagination — leaves the final `.head.sha` comparison equal while the middle pages were enumerated against `H2`, and NOTHING inside this script can see it. **Fenced at the enforced caller since #803** (2026-08-28): the same timeline walk now also counts `pull_push`, so a head that moved at all during classification withholds the write — subject to that walk's own terminator, which is defeatable by a page of filtered rows (**#870**, pre-existing and equally affecting the base axis). The grade stays `UNSAFE-KNOWN` for the SCRIPT, because the script's own guarantee is unchanged and one-way — a caller that does not run the fence inherits the alias. Enumeration bound to an immutable tree remains unavailable: re-probed at 1.27.1 on 2026-08-28, `compare/{base}...{head}` still returns no `files` key. | | `scripts/select-queue.sh` — issue list, then per-issue `/dependencies` | `UNSAFE-KNOWN` | The open-issue list (labels, milestone, priority) is snapshotted once; per-candidate dependency reads happen seconds later and never re-read the issue's own labels, so an issue claimed `in-progress` in that gap still appears on the shortlist. Accepted: the script authorizes **no write**. The real gate is the four-way claim check in `process.parallel-session-claim`, which runs after selection and re-reads live state by construction. Tightening this would move a check that must be adversarial into a tool that is advisory. | | `scripts/ci-detect-already-validated.sh` — prior-head combined status | `UNSAFE-KNOWN` | Reads the PR head's status and emits `skip=true`, with nothing re-checking before the consuming job runs. Accepted and narrow: the skip elides only **re-running** test/migrations on a tree already validated; the `build` job still builds and pushes unconditionally, so no image ever ships from unvalidated source. | diff --git a/scripts/lib/h10-reviewers.sh b/scripts/lib/h10-reviewers.sh index fce9a3026..b2f16d4b2 100644 --- a/scripts/lib/h10-reviewers.sh +++ b/scripts/lib/h10-reviewers.sh @@ -37,12 +37,21 @@ # already fails the suite if the workflow stops declaring it as exactly one double-quoted # assignment. So a shape change reddens CI on the PR that makes it rather than going stale here. # -# That guard used to scope itself to the classify step's `run:` body, which is NOT the same tree -# this file walks: a second anchored assignment in any other `run:` block left it green while making -# this refuse every verdict — a repo-wide merge outage, `main` being PR-only with -# `review-verdict/h10` required. Measured before the fix: green guard, 31 red writer tests whose -# diagnostics said nothing about a second assignment. The populations are now the same one, which is -# the property to preserve if either side is ever rescoped. +# That guard used to scope itself to the classify step's `run:` body, which is NOT the same tree this +# file walks: a second anchored assignment in any other `run:` block left it green while making this +# refuse every verdict. Measured before the fix: green guard, 31 red writer tests whose diagnostics +# said nothing about a second assignment. +# +# BE EXACT ABOUT THE CONSEQUENCE, because the obvious phrasing overstates it. Such an edit landing on +# `main` WOULD be a repo-wide merge outage — no verdict could be posted, and `review-verdict/h10` is +# a required check on a PR-only `main`. But it is not a silent route in: the writer suite is in the +# `script-tests` job, which goes red on that edit. That job is ADVISORY (measured 2026-08-30, the +# required contexts are `Build & test`, `EF migration integrity` and `review-verdict/h10`), so it +# does not block server-side; what it does is make the PR visibly red. The reachable path to the +# outage is therefore the snapshot boundary above — a verdict posted from a checkout that predates +# the edit — not the guard being green. Aligning the populations buys a legible diagnostic on the PR +# that makes the edit, which is worth having and is not the same as being the only thing standing +# between the repo and an outage. # # SNAPSHOT BOUNDARY, and its DIRECTION, because the direction is the part that matters. This reads # the workflow in the LOCAL checkout, while the gate that actually runs is the one at `origin/main` diff --git a/scripts/post-review-verdict.sh b/scripts/post-review-verdict.sh index d14e4afcf..6d37f90c8 100755 --- a/scripts/post-review-verdict.sh +++ b/scripts/post-review-verdict.sh @@ -269,19 +269,29 @@ printf 'posted status: %s = %s on %s\n' "$STATUS_CONTEXT" "$state" "$short" # itself on the next PR event. It is not repaired here on purpose: a second corrective write is the # sticky-sentinel mechanism ersatztv#849 is separately designing, and inventing a parallel one on # this path would be two mechanisms for one invariant. -# `?limit=100` is not decoration. This endpoint PAGES, and it pages in a way that cannot be detected -# from the response: measured on Gitea 1.27.1 (2026-08-29), `?limit=2` on a head carrying 15 contexts -# returned 2 rows AND `total_count: 2` — the count reports the PAGE, not the total, so a truncated -# body is indistinguishable from a complete one by any field in it. A head whose context count grows -# past the page size would silently stop containing our row, and the refusal below would fire on a -# verdict that is in force. +# WHY OUR ROW IS ALWAYS IN THIS RESPONSE — measured, and the first version of this comment had it +# wrong in a way worth recording. This endpoint PAGES, and its `total_count` reports the PAGE rather +# than the total (`?limit=2` on a 15-context head returns 2 rows and `total_count: 2`), so a +# truncated body is not detectable from any field in it. That looked like a risk of the verdict row +# falling off a page. It is not: Gitea selects the MAX id per context, orders those DESCENDING, and +# paginates that. Measured on the same head — ids +# [17,19,21,23,25,27,30,31,32,34,36,38,39,41,43], and `?limit=2` returns 41 and 43, the two highest. +# The status this script has just POSTed is by construction the newest on the head, so it sorts +# first and page 1 contains it whatever the page size. # -# BE EXACT ABOUT THE HEADROOM, because the obvious reading of `100` is wrong: this instance clamps -# `limit` to `MAX_RESPONSE_ITEMS`, measured at 50 (the same cap `pr-changed-files.sh` and the gate -# page against). So the real ceiling is 50 rows, not 100 — against the 8-15 status contexts a head -# here carries. Asking for 100 costs nothing and takes whatever the server will give; the margin is -# roughly 3x, not 6x, and it SHRINKS as required checks are added. The diagnostic below states the -# row count it actually read, so an overflow is legible rather than mysterious. +# `?limit=100` therefore has NO observable effect under that ordering, and is kept anyway as the +# cheapest insurance against the two ways the reasoning could stop holding: a burst of concurrent +# status writes between our POST and this read, and the ordering property changing. Said to be +# unwitnessed ON PURPOSE — no test can show an effect a correct server does not produce — rather +# than left looking like a guard someone forgot to test. The instance clamps `limit` to +# `MAX_RESPONSE_ITEMS`, measured at 50, so 100 asks for whatever the server will give. +# +# THE GATE SOLVES THIS DIFFERENTLY AND THAT IS NOT AN INCONSISTENCY. `review-verdict.yml` rejected a +# hardcoded limit for its own read and uses a page-2 probe it calls cap-independent by construction. +# The two are asking different questions: the gate must prove a verdict is ABSENT before acting on +# that absence, which no single page can establish; this script only has to confirm that a row it +# just wrote is PRESENT, and refuses when it is not. Presence needs no completeness argument, which +# is why the cheaper mechanism is sufficient here and would not be there. status_readback=$(api_get "repos/$owner/$repo/commits/$sha/status?limit=100") \ || die "the '$STATUS_CONTEXT' status was posted on $short, but it could not be read back to confirm the gate will honour it — no comment was written. Re-run once Gitea is reachable." # `.statuses` serialises as `null` rather than `[]` on a head with no statuses (ersatztv#751), so @@ -298,19 +308,34 @@ readback_matches=$(printf '%s' "$status_readback" | jq -c --arg c "$STATUS_CONTE if [ -z "$readback_matches" ] || [ "$readback_matches" = "null" ]; then die "the '$STATUS_CONTEXT' status was posted on $short, but the read-back body carries no \`.statuses\` ARRAY (it is missing, null, or some other type), so nothing about the verdict can be confirmed — no comment was written. This is a response-SHAPE problem; check what the combined-status endpoint returns on this Gitea version." fi +# This `|| die` and the `.[0]` one below are DECLARED UNREACHABLE, not counted as protection: +# `jq -c` has already produced `$readback_matches` as an array, so `length` and `.[0]` cannot fail. +# They are kept for the same reason the rest of this block carries messages — a future edit that +# widens what reaches here should fail with a sentence, not a jq trace — and are stated as +# unreachable so nobody mistakes them for tested guards. readback_count=$(printf '%s' "$readback_matches" | jq -r 'length') \ || die "the '$STATUS_CONTEXT' status was posted on $short, but the matching rows could not be counted — no comment was written." +# A body carrying SEVERAL JSON documents makes jq emit one count per document, and the arithmetic +# comparisons below would then interpolate "0 0" into a message about "0 0 separate rows". Not +# producible by Gitea and it already failed closed, but a mangled diagnostic is the defect class this +# change exists to remove, so the shape is rejected explicitly. +case "$readback_count" in + '' | *[!0-9]*) + die "the '$STATUS_CONTEXT' status was posted on $short, but the read-back did not yield a single countable list of rows (got '$readback_count') — no comment was written. This is a response-SHAPE problem; the body may carry more than one JSON document." + ;; +esac if [ "$readback_count" = 0 ]; then - # Deliberately does NOT assert why. Two causes reach here and the response cannot tell them apart: - # the status genuinely is not there, or the row list was truncated by paging. Naming one would be - # stating a cause that may not have happened (#859). + # Does NOT assert a single cause. Reaching here means the row this run just POSTed is absent from + # a response that, by the ordering measured above, should have it FIRST — so the honest statement + # is that something removed or replaced it, not a guess at which. Paging is deliberately not named: + # an earlier version named it, and it is not a live candidate for THIS row. # - # `[ -n ]` on the VALUE, not `||` on jq's exit. jq exits 0 with EMPTY OUTPUT on empty input, so an - # exit-status fallback is dead exactly where it is needed — a 200 with an empty body printed "not - # among the status rows", with the count this branch exists to report left blank. - readback_rows=$(printf '%s' "$status_readback" | jq -r '.statuses | length' 2>/dev/null) || readback_rows='' - [ -n "$readback_rows" ] || readback_rows='an unreadable number of' - die "the '$STATUS_CONTEXT' status was posted on $short but is not among the $readback_rows status rows read back, so it is UNPROVEN that the verdict landed — no comment was written. Either it was removed, or this head now carries more status contexts than one page returns. Check the PR's checks before re-running." + # `.statuses` is known to be an array here (the branch above refused otherwise), so `length` cannot + # fail and needs no fallback. The `|| readback_rows=...` guard that used to be here was dead for a + # reason worth remembering: jq exits 0 with EMPTY OUTPUT on empty input, so an exit-status fallback + # would not have fired even in the case it was written for. + readback_rows=$(printf '%s' "$status_readback" | jq -r '.statuses | length') + die "the '$STATUS_CONTEXT' status was posted on $short but is not among the $readback_rows status rows read back, so it is UNPROVEN that the verdict landed — no comment was written. The row this run wrote is the newest on the head and should sort first, so its absence means something removed or replaced it. Check the PR's checks before re-running." fi if [ "$readback_count" != 1 ]; then # Not reachable against Gitea as measured (the combined endpoint returns the latest row PER @@ -321,20 +346,24 @@ if [ "$readback_count" != 1 ]; then fi readback_entry=$(printf '%s' "$readback_matches" | jq -c '.[0]') \ || die "the '$STATUS_CONTEXT' status was posted on $short, but its row could not be read — no comment was written." -# `.status`, NOT `.state`. A row inside `.statuses[]` serialises its state under the key `status`; -# `state` exists only as the AGGREGATE at the top level of the combined body. Measured on Gitea -# 1.27.1, 2026-08-29, against a real `review-verdict/h10` row. Reading `.state` here yields the empty -# string for every well-formed response, which compares unequal to the state just posted and would -# have refused EVERY verdict — a repo-wide deadlock, since no PR can merge without one. The gate -# reads the same endpoint and the same key (`review-verdict.yml`, `ex_state=$(... jq -r '.status')`), -# so the two stay wrong or right together. `.state` is accepted as a fallback only so a server that -# spells it the other way degrades to working rather than to a deadlock; it is never a second source -# of truth, because the value still has to EQUAL the state this run posted. +# `.status`, NOT `.state`, and with NO FALLBACK to `.state`. A row inside `.statuses[]` serialises +# its state under the key `status`; `state` exists only as the AGGREGATE at the top level of the +# combined body. Measured on Gitea 1.27.1, 2026-08-29, against a real `review-verdict/h10` row. +# Reading `.state` per row yields the empty string for every well-formed response, which compares +# unequal to the state just posted and would refuse EVERY verdict — a repo-wide deadlock. +# +# A `(.status // .state)` fallback was here and was WRONG, recorded because it looks like harmless +# defensiveness. The gate reads `.status` and nothing else (`review-verdict.yml`, +# `ex_state=$(... jq -r '.status // ""')`). On a response carrying only `.state`, the fallback made +# THIS tool accept and report success while the gate saw no recognisable verdict and re-derived it +# — ersatztv#845 itself, recreated by the code meant to prevent it. The writer must read exactly +# what the gate reads: on a shape neither understands, refusing loudly here is correct, and +# tolerating one the gate cannot is not. # Each extraction carries its own `|| die`. Without one they still fail closed — a jq error under # `set -e` aborts the script before the comment — but with only jq's own stderr, which does not say # that a status was left standing on the head. `.creator` arriving as a STRING rather than an object # is the shape that reaches this (probed: jq exits 5, "Cannot index string with string"). -readback_state=$(printf '%s' "$readback_entry" | jq -r '(.status // .state) // ""') \ +readback_state=$(printf '%s' "$readback_entry" | jq -r '.status // ""') \ || die "the '$STATUS_CONTEXT' status was posted on $short but its state could not be read from the response — no comment was written." readback_desc=$(printf '%s' "$readback_entry" | jq -r '.description // ""') \ || die "the '$STATUS_CONTEXT' status was posted on $short but its description could not be read from the response — no comment was written." @@ -367,23 +396,39 @@ if [ -z "$readback_creator" ]; then # one would assert a cause this response cannot establish. die "the '$STATUS_CONTEXT' status on $short reads back with no attributable creator (absent, null, or an empty login). A status POSTed by an ACTIONS token reads exactly this way, and the gate never inherits one as a reviewer verdict (ersatztv#742), so this verdict would be silently re-derived. No comment was written. Post it with a reviewer's own credential." fi -# Explicit capture rather than `if ! etv_h10_reviewers_contains ...`, so the THIRD outcome cannot be -# folded into the second. The function answers 0 member / 1 not-a-member / 2 cannot-tell, and an -# abort inside it lands here as a non-zero code rather than skipping both arms of an `if !`. -set +e -etv_h10_reviewers_contains "$readback_creator" -h10_membership=$? -set -e -case "$h10_membership" in - 0) ;; - 1) - die "the '$STATUS_CONTEXT' status on $short was posted by '$readback_creator', which is NOT on the gate's H10_REVIEWERS allow-list ('$ETV_H10_REVIEWERS'). The status is written but the gate will not inherit it: the next PR event re-derives it and posts over it, and the PR deadlocks with no visible cause. No comment was written. Either post with an allow-listed reviewer's credential, or add '$readback_creator' to H10_REVIEWERS in .gitea/workflows/review-verdict.yml." - ;; - *) - die "could not determine whether '$readback_creator' is on the gate's H10_REVIEWERS allow-list (membership check returned $h10_membership). Refusing to report a verdict as posted when it is unknown whether the gate will honour it. No comment was written." - ;; -esac -printf 'verdict author: %s (on the gate allow-list: %s)\n' "$readback_creator" "$ETV_H10_REVIEWERS" +# THE ALLOW-LIST APPLIES TO `success` ONLY, because that is what the gate does, and mirroring it is +# the entire point of this check. `review-verdict.yml` short-circuits on an existing `success` only +# when the creator is a MEMBER, but on an existing `failure` when it is merely ATTRIBUTABLE — any +# account, because inheriting a rejection can only ever withhold an exemption while re-deriving one +# can turn it green. Enforcing membership on a `failure` here would therefore refuse a verdict the +# gate honours perfectly well, and say so with a diagnostic that is simply false: it would tell an +# off-list reviewer their rejection will be re-derived when it will not. Worse, it would refuse to +# write the BLOCKED comment, leaving a real reviewer no supported way to record a rejection. +# +# Attributability is already established for both states by the checks above — the creator is +# non-null, and the description is the one this run wrote, which is `Review-verdict:`-shaped and +# carries this base. Membership is the only additional thing `success` needs. +if [ "$state" = "success" ]; then + # Explicit capture rather than `if ! etv_h10_reviewers_contains ...`, so the THIRD outcome cannot + # be folded into the second. The function answers 0 member / 1 not-a-member / 2 cannot-tell. + set +e + etv_h10_reviewers_contains "$readback_creator" + h10_membership=$? + set -e + case "$h10_membership" in + 0) + printf 'verdict author: %s (on the gate allow-list: %s)\n' "$readback_creator" "$ETV_H10_REVIEWERS" + ;; + 1) + die "the '$STATUS_CONTEXT' status on $short was posted by '$readback_creator', which is NOT on the gate's H10_REVIEWERS allow-list ('$ETV_H10_REVIEWERS'). A positive verdict is inherited only from an allow-listed account, so the status is written but the gate will not honour it: the next PR event re-derives it and posts over it, and the PR stalls with no visible cause. No comment was written. Either post with an allow-listed reviewer's credential, or add '$readback_creator' to H10_REVIEWERS in .gitea/workflows/review-verdict.yml." + ;; + *) + die "could not determine whether '$readback_creator' is on the gate's H10_REVIEWERS allow-list (membership check returned $h10_membership). Refusing to report a positive verdict as posted when it is unknown whether the gate will honour it. No comment was written." + ;; + esac +else + printf 'verdict author: %s (a %s verdict is honoured from any attributable account, so the allow-list does not apply)\n' "$readback_creator" "$state" +fi # --- The comment (human-readable artifact + the hook's condition-(c) input). -------------------- # Written LAST, after the gating status exists (ersatztv#792). The verdict line MUST start the line: diff --git a/scripts/tests/test_post_review_verdict.py b/scripts/tests/test_post_review_verdict.py index e0484db15..c62e4543a 100644 --- a/scripts/tests/test_post_review_verdict.py +++ b/scripts/tests/test_post_review_verdict.py @@ -122,17 +122,22 @@ if "/commits/" in url and url.split("?")[0].endswith("/status"): # script's parser by construction and test nothing — the shape below is what Gitea 1.27.1 # actually returns, measured 2026-08-29. entry["status"] = entry.pop("state") - # PAGING, modelled from the measured server rather than assumed away. Gitea applies - # `DEFAULT_PAGING_NUM` (30) when no `limit` is given and clamps to `MAX_RESPONSE_ITEMS` (50), - # and `total_count` reports the PAGE rather than the total. Without this the `?limit=100` the - # script sends is invisible to every test — the shim used to discard the query string, so - # deleting the parameter reddened nothing. + # PAGING, modelled from the measured server. Gitea selects each context's MAX row id, orders + # those DESCENDING, then paginates — so the status just POSTed, being the newest id on the head, + # sorts FIRST and is on page 1 whatever the page size. Measured against the live instance: a head + # with ids [17,19,...,41,43] returns 41 and 43 at `?limit=2`, the two highest. + # + # An earlier version of this shim appended the new row LAST and sliced from the front, which made + # it fall off the page and manufactured a truncation the server cannot produce. Two tests were + # written against that fiction. Getting the ORDER right is what makes the paging model faithful; + # `DEFAULT_PAGING_NUM` (30) and the `MAX_RESPONSE_ITEMS` (50) clamp are modelled too, and + # `total_count` reports the PAGE rather than the total, as measured. filler = int((state / "status_filler").read_text()) if (state / "status_filler").exists() else 0 - rows = [ + older = [ {"context": "ci/filler-%d" % i, "status": "success", "description": "f", "creator": None} for i in range(filler) ] - rows.append(entry) + rows = [entry] + list(reversed(older)) # newest first query = urllib.parse.parse_qs(urllib.parse.urlparse(url).query) limit = min(int(query.get("limit", ["30"])[0]), 50) rows = rows[:limit] @@ -883,11 +888,12 @@ def test_a_status_row_carrying_NO_state_field_says_so_instead_of_blaming_a_race( def test_a_readback_MISSING_our_context_reports_the_row_count_and_names_no_cause(gitea): - """Two causes reach this branch and the response cannot tell them apart: the status is genuinely - gone, or the row list was truncated by paging. Measured on Gitea 1.27.1 (2026-08-29), the - combined endpoint's `total_count` reports the PAGE and not the total (`?limit=2` on a 15-context - head returned 2 rows and `total_count: 2`), so no field in the body distinguishes them. The - diagnostic therefore reports how many rows it read and names neither cause as the one.""" + """The row this run wrote is the newest on the head and sorts first, so its absence means + something removed or replaced it — the diagnostic reports how many rows it read and does not + guess at which. Paging is deliberately NOT offered as a cause: an earlier version named it, on + the strength of `total_count` reporting the page rather than the total, before the selection + ORDER was measured (max id per context, descending) and the truncation shown to be unproducible + for this row. This also witnesses the row count, which the empty-body case cannot reach.""" gitea.set_status_body( json.dumps( { @@ -1011,37 +1017,29 @@ def test_sourcing_the_library_does_not_disturb_the_callers_globbing_setting(tmp_ assert "GLOB=off" in off.stdout, off.stdout -def test_the_readback_asks_for_a_page_that_reaches_past_the_other_contexts(gitea): - """Witnesses `?limit=100`, which was previously unobservable. +def test_the_verdict_row_is_found_however_many_other_contexts_the_head_carries(gitea): + """The property that makes paging a non-issue for this row, pinned rather than argued. - Gitea applies `DEFAULT_PAGING_NUM` (30) when no `limit` is given and clamps to - `MAX_RESPONSE_ITEMS` (50) — so a head carrying more status contexts than one default page holds - would stop containing the verdict row, and the read-back would refuse a verdict that IS in - force. That is the deadlock direction on the one path that must not deadlock. With 40 unrelated - contexts ahead of it, the row is reachable only because the request asks for more than 30. + Gitea selects each context's MAX row id, orders those DESCENDING, and paginates that. The status + this script has just POSTed is the newest id on the head, so it sorts first and page 1 contains + it whatever the page size — measured on the live instance, where `?limit=2` returns the two + highest ids. + + This replaces a test that asserted the opposite: that with 40 other contexts the row was + reachable only because of `?limit=100`. That scenario was an artifact of a shim which appended + the new row LAST and sliced from the front. `?limit=100` has no observable effect under the real + ordering and is kept as insurance, not as a guard — so nothing here claims to witness it. """ - gitea.set_status_filler(40) + gitea.set_status_filler(80) result = gitea.run("42", "MERGEABLE") assert result.returncode == 0, ( - f"the verdict writer REFUSED a verdict that is in force, behind 40 other contexts — the " - f"read-back is not asking for a big enough page: {result.stderr}" + f"the verdict row was not found behind 80 other contexts, though it is the newest on the " + f"head and should sort first: {result.stderr}" ) assert len(gitea.comments()) == 1 -def test_a_head_carrying_more_contexts_than_the_server_cap_refuses_and_says_the_row_count(gitea): - """The residual, pinned rather than described. 50 is the server's own ceiling, so past it the - row genuinely cannot be fetched in one page and the tool refuses — reporting how many rows it - read, so the cause is legible instead of looking like a vanished status.""" - gitea.set_status_filler(60) - result = gitea.run("42", "MERGEABLE") - - assert result.returncode != 0 - assert "not among the 50 status rows" in result.stderr, result.stderr - assert gitea.comments() == [] - - def test_TWO_rows_for_our_context_are_ambiguous_rather_than_resolved_by_taking_the_first(gitea): """`[0]` would pick a row and judge ITS author while a different row for the same context is the one actually standing. Unreachable against Gitea as measured (the combined endpoint returns the @@ -1077,23 +1075,34 @@ def test_TWO_rows_for_our_context_are_ambiguous_rather_than_resolved_by_taking_t assert gitea.comments() == [] -def test_an_EMPTY_body_still_reports_a_row_count_rather_than_a_blank(gitea): - """jq exits 0 with EMPTY OUTPUT on empty input, so an exit-status fallback for the row count is - dead exactly where it is needed: the message read "not among the status rows". The count exists - to make a paging overflow legible, so the one shape where it goes blank is the one to pin.""" +def test_an_EMPTY_response_body_is_refused_as_a_SHAPE_problem(gitea): + """An empty body yields no `.statuses` array, so it must refuse on the shape branch. + + This test previously claimed to witness the row-count fallback and did not: an empty body exits + at the array-TYPE branch and never reaches the count. The count IS witnessed, by + `test_a_readback_MISSING_our_context_...`, which supplies a real array with no matching row. The + fallback the old test was written for turned out to be dead code and is gone — `.statuses` is + known to be an array by the time the count runs. + """ gitea.set_status_body("") result = gitea.run("42", "MERGEABLE") assert result.returncode != 0 - assert "among the status" not in result.stderr, f"the row count came out blank: {result.stderr}" + assert "carries no `.statuses` ARRAY" in result.stderr assert gitea.comments() == [] def test_the_split_pins_IFS_so_a_callers_separator_cannot_merge_or_split_entries(tmp_path): """Globbing is only half of what an unquoted expansion depends on; the other half is the ambient - `IFS`. With `IFS=-` a caller turns `renovate-bot` into two entries and `renovate` reads as a - member. Not reachable from the script today (bash resets IFS at startup and the script never - assigns it), so this pins the hardening the library's own header invites by inviting reuse.""" + `IFS`. With `IFS=-` a caller made `renovate` read as a member of the single entry `renovate-bot` + — measured against the real predecessor, not a hand-written mutant. + + The mechanism is the split inside `etv_h10_reviewers_contains`, NOT the one in + `etv_h10_reviewers_load`: `load` stores the RAW string, so its own split can only affect + validation, never membership. Both are pinned, but only the `contains` one is witnessed by this + test, and that is the honest split of the two. Not reachable from the script today (bash resets + IFS at startup and the script never assigns it) — this pins the hardening the library's header + invites by inviting reuse.""" (tmp_path / "wf.yml").write_text(' H10_REVIEWERS="renovate-bot"\n') result = _lib( 'IFS="-"; etv_h10_reviewers_load "$PWD/wf.yml"; set +e; ' @@ -1150,3 +1159,62 @@ def test_a_row_differing_ONLY_in_description_is_refused(gitea): assert result.returncode != 0, "a row whose DESCRIPTION names a different head was accepted as this run's write" assert "it was overwritten" in result.stderr assert gitea.comments() == [] + + +def test_an_off_list_account_may_still_record_a_REJECTION(gitea): + """The allow-list applies to `success` ONLY, because that is what the gate does. + + `review-verdict.yml` short-circuits on an existing `success` only for an allow-listed creator, + but on an existing `failure` for any ATTRIBUTABLE one — inheriting a rejection can only withhold + an exemption, while re-deriving one can turn it green. Enforcing membership on a `failure` here + refused a verdict the gate honours, told the reviewer their rejection would be re-derived when it + would not, and left a real reviewer no supported way to record a rejection at all. + """ + gitea.set_status_creator(OFF_LIST) + result = gitea.run("42", "BLOCKED", "two findings outstanding") + + assert result.returncode == 0, ( + f"an off-list account could not record a BLOCKED verdict, which the gate honours: {result.stderr}" + ) + assert gitea.statuses()[0]["payload"]["state"] == "failure" + assert len(gitea.comments()) == 1, "a rejection must still get its human-readable comment" + assert "two findings outstanding" in gitea.comments()[0]["payload"]["body"] + + +def test_an_off_list_account_still_cannot_record_an_APPROVAL(gitea): + """The other side of the same asymmetry — the positive direction still requires membership, so + the fix above did not widen the check into a no-op.""" + gitea.set_status_creator(OFF_LIST) + result = gitea.run("42", "MERGEABLE") + + assert result.returncode != 0 + assert "H10_REVIEWERS" in result.stderr + assert gitea.comments() == [] + + +def test_a_row_carrying_ONLY_the_top_level_state_key_is_refused(gitea): + """No `.state` fallback. The gate reads `.status` and nothing else, so tolerating a shape it + cannot read would make this tool report success on a verdict the gate re-derives — ersatztv#845, + recreated by the check meant to prevent it. On a shape neither understands, refusing is correct. + """ + short = SHA_A[:7] + gitea.set_status_body( + json.dumps( + { + "total_count": 1, + "statuses": [ + { + "context": "review-verdict/h10", + "state": "success", # the key the GATE does not read + "description": f"Review-verdict: MERGEABLE @ {short} (base: main)", + "creator": {"login": ALLOWLISTED}, + } + ], + } + ) + ) + result = gitea.run("42", "MERGEABLE") + + assert result.returncode != 0, "a row the gate cannot read was accepted, which is the ersatztv#845 stall re-created" + assert "no state field" in result.stderr + assert gitea.comments() == [] diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index 00534e685..19ce83a66 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -2387,9 +2387,12 @@ def _h10_reviewers_literal(): # `grep -c` over the whole YAML, and refuses unless it finds exactly one. A guard scoped to the # classify step therefore does not walk the same tree as its subject: a second anchored # assignment in ANY OTHER `run:` block is invisible here while making the writer refuse every - # verdict, which — `main` being PR-only with `review-verdict/h10` required — is a repo-wide merge - # outage. Measured on this file: injecting one into the last step left this test green and - # reddened 31 writer tests, whose diagnostics say nothing about a second assignment. + # verdict. Measured on this file: injecting one into the last step left this test green and + # reddened 31 writer tests, whose diagnostics say nothing about a second assignment. Such an edit + # reaching `main` would be a repo-wide merge outage — no verdict could be posted and + # `review-verdict/h10` is required — but the honest claim for THIS guard is narrower: the writer + # suite already reddens, in the ADVISORY `script-tests` job, so what aligning the populations + # buys is a diagnostic that names the cause rather than 31 unrelated failures. # See `a-check-must-walk-the-same-tree`; the classify step remains what the GATE runs, and # `_classify_step()` is still the right scope for every other assertion about that body. body = WORKFLOW.read_text() -- 2.47.3 From f54667cc5a744c910d06d0733a06ab4977521d3a Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 30 Aug 2026 01:22:39 +0200 Subject: [PATCH 10/17] fix(845): a declared security invariant had no guard, and four prose claims were falsifiable by grep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourth cold review, worktree-isolated, on the rebased head. Every finding reproduced before acting; the two behavioural ones were re-verified with the reviewer's own mutations. MEDIUM — "no environment variable selects the source file" was declared as a deliberate security decision, in the library header AND in `docs/remote-state-inventory.md`, with no test anywhere. The property held, but restoring the override the way a refactor naturally would (`ETV_H10_REVIEWERS_WORKFLOW="${ETV_H10_REVIEWERS_WORKFLOW:-}"`) left the whole suite green while an env var installed an arbitrary allow-list and an off-list account was accepted as a reviewer. That is the class this branch removed in `28aab4632`, in a clause labelled as a decision rather than as untested. Now pinned, and confirmed to redden under that exact mutation. LOW (behavioural) — a BLOCKED verdict was refused when the allow-list could not be DERIVED. Membership never applies to a `failure`, but the derivation was loaded unconditionally, so a workflow with a broken `H10_REVIEWERS` declaration refused rejections too — the exact outcome the success-only rule exists to prevent ("no supported way to record a rejection"), reached one condition earlier, on the branch most likely to have broken that declaration: the one editing it. The load is now guarded by state; both of its stated properties (after argument validation, before the first network call) survive. A test covers the rejection AND the approval on the same tree, so the guard was narrowed rather than removed. Prose, all four falsifiable by `grep` or `pytest`: - The comment claimed `review-verdict.yml` "rejected a hardcoded limit for its own read". It does not — it sends `?limit=100` on both reads. What it rejected was using that limit as a COMPLETENESS argument, which was dead code because the instance caps at 50. The real point (presence vs absence) was right and is kept; a reader who greps the workflow no longer finds the opposite of what this says. - "31 red writer tests" had drifted to 40 INSIDE this branch as tests were added, in all four places the previous commit said it had restated. Replaced with the invariant — the whole writer suite reddens — per `state-the-invariant-not-the-measurement`, which is exactly the lesson the stale number re-taught. - The sentinel-reset comment cited a measurement that does not isolate it: removing the reset produces a byte-identical refusal, because `load` overwrites both variables on that path. The reset's real ground is the caller that never reaches `load`, which is what `contains` returning 2 rests on. Re-attributed. - `docs/ci-cd.md` still mirrored a TWO-directory input set for `script-tests` after this same PR widened the workflow's own comment to three. Synced. Also: `scripts/lib/review-verdict-vocabulary.sh` still shipped the `set -u` claim this branch demonstrated to be wrong, one file over, and it was the source the new library copied it from. Corrected there too — measured on bash 3.2.57 and 5.3.15, the shell exits outright in both the `if ! f` and `f; rc=$?` shapes; the old reading holds only inside a command substitution. Its sentinel keeps the ground that survives. Nits: a stale "~10s" runtime in a sentence this branch rewrote, replaced with the reason that does not drift; an unresolvable breadcrumb citing a private memory filename rather than a decision key, dropped; "so only this run writes it" narrowed to what the description actually identifies; and the paging test's docstring now says it is a POSITIVE CONTROL rather than claiming a property pin it cannot deliver. fixes #845 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW --- .gitea/workflows/pr-checks.yml | 4 +- docs/ci-cd.md | 8 +- .../records/ci/exemption-provenance.md | 5 +- docs/remote-state-inventory.md | 2 +- scripts/lib/h10-reviewers.sh | 19 +++-- scripts/lib/review-verdict-vocabulary.sh | 15 +++- scripts/post-review-verdict.sh | 35 +++++--- scripts/tests/test_post_review_verdict.py | 80 ++++++++++++++++--- scripts/tests/test_pr_changed_files.py | 5 +- 9 files changed, 135 insertions(+), 38 deletions(-) diff --git a/.gitea/workflows/pr-checks.yml b/.gitea/workflows/pr-checks.yml index f53996261..373d58f58 100644 --- a/.gitea/workflows/pr-checks.yml +++ b/.gitea/workflows/pr-checks.yml @@ -308,7 +308,9 @@ jobs: # the writer derives the H10 allow-list from it, so editing that literal changes the suite's # outcome. Its true input set therefore spans at least three top-level directories, and this # enumeration is the kind that goes stale: a `scripts/**` filter would silently miss a - # `.claude/hooks/**` or `.gitea/workflows/**` edit. At ~10s a filter buys nothing but drift. + # `.claude/hooks/**` or `.gitea/workflows/**` edit. The reason is the INPUT SET, not the cost — + # the suite was ~10s when that was decided and is minutes now, and filtering on `scripts/**` + # would still be wrong. prove-fix: name: "Fix proofs (Proves trailers)" runs-on: small diff --git a/docs/ci-cd.md b/docs/ci-cd.md index d8e9804f9..fce24509d 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -1254,10 +1254,12 @@ genuine pytest regression surface as precisely the red everyone is told to wave name keeps a real failure unambiguous. **Why it runs unconditionally** rather than behind a `scripts/**` path filter: the suite's true -input set spans more than one directory — `test_post_review_verdict.py` and +input set spans THREE top-level directories — `test_post_review_verdict.py` and `test_merge_consent_exemption.py` execute the real `scripts/post-review-verdict.sh` and -`.claude/hooks/pretooluse-merge-consent.sh` — so a `scripts/**` filter would silently miss a -`.claude/hooks/**` edit. The reason is the input set, not the cost: the suite was ~10s when that was +`.claude/hooks/pretooluse-merge-consent.sh`, and since ersatztv#845 the former also READS +`.gitea/workflows/review-verdict.yml`, because the writer derives the H10 allow-list from it and +editing that literal changes the suite's outcome. So a `scripts/**` filter would silently miss a +`.claude/hooks/**` or `.gitea/workflows/**` edit. The reason is the input set, not the cost: the suite was ~10s when that was decided and is now ~4.5 min, and it would still be wrong to filter on `scripts/**`. **It also lints (ersatztv#780).** Early in the job it installs a **pinned** `ruff==0.12.11` and runs diff --git a/docs/decisions/records/ci/exemption-provenance.md b/docs/decisions/records/ci/exemption-provenance.md index 342bf5522..d8091e31b 100644 --- a/docs/decisions/records/ci/exemption-provenance.md +++ b/docs/decisions/records/ci/exemption-provenance.md @@ -252,8 +252,9 @@ same literal with the same anchors OVER THE SAME POPULATION — the whole file reddens CI on the PR that makes it. The population is part of the rule, not an implementation detail: that guard used to scope itself to the classify step's `run:` body, and a second anchored assignment in any other `run:` block then left it green while making the writer refuse every verdict. -Measured before it was aligned: green guard, 31 red writer tests whose diagnostics named no second -assignment. Such an edit reaching `main` WOULD be a repo-wide merge outage — no verdict could be +Measured before it was aligned: green guard, and the WHOLE writer suite red, with no diagnostic +naming a second assignment. Stated as the invariant rather than a count on purpose — the figure +written here first was 31 and had drifted to 40 within the same branch. Such an edit reaching `main` WOULD be a repo-wide merge outage — no verdict could be posted and `review-verdict/h10` is required — but the guard is not the only thing standing in the way, and saying so would overstate it: the writer suite reddens too, in the ADVISORY `script-tests` job (the required contexts, measured 2026-08-30, are `Build & test`, `EF migration integrity` and diff --git a/docs/remote-state-inventory.md b/docs/remote-state-inventory.md index 71c6f1c31..472f4c2f6 100644 --- a/docs/remote-state-inventory.md +++ b/docs/remote-state-inventory.md @@ -96,7 +96,7 @@ classifications differ; otherwise the strictest applies and the Note names the e | Site | Class | Note | |---|---|---| -| `scripts/post-review-verdict.sh` — commit-status write | `PINNED` | Re-reads the PR and compares **both** `.head.sha` and `.base.ref` immediately before the POST, and `die`s (exit 1, no status written) on a mismatch **or on a field it cannot read**. That last clause is load-bearing: guarding both comparisons with `[ -n "$x" ] &&` alone makes a well-formed 2xx body that merely omits the field a no-op, and the status is posted having confirmed nothing. Regression-tested against the real predecessor (#778), since the redundant-looking `-z` arm alone mutates green. Closes #706 and #632 for this path by read-compare-refuse, not by CAS: Gitea's status API offers no conditional write. Residual closed on the write ORDER since #792: the status is written first and the comment second, so a refusal can no longer leave a verdict comment with no status behind it — the only reachable half-state is a status with no comment, which the merge hook resolves as `ask` (`release.verdict-writes-status-before-comment`). Since #845 the status is also READ BACK after the POST and refused unless it is the write just made AND its `.creator.login` is on the allow-list derived from the gate — the same read-compare-refuse shape, applied to WHOSE verdict it is rather than to which commit it covers. That read is check-then-act too and is graded with this row rather than separately: it can only withhold this tool's success report, never authorize anything, and the residual it leaves — a non-inheritable status standing with no comment beside it — is the `ask` half-state above. Two properties of that read are recorded because neither is visible in the response. `GET /commits/{sha}/status` PAGES and its `total_count` reports the PAGE rather than the total (measured Gitea 1.27.1, 2026-08-29 — `?limit=2` on a 15-context head returned 2 rows and `total_count: 2`), so a truncated body is indistinguishable from a complete one. That looked like a risk of the verdict row falling off a page and is NOT, because of the second property: the endpoint selects each context's MAX row id and orders those DESCENDING before paginating (same measurement — ids `[17,19,...,41,43]`, and `?limit=2` returns 41 and 43, the two highest). The row this tool has just POSTed is the newest on the head, so it sorts first and page 1 holds it at any page size. `?limit=100` (the instance clamps `limit` to `MAX_RESPONSE_ITEMS`, measured at 50) is kept as insurance against a burst of concurrent status writes and against that ordering changing, and has no observable effect otherwise — stated so it is not mistaken for an untested guard. A row it cannot find is therefore refused with the row count it read and no stated cause; an earlier version of this row named truncation as the candidate cause, before the ordering was measured. | +| `scripts/post-review-verdict.sh` — commit-status write | `PINNED` | Re-reads the PR and compares **both** `.head.sha` and `.base.ref` immediately before the POST, and `die`s (exit 1, no status written) on a mismatch **or on a field it cannot read**. That last clause is load-bearing: guarding both comparisons with `[ -n "$x" ] &&` alone makes a well-formed 2xx body that merely omits the field a no-op, and the status is posted having confirmed nothing. Regression-tested against the real predecessor (#778), since the redundant-looking `-z` arm alone mutates green. Closes #706 and #632 for this path by read-compare-refuse, not by CAS: Gitea's status API offers no conditional write. Residual closed on the write ORDER since #792: the status is written first and the comment second, so a refusal can no longer leave a verdict comment with no status behind it — the only reachable half-state is a status with no comment, which the merge hook resolves as `ask` (`release.verdict-writes-status-before-comment`). Since #845 the status is also READ BACK after the POST and refused unless it is the write just made, and — for a `success` only, mirroring the gate's own asymmetry — unless its `.creator.login` is on the allow-list derived from the gate (a `failure` is inherited from any attributable account, so requiring membership there would refuse a rejection the gate honours) — the same read-compare-refuse shape, applied to WHOSE verdict it is rather than to which commit it covers. That read is check-then-act too and is graded with this row rather than separately: it can only withhold this tool's success report, never authorize anything, and the residual it leaves — a non-inheritable status standing with no comment beside it — is the `ask` half-state above. Two properties of that read are recorded because neither is visible in the response. `GET /commits/{sha}/status` PAGES and its `total_count` reports the PAGE rather than the total (measured Gitea 1.27.1, 2026-08-29 — `?limit=2` on a 15-context head returned 2 rows and `total_count: 2`), so a truncated body is indistinguishable from a complete one. That looked like a risk of the verdict row falling off a page and is NOT, because of the second property: the endpoint selects each context's MAX row id and orders those DESCENDING before paginating (same measurement — ids `[17,19,...,41,43]`, and `?limit=2` returns 41 and 43, the two highest). The row this tool has just POSTed is the newest on the head, so it sorts first and page 1 holds it at any page size. `?limit=100` (the instance clamps `limit` to `MAX_RESPONSE_ITEMS`, measured at 50) is kept as insurance against a burst of concurrent status writes and against that ordering changing, and has no observable effect otherwise — stated so it is not mistaken for an untested guard. A row it cannot find is therefore refused with the row count it read and no stated cause; an earlier version of this row named truncation as the candidate cause, before the ordering was measured. | | `scripts/pr-changed-files.sh` — paged file enumeration | `UNSAFE-KNOWN` | #707's fix: `.base.ref`, `.base.sha` and `.head.sha` are captured before paging and re-checked after, and any *observed* movement fails the whole enumeration closed rather than emitting a short list. But before-and-after equality is **ABA-vulnerable** — a `main → scratch → main` retarget during paging can return the same ref and, if nothing merged meanwhile, the same base sha, while the pages in between were diffed against the scratch base. The script's own comment says it narrows rather than erases — "any movement fails" would be stronger than the code. Accepted here because the enumerator cannot close it alone, but be exact about what the caller-side fence does and does not cover: `ci.verdict-write-retarget-fence` counts `change_target_branch` events, so it catches the BASE alias and **nothing else**. A HEAD alias — a force-push `H1 -> H2 -> H1` during pagination — leaves the final `.head.sha` comparison equal while the middle pages were enumerated against `H2`, and NOTHING inside this script can see it. **Fenced at the enforced caller since #803** (2026-08-28): the same timeline walk now also counts `pull_push`, so a head that moved at all during classification withholds the write — subject to that walk's own terminator, which is defeatable by a page of filtered rows (**#870**, pre-existing and equally affecting the base axis). The grade stays `UNSAFE-KNOWN` for the SCRIPT, because the script's own guarantee is unchanged and one-way — a caller that does not run the fence inherits the alias. Enumeration bound to an immutable tree remains unavailable: re-probed at 1.27.1 on 2026-08-28, `compare/{base}...{head}` still returns no `files` key. | | `scripts/select-queue.sh` — issue list, then per-issue `/dependencies` | `UNSAFE-KNOWN` | The open-issue list (labels, milestone, priority) is snapshotted once; per-candidate dependency reads happen seconds later and never re-read the issue's own labels, so an issue claimed `in-progress` in that gap still appears on the shortlist. Accepted: the script authorizes **no write**. The real gate is the four-way claim check in `process.parallel-session-claim`, which runs after selection and re-reads live state by construction. Tightening this would move a check that must be adversarial into a tool that is advisory. | | `scripts/ci-detect-already-validated.sh` — prior-head combined status | `UNSAFE-KNOWN` | Reads the PR head's status and emits `skip=true`, with nothing re-checking before the consuming job runs. Accepted and narrow: the skip elides only **re-running** test/migrations on a tree already validated; the `build` job still builds and pushes unconditionally, so no image ever ships from unvalidated source. | diff --git a/scripts/lib/h10-reviewers.sh b/scripts/lib/h10-reviewers.sh index b2f16d4b2..e227c54dd 100644 --- a/scripts/lib/h10-reviewers.sh +++ b/scripts/lib/h10-reviewers.sh @@ -39,7 +39,9 @@ # # That guard used to scope itself to the classify step's `run:` body, which is NOT the same tree this # file walks: a second anchored assignment in any other `run:` block left it green while making this -# refuse every verdict. Measured before the fix: green guard, 31 red writer tests whose diagnostics +# refuse every verdict. Measured before the fix: green guard, and the WHOLE writer suite red — the invariant, deliberately not a count, +# because the number written here first was 31 and had drifted to 40 inside this branch as tests +# were added. Their diagnostics # said nothing about a second assignment. # # BE EXACT ABOUT THE CONSEQUENCE, because the obvious phrasing overstates it. Such an edit landing on @@ -82,11 +84,16 @@ fi unset _etv_h10_root # --- The validation sentinel. -------------------------------------------------------------------- -# Reset to 0 HERE, at load time, for two reasons. It defeats an inherited `ETV_H10_REVIEWERS_OK=1` -# from the environment — measured: exporting `ETV_H10_REVIEWERS_OK=1` with a hostile -# `ETV_H10_REVIEWERS` does not get a verdict past the writer. And it makes "derived successfully" a -# DATA dependency the query function requires, rather than a control-flow gate a caller is trusted -# to have taken. +# Reset to 0 HERE, at load time, so that "derived successfully" is a DATA dependency the query +# function requires rather than a control-flow step a caller is trusted to have taken. +# +# ATTRIBUTED CAREFULLY, because the obvious second reason is not this line's. An earlier version +# claimed the reset defeats an inherited `ETV_H10_REVIEWERS_OK=1` from the environment, and cited a +# measurement showing a hostile environment does not get a verdict past the writer. The measurement +# is real; the conclusion does not follow. Removing this reset produces a byte-identical refusal, +# because `etv_h10_reviewers_load` runs on that path and overwrites both variables anyway. What the +# reset actually covers is a caller that never reaches `load` at all — the data-dependency reason +# above, and what `etv_h10_reviewers_contains` returning 2 rests on. # # WHAT THIS DOES NOT REST ON, stated because the claim was inherited and is wrong. The vocabulary # library argues that under `set -u` an unbound-variable error inside a function "aborts the function diff --git a/scripts/lib/review-verdict-vocabulary.sh b/scripts/lib/review-verdict-vocabulary.sh index dc12aa2d0..8178607c6 100644 --- a/scripts/lib/review-verdict-vocabulary.sh +++ b/scripts/lib/review-verdict-vocabulary.sh @@ -52,9 +52,18 @@ ETV_VERDICT_NEGATIVE_WORDS=(blocked not-mergeable) # 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: +# `if ! etv_verdict_vocabulary_validate; then exit 2; fi`. +# +# THE ORIGINAL REASON GIVEN HERE WAS WRONG, corrected 2026-08-30 (ersatztv#845) because it was being +# copied into new files. It said that under `set -u` an unbound-variable error inside a FUNCTION +# aborts the function but not the script, so an `if ! f` gate takes NEITHER branch and execution +# continues past it. Measured on bash 3.2.57 and 5.3.15 under `set -euo pipefail`: the shell EXITS +# outright, in the `if ! f` form and the `f; rc=$?` form alike — no branch runs, and nothing after +# the gate runs either. That reading holds only inside a COMMAND SUBSTITUTION (`v=$(f) || v=FALLBACK` +# does continue, with the fallback taken), which is not the shape gating this file. +# +# The sentinel is still right, on the ground that survives: it makes validation a DATA dependency the +# derived views require, rather than a control-flow step a caller is trusted to have taken. 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 diff --git a/scripts/post-review-verdict.sh b/scripts/post-review-verdict.sh index 6d37f90c8..5cc5ce0cb 100755 --- a/scripts/post-review-verdict.sh +++ b/scripts/post-review-verdict.sh @@ -111,11 +111,19 @@ esac # usage, and before the first network call, so a checkout that cannot answer the question refuses # without having touched the server. # +# AND ONLY FOR A `success`, for the same reason membership itself is `success`-only (see the +# read-back below). A `failure` is inherited from any attributable account, so who is on the +# allow-list is irrelevant to one — and loading unconditionally meant a workflow whose +# `H10_REVIEWERS` declaration is broken refused REJECTIONS too. That is precisely the outcome the +# success-only argument exists to avoid ("no supported way to record a rejection"), reached one +# condition earlier, on the branch most likely to have broken that declaration: the one editing it. +# # The list is NOT declared here. `.gitea/workflows/review-verdict.yml` owns it and this derives from # that file; see `scripts/lib/h10-reviewers.sh` for why the derivation is a parse rather than a # declaration both sides source. Fails CLOSED on every branch, exactly like the vocabulary above: a # writer that cannot find out whether its own verdicts are inheritable must not post one and report # success, because that combination is the ersatztv#845 stall itself. +if [ "$state" = "success" ]; then H10_REVIEWERS_LIB="$_here/lib/h10-reviewers.sh" [ -r "$H10_REVIEWERS_LIB" ] \ || die "the reviewer allow-list derivation is missing or unreadable at $H10_REVIEWERS_LIB — no status was written" @@ -130,7 +138,8 @@ h10_probe=$( . "$H10_REVIEWERS_LIB" >/dev/null 2>&1 && etv_h10_reviewers_loaded . "$H10_REVIEWERS_LIB" \ || die "the reviewer allow-list derivation at $H10_REVIEWERS_LIB could not be sourced — no status was written" etv_h10_reviewers_load \ - || die "could not derive the gate's H10_REVIEWERS allow-list (see above) — no status was written. Until it can be read, this tool cannot tell whether the verdict it posts would be honoured or silently re-derived." + || die "could not derive the gate's H10_REVIEWERS allow-list (see above) — no status was written. Until it can be read, this tool cannot tell whether the POSITIVE verdict it posts would be honoured or silently re-derived. A negative verdict does not need the allow-list and is unaffected." +fi base_url="${ETV_GITEA_URL:-http://192.168.1.95:3000}/api/v1" repo_slug="${ETV_GITEA_REPO:-timothy/ersatztv}" @@ -286,12 +295,14 @@ printf 'posted status: %s = %s on %s\n' "$STATUS_CONTEXT" "$state" "$short" # than left looking like a guard someone forgot to test. The instance clamps `limit` to # `MAX_RESPONSE_ITEMS`, measured at 50, so 100 asks for whatever the server will give. # -# THE GATE SOLVES THIS DIFFERENTLY AND THAT IS NOT AN INCONSISTENCY. `review-verdict.yml` rejected a -# hardcoded limit for its own read and uses a page-2 probe it calls cap-independent by construction. -# The two are asking different questions: the gate must prove a verdict is ABSENT before acting on -# that absence, which no single page can establish; this script only has to confirm that a row it -# just wrote is PRESENT, and refuses when it is not. Presence needs no completeness argument, which -# is why the cheaper mechanism is sufficient here and would not be there. +# THE GATE ASKS A HARDER QUESTION WITH THE SAME PARAMETER, which is worth stating so this does not +# read as an inconsistency. `review-verdict.yml` sends `?limit=100` on its reads too — what it +# rejected was using that limit as a COMPLETENESS ARGUMENT ("refuse when the page came back FULL at +# 100"), which was dead code because the instance caps at 50; it settles completeness with a page-2 +# probe instead. The two are asking different questions: the gate must prove a verdict is ABSENT +# before acting on that absence, which no single page can establish, while this script only confirms +# that a row it just wrote is PRESENT and refuses when it is not. Presence needs no completeness +# argument, which is why no page-2 probe appears here. status_readback=$(api_get "repos/$owner/$repo/commits/$sha/status?limit=100") \ || die "the '$STATUS_CONTEXT' status was posted on $short, but it could not be read back to confirm the gate will honour it — no comment was written. Re-run once Gitea is reachable." # `.statuses` serialises as `null` rather than `[]` on a head with no statuses (ersatztv#751), so @@ -369,9 +380,9 @@ readback_desc=$(printf '%s' "$readback_entry" | jq -r '.description // ""') \ || die "the '$STATUS_CONTEXT' status was posted on $short but its description could not be read from the response — no comment was written." readback_creator=$(printf '%s' "$readback_entry" | jq -r '.creator.login // ""') \ || die "the '$STATUS_CONTEXT' status was posted on $short but its creator could not be read from the response — no comment was written. The verdict is UNPROVEN, not necessarily wrong." -# A row carrying NEITHER key is a SHAPE problem, not an overwrite. Kept separate because a refusal -# that states a cause which did not happen is its own defect class in this repo (#859): "something -# overwrote it" would send the reader looking for a race that never occurred. +# A row carrying no `status` key is a SHAPE problem, not an overwrite. Kept as its own branch because +# a refusal that states a cause which did not happen is its own defect class in this repo (#859): +# "something overwrote it" would send the reader looking for a race that never occurred. if [ -z "$readback_state" ]; then die "the '$STATUS_CONTEXT' status on $short read back with no state field, so it is UNPROVEN that the verdict just written is the one standing — no comment was written. This is a response-SHAPE problem, not a race: check what \`.statuses[]\` rows look like on this Gitea version." fi @@ -383,7 +394,9 @@ fi # wrong-credential accusation against a credential that is fine. # WHICH ARM CARRIES THE SAFETY, stated because the two OVERLAP and a fixture that changes both # leaves either one disarmable with the suite still green (#685's shape). The DESCRIPTION is the -# identifier: it carries the verdict word, the short sha and the base, so only this run writes it. +# identifier: it carries the verdict word, the short sha and the base. Precisely, that identifies a +# write BY THIS SCRIPT WITH THIS VERDICT ON THIS HEAD AND BASE — a second run with the same verdict +# reproduces it byte for byte — which is what the check needs, since any such row is equally ours. # The STATE is defence in depth — our own writer cannot produce our description with a different # state, since the verdict word determines it. Both are pinned by their own single-field fixture # below rather than by one fixture that differs in both. diff --git a/scripts/tests/test_post_review_verdict.py b/scripts/tests/test_post_review_verdict.py index c62e4543a..8041fb1e0 100644 --- a/scripts/tests/test_post_review_verdict.py +++ b/scripts/tests/test_post_review_verdict.py @@ -1018,17 +1018,19 @@ def test_sourcing_the_library_does_not_disturb_the_callers_globbing_setting(tmp_ def test_the_verdict_row_is_found_however_many_other_contexts_the_head_carries(gitea): - """The property that makes paging a non-issue for this row, pinned rather than argued. + """A POSITIVE CONTROL over a full page, not a property pin — said plainly because the obvious + reading is wrong. - Gitea selects each context's MAX row id, orders those DESCENDING, and paginates that. The status - this script has just POSTed is the newest id on the head, so it sorts first and page 1 contains - it whatever the page size — measured on the live instance, where `?limit=2` returns the two - highest ids. + The shim places our row at index 0 unconditionally (it models Gitea's real ordering: max id per + context, DESCENDING, and our just-POSTed row is the newest). So this cannot fail for the reason + its name suggests — deleting `?limit=100` leaves it green, and it would stay green against a + naive `.statuses[0]` implementation too. What it does check is that `map(select(...))` still + finds our context when the page is full of other rows. - This replaces a test that asserted the opposite: that with 40 other contexts the row was - reachable only because of `?limit=100`. That scenario was an artifact of a shim which appended - the new row LAST and sliced from the front. `?limit=100` has no observable effect under the real - ordering and is kept as insurance, not as a guard — so nothing here claims to witness it. + It replaces a test that asserted the OPPOSITE — that with 40 other contexts the row was reachable + only because of `?limit=100` — which was an artifact of a shim that appended the new row LAST and + sliced from the front, manufacturing a truncation the server cannot produce. `?limit=100` has no + observable effect under the real ordering and is declared unwitnessed insurance in the script. """ gitea.set_status_filler(80) result = gitea.run("42", "MERGEABLE") @@ -1218,3 +1220,63 @@ def test_a_row_carrying_ONLY_the_top_level_state_key_is_refused(gitea): assert result.returncode != 0, "a row the gate cannot read was accepted, which is the ersatztv#845 stall re-created" assert "no state field" in result.stderr assert gitea.comments() == [] + + +def test_an_ENVIRONMENT_variable_cannot_steer_which_workflow_the_allowlist_comes_from(gitea, tmp_path): + """A declared security property that had no guard: `scripts/lib/h10-reviewers.sh` resolves the + gate definition from its OWN location and offers no `ETV_*_WORKFLOW` override, because an env var + choosing which tree answers a security question is an unsound input (ersatztv#858 is the same + shape one file over). + + It held, but nothing pinned it — and restoring the override the way a refactor naturally would + (`ETV_H10_REVIEWERS_WORKFLOW="${ETV_H10_REVIEWERS_WORKFLOW:-}"`) left the whole suite green while + an environment variable installed an arbitrary allow-list and an off-list account was accepted as + a reviewer. That is the same class this branch removed elsewhere, in a clause labelled as a + deliberate decision rather than as untested. + """ + hostile = tmp_path / "hostile.yml" + hostile.write_text(f' H10_REVIEWERS="{OFF_LIST}"\n') + + env = dict(gitea.env) + env["ETV_H10_REVIEWERS_WORKFLOW"] = str(hostile) + gitea.set_status_creator(OFF_LIST) + result = subprocess.run(["bash", str(SCRIPT), "42", "MERGEABLE"], env=env, capture_output=True, text=True) + + assert result.returncode != 0, ( + f"an environment variable steered the allow-list: {OFF_LIST} was accepted as a reviewer" + ) + assert OFF_LIST in result.stderr and "H10_REVIEWERS" in result.stderr + assert gitea.comments() == [] + + +def test_a_REJECTION_survives_a_workflow_whose_allowlist_cannot_be_derived(gitea, tmp_path): + """Membership never applies to a `failure`, so a broken `H10_REVIEWERS` declaration must not + block one. It used to: the derivation was loaded unconditionally, so a workflow with two + assignments refused REJECTIONS as well — the exact outcome the success-only rule exists to avoid + ("no supported way to record a rejection"), one condition earlier, and on the branch most likely + to have broken that declaration: the one editing it. + """ + script = _sandbox(tmp_path, ALLOWLISTED) + workflow = script.parent.parent / ".gitea" / "workflows" / "review-verdict.yml" + body = workflow.read_text() + marker = f'H10_REVIEWERS="{ALLOWLISTED}"' + assert body.count(marker) == 1 + workflow.write_text(body.replace(marker, f'{marker}\n H10_REVIEWERS="someone-else"')) + + gitea.set_status_creator(OFF_LIST) + result = subprocess.run( + ["bash", str(script), "42", "BLOCKED", "findings outstanding"], + env=gitea.env, + capture_output=True, + text=True, + ) + + assert result.returncode == 0, f"a rejection was blocked by an allow-list it does not depend on: {result.stderr}" + assert len(gitea.comments()) == 1 + + # And the positive direction still refuses on the same tree, so the guard was narrowed, not removed. + (gitea.state / "posts.jsonl").unlink(missing_ok=True) + (gitea.state / "pr_get_count").unlink(missing_ok=True) + approval = subprocess.run(["bash", str(script), "42", "MERGEABLE"], env=gitea.env, capture_output=True, text=True) + assert approval.returncode != 0 + assert gitea.statuses() == [], "an underivable allow-list must refuse an APPROVAL before writing" diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index 19ce83a66..28693b6da 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -2388,12 +2388,13 @@ def _h10_reviewers_literal(): # classify step therefore does not walk the same tree as its subject: a second anchored # assignment in ANY OTHER `run:` block is invisible here while making the writer refuse every # verdict. Measured on this file: injecting one into the last step left this test green and - # reddened 31 writer tests, whose diagnostics say nothing about a second assignment. Such an edit + # reddened the WHOLE writer suite, whose diagnostics say nothing about a second assignment (the + # invariant, not a count: the number here was 31 and drifted to 40 as tests were added). Such an edit # reaching `main` would be a repo-wide merge outage — no verdict could be posted and # `review-verdict/h10` is required — but the honest claim for THIS guard is narrower: the writer # suite already reddens, in the ADVISORY `script-tests` job, so what aligning the populations # buys is a diagnostic that names the cause rather than 31 unrelated failures. - # See `a-check-must-walk-the-same-tree`; the classify step remains what the GATE runs, and + # The classify step remains what the GATE runs, and # `_classify_step()` is still the right scope for every other assertion about that body. body = WORKFLOW.read_text() # findall + a SINGLE-ASSIGNMENT assertion, not `search`. `search` returns the FIRST assignment -- 2.47.3 From 0f4bb4244d4945bc5e15b9c6bfd2a757aec25523 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 30 Aug 2026 01:57:04 +0200 Subject: [PATCH 11/17] fix(845): the invariant that replaced a drifting count was itself false, and two of the counts survived MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fifth cold review, on the previous fix commit. Every finding is prose, and three of them are claims that commit's own message said it had fixed. On a branch whose stated thesis is "four prose claims were falsifiable by grep", shipping four more is the finding. Each verified here before acting. - "replaced in all four places" was wrong: there were FIVE occurrences of `31`. Two survived, one of them four lines below the sentence the same commit added saying the number drifts. - "the WHOLE writer suite reddens" is measurably false. Injecting a second anchored `H10_REVIEWERS=` gives 36 failed / 39 passed — a large part, not all. The commit invoked `state-the-invariant-not-the-measurement` and then wrote a claim STRONGER than the count it removed. Now stated without a count and without "whole". - The sentinel re-attribution retracted the environment-defeating ground in one paragraph and still asserted it seven lines below ("the two that were measured"). The sibling library corrected in the same commit got this right ("the ground that survives", singular). This is `stale-comments-sweep-by-subject`: after a retraction, grep the retracted words. - "spans THREE top-level directories" replaced a TRUE hedge with a false count, and did not match the workflow it was supposed to be synced with (which says "at least three"). The suite reads seven — `.claude`, `.gitea`, `.husky`, `docs`, `ErsatzTV`, `scripts`, `web` — so this PR's own doc edits are inputs to that job. Hedge restored, with the reason. Also from that review: the `if [ "$state" = "success" ]` body was unindented at column 0, and "Fails CLOSED on every branch" now means every branch OF THAT LOAD, since the load is itself conditional. Both fixed; the indent change is whitespace-only (`git diff -w` empty). The behaviour was attacked again and held. The reviewer traced a BLOCKED verdict end to end with `scripts/lib/h10-reviewers.sh` DELETED — rc=0, status and comment both posted, no unbound variable and no command-not-found, because `$state` is set by the `case` well before the guard and every reference to the library sits inside one of the two `state = success` blocks. An APPROVAL on the same tree still refuses before writing anything. Removing the guard reddens the new rejection test; `if true` reddens 20 tests. The declared mutation still reddens its named proof. fixes #845 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW --- docs/ci-cd.md | 3 +- .../records/ci/exemption-provenance.md | 8 ++-- scripts/lib/h10-reviewers.sh | 13 +++---- scripts/post-review-verdict.sh | 38 ++++++++++--------- scripts/tests/test_pr_changed_files.py | 7 ++-- 5 files changed, 36 insertions(+), 33 deletions(-) diff --git a/docs/ci-cd.md b/docs/ci-cd.md index fce24509d..15606bbf0 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -1254,7 +1254,8 @@ genuine pytest regression surface as precisely the red everyone is told to wave name keeps a real failure unambiguous. **Why it runs unconditionally** rather than behind a `scripts/**` path filter: the suite's true -input set spans THREE top-level directories — `test_post_review_verdict.py` and +input set spans AT LEAST three top-level directories — the hedge is deliberate, since the corpus +tests also read `docs/`, and an exact count here is the kind that goes stale — `test_post_review_verdict.py` and `test_merge_consent_exemption.py` execute the real `scripts/post-review-verdict.sh` and `.claude/hooks/pretooluse-merge-consent.sh`, and since ersatztv#845 the former also READS `.gitea/workflows/review-verdict.yml`, because the writer derives the H10 allow-list from it and diff --git a/docs/decisions/records/ci/exemption-provenance.md b/docs/decisions/records/ci/exemption-provenance.md index d8091e31b..f3bd6a6bf 100644 --- a/docs/decisions/records/ci/exemption-provenance.md +++ b/docs/decisions/records/ci/exemption-provenance.md @@ -252,13 +252,13 @@ same literal with the same anchors OVER THE SAME POPULATION — the whole file reddens CI on the PR that makes it. The population is part of the rule, not an implementation detail: that guard used to scope itself to the classify step's `run:` body, and a second anchored assignment in any other `run:` block then left it green while making the writer refuse every verdict. -Measured before it was aligned: green guard, and the WHOLE writer suite red, with no diagnostic -naming a second assignment. Stated as the invariant rather than a count on purpose — the figure -written here first was 31 and had drifted to 40 within the same branch. Such an edit reaching `main` WOULD be a repo-wide merge outage — no verdict could be +Measured before it was aligned: the guard green, and the writer suite red with no diagnostic naming +a second assignment. Deliberately stated without a count — a count drifts as tests are added, and +"the whole suite" would be false, since a large part of it reddens rather than all of it. Such an edit reaching `main` WOULD be a repo-wide merge outage — no verdict could be posted and `review-verdict/h10` is required — but the guard is not the only thing standing in the way, and saying so would overstate it: the writer suite reddens too, in the ADVISORY `script-tests` job (the required contexts, measured 2026-08-30, are `Build & test`, `EF migration integrity` and -`review-verdict/h10`). What alignment buys is a diagnostic naming the cause instead of 31 unrelated +`review-verdict/h10`). What alignment buys is a diagnostic naming the cause instead of unrelated failures. The route by which the outage is actually reachable is the snapshot boundary below — a verdict posted from a checkout predating the edit — which is why the two paragraphs belong together. diff --git a/scripts/lib/h10-reviewers.sh b/scripts/lib/h10-reviewers.sh index e227c54dd..021564ec5 100644 --- a/scripts/lib/h10-reviewers.sh +++ b/scripts/lib/h10-reviewers.sh @@ -39,10 +39,9 @@ # # That guard used to scope itself to the classify step's `run:` body, which is NOT the same tree this # file walks: a second anchored assignment in any other `run:` block left it green while making this -# refuse every verdict. Measured before the fix: green guard, and the WHOLE writer suite red — the invariant, deliberately not a count, -# because the number written here first was 31 and had drifted to 40 inside this branch as tests -# were added. Their diagnostics -# said nothing about a second assignment. +# refuse every verdict. Measured before the fix: the guard green, and the writer suite red with no +# diagnostic naming a second assignment. Stated without a count on purpose — a count drifts as tests +# are added, and "the whole suite" would be false (a large part of it reddens, not all of it). # # BE EXACT ABOUT THE CONSEQUENCE, because the obvious phrasing overstates it. Such an edit landing on # `main` WOULD be a repo-wide merge outage — no verdict could be posted, and `review-verdict/h10` is @@ -93,15 +92,15 @@ unset _etv_h10_root # is real; the conclusion does not follow. Removing this reset produces a byte-identical refusal, # because `etv_h10_reviewers_load` runs on that path and overwrites both variables anyway. What the # reset actually covers is a caller that never reaches `load` at all — the data-dependency reason -# above, and what `etv_h10_reviewers_contains` returning 2 rests on. +# above, and what `etv_h10_reviewers_contains` returning 2 rests on. So the sentinel earns its place +# on that ground alone, which is the one that was measured. # # WHAT THIS DOES NOT REST ON, stated because the claim was inherited and is wrong. The vocabulary # library argues that under `set -u` an unbound-variable error inside a function "aborts the function # but not the script", so an `if ! f` gate takes NEITHER branch. Measured here (bash 5.3 and bash # 3.2, `set -euo pipefail`): the shell EXITS outright, in both the `if ! f` shape and the # `f; rc=$?` shape. So that particular skip-past-the-gate route does not exist, and no reasoning -# below depends on it. The sentinel earns its place on the environment-defeating and data-dependency -# grounds above, which are the two that were measured. +# below depends on it. ETV_H10_REVIEWERS_OK=0 ETV_H10_REVIEWERS='' diff --git a/scripts/post-review-verdict.sh b/scripts/post-review-verdict.sh index 5cc5ce0cb..e2a0a5af5 100755 --- a/scripts/post-review-verdict.sh +++ b/scripts/post-review-verdict.sh @@ -120,25 +120,27 @@ esac # # The list is NOT declared here. `.gitea/workflows/review-verdict.yml` owns it and this derives from # that file; see `scripts/lib/h10-reviewers.sh` for why the derivation is a parse rather than a -# declaration both sides source. Fails CLOSED on every branch, exactly like the vocabulary above: a -# writer that cannot find out whether its own verdicts are inheritable must not post one and report -# success, because that combination is the ersatztv#845 stall itself. +# declaration both sides source. Every branch OF THIS LOAD fails CLOSED, exactly like the vocabulary +# above — scoped that way deliberately, since the load itself is now conditional: a writer that +# cannot find out whether its own POSITIVE verdict is inheritable must not post one and report +# success, because that combination is the ersatztv#845 stall itself. A negative verdict never +# reaches here and needs nothing from it. if [ "$state" = "success" ]; then -H10_REVIEWERS_LIB="$_here/lib/h10-reviewers.sh" -[ -r "$H10_REVIEWERS_LIB" ] \ - || die "the reviewer allow-list derivation is missing or unreadable at $H10_REVIEWERS_LIB — no status was written" -# Probe-source in a subshell and require the end-of-file marker before trusting the file, for the -# same reason the vocabulary does: a top-level `exit` in a sourced library terminates THIS script at -# the `source` line, having posted neither a status nor a comment, with exit 0. -# shellcheck source=lib/h10-reviewers.sh -h10_probe=$( . "$H10_REVIEWERS_LIB" >/dev/null 2>&1 && etv_h10_reviewers_loaded 2>/dev/null ) || h10_probe='' -[ "$h10_probe" = 'etv-h10-reviewers-loaded' ] \ - || die "the reviewer allow-list derivation at $H10_REVIEWERS_LIB did not load to completion (syntax error, truncated, or it exits at top level) — no status was written" -# shellcheck source=lib/h10-reviewers.sh -. "$H10_REVIEWERS_LIB" \ - || die "the reviewer allow-list derivation at $H10_REVIEWERS_LIB could not be sourced — no status was written" -etv_h10_reviewers_load \ - || die "could not derive the gate's H10_REVIEWERS allow-list (see above) — no status was written. Until it can be read, this tool cannot tell whether the POSITIVE verdict it posts would be honoured or silently re-derived. A negative verdict does not need the allow-list and is unaffected." + H10_REVIEWERS_LIB="$_here/lib/h10-reviewers.sh" + [ -r "$H10_REVIEWERS_LIB" ] \ + || die "the reviewer allow-list derivation is missing or unreadable at $H10_REVIEWERS_LIB — no status was written" + # Probe-source in a subshell and require the end-of-file marker before trusting the file, for the + # same reason the vocabulary does: a top-level `exit` in a sourced library terminates THIS script at + # the `source` line, having posted neither a status nor a comment, with exit 0. + # shellcheck source=lib/h10-reviewers.sh + h10_probe=$( . "$H10_REVIEWERS_LIB" >/dev/null 2>&1 && etv_h10_reviewers_loaded 2>/dev/null ) || h10_probe='' + [ "$h10_probe" = 'etv-h10-reviewers-loaded' ] \ + || die "the reviewer allow-list derivation at $H10_REVIEWERS_LIB did not load to completion (syntax error, truncated, or it exits at top level) — no status was written" + # shellcheck source=lib/h10-reviewers.sh + . "$H10_REVIEWERS_LIB" \ + || die "the reviewer allow-list derivation at $H10_REVIEWERS_LIB could not be sourced — no status was written" + etv_h10_reviewers_load \ + || die "could not derive the gate's H10_REVIEWERS allow-list (see above) — no status was written. Until it can be read, this tool cannot tell whether the POSITIVE verdict it posts would be honoured or silently re-derived. A negative verdict does not need the allow-list and is unaffected." fi base_url="${ETV_GITEA_URL:-http://192.168.1.95:3000}/api/v1" diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index 28693b6da..fe8280777 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -2388,12 +2388,13 @@ def _h10_reviewers_literal(): # classify step therefore does not walk the same tree as its subject: a second anchored # assignment in ANY OTHER `run:` block is invisible here while making the writer refuse every # verdict. Measured on this file: injecting one into the last step left this test green and - # reddened the WHOLE writer suite, whose diagnostics say nothing about a second assignment (the - # invariant, not a count: the number here was 31 and drifted to 40 as tests were added). Such an edit + # reddened the writer suite, whose diagnostics say nothing about a second assignment. Stated + # without a count on purpose: a count drifts as tests are added, and "the whole suite" would be + # false — a large part of it reddens, not all of it. Such an edit # reaching `main` would be a repo-wide merge outage — no verdict could be posted and # `review-verdict/h10` is required — but the honest claim for THIS guard is narrower: the writer # suite already reddens, in the ADVISORY `script-tests` job, so what aligning the populations - # buys is a diagnostic that names the cause rather than 31 unrelated failures. + # buys is a diagnostic that names the cause rather than unrelated failures. # The classify step remains what the GATE runs, and # `_classify_step()` is still the right scope for every other assertion about that body. body = WORKFLOW.read_text() -- 2.47.3 From 94cc924c3673f470ca2bf2f231543b1dbb8a3ce2 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 30 Aug 2026 02:11:34 +0200 Subject: [PATCH 12/17] =?UTF-8?q?fix(845):=20the=20retracted=20`set=20-u`?= =?UTF-8?q?=20mechanism=20survived=20seven=20lines=20below=20its=20own=20r?= =?UTF-8?q?etraction=20=E2=80=94=20and=20the=20no-`set=20-e`=20case=20is?= =?UTF-8?q?=20now=20measured?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by sweeping the RETRACTED WORDS repo-wide rather than re-reading the file I had edited, which is the `stale-comments-sweep-by-subject` lesson the previous round already charged me with once. `scripts/lib/review-verdict-vocabulary.sh` still said "A control-flow gate can be skipped by an abort" seven lines under the paragraph retracting exactly that mechanism — the same self-contradiction shape as the sentinel comment in the last round, in the file that round corrected. MEASURING IT PROPERLY CHANGED WHAT I CHECKED. The decision record justifies the concern by noting the READ side deliberately runs without `set -e`, and both prior reviewers had measured the claim only under `set -euo pipefail`. If the original reading held without `set -e`, the correction shipped last round would itself have been wrong. Measured on bash 3.2.57 and 5.3.15 across all three modes: [set -u] in f / DEFINITELY_UNSET: unbound variable / exit=127 [set -eu] in f / DEFINITELY_UNSET: unbound variable / exit=1 [set -euo pipefail] in f / DEFINITELY_UNSET: unbound variable / exit=1 No branch taken and nothing after the gate, in every mode — including the one the record relies on. The correction stands. The old reading holds only inside a command substitution (`v=$(f) || v=FALLBACK` continues, fallback taken), which is not the shape gating either file. WHAT IS NOT IN QUESTION, stated because the retraction is easy to over-read: the fail-open that record documents is REAL — with `.*` in the positive list and the negative list written as a scalar, an explicit `BLOCKED @ ` classified `positive`, exit 0. Only the attributed mechanism is wrong: `${#name[@]}` on a scalar yields 1 rather than erroring, so no abort occurred in that scenario at all. The sentinel is still the right design, on the ground that survives — the caller that never runs validation. The same refuted mechanism remains in `release.verdict-vocabulary-shared` and in `test_review_verdict_vocabulary.py`. Correcting a decision record is its own change with its own review surface, so it is tracked as ersatztv#886 with the full measurement, and cross-referenced from the corrected file rather than folded in here. fixes #845 refs #886 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW --- scripts/lib/review-verdict-vocabulary.sh | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/scripts/lib/review-verdict-vocabulary.sh b/scripts/lib/review-verdict-vocabulary.sh index 8178607c6..e8e77f409 100644 --- a/scripts/lib/review-verdict-vocabulary.sh +++ b/scripts/lib/review-verdict-vocabulary.sh @@ -69,8 +69,18 @@ ETV_VERDICT_NEGATIVE_WORDS=(blocked not-mergeable) # 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. +# WHAT THE DATA DEPENDENCY ACTUALLY BUYS, restated because the previous sentence here asserted the +# mechanism retracted above ("a control-flow gate can be skipped by an abort"). Measured on bash +# 3.2.57 and 5.3.15, with `set -u` alone as well as `set -eu` and `set -euo pipefail`: an +# unbound-variable error inside a function EXITS the shell — nothing continues past the gate, so +# there is no skip to defend against. What the dependency covers is the caller that never runs +# validation in the first place: the words are unobtainable unless it ran all the way to its final +# line, so a derived view cannot be reached by a path that simply did not call the validator. +# +# The same refuted mechanism still appears in `release.verdict-vocabulary-shared` and in +# `test_review_verdict_vocabulary.py`; correcting a decision record is its own change, tracked as +# ersatztv#886. The fail-open that record documents is real and is not in question — only the +# mechanism it attributes. ETV_VERDICT_VOCABULARY_OK=0 # --- Validation. ------------------------------------------------------------------------------- -- 2.47.3 From 06873a01132989214756c842442e017c39baa450 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 30 Aug 2026 02:21:55 +0200 Subject: [PATCH 13/17] =?UTF-8?q?fix(845):=20"no=20diagnostic=20naming=20a?= =?UTF-8?q?=20second=20assignment"=20was=20false=20=E2=80=94=20the=20failu?= =?UTF-8?q?res=20DO=20name=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sixth cold review. One HIGH, and it is the sentence the previous commit rewrote. The claim, in five sites across three files, was that a second anchored `H10_REVIEWERS=` reddened the writer suite "with no diagnostic naming a second assignment". Measured: 14 of the 36 failures carry this library's own refusal, verbatim — E AssertionError: h10 reviewers: .../review-verdict.yml carries 2 anchored H10_REVIEWERS assignments, not exactly 1 — refusing to guess which one the gate runs with — which names both the count and the variable. The message predates the alignment commit that introduced the claim, so it was false when written, not newly broken here. What is actually missing is a failure that POINTS AT THE WORKFLOW: the reds surface under unrelated test names (`test_positive_verdict_posts_success_on_the_resolved_head`, …) and the cause has to be inferred from a stack of them. Aligning the populations buys ONE named guard failure instead. All five sites now say that, and "about half the suite" replaces the earlier "the writer suite" where it read as all of it (36 of 75). Two nits from the same round: the aside inserted into `docs/ci-cd.md` turned an em-dash from an introducer into a closing parenthetical and left the enumeration dangling — the hedge now follows the sentence instead of splitting it; and "on that ground alone, which is the one that was measured" implied a contrast that does not exist, since the retracted ground was measured too. Trailing clause dropped. CORRECTION to the previous commit message, which no artifact carries but which was wrong: it said "Removing the guard reddens the new rejection test; `if true` reddens 20 tests", pairing one mutation with two counts. Measured on the load guard: `if true` reddens 1 (the rejection test), `if false` reddens 20. The behaviour was attacked again and held: BLOCKED end to end with the library DELETED gives rc=0 with status and comment posted and an empty stderr, MERGEABLE on the same tree dies before any write, and the declared mutation still reddens its named proof with the manifest's `expect` string. The reviewer also confirmed the retracted `set -u` wording is now gone repo-wide. fixes #845 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW --- docs/ci-cd.md | 6 +++--- docs/decisions/records/ci/exemption-provenance.md | 10 ++++++---- scripts/lib/h10-reviewers.sh | 15 +++++++++++---- scripts/tests/test_pr_changed_files.py | 9 +++++---- 4 files changed, 25 insertions(+), 15 deletions(-) diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 15606bbf0..f781a70cf 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -1254,13 +1254,13 @@ genuine pytest regression surface as precisely the red everyone is told to wave name keeps a real failure unambiguous. **Why it runs unconditionally** rather than behind a `scripts/**` path filter: the suite's true -input set spans AT LEAST three top-level directories — the hedge is deliberate, since the corpus -tests also read `docs/`, and an exact count here is the kind that goes stale — `test_post_review_verdict.py` and +input set spans AT LEAST three top-level directories. `test_post_review_verdict.py` and `test_merge_consent_exemption.py` execute the real `scripts/post-review-verdict.sh` and `.claude/hooks/pretooluse-merge-consent.sh`, and since ersatztv#845 the former also READS `.gitea/workflows/review-verdict.yml`, because the writer derives the H10 allow-list from it and editing that literal changes the suite's outcome. So a `scripts/**` filter would silently miss a -`.claude/hooks/**` or `.gitea/workflows/**` edit. The reason is the input set, not the cost: the suite was ~10s when that was +`.claude/hooks/**` or `.gitea/workflows/**` edit. The hedge is deliberate: the corpus tests read +`docs/` too, so the real figure is higher, and an exact count here is the kind that goes stale. The reason is the input set, not the cost: the suite was ~10s when that was decided and is now ~4.5 min, and it would still be wrong to filter on `scripts/**`. **It also lints (ersatztv#780).** Early in the job it installs a **pinned** `ruff==0.12.11` and runs diff --git a/docs/decisions/records/ci/exemption-provenance.md b/docs/decisions/records/ci/exemption-provenance.md index f3bd6a6bf..6af2013a6 100644 --- a/docs/decisions/records/ci/exemption-provenance.md +++ b/docs/decisions/records/ci/exemption-provenance.md @@ -252,13 +252,15 @@ same literal with the same anchors OVER THE SAME POPULATION — the whole file reddens CI on the PR that makes it. The population is part of the rule, not an implementation detail: that guard used to scope itself to the classify step's `run:` body, and a second anchored assignment in any other `run:` block then left it green while making the writer refuse every verdict. -Measured before it was aligned: the guard green, and the writer suite red with no diagnostic naming -a second assignment. Deliberately stated without a count — a count drifts as tests are added, and -"the whole suite" would be false, since a large part of it reddens rather than all of it. Such an edit reaching `main` WOULD be a repo-wide merge outage — no verdict could be +Measured before it was aligned: the guard green, and about half the writer suite red. Precisely what +was missing is worth stating, since the obvious phrasing is wrong: those failures DO carry the +library's refusal, which names the count and the variable — 14 of 36 did. What was missing is a +failure POINTING AT THE WORKFLOW, because the reds surface under unrelated test names and the cause +has to be inferred from a stack of them. Such an edit reaching `main` WOULD be a repo-wide merge outage — no verdict could be posted and `review-verdict/h10` is required — but the guard is not the only thing standing in the way, and saying so would overstate it: the writer suite reddens too, in the ADVISORY `script-tests` job (the required contexts, measured 2026-08-30, are `Build & test`, `EF migration integrity` and -`review-verdict/h10`). What alignment buys is a diagnostic naming the cause instead of unrelated +`review-verdict/h10`). What alignment buys is ONE named guard failure pointing at the workflow, instead of half the suite reddening under unrelated failures. The route by which the outage is actually reachable is the snapshot boundary below — a verdict posted from a checkout predating the edit — which is why the two paragraphs belong together. diff --git a/scripts/lib/h10-reviewers.sh b/scripts/lib/h10-reviewers.sh index 021564ec5..4650d863c 100644 --- a/scripts/lib/h10-reviewers.sh +++ b/scripts/lib/h10-reviewers.sh @@ -39,9 +39,16 @@ # # That guard used to scope itself to the classify step's `run:` body, which is NOT the same tree this # file walks: a second anchored assignment in any other `run:` block left it green while making this -# refuse every verdict. Measured before the fix: the guard green, and the writer suite red with no -# diagnostic naming a second assignment. Stated without a count on purpose — a count drifts as tests -# are added, and "the whole suite" would be false (a large part of it reddens, not all of it). +# refuse every verdict. Measured before the fix: the guard green, and about half the writer suite red. +# BE PRECISE ABOUT WHAT WAS MISSING, because the obvious phrasing is wrong: those failures DO carry +# this library's refusal, which names the count and the variable — 14 of 36 did. What was missing is +# a failure that POINTS AT THE WORKFLOW: the reds surface under unrelated test names +# (`test_positive_verdict_posts_success_on_the_resolved_head`, …), so the cause has to be inferred +# from a stack of them. Aligning the populations buys ONE named guard failure instead. Counts are +# avoided elsewhere in this comment because they drift as tests are added. +# +# Stated as "about half" rather than "the whole suite" for the same reason — 36 of 75 at the time of +# writing, so "whole" would be false. # # BE EXACT ABOUT THE CONSEQUENCE, because the obvious phrasing overstates it. Such an edit landing on # `main` WOULD be a repo-wide merge outage — no verdict could be posted, and `review-verdict/h10` is @@ -93,7 +100,7 @@ unset _etv_h10_root # because `etv_h10_reviewers_load` runs on that path and overwrites both variables anyway. What the # reset actually covers is a caller that never reaches `load` at all — the data-dependency reason # above, and what `etv_h10_reviewers_contains` returning 2 rests on. So the sentinel earns its place -# on that ground alone, which is the one that was measured. +# on that ground alone. # # WHAT THIS DOES NOT REST ON, stated because the claim was inherited and is wrong. The vocabulary # library argues that under `set -u` an unbound-variable error inside a function "aborts the function diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index fe8280777..de1cd3625 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -2388,13 +2388,14 @@ def _h10_reviewers_literal(): # classify step therefore does not walk the same tree as its subject: a second anchored # assignment in ANY OTHER `run:` block is invisible here while making the writer refuse every # verdict. Measured on this file: injecting one into the last step left this test green and - # reddened the writer suite, whose diagnostics say nothing about a second assignment. Stated - # without a count on purpose: a count drifts as tests are added, and "the whole suite" would be - # false — a large part of it reddens, not all of it. Such an edit + # reddened about half the writer suite. Those failures DO carry the library's refusal, which + # names the count and the variable (14 of 36 did) — what they do NOT do is point at the workflow, + # since they surface under unrelated test names and the cause must be inferred from a stack of + # them. "About half" rather than "the whole suite" because 36 of 75 is not all of it. Such an edit # reaching `main` would be a repo-wide merge outage — no verdict could be posted and # `review-verdict/h10` is required — but the honest claim for THIS guard is narrower: the writer # suite already reddens, in the ADVISORY `script-tests` job, so what aligning the populations - # buys is a diagnostic that names the cause rather than unrelated failures. + # buys is ONE named guard failure pointing at the workflow, rather than half the suite reddening. # The classify step remains what the GATE runs, and # `_classify_step()` is still the right scope for every other assertion about that body. body = WORKFLOW.read_text() -- 2.47.3 From 3bbe4f0635e26d210f02e2ad17dfe69df7d3ff35 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 30 Aug 2026 03:07:38 +0200 Subject: [PATCH 14/17] fix(845): "14 of 36" was a grep LINE count, and alignment ADDS a named failure rather than replacing them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seventh cold review. Three findings, all in prose I wrote correcting earlier prose. HIGH — "14 of 36 did" is not a failure count. `grep -c` over pytest output counts LINES, and `--tb=line` prints each failure twice, so 14 was 7 failures doubled; only 5 distinct tests are involved, and with truncation disabled the figure moves the other way (31 of 36). Measured: raw grep -c 14 of which '^E ' rows 7 of which '^/' tb=line rows 7 DISTINCT failing tests 5 A number that changes with pytest's display options is not a fact about the code, so no figure is given at all now — which is what the same comment already said it was doing about counts. The trap itself is recorded so one is not added back. MEDIUM — "ONE named guard failure INSTEAD OF half the suite reddening" asserts a substitution that does not happen, and contradicted the sentence directly above it. Measured on the aligned tree: the guard failure appears AND the ~36 unrelated reds still appear, in the same job. Alignment adds a diagnosis; it replaces nothing. Now "ALONGSIDE". MEDIUM — the `set -u` sweep was incomplete, and the previous commit message's "gone repo-wide" was false. Four files still asserted the refuted mechanism. The sharpest was `scripts/check-review-verdict.sh`, whose comment said "see the sentinel comment in the library" — and following that pointer landed on the retraction saying the opposite. Corrected there, in `docs/ci-cd.md` and in `test_review_verdict_vocabulary.py`, all three already touched by this branch. Every remaining mention in code, tests and docs is now a quotation INSIDE a retraction; the only standing assertion left is the decision record, which is its own change and stays with ersatztv#886 (now narrowed to exactly that). CORRECTION to the previous commit message: it said the retracted wording was "gone repo-wide". That was true only of the two phrases the round-6 reviewer checked, not of the mechanism. It is true now for code, tests and docs. Also: dropped a "for the same reason" back-reference that resolved to the wrong reason (drift, where the actual reason is falsity), and removed the session narrative from the figure notes — "a first attempt said 14" is autobiography; the durable form is the trap and its consequence. Verified again on this tree: full suite 1278 passed / 2 skipped, catalog in sync, decisions-validate OK, doc-narrative clean. fixes #845 refs #886 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW --- docs/ci-cd.md | 16 +++++++++---- .../records/ci/exemption-provenance.md | 17 +++++++++----- scripts/check-review-verdict.sh | 18 +++++++++++---- scripts/lib/h10-reviewers.sh | 20 +++++++++------- scripts/lib/review-verdict-vocabulary.sh | 7 +++--- scripts/tests/test_pr_changed_files.py | 12 ++++++---- .../tests/test_review_verdict_vocabulary.py | 23 +++++++++++-------- 7 files changed, 71 insertions(+), 42 deletions(-) diff --git a/docs/ci-cd.md b/docs/ci-cd.md index f781a70cf..90ac02714 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -1515,11 +1515,17 @@ nothing. Words are interpolated into a regex, so a `.*` in the positive list wou 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 +before editing either script, because the first version of #788 shipped the hole. The fail-open is +measured: with the negative list written as a scalar and `.*` in the positive list, unvalidated words +reached the regex and an explicit `BLOCKED @ head` classified `positive`, exit 0. + +The MECHANISM first recorded for it was wrong, corrected in ersatztv#845 and stated here so it is not +re-derived: it held that under `set -u` an unbound-variable error inside a function aborts the +function but not the script, so an `if ! validate` form takes neither branch and the `exit 2` is +skipped. Measured on bash 3.2.57 and 5.3.15, under `set -u` alone as well as `set -eu` — the +distinction that matters, since the reader deliberately runs without `set -e` — the shell EXITS +outright, so nothing continues past the gate. That reading holds only inside a command substitution. +What the sentinel actually covers is the caller that never runs validation in the first place. 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. diff --git a/docs/decisions/records/ci/exemption-provenance.md b/docs/decisions/records/ci/exemption-provenance.md index 6af2013a6..c4c01dd8d 100644 --- a/docs/decisions/records/ci/exemption-provenance.md +++ b/docs/decisions/records/ci/exemption-provenance.md @@ -252,15 +252,20 @@ same literal with the same anchors OVER THE SAME POPULATION — the whole file reddens CI on the PR that makes it. The population is part of the rule, not an implementation detail: that guard used to scope itself to the classify step's `run:` body, and a second anchored assignment in any other `run:` block then left it green while making the writer refuse every verdict. -Measured before it was aligned: the guard green, and about half the writer suite red. Precisely what -was missing is worth stating, since the obvious phrasing is wrong: those failures DO carry the -library's refusal, which names the count and the variable — 14 of 36 did. What was missing is a -failure POINTING AT THE WORKFLOW, because the reds surface under unrelated test names and the cause -has to be inferred from a stack of them. Such an edit reaching `main` WOULD be a repo-wide merge outage — no verdict could be +Measured before it was aligned: the guard green, and about half the writer suite red. What was +missing is worth stating precisely, because the obvious phrasing is wrong twice over. Those failures +DO carry the library's refusal, which names the count and the variable. And alignment ADDS a named +guard failure rather than replacing them. What was missing is a failure POINTING AT THE WORKFLOW, +since the reds surface under unrelated test names and the cause had to be inferred from a stack of +them. No figure is given for how many name it, and the trap is worth recording so one is not added +back: a `grep -c` over pytest output counts LINES rather than failures and `--tb=line` prints each +failure twice, so such a count overstates by 2x, while truncation moves it the other way (7 under the +default display, 31 under `--tb=long -vv` on the same run). A number that changes with display +options is not a fact about the code. Such an edit reaching `main` WOULD be a repo-wide merge outage — no verdict could be posted and `review-verdict/h10` is required — but the guard is not the only thing standing in the way, and saying so would overstate it: the writer suite reddens too, in the ADVISORY `script-tests` job (the required contexts, measured 2026-08-30, are `Build & test`, `EF migration integrity` and -`review-verdict/h10`). What alignment buys is ONE named guard failure pointing at the workflow, instead of half the suite reddening under unrelated +`review-verdict/h10`). What alignment buys is ONE named guard failure pointing at the workflow, ALONGSIDE the half-suite of unrelated failures. The route by which the outage is actually reachable is the snapshot boundary below — a verdict posted from a checkout predating the edit — which is why the two paragraphs belong together. diff --git a/scripts/check-review-verdict.sh b/scripts/check-review-verdict.sh index 4893b84da..090988281 100755 --- a/scripts/check-review-verdict.sh +++ b/scripts/check-review-verdict.sh @@ -84,11 +84,19 @@ esac # 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. +# THE GATE IS THE SENTINEL, NOT THIS `if`. The fail-open this guards against is measured and real: +# with the negative list written as a scalar and `.*` in the positive list, an explicit +# `BLOCKED @ ` classified `positive`, exit 0, with only a stderr line the calling hook +# discards. +# +# THE MECHANISM ORIGINALLY GIVEN HERE WAS WRONG and is corrected rather than repeated (ersatztv#845). +# It said an unbound-variable error inside a function aborts the function and NOT the script, so an +# `if ! validate` form takes neither branch and execution continues past the gate. Measured on bash +# 3.2.57 and 5.3.15, and with `set -u` ALONE as well as `set -eu` — which matters here, since this +# reader deliberately runs without `set -e` — the shell EXITS outright: no branch runs and nothing +# after the gate runs. That reading holds only inside a command substitution, which is not this +# shape. `etv_verdict_alternation` refusing without the sentinel is what actually holds, and what it +# covers is the caller that never runs validation at all. _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" diff --git a/scripts/lib/h10-reviewers.sh b/scripts/lib/h10-reviewers.sh index 4650d863c..949aca9a1 100644 --- a/scripts/lib/h10-reviewers.sh +++ b/scripts/lib/h10-reviewers.sh @@ -40,15 +40,19 @@ # That guard used to scope itself to the classify step's `run:` body, which is NOT the same tree this # file walks: a second anchored assignment in any other `run:` block left it green while making this # refuse every verdict. Measured before the fix: the guard green, and about half the writer suite red. -# BE PRECISE ABOUT WHAT WAS MISSING, because the obvious phrasing is wrong: those failures DO carry -# this library's refusal, which names the count and the variable — 14 of 36 did. What was missing is -# a failure that POINTS AT THE WORKFLOW: the reds surface under unrelated test names -# (`test_positive_verdict_posts_success_on_the_resolved_head`, …), so the cause has to be inferred -# from a stack of them. Aligning the populations buys ONE named guard failure instead. Counts are -# avoided elsewhere in this comment because they drift as tests are added. +# BE PRECISE ABOUT WHAT WAS MISSING, because the obvious phrasing is wrong twice over. Those failures +# DO carry this library's refusal, which names the count and the variable. And alignment does not +# REPLACE them — it ADDS a named guard failure alongside them. What was missing is a failure that +# POINTS AT THE WORKFLOW: the reds surface under unrelated test names +# (`test_positive_verdict_posts_success_on_the_resolved_head`, …), so the cause had to be inferred +# from a stack of them. # -# Stated as "about half" rather than "the whole suite" for the same reason — 36 of 75 at the time of -# writing, so "whole" would be false. +# NO FIGURE IS GIVEN FOR HOW MANY NAME IT, deliberately, and the trap is worth stating so nobody +# adds one back. A `grep -c` over pytest output counts LINES, not failures, and `--tb=line` prints +# each failure twice — so that count overstates by 2x. Truncation moves it again: the same run shows +# 7 under the default display and 31 under `--tb=long -vv`. A figure that changes with display +# options is not a fact about the code. "About half" survives because it is coarse enough to be +# robust. # # BE EXACT ABOUT THE CONSEQUENCE, because the obvious phrasing overstates it. Such an edit landing on # `main` WOULD be a repo-wide merge outage — no verdict could be posted, and `review-verdict/h10` is diff --git a/scripts/lib/review-verdict-vocabulary.sh b/scripts/lib/review-verdict-vocabulary.sh index e8e77f409..9d085dd1f 100644 --- a/scripts/lib/review-verdict-vocabulary.sh +++ b/scripts/lib/review-verdict-vocabulary.sh @@ -77,10 +77,9 @@ ETV_VERDICT_NEGATIVE_WORDS=(blocked not-mergeable) # validation in the first place: the words are unobtainable unless it ran all the way to its final # line, so a derived view cannot be reached by a path that simply did not call the validator. # -# The same refuted mechanism still appears in `release.verdict-vocabulary-shared` and in -# `test_review_verdict_vocabulary.py`; correcting a decision record is its own change, tracked as -# ersatztv#886. The fail-open that record documents is real and is not in question — only the -# mechanism it attributes. +# The same refuted mechanism still stands in `release.verdict-vocabulary-shared`; correcting a +# decision record is its own change, tracked as ersatztv#886. The fail-open that record documents is +# real and is not in question — only the mechanism it attributes. ETV_VERDICT_VOCABULARY_OK=0 # --- Validation. ------------------------------------------------------------------------------- diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index de1cd3625..37e5f9c42 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -2388,14 +2388,16 @@ def _h10_reviewers_literal(): # classify step therefore does not walk the same tree as its subject: a second anchored # assignment in ANY OTHER `run:` block is invisible here while making the writer refuse every # verdict. Measured on this file: injecting one into the last step left this test green and - # reddened about half the writer suite. Those failures DO carry the library's refusal, which - # names the count and the variable (14 of 36 did) — what they do NOT do is point at the workflow, - # since they surface under unrelated test names and the cause must be inferred from a stack of - # them. "About half" rather than "the whole suite" because 36 of 75 is not all of it. Such an edit + # reddened about half the writer suite (36 of 75). Those failures DO carry the library's + # refusal, which names the count and the variable — what they do NOT do is point at the + # workflow, since they surface under unrelated test names. No figure is given for how many name + # it: `grep -c` over pytest output counts LINES rather than failures, and `--tb=line` prints each + # failure twice, so such a count overstates by 2x; truncation moves it the other way. Such an edit # reaching `main` would be a repo-wide merge outage — no verdict could be posted and # `review-verdict/h10` is required — but the honest claim for THIS guard is narrower: the writer # suite already reddens, in the ADVISORY `script-tests` job, so what aligning the populations - # buys is ONE named guard failure pointing at the workflow, rather than half the suite reddening. + # buys is ONE named guard failure pointing at the workflow, ALONGSIDE those unrelated reds — + # it adds a diagnosis, it does not replace them. # The classify step remains what the GATE runs, and # `_classify_step()` is still the right scope for every other assertion about that body. body = WORKFLOW.read_text() diff --git a/scripts/tests/test_review_verdict_vocabulary.py b/scripts/tests/test_review_verdict_vocabulary.py index 731cfe598..badd4800d 100644 --- a/scripts/tests/test_review_verdict_vocabulary.py +++ b/scripts/tests/test_review_verdict_vocabulary.py @@ -318,18 +318,23 @@ def test_the_write_side_maps_the_shipped_words_onto_the_same_states(sandbox, tok assert sandbox.posted_status_state() == state -# --- A control-flow gate can be SKIPPED; the sentinel is what actually holds. -------------------- +# --- The sentinel, not the `if`, is what 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 fail-open is measured and real: with `.*` in the positive list and the negative list written as +# a scalar, unvalidated words reached the regex and an explicit `BLOCKED @ ` classified +# `positive`, exit 0. The calling hook discards stderr, so the only signal was invisible. +# +# THE MECHANISM ORIGINALLY RECORDED FOR IT WAS WRONG, corrected in ersatztv#845 and restated here +# rather than repeated: it held that under `set -u` an unbound-variable error inside a FUNCTION +# aborts the function but not the script, so an `if ! validate` form takes NEITHER branch and the +# `exit 2` never runs. Measured on bash 3.2.57 and 5.3.15, under `set -u` ALONE as well as `set -eu` +# — the case that matters, since `check-review-verdict.sh` deliberately runs without `set -e` — the +# shell EXITS outright, so nothing continues past the gate. That reading holds only inside a command +# substitution. # # 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. +# `ETV_VERDICT_VOCABULARY_OK=1` on its last line, and the derived views refuse without it. What that +# covers is the caller that never runs validation at all. These tests pin the malformation classes. def test_a_SCALAR_word_list_cannot_reach_the_regex_unvalidated(sandbox): -- 2.47.3 From f7d6c737f1cb508eb28d84a9722a97f42bbc37c9 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 30 Aug 2026 03:21:56 +0200 Subject: [PATCH 15/17] fix(845): the note warning that these counts are unreliable cited a count I could not reproduce MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-caught while verifying my own correction, which is the point at which it should have been caught the previous three times. The note added last commit argues that no figure should be given for how many failures name the second assignment, because such counts are artifacts of pytest display flags. It then offered "7 under the default display and 31 under `--tb=long -vv`" as evidence. The 7 is reproducible. The 31 was taken from a review report and never re-measured; counting the same way against `--tb=long -vv` gives 62, so either 31 is a different counting or it is wrong, and I cannot tell which. Quoting a number from a report instead of reproducing it is the exact failure the note was written to prevent, committed inside the note. Every figure is gone from all three sites. What survives is the MECHANISM of the trap — a `grep -c` counts lines and `--tb=line` prints each matching failure twice, so it overstates; raising the traceback detail moves it the other way — plus the qualitative "about half the suite reddens", which is coarse enough to be robust. Reproducing any exact figure needs per-failure parsing rather than grepping, and that is now what the note says instead of demonstrating it with numbers. Also dropped "36 of 75" from the guard comment. It reproduces today, but a note whose argument is that counts drift as tests are added should not carry one. No code changed. Full suite 1278 passed / 2 skipped; catalog in sync; decisions-validate OK. fixes #845 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW --- docs/decisions/records/ci/exemption-provenance.md | 7 ++++--- scripts/lib/h10-reviewers.sh | 9 +++++---- scripts/tests/test_pr_changed_files.py | 7 ++++--- 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/docs/decisions/records/ci/exemption-provenance.md b/docs/decisions/records/ci/exemption-provenance.md index c4c01dd8d..61355a7e1 100644 --- a/docs/decisions/records/ci/exemption-provenance.md +++ b/docs/decisions/records/ci/exemption-provenance.md @@ -259,9 +259,10 @@ guard failure rather than replacing them. What was missing is a failure POINTING since the reds surface under unrelated test names and the cause had to be inferred from a stack of them. No figure is given for how many name it, and the trap is worth recording so one is not added back: a `grep -c` over pytest output counts LINES rather than failures and `--tb=line` prints each -failure twice, so such a count overstates by 2x, while truncation moves it the other way (7 under the -default display, 31 under `--tb=long -vv` on the same run). A number that changes with display -options is not a fact about the code. Such an edit reaching `main` WOULD be a repo-wide merge outage — no verdict could be +matching failure twice, so such a count overstates, while raising the traceback detail moves it the +other way by making more failures show the refusal text. Any figure is therefore a property of the +pytest flags that produced it rather than of the code, and reproducing one means parsing per-failure +sections instead of grepping lines. Such an edit reaching `main` WOULD be a repo-wide merge outage — no verdict could be posted and `review-verdict/h10` is required — but the guard is not the only thing standing in the way, and saying so would overstate it: the writer suite reddens too, in the ADVISORY `script-tests` job (the required contexts, measured 2026-08-30, are `Build & test`, `EF migration integrity` and diff --git a/scripts/lib/h10-reviewers.sh b/scripts/lib/h10-reviewers.sh index 949aca9a1..cc5a88b3c 100644 --- a/scripts/lib/h10-reviewers.sh +++ b/scripts/lib/h10-reviewers.sh @@ -49,10 +49,11 @@ # # NO FIGURE IS GIVEN FOR HOW MANY NAME IT, deliberately, and the trap is worth stating so nobody # adds one back. A `grep -c` over pytest output counts LINES, not failures, and `--tb=line` prints -# each failure twice — so that count overstates by 2x. Truncation moves it again: the same run shows -# 7 under the default display and 31 under `--tb=long -vv`. A figure that changes with display -# options is not a fact about the code. "About half" survives because it is coarse enough to be -# robust. +# each matching failure twice — so that count overstates. Truncation moves it the other way: raising +# the traceback detail makes many more of the failures show the refusal text. Any figure here is +# therefore a property of the pytest flags used to produce it, not of the code, and reproducing one +# requires parsing per-failure sections rather than grepping lines. "About half the suite reddens" +# survives because it is coarse enough to be robust to all of that. # # BE EXACT ABOUT THE CONSEQUENCE, because the obvious phrasing overstates it. Such an edit landing on # `main` WOULD be a repo-wide merge outage — no verdict could be posted, and `review-verdict/h10` is diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index 37e5f9c42..0a26e52c8 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -2388,11 +2388,12 @@ def _h10_reviewers_literal(): # classify step therefore does not walk the same tree as its subject: a second anchored # assignment in ANY OTHER `run:` block is invisible here while making the writer refuse every # verdict. Measured on this file: injecting one into the last step left this test green and - # reddened about half the writer suite (36 of 75). Those failures DO carry the library's + # reddened about half the writer suite. Those failures DO carry the library's # refusal, which names the count and the variable — what they do NOT do is point at the # workflow, since they surface under unrelated test names. No figure is given for how many name - # it: `grep -c` over pytest output counts LINES rather than failures, and `--tb=line` prints each - # failure twice, so such a count overstates by 2x; truncation moves it the other way. Such an edit + # it: `grep -c` over pytest output counts LINES rather than failures, and `--tb=line` prints + # each matching failure twice, so such a count overstates; raising the traceback detail moves it + # the other way. Any figure is a property of the pytest flags, not of the code. Such an edit # reaching `main` would be a repo-wide merge outage — no verdict could be posted and # `review-verdict/h10` is required — but the honest claim for THIS guard is narrower: the writer # suite already reddens, in the ADVISORY `script-tests` job, so what aligning the populations -- 2.47.3 From dc09eceee92ebda19c5c3d5cada89a67f0388045 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 30 Aug 2026 03:38:47 +0200 Subject: [PATCH 16/17] =?UTF-8?q?fix(845):=20WITHDRAW=20the=20`set=20-u`?= =?UTF-8?q?=20retraction=20=E2=80=94=20the=20original=20claim=20was=20righ?= =?UTF-8?q?t=20and=20my=20probe=20used=20the=20wrong=20construct?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eighth cold review. It found that the correction shipped over the last two commits inverted a TRUE statement into a false one, in live merge-gate code. Verified independently before acting, and the reviewer is right. WHAT I MEASURED WRONG. My probe used a PLAIN unset variable reference (`: "$UNSET"`) and I generalised the result to `${#arr[@]}`, which is the expansion the validator actually uses. Those shapes do not behave alike. Measured on bash 3.2.57 and 5.3.15, `if ! f` form, function aborting on each shape: shape set -u set -eu plain $UNSET rc=127 exits rc=1 exits unset ${#UNSET_ARR[@]} rc=0 CONTINUES rc=1 exits scalar ${#SCALAR[@]} rc=0 CONTINUES rc=1 exits For both `${#...[@]}` rows under `set -u` alone: continued-past-gate=1, a-branch-ran=0. So the wording on `main` was correct as written — the abort takes neither branch of `if ! validate` and execution continues past the gate — for exactly the shape the validator uses and exactly the flag set its consumer runs: `check-review-verdict.sh` is `set -uo pipefail`, deliberately without `-e`. The repo's own `test_MUTATION_with_the_array_assertion_disarmed_the_SENTINEL_still_refuses` is the end-to-end proof: it asserts a message only emitted AFTER the gate, and it passes. The concrete harm of the version being withdrawn: it had `review-verdict-vocabulary.sh` saying "there is no skip to defend against" — an invitation to delete the sentinel and reopen the measured fail-open that file exists to close. WITHDRAWN WHOLESALE rather than patched a third time, per the budget-3-rounds-then-withdraw rule this repo records for string/prose predicates. `scripts/lib/review-verdict-vocabulary.sh` and `scripts/check-review-verdict.sh` are byte-identical to `main` again; the `docs/ci-cd.md` paragraph and the `test_review_verdict_vocabulary.py` comment are restored verbatim. Kept from those commits: the `docs/ci-cd.md` input-set fix (and its wrap repair), which is unrelated and sound. `scripts/lib/h10-reviewers.sh` keeps a note on the topic, now correctly scoped by BOTH flag set and expansion shape: the route is real for `${#arr[@]}` under `set -u` without `-e`, which is the vocabulary library's consumer, and is not reachable from `post-review-verdict.sh`, which runs `set -euo pipefail` where the shell exits on every shape. So the sentinel here rests on the data-dependency ground alone. ersatztv#886 is CLOSED as invalid — it asked for a record to be "corrected" that is right — with the measurement recorded on it. CORRECTION to the two previous commit messages: their claims that the mechanism was refuted, and that the retracted wording was "gone repo-wide" for code/tests/docs, are both withdrawn. Reusable: a runtime claim needs a probe that exercises the SAME CONSTRUCT, not merely the same flags. `set -u` is not one behaviour — it is one per expansion shape. Full suite 1278 passed / 2 skipped; catalog in sync; decisions-validate OK. fixes #845 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW --- docs/ci-cd.md | 21 ++++++-------- scripts/check-review-verdict.sh | 18 ++++-------- scripts/lib/h10-reviewers.sh | 13 +++++---- scripts/lib/review-verdict-vocabulary.sh | 28 ++++--------------- .../tests/test_review_verdict_vocabulary.py | 23 ++++++--------- 5 files changed, 34 insertions(+), 69 deletions(-) diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 90ac02714..6009f6fb0 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -1260,8 +1260,9 @@ input set spans AT LEAST three top-level directories. `test_post_review_verdict. `.gitea/workflows/review-verdict.yml`, because the writer derives the H10 allow-list from it and editing that literal changes the suite's outcome. So a `scripts/**` filter would silently miss a `.claude/hooks/**` or `.gitea/workflows/**` edit. The hedge is deliberate: the corpus tests read -`docs/` too, so the real figure is higher, and an exact count here is the kind that goes stale. The reason is the input set, not the cost: the suite was ~10s when that was -decided and is now ~4.5 min, and it would still be wrong to filter on `scripts/**`. +`docs/` too, so the real figure is higher, and an exact count here is the kind that goes stale. +The reason is the input set, not the cost: the suite was ~10s when that was decided and is now +minutes, and it would still be wrong to filter on `scripts/**`. **It also lints (ersatztv#780).** Early in the job it installs a **pinned** `ruff==0.12.11` and runs `ruff check` and `ruff format --check` against the repo-root `ruff.toml`. Five things are deliberate: @@ -1515,17 +1516,11 @@ nothing. Words are interpolated into a regex, so a `.*` in the positive list wou 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 first version of #788 shipped the hole. The fail-open is -measured: with the negative list written as a scalar and `.*` in the positive list, unvalidated words -reached the regex and an explicit `BLOCKED @ head` classified `positive`, exit 0. - -The MECHANISM first recorded for it was wrong, corrected in ersatztv#845 and stated here so it is not -re-derived: it held that under `set -u` an unbound-variable error inside a function aborts the -function but not the script, so an `if ! validate` form takes neither branch and the `exit 2` is -skipped. Measured on bash 3.2.57 and 5.3.15, under `set -u` alone as well as `set -eu` — the -distinction that matters, since the reader deliberately runs without `set -e` — the shell EXITS -outright, so nothing continues past the gate. That reading holds only inside a command substitution. -What the sentinel actually covers is the caller that never runs validation in the first place. Validation now sets a sentinel on its last line and the derived views +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. diff --git a/scripts/check-review-verdict.sh b/scripts/check-review-verdict.sh index 090988281..4893b84da 100755 --- a/scripts/check-review-verdict.sh +++ b/scripts/check-review-verdict.sh @@ -84,19 +84,11 @@ esac # 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`. The fail-open this guards against is measured and real: -# with the negative list written as a scalar and `.*` in the positive list, an explicit -# `BLOCKED @ ` classified `positive`, exit 0, with only a stderr line the calling hook -# discards. -# -# THE MECHANISM ORIGINALLY GIVEN HERE WAS WRONG and is corrected rather than repeated (ersatztv#845). -# It said an unbound-variable error inside a function aborts the function and NOT the script, so an -# `if ! validate` form takes neither branch and execution continues past the gate. Measured on bash -# 3.2.57 and 5.3.15, and with `set -u` ALONE as well as `set -eu` — which matters here, since this -# reader deliberately runs without `set -e` — the shell EXITS outright: no branch runs and nothing -# after the gate runs. That reading holds only inside a command substitution, which is not this -# shape. `etv_verdict_alternation` refusing without the sentinel is what actually holds, and what it -# covers is the caller that never runs validation at all. +# 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" diff --git a/scripts/lib/h10-reviewers.sh b/scripts/lib/h10-reviewers.sh index cc5a88b3c..9f1870ed5 100644 --- a/scripts/lib/h10-reviewers.sh +++ b/scripts/lib/h10-reviewers.sh @@ -107,12 +107,13 @@ unset _etv_h10_root # above, and what `etv_h10_reviewers_contains` returning 2 rests on. So the sentinel earns its place # on that ground alone. # -# WHAT THIS DOES NOT REST ON, stated because the claim was inherited and is wrong. The vocabulary -# library argues that under `set -u` an unbound-variable error inside a function "aborts the function -# but not the script", so an `if ! f` gate takes NEITHER branch. Measured here (bash 5.3 and bash -# 3.2, `set -euo pipefail`): the shell EXITS outright, in both the `if ! f` shape and the -# `f; rc=$?` shape. So that particular skip-past-the-gate route does not exist, and no reasoning -# below depends on it. +# THE SKIP-PAST-THE-GATE ROUTE IS REAL BUT NOT REACHABLE HERE, which is why the sentinel is kept +# without repeating that rationale. `review-verdict-vocabulary.sh` documents it: under `set -u` and +# WITHOUT `set -e`, a `${#arr[@]}` on an unset name or a scalar aborts the FUNCTION only, so an +# `if ! validate` gate takes neither branch and execution continues past it — measured on bash 3.2.57 +# and 5.3.15. That is its consumer's situation (`check-review-verdict.sh` runs `set -uo pipefail`). +# It is NOT this file's: `post-review-verdict.sh` runs `set -euo pipefail`, where the shell exits on +# any of those shapes. So the sentinel here earns its place on the data-dependency ground alone. ETV_H10_REVIEWERS_OK=0 ETV_H10_REVIEWERS='' diff --git a/scripts/lib/review-verdict-vocabulary.sh b/scripts/lib/review-verdict-vocabulary.sh index 9d085dd1f..dc12aa2d0 100644 --- a/scripts/lib/review-verdict-vocabulary.sh +++ b/scripts/lib/review-verdict-vocabulary.sh @@ -52,34 +52,16 @@ ETV_VERDICT_NEGATIVE_WORDS=(blocked not-mergeable) # 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`. -# -# THE ORIGINAL REASON GIVEN HERE WAS WRONG, corrected 2026-08-30 (ersatztv#845) because it was being -# copied into new files. It said that under `set -u` an unbound-variable error inside a FUNCTION -# aborts the function but not the script, so an `if ! f` gate takes NEITHER branch and execution -# continues past it. Measured on bash 3.2.57 and 5.3.15 under `set -euo pipefail`: the shell EXITS -# outright, in the `if ! f` form and the `f; rc=$?` form alike — no branch runs, and nothing after -# the gate runs either. That reading holds only inside a COMMAND SUBSTITUTION (`v=$(f) || v=FALLBACK` -# does continue, with the fallback taken), which is not the shape gating this file. -# -# The sentinel is still right, on the ground that survives: it makes validation a DATA dependency the -# derived views require, rather than a control-flow step a caller is trusted to have taken. Measured: +# `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. # -# WHAT THE DATA DEPENDENCY ACTUALLY BUYS, restated because the previous sentence here asserted the -# mechanism retracted above ("a control-flow gate can be skipped by an abort"). Measured on bash -# 3.2.57 and 5.3.15, with `set -u` alone as well as `set -eu` and `set -euo pipefail`: an -# unbound-variable error inside a function EXITS the shell — nothing continues past the gate, so -# there is no skip to defend against. What the dependency covers is the caller that never runs -# validation in the first place: the words are unobtainable unless it ran all the way to its final -# line, so a derived view cannot be reached by a path that simply did not call the validator. -# -# The same refuted mechanism still stands in `release.verdict-vocabulary-shared`; correcting a -# decision record is its own change, tracked as ersatztv#886. The fail-open that record documents is -# real and is not in question — only the mechanism it attributes. +# 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. ------------------------------------------------------------------------------- diff --git a/scripts/tests/test_review_verdict_vocabulary.py b/scripts/tests/test_review_verdict_vocabulary.py index badd4800d..731cfe598 100644 --- a/scripts/tests/test_review_verdict_vocabulary.py +++ b/scripts/tests/test_review_verdict_vocabulary.py @@ -318,23 +318,18 @@ def test_the_write_side_maps_the_shipped_words_onto_the_same_states(sandbox, tok assert sandbox.posted_status_state() == state -# --- The sentinel, not the `if`, is what holds. --------------------------------------------------- +# --- A control-flow gate can be SKIPPED; the sentinel is what actually holds. -------------------- # -# The fail-open is measured and real: with `.*` in the positive list and the negative list written as -# a scalar, unvalidated words reached the regex and an explicit `BLOCKED @ ` classified -# `positive`, exit 0. The calling hook discards stderr, so the only signal was invisible. -# -# THE MECHANISM ORIGINALLY RECORDED FOR IT WAS WRONG, corrected in ersatztv#845 and restated here -# rather than repeated: it held that under `set -u` an unbound-variable error inside a FUNCTION -# aborts the function but not the script, so an `if ! validate` form takes NEITHER branch and the -# `exit 2` never runs. Measured on bash 3.2.57 and 5.3.15, under `set -u` ALONE as well as `set -eu` -# — the case that matters, since `check-review-verdict.sh` deliberately runs without `set -e` — the -# shell EXITS outright, so nothing continues past the gate. That reading holds only inside a command -# substitution. +# 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. What that -# covers is the caller that never runs validation at all. These tests pin the malformation classes. +# `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): -- 2.47.3 From 65a36f286761b341844a7566e440828fa1362a4f Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 30 Aug 2026 04:11:09 +0200 Subject: [PATCH 17/17] docs(845): the canonical record this PR's own doc rule points at was left behind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 9 returned MERGEABLE with four LOW/NIT observations; these are the two that are actionable in the tree. `docs/decisions/records/ci/script-tests-job.md` still enumerated the `script-tests` input set as TWO top-level directories after this branch raised both derived copies (`docs/ci-cd.md` and the comment in `pr-checks.yml`) to three. Nothing was contradicted — "spans more than `scripts/`" is a true lower bound — but CLAUDE.md says convention lookups resolve at the record, and the `.gitea/workflows/**` member was missing from exactly the place a reader is told to look. This branch created that gap, so it closes it. The enumeration is now explicitly a lower bound rather than a census, since the corpus tests read `docs/` as well, and the stale present-tense "~10s" is reframed as the figure at decision time rather than a current one. `scripts/lib/h10-reviewers.sh`: a colon attributed the `-e` scoping and the bash-version measurement to `review-verdict-vocabulary.sh`, which documents neither. That library documents the MECHANISM; the scoping is stated and measured here. Reworded. The two NITs not acted on, and why: the "CORRECTION to the two previous commit messages" in `c03717f72` actually spans three commits, and a `rc=127` cited there is invocation-dependent (127 under `bash -c`, 1 from a script file). Both are commit-message-only and immutable. The same figure appeared in the ersatztv#886 closing comment, which IS editable — it now reports the stable outcome (which arm ran, whether execution continued) instead of an exit code, with the invocation dependence stated. Full suite 1278 passed / 2 skipped; catalog in sync; decisions-validate OK; doc-narrative clean. fixes #845 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW --- docs/decisions/records/ci/script-tests-job.md | 11 ++++++++--- scripts/lib/h10-reviewers.sh | 6 +++--- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/docs/decisions/records/ci/script-tests-job.md b/docs/decisions/records/ci/script-tests-job.md index eb81d6b07..ba8360c6c 100644 --- a/docs/decisions/records/ci/script-tests-job.md +++ b/docs/decisions/records/ci/script-tests-job.md @@ -34,9 +34,14 @@ sibling job carries an ignore-rule. Same question, opposite answers, for stated **Unconditional, not path-filtered.** The suite's real input set spans more than `scripts/`: `test_post_review_verdict.py` and `test_merge_consent_exemption.py` execute the actual -`scripts/post-review-verdict.sh` and `.claude/hooks/pretooluse-merge-consent.sh`. A `scripts/**` -filter would silently miss a `.claude/hooks/**` edit — and at ~10s a filter buys nothing but a -drift surface. Default checkout depth is sufficient: every `git` call in the suite runs against a +`scripts/post-review-verdict.sh` and `.claude/hooks/pretooluse-merge-consent.sh`, and since #845 the +former also READS `.gitea/workflows/review-verdict.yml` — the verdict writer derives the H10 +allow-list from it, so editing that literal changes the suite's outcome. A `scripts/**` filter would +silently miss a `.claude/hooks/**` or `.gitea/workflows/**` edit. The enumeration is a lower bound +rather than a census, deliberately: the corpus tests read `docs/` too, and an exact list here is the +kind that goes stale — which it did, this way, before #845 extended it. The reason is the INPUT SET, +not the cost: a filter was cheap to skip when the suite ran in seconds and is still the wrong axis +now that it runs in minutes. Default checkout depth is sufficient: every `git` call in the suite runs against a temp repo it creates itself, never this repository's history. **Verified by measurement, not a green tick** (the #631 Done-when, and `ci.docs-only-detect-shallow-safe`'s diff --git a/scripts/lib/h10-reviewers.sh b/scripts/lib/h10-reviewers.sh index 9f1870ed5..d35639d55 100644 --- a/scripts/lib/h10-reviewers.sh +++ b/scripts/lib/h10-reviewers.sh @@ -108,10 +108,10 @@ unset _etv_h10_root # on that ground alone. # # THE SKIP-PAST-THE-GATE ROUTE IS REAL BUT NOT REACHABLE HERE, which is why the sentinel is kept -# without repeating that rationale. `review-verdict-vocabulary.sh` documents it: under `set -u` and +# without repeating that rationale. `review-verdict-vocabulary.sh` documents the mechanism; the +# scoping below is stated here and measured here, on bash 3.2.57 and 5.3.15. Under `set -u` and # WITHOUT `set -e`, a `${#arr[@]}` on an unset name or a scalar aborts the FUNCTION only, so an -# `if ! validate` gate takes neither branch and execution continues past it — measured on bash 3.2.57 -# and 5.3.15. That is its consumer's situation (`check-review-verdict.sh` runs `set -uo pipefail`). +# `if ! validate` gate takes neither branch and execution continues past it. That is its consumer's situation (`check-review-verdict.sh` runs `set -uo pipefail`). # It is NOT this file's: `post-review-verdict.sh` runs `set -euo pipefail`, where the shell exits on # any of those shapes. So the sentinel here earns its place on the data-dependency ground alone. ETV_H10_REVIEWERS_OK=0 -- 2.47.3