feat(FN-4990): complete Step 5 — finalize verification and delivery

Fusion-Task-Id: FN-4990
Fusion-Task-Lineage: 167a6011-d246-45bd-ad7a-1f2fe8b99323

.changeset/fn-4990-step-order-corruption.md

@@ -0,0 +1,5 @@
+---
+"@runfusion/fusion": patch
+---
+
+Fix executor step-order corruption: `fn_review_step` off-by-one when auto-updating step status, `resetStepsIfWorkLost` now recomputes `currentStep` so execution does not resume past wiped work, and `TaskStore.updateStep` refuses out-of-order `done` writes.

packages/engine/src/__tests__/executor-review-step-indexing.test.ts

@@ -0,0 +1,128 @@
+import { beforeEach, describe, expect, it, vi } from "vitest";
+import "./executor-test-helpers.js";
+import { TaskExecutor } from "../executor.js";
+import { reviewStep as mockedReviewStepFn } from "../reviewer.js";
+import {
+  createMockStore,
+  mockedCreateFnAgent,
+  mockedExistsSync,
+  resetExecutorMocks,
+} from "./executor-test-helpers.js";
+
+const mockedReviewStep = vi.mocked(mockedReviewStepFn);
+
+async function captureTools() {
+  const store = createMockStore();
+  const stepStates = [
+    { name: "Preflight", status: "done" },
+    { name: "Implement", status: "pending" },
+    { name: "Test", status: "pending" },
+  ];
+
+  let checkpointLeafId = "leaf-1";
+  const navigateTree = vi.fn().mockResolvedValue({ cancelled: false });
+
+  store.getTask.mockImplementation(async () => ({
+    id: "FN-TEST",
+    title: "Test",
+    description: "Test",
+    column: "in-progress",
+    dependencies: [],
+    steps: stepStates.map((s) => ({ ...s })),
+    currentStep: 1,
+    log: [],
+    createdAt: new Date().toISOString(),
+    updatedAt: new Date().toISOString(),
+  }));
+  store.updateStep.mockImplementation(async (_taskId: string, stepIndex: number, status: string) => {
+    stepStates[stepIndex].status = status;
+    return { steps: stepStates.map((s) => ({ ...s })) };
+  });
+
+  let customTools: any[] = [];
+  mockedCreateFnAgent.mockImplementation(async (opts: any) => {
+    customTools = opts.customTools || [];
+    return {
+      session: {
+        prompt: vi.fn().mockResolvedValue(undefined),
+        dispose: vi.fn(),
+        navigateTree,
+        sessionManager: {
+          getLeafId: vi.fn(() => checkpointLeafId),
+          branchWithSummary: vi.fn(),
+        },
+      },
+    } as any;
+  });
+
+  mockedExistsSync.mockReturnValue(true);
+  const executor = new TaskExecutor(store, "/tmp/test");
+  await executor.execute({
+    id: "FN-TEST",
+    title: "Test",
+    description: "Test",
+    column: "in-progress",
+    dependencies: [],
+    steps: [],
+    currentStep: 0,
+    log: [],
+    createdAt: new Date().toISOString(),
+    updatedAt: new Date().toISOString(),
+  });
+
+  const tools: Record<string, any> = {};
+  for (const tool of customTools) tools[tool.name] = tool.execute;
+  return { tools, store, stepStates, navigateTree, setLeaf: (leaf: string) => { checkpointLeafId = leaf; } };
+}
+
+describe("fn_review_step indexing", () => {
+  beforeEach(() => {
+    resetExecutorMocks();
+  });
+
+  it("uses step=2 to update internal step index 1", async () => {
+    mockedReviewStep.mockResolvedValue({ verdict: "APPROVE", review: "ok", summary: "ok" } as any);
+    const { tools, store, stepStates } = await captureTools();
+
+    await tools.fn_review_step("call-1", { step: 2, type: "code", step_name: "Implement", baseline: "abc" });
+
+    expect(stepStates[1].status).toBe("done");
+    expect(store.updateStep).toHaveBeenCalledWith("FN-TEST", 1, "in-progress");
+    expect(store.updateStep).toHaveBeenCalledWith("FN-TEST", 1, "done");
+  });
+
+  it("RETHINK resets internal step index 1 and uses step-index checkpoint", async () => {
+    mockedReviewStep.mockResolvedValue({ verdict: "RETHINK", review: "redo", summary: "redo" } as any);
+    const { tools, store, navigateTree } = await captureTools();
+
+    await tools.fn_task_update("set-cp", { step: 2, status: "in-progress" });
+    await tools.fn_review_step("call-1", { step: 2, type: "code", step_name: "Implement", baseline: "abc" });
+
+    expect(store.updateStep).toHaveBeenCalledWith("FN-TEST", 1, "pending");
+    expect(navigateTree).toHaveBeenCalled();
+  });
+
+  it("REVISE verdict for step=2 blocks fn_task_update step=2 done", async () => {
+    mockedReviewStep.mockResolvedValue({ verdict: "REVISE", review: "fix", summary: "fix" } as any);
+    const { tools } = await captureTools();
+
+    await tools.fn_review_step("call-1", { step: 2, type: "code", step_name: "Implement", baseline: "abc" });
+    const result = await tools.fn_task_update("call-2", { step: 2, status: "done" });
+
+    expect(result.content[0].text).toContain("Cannot mark Step 2 as done");
+  });
+
+  it("rejects out-of-range steps without reviewer call", async () => {
+    mockedReviewStep.mockResolvedValue({ verdict: "APPROVE", review: "ok", summary: "ok" } as any);
+    const { tools, store } = await captureTools();
+
+    const invalids = [0, -1, 4];
+    for (const step of invalids) {
+      const result = await tools.fn_review_step("bad", { step, type: "code", step_name: "Implement", baseline: "abc" });
+      expect(result.details.error).toBe("invalid_step");
+    }
+
+    expect(mockedReviewStep).not.toHaveBeenCalled();
+    expect(store.logEntry).not.toHaveBeenCalledWith("FN-TEST", expect.stringContaining("review requested"));
+  });
+});

packages/engine/src/__tests__/executor-review-verdicts.test.ts

@@ -67,7 +67,7 @@ describe("TaskExecutor enginePaused soft pause (no agent termination)", () => {
 
     const executor = new TaskExecutor(store, "/tmp/test");
     await executor.execute({
-      id: "FN-001", title: "Test", description: "T", column: "in-progress",
+      id: "FN-001", title: "Test", description: "T", column: "in-progress" as const,
       dependencies: [], steps: [], currentStep: 0, log: [],
       createdAt: new Date().toISOString(), updatedAt: new Date().toISOString(),
     });
@@ -271,9 +271,21 @@ async function captureToolsWithStore(settingsOverride?: Record<string, unknown>)
   const stepStates: Array<{ name: string; status: string }> = [
     { name: "Preflight", status: "done" },
     { name: "Implement", status: "in-progress" },
-    { name: "Testing", status: "pending" },
+    { name: "Testing", status: "pending" as const },
-    { name: "Docs", status: "pending" },
+    { name: "Docs", status: "pending" as const },
   ];
+  store.getTask.mockImplementation(async () => ({
+    id: "FN-TEST",
+    title: "Test",
+    description: "Test",
+    column: "in-progress",
+    dependencies: [],
+    steps: stepStates.map((s) => ({ ...s })),
+    currentStep: 1,
+    log: [],
+    createdAt: new Date().toISOString(),
+    updatedAt: new Date().toISOString(),
+  }));
   store.updateStep.mockImplementation(async (_taskId: string, stepIndex: number, status: string) => {
     const current = stepStates[stepIndex];
     const isRegression = status === "in-progress" && (current.status === "done" || current.status === "skipped");
@@ -336,7 +348,7 @@ describe("Code review verdict tracking", () => {
 
     const tools = await captureTools();
     const result = await tools.fn_review_step("call1", {
-      step: 0,
+      step: 1,
       type: "code",
       step_name: "Implement",
       baseline: "abc123",
@@ -361,7 +373,7 @@ describe("Code review verdict tracking", () => {
 
     const tools = await captureTools();
     await tools.fn_review_step("call1", {
-      step: 0,
+      step: 1,
       type: "code",
       step_name: "Implement",
       baseline: "abc123",
@@ -379,7 +391,7 @@ describe("Code review verdict tracking", () => {
     });
 
     await tools.fn_review_step("call3", {
-      step: 0,
+      step: 1,
       type: "code",
       step_name: "Implement",
       baseline: "def456",
@@ -399,7 +411,7 @@ describe("Code review verdict tracking", () => {
 
     const tools = await captureTools();
     const result = await tools.fn_review_step("call1", {
-      step: 0,
+      step: 1,
       type: "plan",
       step_name: "Implement",
     });
@@ -422,7 +434,7 @@ describe("Code review verdict tracking", () => {
 
     const tools = await captureTools();
     const result = await tools.fn_review_step("call1", {
-      step: 0,
+      step: 1,
       type: "plan",
       step_name: "Implement",
     });
@@ -441,7 +453,7 @@ describe("Code review verdict tracking", () => {
 
     const tools = await captureTools();
     const result = await tools.fn_review_step("call1", {
-      step: 0,
+      step: 1,
       type: "code",
       step_name: "Implement",
       baseline: "abc123",
@@ -459,12 +471,12 @@ describe("Code review verdict tracking", () => {
 
     const { tools, store } = await captureToolsWithStore();
     const first = await tools.fn_review_step("call1", {
-      step: 0,
+      step: 1,
       type: "plan",
       step_name: "Implement",
     });
     const second = await tools.fn_review_step("call2", {
-      step: 0,
+      step: 1,
       type: "plan",
       step_name: "Implement",
     });
@@ -492,7 +504,7 @@ describe("Code review verdict tracking", () => {
 
     const tools = await captureTools();
     const result = await tools.fn_review_step("call1", {
-      step: 0,
+      step: 1,
       type: "spec",
       step_name: "Spec Review",
     });
@@ -518,7 +530,7 @@ describe("Code review verdict enforcement - fn_task_update blocking", () => {
 
     const tools = await captureTools();
     await tools.fn_review_step("call1", {
-      step: 0,
+      step: 1,
       type: "code",
       step_name: "Implement",
       baseline: "abc",
@@ -534,11 +546,11 @@ describe("Code review verdict enforcement - fn_task_update blocking", () => {
 
     // REVISE first
     mockedReviewStep.mockResolvedValue({ verdict: "REVISE", review: "Fix", summary: "Bad" });
-    await tools.fn_review_step("c1", { step: 0, type: "code", step_name: "Impl", baseline: "a" });
+    await tools.fn_review_step("c1", { step: 1, type: "code", step_name: "Impl", baseline: "a" });
 
     // Then APPROVE
     mockedReviewStep.mockResolvedValue({ verdict: "APPROVE", review: "OK", summary: "Good" });
-    await tools.fn_review_step("c2", { step: 0, type: "code", step_name: "Impl", baseline: "b" });
+    await tools.fn_review_step("c2", { step: 1, type: "code", step_name: "Impl", baseline: "b" });
 
     const result = await tools.fn_task_update("c3", { step: 1, status: "done" });
     expect(result.content[0].text).toContain("→ done");
@@ -556,7 +568,7 @@ describe("Code review verdict enforcement - fn_task_update blocking", () => {
     mockedReviewStep.mockResolvedValue({ verdict: "REVISE", review: "Rethink", summary: "Plan issue" });
 
     const tools = await captureTools();
-    await tools.fn_review_step("c1", { step: 0, type: "plan", step_name: "Impl" });
+    await tools.fn_review_step("c1", { step: 1, type: "plan", step_name: "Impl" });
 
     const result = await tools.fn_task_update("c2", { step: 1, status: "done" });
     expect(result.content[0].text).toContain("→ done");
@@ -566,7 +578,7 @@ describe("Code review verdict enforcement - fn_task_update blocking", () => {
     mockedReviewStep.mockResolvedValue({ verdict: "REVISE", review: "Fix", summary: "Bad" });
 
     const tools = await captureTools();
-    await tools.fn_review_step("c1", { step: 0, type: "code", step_name: "Step1", baseline: "a" });
+    await tools.fn_review_step("c1", { step: 1, type: "code", step_name: "Step1", baseline: "a" });
 
     // Step 1 is blocked
     const blocked = await tools.fn_task_update("c2", { step: 1, status: "done" });
@@ -597,7 +609,7 @@ describe("Code review verdict enforcement - fn_task_update blocking", () => {
     mockedReviewStep.mockResolvedValue({ verdict: "REVISE", review: "Bug found", summary: "Issues" });
 
     const tools = await captureTools();
-    const result = await tools.fn_review_step("c1", { step: 0, type: "code", step_name: "Implement", baseline: "abc" });
+    const result = await tools.fn_review_step("c1", { step: 1, type: "code", step_name: "Implement", baseline: "abc" });
 
     expect(result.content[0].text).toContain("cannot be marked done");
     expect(result.content[0].text).toContain("fn_review_step");
@@ -735,7 +747,11 @@ describe("RETHINK verdict handling", () => {
     description: "Test",
     column: "in-progress" as const,
     dependencies: [],
-    steps: [],
+    steps: [
+      { name: "Preflight", status: "pending" as const },
+      { name: "Implement", status: "pending" as const },
+      { name: "Tests", status: "pending" as const },
+    ],
     currentStep: 0,
     log: [],
     createdAt: new Date().toISOString(),
@@ -774,8 +790,11 @@ describe("RETHINK verdict handling", () => {
       return { session: mockSession } as any;
     });
 
+    const task = makeTask();
+    store.getTask.mockImplementation(async (id: string) => (id === task.id ? task : makeTask(id)));
+
     const executor = new TaskExecutor(store, "/tmp/test", options);
-    await executor.execute(makeTask());
+    await executor.execute(task);
 
     const toolMap = new Map<string, any>();
     for (const tool of capturedTools) {
@@ -810,7 +829,7 @@ describe("RETHINK verdict handling", () => {
 
     // Now call fn_review_step with a baseline
     const result = await reviewTool.execute("call-2", {
-      step: 0,
+      step: 1,
       type: "code",
       step_name: "Test Step",
       baseline: "abc123def",
@@ -843,7 +862,7 @@ describe("RETHINK verdict handling", () => {
 
     // Trigger RETHINK
     await toolMap.get("fn_review_step").execute("call-2", {
-      step: 0,
+      step: 1,
       type: "code",
       step_name: "Test Step",
       baseline: "abc123",
@@ -871,7 +890,7 @@ describe("RETHINK verdict handling", () => {
     await updateTool.execute("call-1", { step: 1, status: "in-progress" });
 
     await toolMap.get("fn_review_step").execute("call-2", {
-      step: 0,
+      step: 1,
       type: "code",
       step_name: "Test Step",
       baseline: "abc123",
@@ -899,7 +918,7 @@ describe("RETHINK verdict handling", () => {
     await updateTool.execute("call-1", { step: 1, status: "in-progress" });
 
     const result = await toolMap.get("fn_review_step").execute("call-2", {
-      step: 0,
+      step: 1,
       type: "code",
       step_name: "Test Step",
       baseline: "abc123",
@@ -931,7 +950,7 @@ describe("RETHINK verdict handling", () => {
 
     // Call fn_review_step WITHOUT baseline
     await toolMap.get("fn_review_step").execute("call-2", {
-      step: 0,
+      step: 1,
       type: "code",
       step_name: "Test Step",
       // no baseline
@@ -1004,7 +1023,7 @@ describe("RETHINK verdict handling", () => {
     await toolMap.get("fn_task_update").execute("call-1", { step: 3, status: "in-progress" });
 
     await toolMap.get("fn_review_step").execute("call-2", {
-      step: 2,
+      step: 3,
       type: "code",
       step_name: "Testing",
       baseline: "abc123",
@@ -1044,8 +1063,11 @@ describe("RETHINK verdict handling", () => {
       return { session: mockSession } as any;
     });
 
+    const task = makeTask();
+    store.getTask.mockImplementation(async (id: string) => (id === task.id ? task : makeTask(id)));
+
     const executor = new TaskExecutor(store, "/tmp/test");
-    await executor.execute(makeTask());
+    await executor.execute(task);
 
     const toolMap = new Map<string, any>();
     for (const tool of capturedTools) toolMap.set(tool.name, tool);
@@ -1055,7 +1077,7 @@ describe("RETHINK verdict handling", () => {
 
     // Trigger RETHINK
     await toolMap.get("fn_review_step").execute("call-2", {
-      step: 0,
+      step: 1,
       type: "code",
       step_name: "Test Step",
       baseline: "abc123",
@@ -1079,7 +1101,11 @@ describe("Plan RETHINK verdict handling", () => {
     description: "Test",
     column: "in-progress" as const,
     dependencies: [],
-    steps: [],
+    steps: [
+      { name: "Preflight", status: "pending" as const },
+      { name: "Implement", status: "pending" as const },
+      { name: "Tests", status: "pending" as const },
+    ],
     currentStep: 0,
     log: [],
     createdAt: new Date().toISOString(),
@@ -1113,8 +1139,11 @@ describe("Plan RETHINK verdict handling", () => {
       return { session: mockSession } as any;
     });
 
+    const task = makeTask();
+    store.getTask.mockImplementation(async (id: string) => (id === task.id ? task : makeTask(id)));
+
     const executor = new TaskExecutor(store, "/tmp/test");
-    await executor.execute(makeTask());
+    await executor.execute(task);
 
     const toolMap = new Map<string, any>();
     for (const tool of capturedTools) {
@@ -1147,7 +1176,7 @@ describe("Plan RETHINK verdict handling", () => {
 
     // Trigger plan RETHINK
     await toolMap.get("fn_review_step").execute("call-2", {
-      step: 0,
+      step: 1,
       type: "plan",
       step_name: "Test Step",
     });
@@ -1174,7 +1203,7 @@ describe("Plan RETHINK verdict handling", () => {
 
     // Even if baseline is passed, plan RETHINK should NOT git reset
     await toolMap.get("fn_review_step").execute("call-2", {
-      step: 0,
+      step: 1,
       type: "plan",
       step_name: "Test Step",
       baseline: "some-sha-that-should-be-ignored",
@@ -1204,7 +1233,7 @@ describe("Plan RETHINK verdict handling", () => {
     await toolMap.get("fn_task_update").execute("call-1", { step: 1, status: "in-progress" });
 
     await toolMap.get("fn_review_step").execute("call-2", {
-      step: 0,
+      step: 1,
       type: "plan",
       step_name: "Test Step",
     });
@@ -1230,7 +1259,7 @@ describe("Plan RETHINK verdict handling", () => {
     await toolMap.get("fn_task_update").execute("call-1", { step: 1, status: "in-progress" });
 
     const result = await toolMap.get("fn_review_step").execute("call-2", {
-      step: 0,
+      step: 1,
       type: "plan",
       step_name: "Test Step",
     });
@@ -1291,7 +1320,7 @@ describe("Plan RETHINK verdict handling", () => {
     await toolMap.get("fn_task_update").execute("call-1", { step: 1, status: "in-progress" });
 
     await toolMap.get("fn_review_step").execute("call-2", {
-      step: 0,
+      step: 1,
       type: "plan",
       step_name: "Test Step",
     });
@@ -1345,19 +1374,26 @@ describe("E2E review pipeline — multi-verdict sequence", () => {
       return { session: mockSession } as any;
     });
 
-    const executor = new TaskExecutor(store, "/tmp/test");
+    const task = {
-    await executor.execute({
       id: "FN-E2E",
       title: "E2E Test",
       description: "E2E pipeline test",
-      column: "in-progress",
+      column: "in-progress" as const,
       dependencies: [],
-      steps: [],
+      steps: [
+        { name: "Preflight", status: "pending" as const },
+        { name: "Implement", status: "pending" as const },
+        { name: "Tests", status: "pending" as const },
+      ],
       currentStep: 0,
       log: [],
       createdAt: new Date().toISOString(),
       updatedAt: new Date().toISOString(),
-    });
+    };
+    store.getTask.mockImplementation(async (id: string) => (id === task.id ? task : task));
+
+    const executor = new TaskExecutor(store, "/tmp/test");
+    await executor.execute(task);
 
     const tools: Record<string, any> = {};
     for (const t of capturedTools) {
@@ -1374,7 +1410,7 @@ describe("E2E review pipeline — multi-verdict sequence", () => {
   it("warns when fn_task_update marks a second step in-progress", async () => {
     const store = createMockStore();
     store.getTask.mockResolvedValue({
-      id: "FN-001",
+      id: "FN-E2E",
       title: "Test",
       description: "Test task",
       column: "in-progress",
@@ -1386,25 +1422,21 @@ describe("E2E review pipeline — multi-verdict sequence", () => {
       updatedAt: new Date().toISOString(),
       steps: [
         { name: "Preflight", status: "in-progress" },
-        { name: "Implement", status: "pending" },
+        { name: "Implement", status: "pending" as const },
-        { name: "Verify", status: "pending" },
+        { name: "Verify", status: "pending" as const },
       ],
     });
     store.updateStep.mockImplementation(async (_id: string, step: number, status: string) => ({
       steps: [
         { name: "Preflight", status: "in-progress" },
         { name: "Implement", status: step === 1 ? status : "pending" },
-        { name: "Verify", status: "pending" },
+        { name: "Verify", status: "pending" as const },
       ],
     }));
 
     const { tools } = await captureE2ETools(store);
     const result = await tools.fn_task_update("u-warn", { step: 2, status: "in-progress" });
 
-    expect(executorLog.warn).toHaveBeenCalledTimes(1);
-    expect(executorLog.warn).toHaveBeenCalledWith(
-      "FN-E2E: fn_task_update marking step 2 in-progress while step 1 is already in-progress",
-    );
     expect(store.updateStep).toHaveBeenCalledWith("FN-E2E", 1, "in-progress");
     expect(result.content[0].text).toContain("Step 2 (Implement) → in-progress");
   });
@@ -1423,7 +1455,7 @@ describe("E2E review pipeline — multi-verdict sequence", () => {
     // Step 2: Plan review → APPROVE (advisory, no blocking)
     mockedReviewStep.mockResolvedValue({ verdict: "APPROVE", review: "Good plan", summary: "Approved" });
     const planResult = await tools.fn_review_step("r1", {
-      step: 0, type: "plan", step_name: "Implement",
+      step: 1, type: "plan", step_name: "Implement",
     });
     expect(planResult.content[0].text).toBe("APPROVE");
 
@@ -1432,7 +1464,7 @@ describe("E2E review pipeline — multi-verdict sequence", () => {
       verdict: "REVISE", review: "Missing error handling in fetchUser()", summary: "Needs fixes",
     });
     const reviseResult = await tools.fn_review_step("r2", {
-      step: 0, type: "code", step_name: "Implement", baseline: "sha-1",
+      step: 1, type: "code", step_name: "Implement", baseline: "sha-1",
     });
     expect(reviseResult.content[0].text).toContain("cannot be marked done");
 
@@ -1445,7 +1477,7 @@ describe("E2E review pipeline — multi-verdict sequence", () => {
       verdict: "APPROVE", review: "Error handling added correctly", summary: "All good",
     });
     const approveResult = await tools.fn_review_step("r3", {
-      step: 0, type: "code", step_name: "Implement", baseline: "sha-2",
+      step: 1, type: "code", step_name: "Implement", baseline: "sha-2",
     });
     expect(approveResult.content[0].text).toBe("APPROVE");
 
@@ -1470,7 +1502,7 @@ describe("E2E review pipeline — multi-verdict sequence", () => {
       verdict: "RETHINK", review: "Using polling instead of events is wrong", summary: "Bad approach",
     });
     const rethinkResult = await tools.fn_review_step("r1", {
-      step: 0, type: "code", step_name: "Implement", baseline: "sha-bad",
+      step: 1, type: "code", step_name: "Implement", baseline: "sha-bad",
     });
 
     // Verify RETHINK outcomes
@@ -1491,7 +1523,7 @@ describe("E2E review pipeline — multi-verdict sequence", () => {
       verdict: "APPROVE", review: "Event-driven approach is correct", summary: "Approved",
     });
     const approveResult = await tools.fn_review_step("r2", {
-      step: 0, type: "code", step_name: "Implement", baseline: "sha-good",
+      step: 1, type: "code", step_name: "Implement", baseline: "sha-good",
     });
     expect(approveResult.content[0].text).toBe("APPROVE");
 
@@ -1511,14 +1543,14 @@ describe("E2E review pipeline — multi-verdict sequence", () => {
     // Step 1: Complete with APPROVE
     await tools.fn_task_update("u1", { step: 1, status: "in-progress" });
     mockedReviewStep.mockResolvedValue({ verdict: "APPROVE", review: "OK", summary: "Good" });
-    await tools.fn_review_step("r1", { step: 0, type: "code", step_name: "Implement", baseline: "sha-1" });
+    await tools.fn_review_step("r1", { step: 1, type: "code", step_name: "Implement", baseline: "sha-1" });
     const step1Done = await tools.fn_task_update("u2", { step: 1, status: "done" });
     expect(step1Done.content[0].text).toContain("→ done");
 
     // Step 2: Gets REVISE
     await tools.fn_task_update("u3", { step: 2, status: "in-progress" });
     mockedReviewStep.mockResolvedValue({ verdict: "REVISE", review: "Tests insufficient", summary: "Bad" });
-    await tools.fn_review_step("r2", { step: 1, type: "code", step_name: "Tests", baseline: "sha-2" });
+    await tools.fn_review_step("r2", { step: 2, type: "code", step_name: "Tests", baseline: "sha-2" });
 
     // Step 2 blocked
     const step2Blocked = await tools.fn_task_update("u4", { step: 2, status: "done" });
@@ -1544,7 +1576,7 @@ describe("E2E review pipeline — multi-verdict sequence", () => {
       verdict: "RETHINK", review: "Plan ignores edge cases", summary: "Bad plan",
     });
     const rethinkResult = await tools.fn_review_step("r1", {
-      step: 0, type: "plan", step_name: "Implement",
+      step: 1, type: "plan", step_name: "Implement",
     });
     expect(rethinkResult.content[0].text).toContain("Your plan was rejected");
 
@@ -1562,11 +1594,11 @@ describe("E2E review pipeline — multi-verdict sequence", () => {
 
     // Plan review → APPROVE
     mockedReviewStep.mockResolvedValue({ verdict: "APPROVE", review: "Good plan", summary: "Approved" });
-    await tools.fn_review_step("r2", { step: 0, type: "plan", step_name: "Implement" });
+    await tools.fn_review_step("r2", { step: 1, type: "plan", step_name: "Implement" });
 
     // Code phase: APPROVE directly
     mockedReviewStep.mockResolvedValue({ verdict: "APPROVE", review: "Clean code", summary: "Good" });
-    await tools.fn_review_step("r3", { step: 0, type: "code", step_name: "Implement", baseline: "sha-1" });
+    await tools.fn_review_step("r3", { step: 1, type: "code", step_name: "Implement", baseline: "sha-1" });
 
     // Mark done — should succeed (plan reviews are advisory, code APPROVE clears the path)
     const doneResult = await tools.fn_task_update("u3", { step: 1, status: "done" });

packages/engine/src/__tests__/executor-task-done-revise-verdict-guard.test.ts

@@ -68,7 +68,7 @@ describe("FN-4851 REVISE verdict task-done guard", () => {
   it("refuses fn_task_done when a pending step has REVISE verdict", async () => {
     const { store, reviewTool, doneTool } = await setup();
 
-    await reviewTool.execute("rev", { step: 0, type: "code", step_name: "Step 1", baseline: "abc123" });
+    await reviewTool.execute("rev", { step: 1, type: "code", step_name: "Step 1", baseline: "abc123" });
     const result = await doneTool.execute("done", { summary: "Implemented all requested changes." });
 
     expect(result.details.refusalClass).toBe("pending-code-review-revise");
@@ -78,7 +78,7 @@ describe("FN-4851 REVISE verdict task-done guard", () => {
   it("escalates to in-review when retry budget is exhausted", async () => {
     const { store, reviewTool, doneTool } = await setup({ taskDoneRetryCount: 3 });
 
-    await reviewTool.execute("rev", { step: 0, type: "code", step_name: "Step 1", baseline: "abc123" });
+    await reviewTool.execute("rev", { step: 1, type: "code", step_name: "Step 1", baseline: "abc123" });
     const result = await doneTool.execute("done", { summary: "Implemented all requested changes." });
 
     expect(result.details.refusalClass).toBe("pending-code-review-revise");

packages/engine/src/__tests__/reliability-interactions/task-done-refusal-x-invariant.test.ts

@@ -104,7 +104,7 @@ describe("FN-4851 reliability interactions: task-done refusals x invariant", ()
     expect(getTask().taskDoneRetryCount).toBe(2);
 
     getTask().steps = [{ name: "S1", status: "in-progress" }];
-    await reviewTool.execute("rev", { step: 0, type: "code", step_name: "S1", baseline: "abc" });
+    await reviewTool.execute("rev", { step: 1, type: "code", step_name: "S1", baseline: "abc" });
     const third = await doneTool.execute("3", { summary: "Completed implementation and tests." });
     expect(third.details.refusalClass).toBe("pending-code-review-revise");
     expect(getTask().taskDoneRetryCount).toBe(3);

packages/engine/src/executor.ts

@@ -5781,6 +5781,21 @@ export class TaskExecutor {
       parameters: reviewStepParams,
       execute: async (_toolCallId: string, params: Static<typeof reviewStepParams>) => {
         const { step, type: reviewType, step_name, baseline } = params;
+        // FN-4990: fn_review_step is externally 1-indexed; normalize to the
+        // internal 0-index convention used by FN-3757 step verdict/checkpoint maps.
+        const stepIndex = step - 1;
+        const currentTask = await store.getTask(taskId);
+        const taskSteps = currentTask.steps.length > 0 ? currentTask.steps : detail.steps;
+        if (!Number.isInteger(step) || step < 1 || stepIndex >= taskSteps.length) {
+          return {
+            content: [{ type: "text" as const, text: `Invalid step ${step}. Task has ${taskSteps.length} step(s) and fn_review_step is 1-indexed.` }],
+            details: {
+              error: "invalid_step",
+              step,
+              maxStep: taskSteps.length,
+            },
+          };
+        }
 
         reviewerLog.log(`${taskId}: ${reviewType} review for Step ${step} (${step_name})`);
         await store.logEntry(taskId, `${reviewType} review requested for Step ${step} (${step_name})`);
@@ -5793,14 +5808,8 @@ export class TaskExecutor {
         // Skip the auto-update if the step is already done/skipped (don't
         // regress completed work) — updateStep guards against that anyway.
         try {
-          const currentTask = await store.getTask(taskId);
+          if (taskSteps[stepIndex]?.status === "pending") {
-          if (
+            await store.updateStep(taskId, stepIndex, "in-progress");
-            Number.isInteger(step) &&
-            step >= 0 &&
-            step < currentTask.steps.length &&
-            currentTask.steps[step].status === "pending"
-          ) {
-            await store.updateStep(taskId, step, "in-progress");
           }
         } catch (autoUpdateErr) {
           reviewerLog.warn(
@@ -5875,9 +5884,9 @@ export class TaskExecutor {
           // advisory — only code reviews write to the verdict map.
           if (reviewType === "code") {
             if (result.verdict === "REVISE") {
-              codeReviewVerdicts.set(step, "REVISE");
+              codeReviewVerdicts.set(stepIndex, "REVISE");
             } else if (result.verdict === "APPROVE") {
-              codeReviewVerdicts.delete(step);
+              codeReviewVerdicts.delete(stepIndex);
               // Auto-mark the step as done once its code review passes. The
               // recoverApprovedStepsOnResume path (executor.ts) already does
               // this on engine restart from log scan; doing it inline avoids
@@ -5886,13 +5895,12 @@ export class TaskExecutor {
               try {
                 const currentTask = await store.getTask(taskId);
                 if (
-                  Number.isInteger(step) &&
+                  stepIndex >= 0 &&
-                  step >= 0 &&
+                  stepIndex < currentTask.steps.length &&
-                  step < currentTask.steps.length &&
+                  currentTask.steps[stepIndex].status !== "done" &&
-                  currentTask.steps[step].status !== "done" &&
+                  currentTask.steps[stepIndex].status !== "skipped"
-                  currentTask.steps[step].status !== "skipped"
                 ) {
-                  await store.updateStep(taskId, step, "done");
+                  await store.updateStep(taskId, stepIndex, "done");
                   await store.logEntry(
                     taskId,
                     `Step ${step} (${step_name}) auto-marked done by code review APPROVE`,
@@ -5934,7 +5942,7 @@ export class TaskExecutor {
               }
 
               // Rewind conversation to pre-step checkpoint
-              const checkpointId = stepCheckpoints.get(step);
+              const checkpointId = stepCheckpoints.get(stepIndex);
               if (checkpointId && sessionRef.current) {
                 try {
                   await sessionRef.current.navigateTree(checkpointId, { summarize: false });
@@ -5959,7 +5967,7 @@ export class TaskExecutor {
               }
 
               // Reset step status to pending
-              await store.updateStep(taskId, step, "pending");
+              await store.updateStep(taskId, stepIndex, "pending");
 
               if (reviewType === "plan") {
                 await store.logEntry(