From 0e22308176545a63dacc4e390270ddd7e1695198 Mon Sep 17 00:00:00 2001 From: manzi osee Date: Tue, 26 May 2026 17:46:28 +0200 Subject: [PATCH] fix(cli): derive extraction path from instanceDir in vellum recover MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tar extraction target in `recover` was hardcoded to homedir(), which is wrong for any instance whose instanceDir differs from \~/. When a named instance (instanceDir !== homedir()) was retired, its data was archived under $(retiredDir)/$(name).tar.gz.staging. Extracting with `-C homedir()` would silently unpack those files into the home directory instead of the original location, making the restore unusable. Fix: mirror the logic in retireLocal — - named instance → targetDir = instanceDir - default instance → targetDir = join(instanceDir, '.vellum') Extract with `-C retiredDir` (the directory where the archive lives), then rename the extracted staging entry to targetDir. Also create parent directories with mkdirSync so custom paths like /opt/vellum/… are recoverable even when the parent did not previously exist. Also improves the `--help` output with a description of where archives are stored and two concrete example invocations, per CLI AGENTS.md. Adds recover.test.ts covering: - help text output - missing name / missing archive / missing resources error paths - target-already-exists collision guard - correct extraction path for default instances (instanceDir === homedir()) - correct extraction path for named instances (instanceDir !== homedir()) - parent directory creation for deep custom paths --- cli/src/__tests__/recover.test.ts | 289 ++++++++++++++++++++++++++++++ cli/src/commands/recover.ts | 50 +++++- 2 files changed, 332 insertions(+), 7 deletions(-) create mode 100644 cli/src/__tests__/recover.test.ts diff --git a/cli/src/__tests__/recover.test.ts b/cli/src/__tests__/recover.test.ts new file mode 100644 index 00000000000..27992b121ef --- /dev/null +++ b/cli/src/__tests__/recover.test.ts @@ -0,0 +1,289 @@ +import { + afterEach, + beforeEach, + describe, + expect, + mock, + spyOn, + test, +} from "bun:test"; +import { + existsSync, + mkdirSync, + mkdtempSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { homedir, tmpdir } from "node:os"; +import { basename, join } from "node:path"; + +import type { AssistantEntry } from "../lib/assistant-config.js"; + +// ── Module mocks (must appear before importing the module under test) ──────── + +// Prevent real daemon / gateway from starting +const startLocalDaemonMock = mock(async () => {}); +const startGatewayMock = mock(async () => {}); +mock.module("../lib/local.js", () => ({ + generateLocalSigningKey: () => "deadbeefdeadbeefdeadbeefdeadbeef", + startLocalDaemon: startLocalDaemonMock, + startGateway: startGatewayMock, +})); + +// Capture exec calls without running real tar +const execMock = mock(async (_cmd: string, _args: string[]) => {}); +mock.module("../lib/step-runner.js", () => ({ exec: execMock })); + +import { recover } from "../commands/recover.js"; + +// ── Test fixtures ───────────────────────────────────────────────────────────── + +const testDir = mkdtempSync(join(tmpdir(), "cli-recover-test-")); +const originalArgv = [...process.argv]; +const originalLockfileDir = process.env.VELLUM_LOCKFILE_DIR; +const originalXdgData = process.env.XDG_DATA_HOME; + +// Directories that getRetiredDir() will use when XDG_DATA_HOME is overridden +const retiredDir = join(testDir, "vellum", "retired"); + +function makeEntry(assistantId: string, instanceDir: string): AssistantEntry { + return { + assistantId, + runtimeUrl: "http://127.0.0.1:7831", + cloud: "local", + resources: { + instanceDir, + daemonPort: 7801, + gatewayPort: 7831, + qdrantPort: 6334, + cesPort: 7790, + }, + }; +} + +function writeArchiveFixtures( + name: string, + entry: AssistantEntry, +): { + archivePath: string; + metadataPath: string; + extractedPath: string; +} { + mkdirSync(retiredDir, { recursive: true }); + const archivePath = join(retiredDir, `${name}.tar.gz`); + const metadataPath = join(retiredDir, `${name}.json`); + // The staging dir is what tar extracts — .staging relative to retiredDir + const extractedPath = join(retiredDir, basename(archivePath) + ".staging"); + + // Write a placeholder archive file (exec is mocked; content doesn't matter) + writeFileSync(archivePath, ""); + writeFileSync(metadataPath, JSON.stringify(entry, null, 2) + "\n"); + // Create the staging dir that tar would have created + mkdirSync(extractedPath, { recursive: true }); + + return { archivePath, metadataPath, extractedPath }; +} + +let consoleLogSpy: ReturnType; +let consoleErrorSpy: ReturnType; +let exitSpy: ReturnType; + +beforeEach(() => { + // Route lockfile and retired archives to the temp directory + process.env.VELLUM_LOCKFILE_DIR = testDir; + process.env.XDG_DATA_HOME = testDir; + // Write an empty lockfile so saveAssistantEntry has a dir to write to + mkdirSync(testDir, { recursive: true }); + writeFileSync( + join(testDir, ".vellum.lock.json"), + JSON.stringify({ assistants: [] }) + "\n", + ); + + consoleLogSpy = spyOn(console, "log").mockImplementation(() => {}); + consoleErrorSpy = spyOn(console, "error").mockImplementation(() => {}); + exitSpy = spyOn(process, "exit").mockImplementation((_code?: number) => { + throw new Error(`process.exit(${_code})`); + }); + + execMock.mockClear(); + startLocalDaemonMock.mockClear(); + startGatewayMock.mockClear(); +}); + +afterEach(() => { + process.argv = [...originalArgv]; + process.env.VELLUM_LOCKFILE_DIR = originalLockfileDir; + process.env.XDG_DATA_HOME = originalXdgData; + consoleLogSpy.mockRestore(); + consoleErrorSpy.mockRestore(); + exitSpy.mockRestore(); + // Clean up per-test artifacts inside testDir/vellum/ + if (existsSync(join(testDir, "vellum"))) { + rmSync(join(testDir, "vellum"), { recursive: true, force: true }); + } +}); + +// Runs after all tests finish +import { afterAll } from "bun:test"; +afterAll(() => { + rmSync(testDir, { recursive: true, force: true }); + process.argv = [...originalArgv]; + if (originalLockfileDir !== undefined) { + process.env.VELLUM_LOCKFILE_DIR = originalLockfileDir; + } else { + delete process.env.VELLUM_LOCKFILE_DIR; + } + if (originalXdgData !== undefined) { + process.env.XDG_DATA_HOME = originalXdgData; + } else { + delete process.env.XDG_DATA_HOME; + } +}); + +// ── Tests ───────────────────────────────────────────────────────────────────── + +describe("recover --help", () => { + test("prints usage, description, and examples then exits 0", async () => { + process.argv = ["bun", "vellum", "recover", "--help"]; + await expect(recover()).rejects.toThrow("process.exit(0)"); + const output = consoleLogSpy.mock.calls.flat().join("\n"); + expect(output).toContain("Usage: vellum recover "); + expect(output).toContain("Examples:"); + expect(output).toContain("vellum recover"); + }); +}); + +describe("recover error cases", () => { + test("exits 1 when no name is given", async () => { + process.argv = ["bun", "vellum", "recover"]; + await expect(recover()).rejects.toThrow("process.exit(1)"); + expect(consoleErrorSpy.mock.calls[0][0]).toContain("Usage:"); + }); + + test("exits 1 when archive is missing", async () => { + process.argv = ["bun", "vellum", "recover", "ghost-assistant"]; + await expect(recover()).rejects.toThrow("process.exit(1)"); + expect(consoleErrorSpy.mock.calls[0][0]).toContain( + "No retired archive found for 'ghost-assistant'", + ); + }); + + test("throws when metadata has no resources", async () => { + const name = "no-resources"; + mkdirSync(retiredDir, { recursive: true }); + writeFileSync(join(retiredDir, `${name}.tar.gz`), ""); + const entry: Partial = { + assistantId: name, + runtimeUrl: "http://127.0.0.1:7831", + cloud: "local", + // resources intentionally omitted + }; + writeFileSync( + join(retiredDir, `${name}.json`), + JSON.stringify(entry, null, 2) + "\n", + ); + process.argv = ["bun", "vellum", "recover", name]; + await expect(recover()).rejects.toThrow("missing resource configuration"); + }); + + test("exits 1 when target .vellum/ already exists", async () => { + const name = "already-exists"; + const instanceDir = join(testDir, name); + const entry = makeEntry(name, instanceDir); + writeArchiveFixtures(name, entry); + // Pre-create the collision path that recover checks + mkdirSync(join(instanceDir, ".vellum"), { recursive: true }); + process.argv = ["bun", "vellum", "recover", name]; + await expect(recover()).rejects.toThrow("process.exit(1)"); + expect(consoleErrorSpy.mock.calls[0][0]).toContain("already exists"); + }); +}); + +describe("recover extraction path — default instance (instanceDir === homedir())", () => { + test("extracts to retiredDir and renames staging dir to instanceDir/.vellum", async () => { + const name = "default-instance"; + // Default instance: instanceDir is the real home directory + const entry = makeEntry(name, homedir()); + const { archivePath, extractedPath } = writeArchiveFixtures(name, entry); + + const expectedTargetDir = join(homedir(), ".vellum"); + + process.argv = ["bun", "vellum", "recover", name]; + await recover(); + + // exec must have been called with -C retiredDir, NOT -C homedir() + expect(execMock).toHaveBeenCalledTimes(1); + const [cmd, args] = execMock.mock.calls[0] as [string, string[]]; + expect(cmd).toBe("tar"); + expect(args).toContain("-C"); + const cIndex = args.indexOf("-C"); + expect(args[cIndex + 1]).toBe(retiredDir); + expect(args[cIndex + 1]).not.toBe(homedir()); + + // Staging dir was renamed to the correct target + expect(existsSync(extractedPath)).toBe(false); + expect(existsSync(expectedTargetDir)).toBe(true); + + // Archive and metadata were cleaned up + expect(existsSync(archivePath)).toBe(false); + + // Daemon and gateway were started + expect(startLocalDaemonMock).toHaveBeenCalledTimes(1); + expect(startGatewayMock).toHaveBeenCalledTimes(1); + + // Clean up so we don't leave a .vellum dir in the real home dir + rmSync(expectedTargetDir, { recursive: true, force: true }); + }); +}); + +describe("recover extraction path — named instance (instanceDir !== homedir())", () => { + test("extracts to retiredDir and renames staging dir to instanceDir directly", async () => { + const name = "named-instance"; + const instanceDir = join(testDir, "custom-location", name); + const entry = makeEntry(name, instanceDir); + const { archivePath, extractedPath } = writeArchiveFixtures(name, entry); + + // Named instance: targetDir is instanceDir itself (not instanceDir/.vellum) + const expectedTargetDir = instanceDir; + // Parent dir must exist for renameSync + mkdirSync(join(testDir, "custom-location"), { recursive: true }); + + process.argv = ["bun", "vellum", "recover", name]; + await recover(); + + // exec must have been called with -C retiredDir + expect(execMock).toHaveBeenCalledTimes(1); + const [cmd, args] = execMock.mock.calls[0] as [string, string[]]; + expect(cmd).toBe("tar"); + const cIndex = args.indexOf("-C"); + expect(args[cIndex + 1]).toBe(retiredDir); + + // Staging dir was renamed to instanceDir (not instanceDir/.vellum) + expect(existsSync(extractedPath)).toBe(false); + expect(existsSync(expectedTargetDir)).toBe(true); + + // Archive cleaned up + expect(existsSync(archivePath)).toBe(false); + + // Daemon and gateway were started + expect(startLocalDaemonMock).toHaveBeenCalledTimes(1); + expect(startGatewayMock).toHaveBeenCalledTimes(1); + }); + + test("creates parent directories of instanceDir when they do not exist", async () => { + const name = "deep-nested-instance"; + // Use a path whose parent directory does not yet exist + const instanceDir = join(testDir, "new-parent", "deeper", name); + const entry = makeEntry(name, instanceDir); + const { archivePath, extractedPath } = writeArchiveFixtures(name, entry); + + process.argv = ["bun", "vellum", "recover", name]; + await recover(); + + expect(existsSync(extractedPath)).toBe(false); + expect(existsSync(instanceDir)).toBe(true); + expect(existsSync(archivePath)).toBe(false); + + rmSync(instanceDir, { recursive: true, force: true }); + }); +}); diff --git a/cli/src/commands/recover.ts b/cli/src/commands/recover.ts index 691013cbe41..88725ad681c 100644 --- a/cli/src/commands/recover.ts +++ b/cli/src/commands/recover.ts @@ -1,6 +1,12 @@ -import { existsSync, readFileSync, unlinkSync } from "fs"; +import { + existsSync, + mkdirSync, + readFileSync, + renameSync, + unlinkSync, +} from "fs"; import { homedir } from "os"; -import { join } from "path"; +import { basename, dirname, join } from "path"; import { saveAssistantEntry } from "../lib/assistant-config"; import type { AssistantEntry } from "../lib/assistant-config"; @@ -21,8 +27,22 @@ export async function recover(): Promise { "Restore a previously retired local assistant from its archive.", ); console.log(""); + console.log( + "Extracts the archived workspace data back to its original location,", + ); + console.log( + "restores the lockfile entry, and starts the assistant and gateway.", + ); + console.log( + "Archives are stored in $XDG_DATA_HOME/vellum/retired/ (default: ~/.local/share/vellum/retired/).", + ); + console.log(""); console.log("Arguments:"); console.log(" Name of the retired assistant to recover"); + console.log(""); + console.log("Examples:"); + console.log(" $ vellum recover my-assistant"); + console.log(" $ vellum recover aria-7f3a"); process.exit(0); } @@ -61,11 +81,27 @@ export async function recover(): Promise { process.exit(1); } - // 4. Extract archive - // TODO: extraction target is hardcoded to homedir(); multi-instance entries - // whose instanceDir differs from homedir will extract to the wrong - // location. Tracked separately from the collision-check regression. - await exec("tar", ["xzf", archivePath, "-C", homedir()]); + // 4. Determine the original target directory, then extract and rename. + // + // retireLocal archives either the full instanceDir (named instances) or just + // the .vellum/ subdirectory (default instance whose instanceDir === homedir()). + // The directory is staged under `.staging` inside the retired dir + // before being packed with `tar -C `, so the + // top-level entry inside the tarball is always `.tar.gz.staging`. + // + // Correct restoration: extract to retiredDir, then rename the staging entry + // back to the original target path. Using homedir() as the -C target was + // wrong for any instance stored outside the home directory. + const isNamedInstance = entry.resources.instanceDir !== homedir(); + const targetDir = isNamedInstance + ? entry.resources.instanceDir + : join(entry.resources.instanceDir, ".vellum"); + const retiredDir = dirname(archivePath); + const extractedPath = join(retiredDir, basename(archivePath) + ".staging"); + + await exec("tar", ["xzf", archivePath, "-C", retiredDir]); + mkdirSync(dirname(targetDir), { recursive: true }); + renameSync(extractedPath, targetDir); // 5. Restore lockfile entry saveAssistantEntry(entry);