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` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## 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. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
7
.changeset/external-checkout-routing-review-followup.md
Normal file
7
.changeset/external-checkout-routing-review-followup.md
Normal file
@@ -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.
|
||||
@@ -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
|
||||
|
||||
@@ -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"),
|
||||
|
||||
@@ -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",
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user