diff --git a/.changeset/block-out-of-order-step-starts.md b/.changeset/block-out-of-order-step-starts.md new file mode 100644 index 0000000000..df35ea836d --- /dev/null +++ b/.changeset/block-out-of-order-step-starts.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Prevent workflow task cards from showing later sequential steps active too early. +category: fix +dev: TaskStore now applies step dependency/order guards to in-progress updates as well as done updates. 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 8362137353..a4e75adadd 100644 --- a/packages/core/src/__tests__/store-update-step-order.test.ts +++ b/packages/core/src/__tests__/store-update-step-order.test.ts @@ -55,6 +55,19 @@ describe("TaskStore.updateStep step-order guard", () => { expect(updated.log.some((entry) => entry.action.includes("Ignored done→in-progress regression"))).toBe(true); }); + it("no-ops out-of-order in-progress updates while an earlier step is active", async () => { + const store = harness.store(); + const task = await harness.createTaskWithSteps(); + + await store.updateStep(task.id, 0, "in-progress"); + const updated = await store.updateStep(task.id, 2, "in-progress"); + + expect(updated.steps[0].status).toBe("in-progress"); + expect(updated.steps[2].status).toBe("pending"); + expect(updated.currentStep).toBe(0); + expect(updated.log.some((entry) => entry.action.includes("Ignored out-of-order in-progress for step 2"))).toBe(true); + }); + // ── U6: graph-source projection discipline (KTD-7/KTD-11) ────────────────── it("graph source: done is legal for explicitly independent steps even when an earlier step is in-progress", async () => { @@ -65,6 +78,7 @@ describe("TaskStore.updateStep step-order guard", () => { const steps = primed.steps.map((s, i) => (i === 2 ? { ...s, dependsOn: [] } : { ...s })); await store.updateTask(task.id, { steps }); + await store.updateStep(task.id, 0, "done", { source: "graph" }); await store.updateStep(task.id, 1, "in-progress", { source: "graph" }); const updated = await store.updateStep(task.id, 2, "done", { source: "graph" }); @@ -73,6 +87,22 @@ describe("TaskStore.updateStep step-order guard", () => { expect(updated.log.some((e) => e.action.includes("Ignored out-of-order done for step 2"))).toBe(false); }); + it("graph source: in-progress is legal for explicitly independent steps", async () => { + const store = harness.store(); + const task = await harness.createTaskWithSteps(); + const primed = await store.getTask(task.id); + const steps = primed.steps.map((s, i) => (i === 2 ? { ...s, dependsOn: [] } : { ...s })); + await store.updateTask(task.id, { steps }); + + await store.updateStep(task.id, 0, "in-progress", { source: "graph" }); + const updated = await store.updateStep(task.id, 2, "in-progress", { source: "graph" }); + + expect(updated.steps[0].status).toBe("in-progress"); + expect(updated.steps[2].status).toBe("in-progress"); + expect(updated.currentStep).toBe(2); + expect(updated.log.some((e) => e.action.includes("Ignored dependency-order in-progress for step 2"))).toBe(false); + }); + it("graph source: missing dependsOn defaults to previous step and blocks early verification", async () => { const store = harness.store(); const task = await harness.createTaskWithSteps(); diff --git a/packages/core/src/store.ts b/packages/core/src/store.ts index a78bcc2a2f..bb65585532 100644 --- a/packages/core/src/store.ts +++ b/packages/core/src/store.ts @@ -9388,15 +9388,18 @@ ${TASK_UPSERT_SQL_ASSIGNMENTS} return task; } - if (status === "done") { - // The set of predecessor steps that must be done/skipped before this step - // may go done. Legacy: strict index order (every earlier step). Graph: - // the step's dependsOn list, with absent dependsOn defaulting to the - // immediately-preceding step. A deliberately empty dependsOn array is the - // opt-in for an independent graph step. + if (status === "done" || status === "in-progress") { + // The set of predecessor steps that must be done/skipped before this + // step may start or finish. Legacy: strict index order (every earlier + // step). Graph: the step's dependsOn list, with absent dependsOn + // defaulting to the immediately-preceding step. A deliberately empty + // dependsOn array is the opt-in for an independent graph step. /* FNXC:WorkflowStepControl 2026-06-29-10:51: Graph-owned execution may complete explicitly independent steps out of index order, but unannotated task plans are sequential by default. FN-7228 showed Testing & Verification starting while Preflight/implementation were still active because step-session planning treated missing dependencies as independent. Keep TaskStore projection consistent with the graph scheduler: absent dependsOn means previous-step dependency; explicit dependsOn: [] means independent. + + FNXC:WorkflowStepControl 2026-06-30-07:45: + FN-7260 showed the same ordering invariant can be broken earlier by agent-visible progress updates: a stale resume prompt told the executor to start Step 3 while Step 0 was still in progress, and TaskStore accepted the out-of-order `in-progress` write. Apply the predecessor/dependency gate to step start as well as step completion so the card, task detail, and executor prompt cannot advertise later sequential work before earlier steps finish. */ let blockingIndex = -1; let blockingStatus: import("./types.js").StepStatus | undefined; 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 index b898838cbc..2b0457770b 100644 --- a/packages/engine/src/__tests__/executor-step-numbering-zero-based.test.ts +++ b/packages/engine/src/__tests__/executor-step-numbering-zero-based.test.ts @@ -5,6 +5,7 @@ import { reviewStep as mockedReviewStepFn } from "../reviewer.js"; import { createMockStore, mockedCreateFnAgent, + mockedExecSync, mockedExistsSync, resetExecutorMocks, } from "./executor-test-helpers.js"; @@ -151,6 +152,96 @@ describe("executor tool step numbering is 0-based", () => { ); }); + it("does not reconcile reopened steps from older complete-step commits", async () => { + const store = createMockStore(); + const detail = { + id: "FN-7273", + title: "Reopened suffix", + description: "", + column: "in-progress", + dependencies: [], + baseCommitSha: "base", + steps: [ + { name: "Preflight", status: "done" }, + { name: "Implementation", status: "done" }, + { name: "Testing", status: "pending" }, + ], + currentStep: 2, + log: [ + { timestamp: "2026-06-30T14:59:30.110Z", action: "Step 2 (Testing) → pending" }, + ], + prompt: "# test\n## Steps\n### Step 0: Preflight\n### Step 1: Implementation\n### Step 2: Testing", + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + } as any; + store.getTask.mockResolvedValue(detail); + mockedExecSync.mockImplementation((cmd: string) => { + if (cmd.includes("git log")) { + return "1782831500\tfeat(FN-7273): complete Step 2 — old verification\n"; + } + return ""; + }); + + const executor = new TaskExecutor(store as any, "/tmp/test"); + await (executor as any).reconcileStepsFromGitHistory("FN-7273", detail, "/tmp/wt"); + + expect(store.updateStep).not.toHaveBeenCalled(); + expect(store.logEntry).not.toHaveBeenCalledWith( + "FN-7273", + expect.stringContaining("Reconciled Step 2 as done from git history"), + expect.anything(), + expect.anything(), + ); + }); + + it("does not log git-history reconciliation when TaskStore rejects the done write", async () => { + const store = createMockStore(); + const detail = { + id: "FN-7273", + title: "Out of order reconciliation", + description: "", + column: "in-progress", + dependencies: [], + baseCommitSha: "base", + steps: [ + { name: "Preflight", status: "done" }, + { name: "Fix", status: "in-progress" }, + { name: "Delivery", status: "pending" }, + ], + currentStep: 1, + log: [], + prompt: "# test\n## Steps\n### Step 0: Preflight\n### Step 1: Fix\n### Step 2: Delivery", + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + } as any; + store.getTask.mockResolvedValue(detail); + store.updateStep.mockResolvedValue({ + ...detail, + steps: [ + { name: "Preflight", status: "done" }, + { name: "Fix", status: "in-progress" }, + { name: "Delivery", status: "pending" }, + ], + } as any); + mockedExecSync.mockImplementation((cmd: string) => { + if (cmd.includes("git log")) { + return "1782832000\tfeat(FN-7273): complete Step 2 — old delivery\n"; + } + return ""; + }); + + const executor = new TaskExecutor(store as any, "/tmp/test"); + await (executor as any).reconcileStepsFromGitHistory("FN-7273", detail, "/tmp/wt"); + + expect(store.updateStep).toHaveBeenCalledWith("FN-7273", 2, "done"); + expect(store.logEntry).not.toHaveBeenCalledWith( + "FN-7273", + expect.stringContaining("Reconciled Step 2 as done from git history"), + expect.anything(), + expect.anything(), + ); + }); + it("pending-review loop detection matches 0-based writer strings", async () => { const store = createMockStore(); const task = { diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 007c086775..d84e43c181 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -16352,7 +16352,7 @@ You have access to the file system to review changes.${verdictBlock}`; let logOutput: string; try { const { stdout } = await execAsync( - `git log "${baseCommitSha}..HEAD" --oneline`, + `git log "${baseCommitSha}..HEAD" --format=%ct%x09%s`, { cwd: worktreePath }, ); logOutput = stdout; @@ -16364,17 +16364,35 @@ You have access to the file system to review changes.${verdictBlock}`; if (!logOutput.trim()) return; + const latestPendingByStep = new Map(); + for (const entry of detail.log ?? []) { + const action = entry.action ?? ""; + const match = action.match(/^Step (\d+) \(.+\) → pending$/); + if (!match) continue; + const stepIndex = Number.parseInt(match[1], 10); + const pendingAt = Date.parse(entry.timestamp); + if (!Number.isInteger(stepIndex) || !Number.isFinite(pendingAt)) continue; + latestPendingByStep.set(stepIndex, Math.max(latestPendingByStep.get(stepIndex) ?? -1, pendingAt)); + } + + /* + FNXC:WorkflowResume 2026-06-30-08:02: + Browser Verification and Code Review REVISE intentionally reopen the trailing implementation/verification suffix. FN-7273 showed git-history resume then found older `complete Step 5` commits from the previous attempt, tried to mark Step 5 done while Step 3 was active, and logged a false reconciliation after TaskStore rejected the out-of-order write. A reopened step may only be reconciled from a commit whose author time is newer than the latest `→ pending` transition for that step, and success is logged only after the store confirms the step is terminal. + */ // Match: feat(FN-2978): complete Step 3 / chore(fn-2978)!: Complete step 3 const stepCommitRegex = /^(?:feat|chore|fix)\([Ff][Nn]-\d+\)(?:!)?:\s*complete\s+step\s+(\d+)/i; const reconciledStepIndices = new Set(); for (const line of logOutput.split("\n")) { - // git log --oneline format: " " - const message = line.replace(/^[0-9a-f]+ /, "").trim(); + const [commitSecondsRaw, ...messageParts] = line.split("\t"); + const commitMs = Number.parseInt(commitSecondsRaw ?? "", 10) * 1000; + const message = messageParts.join("\t").trim(); const match = message.match(stepCommitRegex); if (!match) continue; const stepIndex = parseInt(match[1], 10); if (Number.isNaN(stepIndex) || stepIndex < 0 || stepIndex >= detail.steps.length) continue; + const latestPendingAt = latestPendingByStep.get(stepIndex); + if (latestPendingAt !== undefined && (!Number.isFinite(commitMs) || commitMs <= latestPendingAt)) continue; const step = detail.steps[stepIndex]; if (step.status === "pending" || step.status === "in-progress") { reconciledStepIndices.add(stepIndex); @@ -16382,7 +16400,14 @@ You have access to the file system to review changes.${verdictBlock}`; } for (const stepIndex of reconciledStepIndices) { - await this.store.updateStep(taskId, stepIndex, "done"); + const updated = await this.store.updateStep(taskId, stepIndex, "done"); + const updatedStepStatus = updated.steps?.[stepIndex]?.status; + if (updatedStepStatus !== "done" && updatedStepStatus !== "skipped") { + executorLog.warn( + `${taskId}: skipped git-history reconciliation log for Step ${stepIndex}; store kept status ${updatedStepStatus ?? "missing"}`, + ); + continue; + } await this.store.logEntry( taskId, `Reconciled Step ${stepIndex} as done from git history (resume)`,