From 721126d54d67191561ed3fe0f6534a939e4a2de4 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 4 Jul 2026 01:24:10 -0700 Subject: [PATCH 1/2] feat(review): add durable webhook-invalidated CI-state snapshot cache check_run/check_suite completions currently force a fresh REST/GraphQL CI read on every readiness check and disposition pass, even when the same (repo, PR, head_sha) was already read moments earlier in the same or a prior job. Extend the existing pull_request_detail_sync_state cache (#2537) with CI-state columns: a fresh row (TTL-capped, invalidated on check_run/check_suite completed) is served without a GitHub call; a miss falls through to a live fetch and write-throughs the result. The act-boundary re-check (refreshLiveCiAggregate) always forces a live read regardless of cache state, preserving its existing always-fresh contract. --- .../0108_pull_request_ci_state_cache.sql | 29 ++ src/db/repositories.ts | 37 ++ src/db/schema.ts | 21 ++ src/github/backfill.ts | 111 ++++++ src/queue/processors.ts | 79 +++- src/selfhost/metrics.ts | 1 + src/types.ts | 15 + test/unit/pr-detail-durable-cache.test.ts | 336 ++++++++++++++++-- test/unit/queue.test.ts | 227 ++++++++++++ 9 files changed, 823 insertions(+), 33 deletions(-) create mode 100644 migrations/0108_pull_request_ci_state_cache.sql diff --git a/migrations/0108_pull_request_ci_state_cache.sql b/migrations/0108_pull_request_ci_state_cache.sql new file mode 100644 index 0000000000..9f28c8311b --- /dev/null +++ b/migrations/0108_pull_request_ci_state_cache.sql @@ -0,0 +1,29 @@ +ALTER TABLE pull_request_detail_sync_state + ADD COLUMN ci_head_sha TEXT; + +ALTER TABLE pull_request_detail_sync_state + ADD COLUMN ci_state TEXT; + +ALTER TABLE pull_request_detail_sync_state + ADD COLUMN ci_has_pending INTEGER; + +ALTER TABLE pull_request_detail_sync_state + ADD COLUMN ci_has_visible_pending INTEGER; + +ALTER TABLE pull_request_detail_sync_state + ADD COLUMN ci_has_missing_required_context INTEGER; + +ALTER TABLE pull_request_detail_sync_state + ADD COLUMN ci_failing_details_json TEXT; + +ALTER TABLE pull_request_detail_sync_state + ADD COLUMN ci_non_required_failing_details_json TEXT; + +ALTER TABLE pull_request_detail_sync_state + ADD COLUMN ci_completeness_warning TEXT; + +ALTER TABLE pull_request_detail_sync_state + ADD COLUMN ci_required_contexts_key TEXT; + +ALTER TABLE pull_request_detail_sync_state + ADD COLUMN ci_state_fetched_at TEXT; diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 5ced8cab60..025334473e 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -1262,6 +1262,16 @@ export async function upsertPullRequestDetailSyncState(env: Env, state: PullRequ prMergeableState: state.prMergeableState, prState: state.prState, prStateFetchedAt: state.prStateFetchedAt, + ciHeadSha: state.ciHeadSha, + ciState: state.ciState, + ciHasPending: state.ciHasPending, + ciHasVisiblePending: state.ciHasVisiblePending, + ciHasMissingRequiredContext: state.ciHasMissingRequiredContext, + ciFailingDetailsJson: state.ciFailingDetailsJson, + ciNonRequiredFailingDetailsJson: state.ciNonRequiredFailingDetailsJson, + ciCompletenessWarning: state.ciCompletenessWarning, + ciRequiredContextsKey: state.ciRequiredContextsKey, + ciStateFetchedAt: state.ciStateFetchedAt, updatedAt: nowIso(), }) .onConflictDoUpdate({ @@ -1278,6 +1288,16 @@ export async function upsertPullRequestDetailSyncState(env: Env, state: PullRequ prMergeableState: state.prMergeableState, prState: state.prState, prStateFetchedAt: state.prStateFetchedAt, + ciHeadSha: state.ciHeadSha, + ciState: state.ciState, + ciHasPending: state.ciHasPending, + ciHasVisiblePending: state.ciHasVisiblePending, + ciHasMissingRequiredContext: state.ciHasMissingRequiredContext, + ciFailingDetailsJson: state.ciFailingDetailsJson, + ciNonRequiredFailingDetailsJson: state.ciNonRequiredFailingDetailsJson, + ciCompletenessWarning: state.ciCompletenessWarning, + ciRequiredContextsKey: state.ciRequiredContextsKey, + ciStateFetchedAt: state.ciStateFetchedAt, updatedAt: nowIso(), }, }); @@ -4703,6 +4723,16 @@ function toPullRequestDetailSyncStateRecord(row: typeof pullRequestDetailSyncSta prMergeableState: row.prMergeableState, prState: row.prState, prStateFetchedAt: row.prStateFetchedAt, + ciHeadSha: row.ciHeadSha, + ciState: parseCiState(row.ciState), + ciHasPending: row.ciHasPending, + ciHasVisiblePending: row.ciHasVisiblePending, + ciHasMissingRequiredContext: row.ciHasMissingRequiredContext, + ciFailingDetailsJson: row.ciFailingDetailsJson, + ciNonRequiredFailingDetailsJson: row.ciNonRequiredFailingDetailsJson, + ciCompletenessWarning: row.ciCompletenessWarning, + ciRequiredContextsKey: row.ciRequiredContextsKey, + ciStateFetchedAt: row.ciStateFetchedAt, updatedAt: row.updatedAt, }; } @@ -6353,6 +6383,13 @@ function parsePullRequestDetailSyncStatus(value: string): PullRequestDetailSyncS return "never_synced"; } +// Unlike parsePullRequestDetailSyncStatus above, `ci_state` has no sensible non-null default -- absent/invalid +// genuinely means "never cached", so this returns null rather than coercing to a fake status. +function parseCiState(value: string | null): PullRequestDetailSyncStateRecord["ciState"] { + if (value === "passed" || value === "failed" || value === "pending" || value === "unverified") return value; + return null; +} + function loginMatches(column: unknown, login: string) { return sql`lower(${column}) = ${login.toLowerCase()}`; } diff --git a/src/db/schema.ts b/src/db/schema.ts index 3962eff0ce..a47cb7cc39 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -291,6 +291,27 @@ export const pullRequestDetailSyncState = sqliteTable( prMergeableState: text("pr_mergeable_state"), prState: text("pr_state"), prStateFetchedAt: text("pr_state_fetched_at"), + // Durable CI-state snapshot cache (#selfhost-installation-concurrency's sibling feature): mirrors the + // reduced LiveCiAggregate the gate's own live-CI fetch already produces, so a second job/webhook-delivery + // within the TTL can skip re-fetching check-runs/status/check-suites from GitHub entirely. ciHeadSha is a + // SEPARATE column from the files-cache's own `headSha` above -- reusing that column here would entangle two + // independent cache lifecycles on one field (the exact "field A fresh, field B never fetched" bug class the + // prMergeableState/prState/prStateFetchedAt trio above already exists to avoid). ciRequiredContextsKey stores + // the same stable, order-independent fragment of settings.expectedCiContexts the request-scoped LiveGithubFacts + // memo already keys on, so a maintainer's config change invalidates this cache even when head_sha hasn't + // moved. NEVER read by the act-boundary merge/close decision (services/agent-approval-queue.ts, + // services/agent-action-executor.ts) -- both intentionally force a live read immediately before acting, same + // as the PR-state trio above. + ciHeadSha: text("ci_head_sha"), + ciState: text("ci_state"), + ciHasPending: integer("ci_has_pending", { mode: "boolean" }), + ciHasVisiblePending: integer("ci_has_visible_pending", { mode: "boolean" }), + ciHasMissingRequiredContext: integer("ci_has_missing_required_context", { mode: "boolean" }), + ciFailingDetailsJson: text("ci_failing_details_json"), + ciNonRequiredFailingDetailsJson: text("ci_non_required_failing_details_json"), + ciCompletenessWarning: text("ci_completeness_warning"), + ciRequiredContextsKey: text("ci_required_contexts_key"), + ciStateFetchedAt: text("ci_state_fetched_at"), updatedAt: text("updated_at").notNull().$defaultFn(() => nowIso()), }, (table) => ({ diff --git a/src/github/backfill.ts b/src/github/backfill.ts index 9cc0bbf1f7..d30e5bdf4b 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -342,6 +342,14 @@ const PR_STATE_CACHE_METRIC = "gittensory_pr_state_cache_total"; // Safety-net max age for a webhook-invalidated PR-state cache row (a dropped/missed webhook must not pin a stale // value forever). Short enough that a missed synchronize/closed/reopened event self-heals within one sweep tick. const PR_STATE_CACHE_MAX_AGE_MS = 5 * 60 * 1000; +// #selfhost-ci-verification: durable-cache counter for the CI-state snapshot cache, sibling to PR_STATE_CACHE_METRIC. +// Exported: the cache-check/hit/miss orchestration lives in queue/processors.ts (see writeThroughCiStateCache's +// own doc comment for why), which needs this same metric name. +export const CI_STATE_CACHE_METRIC = "gittensory_ci_state_cache_total"; +// Shorter than PR_STATE_CACHE_MAX_AGE_MS (5min): CI state changes faster and more consequentially than bare PR +// state, and check_run/check_suite `completed` webhooks already invalidate this cache explicitly (see +// invalidateCiStateCache below) -- this is purely the backstop for a delayed/missed webhook delivery. +export const CI_STATE_CACHE_MAX_AGE_MS = 60 * 1000; const CURRENT_OPEN_SCAN_MARKER = "gittensory-current-open-scan-v1"; const FRESH_TOTALS_SNAPSHOT_MS = 10 * 60 * 1000; const TOTALS_SNAPSHOT_LOOKBACK = 8; @@ -3279,6 +3287,109 @@ export async function invalidatePrStateCache(env: Env, repoFullName: string, pul }); } +// #selfhost-ci-verification: durable, webhook-invalidated cache for the CI-state aggregate (fetchLiveCiAggregate/ +// fetchLiveCiAggregateViaGraphQl), sibling to the #2537 PR-state cache above. Unlike the request-local +// LiveGithubFacts memo (queue/processors.ts), this survives ACROSS webhook deliveries / job re-checks, cutting +// repeat check-runs/status/check-suites reads for an unchanged (repo, pr, head_sha, expectedCiContexts) tuple. +// NEVER used by the act-boundary merge/close decision (services/agent-approval-queue.ts, +// services/agent-action-executor.ts) -- those call fetchLiveCiAggregate/fetchLiveCiAggregatePreferGraphQl +// directly, by design, and must keep doing so (this cache is a distinct, separately-exported function these two +// call sites simply never import). +export function isCiStateCacheFresh( + cached: Pick | null | undefined, + headSha: string | null | undefined, + requiredContextsKey: string, +): boolean { + if (!cached?.ciStateFetchedAt) return false; + const fetchedAtMs = Date.parse(cached.ciStateFetchedAt); + if (!Number.isFinite(fetchedAtMs)) return false; + if (Date.now() - fetchedAtMs >= CI_STATE_CACHE_MAX_AGE_MS) return false; + // A stale head_sha (a new commit since this row was cached) or a changed expectedCiContexts config is an + // automatic miss regardless of TTL -- mirrors the files-cache's own headSha-matching discipline. + if ((cached.ciHeadSha ?? null) !== (headSha ?? null)) return false; + if ((cached.ciRequiredContextsKey ?? "") !== requiredContextsKey) return false; + return true; +} + +/** Reconstruct a LiveCiAggregate from a cached row, or null on any parse failure / missing ciState (fail-open: + * the caller treats null as a cache miss and falls through to a live fetch, never throws). */ +export function deserializeCachedCiAggregate( + cached: Pick< + PullRequestDetailSyncStateRecord, + "ciState" | "ciHasPending" | "ciHasVisiblePending" | "ciHasMissingRequiredContext" | "ciFailingDetailsJson" | "ciNonRequiredFailingDetailsJson" | "ciCompletenessWarning" + >, +): LiveCiAggregate | null { + if (!cached.ciState) return null; + try { + return { + ciState: cached.ciState, + hasPending: cached.ciHasPending ?? false, + hasVisiblePending: cached.ciHasVisiblePending ?? false, + hasMissingRequiredContext: cached.ciHasMissingRequiredContext ?? false, + failingDetails: JSON.parse(cached.ciFailingDetailsJson ?? "[]"), + nonRequiredFailingDetails: JSON.parse(cached.ciNonRequiredFailingDetailsJson ?? "[]"), + ciCompletenessWarning: cached.ciCompletenessWarning ?? null, + }; + } catch { + return null; + } +} + +/** Best-effort write-through for the CI-state cache fields (mirrors writeThroughPrStateCache's fail-open, + * preserve-status contract). Always stamps ciStateFetchedAt = now on a successful live read. + * + * Exported (not orchestrated in this module) because the actual cache-check-then-live-fetch-then-write-through + * sequence lives in queue/processors.ts's cachedFetchLiveCiAggregate, alongside cachedLiveCiAggregate/ + * refreshLiveCiAggregate -- NOT here, even though this is the natural file for #2537's PR-state cache sibling. + * A same-module call from THIS file to fetchLiveCiAggregatePreferGraphQl (below) would be invisible to + * `vi.spyOn(backfillModule, "fetchLiveCiAggregatePreferGraphQl")`, which many existing tests already rely on to + * intercept the cross-module call processors.ts has always made -- moving the orchestration there preserves + * that exact call shape. */ +export async function writeThroughCiStateCache( + env: Env, + repoFullName: string, + prNumber: number, + previousState: Pick | null | undefined, + headSha: string | null | undefined, + requiredContextsKey: string, + aggregate: LiveCiAggregate, +): Promise { + incr(CI_STATE_CACHE_METRIC, { field: "write", result: "set" }); + await upsertPullRequestDetailSyncState(env, { + repoFullName, + pullNumber: prNumber, + status: previousState?.status ?? "never_synced", + ciHeadSha: headSha ?? null, + ciState: aggregate.ciState, + ciHasPending: aggregate.hasPending, + ciHasVisiblePending: aggregate.hasVisiblePending, + ciHasMissingRequiredContext: aggregate.hasMissingRequiredContext, + ciFailingDetailsJson: JSON.stringify(aggregate.failingDetails), + ciNonRequiredFailingDetailsJson: JSON.stringify(aggregate.nonRequiredFailingDetails), + ciCompletenessWarning: aggregate.ciCompletenessWarning, + ciRequiredContextsKey: requiredContextsKey, + ciStateFetchedAt: nowIso(), + }).catch(() => undefined); +} + +/** Invalidate the durable CI-state cache (mirrors invalidatePrStateCache) -- called from + * maybeReReviewOnCiCompletion on every check_run/check_suite `completed` webhook, best-effort. Explicit null + * (not omitted) so the PARTIAL-UPDATE CONTRACT actually clears the stale value rather than leaving it. */ +export async function invalidateCiStateCache( + env: Env, + repoFullName: string, + prNumber: number, +): Promise { + const existing = await getPullRequestDetailSyncState(env, repoFullName, prNumber).catch(() => null); + await upsertPullRequestDetailSyncState(env, { + repoFullName, + pullNumber: prNumber, + status: existing?.status ?? "never_synced", + ciState: null, + ciStateFetchedAt: null, + }); +} + /** Resolve the OPEN PRs associated with a commit SHA via the REST `GET /repos/{owner}/{repo}/commits/{sha}/pulls` * endpoint. This is the only PR↔commit resolution that works for FORK (cross-repo) PRs, whose CI-completion * webhooks (`check_suite`/`check_run`) carry an EMPTY `pull_requests[]`. Returns the de-duplicated open PR numbers. diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 12fc67943f..12c1f28233 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -82,11 +82,15 @@ import { backfillRegisteredRepositories, backfillRepositorySegment, cachedFetchLivePullRequestMergeState, + CI_STATE_CACHE_METRIC, + deserializeCachedCiAggregate, enqueueRepositoryOpenDataBackfill, fetchAndStorePullRequestFilesForReview, fetchLinkedIssueFacts, fetchLiveBaseBranchAdvancedAt, fetchLiveCiAggregatePreferGraphQl, + invalidateCiStateCache, + isCiStateCacheFresh, type LiveCiAggregate, fetchLiveIssueState, fetchLivePullRequest, @@ -105,6 +109,7 @@ import { refreshContributorActivity, refreshInstallationHealth, refreshPullRequestDetails, + writeThroughCiStateCache, } from "../github/backfill"; import { contributorRepoStatsFromGittensor, @@ -568,23 +573,76 @@ function evictLiveFactOnReject( }); } +/** + * Cached read of the live CI aggregate, backed by pull_request_detail_sync_state (#selfhost-ci-verification, + * sibling to the #2537 PR-state cache in backfill.ts). A fresh cache row (webhook-invalidated on + * check_run/check_suite `completed` via invalidateCiStateCache, capped at CI_STATE_CACHE_MAX_AGE_MS) is served + * without a GitHub call; otherwise fetches live via fetchLiveCiAggregatePreferGraphQl and write-throughs the + * result via writeThroughCiStateCache. Fail-open throughout: any cache read/write hiccup falls back to / + * degrades to a live fetch, never blocks it. + * + * `forceRefresh` (set by refreshLiveCiAggregate below, mirroring refreshLiveMergeState's OWN "never durable- + * cached" contract for merge-state): skips the cache READ entirely, always fetching live -- a "refresh" caller + * needs a genuinely fresh read even within the SAME job pass (e.g. re-checking CI right after this pass's own + * gate/check-run publication, which can flip a status GitHub hasn't sent a webhook for yet). The WRITE-through + * still happens on a forced refresh, so a LATER pass/job still benefits from this read. + * + * Deliberately implemented HERE, not in backfill.ts (where writeThroughCiStateCache/isCiStateCacheFresh/ + * deserializeCachedCiAggregate live) -- a same-module call from backfill.ts to its own + * fetchLiveCiAggregatePreferGraphQl would be invisible to `vi.spyOn(backfillModule, + * "fetchLiveCiAggregatePreferGraphQl")`, which many existing tests already rely on to intercept the CROSS-module + * call this file has always made. Keeping the orchestration here preserves that exact, already-tested call shape. + * + * NEVER call this from an act-boundary merge/close decision -- services/agent-approval-queue.ts and + * services/agent-action-executor.ts call fetchLiveCiAggregate/fetchLiveCiAggregatePreferGraphQl directly, by + * design, and must keep doing so. + */ +async function cachedFetchLiveCiAggregate( + env: Env, + repoFullName: string, + prNumber: number, + headSha: string | null | undefined, + token: string | undefined, + requiredContexts: ReadonlySet | null | undefined, + requiredContextsKey: string, + forceRefresh: boolean, + admissionKey?: GitHubRateLimitAdmissionKey, +): Promise { + const cached = await getPullRequestDetailSyncState(env, repoFullName, prNumber).catch(() => null); + if (!forceRefresh && cached && isCiStateCacheFresh(cached, headSha, requiredContextsKey)) { + const deserialized = deserializeCachedCiAggregate(cached); + if (deserialized) { + incr(CI_STATE_CACHE_METRIC, { field: "aggregate", result: "hit" }); + return deserialized; + } + } + incr(CI_STATE_CACHE_METRIC, { field: "aggregate", result: forceRefresh ? "forced" : "miss" }); + const live = await fetchLiveCiAggregatePreferGraphQl(env, repoFullName, headSha, token, requiredContexts, admissionKey); + await writeThroughCiStateCache(env, repoFullName, prNumber, cached, headSha, requiredContextsKey, live); + return live; +} + function fetchLiveCiAggregateWithRequiredContexts( env: Env, repoFullName: string, facts: LiveGithubFacts, + prNumber: number, headSha: string | null | undefined, baseRef: string | null | undefined, token: string | undefined, expectedCiContexts: ReadonlyArray | null | undefined, + forceRefresh: boolean, admissionKey?: GitHubRateLimitAdmissionKey, ): Promise { // CI refresh callers need fresh check/status state; branch protection contexts move slowly enough to stay // request-cached. When the #1941 flag is on, fetchLiveCiAggregatePreferGraphQl collapses the check/status reads // into one GraphQL rollup (reusing these requiredContexts), else it uses the proven REST aggregate. + // cachedFetchLiveCiAggregate (#selfhost-ci-verification) is the durable, cross-job snapshot cache sibling to + // this request-scoped LiveGithubFacts memo -- it is only ever consulted here, on a LiveGithubFacts miss. return cachedRequiredStatusContexts(env, repoFullName, facts, baseRef, token, expectedCiContexts, admissionKey) .catch(() => null) .then((requiredContexts) => - fetchLiveCiAggregatePreferGraphQl(env, repoFullName, headSha, token, requiredContexts, admissionKey), + cachedFetchLiveCiAggregate(env, repoFullName, prNumber, headSha, token, requiredContexts, expectedCiContextsKeyPart(expectedCiContexts), forceRefresh, admissionKey), ); } @@ -592,6 +650,7 @@ function cachedLiveCiAggregate( env: Env, repoFullName: string, facts: LiveGithubFacts, + prNumber: number, headSha: string | null | undefined, baseRef: string | null | undefined, token: string | undefined, @@ -608,10 +667,12 @@ function cachedLiveCiAggregate( env, repoFullName, facts, + prNumber, headSha, baseRef, token, expectedCiContexts, + false, admissionKey, ), ); @@ -623,6 +684,7 @@ function refreshLiveCiAggregate( env: Env, repoFullName: string, facts: LiveGithubFacts, + prNumber: number, headSha: string | null | undefined, baseRef: string | null | undefined, token: string | undefined, @@ -637,10 +699,12 @@ function refreshLiveCiAggregate( env, repoFullName, facts, + prNumber, headSha, baseRef, token, expectedCiContexts, + true, admissionKey, ), ); @@ -2146,6 +2210,7 @@ async function runAgentMaintenancePlanAndExecute( env, repoFullName, args.liveFacts, + pr.number, pr.headSha, baseRef, token, @@ -2921,7 +2986,7 @@ async function prReadyForReview( } // 2) wait for CI to finish before running the Gittensory review. Required contexts still define which failures // block/close, but hasPending tracks any visible non-bot CI that is not settled yet. - const ci = await cachedLiveCiAggregate(env, repoFullName, liveFacts, pr.headSha, pr.baseRef, token, settings.expectedCiContexts, admissionKey).catch(() => undefined); + const ci = await cachedLiveCiAggregate(env, repoFullName, liveFacts, pr.number, pr.headSha, pr.baseRef, token, settings.expectedCiContexts, admissionKey).catch(() => undefined); if (ci?.hasPending) { // Staleness cap: inferred or unreadable pending CI can otherwise defer FOREVER (orphaned required context, // transiently unreadable pages, fork check that never reports). Past the cap we stop deferring and let the @@ -3610,6 +3675,14 @@ async function maybeReReviewOnCiCompletion( }).catch(() => undefined); } for (const prNumber of prNumbers) { + // #selfhost-ci-verification: invalidate the durable CI-state cache for EVERY resolved PR, regardless of + // whether the re-review below actually fires -- some OTHER reader (a readiness check or disposition- + // planner pass already in flight) may consult the cache in the near future and must not see a stale + // pre-completion snapshot. Best-effort, matches every other cache-invalidation call site's fail-open + // contract; ordered BEFORE reReviewStoredPullRequest so that pass's own refreshLiveCiAggregate read (which + // now also consults this durable cache on a request-scoped memo miss) sees a genuine miss and re-fetches + // live, preserving refreshLiveCiAggregate's existing "always fresh" contract for this triggering PR. + await invalidateCiStateCache(env, repoFullName, prNumber).catch(() => undefined); // Coalesce the CI-completion storm: skip if this PR was re-reviewed within the window. if (await ciReReviewCoalesced(env, repoFullName, prNumber)) continue; await reReviewStoredPullRequest( @@ -8267,7 +8340,7 @@ async function maybePublishPrPublicSurface( const baseRef = pr.baseRef ?? repo?.defaultBranch; // Required contexts still detect missing/pending required CI, but every visible completed red check/status is // adverse and blocks the PR. - const liveCi = await refreshLiveCiAggregate(env, repoFullName, webhook.liveFacts, pr.headSha, baseRef, token, settings.expectedCiContexts, admissionKey); + const liveCi = await refreshLiveCiAggregate(env, repoFullName, webhook.liveFacts, pr.number, pr.headSha, baseRef, token, settings.expectedCiContexts, admissionKey); // Live merge-state too — the SAME source the disposition uses (planAgentMaintenanceActions reads liveMergeState). // The stored pr.mergeableState lags GitHub's async recompute, and the gate's own check/review publication can // also advance mergeability after readiness ran, so refresh at this post-publish boundary. diff --git a/src/selfhost/metrics.ts b/src/selfhost/metrics.ts index eefac79186..69ccae5ed0 100644 --- a/src/selfhost/metrics.ts +++ b/src/selfhost/metrics.ts @@ -120,6 +120,7 @@ const DEFAULT_METRIC_META: readonly (readonly [string, MetricMeta])[] = [ ["gittensory_github_branch_protection_permission_denied_total", { help: "GitHub branch-protection reads denied by permissions.", type: "counter" }], ["gittensory_github_pr_files_fetch_total", { help: "GitHub pull-request file fetch attempts.", type: "counter" }], ["gittensory_pr_state_cache_total", { help: "Pull-request state cache outcomes.", type: "counter" }], + ["gittensory_ci_state_cache_total", { help: "CI-state snapshot cache outcomes.", type: "counter" }], ]; const metricMeta = new Map(DEFAULT_METRIC_META); diff --git a/src/types.ts b/src/types.ts index da84f79e43..71ae850d08 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1156,6 +1156,21 @@ export type PullRequestDetailSyncStateRecord = { prMergeableState?: string | null | undefined; prState?: string | null | undefined; prStateFetchedAt?: string | null | undefined; + // #selfhost-ci-verification (CI-state snapshot cache sibling to the #2537 PR-state trio above): a durable + // mirror of the LiveCiAggregate the gate's own live-CI fetch already produces (src/github/backfill.ts), + // keyed fresh only when BOTH ciHeadSha matches the head_sha being queried AND ciRequiredContextsKey matches + // the current settings.expectedCiContexts. NEVER read by the act-boundary merge/close decision (see the + // schema.ts comment) -- those paths always force a live fetch. + ciHeadSha?: string | null | undefined; + ciState?: "passed" | "failed" | "pending" | "unverified" | null | undefined; + ciHasPending?: boolean | null | undefined; + ciHasVisiblePending?: boolean | null | undefined; + ciHasMissingRequiredContext?: boolean | null | undefined; + ciFailingDetailsJson?: string | null | undefined; + ciNonRequiredFailingDetailsJson?: string | null | undefined; + ciCompletenessWarning?: string | null | undefined; + ciRequiredContextsKey?: string | null | undefined; + ciStateFetchedAt?: string | null | undefined; updatedAt?: string | null | undefined; }; diff --git a/test/unit/pr-detail-durable-cache.test.ts b/test/unit/pr-detail-durable-cache.test.ts index 8403d70bfa..1ee0a501b9 100644 --- a/test/unit/pr-detail-durable-cache.test.ts +++ b/test/unit/pr-detail-durable-cache.test.ts @@ -7,12 +7,70 @@ import { cachedFetchLivePullRequestHeadSha, cachedFetchLivePullRequestMergeState, cachedFetchLivePullRequestState, + deserializeCachedCiAggregate, + invalidateCiStateCache, invalidatePrStateCache, + isCiStateCacheFresh, primeDurablePrStateCache, + writeThroughCiStateCache, } from "../../src/github/backfill"; import { clearGitHubResponseCacheForTest } from "../../src/github/client"; import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics"; import { createTestEnv } from "../helpers/d1"; +import type { LiveCiAggregate } from "../../src/github/backfill"; +import type { PullRequestDetailSyncStateRecord } from "../../src/types"; + +function stubFetchTracking(handler: (url: string, init?: RequestInit) => Response | Promise): string[] { + const urls: string[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + urls.push(url); + return handler(url, init); + }); + return urls; +} + +// Simulates a D1 write hiccup ONLY for pull_request_detail_sync_state upserts, so the cache's fail-open +// write-through can be exercised without a full DB outage. Module-scope (not describe-scoped) so both the +// PR-state and CI-state cache describe blocks below can share it. +function withPrStateWriteFailure(env: Env): Env { + const db = env.DB as unknown as { prepare(sql: string): unknown; batch(statements: unknown[]): Promise }; + return { + ...env, + DB: { + prepare(sql: string) { + if (sql.includes("pull_request_detail_sync_state") && sql.trim().toUpperCase().startsWith("INSERT")) { + throw new Error("pull_request_detail_sync_state write failed"); + } + return db.prepare.call(db, sql); + }, + batch(statements: unknown[]) { + return db.batch.call(db, statements); + }, + } as unknown as D1Database, + }; +} + +// Mirrors withPrStateWriteFailure but for the READ side -- exercises invalidateCiStateCache's own +// getPullRequestDetailSyncState(...).catch(() => null) fail-open arm (it must still write the invalidation +// even when it cannot read the prior row to preserve `status`). +function withPrStateReadFailure(env: Env): Env { + const db = env.DB as unknown as { prepare(sql: string): unknown; batch(statements: unknown[]): Promise }; + return { + ...env, + DB: { + prepare(sql: string) { + if (sql.includes("pull_request_detail_sync_state") && sql.trim().toUpperCase().startsWith("SELECT")) { + throw new Error("pull_request_detail_sync_state read failed"); + } + return db.prepare.call(db, sql); + }, + batch(statements: unknown[]) { + return db.batch.call(db, statements); + }, + } as unknown as D1Database, + }; +} // Durable, webhook-invalidated cache for the bare PR-state read (#2537). Mirrors // backfill-file-hydration-scoping.test.ts's helpers/structure for the sibling files cache. @@ -23,36 +81,6 @@ describe("durable PR-state cache (#2537)", () => { vi.unstubAllGlobals(); }); - function stubFetchTracking(handler: (url: string, init?: RequestInit) => Response | Promise): string[] { - const urls: string[] = []; - vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { - const url = input.toString(); - urls.push(url); - return handler(url, init); - }); - return urls; - } - - // Simulates a D1 write hiccup ONLY for pull_request_detail_sync_state upserts, so the cache's fail-open - // write-through can be exercised without a full DB outage. - function withPrStateWriteFailure(env: Env): Env { - const db = env.DB as unknown as { prepare(sql: string): unknown; batch(statements: unknown[]): Promise }; - return { - ...env, - DB: { - prepare(sql: string) { - if (sql.includes("pull_request_detail_sync_state") && sql.trim().toUpperCase().startsWith("INSERT")) { - throw new Error("pull_request_detail_sync_state write failed"); - } - return db.prepare.call(db, sql); - }, - batch(statements: unknown[]) { - return db.batch.call(db, statements); - }, - } as unknown as D1Database, - }; - } - // REGRESSION (#2595 review defect): the three cached readers below share ONE prStateFetchedAt column as their // freshness stamp. Before this fix, each reader wrote through ONLY the one field it cared about, so a write // from reader A would make reader B's UN-fetched field look "fresh" to a subsequent call -- silently returning @@ -508,3 +536,251 @@ describe("durable PR-state cache (#2537)", () => { }); }); }); + +// Durable, webhook-invalidated cache for the CI-state aggregate (#selfhost-ci-verification), sibling to the +// #2537 PR-state cache above. The read-cache-then-live-fetch-then-write-through ORCHESTRATION +// (cachedFetchLiveCiAggregate) is private to queue/processors.ts (see its own doc comment for why: a same-module +// call to fetchLiveCiAggregatePreferGraphQl would be invisible to many existing tests' `vi.spyOn` on this +// module's export) — its behavior is exercised indirectly via processJob() in queue.test.ts. This file covers +// the pure/DB-level building blocks that DO live here and ARE exported. +describe("durable CI-state cache (#selfhost-ci-verification)", () => { + afterEach(() => { + resetMetrics(); + }); + + const sampleAggregate: LiveCiAggregate = { + ciState: "failed", + hasPending: false, + hasVisiblePending: false, + hasMissingRequiredContext: false, + failingDetails: [{ name: "ci/build", summary: "failed", detailsUrl: "https://ci.example.test/1" }], + nonRequiredFailingDetails: [], + ciCompletenessWarning: null, + }; + + describe("isCiStateCacheFresh", () => { + it("is fresh: fetched within the TTL, head_sha matches, required-contexts key matches", () => { + const cached: Pick = { + ciHeadSha: "sha1", + ciRequiredContextsKey: "build test", + ciStateFetchedAt: new Date().toISOString(), + }; + expect(isCiStateCacheFresh(cached, "sha1", "build test")).toBe(true); + }); + + it("is stale once the TTL has expired", () => { + const cached = { ciHeadSha: "sha1", ciRequiredContextsKey: "", ciStateFetchedAt: "2020-01-01T00:00:00.000Z" }; + expect(isCiStateCacheFresh(cached, "sha1", "")).toBe(false); + }); + + it("is a miss when ciStateFetchedAt is null/missing (never cached)", () => { + expect(isCiStateCacheFresh({ ciHeadSha: "sha1", ciRequiredContextsKey: "", ciStateFetchedAt: null }, "sha1", "")).toBe(false); + expect(isCiStateCacheFresh(null, "sha1", "")).toBe(false); + expect(isCiStateCacheFresh(undefined, "sha1", "")).toBe(false); + }); + + it("is a miss on a malformed fetchedAt timestamp", () => { + expect(isCiStateCacheFresh({ ciHeadSha: "sha1", ciRequiredContextsKey: "", ciStateFetchedAt: "not-a-date" }, "sha1", "")).toBe(false); + }); + + it("is a miss when the cached head_sha no longer matches (a new commit since this row was cached), even within the TTL", () => { + const cached = { ciHeadSha: "sha-old", ciRequiredContextsKey: "", ciStateFetchedAt: new Date().toISOString() }; + expect(isCiStateCacheFresh(cached, "sha-new", "")).toBe(false); + }); + + it("is a miss when the required-contexts key no longer matches (a settings change), even within the TTL", () => { + const cached = { ciHeadSha: "sha1", ciRequiredContextsKey: "old-key", ciStateFetchedAt: new Date().toISOString() }; + expect(isCiStateCacheFresh(cached, "sha1", "new-key")).toBe(false); + }); + + it("treats a null cached head_sha and an undefined queried head_sha as equal (both mean 'no head sha')", () => { + const cached = { ciHeadSha: null, ciRequiredContextsKey: "", ciStateFetchedAt: new Date().toISOString() }; + expect(isCiStateCacheFresh(cached, undefined, "")).toBe(true); + }); + + it("treats a null cached required-contexts key and an empty-string queried key as equal (both mean 'unconfigured')", () => { + const cached = { ciHeadSha: "sha1", ciRequiredContextsKey: null, ciStateFetchedAt: new Date().toISOString() }; + expect(isCiStateCacheFresh(cached, "sha1", "")).toBe(true); + }); + }); + + describe("deserializeCachedCiAggregate", () => { + it("round-trips a valid cached row back into the exact LiveCiAggregate shape", () => { + const cached = { + ciState: "failed" as const, + ciHasPending: false, + ciHasVisiblePending: false, + ciHasMissingRequiredContext: false, + ciFailingDetailsJson: JSON.stringify(sampleAggregate.failingDetails), + ciNonRequiredFailingDetailsJson: JSON.stringify(sampleAggregate.nonRequiredFailingDetails), + ciCompletenessWarning: null, + }; + expect(deserializeCachedCiAggregate(cached)).toEqual(sampleAggregate); + }); + + it("returns null when ciState is missing/null (never cached)", () => { + expect( + deserializeCachedCiAggregate({ + ciState: null, + ciHasPending: null, + ciHasVisiblePending: null, + ciHasMissingRequiredContext: null, + ciFailingDetailsJson: null, + ciNonRequiredFailingDetailsJson: null, + ciCompletenessWarning: null, + }), + ).toBeNull(); + }); + + it("fails open to null on malformed JSON in ciFailingDetailsJson (never throws)", () => { + expect( + deserializeCachedCiAggregate({ + ciState: "passed", + ciHasPending: false, + ciHasVisiblePending: false, + ciHasMissingRequiredContext: false, + ciFailingDetailsJson: "{not valid json", + ciNonRequiredFailingDetailsJson: "[]", + ciCompletenessWarning: null, + }), + ).toBeNull(); + }); + + it("defaults hasPending/hasVisiblePending/hasMissingRequiredContext to false and the JSON arrays to [] when null", () => { + expect( + deserializeCachedCiAggregate({ + ciState: "passed", + ciHasPending: null, + ciHasVisiblePending: null, + ciHasMissingRequiredContext: null, + ciFailingDetailsJson: null, + ciNonRequiredFailingDetailsJson: null, + ciCompletenessWarning: null, + }), + ).toEqual({ + ciState: "passed", + hasPending: false, + hasVisiblePending: false, + hasMissingRequiredContext: false, + failingDetails: [], + nonRequiredFailingDetails: [], + ciCompletenessWarning: null, + }); + }); + }); + + describe("writeThroughCiStateCache", () => { + it("writes every CI field, preserving the row's existing status rather than forcing one", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { repoFullName: "owner/repo", pullNumber: 80, status: "partial" }); + + await writeThroughCiStateCache(env, "owner/repo", 80, { status: "partial" }, "sha1", "build test", sampleAggregate); + + const row = await getPullRequestDetailSyncState(env, "owner/repo", 80); + expect(row).toMatchObject({ + status: "partial", + ciHeadSha: "sha1", + ciState: "failed", + ciHasPending: false, + ciHasVisiblePending: false, + ciHasMissingRequiredContext: false, + ciRequiredContextsKey: "build test", + }); + expect(JSON.parse(row?.ciFailingDetailsJson ?? "[]")).toEqual(sampleAggregate.failingDetails); + expect(typeof row?.ciStateFetchedAt).toBe("string"); + }); + + it("defaults status to never_synced when no prior row exists", async () => { + const env = createTestEnv(); + await writeThroughCiStateCache(env, "owner/repo", 81, null, "sha1", "", sampleAggregate); + expect(await getPullRequestDetailSyncState(env, "owner/repo", 81)).toMatchObject({ status: "never_synced" }); + }); + + it("preserves unrelated existing columns (prMergeableState, the files-cache headSha) on write", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "owner/repo", + pullNumber: 82, + status: "complete", + headSha: "files-cache-sha", + prMergeableState: "clean", + }); + + await writeThroughCiStateCache(env, "owner/repo", 82, { status: "complete" }, "sha1", "", sampleAggregate); + + expect(await getPullRequestDetailSyncState(env, "owner/repo", 82)).toMatchObject({ + headSha: "files-cache-sha", + prMergeableState: "clean", + ciState: "failed", + }); + }); + + it("fail-open: a write hiccup is swallowed, never throws", async () => { + const env = withPrStateWriteFailure(createTestEnv()); + await expect(writeThroughCiStateCache(env, "owner/repo", 83, null, "sha1", "", sampleAggregate)).resolves.toBeUndefined(); + }); + + it("stores a null ciHeadSha when the live read had no resolvable head SHA (nullish fallback, not just a truthy sha)", async () => { + const env = createTestEnv(); + await writeThroughCiStateCache(env, "owner/repo", 85, null, null, "", sampleAggregate); + expect(await getPullRequestDetailSyncState(env, "owner/repo", 85)).toMatchObject({ ciHeadSha: null, ciState: "failed" }); + }); + + it("records the write metric", async () => { + resetMetrics(); + const env = createTestEnv(); + await writeThroughCiStateCache(env, "owner/repo", 84, null, "sha1", "", sampleAggregate); + expect(await renderMetrics()).toContain('gittensory_ci_state_cache_total{field="write",result="set"} 1'); + }); + }); + + describe("invalidateCiStateCache", () => { + it("clears ciState/ciStateFetchedAt, preserving unrelated columns", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "owner/repo", + pullNumber: 90, + status: "complete", + prMergeableState: "clean", + ciHeadSha: "sha1", + ciState: "failed", + ciStateFetchedAt: new Date().toISOString(), + }); + + await invalidateCiStateCache(env, "owner/repo", 90); + + expect(await getPullRequestDetailSyncState(env, "owner/repo", 90)).toMatchObject({ + ciState: null, + ciStateFetchedAt: null, + prMergeableState: "clean", + status: "complete", + }); + }); + + it("clears regardless of prior value (even a fresh, just-written cache entry)", async () => { + const env = createTestEnv(); + await writeThroughCiStateCache(env, "owner/repo", 91, null, "sha1", "", sampleAggregate); + expect(await getPullRequestDetailSyncState(env, "owner/repo", 91)).toMatchObject({ ciState: "failed" }); + + await invalidateCiStateCache(env, "owner/repo", 91); + + expect(await getPullRequestDetailSyncState(env, "owner/repo", 91)).toMatchObject({ ciState: null, ciStateFetchedAt: null }); + }); + + it("is a no-op (does not throw) when no row exists yet, defaulting status to never_synced", async () => { + const env = createTestEnv(); + await expect(invalidateCiStateCache(env, "owner/repo", 92)).resolves.toBeUndefined(); + expect(await getPullRequestDetailSyncState(env, "owner/repo", 92)).toMatchObject({ status: "never_synced", ciState: null }); + }); + + it("fail-open: a read hiccup on the prior-row lookup still lets the invalidation write proceed, defaulting status to never_synced", async () => { + // readFailEnv wraps the SAME underlying D1 instance as baseEnv, breaking only SELECTs -- invalidateCiStateCache's + // OWN getPullRequestDetailSyncState(...).catch(() => null) must swallow that and still issue the INSERT + // (which is unaffected), so a read via the unwrapped baseEnv afterward proves the write actually landed. + const baseEnv = createTestEnv(); + const readFailEnv = withPrStateReadFailure(baseEnv); + await expect(invalidateCiStateCache(readFailEnv, "owner/repo", 93)).resolves.toBeUndefined(); + expect(await getPullRequestDetailSyncState(baseEnv, "owner/repo", 93)).toMatchObject({ status: "never_synced", ciState: null }); + }); + }); +}); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index a6a4cd2f67..aa5587fce0 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -1692,6 +1692,233 @@ describe("queue processors", () => { } }); + describe("durable CI-state snapshot cache (#selfhost-ci-verification, cross-job)", () => { + async function seedRepoAndPr(headSha: string): Promise<{ env: ReturnType }> { + // GITTENSORY_REVIEW_REPOS (review/cutover-gate.ts) gates maybeReReviewOnCiCompletion's whole invalidation + // loop -- an unlisted repo leaves the durable cache never invalidated by a check_run/check_suite webhook, + // relying solely on the 60s TTL. Must be allowlisted for the invalidation tests below to be meaningful. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), GITTENSORY_REVIEW_REPOS: "owner/agent-repo" }); + await upsertInstallation(env, { action: "created", installation: { id: 9001, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: { pull_requests: "write", checks: "write" }, events: [] } }); + await upsertRepositoryFromGitHub(env, { name: "agent-repo", full_name: "owner/agent-repo", private: false, owner: { login: "owner" } }, 9001); + // isAgentConfigured (settings/autonomy.ts) requires at least one ACTING autonomy class before + // prReadyForReview even reaches the live CI read (processors.ts's readiness short-circuits to `true`, + // skipping cachedLiveCiAggregate entirely, when no class is "auto"/"auto_with_approval") -- so this can't + // be omitted or left fully "observe" the way a non-CI-cache test could. `auto_with_approval` (not `auto`) + // keeps both passes comparable: it satisfies isAgentConfigured, but the action executor STAGES the merge + // for approval instead of ever calling the GitHub merge endpoint, so the PR stays open with the same + // head_sha across both passes. + await upsertRepositorySettings(env, { repoFullName: "owner/agent-repo", autonomy: { merge: "auto_with_approval", update_branch: "auto_with_approval" }, aiReviewMode: "off", gatePack: "oss-anti-slop", gateCheckMode: "enabled", checkRunMode: "off", commentMode: "off", publicSurface: "off" }); + await upsertPullRequestFromGitHub(env, "owner/agent-repo", { number: 7, title: "Cross-job CI cache", state: "open", user: { login: "contributor" }, head: { sha: headSha }, base: { ref: "main" }, labels: [], body: "Closes #1" }); + return { env }; + } + + it("a second agent-regate-pr pass for the SAME still-settled head_sha serves the readiness check from the durable cache (fewer live CI reads than the first pass)", async () => { + const { env } = await seedRepoAndPr("a7"); + const requiredContextsSpy = vi.spyOn(backfillModule, "fetchRequiredStatusContexts").mockResolvedValue(null); + const liveCiSpy = vi.spyOn(backfillModule, "fetchLiveCiAggregatePreferGraphQl").mockResolvedValue({ + ciState: "passed", + hasPending: false, + hasVisiblePending: false, + hasMissingRequiredContext: false, + failingDetails: [], + nonRequiredFailingDetails: [], + ciCompletenessWarning: null, + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = (init?.method ?? "GET").toUpperCase(); + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (/\/pulls\/7(?:\?|$)/.test(url) && method === "GET") return Response.json({ number: 7, title: "Cross-job CI cache", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, mergeable_state: "clean", labels: [], body: "Closes #1" }); + if (url.includes("/pulls/7/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + return Response.json({}); + }); + + try { + resetMetrics(); + // Pass 1: cold. Readiness's cachedLiveCiAggregate misses (nothing cached yet); the disposition planner's + // refreshLiveCiAggregate always forces a live read regardless. Both write through the same durable row. + await processJob(env, { type: "agent-regate-pr", deliveryId: "cross-job-pass-1", repoFullName: "owner/agent-repo", prNumber: 7, installationId: 9001 }); + const callsAfterPass1 = liveCiSpy.mock.calls.length; + expect(callsAfterPass1).toBeGreaterThan(0); + expect(await renderMetrics()).toContain('gittensory_ci_state_cache_total{field="aggregate",result="miss"} 1'); + expect(await renderMetrics()).toContain('gittensory_ci_state_cache_total{field="aggregate",result="forced"} 1'); + + // Pass 2: SAME PR, SAME head_sha, no invalidating webhook in between. Readiness's cachedLiveCiAggregate + // now HITS the row pass 1's disposition planner wrote through -- one fewer live call than pass 1, even + // though the disposition planner's OWN refreshLiveCiAggregate still forces a fresh read every time. + await processJob(env, { type: "agent-regate-pr", deliveryId: "cross-job-pass-2", repoFullName: "owner/agent-repo", prNumber: 7, installationId: 9001 }); + const callsDuringPass2 = liveCiSpy.mock.calls.length - callsAfterPass1; + expect(callsDuringPass2).toBeLessThan(callsAfterPass1); + expect(await renderMetrics()).toContain('gittensory_ci_state_cache_total{field="aggregate",result="hit"} 1'); + // The disposition planner's forced refresh fired again on pass 2 too (now 2 total across both passes). + expect(await renderMetrics()).toContain('gittensory_ci_state_cache_total{field="aggregate",result="forced"} 2'); + } finally { + liveCiSpy.mockRestore(); + requiredContextsSpy.mockRestore(); + } + }); + + it("a check_run completed webhook invalidates the durable cache, forcing the NEXT readiness check to miss again", async () => { + const { env } = await seedRepoAndPr("a7"); + const requiredContextsSpy = vi.spyOn(backfillModule, "fetchRequiredStatusContexts").mockResolvedValue(null); + const liveCiSpy = vi.spyOn(backfillModule, "fetchLiveCiAggregatePreferGraphQl").mockResolvedValue({ + ciState: "passed", + hasPending: false, + hasVisiblePending: false, + hasMissingRequiredContext: false, + failingDetails: [], + nonRequiredFailingDetails: [], + ciCompletenessWarning: null, + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = (init?.method ?? "GET").toUpperCase(); + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (/\/pulls\/7(?:\?|$)/.test(url) && method === "GET") return Response.json({ number: 7, title: "Cross-job CI cache", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, mergeable_state: "clean", labels: [], body: "Closes #1" }); + if (url.includes("/pulls/7/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + return Response.json({}); + }); + + try { + resetMetrics(); + await processJob(env, { type: "agent-regate-pr", deliveryId: "invalidate-pass-1", repoFullName: "owner/agent-repo", prNumber: 7, installationId: 9001 }); + expect(await renderMetrics()).toContain('gittensory_ci_state_cache_total{field="aggregate",result="miss"} 1'); + // The durable row now has a fresh ciState, well within the 60s TTL. + expect(await getPullRequestDetailSyncState(env, "owner/agent-repo", 7)).toMatchObject({ ciState: "passed" }); + + // maybeReReviewOnCiCompletion invalidates THEN (unless coalesced) immediately re-reviews the same PR -- + // so a naive "row is null right after the webhook" assertion is a race against that same job's own + // re-review repopulating it. Pre-claim the ci-coalesce window (mirrors the existing technique above at + // "ci-coalesce:owner/agent-repo#7") so this delivery's own re-review is skipped, leaving the + // invalidation's null state directly observable rather than immediately overwritten. + await env.SELFHOST_TRANSIENT_CACHE?.set("ci-coalesce:owner/agent-repo#7", "1", 60); + await processJob(env, { + type: "github-webhook", + deliveryId: "check-run-completed", + eventName: "check_run", + payload: { + action: "completed", + repository: { name: "agent-repo", full_name: "owner/agent-repo", owner: { login: "owner" } }, + installation: { id: 9001 }, + check_run: { head_sha: "a7", pull_requests: [{ number: 7 }] }, + }, + } as never); + + expect(await getPullRequestDetailSyncState(env, "owner/agent-repo", 7)).toMatchObject({ ciState: null, ciStateFetchedAt: null }); + + // A subsequent readiness check misses again -- proving invalidation, not just a coincidental TTL expiry. + resetMetrics(); + await processJob(env, { type: "agent-regate-pr", deliveryId: "invalidate-pass-2", repoFullName: "owner/agent-repo", prNumber: 7, installationId: 9001 }); + expect(await renderMetrics()).toContain('gittensory_ci_state_cache_total{field="aggregate",result="miss"} 1'); + } finally { + liveCiSpy.mockRestore(); + requiredContextsSpy.mockRestore(); + } + }); + + it("a failing cache invalidation write does not crash the check_run webhook's re-review (best-effort, fail-open)", async () => { + const { env } = await seedRepoAndPr("a7"); + const requiredContextsSpy = vi.spyOn(backfillModule, "fetchRequiredStatusContexts").mockResolvedValue(null); + const liveCiSpy = vi.spyOn(backfillModule, "fetchLiveCiAggregatePreferGraphQl").mockResolvedValue({ + ciState: "passed", + hasPending: false, + hasVisiblePending: false, + hasMissingRequiredContext: false, + failingDetails: [], + nonRequiredFailingDetails: [], + ciCompletenessWarning: null, + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = (init?.method ?? "GET").toUpperCase(); + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (/\/pulls\/7(?:\?|$)/.test(url) && method === "GET") return Response.json({ number: 7, title: "Cross-job CI cache", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, mergeable_state: "clean", labels: [], body: "Closes #1" }); + if (url.includes("/pulls/7/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + return Response.json({}); + }); + // invalidateCiStateCache's OWN read already fails open internally; this instead breaks its WRITE (the one + // call the function does not itself wrap in a .catch), so the coverage that matters here is the CALL SITE's + // own .catch(() => undefined) in maybeReReviewOnCiCompletion (processors.ts) -- one bad invalidation write + // must never crash the webhook job or block the coalesced re-review that follows it in the same loop body. + const upsertSyncStateSpy = vi.spyOn(repositoriesModule, "upsertPullRequestDetailSyncState").mockRejectedValueOnce(new Error("D1 write failed")); + + try { + await expect( + processJob(env, { + type: "github-webhook", + deliveryId: "check-run-invalidate-write-fails", + eventName: "check_run", + payload: { + action: "completed", + repository: { name: "agent-repo", full_name: "owner/agent-repo", owner: { login: "owner" } }, + installation: { id: 9001 }, + check_run: { head_sha: "a7", pull_requests: [{ number: 7 }] }, + }, + } as never), + ).resolves.toBeUndefined(); + // The re-review after the failed invalidation still ran and wrote its own fresh entry. + expect(await getPullRequestDetailSyncState(env, "owner/agent-repo", 7)).toMatchObject({ ciState: "passed" }); + } finally { + upsertSyncStateSpy.mockRestore(); + liveCiSpy.mockRestore(); + requiredContextsSpy.mockRestore(); + } + }); + + // #selfhost-ci-verification MVP scope boundary: status/workflow_run events aren't handled by + // maybeReReviewOnCiCompletion for RE-REVIEW TRIGGERING today, so they don't invalidate the CI-state cache + // either -- the 60s TTL is the sole backstop for these two event types. This test documents that boundary + // explicitly rather than leaving it silently unverified. + it.each(["status", "workflow_run"])("a %s webhook event does not invalidate the durable CI-state cache (documented MVP boundary — TTL is the backstop)", async (eventName) => { + const { env } = await seedRepoAndPr("a7"); + const requiredContextsSpy = vi.spyOn(backfillModule, "fetchRequiredStatusContexts").mockResolvedValue(null); + const liveCiSpy = vi.spyOn(backfillModule, "fetchLiveCiAggregatePreferGraphQl").mockResolvedValue({ + ciState: "passed", + hasPending: false, + hasVisiblePending: false, + hasMissingRequiredContext: false, + failingDetails: [], + nonRequiredFailingDetails: [], + ciCompletenessWarning: null, + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = (init?.method ?? "GET").toUpperCase(); + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (/\/pulls\/7(?:\?|$)/.test(url) && method === "GET") return Response.json({ number: 7, title: "Cross-job CI cache", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, mergeable_state: "clean", labels: [], body: "Closes #1" }); + if (url.includes("/pulls/7/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + return Response.json({}); + }); + + try { + await processJob(env, { type: "agent-regate-pr", deliveryId: "mvp-boundary-seed", repoFullName: "owner/agent-repo", prNumber: 7, installationId: 9001 }); + expect(await getPullRequestDetailSyncState(env, "owner/agent-repo", 7)).toMatchObject({ ciState: "passed" }); + + await processJob(env, { + type: "github-webhook", + deliveryId: `${eventName}-event`, + eventName, + payload: { + action: "completed", + repository: { name: "agent-repo", full_name: "owner/agent-repo", owner: { login: "owner" } }, + installation: { id: 9001 }, + [eventName]: { sha: "a7", head_sha: "a7" }, + }, + } as never); + + // Still cached -- this event type is not wired to invalidateCiStateCache today. + expect(await getPullRequestDetailSyncState(env, "owner/agent-repo", 7)).toMatchObject({ ciState: "passed" }); + } finally { + liveCiSpy.mockRestore(); + requiredContextsSpy.mockRestore(); + } + }); + }); + // #selfhost-ci-verification: settings.expectedCiContexts must actually change the live-CI disposition, not just // get threaded through as an inert parameter. Branch protection is unreadable (empty) on BOTH calls, so without // expectedCiContexts folded into mergeRequiredCiContexts every check-run folds to "passed" (fold-all); WITH From 69bc6d66d52094085423c555342b20e5161a1473 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 4 Jul 2026 02:24:00 -0700 Subject: [PATCH 2/2] fix(review): skip write-through on required-context failure, invalidate on legacy CI events Two correctness gaps in the durable CI-state cache: 1. cachedFetchLiveCiAggregate persisted the live aggregate under the normal cache key even when cachedRequiredStatusContexts's lookup had failed (fail-open null required contexts), so a transient branch-protection read error could mask a repo's real required-context state for every reader until TTL expiry. Track whether the lookup actually resolved and skip the durable write-through when it didn't -- the live-fetched aggregate is still used for the current pass's own decision either way. 2. status/workflow_run webhook events weren't wired to invalidate the cache at all (only check_run/check_suite were), so a real legacy CI transition could leave prReadyForReview reading a stale pre-transition aggregate for up to the full TTL. Add a narrower invalidation-only handler for these two event types (re-review triggering stays out of scope, per the existing MVP boundary) that resolves affected PRs via the fast stored-DB head-SHA lookup and invalidates their cache entries. Both fixes verified by temporarily reintroducing the old behavior and confirming the new regression tests fail against it. --- src/queue/processors.ts | 73 ++++++++++++++++- test/unit/queue.test.ts | 173 ++++++++++++++++++++++++++++++++++++---- 2 files changed, 228 insertions(+), 18 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 12c1f28233..443846d78d 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -606,6 +606,11 @@ async function cachedFetchLiveCiAggregate( requiredContexts: ReadonlySet | null | undefined, requiredContextsKey: string, forceRefresh: boolean, + // False when the caller's own required-context lookup FAILED (not merely resolved to "none configured") -- + // that fail-open aggregate must never be persisted under the normal key, or a transient lookup error would + // mask the repo's real required-context state for every other reader until the entry's TTL expires (#selfhost- + // ci-verification gate review finding). The live-fetched aggregate is still returned to THIS caller either way. + requiredContextsResolved: boolean, admissionKey?: GitHubRateLimitAdmissionKey, ): Promise { const cached = await getPullRequestDetailSyncState(env, repoFullName, prNumber).catch(() => null); @@ -618,7 +623,9 @@ async function cachedFetchLiveCiAggregate( } incr(CI_STATE_CACHE_METRIC, { field: "aggregate", result: forceRefresh ? "forced" : "miss" }); const live = await fetchLiveCiAggregatePreferGraphQl(env, repoFullName, headSha, token, requiredContexts, admissionKey); - await writeThroughCiStateCache(env, repoFullName, prNumber, cached, headSha, requiredContextsKey, live); + if (requiredContextsResolved) { + await writeThroughCiStateCache(env, repoFullName, prNumber, cached, headSha, requiredContextsKey, live); + } return live; } @@ -640,9 +647,10 @@ function fetchLiveCiAggregateWithRequiredContexts( // cachedFetchLiveCiAggregate (#selfhost-ci-verification) is the durable, cross-job snapshot cache sibling to // this request-scoped LiveGithubFacts memo -- it is only ever consulted here, on a LiveGithubFacts miss. return cachedRequiredStatusContexts(env, repoFullName, facts, baseRef, token, expectedCiContexts, admissionKey) - .catch(() => null) - .then((requiredContexts) => - cachedFetchLiveCiAggregate(env, repoFullName, prNumber, headSha, token, requiredContexts, expectedCiContextsKeyPart(expectedCiContexts), forceRefresh, admissionKey), + .then((requiredContexts) => ({ requiredContexts, resolved: true })) + .catch(() => ({ requiredContexts: null, resolved: false })) + .then(({ requiredContexts, resolved }) => + cachedFetchLiveCiAggregate(env, repoFullName, prNumber, headSha, token, requiredContexts, expectedCiContextsKeyPart(expectedCiContexts), forceRefresh, resolved, admissionKey), ); } @@ -3706,6 +3714,58 @@ async function maybeReReviewOnCiCompletion( return true; } +/** + * Invalidate the durable CI-state cache on a legacy `status`/`workflow_run` event (#selfhost-ci-verification gate + * review finding). These two event types are NOT wired to re-review triggering (see maybeReReviewOnCiCompletion's + * own doc comment) -- that stays out of scope here -- but leaving the cache itself untouched meant a real legacy + * status/workflow_run transition could leave prReadyForReview reading a stale, pre-transition CI aggregate for up + * to the full cache TTL. Deliberately narrower than maybeReReviewOnCiCompletion: only resolves PR numbers via the + * fast stored-DB head-SHA lookup (no live GitHub fork-fallback call) -- a cache entry only exists for a PR this + * process already tracks, so there is nothing to invalidate for an untracked/fork PR the DB lookup misses. + */ +async function maybeInvalidateCiCacheOnLegacyCiEvent( + env: Env, + deliveryId: string, + eventName: string, + payload: GitHubWebhookPayload, +): Promise { + if (eventName !== "status" && eventName !== "workflow_run") return false; + const repoFullName = payload.repository?.full_name; + const installationId = getInstallationId(payload); + if (!repoFullName || !installationId) return false; + // `status`'s state settles the same event this-transition-matters signal that `action: "completed"` gives + // check_run/check_suite/workflow_run -- "pending" is an in-flight update, not a settled result worth + // invalidating over. workflow_run DOES carry `action`, exactly like check_run/check_suite. + const settled = + eventName === "status" + ? (payload as unknown as { state?: string }).state !== "pending" + : (payload as unknown as { action?: string }).action === "completed"; + if (settled && isConvergenceRepoAllowed(env, repoFullName)) { + const headSha = ( + eventName === "status" + ? ((payload as unknown as { sha?: string }).sha ?? "") + : ((payload as unknown as { workflow_run?: { head_sha?: string } }).workflow_run?.head_sha ?? "") + ).trim(); + if (headSha) { + const open = await listOpenPullRequests(env, repoFullName).catch(() => []); + for (const pr of open) { + if (pr.headSha !== headSha) continue; + await invalidateCiStateCache(env, repoFullName, pr.number).catch(() => undefined); + } + } + } + await recordWebhookEvent(env, { + deliveryId, + eventName, + action: payload.action, + installationId, + repositoryFullName: repoFullName, + payloadHash: "processed", + status: "processed", + }); + return true; +} + /** * Wake linked PRs on an issue-side signal (#2259). Labeling/unlabeling (e.g. maintainer-only) or * assigning/unassigning on a linked ISSUE can flip a linked-issue hard-rule verdict, but that only gets @@ -4975,6 +5035,11 @@ async function processGitHubWebhook( // red). Without this a PR that goes green/red AFTER its open-time review is never re-evaluated. if (await maybeReReviewOnCiCompletion(env, deliveryId, eventName, payload)) return; + // Legacy status/workflow_run CI signals aren't re-review triggers (see the function's own doc comment), but + // must still invalidate the durable CI-state cache so a tracked PR's next reader doesn't see a stale + // pre-transition aggregate for the rest of the cache TTL. + if (await maybeInvalidateCiCacheOnLegacyCiEvent(env, deliveryId, eventName, payload)) + return; // deployment_status (preview deploy finished) → re-review so the visual before/after capture fills in. if ( await maybeCaptureOnDeploymentStatus(env, deliveryId, eventName, payload) diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index aa5587fce0..bb6c067f23 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -1712,6 +1712,61 @@ describe("queue processors", () => { return { env }; } + it("a fresh but undeserializable cached row (corrupted JSON) is treated as a miss, not a crash", async () => { + const { env } = await seedRepoAndPr("a7"); + // Fresh by isCiStateCacheFresh's own contract (matching head_sha, matching -- here absent -- required- + // contexts key, recent ciStateFetchedAt), but ciFailingDetailsJson is malformed, so + // deserializeCachedCiAggregate's JSON.parse throws and it returns null -- the `if (deserialized)` false arm. + await upsertPullRequestDetailSyncState(env, { + repoFullName: "owner/agent-repo", + pullNumber: 7, + status: "complete", + ciHeadSha: "a7", + ciState: "passed", + ciHasPending: false, + ciHasVisiblePending: false, + ciHasMissingRequiredContext: false, + ciFailingDetailsJson: "not-json", + ciNonRequiredFailingDetailsJson: "[]", + ciCompletenessWarning: null, + ciRequiredContextsKey: "", + ciStateFetchedAt: new Date().toISOString(), + }); + const requiredContextsSpy = vi.spyOn(backfillModule, "fetchRequiredStatusContexts").mockResolvedValue(null); + const liveCiSpy = vi.spyOn(backfillModule, "fetchLiveCiAggregatePreferGraphQl").mockResolvedValue({ + ciState: "failed", + hasPending: false, + hasVisiblePending: false, + hasMissingRequiredContext: false, + failingDetails: [], + nonRequiredFailingDetails: [], + ciCompletenessWarning: null, + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = (init?.method ?? "GET").toUpperCase(); + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (/\/pulls\/7(?:\?|$)/.test(url) && method === "GET") return Response.json({ number: 7, title: "Cross-job CI cache", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, mergeable_state: "clean", labels: [], body: "Closes #1" }); + if (url.includes("/pulls/7/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + return Response.json({}); + }); + + try { + resetMetrics(); + await expect( + processJob(env, { type: "agent-regate-pr", deliveryId: "corrupted-cache-row", repoFullName: "owner/agent-repo", prNumber: 7, installationId: 9001 }), + ).resolves.toBeUndefined(); + expect(liveCiSpy).toHaveBeenCalled(); + // No "hit" recorded -- the corrupted row was NOT trusted; the live-fetched aggregate overwrote it. + expect(await renderMetrics()).not.toContain('gittensory_ci_state_cache_total{field="aggregate",result="hit"}'); + expect(await getPullRequestDetailSyncState(env, "owner/agent-repo", 7)).toMatchObject({ ciState: "failed", ciFailingDetailsJson: "[]" }); + } finally { + liveCiSpy.mockRestore(); + requiredContextsSpy.mockRestore(); + } + }); + it("a second agent-regate-pr pass for the SAME still-settled head_sha serves the readiness check from the durable cache (fewer live CI reads than the first pass)", async () => { const { env } = await seedRepoAndPr("a7"); const requiredContextsSpy = vi.spyOn(backfillModule, "fetchRequiredStatusContexts").mockResolvedValue(null); @@ -1868,11 +1923,63 @@ describe("queue processors", () => { } }); - // #selfhost-ci-verification MVP scope boundary: status/workflow_run events aren't handled by - // maybeReReviewOnCiCompletion for RE-REVIEW TRIGGERING today, so they don't invalidate the CI-state cache - // either -- the 60s TTL is the sole backstop for these two event types. This test documents that boundary - // explicitly rather than leaving it silently unverified. - it.each(["status", "workflow_run"])("a %s webhook event does not invalidate the durable CI-state cache (documented MVP boundary — TTL is the backstop)", async (eventName) => { + it("REGRESSION (#selfhost-ci-verification gate review): a required-context lookup failure never writes the fail-open aggregate through to the durable cache", async () => { + const { env } = await seedRepoAndPr("a7"); + const requiredContextsSpy = vi.spyOn(backfillModule, "fetchRequiredStatusContexts").mockRejectedValue(new Error("branch protection unavailable")); + const liveCiSpy = vi.spyOn(backfillModule, "fetchLiveCiAggregatePreferGraphQl").mockResolvedValue({ + ciState: "passed", + hasPending: false, + hasVisiblePending: false, + hasMissingRequiredContext: false, + failingDetails: [], + nonRequiredFailingDetails: [], + ciCompletenessWarning: null, + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = (init?.method ?? "GET").toUpperCase(); + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (/\/pulls\/7(?:\?|$)/.test(url) && method === "GET") return Response.json({ number: 7, title: "Cross-job CI cache", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, mergeable_state: "clean", labels: [], body: "Closes #1" }); + if (url.includes("/pulls/7/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + return Response.json({}); + }); + + try { + // This pass's OWN decision still uses the live-fetched (fail-open) aggregate normally -- only the + // DURABLE cache write is skipped, so a transient required-context lookup error can't poison what every + // OTHER reader sees for the rest of the TTL. + await expect( + processJob(env, { type: "agent-regate-pr", deliveryId: "required-contexts-lookup-fails", repoFullName: "owner/agent-repo", prNumber: 7, installationId: 9001 }), + ).resolves.toBeUndefined(); + expect(requiredContextsSpy).toHaveBeenCalled(); + expect(liveCiSpy).toHaveBeenCalled(); + + // Nothing was ever persisted under this PR's row -- ciState stays absent, not the fail-open "passed". + const row = await getPullRequestDetailSyncState(env, "owner/agent-repo", 7); + expect(row?.ciState ?? null).toBeNull(); + + // A subsequent pass (required-context lookup now succeeds) still correctly misses the cache and re-fetches + // live -- proving the earlier failed pass left no stale/poisoned entry behind for this reader either. + requiredContextsSpy.mockResolvedValue(null); + resetMetrics(); + await processJob(env, { type: "agent-regate-pr", deliveryId: "required-contexts-lookup-recovers", repoFullName: "owner/agent-repo", prNumber: 7, installationId: 9001 }); + expect(await renderMetrics()).toContain('gittensory_ci_state_cache_total{field="aggregate",result="miss"} 1'); + expect(await getPullRequestDetailSyncState(env, "owner/agent-repo", 7)).toMatchObject({ ciState: "passed" }); + } finally { + liveCiSpy.mockRestore(); + requiredContextsSpy.mockRestore(); + } + }); + + // #selfhost-ci-verification gate review finding: status/workflow_run events still aren't wired to + // RE-REVIEW TRIGGERING (see maybeReReviewOnCiCompletion's own doc comment -- that stays out of scope), but + // they now invalidate the durable CI-state cache directly via maybeInvalidateCiCacheOnLegacyCiEvent so a + // tracked PR's next reader within the TTL doesn't see a stale pre-transition aggregate. + it.each([ + ["status", (sha: string) => ({ state: "success", sha, repository: { name: "agent-repo", full_name: "owner/agent-repo", owner: { login: "owner" } }, installation: { id: 9001 } })], + ["workflow_run", (sha: string) => ({ action: "completed", workflow_run: { head_sha: sha }, repository: { name: "agent-repo", full_name: "owner/agent-repo", owner: { login: "owner" } }, installation: { id: 9001 } })], + ] as const)("a %s webhook event invalidates the durable CI-state cache for a tracked PR at the matching head SHA", async (eventName, buildPayload) => { const { env } = await seedRepoAndPr("a7"); const requiredContextsSpy = vi.spyOn(backfillModule, "fetchRequiredStatusContexts").mockResolvedValue(null); const liveCiSpy = vi.spyOn(backfillModule, "fetchLiveCiAggregatePreferGraphQl").mockResolvedValue({ @@ -1895,28 +2002,66 @@ describe("queue processors", () => { }); try { - await processJob(env, { type: "agent-regate-pr", deliveryId: "mvp-boundary-seed", repoFullName: "owner/agent-repo", prNumber: 7, installationId: 9001 }); + await processJob(env, { type: "agent-regate-pr", deliveryId: "legacy-ci-cache-seed", repoFullName: "owner/agent-repo", prNumber: 7, installationId: 9001 }); expect(await getPullRequestDetailSyncState(env, "owner/agent-repo", 7)).toMatchObject({ ciState: "passed" }); await processJob(env, { type: "github-webhook", deliveryId: `${eventName}-event`, eventName, - payload: { - action: "completed", - repository: { name: "agent-repo", full_name: "owner/agent-repo", owner: { login: "owner" } }, - installation: { id: 9001 }, - [eventName]: { sha: "a7", head_sha: "a7" }, - }, + payload: buildPayload("a7"), } as never); - // Still cached -- this event type is not wired to invalidateCiStateCache today. - expect(await getPullRequestDetailSyncState(env, "owner/agent-repo", 7)).toMatchObject({ ciState: "passed" }); + // Invalidated -- ciState is cleared to null, not left at the stale pre-transition "passed". + expect(await getPullRequestDetailSyncState(env, "owner/agent-repo", 7)).toMatchObject({ ciState: null, ciStateFetchedAt: null }); } finally { liveCiSpy.mockRestore(); requiredContextsSpy.mockRestore(); } }); + + it("a status/workflow_run webhook missing repository or installation info invalidates nothing (fails open, does not throw)", async () => { + const { env } = await seedRepoAndPr("a7"); + await upsertPullRequestDetailSyncState(env, { repoFullName: "owner/agent-repo", pullNumber: 7, status: "complete", ciHeadSha: "a7", ciState: "passed", ciStateFetchedAt: new Date().toISOString(), ciRequiredContextsKey: "" }); + await expect( + processJob(env, { + type: "github-webhook", + deliveryId: "status-no-installation", + eventName: "status", + payload: { state: "success", sha: "a7", repository: { name: "agent-repo", full_name: "owner/agent-repo", owner: { login: "owner" } } }, + } as never), + ).resolves.toBeUndefined(); + // No installation on the payload -- the function bails before ever consulting the cache. + expect(await getPullRequestDetailSyncState(env, "owner/agent-repo", 7)).toMatchObject({ ciState: "passed" }); + }); + + it.each([ + ["status", { state: "success", repository: { name: "agent-repo", full_name: "owner/agent-repo", owner: { login: "owner" } }, installation: { id: 9001 } }], + ["workflow_run", { action: "completed", workflow_run: {}, repository: { name: "agent-repo", full_name: "owner/agent-repo", owner: { login: "owner" } }, installation: { id: 9001 } }], + ] as const)("a %s webhook with no sha/head_sha on the payload invalidates nothing", async (eventName, payload) => { + const { env } = await seedRepoAndPr("a7"); + await upsertPullRequestDetailSyncState(env, { repoFullName: "owner/agent-repo", pullNumber: 7, status: "complete", ciHeadSha: "a7", ciState: "passed", ciStateFetchedAt: new Date().toISOString(), ciRequiredContextsKey: "" }); + await expect( + processJob(env, { type: "github-webhook", deliveryId: `${eventName}-no-sha`, eventName, payload } as never), + ).resolves.toBeUndefined(); + expect(await getPullRequestDetailSyncState(env, "owner/agent-repo", 7)).toMatchObject({ ciState: "passed" }); + }); + + it("a status webhook for a DIFFERENT head SHA than any open PR invalidates nothing (loop's non-matching arm)", async () => { + const { env } = await seedRepoAndPr("a7"); + await upsertPullRequestDetailSyncState(env, { repoFullName: "owner/agent-repo", pullNumber: 7, status: "complete", ciHeadSha: "a7", ciState: "passed", ciStateFetchedAt: new Date().toISOString(), ciRequiredContextsKey: "" }); + await expect( + processJob(env, { + type: "github-webhook", + deliveryId: "status-unmatched-sha", + eventName: "status", + payload: { state: "success", sha: "different-sha", repository: { name: "agent-repo", full_name: "owner/agent-repo", owner: { login: "owner" } }, installation: { id: 9001 } }, + } as never), + ).resolves.toBeUndefined(); + // The tracked PR's own head_sha ("a7") doesn't match this event's sha -- the loop's continue arm fires, + // and its cache entry is left untouched. + expect(await getPullRequestDetailSyncState(env, "owner/agent-repo", 7)).toMatchObject({ ciState: "passed" }); + }); }); // #selfhost-ci-verification: settings.expectedCiContexts must actually change the live-CI disposition, not just