From 7c88399401fbde6c07cbc529d42094adc2406ff5 Mon Sep 17 00:00:00 2001 From: Nawfal Saafa Date: Tue, 4 Aug 2026 19:51:44 +0100 Subject: [PATCH 1/3] fix github status check rollup mapping --- internal/scm/github/github.go | 93 +++++++++++++++++++++-------- internal/scm/github/github_test.go | 94 +++++++++++++++++++++++++----- 2 files changed, 150 insertions(+), 37 deletions(-) diff --git a/internal/scm/github/github.go b/internal/scm/github/github.go index 27866a65f..1ac021cb1 100644 --- a/internal/scm/github/github.go +++ b/internal/scm/github/github.go @@ -11,6 +11,7 @@ import ( "strings" "time" + "github.com/kunchenguid/no-mistakes/internal/safeurl" "github.com/kunchenguid/no-mistakes/internal/scm" ) @@ -297,45 +298,89 @@ func (h *Host) GetChecks(ctx context.Context, pr *scm.PR) ([]scm.Check, error) { if err != nil { return nil, err } - args := append([]string{"pr", "checks", selector}, h.repoArgs()...) - args = append(args, "--json", "name,state,bucket,completedAt,link") + args := append([]string{"pr", "view", selector}, h.repoArgs()...) + args = append(args, "--json", "statusCheckRollup") cmd := h.cmd(ctx, "gh", args...) out, err := cmd.CombinedOutput() if err != nil { - if strings.Contains(string(out), "no checks reported") { - return nil, nil - } - return nil, fmt.Errorf("gh pr checks: %w", err) + return nil, fmt.Errorf("gh pr view statusCheckRollup: %s: %w", boundedCommandOutput(out), err) } - var raw []struct { - Name string `json:"name"` - State string `json:"state"` - Bucket string `json:"bucket"` - CompletedAt string `json:"completedAt"` - Link string `json:"link"` + var raw struct { + StatusCheckRollup []githubStatusCheck `json:"statusCheckRollup"` } if err := json.Unmarshal(out, &raw); err != nil { return nil, fmt.Errorf("parse CI checks: %w", err) } - checks := make([]scm.Check, 0, len(raw)) - for _, r := range raw { - var completedAt time.Time - if r.CompletedAt != "" { - if parsed, parseErr := time.Parse(time.RFC3339, r.CompletedAt); parseErr == nil { - completedAt = parsed - } + checks := make([]scm.Check, 0, len(raw.StatusCheckRollup)) + for _, r := range raw.StatusCheckRollup { + state := strings.ToUpper(strings.TrimSpace(r.Conclusion)) + if state == "" { + state = strings.ToUpper(strings.TrimSpace(r.State)) } checks = append(checks, scm.Check{ - Name: r.Name, - Bucket: normalizeCheckBucket(r.Bucket, r.State), - State: strings.ToUpper(strings.TrimSpace(r.State)), - CompletedAt: completedAt, - Link: strings.TrimSpace(r.Link), + Name: r.checkName(), + Bucket: githubStatusCheckBucket(r), + State: state, + CompletedAt: parseGitHubTime(r.CompletedAt), + Link: strings.TrimSpace(r.detailsLink()), }) } return checks, nil } +// githubStatusCheck is the common subset returned by GitHub's CheckRun and +// StatusContext union members in statusCheckRollup. +type githubStatusCheck struct { + Type string `json:"__typename"` + Name string `json:"name"` + Context string `json:"context"` + State string `json:"state"` + Status string `json:"status"` + Conclusion string `json:"conclusion"` + CompletedAt string `json:"completedAt"` + DetailsURL string `json:"detailsUrl"` + TargetURL string `json:"targetUrl"` +} + +func (r githubStatusCheck) checkName() string { + if strings.TrimSpace(r.Name) != "" { + return strings.TrimSpace(r.Name) + } + return strings.TrimSpace(r.Context) +} + +func (r githubStatusCheck) detailsLink() string { + if strings.TrimSpace(r.DetailsURL) != "" { + return r.DetailsURL + } + return r.TargetURL +} + +func parseGitHubTime(raw string) time.Time { + parsed, _ := time.Parse(time.RFC3339, strings.TrimSpace(raw)) + return parsed +} + +func githubStatusCheckBucket(r githubStatusCheck) scm.CheckBucket { + if outcome := strings.TrimSpace(r.Conclusion); outcome != "" { + return normalizeCheckBucket("", outcome) + } + if state := strings.TrimSpace(r.State); state != "" { + return normalizeCheckBucket("", state) + } + return normalizeCheckBucket("", r.Status) +} + +const maxGitHubCommandOutput = 4096 + +func boundedCommandOutput(out []byte) string { + text := strings.TrimSpace(safeurl.RedactText(string(out))) + if len(text) > maxGitHubCommandOutput { + return text[:maxGitHubCommandOutput] + "...[truncated]" + } + return text +} + // RerunCheck re-runs the Actions job behind check for the same commit, so a // check the provider cancelled rather than failed can be retried without a new // push. The job is identified from the check's details link, which is the only diff --git a/internal/scm/github/github_test.go b/internal/scm/github/github_test.go index 7da630403..90fe8ea14 100644 --- a/internal/scm/github/github_test.go +++ b/internal/scm/github/github_test.go @@ -98,8 +98,8 @@ func TestGetChecksPassesRepoFlag(t *testing.T) { t.Parallel() host := New(githubTestCmdFactory(map[string]githubTestResponse{ - "gh pr checks 123 --repo test/repo --json name,state,bucket,completedAt,link": { - stdout: `[{"name":"build","state":"SUCCESS","bucket":"pass"}]` + "\n", + "gh pr view 123 --repo test/repo --json statusCheckRollup": { + stdout: `{"statusCheckRollup":[{"__typename":"CheckRun","name":"build","conclusion":"SUCCESS"}]}` + "\n", }, }), nil, "", "test/repo") @@ -223,8 +223,8 @@ func TestGetChecksFallsBackToStateWhenBucketMissing(t *testing.T) { t.Parallel() host := New(githubTestCmdFactory(map[string]githubTestResponse{ - "gh pr checks 123 --json name,state,bucket,completedAt,link": { - stdout: `[{"name":"build","state":"FAILURE","bucket":""},{"name":"tests","state":"PENDING","bucket":""}]` + "\n", + "gh pr view 123 --json statusCheckRollup": { + stdout: `{"statusCheckRollup":[{"__typename":"CheckRun","name":"build","conclusion":"FAILURE"},{"__typename":"CheckRun","name":"tests","status":"IN_PROGRESS"}]}` + "\n", }, }), nil, "", "") @@ -277,7 +277,7 @@ func failIfInvokedCmdFactory(t *testing.T) CmdFactory { // Masking condition: the daemon runs gh from the detached bare gate repo whose // HEAD is the default branch (main). // Symptom: appending an empty pr.Number produced an argument-less -// `gh pr checks --repo `, so gh fell back to resolving the cwd branch +// `gh pr view --repo `, so gh fell back to resolving the cwd branch // (main) and reported "no pull requests found for branch main" even though the // feature PR's exact-head checks are green — certification could never finish. // @@ -287,7 +287,7 @@ func TestGetChecksTargetsKnownPRByURLWhenNumberMissing(t *testing.T) { t.Parallel() var recorded [][]string - host := New(recordingCmdFactory("[]\n", &recorded), nil, "", "test/repo") + host := New(recordingCmdFactory("{\"statusCheckRollup\":[]}\n", &recorded), nil, "", "test/repo") prURL := "https://github.com/test/repo/pull/123" if _, err := host.GetChecks(context.Background(), &scm.PR{URL: prURL}); err != nil { @@ -297,8 +297,8 @@ func TestGetChecksTargetsKnownPRByURLWhenNumberMissing(t *testing.T) { t.Fatalf("expected exactly one gh invocation, got %d: %v", len(recorded), recorded) } got := recorded[0] - // argv is: gh pr checks --repo ... - if len(got) < 4 || got[1] != "pr" || got[2] != "checks" { + // argv is: gh pr view --repo ... + if len(got) < 4 || got[1] != "pr" || got[2] != "view" { t.Fatalf("unexpected argv: %v", got) } selector := got[3] @@ -313,7 +313,7 @@ func TestGetChecksTargetsKnownPRByNumber(t *testing.T) { t.Parallel() var recorded [][]string - host := New(recordingCmdFactory("[]\n", &recorded), nil, "", "test/repo") + host := New(recordingCmdFactory("{\"statusCheckRollup\":[]}\n", &recorded), nil, "", "test/repo") if _, err := host.GetChecks(context.Background(), &scm.PR{Number: "123", URL: "https://github.com/test/repo/pull/123"}); err != nil { t.Fatalf("GetChecks() error = %v", err) @@ -376,8 +376,8 @@ func TestGetChecksParsesCompletedAt(t *testing.T) { t.Parallel() host := New(githubTestCmdFactory(map[string]githubTestResponse{ - "gh pr checks 123 --json name,state,bucket,completedAt,link": { - stdout: `[{"name":"build","state":"FAILURE","bucket":"fail","completedAt":"2026-04-24T04:15:00Z"},{"name":"tests","state":"SUCCESS","bucket":"pass","completedAt":"not-a-time"}]` + "\n", + "gh pr view 123 --json statusCheckRollup": { + stdout: `{"statusCheckRollup":[{"__typename":"CheckRun","name":"build","conclusion":"FAILURE","completedAt":"2026-04-24T04:15:00Z"},{"__typename":"CheckRun","name":"tests","conclusion":"SUCCESS","completedAt":"not-a-time"}]}` + "\n", }, }), nil, "", "") @@ -403,8 +403,8 @@ func TestGetChecksParsesStateAndLink(t *testing.T) { const link = "https://github.com/test/repo/actions/runs/900/job/901" host := New(githubTestCmdFactory(map[string]githubTestResponse{ - "gh pr checks 123 --json name,state,bucket,completedAt,link": { - stdout: `[{"name":"build","state":"cancelled","bucket":"cancel","link":"` + link + `"}]` + "\n", + "gh pr view 123 --json statusCheckRollup": { + stdout: `{"statusCheckRollup":[{"__typename":"CheckRun","name":"build","conclusion":"CANCELLED","detailsUrl":"` + link + `"}]}` + "\n", }, }), nil, "", "") @@ -426,6 +426,74 @@ func TestGetChecksParsesStateAndLink(t *testing.T) { } } +func TestGetChecksMapsStatusRollupBuckets(t *testing.T) { + t.Parallel() + + host := New(githubTestCmdFactory(map[string]githubTestResponse{ + "gh pr view 58 --json statusCheckRollup": { + stdout: `{"statusCheckRollup":[ + {"__typename":"CheckRun","name":"pass","conclusion":"SUCCESS"}, + {"__typename":"CheckRun","name":"fail","conclusion":"FAILURE"}, + {"__typename":"CheckRun","name":"pending","status":"IN_PROGRESS"}, + {"__typename":"CheckRun","name":"skip","conclusion":"SKIPPED"}, + {"__typename":"StatusContext","context":"third-party","state":"SUCCESS","targetUrl":"https://example.test/check"} + ]}` + "\n", + }, + }), nil, "", "") + + checks, err := host.GetChecks(context.Background(), &scm.PR{Number: "58"}) + if err != nil { + t.Fatalf("GetChecks() error = %v", err) + } + want := map[string]scm.CheckBucket{ + "pass": scm.CheckBucketPass, "fail": scm.CheckBucketFail, + "pending": scm.CheckBucketPending, "skip": scm.CheckBucketSkip, + "third-party": scm.CheckBucketPass, + } + if len(checks) != len(want) { + t.Fatalf("len(checks) = %d, want %d: %+v", len(checks), len(want), checks) + } + for _, check := range checks { + if check.Bucket != want[check.Name] { + t.Errorf("check %q bucket = %q, want %q", check.Name, check.Bucket, want[check.Name]) + } + } +} + +func TestGetChecksZeroStatusRollupIsEmpty(t *testing.T) { + t.Parallel() + + host := New(githubTestCmdFactory(map[string]githubTestResponse{ + "gh pr view 58 --json statusCheckRollup": {stdout: `{"statusCheckRollup":[]}` + "\n"}, + }), nil, "", "") + checks, err := host.GetChecks(context.Background(), &scm.PR{Number: "58"}) + if err != nil || len(checks) != 0 { + t.Fatalf("GetChecks() = %+v, %v, want zero checks", checks, err) + } +} + +func TestGetChecksBoundsAndRedactsFailureOutput(t *testing.T) { + t.Parallel() + + const token = "secret-token" + output := "https://user:" + token + "@github.example.test/org/repo " + strings.Repeat("x", maxGitHubCommandOutput+500) + host := New(githubTestCmdFactory(map[string]githubTestResponse{ + "gh pr view 58 --json statusCheckRollup": {stderr: output, code: 1}, + }), nil, "", "") + + _, err := host.GetChecks(context.Background(), &scm.PR{Number: "58"}) + if err == nil { + t.Fatal("GetChecks() error = nil, want CLI error") + } + message := err.Error() + if strings.Contains(message, token) || strings.Contains(message, "https://user:") { + t.Fatalf("GetChecks() leaked credential: %q", message) + } + if !strings.Contains(message, "...[truncated]") { + t.Fatalf("GetChecks() failure was not bounded: %q", message) + } +} + // A rerun must target the exact job behind the check so a genuinely failing job // in the same workflow run is not re-run along with it. Real details URLs carry // a query (?check_suite_focus=true) or a step fragment (#step:4:12), and neither From 7716e70bbe6e2aa289e25c77a68401ccfe71257c Mon Sep 17 00:00:00 2001 From: Nawfal Saafa Date: Wed, 5 Aug 2026 13:58:59 +0100 Subject: [PATCH 2/3] test: update GitHub CI fixtures for status rollups --- internal/pipeline/steps/helpers_test.go | 3 +- internal/pipeline/steps/steps_test.go | 54 ++++++++++++++++++++----- 2 files changed, 46 insertions(+), 11 deletions(-) diff --git a/internal/pipeline/steps/helpers_test.go b/internal/pipeline/steps/helpers_test.go index 26e59597c..6b7610eee 100644 --- a/internal/pipeline/steps/helpers_test.go +++ b/internal/pipeline/steps/helpers_test.go @@ -421,7 +421,8 @@ func newTestContextWithDBRecords(t *testing.T, ag agent.Agent, workDir, baseSHA, } // fakeCIGH creates a fake gh binary that responds to CI-related -// commands (pr view --json state, pr checks --json, pr view --json comments). +// commands (pr view --json state, pr view --json statusCheckRollup, +// pr view --json comments). func fakeCIGH(t *testing.T, state, checksJSON string) []string { t.Helper() binDir := fakeCLIBinDir(t) diff --git a/internal/pipeline/steps/steps_test.go b/internal/pipeline/steps/steps_test.go index 527e0e542..51489046c 100644 --- a/internal/pipeline/steps/steps_test.go +++ b/internal/pipeline/steps/steps_test.go @@ -1,6 +1,7 @@ package steps import ( + "encoding/json" "errors" "fmt" "io" @@ -308,14 +309,47 @@ func fakeCIGHReconcileHandler(args []string) { fmt.Println("MERGEABLE") os.Exit(0) } - if strings.Contains(joined, "pr checks") { - fmt.Println(`[{"name":"build","state":"SUCCESS","bucket":"pass"}]`) + if strings.Contains(joined, "pr view") && strings.Contains(joined, "--json statusCheckRollup") { + fmt.Println(`{"statusCheckRollup":[{"name":"build","state":"SUCCESS"}]}`) os.Exit(0) } fmt.Fprintln(os.Stderr, "unsupported reconcile gh argv:", joined) os.Exit(1) } +func fakeGitHubStatusRollup(checksJSON string) string { + var checks []map[string]any + if err := json.Unmarshal([]byte(checksJSON), &checks); err == nil { + for _, check := range checks { + if _, ok := check["detailsUrl"]; !ok { + if link, ok := check["link"]; ok { + check["detailsUrl"] = link + } + } + if _, hasState := check["state"]; !hasState { + if bucket, ok := check["bucket"].(string); ok { + switch bucket { + case "pass": + check["conclusion"] = "SUCCESS" + case "fail": + check["conclusion"] = "FAILURE" + case "pending": + check["status"] = "IN_PROGRESS" + case "cancel": + check["conclusion"] = "CANCELLED" + case "skipping": + check["conclusion"] = "SKIPPED" + } + } + } + } + if encoded, err := json.Marshal(checks); err == nil { + checksJSON = string(encoded) + } + } + return fmt.Sprintf(`{"statusCheckRollup":%s}`, checksJSON) +} + func fakeCIGHHandler(args []string) { state := os.Getenv("FAKE_CLI_STATE") stateErr := os.Getenv("FAKE_CLI_STATE_ERR") @@ -347,12 +381,12 @@ func fakeCIGHHandler(args []string) { fmt.Println(state) os.Exit(0) } - if strings.Contains(joined, "pr checks") { + if strings.Contains(joined, "pr view") && strings.Contains(joined, "--json statusCheckRollup") { if checksErr != "" { fmt.Fprintln(os.Stderr, checksErr) os.Exit(1) } - fmt.Println(checksJSON) + fmt.Println(fakeGitHubStatusRollup(checksJSON)) os.Exit(0) } if strings.Contains(joined, "run rerun") { @@ -401,7 +435,7 @@ func fakeCIGHSequenceHandler(args []string) { fmt.Println(state) os.Exit(0) } - if strings.Contains(joined, "pr checks") { + if strings.Contains(joined, "pr view") && strings.Contains(joined, "--json statusCheckRollup") { data, err := os.ReadFile(checksPath) if err != nil { fmt.Fprintln(os.Stderr, err) @@ -409,7 +443,7 @@ func fakeCIGHSequenceHandler(args []string) { } entries := strings.Split(strings.TrimSpace(string(data)), "\n") if len(entries) == 0 || entries[0] == "" { - fmt.Println("[]") + fmt.Println(`{"statusCheckRollup":[]}`) os.Exit(0) } @@ -426,7 +460,7 @@ func fakeCIGHSequenceHandler(args []string) { fmt.Fprintln(os.Stderr, err) os.Exit(1) } - fmt.Println(entries[index]) + fmt.Println(fakeGitHubStatusRollup(entries[index])) os.Exit(0) } if strings.Contains(joined, "run rerun") { @@ -559,9 +593,9 @@ func fakeCIGHNoChecksHandler(args []string) { if len(args) >= 2 && args[0] == "auth" && args[1] == "status" { os.Exit(0) } - if strings.Contains(joined, "pr checks") { - fmt.Fprintln(os.Stderr, "no checks reported on the 'feature/e2e' branch") - os.Exit(1) + if strings.Contains(joined, "pr view") && strings.Contains(joined, "--json statusCheckRollup") { + fmt.Println(`{"statusCheckRollup":[]}`) + os.Exit(0) } if strings.Contains(joined, "pr view") && strings.Contains(joined, "--json state") { fmt.Println("OPEN") From 7a9c5959555766fef56faa24f94a607ce5e85a52 Mon Sep 17 00:00:00 2001 From: Nawfal Saafa Date: Wed, 5 Aug 2026 14:02:54 +0100 Subject: [PATCH 3/3] no-mistakes(document): Update GitHub check-rollup documentation comments --- internal/git/hook_test.go | 7 ++++--- internal/scm/github/github.go | 12 ++++++------ 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/internal/git/hook_test.go b/internal/git/hook_test.go index c961fd463..fec7f9df8 100644 --- a/internal/git/hook_test.go +++ b/internal/git/hook_test.go @@ -833,9 +833,10 @@ func TestIsolateHooksPath_SkipsIsolationWhenWorktreeConfigUnsupported(t *testing // from this directory, and gh resolves the repository by running git there. // This guards the invariant that such a worktree is a real work tree with no // leaked core.bare and a resolvable origin - i.e. gh can determine the repo -// from cwd alone. See issue #255: when this breaks, `gh pr checks` fails every -// poll and the CI step hangs until ci_timeout. No real gh or network is -// needed; the failure is purely in git's repo resolution, which gh depends on. +// from cwd alone. See issue #255: when this breaks, the `gh pr view` status +// rollup query fails every poll and the CI step hangs until ci_timeout. No real +// gh or network is needed; the failure is purely in git's repo resolution, +// which gh depends on. func TestIsolateHooksPath_LinkedWorktreeResolvesRepoForCLI(t *testing.T) { ctx := context.Background() base := t.TempDir() diff --git a/internal/scm/github/github.go b/internal/scm/github/github.go index 1ac021cb1..0dba4132d 100644 --- a/internal/scm/github/github.go +++ b/internal/scm/github/github.go @@ -383,12 +383,12 @@ func boundedCommandOutput(out []byte) string { // RerunCheck re-runs the Actions job behind check for the same commit, so a // check the provider cancelled rather than failed can be retried without a new -// push. The job is identified from the check's details link, which is the only -// run/job identity `gh pr checks` reports: a link naming a job re-runs just that -// job (and its dependencies), and a link naming only a run re-runs that run's -// failed jobs. Anything else - a third-party status pointing at an external -// dashboard, or a run path this backend cannot read - names no re-runnable job, -// and the error says so rather than falling back to a wider rerun. +// push. The job is identified from the CheckRun details URL in the PR status +// rollup: a link naming a job re-runs just that job (and its dependencies), and +// a link naming only a run re-runs that run's failed jobs. Anything else - a +// third-party status pointing at an external dashboard, or a run path this +// backend cannot read - names no re-runnable job, and the error says so rather +// than falling back to a wider rerun. func (h *Host) RerunCheck(ctx context.Context, _ *scm.PR, check scm.Check) error { rerunArgs, ok := rerunTargetArgs(check.Link) if !ok {