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) <noreply@anthropic.com>
8.5 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`, `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. | 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 the
sibling job carries an ignore-rule. Same question, opposite answers, for stated reasons.
Unconditional, not path-filtered. The suite's real input set spans more than scripts/:
test_post_review_verdict.py and test_merge_consent_exemption.py execute the actual
scripts/post-review-verdict.sh and .claude/hooks/pretooluse-merge-consent.sh. A scripts/**
filter would silently miss a .claude/hooks/** edit — and at ~10s a filter buys nothing but a
drift surface. Default checkout depth is sufficient: every git call in the suite runs against a
temp repo it creates itself, never this repository's history.
Verified by measurement, not a green tick (the #631 Done-when, and ci.docs-only-detect-shallow-safe's
lesson): a deliberately-failing test was confirmed to exit non-zero locally and to turn the CI job
red on a scratch PR, before the passing state was accepted as meaningful.
What running it in CI immediately found. The first green-on-my-machine run went red twice, and both reds were real:
ModuleNotFoundError: No module named 'yaml'— the suite is not stdlib-only (test_migration_equivalence.pyimportsmigrate_decisions_split, whose write path uses PyYAML). It passed locally only because the author's machine had PyYAML.- A fail-open in the merge-consent gate itself.
jq -eover 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[], andfiles_complete=yeswas set over a PARTIAL list: the docs-only exemption firing over unread pages that may be pure code. Fixed by rejecting an empty body explicitly instead of inferring it from jq's exit status.
The second one is the argument for this record in miniature. It sat in the gate that decides whether a PR skips the Done-when checks, it was covered by an existing test, and that test could not catch it on a developer Mac (jq 1.8) — only in CI, where the suite had never run. Standing rules it leaves behind, all three the same shape — never infer a CONDITION from a jq exit status or a version-dependent builtin:
- never infer "empty input" from a jq exit status; check the string (
#643); - never infer "parse error" from a jq exit status —
jq emptyis the portable test, because jq >= 1.7 exits 5 where 1.6 exits 4, and 4 is also "no output" (#647); - never use
contains("\u0000")— on jq 1.6 the escape truncates to""and it matches every string (#647).explode | index(0)is version-stable.
The runner ships jq 1.6; a developer Mac ships 1.8.x. Three divergences were found in one day,
so the durable fix is to pin or preflight the version rather than keep patching constructs —
tracked on #647.
An independent cross-family review of that fix then found two further fail-opens in the same enumeration, both reachable with no transport error at all (#643):
- A path containing a newline. The file list is flattened into newline-delimited text before the
allow-list grep, so
"safe.md\ndocs/Program.cs"splits into two lines that each pass while the real path ends in.cs. Git permits newlines in filenames; it was reproduced against the hook. Now rejected outright — a docs path never contains a control character. - A short page read as the last page. Gitea caps
limitat the server-wideMAX_RESPONSE_ITEMSand may return fewer rows than asked for, son < 50does 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.