From a4dee1162e8e5cff089342115f0ce04a89c97947 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Mon, 27 Jul 2026 14:36:32 -0700 Subject: [PATCH] =?UTF-8?q?Phase=20B=20slice=20B3.1=20(U4):=20resolve=20th?= =?UTF-8?q?e=20hold=20column=20in=20recoverStrandedCompletedTodoTasks=20?= =?UTF-8?q?=E2=80=94=20query=20and=20guard=20together=20(#2472)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- ...healing-stranded-todo-renamed-hold.test.ts | 216 ++++++++++++++++++ .../engine/src/__tests__/self-healing.test.ts | 12 +- packages/engine/src/self-healing.ts | 47 +++- 3 files changed, 270 insertions(+), 5 deletions(-) create mode 100644 packages/engine/src/__tests__/self-healing-stranded-todo-renamed-hold.test.ts diff --git 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 new file mode 100644 index 0000000000..07bcdc805a --- /dev/null +++ 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 { + 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 = { + "wf-renamed": ir("drafting"), + "wf-legacy": ir("todo"), + }; + const byTask: Record = { "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); + const errored = strandedTask({ id: "FN-E", column: "drafting", error: "boom" } as Partial); + const h = harness([paused, incomplete, errored], ir("drafting")); + + expect(await h.manager.recoverStrandedCompletedTodoTasks()).toBe(0); + expect(h.recovered).toEqual([]); + }); +}); diff --git a/packages/engine/src/__tests__/self-healing.test.ts b/packages/engine/src/__tests__/self-healing.test.ts index c0eaf5f062..5c459ba41c 100644 --- 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" })); diff --git a/packages/engine/src/self-healing.ts b/packages/engine/src/self-healing.ts index 10e201dc8e..229cc22ebd 100644 --- 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(); - 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>>(); + 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) {