refactor(hooks): share install temp-dir and archive fixtures

src/hooks/install.test.ts

@@ -45,102 +45,96 @@ beforeEach(() => {
   vi.clearAllMocks();
 });
 
+function writeArchiveFixture(params: { fileName: string; contents: Buffer }) {
+  const stateDir = makeTempDir();
+  const workDir = makeTempDir();
+  const archivePath = path.join(workDir, params.fileName);
+  fs.writeFileSync(archivePath, params.contents);
+  return {
+    stateDir,
+    archivePath,
+    hooksDir: path.join(stateDir, "hooks"),
+  };
+}
+
 describe("installHooksFromArchive", () => {
-  it("installs hook packs from zip archives", async () => {
+  it.each([
-    const stateDir = makeTempDir();
+    {
-    const workDir = makeTempDir();
+      name: "zip",
-    const archivePath = path.join(workDir, "hooks.zip");
+      fileName: "hooks.zip",
-    fs.writeFileSync(archivePath, zipHooksBuffer);
+      contents: zipHooksBuffer,
-
+      expectedPackId: "zip-hooks",
-    const hooksDir = path.join(stateDir, "hooks");
+      expectedHook: "zip-hook",
-    const result = await installHooksFromArchive({ archivePath, hooksDir });
+    },
+    {
+      name: "tar",
+      fileName: "hooks.tar",
+      contents: tarHooksBuffer,
+      expectedPackId: "tar-hooks",
+      expectedHook: "tar-hook",
+    },
+  ])("installs hook packs from $name archives", async (tc) => {
+    const fixture = writeArchiveFixture({ fileName: tc.fileName, contents: tc.contents });
+    const result = await installHooksFromArchive({
+      archivePath: fixture.archivePath,
+      hooksDir: fixture.hooksDir,
+    });
 
     expect(result.ok).toBe(true);
     if (!result.ok) {
       return;
     }
-    expect(result.hookPackId).toBe("zip-hooks");
+    expect(result.hookPackId).toBe(tc.expectedPackId);
-    expect(result.hooks).toContain("zip-hook");
+    expect(result.hooks).toContain(tc.expectedHook);
-    expect(result.targetDir).toBe(path.join(stateDir, "hooks", "zip-hooks"));
+    expect(result.targetDir).toBe(path.join(fixture.stateDir, "hooks", tc.expectedPackId));
-    expect(fs.existsSync(path.join(result.targetDir, "hooks", "zip-hook", "HOOK.md"))).toBe(true);
+    expect(fs.existsSync(path.join(result.targetDir, "hooks", tc.expectedHook, "HOOK.md"))).toBe(
+      true,
+    );
   });
 
-  it("rejects zip archives with traversal entries", async () => {
+  it.each([
-    const stateDir = makeTempDir();
+    {
-    const workDir = makeTempDir();
+      name: "zip",
-    const archivePath = path.join(workDir, "traversal.zip");
+      fileName: "traversal.zip",
-    fs.writeFileSync(archivePath, zipTraversalBuffer);
+      contents: zipTraversalBuffer,
-
+      expectedDetail: "archive entry",
-    const hooksDir = path.join(stateDir, "hooks");
+    },
-    const result = await installHooksFromArchive({ archivePath, hooksDir });
+    {
+      name: "tar",
+      fileName: "traversal.tar",
+      contents: tarTraversalBuffer,
+      expectedDetail: "escapes destination",
+    },
+  ])("rejects $name archives with traversal entries", async (tc) => {
+    const fixture = writeArchiveFixture({ fileName: tc.fileName, contents: tc.contents });
+    const result = await installHooksFromArchive({
+      archivePath: fixture.archivePath,
+      hooksDir: fixture.hooksDir,
+    });
 
     expect(result.ok).toBe(false);
     if (result.ok) {
       return;
     }
     expect(result.error).toContain("failed to extract archive");
-    expect(result.error).toContain("archive entry");
+    expect(result.error).toContain(tc.expectedDetail);
   });
 
-  it("installs hook packs from tar archives", async () => {
+  it.each([
-    const stateDir = makeTempDir();
+    {
-    const workDir = makeTempDir();
+      name: "traversal-like ids",
-    const archivePath = path.join(workDir, "hooks.tar");
+      contents: tarEvilIdBuffer,
-    fs.writeFileSync(archivePath, tarHooksBuffer);
+    },
-
+    {
-    const hooksDir = path.join(stateDir, "hooks");
+      name: "reserved ids",
-    const result = await installHooksFromArchive({ archivePath, hooksDir });
+      contents: tarReservedIdBuffer,
-
+    },
-    expect(result.ok).toBe(true);
+  ])("rejects hook packs with $name", async (tc) => {
-    if (!result.ok) {
+    const fixture = writeArchiveFixture({ fileName: "hooks.tar", contents: tc.contents });
-      return;
+    const result = await installHooksFromArchive({
-    }
+      archivePath: fixture.archivePath,
-    expect(result.hookPackId).toBe("tar-hooks");
+      hooksDir: fixture.hooksDir,
-    expect(result.hooks).toContain("tar-hook");
+    });
-    expect(result.targetDir).toBe(path.join(stateDir, "hooks", "tar-hooks"));
-  });
-
-  it("rejects tar archives with traversal entries", async () => {
-    const stateDir = makeTempDir();
-    const workDir = makeTempDir();
-    const archivePath = path.join(workDir, "traversal.tar");
-    fs.writeFileSync(archivePath, tarTraversalBuffer);
-
-    const hooksDir = path.join(stateDir, "hooks");
-    const result = await installHooksFromArchive({ archivePath, hooksDir });
-
-    expect(result.ok).toBe(false);
-    if (result.ok) {
-      return;
-    }
-    expect(result.error).toContain("failed to extract archive");
-    expect(result.error).toContain("escapes destination");
-  });
-
-  it("rejects hook packs with traversal-like ids", async () => {
-    const stateDir = makeTempDir();
-    const workDir = makeTempDir();
-    const archivePath = path.join(workDir, "hooks.tar");
-    fs.writeFileSync(archivePath, tarEvilIdBuffer);
-
-    const hooksDir = path.join(stateDir, "hooks");
-    const result = await installHooksFromArchive({ archivePath, hooksDir });
-
-    expect(result.ok).toBe(false);
-    if (result.ok) {
-      return;
-    }
-    expect(result.error).toContain("reserved path segment");
-  });
-
-  it("rejects hook packs with reserved ids", async () => {
-    const stateDir = makeTempDir();
-    const workDir = makeTempDir();
-    const archivePath = path.join(workDir, "hooks.tar");
-    fs.writeFileSync(archivePath, tarReservedIdBuffer);
-
-    const hooksDir = path.join(stateDir, "hooks");
-    const result = await installHooksFromArchive({ archivePath, hooksDir });
 
     expect(result.ok).toBe(false);
     if (result.ok) {
@@ -201,9 +195,7 @@ describe("installHooksFromPath", () => {
       expectedCwd: res.targetDir,
     });
   });
-});
 
-describe("installHooksFromPath", () => {
   it("installs a single hook directory", async () => {
     const stateDir = makeTempDir();
     const workDir = makeTempDir();

src/hooks/install.ts

@@ -105,6 +105,33 @@ function resolveTimedHookInstallModeOptions(params: {
   };
 }
 
+async function withTempDir<T>(prefix: string, fn: (tmpDir: string) => Promise<T>): Promise<T> {
+  const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), prefix));
+  try {
+    return await fn(tmpDir);
+  } finally {
+    await fs.rm(tmpDir, { recursive: true, force: true }).catch(() => undefined);
+  }
+}
+
+async function resolveInstallTargetDir(
+  id: string,
+  hooksDir?: string,
+): Promise<{ ok: true; targetDir: string } | { ok: false; error: string }> {
+  const baseHooksDir = hooksDir ? resolveUserPath(hooksDir) : path.join(CONFIG_DIR, "hooks");
+  await fs.mkdir(baseHooksDir, { recursive: true });
+
+  const targetDirResult = resolveSafeInstallDir({
+    baseDir: baseHooksDir,
+    id,
+    invalidNameMessage: "invalid hook name: path traversal detected",
+  });
+  if (!targetDirResult.ok) {
+    return { ok: false, error: targetDirResult.error };
+  }
+  return { ok: true, targetDir: targetDirResult.path };
+}
+
 async function resolveHookNameFromDir(hookDir: string): Promise<string> {
   const hookMdPath = path.join(hookDir, "HOOK.md");
   if (!(await fileExists(hookMdPath))) {
@@ -174,20 +201,11 @@ async function installHookPackageFromDir(params: {
     };
   }
 
-  const hooksDir = params.hooksDir
+  const targetDirResult = await resolveInstallTargetDir(hookPackId, params.hooksDir);
-    ? resolveUserPath(params.hooksDir)
-    : path.join(CONFIG_DIR, "hooks");
-  await fs.mkdir(hooksDir, { recursive: true });
-
-  const targetDirResult = resolveSafeInstallDir({
-    baseDir: hooksDir,
-    id: hookPackId,
-    invalidNameMessage: "invalid hook name: path traversal detected",
-  });
   if (!targetDirResult.ok) {
     return { ok: false, error: targetDirResult.error };
   }
-  const targetDir = targetDirResult.path;
+  const targetDir = targetDirResult.targetDir;
   if (mode === "install" && (await fileExists(targetDir))) {
     return { ok: false, error: `hook pack already exists: ${targetDir} (delete it first)` };
   }
@@ -259,20 +277,11 @@ async function installHookFromDir(params: {
     };
   }
 
-  const hooksDir = params.hooksDir
+  const targetDirResult = await resolveInstallTargetDir(hookName, params.hooksDir);
-    ? resolveUserPath(params.hooksDir)
-    : path.join(CONFIG_DIR, "hooks");
-  await fs.mkdir(hooksDir, { recursive: true });
-
-  const targetDirResult = resolveSafeInstallDir({
-    baseDir: hooksDir,
-    id: hookName,
-    invalidNameMessage: "invalid hook name: path traversal detected",
-  });
   if (!targetDirResult.ok) {
     return { ok: false, error: targetDirResult.error };
   }
-  const targetDir = targetDirResult.path;
+  const targetDir = targetDirResult.targetDir;
   if (mode === "install" && (await fileExists(targetDir))) {
     return { ok: false, error: `hook already exists: ${targetDir} (delete it first)` };
   }
@@ -326,8 +335,7 @@ export async function installHooksFromArchive(params: {
     return { ok: false, error: `unsupported archive: ${archivePath}` };
   }
 
-  const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-hook-"));
+  return await withTempDir("openclaw-hook-", async (tmpDir) => {
-  try {
     const extractDir = path.join(tmpDir, "extract");
     await fs.mkdir(extractDir, { recursive: true });
 
@@ -366,9 +374,7 @@ export async function installHooksFromArchive(params: {
       dryRun: params.dryRun,
       expectedHookPackId: params.expectedHookPackId,
     });
-  } finally {
+  });
-    await fs.rm(tmpDir, { recursive: true, force: true }).catch(() => undefined);
-  }
 }
 
 export async function installHooksFromNpmSpec(params: {
@@ -388,8 +394,7 @@ export async function installHooksFromNpmSpec(params: {
     return { ok: false, error: specError };
   }
 
-  const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-hook-pack-"));
+  return await withTempDir("openclaw-hook-pack-", async (tmpDir) => {
-  try {
     logger.info?.(`Downloading ${spec}…`);
     const res = await runCommandWithTimeout(["npm", "pack", spec, "--ignore-scripts"], {
       timeoutMs: Math.max(timeoutMs, 300_000),
@@ -422,9 +427,7 @@ export async function installHooksFromNpmSpec(params: {
       dryRun,
       expectedHookPackId,
     });
-  } finally {
+  });
-    await fs.rm(tmpDir, { recursive: true, force: true }).catch(() => undefined);
-  }
 }
 
 export async function installHooksFromPath(params: {