feat(FN-5436): skip step retries when review is pending

Added executor logic to skip retries when a review is pending for a task, introducing a `pendingReviewBlockHelper` in the task-done path and updating the retry-gate to consult it; two new reliability-interaction test suites cover the feature behavior and composition with existing retry/backstop laye

Fusion-Task-Id: FN-5436
This commit is contained in:
Fusion (runfusion.ai)
2026-05-21 10:57:44 -07:00
committed by gsxdsm
parent 7d25d98b2f
commit a2a643844d
4 changed files with 430 additions and 1 deletions

View File

@@ -439,7 +439,7 @@ The Ink-based TUI is part of `fn` (no separate `@fusion/tui` package). Implement
Structured logging via `createLogger()` from `packages/engine/src/logger.ts`. All lines prefixed with subsystem name. See [docs/diagnostics.md](./docs/diagnostics.md) for the full key-diagnostic-points catalog. Notable subsystems include `[executor]`, `[scheduler]`, `[stuck-detector]`, `[auto-claim-snapshot]`, `[prompt-size]`, `[wake-trigger-diagnostics]`, `[retry-burned]`, and `[room-ambiguity]`.
`AgentSemaphore` (`packages/engine/src/concurrency.ts`) has defensive guards: `limit` getter returns minimum 1; `availableCount` returns 0 for invalid limits.
- `[executor] FN-XXX: fn_task_done refused (<class>) — <reason>` (explicit tool path) and `[executor] FN-XXX: fn_task_done refused (<class>) — <reason> (implicit completion)` (implicit all-steps-done path) now share refusal-class diagnostics for `summary-claims-incomplete` (explicit only), `bulk-step-completion-without-review`, and `pending-code-review-revise`; both paths consume the same `MAX_TASK_DONE_REQUEUE_RETRIES` budget and escalate to `in-review` with `status: "failed"` on exhaustion.
- `[executor] FN-XXX: fn_task_done refused (<class>) — <reason>` (explicit tool path) and `[executor] FN-XXX: fn_task_done refused (<class>) — <reason> (implicit completion)` (implicit all-steps-done path) now share refusal-class diagnostics for `summary-claims-incomplete` (explicit only), `bulk-step-completion-without-review`, and `pending-code-review-revise`; both paths consume the same `MAX_TASK_DONE_REQUEUE_RETRIES` budget and escalate to `in-review` with `status: "failed"` on exhaustion. The no-`fn_task_done` retry loop also emits `[executor] <taskId>: fn_task_done not called but task is blocked on pending review (<reason>) — skipping retry session` and parks the task in `in-review` with `error: "executor-exit-while-review-pending"`.
- Done/archived transitions must clear stale pause metadata (`paused`, `userPaused`, `pausedByAgentId`, `pausedReason`), and `formatTaskLine` suppresses `(paused)` for terminal columns even if stale storage state exists.
## Dashboard UI Styling Guide

View File

@@ -192,6 +192,202 @@ describe("Workflow Steps Execution", () => {
);
});
describe("FN-5436: pending-review skip on no-fn_task_done exit", () => {
it("parks in-review immediately when code review REVISE is pending", async () => {
const store = createMockStore();
const baseTask = {
id: "FN-5436-A",
title: "Test",
description: "Test task",
column: "in-progress",
dependencies: [],
steps: [{ name: "Implement", status: "in-progress" }],
currentStep: 0,
log: [],
prompt: "# test\n## Steps\n### Step 1: Implement\n- [ ] implement",
createdAt: new Date().toISOString(),
updatedAt: new Date().toISOString(),
};
store.getTask.mockResolvedValue(baseTask as any);
mockedReviewStep.mockResolvedValue({
verdict: "REVISE",
review: "needs changes",
summary: "needs changes",
});
mockedCreateFnAgent.mockImplementation((async (opts: any) => {
const tools = opts.customTools || [];
return {
session: {
prompt: vi.fn().mockImplementation(async () => {
const reviewTool = tools.find((t: any) => t.name === "fn_review_step");
if (reviewTool) {
await reviewTool.execute("tool-review", { step: 1, type: "code", step_name: "Implement" });
}
}),
dispose: vi.fn(),
subscribe: vi.fn(),
on: vi.fn(),
sessionManager: { getLeafId: vi.fn().mockReturnValue("leaf-1") },
state: {},
},
};
}) as any);
const onError = vi.fn();
const executor = new TaskExecutor(store, "/tmp/test", { onError });
await executor.execute(baseTask as any);
expect(mockedCreateFnAgent).toHaveBeenCalledTimes(1);
expect(store.updateTask).toHaveBeenCalledWith("FN-5436-A", {
status: "failed",
error: "executor-exit-while-review-pending",
});
expect(store.updateTask).not.toHaveBeenCalledWith("FN-5436-A", expect.objectContaining({ taskDoneRetryCount: expect.anything() }));
expect(store.moveTask).toHaveBeenCalledWith("FN-5436-A", "in-review");
expect(store.logEntry).toHaveBeenCalledWith(
"FN-5436-A",
expect.stringContaining("blocked on pending review (code-review-revise-outstanding)"),
undefined,
expect.objectContaining({ agentId: "executor" }),
);
expect(onError).toHaveBeenCalledWith(
expect.objectContaining({ id: "FN-5436-A" }),
expect.objectContaining({ message: "executor-exit-while-review-pending" }),
);
});
it("parks in-review when review request has no subsequent verdict", async () => {
const store = createMockStore();
const baseTask = {
id: "FN-5436-B",
title: "Test",
description: "Test task",
column: "in-progress",
dependencies: [],
steps: [{ name: "Implement", status: "in-progress" }],
currentStep: 0,
log: [{ action: "code review requested for Step 1 (Implement)", timestamp: new Date().toISOString() }],
prompt: "# test\n## Steps\n### Step 1: Implement\n- [ ] implement",
createdAt: new Date().toISOString(),
updatedAt: new Date().toISOString(),
};
store.getTask.mockResolvedValue(baseTask as any);
mockedCreateFnAgent.mockResolvedValue({
session: {
prompt: vi.fn().mockResolvedValue(undefined),
dispose: vi.fn(),
subscribe: vi.fn(),
on: vi.fn(),
sessionManager: { getLeafId: vi.fn().mockReturnValue("leaf-1") },
state: {},
},
} as any);
const executor = new TaskExecutor(store, "/tmp/test", {});
await executor.execute(baseTask as any);
expect(mockedCreateFnAgent).toHaveBeenCalledTimes(1);
expect(store.updateTask).toHaveBeenCalledWith("FN-5436-B", {
status: "failed",
error: "executor-exit-while-review-pending",
});
expect(store.logEntry).toHaveBeenCalledWith(
"FN-5436-B",
expect.stringContaining("blocked on pending review (review-request-without-verdict)"),
undefined,
expect.objectContaining({ agentId: "executor" }),
);
expect(store.moveTask).toHaveBeenCalledWith("FN-5436-B", "in-review");
});
it("keeps existing retry loop when no pending review block is present", async () => {
const store = createMockStore();
const baseTask = {
id: "FN-5436-C",
title: "Test",
description: "Test task",
column: "in-progress",
dependencies: [],
steps: [{ name: "Implement", status: "in-progress" }],
currentStep: 0,
log: [{ action: "code review Step 1: APPROVE", timestamp: new Date().toISOString() }],
prompt: "# test\n## Steps\n### Step 1: Implement\n- [ ] implement",
createdAt: new Date().toISOString(),
updatedAt: new Date().toISOString(),
};
store.getTask.mockResolvedValue(baseTask as any);
mockedCreateFnAgent.mockResolvedValue({
session: {
prompt: vi.fn().mockResolvedValue(undefined),
dispose: vi.fn(),
subscribe: vi.fn(),
on: vi.fn(),
sessionManager: { getLeafId: vi.fn().mockReturnValue("leaf-1") },
state: {},
},
} as any);
const executor = new TaskExecutor(store, "/tmp/test", {});
await executor.execute(baseTask as any);
expect(mockedCreateFnAgent).toHaveBeenCalledTimes(4);
expect(store.updateTask).toHaveBeenCalledWith("FN-5436-C", {
status: "failed",
error: "Agent finished without calling fn_task_done (after 3 retries)",
taskDoneRetryCount: 1,
});
});
it("allows implicit done to complete when no in-progress step exists", async () => {
const store = createMockStore();
const baseTask = {
id: "FN-5436-D",
title: "Test",
description: "Test task",
column: "in-progress",
dependencies: [],
steps: [{ name: "Implement", status: "done" }],
currentStep: 0,
log: [],
prompt: "# test\n## Steps\n### Step 1: Implement\n- [x] implement",
createdAt: new Date().toISOString(),
updatedAt: new Date().toISOString(),
};
store.getTask.mockResolvedValue(baseTask as any);
mockedCreateFnAgent.mockResolvedValue({
session: {
prompt: vi.fn().mockResolvedValue(undefined),
dispose: vi.fn(),
subscribe: vi.fn(),
on: vi.fn(),
sessionManager: { getLeafId: vi.fn().mockReturnValue("leaf-1") },
state: {},
},
} as any);
const onComplete = vi.fn();
const onError = vi.fn();
const executor = new TaskExecutor(store, "/tmp/test", { onComplete, onError });
await executor.execute(baseTask as any);
expect(mockedCreateFnAgent).toHaveBeenCalledTimes(1);
expect(store.updateTask).toHaveBeenCalledWith("FN-5436-D", { workflowStepRetries: undefined, taskDoneRetryCount: null });
expect(store.updateTask).not.toHaveBeenCalledWith("FN-5436-D", {
status: "failed",
error: "executor-exit-while-review-pending",
});
expect(store.moveTask).toHaveBeenCalledWith("FN-5436-D", "in-review");
expect(onComplete).toHaveBeenCalled();
expect(onError).not.toHaveBeenCalled();
});
});
it("runs workflow steps after main task execution", async () => {
const store = createMockStore();

View File

@@ -0,0 +1,134 @@
import { beforeEach, describe, expect, it, vi } from "vitest";
import "../executor-test-helpers.js";
import { TaskExecutor } from "../../executor.js";
import { createFnAgent } from "../../pi.js";
import { createMockStore, resetExecutorMocks } from "../executor-test-helpers.js";
const mockedCreateFnAgent = vi.mocked(createFnAgent);
function makeTask(overrides: Record<string, unknown> = {}) {
return {
id: "FN-5436-RI",
title: "Pending review skip",
description: "",
column: "in-progress",
dependencies: [],
taskDoneRetryCount: 0,
steps: [{ name: "Step 1", status: "in-progress" as const }],
currentStep: 0,
log: [],
prompt: "# test\n## Steps\n### Step 1: Step 1\n- [ ] do work",
createdAt: new Date().toISOString(),
updatedAt: new Date().toISOString(),
...overrides,
} as any;
}
describe("reliability interactions: FN-5436 executor pending-review skip", () => {
beforeEach(() => {
resetExecutorMocks();
mockedCreateFnAgent.mockResolvedValue({
session: {
prompt: vi.fn().mockResolvedValue(undefined),
dispose: vi.fn(),
subscribe: vi.fn(),
on: vi.fn(),
sessionManager: { getLeafId: vi.fn().mockReturnValue("leaf-1") },
state: {},
},
} as any);
});
it("FN-5436 composition: implicit-done wins when no in-progress step exists despite stale review logs", async () => {
const store = createMockStore();
const task = makeTask({
id: "FN-5436-RI-A",
steps: [{ name: "Step 1", status: "done" }],
log: [{ action: "code review Step 1: REVISE", timestamp: new Date().toISOString() }],
});
store.getTask.mockResolvedValue(task);
const executor = new TaskExecutor(store as any, "/repo");
await executor.execute(task);
expect(store.updateTask).not.toHaveBeenCalledWith("FN-5436-RI-A", {
status: "failed",
error: "executor-exit-while-review-pending",
});
expect(store.moveTask).toHaveBeenCalledWith("FN-5436-RI-A", "in-review");
});
it("FN-5436 composition: reclaim-abort path takes precedence over pending-review skip", async () => {
const store = createMockStore();
const task = makeTask({ id: "FN-5436-RI-B", paused: true });
store.getTask.mockResolvedValue(task);
const executor = new TaskExecutor(store as any, "/repo");
await executor.execute(task);
expect(store.moveTask).toHaveBeenCalledWith("FN-5436-RI-B", "todo", { preserveProgress: true });
expect(store.updateTask).not.toHaveBeenCalledWith("FN-5436-RI-B", {
status: "failed",
error: "executor-exit-while-review-pending",
});
});
it("FN-5436 composition: pending-review park does not consume taskDone requeue budget", async () => {
const store = createMockStore();
const task = makeTask({
id: "FN-5436-RI-C",
taskDoneRetryCount: 2,
log: [{ action: "code review requested for Step 1 (Step 1)", timestamp: new Date().toISOString() }],
});
store.getTask.mockResolvedValue(task);
const executor = new TaskExecutor(store as any, "/repo");
await executor.execute(task);
expect(store.updateTask).toHaveBeenCalledWith("FN-5436-RI-C", {
status: "failed",
error: "executor-exit-while-review-pending",
});
expect(store.updateTask).not.toHaveBeenCalledWith("FN-5436-RI-C", expect.objectContaining({ taskDoneRetryCount: 3 }));
expect(store.moveTask).toHaveBeenCalledWith("FN-5436-RI-C", "in-review");
});
it("FN-5436 composition: recoverApprovedStepsOnResume leaves pending-review skip disabled after approval resolves step", async () => {
const store = createMockStore();
const task = makeTask({
id: "FN-5436-RI-D",
steps: [{ name: "Step 1", status: "done" }],
log: [{ action: "code review Step 1: APPROVE", timestamp: new Date().toISOString() }],
});
store.getTask.mockResolvedValue(task);
const executor = new TaskExecutor(store as any, "/repo");
await executor.execute(task);
expect(store.updateTask).toHaveBeenCalledWith("FN-5436-RI-D", { workflowStepRetries: undefined, taskDoneRetryCount: null });
expect(store.updateTask).not.toHaveBeenCalledWith("FN-5436-RI-D", {
status: "failed",
error: "executor-exit-while-review-pending",
});
expect(store.moveTask).toHaveBeenCalledWith("FN-5436-RI-D", "in-review");
});
it("FN-5436 negative: plan-review UNAVAILABLE advisory remains non-blocking", async () => {
const store = createMockStore();
const task = makeTask({
id: "FN-5436-RI-E",
log: [{ action: "plan review Step 1: UNAVAILABLE — proceeding advisory after fallback retry exhausted", timestamp: new Date().toISOString() }],
});
store.getTask.mockResolvedValue(task);
const executor = new TaskExecutor(store as any, "/repo");
await executor.execute(task);
expect(mockedCreateFnAgent).toHaveBeenCalledTimes(4);
expect(store.updateTask).toHaveBeenCalledWith("FN-5436-RI-E", {
status: "failed",
error: "Agent finished without calling fn_task_done (after 3 retries)",
taskDoneRetryCount: 1,
});
});
});

View File

@@ -209,6 +209,77 @@ type TaskDoneRefusalResult =
reason: string;
};
type PendingReviewBlockResult =
| {
blocked: true;
reason:
| "code-review-revise-outstanding"
| "review-request-without-verdict"
| "code-review-rethink-or-unavailable-outstanding"
| "code-review-unavailable-blocking";
stepIndex: number;
}
| { blocked: false };
function detectPendingReviewBlock(
task: Task,
codeReviewVerdicts: Map<number, ReviewVerdict>,
): PendingReviewBlockResult {
const inProgressStepIndices: number[] = [];
for (let stepIndex = 0; stepIndex < task.steps.length; stepIndex++) {
if (task.steps[stepIndex]?.status === "in-progress") {
inProgressStepIndices.push(stepIndex);
}
}
if (inProgressStepIndices.length === 0) {
return { blocked: false };
}
const recentActions = (task.log ?? [])
.slice(-30)
.map((entry) => entry.action?.trim())
.filter((action): action is string => Boolean(action));
for (const stepIndex of inProgressStepIndices) {
if (codeReviewVerdicts.get(stepIndex) === "REVISE") {
return { blocked: true, reason: "code-review-revise-outstanding", stepIndex };
}
const stepDisplay = stepIndex + 1;
const codeRequest = `code review requested for Step ${stepDisplay}`;
const planRequest = `plan review requested for Step ${stepDisplay}`;
const codeVerdictPrefix = `code review Step ${stepDisplay}:`;
const planVerdictPrefix = `plan review Step ${stepDisplay}:`;
for (let i = recentActions.length - 1; i >= 0; i--) {
const action = recentActions[i];
if (!action) {
continue;
}
if (action.startsWith(codeRequest) || action.startsWith(planRequest)) {
return { blocked: true, reason: "review-request-without-verdict", stepIndex };
}
if (action.startsWith(`${codeVerdictPrefix} RETHINK`)) {
return { blocked: true, reason: "code-review-rethink-or-unavailable-outstanding", stepIndex };
}
if (action.startsWith(`${codeVerdictPrefix} UNAVAILABLE`)
&& action.includes("blocking until reviewer returns a usable verdict")) {
return { blocked: true, reason: "code-review-unavailable-blocking", stepIndex };
}
if (action.startsWith(codeVerdictPrefix) || action.startsWith(planVerdictPrefix)) {
break;
}
}
}
return { blocked: false };
}
function formatTaskDoneRefusal(refusalClass: TaskDoneRefusalClass, reason: string): string {
return `fn_task_done refused (${refusalClass}): ${reason}. ${TASK_DONE_REFUSAL_SUFFIX}`;
}
@@ -4157,6 +4228,7 @@ export class TaskExecutor {
let taskDoneSessionRetries = 0;
let retryAbortedDueToReclaim = false;
let refusalHandled = false;
let pendingReviewParked = false;
while (!taskDone && taskDoneSessionRetries < MAX_TASK_DONE_SESSION_RETRIES) {
const liveTask = await this.store.getTask(task.id);
const hasExplicitWorktreeBinding = typeof liveTask.worktree === "string" || liveTask.worktree === null;
@@ -4176,6 +4248,31 @@ export class TaskExecutor {
break;
}
const pendingReviewBlock = detectPendingReviewBlock(liveTask, codeReviewVerdicts);
if (pendingReviewBlock.blocked) {
executorLog.log(
`[executor] ${task.id}: fn_task_done not called but task is blocked on pending review (${pendingReviewBlock.reason}) — skipping retry session`,
);
await this.store.logEntry(
task.id,
`Agent finished without calling fn_task_done but Step ${pendingReviewBlock.stepIndex + 1} is blocked on pending review (${pendingReviewBlock.reason}) — skipping retry session`,
undefined,
this.getRunContextFor(task.id),
);
this.deleteActiveSession(task.id);
this.tokenUsageBaselines.delete(task.id);
session.dispose();
await this.persistTokenUsage(task.id);
await this.store.updateTask(task.id, {
status: "failed",
error: "executor-exit-while-review-pending",
});
await this.handoffTaskToReview(task, "executor-exit-while-review-pending");
this.options.onError?.(task, new Error("executor-exit-while-review-pending"));
pendingReviewParked = true;
break;
}
taskDoneSessionRetries++;
executorLog.log(
`⚠ ${task.id} finished without fn_task_done — retrying with new session (${taskDoneSessionRetries}/${MAX_TASK_DONE_SESSION_RETRIES})`,
@@ -4405,6 +4502,8 @@ export class TaskExecutor {
executorLog.log(silentMessage);
} else if (refusalHandled) {
return;
} else if (pendingReviewParked) {
return;
} else {
// FN-4806: Genuine "agent finished without calling fn_task_done after N retries"
// exhaustion. Not a reclaim/self-heal — the agent had a fair chance and failed to