fix(649): point the ENFORCED review-verdict gate at the shared PR-file enumeration #666

Merged

timothy merged 4 commits from fix/649-enforced-verdict-guard into main 2026-07-27 00:04:07 +02:00

Conversation 1 Commits 4 Files Changed 4

+501 -91

timothy commented 2026-07-26 23:39:11 +02:00

Owner

Copy Link

Copy Source

fixes #649

The follow-up half of #649. #658 landed scripts/pr-changed-files.sh and rewired the advisory hook onto it; the enforced copy — .gitea/workflows/review-verdict.yml, which writes the branch-protection-required review-verdict/h10 status — was left byte-identical to main. Its fail-closed behaviour on a malformed or empty response therefore remained incidental (an empty n erroring [ "$n" -lt 50 ] to false), which is Done-when box 2 and the whole point of the issue: the gate with real authority was weaker than the gate with none.

What changed

review-verdict.yml now:

• checks out the PR's BASE ref (ref: ${{ github.event.pull_request.base.sha }}, persist-credentials: false), never the head — this job judges the PR, so the PR must not supply the code that judges it;
• runs scripts/jq-preflight.sh in floor-only mode. Never --expect here: this job writes a required check, so an exact pin would turn any jq bump on the runner into a repo-wide merge deadlock;
• calls scripts/pr-changed-files.sh and reads its exit status, never its stdout on a failure path. ~40 lines of inline enumeration deleted, so the two copies can no longer drift.

The env trap flagged in the issue is handled, and then some: the script reads GITEA_BASE_URL and takes owner/repo as two separate arguments. Review pointed out that setting only the GITEA_* names still left this job's config non-authoritative, because the script resolves ETV_GITEA_URL/ETV_GITEA_TOKEN first — a runner exporting a stale ETV_GITEA_URL would have enumerated a different Gitea instance. Both names are now set to the same values, so precedence cannot matter.

A base ref predating #658 has no such script; that posts pending with the reason rather than dying with no status at all.

Verification

Mutation, not a green tick. Nine mutants, each killed by its intended test: ignore the exit status; check out the head; drop persist-credentials; add --expect; re-inline a pulls/N/files? fetch; delete the PROTECTED clause; POST to the wrong host; POST to the wrong repo; delete the URL recorder in the stub.

The caller contract is tested by executing the workflow's run: block, extracted from the YAML, against a stubbed enumeration that fails while emitting a perfectly docs-only list — the one combination the "every failure path also happens to print nothing" redundancy cannot absorb, and the exact mutation that survived the whole suite last round.

Four guards were passing their tests for the wrong reason, all confirmed by deleting the clause and watching the suite stay green:

• The PROTECTED clause. PROTECTED (.claude/ .gitea/ .husky/ scripts/ docker/ci/) and DOCS_ONLY (docs/, root *.md) are disjoint, so on the docs-only path that clause can never fire — DOCS_ONLY was doing all the work. It is load-bearing only on the bot path, so the test now covers a Renovate PR editing the shared script, with a positive control proving the bot exemption fires at all.
• The explicit empty-response clause, uncovered on jq 1.8 because jq 1.8 rejects empty input by itself. jq 1.6 does not, and the runner ships 1.6 — the one environment where the clause is load-bearing had no coverage. That is the #643/#647 failure class reproduced inside the suite meant to prevent it. Now covered by importing the existing jq-1.6 shim (imported, not copied), with a verify-the-verifier test and a positive control.
• type == "array", which needed a body whose values are valid rows. Two earlier attempts failed for a third reason — jq's all(.[]; …) iterates an object's values, so only {"0": {…valid row…}} reaches the fail-open.
• .filename, now isolated by a row with a valid .status and no filename.

Two assertions proved less than their names claimed and were tightened; one test name overclaimed and was narrowed to what it actually proves.

Deferred — pre-existing, filed rather than fixed here

Both were verified against the actual diffs as predating this branch:

• #663 — a commit status is repo-global, so a review-verdict/h10=success earned on one PR is inherited by any other PR with the same head.
• #664 — the head binding detects only one-way movement; an A→B→A force-push restores the expected sha while the pages came from two states.

The decision record now states both limitations narrowly instead of reading as though they are closed.

Docs

Both "Landing note" blocks removed; ci.shared-pr-file-enumeration's base-ref paragraph converted from a future-tense requirement to present-tense fact; docs/ci-cd.md describes the base-ref checkout, the floor-only preflight, and the exit-status contract.

Review

Two rounds of cold cross-family review (Codex), the second re-reviewing the fix commits. Round 2 confirmed the deferrals against three commits' actual content and found the POST-wiring assertion could disable itself two ways — both closed and mutation-verified in e960d5b9.

fixes #649 The follow-up half of #649. #658 landed `scripts/pr-changed-files.sh` and rewired the **advisory** hook onto it; the **enforced** copy — `.gitea/workflows/review-verdict.yml`, which writes the branch-protection-required `review-verdict/h10` status — was left byte-identical to `main`. Its fail-closed behaviour on a malformed or empty response therefore remained **incidental** (an empty `n` erroring `[ "$n" -lt 50 ]` to false), which is Done-when box 2 and the whole point of the issue: the gate with real authority was weaker than the gate with none. ## What changed `review-verdict.yml` now: - **checks out the PR's BASE ref** (`ref: ${{ github.event.pull_request.base.sha }}`, `persist-credentials: false`), never the head — this job judges the PR, so the PR must not supply the code that judges it; - runs `scripts/jq-preflight.sh` in **floor-only** mode. Never `--expect` here: this job writes a *required* check, so an exact pin would turn any jq bump on the runner into a repo-wide merge deadlock; - calls `scripts/pr-changed-files.sh` and reads its **exit status**, never its stdout on a failure path. ~40 lines of inline enumeration deleted, so the two copies can no longer drift. The env trap flagged in the issue is handled, and then some: the script reads `GITEA_BASE_URL` and takes owner/repo as two *separate* arguments. Review pointed out that setting only the `GITEA_*` names still left this job's config **non-authoritative**, because the script resolves `ETV_GITEA_URL`/`ETV_GITEA_TOKEN` first — a runner exporting a stale `ETV_GITEA_URL` would have enumerated a different Gitea instance. Both names are now set to the same values, so precedence cannot matter. A base ref predating #658 has no such script; that posts `pending` with the reason rather than dying with no status at all. ## Verification Mutation, not a green tick. Nine mutants, each killed by its intended test: ignore the exit status; check out the head; drop `persist-credentials`; add `--expect`; re-inline a `pulls/N/files?` fetch; delete the `PROTECTED` clause; POST to the wrong host; POST to the wrong repo; delete the URL recorder in the stub. The caller contract is tested by **executing the workflow's `run:` block**, extracted from the YAML, against a stubbed enumeration that *fails while emitting a perfectly docs-only list* — the one combination the "every failure path also happens to print nothing" redundancy cannot absorb, and the exact mutation that survived the whole suite last round. **Four guards were passing their tests for the wrong reason**, all confirmed by deleting the clause and watching the suite stay green: - The `PROTECTED` clause. `PROTECTED` (`.claude/ .gitea/ .husky/ scripts/ docker/ci/`) and `DOCS_ONLY` (`docs/`, root `*.md`) are **disjoint**, so on the docs-only path that clause can never fire — `DOCS_ONLY` was doing all the work. It is load-bearing only on the **bot** path, so the test now covers a Renovate PR editing the shared script, with a positive control proving the bot exemption fires at all. - The explicit empty-response clause, uncovered on jq 1.8 because jq 1.8 rejects empty input by itself. **jq 1.6 does not, and the runner ships 1.6** — the one environment where the clause is load-bearing had no coverage. That is the #643/#647 failure class reproduced inside the suite meant to prevent it. Now covered by importing the existing jq-1.6 shim (imported, not copied), with a verify-the-verifier test and a positive control. - `type == "array"`, which needed a body whose *values* are valid rows. Two earlier attempts failed for a third reason — `jq`'s `all(.[]; …)` iterates an object's values, so only `{"0": {…valid row…}}` reaches the fail-open. - `.filename`, now isolated by a row with a valid `.status` and no filename. Two assertions proved less than their names claimed and were tightened; one test name overclaimed and was narrowed to what it actually proves. ## Deferred — pre-existing, filed rather than fixed here Both were verified against the actual diffs as predating this branch: - **#663** — a commit status is repo-global, so a `review-verdict/h10=success` earned on one PR is inherited by any other PR with the same head. - **#664** — the head binding detects only *one-way* movement; an A→B→A force-push restores the expected sha while the pages came from two states. The decision record now states both limitations narrowly instead of reading as though they are closed. ## Docs Both "Landing note" blocks removed; `ci.shared-pr-file-enumeration`'s base-ref paragraph converted from a future-tense requirement to present-tense fact; `docs/ci-cd.md` describes the base-ref checkout, the floor-only preflight, and the exit-status contract. ## Review Two rounds of cold cross-family review (Codex), the second re-reviewing the fix commits. Round 2 confirmed the deferrals against three commits' actual content and found the POST-wiring assertion could disable itself two ways — both closed and mutation-verified in `e960d5b9`.

timothy added 4 commits 2026-07-26 23:39:12 +02:00

fix(649): point the ENFORCED review-verdict gate at the shared PR-file enumeration ... 9114a7e8af

#658 landed the shared implementation, `scripts/pr-changed-files.sh`, and rewired the
ADVISORY hook onto it. The ENFORCED copy — the one that writes the branch-protection-
required `review-verdict/h10` status — was left byte-identical to main, so its
fail-closed behaviour on a malformed or empty response stayed INCIDENTAL: an empty `n`
erroring `[ "$n" -lt 50 ]` to false. That is #649's second Done-when box, and the whole
point of the issue was that the gate with real authority was weaker than the gate with
none.

`review-verdict.yml` now:

- checks out the PR's BASE ref (`base.sha`, `persist-credentials: false`), never the
  head, so a PR cannot supply the code that judges it;
- runs `scripts/jq-preflight.sh` in FLOOR-ONLY mode — `--expect` here would deadlock
  every merge on `main` the day the runner's jq changes;
- calls `scripts/pr-changed-files.sh` and reads its EXIT STATUS, never its stdout on a
  failure path. The env trap flagged in review is handled: the script reads
  GITEA_BASE_URL and takes owner/repo as two separate arguments, so passing BASE_URL and
  a combined `owner/repo` would have silently fallen back to the hardcoded LAN default.

The ~40 lines of inline enumeration are deleted, so the two copies can no longer drift.
A base ref predating #658 has no such script; that posts `pending` with the reason
rather than dying with no status at all.

The drift guard is re-tightened from "the hook uses the shared script" to "BOTH callers
do", and the workflow's own preconditions are pinned by parsing the YAML rather than
substring-matching it — `head.sha` for `base.sha` is a nine-character diff.

Verified by mutation, six mutants, each killed by its intended test: ignore the exit
status; check out the head; drop `persist-credentials`; add `--expect`; re-inline a
`pulls/N/files?` fetch; delete the PROTECTED clause.

That last one initially MISSED, and the miss was the useful finding. The test used a
docs-only-plus-protected file list and passed with the clause deleted, because
PROTECTED (`.claude/ .gitea/ .husky/ scripts/ docker/ci/`) and DOCS_ONLY (`docs/`, root
`*.md`) are disjoint — on the docs-only path that clause can never fire, and DOCS_ONLY
was doing all the work. PROTECTED is load-bearing only on the BOT path, so the test now
covers a Renovate PR editing the shared script, with a positive control proving the bot
exemption fires at all.

The caller contract is tested by EXECUTING the workflow's `run:` block against a stubbed
enumeration that fails while emitting a perfectly docs-only list — the one combination
the "every failure path also happens to print nothing" redundancy cannot absorb, and the
exact mutation that survived the whole suite last round.

Docs: both "Landing note" blocks removed, and the record's base-ref paragraph converted
from a future-tense requirement to present-tense fact with its staging rationale kept as
history.

Refs #649

Decisions-Edit: yes

fix(649): close the test-isolation gaps cold review found, and make the job's Gitea config authoritative ... 322dd43d10

Four findings acted on; two more are real but pre-existing and are being filed rather
than fixed here (see below).

**The job's Gitea config was not authoritative.** `pr-changed-files.sh` resolves
`ETV_GITEA_URL` BEFORE `GITEA_BASE_URL` (and `ETV_GITEA_TOKEN` before `GITEA_TOKEN`),
because its other caller is a developer Mac using the ETV_* convention. Setting only the
GITEA_* names meant a runner exporting a stale ETV_GITEA_URL would enumerate a DIFFERENT
Gitea instance and this job would post a verdict here from a diff read there. Both names
are now set to the same value, so precedence cannot matter.

**Three guards passed their tests for the wrong reason.** Each was confirmed by deleting
the clause and watching the suite stay green — the reviewer asserted it, mutation proved
it:

- The explicit empty-response clause was uncovered on jq 1.8, because jq 1.8 rejects
  empty input by itself. jq 1.6 does not, and THE RUNNER SHIPS 1.6 — so the one
  environment where the clause is load-bearing had no coverage. That is the #643/#647
  failure class reproduced inside the suite meant to prevent it. Now covered by importing
  the existing jq-1.6 shim (imported, not copied — a second quirk emulator is the same
  drift problem one level down), with a verify-the-verifier test and a positive control.
- `type == "array"` needed a body whose VALUES are valid rows. Two earlier attempts
  failed for a third reason: `jq`'s `all(.[]; …)` iterates an object's values, so
  `{"message":"…"}` and a single flat row are both rejected by `.filename` erroring on a
  string. Only `{"0": {…valid row…}}` reaches the fail-open, where a non-array body
  enumerates as a complete docs-only list.
- `.filename | ok` is now isolated by a row carrying a valid `.status` and no filename,
  removing the closed-allow-list as a second reason to reject.

**Two assertions proved less than their names claimed.** `"jq-preflight.sh" in code` also
matched the `[ -x … ]` presence guard, so deleting the invocation left it green; it now
requires an invoking line. `_run_classify` accepted every POST, so a status aimed at the
wrong endpoint or sha would not have been noticed; it now asserts the POST lands on
`/statuses/<full head sha>`.

**One test name overclaimed** and is narrowed rather than left implying coverage it does
not have: the head-movement test proves "final head != expected sha", not movement
*during* enumeration.

Deferred, both pre-existing and neither introduced here — filed as follow-ups:
- A commit status is repo-global, so a `review-verdict/h10=success` obtained for head H
  on one PR is inherited by any other PR with the same head, including one opened against
  a different base. Same class as #632, reached by a third route.
- The A->B->A force-push race: paging is several round-trips and the head is re-read once
  at the end, so a restore to the original sha passes the binding while the pages came
  from two states. Inherent to enumerating a mutable list over an API with no
  commit-pinned files endpoint.

Refs #649

docs(649): narrow the enumeration's stated guarantees to what it actually proves ... 3885fd6aea

Two limitations the cold review surfaced are now written where the guarantees are
described, rather than living only in a review transcript. Both are pre-existing and
tracked separately (#663, #664); neither is fixed here.

- Head-sha binding detects ONE-WAY movement. An A->B->A force-push round trip restores
  the expected sha, so the binding holds while the pages came from two states. The record
  previously read as though the race were closed.
- A commit status is repo-GLOBAL, so a success earned on one PR is inherited by any other
  PR with the same head. Same property that makes the per-sha binding work, read from the
  other end.

Refs #649

Decisions-Edit: yes

test(649): make the POST-wiring assertion unable to opt out or accept the wrong host ... e960d5b918

Re-review found the verifier could disable itself two ways, both of which look like
coverage:

- it was guarded by `if url_file.exists()`, so deleting the recorder in the stub turned
  the whole assertion into a no-op and every test stayed green;
- it compared only the URL SUFFIX, so a POST to the right path on the wrong HOST or the
  wrong REPO passed — which is exactly the class the assertion was added to catch.

It now requires the URL to have been recorded whenever a status was posted, and compares
the full URL against the env the job was given. Mutation-verified three ways: wrong host,
wrong repo, and deleting the recorder each redden the suite.

Refs #649

timothy commented 2026-07-26 23:45:12 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ e960d5b

Three rounds of cold cross-family review (Codex). Round 1 found 8 issues; round 2 re-reviewed the fixes and found 2 more in the wiring assertion; round 3 found none in the remaining delta and confirmed the recorder-deletion and full-URL mutations are meaningfully detected.

Nine mutants killed, each by its intended test. Four guards were found passing for the WRONG reason and fixed - most importantly the empty-response clause, which was uncovered on jq 1.8 because jq 1.8 rejects empty input itself; the runner ships jq 1.6, where the clause is load-bearing, and that was the one environment with no coverage.

Live-verified on this PR's own run: 'Changed files (4, complete=yes)' and 'Decision: state=pending - touches a protected path', so the shared script ran from the base checkout and returned exit 0 rather than silently failing closed (both produce an identical pending status). The same log confirms 'jq-preflight: jq version in use = jq-1.6'.

Two pre-existing holes were deferred rather than fixed, verified against the actual diffs as predating this branch: #663 (a commit status is repo-global) and #664 (head binding detects one-way movement only). The decision record states both narrowly.

Review-verdict: MERGEABLE @ e960d5b Three rounds of cold cross-family review (Codex). Round 1 found 8 issues; round 2 re-reviewed the fixes and found 2 more in the wiring assertion; round 3 found none in the remaining delta and confirmed the recorder-deletion and full-URL mutations are meaningfully detected. Nine mutants killed, each by its intended test. Four guards were found passing for the WRONG reason and fixed - most importantly the empty-response clause, which was uncovered on jq 1.8 because jq 1.8 rejects empty input itself; the runner ships jq 1.6, where the clause is load-bearing, and that was the one environment with no coverage. Live-verified on this PR's own run: 'Changed files (4, complete=yes)' and 'Decision: state=pending - touches a protected path', so the shared script ran from the base checkout and returned exit 0 rather than silently failing closed (both produce an identical pending status). The same log confirms 'jq-preflight: jq version in use = jq-1.6'. Two pre-existing holes were deferred rather than fixed, verified against the actual diffs as predating this branch: #663 (a commit status is repo-global) and #664 (head binding detects one-way movement only). The decision record states both narrowly.

timothy merged commit 8de02d5bde into main 2026-07-27 00:04:07 +02:00

timothy deleted branch fix/649-enforced-verdict-guard 2026-07-27 00:04:07 +02:00

timothy referenced this issue from a commit 2026-07-27 00:04:08 +02:00

Merge pull request 'fix(649): point the ENFORCED review-verdict gate at the shared PR-file enumeration' (#666) from fix/649-enforced-verdict-guard into main

timothy referenced this pull request 2026-07-27 00:04:41 +02:00

The ENFORCED review-verdict.yml guard is weaker than the advisory local hook — duplicated security logic has drifted #649

timothy referenced this pull request 2026-07-27 00:06:39 +02:00

The ENFORCED review-verdict.yml guard is weaker than the advisory local hook — duplicated security logic has drifted #649

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

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

timothy referenced this issue from a commit 2026-07-27 00:26:36 +02:00

test(649): cover the review-verdict status read, and the guards that only fire on the bot path

timothy referenced this issue from a commit 2026-07-27 07:41:03 +02:00

test(649): cover the review-verdict status read, and the guards that only fire on the bot path

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 referenced this pull request 2026-07-27 07:46:30 +02:00

review-verdict.yml is head-resolved, so a PR editing it can self-approve the required check #672

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