diff --git a/src/hooks/install.test.ts b/src/hooks/install.test.ts index 1f4dd0a4f68..ad179d5af21 100644 --- 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; +}) { + 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>, + 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", + ); }); }); diff --git a/src/hooks/install.ts b/src/hooks/install.ts index 38992d33c73..87aed5b0c23 100644 --- 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 { + 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 { const hookMdPath = path.join(hookDir, "HOOK.md"); if (!(await fileExists(hookMdPath))) { @@ -139,15 +211,9 @@ async function validateHookDir(hookDir: string): Promise { } } -async function installHookPackageFromDir(params: { - packageDir: string; - hooksDir?: string; - timeoutMs?: number; - logger?: HookInstallLogger; - mode?: "install" | "update"; - dryRun?: boolean; - expectedHookPackId?: string; -}): Promise { +async function installHookPackageFromDir( + params: HookPackageInstallParams, +): Promise { 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 { +export async function installHooksFromArchive( + params: HookArchiveInstallParams, +): Promise { 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 { +export async function installHooksFromPath( + params: HookPathInstallParams, +): Promise { 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, }); }