diff --git a/packages/dashboard/src/__tests__/routes-pull-requests.test.ts b/packages/dashboard/src/__tests__/routes-pull-requests.test.ts index eb4e517f48..6b838b6f14 100644 --- a/packages/dashboard/src/__tests__/routes-pull-requests.test.ts +++ b/packages/dashboard/src/__tests__/routes-pull-requests.test.ts @@ -238,6 +238,39 @@ describe("column move-backward guard (R16)", () => { ); }); + it("does NOT protect a column whose id is not in the legacy COLUMNS array", () => { + /* + FNXC:WorkflowResolvedColumns 2026-07-31-05:30 (PR #2713 review — greptile P1): + The guard's own escape hatch, pinned so callers know it exists. `COLUMNS.indexOf` returns -1 for + any custom column id, and this function bails on a negative index — so passing an unmapped + column silently ALLOWS the move rather than blocking it. + + That is a reasonable contract for a legacy-index helper, and a trap for callers converting to + resolved roles: the re-engagement gate in register-task-workflow-routes.ts started admitting + custom review columns, kept passing `COLUMNS.indexOf(task.column)`, and stopped protecting them + entirely. The fix there is to pass the review LANE'S index, since the caller has already + established the task is in review. + + Asserted as documentation of the hazard, not as desired behaviour for custom boards. + */ + expect( + isBackwardMoveBlockedByOpenPr({ + fromIndex: -1, + toIndex: 2, + activePrEntity: buildEntity({ state: "open" }), + }), + ).toBe(false); + + // And with the review lane's index supplied instead, the same open PR blocks correctly. + expect( + isBackwardMoveBlockedByOpenPr({ + fromIndex: 3, + toIndex: 2, + activePrEntity: buildEntity({ state: "open" }), + }), + ).toBe(true); + }); + it("allows the backward move once the PR is terminal (no active entity)", () => { expect( isBackwardMoveBlockedByOpenPr({ fromIndex: 3, toIndex: 2, activePrEntity: null }), diff --git a/packages/dashboard/src/routes/register-task-workflow-routes.ts b/packages/dashboard/src/routes/register-task-workflow-routes.ts index f103d11834..d9cb4f7f6f 100644 --- a/packages/dashboard/src/routes/register-task-workflow-routes.ts +++ b/packages/dashboard/src/routes/register-task-workflow-routes.ts @@ -229,6 +229,75 @@ async function resolveWipColumnForTask(store: TaskStore, taskId: string): Promis } } +/* +FNXC:WorkflowResolvedColumns 2026-07-31-03:30 (fleet: register-task-workflow-routes.ts): +The three roles this file needed and did not have, following the idiom its own +`resolveIntakeColumnForTask` / `resolveWipColumnForTask` / `resolveReboundColumnForTask` already +established: resolve the column ID from the task's workflow, fall back to the legacy id when the IR +cannot be read. + +Deliberately NOT the `columnRoles` predicate helpers used in `packages/dashboard/app`. Those take +resolved trait flags, which a route handler does not have — it has a store and a task id, and must +do an async lookup. Importing them here would mean fetching flags per request to answer a question +this file already answers a simpler way. One idiom per layer. + +`resolveReviewColumnForTask` accepts EITHER trait, matching `isReviewColumnRole` on the app side: a +lane can block merges without a human in it and vice versa, and every caller here asks "is this card +in review". +*/ +/* +FNXC:WorkflowResolvedColumns 2026-07-31-06:40 (PR #2713 review — greptile P1, same class as the +terminal finding one round earlier): +MEMBERSHIP, because `mergeBlocker` and `humanReview` can sit on DIFFERENT columns. + +The previous shape took `columnsWithFlag(ir, "mergeBlocker")[0] ?? columnsWithFlag(ir, +"humanReview")[0]` — one id. A workflow that splits the two (a merge lane and a separate sign-off +lane) then classified a task in the second as outside review entirely, so comment re-engagement was +suppressed on a card sitting in human review. + +I fixed exactly this arity bug for the terminal columns one review round earlier and did not +generalise it. The rule, stated so the next resolver added here does not repeat it a third time: +single id answers "where should this card GO" — a move target must be one column. A SET answers "is +this card ALREADY there" — membership. Every resolver used in a comparison against `task.column` is +the second kind. +*/ +async function resolveReviewColumnsForTask(store: TaskStore, taskId: string): Promise> { + try { + const ir = await resolveWorkflowIrForTask(store, taskId); + const lanes = [...columnsWithFlag(ir, "mergeBlocker"), ...columnsWithFlag(ir, "humanReview")]; + return lanes.length > 0 ? new Set(lanes) : new Set(["in-review"]); + } catch { + return new Set(["in-review"]); + } +} + +/* +FNXC:WorkflowResolvedColumns 2026-07-31-05:00 (PR #2713 review — greptile P1): +MEMBERSHIP, not "the first one". A workflow may declare MORE THAN ONE column carrying `complete` or +`archived`, and `columnsWithFlag(...)[0]` picks one of them — so a task sitting in the second valid +terminal column failed the revert guard with a 409. + +The single-id shape is right for the file's older resolvers, which answer "where should this card +GO" (a move target must be one column). It is wrong here, because these answer "is this card ALREADY +terminal" — a membership question. Same helper name, opposite arity requirement; that is what made +the flaw easy to inherit. + +Returns the full set and lets callers test membership. The legacy id is the fallback set when the IR +cannot be read, matching the other resolvers' degraded behaviour. +*/ +async function resolveTerminalColumnsForTask(store: TaskStore, taskId: string): Promise> { + try { + const ir = await resolveWorkflowIrForTask(store, taskId); + const complete = columnsWithFlag(ir, "complete"); + const archived = columnsWithFlag(ir, "archived"); + const all = [...complete, ...archived]; + return all.length > 0 ? new Set(all) : new Set(["done", "archived"]); + } catch { + return new Set(["done", "archived"]); + } +} + + function isArtifactType(value: string): value is ArtifactType { return ARTIFACT_TYPES.has(value as ArtifactType); } @@ -797,7 +866,8 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork task: Task, wake: InReviewUserCommentReengagementInput, ): Promise { - if (task.column !== "in-review") { + const reviewGateColumns = await resolveReviewColumnsForTask(scopedStore, task.id); + if (!reviewGateColumns.has(task.column)) { return { task, reengaged: false, suppressedReason: "not-in-review" }; } if (task.sessionFile) { @@ -821,7 +891,21 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork */ if ( isBackwardMoveBlockedByOpenPr({ - fromIndex: COLUMNS.indexOf(task.column as Column), + /* + FNXC:WorkflowResolvedColumns 2026-07-31-05:20 (PR #2713 review — greptile P1): + The REVIEW LANE'S legacy index, not `COLUMNS.indexOf(task.column)`. + + `isBackwardMoveBlockedByOpenPr` bails with `if (fromIndex < 0) return false`, and + `COLUMNS.indexOf` returns -1 for any custom column id. Before this PR the gate above only + admitted a task literally in `in-review`, so the index was always valid; converting the gate + to resolve the review ROLE started admitting custom columns, and the guard then silently + stopped protecting them — the task moved out of review with an open PR. + + A conversion that widens what reaches a downstream legacy-index check has to carry that + check with it. We have already established `task.column` IS this workflow's review lane, so + it occupies the review POSITION for an ordering question, whatever it is named. + */ + fromIndex: COLUMNS.indexOf("in-review"), toIndex: COLUMNS.indexOf("in-progress"), activePrEntity, }) @@ -2063,7 +2147,8 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork if (!task) { throw notFound(`Task ${req.params.id} not found`); } - if (task.column !== "done" && task.column !== "archived") { + const terminalColumns = await resolveTerminalColumnsForTask(scopedStore, task.id); + if (!terminalColumns.has(task.column)) { throw conflict(`Task ${task.id} is in column "${task.column}"; only done/archived tasks can be reverted`); } @@ -2699,8 +2784,14 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork && (task.column === lifecycle.intake || task.column === lifecycle.hold); } } + /* + FNXC:WorkflowResolvedColumns 2026-07-31-04:15 (fleet: register-task-workflow-routes.ts): + Resolved ONCE for the whole retry handler and reused by all three review checks below, so they + cannot disagree about which column is the review lane. + */ + const retryReviewColumns = await resolveReviewColumnsForTask(scopedStore, task.id); const isInReviewStatusNone = - task.column === "in-review" && (task.status === null || task.status === undefined); + retryReviewColumns.has(task.column) && (task.status === null || task.status === undefined); const hasIncompleteSteps = task.steps.some( (s: { status: string }) => s.status === "pending" || s.status === "in-progress", ); @@ -2733,13 +2824,13 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork */ const selfHealingManager = _resolveSelfHealingManager(scopedStore); const isStaleMergeActiveRetry = - task.column === "in-review" && + retryReviewColumns.has(task.column) && isStaleMergeActiveStatus(task, { activeMergeTaskId: selfHealingManager?.getActiveMergeTaskId?.() ?? null, minAgeMs: selfHealingManager?.getStaleMergingStatusMinAgeMs?.(), }); const isInReviewRetry = - task.column === "in-review" && + retryReviewColumns.has(task.column) && (task.status === "failed" || task.status === "stuck-killed" || isInReviewExecutionStall || @@ -3780,7 +3871,8 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork if (!task) { throw notFound(`Task ${req.params.id} not found`); } - if (task.column !== "in-review") { + const prStatusReviewColumns = await resolveReviewColumnsForTask(scopedStore, task.id); + if (!prStatusReviewColumns.has(task.column)) { throw badRequest("Task must be in 'in-review' column to recover branch binding"); } @@ -4098,7 +4190,8 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork triggerDetail: "task-comment", }; if (normalizedAuthor === "user") { - if (task.column === "in-review" && !task.sessionFile) { + const diffReviewColumns = await resolveReviewColumnsForTask(scopedStore, task.id); + if (diffReviewColumns.has(task.column) && !task.sessionFile) { const { task: reengagedTask } = await reengageInReviewTaskForUserComment(scopedStore, task, wake); res.json(reengagedTask); return; @@ -4665,7 +4758,8 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork triggeringCommentIds: newSteeringCommentId ? [newSteeringCommentId] : undefined, triggerDetail: "steering-comment", }; - if (task.column === "in-review" && !task.sessionFile) { + const artifactReviewColumns = await resolveReviewColumnsForTask(scopedStore, task.id); + if (artifactReviewColumns.has(task.column) && !task.sessionFile) { const { task: reengagedTask } = await reengageInReviewTaskForUserComment(scopedStore, task, wake); res.json(reengagedTask); return; @@ -4804,7 +4898,17 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork const currentColumn = "columns" in workflowIr ? workflowIr.columns.find((column) => column.id === task.column) : undefined; - const isArchived = task.column === "archived" || (currentColumn != null && resolveColumnFlags(currentColumn).archived); + /* + FNXC:WorkflowResolvedColumns 2026-07-31-04:00 (fleet: register-task-workflow-routes.ts): + FLAGS-FIRST, id only as the fallback. This ORed the legacy id with the resolved trait + unconditionally, so a column merely NAMED `archived` counted as archived even when its own + workflow says otherwise — the same inversion pattern found in TaskContextMenu and + isPreExecutionHoldColumn. When the column resolves, its traits are the answer; the id is only + for when it does not resolve at all. + */ + const isArchived = currentColumn != null + ? resolveColumnFlags(currentColumn).archived === true + : task.column === "archived"; if (isArchived) { throw badRequest("Respecify is not available for archived tasks; unarchive first."); } @@ -5768,7 +5872,15 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork let updatedTask: Task = await scopedStore.getTask(task.id); - if (task.column === "in-review") { + /* + FNXC:WorkflowResolvedColumns 2026-07-31-03:45 (fleet: register-task-workflow-routes.ts): + Resolved once for this handler and reused by both arms below, so the review/WIP decision cannot + be made from two different answers. + */ + const steeringReviewColumns = await resolveReviewColumnsForTask(scopedStore, task.id); + const steeringWipColumn = await resolveWipColumnForTask(scopedStore, task.id); + + if (steeringReviewColumns.has(task.column)) { updatedTask = (await reengageInReviewTaskForUserComment(scopedStore, updatedTask, { triggeringCommentType: "steering", triggeringCommentIds: steeringCommentId ? [steeringCommentId] : undefined, @@ -5776,7 +5888,7 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork })).task; } else { const hasActiveSession = Boolean(updatedTask.sessionFile); - if (steeringCommentId && updatedTask.column === "in-progress" && updatedTask.assignedAgentId && !hasActiveSession) { + if (steeringCommentId && updatedTask.column === steeringWipColumn && updatedTask.assignedAgentId && !hasActiveSession) { await triggerCommentWakeForAssignedAgent(scopedStore, updatedTask, { triggeringCommentType: "steering", triggeringCommentIds: [steeringCommentId], @@ -5803,7 +5915,9 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork if (!prInfo) { throw badRequest("Task must have a linked pull request before PR feedback can be addressed"); } - if (task.column !== "in-review" && task.column !== "in-progress") { + const prFeedbackReviewColumns = await resolveReviewColumnsForTask(scopedStore, task.id); + const prFeedbackWipColumn = await resolveWipColumnForTask(scopedStore, task.id); + if (!prFeedbackReviewColumns.has(task.column) && task.column !== prFeedbackWipColumn) { throw badRequest("PR feedback can only be addressed for in-review or in-progress tasks"); } @@ -5826,7 +5940,8 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork let updatedTask: Task = await scopedStore.getTask(task.id); - if (task.column === "in-review") { + const reengageReviewColumns = await resolveReviewColumnsForTask(scopedStore, task.id); + if (reengageReviewColumns.has(task.column)) { await scopedStore.updateTask(task.id, { status: null, error: null, @@ -5844,7 +5959,8 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork } const hasActiveSession = Boolean(updatedTask.sessionFile); - if (updatedTask.column === "in-progress" && updatedTask.assignedAgentId && !hasActiveSession) { + const wakeWipColumn = await resolveWipColumnForTask(scopedStore, updatedTask.id); + if (updatedTask.column === wakeWipColumn && updatedTask.assignedAgentId && !hasActiveSession) { await triggerCommentWakeForAssignedAgent(scopedStore, updatedTask, { triggeringCommentType: "steering", triggeringCommentIds: [steeringCommentId], diff --git a/scripts/lib/lifecycle-column-census-baseline.json b/scripts/lib/lifecycle-column-census-baseline.json index f473c0e2d6..e74caef9cf 100644 --- a/scripts/lib/lifecycle-column-census-baseline.json +++ b/scripts/lib/lifecycle-column-census-baseline.json @@ -6,7 +6,6 @@ "packages/dashboard/app/components/TaskCard.tsx": 42, "packages/dashboard/app/components/TaskDetailModal.tsx": 30, "packages/engine/src/scheduler.ts": 26, - "packages/dashboard/src/routes/register-task-workflow-routes.ts": 20, "packages/core/src/store.ts": 12, "packages/engine/src/project-engine.ts": 12, "packages/engine/src/mission-execution-loop.ts": 10, @@ -30,6 +29,7 @@ "packages/cli/src/extension.ts": 5, "packages/core/src/task-store/merge-queue-ops-2.ts": 5, "packages/dashboard/app/hooks/useTaskDiffStats.ts": 5, + "packages/dashboard/src/routes/register-task-workflow-routes.ts": 5, "packages/engine/src/agent-tools.ts": 5, "packages/engine/src/merger.ts": 5, "packages/engine/src/restart-recovery-coordinator.ts": 5,