timothy/ersatztv
1
0
Fork 0
Code Issues 54 Pull Requests 6 Actions Packages Projects Releases 9 Wiki Activity

post-review-verdict.sh does not check that its own account is on the gate's H10_REVIEWERS allow-list #845

New Issue
Closed
opened 2026-08-26 21:46:10 +02:00 by timothy · 2 comments
timothy commented 2026-08-26 21:46:10 +02:00
Owner
Copy Link
Copy Source

Raised by the cold shell/polarity review of #742.

The coupling

#742 made review-verdict.yml inherit an existing review-verdict/h10 only when .creator.login is in the workflow's H10_REVIEWERS literal (timothy today).

scripts/post-review-verdict.sh is the tool that WRITES those verdicts. It posts with whatever account owns ETV_GITEA_TOKEN / ETV_GITEA_BASICAUTH and never checks whose account that is. The two values are now coupled and nothing asserts the coupling.

The failure

The day the writer credential stops being an allow-listed account — a service token, a second machine, a renamed account, a bot minted for automation — every verdict silently stops being inheritable. post-review-verdict.sh reports success (the status really is written), and then the next pull_request_target event re-derives it and posts pending over the human's success. And again on the next event. The PR deadlocks, and the only diagnostic is a ::warning:: inside a workflow run nobody is reading, because the tool that just ran said it worked.

This is the stall direction, not the fail-open direction — no unreviewed code merges. But it is a repo-wide stall whose cause is two files apart.

Verified state at filing (2026-08-26)

Not a live bug: the token's /api/v1/user login is timothy, the instance has exactly the accounts timothy and renovate, and H10_REVIEWERS="timothy". Nothing keeps that true.

Options

  1. post-review-verdict.sh resolves GET /user after auth and refuses (or warns loudly) when the login is not one the gate accepts. Needs the list in two places, which is the duplication #788 is separately trying to remove for the verdict vocabulary — so consider doing it the way #788 lands rather than adding a second hand-copied list.
  2. Read the posted status back and compare .creator.login against the same source. Stronger (it measures the effect rather than the intent) and it fits the re-read-and-compare the script already does.
  3. A guard that derives both values from their sources and asserts membership, so the drift reddens the suite rather than a PR.

Option 2 plus 3 is the shape this repo usually lands on — assert the EFFECT, and let a guard hold the invariant.

Note on scope

Deliberately NOT bundled into #742: post-review-verdict.sh is on the merge-consent path and changing it is its own risk surface, and #742's ## Done-when did not require it.

Done-when

  • A mismatch between the posting account and the gate's accepted set is detected at the point of the mistake, not discovered as a deadlocked PR
  • The accepted set is not hand-copied into a second file (or, if it must be, a guard fails when the two disagree)
  • The guard/check ships with a mutation proof that it reddens when the writer account is off the list
  • ci.exemption-provenance updated to record the coupling as asserted rather than as a tracked residual
  • Adversarial review passed
Raised by the cold shell/polarity review of #742. ## The coupling #742 made `review-verdict.yml` inherit an existing `review-verdict/h10` only when `.creator.login` is in the workflow's `H10_REVIEWERS` literal (`timothy` today). `scripts/post-review-verdict.sh` is the tool that WRITES those verdicts. It posts with whatever account owns `ETV_GITEA_TOKEN` / `ETV_GITEA_BASICAUTH` and **never checks whose account that is**. The two values are now coupled and nothing asserts the coupling. ## The failure The day the writer credential stops being an allow-listed account — a service token, a second machine, a renamed account, a bot minted for automation — every verdict silently stops being inheritable. `post-review-verdict.sh` reports success (the status really is written), and then the next `pull_request_target` event re-derives it and posts `pending` over the human's `success`. And again on the next event. The PR deadlocks, and the only diagnostic is a `::warning::` inside a workflow run nobody is reading, because the tool that just ran said it worked. This is the *stall* direction, not the fail-open direction — no unreviewed code merges. But it is a repo-wide stall whose cause is two files apart. ## Verified state at filing (2026-08-26) Not a live bug: the token's `/api/v1/user` login is `timothy`, the instance has exactly the accounts `timothy` and `renovate`, and `H10_REVIEWERS="timothy"`. Nothing keeps that true. ## Options 1. `post-review-verdict.sh` resolves `GET /user` after auth and refuses (or warns loudly) when the login is not one the gate accepts. Needs the list in two places, which is the duplication #788 is separately trying to remove for the verdict vocabulary — so consider doing it the way #788 lands rather than adding a second hand-copied list. 2. Read the posted status back and compare `.creator.login` against the same source. Stronger (it measures the effect rather than the intent) and it fits the re-read-and-compare the script already does. 3. A guard that derives both values from their sources and asserts membership, so the drift reddens the suite rather than a PR. Option 2 plus 3 is the shape this repo usually lands on — assert the EFFECT, and let a guard hold the invariant. ## Note on scope Deliberately NOT bundled into #742: `post-review-verdict.sh` is on the merge-consent path and changing it is its own risk surface, and #742's `## Done-when` did not require it. ## Done-when - [x] A mismatch between the posting account and the gate's accepted set is detected at the point of the mistake, not discovered as a deadlocked PR - [x] The accepted set is not hand-copied into a second file (or, if it must be, a guard fails when the two disagree) - [x] The guard/check ships with a mutation proof that it reddens when the writer account is off the list - [x] `ci.exemption-provenance` updated to record the coupling as asserted rather than as a tracked residual - [x] Adversarial review passed
timothy added the ci-cdpriority: mediumsecurity labels 2026-08-26 21:46:10 +02:00
timothy added the in-progress label 2026-08-29 22:33:19 +02:00
timothy commented 2026-08-29 22:33:19 +02:00
Author
Owner
Copy Link
Copy Source

Claiming this (Claude Code / Opus 5 orchestrator session, 2026-08-29). Checked before starting: no open PR references #845, no remote branch names it, no prior claiming comment, origin/main freshly fetched at 736649b3b.

Planned shape is the issue's own recommendation — option 2 + 3: post-review-verdict.sh reads the status back and compares .creator.login, and the accepted set is DEDUPLICATED by construction (one declaration both the workflow and the script derive from) rather than hand-copied and detected. Not bundling #849/#858/#859 into this PR: they are the same cluster, but stacking three edits to the gate on one branch is the shape this repo has repeatedly had to withdraw.

Claiming this (Claude Code / Opus 5 orchestrator session, 2026-08-29). Checked before starting: no open PR references #845, no remote branch names it, no prior claiming comment, `origin/main` freshly fetched at 736649b3b. Planned shape is the issue's own recommendation — option 2 + 3: `post-review-verdict.sh` reads the status back and compares `.creator.login`, and the accepted set is DEDUPLICATED by construction (one declaration both the workflow and the script derive from) rather than hand-copied and detected. Not bundling #849/#858/#859 into this PR: they are the same cluster, but stacking three edits to the gate on one branch is the shape this repo has repeatedly had to withdraw.
timothy commented 2026-08-30 04:43:47 +02:00
Author
Owner
Copy Link
Copy Source

Closing record

Outcome: Shipped. scripts/post-review-verdict.sh now verifies that the verdict it just wrote is one the gate will actually honour, instead of reporting success and leaving the PR to stall one event later. PR: http://192.168.1.95:3000/timothy/ersatztv/pulls/889 (squash-merged as 5d955000f)

Root cause: Two values were coupled with nothing asserting the coupling. review-verdict.yml inherits a review-verdict/h10=success only from a creator on its H10_REVIEWERS literal (#742); post-review-verdict.sh posted with whatever account owned ETV_GITEA_TOKEN/ETV_GITEA_BASICAUTH and never asked whose it was. The failure direction was a stall, never a fail-open — but a silent one, whose only diagnostic was a ::warning:: in a workflow run nobody reads.

Decisions/conventions changed: ci.exemption-provenance updated (Decisions-Edit: yes) — the coupling is recorded as ASSERTED at the writer rather than as a tracked residual, with the derivation, the success-only scope, and the one false-accept (the local-checkout snapshot boundary) named with its direction. No new key.

Reusable knowledge:

  • A fixture derived from the code under test cannot disagree with it. The curl shim replayed the status POST payload as the read-back body. A POST legitimately uses state; a status ROW serialises as status. The pair was self-consistent and both wrong, so reading .state per row — which would have refused every verdict and deadlocked the repo — shipped green. Measured after the fixture was fixed: 25 tests redden; before, zero did. Probing the live server, not re-reading the code, is what found it.
  • A "harmless" fallback can recreate the exact bug being fixed. Adding (.status // .state) as defensiveness meant the writer would accept a shape the gate cannot read and report success — #845 itself, reached through the check meant to prevent it. A verifier must read exactly what the thing it verifies reads.
  • GET /commits/{sha}/status pages, and total_count reports the PAGE, not the total — so a truncated body is indistinguishable from a complete one. But it selects each context's MAX row id and orders those DESCENDING, so a just-posted status is always on page 1. Getting that ORDER wrong in a fixture manufactured a truncation the server cannot produce, and made a ?limit= parameter appear witnessed when its "witness" was circular.
  • The gate's success/failure asymmetry is load-bearing on the writer side too. An existing failure is inherited on attributability alone, so enforcing the allow-list on a rejection refuses a verdict the gate honours and leaves an off-list reviewer no supported way to record one.
  • Check that a mutation actually mutates. Two of this session's disarms were inert — a character class that still excluded *, and a clause matching two sites and applied to neither — and each briefly read as "no test covers this".

Verification: Full scripts/tests suite green (1278 passed, 2 skipped) on the merged head; all PR checks green, including Review verdict / Set review-verdict status, which INHERITED the posted verdict rather than re-deriving it — the membership coupling working end to end in production. The declared mutation executes every run and reddens its named proof with the declared diagnostic (scripts/post-review-verdict.sh moves BEHAVIOUR-ONLY → MUTATION, which the manifest's own UNDECLARED note called "the most valuable upgrade on this list"). 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. jq expressions probed against hostile bodies (non-object array elements, creator as a string, .statuses null/non-array/empty) — all fail closed. NINE independent cold review rounds, all worktree-isolated, one cross-family (GPT-5.6 via Codex); round 9 returned MERGEABLE.

Review rounds — what they cost and what they bought: the CODE was clean from round 4 onward; rounds 5-9 found only prose, and specifically prose trying to carry a number. Two findings were serious. Round 2 (cross-family) caught a (.status // .state) fallback I had added as defensiveness that RECREATED #845 — the writer would accept a shape the gate cannot read and report success. Round 8 caught a set -u "correction" that had INVERTED a true statement in live merge-gate code, because my probe used a plain $UNSET while the validator uses ${#arr[@]}; it was withdrawn wholesale and both libraries are byte-identical to main again. ersatztv#886, filed during that detour asking for a decision record to be "corrected", is CLOSED as invalid — the record was right.

Also fixed here, found by the same rounds: the CI guard test_the_H10_REVIEWERS_list_is_glob_free_and_non_empty parsed the classify step's run: body while the library greps the whole YAML, so a second anchored assignment elsewhere was invisible to the guard while making the writer refuse every verdict; and ci.script-tests-job still enumerated a two-directory input set after this PR raised both derived copies to three.

Deferred: The refused-verdict residual — a non-inheritable status left standing with no comment beside it — is deliberately not repaired here. That is the ask half-state release.verdict-writes-status-before-comment designates as safe, and a second corrective write is the sticky-sentinel mechanism #849 is separately designing. The snapshot boundary (the writer reads the LOCAL workflow while the gate runs the base-resolved one) is accepted with its direction recorded: on a branch that ADDS a reviewer it is a false accept, one PR long, and a stall rather than an unreviewed merge.

Docs updated: docs/decisions/records/ci/exemption-provenance.md (+ regenerated docs/decisions/README.md), docs/ci-cd.md, docs/guard-inventory.md, docs/remote-state-inventory.md, CLAUDE.md, and the script-tests population comment in .gitea/workflows/pr-checks.yml.

## Closing record **Outcome:** Shipped. `scripts/post-review-verdict.sh` now verifies that the verdict it just wrote is one the gate will actually honour, instead of reporting success and leaving the PR to stall one event later. PR: http://192.168.1.95:3000/timothy/ersatztv/pulls/889 (squash-merged as 5d955000f) **Root cause:** Two values were coupled with nothing asserting the coupling. `review-verdict.yml` inherits a `review-verdict/h10=success` only from a creator on its `H10_REVIEWERS` literal (#742); `post-review-verdict.sh` posted with whatever account owned `ETV_GITEA_TOKEN`/`ETV_GITEA_BASICAUTH` and never asked whose it was. The failure direction was a stall, never a fail-open — but a silent one, whose only diagnostic was a `::warning::` in a workflow run nobody reads. **Decisions/conventions changed:** `ci.exemption-provenance` updated (`Decisions-Edit: yes`) — the coupling is recorded as ASSERTED at the writer rather than as a tracked residual, with the derivation, the `success`-only scope, and the one false-accept (the local-checkout snapshot boundary) named with its direction. No new key. **Reusable knowledge:** - **A fixture derived from the code under test cannot disagree with it.** The `curl` shim replayed the status POST payload as the read-back body. A POST legitimately uses `state`; a status ROW serialises as `status`. The pair was self-consistent and both wrong, so reading `.state` per row — which would have refused every verdict and deadlocked the repo — shipped green. Measured after the fixture was fixed: 25 tests redden; before, zero did. Probing the live server, not re-reading the code, is what found it. - **A "harmless" fallback can recreate the exact bug being fixed.** Adding `(.status // .state)` as defensiveness meant the writer would accept a shape the gate cannot read and report success — #845 itself, reached through the check meant to prevent it. A verifier must read exactly what the thing it verifies reads. - **`GET /commits/{sha}/status` pages, and `total_count` reports the PAGE, not the total** — so a truncated body is indistinguishable from a complete one. But it selects each context's MAX row id and orders those DESCENDING, so a just-posted status is always on page 1. Getting that ORDER wrong in a fixture manufactured a truncation the server cannot produce, and made a `?limit=` parameter appear witnessed when its "witness" was circular. - **The gate's `success`/`failure` asymmetry is load-bearing on the writer side too.** An existing `failure` is inherited on attributability alone, so enforcing the allow-list on a rejection refuses a verdict the gate honours and leaves an off-list reviewer no supported way to record one. - **Check that a mutation actually mutates.** Two of this session's disarms were inert — a character class that still excluded `*`, and a clause matching two sites and applied to neither — and each briefly read as "no test covers this". **Verification:** Full `scripts/tests` suite green (1278 passed, 2 skipped) on the merged head; all PR checks green, including `Review verdict / Set review-verdict status`, which INHERITED the posted verdict rather than re-deriving it — the membership coupling working end to end in production. The declared mutation executes every run and reddens its named proof with the declared diagnostic (`scripts/post-review-verdict.sh` moves BEHAVIOUR-ONLY → MUTATION, which the manifest's own `UNDECLARED` note called "the most valuable upgrade on this list"). 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. jq expressions probed against hostile bodies (non-object array elements, `creator` as a string, `.statuses` null/non-array/empty) — all fail closed. NINE independent cold review rounds, all worktree-isolated, one cross-family (GPT-5.6 via Codex); round 9 returned MERGEABLE. **Review rounds — what they cost and what they bought:** the CODE was clean from round 4 onward; rounds 5-9 found only prose, and specifically prose trying to carry a number. Two findings were serious. Round 2 (cross-family) caught a `(.status // .state)` fallback I had added as defensiveness that RECREATED #845 — the writer would accept a shape the gate cannot read and report success. Round 8 caught a `set -u` "correction" that had INVERTED a true statement in live merge-gate code, because my probe used a plain `$UNSET` while the validator uses `${#arr[@]}`; it was withdrawn wholesale and both libraries are byte-identical to `main` again. ersatztv#886, filed during that detour asking for a decision record to be "corrected", is CLOSED as invalid — the record was right. **Also fixed here, found by the same rounds:** the CI guard `test_the_H10_REVIEWERS_list_is_glob_free_and_non_empty` parsed the classify step's `run:` body while the library greps the whole YAML, so a second anchored assignment elsewhere was invisible to the guard while making the writer refuse every verdict; and `ci.script-tests-job` still enumerated a two-directory input set after this PR raised both derived copies to three. **Deferred:** The refused-verdict residual — a non-inheritable status left standing with no comment beside it — is deliberately not repaired here. That is the `ask` half-state `release.verdict-writes-status-before-comment` designates as safe, and a second corrective write is the sticky-sentinel mechanism #849 is separately designing. The snapshot boundary (the writer reads the LOCAL workflow while the gate runs the base-resolved one) is accepted with its direction recorded: on a branch that ADDS a reviewer it is a false accept, one PR long, and a stall rather than an unreviewed merge. **Docs updated:** `docs/decisions/records/ci/exemption-provenance.md` (+ regenerated `docs/decisions/README.md`), `docs/ci-cd.md`, `docs/guard-inventory.md`, `docs/remote-state-inventory.md`, `CLAUDE.md`, and the `script-tests` population comment in `.gitea/workflows/pr-checks.yml`.
timothy removed the in-progress label 2026-08-30 04:43:59 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
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 ci-cd priority: medium security
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
renovate timothy (Timothy)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: timothy/ersatztv#845