Skip to content

Commit b745cce

Browse files
committed
fix(agent,data): task-state-based serialization + planner restore for fast-mode bound tasks
Round 5 of branch-isolation hardening per PR #85 review. - coordinator: serialization predicate now combines current config AND task state. Previously, toggling git.create_branches=false mid-pipeline opened the parallel pool even though restorePersistedBranch kept switching HEAD on already-bound tasks. Fix: parallel disabled when EITHER projectUsesSharedBranchIsolation(rootPath) OR hasActiveBranchBoundTasksForProject(projectId) is true. Applied in both processAutoQueueAdvance and resolveProjectConcurrency. Until #86 introduces per-task worktrees, any in-flight bound task forces serial. - data: new hasActiveBranchBoundTasksForProject query. Includes backlog so a task pre-bound by accept_existing_plan / replan does not let the pool open before its first stage starts. Includes blocked_external so retry cycles do not leak parallel concurrency. - planner: restore is no longer gated by plannerMode. For ANY bound non-fix task, restorePersistedBranch fires regardless of full/fast mode. Branch CREATION stays full-mode-only (fast mode never provisions a new branch by design — see aif-handoff#83). Closes the mode-drift gap where a fast-mode replan of an already-bound task ran on whatever HEAD happened to be. Tests: - data: 4 cases for hasActiveBranchBoundTasksForProject (no branchName, in-flight bound, queued backlog bound, terminal bound). - planner: regression test — fast plannerMode + persisted branchName + drifted HEAD → planner ends on the persisted branch, not on main. npm run ai:validate green end-to-end.
1 parent c821f6f commit b745cce

5 files changed

Lines changed: 173 additions & 41 deletions

File tree

packages/agent/src/__tests__/planner.test.ts

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -268,6 +268,63 @@ describe("runPlanner comment selection", () => {
268268
expect(updatedTask?.branchName).toBe(branch);
269269
});
270270

271+
it("restores persisted branch in fast plannerMode for already-bound task (mode-drift safe)", async () => {
272+
const db = testDb.current;
273+
const projectRoot = mkdtempSync(join(tmpdir(), "planner-mode-drift-"));
274+
execFileSync("git", ["init", "--initial-branch=main"], { cwd: projectRoot, stdio: "ignore" });
275+
execFileSync("git", ["config", "user.email", "t@t.local"], {
276+
cwd: projectRoot,
277+
stdio: "ignore",
278+
});
279+
execFileSync("git", ["config", "user.name", "T"], { cwd: projectRoot, stdio: "ignore" });
280+
execFileSync("git", ["config", "commit.gpgsign", "false"], {
281+
cwd: projectRoot,
282+
stdio: "ignore",
283+
});
284+
writeFileSync(join(projectRoot, "README.md"), "# t\n");
285+
execFileSync("git", ["add", "README.md"], { cwd: projectRoot, stdio: "ignore" });
286+
execFileSync("git", ["commit", "-m", "init", "--no-verify"], {
287+
cwd: projectRoot,
288+
stdio: "ignore",
289+
});
290+
// Bind a feature branch then drift HEAD back to main.
291+
execFileSync("git", ["checkout", "-b", "feature/bound-fast"], {
292+
cwd: projectRoot,
293+
stdio: "ignore",
294+
});
295+
execFileSync("git", ["checkout", "main"], { cwd: projectRoot, stdio: "ignore" });
296+
297+
db.insert(projects)
298+
.values({ id: "project-mode-drift", name: "Mode drift", rootPath: projectRoot })
299+
.run();
300+
db.insert(tasks)
301+
.values({
302+
id: "task-mode-drift-1",
303+
projectId: "project-mode-drift",
304+
title: "Mode drift",
305+
description: "",
306+
status: "planning",
307+
// FAST mode + persisted branchName = the dangerous case the previous
308+
// gating broke: restore must still happen, otherwise the planner
309+
// writes plan/log on whatever HEAD happens to be.
310+
plannerMode: "fast",
311+
useSubagents: false,
312+
branchName: "feature/bound-fast",
313+
})
314+
.run();
315+
316+
queryMock.mockReset();
317+
queryMock.mockReturnValue(streamSuccess("## Plan\n- [ ] x"));
318+
319+
await runPlanner("task-mode-drift-1", projectRoot);
320+
321+
const branch = execFileSync("git", ["rev-parse", "--abbrev-ref", "HEAD"], {
322+
cwd: projectRoot,
323+
encoding: "utf8",
324+
}).trim();
325+
expect(branch).toBe("feature/bound-fast");
326+
});
327+
271328
it("throws BranchIsolationError when subagent silently switched branches (drift)", async () => {
272329
const db = testDb.current;
273330
const projectRoot = mkdtempSync(join(tmpdir(), "planner-drift-"));

packages/agent/src/coordinator.ts

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {
1414
listAutoQueueProjects,
1515
nextBacklogTaskByPosition,
1616
countActivePipelineTasksForProject,
17+
hasActiveBranchBoundTasksForProject,
1718
claimBacklogTaskForAdvance,
1819
persistTaskRuntimeLimitSnapshot,
1920
resolveEffectiveRuntimeProfile,
@@ -629,11 +630,22 @@ export function processAutoQueueAdvance(): number {
629630

630631
let advanced = 0;
631632
for (const project of projects) {
632-
const usesSharedBranchIsolation = projectUsesSharedBranchIsolation(project.rootPath);
633+
// Serialization predicate combines:
634+
// - current config (`git.create_branches=true` on a real git repo), AND
635+
// - task state (any in-flight task already has a persisted branchName).
636+
//
637+
// Config alone is not enough: an operator can toggle `create_branches=off`
638+
// mid-pipeline. `restorePersistedBranch` keeps switching HEAD for already
639+
// bound tasks, so concurrent parallel stages would still race on the
640+
// shared work tree. Until #86 introduces per-task worktrees, fall back to
641+
// serial whenever EITHER signal says the project is shared-branch.
642+
const usesSharedBranchIsolation =
643+
projectUsesSharedBranchIsolation(project.rootPath) ||
644+
hasActiveBranchBoundTasksForProject(project.id);
633645
if (project.parallelEnabled && usesSharedBranchIsolation) {
634646
log.warn(
635647
{ projectId: project.id, projectRoot: project.rootPath },
636-
"Auto-queue parallel pool disabled for git.create_branches project until per-task worktrees are available",
648+
"Auto-queue parallel pool disabled for git.create_branches project (or active branch-bound tasks) until per-task worktrees are available",
637649
);
638650
}
639651
const limit =
@@ -764,8 +776,10 @@ export async function pollAndProcess(): Promise<void> {
764776
if (cached === undefined) {
765777
const project = findProjectById(projectId);
766778
const configuredParallel = project?.parallelEnabled ?? false;
779+
// Mirror processAutoQueueAdvance: config OR task-state forces serial.
767780
const usesSharedBranchIsolation = project
768-
? projectUsesSharedBranchIsolation(project.rootPath)
781+
? projectUsesSharedBranchIsolation(project.rootPath) ||
782+
hasActiveBranchBoundTasksForProject(projectId)
769783
: false;
770784
cached = {
771785
parallel: configuredParallel && !usesSharedBranchIsolation,
@@ -774,7 +788,7 @@ export async function pollAndProcess(): Promise<void> {
774788
if (configuredParallel && usesSharedBranchIsolation) {
775789
log.warn(
776790
{ projectId, projectRoot: project?.rootPath },
777-
"Project parallel execution forced to serial because git.create_branches uses a shared worktree",
791+
"Project parallel execution forced to serial because git.create_branches uses a shared worktree (or active branch-bound tasks remain)",
778792
);
779793
}
780794
projectConcurrencyCache.set(projectId, cached);

packages/agent/src/subagents/planner.ts

Lines changed: 36 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -145,52 +145,51 @@ export async function runPlanner(taskId: string, projectRoot: string): Promise<v
145145
const planDocs = task.planDocs ? "true" : "false";
146146
const planTests = task.planTests ? "true" : "false";
147147

148-
// Deterministic branch handling — runs regardless of whether the subagent
149-
// skill honors Step 1.4. Only activates for full-mode plans; fast mode stays
150-
// on current branch by design. See aif-handoff#83.
148+
// Deterministic branch handling. Two contracts, applied in order:
151149
//
152-
// Two paths:
153-
// - First-time provisioning (no persisted branchName): use
154-
// ensureFeatureBranch which can create from base.
155-
// - Re-run / replan (task.branchName already persisted): use
156-
// restorePersistedBranch so config drift cannot release us to current
157-
// HEAD via the `skipped` shortcut, and a deleted branch errors out
158-
// instead of being silently re-created from base.
150+
// 1. RESTORE for ANY bound non-fix task — runs regardless of plannerMode
151+
// (full or fast). `task.branchName` is the source-of-truth: once a
152+
// prior run persisted it, every subsequent stage MUST land on it or
153+
// fail loud. A replan triggered with mode=fast (manual replanning,
154+
// comment-driven re-run) used to skip the restore entirely and let
155+
// the planner write to whatever HEAD happened to be.
156+
//
157+
// 2. CREATE only in full mode for unbound non-fix tasks. Fast mode stays
158+
// on the current branch by design (see aif-handoff#83) — first-time
159+
// branch provisioning is a full-mode-only concern.
159160
//
160161
// Failures throw BranchIsolationError (dirty worktree, missing base branch,
161162
// checkout failure, branch_missing, etc). The coordinator classifies it as
162163
// blocked_external with retryAfter=null so an operator can inspect the work
163164
// tree instead of the stage silently reverting into a bad state.
164165
let preparedBranch: string | null = task.branchName ?? null;
165-
if (plannerMode === "full" && !task.isFix) {
166-
if (task.branchName) {
167-
restorePersistedBranch({
168-
projectRoot,
169-
taskId,
170-
persistedBranchName: task.branchName,
166+
if (!task.isFix && task.branchName) {
167+
restorePersistedBranch({
168+
projectRoot,
169+
taskId,
170+
persistedBranchName: task.branchName,
171+
});
172+
preparedBranch = task.branchName;
173+
logActivity(taskId, "Agent", `Restored feature branch: ${task.branchName}`);
174+
} else if (!task.isFix && plannerMode === "full") {
175+
const branchResult = ensureFeatureBranch({
176+
projectRoot,
177+
taskId,
178+
title: task.title,
179+
});
180+
if (branchResult.action !== "skipped" && branchResult.branchName) {
181+
preparedBranch = branchResult.branchName;
182+
setTaskFields(taskId, {
183+
branchName: branchResult.branchName,
184+
updatedAt: new Date().toISOString(),
171185
});
172-
preparedBranch = task.branchName;
173-
logActivity(taskId, "Agent", `Restored feature branch: ${task.branchName}`);
174-
} else {
175-
const branchResult = ensureFeatureBranch({
176-
projectRoot,
186+
logActivity(
177187
taskId,
178-
title: task.title,
179-
});
180-
if (branchResult.action !== "skipped" && branchResult.branchName) {
181-
preparedBranch = branchResult.branchName;
182-
setTaskFields(taskId, {
183-
branchName: branchResult.branchName,
184-
updatedAt: new Date().toISOString(),
185-
});
186-
logActivity(
187-
taskId,
188-
"Agent",
189-
`Feature branch ${branchResult.action}: ${branchResult.branchName}`,
190-
);
191-
} else if (branchResult.reason) {
192-
log.debug({ taskId, reason: branchResult.reason }, "Branch creation skipped");
193-
}
188+
"Agent",
189+
`Feature branch ${branchResult.action}: ${branchResult.branchName}`,
190+
);
191+
} else if (branchResult.reason) {
192+
log.debug({ taskId, reason: branchResult.reason }, "Branch creation skipped");
194193
}
195194
}
196195

packages/data/src/__tests__/index.test.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ const {
5757
nextBacklogTaskByPosition,
5858
listAutoQueueProjects,
5959
countActivePipelineTasksForProject,
60+
hasActiveBranchBoundTasksForProject,
6061
claimBacklogTaskForAdvance,
6162
} = await import("../index.js");
6263

@@ -1132,6 +1133,34 @@ describe("data layer", () => {
11321133
expect(countActivePipelineTasksForProject("proj-1")).toBe(1);
11331134
});
11341135

1136+
it("hasActiveBranchBoundTasksForProject returns false when no task has a branchName", () => {
1137+
const a = createTask({ projectId: "proj-1", title: "A", description: "" });
1138+
updateTaskStatus(a!.id, "implementing");
1139+
expect(hasActiveBranchBoundTasksForProject("proj-1")).toBe(false);
1140+
});
1141+
1142+
it("hasActiveBranchBoundTasksForProject true once a branch-bound task is in flight", () => {
1143+
const a = createTask({ projectId: "proj-1", title: "A", description: "" });
1144+
setTaskFields(a!.id, { branchName: "feature/a" });
1145+
updateTaskStatus(a!.id, "implementing");
1146+
expect(hasActiveBranchBoundTasksForProject("proj-1")).toBe(true);
1147+
});
1148+
1149+
it("hasActiveBranchBoundTasksForProject true for a queued backlog task that already has branchName", () => {
1150+
// accept_existing_plan / replan can leave a branch-bound task in
1151+
// backlog briefly; serialization must already kick in.
1152+
const a = createTask({ projectId: "proj-1", title: "A", description: "" });
1153+
setTaskFields(a!.id, { branchName: "feature/a-prepared" });
1154+
expect(hasActiveBranchBoundTasksForProject("proj-1")).toBe(true);
1155+
});
1156+
1157+
it("hasActiveBranchBoundTasksForProject false when bound tasks are terminal (done)", () => {
1158+
const a = createTask({ projectId: "proj-1", title: "A", description: "" });
1159+
setTaskFields(a!.id, { branchName: "feature/a" });
1160+
updateTaskStatus(a!.id, "done");
1161+
expect(hasActiveBranchBoundTasksForProject("proj-1")).toBe(false);
1162+
});
1163+
11351164
it("nextBacklogTaskByPosition skips paused tasks", () => {
11361165
const a = createTask({ projectId: "proj-1", title: "A", description: "" });
11371166
setTaskFields(a!.id, { paused: true });

packages/data/src/index.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1319,6 +1319,39 @@ export function countActivePipelineTasksForProject(projectId: string): number {
13191319
return row?.cnt ?? 0;
13201320
}
13211321

1322+
/**
1323+
* True if the project has at least one in-flight task with a persisted
1324+
* `branchName`. Used by the auto-queue scheduler to keep parallel execution
1325+
* disabled even after `git.create_branches` is toggled off mid-pipeline —
1326+
* already-bound tasks still mutate the shared worktree on stage transitions
1327+
* via `restorePersistedBranch`, so concurrent stages would race on HEAD.
1328+
*
1329+
* Includes `backlog` so a queued task whose branch was prepared (e.g. via
1330+
* `accept_existing_plan`) does not let the scheduler open the parallel pool
1331+
* before its first stage starts.
1332+
*/
1333+
export function hasActiveBranchBoundTasksForProject(projectId: string): boolean {
1334+
const row = getDb()
1335+
.select({ cnt: count() })
1336+
.from(tasks)
1337+
.where(
1338+
and(
1339+
eq(tasks.projectId, projectId),
1340+
isNotNull(tasks.branchName),
1341+
inArray(tasks.status, [
1342+
"backlog",
1343+
"planning",
1344+
"plan_ready",
1345+
"implementing",
1346+
"review",
1347+
"blocked_external",
1348+
]),
1349+
),
1350+
)
1351+
.get();
1352+
return (row?.cnt ?? 0) > 0;
1353+
}
1354+
13221355
export function hasActiveLockedTaskForProject(projectId: string): boolean {
13231356
const nowIso = new Date().toISOString();
13241357
const row = getDb()

0 commit comments

Comments
 (0)