feat(FN-4938): complete Step 2 — add helper and wire cleanup
Fusion-Task-Id: FN-4938 Fusion-Task-Lineage: 8b53a5da-1321-4796-9775-ff043efdcef3
This commit is contained in:
committed by
gsxdsm
parent
365639afa6
commit
d702231adc
@@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach } from "vitest";
|
||||
import { promisify } from "node:util";
|
||||
import { acquireTaskWorktree } from "../worktree-acquisition.js";
|
||||
import { classifyTaskWorktree, PoolDoubleLeaseError } from "../worktree-pool.js";
|
||||
import * as desktopArtifacts from "../worktree-desktop-artifacts.js";
|
||||
|
||||
vi.mock("../worktree-pool.js", async () => {
|
||||
const actual = await vi.importActual<any>("../worktree-pool.js");
|
||||
@@ -16,6 +17,10 @@ vi.mock("../worktree-db-hydrate.js", () => ({
|
||||
hydrateWorktreeDb: vi.fn().mockResolvedValue({ degraded: false, tasksCopied: 1, documentsCopied: 1 }),
|
||||
}));
|
||||
|
||||
vi.mock("../worktree-desktop-artifacts.js", () => ({
|
||||
removeDesktopBuildArtifacts: vi.fn().mockResolvedValue({ removed: [], skipped: [], failures: [] }),
|
||||
}));
|
||||
|
||||
describe("acquireTaskWorktree", () => {
|
||||
const task = {
|
||||
id: "FN-1",
|
||||
@@ -27,6 +32,8 @@ describe("acquireTaskWorktree", () => {
|
||||
|
||||
let store: any;
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
vi.mocked(desktopArtifacts.removeDesktopBuildArtifacts).mockResolvedValue({ removed: [], skipped: [], failures: [] });
|
||||
store = {
|
||||
updateTask: vi.fn().mockResolvedValue(undefined),
|
||||
logEntry: vi.fn().mockResolvedValue(undefined),
|
||||
@@ -69,6 +76,7 @@ describe("acquireTaskWorktree", () => {
|
||||
expect.objectContaining({ requestingTaskId: "FN-1" }),
|
||||
);
|
||||
expect(store.updateTask).toHaveBeenCalledWith("FN-1", { worktree: "/tmp/pooled", branch: "fusion/fn-1" });
|
||||
expect(desktopArtifacts.removeDesktopBuildArtifacts).toHaveBeenCalledWith("/tmp/pooled", undefined);
|
||||
});
|
||||
|
||||
it("releases acquired pooled worktree when prepareForTask returns reclaimed path", async () => {
|
||||
@@ -197,6 +205,48 @@ describe("acquireTaskWorktree", () => {
|
||||
expect(runConfiguredCommand).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("invokes desktop artifact cleanup before init command for fresh acquisition", async () => {
|
||||
const runConfiguredCommand = vi.fn().mockResolvedValue({ exitCode: 0, stderr: "", stdout: "" });
|
||||
|
||||
await acquireTaskWorktree({
|
||||
task,
|
||||
rootDir: process.cwd(),
|
||||
store,
|
||||
settings: { worktreeInitCommand: "pnpm install" } as any,
|
||||
createWorktree: vi.fn().mockResolvedValue({ path: "/tmp/new", branch: "fusion/fn-1" }),
|
||||
runConfiguredCommand,
|
||||
runInitCommand: true,
|
||||
});
|
||||
|
||||
expect(desktopArtifacts.removeDesktopBuildArtifacts).toHaveBeenCalledWith("/tmp/new", undefined);
|
||||
const cleanupOrder = vi.mocked(desktopArtifacts.removeDesktopBuildArtifacts).mock.invocationCallOrder[0];
|
||||
const initOrder = runConfiguredCommand.mock.invocationCallOrder[0];
|
||||
expect(cleanupOrder).toBeLessThan(initOrder);
|
||||
});
|
||||
|
||||
it("invokes desktop artifact cleanup once for pooled acquisition", async () => {
|
||||
const runConfiguredCommand = vi.fn();
|
||||
|
||||
await acquireTaskWorktree({
|
||||
task,
|
||||
rootDir: process.cwd(),
|
||||
store,
|
||||
settings: { recycleWorktrees: true, worktreeInitCommand: "pnpm install" } as any,
|
||||
pool: {
|
||||
acquire: (_taskId: string) => "/tmp/pooled",
|
||||
prepareForTask: vi.fn().mockResolvedValue({ branch: "fusion/fn-1", worktreePath: "/tmp/pooled", reclaimed: false }),
|
||||
release: vi.fn(),
|
||||
} as any,
|
||||
createWorktree: vi.fn(),
|
||||
runConfiguredCommand,
|
||||
runInitCommand: true,
|
||||
});
|
||||
|
||||
expect(desktopArtifacts.removeDesktopBuildArtifacts).toHaveBeenCalledTimes(1);
|
||||
expect(desktopArtifacts.removeDesktopBuildArtifacts).toHaveBeenCalledWith("/tmp/pooled", undefined);
|
||||
expect(runConfiguredCommand).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("FN-4834: logs worktree init stderr in task log outcome", async () => {
|
||||
const runConfiguredCommand = vi.fn().mockResolvedValue({
|
||||
exitCode: 1,
|
||||
|
||||
@@ -0,0 +1,90 @@
|
||||
import { mkdtempSync, mkdirSync, existsSync } from "node:fs";
|
||||
import * as fsPromises from "node:fs/promises";
|
||||
import { join } from "node:path";
|
||||
import { tmpdir } from "node:os";
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
vi.mock("node:fs/promises", async () => {
|
||||
const actual = await vi.importActual<typeof import("node:fs/promises")>("node:fs/promises");
|
||||
return {
|
||||
...actual,
|
||||
rm: vi.fn(actual.rm),
|
||||
};
|
||||
});
|
||||
|
||||
import {
|
||||
DESKTOP_ARTIFACT_RELATIVE_PATHS,
|
||||
removeDesktopBuildArtifacts,
|
||||
} from "../worktree-desktop-artifacts.js";
|
||||
|
||||
describe("removeDesktopBuildArtifacts", () => {
|
||||
afterEach(() => {
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
it("removes both desktop artifact directories when both exist", async () => {
|
||||
const root = mkdtempSync(join(tmpdir(), "wt-artifacts-"));
|
||||
for (const path of DESKTOP_ARTIFACT_RELATIVE_PATHS) {
|
||||
mkdirSync(join(root, path), { recursive: true });
|
||||
}
|
||||
|
||||
const result = await removeDesktopBuildArtifacts(root);
|
||||
|
||||
expect(result.removed.sort()).toEqual([...DESKTOP_ARTIFACT_RELATIVE_PATHS].sort());
|
||||
expect(result.skipped).toEqual([]);
|
||||
expect(result.failures).toEqual([]);
|
||||
for (const path of DESKTOP_ARTIFACT_RELATIVE_PATHS) {
|
||||
expect(existsSync(join(root, path))).toBe(false);
|
||||
}
|
||||
});
|
||||
|
||||
it("removes only existing dist-electron path and marks dist as skipped", async () => {
|
||||
const root = mkdtempSync(join(tmpdir(), "wt-artifacts-"));
|
||||
mkdirSync(join(root, "packages/desktop/dist-electron"), { recursive: true });
|
||||
|
||||
const result = await removeDesktopBuildArtifacts(root);
|
||||
|
||||
expect(result.removed).toEqual(["packages/desktop/dist-electron"]);
|
||||
expect(result.skipped).toEqual(["packages/desktop/dist"]);
|
||||
expect(result.failures).toEqual([]);
|
||||
expect(existsSync(join(root, "packages/desktop/dist-electron"))).toBe(false);
|
||||
});
|
||||
|
||||
it("is a no-op when neither path exists", async () => {
|
||||
const root = mkdtempSync(join(tmpdir(), "wt-artifacts-"));
|
||||
|
||||
const result = await removeDesktopBuildArtifacts(root);
|
||||
|
||||
expect(result.removed).toEqual([]);
|
||||
expect(result.skipped.sort()).toEqual([...DESKTOP_ARTIFACT_RELATIVE_PATHS].sort());
|
||||
expect(result.failures).toEqual([]);
|
||||
});
|
||||
|
||||
it("captures per-path failure and continues", async () => {
|
||||
const root = mkdtempSync(join(tmpdir(), "wt-artifacts-"));
|
||||
for (const path of DESKTOP_ARTIFACT_RELATIVE_PATHS) {
|
||||
mkdirSync(join(root, path), { recursive: true });
|
||||
}
|
||||
const rmSpy = vi.mocked(fsPromises.rm).mockImplementation(async (pathLike: fsPromises.PathLike) => {
|
||||
if (String(pathLike).endsWith("packages/desktop/dist")) {
|
||||
throw new Error("boom");
|
||||
}
|
||||
});
|
||||
|
||||
const warn = vi.fn();
|
||||
const result = await removeDesktopBuildArtifacts(root, { log: vi.fn(), warn });
|
||||
|
||||
expect(rmSpy).toHaveBeenCalled();
|
||||
expect(result.removed).toEqual(["packages/desktop/dist-electron"]);
|
||||
expect(result.failures).toEqual([{ path: "packages/desktop/dist", error: "boom" }]);
|
||||
expect(warn).toHaveBeenCalledWith(expect.stringContaining("Failed to remove desktop build artifact directory packages/desktop/dist: boom"));
|
||||
});
|
||||
|
||||
it("returns early when worktreePath is falsy", async () => {
|
||||
const warn = vi.fn();
|
||||
|
||||
const result = await removeDesktopBuildArtifacts("", { log: vi.fn(), warn });
|
||||
|
||||
expect(result).toEqual({ removed: [], skipped: [], failures: [] });
|
||||
expect(warn).toHaveBeenCalledWith("Desktop artifact cleanup skipped: missing worktree path");
|
||||
});
|
||||
});
|
||||
@@ -38,6 +38,10 @@ vi.mock("node:child_process", async () => {
|
||||
return { execSync: execSyncFn, exec: execFn };
|
||||
});
|
||||
|
||||
vi.mock("../worktree-desktop-artifacts.js", () => ({
|
||||
removeDesktopBuildArtifacts: vi.fn().mockResolvedValue({ removed: [], skipped: [], failures: [] }),
|
||||
}));
|
||||
|
||||
vi.mock("node:fs", () => ({
|
||||
existsSync: vi.fn().mockReturnValue(true),
|
||||
lstatSync: vi.fn().mockReturnValue({ isDirectory: () => true, isSymbolicLink: () => false }),
|
||||
@@ -45,6 +49,7 @@ vi.mock("node:fs", () => ({
|
||||
rmSync: vi.fn(),
|
||||
}));
|
||||
|
||||
import * as desktopArtifacts from "../worktree-desktop-artifacts.js";
|
||||
import {
|
||||
WorktreePool,
|
||||
getRegisteredWorktreeBranchMap,
|
||||
@@ -86,6 +91,7 @@ describe("WorktreePool", () => {
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
vi.mocked(desktopArtifacts.removeDesktopBuildArtifacts).mockResolvedValue({ removed: [], skipped: [], failures: [] });
|
||||
mockedExistsSync.mockReturnValue(true);
|
||||
pool = new WorktreePool();
|
||||
});
|
||||
@@ -217,6 +223,21 @@ describe("WorktreePool", () => {
|
||||
expect(calls).toContain("git clean -fd");
|
||||
});
|
||||
|
||||
it("removes desktop artifacts after git clean and before detach checkout", async () => {
|
||||
await pool.prepareForTask("/tmp/wt", "fusion/fn-042");
|
||||
|
||||
expect(desktopArtifacts.removeDesktopBuildArtifacts).toHaveBeenCalledWith("/tmp/wt", expect.anything());
|
||||
const cleanOrder = mockedExecSync.mock.calls.find((c) => c[0] === "git clean -fd");
|
||||
const detachOrder = mockedExecSync.mock.calls.find((c) => c[0] === "git checkout --detach main");
|
||||
expect(cleanOrder).toBeDefined();
|
||||
expect(detachOrder).toBeDefined();
|
||||
const cleanupOrder = vi.mocked(desktopArtifacts.removeDesktopBuildArtifacts).mock.invocationCallOrder[0];
|
||||
const cleanCallOrder = mockedExecSync.mock.invocationCallOrder[mockedExecSync.mock.calls.findIndex((c) => c[0] === "git clean -fd")];
|
||||
const detachCallOrder = mockedExecSync.mock.invocationCallOrder[mockedExecSync.mock.calls.findIndex((c) => c[0] === "git checkout --detach main")];
|
||||
expect(cleanCallOrder).toBeLessThan(cleanupOrder);
|
||||
expect(cleanupOrder).toBeLessThan(detachCallOrder);
|
||||
});
|
||||
|
||||
it("creates branch from main with force-reset", async () => {
|
||||
await pool.prepareForTask("/tmp/wt", "fusion/fn-042");
|
||||
|
||||
|
||||
@@ -33,6 +33,7 @@ import {
|
||||
} from "./worktrunk-failure-handler.js";
|
||||
import type { RunAuditor } from "./run-audit.js";
|
||||
import { writeSecretsEnvFile } from "./secrets-env-writer.js";
|
||||
import { removeDesktopBuildArtifacts } from "./worktree-desktop-artifacts.js";
|
||||
|
||||
const execAsync = promisify(exec);
|
||||
|
||||
@@ -252,6 +253,10 @@ export async function acquireTaskWorktree(opts: AcquireTaskWorktreeOptions): Pro
|
||||
|
||||
if (task.worktree && isResume) {
|
||||
logger?.log(`Reusing existing worktree: ${worktreePath}`);
|
||||
const cleanup = await removeDesktopBuildArtifacts(worktreePath, logger);
|
||||
if (cleanup.removed.length > 0) {
|
||||
await store.logEntry(task.id, `Removed desktop build artifacts from worktree: ${cleanup.removed.join(", ")}`, undefined, runContext);
|
||||
}
|
||||
const hydrated = await hydrate(worktreePath);
|
||||
// FN-4912: resume path reuses the prior on-disk .env (and its fingerprint sidecar). Rewrite is owned by the next fresh acquisition.
|
||||
return { worktreePath, branch: task.branch ?? branchName, source: "existing", hydrated, isResume: true };
|
||||
@@ -331,6 +336,10 @@ export async function acquireTaskWorktree(opts: AcquireTaskWorktreeOptions): Pro
|
||||
} else {
|
||||
await store.logEntry(task.id, `Acquired worktree from pool: ${worktreePath}`, undefined, runContext);
|
||||
}
|
||||
const cleanup = await removeDesktopBuildArtifacts(worktreePath, logger);
|
||||
if (cleanup.removed.length > 0) {
|
||||
await store.logEntry(task.id, `Removed desktop build artifacts from worktree: ${cleanup.removed.join(", ")}`, undefined, runContext);
|
||||
}
|
||||
await maybeWarnForeignTaskStartPoint({
|
||||
baseBranch,
|
||||
rootDir,
|
||||
@@ -438,6 +447,11 @@ export async function acquireTaskWorktree(opts: AcquireTaskWorktreeOptions): Pro
|
||||
await store.logEntry(task.id, `Worktree created at ${worktreePath}`, undefined, runContext);
|
||||
}
|
||||
|
||||
const cleanup = await removeDesktopBuildArtifacts(worktreePath, logger);
|
||||
if (cleanup.removed.length > 0) {
|
||||
await store.logEntry(task.id, `Removed desktop build artifacts from worktree: ${cleanup.removed.join(", ")}`, undefined, runContext);
|
||||
}
|
||||
|
||||
if (runInitCommand && settings.worktreeInitCommand && runConfiguredCommand) {
|
||||
const initStartedAt = Date.now();
|
||||
let initResult: InitCommandResult | undefined;
|
||||
|
||||
44
packages/engine/src/worktree-desktop-artifacts.ts
Normal file
44
packages/engine/src/worktree-desktop-artifacts.ts
Normal file
@@ -0,0 +1,44 @@
|
||||
import { existsSync } from "node:fs";
|
||||
import { rm } from "node:fs/promises";
|
||||
import { resolve } from "node:path";
|
||||
|
||||
export const DESKTOP_ARTIFACT_RELATIVE_PATHS = ["packages/desktop/dist", "packages/desktop/dist-electron"] as const;
|
||||
|
||||
type CleanupLogger = {
|
||||
log: (message: string) => void;
|
||||
warn: (message: string) => void;
|
||||
};
|
||||
|
||||
export async function removeDesktopBuildArtifacts(
|
||||
worktreePath: string,
|
||||
logger?: CleanupLogger,
|
||||
): Promise<{ removed: string[]; skipped: string[]; failures: Array<{ path: string; error: string }> }> {
|
||||
const removed: string[] = [];
|
||||
const skipped: string[] = [];
|
||||
const failures: Array<{ path: string; error: string }> = [];
|
||||
|
||||
if (!worktreePath) {
|
||||
logger?.warn?.("Desktop artifact cleanup skipped: missing worktree path");
|
||||
return { removed, skipped, failures };
|
||||
}
|
||||
|
||||
for (const relativePath of DESKTOP_ARTIFACT_RELATIVE_PATHS) {
|
||||
const absolutePath = resolve(worktreePath, relativePath);
|
||||
if (!existsSync(absolutePath)) {
|
||||
skipped.push(relativePath);
|
||||
continue;
|
||||
}
|
||||
|
||||
try {
|
||||
await rm(absolutePath, { recursive: true, force: true });
|
||||
removed.push(relativePath);
|
||||
logger?.log?.(`Removed desktop build artifact directory: ${relativePath}`);
|
||||
} catch (error) {
|
||||
const message = error instanceof Error ? error.message : String(error);
|
||||
failures.push({ path: relativePath, error: message });
|
||||
logger?.warn?.(`Failed to remove desktop build artifact directory ${relativePath}: ${message}`);
|
||||
}
|
||||
}
|
||||
|
||||
return { removed, skipped, failures };
|
||||
}
|
||||
@@ -15,6 +15,7 @@ import {
|
||||
resolveWorktreeBackend as resolveWorktreeBackendViaSettings,
|
||||
} from "./worktree-backend.js";
|
||||
import { cleanupSecretsEnvFile } from "./secrets-env-writer.js";
|
||||
import { removeDesktopBuildArtifacts } from "./worktree-desktop-artifacts.js";
|
||||
import type { RunAuditor } from "./run-audit.js";
|
||||
|
||||
export {
|
||||
@@ -410,13 +411,16 @@ export class WorktreePool {
|
||||
* the task's branch based on the given start point (or `main` by default).
|
||||
* This ensures the new task starts from the correct base with a clean
|
||||
* working directory, while preserving untracked build caches
|
||||
* (node_modules, target/, dist/).
|
||||
* (node_modules, target/, dist/). As an explicit carve-out, this
|
||||
* preparation removes `packages/desktop/dist` and
|
||||
* `packages/desktop/dist-electron`.
|
||||
*
|
||||
* Steps performed:
|
||||
* 1. `git checkout -- .` — discard tracked file modifications
|
||||
* 2. `git clean -fd` — remove untracked files (but not .gitignore'd caches)
|
||||
* 3. `git checkout --detach <startPoint>` — move HEAD to the latest base commit
|
||||
* 4. `git checkout -B <branchName> <startPoint>` — create/reset branch from start point
|
||||
* 3. Remove `packages/desktop/dist` + `packages/desktop/dist-electron` if present
|
||||
* 4. `git checkout --detach <startPoint>` — move HEAD to the latest base commit
|
||||
* 5. `git checkout -B <branchName> <startPoint>` — create/reset branch from start point
|
||||
*
|
||||
* Returns the actual branch name used. This may differ from `branchName`
|
||||
* when legacy conflict recovery is explicitly enabled and generates a suffixed
|
||||
@@ -444,6 +448,7 @@ export class WorktreePool {
|
||||
|
||||
// Remove untracked files (but not .gitignore'd build caches)
|
||||
await execAsync("git clean -fd", { cwd: worktreePath });
|
||||
await removeDesktopBuildArtifacts(worktreePath, worktreePoolLog);
|
||||
|
||||
const base = startPoint || "main";
|
||||
await execAsync(`git checkout --detach ${base}`, {
|
||||
|
||||
Reference in New Issue
Block a user