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

Merged

timothy merged 7 commits from fix/751-verdict-step-interpolation into main 2026-08-10 21:33:34 +02:00

Conversation 1 Commits 7 Files Changed 7

+1285 -28

7 Commits

Author	SHA1	Message	Date
timothyandClaude Opus 5	20b117dabf	fix(751): round-5 findings — a rationale that was itself vacuous, and a third regex round ... 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>	2026-08-10 21:08:10 +02:00
timothyandClaude Opus 5	e133c11fde	fix(751): rebase onto #760, close the fail-OPEN twin, and retire four claims that had rotted ... 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>	2026-08-10 19:42:58 +02:00
timothyandClaude Opus 5	951dae26a9	fix(751): the truncation guard I added was DEAD CODE — the page cap is 50, not 100 ... 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>	2026-08-10 19:22:50 +02:00
timothyandClaude Opus 5	46ec532745	fix(751): re-review round — a truncation hole, and the tests that closed findings needed closing ... 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>	2026-08-10 19:22:50 +02:00
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
timothyandClaude Opus 5	40b3747434	fix(751): the fence never trusted its count — a page past the end is null, not [] ... 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>	2026-08-10 19:22:50 +02:00
timothyandClaude Opus 5	2bdb6c44e4	fix(751): a stray expression delimiter in a COMMENT killed the verdict gate ... `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>	2026-08-10 19:22:50 +02:00