diff --git a/AGENTS.md b/AGENTS.md index aa64ce856..b98599dba 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -148,6 +148,7 @@ Safest local verification sequence after non-trivial changes: - `ci_timeout` is an idle timeout, not an absolute deadline: only `timeoutAnchor` re-arms when the upstream default-branch tip advances, `started` stays fixed for poll pacing, and re-arm only ever extends the deadline (fail-safe on transient base-tip failures). Value semantics (`0` unset, negative unlimited sentinel, keyword parsing) live in `config.go`; keep `config.DefaultCITimeout` and `defaultConfigYAML` in sync (`TestDefaultConfigYAML_MatchesGoDefaults`). User-facing semantics are owned by `docs/src/content/docs/reference/global-config.md`. - GitHub readiness is the union of the exact current PR head commit's check rollup and every Actions workflow run returned by the Actions API for that same SHA. A workflow rejected before creating jobs/check-runs is absent from the commit rollup but still present in that API; run discovery errors and unknown run states fail closed instead of certifying a green rollup. Regressions: `TestGetChecksIncludesFailedWorkflowRunMissingFromPRRollup`, `TestGetChecksBindsRollupAcrossABAHeadMovement`, `TestCIStep_FailedHeadWorkflowRunPreventsChecksPassed`. +- When the GitHub status-check rollup itself is unreadable with the configured credential (a Checks-API capability failure, which fine-grained tokens hit while still being allowed to read Actions), `internal/scm/github/actions_fallback.go` derives evidence from the Actions REST API bound to the exact repository, PR, and published head. Only that classification opens the fallback; every other rollup failure keeps surfacing as before. It is fail-closed by construction: Actions cannot see other apps' check runs, so green is certified only when branch protection's required-check set is readable and every required identity maps to exactly one exact-head latest-attempt Actions result; non-green evidence needs no certification and is returned as-is. Every failure mode wraps `scm.ErrChecksUnavailable`, and the CI step parks with an ask-user outcome after `maxConsecutiveCheckReadFailures` consecutive evidence-free polls rather than stalling to `ci_timeout` (a successful read resets the budget). Regressions: `internal/scm/github/actions_fallback_test.go`, `TestCIStep_RepeatedEvidenceFailuresParkForADecision`, `TestCIStep_SuccessfulReadResetsTheEvidenceFailureBudget`. - CI readiness never treats an unproven empty forge check list as green. Ready requires observed all-green checks, or trusted default-branch `no_ci: true` with zero registered checks (`internal/pipeline/steps/ci.go` decides whether the declaration applies; `internal/cimonitor` owns the agent-facing log vocabulary and Ready/DeclaredNoCI parse). Delayed registration, pending checks, failures, errors, unknowns, and stale-head evidence stay not-ready; registered checks on a declared no-CI repo are still honored. Regressions: `TestChecksPassed_PR607RealLogSequence`, `TestCIStep_EmptyChecksWithoutNoCIStaysNotReadyPastOldGracePeriod`, `TestCIStep_EmptyChecksWithTrustedNoCIBecomesReady`, `TestCIStep_DelayedCheckRegistrationStaysNotReadyUntilGreen`, `TestCIStep_DeclaredNoCIWithUnexpectedChecksHonorsThem`, `TestEffectiveRepoConfig_NoCITrustedOnly`. - Reap an orphaned monitor from outside its worktree with `no-mistakes axi abort --run `; it needs only `NM_HOME` and never starts a stopped daemon. A known run succeeds only with durable terminal truth, a recorded nonterminal run fails unconfirmed, and only an unknown id is an idempotent no-op. Bare `axi abort` stays worktree/branch-scoped. - A merged or closed PR observation transactionally completes an active run and its CI step; PR lifecycle state is monotonic, so duplicate or delayed observations cannot reactivate or regress a terminal run. Startup reconciles legacy `pending` or `running` rows that already hold terminal PR state before parked-run planning and generic crash recovery. Regressions: `TestUpdateRunPRStateFinalizesActiveTerminalOutcomes`, `TestUpdateRunPRStateIgnoresDuplicateAndDelayedRegressions`, `TestReconcileTerminalPRRunsFinalizesLegacyActiveRows`, `TestRecoverOnStartup_FinalizesLegacyTerminalPRRun`, e2e `TestTerminalPRRunDisappearsFromActiveListing`. diff --git a/docs/src/content/docs/reference/pipeline-steps.md b/docs/src/content/docs/reference/pipeline-steps.md index f6de0fc99..c09b102e3 100644 --- a/docs/src/content/docs/reference/pipeline-steps.md +++ b/docs/src/content/docs/reference/pipeline-steps.md @@ -279,6 +279,8 @@ Monitors PR health after creation and auto-fixes CI failures. Mergeability polli - The [`ci_timeout` reference](/no-mistakes/reference/global-config/#ci_timeout) owns idle re-arming, unlimited monitoring, and fail-closed reconciliation while that gate is parked - On GitHub, GitLab, Forgejo, and Azure DevOps, polls provider mergeability alongside CI checks while the PR remains open - On GitHub, combines the exact current PR head commit's check rollup with Actions workflow runs for that same commit, so a workflow rejected during validation before it creates a job or check-run still blocks readiness +- On GitHub, if that check rollup itself cannot be read with the configured credential - the Checks API answers a permission failure, which some fine-grained tokens receive while still being allowed to read Actions - derives evidence from the Actions REST API for the same repository, pull request, and published head commit instead of producing nothing. That fallback reports failing and still-running jobs as usual, but certifies the commit as green only when the base branch's required-check definition is readable and every required check maps to exactly one Actions result from the current head's latest attempt. A run on another commit, a job from a superseded attempt, an incomplete listing, a required check with no unique Actions result, an unprotected base branch, and an unreadable required-check definition all stay unavailable evidence, never green +- If consecutive polls produce no CI evidence at all - the rollup is unreadable and the fallback cannot complete either - pauses for user approval after a small number of attempts with a finding naming the last provider failure, rather than repeating the same warning until the idle timeout expires. A successful read resets that budget, so ordinary transient failures are absorbed - While the PR stays open, the TUI and terminal title show `Checks passed` once CI readiness is established and known mergeability is clear, and `no-mistakes axi` returns `outcome: checks-passed` with successful-output reporting instructions so agents can summarize the run, ask the user to review and merge, and list any pipeline fixes instead of waiting - An empty forge check list is never treated as green unless the trusted default-branch config declares [`no_ci: true`](/no-mistakes/reference/repo-config/#no_ci). That declaration is positive durable evidence the repository intentionally has no CI; absence means CI is expected and delayed registration stays not-ready. If checks still appear on a declared no-CI repo, their actual states are honored - If the [PR base branch](/no-mistakes/reference/repo-config/#prbase_branch) moves after `checks-passed`, keeps watching the same PR; a clean behind PR needs no action, while an actual GitHub, GitLab, Forgejo, or Azure DevOps merge conflict is auto-fixed by rebasing onto the PR base branch and re-pushing through the force-push safety guard diff --git a/internal/pipeline/steps/ci.go b/internal/pipeline/steps/ci.go index 3b03cb8c7..5431eef28 100644 --- a/internal/pipeline/steps/ci.go +++ b/internal/pipeline/steps/ci.go @@ -17,6 +17,16 @@ import ( const ( defaultBaseBranchTipResolveWindow = 30 * time.Second defaultPublishedHeadResolveWindow = 30 * time.Second + // maxConsecutiveCheckReadFailures bounds how many polls in a row may end + // with no CI evidence at all before the step parks for a decision. + // A read that keeps failing is not the same as CI that keeps running: it + // produces nothing the user or an agent can act on, and letting it repeat + // until the idle timeout turns a credential or provider problem into an + // hours-long silent stall whose only artifact is a repeated warning. The + // budget is generous enough that an ordinary transient failure - a rate + // limit, a dropped connection, a provider blip - is absorbed by the next + // successful poll, which resets it. + maxConsecutiveCheckReadFailures = 5 ) // CI monitoring status messages. These are surfaced to the user and parsed by @@ -244,6 +254,10 @@ func (s *CIStep) Execute(sctx *pipeline.StepContext) (*pipeline.StepOutcome, err timeoutFailingChecks := []string{} timeoutMergeConflict := false lastMonitorLog := "" + // checkReadFailures counts consecutive polls that produced no CI evidence. + // A successful read resets it, so only an uninterrupted run of failures can + // reach the bound. + checkReadFailures := 0 timeoutOutcome := func() (*pipeline.StepOutcome, error) { sctx.Log("CI timeout reached") if len(timeoutFailingChecks) > 0 || timeoutMergeConflict { @@ -347,8 +361,13 @@ func (s *CIStep) Execute(sctx *pipeline.StepContext) (*pipeline.StepOutcome, err if err != nil { clearCIMonitorReady(sctx) lastMonitorLog = "" - sctx.Log(fmt.Sprintf("warning: could not check CI: %v", err)) + checkReadFailures++ + sctx.Log(fmt.Sprintf("warning: could not check CI (attempt %d/%d): %v", checkReadFailures, maxConsecutiveCheckReadFailures, err)) + if checkReadFailures >= maxConsecutiveCheckReadFailures { + return ciEvidenceUnavailableOutcome(err, checkReadFailures), nil + } } else { + checkReadFailures = 0 // checksPending is the narrow execution state: only checks that are // actively running or queued block a rerun or issue escalation. A // provider-cancelled check is terminal enough to enter the transient diff --git a/internal/pipeline/steps/ci_checks.go b/internal/pipeline/steps/ci_checks.go index 35a225a0d..c3065f517 100644 --- a/internal/pipeline/steps/ci_checks.go +++ b/internal/pipeline/steps/ci_checks.go @@ -217,6 +217,31 @@ func ciMergeabilityOutcome(summary, description string) *pipeline.StepOutcome { } } +// ciEvidenceUnavailableOutcome parks the run when consecutive polls produced no +// CI evidence at all. It is deliberately not a failure verdict on the code: the +// step never learned whether the checks passed, so the finding says exactly +// that and asks for a decision, instead of leaving the run to expire against +// its idle timeout with nothing but repeated warnings in the log. +func ciEvidenceUnavailableOutcome(err error, attempts int) *pipeline.StepOutcome { + reason := "the provider reported no reason" + if err != nil { + reason = err.Error() + } + findings := Findings{ + Summary: fmt.Sprintf("CI evidence could not be read on %d consecutive attempts", attempts), + Items: []Finding{{ + Severity: "warning", + Description: fmt.Sprintf("CI check evidence is unavailable, so this run cannot tell whether the checks passed: %s", reason), + Action: types.ActionAskUser, + }}, + } + findingsJSON, _ := json.Marshal(findings) + return &pipeline.StepOutcome{ + NeedsApproval: true, + Findings: string(findingsJSON), + } +} + func ciMonitoringTimeoutOutcome() *pipeline.StepOutcome { findings := Findings{ Summary: "CI monitoring timed out before PR was merged or closed", diff --git a/internal/pipeline/steps/ci_test.go b/internal/pipeline/steps/ci_test.go index 4af1df430..a06d52e92 100644 --- a/internal/pipeline/steps/ci_test.go +++ b/internal/pipeline/steps/ci_test.go @@ -2672,3 +2672,108 @@ func TestCIStep_DelayedSameNameCheckRetainsLegacyNameBehavior(t *testing.T) { t.Fatal("new conclusive link did not retire the rerun record") } } + +// A check read that keeps failing produces no evidence at all, so the step must +// stop at a bounded, actionable outcome instead of repeating the same warning +// until the idle timeout expires. +func TestCIStep_RepeatedEvidenceFailuresParkForADecision(t *testing.T) { + t.Parallel() + dir, baseSHA, headSHA := setupGitRepo(t) + + env := fakeCIGHChecksError(t, "OPEN", "MERGEABLE", "gh: Resource not accessible by personal access token (HTTP 403)") + + prURL := "https://github.com/test/repo/pull/42" + ag := &mockAgent{name: "test"} + sctx := newTestContextWithDBRecords(t, ag, dir, baseSHA, headSHA, config.Commands{}) + sctx.Env = env + sctx.Run.PRURL = &prURL + sctx.Config.CITimeout = 24 * time.Hour + + var logs []string + sctx.Log = func(s string) { logs = append(logs, s) } + + current := time.Date(2026, time.January, 1, 12, 0, 0, 0, time.UTC) + polls := 0 + step := &CIStep{ + pollIntervalOverride: 30 * time.Second, + now: func() time.Time { return current }, + waitForNextPoll: func(ctx context.Context, interval time.Duration) error { + polls++ + if polls > maxConsecutiveCheckReadFailures { + t.Fatalf("step polled %d times without parking, logs: %v", polls, logs) + } + current = current.Add(interval) + return nil + }, + } + + outcome, err := step.Execute(sctx) + if err != nil { + t.Fatalf("Execute() error = %v", err) + } + if outcome == nil || !outcome.NeedsApproval { + t.Fatalf("outcome = %+v, want a parked decision", outcome) + } + var findings Findings + if err := json.Unmarshal([]byte(outcome.Findings), &findings); err != nil { + t.Fatalf("unmarshal findings: %v", err) + } + if !strings.Contains(findings.Summary, "could not be read") { + t.Fatalf("findings.Summary = %q, want the unavailable evidence named", findings.Summary) + } + if len(findings.Items) != 1 || findings.Items[0].Action != types.ActionAskUser { + t.Fatalf("findings.Items = %+v, want one ask-user finding", findings.Items) + } + if !strings.Contains(findings.Items[0].Description, "unavailable") { + t.Fatalf("finding = %q, want it to say the evidence was unavailable", findings.Items[0].Description) + } +} + +// A poll that succeeds resets the budget, so intermittent read failures never +// accumulate into a park while CI is still reporting. +func TestCIStep_SuccessfulReadResetsTheEvidenceFailureBudget(t *testing.T) { + t.Parallel() + dir, baseSHA, headSHA := setupGitRepo(t) + + // Alternate: a failed read, then a pending check, repeatedly. + checks := []string{} + for i := 0; i < maxConsecutiveCheckReadFailures*3; i++ { + if i%2 == 0 { + checks = append(checks, fakeChecksReadFailure) + } else { + checks = append(checks, `[{"name":"build","state":"IN_PROGRESS","bucket":"pending"}]`) + } + } + env := fakeCIGHSequence(t, "OPEN", checks) + + prURL := "https://github.com/test/repo/pull/42" + ag := &mockAgent{name: "test"} + sctx := newTestContextWithDBRecords(t, ag, dir, baseSHA, headSHA, config.Commands{}) + sctx.Env = env + sctx.Run.PRURL = &prURL + sctx.Config.CITimeout = 24 * time.Hour + + current := time.Date(2026, time.January, 1, 12, 0, 0, 0, time.UTC) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + sctx.Ctx = ctx + + polls := 0 + step := &CIStep{ + pollIntervalOverride: 30 * time.Second, + now: func() time.Time { return current }, + waitForNextPoll: func(ctx context.Context, interval time.Duration) error { + polls++ + current = current.Add(interval) + if polls >= maxConsecutiveCheckReadFailures*2 { + cancel() + return ctx.Err() + } + return nil + }, + } + + if _, err := step.Execute(sctx); !errors.Is(err, context.Canceled) { + t.Fatalf("Execute() error = %v, want the monitor still polling", err) + } +} diff --git a/internal/pipeline/steps/steps_test.go b/internal/pipeline/steps/steps_test.go index 08bd3463f..9ea3bd576 100644 --- a/internal/pipeline/steps/steps_test.go +++ b/internal/pipeline/steps/steps_test.go @@ -477,6 +477,10 @@ func fakeCIGHRerun() { os.Exit(0) } +// fakeChecksReadFailure is the sequence entry that makes the fake gh answer a +// check read with a provider failure instead of a check list. +const fakeChecksReadFailure = "ERROR" + func fakeCIGHSequenceHandler(args []string) { state := os.Getenv("FAKE_CLI_STATE") checksPath := os.Getenv("FAKE_CLI_CHECKS_PATH") @@ -532,6 +536,10 @@ func fakeCIGHSequenceHandler(args []string) { fmt.Fprintln(os.Stderr, err) os.Exit(1) } + if entry := entries[index]; entry == fakeChecksReadFailure { + fmt.Fprintln(os.Stderr, "gh: Resource not accessible by personal access token (HTTP 403)") + os.Exit(1) + } fmt.Println(entries[index]) os.Exit(0) } @@ -559,6 +567,12 @@ func fakeCIGHSequenceHandler(args []string) { fmt.Fprintln(os.Stderr, err) os.Exit(1) } + if entry := entries[index]; entry == fakeChecksReadFailure { + // A sequence entry can ask for a failed read, so a test can drive + // the monitor's bounded no-evidence handling. + fmt.Fprintln(os.Stderr, "gh: Resource not accessible by personal access token (HTTP 403)") + os.Exit(1) + } printFakeCommitChecks(entries[index], args) os.Exit(0) } diff --git a/internal/scm/github/actions_fallback.go b/internal/scm/github/actions_fallback.go new file mode 100644 index 000000000..76ec573ce --- /dev/null +++ b/internal/scm/github/actions_fallback.go @@ -0,0 +1,518 @@ +package github + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "sort" + "strings" + "time" + + "github.com/kunchenguid/no-mistakes/internal/scm" +) + +// GitHub check evidence has two independent readers behind it. The primary one +// is the GraphQL `statusCheckRollup` on the head commit (what `gh pr checks` +// reads), which is served by the Checks API. Some credentials - notably +// fine-grained tokens - are refused that context with a 403 while still being +// allowed to read the repository's GitHub Actions workflow runs and jobs for +// the same commit. Before this fallback existed a run in that state produced no +// evidence at all: GetChecks returned the read error on every poll, the CI step +// logged a warning, and the run sat there until its idle timeout with nothing +// actionable to show for it. +// +// The fallback is deliberately narrow. The rollup stays the primary source and +// is never second-guessed when it answers. Actions evidence is derived only +// when the primary failure classifies as the rollup itself being unreadable, +// and it is bound to the exact repository, PR, and currently published head. +// +// Nothing here may turn doubt into a pass. Actions can see workflow jobs; it +// cannot see check runs published by other GitHub Apps, so a set of green +// Actions jobs alone is not proof that the commit's whole check set is green. +// That gap is closed from the other side: green is certified only when the +// branch's required-check definition is readable and every required identity +// has exactly one exact-current-head Actions mapping. Anything short of that - +// an unreadable required set, a required identity with no mapping or more than +// one, a run on another commit, a job from a superseded attempt, an incomplete +// listing - is reported as unavailable evidence, never as green. Non-green +// evidence (a failing or still-running job) needs no such certification and is +// returned as-is, because it can only make the pipeline wait or escalate. +var ( + // ErrRollupUnavailable marks a primary check read that failed because the + // GraphQL status-check rollup / Checks API could not be read with this + // credential, as opposed to a general command failure. It is the only + // classification that opens the Actions fallback. + ErrRollupUnavailable = fmt.Errorf("GitHub status check rollup is unreadable: %w", scm.ErrChecksUnavailable) + // ErrActionsEvidenceMissing marks Actions evidence that does not exist or + // cannot be completed: no run for the head commit, a run whose jobs cannot + // be enumerated, an unreadable required-check definition, or a required + // check identity with no Actions mapping at the current head. + ErrActionsEvidenceMissing = fmt.Errorf("GitHub Actions evidence is missing: %w", scm.ErrChecksUnavailable) + // ErrActionsEvidenceAmbiguous marks Actions evidence that exists more than + // once for one check identity at the current head, so no single result can + // be attributed to it. + ErrActionsEvidenceAmbiguous = fmt.Errorf("GitHub Actions evidence is ambiguous: %w", scm.ErrChecksUnavailable) + // ErrActionsHeadMismatch marks Actions evidence that belongs to a commit + // other than the head the run is certifying, including a job left behind by + // a superseded run attempt. + ErrActionsHeadMismatch = fmt.Errorf("GitHub Actions evidence does not belong to the commit and attempt under test: %w: %w", scm.ErrHeadChanged, scm.ErrChecksUnavailable) + // ErrActionsAPIFailure marks an Actions REST read that failed outright, so + // the fallback produced no evidence either. + ErrActionsAPIFailure = fmt.Errorf("GitHub Actions API read failed: %w", scm.ErrChecksUnavailable) +) + +// rollupUnavailableMarkers are the provider messages that mean "this credential +// may not read the check rollup", rather than "the read itself went wrong". +// They are matched case-insensitively against the combined gh output. The list +// is deliberately specific: a general failure (network, rate limit, malformed +// response) must keep the old behavior of surfacing the error, because the +// fallback's evidence is narrower than the rollup's and must not be reached on +// a failure that a retry would clear. +var rollupUnavailableMarkers = []string{ + "resource not accessible", + "insufficient scopes", + "not authorized to read", + "http 403", + "403 forbidden", + "statuscheckrollup", +} + +// rollupReadError wraps a primary check-read failure, tagging it with +// ErrRollupUnavailable when the provider's own output says the rollup is not +// readable with this credential. +func rollupReadError(stage, output string, err error) error { + trimmed := strings.TrimSpace(output) + if classifyRollupUnavailable(trimmed, err) { + return fmt.Errorf("%s: %s: %w: %w", stage, trimmed, err, ErrRollupUnavailable) + } + if trimmed == "" { + return fmt.Errorf("%s: %w", stage, err) + } + return fmt.Errorf("%s: %s: %w", stage, trimmed, err) +} + +func classifyRollupUnavailable(output string, err error) bool { + haystack := strings.ToLower(output) + if err != nil { + haystack += "\n" + strings.ToLower(err.Error()) + } + for _, marker := range rollupUnavailableMarkers { + if strings.Contains(haystack, marker) { + return true + } + } + return false +} + +// workflowJob is the subset of an Actions job this backend reads. run_id, +// run_attempt, and head_sha are identity, not decoration: they are what proves +// a job belongs to the run, attempt, and commit under test. +type workflowJob struct { + ID int64 `json:"id"` + RunID int64 `json:"run_id"` + RunAttempt int `json:"run_attempt"` + HeadSHA string `json:"head_sha"` + Name string `json:"name"` + Status string `json:"status"` + Conclusion string `json:"conclusion"` + CompletedAt string `json:"completed_at"` + HTMLURL string `json:"html_url"` +} + +// getActionsFallbackChecks derives check evidence for headSHA from the Actions +// REST API. It is only reached when the primary rollup read classified as +// unreadable. Every failure mode returns an error wrapping +// scm.ErrChecksUnavailable so the caller can tell "no evidence" from "evidence +// that is not green". +func (h *Host) getActionsFallbackChecks(ctx context.Context, pr *scm.PR, headSHA string) ([]scm.Check, error) { + headSHA = strings.TrimSpace(headSHA) + if headSHA == "" { + return nil, fmt.Errorf("no head commit to bind Actions evidence to: %w", ErrActionsEvidenceMissing) + } + repo := h.repoSlug() + if repo == "" { + return nil, fmt.Errorf("no repository to bind Actions evidence to: %w", ErrActionsEvidenceMissing) + } + + runs, err := h.listWorkflowRunsForHead(ctx, headSHA) + if err != nil { + return nil, fmt.Errorf("%w: %w", err, ErrActionsAPIFailure) + } + if len(runs) == 0 { + return nil, fmt.Errorf("no GitHub Actions run for commit %s: %w", headSHA, ErrActionsEvidenceMissing) + } + + var checks []scm.Check + for _, run := range runs { + if !sameCommit(run.HeadSHA, headSHA) { + return nil, fmt.Errorf("workflow run %d names commit %q, want %s: %w", run.ID, strings.TrimSpace(run.HeadSHA), headSHA, ErrActionsHeadMismatch) + } + if run.RunAttempt <= 0 { + return nil, fmt.Errorf("workflow run %d reports no run attempt: %w", run.ID, ErrActionsEvidenceMissing) + } + jobs, err := h.listWorkflowRunJobs(ctx, run.ID) + if err != nil { + return nil, fmt.Errorf("%w: %w", err, ErrActionsAPIFailure) + } + runChecks, err := h.checksForRun(run, jobs, headSHA, repo) + if err != nil { + return nil, err + } + checks = append(checks, runChecks...) + } + if len(checks) == 0 { + return nil, fmt.Errorf("no GitHub Actions result for commit %s: %w", headSHA, ErrActionsEvidenceMissing) + } + + // Evidence that is not green needs no certification: it can only make the + // CI step wait or escalate, never merge. Only a would-be pass has to prove + // it accounts for the branch's whole required-check set. + if !actionsEvidenceWouldCertify(checks) { + return checks, nil + } + if err := h.assertRequiredChecksMapped(ctx, pr, checks); err != nil { + return nil, err + } + return checks, nil +} + +// checksForRun turns one workflow run's jobs into checks, refusing anything +// that cannot be attributed to the exact commit and attempt under test. +func (h *Host) checksForRun(run workflowRun, jobs []workflowJob, headSHA, repo string) ([]scm.Check, error) { + if len(jobs) == 0 { + return h.checksForJoblessRun(run, repo) + } + checks := make([]scm.Check, 0, len(jobs)) + for _, job := range jobs { + if job.RunID != 0 && job.RunID != run.ID { + return nil, fmt.Errorf("job %d belongs to workflow run %d, want %d: %w", job.ID, job.RunID, run.ID, ErrActionsEvidenceAmbiguous) + } + if job.RunAttempt <= 0 { + return nil, fmt.Errorf("job %d of workflow run %d reports no run attempt: %w", job.ID, run.ID, ErrActionsEvidenceMissing) + } + // The listing asks for the latest attempt only. A job from any other + // attempt is a superseded rerun's leftover; treating it as current + // would let an old green attempt certify a commit whose current + // attempt has not finished (or has failed). + if job.RunAttempt != run.RunAttempt { + return nil, fmt.Errorf("job %d of workflow run %d is from attempt %d, want %d: %w", job.ID, run.ID, job.RunAttempt, run.RunAttempt, ErrActionsHeadMismatch) + } + if strings.TrimSpace(job.HeadSHA) != "" && !sameCommit(job.HeadSHA, headSHA) { + return nil, fmt.Errorf("job %d of workflow run %d names commit %q, want %s: %w", job.ID, run.ID, strings.TrimSpace(job.HeadSHA), headSHA, ErrActionsHeadMismatch) + } + name := strings.TrimSpace(job.Name) + if name == "" { + // A nameless job has no check identity, so it can neither be + // matched against a required check nor reported to the user. + return nil, fmt.Errorf("job %d of workflow run %d has no name: %w", job.ID, run.ID, ErrActionsEvidenceMissing) + } + checks = append(checks, scm.Check{ + Name: name, + Bucket: jobBucket(job), + State: jobState(job), + CompletedAt: parseActionsTime(job.CompletedAt), + Link: h.jobLink(job, run, repo), + }) + } + return checks, nil +} + +// checksForJoblessRun handles a run the jobs endpoint reports nothing for. A +// run that has not started yet legitimately has no jobs, and so does a run the +// provider concluded without running anything (a path filter skipping the whole +// workflow). A run that claims to have succeeded with no readable job is not +// evidence of anything and must not be able to certify the commit. +func (h *Host) checksForJoblessRun(run workflowRun, repo string) ([]scm.Check, error) { + bucket := normalizeCheckBucket("", run.Conclusion) + if bucket == "" { + bucket = normalizeCheckBucket("", run.Status) + } + if !strings.EqualFold(strings.TrimSpace(run.Status), "completed") { + // Still queued or running: no jobs yet is the normal state, and the + // run itself is the pending evidence. An unrecognized non-terminal + // status stays pending too - unknown is never green. + bucket = scm.CheckBucketPending + } else if bucket == "" || bucket == scm.CheckBucketPass { + return nil, fmt.Errorf("workflow run %d concluded %q with no readable job: %w", run.ID, strings.TrimSpace(run.Conclusion), ErrActionsEvidenceMissing) + } + return []scm.Check{{ + Name: runCheckName(run), + Bucket: bucket, + State: runState(run), + CompletedAt: parseActionsTime(run.UpdatedAt), + Link: h.runLink(run, repo), + }}, nil +} + +// listWorkflowRunJobs returns the latest attempt's jobs for one workflow run. +// Like the run listing, the pagination is validated rather than trusted: a +// short listing would silently drop a failing job, which is the one way this +// fallback could manufacture a pass. +func (h *Host) listWorkflowRunJobs(ctx context.Context, runID int64) ([]workflowJob, error) { + repo := h.repoSlug() + endpoint := fmt.Sprintf("repos/{owner}/{repo}/actions/runs/%d/jobs", runID) + if repo != "" { + endpoint = fmt.Sprintf("repos/%s/actions/runs/%d/jobs", repo, runID) + } + args := []string{"api"} + if h.host != "" { + args = append(args, "--hostname", h.host) + } + args = append(args, "--method", "GET", endpoint, + "-f", "filter=latest", + "-f", "per_page=100", + "--paginate", "--slurp", + ) + out, err := h.cmd(ctx, "gh", args...).CombinedOutput() + if err != nil { + return nil, fmt.Errorf("gh api jobs for workflow run %d: %s: %w", runID, strings.TrimSpace(string(out)), err) + } + var pages []struct { + TotalCount *int `json:"total_count"` + Jobs []workflowJob `json:"jobs"` + } + if err := json.Unmarshal(out, &pages); err != nil { + return nil, fmt.Errorf("parse jobs for workflow run %d: %w", runID, err) + } + if len(pages) == 0 { + return nil, fmt.Errorf("job discovery for workflow run %d returned no pages", runID) + } + var jobs []workflowJob + totalCount := -1 + jobIDs := make(map[int64]struct{}) + for pageIndex, page := range pages { + if page.TotalCount == nil || *page.TotalCount < 0 { + return nil, fmt.Errorf("job page %d of workflow run %d has no valid total_count", pageIndex+1, runID) + } + if totalCount == -1 { + totalCount = *page.TotalCount + } else if *page.TotalCount != totalCount { + return nil, fmt.Errorf("job page %d of workflow run %d total_count is %d, want %d", pageIndex+1, runID, *page.TotalCount, totalCount) + } + for _, job := range page.Jobs { + if job.ID == 0 { + return nil, fmt.Errorf("job page %d of workflow run %d contains a job without an id", pageIndex+1, runID) + } + if _, exists := jobIDs[job.ID]; exists { + return nil, fmt.Errorf("job id %d of workflow run %d appears more than once", job.ID, runID) + } + jobIDs[job.ID] = struct{}{} + jobs = append(jobs, job) + } + } + if len(jobIDs) != totalCount { + return nil, fmt.Errorf("job discovery for workflow run %d returned %d unique jobs, want %d", runID, len(jobIDs), totalCount) + } + return jobs, nil +} + +// assertRequiredChecksMapped is the gate between "every Actions job we could +// see is green" and "this commit is green". Actions cannot see check runs +// published by other GitHub Apps, so the only thing that makes the Actions view +// sufficient is the branch's own required-check definition: when every required +// identity maps to exactly one Actions result at the current head, no unseen +// check run can be required. An unreadable definition, an unprotected branch, +// or a required identity with no unique mapping all leave that gap open, and +// the evidence stays unavailable. +func (h *Host) assertRequiredChecksMapped(ctx context.Context, pr *scm.PR, checks []scm.Check) error { + baseBranch, err := h.checksBaseBranch(ctx, pr) + if err != nil { + return fmt.Errorf("%w: %w", err, ErrActionsEvidenceMissing) + } + required, err := h.requiredCheckContexts(ctx, baseBranch) + if err != nil { + return fmt.Errorf("%w: %w", err, ErrActionsEvidenceMissing) + } + if len(required) == 0 { + return fmt.Errorf("branch %q declares no required check for commit certification: %w", baseBranch, ErrActionsEvidenceMissing) + } + seen := make(map[string]int, len(checks)) + for _, check := range checks { + seen[check.Name]++ + } + for _, context := range required { + switch seen[context] { + case 1: + case 0: + return fmt.Errorf("required check %q has no GitHub Actions result at this commit: %w", context, ErrActionsEvidenceMissing) + default: + return fmt.Errorf("required check %q maps to %d GitHub Actions results at this commit: %w", context, seen[context], ErrActionsEvidenceAmbiguous) + } + } + return nil +} + +// checksBaseBranch resolves the branch whose protection defines the required +// check set. The PR's own recorded base wins; otherwise it is read from the +// forge, which binds the definition to the configured PR rather than to any +// ambient default. +func (h *Host) checksBaseBranch(ctx context.Context, pr *scm.PR) (string, error) { + if pr != nil { + if base := strings.TrimSpace(pr.BaseBranch); base != "" { + return base, nil + } + } + base, err := h.GetPRBaseBranch(ctx, pr) + if err != nil { + return "", err + } + base = strings.TrimSpace(base) + if base == "" { + return "", errors.New("gh pr view returned an empty base branch") + } + return base, nil +} + +// requiredCheckContexts returns the check identities branch protection requires +// on branch, sorted for deterministic reporting. A branch with no protection +// (or no required checks) returns an empty set, which the caller treats as +// insufficient rather than as permission to certify. +func (h *Host) requiredCheckContexts(ctx context.Context, branch string) ([]string, error) { + repo := h.repoSlug() + if repo == "" { + return nil, errors.New("no repository to read required checks from") + } + endpoint := fmt.Sprintf("repos/%s/branches/%s/protection/required_status_checks", repo, branch) + args := []string{"api"} + if h.host != "" { + args = append(args, "--hostname", h.host) + } + args = append(args, "--method", "GET", endpoint) + out, err := h.cmd(ctx, "gh", args...).CombinedOutput() + if err != nil { + body := strings.TrimSpace(string(out)) + if isBranchUnprotected(body) { + return nil, nil + } + return nil, fmt.Errorf("gh api required checks for branch %q: %s: %w", branch, body, err) + } + var payload struct { + Contexts []string `json:"contexts"` + Checks []struct { + Context string `json:"context"` + } `json:"checks"` + } + if err := json.Unmarshal(out, &payload); err != nil { + return nil, fmt.Errorf("parse required checks for branch %q: %w", branch, err) + } + unique := make(map[string]struct{}, len(payload.Contexts)+len(payload.Checks)) + for _, context := range payload.Contexts { + if trimmed := strings.TrimSpace(context); trimmed != "" { + unique[trimmed] = struct{}{} + } + } + for _, check := range payload.Checks { + if trimmed := strings.TrimSpace(check.Context); trimmed != "" { + unique[trimmed] = struct{}{} + } + } + contexts := make([]string, 0, len(unique)) + for context := range unique { + contexts = append(contexts, context) + } + sort.Strings(contexts) + return contexts, nil +} + +// isBranchUnprotected reports whether GitHub answered the required-checks read +// with its definitive "there is no such protection" 404 rather than with a +// failure. Both answers keep the fallback from certifying, but only the +// definitive one must not be reported as an API failure. +func isBranchUnprotected(body string) bool { + lowered := strings.ToLower(body) + return strings.Contains(lowered, "branch not protected") || + strings.Contains(lowered, "required status checks not enabled") || + strings.Contains(lowered, "http 404") +} + +// actionsEvidenceWouldCertify reports whether this evidence set, taken at face +// value, would let the CI step call the commit green. +func actionsEvidenceWouldCertify(checks []scm.Check) bool { + if len(checks) == 0 { + return false + } + for _, check := range checks { + if check.Bucket != scm.CheckBucketPass && check.Bucket != scm.CheckBucketSkip { + return false + } + } + return true +} + +func jobBucket(job workflowJob) scm.CheckBucket { + if !strings.EqualFold(strings.TrimSpace(job.Status), "completed") { + // Queued, in progress, waiting, or a status this version does not + // recognize: none of them is a conclusion, so none of them is green. + return scm.CheckBucketPending + } + if bucket := normalizeCheckBucket("", job.Conclusion); bucket != "" { + return bucket + } + // A completed job whose conclusion this version cannot classify is not + // known to have succeeded. Keep it out of the pass bucket. + return scm.CheckBucketPending +} + +func jobState(job workflowJob) string { + if state := strings.ToUpper(strings.TrimSpace(job.Conclusion)); state != "" { + return state + } + return strings.ToUpper(strings.TrimSpace(job.Status)) +} + +func runState(run workflowRun) string { + if state := strings.ToUpper(strings.TrimSpace(run.Conclusion)); state != "" { + return state + } + return strings.ToUpper(strings.TrimSpace(run.Status)) +} + +func runCheckName(run workflowRun) string { + if name := strings.TrimSpace(run.Name); name != "" { + return name + } + if name := strings.TrimSpace(run.DisplayName); name != "" { + return name + } + return "GitHub Actions workflow" +} + +func (h *Host) jobLink(job workflowJob, run workflowRun, repo string) string { + if link := strings.TrimSpace(job.HTMLURL); link != "" { + return link + } + return fmt.Sprintf("https://%s/%s/actions/runs/%d/job/%d", h.linkHost(), repo, run.ID, job.ID) +} + +func (h *Host) runLink(run workflowRun, repo string) string { + if link := strings.TrimSpace(run.HTMLURL); link != "" { + return link + } + return fmt.Sprintf("https://%s/%s/actions/runs/%d", h.linkHost(), repo, run.ID) +} + +func (h *Host) linkHost() string { + if host := strings.TrimSpace(h.host); host != "" { + return host + } + return "github.com" +} + +func parseActionsTime(raw string) time.Time { + if strings.TrimSpace(raw) == "" { + return time.Time{} + } + parsed, err := time.Parse(time.RFC3339, raw) + if err != nil { + return time.Time{} + } + return parsed +} + +// sameCommit compares two commit identifiers. GitHub always answers with full +// 40-character object names here, so this is an exact comparison; it is +// case-insensitive only because hex case is not part of a commit's identity. +func sameCommit(a, b string) bool { + return strings.EqualFold(strings.TrimSpace(a), strings.TrimSpace(b)) +} diff --git a/internal/scm/github/actions_fallback_test.go b/internal/scm/github/actions_fallback_test.go new file mode 100644 index 000000000..39fb9fea0 --- /dev/null +++ b/internal/scm/github/actions_fallback_test.go @@ -0,0 +1,420 @@ +package github + +import ( + "context" + "errors" + "fmt" + "strings" + "testing" + + "github.com/kunchenguid/no-mistakes/internal/scm" +) + +const ( + fallbackRepo = "test/repo" + fallbackHead = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef" + fallbackOther = "0000000000000000000000000000000000000000" + fallbackPRView = "gh pr view 123 --repo test/repo --json headRefOid --jq .headRefOid" + fallbackBaseCmd = "gh pr view 123 --repo test/repo --json baseRefName --jq .baseRefName" +) + +func fallbackRunsCmd(headSHA string) string { + return "gh api --method GET repos/test/repo/actions/runs -f head_sha=" + headSHA + " -f per_page=100 --paginate --slurp" +} + +func fallbackJobsCmd(runID int64) string { + return fmt.Sprintf("gh api --method GET repos/test/repo/actions/runs/%d/jobs -f filter=latest -f per_page=100 --paginate --slurp", runID) +} + +func fallbackRequiredCmd(branch string) string { + return "gh api --method GET repos/test/repo/branches/" + branch + "/protection/required_status_checks" +} + +func slurped(pages ...string) githubTestResponse { + return githubTestResponse{stdout: "[" + strings.Join(pages, ",") + "]\n"} +} + +// rollupForbidden is what GitHub answers when the credential may read the +// repository but not the Checks API context behind the status rollup. +func rollupForbidden() githubTestResponse { + return githubTestResponse{ + stderr: "gh: Resource not accessible by personal access token (HTTP 403)", + code: 1, + } +} + +// fallbackResponses is the command set a healthy fallback poll needs: the PR +// head read, a rollup that refuses, one workflow run at that head, its jobs, +// the PR base branch, and that branch's required checks. +func fallbackResponses(runsPage, jobsPage, requiredChecks string) map[string]githubTestResponse { + return map[string]githubTestResponse{ + fallbackPRView: {stdout: fallbackHead + "\n"}, + githubCommitChecksCommand("", fallbackRepo, fallbackHead): rollupForbidden(), + fallbackRunsCmd(fallbackHead): slurped(runsPage), + fallbackJobsCmd(101): slurped(jobsPage), + fallbackBaseCmd: {stdout: "main\n"}, + fallbackRequiredCmd("main"): {stdout: requiredChecks}, + } +} + +func fallbackPR() *scm.PR { + return &scm.PR{Number: "123", HeadSHA: fallbackHead} +} + +const ( + oneGreenRunPage = `{"total_count":1,"workflow_runs":[{"id":101,"name":"CI","status":"completed","conclusion":"success","head_sha":"` + fallbackHead + `","run_attempt":1,"workflow_id":7,"event":"pull_request","html_url":"https://github.com/test/repo/actions/runs/101"}]}` + oneGreenJobPage = `{"total_count":1,"jobs":[{"id":201,"run_id":101,"run_attempt":1,"head_sha":"` + fallbackHead + `","name":"build","status":"completed","conclusion":"success","completed_at":"2026-08-21T10:00:00Z","html_url":"https://github.com/test/repo/actions/runs/101/job/201"}]}` + buildRequired = `{"strict":true,"contexts":["build"],"checks":[{"context":"build","app_id":15368}]}` +) + +// A rollup failure that is not capability evidence must keep the old behavior: +// surface the error, never derive narrower Actions evidence from it. No Actions +// command is registered here, so reaching the fallback would fail differently. +func TestGetChecksRollupFailureWithoutCapabilityEvidenceDoesNotFallBack(t *testing.T) { + t.Parallel() + + host := New(githubTestCmdFactory(map[string]githubTestResponse{ + fallbackPRView: {stdout: fallbackHead + "\n"}, + githubCommitChecksCommand("", fallbackRepo, fallbackHead): { + stderr: "gh: Something went wrong (HTTP 502)", + code: 1, + }, + }), nil, "", fallbackRepo) + + _, err := host.GetChecks(context.Background(), fallbackPR()) + if err == nil { + t.Fatal("GetChecks() error = nil, want the rollup read failure") + } + if errors.Is(err, ErrRollupUnavailable) { + t.Fatalf("GetChecks() error = %v, want it not classified as rollup-unavailable", err) + } + if !strings.Contains(err.Error(), "502") { + t.Fatalf("GetChecks() error = %v, want the provider failure preserved", err) + } +} + +func TestGetChecksFallsBackToActionsJobsAtExactHead(t *testing.T) { + t.Parallel() + + host := New(githubTestCmdFactory(fallbackResponses(oneGreenRunPage, oneGreenJobPage, buildRequired)), nil, "", fallbackRepo) + + checks, err := host.GetChecks(context.Background(), fallbackPR()) + if err != nil { + t.Fatalf("GetChecks() error = %v", err) + } + if len(checks) != 1 { + t.Fatalf("checks = %+v, want one job-level check", checks) + } + check := checks[0] + if check.Name != "build" || check.Bucket != scm.CheckBucketPass || check.State != "SUCCESS" { + t.Fatalf("check = %+v, want a passing build job", check) + } + if check.Link != "https://github.com/test/repo/actions/runs/101/job/201" { + t.Fatalf("check.Link = %q, want the job link", check.Link) + } + if check.CompletedAt.IsZero() { + t.Fatal("check.CompletedAt is zero, want the job completion time") + } +} + +// A run still in flight is pending evidence, not unavailable evidence, and it +// needs no required-check certification: pending can never be read as green. +// The required-check commands are deliberately absent. +func TestGetChecksFallbackPendingJobStaysPending(t *testing.T) { + t.Parallel() + + host := New(githubTestCmdFactory(map[string]githubTestResponse{ + fallbackPRView: {stdout: fallbackHead + "\n"}, + githubCommitChecksCommand("", fallbackRepo, fallbackHead): rollupForbidden(), + fallbackRunsCmd(fallbackHead): slurped(`{"total_count":1,"workflow_runs":[{"id":101,"name":"CI","status":"in_progress","conclusion":"","head_sha":"` + fallbackHead + `","run_attempt":1,"workflow_id":7}]}`), + fallbackJobsCmd(101): slurped(`{"total_count":1,"jobs":[{"id":201,"run_id":101,"run_attempt":1,"head_sha":"` + fallbackHead + `","name":"build","status":"in_progress","conclusion":""}]}`), + }), nil, "", fallbackRepo) + + checks, err := host.GetChecks(context.Background(), fallbackPR()) + if err != nil { + t.Fatalf("GetChecks() error = %v", err) + } + if len(checks) != 1 || checks[0].Bucket != scm.CheckBucketPending { + t.Fatalf("checks = %+v, want a single pending check", checks) + } +} + +func TestGetChecksFallbackFailedJobIsNonGreen(t *testing.T) { + t.Parallel() + + host := New(githubTestCmdFactory(map[string]githubTestResponse{ + fallbackPRView: {stdout: fallbackHead + "\n"}, + githubCommitChecksCommand("", fallbackRepo, fallbackHead): rollupForbidden(), + fallbackRunsCmd(fallbackHead): slurped(`{"total_count":1,"workflow_runs":[{"id":101,"name":"CI","status":"completed","conclusion":"failure","head_sha":"` + fallbackHead + `","run_attempt":1,"workflow_id":7}]}`), + fallbackJobsCmd(101): slurped(`{"total_count":2,"jobs":[{"id":201,"run_id":101,"run_attempt":1,"head_sha":"` + fallbackHead + `","name":"build","status":"completed","conclusion":"success"},{"id":202,"run_id":101,"run_attempt":1,"head_sha":"` + fallbackHead + `","name":"test","status":"completed","conclusion":"failure"}]}`), + }), nil, "", fallbackRepo) + + checks, err := host.GetChecks(context.Background(), fallbackPR()) + if err != nil { + t.Fatalf("GetChecks() error = %v", err) + } + failing := 0 + for _, check := range checks { + if check.Failing() { + failing++ + } + } + if len(checks) != 2 || failing != 1 { + t.Fatalf("checks = %+v, want two checks with one failing", checks) + } +} + +// Actions cannot see check runs published by other apps, so a required check +// with no Actions result at this head leaves the evidence incomplete. +func TestGetChecksFallbackRequiredCheckWithoutMappingStaysUnavailable(t *testing.T) { + t.Parallel() + + required := `{"strict":true,"contexts":["build","coverage/project"]}` + host := New(githubTestCmdFactory(fallbackResponses(oneGreenRunPage, oneGreenJobPage, required)), nil, "", fallbackRepo) + + _, err := host.GetChecks(context.Background(), fallbackPR()) + if !errors.Is(err, ErrActionsEvidenceMissing) || !errors.Is(err, scm.ErrChecksUnavailable) { + t.Fatalf("GetChecks() error = %v, want missing Actions evidence", err) + } + if !strings.Contains(err.Error(), "coverage/project") { + t.Fatalf("GetChecks() error = %v, want the unmapped required check named", err) + } +} + +// Two green Actions results for one required identity - the routine shape when +// a workflow runs for both the push and the pull_request event - must never be +// resolved into a pass. +func TestGetChecksFallbackAmbiguousMappingNeverPasses(t *testing.T) { + t.Parallel() + + runs := `{"total_count":2,"workflow_runs":[` + + `{"id":101,"name":"CI","status":"completed","conclusion":"success","head_sha":"` + fallbackHead + `","run_attempt":1,"workflow_id":7,"event":"pull_request"},` + + `{"id":102,"name":"CI","status":"completed","conclusion":"success","head_sha":"` + fallbackHead + `","run_attempt":1,"workflow_id":7,"event":"push"}]}` + responses := fallbackResponses(runs, oneGreenJobPage, buildRequired) + responses[fallbackJobsCmd(102)] = slurped(`{"total_count":1,"jobs":[{"id":202,"run_id":102,"run_attempt":1,"head_sha":"` + fallbackHead + `","name":"build","status":"completed","conclusion":"success"}]}`) + host := New(githubTestCmdFactory(responses), nil, "", fallbackRepo) + + _, err := host.GetChecks(context.Background(), fallbackPR()) + if !errors.Is(err, ErrActionsEvidenceAmbiguous) || !errors.Is(err, scm.ErrChecksUnavailable) { + t.Fatalf("GetChecks() error = %v, want ambiguous Actions evidence", err) + } +} + +func TestGetChecksFallbackRejectsRunOnAnotherCommit(t *testing.T) { + t.Parallel() + + runs := `{"total_count":1,"workflow_runs":[{"id":101,"name":"CI","status":"completed","conclusion":"success","head_sha":"` + fallbackOther + `","run_attempt":1,"workflow_id":7}]}` + host := New(githubTestCmdFactory(fallbackResponses(runs, oneGreenJobPage, buildRequired)), nil, "", fallbackRepo) + + _, err := host.GetChecks(context.Background(), fallbackPR()) + if !errors.Is(err, ErrActionsHeadMismatch) || !errors.Is(err, scm.ErrHeadChanged) { + t.Fatalf("GetChecks() error = %v, want a head mismatch", err) + } +} + +// A green job left behind by attempt 1 must not certify a commit whose current +// attempt is 2. +func TestGetChecksFallbackRejectsSupersededAttemptJob(t *testing.T) { + t.Parallel() + + runs := `{"total_count":1,"workflow_runs":[{"id":101,"name":"CI","status":"completed","conclusion":"success","head_sha":"` + fallbackHead + `","run_attempt":2,"workflow_id":7}]}` + host := New(githubTestCmdFactory(fallbackResponses(runs, oneGreenJobPage, buildRequired)), nil, "", fallbackRepo) + + _, err := host.GetChecks(context.Background(), fallbackPR()) + if !errors.Is(err, ErrActionsHeadMismatch) || !errors.Is(err, scm.ErrChecksUnavailable) { + t.Fatalf("GetChecks() error = %v, want the superseded attempt refused", err) + } + if !strings.Contains(err.Error(), "attempt 1") { + t.Fatalf("GetChecks() error = %v, want the stale attempt named", err) + } +} + +func TestGetChecksFallbackFollowsRunAndJobPagination(t *testing.T) { + t.Parallel() + + runPage1 := `{"total_count":2,"workflow_runs":[{"id":101,"name":"CI","status":"completed","conclusion":"success","head_sha":"` + fallbackHead + `","run_attempt":1,"workflow_id":7}]}` + runPage2 := `{"total_count":2,"workflow_runs":[{"id":102,"name":"Docs","status":"completed","conclusion":"success","head_sha":"` + fallbackHead + `","run_attempt":1,"workflow_id":8}]}` + jobPage1 := `{"total_count":2,"jobs":[{"id":201,"run_id":101,"run_attempt":1,"head_sha":"` + fallbackHead + `","name":"build","status":"completed","conclusion":"success"}]}` + jobPage2 := `{"total_count":2,"jobs":[{"id":202,"run_id":101,"run_attempt":1,"head_sha":"` + fallbackHead + `","name":"lint","status":"completed","conclusion":"success"}]}` + + responses := map[string]githubTestResponse{ + fallbackPRView: {stdout: fallbackHead + "\n"}, + githubCommitChecksCommand("", fallbackRepo, fallbackHead): rollupForbidden(), + fallbackRunsCmd(fallbackHead): slurped(runPage1, runPage2), + fallbackJobsCmd(101): slurped(jobPage1, jobPage2), + fallbackJobsCmd(102): slurped(`{"total_count":1,"jobs":[{"id":203,"run_id":102,"run_attempt":1,"head_sha":"` + fallbackHead + `","name":"docs","status":"completed","conclusion":"success"}]}`), + fallbackBaseCmd: {stdout: "main\n"}, + fallbackRequiredCmd("main"): {stdout: `{"strict":true,"contexts":["build","lint","docs"]}`}, + } + host := New(githubTestCmdFactory(responses), nil, "", fallbackRepo) + + checks, err := host.GetChecks(context.Background(), fallbackPR()) + if err != nil { + t.Fatalf("GetChecks() error = %v", err) + } + if len(checks) != 3 { + t.Fatalf("checks = %+v, want every job from every page", checks) + } +} + +// A jobs listing whose pages disagree with total_count is short, and a short +// listing is exactly how a failing job would disappear into a pass. +func TestGetChecksFallbackRejectsIncompleteJobPagination(t *testing.T) { + t.Parallel() + + jobs := `{"total_count":2,"jobs":[{"id":201,"run_id":101,"run_attempt":1,"head_sha":"` + fallbackHead + `","name":"build","status":"completed","conclusion":"success"}]}` + host := New(githubTestCmdFactory(fallbackResponses(oneGreenRunPage, jobs, buildRequired)), nil, "", fallbackRepo) + + _, err := host.GetChecks(context.Background(), fallbackPR()) + if !errors.Is(err, ErrActionsAPIFailure) || !errors.Is(err, scm.ErrChecksUnavailable) { + t.Fatalf("GetChecks() error = %v, want the short job listing refused", err) + } +} + +func TestGetChecksFallbackUnreadableRequiredChecksStaysUnavailable(t *testing.T) { + t.Parallel() + + responses := fallbackResponses(oneGreenRunPage, oneGreenJobPage, "") + responses[fallbackRequiredCmd("main")] = githubTestResponse{ + stderr: "gh: Must have admin rights to Repository. (HTTP 403)", + code: 1, + } + host := New(githubTestCmdFactory(responses), nil, "", fallbackRepo) + + _, err := host.GetChecks(context.Background(), fallbackPR()) + if !errors.Is(err, ErrActionsEvidenceMissing) || !errors.Is(err, scm.ErrChecksUnavailable) { + t.Fatalf("GetChecks() error = %v, want an unreadable required-check definition", err) + } +} + +// With no required-check definition nothing bounds the check runs Actions +// cannot see, so all-green Actions jobs still do not certify the commit. +func TestGetChecksFallbackUnprotectedBranchNeverCertifies(t *testing.T) { + t.Parallel() + + responses := fallbackResponses(oneGreenRunPage, oneGreenJobPage, "") + responses[fallbackRequiredCmd("main")] = githubTestResponse{ + stderr: "gh: Branch not protected (HTTP 404)", + code: 1, + } + host := New(githubTestCmdFactory(responses), nil, "", fallbackRepo) + + _, err := host.GetChecks(context.Background(), fallbackPR()) + if !errors.Is(err, ErrActionsEvidenceMissing) || !errors.Is(err, scm.ErrChecksUnavailable) { + t.Fatalf("GetChecks() error = %v, want no certification without required checks", err) + } +} + +func TestGetChecksFallbackActionsAPIFailureIsUnavailableEvidence(t *testing.T) { + t.Parallel() + + host := New(githubTestCmdFactory(map[string]githubTestResponse{ + fallbackPRView: {stdout: fallbackHead + "\n"}, + githubCommitChecksCommand("", fallbackRepo, fallbackHead): rollupForbidden(), + fallbackRunsCmd(fallbackHead): { + stderr: "gh: Resource not accessible by personal access token (HTTP 403)", + code: 1, + }, + }), nil, "", fallbackRepo) + + _, err := host.GetChecks(context.Background(), fallbackPR()) + if !errors.Is(err, ErrActionsAPIFailure) || !errors.Is(err, scm.ErrChecksUnavailable) { + t.Fatalf("GetChecks() error = %v, want an Actions API failure", err) + } + if !errors.Is(err, ErrRollupUnavailable) { + t.Fatalf("GetChecks() error = %v, want the primary failure preserved too", err) + } +} + +func TestGetChecksFallbackWithoutAnyRunIsMissingEvidence(t *testing.T) { + t.Parallel() + + host := New(githubTestCmdFactory(map[string]githubTestResponse{ + fallbackPRView: {stdout: fallbackHead + "\n"}, + githubCommitChecksCommand("", fallbackRepo, fallbackHead): rollupForbidden(), + fallbackRunsCmd(fallbackHead): slurped(`{"total_count":0,"workflow_runs":[]}`), + }), nil, "", fallbackRepo) + + _, err := host.GetChecks(context.Background(), fallbackPR()) + if !errors.Is(err, ErrActionsEvidenceMissing) || !errors.Is(err, scm.ErrChecksUnavailable) { + t.Fatalf("GetChecks() error = %v, want missing Actions evidence", err) + } +} + +// A run that claims success while exposing no job is not evidence of anything. +func TestGetChecksFallbackJoblessSuccessfulRunIsNotEvidence(t *testing.T) { + t.Parallel() + + responses := fallbackResponses(oneGreenRunPage, `{"total_count":0,"jobs":[]}`, buildRequired) + host := New(githubTestCmdFactory(responses), nil, "", fallbackRepo) + + _, err := host.GetChecks(context.Background(), fallbackPR()) + if !errors.Is(err, ErrActionsEvidenceMissing) || !errors.Is(err, scm.ErrChecksUnavailable) { + t.Fatalf("GetChecks() error = %v, want a jobless successful run refused", err) + } +} + +// A skipped workflow legitimately has no job, and its own conclusion is the +// evidence. It cannot be mistaken for a pass because the required-check set +// still has to map. +func TestGetChecksFallbackJoblessSkippedRunReportsTheRun(t *testing.T) { + t.Parallel() + + runs := `{"total_count":1,"workflow_runs":[{"id":101,"name":"CI","status":"completed","conclusion":"skipped","head_sha":"` + fallbackHead + `","run_attempt":1,"workflow_id":7}]}` + responses := fallbackResponses(runs, `{"total_count":0,"jobs":[]}`, `{"strict":true,"contexts":["CI"]}`) + host := New(githubTestCmdFactory(responses), nil, "", fallbackRepo) + + checks, err := host.GetChecks(context.Background(), fallbackPR()) + if err != nil { + t.Fatalf("GetChecks() error = %v", err) + } + if len(checks) != 1 || checks[0].Bucket != scm.CheckBucketSkip || checks[0].Name != "CI" { + t.Fatalf("checks = %+v, want the skipped run reported", checks) + } +} + +func TestGetChecksFallbackUsesTheRecordedPRBaseBranch(t *testing.T) { + t.Parallel() + + responses := fallbackResponses(oneGreenRunPage, oneGreenJobPage, "") + delete(responses, fallbackBaseCmd) + delete(responses, fallbackRequiredCmd("main")) + responses[fallbackRequiredCmd("release")] = githubTestResponse{stdout: buildRequired} + host := New(githubTestCmdFactory(responses), nil, "", fallbackRepo) + + pr := fallbackPR() + pr.BaseBranch = "release" + checks, err := host.GetChecks(context.Background(), pr) + if err != nil { + t.Fatalf("GetChecks() error = %v", err) + } + if len(checks) != 1 || checks[0].Name != "build" { + t.Fatalf("checks = %+v, want the build job certified against the recorded base", checks) + } +} + +func TestClassifyRollupUnavailable(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + output string + want bool + }{ + {"fine-grained token 403", "gh: Resource not accessible by personal access token (HTTP 403)", true}, + {"integration 403", "Resource not accessible by integration", true}, + {"insufficient scopes", "your token has not been granted the required scopes: insufficient scopes", true}, + {"rollup field error", "GraphQL: could not resolve statusCheckRollup", true}, + {"bad gateway", "gh: Something went wrong (HTTP 502)", false}, + {"rate limited", "API rate limit exceeded", false}, + {"empty", "", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + if got := classifyRollupUnavailable(tc.output, errors.New("exit status 1")); got != tc.want { + t.Fatalf("classifyRollupUnavailable(%q) = %v, want %v", tc.output, got, tc.want) + } + }) + } +} diff --git a/internal/scm/github/github.go b/internal/scm/github/github.go index 381f119a5..3f61e12e3 100644 --- a/internal/scm/github/github.go +++ b/internal/scm/github/github.go @@ -370,7 +370,21 @@ func (h *Host) GetChecks(ctx context.Context, pr *scm.PR) ([]scm.Check, error) { checks, err = h.getPRChecks(ctx, selector) } if err != nil { - return nil, err + // The rollup stays the only primary source. Actions evidence is + // derived only when the rollup itself is unreadable with this + // credential, and only for a commit-scoped read, which is the one + // shape that can be bound to an exact head. See actions_fallback.go. + if headSHA == "" || !errors.Is(err, ErrRollupUnavailable) { + return nil, err + } + fallback, fallbackErr := h.getActionsFallbackChecks(ctx, pr, headSHA) + if fallbackErr != nil { + return nil, fmt.Errorf("%w; GitHub Actions fallback: %w", err, fallbackErr) + } + if err := h.assertHeadUnchanged(ctx, selector, headSHA); err != nil { + return nil, err + } + return fallback, nil } if headSHA != "" { runs, err := h.getWorkflowRunChecks(ctx, headSHA) @@ -378,17 +392,27 @@ func (h *Host) GetChecks(ctx context.Context, pr *scm.PR) ([]scm.Check, error) { return nil, err } checks = h.appendUnrepresentedWorkflowRuns(checks, runs) - currentHeadSHA, err := h.getPRHeadSHA(ctx, selector) - if err != nil { + if err := h.assertHeadUnchanged(ctx, selector, headSHA); err != nil { return nil, err } - if currentHeadSHA != headSHA { - return nil, fmt.Errorf("PR head changed during check discovery from %s to %s", headSHA, currentHeadSHA) - } } return checks, nil } +// assertHeadUnchanged re-reads the PR head after check discovery, so evidence +// gathered for one commit can never be attributed to a head that moved while it +// was being read. +func (h *Host) assertHeadUnchanged(ctx context.Context, selector, headSHA string) error { + currentHeadSHA, err := h.getPRHeadSHA(ctx, selector) + if err != nil { + return err + } + if currentHeadSHA != headSHA { + return fmt.Errorf("PR head changed during check discovery from %s to %s", headSHA, currentHeadSHA) + } + return nil +} + func (h *Host) getPRChecks(ctx context.Context, selector string) ([]scm.Check, error) { args := append([]string{"pr", "checks", selector}, h.repoArgs()...) args = append(args, "--json", "name,state,bucket,completedAt,link") @@ -398,7 +422,7 @@ func (h *Host) getPRChecks(ctx context.Context, selector string) ([]scm.Check, e if strings.Contains(string(out), "no checks reported") { out = []byte("[]") } else { - return nil, fmt.Errorf("gh pr checks: %w", err) + return nil, rollupReadError("gh pr checks", string(out), err) } } var raw []struct { @@ -452,7 +476,7 @@ func (h *Host) getCommitChecks(ctx context.Context, headSHA string) ([]scm.Check } out, err := h.cmd(ctx, "gh", args...).CombinedOutput() if err != nil { - return nil, fmt.Errorf("gh api checks for head commit: %s: %w", strings.TrimSpace(string(out)), err) + return nil, rollupReadError("gh api checks for head commit", string(out), err) } var response struct { Data struct { @@ -573,7 +597,28 @@ func (h *Host) getPRHeadSHA(ctx context.Context, selector string) (string, error return headSHA, nil } -func (h *Host) getWorkflowRunChecks(ctx context.Context, headSHA string) ([]scm.Check, error) { +// workflowRun is the subset of an Actions run listing entry this backend +// reads. head_sha and run_attempt are read only by the rollup-unavailable +// fallback (actions_fallback.go), which must bind every result to the exact +// commit and attempt under test. +type workflowRun struct { + ID int64 `json:"id"` + Name string `json:"name"` + DisplayName string `json:"display_title"` + Status string `json:"status"` + Conclusion string `json:"conclusion"` + UpdatedAt string `json:"updated_at"` + HTMLURL string `json:"html_url"` + HeadSHA string `json:"head_sha"` + RunAttempt int `json:"run_attempt"` +} + +// listWorkflowRunsForHead returns every Actions workflow run GitHub reports +// for headSHA. The listing is validated rather than trusted: a page without a +// coherent total_count, a run without an id, a duplicated run, or a unique-run +// count that disagrees with total_count all mean the pagination was +// incomplete, and an incomplete listing must never become evidence. +func (h *Host) listWorkflowRunsForHead(ctx context.Context, headSHA string) ([]workflowRun, error) { repo := h.repoSlug() endpoint := "repos/{owner}/{repo}/actions/runs" if repo != "" { @@ -592,15 +637,6 @@ func (h *Host) getWorkflowRunChecks(ctx context.Context, headSHA string) ([]scm. if err != nil { return nil, fmt.Errorf("gh api workflow runs for head commit: %s: %w", strings.TrimSpace(string(out)), err) } - type workflowRun struct { - ID int64 `json:"id"` - Name string `json:"name"` - DisplayName string `json:"display_title"` - Status string `json:"status"` - Conclusion string `json:"conclusion"` - UpdatedAt string `json:"updated_at"` - HTMLURL string `json:"html_url"` - } var pages []struct { TotalCount *int `json:"total_count"` WorkflowRuns []workflowRun `json:"workflow_runs"` @@ -637,21 +673,17 @@ func (h *Host) getWorkflowRunChecks(ctx context.Context, headSHA string) ([]scm. if len(runIDs) != totalCount { return nil, fmt.Errorf("workflow run discovery returned %d unique runs, want %d", len(runIDs), totalCount) } + return raw, nil +} + +func (h *Host) getWorkflowRunChecks(ctx context.Context, headSHA string) ([]scm.Check, error) { + raw, err := h.listWorkflowRunsForHead(ctx, headSHA) + if err != nil { + return nil, err + } + repo := h.repoSlug() checks := make([]scm.Check, 0, len(raw)) for _, run := range raw { - name := strings.TrimSpace(run.Name) - if name == "" { - name = strings.TrimSpace(run.DisplayName) - } - if name == "" { - name = "GitHub Actions workflow" - } - var completedAt time.Time - if run.UpdatedAt != "" { - if parsed, parseErr := time.Parse(time.RFC3339, run.UpdatedAt); parseErr == nil { - completedAt = parsed - } - } bucket := normalizeCheckBucket("", run.Conclusion) if bucket == "" { bucket = normalizeCheckBucket("", run.Status) @@ -662,19 +694,13 @@ func (h *Host) getWorkflowRunChecks(ctx context.Context, headSHA string) ([]scm. // state that can be classified. bucket = scm.CheckBucketPending } - state := strings.ToUpper(strings.TrimSpace(run.Conclusion)) - if state == "" { - state = strings.ToUpper(strings.TrimSpace(run.Status)) - } - link := strings.TrimSpace(run.HTMLURL) - if link == "" { - host := strings.TrimSpace(h.host) - if host == "" { - host = "github.com" - } - link = fmt.Sprintf("https://%s/%s/actions/runs/%d", host, repo, run.ID) - } - checks = append(checks, scm.Check{Name: name, Bucket: bucket, State: state, CompletedAt: completedAt, Link: link}) + checks = append(checks, scm.Check{ + Name: runCheckName(run), + Bucket: bucket, + State: runState(run), + CompletedAt: parseActionsTime(run.UpdatedAt), + Link: h.runLink(run, repo), + }) } return checks, nil } diff --git a/internal/scm/host.go b/internal/scm/host.go index c426f4de5..4fccc3659 100644 --- a/internal/scm/host.go +++ b/internal/scm/host.go @@ -184,6 +184,15 @@ var ( // monitoring. It prevents a late status or already-merged race from proving // the wrong commit. ErrHeadChanged = errors.New("pull request head changed") + // ErrChecksUnavailable marks a check read that produced no usable evidence + // for the commit under test, as opposed to one that produced evidence the + // caller dislikes. Providers wrap it around every "the answer could not be + // established" failure - an unreadable rollup, an unreadable fallback, a + // mapping that cannot be pinned to the current head - so callers can bound + // repeated evidence-free polls and end with an actionable outcome instead + // of waiting for a global timeout. It never means "not green": a caller + // must keep failing closed on it. + ErrChecksUnavailable = errors.New("CI check evidence is unavailable") ) // MergedProof is provider evidence that a specific PR head was merged.