fix(triage): prevent orphaned deps when splitting tasks + detect worktree drift
Root cause: during a triage split the AI could set a child task's `dependencies` to the parent id. The parent is hard-deleted after the split, and the scheduler's dep check treats a missing id as unmet — permanently blocking the dependent. This stranded FN-2164 behind the deleted FN-2163. - core/store.deleteTask: refuse to delete when any live task still has the id in its `dependencies` array. Throws TaskHasDependentsError listing dependents so callers can rewrite or recover. Covers the triage-split path and any future caller. - engine/triage task_create: validate each proposed dependency before creating a child — reject the parent id, reject unknown task ids, allow siblings created earlier in the same split or pre-existing tasks. - engine/triage split cleanup: wrap the parent deleteTask in try/catch that keeps the parent alive (safer than stranding dependents) and logs the reason. - engine/triage prompts: both the mandatory-split and proactive-split prompts now explicitly state that subtask deps must never reference the parent. - dashboard/routes /subtasks/create-tasks: reject parent-id deps, drop unknown deps with an audit log entry, surface parentTaskCloseError + droppedDependencies in the response instead of silently swallowing them. - engine/executor: on execute entry, detect the drift state (in-progress task with no worktree) and emit a loud log + task log entry; the existing fresh-worktree path then recovers. Prevents silent "operating without a worktree" behavior that we saw on FN-2152. Tests: core: 2907/2907 pass (+5 new, incl. deleteTask guard regression) engine: 2554/2554 pass (+17 new, incl. task_create dep validation) dashboard: 9064/9064 pass (+2 new for /subtasks/create-tasks). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
@@ -23,7 +23,7 @@ const mockedExecSync = vi.mocked(execSync);
|
||||
import { runCommandAsync } from "./run-command.js";
|
||||
const mockedRunCommandAsync = vi.mocked(runCommandAsync);
|
||||
|
||||
import { TaskStore } from "./store.js";
|
||||
import { TaskStore, TaskHasDependentsError } from "./store.js";
|
||||
import { appendFile, readFile, writeFile, mkdir, rm, readdir, unlink } from "node:fs/promises";
|
||||
import { join } from "node:path";
|
||||
import { mkdtempSync, existsSync } from "node:fs";
|
||||
@@ -3888,6 +3888,50 @@ Task with acceptance criteria
|
||||
expect(logs).toEqual([]);
|
||||
});
|
||||
|
||||
it("deleteTask refuses when another live task depends on this id", async () => {
|
||||
// Regression for the triage-split bug: splitting a parent into children
|
||||
// used to hard-delete the parent even when a child carried the parent id
|
||||
// in its dependencies array, permanently blocking the child because the
|
||||
// scheduler treats missing-dep ids as unmet.
|
||||
const parent = await store.createTask({ description: "Parent to be split" });
|
||||
const child = await store.createTask({
|
||||
description: "Child that accidentally depends on parent",
|
||||
});
|
||||
await store.updateTask(child.id, { dependencies: [parent.id] });
|
||||
|
||||
await expect(store.deleteTask(parent.id)).rejects.toBeInstanceOf(TaskHasDependentsError);
|
||||
|
||||
// Parent must still exist so the dependent isn't stranded.
|
||||
const stillThere = await store.getTask(parent.id);
|
||||
expect(stillThere.id).toBe(parent.id);
|
||||
|
||||
// The error must name the dependent so callers/logs can triage it.
|
||||
try {
|
||||
await store.deleteTask(parent.id);
|
||||
} catch (err) {
|
||||
expect(err).toBeInstanceOf(TaskHasDependentsError);
|
||||
expect((err as TaskHasDependentsError).dependentIds).toContain(child.id);
|
||||
}
|
||||
|
||||
// After the dependent's reference is removed, delete succeeds.
|
||||
await store.updateTask(child.id, { dependencies: [] });
|
||||
await expect(store.deleteTask(parent.id)).resolves.toMatchObject({ id: parent.id });
|
||||
});
|
||||
|
||||
it("deleteTask allows deletion when a similarly-named id contains the target (substring false-positive guard)", async () => {
|
||||
// The LIKE probe uses '%id%'; ensure we don't misidentify e.g. FN-1 as
|
||||
// referencing FN-10 just because the id string appears inside a JSON
|
||||
// array containing "FN-10".
|
||||
const targetTask = await store.createTask({ description: "Target" }); // e.g. FN-001
|
||||
const similarId = `${targetTask.id}X`; // definitely not a real task id
|
||||
const other = await store.createTask({ description: "Other" });
|
||||
await store.updateTask(other.id, { dependencies: [similarId] });
|
||||
|
||||
// Should NOT throw — the LIKE probe's string match is disambiguated by
|
||||
// JSON.parse + array.includes.
|
||||
await expect(store.deleteTask(targetTask.id)).resolves.toMatchObject({ id: targetTask.id });
|
||||
});
|
||||
|
||||
it("deleting a task cascades agent log entry deletion", async () => {
|
||||
const task = await createTestTask();
|
||||
await store.appendAgentLog(task.id, "cascade me", "text");
|
||||
|
||||
@@ -85,6 +85,30 @@ export interface TaskStoreEvents {
|
||||
"agent:log": [entry: AgentLogEntry];
|
||||
}
|
||||
|
||||
/**
|
||||
* Thrown by {@link TaskStore.deleteTask} when the target task is still
|
||||
* referenced by at least one other live task's `dependencies` array.
|
||||
*
|
||||
* Callers that intend to split a task into children (e.g. triage, the
|
||||
* dashboard subtask-breakdown endpoint) must rewrite or drop those
|
||||
* references *before* deleting the parent — otherwise the dependents
|
||||
* would be permanently blocked by a nonexistent id.
|
||||
*/
|
||||
export class TaskHasDependentsError extends Error {
|
||||
readonly taskId: string;
|
||||
readonly dependentIds: string[];
|
||||
|
||||
constructor(taskId: string, dependentIds: string[]) {
|
||||
super(
|
||||
`Cannot delete task ${taskId}: still referenced as a dependency by ${dependentIds.join(", ")}. ` +
|
||||
`Rewrite or remove these dependencies before deleting.`,
|
||||
);
|
||||
this.name = "TaskHasDependentsError";
|
||||
this.taskId = taskId;
|
||||
this.dependentIds = dependentIds;
|
||||
}
|
||||
}
|
||||
|
||||
export class TaskStore extends EventEmitter<TaskStoreEvents> {
|
||||
static async getOrCreateForProject(
|
||||
projectId?: string,
|
||||
@@ -290,6 +314,7 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
|
||||
stuckKillCount: row.stuckKillCount ?? undefined,
|
||||
postReviewFixCount: row.postReviewFixCount ?? undefined,
|
||||
recoveryRetryCount: row.recoveryRetryCount ?? undefined,
|
||||
taskDoneRetryCount: row.taskDoneRetryCount ?? undefined,
|
||||
nextRecoveryAt: row.nextRecoveryAt || undefined,
|
||||
error: row.error || undefined,
|
||||
summary: row.summary || undefined,
|
||||
@@ -539,7 +564,7 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
|
||||
"modelPresetId", "modelProvider", "modelId",
|
||||
"validatorModelProvider", "validatorModelId",
|
||||
"planningModelProvider", "planningModelId",
|
||||
"mergeRetries", "workflowStepRetries", "stuckKillCount", "postReviewFixCount", "recoveryRetryCount", "nextRecoveryAt",
|
||||
"mergeRetries", "workflowStepRetries", "stuckKillCount", "postReviewFixCount", "recoveryRetryCount", "taskDoneRetryCount", "nextRecoveryAt",
|
||||
"error", "summary", "thinkingLevel",
|
||||
"createdAt", "updatedAt", "columnMovedAt",
|
||||
"dependencies", "steps", "comments", "workflowStepResults", "steeringComments",
|
||||
@@ -557,7 +582,7 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
|
||||
"modelPresetId", "modelProvider", "modelId",
|
||||
"validatorModelProvider", "validatorModelId",
|
||||
"planningModelProvider", "planningModelId",
|
||||
"mergeRetries", "workflowStepRetries", "stuckKillCount", "postReviewFixCount", "recoveryRetryCount", "nextRecoveryAt",
|
||||
"mergeRetries", "workflowStepRetries", "stuckKillCount", "postReviewFixCount", "recoveryRetryCount", "taskDoneRetryCount", "nextRecoveryAt",
|
||||
"error", "summary", "thinkingLevel",
|
||||
"createdAt", "updatedAt", "columnMovedAt",
|
||||
"dependencies", "steps", "attachments", "steeringComments",
|
||||
@@ -598,14 +623,14 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
|
||||
id, title, description, "column", status, size, reviewLevel, currentStep,
|
||||
worktree, blockedBy, paused, baseBranch, branch, baseCommitSha, modelPresetId, modelProvider,
|
||||
modelId, validatorModelProvider, validatorModelId, planningModelProvider, planningModelId, mergeRetries,
|
||||
workflowStepRetries, stuckKillCount, postReviewFixCount, recoveryRetryCount, nextRecoveryAt, error,
|
||||
workflowStepRetries, stuckKillCount, postReviewFixCount, recoveryRetryCount, taskDoneRetryCount, nextRecoveryAt, error,
|
||||
summary, thinkingLevel, createdAt, updatedAt, columnMovedAt,
|
||||
dependencies, steps, log, attachments, steeringComments,
|
||||
comments, workflowStepResults, prInfo, issueInfo, mergeDetails,
|
||||
breakIntoSubtasks, enabledWorkflowSteps, modifiedFiles, missionId, sliceId, assignedAgentId, assigneeUserId, checkedOutBy, checkedOutAt
|
||||
) VALUES (
|
||||
?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?,
|
||||
?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?
|
||||
?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?
|
||||
)
|
||||
ON CONFLICT(id) DO UPDATE SET
|
||||
title = excluded.title,
|
||||
@@ -633,6 +658,7 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
|
||||
stuckKillCount = excluded.stuckKillCount,
|
||||
postReviewFixCount = excluded.postReviewFixCount,
|
||||
recoveryRetryCount = excluded.recoveryRetryCount,
|
||||
taskDoneRetryCount = excluded.taskDoneRetryCount,
|
||||
nextRecoveryAt = excluded.nextRecoveryAt,
|
||||
error = excluded.error,
|
||||
summary = excluded.summary,
|
||||
@@ -686,6 +712,7 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
|
||||
task.stuckKillCount ?? 0,
|
||||
task.postReviewFixCount ?? 0,
|
||||
task.recoveryRetryCount ?? null,
|
||||
task.taskDoneRetryCount ?? 0,
|
||||
task.nextRecoveryAt ?? null,
|
||||
task.error ?? null,
|
||||
task.summary ?? null,
|
||||
@@ -728,6 +755,33 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
|
||||
return this.rowToTask(row);
|
||||
}
|
||||
|
||||
/**
|
||||
* Return the ids of live tasks whose `dependencies` array contains `id`.
|
||||
*
|
||||
* Uses a SQL LIKE probe as a cheap pre-filter then parses the JSON column
|
||||
* to rule out false positives (substring matches on similar ids, matches
|
||||
* inside escaped strings, etc.).
|
||||
*/
|
||||
private findLiveDependents(id: string): string[] {
|
||||
const rows = this.db
|
||||
.prepare(`SELECT id, dependencies FROM tasks WHERE dependencies LIKE ? AND id != ?`)
|
||||
.all(`%${id}%`, id) as Array<{ id: string; dependencies: string | null }>;
|
||||
|
||||
const dependents: string[] = [];
|
||||
for (const row of rows) {
|
||||
if (!row.dependencies) continue;
|
||||
try {
|
||||
const deps = JSON.parse(row.dependencies) as unknown;
|
||||
if (Array.isArray(deps) && deps.includes(id)) {
|
||||
dependents.push(row.id);
|
||||
}
|
||||
} catch {
|
||||
// Malformed JSON — skip; nothing we can verify.
|
||||
}
|
||||
}
|
||||
return dependents;
|
||||
}
|
||||
|
||||
/**
|
||||
* Set up event listeners for activity logging.
|
||||
* Call after init() to record task lifecycle events.
|
||||
@@ -2109,7 +2163,7 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
|
||||
|
||||
async updateTask(
|
||||
id: string,
|
||||
updates: { title?: string; description?: string; prompt?: string; worktree?: string | null; status?: string | null; dependencies?: string[]; steps?: import("./types.js").TaskStep[]; blockedBy?: string | null; assignedAgentId?: string | null; assigneeUserId?: string | null; checkedOutBy?: string | null; checkedOutAt?: string | null; paused?: boolean; baseBranch?: string | null; branch?: string | null; baseCommitSha?: string | null; size?: "S" | "M" | "L"; reviewLevel?: number; mergeRetries?: number; workflowStepRetries?: number; stuckKillCount?: number | null; postReviewFixCount?: number | null; recoveryRetryCount?: number | null; nextRecoveryAt?: string | null; enabledWorkflowSteps?: string[]; modelProvider?: string | null; modelId?: string | null; validatorModelProvider?: string | null; validatorModelId?: string | null; planningModelProvider?: string | null; planningModelId?: string | null; thinkingLevel?: string | null; error?: string | null; summary?: string | null; sessionFile?: string | null; workflowStepResults?: import("./types.js").WorkflowStepResult[] | null; mergeDetails?: import("./types.js").MergeDetails | null; modifiedFiles?: string[] | null; missionId?: string | null; sliceId?: string | null },
|
||||
updates: { title?: string; description?: string; prompt?: string; worktree?: string | null; status?: string | null; dependencies?: string[]; steps?: import("./types.js").TaskStep[]; blockedBy?: string | null; assignedAgentId?: string | null; assigneeUserId?: string | null; checkedOutBy?: string | null; checkedOutAt?: string | null; paused?: boolean; baseBranch?: string | null; branch?: string | null; baseCommitSha?: string | null; size?: "S" | "M" | "L"; reviewLevel?: number; mergeRetries?: number; workflowStepRetries?: number; stuckKillCount?: number | null; postReviewFixCount?: number | null; recoveryRetryCount?: number | null; taskDoneRetryCount?: number | null; nextRecoveryAt?: string | null; enabledWorkflowSteps?: string[]; modelProvider?: string | null; modelId?: string | null; validatorModelProvider?: string | null; validatorModelId?: string | null; planningModelProvider?: string | null; planningModelId?: string | null; thinkingLevel?: string | null; error?: string | null; summary?: string | null; sessionFile?: string | null; workflowStepResults?: import("./types.js").WorkflowStepResult[] | null; mergeDetails?: import("./types.js").MergeDetails | null; modifiedFiles?: string[] | null; missionId?: string | null; sliceId?: string | null },
|
||||
runContext?: RunMutationContext,
|
||||
): Promise<Task> {
|
||||
return this.withTaskLock(id, async () => {
|
||||
@@ -2219,6 +2273,11 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
|
||||
} else if (updates.recoveryRetryCount !== undefined) {
|
||||
task.recoveryRetryCount = updates.recoveryRetryCount;
|
||||
}
|
||||
if (updates.taskDoneRetryCount === null) {
|
||||
task.taskDoneRetryCount = undefined;
|
||||
} else if (updates.taskDoneRetryCount !== undefined) {
|
||||
task.taskDoneRetryCount = updates.taskDoneRetryCount;
|
||||
}
|
||||
if (updates.nextRecoveryAt === null) {
|
||||
task.nextRecoveryAt = undefined;
|
||||
} else if (updates.nextRecoveryAt !== undefined) {
|
||||
@@ -2789,6 +2848,16 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
|
||||
throw new Error(`Task ${id} not found`);
|
||||
}
|
||||
|
||||
// Refuse to delete a task that is still referenced as a dependency
|
||||
// by another live task. Scheduler treats missing-dep ids as unmet,
|
||||
// so silently deleting a task with live dependents would permanently
|
||||
// block them. Callers that want to split/replace a task must rewrite
|
||||
// or drop the incoming references first.
|
||||
const dependentIds = this.findLiveDependents(id);
|
||||
if (dependentIds.length > 0) {
|
||||
throw new TaskHasDependentsError(id, dependentIds);
|
||||
}
|
||||
|
||||
// Clean up the task's branch before deleting from DB
|
||||
const cleanedBranches = await this.cleanupBranchForTask(task);
|
||||
if (cleanedBranches.length > 0) {
|
||||
|
||||
Reference in New Issue
Block a user