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);
|
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 () => {
|
it("completed-task recovery captures the live external checkout instead of a stale task worktree", async () => {
|
||||||
const liveTask = task({
|
const liveTask = task({
|
||||||
id: "FN-7283-EXTERNAL-RECOVERY",
|
id: "FN-7283-EXTERNAL-RECOVERY",
|
||||||
@@ -1079,6 +1084,7 @@ describe("fast mode workflow/runtime invariants", () => {
|
|||||||
const recovered = await executor.recoverCompletedTask(staleSnapshot as any);
|
const recovered = await executor.recoverCompletedTask(staleSnapshot as any);
|
||||||
|
|
||||||
expect(recovered).toBe(true);
|
expect(recovered).toBe(true);
|
||||||
|
expect(mockedResolveExternalExecutionCheckoutRoute).toHaveBeenCalledWith(liveTask);
|
||||||
expect(captureModifiedFiles).toHaveBeenCalledWith(
|
expect(captureModifiedFiles).toHaveBeenCalledWith(
|
||||||
"/tmp/external-runtime",
|
"/tmp/external-runtime",
|
||||||
"base",
|
"base",
|
||||||
@@ -1118,6 +1124,7 @@ describe("fast mode workflow/runtime invariants", () => {
|
|||||||
"Review requested changes",
|
"Review requested changes",
|
||||||
);
|
);
|
||||||
|
|
||||||
|
expect(mockedResolveExternalExecutionCheckoutRoute).toHaveBeenCalledWith(liveTask);
|
||||||
expect(scheduleWorkflowRerun).toHaveBeenCalledWith(
|
expect(scheduleWorkflowRerun).toHaveBeenCalledWith(
|
||||||
"FN-7283-EXTERNAL-REMEDIATION",
|
"FN-7283-EXTERNAL-REMEDIATION",
|
||||||
"/tmp/external-runtime",
|
"/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):
|
FNXC:EngineTests 2026-07-19-18:20 (U10b):
|
||||||
The requirement under test is a store that CANNOT resolve a workflow selection (minimal/older embedded
|
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");
|
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 () => {
|
it("does not remove or delete an operator-owned external execution checkout", async () => {
|
||||||
resetExecutorMocks();
|
resetExecutorMocks();
|
||||||
const store = createMockStore();
|
const store = createMockStore();
|
||||||
@@ -70,8 +75,9 @@ describe("dependency-abort cleanup requeues to a DECLARED column", () => {
|
|||||||
const executor = new TaskExecutor(store, "/tmp/test");
|
const executor = new TaskExecutor(store, "/tmp/test");
|
||||||
const removeManagedWorktree = vi.spyOn(executor as any, "removeOwnWorktreeWithReconcile");
|
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(removeManagedWorktree).not.toHaveBeenCalled();
|
||||||
expect(mockedExec).not.toHaveBeenCalledWith(
|
expect(mockedExec).not.toHaveBeenCalledWith(
|
||||||
expect.stringContaining("git branch -D"),
|
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 { beforeEach, describe, expect, it, vi } from "vitest";
|
||||||
import "./executor-test-helpers.js";
|
import "./executor-test-helpers.js";
|
||||||
import { TaskExecutor } from "../executor.js";
|
import { TaskExecutor } from "../executor.js";
|
||||||
@@ -62,6 +65,10 @@ describe("FN-009: verifyWorktreeInvariants with missing worktree directory", ()
|
|||||||
store.getTask.mockResolvedValue(task as any);
|
store.getTask.mockResolvedValue(task as any);
|
||||||
// Mock existsSync to return true for the worktree path
|
// Mock existsSync to return true for the worktree path
|
||||||
mockedExistsSync.mockReturnValue(true);
|
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);
|
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.
|
// 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.
|
// We verify that existsSync was called by checking it was configured.
|
||||||
expect(mockedExistsSync).toHaveBeenCalled();
|
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 () => {
|
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) }));
|
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 () => {
|
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 = {
|
const staleTask = {
|
||||||
id: "FN-9006",
|
id: "FN-9006",
|
||||||
title: "Test",
|
title: "Test",
|
||||||
@@ -152,17 +167,18 @@ describe("FN-009: verifyWorktreeInvariants with missing worktree directory", ()
|
|||||||
store.getTask.mockResolvedValue({
|
store.getTask.mockResolvedValue({
|
||||||
...staleTask,
|
...staleTask,
|
||||||
sourceMetadata: {
|
sourceMetadata: {
|
||||||
externalExecutionCheckout: "/tmp/missing-operator-checkout",
|
externalExecutionCheckout: missingExternalCheckout,
|
||||||
externalExecutionBranch: "operator/runtime-fixes",
|
externalExecutionBranch: "operator/runtime-fixes",
|
||||||
},
|
},
|
||||||
});
|
});
|
||||||
|
mockedExistsSync.mockImplementation((path: any) => path !== missingExternalCheckout);
|
||||||
|
|
||||||
const result = await (executor as any).verifyWorktreeInvariants(staleTask);
|
const result = await (executor as any).verifyWorktreeInvariants(staleTask);
|
||||||
|
|
||||||
expect(result).toMatchObject({
|
expect(result).toMatchObject({
|
||||||
ok: false,
|
ok: false,
|
||||||
reason: "wrong_toplevel",
|
reason: "wrong_toplevel",
|
||||||
observed: expect.stringContaining("checkoutPath"),
|
observed: `checkoutPath is not a directory: ${missingExternalCheckout}`,
|
||||||
expected: "valid persisted external execution checkout",
|
expected: "valid persisted external execution checkout",
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -19688,8 +19688,16 @@ ${failureContext.output.slice(0, VERIFICATION_LOG_MAX_CHARS)}
|
|||||||
if (externalExecutionRoute.configured && !externalExecutionRoute.valid) {
|
if (externalExecutionRoute.configured && !externalExecutionRoute.valid) {
|
||||||
throw new Error(`Persisted external execution checkout is invalid: ${externalExecutionRoute.reason ?? "unknown error"}`);
|
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
|
const remediationWorktreePath = externalExecutionRoute.configured
|
||||||
? externalExecutionRoute.checkoutPath ?? ""
|
? externalExecutionRoute.checkoutPath!
|
||||||
: worktreePath;
|
: worktreePath;
|
||||||
|
|
||||||
// 1. Add a task comment explaining the failure
|
// 1. Add a task comment explaining the failure
|
||||||
|
|||||||
Reference in New Issue
Block a user