refactor(hooks): dedupe install parameter wiring

2026-05-09 16:44:33 +00:00 · 2026-03-03 01:05:17 +00:00
parent 1bd20dbdb6
commit c3d5159121
2 changed files with 177 additions and 143 deletions
--- a/src/hooks/install.test.ts
+++ b/src/hooks/install.test.ts
@@ -87,6 +87,43 @@ function expectInstallFailureContains(
  }
 }

+function writeHookPackManifest(params: {
+  pkgDir: string;
+  hooks: string[];
+  dependencies?: Record<string, string>;
+}) {
+  fs.writeFileSync(
+    path.join(params.pkgDir, "package.json"),
+    JSON.stringify({
+      name: "@openclaw/test-hooks",
+      version: "0.0.1",
+      openclaw: { hooks: params.hooks },
+      ...(params.dependencies ? { dependencies: params.dependencies } : {}),
+    }),
+    "utf-8",
+  );
+}
+
+async function installArchiveFixture(params: { fileName: string; contents: Buffer }) {
+  const fixture = writeArchiveFixture(params);
+  const result = await installHooksFromArchive({
+    archivePath: fixture.archivePath,
+    hooksDir: fixture.hooksDir,
+  });
+  return { fixture, result };
+}
+
+function expectPathInstallFailureContains(
+  result: Awaited<ReturnType<typeof installHooksFromPath>>,
+  snippet: string,
+) {
+  expect(result.ok).toBe(false);
+  if (result.ok) {
+    throw new Error("expected install failure");
+  }
+  expect(result.error).toContain(snippet);
+}
+
 describe("installHooksFromArchive", () => {
  it.each([
    {
@@ -104,10 +141,9 @@ describe("installHooksFromArchive", () => {
      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,
+    const { fixture, result } = await installArchiveFixture({
+      fileName: tc.fileName,
+      contents: tc.contents,
    });

    expect(result.ok).toBe(true);
@@ -136,10 +172,9 @@ describe("installHooksFromArchive", () => {
      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,
+    const { result } = await installArchiveFixture({
+      fileName: tc.fileName,
+      contents: tc.contents,
    });
    expectInstallFailureContains(result, ["failed to extract archive", tc.expectedDetail]);
  });
@@ -154,10 +189,9 @@ describe("installHooksFromArchive", () => {
      contents: tarReservedIdBuffer,
    },
  ])("rejects hook packs with $name", async (tc) => {
-    const fixture = writeArchiveFixture({ fileName: "hooks.tar", contents: tc.contents });
-    const result = await installHooksFromArchive({
-      archivePath: fixture.archivePath,
-      hooksDir: fixture.hooksDir,
+    const { result } = await installArchiveFixture({
+      fileName: "hooks.tar",
+      contents: tc.contents,
    });
    expectInstallFailureContains(result, ["reserved path segment"]);
  });
@@ -169,16 +203,11 @@ describe("installHooksFromPath", () => {
    const stateDir = makeTempDir();
    const pkgDir = path.join(workDir, "package");
    fs.mkdirSync(path.join(pkgDir, "hooks", "one-hook"), { recursive: true });
-    fs.writeFileSync(
-      path.join(pkgDir, "package.json"),
-      JSON.stringify({
-        name: "@openclaw/test-hooks",
-        version: "0.0.1",
-        openclaw: { hooks: ["./hooks/one-hook"] },
-        dependencies: { "left-pad": "1.3.0" },
-      }),
-      "utf-8",
-    );
+    writeHookPackManifest({
+      pkgDir,
+      hooks: ["./hooks/one-hook"],
+      dependencies: { "left-pad": "1.3.0" },
+    });
    fs.writeFileSync(
      path.join(pkgDir, "hooks", "one-hook", "HOOK.md"),
      [
@@ -249,15 +278,10 @@ describe("installHooksFromPath", () => {
    const outsideHookDir = path.join(workDir, "outside");
    fs.mkdirSync(pkgDir, { recursive: true });
    fs.mkdirSync(outsideHookDir, { recursive: true });
-    fs.writeFileSync(
-      path.join(pkgDir, "package.json"),
-      JSON.stringify({
-        name: "@openclaw/test-hooks",
-        version: "0.0.1",
-        openclaw: { hooks: ["../outside"] },
-      }),
-      "utf-8",
-    );
+    writeHookPackManifest({
+      pkgDir,
+      hooks: ["../outside"],
+    });
    fs.writeFileSync(path.join(outsideHookDir, "HOOK.md"), "---\nname: outside\n---\n", "utf-8");
    fs.writeFileSync(path.join(outsideHookDir, "handler.ts"), "export default async () => {};\n");

@@ -266,11 +290,7 @@ describe("installHooksFromPath", () => {
      hooksDir: path.join(stateDir, "hooks"),
    });

-    expect(result.ok).toBe(false);
-    if (result.ok) {
-      return;
-    }
-    expect(result.error).toContain("openclaw.hooks entry escapes package directory");
+    expectPathInstallFailureContains(result, "openclaw.hooks entry escapes package directory");
  });

  it("rejects hook pack entries that escape via symlink", async () => {
@@ -288,26 +308,20 @@ describe("installHooksFromPath", () => {
    } catch {
      return;
    }
-    fs.writeFileSync(
-      path.join(pkgDir, "package.json"),
-      JSON.stringify({
-        name: "@openclaw/test-hooks",
-        version: "0.0.1",
-        openclaw: { hooks: ["./linked"] },
-      }),
-      "utf-8",
-    );
+    writeHookPackManifest({
+      pkgDir,
+      hooks: ["./linked"],
+    });

    const result = await installHooksFromPath({
      path: pkgDir,
      hooksDir: path.join(stateDir, "hooks"),
    });

-    expect(result.ok).toBe(false);
-    if (result.ok) {
-      return;
-    }
-    expect(result.error).toContain("openclaw.hooks entry resolves outside package directory");
+    expectPathInstallFailureContains(
+      result,
+      "openclaw.hooks entry resolves outside package directory",
+    );
  });
 });

--- a/src/hooks/install.ts
+++ b/src/hooks/install.ts
@@ -58,6 +58,30 @@ export type HookNpmIntegrityDriftParams = {

 const defaultLogger: HookInstallLogger = {};

+type HookInstallForwardParams = {
+  hooksDir?: string;
+  timeoutMs?: number;
+  logger?: HookInstallLogger;
+  mode?: "install" | "update";
+  dryRun?: boolean;
+  expectedHookPackId?: string;
+};
+
+type HookPackageInstallParams = { packageDir: string } & HookInstallForwardParams;
+type HookArchiveInstallParams = { archivePath: string } & HookInstallForwardParams;
+type HookPathInstallParams = { path: string } & HookInstallForwardParams;
+
+function buildHookInstallForwardParams(params: HookInstallForwardParams): HookInstallForwardParams {
+  return {
+    hooksDir: params.hooksDir,
+    timeoutMs: params.timeoutMs,
+    logger: params.logger,
+    mode: params.mode,
+    dryRun: params.dryRun,
+    expectedHookPackId: params.expectedHookPackId,
+  };
+}
+
 function validateHookId(hookId: string): string | null {
  if (!hookId) {
    return "invalid hook name: missing";
@@ -113,6 +137,54 @@ async function resolveInstallTargetDir(
  });
 }

+async function resolveAvailableHookInstallTarget(params: {
+  id: string;
+  hooksDir?: string;
+  mode: "install" | "update";
+  alreadyExistsError: (targetDir: string) => string;
+}): Promise<{ ok: true; targetDir: string } | { ok: false; error: string }> {
+  const targetDirResult = await resolveInstallTargetDir(params.id, params.hooksDir);
+  if (!targetDirResult.ok) {
+    return targetDirResult;
+  }
+  const targetDir = targetDirResult.targetDir;
+  const availability = await ensureInstallTargetAvailable({
+    mode: params.mode,
+    targetDir,
+    alreadyExistsError: params.alreadyExistsError(targetDir),
+  });
+  if (!availability.ok) {
+    return availability;
+  }
+  return { ok: true, targetDir };
+}
+
+async function installFromResolvedHookDir(
+  resolvedDir: string,
+  params: HookInstallForwardParams,
+): Promise<InstallHooksResult> {
+  const manifestPath = path.join(resolvedDir, "package.json");
+  if (await fileExists(manifestPath)) {
+    return await installHookPackageFromDir({
+      packageDir: resolvedDir,
+      hooksDir: params.hooksDir,
+      timeoutMs: params.timeoutMs,
+      logger: params.logger,
+      mode: params.mode,
+      dryRun: params.dryRun,
+      expectedHookPackId: params.expectedHookPackId,
+    });
+  }
+  return await installHookFromDir({
+    hookDir: resolvedDir,
+    hooksDir: params.hooksDir,
+    logger: params.logger,
+    mode: params.mode,
+    dryRun: params.dryRun,
+    expectedHookPackId: params.expectedHookPackId,
+  });
+}
+
 async function resolveHookNameFromDir(hookDir: string): Promise<string> {
  const hookMdPath = path.join(hookDir, "HOOK.md");
  if (!(await fileExists(hookMdPath))) {
@@ -139,15 +211,9 @@ async function validateHookDir(hookDir: string): Promise<void> {
  }
 }

-async function installHookPackageFromDir(params: {
-  packageDir: string;
-  hooksDir?: string;
-  timeoutMs?: number;
-  logger?: HookInstallLogger;
-  mode?: "install" | "update";
-  dryRun?: boolean;
-  expectedHookPackId?: string;
-}): Promise<InstallHooksResult> {
+async function installHookPackageFromDir(
+  params: HookPackageInstallParams,
+): Promise<InstallHooksResult> {
  const { logger, timeoutMs, mode, dryRun } = resolveTimedInstallModeOptions(params, defaultLogger);

  const manifestPath = path.join(params.packageDir, "package.json");
@@ -182,19 +248,16 @@ async function installHookPackageFromDir(params: {
    };
  }

-  const targetDirResult = await resolveInstallTargetDir(hookPackId, params.hooksDir);
-  if (!targetDirResult.ok) {
-    return { ok: false, error: targetDirResult.error };
-  }
-  const targetDir = targetDirResult.targetDir;
-  const availability = await ensureInstallTargetAvailable({
+  const target = await resolveAvailableHookInstallTarget({
+    id: hookPackId,
+    hooksDir: params.hooksDir,
    mode,
-    targetDir,
-    alreadyExistsError: `hook pack already exists: ${targetDir} (delete it first)`,
+    alreadyExistsError: (targetDir) => `hook pack already exists: ${targetDir} (delete it first)`,
  });
-  if (!availability.ok) {
-    return availability;
+  if (!target.ok) {
+    return target;
  }
+  const targetDir = target.targetDir;

  const resolvedHooks = [] as string[];
  for (const entry of hookEntries) {
@@ -277,19 +340,16 @@ async function installHookFromDir(params: {
    };
  }

-  const targetDirResult = await resolveInstallTargetDir(hookName, params.hooksDir);
-  if (!targetDirResult.ok) {
-    return { ok: false, error: targetDirResult.error };
-  }
-  const targetDir = targetDirResult.targetDir;
-  const availability = await ensureInstallTargetAvailable({
+  const target = await resolveAvailableHookInstallTarget({
+    id: hookName,
+    hooksDir: params.hooksDir,
    mode,
-    targetDir,
-    alreadyExistsError: `hook already exists: ${targetDir} (delete it first)`,
+    alreadyExistsError: (targetDir) => `hook already exists: ${targetDir} (delete it first)`,
  });
-  if (!availability.ok) {
-    return availability;
+  if (!target.ok) {
+    return target;
  }
+  const targetDir = target.targetDir;

  if (dryRun) {
    return { ok: true, hookPackId: hookName, hooks: [hookName], targetDir };
@@ -312,15 +372,9 @@ async function installHookFromDir(params: {
  return { ok: true, hookPackId: hookName, hooks: [hookName], targetDir };
 }

-export async function installHooksFromArchive(params: {
-  archivePath: string;
-  hooksDir?: string;
-  timeoutMs?: number;
-  logger?: HookInstallLogger;
-  mode?: "install" | "update";
-  dryRun?: boolean;
-  expectedHookPackId?: string;
-}): Promise<InstallHooksResult> {
+export async function installHooksFromArchive(
+  params: HookArchiveInstallParams,
+): Promise<InstallHooksResult> {
  const logger = params.logger ?? defaultLogger;
  const timeoutMs = params.timeoutMs ?? 120_000;
  const archivePathResult = await resolveArchiveSourcePath(params.archivePath);
@@ -334,29 +388,18 @@ export async function installHooksFromArchive(params: {
    tempDirPrefix: "openclaw-hook-",
    timeoutMs,
    logger,
-    onExtracted: async (rootDir) => {
-      const manifestPath = path.join(rootDir, "package.json");
-      if (await fileExists(manifestPath)) {
-        return await installHookPackageFromDir({
-          packageDir: rootDir,
+    onExtracted: async (rootDir) =>
+      await installFromResolvedHookDir(
+        rootDir,
+        buildHookInstallForwardParams({
          hooksDir: params.hooksDir,
          timeoutMs,
          logger,
          mode: params.mode,
          dryRun: params.dryRun,
          expectedHookPackId: params.expectedHookPackId,
-        });
-      }
-
-      return await installHookFromDir({
-        hookDir: rootDir,
-        hooksDir: params.hooksDir,
-        logger,
-        mode: params.mode,
-        dryRun: params.dryRun,
-        expectedHookPackId: params.expectedHookPackId,
-      });
-    },
+        }),
+      ),
  });
 }

@@ -386,54 +429,36 @@ export async function installHooksFromNpmSpec(params: {
      logger.warn?.(message);
    },
    installFromArchive: installHooksFromArchive,
-    archiveInstallParams: {
+    archiveInstallParams: buildHookInstallForwardParams({
      hooksDir: params.hooksDir,
      timeoutMs,
      logger,
      mode,
      dryRun,
      expectedHookPackId,
-    },
+    }),
  });
 }

-export async function installHooksFromPath(params: {
-  path: string;
-  hooksDir?: string;
-  timeoutMs?: number;
-  logger?: HookInstallLogger;
-  mode?: "install" | "update";
-  dryRun?: boolean;
-  expectedHookPackId?: string;
-}): Promise<InstallHooksResult> {
+export async function installHooksFromPath(
+  params: HookPathInstallParams,
+): Promise<InstallHooksResult> {
  const pathResult = await resolveExistingInstallPath(params.path);
  if (!pathResult.ok) {
    return pathResult;
  }
  const { resolvedPath: resolved, stat } = pathResult;
+  const forwardParams = buildHookInstallForwardParams({
+    hooksDir: params.hooksDir,
+    timeoutMs: params.timeoutMs,
+    logger: params.logger,
+    mode: params.mode,
+    dryRun: params.dryRun,
+    expectedHookPackId: params.expectedHookPackId,
+  });

  if (stat.isDirectory()) {
-    const manifestPath = path.join(resolved, "package.json");
-    if (await fileExists(manifestPath)) {
-      return await installHookPackageFromDir({
-        packageDir: resolved,
-        hooksDir: params.hooksDir,
-        timeoutMs: params.timeoutMs,
-        logger: params.logger,
-        mode: params.mode,
-        dryRun: params.dryRun,
-        expectedHookPackId: params.expectedHookPackId,
-      });
-    }
-
-    return await installHookFromDir({
-      hookDir: resolved,
-      hooksDir: params.hooksDir,
-      logger: params.logger,
-      mode: params.mode,
-      dryRun: params.dryRun,
-      expectedHookPackId: params.expectedHookPackId,
-    });
+    return await installFromResolvedHookDir(resolved, forwardParams);
  }

  if (!resolveArchiveKind(resolved)) {
@@ -442,11 +467,6 @@ export async function installHooksFromPath(params: {

  return await installHooksFromArchive({
    archivePath: resolved,
-    hooksDir: params.hooksDir,
-    timeoutMs: params.timeoutMs,
-    logger: params.logger,
-    mode: params.mode,
-    dryRun: params.dryRun,
-    expectedHookPackId: params.expectedHookPackId,
+    ...forwardParams,
  });
 }