From 2eafa41865c606260799489be9ddfe20dc04bfb7 Mon Sep 17 00:00:00 2001 From: guitavano Date: Wed, 12 Aug 2026 14:05:34 -0300 Subject: [PATCH] fix(task-board): a repo-backed task reaches In Review only with a PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A repo-backed task advanced to In Review on thread-finish even when no PR was opened — dead-ending it In Review with nothing to review (the CMS submit path opens the PR later). Now the thread-finish backstop advances a repo-backed task only once a PR is linked; the agent's PR-open hook still moves it mid-run, and non-repo tasks advance on finish unchanged. - `shouldAdvanceToReview` gains a `hasPr` arg; a `repo`-named item stays In Progress on finish unless a PR is linked. - Both callers (thread-finish hook + stall sweep) compute `hasPr` via `listPrs`, querying only for repo-backed items. Tests: unit for the gate; real-Postgres that a repo task holds In Progress with no PR, then advances once a PR is linked. Co-Authored-By: Claude Opus 4.8 --- ...k-board-advance-review.integration.test.ts | 50 +++++++++++++++++++ apps/api/src/storage/task-board.test.ts | 28 +++++++++++ apps/api/src/storage/task-board.ts | 22 ++++++-- .../src/tools/task-board/stall-recovery.ts | 6 ++- 4 files changed, 100 insertions(+), 6 deletions(-) diff --git a/apps/api/src/storage/task-board-advance-review.integration.test.ts b/apps/api/src/storage/task-board-advance-review.integration.test.ts index 18223ee9aa..3b02bc4826 100644 --- a/apps/api/src/storage/task-board-advance-review.integration.test.ts +++ b/apps/api/src/storage/task-board-advance-review.integration.test.ts @@ -172,6 +172,56 @@ describe("advanceToReviewIfInProgress (real Postgres)", () => { expect(results.filter((r) => r !== null)).toHaveLength(1); }); + + // A repo-backed task can't dead-end In Review with no PR — on finish it stays In Progress until a PR is linked, then the finish backstop advances it. + it("holds a repo-backed task on finish until a PR is linked", async () => { + const task = await taskBoard.create({ + organizationId: ORG, + title: "repo, no PR yet", + status: "in_progress", + repo: "acme/site", + by: USER, + }); + const thread = await threads.create({ + organization_id: ORG, + title: "run", + status: "completed", + message_storage_version: 2, + created_by: USER, + }); + await taskBoard.linkThread(task.id, thread.id, ORG); + await database.db + .insertInto("thread_message_parts") + .values({ + id: `${thread.id}:m:0`, + seq: 0, + org_id: ORG, + thread_id: thread.id, + run_id: thread.id, + message_id: `${thread.id}:m`, + role: "user", + kind: "text", + payload: JSON.stringify({ type: "text", text: "go" }), + created_at: new Date().toISOString(), + }) + .execute(); + + // No PR → stays In Progress. + await taskBoard.advanceLinkedTasksToReviewOnThreadFinish(thread.id, ORG); + expect((await taskBoard.getById(task.id, ORG))?.status).toBe("in_progress"); + + // Link a PR → the finish backstop now advances it. + await taskBoard.linkPr({ + taskBoardItemId: task.id, + organizationId: ORG, + url: "https://github.com/acme/site/pull/3", + prNumber: 3, + repoOwner: "acme", + repoName: "site", + }); + await taskBoard.advanceLinkedTasksToReviewOnThreadFinish(thread.id, ORG); + expect((await taskBoard.getById(task.id, ORG))?.status).toBe("in_review"); + }); }); /** diff --git a/apps/api/src/storage/task-board.test.ts b/apps/api/src/storage/task-board.test.ts index a86434b4a3..cd287bba79 100644 --- a/apps/api/src/storage/task-board.test.ts +++ b/apps/api/src/storage/task-board.test.ts @@ -23,6 +23,34 @@ describe("shouldAdvanceToReview", () => { ).toBe(true); }); + // A repo-backed task that finished with no PR has nothing to review yet. + it("does NOT advance a repo-backed task on finish with no PR", () => { + expect( + shouldAdvanceToReview( + { + status: "in_progress", + repo: "acme/site", + threads: [thread("completed")], + }, + false, + ), + ).toBe(false); + }); + + // Backstop for missed PR-open detection: a PR exists, so finishing means there is one to review. + it("advances a repo-backed task on finish once a PR is linked", () => { + expect( + shouldAdvanceToReview( + { + status: "in_progress", + repo: "acme/site", + threads: [thread("completed")], + }, + true, + ), + ).toBe(true); + }); + // Inverted deliberately: this used to assert that a failed run advances the // card. It is how eight tasks whose sandboxes never came up landed In Review // with no PR and no work done. In Review means there is something to review. diff --git a/apps/api/src/storage/task-board.ts b/apps/api/src/storage/task-board.ts index e4a5f62918..26d0f2183a 100644 --- a/apps/api/src/storage/task-board.ts +++ b/apps/api/src/storage/task-board.ts @@ -100,13 +100,20 @@ export const TERMINAL_THREAD_STATUSES = new Set([ * one would advance a card whose work nobody ever started; a card whose only * thread is empty has effectively no thread at all. */ -export function shouldAdvanceToReview(item: { - status: TaskBoardItemStatus; - threads: { status: string | null; hasMessages: boolean }[]; -}): boolean { +export function shouldAdvanceToReview( + item: { + status: TaskBoardItemStatus; + repo?: string | null; + threads: { status: string | null; hasMessages: boolean }[]; + }, + /** Whether a PR is linked — a repo-backed task needs one to advance on finish. */ + hasPr = false, +): boolean { if (item.status !== "in_progress") return false; const used = item.threads.filter((t) => t.hasMessages); if (used.length === 0) return false; + // A repo-backed task reaches In Review only once a PR exists (the agent's PR-open hook moves it mid-run); on thread-finish we require a linked PR so a finished edit with no PR doesn't dead-end In Review. Non-repo tasks advance on finish. + if (item.repo != null && !hasPr) return false; if ( !used.every( (t) => t.status !== null && TERMINAL_THREAD_STATUSES.has(t.status), @@ -1005,7 +1012,12 @@ export class TaskBoardStorage { const moved: TaskBoardItem[] = []; for (const taskId of await this.linkedTaskIds(threadId, organizationId)) { const item = await this.getById(taskId, organizationId); - if (!item || !shouldAdvanceToReview(item)) continue; + if (!item) continue; + // Only query PRs for a repo-backed task — that's the one gate that needs it. + const hasPr = + item.repo != null && + (await this.listPrs(taskId, organizationId)).length > 0; + if (!shouldAdvanceToReview(item, hasPr)) continue; // The status flip is a CONDITIONAL update guarded on the status we just // read, so exactly one concurrent caller can win it. // diff --git a/apps/api/src/tools/task-board/stall-recovery.ts b/apps/api/src/tools/task-board/stall-recovery.ts index 72f89dadd6..bf7591cd92 100644 --- a/apps/api/src/tools/task-board/stall-recovery.ts +++ b/apps/api/src/tools/task-board/stall-recovery.ts @@ -281,7 +281,11 @@ export async function recoverStalledTasks( // costs zero queries. Threads are newest-first (`attachThreads` orders by // `link.created_at desc`), so the newest *used* one is the last run to have // happened — an empty thread linked afterwards must not shadow it. - if (!shouldAdvanceToReview(item)) continue; + // Only query PRs for a repo-backed task — that's the one gate that needs it. + const hasPr = + item.repo != null && + (await ctx.storage.taskBoard.listPrs(item.id, organizationId)).length > 0; + if (!shouldAdvanceToReview(item, hasPr)) continue; const thread = item.threads.find((t) => t.hasMessages); if (!thread) continue; try {