From b28b6d10539fdffd4f68716862953035cf4857c6 Mon Sep 17 00:00:00 2001 From: Phil Larson Date: Sun, 9 Aug 2026 18:23:39 -0700 Subject: [PATCH] fix: fail closed on incomplete external checkout routes (#3401) ## Summary - fail closed when a persisted external remediation route lacks a concrete checkout path - verify recovery, remediation, dependency-abort cleanup, and completion validation use the live persisted task route - use unique missing-checkout fixtures and exact observed-path assertions ## Test plan - `pnpm --filter @fusion/engine exec vitest run src/__tests__/verify-worktree-invariants-missing.test.ts src/__tests__/executor-triage-column-audit.test.ts --silent=passed-only --reporter=dot` - `pnpm --filter @fusion/engine exec vitest run src/__tests__/executor-fast-mode-workflows.test.ts -t 'completed-task recovery captures the live external checkout|pre-merge remediation' --silent=passed-only --reporter=dot` - `pnpm --filter @fusion/engine typecheck` - `pnpm exec eslint packages/engine/src/executor.ts packages/engine/src/__tests__/verify-worktree-invariants-missing.test.ts packages/engine/src/__tests__/executor-triage-column-audit.test.ts packages/engine/src/__tests__/executor-fast-mode-workflows.test.ts` - `pnpm check:fnxc-future-dates` - `pnpm check:changesets --strict` - `git diff --check` ## Summary by CodeRabbit * **Bug Fixes** * Improved recovery for externally executed tasks by using the latest routing information instead of outdated task data. * External remediation now stops safely when a checkout location is missing or invalid, preventing execution in an unintended location. * Improved cleanup behavior to preserve operator-owned checkouts. * Enhanced validation and error reporting for missing or invalid checkout paths. * **Tests** * Expanded coverage for recovery, remediation safety, checkout ownership, and worktree validation scenarios. --- ...ternal-checkout-routing-review-followup.md | 7 ++++ .../executor-fast-mode-workflows.test.ts | 36 +++++++++++++++++++ .../executor-triage-column-audit.test.ts | 8 ++++- ...verify-worktree-invariants-missing.test.ts | 22 ++++++++++-- packages/engine/src/executor.ts | 10 +++++- 5 files changed, 78 insertions(+), 5 deletions(-) create mode 100644 .changeset/external-checkout-routing-review-followup.md diff --git a/.changeset/external-checkout-routing-review-followup.md b/.changeset/external-checkout-routing-review-followup.md new file mode 100644 index 0000000000..7465822425 --- /dev/null +++ b/.changeset/external-checkout-routing-review-followup.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Fail closed when external remediation routing lacks a checkout path. +category: fix +dev: Strengthens live-route recovery, verification, and cleanup regression coverage. diff --git a/packages/engine/src/__tests__/executor-fast-mode-workflows.test.ts b/packages/engine/src/__tests__/executor-fast-mode-workflows.test.ts index da3f5e1ef8..389eb9099a 100644 --- a/packages/engine/src/__tests__/executor-fast-mode-workflows.test.ts +++ b/packages/engine/src/__tests__/executor-fast-mode-workflows.test.ts @@ -1052,6 +1052,11 @@ describe("fast mode workflow/runtime invariants", () => { expect(graph).toHaveBeenCalledWith(liveTask); }); + /* + FNXC:ExternalExecutionCheckout 2026-08-10-01:06: + Recovery and remediation must resolve the persisted live task, not a stale caller snapshot. + A configured route is usable only when it provides the concrete operator-owned checkout path. + */ it("completed-task recovery captures the live external checkout instead of a stale task worktree", async () => { const liveTask = task({ id: "FN-7283-EXTERNAL-RECOVERY", @@ -1079,6 +1084,7 @@ describe("fast mode workflow/runtime invariants", () => { const recovered = await executor.recoverCompletedTask(staleSnapshot as any); expect(recovered).toBe(true); + expect(mockedResolveExternalExecutionCheckoutRoute).toHaveBeenCalledWith(liveTask); expect(captureModifiedFiles).toHaveBeenCalledWith( "/tmp/external-runtime", "base", @@ -1118,6 +1124,7 @@ describe("fast mode workflow/runtime invariants", () => { "Review requested changes", ); + expect(mockedResolveExternalExecutionCheckoutRoute).toHaveBeenCalledWith(liveTask); expect(scheduleWorkflowRerun).toHaveBeenCalledWith( "FN-7283-EXTERNAL-REMEDIATION", "/tmp/external-runtime", @@ -1127,6 +1134,35 @@ describe("fast mode workflow/runtime invariants", () => { ); }); + it("pre-merge remediation fails closed when a configured route has no checkout path", async () => { + const liveTask = task({ + id: "FN-7283-EXTERNAL-REMEDIATION-MISSING-PATH", + worktree: "/tmp/stale-managed-worktree", + steps: [{ name: "Do it", status: "done" }], + sourceMetadata: { + externalExecutionCheckout: "/tmp/external-runtime", + externalExecutionBranch: "local/runtime-fixes", + }, + }); + const { executor } = makeExecutorForTask(liveTask); + mockedResolveExternalExecutionCheckoutRoute.mockResolvedValue({ + configured: true, + valid: true, + branch: "local/runtime-fixes", + }); + const scheduleWorkflowRerun = vi.spyOn(executor as any, "scheduleWorkflowRerun").mockImplementation(() => undefined); + + await expect((executor as any).sendTaskBackForFix( + liveTask, + "/tmp/stale-managed-worktree", + "fix it", + "Code Review", + "Review requested changes", + )).rejects.toThrow("checkoutPath is missing"); + + expect(scheduleWorkflowRerun).not.toHaveBeenCalled(); + }); + /* FNXC:EngineTests 2026-07-19-18:20 (U10b): The requirement under test is a store that CANNOT resolve a workflow selection (minimal/older embedded diff --git a/packages/engine/src/__tests__/executor-triage-column-audit.test.ts b/packages/engine/src/__tests__/executor-triage-column-audit.test.ts index 33ae87db27..68b416eb0a 100644 --- a/packages/engine/src/__tests__/executor-triage-column-audit.test.ts +++ b/packages/engine/src/__tests__/executor-triage-column-audit.test.ts @@ -55,6 +55,11 @@ describe("dependency-abort cleanup requeues to a DECLARED column", () => { expect(store.moveTask).not.toHaveBeenCalledWith("FN-DEP", "triage"); }); + /* + FNXC:ExternalExecutionCheckout 2026-08-10-01:06: + Dependency-abort cleanup must re-read persisted ownership and ignore stale managed-path arguments, + so Fusion never removes or branch-deletes an operator-owned external checkout. + */ it("does not remove or delete an operator-owned external execution checkout", async () => { resetExecutorMocks(); const store = createMockStore(); @@ -70,8 +75,9 @@ describe("dependency-abort cleanup requeues to a DECLARED column", () => { const executor = new TaskExecutor(store, "/tmp/test"); const removeManagedWorktree = vi.spyOn(executor as any, "removeOwnWorktreeWithReconcile"); - await (executor as any).handleDepAbortCleanup("FN-EXT", "/tmp/operator-owned-checkout"); + await (executor as any).handleDepAbortCleanup("FN-EXT", "/tmp/test/.worktrees/fn-ext"); + expect(store.getTask).toHaveBeenCalledWith("FN-EXT"); expect(removeManagedWorktree).not.toHaveBeenCalled(); expect(mockedExec).not.toHaveBeenCalledWith( expect.stringContaining("git branch -D"), diff --git a/packages/engine/src/__tests__/verify-worktree-invariants-missing.test.ts b/packages/engine/src/__tests__/verify-worktree-invariants-missing.test.ts index f8a2827ab1..847d6c3bf4 100644 --- a/packages/engine/src/__tests__/verify-worktree-invariants-missing.test.ts +++ b/packages/engine/src/__tests__/verify-worktree-invariants-missing.test.ts @@ -1,3 +1,6 @@ +import { randomUUID } from "node:crypto"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { beforeEach, describe, expect, it, vi } from "vitest"; import "./executor-test-helpers.js"; import { TaskExecutor } from "../executor.js"; @@ -62,6 +65,10 @@ describe("FN-009: verifyWorktreeInvariants with missing worktree directory", () store.getTask.mockResolvedValue(task as any); // Mock existsSync to return true for the worktree path mockedExistsSync.mockReturnValue(true); + mockedExecSync.mockImplementation((cmd: string) => { + if (cmd.includes("rev-parse --show-toplevel")) return Buffer.from("/repo\n"); + return Buffer.from(""); + }); const result = await (executor as any).verifyWorktreeInvariants(task); @@ -70,7 +77,9 @@ describe("FN-009: verifyWorktreeInvariants with missing worktree directory", () // that the function doesn't return early with { ok: true } due to missing directory. // We verify that existsSync was called by checking it was configured. expect(mockedExistsSync).toHaveBeenCalled(); - expect(result).toBeDefined(); + expect(mockedExecSync).toHaveBeenCalled(); + expect(result).not.toEqual({ ok: true }); + expect(result).toMatchObject({ ok: false, reason: "wrong_toplevel" }); }); it("re-anchors nested task worktree to registered root and passes invariants", async () => { @@ -135,7 +144,13 @@ describe("FN-009: verifyWorktreeInvariants with missing worktree directory", () expect(store.updateTask).not.toHaveBeenCalledWith("FN-9005", expect.objectContaining({ worktree: expect.any(String) })); }); + /* + FNXC:ExternalExecutionCheckout 2026-08-10-01:06: + Verification must re-read persisted routing, fail closed when the live operator checkout is missing, + and report that live path rather than a stale managed-worktree snapshot. + */ it("fails closed from the live external route when the verification snapshot is stale", async () => { + const missingExternalCheckout = join(tmpdir(), `fusion-missing-operator-checkout-${randomUUID()}`); const staleTask = { id: "FN-9006", title: "Test", @@ -152,17 +167,18 @@ describe("FN-009: verifyWorktreeInvariants with missing worktree directory", () store.getTask.mockResolvedValue({ ...staleTask, sourceMetadata: { - externalExecutionCheckout: "/tmp/missing-operator-checkout", + externalExecutionCheckout: missingExternalCheckout, externalExecutionBranch: "operator/runtime-fixes", }, }); + mockedExistsSync.mockImplementation((path: any) => path !== missingExternalCheckout); const result = await (executor as any).verifyWorktreeInvariants(staleTask); expect(result).toMatchObject({ ok: false, reason: "wrong_toplevel", - observed: expect.stringContaining("checkoutPath"), + observed: `checkoutPath is not a directory: ${missingExternalCheckout}`, expected: "valid persisted external execution checkout", }); }); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index df80ef5ea9..f58422411b 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -19688,8 +19688,16 @@ ${failureContext.output.slice(0, VERIFICATION_LOG_MAX_CHARS)} if (externalExecutionRoute.configured && !externalExecutionRoute.valid) { throw new Error(`Persisted external execution checkout is invalid: ${externalExecutionRoute.reason ?? "unknown error"}`); } + /* + FNXC:ExternalExecutionCheckout 2026-08-10-01:06: + Remediation must fail closed unless a configured persisted route resolves to a concrete checkout path. + Never turn malformed operator-owned routing into an empty managed-worktree path. + */ + if (externalExecutionRoute.configured && !externalExecutionRoute.checkoutPath) { + throw new Error("Persisted external execution checkout is invalid: checkoutPath is missing"); + } const remediationWorktreePath = externalExecutionRoute.configured - ? externalExecutionRoute.checkoutPath ?? "" + ? externalExecutionRoute.checkoutPath! : worktreePath; // 1. Add a task comment explaining the failure