fix(hooks): bound hook workspace manifest and HOOK.md reads (#101472)

cxbAsDev · steipete · web-flow · commit 4b2b5bcf8955 · 2026-07-17T11:23:21.000+01:00
* fix(hooks): bound hook workspace manifest and HOOK.md reads

* fix(hooks): enforce maxBytes while reading hook metadata fd

ClawSweeper review noted that openRootFileSync validates stat size
before returning the fd, but the subsequent fs.readFileSync of the fd
was unbounded. A file that grows after validation could still OOM
workspace discovery. Replace the full fd read with a chunked bounded
reader that throws once maxBytes is exceeded, and add regression
coverage for the overflow case.

* test(hooks): cover bounded hook reads through public workspace API

* refactor(hooks): use canonical bounded fd reader for hook metadata

* fix(hooks): warn and skip oversized hook metadata during discovery

ClawSweeper review required the maintainer-selected oversized-metadata
diagnostic contract: warn and skip. readRootFileUtf8 now catches the
RangeError from the canonical readFileDescriptorBoundedSync helper and
emits one warning per oversized package.json/HOOK.md identifying the
path and the byte limit, while discovery continues for other hooks.

The redundant open-time maxBytes stat check is removed so the shared
bounded reader is the single owner of the byte cap; overflow always
surfaces as RangeError, keeping the warning accurate for both static
oversized files and post-open growth.

Add focused tests: warning content for both oversized surfaces,
continued discovery alongside an oversized hook, exact-limit acceptance,
and the plain-hook fallback when a package manifest is oversized.

* chore: retrigger CI after rebase

* refactor(hooks): tighten bounded metadata coverage

---------

Co-authored-by: Peter Steinberger &lt;steipete@gmail.com&gt;
diff --git a/src/hooks/workspace.test.ts b/src/hooks/workspace.test.ts
@@ -2,10 +2,16 @@
 import fs from "node:fs";
 import os from "node:os";
 import path from "node:path";
-import { describe, expect, it } from "vitest";
+import { beforeEach, describe, expect, it, vi } from "vitest";
 import { MANIFEST_KEY } from "../compat/legacy-names.js";
 import { loadWorkspaceHookEntries } from "./workspace.js";
 
+const { warnMock } = vi.hoisted(() => ({ warnMock: vi.fn() }));
+
+vi.mock("../logging/subsystem.js", () => ({
+  createSubsystemLogger: () => ({ warn: warnMock }),
+}));
+
 function writeHookPackageManifest(pkgDir: string, hooks: string[]): void {
   fs.writeFileSync(
     path.join(pkgDir, "package.json"),
@@ -62,7 +68,38 @@ function loadWorkspaceEntriesFromHooksRoot(hooksRoot: string) {
   });
 }
 
+const METADATA_MAX_BYTES = 1024 * 1024;
+
+function writePlainHook(hooksRoot: string, name: string, content?: string): string {
+  const hookDir = path.join(hooksRoot, name);
+  fs.mkdirSync(hookDir, { recursive: true });
+  fs.writeFileSync(path.join(hookDir, "HOOK.md"), content ?? `---\nname: ${name}\n---\n`);
+  fs.writeFileSync(path.join(hookDir, "handler.js"), "export default async () => {};\n");
+  return hookDir;
+}
+
+function oversizedMetadataWarnings(filePath: string): string[] {
+  return warnMock.mock.calls
+    .map(([message]) => String(message))
+    .filter((message) => message.includes(filePath) && message.includes(`${METADATA_MAX_BYTES}`));
+}
+
+function padToExactBytes(content: string, targetBytes: number): string {
+  const padding = targetBytes - Buffer.byteLength(content, "utf8");
+  return padding > 0 ? content + " ".repeat(padding) : content;
+}
+
+function exactSizeHookPackageManifest(targetBytes: number): string {
+  const base = { name: "pkg", [MANIFEST_KEY]: { hooks: ["./nested"] }, pad: "" };
+  const baseBytes = Buffer.byteLength(JSON.stringify(base), "utf8");
+  return JSON.stringify({ ...base, pad: "x".repeat(targetBytes - baseBytes) });
+}
+
 describe("hooks workspace", () => {
+  beforeEach(() => {
+    warnMock.mockClear();
+  });
+
   it("ignores package.json hook paths that traverse outside package directory", () => {
     const root = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-hooks-workspace-"));
     const hooksRoot = path.join(root, "hooks");
@@ -100,6 +137,63 @@ describe("hooks workspace", () => {
     expect(hookNames(entries)).toContain("nested");
   });
 
+  it("warns, skips oversized metadata, and continues discovering other hooks", () => {
+    const root = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-hooks-oversized-mixed-"));
+    const hooksRoot = path.join(root, "hooks");
+    fs.mkdirSync(hooksRoot, { recursive: true });
+
+    const packageDir = path.join(hooksRoot, "big-package");
+    fs.mkdirSync(packageDir);
+    const manifestPath = path.join(packageDir, "package.json");
+    fs.writeFileSync(manifestPath, "x".repeat(METADATA_MAX_BYTES + 1));
+
+    const bigHookDir = writePlainHook(hooksRoot, "big-hook", "x".repeat(METADATA_MAX_BYTES + 1));
+    const bigHookMdPath = path.join(bigHookDir, "HOOK.md");
+    writePlainHook(hooksRoot, "small-hook");
+
+    const entries = loadWorkspaceEntriesFromHooksRoot(hooksRoot);
+    expect(hookNames(entries)).toEqual(["small-hook"]);
+    expect(oversizedMetadataWarnings(manifestPath)).toHaveLength(1);
+    expect(oversizedMetadataWarnings(bigHookMdPath)).toHaveLength(1);
+  });
+
+  it("loads hooks whose metadata sits exactly at the byte limit", () => {
+    const root = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-hooks-exact-limit-"));
+    const hooksRoot = path.join(root, "hooks");
+    fs.mkdirSync(hooksRoot, { recursive: true });
+
+    const pkgDir = path.join(hooksRoot, "pkg");
+    const nested = path.join(pkgDir, "nested");
+    fs.mkdirSync(nested, { recursive: true });
+    fs.writeFileSync(
+      path.join(pkgDir, "package.json"),
+      exactSizeHookPackageManifest(METADATA_MAX_BYTES),
+    );
+    fs.writeFileSync(
+      path.join(nested, "HOOK.md"),
+      padToExactBytes("---\nname: exact-limit\n---\n", METADATA_MAX_BYTES),
+    );
+    fs.writeFileSync(path.join(nested, "handler.js"), "export default async () => {};\n");
+
+    const entries = loadWorkspaceEntriesFromHooksRoot(hooksRoot);
+    expect(hookNames(entries)).toContain("exact-limit");
+    expect(warnMock).not.toHaveBeenCalled();
+  });
+
+  it("still loads a plain hook when its package.json is oversized", () => {
+    const root = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-hooks-oversized-compat-"));
+    const hooksRoot = path.join(root, "hooks");
+    fs.mkdirSync(hooksRoot, { recursive: true });
+
+    const hookDir = writePlainHook(hooksRoot, "compat-hook");
+    const manifestPath = path.join(hookDir, "package.json");
+    fs.writeFileSync(manifestPath, "x".repeat(METADATA_MAX_BYTES + 1), "utf8");
+
+    const entries = loadWorkspaceEntriesFromHooksRoot(hooksRoot);
+    expect(hookNames(entries)).toContain("compat-hook");
+    expect(oversizedMetadataWarnings(manifestPath)).toHaveLength(1);
+  });
+
   it("ignores package.json hook paths that escape via symlink", () => {
     const root = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-hooks-workspace-link-"));
     const hooksRoot = path.join(root, "hooks");
diff --git a/src/hooks/workspace.ts b/src/hooks/workspace.ts
@@ -5,6 +5,7 @@ import { normalizeTrimmedStringList } from "@openclaw/normalization-core/string-
 import { MANIFEST_KEY } from "../compat/legacy-names.js";
 import type { OpenClawConfig } from "../config/types.openclaw.js";
 import { openRootFileSync } from "../infra/boundary-file-read.js";
+import { readFileDescriptorBoundedSync } from "../infra/file-descriptor-read.js";
 import { createSubsystemLogger } from "../logging/subsystem.js";
 import { isPathInsideWithRealpath } from "../security/scan-paths.js";
 import { CONFIG_DIR, resolveUserPath } from "../utils.js";
@@ -18,6 +19,10 @@ import { resolvePluginHookDirs } from "./plugin-hooks.js";
 import { resolveHookEntries } from "./policy.js";
 import type { Hook, HookEntry, HookSource, ParsedHookFrontmatter } from "./types.js";
 
+// Hook descriptors are small metadata. Bounding the pinned descriptor read also
+// covers files that grow after the boundary open validates their identity.
+const HOOK_METADATA_MAX_BYTES = 1024 * 1024;
+
 type HookPackageManifest = {
   name?: string;
 } & Partial<Record<typeof MANIFEST_KEY, { hooks?: string[] }>>;
@@ -34,6 +39,7 @@ function readHookPackageManifest(dir: string): HookPackageManifest | null {
     absolutePath: manifestPath,
     rootPath: dir,
     boundaryLabel: "hook package directory",
+    maxBytes: HOOK_METADATA_MAX_BYTES,
   });
   if (raw === null) {
     return null;
@@ -73,6 +79,7 @@ function loadHookFromDir(params: {
     absolutePath: hookMdPath,
     rootPath: params.hookDir,
     boundaryLabel: "hook directory",
+    maxBytes: HOOK_METADATA_MAX_BYTES,
   });
   if (content === null) {
     return null;
@@ -293,11 +300,17 @@ function readRootFileUtf8(params: {
   absolutePath: string;
   rootPath: string;
   boundaryLabel: string;
+  maxBytes: number;
 }): string | null {
   return withOpenedRootFileSync(params, (opened) => {
     try {
-      return fs.readFileSync(opened.fd, "utf-8");
-    } catch {
+      return readFileDescriptorBoundedSync(opened.fd, params.maxBytes).toString("utf-8");
+    } catch (err) {
+      if (err instanceof RangeError) {
+        log.warn(
+          `Ignoring oversized hook metadata ${params.absolutePath}: file exceeds the ${params.maxBytes}-byte limit`,
+        );
+      }
       return null;
     }
   });
