From 0acbc860b222e28962d97e748b85e286660f2381 Mon Sep 17 00:00:00 2001 From: Courier Date: Sun, 27 Sep 2026 20:46:26 +0000 Subject: [PATCH 1/2] fix(pr-fix): keep the item URL as the PR URL when CI evidence re-enqueues it The item URL is identity: metadataPatch no longer carries url, so a CI-failure re-enqueue can never flip it to a job URL. url is set at first enqueue and backfilled only while empty. Ingestion now passes the PR URL as the event url for check_run events; the job URL moves to checkRunUrl and stays evidence in the feedback text ("Full log:" line). Closes #1098 --- src/app/api/pr-followup/sync/route.ts | 3 +- src/app/api/pr-followup/webhook/route.test.ts | 56 ++++++++++++++- src/app/api/pr-followup/webhook/route.ts | 6 +- src/lib/pr-fix-queue.test.ts | 72 +++++++++++++++++++ src/lib/pr-fix-queue.ts | 13 +++- src/lib/pr-followup-ingestion.test.ts | 57 +++++++++++++++ src/lib/pr-followup-ingestion.ts | 18 ++++- 7 files changed, 219 insertions(+), 6 deletions(-) diff --git a/src/app/api/pr-followup/sync/route.ts b/src/app/api/pr-followup/sync/route.ts index b96cb620..52f4c2a4 100644 --- a/src/app/api/pr-followup/sync/route.ts +++ b/src/app/api/pr-followup/sync/route.ts @@ -287,7 +287,8 @@ export async function POST(request: NextRequest) { repoFullName, prNumber: pr.number, branch: pr.head?.ref ?? null, - url: checkRun.html_url, + url: pr.url, + checkRunUrl: checkRun.html_url, title: checkRun.name, author: pr.user.login, body: excerpt || checkRun.output?.summary || "", diff --git a/src/app/api/pr-followup/webhook/route.test.ts b/src/app/api/pr-followup/webhook/route.test.ts index e56fa16b..a0d73356 100644 --- a/src/app/api/pr-followup/webhook/route.test.ts +++ b/src/app/api/pr-followup/webhook/route.test.ts @@ -680,7 +680,8 @@ describe("POST /api/pr-followup/webhook — event dispatch", () => { repoFullName: "org/repo", prNumber: 42, branch: "fix/issue-7", - url: "https://github.com/org/repo/runs/321", + url: "https://api.github.com/repos/org/repo/pulls/42", + checkRunUrl: "https://github.com/org/repo/runs/321", title: "lint", author: null, body: "2 errors", @@ -693,6 +694,59 @@ describe("POST /api/pr-followup/webhook — event dispatch", () => { ]); }); + // #1098: when the associated PR object has no `url`, the event's `url` must + // be the empty string (falsy → the next sync enqueue backfills the real PR + // URL), never the CI job URL, which the write-once queue guard would freeze + // as the item identity. The job URL belongs in `checkRunUrl`. + it("sets url to the empty string when the associated PR has no url (#1098)", async () => { + const res = await signedRequest("check_run", { + action: "completed", + check_run: { + id: 324, + name: "lint", + head_sha: "abc123", + status: "completed", + conclusion: "failure", + url: "https://api.github.com/repos/org/repo/check-runs/324", + html_url: "https://github.com/org/repo/runs/324", + details_url: "https://github.com/org/repo/actions/runs/1/job/324", + output: { title: "Lint failed", summary: "2 errors", text: null, annotations_count: 2 }, + check_suite: { id: 8, head_branch: "fix/issue-7", head_sha: "abc123" }, + app: { slug: "github-actions" }, + pull_requests: [ + { + id: 9001, + number: 42, + head: { ref: "fix/issue-7", sha: "abc123", repo: { id: 1296269, url: "https://api.github.com/repos/org/repo", name: "repo" } }, + base: { ref: "main", sha: "def456", repo: { id: 1296269, url: "https://api.github.com/repos/org/repo", name: "repo" } }, + }, + ], + }, + repository, + sender: { login: "github-actions[bot]", type: "Bot" }, + }); + + expect(res.status).toBe(200); + expect(ingestedEvents()).toEqual([ + { + eventType: "check_run", + repoFullName: "org/repo", + prNumber: 42, + branch: "fix/issue-7", + url: "", + checkRunUrl: "https://github.com/org/repo/runs/324", + title: "lint", + author: null, + body: "2 errors", + id: "324", + conclusion: "failure", + checkName: "lint", + linkedIssue: null, + headSha: "abc123", + }, + ]); + }); + it("falls back to check_suite.head_branch and check_run.head_sha when the PR head is absent", async () => { const res = await signedRequest("check_run", { action: "completed", diff --git a/src/app/api/pr-followup/webhook/route.ts b/src/app/api/pr-followup/webhook/route.ts index c3a82150..ba30e6c9 100644 --- a/src/app/api/pr-followup/webhook/route.ts +++ b/src/app/api/pr-followup/webhook/route.ts @@ -120,7 +120,11 @@ function parseWebhookEvent(githubEvent: string, body: Record): repoFullName: checkRun.repository?.full_name ?? null, prNumber, branch: firstPr?.head?.ref ?? check.check_suite?.head_branch ?? null, - url: check.html_url, + // #1098: `url` is the item's identity and the queue guard is + // write-once — a job-URL fallback would freeze it in place. The empty + // string stays falsy, so the next sync enqueue backfills the real PR URL. + url: typeof firstPr?.url === "string" ? firstPr.url : "", + checkRunUrl: check.html_url, title: check.name, author: null, body: check.output?.summary ?? "", diff --git a/src/lib/pr-fix-queue.test.ts b/src/lib/pr-fix-queue.test.ts index 4a8dfcf5..dfac7747 100644 --- a/src/lib/pr-fix-queue.test.ts +++ b/src/lib/pr-fix-queue.test.ts @@ -274,6 +274,78 @@ describe("PR review-fix queue", () => { }); }); +describe("item URL is identity, write-once (#1098)", () => { + let client: ReturnType; + + beforeEach(() => { + client = makeClient(); + surfacingMocks.surfacePrFixBlocked.mockReset(); + surfacingMocks.surfacePrFixBlocked.mockResolvedValue({ labelApplied: true, commentPosted: true, errors: [] }); + }); + + it("keeps the PR URL when a CI-failure re-enqueue carries a job URL", async () => { + await enqueuePrFixItem(client, { + repo: "o/repo", pr: 7, lane: "NORMAL", type: "REVIEW_FEEDBACK", + reason: "PR review: CHANGES_REQUESTED", feedback: "changes requested", + evidenceKey: "rev-1", + url: "https://api.github.com/repos/o/repo/pulls/7", + }); + + const after = await enqueuePrFixItem(client, { + repo: "o/repo", pr: 7, lane: "NORMAL", type: "CI_FAILURE", + reason: "checks failed", feedback: "build failed", + evidenceKey: "cr-1", + url: "https://github.com/o/repo/actions/runs/9/job/1", + }); + + // The item URL is identity: it must stay the PR URL, never flip to the + // CI job URL, so next-task keeps handing out pullRequest.url. + expect(after.url).toBe("https://api.github.com/repos/o/repo/pulls/7"); + expect(client.items[0].url).toBe("https://api.github.com/repos/o/repo/pulls/7"); + // Evidence is still appended even when the URL is not. + expect(client.items[0].feedback).toEqual(["changes requested", "build failed"]); + expect(client.items[0].evidenceKeys).toEqual(["rev-1", "cr-1"]); + }); + + it("backfills the URL only when the item has none", async () => { + await enqueuePrFixItem(client, { + repo: "o/repo", pr: 8, lane: "NORMAL", type: "REVIEW_FEEDBACK", + reason: "r", feedback: "f1", evidenceKey: "rev-1", + }); + expect(client.items[0].url).toBeUndefined(); + + const after = await enqueuePrFixItem(client, { + repo: "o/repo", pr: 8, lane: "NORMAL", type: "CI_FAILURE", + reason: "r", feedback: "f2", evidenceKey: "cr-1", + url: "https://api.github.com/repos/o/repo/pulls/8", + }); + + expect(after.url).toBe("https://api.github.com/repos/o/repo/pulls/8"); + expect(client.items[0].url).toBe("https://api.github.com/repos/o/repo/pulls/8"); + }); + + it("backfills the URL when the stored value is the empty string (String? column shape)", async () => { + await enqueuePrFixItem(client, { + repo: "o/repo", pr: 8, lane: "NORMAL", type: "CI_FAILURE", + reason: "r", feedback: "f1", evidenceKey: "cr-1", + url: "", + }); + // The webhook enqueues with url: "" when the PR object has no url (#1098); + // a String? column can carry the empty string, which the write-once guard + // must treat as empty. + client.items[0].url = ""; + + const after = await enqueuePrFixItem(client, { + repo: "o/repo", pr: 8, lane: "NORMAL", type: "CI_FAILURE", + reason: "r", feedback: "f2", evidenceKey: "cr-2", + url: "https://api.github.com/repos/o/repo/pulls/8", + }); + + expect(after.url).toBe("https://api.github.com/repos/o/repo/pulls/8"); + expect(client.items[0].url).toBe("https://api.github.com/repos/o/repo/pulls/8"); + }); +}); + describe("work generation identity (#1044)", () => { let client: ReturnType; diff --git a/src/lib/pr-fix-queue.ts b/src/lib/pr-fix-queue.ts index d9a2cb86..2714996a 100644 --- a/src/lib/pr-fix-queue.ts +++ b/src/lib/pr-fix-queue.ts @@ -32,6 +32,7 @@ export interface EnqueuePrFixInput { evidenceKey: string; issue?: number | null; branch?: string | null; + /** Item identity — should be the PR URL. Set at first enqueue, only backfilled while empty (#1098). */ url?: string | null; title?: string | null; headSha?: string | null; @@ -206,12 +207,15 @@ function extractUrlsFromTextSafe(text: string): string[] { } } +// `url` is not part of this patch (#1098): the item URL is identity (the PR +// URL) — set at first enqueue, backfilled only while empty. A CI-failure +// re-enqueue's job URL must not flip it; job links belong in feedback/ +// evidence (pr-followup-ingestion), not the item URL. function metadataPatch(input: EnqueuePrFixInput): Record { const patch: Record = {}; for (const [key, value] of Object.entries({ issue: input.issue ?? undefined, branch: input.branch ?? undefined, - url: input.url ?? undefined, title: input.title ?? undefined, headSha: input.headSha ?? undefined, author: input.author ?? undefined, @@ -324,6 +328,10 @@ export async function enqueuePrFixItem(client: PrFixQueueClient, input: EnqueueP reason: input.reason, feedback: uniqueAppend(existing.feedback ?? [], input.feedback, 12), evidenceKeys: nextEvidenceKeys, + // #1098: the item URL is identity — write-once. Backfill it only + // while empty; never overwrite an existing URL with a + // re-enqueue's value (e.g. a CI job URL). + ...(!existing.url && input.url ? { url: input.url } : {}), // A fresh attempt gets a fresh per-attempt head baseline (#1074): // the head the sync observed in THIS enqueue, else the last one // observed (#1104). metadataPatch below keeps refreshing the mutable @@ -364,6 +372,9 @@ export async function enqueuePrFixItem(client: PrFixQueueClient, input: EnqueueP reason: input.reason, feedback: [input.feedback], evidenceKeys: [input.evidenceKey], + // #1098: the item URL is identity (the PR URL) — set it at first + // enqueue, explicitly, so it is never left to the update patch. + ...(input.url ? { url: input.url } : {}), // A brand-new item is a fresh attempt: capture the head the sync // observed now as its immutable per-attempt baseline (#1074). attemptHeadSha: input.headSha ?? null, diff --git a/src/lib/pr-followup-ingestion.test.ts b/src/lib/pr-followup-ingestion.test.ts index 34f25fda..bf766c29 100644 --- a/src/lib/pr-followup-ingestion.test.ts +++ b/src/lib/pr-followup-ingestion.test.ts @@ -1011,6 +1011,63 @@ describe("processPrFollowupEvents", () => { error.mockRestore(); }); + it("enqueues a check_run with the PR URL as the item URL and the job URL in feedback (#1098)", async () => { + process.env.PR_FOLLOWUP_BOT_IDENTITIES = "itsmiso-ai"; + const client = makeClient(); + + const result = await processPrFollowupEvents(client, [ + { + eventType: "check_run", + repoFullName: "org/repo", + prNumber: 7, + branch: "fix/c", + url: "https://api.github.com/repos/org/repo/pulls/7", + checkRunUrl: "https://github.com/org/repo/actions/runs/9/job/1", + title: "Fix C", + author: "itsmiso-ai", + body: "Error: Cannot read property 'x' of undefined", + id: "cr1098", + conclusion: "failure", + checkName: "lint", + }, + ]); + + expect(result.enqueued).toBe(1); + expect(client.items).toHaveLength(1); + // The item URL is the PR URL, not the CI job URL. + expect(client.items[0].url).toBe("https://api.github.com/repos/org/repo/pulls/7"); + // The job URL is evidence in the feedback text ("Full log:" line). + const feedback = String(client.items[0].feedback); + expect(feedback).toContain("Full log: https://github.com/org/repo/actions/runs/9/job/1"); + expect(feedback).not.toContain("pulls/7"); + }); + + it("keeps the job URL in feedback when checkRunUrl is absent (legacy shape)", async () => { + process.env.PR_FOLLOWUP_BOT_IDENTITIES = "itsmiso-ai"; + const client = makeClient(); + + const result = await processPrFollowupEvents(client, [ + { + eventType: "check_run", + repoFullName: "org/repo", + prNumber: 7, + branch: "fix/c", + url: "https://github.com/org/repo/actions/runs/9/job/1", + title: "Fix C", + author: "itsmiso-ai", + body: "Error: Cannot read property 'x' of undefined", + id: "cr1098-legacy", + conclusion: "failure", + checkName: "lint", + }, + ]); + + expect(result.enqueued).toBe(1); + expect(client.items).toHaveLength(1); + const feedback = String(client.items[0].feedback); + expect(feedback).toContain("Full log: https://github.com/org/repo/actions/runs/9/job/1"); + }); + afterEach(() => { delete process.env.PR_FOLLOWUP_BOT_IDENTITIES; }); diff --git a/src/lib/pr-followup-ingestion.ts b/src/lib/pr-followup-ingestion.ts index 88ec2b39..78a9d2c7 100644 --- a/src/lib/pr-followup-ingestion.ts +++ b/src/lib/pr-followup-ingestion.ts @@ -439,13 +439,17 @@ const INGEST_DESCRIPTORS: Record // checks rarely set output.summary, so without this the coder gets a // contentless "check failed" and fixes blind). Present the real error + // the log URL for reference; degrade to reason + URL when no excerpt. + // The job URL is evidence, not the item URL: `url` is the PR URL and the + // job URL belongs in the feedback text. The `?? event.url` fallback keeps + // legacy callers (no checkRunUrl) behaving as before (#1098). + const logUrl = event.checkRunUrl ?? event.url; const excerpt = event.body?.trim(); const lines = [`CI check "${checkName}" failed (${event.conclusion}) on this PR.`]; if (excerpt) { lines.push("", "Error from the job log:", excerpt); } - if (event.url) { - lines.push("", `Full log: ${event.url}`); + if (logUrl) { + lines.push("", `Full log: ${logUrl}`); } if (!excerpt) { lines.push("Read the full log at the URL above to find the error, then fix the root cause."); @@ -674,6 +678,9 @@ export async function ingestReviewCommentEvent( /** * Ingest a failing check run event. + * + * `url` must be the PR URL; the check-run job URL goes in `checkRunUrl` + * (#1098) — it is evidence for the feedback text, not the item identity. */ export async function ingestCheckRunEvent( client: PrFixQueueClient, @@ -687,6 +694,7 @@ export async function ingestCheckRunEvent( checkName: string; conclusion: string; // "failure", "cancelled", "timed_out" etc. checkRunId: string; + checkRunUrl?: string | null; checkDetails?: string; linkedIssue?: number | null; }, @@ -701,6 +709,7 @@ export async function ingestCheckRunEvent( author: opts.author, body: opts.checkDetails, id: opts.checkRunId, + checkRunUrl: opts.checkRunUrl ?? null, conclusion: opts.conclusion, checkName: opts.checkName, linkedIssue: opts.linkedIssue, @@ -857,6 +866,11 @@ export interface PrFollowupEvent { state?: string; conclusion?: string; checkName?: string; + /** + * The check-run job URL for `check_run` events. `url` is always the PR URL; + * the job URL is evidence for the feedback text (#1098). + */ + checkRunUrl?: string | null; mergeStateStatus?: string; prState?: string | null; prMergedAt?: string | null; From ad33042b1089c839c991fd540ce31df604fa60f6 Mon Sep 17 00:00:00 2001 From: Jory Irving Date: Sun, 27 Sep 2026 15:34:41 -0600 Subject: [PATCH 2/2] fix(pr-fix): heal and repair item URLs poisoned with CI job URLs --- .../migration.sql | 6 +++ src/lib/pr-fix-queue.test.ts | 49 +++++++++++++++++++ src/lib/pr-fix-queue.ts | 25 ++++++++-- 3 files changed, 77 insertions(+), 3 deletions(-) create mode 100644 prisma/migrations/20261004000000_repair_pr_fix_item_urls/migration.sql diff --git a/prisma/migrations/20261004000000_repair_pr_fix_item_urls/migration.sql b/prisma/migrations/20261004000000_repair_pr_fix_item_urls/migration.sql new file mode 100644 index 00000000..6b4a963a --- /dev/null +++ b/prisma/migrations/20261004000000_repair_pr_fix_item_urls/migration.sql @@ -0,0 +1,6 @@ +-- Repair PR-fix item URLs that the pre-#1098 ingestion overwrote with a CI job +-- URL (#1118). The item URL is identity and is now write-once, so these rows +-- would otherwise keep the job URL. repo + pr already identify the PR. +UPDATE "PrFixQueueItem" +SET "url" = 'https://api.github.com/repos/' || "repo" || '/pulls/' || "pr" +WHERE "url" ~ '^https://github\.com/[^/]+/[^/]+/actions/runs/'; diff --git a/src/lib/pr-fix-queue.test.ts b/src/lib/pr-fix-queue.test.ts index dfac7747..1bed5fa5 100644 --- a/src/lib/pr-fix-queue.test.ts +++ b/src/lib/pr-fix-queue.test.ts @@ -307,6 +307,55 @@ describe("item URL is identity, write-once (#1098)", () => { expect(client.items[0].evidenceKeys).toEqual(["rev-1", "cr-1"]); }); + it("repairs a stored CI job URL on the next enqueue that carries the PR URL (#1118)", async () => { + await enqueuePrFixItem(client, { + repo: "o/repo", pr: 9, lane: "NORMAL", type: "CI_FAILURE", + reason: "r", feedback: "f1", evidenceKey: "cr-1", + }); + // A row poisoned by the pre-#1098 ingestion. + client.items[0].url = "https://github.com/o/repo/actions/runs/9/job/1"; + + const after = await enqueuePrFixItem(client, { + repo: "o/repo", pr: 9, lane: "NORMAL", type: "REVIEW_FEEDBACK", + reason: "r", feedback: "f2", evidenceKey: "rev-1", + url: "https://api.github.com/repos/o/repo/pulls/9", + }); + + expect(after.url).toBe("https://api.github.com/repos/o/repo/pulls/9"); + }); + + it("leaves a legitimate stored URL alone even when a different one arrives", async () => { + await enqueuePrFixItem(client, { + repo: "o/repo", pr: 10, lane: "NORMAL", type: "REVIEW_FEEDBACK", + reason: "r", feedback: "f1", evidenceKey: "rev-1", + url: "https://github.com/o/repo/pull/10", + }); + + const after = await enqueuePrFixItem(client, { + repo: "o/repo", pr: 10, lane: "NORMAL", type: "REVIEW_FEEDBACK", + reason: "r", feedback: "f2", evidenceKey: "rev-2", + url: "https://api.github.com/repos/o/repo/pulls/10", + }); + + expect(after.url).toBe("https://github.com/o/repo/pull/10"); + }); + + it("never stores a CI job URL as the item URL, on create or on backfill (#1118)", async () => { + const created = await enqueuePrFixItem(client, { + repo: "o/repo", pr: 11, lane: "NORMAL", type: "CI_FAILURE", + reason: "r", feedback: "f1", evidenceKey: "cr-1", + url: "https://github.com/o/repo/actions/runs/9/job/1", + }); + expect(created.url).toBeUndefined(); + + const reenqueued = await enqueuePrFixItem(client, { + repo: "o/repo", pr: 11, lane: "NORMAL", type: "CI_FAILURE", + reason: "r", feedback: "f2", evidenceKey: "cr-2", + url: "https://github.com/o/repo/actions/runs/10/job/2", + }); + expect(reenqueued.url).toBeUndefined(); + }); + it("backfills the URL only when the item has none", async () => { await enqueuePrFixItem(client, { repo: "o/repo", pr: 8, lane: "NORMAL", type: "REVIEW_FEEDBACK", diff --git a/src/lib/pr-fix-queue.ts b/src/lib/pr-fix-queue.ts index 2714996a..5b18df94 100644 --- a/src/lib/pr-fix-queue.ts +++ b/src/lib/pr-fix-queue.ts @@ -207,6 +207,21 @@ function extractUrlsFromTextSafe(text: string): string[] { } } +/** + * A GitHub Actions run/job URL: what the pre-#1098 ingestion stored as an + * item URL for CI failures. Never valid item identity (#1118). + */ +const ACTIONS_RUN_URL = /^https:\/\/github\.com\/[^/]+\/[^/]+\/actions\/runs\//; + +function isActionsRunUrl(url: string | null | undefined): boolean { + return typeof url === "string" && ACTIONS_RUN_URL.test(url); +} + +/** The item URL an enqueue may write: never a CI job URL (#1098, #1118). */ +function itemUrlFromInput(input: EnqueuePrFixInput): string | undefined { + return input.url && !isActionsRunUrl(input.url) ? input.url : undefined; +} + // `url` is not part of this patch (#1098): the item URL is identity (the PR // URL) — set at first enqueue, backfilled only while empty. A CI-failure // re-enqueue's job URL must not flip it; job links belong in feedback/ @@ -330,8 +345,12 @@ export async function enqueuePrFixItem(client: PrFixQueueClient, input: EnqueueP evidenceKeys: nextEvidenceKeys, // #1098: the item URL is identity — write-once. Backfill it only // while empty; never overwrite an existing URL with a - // re-enqueue's value (e.g. a CI job URL). - ...(!existing.url && input.url ? { url: input.url } : {}), + // re-enqueue's value (e.g. a CI job URL). A stored CI job URL left + // by the pre-#1098 ingestion counts as empty, so it heals on the + // next enqueue instead of being frozen by the guard (#1118). + ...((!existing.url || isActionsRunUrl(existing.url)) && itemUrlFromInput(input) + ? { url: itemUrlFromInput(input) } + : {}), // A fresh attempt gets a fresh per-attempt head baseline (#1074): // the head the sync observed in THIS enqueue, else the last one // observed (#1104). metadataPatch below keeps refreshing the mutable @@ -374,7 +393,7 @@ export async function enqueuePrFixItem(client: PrFixQueueClient, input: EnqueueP evidenceKeys: [input.evidenceKey], // #1098: the item URL is identity (the PR URL) — set it at first // enqueue, explicitly, so it is never left to the update patch. - ...(input.url ? { url: input.url } : {}), + ...(itemUrlFromInput(input) ? { url: itemUrlFromInput(input) } : {}), // A brand-new item is a fresh attempt: capture the head the sync // observed now as its immutable per-attempt baseline (#1074). attemptHeadSha: input.headSha ?? null,