From d9bc783faf7e66e38dc000d8e2ecb7a98ea1f333 Mon Sep 17 00:00:00 2001 From: Nathan Flurry Date: Fri, 12 Jun 2026 20:34:46 -0700 Subject: [PATCH] [SLOP(claude-opus-4-8)] feat(sidecar): generalize non-flat node_modules detection across pnpm/bun/yarn-pnp/yarn-pnpm; add Docker layout test suite --- crates/client/tests/module_layout_e2e.rs | 99 +++++++++++++++ crates/sidecar/src/service.rs | 118 ++++++++++++++---- .../sidecar/tests/fixtures/gen-pm-layouts.sh | 60 +++++++++ 3 files changed, 251 insertions(+), 26 deletions(-) create mode 100644 crates/client/tests/module_layout_e2e.rs create mode 100755 crates/sidecar/tests/fixtures/gen-pm-layouts.sh diff --git a/crates/client/tests/module_layout_e2e.rs b/crates/client/tests/module_layout_e2e.rs new file mode 100644 index 000000000..8be56e465 --- /dev/null +++ b/crates/client/tests/module_layout_e2e.rs @@ -0,0 +1,99 @@ +//! Validates, against REAL package-manager layouts, the store signatures the +//! sidecar's `symlinked_node_modules_hint` detector keys on. Each layout is a +//! clean, isolated monorepo generated in Docker by +//! `crates/sidecar/tests/fixtures/gen-pm-layouts.sh` (one node_modules per +//! package manager). Point `PM_LAYOUTS_DIR` at that output and run: +//! PM_LAYOUTS_DIR=/path cargo test -p agent-os-client --test module_layout_e2e -- --nocapture +//! +//! This is a pure filesystem inspection (no VM): it proves that the failing +//! package managers lay a transitive/direct dep into a store dir that escapes the +//! mounted project (so the runtime ENOENT carries that store path — see the +//! detector unit tests in crates/sidecar/src/service.rs for the matching error +//! strings, including the real pi-coding-agent pnpm failure), and that flat +//! managers do not. +//! +//! Skips honestly when `PM_LAYOUTS_DIR` is unset. + +use std::path::Path; + +/// What the direct dep's entry under `app/node_modules` should look like. +enum Expect { + /// A symlink whose target contains this store marker (escapes the mount). + StoreSymlink(&'static str), + /// No `node_modules` at all; a Plug'n'Play runtime file is present at root. + PlugNPlay, + /// Flat: deps hoisted to the workspace root, no per-PM store dir. + Flat, +} + +struct Case { + name: &'static str, + expect: Expect, +} + +const CASES: &[Case] = &[ + Case { name: "pnpm-isolated", expect: Expect::StoreSymlink("node_modules/.pnpm/") }, + Case { name: "bun", expect: Expect::StoreSymlink("node_modules/.bun/") }, + Case { name: "yarn-pnpm", expect: Expect::StoreSymlink("node_modules/.store/") }, + Case { name: "yarn-pnp", expect: Expect::PlugNPlay }, + Case { name: "npm", expect: Expect::Flat }, + Case { name: "yarn-nm", expect: Expect::Flat }, +]; + +#[test] +fn package_manager_layouts_carry_expected_store_signatures() { + let Ok(base) = std::env::var("PM_LAYOUTS_DIR") else { + eprintln!("skipping module_layout_e2e: set PM_LAYOUTS_DIR (run crates/sidecar/tests/fixtures/gen-pm-layouts.sh)"); + return; + }; + let base = Path::new(&base); + let mut checked = 0; + + for case in CASES { + let layout = base.join(case.name); + if !layout.exists() { + eprintln!("[{}] layout missing — skipping", case.name); + continue; + } + checked += 1; + let app_nm = layout.join("app/node_modules"); + let is_odd = app_nm.join("is-odd"); + + match case.expect { + Expect::StoreSymlink(marker) => { + let target = std::fs::read_link(&is_odd).unwrap_or_else(|e| { + panic!("[{}] app/node_modules/is-odd should be a symlink: {e}", case.name) + }); + let target = target.to_string_lossy(); + eprintln!("[{}] is-odd -> {target} (expect store marker {marker:?})", case.name); + assert!( + target.contains(marker), + "[{}] symlink target {target:?} should contain store marker {marker:?}", + case.name + ); + } + Expect::PlugNPlay => { + eprintln!("[{}] expect .pnp.cjs + no node_modules", case.name); + assert!(layout.join(".pnp.cjs").exists(), "[{}] expected .pnp.cjs", case.name); + assert!(!app_nm.exists(), "[{}] PnP should have no app/node_modules", case.name); + } + Expect::Flat => { + eprintln!("[{}] expect flat (hoisted to root, no store)", case.name); + // Direct dep resolves to a real dir at the workspace root, no store. + let root_isodd = layout.join("node_modules/is-odd"); + assert!(root_isodd.exists(), "[{}] expected hoisted node_modules/is-odd", case.name); + assert!( + !root_isodd.symlink_metadata().unwrap().file_type().is_symlink(), + "[{}] flat layout's is-odd should be a real dir, not a symlink", + case.name + ); + for store in ["node_modules/.pnpm", "node_modules/.bun", "node_modules/.store"] { + assert!(!layout.join(store).exists(), "[{}] flat layout must not have {store}", case.name); + } + } + } + } + + assert!(checked > 0, "no layouts found under PM_LAYOUTS_DIR — generate them first"); + eprintln!("\nvalidated {checked} package-manager layouts"); +} diff --git a/crates/sidecar/src/service.rs b/crates/sidecar/src/service.rs index 45133c311..a27139ed9 100644 --- a/crates/sidecar/src/service.rs +++ b/crates/sidecar/src/service.rs @@ -4037,60 +4037,126 @@ pub(crate) fn vfs_error(error: VfsError) -> SidecarError { SidecarError::Kernel(error.to_string()) } -/// Actionable guidance shown when an agent adapter fails because its packages live -/// in a symlinked (non-hoisted) `node_modules` whose pnpm store is not visible in -/// the VM. Mounting host `node_modules` is a bind mount, so a symlinked layout -/// (pnpm default, yarn-berry) does not resolve inside the VM: Node canonicalizes a -/// module to its store realpath (`node_modules/.pnpm/...`) which the guest `fs` -/// cannot read. A flat (hoisted) layout is required. -const HOISTED_NODE_MODULES_GUIDANCE: &str = "Agent OS can't load this agent: its node_modules uses a symlinked layout (pnpm / yarn-berry) whose package store isn't visible inside the VM. A flat (hoisted) node_modules is required.\n - pnpm -> add `node-linker=hoisted` to .npmrc, then reinstall\n - yarn berry -> set `nodeLinker: node-modules` in .yarnrc.yml (not PnP)\n - npm / yarn classic / bun -> already flat, no change needed"; - -/// Detect, from an adapter's captured stderr, the symlinked-`node_modules` -/// failure signature: a missing-file error on a path inside a pnpm `.pnpm` store -/// (which escapes the VM module-access mount). Returns the actionable guidance to -/// fold into the surfaced error, or `None` when the failure is unrelated. +/// Actionable guidance shown when an agent adapter fails because its packages +/// live in a non-flat `node_modules` whose package store is not visible in the +/// VM. Mounting host `node_modules` is a bind mount, so symlinked/store layouts +/// do not resolve inside the VM: Node canonicalizes a module to its store +/// realpath (e.g. `node_modules/.pnpm/...`, `.bun/...`, `.store/...`) which lives +/// above the mounted directory and the guest `fs` cannot read. Plug'n'Play +/// (yarn-berry default) has no `node_modules` at all. A flat (hoisted) layout is +/// required. The empirically-supported package managers are captured in +/// `crates/sidecar/tests/module_layout_e2e.rs`. +const HOISTED_NODE_MODULES_GUIDANCE: &str = "Agent OS can't load this agent: its node_modules uses a non-flat layout (pnpm / bun / yarn workspaces store, or yarn Plug'n'Play) whose package store isn't visible inside the VM. A flat (hoisted) node_modules is required.\n - pnpm -> add `node-linker=hoisted` to .npmrc, then reinstall\n - yarn berry -> set `nodeLinker: node-modules` in .yarnrc.yml (not pnp/pnpm)\n - bun -> install the agent outside a workspace (workspaces use a .bun store)\n - npm / yarn classic -> already flat, no change needed"; + +/// Detect, from an adapter's captured stderr, a non-flat-`node_modules` failure +/// signature. Returns the actionable guidance to fold into the surfaced error, +/// or `None` when the failure is unrelated. /// -/// Kept deliberately specific (an ENOENT-class error AND a `node_modules/.pnpm` -/// path) so it never fires on unrelated adapter crashes. +/// Two signatures, both kept specific so they never fire on unrelated crashes: +/// - a missing-file / cannot-resolve error referencing a package STORE path that +/// lives above the mounted project (`.pnpm`, `.bun`, `.store`, PnP `__virtual__`), +/// - a yarn Plug'n'Play fingerprint (`.pnp.cjs`, the zip cache, or PnP's +/// "isn't declared in your dependencies" resolver error). fn symlinked_node_modules_hint(stderr: &str) -> Option<&'static str> { - let mentions_store = stderr.contains("node_modules/.pnpm") - || stderr.contains("node_modules\\.pnpm"); - if !mentions_store { - return None; - } - let missing_file = stderr.contains("ENOENT") + // Package stores that only appear in a path when a non-flat layout is used. + // pnpm (isolated), bun (workspace), yarn-berry (nodeLinker: pnpm), and PnP + // virtual instances all keep real package files under these store dirs, which + // sit above the mounted project node_modules and so are not guest-visible. + const STORE_MARKERS: &[&str] = &[ + "node_modules/.pnpm/", + "node_modules/.bun/", + "node_modules/.store/", + "/__virtual__/", + ]; + // Yarn Plug'n'Play has no node_modules at all; resolution fails against the + // .pnp runtime / zip cache. "isn't declared in your dependencies" is PnP's + // distinctive resolver error and is specific enough to fire on its own. + const PNP_STRICT_MARKERS: &[&str] = &["isn't declared in your dependencies"]; + const PNP_PATH_MARKERS: &[&str] = &[".pnp.cjs", ".pnp.loader.mjs", "/.yarn/cache/"]; + + if PNP_STRICT_MARKERS.iter().any(|m| stderr.contains(m)) { + return Some(HOISTED_NODE_MODULES_GUIDANCE); + } + + let missing = stderr.contains("ENOENT") || stderr.contains("no such file or directory") - || stderr.contains("Cannot find module"); - if !missing_file { + || stderr.contains("Cannot find module") + || stderr.contains("MODULE_NOT_FOUND"); + if !missing { return None; } - Some(HOISTED_NODE_MODULES_GUIDANCE) + if STORE_MARKERS.iter().any(|m| stderr.contains(m)) + || PNP_PATH_MARKERS.iter().any(|m| stderr.contains(m)) + { + return Some(HOISTED_NODE_MODULES_GUIDANCE); + } + None } #[cfg(test)] mod symlinked_node_modules_hint_tests { use super::symlinked_node_modules_hint; + // Positive cases: each non-flat package manager's store/PnP signature. #[test] fn matches_pnpm_store_enoent() { - // The real pi-coding-agent failure: getPackageDir() falls back to a + // Real pi-coding-agent failure: getPackageDir() falls back to a // dist/package.json inside the unreachable .pnpm store. let stderr = "Error: ENOENT: no such file or directory, open '/root/node_modules/.pnpm/@mariozechner+pi-coding-agent@0.60.0_x/node_modules/@mariozechner/pi-coding-agent/dist/package.json'"; assert!(symlinked_node_modules_hint(stderr).is_some()); } #[test] - fn ignores_enoent_outside_pnpm_store() { + fn matches_bun_store_enoent() { + let stderr = "Error: ENOENT: no such file or directory, open '/root/node_modules/.bun/is-odd@3.0.1/node_modules/is-odd/package.json'"; + assert!(symlinked_node_modules_hint(stderr).is_some()); + } + + #[test] + fn matches_yarn_pnpm_store_enoent() { + let stderr = "Error: ENOENT: no such file or directory, open '/root/node_modules/.store/is-odd-npm-3.0.1-93c3c3f41b/package/package.json'"; + assert!(symlinked_node_modules_hint(stderr).is_some()); + } + + #[test] + fn matches_pnp_declared_error() { + // Yarn PnP's distinctive resolver error (no node_modules at all). + let stderr = "Error: Your application tried to access is-number, but it isn't declared in your dependencies; this makes the require call ambiguous and unsound."; + assert!(symlinked_node_modules_hint(stderr).is_some()); + } + + #[test] + fn matches_pnp_cjs_module_not_found() { + let stderr = "Error: Cannot find module 'is-odd'\n at /root/.pnp.cjs:12345:18\n code: 'MODULE_NOT_FOUND'"; + assert!(symlinked_node_modules_hint(stderr).is_some()); + } + + #[test] + fn matches_virtual_instance() { + let stderr = "Error: ENOENT: no such file or directory, open '/root/.yarn/__virtual__/is-odd-abc/1/node_modules/is-odd/package.json'"; + assert!(symlinked_node_modules_hint(stderr).is_some()); + } + + // Negative cases: must not fire. + #[test] + fn ignores_enoent_outside_a_store() { let stderr = "Error: ENOENT: no such file or directory, open '/tmp/scratch/config.json'"; assert!(symlinked_node_modules_hint(stderr).is_none()); } #[test] - fn ignores_pnpm_path_without_missing_file() { + fn ignores_store_path_without_missing_file() { let stderr = "loaded /root/node_modules/.pnpm/some-pkg@1.0.0/node_modules/some-pkg/index.js"; assert!(symlinked_node_modules_hint(stderr).is_none()); } + #[test] + fn ignores_flat_node_modules_enoent() { + // npm / yarn-nm / pnpm-hoisted: flat, no store dir in the path. + let stderr = "Error: ENOENT: no such file or directory, open '/root/node_modules/is-odd/missing-asset.json'"; + assert!(symlinked_node_modules_hint(stderr).is_none()); + } + #[test] fn ignores_unrelated_failure() { let stderr = "Error: connect ECONNREFUSED 127.0.0.1:443"; diff --git a/crates/sidecar/tests/fixtures/gen-pm-layouts.sh b/crates/sidecar/tests/fixtures/gen-pm-layouts.sh new file mode 100755 index 000000000..dfb204293 --- /dev/null +++ b/crates/sidecar/tests/fixtures/gen-pm-layouts.sh @@ -0,0 +1,60 @@ +#!/usr/bin/env bash +# Generate a clean, isolated node_modules layout per package manager, using +# Docker containers so each install runs with fresh caches/stores and no host +# pollution. Each layout is a workspace root + an `app` subpackage depending on +# `is-odd` (which pulls the transitive `is-number`), reproducing the real +# monorepo case where a package's store escapes the mounted project dir. +# +# Usage: +# crates/sidecar/tests/fixtures/gen-pm-layouts.sh [OUT_DIR] +# then point the layout test at it: +# PM_LAYOUTS_DIR=$OUT_DIR cargo test -p agent-os-client --test module_layout_e2e -- --nocapture +# +# The resulting per-PM store signatures (what the sidecar detector keys on): +# pnpm (isolated) app/node_modules/ -> ../../node_modules/.pnpm/... +# bun (workspace) app/node_modules/ -> ../../node_modules/.bun/... +# yarn (nodeLinker:pnpm) app/node_modules/ -> ../../node_modules/.store/... +# yarn (pnp, default) .pnp.cjs + .yarn/cache, NO node_modules +# npm / yarn-nm / pnpm-hoisted flat (hoisted to root, no store) -> work +set -uo pipefail + +OUT="${1:-$(mktemp -d /tmp/pm-layouts.XXXXXX)}" +mkdir -p "$OUT" +echo "output: $OUT" + +NODE_IMG=node:22-bookworm-slim +BUN_IMG=oven/bun:1-slim +export COREPACK_ENABLE_DOWNLOAD_PROMPT=0 + +scaffold=' +mkdir -p app +cat > package.json < app/package.json < "$OUT/$name.log" 2>&1; then + echo "ok" + else + echo "FAILED (see $OUT/$name.log)" + fi +} + +run npm "$NODE_IMG" 'npm install --silent' +run pnpm-isolated "$NODE_IMG" 'echo "packages: [app]" > pnpm-workspace.yaml; corepack enable >/dev/null 2>&1; corepack pnpm install --config.store-dir=/work/.store --silent' +run pnpm-hoisted "$NODE_IMG" 'echo "packages: [app]" > pnpm-workspace.yaml; printf "node-linker=hoisted\n" > .npmrc; corepack enable >/dev/null 2>&1; corepack pnpm install --config.store-dir=/work/.store --silent' +run yarn-pnp "$NODE_IMG" 'corepack enable >/dev/null 2>&1; corepack yarn set version stable >/dev/null 2>&1; corepack yarn install >/dev/null 2>&1' +run yarn-nm "$NODE_IMG" 'printf "nodeLinker: node-modules\n" > .yarnrc.yml; corepack enable >/dev/null 2>&1; corepack yarn set version stable >/dev/null 2>&1; corepack yarn install >/dev/null 2>&1' +run yarn-pnpm "$NODE_IMG" 'printf "nodeLinker: pnpm\n" > .yarnrc.yml; corepack enable >/dev/null 2>&1; corepack yarn set version stable >/dev/null 2>&1; corepack yarn install >/dev/null 2>&1' +run bun "$BUN_IMG" 'bun install >/dev/null 2>&1' + +echo "" +echo "PM_LAYOUTS_DIR=$OUT"