[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

@@ -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");
+}

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";

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/<dep> -> ../../node_modules/.pnpm/...
+#   bun (workspace)      app/node_modules/<dep> -> ../../node_modules/.bun/...
+#   yarn (nodeLinker:pnpm) app/node_modules/<dep> -> ../../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 <<JSON
+{ "name": "root", "private": true, "version": "1.0.0", "workspaces": ["app"] }
+JSON
+cat > app/package.json <<JSON
+{ "name": "app", "version": "1.0.0", "dependencies": { "is-odd": "3.0.1" } }
+JSON
+'
+
+run() { # name image script
+  local name="$1" image="$2" script="$3"
+  mkdir -p "$OUT/$name"
+  printf '%-16s ' "$name"
+  if docker run --rm -v "$OUT/$name:/work" -w /work -e COREPACK_ENABLE_DOWNLOAD_PROMPT=0 \
+      "$image" bash -lc "$scaffold $script" > "$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"