Remove ChecksStatus slow path (dead code on all supported GHES)#12909
Open
BagToad wants to merge 1 commit intofix-pr-status-cancelledfrom
Open
Remove ChecksStatus slow path (dead code on all supported GHES)#12909BagToad wants to merge 1 commit intofix-pr-status-cancelledfrom
ChecksStatus slow path (dead code on all supported GHES)#12909BagToad wants to merge 1 commit intofix-pr-status-cancelledfrom
Conversation
All supported GHES versions (3.16 through 3.20) support the checkRunCountsByState and statusContextCountsByState fields on StatusCheckRollupContextConnection. The slow path that iterated individual CheckContext nodes in ChecksStatus() is dead code. This commit: - Removes the slow path from ChecksStatus(), keeping only the aggregated counts-by-state path - Removes parseCheckStatusFromCheckConclusionState (no callers remain) - Removes CheckRunAndStatusContextCounts from PullRequestFeatures and its introspection detection - Consolidates the two feature detection introspection queries into one (PullRequest + WorkflowRun fits within the platform limit of two __type expressions) - Removes the errgroup dependency from feature detection - Always uses statusCheckRollupWithCountByState in pr status queries - Updates pr view fixtures to include counts-by-state fields Co-authored-by: Copilot <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
Removes the legacy “slow path” for deriving PR check status and standardizes on the GraphQL aggregated counts-by-state fields that are available on all supported GHES versions, while also cleaning up related feature detection and test fixtures.
Changes:
- Simplifies
PullRequest.ChecksStatus()to use only counts-by-state and removes related conclusion parsing. - Removes
CheckRunAndStatusContextCountsfeature detection and consolidates PR feature detection queries. - Exports check deduplication as
api.EliminateDuplicateChecksand updates callers/tests; updatespr status+pr viewfixtures/tests for counts-by-state.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/cmd/pr/view/fixtures/prViewPreviewWithSomeChecksPending.json | Adds counts-by-state fields to PR view preview fixture. |
| pkg/cmd/pr/view/fixtures/prViewPreviewWithSomeChecksFailing.json | Adds counts-by-state fields to PR view preview fixture. |
| pkg/cmd/pr/view/fixtures/prViewPreviewWithAllChecksPassing.json | Adds counts-by-state fields to PR view preview fixture. |
| pkg/cmd/pr/view/fixtures/prViewPreviewWithAllChecksFailing.json | Adds counts-by-state fields to PR view preview fixture. |
| pkg/cmd/pr/status/status_test.go | Removes slow-path test wiring; tests only counts-by-state path. |
| pkg/cmd/pr/status/status.go | Removes feature detection plumbing from pr status execution path. |
| pkg/cmd/pr/status/http.go | Always queries statusCheckRollupWithCountByState for pr status. |
| pkg/cmd/pr/status/fixtures/prStatusChecks.json | Updates typename/name/context fields in legacy fixture. |
| pkg/cmd/pr/checks/checks_test.go | Switches deduplication tests to api.EliminateDuplicateChecks. |
| pkg/cmd/pr/checks/aggregate.go | Uses api.EliminateDuplicateChecks and deletes local dedup implementation. |
| internal/featuredetection/feature_detection_test.go | Updates PR feature detection stubs/expectations after removing counts feature. |
| internal/featuredetection/feature_detection.go | Removes counts feature + errgroup usage; single introspection query for PR/workflow features. |
| api/queries_pr.go | Removes ChecksStatus slow path; adds cancelled handling; introduces api.EliminateDuplicateChecks. |
| api/pull_request_test.go | Reworks ChecksStatus tests for counts-by-state and adds deduplication tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
340
to
+356
| contexts := pr.StatusCheckRollup.Nodes[0].Commit.StatusCheckRollup.Contexts | ||
|
|
||
| // If this commit has counts by state then we can summarise check status from those | ||
| if len(contexts.CheckRunCountsByState) != 0 && len(contexts.StatusContextCountsByState) != 0 { | ||
| summary.Total = contexts.CheckRunCount + contexts.StatusContextCount | ||
| for _, countByState := range contexts.CheckRunCountsByState { | ||
| switch parseCheckStatusFromCheckRunState(countByState.State) { | ||
| case passing: | ||
| summary.Passing += countByState.Count | ||
| case failing: | ||
| summary.Failing += countByState.Count | ||
| case cancelled: | ||
| summary.Total -= countByState.Count | ||
| default: | ||
| summary.Pending += countByState.Count | ||
| } | ||
| } | ||
|
|
||
| for _, countByState := range contexts.StatusContextCountsByState { | ||
| switch parseCheckStatusFromStatusState(countByState.State) { | ||
| case passing: | ||
| summary.Passing += countByState.Count | ||
| case failing: | ||
| summary.Failing += countByState.Count | ||
| default: | ||
| summary.Pending += countByState.Count | ||
| } | ||
| summary.Total = contexts.CheckRunCount + contexts.StatusContextCount | ||
| for _, countByState := range contexts.CheckRunCountsByState { | ||
| switch parseCheckStatusFromCheckRunState(countByState.State) { | ||
| case passing: | ||
| summary.Passing += countByState.Count | ||
| case failing: | ||
| summary.Failing += countByState.Count | ||
| case cancelled: | ||
| summary.Total -= countByState.Count | ||
| default: | ||
| summary.Pending += countByState.Count | ||
| } | ||
|
|
||
| return summary | ||
| } | ||
|
|
||
| // If we don't have the counts by state, then we'll need to summarise by looking at the more detailed contexts | ||
| for _, c := range EliminateDuplicateChecks(contexts.Nodes) { | ||
| // Nodes are a discriminated union of CheckRun or StatusContext and we can match on | ||
| // the TypeName to narrow the type. | ||
| if c.TypeName == "CheckRun" { | ||
| // https://docs.github.com/en/graphql/reference/enums#checkstatusstate | ||
| // If the status is completed then we can check the conclusion field | ||
| if c.Status == "COMPLETED" { | ||
| switch parseCheckStatusFromCheckConclusionState(c.Conclusion) { | ||
| case passing: | ||
| summary.Passing++ | ||
| case failing: | ||
| summary.Failing++ | ||
| case cancelled: | ||
| continue | ||
| default: | ||
| summary.Pending++ | ||
| } | ||
| // otherwise we're in some form of pending state: | ||
| // "COMPLETED", "IN_PROGRESS", "PENDING", "QUEUED", "REQUESTED", "WAITING" or otherwise unknown | ||
| } else { | ||
| summary.Pending++ | ||
| } | ||
|
|
||
| } else { // c.TypeName == StatusContext | ||
| switch parseCheckStatusFromStatusState(c.State) { | ||
| case passing: | ||
| summary.Passing++ | ||
| case failing: | ||
| summary.Failing++ | ||
| default: | ||
| summary.Pending++ | ||
| } | ||
| for _, countByState := range contexts.StatusContextCountsByState { |
There was a problem hiding this comment.
PullRequest.ChecksStatus() now always summarizes from checkRunCountsByState/statusContextCountsByState and no longer falls back to iterating contexts.Nodes. However pr view still queries the statusCheckRollup pseudo-field, which expands to StatusCheckRollupGraphQLWithoutCountByState (nodes only, no counts). As a result, pr view will report checks.Total == 0 and omit the checks summary even when checks exist. Either update pr view/its finder fields to fetch the counts-by-state fields (or a combined query that includes both nodes + counts), or reintroduce a fallback path in ChecksStatus() when count fields are missing.
Comment on lines
+36
to
+47
| "contexts": { | ||
| "checkRunCount": 14, | ||
| "checkRunCountsByState": [ | ||
| { | ||
| "state": "ACTION_REQUIRED", | ||
| "count": 1 | ||
| }, | ||
| { | ||
| "state": "CANCELLED", | ||
| "count": 1 | ||
| }, | ||
| { |
There was a problem hiding this comment.
In this test payload, checkRunCount is set to 14 but checkRunCountsByState contains 15 entries with count: 1 each (sum=15). That makes Passing+Failing+Pending not line up with Total and doesn't reflect how the API reports these fields (counts-by-state should sum to the corresponding total count). Consider adjusting the payload (and expected Total) so the totals are internally consistent.
williammartin
requested changes
Mar 25, 2026
Member
There was a problem hiding this comment.
On gh 2.88.1:
➜ GH_FORCE_TTY=true gh pr view 12909 --repo cli/cli | cat
Remove `ChecksStatus` slow path (dead code on all supported GHES) cli/cli#12909
Open • BagToad (Kynan Ware) wants to merge 1 commit into fix-pr-status-cancelled from remove-checks-status-slow-path • about 13 days ago
+181 -666 • ✓ Checks passing
On this branch:
➜ GH_FORCE_TTY=true ./bin/gh pr view 12909 --repo cli/cli | cat
Remove `ChecksStatus` slow path (dead code on all supported GHES) cli/cli#12909
Open • BagToad (Kynan Ware) wants to merge 1 commit into fix-pr-status-cancelled from remove-checks-status-slow-path • about 13 days ago
+181 -666 • No checks
See the difference between ✓ Checks passing and No checks on the final line.
Here's a copilot diagnosis that I have not verified:
● The bug is a mismatch between what pr view queries and what ChecksStatus() reads.
pr view queries statusCheckRollup (view.go:89), which maps to StatusCheckRollupGraphQLWithoutCountByState() in the query builder (
query_builder.go:431-432). That GraphQL query requests individual check Nodes — it does NOT request checkRunCount, checkRunCountsByState,
etc.
ChecksStatus() now only reads count fields (queries_pr.go:342-365). This commit removed the slow path that iterated over Nodes. So when pr
view calls ChecksStatus(), all count fields are zero (they were never in the query), it returns Total=0, and the display function shows
"No checks".
The tests don't catch it because the commit also updated the pr view fixtures to include count fields — but the mock HTTP layer serves the
full fixture JSON regardless of what the GraphQL query actually requested. In production, the API only returns fields you ask for.
The fix is either:
1. Change view.go:89 to request "statusCheckRollupWithCountByState" instead of "statusCheckRollup", or
2. Keep a Nodes-based fallback in ChecksStatus() for callers that still use the old query
I checked and this regression is in this PR specifically, not #12906 on which this is based.
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.
Description
Stacked on #12906.
All supported GHES versions (3.16 through 3.20) support
checkRunCountsByStateandstatusContextCountsByStateonStatusCheckRollupContextConnection. The slow path that iterated individualCheckContextnodes inChecksStatus()is dead code.Changes Overview
ChecksStatus(), keeping only the aggregated counts-by-state pathparseCheckStatusFromCheckConclusionState(no callers remain)CheckRunAndStatusContextCountsfromPullRequestFeaturesand its GraphQL introspection detectionPullRequest+WorkflowRunfits within the platform limit of two__typeexpressions per query), removing theerrgroupdependencystatusCheckRollupWithCountByStateinpr statusqueries, removing the conditional togglepr viewfixtures to include counts-by-state fieldsNotes to Reviewers
Net result: -485 lines. The
EliminateDuplicateChecksfunction andStatusCheckRollupGraphQLWithoutCountByStatequery builder are kept becausepr checksstill uses individual nodes for its detailed check listing.Acceptance Testing Results
6 scenarios tested on github.com and GHES 3.20, all passing.
Full acceptance test results