diff --git a/packages/plugins/scm-github/src/graphql-batch.ts b/packages/plugins/scm-github/src/graphql-batch.ts index db4ed209c..a7165994d 100644 --- a/packages/plugins/scm-github/src/graphql-batch.ts +++ b/packages/plugins/scm-github/src/graphql-batch.ts @@ -42,7 +42,7 @@ export function setExecFileAsync(fn: typeof execFileAsync): void { * Configuration constants for cache sizing. * LRU cache automatically evicts oldest entries when these limits are reached. */ -const MAX_PR_LIST_ETAGS = 100; // Number of repos to cache +const MAX_PR_RESOURCE_ETAGS = 500; // Number of PRs to cache (per-PR resource ETags) const MAX_COMMIT_STATUS_ETAGS = 500; // Number of commits to cache const MAX_PR_METADATA = 200; // Number of PRs to cache full data @@ -51,11 +51,11 @@ const MAX_PR_METADATA = 200; // Number of PRs to cache full data * Used to avoid expensive GraphQL queries when nothing has changed. * * Keys: - * - PR list: "prList:{owner}/{repo}" - * - Commit status: "commit:{owner}/{repo}#{sha}" + * - PR resource: "owner/repo#number" + * - Commit status: "owner/repo#sha" */ interface ETagCache { - prList: LRUCache; // Key: "owner/repo", Value: ETag + prResource: LRUCache; // Key: "owner/repo#number", Value: ETag commitStatus: LRUCache; // Key: "owner/repo#sha", Value: ETag } @@ -67,15 +67,17 @@ interface ETagCache { * Uses LRU eviction to ensure bounded memory usage. */ const etagCache: ETagCache = { - prList: new LRUCache(MAX_PR_LIST_ETAGS), + prResource: new LRUCache(MAX_PR_RESOURCE_ETAGS), commitStatus: new LRUCache(MAX_COMMIT_STATUS_ETAGS), }; /** - * Result of checking if PR data has changed via ETag guards. + * Refresh plan returned by shouldRefreshPREnrichment. + * Tells enrichSessionsPRBatch which PRs need GraphQL and which are cached. */ -interface ETagGuardResult { - shouldRefresh: boolean; +interface PREnrichmentRefreshPlan { + prsToRefresh: PRInfo[]; + cachedResults: Map; details: string[]; } @@ -84,15 +86,15 @@ interface ETagGuardResult { * Useful for testing or when forcing a refresh. */ export function clearETagCache(): void { - etagCache.prList.clear(); + etagCache.prResource.clear(); etagCache.commitStatus.clear(); } /** - * Get PR list ETag for a repository. + * Get PR resource ETag for a specific PR. */ -export function getPRListETag(owner: string, repo: string): string | undefined { - return etagCache.prList.get(`${owner}/${repo}`); +export function getPRResourceETag(owner: string, repo: string, number: number): string | undefined { + return etagCache.prResource.get(`${owner}/${repo}#${number}`); } /** @@ -107,11 +109,11 @@ export function getCommitStatusETag( } /** - * Set PR list ETag for a repository. + * Set PR resource ETag for a specific PR. * Exported for testing. */ -export function setPRListETag(owner: string, repo: string, etag: string): void { - etagCache.prList.set(`${owner}/${repo}`, etag); +export function setPRResourceETag(owner: string, repo: string, number: number, etag: string): void { + etagCache.prResource.set(`${owner}/${repo}#${number}`, etag); } /** @@ -167,104 +169,70 @@ function updatePRMetadataCache( } /** - * 2-Guard ETag Strategy: Check if PR enrichment cache needs refreshing. + * 2-Guard ETag Strategy: Build a per-PR refresh plan. * - * Before running expensive GraphQL batch queries, use two lightweight REST API - * ETag checks to detect if anything actually changed: + * For each PR, runs two lightweight REST ETag checks to decide whether it + * needs a GraphQL refresh or can be served from cache: * - * Guard 1: PR List ETag Check (per repo) - * - Detects: New commits, PR title/body edits, labels changes, reviews, PR state changes - * - Misses: CI status changes + * Guard 1: PR Resource ETag Check (per PR) + * - Endpoint: GET /repos/{owner}/{repo}/pulls/{number} + * - Detects: New commits, PR title/body edits, label changes, reviews, state changes + * - 304 = unchanged, 200 = changed → refresh that PR * - * Guard 2: Commit Status ETag Check (per PR with cached metadata) - * - Checks ALL PRs with cached metadata and head SHA + * Guard 2: Commit Status ETag Check (per PR, only when Guard 1 returns 304) * - Detects: CI check starts, passes, fails, or external status updates - * - Critical for catching CI transitions (failing -> passing, passing -> failing, etc.) + * - 304 = unchanged → serve from cache, 200 = changed → refresh that PR * * @param prs - PRs to check - * @returns true if GraphQL batch should run, false if nothing changed + * @returns refresh plan: which PRs need GraphQL, which are cached */ export async function shouldRefreshPREnrichment( prs: PRInfo[], -): Promise { +): Promise { const details: string[] = []; - let shouldRefresh = false; + const prsToRefresh: PRInfo[] = []; + const cachedResults = new Map(); if (prs.length === 0) { - return { shouldRefresh: false, details: ["No PRs to check"] }; + return { prsToRefresh: [], cachedResults, details: ["No PRs to check"] }; } - // Group PRs by repository for Guard 1 (PR list check) - const repos = new Map(); - for (const pr of prs) { - const repoKey = `${pr.owner}/${pr.repo}`; - if (!repos.has(repoKey)) { - repos.set(repoKey, []); + const prKey = `${pr.owner}/${pr.repo}#${pr.number}`; + + // Guard 1: PR resource ETag check + const prChanged = await checkPRResourceETag(pr.owner, pr.repo, pr.number); + if (prChanged) { + prsToRefresh.push(pr); + details.push(`PR resource changed for ${prKey} (Guard 1)`); + continue; } - const repoPrs = repos.get(repoKey); - if (repoPrs) { - repoPrs.push(pr); + + // Guard 1 returned 304 — check if we have cached data to serve + const cachedEnrichment = prEnrichmentDataCache.get(prKey); + const cachedMeta = prMetadataCache.get(prKey); + + if (!cachedEnrichment || !cachedMeta || cachedMeta.headSha === null) { + prsToRefresh.push(pr); + details.push(`No cached data for ${prKey} (Guard 1: 304 but cache miss)`); + continue; + } + + // Guard 2: commit status ETag check + const statusChanged = await checkCommitStatusETag( + pr.owner, + pr.repo, + cachedMeta.headSha, + ); + if (statusChanged) { + prsToRefresh.push(pr); + details.push(`CI status changed for ${prKey} (Guard 2)`); + } else { + cachedResults.set(prKey, cachedEnrichment); } } - // Guard 1: Check PR list ETag for each repository - let guard1DetectedChanges = false; - for (const [repoKey] of repos) { - const [owner, repo] = repoKey.split("/"); - const prListChanged = await checkPRListETag(owner, repo); - if (prListChanged) { - guard1DetectedChanges = true; - shouldRefresh = true; - details.push(`PR list changed for ${repoKey} (Guard 1)`); - } - } - - // Guard 2: Check commit status ETag only when Guard 1 didn't detect changes - // We check ALL PRs (not just pending) to catch CI status transitions: - // - failing -> passing (PR becomes merge-ready) - // - passing -> failing (PR becomes unmergeable) - // - pending -> passing/failing (CI completes) - // - passing -> pending (new CI run starts) - // - // Guard 2 is only needed when Guard 1 returns 304 (no PR list changes). - // If Guard 1 detected changes, we're going to refresh all PRs anyway. - if (!guard1DetectedChanges) { - for (const pr of prs) { - const prKey = `${pr.owner}/${pr.repo}#${pr.number}`; - const cached = prMetadataCache.get(prKey); - - // Check for incomplete cache (cached but no headSha) - // This happens when PR was cached but headSha wasn't captured - // We need to refresh to get complete data including headSha - if (cached && cached.headSha === null) { - shouldRefresh = true; - details.push(`First time seeing PR #${pr.number} (Guard 2: no cached head SHA)`); - continue; - } - - // Only check commit status ETag if we have cached data with a non-null head SHA - if (!cached || !cached.headSha) { - // No cached metadata - skip Guard 2. Since Guard 1 didn't detect changes - // and we have no cached data, there's nothing to check. - continue; - } - - const statusChanged = await checkCommitStatusETag( - pr.owner, - pr.repo, - cached.headSha, - ); - if (statusChanged) { - shouldRefresh = true; - details.push( - `CI status changed for ${pr.owner}/${pr.repo}#${pr.number} (Guard 2)`, - ); - } - } - } - - return { shouldRefresh, details }; + return { prsToRefresh, cachedResults, details }; } /** @@ -353,25 +321,26 @@ function extractErrorOutput(err: unknown): string | null { } /** - * Guard 1: PR List ETag Check (per repo) + * Guard 1: PR Resource ETag Check (per PR) * - * Detects if PR metadata has changed in a repository using REST ETag. + * Detects if a specific PR has changed using REST ETag. * - * - Endpoint: GET /repos/{owner}/{repo}/pulls?state=open&sort=updated&direction=desc + * - Endpoint: GET /repos/{owner}/{repo}/pulls/{number} * - Detects: New commits, PR title/body edits, label changes, reviews, PR state changes * - Misses: CI status changes (handled by Guard 2) * - * @returns true if PR list has changed (200 OK), false if unchanged (304 Not Modified) + * @returns true if PR has changed (200 OK), false if unchanged (304 Not Modified) */ -async function checkPRListETag( +async function checkPRResourceETag( owner: string, repo: string, + number: number, ): Promise { - const repoKey = `${owner}/${repo}`; - const cachedETag = etagCache.prList.get(repoKey); + const prKey = `${owner}/${repo}#${number}`; + const cachedETag = etagCache.prResource.get(prKey); // Build gh CLI args for REST API call - const url = `repos/${repoKey}/pulls?state=open&sort=updated&direction=desc&per_page=1`; + const url = `repos/${owner}/${repo}/pulls/${number}`; const args = ["api", "--method", "GET", url, "-i"]; // -i includes headers // Add If-None-Match header if we have a cached ETag @@ -380,7 +349,7 @@ async function checkPRListETag( } try { - const output = await execGhAsync(args, 10_000, "gh.api.guard-pr-list"); + const output = await execGhAsync(args, 10_000, "gh.api.guard-pr-resource"); // Check for HTTP 304 Not Modified response if (is304(output)) { @@ -391,10 +360,10 @@ async function checkPRListETag( const etagMatch = output.match(/etag:\s*(.+)/i); if (etagMatch) { const newETag = etagMatch[1].trim(); - setPRListETag(owner, repo, newETag); + setPRResourceETag(owner, repo, number, newETag); } - // PR list changed - cost: 1 REST point + // PR resource changed - cost: 1 REST point return true; } catch (err) { // gh exits code 1 on 304 Not Modified — check stdout/stderr for the status line @@ -405,7 +374,7 @@ async function checkPRListETag( const errorMsg = err instanceof Error ? err.message : String(err); // eslint-disable-next-line no-console -- Observability logging for ETag errors - console.warn(`[ETag Guard 1] PR list check failed for ${repoKey}: ${errorMsg}`); + console.warn(`[ETag Guard 1] PR resource check failed for ${prKey}: ${errorMsg}`); return true; // Assume changed to be safe } } @@ -906,19 +875,17 @@ function extractPREnrichment( /** * Main batch enrichment function with 2-Guard ETag Strategy. * - * Before running expensive GraphQL batch queries, uses two lightweight REST API - * ETag checks to detect if anything actually changed: + * Uses per-PR ETag checks to build a selective refresh plan: * - * 1. Guard 1: PR List ETag Check (per repo) + * 1. Guard 1: PR Resource ETag Check (per PR) * - Detects PR metadata changes (commits, reviews, labels, state) - * - Cost: 1 REST point if changed, 0 if unchanged (304) + * - Cost: 0 if unchanged (304), 1 REST point if changed (200) * - * 2. Guard 2: Commit Status ETag Check (per PR with pending CI) - * - Detects CI status changes for PRs with pending CI - * - Cost: 1 REST point if changed, 0 if unchanged (304) + * 2. Guard 2: Commit Status ETag Check (per PR, only when Guard 1 returns 304) + * - Detects CI status changes + * - Cost: 0 if unchanged (304), 1 REST point if changed (200) * - * If guards indicate no changes, skips GraphQL entirely (saves ~50 points per batch). - * If any guard detects a change, runs GraphQL batch queries. + * Only changed PRs go into the GraphQL batch. Unchanged PRs are served from cache. * * Returns a Map keyed by "${owner}/${repo}#${number}" for efficient lookup. */ @@ -932,46 +899,31 @@ export async function enrichSessionsPRBatch( return result; } - // Step 1: Check if we need to refresh using 2-Guard ETag Strategy - const guardResult = await shouldRefreshPREnrichment(prs); + // Step 1: Build per-PR refresh plan using 2-Guard ETag Strategy + const plan = await shouldRefreshPREnrichment(prs); - if (!guardResult.shouldRefresh) { - // No changes detected - try to return cached data - // If any PRs are missing from cache, we need to fetch them via GraphQL - const missingPRs: PRInfo[] = []; - - for (const pr of prs) { - const prKey = `${pr.owner}/${pr.repo}#${pr.number}`; - const cachedData = prEnrichmentDataCache.get(prKey); - if (cachedData) { - result.set(prKey, cachedData); - } else { - missingPRs.push(pr); - } - } - - if (missingPRs.length === 0) { - // All PRs cached - return cached data - observer?.log( - "info", - `[ETag Guard] Skipping GraphQL batch - all ${result.size} PRs cached. Reasons: ${guardResult.details.join(", ")}`, - ); - return result; - } - - // Some PRs not cached - fetch missing PRs via GraphQL - observer?.log( - "info", - `[ETag Guard] Partial cache: ${result.size} cached, ${missingPRs.length} missing. Fetching missing PRs via GraphQL.`, - ); - prs = missingPRs; // Update to only fetch missing PRs - // Continue to GraphQL batch processing below + // Seed result with cached data from the plan + for (const [key, data] of plan.cachedResults) { + result.set(key, data); } - // Step 2: Split into batches if we have too many PRs + if (plan.prsToRefresh.length === 0) { + observer?.log( + "info", + `[ETag Guard] Skipping GraphQL batch - all ${result.size} PRs cached. Reasons: ${plan.details.join(", ")}`, + ); + return result; + } + + observer?.log( + "info", + `[ETag Guard] ${result.size} cached, ${plan.prsToRefresh.length} need refresh. Reasons: ${plan.details.join(", ")}`, + ); + + // Step 2: Split prsToRefresh into batches const batches: PRInfo[][] = []; - for (let i = 0; i < prs.length; i += MAX_BATCH_SIZE) { - batches.push(prs.slice(i, i + MAX_BATCH_SIZE)); + for (let i = 0; i < plan.prsToRefresh.length; i += MAX_BATCH_SIZE) { + batches.push(plan.prsToRefresh.slice(i, i + MAX_BATCH_SIZE)); } // Step 3: Execute each batch @@ -1058,11 +1010,11 @@ export { parseReviewDecision, parsePRState, extractPREnrichment, - checkPRListETag, + checkPRResourceETag, checkCommitStatusETag, // shouldRefreshPREnrichment is already exported as async function updatePRMetadataCache, }; // Export types for testing -export type { ETagCache, ETagGuardResult }; +export type { ETagCache, PREnrichmentRefreshPlan }; diff --git a/packages/plugins/scm-github/test/graphql-batch.test.ts b/packages/plugins/scm-github/test/graphql-batch.test.ts index cdab3eea2..d845b8257 100644 --- a/packages/plugins/scm-github/test/graphql-batch.test.ts +++ b/packages/plugins/scm-github/test/graphql-batch.test.ts @@ -18,16 +18,18 @@ import { parsePRState, extractPREnrichment, clearETagCache, - getPRListETag, + getPRResourceETag, getCommitStatusETag, - setPRListETag, + setPRResourceETag, setCommitStatusETag, setPRMetadata, getPRMetadataCache, clearPRMetadataCache, shouldRefreshPREnrichment, + updatePRMetadataCache, setExecFileAsync, } from "../src/graphql-batch.js"; +import type { PREnrichmentData } from "@aoagents/ao-core"; // Mock execFile using the injection function // Create a mock function that returns a promise matching the execFile signature @@ -733,11 +735,11 @@ describe("ETag Cache", () => { }); describe("ETag Cache Getters (Testing Exposed APIs)", () => { - it("should return undefined for PR list ETag not in cache", () => { + it("should return undefined for PR resource ETag not in cache", () => { const owner = "testowner"; const repo = "testrepo"; - expect(getPRListETag(owner, repo)).toBeUndefined(); + expect(getPRResourceETag(owner, repo, 123)).toBeUndefined(); }); it("should return undefined for commit status ETag not in cache", () => { @@ -811,40 +813,49 @@ describe("ETag Cache", () => { }); describe("shouldRefreshPREnrichment - ETag Guard Strategy", () => { + // Shared test enrichment data for populating caches + const testEnrichment: PREnrichmentData = { + state: "open", + ciStatus: "passing", + reviewDecision: "none", + mergeable: true, + hasConflicts: false, + isBehind: false, + blockers: [], + }; + + const makePR = (owner: string, repo: string, number: number) => ({ + owner, + repo, + number, + url: `https://github.com/${owner}/${repo}/pull/${number}`, + title: `Test PR ${number}`, + branch: `feature-${number}`, + baseBranch: "main", + isDraft: false, + }); + beforeEach(() => { clearETagCache(); clearPRMetadataCache(); - // Don't clear all mocks - reset only our mock call counts mockExecFileImpl.mockClear(); }); describe("Empty PRs", () => { - it("should not refresh when no PRs provided", async () => { + it("should return empty plan when no PRs provided", async () => { const result = await shouldRefreshPREnrichment([]); - expect(result.shouldRefresh).toBe(false); + expect(result.prsToRefresh).toHaveLength(0); + expect(result.cachedResults.size).toBe(0); expect(result.details).toContain("No PRs to check"); - // Should not make any API calls expect(mockExecFileImpl).not.toHaveBeenCalled(); }); }); - describe("Guard 1: PR List ETag - First-time PR (no cache)", () => { - it("should refresh when PR list ETag check returns 200 (first time)", async () => { - const prs = [ - { - owner: "owner", - repo: "repo", - number: 123, - url: "https://github.com/owner/repo/pull/123", - title: "Test PR", - branch: "feature", - baseBranch: "main", - isDraft: false, - }, - ]; + describe("Guard 1: PR Resource ETag - First-time PR (no cache)", () => { + it("should refresh when PR resource ETag returns 200 (first time)", async () => { + const prs = [makePR("owner", "repo", 123)]; - // Mock gh CLI response for PR list check (200 OK, not 304) mockExecFileImpl.mockResolvedValueOnce({ stdout: 'HTTP/2 200\neTag: "abc123"', stderr: "", @@ -852,323 +863,182 @@ describe("shouldRefreshPREnrichment - ETag Guard Strategy", () => { const result = await shouldRefreshPREnrichment(prs); - expect(result.shouldRefresh).toBe(true); - expect(result.details).toContain("PR list changed for owner/repo (Guard 1)"); + expect(result.prsToRefresh).toHaveLength(1); + expect(result.prsToRefresh[0].number).toBe(123); + expect(result.cachedResults.size).toBe(0); + expect(result.details).toContain("PR resource changed for owner/repo#123 (Guard 1)"); expect(mockExecFileImpl).toHaveBeenCalledTimes(1); }); - it("should not refresh when PR list ETag returns 304 Not Modified", async () => { - const prs = [ - { - owner: "owner", - repo: "repo", - number: 123, - url: "https://github.com/owner/repo/pull/123", - title: "Test PR", - branch: "feature", - baseBranch: "main", - isDraft: false, - }, - ]; + it("should serve from cache when Guard 1 returns 304 and cache is populated", async () => { + const prs = [makePR("owner", "repo", 123)]; - // Mock gh CLI response for PR list check (304 Not Modified) - mockExecFileImpl.mockResolvedValueOnce({ - stdout: 'HTTP/2 304', - stderr: "", - }); + // Populate both metadata and enrichment caches + updatePRMetadataCache("owner/repo#123", testEnrichment, "abc123"); + + // Guard 1: 304, Guard 2: 304 + mockExecFileImpl + .mockResolvedValueOnce({ stdout: "HTTP/2 304", stderr: "" }) + .mockResolvedValueOnce({ stdout: "HTTP/2 304", stderr: "" }); const result = await shouldRefreshPREnrichment(prs); - expect(result.shouldRefresh).toBe(false); - // When no changes detected, details may be empty (only changes add to details) - expect(result.details).toHaveLength(0); + expect(result.prsToRefresh).toHaveLength(0); + expect(result.cachedResults.size).toBe(1); + expect(result.cachedResults.has("owner/repo#123")).toBe(true); + expect(mockExecFileImpl).toHaveBeenCalledTimes(2); // Guard 1 + Guard 2 + }); + + it("should refresh when Guard 1 returns 304 but no enrichment cache exists", async () => { + const prs = [makePR("owner", "repo", 123)]; + + // Only metadata cache, no enrichment cache + setPRMetadata("owner/repo#123", { headSha: "abc123", ciStatus: "passing" as const }); + + mockExecFileImpl.mockResolvedValueOnce({ stdout: "HTTP/2 304", stderr: "" }); + + const result = await shouldRefreshPREnrichment(prs); + + expect(result.prsToRefresh).toHaveLength(1); + expect(result.details).toContain("No cached data for owner/repo#123 (Guard 1: 304 but cache miss)"); + expect(mockExecFileImpl).toHaveBeenCalledTimes(1); // Only Guard 1, no Guard 2 }); it("should refresh on error and log warning", async () => { - const prs = [ - { - owner: "owner", - repo: "repo", - number: 123, - url: "https://github.com/owner/repo/pull/123", - title: "Test PR", - branch: "feature", - baseBranch: "main", - isDraft: false, - }, - ]; + const prs = [makePR("owner", "repo", 123)]; - // Mock gh CLI error const consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); mockExecFileImpl.mockRejectedValueOnce(new Error("gh CLI failed")); const result = await shouldRefreshPREnrichment(prs); - expect(result.shouldRefresh).toBe(true); // Fail-safe: assume changed on error + expect(result.prsToRefresh).toHaveLength(1); expect(consoleWarnSpy).toHaveBeenCalled(); consoleWarnSpy.mockRestore(); }); }); - describe("Guard 2: Commit Status ETag - Pending CI PRs", () => { - it("should refresh when commit status ETag check returns 200", async () => { - // Set up cached PR with pending CI - setPRMetadata("owner/repo#123", { headSha: "abc123", ciStatus: "pending" }); + describe("Guard 2: Commit Status ETag", () => { + it("should refresh when commit status ETag returns 200", async () => { + updatePRMetadataCache("owner/repo#123", testEnrichment, "abc123"); + const prs = [makePR("owner", "repo", 123)]; - const prs = [ - { - owner: "owner", - repo: "repo", - number: 123, - url: "https://github.com/owner/repo/pull/123", - title: "Test PR", - branch: "feature", - baseBranch: "main", - isDraft: false, - }, - ]; - - // Mock: Guard 1 returns 304 (no change), Guard 2 returns 200 (CI changed) + // Guard 1: 304, Guard 2: 200 mockExecFileImpl - .mockResolvedValueOnce({ - stdout: 'HTTP/2 304', // Guard 1: no change - stderr: "", - }) - .mockResolvedValueOnce({ - stdout: 'HTTP/2 200\neTag: "xyz789"', // Guard 2: CI changed - stderr: "", - }); + .mockResolvedValueOnce({ stdout: "HTTP/2 304", stderr: "" }) + .mockResolvedValueOnce({ stdout: 'HTTP/2 200\neTag: "xyz789"', stderr: "" }); const result = await shouldRefreshPREnrichment(prs); - expect(result.shouldRefresh).toBe(true); + expect(result.prsToRefresh).toHaveLength(1); + expect(result.cachedResults.size).toBe(0); expect(result.details).toContain("CI status changed for owner/repo#123 (Guard 2)"); expect(mockExecFileImpl).toHaveBeenCalledTimes(2); }); - it("should not refresh when commit status ETag returns 304", async () => { - // Set up cached PR with pending CI - setPRMetadata("owner/repo#123", { headSha: "abc123", ciStatus: "pending" }); + it("should serve from cache when both guards return 304", async () => { + updatePRMetadataCache("owner/repo#123", testEnrichment, "abc123"); + const prs = [makePR("owner", "repo", 123)]; - const prs = [ - { - owner: "owner", - repo: "repo", - number: 123, - url: "https://github.com/owner/repo/pull/123", - title: "Test PR", - branch: "feature", - baseBranch: "main", - isDraft: false, - }, - ]; - - // Mock both guards return 304 (no change) mockExecFileImpl - .mockResolvedValueOnce({ - stdout: 'HTTP/2 304', - stderr: "", - }) - .mockResolvedValueOnce({ - stdout: 'HTTP/2 304', - stderr: "", - }); + .mockResolvedValueOnce({ stdout: "HTTP/2 304", stderr: "" }) + .mockResolvedValueOnce({ stdout: "HTTP/2 304", stderr: "" }); const result = await shouldRefreshPREnrichment(prs); - expect(result.shouldRefresh).toBe(false); + expect(result.prsToRefresh).toHaveLength(0); + expect(result.cachedResults.size).toBe(1); expect(mockExecFileImpl).toHaveBeenCalledTimes(2); }); - it("should check Guard 2 for ALL PRs with cached metadata", async () => { - // Set up cached PR with passing CI (not pending) - still should be checked - setPRMetadata("owner/repo#123", { headSha: "abc123", ciStatus: "passing" }); + it("should refresh when PR has null headSha (incomplete cache)", async () => { + // headSha null → cache miss even though metadata exists + setPRMetadata("owner/repo#123", { headSha: null, ciStatus: "pending" as const }); + const prs = [makePR("owner", "repo", 123)]; - const prs = [ - { - owner: "owner", - repo: "repo", - number: 123, - url: "https://github.com/owner/repo/pull/123", - title: "Test PR", - branch: "feature", - baseBranch: "main", - isDraft: false, - }, - ]; - - // Mock both guards return 304 (no change) - mockExecFileImpl - .mockResolvedValueOnce({ - stdout: 'HTTP/2 304', - stderr: "", - }) - .mockResolvedValueOnce({ - stdout: 'HTTP/2 304', - stderr: "", - }); + mockExecFileImpl.mockResolvedValueOnce({ stdout: "HTTP/2 304", stderr: "" }); const result = await shouldRefreshPREnrichment(prs); - expect(result.shouldRefresh).toBe(false); - expect(mockExecFileImpl).toHaveBeenCalledTimes(2); // Guard 1 + Guard 2 called - }); - - it("should refresh when PR has no cached head SHA", async () => { - // Set up cached PR with null head SHA (incomplete cache) - setPRMetadata("owner/repo#123", { headSha: null, ciStatus: "pending" }); - - const prs = [ - { - owner: "owner", - repo: "repo", - number: 123, - url: "https://github.com/owner/repo/pull/123", - title: "Test PR", - branch: "feature", - baseBranch: "main", - isDraft: false, - }, - ]; - - // Mock Guard 1 (PR list check) - mockExecFileImpl.mockResolvedValueOnce({ - stdout: 'HTTP/2 304', - stderr: "", - }); - - const result = await shouldRefreshPREnrichment(prs); - - expect(result.shouldRefresh).toBe(true); - expect(result.details).toContain("First time seeing PR #123 (Guard 2: no cached head SHA)"); - // Guard 1 called for PR list, Guard 2 skipped (no head SHA to check) + expect(result.prsToRefresh).toHaveLength(1); + expect(result.details).toContain("No cached data for owner/repo#123 (Guard 1: 304 but cache miss)"); expect(mockExecFileImpl).toHaveBeenCalledTimes(1); }); }); - describe("Multiple Repositories", () => { - it("should check PR list ETag for each repository", async () => { - const prs = [ - { - owner: "owner1", - repo: "repo1", - number: 1, - url: "https://github.com/owner1/repo1/pull/1", - title: "Test PR 1", - branch: "feature1", - baseBranch: "main", - isDraft: false, - }, - { - owner: "owner2", - repo: "repo2", - number: 2, - url: "https://github.com/owner2/repo2/pull/2", - title: "Test PR 2", - branch: "feature2", - baseBranch: "main", - isDraft: false, - }, - ]; + describe("Multiple PRs", () => { + it("should check PR resource ETag for each PR individually", async () => { + const prs = [makePR("owner1", "repo1", 1), makePR("owner2", "repo2", 2)]; - // With new behavior: ALL PRs with cached metadata get Guard 2 checked - // Add metadata for both PRs to validate Guard 2 is called - setPRMetadata("owner1/repo1#1", { headSha: "sha1", ciStatus: "passing" }); - setPRMetadata("owner2/repo2#2", { headSha: "sha2", ciStatus: "pending" }); - - // Both repos changed - Guard 1 calls + // Both PRs changed mockExecFileImpl - .mockResolvedValueOnce({ - stdout: 'HTTP/2 200', - stderr: "", - }) - .mockResolvedValueOnce({ - stdout: 'HTTP/2 200', - stderr: "", - }); + .mockResolvedValueOnce({ stdout: "HTTP/2 200", stderr: "" }) + .mockResolvedValueOnce({ stdout: "HTTP/2 200", stderr: "" }); const result = await shouldRefreshPREnrichment(prs); - expect(result.shouldRefresh).toBe(true); - // Guard 1 adds 2 details (one per repo), Guard 2 skipped (304 = no change, no detail) - expect(result.details).toHaveLength(2); - expect(result.details).toContain("PR list changed for owner1/repo1 (Guard 1)"); - expect(result.details).toContain("PR list changed for owner2/repo2 (Guard 1)"); - // 2 Guard 1 calls only (Guard 2 calls return 304, no details added) + expect(result.prsToRefresh).toHaveLength(2); + expect(result.details).toContain("PR resource changed for owner1/repo1#1 (Guard 1)"); + expect(result.details).toContain("PR resource changed for owner2/repo2#2 (Guard 1)"); expect(mockExecFileImpl).toHaveBeenCalledTimes(2); }); + + it("should refresh only the changed PR when one returns 200 and another 304", async () => { + // Both PRs have cached enrichment + updatePRMetadataCache("owner/repo#1", testEnrichment, "sha1"); + updatePRMetadataCache("owner/repo#2", testEnrichment, "sha2"); + setCommitStatusETag("owner", "repo", "sha2", "commit-etag-2"); + + const prs = [makePR("owner", "repo", 1), makePR("owner", "repo", 2)]; + + mockExecFileImpl + .mockResolvedValueOnce({ stdout: 'HTTP/2 200\netag: "new"', stderr: "" }) // Guard 1 PR#1: changed + .mockResolvedValueOnce({ stdout: "HTTP/2 304", stderr: "" }) // Guard 1 PR#2: unchanged + .mockResolvedValueOnce({ stdout: "HTTP/2 304", stderr: "" }); // Guard 2 PR#2: CI unchanged + + const result = await shouldRefreshPREnrichment(prs); + + expect(result.prsToRefresh).toHaveLength(1); + expect(result.prsToRefresh[0].number).toBe(1); + expect(result.cachedResults.size).toBe(1); + expect(result.cachedResults.has("owner/repo#2")).toBe(true); + }); }); describe("If-None-Match Header", () => { it("should send If-None-Match header with cached ETag", async () => { - const prs = [ - { - owner: "owner", - repo: "repo", - number: 123, - url: "https://github.com/owner/repo/pull/123", - title: "Test PR", - branch: "feature", - baseBranch: "main", - isDraft: false, - }, - ]; + const prs = [makePR("owner", "repo", 123)]; - // First call - cache miss, returns new ETag + // First call — cache miss, returns new ETag mockExecFileImpl.mockResolvedValueOnce({ stdout: 'HTTP/2 200\netag: "cached-etag"', stderr: "", }); const result1 = await shouldRefreshPREnrichment(prs); - expect(result1.shouldRefresh).toBe(true); + expect(result1.prsToRefresh).toHaveLength(1); - // Cache metadata and ETags after first poll so Guard 2 has data for second poll - setPRMetadata("owner/repo#123", { headSha: "abc123", ciStatus: "passing" }); - setPRListETag("owner", "repo", "cached-etag"); + // Populate caches after first poll + updatePRMetadataCache("owner/repo#123", testEnrichment, "abc123"); + setPRResourceETag("owner", "repo", 123, "cached-etag"); setCommitStatusETag("owner", "repo", "abc123", "commit-status-etag"); - // Second call - should use cached ETag in If-None-Match headers (Guard 1 + Guard 2) + // Second call — should use cached ETag in If-None-Match mockExecFileImpl - .mockResolvedValueOnce({ - stdout: 'HTTP/2 304', - stderr: "", - }) - .mockResolvedValueOnce({ - stdout: 'HTTP/2 304', - stderr: "", - }); + .mockResolvedValueOnce({ stdout: "HTTP/2 304", stderr: "" }) + .mockResolvedValueOnce({ stdout: "HTTP/2 304", stderr: "" }); const result2 = await shouldRefreshPREnrichment(prs); - expect(result2.shouldRefresh).toBe(false); - - // Cache metadata after first poll so Guard 2 has data for second poll - setPRMetadata("owner/repo#123", { headSha: "abc123", ciStatus: "passing" }); - - // Second poll - should use cached ETag in If-None-Match headers (Guard 1 + Guard 2) - mockExecFileImpl - .mockResolvedValueOnce({ - stdout: 'HTTP/2 304', - stderr: "", - }) - .mockResolvedValueOnce({ - stdout: 'HTTP/2 304', - stderr: "", - }); - - const result3 = await shouldRefreshPREnrichment(prs); - expect(result3.shouldRefresh).toBe(false); + expect(result2.prsToRefresh).toHaveLength(0); + expect(result2.cachedResults.size).toBe(1); // Verify the second poll included If-None-Match headers - // Get all call arguments and find those with -H flag const allCalls = mockExecFileImpl.mock.calls; - // Second poll has 2 calls: Guard 1 (index 1) and Guard 2 (index 2) const secondPollCalls = allCalls.slice(1, 3); - // Mock call format: [file, args, options], so check call[1] for args const callsWithHeader = secondPollCalls.filter((call) => Array.isArray(call) && call[1] && call[1].includes("-H") ); - expect(callsWithHeader).toHaveLength(2); // Both Guard 1 and Guard 2 + expect(callsWithHeader).toHaveLength(2); // Guard 1 + Guard 2 }); }); });