root/fusion
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(scheduler): a renamed wip column held NO file-scope lease — two agents could edit the same files (#2693)

Browse Source 
## 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.
This commit is contained in:
gsxdsm
2026-07-30 03:19:02 -07:00
committed by GitHub
parent e9e63d8e0f
commit dfbab18fd5
2 changed files with 234 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

196
packages/engine/src/__tests__/scheduler-renamed-wip-file-scope-lease.test.ts Normal file
View File

@@ -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> = {}): 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<string, string[]>,
workflowIr: WorkflowIr,
settings: Partial<Settings> = {},
): TaskStore {
const resolved = { maxConcurrent: 10, maxWorktrees: 10, groupOverlappingFiles: true, ...settings };
const selection = { workflowId: WF, stepIds: [] };
const updateTask = vi.fn(async (id: string, patch: Partial<Task>) => {
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<string, unknown>) => {
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<boolean>,
opts?: Record<string, unknown>,
) => {
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<typeof vi.fn>).mock.calls.some(
(call: unknown[]) =>
call[0] === "FN-CAND"
&& (call[1] as Partial<Task> | undefined)?.overlapBlockedBy === "FN-OCC",
);
const dispatched = (store.moveTask as ReturnType<typeof vi.fn>).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);
});
});

42
packages/engine/src/scheduler.ts
View File

@@ -329,17 +329,40 @@ export function shouldHoldActiveFileScopeLease(
mergeRequestContractShadowEnabled?: boolean;
handoffAccepted?: boolean;
schedulingDependencyOptions?: Parameters<typeof getUnmetSchedulingDependencies>[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;