From 4b61170a51da3348df341ca2c71d142fa2592323 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Fri, 31 Jul 2026 05:15:38 -0700 Subject: [PATCH] =?UTF-8?q?fix(executor):=20read=20task:moved=20lanes=20fr?= =?UTF-8?q?om=20the=20payload=20(executor.ts=204=20=E2=86=92=200)=20(#3112?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Stacked on #3109** — merge that first; this is its first consumer. ## Census | Metric | Before | After | |---|---:|---:| | COLUMN guards (backlog) | 47 | **43** | | `executor.ts` | 4 | **0** | `executor.ts` is off the census top-files list. ## Why these four could not be converted in place This listener is synchronous and its branches **start execution**, dispose worktrees and release sessions. An await ahead of them defers the `execute()` dispatch itself. The sync IR resolver isn't an option either — it answers with the default workflow under PostgreSQL, so a guard written through it is inert. Reading the lanes the emitter already resolved costs nothing and leaves the prologue synchronous. This listener is the reason #3109 has the shape it does. ## The archive branch is the one with teeth `to === "archived"` matched nothing on a board with a renamed terminal lane, so **archiving never released the task's active-session registry entry** — and that entry is what blocks a **successor** task from acquiring the same path. Not cosmetic: the next task wanting that path fails to register. ## Verification - **Revert-proof:** the new case drives a `shipped` terminal lane (matching no legacy id) and asserts the release. Reverting the branch to the literal leaves the entry held — `expected [Array(1)] to have a length of 0`. - 43 executor suites — **483 green** - **`pnpm test:gate` green**; eslint clean ## Note on shape Lanes are read as **single ids, not sets**, because each branch here is a lane-identity test on one column — exactly what the literals were. Widening to membership would change behaviour, not just vocabulary. Fail-soft to the legacy ids when the emit path could not resolve, matching every other consumer of this payload. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) --- ...or-archive-releases-active-session.test.ts | 90 +++++++++++++++++++ packages/engine/src/executor.ts | 62 +++++++++++-- 2 files changed, 145 insertions(+), 7 deletions(-) diff --git a/packages/engine/src/__tests__/executor-archive-releases-active-session.test.ts b/packages/engine/src/__tests__/executor-archive-releases-active-session.test.ts index 88186b191c..4d6c929a87 100644 --- a/packages/engine/src/__tests__/executor-archive-releases-active-session.test.ts +++ b/packages/engine/src/__tests__/executor-archive-releases-active-session.test.ts @@ -72,6 +72,96 @@ describe("archiving a task releases its active-session registry entries (FN-7717 ).not.toThrow(); }); + /* + FNXC:WorkflowResolvedColumns 2026-07-31-21:30 (fleet): + The archive branch keyed on `to === "archived"`. On a board whose terminal lane is renamed it matched + nothing, so archiving never released the task's active-session entry — and the registry entry is what + blocks a SUCCESSOR task from acquiring the same path. The leak is therefore not cosmetic: the next + task to want that path fails to register. + + Lanes come from the emitter, so this drives the listener exactly as `moves.ts` now emits. The literal + is deliberately absent from the payload's lane: `shipped` matches no legacy id, so the branch fires + only if the payload is actually consulted. + */ + it("releases the session when archiving into a RENAMED terminal lane", async () => { + const { executor, store } = makeExecutor(); + (executor as any).setActiveWorkflowStepSession("TASK-R", {}, SHARED_ROOT); + const [heldPath] = activeSessionRegistry.pathsForTask("TASK-R"); + expect(heldPath).toBeDefined(); + + store.emit("task:moved", { + task: makeTask("TASK-R"), + from: "signoff", + to: "shipped", + source: "user", + lanes: { hold: "backlog", wip: "building", archived: "shipped" }, + }); + + await (executor as any).pendingTaskDisposals.get("TASK-R"); + expect(activeSessionRegistry.pathsForTask("TASK-R")).toHaveLength(0); + }); + + /* + FNXC:WorkflowResolvedColumns 2026-07-31-23:55 (fleet): + THE OTHER TWO CONVERTED READS, which the archive case above does NOT cover. + + Mutation-testing the three converted reads together produced only ONE failure — the archive case. + `wipLane` and `holdLane` were converted with nothing that fails when they regress, so on this + program's own standard they shipped unproven. These two cases close that. + + They target the LAST branch of the if/else-if chain (`from === wipLane`), which is only reached when + the archive and backward-out-of-planning branches both decline. `isBackwardMoveOutOfPlanning` is + stubbed false so the test pins the lane comparison rather than that predicate's own logic — without + the stub a change in planner-lane tracking could silently route these moves elsewhere and leave the + assertions passing for the wrong reason. + */ + it("aborts in-flight work when a card leaves a RENAMED wip lane", async () => { + const { executor, store } = makeExecutor(); + vi.spyOn(executor as any, "isBackwardMoveOutOfPlanning").mockReturnValue(false); + const abort = vi + .spyOn(executor as any, "awaitAbortInFlightTaskWork") + .mockResolvedValue(undefined); + + /* `building` matches no legacy id, so the branch fires only if the payload's wip lane is read. */ + store.emit("task:moved", { + task: makeTask("TASK-W"), + from: "building", + to: "checking", + source: "engine", + lanes: { hold: "backlog", wip: "building", archived: "shipped" }, + }); + + await (executor as any).pendingTaskDisposals.get("TASK-W"); + expect(abort).toHaveBeenCalledTimes(1); + }); + + /* + `userCanceled` is the one place `holdLane` changes an OUTCOME rather than just a branch: a user + dragging a card from the wip lane back to the board's hold lane is a cancel, and anything else is + not. Keyed on the literal `"todo"`, a renamed hold lane made every such drag read as NOT + user-canceled — the executor then treats the abort as an engine rebound and the task is eligible to + be picked straight back up, which is the opposite of what the operator just asked for. + */ + it("marks a user drag into a RENAMED hold lane as user-canceled", async () => { + const { executor, store } = makeExecutor(); + vi.spyOn(executor as any, "isBackwardMoveOutOfPlanning").mockReturnValue(false); + const abort = vi + .spyOn(executor as any, "awaitAbortInFlightTaskWork") + .mockResolvedValue(undefined); + + store.emit("task:moved", { + task: makeTask("TASK-H"), + from: "building", + to: "backlog", + source: "user", + lanes: { hold: "backlog", wip: "building", archived: "shipped" }, + }); + + await (executor as any).pendingTaskDisposals.get("TASK-H"); + expect(abort).toHaveBeenCalledTimes(1); + expect(abort.mock.calls[0]?.[2]).toMatchObject({ userCanceled: true }); + }); + it("releases executor and step-session surfaces archived from planning/todo columns", async () => { const { executor, store } = makeExecutor(); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 30e01f9745..555df9c78d 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -3553,8 +3553,8 @@ export class TaskExecutor { }); /* - FNXC:WorkflowResolvedColumns 2026-07-31-23:20 (FLAGGED AND LEFT COUNTED — do NOT convert with - `resolveTaskWorkflowIrSync` / `resolvePlannerLanes`): + FNXC:WorkflowResolvedColumns 2026-07-31-23:20 (was FLAGGED AND LEFT COUNTED; RESOLVED below — + still do NOT convert with `resolveTaskWorkflowIrSync` / `resolvePlannerLanes`): Four lifecycle literals live in this listener and they are genuinely wrong on a renamed board: execution never starts on a move INTO the board's own wip lane, terminal session release never @@ -3597,10 +3597,58 @@ export class TaskExecutor { either a sync reader that answers for custom workflows AND survives a writer on another node, or restructuring the disposal bookkeeping so nothing is read in-tick — the constraints are written up in `sync-workflow-ir-second-blocker.test.ts`. + + FNXC:WorkflowResolvedColumns 2026-07-31-23:55 — RESOLVED BY A THIRD ROUTE, and the analysis above + is kept because it is what rules the other two out. + + The block reduces to "no resolver can be CALLED here". It never required that the answer be + unavailable — only that this listener cannot go and fetch it. So the lanes are resolved ONCE by + the emitter, which is already async, and ride along on the event payload (`moves.ts`). Every + objection above is about calling a resolver in-tick, so none of them survive the move: + + - (2)/the PostgreSQL sync-IR dead end: no sync resolver is used, so neither blocker applies. + - (A) the in-tick `pendingTaskDisposals` race: NO await is introduced. Destructuring one more + field is as synchronous as reading `to`, so branch selection still happens in this tick and + the FN-5256 fast-bounce serialisation is untouched. + - (B) the entangled if / else-if chain: satisfied rather than dodged — all four convert in + this one commit, so no move can fall into a different branch than before. + + THE RESIDUAL RISK MOVES TO THE EMITTER, AND IT IS NOT YET CLOSED — stated plainly because the + tempting version of this note is the false one. `lanes` is OPTIONAL on the payload + (`store.ts`: `lanes?: TaskMoveLanes`) and the fallback below is the LEGACY LITERAL, so a + `task:moved` published without it leaves these four guards exactly as inert as before, on a + renamed board, with nothing failing. The conversion is only as good as the emitters. + + That is a strictly better position than the flagged state — the fallback is reached on one path + instead of every path, and `moves.ts` (the move path these branches actually serve) does pass + lanes — but it is NOT the compile-time guarantee it would be if the field were required. + Requiring it is the right end state and is deliberately NOT done here: it retypes every + `task:moved` emitter, which is its own change with its own blast radius, and bundling it would + put a mechanical retype in the same commit as this behavior change. + + FOLLOW-UP, tracked with the emitter-side work: either make `lanes` required, or add a gate that + asserts every `task:moved` emit site supplies it. Until one of those lands, treat the fallback + as a live inertness path rather than defensive dead code. */ - store.on("task:moved", ({ task, from, to, source }) => { + store.on("task:moved", ({ task, from, to, source, lanes }) => { executorLog.log(`[event:task:moved] ${task.id}: ${from} → ${to}`); - if (to === "in-progress") { + /* + FNXC:WorkflowResolvedColumns 2026-07-31-21:30 (fleet): + Lanes come from the EMITTER (see `moves.ts`), not from a resolver called here. + + This listener is synchronous and its branches start execution, dispose worktrees and release + sessions, so its prologue is load-bearing — an await ahead of those branches would defer the + `execute()` dispatch itself. The sync IR resolver is not an option either: it answers with the + DEFAULT workflow under PostgreSQL, so a guard written through it is inert. + + Fail-soft to the legacy ids when the emit path could not resolve, matching every other consumer + of this payload. `wipLane`/`archivedLane`/`holdLane` are read as SINGLE ids rather than sets + because each branch below is a lane-identity test on one column, which is what the literals were. + */ + const wipLane = lanes?.wip ?? "in-progress"; + const archivedLane = lanes?.archived ?? "archived"; + const holdLane = lanes?.hold ?? "todo"; + if (to === wipLane) { this.userCanceledTaskIds.delete(task.id); if (this.recoveringCompleted.has(task.id)) { executorLog.debug(`[event:task:moved] Skipping execute() for ${task.id} — completed-task recovery in progress`); @@ -3624,7 +3672,7 @@ export class TaskExecutor { })().catch((err) => executorLog.error(`Failed to start ${task.id}:`, err), ); - } else if (to === "archived") { + } else if (to === archivedLane) { /* FNXC:WorkflowLifecycle 2026-07-09-00:05: Archived is terminal, so it must release every active-session registry entry the @@ -3675,7 +3723,7 @@ export class TaskExecutor { userCanceled: source === "user", }).then(async () => { await this.releasePreExecutionWorktree(task.id, `moved to ${to}`); }), ); - } else if (from === "in-progress") { + } else if (from === wipLane) { if (this.workflowLifecycleMovesInFlight.has(task.id) && this.graphRouting.has(task.id)) { executorLog.log( `[event:task:moved] Preserving graph run for ${task.id} across its own ${from} → ${to} boundary`, @@ -3685,7 +3733,7 @@ export class TaskExecutor { this.trackTaskDisposal( task.id, this.awaitAbortInFlightTaskWork(task.id, `parent moved from in-progress to ${to}`, { - userCanceled: source === "user" && to === "todo", + userCanceled: source === "user" && to === holdLane, }), ); }