From dfbab18fd5e2a9530c3af335ccd002d30dbcf8a7 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 03:19:02 -0700 Subject: [PATCH] =?UTF-8?q?fix(scheduler):=20a=20renamed=20wip=20column=20?= =?UTF-8?q?held=20NO=20file-scope=20lease=20=E2=80=94=20two=20agents=20cou?= =?UTF-8?q?ld=20edit=20the=20same=20files=20(#2693)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## The defect `activeScopes` is the file-scope lease registry the dispatch path reads (`scheduler.ts:2167`) to decide whether a candidate overlaps work already in flight. Two column-id literals kept it empty on any board whose columns are renamed: 1. the lease loop gated on `task.column !== "in-progress"`; 2. `shouldHoldActiveFileScopeLease` keyed **both** its branches on `in-progress` / `in-review`, so it returned `false` for *every* card on a renamed board. Forty lines above that loop, the same sweep resolves `countsTowardWip` from the workflow IR for capacity arithmetic. **The scheduler was simultaneously right about capacity and wrong about leases.** Consequence: a second task sharing a file scope **dispatched instead of queueing** — two agents editing the same files, which is precisely what `groupOverlappingFiles` exists to prevent. ## Measured, differential Same workflow *shape* under two vocabularies with identical traits; only the column ids differ, so any difference is attributable to a surviving literal. No renamed id collides with a legacy one, so a surviving `=== "in-progress"` cannot pass by luck. | | default vocabulary (control) | renamed vocabulary | |---|---|---| | fix reverted | queued on lease ✓ | **dispatched into the wip column** ✗ | | fix applied | queued on lease ✓ | queued on lease ✓ | `2 of 3 fail` reverted → `3 of 3 pass` applied. The control passes on **both** sides, so a change that breaks overlap protection generally cannot hide behind this test. I checked the test wasn't vacuous before trusting it: instrumented the run to print the actual `moveTask` calls, and confirmed the renamed case really produced `[["FN-CAND","building"]]` — a genuine dispatch — rather than the candidate simply never being considered. Both failure modes look identical in the assertion. ## Why optional booleans, not a flags object `shouldHoldActiveFileScopeLease` is **exported** and shared with the self-healing / repair paths (`self-healing.ts:4488`, `:5406`) — its own comment says those "must use this same predicate so stale `overlapBlockedBy` cleanup does not preserve blockers the scheduler would ignore". So the role questions became optional parameters that **default to today's literals**: a caller that resolved the traits passes the answer, a caller that has not gets exactly current behaviour. No existing call site changes meaning, and no dependency on #2690. ## Verification | Check | Result | |---|---| | scheduler / capacity / hold-release / overlap / self-healing | **59 test files green** | | `pnpm test:gate` | **726 passed** | | `pnpm lint`, engine `tsc --noEmit` | clean | `self-healing-advanced-triage`, `-agent-link-drift`, `-starved-refinement` are **7 failed / 19 passed both before and after** — verified pre-existing on clean `origin/main` by reverting only `scheduler.ts` and re-running. Flagged, not fixed, and not in scope here. ## Census **722 → 721**, `scheduler.ts` 28 → 27. Baseline re-recorded in the same commit. To be precise about what that −1 is: the *loop* literal is gone, while the two literals **inside** the predicate remain by design as the documented defaults. So this is not "scheduler is now trait-aware" — it is one site, plus the seam that lets callers be. ## Merge-order note **#2690 also records `scheduler.ts` 28 → 27**, converting a *different* site (`isWipColumnTask`'s hand-rolled flags-first copy, `:1690`). The two are independent and do not double-count: if both land, `scheduler.ts` is **26**, and whichever merges second will conflict on `scripts/lib/lifecycle-column-census-baseline.json` and must re-record to 26 rather than resolve to 27. Flagging so the merger does not take one side blindly. ## Still broken, flagged for an owner The **in-review** half. `activeScopes` is also populated for review-lane cards via `t.column === "in-review"` (`scheduler.ts:1751`, `:1757`), and this PR leaves those literals in place: the sweep's flags map holds only `countsTowardWip`, so no review-role answer is available to pass in. Fixing it needs the flags-object change in #2690, after which the same optional parameter added here carries it. Until then a renamed review column still holds no lease. --- ...duler-renamed-wip-file-scope-lease.test.ts | 196 ++++++++++++++++++ packages/engine/src/scheduler.ts | 42 +++- 2 files changed, 234 insertions(+), 4 deletions(-) create mode 100644 packages/engine/src/__tests__/scheduler-renamed-wip-file-scope-lease.test.ts diff --git a/packages/engine/src/__tests__/scheduler-renamed-wip-file-scope-lease.test.ts b/packages/engine/src/__tests__/scheduler-renamed-wip-file-scope-lease.test.ts new file mode 100644 index 0000000000..0534414eb0 --- /dev/null +++ b/packages/engine/src/__tests__/scheduler-renamed-wip-file-scope-lease.test.ts @@ -0,0 +1,196 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { Scheduler } from "../scheduler.js"; +import type { Settings, Task, TaskStore, WorkflowIr } from "@fusion/core"; + +/* +FNXC:WorkflowResolvedColumns 2026-07-30-16:30: +A card in a RENAMED wip column must still hold a file-scope lease. + +THE DEFECT. `Scheduler.runHoldReleaseSweepPass` builds `activeScopes` — the registry the dispatch +path reads to decide whether a candidate overlaps work already in flight — behind +`if (task.column !== "in-progress") continue;`. Forty lines above it, the capacity arithmetic for the +same cards resolves the `countsTowardWip` trait from the workflow IR. So on a board whose wip column +is not literally `in-progress`, capacity counts the occupant correctly while the lease loop skips it, +`activeScopes` stays empty, `overlappingTaskId` resolves to null, and a second task sharing the same +file scope is DISPATCHED instead of queued — two agents editing the same files, which is precisely +what `groupOverlappingFiles` exists to prevent. + +WHY IT IS DIFFERENTIAL. The scenario runs twice against the same workflow SHAPE under two +vocabularies whose traits are identical; only the column ids differ. Any behavioural difference +between the two runs is therefore attributable to a surviving column-id literal and nothing else. +The default-vocabulary case is the control: it passes before and after, so a change that breaks +overlap protection generally cannot hide behind this test. + +None of the renamed ids collides with a legacy literal, so a surviving `=== "in-progress"` cannot +pass by luck. +*/ + +const WF = "custom:renamed-wip"; + +const PASSED_PLAN_REVIEW = { + workflowStepId: "plan-review", + workflowStepName: "Plan Review", + status: "passed" as const, + source: "node" as const, + phase: "pre-merge" as const, +}; + +function makeTask(overrides: Partial = {}): Task { + return { + id: "FN-001", + title: "task", + description: "", + column: "todo", + dependencies: [], + steps: [], + currentStep: 0, + log: [], + workflowStepResults: [PASSED_PLAN_REVIEW], + createdAt: "2026-01-01T00:00:00.000Z", + updatedAt: "2026-01-01T00:00:00.000Z", + ...overrides, + } as Task; +} + +/** One workflow shape; the traits are identical under both vocabularies. */ +function ir(names: { hold: string; wip: string; complete: string }): WorkflowIr { + return { + version: "v2", + id: WF, + nodes: [], + edges: [], + columns: [ + { 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" }] }, + ], + } as unknown as WorkflowIr; +} + +const DEFAULT_NAMES = { hold: "todo", wip: "in-progress", complete: "done" }; +const RENAMED_NAMES = { hold: "drafting", wip: "building", complete: "shipped" }; + +function createStore( + tasks: Task[], + scopes: Record, + workflowIr: WorkflowIr, + settings: Partial = {}, +): TaskStore { + const resolved = { maxConcurrent: 10, maxWorktrees: 10, groupOverlappingFiles: true, ...settings }; + const selection = { workflowId: WF, stepIds: [] }; + const updateTask = vi.fn(async (id: string, patch: Partial) => { + const task = tasks.find((candidate) => candidate.id === id); + if (task) Object.assign(task, patch); + return task as Task; + }); + const moveTask = vi.fn(async (id: string, column: Task["column"], _opts?: Record) => { + const task = tasks.find((candidate) => candidate.id === id); + if (task) task.column = column; + return task as Task; + }); + const moveTaskIf = vi.fn(async ( + id: string, + column: Task["column"], + predicate: (live: Task) => boolean | Promise, + opts?: Record, + ) => { + const task = tasks.find((candidate) => candidate.id === id); + if (!task) return { task: task as unknown as Task, moved: false }; + if (!(await predicate(task)) || task.column === column) return { task, moved: false }; + const movedTask = await moveTask(id, column, opts as never); + return { task: movedTask ?? task, moved: true }; + }); + + return { + listTasks: vi.fn(async () => tasks), + getSettings: vi.fn(async () => resolved), + updateSettings: vi.fn(async () => resolved), + parseFileScopeFromPrompt: vi.fn(async (id: string) => scopes[id] ?? []), + updateTask, + moveTask, + moveTaskIf, + getTask: vi.fn(async (id: string) => tasks.find((task) => task.id === id) ?? null), + logEntry: vi.fn(async () => undefined), + getRootDir: vi.fn(() => "/tmp/project"), + getTasksDir: vi.fn(() => "/tmp/project/.fusion/tasks"), + on: vi.fn(), + off: vi.fn(), + recordRunAuditEvent: vi.fn(async () => undefined), + getCompletionHandoffAcceptedMarker: vi.fn(async () => null), + // The readers that let the scheduler resolve trait flags for these columns. + getTaskWorkflowSelection: vi.fn(() => selection), + getTaskWorkflowSelectionAsync: vi.fn(async () => selection), + getWorkflowDefinition: vi.fn(async () => ({ ir: workflowIr })), + } as unknown as TaskStore; +} + +/** + * One occupant already in the wip column holding a file scope, and one ready candidate in the hold + * column whose scope overlaps it. Reported in ROLE terms so both runs are directly comparable. + */ +async function overlapScenario(names: { hold: string; wip: string; complete: string }) { + const tasks = [ + makeTask({ id: "FN-OCC", column: names.wip, priority: "normal" }), + makeTask({ id: "FN-CAND", column: names.hold, priority: "urgent" }), + ]; + const store = createStore( + tasks, + { + "FN-OCC": ["packages/engine/src/scheduler.ts"], + "FN-CAND": ["packages/engine/src/scheduler.ts"], + }, + ir(names), + ); + + const scheduler = new Scheduler(store); + (scheduler as unknown as { running: boolean }).running = true; + await scheduler.schedule(); + + const queuedOnLease = (store.updateTask as ReturnType).mock.calls.some( + (call: unknown[]) => + call[0] === "FN-CAND" + && (call[1] as Partial | undefined)?.overlapBlockedBy === "FN-OCC", + ); + const dispatched = (store.moveTask as ReturnType).mock.calls.some( + (call: unknown[]) => call[0] === "FN-CAND" && call[1] === names.wip, + ); + + return { queuedOnLease, dispatched }; +} + +describe("scheduler file-scope lease is held for a RENAMED wip column", () => { + beforeEach(() => { + vi.restoreAllMocks(); + vi.spyOn(Scheduler.prototype as never as { validateTaskFilesystem: () => unknown }, "validateTaskFilesystem") + .mockResolvedValue({ valid: true } as never); + }); + + /* Control. Passes before and after the fix; if overlap protection breaks generally, this fails. */ + it("default vocabulary: the overlapping candidate is queued on the lease, not dispatched", async () => { + const outcome = await overlapScenario(DEFAULT_NAMES); + + expect(outcome.queuedOnLease).toBe(true); + expect(outcome.dispatched).toBe(false); + }); + + /* + The defect. Before the fix the lease loop skipped the occupant (its column is not literally + `in-progress`), so `activeScopes` was empty and this dispatched with no overlap block. + */ + it("renamed vocabulary: the overlapping candidate is queued on the lease, not dispatched", async () => { + const outcome = await overlapScenario(RENAMED_NAMES); + + expect(outcome.queuedOnLease).toBe(true); + expect(outcome.dispatched).toBe(false); + }); + + /* States the invariant directly: the two vocabularies must be indistinguishable. */ + it("both vocabularies reach the SAME outcome — no column-id literal survives on this path", async () => { + const [byDefault, renamed] = await Promise.all([ + overlapScenario(DEFAULT_NAMES), + overlapScenario(RENAMED_NAMES), + ]); + + expect(renamed).toEqual(byDefault); + }); +}); diff --git a/packages/engine/src/scheduler.ts b/packages/engine/src/scheduler.ts index de5c87c4e4..6c6bd90434 100644 --- a/packages/engine/src/scheduler.ts +++ b/packages/engine/src/scheduler.ts @@ -329,17 +329,40 @@ export function shouldHoldActiveFileScopeLease( mergeRequestContractShadowEnabled?: boolean; handoffAccepted?: boolean; schedulingDependencyOptions?: Parameters[2]; + /** Resolved role answers. Omitted → the legacy column-id literals, i.e. today's behaviour. */ + isWipColumn?: boolean; + isReviewColumn?: boolean; }, ): boolean { /* FNXC:OverlapScheduling 2026-06-25-04:34: Active file-scope leases are a scheduler contract, not just a column check. Self-healing and repair paths must use this same predicate so stale `overlapBlockedBy` cleanup does not preserve blockers the scheduler would ignore on the next tick. */ + /* + FNXC:WorkflowResolvedColumns 2026-07-30-17:00: + The two ROLE questions are parameters with literal defaults, not hard-coded ids. + + This predicate decides whether a card holds an active file-scope lease, and it was keyed on + `column === "in-progress"` / `!== "in-review"`. On a board whose columns are renamed, BOTH branches + fell through and the function returned false for every card, so `activeScopes` stayed empty and the + dispatch path saw no overlap — two agents editing the same files, which is exactly what + `groupOverlappingFiles` prevents. Capacity arithmetic in the same sweep already resolved + `countsTowardWip` from the IR, so the scheduler was simultaneously right about capacity and wrong + about leases. + + Why optional booleans rather than a flags object: this is an EXPORTED predicate shared with the + self-healing / repair paths (see the note below), which must keep agreeing with the scheduler. A + caller that has resolved the column's traits passes the answer; a caller that has not gets exactly + today's behaviour, so no existing call site changes meaning. Covered by + scheduler-renamed-wip-file-scope-lease.test.ts. + */ if (task.paused || task.userPaused) return false; - if (task.column === "in-progress") { + const isWipColumn = options?.isWipColumn ?? task.column === "in-progress"; + const isReviewColumn = options?.isReviewColumn ?? task.column === "in-review"; + if (isWipColumn) { return getUnmetSchedulingDependencies(task, tasks, options?.schedulingDependencyOptions).length === 0; } - if (task.column !== "in-review") return false; + if (!isReviewColumn) return false; if (!task.worktree || task.status === "failed") return false; if (options?.mergeRequestContractShadowEnabled === true && options.handoffAccepted === true) return false; return true; @@ -1743,8 +1766,19 @@ export class Scheduler { if (settings.groupOverlappingFiles) { for (const task of tasks) { - if (task.column !== "in-progress") continue; - if (!shouldHoldActiveFileScopeLease(task, tasks, { schedulingDependencyOptions })) continue; + /* + FNXC:WorkflowResolvedColumns 2026-07-30-16:30: + Trait-aware, not `column !== "in-progress"`. `activeScopes` is the file-scope lease registry + the dispatch path reads to decide whether a candidate overlaps work already in flight. The + literal skipped every card in a RENAMED wip column, so the registry stayed empty while the + capacity arithmetic above — which resolves `countsTowardWip` from the IR — counted the same + cards correctly; a second task sharing the file scope then dispatched instead of queueing, + putting two agents on the same files. `isWipColumnTask` is the same predicate capacity uses, + so the two cannot disagree again. Covered by + scheduler-renamed-wip-file-scope-lease.test.ts. + */ + if (!isWipColumnTask(task)) continue; + if (!shouldHoldActiveFileScopeLease(task, tasks, { schedulingDependencyOptions, isWipColumn: true })) continue; const filteredScope = await getFilteredFileScope(task.id); if (isCoordinationOnlyTask(task, filteredScope)) continue; if (filteredScope.length === 0) continue;