diff --git a/apps/api/src/api/app.ts b/apps/api/src/api/app.ts index 36395587ec..2c3c07ea5e 100644 --- a/apps/api/src/api/app.ts +++ b/apps/api/src/api/app.ts @@ -171,6 +171,7 @@ import { emitTerminalThreadStatus } from "./routes/decopilot/thread-status-event import { SqlThreadStorage } from "../storage/threads"; import { OrganizationBillingStorage } from "../storage/organization-billing"; import { TaskBoardStorage } from "../storage/task-board"; +import { boardLanesForDb } from "../tools/task-board/board-handler"; import { advanceTasksToReviewOnThreadFinish } from "../tools/task-board/run-reactions"; import { SqlAsyncResearchJobStorage } from "../storage/async-research-jobs"; import { AsyncResearchJobSweeper } from "../storage/async-research-jobs-sweeper"; @@ -1131,12 +1132,13 @@ export async function createApp(options: CreateAppOptions = {}) { // reaches the projector, so this reactor is its only terminal writer — and // owes the board the pass the projector's own terminals already run. Same // storages, built here because this wiring precedes theirs. - onThreadFinished: (threadId, orgId) => + onThreadFinished: async (threadId, orgId) => advanceTasksToReviewOnThreadFinish( new TaskBoardStorage(database.db), threadId, orgId, new OrganizationBillingStorage(database.db), + await boardLanesForDb(database.db, orgId), ), }; @@ -1626,6 +1628,7 @@ export async function createApp(options: CreateAppOptions = {}) { projectorTaskBoard, automationContextFactory, projectorBilling, + database.db, ); if (getSettings().taskBoardReviewSweeperEnabled) { taskBoardReviewSweeper.start(); @@ -1684,6 +1687,7 @@ export async function createApp(options: CreateAppOptions = {}) { runId, orgId, projectorBilling, + await boardLanesForDb(database.db, orgId), ); // The headless reviewer trigger used to be called here and could never // work: this callback runs inside a DBOS step, and the dispatch bottoms @@ -1718,6 +1722,7 @@ export async function createApp(options: CreateAppOptions = {}) { runId, orgId, projectorBilling, + await boardLanesForDb(database.db, orgId), ); // No reviewer trigger here either — see completeRunIfNotCompleted above // for why it cannot live in a step. `TaskBoardReviewSweeper` owns it. diff --git a/apps/api/src/jira/sync.ts b/apps/api/src/jira/sync.ts index 8e71b93c71..b52235d024 100644 --- a/apps/api/src/jira/sync.ts +++ b/apps/api/src/jira/sync.ts @@ -25,7 +25,12 @@ */ import { orgFlagEnabled } from "@decocms/shared/organization/schema"; -import { boardFor } from "@/tools/task-board/board-handler"; +import { + boardAutomationFor, + boardFor, + boardCan, + boardLanes, +} from "@/tools/task-board/board-handler"; import { SUPER_AGENT_ASSIGNEE_ID } from "@decocms/shared/task-board"; import type { StudioContext } from "@/core/studio-context"; import type { @@ -337,18 +342,23 @@ async function maybeAutoDelegate( // The board decides: a column with no rule on it is uneventful. This is also // what replaced `integration.autoDelegate`, which could only ever mean the // Super Agent, on To Do, for an org that had Jira. - const automation = await (await boardFor(ctx, orgId)).automationFor( - item.status, - ); + const automation = await boardAutomationFor(ctx, orgId, item.status); if (!automation) return item; // Conditional claim, not a plain update: the cron, a webhook wake-up (its // debounce is per-pod) and a manual JIRA_SYNC_RUN can all be mid-sync on the // same issue, and a read-then-write would dispatch two paid agent runs on it. + const queue = (await boardLanes(ctx, orgId)).queue; + if ( + !boardCan(orgId, "todo", queue, "auto-delegating Jira issues to the agent") + ) { + return item; + } const delegated = await ctx.storage.taskBoard.claimUnassignedForSuperAgent( item.id, orgId, integration.createdBy, JIRA_SYNC_ACTOR, + queue, ); if (!delegated) return item; await ctx.storage.taskBoard.recordActivity({ 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 fc5ca587da..f9f0ba9b38 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 @@ -30,6 +30,15 @@ import { TaskBoardStorage } from "./task-board"; import { SqlThreadStorage } from "./threads"; import { SUPER_AGENT_ASSIGNEE_ID } from "@decocms/shared/task-board"; +/** Studio's own board, which is what these fixtures run on. */ +const CANON_LANES = { + intake: "triage", + queue: "todo", + progress: "in_progress", + review: "in_review", + archive: "archived", +}; + const ORG = "org_advance_review"; const USER = "user_advance_review"; @@ -152,7 +161,11 @@ describe("advanceToReviewIfInProgress (real Postgres)", () => { await Promise.all( Array.from({ length: 10 }, () => - taskBoard.advanceLinkedTasksToReviewOnThreadFinish(thread.id, ORG), + taskBoard.advanceLinkedTasksToReviewOnThreadFinish( + thread.id, + ORG, + CANON_LANES, + ), ), ); @@ -211,7 +224,11 @@ describe("advanceToReviewIfInProgress (real Postgres)", () => { .execute(); // No PR → stays In Progress with nothing to review. - await taskBoard.advanceLinkedTasksToReviewOnThreadFinish(thread.id, ORG); + await taskBoard.advanceLinkedTasksToReviewOnThreadFinish( + thread.id, + ORG, + CANON_LANES, + ); const before = await taskBoard.getById(task.id, ORG); expect(before?.status).toBe("in_progress"); expect(before?.reviewCycleStartedAt).toBeNull(); @@ -225,7 +242,11 @@ describe("advanceToReviewIfInProgress (real Postgres)", () => { repoOwner: "acme", repoName: "site", }); - await taskBoard.advanceLinkedTasksToReviewOnThreadFinish(thread.id, ORG); + await taskBoard.advanceLinkedTasksToReviewOnThreadFinish( + thread.id, + ORG, + CANON_LANES, + ); const after = await taskBoard.getById(task.id, ORG); expect(after?.status).toBe("in_progress"); expect(after?.reviewCycleStartedAt).not.toBeNull(); @@ -244,7 +265,7 @@ describe("advanceToReviewIfInProgress (real Postgres)", () => { const winners = await Promise.all( Array.from({ length: 8 }, () => - taskBoard.openReviewCycleIfInProgress(task.id, ORG), + taskBoard.openReviewCycleIfInProgress(task.id, ORG, CANON_LANES), ), ); @@ -253,20 +274,28 @@ describe("advanceToReviewIfInProgress (real Postgres)", () => { it("re-opens a cycle only after it is closed", async () => { const { task } = await cardWithFinishedRun("second round"); - const first = await taskBoard.openReviewCycleIfInProgress(task.id, ORG); + const first = await taskBoard.openReviewCycleIfInProgress( + task.id, + ORG, + CANON_LANES, + ); expect(first).not.toBeNull(); const firstAt = first?.reviewCycleStartedAt; // Still open — the boundary must not move under a standing verdict. expect( - await taskBoard.openReviewCycleIfInProgress(task.id, ORG), + await taskBoard.openReviewCycleIfInProgress(task.id, ORG, CANON_LANES), ).toBeNull(); expect((await taskBoard.getById(task.id, ORG))?.reviewCycleStartedAt).toBe( firstAt as string, ); await taskBoard.closeReviewCycle(task.id, ORG); - const second = await taskBoard.openReviewCycleIfInProgress(task.id, ORG); + const second = await taskBoard.openReviewCycleIfInProgress( + task.id, + ORG, + CANON_LANES, + ); expect(second).not.toBeNull(); expect(second?.reviewCycleStartedAt).not.toBe(firstAt as string); }); @@ -300,7 +329,11 @@ describe("advanceToReviewIfInProgress (real Postgres)", () => { repoName: "site", }); - await taskBoard.advanceLinkedTasksToReviewOnThreadFinish(thread.id, ORG); + await taskBoard.advanceLinkedTasksToReviewOnThreadFinish( + thread.id, + ORG, + CANON_LANES, + ); const after = await taskBoard.getById(task.id, ORG); expect(after?.status).toBe("in_progress"); @@ -311,9 +344,13 @@ describe("advanceToReviewIfInProgress (real Postgres)", () => { // the PR read says — that is the move this whole change exists to prevent. it("leaves a card with an open cycle where it is", async () => { const { task, thread } = await cardWithFinishedRun("already reviewing"); - await taskBoard.openReviewCycleIfInProgress(task.id, ORG); + await taskBoard.openReviewCycleIfInProgress(task.id, ORG, CANON_LANES); - await taskBoard.advanceLinkedTasksToReviewOnThreadFinish(thread.id, ORG); + await taskBoard.advanceLinkedTasksToReviewOnThreadFinish( + thread.id, + ORG, + CANON_LANES, + ); expect((await taskBoard.getById(task.id, ORG))?.status).toBe("in_progress"); }); @@ -329,7 +366,11 @@ describe("advanceToReviewIfInProgress (real Postgres)", () => { await taskBoard.advanceToReviewIfInProgress(task.id, ORG, USER); expect((await taskBoard.getById(task.id, ORG))?.status).toBe("in_review"); - const opened = await taskBoard.openReviewCycleIfInProgress(task.id, ORG); + const opened = await taskBoard.openReviewCycleIfInProgress( + task.id, + ORG, + CANON_LANES, + ); expect(opened?.status).toBe("in_progress"); expect(opened?.reviewCycleStartedAt).not.toBeNull(); @@ -340,11 +381,11 @@ describe("advanceToReviewIfInProgress (real Postgres)", () => { // would undo that and re-stamp a boundary verdicts already stand on. it("leaves an In Review card with a cycle already open alone", async () => { const { task } = await cardWithFinishedRun("mid review"); - await taskBoard.openReviewCycleIfInProgress(task.id, ORG); + await taskBoard.openReviewCycleIfInProgress(task.id, ORG, CANON_LANES); await taskBoard.update(task.id, ORG, { status: "in_review" }, USER); expect( - await taskBoard.openReviewCycleIfInProgress(task.id, ORG), + await taskBoard.openReviewCycleIfInProgress(task.id, ORG, CANON_LANES), ).toBeNull(); expect((await taskBoard.getById(task.id, ORG))?.status).toBe("in_review"); }); @@ -363,7 +404,7 @@ describe("advanceToReviewIfInProgress (real Postgres)", () => { await taskBoard.unassignSuperAgent(task.id, ORG, USER); expect( - await taskBoard.openReviewCycleIfInProgress(task.id, ORG), + await taskBoard.openReviewCycleIfInProgress(task.id, ORG, CANON_LANES), ).toBeNull(); expect((await taskBoard.getById(task.id, ORG))?.status).toBe("in_review"); }); @@ -373,7 +414,7 @@ describe("advanceToReviewIfInProgress (real Postgres)", () => { await taskBoard.update(task.id, ORG, { status: "done" }, USER); expect( - await taskBoard.openReviewCycleIfInProgress(task.id, ORG), + await taskBoard.openReviewCycleIfInProgress(task.id, ORG, CANON_LANES), ).toBeNull(); }); @@ -381,7 +422,11 @@ describe("advanceToReviewIfInProgress (real Postgres)", () => { const { task } = await cardWithFinishedRun("cross-org cycle"); expect( - await taskBoard.openReviewCycleIfInProgress(task.id, "org_other"), + await taskBoard.openReviewCycleIfInProgress( + task.id, + "org_other", + CANON_LANES, + ), ).toBeNull(); expect( (await taskBoard.getById(task.id, ORG))?.reviewCycleStartedAt, @@ -392,7 +437,7 @@ describe("advanceToReviewIfInProgress (real Postgres)", () => { // reviewer is working reads In Progress and still has to be swept. it("lists an In Progress card with an open cycle as pending review", async () => { const { task } = await cardWithFinishedRun("pending while in progress"); - await taskBoard.openReviewCycleIfInProgress(task.id, ORG); + await taskBoard.openReviewCycleIfInProgress(task.id, ORG, CANON_LANES); const pending = await taskBoard.listItemsPendingReview(100); @@ -401,7 +446,7 @@ describe("advanceToReviewIfInProgress (real Postgres)", () => { it("drops a card out of the work list once it ships", async () => { const { task } = await cardWithFinishedRun("shipped, stop sweeping"); - await taskBoard.openReviewCycleIfInProgress(task.id, ORG); + await taskBoard.openReviewCycleIfInProgress(task.id, ORG, CANON_LANES); await taskBoard.update(task.id, ORG, { status: "done" }, USER); expect( @@ -497,7 +542,11 @@ describe("failed runs never reach In Review (real Postgres)", () => { it("leaves a card whose only run failed In Progress", async () => { const { task, thread } = await cardWithRun("failed run", "failed"); - await taskBoard.advanceLinkedTasksToReviewOnThreadFinish(thread.id, ORG2); + await taskBoard.advanceLinkedTasksToReviewOnThreadFinish( + thread.id, + ORG2, + CANON_LANES, + ); expect((await taskBoard.getById(task.id, ORG2))?.status).toBe( "in_progress", @@ -586,7 +635,15 @@ describe("failed runs never reach In Review (real Postgres)", () => { const { task } = await cardWithRun("retry me", "failed"); const due = new Date(Date.now() - 1000); - expect(await taskBoard.scheduleRunRetry(task.id, ORG2, 1, due)).toBe(true); + expect( + await taskBoard.scheduleRunRetry( + task.id, + ORG2, + 1, + due, + CANON_LANES.progress, + ), + ).toBe(true); expect((await taskBoard.getById(task.id, ORG2))?.retryAttempts).toBe(1); expect( (await taskBoard.listItemsDueForRetry(10, new Date())).map((r) => r.id), @@ -643,6 +700,7 @@ describe("failed runs never reach In Review (real Postgres)", () => { ORG2, 1, new Date(Date.now() + 60_000), + CANON_LANES.progress, ); const stuck = await taskBoard.listItemsStuckAfterFailure(10, new Date()); @@ -754,19 +812,31 @@ describe("failed runs never reach In Review (real Postgres)", () => { it("sends an exhausted card back to To Do and clears its retry state", async () => { const { task } = await cardWithRun("out of retries", "failed"); - await taskBoard.scheduleRunRetry(task.id, ORG2, 3, new Date()); + await taskBoard.scheduleRunRetry( + task.id, + ORG2, + 3, + new Date(), + CANON_LANES.progress, + ); const returned = await taskBoard.returnToTodoAfterFailure( task.id, ORG2, USER2, + CANON_LANES, ); expect(returned?.status).toBe("todo"); expect(returned?.retryAttempts).toBe(0); // A card that already left In Progress is not dragged backwards. expect( - await taskBoard.returnToTodoAfterFailure(task.id, ORG2, USER2), + await taskBoard.returnToTodoAfterFailure( + task.id, + ORG2, + USER2, + CANON_LANES, + ), ).toBeNull(); }); }); diff --git a/apps/api/src/storage/task-board-columns.ts b/apps/api/src/storage/task-board-columns.ts index 10908a64a2..eccb04287e 100644 --- a/apps/api/src/storage/task-board-columns.ts +++ b/apps/api/src/storage/task-board-columns.ts @@ -112,7 +112,11 @@ export class BoardColumnStorage { title: column.title, position, role: roleOf.get(column.key) ?? null, - tracker_statuses: column.trackerStatuses, + // Stringified, not handed over as an array: `pg` serialises a JS array + // as a Postgres ARRAY literal (`{a,b}`), which jsonb rejects outright + // — and an EMPTY one as `{}`, which it accepts as an empty OBJECT. So + // the silent case is the dangerous one. + tracker_statuses: JSON.stringify(column.trackerStatuses), })); await tx .insertInto("task_board_columns") @@ -126,7 +130,13 @@ export class BoardColumnStorage { })), ) .execute(); - return rows.map(toEntity); + return all.map((column, position) => ({ + key: column.key, + title: column.title, + position, + role: roleOf.get(column.key) ?? null, + trackerStatuses: column.trackerStatuses, + })); }); } diff --git a/apps/api/src/storage/task-board-due-retry.integration.test.ts b/apps/api/src/storage/task-board-due-retry.integration.test.ts index 368b0b78a8..862ca3be3a 100644 --- a/apps/api/src/storage/task-board-due-retry.integration.test.ts +++ b/apps/api/src/storage/task-board-due-retry.integration.test.ts @@ -20,6 +20,15 @@ import { } from "../database/test-db-pg"; import { TaskBoardStorage } from "./task-board"; +/** Studio's own board, which is what these fixtures run on. */ +const CANON_LANES = { + intake: "triage", + queue: "todo", + progress: "in_progress", + review: "in_review", + archive: "archived", +}; + const ORG = "org_due_retry"; const USER = "user_due_retry"; @@ -39,6 +48,7 @@ describe("listItemsDueForRetry (real Postgres)", () => { ORG, 1, new Date(Date.now() - 1000), + CANON_LANES.progress, ); expect(claimed).toBe(true); return task; diff --git a/apps/api/src/storage/task-board-retry-dismissed.integration.test.ts b/apps/api/src/storage/task-board-retry-dismissed.integration.test.ts index 25b365fabf..03e26abbbb 100644 --- a/apps/api/src/storage/task-board-retry-dismissed.integration.test.ts +++ b/apps/api/src/storage/task-board-retry-dismissed.integration.test.ts @@ -21,6 +21,15 @@ import { } from "../database/test-db-pg"; import { TaskBoardStorage } from "./task-board"; +/** Studio's own board, which is what these fixtures run on. */ +const CANON_LANES = { + intake: "triage", + queue: "todo", + progress: "in_progress", + review: "in_review", + archive: "archived", +}; + const ORG = "org_retry_dismissed"; const USER = "user_retry_dismissed"; @@ -65,6 +74,7 @@ describe("scheduleRunRetry / returnToTodoAfterFailure (real Postgres)", () => { ORG, 1, new Date(Date.now() + 1000), + CANON_LANES.progress, ); expect(scheduled).toBe(true); @@ -86,6 +96,7 @@ describe("scheduleRunRetry / returnToTodoAfterFailure (real Postgres)", () => { ORG, 1, new Date(Date.now() + 1000), + CANON_LANES.progress, ); expect(scheduled).toBe(false); @@ -103,6 +114,7 @@ describe("scheduleRunRetry / returnToTodoAfterFailure (real Postgres)", () => { task.id, ORG, USER, + CANON_LANES, ); expect(returned?.status).toBe("todo"); @@ -122,6 +134,7 @@ describe("scheduleRunRetry / returnToTodoAfterFailure (real Postgres)", () => { task.id, ORG, USER, + CANON_LANES, ); expect(returned).toBeNull(); diff --git a/apps/api/src/storage/task-board.ts b/apps/api/src/storage/task-board.ts index f7fc2f5682..a062e4dfda 100644 --- a/apps/api/src/storage/task-board.ts +++ b/apps/api/src/storage/task-board.ts @@ -697,7 +697,25 @@ export class TaskBoardStorage { .where((eb) => eb.or([ eb("review_cycle_started_at", "is not", null), + // The card's OWN board's review column. A pre-filter across orgs + // cannot be handed one lane — every row may belong to a different + // board — so it reads the same rows the handler reads. An org on + // Studio's board has no rows here at all, which is why the literal + // stays beside it rather than being replaced by it. eb("status", "=", "in_review"), + eb( + "status", + "in", + eb + .selectFrom("task_board_columns as c") + .select("c.key") + .whereRef( + "c.organization_id", + "=", + "task_board_items.organization_id", + ) + .where("c.role", "=", "in_review"), + ), ]), ) .where("dismissed_at", "is", null); @@ -923,13 +941,17 @@ export class TaskBoardStorage { organizationId: string, attempts: number, retryAt: Date, + /** This board's in-progress column. Null means the board has none, so + * there is no card to schedule a retry for. */ + progressLane: string | null, ): Promise { + if (progressLane === null) return false; const rows = await this.db .updateTable("task_board_items") .set({ retry_at: retryAt, retry_attempts: attempts }) .where("id", "=", id) .where("organization_id", "=", organizationId) - .where("status", "=", "in_progress") + .where("status", "=", progressLane) .where("dismissed_at", "is", null) .returning("id") .execute(); @@ -947,11 +969,16 @@ export class TaskBoardStorage { id: string, organizationId: string, updatedBy: string, + /** Where the card is coming from and going to on THIS board. Either being + * null means the board cannot express this move, so it does not happen — + * the run's failure is still on the card's timeline either way. */ + lanes: { progress: string | null; queue: string | null }, ): Promise { + if (lanes.progress === null || lanes.queue === null) return null; const rows = await this.db .updateTable("task_board_items") .set({ - status: "todo", + status: lanes.queue, retry_at: null, retry_attempts: 0, updated_by: updatedBy, @@ -959,7 +986,7 @@ export class TaskBoardStorage { }) .where("id", "=", id) .where("organization_id", "=", organizationId) - .where("status", "=", "in_progress") + .where("status", "=", lanes.progress) .where("dismissed_at", "is", null) .returning("id") .execute(); @@ -1351,6 +1378,7 @@ export class TaskBoardStorage { async advanceLinkedTasksToReviewOnThreadFinish( threadId: string, organizationId: string, + lanes: { progress: string | null; review: string | null }, ): Promise { const moved: TaskBoardItem[] = []; for (const taskId of await this.linkedTaskIds(threadId, organizationId)) { @@ -1394,6 +1422,7 @@ export class TaskBoardStorage { const opened = await this.openReviewCycleIfInProgress( taskId, organizationId, + lanes, ); if (!opened) continue; moved.push(opened); @@ -1441,10 +1470,14 @@ export class TaskBoardStorage { async openReviewCycleIfInProgress( id: string, organizationId: string, + lanes: { progress: string | null; review: string | null }, ): Promise { + // No in-progress column: the review still happens, the card just stays put. + if (lanes.progress === null) return null; + const reviewLane = lanes.review; const row = await this.db .updateTable("task_board_items") - .set({ review_cycle_started_at: new Date(), status: "in_progress" }) + .set({ review_cycle_started_at: new Date(), status: lanes.progress }) .where("id", "=", id) .where("organization_id", "=", organizationId) // In Review is in here because the PR link can LOSE THE RACE to the @@ -1466,11 +1499,15 @@ export class TaskBoardStorage { // statement — a separate flip could interleave with the sweeper's read. .where((eb) => eb.or([ - eb("status", "=", "in_progress"), - eb.and([ - eb("status", "=", "in_review"), - eb("assignee_id", "=", SUPER_AGENT_ASSIGNEE_ID), - ]), + eb("status", "=", lanes.progress), + ...(reviewLane === null + ? [] + : [ + eb.and([ + eb("status", "=", reviewLane), + eb("assignee_id", "=", SUPER_AGENT_ASSIGNEE_ID), + ]), + ]), ]), ) .where("review_cycle_started_at", "is", null) @@ -1594,8 +1631,9 @@ export class TaskBoardStorage { id: string, organizationId: string, by: string, + lanes: { review: string | null; progress: string | null }, ): Promise { - return this.claimInReviewSuperAgentSlot(id, organizationId, by); + return this.claimInReviewSuperAgentSlot(id, organizationId, by, lanes); } /** @@ -1610,11 +1648,16 @@ export class TaskBoardStorage { id: string, organizationId: string, by: string, + /** The fence's two ends on THIS board. Either being null means the board + * cannot express the claim, so nobody wins it — which reads to the caller + * as "another trigger got there first", the same as losing the race. */ + lanes: { review: string | null; progress: string | null }, ): Promise { + if (lanes.review === null || lanes.progress === null) return null; const row = await this.db .updateTable("task_board_items") .set({ - status: "in_progress", + status: lanes.progress, // The claim exists to put the card back in the Super Agent's hands to // fix something, so the review that just ended is over. Cleared in the // SAME statement as the flip: a separate write could lose the race with @@ -1626,7 +1669,7 @@ export class TaskBoardStorage { }) .where("id", "=", id) .where("organization_id", "=", organizationId) - .where("status", "=", "in_review") + .where("status", "=", lanes.review) .where("assignee_id", "=", SUPER_AGENT_ASSIGNEE_ID) .returningAll() .executeTakeFirst(); @@ -1652,7 +1695,12 @@ export class TaskBoardStorage { organizationId: string, assignedBy: string, by: string, + /** This board's queue column — the claim's starting line. Null means the + * board has no column that means "queued", so there is nothing to claim + * and the caller reads it the same as losing the race. */ + queueLane: string | null, ): Promise { + if (queueLane === null) return null; const row = await this.db .updateTable("task_board_items") .set({ @@ -1663,7 +1711,7 @@ export class TaskBoardStorage { }) .where("id", "=", id) .where("organization_id", "=", organizationId) - .where("status", "=", "todo") + .where("status", "=", queueLane) .where("assignee_id", "is", null) .returningAll() .executeTakeFirst(); diff --git a/apps/api/src/storage/types.ts b/apps/api/src/storage/types.ts index 25d683785a..842b8913e2 100644 --- a/apps/api/src/storage/types.ts +++ b/apps/api/src/storage/types.ts @@ -1738,7 +1738,7 @@ export interface TaskBoardColumnTable { /** Tracker statuses this column groups, in the tracker's own order. A Jira * column is a bucket of statuses, not one status, so the push needs the * whole list to pick a reachable transition. Empty for Studio's columns. */ - tracker_statuses: ColumnType; + tracker_statuses: ColumnType; created_at: ColumnType; updated_at: ColumnType; } diff --git a/apps/api/src/tools/task-board/automations.ts b/apps/api/src/tools/task-board/automations.ts index 3059e53b2b..b848de88f6 100644 --- a/apps/api/src/tools/task-board/automations.ts +++ b/apps/api/src/tools/task-board/automations.ts @@ -1,7 +1,7 @@ import { z } from "zod"; import { defineTool } from "@/core/define-tool"; import { requireAuth } from "@/core/studio-context"; -import { boardFor } from "./board-handler"; +import { boardColumnsOf } from "./board-handler"; import { MAX_AUTOMATION_PROMPT_LENGTH } from "./schema"; const AutomationSchema = z.object({ @@ -65,7 +65,7 @@ export const TASK_BOARD_AUTOMATION_UPSERT = defineTool({ // Rejected here rather than stored and ignored: a rule on a column this // board does not have never fires, and looks configured to whoever set it. - const columns = await (await boardFor(ctx, organizationId)).columns(); + const columns = await boardColumnsOf(ctx, organizationId); if (!columns.some((c) => c.key === input.columnKey)) { throw new Error( `This board has no column "${input.columnKey}" — it has ${columns diff --git a/apps/api/src/tools/task-board/board-handler.integration.test.ts b/apps/api/src/tools/task-board/board-handler.integration.test.ts index fca2c0b4dc..db04eb4b7c 100644 --- a/apps/api/src/tools/task-board/board-handler.integration.test.ts +++ b/apps/api/src/tools/task-board/board-handler.integration.test.ts @@ -285,3 +285,118 @@ describe("boardHandler — a board whose columns are the org's own", () => { expect(await board().automationFor("todo")).toBe(null); }); }); + +/** + * The property that makes this seam safe to introduce at all. + * + * Every lane decision used to be a string literal spelled out at the call + * site. Studio's board has to keep answering with exactly those literals, or + * the conversion is not a refactor for the orgs that never opted into + * mirroring — which is nearly all of them. + */ +describe("the canonical board answers what the code used to hardcode", () => { + it("gives back Studio's own lane names, unchanged", async () => { + const board = boardHandler(ORG, { + automations: new ColumnAutomationStorage(database.db), + boardColumns: new BoardColumnStorage(database.db), + orgOwnedColumns: false, + }); + expect(await board.lanes()).toEqual({ + intake: "triage", + queue: "todo", + progress: "in_progress", + review: "in_review", + archive: "archived", + }); + }); +}); + +describe("a mirrored board answers with its own columns", () => { + const board = () => + boardHandler(ORG, { + automations: new ColumnAutomationStorage(database.db), + boardColumns: new BoardColumnStorage(database.db), + orgOwnedColumns: true, + }); + + it("names each lane by the role the org gave a column", async () => { + await new BoardColumnStorage(database.db).replaceAll(ORG, [ + { key: "Backlog", title: "Backlog", trackerStatuses: ["BACKLOG"] }, + { key: "Fazendo", title: "Fazendo", trackerStatuses: ["Fazendo"] }, + { + key: "Code Review", + title: "Code Review", + trackerStatuses: ["Code Review"], + }, + ]); + await new BoardColumnStorage(database.db).setRole( + ORG, + "Fazendo", + "in_progress", + ); + await new BoardColumnStorage(database.db).setRole( + ORG, + "Code Review", + "in_review", + ); + + expect(await board().lanes()).toEqual({ + // The leftmost column, not a role: a card has to be born somewhere, and + // making intake configurable would make "create a card" a setup step. + intake: "Backlog", + queue: null, + progress: "Fazendo", + review: "Code Review", + archive: null, + }); + }); + + /** Null is the honest answer, and the callers all read it as "do nothing" + * rather than writing one of Studio's keys into a column that is not there. */ + it("answers null for a meaning nobody assigned", async () => { + await new BoardColumnStorage(database.db).replaceAll(ORG, [ + { key: "Backlog", title: "Backlog", trackerStatuses: ["BACKLOG"] }, + ]); + const lanes = await board().lanes(); + expect(lanes.intake).toBe("Backlog"); + expect([lanes.queue, lanes.progress, lanes.review, lanes.archive]).toEqual([ + null, + null, + null, + null, + ]); + }); + + /** + * `tracker_statuses` is jsonb, and `pg` serialises a JS array as a Postgres + * ARRAY literal. A populated one is rejected outright; an EMPTY one is + * accepted as `{}` — an empty OBJECT — so the failure that shipped was the + * silent half. Asserting the shape read back is what catches both. + */ + it("round-trips a column's tracker statuses as an array, not an object", async () => { + await new BoardColumnStorage(database.db).replaceAll(ORG, [ + { key: "Em Progresso", title: "Em Progresso", trackerStatuses: [] }, + { + key: "Code Review", + title: "Code Review", + trackerStatuses: ["Code Review", "Revisao"], + }, + ]); + const columns = await new BoardColumnStorage(database.db).listByOrg(ORG); + expect(columns.map((c) => c.trackerStatuses)).toEqual([ + [], + ["Code Review", "Revisao"], + ]); + expect(Array.isArray(columns[0]?.trackerStatuses)).toBe(true); + }); + + /** A board nothing has been mirrored onto cannot say where a card is born, + * and inventing an answer would strand whatever is created next. */ + it("refuses to invent an intake column for an empty board", async () => { + await database.db + .deleteFrom("task_board_columns") + .where("organization_id", "=", ORG) + .execute(); + await expect(board().lanes()).rejects.toThrow(/no columns yet/); + }); +}); diff --git a/apps/api/src/tools/task-board/board-handler.test.ts b/apps/api/src/tools/task-board/board-handler.test.ts index a0c28bd51b..43f6a59af8 100644 --- a/apps/api/src/tools/task-board/board-handler.test.ts +++ b/apps/api/src/tools/task-board/board-handler.test.ts @@ -3,9 +3,9 @@ * write — pure, so unit-tested directly rather than through a fake ctx. */ -import { describe, expect, it } from "bun:test"; +import { afterEach, beforeEach, describe, expect, it } from "bun:test"; import type { BoardHandler } from "./board-handler"; -import { shippedPatch } from "./board-handler"; +import { boardCan, shippedPatch } from "./board-handler"; const boardWithOwner = (columnOwner: string | null): BoardHandler => ({ columnOwner: () => columnOwner }) as BoardHandler; @@ -26,3 +26,49 @@ describe("shippedPatch", () => { }); }); }); + +describe("boardCan", () => { + const warns: string[] = []; + const original = console.warn; + beforeEach(() => { + warns.length = 0; + console.warn = (msg: string) => warns.push(msg); + }); + afterEach(() => { + console.warn = original; + }); + + it("passes a lane through and says nothing", () => { + expect(boardCan("org-quiet", "in_review", "Code Review", "reviewing")).toBe( + true, + ); + expect(warns).toEqual([]); + }); + + it("names the meaning and what will not happen", () => { + expect( + boardCan("org-a", "in_review", null, "automatic conflict resolution"), + ).toBe(false); + expect(warns).toHaveLength(1); + expect(warns[0]).toContain("in_review"); + expect(warns[0]).toContain("automatic conflict resolution"); + }); + + /** + * The reason this is not a bare `console.warn`. These sit on sweep and sync + * paths that fire every few seconds, so an unconfigured board would bury the + * log it is trying to write. + */ + it("warns once per org and meaning, not once per call", () => { + for (let i = 0; i < 5; i++) boardCan("org-b", "todo", null, "delegating"); + expect(warns).toHaveLength(1); + + // A different meaning on the same board is a different thing to fix. + boardCan("org-b", "in_progress", null, "moving the card"); + expect(warns).toHaveLength(2); + + // And another org's board is another team's problem to hear about. + boardCan("org-c", "todo", null, "delegating"); + expect(warns).toHaveLength(3); + }); +}); diff --git a/apps/api/src/tools/task-board/board-handler.ts b/apps/api/src/tools/task-board/board-handler.ts index b51cb391dd..9001965b07 100644 --- a/apps/api/src/tools/task-board/board-handler.ts +++ b/apps/api/src/tools/task-board/board-handler.ts @@ -1,10 +1,13 @@ import type { BoardColumn } from "@decocms/shared/task-board"; import { CANONICAL_COLUMN_KEYS } from "@decocms/shared/task-board"; -import type { - ColumnAutomation, +import type { Kysely } from "kysely"; +import { + type ColumnAutomation, ColumnAutomationStorage, } from "@/storage/task-board-column-automations"; -import type { BoardColumnStorage } from "@/storage/task-board-columns"; +import { BoardColumnStorage } from "@/storage/task-board-columns"; +import { OrganizationSettingsStorage } from "@/storage/organization-settings"; +import type { Database } from "@/storage/types"; import type { StudioContext } from "@/core/studio-context"; import { orgFlagEnabled } from "@decocms/shared/organization/schema"; @@ -22,6 +25,30 @@ import { orgFlagEnabled } from "@decocms/shared/organization/schema"; * nothing is — which is most columns, and must stay uneventful rather than * fall back to a guess. */ +/** + * The columns a board gives Studio's lifecycle meanings. + * + * Every field but `intake` is nullable, and null means the same thing + * throughout: this board has no column that means this. The honest response is + * then to do nothing — not to write one of Studio's keys, which on a mirrored + * board files the card under a column that does not exist and makes it vanish. + * + * `intake` cannot be null because a card has to be created somewhere, and + * refusing to create it is worse than any column. + */ +export interface BoardLanes { + /** Where a card is born. */ + intake: string; + /** Queued for the agent to pick up — the claim's starting line. */ + queue: string | null; + /** Being worked on. */ + progress: string | null; + /** Waiting on review. */ + review: string | null; + /** Retired. */ + archive: string | null; +} + export interface BoardHandler { /** The columns to render, left to right. */ columns(): Promise; @@ -43,6 +70,16 @@ export interface BoardHandler { */ archiveColumn(): Promise; + /** + * Every column Studio's own lifecycle needs a name for, resolved together. + * + * Together rather than one method each, for two reasons. It is one read + * instead of five. And the storage layer needs these as VALUES — its fences + * and sweeps are SQL predicates, so "which column means in-progress" has to + * be answered before the query is built, not asked from inside it. + */ + lanes(): Promise; + /** * What to write into a card's `board_column_org` — the org id when this * board's columns are rows the foreign key can hold it to, null when they @@ -54,6 +91,22 @@ export interface BoardHandler { columnOwner(): string | null; } +/** + * Studio's own board, as lanes. + * + * These are exactly the string literals that used to be hardcoded at every + * writer and every SQL predicate. Keeping them identical is what makes this + * seam a no-op for every org on the canonical board — which is every org but + * the ones that opted into mirroring. + */ +const STUDIO_LANES: BoardLanes = { + intake: "triage", + queue: "todo", + progress: "in_progress", + review: "in_review", + archive: "archived", +}; + /** `title` is the key: the canonical columns are translated by the client, * which is the only place that knows the reader's language. A mirrored column * carries the name its tracker gave it, which is not ours to translate. */ @@ -85,7 +138,11 @@ class StudioBoardHandler implements BoardHandler { } archiveColumn(): Promise { - return Promise.resolve("archived"); + return Promise.resolve(STUDIO_LANES.archive); + } + + lanes(): Promise { + return Promise.resolve(STUDIO_LANES); } columnOwner(): string | null { @@ -124,8 +181,36 @@ class OrgBoardHandler implements BoardHandler { * column mirrored from a tracker means nothing to us until someone says it * does. */ async archiveColumn(): Promise { + return (await this.lanes()).archive; + } + + /** + * Whichever columns the org gave these meanings to, and none by default: a + * column mirrored from a tracker means nothing to us until someone says it + * does. + * + * `intake` is the leftmost column rather than a role. Intake is the one + * decision with no acceptable null, and making it a role would turn "create + * a card" into a setup step; the left edge of a board is where new work + * appears in every tracker that has one. + */ + async lanes(): Promise { const columns = await this.boardColumns.listByOrg(this.organizationId); - return columns.find((column) => column.role === "archived")?.key ?? null; + const withRole = (role: string) => + columns.find((column) => column.role === role)?.key ?? null; + const first = columns[0]; + if (!first) { + throw new Error( + "This board has no columns yet — nothing has been mirrored from the tracker", + ); + } + return { + intake: first.key, + queue: withRole("todo"), + progress: withRole("in_progress"), + review: withRole("in_review"), + archive: withRole("archived"), + }; } columnOwner(): string | null { @@ -176,6 +261,104 @@ export async function boardFor( }); } +/** One warning per org and meaning. A board nobody configured would otherwise + * log on every sweep tick, which is the fastest way to make the signal + * worthless. Capped rather than TTL'd: the key set is bounded by orgs times + * meanings, and a full reset just re-warns once. */ +const warnedLanes = new Set(); +const WARNED_LANES_CAP = 10_000; + +/** + * Whether this board can express `meaning`, warning once when it cannot. + * + * A fence that declines for want of a column looks exactly like a fence that + * lost a race — both just return null — so an unconfigured board does nothing + * and says nothing. This is the line that tells the difference, and it narrows + * `lane` to a string for the caller that proceeds. + */ +export function boardCan( + organizationId: string, + meaning: string, + lane: string | null, + /** What will not happen, in words a person can act on. */ + what: string, +): lane is string { + if (lane !== null) return true; + const key = `${organizationId}:${meaning}`; + if (!warnedLanes.has(key)) { + if (warnedLanes.size >= WARNED_LANES_CAP) warnedLanes.clear(); + warnedLanes.add(key); + console.warn( + `[task-board] no column on this board means "${meaning}", so ${what} ` + + `will not happen — set the role on a column in the board's settings`, + ); + } + return false; +} + +/** + * This org's lanes, in one await. + * + * `(await boardLanes(ctx, org)).review` is what asking the long + * way looks like, and almost every caller wants exactly this. Naming the + * question is cheaper than reading the nesting. + */ +export async function boardLanes( + ctx: StudioContext, + organizationId: string, +): Promise { + return await (await boardFor(ctx, organizationId)).lanes(); +} + +/** What this org's board runs when a card lands in `columnKey`, in one await. + * See {@link boardLanes}. */ +export async function boardAutomationFor( + ctx: StudioContext, + organizationId: string, + columnKey: string, +): Promise { + return await (await boardFor(ctx, organizationId)).automationFor(columnKey); +} + +/** This org's columns, in one await. See {@link boardLanes}. */ +export async function boardColumnsOf( + ctx: StudioContext, + organizationId: string, +): Promise { + return await (await boardFor(ctx, organizationId)).columns(); +} + +/** + * The same board, for the callers that have no `StudioContext`. + * + * The projector wiring, the sweeper and the thread-finish reactions run with a + * database handle and nothing else. Without this they would each have to be + * TOLD which columns mean what, by whoever called them — and a caller that + * forgot would silently reintroduce Studio's vocabulary on someone else's + * board. Building the same handler from the same rows keeps one answer. + */ +async function boardForDb( + db: Kysely, + organizationId: string, +): Promise { + const settings = await new OrganizationSettingsStorage(db).get( + organizationId, + ); + return boardHandler(organizationId, { + automations: new ColumnAutomationStorage(db), + boardColumns: new BoardColumnStorage(db), + orgOwnedColumns: orgFlagEnabled(settings?.flags, "org_board_columns"), + }); +} + +/** This org's lanes from a database handle. See {@link boardLanes}. */ +export async function boardLanesForDb( + db: Kysely, + organizationId: string, +): Promise { + return await (await boardForDb(db, organizationId)).lanes(); +} + /** * The patch every route that ships or archives a card writes: the target * status alongside the board's own discriminator. One helper so a new ship diff --git a/apps/api/src/tools/task-board/column-roles.ts b/apps/api/src/tools/task-board/column-roles.ts index 7346bd4a02..f16efc2fa8 100644 --- a/apps/api/src/tools/task-board/column-roles.ts +++ b/apps/api/src/tools/task-board/column-roles.ts @@ -1,18 +1,18 @@ import { z } from "zod"; import { defineTool } from "@/core/define-tool"; import { requireAuth } from "@/core/studio-context"; -import { boardFor } from "./board-handler"; +import { boardColumnsOf } from "./board-handler"; /** * What a column means to Studio's automation. * * A column mirrored from a tracker is a name and a position — nothing tells us - * that "Code Review" is where review happens or that "Arquivado" retires a - * card. These are the meanings Studio acts on, and a column carries at most + * that "Fazendo" is where work happens, "Code Review" is where review happens, + * or that "Arquivado" retires a card. These are the meanings Studio acts on, and a column carries at most * one; every other column simply means nothing, which is the safe default for * a column we did not invent. */ -const COLUMN_ROLES = ["in_review", "archived"] as const; +const COLUMN_ROLES = ["todo", "in_progress", "in_review", "archived"] as const; const roleSchema = z.enum(COLUMN_ROLES); @@ -45,7 +45,7 @@ export const TASK_BOARD_COLUMN_ROLE_SET = defineTool({ // Rejected rather than stored: a role on a column this board does not have // never fires, and reads as configured to whoever set it. - const columns = await (await boardFor(ctx, organizationId)).columns(); + const columns = await boardColumnsOf(ctx, organizationId); if (!columns.some((column) => column.key === input.columnKey)) { throw new Error( `This board has no column "${input.columnKey}" — it has ${ diff --git a/apps/api/src/tools/task-board/conflict-reaction.ts b/apps/api/src/tools/task-board/conflict-reaction.ts index b141bce38e..f7c2a380c7 100644 --- a/apps/api/src/tools/task-board/conflict-reaction.ts +++ b/apps/api/src/tools/task-board/conflict-reaction.ts @@ -8,6 +8,7 @@ import { } from "@decocms/shared/task-board"; import { autoResolveConflictsEnabled } from "@decocms/shared/organization/schema"; import { recordTaskActivity } from "./activity"; +import { boardCan, boardLanes } from "./board-handler"; import { emitTaskBoardUpdated, parkOnRunsExhausted } from "./run-reactions"; import { enqueueSuperAgentForTask } from "./enqueue-super-agent"; @@ -110,10 +111,31 @@ export async function reactToApprovedPrConflict( // for the single winner only, so the activity log (which feeds the cap count) // stays accurate. No `status_changed` entry — mirrors the request_changes // bounce; the review cycle resets only when the run advances back to In Review. + const lanes = await boardLanes(ctx, orgId); + // The fence moves the card between these two, and the failure path moves it + // back. Narrowing here is what lets the revert below write a string rather + // than an "undefined means leave it alone" that would strand the card. + if ( + !boardCan( + orgId, + "in_review", + lanes.review, + "automatic conflict resolution", + ) || + !boardCan( + orgId, + "in_progress", + lanes.progress, + "automatic conflict resolution", + ) + ) { + return false; + } const claimed = await ctx.storage.taskBoard.claimConflictResolution( item.id, orgId, item.updatedBy, + lanes, ); if (!claimed) return false; emitTaskBoardUpdated(orgId, claimed); @@ -132,7 +154,7 @@ export async function reactToApprovedPrConflict( // strands the task forever: the guard above only fires on `in_review`, // so no future poll or approval retries it. Bounce back so it does. await ctx.storage.taskBoard - .update(claimed.id, orgId, { status: "in_review" }, "system") + .update(claimed.id, orgId, { status: lanes.review }, "system") .then((reverted) => emitTaskBoardUpdated(orgId, reverted)) .catch((revertErr) => console.error( diff --git a/apps/api/src/tools/task-board/lanes.test.ts b/apps/api/src/tools/task-board/lanes.test.ts index c2fd8cbe7e..b675c99ef0 100644 --- a/apps/api/src/tools/task-board/lanes.test.ts +++ b/apps/api/src/tools/task-board/lanes.test.ts @@ -120,14 +120,14 @@ describe("inReviewPhase", () => { // The whole point of migration 190: a card whose reviewer is working reads // In Progress, and only the open cycle says it is under review. it("covers an In Progress card with an open cycle", () => { - expect(inReviewPhase(card("in_progress", CYCLE))).toBe(true); - expect(inReviewPhase(card("in_progress"))).toBe(false); + expect(inReviewPhase(card("in_progress", CYCLE), "in_review")).toBe(true); + expect(inReviewPhase(card("in_progress"), "in_review")).toBe(false); }); it("covers In Review with or without a cycle stamp", () => { - expect(inReviewPhase(card("in_review", CYCLE))).toBe(true); + expect(inReviewPhase(card("in_review", CYCLE), "in_review")).toBe(true); // Pre-migration cards carry no stamp; the lane still answers for them. - expect(inReviewPhase(card("in_review"))).toBe(true); + expect(inReviewPhase(card("in_review"), "in_review")).toBe(true); }); // A stale stamp must never drag a shipped card back into the sweeper's work. @@ -139,12 +139,36 @@ describe("inReviewPhase", () => { "done", "archived", ] as const) { - expect(inReviewPhase(card(lane, CYCLE))).toBe(false); + expect(inReviewPhase(card(lane, CYCLE), "in_review")).toBe(false); } }); it("is false for a card that has not been worked yet", () => { - expect(inReviewPhase(card("triage"))).toBe(false); - expect(inReviewPhase(card("todo"))).toBe(false); + expect(inReviewPhase(card("triage"), "in_review")).toBe(false); + expect(inReviewPhase(card("todo"), "in_review")).toBe(false); + }); + + /** + * The bug this argument exists for. On a board mirrored from a tracker the + * review column is called whatever that tracker calls it, so comparing + * against Studio's name read every parked card as out of the phase — and the + * sweeper, the merge retry and the own-review guard all skipped it in + * silence. + */ + it("answers for a review column the tracker named", () => { + const parked = { status: "Code Review", reviewCycleStartedAt: null }; + expect(inReviewPhase(parked, "Code Review")).toBe(true); + expect(inReviewPhase(parked, "in_review")).toBe(false); + }); + + /** A board with no review column still has cards under review — the open + * cycle is the durable fact, and it does not need a lane to be true. */ + it("still covers an open cycle when the board has no review column", () => { + expect( + inReviewPhase({ status: "Fazendo", reviewCycleStartedAt: CYCLE }, null), + ).toBe(true); + expect( + inReviewPhase({ status: "Fazendo", reviewCycleStartedAt: null }, null), + ).toBe(false); }); }); diff --git a/apps/api/src/tools/task-board/lanes.ts b/apps/api/src/tools/task-board/lanes.ts index 753f5c34e0..817292260b 100644 --- a/apps/api/src/tools/task-board/lanes.ts +++ b/apps/api/src/tools/task-board/lanes.ts @@ -110,15 +110,25 @@ export function isTaggableMergedStatus(status: string): boolean { * of the phase whatever a stale cycle stamp says, so a missed `closeReviewCycle` * can never drag a merged card back into the sweeper's work. */ -export function inReviewPhase(item: { - status: string; - reviewCycleStartedAt: string | null; -}): boolean { +export function inReviewPhase( + item: { + status: string; + reviewCycleStartedAt: string | null; + }, + /** The column this board parks a card in for review, or null when it has + * none. Passed rather than assumed: on a board mirrored from a tracker the + * lane is called whatever that tracker calls it, and comparing against + * Studio's name reads every such card as out of the phase. */ + reviewLane: string | null, +): boolean { const rank = laneRank(item.status); // A column Studio did not define has no place in this order, so the rank // bound simply does not apply to it. if (rank !== null && rank > LANE_RANK.in_review) return false; // Truthiness, not `!== null`: an absent stamp must read as "no cycle", and // a partial item (a fixture, a projection) carries `undefined`, not `null`. - return item.status === "in_review" || Boolean(item.reviewCycleStartedAt); + return ( + (reviewLane !== null && item.status === reviewLane) || + Boolean(item.reviewCycleStartedAt) + ); } diff --git a/apps/api/src/tools/task-board/merge-pr.ts b/apps/api/src/tools/task-board/merge-pr.ts index 5538201976..32fc3fcab4 100644 --- a/apps/api/src/tools/task-board/merge-pr.ts +++ b/apps/api/src/tools/task-board/merge-pr.ts @@ -8,7 +8,7 @@ import { shippedLane, } from "@decocms/shared/task-board"; import { recordTaskActivity } from "./activity"; -import { boardFor, shippedPatch } from "./board-handler"; +import { boardFor, boardLanes, shippedPatch } from "./board-handler"; import { reactToApprovedPrConflict } from "./conflict-reaction"; import { type ChecksStatus, @@ -443,7 +443,7 @@ export async function retryAutoMergeIfApproved( item: TaskBoardItem, ): Promise { const orgId = item.organizationId; - if (!inReviewPhase(item)) return false; + if (!inReviewPhase(item, (await boardLanes(ctx, orgId)).review)) return false; const settings = await ctx.storage.organizationSettings.get(orgId); if (settings?.flags?.auto_merge !== true) return false; // Same human-override guard `review-decision.ts` and `prs-get` honor. diff --git a/apps/api/src/tools/task-board/pr-link.ts b/apps/api/src/tools/task-board/pr-link.ts index 0891ce5f5d..6affa9e24c 100644 --- a/apps/api/src/tools/task-board/pr-link.ts +++ b/apps/api/src/tools/task-board/pr-link.ts @@ -22,6 +22,7 @@ */ import { z } from "zod"; +import { boardLanes } from "./board-handler"; import { defineTool } from "@/core/define-tool"; import { requireAuth, requireOrganization } from "@/core/studio-context"; import { extractPrFromText } from "./pr-extract"; @@ -92,6 +93,7 @@ export const TASK_BOARD_ITEM_PR_LINK = defineTool({ await ctx.storage.taskBoard.openReviewCycleIfInProgress( taskBoardItemId, organizationId, + await boardLanes(ctx, organizationId), ); // The sweeper is what hands the card to the reviewers, and it may have // just claimed this card's 5-minute budget while it had no PR to look at. diff --git a/apps/api/src/tools/task-board/pr-open-board-reaction.integration.test.ts b/apps/api/src/tools/task-board/pr-open-board-reaction.integration.test.ts index f474c2bd2d..4a7eba1999 100644 --- a/apps/api/src/tools/task-board/pr-open-board-reaction.integration.test.ts +++ b/apps/api/src/tools/task-board/pr-open-board-reaction.integration.test.ts @@ -19,6 +19,15 @@ import { type BoardDecision, } from "./pr-open-board-reaction"; +/** Studio's own board, which is what these fixtures run on. */ +const CANON_LANES = { + intake: "triage", + queue: "todo", + progress: "in_progress", + review: "in_review", + archive: "archived", +}; + const ORG = "org_propen_1"; const USER = "user_propen_1"; const PR: ExtractedPr = { @@ -50,6 +59,7 @@ describe("applyBoardDecision", () => { userId: USER, threadId: thread, pr: PR, + lanes: CANON_LANES, decision, openCards, }); diff --git a/apps/api/src/tools/task-board/pr-open-board-reaction.ts b/apps/api/src/tools/task-board/pr-open-board-reaction.ts index 9f4d31bb0f..2bb7b81c6c 100644 --- a/apps/api/src/tools/task-board/pr-open-board-reaction.ts +++ b/apps/api/src/tools/task-board/pr-open-board-reaction.ts @@ -14,6 +14,7 @@ */ import { generateObject } from "ai"; +import { type BoardLanes, boardCan, boardLanes } from "./board-handler"; import { z } from "zod"; import { SUPER_AGENT_ASSIGNEE_ID } from "@decocms/shared/task-board"; import type { StudioContext } from "@/core/studio-context"; @@ -133,9 +134,11 @@ export async function applyBoardDecision( decision: BoardDecision; /** The card set the decision was made against (this org's), for taskId validation. */ openCards: TaskBoardItem[]; + /** This org's board lanes. */ + lanes: BoardLanes; }, ): Promise { - const { orgId, userId, threadId, pr, decision, openCards } = params; + const { orgId, userId, threadId, pr, decision, openCards, lanes } = params; const linkPr = (taskBoardItemId: string) => storage.linkPr({ @@ -155,8 +158,25 @@ export async function applyBoardDecision( let item: TaskBoardItem | null; if (target) { - // Enter the review phase only from an earlier lane; never regress a finished card. - const advancing = ADVANCEABLE.has(target.status); + // Enter the review phase only from an earlier lane; never regress a finished + // card — and only onto a lane this board HAS. Folded into `advancing` rather + // than left to a null status: `undefined` already means "leave the status + // alone" here, so reusing it for "nowhere to advance to" would also skip the + // Super Agent claim and the review cycle without saying why. + const progressLane = lanes.progress; + // The LANE, not a boolean: `boardCan` narrows `progressLane` inside this + // expression, and a boolean would not carry that to the write below. + const advanceTo = + ADVANCEABLE.has(target.status) && + boardCan( + orgId, + "in_progress", + progressLane, + "moving a card when its PR opens", + ) + ? progressLane + : null; + const advancing = advanceTo !== null; // Claim an unowned card for the Super Agent (reviewer dispatch gates on it); never a human's. const claimSuperAgent = advancing && target.assigneeId == null; item = await storage.update( @@ -166,25 +186,29 @@ export async function applyBoardDecision( // In Progress, not In Review: a reviewer is about to work on this PR, // and In Review is what the board says once it is a person's turn. // The open cycle below is what puts it on the reviewer's work list. - status: advancing ? "in_progress" : undefined, + status: advanceTo ?? undefined, ...(claimSuperAgent ? { assigneeId: SUPER_AGENT_ASSIGNEE_ID, assignedBy: userId } : {}), }, userId, ); - if (advancing) await storage.openReviewCycleIfInProgress(target.id, orgId); + if (advancing) { + await storage.openReviewCycleIfInProgress(target.id, orgId, lanes); + } } else { // Create (also the unknown-taskId fallback), owned by the Super Agent so reviewers pick it up. item = await storage.create({ organizationId: orgId, title: decision.title?.trim() || `PR #${pr.number}`, - status: "in_progress", + // A card born mid-review with no in-progress column starts at intake — + // the one lane every board has. + status: lanes.progress ?? lanes.intake, assigneeId: SUPER_AGENT_ASSIGNEE_ID, assignedBy: userId, by: userId, }); - if (item) await storage.openReviewCycleIfInProgress(item.id, orgId); + if (item) await storage.openReviewCycleIfInProgress(item.id, orgId, lanes); } if (!item) return null; @@ -241,6 +265,7 @@ export async function reactToPrOpenedForBoard( if (!decision) return; await applyBoardDecision(ctx.storage.taskBoard, { + lanes: await boardLanes(ctx, orgId), orgId, userId, threadId, diff --git a/apps/api/src/tools/task-board/prs-get.ts b/apps/api/src/tools/task-board/prs-get.ts index d0d2d2f7d9..4857993003 100644 --- a/apps/api/src/tools/task-board/prs-get.ts +++ b/apps/api/src/tools/task-board/prs-get.ts @@ -14,7 +14,7 @@ import { retry, RetryError } from "@decocms/shared/std"; import { InMemoryMcpReadCache } from "@/mcp-clients/mcp-read-cache"; import { TaskBoardItemPrSchema } from "./schema"; import { cardWorkLanded } from "./archive-merged"; -import { boardFor, shippedPatch } from "./board-handler"; +import { boardFor, boardLanes, shippedPatch } from "./board-handler"; import { recordTaskActivity } from "./activity"; import { inReviewPhase, movesForward } from "./lanes"; import { emitTaskBoardUpdated } from "./run-reactions"; @@ -1212,7 +1212,7 @@ export const TASK_BOARD_ITEM_PRS_GET = defineTool({ const openPr = prs.find((p) => p.state === "open" && !p.merged); if ( item && - inReviewPhase(item) && + inReviewPhase(item, (await boardLanes(ctx, organizationId)).review) && item.assigneeId === SUPER_AGENT_ASSIGNEE_ID && prReadyForReview(prs) ) { diff --git a/apps/api/src/tools/task-board/quota-refund.integration.test.ts b/apps/api/src/tools/task-board/quota-refund.integration.test.ts index 2e6d3a7104..130d84c155 100644 --- a/apps/api/src/tools/task-board/quota-refund.integration.test.ts +++ b/apps/api/src/tools/task-board/quota-refund.integration.test.ts @@ -25,6 +25,15 @@ import { } from "../../billing/task-quota"; import { advanceTasksToReviewOnThreadFinish } from "./run-reactions"; +/** Studio's own board, which is what these fixtures run on. */ +const CANON_LANES = { + intake: "triage", + queue: "todo", + progress: "in_progress", + review: "in_review", + archive: "archived", +}; + const ORG = "org_refund_wiring"; const USER = "user_refund_wiring"; @@ -139,6 +148,7 @@ describe("quota refund on thread finish (wiring)", () => { thread.id, ORG, billing, + CANON_LANES, ); expect(await stateOf(task.id)).toBe("released"); @@ -156,6 +166,7 @@ describe("quota refund on thread finish (wiring)", () => { thread.id, ORG, billing, + CANON_LANES, ); expect(await stateOf(task.id)).toBe("held"); @@ -180,6 +191,7 @@ describe("quota refund on thread finish (wiring)", () => { thread.id, ORG, billing, + CANON_LANES, ); expect(await stateOf(task.id)).toBe("held"); @@ -194,6 +206,7 @@ describe("quota refund on thread finish (wiring)", () => { thread.id, ORG, billing, + CANON_LANES, ); expect((await taskBoard.getById(task.id, ORG))?.status).toBe("in_review"); @@ -216,6 +229,7 @@ describe("quota refund on thread finish (wiring)", () => { thread.id, ORG, billing, + CANON_LANES, ); expect(await billing.taskClaim(task.id)).toBeNull(); diff --git a/apps/api/src/tools/task-board/reconcile-merged.test.ts b/apps/api/src/tools/task-board/reconcile-merged.test.ts index d946f93a2b..e9032197f8 100644 --- a/apps/api/src/tools/task-board/reconcile-merged.test.ts +++ b/apps/api/src/tools/task-board/reconcile-merged.test.ts @@ -55,6 +55,28 @@ function fakeCtx( }, }), }, + // The org-owned path reads the board's columns to learn which one means + // review. Keyed like Studio's lanes on purpose: this fixture is about + // the discriminator, not about vocabulary. + boardColumns: { + listByOrg: async () => [ + { + key: "in_review", + title: "in_review", + position: 0, + role: "in_review", + trackerStatuses: [], + }, + { + key: "done", + title: "done", + position: 1, + role: null, + trackerStatuses: [], + }, + ], + }, + columnAutomations: { get: async () => null }, taskBoard: { hasHumanRejectedDone: async () => over.humanRejectedDone ?? false, update: async ( diff --git a/apps/api/src/tools/task-board/reconcile-merged.ts b/apps/api/src/tools/task-board/reconcile-merged.ts index 1ea00e2055..71cd5ef13c 100644 --- a/apps/api/src/tools/task-board/reconcile-merged.ts +++ b/apps/api/src/tools/task-board/reconcile-merged.ts @@ -24,7 +24,7 @@ import type { TaskBoardItem } from "@/storage/types"; import { shippedLane } from "@decocms/shared/task-board"; import { recordTaskActivity } from "./activity"; import { cardWorkLanded, type PrLanding } from "./archive-merged"; -import { boardFor, shippedPatch } from "./board-handler"; +import { boardFor, boardLanes, shippedPatch } from "./board-handler"; import { inReviewPhase } from "./lanes"; import { emitTaskBoardUpdated } from "./run-reactions"; @@ -55,7 +55,7 @@ export async function advanceToDoneIfMerged( prs: PrLanding[], ): Promise { const orgId = item.organizationId; - if (!inReviewPhase(item)) return false; + if (!inReviewPhase(item, (await boardLanes(ctx, orgId)).review)) return false; if (!cardWorkLanded(prs)) return false; if (await ctx.storage.taskBoard.hasHumanRejectedDone(item.id, orgId)) { return false; diff --git a/apps/api/src/tools/task-board/rerun-closes-review-cycle.integration.test.ts b/apps/api/src/tools/task-board/rerun-closes-review-cycle.integration.test.ts index 7337d83caf..d60f7640c3 100644 --- a/apps/api/src/tools/task-board/rerun-closes-review-cycle.integration.test.ts +++ b/apps/api/src/tools/task-board/rerun-closes-review-cycle.integration.test.ts @@ -21,6 +21,15 @@ import { } from "../../database/test-db-pg"; import { TaskBoardStorage } from "../../storage/task-board"; +/** Studio's own board, which is what these fixtures run on. */ +const CANON_LANES = { + intake: "triage", + queue: "todo", + progress: "in_progress", + review: "in_review", + archive: "archived", +}; + const ORG = "org_rerun_cycle_1"; const USER = "user_rc1"; @@ -54,7 +63,11 @@ describe("a re-run closes the review cycle it inherited", () => { by: USER, }); // A reviewer already stamped an open cycle — the state a rerun targets. - const opened = await taskBoard.openReviewCycleIfInProgress(item.id, ORG); + const opened = await taskBoard.openReviewCycleIfInProgress( + item.id, + ORG, + CANON_LANES, + ); expect(opened?.reviewCycleStartedAt).not.toBeNull(); // The exact sequence TASK_BOARD_ITEM_RERUN's handler now runs. @@ -62,7 +75,11 @@ describe("a re-run closes the review cycle it inherited", () => { await taskBoard.update(item.id, ORG, { status: "in_progress" }, USER); // Without the fix this returns null: the column is still non-null. - const reopened = await taskBoard.openReviewCycleIfInProgress(item.id, ORG); + const reopened = await taskBoard.openReviewCycleIfInProgress( + item.id, + ORG, + CANON_LANES, + ); expect(reopened).not.toBeNull(); expect(reopened?.reviewCycleStartedAt).not.toBe( opened?.reviewCycleStartedAt, diff --git a/apps/api/src/tools/task-board/rerun.ts b/apps/api/src/tools/task-board/rerun.ts index 7dd146dc41..08c2869a96 100644 --- a/apps/api/src/tools/task-board/rerun.ts +++ b/apps/api/src/tools/task-board/rerun.ts @@ -43,6 +43,7 @@ import { SUPER_AGENT_ASSIGNEE_ID, } from "@decocms/shared/task-board"; import { TERMINAL_THREAD_STATUSES } from "@/storage/task-board"; +import { boardLanes } from "./board-handler"; import { inReviewPhase } from "./lanes"; import { broadcastRunCancel } from "@/api/routes/decopilot/cancel-registry"; import { cancelHostedHarness } from "@/dispatch-queue"; @@ -258,7 +259,8 @@ export async function refuseIfMergePending( reviewCycleStartedAt: string | null; }, ): Promise { - if (!inReviewPhase(item)) return; + const reviewLane = (await boardLanes(ctx, item.organizationId)).review; + if (!inReviewPhase(item, reviewLane)) return; const settings = await ctx.storage.organizationSettings.get( item.organizationId, ); @@ -375,12 +377,15 @@ export const TASK_BOARD_ITEM_RERUN = defineTool({ // `TaskQuotaError` on an empty period bucket — which must surface, so NOT // best-effort: a swallowed failure here is exactly the silent no-op this // tool exists to remove. - const updated = await ctx.storage.taskBoard.update( - id, - organizationId, - { status: "in_progress" }, - getUserId(ctx)!, - ); + const progress = (await boardLanes(ctx, organizationId)).progress; + const updated = progress + ? await ctx.storage.taskBoard.update( + id, + organizationId, + { status: progress }, + getUserId(ctx)!, + ) + : item; await recordTaskActivity(ctx, { taskBoardItemId: id, diff --git a/apps/api/src/tools/task-board/resolve-conflict.ts b/apps/api/src/tools/task-board/resolve-conflict.ts index ca3e3c1f8a..924910e095 100644 --- a/apps/api/src/tools/task-board/resolve-conflict.ts +++ b/apps/api/src/tools/task-board/resolve-conflict.ts @@ -39,6 +39,7 @@ import { userInitiatedTaskQuotaConfig, } from "@/billing/task-quota"; import { recordTaskActivity } from "./activity"; +import { boardLanes } from "./board-handler"; import { emitTaskBoardUpdated } from "./run-reactions"; import { enqueueSuperAgentForTask } from "./enqueue-super-agent"; import { fetchPrConflict } from "./prs-get"; @@ -105,10 +106,24 @@ export const TASK_BOARD_RESOLVE_CONFLICT = defineTool({ await ensureTaskExecutionAllowed(ctx, item, userInitiatedTaskQuotaConfig()); // Atomic dispatch fence shared with the automatic reaction (see doc above). + const lanes = await boardLanes(ctx, organizationId); + // The fence moves the card between these two, and the failure path moves it + // back. A board missing either cannot express the round trip, so it never + // starts one — and narrowing here is what lets the revert below write a + // string instead of an "undefined means leave it alone" that would strand + // the card mid-fence. + if (lanes.review === null || lanes.progress === null) { + throw new Error( + "This board has no column meaning 'under review' or 'in progress', so " + + "there is nowhere to move the card while the conflict is resolved. " + + "Set those roles on the board's columns first.", + ); + } const claimed = await ctx.storage.taskBoard.claimConflictResolution( id, organizationId, getUserId(ctx)!, + lanes, ); if (!claimed) { throw new Error( @@ -128,7 +143,7 @@ export const TASK_BOARD_RESOLVE_CONFLICT = defineTool({ } catch (err) { // Dispatch failed after the fence bounced the status — bounce it back. await ctx.storage.taskBoard - .update(claimed.id, organizationId, { status: "in_review" }, "system") + .update(claimed.id, organizationId, { status: lanes.review }, "system") .then((reverted) => emitTaskBoardUpdated(organizationId, reverted)) .catch((revertErr) => console.error( diff --git a/apps/api/src/tools/task-board/review-sweeper.ts b/apps/api/src/tools/task-board/review-sweeper.ts index d11f1069b7..a8299426fe 100644 --- a/apps/api/src/tools/task-board/review-sweeper.ts +++ b/apps/api/src/tools/task-board/review-sweeper.ts @@ -61,6 +61,9 @@ */ import type { StudioContextFactory } from "@/automations/fire"; +import { boardLanesForDb } from "./board-handler"; +import type { Kysely } from "kysely"; +import type { Database } from "@/storage/types"; import type { StudioContext } from "@/core/studio-context"; import type { OrganizationBillingStorage } from "@/storage/organization-billing"; import type { TaskBoardStorage } from "@/storage/task-board"; @@ -170,6 +173,9 @@ export class TaskBoardReviewSweeper { private readonly taskBoard: TaskBoardStorage, private readonly contextFactory: StudioContextFactory, private readonly billing: OrganizationBillingStorage, + /** Resolves each card's own board — the sweep spans orgs, so the lane a + * status has to match is per row, not per tick. */ + private readonly db: Kysely, private readonly options: TaskBoardReviewSweeperOptions = {}, ) {} @@ -283,7 +289,12 @@ export class TaskBoardReviewSweeper { console.warn( `[task-board-review-sweeper] reacting to an unhandled failure on ${id}`, ); - await reactToFailedTaskRun(this.taskBoard, threadId, organizationId); + await reactToFailedTaskRun( + this.taskBoard, + threadId, + organizationId, + await boardLanesForDb(this.db, organizationId), + ); await refundUnproductiveTaskClaims( this.taskBoard, this.billing, @@ -375,6 +386,7 @@ export class TaskBoardReviewSweeper { organizationId, attempts, new Date(Date.now() + REARM_DELAY_MS), + (await boardLanesForDb(this.db, organizationId)).progress, ) .catch((rearmErr) => console.error( @@ -399,12 +411,14 @@ export class TaskBoardReviewSweeper { reason: string, ): Promise { try { + const lanes = await boardLanesForDb(this.db, organizationId); const item = await this.taskBoard.getById(id, organizationId); if (!item || item.status !== "in_progress") return; const returned = await this.taskBoard.returnToTodoAfterFailure( id, organizationId, item.updatedBy, + lanes, ); if (!returned) return; await this.taskBoard @@ -477,7 +491,8 @@ export class TaskBoardReviewSweeper { // Re-check against the fresh row: `listItemsPendingReview` scanned a // possibly-stale snapshot, and a human can bounce the card between that scan // and this reconcile. - if (!inReviewPhase(item)) return false; + const lanes = await boardLanesForDb(this.db, organizationId); + if (!inReviewPhase(item, lanes.review)) return false; // Everything below EXCEPT the merge retry needs the Super Agent to still own // the card — the same gate `TASK_BOARD_ITEM_PRS_GET` applies before its own // `enqueueEnabledReviewers` call. A handed-off card burns no agent runs, but diff --git a/apps/api/src/tools/task-board/run-reactions.test.ts b/apps/api/src/tools/task-board/run-reactions.test.ts index 9c75fe080e..106d1e756e 100644 --- a/apps/api/src/tools/task-board/run-reactions.test.ts +++ b/apps/api/src/tools/task-board/run-reactions.test.ts @@ -10,6 +10,15 @@ import { import { SUPER_AGENT_ASSIGNEE_ID } from "@decocms/shared/task-board"; import type { TaskBoardItem } from "@/storage/types"; +/** Studio's own board, which is what these fixtures run on. */ +const CANON_LANES = { + intake: "triage", + queue: "todo", + progress: "in_progress", + review: "in_review", + archive: "archived", +}; + type LinkPrCall = { taskBoardItemId: string; organizationId: string; @@ -230,6 +239,9 @@ describe("handTaskToHuman", () => { const activityCalls: unknown[] = []; const ctx = { storage: { + // The hand-off parks the card first, and parking asks the board where + // its review column is. No settings row = Studio's own board. + organizationSettings: { get: async () => null }, taskBoard: { unassignSuperAgent: async () => makeItem({ assigneeId: null }), recordActivity: async (p: unknown) => { @@ -300,13 +312,13 @@ describe("reactToFailedTaskRun on a card that already moved on", () => { it("relabels the failure and schedules nothing", async () => { const { taskBoard, calls } = fakeBoard("in_review"); - await reactToFailedTaskRun(taskBoard, "thr-1", "org-1"); + await reactToFailedTaskRun(taskBoard, "thr-1", "org-1", CANON_LANES); expect(calls).toEqual(["relabel"]); }); it("still retries a card that is genuinely mid-work", async () => { const { taskBoard, calls } = fakeBoard("in_progress"); - await reactToFailedTaskRun(taskBoard, "thr-1", "org-1"); + await reactToFailedTaskRun(taskBoard, "thr-1", "org-1", CANON_LANES); expect(calls).toEqual(["retry"]); }); }); diff --git a/apps/api/src/tools/task-board/run-reactions.ts b/apps/api/src/tools/task-board/run-reactions.ts index 71975c82e1..1b9558a771 100644 --- a/apps/api/src/tools/task-board/run-reactions.ts +++ b/apps/api/src/tools/task-board/run-reactions.ts @@ -31,6 +31,7 @@ import { captureOrgEvent } from "@/posthog"; import type { OrganizationBillingStorage } from "@/storage/organization-billing"; import { TERMINAL_THREAD_STATUSES } from "@/storage/task-board"; import type { StudioContext } from "@/core/studio-context"; +import { type BoardLanes, boardCan, boardLanes } from "./board-handler"; import { extractPrFromValue } from "./pr-extract"; import { retryBudgetFor } from "./transient-failure"; import { exponentialBackoffWithJitter } from "@decocms/shared/std"; @@ -224,6 +225,7 @@ export async function openReviewCycleForRun( const opened = await ctx.storage.taskBoard.openReviewCycleIfInProgress( itemId, orgId, + await boardLanes(ctx, orgId), ); if (!opened) continue; emitTaskBoardUpdated(orgId, opened); @@ -287,11 +289,14 @@ export async function advanceTasksToReviewOnThreadFinish( /** Quota bookkeeping (billing/task-quota.ts). Required on purpose: an * optional arg here is a silent way for a caller to stop refunding. */ billing: OrganizationBillingStorage, + /** This org's board lanes — see `reactToFailedTaskRun`. */ + lanes: BoardLanes, ): Promise { try { const moved = await taskBoard.advanceLinkedTasksToReviewOnThreadFinish( threadId, orgId, + lanes, ); for (const item of moved) { emitTaskBoardUpdated(orgId, item); @@ -308,7 +313,7 @@ export async function advanceTasksToReviewOnThreadFinish( } catch (err) { console.error("[task-board] thread-finish transition failed", err); } - await reactToFailedTaskRun(taskBoard, threadId, orgId); + await reactToFailedTaskRun(taskBoard, threadId, orgId, lanes); await refundUnproductiveTaskClaims(taskBoard, billing, threadId, orgId); } @@ -373,6 +378,10 @@ export async function reactToFailedTaskRun( taskBoard: TaskBoardStorage, threadId: string, orgId: string, + /** This org's board lanes. Passed rather than resolved here: the caller + * already holds a board or a handle to build one from, and taking the I/O + * out keeps this reaction testable without a database. */ + lanes: BoardLanes, ): Promise { try { const failure = await taskBoard.failedRunInfo(threadId, orgId); @@ -418,6 +427,7 @@ export async function reactToFailedTaskRun( orgId, attempts + 1, new Date(Date.now() + delay), + lanes.progress, ); if (!scheduled) continue; await taskBoard @@ -440,6 +450,7 @@ export async function reactToFailedTaskRun( itemId, orgId, item.updatedBy, + lanes, ); if (!returned) continue; await taskBoard @@ -532,12 +543,24 @@ export async function parkReviewedCardForHuman( ctx: StudioContext, item: TaskBoardItem, ): Promise { - if (item.status !== "in_progress" || !inReviewPhase(item)) return; + const lanes = await boardLanes(ctx, item.organizationId); + if (item.status !== lanes.progress || !inReviewPhase(item, lanes.review)) + return; + if ( + !boardCan( + item.organizationId, + "in_review", + lanes.review, + "parking a reviewed card for a person", + ) + ) { + return; + } try { const parked = await ctx.storage.taskBoard.update( item.id, item.organizationId, - { status: "in_review" }, + { status: lanes.review }, item.updatedBy, ); await ctx.storage.taskBoard @@ -582,7 +605,14 @@ export async function handTaskToHuman( const orgId = item.organizationId; if (item.assigneeId !== SUPER_AGENT_ASSIGNEE_ID) return false; try { - await parkReviewedCardForHuman(ctx, item); + // Best-effort: parking needs to know the board, and a card must still + // reach a person when that read fails. + await parkReviewedCardForHuman(ctx, item).catch((err) => + console.warn( + `[task-board] parking ${item.id} before hand-off failed`, + err, + ), + ); // Re-checks the assignee against the DB, not this stale `item`. const handed = await ctx.storage.taskBoard.unassignSuperAgent( item.id, diff --git a/apps/api/src/tools/task-board/stall-recovery.ts b/apps/api/src/tools/task-board/stall-recovery.ts index 5a01e61f58..f8c0cb16b0 100644 --- a/apps/api/src/tools/task-board/stall-recovery.ts +++ b/apps/api/src/tools/task-board/stall-recovery.ts @@ -33,6 +33,7 @@ */ import type { StudioContext } from "@/core/studio-context"; +import { boardLanes } from "./board-handler"; import { nudgeThreadTurn } from "./nudge-thread"; import { shouldAdvanceToReview } from "@/storage/task-board"; import type { TaskBoardItem, Thread } from "@/storage/types"; @@ -246,6 +247,7 @@ export async function recoverStalledTasks( thread.threadId, organizationId, ctx.storage.organizationBilling, + await boardLanes(ctx, organizationId), ); } else { await nudgeThread(ctx, item, row); diff --git a/apps/api/src/tools/task-board/update.test.ts b/apps/api/src/tools/task-board/update.test.ts index 5d7c69ea85..a68c39a702 100644 --- a/apps/api/src/tools/task-board/update.test.ts +++ b/apps/api/src/tools/task-board/update.test.ts @@ -218,20 +218,33 @@ describe("closesOwnReview", () => { ) => ({ status, reviewCycleStartedAt }); it("catches a run completing a task under review", () => { - expect(closesOwnReview("done", underReview(), true)).toBe(true); + expect(closesOwnReview("done", underReview(), true, "in_review")).toBe( + true, + ); }); it("catches a run archiving a task under review — archiving skips review just like completing", () => { - expect(closesOwnReview("archived", underReview(), true)).toBe(true); + expect(closesOwnReview("archived", underReview(), true, "in_review")).toBe( + true, + ); }); // Shipping yourself past review also drops the card out of the review sweep. it("catches a run shipping a task under review into a delivery lane", () => { - expect(closesOwnReview("approved", underReview(), true)).toBe(true); - expect(closesOwnReview("merged", underReview(), true)).toBe(true); - expect(closesOwnReview("post_deploy_validation", underReview(), true)).toBe( + expect(closesOwnReview("approved", underReview(), true, "in_review")).toBe( + true, + ); + expect(closesOwnReview("merged", underReview(), true, "in_review")).toBe( true, ); + expect( + closesOwnReview( + "post_deploy_validation", + underReview(), + true, + "in_review", + ), + ).toBe(true); }); // The lane alone used to answer this, and In Progress read as "not under @@ -243,27 +256,34 @@ describe("closesOwnReview", () => { "done", underReview("in_progress", "2026-08-13T02:39:20Z"), true, + "in_review", ), ).toBe(true); }); it("allows a run to complete a task that needed no code change", () => { - expect(closesOwnReview("done", underReview("in_progress"), true)).toBe( - false, - ); + expect( + closesOwnReview("done", underReview("in_progress"), true, "in_review"), + ).toBe(false); }); it("allows a run to move a task under review BACKWARD, or not at all", () => { - expect(closesOwnReview("in_progress", underReview(), true)).toBe(false); - expect(closesOwnReview(undefined, underReview(), true)).toBe(false); + expect( + closesOwnReview("in_progress", underReview(), true, "in_review"), + ).toBe(false); + expect(closesOwnReview(undefined, underReview(), true, "in_review")).toBe( + false, + ); }); it("never catches a person", () => { - expect(closesOwnReview("done", underReview(), false)).toBe(false); + expect(closesOwnReview("done", underReview(), false, "in_review")).toBe( + false, + ); }); it("has nothing to protect without a pre-update card", () => { - expect(closesOwnReview("done", undefined, true)).toBe(false); + expect(closesOwnReview("done", undefined, true, "in_review")).toBe(false); }); }); diff --git a/apps/api/src/tools/task-board/update.ts b/apps/api/src/tools/task-board/update.ts index aadf99c9c3..d904ab16c5 100644 --- a/apps/api/src/tools/task-board/update.ts +++ b/apps/api/src/tools/task-board/update.ts @@ -165,13 +165,16 @@ export function closesOwnReview( inputStatus: string | undefined, previous: { status: string; reviewCycleStartedAt: string | null } | undefined, isTaskRun: boolean, + /** This board's review column — see `inReviewPhase`. */ + reviewLane: string | null, ): boolean { const completesTask = inputStatus !== undefined && REVIEW_CLOSING_STATUSES.has(inputStatus); // The PHASE, not the lane: a card whose reviewer is still working reads In // Progress since migration 190, and gating on the lane alone would let the // author's own run mark its work Done out from under that reviewer. - const awaitingReview = previous !== undefined && inReviewPhase(previous); + const awaitingReview = + previous !== undefined && inReviewPhase(previous, reviewLane); return isTaskRun && completesTask && awaitingReview; } @@ -338,7 +341,14 @@ export const TASK_BOARD_ITEM_UPDATE = defineTool({ } const isTaskRun = taskRunContextStore.getStore() !== undefined; - if (closesOwnReview(input.status, previous ?? undefined, isTaskRun)) { + if ( + closesOwnReview( + input.status, + previous ?? undefined, + isTaskRun, + (await board.lanes()).review, + ) + ) { throw new Error( "This task is under review — a run can't move it to Done or Archived. " + "Leave it for the reviewer; only a person, or " + diff --git a/apps/web/src/i18n/en/settings.ts b/apps/web/src/i18n/en/settings.ts index a9e0deffb7..6ac2846ce8 100644 --- a/apps/web/src/i18n/en/settings.ts +++ b/apps/web/src/i18n/en/settings.ts @@ -48,6 +48,8 @@ export const settings = { "settings.jira.rolesDescription": "Your board's columns come from Jira. Tell Studio which of them is where review happens, and which retires a card — most columns mean nothing to it, and that is fine.", "settings.jira.roleNone": "Nothing special", + "settings.jira.roleQueued": "Queued for the agent", + "settings.jira.roleInProgress": "Being worked on", "settings.jira.roleInReview": "Under review", "settings.jira.roleArchived": "Archive", "settings.jira.noColumnsYet": diff --git a/apps/web/src/i18n/pt-br/settings.ts b/apps/web/src/i18n/pt-br/settings.ts index 967cd0e1cd..beb768f37c 100644 --- a/apps/web/src/i18n/pt-br/settings.ts +++ b/apps/web/src/i18n/pt-br/settings.ts @@ -50,6 +50,8 @@ export const settings = { "settings.jira.rolesDescription": "As colunas do seu board vêm do Jira. Diga ao Studio qual delas é onde a revisão acontece, e qual arquiva um card — a maioria não significa nada pra ele, e tudo bem.", "settings.jira.roleNone": "Nada em especial", + "settings.jira.roleQueued": "Na fila do agente", + "settings.jira.roleInProgress": "Em progresso", "settings.jira.roleInReview": "Em revisão", "settings.jira.roleArchived": "Arquivo", "settings.jira.noColumnsYet": diff --git a/apps/web/src/views/settings/jira.tsx b/apps/web/src/views/settings/jira.tsx index b0a2ae3f6f..0f87a09970 100644 --- a/apps/web/src/views/settings/jira.tsx +++ b/apps/web/src/views/settings/jira.tsx @@ -259,7 +259,8 @@ const NO_ROLE = "__none__"; * Replaces the status mapping for an org whose board is its own. The mapping * asked a team to restate, lane by lane, something their tracker already knew; * this asks the one thing it could not know — which of THEIR columns is where - * review happens, and which retires a card. Most columns mean nothing to us, + * the agent picks work up, where it works, where review happens, and which + * retires a card. Most columns mean nothing to us, * and leaving them that way is the safe answer rather than a gap to fill. */ function ColumnRoleRows() { @@ -308,6 +309,12 @@ function ColumnRoleRows() { {t("settings.jira.roleNone")} + + {t("settings.jira.roleQueued")} + + + {t("settings.jira.roleInProgress")} + {t("settings.jira.roleInReview")} diff --git a/packages/shared/src/tools/tool-io.ts b/packages/shared/src/tools/tool-io.ts index 71ceda19bd..e91cb3d18f 100644 --- a/packages/shared/src/tools/tool-io.ts +++ b/packages/shared/src/tools/tool-io.ts @@ -562,7 +562,10 @@ export interface StudioToolIO { output: { removed: boolean }; }; TASK_BOARD_COLUMN_ROLE_SET: { - input: { columnKey: string; role: "in_review" | "archived" | null }; + input: { + columnKey: string; + role: "in_progress" | "todo" | "in_review" | "archived" | null; + }; output: { columnKey: string; role: string | null }; }; TASK_BOARD_ITEM_PRS_GET: {