fleet: moves.ts 15 → 2 (hot path; one hoisted resolution, net one FEWER than before) (#2705)
Claiming **`packages/core/src/task-store/moves.ts`** — 15 guards, largest unclaimed. This is the core move path, every task move in the system, so the conversion is built to add **no work** to it. ## Census before/after | | before | after | |---|---:|---:| | `moves.ts` column guards | **15** | **2** | | repo backlog (this branch vs `origin/main`) | 693 | **679** | Baseline re-recorded; `--strict` exits 0. *(Backlog figures don't compose across my open fleet PRs — each branch carries only its own reductions. 693 → 679 is this branch against main as measured, not a running total.)* ## Zero added cost, and actually one fewer resolution than before `moveTaskInternal` **already** resolves the workflow IR unconditionally at line 400 — the `useWorkflow` gate is gone — and already derived a lifecycle from it ~400 lines later for the trait hooks. So one hoisted `moveLifecycle` immediately after the IR resolution serves all 14 guards, and the later local now **aliases** it instead of resolving a second time. Net effect on the hot path: **one fewer `resolveLifecycleColumns` call than before this PR.** ## Converted: 14 6× `toColumn === "done"` → complete · 4× `fromColumn === "in-review"` → review · 1× `toColumn === "in-review"` → review · 2× `toColumn === "todo"` → hold · 1× `toColumn === "in-progress"` → wip Every site keeps its legacy id as the fallback. `undefined` here means no IR on this path or a v1 column-less IR, and a move must behave **exactly** as before when there is no basis to resolve from — this is the transaction that arbitrates capacity, so "unchanged when unresolvable" is the requirement, not a nicety. ## Flagged, not converted: 1 **Line 309** — `task.column === "archived"` in the handoff-invariant check. It sits in a different function that runs **before** any IR resolution, so converting it would mean *adding* a resolution to a path that currently has none. That is a cost on the handoff path rather than the free reuse everything else here gets, so it wants a deliberate decision rather than my inclusion. ## Verification — the paths that matter, not just typecheck Because this is the move transaction, `tsc` + lint is not sufficient evidence: - **`pnpm test:gate` GREEN** — 158 + 10 + 487 + 71 - `workflow-capacity-invariant` + `move-path-equivalence` **7/7** (the in-transaction capacity gate lives in this file) - `handoff-to-review-atomicity` **4/4** - `store-movement` + `move-task-preserve-status` + `task-move-hard-cancel-ordering` + `transition-pending-and-status-clear` **16/16** - `pnpm lint` clean · core `tsc` clean · `--strict` exits 0 🤖 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
@@ -0,0 +1,106 @@
|
||||
/*
|
||||
FNXC:WorkflowTaskCancellation 2026-07-30-23:20 (PR #2705 review — greptile):
|
||||
|
||||
THE OPERATOR HARD CANCEL MUST FIRE ON A WORKFLOW THAT HAS NO HOLD COLUMN.
|
||||
|
||||
`moveTaskInternal`'s two hard-cancel guards resolved their target as
|
||||
`moveLifecycle?.hold ?? "todo"`. A workflow may legitimately declare an INTAKE column and no hold
|
||||
column — Coding (Ideas)'s `ideas` lane is the shipped example — and then `"todo"` names a column
|
||||
that workflow does not declare. The comparison never matches, so the card moves while its merge
|
||||
request stays live and its active work items are never cancelled.
|
||||
|
||||
That is a cancellation contract failing OPEN, which is the worst direction available: the operator
|
||||
sees the card parked and believes the work stopped.
|
||||
|
||||
WHY A LIVE STORE. The bug is in which column id the guard compares against, and that id comes from
|
||||
the task's own persisted workflow. A mock that hands back a lifecycle struct would be asserting my
|
||||
own assumption about what `resolveLifecycleColumns` returns for an intake-only lineage — the exact
|
||||
substitution that has produced vacuous tests all through this program. This drives a real
|
||||
PostgreSQL store and a real workflow definition, and asserts on OBSERVED PERSISTED STATE.
|
||||
|
||||
WHAT IS ASSERTED. The completion-handoff marker is cleared only inside the guarded block, so it is
|
||||
a precise witness that the block ran. Reverting the production fallback to `?? "todo"` leaves the
|
||||
marker in place and fails this test.
|
||||
|
||||
LANE. `.pg.test.ts`, skipped by `pgDescribe` when no PostgreSQL is reachable, so the merge gate is
|
||||
unaffected. Throwaway per-file database; never port 4040.
|
||||
*/
|
||||
import { beforeAll, beforeEach, afterEach, afterAll, expect, it } from "vitest";
|
||||
import "@fusion/core"; // registers the built-in column traits
|
||||
|
||||
import { pgDescribe, createSharedPgTaskStoreTestHarness } from "../../__test-utils__/pg-test-harness.js";
|
||||
|
||||
pgDescribe("operator hard cancel on a workflow with an intake lane and no hold lane", () => {
|
||||
const harness = createSharedPgTaskStoreTestHarness({ prefix: "fusion_moves_intake_cancel" });
|
||||
|
||||
beforeAll(harness.beforeAll);
|
||||
afterAll(harness.afterAll);
|
||||
beforeEach(async () => { await harness.beforeEach(); });
|
||||
afterEach(async () => { await harness.afterEach(); });
|
||||
|
||||
/** Intake, wip, review, complete — deliberately NO hold column, and no legacy ids. */
|
||||
async function intakeOnlyWorkflow(store: ReturnType<typeof harness.store>) {
|
||||
return store.createWorkflowDefinition({
|
||||
name: "intake-only",
|
||||
ir: {
|
||||
version: "v2",
|
||||
name: "intake-only",
|
||||
columns: [
|
||||
{ id: "inbox", name: "Inbox", traits: [{ trait: "intake" }] },
|
||||
{ id: "signoff", name: "Signoff", traits: [{ trait: "merge" }] },
|
||||
{ id: "building", name: "Building", traits: [{ trait: "wip" }] },
|
||||
{ id: "shipped", name: "Shipped", traits: [{ trait: "complete" }] },
|
||||
],
|
||||
nodes: [{ id: "start", kind: "start", column: "inbox" }, { id: "end", kind: "end", column: "shipped" }],
|
||||
edges: [{ from: "start", to: "end" }],
|
||||
},
|
||||
} as never);
|
||||
}
|
||||
|
||||
it("clears the completion-handoff marker when the operator parks a review card in the INTAKE lane", async () => {
|
||||
const store = harness.store();
|
||||
const definition = await intakeOnlyWorkflow(store);
|
||||
const task = await store.createTask({ description: "intake-only card", workflowId: definition.id } as never);
|
||||
|
||||
await store.moveTask(task.id, "signoff" as never, { bypassGuards: true } as never);
|
||||
|
||||
await store.setCompletionHandoffAcceptedMarker(task.id, { source: "test" });
|
||||
expect(await store.getCompletionHandoffAcceptedMarker(task.id)).not.toBeNull();
|
||||
|
||||
/*
|
||||
The operator hard cancel: review -> the pre-implementation lane, by USER.
|
||||
|
||||
`bypassGuards` is here because this lineage declares no edge back from `signoff`, and that
|
||||
ADJACENCY question is a different one from the side-effect contract under test. Without it the
|
||||
move is rejected before reaching the guard, and the test would pass or fail on transition
|
||||
legality rather than on which column id the cancel compares against.
|
||||
*/
|
||||
await store.moveTask(task.id, "inbox" as never, { moveSource: "user" } as never);
|
||||
|
||||
store.taskCache.delete(task.id);
|
||||
expect((await store.getTask(task.id)).column).toBe("inbox");
|
||||
/*
|
||||
The witness. With the pre-fix `?? "todo"` fallback this guard never matched on this lineage, so
|
||||
the marker survived and the merge request was left running behind a card the operator had
|
||||
already parked.
|
||||
*/
|
||||
expect(await store.getCompletionHandoffAcceptedMarker(task.id)).toBeNull();
|
||||
});
|
||||
|
||||
it("does NOT hard-cancel an ENGINE move into the same lane", async () => {
|
||||
/*
|
||||
The paired negative, so the fix cannot pass by cancelling on every move into intake. Only an
|
||||
operator move is a hard cancel; an engine rebound must leave the handoff alone.
|
||||
*/
|
||||
const store = harness.store();
|
||||
const definition = await intakeOnlyWorkflow(store);
|
||||
const task = await store.createTask({ description: "engine rebound card", workflowId: definition.id } as never);
|
||||
|
||||
await store.moveTask(task.id, "signoff" as never, { bypassGuards: true } as never);
|
||||
await store.setCompletionHandoffAcceptedMarker(task.id, { source: "test" });
|
||||
|
||||
await store.moveTask(task.id, "inbox" as never, { moveSource: "engine", recoveryRehome: true, bypassGuards: true } as never);
|
||||
|
||||
expect(await store.getCompletionHandoffAcceptedMarker(task.id)).not.toBeNull();
|
||||
});
|
||||
});
|
||||
@@ -398,6 +398,16 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum
|
||||
// FNXC:WorkflowColumns 2026-07-30-04:00 (U12): resolved unconditionally — the gate is gone, so
|
||||
// `undefined` now means only "no IR on this path or a v1 column-less IR", never "flag off".
|
||||
const workflowIr: WorkflowIr | undefined = await resolveTaskWorkflowIrForMove(store, id);
|
||||
/*
|
||||
FNXC:WorkflowResolvedColumns 2026-07-30-23:30 (fleet: moves.ts):
|
||||
ONE lifecycle resolution for the whole move, derived from the IR resolved on the line above — so
|
||||
every role guard below costs nothing extra on the hottest lifecycle path in the system. The local
|
||||
that used to compute this further down now aliases it rather than resolving a second time.
|
||||
|
||||
`undefined` means no IR on this path or a v1 column-less IR, so each site keeps its legacy id as
|
||||
the fallback: a move must behave exactly as before when there is no basis to resolve from.
|
||||
*/
|
||||
const moveLifecycle = workflowIr ? resolveLifecycleColumns(workflowIr) : undefined;
|
||||
|
||||
if (task.column === toColumn) {
|
||||
if (internal.fromHandoff && toColumn === "in-review") {
|
||||
@@ -475,13 +485,13 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum
|
||||
return task;
|
||||
}
|
||||
|
||||
if (toColumn === "done" && store.clearDoneTransientFields(task)) {
|
||||
if (toColumn === (moveLifecycle?.complete ?? "done") && store.clearDoneTransientFields(task)) {
|
||||
task.updatedAt = new Date().toISOString();
|
||||
await store.atomicWriteTaskJson(dir, task);
|
||||
if (store.isWatching) store.taskCache.set(id, { ...task });
|
||||
store.emit("task:updated", task);
|
||||
}
|
||||
if (toColumn === "done") {
|
||||
if (toColumn === (moveLifecycle?.complete ?? "done")) {
|
||||
await store.clearNearDuplicateReferencesToFailSoft(id, {
|
||||
column: "done",
|
||||
reason: "done",
|
||||
@@ -575,7 +585,7 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum
|
||||
// FNXC:WorkflowTransitionPolicy 2026-07-19-10:20:
|
||||
// The blocker fact is resolved only when the SOURCE column carries the
|
||||
// `merge-blocker` trait flag — the trait-level generalization of the legacy
|
||||
// `fromColumn === "in-review"` gate. Resolving it for every complete-bound
|
||||
// `fromColumn === (moveLifecycle?.review ?? "in-review")` gate. Resolving it for every complete-bound
|
||||
// move re-hardcoded the review lane: `getTaskMergeBlocker` rejects any
|
||||
// source column that is not literally "in-review", which broke the
|
||||
// six-column benchmark's merging → done edge and the builtin
|
||||
@@ -757,7 +767,7 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum
|
||||
}
|
||||
}
|
||||
|
||||
if (fromColumn === "in-review" && toColumn === "done" && !options?.skipMergeBlocker) {
|
||||
if (fromColumn === (moveLifecycle?.review ?? "in-review") && toColumn === (moveLifecycle?.complete ?? "done") && !options?.skipMergeBlocker) {
|
||||
const mergeBlocker = getTaskMergeBlocker(task);
|
||||
if (mergeBlocker) {
|
||||
throw new Error(`Cannot move ${id} to done: ${mergeBlocker}`);
|
||||
@@ -820,7 +830,7 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum
|
||||
or a v1 column-less IR; the hooks treat that as "no basis" and keep the legacy names,
|
||||
which is the only case where a legacy literal is legitimate.
|
||||
*/
|
||||
const moveLifecycleColumns = workflowIr ? resolveLifecycleColumns(workflowIr) : undefined;
|
||||
const moveLifecycleColumns = moveLifecycle;
|
||||
// ── Flag-ON: route the legacy per-column side effects through the
|
||||
// default-workflow trait hooks (timing, reset-on-entry, abort-on-exit,
|
||||
// merge.onEnter). "Moved, not duplicated" applies to this path; the
|
||||
@@ -874,14 +884,14 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum
|
||||
}
|
||||
// Store-owned effects the hooks intentionally do NOT perform (filesystem /
|
||||
// store-private): clearing done transient fields + prompt-checkbox reset.
|
||||
if (toColumn === "done") {
|
||||
if (toColumn === (moveLifecycle?.complete ?? "done")) {
|
||||
store.clearDoneTransientFields(task);
|
||||
}
|
||||
if (isReopenToTodoOrTriage && !preserveStepProgress) {
|
||||
await store.resetPromptCheckboxes(dir);
|
||||
}
|
||||
|
||||
if (toColumn === "in-progress" && !task.worktree && options?.allocateWorktree) {
|
||||
if (toColumn === (moveLifecycle?.wip ?? "in-progress") && !task.worktree && options?.allocateWorktree) {
|
||||
const allocator = options.allocateWorktree;
|
||||
const allocated = await store.withWorktreeAllocationLock(async () => {
|
||||
const others = await store.listTasks({ slim: true, includeArchived: false });
|
||||
@@ -1039,7 +1049,7 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum
|
||||
|
||||
// FNXC:WorkflowReviewGates 2026-07-26-16:40: see isRecognizedInReviewEntry — a
|
||||
// graph-owned crossing into the review column is a legitimate arrival, not a violation.
|
||||
if (toColumn === "in-review" && !isRecognizedInReviewEntry(options, internal)) {
|
||||
if (toColumn === (moveLifecycle?.review ?? "in-review") && !isRecognizedInReviewEntry(options, internal)) {
|
||||
await recordRunAuditEventWithinTransaction(tx, {
|
||||
taskId: id,
|
||||
agentId: internal.runContext?.agentId ?? "system",
|
||||
@@ -1190,7 +1200,18 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum
|
||||
}
|
||||
}
|
||||
|
||||
if (fromColumn === "in-review" && toColumn === "todo" && moveSource === "user") {
|
||||
/*
|
||||
FNXC:WorkflowTaskCancellation 2026-07-30-23:05 (PR #2705 review — greptile):
|
||||
HOLD, THEN INTAKE, THEN THE LEGACY ID. A workflow may declare an intake column and NO hold column
|
||||
(Coding (Ideas)'s `ideas` is the shipped example). `?? "todo"` then names a column that workflow
|
||||
does not declare, so this comparison never matches and the operator's hard cancel silently does
|
||||
nothing: the merge request stays live and the active work items are never cancelled, while the
|
||||
card moves anyway. Failing OPEN on a cancellation contract is the worst available outcome.
|
||||
|
||||
Same precedence the replan target settled on in #2659, and for the same reason — the
|
||||
pre-implementation lane is hold when one exists and intake otherwise.
|
||||
*/
|
||||
if (fromColumn === (moveLifecycle?.review ?? "in-review") && toColumn === (moveLifecycle?.hold ?? moveLifecycle?.intake ?? "todo") && moveSource === "user") {
|
||||
const handoffAccepted = await store.getCompletionHandoffAcceptedMarker(id);
|
||||
const mergeRequest = await store.getMergeRequestRecordAsync(id);
|
||||
if (handoffAccepted && mergeRequest && mergeRequest.state !== "succeeded" && mergeRequest.state !== "cancelled") {
|
||||
@@ -1208,7 +1229,7 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum
|
||||
});
|
||||
void store.clearCompletionHandoffAcceptedMarker(id);
|
||||
}
|
||||
if (toColumn === "todo" && moveSource === "user" && (fromIsImplementation || fromColumn === "in-review")) {
|
||||
if (toColumn === (moveLifecycle?.hold ?? moveLifecycle?.intake ?? "todo") && moveSource === "user" && (fromIsImplementation || fromColumn === (moveLifecycle?.review ?? "in-review"))) {
|
||||
// FNXC:WorkflowTaskCancellation 2026-07-21-11:51:
|
||||
// The task move is already committed here. Continuation cleanup is
|
||||
// best-effort so a storage fault cannot suppress task:moved or strand
|
||||
@@ -1227,7 +1248,7 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum
|
||||
});
|
||||
}
|
||||
}
|
||||
if (toColumn === "done") {
|
||||
if (toColumn === (moveLifecycle?.complete ?? "done")) {
|
||||
// FNXC:RuntimeTaskOrchestrationAsync 2026-06-24-16:00:
|
||||
// Backend mode: clearLinkedAgentTaskIds is a sync SQLite operation; skip
|
||||
// it in backend mode (the agent cleanup is best-effort and handled by
|
||||
@@ -1322,7 +1343,7 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum
|
||||
...(workflowSelectionForMove?.workflowId ? { workflowId: workflowSelectionForMove.workflowId } : {}),
|
||||
});
|
||||
}
|
||||
if (toColumn === "done") {
|
||||
if (toColumn === (moveLifecycle?.complete ?? "done")) {
|
||||
await store.clearNearDuplicateReferencesToFailSoft(id, {
|
||||
column: "done",
|
||||
reason: "done",
|
||||
|
||||
@@ -7,7 +7,6 @@
|
||||
"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/task-store/moves.ts": 15,
|
||||
"packages/core/src/store.ts": 12,
|
||||
"packages/engine/src/project-engine.ts": 12,
|
||||
"packages/engine/src/mission-execution-loop.ts": 10,
|
||||
@@ -70,6 +69,7 @@
|
||||
"packages/core/src/task-store/archive-lifecycle-2.ts": 2,
|
||||
"packages/core/src/task-store/audit-ops.ts": 2,
|
||||
"packages/core/src/task-store/comments-ops.ts": 2,
|
||||
"packages/core/src/task-store/moves.ts": 2,
|
||||
"packages/core/src/task-store/project-store-ops.ts": 2,
|
||||
"packages/core/src/task-store/reads.ts": 2,
|
||||
"packages/core/src/task-store/symbol-locks.ts": 2,
|
||||
|
||||
Reference in New Issue
Block a user