From 69d8d3ccfed691189c4018162441893394e287cf Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 00:32:57 +0200 Subject: [PATCH 01/10] ci(631): run scripts/tests in CI as its own job, not inside the flake-covered decisions-guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nothing executed scripts/tests/. No workflow and no Husky hook invoked pytest: decisions-guard runs decisions_validate.py and build_decisions_catalog.py directly (the code, never its tests) and the `test` job is dotnet test only. The 111 tests guarding the decision corpus, the #610 migration-equivalence harness, the merge-consent exemption logic and the #622 review-verdict poster were enforced only by whoever remembered to run pytest locally. Adds a `script-tests` job to pr-checks.yml rather than the step inside decisions-guard the issue proposed. decisions-guard is covered by ci.decisions-lifecycle-flake, a standing instruction that a lone `decisions lifecycle` red is a known flake and must not be investigated — so a genuine pytest regression there would surface as exactly the red every session is told to wave through. A gate inherits the credibility of the job it lives in. Runs unconditionally: the suite executes the real post-review-verdict.sh and pretooluse-merge-consent.sh, so a `scripts/**` path filter would miss a .claude/hooks/** edit. ~10s, pure stdlib + pytest. Negative control verified locally (broken test -> exit 1); CI-red measurement follows on a scratch PR per the Done-when. Refs #631 --- .gitea/workflows/pr-checks.yml | 47 +++++++++++++++-- docs/ci-cd.md | 50 ++++++++++++++++--- docs/decisions/README.md | 1 + docs/decisions/records/ci/script-tests-job.md | 48 ++++++++++++++++++ 4 files changed, 137 insertions(+), 9 deletions(-) create mode 100644 docs/decisions/records/ci/script-tests-job.md diff --git a/.gitea/workflows/pr-checks.yml b/.gitea/workflows/pr-checks.yml index cee68a461..0a6fb8065 100644 --- a/.gitea/workflows/pr-checks.yml +++ b/.gitea/workflows/pr-checks.yml @@ -3,7 +3,8 @@ 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 +# WHY THIS FILE EXISTS. These checks are cheap `checkout + git diff` (or, for `script-tests`, +# checkout + pytest) gates: 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 @@ -22,8 +23,9 @@ 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 +# 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". @@ -187,3 +189,42 @@ 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. Pure-stdlib + pytest (the decisions read path is deliberately + # dependency-free — no PyYAML), so the only install is pytest itself. + 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' + - name: Install pytest + run: python3 -m pip install --disable-pip-version-check --quiet pytest + - name: Run scripts/tests + run: PYTHONPATH=. python3 -m pytest scripts/tests -q diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 5ad5f92c2..fbaf8eccd 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -149,8 +149,11 @@ 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 pure-stdlib `pytest` run +(ersatztv#631). 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 +510,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 +601,47 @@ 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` +(111 tests, ~10s). 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. The suite is pure stdlib plus +`pytest` (the decisions read path is deliberately dependency-free — no PyYAML), so the only install +step is `pip install pytest`; 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. + ## 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/ci/script-tests-job.md b/docs/decisions/records/ci/script-tests-job.md new file mode 100644 index 000000000..be03468a7 --- /dev/null +++ b/docs/decisions/records/ci/script-tests-job.md @@ -0,0 +1,48 @@ +--- +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 111 +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 (pytest needs only a checkout and `pip install pytest`), +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. + +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. -- 2.47.3 From c0f4a52d7a4909ab56aa60964de6eabf74a5de89 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 00:39:15 +0200 Subject: [PATCH 02/10] ci(631): preflight jq/git in script-tests, and reflow the header comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit test_post_review_verdict.py and test_merge_consent_exemption.py exec the real post-review-verdict.sh and pretooluse-merge-consent.sh, which shell out to jq ~26 times. Those tests shim `curl` on PATH but NOT `jq`, and no existing small-lane job proves jq is present on that image — so a missing jq would land as ~20 opaque assertion failures rather than a diagnosis. Checks rather than installs: #390 deliberately removed run-time apt-get from CI, so the correct fix for a genuine miss is baking the tool into the runner image, and the error message says so. Refs #631 --- .gitea/workflows/pr-checks.yml | 27 +++++++++++++++++++++------ docs/ci-cd.md | 6 ++++++ 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/.gitea/workflows/pr-checks.yml b/.gitea/workflows/pr-checks.yml index 0a6fb8065..f24ad58d6 100644 --- a/.gitea/workflows/pr-checks.yml +++ b/.gitea/workflows/pr-checks.yml @@ -3,9 +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 checks are cheap `checkout + git diff` (or, for `script-tests`, -# checkout + pytest) 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 @@ -25,9 +25,8 @@ name: PR Gates # docker-build.yml still validates the five pin-bearing jobs (test/migrations/functional-e2e/ # 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". +# 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: @@ -226,5 +225,21 @@ jobs: python-version: '3.x' - name: Install pytest run: python3 -m pip install --disable-pip-version-check --quiet pytest + # 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/docs/ci-cd.md b/docs/ci-cd.md index fbaf8eccd..c5075c3f1 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -635,6 +635,12 @@ step is `pip install pytest`; 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. -- 2.47.3 From 73577f484fec6684a4bf9df0a1a95d43198e6c14 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 00:44:17 +0200 Subject: [PATCH 03/10] =?UTF-8?q?ci(631):=20install=20pyyaml=20too=20?= =?UTF-8?q?=E2=80=94=20the=20suite=20is=20not=20pure-stdlib,=20and=20CI=20?= =?UTF-8?q?proved=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First CI run went red on `ModuleNotFoundError: No module named 'yaml'` at collection: test_migration_equivalence.py imports scripts/migrate_decisions_split.py, which uses PyYAML by design. It passed locally only because this machine already had PyYAML. The original claim came from reading decisions_lib/decisions_validate/build_catalog and the test files themselves — which is precisely the set that does NOT import yaml. Replaced with an AST import scan over all of scripts/, which gives the complete third-party set: pytest and yaml. (etv_client is imported only by scripts/scripted-schedules/entrypoint.py, a container entrypoint pytest never collects.) This does not contradict the dependency-free decisions READ path: decisions_lib._read_frontmatter is hand-written exactly so validation runs where nothing is installed. The one-shot WRITE path is allowed PyYAML, and its test needs it. Verified the way it should have been the first time: a clean venv with pytest alone reproduces CI's collection error; adding pyyaml gives 111 passed. Comments, ci-cd.md and the decision record all corrected to drop the "pure stdlib" claim. Refs #631 --- .gitea/workflows/pr-checks.yml | 15 +++++++++++---- docs/ci-cd.md | 15 +++++++++++---- docs/decisions/records/ci/script-tests-job.md | 4 ++-- 3 files changed, 24 insertions(+), 10 deletions(-) diff --git a/.gitea/workflows/pr-checks.yml b/.gitea/workflows/pr-checks.yml index f24ad58d6..0fe73e668 100644 --- a/.gitea/workflows/pr-checks.yml +++ b/.gitea/workflows/pr-checks.yml @@ -210,8 +210,7 @@ jobs: # 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. Pure-stdlib + pytest (the decisions read path is deliberately - # dependency-free — no PyYAML), so the only install is pytest itself. + # filter buys nothing but drift. script-tests: name: Script tests (pytest) runs-on: small @@ -223,8 +222,16 @@ jobs: uses: actions/setup-python@v5 with: python-version: '3.x' - - name: Install pytest - run: python3 -m pip install --disable-pip-version-check --quiet pytest + # 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. diff --git a/docs/ci-cd.md b/docs/ci-cd.md index c5075c3f1..0d43f5a6f 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -629,10 +629,17 @@ name keeps a real failure unambiguous. 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. The suite is pure stdlib plus -`pytest` (the decisions read path is deliberately dependency-free — no PyYAML), so the only install -step is `pip install pytest`; 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 +`.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 diff --git a/docs/decisions/records/ci/script-tests-job.md b/docs/decisions/records/ci/script-tests-job.md index be03468a7..8835c612e 100644 --- a/docs/decisions/records/ci/script-tests-job.md +++ b/docs/decisions/records/ci/script-tests-job.md @@ -29,8 +29,8 @@ 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 (pytest needs only a checkout and `pip install pytest`), -and the sibling job carries an ignore-rule. Same question, opposite answers, for stated reasons. +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 -- 2.47.3 From 5f068a24887f233309a9f7d2e63258de4fc0124a Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 01:00:28 +0200 Subject: [PATCH 04/10] fix(631): close a fail-open in the merge-consent gate that only CI could see MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Running scripts/tests in CI for the first time turned up a real defect in the gate itself, not just in the harness. `jq -e` over EMPTY input exits 4 on jq >= 1.7 but 0 on jq 1.6 — verified against both binaries, not inferred. The docs-only pagination guard in pretooluse-merge-consent.sh leaned on that exit status to reject a transport failure. On jq 1.6, which the CI runner ships: page 2 errors -> gq returns empty -> jq guard wrongly PASSES -> n is empty so [ "$n" -lt 50 ] errors into false -> the loop walks PAST the failed page -> page 3 legitimately returns [] -> files_complete=yes over a PARTIAL list -> the docs-only exemption fires over unread pages that may be pure code. That is the very defect the guard's own comment describes, reintroduced one layer down by a jq version difference. Fixed by rejecting an empty body explicitly rather than inferring it from jq's exit status. The same hardening is applied to the review-verdict status read, which fell through to `vstate=""` -> deny (fail-CLOSED, so never a hole) but would have surfaced the wrong message. Why it survived: the existing transport-failure test asserts the right thing but can only observe the bug where jq is 1.6, so it passes on a developer Mac with the bug fully present — and the suite had never run anywhere else. The new test removes that dependency by shimming ONLY jq 1.6's empty-input exit status, so it pins the property on any host. Mutation-verified: revert the fix and it goes red on jq 1.8.2. The shim is deliberately narrow (no `-n`): a broader first version swallowed the `jq -n` calls `decide` uses to build its JSON, so the hook emitted nothing and every decision read as a passthrough — the verifier manufacturing the exemption it was meant to disprove. test_jq16_shim_actually_reproduces_the_quirk now pins the shim itself, `-n` case included. Suite: 113 passed under jq 1.8.2 AND under jq 1.6. Refs #631 --- .claude/hooks/pretooluse-merge-consent.sh | 18 ++- docs/decisions/records/ci/script-tests-job.md | 19 ++++ scripts/tests/test_merge_consent_exemption.py | 106 ++++++++++++++++++ 3 files changed, 142 insertions(+), 1 deletion(-) diff --git a/.claude/hooks/pretooluse-merge-consent.sh b/.claude/hooks/pretooluse-merge-consent.sh index b0c03c9ac..79cdf7d0e 100755 --- a/.claude/hooks/pretooluse-merge-consent.sh +++ b/.claude/hooks/pretooluse-merge-consent.sh @@ -103,6 +103,18 @@ 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 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))' \ >/dev/null 2>&1; then @@ -210,7 +222,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/docs/decisions/records/ci/script-tests-job.md b/docs/decisions/records/ci/script-tests-job.md index 8835c612e..878d6badb 100644 --- a/docs/decisions/records/ci/script-tests-job.md +++ b/docs/decisions/records/ci/script-tests-job.md @@ -43,6 +43,25 @@ temp repo it creates itself, never this repository's history. 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 rule it leaves +behind: never infer "empty input" from a jq exit status; check the string.** + 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/tests/test_merge_consent_exemption.py b/scripts/tests/test_merge_consent_exemption.py index ac6bbd9c4..ef2f37af2 100644 --- a/scripts/tests/test_merge_consent_exemption.py +++ b/scripts/tests/test_merge_consent_exemption.py @@ -230,3 +230,109 @@ 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 -- 2.47.3 From c046add10af457837003b0254bb41ed5c9864201 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 11:04:25 +0200 Subject: [PATCH 05/10] fix(643): close two more fail-opens in the docs-only enumeration, found by cold review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An independent cross-family review of the jq-1.6 fix found two further ways the docs-only exemption can fire over an incomplete file list — both reachable with NO transport error, so neither had anything to do with the original bug. 1. HIGH — a path containing a newline. `chunk` flattens paths into newline-delimited text before the allow-list grep, so a filename of "safe.md\ndocs/Program.cs" splits into two lines that BOTH match the allow-list, while the real single path ends in .cs. Git permits newlines in filenames and the reviewer reproduced the bypass against this hook. Now rejected outright at the row-schema guard, on both `filename` and `previous_filename`: no docs path contains a control character, so failing closed costs nothing. 2. HIGH — a short page read as the last page. `n < 50` assumed the server's page size is the 50 we requested, but Gitea caps `limit` at the server-wide MAX_RESPONSE_ITEMS (default 50, configurable) and may return fewer. A 30-row docs page followed by a page of code completed the enumeration over a PARTIAL list. Only a validated EMPTY page may now terminate it; the page<=40 cap still fails closed, and the cost is one extra request. 3. MEDIUM — the enumeration was not bound to one head. Paging is several round-trips, so a force-push between them assembles a list belonging to no single commit: page 1 from head A plus a short docs tail from head B, with B's code page never read. The head sha is re-read after enumeration and the exemption refused if it moved. All three mutation-verified: reverting each fix reddens exactly its own test and nothing else. A positive control (short page then empty page) pins that the stricter terminator still exempts a genuinely docs-only PR, so "never terminate early" cannot be satisfied by never exempting anything. 118 passed under BOTH jq 1.8.2 and jq 1.6. The record now states the generalisable lesson: every defect here was an exhaustiveness failure in an enumeration whose completeness is load-bearing. When a security decision depends on having seen ALL of something, the termination condition must be positive and explicit, never inferred from a proxy. Refs #643, #631 --- .claude/hooks/pretooluse-merge-consent.sh | 31 +++++++++- docs/decisions/records/ci/script-tests-job.md | 21 +++++++ scripts/tests/test_merge_consent_exemption.py | 62 ++++++++++++++++++- 3 files changed, 111 insertions(+), 3 deletions(-) diff --git a/.claude/hooks/pretooluse-merge-consent.sh b/.claude/hooks/pretooluse-merge-consent.sh index 79cdf7d0e..e72801856 100755 --- a/.claude/hooks/pretooluse-merge-consent.sh +++ b/.claude/hooks/pretooluse-merge-consent.sh @@ -115,8 +115,19 @@ while [ "$page" -le 40 ]; do 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. 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 'type == "array" and all(.[]; + (.filename | type == "string" and length > 0 and (test("[\\r\\n]") | not)) + and (if .status == "renamed" + then (.previous_filename | type == "string" and length > 0 and (test("[\\r\\n]") | not)) + else true end))' \ >/dev/null 2>&1; then files_complete=no; break fi @@ -127,10 +138,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 diff --git a/docs/decisions/records/ci/script-tests-job.md b/docs/decisions/records/ci/script-tests-job.md index 878d6badb..448771a06 100644 --- a/docs/decisions/records/ci/script-tests-job.md +++ b/docs/decisions/records/ci/script-tests-job.md @@ -62,6 +62,27 @@ a PR skips the Done-when checks, it was covered by an existing test, and that te on a developer Mac (jq 1.8) — only in CI, where the suite had never run. **Standing rule it leaves behind: never infer "empty input" from a jq exit status; check the string.** +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. + +**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/tests/test_merge_consent_exemption.py b/scripts/tests/test_merge_consent_exemption.py index ef2f37af2..7311d0d27 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("{}") @@ -336,3 +345,54 @@ def test_transport_failure_withholds_exemption_even_on_jq16(hook_jq16): 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.""" + hook.set_pages([{"filename": "docs/ok.md", "previous_filename": "safe.md\nErsatzTV/Program.cs", + "status": "renamed"}]) + assert hook.exempted() is False + + +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 -- 2.47.3 From 54c875414c8235b55e48e4374ff956255f1523e8 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 11:23:28 +0200 Subject: [PATCH 06/10] fix(643): validate every field the extraction consumes; fix a vacuous test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review of the previous fix commit found it incomplete and its test vacuous. Both reproduced before fixing. MEDIUM — the validation domain did not match the CONSUMPTION domain. `chunk` emits `(.previous_filename // empty)` for EVERY row regardless of status, but the guard validated that field only when `.status == "renamed"`. So a row marked `modified` — or Gitea's distinct `copied` — carrying a newline in previous_filename was still exempted. Verified EXEMPT for both statuses against the previous commit. Now validated whenever present, with the `renamed` => REQUIRED clause kept on top. MEDIUM — test_newline_in_previous_filename_is_also_rejected asserted the right outcome without exercising the mechanism: its payload's second segment was `ErsatzTV/Program.cs`, which the allow-list rejects on its own, so the test passed with the newline guard entirely removed. That is why the hole above went unnoticed — the same filter-hides-the-defect trap the guard itself is about. Payload changed to a segment that PASSES the allow-list, so the test now discriminates, plus parametrized regressions for modified/copied/added. LOW — `..` components rejected. The allow-list anchors `^docs/`, so `docs/../ErsatzTV/Program.cs` matched it (reproduced). Git will not produce such a path, but this guard exists to fail closed on unexpected 2xx shapes. Added a positive control (a legitimate docs->docs rename still exempts) so the tightened row schema cannot be satisfied by never exempting anything. Severity calibrated in the record: the docs-only exemption ends in `decide allow ""`, a passthrough to the normal permission prompt, NOT an auto-grant. Every bypass here downgrades a mechanical deny/ask to a human prompt; none can silently self-merge. Real, worth fixing, but not what an earlier framing of #643 implied. All mutation-verified; 123 passed under BOTH jq 1.8.2 and jq 1.6. Refs #643, #631 --- .claude/hooks/pretooluse-merge-consent.sh | 22 +++++++++-- docs/decisions/records/ci/script-tests-job.md | 14 +++++++ scripts/tests/test_merge_consent_exemption.py | 39 +++++++++++++++++-- 3 files changed, 69 insertions(+), 6 deletions(-) diff --git a/.claude/hooks/pretooluse-merge-consent.sh b/.claude/hooks/pretooluse-merge-consent.sh index e72801856..9c30a90cf 100755 --- a/.claude/hooks/pretooluse-merge-consent.sh +++ b/.claude/hooks/pretooluse-merge-consent.sh @@ -122,11 +122,27 @@ while [ "$page" -le 40 ]; do # `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. if ! printf '%s' "$raw" \ - | jq -e 'type == "array" and all(.[]; - (.filename | type == "string" and length > 0 and (test("[\\r\\n]") | not)) + | 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 (if .status == "renamed" - then (.previous_filename | type == "string" and length > 0 and (test("[\\r\\n]") | not)) + then (.previous_filename | type == "string" and length > 0) else true end))' \ >/dev/null 2>&1; then files_complete=no; break diff --git a/docs/decisions/records/ci/script-tests-job.md b/docs/decisions/records/ci/script-tests-job.md index 448771a06..a60374381 100644 --- a/docs/decisions/records/ci/script-tests-job.md +++ b/docs/decisions/records/ci/script-tests-job.md @@ -77,6 +77,20 @@ Plus a **Medium**: paging is several round-trips, so a force-push between them y 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, diff --git a/scripts/tests/test_merge_consent_exemption.py b/scripts/tests/test_merge_consent_exemption.py index 7311d0d27..d7fa4e4ea 100644 --- a/scripts/tests/test_merge_consent_exemption.py +++ b/scripts/tests/test_merge_consent_exemption.py @@ -362,12 +362,45 @@ def test_newline_in_filename_does_not_split_into_two_passing_paths(hook): def test_newline_in_previous_filename_is_also_rejected(hook): - """Same hole via the rename side — `previous_filename` is flattened identically.""" - hook.set_pages([{"filename": "docs/ok.md", "previous_filename": "safe.md\nErsatzTV/Program.cs", - "status": "renamed"}]) + """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". -- 2.47.3 From f4473926d47642a04e5f00fcc0a0616a0bf86f59 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 12:10:02 +0200 Subject: [PATCH 07/10] fix(643): close the status-string dodge, pin the allow-list anchors, fix two doc claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-3 review (MERGEABLE, all findings Low/Nit) — applied anyway, because each is the over-claim or unpinned-anchor family this PR keeps hitting. LOW — the `renamed => previous_filename REQUIRED` clause matched `.status` by exact lowercase string, so any other value took the `else true` branch: a row with `"Renamed"`, or with no status at all, validated fine and silently dropped its SOURCE path, letting `git mv ErsatzTV/Program.cs -> docs/a.md` read as docs-only. `.status` is now checked against a closed set and an unknown value fails closed. Two things that fix taught me, both caught by my own positive control rather than by review: 1. The first predicate was WRONG in a way that gated everything: inside `[...] | index(.status)`, jq's `.` is the ARRAY, so `.status` was null and every row failed. `$s` is now bound from the row before the context switches. A security check that rejects everything looks identical to a working one from the failing side — only test_gitea_real_status_values_are_accepted caught it. 2. The set includes BOTH `changed` and `modified`. Live Gitea 1.25.4 emits `changed`, but a closed allow-list built from the wrong vocabulary is worse than the hole it closes: it would gate every genuine docs-only PR. The property wanted is "reject what we don't recognise", not "enumerate one version exactly". LOW — three allow-list anchors had no test at all: dropping `^` from the `docs/` alternative (`ErsatzTV/docs/Program.cs` would exempt), dropping `$` from `.md` (`x.md.cs` would exempt), and dropping the non-empty-list guard. Since the round-3 `..` finding WAS an anchor subversion, they are now pinned; all three mutation-verified. NIT — docs/ci-cd.md called this job "a checkout plus a pure-stdlib pytest run", which the same file contradicts 450 lines later and which this PR's own record names as the bug that turned the job red on its first CI run. Also replaced a frozen "111 tests" with an explicitly indicative figure — the suite is ~190 after rebasing onto main, and a frozen count has rotted four times this session. 198 passed under both jq 1.8.2 and jq 1.6. Refs #643, #631 Co-Authored-By: Claude Opus 5 (1M context) --- .claude/hooks/pretooluse-merge-consent.sh | 13 +++++ docs/ci-cd.md | 7 +-- docs/decisions/records/ci/script-tests-job.md | 2 +- scripts/tests/test_merge_consent_exemption.py | 53 ++++++++++++++++++- 4 files changed, 70 insertions(+), 5 deletions(-) diff --git a/.claude/hooks/pretooluse-merge-consent.sh b/.claude/hooks/pretooluse-merge-consent.sh index 9c30a90cf..e431aae22 100755 --- a/.claude/hooks/pretooluse-merge-consent.sh +++ b/.claude/hooks/pretooluse-merge-consent.sh @@ -134,6 +134,18 @@ while [ "$page" -le 40 ]; do # `..` 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 'def ok: type == "string" and length > 0 and (test("[\\r\\n]") | not) @@ -141,6 +153,7 @@ while [ "$page" -le 40 ]; do type == "array" and all(.[]; (.filename | ok) and (.previous_filename == null or (.previous_filename | ok)) + and ((.status // "") as $s | ["added","deleted","changed","modified","renamed","copied"] | index($s)) and (if .status == "renamed" then (.previous_filename | type == "string" and length > 0) else true end))' \ diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 0d43f5a6f..7db5282d1 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -149,8 +149,9 @@ 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` — plus `script-tests`, which is a checkout plus a pure-stdlib `pytest` run -(ersatztv#631). Nothing there runs a compiler or a `docker build`, which is why the lane +`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 @@ -608,7 +609,7 @@ compiler/docker build), so it doesn't violate the "small is git-only" lane rule. > `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` -(111 tests, ~10s). It covers the decision-corpus parser/validator/catalog builder, the ersatztv#610 +(~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. diff --git a/docs/decisions/records/ci/script-tests-job.md b/docs/decisions/records/ci/script-tests-job.md index a60374381..5cfb4eab9 100644 --- a/docs/decisions/records/ci/script-tests-job.md +++ b/docs/decisions/records/ci/script-tests-job.md @@ -12,7 +12,7 @@ mechanics: '`.gitea/workflows/pr-checks.yml` -> `script-tests`; `docs/ci-cd.md` 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 111 +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. diff --git a/scripts/tests/test_merge_consent_exemption.py b/scripts/tests/test_merge_consent_exemption.py index d7fa4e4ea..036e50294 100644 --- a/scripts/tests/test_merge_consent_exemption.py +++ b/scripts/tests/test_merge_consent_exemption.py @@ -228,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 @@ -429,3 +434,49 @@ def test_head_moving_mid_enumeration_withholds_the_exemption(hook, tmp_path): (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 -- 2.47.3 From 7265fba36dc24f7c0af0df002ae707f1dbfa3927 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 12:17:46 +0200 Subject: [PATCH 08/10] =?UTF-8?q?fix(647):=20the=20H10=20verdict=20classif?= =?UTF-8?q?ier=20was=20inert=20on=20jq=201.6=20=E2=80=94=20the=20runner's?= =?UTF-8?q?=20version?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Turning on the scripts/tests suite in CI immediately paid for itself: measured on origin/main, 61 of 178 tests FAIL under jq 1.6, which is what the CI runner ships. They pass on a dev Mac's jq 1.8.2, which is why this was invisible — and the suite has never run anywhere else, which is exactly #631's thesis. Two defects in scripts/check-review-verdict.sh (from #629, the single source of truth for H10 verdict classification): 1. `contains("")` is TRUE FOR EVERY STRING on jq 1.6 — the escape truncates the literal to the empty string, and every string contains "". So the body guard errored "NUL in body" on every comment and the H10 grammar was entirely inert on the runner. Verified against both binaries: 1.6 says true for "hello", 1.7+ says false. Replaced with `(explode | index(0)) != null`, which involves no regex engine and agrees on both. 2. A parse error was indistinguishable from "no output". The script used jq's exit code to separate malformed input from a legitimately empty comment list, treating 4 as benign — but jq >= 1.7 exits 5 on a parse error while 1.6 exits 4, the same code both use for "filter produced no output". On 1.6 a garbage API response therefore returned `absent` instead of an input error. Fixed with an explicit `jq empty` pre-check, which is non-zero iff the input does not parse regardless of output volume. Severity: fail-closed, not exploitable. The classifier is only invoked from the merge-consent hook, which runs on the dev machine (jq 1.8.2), so the live gate is unaffected. The cost is that #629's hardening was inert on the runner and would have stayed invisible. 198 tests now pass under BOTH jq 1.8.2 and jq 1.6 (was 138/60 split under 1.6). This is the third distinct jq-1.6 divergence found in this codebase today (the first was #643's `jq -e` on empty input). The rule: a shell gate's behaviour is a function of its interpreter's version — test against the version CI actually runs, or pin it. Refs #647, #631 Co-Authored-By: Claude Opus 5 (1M context) --- scripts/check-review-verdict.sh | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) 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 -- 2.47.3 From bb1809fbf018c6eb79042615ea71c7a9939c23b7 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 12:42:20 +0200 Subject: [PATCH 09/10] =?UTF-8?q?fix(647):=20the=20jq-1.6=20fail-open=20wa?= =?UTF-8?q?s=20ALSO=20in=20review-verdict.yml=20=E2=80=94=20the=20enforced?= =?UTF-8?q?=20gate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-4 review caught the miss that matters most: #643 fixed the `jq -e`-on-empty fail-open in .claude/hooks/pretooluse-merge-consent.sh, but the SAME construct sits in .gitea/workflows/review-verdict.yml — and that is the copy that runs on the CI runner, where jq is 1.6, and that feeds the branch-protection-required review-verdict/h10 status. Reproduced: `printf "" | jq -e '.statuses | type == "array"'` exits 4 on jq 1.8.2 (guard fires, correct) and 0 on jq 1.6 (guard passes). So on a transient API error `statusjson` is empty, the guard lets it through, `existing` reads "", and the job posts `pending` — or for a bot/docs-only PR an exemption `success` — over a possibly-existing human verdict. That is precisely what the comment three lines above it says must never happen. The hook version was harmless in practice because it runs on a dev Mac with jq 1.8. This one is live. Fixed identically, with a comment naming why the sibling fix missed it, and the same hardening applied to the changed-files read in the same workflow. Also from round 4: - LOW, reproduced — an ARRAY-valued `.status` dodged the closed allow-list. `index` is polymorphic: with an array argument it does SUBSEQUENCE matching, so `[...,"renamed",...] | index(["renamed"])` is truthy while `.status == "renamed"` is false — the row passed the allow-list AND skipped the `previous_filename REQUIRED` clause. The same `git mv code -> docs/` dodge the closed set exists to block, one type away. Now requires `.status` to be a string first; mutation-verified. - The record now carries all THREE jq rules rather than the one, and notes that the durable fix is pinning/preflighting the runner's jq version rather than patching constructs one at a time (tracked on #647). 200 tests pass under BOTH jq 1.8.2 and jq 1.6. Refs #647, #643, #631 Co-Authored-By: Claude Opus 5 (1M context) --- .claude/hooks/pretooluse-merge-consent.sh | 3 ++- .gitea/workflows/review-verdict.yml | 9 ++++++++- docs/decisions/records/ci/script-tests-job.md | 13 +++++++++++-- scripts/tests/test_merge_consent_exemption.py | 15 +++++++++++++++ 4 files changed, 36 insertions(+), 4 deletions(-) diff --git a/.claude/hooks/pretooluse-merge-consent.sh b/.claude/hooks/pretooluse-merge-consent.sh index e431aae22..6487efeae 100755 --- a/.claude/hooks/pretooluse-merge-consent.sh +++ b/.claude/hooks/pretooluse-merge-consent.sh @@ -153,7 +153,8 @@ while [ "$page" -le 40 ]; do type == "array" and all(.[]; (.filename | ok) and (.previous_filename == null or (.previous_filename | ok)) - and ((.status // "") as $s | ["added","deleted","changed","modified","renamed","copied"] | index($s)) + 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))' \ 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/decisions/records/ci/script-tests-job.md b/docs/decisions/records/ci/script-tests-job.md index 5cfb4eab9..476816b71 100644 --- a/docs/decisions/records/ci/script-tests-job.md +++ b/docs/decisions/records/ci/script-tests-job.md @@ -59,8 +59,17 @@ both reds were real: 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 rule it leaves -behind: never infer "empty input" from a jq exit status; check the string.** +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): diff --git a/scripts/tests/test_merge_consent_exemption.py b/scripts/tests/test_merge_consent_exemption.py index 036e50294..517ab2abe 100644 --- a/scripts/tests/test_merge_consent_exemption.py +++ b/scripts/tests/test_merge_consent_exemption.py @@ -480,3 +480,18 @@ def test_an_empty_file_list_is_never_exempt(hook): '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 -- 2.47.3 From 37fd30dce7c4bfd91b0ef9a726efc2f5162d8b5f Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 13:27:05 +0200 Subject: [PATCH 10/10] fix(631): quote a record title whose unquoted ` #` was a YAML comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third latent defect surfaced purely by running scripts/tests in CI — and this one was already red on origin/main, where test_frontmatter_reader_matches_pyyaml_on_every_real_record fails. `api/paging-zero-based.md` carried an UNQUOTED plain scalar title ending "... (#616, #633)". In YAML a space followed by `#` starts a COMMENT, so PyYAML reads the title as truncated at "(#616," while the hand-written dependency-free reader keeps the whole line. PyYAML is correct per spec; the record's title was genuinely losing "#633)" for any spec-compliant consumer, which includes MemPalace's ingest. Fixed by quoting the scalar; both parsers now agree byte-for-byte. Asked "instance or class?" before fixing: swept every frontmatter field of all 179 records under both parsers and compared. This is the ONLY divergence in the corpus, so a one-line fix is the right scope — no lint rule needed, and #621's structural guard already makes a parse FAILURE loud (this was a parse-to-different, which no structural check can see). 216 passed under jq 1.8.2 and jq 1.6. Refs #631 Co-Authored-By: Claude Opus 5 (1M context) --- docs/decisions/records/api/paging-zero-based.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 -- 2.47.3