From 38c8a91b081b72328e641fd4582be8b1c31fe9e0 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 17:08:36 -0700 Subject: [PATCH 1/2] fix(webhook): stop a stale payload reverting close-critical columns; handle transfers; wake on legacy CI events #9057 -- the out-of-order guard protected only state/headSha/mergedAt/ githubUpdatedAt/labels. Every OTHER column was written unconditionally from a possibly-stale payload, including the ones the linked-issue gate reads -- a hard blocker and a close reason. Real sequence: a contributor pushes (`synchronize`, T1), then edits the body to add "Fixes #123" (`edited`, T2). GitHub does not guarantee delivery order, and once heads diverge the two jobs have different coalesce keys, so `synchronize` can be dequeued LAST. The guard correctly preserved state/headSha/labels -- then reverted body and linkedIssuesJson to the pre-edit snapshot, and since `synchronize` is itself in PR_PUBLIC_SURFACE_ACTIONS the same pass re-ran the gate against the reverted body: a wrong autonomous close for "No linked issue detected" on a PR that has one. That close reason appears in the live ledger. body/linkedIssuesJson/linkedIssueClaimedAt now reuse the existing sparse-body preservation path (the incoming value is not current truth in either case), and title/baseRef/headRef/authorAssociation are protected on the same condition. Applied to the onConflictDoUpdate SET clause as well as the INSERT values -- the UPDATE is the path that actually runs for an existing row -- and to the returned record, which callers act on in-process. #9056 -- `repository.transferred` was never handled despite already being in WEBHOOK_METRIC_ACTIONS, so the case was anticipated and never implemented. Since `repositories` is keyed by full_name with no github_id, a transfer just INSERTed a fresh row and orphaned every table renameRepositoryIdentity migrates -- including repository_settings, so autonomy and gate config silently reverted to defaults while the repo kept operating, and staged approvals in agent_pending_actions were lost. Now derives the old name from changes.owner.from.{organization,user}.login plus the current repo name and runs the same identity migration as a rename. #9059(b) -- `status`/`workflow_run` invalidated the CI cache but never woke a re-review, which only moved the stall: the PR then waited on the ~2-minute sweep, which REST-budget backpressure can skip. That matters specifically because `codecov/patch` arrives as a commit STATUS and is the gate's hardest required check, so a repo whose last green signal is a status got no webhook wake at all and the gate looked hung. Now re-reviews behind the same ciReReviewCoalesced guard the check_run/check_suite path uses, so a status storm cannot amplify. Closes #9056 Closes #9057 Tests: 2 stale-payload regressions (a stale delivery cannot revert body, linked issues, title, baseRef or headRef; a newer one still updates all of them), a status-wakes-re-review test, and the two existing invalidation tests updated to pre-claim the coalesce window so the invalidation stays observable alongside the new wake. Full suite green: 22,175 passing. --- src/db/repositories.ts | 54 +++++++++++++++++++++++++++++------- src/queue/processors.ts | 46 ++++++++++++++++++++++++------ src/types.ts | 8 ++++++ test/unit/db-parsers.test.ts | 49 ++++++++++++++++++++++++++++++++ test/unit/queue.test.ts | 49 ++++++++++++++++++++++++++++++++ 5 files changed, 187 insertions(+), 19 deletions(-) diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 692db5bf6..0e26b73a6 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -365,6 +365,11 @@ export async function upsertPullRequestFromGitHub( mergedAt: pullRequests.mergedAt, githubUpdatedAt: pullRequests.githubUpdatedAt, labelsJson: pullRequests.labelsJson, + // #9057: also read the columns a stale payload could revert (see resolvedTitle/baseRef/etc. below). + title: pullRequests.title, + baseRef: pullRequests.baseRef, + headRef: pullRequests.headRef, + authorAssociation: pullRequests.authorAssociation, }) .from(pullRequests) .where(and(eq(pullRequests.repoFullName, repoFullName), eq(pullRequests.number, pr.number))) @@ -407,8 +412,33 @@ export async function upsertPullRequestFromGitHub( // disposition label (pending-closure, manual-review) even while state/headSha stayed correctly protected — // breaking the flag-then-close two-pass machine, whose Pass 2 keys on the label's presence. const resolvedLabelsJson = isStalePayload ? existingClaimRow!.labelsJson : jsonString(record.labels); + // #9057: the out-of-order guard protected only state/headSha/mergedAt/githubUpdatedAt/labels. Every OTHER + // column was written unconditionally from a possibly-stale payload — including the ones the linked-issue + // gate reads, which is a hard blocker and a close reason. + // + // Sequence: a contributor pushes (`synchronize`, updated_at=T1), then edits the body to add "Fixes #123" + // (`edited`, updated_at=T2). GitHub does not guarantee delivery order, and once heads diverge the two jobs + // have different coalesce keys, so `synchronize` can be dequeued AFTER `edited`. The guard correctly + // preserved state/headSha/labels — and then reverted `body` and `linkedIssuesJson` to the pre-edit + // snapshot. `synchronize` is itself in PR_PUBLIC_SURFACE_ACTIONS, so the same pass re-ran the gate against + // the reverted body: a wrong autonomous close for "No linked issue detected" on a PR that has one. That + // close reason appears in the live ledger. + // + // These columns are now protected on exactly the same condition. `payloadJson` carries `body`, which feeds + // the AI-review prompt, the screenshot gate, and linked-issue extraction; `baseRef` feeds required contexts, + // migration-collision detection, and the CI aggregate key; `title` feeds the PR-type label and the AI-review + // cache key. + const resolvedTitle = isStalePayload ? existingClaimRow!.title : pr.title; + const resolvedBaseRef = isStalePayload ? (existingClaimRow!.baseRef ?? undefined) : pr.base?.ref; + const resolvedHeadRef = isStalePayload ? (existingClaimRow!.headRef ?? undefined) : pr.head?.ref; + const resolvedAuthorAssociation = isStalePayload ? (existingClaimRow!.authorAssociation ?? undefined) : pr.author_association; const lastSeenOpenAt = resolvedState === "open" ? (options.seenOpenAt ?? syncedAt) : null; - const preserveSparseBody = pr.body === undefined && existingClaimRow !== undefined; + // #9057: a STALE payload's body must be preserved for exactly the same reason a SPARSE one's is — in both + // cases the incoming body is not the current truth, and re-deriving linked issues from it reverts a + // just-added "Fixes #123" and can drive a wrong "No linked issue detected" close on the very next gate pass. + // Folding it into this one flag keeps the body, payloadJson, linkedIssuesJson and linkedIssueClaimedAt + // derivations below consistent by construction rather than needing four parallel stale-branches. + const preserveSparseBody = (pr.body === undefined || isStalePayload) && existingClaimRow !== undefined; const existingPayload = preserveSparseBody ? parseJson<{ body?: string | null }>(existingClaimRow.payloadJson, {}) : undefined; const existingBody = existingPayload?.body ?? null; const body = preserveSparseBody ? existingBody : record.body; @@ -449,13 +479,13 @@ export async function upsertPullRequestFromGitHub( id: `${repoFullName}#${pr.number}`, repoFullName, number: pr.number, - title: pr.title, + title: resolvedTitle, state: resolvedState, authorLogin: pr.user?.login, - authorAssociation: pr.author_association, + authorAssociation: resolvedAuthorAssociation, headSha: resolvedHeadSha, - headRef: pr.head?.ref, - baseRef: pr.base?.ref, + headRef: resolvedHeadRef, + baseRef: resolvedBaseRef, mergedAt: resolvedMergedAt, htmlUrl: pr.html_url, labelsJson: resolvedLabelsJson, @@ -476,13 +506,15 @@ export async function upsertPullRequestFromGitHub( .onConflictDoUpdate({ target: [pullRequests.repoFullName, pullRequests.number], set: { - title: pr.title, + // #9057: the UPDATE path is the one that actually runs for an existing row, so the stale-payload + // protections must be applied here too — not only in the INSERT values above. + title: resolvedTitle, state: resolvedState, authorLogin: pr.user?.login, - authorAssociation: pr.author_association, + authorAssociation: resolvedAuthorAssociation, headSha: resolvedHeadSha, - headRef: pr.head?.ref, - baseRef: pr.base?.ref, + headRef: resolvedHeadRef, + baseRef: resolvedBaseRef, mergedAt: resolvedMergedAt, htmlUrl: pr.html_url, labelsJson: resolvedLabelsJson, @@ -496,7 +528,9 @@ export async function upsertPullRequestFromGitHub( updatedAt: syncedAt, }, }); - return { ...record, state: resolvedState, headSha: resolvedHeadSha, mergedAt: resolvedMergedAt ?? null, labels: parseJson(resolvedLabelsJson, []), body, linkedIssues, linkedIssueClaimedAt, bodyObservedAt, headShaObservedAt }; + // #9057: the returned record must describe what was PERSISTED, not the stale incoming payload — every caller + // (handlePullRequestWebhookEvent's own closed-check, the gate pass that follows) acts on this in-process. + return { ...record, title: resolvedTitle, state: resolvedState, headSha: resolvedHeadSha, headRef: resolvedHeadRef ?? null, baseRef: resolvedBaseRef ?? null, mergedAt: resolvedMergedAt ?? null, labels: parseJson(resolvedLabelsJson, []), body, linkedIssues, linkedIssueClaimedAt, bodyObservedAt, headShaObservedAt }; } function resolveLinkedIssueClaimedAt( diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 4cf5a012e..81cf7edf8 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -4874,10 +4874,12 @@ async function maybeReReviewOnCiCompletion( /** * 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 + * review finding), and (#9059) wake the affected PRs. Leaving the cache 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 + * to the full cache TTL; invalidating WITHOUT re-reviewing (the prior behavior) merely moved the stall, since the + * PR then waited on the ~2-minute sweep, which backpressure can skip. `codecov/patch` arrives as a commit + * STATUS and is the gate's hardest required check, so a repo whose last green signal is a status previously got + * no webhook wake at all. 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. */ @@ -4909,6 +4911,13 @@ async function maybeInvalidateCiCacheOnLegacyCiEvent( for (const pr of open) { if (pr.headSha !== headSha) continue; await invalidateCiStateCache(env, repoFullName, pr.number).catch(() => undefined); + // #9059: invalidating the cache without waking a re-review left the PR waiting for the ~2-minute + // sweep -- which is itself skipped under REST-budget backpressure. That matters specifically because + // `codecov/patch` is a COMMIT STATUS, and it is the gate's hardest required check: a repo whose last + // green signal arrives as a status got no webhook wake at all, so the gate looked hung. Same coalesce + // guard and same call as the check_run/check_suite completion path, so a status storm cannot amplify. + if (await ciReReviewCoalesced(env, repoFullName, pr.number)) continue; + await reReviewStoredPullRequest(env, deliveryId, installationId, repoFullName, pr.number).catch(() => undefined); } } } @@ -5910,16 +5919,35 @@ async function maybeHandleRepositoryRenamedWebhookEvent( eventName: string, payload: GitHubWebhookPayload, ): Promise { - if (eventName !== "repository" || payload.action !== "renamed") return; - const oldName = payload.changes?.repository?.name?.from; + // #9056: `transferred` was never handled even though it is already in WEBHOOK_METRIC_ACTIONS, so the case + // was anticipated and simply never implemented. On a transfer GitHub sends `repository.transferred` carrying + // `changes.owner.from`, then ordinary events under the NEW full_name — and since `repositories` is keyed by + // full_name with no github_id, upsertRepositoryFromGitHub just INSERTs a fresh row. Every table + // renameRepositoryIdentity migrates is then orphaned, including repository_settings: autonomy and gate + // config silently revert to defaults while the repo keeps operating, and staged maintainer approvals in + // agent_pending_actions are lost. Calibration/reversal joins on `project` split across two names, and + // nothing ever heals it. + if (eventName !== "repository" || (payload.action !== "renamed" && payload.action !== "transferred")) return; const newFullName = payload.repository?.full_name; - if (!oldName || !newFullName) return; - const owner = payload.repository?.owner?.login ?? repoParts(newFullName).owner; - const oldFullName = `${owner}/${oldName}`; + if (!newFullName) return; + const currentOwner = payload.repository?.owner?.login ?? repoParts(newFullName).owner; + const currentName = payload.repository?.name ?? repoParts(newFullName).name; + // A rename changes the NAME under the same owner; a transfer changes the OWNER with the same name. + const previousOwner = + payload.changes?.owner?.from?.organization?.login ?? payload.changes?.owner?.from?.user?.login; + const oldFullName = + payload.action === "transferred" + ? previousOwner && currentName + ? `${previousOwner}/${currentName}` + : undefined + : payload.changes?.repository?.name?.from + ? `${currentOwner}/${payload.changes.repository.name.from}` + : undefined; + if (!oldFullName) return; if (oldFullName === newFullName) return; await renameRepositoryIdentity(env, oldFullName, newFullName); await recordAuditEvent(env, { - eventType: "github_app.repository_renamed", + eventType: payload.action === "transferred" ? "github_app.repository_transferred" : "github_app.repository_renamed", actor: "loopover", targetKey: newFullName, outcome: "completed", diff --git a/src/types.ts b/src/types.ts index b81c41b19..16db91fe2 100644 --- a/src/types.ts +++ b/src/types.ts @@ -366,6 +366,14 @@ export type GitHubWebhookPayload = { from?: string; }; }; + /** #9056: `repository.transferred` carries the PREVIOUS owner here (the repo name itself is unchanged), + * so the old full name is `changes.owner.from.{organization,user}.login` + the current repo name. */ + owner?: { + from?: { + organization?: { login?: string }; + user?: { login?: string }; + }; + }; }; }; diff --git a/test/unit/db-parsers.test.ts b/test/unit/db-parsers.test.ts index 14ee3e8e8..3181e80e6 100644 --- a/test/unit/db-parsers.test.ts +++ b/test/unit/db-parsers.test.ts @@ -654,6 +654,55 @@ describe("database row parser hardening", () => { expect(stored?.headSha).toBe("a1"); }); + // #9057: the guard protected only state/headSha/mergedAt/githubUpdatedAt/labels. Everything else was + // written unconditionally from the stale payload — including body and linkedIssuesJson, which the + // linked-issue gate reads as a hard blocker and close reason. Real sequence: a contributor pushes + // (`synchronize`, T1), then edits the body to add "Fixes #123" (`edited`, T2). Delivery order is not + // guaranteed and the two jobs have different coalesce keys once heads diverge, so `synchronize` could be + // dequeued last — reverting the body, and (because `synchronize` is itself in PR_PUBLIC_SURFACE_ACTIONS) + // re-running the gate against the reverted body for a wrong "No linked issue detected" close. + it("#9057: a stale webhook cannot revert the body, linked issues, title, or baseRef a newer one wrote", async () => { + const env = createTestEnv(); + // The `edited` delivery lands first and adds the linked issue. + await upsertPullRequestFromGitHub(env, "owner/repo", { + number: 32, title: "Add retry logic", state: "open", user: { login: "bob" }, head: { sha: "a1", ref: "feature-v2" }, + base: { ref: "main" }, body: "Fixes #123", labels: [], updated_at: "2026-07-21T12:20:00.000Z", + }); + const afterEdit = await getPullRequest(env, "owner/repo", 32); + expect(afterEdit?.linkedIssues).toEqual([123]); + + // The delayed `synchronize` job finally dequeues carrying the PRE-edit snapshot. + const stale = await upsertPullRequestFromGitHub(env, "owner/repo", { + number: 32, title: "wip", state: "open", user: { login: "bob" }, head: { sha: "a0", ref: "old-branch" }, + base: { ref: "develop" }, body: "no issue yet", labels: [], updated_at: "2026-07-21T12:10:00.000Z", + }); + + // The linked issue survives — this is the one that drove the wrong close. + expect(stale.linkedIssues).toEqual([123]); + const stored = await getPullRequest(env, "owner/repo", 32); + expect(stored?.linkedIssues).toEqual([123]); + expect(stored?.body).toBe("Fixes #123"); + // ...along with the other columns a stale payload could silently revert. + expect(stored?.title).toBe("Add retry logic"); + expect(stored?.baseRef).toBe("main"); + expect(stored?.headRef).toBe("feature-v2"); + }); + + it("#9057: a NEWER webhook still updates body/linked issues/title/baseRef normally", async () => { + const env = createTestEnv(); + await upsertPullRequestFromGitHub(env, "owner/repo", { + number: 33, title: "wip", state: "open", user: { login: "bob" }, head: { sha: "a1", ref: "old-branch" }, + base: { ref: "develop" }, body: "no issue yet", labels: [], updated_at: "2026-07-21T12:00:00.000Z", + }); + await upsertPullRequestFromGitHub(env, "owner/repo", { + number: 33, title: "Add retry logic", state: "open", user: { login: "bob" }, head: { sha: "a2", ref: "feature-v2" }, + base: { ref: "main" }, body: "Fixes #123", labels: [], updated_at: "2026-07-21T12:30:00.000Z", + }); + const stored = await getPullRequest(env, "owner/repo", 33); + expect(stored?.linkedIssues).toEqual([123]); + expect(stored).toMatchObject({ title: "Add retry logic", baseRef: "main", headRef: "feature-v2", body: "Fixes #123" }); + }); + it("a NEWER webhook (later updated_at) still applies normally", async () => { const env = createTestEnv(); await upsertPullRequestFromGitHub(env, "owner/repo", { diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 6d9875490..fde44f13b 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -3251,6 +3251,11 @@ describe("queue processors", () => { 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" }); + // #9059: the same delivery now also WAKES a re-review, whose own aggregate read would immediately + // repopulate the row -- racing this assertion. Pre-claim the ci-coalesce window so this delivery's + // re-review is skipped, leaving the invalidation itself directly observable. (Same technique the + // check_run invalidation test one describe over already uses, for the same reason.) + await env.SELFHOST_TRANSIENT_CACHE?.set("ci-coalesce:owner/agent-repo#7", "1", 60); await processJob(env, { type: "github-webhook", deliveryId: `${eventName}-event`, @@ -3266,6 +3271,50 @@ describe("queue processors", () => { } }); + // #9059: invalidating the cache without waking a re-review just moved the stall — the PR then waited on the + // ~2-minute sweep, which REST-budget backpressure can skip entirely. That matters specifically because + // `codecov/patch` arrives as a commit STATUS and is the gate's hardest required check, so a repo whose last + // green signal is a status got no webhook wake at all and the gate looked hung. + it("#9059: a settled status event also WAKES a re-review for the matching PR (not just cache invalidation)", 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: [], + advisoryHoldDetails: [], + 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 { + // No coalesce pre-claim this time, so the wake is allowed to run. A re-review repopulates the durable + // row it just invalidated — which is exactly the observable proof that the wake happened. + await processJob(env, { + type: "github-webhook", + deliveryId: "status-wakes-rereview", + eventName: "status", + payload: { sha: "a7", state: "success", repository: { name: "agent-repo", full_name: "owner/agent-repo", owner: { login: "owner" } }, installation: { id: 9001 } }, + } as never); + + expect(await getPullRequestDetailSyncState(env, "owner/agent-repo", 7)).toMatchObject({ ciState: "passed" }); + } 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: "" }); From 4aa2d75bc0b3f8746f53b959db87c4179cb68cee Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 18:39:29 -0700 Subject: [PATCH 2/2] =?UTF-8?q?fix(webhook):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20protect=20authorAssociation=20on=20the=20returned?= =?UTF-8?q?=20record,=20and=20close=20the=20coverage=20gaps?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review on #9116 was correct on both counts. 1. The returned record overrode title/headSha/headRef/baseRef/mergedAt/labels but omitted authorAssociation, so an in-process caller could still read the stale value even though the DB row was correctly protected -- contradicting the very invariant the previous commit stated. Added, and asserted on the return value (not just the stored row) in the regression test. 2. codecov/patch was low because the transferred-event derivation and the authorAssociation stale-path had no tests. Both covered now. The transfer-predecessor derivation is extracted into an exported pure helper, resolveRepositoryIdentityPredecessor, because several of its shapes are unreachable through the live pipeline -- the repository upsert running alongside this handler requires a full_name and fails first -- so they could not be tested end to end. That also let the dead `?? payload.repository.owner/name` fallbacks go: full_name is authoritative and already required. Full suite green: 22,182 passing. 100% line+branch coverage on all added lines. --- src/db/repositories.ts | 2 +- src/queue/processors.ts | 47 +++++++----- test/unit/db-parsers.test.ts | 20 ++++-- test/unit/repo-rename-webhook.test.ts | 100 ++++++++++++++++++++++++++ 4 files changed, 147 insertions(+), 22 deletions(-) diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 0e26b73a6..c50921623 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -530,7 +530,7 @@ export async function upsertPullRequestFromGitHub( }); // #9057: the returned record must describe what was PERSISTED, not the stale incoming payload — every caller // (handlePullRequestWebhookEvent's own closed-check, the gate pass that follows) acts on this in-process. - return { ...record, title: resolvedTitle, state: resolvedState, headSha: resolvedHeadSha, headRef: resolvedHeadRef ?? null, baseRef: resolvedBaseRef ?? null, mergedAt: resolvedMergedAt ?? null, labels: parseJson(resolvedLabelsJson, []), body, linkedIssues, linkedIssueClaimedAt, bodyObservedAt, headShaObservedAt }; + return { ...record, title: resolvedTitle, authorAssociation: resolvedAuthorAssociation ?? null, state: resolvedState, headSha: resolvedHeadSha, headRef: resolvedHeadRef ?? null, baseRef: resolvedBaseRef ?? null, mergedAt: resolvedMergedAt ?? null, labels: parseJson(resolvedLabelsJson, []), body, linkedIssues, linkedIssueClaimedAt, bodyObservedAt, headShaObservedAt }; } function resolveLinkedIssueClaimedAt( diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 81cf7edf8..3dbfaff11 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -5906,6 +5906,35 @@ async function maybeHandleForeignAppInstallationWebhookEvent( return false; } +/** + * #9056: resolve the PREVIOUS full name for a `repository.renamed` or `repository.transferred` delivery, or + * undefined when it cannot be determined (in which case the caller must do nothing rather than migrate toward + * a guessed identity). A rename changes the NAME under the same owner; a transfer changes the OWNER and keeps + * the name, carrying the old one in `changes.owner.from.{organization,user}.login`. + * + * Pure and exported so every shape — including the ones the live pipeline cannot reach, because the repository + * upsert running alongside this handler requires a full_name and would fail first — is directly testable. + * Returns undefined for a same-name result so an idempotent redelivery is a no-op. + */ +export function resolveRepositoryIdentityPredecessor( + action: string | undefined, + newFullName: string | undefined, + changes: GitHubWebhookPayload["changes"], +): string | undefined { + if (!newFullName) return undefined; + const { owner: currentOwner, name: currentName } = repoParts(newFullName); + const oldFullName = + action === "transferred" + ? ((): string | undefined => { + const previousOwner = changes?.owner?.from?.organization?.login ?? changes?.owner?.from?.user?.login; + return previousOwner ? `${previousOwner}/${currentName}` : undefined; + })() + : changes?.repository?.name?.from + ? `${currentOwner}/${changes.repository.name.from}` + : undefined; + return oldFullName && oldFullName !== newFullName ? oldFullName : undefined; +} + /** * Handles a `repository` webhook with `action: "renamed"`: migrates the repo's identity forward across * every structural repo-identity column (see repo-identity-rename.ts) BEFORE the caller's normal @@ -5929,22 +5958,8 @@ async function maybeHandleRepositoryRenamedWebhookEvent( // nothing ever heals it. if (eventName !== "repository" || (payload.action !== "renamed" && payload.action !== "transferred")) return; const newFullName = payload.repository?.full_name; - if (!newFullName) return; - const currentOwner = payload.repository?.owner?.login ?? repoParts(newFullName).owner; - const currentName = payload.repository?.name ?? repoParts(newFullName).name; - // A rename changes the NAME under the same owner; a transfer changes the OWNER with the same name. - const previousOwner = - payload.changes?.owner?.from?.organization?.login ?? payload.changes?.owner?.from?.user?.login; - const oldFullName = - payload.action === "transferred" - ? previousOwner && currentName - ? `${previousOwner}/${currentName}` - : undefined - : payload.changes?.repository?.name?.from - ? `${currentOwner}/${payload.changes.repository.name.from}` - : undefined; - if (!oldFullName) return; - if (oldFullName === newFullName) return; + const oldFullName = resolveRepositoryIdentityPredecessor(payload.action, newFullName, payload.changes); + if (!oldFullName || !newFullName) return; await renameRepositoryIdentity(env, oldFullName, newFullName); await recordAuditEvent(env, { eventType: payload.action === "transferred" ? "github_app.repository_transferred" : "github_app.repository_renamed", diff --git a/test/unit/db-parsers.test.ts b/test/unit/db-parsers.test.ts index 3181e80e6..e19d997bd 100644 --- a/test/unit/db-parsers.test.ts +++ b/test/unit/db-parsers.test.ts @@ -666,7 +666,7 @@ describe("database row parser hardening", () => { // The `edited` delivery lands first and adds the linked issue. await upsertPullRequestFromGitHub(env, "owner/repo", { number: 32, title: "Add retry logic", state: "open", user: { login: "bob" }, head: { sha: "a1", ref: "feature-v2" }, - base: { ref: "main" }, body: "Fixes #123", labels: [], updated_at: "2026-07-21T12:20:00.000Z", + base: { ref: "main" }, body: "Fixes #123", labels: [], author_association: "MEMBER", updated_at: "2026-07-21T12:20:00.000Z", }); const afterEdit = await getPullRequest(env, "owner/repo", 32); expect(afterEdit?.linkedIssues).toEqual([123]); @@ -674,7 +674,7 @@ describe("database row parser hardening", () => { // The delayed `synchronize` job finally dequeues carrying the PRE-edit snapshot. const stale = await upsertPullRequestFromGitHub(env, "owner/repo", { number: 32, title: "wip", state: "open", user: { login: "bob" }, head: { sha: "a0", ref: "old-branch" }, - base: { ref: "develop" }, body: "no issue yet", labels: [], updated_at: "2026-07-21T12:10:00.000Z", + base: { ref: "develop" }, body: "no issue yet", labels: [], author_association: "FIRST_TIME_CONTRIBUTOR", updated_at: "2026-07-21T12:10:00.000Z", }); // The linked issue survives — this is the one that drove the wrong close. @@ -686,21 +686,31 @@ describe("database row parser hardening", () => { expect(stored?.title).toBe("Add retry logic"); expect(stored?.baseRef).toBe("main"); expect(stored?.headRef).toBe("feature-v2"); + expect(stored?.authorAssociation).toBe("MEMBER"); + // The RETURNED record must describe what was persisted too — in-process callers act on it directly, so a + // protected DB row plus a stale return value would still drive a wrong decision this same pass. + expect(stale).toMatchObject({ + title: "Add retry logic", + baseRef: "main", + headRef: "feature-v2", + authorAssociation: "MEMBER", + body: "Fixes #123", + }); }); it("#9057: a NEWER webhook still updates body/linked issues/title/baseRef normally", async () => { const env = createTestEnv(); await upsertPullRequestFromGitHub(env, "owner/repo", { number: 33, title: "wip", state: "open", user: { login: "bob" }, head: { sha: "a1", ref: "old-branch" }, - base: { ref: "develop" }, body: "no issue yet", labels: [], updated_at: "2026-07-21T12:00:00.000Z", + base: { ref: "develop" }, body: "no issue yet", labels: [], author_association: "FIRST_TIME_CONTRIBUTOR", updated_at: "2026-07-21T12:00:00.000Z", }); await upsertPullRequestFromGitHub(env, "owner/repo", { number: 33, title: "Add retry logic", state: "open", user: { login: "bob" }, head: { sha: "a2", ref: "feature-v2" }, - base: { ref: "main" }, body: "Fixes #123", labels: [], updated_at: "2026-07-21T12:30:00.000Z", + base: { ref: "main" }, body: "Fixes #123", labels: [], author_association: "MEMBER", updated_at: "2026-07-21T12:30:00.000Z", }); const stored = await getPullRequest(env, "owner/repo", 33); expect(stored?.linkedIssues).toEqual([123]); - expect(stored).toMatchObject({ title: "Add retry logic", baseRef: "main", headRef: "feature-v2", body: "Fixes #123" }); + expect(stored).toMatchObject({ title: "Add retry logic", baseRef: "main", headRef: "feature-v2", body: "Fixes #123", authorAssociation: "MEMBER" }); }); it("a NEWER webhook (later updated_at) still applies normally", async () => { diff --git a/test/unit/repo-rename-webhook.test.ts b/test/unit/repo-rename-webhook.test.ts index b6c7b9b96..cd5f2a8fc 100644 --- a/test/unit/repo-rename-webhook.test.ts +++ b/test/unit/repo-rename-webhook.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it, vi } from "vitest"; import { processJob } from "../../src/queue/job-dispatch"; +import { resolveRepositoryIdentityPredecessor } from "../../src/queue/processors"; import { getPullRequest, getRepository, upsertInstallation, upsertPullRequestFromGitHub, upsertRepositoryFromGitHub } from "../../src/db/repositories"; import { createTestEnv } from "../helpers/d1"; @@ -112,3 +113,102 @@ describe("repository renamed webhook", () => { expect(renamed?.installationId).toBe(9704); }, 30_000); }); + +// #9056: `repository.transferred` was never handled even though it was already listed in +// WEBHOOK_METRIC_ACTIONS, so the case was anticipated and simply never implemented. `repositories` is keyed by +// full_name with no github_id, so a transfer just INSERTed a fresh row and orphaned every table +// renameRepositoryIdentity migrates — including repository_settings, meaning autonomy and gate config silently +// reverted to defaults while the repo kept operating, and staged approvals in agent_pending_actions were lost. +describe("repository transferred webhook (#9056)", () => { + function transferredWebhookPayload(fromOwner: string, toFullName: string, installationId: number, ownerKind: "organization" | "user" = "organization") { + return { + action: "transferred", + // A transfer changes the OWNER and keeps the name — the inverse of a rename. + changes: { owner: { from: { [ownerKind]: { login: fromOwner } } } }, + repository: { name: toFullName.split("/")[1]!, full_name: toFullName, private: false, owner: { login: toFullName.split("/")[0]! } }, + installation: { id: installationId, account: { login: toFullName.split("/")[0]!, id: 1, type: "Organization" }, repository_selection: "selected", permissions: {}, events: [] }, + sender: { login: "owner", type: "User" }, + }; + } + + it("migrates PR history forward on a transfer instead of orphaning it under the old owner", async () => { + const env = createTestEnv(); + await seedInstalledRepo(env, "old-org/widgets", 9800); + await upsertPullRequestFromGitHub(env, "old-org/widgets", { number: 4, title: "Pre-transfer PR", state: "open", labels: [] }); + vi.stubGlobal("fetch", async () => new Response("{}", { status: 200 })); + + await processJob(env, { + type: "github-webhook", + deliveryId: "repo-transferred", + eventName: "repository", + payload: transferredWebhookPayload("old-org", "new-org/widgets", 9800), + } as never); + + expect(await getPullRequest(env, "new-org/widgets", 4)).toMatchObject({ title: "Pre-transfer PR" }); + expect(await getRepository(env, "new-org/widgets")).toBeTruthy(); + }); + + it("derives the previous owner from a USER-owned transfer too (changes.owner.from.user)", async () => { + const env = createTestEnv(); + await seedInstalledRepo(env, "alice/widgets", 9801); + await upsertPullRequestFromGitHub(env, "alice/widgets", { number: 5, title: "User-owned PR", state: "open", labels: [] }); + vi.stubGlobal("fetch", async () => new Response("{}", { status: 200 })); + + await processJob(env, { + type: "github-webhook", + deliveryId: "repo-transferred-user", + eventName: "repository", + payload: transferredWebhookPayload("alice", "new-org/widgets", 9801, "user"), + } as never); + + expect(await getPullRequest(env, "new-org/widgets", 5)).toMatchObject({ title: "User-owned PR" }); + }); + + it("is a no-op when the previous owner cannot be derived (no changes.owner) — never guesses an identity", async () => { + const env = createTestEnv(); + await seedInstalledRepo(env, "old-org/widgets", 9802); + await upsertPullRequestFromGitHub(env, "old-org/widgets", { number: 6, title: "Untouched", state: "open", labels: [] }); + vi.stubGlobal("fetch", async () => new Response("{}", { status: 200 })); + + await processJob(env, { + type: "github-webhook", + deliveryId: "repo-transferred-no-changes", + eventName: "repository", + payload: { + action: "transferred", + repository: { name: "widgets", full_name: "new-org/widgets", private: false, owner: { login: "new-org" } }, + installation: { id: 9802, account: { login: "new-org", id: 1, type: "Organization" }, repository_selection: "selected", permissions: {}, events: [] }, + }, + } as never); + + // The original row is left exactly where it was rather than being migrated to a guessed name. + expect(await getPullRequest(env, "old-org/widgets", 6)).toMatchObject({ title: "Untouched" }); + }); +}); + +// #9056: the pure predecessor resolver, tested directly. The live pipeline cannot reach several of these +// shapes (the repository upsert alongside this handler requires a full_name and fails first), which is +// exactly why the logic is extracted rather than left inline. +describe("resolveRepositoryIdentityPredecessor (#9056)", () => { + it("derives a TRANSFER's old name from the previous org or user owner, keeping the repo name", () => { + expect(resolveRepositoryIdentityPredecessor("transferred", "new-org/widgets", { owner: { from: { organization: { login: "old-org" } } } })).toBe("old-org/widgets"); + expect(resolveRepositoryIdentityPredecessor("transferred", "new-org/widgets", { owner: { from: { user: { login: "alice" } } } })).toBe("alice/widgets"); + }); + + it("derives a RENAME's old name from the previous repo name, keeping the owner", () => { + expect(resolveRepositoryIdentityPredecessor("renamed", "owner/loopover", { repository: { name: { from: "gittensory" } } })).toBe("owner/gittensory"); + }); + + it("returns undefined when the predecessor cannot be determined — never guesses an identity", () => { + expect(resolveRepositoryIdentityPredecessor("transferred", "new-org/widgets", {})).toBeUndefined(); + expect(resolveRepositoryIdentityPredecessor("transferred", "new-org/widgets", undefined)).toBeUndefined(); + expect(resolveRepositoryIdentityPredecessor("renamed", "owner/loopover", {})).toBeUndefined(); + // No full_name ⇒ nothing to migrate toward. + expect(resolveRepositoryIdentityPredecessor("transferred", undefined, { owner: { from: { organization: { login: "old-org" } } } })).toBeUndefined(); + }); + + it("returns undefined when the result equals the current name — an idempotent redelivery is a no-op", () => { + expect(resolveRepositoryIdentityPredecessor("transferred", "same-org/widgets", { owner: { from: { organization: { login: "same-org" } } } })).toBeUndefined(); + expect(resolveRepositoryIdentityPredecessor("renamed", "owner/widgets", { repository: { name: { from: "widgets" } } })).toBeUndefined(); + }); +});