fix(global-cli): bound local vite-plus resolution to the workspace root - #2574
fix(global-cli): bound local vite-plus resolution to the workspace root#2574Akokk0 wants to merge 3 commits into
Conversation
Local CLI resolution (oxc_resolver in the JS executor) and the `vp --version` "Local vite-plus" probe both walk every ancestor directory's node_modules, Node-style. When the project's own install is missing or broken (e.g. after a corrupted install), resolution escapes the project and silently picks up an unrelated ancestor project's copy: delegation then runs another project's vite-plus, and `vp --version` reports that copy's version and bundled tool versions as "Local". Bound the walk at the project's workspace root via `vt_workspace::find_workspace_root`: - within the workspace, nearest wins - a workspace member still resolves the workspace root's install; - beyond it, resolution fails, so delegation falls back to the global installation and the existing missing-local-cli warning (voidzero-dev#2361) explains the state instead of masking it; - when there is no workspace or package root at all, the walk stays unbounded (unchanged behavior for markerless directories). `find_local_vite_plus` in version.rs now derives from the same bounded walk, so what --version displays is what delegation would execute. Tested: unit tests cover the escape (red without the gate), the workspace-member case, and the markerless case; verified end-to-end with a nested-project fixture where 0.3.0 reports the outer project's copy and the patched build reports "Not found". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The test's premise is that no ancestor of the tempdir carries a package.json. That holds for /tmp and /var/folders, but Windows' %TEMP% lives under the user profile, where a stray package.json would create a workspace boundary and fail the test for environmental reasons. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
✅ Deploy Preview for viteplus-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
The red CI is a real regression from this PR. All 75 snapshot failures have the same cause: the snapshot harness links the only real I'll push a narrower fix: the boundary only applies when the project itself declares a |
the previous commit bounded local resolution at the workspace root for every project. that breaks a layout the repo itself relies on: the snapshot harness stages workspaces with no node_modules of their own and resolves the run-root install through Node's unbounded upward walk, so all three CLI snapshot jobs went red with the same signature - the global CLI stopped seeing the project-local install (75 cases, every diff a "does not use vite-plus" warning) walking past the package root is ordinary Node resolution semantics and hoisted installs depend on it, so the default stays unbounded. the boundary now applies only when the project declares a vite-plus dependency - directly or at its workspace root, the same test warn_missing_local_cli_if_project uses - because that is exactly the case where "run vp install" is the right answer rather than silently borrowing an unrelated ancestor's copy - new test pins the harness-shaped layout: an undeclared staged workspace keeps resolving the run-root install (mutation-verified: removing the declaration filter reds it) - the workspace-member test's root now declares the dependency so the bounded walk is actually engaged rather than passing via the unbounded default - snapshot fixtures do not declare vite-plus, so they take the unbounded path; the declared-but-missing fixture resolves its own install at the first hop either way Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fengmk2
added
test: e2e
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
What
Local vite-plus resolution walks every ancestor directory's
node_modules, Node-style, in two independent places:js_executor.rs—resolve_local_vite_plus_package_dir(oxc_resolver), which decides which local CLI delegation actually executes;commands/version.rs—find_local_vite_plus(a hand-rolled upward loop), which fillsvp --version's "Local vite-plus" row and the Tools table.Neither has a project boundary. Whenever the project's own install is missing — for any reason: a deleted or partially-installed
node_modules, a fresh checkout before the first install, a broken install — resolution escapes the project and silently picks up an unrelated ancestor project's copy:vp run/vp test/ etc. delegate to another project's vite-plus, andvp --versionreports that copy's version and bundled tool versions as "Local".The escape reproduces deterministically on a clean nested-project fixture with the release binary (see Testing) — no special prior state needed beyond an ancestor directory that happens to contain a
node_modules/vite-plus.Real-world case where I first noticed it: a workspace checked out at
~/NodeProject/parent/external/repowhose own install had lostnode_modules/vite-plus— every vp command silently used~/NodeProject/parent/node_modules/vite-plus(an old 0.2.1), andvp --versionshowed vitest 4.1.9 / oxfmt 0.55 / oxlint 1.70, versions nothing in the workspace declared. All gates stayed green on the wrong toolchain.How
Added
JsExecutor::local_vite_plus_install_host: an upward walk bounded at the project's workspace root (viavt_workspace::find_workspace_root), used as an eligibility gate before the existing oxc_resolver resolution, and as the single source forfind_local_vite_plusin version.rs — so what--versiondisplays is what delegation would execute.Semantics:
find_workspace_rooterrors): the walk stays unbounded — unchanged behavior for markerless directories.Testing
New unit tests in
js_executor.rs: the nested-project escape (fails without the gate — verified red), the workspace-member case, and the markerless case; the two existing pnpm-symlink-layout tests inversion.rskeep passing through the new path.cargo test -p vp_global_cli --bins: 485 passed, 1 failed —detect_system_node_version_returns_version, which fails identically on unmodifiedmainin my environment (node on PATH is a vp shim, no "system node" to detect), unrelated to this change.cargo clippyclean,cargo fmtclean.End-to-end against a nested-project fixture: release 0.3.0 reports the outer project's 0.2.1 as "Local vite-plus"; this build reports "Not found" and falls back with the existing warning. A workspace-member fixture still resolves the root install.
Minimal reproduction, ~30 seconds, no install needed (the ancestor copy is a fabricated one-line
package.json):Release 0.3.0 prints
Local vite-plus v0.2.1— the fabricated ancestor copy, despite inner declaring 0.3.0; this build printsNot found.Not run locally: the
vp_cli_snapshotssuite — its bootstrap (install-global-cli) installs a local-dev build into the real vp data dir and switchescurrent, which I didn't want to do to my daily installation; deferring to CI. I reviewedmissing_local_cli_warning's steps against the new semantics and expect no snapshot change (the fixture's install lives inside the project, so the gate admits it at the first hop).For transparency: I originally observed this alongside #2573, whose lockfile-corruption half I could not reproduce afterwards and have retracted there. The escape described here is independent of that — it reproduces from a clean fixture and is pinned by the new unit tests. Related to the fallback discussion in #2361.