From 50ccd79b9f7b9df574708cd1afede4263c724f88 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Mon, 29 Jun 2026 03:15:30 -0700 Subject: [PATCH] fix(FN-7226): land graph step-session commits --- .../fn-7226-step-session-file-capture.md | 7 ++++ .../__tests__/store-update-step-order.test.ts | 9 ++++- .../__tests__/step-session-executor.test.ts | 40 +++++++++++++++---- packages/engine/src/executor.ts | 18 ++++----- packages/engine/src/step-runner.ts | 22 +++++----- packages/engine/src/step-session-executor.ts | 33 +++++++++++++-- 6 files changed, 92 insertions(+), 37 deletions(-) create mode 100644 .changeset/fn-7226-step-session-file-capture.md diff --git a/.changeset/fn-7226-step-session-file-capture.md b/.changeset/fn-7226-step-session-file-capture.md new file mode 100644 index 0000000000..8732af9eb4 --- /dev/null +++ b/.changeset/fn-7226-step-session-file-capture.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Preserve files changed by workflow-owned parallel step sessions on task branches. +category: fix +dev: Step-session cherry-pick now uses merge-base ranges and skips empty cherry-picks instead of dropping real step commits. diff --git a/packages/core/src/__tests__/store-update-step-order.test.ts b/packages/core/src/__tests__/store-update-step-order.test.ts index f70dbfe2e5..de5b0deb4d 100644 --- a/packages/core/src/__tests__/store-update-step-order.test.ts +++ b/packages/core/src/__tests__/store-update-step-order.test.ts @@ -63,6 +63,7 @@ describe("TaskStore.updateStep step-order guard", () => { // runnable; TaskStore must not invent a hidden previous-step dependency. const store = harness.store(); const task = await harness.createTaskWithSteps(); + await store.updateStep(task.id, 0, "pending"); await store.updateStep(task.id, 1, "in-progress", { source: "graph" }); const updated = await store.updateStep(task.id, 2, "done", { source: "graph" }); @@ -75,7 +76,9 @@ describe("TaskStore.updateStep step-order guard", () => { it("graph source: explicit dependsOn still suppresses completion until dependencies finish", async () => { const store = harness.store(); const task = await harness.createTaskWithSteps(); - const steps = task.steps.map((s, i) => (i === 2 ? { ...s, dependsOn: [1] } : { ...s })); + await store.updateStep(task.id, 0, "pending"); + const primed = await store.getTask(task.id); + const steps = primed.steps.map((s, i) => (i === 2 ? { ...s, dependsOn: [1] } : { ...s })); await store.updateTask(task.id, { steps }); await store.updateStep(task.id, 1, "in-progress", { source: "graph" }); @@ -90,7 +93,9 @@ describe("TaskStore.updateStep step-order guard", () => { it("graph source: out-of-order done (unmet dependency) is suppressed AND audited loudly", async () => { const store = harness.store(); const task = await harness.createTaskWithSteps(); - const steps = task.steps.map((s, i) => (i === 1 ? { ...s, dependsOn: [0] } : { ...s })); + await store.updateStep(task.id, 0, "pending"); + const primed = await store.getTask(task.id); + const steps = primed.steps.map((s, i) => (i === 1 ? { ...s, dependsOn: [0] } : { ...s })); await store.updateTask(task.id, { steps }); await store.updateStep(task.id, 1, "in-progress"); diff --git a/packages/engine/src/__tests__/step-session-executor.test.ts b/packages/engine/src/__tests__/step-session-executor.test.ts index 0c8adb7536..e53750247e 100644 --- a/packages/engine/src/__tests__/step-session-executor.test.ts +++ b/packages/engine/src/__tests__/step-session-executor.test.ts @@ -719,7 +719,7 @@ Some freeform text without checkboxes.`; it("does not ask graph-owned step sessions to call task lifecycle tools", () => { const task = makeTaskDetail({ prompt: fullPrompt }); const result = buildStepPrompt(task, 1); - expect(result).toContain("the workflow graph records completion"); + expect(result).toContain("The workflow graph records step status, ordering, review, and completion."); expect(result).not.toContain("fn_task_done()"); }); @@ -1690,10 +1690,16 @@ describe("StepSessionExecutor", () => { const session = makeMockSession(); mockedCreateFnAgent.mockResolvedValue({ session } as any); - // Make git log return commits, but cherry-pick fails + // Make the merge-base bounded commit list return a step commit, but cherry-pick fails. mockedExecSync.mockImplementation((cmd: string) => { - if (typeof cmd === "string" && cmd.includes("git log")) { - return "abc123def Some commit"; + if (typeof cmd === "string" && cmd.includes("git rev-parse HEAD")) { + return "primary-head"; + } + if (typeof cmd === "string" && cmd.includes("git merge-base HEAD primary-head")) { + return "merge-base-sha"; + } + if (typeof cmd === "string" && cmd.includes("git rev-list --reverse merge-base-sha..HEAD")) { + return "abc123def"; } if (typeof cmd === "string" && cmd.includes("git cherry-pick") && !cmd.includes("--abort")) { throw new Error("Merge conflict"); @@ -1737,8 +1743,14 @@ describe("StepSessionExecutor", () => { mockedCreateFnAgent.mockResolvedValue({ session } as any); mockedExecSync.mockImplementation((cmd: string) => { - if (typeof cmd === "string" && cmd.includes("git log")) { - return "abc123def Some commit"; + if (typeof cmd === "string" && cmd.includes("git rev-parse HEAD")) { + return "primary-head"; + } + if (typeof cmd === "string" && cmd.includes("git merge-base HEAD primary-head")) { + return "merge-base-sha"; + } + if (typeof cmd === "string" && cmd.includes("git rev-list --reverse merge-base-sha..HEAD")) { + return "abc123def"; } if (typeof cmd === "string" && cmd.includes("git cherry-pick") && cmd.includes("--abort")) { throw new Error("abort failed"); @@ -1775,7 +1787,13 @@ describe("StepSessionExecutor", () => { }); mockedExecSync.mockImplementation((cmd: string) => { - if (cmd.includes("git log")) { + if (cmd.includes("git rev-parse HEAD")) { + return "primary-head"; + } + if (cmd.includes("git merge-base HEAD primary-head")) { + return "merge-base-sha"; + } + if (cmd.includes("git rev-list --reverse merge-base-sha..HEAD")) { return "abc123"; } if (cmd.includes("git cherry-pick") && cmd.includes("--abort")) { @@ -2022,7 +2040,13 @@ describe("StepSessionExecutor", () => { throw new Error("step 2 worktree failed"); } } - if (cmd.includes("git log")) { + if (cmd.includes("git rev-parse HEAD")) { + return "primary-head"; + } + if (cmd.includes("git merge-base HEAD primary-head")) { + return "merge-base-sha"; + } + if (cmd.includes("git rev-list --reverse merge-base-sha..HEAD")) { return "abc123"; } return ""; diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index fe4826f2db..8478a36b19 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -1678,20 +1678,16 @@ export class TaskExecutor { } if (task.paused === true || task.userPaused === true || globalPause) return; /* - * FNXC:WorkflowLifecycle 2026-06-29-00:57: + * FNXC:WorkflowLifecycle 2026-06-29-10:35: * A stale pause-abort marker must not survive into a fresh unpaused dispatch. - * FN-7225 showed graph-owned Plan Review and execution failures being logged - * as "engine pause/resume" even though the task row was not paused. Clear the - * volatile marker at dispatch entry so real workflow/execution failures keep - * their actual cause and do not loop through pause recovery. + * FN-7225/FN-7226 showed graph-owned execution failures being narrated as + * pause/resume cleanup even though the task row was not paused. Clear the + * volatile marker silently at dispatch entry so the task log names the real + * workflow failure (`step-execute`, parse, review, etc.) instead of implying + * the engine actually paused. */ this.clearPausedAborted(task.id); - await this.store.logEntry( - task.id, - "Cleared stale pause-abort marker before unpaused execution dispatch", - undefined, - this.getRunContextFor(task.id), - ).catch(() => undefined); + executorLog.log(`${task.id}: cleared stale pause-abort marker before unpaused execution dispatch`); } clearPauseAbortStateForManualRetry(taskId: string): void { diff --git a/packages/engine/src/step-runner.ts b/packages/engine/src/step-runner.ts index 2672b96d14..9549342f27 100644 --- a/packages/engine/src/step-runner.ts +++ b/packages/engine/src/step-runner.ts @@ -136,12 +136,11 @@ export async function runTaskStep( // 1. Projection: step → in-progress (KTD-7). updateStep's own guards apply. try { - await store.updateStep( - task.id, - stepIndex, - "in-progress", - opts.projectionSource ? { source: opts.projectionSource } : undefined, - ); + if (opts.projectionSource) { + await store.updateStep(task.id, stepIndex, "in-progress", { source: opts.projectionSource }); + } else { + await store.updateStep(task.id, stepIndex, "in-progress"); + } } catch (err) { executorLog.warn( `${task.id}: runTaskStep failed to mark step ${stepIndex} in-progress: ${errMsg(err)}`, @@ -175,12 +174,11 @@ export async function runTaskStep( if (result.success) { if (markDoneOnSuccess) { try { - await store.updateStep( - task.id, - stepIndex, - "done", - opts.projectionSource ? { source: opts.projectionSource } : undefined, - ); + if (opts.projectionSource) { + await store.updateStep(task.id, stepIndex, "done", { source: opts.projectionSource }); + } else { + await store.updateStep(task.id, stepIndex, "done"); + } } catch (err) { executorLog.warn( `${task.id}: runTaskStep failed to mark step ${stepIndex} done: ${errMsg(err)}`, diff --git a/packages/engine/src/step-session-executor.ts b/packages/engine/src/step-session-executor.ts index ae6672c1c6..3a30695a25 100644 --- a/packages/engine/src/step-session-executor.ts +++ b/packages/engine/src/step-session-executor.ts @@ -1525,11 +1525,24 @@ Follow instructions precisely and avoid unrelated changes.`, private async cherryPickCommits(stepIndex: number, worktreePath: string): Promise { const { worktreePath: primaryPath, taskDetail } = this.options; - // Get commits made in the parallel worktree since it was created + /* + * FNXC:WorkflowStepControl 2026-06-29-10:31: + * Parallel step-session worktrees must land their step commits back into the primary task worktree before modifiedFiles capture runs. Use the actual merge-base with the primary worktree HEAD; the old time/HEAD~10 range could include already-present ancestors, hit an empty cherry-pick first, and leave the task branch with no files changed. + */ let commits: string; try { + const { stdout: primaryHeadRaw } = await execAsync("git rev-parse HEAD", { + cwd: primaryPath, + encoding: "utf-8", + }); + const primaryHead = primaryHeadRaw.trim(); + const { stdout: mergeBaseRaw } = await execAsync(`git merge-base HEAD ${primaryHead}`, { + cwd: worktreePath, + encoding: "utf-8", + }); + const mergeBase = mergeBaseRaw.trim(); const { stdout } = await execAsync( - `git log --oneline --format="%H" HEAD...HEAD~10 --since="1 hour ago"`, + `git rev-list --reverse ${mergeBase}..HEAD`, { cwd: worktreePath, encoding: "utf-8" }, ); commits = stdout.trim(); @@ -1544,18 +1557,30 @@ Follow instructions precisely and avoid unrelated changes.`, return; } - const shas = commits.split("\n").filter(Boolean); + const shas = commits.split("\n").map((line) => line.trim()).filter(Boolean); stepExecLog.log( `Cherry-picking ${shas.length} commit(s) from step ${stepIndex} ` + `into primary worktree for task ${taskDetail.id}`, ); - for (const sha of shas.reverse()) { + for (const sha of shas) { try { await execAsync(`git cherry-pick "${sha}"`, { cwd: primaryPath, }); } catch (err) { + const errText = `${err instanceof Error ? err.message : String(err)} ${String((err as { stdout?: unknown; stderr?: unknown })?.stdout ?? "")} ${String((err as { stderr?: unknown })?.stderr ?? "")}`; + if ( + errText.includes("The previous cherry-pick is now empty") || + errText.includes("nothing to commit") || + errText.includes("is empty") + ) { + await execAsync("git cherry-pick --skip", { cwd: primaryPath }).catch(async () => { + await execAsync("git cherry-pick --abort", { cwd: primaryPath }).catch(() => undefined); + }); + stepExecLog.warn(`Skipped empty cherry-pick for step ${stepIndex}: ${sha}`); + continue; + } // Cherry-pick conflict — abort and log try { await execAsync("git cherry-pick --abort", { cwd: primaryPath });