refactor(sandbox): clarify fs bridge read and shell plans

src/agents/sandbox/fs-bridge-path-safety.ts

@@ -49,19 +49,17 @@ export class SandboxFsPathGuard {
   }
 
   async assertPathSafety(target: SandboxResolvedFsPath, options: PathSafetyOptions) {
-    const lexicalMount = this.resolveRequiredMount(target.containerPath, options.action);
-    await this.assertPathSafetyWithinMount(target, options, lexicalMount);
+    const guarded = await this.openBoundaryWithinRequiredMount(target, options.action, {
+      aliasPolicy: options.aliasPolicy,
+      allowedType: options.allowedType,
+    });
+    await this.assertGuardedPathSafety(target, options, guarded);
   }
 
   async openReadableFile(
     target: SandboxResolvedFsPath,
   ): Promise<BoundaryFileOpenResult & { ok: true }> {
-    const lexicalMount = this.resolveRequiredMount(target.containerPath, "read files");
-    const opened = await openBoundaryFile({
-      absolutePath: target.hostPath,
-      rootPath: lexicalMount.hostRoot,
-      boundaryLabel: "sandbox mount root",
-    });
+    const opened = await this.openBoundaryWithinRequiredMount(target, "read files");
     if (!opened.ok) {
       throw opened.error instanceof Error
         ? opened.error
@@ -78,18 +76,11 @@ export class SandboxFsPathGuard {
     return lexicalMount;
   }
 
-  private async assertPathSafetyWithinMount(
+  private async assertGuardedPathSafety(
     target: SandboxResolvedFsPath,
     options: PathSafetyOptions,
-    lexicalMount: SandboxFsMount,
+    guarded: BoundaryFileOpenResult,
   ) {
-    const guarded = await openBoundaryFile({
-      absolutePath: target.hostPath,
-      rootPath: lexicalMount.hostRoot,
-      boundaryLabel: "sandbox mount root",
-      aliasPolicy: options.aliasPolicy,
-      allowedType: options.allowedType,
-    });
     if (!guarded.ok) {
       if (guarded.reason !== "path") {
         const canFallbackToDirectoryStat =
@@ -110,12 +101,7 @@ export class SandboxFsPathGuard {
       containerPath: target.containerPath,
       allowFinalSymlinkForUnlink: options.aliasPolicy?.allowFinalSymlinkForUnlink === true,
     });
-    const canonicalMount = this.resolveMountByContainerPath(canonicalContainerPath);
-    if (!canonicalMount) {
-      throw new Error(
-        `Sandbox path escapes allowed mounts; cannot ${options.action}: ${target.containerPath}`,
-      );
-    }
+    const canonicalMount = this.resolveRequiredMount(canonicalContainerPath, options.action);
     if (options.requireWritable && !canonicalMount.writable) {
       throw new Error(
         `Sandbox path is read-only; cannot ${options.action}: ${target.containerPath}`,
@@ -123,6 +109,25 @@ export class SandboxFsPathGuard {
     }
   }
 
+  private async openBoundaryWithinRequiredMount(
+    target: SandboxResolvedFsPath,
+    action: string,
+    options?: {
+      aliasPolicy?: PathAliasPolicy;
+      allowedType?: SafeOpenSyncAllowedType;
+    },
+  ): Promise<BoundaryFileOpenResult> {
+    const lexicalMount = this.resolveRequiredMount(target.containerPath, action);
+    const guarded = await openBoundaryFile({
+      absolutePath: target.hostPath,
+      rootPath: lexicalMount.hostRoot,
+      boundaryLabel: "sandbox mount root",
+      aliasPolicy: options?.aliasPolicy,
+      allowedType: options?.allowedType,
+    });
+    return guarded;
+  }
+
   async resolveAnchoredSandboxEntry(target: SandboxResolvedFsPath): Promise<AnchoredSandboxEntry> {
     const basename = path.posix.basename(target.containerPath);
     if (!basename || basename === "." || basename === "/") {

src/agents/sandbox/fs-bridge.anchored-ops.test.ts

@@ -1,3 +1,5 @@
+import fs from "node:fs/promises";
+import path from "node:path";
 import { describe, expect, it } from "vitest";
 import {
   createSandbox,
@@ -6,60 +8,113 @@ import {
   findCallsByScriptFragment,
   getDockerArg,
   installFsBridgeTestHarness,
+  mockedExecDockerRaw,
+  withTempDir,
 } from "./fs-bridge.test-helpers.js";
 
 describe("sandbox fs bridge anchored ops", () => {
   installFsBridgeTestHarness();
 
-  it("anchors mkdirp operations on canonical parent + basename", async () => {
+  const pinnedReadCases = [
+    {
+      name: "workspace reads use pinned file descriptors",
+      filePath: "notes/todo.txt",
+      contents: "todo",
+      setup: async (workspaceDir: string) => {
+        await fs.mkdir(path.join(workspaceDir, "notes"), { recursive: true });
+        await fs.writeFile(path.join(workspaceDir, "notes", "todo.txt"), "todo");
+      },
+      sandbox: (workspaceDir: string) =>
+        createSandbox({
+          workspaceDir,
+          agentWorkspaceDir: workspaceDir,
+        }),
+    },
+    {
+      name: "bind-mounted reads use pinned file descriptors",
+      filePath: "/workspace-two/README.md",
+      contents: "bind-read",
+      setup: async (workspaceDir: string, stateDir: string) => {
+        const bindRoot = path.join(stateDir, "workspace-two");
+        await fs.mkdir(workspaceDir, { recursive: true });
+        await fs.mkdir(bindRoot, { recursive: true });
+        await fs.writeFile(path.join(bindRoot, "README.md"), "bind-read");
+      },
+      sandbox: (workspaceDir: string, stateDir: string) =>
+        createSandbox({
+          workspaceDir,
+          agentWorkspaceDir: workspaceDir,
+          docker: {
+            ...createSandbox().docker,
+            binds: [`${path.join(stateDir, "workspace-two")}:/workspace-two:ro`],
+          },
+        }),
+    },
+  ] as const;
+
+  it.each(pinnedReadCases)("$name", async (testCase) => {
+    await withTempDir("openclaw-fs-bridge-contract-read-", async (stateDir) => {
+      const workspaceDir = path.join(stateDir, "workspace");
+      await testCase.setup(workspaceDir, stateDir);
+      const bridge = createSandboxFsBridge({
+        sandbox: testCase.sandbox(workspaceDir, stateDir),
+      });
+
+      await expect(bridge.readFile({ filePath: testCase.filePath })).resolves.toEqual(
+        Buffer.from(testCase.contents),
+      );
+      expect(mockedExecDockerRaw).not.toHaveBeenCalled();
+    });
+  });
+
+  const anchoredCases = [
+    {
+      name: "mkdirp anchors parent + basename",
+      invoke: (bridge: ReturnType<typeof createSandboxFsBridge>) =>
+        bridge.mkdirp({ filePath: "nested/leaf" }),
+      scriptFragment: 'mkdir -p -- "$2"',
+      expectedArgs: ["/workspace/nested", "leaf"],
+      forbiddenArgs: ["/workspace/nested/leaf"],
+      canonicalProbe: "/workspace/nested",
+    },
+    {
+      name: "remove anchors parent + basename",
+      invoke: (bridge: ReturnType<typeof createSandboxFsBridge>) =>
+        bridge.remove({ filePath: "nested/file.txt" }),
+      scriptFragment: 'rm -f -- "$2"',
+      expectedArgs: ["/workspace/nested", "file.txt"],
+      forbiddenArgs: ["/workspace/nested/file.txt"],
+      canonicalProbe: "/workspace/nested",
+    },
+    {
+      name: "rename anchors both parents + basenames",
+      invoke: (bridge: ReturnType<typeof createSandboxFsBridge>) =>
+        bridge.rename({ from: "from.txt", to: "nested/to.txt" }),
+      scriptFragment: 'mv -- "$3" "$2/$4"',
+      expectedArgs: ["/workspace", "/workspace/nested", "from.txt", "to.txt"],
+      forbiddenArgs: ["/workspace/from.txt", "/workspace/nested/to.txt"],
+      canonicalProbe: "/workspace/nested",
+    },
+  ] as const;
+
+  it.each(anchoredCases)("$name", async (testCase) => {
     const bridge = createSandboxFsBridge({ sandbox: createSandbox() });
 
-    await bridge.mkdirp({ filePath: "nested/leaf" });
+    await testCase.invoke(bridge);
 
-    const mkdirCall = findCallByScriptFragment('mkdir -p -- "$2"');
-    expect(mkdirCall).toBeDefined();
-    const args = mkdirCall?.[0] ?? [];
-    expect(getDockerArg(args, 1)).toBe("/workspace/nested");
-    expect(getDockerArg(args, 2)).toBe("leaf");
-    expect(args).not.toContain("/workspace/nested/leaf");
+    const opCall = findCallByScriptFragment(testCase.scriptFragment);
+    expect(opCall).toBeDefined();
+    const args = opCall?.[0] ?? [];
+    testCase.expectedArgs.forEach((value, index) => {
+      expect(getDockerArg(args, index + 1)).toBe(value);
+    });
+    testCase.forbiddenArgs.forEach((value) => {
+      expect(args).not.toContain(value);
+    });
 
     const canonicalCalls = findCallsByScriptFragment('readlink -f -- "$cursor"');
     expect(
-      canonicalCalls.some(([callArgs]) => getDockerArg(callArgs, 1) === "/workspace/nested"),
+      canonicalCalls.some(([callArgs]) => getDockerArg(callArgs, 1) === testCase.canonicalProbe),
     ).toBe(true);
   });
-
-  it("anchors remove operations on canonical parent + basename", async () => {
-    const bridge = createSandboxFsBridge({ sandbox: createSandbox() });
-
-    await bridge.remove({ filePath: "nested/file.txt" });
-
-    const removeCall = findCallByScriptFragment('rm -f -- "$2"');
-    expect(removeCall).toBeDefined();
-    const args = removeCall?.[0] ?? [];
-    expect(getDockerArg(args, 1)).toBe("/workspace/nested");
-    expect(getDockerArg(args, 2)).toBe("file.txt");
-    expect(args).not.toContain("/workspace/nested/file.txt");
-
-    const canonicalCalls = findCallsByScriptFragment('readlink -f -- "$cursor"');
-    expect(
-      canonicalCalls.some(([callArgs]) => getDockerArg(callArgs, 1) === "/workspace/nested"),
-    ).toBe(true);
-  });
-
-  it("anchors rename operations on canonical parents + basenames", async () => {
-    const bridge = createSandboxFsBridge({ sandbox: createSandbox() });
-
-    await bridge.rename({ from: "from.txt", to: "nested/to.txt" });
-
-    const renameCall = findCallByScriptFragment('mv -- "$3" "$2/$4"');
-    expect(renameCall).toBeDefined();
-    const args = renameCall?.[0] ?? [];
-    expect(getDockerArg(args, 1)).toBe("/workspace");
-    expect(getDockerArg(args, 2)).toBe("/workspace/nested");
-    expect(getDockerArg(args, 3)).toBe("from.txt");
-    expect(getDockerArg(args, 4)).toBe("to.txt");
-    expect(args).not.toContain("/workspace/from.txt");
-    expect(args).not.toContain("/workspace/nested/to.txt");
-  });
 });

src/agents/sandbox/fs-bridge.ts

@@ -1,5 +1,6 @@
 import fs from "node:fs";
 import { execDockerRaw, type ExecDockerRawResult } from "./docker.js";
+import { SandboxFsPathGuard } from "./fs-bridge-path-safety.js";
 import {
   buildMkdirpPlan,
   buildRemovePlan,
@@ -7,8 +8,7 @@ import {
   buildStatPlan,
   buildWriteCommitPlan,
   type SandboxFsCommandPlan,
-} from "./fs-bridge-command-plans.js";
-import { SandboxFsPathGuard } from "./fs-bridge-path-safety.js";
+} from "./fs-bridge-shell-command-plans.js";
 import {
   buildSandboxFsMounts,
   resolveSandboxFsPathWithMounts,
@@ -99,12 +99,7 @@ class SandboxFsBridgeImpl implements SandboxFsBridge {
     signal?: AbortSignal;
   }): Promise<Buffer> {
     const target = this.resolveResolvedPath(params);
-    const opened = await this.pathGuard.openReadableFile(target);
-    try {
-      return fs.readFileSync(opened.fd);
-    } finally {
-      fs.closeSync(opened.fd);
-    }
+    return this.readPinnedFile(target);
   }
 
   async writeFile(params: {
@@ -239,6 +234,15 @@ class SandboxFsBridgeImpl implements SandboxFsBridge {
     });
   }
 
+  private async readPinnedFile(target: SandboxResolvedFsPath): Promise<Buffer> {
+    const opened = await this.pathGuard.openReadableFile(target);
+    try {
+      return fs.readFileSync(opened.fd);
+    } finally {
+      fs.closeSync(opened.fd);
+    }
+  }
+
   private async runCheckedCommand(
     plan: SandboxFsCommandPlan & { stdin?: Buffer | string; signal?: AbortSignal },
   ): Promise<ExecDockerRawResult> {