Skip to content

Commit 630dce6

Browse files
Fix hidden npm workspace members left unpatched (#1827) (#1829)
* Start fix for #1827 Assisted-by: Claude Code:claude-opus-5-5 * Crawl npm workspace members in hidden dirs A workspace member declared under a hidden directory, such as a GitHub Action kept at .github/actions/<name> or an explicit .config/tool entry, was never crawled in agent mode. Its nested node_modules copy stayed unpatched while apply --check reported in sync and vex attested not_affected. The member walk that adds back declared members the project walk prunes now descends into hidden dirs and keeps members whose path crosses a hidden name, as it already did for build-dir names. Each package manager's glob reading still decides whether a wildcard matches a dot component. Fixes #1827 Assisted-by: Claude Code:claude-opus-5-5 * Count hidden members only where a dot is spelled The previous commit let wildcards walk into every hidden dir. A ** pattern could then spend its walk budget inside .git or .next and drop real members, and build output such as .output/server could be crawled as a member that no package manager installed. A hidden dir now counts only where the workspaces pattern spells its dot (.github/actions/*, .config/tool), the way pnpm, Bun and vlt read patterns. The walk and the member check also share one pruned-name rule with the main walk, and run the cheap listing checks before the glob match. Refs #1827 Assisted-by: Claude Code:claude-opus-5-5 * Expand workspace braces before the member walk A pattern such as {.github,packages}/** has a dot only in one brace alternative, but the walk treated the whole pattern as spelling a dot and descended into every hidden dir, .git and .next included. That could use up the walk's dir budget and drop real members. Each brace alternative now 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. Refs #1827 Assisted-by: Claude Code:claude-opus-5-5 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 9932134 commit 630dce6

6 files changed

Lines changed: 299 additions & 93 deletions

File tree

‎crates/socket-patch-cli/tests/in_process_alternate_installers.rs‎

Lines changed: 87 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1130,10 +1130,22 @@ async fn yarn_berry_node_modules_linker_apply_patches_file() {
11301130
fn stage_skip_dir_member_workspace(
11311131
root: &Path,
11321132
member: &str,
1133+
) -> (PathBuf, PathBuf, Vec<u8>, String, String) {
1134+
stage_member_workspace(root, r#"["packages/*"]"#, &format!("packages/{member}"))
1135+
}
1136+
1137+
/// [`stage_skip_dir_member_workspace`] with the root's `workspaces` value
1138+
/// and the second member's path (`/`-separated, below the root) given.
1139+
fn stage_member_workspace(
1140+
root: &Path,
1141+
workspaces: &str,
1142+
member: &str,
11331143
) -> (PathBuf, PathBuf, Vec<u8>, String, String) {
11341144
std::fs::write(
11351145
root.join("package.json"),
1136-
r#"{"name":"root","version":"1.0.0","private":true,"workspaces":["packages/*"],"dependencies":{"ms":"2.1.3"}}"#,
1146+
format!(
1147+
r#"{{"name":"root","version":"1.0.0","private":true,"workspaces":{workspaces},"dependencies":{{"ms":"2.1.3"}}}}"#
1148+
),
11371149
)
11381150
.unwrap();
11391151
let stage_ms = |nm: PathBuf, version: &str| {
@@ -1153,8 +1165,9 @@ fn stage_skip_dir_member_workspace(
11531165
};
11541166
stage_ms(root.join("node_modules"), "2.1.3");
11551167
let mut copies = Vec::new();
1156-
for name in ["app", member] {
1157-
let dir = root.join("packages").join(name);
1168+
for rel in ["packages/app", member] {
1169+
let dir = root.join(rel);
1170+
let name = rel.rsplit('/').next().unwrap();
11581171
std::fs::create_dir_all(&dir).unwrap();
11591172
std::fs::write(
11601173
dir.join("package.json"),
@@ -1256,6 +1269,77 @@ async fn apply_check_and_vex_see_unpatched_member_named_like_a_build_dir() {
12561269
assert!(doc.contains("not_affected"), "{doc}");
12571270
}
12581271

1272+
/// #1827: agent-mode apply patches the nested copy of a declared workspace
1273+
/// member inside a hidden directory (`.github/actions/x` through a glob,
1274+
/// `.config/tool` listed literally), not just the other member's copy.
1275+
#[tokio::test]
1276+
#[serial]
1277+
async fn apply_patches_workspace_member_in_a_hidden_dir() {
1278+
for (workspaces, member) in [
1279+
(r#"["packages/*",".github/actions/*"]"#, ".github/actions/x"),
1280+
(r#"["packages/*",".config/tool"]"#, ".config/tool"),
1281+
] {
1282+
let tmp = tempfile::tempdir().unwrap();
1283+
let (app_copy, member_copy, patched, before_hash, after_hash) =
1284+
stage_member_workspace(tmp.path(), workspaces, member);
1285+
1286+
let code = apply_run(default_apply(tmp.path())).await;
1287+
assert_eq!(code, 0, "apply (member {member})");
1288+
assert_patched(&app_copy, &patched, &before_hash, &after_hash);
1289+
assert_patched(&member_copy, &patched, &before_hash, &after_hash);
1290+
}
1291+
}
1292+
1293+
/// #1827: with only the other member's copy patched, `apply --check`
1294+
/// reports the `.github/actions/x` copy as drift (exit 1), and `vex`
1295+
/// attests nothing as `not_affected` until that copy is patched too.
1296+
#[tokio::test]
1297+
#[serial]
1298+
async fn apply_check_and_vex_see_unpatched_member_in_a_hidden_dir() {
1299+
let tmp = tempfile::tempdir().unwrap();
1300+
let (app_copy, member_copy, patched, _, _) = stage_member_workspace(
1301+
tmp.path(),
1302+
r#"["packages/*",".github/actions/*"]"#,
1303+
".github/actions/x",
1304+
);
1305+
std::fs::write(&app_copy, &patched).unwrap();
1306+
assert_ne!(std::fs::read(&member_copy).unwrap(), patched);
1307+
1308+
let mut args = default_apply(tmp.path());
1309+
args.check = true;
1310+
let code = apply_run(args).await;
1311+
assert_eq!(
1312+
code, 1,
1313+
"apply --check must report the unpatched hidden member copy"
1314+
);
1315+
1316+
let vex_path = tmp.path().join("vex.json");
1317+
let vex = || async {
1318+
let _ = std::fs::remove_file(&vex_path);
1319+
let code = socket_patch_cli::commands::vex::run(socket_patch_cli::commands::vex::VexArgs {
1320+
common: default_apply(tmp.path()).common,
1321+
output: Some(vex_path.clone()),
1322+
product: Some("pkg:npm/root@1.0.0".to_string()),
1323+
no_verify: false,
1324+
doc_id: None,
1325+
compact: false,
1326+
})
1327+
.await;
1328+
(code, std::fs::read_to_string(&vex_path).unwrap_or_default())
1329+
};
1330+
let (code, doc) = vex().await;
1331+
assert!(
1332+
!doc.contains("not_affected"),
1333+
"VEX must not attest not_affected while .github/actions/x's copy is \
1334+
unpatched (exit {code}):\n{doc}"
1335+
);
1336+
// The control: with both copies patched, the same run attests it.
1337+
std::fs::write(&member_copy, &patched).unwrap();
1338+
let (code, doc) = vex().await;
1339+
assert_eq!(code, 0, "{doc}");
1340+
assert!(doc.contains("not_affected"), "{doc}");
1341+
}
1342+
12591343
// ---------------------------------------------------------------------------
12601344
// Rush pnpm symlink farm (hand-built, no real installer)
12611345
// ---------------------------------------------------------------------------

‎crates/socket-patch-core/src/crawlers/npm_crawler.rs‎

Lines changed: 112 additions & 86 deletions
Original file line numberDiff line numberDiff line change
@@ -66,8 +66,9 @@ pub(super) fn configured_install_roots(start_path: &Path) -> Vec<PathBuf> {
6666
}
6767

6868
/// The `node_modules` dirs of declared workspace members that the
69-
/// workspace walk prunes by name (#1627): members whose path crosses a
70-
/// [`SKIP_DIRS`] name (`packages/build`, `vendor/lib`). npm, yarn and Bun
69+
/// workspace walk prunes by name: members whose path crosses a
70+
/// [`SKIP_DIRS`] name (`packages/build`, `vendor/lib`, #1627) or a hidden
71+
/// one (`.github/actions/x`, `.config/tool`, #1827). npm, yarn and Bun
7172
/// install a member's unhoistable deps into `<member>/node_modules` from
7273
/// the root lock, so such a copy is as live as any other member's. The
7374
/// workspace roots are `start_path` and the project of every `walked`
@@ -95,19 +96,31 @@ pub(super) fn skipped_workspace_member_roots(
9596
roots
9697
}
9798

99+
/// Whether the workspace walk prunes a dir by its name alone: a hidden dir
100+
/// or a well-known build dir ([`SKIP_DIRS`]). Declared workspace members
101+
/// under such a name are added back by [`skipped_workspace_member_roots`].
102+
fn walk_prunes_name(name: &str) -> bool {
103+
name.starts_with('.') || SKIP_DIRS.contains(&name)
104+
}
105+
98106
/// [`skipped_workspace_member_roots`] for the one workspace root `root`. A
99107
/// member is a dir the root `package.json` `workspaces` patterns list in
100108
/// any package manager's reading ([`WorkspacesDialect::ALL`]: an extra root
101109
/// can only keep a copy, never drop one) that holds a `package.json`. Each
102110
/// pattern is walked from its literal prefix to the depth its segments
103111
/// allow (any depth for `**`, within [`WORKSPACE_GLOB_DIR_BUDGET`] dirs per
104112
/// pattern), with the walk's own rules: real dirs only (never a symlink),
105-
/// never a hidden dir, a `node_modules` or a tagged cache dir below the
106-
/// root, literal prefix included. Names are
107-
/// read from the listings, so the [`SKIP_DIRS`] test sees each dir as the
108-
/// walk does even when a pattern spells it in another case.
113+
/// never a `node_modules` or a tagged cache dir below the root, literal
114+
/// prefix included. A hidden dir counts only where the pattern spells the
115+
/// dot (`.github/actions/*`, `.config/tool`): the glob part descends into
116+
/// one only when it names a dot, and a path through one is read with no
117+
/// wildcard matching a dot component, as pnpm, Bun and vlt read it. So a
118+
/// `**` never spends its budget on `.git` or `.next`, and build output
119+
/// such as `.output/server` never becomes a member. Names are read from
120+
/// the listings, so the pruned-name test sees each dir as the walk does
121+
/// even when a pattern spells it in another case.
109122
fn skipped_members_of(root: &Path) -> Vec<PathBuf> {
110-
use crate::utils::workspace_globs::{workspaces_include_as, WorkspacesDialect};
123+
use crate::utils::workspace_globs::{expand_braces, workspaces_include_as, WorkspacesDialect};
111124

112125
let Some(patterns) = crate::utils::fs::read_regular_to_string_sync(&root.join("package.json"))
113126
.ok()
@@ -116,90 +129,105 @@ fn skipped_members_of(root: &Path) -> Vec<PathBuf> {
116129
return Vec::new();
117130
};
118131
let walkable = |entry: &ListedEntry| {
119-
entry.file_type.is_some_and(|ft| ft.is_dir())
120-
&& entry.name_str != "node_modules"
121-
&& !entry.name_str.starts_with('.')
132+
entry.file_type.is_some_and(|ft| ft.is_dir()) && entry.name_str != "node_modules"
122133
};
123134
let mut roots = Vec::new();
124135
// A leading `!` is a negation in some readings and part of an extglob
125136
// or a literal name in others; the dirs it can reach are walked either
126-
// way, and `workspaces_include_as` decides.
137+
// way, and `workspaces_include_as` decides. Braces are expanded first,
138+
// so each alternative walks from its own literal prefix and only an
139+
// alternative that spells a dot descends into hidden dirs; the
140+
// alternatives of one pattern share its dir budget.
127141
for pattern in &patterns {
128-
let segments: Vec<&str> = pattern
129-
.trim_start_matches('!')
130-
.split(['/', '\\'])
131-
.filter(|s| !s.is_empty() && *s != ".")
132-
.collect();
133-
let is_glob = |s: &str| s.contains(['*', '?', '[', ']', '{', '}', '(', ')', '!', '+', '@']);
134-
let literal = segments.iter().take_while(|s| !is_glob(s)).count();
135-
// The literal prefix, each component as its parent lists it (an
136-
// exact name first, else the ASCII-case-insensitive match a
137-
// case-insensitive filesystem resolves). A prefix that climbs out
138-
// (`..`) or is not walkable names nothing here.
139-
let mut dir = root.to_path_buf();
140-
let mut rel: Vec<String> = Vec::new();
141-
let mut reachable = true;
142-
for segment in &segments[..literal] {
143-
let entries: Vec<ListedEntry> = list_dir_sync(&dir)
144-
.entries
145-
.into_iter()
146-
.filter(|e| walkable(e))
147-
.collect();
148-
let found = entries.iter().find(|e| e.name_str == *segment).or_else(|| {
149-
entries
150-
.iter()
151-
.find(|e| e.name_str.eq_ignore_ascii_case(segment))
152-
});
153-
let Some(entry) = found else {
154-
reachable = false;
155-
break;
156-
};
157-
dir.push(&entry.name);
158-
rel.push(entry.name_str.clone());
159-
if is_tagged_cache_dir(&dir, &list_dir_sync(&dir)) {
160-
reachable = false;
161-
break;
162-
}
163-
}
164-
if !reachable {
165-
continue;
166-
}
167-
let rest = &segments[literal..];
168-
let max_depth = if rest.contains(&"**") {
169-
usize::MAX
170-
} else {
171-
rest.len()
172-
};
173142
let mut budget = WORKSPACE_GLOB_DIR_BUDGET;
174-
let mut stack = vec![(dir, rel, 0usize)];
175-
while let Some((dir, rel, depth)) = stack.pop() {
176-
let listing = list_dir_sync(&dir);
177-
// The workspace root itself is exempt, as the walk always
178-
// crawls the scan root (and never reaches a nested project
179-
// inside a tagged dir).
180-
if !rel.is_empty() && is_tagged_cache_dir(&dir, &listing) {
181-
continue;
182-
}
183-
if rel.iter().any(|seg| SKIP_DIRS.contains(&seg.as_str()))
184-
&& WorkspacesDialect::ALL
185-
.iter()
186-
.any(|&dialect| workspaces_include_as(&patterns, &rel, dialect))
187-
&& dir.join("package.json").is_file()
188-
&& has_node_modules_dir(&dir, &listing)
189-
{
190-
roots.push(dir.join("node_modules"));
143+
for alternative in expand_braces(pattern.trim_start_matches('!')) {
144+
let segments: Vec<&str> = alternative
145+
.split(['/', '\\'])
146+
.filter(|s| !s.is_empty() && *s != ".")
147+
.collect();
148+
let is_glob =
149+
|s: &str| s.contains(['*', '?', '[', ']', '{', '}', '(', ')', '!', '+', '@']);
150+
let literal = segments.iter().take_while(|s| !is_glob(s)).count();
151+
// The literal prefix, each component as its parent lists it (an
152+
// exact name first, else the ASCII-case-insensitive match a
153+
// case-insensitive filesystem resolves). A prefix that climbs out
154+
// (`..`) or is not walkable names nothing here.
155+
let mut dir = root.to_path_buf();
156+
let mut rel: Vec<String> = Vec::new();
157+
let mut reachable = true;
158+
for segment in &segments[..literal] {
159+
let entries: Vec<ListedEntry> = list_dir_sync(&dir)
160+
.entries
161+
.into_iter()
162+
.filter(|e| walkable(e))
163+
.collect();
164+
let found = entries.iter().find(|e| e.name_str == *segment).or_else(|| {
165+
entries
166+
.iter()
167+
.find(|e| e.name_str.eq_ignore_ascii_case(segment))
168+
});
169+
let Some(entry) = found else {
170+
reachable = false;
171+
break;
172+
};
173+
dir.push(&entry.name);
174+
rel.push(entry.name_str.clone());
175+
if is_tagged_cache_dir(&dir, &list_dir_sync(&dir)) {
176+
reachable = false;
177+
break;
178+
}
191179
}
192-
if depth >= max_depth {
180+
if !reachable {
193181
continue;
194182
}
195-
for entry in listing.entries.into_iter().filter(|e| walkable(e)) {
196-
let Some(left) = budget.checked_sub(1) else {
197-
break;
198-
};
199-
budget = left;
200-
let mut child_rel = rel.clone();
201-
child_rel.push(entry.name_str.clone());
202-
stack.push((dir.join(&entry.name), child_rel, depth + 1));
183+
let rest = &segments[literal..];
184+
let rest_names_dot = rest
185+
.iter()
186+
.any(|s| s.starts_with('.') || s.contains("(.") || s.contains("|."));
187+
let max_depth = if rest.contains(&"**") {
188+
usize::MAX
189+
} else {
190+
rest.len()
191+
};
192+
let mut stack = vec![(dir, rel, 0usize)];
193+
while let Some((dir, rel, depth)) = stack.pop() {
194+
let listing = list_dir_sync(&dir);
195+
// The workspace root itself is exempt, as the walk always
196+
// crawls the scan root (and never reaches a nested project
197+
// inside a tagged dir).
198+
if !rel.is_empty() && is_tagged_cache_dir(&dir, &listing) {
199+
continue;
200+
}
201+
let hidden = rel.iter().any(|seg| seg.starts_with('.'));
202+
if rel.iter().any(|seg| walk_prunes_name(seg))
203+
&& has_node_modules_dir(&dir, &listing)
204+
&& dir.join("package.json").is_file()
205+
&& WorkspacesDialect::ALL.iter().any(|&dialect| {
206+
let dialect = WorkspacesDialect {
207+
dot: dialect.dot && !hidden,
208+
..dialect
209+
};
210+
workspaces_include_as(&patterns, &rel, dialect)
211+
})
212+
{
213+
roots.push(dir.join("node_modules"));
214+
}
215+
if depth >= max_depth {
216+
continue;
217+
}
218+
for entry in listing
219+
.entries
220+
.into_iter()
221+
.filter(|e| walkable(e) && (rest_names_dot || !e.name_str.starts_with('.')))
222+
{
223+
let Some(left) = budget.checked_sub(1) else {
224+
break;
225+
};
226+
budget = left;
227+
let mut child_rel = rel.clone();
228+
child_rel.push(entry.name_str.clone());
229+
stack.push((dir.join(&entry.name), child_rel, depth + 1));
230+
}
203231
}
204232
}
205233
}
@@ -3412,7 +3440,7 @@ impl NpmCrawler {
34123440

34133441
// Recursively search for workspace node_modules
34143442
results.extend(Self::find_workspace_node_modules(start_path, listing));
3415-
// Plus the declared members the walk prunes by name (#1627).
3443+
// Plus the declared members the walk prunes by name (#1627, #1827).
34163444
let members = skipped_workspace_member_roots(start_path, &results);
34173445
results.extend(members);
34183446

@@ -3507,9 +3535,7 @@ impl NpmCrawler {
35073535
.into_iter()
35083536
.filter(|entry| {
35093537
entry.file_type.is_some_and(|ft| ft.is_dir())
3510-
&& !(entry.name_str == "node_modules"
3511-
|| entry.name_str.starts_with('.')
3512-
|| SKIP_DIRS.contains(&entry.name_str.as_str()))
3538+
&& !(entry.name_str == "node_modules" || walk_prunes_name(&entry.name_str))
35133539
})
35143540
.map(|entry| dir.join(&entry.name))
35153541
.collect()

‎crates/socket-patch-core/src/crawlers/npm_crawler/oracle.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -494,7 +494,7 @@ impl LegacyNpmCrawler {
494494

495495
// Recursively search for workspace node_modules
496496
Self::find_workspace_node_modules(start_path, &mut results).await;
497-
// The declared members the walk prunes by name (#1627) and the
497+
// The declared members the walk prunes by name (#1627, #1827) and the
498498
// package-manager-configured roots are shared with the parent
499499
// module (not part of the walk this oracle checks).
500500
let members = skipped_workspace_member_roots(start_path, &results);

‎crates/socket-patch-core/src/utils/workspace_globs.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -518,7 +518,7 @@ const MAX_BRACE_EXPANSIONS: usize = 4096;
518518
/// the first `{...}` group holding a top-level `,` or a `x..y[..step]`
519519
/// sequence is replaced by each alternative, recursively. A group with
520520
/// neither, and an unbalanced `{`, stay literal.
521-
fn expand_braces(pattern: &str) -> Vec<String> {
521+
pub(crate) fn expand_braces(pattern: &str) -> Vec<String> {
522522
let mut out = Vec::new();
523523
expand_braces_into(pattern, &mut out);
524524
out

0 commit comments

Comments
 (0)