Files
my-pi/pi-permission-system/docs/plans/archive/0093-fix-infra-read-bypass-dev-checkout.md

12 KiB

issue, issue_title
issue issue_title
93 Infrastructure read bypass fails in local development checkout

Fix infrastructure read bypass in local development checkout

Problem Statement

discoverGlobalNodeModulesRoot() walks up from the extension's own import.meta.url to find a node_modules ancestor directory. When the extension is globally installed this works — the walk finds e.g. /opt/homebrew/lib/node_modules. When running from a local development checkout (e.g. /Users/chris/development/pi/pi-permission-system), there is no node_modules ancestor, so the function returns null. This causes piInfrastructureDirs to omit the global node_modules root, and skill file reads trigger unexpected external-directory permission prompts.

Goals

  • Make discoverGlobalNodeModulesRoot() find the global node_modules root even when the extension itself is not installed inside it.
  • Eliminate spurious external-directory prompts for skill file reads during local development.
  • Keep the existing walk-up-from-self as the primary strategy (zero subprocess overhead for production installs).

Non-Goals

  • Upstream API request for getGlobalNpmRoot() — useful but orthogonal (tracked in #48 discussion).
  • Changing piInfrastructureReadPaths config semantics — the manual workaround stays as-is.
  • Multi-package-manager detection — Pi defaults to npm for package installation; users with custom npmCommand in settings.json can use piInfrastructureReadPaths for non-npm global roots.

Background

Relevant modules

  • src/external-directory.tsdiscoverGlobalNodeModulesRoot() (the broken function), isPiInfrastructureRead().
  • src/runtime.tscreateExtensionRuntime() calls discoverGlobalNodeModulesRoot() once at construction and stores the result in piInfrastructureDirs.
  • src/handlers/tool-call.ts — combines piInfrastructureDirs with config.piInfrastructureReadPaths and passes them to isPiInfrastructureRead().

Permission surface

special.external_directory — the external-directory gate for path-bearing tools.

Existing workaround

Users can add the global node_modules path to piInfrastructureReadPaths in their config, but this is non-obvious and machine-specific.

Why the original createRequire fallback plan was wrong

The initial plan proposed using createRequire(import.meta.url) to resolve @mariozechner/pi-coding-agent and walk up from that path. This fails because from a dev checkout, createRequire resolves to the local node_modules/.pnpm/... (the devDependency copy), not the global root. Walking up from that path finds pnpm's internal node_modules, not /opt/homebrew/lib/node_modules.

Verified empirically:

import.meta.resolve('@mariozechner/pi-coding-agent')
→ file:///.../pi-permission-system/node_modules/.pnpm/@mariozechner+pi-coding-agent@0.72.1_.../node_modules/@mariozechner/pi-coding-agent/dist/index.js

Walk-up finds: .../node_modules/.pnpm/.../node_modules  (WRONG — pnpm internal)
Need:          /opt/homebrew/lib/node_modules            (RIGHT — global root)

Design Overview

Strategy: npm root -g subprocess fallback

When the walk-up-from-self strategy returns null (no node_modules ancestor), fall back to npm root -g to discover the global node_modules root.

export function discoverGlobalNodeModulesRoot(
  fromUrl = import.meta.url,
): string | null {
  // Strategy 1: walk up from own location (covers global installs).
  const fromSelf = walkUpToNodeModules(fromUrl);
  if (fromSelf) return fromSelf;

  // Strategy 2: ask npm for the global root (covers dev checkouts).
  return discoverGlobalNodeModulesViaSubprocess();
}

The walk-up loop is extracted to a private walkUpToNodeModules(fromUrl) helper. The subprocess fallback is a separate private function with its own error handling.

Why npm root -g?

  • Pi defaults to npm for package installation (getNpmCommand() returns { command: "npm", args: [] } unless overridden in settings.json). Skills and extensions installed by Pi live under npm root -g.
  • npm is always available when Node.js is available — it ships with Node.
  • The subprocess only runs when the walk-up fails (dev checkout only), so production installs pay zero cost.
  • The result is cached in the existing discoverGlobalNodeModulesRoot() call (called once at createExtensionRuntime() construction).

Why not multi-PM detection?

Pi's config.ts has a detectInstallMethod() function with getGlobalPackageRoots() that handles npm/pnpm/yarn/bun, but those are not exported and the detection logic relies on __dirname being inside node_modules. Replicating that detection is fragile and unnecessary:

  • npm root -g covers the default Pi installation method.
  • Users who override npmCommand in settings.json to use pnpm/bun already have a non-default setup and can use the existing piInfrastructureReadPaths config field.
  • The Bun binary case has no global node_modules tree — extensions are bundled.

Subprocess implementation

function discoverGlobalNodeModulesViaSubprocess(): string | null {
  try {
    const result = spawnSync("npm", ["root", "-g"], {
      encoding: "utf-8",
      timeout: 5000,
      stdio: ["ignore", "pipe", "ignore"],
    });
    const root = result.stdout?.trim();
    if (result.status === 0 && root && existsSync(root)) {
      return root;
    }
    return null;
  } catch {
    return null;
  }
}

Key details:

  • timeout: 5000 — 5 second timeout prevents hanging if npm is broken.
  • stdio: ["ignore", "pipe", "ignore"] — only capture stdout; discard stdin and stderr.
  • existsSync(root) — sanity-check the returned path actually exists.
  • All failures return null — same graceful degradation as today.

Edge cases

  • npm not installed: spawnSync throws ENOENT → caught → returns null.
  • npm root -g returns a non-existent path: existsSync check → returns null.
  • Bun binary runtime: walk-up fails (virtual filesystem), npm may not be available → subprocess fails → returns null. Acceptable — Bun binary bundles extensions.
  • Windows: npm root -g works on Windows. spawnSync handles cross-platform.
  • NVM / fnm: npm root -g returns the correct root for the active Node version.
  • Custom npm prefix: npm root -g respects the configured prefix.

Module-Level Changes

src/external-directory.ts

  • Extract the walk-up loop body into a private walkUpToNodeModules(fromUrl: string): string | null helper.
  • Add private discoverGlobalNodeModulesViaSubprocess(): string | null function.
  • Update discoverGlobalNodeModulesRoot() to try walk-up first, then subprocess fallback.
  • Add imports: spawnSync from node:child_process, existsSync from node:fs.

tests/external-directory.test.ts

  • Add tests for the subprocess fallback path:
    • Walk-up-from-self succeeds → returns result without invoking subprocess.
    • Walk-up-from-self fails, npm root -g returns a valid path → returns that path.
    • Walk-up-from-self fails, npm root -g fails → returns null.
    • Walk-up-from-self fails, npm root -g returns a non-existent path → returns null.

tests/runtime.test.ts

  • No changes needed — the existing mock of discoverGlobalNodeModulesRoot covers the runtime's consumption of the return value. The new fallback logic is internal to discoverGlobalNodeModulesRoot and tested in external-directory.test.ts.

No changes needed

  • src/runtime.ts — no API change; it already calls discoverGlobalNodeModulesRoot() and handles null.
  • src/handlers/tool-call.ts — no change; it already combines piInfrastructureDirs with config paths.
  • schemas/permissions.schema.json — no config field changes.
  • config/config.example.json — no config field changes.
  • docs/architecture/ — no architecture doc describes discoverGlobalNodeModulesRoot in detail.

TDD Order

  1. test: cover npm root -g fallback in discoverGlobalNodeModulesRoot Add tests in tests/external-directory.test.ts:

    • Walk-up-from-self succeeds → returns result without invoking subprocess.
    • Walk-up-from-self fails, subprocess returns valid path → returns that path.
    • Walk-up-from-self fails, subprocess fails (non-zero exit / throws) → returns null.
    • Walk-up-from-self fails, subprocess returns non-existent path → returns null. Mock spawnSync to control subprocess behavior without actually spawning. Commit: test: cover npm root -g fallback for global node_modules discovery
  2. feat: add npm root -g fallback to discoverGlobalNodeModulesRoot Extract walkUpToNodeModules helper, add discoverGlobalNodeModulesViaSubprocess, wire into discoverGlobalNodeModulesRoot. Commit: fix: discover global node_modules root from dev checkout via npm root -g fallback

  3. docs: note the fallback in README The README already documents piInfrastructureReadPaths as the manual workaround. Add a brief note that the automatic discovery now works from dev checkouts via npm root -g fallback. Commit: docs: note npm root -g fallback for dev checkout infrastructure reads

Risks and Mitigations

Risk Mitigation
npm root -g returns an unexpected path, widening the auto-allow set existsSync check validates the path exists. The auto-allow is restricted to READ_ONLY_PATH_BEARING_TOOLS via isPiInfrastructureRead. Writes are never bypassed.
Could this silently weaken a permission? No. The change only affects which directories are added to piInfrastructureDirs, and only for read-only tools. The directory added is the npm global root — the same directory that production installs already auto-allow via the walk-up.
Subprocess hangs or is slow 5-second timeout. Only runs when walk-up fails (dev checkout only). Production installs never hit this path.
npm not available (Bun binary, restricted env) catch returns null, identical to current behavior. No regression.
#48 rejected npm root -g #48 rejected it as the primary strategy because the walk-up-from-self approach was zero-cost for production. Here it's a fallback that only fires from dev checkouts where the walk-up fails. The production path is unchanged.

Open Questions

  • Should we log a debug message when the subprocess fallback is used? This would help diagnose issues but adds noise. Leaning yes — it's a dev-only path and the debug log is opt-in.