fix(FN-7273): prevent stale step resume regressions
This commit is contained in:
7
.changeset/block-out-of-order-step-starts.md
Normal file
7
.changeset/block-out-of-order-step-starts.md
Normal file
@@ -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.
|
||||
@@ -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();
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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 = {
|
||||
|
||||
@@ -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<number, number>();
|
||||
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<number>();
|
||||
|
||||
for (const line of logOutput.split("\n")) {
|
||||
// git log --oneline format: "<sha> <message>"
|
||||
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)`,
|
||||
|
||||
Reference in New Issue
Block a user