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
142 changes: 141 additions & 1 deletion apps/api/src/services/pr-detection-service.test.ts
Original file line number Diff line number Diff line change
@@ -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 = {
Expand Down Expand Up @@ -170,3 +175,138 @@ describe("checkExistingPr", () => {
});
});
});

function makePr(overrides: Record<string, unknown> = {}) {
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");
});
});
77 changes: 70 additions & 7 deletions apps/api/src/services/pr-detection-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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" }
Expand All @@ -31,10 +47,26 @@ export async function checkExistingPr(
taskId: string,
workspaceId: string | null,
): Promise<ExistingPr | null> {
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<PrVerification> {
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;
Expand All @@ -43,24 +75,55 @@ 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}`;

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 };
}
}
12 changes: 12 additions & 0 deletions apps/api/src/services/task-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down
59 changes: 52 additions & 7 deletions apps/api/src/workers/task-worker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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 {
Expand Down
Loading