feat(scm-github): replace repo-scoped Guard 1 with PR-scoped ETag checks (D4)
Guard 1 now checks GET /repos/{owner}/{repo}/pulls/{number} per PR
instead of GET /repos/{owner}/{repo}/pulls?... per repo. This means:
- Only changed PRs flow into the GraphQL batch
- Unchanged PRs are served directly from the enrichment cache
- shouldRefreshPREnrichment returns a refresh plan (prsToRefresh +
cachedResults) instead of a boolean
When 1 of 10 PRs changes, the old guard refreshed all 10 via GraphQL.
Now only the 1 changed PR is fetched; the other 9 are served from cache
at zero GraphQL cost.
Trade-off: more REST guard calls (1 per PR instead of 1 per repo), but
304 responses cost zero rate limit points.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
adil
parent
48d74af00e
commit
25ae6013db
|
|
@ -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<string, string>; // Key: "owner/repo", Value: ETag
|
||||
prResource: LRUCache<string, string>; // Key: "owner/repo#number", Value: ETag
|
||||
commitStatus: LRUCache<string, string>; // 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<string, PREnrichmentData>;
|
||||
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<ETagGuardResult> {
|
||||
): Promise<PREnrichmentRefreshPlan> {
|
||||
const details: string[] = [];
|
||||
let shouldRefresh = false;
|
||||
const prsToRefresh: PRInfo[] = [];
|
||||
const cachedResults = new Map<string, PREnrichmentData>();
|
||||
|
||||
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<string, PRInfo[]>();
|
||||
|
||||
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<boolean> {
|
||||
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 };
|
||||
|
|
|
|||
|
|
@ -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
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in New Issue