From 6b978a64481b98de972aeae48c366bb5167dcc8e Mon Sep 17 00:00:00 2001 From: ghost <49853598+JSONbored@users.noreply.github.com> Date: Tue, 7 Jul 2026 00:37:40 -0700 Subject: [PATCH] fix(review): keep missing required CI pending --- src/queue/processors.ts | 24 ++++++++++++++++-------- test/unit/queue.test.ts | 16 ++++++++++------ 2 files changed, 26 insertions(+), 14 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 92e3ed89cd..887b286900 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -3440,14 +3440,10 @@ async function prReadyForReview( // 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 // gate FINALIZE so the PR surfaces. A trusted required/base-repo visibly queued/in_progress CI signal is - // active CI, though, so never cut in front of it. A required context that never appeared in any page this - // fetch read to completion (hasMissingRequiredContext) has NO webhook to ever wait for — nothing fires - // check_run/check_suite "completed" for a context name that structurally never runs (a path-filtered - // workflow, a mistyped branch-protection context) — so it gets a much shorter cap than genuinely active or - // merely unreadable/non-required pending CI (#selfhost-ci-deferral-staleness). first-seen is tracked in the - // self-host Redis transient cache per PR+headSha (a new push = a fresh window, and the SAME key anchors - // both cap classes so a pending reason that changes class mid-window doesn't reset the clock); a cache - // miss degrades to the old defer. (#ci-stuck-finalize) + // active CI, though, so never cut in front of it. first-seen is tracked in the self-host Redis transient + // cache per PR+headSha (a new push = a fresh window, and the SAME key anchors both cap classes so a pending + // reason that changes class mid-window doesn't reset the clock); a cache miss degrades to the old defer. + // (#ci-stuck-finalize) const deferCapMs = ci.hasMissingRequiredContext ? MISSING_REQUIRED_CONTEXT_DEFER_MS : STUCK_CI_DEFER_MS; if ( ci.hasVisiblePending || @@ -3463,6 +3459,18 @@ async function prReadyForReview( }).catch(() => undefined); return false; } + if (ci.hasMissingRequiredContext) { + await recordAuditEvent(env, { + eventType: "github_app.review_deferred_ci_pending", + actor: "gittensory", + targetKey: `${repoFullName}#${pr.number}`, + outcome: "queued", + detail: + "Required CI context is still missing — review deferred instead of publishing a passing gate before expected CI reports", + metadata: { deliveryId, repoFullName }, + }).catch(() => undefined); + return false; + } // #orb-ci-stuck-repeat: finalizing here runs a full paid AI review -- but a permanently-stuck CI context // (a fork check that will never report, an orphaned required context) never resolves, so every later // evaluation of the SAME head SHA hits this exact branch again and would re-spend another review for a diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 4eaa7612a0..28dab72521 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -1859,16 +1859,16 @@ describe("queue processors", () => { } }); - it("finalizes a missing-required-context PR within minutes, well before the old 30-minute stale-CI cap (#selfhost-ci-deferral-staleness)", async () => { + it("keeps deferring a missing-required-context PR after the short surfacing cap (#selfhost-ci-deferral-staleness)", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); 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); await upsertRepositorySettings(env, { repoFullName: "owner/agent-repo", autonomy: { merge: "auto", update_branch: "auto" }, aiReviewMode: "off", gatePack: "oss-anti-slop", gateCheckMode: "enabled", checkRunMode: "off", commentMode: "off", publicSurface: "off" }); await upsertPullRequestFromGitHub(env, "owner/agent-repo", { number: 7, title: "Missing required context, past short cap", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, base: { ref: "main" }, labels: [], body: "Closes #1" }); vi.setSystemTime(new Date("2026-05-28T02:00:00.000Z")); - // 3 minutes elapsed: past the new 2-minute missing-required-context cap, but nowhere near the old 30-minute - // cap — this is the key regression proving #selfhost-ci-deferral-staleness actually shortens the wait - // instead of the PR sitting deferred for up to half an hour on a context that will never post. + // 3 minutes elapsed: past the 2-minute missing-required-context surfacing cap, but nowhere near the old + // 30-minute stale-CI cap. Missing required contexts must still not publish a passing gate before expected CI + // reports, because the review check may itself be branch-protection-required. await env.SELFHOST_TRANSIENT_CACHE?.set( "ci-pending-first-seen:owner/agent-repo#7:a7", String(Date.now() - 3 * 60 * 1000), @@ -1902,11 +1902,15 @@ describe("queue processors", () => { try { await processJob(env, { type: "agent-regate-pr", deliveryId: "missing-context-past-short-cap", repoFullName: "owner/agent-repo", prNumber: 7, installationId: 9001 }); - expect(gateChecks).toBeGreaterThan(0); + expect(gateChecks).toBe(0); + const deferred = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?") + .bind("github_app.review_deferred_ci_pending") + .first<{ n: number }>(); const finalized = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?") .bind("github_app.review_finalized_ci_stuck") .first<{ n: number }>(); - expect(finalized?.n).toBe(1); + expect(deferred?.n).toBe(1); + expect(finalized?.n).toBe(0); } finally { liveCiSpy.mockRestore(); requiredContextsSpy.mockRestore();