From 8aba310d78e1a815f90ceae1a440e429ca1c69d3 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Tue, 28 Jul 2026 11:36:13 -0700 Subject: [PATCH] U11: resolve the worktree-acquisition requeue column by trait (2 sites, both branches) (#2496) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Based on `main`. First of my U11 conversion PRs — small, green, independently revertable. Both heartbeat worktree-acquisition requeue sites hardcoded `"todo"`. ## Why this is critical path, not a renamed-workflow nicety **U11 deletes the `todo` column from the builtin workflows.** After that, these two sites would requeue every acquisition-failed card into a column that no longer exists. ## Both sites converted together They are different branches of the same failure: - the **bounded-retry** requeue, and - the **retry-cap-exhausted** terminal park. Converting one and not the other would leave the rarer path — which fires only after three consecutive failures, so it's the one least likely to be noticed — still writing the literal. Target is the KTD-10 ordering via `resolveReboundTarget` (hold → intake → first column): the same helper `self-healing` and `mesh-lease-manager` already use for "requeue a recovered card", so the recovery paths cannot drift apart. ## What is deliberately untouched `preserveStatus: true` on the exhausted path. It exists because reopen-to-todo semantics would otherwise wipe the `status: "failed"` written immediately before (FN-7721) — changing the column must not disturb that flag. A test asserts the full options object, not just the column. Fail-soft to the legacy id: a requeue must not be abandoned because a workflow lookup failed, or the card is left holding a worktree it could not acquire. Covered by a regression-floor test. ## Verification - **Mutation-verified:** restoring the literal fails 2 of the 3 new tests - 7 tests green (3 new + the 4 pre-existing worktree tests, unchanged) - tsc clean, lint clean, merge gate green (299 + 10 + 71) ## Measured progress **2 of the 74** code-level `"todo"` sites in my unit (engine recovery/scheduling core) are now trait-resolved. Remaining in-unit: `self-healing` 48, `scheduler` 15, `triage` 8, `replan-target` 1. `stuck-task-detector` needs **no work** — all 4 of its occurrences are comments, not code. No changeset: `@fusion/engine` is private. 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Summary by CodeRabbit * **Bug Fixes** * Tasks now return to the workflow’s configured hold column when heartbeat worktree acquisition fails, including workflows that use a renamed hold column. * Retry and retry-limit handling now preserves task progress and, when applicable, status. * Added a safe fallback to the default “todo” column when workflow details cannot be resolved. --- ...nt-heartbeat-worktree-renamed-hold.test.ts | 158 ++++++++++++++++++ packages/engine/src/agent-heartbeat.ts | 28 +++- 2 files changed, 184 insertions(+), 2 deletions(-) create mode 100644 packages/engine/src/__tests__/agent-heartbeat-worktree-renamed-hold.test.ts diff --git a/packages/engine/src/__tests__/agent-heartbeat-worktree-renamed-hold.test.ts b/packages/engine/src/__tests__/agent-heartbeat-worktree-renamed-hold.test.ts new file mode 100644 index 0000000000..30c78c1ad8 --- /dev/null +++ b/packages/engine/src/__tests__/agent-heartbeat-worktree-renamed-hold.test.ts @@ -0,0 +1,158 @@ +/* +FNXC:WorkflowLifecycleColumns 2026-07-28-09:10 (U11 conversion — engine recovery core): + +When worktree acquisition fails, the heartbeat requeues the card to the BACKLOG so +another cycle can retry it. Both requeue sites hardcoded `"todo"`, so for a +workflow whose hold column is named anything else the card was shoved into a +column that workflow does not declare. + +This is the rebound-target shape already converted in mesh-lease-manager and +`recoverStrandedCompletedTodoTasks`: the target is the KTD-10 ordering +`resolveReboundTarget` (hold -> intake -> first column), not the literal `todo`. + +It matters more than usual here because U11 DELETES the `todo` column from the +builtin workflows. After that these two sites would requeue every +acquisition-failed card into a column that no longer exists. + +Both sites are covered, because they are different branches of the same failure: +the bounded-retry requeue and the retry-cap-exhausted terminal park. Converting +one and not the other would leave the rarer path — the one that fires only after +three consecutive failures — still writing the literal. + +Written against the literal implementation and observed FAILING first. +*/ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import type { Agent, AgentHeartbeatRun, WorkflowIr } from "@fusion/core"; +import { HeartbeatMonitor } from "../agent-heartbeat.js"; +import * as worktreeAcquisition from "../worktree-acquisition.js"; +import * as piModule from "../pi.js"; + +const WF = "custom:wf"; + +/** A workflow whose hold column is `drafting` — it declares NO `todo` column. */ +function renamedIr(): WorkflowIr { + return { + version: "v2", + id: WF, + nodes: [], + edges: [], + columns: [ + { id: "inbox", name: "inbox", traits: [{ trait: "intake" }] }, + { id: "drafting", name: "drafting", traits: [{ trait: "hold", config: { release: "capacity" } }] }, + { id: "building", name: "building", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] }, + { id: "shipped", name: "shipped", traits: [{ trait: "complete" }] }, + ], + } as unknown as WorkflowIr; +} + +describe("heartbeat worktree-acquisition requeue under a renamed hold column", () => { + let store: any; + let taskStore: any; + const agent: Agent = { + id: "a1", name: "A", role: "executor", state: "active", taskId: "FN-1", + createdAt: "", updatedAt: "", metadata: {}, + } as any; + + beforeEach(() => { + vi.restoreAllMocks(); + vi.spyOn(piModule, "createFnAgent").mockResolvedValue({ session: { prompt: vi.fn(), dispose: vi.fn() } } as any); + + const run: AgentHeartbeatRun = { + id: "r1", agentId: "a1", status: "active", startedAt: new Date().toISOString(), endedAt: null, + } as any; + store = { + startHeartbeatRun: vi.fn().mockResolvedValue(run), + saveRun: vi.fn(), + getRunDetail: vi.fn().mockResolvedValue(run), + getAgent: vi.fn().mockResolvedValue(agent), + updateAgentState: vi.fn(), + updateAgent: vi.fn(), + endHeartbeatRun: vi.fn(), + assignTask: vi.fn(), + getBudgetStatus: vi.fn().mockResolvedValue({ isOverBudget: false, isOverThreshold: false, usagePercent: 0 }), + getCachedAgent: vi.fn().mockReturnValue(null), + getLastBlockedState: vi.fn().mockResolvedValue(null), + setLastBlockedState: vi.fn(), + clearLastBlockedState: vi.fn(), + appendRunLog: vi.fn(), + getAgentsByReportsTo: vi.fn().mockResolvedValue([]), + recordHeartbeat: vi.fn(), + }; + const selection = { workflowId: WF, stepIds: [] }; + taskStore = { + getSettings: vi.fn().mockResolvedValue({}), + getTask: vi.fn().mockResolvedValue({ + id: "FN-1", title: "t", description: "d", column: "drafting", dependencies: [], steps: [], log: [], + }), + moveTask: vi.fn(), + updateTask: vi.fn(), + logEntry: vi.fn(), + appendAgentLog: vi.fn(), + listTasks: vi.fn().mockResolvedValue([]), + selectNextTaskForAgent: vi.fn().mockResolvedValue(null), + getTaskWorkflowSelection: vi.fn(() => selection), + getTaskWorkflowSelectionAsync: vi.fn(async () => selection), + getWorkflowDefinition: vi.fn(async () => ({ ir: renamedIr() })), + }; + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("requeues to the workflow's HOLD column, not the literal todo", async () => { + vi.spyOn(worktreeAcquisition, "acquireTaskWorktree").mockRejectedValueOnce(new Error("nope")); + const monitor = new HeartbeatMonitor({ store, taskStore, rootDir: "/repo" }); + + await monitor.executeHeartbeat({ agentId: "a1", source: "on_demand" }); + + expect(taskStore.moveTask).toHaveBeenCalledWith("FN-1", "drafting", { preserveProgress: true }); + expect(taskStore.moveTask).not.toHaveBeenCalledWith("FN-1", "todo", expect.anything()); + }); + + it("parks to the HOLD column on the retry-cap-exhausted path too", async () => { + /* + The rarer branch, which only fires after three consecutive failures. It keeps + `preserveStatus: true` so the `status: "failed"` written just before is not + wiped by reopen-to-todo semantics (FN-7721) — converting the column must not + disturb that flag. + */ + vi.spyOn(worktreeAcquisition, "acquireTaskWorktree").mockRejectedValue(new Error("branch exists")); + const monitor = new HeartbeatMonitor({ store, taskStore, rootDir: "/repo" }); + + let recoveryRetryCount: number | null | undefined; + taskStore.updateTask.mockImplementation((_id: string, patch: Record) => { + if ("recoveryRetryCount" in patch) recoveryRetryCount = patch.recoveryRetryCount as number | null; + return Promise.resolve(); + }); + + for (let cycle = 0; cycle < 3; cycle++) { + taskStore.getTask.mockResolvedValue({ + id: "FN-1", title: "t", description: "d", column: "drafting", + dependencies: [], steps: [], log: [], recoveryRetryCount, + }); + await monitor.executeHeartbeat({ agentId: "a1", source: "on_demand" }); + } + + expect(taskStore.moveTask).toHaveBeenCalledWith("FN-1", "drafting", { + preserveProgress: true, + preserveStatus: true, + }); + expect(taskStore.moveTask).not.toHaveBeenCalledWith("FN-1", "todo", expect.anything()); + }); + + it("still requeues to todo when the workflow cannot be resolved (regression floor)", async () => { + /* Conservative fallback: an unresolvable workflow must behave exactly as it + did before this conversion rather than guessing a column. */ + taskStore.getWorkflowDefinition = vi.fn(async () => null); + taskStore.getTask.mockResolvedValue({ + id: "FN-1", title: "t", description: "d", column: "todo", dependencies: [], steps: [], log: [], + }); + vi.spyOn(worktreeAcquisition, "acquireTaskWorktree").mockRejectedValueOnce(new Error("nope")); + const monitor = new HeartbeatMonitor({ store, taskStore, rootDir: "/repo" }); + + await monitor.executeHeartbeat({ agentId: "a1", source: "on_demand" }); + + expect(taskStore.moveTask).toHaveBeenCalledWith("FN-1", "todo", { preserveProgress: true }); + }); +}); diff --git a/packages/engine/src/agent-heartbeat.ts b/packages/engine/src/agent-heartbeat.ts index 4166c1e5fc..cf1d68842c 100644 --- a/packages/engine/src/agent-heartbeat.ts +++ b/packages/engine/src/agent-heartbeat.ts @@ -34,6 +34,8 @@ import { formatAssignedTasksWakeDeltaSection, resolveEffectiveSettingsById, resolveEffectivePlannerHeartbeatPatrolEnabled, + resolveReboundTarget, + resolveWorkflowIrForTask, } from "@fusion/core"; import type { ToolDefinition } from "@earendil-works/pi-coding-agent"; import { Type, type Static } from "@earendil-works/pi-ai"; @@ -97,6 +99,28 @@ import { trimPromptMd, trimTaskDescription, trimTriggeringComments } from "./hea import { detectDeicticReference, extractAntecedentCandidates, renderAmbiguityPromptBlock, scoreReferentConfidence } from "./room-ambiguity.js"; import { countActiveAgentMembers, decideRoomCoordination, detectTaskFilingIntent, renderRoomCoordinationPromptBlock } from "./room-coordination.js"; import { evaluateParkedAgentTaskLink, isParkedTaskColumn, type AgentTaskLinkExecutionProof } from "./task-agent-sync.js"; + +/* +FNXC:WorkflowLifecycleColumns 2026-07-28-09:25 (U11 conversion): +Where a worktree-acquisition failure requeues the card. KTD-10 ordering via +`resolveReboundTarget` (hold -> intake -> first column) — the same helper +self-healing and mesh-lease-manager use for "requeue a recovered card", so the +recovery paths cannot drift apart. + +This matters beyond renamed workflows: U11 DELETES the `todo` column from the +builtin workflows, after which the old literal would requeue every +acquisition-failed card into a column that no longer exists. + +Fail-soft to the legacy id: a requeue must not be abandoned because a workflow +lookup failed, or the card is left holding a worktree it could not acquire. +*/ +async function resolveHeartbeatReboundColumn(taskStore: TaskStore, taskId: string): Promise { + try { + return resolveReboundTarget(await resolveWorkflowIrForTask(taskStore, taskId)) ?? "todo"; + } catch { + return "todo"; + } +} import { classifyReportHealth } from "./reports-health.js"; import { accumulateSessionTokenUsage, captureSessionTokenBaseline } from "./session-token-usage.js"; @@ -2848,11 +2872,11 @@ export class HeartbeatMonitor { * reassigned and retried from scratch, defeating the terminal- * failure intent of this fix (FN-7721). */ - await taskStore.moveTask(taskDetail.id, "todo", { preserveProgress: true, preserveStatus: true }); + await taskStore.moveTask(taskDetail.id, await resolveHeartbeatReboundColumn(taskStore, taskDetail.id), { preserveProgress: true, preserveStatus: true }); this.onTaskAcquisitionExhausted?.(taskDetail.id, exhaustionMessage); } else { await taskStore.updateTask(taskDetail.id, { recoveryRetryCount: attemptsSoFar }); - await taskStore.moveTask(taskDetail.id, "todo", { preserveProgress: true }); + await taskStore.moveTask(taskDetail.id, await resolveHeartbeatReboundColumn(taskStore, taskDetail.id), { preserveProgress: true }); } } await this.completeRun(agentId, run.id, {