From f65e7d95bfbdb9dfdd33a94632488283fcf436b3 Mon Sep 17 00:00:00 2001 From: Harsh Batheja <40922251+harsh-batheja@users.noreply.github.com> Date: Mon, 6 Apr 2026 14:59:00 +0530 Subject: [PATCH] fix(lifecycle): reduce GitHub API rate limiting from batch enrichment bypass (#906) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(lifecycle): reduce GitHub API rate limiting from batch enrichment bypass Three optimizations to prevent API storms in the lifecycle manager poll cycle: 1. **CRITICAL - maybeDispatchMergeConflicts**: Gate the getMergeability() fallback to only run when batch enrichment didn't run at all. Previously it called getMergeability() (3 REST calls) whenever hasConflicts was undefined, even when the batch had already fetched PR data. Now uses cachedData.hasConflicts ?? false when the batch ran. 2. **HIGH - maybeDispatchCIFailureDetails**: Use batch enrichment ciChecks when available instead of calling getCIChecks() (separate REST call) on every poll. The GraphQL batch query now fetches statusCheckRollup contexts (individual check names, statuses, URLs) alongside the rollup state. Falls back to getCIChecks() only when batch didn't run. 3. **MEDIUM - maybeDispatchReviewBacklog**: Throttle getPendingComments + getAutomatedComments API calls to at most once per 2 minutes per session. These were called every 30s even when nothing had changed. Impact: ~8-10 API calls/PR/poll reduced to ~2-4, enabling 3-4x more concurrent sessions before hitting GitHub's 5,000/hr REST limit. Also extends PREnrichmentData with ciChecks?: CICheck[] and adds parseCheckContexts() helper to graphql-batch.ts for parsing CheckRun and StatusContext nodes from the GraphQL statusCheckRollup.contexts field. * fix(scm-github): fall back to getCIChecks() when contexts list is truncated When a PR has >20 CI checks, contexts(first: 20) silently truncates the list. Setting ciChecks to undefined when pageInfo.hasNextPage is true ensures maybeDispatchCIFailureDetails falls back to the getCIChecks() REST call, which returns all checks without truncation. Also adds pageInfo { hasNextPage } to the contexts GraphQL query so truncation can be detected. * fix(lifecycle): prune lastReviewBacklogCheckAt in pollAll cleanup loop Add the new throttle map to the existing pruning loop that removes stale entries for sessions no longer in the session list. Previously the map was only cleared on terminal status transitions, leaving orphaned entries for sessions removed externally (killed + cleaned up without transition). * fix(lifecycle): bypass throttle on review transition; fix StatusContext conclusion Two fixes for automated review findings: 1. Bypass review backlog throttle when a transition reaction just fired for humanReactionKey or automatedReactionKey. The transitionReaction branch needs to read the current fingerprint via the API to record lastPendingReviewDispatchHash. Without bypassing, the throttle prevents this write and the next unthrottled poll sees a stale (empty) hash, clears the reaction tracker, and fires a duplicate dispatch. 2. Set conclusion on StatusContext nodes in parseCheckContexts() to match the REST getCIChecksFromStatusRollup() format (rawState.toUpperCase()). The CI failure fingerprint includes c.conclusion ?? '', so inconsistent conclusion values between GraphQL and REST paths caused phantom fingerprint changes when switching sources, triggering duplicate dispatches. * fix(scm-github): normalize CheckRun conclusion and map NEUTRAL to skipped Two consistency fixes in parseCheckContexts() vs the REST path: 1. NEUTRAL conclusion: was mapped to 'passed' (with SUCCESS), but mapRawCheckStateToStatus() in the REST path maps NEUTRAL to 'skipped'. Changed to treat NEUTRAL the same as SKIPPED. 2. CheckRun conclusion: was stored as the raw GraphQL string (may be lowercase). REST getCIChecks/getCIChecksFromStatusRollup always store conclusion as rawState.toUpperCase(). Now stores rawConclusion which is already uppercased during the status branching logic. Both fixes prevent phantom fingerprint changes when maybeDispatchCIFailureDetails switches between GraphQL batch and REST fallback across poll cycles. * fix(scm-github): map STALE/NOT_REQUIRED/NONE conclusions to skipped parseCheckContexts() was mapping these conclusions to 'failed' via the else fallback, while mapRawCheckStateToStatus() in the REST path explicitly maps all of them to 'skipped'. Added them to the skipped branch alongside SKIPPED and NEUTRAL to fully mirror the REST mapping. * fix(scm-github): map QUEUED/WAITING to pending not running parseCheckContexts() mapped QUEUED and WAITING CheckRun statuses to 'running', but mapRawCheckStateToStatus() in the REST path maps both to 'pending'. Only IN_PROGRESS maps to 'running' in the REST path. Fixes fingerprint inconsistency when switching between GraphQL batch and REST fallback across poll cycles. * fix(scm-github): map STARTUP_FAILURE to skipped; guard null pageInfo - STARTUP_FAILURE conclusion now falls through to the "skipped" branch (matching mapRawCheckStateToStatus() REST default) instead of the explicit failure enumeration catch-all - Null pageInfo guard prevents TypeError from typeof null === "object" JavaScript quirk when accessing hasNextPage on a null pageInfo field - Tests added for both cases * fix(scm-github): map COMPLETED+null conclusion to skipped not passed When a CheckRun has status COMPLETED and conclusion null, the REST path's mapRawCheckStateToStatus() converts it to "" which maps to "skipped". The GraphQL path was incorrectly mapping it to "passed" via !rawConclusion. Fix: only map rawConclusion === "SUCCESS" to "passed"; null falls through to the else branch → "skipped", matching the REST path exactly. --- .../src/__tests__/lifecycle-manager.test.ts | 195 ++++++ packages/core/src/lifecycle-manager.ts | 73 ++- packages/core/src/types.ts | 2 + .../plugins/scm-github/src/graphql-batch.ts | 149 ++++- .../scm-github/test/graphql-batch.test.ts | 573 ++++++++++++++++++ 5 files changed, 973 insertions(+), 19 deletions(-) diff --git a/packages/core/src/__tests__/lifecycle-manager.test.ts b/packages/core/src/__tests__/lifecycle-manager.test.ts index e18643aa0..94566d37a 100644 --- a/packages/core/src/__tests__/lifecycle-manager.test.ts +++ b/packages/core/src/__tests__/lifecycle-manager.test.ts @@ -1477,3 +1477,198 @@ describe("getStates", () => { expect(lm.getStates().get("app-1")).toBe("working"); }); }); + +describe("rate limiting optimizations", () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + // PR with owner/repo that matches the test config's "org/my-app" + function makeMatchingPR() { + return makePR({ owner: "org", repo: "my-app" }); + } + + it("skips getMergeability() when batch enrichment has hasConflicts data", async () => { + config.reactions = { + "merge-conflicts": { + auto: true, + action: "send-to-agent", + message: "Resolve conflicts.", + }, + }; + + const pr = makeMatchingPR(); + const getMergeabilityMock = vi.fn(); + const mockSCM = createMockSCM({ + getMergeability: getMergeabilityMock, + getCISummary: vi.fn().mockResolvedValue("passing"), + enrichSessionsPRBatch: vi.fn().mockResolvedValue( + new Map([ + [ + `${pr.owner}/${pr.repo}#${pr.number}`, + { + state: "open" as const, + ciStatus: "passing" as const, + reviewDecision: "none" as const, + mergeable: false, + hasConflicts: true, + }, + ], + ]), + ), + }); + + const registry = createMockRegistry({ + runtime: plugins.runtime, + agent: plugins.agent, + scm: mockSCM, + }); + + const session = makeSession({ id: "s-1", status: "pr_open", pr }); + vi.mocked(mockSessionManager.list).mockResolvedValue([session]); + vi.mocked(mockSessionManager.send).mockResolvedValue(undefined); + + const lm = createLifecycleManager({ config, registry, sessionManager: mockSessionManager }); + lm.start(60_000); + await vi.advanceTimersByTimeAsync(0); + lm.stop(); + + // getMergeability() should NOT be called — batch enrichment has the data + expect(getMergeabilityMock).not.toHaveBeenCalled(); + // Conflict notification should have been sent + expect(mockSessionManager.send).toHaveBeenCalledWith("s-1", "Resolve conflicts."); + }); + + it("skips getCIChecks() when batch enrichment has ciChecks data", async () => { + config.reactions = { + "ci-failed": { + auto: true, + action: "send-to-agent", + message: "CI failing.", + retries: 3, + escalateAfter: 3, + }, + }; + + const pr = makeMatchingPR(); + const getCIChecksMock = vi.fn(); + const mockSCM = createMockSCM({ + getCIChecks: getCIChecksMock, + getCISummary: vi.fn().mockResolvedValue("failing"), + enrichSessionsPRBatch: vi.fn().mockResolvedValue( + new Map([ + [ + `${pr.owner}/${pr.repo}#${pr.number}`, + { + state: "open" as const, + ciStatus: "failing" as const, + reviewDecision: "none" as const, + mergeable: false, + hasConflicts: false, + ciChecks: [ + { name: "lint", status: "failed" as const, conclusion: "FAILURE", url: "https://example.com/lint" }, + { name: "test", status: "passed" as const, conclusion: "SUCCESS" }, + ], + }, + ], + ]), + ), + }); + + const registry = createMockRegistry({ + runtime: plugins.runtime, + agent: plugins.agent, + scm: mockSCM, + }); + + // Start with pr_open state so that ci_failed transition happens on first poll + const session = makeSession({ id: "s-2", status: "pr_open", pr }); + vi.mocked(mockSessionManager.list).mockResolvedValue([session]); + vi.mocked(mockSessionManager.send).mockResolvedValue(undefined); + + const lm = createLifecycleManager({ config, registry, sessionManager: mockSessionManager }); + lm.start(60_000); + // First poll: transitions to ci_failed, sends reaction message + await vi.advanceTimersByTimeAsync(0); + + vi.mocked(mockSessionManager.send).mockClear(); + + // Second poll: dispatches detailed CI failure info + await vi.advanceTimersByTimeAsync(60_000); + + // getCIChecks() should NOT be called — batch enrichment has ciChecks + expect(getCIChecksMock).not.toHaveBeenCalled(); + // Detailed message with lint check name/URL should be sent + const calls = vi.mocked(mockSessionManager.send).mock.calls; + const sentMessages = calls.map((c) => c[1] as string); + const detailMessage = sentMessages.find((m) => m.includes("lint")); + expect(detailMessage).toBeDefined(); + expect(detailMessage).toContain("https://example.com/lint"); + // Passing check should not be included + expect(detailMessage).not.toContain("test"); + + lm.stop(); + }); + + it("throttles review backlog API calls to at most once per 2 minutes", async () => { + config.reactions = { + "changes-requested": { + auto: true, + action: "send-to-agent", + message: "Handle review comments.", + }, + }; + + const getPendingMock = vi.fn().mockResolvedValue([ + { + id: "c1", + author: "reviewer", + body: "Please fix this", + path: "src/index.ts", + line: 10, + isResolved: false, + createdAt: new Date(), + url: "https://example.com/comment/1", + }, + ]); + const getAutomatedMock = vi.fn().mockResolvedValue([]); + const mockSCM = createMockSCM({ + getPendingComments: getPendingMock, + getAutomatedComments: getAutomatedMock, + }); + const registry = createMockRegistry({ + runtime: plugins.runtime, + agent: plugins.agent, + scm: mockSCM, + }); + + vi.mocked(mockSessionManager.send).mockResolvedValue(undefined); + + const lm = setupCheck("app-1", { + session: makeSession({ status: "pr_open", pr: makePR() }), + registry, + }); + + // First check: API called, dispatch happens + await lm.check("app-1"); + expect(getPendingMock).toHaveBeenCalledTimes(1); + vi.mocked(mockSessionManager.send).mockClear(); + getPendingMock.mockClear(); + + // Second check immediately after: throttled — API NOT called + await lm.check("app-1"); + expect(getPendingMock).not.toHaveBeenCalled(); + expect(mockSessionManager.send).not.toHaveBeenCalled(); + + // Advance time past the 2-minute throttle window + await vi.advanceTimersByTimeAsync(2 * 60 * 1000 + 100); + + // Third check: throttle expired — API called again + await lm.check("app-1"); + expect(getPendingMock).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/core/src/lifecycle-manager.ts b/packages/core/src/lifecycle-manager.ts index 944617512..8e1841c24 100644 --- a/packages/core/src/lifecycle-manager.ts +++ b/packages/core/src/lifecycle-manager.ts @@ -208,6 +208,16 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan */ const prEnrichmentCache = new Map(); + /** + * Per-session timestamp of last review backlog API check. + * Used to throttle getPendingComments/getAutomatedComments to at most once per 2 minutes. + * In-memory only — resets on restart (acceptable since it's a rate-limit hint, not state). + */ + const lastReviewBacklogCheckAt = new Map(); + + /** Throttle interval for review backlog API calls (2 minutes). */ + const REVIEW_BACKLOG_THROTTLE_MS = 2 * 60 * 1000; + /** * Populate the PR enrichment cache using batch GraphQL queries. * This is called once per poll cycle to fetch data for all PRs efficiently. @@ -739,6 +749,7 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan if (TERMINAL_STATUSES.has(newStatus)) { clearReactionTracker(session.id, humanReactionKey); clearReactionTracker(session.id, automatedReactionKey); + lastReviewBacklogCheckAt.delete(session.id); updateSessionMetadata(session, { lastPendingReviewFingerprint: "", lastPendingReviewDispatchHash: "", @@ -750,6 +761,27 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan return; } + // Throttle review backlog API calls to at most once per 2 minutes. + // Comments don't change faster than this in practice, and the SCM calls + // (getPendingComments + getAutomatedComments) consume API quota on every poll. + // + // Exception: bypass throttle when a transition reaction just fired for a + // review reaction key. The transitionReaction branch records + // lastPendingReviewDispatchHash, which requires the current fingerprint from + // the API. If we throttle here, that metadata never gets written and the + // next unthrottled poll sees a "new" fingerprint, clears the reaction tracker, + // and fires a duplicate dispatch. + const hasRelevantTransition = + transitionReaction?.key === humanReactionKey || + transitionReaction?.key === automatedReactionKey; + if (!hasRelevantTransition) { + const lastCheckAt = lastReviewBacklogCheckAt.get(session.id) ?? 0; + if (Date.now() - lastCheckAt < REVIEW_BACKLOG_THROTTLE_MS) { + return; + } + } + lastReviewBacklogCheckAt.set(session.id, Date.now()); + const [pendingResult, automatedResult] = await Promise.allSettled([ scm.getPendingComments(session.pr), scm.getAutomatedComments(session.pr), @@ -941,13 +973,22 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan return; } - // Fetch individual CI checks for failure details + // Fetch individual CI checks for failure details. + // Use batch enrichment data when available to avoid an extra REST call; + // fall back to getCIChecks() when the batch didn't run this cycle. + const prKey = `${session.pr.owner}/${session.pr.repo}#${session.pr.number}`; + const cachedEnrichment = prEnrichmentCache.get(prKey); + let checks: CICheck[]; - try { - checks = await scm.getCIChecks(session.pr); - } catch { - // Failed to fetch checks — skip this cycle - return; + if (cachedEnrichment?.ciChecks !== undefined) { + checks = cachedEnrichment.ciChecks; + } else { + try { + checks = await scm.getCIChecks(session.pr); + } catch { + // Failed to fetch checks — skip this cycle + return; + } } const failedChecks = checks.filter( @@ -1058,14 +1099,19 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan return; } - // Check for conflicts using cached enrichment data or fallback to individual call + // Check for conflicts using cached enrichment data or fallback to individual call. + // When batch enrichment ran (cachedData is present), use its hasConflicts value + // to avoid 3 redundant REST calls from getMergeability() — the batch already + // fetched the mergeable/mergeStateStatus fields via GraphQL. const prKey = `${session.pr.owner}/${session.pr.repo}#${session.pr.number}`; const cachedData = prEnrichmentCache.get(prKey); let hasConflicts: boolean; - if (cachedData && cachedData.hasConflicts !== undefined) { - hasConflicts = cachedData.hasConflicts; + if (cachedData) { + // Batch ran — trust its data (undefined means CONFLICTING wasn't set → no conflicts) + hasConflicts = cachedData.hasConflicts ?? false; } else { + // Batch didn't run this cycle — fall back to individual API call try { const mergeReadiness = await scm.getMergeability(session.pr); hasConflicts = !mergeReadiness.noConflicts; @@ -1256,8 +1302,8 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan // Poll all sessions concurrently await Promise.allSettled(sessionsToCheck.map((s) => checkSession(s))); - // Prune stale entries from states and reactionTrackers for sessions - // that no longer appear in the session list (e.g., after kill/cleanup) + // Prune stale entries from states, reactionTrackers, and lastReviewBacklogCheckAt + // for sessions that no longer appear in the session list (e.g., after kill/cleanup) const currentSessionIds = new Set(sessions.map((s) => s.id)); for (const trackedId of states.keys()) { if (!currentSessionIds.has(trackedId)) { @@ -1270,6 +1316,11 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan reactionTrackers.delete(trackerKey); } } + for (const sessionId of lastReviewBacklogCheckAt.keys()) { + if (!currentSessionIds.has(sessionId)) { + lastReviewBacklogCheckAt.delete(sessionId); + } + } // Check if all sessions are complete (trigger reaction only once) const activeSessions = sessions.filter((s) => !TERMINAL_STATUSES.has(s.status)); diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index 1d6d0eee0..0416bd6b3 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -816,6 +816,8 @@ export interface PREnrichmentData { isBehind?: boolean; /** List of blockers preventing merge */ blockers?: string[]; + /** Individual CI check results (populated from batch enrichment when available) */ + ciChecks?: CICheck[]; } /** diff --git a/packages/plugins/scm-github/src/graphql-batch.ts b/packages/plugins/scm-github/src/graphql-batch.ts index 783d207a0..f877e3d58 100644 --- a/packages/plugins/scm-github/src/graphql-batch.ts +++ b/packages/plugins/scm-github/src/graphql-batch.ts @@ -9,6 +9,7 @@ import { execFile } from "node:child_process"; import { promisify } from "node:util"; import type { BatchObserver, + CICheck, CIStatus, PREnrichmentData, PRInfo, @@ -466,6 +467,24 @@ const PR_FIELDS = ` commit { statusCheckRollup { state + contexts(first: 20) { + nodes { + ... on CheckRun { + name + status + conclusion + detailsUrl + } + ... on StatusContext { + context + state + targetUrl + } + } + pageInfo { + hasNextPage + } + } } } } @@ -574,12 +593,108 @@ async function executeBatchQuery( return (result.data ?? {}) as Record; } +/** + * Parse individual CI check contexts from statusCheckRollup.contexts.nodes. + * Handles both CheckRun (GitHub Actions) and StatusContext (legacy status checks). + */ +function parseCheckContexts(contexts: unknown): CICheck[] { + if (!contexts || typeof contexts !== "object") return []; + + const nodes = (contexts as Record)["nodes"]; + if (!Array.isArray(nodes)) return []; + + const checks: CICheck[] = []; + for (const node of nodes) { + if (!node || typeof node !== "object") continue; + const n = node as Record; + + // CheckRun node (GitHub Actions) + if (typeof n["name"] === "string" && typeof n["status"] === "string") { + const rawStatus = (n["status"] as string).toUpperCase(); + // Uppercase conclusion to match REST getCIChecks/getCIChecksFromStatusRollup format + // so fingerprints are consistent regardless of which data source is used. + const rawConclusion = + typeof n["conclusion"] === "string" ? (n["conclusion"] as string).toUpperCase() : null; + + let status: CICheck["status"]; + if (rawStatus === "COMPLETED") { + if (rawConclusion === "SUCCESS") { + status = "passed"; + } else if ( + rawConclusion === "SKIPPED" || + rawConclusion === "NEUTRAL" || + rawConclusion === "STALE" || + rawConclusion === "NOT_REQUIRED" || + rawConclusion === "NONE" + ) { + // Mirror mapRawCheckStateToStatus() in the REST path: all non-failure + // terminal conclusions that are not SUCCESS map to "skipped". + status = "skipped"; + } else if ( + rawConclusion === "FAILURE" || + rawConclusion === "TIMED_OUT" || + rawConclusion === "CANCELLED" || + rawConclusion === "ACTION_REQUIRED" || + rawConclusion === "ERROR" + ) { + // Explicit failure conclusions — mirrors the failure list in mapRawCheckStateToStatus() + status = "failed"; + } else { + // STARTUP_FAILURE and any other unrecognized conclusion → "skipped", + // matching mapRawCheckStateToStatus()'s default return "skipped" in the REST path. + status = "skipped"; + } + } else if (rawStatus === "IN_PROGRESS") { + // Only IN_PROGRESS maps to "running" — matches mapRawCheckStateToStatus() in REST path + status = "running"; + } else { + // QUEUED, WAITING, and any other non-COMPLETED status → "pending" + // (REST path maps QUEUED/WAITING to "pending", not "running") + status = "pending"; + } + + checks.push({ + name: n["name"] as string, + status, + // Store the uppercased conclusion to match REST format + conclusion: rawConclusion ?? undefined, + url: typeof n["detailsUrl"] === "string" ? (n["detailsUrl"] as string) : undefined, + }); + continue; + } + + // StatusContext node (legacy commit statuses) + if (typeof n["context"] === "string" && typeof n["state"] === "string") { + const rawState = (n["state"] as string).toUpperCase(); + let status: CICheck["status"]; + if (rawState === "SUCCESS") { + status = "passed"; + } else if (rawState === "FAILURE" || rawState === "ERROR") { + status = "failed"; + } else { + status = "pending"; + } + + // Set conclusion to match the REST getCIChecksFromStatusRollup format + // (which sets conclusion = rawState.toUpperCase()) so fingerprints are + // consistent regardless of which data source is used. + checks.push({ + name: n["context"] as string, + status, + conclusion: rawState, + url: typeof n["targetUrl"] === "string" ? (n["targetUrl"] as string) : undefined, + }); + } + } + + return checks; +} + /** * Parse raw CI state from status check rollup. * - * Uses only the top-level aggregate state, which is much cheaper - * than fetching individual check contexts. The top-level state - * provides the same semantic information (passing/failing/pending). + * Uses only the top-level aggregate state to determine overall CI status. + * Individual check details are parsed separately via parseCheckContexts(). */ function parseCIState( statusCheckRollup: unknown, @@ -679,13 +794,30 @@ function extractPREnrichment( // Extract review decision const reviewDecision = parseReviewDecision(pr["reviewDecision"]); - // Extract CI status from commits + // Extract CI status and individual checks from commits const commits = pr["commits"] as - | { nodes?: Array<{ commit?: { statusCheckRollup?: unknown } }> } + | { nodes?: Array<{ commit?: { statusCheckRollup?: Record } }> } | undefined; - const ciStatus = commits?.nodes?.[0]?.commit?.statusCheckRollup - ? parseCIState(commits.nodes[0].commit.statusCheckRollup) - : "none"; + const statusCheckRollup = commits?.nodes?.[0]?.commit?.statusCheckRollup; + const ciStatus = statusCheckRollup ? parseCIState(statusCheckRollup) : "none"; + + // Only include ciChecks when the list is complete (no truncation). + // contexts(first: 20) silently truncates PRs with >20 checks — when truncated, + // the failing check may be missing, so we set ciChecks to undefined to force + // the getCIChecks() REST fallback in maybeDispatchCIFailureDetails. + const contextsField = statusCheckRollup?.["contexts"] as + | Record + | undefined; + const pageInfo = contextsField?.["pageInfo"]; + const contextsHasNextPage = + pageInfo !== null && + pageInfo !== undefined && + typeof pageInfo === "object" && + (pageInfo as Record)["hasNextPage"] === true; + const ciChecks = + contextsField && !contextsHasNextPage + ? parseCheckContexts(contextsField) + : undefined; // Build blockers list const blockers: string[] = []; @@ -722,6 +854,7 @@ function extractPREnrichment( hasConflicts, isBehind, blockers, + ...(ciChecks !== undefined ? { ciChecks } : {}), }; return { data, headSha }; diff --git a/packages/plugins/scm-github/test/graphql-batch.test.ts b/packages/plugins/scm-github/test/graphql-batch.test.ts index a25417087..cdab3eea2 100644 --- a/packages/plugins/scm-github/test/graphql-batch.test.ts +++ b/packages/plugins/scm-github/test/graphql-batch.test.ts @@ -1172,3 +1172,576 @@ describe("shouldRefreshPREnrichment - ETag Guard Strategy", () => { }); }); }); + +describe("extractPREnrichment ciChecks", () => { + it("parses CheckRun contexts into ciChecks", () => { + const pullRequest = { + title: "Fix CI", + state: "OPEN", + additions: 10, + deletions: 5, + isDraft: false, + mergeable: "MERGEABLE", + mergeStateStatus: "CLEAN", + reviewDecision: "NONE", + reviews: { nodes: [] }, + commits: { + nodes: [ + { + commit: { + statusCheckRollup: { + state: "FAILURE", + contexts: { + nodes: [ + { + name: "lint", + status: "COMPLETED", + conclusion: "FAILURE", + detailsUrl: "https://github.com/org/repo/actions/runs/123", + }, + { + name: "test", + status: "COMPLETED", + conclusion: "SUCCESS", + detailsUrl: "https://github.com/org/repo/actions/runs/124", + }, + { + name: "typecheck", + status: "IN_PROGRESS", + conclusion: null, + detailsUrl: "https://github.com/org/repo/actions/runs/125", + }, + ], + }, + }, + }, + }, + ], + }, + }; + + const extracted = extractPREnrichment(pullRequest); + const ciChecks = extracted?.data.ciChecks; + + expect(ciChecks).toBeDefined(); + expect(ciChecks).toHaveLength(3); + + const lint = ciChecks?.find((c) => c.name === "lint"); + expect(lint?.status).toBe("failed"); + expect(lint?.conclusion).toBe("FAILURE"); + expect(lint?.url).toBe("https://github.com/org/repo/actions/runs/123"); + + const test = ciChecks?.find((c) => c.name === "test"); + expect(test?.status).toBe("passed"); + expect(test?.conclusion).toBe("SUCCESS"); + + const typecheck = ciChecks?.find((c) => c.name === "typecheck"); + expect(typecheck?.status).toBe("running"); + }); + + it("maps QUEUED and WAITING to pending, not running (matches REST mapRawCheckStateToStatus)", () => { + const pullRequest = { + title: "Queued checks", + state: "OPEN", + additions: 1, + deletions: 0, + isDraft: false, + mergeable: "MERGEABLE", + mergeStateStatus: "CLEAN", + reviewDecision: "NONE", + reviews: { nodes: [] }, + commits: { + nodes: [ + { + commit: { + statusCheckRollup: { + state: "PENDING", + contexts: { + nodes: [ + { name: "queued-check", status: "QUEUED", conclusion: null, detailsUrl: null }, + { name: "waiting-check", status: "WAITING", conclusion: null, detailsUrl: null }, + ], + }, + }, + }, + }, + ], + }, + }; + + const extracted = extractPREnrichment(pullRequest); + const ciChecks = extracted?.data.ciChecks; + expect(ciChecks?.find((c) => c.name === "queued-check")?.status).toBe("pending"); + expect(ciChecks?.find((c) => c.name === "waiting-check")?.status).toBe("pending"); + }); + + it("parses StatusContext nodes into ciChecks", () => { + const pullRequest = { + title: "Legacy CI", + state: "OPEN", + additions: 5, + deletions: 2, + isDraft: false, + mergeable: "MERGEABLE", + mergeStateStatus: "CLEAN", + reviewDecision: "NONE", + reviews: { nodes: [] }, + commits: { + nodes: [ + { + commit: { + statusCheckRollup: { + state: "FAILURE", + contexts: { + nodes: [ + { + context: "ci/build", + state: "FAILURE", + targetUrl: "https://ci.example.com/build/1", + }, + { + context: "ci/test", + state: "SUCCESS", + targetUrl: "https://ci.example.com/test/1", + }, + { + context: "ci/deploy", + state: "PENDING", + targetUrl: null, + }, + ], + }, + }, + }, + }, + ], + }, + }; + + const extracted = extractPREnrichment(pullRequest); + const ciChecks = extracted?.data.ciChecks; + + expect(ciChecks).toBeDefined(); + expect(ciChecks).toHaveLength(3); + + const build = ciChecks?.find((c) => c.name === "ci/build"); + expect(build?.status).toBe("failed"); + expect(build?.conclusion).toBe("FAILURE"); // matches REST getCIChecksFromStatusRollup format + expect(build?.url).toBe("https://ci.example.com/build/1"); + + const test = ciChecks?.find((c) => c.name === "ci/test"); + expect(test?.status).toBe("passed"); + expect(test?.conclusion).toBe("SUCCESS"); + + const deploy = ciChecks?.find((c) => c.name === "ci/deploy"); + expect(deploy?.status).toBe("pending"); + expect(deploy?.conclusion).toBe("PENDING"); + expect(deploy?.url).toBeUndefined(); + }); + + it("returns empty array when contexts nodes is empty", () => { + const pullRequest = { + title: "Clean PR", + state: "OPEN", + additions: 5, + deletions: 2, + isDraft: false, + mergeable: "MERGEABLE", + mergeStateStatus: "CLEAN", + reviewDecision: "NONE", + reviews: { nodes: [] }, + commits: { + nodes: [ + { + commit: { + statusCheckRollup: { + state: "SUCCESS", + contexts: { nodes: [] }, + }, + }, + }, + ], + }, + }; + + const extracted = extractPREnrichment(pullRequest); + expect(extracted?.data.ciChecks).toEqual([]); + }); + + it("returns undefined ciChecks when statusCheckRollup has no contexts field", () => { + const pullRequest = { + title: "No contexts", + state: "OPEN", + additions: 5, + deletions: 2, + isDraft: false, + mergeable: "MERGEABLE", + mergeStateStatus: "CLEAN", + reviewDecision: "NONE", + reviews: { nodes: [] }, + commits: { + nodes: [ + { + commit: { + statusCheckRollup: { + state: "SUCCESS", + // No contexts field — older API response + }, + }, + }, + ], + }, + }; + + const extracted = extractPREnrichment(pullRequest); + expect(extracted?.data.ciChecks).toBeUndefined(); + }); + + it("maps COMPLETED+SKIPPED conclusion to skipped status", () => { + const pullRequest = { + title: "Skipped check", + state: "OPEN", + additions: 5, + deletions: 2, + isDraft: false, + mergeable: "MERGEABLE", + mergeStateStatus: "CLEAN", + reviewDecision: "NONE", + reviews: { nodes: [] }, + commits: { + nodes: [ + { + commit: { + statusCheckRollup: { + state: "SUCCESS", + contexts: { + nodes: [ + { + name: "optional-check", + status: "COMPLETED", + conclusion: "SKIPPED", + detailsUrl: null, + }, + ], + }, + }, + }, + }, + ], + }, + }; + + const extracted = extractPREnrichment(pullRequest); + const check = extracted?.data.ciChecks?.[0]; + expect(check?.status).toBe("skipped"); + expect(check?.conclusion).toBe("SKIPPED"); // uppercased to match REST format + }); + + it("maps COMPLETED+NEUTRAL conclusion to skipped (matches REST mapRawCheckStateToStatus)", () => { + const pullRequest = { + title: "Neutral check", + state: "OPEN", + additions: 5, + deletions: 2, + isDraft: false, + mergeable: "MERGEABLE", + mergeStateStatus: "CLEAN", + reviewDecision: "NONE", + reviews: { nodes: [] }, + commits: { + nodes: [ + { + commit: { + statusCheckRollup: { + state: "SUCCESS", + contexts: { + nodes: [ + { + name: "optional-check", + status: "COMPLETED", + conclusion: "NEUTRAL", + detailsUrl: null, + }, + ], + }, + }, + }, + }, + ], + }, + }; + + const extracted = extractPREnrichment(pullRequest); + const check = extracted?.data.ciChecks?.[0]; + // NEUTRAL maps to "skipped" in REST mapRawCheckStateToStatus — must match + expect(check?.status).toBe("skipped"); + expect(check?.conclusion).toBe("NEUTRAL"); + }); + + it("uppercases CheckRun conclusion to match REST getCIChecks format", () => { + const pullRequest = { + title: "Mixed case conclusion", + state: "OPEN", + additions: 5, + deletions: 2, + isDraft: false, + mergeable: "MERGEABLE", + mergeStateStatus: "CLEAN", + reviewDecision: "NONE", + reviews: { nodes: [] }, + commits: { + nodes: [ + { + commit: { + statusCheckRollup: { + state: "FAILURE", + contexts: { + nodes: [ + { + name: "lint", + status: "COMPLETED", + conclusion: "failure", // lowercase from API + detailsUrl: null, + }, + ], + }, + }, + }, + }, + ], + }, + }; + + const extracted = extractPREnrichment(pullRequest); + const check = extracted?.data.ciChecks?.[0]; + expect(check?.status).toBe("failed"); + expect(check?.conclusion).toBe("FAILURE"); // must be uppercased + }); + + it("maps STALE/NOT_REQUIRED/NONE conclusions to skipped (matches REST mapRawCheckStateToStatus)", () => { + const makeContextsWithConclusion = (conclusion: string) => ({ + title: "Check", + state: "OPEN", + additions: 1, + deletions: 0, + isDraft: false, + mergeable: "MERGEABLE", + mergeStateStatus: "CLEAN", + reviewDecision: "NONE", + reviews: { nodes: [] }, + commits: { + nodes: [ + { + commit: { + statusCheckRollup: { + state: "SUCCESS", + contexts: { + nodes: [{ name: "check", status: "COMPLETED", conclusion, detailsUrl: null }], + }, + }, + }, + }, + ], + }, + }); + + for (const conclusion of ["STALE", "NOT_REQUIRED", "NONE"]) { + const extracted = extractPREnrichment(makeContextsWithConclusion(conclusion)); + const check = extracted?.data.ciChecks?.[0]; + expect(check?.status, `${conclusion} should map to skipped`).toBe("skipped"); + } + }); + + it("returns undefined ciChecks when contexts list is truncated (hasNextPage=true)", () => { + const pullRequest = { + title: "Many CI checks", + state: "OPEN", + additions: 5, + deletions: 2, + isDraft: false, + mergeable: "MERGEABLE", + mergeStateStatus: "CLEAN", + reviewDecision: "NONE", + reviews: { nodes: [] }, + commits: { + nodes: [ + { + commit: { + statusCheckRollup: { + state: "FAILURE", + contexts: { + nodes: [ + { name: "check-1", status: "COMPLETED", conclusion: "FAILURE", detailsUrl: null }, + // ... 19 more checks truncated + ], + pageInfo: { hasNextPage: true }, // list was truncated! + }, + }, + }, + }, + ], + }, + }; + + const extracted = extractPREnrichment(pullRequest); + // ciChecks should be undefined when truncated — forces getCIChecks() REST fallback + expect(extracted?.data.ciChecks).toBeUndefined(); + }); + + it("returns ciChecks when contexts list is complete (hasNextPage=false)", () => { + const pullRequest = { + title: "Few CI checks", + state: "OPEN", + additions: 5, + deletions: 2, + isDraft: false, + mergeable: "MERGEABLE", + mergeStateStatus: "CLEAN", + reviewDecision: "NONE", + reviews: { nodes: [] }, + commits: { + nodes: [ + { + commit: { + statusCheckRollup: { + state: "FAILURE", + contexts: { + nodes: [ + { name: "lint", status: "COMPLETED", conclusion: "FAILURE", detailsUrl: "https://ci.example.com/lint" }, + ], + pageInfo: { hasNextPage: false }, // complete list + }, + }, + }, + }, + ], + }, + }; + + const extracted = extractPREnrichment(pullRequest); + expect(extracted?.data.ciChecks).toBeDefined(); + expect(extracted?.data.ciChecks).toHaveLength(1); + expect(extracted?.data.ciChecks?.[0]?.name).toBe("lint"); + }); + + it("maps COMPLETED with null conclusion to skipped (matches REST mapRawCheckStateToStatus empty-string branch)", () => { + // REST path: mapRawCheckStateToStatus(undefined) → state="" → "skipped" + // GraphQL path must match: COMPLETED + null conclusion → "skipped", not "passed" + const pullRequest = { + title: "Null conclusion check", + state: "OPEN", + additions: 1, + deletions: 0, + isDraft: false, + mergeable: "MERGEABLE", + mergeStateStatus: "CLEAN", + reviewDecision: "NONE", + reviews: { nodes: [] }, + commits: { + nodes: [ + { + commit: { + statusCheckRollup: { + state: "SUCCESS", + contexts: { + nodes: [ + { + name: "some-check", + status: "COMPLETED", + conclusion: null, // null conclusion — shouldn't map to "passed" + detailsUrl: null, + }, + ], + }, + }, + }, + }, + ], + }, + }; + + const extracted = extractPREnrichment(pullRequest); + const check = extracted?.data.ciChecks?.[0]; + expect(check?.status).toBe("skipped"); + }); + + it("maps COMPLETED+STARTUP_FAILURE to skipped (matches REST mapRawCheckStateToStatus default fallback)", () => { + const pullRequest = { + title: "Startup failure check", + state: "OPEN", + additions: 1, + deletions: 0, + isDraft: false, + mergeable: "MERGEABLE", + mergeStateStatus: "CLEAN", + reviewDecision: "NONE", + reviews: { nodes: [] }, + commits: { + nodes: [ + { + commit: { + statusCheckRollup: { + state: "FAILURE", + contexts: { + nodes: [ + { + name: "infra-check", + status: "COMPLETED", + conclusion: "STARTUP_FAILURE", + detailsUrl: null, + }, + ], + }, + }, + }, + }, + ], + }, + }; + + const extracted = extractPREnrichment(pullRequest); + const check = extracted?.data.ciChecks?.[0]; + // STARTUP_FAILURE is not in the explicit failure list → falls through to "skipped" + // matching mapRawCheckStateToStatus()'s default return "skipped" + expect(check?.status).toBe("skipped"); + expect(check?.conclusion).toBe("STARTUP_FAILURE"); + }); + + it("handles null pageInfo safely without TypeError (typeof null === 'object' quirk)", () => { + const pullRequest = { + title: "Null pageInfo", + state: "OPEN", + additions: 1, + deletions: 0, + isDraft: false, + mergeable: "MERGEABLE", + mergeStateStatus: "CLEAN", + reviewDecision: "NONE", + reviews: { nodes: [] }, + commits: { + nodes: [ + { + commit: { + statusCheckRollup: { + state: "SUCCESS", + contexts: { + nodes: [ + { name: "lint", status: "COMPLETED", conclusion: "SUCCESS", detailsUrl: null }, + ], + pageInfo: null, // null pageInfo — older API responses may omit this + }, + }, + }, + }, + ], + }, + }; + + // Must not throw TypeError: Cannot read properties of null + expect(() => extractPREnrichment(pullRequest)).not.toThrow(); + const extracted = extractPREnrichment(pullRequest); + // null pageInfo is treated as "not truncated" → ciChecks should be defined + expect(extracted?.data.ciChecks).toBeDefined(); + expect(extracted?.data.ciChecks).toHaveLength(1); + }); +});