fix(agent-tools): keep desktop read/write/edit inside the workspace

packages/agent-tools/src/cloud/matrix-tools/path-guard.ts

@@ -1,5 +1,18 @@
+/**
+ * Workspace path boundary (cloud version). Cloud tools must pass this check before fs access to
+ * prevent the LLM escaping the working directory with `..`, absolute paths, or symlinks.
+ *
+ * NOTE(shared-logic): Equivalent to `../../desktop/path-guard.ts`. Each side currently keeps a
+ * copy to preserve local / cloud tool directory isolation (AGENTS.md §1), so the two must be
+ * changed together. The real-path resolution both copies now use lives in
+ * `../../shared/workspace-real-path.ts` — one implementation, so the copies cannot drift apart on
+ * the part that decides whether a symlink escapes.
+ */
+
 import { realpath } from 'node:fs/promises';
-import { basename, dirname, isAbsolute, relative, resolve, sep } from 'node:path';
+import { isAbsolute, resolve } from 'node:path';
+
+import { isInsideDirectory, resolveRealPath } from '../../shared/workspace-real-path.js';
 
 /**
  * Input-path fence scope. A plain string keeps the legacy workspace-only
@@ -56,31 +69,12 @@ export async function assertWithinWorkspace(
 ): Promise<void> {
   const rootReal = await realpath(workspaceRoot);
 
-  let candidate = resolve(absolutePath);
-  const suffixParts: string[] = [];
-  while (true) {
-    let ancestorReal: string;
-    try {
-      ancestorReal = await realpath(candidate);
-    } catch (err) {
-      const code = (err as NodeJS.ErrnoException).code;
-      if (code !== 'ENOENT' && code !== 'ENOTDIR') throw err;
-      const parent = dirname(candidate);
-      if (parent === candidate) {
-        throw new Error(`Workspace path escapes Matrix tool root: ${absolutePath}`);
-      }
-      suffixParts.unshift(basename(candidate));
-      candidate = parent;
-      continue;
-    }
-
-    const finalPath =
-      suffixParts.length === 0 ? ancestorReal : resolve(ancestorReal, ...suffixParts);
-    const rel = relative(rootReal, finalPath);
-    if (rel === '..' || rel.startsWith(`..${sep}`) || isAbsolute(rel)) {
-      throw new Error(`Workspace path escapes Matrix tool root: ${absolutePath}`);
-    }
-    return;
+  // `resolveRealPath` follows symlinks — dangling ones included. A plain
+  // `realpath` walk treated a dangling link as a missing file and compared the
+  // lexical spelling, so a link pointing outside the workspace passed.
+  const targetReal = await resolveRealPath(absolutePath);
+  if (targetReal === undefined || !isInsideDirectory(rootReal, targetReal)) {
+    throw new Error(`Workspace path escapes Matrix tool root: ${absolutePath}`);
   }
 }
 

packages/agent-tools/src/desktop/local-pi-tools.ts

@@ -17,11 +17,12 @@ import {
 import { getShellConfig } from '@earendil-works/pi-coding-agent/shell';
 import type { AgentTool, AgentToolResult } from '@earendil-works/pi-agent-core';
 import { access } from 'node:fs/promises';
-import { isAbsolute, resolve as resolvePath } from 'node:path';
+import { isAbsolute, join, resolve as resolvePath } from 'node:path';
 import { homedir, release } from 'node:os';
 import { fileURLToPath } from 'node:url';
 import { randomUUID } from 'node:crypto';
 import { resolveBashDescription } from './local-bash-input.js';
+import { findWorkspaceFenceViolation } from './workspace-fence.js';
 import {
   DEFAULT_FOREGROUND_BASH_SOFT_YIELD_MS,
   resolveLocalBashTiming,
@@ -124,6 +125,20 @@ export class LocalReadTool implements ToolImpl<
     if (signal?.aborted) throw new Error('Operation aborted');
     const readAbs = await resolveLocalReadPath(input.path, this.workspaceRoot);
 
+    // Workspace fence: the file the tool touches has to really live inside
+    // the workspace. A symlink inside the workspace would otherwise let the
+    // read reach out of it, and the permission layer cannot see that because
+    // it compares paths lexically. Validation only — pi still receives the
+    // lexical path below, so the ledger key stays aligned with pi's own
+    // `resolveToCwd` output.
+    const fenceViolation = await findWorkspaceFenceViolation(
+      [...piReadPathCandidates(input.path, this.workspaceRoot), readAbs],
+      this.workspaceRoot,
+    );
+    if (fenceViolation) {
+      return workspaceFenceErrorResult(LocalReadToolDef.name, fenceViolation);
+    }
+
     // Guard order (design doc §3): the device check comes first because
     // every later step that opens the file (binary sampling, PDF/notebook
     // parsing, pi itself) would hang forever on /dev/zero-style paths.
@@ -251,6 +266,17 @@ export class LocalWriteTool implements ToolImpl<
     signal?: AbortSignal,
   ): Promise<ToolResult> {
     if (signal?.aborted) throw new Error('Operation aborted');
+    // Workspace fence: pi writes the path it resolves from `input.path`, so
+    // that is the path the fence has to check. A symlinked directory or a
+    // dangling symlink would otherwise create the file outside the
+    // workspace.
+    const fenceViolation = await findWorkspaceFenceViolation(
+      [piResolvedPath(input.path, this.workspaceRoot)],
+      this.workspaceRoot,
+    );
+    if (fenceViolation) {
+      return workspaceFenceErrorResult(LocalWriteToolDef.name, fenceViolation);
+    }
     // Per-execute capture context. This tool instance is a process-level
     // singleton (buildLocalToolRegistry), so an instance-level pending map
     // would be shared across concurrent writes; binding capture to a single
@@ -286,6 +312,20 @@ export class LocalEditTool implements ToolImpl<
   ): Promise<ToolResult> {
     if (signal?.aborted) throw new Error('Operation aborted');
 
+    // Workspace fence: pi edits the path it resolves from `input.file_path`,
+    // and the `replace_all` branch below reads that path itself — so both
+    // spellings have to be inside the workspace for real.
+    const fenceViolation = await findWorkspaceFenceViolation(
+      [
+        piResolvedPath(input.file_path, this.workspaceRoot),
+        resolveLocalCandidatePath(input.file_path, this.workspaceRoot),
+      ],
+      this.workspaceRoot,
+    );
+    if (fenceViolation) {
+      return workspaceFenceErrorResult(LocalEditToolDef.name, fenceViolation);
+    }
+
     // Translate the compatible style public schema (file_path + old_string +
     // new_string + optional replace_all) into pi's internal
     // { path, edits: [{ oldText, newText }] } shape so we still get
@@ -787,6 +827,61 @@ function resolveLocalCandidatePath(input: string, workspaceRoot: string): string
   return normalizeLocalExistingPath(isAbsolute(path) ? path : resolvePath(workspaceRoot, path));
 }
 
+/**
+ * The absolute path pi itself opens for `input`.
+ *
+ * Mirrors pi's `resolveToCwd` (`normalizePath` + `path.resolve` against the
+ * tool cwd): unicode-space normalization, the `@` prefix strip, `~` expansion
+ * and `file://` URLs all happen before the path is resolved. The fence has to
+ * check the same target pi touches — otherwise a spelling pi rewrites could
+ * still reach outside the workspace.
+ */
+function piResolvedPath(input: string, workspaceRoot: string): string {
+  let normalized = input.replace(/[\u00a0\u2000-\u200a\u202f\u205f\u3000]/g, ' ');
+  if (normalized.startsWith('@')) normalized = normalized.slice(1);
+  if (normalized === '~') return resolvePath(homedir());
+  if (normalized.startsWith('~/') || (process.platform === 'win32' && normalized.startsWith('~\\'))) {
+    normalized = join(homedir(), normalized.slice(2));
+  }
+  if (/^file:\/\//.test(normalized)) normalized = fileURLToPath(normalized);
+  return isAbsolute(normalized) ? resolvePath(normalized) : resolvePath(workspaceRoot, normalized);
+}
+
+/**
+ * Every path the read tool can end up opening for `input`.
+ *
+ * pi's read tool resolves the requested path and, when that spelling does not
+ * exist, retries the macOS-flavoured ones (narrow no-break space before
+ * AM/PM, NFD, curly quote). The desktop wrapper resolves the same variants
+ * for its own dispatch (PDF / notebook / binary sniffing). The fence checks
+ * all of them: a variant spelling must not be a way around it.
+ */
+function piReadPathCandidates(input: string, workspaceRoot: string): string[] {
+  const resolved = piResolvedPath(input, workspaceRoot);
+  const amPm = resolved.replace(/ (AM|PM)\./gi, '\u202f$1.');
+  const nfd = resolved.normalize('NFD');
+  const curly = resolved.replace(/'/g, '\u2019');
+  const nfdCurly = nfd.replace(/'/g, '\u2019');
+  return [resolved, amPm, nfd, curly, nfdCurly];
+}
+
+/**
+ * Uniform, recoverable tool error for a workspace-fence violation.
+ *
+ * The model sees it as a tool error it can react to — the same channel the
+ * other wrapper-level guards use — instead of a thrown exception that
+ * arrives without structured details.
+ */
+function workspaceFenceErrorResult(toolName: string, reason: string): ToolResult {
+  return {
+    tool_name: toolName,
+    text: reason,
+    content: [{ type: 'text', text: reason }],
+    details: { status: 'workspace_fence_violation', error_code: 'WORKSPACE_PATH_ESCAPE' },
+    isError: true,
+  };
+}
+
 async function resolveLocalReadPath(input: string, workspaceRoot: string): Promise<string> {
   const literalResolved = resolveLocalCandidatePath(input, workspaceRoot);
   if (await pathExists(literalResolved)) return literalResolved;

packages/agent-tools/src/desktop/path-guard.test.ts

@@ -0,0 +1,84 @@
+import { mkdir, mkdtemp, rm, symlink, writeFile } from 'node:fs/promises';
+import { tmpdir } from 'node:os';
+import { join } from 'node:path';
+
+import { afterEach, beforeEach, describe, expect, it } from 'vitest';
+
+import { resolveWithinWorkspace } from './path-guard.js';
+
+/**
+ * The desktop path guard has to hold for symlinks, not just for `..` and
+ * absolute paths: a link inside the workspace that points outside it is the
+ * shape an escape actually takes.
+ */
+describe('desktop workspace path guard', () => {
+  let workspace: string;
+  let outside: string;
+
+  beforeEach(async () => {
+    workspace = await mkdtemp(join(tmpdir(), 'path-guard-ws-'));
+    outside = await mkdtemp(join(tmpdir(), 'path-guard-out-'));
+  });
+
+  afterEach(async () => {
+    await rm(workspace, { recursive: true, force: true });
+    await rm(outside, { recursive: true, force: true });
+  });
+
+  it('rejects a symlink that points outside the workspace', async () => {
+    await writeFile(join(outside, 'secret.txt'), 'secret\n');
+    await symlink(join(outside, 'secret.txt'), join(workspace, 'escape.txt'));
+
+    await expect(resolveWithinWorkspace('escape.txt', workspace)).rejects.toThrow(
+      /escapes local runtime root/,
+    );
+  });
+
+  it('rejects a dangling symlink that points outside the workspace', async () => {
+    await symlink(join(outside, 'not-created-yet.txt'), join(workspace, 'dangling.txt'));
+
+    await expect(resolveWithinWorkspace('dangling.txt', workspace)).rejects.toThrow(
+      /escapes local runtime root/,
+    );
+  });
+
+  it('rejects a path whose parent directory is a symlink pointing outside', async () => {
+    await mkdir(join(outside, 'sub'), { recursive: true });
+    await symlink(join(outside, 'sub'), join(workspace, 'outdir'));
+
+    await expect(resolveWithinWorkspace('outdir/file.txt', workspace)).rejects.toThrow(
+      /escapes local runtime root/,
+    );
+  });
+
+  it('rejects a lexical escape', async () => {
+    await expect(resolveWithinWorkspace('../outside/secret.txt', workspace)).rejects.toThrow(
+      /escapes local runtime root/,
+    );
+  });
+
+  it('allows a symlink that points inside the workspace', async () => {
+    await writeFile(join(workspace, 'real.txt'), 'inside\n');
+    await symlink(join(workspace, 'real.txt'), join(workspace, 'inside-link.txt'));
+
+    await expect(resolveWithinWorkspace('inside-link.txt', workspace)).resolves.toBe(
+      join(workspace, 'inside-link.txt'),
+    );
+  });
+
+  it('allows a dangling symlink that points inside the workspace', async () => {
+    await symlink(join(workspace, 'not-created-yet.txt'), join(workspace, 'inside-dangling.txt'));
+
+    await expect(resolveWithinWorkspace('inside-dangling.txt', workspace)).resolves.toBe(
+      join(workspace, 'inside-dangling.txt'),
+    );
+  });
+
+  it('allows a workspace root that is itself a symlink', async () => {
+    await writeFile(join(workspace, 'real.txt'), 'inside\n');
+    const alias = join(outside, 'ws-alias');
+    await symlink(workspace, alias);
+
+    await expect(resolveWithinWorkspace('real.txt', alias)).resolves.toBe(join(alias, 'real.txt'));
+  });
+});

packages/agent-tools/src/desktop/path-guard.ts

@@ -2,44 +2,27 @@
  * Workspace path boundary (desktop local version). Local tools must pass this check before fs
  * access to prevent the LLM escaping the working directory with `..`, absolute paths, or symlinks.
  *
- * NOTE(shared-logic): Equivalent to `../cloud/path-guard.ts`. Each side currently keeps a copy to
- * preserve local / cloud tool directory isolation (AGENTS.md §1). This is a prime candidate for
- * future shared-logic extraction.
+ * NOTE(shared-logic): Equivalent to `../cloud/matrix-tools/path-guard.ts`. Each side currently
+ * keeps a copy to preserve local / cloud tool directory isolation (AGENTS.md §1), so the two must
+ * be changed together. The real-path resolution both copies now use lives in
+ * `../shared/workspace-real-path.ts` — one implementation, so the copies cannot drift apart on
+ * the part that decides whether a symlink escapes.
  */
 
 import { realpath } from 'node:fs/promises';
-import { basename, dirname, isAbsolute, relative, resolve, sep } from 'node:path';
+import { isAbsolute, resolve } from 'node:path';
+
+import { isInsideDirectory, resolveRealPath } from '../shared/workspace-real-path.js';
 
 async function assertWithinWorkspace(absolutePath: string, workspaceRoot: string): Promise<void> {
   const rootReal = await realpath(workspaceRoot);
 
-  let candidate = resolve(absolutePath);
-  const suffixParts: string[] = [];
-  while (true) {
-    let ancestorReal: string;
-    try {
-      ancestorReal = await realpath(candidate);
-    } catch (err) {
-      const code = (err as NodeJS.ErrnoException).code;
-      if (code !== 'ENOENT' && code !== 'ENOTDIR') {
-        throw err;
-      }
-      const parent = dirname(candidate);
-      if (parent === candidate) {
-        throw new Error(`Workspace path escapes local runtime root: ${absolutePath}`);
-      }
-      suffixParts.unshift(basename(candidate));
-      candidate = parent;
-      continue;
-    }
-
-    const finalPath =
-      suffixParts.length === 0 ? ancestorReal : resolve(ancestorReal, ...suffixParts);
-    const rel = relative(rootReal, finalPath);
-    if (rel === '..' || rel.startsWith(`..${sep}`) || isAbsolute(rel)) {
-      throw new Error(`Workspace path escapes local runtime root: ${absolutePath}`);
-    }
-    return;
+  // `resolveRealPath` follows symlinks — dangling ones included. A plain
+  // `realpath` walk treated a dangling link as a missing file and compared the
+  // lexical spelling, so a link pointing outside the workspace passed.
+  const targetReal = await resolveRealPath(absolutePath);
+  if (targetReal === undefined || !isInsideDirectory(rootReal, targetReal)) {
+    throw new Error(`Workspace path escapes local runtime root: ${absolutePath}`);
   }
 }