root/fusion
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(executor): read task:moved lanes from the payload (executor.ts 4 → 0) (#3112)

Browse Source 
**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) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-07-31 05:15:38 -07:00
committed by GitHub
parent 218086bea2
commit 4b61170a51
2 changed files with 145 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files

90
packages/engine/src/__tests__/executor-archive-releases-active-session.test.ts
View File

@@ -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();

62
packages/engine/src/executor.ts
View File

@@ -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,
}),
);
}