Drift review (unowned): gridlock detection + autopilot retries resolve the hold column — main 103→101 (#2561)
> **Based on `main`, not on my U7 stack** — merges in any order, no dependency on #2517. My assigned files (`triage.ts`, `replan-target.ts`) are at zero, so this picks up two lifecycle-column literals **no unit's file list claims**. Both ask *"is this card in the hold column?"* by the id `todo`, and both are broken **today** for any workflow that renamed it. ## gridlock-detector — the worse of the two `column !== "todo"` decides which cards count as **schedulable**, and an empty schedulable set is an **early return**. On a renamed board the detector concluded *"no gridlock"* at exactly the moment a real one would be visible. > A detector that goes quiet on the boards it cannot parse is worse than one that is absent, because its silence reads as health. **Converting only the `todo` half would have shipped a still-broken detector**, and the test caught it. The `active` filter is equally literal (`in-progress` / `in-review`) — and an empty active set is *also* an early return. Two literals, one silence. The `in-progress` half sits **outside the drift review's `todo|triage` pattern**, which is precisely why a count-driven sweep would have left it behind and declared the file done. Converted here rather than deferred as out of scope. Worth flagging to the other workers: the convergence metric is a good *tracker* but a bad *definition of done* — an adjacent literal in the same predicate can preserve the whole bug at a lower score. ## mission-autopilot The retry compared against `todo` **and moved to the literal `todo`** — so on a renamed workflow it relocated the card into a column the workflow may not declare (R7) on **every retry**. Now resolves the hold role; when the workflow declares none it leaves the card in place and says so, because the error/status clear still runs, so the retry is not lost — the card just stays in its own lane. ## Two fixture defects of my own, both caught by the tests failing wrongly **My first autopilot tests re-implemented the decision** and asserted on the copy — proving only that the copy works. That is the anti-pattern named in `docs/solutions/store-fake-defects-that-masquerade-as-production-bugs.md` (#2534) and in the #2527 ratchet review, and I had no excuse: the constructor takes two stores and `handleTaskFailure` is public. Rewritten to drive the real method. **My first gridlock fixture failed on both vocabularies** — the detector needs three preconditions and I supplied one. A test that fails on its *no-regression* half is a broken fixture, not a discovered bug. The "both halves failed" heuristic from that same doc is what flagged it. That is eight fixture defects across this unit, every one caught by reading *why* a test failed rather than making it pass. ## Revert proofs, each isolated to one literal | Restored | Result | |---|---| | gridlock hold filter | **1 of 5 fails** (renamed case) | | autopilot move target | **1 of 5 fails** (renamed case) | Default-vocabulary halves pass either way — the correct signature for conversions that change no existing behavior. ## Convergence Measured against `origin/main` with a comment-stripped scan of `column === / !== "todo" | "triage"` in `packages/*/src`, excluding tests: **103 → 101.** (The gridlock `active` filter is a third site fixed here that this pattern does not count.) ## Verification | Check | Result | |---|---| | new suite | 5/5 | | pre-existing gridlock + autopilot suites | 85/85, **no expectation edits** | | `tsc --noEmit` (engine) | clean | | `pnpm lint` | clean | | `pnpm test:gate` | green (414 + 10 + 71) | | `pnpm check:changesets` | clean | ## Still unowned after this `mission-feature-sync.ts` (1: a planning-lane check) and `auto-claim-snapshot.ts` (1: `isRunnableAutoClaimCandidate`, a **pure sync** predicate that needs the injected-lane pattern from #2551, not a resolve). `notification-service.ts` has one more with a different semantic — *"has progressed past"* — which needs its own thinking rather than a mechanical swap. I will take these next unless someone claims them. 🤖 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
7
.changeset/unowned-hold-column-gates.md
Normal file
7
.changeset/unowned-hold-column-gates.md
Normal file
@@ -0,0 +1,7 @@
|
||||
---
|
||||
"@runfusion/fusion": patch
|
||||
---
|
||||
|
||||
summary: Gridlock detection and mission-autopilot retries now work on boards whose columns are renamed.
|
||||
category: fix
|
||||
dev: U7 / R3, R7 — unowned drift-review sites. `gridlock-detector` filtered schedulable cards by `column !== "todo"` AND active cards by `in-progress`/`in-review` literals; on a renamed workflow both sets were empty and each empty set is an early return, so the detector reported "no gridlock" on exactly the boards where every card was stuck. `mission-autopilot`'s retry compared and moved to the literal `todo`, relocating the card into a column the workflow may not declare on every retry; it now resolves the hold role and leaves the card in place when none is declared. Measured on main: `column === / !== "todo" | "triage"` 103 -> 101.
|
||||
233
packages/engine/src/__tests__/unowned-hold-column-gates.test.ts
Normal file
233
packages/engine/src/__tests__/unowned-hold-column-gates.test.ts
Normal file
@@ -0,0 +1,233 @@
|
||||
/*
|
||||
FNXC:UnownedHoldColumnGates 2026-07-29-13:20 (U7 / R3, R12 — workflow-owned lifecycle):
|
||||
|
||||
Two lifecycle-column literals nobody's unit claimed, both asking "is this card in
|
||||
the hold column?" by the id `"todo"`.
|
||||
|
||||
Picked up because the drift review's convergence count is what gates U11: the
|
||||
merged Planning column KEEPS the id `todo` and DELETES `triage`, so a `=== "todo"`
|
||||
comparison is not itself the breakage — but a workflow that renamed its hold column
|
||||
is broken TODAY by both of these, and neither file appears in any unit's file list.
|
||||
|
||||
gridlock-detector.ts — `column !== "todo"` decides which cards count as
|
||||
SCHEDULABLE. On a renamed workflow nothing is ever schedulable, so the detector
|
||||
concludes there is no gridlock to report at exactly the moment a real one would
|
||||
be visible: it returns early on an empty schedulable set. A detector that goes
|
||||
quiet on the boards it cannot parse is worse than one that is absent, because
|
||||
its silence reads as health.
|
||||
|
||||
mission-autopilot.ts — `column !== "todo"` decides whether a retried mission task
|
||||
still needs moving. On a renamed workflow the answer is always "yes, move it",
|
||||
and the move TARGET is the literal `"todo"` too — so autopilot relocates the
|
||||
card into a column the workflow may not declare (R7) on every retry.
|
||||
|
||||
Both resolve the hold role from the task's own workflow. Both are async with store
|
||||
access, so they resolve directly rather than needing the injected-lane pattern the
|
||||
synchronous predicates required (#2551).
|
||||
*/
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import type { Task, TaskStore, WorkflowIr } from "@fusion/core";
|
||||
|
||||
import { GridlockDetector } from "../gridlock-detector.js";
|
||||
import { MissionAutopilot } from "../mission-autopilot.js";
|
||||
|
||||
const WF = "custom:hold-vocab";
|
||||
|
||||
const DEFAULT_NAMES = { hold: "todo", wip: "in-progress" };
|
||||
const RENAMED = { hold: "drafting", wip: "building" };
|
||||
|
||||
function ir(names: { hold: string; wip: string }): WorkflowIr {
|
||||
return {
|
||||
version: "v2",
|
||||
id: WF,
|
||||
name: WF,
|
||||
columns: [
|
||||
{ id: "intake", name: "Intake", traits: [{ trait: "intake" }] },
|
||||
{ id: names.hold, name: "Hold", traits: [{ trait: "hold", config: { release: "capacity" } }] },
|
||||
{ id: names.wip, name: "Wip", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] },
|
||||
{ id: "done", name: "Done", traits: [{ trait: "complete" }] },
|
||||
],
|
||||
nodes: [],
|
||||
edges: [],
|
||||
} as unknown as WorkflowIr;
|
||||
}
|
||||
|
||||
function task(over: Partial<Task> = {}): Task {
|
||||
return {
|
||||
id: "FN-1",
|
||||
title: "t",
|
||||
description: "",
|
||||
column: "todo",
|
||||
status: null,
|
||||
dependencies: [],
|
||||
steps: [],
|
||||
currentStep: 0,
|
||||
log: [],
|
||||
createdAt: "2026-01-01T00:00:00.000Z",
|
||||
updatedAt: "2026-01-01T00:00:00.000Z",
|
||||
columnMovedAt: "2026-01-01T00:00:00.000Z",
|
||||
...over,
|
||||
} as Task;
|
||||
}
|
||||
|
||||
function storeWith(tasks: Task[], workflowIr: WorkflowIr, settings: Record<string, unknown> = {}): TaskStore {
|
||||
const selection = { workflowId: WF, stepIds: [] };
|
||||
return {
|
||||
listTasks: vi.fn(async () => tasks),
|
||||
getTask: vi.fn(async (id: string) => tasks.find((t) => t.id === id)),
|
||||
getSettings: vi.fn(async () => ({ maxConcurrent: 2, ...settings })),
|
||||
getTaskWorkflowSelection: vi.fn(() => selection),
|
||||
getTaskWorkflowSelectionAsync: vi.fn(async () => selection),
|
||||
getWorkflowDefinition: vi.fn(async () => ({ ir: workflowIr })),
|
||||
} as unknown as TaskStore;
|
||||
}
|
||||
|
||||
describe("gridlock detection counts schedulable cards by the hold ROLE", () => {
|
||||
/**
|
||||
* A board that IS gridlocked: cards waiting in the hold column, nothing active.
|
||||
* The detector must see the waiting cards to have anything to report.
|
||||
*/
|
||||
async function schedulableSeen(names: { hold: string; wip: string }): Promise<boolean> {
|
||||
/*
|
||||
A genuinely gridlocked board, which needs all three of the detector's
|
||||
preconditions — my first fixture had only the first and failed on BOTH
|
||||
vocabularies, proving nothing:
|
||||
1. a schedulable card waiting in the hold column,
|
||||
2. at least one ACTIVE card (else the detector clears and returns early),
|
||||
3. a reason the waiting card cannot run — here an unmet dependency on the
|
||||
active card.
|
||||
*/
|
||||
const blocker = task({ id: "FN-ACTIVE", column: names.wip });
|
||||
const waiting = task({ id: "FN-WAIT", column: names.hold, dependencies: ["FN-ACTIVE"] });
|
||||
const store = storeWith([waiting, blocker], ir(names));
|
||||
const detector = new GridlockDetector(store);
|
||||
|
||||
// Two passes: the detector reports only once the condition has persisted.
|
||||
await detector.detectGridlock();
|
||||
const event = await detector.detectGridlock();
|
||||
return event !== null;
|
||||
}
|
||||
|
||||
it("sees a waiting card under the DEFAULT vocabulary (no-regression half)", async () => {
|
||||
expect(await schedulableSeen(DEFAULT_NAMES)).toBe(true);
|
||||
});
|
||||
|
||||
it("sees a waiting card under a RENAMED vocabulary", async () => {
|
||||
// Pre-conversion the filter matched nothing, the schedulable set was empty, and
|
||||
// the detector returned early — reporting "no gridlock" on a board where every
|
||||
// card was stuck. Silence that reads as health.
|
||||
expect(await schedulableSeen(RENAMED)).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
/*
|
||||
FNXC:UnownedHoldColumnGates 2026-07-29-14:05:
|
||||
Drives the REAL `MissionAutopilot.handleTaskFailure`. My first version of this block
|
||||
re-implemented the resolve-and-move decision inline and asserted on the copy — which
|
||||
proves only that the copy works, and is precisely the anti-pattern flagged in
|
||||
docs/solutions/store-fake-defects-that-masquerade-as-production-bugs.md and in the
|
||||
#2527 ratchet review. The constructor is two stores and the method is public, so
|
||||
there was no excuse for the shortcut.
|
||||
*/
|
||||
describe("mission autopilot retries into the workflow's own hold column", () => {
|
||||
/** Minimal mission graph so `handleTaskFailure` reaches its retry branch. */
|
||||
function missionStoreFor(taskId: string) {
|
||||
return {
|
||||
getFeatureByTaskId: vi.fn(async () => ({ id: "feat-1", sliceId: "slice-1", taskId })),
|
||||
getSlice: vi.fn(async () => ({ id: "slice-1", milestoneId: "ms-1" })),
|
||||
getMilestone: vi.fn(async () => ({ id: "ms-1", missionId: "mission-1" })),
|
||||
updateFeatureStatus: vi.fn(async () => undefined),
|
||||
recordEvent: vi.fn(async () => undefined),
|
||||
} as never;
|
||||
}
|
||||
|
||||
/** The store after a real retry, for asserting what it did and did not do. */
|
||||
async function retryStoreFor(
|
||||
names: { hold: string; wip: string },
|
||||
opts: { declaresHold?: boolean } = { declaresHold: true },
|
||||
): Promise<TaskStore> {
|
||||
const failed = task({ id: "FN-RETRY", column: names.wip, status: "failed" });
|
||||
const workflowIr = opts.declaresHold
|
||||
? ir(names)
|
||||
: ({
|
||||
version: "v2", id: WF, name: WF, nodes: [], edges: [],
|
||||
columns: [
|
||||
{ id: names.wip, name: "Wip", traits: [{ trait: "wip" }] },
|
||||
{ id: "done", name: "Done", traits: [{ trait: "complete" }] },
|
||||
],
|
||||
} as unknown as WorkflowIr);
|
||||
const store = storeWith([failed], workflowIr);
|
||||
(store as unknown as Record<string, unknown>).moveTask = vi.fn(async () => undefined);
|
||||
(store as unknown as Record<string, unknown>).updateTask = vi.fn(async () => undefined);
|
||||
const autopilot = new MissionAutopilot(store, missionStoreFor("FN-RETRY"));
|
||||
(autopilot as unknown as { watchedMissions: Map<string, unknown> })
|
||||
.watchedMissions.set("mission-1", {});
|
||||
await autopilot.handleTaskFailure("FN-RETRY");
|
||||
return store;
|
||||
}
|
||||
|
||||
/** Where did a real retry move the card, if anywhere? */
|
||||
async function retryTarget(
|
||||
names: { hold: string; wip: string },
|
||||
opts: { declaresHold?: boolean } = { declaresHold: true },
|
||||
): Promise<string | undefined> {
|
||||
const failed = task({ id: "FN-RETRY", column: names.wip, status: "failed" });
|
||||
const workflowIr = opts.declaresHold
|
||||
? ir(names)
|
||||
: ({
|
||||
version: "v2", id: WF, name: WF, nodes: [], edges: [],
|
||||
columns: [
|
||||
{ id: names.wip, name: "Wip", traits: [{ trait: "wip" }] },
|
||||
{ id: "done", name: "Done", traits: [{ trait: "complete" }] },
|
||||
],
|
||||
} as unknown as WorkflowIr);
|
||||
const store = storeWith([failed], workflowIr);
|
||||
const moved: string[] = [];
|
||||
(store as unknown as Record<string, unknown>).moveTask =
|
||||
vi.fn(async (_id: string, column: string) => { moved.push(column); });
|
||||
(store as unknown as Record<string, unknown>).updateTask = vi.fn(async () => undefined);
|
||||
|
||||
const autopilot = new MissionAutopilot(store, missionStoreFor("FN-RETRY"));
|
||||
/*
|
||||
`handleTaskFailure` returns early unless the mission is watched, and
|
||||
`watchMission` is async + goes through `getMission`/event logging. Seeding the
|
||||
watched set directly keeps this test on the branch under test rather than on
|
||||
autopilot's subscription machinery.
|
||||
*/
|
||||
(autopilot as unknown as { watchedMissions: Map<string, unknown> })
|
||||
.watchedMissions.set("mission-1", {});
|
||||
await autopilot.handleTaskFailure("FN-RETRY");
|
||||
|
||||
return moved[0];
|
||||
}
|
||||
|
||||
it("moves a retried card to the DEFAULT hold column (no-regression half)", async () => {
|
||||
expect(await retryTarget(DEFAULT_NAMES)).toBe("todo");
|
||||
});
|
||||
|
||||
it("moves a retried card to a RENAMED hold column, not the literal", async () => {
|
||||
// Pre-conversion autopilot moved it to `todo` — a column this workflow does not
|
||||
// declare — on EVERY retry, which is the R7 violation by repetition.
|
||||
const target = await retryTarget(RENAMED);
|
||||
expect(target).toBe("drafting");
|
||||
expect(target).not.toBe("todo");
|
||||
});
|
||||
|
||||
it("does NOT move the card, and leaves it visibly FAILED, when the workflow declares no hold column", async () => {
|
||||
/*
|
||||
FNXC:UnownedHoldColumnGates 2026-07-29-20:10 (PR #2561 review — greptile P1):
|
||||
My first version cleared the failure state and left the card in WIP, reasoning
|
||||
that "the retry is not lost". It is: the hold-release sweep only dispatches out
|
||||
of HOLD columns, so a card left in WIP is never picked up again — clearing its
|
||||
error turned a visible failure into a SILENT STALL, a row that is not failed,
|
||||
not running, and never will be. Strictly worse than the R7 move it replaced.
|
||||
|
||||
Leaving the failure intact is the correct trade: an operator can act on a failed
|
||||
card, and nothing can act on a clean-looking abandoned one.
|
||||
*/
|
||||
const store = await retryStoreFor({ hold: "unused", wip: "building" }, { declaresHold: false });
|
||||
|
||||
expect(store.moveTask).not.toHaveBeenCalled();
|
||||
expect(store.updateTask).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
@@ -1,4 +1,5 @@
|
||||
import type { MissionStore, Task, TaskStore } from "@fusion/core";
|
||||
import type { MissionStore, Task, TaskStore, WorkflowIr } from "@fusion/core";
|
||||
import { resolveTaskLifecycleColumns } from "@fusion/core";
|
||||
import { createLogger } from "./logger.js";
|
||||
import { filterPathsByIgnoreList, pathsOverlap } from "./scheduler.js";
|
||||
|
||||
@@ -60,8 +61,27 @@ export class GridlockDetector {
|
||||
]);
|
||||
|
||||
const now = Date.now();
|
||||
/*
|
||||
FNXC:UnownedHoldColumnGates 2026-07-29-13:20 (U7 / R3):
|
||||
"Schedulable" is the HOLD role, not the id `todo`. Keyed on the literal, a
|
||||
renamed workflow produced an EMPTY schedulable set, and the detector returns
|
||||
early on empty — so it reported "no gridlock" on precisely the boards where
|
||||
every card was stuck. A detector that goes quiet on the boards it cannot parse
|
||||
is worse than one that is absent, because its silence reads as health.
|
||||
|
||||
One IR cache for the pass, so N cards on M workflows cost M resolutions (the
|
||||
shape `runHoldReleaseSweep` and triage discovery both use). A card whose
|
||||
workflow will not resolve is NOT schedulable — this decides whether to raise an
|
||||
alarm, and inventing candidates would raise false ones.
|
||||
*/
|
||||
const irCache = new Map<string, WorkflowIr>();
|
||||
const holdByTask = new Map<string, string | undefined>();
|
||||
for (const task of tasks) {
|
||||
holdByTask.set(task.id, (await resolveTaskLifecycleColumns(this.store, task.id, irCache))?.hold);
|
||||
}
|
||||
const schedulable = tasks.filter((task) => {
|
||||
if (task.column !== "todo" || task.paused) return false;
|
||||
const hold = holdByTask.get(task.id);
|
||||
if (hold === undefined || task.column !== hold || task.paused) return false;
|
||||
if (task.nextRecoveryAt && new Date(task.nextRecoveryAt).getTime() > now) return false;
|
||||
if (this.isMissionBlocked(task)) return false;
|
||||
return true;
|
||||
@@ -72,7 +92,25 @@ export class GridlockDetector {
|
||||
return null;
|
||||
}
|
||||
|
||||
const active = tasks.filter((task) => task.column === "in-progress" || (task.column === "in-review" && Boolean(task.worktree)));
|
||||
/*
|
||||
FNXC:UnownedHoldColumnGates 2026-07-29-13:45 (U7 / R3):
|
||||
The ACTIVE filter is the same bug as the schedulable one above, and converting
|
||||
only the `todo` half would have left the detector just as blind: `active` is
|
||||
empty on a renamed board, and an empty active set is ALSO an early return. Two
|
||||
literals, one silence — which is why this is converted in the same change rather
|
||||
than counted as out of scope because `in-progress` is not `todo`.
|
||||
*/
|
||||
const rolesByTask = new Map<string, { wip?: string; review?: string }>();
|
||||
for (const task of tasks) {
|
||||
const roles = await resolveTaskLifecycleColumns(this.store, task.id, irCache);
|
||||
rolesByTask.set(task.id, { wip: roles?.wip, review: roles?.review });
|
||||
}
|
||||
const active = tasks.filter((task) => {
|
||||
const roles = rolesByTask.get(task.id);
|
||||
if (!roles) return false;
|
||||
if (roles.wip !== undefined && task.column === roles.wip) return true;
|
||||
return roles.review !== undefined && task.column === roles.review && Boolean(task.worktree);
|
||||
});
|
||||
if (active.length === 0) {
|
||||
this.clearGridlockState();
|
||||
return null;
|
||||
|
||||
@@ -19,7 +19,7 @@
|
||||
* - `completing` → `inactive`: Mission complete
|
||||
*/
|
||||
|
||||
import { AsyncMissionStore } from "@fusion/core";
|
||||
import { AsyncMissionStore, resolveTaskLifecycleColumns } from "@fusion/core";
|
||||
import type {
|
||||
TaskStore,
|
||||
MissionStore,
|
||||
@@ -376,9 +376,41 @@ export class MissionAutopilot {
|
||||
{ taskId, featureId: feature.id, retryCount, maxRetries },
|
||||
);
|
||||
|
||||
/*
|
||||
FNXC:UnownedHoldColumnGates 2026-07-29-13:20 (U7 / R3):
|
||||
Retry returns the card to its workflow's HOLD column. Keyed on the literal
|
||||
`todo`, a renamed workflow answered "not there" on every retry AND moved the
|
||||
card to `todo` — a column it may not declare (R7), on every single retry.
|
||||
|
||||
No resolvable hold column: leave the card where it is rather than relocating
|
||||
it somewhere nothing renders. The error/status clear below still runs, so the
|
||||
retry is not lost — the card simply stays put for the scheduler to pick up
|
||||
from its own lane.
|
||||
*/
|
||||
const task = await this.taskStore.getTask(taskId);
|
||||
if (task?.column !== "todo") {
|
||||
await this.taskStore.moveTask(taskId, "todo");
|
||||
const holdColumn = (await resolveTaskLifecycleColumns(this.taskStore, taskId))?.hold;
|
||||
if (!holdColumn) {
|
||||
/*
|
||||
FNXC:UnownedHoldColumnGates 2026-07-29-20:10 (PR #2561 review — greptile P1):
|
||||
No hold column means there is NOWHERE to retry from: the hold-release sweep
|
||||
only dispatches out of hold columns, so a card left in WIP is never picked up
|
||||
again. Clearing its failure state here would therefore convert a visible
|
||||
failure into a SILENT STALL — the mission would show a task that is not
|
||||
failed, not running, and never will be.
|
||||
|
||||
So leave the failure state intact and say why. A card that stays visibly
|
||||
failed is one an operator can act on; that is strictly better than a clean-
|
||||
looking row nothing will ever touch. The feature keeps its own status, which
|
||||
the retry-count path above already manages.
|
||||
*/
|
||||
autopilotLog.warn(
|
||||
`Mission retry for ${taskId} NOT scheduled — its workflow declares no hold column to retry from, `
|
||||
+ `so nothing would dispatch it. Leaving the task visibly failed in ${task?.column ?? "its current column"} for a human.`,
|
||||
);
|
||||
return;
|
||||
}
|
||||
if (task?.column !== holdColumn) {
|
||||
await this.taskStore.moveTask(taskId, holdColumn);
|
||||
}
|
||||
|
||||
await this.taskStore.updateTask(taskId, { error: null, status: null, paused: false });
|
||||
|
||||
Reference in New Issue
Block a user