diff --git a/.claude/hooks/pretooluse-merge-consent.sh b/.claude/hooks/pretooluse-merge-consent.sh index b0c03c9ac..6487efeae 100755 --- a/.claude/hooks/pretooluse-merge-consent.sh +++ b/.claude/hooks/pretooluse-merge-consent.sh @@ -103,8 +103,61 @@ while [ "$page" -le 40 ]; do # the enumeration from a partial list. Require every row to carry a non-empty string `filename` # (an empty array is still valid — that is a genuine end-of-pagination). This also rejects arrays # of scalars, which would otherwise make the `.filename` extraction below fail under `set -e`. + # + # An EMPTY body is rejected EXPLICITLY here rather than left to jq's exit status, because that + # status is not portable: `jq -e` over empty input exits 4 on jq >= 1.7 but **0 on jq 1.6** + # (verified against both binaries — ersatztv#631). Relying on it made this guard fail OPEN on any + # host with the older jq, including the CI runner, which ships jq 1.6. The chain: a transport + # failure makes `gq` return empty, the jq guard wrongly passes, `n` is empty so `[ "$n" -lt 50 ]` + # errors into false, the loop walks PAST the failed page, the NEXT page legitimately returns `[]`, + # and `files_complete=yes` is set over a PARTIAL list — exempting a PR whose unread pages may be + # pure code. That is the very defect the paragraph above describes, reintroduced one layer down. + if [ -z "${raw//[[:space:]]/}" ]; then + files_complete=no; break + fi + # + # CR/LF in a path is REJECTED outright (ersatztv#643 review). `chunk` below flattens paths into + # newline-delimited text, so a filename containing a newline splits into TWO lines that are each + # matched against the allow-list separately: `"safe.md\ndocs/Program.cs"` yields `safe.md` and + # `docs/Program.cs`, both of which pass, while the actual single path ends in `.cs`. Git permits + # newlines in filenames, so this is reachable, and it was reproduced against this hook. Failing + # closed on control characters is the cheap fix; no decision/docs path ever contains one. + # + # VALIDATE EVERY FIELD THE EXTRACTION BELOW CONSUMES. `chunk` emits `(.previous_filename // + # empty)` for EVERY row regardless of `.status`, so validating that field only on `renamed` rows + # left a hole one predicate wide: a row with `status: "modified"` (or Gitea's distinct `copied`) + # carrying a newline in `previous_filename` was reproducibly exempted. The rule this encodes: + # the validation domain must match the CONSUMPTION domain, not the domain the field is + # semantically "supposed to" appear in. The `renamed` => REQUIRED clause is kept on top of the + # unconditional if-present check. + # + # `..` is rejected for the same reason: the allow-list anchors `^docs/`, so + # `docs/../ErsatzTV/Program.cs` matches it. Git will not produce such a path, but this guard's + # whole job is to fail closed on unexpected 2xx shapes rather than to assume a well-behaved peer. + # + # `.status` is checked against a CLOSED set, verified against live Gitea 1.25.4 output: + # added|deleted|changed|renamed|copied. Without it, the `renamed => previous_filename REQUIRED` + # clause could be dodged by any other value — `"Renamed"` with a capital R, or an absent status — + # letting a `git mv ErsatzTV/Program.cs -> docs/a.md` drop its source path and read as docs-only. + # An unknown status now fails closed rather than silently taking the `else true` branch. + # + # `modified` is accepted ALONGSIDE `changed` deliberately. Live Gitea 1.25.4 emits `changed`, but + # a closed allow-list built from the wrong vocabulary is a worse failure than the hole it closes: + # it would gate every genuine docs-only PR, on every version that spells it differently. The + # security property here is "reject values we do not recognise", not "enumerate one version + # exactly", so the set errs toward accepting plausible synonyms. if ! printf '%s' "$raw" \ - | jq -e 'type == "array" and all(.[]; (.filename | type == "string" and length > 0) and (if .status == "renamed" then (.previous_filename | type == "string" and length > 0) else true end))' \ + | jq -e 'def ok: type == "string" and length > 0 + and (test("[\\r\\n]") | not) + and (split("/") | index("..") | not); + type == "array" and all(.[]; + (.filename | ok) + and (.previous_filename == null or (.previous_filename | ok)) + and ((.status // "") as $s | ($s | type) == "string" + and (["added","deleted","changed","modified","renamed","copied"] | index($s)) != null) + and (if .status == "renamed" + then (.previous_filename | type == "string" and length > 0) + else true end))' \ >/dev/null 2>&1; then files_complete=no; break fi @@ -115,10 +168,26 @@ while [ "$page" -le 40 ]; do n=$(printf '%s' "$raw" | jq -r 'length') chunk=$(printf '%s' "$raw" | jq -r '.[] | (.filename // empty), (.previous_filename // empty)') [ -n "$chunk" ] && files=$(printf '%s\n%s' "$files" "$chunk") - if [ "$n" -lt 50 ]; then files_complete=yes; break; fi + # Terminate ONLY on an explicitly validated EMPTY page — never on a merely SHORT one + # (ersatztv#643 review). "Fewer than 50 rows means last page" assumes the server's page size is + # the 50 we asked for, but Gitea caps `limit` at the server-wide `MAX_RESPONSE_ITEMS` (default 50, + # configurable) and is free to return fewer. A 30-row page followed by a page of code would set + # files_complete=yes over a PARTIAL list — the same fail-open, reached without any transport error. + # Costs one extra request per enumeration; the `page <= 40` cap still fails closed. + if [ "$n" -eq 0 ]; then files_complete=yes; break; fi page=$((page + 1)) done files=$(printf '%s\n' "$files" | grep -v '^$' || true) +# Bind the enumeration to ONE head (ersatztv#643 review). Paging is several round-trips; a +# force-push between them means page 1 came from head A and page 2 from head B, so the assembled +# list belongs to no single commit — B's code page can be skipped entirely while B's docs page +# reads as a clean short tail. Re-read the head and refuse the exemption if it moved. +if [ "$files_complete" = yes ]; then + sha_after=$(printf '%s' "$(gq "repos/$owner/$repo/pulls/$pr")" | jq -r '.head.sha // ""' 2>/dev/null || true) + if [ -z "$sha_after" ] || [ "$sha_after" != "$sha" ]; then + files_complete=no + fi +fi if [ "$files_complete" = yes ] && [ -n "$files" ] && ! printf '%s\n' "$files" | grep -qvE '^(docs/|\.claude/|\.husky/|\.gitea/|.*\.md$)'; then # Docs/process-only PR: the Done-when + review-verdict gate doesn't apply — but this exemption is a # file-TYPE bypass, NOT the a+b+c "provably reviewed & ready" proof, so it does NOT auto-grant. It @@ -210,7 +279,11 @@ else # the first page and read as absent — a confusing false deny. The combined endpoint returns # latest-per-context, which is exactly the question being asked. vjson=$(gq "repos/$owner/$repo/commits/$sha/status?limit=100") - if ! printf '%s' "$vjson" | jq -e '.statuses | type == "array"' >/dev/null 2>&1; then + # Same portability point as the file-pagination guard above: do not let jq's empty-input exit + # status decide this. Here the fallthrough happens to land on `vstate=""` -> deny (fail-CLOSED, + # so this was never a hole), but it would have surfaced the wrong message — a "BLOCKED, no + # verdict" deny instead of the "could not read the status" ask this branch exists to give. + if [ -z "${vjson//[[:space:]]/}" ] || ! printf '%s' "$vjson" | jq -e '.statuses | type == "array"' >/dev/null 2>&1; then decide ask "H6/H10 merge gate: could not read the 'review-verdict/h10' status for PR #$pr head ${sha:0:7} (Gitea unreachable or an unexpected response). Confirm the current head is reviewed before scheduling an auto-merge." fi vstate=$(printf '%s' "$vjson" | jq -r '[.statuses[] | select(.context == "review-verdict/h10")] | first | .status // ""') diff --git a/.gitea/workflows/pr-checks.yml b/.gitea/workflows/pr-checks.yml index cee68a461..0fe73e668 100644 --- a/.gitea/workflows/pr-checks.yml +++ b/.gitea/workflows/pr-checks.yml @@ -3,8 +3,9 @@ name: PR Gates # Fast, git-only PR gates split out of docker-build.yml into a dedicated `on: pull_request` # workflow (ersatztv#535) so they are NEVER created on a tag/main push. # -# WHY THIS FILE EXISTS. These three checks are pure `checkout + git diff` gates: they carry no -# `container:`, run on the `small` lane (git-only, 1 GiB; server-management#639), and are PR-only. +# WHY THIS FILE EXISTS. These checks are cheap `checkout + git diff` gates (or, for `script-tests`, +# checkout + pytest): they carry no `container:`, run on the `small` lane (git-only, 1 GiB; +# server-management#639), and are PR-only. # While they lived in docker-build.yml — which also triggers on push to main and on `v*` tags — # Gitea still DISPATCHED them as runner tasks on every such push to evaluate the `if:` skip, because # **Gitea dispatches a job as a runner task even when its `if` skips it** (docs/ci-cd.md -> the @@ -22,10 +23,10 @@ name: PR Gates # # These stay on `runs-on: small` and carry NO CI toolchain image pin, so `ci-image-pin`'s grep of # docker-build.yml still validates the five pin-bearing jobs (test/migrations/functional-e2e/ -# api-docs/format) that remain there. None of these three are required checks — branch protection -# requires only `Build & test (.NET)` and `EF migration integrity` — so relocating them (which -# changes their status-context prefix from "Build ErsatzTV Image / …" to "PR Gates / …") does not -# affect merges. See docs/ci-cd.md -> "PR gates workflow". +# api-docs/format) that remain there. None of these jobs are required checks — branch protection +# requires only `Build & test (.NET)`, `EF migration integrity` and `review-verdict/h10` — so +# relocating them (which changes their status-context prefix from "Build ErsatzTV Image / …" to +# "PR Gates / …") does not affect merges. See docs/ci-cd.md -> "PR gates workflow". on: pull_request: @@ -187,3 +188,65 @@ jobs: run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check - name: Kickoff guard run: bash scripts/check-kickoff-guard.sh + + # FAILS THE RUN on a red (ersatztv#631) — like its sibling gates here it is not (yet) a required + # status check, so it reddens the PR without hard-blocking the merge button; see the header. + # Runs scripts/tests/ — the pytest suite covering the decision-corpus + # parser/validator/catalog builder, the #610 migration-equivalence harness, the merge-consent + # exemption logic and the #622 review-verdict poster. Until #631 NOTHING executed these: no + # workflow and no Husky hook invoked pytest, so the suite guarding our merge-gating machinery was + # local-only and a regression in it was caught only by luck. `decisions-guard` above runs that + # code, but never its tests. + # + # WHY ITS OWN JOB rather than a step inside decisions-guard (which the issue proposed as the + # cheapest home): `ci.decisions-lifecycle-flake` is a STANDING instruction that a lone + # `decisions lifecycle` red is a known infra flake to be ignored — "do not investigate". Folding + # the suite into that job would make a genuine pytest regression present as exactly the red every + # session is told to wave through, which is the same silently-green failure mode #631 exists to + # 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. + script-tests: + name: Script tests (pytest) + runs-on: small + if: github.event_name == 'pull_request' + steps: + - name: Checkout + uses: actions/checkout@v4 + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: '3.x' + # pytest + PyYAML. PyYAML is NOT a contradiction of the dependency-free decisions READ path: + # `decisions_lib._read_frontmatter` is hand-written precisely so validation runs where nothing + # is installed, but the one-shot WRITE path `migrate_decisions_split.py` uses PyYAML by + # design — and `test_migration_equivalence.py` imports that module, so the suite needs it. + # `pytest` and `yaml` are the complete third-party set, established by an AST import scan over + # all of scripts/ rather than by reading the files that seemed relevant: the first cut of this + # job claimed "pure stdlib", passed locally on a machine that happened to have PyYAML, and + # went red in CI on a collection error. + - name: Install test dependencies + run: python3 -m pip install --disable-pip-version-check --quiet pytest pyyaml + # Preflight, not an install (ersatztv#390 removed run-time `apt-get` from CI on purpose). + # test_post_review_verdict.py and test_merge_consent_exemption.py exec the REAL + # post-review-verdict.sh / pretooluse-merge-consent.sh, which shell out to `jq` ~26 times. + # `curl` those tests shim on PATH; `jq` they do NOT. If it were missing, the suite would fail + # as ~20 opaque assertion errors — this turns that into one actionable line. + - name: Preflight external tools + run: | + missing=() + for t in jq git; do command -v "$t" >/dev/null 2>&1 || missing+=("$t"); done + if [ ${#missing[@]} -gt 0 ]; then + echo "::error::script-tests needs these on PATH but they are absent: ${missing[*]}." \ + "The suite execs real shell scripts that use them. Bake them into the runner" \ + "image rather than apt-get installing here (see ersatztv#390)." + exit 1 + fi + echo "Preflight OK: $(jq --version), $(git --version)" + - name: Run scripts/tests + run: PYTHONPATH=. python3 -m pytest scripts/tests -q diff --git a/.gitea/workflows/review-verdict.yml b/.gitea/workflows/review-verdict.yml index 69e66a80f..82f5b7194 100644 --- a/.gitea/workflows/review-verdict.yml +++ b/.gitea/workflows/review-verdict.yml @@ -98,7 +98,14 @@ jobs: # An unreadable/unparseable response must NOT be read as "no verdict exists": fail the # job WITHOUT posting anything, so a transient API error can never overwrite a verdict. statusjson=$(gh "$BASE_URL/repos/$REPO/commits/$SHA/status?limit=100") || statusjson="" - if ! printf '%s' "$statusjson" | jq -e '.statuses | type == "array"' >/dev/null 2>&1; then + # The empty case is checked EXPLICITLY, not left to jq's exit status: `jq -e` over empty + # input exits 4 on jq >= 1.7 but 0 on jq 1.6 (ersatztv#647), and the RUNNER SHIPS 1.6 — + # so on a transient API error this guard passed, `existing` came back "", and the job + # went on to post `pending` (or an exemption `success`) over a possibly-existing human + # verdict. Exactly what the paragraph above says must never happen. The sibling fix in + # .claude/hooks/pretooluse-merge-consent.sh (ersatztv#643) missed this copy: that hook + # runs on a dev Mac with jq 1.8, this workflow runs where the bug is live. + if [ -z "${statusjson//[[:space:]]/}" ] || ! printf '%s' "$statusjson" | jq -e '.statuses | type == "array"' >/dev/null 2>&1; then echo "::error::Could not read existing commit statuses for ${SHA:0:7}. Refusing to post anything rather than risk overwriting an existing verdict." exit 1 fi diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 5ad5f92c2..7db5282d1 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -149,8 +149,12 @@ rather than in `docker-build.yml` — see that section (ersatztv#535). **`small` is git-only, and that is load-bearing (server-management#639).** Everything in the lane is a checkout plus a `git diff`: `decisions-guard`, `ci-image-pin`, -`docs-reminder`. Nothing there runs a compiler or a `docker build`, which is why the lane -can be capped at 1 GiB per job. Route a heavy job here and it will OOM — give it +`docs-reminder` — plus `script-tests`, which is a checkout plus a `pytest` run needing only +`pytest` and `pyyaml` (ersatztv#631; it is NOT stdlib-only — that assumption is what turned the +job red on its first CI run, see below). Nothing there runs a compiler or a `docker build`, which is why the lane +can be capped at 1 GiB per job. The lightweight-Python jobs are the deliberate edge of the +"git-only" rule, not an exception to it: `setup-python` + `pip install pytest` + a suite whose +heaviest allocation is a handful of temp-dir git repos stays far under the cap. Route a heavy job here and it will OOM — give it `ubuntu-latest`, or its own label on `ci-runner`, the only host with no prod workload. **Lane assignment (ersatztv#390).** *Slot counts below are as-of 2026-07-17; the table above is @@ -507,8 +511,8 @@ expensive 787-migration replay is skipped; the service is capped and idle for se `api-docs` and `format` already short-circuit on docs-only changes via their own path detection (no API path / no `.cs` changed → they pass in ~5s), so they needed no change. `docs-reminder`, -`decisions-guard` and `ci-image-pin` keep running on docs-only changes — the first two are *about* -docs and must. +`decisions-guard`, `ci-image-pin` and `script-tests` keep running on docs-only changes — the first +two are *about* docs and must, and `script-tests` is unconditional by design (ersatztv#631). Not in scope: the within-run triple `dotnet build` (ersatztv#398; measured and rejected as build-once — see `docs/decisions.md`). The separate redundancy of running the **whole matrix on a @@ -598,12 +602,60 @@ compiler/docker build), so it doesn't violate the "small is git-only" lane rule. `docs-reminder`, otherwise a seconds-long `git diff` + parse with no dotnet/node setup (`runs-on: small`). +### `script-tests` job (`Script tests (pytest)`, PR-only — in `pr-checks.yml`) + +> Reddens the run on failure, but like the other `pr-checks.yml` gates it is **not** one of the +> three required status checks on `main` (`Build & test (.NET)`, `EF migration integrity`, +> `review-verdict/h10`). Promoting it to required is a branch-protection change, tracked separately. + +Runs the repository's Python test suite: `PYTHONPATH=. python3 -m pytest scripts/tests -q` +(~190 tests at time of writing, ~10s; the suite grows, so treat the figure as indicative). It covers the decision-corpus parser/validator/catalog builder, the ersatztv#610 +migration-equivalence harness, the merge-consent exemption logic and the ersatztv#622 review-verdict +poster. + +**Until ersatztv#631, nothing ran these tests.** No workflow and no Husky hook invoked `pytest`. +`decisions-guard` executes `decisions_validate.py` and `build_decisions_catalog.py` directly — it +exercises that *code* but never its *tests* — and the `test` job is `dotnet test` only. The suite +guarding our merge-gating machinery was therefore local-only, and a test added "for CI enforcement" +was decorative. + +**Why it is its own job, not a step inside `decisions-guard`.** `decisions-guard` is covered by the +standing `ci.decisions-lifecycle-flake` rule: a lone `decisions lifecycle` red is a known infra +flake and sessions are instructed *not to investigate it*. Adding the suite there would make a +genuine pytest regression surface as precisely the red everyone is told to wave through — the same +"reports success while doing nothing" failure mode ersatztv#631 exists to close. A distinct job +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 +`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. At ~10s, a filter buys nothing but drift. + +**Dependencies: `pytest` and `pyyaml`** — the complete third-party set across `scripts/`, established +by an AST import scan rather than by reading the files that looked relevant. PyYAML does **not** +contradict the dependency-free decisions *read* path: `decisions_lib._read_frontmatter` is +hand-written exactly so validation runs where nothing is installed, but the one-shot *write* path +`migrate_decisions_split.py` uses PyYAML by design, and `test_migration_equivalence.py` imports that +module. (The first cut of this job claimed "pure stdlib + pytest", passed locally on a machine that +happened to have PyYAML installed, and went red in CI on a `ModuleNotFoundError` at collection — +which is itself a small demonstration of why the suite needed to run in CI at all.) Like the other +`small`-lane Python jobs it adds `actions/setup-python@v5` first. Checkout is at default depth: every `git` call in the suite runs +against a temp repo it creates itself, never this repository's history. + +A **preflight step** asserts `jq` and `git` are on PATH before running the suite. Those two tests +exec the real shell scripts, which shell out to `jq` ~26 times; the tests shim `curl` on PATH but +not `jq`, so a runner image without it would surface as ~20 opaque assertion failures instead of one +diagnosis. It deliberately **checks** rather than installs — ersatztv#390 removed run-time +`apt-get` from CI; the fix for a genuine miss is to bake the tool into the runner image. + ## PR gates workflow **File:** `.gitea/workflows/pr-checks.yml` — `on: pull_request` only. -The three git-only PR gates — `ci-image-pin`, `docs-reminder`, `decisions-guard` (described above) -— live here, **not** in `docker-build.yml`, and that separation is the fix for **ersatztv#535**. +The four git-only PR gates — `ci-image-pin`, `docs-reminder`, `decisions-guard`, `script-tests` +(all described above) — live here, **not** in `docker-build.yml`, and that separation is the fix +for **ersatztv#535**. **Why they are split out.** All three are pure `checkout + git diff` gates on the `small` lane (no `container:`) and are PR-only (`if: github.event_name == 'pull_request'`). While they lived in diff --git a/docs/decisions/README.md b/docs/decisions/README.md index f24dbcbd2..391a3e14a 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -51,6 +51,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.peak-anon-measurement` | The `test` job's headline memory figure is a sampled high-water mark of cgroup `anon`, produced by `scripts/ci-peak-anon.sh`; `memory.peak` and the end-of-job `anon`/`file` split are kept only as a cache-inflated reference. | 2026-07-19 | [link](records/ci/peak-anon-measurement.md) | | `ci.root-screenshot-guard` | The Husky `pre-commit` hook refuses a staged root-level `*.png` (belt-and-suspenders with the `.gitignore` rule); nested `*.png` real assets are unaffected. | 2026-07-12 | [link](records/ci/root-screenshot-guard.md) | | `ci.runner-placement` | No persistent Roslyn compiler server survives a CI build (`UseSharedCompilation=false` etc., runner env + Dockerfile `ENV`); every `services:` container gets its own explicit `--memory`/`--memory-swap`/`--cpus` cap (it does not inherit the job container's). | 2026-07-17 | [link](records/ci/runner-placement.md) | +| `ci.script-tests-job` | The `scripts/tests/` pytest suite runs on every PR as a dedicated `script-tests` job in `pr-checks.yml` (`runs-on: small`, `setup-python` + `pip install pytest`, `PYTHONPATH=. python3 -m pytest scripts/tests -q`), unconditionally rather than behind a `scripts/**` path filter, and **never as a step inside `decisions-guard`** — a job whose reds a standing rule instructs sessions to ignore must never host a gate whose reds are real. Any new CI gate must be reachable by a failure that is unambiguously attributable to it. | 2026-07-26 | [link](records/ci/script-tests-job.md) | | `ci.small-lane-git-only` | `runs-on: small` is defined by what a job does (git-only), not its usual runtime; the two `docker build` jobs (docker-build.yml, ci-image.yml) move to `ubuntu-latest` because their worst-case memory, not median runtime, was pinning the small lane's per-slot cap. | 2026-07-20 | [link](records/ci/small-lane-git-only.md) | | `ci.ui-e2e-harness` | The UI-interactive E2E flows run as headless Playwright specs (`web/e2e/*.spec.ts`, driven by `scripts/e2e-ui.sh`) in a **second step of the existing advisory `functional-e2e` job**, never their own job; the browser is `chromium-headless-shell` **baked into the CI toolchain image** (`docker/ci/Dockerfile`, `PLAYWRIGHT_VERSION` kept equal to `web/package.json`'s EXACT `@playwright/test` pin), never installed per run; specs are `serial` with `retries: 0` and assert only contracts the curl harness structurally cannot reach. | 2026-07-25 | [link](records/ci/ui-e2e-harness.md) | | `ci.verify-locally-ci-confirms` | Treat the local build/verify/review pass as the decision point and CI as confirmation — don't idle waiting on a run you have no reason to doubt. | 2026-07-21 | [link](records/ci/verify-locally-ci-confirms.md) | diff --git a/docs/decisions/records/api/paging-zero-based.md b/docs/decisions/records/api/paging-zero-based.md index 9c1ec058c..3a11aa676 100644 --- a/docs/decisions/records/api/paging-zero-based.md +++ b/docs/decisions/records/api/paging-zero-based.md @@ -1,6 +1,6 @@ --- key: api.paging-zero-based -title: 2026-07-25 — Paging is 0-based everywhere; every wrapper must say so (#616, #633) +title: '2026-07-25 — Paging is 0-based everywhere; every wrapper must say so (#616, #633)' status: active since: '2026-07-25' supersedes: none diff --git a/docs/decisions/records/ci/script-tests-job.md b/docs/decisions/records/ci/script-tests-job.md new file mode 100644 index 000000000..476816b71 --- /dev/null +++ b/docs/decisions/records/ci/script-tests-job.md @@ -0,0 +1,111 @@ +--- +key: ci.script-tests-job +title: '2026-07-26 — `scripts/tests/` runs in CI as its own `script-tests` job, never inside the flake-covered `decisions-guard` (#631)' +status: active +since: '2026-07-26' +supersedes: none +superseded-by: none +rule: 'The `scripts/tests/` pytest suite runs on every PR as a dedicated `script-tests` job in `pr-checks.yml` (`runs-on: small`, `setup-python` + `pip install pytest`, `PYTHONPATH=. python3 -m pytest scripts/tests -q`), unconditionally rather than behind a `scripts/**` path filter, and **never as a step inside `decisions-guard`** — a job whose reds a standing rule instructs sessions to ignore must never host a gate whose reds are real. Any new CI gate must be reachable by a failure that is unambiguously attributable to it.' +signals: 'scripts/tests never ran in CI, pytest not in any workflow, python test suite local-only, decorative test, decisions-guard runs the code not the tests, script-tests job, small lane pytest, negative control CI goes red · paths: `.gitea/workflows/pr-checks.yml`, `scripts/tests/`, `docs/ci-cd.md` · issues: #631, #610, #621, #622, #542' +mechanics: '`.gitea/workflows/pr-checks.yml` -> `script-tests`; `docs/ci-cd.md` -> "`script-tests` job"' +--- + +Until #631 **nothing executed `scripts/tests/`**. No workflow and no Husky hook invoked `pytest`. +`decisions-guard` runs `decisions_validate.py` and `build_decisions_catalog.py` directly — it +exercises that *code* but never its *tests* — and the `test` job is `dotnet test` only. So the +tests guarding the decision corpus, the #610 migration-equivalence harness, the merge-consent +exemption logic and the #622 review-verdict poster were caught only if someone happened to run +pytest locally. #622's suite was very nearly shipped in the belief that it was enforced. + +**The durable part is where the job does NOT go.** The obvious home — a step inside `decisions-guard`, +which already has `setup-python` and the right lane — is the one place it must not live. +`ci.decisions-lifecycle-flake` is a standing instruction that a lone `decisions lifecycle` red is a +known infra flake and must **not** be investigated. Folding the suite in there would make a genuine +pytest regression present as precisely the red every session is told to wave through: the gate would +be enforced on paper and inert in practice, which is the same "reports success while doing nothing" +family as the #603 `stale-after` field that never fired, the #609 marker that printed `OK` while +doing nothing, and the #621 record that vanishes silently. A gate inherits the credibility of the job +it lives in, so **a job under a standing ignore-rule can host no real gate.** + +This does not conflict with `ci.ui-e2e-harness` ("never their own job"). That record folds UI-E2E +into `functional-e2e` because the specs need an app the job has *already booted* — sharing expensive +setup. Here there is no shared setup to reuse (a checkout plus `pip install pytest pyyaml`), and the +sibling job carries an ignore-rule. Same question, opposite answers, for stated reasons. + +**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 +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 +lesson): a deliberately-failing test was confirmed to exit non-zero locally and to turn the CI job +red on a scratch PR, before the passing state was accepted as meaningful. + +**What running it in CI immediately found.** The first green-on-my-machine run went red twice, and +both reds were real: + +1. `ModuleNotFoundError: No module named 'yaml'` — the suite is not stdlib-only + (`test_migration_equivalence.py` imports `migrate_decisions_split`, whose write path uses + PyYAML). It passed locally only because the author's machine had PyYAML. +2. A **fail-open in the merge-consent gate itself.** `jq -e` over EMPTY input exits 4 on jq ≥ 1.7 + but **0 on jq 1.6** (verified against both binaries), and the docs-only pagination guard leaned + on that exit status to reject a transport failure. On jq 1.6 — which the CI runner ships — the + failed page passed the guard, the loop walked past it, the next page legitimately returned `[]`, + and `files_complete=yes` was set over a PARTIAL list: the docs-only exemption firing over unread + pages that may be pure code. Fixed by rejecting an empty body explicitly instead of inferring it + from jq's exit status. + +The second one is the argument for this record in miniature. It sat in the gate that decides whether +a PR skips the Done-when checks, it was covered by an existing test, and that test could not catch it +on a developer Mac (jq 1.8) — only in CI, where the suite had never run. **Standing rules it leaves behind, all three the same shape — never infer a +CONDITION from a jq exit status or a version-dependent builtin:** +- never infer "empty input" from a jq exit status; check the string (`#643`); +- never infer "parse error" from a jq exit status — `jq empty` is the portable test, because + jq >= 1.7 exits 5 where 1.6 exits 4, and 4 is also "no output" (`#647`); +- never use `contains("\u0000")` — on jq 1.6 the escape truncates to `""` and it matches every + string (`#647`). `explode | index(0)` is version-stable. + +The runner ships **jq 1.6**; a developer Mac ships 1.8.x. Three divergences were found in one day, +so the durable fix is to pin or preflight the version rather than keep patching constructs — +tracked on `#647`. + +An independent cross-family review of that fix then found **two further fail-opens in the same +enumeration, both reachable with no transport error at all** (#643): + +- **A path containing a newline.** The file list is flattened into newline-delimited text before the + allow-list grep, so `"safe.md\ndocs/Program.cs"` splits into two lines that each pass while the + real path ends in `.cs`. Git permits newlines in filenames; it was reproduced against the hook. + Now rejected outright — a docs path never contains a control character. +- **A short page read as the last page.** Gitea caps `limit` at the server-wide `MAX_RESPONSE_ITEMS` + and may return fewer rows than asked for, so `n < 50` does not mean "end of list". Only a + validated EMPTY page may terminate the enumeration. + +Plus a **Medium**: paging is several round-trips, so a force-push between them yields a list +belonging to no single commit. The head sha is now re-read after enumeration and the exemption +refused if it moved. + +A re-review of that fix then found it **incomplete**, and the test for it **vacuous**: `chunk` +consumes `previous_filename` on EVERY row, but the guard validated it only on `renamed` rows, so a +`modified`/`copied` row carrying a newline there was still exempted — while the test meant to cover +that side used a payload the allow-list rejected anyway, so it passed with the guard removed. **The +rule that generalises: validate every field the extraction CONSUMES, not the fields it is +semantically supposed to contain — and a test whose payload fails for an unrelated reason asserts +nothing.** + +**Severity, stated honestly.** The docs-only exemption ends in `decide allow ""` — a *passthrough* +to the normal permission prompt, not an auto-grant (only the satisfied a+b+c path grants). So every +bypass above downgrades a mechanical deny/ask to a human prompt; none of them can produce a silent +self-merge. That is a real weakening of the gate, and worth fixing, but it is not the +"unreviewed code merges itself" scenario an earlier framing of #643 implied. + +**The generalisable lesson is about the SHAPE of this guard, not any one bug.** Every defect here +was an *exhaustiveness* failure in an enumeration whose completeness is load-bearing: each looked +like a complete list and wasn't. When a security decision depends on having seen ALL of something, +the termination condition must be positive and explicit ("the server said empty"), never inferred +from a proxy ("fewer than we asked for", "jq didn't complain"). + +It is not yet a *required* status check — `main` requires only `Build & test (.NET)`, +`EF migration integrity` and `review-verdict/h10`. It reddens the run; promoting it to required is a +branch-protection change left deliberately separate. diff --git a/scripts/check-review-verdict.sh b/scripts/check-review-verdict.sh index 1513e9a2d..2b930afb1 100755 --- a/scripts/check-review-verdict.sh +++ b/scripts/check-review-verdict.sh @@ -82,13 +82,30 @@ comments=$(cat) # scanner and simply match nothing — a malformed payload reading as "no verdict posted" is a # fail-OPEN on a gate whose whole job is to withhold approval. # A body containing a NUL is rejected outright: bash strips NULs in command substitution, so +# NOTE the NUL test is `explode | index(0)`, NOT `contains("\u0000")` (ersatztv#647). On jq 1.6 the +# escape truncates the literal to the EMPTY string, and every string contains "" — so that form +# returns true for ALL input, making this guard reject every comment body as malformed. Verified +# against both binaries: 1.6 says true for "hello", 1.7+ says false. The CI runner ships jq 1.6, so +# the whole verdict classifier was inert there. `explode | index(0)` agrees on both. +# # `Review-verdict: MERGEABLE @ ` would arrive at the matcher as a valid verdict line — # text that is not a verdict silently becoming one. +# PARSE CHECK FIRST, separately, because jq's exit codes are not portable enough to distinguish +# "malformed input" from "valid input, no output" (ersatztv#647): jq >= 1.7 exits 5 on a parse error +# while jq 1.6 exits 4 — the SAME code both versions use for "filter produced no output", which is +# the legitimate empty-comment-list case. So on jq 1.6 the check below could not tell a garbage API +# response from "no comments yet", and silently returned `absent` where it should have raised an +# input error. `jq empty` separates the two on every version: non-zero iff the input does not parse, +# regardless of how much output the filter would produce. +if ! printf '%s' "$comments" | jq empty >/dev/null 2>&1; then + printf 'check-review-verdict: stdin is not valid JSON\n' >&2 + exit 2 +fi encoded=$(printf '%s' "$comments" | jq -ce ' if type != "array" then error("not an array") else .[] end | (.body // "") | if type != "string" then error("non-string body") - elif contains("\u0000") then error("NUL in body") + elif (explode | index(0)) != null then error("NUL in body") else . end' 2>/dev/null) jq_rc=$? # `jq -e` exits 4 when a filter produced NO output — which is exactly the legitimate empty-comment-list diff --git a/scripts/tests/test_merge_consent_exemption.py b/scripts/tests/test_merge_consent_exemption.py index ac6bbd9c4..517ab2abe 100644 --- a/scripts/tests/test_merge_consent_exemption.py +++ b/scripts/tests/test_merge_consent_exemption.py @@ -55,7 +55,16 @@ if "/pulls/" in url and "/files" in url: print(json.dumps(entry)); sys.exit(0) if "/pulls/" in url: - print(json.dumps({"head": {"sha": os.environ["STUB_SHA"]}, "body": "no linked issue here"})) + # Optional: a second head sha served from the Nth PR-object read onward, modelling a + # force-push landing between pagination round-trips. + shas = [os.environ["STUB_SHA"]] + alt = state / "pr_sha_after.txt" + ctr = state / "pr_reads.txt" + nread = int(ctr.read_text()) if ctr.exists() else 0 + ctr.write_text(str(nread + 1)) + if alt.exists() and nread >= 1: + shas = [alt.read_text().strip()] + print(json.dumps({"head": {"sha": shas[0]}, "body": "no linked issue here"})) sys.exit(0) print("{}") @@ -219,10 +228,15 @@ def test_ordinary_row_without_previous_filename_is_still_valid(hook): Requiring it globally would reject every normal modified/added row and make the gate refuse all exemptions — which the 'withholds' tests above could not distinguish from working. + + Every row carries a `status`: since the round-3 hardening an ABSENT status fails closed (it + would otherwise dodge the `renamed => previous_filename REQUIRED` clause), which is asserted by + `test_a_rename_disguised_by_an_unknown_status_is_rejected[None]`. The statusless row this test + used to carry was incidental to what it is actually pinning. """ hook.set_pages([{"filename": "docs/a.md", "status": "modified"}, {"filename": "docs/b.md", "status": "added"}, - {"filename": "docs/c.md"}]) + {"filename": "docs/c.md", "status": "changed"}]) assert hook.exempted() is True @@ -230,3 +244,254 @@ def test_exceeding_max_pages_withholds_the_exemption(hook): """41 full pages: enumeration cannot be proven exhaustive, so no exemption.""" hook.set_pages(*[_rows([f"docs/p{p}f{i}.md" for i in range(50)]) for p in range(41)]) assert hook.exempted() is False + + +# --- #631: the guard must not depend on jq's empty-input exit status --------------------------- +# +# `jq -e` over EMPTY input exits 4 on jq >= 1.7 but 0 on jq 1.6. The pagination guard originally +# leaned on that status to reject a transport failure, so on jq 1.6 the failed page passed the +# guard, the loop walked PAST it, the next page legitimately returned `[]`, and the enumeration +# completed from a PARTIAL list — the docs-only exemption firing over unread pages. +# +# This was invisible for two compounding reasons: the suite never ran in CI at all (#631), and +# `test_transport_failure_mid_pagination_withholds_the_exemption` above only exposes it when the +# host jq happens to be 1.6 — it passes on a developer Mac (1.8.x) with the bug fully present. +# So that test cannot pin this property; this one does, on ANY jq, by putting a shim on PATH that +# reproduces the single 1.6 behaviour and nothing else. + +_JQ16_SHIM = r'''#!/usr/bin/env python3 +"""jq wrapper reproducing exactly one jq-1.6 behaviour: `-e` over EMPTY input exits 0 (not 4). + +Deliberately narrow. The quirk applies ONLY to a `-e` invocation that reads stdin; it must not +touch `jq -n`, which the hook's `decide` uses to build its decision JSON and which legitimately +has empty stdin. An earlier, broader version of this shim swallowed those `-n` calls, so the hook +emitted nothing and every decision read as "passthrough" — the shim manufacturing the very result +the test was trying to disprove. +""" +import os, subprocess, sys + +argv = sys.argv[1:] +uses_null_input = any(a in ("-n", "--null-input") for a in argv) +wants_exit_status = any(a in ("-e", "--exit-status") for a in argv) + +data = b"" if uses_null_input else sys.stdin.buffer.read() +if wants_exit_status and not uses_null_input and not data.strip(): + sys.exit(0) # <-- the jq 1.6 quirk under test + +here = os.path.dirname(os.path.abspath(__file__)) +parts = [p for p in os.environ.get("PATH", "").split(os.pathsep) if os.path.abspath(p or ".") != here] +for d in parts: + cand = os.path.join(d, "jq") + if os.path.isfile(cand) and os.access(cand, os.X_OK): + sys.exit(subprocess.run([cand, *argv], input=data).returncode) +sys.stderr.write("real jq not found\n") +sys.exit(127) +''' + + +@pytest.fixture +def hook_jq16(tmp_path): + """Same harness as `hook`, plus a jq shim emulating jq 1.6's empty-input exit status.""" + bindir = tmp_path / "bin"; bindir.mkdir() + (bindir / "curl").write_text(CURL_SHIM); (bindir / "curl").chmod(0o755) + (bindir / "jq").write_text(_JQ16_SHIM); (bindir / "jq").chmod(0o755) + state = tmp_path / "state"; state.mkdir() + + env = dict(os.environ) + env["PATH"] = f"{bindir}{os.pathsep}{env['PATH']}" + env["STUB_DIR"] = str(state) + env["STUB_SHA"] = SHA + env["ETV_GITEA_TOKEN"] = "stub" + env["ETV_GITEA_URL"] = "http://gitea.example" + env.pop("ETV_GITEA_BASICAUTH", None) + + class Handle: + def set_pages(self, *pages): + (state / "pages.json").write_text(json.dumps(list(pages))) + + def exempted(self): + payload = {"tool_input": {"method": "merge", "owner": "timothy", + "repo": "ersatztv", "pull_number": 42}} + r = subprocess.run(["bash", str(HOOK)], input=json.dumps(payload), + env=env, capture_output=True, text=True) + assert r.returncode == 0, r.stderr + return r.stdout.strip() == "" + + return Handle() + + +def test_jq16_shim_actually_reproduces_the_quirk(hook_jq16, tmp_path): + """Verify the verifier: the shim must really exit 0 on empty and still work otherwise. + + Without this, a shim that silently failed to install would make the test below pass + vacuously — reporting the guard safe on jq 1.6 without ever exercising the quirk.""" + jq = tmp_path / "bin" / "jq" + + # the quirk itself + empty = subprocess.run([str(jq), "-e", "type"], input=b"", capture_output=True) + assert empty.returncode == 0, "shim did not reproduce jq 1.6's empty-input exit 0" + + # ...and everything the shim must NOT disturb + ok = subprocess.run([str(jq), "-r", "length"], input=b"[1,2,3]", capture_output=True) + assert ok.returncode == 0 and ok.stdout.strip() == b"3", f"shim broke real jq delegation: {ok}" + + # `jq -n` legitimately has empty stdin and MUST still produce output — the hook's `decide` + # builds its decision JSON that way. A shim that swallowed it made every decision look like a + # passthrough, i.e. manufactured the exemption the test below is trying to disprove. + nullin = subprocess.run([str(jq), "-n", "--arg", "r", "hi", "{a:$r}"], input=b"", capture_output=True) + assert nullin.returncode == 0 and b"hi" in nullin.stdout, f"shim broke `jq -n`: {nullin}" + + +def test_transport_failure_withholds_exemption_even_on_jq16(hook_jq16): + """The #631 regression: a mid-pagination transport failure must withhold the exemption on a + host whose jq returns 0 for empty input, not just on jq >= 1.7. + + Page 1 = 50 docs rows (so the path list is non-empty and the exemption would genuinely fire), + page 2 = transport failure, page 3 = a legitimate empty page. Pre-fix this returned True.""" + hook_jq16.set_pages(_rows([f"docs/f{i}.md" for i in range(50)]), "ERROR", []) + assert hook_jq16.exempted() is False + + +# --- #643 review findings: fail-opens independent of the jq version ----------------------------- + + +def test_newline_in_filename_does_not_split_into_two_passing_paths(hook): + """A path containing a newline must NOT be flattened into two allow-list-passing lines. + + `chunk` renders paths as newline-delimited text, so `"safe.md\\ndocs/Program.cs"` becomes two + lines — `safe.md` and `docs/Program.cs` — which BOTH match the docs allow-list, while the real + single path ends in `.cs`. Git permits newlines in filenames, so this is reachable. Fail closed + on control characters.""" + hook.set_pages([{"filename": "safe.md\ndocs/Program.cs", "status": "added"}]) + assert hook.exempted() is False + + +def test_newline_in_previous_filename_is_also_rejected(hook): + """Same hole via the rename side — `previous_filename` is flattened identically. + + NOTE the payload's second segment must itself be allow-list-PASSING (`docs/Program.cs`, not + `ErsatzTV/Program.cs`). The first version of this test used the latter, which the allow-list + rejects on its own merits, so the test passed with the newline guard entirely removed — it + asserted the outcome without ever exercising the mechanism. That is the same + filter-hides-the-defect trap the guard itself is about.""" + hook.set_pages([{"filename": "docs/ok.md", "previous_filename": "safe.md\ndocs/Program.cs", + "status": "renamed"}], []) + assert hook.exempted() is False + + +@pytest.mark.parametrize("status", ["modified", "copied", "added"]) +def test_previous_filename_is_validated_on_NON_renamed_rows_too(hook, status): + """The validation domain must match the CONSUMPTION domain. + + `chunk` emits `(.previous_filename // empty)` for EVERY row regardless of `.status`, but the + field was validated only when `.status == "renamed"`. A row marked `modified` (or Gitea's + distinct `copied`) carrying a newline in `previous_filename` was reproducibly exempted.""" + hook.set_pages([{"filename": "docs/ok.md", "status": status, + "previous_filename": "safe.md\ndocs/Program.cs"}], []) + assert hook.exempted() is False + + +def test_dotdot_path_component_is_rejected(hook): + """The allow-list anchors `^docs/`, so `docs/../ErsatzTV/Program.cs` matches it. Git will not + produce such a path, but this guard's job is to fail closed on unexpected 2xx shapes rather + than assume a well-behaved peer.""" + hook.set_pages([{"filename": "docs/../ErsatzTV/Program.cs", "status": "modified"}], []) + assert hook.exempted() is False + + +def test_legitimate_rename_within_docs_still_exempts(hook): + """Positive control: the tightened row schema must not break a real docs-only rename.""" + hook.set_pages([{"filename": "docs/b.md", "status": "renamed", + "previous_filename": "docs/a.md"}], []) + assert hook.exempted() is True + + +def test_short_NONTERMINAL_page_does_not_end_the_enumeration(hook): + """"Fewer rows than we asked for" must not be read as "last page". + + Gitea caps `limit` at the server-wide MAX_RESPONSE_ITEMS (default 50, configurable) and may + return fewer rows than requested. A 30-row docs page followed by a page of code would otherwise + complete the enumeration over a PARTIAL list — the same fail-open, reached with no transport + error at all. Only a validated EMPTY page may terminate it.""" + hook.set_pages(_rows([f"docs/f{i}.md" for i in range(30)]), + _rows(["ErsatzTV/Program.cs"]), + []) + assert hook.exempted() is False + + +def test_short_page_then_empty_page_still_exempts_a_genuinely_docs_only_pr(hook): + """Positive control for the change above: the stricter terminator must not break the happy path. + + Without this, 'never terminate on a short page' could be satisfied by never exempting anything.""" + hook.set_pages(_rows([f"docs/f{i}.md" for i in range(30)]), []) + assert hook.exempted() is True + + +def test_head_moving_mid_enumeration_withholds_the_exemption(hook, tmp_path): + """Paging is several round-trips; a force-push between them means the assembled list belongs to + no single commit. Page 1 from head A can be combined with a short docs tail from head B while + B's code page is never read. Re-read the head and refuse if it moved.""" + (tmp_path / "state" / "pr_sha_after.txt").write_text("b" * 40) + hook.set_pages(_rows([f"docs/f{i}.md" for i in range(30)]), []) + assert hook.exempted() is False + + +@pytest.mark.parametrize("status", ["Renamed", "RENAMED", "bogus", None]) +def test_a_rename_disguised_by_an_unknown_status_is_rejected(hook, status): + """`renamed => previous_filename REQUIRED` was keyed on an exact lowercase string, so any other + value took the `else true` branch: a `git mv ErsatzTV/Program.cs -> docs/a.md` row whose status + is `"Renamed"` (or absent) validated fine and silently dropped its SOURCE path, reading as + docs-only. `.status` is now checked against the closed set Gitea actually emits.""" + row = {"filename": "docs/a.md"} + if status is not None: + row["status"] = status + hook.set_pages([row], []) + assert hook.exempted() is False + + +def test_gitea_real_status_values_are_accepted(hook): + """Positive control for the closed set. The real Gitea 1.25.4 value for an edit is `changed`, + NOT `modified` — a closed allow-list built from the wrong vocabulary would reject every real + docs-only PR, which is a far worse failure than the hole it closes.""" + hook.set_pages([{"filename": "docs/a.md", "status": "changed"}, + {"filename": "docs/b.md", "status": "added"}, + {"filename": "docs/c.md", "status": "deleted"}], []) + assert hook.exempted() is True + + +# --- allow-list ANCHOR pins (round-3 review: three surviving mutants) -------------------------- +# The round-3 `..` finding was an anchor subversion, and mutating the anchors showed no test +# covered them: dropping `^` from the docs/ alternative, or `$` from `.md`, both survived. + +def test_docs_must_be_a_PREFIX_not_a_substring(hook): + """Dropping `^` would exempt `ErsatzTV/docs/Program.cs`.""" + hook.set_pages([{"filename": "ErsatzTV/docs/Program.cs", "status": "changed"}], []) + assert hook.exempted() is False + + +def test_md_must_be_a_SUFFIX_not_a_substring(hook): + """Dropping `$` would exempt `x.md.cs`.""" + hook.set_pages([{"filename": "ErsatzTV/x.md.cs", "status": "changed"}], []) + assert hook.exempted() is False + + +def test_an_empty_file_list_is_never_exempt(hook): + """`[ -n "$files" ]` guards this: a PR whose enumeration yields no paths must not read as + 'all of its files are docs'.""" + hook.set_pages([]) + assert hook.exempted() is False + + +def test_array_valued_status_does_not_dodge_the_allow_list(hook): + """`index` is polymorphic: with an ARRAY argument it does SUBSEQUENCE matching, not element + equality. So `["added",...,"renamed",...] | index(["renamed"])` is 4 (truthy) while + `.status == "renamed"` is false — the row passed the allow-list AND skipped the + `previous_filename REQUIRED` clause, which is the same `git mv code -> docs/` dodge the closed + set exists to block, one type away. Fixed by requiring `.status` to be a string first.""" + hook.set_pages([{"filename": "docs/a.md", "status": ["renamed"]}], []) + assert hook.exempted() is False + + +def test_object_valued_status_is_also_rejected(hook): + hook.set_pages([{"filename": "docs/a.md", "status": {"x": "renamed"}}], []) + assert hook.exempted() is False