From 9b61d795c961949eabbf9d8f6949031caffb5068 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 08:42:00 -0700 Subject: [PATCH] =?UTF-8?q?fix(engine):=20heartbeat=20asked=20'is=20this?= =?UTF-8?q?=20task=20finished=3F'=20with=20legacy=20ids=20=E2=80=94=20and?= =?UTF-8?q?=20one=20of=20the=20two=20sites=20writes=20status:failed=20onto?= =?UTF-8?q?=20completed=20work=20(#2769)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Two heartbeat sites asked "is this task finished?" with the legacy ids `agent-heartbeat.ts` **4 → 0**. Neither site is cosmetic. **Linked-task clear.** The heartbeat clears an agent's assignment once its card is finished. Keyed on the literals, an agent on a renamed board stayed bound to a **completed** card indefinitely — every later heartbeat ran with stale task context instead of picking up new work, and nothing else clears it. **Worktree-acquisition gate.** Its failure bookkeeping runs only for a **non-terminal** task. A card in a renamed complete lane read as non-terminal, so an acquisition failure could stamp `status: "failed"` and an error message onto work that was **already done**. That second site *writes*, which drives the fallback direction: an unresolvable workflow degrades toward "terminal", because treating a finished card as unfinished is the expensive mistake here. Both sit in async paths — the first has `await taskStore.getTask(...)` three lines above — so this is an `await`, not a restructure. Extracted to one predicate rather than converted twice: they are the same question, and the two must not drift when one of them acts destructively. ## How this was found, and the part worth recording Generalising #2767. That PR marked a documented false positive the census kept advertising, so I swept for **other** files whose lifecycle literals were reasoned about in prose but still counted — to find out whether the trap was systemic. **It is not.** Of twelve candidate files, only this one carried real unconverted guards, and its "false positive" mentions turned out to be unrelated (detection heuristics, not column literals). The sweep mostly came back **negative**, and that is worth saying so nobody repeats it expecting a haul. ## Revert proof | reverted | result | |---|---| | neuter the resolution (predicate → literals) | **3 failed** / 2 passed | | shipped | **5 passed** | The two that survive the revert are the degraded-mode pair, which is correct — they assert the *legacy* answer, so they must pass either way. One case also pins that a legacy `done` id is **not** terminal on a board that does not declare it, which is what a board-wide union would get wrong. ## Verification - 13 heartbeat suites — **501 passed** (496 before, +5 new) - `pnpm test:gate` — **158 / 10 / 487 / 71** · `pnpm lint` clean · engine `tsc --noEmit` **0 errors** · `--strict` exits 0 Co-authored-by: Claude Opus 5 (1M context) --- .../__tests__/heartbeat-terminal-lane.test.ts | 79 +++++++++++++++++++ packages/engine/src/agent-heartbeat.ts | 34 +++++++- .../lib/lifecycle-column-census-baseline.json | 3 +- 3 files changed, 113 insertions(+), 3 deletions(-) create mode 100644 packages/engine/src/__tests__/heartbeat-terminal-lane.test.ts diff --git a/packages/engine/src/__tests__/heartbeat-terminal-lane.test.ts b/packages/engine/src/__tests__/heartbeat-terminal-lane.test.ts new file mode 100644 index 0000000000..80c037c98c --- /dev/null +++ b/packages/engine/src/__tests__/heartbeat-terminal-lane.test.ts @@ -0,0 +1,79 @@ +import { describe, expect, it, vi } from "vitest"; +import { isTaskInTerminalLane } from "../agent-heartbeat.js"; +import type { TaskStore, WorkflowIr } from "@fusion/core"; + +/* +FNXC:WorkflowLifecycleColumns 2026-08-01-07:20 (fleet — two heartbeat terminal checks): + +Both call sites asked "is this task finished?" with `column === "done" || "archived"`, and neither is +cosmetic: + + LINKED-TASK CLEAR. The heartbeat clears an agent's assignment once its card is finished. Keyed on + the literals, an agent on a renamed board stayed bound to a COMPLETED card indefinitely, so every + later heartbeat ran with stale task context instead of picking up new work. + + WORKTREE-ACQUISITION GATE. Its failure bookkeeping runs only for a NON-terminal task. A card in a + renamed complete lane read as non-terminal, so an acquisition failure could stamp + `status: "failed"` and an error onto work that was already done. That site WRITES, which is why + the fallback below degrades toward "terminal" rather than "unfinished". + +Tested at the predicate because it IS the conversion — both call sites are one-line swaps to it. The +cases are differential: one workflow SHAPE, two vocabularies, only the ids differ, so any difference +between the runs is attributable to a surviving literal. +*/ + +const RENAMED_IR = { + version: "v2", + id: "custom:renamed-terminal", + nodes: [], + edges: [], + columns: [ + { id: "drafting", label: "Drafting", traits: [{ trait: "hold", config: { release: "capacity" } }] }, + { id: "building", label: "Building", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] }, + { id: "shipped", label: "Shipped", traits: [{ trait: "complete" }] }, + { id: "filed", label: "Filed", traits: [{ trait: "archived" }] }, + ], +} as unknown as WorkflowIr; + +function storeFor(ir: WorkflowIr | undefined): TaskStore { + const selection = { workflowId: "custom:renamed-terminal", stepIds: [] as string[] }; + return { + getTaskWorkflowSelection: vi.fn(() => undefined), + getTaskWorkflowSelectionAsync: vi.fn(async () => (ir ? selection : undefined)), + getWorkflowDefinition: vi.fn(async () => (ir ? { ir } : undefined)), + } as unknown as TaskStore; +} + +describe("heartbeat resolves the terminal lanes from the task's own board", () => { + it("recognises a RENAMED complete lane as terminal", async () => { + expect(await isTaskInTerminalLane(storeFor(RENAMED_IR), { id: "FN-1", column: "shipped" })).toBe(true); + }); + + it("recognises a RENAMED archived lane as terminal", async () => { + expect(await isTaskInTerminalLane(storeFor(RENAMED_IR), { id: "FN-1", column: "filed" })).toBe(true); + }); + + /* The paired negative, and the one that matters most: the writing call site must not fire on it. */ + it("does NOT treat an active lane as terminal", async () => { + expect(await isTaskInTerminalLane(storeFor(RENAMED_IR), { id: "FN-1", column: "building" })).toBe(false); + expect(await isTaskInTerminalLane(storeFor(RENAMED_IR), { id: "FN-1", column: "drafting" })).toBe(false); + }); + + /* + A card sitting on a legacy id that the RENAMED board does not declare is NOT terminal there. This is + the case a `terminal.has(column)`-style union would get wrong, and it is why the check compares + against the task's own resolved lanes rather than a board-wide set. + */ + it("does not treat a legacy id as terminal on a board that does not declare it", async () => { + expect(await isTaskInTerminalLane(storeFor(RENAMED_IR), { id: "FN-1", column: "done" })).toBe(false); + }); + + /* Degraded mode: an unresolvable workflow keeps exactly the legacy answer, both directions. */ + it("falls back to the legacy pair when the workflow cannot be resolved", async () => { + const store = storeFor(undefined); + + expect(await isTaskInTerminalLane(store, { id: "FN-1", column: "done" })).toBe(true); + expect(await isTaskInTerminalLane(store, { id: "FN-1", column: "archived" })).toBe(true); + expect(await isTaskInTerminalLane(store, { id: "FN-1", column: "in-progress" })).toBe(false); + }); +}); diff --git a/packages/engine/src/agent-heartbeat.ts b/packages/engine/src/agent-heartbeat.ts index cf1d68842c..02fcd7e449 100644 --- a/packages/engine/src/agent-heartbeat.ts +++ b/packages/engine/src/agent-heartbeat.ts @@ -36,6 +36,7 @@ import { resolveEffectivePlannerHeartbeatPatrolEnabled, resolveReboundTarget, resolveWorkflowIrForTask, + resolveTaskLifecycleColumns, } from "@fusion/core"; import type { ToolDefinition } from "@earendil-works/pi-coding-agent"; import { Type, type Static } from "@earendil-works/pi-ai"; @@ -676,6 +677,35 @@ async function getHeartbeatMemorySettings(taskStore: TaskStore): Promise, +): Promise { + const columns = await resolveTaskLifecycleColumns(taskStore, task.id, cache).catch(() => undefined); + /* DELIBERATE-LITERAL — the no-metadata fallback. Deleting it makes an unresolvable workflow read + as NEVER terminal, which is the direction that writes: the second call site would then run its + failure bookkeeping against finished work. Strictly worse than the legacy answer. */ + if (!columns) return task.column === "done" || task.column === "archived"; + return task.column === columns.complete || task.column === columns.archived; +} + export class HeartbeatMonitor { private store: AgentStore; private configStore: AgentStore; @@ -2413,7 +2443,7 @@ export class HeartbeatMonitor { return (await this.store.getRunDetail(agentId, run.id))!; } - if (taskDetail.column === "done" || taskDetail.column === "archived") { + if (await isTaskInTerminalLane(taskStore, taskDetail)) { if (agent.taskId === resolvedTaskId) { heartbeatLog.log( `Agent ${agentId} linked task ${resolvedTaskId} is ${taskDetail.column} — clearing assignment and running heartbeat without task context`, @@ -2852,7 +2882,7 @@ export class HeartbeatMonitor { const attemptsSoFar = priorAttempts + 1; const retryCapExhausted = attemptsSoFar >= MAX_HEARTBEAT_WORKTREE_ACQUISITION_RETRIES; - if (taskDetail.column !== "done" && taskDetail.column !== "archived") { + if (!(await isTaskInTerminalLane(taskStore, taskDetail))) { if (retryCapExhausted) { const exhaustionMessage = `Worktree acquisition failed after ${MAX_HEARTBEAT_WORKTREE_ACQUISITION_RETRIES} heartbeat attempts for branch "${taskDetail.branch ?? `fusion/${taskDetail.id.toLowerCase()}`}": ${detail}`; await taskStore.updateTask(taskDetail.id, { diff --git a/scripts/lib/lifecycle-column-census-baseline.json b/scripts/lib/lifecycle-column-census-baseline.json index 4edd863ed8..b0d9d84ec8 100644 --- a/scripts/lib/lifecycle-column-census-baseline.json +++ b/scripts/lib/lifecycle-column-census-baseline.json @@ -17,7 +17,6 @@ "packages/engine/src/restart-recovery-coordinator.ts": 5, "packages/dashboard/app/components/TaskDetailModal.tsx": 4, "packages/dashboard/src/routes/register-git-github.ts": 4, - "packages/engine/src/agent-heartbeat.ts": 4, "packages/engine/src/replan-target.ts": 4, "packages/engine/src/triage.ts": 4, "packages/core/src/async-mission-store-queries.ts": 3, @@ -154,6 +153,8 @@ "packages/dashboard/src/reliability-metrics.ts\u0000in-progress": 1, "packages/dashboard/src/routes/register-task-workflow-routes.ts\u0000todo": 1, "packages/dashboard/src/routes/register-task-workflow-routes.ts\u0000triage": 1, + "packages/engine/src/agent-heartbeat.ts\u0000archived": 1, + "packages/engine/src/agent-heartbeat.ts\u0000done": 1, "packages/engine/src/hold-release.ts\u0000archived": 1, "packages/engine/src/hold-release.ts\u0000done": 1, "packages/engine/src/hold-release.ts\u0000in-review": 1,