Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions migrations/0175_issues_github_updated_at.sql
Original file line number Diff line number Diff line change
@@ -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;
43 changes: 34 additions & 9 deletions src/db/repositories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)))
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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<string[]>(resolvedLabelsJson, []), body, linkedIssues, linkedIssueClaimedAt, bodyObservedAt, headShaObservedAt };
}

function resolveLinkedIssueClaimedAt(
Expand Down Expand Up @@ -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<IssueRecord> {
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)
Expand All @@ -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<string[]>(resolvedLabelsJson, []) };
}

export async function getRepository(env: Env, fullName: string): Promise<RepositoryRecord | null> {
Expand Down
2 changes: 2 additions & 0 deletions src/db/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()),
},
Expand Down
10 changes: 9 additions & 1 deletion src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
Expand Down
70 changes: 70 additions & 0 deletions test/unit/db-parsers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ import {
recordAuditEvent,
recordWebhookEvent,
upsertOfficialMinerDetection,
getIssue,
upsertIssueFromGitHub,
upsertPullRequestFromGitHub,
extractLinkedIssueNumbers,
extractLinkedIssueNumbersWithOverflow,
Expand Down Expand Up @@ -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", {
Expand Down Expand Up @@ -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");
});
});
35 changes: 33 additions & 2 deletions test/unit/queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: [] });
Expand All @@ -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: [] } });
Expand Down