Files
ersatztv/scripts/tests
timothyandClaude Opus 5 edd8d3d9c9 fix(751): review round — the twin null-shape, a governance self-exemption, and four over-claims
Two independent cold reviews (a cross-family GPT-5.6 pass and an isolated Opus pass).
Neither found a path to a green `review-verdict/h10` on an unreviewed head. Both found
real defects BESIDE the fix, which is the failure mode this file keeps producing.

THE TWIN, and the reason not to trust "I fixed the two I could see". `GET
/commits/{sha}/status` returns `statuses: null` — not `[]` — for a head with no
statuses yet: `{"state":"pending","total_count":0,"statuses":null}`, measured on PR
#739's head. `read_existing_verdict` gated on `.statuses | type == "array"` and took
its `exit 1` path, posting NOTHING. Fail-closed, but the user-visible outcome is the
one this issue is about: an exempt PR with no status and, since #743, no bypass. Its
double printed `{"statuses": []}` at all three no-verdict sites, so that branch was
unreachable in the suite — the same unfaithful-double story as the timeline, one
function over. `null` is accepted only when `total_count` is 0, so a body that merely
lost its array is still refused and an existing verdict is still protected. Swept
`scripts/pr-changed-files.sh` too: `pulls/{n}/files` returns `[]`, unaffected. The
generalisable rule is that a nil Go slice serialises to `null`, so every list-shaped
field on this API is suspect and only a per-endpoint measurement settles it.

A GOVERNANCE SELF-EXEMPTION, reachable again precisely because this change works.
`DOCS_ONLY` matched `CLAUDE.md` and `AGENTS.md` — the documents that DEFINE the
completion protocol, the merge-consent convention and the H10 rule. Driving the real
classify body with a lone `CLAUDE.md` change produced `review-verdict/h10=success`.
Protecting `.claude/` while the file specifying what it enforces stayed exemptible is
the same self-exemption the header rules out, one directory over. Both added to
PROTECTED; `README.md` deliberately not (ordinary prose, no enforcement).

FOUR OVER-CLAIMS, corrected rather than defended:

* The repo-wide expression test does NOT catch "any payload that cannot evaluate".
  It checks the HEAD TOKEN of each dotted path. `${{ github.ref == }}` and
  `${{ …head.sha + }}` pass; so does a renamed output, since tokens after the first
  are skipped by design. Claim corrected in the docstring, `docs/ci-cd.md` and the
  record. The test is kept permissive on purpose: a red here blocks every merge.
* The strict test's anti-vacuity half banned expressions ANYWHERE outside
  `with:`/`env:`, so the standard `if: ${{ always() }}` spelling and even a delimiter
  in an inert top-level comment went red — a guard more dangerous than its target.
  Replaced with the honest property: the YAML walk saw every `run:` body it declares.
* The `if:` assertion demanded the bare `always()` exactly; now normalised, since the
  wrapped form is identical to the runner.
* `exit 1` was matched anywhere in the guard body, so an unreachable
  `if false; then exit 1; fi` satisfied it while the real branch said `exit 0`. Now
  required INSIDE the missing-marker branch — and the new behavioural test settles it
  properly by EXECUTING the guard body both ways.
* The record asserted a repo-wide obligation to guard consequential steps. It is not
  repo-wide: `docker-build.yml`'s `test`/`migrations` are also required contexts and a
  dropped step there is fail-OPEN (green having done no work), strictly worse than
  here. Scoped to this file and tracked as #756 rather than asserted as done.

Also: comments in both files still said it was unestablished whether a later step runs
after a drop — runs 1863/1866 established it, so they now record the measurement; a
cited test name that never existed; `kind` leaked to global scope; a mangled comment
wrap; and an already-false "one event on page 1".

Hardening of my own: `null` now counts as exhaustion only from page 2 ON. Every real
PR's first page carries events (4, 2, 9, 5, 10 across #752/#753/#749/#739/#717), so a
terminator on page 1 means no page was ever read, and certifying "no retarget" from a
response we cannot explain is the one thing the fence exists to refuse. Narrows rather
than closes it: a wrong `null` on page 3 still reads as exhaustion.

Verification: 137 in this file / 452 total green; ELEVEN mutations each red —
reintroducing the defect, deleting the guard, deleting the marker write, removing
`if: always()`, `exit 1`→`exit 0`, a delimiter in the guard body, the fence gate (20
red), the TWIN gate (70 red), dropping the governance paths, accepting a null first
page, and diverging the marker path between the two steps.

Refs: #751
Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-10 19:22:50 +02:00
..