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

post-review-verdict.sh exits 0 when it deliberately writes NO status #792

New Issue
Closed
opened 2026-08-14 01:44:55 +02:00 by timothy · 2 comments
timothy commented 2026-08-14 01:44:55 +02:00
Owner
Copy Link
Copy Source

Found live during #774's own merge, and it is the failure class that repo's decision records keep naming.

scripts/post-review-verdict.sh 791 MERGEABLE printed:

posted comment: Review-verdict: MERGEABLE @ 57030d5
post-review-verdict: could not re-read PR #791 to confirm the head and base had not moved while posting — no status was written. Re-run once Gitea is reachable.

and exited 0.

The fail-closed behaviour itself is CORRECT and should not change — Gitea was slow (the call ran past 120s; it was answering in ~140ms minutes later), the TOCTOU re-read failed, and refusing to write a status it could not bind safely is exactly right per ci.verdict-write-retarget-fence.

The defect is the exit code. The script did not do the thing its caller asked, and reported success. Consequences:

  • Any automation or agent checking $? concludes the verdict was posted. It was not.
  • The half-completed state is actively misleading: the comment exists, the status does not. Someone eyeballing the PR sees Review-verdict: MERGEABLE @ <head> and reasonably infers the gate is satisfied — but the comment is not the gate, the sha-bound review-verdict/h10 status is, and it was still pending. This is release.verdict-status-check's "a comment alone leaves it absent", reached by a new route.
  • I only caught it because I read the write back instead of trusting the exit code.

Not a security hole — the merge gate reads the STATUS, so it correctly kept denying; the system stayed fail-closed end to end. It is a lie to the operator, on the script whose entire job is making consent derivable rather than assertable.

Suggested fix

Exit non-zero on any path that posts no status. Consider also making the comment+status write atomic-ish (status first, or delete/annotate the orphaned comment on failure) so the two cannot disagree — a re-run currently leaves two identical verdict comments, which is untidy but harmless.

Done-when

  • A path that writes no status exits non-zero
  • A test covers it (the script already has a stubbed-curl harness in scripts/tests/test_post_review_verdict.py that can simulate the failed re-read)
  • Decide and record whether an orphaned comment should be prevented or tolerated
  • Adversarial review passed
Found live during #774's own merge, and it is the failure class that repo's decision records keep naming. `scripts/post-review-verdict.sh 791 MERGEABLE` printed: ``` posted comment: Review-verdict: MERGEABLE @ 57030d5 post-review-verdict: could not re-read PR #791 to confirm the head and base had not moved while posting — no status was written. Re-run once Gitea is reachable. ``` **and exited 0.** The fail-closed behaviour itself is CORRECT and should not change — Gitea was slow (the call ran past 120s; it was answering in ~140ms minutes later), the TOCTOU re-read failed, and refusing to write a status it could not bind safely is exactly right per `ci.verdict-write-retarget-fence`. **The defect is the exit code.** The script did not do the thing its caller asked, and reported success. Consequences: - Any automation or agent checking `$?` concludes the verdict was posted. It was not. - The half-completed state is actively misleading: the **comment** exists, the **status** does not. Someone eyeballing the PR sees `Review-verdict: MERGEABLE @ <head>` and reasonably infers the gate is satisfied — but the comment is not the gate, the sha-bound `review-verdict/h10` status is, and it was still `pending`. This is `release.verdict-status-check`'s "a comment alone leaves it absent", reached by a new route. - I only caught it because I read the write back instead of trusting the exit code. Not a security hole — the merge gate reads the STATUS, so it correctly kept denying; the system stayed fail-closed end to end. It is a lie to the operator, on the script whose entire job is making consent derivable rather than assertable. ## Suggested fix Exit non-zero on any path that posts no status. Consider also making the comment+status write atomic-ish (status first, or delete/annotate the orphaned comment on failure) so the two cannot disagree — a re-run currently leaves two identical verdict comments, which is untidy but harmless. ## Done-when - [x] A path that writes no status exits non-zero - [x] A test covers it (the script already has a stubbed-`curl` harness in `scripts/tests/test_post_review_verdict.py` that can simulate the failed re-read) - [x] Decide and record whether an orphaned comment should be prevented or tolerated - [x] Adversarial review passed
timothy added the ci-cdpriority: high labels 2026-08-14 01:44:55 +02:00
timothy added the in-progress label 2026-08-22 22:19:16 +02:00
timothy commented 2026-08-22 22:19:17 +02:00
Author
Owner
Copy Link
Copy Source

Claiming this (Claude Code session, 2026-08-22), bundled with #772 — see the claim comment there. Pre-claim checks per process.parallel-session-claim all clear: no open PR referencing it, no remote branch naming it, no prior comments, fresh git fetch origin main at 29d7a06e6.

Claiming this (Claude Code session, 2026-08-22), bundled with #772 — see the claim comment there. Pre-claim checks per `process.parallel-session-claim` all clear: no open PR referencing it, no remote branch naming it, no prior comments, fresh `git fetch origin main` at 29d7a06e6.
timothy commented 2026-08-23 01:06:45 +02:00
Author
Owner
Copy Link
Copy Source

Closing record

Outcome: SHIPPED — PR #825, but not the fix this issue asked for, because the premise did not survive measurement.

Root cause: two separate things, and the issue conflated them.

  1. The exit code was never the defect. Every path that writes no status already exited non-zero on the current tree — die exits 1, the usage path 2, a failing jq its own status under set -e+pipefail, a signal 128+n. The eight refusal modes are now driven through the real entry point and asserted, and those assertions pass against origin/main's script, which is the evidence: the defect was not in the script. The observed 0 came from the invocation around it (a pipeline reports its last command's status). The contract is asserted anyway — it held by convention through one shared helper and nothing had ever executed it.
  2. The half-state was real, and it is the one this issue named second. The comment was written BEFORE the status, so every refusal left Review-verdict: MERGEABLE @ <head> on a PR with no gating status behind it — an artifact that reads as granted consent, produced by the run that refused to grant it.

Decisions/conventions changed: added release.verdict-writes-status-before-comment — the status is written first, the comment second, and an orphaned comment is PREVENTED rather than tolerated (the issue's third Done-when box). Rollback was the option rejected: deleting or annotating the orphan needs a Gitea call, and these refusals are usually caused by Gitea being unreachable, so the compensating write is unavailable exactly when it is needed. Ordering costs nothing and cannot fail to apply. The docs/remote-state-inventory.md residual that pointed here is closed.

Reusable knowledge:

  • The two half-states are asymmetric, and that asymmetry is the whole argument. Comment-without-status reads as consent and leaves only the required check between it and a merge. Status-without-comment leaves the hook's condition (c) with nothing to classify → ask. Order the writes so the survivor is the safe one.
  • One qualification worth carrying: condition (c) reads the whole comment history, not this run's write. If a positive verdict for the SAME head already exists it still satisfies (c) — correct, since that comment covers that sha.
  • A reported defect is a hypothesis. This one was filed from a live observation by a careful reader and was still wrong about the mechanism. Re-measure before fixing, and prove the retraction the same way you would prove a fix: by running the tests against the real predecessor and showing which ones do NOT go red.

Verification: scripts/tests 910 passed / 2 skipped. The ordering is proved by the predecessor split — against origin/main's script the 8 orphan/ordering tests go red and the 8 exit-code tests stay green. Dogfooded on this PR's own merge: the script wrote review-verdict/h10 first and the comment second, and the status was read back before merging.

Deferred: none for this issue.

Docs updated: new record docs/decisions/records/release/verdict-writes-status-before-comment.md, catalog regenerated, docs/remote-state-inventory.md row updated.

## Closing record **Outcome:** SHIPPED — PR #825, but **not the fix this issue asked for**, because the premise did not survive measurement. **Root cause:** two separate things, and the issue conflated them. 1. **The exit code was never the defect.** Every path that writes no status already exited non-zero on the current tree — `die` exits 1, the usage path 2, a failing `jq` its own status under `set -e`+`pipefail`, a signal 128+n. The eight refusal modes are now driven through the real entry point and asserted, and **those assertions pass against `origin/main`'s script**, which is the evidence: the defect was not in the script. The observed `0` came from the invocation around it (a pipeline reports its last command's status). The contract is asserted anyway — it held by convention through one shared helper and nothing had ever executed it. 2. **The half-state was real, and it is the one this issue named second.** The comment was written BEFORE the status, so every refusal left `Review-verdict: MERGEABLE @ <head>` on a PR with no gating status behind it — an artifact that reads as granted consent, produced by the run that refused to grant it. **Decisions/conventions changed:** added `release.verdict-writes-status-before-comment` — the status is written first, the comment second, and an orphaned comment is PREVENTED rather than tolerated (the issue's third Done-when box). Rollback was the option rejected: deleting or annotating the orphan needs a Gitea call, and these refusals are usually *caused* by Gitea being unreachable, so the compensating write is unavailable exactly when it is needed. Ordering costs nothing and cannot fail to apply. The `docs/remote-state-inventory.md` residual that pointed here is closed. **Reusable knowledge:** - **The two half-states are asymmetric, and that asymmetry is the whole argument.** Comment-without-status reads as consent and leaves only the required check between it and a merge. Status-without-comment leaves the hook's condition (c) with nothing to classify → `ask`. Order the writes so the survivor is the safe one. - One qualification worth carrying: condition (c) reads the whole comment history, not this run's write. If a positive verdict for the SAME head already exists it still satisfies (c) — correct, since that comment covers that sha. - **A reported defect is a hypothesis.** This one was filed from a live observation by a careful reader and was still wrong about the mechanism. Re-measure before fixing, and prove the retraction the same way you would prove a fix: by running the tests against the real predecessor and showing which ones do NOT go red. **Verification:** `scripts/tests` 910 passed / 2 skipped. The ordering is proved by the predecessor split — against `origin/main`'s script the 8 orphan/ordering tests go red and the 8 exit-code tests stay green. Dogfooded on this PR's own merge: the script wrote `review-verdict/h10` first and the comment second, and the status was read back before merging. **Deferred:** none for this issue. **Docs updated:** new record `docs/decisions/records/release/verdict-writes-status-before-comment.md`, catalog regenerated, `docs/remote-state-inventory.md` row updated.
timothy removed the in-progress label 2026-08-23 01:06:46 +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: high
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#792