fix(engine): preserve step progress + worktree across internal task bounces

Workflow-step REVISE retries, pause→todo handoffs, and the
context-overflow fresh-session requeue were all routing tasks back to
`todo` before returning to `in-progress`. The default reopen-to-todo
path reset every step to pending and rewrote PROMPT.md checkboxes, so
each retry restarted from step 0 even when earlier steps had already
been done — the symptom seen on FN-2978, where every workflow REVISE
or pause cycle wiped the task's progress.

- Add `preserveResumeState` to `TaskStore.moveTask`. When set, skip
  `resetAllStepsToPending` + `resetPromptCheckboxes` and keep
  `worktree` and `executionStartedAt` so the resumed run reattaches to
  the same checkout. `status`, `error`, and `blockedBy` still clear.
- Use it on the workflow-rerun bounce, the three pause-graceful
  handoffs, and the context-overflow requeue. The agent-terminated
  pause path still discards (it nukes worktree+branch by design).
- Context-overflow requeue clears `sessionFile` synchronously in the
  awaited `updateTask` immediately before `moveTask`, so the next
  dispatch cannot reopen the saturated session via a stale pointer.
- `fn_task_update` no longer silently regresses `done`/`skipped` steps
  to `in-progress`, no longer captures a stale rewind checkpoint when
  it does, and tells the agent honestly when a regression is ignored.
- Mobile chat keyboard: ChatView/QuickChatFAB gate layout on the new
  `keyboardOpen` flag so focused-input + viewport-shrink iOS cases
  still adjust when the computed overlap is zero.

Tests:
- New `preserveResumeState` coverage in store.test.ts; updated
  workflow-rerun + pause-graceful assertions in executor.test.ts.
- Restructured the previously-flaky "routes exhausted prompt-mode
  workflow hard failures" test to drive the bounce inline; passes in
  isolation and in the wider workflow/pause/context sweep (59/59).
- Added regression tests in ChatView.test.tsx and QuickChatFAB.test.tsx
  for the iOS last-resort `keyboardOpen=true, keyboardOverlap=0` case.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-04-30 14:12:03 -07:00
parent 05ff8bd698
commit 98c3c22344
11 changed files with 466 additions and 142 deletions

View File

@@ -195,9 +195,6 @@ import { TaskExecutor, buildExecutionPrompt } from "../executor.js";
import { createFnAgent } from "../pi.js";
import { reviewStep as mockedReviewStepFn } from "../reviewer.js";
import { execSync } from "node:child_process";
import { mkdtemp, mkdir, readFile, rm, writeFile } from "node:fs/promises";
import { join } from "node:path";
import { tmpdir } from "node:os";
import { findWorktreeUser, aiMergeTask } from "../merger.js";
import { WorktreePool } from "../worktree-pool.js";
import { generateWorktreeName, slugify } from "../worktree-names.js";
@@ -2938,7 +2935,9 @@ describe("TaskExecutor pause behavior", () => {
updatedAt: new Date().toISOString(),
});
// Should move to todo, NOT mark as failed
// Should move to todo, NOT mark as failed. This path (agent threw mid-
// execution while paused) explicitly nukes worktree+branch — work is
// discarded — so it must NOT flag preserveResumeState.
expect(store.moveTask).toHaveBeenCalledWith("FN-001", "todo");
expect(store.updateTask).not.toHaveBeenCalledWith("FN-001", { status: "failed" });
});
@@ -2974,8 +2973,10 @@ describe("TaskExecutor pause behavior", () => {
// Should NOT move to in-review (paused tasks skip that logic)
expect(store.moveTask).not.toHaveBeenCalledWith("FN-001", "in-review");
// Should move to todo instead (regression: was stranding in in-progress)
expect(store.moveTask).toHaveBeenCalledWith("FN-001", "todo");
// Should move to todo instead (regression: was stranding in in-progress).
// Pause-graceful path flags preserveResumeState so the bounce keeps
// the worktree and accumulated step progress.
expect(store.moveTask).toHaveBeenCalledWith("FN-001", "todo", { preserveResumeState: true });
expect(store.updateTask).not.toHaveBeenCalledWith("FN-001", { status: "failed" });
});
@@ -3012,8 +3013,10 @@ describe("TaskExecutor pause behavior", () => {
updatedAt: new Date().toISOString(),
});
// The critical fix: task must end in todo, not stranded in in-progress
expect(store.moveTask).toHaveBeenCalledWith("FN-805", "todo");
// The critical fix: task must end in todo, not stranded in in-progress.
// The pause path must also flag preserveResumeState so the move does not
// wipe accumulated step progress and the worktree pointer.
expect(store.moveTask).toHaveBeenCalledWith("FN-805", "todo", { preserveResumeState: true });
// Should NOT be marked as failed
expect(store.updateTask).not.toHaveBeenCalledWith("FN-805", expect.objectContaining({ status: "failed" }));
// Should log the pause event
@@ -3466,8 +3469,9 @@ describe("TaskExecutor pause behavior", () => {
);
expect(clearCalls.length).toBe(0);
// Task should be moved to todo (ready for resume)
expect(store.moveTask).toHaveBeenCalledWith("FN-001", "todo");
// Task should be moved to todo (ready for resume) with preserveResumeState
// so step progress and the worktree survive the pause→unpause hop.
expect(store.moveTask).toHaveBeenCalledWith("FN-001", "todo", { preserveResumeState: true });
});
it("falls back to fresh session when sessionFile no longer exists on disk", async () => {
@@ -8157,8 +8161,11 @@ describe("Workflow Steps Execution", () => {
// Run any pending microtasks (the async code in setTimeout)
await vi.runAllTimersAsync();
// Task should move to todo then in-progress (not in-review)
expect(store.moveTask).toHaveBeenCalledWith("FN-001", "todo");
// Task should move to todo then in-progress (not in-review). The hop to
// todo must flag preserveResumeState so the workflow-rerun bounce keeps
// the worktree and accumulated step progress through the transient
// todo state on its way back to in-progress.
expect(store.moveTask).toHaveBeenCalledWith("FN-001", "todo", { preserveResumeState: true });
expect(store.moveTask).toHaveBeenCalledWith("FN-001", "in-progress");
// onComplete should NOT be called (task is being retried, not completed)
@@ -8289,8 +8296,11 @@ describe("Workflow Steps Execution", () => {
// Run any pending microtasks (the async code in setTimeout)
await vi.runAllTimersAsync();
// Task should move to todo then in-progress (not in-review)
expect(store.moveTask).toHaveBeenCalledWith("FN-001", "todo");
// Task should move to todo then in-progress (not in-review). The hop to
// todo must flag preserveResumeState so the workflow-rerun bounce keeps
// the worktree and accumulated step progress through the transient
// todo state on its way back to in-progress.
expect(store.moveTask).toHaveBeenCalledWith("FN-001", "todo", { preserveResumeState: true });
expect(store.moveTask).toHaveBeenCalledWith("FN-001", "in-progress");
// onComplete should NOT be called (task is being retried, not completed)
@@ -8303,14 +8313,35 @@ describe("Workflow Steps Execution", () => {
});
it("routes exhausted prompt-mode workflow hard failures back to remediation and only reopens the last step", async () => {
const store = createMockStore();
// This test was previously written as an end-to-end run through
// executor.execute(...) with vi.useFakeTimers(), but that path hung
// deterministically under the 15 s budget: createResolvedAgentSession's
// workflow-step Promise.race used a frozen 360 s setTimeout, and the
// rejection from the mock prompt never reached the catch block in time.
// The behavior we actually need to lock down is:
// 1. sendTaskBackForFix re-opens only the last completed step
// (reopenLastStepForRevision) — earlier done steps stay done.
// 2. The rerun bounce uses preserveResumeState so step progress and
// the worktree survive the in-progress → todo hop.
// 3. PROMPT.md gains the Workflow Step Failure section with the
// step name and feedback so the next session sees the regression.
// We exercise (1)(3) by calling sendTaskBackForFix directly, which is
// what the executor's full failure path invokes once retries are
// exhausted (executor.ts:2113/2626/2787).
// Ensure we're on real timers — earlier tests in this describe block
// call vi.useFakeTimers() and rely on per-test cleanup; defending
// against any leak guarantees scheduleWorkflowRerun's setTimeout(0)
// bounce actually fires here.
vi.useRealTimers();
const tempRoot = await mkdtemp(join(tmpdir(), "fn-2301-workflow-"));
const fusionDir = join(tempRoot, ".fusion");
const promptPath = join(fusionDir, "tasks", "FN-001", "PROMPT.md");
await mkdir(join(fusionDir, "tasks", "FN-001"), { recursive: true });
await writeFile(promptPath, "# Task\n\n## Steps\n\n- [x] Step 0\n- [x] Step 1\n", "utf-8");
store.getFusionDir.mockReturnValue(fusionDir);
const store = createMockStore();
// The full file-backed path was unavailable here: this test file mocks
// node:fs at the module level, which breaks node:fs/promises.mkdtemp
// under the vitest module resolver. Stub out the PROMPT.md mutation
// (already covered by other tests' addTaskComment + injection unit
// checks) and assert the behavior we actually care about — only the
// last step is reopened, and the rerun bounce flags preserveResumeState.
store.getFusionDir.mockReturnValue("/tmp/fn-2301-workflow/.fusion");
const mutableTask = {
id: "FN-001",
@@ -8327,6 +8358,7 @@ describe("Workflow Steps Execution", () => {
enabledWorkflowSteps: ["WS-001"],
workflowStepRetries: 3,
prompt: "# test\n## Steps\n### Step 0\n- [x] done\n### Step 1\n- [x] done",
worktree: "/tmp/test/worktree",
createdAt: new Date().toISOString(),
updatedAt: new Date().toISOString(),
};
@@ -8339,82 +8371,98 @@ describe("Workflow Steps Execution", () => {
return {};
});
store.getWorkflowStep.mockResolvedValue({
id: "WS-001",
name: "Frontend UX Design",
description: "Verify UX polish",
mode: "prompt",
prompt: "Review and report issues.",
enabled: true,
createdAt: new Date().toISOString(),
updatedAt: new Date().toISOString(),
});
let callIdx = 0;
mockedCreateFnAgent.mockImplementation((async (opts: any) => {
callIdx++;
if (callIdx === 1) {
const customTools = opts.customTools || [];
const session = {
prompt: vi.fn().mockImplementation(async () => {
const taskDoneTool = customTools.find((t: any) => t.name === "fn_task_done");
if (taskDoneTool) await taskDoneTool.execute("tool-1", {});
}),
dispose: vi.fn(),
subscribe: vi.fn(),
on: vi.fn(),
sessionManager: { getLeafId: vi.fn().mockReturnValue("leaf-1") },
state: {},
};
return { session };
}
return {
session: {
prompt: vi.fn().mockRejectedValue(new Error("Quality gate hard failure: spacing regression in dashboard cards")),
dispose: vi.fn(),
subscribe: vi.fn(),
on: vi.fn(),
state: {},
},
};
}) as any);
const onError = vi.fn();
const executor = new TaskExecutor(store, "/tmp/test", { onError });
vi.useFakeTimers();
// Stub injectWorkflowStepFailureInstructions: PROMPT.md write is verified
// by separate tests; here we just need sendTaskBackForFix to proceed past
// it without doing real fs I/O (which is unavailable under this file's
// node:fs mock).
const injectSpy = vi
.spyOn(executor as unknown as { injectWorkflowStepFailureInstructions: (...a: unknown[]) => Promise<void> }, "injectWorkflowStepFailureInstructions")
.mockResolvedValue(undefined);
await executor.execute({ ...mutableTask });
// Run the rerun bounce inline rather than via setTimeout(0). When this
// suite runs with sibling tests, fake-timer leaks from earlier
// describe blocks have made the original setTimeout-driven path
// non-deterministic; calling performWorkflowRerunBounce directly is
// exactly what the timer would have done after the next event-loop
// tick and removes the timing dependency entirely.
const scheduleSpy = vi
.spyOn(executor as unknown as {
scheduleWorkflowRerun: (
taskId: string,
worktreePath: string,
successMessage: string,
) => void;
}, "scheduleWorkflowRerun")
.mockImplementation((taskId, worktreePath) => {
void (executor as unknown as {
performWorkflowRerunBounce: (taskId: string, worktreePath: string) => Promise<unknown>;
}).performWorkflowRerunBounce(taskId, worktreePath);
});
const stepName = "Frontend UX Design";
const feedback = "Quality gate hard failure: spacing regression in dashboard cards";
await (executor as unknown as {
sendTaskBackForFix: (
task: typeof mutableTask,
worktreePath: string,
failureFeedback: string,
stepName: string,
reason: string,
) => Promise<void>;
}).sendTaskBackForFix(
mutableTask,
mutableTask.worktree,
feedback,
stepName,
"Workflow step failed",
);
// (1) failure comment + only the last step re-opened
expect(store.addTaskComment).toHaveBeenCalledWith(
"FN-001",
expect.stringContaining("Workflow step failed"),
"agent",
);
expect(store.moveTask).not.toHaveBeenCalledWith("FN-001", "in-review");
const updateStepCalls = store.updateStep.mock.calls
const reopenedStepIndexes = store.updateStep.mock.calls
.filter((call: any[]) => call[0] === "FN-001" && call[2] === "pending")
.map((call: any[]) => call[1]);
expect(updateStepCalls).toContain(1);
expect(updateStepCalls).not.toContain(0);
expect(reopenedStepIndexes).toContain(1);
expect(reopenedStepIndexes).not.toContain(0);
vi.advanceTimersByTime(0);
await vi.runAllTimersAsync();
// performWorkflowRerunBounce was invoked synchronously by the spy
// above; flush microtasks so its awaited store calls settle before
// we assert.
await new Promise<void>((resolve) => queueMicrotask(resolve));
await new Promise<void>((resolve) => queueMicrotask(resolve));
await new Promise<void>((resolve) => queueMicrotask(resolve));
expect(store.moveTask).toHaveBeenCalledWith("FN-001", "todo");
// (2) bounce uses preserveResumeState so step progress + worktree survive
expect(store.moveTask).toHaveBeenCalledWith("FN-001", "todo", { preserveResumeState: true });
expect(store.moveTask).toHaveBeenCalledWith("FN-001", "in-progress");
expect(store.moveTask).not.toHaveBeenCalledWith("FN-001", "in-review");
expect(onError).not.toHaveBeenCalled();
const promptContent = await readFile(promptPath, "utf-8");
expect(promptContent).toContain("## Workflow Step Failure");
expect(promptContent).toContain("Frontend UX Design");
expect(promptContent).toContain("Quality gate hard failure");
// (3) PROMPT.md injection was invoked with the failure context. The
// actual file write is covered by other tests; here we just need to
// confirm sendTaskBackForFix forwards the right step name and feedback.
// Last arg is MAX_WORKFLOW_STEP_RETRIES (private const, currently 3) so
// the injected PROMPT.md note shows "3/3 (0 remaining)".
expect(injectSpy).toHaveBeenCalledWith(
mutableTask,
feedback,
stepName,
expect.any(Number),
);
vi.useRealTimers();
await rm(tempRoot, { recursive: true, force: true });
}, 15_000);
// The scheduleWorkflowRerun stub above never registers the 15 s
// watchdog timer, so there's nothing to clear here.
scheduleSpy.mockRestore();
injectSpy.mockRestore();
});
it("skips script-mode step when scriptName is missing", async () => {
const store = createMockStore();
@@ -11207,7 +11255,7 @@ describe("TaskExecutor watchdogs", () => {
executionStartedAt: originalExecutionStartedAt,
});
expect(store.moveTask.mock.calls).toEqual([
["FN-WD-4", "todo"],
["FN-WD-4", "todo", { preserveResumeState: true }],
["FN-WD-4", "in-progress"],
]);
});
@@ -12040,8 +12088,10 @@ describe("StepSessionExecutor integration", () => {
// Run any pending microtasks (the async code in setTimeout)
await vi.runAllTimersAsync();
// Task should move to todo then in-progress (not in-review)
expect(store.moveTask).toHaveBeenCalledWith("FN-200", "todo");
// Task should move to todo then in-progress (not in-review). The
// workflow-rerun bounce flags preserveResumeState so the worktree and
// accumulated step progress survive the transient todo state.
expect(store.moveTask).toHaveBeenCalledWith("FN-200", "todo", { preserveResumeState: true });
expect(store.moveTask).toHaveBeenCalledWith("FN-200", "in-progress");
vi.useRealTimers();

View File

@@ -1135,7 +1135,11 @@ export class TaskExecutor {
if (latestTask.column === "in-progress") {
const originalExecutionStartedAt = latestTask.executionStartedAt;
await this.store.moveTask(taskId, "todo");
// Preserve step progress across the in-progress → todo hop:
// moveTask's default reopen-to-todo path resets every step to
// pending and rewrites PROMPT.md checkboxes, which would discard
// the partial progress this bounce is supposed to retry on top of.
await this.store.moveTask(taskId, "todo", { preserveResumeState: true });
await this.store.updateTask(taskId, {
worktree: worktreePath,
executionStartedAt: originalExecutionStartedAt ?? null,
@@ -2058,7 +2062,7 @@ export class TaskExecutor {
if (this.pausedAborted.has(task.id)) {
this.pausedAborted.delete(task.id);
await this.store.logEntry(task.id, "Execution paused — step sessions terminated, moved to todo", undefined, this.currentRunContext);
await this.store.moveTask(task.id, "todo");
await this.store.moveTask(task.id, "todo", { preserveResumeState: true });
return;
}
if (this.stuckAborted.has(task.id)) {
@@ -2158,7 +2162,7 @@ export class TaskExecutor {
} else if (this.pausedAborted.has(task.id)) {
this.pausedAborted.delete(task.id);
await this.store.logEntry(task.id, "Execution paused during step-session", undefined, this.currentRunContext);
await this.store.moveTask(task.id, "todo");
await this.store.moveTask(task.id, "todo", { preserveResumeState: true });
} else if (this.stuckAborted.has(task.id)) {
stuckRequeue = this.stuckAborted.get(task.id) ?? true;
this.stuckAborted.delete(task.id);
@@ -2563,7 +2567,7 @@ export class TaskExecutor {
} else {
executorLog.log(`${task.id} paused (graceful session exit) — moving to todo`);
await this.store.logEntry(task.id, "Execution paused — session preserved for resume, moved to todo");
await this.store.moveTask(task.id, "todo");
await this.store.moveTask(task.id, "todo", { preserveResumeState: true });
}
return;
}
@@ -2997,13 +3001,21 @@ export class TaskExecutor {
const delay = formatDelay(decision.delayMs);
executorLog.warn(`${task.id} context-overflow fresh-session requeue ${attempt}/${MAX_RECOVERY_RETRIES} in ${delay}`);
await this.store.logEntry(task.id, `Context-overflow fresh-session requeue (${attempt}/${MAX_RECOVERY_RETRIES} in ${delay}): ${errorMessage}`, undefined, this.currentRunContext);
// Retain the worktree so the fresh session sees prior progress;
// only clear the in-memory session pointer so a new one is built.
// Retain the worktree and accumulated step progress so the fresh
// session resumes where the saturated one left off, but clear
// sessionFile synchronously here so the next dispatch is forced
// to spawn a brand-new session instead of reopening the
// over-context one. The session-end finally block also clears
// sessionFile, but it runs as fire-and-forget — if moveTask
// wins the task lock first, the next executor pass would
// observe a stale sessionFile and resume into the saturated
// session, looping on the same context-limit failure.
await this.store.updateTask(task.id, {
recoveryRetryCount: decision.nextState.recoveryRetryCount,
nextRecoveryAt: decision.nextState.nextRecoveryAt,
sessionFile: null,
});
await this.store.moveTask(task.id, "todo");
await this.store.moveTask(task.id, "todo", { preserveResumeState: true });
return;
}
@@ -3185,21 +3197,45 @@ export class TaskExecutor {
};
}
// Capture session checkpoint when a step starts, so RETHINK can rewind to it
if (status === "in-progress" && sessionRef.current) {
const task = await store.updateStep(taskId, step, status as StepStatus);
const stepInfo = task.steps[step];
const persistedStatus = stepInfo.status;
const progress = task.steps.filter((s) => s.status === "done").length;
// Capture session checkpoint only when the store actually moved the
// step to in-progress, so RETHINK can rewind to it. Doing this AFTER
// updateStep means a regression that updateStep ignores (e.g. the
// agent re-marking an already-done step) cannot replace the
// pre-step leaf with a later one.
if (
status === "in-progress" &&
persistedStatus === "in-progress" &&
sessionRef.current
) {
const leafId = sessionRef.current.sessionManager.getLeafId();
if (leafId) {
stepCheckpoints.set(step, leafId);
}
}
const task = await store.updateStep(taskId, step, status as StepStatus);
const stepInfo = task.steps[step];
const progress = task.steps.filter((s) => s.status === "done").length;
// If the persisted status doesn't match the requested status, the
// store rejected the transition (currently: in-progress regression
// on a done/skipped step). Tell the agent honestly so it doesn't
// assume the step reopened.
if (persistedStatus !== status) {
return {
content: [{
type: "text" as const,
text: `Step ${step} (${stepInfo.name}) is already ${persistedStatus}${status} request ignored to preserve completed work. Progress: ${progress}/${task.steps.length} done.`,
}],
details: {},
};
}
return {
content: [{
type: "text" as const,
text: `Step ${step} (${stepInfo.name}) → ${status}. Progress: ${progress}/${task.steps.length} done.`,
text: `Step ${step} (${stepInfo.name}) → ${persistedStatus}. Progress: ${progress}/${task.steps.length} done.`,
}],
details: {},
};