Files
ersatztv/docs/decisions/records/ci/script-tests-job.md
T
5d955000f3
Build ErsatzTV Image / CI toolchain image resolves (push) Successful in 10s
Build ErsatzTV Image / Delimiter ban (release path) (push) Successful in 31s
Build ErsatzTV Image / Build & test (.NET) (push) Successful in 15m44s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (push) Successful in 10m51s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (push) Successful in 8m27s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (push) Skipped
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (push) Skipped
Build ErsatzTV Image / Build & push image (amd64) (push) Failing after 1m46s
fix(845): the verdict writer checks that the gate will honour what it just posted (#889)
`review-verdict.yml` inherits an existing `review-verdict/h10=success` only from a status whose
`.creator.login` is on its `H10_REVIEWERS` allow-list (#742). `post-review-verdict.sh` wrote those
verdicts with whatever account owned the credential in the environment and never asked whose it was.
Two coupled values, nothing asserting the coupling, and the failure was the silent kind: the status
is written, the tool reports success, and the next `pull_request_target` event re-derives it and
posts over it. The PR stalls with no visible cause.

The writer now READS ITS OWN STATUS BACK, identifies that write by state and description, and
refuses — before the verdict comment, so the surviving half-state is the documented `ask` one —
unless the recorded creator is allow-listed. Measured after the write rather than probed before it:
that tests what Gitea recorded as the author, which is the value the gate reads, and needs no scope
beyond the repo access the POST already required.

Membership is required for a `success` ONLY, mirroring the gate's own asymmetry: a `failure` is
inherited from any attributable account, so requiring it there would refuse a verdict the gate
honours and leave an off-list reviewer no supported way to record a rejection.

The allow-list is DERIVED from the gate's own literal by the new `scripts/lib/h10-reviewers.sh` —
one declaration, not two plus a parity test. It is a parse rather than a shared declaration both
sides source because the gate runs against a checkout of the PR's BASE sha: a PR whose base predates
such a file would not have it, and a missing `source` under `set -euo pipefail` kills the job, which
posts no `review-verdict/h10` at all and blocks every merge including its own repair (#743).

`scripts/post-review-verdict.sh` moves BEHAVIOUR-ONLY -> MUTATION in the guard inventory, which the
manifest's own note called "the most valuable upgrade on this list". The declared clause lives in the
GATE: rewriting `H10_REVIEWERS` while the posting account stays fixed reddens the accept path only if
the writer reads the list live AND the comparison gates the outcome.

Two defects were caught by probing the live instance rather than re-reading the code. Reading `.state`
instead of `.status` per row would have refused EVERY verdict — a repo-wide deadlock, shipped green,
because the test shim replayed the POST payload as the read-back body and so agreed with the parser
by construction. Then a `(.status // .state)` fallback added as defensiveness recreated #845 exactly:
the writer would accept a shape the gate cannot read and report success.

Nine independent cold review rounds, all worktree-isolated, one cross-family (GPT-5.6 via Codex).
Round 8 caught the most important one: a `set -u` "correction" made mid-branch had inverted a TRUE
statement in live merge-gate code, because the probe used a plain `$UNSET` while the validator uses
`${#arr[@]}` — different shapes, different behaviour. Withdrawn wholesale; both libraries are
byte-identical to `main` again.

Verification: full `scripts/tests` suite green (1278 passed, 2 skipped); the declared mutation
executes every run and reddens its named proof with the manifest's `expect` string; every clause
disarmed individually and confirmed to redden its own named test; live probes against Gitea 1.27.1
for the row shape, the description round-trip, the paging order and the required-check list.

Docs: `ci.exemption-provenance` records the coupling as asserted rather than as a tracked residual,
plus `docs/ci-cd.md`, `CLAUDE.md`, `docs/guard-inventory.md`, `docs/remote-state-inventory.md`,
`ci.script-tests-job` and the `script-tests` population comment in `pr-checks.yml`.

Deferred: the refused-verdict residual (a non-inheritable status left standing with no comment) is
the `ask` half-state `release.verdict-writes-status-before-comment` designates as safe; a second
corrective write is the sticky-sentinel mechanism #849 is separately designing.

fixes #845

Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Timothy <timothy@noreply.gitea.tblindustries.be>
2026-08-30 02:43:08 +00:00

9.2 KiB

key, title, status, since, supersedes, superseded-by, rule, signals, mechanics
key title status since supersedes superseded-by rule signals mechanics
ci.script-tests-job 2026-07-26 — `scripts/tests/` runs in CI as its own `script-tests` job, never inside the flake-covered `decisions-guard` (#631) active 2026-07-26 none none 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 pyyaml`, `PYTHONPATH=. python3 -m pytest scripts/tests -q`; since #780 it also runs a pinned ruff over a `git ls-files` population first), 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. 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 `.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 a pinned ruff), 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, and since #845 the former also READS .gitea/workflows/review-verdict.yml — the verdict writer derives the H10 allow-list from it, so editing that literal changes the suite's outcome. A scripts/** filter would silently miss a .claude/hooks/** or .gitea/workflows/** edit. The enumeration is a lower bound rather than a census, deliberately: the corpus tests read docs/ too, and an exact list here is the kind that goes stale — which it did, this way, before #845 extended it. The reason is the INPUT SET, not the cost: a filter was cheap to skip when the suite ran in seconds and is still the wrong axis now that it runs in minutes. Default checkout depth is sufficient: every git call in the suite runs against a temp repo it creates itself, never this repository's history.

Verified by measurement, not a green tick (the #631 Done-when, and ci.docs-only-detect-shallow-safe's 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. Two further divergences of the same shape (a contains("\u0000") false positive and a colliding parse-error exit code) turned up the same day; the durable contract that came out of all three — the exact constructs to avoid, and why jq-preflight.sh pins in script-tests but only floors the version in review-verdict.yml — is recorded once, in ci.jq-version-contract (docs/decisions/records/ci/jq-version-contract.md), and is not restated here.

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.

Since #780 the job also lints Python before pytest, so its display name is Script lint and tests (ruff + pytest). It does not invoke ruff check . — the invocation and the reasons for its exact shape are ci.python-lint-ruff-config-committed. Why the lint lives here rather than in a job of its own: it needs the same setup-python, it costs seconds, and a second job would double the dispatch overhead this file exists to keep small.