From a013bc0309a8115fec21654e5496edfab2cf6796 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 17 Jun 2026 19:52:20 -0700 Subject: [PATCH] FN-6607: align step tools with zero-based prompt steps Align executor step tools and review bookkeeping with the 0-based Step N labels agents see in PROMPT.md. - Treat fn_task_update and fn_review_step step parameters as 0-indexed values, including validation, logs, checkpoints, and review verdict maps. - Update executor/reviewer/step-runner guidance and generated tool docs to describe Step 0 semantics consistently. - Adjust affected executor and reliability tests and add coverage proving Step 0 progress, review, and revise handling work without off-by-one shifts. - Add a patch changeset for the published Fusion CLI package. Files changed: .changeset/fn-6607-step-numbering.md | 5 + .../cli/skill/fusion/references/engine-tools.md | 4 +- .../engine/src/__tests__/executor-pause.test.ts | 2 +- .../executor-review-step-indexing.test.ts | 18 +- .../src/__tests__/executor-review-verdicts.test.ts | 18 +- .../executor-step-numbering-zero-based.test.ts | 196 +++++++++++++++++++++ .../src/__tests__/executor-step-session.test.ts | 24 ++- ...executor-task-done-revise-verdict-guard.test.ts | 4 +- .../executor-pending-review-skip-retry.test.ts | 8 +- .../task-done-refusal-x-invariant.test.ts | 2 +- packages/engine/src/__tests__/step-runner.test.ts | 4 +- packages/engine/src/executor.ts | 57 +++--- packages/engine/src/reviewer.ts | 3 + packages/engine/src/step-runner.ts | 6 +- 14 files changed, 284 insertions(+), 67 deletions(-) Fusion-Task-Id: FN-6607 Fusion-Task-Lineage: 1b1fb1d8-07ca-4a33-84f5-1dab83388c01 --- .changeset/fn-6607-step-numbering.md | 5 + .../skill/fusion/references/engine-tools.md | 4 +- .../src/__tests__/executor-pause.test.ts | 2 +- .../executor-review-step-indexing.test.ts | 18 +- .../executor-review-verdicts.test.ts | 18 +- ...executor-step-numbering-zero-based.test.ts | 196 ++++++++++++++++++ .../__tests__/executor-step-session.test.ts | 24 ++- ...tor-task-done-revise-verdict-guard.test.ts | 4 +- ...executor-pending-review-skip-retry.test.ts | 8 +- .../task-done-refusal-x-invariant.test.ts | 2 +- .../engine/src/__tests__/step-runner.test.ts | 4 +- packages/engine/src/executor.ts | 57 ++--- packages/engine/src/reviewer.ts | 3 + packages/engine/src/step-runner.ts | 6 +- 14 files changed, 284 insertions(+), 67 deletions(-) create mode 100644 .changeset/fn-6607-step-numbering.md create mode 100644 packages/engine/src/__tests__/executor-step-numbering-zero-based.test.ts diff --git a/.changeset/fn-6607-step-numbering.md b/.changeset/fn-6607-step-numbering.md new file mode 100644 index 0000000000..452f8f3998 --- /dev/null +++ b/.changeset/fn-6607-step-numbering.md @@ -0,0 +1,5 @@ +--- +"@runfusion/fusion": patch +--- + +Fix the perpetual step off-by-one: `fn_task_update` and `fn_review_step` now treat `step` as 0-based, matching the `### Step N:` numbering in PROMPT.md (Step 0 = Preflight) and `TaskStore.updateStep`. Previously the tools were 1-indexed while everything agent-facing was 0-based, so agents could not mark Step 0 done and reviews/progress landed one step early. diff --git a/packages/cli/skill/fusion/references/engine-tools.md b/packages/cli/skill/fusion/references/engine-tools.md index 19c8ce6e51..9f809f68fe 100644 --- a/packages/cli/skill/fusion/references/engine-tools.md +++ b/packages/cli/skill/fusion/references/engine-tools.md @@ -68,10 +68,10 @@ Note: step-session execution (`step-session-executor.ts`) reuses executor coordi | Tool | Purpose | Parameters | |---|---|---| -| `fn_task_update` | Update a spec step status (`pending`/`in-progress`/`done`/`skipped`), task dependencies, and/or workflow-defined custom field values | `step?` (number, 1-indexed), `status?` (enum), `dependencies?` (string[]), `custom_fields?` (object keyed by field id; validated against the workflow field schema, `null` clears a field) | +| `fn_task_update` | Update a spec step status (`pending`/`in-progress`/`done`/`skipped`), task dependencies, and/or workflow-defined custom field values | `step?` (number, 0-indexed; matches `### Step N:` in PROMPT.md, Step 0 = Preflight), `status?` (enum), `dependencies?` (string[]), `custom_fields?` (object keyed by field id; validated against the workflow field schema, `null` clears a field) | | `fn_task_add_dep` | Add a dependency to current task (confirmation-gated) | `task_id` (string), `confirm?` (boolean) | | `fn_task_done` | Mark task complete and optionally store summary | `summary?` (string) | -| `fn_review_step` | Spawn step plan/code reviewer | `step` (number), `type` (`plan` \| `code`), `step_name` (string), `baseline?` (string) | +| `fn_review_step` | Spawn step plan/code reviewer | `step` (number, 0-indexed; matches `### Step N:` in PROMPT.md), `type` (`plan` \| `code`), `step_name` (string), `baseline?` (string) | | `fn_spawn_agent` | Spawn child agent in separate worktree | `name` (string), `role` (enum), `task` (string) | ## Merger-only runtime tools (`merger.ts`) diff --git a/packages/engine/src/__tests__/executor-pause.test.ts b/packages/engine/src/__tests__/executor-pause.test.ts index c9d6c99dcd..3239a763ae 100644 --- a/packages/engine/src/__tests__/executor-pause.test.ts +++ b/packages/engine/src/__tests__/executor-pause.test.ts @@ -1402,7 +1402,7 @@ describe("TaskExecutor agent execution flow (FN-978)", () => { stuckDetector, ); - const result = await tool.execute("call-1", { step: 1, status: "in-progress" }); + const result = await tool.execute("call-1", { step: 0, status: "in-progress" }); expect(stuckDetector.recordIgnoredStepUpdate).toHaveBeenCalledWith("FN-001"); expect(result.content[0].text).toContain("already done"); diff --git a/packages/engine/src/__tests__/executor-review-step-indexing.test.ts b/packages/engine/src/__tests__/executor-review-step-indexing.test.ts index 095abc201d..7ed152a7e9 100644 --- a/packages/engine/src/__tests__/executor-review-step-indexing.test.ts +++ b/packages/engine/src/__tests__/executor-review-step-indexing.test.ts @@ -80,11 +80,11 @@ describe("fn_review_step indexing", () => { resetExecutorMocks(); }); - it("uses step=2 to update internal step index 1", async () => { + it("uses step=1 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" }); + await tools.fn_review_step("call-1", { step: 1, type: "code", step_name: "Implement", baseline: "abc" }); expect(stepStates[1].status).toBe("done"); expect(store.updateStep).toHaveBeenCalledWith("FN-TEST", 1, "in-progress"); @@ -95,28 +95,28 @@ describe("fn_review_step indexing", () => { 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" }); + await tools.fn_task_update("set-cp", { step: 1, status: "in-progress" }); + await tools.fn_review_step("call-1", { step: 1, 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 () => { + it("REVISE verdict for step=1 blocks fn_task_update step=1 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" }); + await tools.fn_review_step("call-1", { step: 1, type: "code", step_name: "Implement", baseline: "abc" }); + const result = await tools.fn_task_update("call-2", { step: 1, status: "done" }); - expect(result.content[0].text).toContain("Cannot mark Step 2 as done"); + expect(result.content[0].text).toContain("Cannot mark Step 1 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]; + const invalids = [-1, 3, 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"); diff --git a/packages/engine/src/__tests__/executor-review-verdicts.test.ts b/packages/engine/src/__tests__/executor-review-verdicts.test.ts index 6ab4bd6a58..c891974798 100644 --- a/packages/engine/src/__tests__/executor-review-verdicts.test.ts +++ b/packages/engine/src/__tests__/executor-review-verdicts.test.ts @@ -732,7 +732,7 @@ describe("Code review verdict enforcement - fn_task_update blocking", () => { // the same step does not produce the "Cannot mark … as done" block. await tools.fn_review_step("c1", { step: 2, type: "code", step_name: "Testing", baseline: "a" }); - const result = await tools.fn_task_update("c2", { step: 3, status: "in-progress" }); + const result = await tools.fn_task_update("c2", { step: 2, status: "in-progress" }); expect(result.content[0].text).not.toContain("Cannot mark"); expect(result.content[0].text).toContain("→ in-progress"); }); @@ -897,7 +897,7 @@ describe("RETHINK verdict handling", () => { }); // updateStep should be called: once for in-progress, once for pending (reset) - expect(store.updateStep).toHaveBeenCalledWith("FN-040", 0, "pending"); + expect(store.updateStep).toHaveBeenCalledWith("FN-040", 1, "pending"); }); it("RETHINK re-prompt includes reviewer feedback", async () => { @@ -1012,7 +1012,7 @@ describe("RETHINK verdict handling", () => { expect(mockSessionManager.getLeafId).toHaveBeenCalled(); }); - it("uses step-1 checkpoint key when step 3 enters in-progress and step index 2 is reviewed", async () => { + it("uses zero-based checkpoint key when step 2 enters in-progress and is reviewed", async () => { const store = createMockStore(); store.updateStep.mockImplementation(async (_id: string, step: number, status: string) => makeStepResult(step, status), @@ -1020,10 +1020,10 @@ describe("RETHINK verdict handling", () => { mockedReviewStep.mockResolvedValue({ verdict: "RETHINK", review: "Bad", summary: "Redo" }); const { toolMap, mockNavigateTree } = await captureRethinkTools(store); - await toolMap.get("fn_task_update").execute("call-1", { step: 3, status: "in-progress" }); + await toolMap.get("fn_task_update").execute("call-1", { step: 2, status: "in-progress" }); await toolMap.get("fn_review_step").execute("call-2", { - step: 3, + step: 2, type: "code", step_name: "Testing", baseline: "abc123", @@ -1239,7 +1239,7 @@ describe("Plan RETHINK verdict handling", () => { }); // updateStep should be called with "pending" to reset the step - expect(store.updateStep).toHaveBeenCalledWith("FN-050", 0, "pending"); + expect(store.updateStep).toHaveBeenCalledWith("FN-050", 1, "pending"); }); it("plan RETHINK re-prompt includes reviewer feedback and plan-specific language", async () => { @@ -1435,10 +1435,10 @@ describe("E2E review pipeline — multi-verdict sequence", () => { })); const { tools } = await captureE2ETools(store); - const result = await tools.fn_task_update("u-warn", { step: 2, status: "in-progress" }); + const result = await tools.fn_task_update("u-warn", { step: 1, status: "in-progress" }); expect(store.updateStep).toHaveBeenCalledWith("FN-E2E", 1, "in-progress"); - expect(result.content[0].text).toContain("Step 2 (Implement) → in-progress"); + expect(result.content[0].text).toContain("Step 1 (Implement) → in-progress"); }); it("full sequence: plan APPROVE → code REVISE (blocked) → code APPROVE (unblocked) → done", async () => { @@ -1513,7 +1513,7 @@ describe("E2E review pipeline — multi-verdict sequence", () => { expect.objectContaining({ cwd: expect.any(String) }), ); expect(mockNavigateTree).toHaveBeenCalledWith("e2e-checkpoint", { summarize: false }); - expect(store.updateStep).toHaveBeenCalledWith("FN-E2E", 0, "pending"); + expect(store.updateStep).toHaveBeenCalledWith("FN-E2E", 1, "pending"); // Step 3: Restart the step (new approach) await tools.fn_task_update("u2", { step: 1, status: "in-progress" }); diff --git a/packages/engine/src/__tests__/executor-step-numbering-zero-based.test.ts b/packages/engine/src/__tests__/executor-step-numbering-zero-based.test.ts new file mode 100644 index 0000000000..b898838cbc --- /dev/null +++ b/packages/engine/src/__tests__/executor-step-numbering-zero-based.test.ts @@ -0,0 +1,196 @@ +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); + +describe("executor tool step numbering is 0-based", () => { + beforeEach(() => { + resetExecutorMocks(); + mockedExistsSync.mockReturnValue(true); + }); + + async function captureTools(stepStates = [ + { name: "Preflight", status: "pending" }, + { name: "First", status: "pending" }, + { name: "Second", status: "pending" }, + ]) { + const store = createMockStore(); + store.getTask.mockImplementation(async () => ({ + id: "FN-6607-T", + title: "Zero based steps", + description: "", + column: "in-progress", + dependencies: [], + steps: stepStates.map((step) => ({ ...step })), + currentStep: 0, + log: [], + prompt: "# test\n## Steps\n### Step 0: Preflight\n### Step 1: First\n### Step 2: Second", + 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((step) => ({ ...step })) }; + }); + + let customTools: any[] = []; + mockedCreateFnAgent.mockImplementation(async (opts: any) => { + customTools = opts.customTools || []; + return { + session: { + prompt: vi.fn().mockResolvedValue(undefined), + dispose: vi.fn(), + subscribe: vi.fn(), + on: vi.fn(), + navigateTree: vi.fn(), + sessionManager: { + getLeafId: vi.fn().mockReturnValue("leaf-step"), + branchWithSummary: vi.fn(), + }, + state: {}, + }, + } as any; + }); + + const executor = new TaskExecutor(store, "/tmp/test"); + await executor.execute({ + id: "FN-6607-T", + title: "Zero based steps", + description: "", + column: "in-progress", + dependencies: [], + steps: [], + currentStep: 0, + log: [], + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + } as any); + + const tools: Record = {}; + for (const tool of customTools) tools[tool.name] = tool.execute; + return { tools, store, stepStates }; + } + + it("maps fn_task_update and fn_review_step step directly to task.steps index", async () => { + mockedReviewStep.mockResolvedValue({ verdict: "APPROVE", review: "ok", summary: "ok" } as any); + const { tools, store, stepStates } = await captureTools(); + + const preflightDone = await tools.fn_task_update("update-0", { step: 0, status: "done" }); + expect(preflightDone.content[0].text).toContain("Step 0 (Preflight) → done"); + expect(store.updateStep).toHaveBeenCalledWith("FN-6607-T", 0, "done"); + expect(stepStates[0].status).toBe("done"); + + const firstStarted = await tools.fn_task_update("update-1", { step: 1, status: "in-progress" }); + expect(firstStarted.content[0].text).toContain("Step 1 (First) → in-progress"); + expect(store.updateStep).toHaveBeenCalledWith("FN-6607-T", 1, "in-progress"); + expect(stepStates[1].status).toBe("in-progress"); + + const review = await tools.fn_review_step("review-1", { + step: 1, + type: "code", + step_name: "First", + baseline: "abc123", + }); + expect(review.content[0].text).toBe("APPROVE"); + expect(mockedReviewStep).toHaveBeenCalledWith( + expect.any(String), + "FN-6607-T", + 1, + "First", + "code", + expect.any(String), + "abc123", + expect.any(Object), + ); + expect(store.logEntry).toHaveBeenCalledWith("FN-6607-T", "code review Step 1: APPROVE", "ok"); + expect(store.updateStep).toHaveBeenCalledWith("FN-6607-T", 1, "done"); + + const invalidNegative = await tools.fn_task_update("bad-update-negative", { step: -1, status: "done" }); + expect(invalidNegative.content[0].text).toContain("0-indexed"); + const invalidReview = await tools.fn_review_step("bad-review", { step: 3, type: "code", step_name: "Missing", baseline: "abc" }); + expect(invalidReview.details.error).toBe("invalid_step"); + }); + + it("resume recovery reads the same 0-based review log written by fn_review_step", async () => { + const store = createMockStore(); + store.getTask.mockResolvedValue({ + id: "FN-6607-R", + title: "Resume", + description: "", + column: "in-progress", + dependencies: [], + steps: [ + { name: "Preflight", status: "done" }, + { name: "First", status: "in-progress" }, + { name: "Second", status: "pending" }, + ], + currentStep: 1, + log: [ + { timestamp: "2026-06-17T00:00:00.000Z", action: "Step 1 (First) → in-progress" }, + { timestamp: "2026-06-17T00:00:01.000Z", action: "code review Step 1: APPROVE" }, + ], + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + } as any); + + const executor = new TaskExecutor(store as any, "/tmp/test"); + await (executor as any).recoverApprovedStepsOnResume("FN-6607-R"); + + expect(store.updateStep).toHaveBeenCalledWith("FN-6607-R", 1, "done"); + expect(store.logEntry).toHaveBeenCalledWith( + "FN-6607-R", + expect.stringContaining("Step 1 (First) recovered as done on resume"), + ); + }); + + it("pending-review loop detection matches 0-based writer strings", async () => { + const store = createMockStore(); + const task = { + id: "FN-6607-P", + title: "Pending review", + description: "", + column: "in-progress", + dependencies: [], + taskDoneRetryCount: 2, + steps: [ + { name: "Preflight", status: "done" }, + { name: "First", status: "in-progress" }, + ], + currentStep: 1, + log: [{ timestamp: new Date().toISOString(), action: "code review requested for Step 1 (First)" }], + prompt: "# test\n## Steps\n### Step 0: Preflight\n### Step 1: First", + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + } as any; + store.getTask.mockResolvedValue(task); + 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 as any, "/tmp/test"); + await executor.execute(task); + + expect(store.logEntry).toHaveBeenCalledWith( + "FN-6607-P", + expect.stringContaining("Step 1 is blocked on pending review"), + undefined, + expect.objectContaining({ agentId: "executor" }), + ); + expect(store.moveTask).toHaveBeenCalledWith("FN-6607-P", "in-review"); + }); +}); diff --git a/packages/engine/src/__tests__/executor-step-session.test.ts b/packages/engine/src/__tests__/executor-step-session.test.ts index 0006f86925..23c5f98efa 100644 --- a/packages/engine/src/__tests__/executor-step-session.test.ts +++ b/packages/engine/src/__tests__/executor-step-session.test.ts @@ -224,7 +224,7 @@ describe("Workflow Steps Execution", () => { 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" }); + await reviewTool.execute("tool-review", { step: 0, type: "code", step_name: "Implement" }); } }), dispose: vi.fn(), @@ -269,7 +269,7 @@ describe("Workflow Steps Execution", () => { dependencies: [], steps: [{ name: "Implement", status: "in-progress" }], currentStep: 0, - log: [{ action: "code review requested for Step 1 (Implement)", timestamp: new Date().toISOString() }], + log: [{ action: "code review requested for Step 0 (Implement)", timestamp: new Date().toISOString() }], prompt: "# test\n## Steps\n### Step 1: Implement\n- [ ] implement", createdAt: new Date().toISOString(), updatedAt: new Date().toISOString(), @@ -314,7 +314,7 @@ describe("Workflow Steps Execution", () => { dependencies: [], steps: [{ name: "Implement", status: "in-progress" }], currentStep: 0, - log: [{ action: "code review Step 1: APPROVE", timestamp: new Date().toISOString() }], + log: [{ action: "code review Step 0: APPROVE", timestamp: new Date().toISOString() }], prompt: "# test\n## Steps\n### Step 1: Implement\n- [ ] implement", createdAt: new Date().toISOString(), updatedAt: new Date().toISOString(), @@ -2866,12 +2866,18 @@ describe("Workflow Steps Execution", () => { }; return { session }; } else { - // Workflow step agent that passes (no REQUEST REVISION) + // Workflow step agent that passes with an explicit parseable verdict. + let subscribeHandler: any; return { session: { - prompt: vi.fn().mockResolvedValue(undefined), + prompt: vi.fn().mockImplementation(async () => { + subscribeHandler?.({ + type: "message_update", + assistantMessageEvent: { type: "text_delta", delta: "Verdict: APPROVE\n\nWorkflow step passed." }, + }); + }), dispose: vi.fn(), - subscribe: vi.fn(), + subscribe: vi.fn((handler: any) => { subscribeHandler = handler; }), state: {}, }, }; @@ -3553,14 +3559,14 @@ describe("U2: fn_review_step RETHINK delegates to resetStepToBaseline (character const updateTool = tools.find((t: any) => t.name === "fn_task_update"); if (updateTool) { try { - await updateTool.execute("tool-update", { step: 1, status: "in-progress" }); + await updateTool.execute("tool-update", { step: 0, status: "in-progress" }); } catch { /* tool param shape varies; ignore */ } } const reviewTool = tools.find((t: any) => t.name === "fn_review_step"); if (reviewTool) { try { await reviewTool.execute("tool-review", { - step: 1, + step: 0, type: reviewType, step_name: "Implement", baseline: reviewType === "code" ? "agentBaselineSHA" : undefined, @@ -3624,7 +3630,7 @@ describe("U2: fn_review_step RETHINK delegates to resetStepToBaseline (character expect(store.updateStep).toHaveBeenCalledWith("FN-RT-1", 0, "pending"); expect(store.logEntry).toHaveBeenCalledWith( "FN-RT-1", - expect.stringContaining("Step 1 plan rewound"), + expect.stringContaining("Step 0 plan rewound"), "rejected approach", ); }); diff --git a/packages/engine/src/__tests__/executor-task-done-revise-verdict-guard.test.ts b/packages/engine/src/__tests__/executor-task-done-revise-verdict-guard.test.ts index c71336f9d3..2befec264b 100644 --- a/packages/engine/src/__tests__/executor-task-done-revise-verdict-guard.test.ts +++ b/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: 1, type: "code", step_name: "Step 1", baseline: "abc123" }); + await reviewTool.execute("rev", { step: 0, 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: 1, type: "code", step_name: "Step 1", baseline: "abc123" }); + await reviewTool.execute("rev", { step: 0, 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"); diff --git a/packages/engine/src/__tests__/reliability-interactions/executor-pending-review-skip-retry.test.ts b/packages/engine/src/__tests__/reliability-interactions/executor-pending-review-skip-retry.test.ts index bb19b43486..23ffe9dcf7 100644 --- a/packages/engine/src/__tests__/reliability-interactions/executor-pending-review-skip-retry.test.ts +++ b/packages/engine/src/__tests__/reliability-interactions/executor-pending-review-skip-retry.test.ts @@ -44,7 +44,7 @@ describe("reliability interactions: FN-5436 executor pending-review skip", () => 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() }], + log: [{ action: "code review Step 0: REVISE", timestamp: new Date().toISOString() }], }); store.getTask.mockResolvedValue(task); @@ -78,7 +78,7 @@ describe("reliability interactions: FN-5436 executor pending-review skip", () => const task = makeTask({ id: "FN-5436-RI-C", taskDoneRetryCount: 2, - log: [{ action: "code review requested for Step 1 (Step 1)", timestamp: new Date().toISOString() }], + log: [{ action: "code review requested for Step 0 (Step 1)", timestamp: new Date().toISOString() }], }); store.getTask.mockResolvedValue(task); @@ -98,7 +98,7 @@ describe("reliability interactions: FN-5436 executor pending-review skip", () => 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() }], + log: [{ action: "code review Step 0: APPROVE", timestamp: new Date().toISOString() }], }); store.getTask.mockResolvedValue(task); @@ -117,7 +117,7 @@ describe("reliability interactions: FN-5436 executor pending-review skip", () => 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() }], + log: [{ action: "plan review Step 0: UNAVAILABLE — proceeding advisory after fallback retry exhausted", timestamp: new Date().toISOString() }], }); store.getTask.mockResolvedValue(task); diff --git a/packages/engine/src/__tests__/reliability-interactions/task-done-refusal-x-invariant.test.ts b/packages/engine/src/__tests__/reliability-interactions/task-done-refusal-x-invariant.test.ts index ca6b4ab6cf..12880e8b74 100644 --- a/packages/engine/src/__tests__/reliability-interactions/task-done-refusal-x-invariant.test.ts +++ b/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: 1, type: "code", step_name: "S1", baseline: "abc" }); + await reviewTool.execute("rev", { step: 0, 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); diff --git a/packages/engine/src/__tests__/step-runner.test.ts b/packages/engine/src/__tests__/step-runner.test.ts index 25c885048f..a2906b191c 100644 --- a/packages/engine/src/__tests__/step-runner.test.ts +++ b/packages/engine/src/__tests__/step-runner.test.ts @@ -220,8 +220,8 @@ describe("resetStepToBaseline", () => { expect(store.updateStep).toHaveBeenCalledWith("FN-001", 2, "pending"); expect(store.logEntry).toHaveBeenCalledWith( "FN-001", - // 0-indexed step 2 → 1-indexed "Step 3" - expect.stringContaining("Step 3 plan rewound"), + // 0-indexed step 2 is displayed as "Step 2" to match PROMPT.md headings. + expect.stringContaining("Step 2 plan rewound"), "plan rejected", ); }); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index b15d52b81a..573c4b15b9 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -434,7 +434,7 @@ function detectPendingReviewBlock( .filter((action): action is string => Boolean(action)); for (const stepIndex of inProgressStepIndices) { - const stepDisplay = stepIndex + 1; + const stepDisplay = stepIndex; const codeRequest = `code review requested for Step ${stepDisplay}`; const planRequest = `plan review requested for Step ${stepDisplay}`; const codeVerdictPrefix = `code review Step ${stepDisplay}:`; @@ -485,7 +485,7 @@ export function evaluateTaskDoneRefusal( } pendingSteps.push(stepIndex); if (codeReviewVerdicts.get(stepIndex) === "REVISE") { - const reason = `Step ${stepIndex + 1} (${step.name}) has a pending code review verdict of REVISE`; + const reason = `Step ${stepIndex} (${step.name}) has a pending code review verdict of REVISE`; return { ok: false, refusalClass: "pending-code-review-revise", @@ -918,7 +918,7 @@ export async function __runConfiguredCommandForTests( // ── Tool parameter schemas (module-level for reuse in ToolDefinition generics) ── const taskUpdateParams = Type.Object({ - step: Type.Optional(Type.Number({ description: "Step number (1-indexed). Omit when updating only custom_fields/dependencies." })), + step: Type.Optional(Type.Number({ description: "Step number (0-indexed; matches the `### Step N:` numbers in PROMPT.md — Step 0 is Preflight). Omit when updating only custom_fields/dependencies." })), status: Type.Optional(Type.Union( STEP_STATUSES.map((s) => Type.Literal(s)), { description: "New status: pending, in-progress, done, or skipped. Required when step is set." }, @@ -1093,7 +1093,7 @@ export function parseWorkflowStepOutput(rawOutput: string): { } const reviewStepParams = Type.Object({ - step: Type.Number({ description: "Step number to review" }), + step: Type.Number({ description: "Step number to review (0-indexed; matches the `### Step N:` numbers in PROMPT.md — Step 0 is Preflight)." }), type: Type.Union( [Type.Literal("plan"), Type.Literal("code")], { description: 'Review type: "plan" or "code"' }, @@ -1150,6 +1150,7 @@ If you genuinely cannot proceed (blocked on a dependency, missing information, o You have tools to report progress. The board updates in real-time. **Step lifecycle:** +The \`step\` argument is 0-based and equals the literal \`### Step N:\` number in PROMPT.md (Step 0 is Preflight). - Before starting a step: \`fn_task_update(step=N, status="in-progress")\` - After completing a step: \`fn_task_update(step=N, status="done")\` - If skipping a step: \`fn_task_update(step=N, status="skipped")\` @@ -5476,7 +5477,7 @@ export class TaskExecutor { const detail = await this.store.getTask(seamTask.id); // Worktree isolation (KTD-11): review the instance's OWN worktree when set. const worktreePath = active.worktreePath || detail.worktree || this.rootDir; - const stepName = detail.steps[stepIndex]?.name ?? `Step ${stepIndex + 1}`; + const stepName = detail.steps[stepIndex]?.name ?? `Step ${stepIndex}`; const promptContent = detail.prompt ?? ""; // Merge per-task effective workflow settings (U3, KTD-3) so the validator // model-lane reads below pick up workflow values. Behavior-inert by default. @@ -5487,7 +5488,7 @@ export class TaskExecutor { reviewStep( worktreePath, seamTask.id, - stepIndex + 1, // reviewStep is 1-indexed (matches fn_review_step) + stepIndex, stepName, config.type, promptContent, @@ -5533,7 +5534,7 @@ export class TaskExecutor { await this.store.logEntry( seamTask.id, - `${config.type} step-review Step ${stepIndex + 1}: ${review.verdict}${config.advisory ? " (advisory)" : ""}`, + `${config.type} step-review Step ${stepIndex}: ${review.verdict}${config.advisory ? " (advisory)" : ""}`, review.summary, ); @@ -5548,12 +5549,12 @@ export class TaskExecutor { await this.updateStepGraph(seamTask.id, stepIndex, "done"); await this.store.logEntry( seamTask.id, - `Step ${stepIndex + 1} (${stepName}) marked done by step-review APPROVE (graph)`, + `Step ${stepIndex} (${stepName}) marked done by step-review APPROVE (graph)`, ); } } catch (err) { reviewerLog.warn( - `${seamTask.id}: failed to mark Step ${stepIndex + 1} done after APPROVE: ${err instanceof Error ? err.message : String(err)}`, + `${seamTask.id}: failed to mark Step ${stepIndex} done after APPROVE: ${err instanceof Error ? err.message : String(err)}`, ); } } @@ -7668,8 +7669,7 @@ export class TaskExecutor { // Build custom tools for the worker // Track the last code review verdict per step so we can enforce REVISE // (block fn_task_update status="done" until the agent re-reviews and gets APPROVE). - // Keyed by 0-indexed step (stepIndex). fn_task_update translates from - // its 1-indexed `step` parameter via `stepIndex = step - 1` (FN-3757). + // Keyed by the canonical 0-indexed step number used by PROMPT.md headings. const codeReviewVerdicts = new Map(); let wasPaused = false; @@ -8294,7 +8294,7 @@ export class TaskExecutor { ); 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`, + `Agent finished without calling fn_task_done but Step ${pendingReviewBlock.stepIndex} is blocked on pending review (${pendingReviewBlock.reason}) — skipping retry session`, undefined, this.getRunContextFor(task.id), ); @@ -9488,17 +9488,21 @@ export class TaskExecutor { }; } - if (!Number.isInteger(step) || step < 1) { + if (!Number.isInteger(step) || step < 0) { return { content: [{ type: "text" as const, - text: `Invalid step number: ${step}. Steps are 1-indexed.`, + text: `Invalid step number: ${step}. Steps are 0-indexed; Step 0 is Preflight.`, }], details: {}, }; } - const stepIndex = step - 1; + /* + * FNXC:StepNumbering 2026-06-17-00:00: + * FN-6607 makes fn_task_update.step the same 0-based number agents see in PROMPT.md (`### Step N:`) and TaskStore.updateStep uses internally. The prior `step - 1` conversion made Step 0 impossible to mark done and shifted every review/progress update one array slot early. + */ + const stepIndex = step; if (status === "in-progress") { try { @@ -9508,7 +9512,7 @@ export class TaskExecutor { ); if (otherInProgressStepIndex !== -1) { executorLog.warn( - `${taskId}: fn_task_update marking step ${step} in-progress while step ${otherInProgressStepIndex + 1} is already in-progress`, + `${taskId}: fn_task_update marking step ${step} in-progress while step ${otherInProgressStepIndex} is already in-progress`, ); } } catch (err) { @@ -9519,7 +9523,7 @@ export class TaskExecutor { // Enforce code review REVISE: block advancing to "done" when the last // code review for this step returned REVISE. The agent must fix the // issues and call fn_review_step(type="code") again before proceeding. - // FN-3757: verdict/checkpoint maps are keyed by 0-indexed stepIndex. + // FN-6607: verdict/checkpoint maps are keyed directly by the 0-indexed tool step. if (status === "done" && codeReviewVerdicts.get(stepIndex) === "REVISE") { return { content: [{ @@ -9576,7 +9580,7 @@ export class TaskExecutor { return { content: [{ type: "text" as const, - text: `Invalid step number: ${step}. This task has ${task.steps.length} step(s) (1-indexed).`, + text: `Invalid step number: ${step}. This task has ${task.steps.length} step(s) (0-indexed; valid range 0-${Math.max(0, task.steps.length - 1)}).`, }], details: {}, }; @@ -9596,7 +9600,7 @@ export class TaskExecutor { ) { const leafId = sessionRef.current.sessionManager.getLeafId(); if (leafId) { - // FN-3757: verdict/checkpoint maps are keyed by 0-indexed stepIndex. + // FN-6607: verdict/checkpoint maps are keyed directly by the 0-indexed tool step. stepCheckpoints.set(stepIndex, leafId); } } @@ -10487,18 +10491,16 @@ export class TaskExecutor { parameters: reviewStepParams, execute: async (_toolCallId: string, params: Static) => { 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 stepIndex = step; 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) { + if (!Number.isInteger(step) || step < 0 || step >= 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.` }], + content: [{ type: "text" as const, text: `Invalid step ${step}. Task has ${taskSteps.length} step(s) and fn_review_step is 0-indexed; Step 0 is Preflight.` }], details: { error: "invalid_step", step, - maxStep: taskSteps.length, + maxStep: taskSteps.length > 0 ? taskSteps.length - 1 : -1, }, }; } @@ -10590,7 +10592,8 @@ export class TaskExecutor { stuckDetector?.recordProgress(taskId); // Track code review verdicts for enforcement. Plan reviews remain - // advisory — only code reviews write to the verdict map. + // advisory — only code reviews write to the verdict map. FN-6607 keeps + // the map keyed by the same 0-indexed `step` value the tool receives. if (reviewType === "code") { if (result.verdict === "REVISE") { codeReviewVerdicts.set(stepIndex, "REVISE"); @@ -15103,7 +15106,7 @@ You are running in an **isolated git worktree**. This means: ${hasProgress ? `Resume from Step ${task.currentStep}. Do NOT redo completed steps.` : "Start with Step 0 (Preflight). Work through each step in order."} -Use \`fn_task_update\` to report progress on every step transition. +Use \`fn_task_update\` to report progress on every step transition; its \`step\` value is 0-based and equals the \`### Step N:\` number in PROMPT.md. Use \`fn_task_log\` for important actions and decisions. Use \`fn_task_create\` for truly separate follow-up work, including unrelated/pre-existing broad-suite failures. Commit at step boundaries: \`git commit -m "feat(${task.id}): complete Step N — "${sourceIssueRef ? ` -m "Ref: ${sourceIssueRef}"` : ""}${authorArg}\` diff --git a/packages/engine/src/reviewer.ts b/packages/engine/src/reviewer.ts index 1730c5d1d3..04e4d2cc5c 100644 --- a/packages/engine/src/reviewer.ts +++ b/packages/engine/src/reviewer.ts @@ -115,6 +115,9 @@ export interface ReviewOptions { /** * Spawn a reviewer agent to evaluate a worker's plan or code for a step. + * + * FNXC:StepNumbering 2026-06-17-00:00: + * `stepNumber` is display-only and must remain the same 0-based number shown in PROMPT.md (`### Step N:`). Review prompts, task logs, resume reconciliation, and loop-detection all compare this literal Step N string. */ export async function reviewStep( cwd: string, diff --git a/packages/engine/src/step-runner.ts b/packages/engine/src/step-runner.ts index 8e7ef41e85..edf7d38375 100644 --- a/packages/engine/src/step-runner.ts +++ b/packages/engine/src/step-runner.ts @@ -238,7 +238,11 @@ export async function resetStepToBaseline( const { store, worktreePath, sessionRef } = deps; const reviewType = deps.reviewType ?? "code"; const taskId = task.id; - const step = stepIndex + 1; // legacy log lines are 1-indexed + const step = stepIndex; + /* + * FNXC:StepReset 2026-06-17-00:00: + * RETHINK reset logs use the same 0-based Step N as fn_review_step and PROMPT.md so recovery tooling can correlate review verdicts, checkpoints, and reset events without off-by-one translation. + */ // ── KTD-2 blast-radius guard — assert BEFORE mutating anything. ────────── if (deps.blastRadiusGuard) {