From cf7b1a3d46202df8dc037d1a9d9a22d2b29b32a7 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 29 Jul 2026 10:54:35 -0700 Subject: [PATCH] =?UTF-8?q?Drift=20review=20(unowned):=20gridlock=20detect?= =?UTF-8?q?ion=20+=20autopilot=20retries=20resolve=20the=20hold=20column?= =?UTF-8?q?=20=E2=80=94=20main=20103=E2=86=92101=20(#2561)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit > **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) --- .changeset/unowned-hold-column-gates.md | 7 + .../unowned-hold-column-gates.test.ts | 233 ++++++++++++++++++ packages/engine/src/gridlock-detector.ts | 44 +++- packages/engine/src/mission-autopilot.ts | 38 ++- 4 files changed, 316 insertions(+), 6 deletions(-) create mode 100644 .changeset/unowned-hold-column-gates.md create mode 100644 packages/engine/src/__tests__/unowned-hold-column-gates.test.ts diff --git a/.changeset/unowned-hold-column-gates.md b/.changeset/unowned-hold-column-gates.md new file mode 100644 index 0000000000..23481c4c2d --- /dev/null +++ b/.changeset/unowned-hold-column-gates.md @@ -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. diff --git a/packages/engine/src/__tests__/unowned-hold-column-gates.test.ts b/packages/engine/src/__tests__/unowned-hold-column-gates.test.ts new file mode 100644 index 0000000000..503804308b --- /dev/null +++ b/packages/engine/src/__tests__/unowned-hold-column-gates.test.ts @@ -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 { + 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 = {}): 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 { + /* + 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 { + 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).moveTask = vi.fn(async () => undefined); + (store as unknown as Record).updateTask = vi.fn(async () => undefined); + const autopilot = new MissionAutopilot(store, missionStoreFor("FN-RETRY")); + (autopilot as unknown as { watchedMissions: Map }) + .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 { + 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).moveTask = + vi.fn(async (_id: string, column: string) => { moved.push(column); }); + (store as unknown as Record).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 }) + .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(); + }); +}); diff --git a/packages/engine/src/gridlock-detector.ts b/packages/engine/src/gridlock-detector.ts index 49a9df519c..d141484f08 100644 --- a/packages/engine/src/gridlock-detector.ts +++ b/packages/engine/src/gridlock-detector.ts @@ -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(); + const holdByTask = new Map(); + 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(); + 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; diff --git a/packages/engine/src/mission-autopilot.ts b/packages/engine/src/mission-autopilot.ts index d229d94cc3..4633f515b2 100644 --- a/packages/engine/src/mission-autopilot.ts +++ b/packages/engine/src/mission-autopilot.ts @@ -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 });