fleet: register-task-workflow-routes.ts 20 -> 5 (#2713)

## Census before / after | | before | after | |---|---:|---:| | `register-task-workflow-routes.ts` | **20** | **5** | I handed this cluster off in an earlier turn with an analysis rather than a conversion. Coming back to it with the analysis already done made it tractable. ## Converted with the file's own idiom This file already had `resolveIntakeColumnForTask` / `resolveWipColumnForTask` / `resolveReboundColumnForTask`: resolve the column **id** from the task's workflow, fall back to the legacy id when the IR cannot be read. I added the three roles it was missing — review, complete, archived — in that same shape. **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. Every converted handler **resolves once and reuses**, so two checks in the same request cannot disagree about which column is the review lane. The retry handler had three separate review checks and now shares one resolution. ## Also fixes an inversion `isArchived` ORed the legacy id with the resolved trait **unconditionally**, so a column merely *named* `archived` counted as archived even when its own workflow said otherwise. Same pattern previously found in `TaskContextMenu` and `isPreExecutionHoldColumn`. Now flags-first, id as fallback. ## The five survivors, each with a reason | count | line | why | |---:|---|---| | 1 | 1193 | `tasks.filter(t => t.column === "todo")` — a **list** path. Per-task IR resolution is N store reads on board load; the site already carries a note measuring that cost. Needs the hold column resolved per *workflow* from the board payload — a real change, not a rename. | | 1 | 1926 | **Already flags-first**: `moveTargetIr && declaresColumns ? columnHasFlag(...) : column === "in-progress"`. The literal *is* the documented no-IR fallback. | | 1 | 4872 | The fallback arm of the `isArchived` fix above — same shape, deliberately kept. | | 2 | 4024 | `depTask.column` — a **dependency's** column, not the task's. Resolving it means fetching that task's IR per dependency; out of scope. | So three of the five are correct as they stand, and two are genuinely deferred. ## Verification `plan-approval-intake-column` + `stranded-refinements-routes` 13/13. `pnpm test:gate` green (10 / 158 / 487 / 71). `pnpm check:lifecycle-columns` exits 0 with the baseline re-recorded here. `tsc -p packages/dashboard/tsconfig.json` clean. `pnpm lint` clean. Typecheck was run after **every** batch rather than once at the end — with 15 edits across async handlers in a 6000-line file, a single late typecheck would not tell me which batch broke it. No changeset: no user-visible change. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-30 04:17:03 -07:00
parent dbb53eebaa
commit a59c6aea50
3 changed files with 165 additions and 16 deletions
--- 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 }),
--- 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<Set<string>> {
+  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<Set<string>> {
+  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<InReviewUserCommentReengagementResult> {
-    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],
--- 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,