From 3d03505a62fa20ad8ba2b4d79a50d39d9bb61321 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 4 Jun 2026 11:50:45 -0700 Subject: [PATCH] =?UTF-8?q?feat(engine):=20U2=20=E2=80=94=20runTaskStep/re?= =?UTF-8?q?setStepToBaseline=20substrate=20seams=20with=20blast-radius=20g?= =?UTF-8?q?uard=20(RETHINK=20extraction)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.8 (1M context) --- .../__tests__/executor-step-session.test.ts | 130 ++++++ .../engine/src/__tests__/step-runner.test.ts | 397 ++++++++++++++++++ packages/engine/src/executor.ts | 71 +--- packages/engine/src/index.ts | 15 + packages/engine/src/step-runner.ts | 375 +++++++++++++++++ 5 files changed, 938 insertions(+), 50 deletions(-) create mode 100644 packages/engine/src/__tests__/step-runner.test.ts create mode 100644 packages/engine/src/step-runner.ts diff --git a/packages/engine/src/__tests__/executor-step-session.test.ts b/packages/engine/src/__tests__/executor-step-session.test.ts index dd7ba60762..9fee7cdc53 100644 --- a/packages/engine/src/__tests__/executor-step-session.test.ts +++ b/packages/engine/src/__tests__/executor-step-session.test.ts @@ -3583,3 +3583,133 @@ describe("TaskExecutor loop recovery", () => { // ── Context limit error recovery tests ──────────────────────────────── +// ── U2 RETHINK delegation characterization (plan 2026-06-04-001, KTD-2) ── +// +// The legacy in-session fn_review_step RETHINK case now DELEGATES to +// step-runner.ts's resetStepToBaseline. These tests pin that the observable +// side effects are byte-identical to the pre-extraction block: git reset to +// the agent-supplied baseline, session rewind via navigateTree, step→pending, +// and the RETHINK log entry — all reached through the real executor session. +describe("U2: fn_review_step RETHINK delegates to resetStepToBaseline (characterization)", () => { + beforeEach(() => { + resetExecutorMocks(); + }); + + function runRethinkScenario(reviewType: "code" | "plan", navigateTree: any) { + const store = createMockStore(); + const baseTask = { + id: "FN-RT-1", + title: "Test", + description: "Test task", + column: "in-progress", + dependencies: [], + steps: [{ name: "Implement", status: "in-progress" }], + currentStep: 0, + log: [], + prompt: "# test\n## Steps\n### Step 1: Implement\n- [ ] implement", + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + }; + store.getTask.mockResolvedValue(baseTask as any); + // updateStep returns the task with the step persisted in-progress so the + // executor's checkpoint-capture path (executor.ts ~6517) populates the + // stepCheckpoints map that RETHINK rewinds to. + store.updateStep.mockResolvedValue({ + ...baseTask, + steps: [{ name: "Implement", status: "in-progress" }], + } as any); + + mockedReviewStep.mockResolvedValue({ + verdict: "RETHINK", + review: "wrong approach", + summary: "rejected approach", + } as any); + + let reviewToolError: unknown; + mockedCreateFnAgent.mockImplementation((async (opts: any) => { + const tools = opts.customTools || []; + return { + session: { + prompt: vi.fn().mockImplementation(async () => { + // First, flip the step to in-progress via fn_task_update so the + // checkpoint map is populated (mirrors the real session lifecycle). + const updateTool = tools.find((t: any) => t.name === "fn_task_update"); + if (updateTool) { + try { + await updateTool.execute("tool-update", { step: 1, 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, + type: reviewType, + step_name: "Implement", + baseline: reviewType === "code" ? "agentBaselineSHA" : undefined, + }); + } catch (e) { + reviewToolError = e; + } + } + }), + dispose: vi.fn(), + subscribe: vi.fn(), + on: vi.fn(), + navigateTree, + sessionManager: { + getLeafId: vi.fn().mockReturnValue("leaf-pre-step"), + branchWithSummary: vi.fn(), + }, + state: {}, + }, + }; + }) as any); + + const executor = new TaskExecutor(store, "/tmp/test", {}); + return { store, baseTask, executor, getReviewToolError: () => reviewToolError }; + } + + it("code RETHINK: git reset to baseline, navigateTree rewind, step→pending, RETHINK log", async () => { + const navigateTree = vi.fn().mockResolvedValue(undefined); + const { store, baseTask, executor } = runRethinkScenario("code", navigateTree); + + await executor.execute(baseTask as any); + + // git reset --hard issued in the worktree (via the mocked exec). + const resetIssued = mockedExecSync.mock.calls.some( + (c) => typeof c[0] === "string" && (c[0] as string).includes("git reset --hard agentBaselineSHA"), + ); + expect(resetIssued).toBe(true); + // Session rewound to the captured pre-step checkpoint. + expect(navigateTree).toHaveBeenCalledWith("leaf-pre-step", { summarize: false }); + // Step reset to pending through the projection sink. + expect(store.updateStep).toHaveBeenCalledWith("FN-RT-1", 0, "pending"); + // RETHINK log entry (code-review variant references the git reset). + expect(store.logEntry).toHaveBeenCalledWith( + "FN-RT-1", + expect.stringContaining("git reset to agentBaselineSHA"), + "rejected approach", + ); + }); + + it("plan RETHINK: no git reset, navigateTree rewind, step→pending, plan-rewound log", async () => { + const navigateTree = vi.fn().mockResolvedValue(undefined); + const { store, baseTask, executor } = runRethinkScenario("plan", navigateTree); + + await executor.execute(baseTask as any); + + const resetIssued = mockedExecSync.mock.calls.some( + (c) => typeof c[0] === "string" && (c[0] as string).includes("git reset --hard"), + ); + expect(resetIssued).toBe(false); + expect(navigateTree).toHaveBeenCalledWith("leaf-pre-step", { summarize: false }); + expect(store.updateStep).toHaveBeenCalledWith("FN-RT-1", 0, "pending"); + expect(store.logEntry).toHaveBeenCalledWith( + "FN-RT-1", + expect.stringContaining("Step 1 plan rewound"), + "rejected approach", + ); + }); +}); + diff --git a/packages/engine/src/__tests__/step-runner.test.ts b/packages/engine/src/__tests__/step-runner.test.ts new file mode 100644 index 0000000000..25c885048f --- /dev/null +++ b/packages/engine/src/__tests__/step-runner.test.ts @@ -0,0 +1,397 @@ +/** + * Unit tests for the U2 substrate seams (plan 2026-06-04-001, KTD-2): + * - runTaskStep — per-step driver over step-session physics. + * - resetStepToBaseline — verbatim RETHINK mechanics + blast-radius guard. + * + * Fast tests: real git / sessions / StepSessionExecutor are never touched — + * every external is injected via the explicit `deps` object (FN-5048 fake-timer + * convention is moot here since the seams take no clock). The executor's + * delegation of the legacy RETHINK block is characterized separately in + * executor-step-session.test.ts. + */ + +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { + runTaskStep, + resetStepToBaseline, + makeAncestryBlastRadiusGuard, + type StepRunnerTask, + type SessionRef, +} from "../step-runner.js"; + +function makeStore() { + return { + updateStep: vi.fn().mockResolvedValue(undefined), + logEntry: vi.fn().mockResolvedValue(undefined), + }; +} + +function makeTask(steps: Array<{ name?: string; status?: string }>): StepRunnerTask { + return { id: "FN-001", steps }; +} + +function makeSessionRef(opts?: { + navigateTree?: ReturnType; + branchWithSummary?: ReturnType; + leafId?: string; +}): SessionRef { + const navigateTree = opts?.navigateTree ?? vi.fn().mockResolvedValue(undefined); + const branchWithSummary = opts?.branchWithSummary ?? vi.fn(); + return { + current: { + navigateTree, + sessionManager: { + branchWithSummary, + getLeafId: vi.fn().mockReturnValue(opts?.leafId ?? "leaf-pre-step"), + }, + } as unknown as SessionRef["current"], + }; +} + +describe("runTaskStep", () => { + beforeEach(() => vi.clearAllMocks()); + + it("marks the step in-progress then done on success, capturing baseline + checkpoint", async () => { + const store = makeStore(); + const task = makeTask([{ name: "Implement", status: "pending" }]); + const gitRevParse = vi.fn().mockResolvedValue("baseSHA123"); + const captureCheckpointId = vi.fn().mockReturnValue("leaf-pre-step"); + const runStep = vi.fn().mockResolvedValue({ success: true }); + + const result = await runTaskStep( + { store, worktreePath: "/wt", runStep, gitRevParse, captureCheckpointId }, + task, + 0, + ); + + expect(result).toEqual({ outcome: "success", baselineSha: "baseSHA123", checkpointId: "leaf-pre-step" }); + // Baseline is captured BEFORE the step runs. + expect(gitRevParse).toHaveBeenCalledWith("/wt"); + expect(runStep).toHaveBeenCalledWith(0); + // Projection ordering: in-progress before done. + expect(store.updateStep.mock.calls).toEqual([ + ["FN-001", 0, "in-progress"], + ["FN-001", 0, "done"], + ]); + }); + + it("captures the baseline before running the step (order check)", async () => { + const store = makeStore(); + const order: string[] = []; + const gitRevParse = vi.fn().mockImplementation(async () => { + order.push("baseline"); + return "sha"; + }); + const runStep = vi.fn().mockImplementation(async () => { + order.push("run"); + return { success: true }; + }); + + await runTaskStep( + { store, worktreePath: "/wt", runStep, gitRevParse, captureCheckpointId: () => "leaf" }, + makeTask([{ status: "pending" }]), + 0, + ); + + expect(order).toEqual(["baseline", "run"]); + }); + + it("leaves the step non-done on failure (no 'done'/'skipped' write)", async () => { + const store = makeStore(); + const runStep = vi.fn().mockResolvedValue({ success: false, error: "boom" }); + + const result = await runTaskStep( + { + store, + worktreePath: "/wt", + runStep, + gitRevParse: async () => "baseSHA", + captureCheckpointId: () => "leaf", + }, + makeTask([{ status: "pending" }]), + 0, + ); + + expect(result).toEqual({ outcome: "failure", baselineSha: "baseSHA", checkpointId: "leaf" }); + // Only the in-progress write happened — the failed step is left non-done. + expect(store.updateStep.mock.calls).toEqual([["FN-001", 0, "in-progress"]]); + expect(store.updateStep).not.toHaveBeenCalledWith("FN-001", 0, "done"); + expect(store.updateStep).not.toHaveBeenCalledWith("FN-001", 0, "skipped"); + }); + + it("still returns a result when baseline capture fails (best-effort)", async () => { + const store = makeStore(); + const runStep = vi.fn().mockResolvedValue({ success: true }); + const gitRevParse = vi.fn().mockRejectedValue(new Error("not a git repo")); + + const result = await runTaskStep( + { store, worktreePath: "/wt", runStep, gitRevParse, captureCheckpointId: () => "leaf" }, + makeTask([{ status: "pending" }]), + 0, + ); + + expect(result.outcome).toBe("success"); + expect(result.baselineSha).toBeUndefined(); + expect(result.checkpointId).toBe("leaf"); + }); + + it("uses the default checkpoint capture from the session ref when none injected", async () => { + const store = makeStore(); + const sessionRef = makeSessionRef({ leafId: "leaf-xyz" }); + const result = await runTaskStep( + { + store, + worktreePath: "/wt", + runStep: async () => ({ success: true }), + gitRevParse: async () => "sha", + }, + makeTask([{ status: "pending" }]), + 0, + { sessionRef }, + ); + expect(result.checkpointId).toBe("leaf-xyz"); + }); +}); + +describe("resetStepToBaseline", () => { + beforeEach(() => vi.clearAllMocks()); + + it("does git reset + session rewind + step→pending with baseline and checkpoint (code review)", async () => { + const store = makeStore(); + const navigateTree = vi.fn().mockResolvedValue(undefined); + const sessionRef = makeSessionRef({ navigateTree }); + // We can't observe the real git command without mocking child_process; verify + // the session rewind + projection happen. (The git path is exercised through + // the executor characterization test.) + const result = await resetStepToBaseline( + { store, worktreePath: "/wt", sessionRef, reviewType: "code", summary: "rejected" }, + makeTask([{ status: "in-progress" }]), + 0, + "baseSHA", + "leaf-checkpoint", + ); + + expect(result).toEqual({ ok: true }); + expect(navigateTree).toHaveBeenCalledWith("leaf-checkpoint", { summarize: false }); + expect(store.updateStep).toHaveBeenCalledWith("FN-001", 0, "pending"); + expect(store.logEntry).toHaveBeenCalledWith( + "FN-001", + expect.stringContaining("git reset to baseSHA"), + "rejected", + ); + }); + + it("skips the session rewind when no checkpoint is provided (partial path)", async () => { + const store = makeStore(); + const navigateTree = vi.fn(); + const branchWithSummary = vi.fn(); + const sessionRef = makeSessionRef({ navigateTree, branchWithSummary }); + + const result = await resetStepToBaseline( + { store, worktreePath: "/wt", sessionRef, reviewType: "code" }, + makeTask([{ status: "in-progress" }]), + 0, + "baseSHA", + undefined, + ); + + expect(result.ok).toBe(true); + expect(navigateTree).not.toHaveBeenCalled(); + expect(branchWithSummary).not.toHaveBeenCalled(); + // Step still flips to pending. + expect(store.updateStep).toHaveBeenCalledWith("FN-001", 0, "pending"); + }); + + it("plan review skips git reset, logs the plan-rewound line, still flips pending", async () => { + const store = makeStore(); + const navigateTree = vi.fn().mockResolvedValue(undefined); + const sessionRef = makeSessionRef({ navigateTree }); + + const result = await resetStepToBaseline( + { store, worktreePath: "/wt", sessionRef, reviewType: "plan", summary: "plan rejected" }, + makeTask([{ status: "in-progress" }]), + 2, + undefined, + "leaf-checkpoint", + ); + + expect(result.ok).toBe(true); + expect(navigateTree).toHaveBeenCalledWith("leaf-checkpoint", { summarize: false }); + 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"), + "plan rejected", + ); + }); + + it("falls back to branchWithSummary when navigateTree throws", async () => { + const store = makeStore(); + const navigateTree = vi.fn().mockRejectedValue(new Error("navigate failed")); + const branchWithSummary = vi.fn(); + const sessionRef = makeSessionRef({ navigateTree, branchWithSummary }); + + const result = await resetStepToBaseline( + { store, worktreePath: "/wt", sessionRef, reviewType: "code", summary: "why" }, + makeTask([{ status: "in-progress" }]), + 0, + "baseSHA", + "leaf-checkpoint", + ); + + expect(result.ok).toBe(true); + expect(branchWithSummary).toHaveBeenCalledWith("leaf-checkpoint", "RETHINK: why"); + expect(store.updateStep).toHaveBeenCalledWith("FN-001", 0, "pending"); + }); + + // ── KTD-2 blast-radius guard refusal cases ────────────────────────────── + + it("REFUSES and mutates nothing when the guard reports a violation", async () => { + const store = makeStore(); + const navigateTree = vi.fn(); + const sessionRef = makeSessionRef({ navigateTree }); + const audit = { database: vi.fn().mockResolvedValue(undefined) }; + const blastRadiusGuard = vi.fn().mockResolvedValue("baseSHA is not an ancestor of HEAD"); + + const result = await resetStepToBaseline( + { store, worktreePath: "/wt", sessionRef, reviewType: "code", audit, blastRadiusGuard }, + makeTask([{ status: "in-progress" }]), + 0, + "baseSHA", + "leaf-checkpoint", + ); + + expect(result).toEqual({ ok: false, reason: "baseSHA is not an ancestor of HEAD" }); + // No mutation: no rewind, no updateStep, no RETHINK logEntry. + expect(navigateTree).not.toHaveBeenCalled(); + expect(store.updateStep).not.toHaveBeenCalled(); + expect(store.logEntry).not.toHaveBeenCalled(); + // Audit warning emitted (task:integrity-warning, database domain). + expect(audit.database).toHaveBeenCalledWith( + expect.objectContaining({ + type: "task:integrity-warning", + target: "FN-001", + metadata: expect.objectContaining({ + guard: "step-reset-blast-radius", + reason: "baseSHA is not an ancestor of HEAD", + }), + }), + ); + }); + + it("fails closed (refuses) when the guard itself throws", async () => { + const store = makeStore(); + const sessionRef = makeSessionRef(); + const blastRadiusGuard = vi.fn().mockRejectedValue(new Error("git exploded")); + + const result = await resetStepToBaseline( + { store, worktreePath: "/wt", sessionRef, reviewType: "code", blastRadiusGuard }, + makeTask([{ status: "in-progress" }]), + 0, + "baseSHA", + "leaf", + ); + + expect(result.ok).toBe(false); + expect(result.reason).toContain("git exploded"); + expect(store.updateStep).not.toHaveBeenCalled(); + }); + + it("proceeds with the reset when the guard returns null (safe)", async () => { + const store = makeStore(); + const navigateTree = vi.fn().mockResolvedValue(undefined); + const sessionRef = makeSessionRef({ navigateTree }); + const blastRadiusGuard = vi.fn().mockResolvedValue(null); + + const result = await resetStepToBaseline( + { store, worktreePath: "/wt", sessionRef, reviewType: "code", blastRadiusGuard }, + makeTask([{ status: "in-progress" }]), + 0, + "baseSHA", + "leaf-checkpoint", + ); + + expect(result.ok).toBe(true); + expect(navigateTree).toHaveBeenCalledWith("leaf-checkpoint", { summarize: false }); + expect(store.updateStep).toHaveBeenCalledWith("FN-001", 0, "pending"); + }); +}); + +describe("makeAncestryBlastRadiusGuard", () => { + beforeEach(() => vi.clearAllMocks()); + + it("refuses when a LATER step is already done", async () => { + const guard = makeAncestryBlastRadiusGuard({ + worktreePath: "/wt", + task: makeTask([{ status: "in-progress" }, { status: "done" }]), + stepIndex: 0, + isAncestor: async () => true, + }); + const reason = await guard("baseSHA"); + expect(reason).toContain("later step 1 is done"); + }); + + it("refuses when a LATER step is already skipped", async () => { + const guard = makeAncestryBlastRadiusGuard({ + worktreePath: "/wt", + task: makeTask([{ status: "in-progress" }, { status: "skipped" }]), + stepIndex: 0, + isAncestor: async () => true, + }); + const reason = await guard("baseSHA"); + expect(reason).toContain("later step 1 is skipped"); + }); + + it("refuses when the baseline is NOT an ancestor of HEAD", async () => { + const guard = makeAncestryBlastRadiusGuard({ + worktreePath: "/wt", + task: makeTask([{ status: "in-progress" }]), + stepIndex: 0, + isAncestor: async () => false, + }); + const reason = await guard("baseSHA"); + expect(reason).toContain("not an ancestor of HEAD"); + }); + + it("allows when baseline is an ancestor and no later step is terminal", async () => { + const isAncestor = vi.fn().mockResolvedValue(true); + const guard = makeAncestryBlastRadiusGuard({ + worktreePath: "/wt", + task: makeTask([ + { status: "pending" }, + { status: "in-progress" }, + { status: "pending" }, + ]), + stepIndex: 1, + isAncestor, + }); + const reason = await guard("baseSHA"); + expect(reason).toBeNull(); + expect(isAncestor).toHaveBeenCalledWith("baseSHA", "/wt"); + }); + + it("allows (skipping ancestry) when no baseline is supplied", async () => { + const isAncestor = vi.fn(); + const guard = makeAncestryBlastRadiusGuard({ + worktreePath: "/wt", + task: makeTask([{ status: "in-progress" }]), + stepIndex: 0, + isAncestor, + }); + const reason = await guard(undefined); + expect(reason).toBeNull(); + expect(isAncestor).not.toHaveBeenCalled(); + }); + + it("treats an earlier done step as harmless (only LATER steps matter)", async () => { + const guard = makeAncestryBlastRadiusGuard({ + worktreePath: "/wt", + task: makeTask([{ status: "done" }, { status: "in-progress" }]), + stepIndex: 1, + isAncestor: async () => true, + }); + const reason = await guard("baseSHA"); + expect(reason).toBeNull(); + }); +}); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index c0013945d0..c94f134d78 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -103,6 +103,7 @@ import type { StuckTaskDetector, StuckTaskEvent } from "./stuck-task-detector.js import type { PluginRunner } from "./plugin-runner.js"; import { isContextLimitError } from "./context-limit-detector.js"; import { StepSessionExecutor } from "./step-session-executor.js"; +import { resetStepToBaseline } from "./step-runner.js"; import { acquireTaskWorktree } from "./worktree-acquisition.js"; import { resolveCapturedBaseCommitSha } from "./base-commit-capture.js"; import { installTaskWorktreeIdentityGuard } from "./worktree-hooks.js"; @@ -7453,61 +7454,31 @@ export class TaskExecutor { } break; case "RETHINK": { - // For code reviews: git reset to baseline to revert file changes - // For plan reviews: skip git reset (no code has been written yet) - if (reviewType === "code" && baseline) { - try { - await execAsync(`git reset --hard ${baseline}`, { cwd: worktreePath }); - executorLog.log(`${taskId}: RETHINK — git reset --hard ${baseline}`); - } catch (gitErr: unknown) { - const gitErrMessage = gitErr instanceof Error ? gitErr.message : String(gitErr); - executorLog.error(`${taskId}: RETHINK git reset failed: ${gitErrMessage}`); - } - } else if (reviewType === "code") { - executorLog.log(`${taskId}: RETHINK — no baseline SHA, skipping git reset`); - } - - // Rewind conversation to pre-step checkpoint + // RETHINK mechanics (git reset to baseline + session rewind + + // step→pending + RETHINK log entry) are the U2 substrate seam. + // The legacy in-session path delegates to the single extracted + // implementation in step-runner.ts so there is exactly one copy. + // No blast-radius guard here: this path is intra-session with an + // agent-supplied baseline (KTD-2 — the guard is for graph-owned + // shared-isolation resets), so behavior stays byte-identical. const checkpointId = stepCheckpoints.get(stepIndex); - if (checkpointId && sessionRef.current) { - try { - await sessionRef.current.navigateTree(checkpointId, { summarize: false }); - executorLog.log(`${taskId}: RETHINK — session rewound to checkpoint ${checkpointId}`); - } catch (rewindErr: unknown) { - const msg = rewindErr instanceof Error ? rewindErr.message : String(rewindErr); - executorLog.warn(`${taskId}: RETHINK navigateTree rewind failed, falling back to branchWithSummary: ${msg}`); - // Fallback to branchWithSummary - try { - sessionRef.current.sessionManager.branchWithSummary( - checkpointId, - `RETHINK: ${result.summary || "Approach rejected by reviewer"}`, - ); - executorLog.log(`${taskId}: RETHINK — branched from checkpoint ${checkpointId}`); - } catch (branchErr: unknown) { - const branchErrMessage = branchErr instanceof Error ? branchErr.message : String(branchErr); - executorLog.error(`${taskId}: RETHINK session rewind failed: ${branchErrMessage}`); - } - } - } else { - executorLog.log(`${taskId}: RETHINK — no session checkpoint for step ${step}, skipping rewind`); - } - - // Reset step status to pending - await store.updateStep(taskId, stepIndex, "pending"); + await resetStepToBaseline( + { + store, + worktreePath, + sessionRef, + reviewType: reviewType === "plan" ? "plan" : "code", + summary: result.summary, + }, + { id: taskId, steps: taskSteps }, + stepIndex, + reviewType === "code" ? baseline : undefined, + checkpointId, + ); if (reviewType === "plan") { - await store.logEntry( - taskId, - `RETHINK: Step ${step} plan rewound — session checkpoint ${checkpointId || "N/A"}`, - result.summary, - ); text = `RETHINK\n\nYour plan was rejected. Here is why:\n\n${result.review}\n\nTake a different approach to planning this step. Do NOT repeat the rejected strategy.`; } else { - await store.logEntry( - taskId, - `RETHINK: Step ${step} rewound — git reset to ${baseline || "N/A"}, session checkpoint ${checkpointId || "N/A"}`, - result.summary, - ); text = `RETHINK\n\nYour previous approach was rejected. Here is why:\n\n${result.review}\n\nTake a different approach. Do NOT repeat the rejected strategy. Re-read the step requirements and find an alternative solution.`; } break; diff --git a/packages/engine/src/index.ts b/packages/engine/src/index.ts index 9eb88b2391..b60c72874e 100644 --- a/packages/engine/src/index.ts +++ b/packages/engine/src/index.ts @@ -554,6 +554,21 @@ export { } from "./hold-release.js"; export { StepSessionExecutor } from "./step-session-executor.js"; export type { StepResult, ParallelWave, StepSessionExecutorOptions } from "./step-session-executor.js"; +export { + runTaskStep, + resetStepToBaseline, + makeAncestryBlastRadiusGuard, +} from "./step-runner.js"; +export type { + RunTaskStepDeps, + RunTaskStepOptions, + RunTaskStepResult, + ResetStepDeps, + ResetStepResult, + RunSingleStep, + SessionRef, + StepRunnerTask, +} from "./step-runner.js"; // Multi-project runtime types export { type ProjectRuntime, diff --git a/packages/engine/src/step-runner.ts b/packages/engine/src/step-runner.ts new file mode 100644 index 0000000000..d6d6d54da9 --- /dev/null +++ b/packages/engine/src/step-runner.ts @@ -0,0 +1,375 @@ +/** + * step-runner — the two substrate seams for graph-owned stepwise execution + * (plan 2026-06-04-001, KTD-2 / U2). + * + * This module exposes exactly two capabilities that the workflow-graph executor + * (U3/U5) will drive — it does NOT wire itself into any graph path here: + * + * - {@link runTaskStep} — run exactly step `i` of a task inside its + * session/worktree and return the outcome plus + * the per-step `baselineSha` / `checkpointId` + * that a later RETHINK needs. + * - {@link resetStepToBaseline} — the RETHINK mechanics, extracted verbatim + * from `executor.ts`'s `fn_review_step` RETHINK + * block (`git reset --hard ` + session + * rewind via `navigateTree`/`branchWithSummary` + * fallback + `store.updateStep(..., "pending")`), + * plus a defensive blast-radius guard (KTD-2). + * + * Both are parameterized via an explicit `deps` object (the DI style used by + * `hold-release.ts` / `merge-trait.ts`) so they stay unit-testable without real + * git, real sessions, or a real `StepSessionExecutor`. Production callers (U3/U5) + * pass thin adapters over the existing engine machinery; the legacy in-session + * `fn_review_step` path is untouched and keeps its own copy's behavior — this + * extraction is the single implementation the executor's RETHINK block now + * delegates to (see `TaskExecutor.applyStepRethink`). + */ + +import { exec } from "node:child_process"; +import { promisify } from "node:util"; + +import type { TaskStore } from "@fusion/core"; + +const execAsync = promisify(exec); + +import type { AgentSession as PiAgentSession } from "@earendil-works/pi-coding-agent"; +import { executorLog } from "./logger.js"; +import type { RunAuditor } from "./run-audit.js"; + +// ── Shared minimal shapes ─────────────────────────────────────────────── + +/** The slice of `Task` the step runner reads. */ +export interface StepRunnerTask { + id: string; + steps: Array<{ name?: string; status?: string }>; +} + +/** A minimal session ref mirroring the executor's `{ current: AgentSession }`. */ +export interface SessionRef { + current: PiAgentSession | null; +} + +/** + * Run exactly one step inside the task's session/worktree. Production wires this + * to a {@link import("./step-session-executor.js").StepSessionExecutor} configured + * for a single step (graph-owned runs force step-session physics, KTD-2/KTD-8); + * tests inject a fake. Returns whether the step's session completed successfully. + */ +export type RunSingleStep = (stepIndex: number) => Promise<{ success: boolean; error?: string }>; + +// ── runTaskStep ───────────────────────────────────────────────────────── + +/** Dependencies for {@link runTaskStep}. */ +export interface RunTaskStepDeps { + /** Step-state projection sink (KTD-7). */ + store: Pick; + /** Absolute path to the task's worktree (where `git rev-parse HEAD` runs). */ + worktreePath: string; + /** Run exactly step `i` (step-session physics). */ + runStep: RunSingleStep; + /** + * Capture HEAD in the worktree before step work begins (the per-step baseline, + * KTD-2 documented behavior change). Defaults to + * `git rev-parse HEAD` in {@link RunTaskStepDeps.worktreePath}; inject in tests. + */ + gitRevParse?: (worktreePath: string) => Promise; + /** + * Capture the session checkpoint (leaf) id for the step — observed the same way + * the legacy `stepCheckpoints` map is populated (`session.sessionManager.getLeafId()`). + * Defaults to reading {@link RunTaskStepOptions.sessionRef}; inject in tests. + */ + captureCheckpointId?: () => string | undefined; +} + +/** Options for {@link runTaskStep}. */ +export interface RunTaskStepOptions { + /** Session ref used for the default checkpoint capture. */ + sessionRef?: SessionRef; +} + +/** Result of {@link runTaskStep}. */ +export interface RunTaskStepResult { + outcome: "success" | "failure"; + baselineSha?: string; + checkpointId?: string; +} + +/** + * Drive execution of exactly step `stepIndex` of `task`. + * + * Order of operations (matches the legacy step-session lifecycle the + * characterization tests pin): + * 1. mark the step `in-progress` via `store.updateStep` (projection sink); + * 2. capture `baselineSha` = HEAD in the worktree, BEFORE any step work; + * 3. run exactly step `i` as a step-session (the agent authors its own + * `complete Step N` commit — this driver only observes); + * 4. capture `checkpointId` (session leaf) for a later RETHINK rewind; + * 5. on success, mark the step `done`; on failure, leave the step non-done + * (the graph decides routing — KTD-4). + */ +export async function runTaskStep( + deps: RunTaskStepDeps, + task: StepRunnerTask, + stepIndex: number, + opts: RunTaskStepOptions = {}, +): Promise { + const { store, worktreePath } = deps; + const gitRevParse = deps.gitRevParse ?? defaultGitRevParse; + const captureCheckpointId = + deps.captureCheckpointId ?? (() => defaultCaptureCheckpointId(opts.sessionRef)); + + // 1. Projection: step → in-progress (KTD-7). updateStep's own guards apply. + try { + await store.updateStep(task.id, stepIndex, "in-progress"); + } catch (err) { + executorLog.warn( + `${task.id}: runTaskStep failed to mark step ${stepIndex} in-progress: ${errMsg(err)}`, + ); + } + + // 2. Baseline capture at instance start, before step work (KTD-2). + let baselineSha: string | undefined; + try { + baselineSha = await gitRevParse(worktreePath); + } catch (err) { + executorLog.warn(`${task.id}: runTaskStep baseline capture failed: ${errMsg(err)}`); + } + + // 3. Run exactly step i. The agent authors the commit; we observe only. + const result = await deps.runStep(stepIndex); + + // 4. Capture the session checkpoint (leaf) for a later RETHINK rewind. + let checkpointId: string | undefined; + try { + checkpointId = captureCheckpointId() ?? undefined; + } catch (err) { + executorLog.warn(`${task.id}: runTaskStep checkpoint capture failed: ${errMsg(err)}`); + } + + // 5. Projection: success → done; failure leaves the step non-done. + if (result.success) { + try { + await store.updateStep(task.id, stepIndex, "done"); + } catch (err) { + executorLog.warn( + `${task.id}: runTaskStep failed to mark step ${stepIndex} done: ${errMsg(err)}`, + ); + } + return { outcome: "success", baselineSha, checkpointId }; + } + + return { outcome: "failure", baselineSha, checkpointId }; +} + +// ── resetStepToBaseline ────────────────────────────────────────────────── + +/** Dependencies for {@link resetStepToBaseline}. */ +export interface ResetStepDeps { + /** Step-state projection sink (KTD-7). */ + store: Pick; + /** Absolute path to the task's worktree (where `git reset --hard` runs). */ + worktreePath: string; + /** Session ref for the conversation rewind (`navigateTree` / `branchWithSummary`). */ + sessionRef: SessionRef; + /** + * Review type — `code` reverts file changes via git reset; `plan` skips the + * git reset (no code was written), matching the legacy RETHINK branch. + */ + reviewType?: "code" | "plan"; + /** Optional reviewer summary used as the `branchWithSummary` fallback label. */ + summary?: string; + /** Optional auditor for the blast-radius guard refusal warning (KTD-2). */ + audit?: Pick; + /** + * Blast-radius guard hook (KTD-2, shared isolation). Returns `null` when the + * reset is safe, or a refusal `reason` string when it would destroy other + * steps' approved work (baseline not an ancestor of HEAD, or a later step is + * already done/skipped past the baseline). When omitted the guard is skipped + * (worktree isolation makes it structural — KTD-11). Tests inject a fake; + * production wires {@link makeAncestryBlastRadiusGuard}. + */ + blastRadiusGuard?: (baselineSha: string | undefined) => Promise; +} + +/** Result of {@link resetStepToBaseline}. */ +export interface ResetStepResult { + ok: boolean; + reason?: string; +} + +/** + * Reset step `stepIndex` to its per-step baseline — the verbatim RETHINK + * mechanics extracted from `executor.ts` (`fn_review_step` RETHINK case): + * + * - `git reset --hard ` in the worktree (code review only; skipped + * when `baselineSha` is missing or for plan reviews — today's semantics); + * - session rewind to the pre-step checkpoint via `navigateTree`, falling back + * to `sessionManager.branchWithSummary` (skipped when `checkpointId` is + * missing — today's semantics); + * - `store.updateStep(..., "pending")`. + * + * Before any mutation, the KTD-2 blast-radius guard runs (when provided): on a + * violation it returns `{ ok: false, reason }`, emits an audit warning, and + * mutates NOTHING. + */ +export async function resetStepToBaseline( + deps: ResetStepDeps, + task: StepRunnerTask, + stepIndex: number, + baselineSha?: string, + checkpointId?: string, +): Promise { + const { store, worktreePath, sessionRef } = deps; + const reviewType = deps.reviewType ?? "code"; + const taskId = task.id; + const step = stepIndex + 1; // legacy log lines are 1-indexed + + // ── KTD-2 blast-radius guard — assert BEFORE mutating anything. ────────── + if (deps.blastRadiusGuard) { + let refusal: string | null = null; + try { + refusal = await deps.blastRadiusGuard(baselineSha); + } catch (err) { + // A guard that itself fails is treated as a refusal — fail closed. + refusal = `blast-radius guard error: ${errMsg(err)}`; + } + if (refusal) { + executorLog.warn( + `${taskId}: RETHINK reset for step ${step} REFUSED by blast-radius guard: ${refusal}`, + ); + await deps.audit?.database({ + type: "task:integrity-warning", + target: taskId, + metadata: { + guard: "step-reset-blast-radius", + stepIndex, + baselineSha: baselineSha ?? null, + reason: refusal, + }, + }); + return { ok: false, reason: refusal }; + } + } + + // ── git reset --hard (code reviews only). ───────────────────── + if (reviewType === "code" && baselineSha) { + try { + await execAsync(`git reset --hard ${baselineSha}`, { cwd: worktreePath }); + executorLog.log(`${taskId}: RETHINK — git reset --hard ${baselineSha}`); + } catch (gitErr: unknown) { + executorLog.error(`${taskId}: RETHINK git reset failed: ${errMsg(gitErr)}`); + } + } else if (reviewType === "code") { + executorLog.log(`${taskId}: RETHINK — no baseline SHA, skipping git reset`); + } + + // ── Rewind conversation to the pre-step checkpoint. ────────────────────── + if (checkpointId && sessionRef.current) { + try { + await sessionRef.current.navigateTree(checkpointId, { summarize: false }); + executorLog.log(`${taskId}: RETHINK — session rewound to checkpoint ${checkpointId}`); + } catch (rewindErr: unknown) { + executorLog.warn( + `${taskId}: RETHINK navigateTree rewind failed, falling back to branchWithSummary: ${errMsg(rewindErr)}`, + ); + try { + sessionRef.current.sessionManager.branchWithSummary( + checkpointId, + `RETHINK: ${deps.summary || "Approach rejected by reviewer"}`, + ); + executorLog.log(`${taskId}: RETHINK — branched from checkpoint ${checkpointId}`); + } catch (branchErr: unknown) { + executorLog.error(`${taskId}: RETHINK session rewind failed: ${errMsg(branchErr)}`); + } + } + } else { + executorLog.log(`${taskId}: RETHINK — no session checkpoint for step ${step}, skipping rewind`); + } + + // ── Reset step status to pending (projection sink). ────────────────────── + await store.updateStep(taskId, stepIndex, "pending"); + + if (reviewType === "plan") { + await store.logEntry( + taskId, + `RETHINK: Step ${step} plan rewound — session checkpoint ${checkpointId || "N/A"}`, + deps.summary, + ); + } else { + await store.logEntry( + taskId, + `RETHINK: Step ${step} rewound — git reset to ${baselineSha || "N/A"}, session checkpoint ${checkpointId || "N/A"}`, + deps.summary, + ); + } + + return { ok: true }; +} + +// ── Blast-radius guard factory (shared isolation, KTD-2) ───────────────── + +/** + * Build the shared-isolation blast-radius guard: a reset for step `stepIndex` is + * legal only when (a) `baselineSha` is an ancestor of HEAD in the worktree + * (`git merge-base --is-ancestor`), and (b) no LATER step is already + * `done`/`skipped` (which would postdate the baseline). On violation it returns + * the refusal reason; otherwise `null`. A missing baseline is allowed (the reset + * simply skips its git portion — today's partial-recovery semantics). + */ +export function makeAncestryBlastRadiusGuard(opts: { + worktreePath: string; + task: StepRunnerTask; + stepIndex: number; + isAncestor?: (baselineSha: string, worktreePath: string) => Promise; +}): (baselineSha: string | undefined) => Promise { + const isAncestor = opts.isAncestor ?? defaultIsAncestorOfHead; + return async (baselineSha: string | undefined): Promise => { + // (b) No later step may already be terminal-done past this baseline. + const laterDone = opts.task.steps.findIndex( + (s, i) => i > opts.stepIndex && (s.status === "done" || s.status === "skipped"), + ); + if (laterDone !== -1) { + return `later step ${laterDone} is ${opts.task.steps[laterDone]?.status} — reset would destroy approved work`; + } + // (a) Baseline must be an ancestor of HEAD (skipped when no baseline). + if (baselineSha) { + let ancestor = false; + try { + ancestor = await isAncestor(baselineSha, opts.worktreePath); + } catch (err) { + return `ancestry check failed: ${errMsg(err)}`; + } + if (!ancestor) { + return `baseline ${baselineSha} is not an ancestor of HEAD`; + } + } + return null; + }; +} + +// ── Defaults (production adapters over real git/session) ───────────────── + +async function defaultGitRevParse(worktreePath: string): Promise { + const { stdout } = await execAsync("git rev-parse HEAD", { cwd: worktreePath }); + const sha = stdout.trim(); + return sha.length > 0 ? sha : undefined; +} + +function defaultCaptureCheckpointId(sessionRef?: SessionRef): string | undefined { + const leaf = sessionRef?.current?.sessionManager?.getLeafId?.(); + return leaf ?? undefined; +} + +async function defaultIsAncestorOfHead(baselineSha: string, worktreePath: string): Promise { + try { + await execAsync(`git merge-base --is-ancestor ${baselineSha} HEAD`, { cwd: worktreePath }); + return true; + } catch { + // Non-zero exit → not an ancestor. + return false; + } +} + +function errMsg(err: unknown): string { + return err instanceof Error ? err.message : String(err); +}