test(649): cover the review-verdict status read and the bot-path guards #673

Merged

timothy merged 2 commits from test/649-workflow-body-coverage into main 2026-07-27 18:22:48 +02:00

Conversation 3 Commits 2 Files Changed 1

+150 -2

timothy commented 2026-07-27 00:26:35 +02:00

Owner

Copy Link

Copy Source

Tests only — no behaviour change. Adds coverage for properties #666 shipped correctly but left unguarded, so a future edit could regress them silently.

Context, stated plainly

I started implementing #649 in parallel, unaware #666 was in flight, and got a long way before a review caught that origin/main had moved. That branch was discarded rather than pushed — it would have reverted #666 and #667. #666's implementation is better than mine in at least one respect I got wrong: it exports ETV_GITEA_URL as well, because pr-changed-files.sh reads that at higher precedence than GITEA_BASE_URL.

What survived is the part #666 does not have: coverage of the classify step's remaining branches.

What was unguarded

The stub's status read was hardcoded to {"statuses": []} — "no verdict yet" — so two whole branches were unreachable from any test. Mutations that survived the full suite:

Mutation	Effect
drop the shell emptiness check	re-introduces the literal #647 fail-open
if [ "$existing" = ... ] → if false	overwrites an existing human verdict
drop the = "failure" half	overwrites a human BLOCKED verdict specifically
grep -c . → grep -c ''	an empty enumeration grants a bot PR success
grep -qxF → grep -qF	author ova gets renovate's exemption
DOCS_ONLY loses its $	evil.mdx reads as docs-only

The bot-author detail is the interesting one

test_an_EMPTY_enumeration_is_not_exempt_even_for_a_BOT only tests anything because the author is a bot. With a non-bot, the blank line an empty list produces already fails DOCS_ONLY, so count -eq 0 never decides the outcome — it would be a duplicate of an existing test wearing a security docstring.

That is the same short-circuit this file already documents for PROTECTED/DOCS_ONLY disjointness. Worth noting it recurred in a different predicate: whenever an earlier elif can satisfy the case, the branch you meant to test is unreachable.

One mutation cannot be caught locally

Dropping the shell emptiness check leaves the empty case to jq -e's exit status: 4 on jq 1.8 (guard still fires, test still passes on a dev Mac) and 0 on the runner's jq 1.6, where it is the actual bug. So the behavioural test catches it in CI and not locally.

A structural assertion closes the local gap — with comments stripped first, because a raw substring search is satisfiable by moving the guard into a comment while deleting the real one. Verified by doing exactly that.

Verification

scripts/tests: 333 passed. Six mutations applied to review-verdict.yml individually, five caught behaviourally and the sixth structurally; workflow restored byte-identical each time.

Not included, deliberately

A branches: [main] filter on on: pull_request would close a further hole (the base-ref trust argument is "a PR cannot push to its base branch", true for main and false elsewhere). Left out to keep this tests-only and avoid a second parallel edit to the same workflow — the failure mode this PR's own history demonstrates. Noted on #672, which covers the dominating case.

Tests only — no behaviour change. Adds coverage for properties #666 shipped **correctly** but left unguarded, so a future edit could regress them silently. ## Context, stated plainly I started implementing #649 in parallel, unaware #666 was in flight, and got a long way before a review caught that `origin/main` had moved. That branch was discarded rather than pushed — it would have reverted #666 and #667. #666's implementation is better than mine in at least one respect I got wrong: it exports `ETV_GITEA_URL` as well, because `pr-changed-files.sh` reads that at *higher* precedence than `GITEA_BASE_URL`. What survived is the part #666 does not have: coverage of the classify step's remaining branches. ## What was unguarded The stub's status read was hardcoded to `{"statuses": []}` — "no verdict yet" — so two whole branches were unreachable from any test. Mutations that survived the full suite: | Mutation | Effect | |---|---| | drop the shell emptiness check | re-introduces the literal #647 fail-open | | `if [ "$existing" = ... ]` → `if false` | overwrites an existing human verdict | | drop the `= "failure"` half | overwrites a human **BLOCKED** verdict specifically | | `grep -c .` → `grep -c ''` | an empty enumeration grants a **bot** PR `success` | | `grep -qxF` → `grep -qF` | author `ova` gets `renovate`'s exemption | | `DOCS_ONLY` loses its `$` | `evil.mdx` reads as docs-only | ## The bot-author detail is the interesting one `test_an_EMPTY_enumeration_is_not_exempt_even_for_a_BOT` only tests anything because the author is a bot. With a non-bot, the blank line an empty list produces already fails `DOCS_ONLY`, so `count -eq 0` never decides the outcome — it would be a duplicate of an existing test wearing a security docstring. That is the same short-circuit this file already documents for `PROTECTED`/`DOCS_ONLY` disjointness. Worth noting it recurred in a different predicate: **whenever an earlier `elif` can satisfy the case, the branch you meant to test is unreachable.** ## One mutation cannot be caught locally Dropping the shell emptiness check leaves the empty case to `jq -e`'s exit status: **4 on jq 1.8** (guard still fires, test still passes on a dev Mac) and **0 on the runner's jq 1.6**, where it is the actual bug. So the behavioural test catches it in CI and not locally. A structural assertion closes the local gap — with comments stripped first, because a raw substring search is satisfiable by moving the guard into a comment while deleting the real one. Verified by doing exactly that. ## Verification `scripts/tests`: **333 passed**. Six mutations applied to `review-verdict.yml` individually, five caught behaviourally and the sixth structurally; workflow restored byte-identical each time. ## Not included, deliberately A `branches: [main]` filter on `on: pull_request` would close a further hole (the base-ref trust argument is "a PR cannot push to its base branch", true for `main` and false elsewhere). Left out to keep this tests-only and avoid a second parallel edit to the same workflow — the failure mode this PR's own history demonstrates. Noted on #672, which covers the dominating case.

timothy force-pushed test/649-workflow-body-coverage from 33c84abede to 2a2dcacd58 2026-07-27 07:41:03 +02:00 Compare

timothy commented 2026-07-27 07:43:21 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ 2a2dcac

Independently verified in both directions rather than taken on report.

Ran all six mutations against origin/main WITHOUT this PR: 6 of 6 SURVIVED, confirming the classify step's status-read branches were unreachable from my suite - the stub hardcoded {"statuses": []}, so no test could make the #647 emptiness guard or the never-overwrite short-circuit fire. With this PR applied: 0 of 6 survive, each killed by its named test. Full suite 333 green.

Also verified the subtlest claim directly, since it is the kind that is usually asserted and wrong: flipping ONLY the author in test_an_EMPTY_enumeration_is_not_exempt_even_for_a_BOT from renovate to timothy MASKS the grep -c '' mutation. So the bot author is genuinely load-bearing, exactly as the docstring says - the blank line an empty list produces already fails DOCS_ONLY on the non-bot path, so count -eq 0 never decides the outcome there.

The generalisation in the PR body is the durable part and I agree with it: whenever an earlier elif can satisfy the case, the branch under test is unreachable. That is the third distinct instance tonight (PROTECTED/DOCS_ONLY disjointness, jq's all(.[]) over an object's values, and now this), which argues it belongs in the reviewer checklist rather than in three separate docstrings.

The structural assertion for the jq-1.6-only mutation is the right call: stripping comments before the substring search closes the 'a comment mentioning it is not a call' vacuity this same file had to fix for the drift guard. Honest about being a local-gap closer rather than a substitute for the behavioural test.

No findings.

Review-verdict: MERGEABLE @ 2a2dcac Independently verified in both directions rather than taken on report. Ran all six mutations against origin/main WITHOUT this PR: 6 of 6 SURVIVED, confirming the classify step's status-read branches were unreachable from my suite - the stub hardcoded {"statuses": []}, so no test could make the #647 emptiness guard or the never-overwrite short-circuit fire. With this PR applied: 0 of 6 survive, each killed by its named test. Full suite 333 green. Also verified the subtlest claim directly, since it is the kind that is usually asserted and wrong: flipping ONLY the author in test_an_EMPTY_enumeration_is_not_exempt_even_for_a_BOT from renovate to timothy MASKS the grep -c '' mutation. So the bot author is genuinely load-bearing, exactly as the docstring says - the blank line an empty list produces already fails DOCS_ONLY on the non-bot path, so count -eq 0 never decides the outcome there. The generalisation in the PR body is the durable part and I agree with it: whenever an earlier elif can satisfy the case, the branch under test is unreachable. That is the third distinct instance tonight (PROTECTED/DOCS_ONLY disjointness, jq's all(.[]) over an object's values, and now this), which argues it belongs in the reviewer checklist rather than in three separate docstrings. The structural assertion for the jq-1.6-only mutation is the right call: stripping comments before the substring search closes the 'a comment mentioning it is not a call' vacuity this same file had to fix for the drift guard. Honest about being a local-gap closer rather than a substitute for the behavioural test. No findings.

timothy referenced this pull request 2026-07-27 07:44:52 +02:00

docs: permit subagents explicitly, and make claiming an issue a check rather than a label #682

timothy commented 2026-07-27 07:46:05 +02:00

Author

Owner

Copy Link

Copy Source

Disclosure about the verdict above — it was posted against a sha I had not read at the time.

I reviewed 33c84abe. Between finishing that review and running post-review-verdict.sh, this branch was rebased onto the newer main (which had picked up #678), so the script bound the verdict to 2a2dcac. It did not refuse, and correctly so — its TOCTOU guard covers the window between its own two reads, not the window between a human's review and their invocation of it. The convention's real protection is "re-review the current head before posting", and that step was mine to perform. I didn't.

Re-verified against 2a2dcac after noticing, which is what the verdict should have rested on:

• the PR's own contribution is byte-identical to what I reviewed (git diff main...head on the test file compares equal across both bases — the 3,200-line delta between the two shas is main's #651/#678 work arriving underneath, not new work here);
• all six mutations re-run on the certified head: 0 of 6 survive;
• full suite on the certified head: 333 passed.

So the verdict stands on evidence rather than on the rebase being harmless-looking. Leaving the original comment in place rather than editing it, because the sequence is the useful part.

Worth noting what this is an instance of: a verdict outliving what it was granted for — the same family as #622 (head moves), #632 (base moves), and #663 (a different PR inherits it). This is the human-latency variant: the reviewer's eyes and the reviewer's command observe two different heads. It is not fixable by another guard in the script, since the script cannot know what a human read; the mitigation is to re-read the head immediately before posting, which is now the thing I'd change about my own routine rather than about the tooling.

**Disclosure about the verdict above — it was posted against a sha I had not read at the time.** I reviewed `33c84abe`. Between finishing that review and running `post-review-verdict.sh`, this branch was rebased onto the newer `main` (which had picked up #678), so the script bound the verdict to `2a2dcac`. It did not refuse, and correctly so — its TOCTOU guard covers the window between its *own* two reads, not the window between a human's review and their invocation of it. The convention's real protection is "re-review the current head before posting", and that step was mine to perform. I didn't. **Re-verified against `2a2dcac` after noticing**, which is what the verdict should have rested on: - the PR's own contribution is byte-identical to what I reviewed (`git diff main...head` on the test file compares equal across both bases — the 3,200-line delta between the two shas is *main's* #651/#678 work arriving underneath, not new work here); - all six mutations re-run on the certified head: **0 of 6 survive**; - full suite on the certified head: **333 passed**. So the verdict stands on evidence rather than on the rebase being harmless-looking. Leaving the original comment in place rather than editing it, because the sequence is the useful part. Worth noting what this *is* an instance of: a verdict outliving what it was granted for — the same family as #622 (head moves), #632 (base moves), and #663 (a different PR inherits it). This is the human-latency variant: the reviewer's eyes and the reviewer's command observe two different heads. It is not fixable by another guard in the script, since the script cannot know what a human read; the mitigation is to re-read the head immediately before posting, which is now the thing I'd change about my own routine rather than about the tooling.

timothy added 1 commit 2026-07-27 08:01:53 +02:00

test(649): cold-review fixes — the never-overwrite test skipped the case its docstring called sharpest ... 7ed0a59c56

Four gaps, all found by mutation rather than reading.

The never-overwrite test used only a NON-EXEMPT file list, so "an exemption posted over a human
BLOCKED verdict" — the scenario its own docstring named as the sharp one — was asserted nowhere.
Moving the short-circuit to after classification, gated on non-exempt, survived the whole suite
while turning a human rejection into a green required status for both a docs-only PR and a bot PR.
Now parametrised over non-exempt, docs-only-exempt and bot-exempt file lists.

The structural emptiness pin is REPLACED by a real jq-1.6 behavioural test. Its stated
justification — "no behavioural test can catch this on a dev machine" — was simply false: this
file already imports _JQ16_SHIM for pr-changed-files.sh, so the runner's quirk is reproducible
here. The structural version was also weaker than it looked, stripping only FULL-LINE comments, so
leaving the literal as a trailing comment on the surviving guard satisfied it while the real check
was gone. The behavioural test catches that mutant and needs no comment-stripping.

The status-read stub now returns DECOY contexts either side of the verdict row, so dropping
`select(.context == $c)` is caught. First attempt gave the decoys `status: success`, which triggers
the same short-circuit as a real verdict — the mutation still produced an identical outcome and
survived. `pending` decoys make mis-selection observable.

DOCS_ONLY's `^` anchor is now covered alongside its `$`: losing it exempts ErsatzTV/docs/Evil.cs,
a C# file, and is fail-OPEN.

Two remaining survivors are documented in-file as behaviourally equivalent, not gaps: `first` ->
`last` (the combined endpoint returns one row per context by contract, so a two-row fixture would
test a fiction), and the garbage-response test defending the type guard only by redundancy.

timothy commented 2026-07-27 08:19:01 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ 7ed0a59

Re-review of 7ed0a59c. Head confirmed unchanged immediately before posting this time - the previous verdict on 2a2dcac was posted against a sha I had not read, and the disclosure is above.

This revision is stronger than the one I approved, and the self-catch is the substantive part. Verified rather than accepted:

• Eight mutations run against the new head, 0 survive: the shell emptiness check, select(.context == c), DOCS_ONLY losing its ^ and losing its , the never-overwrite guard and its = failure half, grep -c '' and grep -qF. Full suite 338 green.
• The claim that the SUPERSEDED structural test was satisfiable by a trailing comment is CORRECT - confirmed by deleting the real guard while leaving the literal as a trailing comment on another line, which the old test survived. Worth noting my first attempt to check this was a broken verifier: putting the comment mid-condition broke the shell syntax, so the behavioural tests failed and it looked CAUGHT. Had I stopped there I would have published the opposite conclusion.
• Replacing that structural test with a behavioural one under the jq-1.6 shim is the right call, and the reasoning that retired it - 'no behavioural test can catch this locally' was false because the shim already exists in this file - is a better justification than the one I accepted last round.
• The pending decoy contexts are the sharp detail: a success decoy would trigger the same short-circuit as a real verdict, so the select mutation would have been invisible. Confirmed by mutation.
• The 'two mutations NOT covered and why' comment is accurate on both counts, including that the type guard is fail-closed by redundancy under set -e rather than by its own assertion.

No findings.

Review-verdict: MERGEABLE @ 7ed0a59 Re-review of 7ed0a59c. Head confirmed unchanged immediately before posting this time - the previous verdict on 2a2dcac was posted against a sha I had not read, and the disclosure is above. This revision is stronger than the one I approved, and the self-catch is the substantive part. Verified rather than accepted: - Eight mutations run against the new head, 0 survive: the shell emptiness check, select(.context == $c), DOCS_ONLY losing its ^ and losing its $, the never-overwrite guard and its = failure half, grep -c '' and grep -qF. Full suite 338 green. - The claim that the SUPERSEDED structural test was satisfiable by a trailing comment is CORRECT - confirmed by deleting the real guard while leaving the literal as a trailing comment on another line, which the old test survived. Worth noting my first attempt to check this was a broken verifier: putting the comment mid-condition broke the shell syntax, so the behavioural tests failed and it looked CAUGHT. Had I stopped there I would have published the opposite conclusion. - Replacing that structural test with a behavioural one under the jq-1.6 shim is the right call, and the reasoning that retired it - 'no behavioural test can catch this locally' was false because the shim already exists in this file - is a better justification than the one I accepted last round. - The pending decoy contexts are the sharp detail: a success decoy would trigger the same short-circuit as a real verdict, so the select mutation would have been invisible. Confirmed by mutation. - The 'two mutations NOT covered and why' comment is accurate on both counts, including that the type guard is fail-closed by redundancy under set -e rather than by its own assertion. No findings.

timothy merged commit aeff810cad into main 2026-07-27 18:22:48 +02:00

timothy deleted branch test/649-workflow-body-coverage 2026-07-27 18:22:48 +02:00

timothy referenced this issue from a commit 2026-07-27 18:22:49 +02:00

Merge pull request 'test(649): cover the review-verdict status read and the bot-path guards' (#673) from test/649-workflow-body-coverage into main

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#673