Repository navigation
Conversation
|
@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: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
| let node_system_only = vp_shared::EnvConfig::get().node_system_only(); | ||
| if !node_system_only { | ||
| env.prepend(node_bin_dir, &["node"], PrependOptions::default())?; | ||
| } | ||
| if vp_pm_cli::system_only_variable(PackageManagerType::Npm).is_some() { | ||
| return Ok(env); |
There was a problem hiding this comment.
[P2] Enforce system-only selection in the child PATH
With a managed runtime inherited first on PATH, setting both VP_NODE_MANAGER=system-only and VP_NPM_MANAGER=system-only selects the system npm executable but leaves that managed runtime first in its environment. The normal #!/usr/bin/env node shebang therefore executes managed Node, violating the new restriction despite successful system-tool discovery. The reverse combination also fails: with only npm set to system-only, prepending the permitted managed Node directory exposes its bundled npm, so nested npm calls use the managed copy. Preserve the restrictions through interpreter and child-process execution, including inherited-tool shortcuts, and add tests that execute the selected tools instead of only checking their paths.
Reproduced with the built PR binary: the system npm shebang printed MANAGED_NODE; the separate npm-only case printed SYSTEM_NPM_PARENT followed by MANAGED_NPM_CHILD. Both exited 0. These are two manifestations of the same PATH enforcement issue.
There was a problem hiding this comment.
Thanks — fixed in ba2d71a.
The shims now remove managed directories from the child PATH, instead of only choosing the system tool. When Node.js or npm is system-only, the managed runtime directory is removed, which also covers its bundled npm. When a package-manager family is system-only, that family's managed directories are removed. Other entries keep their order. This runs wherever the shim starts a tool (exec_tool / spawn_tool), so the inherited-tool shortcut is covered too. With only npm system-only, children still reach managed Node.js through the node shim.
The new snapshot case system_only_child_processes runs a #!/usr/bin/env node system npm that starts a nested npm. It covers both of your scenarios, and every step fails without the fix.
vpx (6fb3e80): I found the same issue in vpx; it predates this PR. vpx picks the executable from PATH before starting it, so restricting the child environment isn't enough. For system-only tools, vpx node / vpx npm and script mode now skip managed installs on PATH. When there is no system install, vpx fails instead of falling back to dlx, which could download the tool.
| let system_node = if node_system_only { | ||
| resolve_effective_tool("node", EffectiveMode::SystemOnly) | ||
| .map_err(|e| vec![e.to_string()])? |
There was a problem hiding this comment.
[P2] Stop re-entering an external manager that forwards to vp
An external Node manager can forward to the vp fallback shim without removing itself from PATH; the existing foreign_manager_falls_back_without_recursion fixture covers this arrangement. Under VP_NODE_MANAGER=system-only, this branch selects the same external manager again each time vp receives that forwarded call. node --version then loops through repeated execs instead of reporting that no usable system Node exists. Track the external selection across re-entry and stop or exclude it when it forwards back to vp.
Reproduced with the built PR binary: using the forwarding script from setup-foreign-manager.cjs, the system-only invocation timed out after three seconds and the harness killed it. The same PATH with VP_NODE_MANAGER=yes and a cached managed runtime exited 0 and printed MANAGED_FALLBACK_NODE.
| fn find_system_only_tool(tool: &str, cwd: &AbsolutePath) -> Option<AbsolutePathBuf> { | ||
| let inputs = ExternalToolInputs::from_process_env()?; | ||
| vp_command::find_external_tool(tool, &inputs.query(true), cwd) |
There was a problem hiding this comment.
[P2] Keep foreign executables eligible in a shared bin directory
The new system-only lookup calls find_external_tool, which removes the whole configured VP_BIN_DIR before checking executable identity. A supported split layout can place a foreign Node in that shared directory; setup preserves such tools, and the existing split_layout_preserves_foreign_tools fixture covers them. If that is the only system Node on PATH, a shim that needs Node now reports that no system Node exists. Keep foreign executables eligible while rejecting vp-owned shims and managed installations by identity.
Reproduced with the built PR binary: a foreign Node in VP_BIN_DIR produced exit 1 with VP_NODE_MANAGER=system-only requires a system 'node' on PATH, but none was found. An equivalent executable outside VP_BIN_DIR exited 0.
| // A target without system-only support would download managed tools. | ||
| ensure_system_only_support(&binary_path, new_version).await?; |
There was a problem hiding this comment.
[P2] Preserve retained versions when rejecting an incompatible upgrade
If the requested target already exists as the retained previous version, VP_NODE_MANAGER=system-only vp upgrade <target-version> extracts into that installation before this new compatibility check. Rejecting an unsupported target returns an error to the caller, whose cleanup deletes the entire target directory while leaving .previous-version pointing to it. The refusal therefore destroys the rollback installation; unsetting the variable does not restore it. Probe the downloaded binary in a staging directory before changing a retained installation, and preserve existing directories on rejection. Although generic failed-install cleanup predates this PR, this new policy refusal now triggers that destructive path before dependency installation.
Reproduced with the built PR binary and a local registry: the retained directory existed before the command and disappeared afterward; the command exited 1 with the unsupported-system-only error, while current and .previous-version stayed unchanged. The server received only metadata and payload requests.
liangmiQwQ
left a comment
liangmiQwQ
left a comment
There was a problem hiding this comment.
(🙇 apogolise for late replying)
|
|
||
| /// The variable that restricts `tool` (Node.js or a package-manager family) | ||
| /// to the system, if any. Package binaries have no such variable. | ||
| fn system_only_variable_for_tool(tool: &str) -> Option<&'static str> { |
There was a problem hiding this comment.
I'm worried about if the logic finding in the system-PATH in a Vite+ shim call got re-introduced. PATH-forwarding is hacky and has been proven to cause some tricky problems like #2728.
Is it possible to only allow system-only when the tool is in system-first mode, then directly throw an error for shim calls?
| /// System-only tools add no directories: a resolved system directory may be a | ||
| /// canonical path missing from PATH, and adding it would reorder the user's | ||
| /// PATH. Children reach the same system tools through PATH or the shims, after | ||
| /// [`restrict_child_env`] removes the managed directories they must not run. |
There was a problem hiding this comment.
I'm not familiar with Nix, may I ask why managed directories have to be removed here?
My understanding is that system-only is relatively stable. If it is already set, the only mission is to avoid an incorrect prepend (just skip this function call). And since these directories have never been prepended, there is no need to remove it either.
Summary
Part of #1899 (
system-onlymanagement mode for NixOS). Follows #3143.VP_*_MANAGER=system-onlyrestricts Node.js or a package-manager family to the system installation. Shims then use only tools found on PATH: they never fall back to a managed version, download, or reuse a cached managed install. Every other managed entry point fails with a clear error instead of downloading.system-onlyis intentionally undocumented until the remaining paths are covered (vpcommands, reinstall/upgrade dependency installation, and installation-time setup). Without asystem-onlyvariable, nothing changes:yes/no,config.jsonmodes, and the existing installer probe response behave as before.What's Changed
Parsing and safety net
ManagerPreference::SystemOnlymatches only the exact lowercase value. A family variable (VP_PNPM_MANAGER, ...) takes precedence overVP_PM_MANAGER. At run time,system-onlyoverridesconfig.json. The other values still apply only at installation.DisabledBySystemOnlybefore their cache lookups: runtime download, the Node.js version index, package-manager download and binary lookup, and registry version resolution. The error is not an integrity failure.system-onlyfor now:self_setuptreats it like an unknown value.Shims
vp_command::find_external_toolgets anexclude_managedoption. With it, installs under<DATA>/js_runtimeand<DATA>/package_manager, and the fallback-bin directory, never count as the system tool. Paths are compared after canonicalization, so/nix/store/...tools are accepted.EffectiveMode { Managed, SystemFirst, SystemOnly }combines theconfig.jsonshim mode with the environment.dispatch()intoselect_core_tool, which does not exec and can be tested directly. With system-only Node.js, it ignores session overrides,.node-version-style pins, and a managed runtime inherited from a parent shim. A system-only family ignorespackageManagerpins and makes no registry requests.VP_BYPASSpath, the reuse of a tool selected by a parent shim, global package binaries, andvpxglobal binaries follow the same rules. The npm bundled with a managed runtime is treated as part of that runtime./nix/store/...-nodejs/binmatches no PATH entry, so adding it would reorder the user's PATH. Children reach the same tools through PATH or the shims.VP_NODE_MANAGER=system-only requires a system 'node' on PATH, but none was found.Installers and
vp upgradeA
vpthat predates this change would ignoresystem-onlyand download managed tools. Installers andvp upgradetherefore refuse to hand over to such a binary while anysystem-onlyvariable is set.vpanswersVP_SELF_SETUP_SUPPORT_CHECK=system-onlywith a second line,vite-plus-self-setup-system-only. The response to the existing check (vite-plus-self-setup-v1) is unchanged, because released installers compare it exactly.install.sh,install.ps1, andvp_installerprobe with the new value only when asystem-onlyvariable is set. They fail before the legacy installer path, which would download Node.js itself.vp upgradechecks the extracted binary before installing dependencies, andvp upgrade --rollbackchecks the previous version before switching.vp_installertracks the Node.js choice asNodeManager { Enabled, Disabled, SystemOnly }.system-onlytakes precedence over--no-node-managerand cannot be toggled in the menu. When package managers inherit the Node.js choice,system-onlybecomesno, so only an explicit variable makes them system-only.Current limitations (follow-up PRs)
With
system-onlyset, after this PR:node,npm,pnpm, ...) use system tools.vp install,vp run,vp env *, andvpxpackage downloads fail withDisabledBySystemOnlyinstead of using or downloading managed tools.vp upgradewithsystem-onlyexported fails while installing dependencies, for the same reason.system-onlyat installation is not saved yet.Testing
On the current
main:cargo fmt --all -- --check,cargo check --workspace --locked,cargo clippy -- -D warningsandcargo testfor the affected cratesshim_system_first_fallback/system_only_overrides_managed_shims(an unreachable Node.js mirror and registry, checks that no managed install is created and that the child PATH is unchanged), andcommand_self_setup/command_self_setup_system_only_probeBefore rebasing onto the current
main(the only conflict was an import inshim/dispatch.rs):just check,just lint,just test.github/scripts/test-install-bootstrap.sh: all scenarios, including five new system-only scenariosNot run locally:
.github/scripts/test-install-bootstrap.ps1andtest-install-legacy-remote.ps1(CI; no PowerShell locally)Some existing
command_self_setupand shim snapshot cases fail locally on NixOS both before and after this change (same cases); relying on CI for those.