Skip to content

Commit 556f0f5

Browse files
committed
refactor(global-cli): enforce local CLI boundaries with oxc_resolver
1 parent 0167510 commit 556f0f5

5 files changed

Lines changed: 226 additions & 62 deletions

File tree

crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/local_cli_workspace_boundary/snapshots.toml

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,3 +64,29 @@ steps = [
6464
{ argv = ["vp", "--version"], cwd = "outer/external/inner" },
6565
{ argv = ["vp", "lint", "--version"], cwd = "outer/external/inner" },
6666
]
67+
68+
[[case]]
69+
name = "package_json_not_exported"
70+
vp = "global"
71+
comment = "Version reporting and delegation both require the local package.json export."
72+
steps = [
73+
{ argv = ["vpt", "mkdir", "-p", "outer/external/inner/node_modules"], snapshot = false },
74+
{ argv = ["vpt", "cp", "-r", "outer-cli", "outer/external/inner/node_modules/vite-plus"], snapshot = false },
75+
{ argv = ["vpt", "write-file", "outer/external/inner/node_modules/vite-plus/package.json", '{"name":"vite-plus","version":"9.8.7","exports":{".":"./dist/bin.js"}}'], snapshot = false },
76+
{ argv = ["vp", "--version"], cwd = "outer/external/inner" },
77+
{ argv = ["vp", "lint", "--version"], cwd = "outer/external/inner" },
78+
]
79+
80+
[[case]]
81+
name = "rootless_malformed_ancestor"
82+
vp = "global"
83+
comment = "Oxc rejects a malformed ancestor manifest even when a rootless workspace has a local installation."
84+
steps = [
85+
{ argv = ["vpt", "mkdir", "-p", "outer/external/inner/node_modules"], snapshot = false },
86+
{ argv = ["vpt", "cp", "-r", "outer-cli", "outer/external/inner/node_modules/vite-plus"], snapshot = false },
87+
{ argv = ["vpt", "rm", "outer/external/inner/package.json"], snapshot = false },
88+
{ argv = ["vpt", "write-file", "outer/external/inner/pnpm-workspace.yaml", "packages: []\n"], snapshot = false },
89+
{ argv = ["vpt", "write-file", "outer/package.json", "{"], snapshot = false },
90+
{ argv = ["vp", "--version"], cwd = "outer/external/inner" },
91+
{ argv = ["vp", "lint", "--version"], cwd = "outer/external/inner" },
92+
]
Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
# package_json_not_exported
2+
3+
Version reporting and delegation both require the local package.json export.
4+
5+
## `vpt mkdir -p outer/external/inner/node_modules`
6+
7+
8+
## `vpt cp -r outer-cli outer/external/inner/node_modules/vite-plus`
9+
10+
11+
## `vpt write-file outer/external/inner/node_modules/vite-plus/package.json '{"name":"vite-plus","version":"9.8.7","exports":{".":"./dist/bin.js"}}'`
12+
13+
14+
## `cd outer/external/inner && vp --version`
15+
16+
```
17+
VITE+ - The Unified Toolchain for the Web
18+
19+
vp <version>
20+
21+
Local vite-plus:
22+
vite-plus Not found
23+
24+
Tools:
25+
vite Not found
26+
rolldown Not found
27+
vitest Not found
28+
oxfmt Not found
29+
oxlint Not found
30+
oxlint-tsgolint Not found
31+
tsdown Not found
32+
33+
Environment:
34+
Package manager Not found
35+
Node.js <version>
36+
```
37+
38+
## `cd outer/external/inner && vp lint --version`
39+
40+
```
41+
VITE+ - The Unified Toolchain for the Web
42+
43+
warn: No project-local vite-plus installation was found. Run `vp install` in `<workspace>/outer/external/inner` to install dependencies.
44+
Version: 1.81.0
45+
```
Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
# rootless_malformed_ancestor
2+
3+
Oxc rejects a malformed ancestor manifest even when a rootless workspace has a local installation.
4+
5+
## `vpt mkdir -p outer/external/inner/node_modules`
6+
7+
8+
## `vpt cp -r outer-cli outer/external/inner/node_modules/vite-plus`
9+
10+
11+
## `vpt rm outer/external/inner/package.json`
12+
13+
14+
## `vpt write-file outer/external/inner/pnpm-workspace.yaml 'packages: []
15+
'`
16+
17+
18+
## `vpt write-file outer/package.json {`
19+
20+
21+
## `cd outer/external/inner && vp --version`
22+
23+
```
24+
VITE+ - The Unified Toolchain for the Web
25+
26+
vp <version>
27+
28+
Local vite-plus:
29+
vite-plus Not found
30+
31+
Tools:
32+
vite Not found
33+
rolldown Not found
34+
vitest Not found
35+
oxfmt Not found
36+
oxlint Not found
37+
oxlint-tsgolint Not found
38+
tsdown Not found
39+
40+
Environment:
41+
Package manager pnpm latest
42+
Node.js <version>
43+
```
44+
45+
## `cd outer/external/inner && vp lint --version`
46+
47+
```
48+
VITE+ - The Unified Toolchain for the Web
49+
50+
warn: This project does not use vite-plus. Learn how to migrate: https://viteplus.dev/guide/migrate
51+
Version: 1.81.0
52+
```

crates/vp_global_cli/src/commands/version.rs

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -65,15 +65,11 @@ fn read_package_json(package_json_path: &Path) -> Option<PackageJson> {
6565
}
6666

6767
fn find_local_vite_plus(cwd: &AbsolutePath) -> Option<LocalVitePlus> {
68-
// The workspace-bounded walk keeps this display consistent with what
69-
// delegation would actually execute (see `local_vite_plus_install_host`).
70-
let host = JsExecutor::local_vite_plus_install_host(cwd)?;
71-
let package_dir = host.as_path().join("node_modules").join("vite-plus");
72-
let pkg = read_package_json(&package_dir.join("package.json"))?;
73-
// Follow symlinks (pnpm links node_modules/vite-plus -> node_modules/.pnpm/.../vite-plus)
74-
// so parent traversal can discover colocated dependency links.
75-
let package_dir = fs::canonicalize(&package_dir).unwrap_or(package_dir);
76-
Some(LocalVitePlus { version: pkg.version, package_dir })
68+
let resolved = JsExecutor::resolve_local_vite_plus_package(cwd)?;
69+
Some(LocalVitePlus {
70+
version: resolved.package_json()?.version()?.to_owned(),
71+
package_dir: resolved.path().parent()?.to_path_buf(),
72+
})
7773
}
7874

7975
fn read_toolchain_manifest(local: &LocalVitePlus) -> Option<vp_toolchain::Manifest> {

crates/vp_global_cli/src/js_executor.rs

Lines changed: 98 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -407,49 +407,31 @@ impl JsExecutor {
407407
Ok(output)
408408
}
409409

410-
/// Find the directory whose `node_modules/vite-plus` an upward walk from
411-
/// `project_path` is allowed to use.
412-
///
413-
/// Declared Vite+ projects stay within their package or workspace boundary;
414-
/// an ancestor workspace must include the package to supply its install.
415-
/// Unreadable manifests preserve the nearest known package boundary.
416-
/// Undeclared projects and markerless directories keep Node's upward walk.
417-
pub(crate) fn local_vite_plus_install_host(
410+
/// Resolve the local package while restricting lookup to the project boundary.
411+
pub(crate) fn resolve_local_vite_plus_package(
418412
project_path: &AbsolutePath,
419-
) -> Option<AbsolutePathBuf> {
420-
let boundary = commands::local_vite_plus_boundary(project_path);
413+
) -> Option<oxc_resolver::Resolution> {
414+
use oxc_resolver::{ResolveOptions, Resolver, Restriction};
421415

422-
let mut current = project_path;
423-
loop {
424-
if current.join("node_modules/vite-plus/package.json").as_path().exists() {
425-
return Some(current.to_absolute_path_buf());
426-
}
427-
if boundary.as_deref().is_some_and(|boundary| current == boundary) {
428-
return None;
429-
}
430-
match current.parent() {
431-
Some(parent) if parent != current => current = parent,
432-
_ => return None,
433-
}
416+
let mut options = ResolveOptions {
417+
condition_names: vec!["import".into(), "node".into()],
418+
..ResolveOptions::default()
419+
};
420+
if let Some(boundary) = commands::local_vite_plus_boundary(project_path) {
421+
// Restrictions inspect the lookup path before symlinks are resolved.
422+
// A project-local link may point to a package stored outside the project.
423+
options.restrictions.push(Restriction::Fn(std::sync::Arc::new(move |path| {
424+
path.starts_with(boundary.as_path())
425+
})));
434426
}
427+
Resolver::new(options).resolve(project_path, "vite-plus/package.json").ok()
435428
}
436429

437430
/// Resolve the local vite-plus package root from the project directory.
438431
pub(crate) fn resolve_local_vite_plus_package_dir(
439432
project_path: &AbsolutePath,
440433
) -> Option<AbsolutePathBuf> {
441-
use oxc_resolver::{ResolveOptions, Resolver};
442-
443-
// Enforce the workspace boundary before using Node's unbounded resolver.
444-
Self::local_vite_plus_install_host(project_path)?;
445-
446-
let resolver = Resolver::new(ResolveOptions {
447-
condition_names: vec!["import".into(), "node".into()],
448-
..ResolveOptions::default()
449-
});
450-
451-
// Resolve vite-plus/package.json from the project directory to find the package root
452-
let resolved = resolver.resolve(project_path, "vite-plus/package.json").ok()?;
434+
let resolved = Self::resolve_local_vite_plus_package(project_path)?;
453435
let pkg_dir = resolved.path().parent()?;
454436
AbsolutePathBuf::new(pkg_dir.to_path_buf())
455437
}
@@ -563,7 +545,10 @@ mod tests {
563545
std::fs::create_dir_all(package_dir.join("dist")).unwrap();
564546
std::fs::write(
565547
package_dir.join("package.json"),
566-
vt_str::format!(r#"{{"version":"{version}"}}"#).as_bytes(),
548+
vt_str::format!(
549+
r#"{{"name":"vite-plus","version":"{version}","exports":{{"./package.json":"./package.json"}}}}"#
550+
)
551+
.as_bytes(),
567552
)
568553
.unwrap();
569554
std::fs::write(package_dir.join("dist/bin.js"), "").unwrap();
@@ -587,7 +572,6 @@ mod tests {
587572
.unwrap();
588573
std::fs::write(inner.join("pnpm-workspace.yaml"), "packages: []\n").unwrap();
589574

590-
assert_eq!(JsExecutor::local_vite_plus_install_host(&inner), None);
591575
assert_eq!(JsExecutor::resolve_local_vite_plus_package_dir(&inner), None);
592576
assert_eq!(JsExecutor::resolve_local_vite_plus(&inner), None);
593577
}
@@ -610,12 +594,12 @@ mod tests {
610594
std::fs::create_dir_all(&member).unwrap();
611595
std::fs::write(member.join("package.json"), r#"{"name":"app"}"#).unwrap();
612596

613-
let host = JsExecutor::local_vite_plus_install_host(&member)
614-
.expect("workspace root install must stay resolvable");
615-
assert_eq!(&host, ws);
616597
let pkg_dir = JsExecutor::resolve_local_vite_plus_package_dir(&member)
617598
.expect("workspace root install must stay resolvable");
618-
assert!(pkg_dir.as_path().ends_with("node_modules/vite-plus"));
599+
assert_eq!(
600+
pkg_dir.as_path(),
601+
std::fs::canonicalize(ws.join("node_modules/vite-plus")).unwrap()
602+
);
619603
}
620604

621605
#[test]
@@ -635,19 +619,25 @@ mod tests {
635619
std::fs::write(workspace.join("pnpm-workspace.yaml"), "packages: []\n").unwrap();
636620
for cwd in [&workspace, &workspace.join("src")] {
637621
assert_eq!(
638-
JsExecutor::local_vite_plus_install_host(cwd),
622+
JsExecutor::resolve_local_vite_plus_package_dir(cwd),
639623
None,
640624
"ancestor: {ancestor:?}"
641625
);
642626
assert_eq!(JsExecutor::resolve_local_vite_plus(cwd), None);
643627
}
644628

645-
// A missing root manifest must not prevent a workspace-local install.
646629
write_local_cli(&workspace, "0.3.0");
647-
assert_eq!(
648-
JsExecutor::local_vite_plus_install_host(&workspace).as_deref(),
649-
Some(workspace.as_ref())
650-
);
630+
if ancestor == Some("{") {
631+
// Oxc still reads package scope above a rootless workspace.
632+
assert!(JsExecutor::resolve_local_vite_plus_package(&workspace).is_none());
633+
} else {
634+
let package = JsExecutor::resolve_local_vite_plus_package_dir(&workspace)
635+
.expect("a missing root manifest must not prevent a local installation");
636+
assert_eq!(
637+
package.as_path(),
638+
std::fs::canonicalize(workspace.join("node_modules/vite-plus")).unwrap()
639+
);
640+
}
651641
}
652642
}
653643

@@ -730,7 +720,7 @@ mod tests {
730720
std::fs::create_dir_all(&inner).unwrap();
731721
std::fs::write(inner.join("package.json"), project).unwrap();
732722
assert_eq!(
733-
JsExecutor::local_vite_plus_install_host(&inner),
723+
JsExecutor::resolve_local_vite_plus_package_dir(&inner),
734724
None,
735725
"{ancestor}: {project}"
736726
);
@@ -750,12 +740,12 @@ mod tests {
750740
std::fs::write(inner.join("package.json"), r#"{"name":"inner"}"#).unwrap();
751741
std::fs::write(inner.join("pnpm-workspace.yaml"), "packages: []\n").unwrap();
752742

753-
let host = JsExecutor::local_vite_plus_install_host(&inner)
754-
.expect("undeclared projects keep the unbounded walk");
755-
assert_eq!(&host, outer);
756743
let pkg_dir = JsExecutor::resolve_local_vite_plus_package_dir(&inner)
757744
.expect("undeclared projects keep the unbounded walk");
758-
assert!(pkg_dir.as_path().ends_with("node_modules/vite-plus"));
745+
assert_eq!(
746+
pkg_dir.as_path(),
747+
std::fs::canonicalize(outer.join("node_modules/vite-plus")).unwrap()
748+
);
759749
}
760750

761751
/// Unix-only: Windows tempdirs can have a package.json in an ancestor profile
@@ -771,9 +761,64 @@ mod tests {
771761
let nested = root.join("a/b");
772762
std::fs::create_dir_all(&nested).unwrap();
773763

774-
let host = JsExecutor::local_vite_plus_install_host(&nested)
764+
let package = JsExecutor::resolve_local_vite_plus_package_dir(&nested)
775765
.expect("markerless directories keep the unbounded walk");
776-
assert_eq!(&host, root);
766+
assert_eq!(
767+
package.as_path(),
768+
std::fs::canonicalize(root.join("node_modules/vite-plus")).unwrap()
769+
);
770+
}
771+
772+
#[cfg(unix)]
773+
#[test]
774+
fn local_resolution_checks_symlink_location_before_its_target() {
775+
for local_link in [false, true] {
776+
for local_target in [false, true] {
777+
let temp = tempfile::tempdir().unwrap();
778+
let outer = AbsolutePath::new(temp.path()).unwrap();
779+
let project = outer.join("inner");
780+
std::fs::create_dir_all(&project).unwrap();
781+
std::fs::write(outer.join("package.json"), "{}").unwrap();
782+
std::fs::write(
783+
project.join("package.json"),
784+
r#"{"devDependencies":{"vite-plus":"0.3.0"}}"#,
785+
)
786+
.unwrap();
787+
788+
let store = if local_target { project.join("store") } else { outer.join("store") };
789+
write_local_cli(&store, "0.3.0");
790+
let target = store.join("node_modules/vite-plus");
791+
let host = if local_link { project.as_ref() } else { outer };
792+
std::fs::create_dir_all(host.join("node_modules")).unwrap();
793+
std::os::unix::fs::symlink(&target, host.join("node_modules/vite-plus")).unwrap();
794+
795+
let resolved = JsExecutor::resolve_local_vite_plus_package_dir(&project);
796+
let expected = local_link.then(|| {
797+
AbsolutePathBuf::new(std::fs::canonicalize(&target).unwrap()).unwrap()
798+
});
799+
assert_eq!(
800+
resolved, expected,
801+
"local link: {local_link}, local target: {local_target}"
802+
);
803+
}
804+
}
805+
}
806+
807+
#[test]
808+
fn local_resolution_supports_package_self_reference() {
809+
let temp = tempfile::tempdir().unwrap();
810+
let project = AbsolutePath::new(temp.path()).unwrap();
811+
std::fs::write(
812+
project.join("package.json"),
813+
r#"{"name":"vite-plus","version":"0.3.0","exports":{"./package.json":"./package.json"}}"#,
814+
)
815+
.unwrap();
816+
std::fs::create_dir_all(project.join("dist")).unwrap();
817+
std::fs::write(project.join("dist/bin.js"), "").unwrap();
818+
819+
let resolved = JsExecutor::resolve_local_vite_plus(project)
820+
.expect("a package can resolve its own exported CLI without node_modules");
821+
assert_eq!(resolved.as_path(), std::fs::canonicalize(project.join("dist/bin.js")).unwrap());
777822
}
778823

779824
#[test]

0 commit comments

Comments
 (0)