fix(751): a stray expression delimiter in a COMMENT killed the verdict gate — plus the two nil-slice twins and a dead-code guard it hid #764

2026-08-10T21:09:35+02:00

timothy commented

2026-08-10 21:09:35 +02:00

fixes #751

review-verdict.yml's classify step stopped executing on 2026-08-03 and the job reported success anyway. So review-verdict/h10 — the branch-protection-required status — was posted by nothing but a human hand for three days, and both exemption classes silently stopped working.

Three defects, not one

Each of these alone is sufficient to stop the gate posting. The first two shipped in the same commit (8f6d4f443); the third has been latent much longer.

1. A stray expression delimiter, in a shell comment. The #706 note explaining why a concurrency group does not work here quoted a concurrency: snippet containing a PR-number expression as an illustration. A run: body is not shell when the runner reads it: it scans the whole scalar for the expression opener and rewrites the entire body into one format(...) call. That rewrite is all-or-nothing, so pr number — not a valid expression — fails the interpolation of the whole scalar, and the runner drops the step and concludes the job success. The prose documenting a fix disabled the fix.

2. The retarget fence never trusted its count. Found only by running the probe. A page past the end of /issues/{n}/timeline is JSON null, not []. count_retargets gated on type == "array", so it read the real terminator as unreadable, never reached a validated empty page, and rt_ok was never yes for any PR — so the fence withheld every exemption success. Fixing defect 1 alone would not have restored the exemptions.

3. The same nil-slice shape on /commits/{sha}/status. Found by cold review of the fix for 2. A head with no statuses returns {"state":"pending","total_count":0,"statuses":null}. read_existing_verdict gated on array and took its exit 1 path, posting nothing.

The generalisable rule: 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. /issues/{n}/comments returns [] when empty; pulls/{n}/files returns [] (so scripts/pr-changed-files.sh is unaffected — swept and verified).

The silent green is the actual defect

An absent required status reads as "not reviewed yet", which is indistinguishable from the correct pending state — so an ordinary PR looked ordinary while the gate was dead. The cost landed only where no human is in the loop:

PR	Class	Outcome
#739 docs-only	exempt	merged 08-05 with zero commit statuses on its head, via admin force-merge
#717, #718	exempt (renovate)	unaffected by timing only — merged minutes before the bad commit
6 others	non-exempt	fine; humans posted verdicts by hand

#743 disabled admin force-merge on 08-06. The escape that let #739 through is gone, so the next docs-only or Renovate-manifest PR would have been permanently stuck with no bypass.

So the fix is at the boundary, not the site:

A start-marker guard (if: always()) fails the job when the classify step did not execute — for any drop cause, not just interpolation. It asserts execution started, never completed (the classifier has legitimate exit 0 abstention paths). Its own body is expression-free, because a guard the guarded mechanism can delete is worse than none.
Static guards: no delimiter in any run: body of this file (absolute — a dropped step here is a dead merge gate, and its bodies are ~700 lines of prose), and repo-wide, every expression payload's head token must name a real context or function.

Note every pre-existing workflow-shape test reads _code_lines(), which strips comments — structurally blind to a bug living in a comment. The new strict test reads the raw scalar and must never adopt _code_lines.

Measured on the live instance, with a negative control

Scratch-base probe pair (#752 positive, #753 negative), re-run against the final body.

Negative control — the defect reintroduced:

Unable to interpolate expression 'format('set -euo pipefail\n\n# NEVER WRITE AN EXPRESSION DELIMITER…
(no ⭐ Run Main Classify… — step dropped)
❌  Failure - Main Assert the classifier actually executed
🏁  Job failed

This settles the question the original report could not: a later step does still run after an earlier one is dropped (the classifier used to be the job's last step, so there was never a subsequent step to observe). The guard is load-bearing, and the body does not need to move into scripts/.

Positive:

Retarget fence: 0 retarget event(s) observed before classifying (trusted=yes).
Decision: state=success — docs-only change (no code, no protected path)
Posted probe751/h10=success on c0b526b.
The classify step executed (start marker present at /tmp/h10-classifier-started-1880-1).
🏁  Job succeeded

Both controls were re-run against 5072d0811 itself, not an earlier body — an earlier negative control had been taken before the marker was keyed on the run id, so that evidence no longer described what would merge.

A governance self-exemption, fixed here because this change re-opens it

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).

For the same reason, #706's known residual returns with the working fence — while rt_ok was never yes, route 1 was closed by accident. Said plainly rather than presenting this as pure repair.

Verification

At 20b117dab: 460 tests green, and 29 mutations across six rounds each land as intended. The load-bearing ones: reintroducing the exact defect; deleting the guard step; deleting only the marker write; removing if: always(); exit 1→exit 0 and the same outside the missing-marker branch; a delimiter in the guard's own body; reverting the fence type gate (20 red); reverting the twin statuses: null gate (48 red); dropping the governance paths; accepting a null first page; diverging the marker path between the two steps; hiding the marker write behind each of function mk { and function mk() {; deleting the page-2 completeness probe; accepting an unreadable or garbage page 2 on either endpoint; a walk that drops the short steps; and — as controls that must not red — an inert top-level YAML comment and a legitimate step deletion.

Five cold review rounds, alternating families (GPT-5.6 Sol and Opus, each from a cold review-only brief in an isolated worktree). No round found a path to a green review-verdict/h10 on an unreviewed head. Every round found real defects beside the fix, which is this file's recurring failure mode:

Round	Found
1	the twin `statuses: null` shape; the `CLAUDE.md` self-exemption; four over-claims
2	a truncation hole — and holes in both tests written to close round 1
3	that round 2's guard was dead code (it compared against a page limit of 100; the server caps at 50 — documented in three places in this repo already)
4	the stale base (this branch would have reverted #760); the fail-OPEN twin in the post-write race check
5	a stated rationale that was itself vacuous; a third round on one regex; MERGEABLE

Three guards were written for one hole and the first two were no-ops. total_count is per-page here, so length-vs-total is equal by construction; "refuse on a full page at 100" was unreachable at a cap of 50. The working version asks the server: read page 2 when the row is absent from page 1. Cap-independent, so no reconfiguration re-breaks it.

Probe evidence applies to this exact head. The round-6 probes ran against e133c11fd, and the round-5 commit changed only comments: the workflow's executable content is byte-identical (360 executable lines, and a sha256 over the non-comment run: bodies plus if/with/env matches). Verified, not assumed.

Also corrected

Four over-claims found by review, corrected rather than defended — the repo-wide expression test's reach (it checks head tokens only; ${{ github.ref == }} passes), the strict test's anti-vacuity half (it banned expressions anywhere outside with:/env:, false-redding the standard if: ${{ always() }} spelling), an exit 1 matched anywhere rather than inside the missing-marker branch, and a record asserting a repo-wide obligation the repo does not meet. Plus: ci.verdict-write-retarget-fence described the universal failure as a narrow over-cap residual; a cited decision key (ci.paged-endpoint-completeness) that has never existed; a cited test name that never existed; kind leaking to global scope; and comments still calling the guard's behaviour unestablished after runs 1863/1866 established it.

Rebase

Rebased onto 9881d1ff8 (#760), which landed mid-review. The tell was the one CLAUDE.md documents — git diff origin/main HEAD showing deletions I never made (records/mcp/tool-schema-openapi-parity.md, docs/mcp.md). The generated catalog was regenerated rather than trusted to the rebase and verified byte-identical to the generator's output; 9881d1ff8 is an ancestor and none of its content differs.

Deliberately deferred

ersatztv#763 — /statuses/{sha} is read twice without paging while limit clamps to 50, so the post-write race check can miss a raced verdict on a >50-row head. Mitigated here conservatively (unread or unreadable page 2 → assume raced → repair to pending), which is blunt: on such a head it removes that sha's exemption until a human posts a verdict. Real paging replaces it there. Reachable but not reached — a live probe head carried 33 rows against the cap of 50.

ersatztv#756 — docker-build.yml's test and migrations jobs are also required contexts, and there a dropped step is fail-OPEN: the required check goes green having done no work, strictly worse than an absent status (compare #684). This PR guarded the file where the failure mode is safe, because that is where the live bug was. Currently latent (neither job has a delimiter in a run: body today), which is exactly when it is cheap.

Docs

docs/ci-cd.md (Review-verdict gate), a new record ci.workflow-run-body-no-expressions, and amendments to ci.verdict-write-retarget-fence + ci.exemption-provenance.

fixes #751 `review-verdict.yml`'s classify step stopped executing on **2026-08-03** and the job reported `success` anyway. So `review-verdict/h10` — the branch-protection-**required** status — was posted by nothing but a human hand for three days, and both exemption classes silently stopped working. ## Three defects, not one Each of these alone is sufficient to stop the gate posting. The first two shipped in the same commit (8f6d4f443); the third has been latent much longer. **1. A stray expression delimiter, in a shell comment.** The #706 note explaining why a concurrency group does not work here quoted a `concurrency:` snippet containing a PR-number expression *as an illustration*. A `run:` body is not shell when the runner reads it: it scans the whole scalar for the expression opener and rewrites the **entire** body into one `format(...)` call. That rewrite is all-or-nothing, so `pr number` — not a valid expression — fails the interpolation of the whole scalar, and **the runner drops the step and concludes the job `success`**. The prose documenting a fix disabled the fix. **2. The retarget fence never trusted its count.** Found only by running the probe. A page past the end of `/issues/{n}/timeline` is JSON `null`, not `[]`. `count_retargets` gated on `type == "array"`, so it read the real terminator as *unreadable*, never reached a validated empty page, and `rt_ok` was never `yes` **for any PR** — so the fence withheld **every** exemption `success`. Fixing defect 1 alone would not have restored the exemptions. **3. The same nil-slice shape on `/commits/{sha}/status`.** Found by cold review of the fix for 2. A head with no statuses returns `{"state":"pending","total_count":0,"statuses":null}`. `read_existing_verdict` gated on `array` and took its `exit 1` path, posting nothing. The generalisable rule: **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.** `/issues/{n}/comments` returns `[]` when empty; `pulls/{n}/files` returns `[]` (so `scripts/pr-changed-files.sh` is unaffected — swept and verified). ## The silent green is the actual defect An absent required status reads as "not reviewed yet", which is indistinguishable from the correct pending state — so an ordinary PR looked ordinary while the gate was dead. The cost landed only where no human is in the loop: | PR | Class | Outcome | |---|---|---| | #739 docs-only | exempt | **merged 08-05 with zero commit statuses on its head**, via admin force-merge | | #717, #718 | exempt (renovate) | unaffected **by timing only** — merged minutes before the bad commit | | 6 others | non-exempt | fine; humans posted verdicts by hand | **#743 disabled admin force-merge on 08-06.** The escape that let #739 through is gone, so the next docs-only or Renovate-manifest PR would have been permanently stuck with no bypass. So the fix is at the boundary, not the site: - A **start-marker guard** (`if: always()`) fails the job when the classify step did not execute — for *any* drop cause, not just interpolation. It asserts execution *started*, never completed (the classifier has legitimate `exit 0` abstention paths). Its own body is expression-free, because a guard the guarded mechanism can delete is worse than none. - **Static guards**: no delimiter in *any* `run:` body of this file (absolute — a dropped step here is a dead merge gate, and its bodies are ~700 lines of prose), and repo-wide, every expression payload's *head token* must name a real context or function. Note every pre-existing workflow-shape test reads `_code_lines()`, which strips comments — structurally blind to a bug living in a comment. The new strict test reads the raw scalar and must never adopt `_code_lines`. ## Measured on the live instance, with a negative control Scratch-base probe pair (#752 positive, #753 negative), re-run against the **final** body. Negative control — the defect reintroduced: ``` Unable to interpolate expression 'format('set -euo pipefail\n\n# NEVER WRITE AN EXPRESSION DELIMITER… (no ⭐ Run Main Classify… — step dropped) ❌ Failure - Main Assert the classifier actually executed 🏁 Job failed ``` This settles the question the original report could not: **a later step does still run after an earlier one is dropped** (the classifier used to be the job's last step, so there was never a subsequent step to observe). The guard is load-bearing, and the body does **not** need to move into `scripts/`. Positive: ``` Retarget fence: 0 retarget event(s) observed before classifying (trusted=yes). Decision: state=success — docs-only change (no code, no protected path) Posted probe751/h10=success on c0b526b. The classify step executed (start marker present at /tmp/h10-classifier-started-1880-1). 🏁 Job succeeded ``` Both controls were re-run against **`5072d0811` itself**, not an earlier body — an earlier negative control had been taken before the marker was keyed on the run id, so that evidence no longer described what would merge. ## A governance self-exemption, fixed here because this change re-opens it `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). For the same reason, **#706's known residual returns with the working fence** — while `rt_ok` was never `yes`, route 1 was closed by accident. Said plainly rather than presenting this as pure repair. ## Verification At `20b117dab`: **460 tests green**, and **29 mutations across six rounds** each land as intended. The load-bearing ones: reintroducing the exact defect; deleting the guard step; deleting only the marker write; removing `if: always()`; `exit 1`→`exit 0` and the same outside the missing-marker branch; a delimiter in the guard's own body; reverting the fence type gate (**20 red**); reverting the twin `statuses: null` gate (**48 red**); dropping the governance paths; accepting a `null` first page; diverging the marker path between the two steps; hiding the marker write behind each of `function mk {` and `function mk() {`; deleting the page-2 completeness probe; accepting an unreadable or garbage page 2 on either endpoint; a walk that drops the short steps; and — as controls that must **not** red — an inert top-level YAML comment and a legitimate step deletion. **Five cold review rounds**, alternating families (GPT-5.6 Sol and Opus, each from a cold review-only brief in an isolated worktree). **No round found a path to a green `review-verdict/h10` on an unreviewed head.** Every round found real defects *beside* the fix, which is this file's recurring failure mode: | Round | Found | |---|---| | 1 | the twin `statuses: null` shape; the `CLAUDE.md` self-exemption; four over-claims | | 2 | a truncation hole — **and holes in both tests written to close round 1** | | 3 | that round 2's guard was **dead code** (it compared against a page limit of 100; the server caps at 50 — documented in three places in this repo already) | | 4 | the **stale base** (this branch would have reverted #760); the fail-OPEN twin in the post-write race check | | 5 | a stated rationale that was **itself vacuous**; a third round on one regex; MERGEABLE | **Three guards were written for one hole and the first two were no-ops.** `total_count` is per-*page* here, so length-vs-total is equal by construction; "refuse on a full page at 100" was unreachable at a cap of 50. The working version asks the server: read page 2 when the row is absent from page 1. Cap-independent, so no reconfiguration re-breaks it. **Probe evidence applies to this exact head.** The round-6 probes ran against `e133c11fd`, and the round-5 commit changed only comments: the workflow's executable content is byte-identical (360 executable lines, and a sha256 over the non-comment `run:` bodies plus `if`/`with`/`env` matches). Verified, not assumed. ## Also corrected Four over-claims found by review, corrected rather than defended — the repo-wide expression test's reach (it checks head tokens only; `${{ github.ref == }}` passes), the strict test's anti-vacuity half (it banned expressions *anywhere* outside `with:`/`env:`, false-redding the standard `if: ${{ always() }}` spelling), an `exit 1` matched anywhere rather than inside the missing-marker branch, and a record asserting a repo-wide obligation the repo does not meet. Plus: `ci.verdict-write-retarget-fence` described the universal failure as a narrow over-cap residual; a cited decision key (`ci.paged-endpoint-completeness`) that **has never existed**; a cited test name that never existed; `kind` leaking to global scope; and comments still calling the guard's behaviour unestablished after runs 1863/1866 established it. ## Rebase Rebased onto `9881d1ff8` (#760), which landed mid-review. The tell was the one `CLAUDE.md` documents — `git diff origin/main HEAD` showing deletions I never made (`records/mcp/tool-schema-openapi-parity.md`, `docs/mcp.md`). The generated catalog was **regenerated** rather than trusted to the rebase and verified byte-identical to the generator's output; `9881d1ff8` is an ancestor and none of its content differs. ## Deliberately deferred **ersatztv#763** — `/statuses/{sha}` is read twice without paging while `limit` clamps to 50, so the post-write race check can miss a raced verdict on a >50-row head. Mitigated here conservatively (unread or unreadable page 2 → assume raced → repair to `pending`), which is blunt: on such a head it removes that sha's exemption until a human posts a verdict. Real paging replaces it there. Reachable but not reached — a live probe head carried 33 rows against the cap of 50. **ersatztv#756** — `docker-build.yml`'s `test` and `migrations` jobs are also required contexts, and there a dropped step is **fail-OPEN**: the required check goes green having done no work, strictly worse than an absent status (compare #684). This PR guarded the file where the failure mode is *safe*, because that is where the live bug was. Currently latent (neither job has a delimiter in a `run:` body today), which is exactly when it is cheap. ## Docs `docs/ci-cd.md` (Review-verdict gate), a new record `ci.workflow-run-body-no-expressions`, and amendments to `ci.verdict-write-retarget-fence` + `ci.exemption-provenance`.

timothy added 7 commits 2026-08-10 21:09:36 +02:00

fix(751): a stray expression delimiter in a COMMENT killed the verdict gate 2bdb6c44e4

`review-verdict.yml`'s classify step stopped executing on 2026-08-03 and the job
reported `success` anyway, so `review-verdict/h10` — the branch-protection-required
status — was posted by nothing but a human hand for three days, and both exemption
classes (Renovate-manifest, docs-only) silently stopped working.

The cause is one token in prose. The #706 note explaining why a concurrency group
does not work here quoted a `concurrency:` snippet containing a PR-number expression
as an ILLUSTRATION, inside a shell comment. A shell comment is not inert there: the
runner scans the whole `run:` scalar for the expression opener before bash sees it,
and one occurrence makes it rewrite the ENTIRE body into a single `format(...)` call.
That rewrite is all-or-nothing, so a payload that does not parse — `pr number` does
not — fails the interpolation of the whole scalar, and the runner then DROPS THE STEP
AND CONCLUDES THE JOB GREEN. The prose documenting a fix disabled the fix.

`git blame`/`git log -S` put the line in 8f6d4f443 (2026-08-03), which dates the
outage precisely rather than "present in run 1832, not bisected further back".

Three changes, deliberately different in kind:

* The prose no longer writes the delimiters. It names the expression instead.
* The classifier writes a start marker and a new `if: always()` step FAILS THE JOB
  when it is missing. This is the half that generalises: the delimiter was one bug in
  one comment, but a dropped step concluding `success` is what made it cost three
  days behind a green tick. It asserts execution STARTED, never completed — the
  classifier has several legitimate `exit 0` abstention paths.
* Two static guards in scripts/tests/test_pr_changed_files.py: no expression
  delimiter in ANY `run:` body of the gate file (absolute, because a dropped step
  here is a dead merge gate and its bodies are ~700 lines of prose), and repo-wide,
  every expression payload must name a real context or function (permissive, because
  other workflows interpolate into `run:` legitimately). The second catches the class
  — a payload that cannot evaluate, wherever it appears.

Note every pre-existing workflow-shape test reads `_code_lines()`, which strips
comments. That is correct for what it was for, but it encodes the assumption this bug
falsifies: inside a `run:` scalar a comment CAN change behaviour. The new strict test
reads the raw scalar for that reason and must never adopt `_code_lines`.

Blast radius, audited: PR #739 (docs-only) merged 2026-08-05 with ZERO commit
statuses on its head, and got in only because admin force-merge was still enabled.
#743 disabled that on 2026-08-06, so the workaround that absorbed this bug is gone —
the next docs-only or Renovate-manifest PR would be permanently stuck. The two
Renovate PRs in the window escaped by timing, merging minutes before the bad commit.
Non-exempt PRs were unaffected throughout: humans posted their verdicts by hand.

Verification: all six mutations red, restored tree green — reintroducing the exact
defect (strict + general tests), deleting the guard step, deleting only the marker
write, weakening `if: always()`, turning the guard's `exit 1` into `exit 0`, and
putting a delimiter in the guard's own body. 129 passed on the fixed tree.

Still to establish, and NOT claimed here: that the runner executes a LATER step after
dropping an earlier one. The #751 evidence cannot say — the classifier was the job's
last step, so there was never a subsequent step to observe. A scratch-base probe with
a negative control answers it next; if the runner drops the rest of the steps too,
this guard is inert and the body has to move into `scripts/`.

Refs #751

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

fix(751): the fence never trusted its count — a page past the end is null, not [] 40b3747434

The scratch-base probe found a SECOND, independent reason `review-verdict/h10` was
never posted automatically. Fixing the dropped step alone would NOT have restored the
exemptions.

`count_retargets` pages `/issues/{n}/timeline` and trusts its count only on a
validated empty page, gated on `type == "array"`. But a page past the end of that
endpoint is the JSON value `null` — measured at Gitea 1.27.1 on PR #752, four bytes —
so the real terminator read as UNREADABLE. The walk never reached a validated empty
page, `rt_ok` was never `yes` for ANY pull request, and the fence therefore withheld
EVERY exemption `success`. Renovate and docs-only PRs got no status at all: the same
user-visible outcome as the dropped step, by a completely unrelated route.

The instance is not consistent between endpoints — `/issues/{n}/comments` returns `[]`
when empty — so both shapes terminate the walk now, and the regression test is
parameterised over both. The type is read as a VALUE (`case` over `jq -r 'type'`)
rather than through `jq -e`, whose exit-status semantics already bit this workflow at
jq 1.6 (#647).

TWO REASONS THIS LOOKED DELIBERATE RATHER THAN BROKEN, both worth generalising:

* It had never run. This fence shipped in 8f6d4f443 — the same commit whose prose
  comment stopped the classify step executing at all. Merging a guard and first
  executing it are different events, and only the second tells you anything.
* The test double asserted the wrong shape while claiming to be measured. Its comment
  read "Real shapes, measured on this instance and deliberately mirrored" and it
  printed `[]` past the end, so the `array`-only gate was never exercised by the suite
  either. Correcting the double and restoring the old gate turns 18 TESTS RED — every
  one of them had been green for the wrong reason. A fidelity claim in a double is an
  assertion and it decays like any other.

The new test asserts the POSTED STATUS, not the log: on the real probe run the log
said `Decision: state=success` and the job still posted nothing, so the decision and
the write are separate events and only the write is what a merge reads.

Also here, both found while editing this code:

* `ci.verdict-write-retarget-fence` stated this as a narrow residual ("a timeline over
  the 20-page cap can never be exempted") when the behaviour was universal. Corrected
  in place rather than left as a checked-looking claim that talks the next reader out
  of verifying.
* The workflow cited `ci.paged-endpoint-completeness`, a key that has never existed as
  a record anywhere. Repointed at the record that actually owns this walk.

Marker hardening from the probe: `RUNNER_TEMP` is `/tmp` on this runner, not a private
per-job directory, so the start marker is now keyed on the run id and attempt. The
lane starts a container per job today, which makes a fixed name fresh in practice, but
that is a property of the lane and a stale marker would make the guard PASS on a run
whose step was dropped — the exact silent pass it exists to remove.

Verification: 446 passed; M7 (revert only the type gate, keep the corrected double) →
18 red. Probe evidence in the issue.

Refs: #751
Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

fix(751): review round — the twin null-shape, a governance self-exemption, and four over-claims edd8d3d9c9

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>

fix(751): re-review round — a truncation hole, and the tests that closed findings needed closing 46ec532745

Cross-family re-review of the previous fix commit. It did NOT pass, and it was right
not to: the round that fixed the reviewers' findings introduced two of its own, both in
the tests written to close them. That is this file's recurring shape, and it is the
reason the fix commit gets re-reviewed rather than the initial diff only.

TRUNCATION (High). `read_existing_verdict` asks for 100 statuses and never checked
whether the page was full. If a head ever carried more contexts than that, an existing
`review-verdict/h10` could fall off page 1, the job would conclude no verdict exists,
and it could post an exemption `success` over a human `failure` — the worst thing this
gate can do. Six contexts exist today, so this guards a future shape, not a live bug.

BUT THE PROPOSED GUARD WAS A NO-OP, and measuring is what showed it. The review asked
for `.statuses | length` compared against `.total_count`. On this instance `total_count`
is the count for the PAGE RETURNED, not for the commit: on 3aed43c6 (6 contexts),
`?limit=1` gives `len=1, total_count=1` and `?limit=3` gives `len=3, total_count=3`.
The two are equal by construction, so that check would have read as a completeness
proof while proving nothing — and it would have been the second guard in this file to
look like a check and not be one. What IS observable is a page at the requested limit,
which means "maybe more", so that is now treated as unreadable: post nothing, leave the
required check absent. The stub mirrors the per-page `total_count` deliberately, so the
new test cannot pass for the wrong reason either.

THE TESTS THAT CLOSED THE LAST ROUND'S FINDINGS:

* The behavioural guard test — added to answer "a bare `exit 1` substring is satisfiable
  by dead code" — extracted the two marker lines BY TEXT and ran them alone. That
  passes even if the write is moved into a function nobody calls: the extractor finds
  the text, runs it at top level, the marker appears, and the test reports the guard
  proven while production writes no marker. It now executes the classify body's real
  PREFIX down to and including the write, which reproduces the production control flow
  instead of a reconstruction of it. Mutation: move the write into an uncalled function
  -> RED (it previously passed).
* The anti-vacuity check — added to replace an over-broad assertion — hand-counted
  `run:` keys with a regex that only matched an indented `run:` starting `|` or `>`. It
  false-redded legal spellings (`- run: |`, a single-line `run: echo ok`) and could
  count a `run: |` sitting inside a heredoc. Hand-parsing YAML to validate a YAML parse
  is the wrong shape: it adds a second, worse parser whose every disagreement is a
  false alarm, and a red here blocks all merges. Now asserted on CONTENT — the walk
  reached >=3 bodies and one over 5000 chars.

FALSE RED, THIRD INSTANCE IN THIS FILE. The repo-wide expression test scanned raw file
text, so a delimiter in an inert top-level YAML comment redded the repo even though the
runner never evaluates it. It now scans PARSED scalars: PyYAML drops YAML comments,
while a `run:` body is itself a scalar and keeps its SHELL comments — which is exactly
the distinction that matters, since inside a `run:` scalar a comment is not inert.
Verified in both directions: an inert top-level comment passes, the same payload in a
run-body comment still reds.

Also: the `total_count` zero check now requires the JSON TYPE to be number — `jq -r`
renders `0` and `"0"` identically, so a text compare accepted a corrupted
`"total_count": "0"` as "no statuses".

Verification: 455 green. Six further mutations, each landing as intended — the uncalled
function (red), an inert YAML comment (PASSES, no false red), the same payload in a
run-body comment (red), accepting a full status page (red), comparing total_count as
text (red), and breaking the YAML walk's job key (red). Seventeen mutations across the
three rounds.

Re-probe of both live controls follows on this body; the previous probe evidence was
taken before this commit and no longer describes what would merge.

Refs: #751
Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

fix(751): the truncation guard I added was DEAD CODE — the page cap is 50, not 100 951dae26a9

Third review round, cut short by a transport hang after ~11h, but it had already found
the thing that mattered: the guard added last round could never fire.

`read_existing_verdict` asks for `limit=100` and refused when the page came back with
100 rows. This instance caps `limit` at the server-wide `MAX_RESPONSE_ITEMS`, MEASURED
AT 50 — `/issues?limit=100` returns 50 items. A response can therefore never carry 100
rows, so the comparison was unreachable and the hole it was written for was still open.

The sting is that the repo already knew. `scripts/pr-changed-files.sh`, two test files
and `ci.script-tests-job` all document that Gitea caps `limit` at `MAX_RESPONSE_ITEMS`
(50 in the PR #619 measurement). The review found it by grepping this codebase, not
upstream. Writing a guard against a constant the repo had already measured as wrong is
the same failure as the unfaithful test double two rounds ago: a number believed rather
than checked.

So this is now the THIRD guard for one hole, and the first two were both no-ops:

  1. `.statuses | length` vs `.total_count` — `total_count` is the count for the PAGE
     RETURNED, not the commit (`?limit=1` on a 6-context head gives
     `len=1, total_count=1`). Equal by construction.
  2. "refuse when the page is full at 100" — dead code, as above.
  3. Ask the server. Completeness is needed ONLY to justify "no verdict exists on this
     head", so when the row is absent from page 1 the job reads PAGE 2, and refuses if
     it carries anything. Cap-independent: no reconfiguration re-breaks it, and nothing
     is hardcoded that a measurement could contradict.

Measured to make sure page 2 is real rather than assumed: `?limit=3&page=2` on
3aed43c6 returns three further rows, and `page=9` returns the same `statuses: null`
terminator the timeline uses.

The probe is skipped when the row IS on page 1, because there is nothing to learn — the
combined endpoint returns the latest status per CONTEXT, so a context cannot recur on a
later page.

TEST-DOUBLE FIDELITY, again the fiddly part. The stub now honours `page`, and that guard
had to go BEFORE the read-counting modes: `appears-on-read:N` counts how many times the
job has LOOKED at the status, and the completeness probe is part of the same look, not a
further one. Letting it increment those counters shifted "the verdict appears on read N"
by one and broke three mid-run-race tests — a false red that would have been easy to
"fix" by adjusting the expected counts, which would have quietly destroyed what those
three tests measure.

Verification: 455 green. Four more mutations, each as intended — deleting the probe
(red), accepting a non-empty page 2 (red), refusing even on an EMPTY page 2 (red, the
deadlock control), and reverting the twin `statuses: null` gate (48 red). Twenty-one
mutations across four rounds.

Refs: #751
Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

fix(751): rebase onto #760 , close the fail-OPEN twin, and retire four claims that had rotted e133c11fde

Fourth cold review round: no Blocker, no new path to a green `review-verdict/h10` on an
unreviewed head, and it independently re-measured 14 claims in the diff. It also caught
that this branch was about to revert someone else's work, and found the one remaining
place where the nil-slice/clamp lesson had not been applied.

REBASED ONTO 9881d1ff8 (#760), which landed while this was in review. The tell was the
one CLAUDE.md documents: `git diff origin/main HEAD` showed deletions I never made —
`docs/decisions/records/mcp/tool-schema-openapi-parity.md` and edits to `docs/mcp.md`.
Pushing would have reverted them. The generated catalog was regenerated rather than
trusted to the rebase, and verified to carry BOTH records.

THE FAIL-OPEN TWIN, one function further on than the last round reached.
`/statuses/{sha}?limit=100` is read twice — for the high-water mark and for the
post-write race check — and neither pages, while `limit` clamps to 50. So "no raced row
on page 1" does not establish "no race": a human `BLOCKED` landing in the write window
can sit on a page the job never reads, leaving a forged green over a rejection. This is
the ONE path in the design whose failure direction is toward SUCCESS.

Measured rather than argued: a probe head reached 33 rows after ~5 runs against a cap of
50, and the ordering is only coarsely newest-first (`33,32,31,30,28,29,27,…`), so a few
CI reruns reach it and the row's position cannot be relied on — which this workflow's own
comment already disclaimed. That comment ALSO claimed order-independence flatly; false
once the page clamps, so it now says what actually holds and what saves us.

The mitigation is conservative rather than complete: if page 1 shows no race, page 2 is
read, and any rows there — or an unreadable page 2 — count as "assume raced" and repair
to `pending`. Uncertainty resolves to a stall a reviewer can clear, never to leaving
green. Real paging of both reads, including the high-water mark, is #763.

A THIRD empty shape turned up while modelling it: `/statuses/{sha}` past the end returns
`[]`, where `/commits/{sha}/status` returns `{"statuses": null}` and the timeline returns
bare `null`. Three endpoints, three shapes, one server. The code tolerates both here
because guessing per endpoint has now been wrong twice.

MY OWN COVERAGE GAP, found by mutation rather than by reading: inverting the
unreadable-history-page-2 branch reddened NOTHING. Now tested both ways. Same class as
the two untested refuse branches the review flagged, which are also covered now.

FOUR CLAIMS RETIRED, all of the shape this issue is about — text that reads as checked
and is not:

* "18 tests fail" for the corrected-double mutation is 21 now, because rounds 3-4 added
  three fence-dependent tests. Broke a number while documenting broken numbers. Both
  citations now give the range and lead with the invariant.
* "measured: 4, 2, 9, 5, 10" first-page timeline events are 7, 5, 9, 6, 10 today.
  Timelines grow; the figures are gone and the invariant stated instead — a PR is created
  by a push, and a push is an event, so page 1 is never empty.
* The strict test's docstring said "RAW TEXT" while the test reads parsed `run:` scalars,
  with a dead `raw =` assignment left behind (a new ruff F841).
* `len(bodies) >= 3` had zero slack: deleting the optional jq-preflight step reddened it
  with a message asserting the classifier had not been examined, which was untrue. The
  length assertion already carries the property, so the count only needs to be non-empty.

Also: dead `_workflow_expression_fields_text` removed; assertion C's regex now catches
`function foo {` as well as `foo() {`; the page-2 refusal says a human verdict clears it.

Verification: 460 green. Five more mutations as intended — deleting the history page-2
check (red), accepting an unreadable one (red, after the coverage gap was closed),
accepting a garbage page 2 in read_existing_verdict (red), hiding the marker write behind
`function mk {` (red), and the earlier twenty-one still hold. Live re-probe on this body
follows.

Refs: #751
Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

fix(751): round-5 findings — a rationale that was itself vacuous, and a third regex round

PR Gates / CI image pin matches docker/ci (pull_request) Successful in 13s

Details

PR Gates / Docs update reminder (pull_request) Successful in 15s

Details

PR Gates / decisions lifecycle (pull_request) Successful in 18s

Details

Review verdict / Set review-verdict status (pull_request_target) Successful in 6s

Details

PR Gates / Script tests (pytest) (pull_request) Successful in 2m14s

Details

Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 9m34s

Details

review-verdict/h10 Review-verdict: MERGEABLE @ 20b117d (base: main)

Details

Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 6m41s

Details

Build ErsatzTV Image / Build & push image (amd64) (pull_request) Skipped

Details

Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 6m27s

Details

Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 10s

Details

Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 7s

Details

20b117dabf

Fifth cold review: MERGEABLE, no Blocker, no High. Four Low findings, none behavioural.
Fixing all four rather than accepting them, because two are the exact class this issue
exists to retire: text that reads as a checked reason and is not.

A VACUOUS RATIONALE, on the branch about vacuous rationales. The comment on the history
stub's `page` guard said it sits ahead of the read-counting modes "so the page-2 probe
cannot shift 'raced row appears on read N'", by analogy with the combined endpoint.
Measured: moving that guard AFTER the counter modes reddens NOTHING, because no history
mode that counts reads ever issues a page-2 request — `raced=1` on page 1 short-circuits
the probe. The real reason is the other half: page 2 must terminate for modes that
describe page 1 only, and dropping just that `print("[]")` reddens
`test_a_PRE_EXISTING_human_row_does_NOT_trigger_a_repair`. Comment now says which half is
load-bearing and which was wrong. (The COMBINED endpoint's guard genuinely is
counter-related — moving it reddens three mid-run-race tests.)

THE FALSE REPAIR IS STICKY, and the previous commit undersold it as "a stall a reviewer
can clear". It writes `$REPAIR_DESC`, which the classification refuses to grant an
exemption over and re-writes as a fixed point on every later run — so a spurious repair
removes that head's exemption PERMANENTLY, not for one run, and only a human verdict
clears it. Still the right direction against a forged green over a rejection, but it is a
per-sha loss of the exemption, and that is the argument for real paging (#763) rather
than living with this. Said in the comment now.

CORRECTING THE PREVIOUS COMMIT MESSAGE, which over-generalised: "uncertainty resolves to
a stall … never to leaving green" is true of the page-2 probe and NOT of the enclosing
path. An unreadable page 1, or a non-numeric high-water mark, still leaves the exemption
`success` standing unverified. The workflow's own comments state that correctly; the
message did not.

THIRD ROUND ON ONE REGEX, which is the documented budget for a string-matching predicate.
Assertion C started as `\w+\s*\s*\{`, gained `function\s+\w+` when review found
`function mk {` slipped it, and STILL missed the union form `function mk() {` — the
natural next spelling once the previous one is caught. Now
`^\s*(function\s+)?\w+\s*($\s*$)?\s*\{`, verified against all seven spellings.

THE COMPLETENESS COUNT, restored properly. Relaxing `len(bodies) >= 3` to `assert bodies`
fixed a false red but threw away the only check that the walk reached ALL run-bearing
steps: `max(len) > 5000` proves it reached the classifier and nothing about the short
ones, so a helper that silently stopped yielding them would pass an unscanned delimiter.
Now counted against the job's own step list, read directly rather than through the helper
under test — which catches a helper reading the wrong key or dropping steps, while still
tolerating a step being legitimately added or removed.

Verification: 460 green. Three mutations, each as intended — the union spelling `function
mk() {` (red, previously passed), a walk that drops the short steps (red, the property the
count guard restores), and a legitimate step deletion (PASSES, confirming the false red it
replaced stays fixed). Twenty-nine mutations across six rounds.

Refs: #751
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

timothy referenced this pull request

2026-08-10 21:10:12 +02:00

probe(751): POSITIVE — does the fixed classify step execute? #752

timothy referenced this pull request

2026-08-10 21:10:13 +02:00

probe(751): NEGATIVE CONTROL — is a dropped step now RED? #753

timothy commented

2026-08-10 21:22:10 +02:00

Review-verdict: MERGEABLE @ 20b117d

Five cold review rounds, alternating model families from cold review-only briefs in isolated worktrees (GPT-5.6 Sol and Opus). Round 5 verdict: MERGEABLE, no Blocker, no High. No round found a path to a green review-verdict/h10 on an unreviewed head. Every round found real defects beside the fix and all are addressed: the twin statuses:null shape, the CLAUDE.md/AGENTS.md self-exemption, a dead-code guard written against a page limit of 100 on a server that caps at 50, the fail-OPEN gap in the post-write race check, a stale base that would have reverted #760, and eight rotted or over-broad claims. 460 tests green; 29 mutations across six rounds each land as intended, including two controls that must not red. Live scratch-base probe pair with a negative control on this exact executable body (byte-identical to the probed e133c11fd; only comments changed after it): positive posts the docs-only exemption with trusted=yes, negative drops the step and the new guard fails the job. Deferred with issues: #756 (docker-build required jobs, fail-OPEN) and #763 (paging both /statuses/{sha} reads).

Review-verdict: MERGEABLE @ 20b117d Five cold review rounds, alternating model families from cold review-only briefs in isolated worktrees (GPT-5.6 Sol and Opus). Round 5 verdict: MERGEABLE, no Blocker, no High. No round found a path to a green review-verdict/h10 on an unreviewed head. Every round found real defects beside the fix and all are addressed: the twin statuses:null shape, the CLAUDE.md/AGENTS.md self-exemption, a dead-code guard written against a page limit of 100 on a server that caps at 50, the fail-OPEN gap in the post-write race check, a stale base that would have reverted #760, and eight rotted or over-broad claims. 460 tests green; 29 mutations across six rounds each land as intended, including two controls that must not red. Live scratch-base probe pair with a negative control on this exact executable body (byte-identical to the probed e133c11fd; only comments changed after it): positive posts the docs-only exemption with trusted=yes, negative drops the step and the new guard fails the job. Deferred with issues: #756 (docker-build required jobs, fail-OPEN) and #763 (paging both /statuses/{sha} reads).

timothy merged commit 9a5d34e888 into main

2026-08-10 21:33:34 +02:00

timothy deleted branch fix/751-verdict-step-interpolation

2026-08-10 21:33:35 +02:00

~~timothy referenced this pull request 2026-08-10 21:34:34 +02:00~~

chore(deps): update dependency meziantou.analyzer to 3.0.317 #761

timothy referenced this pull request

2026-08-10 21:34:35 +02:00

chore(deps): update dependency scriban.signed to 7.2.6 #762

timothy referenced this pull request

2026-08-10 21:35:42 +02:00

review-verdict.yml's classify/post step never executes — job goes green, review-verdict/h10 is never posted automatically #751