From 02b0f4f8607394d7891d912e35398f893d02f237 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Mon, 27 Jul 2026 14:28:53 -0700 Subject: [PATCH] =?UTF-8?q?Phase=20B=20slice=20B2:=20U5=20small=20movers?= =?UTF-8?q?=20=E2=80=94=2012=20literal=20sites=20converted,=20plus=20a=20n?= =?UTF-8?q?egative=20result=20on=20hold-release=20(#2471)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stacked on #2470 (Phase B slice B1). Base is `feature/workflow-vocabulary-conversion` — do not merge before it. ## What this is Phase B slice B2 — the U5 small movers. **12 literal sites converted, plus one negative result.** The plan estimated 36 sites. A survey found 12 genuinely-convertible ones, and separately found that the plan's headline hold-release scenario **was already fixed**. Both are reported below rather than padded into a bigger-looking diff. ## The negative result (commit 1) The plan named hold-release.ts as a target on the scenario *"release readiness must hold and release identically for a RENAMED hold column."* I wrote that test first, to prove it broken. **It is not broken.** All five assertions passed against unmodified `hold-release.ts`. U6/KTD-5 had already converted the module — `isHeldTask`, `resolveReleaseTarget`, and `dependencySatisfied` each resolve the task's IR. **`hold-release.ts` has no production change in this PR.** The tests are kept as a regression floor: the invariant rests on three independent trait resolutions any of which could be "simplified" back to a literal, and nothing else covered a renamed vocabulary end-to-end through the sweep. **I verified the tests can actually fail.** Mutating `isHeldTask` back to `task.column === "todo"` kills all five. Without that check, a green run against unmodified code is indistinguishable from a test asserting something trivially true. Two drafting notes kept in the file: the renamed ids deliberately avoid colliding with any legacy literal, and the first draft's two dependency tests used a `capacity` hold — which never consults dependencies at all, so one passed **vacuously**. Both now use a `dependency` hold. ## The 12 conversions Each was red-green: the renamed-workflow test written first and **observed failing**, then made to pass. | Site | Was | Now | |---|---|---| | `task-agent-sync` CLEAR_COLUMNS | `{done,archived,todo,triage}` | resolved complete+archived+hold+intake | | `task-agent-sync` isParkedTaskColumn | `{todo,triage}` | `parkedColumns` param (hold+intake) | | `task-agent-sync` handler branch | `to === "todo" \|\| "triage"` | resolved parked set | | `mesh-lease` parked guard | `task.column !== "todo"` | resolved rebound column | | `mesh-lease` rebound move | `moveTask(id,"todo")` | resolved rebound column | | `mesh-lease` audit decisionPath | `=== "todo" ? … : …` | same resolved column | | `mesh-lease` audit newColumn | `… : "todo"` | same resolved column | | `merger-ai` already-finalized | `=== "done" \|\| "archived"` | resolved complete+archived | | `merger-ai` ×4 rebounds | `moveTask(id,"todo")` | shared `resolveFinalizeReboundColumn` | Rebound targets all use KTD-10 `resolveReboundTarget` (hold → intake → first column), the helper `self-healing.ts:714` already uses — reused, not invented. ## Three findings worth reading **1. The mesh-lease bug was in the AUDIT, not the move.** The guard and the audit were *independent* `=== "todo"` comparisons, so `newColumn` asserted the card landed in `todo` regardless of what the move did. For a workflow with no `todo` column that produced a lease-recovery trail naming a nonexistent column — and run-audit is the only post-hoc record of a lease recovery. Now resolved once and threaded to both, so they are structurally incapable of disagreeing. **2. The merger-ai failure mode was not what I predicted.** I expected the already-finalized guard to fail open and re-merge a finished card. The red run showed it actually throws `Cannot merge FN-1: task is in 'shipped', must be in 'in-review'` — a hard error blaming the column, on a task whose real state is "already done". The thing preventing the re-merge is *itself* a literal in core's `getTaskMergeBlocker`, outside this slice. Two bugs coinciding, not a design. **3. A fourth site had to move that wasn't on the list.** `evaluateParkedAgentTaskLink` calls `isParkedTaskColumn` internally. Converting only the handler would have left the preservation branch on legacy ids after the caller resolved a renamed workflow — trading a stale-link bug for a **worse** dropped-link bug (a live agent's link cleared mid-run). ## Deliberately NOT converted Both keep their literals with the reason recorded at the site under a greppable `DELIBERATE-LITERAL` tag: - **`hold-release.ts:326` `legacyDependencySatisfied`** — the FN-5719 dual-accept half. Converting makes both halves compute the same answer, deleting the compatibility signal *and* its divergence detector while looking like a cleanup. - **`replan-target.ts` final fallback** — its value is precisely that it is *not* trait-resolved; resolving it against the workflow is the stranded-card bug it was written to fix. ⚠️ **The U12 literal ratchet does not exist in the tree yet.** The brief assumed an allowlist to add entries to; there is none. `grep -rn DELIBERATE-LITERAL packages/*/src` enumerates the sites it must admit. ## What I could NOT verify - **One of the four merger-ai rebound sites is untested.** The `landWorkspaceTask` rebound is verified by inspection and the shared resolver's unit tests only — `landWorkspaceTask` is only ever *mocked* (project-engine.test.ts), never executed. Covering it needs a multi-repo git fixture and a full land run. The **other three are genuinely exercised** by pre-existing merger-ai.test.ts (lines 676/716/777/895 assert `moveTask("FN-1","todo",…)` through a real git repo) and pass unchanged — real wiring proof for those. - **3 of the 9 task-agent-sync tests passed before the conversion too**, vacuously — the literal handler early-returned and cleared nothing. They assert nothing about the old code; they are guardrails against the conversion over-clearing. - **No renamed workflow was run against a live engine.** All evidence is unit-level. ## Call sites outside this slice — NOT converted, byte-identical They keep the legacy defaults: `scheduler.ts:1273`, `agent-heartbeat.ts:1169/3642`, `self-healing.ts:11600/11665` (all `evaluateParkedAgentTaskLink`), and `merger.ts:6585` (the sibling terminal guard). Each is its own Phase C/D surface. ## Behavior changes (not a pure refactor) For a **renamed** workflow: agent links now actually get cleared on terminal moves (they never were); lease rebounds land in the resolved hold column; finalize-blocked rebounds land in the resolved hold column and their operator-facing task-log lines name the real column; already-finalized cards short-circuit cleanly instead of throwing. For **builtin:coding** and any unresolvable workflow: byte-identical. Every new parameter defaults to the legacy set, and both merger-ai resolvers fail *soft* to legacy ids in opposite directions — the terminal guard keeps `done`/`archived` (losing it sends a finished card into the merge path), the rebound keeps `todo` (abandoning it strands the card in the merge lane with no owner). ## Verification - Merge gate **green**: 299 + 10 + 71 tests - Slice suites **green**: 100 tests across 8 files (new + all pre-existing neighbours) - Existing merger suites **green**: 82 tests across 5 files, unchanged - `tsc --noEmit` clean, `pnpm lint` clean No changeset: `@fusion/engine` is private. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- .../hold-release-renamed-columns.test.ts | 219 ++++++++++++++ .../merger-ai-renamed-columns.test.ts | 272 ++++++++++++++++++ ...mesh-lease-manager-renamed-columns.test.ts | 198 +++++++++++++ .../task-agent-sync-renamed-columns.test.ts | 195 +++++++++++++ packages/engine/src/hold-release.ts | 14 + packages/engine/src/merger-ai.ts | 108 ++++++- packages/engine/src/mesh-lease-manager.ts | 71 ++++- packages/engine/src/replan-target.ts | 11 + packages/engine/src/task-agent-sync.ts | 118 +++++++- 9 files changed, 1182 insertions(+), 24 deletions(-) create mode 100644 packages/engine/src/__tests__/hold-release-renamed-columns.test.ts create mode 100644 packages/engine/src/__tests__/merger-ai-renamed-columns.test.ts create mode 100644 packages/engine/src/__tests__/mesh-lease-manager-renamed-columns.test.ts create mode 100644 packages/engine/src/__tests__/task-agent-sync-renamed-columns.test.ts diff --git a/packages/engine/src/__tests__/hold-release-renamed-columns.test.ts b/packages/engine/src/__tests__/hold-release-renamed-columns.test.ts new file mode 100644 index 0000000000..9f857cfcbb --- /dev/null +++ b/packages/engine/src/__tests__/hold-release-renamed-columns.test.ts @@ -0,0 +1,219 @@ +/* +FNXC:WorkflowLifecycleColumns 2026-07-27-22:10 (Phase B / slice B2): + +NEGATIVE RESULT, pinned deliberately. The Phase B plan named hold-release as a +conversion target: "release readiness must hold and release identically for a +RENAMED hold column." These tests were written FIRST, against unmodified +hold-release.ts, to prove that scenario BROKEN. It is not broken — every +assertion below passed on the first run with no production change. hold-release +was already converted to trait resolution by U6/KTD-5 (`isAtHoldColumn`, +`resolveReleaseTarget`, and `dependencySatisfied` all resolve the task's IR +rather than comparing against a literal id), so slice B2 has no work to do here. + +They are kept as a REGRESSION FLOOR rather than deleted. The invariant is +currently upheld by three separate trait resolutions, any one of which could be +"simplified" back to a literal id by a later change; nothing else in the suite +covers the renamed-column shape end-to-end through the sweep. A test that has +never failed is weak evidence on its own — its value here is that it is +differential: it runs the SAME scenario against default-named and renamed +workflows and asserts the outcomes match, so it fails if a literal creeps back +into any of the three paths. + +The one literal that remains in this module — `legacyDependencySatisfied`'s +`done`/`in-review`/`archived` check at hold-release.ts:326 — is deliberately NOT +converted. It is the FN-5719 DUAL-ACCEPT half that honors the legacy completion +signal and logs an audit diff when the two halves disagree; converting it would +delete a compatibility signal rather than fix a bug. The renamed workflow below +is satisfied through the TRAIT half, which is the point: the literal half is +additive, not load-bearing, for a renamed workflow. +*/ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { Task, TaskStore, WorkflowIr } from "@fusion/core"; + +import { runHoldReleaseSweep, resetHoldReleaseInstrumentation } from "../hold-release.js"; +import { schedulerLog } from "../logger.js"; + +const WF = "custom:wf"; + +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; +} + +/** + * The same workflow SHAPE under two vocabularies. `names` supplies the column + * ids; the traits — which are what the sweep actually reasons about — are + * identical in both. Any behavioral difference between the two is therefore + * attributable to a surviving column-id literal and nothing else. + */ +function ir( + names: { hold: string; wip: string; complete: string }, + release: "capacity" | "dependency" = "capacity", +): WorkflowIr { + return { + version: "v2", + id: WF, + nodes: [], + edges: [], + columns: [ + { id: names.hold, label: "Hold", traits: [{ trait: "hold", config: { release } }] }, + { id: names.wip, label: "Wip", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] }, + { id: names.complete, label: "Complete", traits: [{ trait: "complete" }] }, + ], + } as unknown as WorkflowIr; +} + +const DEFAULT_NAMES = { hold: "todo", wip: "in-progress", complete: "done" }; +/* Every role renamed, and deliberately NONE of the renamed ids collides with a + legacy literal — so a surviving `=== "todo"` comparison cannot pass by luck. */ +const RENAMED = { hold: "drafting", wip: "building", complete: "shipped" }; + +function storeWith(tasks: Task[], workflowIr: WorkflowIr, settings: Record): TaskStore { + const selection = { workflowId: WF, stepIds: [] }; + return { + getSettings: vi.fn(async () => settings), + listTasks: vi.fn(async () => tasks), + getTask: vi.fn(async (id: string) => tasks.find((t) => t.id === id) ?? null), + moveTaskIf: vi.fn(async (id: string, column: string) => { + const cur = tasks.find((t) => t.id === id)!; + cur.column = column; + return { task: cur, moved: true }; + }), + logEntry: vi.fn(async () => undefined), + recordRunAuditEvent: vi.fn(async () => undefined), + getCompletionHandoffAcceptedMarker: vi.fn(async () => null), + getTaskWorkflowSelection: vi.fn(() => selection), + getTaskWorkflowSelectionAsync: vi.fn(async () => selection), + getWorkflowDefinition: vi.fn(async () => ({ ir: workflowIr })), + } as unknown as TaskStore; +} + +/** + * Run the capacity-hold scenario under one vocabulary and report the outcome in + * ROLE terms (not column ids) so the two runs are directly comparable. + */ +async function capacityScenario(names: { hold: string; wip: string; complete: string }) { + const held = task({ id: "H", column: names.hold }); + const occupant = task({ id: "O", column: names.wip }); + const store = storeWith([held, occupant], ir(names), { maxConcurrent: 1 }); + + // Pass 1 — the single wip slot is occupied, so the card must be HELD. + const saturated = await runHoldReleaseSweep(store, { now: () => 1_000_000 }); + + // Pass 2 — the occupant leaves for the complete column, freeing the slot. + occupant.column = names.complete; + const freed = await runHoldReleaseSweep(store, { now: () => 1_045_000 }); + + return { + heldWhileSaturated: saturated.held.some((h) => h.taskId === "H"), + heldReason: saturated.held.find((h) => h.taskId === "H")?.reason, + releasedWhileSaturated: saturated.released, + releasedOnceFreed: freed.released, + // Reported as "did it land in the WIP role", not "did it land in in-progress". + landedInWipRole: held.column === names.wip, + }; +} + +describe("hold/release under a renamed column vocabulary", () => { + beforeEach(() => { + resetHoldReleaseInstrumentation(); + vi.restoreAllMocks(); + vi.spyOn(schedulerLog, "log").mockImplementation(() => {}); + vi.spyOn(schedulerLog, "debug").mockImplementation(() => {}); + vi.spyOn(schedulerLog, "warn").mockImplementation(() => {}); + }); + + it("holds and releases a capacity-held card identically whether or not the columns are renamed", async () => { + const legacy = await capacityScenario(DEFAULT_NAMES); + resetHoldReleaseInstrumentation(); + const renamed = await capacityScenario(RENAMED); + + // The renamed run is not vacuously equal: it really did hold, then release. + expect(legacy.heldWhileSaturated).toBe(true); + expect(legacy.releasedOnceFreed).toEqual(["H"]); + expect(legacy.landedInWipRole).toBe(true); + + // …and the renamed vocabulary produces the identical role-level outcome. + expect(renamed).toEqual(legacy); + }); + + it("recognizes a renamed hold column as a hold at all (the card is not simply ignored)", async () => { + /* + Guards the failure mode a naive equality check would produce: not a wrong + release, but NO decision — an unrecognized hold column makes the card + invisible to the sweep, which looks like a quiet, permanently-stuck card + rather than an error. + */ + const held = task({ id: "H", column: RENAMED.hold }); + const occupant = task({ id: "O", column: RENAMED.wip }); + const store = storeWith([held, occupant], ir(RENAMED), { maxConcurrent: 1 }); + + const result = await runHoldReleaseSweep(store, { now: () => 1_000_000 }); + + expect(result.held.map((h) => h.taskId)).toContain("H"); + expect(result.held.find((h) => h.taskId === "H")?.reason).toBe("downstream-full"); + }); + + it("releases into the workflow's own wip column, never into a literal 'in-progress'", async () => { + const held = task({ id: "H", column: RENAMED.hold }); + const store = storeWith([held], ir(RENAMED), { maxConcurrent: 5 }); + + const result = await runHoldReleaseSweep(store, { now: () => 1_000_000 }); + + expect(result.released).toEqual(["H"]); + expect(held.column).toBe(RENAMED.wip); + expect(held.column).not.toBe("in-progress"); + }); + + /* + These two use a `dependency` hold, NOT a capacity hold. A capacity hold never + consults dependencies at all (hold-release.ts dispatches on the hold's + `release` kind), so the same scenario under `release: "capacity"` releases the + card on the free slot and would assert nothing about dependency satisfaction. + The first draft of this file made exactly that mistake and passed vacuously. + */ + it("satisfies a dependency through the COMPLETE trait when the complete column is renamed", async () => { + /* + FN-5719 dual-accept: `dependencySatisfied` ORs a trait check against the + legacy done/in-review/archived literal. A renamed complete column + ("shipped") matches ONLY the trait half, so this asserts the half that + survives a rename — and would fail if the trait half were ever dropped in + favor of the literal one. + */ + const dep = task({ id: "DEP", column: RENAMED.complete }); + const held = task({ id: "H", column: RENAMED.hold, dependencies: ["DEP"] }); + const store = storeWith([held, dep], ir(RENAMED, "dependency"), { maxConcurrent: 5 }); + + const result = await runHoldReleaseSweep(store, { now: () => 1_000_000 }); + + expect(result.released).toEqual(["H"]); + expect(held.column).toBe(RENAMED.wip); + }); + + it("still blocks on an unsatisfied dependency under the renamed vocabulary", async () => { + /* The negative half — otherwise the test above would pass even if + dependencies were not being evaluated at all under a renamed workflow. */ + const dep = task({ id: "DEP", column: RENAMED.wip }); + const held = task({ id: "H", column: RENAMED.hold, dependencies: ["DEP"] }); + const store = storeWith([held, dep], ir(RENAMED, "dependency"), { maxConcurrent: 5 }); + + const result = await runHoldReleaseSweep(store, { now: () => 1_000_000 }); + + expect(result.released).toEqual([]); + expect(result.held.find((h) => h.taskId === "H")?.reason).toBe("deps-unsatisfied"); + expect(held.column).toBe(RENAMED.hold); + }); +}); diff --git a/packages/engine/src/__tests__/merger-ai-renamed-columns.test.ts b/packages/engine/src/__tests__/merger-ai-renamed-columns.test.ts new file mode 100644 index 0000000000..ca2667045b --- /dev/null +++ b/packages/engine/src/__tests__/merger-ai-renamed-columns.test.ts @@ -0,0 +1,272 @@ +/* +FNXC:WorkflowLifecycleColumns 2026-07-27-23:50 (Phase B / slice B2): + +merger-ai decides two things by literal column id: + + 1. runAiMerge's already-finalized short circuit (`task.column === "done" || + task.column === "archived"`) — the COMPLETE and ARCHIVED roles. Under a + renamed workflow this stops matching and the already-finalized card falls + through to `getTaskMergeBlocker`, which throws + `Cannot merge FN-1: task is in 'shipped', must be in 'in-review'`. + + Observed, not assumed — that is the actual failure these tests produced + against the literal code. So the consequence is NOT a silent re-merge (a + downstream literal happens to catch it); it is a hard error blaming the + card's column, on a task whose real state is "already done, nothing to do". + The correct outcome is a clean no-op. Note the safety net is itself a + literal (`must be in 'in-review'`) living in core's `getTaskMergeBlocker`, + outside this slice — so it is a coincidence of two bugs, not a design. + + 2. Four "finalize blocked → return the card to the backlog" rebounds + (`moveTask(taskId, "todo", …)`), all of which park work for operator review + after a no-commits / no-landed-proof / vetoed-no-op guard fires. Under a + workflow with no `todo` column those moves target a column that does not + exist. + +The tests below were written against the literal implementation and observed +FAILING first. The rebound target is the KTD-10 `resolveReboundTarget` ordering +already used by self-healing.ts:714 and (in this slice) mesh-lease-manager, not +a new rule. + +Scope note, stated rather than implied: the four rebound CALL SITES are wired to +the shared resolver but are not each driven end-to-end here. Reaching them +requires a real git repo plus a full merge run (see merger-ai.test.ts), and the +project's standing rule is not to add slow tests when a narrower seam exists. +What is asserted here is the resolver those four sites now share; what is NOT +asserted is that each site is reachable under a renamed workflow. That gap is +reported in the PR rather than papered over. +*/ +import { describe, expect, it, vi } from "vitest"; +import type { Task, TaskStore, WorkflowIr } from "@fusion/core"; + +import { runAiMerge, resolveFinalizeReboundColumn } from "../merger-ai.js"; + +const WF = "custom:wf"; + +function task(overrides: Partial = {}): Task { + return { + id: "FN-1", + title: "t", + description: "", + column: "in-review", + dependencies: [], + steps: [], + currentStep: 0, + log: [], + createdAt: "2026-05-01T00:00:00.000Z", + updatedAt: "2026-05-01T00:00:00.000Z", + ...overrides, + } as Task; +} + +/** No `todo`, no `done`, no `archived` — every role renamed. */ +function renamedIr(): WorkflowIr { + return { + version: "v2", + id: WF, + nodes: [], + edges: [], + columns: [ + { id: "inbox", label: "Inbox", traits: [{ trait: "intake" }] }, + { id: "drafting", label: "Drafting", traits: [{ trait: "hold", config: { release: "capacity" } }] }, + { id: "building", label: "Building", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] }, + { id: "reviewing", label: "Reviewing", traits: [{ trait: "mergeOrchestration" }] }, + { id: "shipped", label: "Shipped", traits: [{ trait: "complete" }] }, + { id: "retired", label: "Retired", traits: [{ trait: "archived" }] }, + ], + } as unknown as WorkflowIr; +} + +function defaultIr(): WorkflowIr { + return { + version: "v2", + id: WF, + nodes: [], + edges: [], + columns: [ + { id: "triage", label: "Triage", traits: [{ trait: "intake" }] }, + { id: "todo", label: "Todo", traits: [{ trait: "hold", config: { release: "capacity" } }] }, + { id: "in-progress", label: "In Progress", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] }, + { id: "in-review", label: "In Review", traits: [{ trait: "mergeOrchestration" }] }, + { id: "done", label: "Done", traits: [{ trait: "complete" }] }, + { id: "archived", label: "Archived", traits: [{ trait: "archived" }] }, + ], + } as unknown as WorkflowIr; +} + +function storeWith(current: Task, ir: WorkflowIr | undefined): TaskStore { + const selection = { workflowId: WF, stepIds: [] }; + return { + getTask: vi.fn(async () => current), + listTasks: vi.fn(async () => [current]), + updateTask: vi.fn(async () => current), + moveTask: vi.fn(async () => current), + logEntry: vi.fn(async () => undefined), + recordRunAuditEvent: vi.fn(async () => undefined), + getTaskWorkflowSelection: vi.fn(() => selection), + getTaskWorkflowSelectionAsync: vi.fn(async () => selection), + getWorkflowDefinition: vi.fn(async () => (ir ? { ir } : null)), + } as unknown as TaskStore; +} + +describe("merger-ai under a renamed column vocabulary", () => { + describe("already-finalized short circuit", () => { + it("short-circuits a card resting in the RENAMED complete column", async () => { + /* Under the literal this threw "task is in 'shipped', must be in + 'in-review'" instead of reporting the card was already finalized. */ + const current = task({ column: "shipped" }); + const store = storeWith(current, renamedIr()); + + const result = await runAiMerge(store, "/tmp/root", "FN-1"); + + expect(result.noOp).toBe(true); + expect(result.ok).toBe(true); + expect(result.reason).toBe("already-finalized"); + }); + + it("short-circuits a card resting in the RENAMED archived column", async () => { + const current = task({ column: "retired" }); + const store = storeWith(current, renamedIr()); + + const result = await runAiMerge(store, "/tmp/root", "FN-1"); + + expect(result.noOp).toBe(true); + expect(result.reason).toBe("already-finalized"); + }); + + it("does NOT short-circuit a card still in the renamed merge lane", async () => { + /* The negative half: otherwise a conversion that returned "finalized" for + everything would pass the two tests above. */ + const current = task({ column: "reviewing" }); + const store = storeWith(current, renamedIr()); + + // It must get PAST the guard — whatever it then fails on is not this + // test's concern, only that it did not report already-finalized. + const result = await runAiMerge(store, "/tmp/root", "FN-1").catch((e: unknown) => e); + + const reason = (result as { reason?: string })?.reason; + expect(reason).not.toBe("already-finalized"); + }); + + it.each(["done", "archived"] as const)( + "still short-circuits the builtin workflow's %s column (regression floor)", + async (column) => { + const current = task({ column }); + const store = storeWith(current, defaultIr()); + + const result = await runAiMerge(store, "/tmp/root", "FN-1"); + + expect(result.noOp).toBe(true); + expect(result.reason).toBe("already-finalized"); + }, + ); + + it("still short-circuits done/archived when the workflow cannot be resolved", async () => { + /* Conservative fallback — an unresolvable workflow must keep the legacy + terminal ids rather than losing the guard entirely. Losing it would + re-merge a finished card, which is the worse direction to fail. */ + const current = task({ column: "done" }); + const store = storeWith(current, undefined); + + const result = await runAiMerge(store, "/tmp/root", "FN-1"); + + expect(result.reason).toBe("already-finalized"); + }); + + /* + FNXC:WorkflowLifecycleColumns 2026-07-28-02:10 (PR #2471 review, P1): + PARTIAL role declaration. The first cut of `isAlreadyFinalizedColumn` + swapped the legacy pair for the resolved set WHOLESALE — `if + (resolved.length > 0) terminal = resolved`. A workflow declaring `complete` + but not `archived` therefore resolved to a ONE-element set and silently + dropped the archived short-circuit: an archived card fell through to + `getTaskMergeBlocker` and threw "must be in 'in-review'". + + The fallback has to be PER-ROLE, not per-set — `complete` falls back to + `done` and `archived` falls back to `archived` independently — so a + partially-declared workflow keeps both halves of the guard. Both directions + are covered below because the two roles fail independently and a per-set fix + would pass whichever one happened to be declared. + */ + function partialIr(columns: Array>): WorkflowIr { + return { + version: "v2", + id: WF, + nodes: [], + edges: [], + columns: [ + { id: "drafting", label: "Drafting", traits: [{ trait: "hold", config: { release: "capacity" } }] }, + { id: "reviewing", label: "Reviewing", traits: [{ trait: "mergeOrchestration" }] }, + ...columns, + ], + } as unknown as WorkflowIr; + } + + it("keeps the legacy archived fallback when the workflow declares complete but NOT archived", async () => { + const ir = partialIr([{ id: "shipped", label: "Shipped", traits: [{ trait: "complete" }] }]); + + // The declared role still works… + const shipped = storeWith(task({ column: "shipped" }), ir); + expect((await runAiMerge(shipped, "/tmp/root", "FN-1")).reason).toBe("already-finalized"); + + // …and the UNdeclared one must not be lost with it. + const archived = storeWith(task({ column: "archived" }), ir); + expect((await runAiMerge(archived, "/tmp/root", "FN-1")).reason).toBe("already-finalized"); + }); + + it("keeps the legacy done fallback when the workflow declares archived but NOT complete", async () => { + const ir = partialIr([{ id: "retired", label: "Retired", traits: [{ trait: "archived" }] }]); + + const retired = storeWith(task({ column: "retired" }), ir); + expect((await runAiMerge(retired, "/tmp/root", "FN-1")).reason).toBe("already-finalized"); + + const done = storeWith(task({ column: "done" }), ir); + expect((await runAiMerge(done, "/tmp/root", "FN-1")).reason).toBe("already-finalized"); + }); + + it("does not treat a non-terminal column as finalized under a partial workflow", async () => { + /* The negative half: a per-role fallback must not widen the guard into + "anything not explicitly non-terminal is finalized". */ + const ir = partialIr([{ id: "shipped", label: "Shipped", traits: [{ trait: "complete" }] }]); + const store = storeWith(task({ column: "reviewing" }), ir); + + const result = await runAiMerge(store, "/tmp/root", "FN-1").catch((e: unknown) => e); + + expect((result as { reason?: string })?.reason).not.toBe("already-finalized"); + }); + }); + + describe("finalize-blocked rebound target", () => { + it("resolves the workflow's hold column, not the literal todo", async () => { + const store = storeWith(task(), renamedIr()); + await expect(resolveFinalizeReboundColumn(store, "FN-1")).resolves.toBe("drafting"); + }); + + it("resolves todo for the builtin coding workflow (regression floor)", async () => { + const store = storeWith(task(), defaultIr()); + await expect(resolveFinalizeReboundColumn(store, "FN-1")).resolves.toBe("todo"); + }); + + it("falls back to the legacy todo when the workflow cannot be resolved", async () => { + const store = storeWith(task(), undefined); + await expect(resolveFinalizeReboundColumn(store, "FN-1")).resolves.toBe("todo"); + }); + + it("falls back to the legacy todo when resolution throws", async () => { + /* A finalize-blocked rebound parks work for an operator. It must not be + abandoned because a workflow lookup failed — the card would otherwise + be left stranded in the merge lane with no owner. */ + const store = { + ...storeWith(task(), renamedIr()), + getTaskWorkflowSelectionAsync: vi.fn(async () => { + throw new Error("boom"); + }), + getWorkflowDefinition: vi.fn(async () => { + throw new Error("boom"); + }), + } as unknown as TaskStore; + + await expect(resolveFinalizeReboundColumn(store, "FN-1")).resolves.toBe("todo"); + }); + }); +}); diff --git a/packages/engine/src/__tests__/mesh-lease-manager-renamed-columns.test.ts b/packages/engine/src/__tests__/mesh-lease-manager-renamed-columns.test.ts new file mode 100644 index 0000000000..9b539e5d1b --- /dev/null +++ b/packages/engine/src/__tests__/mesh-lease-manager-renamed-columns.test.ts @@ -0,0 +1,198 @@ +/* +FNXC:WorkflowLifecycleColumns 2026-07-27-23:20 (Phase B / slice B2): + +MeshLeaseManager recovers a task whose owning node abandoned its lease by +clearing the local lease and rebounding the card to the backlog. Four literal +sites decided where "the backlog" is: + + - `if (task.column !== "todo")` — the already-parked guard + - `moveTask(task.id, "todo", …)` — the rebound target + - `decisionPath: … "lease-recovered-in-place" : "lease-recovered-to-todo"` + - `newColumn: … task.column : "todo"` — the audit's record of where it landed + +Under a workflow with no `todo` column the first three misbehave TOGETHER and +quietly: the guard says "not already parked" (true, but for the wrong reason), +and the move then targets a column id the workflow does not define. The audit +meanwhile asserts the card landed in `todo` regardless of what actually +happened, so the run-audit trail — the only post-hoc record of a lease recovery +— records a column that does not exist. + +These tests were written against the literal implementation and observed +FAILING first. The rebound target is the KTD-10 `resolveReboundTarget` ordering +(hold → intake → first column) already used by self-healing.ts:714, not a new +rule invented here. +*/ +import { describe, expect, it, vi } from "vitest"; +import type { RunAuditEventInput, Task, TaskStore, WorkflowIr } from "@fusion/core"; + +import { MeshLeaseManager } from "../mesh-lease-manager.js"; + +const WF = "custom:wf"; + +function task(overrides: Partial = {}): Task { + return { + id: "FN-1", + description: "x", + column: "in-progress", + dependencies: [], + steps: [], + currentStep: 0, + log: [], + createdAt: "2026-05-01T00:00:00.000Z", + updatedAt: "2026-05-01T00:00:00.000Z", + checkedOutBy: "agent-1", + checkedOutAt: "2026-05-01T00:00:00.000Z", + checkoutLeaseRenewedAt: "2026-05-01T00:00:00.000Z", + checkoutLeaseEpoch: 1, + checkoutNodeId: "node-a", + ...overrides, + } as Task; +} + +/** A workflow whose hold column is `drafting` — there is NO `todo` column. */ +function renamedIr(): WorkflowIr { + return { + version: "v2", + id: WF, + nodes: [], + edges: [], + columns: [ + { id: "inbox", label: "Inbox", traits: [{ trait: "intake" }] }, + { 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" }] }, + ], + } as unknown as WorkflowIr; +} + +/** The builtin coding shape, for the byte-identical regression floor. */ +function defaultIr(): WorkflowIr { + return { + version: "v2", + id: WF, + nodes: [], + edges: [], + columns: [ + { id: "triage", label: "Triage", traits: [{ trait: "intake" }] }, + { id: "todo", label: "Todo", traits: [{ trait: "hold", config: { release: "capacity" } }] }, + { id: "in-progress", label: "In Progress", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] }, + { id: "done", label: "Done", traits: [{ trait: "complete" }] }, + ], + } as unknown as WorkflowIr; +} + +function harness(currentTask: Task, ir: WorkflowIr | undefined) { + const recordRunAuditEvent = vi.fn().mockResolvedValue(undefined); + const moveTask = vi.fn().mockResolvedValue(currentTask); + const selection = { workflowId: WF, stepIds: [] }; + + const taskStore = { + getTask: vi.fn().mockResolvedValue(currentTask), + updateTask: vi.fn().mockResolvedValue(currentTask), + moveTask, + logEntry: vi.fn().mockResolvedValue(undefined), + recordRunAuditEvent, + getTaskWorkflowSelection: vi.fn(() => selection), + getTaskWorkflowSelectionAsync: vi.fn(async () => selection), + // `undefined` ir models a workflow that cannot be resolved at all. + getWorkflowDefinition: vi.fn(async () => (ir ? { ir } : null)), + } as unknown as TaskStore; + + const manager = new MeshLeaseManager({ + taskStore, + nodeHealthMonitor: { getNodeHealth: () => "offline" } as never, + getHandoffPolicy: vi.fn().mockResolvedValue("reassign-any-healthy"), + localNodeId: "local", + }); + + const unreachableEvent = () => + recordRunAuditEvent.mock.calls + .map((call) => call[0] as RunAuditEventInput) + .find((c) => c.mutationType === "task:auto-recover-node-unreachable"); + + return { manager, moveTask, unreachableEvent }; +} + +describe("MeshLeaseManager lease rebound under a renamed column vocabulary", () => { + it("rebounds an abandoned lease to the workflow's HOLD column, not the literal todo", async () => { + const current = task({ column: "building" }); + const h = harness(current, renamedIr()); + + const ok = await h.manager.recoverAbandonedLease("FN-1", "stale lease"); + + expect(ok).toBe(true); + // The failure this pins: the card was previously shoved into a column id the + // workflow does not define. + expect(h.moveTask).toHaveBeenCalledWith("FN-1", "drafting", expect.any(Object)); + expect(h.moveTask).not.toHaveBeenCalledWith("FN-1", "todo", expect.any(Object)); + }); + + it("records the column it ACTUALLY rebounded to in the unreachable audit", async () => { + const current = task({ column: "building" }); + const h = harness(current, renamedIr()); + + await h.manager.recoverAbandonedLease("FN-1", "stale lease"); + + /* The audit is the only post-hoc record of a lease recovery; asserting + `todo` when the move went elsewhere makes it actively misleading. */ + expect(h.unreachableEvent()?.metadata).toMatchObject({ + previousColumn: "building", + newColumn: "drafting", + decisionPath: "lease-recovered-to-todo", + }); + }); + + it("treats a card already AT the renamed hold column as recovered in place", async () => { + const current = task({ column: "drafting" }); + const h = harness(current, renamedIr()); + + await h.manager.recoverAbandonedLease("FN-1", "stale lease"); + + // No redundant move, and the audit says in-place rather than claiming a move. + expect(h.moveTask).not.toHaveBeenCalled(); + expect(h.unreachableEvent()?.metadata).toMatchObject({ + newColumn: "drafting", + decisionPath: "lease-recovered-in-place", + }); + }); + + it("falls back to the hold column when the card sits in intake", async () => { + /* Intake is not the rebound target — KTD-10 prefers hold, and only falls + back to intake when the workflow declares no hold column. */ + const current = task({ column: "inbox" }); + const h = harness(current, renamedIr()); + + await h.manager.recoverAbandonedLease("FN-1", "stale lease"); + + expect(h.moveTask).toHaveBeenCalledWith("FN-1", "drafting", expect.any(Object)); + }); + + it("keeps the legacy todo target when the workflow cannot be resolved", async () => { + /* Conservative fallback: an unresolvable workflow must behave exactly as it + did before this conversion rather than guess. */ + const current = task({ column: "in-progress" }); + const h = harness(current, undefined); + + await h.manager.recoverAbandonedLease("FN-1", "stale lease"); + + expect(h.moveTask).toHaveBeenCalledWith("FN-1", "todo", expect.any(Object)); + expect(h.unreachableEvent()?.metadata).toMatchObject({ + newColumn: "todo", + decisionPath: "lease-recovered-to-todo", + }); + }); + + it("is byte-identical for the builtin coding workflow (regression floor)", async () => { + const current = task({ column: "in-progress" }); + const h = harness(current, defaultIr()); + + await h.manager.recoverAbandonedLease("FN-1", "stale lease"); + + expect(h.moveTask).toHaveBeenCalledWith("FN-1", "todo", expect.any(Object)); + expect(h.unreachableEvent()?.metadata).toMatchObject({ + previousColumn: "in-progress", + newColumn: "todo", + decisionPath: "lease-recovered-to-todo", + }); + }); +}); diff --git a/packages/engine/src/__tests__/task-agent-sync-renamed-columns.test.ts b/packages/engine/src/__tests__/task-agent-sync-renamed-columns.test.ts new file mode 100644 index 0000000000..d6e8bb41af --- /dev/null +++ b/packages/engine/src/__tests__/task-agent-sync-renamed-columns.test.ts @@ -0,0 +1,195 @@ +/* +FNXC:WorkflowLifecycleColumns 2026-07-27-22:40 (Phase B / slice B2): + +task-agent-sync clears an agent's `taskId` link when its task moves to a column +where the agent is no longer working it. Both halves of that decision were keyed +on literal column ids: + + - CLEAR_COLUMNS = {done, archived, todo, triage} — the roles complete, + archived, hold and intake. + - isParkedTaskColumn = {todo, triage} — the roles hold and intake, where a + link is preserved if (and only if) there is live execution proof. + +Under a renamed workflow neither set matches, and the failure is SILENT in the +worst direction: the handler returns early, so an agent keeps a stale `taskId` +pointing at a card it is no longer working — indefinitely, with no error and no +failing test. The agent also stays `running`. These tests were written against +the literal implementation and observed FAILING first. + +The role mapping is asserted, not assumed: hold/intake preserve-with-proof, +complete/archived always clear. A renamed workflow must behave identically to +the default-named one role-for-role. +*/ +import { describe, expect, it, vi } from "vitest"; +import type { Agent, AgentStore, Task, TaskStore, WorkflowIr } from "@fusion/core"; + +import { attachAgentLinkSync, isParkedTaskColumn } from "../task-agent-sync.js"; + +const WF = "custom:wf"; + +/** Same workflow SHAPE under two vocabularies; only the ids differ. */ +function ir(names: Record<"intake" | "hold" | "wip" | "complete" | "archived", string>): WorkflowIr { + return { + version: "v2", + id: WF, + nodes: [], + edges: [], + columns: [ + { id: names.intake, label: "Intake", traits: [{ trait: "intake" }] }, + { id: names.hold, label: "Hold", traits: [{ trait: "hold", config: { release: "capacity" } }] }, + { id: names.wip, label: "Wip", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] }, + { id: names.complete, label: "Complete", traits: [{ trait: "complete" }] }, + { id: names.archived, label: "Archived", traits: [{ trait: "archived" }] }, + ], + } as unknown as WorkflowIr; +} + +const DEFAULT_NAMES = { + intake: "triage", + hold: "todo", + wip: "in-progress", + complete: "done", + archived: "archived", +}; +/* No renamed id collides with a legacy literal, so a surviving `=== "todo"` + cannot pass by coincidence. */ +const RENAMED = { + intake: "inbox", + hold: "drafting", + wip: "building", + complete: "shipped", + archived: "retired", +}; + +interface Harness { + store: TaskStore; + agentStore: AgentStore; + agent: Agent; + emitMove: (to: string) => Promise; + syncCalls: () => Array; + stateCalls: () => string[]; +} + +function harness(names: typeof DEFAULT_NAMES, opts: { hasFreshRun?: boolean } = {}): Harness { + const agent = { id: "A1", taskId: "FN-1", state: "running" } as Agent; + const selection = { workflowId: WF, stepIds: [] }; + const syncCalls: Array = []; + const stateCalls: string[] = []; + let handler: ((e: { task: { id: string }; from: string; to: string }) => Promise) | undefined; + + const store = { + on: vi.fn((_evt: string, h: typeof handler) => { + handler = h; + }), + off: vi.fn(), + getTaskWorkflowSelection: vi.fn(() => selection), + getTaskWorkflowSelectionAsync: vi.fn(async () => selection), + getWorkflowDefinition: vi.fn(async () => ({ ir: ir(names) })), + getTask: vi.fn(async () => ({ id: "FN-1", column: names.hold }) as Task), + } as unknown as TaskStore; + + const agentStore = { + listAgents: vi.fn(async () => [agent]), + // A fresh run only when the scenario asks for live execution proof. + getActiveHeartbeatRun: vi.fn(async () => + opts.hasFreshRun ? { startedAt: new Date().toISOString() } : null, + ), + updateAgentState: vi.fn(async (_id: string, state: string) => { + stateCalls.push(state); + }), + syncExecutionTaskLink: vi.fn(async (_id: string, taskId: string | undefined) => { + syncCalls.push(taskId); + }), + } as unknown as AgentStore; + + attachAgentLinkSync({ store, agentStore, logger: { log: () => {}, warn: () => {} } }); + + return { + store, + agentStore, + agent, + emitMove: async (to: string) => { + await handler?.({ task: { id: "FN-1" }, from: names.wip, to }); + }, + syncCalls: () => syncCalls, + stateCalls: () => stateCalls, + }; +} + +describe("task-agent-sync under a renamed column vocabulary", () => { + describe("isParkedTaskColumn", () => { + it("treats the resolved hold and intake columns as parked", () => { + // Explicitly-supplied roles: the renamed columns are parked… + expect(isParkedTaskColumn({ column: RENAMED.hold }, [RENAMED.hold, RENAMED.intake])).toBe(true); + expect(isParkedTaskColumn({ column: RENAMED.intake }, [RENAMED.hold, RENAMED.intake])).toBe(true); + // …and a wip column is not, under either vocabulary. + expect(isParkedTaskColumn({ column: RENAMED.wip }, [RENAMED.hold, RENAMED.intake])).toBe(false); + }); + + it("keeps the legacy todo/triage default when no roles are supplied", () => { + // Byte-identical for every caller that cannot resolve a workflow. + expect(isParkedTaskColumn({ column: "todo" })).toBe(true); + expect(isParkedTaskColumn({ column: "triage" })).toBe(true); + expect(isParkedTaskColumn({ column: "in-progress" })).toBe(false); + expect(isParkedTaskColumn(null)).toBe(false); + }); + }); + + describe("link clearing on move", () => { + it("clears the agent link when a card moves to a RENAMED complete column", async () => { + const h = harness(RENAMED); + await h.emitMove(RENAMED.complete); + // The stale-link failure mode: under the literal set this move is ignored + // entirely and the agent keeps pointing at a finished card. + expect(h.syncCalls()).toEqual([undefined]); + expect(h.stateCalls()).toEqual(["active"]); + }); + + it("clears the agent link when a card moves to a RENAMED archived column", async () => { + const h = harness(RENAMED); + await h.emitMove(RENAMED.archived); + expect(h.syncCalls()).toEqual([undefined]); + }); + + it("clears the link on a move to a RENAMED hold column with no live execution proof", async () => { + const h = harness(RENAMED, { hasFreshRun: false }); + await h.emitMove(RENAMED.hold); + expect(h.syncCalls()).toEqual([undefined]); + }); + + it("PRESERVES the link on a move to a RENAMED hold column with a fresh run", async () => { + /* The parked-link protection must survive a rename too — otherwise the + conversion would trade a stale-link bug for a dropped-link bug. */ + const h = harness(RENAMED, { hasFreshRun: true }); + await h.emitMove(RENAMED.hold); + expect(h.syncCalls()).toEqual([]); + expect(h.stateCalls()).toEqual([]); + }); + + it("PRESERVES the link on a move to a RENAMED intake column with a fresh run", async () => { + const h = harness(RENAMED, { hasFreshRun: true }); + await h.emitMove(RENAMED.intake); + expect(h.syncCalls()).toEqual([]); + }); + + it("ignores a move into a wip column, which is not a clearing role", async () => { + const h = harness(RENAMED); + await h.emitMove(RENAMED.wip); + expect(h.syncCalls()).toEqual([]); + }); + + it("behaves identically under the default vocabulary (regression floor)", async () => { + const complete = harness(DEFAULT_NAMES); + await complete.emitMove(DEFAULT_NAMES.complete); + expect(complete.syncCalls()).toEqual([undefined]); + + const parkedWithProof = harness(DEFAULT_NAMES, { hasFreshRun: true }); + await parkedWithProof.emitMove(DEFAULT_NAMES.hold); + expect(parkedWithProof.syncCalls()).toEqual([]); + + const parkedNoProof = harness(DEFAULT_NAMES, { hasFreshRun: false }); + await parkedNoProof.emitMove(DEFAULT_NAMES.hold); + expect(parkedNoProof.syncCalls()).toEqual([undefined]); + }); + }); +}); diff --git a/packages/engine/src/hold-release.ts b/packages/engine/src/hold-release.ts index 354dd925a6..f000e89593 100644 --- a/packages/engine/src/hold-release.ts +++ b/packages/engine/src/hold-release.ts @@ -321,6 +321,20 @@ function resolveReleaseTarget(ir: WorkflowIr, fromColumn: string, preferCapacity // ── Dependency satisfaction (KTD-5 + FN-5719 dual-accept) ───────────────────── +/* +FNXC:WorkflowLifecycleColumns 2026-07-28-00:20 (Phase B / slice B2) DELIBERATE-LITERAL: +Reviewed as a U5 conversion candidate and deliberately NOT converted. This is the LEGACY +half of the FN-5719 dual-accept pair in `dependencySatisfied` below: the trait half already +handles renamed workflows, and this half exists specifically to honor the pre-trait +completion signal and to log a `merge:dependency-parity-diff` audit event when the two +disagree. Converting it to traits would make both halves compute the same answer — deleting +the compatibility signal AND the divergence detector in one move, while looking like a +cleanup. The literal IS the semantic here. + +Its removal is the dual-accept window CLOSING, which is U12's call, not a Phase B refactor. +Recorded for the U12 literal ratchet's allowlist; that ratchet does not exist in the tree +yet, so grep `DELIBERATE-LITERAL` to enumerate the sites it must admit. +*/ /** Legacy completion signal: dependency's column is a terminal/handoff column. */ function legacyDependencySatisfied(dep: Task): boolean { return dep.column === "done" || dep.column === "in-review" || dep.column === "archived"; diff --git a/packages/engine/src/merger-ai.ts b/packages/engine/src/merger-ai.ts index 9bec6abe96..0d6abe39f7 100644 --- a/packages/engine/src/merger-ai.ts +++ b/packages/engine/src/merger-ai.ts @@ -52,6 +52,9 @@ import { resolveTaskMergeTarget, resolveValidatorSettingsModel, resolveMergerFallbackModel, + resolveReboundTarget, + resolveLifecycleColumns, + resolveWorkflowIrForTask, type MergeDetails, type MergeResult, type MergeTargetResolution, @@ -984,6 +987,81 @@ export async function landOneRepo( // Orchestrator // --------------------------------------------------------------------------- +/* +FNXC:WorkflowLifecycleColumns 2026-07-27-23:50 (Phase B / U5): +Legacy ids for the roles this module decides by: the builtin coding workflow's +`complete`/`archived` terminal pair and its `hold` rebound column. Used only +when the task's workflow resolves to no column vocabulary, where preserving +today's behavior exactly beats guessing. +*/ +const LEGACY_COMPLETE_COLUMN = "done"; +const LEGACY_ARCHIVED_COLUMN = "archived"; +/* The pair, for the no-vocabulary-at-all case. Derived from the per-role ids so + the set and the individual fallbacks cannot drift apart. */ +const LEGACY_TERMINAL_COLUMNS: readonly string[] = [LEGACY_COMPLETE_COLUMN, LEGACY_ARCHIVED_COLUMN]; +const LEGACY_REBOUND_COLUMN = "todo"; + +/** + * Where a finalize-blocked card is returned to for operator review. + * + * KTD-10 ordering via `resolveReboundTarget` (hold → intake → first column) — + * the same helper self-healing.ts:714 and mesh-lease-manager use for "requeue a + * recovered card", so the recovery paths cannot drift apart. + * + * Fail-soft to the legacy literal: these rebounds PARK WORK for a human after a + * no-commits / no-landed-proof / vetoed-no-op guard fires. Abandoning the + * rebound because a workflow lookup failed would strand the card in the merge + * lane with no owner, which is strictly worse than rebounding to a stale id. + * + * Exported for direct testing: the four call sites sit deep inside `runAiMerge` + * and `landWorkspaceTask`, behind a real git repo and a full merge run. + */ +export async function resolveFinalizeReboundColumn(store: TaskStore, taskId: string): Promise { + try { + const ir = await resolveWorkflowIrForTask(store, taskId); + return resolveReboundTarget(ir) ?? LEGACY_REBOUND_COLUMN; + } catch { + return LEGACY_REBOUND_COLUMN; + } +} + +/** + * True when the card already rests in a terminal column (`complete` or + * `archived`) of its OWN workflow — the already-finalized short circuit. + * + * Fail-soft to the legacy pair: losing this guard means an already-finalized + * card proceeds into the merge path, so an unresolvable workflow must keep the + * legacy ids rather than answer "not terminal". + */ +async function isAlreadyFinalizedColumn(store: TaskStore, task: Task): Promise { + let terminal: readonly string[] = LEGACY_TERMINAL_COLUMNS; + try { + const lifecycle = resolveLifecycleColumns(await resolveWorkflowIrForTask(store, task.id)); + if (lifecycle) { + /* + FNXC:WorkflowLifecycleColumns 2026-07-28-02:10 (PR #2471 review, P1): + The fallback is PER-ROLE, not per-set. The first cut replaced the whole + legacy pair whenever ANY terminal role resolved, so a workflow declaring + `complete` but no `archived` collapsed to a one-element set and silently + lost the archived short-circuit — an archived card then fell through to + `getTaskMergeBlocker` and threw "must be in 'in-review'". + + Resolving each role against its OWN legacy id keeps both halves of the + guard for a partially-declared workflow. The two roles are independent: + a per-set rule passes for whichever role happens to be declared and fails + for the other, which is why both directions are tested. + */ + terminal = [ + lifecycle.complete ?? LEGACY_COMPLETE_COLUMN, + lifecycle.archived ?? LEGACY_ARCHIVED_COLUMN, + ]; + } + } catch { + terminal = LEGACY_TERMINAL_COLUMNS; + } + return terminal.includes(task.column); +} + function noOpResult(task: Task, branch: string, reason: string): MergeResult { return { task, @@ -1092,7 +1170,15 @@ export async function runAiMerge( assertNotWorkspaceTaskMerge(task); const branch = resolveTaskWorkingBranch(task); - if (task.column === "done" || task.column === "archived") { + /* + FNXC:WorkflowLifecycleColumns 2026-07-27-23:50 (Phase B / U5): + Resolve the terminal roles from the task's own workflow. Under a renamed + workflow the literal `done`/`archived` pair stopped matching, and the + already-finalized card fell through to `getTaskMergeBlocker` — which threw + "task is in 'shipped', must be in 'in-review'" for a task whose real state was + "already done, nothing to do". The correct outcome is this clean no-op. + */ + if (await isAlreadyFinalizedColumn(store, task)) { return noOpResult(task, branch, "already-finalized"); } const blocker = getTaskMergeBlocker(task, { manual: options.manual === true }); @@ -1218,9 +1304,10 @@ export async function runAiMerge( * FN-6461/FN-6455 requires the AI empty-merge lane to demote no-commits tasks whose skipped/incomplete steps outweigh done steps instead of finalizing the operational work as done. */ await store.updateTask(taskId, { error: reason }); + const reboundColumn = await resolveFinalizeReboundColumn(store, taskId); await store.logEntry( taskId, - `Finalize blocked (no-commits incomplete-work guard): ${reason} — moving back to todo with progress preserved`, + `Finalize blocked (no-commits incomplete-work guard): ${reason} — moving back to ${reboundColumn} with progress preserved`, JSON.stringify({ doneCount: noCommitsFinalize.doneCount, incompleteCount: noCommitsFinalize.incompleteCount, @@ -1241,7 +1328,7 @@ export async function runAiMerge( lane: "ai-empty-merge", }, }); - await store.moveTask(taskId, "todo", { preserveProgress: true, moveSource: "engine" } as Parameters[2]); + await store.moveTask(taskId, reboundColumn, { preserveProgress: true, moveSource: "engine" } as Parameters[2]); return { task, branch, @@ -1277,9 +1364,10 @@ export async function runAiMerge( const reason = "branch had no net changes vs main — work may have been reverted or lost; operator review required"; await store.updateTask(taskId, { error: reason }); + const reboundColumn = await resolveFinalizeReboundColumn(store, taskId); await store.logEntry( taskId, - `Finalize blocked (empty-merge no-landed-proof guard): ${reason} — moving back to todo with progress preserved`, + `Finalize blocked (empty-merge no-landed-proof guard): ${reason} — moving back to ${reboundColumn} with progress preserved`, JSON.stringify({ branch, integrationBranch, lane: "ai-empty-merge", baseCommitSha: task.baseCommitSha }, null, 2), ); await audit.database({ @@ -1294,7 +1382,7 @@ export async function runAiMerge( hadPriorNoOpProof: false, }, }); - await store.moveTask(taskId, "todo", { preserveProgress: true, moveSource: "engine" } as Parameters[2]); + await store.moveTask(taskId, reboundColumn, { preserveProgress: true, moveSource: "engine" } as Parameters[2]); return { task, branch, @@ -1342,9 +1430,10 @@ export async function runAiMerge( if (executorVeto.veto) { const vetoReason = executorVeto.reason ?? "overseer failed-executor no-op-finalize veto"; await store.updateTask(taskId, { error: vetoReason }); + const reboundColumn = await resolveFinalizeReboundColumn(store, taskId); await store.logEntry( taskId, - `Finalize blocked (overseer failed-executor veto): ${vetoReason} — moving back to todo with progress preserved`, + `Finalize blocked (overseer failed-executor veto): ${vetoReason} — moving back to ${reboundColumn} with progress preserved`, JSON.stringify({ executorSignal: executorMemory?.signal, executorSignalObservedAt: executorMemory?.observedAt, @@ -1365,7 +1454,7 @@ export async function runAiMerge( lane: "ai-empty-merge", }, }); - await store.moveTask(taskId, "todo", { preserveProgress: true, moveSource: "engine" } as Parameters[2]); + await store.moveTask(taskId, reboundColumn, { preserveProgress: true, moveSource: "engine" } as Parameters[2]); return { task, branch, @@ -1904,9 +1993,10 @@ export async function landWorkspaceTask( const reason = "branch had no net changes vs main — work may have been reverted or lost; operator review required"; await store.updateTask(taskId, { error: reason }); + const reboundColumn = await resolveFinalizeReboundColumn(store, taskId); await store.logEntry( taskId, - `Finalize blocked (empty-merge no-landed-proof guard, workspace): ${reason} — moving back to todo with progress preserved`, + `Finalize blocked (empty-merge no-landed-proof guard, workspace): ${reason} — moving back to ${reboundColumn} with progress preserved`, JSON.stringify({ lane: "ai-empty-merge-workspace", repoCount: repos.length, landedCount, repos: repos.map((r) => r.repo) }, null, 2), ).catch(() => undefined); await audit.database({ @@ -1914,7 +2004,7 @@ export async function landWorkspaceTask( target: taskId, metadata: { reason, lane: "ai-empty-merge-workspace", repoCount: repos.length, landedCount, hadPriorNoOpProof: false }, }).catch(() => undefined); - await store.moveTask(taskId, "todo", { preserveProgress: true, moveSource: "engine" } as Parameters[2]); + await store.moveTask(taskId, reboundColumn, { preserveProgress: true, moveSource: "engine" } as Parameters[2]); return { taskId, repos, allLanded, finalized: false }; } const finalized = await finalizeWorkspaceTask(store, taskId, task, repos); diff --git a/packages/engine/src/mesh-lease-manager.ts b/packages/engine/src/mesh-lease-manager.ts index c80c03f339..74bcee2bf3 100644 --- a/packages/engine/src/mesh-lease-manager.ts +++ b/packages/engine/src/mesh-lease-manager.ts @@ -1,3 +1,4 @@ +import { resolveReboundTarget, resolveWorkflowIrForTask } from "@fusion/core"; import type { AgentStore, CentralClaimStore, @@ -29,9 +30,40 @@ export interface LeaseRecoveryContext { preserveProgress?: boolean; } +/* +FNXC:WorkflowLifecycleColumns 2026-07-27-23:20 (Phase B / U5): +The legacy rebound target — the builtin coding workflow's hold column. Used only +when the task's workflow resolves to no column vocabulary at all, where the +conservative choice is to preserve today's behavior exactly rather than guess. +*/ +const LEGACY_REBOUND_COLUMN = "todo"; + export class MeshLeaseManager { constructor(private readonly options: MeshLeaseManagerOptions) {} + /* + FNXC:WorkflowLifecycleColumns 2026-07-27-23:20 (Phase B / U5): + Where a recovered lease rebounds to. KTD-10 ordering via `resolveReboundTarget` + (hold → intake → first column) — the same helper self-healing.ts:714 already + uses for "requeue a recovered card", so the two recovery paths cannot drift. + + Resolved ONCE per recovery and threaded to both the move and the audit + metadata. They were previously two independent `=== "todo"` comparisons that + could disagree, which is how the audit came to claim a card landed in `todo` + when the workflow has no such column. + + Fail-soft: any resolution failure falls back to the legacy literal, since a + lease recovery must not be abandoned because a workflow lookup failed. + */ + private async resolveReboundColumn(taskId: string): Promise { + try { + const ir = await resolveWorkflowIrForTask(this.options.taskStore, taskId); + return resolveReboundTarget(ir) ?? LEGACY_REBOUND_COLUMN; + } catch { + return LEGACY_REBOUND_COLUMN; + } + } + private staleThresholdMs(agentHeartbeatTimeoutMs?: number): number { return Math.max((agentHeartbeatTimeoutMs ?? 60_000) * 2, 120_000); } @@ -123,7 +155,13 @@ export class MeshLeaseManager { } } - private async clearLocalLease(task: Task, reason: string, context: LeaseRecoveryContext, nextEpoch: number): Promise { + private async clearLocalLease( + task: Task, + reason: string, + context: LeaseRecoveryContext, + nextEpoch: number, + reboundColumn: string, + ): Promise { await this.options.taskStore.updateTask( task.id, { @@ -142,8 +180,8 @@ export class MeshLeaseManager { `${reason}; epoch=${nextEpoch}`, context.runContext, ); - if (task.column !== "todo") { - await this.options.taskStore.moveTask(task.id, "todo", { + if (task.column !== reboundColumn) { + await this.options.taskStore.moveTask(task.id, reboundColumn, { preserveProgress: context.preserveProgress ?? (task.currentStep > 0 || task.steps.some((step) => step.status !== "pending")), @@ -446,11 +484,18 @@ export class MeshLeaseManager { } } + /* + FNXC:WorkflowLifecycleColumns 2026-07-27-23:20 (Phase B / U5): + Resolved once here so the move below and the unreachable audit further down + report the SAME column. Two independent resolutions could disagree. + */ + const reboundColumn = await this.resolveReboundColumn(task.id); + try { - await this.clearLocalLease(task, `${reason} (${stale.reason ?? "stale"})`, context, nextEpoch); + await this.clearLocalLease(task, `${reason} (${stale.reason ?? "stale"})`, context, nextEpoch, reboundColumn); } catch (_error) { try { - await this.clearLocalLease(task, `${reason} (${stale.reason ?? "stale"})`, context, nextEpoch); + await this.clearLocalLease(task, `${reason} (${stale.reason ?? "stale"})`, context, nextEpoch, reboundColumn); } catch (retryError) { if (this.options.centralClaimStore && this.options.projectId) { await this.emitLeaseAudit(task, "task:auto-recover-lease-partial-write", { @@ -502,8 +547,20 @@ export class MeshLeaseManager { if (isUnreachableOwnerReason) { await emitNodeUnreachableRecovery({ - decisionPath: task.column === "todo" ? "lease-recovered-in-place" : "lease-recovered-to-todo", - newColumn: task.column === "todo" ? task.column : "todo", + /* + FNXC:WorkflowLifecycleColumns 2026-07-27-23:20 (Phase B / U5): + Both fields read the SAME resolved `reboundColumn` the move used, so the + audit can no longer claim a landing column the card never reached. Note + `task` is the pre-move snapshot, so `task.column` is still the ORIGINAL + column here — that is what makes the in-place comparison meaningful. + + `decisionPath` keeps its legacy `lease-recovered-to-todo` wording: it is + a stable audit discriminator that existing queries and dashboards match + on, and renaming it would break them to describe the same decision. The + column that was actually used is carried by `newColumn`. + */ + decisionPath: task.column === reboundColumn ? "lease-recovered-in-place" : "lease-recovered-to-todo", + newColumn: reboundColumn, leaseEpoch: nextEpoch, recoveryReason: reason, handoffPolicy, diff --git a/packages/engine/src/replan-target.ts b/packages/engine/src/replan-target.ts index b9c7e0c8f2..1c0fc4352e 100644 --- a/packages/engine/src/replan-target.ts +++ b/packages/engine/src/replan-target.ts @@ -19,6 +19,17 @@ service scans, so parking a needs-replan card in their custom entry column stran — and the legacy move path throws on custom targets, aborting the replan before the status write. "triage" preserves the pre-workflow-aware behavior for these workflows: the move is legal from every legacy column and eligibleTriageTasks re-specifies unconditionally. + +FNXC:WorkflowLifecycleColumns 2026-07-28-00:20 (Phase B / slice B2) DELIBERATE-LITERAL: +Reviewed as a U5 conversion candidate and deliberately NOT converted. The value of this +fallback is precisely that it is NOT trait-resolved: it fires only when the workflow +declares neither planner column, and the whole point (documented above) is that resolving +it against the workflow — to that workflow's entry column — is the bug it was written to +fix. Converting it would reintroduce the stranded-card behavior. + +Recorded here for the U12 literal ratchet's allowlist. That ratchet does not exist in the +tree yet; grep `DELIBERATE-LITERAL` to enumerate the sites it must admit, with the reason +attached at the site rather than in a separate list that can drift from it. */ /* * FNXC:WorkflowReplan 2026-07-15-13:15: diff --git a/packages/engine/src/task-agent-sync.ts b/packages/engine/src/task-agent-sync.ts index 05bf74ed5f..63d6c2da27 100644 --- a/packages/engine/src/task-agent-sync.ts +++ b/packages/engine/src/task-agent-sync.ts @@ -1,4 +1,5 @@ -import type { Agent, AgentHeartbeatRun, AgentStore, Task, TaskStore } from "@fusion/core"; +import { resolveTaskLifecycleColumns } from "@fusion/core"; +import type { Agent, AgentHeartbeatRun, AgentStore, Task, TaskStore, WorkflowIr } from "@fusion/core"; export const PARKED_AGENT_LINK_FRESH_RUN_MS = 5 * 60_000; @@ -9,6 +10,65 @@ export interface AgentTaskLinkExecutionProof { runAgeMs: number; } +/* +FNXC:WorkflowLifecycleColumns 2026-07-27-22:55 (Phase B / U5): +The roles at which an agent's task link is CLEARED: terminal (`complete`, +`archived`) plus parked (`hold`, `intake`). Legacy default = the ids the builtin +coding workflow gives those four roles, used when the workflow cannot be +resolved — the conservative choice, since it preserves today's behavior exactly +rather than guessing a role for an unknown column. +*/ +/* +FNXC:WorkflowLifecycleColumns 2026-07-27-22:55 (Phase B / U5): +The legacy PARKED ids — the builtin coding workflow's `hold` and `intake` +columns. Exported because `isParkedTaskColumn` defaults to it for callers that +cannot resolve a workflow. +*/ +export const LEGACY_PARKED_COLUMNS: readonly string[] = ["todo", "triage"]; + +/* Terminal (`complete`, `archived`) plus parked. Derived from the parked list + rather than restated so the two legacy sets cannot drift apart. */ +const LEGACY_CLEAR_COLUMNS: readonly string[] = ["done", "archived", ...LEGACY_PARKED_COLUMNS]; + +interface LinkSyncColumnRoles { + /** Columns whose arrival clears the link (terminal + parked). */ + clear: readonly string[]; + /** The subset that is merely parked, where live execution proof preserves it. */ + parked: readonly string[]; +} + +const LEGACY_COLUMN_ROLES: LinkSyncColumnRoles = { + clear: LEGACY_CLEAR_COLUMNS, + parked: LEGACY_PARKED_COLUMNS, +}; + +/** + * Resolve the clearing/parked column roles for a task's own workflow, falling + * back to the legacy literal sets when the workflow has no column vocabulary. + * + * Fail-soft on purpose: this handler runs off a `task:moved` event and its only + * job is link hygiene. A resolution failure must not throw into the emitter, and + * degrading to the legacy sets keeps the builtin workflow correct while leaving + * a renamed workflow no worse off than before this conversion. + */ +async function resolveLinkSyncColumnRoles( + store: TaskStore, + taskId: string, + cache?: Map, +): Promise { + const lifecycle = await resolveTaskLifecycleColumns(store, taskId, cache); + if (!lifecycle) return LEGACY_COLUMN_ROLES; + + const parked = [lifecycle.hold, lifecycle.intake].filter((c): c is string => typeof c === "string"); + const terminal = [lifecycle.complete, lifecycle.archived].filter((c): c is string => typeof c === "string"); + const clear = [...terminal, ...parked]; + + // A v2 workflow declaring none of the four roles yields an empty clear set, + // which would silently disable link hygiene entirely. Prefer the legacy sets. + if (clear.length === 0) return LEGACY_COLUMN_ROLES; + return { clear, parked }; +} + export function hasFreshActiveHeartbeatRun( activeRun: AgentHeartbeatRun | null | undefined, now = Date.now(), @@ -22,8 +82,24 @@ export function hasFreshActiveHeartbeatRun( }; } -export function isParkedTaskColumn(task: Pick | null | undefined): boolean { - return task?.column === "todo" || task?.column === "triage"; +/* +FNXC:WorkflowLifecycleColumns 2026-07-27-22:55 (Phase B / U5): +"Parked" is the HOLD and INTAKE roles — a card resting before or between work, +not a card at the literal ids `todo`/`triage` (those are merely what the builtin +coding workflow calls those two columns). Under a renamed workflow the literal +check silently returned false for every card, which disabled the parked-link +preservation branch below rather than erroring. + +`parkedColumns` defaults to the legacy pair so every caller that cannot resolve +a workflow is byte-identical (R11 keeps `todo`/`triage` legal column ids). +Callers that can resolve pass the task's `hold` and `intake` roles. +*/ +export function isParkedTaskColumn( + task: Pick | null | undefined, + parkedColumns: readonly string[] = LEGACY_PARKED_COLUMNS, +): boolean { + if (!task?.column) return false; + return parkedColumns.includes(task.column); } export function evaluateParkedAgentTaskLink(options: { @@ -32,6 +108,15 @@ export function evaluateParkedAgentTaskLink(options: { activeRun?: AgentHeartbeatRun | null; hasActiveAgentExecution?: (agentId: string) => boolean; now?: number; + /* + FNXC:WorkflowLifecycleColumns 2026-07-27-22:55 (Phase B / U5): + The task's resolved parked (`hold` + `intake`) columns. Defaults to the legacy + pair so existing callers are byte-identical. Without this the preservation + branch consulted the legacy ids even when the CALLER had already resolved a + renamed workflow — turning a stale-link bug into a dropped-link bug, since the + card would be treated as unparked and its live agent link cleared. + */ + parkedColumns?: readonly string[]; }): AgentTaskLinkExecutionProof { const { hasFreshRun, runAgeMs } = hasFreshActiveHeartbeatRun(options.activeRun, options.now); const hasActiveExecution = options.hasActiveAgentExecution?.(options.agent.id) === true; @@ -42,7 +127,9 @@ export function evaluateParkedAgentTaskLink(options: { return { hasFreshRun, hasActiveExecution, - shouldPreserveParkedLink: isParkedTaskColumn(options.linkedTask) && (hasFreshRun || hasActiveExecution), + shouldPreserveParkedLink: + isParkedTaskColumn(options.linkedTask, options.parkedColumns ?? LEGACY_PARKED_COLUMNS) && + (hasFreshRun || hasActiveExecution), runAgeMs, }; } @@ -56,13 +143,27 @@ export interface AttachAgentLinkSyncOptions { logger?: LoggerLike; } -const CLEAR_COLUMNS = new Set(["done", "archived", "todo", "triage"]); - export function attachAgentLinkSync(opts: AttachAgentLinkSyncOptions): () => void { const logger: LoggerLike = opts.logger ?? console; const handler = async ({ task, from, to }: { task: { id: string }; from: string; to: string }) => { - if (!CLEAR_COLUMNS.has(to)) { + /* + FNXC:WorkflowLifecycleColumns 2026-07-27-22:55 (Phase B / U5): + Resolve the roles from the moved task's OWN workflow rather than matching + `to` against a fixed id set. Previously a move into a renamed terminal + column matched nothing and this handler returned early — so the agent kept a + `taskId` pointing at a finished card and stayed `running`, with no error and + no failing test. The IR read happens before the agent listing so an + unresolvable workflow still degrades to the legacy sets rather than throwing. + */ + let roles: LinkSyncColumnRoles; + try { + roles = await resolveLinkSyncColumnRoles(opts.store, task.id); + } catch { + roles = LEGACY_COLUMN_ROLES; + } + + if (!roles.clear.includes(to)) { return; } @@ -71,13 +172,14 @@ export function attachAgentLinkSync(opts: AttachAgentLinkSyncOptions): () => voi const linkedAgents = agents.filter((agent) => agent.taskId === task.id); for (const agent of linkedAgents) { - if (to === "todo" || to === "triage") { + if (roles.parked.includes(to)) { const activeRun = await opts.agentStore.getActiveHeartbeatRun?.(agent.id); const proof = evaluateParkedAgentTaskLink({ agent, linkedTask: { column: to } as Pick, activeRun, hasActiveAgentExecution: opts.hasActiveAgentExecution, + parkedColumns: roles.parked, }); if (proof.shouldPreserveParkedLink) { continue;