From a8cfce8fbd5d71092e363bf3c8667f852805b60e Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 12:02:41 -0700 Subject: [PATCH] =?UTF-8?q?executor:=20stale=20merge=20evidence=20re-enter?= =?UTF-8?q?ing=20execution,=20and=20a=20live=20checkout=20that=20read=20as?= =?UTF-8?q?=20unowned=20(12=20=E2=86=92=208)=20(#2805)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two executor conversions with real operator consequences, one census false positive, and three sites deliberately left with their reasons recorded. ## Census | file | main | here | | --- | ---: | ---: | | `packages/engine/src/executor.ts` | 12 | **8** | Of the 4: three genuine conversions, one reclassification. ## What was broken **`resetMergeStateIfNeeded` — cards re-entered execution carrying stale merge evidence.** Merge state is cleared when a card *leaves* a lane where a merge could have been recorded. Keyed on `in-review`/`done`, a renamed board matched neither, so a card bouncing back into execution kept `mergeDetails` — including a **commit sha from its previous pass** — into its next run. `review` is not a trait, so this resolves through the same five flags (`complete`, `mergeOrchestration`, `mergeBlocker`, `humanReview`) the dependency gates in this file already use; two gates answering "is this a merge-bearing lane?" differently would be a split brain. **The worktree-owner scan — a live checkout read as unowned.** `findActiveWorktreeOwner` asks "is anyone else working in this checkout?". Its in-memory `activeWorktrees` leg is vocabulary-independent, but the **durable** leg — the one that answers after an engine restart, when the in-memory map is empty — filtered with `t.column !== "in-progress"`. On a renamed board that matched nobody, so the worktree read as free and a second task could be handed a checkout another task is live in. Post-restart is exactly when this function matters. Not the query-filter class: that `listTasks` call passes no `column`, so the predicate is the only lane gate on the path. ## A third census false positive in this package Line 16094's `to` is a **review-addressing record status** — the method signature is `to: "queued" | "in-progress" | "addressed" | "failed"`, and the next two lines test it against `"addressed"` and `"failed"`, which are not columns at all. Marked `DELIBERATE-LITERAL`. That is the third in `packages/engine` after the two `cli-agent` `CliMachineState` ones (#2797). The backlog total includes non-columns; a sweep that "converts" them turns a status machine into a workflow role. ## Revert results (measured, each run independently) | conversion | reverted → | | --- | --- | | worktree-owner wip predicate | RENAMED case fails — checkout reads as **free** while another task is live in it | | `resetMergeStateIfNeeded` lanes | RENAMED case fails — card keeps `commitSha: "abc123"` from its previous pass | Both DEFAULT cases pass before and after, which is why both vocabularies run. Each has a non-vacuous companion (holder sitting in the complete lane; a return from the hold lane) so a predicate matching every column would not pass. **Both reach their seam directly through a cast.** The public routes are `handleBranchConflict` (needs a real `BranchConflictError` plus a git repo) and the `task:moved` listener (drags in the whole `execute()` path); going through either would make these tests about a git fixture rather than about the lane predicate. The alternative was the status quo — all 91 `executor-worktree*.test.ts` cases seed `column: "in-progress"`, so they assert the legacy fallback and pass either way. I shipped the conversions in one commit *stating* they were unproven, then closed that gap in the next; the history shows both. ### Two fake defects found while writing those tests Worth naming, because both are the documented green-for-the-wrong-reason shape: 1. The first fake had no `updateTask`, so the cleanup **threw** rather than asserting anything. 2. The second returned a new object without persisting — and `cleanupMergeStateForReverification` **re-reads through `getTask`**. The re-read handed back the stale row, so *both* vocabularies reported "nothing changed" and it would have read as a passing negative test. ## Deliberately NOT converted, with reasons - **The `task:moved` listener cluster** (`3521`/`3545`/`3596`/`3606`), including the AGENTS Move-Task hard-cancel contract `userCanceled: source === "user" && to === "todo"`. Its prologue is synchronous (`userCanceledTaskIds.delete`, watchdog clear) and deferring it to a microtask changes hard-cancel ordering. The sync IR reader is not an option — it returns the DEFAULT workflow for every task in production. A safe conversion needs lanes resolved on an earlier async boundary: new machinery plus an ordering change, which is out of fleet scope and not a guess worth making on a hard-cancel path. - **`17081`** pairs `latestColumn === "in-progress"` with a **hardcoded** `moveTask(taskId, "in-progress")` two lines above — census-invisible, the same shape as the branch-worktree requeue bug in #2797. They have to convert together, and the move needs the same rejection guard. - **`5903`** is the query-filter class: `listTasks({ column: "in-progress" })` followed by a re-assertion of the same literal. Converting it drops a count and changes nothing — see `docs/solutions/architecture-patterns/self-healing-sweeps-are-blind-on-a-renamed-board.md` (#2800). ## Verification - `pnpm test:gate` — 161 + 487 + 13 + 71, green - `npx tsc -p packages/engine/tsconfig.json --noEmit` — clean - `pnpm lint` — clean - `--strict` exits 0 --------- Co-authored-by: Claude Opus 5 (1M context) --- ...med-lane-merge-state-and-worktree-owner.md | 7 + ...or-stale-merge-state-and-worktree-owner.md | 7 + ...cutor-worktree-owner-renamed-lanes.test.ts | 171 ++++++++++++++++++ packages/engine/src/executor.ts | 47 ++++- .../lib/lifecycle-column-census-baseline.json | 3 +- 5 files changed, 232 insertions(+), 3 deletions(-) create mode 100644 .changeset/executor-renamed-lane-merge-state-and-worktree-owner.md create mode 100644 .changeset/executor-stale-merge-state-and-worktree-owner.md create mode 100644 packages/engine/src/__tests__/executor-worktree-owner-renamed-lanes.test.ts diff --git a/.changeset/executor-renamed-lane-merge-state-and-worktree-owner.md b/.changeset/executor-renamed-lane-merge-state-and-worktree-owner.md new file mode 100644 index 0000000000..14b0ecff81 --- /dev/null +++ b/.changeset/executor-renamed-lane-merge-state-and-worktree-owner.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Fix stale merge state and a duplicate worktree hand-off on boards with renamed columns. +category: fix +dev: Two executor guards compared lifecycle column ids literally. `resetMergeStateIfNeeded` clears merge state when a card leaves a lane where a merge could have been recorded (the review and complete roles); on a renamed board neither comparison matched, so a card re-entering execution carried stale `mergeDetails` from its previous pass. The worktree-owner scan asks "who else is actively working here?" — the WIP role — and matched nobody on a renamed board, so the worktree read as unowned and a second task could be handed a checkout already in use. Both now resolve from the task's own workflow. diff --git a/.changeset/executor-stale-merge-state-and-worktree-owner.md b/.changeset/executor-stale-merge-state-and-worktree-owner.md new file mode 100644 index 0000000000..dcbba80df3 --- /dev/null +++ b/.changeset/executor-stale-merge-state-and-worktree-owner.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Renamed-column boards no longer reuse an in-use checkout or carry stale merge details into a re-run. +category: fix +dev: `executor.ts` — `findActiveWorktreeOwner`'s durable leg and `resetMergeStateIfNeeded` compared `task.column` against the legacy ids, so on a renamed board the first matched nobody (a live checkout read as unowned after a restart) and the second never fired (a re-entering card kept its previous `mergeDetails`). Both now resolve from the task's own workflow, unioned with the legacy ids. diff --git a/packages/engine/src/__tests__/executor-worktree-owner-renamed-lanes.test.ts b/packages/engine/src/__tests__/executor-worktree-owner-renamed-lanes.test.ts new file mode 100644 index 0000000000..502ab22e57 --- /dev/null +++ b/packages/engine/src/__tests__/executor-worktree-owner-renamed-lanes.test.ts @@ -0,0 +1,171 @@ +/* +FNXC:WorkflowResolvedColumns 2026-07-30-17:05 (executor — the worktree-owner scan): +DIFFERENTIAL: one worktree path, one owning task, two column VOCABULARIES. + +`findActiveWorktreeOwner` answers "is anyone else actively working in this checkout?". Its in-memory +`activeWorktrees` leg is vocabulary-independent, but the DURABLE leg — the one that survives an engine +restart, when `activeWorktrees` is empty — filtered candidates with `t.column !== "in-progress"`. + +On a board whose wip lane is renamed that matched NOBODY, so the worktree read as UNOWNED and a second +task could be handed a checkout another task is live in. That is the failure mode this function exists to +prevent, and it is exactly the state after a restart. + +NOT the query-filter class: the `listTasks` call here passes no `column`, so the predicate is the only +lane gate on the path. (Contrast `executor.ts`'s in-progress limbo sweep, which IS query-bounded.) + +WHY IT REACHES THROUGH A PRIVATE: `findActiveWorktreeOwner` is private, and the public routes into it +(`handleBranchConflict`) need a real `BranchConflictError` and a git repo. Calling the seam directly keeps +this a test about the lane predicate instead of a git fixture — the alternative was no coverage at all, +which is what the conversion originally shipped with. + +REVERT CHECK, measured: with `if (t.column !== "in-progress") continue;` restored, the RENAMED case fails +— the owner comes back `null` and the checkout reads as free. The DEFAULT case passes both ways, which is +why both are run. +*/ +import { describe, expect, it, vi } from "vitest"; +import "./executor-test-helpers.js"; +import type { Task, TaskStore } from "@fusion/core"; +import { TaskExecutor } from "../executor.js"; +import { DEFAULT_VOCAB, RENAMED_VOCAB, lifecycleIr, type Vocabulary } from "./_workflow-vocabulary-fixture.js"; + +const WORKTREE = "/tmp/worktrees/FN-OWNER"; + +/** A card carrying merge evidence from a previous pass, as `resetMergeStateIfNeeded` looks for. */ +function mergedOnceTask(vocab: Vocabulary): Task { + return { + ...ownerTask(vocab), + id: "FN-REENTER", + column: vocab.wip, + mergeDetails: { commitSha: "abc123" }, + mergeRetries: 1, + } as unknown as Task; +} + +function ownerTask(vocab: Vocabulary): Task { + return { + id: "FN-OWNER", + title: "holds the checkout", + description: "", + column: vocab.wip, + worktree: WORKTREE, + dependencies: [], + steps: [], + currentStep: 0, + log: [], + createdAt: "2026-07-30T00:00:00.000Z", + updatedAt: "2026-07-30T00:00:00.000Z", + } as unknown as Task; +} + +function storeFor(seed: Task[], vocab: Vocabulary): TaskStore { + const tasks = [...seed]; + const ir = lifecycleIr(vocab, "executor-owner-lifecycle"); + return { + listTasks: vi.fn(async () => tasks), + getTask: vi.fn(async (id: string) => tasks.find((t) => t.id === id)), + getSettings: vi.fn(async () => ({})), + /* + `cleanupMergeStateForReverification` writes via updateTask and then RE-READS through getTask, so the + fake has to actually persist — a non-mutating updateTask hands the re-read a stale row and the test + sees "nothing changed" for the wrong reason. (It did, first time round.) + */ + updateTask: vi.fn(async (id: string, patch: Record) => { + const index = tasks.findIndex((t) => t.id === id); + tasks[index] = { ...tasks[index], ...patch } as unknown as Task; + return tasks[index]; + }), + logEntry: vi.fn(async () => undefined), + getTaskWorkflowSelectionAsync: vi.fn(async () => ({ workflowId: "executor-owner-lifecycle", stepIds: [] })), + getTaskWorkflowSelection: vi.fn(() => ({ workflowId: "executor-owner-lifecycle", stepIds: [] })), + getWorkflowDefinition: vi.fn(async (id: string) => (id === "executor-owner-lifecycle" ? { ir } : undefined)), + on: vi.fn(), + off: vi.fn(), + } as unknown as TaskStore; +} + +/** The private seam under test; see the header for why this is reached directly. */ +function findOwner(executor: TaskExecutor, requestingTaskId: string): Promise { + return (executor as unknown as { + findActiveWorktreeOwner: (worktreePath: string, requestingTaskId: string) => Promise; + }).findActiveWorktreeOwner(WORKTREE, requestingTaskId); +} + +describe("findActiveWorktreeOwner resolves the WIP lane by ROLE, not by id", () => { + for (const [label, vocab] of [["DEFAULT", DEFAULT_VOCAB], ["RENAMED", RENAMED_VOCAB]] as const) { + it(`sees the durable owner of a checkout on a ${label} wip lane (${vocab.wip})`, async () => { + const store = storeFor([ownerTask(vocab)], vocab); + const executor = new TaskExecutor(store, "/tmp/test", {}); + + // `activeWorktrees` is empty — the post-restart state, where only the durable leg can answer. + await expect(findOwner(executor, "FN-REQUESTER")).resolves.toBe("FN-OWNER"); + }); + } + + it("does not report an owner whose card has left the wip lane on a RENAMED board", async () => { + /* + Non-vacuous companion: without it, a predicate that matched every column would satisfy both cases + above. Same renamed board, same checkout — only the holder's lane changes. + */ + const shipped = { ...ownerTask(RENAMED_VOCAB), column: RENAMED_VOCAB.complete } as Task; + const store = storeFor([shipped], RENAMED_VOCAB); + const executor = new TaskExecutor(store, "/tmp/test", {}); + + await expect(findOwner(executor, "FN-REQUESTER")).resolves.toBeNull(); + }); + + it("never reports the requesting task as its own blocker", async () => { + const store = storeFor([ownerTask(RENAMED_VOCAB)], RENAMED_VOCAB); + const executor = new TaskExecutor(store, "/tmp/test", {}); + + await expect(findOwner(executor, "FN-OWNER")).resolves.toBeNull(); + }); +}); + +/* +FNXC:WorkflowResolvedColumns 2026-07-30-17:20 (executor — the merge-state reset): +The second conversion in the same commit, and it needs its own differential: `resetMergeStateIfNeeded` +clears merge state when a card LEAVES a lane where a merge could have been recorded (the review and +complete roles). Keyed on `in-review`/`done`, a renamed board matched neither, so a card re-entering +execution carried STALE mergeDetails — a commit sha from a previous pass — into its next run. + +Reached through the same private seam and for the same reason as above: the public route is the +`task:moved` listener, which would drag in the whole execute() path. + +REVERT CHECK, measured: with `from !== "in-review" && from !== "done"` restored, the RENAMED case fails +— the task comes back with its stale `mergeDetails` intact. The DEFAULT case passes both ways. +*/ +function resetMergeState(executor: TaskExecutor, task: Task, from: string): Promise { + return (executor as unknown as { + resetMergeStateIfNeeded: (task: Task, from: string) => Promise; + }).resetMergeStateIfNeeded(task, from); +} + +describe("resetMergeStateIfNeeded resolves the merge-bearing lanes by ROLE, not by id", () => { + for (const [label, vocab] of [["DEFAULT", DEFAULT_VOCAB], ["RENAMED", RENAMED_VOCAB]] as const) { + it(`clears stale merge state for a card returning from a ${label} review lane (${vocab.review})`, async () => { + const task = mergedOnceTask(vocab); + const store = storeFor([task], vocab); + const executor = new TaskExecutor(store, "/tmp/test", {}); + + const result = await resetMergeState(executor, task, vocab.review); + + expect(result).not.toBe(task); + expect(result.mergeDetails?.commitSha).toBeUndefined(); + }); + } + + it("leaves merge state alone for a card returning from a lane that bears no merge on a RENAMED board", async () => { + /* + Non-vacuous companion: without it, a gate matching every column would satisfy both cases above. The + renamed HOLD lane is a real column — just not one where a merge could have been recorded. + */ + const task = mergedOnceTask(RENAMED_VOCAB); + const store = storeFor([task], RENAMED_VOCAB); + const executor = new TaskExecutor(store, "/tmp/test", {}); + + const result = await resetMergeState(executor, task, RENAMED_VOCAB.hold); + + expect(result).toBe(task); + expect(result.mergeDetails?.commitSha).toBe("abc123"); + }); +}); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 63b4e3c90f..ec640d97b0 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -4127,7 +4127,26 @@ export class TaskExecutor { } private async resetMergeStateIfNeeded(task: Task, from: Task["column"]): Promise { - if (from !== "in-review" && from !== "done") { + /* + FNXC:WorkflowResolvedColumns 2026-07-30-16:40 (executor): + Merge state is reset when a card leaves a lane where a merge could have been recorded — the REVIEW + and COMPLETE roles, not the two ids. On a renamed board neither comparison matched, so a card + re-entering execution carried STALE mergeDetails from its previous pass. + + `review` is not a trait: the role is carried by mergeOrchestration/mergeBlocker/humanReview, the same + five-flag set the dependency gates in this file use. Unioned with the legacy pair because + `resolveWorkflowIrForTask` degrades to the BUILT-IN IR rather than throwing. + */ + const mergeBearingColumns = new Set(["in-review", "done"]); + try { + const ir = await resolveWorkflowIrForTask(this.store, task.id); + if (ir) { + for (const flag of ["complete", "mergeOrchestration", "mergeBlocker", "humanReview"] as const) { + for (const id of columnsWithFlag(ir, flag)) mergeBearingColumns.add(id); + } + } + } catch { /* degraded: legacy pair only */ } + if (!mergeBearingColumns.has(from)) { return task; } @@ -16088,6 +16107,13 @@ export class TaskExecutor { return record; } changed = true; + /* + FNXC:ReviewAddressing 2026-07-30-16:40 DELIBERATE-LITERAL: + `to` here is a review-addressing RECORD STATUS (`"queued" | "in-progress" | "addressed" | "failed"`, + see this method's signature), NOT a board column — the very next lines test it against `"addressed"` + and `"failed"`, which are not columns at all. The lifecycle-column census matches the bare string + and counted it; resolving it to a workflow role would be nonsense. + */ return { ...record, status: to, @@ -20338,9 +20364,26 @@ You have access to the file system to review changes.${inlineFixBlock}${verdictB } try { const tasks = await this.store.listTasks({ slim: true, includeArchived: false }); + /* + FNXC:WorkflowResolvedColumns 2026-07-30-16:40 (executor): + "Who else is actively working in this worktree?" is the WIP role, not the id. NOT the query-filter + class — this listTasks call passes no `column`. On a renamed board the check matched nobody, so the + worktree read as unowned and a second task could be handed a checkout already in use. + + Resolved per CANDIDATE task, one IR cache for the scan, and only for rows that could still match. + */ + const ownerIrCache = new Map>>(); for (const t of tasks) { if (t.id === requestingTaskId) continue; - if (t.column !== "in-progress") continue; + const wipColumns = new Set(["in-progress"]); + try { + const ir = await resolveWorkflowIrForTask(this.store, t.id, ownerIrCache); + if (ir) { + const resolved = columnsWithFlag(ir, "countsTowardWip"); + if (resolved.length > 0) { wipColumns.clear(); for (const id of resolved) wipColumns.add(id); } + } + } catch { /* degraded: legacy id only */ } + if (!wipColumns.has(t.column)) continue; if (t.paused === true) continue; if (t.worktree === worktreePath) return t.id; // FNXC:Workspace 2026-06-22-09:00: workspace tasks hold their worktrees in diff --git a/scripts/lib/lifecycle-column-census-baseline.json b/scripts/lib/lifecycle-column-census-baseline.json index c8f9652fa5..f0e831c5a3 100644 --- a/scripts/lib/lifecycle-column-census-baseline.json +++ b/scripts/lib/lifecycle-column-census-baseline.json @@ -2,10 +2,10 @@ "generatedFrom": "node scripts/lifecycle-column-census.mjs --strict --update-baseline", "byFile": { "packages/engine/src/self-healing.ts": 97, - "packages/engine/src/executor.ts": 12, "packages/engine/src/scheduler.ts": 12, "packages/core/src/task-store/async-comments-attachments.ts": 9, "packages/dashboard/app/components/TaskContextMenu.tsx": 9, + "packages/engine/src/executor.ts": 8, "packages/dashboard/app/components/Column.tsx": 7, "packages/dashboard/app/components/ListView.tsx": 6, "packages/engine/src/notification/notification-service.ts": 5, @@ -141,6 +141,7 @@ "packages/engine/src/agent-heartbeat.ts\u0000archived": 1, "packages/engine/src/agent-heartbeat.ts\u0000done": 1, "packages/engine/src/cli-agent/task-session.ts\u0000done": 1, + "packages/engine/src/executor.ts\u0000in-progress": 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,