STACKITGit/cli
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(pr status): don't count cancelled checks as failures

Browse source 
When a GitHub Actions workflow uses concurrency with cancel-in-progress,
cancelled runs were counted as failures in `gh pr status` and
`gh pr view`, even when a newer run for the same check name succeeded.
The GitHub web UI and `gh pr checks` both handle this correctly.

Three changes fix this:

1. Add a `cancelled` check status category. Cancelled runs are now
   excluded from all summary counts (passing/failing/pending) and
   subtracted from the total, matching the web UI behavior.

2. Move `eliminateDuplicates` from pkg/cmd/pr/checks to
   `api.EliminateDuplicateChecks` (exported). The function operates
   entirely on `api.CheckContext` and is now shared by both `pr checks`
   and `ChecksStatus()` (used by `pr status` and `pr view`).

3. Apply deduplication in the `ChecksStatus()` slow path, keeping only
   the most recent run per check name — consistent with `pr checks`.

Fixes #12895

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
Kynan Ware 2026-03-11 12:33:31 -06:00
parent 19864b9e1e
commit 1c274a8a56
5 changed files with 296 additions and 50 deletions
Download patch file Download diff file Expand all files Collapse all files

248
api/pull_request_test.go
View file

@ -3,7 +3,9 @@ package api
import (
"encoding/json"
"fmt"
"reflect"
"testing"
"time"
"github.com/stretchr/testify/require"
)
@ -100,14 +102,14 @@ func TestChecksStatus_SummarisingCheckRuns(t *testing.T) {
expectedChecksStatus: PullRequestChecksStatus{Failing: 1, Total: 1},
},
{
name: "COMPLETED / CANCELLED is treated as Failing",
name: "COMPLETED / CANCELLED is excluded from counts",
payload: singleCompletedCheckRunWithConclusion("CANCELLED"),
expectedChecksStatus: PullRequestChecksStatus{Failing: 1, Total: 1},
expectedChecksStatus: PullRequestChecksStatus{},
},
{
name: "COMPLETED / CANCELLED is treated as Failing",
name: "COMPLETED / CANCELLED is excluded from counts (duplicate)",
payload: singleCompletedCheckRunWithConclusion("CANCELLED"),
expectedChecksStatus: PullRequestChecksStatus{Failing: 1, Total: 1},
expectedChecksStatus: PullRequestChecksStatus{},
},
{
name: "COMPLETED / FAILURE is treated as Failing",
@ -208,9 +210,9 @@ func TestChecksStatus_SummarisingCheckRunsAndStatusContexts(t *testing.T) {
}
} }] } }
`,
completedCheckRunNode("SUCCESS"),
statusContextNode("PENDING"),
completedCheckRunNode("FAILURE"),
completedCheckRunNodeWithName("build", "SUCCESS"),
statusContextNodeWithName("ci/deploy", "PENDING"),
completedCheckRunNodeWithName("lint", "FAILURE"),
)
var pr PullRequest
@ -332,9 +334,9 @@ func TestChecksStatus_SummarisingCheckRunAndStatusContextCountsByState(t *testin
expectedChecksStatus := PullRequestChecksStatus{
Pending: 11,
Failing: 6,
Failing: 5,
Passing: 4,
Total: 20,
Total: 19,
}
require.Equal(t, expectedChecksStatus, pr.ChecksStatus())
}
@ -404,6 +406,16 @@ func completedCheckRunNode(conclusion string) string {
}`, conclusion)
}
func completedCheckRunNodeWithName(name, conclusion string) string {
return fmt.Sprintf(`
{
"__typename": "CheckRun",
"name": "%s",
"status": "COMPLETED",
"conclusion": "%s"
}`, name, conclusion)
}
func statusContextNode(state string) string {
return fmt.Sprintf(`
{
@ -411,3 +423,221 @@ func statusContextNode(state string) string {
"state": "%s"
}`, state)
}
func statusContextNodeWithName(context, state string) string {
return fmt.Sprintf(`
{
"__typename": "StatusContext",
"context": "%s",
"state": "%s"
}`, context, state)
}
func TestChecksStatus_DuplicateCheckRunsAreDeduplicated(t *testing.T) {
t.Parallel()
// Simulate cancel-in-progress: a cancelled run followed by a newer successful run
// for the same check name. Only the newer (successful) run should be counted.
pr := PullRequest{}
pr.StatusCheckRollup.Nodes = []StatusCheckRollupNode{
{
Commit: StatusCheckRollupCommit{
StatusCheckRollup: CommitStatusCheckRollup{
Contexts: CheckContexts{
Nodes: []CheckContext{
{
TypeName: "CheckRun",
Name: "Prevent Merging",
Status: "COMPLETED",
Conclusion: CheckConclusionStateCancelled,
StartedAt: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC),
},
{
TypeName: "CheckRun",
Name: "Prevent Merging",
Status: "COMPLETED",
Conclusion: CheckConclusionStateSuccess,
StartedAt: time.Date(2024, 1, 1, 1, 0, 0, 0, time.UTC),
},
{
TypeName: "CheckRun",
Name: "Build",
Status: "COMPLETED",
Conclusion: CheckConclusionStateSuccess,
StartedAt: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC),
},
},
},
},
},
},
}
status := pr.ChecksStatus()
require.Equal(t, PullRequestChecksStatus{
Passing: 2,
Failing: 0,
Pending: 0,
Total: 2,
}, status)
}
func TestEliminateDuplicateChecks(t *testing.T) {
t.Parallel()
tests := []struct {
name string
checkContexts []CheckContext
want []CheckContext
}{
{
name: "duplicate CheckRun keeps most recent",
checkContexts: []CheckContext{
{
TypeName: "CheckRun",
Name: "lint",
Status: "COMPLETED",
Conclusion: "FAILURE",
StartedAt: time.Date(2022, 1, 1, 1, 1, 1, 1, time.UTC),
},
{
TypeName: "CheckRun",
Name: "lint",
Status: "COMPLETED",
Conclusion: "SUCCESS",
StartedAt: time.Date(2022, 2, 2, 2, 2, 2, 2, time.UTC),
},
{
TypeName: "CheckRun",
Name: "build",
Status: "COMPLETED",
Conclusion: "SUCCESS",
StartedAt: time.Date(2022, 1, 1, 1, 1, 1, 1, time.UTC),
},
},
want: []CheckContext{
{
TypeName: "CheckRun",
Name: "lint",
Status: "COMPLETED",
Conclusion: "SUCCESS",
StartedAt: time.Date(2022, 2, 2, 2, 2, 2, 2, time.UTC),
},
{
TypeName: "CheckRun",
Name: "build",
Status: "COMPLETED",
Conclusion: "SUCCESS",
StartedAt: time.Date(2022, 1, 1, 1, 1, 1, 1, time.UTC),
},
},
},
{
name: "duplicate StatusContext keeps most recent",
checkContexts: []CheckContext{
{
TypeName: "StatusContext",
Context: "ci/test",
State: "FAILURE",
StartedAt: time.Date(2022, 1, 1, 1, 1, 1, 1, time.UTC),
},
{
TypeName: "StatusContext",
Context: "ci/test",
State: "SUCCESS",
StartedAt: time.Date(2022, 2, 2, 2, 2, 2, 2, time.UTC),
},
},
want: []CheckContext{
{
TypeName: "StatusContext",
Context: "ci/test",
State: "SUCCESS",
StartedAt: time.Date(2022, 2, 2, 2, 2, 2, 2, time.UTC),
},
},
},
{
name: "unique checks are preserved",
checkContexts: []CheckContext{
{
TypeName: "CheckRun",
Name: "build",
Status: "COMPLETED",
Conclusion: "SUCCESS",
StartedAt: time.Date(2022, 1, 1, 1, 1, 1, 1, time.UTC),
},
{
TypeName: "StatusContext",
Context: "ci/test",
State: "SUCCESS",
StartedAt: time.Date(2022, 1, 1, 1, 1, 1, 1, time.UTC),
},
},
want: []CheckContext{
{
TypeName: "CheckRun",
Name: "build",
Status: "COMPLETED",
Conclusion: "SUCCESS",
StartedAt: time.Date(2022, 1, 1, 1, 1, 1, 1, time.UTC),
},
{
TypeName: "StatusContext",
Context: "ci/test",
State: "SUCCESS",
StartedAt: time.Date(2022, 1, 1, 1, 1, 1, 1, time.UTC),
},
},
},
{
name: "different workflow names are not deduplicated",
checkContexts: []CheckContext{
{
TypeName: "CheckRun",
Name: "build",
Status: "COMPLETED",
Conclusion: "SUCCESS",
StartedAt: time.Date(2022, 1, 1, 1, 1, 1, 1, time.UTC),
CheckSuite: CheckSuite{WorkflowRun: WorkflowRun{Event: "push", Workflow: Workflow{Name: "CI"}}},
},
{
TypeName: "CheckRun",
Name: "build",
Status: "COMPLETED",
Conclusion: "SUCCESS",
StartedAt: time.Date(2022, 1, 1, 1, 1, 1, 1, time.UTC),
CheckSuite: CheckSuite{WorkflowRun: WorkflowRun{Event: "push", Workflow: Workflow{Name: "Release"}}},
},
},
want: []CheckContext{
{
TypeName: "CheckRun",
Name: "build",
Status: "COMPLETED",
Conclusion: "SUCCESS",
StartedAt: time.Date(2022, 1, 1, 1, 1, 1, 1, time.UTC),
CheckSuite: CheckSuite{WorkflowRun: WorkflowRun{Event: "push", Workflow: Workflow{Name: "CI"}}},
},
{
TypeName: "CheckRun",
Name: "build",
Status: "COMPLETED",
Conclusion: "SUCCESS",
StartedAt: time.Date(2022, 1, 1, 1, 1, 1, 1, time.UTC),
CheckSuite: CheckSuite{WorkflowRun: WorkflowRun{Event: "push", Workflow: Workflow{Name: "Release"}}},
},
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
got := EliminateDuplicateChecks(tt.checkContexts)
if !reflect.DeepEqual(tt.want, got) {
t.Errorf("got EliminateDuplicateChecks %+v, want %+v", got, tt.want)
}
})
}
}

47
api/queries_pr.go
View file

@ -4,6 +4,7 @@ import (
"fmt"
"net/http"
"net/url"
"sort"
"time"
"github.com/cli/cli/v2/internal/ghrepo"
@ -347,6 +348,8 @@ func (pr *PullRequest) ChecksStatus() PullRequestChecksStatus {
summary.Passing += countByState.Count
case failing:
summary.Failing += countByState.Count
case cancelled:
summary.Total -= countByState.Count
default:
summary.Pending += countByState.Count
}
@ -367,7 +370,7 @@ func (pr *PullRequest) ChecksStatus() PullRequestChecksStatus {
}
// 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 contexts.Nodes {
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" {
@ -379,6 +382,8 @@ func (pr *PullRequest) ChecksStatus() PullRequestChecksStatus {
summary.Passing++
case failing:
summary.Failing++
case cancelled:
continue
default:
summary.Pending++
}
@ -407,9 +412,10 @@ func (pr *PullRequest) ChecksStatus() PullRequestChecksStatus {
type checkStatus int
const (
passing checkStatus = iota
passing checkStatus = iota
failing
pending
cancelled
)
func parseCheckStatusFromStatusState(state StatusState) checkStatus {
@ -432,8 +438,10 @@ func parseCheckStatusFromCheckRunState(state CheckRunState) checkStatus {
switch state {
case CheckRunStateNeutral, CheckRunStateSkipped, CheckRunStateSuccess:
return passing
case CheckRunStateActionRequired, CheckRunStateCancelled, CheckRunStateFailure, CheckRunStateTimedOut:
case CheckRunStateActionRequired, CheckRunStateFailure, CheckRunStateTimedOut:
return failing
case CheckRunStateCancelled:
return cancelled
case CheckRunStateCompleted, CheckRunStateInProgress, CheckRunStatePending, CheckRunStateQueued,
CheckRunStateStale, CheckRunStateStartupFailure, CheckRunStateWaiting:
return pending
@ -449,8 +457,10 @@ func parseCheckStatusFromCheckConclusionState(state CheckConclusionState) checkS
switch state {
case CheckConclusionStateNeutral, CheckConclusionStateSkipped, CheckConclusionStateSuccess:
return passing
case CheckConclusionStateActionRequired, CheckConclusionStateCancelled, CheckConclusionStateFailure, CheckConclusionStateTimedOut:
case CheckConclusionStateActionRequired, CheckConclusionStateFailure, CheckConclusionStateTimedOut:
return failing
case CheckConclusionStateCancelled:
return cancelled
case CheckConclusionStateStale, CheckConclusionStateStartupFailure:
return pending
// Currently, we treat anything unknown as pending, which includes any future unknown
@ -461,6 +471,35 @@ func parseCheckStatusFromCheckConclusionState(state CheckConclusionState) checkS
}
}
// EliminateDuplicateChecks filters a set of checks to only the most recent ones if the set
// includes repeated runs. It deduplicates CheckRun entries by name/workflow/event and
// StatusContext entries by context name, keeping the most recently started entry in each case.
func EliminateDuplicateChecks(checkContexts []CheckContext) []CheckContext {
sort.Slice(checkContexts, func(i, j int) bool { return checkContexts[i].StartedAt.After(checkContexts[j].StartedAt) })
mapChecks := make(map[string]struct{})
mapContexts := make(map[string]struct{})
unique := make([]CheckContext, 0, len(checkContexts))
for _, ctx := range checkContexts {
if ctx.Context != "" {
if _, exists := mapContexts[ctx.Context]; exists {
continue
}
mapContexts[ctx.Context] = struct{}{}
} else {
key := fmt.Sprintf("%s/%s/%s", ctx.Name, ctx.CheckSuite.WorkflowRun.Workflow.Name, ctx.CheckSuite.WorkflowRun.Event)
if _, exists := mapChecks[key]; exists {
continue
}
mapChecks[key] = struct{}{}
}
unique = append(unique, ctx)
}
return unique
}
// CreatePullRequest creates a pull request in a GitHub repository
func CreatePullRequest(client *Client, repo *Repository, params map[string]interface{}) (*PullRequest, error) {
query := `

31
pkg/cmd/pr/checks/aggregate.go
View file

@ -1,8 +1,6 @@
package checks
import (
"fmt"
"sort"
"time"
"github.com/cli/cli/v2/api"
@ -34,7 +32,7 @@ func (ch *check) ExportData(fields []string) map[string]interface{} {
}
func aggregateChecks(checkContexts []api.CheckContext, requiredChecks bool) (checks []check, counts checkCounts) {
for _, c := range eliminateDuplicates(checkContexts) {
for _, c := range api.EliminateDuplicateChecks(checkContexts) {
if requiredChecks && !c.IsRequired {
continue
}
@ -91,30 +89,3 @@ func aggregateChecks(checkContexts []api.CheckContext, requiredChecks bool) (che
}
return
}
// eliminateDuplicates filters a set of checks to only the most recent ones if the set includes repeated runs
func eliminateDuplicates(checkContexts []api.CheckContext) []api.CheckContext {
sort.Slice(checkContexts, func(i, j int) bool { return checkContexts[i].StartedAt.After(checkContexts[j].StartedAt) })
mapChecks := make(map[string]struct{})
mapContexts := make(map[string]struct{})
unique := make([]api.CheckContext, 0, len(checkContexts))
for _, ctx := range checkContexts {
if ctx.Context != "" {
if _, exists := mapContexts[ctx.Context]; exists {
continue
}
mapContexts[ctx.Context] = struct{}{}
} else {
key := fmt.Sprintf("%s/%s/%s", ctx.Name, ctx.CheckSuite.WorkflowRun.Workflow.Name, ctx.CheckSuite.WorkflowRun.Event)
if _, exists := mapChecks[key]; exists {
continue
}
mapChecks[key] = struct{}{}
}
unique = append(unique, ctx)
}
return unique
}

2
pkg/cmd/pr/checks/checks_test.go
View file

@ -1018,7 +1018,7 @@ func TestEliminateDuplicates(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := eliminateDuplicates(tt.checkContexts)
got := api.EliminateDuplicateChecks(tt.checkContexts)
if !reflect.DeepEqual(tt.want, got) {
t.Errorf("got eliminateDuplicates %+v, want %+v\n", got, tt.want)
}

18
pkg/cmd/pr/status/fixtures/prStatusChecks.json
View file

@ -26,7 +26,8 @@
"contexts": {
"nodes": [
{
"__typeName": "StatusContext",
"__typename": "StatusContext",
"context": "ci/build",
"state": "SUCCESS"
}
]
@ -81,7 +82,8 @@
"contexts": {
"nodes": [
{
"__typeName": "CheckRun",
"__typename": "CheckRun",
"name": "build",
"status": "IN_PROGRESS",
"conclusion": ""
}
@ -111,16 +113,19 @@
"contexts": {
"nodes": [
{
"__typeName": "CheckRun",
"__typename": "CheckRun",
"name": "build",
"status": "IN_PROGRESS",
"conclusion": ""
},
{
"__typeName": "StatusContext",
"__typename": "StatusContext",
"context": "ci/deploy",
"state": "FAILURE"
},
{
"__typeName": "CheckRun",
"__typename": "CheckRun",
"name": "lint",
"status": "COMPLETED",
"conclusion": "NEUTRAL"
}
@ -149,7 +154,8 @@
"contexts": {
"nodes": [
{
"__typeName": "StatusContext",
"__typename": "StatusContext",
"context": "ci/build",
"state": "SUCCESS"
}
]