diff --git a/src/github/backfill.ts b/src/github/backfill.ts index f80c1b48ed..8573942dd1 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -885,9 +885,46 @@ export const OPTIONAL_CONTENTS_WRITE_PERMISSION: Record = { contents: "write", }; -export const REQUIRED_INSTALLATION_EVENTS = ["issues", "issue_comment", "pull_request", "repository"] as const; +/** + * Events without which LoopOver is functionally broken, so a missing one keeps the installation at + * `needs_attention` with named remediation. + * + * #9058 promoted three. `check_run` and `check_suite` are how CI settlement reaches ORB at all — + * maybeReReviewOnCiCompletion is documented in its own header as THE auto-merge / close-on-red trigger — so an + * installation missing them reported "healthy" while silently degrading from event-driven to sweep-only, which + * is exactly the "the gate hung" shape an operator cannot diagnose from a green health page. `pull_request_review` + * is the same story for the human-approval signal the merge gate reads. + */ +export const REQUIRED_INSTALLATION_EVENTS = [ + "issues", + "issue_comment", + "pull_request", + "pull_request_review", + "repository", + "check_run", + "check_suite", +] as const; + +/** + * Events LoopOver uses but can do without — a missing one degrades a specific path rather than the product, so + * these are DIAGNOSED (surfaced in health) without holding the installation at needs_attention (#9058). + * `status` in particular matters more than its optional standing suggests: codecov/patch is a commit status, + * and it is the gate's hardest required check. + */ +export const DIAGNOSED_INSTALLATION_EVENTS = ["pull_request_review_thread", "status", "workflow_run", "deployment_status", "push"] as const; + export const OPTIONAL_VISIBLE_INSTALLATION_EVENTS = ["installation_target", "installation_repositories"] as const; +/** + * The complete event set a new App should subscribe to: everything required, plus everything diagnosed. + * + * #9058 — the setup wizard used to hand-maintain its own `default_events` list, and it had drifted: it omitted + * `issue_comment` and `repository`, both of which the health check calls REQUIRED. A wizard-created self-host + * therefore started with every `@loopover …` command dead and no rename handling, from a manifest the product's + * own health page would immediately mark unhealthy. Deriving one from the other makes that drift impossible. + */ +export const RECOMMENDED_APP_EVENTS: readonly string[] = [...REQUIRED_INSTALLATION_EVENTS, ...DIAGNOSED_INSTALLATION_EVENTS]; + type InstallationModeImpact = { mode: "comment" | "label" | "check_run" | "gate_check" | "agent_pr_action" | "agent_merge"; enabled: boolean; diff --git a/src/github/pr-freshness.ts b/src/github/pr-freshness.ts index 3829c812a1..0ae4e6a342 100644 --- a/src/github/pr-freshness.ts +++ b/src/github/pr-freshness.ts @@ -10,6 +10,9 @@ type PullRequestFreshnessOptions = { requireDraft?: boolean; unavailableSource?: PullRequestUnavailableSource; unavailableDetail?: string; + // #9055: the base the caller last computed the diff/review/CI against. Present only when the caller actually + // tracked one (older stored PRs predate the column); absent preserves every existing caller's behavior exactly. + expectedBaseRef?: string | null | undefined; }; export type PullRequestFreshness = @@ -24,7 +27,13 @@ export type PullRequestFreshness = } | { status: "stale"; - reason: "unavailable" | "closed" | "head_unresolved" | "head_changed" | "no_longer_draft"; + // #9055: `base_changed` — a contributor can retarget a PR's base AFTER CI is green with no new commit, + // so head/state/draft alone see nothing wrong. Everything downstream (diff, review, CI, guardrail path + // matching, migration-collision detection) was computed against the ABANDONED base, and the divergence + // was permanent for that head: nothing re-syncs on a base change alone. This is checked at the last + // possible moment, immediately before a merge/approve mutation, using the SAME live fetch that already + // proves the head — no extra GitHub call. + reason: "unavailable" | "closed" | "head_unresolved" | "head_changed" | "no_longer_draft" | "base_changed"; expectedHeadSha: string | null; liveHeadSha: string | null; liveState: string | null; @@ -45,7 +54,7 @@ export function reviewedPullRequestHeadSha( } export function classifyPullRequestFreshness( - live: Pick | null | undefined, + live: Pick | null | undefined, expectedHeadSha: string | null | undefined, options?: PullRequestFreshnessOptions, ): PullRequestFreshness { @@ -83,6 +92,12 @@ export function classifyPullRequestFreshness( if (expected && liveHeadSha !== expected) { return { status: "stale", reason: "head_changed", expectedHeadSha: expected, liveHeadSha, liveState }; } + // #9055: the base can change with the head UNCHANGED, which is exactly the case the check above cannot see. + // A repo whose per-repo settings pin a specific expected base (options?.expectedBaseRef) denies the mutation + // rather than merging into a base the diff/review/CI were never computed against. + if (options?.expectedBaseRef && live.base?.ref && live.base.ref !== options.expectedBaseRef) { + return { status: "stale", reason: "base_changed", expectedHeadSha: expected, liveHeadSha, liveState }; + } // The draft-dodge close is only justified while the PR is STILL a draft -- a same-head, still-open PR // that was converted back to ready_for_review before the close fires has cleared its own justification // (#2130 follow-up: head/state alone can't see this transition). @@ -103,10 +118,14 @@ export async function fetchPullRequestFreshness( // Require the LIVE PR to still be a draft (the draft-dodge close's own justification). Absent/false // preserves every other caller's existing head/state-only behavior exactly. requireDraft?: boolean; + // #9055: see PullRequestFreshnessOptions' own doc comment. + expectedBaseRef?: string | null | undefined; }, ): Promise { - const options: PullRequestFreshnessOptions = - args.requireDraft !== undefined ? { requireDraft: args.requireDraft } : {}; + const options: PullRequestFreshnessOptions = { + ...(args.requireDraft !== undefined ? { requireDraft: args.requireDraft } : {}), + ...(args.expectedBaseRef ? { expectedBaseRef: args.expectedBaseRef } : {}), + }; let tokenError: unknown; const installationToken = await createInstallationToken(env, args.installationId).catch((error) => { tokenError = error; @@ -148,5 +167,6 @@ export function pullRequestFreshnessDetail(result: PullRequestFreshness): string if (result.reason === "closed") return `PR is no longer open (live state: ${result.liveState ?? "unknown"})`; if (result.reason === "head_unresolved") return "live PR head SHA could not be verified"; if (result.reason === "no_longer_draft") return "PR is no longer a draft"; + if (result.reason === "base_changed") return "PR base branch changed since the diff/review/CI it will merge with were computed"; return `PR head changed from ${result.expectedHeadSha ?? "unknown"} to ${result.liveHeadSha ?? "unknown"}`; } diff --git a/src/queue/processors.ts b/src/queue/processors.ts index dc84d64d94..ad8c8bd624 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -698,6 +698,13 @@ const PR_PUBLIC_SURFACE_ACTIONS = new Set([ "synchronize", "ready_for_review", "edited", + // #9059: a maintainer adding or removing a disposition label IS a disposition input -- the manual-review hold + // is read straight off the PR's labels. Without these, adding the label did a row re-sync and nothing else, + // so the hold only took effect on the next ~2-minute sweep, and REMOVING it to unblock a PR had the same lag + // in the other direction. A sweep that is itself skipped under REST-budget backpressure makes that lag + // unbounded, which is how manually unblocking a PR ends up looking like the gate ignoring you. + "labeled", + "unlabeled", ]); const PR_GATE_CLOSED_ACTIONS = new Set(["closed"]); // #4818 follow-up: the three review-family event names `shouldProcessPullRequestPublicSurface` (below) also @@ -2883,6 +2890,9 @@ async function maybeCloseForContributorCapOnOpen( repoFullName, pullNumber: pr.number, headSha: pr.headSha, + // #9055: threaded so the executor's live pre-merge check denies a merge/approve into a base the diff, + // review, and CI it is acting on were never computed against. + expectedBaseRef: pr.baseRef, autonomy: settings.autonomy, agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun, @@ -3604,6 +3614,7 @@ async function runAgentMaintenancePlanAndExecute( repoFullName, pullNumber: pr.number, headSha: pr.headSha, + expectedBaseRef: pr.baseRef, autonomy: settings.autonomy, agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun, @@ -4984,6 +4995,19 @@ async function maybeReReviewOnLinkedIssueChange( const installationId = getInstallationId(payload); const issueNumber = payload.issue?.number; if (!repoFullName || !installationId || !issueNumber) return false; + // #9059: persist the issue row BEFORE the wake fan-out. This function returns `true` unconditionally once + // repo + installation + issue are present, and the caller short-circuits on that -- so handleIssueWebhookEvent + // never ran, and `issues.labels_json` plus assignees only advanced on opened/edited/closed/reopened or when + // the (up to 6-hourly) backfill reached the repo. Relabelling is the single most common issue mutation, so + // the row most likely to be read was the row most likely to be stale. + // + // Blast radius was bounded -- the linked-issue HARD rules fetch live and uncached -- but + // resolveLinkedIssueAuthorLogins is cache-first and only falls back live on a MISS, not on staleness, and the + // issue-side advisories, slop triage, enrichment and the MCP/API issue surfaces all read these rows directly. + // + // Best-effort: this is a bookkeeping write, and failing it must not cost the PRs their wake, which is the + // part that actually changes a disposition. + if (payload.issue) await upsertIssueFromGitHub(env, repoFullName, payload.issue).catch(() => undefined); // #5385: mirrors sweepRepoRegate's own gate exactly -- a repo with acting autonomy configured but NOT in the // LOOPOVER_REVIEW_REPOS allowlist (e.g. removed during a rollback, or a self-hoster who configured autonomy // without also updating the env allowlist) used to silently never wake affected PRs here, leaving a stale @@ -6443,6 +6467,20 @@ async function handlePullRequestWebhookEvent( /* v8 ignore next -- best-effort: invalidatePrStateCache never rejects against a healthy D1, and a cache-invalidation failure here must never block the webhook. */ await invalidatePrStateCache(env, repoFullName, pr.number).catch(() => undefined); } + // #9055 — a base retarget with the HEAD unchanged. A contributor can move a green PR onto a new base after + // CI passed against the old one; GitHub does not re-run `pull_request` workflows or emit a new head SHA for + // this, so nothing else in this handler notices. Left alone, the stored diff/patches, the AI-review cache + // (its fingerprint deliberately excludes baseSha), and the CI aggregate all keep describing the ABANDONED + // base — and the divergence is permanent for that head, since the sweep re-syncs only on head/label drift. + // `changes.base` is GitHub's own signal that this specific mutation happened; treated as review-invalidating + // exactly like a new commit or a fresh review event, and files are FORCED (bypassing the head-SHA-keyed + // `filesUpToDate` check that would otherwise skip the refetch entirely since the head has not moved). + if (eventName === "pull_request" && payload.action === "edited" && payload.changes?.base?.from?.ref) { + await invalidatePrStateCache(env, repoFullName, pr.number).catch(() => undefined); + await invalidateCiStateCache(env, repoFullName, pr.number).catch(() => undefined); + await markPullRequestReviewsInvalidated(env, repoFullName, pr.number).catch(() => undefined); + await refreshPullRequestDetails(env, repoFullName, pr.number, { force: true }).catch(() => undefined); + } // Review-evasion protection (#review-evasion-protection): a head change (synchronize) invalidates any // active-review tracking for the OLD head immediately -- a fresh pass starts its own tracking later in // this same handler. Best-effort; the guarded CAS update is a safe no-op when nothing is active. The diff --git a/src/selfhost/setup-wizard.ts b/src/selfhost/setup-wizard.ts index 87fc81252a..d469df4e76 100644 --- a/src/selfhost/setup-wizard.ts +++ b/src/selfhost/setup-wizard.ts @@ -5,6 +5,7 @@ // disabled once an App is configured (server.ts gates on GITHUB_APP_ID), so this can't rebind a live install. import { createHmac, timingSafeEqual } from "node:crypto"; import { timeoutFetch } from "../github/client"; +import { RECOMMENDED_APP_EVENTS } from "../github/backfill"; export const SETUP_TOKEN_FORM_MAX_BYTES = 4096; @@ -59,7 +60,11 @@ export function buildManifest(origin: string, state: string): Record { expect(fetchPullRequestFreshness).toHaveBeenCalledWith(env, expect.objectContaining({ expectedHeadSha: "reviewed-sha" })); }); + // #9055: a contributor can retarget a PR's base with the head unchanged, so nothing else in the freshness + // check sees anything wrong. The executor's live pre-mutation check is the last chance to catch it, and it + // must be given something to check against. + it("threads the context's expectedBaseRef into the live freshness check before a merge (#9055)", async () => { + const env = createTestEnv({}); + await executeAgentMaintenanceActions(env, ctx({ expectedBaseRef: "main" }), [merge]); + expect(fetchPullRequestFreshness).toHaveBeenCalledWith(env, expect.objectContaining({ expectedBaseRef: "main" })); + }); + + it("denies a merge when the live base has moved even though the head is current (#9055)", async () => { + const env = createTestEnv({}); + vi.mocked(fetchPullRequestFreshness).mockResolvedValueOnce({ status: "stale", reason: "base_changed", expectedHeadSha: "sha7", liveHeadSha: "sha7", liveState: "open" }); + + const outcomes = await executeAgentMaintenanceActions(env, ctx({ expectedBaseRef: "main" }), [merge]); + + expect(outcomes[0]).toMatchObject({ actionClass: "merge", outcome: "denied", detail: expect.stringContaining("base branch changed") }); + expect(mergePullRequest).not.toHaveBeenCalled(); + }); + it("LIVE approve pins the review to the action's reviewed head (expectedHeadSha) over the context head, falling back to an empty body (#2262)", async () => { const env = createTestEnv({}); // A staged approve replayed on accept carries the REVIEWED head — same pin as merge already has — and this diff --git a/test/unit/issue-label-churn-upsert.test.ts b/test/unit/issue-label-churn-upsert.test.ts new file mode 100644 index 0000000000..5859100cd6 --- /dev/null +++ b/test/unit/issue-label-churn-upsert.test.ts @@ -0,0 +1,88 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { clearInstallationTokenCacheForTest } from "../../src/github/app"; +import { getIssue, upsertInstallation, upsertIssueFromGitHub, upsertRepositoryFromGitHub } from "../../src/db/repositories"; +import { processJob } from "../../src/queue/processors"; +import { generatePrivateKeyPem } from "../helpers/github-app-key"; +import { createTestEnv } from "../helpers/d1"; + +// #9059(a): maybeReReviewOnLinkedIssueChange returns `true` unconditionally once repo + installation + issue +// are present, and the caller short-circuits on that BEFORE handleIssueWebhookEvent — the only place +// upsertIssueFromGitHub is otherwise called for an `issues` event. So `issues.labels_json` and assignees only +// advanced on opened/edited/closed/reopened, or when the (up to 6-hourly) backfill reached the repo. Label +// churn is the single most common issue mutation, so the row most likely to be read (by issue-side advisories, +// slop triage, enrichment, and the MCP/API issue surfaces) was the row most likely to be stale. +describe("relabelling an issue updates its stored row immediately (#9059)", () => { + afterEach(() => { + vi.unstubAllGlobals(); + clearInstallationTokenCacheForTest(); + }); + + it("advances issues.labels_json on a labeled event instead of leaving it stale", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertInstallation(env, { + action: "created", + installation: { id: 9400, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: {}, events: [] }, + }); + await upsertRepositoryFromGitHub(env, { name: "issue-repo", full_name: "owner/issue-repo", private: false, owner: { login: "owner" } }, 9400); + await upsertIssueFromGitHub(env, "owner/issue-repo", { number: 5, title: "Bug report", state: "open", labels: [], body: "x" }); + + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + // No open PRs link this issue — the wake fan-out itself has nothing to do, which is exactly the case + // that used to make the return-true short-circuit a pure loss with no compensating benefit. + if (url.includes("/search/issues")) return Response.json({ items: [] }); + return new Response("not found", { status: 404 }); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "issue-labeled-1", + eventName: "issues", + payload: { + action: "labeled", + installation: { id: 9400 }, + repository: { name: "issue-repo", full_name: "owner/issue-repo", private: false, owner: { login: "owner" } }, + issue: { number: 5, title: "Bug report", state: "open", labels: [{ name: "priority:high" }], body: "x" }, + label: { name: "priority:high" }, + }, + }); + + const issue = await getIssue(env, "owner/issue-repo", 5); + expect(issue?.labels).toEqual(["priority:high"]); + }); + + it("advances the row's title/state on an assignment-change event too, not just labeled", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertInstallation(env, { + action: "created", + installation: { id: 9401, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: {}, events: [] }, + }); + await upsertRepositoryFromGitHub(env, { name: "assign-issue-repo", full_name: "owner/assign-issue-repo", private: false, owner: { login: "owner" } }, 9401); + await upsertIssueFromGitHub(env, "owner/assign-issue-repo", { number: 6, title: "Stale title", state: "open", labels: [], body: "x" }); + + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/search/issues")) return Response.json({ items: [] }); + return new Response("not found", { status: 404 }); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "issue-assigned-1", + eventName: "issues", + payload: { + action: "assigned", + installation: { id: 9401 }, + repository: { name: "assign-issue-repo", full_name: "owner/assign-issue-repo", private: false, owner: { login: "owner" } }, + // The title changed too — this is what the row upsert running is what actually proves: BEFORE the fix, + // an assignment event never reached upsertIssueFromGitHub at all, so this rename would stay invisible. + issue: { number: 6, title: "Renamed while assigning", state: "open", labels: [], body: "x" }, + }, + }); + + const issue = await getIssue(env, "owner/assign-issue-repo", 6); + expect(issue?.title).toBe("Renamed while assigning"); + }); +}); diff --git a/test/unit/pending-closure-watchdog.test.ts b/test/unit/pending-closure-watchdog.test.ts index a9303624fd..7bbab43916 100644 --- a/test/unit/pending-closure-watchdog.test.ts +++ b/test/unit/pending-closure-watchdog.test.ts @@ -87,6 +87,26 @@ describe("sweepStrandedPendingClosures (#9031)", () => { expect(await sweepStrandedPendingClosures(env, now + PENDING_CLOSURE_LOOKBACK_MS + 60_000)).toEqual({ scanned: 0, requeued: 0 }); }); + it("falls back to the flag's own timestamp when the deadline is absent entirely", async () => { + const { env, sent } = envWithQueue(); + await seedPr(env, 19, "open"); + await env.DB.prepare("INSERT INTO audit_events (id, event_type, actor, target_key, outcome, detail, metadata_json, created_at) VALUES (?,?,?,?,?,?,?,?)") + .bind( + crypto.randomUUID(), + "agent.linked_issue.pending_closure_flagged", + "loopover", + "alice/repo#19", + "queued", + "flagged", + JSON.stringify({ repoFullName: "alice/repo", pullNumber: 19, installationId: 88 }), + new Date(Date.now() - 3 * PENDING_CLOSURE_GRACE_MS).toISOString(), + ) + .run(); + + expect((await sweepStrandedPendingClosures(env)).requeued).toBe(1); + expect(sent).toHaveLength(1); + }); + it("falls back to the flag's own timestamp when the recorded deadline is unreadable", async () => { const { env, sent } = envWithQueue(); await seedPr(env, 16, "open"); @@ -131,6 +151,56 @@ describe("sweepStrandedPendingClosures (#9031)", () => { expect((await sweepStrandedPendingClosures(env, now + 1000)).scanned).toBe(1); }); + it("still applies the flag when its own audit write fails — best-effort, like the enqueue it accompanies", async () => { + const { env } = envWithQueue(); + const repositories = await import("../../src/db/repositories"); + vi.spyOn(repositories, "recordAuditEvent").mockRejectedValue(new Error("audit down")); + await expect(recordPendingClosureFlag(env, { repoFullName: "alice/repo", pullNumber: 30, installationId: 88 }, Date.now())).resolves.toBeUndefined(); + vi.restoreAllMocks(); + }); + + it("counts the rescue even when the follow-up audit write fails", async () => { + const { env, sent } = envWithQueue(); + await seedPr(env, 31, "open"); + const now = Date.now(); + await recordPendingClosureFlag(env, { repoFullName: "alice/repo", pullNumber: 31, installationId: 88 }, PAST_DUE(now)); + const repositories = await import("../../src/db/repositories"); + vi.spyOn(repositories, "recordAuditEvent").mockRejectedValue(new Error("audit down")); + + // The job WAS enqueued, which is the part that rescues the PR; losing the bookkeeping row must not undo it. + expect((await sweepStrandedPendingClosures(env, now)).requeued).toBe(1); + expect(sent).toHaveLength(1); + vi.restoreAllMocks(); + }); + + it("treats an unreadable rescue history as 'already rescued', so a broken ledger cannot cause a flood", async () => { + const { env, sent } = envWithQueue(); + await seedPr(env, 32, "open"); + const now = Date.now(); + await recordPendingClosureFlag(env, { repoFullName: "alice/repo", pullNumber: 32, installationId: 88 }, PAST_DUE(now)); + const repositories = await import("../../src/db/repositories"); + vi.spyOn(repositories, "countRecentAuditEventsForActorAndTarget").mockRejectedValue(new Error("db down")); + + expect((await sweepStrandedPendingClosures(env, now)).requeued).toBe(0); + expect(sent).toEqual([]); + vi.restoreAllMocks(); + }); + + it("skips a flag whose pullNumber or installationId is the wrong shape", async () => { + const { env, sent } = envWithQueue(); + for (const [n, metadata] of [ + [40, { repoFullName: "alice/repo", pullNumber: "40", installationId: 88 }], + [41, { repoFullName: "alice/repo", pullNumber: 41 }], + [42, { pullNumber: 42, installationId: 88 }], + ] as const) { + await env.DB.prepare("INSERT INTO audit_events (id, event_type, actor, target_key, outcome, detail, metadata_json, created_at) VALUES (?,?,?,?,?,?,?,?)") + .bind(crypto.randomUUID(), "agent.linked_issue.pending_closure_flagged", "loopover", `alice/repo#${n}`, "queued", "flagged", JSON.stringify(metadata), new Date().toISOString()) + .run(); + } + expect((await sweepStrandedPendingClosures(env)).requeued).toBe(0); + expect(sent).toEqual([]); + }); + it("returns an empty result instead of throwing when the scan fails", async () => { const { env } = envWithQueue(); const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); diff --git a/test/unit/pr-base-retarget-webhook.test.ts b/test/unit/pr-base-retarget-webhook.test.ts new file mode 100644 index 0000000000..49026ac797 --- /dev/null +++ b/test/unit/pr-base-retarget-webhook.test.ts @@ -0,0 +1,116 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { clearInstallationTokenCacheForTest } from "../../src/github/app"; +import { getPullRequestDetailSyncState, upsertInstallation, upsertPullRequestDetailSyncState, upsertPullRequestFromGitHub, upsertRepositoryFromGitHub, upsertRepositorySettings } from "../../src/db/repositories"; +import { processJob } from "../../src/queue/processors"; +import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; +import { generatePrivateKeyPem } from "../helpers/github-app-key"; +import { createTestEnv } from "../helpers/d1"; + +// #9055: a contributor can retarget a PR's base branch AFTER CI is green, with the head SHA UNCHANGED. GitHub +// does not re-run pull_request workflows or emit a new head for this, so a stored PR whose "files up to date" +// check keys on head SHA alone (filesUpToDate) skips the files refetch entirely — the diff, review, and CI +// aggregate all keep describing the ABANDONED base. `pull_request.edited` with `changes.base` is GitHub's own +// signal that this specific mutation happened, and it is the one thing available to notice it. +describe("a base retarget forces a fresh sync even though the head has not changed (#9055)", () => { + afterEach(() => { + vi.unstubAllGlobals(); + clearInstallationTokenCacheForTest(); + }); + + async function seed(env: Env): Promise { + await upsertInstallation(env, { + action: "created", + installation: { id: 9200, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: {}, events: [] }, + }); + await upsertRepositoryFromGitHub(env, { name: "retarget-repo", full_name: "owner/retarget-repo", private: false, owner: { login: "owner" } }, 9200); + await upsertRepositorySettings(env, { repoFullName: "owner/retarget-repo" }); + await upsertRepoFocusManifest(env, "owner/retarget-repo", { settings: { checkRunMode: "off", commentMode: "off", publicSurface: "off" } }); + await upsertPullRequestFromGitHub(env, "owner/retarget-repo", { number: 21, title: "Retargeted PR", state: "open", user: { login: "contributor" }, head: { sha: "r21" }, base: { ref: "main" }, labels: [], body: "x" }); + // A prior, COMPLETE sync for the same head — this is exactly the state filesUpToDate would otherwise treat + // as "nothing to do", which is the bug: the head really hasn't changed, only the base has. + await upsertPullRequestDetailSyncState(env, { repoFullName: "owner/retarget-repo", pullNumber: 21, status: "complete", headSha: "r21", filesSyncedAt: "2026-01-01T00:00:00.000Z", reviewsSyncedAt: "2026-01-01T00:00:00.000Z", checksSyncedAt: "2026-01-01T00:00:00.000Z", lastSyncedAt: "2026-01-01T00:00:00.000Z" }); + } + + function stubGitHub(fileFetchCount: { n: number }): void { + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/21/files")) { + fileFetchCount.n += 1; + return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + } + if (/\/pulls\/21(?:\?|$)/.test(url)) return Response.json({ number: 21, title: "Retargeted PR", state: "open", user: { login: "contributor" }, head: { sha: "r21" }, base: { ref: "release" }, labels: [], body: "x" }); + if (url.includes("/commits/r21/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/r21/status")) return Response.json({ state: "success", statuses: [] }); + return new Response("not found", { status: 404 }); + }); + } + + it("re-fetches files on a base-changing edit, bypassing the head-keyed freshness check that would otherwise skip it", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seed(env); + const fileFetchCount = { n: 0 }; + stubGitHub(fileFetchCount); + + await processJob(env, { + type: "github-webhook", + deliveryId: "retarget-1", + eventName: "pull_request", + payload: { + action: "edited", + installation: { id: 9200 }, + repository: { name: "retarget-repo", full_name: "owner/retarget-repo", private: false, owner: { login: "owner" } }, + pull_request: { number: 21, title: "Retargeted PR", state: "open", user: { login: "contributor" }, head: { sha: "r21" }, base: { ref: "release" }, labels: [], body: "x" }, + changes: { base: { from: { ref: "main" } } }, + }, + }); + + // The stale-complete sync state would otherwise have made this zero. + expect(fileFetchCount.n).toBeGreaterThan(0); + }); + + it("does NOT force a refetch for an ordinary edit that does not touch the base", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seed(env); + const fileFetchCount = { n: 0 }; + stubGitHub(fileFetchCount); + + await processJob(env, { + type: "github-webhook", + deliveryId: "retarget-2", + eventName: "pull_request", + payload: { + action: "edited", + installation: { id: 9200 }, + repository: { name: "retarget-repo", full_name: "owner/retarget-repo", private: false, owner: { login: "owner" } }, + pull_request: { number: 21, title: "Retitled, not retargeted", state: "open", user: { login: "contributor" }, head: { sha: "r21" }, base: { ref: "main" }, labels: [], body: "x" }, + changes: {}, + }, + }); + + expect(fileFetchCount.n).toBe(0); + }); + + it("invalidates the stored sync state, not just refetching files silently", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seed(env); + stubGitHub({ n: 0 }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "retarget-3", + eventName: "pull_request", + payload: { + action: "edited", + installation: { id: 9200 }, + repository: { name: "retarget-repo", full_name: "owner/retarget-repo", private: false, owner: { login: "owner" } }, + pull_request: { number: 21, title: "Retargeted PR", state: "open", user: { login: "contributor" }, head: { sha: "r21" }, base: { ref: "release" }, labels: [], body: "x" }, + changes: { base: { from: { ref: "main" } } }, + }, + }); + + const state = await getPullRequestDetailSyncState(env, "owner/retarget-repo", 21); + // A fresh sync ran and re-recorded a timestamp — not the stale 2026-01-01 marker seeded above. + expect(state?.lastSyncedAt).not.toBe("2026-01-01T00:00:00.000Z"); + }); +}); diff --git a/test/unit/pr-freshness.test.ts b/test/unit/pr-freshness.test.ts index 989a0f9993..80b9687bda 100644 --- a/test/unit/pr-freshness.test.ts +++ b/test/unit/pr-freshness.test.ts @@ -272,4 +272,43 @@ describe("PR freshness guards", () => { unavailableDetail: expect.any(String), }); }); + + // #9055: a contributor can retarget a PR's base with the head UNCHANGED — CI green against the old base, + // review computed against it, no new commit anywhere. Every other freshness check (head, state, draft) + // reports nothing wrong, because none of them look at the base. This is the one check that does. + describe("detects a base retarget the head/state checks cannot see (#9055)", () => { + it("is current when the live base matches what the caller expected", () => { + const result = classifyPullRequestFreshness({ state: "open", head: { sha: "sha1" }, base: { ref: "main" } }, "sha1", { expectedBaseRef: "main" }); + expect(result).toEqual({ status: "current", liveHeadSha: "sha1", liveState: "open", liveLabels: [] }); + }); + + it("goes stale when the live base has moved, even though the head has not", () => { + const result = classifyPullRequestFreshness({ state: "open", head: { sha: "sha1" }, base: { ref: "release/2.0" } }, "sha1", { expectedBaseRef: "main" }); + expect(result).toMatchObject({ status: "stale", reason: "base_changed", liveHeadSha: "sha1" }); + expect(pullRequestFreshnessDetail(result)).toContain("base branch changed"); + }); + + it("does not check the base when the caller supplied no expectation — every existing caller is unaffected", () => { + const result = classifyPullRequestFreshness({ state: "open", head: { sha: "sha1" }, base: { ref: "release/2.0" } }, "sha1"); + expect(result.status).toBe("current"); + }); + + it("threads expectedBaseRef through the live fetch path", async () => { + const env = createTestEnv(); + setInstallationTokenStore({ + get: async () => ({ token: "tok", expiresAtMs: Date.now() + 10 * 60_000 }), + set: async () => {}, + }); + vi.stubGlobal("fetch", async () => Response.json({ state: "open", head: { sha: "sha1" }, base: { ref: "release/2.0" } })); + + const result = await fetchPullRequestFreshness(env, { + installationId: 123, + repoFullName: "owner/repo", + pullNumber: 7, + expectedHeadSha: "sha1", + expectedBaseRef: "main", + }); + expect(result).toMatchObject({ status: "stale", reason: "base_changed" }); + }); + }); }); diff --git a/test/unit/pr-labeled-public-surface.test.ts b/test/unit/pr-labeled-public-surface.test.ts new file mode 100644 index 0000000000..f27451ccba --- /dev/null +++ b/test/unit/pr-labeled-public-surface.test.ts @@ -0,0 +1,110 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { clearInstallationTokenCacheForTest } from "../../src/github/app"; +import { getPullRequestDetailSyncState, upsertInstallation, upsertPullRequestDetailSyncState, upsertPullRequestFromGitHub, upsertRepositoryFromGitHub, upsertRepositorySettings } from "../../src/db/repositories"; +import { processJob } from "../../src/queue/processors"; +import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; +import { generatePrivateKeyPem } from "../helpers/github-app-key"; +import { createTestEnv } from "../helpers/d1"; + +// #9059(c): pull_request.labeled/unlabeled did a row re-sync but were never in PR_PUBLIC_SURFACE_ACTIONS, so a +// maintainer adding or removing the manual-review hold label was only picked up by the ~2-minute sweep — which, +// combined with the sticky-label issue elsewhere in this audit, meant manually unblocking a PR had up to that +// lag before it took effect. labeled/unlabeled are disposition INPUTS (the hold reads straight off the PR's +// labels), not mere metadata, so they belong in the same set as edited/synchronize. +describe("a label change runs the public-surface pipeline immediately (#9059)", () => { + afterEach(() => { + vi.unstubAllGlobals(); + clearInstallationTokenCacheForTest(); + }); + + async function seed(env: Env, repo: string, installationId: number, number: number, head: string): Promise { + await upsertInstallation(env, { + action: "created", + installation: { id: installationId, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: {}, events: [] }, + }); + await upsertRepositoryFromGitHub(env, { name: repo, full_name: `owner/${repo}`, private: false, owner: { login: "owner" } }, installationId); + // slopGateMode !== "off" is what makes maybePublishPrPublicSurface's file-refresh branch fire — the same + // gate the pre-existing "#review-pre-merge-checks" recapture-preview test uses for the same reason. + await upsertRepositorySettings(env, { repoFullName: `owner/${repo}`, slopGateMode: "advisory" }); + await upsertRepoFocusManifest(env, `owner/${repo}`, { settings: { checkRunMode: "off", commentMode: "off", publicSurface: "off" } }); + await upsertPullRequestFromGitHub(env, `owner/${repo}`, { number, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: head }, labels: [], body: "x" }); + await upsertPullRequestDetailSyncState(env, { repoFullName: `owner/${repo}`, pullNumber: number, status: "complete", headSha: head, filesSyncedAt: "2020-01-01T00:00:00.000Z", reviewsSyncedAt: "2020-01-01T00:00:00.000Z", checksSyncedAt: "2020-01-01T00:00:00.000Z", lastSyncedAt: "2020-01-01T00:00:00.000Z" }); + } + + function stubGitHub(repo: string, number: number, head: string): void { + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes(`/pulls/${number}/files`)) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + if (new RegExp(`/pulls/${number}(?:\\?|$)`).test(url)) return Response.json({ number, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: head }, labels: [{ name: "manual-review" }], body: "x" }); + if (url.includes(`/commits/${head}/check-runs`)) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes(`/commits/${head}/status`)) return Response.json({ state: "success", statuses: [] }); + return new Response("not found", { status: 404 }); + }); + } + + it("re-syncs immediately when a label is added, instead of waiting for the sweep", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seed(env, "label-repo", 9300, 31, "lh31"); + stubGitHub("label-repo", 31, "lh31"); + + await processJob(env, { + type: "github-webhook", + deliveryId: "labeled-1", + eventName: "pull_request", + payload: { + action: "labeled", + installation: { id: 9300 }, + repository: { name: "label-repo", full_name: "owner/label-repo", private: false, owner: { login: "owner" } }, + pull_request: { number: 31, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "lh31" }, labels: [{ name: "manual-review" }], body: "x" }, + label: { name: "manual-review" }, + }, + }); + + const state = await getPullRequestDetailSyncState(env, "owner/label-repo", 31); + expect(state?.lastSyncedAt).not.toBe("2020-01-01T00:00:00.000Z"); + }); + + it("re-syncs immediately when a label is removed", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seed(env, "unlabel-repo", 9301, 32, "lh32"); + stubGitHub("unlabel-repo", 32, "lh32"); + + await processJob(env, { + type: "github-webhook", + deliveryId: "unlabeled-1", + eventName: "pull_request", + payload: { + action: "unlabeled", + installation: { id: 9301 }, + repository: { name: "unlabel-repo", full_name: "owner/unlabel-repo", private: false, owner: { login: "owner" } }, + pull_request: { number: 32, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "lh32" }, labels: [], body: "x" }, + label: { name: "manual-review" }, + }, + }); + + const state = await getPullRequestDetailSyncState(env, "owner/unlabel-repo", 32); + expect(state?.lastSyncedAt).not.toBe("2020-01-01T00:00:00.000Z"); + }); + + it("does not run the public-surface pipeline for an action outside its scope (contrast case)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seed(env, "assign-repo", 9302, 33, "lh33"); + stubGitHub("assign-repo", 33, "lh33"); + + await processJob(env, { + type: "github-webhook", + deliveryId: "assigned-1", + eventName: "pull_request", + payload: { + action: "assigned", + installation: { id: 9302 }, + repository: { name: "assign-repo", full_name: "owner/assign-repo", private: false, owner: { login: "owner" } }, + pull_request: { number: 33, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "lh33" }, labels: [], body: "x" }, + }, + }); + + const state = await getPullRequestDetailSyncState(env, "owner/assign-repo", 33); + expect(state?.lastSyncedAt).toBe("2020-01-01T00:00:00.000Z"); + }); +}); diff --git a/test/unit/pr-outcome-reconciler.test.ts b/test/unit/pr-outcome-reconciler.test.ts index 8155c35501..197e22d7c0 100644 --- a/test/unit/pr-outcome-reconciler.test.ts +++ b/test/unit/pr-outcome-reconciler.test.ts @@ -130,6 +130,19 @@ describe("reconcileMissingPrOutcomes (#9026)", () => { vi.restoreAllMocks(); }); + it("treats a driver returning no results array as an empty scan", async () => { + const env = createTestEnv(); + const original = env.DB.prepare.bind(env.DB); + vi.spyOn(env.DB, "prepare").mockImplementation((query: string) => { + if (query.includes("FROM pull_requests")) { + return { bind: () => ({ all: async () => ({}) }) } as never; + } + return original(query); + }); + expect(await reconcileMissingPrOutcomes(env)).toEqual({ scanned: 0, backfilled: 0 }); + vi.restoreAllMocks(); + }); + it("bounds each run so a large backlog drains across runs instead of blocking one", () => { expect(PR_OUTCOME_RECONCILE_LIMIT).toBeGreaterThan(0); expect(PR_OUTCOME_RECONCILE_LIMIT).toBeLessThanOrEqual(1000); diff --git a/test/unit/queue-2.test.ts b/test/unit/queue-2.test.ts index 906fa84025..824a1250b6 100644 --- a/test/unit/queue-2.test.ts +++ b/test/unit/queue-2.test.ts @@ -3634,7 +3634,9 @@ describe("queue processors", () => { account: { login: "JSONbored", id: 1, type: "User" }, repository_selection: "selected", permissions: { metadata: "read", contents: "write", pull_requests: "write", issues: "write" }, - events: ["issues", "issue_comment", "pull_request", "repository", "installation_repositories"], + // #9058 promoted check_run/check_suite/pull_request_review to REQUIRED. A "healthy" fixture has to + // actually be healthy, so it subscribes to the full required set. + events: ["issues", "issue_comment", "pull_request", "pull_request_review", "repository", "check_run", "check_suite", "installation_repositories"], }, repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: true, owner: { login: "JSONbored" } }], }); @@ -3647,7 +3649,7 @@ describe("queue processors", () => { account: { login: "JSONbored", id: 1, type: "User" }, repository_selection: "selected", permissions: { metadata: "read", pull_requests: "write", issues: "write" }, - events: ["issues", "issue_comment", "pull_request", "repository", "installation_repositories"], + events: ["issues", "issue_comment", "pull_request", "pull_request_review", "repository", "check_run", "check_suite", "installation_repositories"], }); } return new Response("not found", { status: 404 });