diff --git a/migrations/0175_issues_github_updated_at.sql b/migrations/0175_issues_github_updated_at.sql new file mode 100644 index 0000000000..767fe7eebe --- /dev/null +++ b/migrations/0175_issues_github_updated_at.sql @@ -0,0 +1,6 @@ +-- #8804 (round-2 audit): extend the out-of-order webhook guard (#webhook-reorder-clobber, 0172) to ISSUES. +-- upsertIssueFromGitHub had no reorder protection at all -- a delayed webhook's stale snapshot could regress +-- an issue's state and wipe a just-applied label. Same design as pull_requests.github_updated_at: stores +-- GitHub's OWN `updated_at` so the upsert can compare incoming vs. stored; NULL for existing rows -- the +-- guard fails open until each issue's next sync backfills it. +ALTER TABLE issues ADD COLUMN github_updated_at TEXT; diff --git a/src/db/repositories.ts b/src/db/repositories.ts index dcdae5a383..5dc1e44da2 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -364,6 +364,7 @@ export async function upsertPullRequestFromGitHub( state: pullRequests.state, mergedAt: pullRequests.mergedAt, githubUpdatedAt: pullRequests.githubUpdatedAt, + labelsJson: pullRequests.labelsJson, }) .from(pullRequests) .where(and(eq(pullRequests.repoFullName, repoFullName), eq(pullRequests.number, pr.number))) @@ -402,6 +403,10 @@ export async function upsertPullRequestFromGitHub( // No `?? undefined` fallback on the stale branch (unlike headSha/mergedAt above): isStalePayload's own // definition already requires existingClaimRow.githubUpdatedAt to be non-null, so that branch is unreachable. const resolvedGithubUpdatedAt = isStalePayload ? existingClaimRow!.githubUpdatedAt : (incomingGithubUpdatedAt ?? undefined); + // #8804: labels join the protected set. A reordered stale snapshot could silently wipe a JUST-applied + // 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); const lastSeenOpenAt = resolvedState === "open" ? (options.seenOpenAt ?? syncedAt) : null; const preserveSparseBody = pr.body === undefined && existingClaimRow !== undefined; const existingPayload = preserveSparseBody ? parseJson<{ body?: string | null }>(existingClaimRow.payloadJson, {}) : undefined; @@ -453,7 +458,7 @@ export async function upsertPullRequestFromGitHub( baseRef: pr.base?.ref, mergedAt: resolvedMergedAt, htmlUrl: pr.html_url, - labelsJson: jsonString(record.labels), + labelsJson: resolvedLabelsJson, linkedIssuesJson, linkedIssueClaimedAt, bodyObservedAt, @@ -480,7 +485,7 @@ export async function upsertPullRequestFromGitHub( baseRef: pr.base?.ref, mergedAt: resolvedMergedAt, htmlUrl: pr.html_url, - labelsJson: jsonString(record.labels), + labelsJson: resolvedLabelsJson, linkedIssuesJson, linkedIssueClaimedAt, bodyObservedAt, @@ -491,7 +496,7 @@ export async function upsertPullRequestFromGitHub( updatedAt: syncedAt, }, }); - return { ...record, state: resolvedState, headSha: resolvedHeadSha, mergedAt: resolvedMergedAt ?? null, body, linkedIssues, linkedIssueClaimedAt, bodyObservedAt, headShaObservedAt }; + return { ...record, state: resolvedState, headSha: resolvedHeadSha, mergedAt: resolvedMergedAt ?? null, labels: parseJson(resolvedLabelsJson, []), body, linkedIssues, linkedIssueClaimedAt, bodyObservedAt, headShaObservedAt }; } function resolveLinkedIssueClaimedAt( @@ -540,7 +545,23 @@ function linkedIssueSetsOverlap(left: number[], right: number[]): boolean { export async function upsertIssueFromGitHub(env: Env, repoFullName: string, issue: GitHubIssuePayload, options: { seenOpenAt?: string } = {}): Promise { const record = toIssueRecord(repoFullName, issue); const db = getDb(env.DB); - const lastSeenOpenAt = issue.state === "open" ? (options.seenOpenAt ?? nowIso()) : null; + // #8804: the same out-of-order webhook guard upsertPullRequestFromGitHub carries (#webhook-reorder-clobber), + // previously absent here entirely — a delayed issue webhook's stale snapshot could regress state and wipe a + // just-applied label. Same fail-open contract: a sparse payload (no updated_at) or a pre-migration row + // (githubUpdatedAt NULL) applies the write exactly as before. + const existingRows = await db + .select({ state: issues.state, labelsJson: issues.labelsJson, githubUpdatedAt: issues.githubUpdatedAt }) + .from(issues) + .where(and(eq(issues.repoFullName, repoFullName), eq(issues.number, issue.number))) + .limit(1); + const existingRow = existingRows[0]; + const incomingGithubUpdatedAt = issue.updated_at ?? null; + const isStalePayload = + incomingGithubUpdatedAt !== null && existingRow?.githubUpdatedAt != null && incomingGithubUpdatedAt < existingRow.githubUpdatedAt; + const resolvedState = isStalePayload ? existingRow!.state : issue.state; + const resolvedLabelsJson = isStalePayload ? existingRow!.labelsJson : jsonString(record.labels); + const resolvedGithubUpdatedAt = isStalePayload ? existingRow!.githubUpdatedAt : (incomingGithubUpdatedAt ?? undefined); + const lastSeenOpenAt = resolvedState === "open" ? (options.seenOpenAt ?? nowIso()) : null; logIfBodyTruncated("issue", repoFullName, issue.number, issue.body); await db .insert(issues) @@ -549,32 +570,36 @@ export async function upsertIssueFromGitHub(env: Env, repoFullName: string, issu repoFullName, number: issue.number, title: issue.title, - state: issue.state, + state: resolvedState, authorLogin: issue.user?.login, authorAssociation: issue.author_association, htmlUrl: issue.html_url, - labelsJson: jsonString(record.labels), + labelsJson: resolvedLabelsJson, linkedPrsJson: jsonString(record.linkedPrs), lastSeenOpenAt, payloadJson: jsonString(compactGitHubPayload(issue)), + githubUpdatedAt: resolvedGithubUpdatedAt, updatedAt: nowIso(), }) .onConflictDoUpdate({ target: [issues.repoFullName, issues.number], set: { title: issue.title, - state: issue.state, + state: resolvedState, authorLogin: issue.user?.login, authorAssociation: issue.author_association, htmlUrl: issue.html_url, - labelsJson: jsonString(record.labels), + labelsJson: resolvedLabelsJson, linkedPrsJson: jsonString(record.linkedPrs), lastSeenOpenAt, payloadJson: jsonString(compactGitHubPayload(issue)), + githubUpdatedAt: resolvedGithubUpdatedAt, updatedAt: nowIso(), }, }); - return record; + // The returned record reflects what was actually PERSISTED (matching upsertPullRequestFromGitHub's own + // contract) — a delayed job must not keep acting on its stale in-process snapshot. + return { ...record, state: resolvedState, labels: parseJson(resolvedLabelsJson, []) }; } export async function getRepository(env: Env, fullName: string): Promise { diff --git a/src/db/schema.ts b/src/db/schema.ts index b73fd04452..cd23d68d9b 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -542,6 +542,8 @@ export const issues = sqliteTable( linkedPrsJson: text("linked_prs_json").notNull().default("[]"), lastSeenOpenAt: text("last_seen_open_at"), payloadJson: text("payload_json").notNull().default("{}"), + // #8804: GitHub's OWN updated_at for the reorder guard (mirrors pull_requests.github_updated_at, 0172). + githubUpdatedAt: text("github_updated_at"), createdAt: text("created_at").notNull().$defaultFn(() => nowIso()), updatedAt: text("updated_at").notNull().$defaultFn(() => nowIso()), }, diff --git a/src/queue/processors.ts b/src/queue/processors.ts index d5584ab309..61f9871c75 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -3555,7 +3555,15 @@ export async function reReviewStoredPullRequest( } return false; } - if (live?.head?.sha && live.head.sha !== pr.headSha) { + // #8804: resync on a LABEL mismatch too, not just head drift. A label-only change — exactly what Pass 1 of + // the flag-then-close machine produces (pending-closure), or a maintainer applying/removing manual-review — + // arrives via a `labeled` webhook that can still be queued behind this sweep pass; the live fetch already + // carries the current labels, so discarding them meant Pass 2 could misread the stored stale label set and + // re-run Pass 1 (duplicate warning comment, delayed enforcement). Sorted-set comparison: order is not signal. + const liveLabelNames = (live?.labels ?? []).map((label) => label.name ?? "").filter(Boolean).sort(); + const storedLabelNames = [...pr.labels].sort(); + const labelsDrifted = live !== undefined && JSON.stringify(liveLabelNames) !== JSON.stringify(storedLabelNames); + if (live?.head?.sha && (live.head.sha !== pr.headSha || labelsDrifted)) { await upsertPullRequestFromGitHub(env, repoFullName, live).catch( () => undefined, ); diff --git a/test/unit/db-parsers.test.ts b/test/unit/db-parsers.test.ts index a2be0bd93d..14ee3e8e80 100644 --- a/test/unit/db-parsers.test.ts +++ b/test/unit/db-parsers.test.ts @@ -32,6 +32,8 @@ import { recordAuditEvent, recordWebhookEvent, upsertOfficialMinerDetection, + getIssue, + upsertIssueFromGitHub, upsertPullRequestFromGitHub, extractLinkedIssueNumbers, extractLinkedIssueNumbersWithOverflow, @@ -686,6 +688,31 @@ describe("database row parser hardening", () => { expect(synced.state).toBe("closed"); // nothing stored to compare against -> guard can't prove staleness, applies the write }); + it("#8804: LABELS are guarded too -- a stale payload cannot wipe a just-applied disposition label (the pending-closure two-pass machine's Pass-2 key)", async () => { + const env = createTestEnv(); + await upsertPullRequestFromGitHub(env, "owner/repo", { + number: 35, title: "PR", state: "open", user: { login: "bob" }, head: { sha: "a1" }, labels: [{ name: "pending-closure" }], updated_at: "2026-07-21T12:21:17.000Z", + }); + const stale = await upsertPullRequestFromGitHub(env, "owner/repo", { + number: 35, title: "PR", state: "open", user: { login: "bob" }, head: { sha: "a1" }, labels: [], updated_at: "2026-07-21T12:15:52.000Z", + }); + expect(stale.labels).toEqual(["pending-closure"]); // the returned record agrees with what was persisted + const stored = await getPullRequest(env, "owner/repo", 35); + expect(stored?.labels).toEqual(["pending-closure"]); + }); + + it("#8804: a NEWER payload's labels still apply normally (removal included)", async () => { + const env = createTestEnv(); + await upsertPullRequestFromGitHub(env, "owner/repo", { + number: 36, title: "PR", state: "open", user: { login: "bob" }, head: { sha: "a1" }, labels: [{ name: "pending-closure" }], updated_at: "2026-07-21T12:00:00.000Z", + }); + const fresh = await upsertPullRequestFromGitHub(env, "owner/repo", { + number: 36, title: "PR", state: "open", user: { login: "bob" }, head: { sha: "a1" }, labels: [], updated_at: "2026-07-21T12:05:00.000Z", + }); + expect(fresh.labels).toEqual([]); + expect((await getPullRequest(env, "owner/repo", 36))?.labels).toEqual([]); + }); + it("mergedAt is guarded the same way -- a stale payload cannot regress a real merge back to null", async () => { const env = createTestEnv(); await upsertPullRequestFromGitHub(env, "owner/repo", { @@ -1640,3 +1667,46 @@ describe("listPullRequests / listAllPullRequests ordering (#ops-anomaly-calibrat expect(ordered).toEqual(["owner/repo-b#1", "owner/repo-a#2", "owner/repo-a#1"]); }); }); + +describe("upsertIssueFromGitHub out-of-order webhook guard (#8804)", () => { + it("REGRESSION: a delayed OLDER issue webhook cannot regress state or wipe a just-applied label", async () => { + const env = createTestEnv(); + await upsertIssueFromGitHub(env, "owner/repo", { + number: 50, title: "Issue", state: "closed", user: { login: "bob" }, labels: [{ name: "gittensor:bug" }], updated_at: "2026-07-21T12:21:17.000Z", + }); + const stale = await upsertIssueFromGitHub(env, "owner/repo", { + number: 50, title: "Issue", state: "open", user: { login: "bob" }, labels: [], updated_at: "2026-07-21T12:15:52.000Z", + }); + // The returned record agrees with what was persisted (the upsertPullRequestFromGitHub contract). + expect(stale.state).toBe("closed"); + expect(stale.labels).toEqual(["gittensor:bug"]); + const stored = await getIssue(env, "owner/repo", 50); + expect(stored?.state).toBe("closed"); + expect(stored?.labels).toEqual(["gittensor:bug"]); + }); + + it("a NEWER issue webhook still applies normally", async () => { + const env = createTestEnv(); + await upsertIssueFromGitHub(env, "owner/repo", { + number: 51, title: "Issue", state: "open", user: { login: "bob" }, labels: [{ name: "help wanted" }], updated_at: "2026-07-21T12:00:00.000Z", + }); + const fresh = await upsertIssueFromGitHub(env, "owner/repo", { + number: 51, title: "Issue", state: "closed", user: { login: "bob" }, labels: [], updated_at: "2026-07-21T12:05:00.000Z", + }); + expect(fresh.state).toBe("closed"); + expect(fresh.labels).toEqual([]); + }); + + it("fails OPEN on a sparse payload (no updated_at) and on a pre-migration row (no stored timestamp)", async () => { + const env = createTestEnv(); + // Pre-migration shape: first sync carries no updated_at -> nothing stored to compare against later. + await upsertIssueFromGitHub(env, "owner/repo", { number: 52, title: "Issue", state: "open", user: { login: "bob" }, labels: [] }); + const synced = await upsertIssueFromGitHub(env, "owner/repo", { + number: 52, title: "Issue", state: "closed", user: { login: "bob" }, labels: [], updated_at: "2026-07-21T12:00:00.000Z", + }); + expect(synced.state).toBe("closed"); + // Sparse follow-up (no updated_at at all) applies the write exactly as before the guard existed. + const sparse = await upsertIssueFromGitHub(env, "owner/repo", { number: 52, title: "Issue", state: "open", user: { login: "bob" }, labels: [] }); + expect(sparse.state).toBe("open"); + }); +}); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index f6c0b152bc..383c06b872 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -1365,8 +1365,10 @@ describe("queue processors", () => { vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { const url = input.toString(); if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); - // GET /pulls/7 reports the SAME head a7 — no drift, so the resync upsert must not fire. - if (url.endsWith("/pulls/7")) return Response.json({ number: 7, title: "Current PR", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, labels: [], body: "Closes #1" }); + // GET /pulls/7 reports the SAME head a7 — no drift, so the resync upsert must not fire. Labels are + // OMITTED from the live payload (the #8804 drift check's ?? [] arm): absent live labels vs stored [] + // is not drift either. + if (url.endsWith("/pulls/7")) return Response.json({ number: 7, title: "Current PR", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, 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;" }]); if (url.includes("/commits/a7/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); if (url.includes("/commits/a7/status")) return Response.json({ state: "success", statuses: [] }); @@ -1385,6 +1387,35 @@ describe("queue processors", () => { resyncUpsertSpy.mockRestore(); }); + it("#8804: re-review RESYNCS on a LABEL-ONLY drift (same head) — the live labels the sweep already fetched are persisted, not discarded", 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: {}, 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" }, gatePack: "oss-anti-slop" }); + await upsertRepoFocusManifest(env, "owner/agent-repo", { settings: { checkRunMode: "off", commentMode: "off", publicSurface: "off", aiReviewMode: "off", reviewCheckMode: "required" } }); + // STORED labels are stale-empty (the `labeled` webhook is still queued behind this sweep); live has the + // pending-closure label Pass 1 of the flag-then-close machine just applied. Same head — label-only drift. + await upsertPullRequestFromGitHub(env, "owner/agent-repo", { number: 7, title: "Labeled PR", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, labels: [], body: "Closes #1" }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/pulls/7")) return Response.json({ number: 7, title: "Labeled PR", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, labels: [{ name: "pending-closure" }, {}], 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;" }]); + if (url.includes("/commits/a7/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/a7/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); + vi.setSystemTime(new Date("2026-05-28T02:00:00.000Z")); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "resync-label-drift", repoFullName: "owner/agent-repo", prNumber: 7, installationId: 9001 }); + + const stored = await getPullRequest(env, "owner/agent-repo", 7); + expect(stored?.labels).toEqual(["pending-closure"]); // the label-only drift was persisted + expect(stored?.headSha).toBe("a7"); // no head change involved + }); + it("#regate-terminal-exit: a swept PR CLOSED on GitHub reconciles the stored row then early-exits — no files/CI reads, no review (#1942)", 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: {}, events: [] } });