From 095007c640befa95e067c2562a217ebca9148bb6 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Mon, 29 Jun 2026 21:51:13 -0700 Subject: [PATCH] fix(FN-7262): honor step session setting --- .../workflow-graph-step-rerun.test.ts | 24 +++++++++ packages/engine/src/executor.ts | 50 ++++++++++--------- 2 files changed, 50 insertions(+), 24 deletions(-) diff --git a/packages/engine/src/__tests__/workflow-graph-step-rerun.test.ts b/packages/engine/src/__tests__/workflow-graph-step-rerun.test.ts index 376e5bb48e..22c86aa70b 100644 --- a/packages/engine/src/__tests__/workflow-graph-step-rerun.test.ts +++ b/packages/engine/src/__tests__/workflow-graph-step-rerun.test.ts @@ -100,6 +100,30 @@ describe("runGraphTaskStep (FIX 3)", () => { expect(result.success).toBe(true); }); + it("does not force step-session mode for final-review coding steps", async () => { + const { executor } = makeExecutor("done", { deferDoneToReview: false }); + executor.runImplementationPhase = vi.fn().mockResolvedValue({ taskDone: true, modifiedFiles: [] }); + + const result = await executor.runGraphTaskStep(task, 0, "inst-0"); + + /* + * FNXC:WorkflowStepSessions 2026-06-30-00:00: + * Default Coding has no per-step review, so it must let `runStepsInNewSessions` control session reuse. The graph driver should not pin StepSessionExecutor for this shape. + */ + expect(result.success).toBe(true); + expect(executor.graphStepSessionPinned.has("FN-001")).toBe(false); + }); + + it("forces step-session mode when step-review owns done-marking", async () => { + const { executor } = makeExecutor("in-progress", { deferDoneToReview: true }); + executor.runImplementationPhase = vi.fn().mockResolvedValue({ taskDone: false, modifiedFiles: [] }); + + const result = await executor.runGraphTaskStep(task, 0, "inst-0"); + + expect(result.success).toBe(true); + expect(executor.graphStepSessionPinned.has("FN-001")).toBe(true); + }); + // T9: a RETHINK after a SUCCESSFUL pass must clear the memoized implementation // so the rework re-runs implementation rather than re-awaiting the resolved memo. it("clears the memo on rethink reset so implementation re-runs after a successful pass", async () => { diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index c08b821f7f..7cb2806b9c 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -4377,11 +4377,12 @@ export class TaskExecutor { * Doubles as the re-entrancy guard for graph routing. */ private graphCompletionInterceptors = new Map void>(); - /** Step-inversion (KTD-2/KTD-8, U6/U8): tasks whose graph-owned step-execute - * driver has pinned step-session physics for the run. Forces the step-session - * path in execute() regardless of the `runStepsInNewSessions` setting, so the - * graph/step-sessions flag matrix cannot select an unsupported physics combo. - * Cleared when the graph run ends (maybeExecuteWorkflowGraph finally). */ + /** Step-inversion (KTD-2/KTD-8, U6/U8): graph-owned step-execute can pin + * step-session physics for workflows that need a hard per-step boundary + * before step-review. Default final-review coding does not pin here and + * therefore respects `runStepsInNewSessions` (reuse one session when false, + * fresh per-step sessions when true). Cleared when the graph run ends + * (maybeExecuteWorkflowGraph finally). */ private graphStepSessionPinned = new Set(); /** Step-inversion (U6/U8): caches the per-run implementation-phase result for a @@ -5415,17 +5416,13 @@ export class TaskExecutor { * The U3 stand-in ran `runImplementationPhase` once per foreach instance, which * re-ran the whole implementation for every step. The real driver: * - * 1. PINS step-session physics for the run (graph-owned runs force - * StepSessionExecutor regardless of `runStepsInNewSessions`, KTD-2/KTD-8) — - * the only path with a discrete per-step boundary (`onStepStart`/ - * `onStepComplete`); the monolithic single-session path has no "run one - * step and return control" seam. - * 2. Drives the (step-session) implementation phase exactly ONCE per run, - * memoized by task id. StepSessionExecutor itself walks every step in step - * order inside that single pass and writes the projection per step via its - * `onStepStart`/`onStepComplete` callbacks (executor.ts step-session path). - * Each foreach instance's `runTaskStep` therefore observes the projection - * truth for its step rather than re-running the agent per step. + * 1. Pins step-session physics only when the workflow needs a discrete + * per-step boundary before a step-review node. Final-review coding lets + * `runStepsInNewSessions` choose between one reused executor session and + * fresh per-step sessions. + * 2. Drives the implementation phase exactly ONCE per run, memoized by task + * id. Each foreach instance's `runTaskStep` observes projection truth for + * its step rather than re-running the agent per step. * * Worktree/taskEnv/agent/semaphore state is threaded exactly the way * `runImplementationPhase` gets it — by re-entering `execute()` under a @@ -5441,8 +5438,14 @@ export class TaskExecutor { instanceId?: string, governingNodeId?: string, ): Promise<{ success: boolean; error?: string }> { - // Pin step-session physics for the run before the implementation pass. - this.graphStepSessionPinned.add(task.id); + const active = this.foreachActiveForTask(task.id, instanceId); + /* + FNXC:WorkflowStepSessions 2026-06-30-00:00: + Default Coding is graph-owned stepwise execution without per-step review. It should reuse the existing executor session when the workflow setting `runStepsInNewSessions` is false, and create fresh step sessions only when that setting is true. Workflows with a step-review node still pin StepSessionExecutor because review must run between step execution and done-marking. + */ + if (active?.deferDoneToReview === true) { + this.graphStepSessionPinned.add(task.id); + } // Single-flight per attempt (KTD-2/KTD-8): the implementation phase runs once // per run, memoized by task id, so each foreach instance's `runStep` observes @@ -5515,7 +5518,6 @@ export class TaskExecutor { error: `step ${stepIndex} live task unavailable after implementation pass`, }; } - const active = this.foreachActiveForTask(task.id, instanceId); const status = live.steps[stepIndex]?.status; if (status === "done" || status === "skipped") return { success: true }; // Step not terminal after the pass: when a review will author done-ness @@ -6082,11 +6084,11 @@ export class TaskExecutor { { store: this.store, worktreePath, - // U6/U8: per-step session physics — graph-owned runs force - // step-session mode for the run (KTD-2/KTD-8) regardless of the - // runStepsInNewSessions setting. The agent authors the step's commit; - // this driver only observes (KTD-2). Thread the instanceId so the - // active-context read is per-instance (parallel-foreach safe). + // U6/U8: graph-owned per-step physics. Per-step-review workflows + // pin StepSessionExecutor inside runGraphTaskStep; final-review coding + // honors runStepsInNewSessions and may reuse one executor session. + // Thread the instanceId so the active-context read is per-instance + // (parallel-foreach safe). runStep: (stepIndex) => this.runGraphTaskStep( seamTask,