fix(631): run scripts/tests in CI as its own job, not inside the flake-covered decisions-guard #637

Merged

timothy merged 10 commits from fix/631-script-tests-ci into main 2026-07-26 13:56:45 +02:00

Conversation 2 Commits 10 Files Changed 9

+609 -20

timothy commented 2026-07-26 00:40:04 +02:00

Owner

Copy Link

Copy Source

fixes #631

What

Adds a script-tests job to pr-checks.yml that runs PYTHONPATH=. python3 -m pytest scripts/tests -q (111 tests, ~10s) on every PR.

Until now 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 suite guarding the decision corpus, the #610 migration-equivalence harness, the merge-consent exemption logic and the #622 review-verdict poster was enforced only by whoever remembered to run pytest locally.

Deviation from the issue's proposed shape (deliberate, and the load-bearing part)

The issue proposed a step inside decisions-guard as "probably the cheapest home". That is the one place it must not go. 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 exactly the red every session is told to wave through — the gate enforced on paper and inert in practice, which is the same "reports success while doing nothing" family (#603 stale-after, #609 marker, #621 silent record invisibility) that #631 exists to close.

A gate inherits the credibility of the job it lives in, so a job under a standing ignore-rule can host no real gate. Recorded as ci.script-tests-job.

Other decisions

• Unconditional, not scripts/**-filtered. test_post_review_verdict.py and test_merge_consent_exemption.py exec the real scripts/post-review-verdict.sh and .claude/hooks/pretooluse-merge-consent.sh, so the suite's true input set spans at least two top-level directories — a scripts/** filter would silently miss a .claude/hooks/** edit. At ~10s a filter buys nothing but drift.
• Preflight, not install. Those same tests shim curl on PATH but not jq, which the scripts call ~26 times, and no existing small-lane job proves jq is on that image. A preflight step fails with one actionable line instead of ~20 opaque assertion errors. It checks rather than installs because #390 deliberately removed run-time apt-get from CI.
• Not a required status check. main requires only Build & test (.NET), EF migration integrity, review-verdict/h10. This reddens the run; promoting it is a separate branch-protection change.

Verification

• Suite green locally: 111 passed in ~9.7s.
• Negative control run locally: a deliberately failing test makes the exact job command exit 1; restored clean, 111 pass again.
• Workflow YAML parses; all five steps enumerate correctly; preflight logic tested in both directions (present → OK, absent → exit 1).
• The CI-red measurement the Done-when requires (deliberately break one test and confirm CI goes red) is done on a throwaway PR off this branch, linked in a comment below, so this PR's own history stays clean.

Docs

docs/ci-cd.md — new script-tests job section, corrected "three gates" → four, the small-lane git-only paragraph updated to state lightweight-Python jobs as the deliberate edge of that rule, and the docs-only paragraph. New decision record docs/decisions/records/ci/script-tests-job.md + regenerated catalog.

Independent-review note (process.independent-review-rubric): no C#, no locks/auth/API-write-path/migrations — below every trigger. Reviewed cold against the diff in-session; the jq preflight and a factually wrong justification comment about which files the suite reads were both found and fixed that way.

fixes #631 ## What Adds a `script-tests` job to `pr-checks.yml` that runs `PYTHONPATH=. python3 -m pytest scripts/tests -q` (111 tests, ~10s) on every PR. **Until now 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 suite guarding the decision corpus, the #610 migration-equivalence harness, the merge-consent exemption logic and the #622 review-verdict poster was enforced only by whoever remembered to run pytest locally. ## Deviation from the issue's proposed shape (deliberate, and the load-bearing part) The issue proposed a step inside `decisions-guard` as "probably the cheapest home". **That is the one place it must not go.** `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 exactly the red every session is told to wave through — the gate enforced on paper and inert in practice, which is the same "reports success while doing nothing" family (#603 `stale-after`, #609 marker, #621 silent record invisibility) that #631 exists to close. A gate inherits the credibility of the job it lives in, so **a job under a standing ignore-rule can host no real gate.** Recorded as `ci.script-tests-job`. ## Other decisions - **Unconditional, not `scripts/**`-filtered.** `test_post_review_verdict.py` and `test_merge_consent_exemption.py` exec the real `scripts/post-review-verdict.sh` and `.claude/hooks/pretooluse-merge-consent.sh`, so the suite's true input set spans at least two top-level directories — a `scripts/**` filter would silently miss a `.claude/hooks/**` edit. At ~10s a filter buys nothing but drift. - **Preflight, not install.** Those same tests shim `curl` on PATH but **not** `jq`, which the scripts call ~26 times, and no existing small-lane job proves `jq` is on that image. A preflight step fails with one actionable line instead of ~20 opaque assertion errors. It checks rather than installs because #390 deliberately removed run-time `apt-get` from CI. - **Not a required status check.** `main` requires only `Build & test (.NET)`, `EF migration integrity`, `review-verdict/h10`. This reddens the run; promoting it is a separate branch-protection change. ## Verification - Suite green locally: 111 passed in ~9.7s. - **Negative control run locally**: a deliberately failing test makes the exact job command exit **1**; restored clean, 111 pass again. - Workflow YAML parses; all five steps enumerate correctly; preflight logic tested in both directions (present → OK, absent → exit 1). - The **CI-red measurement** the Done-when requires (`deliberately break one test and confirm CI goes red`) is done on a throwaway PR off this branch, linked in a comment below, so this PR's own history stays clean. ## Docs `docs/ci-cd.md` — new `script-tests` job section, corrected "three gates" → four, the `small`-lane git-only paragraph updated to state lightweight-Python jobs as the deliberate edge of that rule, and the docs-only paragraph. New decision record `docs/decisions/records/ci/script-tests-job.md` + regenerated catalog. Independent-review note (`process.independent-review-rubric`): no C#, no locks/auth/API-write-path/migrations — below every trigger. Reviewed cold against the diff in-session; the `jq` preflight and a factually wrong justification comment about which files the suite reads were both found and fixed that way.

timothy added 2 commits 2026-07-26 00:40:04 +02:00

ci(631): run scripts/tests in CI as its own job, not inside the flake-covered decisions-guard ... fedec84997

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

ci(631): preflight jq/git in script-tests, and reflow the header comment ... 8d3fbe215b

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

timothy referenced this pull request 2026-07-26 00:40:32 +02:00

DO NOT MERGE — probe(631): negative control, proves script-tests can go red #638

timothy added 1 commit 2026-07-26 00:44:21 +02:00

ci(631): install pyyaml too — the suite is not pure-stdlib, and CI proved it ... b3d4759054

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

timothy added 1 commit 2026-07-26 01:00:31 +02:00

fix(631): close a fail-open in the merge-consent gate that only CI could see ... 6314646083

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

timothy referenced this pull request 2026-07-26 01:11:42 +02:00

`scripts/tests/` is never executed by CI — the Python test suite is local-only #631

timothy referenced this pull request 2026-07-26 01:40:35 +02:00

Merge-consent gate fails OPEN on jq 1.6: a transport failure mid-pagination lets the docs-only exemption fire over a PARTIAL file list #643

timothy commented 2026-07-26 01:41:00 +02:00

Author

Owner

Copy Link

Copy Source

The merge-consent fail-open found by this PR is now tracked on its own as #643, so the security-posture change to the merge gate is findable without reading a "run the tests in CI" PR. This PR carries the fix; #643 closes when this merges.

The merge-consent fail-open found by this PR is now tracked on its own as **#643**, so the security-posture change to the merge gate is findable without reading a "run the tests in CI" PR. This PR carries the fix; #643 closes when this merges.

timothy referenced this pull request 2026-07-26 01:41:02 +02:00

`scripts/tests/` is never executed by CI — the Python test suite is local-only #631

timothy referenced this pull request 2026-07-26 10:52:33 +02:00

Merge-consent gate fails OPEN on jq 1.6: a transport failure mid-pagination lets the docs-only exemption fire over a PARTIAL file list #643

timothy referenced this pull request 2026-07-26 10:56:00 +02:00

Merge-consent gate fails OPEN on jq 1.6: a transport failure mid-pagination lets the docs-only exemption fire over a PARTIAL file list #643

timothy referenced this pull request 2026-07-26 10:59:20 +02:00

Merge-consent gate fails OPEN on jq 1.6: a transport failure mid-pagination lets the docs-only exemption fire over a PARTIAL file list #643

timothy referenced this pull request 2026-07-26 11:23:52 +02:00

Merge-consent gate fails OPEN on jq 1.6: a transport failure mid-pagination lets the docs-only exemption fire over a PARTIAL file list #643

timothy force-pushed fix/631-script-tests-ci from 30263a66e1 to 289832f233 2026-07-26 12:10:07 +02:00 Compare

timothy referenced this pull request 2026-07-26 12:17:25 +02:00

H10 verdict classifier is inert on jq 1.6: `contains("�")` matches every string, and a parse error is indistinguishable from an empty list #647

timothy force-pushed fix/631-script-tests-ci from 95ea31006e to 39a1313c30 2026-07-26 12:43:44 +02:00 Compare

timothy force-pushed fix/631-script-tests-ci from 39a1313c30 to 37fd30dce7 2026-07-26 13:30:52 +02:00 Compare

timothy commented 2026-07-26 13:56:37 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ 37fd30d

Four independent cold review rounds (one cross-family via Codex, three Opus cold-context, the last two in isolated worktrees). Rounds 3 and 4 both returned MERGEABLE; every finding from all four rounds is applied and mutation-verified. Closes #631 (suite runs in CI as its own job, deliberately NOT inside the flake-covered decisions-guard), #643 (jq-1.6 empty-input fail-open in the merge hook, plus newline-in-path, short-page-as-last-page, head-rebind, array-valued status, and .. components) and #647 (the same jq-1.6 fail-open in review-verdict.yml, the ENFORCED gate, plus contains-NUL true-for-every-string and parse-error-vs-empty). Negative control measured on throwaway PR #638: identical branch plus one broken test turns the job red, everything else green. 216 tests pass under BOTH jq 1.8.2 and the runner's jq 1.6 — verified on every push, since three version divergences surfaced here. CI 11/11 green.

Review-verdict: MERGEABLE @ 37fd30d Four independent cold review rounds (one cross-family via Codex, three Opus cold-context, the last two in isolated worktrees). Rounds 3 and 4 both returned MERGEABLE; every finding from all four rounds is applied and mutation-verified. Closes #631 (suite runs in CI as its own job, deliberately NOT inside the flake-covered decisions-guard), #643 (jq-1.6 empty-input fail-open in the merge hook, plus newline-in-path, short-page-as-last-page, head-rebind, array-valued status, and .. components) and #647 (the same jq-1.6 fail-open in review-verdict.yml, the ENFORCED gate, plus contains-NUL true-for-every-string and parse-error-vs-empty). Negative control measured on throwaway PR #638: identical branch plus one broken test turns the job red, everything else green. 216 tests pass under BOTH jq 1.8.2 and the runner's jq 1.6 — verified on every push, since three version divergences surfaced here. CI 11/11 green.

timothy merged commit 59558e134f into main 2026-07-26 13:56:45 +02:00

timothy deleted branch fix/631-script-tests-ci 2026-07-26 13:56:46 +02:00

timothy referenced this issue from a commit 2026-07-26 13:56:47 +02:00

Merge pull request 'fix(631): run scripts/tests in CI as its own job, not inside the flake-covered decisions-guard' (#637) from fix/631-script-tests-ci into main

timothy referenced this pull request 2026-07-26 13:57:44 +02:00

`scripts/tests/` is never executed by CI — the Python test suite is local-only #631

timothy referenced this pull request 2026-07-26 13:58:21 +02:00

Merge-consent gate fails OPEN on jq 1.6: a transport failure mid-pagination lets the docs-only exemption fire over a PARTIAL file list #643

timothy referenced this pull request 2026-07-26 13:59:51 +02:00

H10 verdict classifier is inert on jq 1.6: `contains("�")` matches every string, and a parse error is indistinguishable from an empty list #647

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

ad-hoc

One-off / ad-hoc work not tracked by a dedicated issue

api

REST API / HTTP endpoints

bug

Something isn't working

ci-cd

Build, test, deploy pipeline

content

Channel content / schedules / playlists

dependencies

Dependency updates (Renovate)

enhancement

New feature or improvement

frontend

ChicoryTV React SPA frontend

in-progress

Claimed by an active session — do not pick up

jellyfin

Jellyfin tuner / IPTV integration

parked

Excluded from automatic queue pickup; work only when explicitly selected

priority: high

priority: low

priority: medium

review

Adversarial review finding

security

Security / vulnerability fix

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate timothy (Timothy)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: timothy/ersatztv#637