fix: evaluate status checks per name and page beyond the first 100 - #40
Closed
mmalyska wants to merge 3 commits into
Closed
fix: evaluate status checks per name and page beyond the first 100#40mmalyska wants to merge 3 commits into
mmalyska wants to merge 3 commits into
Conversation
Characterization tests only — no production code is touched, so these pass on
master as-is. They exist to make the follow-up refactor's behaviour changes
explicit rather than incidental.
github/manager_test.go covers hydrateCommits and PickFirstParentCommits with
synthetic fixtures, and deliberately asserts today's defects as the expected
result, each marked KNOWN DEFECT:
- a name found both as a classic status and as a check run is counted twice,
so `checksPassed == numChecks` rejects a commit whose check did succeed
- duplicate successful check runs with the same name are likewise counted
once each, and blocked by the same equality gate
- a name absent everywhere is indistinguishable from one that failed
The follow-up flips exactly those expectations; any other assertion moving means
something broke.
Not covered by unit tests, and noted as such in the file: a check that falls
outside checkSuites(first: 20) / checkRuns(first: 25) and is therefore never
fetched. hydrateCommits only sees what the query returned, so no fixture can
express it. testdata/baseline/ covers that end to end instead — it records the
per-commit verdict for real repositories so a query regression is visible as a
changed verdict.
The baseline harness is tracked; its repository list and recordings are
gitignored, since recorded output embeds commit messages, author names and
branch names and this repository is public.
Three defects made a commit silently fail evaluation, and because the CLI exits 0 either way the symptom was a promotion that just stopped moving. Consumer repository and check names are kept generic here: this repository is public, the repositories that consume it are not. Counting. checksPassed was incremented once per matching check and compared for equality with the number of requested names, so a name found twice pushed the count past the target and rejected a commit whose checks had passed. This is not hypothetical: one consumer repository publishes its aggregate required check as both a classic commit status and a check run, and only promotes today because the check run happens to fall outside the fetched window. Any widening of that window — including PR #33's — would have broken it. Names now resolve to a state rather than a tally, so duplicates and cross-source overlaps are harmless. Coverage. checkSuites(first: 20) with a nested checkRuns(first: 25) meant a required check could sit outside the window and be indistinguishable from one that failed. Check runs now come from statusCheckRollup.contexts, which is flat and de-duplicated by GitHub, so ordering across suites stops mattering. Dropping the nested connection also removes a commits × suites × runs multiplier: for 25 commits the query goes from 13,075 nodes to 5,050, and its rate-limit cost from 6 points to 1. That is the objection behind PR #35's revert, answered — the matching token is a GitHub App installation token shared by every repository using the shared reusable promote workflow, so this budget is not per-repo. Both connections cap at 100 per page and real repositories exceed it (the heaviest consumer has over 130 check suites on a single commit). Remaining pages are fetched per commit by TopUpChecks, called from the evaluation loop rather than from hydration: that loop already stops at the first commit that passes, so the usual case costs no extra requests and no commit is ever judged on a partial view. Diagnosability. --verbose now lists, per commit and per name, which checks were not satisfied and whether each was found-but-failing or not found at all. Those need opposite fixes and previously looked identical. --contexts-number and --check-suites-number size only the first page, so they trade requests against page size instead of dropping data. They default to 50, not 100: on the heaviest repository a 100/100 first page makes GitHub exceed its own execution limit and return 504. At 50/50 the new query fails in exactly the same places the old one did (both fail above -c 25 and succeed at or below it, which is what the shared workflow passes), so this is not a new limit. GetCommits now says which knobs to turn when that happens. Verified against the 11 repositories that consume the shared workflow, using the recorded baseline: 10 byte-identical verdicts, 1 differing only by a commit that landed between runs while agreeing on all 24 shared commits, 0 verdict changes. The three unit assertions that flip are marked in manager_test.go.
CodeQL flagged the new job for running with the default GITHUB_TOKEN scope. It only reads the code, so grant it exactly that.
Author
|
Superseded by #41, which is stacked on #37. #37 was already open and fixes the same counting defect, with better coverage than this PR had: it also catches the false-pass direction (a surplus on one name masking a deficit on another), trims names so a list with a space after the separator is satisfiable, and works through the policy question on SKIPPED/NEUTRAL. My mistake for not checking open PRs before starting. #41 keeps only the part #37 explicitly deferred — the GraphQL page caps — rebased on top of it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #39.
Consumer repository names, check names and commit SHAs are deliberately generic below: this repository is public, the repositories that consume it are not. The concrete identifiers behind each measurement are available internally.
The measurements that shaped this
PR #33 widened
checkSuites/checkRunsto 100 and #35 reverted it on cost grounds. Both were reasoning about the right risk with the wrong numbers, so I measured before touching anything (rateLimit(dryRun: true), against a large internal monorepo, at-c 25— what the shared promote workflow passes):suites 20 × runs 25)100 × 100)100 × 100)MAX_NODE_LIMIT_EXCEEDEDnodeCount/100, not the cost GitHub charges. The real increase was 6 → 26, so ~4.3×, not ~20×.--commits-number 100, the CLI's own default, that shape is rejected before it runs.This PR goes the other way — 1 point at
-c 25, 6× cheaper than today — while seeing more than #33 did.What was actually broken
Counting, and it is live.
checksPassedwas incremented per matching check and compared with==against the number of requested names, so finding a name twice pushed the count past the target and rejected a commit whose checks had passed. One consumer repository publishes its aggregate required check as both a classic commit status and a check run on the same commit. It promotes today only because the check run falls outside the fetched window:Widen the window and that becomes
2 == 1→ false → promotion silently stops. Any coverage increase, #33's included, had to land together with the counting fix. Names now resolve to a state instead of a tally.Coverage. Check runs now come from
statusCheckRollup.contexts, which is flat and de-duplicated by GitHub, so ordering across suites stops deciding whether a check is seen. Dropping the nestedcheckRunsconnection is where the cost win comes from. I checked whether the rollup could replacecheckSuitesentirely (CheckRun.checkSuite.workflowRun.workflow.namecosts 0 extra nodes) — it cannot: on a commit with over 130 check suites it yields 4 of 6 SUCCESS workflow names, so both connections stay.The 100-per-page ceiling. Both connections cap at 100 and real repositories exceed it. Remaining pages are fetched by
TopUpChecksfrom the evaluation loop, not from hydration — that loop already stops at the first passing commit, so the usual case costs no extra requests. Proven by asymmetry: at--check-suites-number 5the first page contains no workflow runs at all, yet a required check that resolves only via a workflow name still comes back ✔.Two things I did not expect
Defect 2 in the issue has no functional impact. The loop reads
Commit.status.contexts, which is a plain LIST in GitHub's schema — nofirst, never truncated. Thecontexts(first: $parentsNumber)connection is onstatusCheckRollupand selected onlytotalCount, which is read nowhere. It was dead weight, not an under-count. It now has a real purpose and its own variable.Defaults are 50, not 100. A 100/100 first page makes GitHub exceed its own execution limit on the heaviest repository and return
504— cost is not the only ceiling. At 50/50 the new query fails in exactly the same places the old one does (on that repository both fail above-c 25and both succeed at-c 25, which is what the shared workflow passes), so this is not a new limit.GetCommitsnow names the knobs to turn when it happens.Diagnosability
The issue asked for not-found to read differently from found-but-failing. It does now:
That second line is not hypothetical — one consumer requests a check name that exists nowhere on its commits. No truncation involved, the name is simply stale, and until now that was indistinguishable from a failing build.
Verification
WAS A DEFECT, NOW FIXED; every other expectation is unchanged.go build,go vet,go test ./...,gofmt -l,golangci-lint(v1.44.2, as CI) all clean. Ago test ./...job is added — CI previously ran lint only.testdata/baseline/, always--dry-runand with an unsatisfiable expression, so nothing is ever promoted): 10 byte-identical verdicts across 244 commits, 1 differing only by a commit that landed between runs while agreeing on all 24 shared commits, 0 verdict changes.The baseline harness is tracked but its repository list and recordings are gitignored — recorded output embeds commit messages, author names and branch names, and this repository is public.
Not changed
One consumer's required check currently concludes
NEUTRALon recent commits, andNEUTRALdoes not satisfy a check. This PR does not change which conclusions count as passing.No tag. The shared reusable workflow defaults to the
latestimage tag, so releasing reaches every repository at once.