Phase B slice B3.1 (U4): resolve the hold column in recoverStrandedCompletedTodoTasks — query and guard together (#2472)

Stacked on #2471 (Phase B slice B2). Base is `feature/workflow-vocabulary-b2` — do not merge before it. **First landable slice of U4 (self-healing.ts).** One sweep, one PR, per the phase's sub-split rule. ## The finding: the guard and the query must convert together `recoverStrandedCompletedTodoTasks` promotes a card whose steps are all done/skipped but which is still sitting in the hold column — finished work that never handed off to review. It decided *"is this card in the hold column?"* **twice**, and both were literal: | | was | |---|---| | the QUERY | `listTasks({ column: "todo", slim: true })` | | the GUARD | `task.column !== "todo"` | **Either half alone is a green diff with zero behavior change.** A correct guard behind a literal query never runs; a converted query behind a literal guard rejects every row it just fetched. This is the shape that made B1's stale-paused-todo fix cosmetic, and the phase brief predicted more of it here — correctly. I proved it rather than asserting it: - literal **QUERY** restored (converted guard kept) → **3 tests fail** - literal **GUARD** restored (converted query kept) → **2 tests fail** Neither half passes the suite alone. ## Falsification came first Per the brief I tried to prove the work unnecessary before doing it. It is necessary, and the evidence is empirical, not assumed: the 7 tests were written against unmodified code and 3 failed. Unlike B2's hold-release — which turned out already converted — **self-healing is uniformly unconverted at the query level**: 53 of its sweeps carry a hardcoded `column:` filter (survey in the worker report). ## Negative half, per the brief A completed card resting in a WIP or review column is **not** promoted. Dropping a column filter without a per-task hold check would promote finished cards out of every column — laundering work past review, a louder bug than the silent one being fixed. ## Test-harness hazard (will recur in every remaining U4 slice) The pre-existing self-healing store mock returns its fixture from `listTasks` **regardless of arguments**. A renamed-hold test on that harness passes while the query stays hardcoded, because the mock hands the sweep rows the real store never would. The new harness **honors** the column filter, and one test asserts the query is no longer scoped to the literal. This is documented in the new file's header for whoever writes the next slice. ## Cost The column filter is gone, so the cheap non-column rejections (paused / executing / incomplete steps / errored / no-commits / skip-bypass taint) run **first and synchronously**; only survivors pay an IR resolution, shared through an `irCache`. A board spanning three workflows resolves three IRs regardless of card count. `includeArchived: false` preserves what the column filter did implicitly. The hold column resolves **per task** — a board spans workflows, and a card in *another* workflow's hold column must not be promoted. ## One pre-existing assertion changed, deliberately `self-healing.test.ts` pinned `listTasks` being called with `{ column: "todo", slim: true }`. That query shape changed on purpose; the assertion now pins the new one. The behavioral assertions either side of it (one qualifying card, promoted exactly once) are untouched and still pass. ## Carried A3 questions — both answered **Q1 — does the sync/SQLite counter have the same pool-id mismatch?** **Not applicable: there is no sync counter.** `occupantsByColumnForWorkflowImpl` and `listWorkflowOccupantTaskIds` are async/PG-only and throw without an initialized `AsyncDataLayer`; the sync twin went with the PG cutover. There is no second counter that could mismatch. The surviving pool-id sentinel sites are `project-store-ops.ts:767/819` and `moves.ts` — both parked by operator decision, untouched here. **Q2 — are custom workflows with an explicit numeric limit affected?** **No, by design.** `resolveColumnCapacity` gives `config.limit` top precedence (`configLimit` → `limitSetting` → default-workflow read-through → `Infinity`), and `resolveWipBudgetColumns` documents that a column with an explicit numeric limit is **independent — its budget is itself alone**. Such a column never pools, so there is no pool id to mismatch. Read-only analysis; no code changed for either question. ## Remaining U4 scope (not in this PR) 214 literal occurrences across ~70 methods; **53 sweeps carry a query-level column filter**. Hold-gated sweeps still to convert: `clearStaleBlockedBy`, `reclaimSelfOwnedBranchConflicts`, `reconcileCompletedTask`, `recoverMergedReviewTasks`, `recoverStuckMergeDeadlocks`, plus non-query `todo` guards in `recoverPausedAbortFailures`, `reconcileDependencyBlockingLeases`, and others. `surfaceStalePausedTodos` was already converted (B1 follow-up) and is verified intact on this branch. ## Verification - 7 new tests green; **both mutations kill the suite** - self-healing suite: 415 passed, **1 failure pre-existing** (`archiveStaleDoneTasks` — confirmed identical by stashing my changes) - merge gate green (299 + 10 + 71) - `tsc --noEmit` clean, `pnpm lint` clean No changeset: `@fusion/engine` is private. 🤖 Generated with [Claude Code](https://claude.com/claude-code)  ## Summary by CodeRabbit * **Bug Fixes** * Improved recovery of completed tasks stranded in workflow-specific hold columns, including renamed hold columns. * Preserved recovery for built-in workflows while correctly handling boards with mixed workflow configurations. * Prevented recovery for tasks in non-hold columns or with paused, incomplete, or errored states. * Added fallback handling when workflow details cannot be resolved.
2026-07-27 14:36:32 -07:00
parent 02b0f4f860
commit a4dee1162e
3 changed files with 270 additions and 5 deletions
--- a/packages/engine/src/tests/self-healing-stranded-todo-renamed-hold.test.ts
+++ b/packages/engine/src/tests/self-healing-stranded-todo-renamed-hold.test.ts
@@ -0,0 +1,216 @@
+/*
+FNXC:WorkflowLifecycleColumns 2026-07-28-06:10 (Phase B / slice B3.1 — U4):
+
+`recoverStrandedCompletedTodoTasks` promotes a card whose steps are all
+done/skipped but which is still sitting in the HOLD column — work that finished
+and never handed off to review. It decides "is this card in the hold column?"
+TWICE, and both were literal:
+
+  1. the QUERY — `listTasks({ column: "todo", slim: true })`
+  2. the per-task GUARD — `task.column !== "todo"`
+
+Both must convert together. Converting only the guard leaves a correct predicate
+the sweep never reaches, because the query already returned an empty list;
+converting only the query leaves the guard rejecting every row it just fetched.
+Either half alone is a green diff with zero behavior change — the exact shape
+that made B1's stale-paused-todo fix cosmetic.
+
+TEST-HARNESS WARNING, load-bearing. The pre-existing `self-healing.test.ts` store
+mock returns its fixture from `listTasks` REGARDLESS of arguments. A renamed-hold
+test written on that harness passes while the query stays hardcoded, because the
+mock hands the sweep rows the real store never would. The mock below therefore
+HONORS `options.column`, and one test asserts the query is no longer scoped to
+the literal. Do not "simplify" this mock.
+*/
+import { describe, expect, it, vi } from "vitest";
+import type { Task, TaskStore, WorkflowIr } from "@fusion/core";
+
+import { SelfHealingManager } from "../self-healing.js";
+
+const WF = "custom:wf";
+
+/** A card whose steps are all done — stranded, awaiting promotion to review. */
+function strandedTask(over: Partial<Task> = {}): Task {
+  return {
+    id: "FN-1",
+    title: "t",
+    description: "",
+    column: "todo",
+    paused: false,
+    dependencies: [],
+    steps: [{ name: "s1", status: "done" }],
+    currentStep: 1,
+    log: [],
+    createdAt: "2026-01-01T00:00:00.000Z",
+    columnMovedAt: "2026-01-01T00:00:00.000Z",
+    updatedAt: "2026-01-01T00:00:00.000Z",
+    ...over,
+  } as unknown as Task;
+}
+
+function ir(holdId: string): WorkflowIr {
+  return {
+    version: "v2",
+    id: WF,
+    nodes: [],
+    edges: [],
+    columns: [
+      { id: holdId, label: "Hold", traits: [{ trait: "hold", config: { release: "capacity" } }] },
+      { id: "building", label: "Building", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] },
+      { id: "reviewing", label: "Reviewing", traits: [{ trait: "mergeOrchestration" }] },
+      { id: "shipped", label: "Shipped", traits: [{ trait: "complete" }] },
+    ],
+  } as unknown as WorkflowIr;
+}
+
+function harness(tasks: Task[], workflowIr: WorkflowIr | undefined) {
+  const recovered: string[] = [];
+  const selection = { workflowId: WF, stepIds: [] };
+  const listTasks = vi.fn(async (opts?: { column?: string }) =>
+    /* HONORS the column filter — see the file header. */
+    opts?.column ? tasks.filter((t) => t.column === opts.column) : tasks,
+  );
+  const store = {
+    listTasks,
+    getTask: vi.fn(async (id: string) => tasks.find((t) => t.id === id) ?? null),
+    getSettings: vi.fn(async () => ({})),
+    logEntry: vi.fn(async () => undefined),
+    recordRunAuditEvent: vi.fn(async () => undefined),
+    getTaskWorkflowSelection: vi.fn(() => selection),
+    getTaskWorkflowSelectionAsync: vi.fn(async () => selection),
+    getWorkflowDefinition: vi.fn(async () => (workflowIr ? { ir: workflowIr } : null)),
+  } as unknown as TaskStore;
+
+  const manager = new SelfHealingManager(store, {
+    rootDir: "/tmp/test-project",
+    recoverCompletedTask: async (task: Task) => {
+      recovered.push(task.id);
+      return true;
+    },
+  } as never);
+
+  return { manager, store, listTasks, recovered };
+}
+
+describe("recoverStrandedCompletedTodoTasks under a renamed hold column", () => {
+  it("recovers a stranded completed card resting in a RENAMED hold column", async () => {
+    /* The whole point: this card finished its work and is stuck. Under the
+       literal query+guard the sweep does not see it at all, so it sits in
+       `drafting` forever with no error and no failing test. */
+    const task = strandedTask({ id: "FN-R", column: "drafting" });
+    const h = harness([task], ir("drafting"));
+
+    const count = await h.manager.recoverStrandedCompletedTodoTasks();
+
+    expect(count).toBe(1);
+    expect(h.recovered).toEqual(["FN-R"]);
+  });
+
+  it("does not scope its query to the literal todo column", async () => {
+    /* Pins the QUERY half directly. A correct guard behind a `column: "todo"`
+       query is a guard that never runs — the conversion would be cosmetic. */
+    const task = strandedTask({ id: "FN-R", column: "drafting" });
+    const h = harness([task], ir("drafting"));
+
+    await h.manager.recoverStrandedCompletedTodoTasks();
+
+    const columnArgs = h.listTasks.mock.calls.map(
+      (call) => (call[0] as { column?: string } | undefined)?.column,
+    );
+    expect(columnArgs).not.toContain("todo");
+  });
+
+  it("does NOT recover a completed card resting in a NON-hold column", async () => {
+    /*
+    The negative half the phase brief requires. Dropping the column filter
+    without a per-task hold check would promote finished cards out of the WIP and
+    review columns too — a louder bug than the silent one it replaces, and one
+    that would launder work past review.
+    */
+    const wip = strandedTask({ id: "FN-W", column: "building" });
+    const review = strandedTask({ id: "FN-V", column: "reviewing" });
+    const h = harness([wip, review], ir("drafting"));
+
+    const count = await h.manager.recoverStrandedCompletedTodoTasks();
+
+    expect(count).toBe(0);
+    expect(h.recovered).toEqual([]);
+  });
+
+  it("still recovers a builtin todo card (regression floor)", async () => {
+    const task = strandedTask({ id: "FN-D", column: "todo" });
+    const h = harness([task], ir("todo"));
+
+    expect(await h.manager.recoverStrandedCompletedTodoTasks()).toBe(1);
+    expect(h.recovered).toEqual(["FN-D"]);
+  });
+
+  it("falls back to the legacy todo column when the workflow cannot be resolved", async () => {
+    /* Conservative: an unresolvable workflow must behave exactly as it did
+       before this conversion rather than guessing. */
+    const task = strandedTask({ id: "FN-U", column: "todo" });
+    const h = harness([task], undefined);
+
+    expect(await h.manager.recoverStrandedCompletedTodoTasks()).toBe(1);
+  });
+
+  it("handles a board mixing a renamed and a builtin workflow", async () => {
+    /* Per-task resolution, not one board-wide vocabulary: each card's hold
+       column comes from ITS OWN workflow, and a card in the OTHER workflow's
+       hold column must not be promoted. */
+    const renamed = strandedTask({ id: "FN-R", column: "drafting" });
+    const legacy = strandedTask({ id: "FN-D", column: "todo" });
+    // FN-R belongs to the renamed workflow; FN-D to the builtin one.
+    const irByWorkflow: Record<string, WorkflowIr> = {
+      "wf-renamed": ir("drafting"),
+      "wf-legacy": ir("todo"),
+    };
+    const byTask: Record<string, string> = { "FN-R": "wf-renamed", "FN-D": "wf-legacy" };
+    const tasks = [renamed, legacy];
+    const recovered: string[] = [];
+    const store = {
+      listTasks: vi.fn(async (opts?: { column?: string }) =>
+        opts?.column ? tasks.filter((t) => t.column === opts.column) : tasks,
+      ),
+      getTask: vi.fn(async (id: string) => tasks.find((t) => t.id === id) ?? null),
+      getSettings: vi.fn(async () => ({})),
+      logEntry: vi.fn(async () => undefined),
+      recordRunAuditEvent: vi.fn(async () => undefined),
+      getTaskWorkflowSelection: vi.fn((id: string) => ({ workflowId: byTask[id], stepIds: [] })),
+      getTaskWorkflowSelectionAsync: vi.fn(async (id: string) => ({ workflowId: byTask[id], stepIds: [] })),
+      getWorkflowDefinition: vi.fn(async (id: string) => (irByWorkflow[id] ? { ir: irByWorkflow[id] } : null)),
+    } as unknown as TaskStore;
+    const manager = new SelfHealingManager(store, {
+      rootDir: "/tmp/test-project",
+      recoverCompletedTask: async (task: Task) => {
+        recovered.push(task.id);
+        return true;
+      },
+    } as never);
+
+    const count = await manager.recoverStrandedCompletedTodoTasks();
+
+    expect(count).toBe(2);
+    expect(recovered.sort()).toEqual(["FN-D", "FN-R"]);
+  });
+
+  it("leaves the existing non-column guards intact under a renamed workflow", async () => {
+    /*
+    The conversion must not widen the sweep. These three rejections
+    (paused / incomplete steps / errored) are the ones most likely to be lost
+    when a filter is rewritten, and each independently protects against
+    promoting work that is not actually finished.
+    */
+    const paused = strandedTask({ id: "FN-P", column: "drafting", paused: true });
+    const incomplete = strandedTask({
+      id: "FN-I",
+      column: "drafting",
+      steps: [{ name: "s1", status: "pending" }],
+    } as Partial<Task>);
+    const errored = strandedTask({ id: "FN-E", column: "drafting", error: "boom" } as Partial<Task>);
+    const h = harness([paused, incomplete, errored], ir("drafting"));
+
+    expect(await h.manager.recoverStrandedCompletedTodoTasks()).toBe(0);
+    expect(h.recovered).toEqual([]);
+  });
+});
--- a/packages/engine/src/tests/self-healing.test.ts
+++ b/packages/engine/src/tests/self-healing.test.ts
@@ -3035,7 +3035,17 @@ describe("SelfHealingManager", () => {
      const result = await managerWithRecovery.recoverStrandedCompletedTodoTasks();

      expect(result).toBe(1);
-      expect(store.listTasks).toHaveBeenCalledWith({ column: "todo", slim: true });
+      /*
+      FNXC:WorkflowLifecycleColumns 2026-07-28-06:40 (Phase B / slice B3.1 — U4):
+      The query shape CHANGED on purpose. This sweep can no longer scope itself to
+      `column: "todo"` — that literal made it blind to every workflow whose hold
+      column is named something else, so a finished card sat in `drafting` forever.
+      It now reads the board and filters by each task's RESOLVED hold column.
+
+      The behavioral assertions below are the ones that matter and are unchanged:
+      one qualifying card, promoted exactly once. Only the query shape moved.
+      */
+      expect(store.listTasks).toHaveBeenCalledWith({ slim: true, includeArchived: false });
      expect(recoverFn).toHaveBeenCalledTimes(1);
      expect(recoverFn).toHaveBeenCalledWith(expect.objectContaining({ id: "FN-101" }));

--- a/packages/engine/src/self-healing.ts
+++ b/packages/engine/src/self-healing.ts
@@ -3209,11 +3209,27 @@ export class SelfHealingManager {
    if (!recoverFn) return 0;

    try {
-      const tasks = await this.store.listTasks({ column: "todo", slim: true });
+      /*
+      FNXC:WorkflowLifecycleColumns 2026-07-28-06:25 (Phase B / slice B3.1 — U4):
+      This sweep decided "is this card in the hold column?" TWICE — the QUERY
+      (`{ column: "todo" }`) and the per-task GUARD (`task.column !== "todo"`) —
+      and both were literal. They convert TOGETHER or not at all: a correct guard
+      behind a literal query never runs, and a converted query behind a literal
+      guard rejects every row it just fetched. Either half alone is a green diff
+      with no behavior change.
+
+      Cost discipline: the column filter is gone, so the CHEAP non-column
+      rejections (paused / executing / incomplete steps / errored / taint) run
+      FIRST and synchronously, and only the handful of survivors pay an IR
+      resolution — shared through `irCache`, so a board spanning three workflows
+      resolves three times regardless of card count. `includeArchived: false`
+      keeps archived rows out, which the old column filter did implicitly.
+      */
+      const tasks = await this.store.listTasks({ slim: true, includeArchived: false });
      const executingIds = this.options.getExecutingTaskIds?.() ?? new Set<string>();

-      const stranded = tasks.filter((task) => {
-        if (task.column !== "todo" || task.paused) return false;
+      const completedNonColumnCandidates = tasks.filter((task) => {
+        if (task.paused) return false;
        if (executingIds.has(task.id)) return false;
        if (task.steps.length === 0 || !task.steps.every((s) => s.status === "done" || s.status === "skipped")) return false;
        /*
@@ -3235,9 +3251,32 @@ export class SelfHealingManager {
        return true;
      });

+      /*
+      FNXC:WorkflowLifecycleColumns 2026-07-28-06:25 (Phase B / slice B3.1 — U4):
+      The column half of the old predicate, now resolved per task against the
+      card's OWN workflow. A board can span workflows with different hold
+      columns, so this cannot be hoisted to one board-wide value. A workflow that
+      declares no hold column keeps the legacy `todo`, matching the pre-conversion
+      behavior rather than matching nothing.
+      */
+      const irCache = new Map<string, Awaited<ReturnType<typeof resolveWorkflowIrForTask>>>();
+      const stranded: Task[] = [];
+      for (const task of completedNonColumnCandidates) {
+        let holdColumn = "todo";
+        try {
+          const lifecycle = resolveLifecycleColumns(
+            await resolveWorkflowIrForTask(this.store, task.id, irCache),
+          );
+          if (lifecycle?.hold) holdColumn = lifecycle.hold;
+        } catch {
+          holdColumn = "todo";
+        }
+        if (task.column === holdColumn) stranded.push(task);
+      }
+
      if (stranded.length === 0) return 0;

-      log.warn(`Found ${stranded.length} completed task(s) stranded in todo`);
+      log.warn(`Found ${stranded.length} completed task(s) stranded in the hold column`);

      let recovered = 0;
      for (const task of stranded) {