diff --git a/crates/socket-patch-cli/tests/in_process_alternate_installers.rs b/crates/socket-patch-cli/tests/in_process_alternate_installers.rs index f51c3685b..0fbbab596 100644 --- a/crates/socket-patch-cli/tests/in_process_alternate_installers.rs +++ b/crates/socket-patch-cli/tests/in_process_alternate_installers.rs @@ -1130,10 +1130,22 @@ async fn yarn_berry_node_modules_linker_apply_patches_file() { fn stage_skip_dir_member_workspace( root: &Path, member: &str, +) -> (PathBuf, PathBuf, Vec, String, String) { + stage_member_workspace(root, r#"["packages/*"]"#, &format!("packages/{member}")) +} + +/// [`stage_skip_dir_member_workspace`] with the root's `workspaces` value +/// and the second member's path (`/`-separated, below the root) given. +fn stage_member_workspace( + root: &Path, + workspaces: &str, + member: &str, ) -> (PathBuf, PathBuf, Vec, String, String) { std::fs::write( root.join("package.json"), - r#"{"name":"root","version":"1.0.0","private":true,"workspaces":["packages/*"],"dependencies":{"ms":"2.1.3"}}"#, + format!( + r#"{{"name":"root","version":"1.0.0","private":true,"workspaces":{workspaces},"dependencies":{{"ms":"2.1.3"}}}}"# + ), ) .unwrap(); let stage_ms = |nm: PathBuf, version: &str| { @@ -1153,8 +1165,9 @@ fn stage_skip_dir_member_workspace( }; stage_ms(root.join("node_modules"), "2.1.3"); let mut copies = Vec::new(); - for name in ["app", member] { - let dir = root.join("packages").join(name); + for rel in ["packages/app", member] { + let dir = root.join(rel); + let name = rel.rsplit('/').next().unwrap(); std::fs::create_dir_all(&dir).unwrap(); std::fs::write( dir.join("package.json"), @@ -1256,6 +1269,77 @@ async fn apply_check_and_vex_see_unpatched_member_named_like_a_build_dir() { assert!(doc.contains("not_affected"), "{doc}"); } +/// #1827: agent-mode apply patches the nested copy of a declared workspace +/// member inside a hidden directory (`.github/actions/x` through a glob, +/// `.config/tool` listed literally), not just the other member's copy. +#[tokio::test] +#[serial] +async fn apply_patches_workspace_member_in_a_hidden_dir() { + for (workspaces, member) in [ + (r#"["packages/*",".github/actions/*"]"#, ".github/actions/x"), + (r#"["packages/*",".config/tool"]"#, ".config/tool"), + ] { + let tmp = tempfile::tempdir().unwrap(); + let (app_copy, member_copy, patched, before_hash, after_hash) = + stage_member_workspace(tmp.path(), workspaces, member); + + let code = apply_run(default_apply(tmp.path())).await; + assert_eq!(code, 0, "apply (member {member})"); + assert_patched(&app_copy, &patched, &before_hash, &after_hash); + assert_patched(&member_copy, &patched, &before_hash, &after_hash); + } +} + +/// #1827: with only the other member's copy patched, `apply --check` +/// reports the `.github/actions/x` copy as drift (exit 1), and `vex` +/// attests nothing as `not_affected` until that copy is patched too. +#[tokio::test] +#[serial] +async fn apply_check_and_vex_see_unpatched_member_in_a_hidden_dir() { + let tmp = tempfile::tempdir().unwrap(); + let (app_copy, member_copy, patched, _, _) = stage_member_workspace( + tmp.path(), + r#"["packages/*",".github/actions/*"]"#, + ".github/actions/x", + ); + std::fs::write(&app_copy, &patched).unwrap(); + assert_ne!(std::fs::read(&member_copy).unwrap(), patched); + + let mut args = default_apply(tmp.path()); + args.check = true; + let code = apply_run(args).await; + assert_eq!( + code, 1, + "apply --check must report the unpatched hidden member copy" + ); + + let vex_path = tmp.path().join("vex.json"); + let vex = || async { + let _ = std::fs::remove_file(&vex_path); + let code = socket_patch_cli::commands::vex::run(socket_patch_cli::commands::vex::VexArgs { + common: default_apply(tmp.path()).common, + output: Some(vex_path.clone()), + product: Some("pkg:npm/root@1.0.0".to_string()), + no_verify: false, + doc_id: None, + compact: false, + }) + .await; + (code, std::fs::read_to_string(&vex_path).unwrap_or_default()) + }; + let (code, doc) = vex().await; + assert!( + !doc.contains("not_affected"), + "VEX must not attest not_affected while .github/actions/x's copy is \ + unpatched (exit {code}):\n{doc}" + ); + // The control: with both copies patched, the same run attests it. + std::fs::write(&member_copy, &patched).unwrap(); + let (code, doc) = vex().await; + assert_eq!(code, 0, "{doc}"); + assert!(doc.contains("not_affected"), "{doc}"); +} + // --------------------------------------------------------------------------- // Rush pnpm symlink farm (hand-built, no real installer) // --------------------------------------------------------------------------- diff --git a/crates/socket-patch-core/src/crawlers/npm_crawler.rs b/crates/socket-patch-core/src/crawlers/npm_crawler.rs index 2d6acefd5..763dc94c8 100644 --- a/crates/socket-patch-core/src/crawlers/npm_crawler.rs +++ b/crates/socket-patch-core/src/crawlers/npm_crawler.rs @@ -66,8 +66,9 @@ pub(super) fn configured_install_roots(start_path: &Path) -> Vec { } /// The `node_modules` dirs of declared workspace members that the -/// workspace walk prunes by name (#1627): members whose path crosses a -/// [`SKIP_DIRS`] name (`packages/build`, `vendor/lib`). npm, yarn and Bun +/// workspace walk prunes by name: members whose path crosses a +/// [`SKIP_DIRS`] name (`packages/build`, `vendor/lib`, #1627) or a hidden +/// one (`.github/actions/x`, `.config/tool`, #1827). npm, yarn and Bun /// install a member's unhoistable deps into `/node_modules` from /// the root lock, so such a copy is as live as any other member's. The /// workspace roots are `start_path` and the project of every `walked` @@ -95,6 +96,13 @@ pub(super) fn skipped_workspace_member_roots( roots } +/// Whether the workspace walk prunes a dir by its name alone: a hidden dir +/// or a well-known build dir ([`SKIP_DIRS`]). Declared workspace members +/// under such a name are added back by [`skipped_workspace_member_roots`]. +fn walk_prunes_name(name: &str) -> bool { + name.starts_with('.') || SKIP_DIRS.contains(&name) +} + /// [`skipped_workspace_member_roots`] for the one workspace root `root`. A /// member is a dir the root `package.json` `workspaces` patterns list in /// any package manager's reading ([`WorkspacesDialect::ALL`]: an extra root @@ -102,12 +110,17 @@ pub(super) fn skipped_workspace_member_roots( /// pattern is walked from its literal prefix to the depth its segments /// allow (any depth for `**`, within [`WORKSPACE_GLOB_DIR_BUDGET`] dirs per /// pattern), with the walk's own rules: real dirs only (never a symlink), -/// never a hidden dir, a `node_modules` or a tagged cache dir below the -/// root, literal prefix included. Names are -/// read from the listings, so the [`SKIP_DIRS`] test sees each dir as the -/// walk does even when a pattern spells it in another case. +/// never a `node_modules` or a tagged cache dir below the root, literal +/// prefix included. A hidden dir counts only where the pattern spells the +/// dot (`.github/actions/*`, `.config/tool`): the glob part descends into +/// one only when it names a dot, and a path through one is read with no +/// wildcard matching a dot component, as pnpm, Bun and vlt read it. So a +/// `**` never spends its budget on `.git` or `.next`, and build output +/// such as `.output/server` never becomes a member. Names are read from +/// the listings, so the pruned-name test sees each dir as the walk does +/// even when a pattern spells it in another case. fn skipped_members_of(root: &Path) -> Vec { - use crate::utils::workspace_globs::{workspaces_include_as, WorkspacesDialect}; + use crate::utils::workspace_globs::{expand_braces, workspaces_include_as, WorkspacesDialect}; let Some(patterns) = crate::utils::fs::read_regular_to_string_sync(&root.join("package.json")) .ok() @@ -116,90 +129,105 @@ fn skipped_members_of(root: &Path) -> Vec { return Vec::new(); }; let walkable = |entry: &ListedEntry| { - entry.file_type.is_some_and(|ft| ft.is_dir()) - && entry.name_str != "node_modules" - && !entry.name_str.starts_with('.') + entry.file_type.is_some_and(|ft| ft.is_dir()) && entry.name_str != "node_modules" }; let mut roots = Vec::new(); // A leading `!` is a negation in some readings and part of an extglob // or a literal name in others; the dirs it can reach are walked either - // way, and `workspaces_include_as` decides. + // way, and `workspaces_include_as` decides. Braces are expanded first, + // so each alternative walks from its own literal prefix and only an + // alternative that spells a dot descends into hidden dirs; the + // alternatives of one pattern share its dir budget. for pattern in &patterns { - let segments: Vec<&str> = pattern - .trim_start_matches('!') - .split(['/', '\\']) - .filter(|s| !s.is_empty() && *s != ".") - .collect(); - let is_glob = |s: &str| s.contains(['*', '?', '[', ']', '{', '}', '(', ')', '!', '+', '@']); - let literal = segments.iter().take_while(|s| !is_glob(s)).count(); - // The literal prefix, each component as its parent lists it (an - // exact name first, else the ASCII-case-insensitive match a - // case-insensitive filesystem resolves). A prefix that climbs out - // (`..`) or is not walkable names nothing here. - let mut dir = root.to_path_buf(); - let mut rel: Vec = Vec::new(); - let mut reachable = true; - for segment in &segments[..literal] { - let entries: Vec = list_dir_sync(&dir) - .entries - .into_iter() - .filter(|e| walkable(e)) - .collect(); - let found = entries.iter().find(|e| e.name_str == *segment).or_else(|| { - entries - .iter() - .find(|e| e.name_str.eq_ignore_ascii_case(segment)) - }); - let Some(entry) = found else { - reachable = false; - break; - }; - dir.push(&entry.name); - rel.push(entry.name_str.clone()); - if is_tagged_cache_dir(&dir, &list_dir_sync(&dir)) { - reachable = false; - break; - } - } - if !reachable { - continue; - } - let rest = &segments[literal..]; - let max_depth = if rest.contains(&"**") { - usize::MAX - } else { - rest.len() - }; let mut budget = WORKSPACE_GLOB_DIR_BUDGET; - let mut stack = vec![(dir, rel, 0usize)]; - while let Some((dir, rel, depth)) = stack.pop() { - let listing = list_dir_sync(&dir); - // The workspace root itself is exempt, as the walk always - // crawls the scan root (and never reaches a nested project - // inside a tagged dir). - if !rel.is_empty() && is_tagged_cache_dir(&dir, &listing) { - continue; - } - if rel.iter().any(|seg| SKIP_DIRS.contains(&seg.as_str())) - && WorkspacesDialect::ALL - .iter() - .any(|&dialect| workspaces_include_as(&patterns, &rel, dialect)) - && dir.join("package.json").is_file() - && has_node_modules_dir(&dir, &listing) - { - roots.push(dir.join("node_modules")); + for alternative in expand_braces(pattern.trim_start_matches('!')) { + let segments: Vec<&str> = alternative + .split(['/', '\\']) + .filter(|s| !s.is_empty() && *s != ".") + .collect(); + let is_glob = + |s: &str| s.contains(['*', '?', '[', ']', '{', '}', '(', ')', '!', '+', '@']); + let literal = segments.iter().take_while(|s| !is_glob(s)).count(); + // The literal prefix, each component as its parent lists it (an + // exact name first, else the ASCII-case-insensitive match a + // case-insensitive filesystem resolves). A prefix that climbs out + // (`..`) or is not walkable names nothing here. + let mut dir = root.to_path_buf(); + let mut rel: Vec = Vec::new(); + let mut reachable = true; + for segment in &segments[..literal] { + let entries: Vec = list_dir_sync(&dir) + .entries + .into_iter() + .filter(|e| walkable(e)) + .collect(); + let found = entries.iter().find(|e| e.name_str == *segment).or_else(|| { + entries + .iter() + .find(|e| e.name_str.eq_ignore_ascii_case(segment)) + }); + let Some(entry) = found else { + reachable = false; + break; + }; + dir.push(&entry.name); + rel.push(entry.name_str.clone()); + if is_tagged_cache_dir(&dir, &list_dir_sync(&dir)) { + reachable = false; + break; + } } - if depth >= max_depth { + if !reachable { continue; } - for entry in listing.entries.into_iter().filter(|e| walkable(e)) { - let Some(left) = budget.checked_sub(1) else { - break; - }; - budget = left; - let mut child_rel = rel.clone(); - child_rel.push(entry.name_str.clone()); - stack.push((dir.join(&entry.name), child_rel, depth + 1)); + let rest = &segments[literal..]; + let rest_names_dot = rest + .iter() + .any(|s| s.starts_with('.') || s.contains("(.") || s.contains("|.")); + let max_depth = if rest.contains(&"**") { + usize::MAX + } else { + rest.len() + }; + let mut stack = vec![(dir, rel, 0usize)]; + while let Some((dir, rel, depth)) = stack.pop() { + let listing = list_dir_sync(&dir); + // The workspace root itself is exempt, as the walk always + // crawls the scan root (and never reaches a nested project + // inside a tagged dir). + if !rel.is_empty() && is_tagged_cache_dir(&dir, &listing) { + continue; + } + let hidden = rel.iter().any(|seg| seg.starts_with('.')); + if rel.iter().any(|seg| walk_prunes_name(seg)) + && has_node_modules_dir(&dir, &listing) + && dir.join("package.json").is_file() + && WorkspacesDialect::ALL.iter().any(|&dialect| { + let dialect = WorkspacesDialect { + dot: dialect.dot && !hidden, + ..dialect + }; + workspaces_include_as(&patterns, &rel, dialect) + }) + { + roots.push(dir.join("node_modules")); + } + if depth >= max_depth { + continue; + } + for entry in listing + .entries + .into_iter() + .filter(|e| walkable(e) && (rest_names_dot || !e.name_str.starts_with('.'))) + { + let Some(left) = budget.checked_sub(1) else { + break; + }; + budget = left; + let mut child_rel = rel.clone(); + child_rel.push(entry.name_str.clone()); + stack.push((dir.join(&entry.name), child_rel, depth + 1)); + } } } } @@ -3412,7 +3440,7 @@ impl NpmCrawler { // Recursively search for workspace node_modules results.extend(Self::find_workspace_node_modules(start_path, listing)); - // Plus the declared members the walk prunes by name (#1627). + // Plus the declared members the walk prunes by name (#1627, #1827). let members = skipped_workspace_member_roots(start_path, &results); results.extend(members); @@ -3507,9 +3535,7 @@ impl NpmCrawler { .into_iter() .filter(|entry| { entry.file_type.is_some_and(|ft| ft.is_dir()) - && !(entry.name_str == "node_modules" - || entry.name_str.starts_with('.') - || SKIP_DIRS.contains(&entry.name_str.as_str())) + && !(entry.name_str == "node_modules" || walk_prunes_name(&entry.name_str)) }) .map(|entry| dir.join(&entry.name)) .collect() diff --git a/crates/socket-patch-core/src/crawlers/npm_crawler/oracle.rs b/crates/socket-patch-core/src/crawlers/npm_crawler/oracle.rs index 5663ba351..4ccdf59a0 100644 --- a/crates/socket-patch-core/src/crawlers/npm_crawler/oracle.rs +++ b/crates/socket-patch-core/src/crawlers/npm_crawler/oracle.rs @@ -494,7 +494,7 @@ impl LegacyNpmCrawler { // Recursively search for workspace node_modules Self::find_workspace_node_modules(start_path, &mut results).await; - // The declared members the walk prunes by name (#1627) and the + // The declared members the walk prunes by name (#1627, #1827) and the // package-manager-configured roots are shared with the parent // module (not part of the walk this oracle checks). let members = skipped_workspace_member_roots(start_path, &results); diff --git a/crates/socket-patch-core/src/utils/workspace_globs.rs b/crates/socket-patch-core/src/utils/workspace_globs.rs index 94d1d4ff6..8d6e1a774 100644 --- a/crates/socket-patch-core/src/utils/workspace_globs.rs +++ b/crates/socket-patch-core/src/utils/workspace_globs.rs @@ -518,7 +518,7 @@ const MAX_BRACE_EXPANSIONS: usize = 4096; /// the first `{...}` group holding a top-level `,` or a `x..y[..step]` /// sequence is replaced by each alternative, recursively. A group with /// neither, and an unbalanced `{`, stay literal. -fn expand_braces(pattern: &str) -> Vec { +pub(crate) fn expand_braces(pattern: &str) -> Vec { let mut out = Vec::new(); expand_braces_into(pattern, &mut out); out diff --git a/crates/socket-patch-core/tests/crawler_npm_e2e.rs b/crates/socket-patch-core/tests/crawler_npm_e2e.rs index 39079287c..2e372c21b 100644 --- a/crates/socket-patch-core/tests/crawler_npm_e2e.rs +++ b/crates/socket-patch-core/tests/crawler_npm_e2e.rs @@ -1036,6 +1036,99 @@ async fn crawl_all_finds_declared_members_with_skip_dir_names() { } } +/// #1827: a declared `workspaces` member inside a hidden directory (a +/// JavaScript GitHub Action kept at `.github/actions/`, an explicit +/// `.config/tool`) is crawled too, though the walk prunes hidden dirs: +/// npm, yarn and Bun install its unhoistable deps into +/// `/node_modules` from the root lock. +#[tokio::test] +#[serial_test::parallel] +async fn crawl_all_finds_declared_members_in_hidden_dirs() { + for (workspaces, members) in [ + (r#"[".github/actions/*"]"#, &[".github/actions/x"][..]), + ( + r#"["packages/*",".github/actions/*"]"#, + &["packages/y", ".github/actions/x"][..], + ), + (r#"[".config/tool"]"#, &[".config/tool"][..]), + (r#"{"packages":[".github/**"]}"#, &[".github/actions/x"][..]), + ( + r#"["{.github/actions,tools}/*"]"#, + &[".github/actions/x"][..], + ), + ( + r#"["{.github,packages}/**"]"#, + &[".github/actions/x", "packages/build"][..], + ), + // A dot the glob part spells, below a skip-named dir, and a + // skip-named member below a hidden one. + ( + r#"["build/.*",".tools/*"]"#, + &["build/.x", ".tools/dist"][..], + ), + ] { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + tokio::fs::write( + root.join("package.json"), + format!(r#"{{"name":"root","private":true,"workspaces":{workspaces}}}"#), + ) + .await + .unwrap(); + stage_npm_pkg(&root.join("node_modules"), "ms", "2.1.3").await; + let mut expected = vec![root.join("node_modules").join("ms")]; + for member in members { + expected.push(stage_member(root, member).await); + } + + let result = NpmCrawler.crawl_all(&options_at(root)).await; + let mut got: Vec = result.iter().map(|p| p.path.clone()).collect(); + got.sort(); + expected.sort(); + assert_eq!(got, expected, "workspaces {workspaces}"); + } +} + +/// #1827 control: a hidden dir no `workspaces` pattern declares, one +/// declared but without a `package.json`, and one a wildcard reaches but +/// no pattern spells (build output such as Nuxt's `.output/server`) +/// stay pruned. +#[tokio::test] +#[serial_test::parallel] +async fn crawl_all_still_skips_undeclared_hidden_dirs() { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + tokio::fs::write( + root.join("package.json"), + r#"{"name":"root","private":true,"workspaces":["packages/*",".github/actions/*","apps/**","{.github,tools}/**"]}"#, + ) + .await + .unwrap(); + // Reached only through a wildcard. + stage_member(root, "packages/.hidden").await; + stage_member(root, "apps/site/.output/server").await; + // A brace alternative that spells a dot reaches only the dir it + // names, not every hidden dir. + stage_member(root, ".git/build").await; + stage_member(root, "tools/.cache/build").await; + // Undeclared: `.cache/tool` at the root and `packages/x/.out` below a + // member, each a would-be member with its own `package.json`. + stage_member(root, ".cache/tool").await; + let member = stage_member(root, "packages/x").await; + stage_member(root, "packages/x/.out").await; + // Declared by `.github/actions/*` but no package.json. + stage_npm_pkg( + &root.join(".github/actions/bare/node_modules"), + "not-a-member", + "1.0", + ) + .await; + + let result = NpmCrawler.crawl_all(&options_at(root)).await; + let got: Vec = result.iter().map(|p| p.path.clone()).collect(); + assert_eq!(got, vec![member]); +} + /// #1627: the members of a workspace nested below the scan root (found by /// the walk through its own `node_modules`) count too. #[tokio::test] diff --git a/docs/ecosystems.md b/docs/ecosystems.md index ea59c4dc2..c694d1ee0 100644 --- a/docs/ecosystems.md +++ b/docs/ecosystems.md @@ -492,8 +492,11 @@ itself (packages are read from it, not searched for workspaces), or directories named `dist`, `build`, `coverage`, `tmp`, `temp`, `__pycache__` or `vendor`, except that a workspace member the root `package.json` `workspaces` field declares (a directory holding a `package.json` that any -of npm, yarn or Bun reads the patterns as listing, such as `packages/build` -or `vendor/lib`) still has its own `node_modules` crawled. npm, yarn and +of npm, yarn or Bun reads the patterns as listing, such as `packages/build`, +`vendor/lib`, or a GitHub Action kept at `.github/actions/`) still has +its own `node_modules` crawled. A hidden directory counts only where a +pattern spells its dot (`.github/actions/*`, `.config/tool`); a wildcard +alone never reaches one. npm, yarn and Bun install a member's unhoistable dependencies there from the root lock. It also prunes every directory that carries a [Cache Directory Tagging](https://bford.info/cachedir/) `CACHEDIR.TAG` file