fix(scripts): an empty ci run list is an unknown, not ten unverified commits - #6479
fix(scripts): an empty ci run list is an unknown, not ten unverified commits#6479macanderson wants to merge 1 commit into
Conversation
…commits `check-main-verified.sh` guarded its commit read for emptiness and never guarded its run read. The matching loop opens every commit at `missing` and only a row for that commit moves it, so a run list that came back with zero rows did not report one absence — it reported the entire window as absent, with no row anywhere able to contradict it. `gh` exits 0 on an empty page, so the existing check for a failed call never saw it. On 2026-09-09 at 16:58 UTC one such page filed against the ten newest commits on `main` at once. Each of them had a completed, successful `ci` run: the same query returned all sixty rows twenty minutes later, and a live run of the fixed script now reports every one of those ten verified. A read that returned nothing cannot tell "no `ci` run exists for any of these commits" from "the API declined to list them", and those are opposite states. The script's own header already decided this — every unknown exits 0 and says so — because a monitor that fabricates blocks merges during exactly the incident its repair has to land in, and one that files falsely is one nobody reads the second time. An empty page is now one of those unknowns, named in the header beside the others. Only the empty page. A list that carries rows but none for a given commit is the outage this script was written to catch, and still reports: the new negative-control case pins that direction so the guard cannot be satisfied by refusing to answer whenever a commit has no run. Witness: `scripts/test-main-verified.sh` — 35 passed, 2 failed before the change, 37 passed, 0 failed after. Closes #6475
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @macanderson, you've used your own review budget of 250,000 diff characters for the last 7 days.
You can request another review in 4 days and 1 hour by commenting @sourcery-ai review. Upgrade to get a review now.
Reviewer's GuideThe PR prevents Flow diagram for empty CI run list handlingflowchart TD
A["check-main-verified.sh reads commits and CI runs"] --> B{"gh run list succeeded?"}
B -- "no" --> U["unknown: API unavailable"]
B -- "yes" --> C{"run list is empty?"}
C -- "yes" --> E["unknown: empty run list"]
C -- "no" --> D{"matching CI run for each commit?"}
D -- "yes" --> O["OK"]
D -- "no" --> M["report unverified commit"]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Answering the ❌ row in Sourcery's assessment
This row reads #6475 as a report of a real outage. It was not one, and there is nothing to re-dispatch — so the objective is not deferred or out of scope, it is vacuous for this issue. The assumption Sourcery names is not an assumption; it is the measurement the PR is built on. Every one of the ten commits #6475 names already had a completed, successful Nine of those ten concluded hours before the issue was filed; the tenth, The issue's own definition of done is already satisfied, and the check now says so. That is the same script, reading the same API, reaching the opposite verdict from the one that filed the issue — which is precisely the defect. So the honest reading of that DoD line is that it was never unmet: the monitor asserted an absence that was not there. Why the row is worth answering rather than silently accepting. Treating it as owed work would mean dispatching ten redundant full-workspace CI runs to "establish" verification that already exists — spending an hour of runners to answer a question with an answer already on file. That is the cost the false alarm imposes, and re-dispatching would be paying it a second time. The one thing genuinely worth checking is whether a future real outage still reports, and it does: the negative-control case |
|
Closing as a duplicate of #6477, which merged at 21:12:57Z and reached the same diagnosis and the same fix: the run read had no emptiness guard, #6477 is the better of the two and nothing here is worth salvaging on top of it. Its test suite covers both of my new cases and adds a third — that an empty read files no issue at all — and its write-up settles one thing mine only asserted: it checked directly that the same query under an invalid token exits 1, which rules out a permission fault and leaves the empty-answer-with-exit-0 shape as the only explanation. How this collided, since the machinery exists to stop it. I ran
The second run would have found #6477 with What does not close with this PR, because neither is covered by #6477:
|
1 participant
What & why
scripts/check-main-verified.shfiled a false alarm this afternoon. At16:58 UTC it reported the ten newest commits on
mainas having nocirunat all and opened this issue. Every one of those ten commits has a completed,
successful
cirun:The script guards its commit read for emptiness and never guards its
run read. The matching loop opens every commit at
verdict="missing"andonly a row for that commit moves it, so a run list that came back with zero
rows did not report one absence — it reported the whole window as absent at
once, with no row anywhere able to contradict it.
ghexits 0 on an emptypage, so the existing
if ! runs=...check for a failed call never saw it.A read that returned nothing cannot tell "no
cirun exists for any of thesecommits" from "the API declined to list them", and those are opposite states.
The script's own header already decided which way that resolves — "It fails
OPEN, at every unknown" — because a monitor that fabricates blocks merges
during exactly the incident its repair has to land in, and one that files
falsely is one nobody reads the second time. An empty page is now one of those
unknowns, and it is named in the header beside
ghbeing absent and the APIbeing unreachable.
Only the empty page. A list that carries rows but none for a given commit
is the outage this script was written to catch, and still reports. A
negative-control case pins that direction so the new guard cannot be satisfied
by refusing to answer whenever a commit has no run of its own.
Closes #6475
The witness
main, passes here)scripts/test-main-verified.sh, run against this branch:The two failing assertions are
an empty run list is UNKNOWN, not every commit missingand...and names an empty run list as the reason. The third newcase —
a run list carrying nothing for the commit is still unverified—passes on both sides by design: it is the negative control, and it is what
stops the fix being a blanket "answer nothing when in doubt".
Verified against the real repository as well:
./scripts/check-main-verified.shon this branch prints
OK — each of the last 10 commit(s) on main has a completed ci run.and exits 0, which is the state the issue claims otherwise.The gate
cargo fmt --check— viamake guards-fastCloses #6475appears both above and as a commit trailermake guards-fast,make prose,make line-citationsandmake shellcheckall pass on this branch. No test was deleted.
Fix over file
scripts/check-ci-tests.sh,which reads the same run list the same way — it is not affected: with
an empty list its
verdictstays empty and it already exits 0 throughunknown, so it fails open as intended.the sibling guards after this fix turned up the same shape in
scripts/check-releases-published.sh: its releases read is unguarded, andan empty page makes
$publishedempty, so every non-grandfathered tagpast the grace window reports as
absent. It did not ride this PR becausethe obvious guard collides with the script's own purpose — "many tags,
zero releases" is also the genuine catastrophic finding it exists to
report, and nothing in the payload separates a broken read from a real
total failure. Which cost to pay is a maintainer call, so it is written up
with the three options rather than decided here.
Anything reviewers should know?
Unrelated to this diff, and reported rather than fixed because it is an AWS
IAM change outside this repository: the docs site deploy has failed on every
push to
mainsince 2026-09-08 23:25 UTC (three consecutive runs ofdocs).stella.oxagen.shhas not been updated since.The role needs
ssm:SendCommandon that instance. Nothing in this tree cangrant it.