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:
@@ -430,6 +430,7 @@ describe("SelfHealingManager", () => {
|
||||
const recoverNoProgressNoTaskDoneFailures = vi.spyOn(manager, "recoverNoProgressNoTaskDoneFailures").mockResolvedValue(1);
|
||||
const recoverCompletedTasks = vi.spyOn(manager, "recoverCompletedTasks").mockResolvedValue(1);
|
||||
const recoverMisclassifiedFailures = vi.spyOn(manager, "recoverMisclassifiedFailures").mockResolvedValue(1);
|
||||
const recoverPartialProgressNoTaskDoneFailures = vi.spyOn(manager, "recoverPartialProgressNoTaskDoneFailures").mockResolvedValue(1);
|
||||
const recoverOrphanedExecutions = vi.spyOn(manager, "recoverOrphanedExecutions").mockResolvedValue(1);
|
||||
const recoverApprovedTriageTasks = vi.spyOn(manager, "recoverApprovedTriageTasks").mockResolvedValue(1);
|
||||
|
||||
@@ -438,6 +439,7 @@ describe("SelfHealingManager", () => {
|
||||
expect(recoverNoProgressNoTaskDoneFailures).toHaveBeenCalledTimes(1);
|
||||
expect(recoverCompletedTasks).toHaveBeenCalledTimes(1);
|
||||
expect(recoverMisclassifiedFailures).toHaveBeenCalledTimes(1);
|
||||
expect(recoverPartialProgressNoTaskDoneFailures).toHaveBeenCalledTimes(1);
|
||||
expect(recoverOrphanedExecutions).toHaveBeenCalledTimes(1);
|
||||
expect(recoverApprovedTriageTasks).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
@@ -1023,6 +1025,145 @@ describe("SelfHealingManager", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("recoverPartialProgressNoTaskDoneFailures", () => {
|
||||
it("requeues partial-progress no-task_done failures with bounded retry count", async () => {
|
||||
const managerWithRecovery = new SelfHealingManager(store, {
|
||||
rootDir: "/tmp/test-project",
|
||||
});
|
||||
|
||||
(store.listTasks as ReturnType<typeof vi.fn>).mockResolvedValue([
|
||||
{
|
||||
id: "FN-2164",
|
||||
column: "in-review",
|
||||
status: "failed",
|
||||
error: "Agent finished without calling task_done (after retry)",
|
||||
paused: false,
|
||||
steps: [{ status: "done" }, { status: "pending" }, { status: "pending" }],
|
||||
log: [],
|
||||
},
|
||||
]);
|
||||
|
||||
const result = await managerWithRecovery.recoverPartialProgressNoTaskDoneFailures();
|
||||
|
||||
expect(result).toBe(1);
|
||||
expect(store.listTasks).toHaveBeenCalledWith({ column: "in-review" });
|
||||
expect(store.updateTask).toHaveBeenCalledWith("FN-2164", {
|
||||
status: null,
|
||||
error: null,
|
||||
sessionFile: null,
|
||||
taskDoneRetryCount: 1,
|
||||
});
|
||||
expect(store.logEntry).toHaveBeenCalledWith(
|
||||
"FN-2164",
|
||||
expect.stringContaining("Auto-retry 1/3"),
|
||||
);
|
||||
expect(store.moveTask).toHaveBeenCalledWith("FN-2164", "todo");
|
||||
|
||||
managerWithRecovery.stop();
|
||||
});
|
||||
|
||||
it("skips tasks whose retry count has reached the max", async () => {
|
||||
const managerWithRecovery = new SelfHealingManager(store, {
|
||||
rootDir: "/tmp/test-project",
|
||||
});
|
||||
|
||||
(store.listTasks as ReturnType<typeof vi.fn>).mockResolvedValue([
|
||||
{
|
||||
id: "FN-2164",
|
||||
column: "in-review",
|
||||
status: "failed",
|
||||
error: "Agent finished without calling task_done (after retry)",
|
||||
paused: false,
|
||||
taskDoneRetryCount: 3,
|
||||
steps: [{ status: "done" }, { status: "pending" }],
|
||||
log: [],
|
||||
},
|
||||
]);
|
||||
|
||||
const result = await managerWithRecovery.recoverPartialProgressNoTaskDoneFailures();
|
||||
|
||||
expect(result).toBe(0);
|
||||
expect(store.updateTask).not.toHaveBeenCalled();
|
||||
expect(store.moveTask).not.toHaveBeenCalled();
|
||||
|
||||
managerWithRecovery.stop();
|
||||
});
|
||||
|
||||
it("skips tasks where all steps are already done (handled by misclassified recovery)", async () => {
|
||||
const managerWithRecovery = new SelfHealingManager(store, {
|
||||
rootDir: "/tmp/test-project",
|
||||
});
|
||||
|
||||
(store.listTasks as ReturnType<typeof vi.fn>).mockResolvedValue([
|
||||
{
|
||||
id: "FN-2164",
|
||||
column: "in-review",
|
||||
status: "failed",
|
||||
error: "Agent finished without calling task_done (after retry)",
|
||||
paused: false,
|
||||
steps: [{ status: "done" }, { status: "done" }, { status: "skipped" }],
|
||||
log: [],
|
||||
},
|
||||
]);
|
||||
|
||||
const result = await managerWithRecovery.recoverPartialProgressNoTaskDoneFailures();
|
||||
|
||||
expect(result).toBe(0);
|
||||
expect(store.updateTask).not.toHaveBeenCalled();
|
||||
|
||||
managerWithRecovery.stop();
|
||||
});
|
||||
|
||||
it("skips tasks with zero step progress (handled by no-progress recovery)", async () => {
|
||||
const managerWithRecovery = new SelfHealingManager(store, {
|
||||
rootDir: "/tmp/test-project",
|
||||
});
|
||||
|
||||
(store.listTasks as ReturnType<typeof vi.fn>).mockResolvedValue([
|
||||
{
|
||||
id: "FN-2164",
|
||||
column: "in-review",
|
||||
status: "failed",
|
||||
error: "Agent finished without calling task_done (after retry)",
|
||||
paused: false,
|
||||
steps: [{ status: "pending" }, { status: "pending" }],
|
||||
log: [],
|
||||
},
|
||||
]);
|
||||
|
||||
const result = await managerWithRecovery.recoverPartialProgressNoTaskDoneFailures();
|
||||
|
||||
expect(result).toBe(0);
|
||||
expect(store.updateTask).not.toHaveBeenCalled();
|
||||
|
||||
managerWithRecovery.stop();
|
||||
});
|
||||
|
||||
it("skips tasks with unrelated failure reasons", async () => {
|
||||
const managerWithRecovery = new SelfHealingManager(store, {
|
||||
rootDir: "/tmp/test-project",
|
||||
});
|
||||
|
||||
(store.listTasks as ReturnType<typeof vi.fn>).mockResolvedValue([
|
||||
{
|
||||
id: "FN-2164",
|
||||
column: "in-review",
|
||||
status: "failed",
|
||||
error: "Workflow step failed",
|
||||
paused: false,
|
||||
steps: [{ status: "done" }, { status: "pending" }],
|
||||
log: [],
|
||||
},
|
||||
]);
|
||||
|
||||
const result = await managerWithRecovery.recoverPartialProgressNoTaskDoneFailures();
|
||||
|
||||
expect(result).toBe(0);
|
||||
|
||||
managerWithRecovery.stop();
|
||||
});
|
||||
});
|
||||
|
||||
describe("recoverMergedReviewTasks", () => {
|
||||
it("finalizes stale merging tasks when a task commit already landed", async () => {
|
||||
const managerWithRecovery = new SelfHealingManager(store, {
|
||||
@@ -2210,6 +2351,7 @@ describe("maintenance cycle concurrency", () => {
|
||||
(vi.spyOn(manager as any, "recoverMergedReviewTasks").mockResolvedValue(0) as any);
|
||||
(vi.spyOn(manager as any, "recoverMisclassifiedFailures").mockResolvedValue(0) as any);
|
||||
(vi.spyOn(manager as any, "recoverNoProgressNoTaskDoneFailures").mockResolvedValue(0) as any);
|
||||
(vi.spyOn(manager as any, "recoverPartialProgressNoTaskDoneFailures").mockResolvedValue(0) as any);
|
||||
(vi.spyOn(manager as any, "recoverOrphanedExecutions").mockResolvedValue(0) as any);
|
||||
(vi.spyOn(manager as any, "recoverApprovedTriageTasks").mockResolvedValue(0) as any);
|
||||
(vi.spyOn(manager as any, "recoverOrphanedSpecifyingTasks").mockResolvedValue(0) as any);
|
||||
@@ -2276,6 +2418,7 @@ describe("maintenance cycle concurrency", () => {
|
||||
makeSlow("recoverMergedReviewTasks");
|
||||
makeSlow("recoverMisclassifiedFailures");
|
||||
makeSlow("recoverNoProgressNoTaskDoneFailures");
|
||||
makeSlow("recoverPartialProgressNoTaskDoneFailures");
|
||||
makeSlow("recoverOrphanedExecutions");
|
||||
makeSlow("recoverApprovedTriageTasks");
|
||||
makeSlow("recoverOrphanedSpecifyingTasks");
|
||||
@@ -2297,6 +2440,7 @@ describe("maintenance cycle concurrency", () => {
|
||||
"recoverMergedReviewTasks",
|
||||
"recoverMisclassifiedFailures",
|
||||
"recoverNoProgressNoTaskDoneFailures",
|
||||
"recoverPartialProgressNoTaskDoneFailures",
|
||||
"recoverOrphanedExecutions",
|
||||
"recoverApprovedTriageTasks",
|
||||
"recoverOrphanedSpecifyingTasks",
|
||||
|
||||
@@ -80,6 +80,13 @@ const NON_TERMINAL_STEP_STATUSES = new Set(["pending", "in-progress"]);
|
||||
*/
|
||||
const ORPHANED_WITH_WORKTREE_GRACE_MS = 300_000;
|
||||
|
||||
/**
|
||||
* Maximum times a task can be auto-requeued after the agent exits without
|
||||
* calling `task_done`. Bounded so a persistently-broken task cannot loop
|
||||
* forever; when exhausted the task stays in `in-review` for human inspection.
|
||||
*/
|
||||
const MAX_TASK_DONE_RETRIES = 3;
|
||||
|
||||
interface LandedTaskCommit {
|
||||
sha: string;
|
||||
subject?: string;
|
||||
@@ -155,6 +162,7 @@ export class SelfHealingManager {
|
||||
{ name: "failed-pre-merge-steps", fn: () => this.recoverReviewTasksWithFailedPreMergeSteps().then(() => undefined) },
|
||||
{ name: "interrupted-merging", fn: () => this.recoverInterruptedMergingTasks().then(() => undefined) },
|
||||
{ name: "misclassified-failures", fn: () => this.recoverMisclassifiedFailures().then(() => undefined) },
|
||||
{ name: "partial-progress-no-task-done", fn: () => this.recoverPartialProgressNoTaskDoneFailures().then(() => undefined) },
|
||||
{ name: "orphaned-executions", fn: () => this.recoverOrphanedExecutions().then(() => undefined) },
|
||||
{ name: "approved-triage", fn: () => this.recoverApprovedTriageTasks().then(() => undefined) },
|
||||
{ name: "orphaned-specifying", fn: () => this.recoverOrphanedSpecifyingTasks().then(() => undefined) },
|
||||
@@ -532,6 +540,7 @@ export class SelfHealingManager {
|
||||
{ name: "recover-merged-review", fn: () => this.recoverMergedReviewTasks() },
|
||||
{ name: "recover-misclassified-failures", fn: () => this.recoverMisclassifiedFailures() },
|
||||
{ name: "recover-no-progress-no-task-done", fn: () => this.recoverNoProgressNoTaskDoneFailures() },
|
||||
{ name: "recover-partial-progress-no-task-done", fn: () => this.recoverPartialProgressNoTaskDoneFailures() },
|
||||
{ name: "recover-orphaned-executions", fn: () => this.recoverOrphanedExecutions() },
|
||||
{ name: "recover-approved-triage", fn: () => this.recoverApprovedTriageTasks() },
|
||||
{ name: "recover-orphaned-specifying", fn: () => this.recoverOrphanedSpecifyingTasks() },
|
||||
@@ -1197,6 +1206,83 @@ export class SelfHealingManager {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Recover `in-review` tasks marked as `failed` because the agent exited
|
||||
* without calling `task_done` *with partial step progress* (some steps done,
|
||||
* some still pending). The work-in-progress is valuable but incomplete —
|
||||
* the existing worktree and branch are preserved and the task is moved back
|
||||
* to `todo` so the scheduler re-dispatches it for a fresh execution that
|
||||
* continues from where the previous attempt left off.
|
||||
*
|
||||
* Bounded by `MAX_TASK_DONE_RETRIES` (per-task `taskDoneRetryCount`) so a
|
||||
* persistently-broken task cannot loop forever; when exhausted the task
|
||||
* remains parked in `in-review` for manual intervention. The counter is
|
||||
* cleared by the executor on successful completion.
|
||||
*
|
||||
* Distinct from sibling recoveries:
|
||||
* - `recoverMisclassifiedFailures`: all steps done → clear error, leave for review.
|
||||
* - `recoverNoProgressNoTaskDoneFailures`: `in-progress` with zero progress → clean requeue.
|
||||
* - This one: `in-review` with partial progress → bounded requeue preserving work.
|
||||
*
|
||||
* @returns Number of tasks requeued for retry
|
||||
*/
|
||||
async recoverPartialProgressNoTaskDoneFailures(): Promise<number> {
|
||||
try {
|
||||
const tasks = await this.store.listTasks({ column: "in-review" });
|
||||
|
||||
const candidates = tasks.filter((task) =>
|
||||
task.column === "in-review" &&
|
||||
task.status === "failed" &&
|
||||
isNoTaskDoneFailure(task) &&
|
||||
!task.paused &&
|
||||
!isTaskWorkComplete(task) &&
|
||||
hasStepProgress(task) &&
|
||||
(task.taskDoneRetryCount ?? 0) < MAX_TASK_DONE_RETRIES,
|
||||
);
|
||||
|
||||
if (candidates.length === 0) return 0;
|
||||
|
||||
log.warn(
|
||||
`Found ${candidates.length} partial-progress no-task_done failure(s) eligible for auto-retry`,
|
||||
);
|
||||
|
||||
let recovered = 0;
|
||||
for (const task of candidates) {
|
||||
try {
|
||||
const nextCount = (task.taskDoneRetryCount ?? 0) + 1;
|
||||
await this.store.updateTask(task.id, {
|
||||
status: null,
|
||||
error: null,
|
||||
sessionFile: null,
|
||||
taskDoneRetryCount: nextCount,
|
||||
});
|
||||
await this.store.logEntry(
|
||||
task.id,
|
||||
`Auto-retry ${nextCount}/${MAX_TASK_DONE_RETRIES}: agent finished without task_done — requeuing to todo to resume partial work`,
|
||||
);
|
||||
await this.store.moveTask(task.id, "todo");
|
||||
recovered++;
|
||||
} catch (err: unknown) {
|
||||
const errorMessage = err instanceof Error ? err.message : String(err);
|
||||
log.error(
|
||||
`Failed to auto-retry partial-progress no-task_done failure ${task.id}: ${errorMessage}`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
if (recovered > 0) {
|
||||
log.log(
|
||||
`Auto-retried ${recovered} partial-progress no-task_done failure(s) → todo`,
|
||||
);
|
||||
}
|
||||
return recovered;
|
||||
} catch (err: unknown) {
|
||||
const errorMessage = err instanceof Error ? err.message : String(err);
|
||||
log.error(`Partial-progress no-task_done recovery failed: ${errorMessage}`);
|
||||
return 0;
|
||||
}
|
||||
}
|
||||
|
||||
private async hasRecoverableGitWork(task: Task): Promise<boolean> {
|
||||
if (task.worktree && existsSync(task.worktree)) {
|
||||
try {
|
||||
|
||||
Reference in New Issue
Block a user