fix(scm): derive CI evidence from Actions when the check rollup is unreadable - #815
fix(scm): derive CI evidence from Actions when the check rollup is unreadable#815quinnbot-ai wants to merge 1 commit into
Conversation
GetChecks reads the GraphQL statusCheckRollup, 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 same repository's Actions workflow runs and jobs for the same commit. That failure returned immediately, so the Actions supplement added in kunchenguid#616 was never reached, and the CI step logged the same warning on every poll until ci_timeout with nothing actionable to show for it. Classify the primary failure and, only when it is capability evidence that the rollup itself is unreadable, derive job-level evidence from the Actions REST API bound to the exact repository, PR, and published head SHA. Nothing here turns doubt into a pass. Actions cannot see check runs published by other GitHub Apps, so a green Actions set is certified only when the base branch's required-check definition is readable and every required identity has exactly one exact-current-head mapping from the latest attempt. A run on another commit, a job from a superseded attempt, an incomplete listing, a required check with no unique mapping, an unprotected base branch, an unreadable required-check definition, and a successful run exposing no job all stay unavailable evidence. Non-green evidence needs no certification and is returned as-is, so a genuine failure is still surfaced when the required set cannot be read. Every failure mode wraps the new scm.ErrChecksUnavailable, and the CI step now parks with an ask-user outcome after five consecutive evidence-free polls instead of stalling to its idle timeout. A successful read resets that budget. Closes kunchenguid#690 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R5UdTu5nTj8kMPC7pidvRG
Confidence Score: 3/5The PR should not merge until required-check app identities are preserved and base branch names are safely encoded in the GitHub API route. The fallback can incorrectly certify an app-bound required check using a same-named Actions job, while valid base branches containing slashes prevent required-check discovery and force otherwise healthy runs to park. Files Needing Attention: internal/scm/github/actions_fallback.go Reviews (1): Last reviewed commit: "fix(scm): derive CI evidence from Action..." | Re-trigger Greptile |
| var payload struct { | ||
| Contexts []string `json:"contexts"` | ||
| Checks []struct { | ||
| Context string `json:"context"` | ||
| } `json:"checks"` |
There was a problem hiding this comment.
Required app identity is discarded
If branch protection binds a required check to a non-Actions GitHub App and a successful Actions job has the same context name, requiredCheckContexts discards app_id and the name-only mapping treats that job as satisfying the requirement, causing the fallback to certify green without evidence from the required app.
| 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) |
There was a problem hiding this comment.
When the PR targets a branch containing a slash or another URL-significant character, interpolating the raw branch into this REST path produces the wrong route, causing required-check discovery to fail and otherwise-green Actions evidence to remain unavailable until the CI step parks.
|
Speaking as Kun's firstmate: inspected the live diff vs main at Classification: corrective for ready-for-pr #690. Slower-correct fallback: rollup stays primary; Actions evidence is used only when the rollup itself is unreadable (capability 403 / insufficient scopes / statusCheckRollup field error). Ambiguous or incomplete Actions listings stay unavailable ( VISION: R1 pass as slower-correct fail-closed fallback (does not turn missing/ambiguous listings into a pass). Residual: required-check Required CI was |
|
Speaking as Kun's firstmate: required check Not auto-merging. CI red / no-mistakes failing is a nudge, not a captain hold. Waiting on the author to submit this change via |
Implements #690.
Problem
internal/scm/github.Host.GetChecksreads the GraphQLstatusCheckRollupon the exact PR head commit, 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 same repository's Actions workflow runs and jobs for the same commit.Today that failure returns immediately.
getWorkflowRunChecks(#616) is a supplement on the success path, so it is never reached. The CI step then logswarning: could not check CI: ...on every poll and keeps polling untilci_timeout, so a credential problem becomes an hours-long silent stall whose only artifact is a repeated warning.Behavior
internal/scm/github/actions_fallback.go(new) adds a fallback, and the CI step bounds evidence-free polling.rollupReadError/classifyRollupUnavailable); only "this credential may not read the rollup" evidence —Resource not accessible,insufficient scopes,HTTP 403, astatusCheckRollupfield error — opens the fallback. Every other failure (502, rate limit, malformed response) surfaces exactly as before.head_shawith pagination, each relevant run's jobs are listed with pagination (filter=latest), and the PR head is re-read afterwards through the sameassertHeadUnchangedguard the primary path uses.scm.Checks with the job's name, bucket, provider state, completion time, and job link — the same link shapeRerunCheckalready parses, so transient-rerun targeting keeps working on the fallback path.success(or skipped). Queued and in-progress stay pending; terminal non-success is non-green.internal/pipeline/steps/ci.gocounts consecutive polls that produced no evidence at all. AftermaxConsecutiveCheckReadFailures(5) it parks with a typed ask-user outcome naming the last provider failure instead of waiting forci_timeout. A successful read resets the budget, so ordinary transient failures are absorbed.Typed outcomes, all wrapping the new
scm.ErrChecksUnavailableso the step layer can classify without importing the provider:ErrRollupUnavailable,ErrActionsEvidenceMissing,ErrActionsEvidenceAmbiguous,ErrActionsHeadMismatch(also wrapsscm.ErrHeadChanged),ErrActionsAPIFailure.Fail-closed guarantees
An ambiguous Actions listing never becomes a pass. Everything below stays unavailable evidence rather than green:
head_shais not the head under test.total_count, contain a duplicate id, or contain an id-less entry. A short listing is the one way a failing job could disappear into a pass, so pagination is validated rather than trusted (the run listing already did this; the job listing now does the same).pushandpull_requestevent, which is exactly where a naive matcher would invent a pass.successwhile exposing no readable job. (Askippedjobless run is reported as its own skip result — that is a real conclusion, not an absence.)Non-green evidence needs none of this certification and is returned as-is, because failing and still-running results can only make the pipeline wait or escalate. That is deliberate: a genuine Actions failure is still surfaced (and auto-fixable) even when the required-check definition is unreadable.
Coverage
internal/scm/github/actions_fallback_test.go:TestGetChecksRollupFailureWithoutCapabilityEvidenceDoesNotFallBack— existing primary path is unchanged for non-capability failuresTestGetChecksFallsBackToActionsJobsAtExactHead— successful exact-head fallback, incl. job link and completion timeTestGetChecksFallbackPendingJobStaysPending,TestGetChecksFallbackFailedJobIsNonGreenTestGetChecksFallbackRequiredCheckWithoutMappingStaysUnavailable,TestGetChecksFallbackAmbiguousMappingNeverPassesTestGetChecksFallbackRejectsRunOnAnotherCommit,TestGetChecksFallbackRejectsSupersededAttemptJobTestGetChecksFallbackFollowsRunAndJobPagination,TestGetChecksFallbackRejectsIncompleteJobPaginationTestGetChecksFallbackUnreadableRequiredChecksStaysUnavailable,TestGetChecksFallbackUnprotectedBranchNeverCertifiesTestGetChecksFallbackActionsAPIFailureIsUnavailableEvidence,TestGetChecksFallbackWithoutAnyRunIsMissingEvidenceTestGetChecksFallbackJoblessSuccessfulRunIsNotEvidence,TestGetChecksFallbackJoblessSkippedRunReportsTheRunTestGetChecksFallbackUsesTheRecordedPRBaseBranch,TestClassifyRollupUnavailableinternal/pipeline/steps/ci_test.go:TestCIStep_RepeatedEvidenceFailuresParkForADecision— bounded API failure handlingTestCIStep_SuccessfulReadResetsTheEvidenceFailureBudget— an intermittent failure never accumulates into a parkgofmt,make lint, andgo test -race ./...are green.Note on the reviewer's code map
The triage comment inspected
8d6ebbf9; this branch is cut from4a5cec6. The map still holds —GetChecks/getCommitChecks/getWorkflowRunChecksand the CI monitor loop are where the work landed, with the head-SHA assignment now atci.go:359after the intervening commits.getWorkflowRunCheckskeeps its supplement-on-success role untouched; its listing/pagination validation was extracted tolistWorkflowRunsForHeadso the fallback reuses one owner of that check instead of writing a second copy.Deliberately out of scope
scm.ErrChecksUnavailableis available to them but nothing else adopts it here.