From 5f628ac7baeb3e15458916792d482e753c3d39e3 Mon Sep 17 00:00:00 2001 From: Jon Wiggins Date: Mon, 20 Jul 2026 17:13:02 -0600 Subject: [PATCH] fix: verify scraped PR URLs against the git platform before pr_opened MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A /pull/N URL matching the task's repo in agent output was trusted as proof that the task opened a PR — including example URLs echoed straight from the prompt. Before transitioning to pr_opened, ask the platform (GitHub/GitLab/CodeCommit via the GitPlatform abstraction) whether an open PR exists for the task's deterministic branch (optio/task-): - verified: use the canonical PR URL from the platform - no PR on the branch: ignore the scraped URL (logged) and clear any prUrl persisted mid-stream, so the task escalates to needs_attention (completed_without_pr) instead of pr_opened - platform unavailable (no token / API error): fall back to the previous trust-the-logs behavior Fixes #531 --- .../src/services/pr-detection-service.test.ts | 142 +++++++++++++++++- apps/api/src/services/pr-detection-service.ts | 77 +++++++++- apps/api/src/services/task-service.ts | 12 ++ apps/api/src/workers/task-worker.ts | 59 +++++++- 4 files changed, 275 insertions(+), 15 deletions(-) diff --git a/apps/api/src/services/pr-detection-service.test.ts b/apps/api/src/services/pr-detection-service.test.ts index 03767f19..e9dd0b29 100644 --- a/apps/api/src/services/pr-detection-service.test.ts +++ b/apps/api/src/services/pr-detection-service.test.ts @@ -1,5 +1,10 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; -import { parseOwnerRepo, checkExistingPr } from "./pr-detection-service.js"; +import { + parseOwnerRepo, + checkExistingPr, + verifyTaskPr, + resolveDetectedPrUrl, +} from "./pr-detection-service.js"; // Mock git-token-service const mockPlatform = { @@ -170,3 +175,138 @@ describe("checkExistingPr", () => { }); }); }); + +function makePr(overrides: Record = {}) { + return { + url: "https://github.com/owner/repo/pull/42", + number: 42, + state: "open", + title: "", + body: "", + merged: false, + mergeable: true, + draft: false, + headSha: "abc", + baseBranch: "main", + author: "", + assignees: [], + labels: [], + createdAt: "", + updatedAt: "", + ...overrides, + }; +} + +describe("verifyTaskPr", () => { + beforeEach(() => { + vi.clearAllMocks(); + mockGetGitPlatformForRepo.mockResolvedValue({ + platform: mockPlatform, + ri: { + platform: "github", + host: "github.com", + owner: "owner", + repo: "repo", + apiBaseUrl: "https://api.github.com", + }, + }); + }); + + it("returns verified when an open PR exists for the task branch", async () => { + mockPlatform.listOpenPullRequests.mockResolvedValue([makePr()]); + + const result = await verifyTaskPr("https://github.com/owner/repo", "task-123", null); + + expect(result).toEqual({ + status: "verified", + pr: { url: "https://github.com/owner/repo/pull/42", number: 42, state: "open" }, + }); + expect(mockPlatform.listOpenPullRequests).toHaveBeenCalledWith(expect.any(Object), { + branch: "optio/task-task-123", + }); + }); + + it("returns no_pr when the platform reports no open PR for the branch", async () => { + mockPlatform.listOpenPullRequests.mockResolvedValue([]); + + const result = await verifyTaskPr("https://github.com/owner/repo", "task-456", null); + + expect(result).toEqual({ status: "no_pr" }); + }); + + it("returns unavailable when no git token is available", async () => { + mockGetGitPlatformForRepo.mockRejectedValue(new Error("No token")); + + const result = await verifyTaskPr("https://github.com/owner/repo", "task-789", null); + + expect(result).toEqual({ status: "unavailable", reason: "no_git_token" }); + }); + + it("returns unavailable when the platform API errors", async () => { + mockPlatform.listOpenPullRequests.mockRejectedValue(new Error("API error")); + + const result = await verifyTaskPr("https://github.com/owner/repo", "task-err", null); + + expect(result).toEqual({ status: "unavailable", reason: "platform_lookup_failed" }); + }); + + it("returns unavailable for unparseable repo URLs", async () => { + const result = await verifyTaskPr("", "task-bad", null); + + expect(result).toEqual({ status: "unavailable", reason: "unparseable_repo_url" }); + expect(mockGetGitPlatformForRepo).not.toHaveBeenCalled(); + }); +}); + +describe("resolveDetectedPrUrl", () => { + // Issue #531: a /pull/N URL echoed from the prompt must not be treated as + // the task's opened PR when the platform says no PR exists for the branch. + it("rejects a prompt-echoed URL when the platform reports no PR", () => { + const scraped = "https://github.com/owner/repo/pull/5678"; + + const result = resolveDetectedPrUrl(scraped, { status: "no_pr" }); + + expect(result.url).toBeUndefined(); + expect(result.rejectedUrl).toBe(scraped); + }); + + it("accepts a genuine PR whose head is the task branch", () => { + const result = resolveDetectedPrUrl("https://github.com/owner/repo/pull/99", { + status: "verified", + pr: { url: "https://github.com/owner/repo/pull/99", number: 99, state: "open" }, + }); + + expect(result.url).toBe("https://github.com/owner/repo/pull/99"); + expect(result.rejectedUrl).toBeUndefined(); + }); + + it("prefers the canonical platform URL over a differing scraped URL", () => { + const result = resolveDetectedPrUrl("https://github.com/owner/repo/pull/5678", { + status: "verified", + pr: { url: "https://github.com/owner/repo/pull/100", number: 100, state: "open" }, + }); + + expect(result.url).toBe("https://github.com/owner/repo/pull/100"); + }); + + it("falls back to the scraped URL when verification is unavailable", () => { + const scraped = "https://github.com/owner/repo/pull/7"; + + const result = resolveDetectedPrUrl(scraped, { + status: "unavailable", + reason: "no_git_token", + }); + + expect(result.url).toBe(scraped); + expect(result.rejectedUrl).toBeUndefined(); + }); + + it("verified result applies even when no URL was scraped (API-only detection)", () => { + const result = resolveDetectedPrUrl(undefined, { + status: "verified", + pr: { url: "https://github.com/owner/repo/pull/3", number: 3, state: "open" }, + }); + + expect(result.url).toBe("https://github.com/owner/repo/pull/3"); + }); +}); diff --git a/apps/api/src/services/pr-detection-service.ts b/apps/api/src/services/pr-detection-service.ts index 1f821068..98880c86 100644 --- a/apps/api/src/services/pr-detection-service.ts +++ b/apps/api/src/services/pr-detection-service.ts @@ -8,6 +8,22 @@ export interface ExistingPr { state: string; } +/** + * Result of verifying a task's PR against the git platform. + * + * - `verified` — the platform confirmed an open PR whose head/source branch + * is the task's deterministic branch (`optio/task-{taskId}`). + * - `no_pr` — the platform was reachable and authoritatively reported no + * open PR for the task branch. + * - `unavailable` — the platform could not be consulted (no token, unparseable + * repo URL, or API error). Callers should fall back to their + * previous behavior rather than treating this as "no PR". + */ +export type PrVerification = + | { status: "verified"; pr: ExistingPr } + | { status: "no_pr" } + | { status: "unavailable"; reason: string }; + /** * Extract owner and repo from a normalized repo URL. * e.g. "https://github.com/owner/repo" → { owner: "owner", repo: "repo" } @@ -31,10 +47,26 @@ export async function checkExistingPr( taskId: string, workspaceId: string | null, ): Promise { + const verification = await verifyTaskPr(repoUrl, taskId, workspaceId); + return verification.status === "verified" ? verification.pr : null; +} + +/** + * Verify against the git platform whether an open PR exists for a task's + * branch. Unlike {@link checkExistingPr}, this distinguishes "the platform + * says there is no PR" from "the platform could not be consulted", so callers + * can reject unverified PR URLs scraped from agent output without breaking + * platforms/configurations where verification is impossible. + */ +export async function verifyTaskPr( + repoUrl: string, + taskId: string, + _workspaceId: string | null, +): Promise { const ri = parseRepoUrl(repoUrl); if (!ri) { logger.debug({ repoUrl }, "Cannot parse repo URL — skipping PR check"); - return null; + return { status: "unavailable", reason: "unparseable_repo_url" }; } let platform; @@ -43,7 +75,7 @@ export async function checkExistingPr( platform = result.platform; } catch { logger.debug("No git token available — skipping existing PR check"); - return null; + return { status: "unavailable", reason: "no_git_token" }; } const branch = `${TASK_BRANCH_PREFIX}${taskId}`; @@ -51,16 +83,47 @@ export async function checkExistingPr( try { const pulls = await platform.listOpenPullRequests(ri, { branch }); - if (pulls.length === 0) return null; + if (pulls.length === 0) return { status: "no_pr" }; const pr = pulls[0]; return { - url: pr.url, - number: pr.number, - state: pr.state, + status: "verified", + pr: { + url: pr.url, + number: pr.number, + state: pr.state, + }, }; } catch (err) { logger.debug({ err }, "Failed to check for existing PR"); - return null; + return { status: "unavailable", reason: "platform_lookup_failed" }; + } +} + +/** + * Decide which PR URL (if any) a task should trust, given a URL scraped from + * agent output and the platform verification result. + * + * A `/pull/N` URL in agent output is not proof that a PR was opened — it may + * be an example URL echoed from the prompt (see issue #531). The task branch + * is deterministic, so the platform's answer for that branch is authoritative: + * + * - `verified` → use the canonical URL reported by the platform (it wins + * even over a differing scraped URL). + * - `no_pr` → reject the scraped URL (`rejectedUrl` is set so callers + * can log the skip). + * - `unavailable` → fall back to trusting the scraped URL (legacy behavior). + */ +export function resolveDetectedPrUrl( + scrapedUrl: string | undefined, + verification: PrVerification, +): { url: string | undefined; rejectedUrl?: string } { + switch (verification.status) { + case "verified": + return { url: verification.pr.url }; + case "no_pr": + return { url: undefined, rejectedUrl: scrapedUrl }; + case "unavailable": + return { url: scrapedUrl }; } } diff --git a/apps/api/src/services/task-service.ts b/apps/api/src/services/task-service.ts index 1087fdc6..09ae5caf 100644 --- a/apps/api/src/services/task-service.ts +++ b/apps/api/src/services/task-service.ts @@ -443,6 +443,18 @@ export async function updateTaskPr(id: string, prUrl: string) { .where(eq(tasks.id, id)); } +/** + * Clear a task's PR association. Used when a PR URL captured from agent + * output fails platform verification (e.g. an example URL echoed from the + * prompt) and must not be treated as the task's opened PR. + */ +export async function clearTaskPr(id: string) { + await db + .update(tasks) + .set({ prUrl: null, prNumber: null, updatedAt: new Date() }) + .where(eq(tasks.id, id)); +} + export async function updateTaskSession(id: string, sessionId: string) { await db.update(tasks).set({ sessionId, updatedAt: new Date() }).where(eq(tasks.id, id)); } diff --git a/apps/api/src/workers/task-worker.ts b/apps/api/src/workers/task-worker.ts index 41397c1b..49871bd0 100644 --- a/apps/api/src/workers/task-worker.ts +++ b/apps/api/src/workers/task-worker.ts @@ -21,7 +21,12 @@ import { parseCopilotEvent } from "../services/copilot-event-parser.js"; import { parseOpenCodeEvent } from "../services/opencode-event-parser.js"; import { parseGeminiEvent } from "../services/gemini-event-parser.js"; import { parseOpenClawEvent } from "../services/openclaw-event-parser.js"; -import { checkExistingPr, type ExistingPr } from "../services/pr-detection-service.js"; +import { + checkExistingPr, + resolveDetectedPrUrl, + verifyTaskPr, + type ExistingPr, +} from "../services/pr-detection-service.js"; import { db } from "../db/client.js"; import { tasks } from "../db/schema.js"; import { eq, sql } from "drizzle-orm"; @@ -1145,7 +1150,45 @@ export function startTaskWorker() { fallbackPrUrl = undefined; } } - const detectedPrUrl = capturedPrUrl || taskAfterExec?.prUrl || fallbackPrUrl; + const scrapedPrUrl = capturedPrUrl || taskAfterExec?.prUrl || fallbackPrUrl || undefined; + + // A `/pull/N` URL in agent output is not proof that a PR was opened — + // it may be an example URL echoed from the prompt (issue #531). The + // task branch is deterministic (`optio/task-{id}`), so ask the git + // platform whether an open PR actually exists for it before trusting + // any scraped URL. If the platform can't be consulted (no token, API + // error), fall back to the previous trust-the-logs behavior. + let detectedPrUrl = scrapedPrUrl; + // Set when the platform authoritatively reported no open PR for the + // task branch — lets later API-fallback checks skip a redundant call. + let prKnownAbsent = false; + if (scrapedPrUrl && !isReviewTask) { + const verification = await verifyTaskPr(task.repoUrl, taskId, taskWorkspaceId); + const resolved = resolveDetectedPrUrl(scrapedPrUrl, verification); + detectedPrUrl = resolved.url; + if (verification.status === "no_pr") { + prKnownAbsent = true; + log.warn( + { rejectedPrUrl: resolved.rejectedUrl }, + "Ignoring PR URL from agent output — platform reports no open PR for the task branch", + ); + if (taskAfterExec?.prUrl) { + // A bogus URL was already persisted during streaming — clear it + // so the task doesn't advertise a PR that was never opened. + await taskService.clearTaskPr(taskId); + } + } else if (verification.status === "unavailable") { + log.info( + { prUrl: scrapedPrUrl, reason: verification.reason }, + "PR verification unavailable — falling back to PR URL from agent output", + ); + } else if (detectedPrUrl !== scrapedPrUrl) { + log.info( + { scrapedPrUrl, verifiedPrUrl: detectedPrUrl }, + "Using canonical PR URL from platform instead of URL scraped from agent output", + ); + } + } if (!sessionId && !isReviewTask) { // Agent never started — no session ID means no agent output was produced. @@ -1200,10 +1243,12 @@ export function startTaskWorker() { // check the API as a fallback — the agent may have pushed a PR // that wasn't captured in log output. let apiFallbackPr: ExistingPr | null = null; - try { - apiFallbackPr = await checkExistingPr(task.repoUrl, taskId, taskWorkspaceId); - } catch { - // Non-fatal — proceed with escalation + if (!prKnownAbsent) { + try { + apiFallbackPr = await checkExistingPr(task.repoUrl, taskId, taskWorkspaceId); + } catch { + // Non-fatal — proceed with escalation + } } if (apiFallbackPr) { @@ -1245,7 +1290,7 @@ export function startTaskWorker() { // Log-based PR detection can miss URLs (e.g. agent created a PR but // the URL wasn't in stdout, or repo validation filtered it out). let apiFallbackPr: ExistingPr | null = null; - if (!isReviewTask) { + if (!isReviewTask && !prKnownAbsent) { try { apiFallbackPr = await checkExistingPr(task.repoUrl, taskId, taskWorkspaceId); } catch {