fix(list): count only the newest run of each check - #332
Conversation
|
@bot review |
|
Claude finished @lararosekelley's task in 6m 11s —— View job Code Review - Confidence: 72/100Stops a superseded (cancelled) check run from pinning a review red, by deduplicating GitHub's rollup to the newest run per check and adding a
Both bugs turn red or yellow into green — the one direction a CI dot must not get wrong — and neither is caught by the new tests, because every fixture uses Not findings, but checked: The diagnosis in the description is right and the correction to the
|
|
Claude finished @lararosekelley's task in 6m 22s —— View job Code Review - Confidence: 78/100Stops a superseded (cancelled) check run from pinning a review's CI dot red, by fetching the rollup's
Both are confined to the display dot. I traced the merge gate separately: CI is still in progress ( The diagnosis is the genuinely good part here: correcting the report's premise for |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| let stamp = field("completedAt") | ||
| .or_else(|| field("startedAt")) | ||
| .or_else(|| field("createdAt")) | ||
| .unwrap_or("") | ||
| .to_owned(); |
There was a problem hiding this comment.
[Bug]
gh pr view --json statusCheckRollup renders an unset DateTime as Go's zero time, not null. Captured from this very PR:
name status conclusion startedAt completedAt
bot-code-review IN_PROGRESS "" 2026-08-30T03:05:04Z 0001-01-01T00:00:00Z
lint COMPLETED SUCCESS 2026-08-30T03:05:04Z 2026-08-30T03:05:31Z
test (windows-...) IN_PROGRESS "" 2026-08-30T03:05:04Z 0001-01-01T00:00:00Z
field("completedAt") therefore returns Some("0001-01-01T00:00:00Z") — not None — for anything still running, so the fallback to startedAt never fires and the stamp sorts below every real 20xx timestamp. On the check_status path (rollup_status → here, reached via generic_annotate / generic_annotate_review whenever the batched GraphQL query fails — GHES, rate limit, a network hiccup) an in-flight re-run always loses to the older completed run of the same name:
- old
lintSUCCESScompleted 03:05:31, re-runIN_PROGRESS→ keeps the oldSUCCESS, returnsPassing. Before this PR the in-flight entry setpendingand the dot was 🟡. Now it is 🟢 while CI is still running. - old
lintCANCELLEDcompleted 03:05:31, re-runIN_PROGRESS→ keeps theCANCELLEDentry, returnsInconclusive(⚪) instead of 🟡 — for the whole duration of the re-run, which is exactly the list: a superseded CANCELLED check pins a review red even though a later run of the same check passed #331 window.
The GraphQL path escapes the sentinel (a real null there), but has a milder version of the same problem: completedAt on one entry is compared against startedAt on another, which is not a common clock. With cancel-in-progress, the superseding run's startedAt routinely precedes the cancelled run's completedAt — the cancel has to propagate to the runner — so the cancelled entry wins there too.
Order by when a run started, and reject the sentinel:
| let stamp = field("completedAt") | |
| .or_else(|| field("startedAt")) | |
| .or_else(|| field("createdAt")) | |
| .unwrap_or("") | |
| .to_owned(); | |
| // `gh pr view --json statusCheckRollup` renders an unset DateTime as | |
| // Go's zero time rather than null, so a run still in flight would | |
| // sort below every finished one. Order on when a run started - the | |
| // only field the two node types can be compared on. | |
| let real = |name| field(name).filter(|at| !at.starts_with("0001-")); | |
| let stamp = real("startedAt") | |
| .or_else(|| real("createdAt")) | |
| .or_else(|| real("completedAt")) | |
| .unwrap_or("") | |
| .to_owned(); |
That still leaves a queued re-run (startedAt also zeroed) losing to the old completed run. If you want that covered too, rank on (status != "COMPLETED", stamp) so an unfinished run outranks a finished one of the same name regardless of clock.
Note none of the new unit cases exercise completedAt — every fixture uses startedAt or createdAt only — which is why the tests pass with the field that misorders as the first preference. Worth a case with the literal "0001-01-01T00:00:00Z" gh emits.
| let key = field("name") | ||
| .or_else(|| field("context")) | ||
| .map_or_else(|| format!("\u{0}{index}"), str::to_owned); |
There was a problem hiding this comment.
[Bug]
A CheckRun's name is the job name, not a globally unique check identity — two workflows that both define a job called lint (or build, or test) produce two check runs named lint on the same head commit, neither superseding the other. gh's own output carries workflowName precisely because name does not disambiguate:
name workflowName
lint CI
plan Release
Grouping on the bare name collapses them, and the loser is discarded silently:
{name: "lint", conclusion: "FAILURE", startedAt: "10:00"}(workflowdocs){name: "lint", conclusion: "SUCCESS", startedAt: "10:05"}(workflowci)
newest keeps only the SUCCESS, so aggregate_rollup returns Passing and list shows 🟢 on a PR whose checks are red. Before this PR both paths caught it: the rollup's own state was FAILURE, and check_status walked every entry. This is the one direction a status dot must never get wrong.
Two ways out:
- Put the workflow identity in the key.
gh pr viewalready givesworkflowName; the batched query would needcheckSuite{workflowRun{workflow{name}}}added to the CheckRun fragment on line 598, and the key becomes that plusname. A concurrency-cancelled run and its re-run share a workflow name, so list: a superseded CANCELLED check pins a review red even though a later run of the same check passed #331 still collapses correctly. - If you'd rather not grow the query, narrow the collapse instead: drop an entry only when its conclusion is
CANCELLEDand a strictly newer entry shares its name. That fixes the reported shape exactly and cannot discard a failure, at the cost of not deduplicating a superseded failure after a green re-run.
Either way, please add a case for two same-named runs where the older one failed — the current tests only cover same-name pairs that really are the same check.
| let stamp = field("completedAt") | ||
| .or_else(|| field("startedAt")) | ||
| .or_else(|| field("createdAt")) | ||
| .unwrap_or("") | ||
| .to_owned(); |
There was a problem hiding this comment.
[Bug]
Preferring completedAt compares two different events across entries, so "newest" can select the older run — in the exact scenario this PR targets.
Take the superseded pair while the re-run is still in flight:
{"name":"plan","status":"COMPLETED","conclusion":"CANCELLED","startedAt":"T1","completedAt":"T3"}
{"name":"plan","status":"IN_PROGRESS","startedAt":"T2"}The in-flight entry has no completedAt, so its stamp is T2; the cancelled one's is T3. A concurrency group queues the new run first and the cancelled run's teardown finalises after that, so T2 < T3 is the ordinary shape, not a corner case. The cancelled entry wins the key and the newest run is dropped from the map entirely — so pending is never set for plan, and a rollup with CI actively running renders ⚪ (settled, no verdict) instead of 🟡.
Start time is the field that orders runs: a re-run always starts after the run it supersedes, and both node types carry one (startedAt / createdAt). completedAt only makes the comparison incoherent when one candidate has not finished.
| let stamp = field("completedAt") | |
| .or_else(|| field("startedAt")) | |
| .or_else(|| field("createdAt")) | |
| .unwrap_or("") | |
| .to_owned(); | |
| // Order on start time: a re-run always starts after the run it | |
| // supersedes, while `completedAt` is a different event - comparing one | |
| // entry's completion against another's start ranks a finished run | |
| // above the in-flight run that replaced it. | |
| let stamp = field("startedAt") | |
| .or_else(|| field("createdAt")) | |
| .or_else(|| field("completedAt")) | |
| .unwrap_or("") | |
| .to_owned(); |
That leaves one residual gap: a queued CheckRun with a null startedAt stamps "" and still loses. If you want the invariant to hold outright, rank on (status != "COMPLETED", stamp) so an unfinished run always outranks a finished one of the same name — which is also what makes pending reliable.
The existing cases all use startedAt/createdAt, so they pass either way; worth adding the mixed pair above as a case, since it is the state a user sees during the re-run window.
| /// Finished without a verdict - a run that was cancelled, or one waiting | ||
| /// on a human. Nothing is wrong, but not everything is green, so folding | ||
| /// it into either would say something untrue. | ||
| Inconclusive, |
There was a problem hiding this comment.
[Partial Upgrade]
CheckStatus is the shared vocabulary all four providers render through, and this variant's contract — "a run that was cancelled, or one waiting on a human" — is contradicted by the provider next door. gitlab.rs:416 still maps:
Some("success" | "skipped" | "manual") => CheckStatus::Passing,
Some("failed" | "canceled") => CheckStatus::Failing,So after this PR the same real-world state renders two different dots depending on the forge: a cancelled pipeline is ⚪ on GitHub and 🔴 on GitLab, and a pipeline blocked on a manual job — GitLab's exact analogue of ACTION_REQUIRED — is ⚪ on GitHub and 🟢 on GitLab. The docs/COMMANDS.md paragraph this PR adds describes ⚪ as a property of list/status, with no forge qualifier, so it documents behaviour GitLab users will not get.
Minimal fix: move "canceled" to CheckStatus::Inconclusive in map_pipeline_status, with a unit case beside the existing map_pipeline_status tests. "manual" reading green is the ACTION_REQUIRED analogue and looks like the same call, though it is a pre-existing mapping rather than something this PR moved — your judgement whether it belongs here.
Gitea needs nothing: its combined-status states (pending/success/error/failure/warning) have no cancelled analogue, and map_gitea_state's _ => Pending already covers the rest.
|
@bot review |
|
Claude finished @lararosekelley's task in 5m 46s —— View job Code Review - Confidence: 76/100 (+4 vs 72 on the first commit)Stops a superseded (cancelled) check run from pinning a review red, by fetching the rollup's
The remaining bug is display-only — I re-traced the merge gate and Resolved since the first commitBoth verified against real
Also checked and clear: CI at review time: Correcting the report's premise for
|
4e33460 to
23fb9b2
Compare
| let key = field("name") | ||
| .or_else(|| field("context")) | ||
| .map_or_else(|| format!("\u{0}{index}"), str::to_owned); |
There was a problem hiding this comment.
[Bug]
The dedup key for a CheckRun is the bare name, which is the job name — not an identity unique to one check on the commit. gh carries a separate workflowName field precisely because name does not disambiguate. From this PR's own head commit (gh pr checks 332 --json name,workflow):
name workflow
lint CI
plan Release
test (ubuntu-latest) CI
Two workflows in one repo that both define a job called build (or test, or lint), or an Actions job named build beside a build check run from a third-party CI App, produce two unrelated check runs with the same name on the same head commit. Neither supersedes the other — but this groups them, and newest discards the loser silently:
[{"name":"build","status":"COMPLETED","conclusion":"FAILURE","startedAt":"2026-08-29T10:00:00Z"},
{"name":"build","status":"COMPLETED","conclusion":"SUCCESS","startedAt":"2026-08-29T10:05:00Z"}]Both are COMPLETED, so in_flight ties and the stamp decides. The SUCCESS wins the key, the FAILURE never reaches the aggregate loop, and aggregate_rollup returns Passing. list paints 🟢 on a PR whose checks are red.
Both old paths caught this: list read the rollup's own state, which was FAILURE, and check_status walked every entry without grouping. Turning red into green is the one direction a status dot must not get wrong.
Two ways out:
- Widen the key. Put the workflow identity in it —
gh pr viewalready returnsworkflowName, and the batched query needscheckSuite{workflowRun{workflow{name}}}added to the CheckRun fragment on line 598. A concurrency-cancelled run and its re-run share a workflow name, so list: a superseded CANCELLED check pins a review red even though a later run of the same check passed #331 still collapses correctly. (checkSuite{app{slug}}covers the non-Actions case, whereworkflowRunis null.) - Narrow the collapse. Drop an entry only when its conclusion is
CANCELLEDand a strictly newer entry shares its name. That fixes the reported shape exactly and cannot discard a failure, at the cost of not collapsing a superseded failure after a green re-run.
Either way, please add a case for two same-named runs where the older one failed — every current fixture is a same-name pair that really is the same check, so nothing here fails today.
| Some("failed" | "canceled") => CheckStatus::Failing, | ||
| Some("success" | "skipped") => CheckStatus::Passing, | ||
| Some("failed") => CheckStatus::Failing, | ||
| Some("canceled" | "manual") => CheckStatus::Inconclusive, |
There was a problem hiding this comment.
[Partial Upgrade]
gitlab.rs encodes these same status strings in two places, and before this PR they agreed exactly. Only the dot moved:
| pipeline status | map_pipeline_status (the dot) |
wait_for_checks (the merge gate) |
|---|---|---|
success / skipped |
Passing |
Passed |
manual |
PassingInconclusive |
Passed (line 206) |
canceled |
FailingInconclusive |
Failed (line 209) |
So the doc comment six lines up — "a pipeline in that state still holds the merge" — is contradicted by line 206. On an MR whose pipeline is manual, stk merge returns WaitOutcome::Passed and merges. The user reads ⚪, documented in docs/COMMANDS.md as "finished without a verdict… waiting on a person", runs stk merge, and it merges anyway. The canceled half is the milder mirror: the dot says nothing failed, merge reports a check failure.
Note the claim in the comment is right about GitLab itself — a pipeline only reports manual when a blocking manual job (allow_failure: false) is waiting, and GitLab holds the merge for it. It is wait_for_checks line 206 that is out of step. But changing it makes stk merge wait to checkTimeout on every repo with a blocking manual job, which is a behavior change well outside #331.
Minimal fix for this PR: narrow the comment (and the matching CHANGELOG.md sentence) so it describes the dot rather than asserting a gate git-stk does not apply. If the claim is meant to hold, route wait_for_checks through map_pipeline_status so there is one mapping instead of two — but that belongs in its own change, with an integration test for the merge path.
|
@bot review |
|
Claude finished @lararosekelley's task in 6m 22s —— View job Code Review - Confidence: 74/100 (-2 vs 76 on the second commit)Stops a superseded (cancelled) check run from pinning a review red, by fetching the rollup's
The score moves down two rather than up because the first finding is the same bug the previous round raised, fixed on one of its two paths — the guard, the comment explaining it, and the test asserting it all exist, and none of them apply to Resolved since the second commit
Also checked and clear: the batched query selects CI at review time: Arriving independently at gh's own
|
| let workflow = item | ||
| .pointer("/checkSuite/workflowRun/workflow/name") | ||
| .or_else(|| item.pointer("/checkSuite/app/slug")) | ||
| .and_then(serde_json::Value::as_str) | ||
| .unwrap_or(""); |
There was a problem hiding this comment.
[Bug]
The workflow identity is read only out of checkSuite, which the batched GraphQL query now selects — but check_status does not use that query. It runs gh pr view --json statusCheckRollup, and gh's exporter emits a flat workflowName with no checkSuite object at all. Captured just now from this PR's own head commit:
{"__typename":"CheckRun","name":"lint","status":"COMPLETED","conclusion":"SUCCESS",
"startedAt":"2026-08-30T12:53:18Z","completedAt":"2026-08-30T12:53:46Z","workflowName":"CI"}
{"__typename":"CheckRun","name":"plan","status":"COMPLETED","conclusion":"SUCCESS",
"startedAt":"2026-08-30T12:53:18Z","completedAt":"2026-08-30T12:53:39Z","workflowName":"Release"}No checkSuite key anywhere in that payload, so both pointer lookups return None, workflow is "", and the key collapses to \u{1}{name} — the bare job name this commit set out to stop using. On that path two same-named jobs in different workflows group, newest keeps one, and the other's FAILURE never reaches the aggregate loop:
[{"name":"build","status":"COMPLETED","conclusion":"FAILURE","startedAt":"...:17Z","workflowName":"Release"},
{"name":"build","status":"COMPLETED","conclusion":"SUCCESS","startedAt":"...:18Z","workflowName":"CI"}]→ Passing. Before this PR aggregate_rollup walked every entry, so the same input returned Failing. Red into green, on a path this PR regresses.
check_status is not a rare fallback. github.rs:397 routes there for any review the batched query cannot see — every merged or closed layer, which is status's ordinary case — and github.rs:415 routes list there whenever the GraphQL call fails (GHES rejecting a field, rate limit, a network hiccup).
aggregate_rollup_does_not_merge_the_same_job_name_across_workflows does not catch it because every fixture in it is the checkSuite shape; nothing in the suite feeds the flat shape gh actually returns.
Add the flat field to the chain:
| let workflow = item | |
| .pointer("/checkSuite/workflowRun/workflow/name") | |
| .or_else(|| item.pointer("/checkSuite/app/slug")) | |
| .and_then(serde_json::Value::as_str) | |
| .unwrap_or(""); | |
| let workflow = item | |
| .pointer("/checkSuite/workflowRun/workflow/name") | |
| .or_else(|| item.pointer("/checkSuite/app/slug")) | |
| // `gh pr view --json statusCheckRollup` flattens the workflow onto | |
| // the node instead of nesting a checkSuite, so the two shapes name | |
| // the same thing differently. | |
| .or_else(|| item.get("workflowName")) | |
| .and_then(serde_json::Value::as_str) | |
| .unwrap_or(""); |
and a case in the gh shape beside the existing one — workflowName rather than checkSuite, older run FAILURE, asserting CheckStatus::Failing.
Worth noting the design here matches gh's own eliminateDuplicates, which keys on workflow/name and orders on StartedAt — the same two decisions this PR arrived at independently. That is the reason gh pr checks already said pass where the web UI said red.
| /// Checks stopped without a verdict - a cancelled run, or one waiting on a | ||
| /// person. Nothing failed, but nothing passed either, and the platform | ||
| /// still holds the merge, so the run stops and says which it was. | ||
| /// | ||
| /// The same distinction [`CheckStatus::Inconclusive`] draws for the dot. | ||
| /// A provider that encodes check states in both places must map them the | ||
| /// same way, or the dot and the merge disagree about the same pipeline. | ||
| Inconclusive, |
There was a problem hiding this comment.
[Partial Upgrade]
"A provider that encodes check states in both places must map them the same way" is an obligation GitHub cannot meet as written, and this PR is what makes that visible.
GitLab does encode them in both places, and gitlab.rs:206-212 now maps from the same string table as map_pipeline_status — the fix from the last round, and it holds. GitHub does not: check_status reduces the rollup through aggregate_rollup, while wait_for_checks (github.rs:307) reduces gh pr checks exit codes through interpret_checks, which only knows 0 → Passed, 8 → Pending, anything else → Failed/Errored. There is no exit code for "cancelled", so no GitHub path can return WaitOutcome::Inconclusive — grep confirms it: the only construction site in the tree is gitlab.rs:212.
The consequence is one screen of output contradicting the next. A current cancellation on GitHub now renders ⚪, documented in docs/COMMANDS.md:70 as "finished without a verdict"; the same PR under merge --all --wait resolves through the exit code to Failed and bails with "checks failed for #N; fix them and rerun". Before this PR the two agreed — CANCELLED was in the failing list on both sides. Gitea and demo are in the same position for the same reason.
Nothing unsafe follows from it: every divergence lands on the refuse-to-merge side, and merge.rs:199 handles the new variant. The ask is that the comment stop asserting something the tree does not do. Either narrow it —
| /// Checks stopped without a verdict - a cancelled run, or one waiting on a | |
| /// person. Nothing failed, but nothing passed either, and the platform | |
| /// still holds the merge, so the run stops and says which it was. | |
| /// | |
| /// The same distinction [`CheckStatus::Inconclusive`] draws for the dot. | |
| /// A provider that encodes check states in both places must map them the | |
| /// same way, or the dot and the merge disagree about the same pipeline. | |
| Inconclusive, | |
| /// Checks stopped without a verdict - a cancelled run, or one waiting on a | |
| /// person. Nothing failed, but nothing passed either, and the platform | |
| /// still holds the merge, so the run stops and says which it was. | |
| /// | |
| /// The same distinction [`CheckStatus::Inconclusive`] draws for the dot. | |
| /// Only GitLab reaches it: its dot and its gate read one status string, so | |
| /// they can agree. GitHub's gate reads `gh pr checks` exit codes, which | |
| /// have no cancelled code, so there the dot may read `⚪` where the gate | |
| /// reports a failure. | |
| Inconclusive, |
— or make GitHub's gate read the rollup it already fetches elsewhere, which is a larger change than #331 and wants its own integration test on the merge path.
|
@bot review |
|
Claude finished @lararosekelley's task in 13m 35s —— View job Code Review - Confidence: 82/100 (+8 vs 74 on the third commit)Stops a superseded (cancelled) check run from pinning a review red, by fetching the rollup's
Neither can merge past a red check - Resolved since the third commitChecked against a real payload rather than taken on trust. ["__typename","completedAt","conclusion","detailsUrl","name","startedAt","status","workflowName"]
Also checked and clear: the new query is brace-balanced and closes the same two outer scopes the old one did; CI at review time ( Reaching for the real payload to decide the
|
| // `gh pr checks` reports a verdict per check but has no exit | ||
| // code for "stopped without one", so a cancelled newest run | ||
| // can land in the green code. Ask the rollup, which is what | ||
| // the dot reads - the gate and the dot must not disagree | ||
| // about the same commit. | ||
| ChecksState::Passed => { | ||
| return Ok(match self.check_status(review) { | ||
| Ok(CheckStatus::Inconclusive) => WaitOutcome::Inconclusive, | ||
| _ => WaitOutcome::Passed, | ||
| }); | ||
| } | ||
| ChecksState::Failed => return Ok(WaitOutcome::Failed), |
There was a problem hiding this comment.
[Bug]
The consult is on one arm, and it is the arm the other half of Inconclusive cannot reach.
gh pr checks sorts every check into one of five buckets - its own help, from the gh in this runner, says so:
When the `--json` flag is used, it includes a `bucket` field, which categorizes
the `state` field into `pass`, `fail`, `pending`, `skipping`, or `cancel`.
There is no bucket for ACTION_REQUIRED, so it lands in fail (where it has always been) or in pending. Neither reaches the arm you hooked:
fail→ exit 1 →ChecksState::Failed→WaitOutcome::Failed, andmerge_allbails with "checks failed for #N" - for a checklistjust painted⚪anddocs/COMMANDS.md:70describes as "waiting on a person". Nothing failed; the gate says it did.pending→ exit 8 → the poll loop keeps going untilstk.checkTimeoutand the run ends inchecks_timed_out, for a state that will never move on its own.
So WaitOutcome::Inconclusive's doc comment - "Where a provider's gate cannot see the difference on its own … it asks the same rollup the dot came from" - is true of GitHub only on the green path, and the dot/gate contradiction this commit set out to close is still there for ACTION_REQUIRED.
That leaves the cancelled half resting on an unverified premise: whether gh's cancel bucket exits 0. Nothing in the tree checks it. merge_wait_stops_on_a_github_check_that_was_cancelled tells the fake to exit 0 for pr checks, which is the repo's own point about fakes (docs/CODE_REVIEW_BOT.md: they prove git-stk would run the command, "they cannot prove gh accepts that flag"), and the e2e --wait cell runs against a repo with no checks at all. If gh counts cancel toward the failing exit, this arm never fires and the CHANGELOG's third bullet describes behavior no user gets.
Asking the rollup on the red side too settles both, whichever bucket gh chose:
| // `gh pr checks` reports a verdict per check but has no exit | |
| // code for "stopped without one", so a cancelled newest run | |
| // can land in the green code. Ask the rollup, which is what | |
| // the dot reads - the gate and the dot must not disagree | |
| // about the same commit. | |
| ChecksState::Passed => { | |
| return Ok(match self.check_status(review) { | |
| Ok(CheckStatus::Inconclusive) => WaitOutcome::Inconclusive, | |
| _ => WaitOutcome::Passed, | |
| }); | |
| } | |
| ChecksState::Failed => return Ok(WaitOutcome::Failed), | |
| // `gh pr checks` reports a verdict per check but has no exit | |
| // code for "stopped without one", so a cancelled or | |
| // action-required newest run lands in whichever code its | |
| // bucket maps to. Ask the rollup on both sides - the gate and | |
| // the dot must not disagree about the same commit. | |
| ChecksState::Passed => { | |
| return Ok(match self.check_status(review) { | |
| Ok(CheckStatus::Inconclusive) => WaitOutcome::Inconclusive, | |
| _ => WaitOutcome::Passed, | |
| }); | |
| } | |
| ChecksState::Failed => { | |
| return Ok(match self.check_status(review) { | |
| Ok(CheckStatus::Inconclusive) => WaitOutcome::Inconclusive, | |
| _ => WaitOutcome::Failed, | |
| }); | |
| } |
That cannot walk a merge past a red check: aggregate_rollup returns Inconclusive only when no newest run failed, and merge_all bails on both variants either way - only the message changes. The pending case needs no change if gh buckets ACTION_REQUIRED as fail; worth confirming which, since that also tells you whether the Passed arm above is live.
| aliases.push_str( | ||
| "commits(last:1){nodes{commit{statusCheckRollup{contexts(last:100){nodes{\ | ||
| __typename ... on CheckRun{name status conclusion startedAt \ | ||
| checkSuite{workflowRun{workflow{name}} app{slug}}} \ | ||
| ... on StatusContext{context state createdAt}}}}}}}}", | ||
| ); |
There was a problem hiding this comment.
[Bug]
contexts(last:100) caps what the aggregate can see. statusCheckRollup{state} did not - GitHub computed it over the whole list, so it could be stale about superseded runs but never blind.
The cap is not "100 checks", it is 100 runs, and this PR's own premise is that the rollup keeps every run recorded against the head commit. A 40-job matrix on a branch restacked three times is 120 contexts; the response carries the newest 100 and the rest are gone. Usually the ones that fall off are the superseded runs, which is exactly right. But a check whose only entry is old - one that failed and was never re-run, while the noisy checks around it were - falls off with them. aggregate_rollup never sees it, no entry means no failure, and list paints 🟢 on a PR whose checks are red. Same direction as the two bugs the earlier rounds caught, on a path only this PR opens.
check_status has always had this cap (gh selects last:100 too), so it is the batched path that changes: the branch list normally takes traded an exact aggregate for a truncatable one.
StatusCheckRollupContextConnection exposes totalCount, which says when it happened. Select it alongside the nodes, keep state in the selection, and when totalCount > 100 fall back to the aggregate - a stale red for a repo that large is the safe direction, and it is not a repo #331 can currently be fixed for anyway. pageInfo{hasNextPage} works as well if you prefer it. Either way, please say in the comment that 100 is a cap and what happens past it, rather than leaving the number to look like a generous limit.
|
@bot review |
|
Claude finished @lararosekelley's task in 9m 48s —— View job Code Review - Confidence: 88/100 (+6 vs 82 on the fourth commit)Stops a superseded (cancelled) check run from pinning a review red, by fetching the rollup's No correctness bug survives this round. Both findings below are about the code staying right rather than being right today, which is why the score moves up rather than down.
Resolved since the fourth commit
Also checked and clear: CI is fully green: Answering "which bucket does gh put this in?" by covering both instead of guessing is the right shape of fix — it closes the finding and the uncertainty behind it at the same time.
|
| /// How many rollup entries the annotate query asks for. Past this the oldest | ||
| /// are dropped, and [`parse_annotation_batch`] falls back to GitHub's own | ||
| /// aggregate rather than answering from a slice. | ||
| const ROLLUP_CONTEXTS: u64 = 100; |
There was a problem hiding this comment.
[Complexity]
The guard is correct only while two numbers written 73 lines apart agree, and nothing makes them. Line 615 hardcodes the cap in a string literal:
"commits(last:1){nodes{commit{statusCheckRollup{state contexts(last:100){\
and this const restates it in Rust, with a doc comment that asserts the coupling ("how many rollup entries the annotate query asks for") rather than establishing it. The compiler cannot check a claim about a string.
The drift is silent and one-directional in the wrong way. Lower the query to last:50 — for cost, for a GHES node budget, for anything — and truncated keeps testing > 100: entries 51..100 are gone from the response and the fallback never fires. That is precisely the state this commit added the const to prevent, and it would present as the same symptom (🟢 on a red commit), from a change whose author had no reason to look here. Raise it to last:200 and the fallback fires for a commit that was read whole, downgrading an accurate dot to GitHub's superseded-run-counting aggregate — the #331 bug, reintroduced for the repos most likely to hit it.
The format! on line 599 is right next door, so interpolating costs nothing:
aliases.push_str(&format!(
"commits(last:1){{nodes{{commit{{statusCheckRollup{{state \
contexts(last:{ROLLUP_CONTEXTS}){{totalCount nodes{{\
__typename ... on CheckRun{{name status conclusion startedAt \
checkSuite{{workflowRun{{workflow{{name}}}} app{{slug}}}}}} \
... on StatusContext{{context state createdAt}}}}}}}}}}}}}}"
));The escaping is noisy; const ROLLUP_CONTEXTS: &str = "100" with a parse() at the comparison, or a debug_assert!(query.contains(&format!("last:{ROLLUP_CONTEXTS}"))) in the existing build_annotation_query test, would do as well. What matters is that one edit cannot leave the two disagreeing.
| aliases.push_str( | ||
| "commits(last:1){nodes{commit{statusCheckRollup{state contexts(last:100){\ | ||
| totalCount nodes{\ | ||
| __typename ... on CheckRun{name status conclusion startedAt \ | ||
| checkSuite{workflowRun{workflow{name}} app{slug}}} \ | ||
| ... on StatusContext{context state createdAt}}}}}}}}", | ||
| ); |
There was a problem hiding this comment.
[Missing Coverage]
The selection set went from one scalar to five levels of nesting, a paginated connection, two inline fragments and totalCount — and nothing in the tree proves GitHub accepts it. The fakes replay canned JSON keyed on api graphql, which exercises parse_annotation_batch and never gh. The live suite would, but it never runs the command:
$ grep -n 'stk(work, &\["list"' src/bin/git-stk-e2e.rs # nothing
$ grep -n 'stk(work, &\["status"' src/bin/git-stk-e2e.rs # nothingThat is the repo's own rule about fakes (docs/CODE_REVIEW_BOT.md: they prove git-stk would run the command, "they cannot prove gh accepts that flag"), and this is the surface it was written for.
What makes it worth a change rather than a note is that a rejected query says nothing, at any verbosity:
batched_annotate:519gets theErrand retries at :525 — but the retry only dropsstack/stackEntry, so it carries this same selection and fails identically.- :528 sees
retried.is_err(), resets the flag, returns the original error. The--verbosenotice at :542 is on the success path, so it never prints. annotate_branches:433swallows it:Err(_) => generic_annotate(...).
So the observable result of a malformed field path is list quietly paying two rejected GraphQL calls plus one gh pr view per branch, forever, on every invocation — and the truncation guard added in this same commit lives only on the path that just died, so the >100 case silently returns to answering from a slice. Green on a red commit, by the route this commit closed.
The cheap catch already exists. ⛁ is rendered from stack{number size} stackEntry{position} in this query, and a rejection of any field here kills both attempts, so the marker disappears. The stacked_pull_requests cell has two open PRs in a GitHub stack by the time it reaches expect_open_stack (line 174) — one assertion on stk list output there transitively proves the whole selection set parses against the live schema:
let listed = stk(work, &["list"])?;
if !listed.contains('⛁') {
return Err(format!("list did not mark the stack layers - the annotate query \
was rejected and fell back per branch:\n{listed}"));
}Worth adding regardless of this PR, but this PR is what makes the query big enough to get wrong.
|
@bot review |
|
Claude finished @lararosekelley's task in 9m 16s —— View job Code Review - Confidence: 90/100 (+2 vs 88 on the fifth commit)Stops a superseded (cancelled) check run from pinning a review red, by fetching the rollup's No correctness bug survives this round either. The one finding that matters is that the coverage added to prove the fix works cannot fail for the reason it names — which is load-bearing this time, because the query really was broken and two review rounds said it wasn't.
The query was rejected, from
|
| commit | { |
} |
net |
|---|---|---|---|
afcd08e |
8 | 10 | −2 ✅ |
41cda67 (adds checkSuite) |
12 | 13 | −1 |
8b717e3 (adds totalCount) |
12 | 13 | −1 |
cf42e1d |
— | — | balanced ✅ |
One unclosed scope is a GraphQL syntax error, so batched_annotate failed on both attempts, annotate_branches:433 swallowed it, and every list/status on GitHub silently ran the per-branch path. Which means the contexts selection, the checkSuite dedup key, and the truncation guard have never executed against GitHub — and rounds four and five each stated the query was fine. Catching that from the outside, and generalizing it into a brace-balance assertion rather than a one-off correction, is the substantive work in this commit.
What keeps it at a coverage finding rather than a bug: when the batch is rejected, list falls through to check_status → rollup_status → aggregate_rollup, which carries the same dedup fix. #331 stays fixed on the fallback; what is lost is ⛁, the truncation guard, and 1 call in place of 1 + N. Degraded, not wrong.
Resolved since the fifth commit
- The const/literal coupling.
ROLLUP_CONTEXTSis interpolated into the query, and the unit test pins bothcontexts(last:{ROLLUP_CONTEXTS})andtotalCount, so the guard and the cap cannot drift apart. - Brace balance as a class. The
{/}count assertion is the right generalization of the defect above — it would have failed on41cda67.
Also checked and clear: the workflow chain reads /checkSuite/workflowRun/workflow/name → workflowName → /checkSuite/app/slug, and the two payload shapes never overlap (GraphQL has no workflowName, gh emits no checkSuite), so the reordering relative to the earlier suggestion is inert. Truncation on the gh pr view path drops only oldest entries, so it can never promote a stale run over the newest — the check_status gate consult cannot be made to say Inconclusive by truncation alone. rollup_state_to_status still degrades a null rollup to CheckStatus::None. CheckStatus derives no Ord/serde and dot() remains its only match; WaitOutcome has no _ arm. core_lifecycle runs for all three providers, so the #/! pair is the right generic shape for the id check even though it proves less than intended.
CI at review time: lint, commitlint, test (ubuntu-latest), test (macos-latest) pass; test (windows-latest) in flight; Release green. Nothing failing, and lint passing settles that rustfmt accepts the long interpolated literal.
- Read repo guidelines (
docs/CODE_REVIEW_BOT.md) - Gather PR context (diff, files, CI checks)
- Requirements gate + boundary/pattern check
- Read for correctness, safety, surface area
- Coverage / output review
- Post findings
· branchfix/331-superseded-check-runs
| return Err(format!("list did not show {branch}:\n{listed}")); | ||
| } | ||
| } | ||
| if !listed.contains('#') && !listed.contains('!') { |
There was a problem hiding this comment.
[Missing Coverage]
This assertion cannot fail for the reason it names. When the annotate query is rejected, annotate_branches (github.rs:433) falls back to generic_annotate, which lists reviews with gh pr list and sets id: review.id — and github_review_from (github.rs:1222) builds that as format!("#{}", number). #1/#2 are in the tree on both paths, so listed.contains('#') is true whether the batch was accepted or died.
The comment above it, and the CHANGELOG's new Internal entry, state the opposite: "the tree still prints, just without review numbers". The numbers are there. So are the CI dots — the fallback calls check_status per branch (mod.rs:680). What a rejection actually costs is ⛁, plus 1 + N extra calls on every invocation.
That matters more than usual here, because the query really was rejected, and static review missed it twice. Counting braces in the literal at each commit:
| commit | { |
} |
net |
|---|---|---|---|
afcd08e |
8 | 10 | −2 ✅ |
41cda67 (adds checkSuite) |
12 | 13 | −1 |
8b717e3 |
12 | 13 | −1 |
cf42e1d |
— | — | balanced ✅ |
The prefix {nodes{number headRefName mergeQueueEntry{state} contributes +2 and the format! wrapper closes itself, so −2 is the balanced value. From 41cda67 until this commit the query was a GraphQL syntax error: both attempts in batched_annotate failed, annotate_branches swallowed it, and every list/status on GitHub ran the per-branch path. The contexts/checkSuite/totalCount selection — and the truncation guard added in 8b717e3 — have never run against GitHub. The new brace-balance unit test closes that exact class, but a balanced query is not an accepted one; nothing yet proves checkSuite{workflowRun{workflow{name}} app{slug}} and contexts(last:100){totalCount …} validate against the schema. (I could not run gh api graphql from this runner to settle it.)
The signal that does separate the two paths is ⛁. It renders from StackPosition, which only the batched query fills in and which generic_annotate sets to None unconditionally (mod.rs:696). github_native_stack is already behind the stacks_enabled guard, so it is not flaky there — after expect_open_stack at line 174:
// The marker comes from `stack`/`stackEntry` in the batched annotate query,
// and the per-branch fallback has no way to fill it in - so a marker here is
// proof the host accepted the whole selection, contexts and all.
let listed = stk(work, &["list"])?;
if !listed.contains('⛁') {
return Err(format!(
"list did not mark the stack layers - the annotate query was rejected:\n{listed}"
));
}One caveat worth naming rather than hiding: that cell is skipped when the preview is off for the runner's account, so it would not cover every run. The durable version is to make the fallback observable — the --verbose notice at github.rs:542 fires only on the stack-fields retry, and nothing prints when the query fails outright — then assert on its absence here, for all three providers.
Keep the branch-name assertions; those earn their place. It is the review-id one that proves nothing.
| // The cap comes from the constant the truncation guard reads, so the | ||
| // two cannot drift: ask for a different number and the guard fires at | ||
| // the new one. | ||
| aliases.push_str(&format!("commits(last:1){{nodes{{commit{{statusCheckRollup{{state contexts(last:{ROLLUP_CONTEXTS}){{totalCount nodes{{__typename ... on CheckRun{{name status conclusion startedAt checkSuite{{workflowRun{{workflow{{name}}}} app{{slug}}}}}} ... on StatusContext{{context state createdAt}}}}}}}}}}}}}}}}}}")); |
There was a problem hiding this comment.
[Complexity]
The doc comment on build_annotation_query (github.rs:578) now says the opposite of what this line does:
/// Build the aliased GraphQL query for [`batched_annotate`]. Kept out of
/// `format!` to dodge brace-escaping; the fields are exactly what
/// [`ReviewAnnotation`] needs, no more.
The selection is built with format! now, and every brace in it is doubled. That would be merely stale, except the rationale it states is the one that produced the bug this commit repairs: a hand-balanced brace run in a literal that nothing could check. A maintainer who reads it and moves the next field back out of format! "to dodge escaping" walks into it again.
Minimal fix: state what is now true — the braces are escaped so the cap comes from ROLLUP_CONTEXTS rather than being restated, and the balance is asserted in build_annotation_query_includes_reviews_only_with_detail.
|
@bot review |
|
Claude finished @lararosekelley's task in 9m 48s —— View job Code Review - Confidence: 92/100 (+2 vs 90 on the sixth commit)Stops a superseded (cancelled) check run from pinning a review red, by fetching the rollup's No correctness bug survives this round. The one finding is a coverage removal, not a defect.
Resolved since the sixth commitBoth closed, and I verified the mechanism rather than the claim:
The query itselfI could not reach One thing to name rather than leave implied: Also checked and clear: CI is fully green on this commit: Moving the proof from the review ids to
|
| /// GitHub's own rollup verdict, for the commits whose check list is too long | ||
| /// to read whole. Coarser than [`aggregate_rollup`] - it counts superseded | ||
| /// runs, which is the bug this all started with - but it sees every entry, | ||
| /// and reporting a stale red beats reporting a green that is not there. |
There was a problem hiding this comment.
[Missing Coverage]
This function gained a new job in this PR and lost its only test in the same diff.
Before, it was the whole of list's answer, and rollup_state_to_status_maps_the_graphql_aggregate pinned every arm — SUCCESS, FAILURE, ERROR, PENDING, EXPECTED, None. That test was replaced in place by aggregate_rollup_keeps_only_the_newest_run_of_each_check, which tests a different function. Nothing calls rollup_state_to_status directly from the suite any more.
What still reaches it is one branch: parse_annotation_batch when totalCount > ROLLUP_CONTEXTS. parse_annotation_batch_falls_back_to_the_aggregate_when_truncated exercises exactly one arm of it, FAILURE, incidentally. So the four remaining arms — including PENDING/EXPECTED, the two whose whole purpose is to keep a 120-context commit from reading settled while its checks run — have no coverage at all, on the one path that by construction never runs on a small repo and never runs under the fakes.
That is the wrong direction for a function this PR just made harder to reach. A stale red is the safe answer only if the mapping is right, and the mapping is now the sole answer for the repos most likely to hit #331 in the first place.
The deleted asserts are still correct and still cheap:
#[test]
fn rollup_state_to_status_maps_the_graphql_aggregate() {
assert_eq!(rollup_state_to_status(Some("SUCCESS")), CheckStatus::Passing);
assert_eq!(rollup_state_to_status(Some("FAILURE")), CheckStatus::Failing);
assert_eq!(rollup_state_to_status(Some("ERROR")), CheckStatus::Failing);
assert_eq!(rollup_state_to_status(Some("PENDING")), CheckStatus::Pending);
assert_eq!(rollup_state_to_status(Some("EXPECTED")), CheckStatus::Pending);
assert_eq!(rollup_state_to_status(None), CheckStatus::None);
}Worth a sentence in its doc comment too, since the reason it survives is no longer obvious from its call graph: it covers StatusState, GitHub's own aggregate, which is a closed enum the field is non-null on — so _ => None is unreachable in practice rather than a swallow.
1 participant
Fixes #331, on both paths — the report's diagnosis fits one of them and I want to be clear about the other.
listwas not aggregating anything. The batched GraphQL query asked forstatusCheckRollup { state }— GitHub's own aggregate — so git-stk was faithfully rendering a value GitHub computed from every run on the head commit, cancelled ones included. There was no list to group. Fixed by asking forcontextsinstead and aggregating ourselves.There is a comment near
open_reviewswarning thatstatusCheckRollup"would fetch every open PR's full check list (huge and slow)". That is about the all-open-PRs listing, not the batched annotate, which is already scoped to the branches being shown. Measured on this repo: 2.3 KiB and 13 contexts for one branch, so about 17 KiB for an eight-branch stack, in the one querylistalready makes.check_statuswas aggregating, and had the bug as described.aggregate_rollupwalked the array with no deduplication and listedCANCELLEDamong the failing conclusions. It now keeps the newest entry per check —namefor a CheckRun,contextfor a StatusContext, withcompletedAt/startedAt/createdAtas the timestamp, since the two node types differ — and aggregates those. An entry with no identity of its own stands alone rather than colliding with its neighbours.On the secondary question.
CANCELLEDno longer reads as red. Nothing failed; the run was stopped. But green would be wrong too — a required check in that state still blocks the merge — so there is a newCheckStatus::Inconclusive, rendered⚪.ACTION_REQUIREDmoves there too: it is waiting on a person, not broken.TIMED_OUTandSTARTUP_FAILUREstay red. Anything still running outranks it, since it may yet resolve.Six unit cases cover the reported shape in both orders, a real failure still winning over a superseded cancellation, a current cancellation,
ACTION_REQUIRED, the pending precedence, legacy status contexts, and anonymous entries. One end-to-end case throughlistasserts🟢for the superseded pair and⚪for a current cancellation. The dots are now documented in the command reference, which never described them.Worth noting this repo reproduces the trigger: stacked PRs get two CI runs, one cancelled by the concurrency group, which is why they have been showing red in the UI while
gh pr checkssaid pass.Closes #331
fix(list): count only the newest run of each check (#332)(merged) 👈mainStack managed by
git-stk