diff --git a/CHANGELOG.md b/CHANGELOG.md index ba246cb..2525fcf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,8 +15,52 @@ The format is based on [Keep a Changelog], and this project adheres to [Semantic - [ ] Update the `[Unreleased]` compare link to the new tag. - [ ] Create and push a signed `vX.Y.Z` tag from `master`. +### Added + +- `runner doctor` (and `info --json`) now classify PATH-probe hits that + are Volta shims and resolve them to the real provisioned binary via + `volta which`: the `PATH probe` line shows + `npm= -> (volta)`, or + `(volta shim, not provisioned)` when Volta fronts a tool it has no + version of. JSON gains an additive `signals.node.volta_shims` map + (omitted on hosts without Volta; no schema bump). Display only — + execution still spawns the shim, which performs Volta's per-project + version selection. + +### Changed + +- `runner install` now honors the `--pm`/`RUNNER_PM` override: when set, + only that package manager installs (previously the override was + ignored and every detected PM installed — e.g. a project with both + `bun.lock` and `deno.json` always ran `deno install` too, writing an + unwanted `deno.lock`). An override naming a PM that detection did not + find refuses the install with exit code 2. runner.toml + `[pm].node`/`[pm].python` continue to scope script dispatch only. +- Invalid `--pm`/`RUNNER_PM`/`--runner`/`RUNNER_RUNNER` values now produce + a readable error: the message names the source that carried the value, + escapes control characters (no more raw ANSI codes), truncates long + garbage, and — when the value contains line breaks — hints that it + looks like captured command output with the correctly quoted PowerShell + spelling. (An unquoted `$env:RUNNER_PM=deno` executes deno and assigns + its REPL banner to the variable.) + ### Fixed +- `runner doctor` no longer dies when a `RUNNER_*` override variable + holds an unparseable value — the condition it exists to diagnose. The + invalid value is ignored for the report and surfaced as an `env:` + warning (human output and the `warnings` array of `doctor --json`, + additively — no schema bump). Every other command, and an explicit bad + `--pm`/`--runner` flag even on doctor, still fails fast. +- Node version constraints are now evaluated with real range semantics + (via the `semver` crate) instead of a prefix match that treated + `>=22.22.2` as `=22.22.2`. Operators (`>=`, `>`, `<=`, `<`, `=`), + caret/tilde ranges, space-separated AND comparators, `||` unions, + hyphen ranges, and `x` wildcards all match per node-semver rules, so + `engines.node: ">=22.22.2"` no longer warns on Node 22.22.3 or 25.9.0. + Bare versions (`.nvmrc` `20.11`) keep the stricter + prefix-at-segment-boundary behavior; unevaluable inputs (`lts/*`) fall + back to the previous prefix match. - Task dispatch now prepends every existing `node_modules/.bin` between the project directory and the filesystem root (nearest first) to the child's `PATH`, the way `npm run` / `pnpm run` / `bun run` do for diff --git a/src/cmd/doctor.rs b/src/cmd/doctor.rs index c4990cc..6abb689 100644 --- a/src/cmd/doctor.rs +++ b/src/cmd/doctor.rs @@ -34,7 +34,7 @@ pub(crate) fn doctor( json: bool, schema_version: u32, ) -> Result<()> { - let project = Project::build_with_schema(ctx, overrides, schema_version); + let project = Project::build_with_schema(ctx, overrides, schema_version, true); if json { println!("{}", serde_json::to_string_pretty(&project)?); @@ -176,13 +176,11 @@ fn print_human(report: &Value, overrides: &ResolutionOverrides) { ); } if let Some(probe) = node["path_probe"].as_object() { + let shims = node["volta_shims"].as_object(); let parts: Vec = probe .iter() .map(|(bin, path)| { - let val = path - .as_str() - .map_or_else(|| "not found".dimmed().to_string(), ToOwned::to_owned); - format!("{bin}={val}") + format_probe_entry(bin, path.as_str(), shims.and_then(|s| s.get(bin))) }) .collect(); writeln_field(out, "PATH probe", &parts.join(", ")); @@ -218,6 +216,22 @@ fn print_human(report: &Value, overrides: &ResolutionOverrides) { } } +/// Render one `PATH probe` entry. Four cases: +/// `npm=not found` (dimmed), `bun=`, +/// `npm= -> (volta)` for a provisioned Volta shim, and +/// `pnpm= (volta shim, not provisioned)` (dimmed suffix) when +/// Volta fronts the tool but has no version of it. +fn format_probe_entry(bin: &str, path: Option<&str>, shim: Option<&Value>) -> String { + let Some(path) = path else { + return format!("{bin}={}", "not found".dimmed()); + }; + match shim.map(|s| s["resolved"].as_str()) { + Some(Some(real)) => format!("{bin}={path} -> {real} {}", "(volta)".dimmed()), + Some(None) => format!("{bin}={path} {}", "(volta shim, not provisioned)".dimmed()), + None => format!("{bin}={path}"), + } +} + fn print_section(title: &str, fill: F) where F: FnOnce(&mut String), @@ -257,6 +271,55 @@ mod tests { } } + #[test] + fn format_probe_entry_renders_all_four_cases() { + use serde_json::json; + + use super::format_probe_entry; + + // Strip color control codes by asserting on substrings only. + let not_found = format_probe_entry("npm", None, None); + assert!(not_found.starts_with("npm="), "{not_found}"); + assert!(not_found.contains("not found"), "{not_found}"); + + let plain = format_probe_entry("bun", Some(r"C:\bun\bun.EXE"), None); + assert!(plain.contains(r"bun=C:\bun\bun.EXE"), "{plain}"); + assert!(!plain.contains("volta"), "{plain}"); + + let shim = json!({ "resolved": r"C:\Volta\image\npm\11.6.2\npm.cmd" }); + let resolved = format_probe_entry("npm", Some(r"C:\Volta\npm.EXE"), Some(&shim)); + assert!( + resolved.contains(r"npm=C:\Volta\npm.EXE -> C:\Volta\image\npm\11.6.2\npm.cmd"), + "{resolved}" + ); + assert!(resolved.contains("(volta)"), "{resolved}"); + + let phantom = json!({ "resolved": null }); + let unprovisioned = format_probe_entry("pnpm", Some(r"C:\Volta\pnpm.EXE"), Some(&phantom)); + assert!( + unprovisioned.contains("volta shim, not provisioned"), + "{unprovisioned}" + ); + } + + #[test] + fn build_report_omits_volta_shims_when_not_resolving() { + let ctx = context(); + let report = build_report(&ctx, &ResolutionOverrides::default()); + + // `Project::build` passes `resolve_shims = false`; the additive + // field must vanish entirely, keeping the v1/v2 shape untouched. + assert!( + report["signals"]["node"].get("volta_shims").is_none(), + "volta_shims must be omitted when empty: {}", + report["signals"]["node"], + ); + assert!( + report["signals"]["node"].get("path_probe").is_some(), + "path_probe shape must be unchanged", + ); + } + #[test] fn build_report_includes_schema_version() { let ctx = context(); diff --git a/src/cmd/info.rs b/src/cmd/info.rs index 2d75424..546e21d 100644 --- a/src/cmd/info.rs +++ b/src/cmd/info.rs @@ -29,7 +29,8 @@ pub(crate) fn info( schema_version: u32, ) -> Result<()> { if json { - let view = Project::build_with_schema(ctx, overrides, schema_version).into_info_view(); + let view = + Project::build_with_schema(ctx, overrides, schema_version, true).into_info_view(); println!("{}", serde_json::to_string_pretty(&view)?); return Ok(()); } diff --git a/src/cmd/install.rs b/src/cmd/install.rs index 6291f66..ca43e05 100644 --- a/src/cmd/install.rs +++ b/src/cmd/install.rs @@ -9,7 +9,7 @@ use anyhow::{Result, bail}; use colored::Colorize; use crate::chain::mux::{LineSink, StdioSink, prefix_width, render_prefix, spawn_readers}; -use crate::resolver::ResolutionOverrides; +use crate::resolver::{ResolutionOverrides, ResolveError}; use crate::tool; use crate::types::{PackageManager, ProjectContext, TaskRunner, version_matches}; @@ -47,9 +47,13 @@ pub(crate) fn install_pms( bail!("No package manager detected."); } + // Resolved before the GHA group opens so a refused override doesn't + // emit an empty `runner: install` group — same rationale as the + // no-PM bail above. + let pms = select_install_pms(ctx, overrides)?; + // Collapse the whole install (single- or multi-PM) under one - // `runner: install` GitHub Actions group when enabled. Opened after the - // no-PM bail so a failed detection doesn't emit an empty group. + // `runner: install` GitHub Actions group when enabled. let _group = super::task_group(overrides, "install"); if let (Some(nv), Some(cur)) = (&ctx.node_version, &ctx.current_node) @@ -65,20 +69,48 @@ pub(crate) fn install_pms( suggest_version_switch(ctx); } - if ctx.package_managers.len() == 1 { - let pm = ctx.package_managers[0]; - eprintln!("{} {}", "installing with".dimmed(), pm.label().bold()); - let mut cmd = build_install_command(ctx, pm, frozen); - super::configure_command(&mut cmd, &ctx.root); - let status = cmd.status()?; - return Ok(if status.success() { - 0 - } else { - super::exit_code(status) - }); + if let [pm] = pms.as_slice() { + return install_single(ctx, *pm, frozen); } - run_installs_parallel(ctx, frozen) + run_installs_parallel(ctx, &pms, frozen) +} + +/// Which PMs this invocation installs with: the cross-ecosystem +/// `--pm`/`RUNNER_PM` override when present (which must name a detected +/// PM), else every detected PM. +/// +/// `pm_by_ecosystem` (runner.toml `[pm].node`/`[pm].python`) is +/// deliberately NOT consulted: it scopes *script dispatch* to an +/// ecosystem, and filtering a polyglot install by it is ill-defined — +/// `[pm].node = "yarn"` saying anything about whether `cargo fetch` +/// runs would be surprising in both directions. +fn select_install_pms( + ctx: &ProjectContext, + overrides: &ResolutionOverrides, +) -> Result, ResolveError> { + match &overrides.pm { + Some(o) if ctx.package_managers.contains(&o.pm) => Ok(vec![o.pm]), + Some(o) => Err(ResolveError::PmOverrideNotDetected { + pm: o.pm, + origin: o.origin.clone(), + detected: ctx.package_managers.clone(), + }), + None => Ok(ctx.package_managers.clone()), + } +} + +/// Run a single PM's install in the foreground, inheriting stdio. +fn install_single(ctx: &ProjectContext, pm: PackageManager, frozen: bool) -> Result { + eprintln!("{} {}", "installing with".dimmed(), pm.label().bold()); + let mut cmd = build_install_command(ctx, pm, frozen); + super::configure_command(&mut cmd, &ctx.root); + let status = cmd.status()?; + Ok(if status.success() { + 0 + } else { + super::exit_code(status) + }) } /// Run every detected package manager's install in parallel, multiplexing @@ -91,19 +123,23 @@ pub(crate) fn install_pms( /// isn't exposed yet — the v1 `runner install` CLI has no flag for it, /// and the conservative default for a top-level command is "don't tear /// down the user's slow `cargo fetch` because `npm` blew up on a 404." -fn run_installs_parallel(ctx: &ProjectContext, frozen: bool) -> Result { +fn run_installs_parallel( + ctx: &ProjectContext, + pms: &[PackageManager], + frozen: bool, +) -> Result { use std::process::Child; - let names: Vec<&str> = ctx.package_managers.iter().map(|pm| pm.label()).collect(); + let names: Vec<&str> = pms.iter().map(|pm| pm.label()).collect(); let width = prefix_width(&names); let colorize = colored::control::SHOULD_COLORIZE.should_colorize(); let sink: Arc = Arc::new(StdioSink); - let mut children: Vec<(PackageManager, Child)> = Vec::with_capacity(ctx.package_managers.len()); + let mut children: Vec<(PackageManager, Child)> = Vec::with_capacity(pms.len()); let mut reader_handles = Vec::new(); let spawn_outcome: Result<()> = (|| { - for pm in &ctx.package_managers { + for pm in pms { eprintln!("{} {}", "installing with".dimmed(), pm.label().bold()); let mut cmd = build_install_command(ctx, *pm, frozen); super::configure_command(&mut cmd, &ctx.root); @@ -204,3 +240,97 @@ fn suggest_version_switch(ctx: &ProjectContext) { }; eprintln!(" {} {}", "hint:".dimmed(), hint); } + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + use std::path::PathBuf; + + use super::select_install_pms; + use crate::resolver::{OverrideOrigin, PmOverride, ResolutionOverrides, ResolveError}; + use crate::types::{Ecosystem, PackageManager, ProjectContext}; + + fn context(pms: Vec) -> ProjectContext { + ProjectContext { + root: PathBuf::from("/tmp/test"), + package_managers: pms, + task_runners: Vec::new(), + tasks: Vec::new(), + node_version: None, + current_node: None, + is_monorepo: false, + warnings: Vec::new(), + } + } + + fn override_pm(pm: PackageManager, origin: OverrideOrigin) -> ResolutionOverrides { + ResolutionOverrides { + pm: Some(PmOverride { pm, origin }), + ..Default::default() + } + } + + #[test] + fn no_override_installs_with_every_detected_pm() { + let ctx = context(vec![PackageManager::Bun, PackageManager::Deno]); + let pms = select_install_pms(&ctx, &ResolutionOverrides::default()) + .expect("default selection should succeed"); + assert_eq!(pms, vec![PackageManager::Bun, PackageManager::Deno]); + } + + #[test] + fn detected_override_installs_with_it_alone() { + // The dreamcli CI bug: bun + deno detected, RUNNER_PM=bun set — + // deno must not install (and must not write deno.lock). + let ctx = context(vec![PackageManager::Bun, PackageManager::Deno]); + let overrides = override_pm(PackageManager::Bun, OverrideOrigin::EnvVar); + let pms = select_install_pms(&ctx, &overrides).expect("detected override should filter"); + assert_eq!(pms, vec![PackageManager::Bun]); + } + + #[test] + fn undetected_override_errors_with_origin_and_detected_list() { + let ctx = context(vec![PackageManager::Cargo]); + let overrides = override_pm(PackageManager::Npm, OverrideOrigin::EnvVar); + let err = select_install_pms(&ctx, &overrides).expect_err("undetected override must error"); + + assert!(matches!(err, ResolveError::PmOverrideNotDetected { .. })); + let msg = format!("{err}"); + assert!(msg.contains("RUNNER_PM"), "should name the source: {msg}"); + assert!(msg.contains("cargo"), "should list detected PMs: {msg}"); + } + + #[test] + fn undetected_cli_override_names_the_flag() { + let ctx = context(vec![PackageManager::Cargo]); + let overrides = override_pm(PackageManager::Npm, OverrideOrigin::CliFlag); + let err = select_install_pms(&ctx, &overrides).expect_err("undetected override must error"); + + let msg = format!("{err}"); + assert!(msg.contains("--pm"), "should name the flag: {msg}"); + } + + #[test] + fn ecosystem_config_override_does_not_filter_installs() { + // Pins the documented non-goal: `[pm].node` in runner.toml scopes + // script dispatch, not the install set. + let ctx = context(vec![PackageManager::Bun, PackageManager::Cargo]); + let mut pm_by_ecosystem = HashMap::new(); + pm_by_ecosystem.insert( + Ecosystem::Node, + PmOverride { + pm: PackageManager::Pnpm, + origin: OverrideOrigin::ConfigFile { + path: PathBuf::from("/tmp/test/runner.toml"), + }, + }, + ); + let overrides = ResolutionOverrides { + pm_by_ecosystem, + ..Default::default() + }; + + let pms = select_install_pms(&ctx, &overrides).expect("config must not filter installs"); + assert_eq!(pms, vec![PackageManager::Bun, PackageManager::Cargo]); + } +} diff --git a/src/cmd/list.rs b/src/cmd/list.rs index 8a240a2..6e9fce7 100644 --- a/src/cmd/list.rs +++ b/src/cmd/list.rs @@ -46,7 +46,7 @@ pub(crate) fn list( }; if json { - let view = Project::build_with_schema(ctx, overrides, schema_version) + let view = Project::build_with_schema(ctx, overrides, schema_version, false) .into_list_view(parsed_source); println!("{}", serde_json::to_string_pretty(&view)?); return Ok(()); diff --git a/src/lib.rs b/src/lib.rs index 0c98394..c8ae401 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -615,10 +615,62 @@ fn build_overrides( ) } +/// Lenient sibling of [`build_overrides`] used when strict parsing +/// failed and the command is `doctor`: invalid env-sourced override +/// values degrade to [`types::DetectionWarning`]s instead of killing +/// the one command whose job is to report a broken environment. +fn build_overrides_lenient( + cli: &cli::Cli, + loaded_config: Option<&config::LoadedConfig>, +) -> Result<(resolver::ResolutionOverrides, Vec)> { + let (cli_keep_going, cli_kill_on_fail) = match cli.command.as_ref() { + Some(cli::Command::Run { failure, .. } | cli::Command::Install { failure, .. }) => { + (failure.keep_going, failure.kill_on_fail) + } + _ => (false, false), + }; + resolver::ResolutionOverrides::from_cli_and_env_lenient( + cli.global.pm_override.as_deref(), + cli.global.runner_override.as_deref(), + cli.global.fallback.as_deref(), + cli.global.on_mismatch.as_deref(), + resolver::DiagnosticFlags { + no_warnings: cli.global.no_warnings, + explain: cli.global.explain, + }, + cli::ChainFailureFlags { + keep_going: cli_keep_going, + kill_on_fail: cli_kill_on_fail, + }, + loaded_config, + ) +} + +/// Resolve overrides for [`dispatch`]. Strict for every command; +/// `doctor` retries leniently on failure because it must survive the +/// misconfigured environment it exists to diagnose — env garbage +/// degrades to warnings appended to `ctx`, while CLI flag garbage +/// re-raises from the lenient pass and stays fatal. +fn dispatch_overrides( + cli: &cli::Cli, + loaded_config: Option<&config::LoadedConfig>, + ctx: &mut types::ProjectContext, +) -> Result { + match build_overrides(cli, loaded_config) { + Ok(overrides) => Ok(overrides), + Err(_) if matches!(cli.command, Some(cli::Command::Doctor { .. })) => { + let (overrides, env_warnings) = build_overrides_lenient(cli, loaded_config)?; + ctx.warnings.extend(env_warnings); + Ok(overrides) + } + Err(e) => Err(e), + } +} + fn dispatch(cli: cli::Cli, dir: &Path) -> Result { - let ctx = detect::detect(dir); + let mut ctx = detect::detect(dir); let loaded_config = config::load(dir)?; - let overrides = build_overrides(&cli, loaded_config.as_ref())?; + let overrides = dispatch_overrides(&cli, loaded_config.as_ref(), &mut ctx)?; match cli.command { // A project task named `info` always shadows the deprecated @@ -1117,6 +1169,51 @@ mod tests { assert!(format!("{err}").contains("unknown package manager")); } + #[test] + fn install_with_undetected_pm_override_exits_2() { + // A cargo-only project with `--pm npm`: the override can't be + // honored, so install must refuse with a ResolveError (exit 2) + // before spawning anything. + let dir = TempDir::new("runner-install-undetected-pm"); + fs::write( + dir.path().join("Cargo.toml"), + "[package]\nname = \"fixture\"\nversion = \"0.0.0\"\n", + ) + .expect("write Cargo.toml"); + + let err = run_in_dir(["runner", "--pm", "npm", "install"], dir.path()) + .expect_err("undetected --pm should refuse the install"); + + assert_eq!( + exit_code_for_error(&err), + 2, + "ResolveError must map to exit 2" + ); + let msg = format!("{err}"); + assert!(msg.contains("--pm"), "should name the source: {msg}"); + assert!(msg.contains("cargo"), "should list detected PMs: {msg}"); + } + + #[test] + fn install_chain_with_undetected_pm_override_exits_2() { + // Same refusal through the chain path (`runner install `). + let dir = TempDir::new("runner-install-chain-undetected-pm"); + fs::write( + dir.path().join("Cargo.toml"), + "[package]\nname = \"fixture\"\nversion = \"0.0.0\"\n", + ) + .expect("write Cargo.toml"); + + let err = run_in_dir(["runner", "--pm", "npm", "install", "build"], dir.path()) + .expect_err("undetected --pm should refuse the install chain"); + + assert_eq!( + exit_code_for_error(&err), + 2, + "ResolveError must map to exit 2" + ); + } + #[test] fn schema_version_rejects_invalid_for_non_json_commands() { let dir = TempDir::new("runner-schema-invalid-completions"); diff --git a/src/resolver/error.rs b/src/resolver/error.rs index bd36023..035283c 100644 --- a/src/resolver/error.rs +++ b/src/resolver/error.rs @@ -71,6 +71,18 @@ pub(crate) enum ResolveError { /// so the `Display` impl produces a clean one-line message. reason: &'static str, }, + /// A `--pm` / `RUNNER_PM` override names a PM that detection did not + /// find in the project, so `runner install` cannot honor it. Erroring + /// (rather than silently installing with the detected set) keeps the + /// override a contract: what the user pinned is what runs. + PmOverrideNotDetected { + /// The PM the override named. + pm: PackageManager, + /// Where the override came from (flag, env var, config file). + origin: super::types::OverrideOrigin, + /// What detection actually found, for the error message. + detected: Vec, + }, /// Both `keep_going` and `kill_on_fail` were set to true at the same /// source (or once layered across CLI/env/config). The chain executor /// can't honour both, so fail loudly before dispatching anything. @@ -137,6 +149,29 @@ impl fmt::Display for ResolveError { Self::InvalidOverride { value, reason } => { write!(f, "invalid override value {value:?}: {reason}") } + Self::PmOverrideNotDetected { + pm, + origin, + detected, + } => { + let detected = if detected.is_empty() { + "none".to_string() + } else { + detected + .iter() + .map(|pm| pm.label()) + .collect::>() + .join(", ") + }; + write!( + f, + "cannot install with {} {}: not a detected package manager in this project \ + (detected: {detected}). Install {} or drop the override.", + pm.label(), + origin.describe_pm_source(), + pm.label(), + ) + } Self::ConflictingFailurePolicy { source } => write!( f, "`keep_going` and `kill_on_fail` are mutually exclusive but both were set ({source}). \ @@ -153,6 +188,31 @@ impl std::error::Error for ResolveError {} mod tests { use super::*; + #[test] + fn pm_override_not_detected_display_names_source_and_detected() { + let err = ResolveError::PmOverrideNotDetected { + pm: PackageManager::Pnpm, + origin: super::super::types::OverrideOrigin::EnvVar, + detected: vec![PackageManager::Npm, PackageManager::Cargo], + }; + let msg = format!("{err}"); + assert!(msg.contains("pnpm"), "msg: {msg}"); + assert!(msg.contains("RUNNER_PM"), "msg: {msg}"); + assert!(msg.contains("npm, cargo"), "msg: {msg}"); + } + + #[test] + fn pm_override_not_detected_display_handles_empty_detected() { + let err = ResolveError::PmOverrideNotDetected { + pm: PackageManager::Pnpm, + origin: super::super::types::OverrideOrigin::CliFlag, + detected: Vec::new(), + }; + let msg = format!("{err}"); + assert!(msg.contains("detected: none"), "msg: {msg}"); + assert!(msg.contains("--pm"), "msg: {msg}"); + } + #[test] fn conflicting_failure_policy_display_includes_source() { let err = ResolveError::ConflictingFailurePolicy { source: "env vars" }; diff --git a/src/resolver/mod.rs b/src/resolver/mod.rs index 351cfa0..665d699 100644 --- a/src/resolver/mod.rs +++ b/src/resolver/mod.rs @@ -42,6 +42,11 @@ pub(crate) use error::{DevEnginesFailReason, ResolveError}; /// Lets `cmd::doctor` exercise the same PATH walk the resolver uses without /// owning the env-reading logic. pub(crate) use probe::probe_in as probe_path_for_doctor; +/// Re-exported for unit tests that need to construct override state +/// directly (e.g. `cmd::install::tests`); production code receives +/// overrides fully built by [`ResolutionOverrides::from_cli_and_env`]. +#[cfg(test)] +pub(crate) use types::PmOverride; pub(crate) use types::{ DiagnosticFlags, FallbackPolicy, MismatchPolicy, OverrideOrigin, ResolutionOverrides, ResolvedPm, Resolver, @@ -67,7 +72,7 @@ mod tests { use super::{FallbackPolicy, OverrideOrigin, ResolutionOverrides, ResolveError, Resolver}; use crate::config::{LoadedConfig, PmSection, RunnerConfig}; use crate::tool::test_support::TempDir; - use crate::types::{Ecosystem, PackageManager, ProjectContext, TaskRunner}; + use crate::types::{DetectionWarning, Ecosystem, PackageManager, ProjectContext, TaskRunner}; fn context(package_managers: Vec) -> ProjectContext { ProjectContext { @@ -473,6 +478,216 @@ mod tests { assert!(msg.contains("turbo")); } + #[test] + fn unknown_pm_env_value_names_env_source() { + let err = ResolutionOverrides::from_sources(OverrideSources { + pm: SourceValue { + cli: None, + env: Some("zoot"), + }, + ..OverrideSources::default() + }) + .expect_err("unknown PM via env should error"); + + let msg = format!("{err}"); + assert!( + msg.contains("RUNNER_PM"), + "should name the env source: {msg}" + ); + assert!(msg.contains("unknown package manager")); + } + + #[test] + fn unknown_pm_cli_value_names_cli_source() { + let err = ResolutionOverrides::from_sources(OverrideSources { + pm: SourceValue { + cli: Some("zoot"), + env: None, + }, + ..OverrideSources::default() + }) + .expect_err("unknown PM via CLI should error"); + + let msg = format!("{err}"); + assert!(msg.contains("--pm"), "should name the CLI source: {msg}"); + } + + #[test] + fn multiline_env_pm_value_is_sanitized_and_hinted() { + // The PowerShell unquoted-assignment footgun: `$env:RUNNER_PM=deno` + // executes deno and captures its REPL banner (ANSI codes included) + // into the variable. + let banner = "Deno 2.8.2 exit using ctrl+d\n\u{1b}[33mREPL is running\u{1b}[0m"; + let err = ResolutionOverrides::from_sources(OverrideSources { + pm: SourceValue { + cli: None, + env: Some(banner), + }, + ..OverrideSources::default() + }) + .expect_err("captured banner should error"); + + let msg = format!("{err}"); + assert!(!msg.contains('\u{1b}'), "raw ESC byte must not leak: {msg}"); + assert!( + msg.contains("captured command output"), + "should hint at the footgun: {msg}" + ); + assert!( + msg.contains("$env:RUNNER_PM='pnpm'"), + "should show the quoted PowerShell spelling: {msg}" + ); + } + + #[test] + fn oversized_pm_value_is_truncated() { + let huge = "z".repeat(500); + let err = ResolutionOverrides::from_sources(OverrideSources { + pm: SourceValue { + cli: None, + env: Some(&huge), + }, + ..OverrideSources::default() + }) + .expect_err("oversized garbage should error"); + + let msg = format!("{err}"); + assert!(msg.contains('…'), "long values should be truncated: {msg}"); + assert!( + !msg.contains(&"z".repeat(100)), + "the full 500-char value must not be rendered: {msg}" + ); + } + + #[test] + fn unknown_runner_env_value_names_env_source() { + let err = ResolutionOverrides::from_sources(OverrideSources { + runner: SourceValue { + cli: None, + env: Some("zoot"), + }, + ..OverrideSources::default() + }) + .expect_err("unknown runner via env should error"); + + let msg = format!("{err}"); + assert!( + msg.contains("RUNNER_RUNNER"), + "should name the env source: {msg}" + ); + assert!(msg.contains("unknown task runner")); + } + + #[test] + fn lenient_env_pm_garbage_degrades_to_warning() { + let (overrides, warnings) = ResolutionOverrides::from_sources_lenient(OverrideSources { + pm: SourceValue { + cli: None, + env: Some("Deno 2.8.2 exit using ctrl+d\n\u{1b}[33mbanner"), + }, + ..OverrideSources::default() + }) + .expect("lenient pass must absorb env garbage"); + + assert!(overrides.pm.is_none(), "garbage override must be blanked"); + assert_eq!(warnings.len(), 1); + match &warnings[0] { + DetectionWarning::InvalidEnvOverride { var, raw, .. } => { + assert_eq!(*var, "RUNNER_PM"); + assert!(!raw.contains('\u{1b}'), "raw must be sanitized: {raw}"); + } + other => panic!("expected InvalidEnvOverride, got {other:?}"), + } + let detail = warnings[0].detail(); + assert!(detail.contains("ignored"), "detail: {detail}"); + } + + #[test] + fn lenient_cli_garbage_still_errors() { + ResolutionOverrides::from_sources_lenient(OverrideSources { + pm: SourceValue { + cli: Some("zoot"), + env: None, + }, + ..OverrideSources::default() + }) + .expect_err("explicit CLI garbage must stay fatal even leniently"); + } + + #[test] + fn lenient_valid_env_produces_no_warnings() { + let (overrides, warnings) = ResolutionOverrides::from_sources_lenient(OverrideSources { + pm: SourceValue { + cli: None, + env: Some("bun"), + }, + ..OverrideSources::default() + }) + .expect("valid env value should parse"); + + assert!(warnings.is_empty()); + assert_eq!( + overrides.pm.expect("pm should be set").pm, + PackageManager::Bun + ); + } + + #[test] + fn lenient_cli_value_shadows_env_garbage() { + // Strict precedence never parses a CLI-shadowed env value, so the + // lenient pass must not warn about it either. + let (overrides, warnings) = ResolutionOverrides::from_sources_lenient(OverrideSources { + pm: SourceValue { + cli: Some("yarn"), + env: Some("complete garbage\nwith newlines"), + }, + ..OverrideSources::default() + }) + .expect("CLI value should shadow env garbage"); + + assert!( + warnings.is_empty(), + "shadowed env must not warn: {warnings:?}" + ); + assert_eq!( + overrides.pm.expect("pm should be set").pm, + PackageManager::Yarn + ); + } + + #[test] + fn lenient_covers_runner_fallback_and_mismatch_vars() { + let (overrides, warnings) = ResolutionOverrides::from_sources_lenient(OverrideSources { + runner: SourceValue { + cli: None, + env: Some("bogus-runner"), + }, + fallback: SourceValue { + cli: None, + env: Some("bogus-policy"), + }, + on_mismatch: SourceValue { + cli: None, + env: Some("bogus-mismatch"), + }, + ..OverrideSources::default() + }) + .expect("lenient pass must absorb all env-sourced garbage"); + + assert!(overrides.runner.is_none()); + let vars: Vec<&str> = warnings + .iter() + .map(|w| match w { + DetectionWarning::InvalidEnvOverride { var, .. } => *var, + other => panic!("expected InvalidEnvOverride, got {other:?}"), + }) + .collect(); + assert_eq!( + vars, + vec!["RUNNER_RUNNER", "RUNNER_FALLBACK", "RUNNER_ON_MISMATCH"] + ); + } + #[test] fn pm_label_that_names_a_runner_suggests_runner_flag() { let err = ResolutionOverrides::from_sources(OverrideSources { diff --git a/src/resolver/overrides.rs b/src/resolver/overrides.rs index cd5c020..82dbcb5 100644 --- a/src/resolver/overrides.rs +++ b/src/resolver/overrides.rs @@ -8,15 +8,15 @@ use anyhow::{Result, anyhow}; use super::join_labels; use super::policies::{ - is_env_truthy, parse_prefer_runners, resolve_failure_policy, resolve_fallback_policy, - resolve_mismatch_policy, + is_env_truthy, parse_fallback_label, parse_mismatch_label, parse_prefer_runners, + resolve_failure_policy, resolve_fallback_policy, resolve_mismatch_policy, }; use super::types::{ DiagnosticFlags, ExplainSource, OverrideOrigin, OverrideSources, PmOverride, ResolutionOverrides, RunnerOverride, SourceValue, }; use crate::config::{LoadedConfig, parse_node_pm, parse_python_pm}; -use crate::types::{Ecosystem, PackageManager, TaskRunner}; +use crate::types::{DetectionWarning, Ecosystem, PackageManager, TaskRunner}; impl ResolutionOverrides { /// Assemble overrides from CLI flag values (already parsed by clap), @@ -42,49 +42,87 @@ impl ResolutionOverrides { failure: crate::cli::ChainFailureFlags, config: Option<&LoadedConfig>, ) -> Result { - let env_pm = std::env::var("RUNNER_PM").ok(); - let env_runner = std::env::var("RUNNER_RUNNER").ok(); - let env_fallback = std::env::var("RUNNER_FALLBACK").ok(); - let env_on_mismatch = std::env::var("RUNNER_ON_MISMATCH").ok(); - let env_no_warnings = std::env::var("RUNNER_NO_WARNINGS").ok(); - let env_explain = std::env::var("RUNNER_EXPLAIN").ok(); - let env_keep_going = std::env::var("RUNNER_KEEP_GOING").ok(); - let env_kill_on_fail = std::env::var("RUNNER_KILL_ON_FAIL").ok(); - Self::from_sources(OverrideSources { - pm: SourceValue { - cli: cli_pm, - env: env_pm.as_deref(), - }, - runner: SourceValue { - cli: cli_runner, - env: env_runner.as_deref(), - }, - fallback: SourceValue { - cli: cli_fallback, - env: env_fallback.as_deref(), - }, - on_mismatch: SourceValue { - cli: cli_on_mismatch, - env: env_on_mismatch.as_deref(), - }, - no_warnings: ExplainSource { - cli: diagnostics.no_warnings, - env: env_no_warnings.as_deref(), - }, - explain: ExplainSource { - cli: diagnostics.explain, - env: env_explain.as_deref(), - }, - keep_going: ExplainSource { - cli: failure.keep_going, - env: env_keep_going.as_deref(), - }, - kill_on_fail: ExplainSource { - cli: failure.kill_on_fail, - env: env_kill_on_fail.as_deref(), - }, - config, - }) + let env = EnvSnapshot::capture(); + let cli = CliSides { + pm: cli_pm, + runner: cli_runner, + fallback: cli_fallback, + on_mismatch: cli_on_mismatch, + diagnostics, + failure, + }; + Self::from_sources(env.sources(cli, config)) + } + + /// Lenient sibling of [`Self::from_cli_and_env`] for commands that + /// must keep working when the *environment* is misconfigured — + /// `runner doctor` exists to diagnose exactly that, so it can't die + /// on the condition it should report. Invalid env-sourced override + /// values are blanked and returned as + /// [`DetectionWarning::InvalidEnvOverride`]; CLI flag values stay + /// strict (an explicit flag is an explicit failure). + /// + /// # Errors + /// + /// Returns an error for everything the strict path rejects except + /// unparseable env override values: bad CLI values, invalid + /// `runner.toml` fields, conflicting failure-policy toggles. + pub(crate) fn from_cli_and_env_lenient( + cli_pm: Option<&str>, + cli_runner: Option<&str>, + cli_fallback: Option<&str>, + cli_on_mismatch: Option<&str>, + diagnostics: DiagnosticFlags, + failure: crate::cli::ChainFailureFlags, + config: Option<&LoadedConfig>, + ) -> Result<(Self, Vec)> { + let env = EnvSnapshot::capture(); + let cli = CliSides { + pm: cli_pm, + runner: cli_runner, + fallback: cli_fallback, + on_mismatch: cli_on_mismatch, + diagnostics, + failure, + }; + Self::from_sources_lenient(env.sources(cli, config)) + } + + /// Pure-function counterpart of [`Self::from_cli_and_env_lenient`]: + /// pre-validates every env-sourced string field, blanking invalid + /// values into warnings, then delegates to [`Self::from_sources`]. + /// + /// Mirrors [`parse_override`] precedence exactly — an env value + /// shadowed by a CLI value is never parsed by the strict path, so + /// it is not validated (or warned about) here either. + /// + /// # Errors + /// + /// Same as [`Self::from_cli_and_env_lenient`]. + pub(crate) fn from_sources_lenient( + mut sources: OverrideSources<'_>, + ) -> Result<(Self, Vec)> { + let mut warnings = Vec::new(); + lenient_env_field(&mut sources.pm, "RUNNER_PM", &mut warnings, |raw| { + parse_pm_label(raw).map(drop) + }); + lenient_env_field(&mut sources.runner, "RUNNER_RUNNER", &mut warnings, |raw| { + parse_runner_label(raw).map(drop) + }); + lenient_env_field( + &mut sources.fallback, + "RUNNER_FALLBACK", + &mut warnings, + |raw| parse_fallback_label(raw).map(drop), + ); + lenient_env_field( + &mut sources.on_mismatch, + "RUNNER_ON_MISMATCH", + &mut warnings, + |raw| parse_mismatch_label(raw).map(drop), + ); + let overrides = Self::from_sources(sources)?; + Ok((overrides, warnings)) } /// Pure-function constructor that consumes a fully-populated @@ -106,12 +144,14 @@ impl ResolutionOverrides { let pm = parse_override( sources.pm.cli, sources.pm.env, + &PM_SOURCE_NAMES, parse_pm_label, |pm, origin| PmOverride { pm, origin }, )?; let runner = parse_override( sources.runner.cli, sources.runner.env, + &RUNNER_SOURCE_NAMES, parse_runner_label, |runner, origin| RunnerOverride { runner, origin }, )?; @@ -197,7 +237,8 @@ fn parse_pm_label(raw: &str) -> Result { )); } Err(anyhow!( - "unknown package manager {raw:?}; expected one of {}", + "unknown package manager \"{}\"; expected one of {}", + sanitize_raw_label(raw), join_labels( PackageManager::all() .iter() @@ -219,20 +260,257 @@ fn parse_runner_label(raw: &str) -> Result { )); } Err(anyhow!( - "unknown task runner {raw:?}; expected one of {}", + "unknown task runner \"{}\"; expected one of {}", + sanitize_raw_label(raw), join_labels(TaskRunner::all().iter().copied().map(TaskRunner::label)), )) } +/// Maximum characters of a raw override value rendered in an error. +const MAX_RAW_DISPLAY: usize = 60; + +/// Render an untrusted override value safely for a one-line error: +/// control characters (ANSI escapes, newlines) are escaped via +/// [`char::escape_debug`], then the escaped string is truncated to +/// [`MAX_RAW_DISPLAY`] characters with an ellipsis. Values come straight +/// from the environment and can be arbitrary captured command output — +/// an unquoted PowerShell `$env:RUNNER_PM=deno` assigns deno's entire +/// REPL banner, ANSI codes and all. +fn sanitize_raw_label(raw: &str) -> String { + let escaped: String = raw.chars().flat_map(char::escape_debug).collect(); + let mut chars = escaped.chars(); + let truncated: String = chars.by_ref().take(MAX_RAW_DISPLAY).collect(); + if chars.next().is_some() { + format!("{truncated}…") + } else { + truncated + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn lenient_policy_env_garbage_does_not_leak_full_raw_value() { + let token_prefix = "ghp_"; + let fake_token = format!( + "{token_prefix}{}DO_NOT_LEAK_ME", + "A".repeat(MAX_RAW_DISPLAY.saturating_sub(token_prefix.len())) + ); + let huge = fake_token.repeat(6); + let (_overrides, warnings) = ResolutionOverrides::from_sources_lenient(OverrideSources { + fallback: SourceValue { + cli: None, + env: Some(&huge), + }, + ..OverrideSources::default() + }) + .expect("lenient pass must absorb fallback env garbage"); + + assert_eq!(warnings.len(), 1); + let detail = warnings[0].detail(); + assert!( + detail.contains('…'), + "long invalid env value should be truncated in warning detail" + ); + assert!( + !detail.contains("DO_NOT_LEAK_ME"), + "secret-looking env tail must not leak in warning detail" + ); + } +} + +/// The CLI-flag half of an override assembly, bundled so +/// [`EnvSnapshot::sources`] pairs one CLI side with one env snapshot +/// instead of threading seven loose parameters. +#[derive(Clone, Copy)] +struct CliSides<'a> { + pm: Option<&'a str>, + runner: Option<&'a str>, + fallback: Option<&'a str>, + on_mismatch: Option<&'a str>, + diagnostics: DiagnosticFlags, + failure: crate::cli::ChainFailureFlags, +} + +/// Captured `RUNNER_*` environment, separated from [`OverrideSources`] +/// assembly so the strict and lenient constructors share one read path +/// and can never drift on which variables they consult. +struct EnvSnapshot { + pm: Option, + runner: Option, + fallback: Option, + on_mismatch: Option, + no_warnings: Option, + explain: Option, + keep_going: Option, + kill_on_fail: Option, +} + +impl EnvSnapshot { + /// Read every `RUNNER_*` override variable from the process + /// environment. + fn capture() -> Self { + Self { + pm: std::env::var("RUNNER_PM").ok(), + runner: std::env::var("RUNNER_RUNNER").ok(), + fallback: std::env::var("RUNNER_FALLBACK").ok(), + on_mismatch: std::env::var("RUNNER_ON_MISMATCH").ok(), + no_warnings: std::env::var("RUNNER_NO_WARNINGS").ok(), + explain: std::env::var("RUNNER_EXPLAIN").ok(), + keep_going: std::env::var("RUNNER_KEEP_GOING").ok(), + kill_on_fail: std::env::var("RUNNER_KILL_ON_FAIL").ok(), + } + } + + /// Pair the captured environment with the CLI flag values into the + /// [`OverrideSources`] consumed by the constructors. + fn sources<'a>( + &'a self, + cli: CliSides<'a>, + config: Option<&'a LoadedConfig>, + ) -> OverrideSources<'a> { + OverrideSources { + pm: SourceValue { + cli: cli.pm, + env: self.pm.as_deref(), + }, + runner: SourceValue { + cli: cli.runner, + env: self.runner.as_deref(), + }, + fallback: SourceValue { + cli: cli.fallback, + env: self.fallback.as_deref(), + }, + on_mismatch: SourceValue { + cli: cli.on_mismatch, + env: self.on_mismatch.as_deref(), + }, + no_warnings: ExplainSource { + cli: cli.diagnostics.no_warnings, + env: self.no_warnings.as_deref(), + }, + explain: ExplainSource { + cli: cli.diagnostics.explain, + env: self.explain.as_deref(), + }, + keep_going: ExplainSource { + cli: cli.failure.keep_going, + env: self.keep_going.as_deref(), + }, + kill_on_fail: ExplainSource { + cli: cli.failure.kill_on_fail, + env: self.kill_on_fail.as_deref(), + }, + config, + } + } +} + +/// Pre-validate one env-sourced override field for the lenient +/// constructor. The env side is only consulted (and therefore only +/// validated) when the CLI side is unset or whitespace-only — exactly +/// the precedence [`parse_override`] applies — so CLI-shadowed env +/// garbage stays invisible, same as the strict path. An invalid env +/// value is blanked from `field` and reported as a warning carrying +/// the sanitized value and the bare parse error. +fn lenient_env_field( + field: &mut SourceValue<'_>, + var: &'static str, + warnings: &mut Vec, + validate: impl Fn(&str) -> Result<()>, +) { + if field.cli.map(str::trim).is_some_and(|s| !s.is_empty()) { + return; + } + let Some(raw) = field.env.map(str::trim).filter(|s| !s.is_empty()) else { + return; + }; + if let Err(err) = validate(raw) { + let sanitized = sanitize_raw_label(raw); + warnings.push(DetectionWarning::InvalidEnvOverride { + var, + raw: sanitized.clone(), + message: sanitize_error_message(raw, &sanitized, &format!("{err}")), + }); + field.env = None; + } +} + +fn sanitize_error_message(raw: &str, sanitized: &str, message: &str) -> String { + let escaped: String = raw.chars().flat_map(char::escape_debug).collect(); + message.replace(raw, sanitized).replace(&escaped, sanitized) +} + +/// Source names for the cross-ecosystem PM override. +const PM_SOURCE_NAMES: SourceNames = SourceNames { + cli: "--pm", + env: "RUNNER_PM", + example: "pnpm", +}; + +/// Source names for the task-runner override. +const RUNNER_SOURCE_NAMES: SourceNames = SourceNames { + cli: "--runner", + env: "RUNNER_RUNNER", + example: "just", +}; + +/// The user-facing names of one override's sources, used to attribute +/// parse errors to the flag or variable that carried the bad value. +struct SourceNames { + /// CLI flag, e.g. `--pm`. + cli: &'static str, + /// Environment variable, e.g. `RUNNER_PM`. + env: &'static str, + /// A valid example value, e.g. `pnpm`. + example: &'static str, +} + +impl SourceNames { + /// Prefix `err` with the source that supplied `raw`. When the value + /// contains line breaks it is almost certainly captured command + /// output rather than a name the user typed (the PowerShell + /// unquoted-assignment footgun), so append a hint showing the + /// correct spelling for that source. + fn decorate(&self, err: &anyhow::Error, raw: &str, origin: &OverrideOrigin) -> anyhow::Error { + let from_env = matches!(origin, OverrideOrigin::EnvVar); + let source = if from_env { self.env } else { self.cli }; + let hint = if raw.contains('\n') || raw.contains('\r') { + let example = if from_env { + format!( + "$env:{}='{}' (quote the value in PowerShell)", + self.env, self.example + ) + } else { + format!("{} {}", self.cli, self.example) + }; + format!( + "\n hint: the value contains line breaks and looks like captured command \ + output; pass a plain name instead, e.g. {example}" + ) + } else { + String::new() + }; + anyhow!("{source}: {err}{hint}") + } +} + /// Generic CLI-then-env override parser. CLI wins; whitespace is /// trimmed from both sources before parsing so `RUNNER_PM=" pnpm "` /// works the same as `RUNNER_PM=pnpm`. Empty/whitespace-only values /// are treated as unset so a user can clear an inherited variable with /// `RUNNER_PM= runner …`. Matches the whitespace handling used by /// [`super::policies::is_env_truthy`] for boolean env flags. +/// +/// Parse failures are attributed to the source that carried the value +/// (`names.cli` or `names.env`) via [`SourceNames::decorate`]. fn parse_override( cli: Option<&str>, env: Option<&str>, + names: &SourceNames, parse: V, build: B, ) -> Result> @@ -241,11 +519,13 @@ where B: Fn(P, OverrideOrigin) -> T, { if let Some(raw) = cli.map(str::trim).filter(|s| !s.is_empty()) { - let parsed = parse(raw)?; + let parsed = + parse(raw).map_err(|err| names.decorate(&err, raw, &OverrideOrigin::CliFlag))?; return Ok(Some(build(parsed, OverrideOrigin::CliFlag))); } if let Some(raw) = env.map(str::trim).filter(|s| !s.is_empty()) { - let parsed = parse(raw)?; + let parsed = + parse(raw).map_err(|err| names.decorate(&err, raw, &OverrideOrigin::EnvVar))?; return Ok(Some(build(parsed, OverrideOrigin::EnvVar))); } Ok(None) diff --git a/src/resolver/policies.rs b/src/resolver/policies.rs index ba99b05..ff98582 100644 --- a/src/resolver/policies.rs +++ b/src/resolver/policies.rs @@ -30,7 +30,7 @@ pub(super) fn is_env_truthy(raw: &str) -> bool { && !v.eq_ignore_ascii_case("off") } -fn parse_fallback_label(raw: &str) -> Result { +pub(super) fn parse_fallback_label(raw: &str) -> Result { match raw { "probe" => Ok(FallbackPolicy::Probe), "npm" => Ok(FallbackPolicy::Npm), diff --git a/src/resolver/types.rs b/src/resolver/types.rs index 9e42de1..81d47b6 100644 --- a/src/resolver/types.rs +++ b/src/resolver/types.rs @@ -164,6 +164,21 @@ pub(crate) enum OverrideOrigin { }, } +impl OverrideOrigin { + /// Render the "via …" provenance fragment for a PM override from + /// this origin: `via --pm (CLI override)`, `via RUNNER_PM + /// (environment)`, or `via runner.toml at `. Shared by + /// [`ResolvedPm::describe`] and install's override errors so the + /// attribution wording stays identical everywhere. + pub(crate) fn describe_pm_source(&self) -> String { + match self { + Self::CliFlag => "via --pm (CLI override)".to_string(), + Self::EnvVar => "via RUNNER_PM (environment)".to_string(), + Self::ConfigFile { path } => format!("via runner.toml at {}", path.display()), + } + } +} + /// A package-manager decision plus the chain step that produced it. #[derive(Debug, Clone)] pub(crate) struct ResolvedPm { @@ -291,14 +306,8 @@ impl ResolvedPm { /// decision. Used by `--explain` to attribute the PM choice. pub(crate) fn describe(&self) -> String { match &self.via { - ResolutionStep::Override(OverrideOrigin::CliFlag) => { - format!("{} via --pm (CLI override)", self.pm.label()) - } - ResolutionStep::Override(OverrideOrigin::EnvVar) => { - format!("{} via RUNNER_PM (environment)", self.pm.label()) - } - ResolutionStep::Override(OverrideOrigin::ConfigFile { path }) => { - format!("{} via runner.toml at {}", self.pm.label(), path.display()) + ResolutionStep::Override(origin) => { + format!("{} {}", self.pm.label(), origin.describe_pm_source()) } ResolutionStep::ManifestPackageManager => { format!("{} via package.json \"packageManager\"", self.pm.label()) diff --git a/src/schema/project.rs b/src/schema/project.rs index 0fdd3d7..61956d8 100644 --- a/src/schema/project.rs +++ b/src/schema/project.rs @@ -62,7 +62,9 @@ impl<'a> Project<'a> { /// passes a concrete version to [`Self::build_with_schema`]. #[cfg(test)] pub(crate) fn build(ctx: &'a ProjectContext, overrides: &ResolutionOverrides) -> Self { - Self::build_with_schema(ctx, overrides, super::CURRENT_VERSION) + // `resolve_shims = false` keeps unit tests hermetic — no `volta + // which` spawns against the test host. + Self::build_with_schema(ctx, overrides, super::CURRENT_VERSION, false) } /// Build the report against a specific schema version. `schema_version` @@ -72,10 +74,16 @@ impl<'a> Project<'a> { /// Per-field versioning: source labels route through /// [`super::labels::source_label_for`]. PM and `TaskRunner` labels /// are unchanged across versions. + /// + /// `resolve_shims` controls whether PATH-probe hits are classified + /// against a Volta installation (one `volta which` spawn per + /// shimmed tool). Diagnostic surfaces (`doctor`, `info --json`) + /// pass `true`; `list` passes `false` — it drops signals anyway. pub(crate) fn build_with_schema( ctx: &'a ProjectContext, overrides: &ResolutionOverrides, schema_version: u32, + resolve_shims: bool, ) -> Self { let manifest_decl = detect_pm_from_manifest(&ctx.root); let manifest_pm = manifest_decl.as_ref().map(|d| ManifestPm { @@ -109,6 +117,8 @@ impl<'a> Project<'a> { }) .collect(); + let probes = probe_signals(&ctx.root, resolve_shims); + Self { schema_version, root: ctx.root.display().to_string(), @@ -123,7 +133,8 @@ impl<'a> Project<'a> { node: NodeSignals { lockfile_pm: ctx.primary_node_pm().map(PackageManager::label), manifest_pm, - path_probe: path_probe_map(), + path_probe: probes.path_probe, + volta_shims: probes.volta_shims, }, }, decisions, @@ -306,6 +317,20 @@ pub(crate) struct NodeSignals { pub manifest_pm: Option, /// `bun`/`pnpm`/`yarn`/`npm` -> absolute path on `$PATH` (or null). pub path_probe: BTreeMap<&'static str, Option>, + /// PATH-probe hits identified as Volta shims, keyed like + /// [`Self::path_probe`]. Additive field (no schema bump): absent on + /// hosts without Volta and on surfaces that skip shim resolution. + #[serde(skip_serializing_if = "BTreeMap::is_empty")] + pub volta_shims: BTreeMap<&'static str, VoltaShimInfo>, +} + +/// What `volta which` said about one shimmed tool. +#[derive(Debug, Serialize)] +pub(crate) struct VoltaShimInfo { + /// Real provisioned binary behind the shim; `null` when Volta has + /// no version of the tool ("not provisioned"). Shims Volta could + /// not classify at all are omitted from the map instead of guessed. + pub resolved: Option, } /// Manifest-level PM declaration plus the field it came from. @@ -458,13 +483,30 @@ const PATH_PROBE_PMS: [PackageManager; 4] = [ PackageManager::Npm, ]; -fn path_probe_map() -> BTreeMap<&'static str, Option> { +/// Probe results for the signals section: every PATH hit, plus Volta +/// shim classification when requested. +struct ProbeSignals { + path_probe: BTreeMap<&'static str, Option>, + volta_shims: BTreeMap<&'static str, VoltaShimInfo>, +} + +fn probe_signals(root: &std::path::Path, resolve_shims: bool) -> ProbeSignals { use std::env; use std::thread; + use crate::tool::volta::{ShimResolution, VoltaInstall}; + let path = env::var_os("PATH").unwrap_or_default(); let pathext = env::var_os("PATHEXT"); let pathext_ref = pathext.as_deref(); + // Located once, shared by every probe thread. `None` either means + // "no Volta on this host" or "shim resolution not requested" — + // both collapse to "classify nothing". + let volta = if resolve_shims { + VoltaInstall::locate() + } else { + None + }; thread::scope(|s| { // Spawn all probes first (push, don't lazy-iterate) so they @@ -475,20 +517,46 @@ fn path_probe_map() -> BTreeMap<&'static str, Option> { let mut handles = Vec::with_capacity(PATH_PROBE_PMS.len()); for pm in &PATH_PROBE_PMS { let path = &path; + let volta = volta.as_ref(); handles.push(s.spawn(move || { let resolved = - crate::resolver::probe_path_for_doctor(pm.label(), path, pathext_ref) - .map(|p| p.display().to_string()); - (pm.label(), resolved) + crate::resolver::probe_path_for_doctor(pm.label(), path, pathext_ref); + // The `volta which` spawn rides the same per-PM thread + // as the probe, so shim resolution adds one process + // wait of wall time, not four. + let shim = resolved + .as_deref() + .filter(|hit| volta.is_some_and(|v| v.is_shim(hit))) + .map(|_| crate::tool::volta::resolve_shim(pm.label(), root)); + (pm.label(), resolved.map(|p| p.display().to_string()), shim) })); } - let mut map = BTreeMap::new(); + let mut path_probe = BTreeMap::new(); + let mut volta_shims = BTreeMap::new(); for handle in handles { - let (label, resolved) = handle.join().expect("path probe thread panicked"); - map.insert(label, resolved); + let (label, resolved, shim) = handle.join().expect("path probe thread panicked"); + path_probe.insert(label, resolved); + match shim { + Some(ShimResolution::Resolved(real)) => { + volta_shims.insert( + label, + VoltaShimInfo { + resolved: Some(real.display().to_string()), + }, + ); + } + Some(ShimResolution::NotProvisioned) => { + volta_shims.insert(label, VoltaShimInfo { resolved: None }); + } + // Unknown: volta failed to answer — claim nothing. + Some(ShimResolution::Unknown) | None => {} + } + } + ProbeSignals { + path_probe, + volta_shims, } - map }) } @@ -593,12 +661,12 @@ mod tests { warnings: Vec::new(), }; - let v1 = Project::build_with_schema(&ctx, &ResolutionOverrides::default(), 1); + let v1 = Project::build_with_schema(&ctx, &ResolutionOverrides::default(), 1, false); let v1_json = serde_json::to_value(&v1).expect("v1 serialization"); assert_eq!(v1_json["schema_version"], 1); assert_eq!(v1_json["tasks"][0]["source"], "justfile"); - let v2 = Project::build_with_schema(&ctx, &ResolutionOverrides::default(), 2); + let v2 = Project::build_with_schema(&ctx, &ResolutionOverrides::default(), 2, false); let v2_json = serde_json::to_value(&v2).expect("v2 serialization"); assert_eq!(v2_json["schema_version"], 2); assert_eq!(v2_json["tasks"][0]["source"], "just"); diff --git a/src/tool/mod.rs b/src/tool/mod.rs index 2ebc857..0c34149 100644 --- a/src/tool/mod.rs +++ b/src/tool/mod.rs @@ -60,6 +60,8 @@ pub(crate) mod python; pub(crate) mod turbo; /// uv, a fast Python package manager (`uv.lock`). pub(crate) mod uv; +/// Volta toolchain manager — shim classification and `volta which` resolution. +pub(crate) mod volta; /// Yarn, a Node.js package manager (`yarn.lock`). pub(crate) mod yarn; diff --git a/src/tool/volta.rs b/src/tool/volta.rs new file mode 100644 index 0000000..51ce9be --- /dev/null +++ b/src/tool/volta.rs @@ -0,0 +1,238 @@ +//! Volta toolchain manager — shim classification and `volta which` +//! resolution. +//! +//! Volta interposes shims for `node`/`npm`/`yarn`/`pnpm` on `PATH` +//! (Windows: next to `volta.exe`, e.g. `C:\Program Files\Volta\`; +//! Unix: `~/.volta/bin`). A shim exists even when the tool it fronts +//! was never provisioned, so a raw PATH probe can report a binary that +//! cannot actually run. This module classifies probe hits as shims and +//! resolves them to the real provisioned binary via `volta which`. +//! +//! Display/diagnostics only: execution always spawns the shim itself, +//! because the shim performs Volta's per-project version selection. + +use std::path::{Path, PathBuf}; + +use crate::tool::program; + +/// A located Volta installation reduced to its shim directories. +#[derive(Debug, Clone)] +pub(crate) struct VoltaInstall { + /// Canonicalized directories whose executables are Volta shims. + shim_dirs: Vec, +} + +impl VoltaInstall { + /// Locate Volta from the live environment: the directory holding + /// the `volta` binary on `PATH`, plus `$VOLTA_HOME/bin` when set. + /// Returns `None` when neither signal exists — no Volta, nothing + /// to classify. + pub(crate) fn locate() -> Option { + let volta_bin = std::env::var_os("PATH").and_then(|path| { + crate::resolver::probe_path_for_doctor( + "volta", + &path, + std::env::var_os("PATHEXT").as_deref(), + ) + }); + let volta_home = std::env::var_os("VOLTA_HOME").map(PathBuf::from); + Self::from_candidates(volta_bin.as_deref(), volta_home.as_deref()) + } + + /// Pure constructor for tests — injected candidates, no env reads. + pub(crate) fn from_candidates( + volta_bin: Option<&Path>, + volta_home: Option<&Path>, + ) -> Option { + let mut shim_dirs = Vec::new(); + if let Some(parent) = volta_bin.and_then(Path::parent) { + shim_dirs.push(canonical_dir(parent)); + } + if let Some(home) = volta_home { + shim_dirs.push(canonical_dir(&home.join("bin"))); + } + shim_dirs.dedup(); + if shim_dirs.is_empty() { + None + } else { + Some(Self { shim_dirs }) + } + } + + /// True when `bin` lives directly in one of the shim directories. + /// Exact parent-directory equality, not prefix matching — + /// `/nested/npm` is not a shim. Only the *parent* is + /// canonicalized: on Unix the shims themselves are symlinks to + /// `volta-shim`, and canonicalizing the file would escape the bin + /// directory entirely. + pub(crate) fn is_shim(&self, bin: &Path) -> bool { + let Some(parent) = bin.parent() else { + return false; + }; + let canonical = canonical_dir(parent); + self.shim_dirs.contains(&canonical) + } +} + +/// Canonicalize a directory for comparison (resolves `\\?\` prefixes +/// and on-disk casing on Windows); fall back to the lexical path when +/// canonicalization fails so comparison degrades instead of erroring. +fn canonical_dir(dir: &Path) -> PathBuf { + dir.canonicalize().unwrap_or_else(|_| dir.to_path_buf()) +} + +/// Outcome of resolving one shim through `volta which`. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum ShimResolution { + /// The tool is provisioned; this is the real binary the shim runs. + Resolved(PathBuf), + /// Volta answered but has no version of the tool ("No default …"). + NotProvisioned, + /// Volta itself failed to answer (spawn error, empty output) — + /// claim nothing. + Unknown, +} + +/// Ask `volta which ` for the real provisioned binary. +/// +/// Runs with `project_root` as the working directory because `volta +/// which` honors the project pinning of its CWD. Classification uses +/// exit status and stdout only — Volta's error wording ("No default +/// npm version set") varies across versions and must not be parsed. +pub(crate) fn resolve_shim(tool: &str, project_root: &Path) -> ShimResolution { + match program::command("volta") + .args(["which", tool]) + .current_dir(project_root) + .output() + { + Ok(out) => classify_which_output(out.status.success(), &out.stdout), + Err(_) => ShimResolution::Unknown, + } +} + +/// Pure decoder for `volta which` output, split out so tests don't +/// need a Volta installation. +fn classify_which_output(success: bool, stdout: &[u8]) -> ShimResolution { + if !success { + return ShimResolution::NotProvisioned; + } + let text = String::from_utf8_lossy(stdout); + let trimmed = text.trim(); + if trimmed.is_empty() { + ShimResolution::Unknown + } else { + ShimResolution::Resolved(PathBuf::from(trimmed)) + } +} + +#[cfg(test)] +mod tests { + use std::fs; + use std::path::{Path, PathBuf}; + + use super::{ShimResolution, VoltaInstall, classify_which_output}; + use crate::tool::test_support::TempDir; + + #[test] + fn from_candidates_uses_parent_of_volta_bin() { + let dir = TempDir::new("volta-bin-parent"); + let volta = dir.path().join("volta.exe"); + let npm = dir.path().join("npm.exe"); + fs::write(&volta, "").expect("write volta stub"); + fs::write(&npm, "").expect("write npm stub"); + + let install = + VoltaInstall::from_candidates(Some(&volta), None).expect("volta bin is evidence"); + assert!( + install.is_shim(&npm), + "sibling of volta must classify as shim" + ); + assert!( + !install.is_shim(Path::new("/somewhere/else/npm")), + "unrelated dirs must not classify" + ); + } + + #[test] + fn from_candidates_adds_volta_home_bin() { + let home = TempDir::new("volta-home"); + let bin = home.path().join("bin"); + fs::create_dir_all(&bin).expect("create bin dir"); + let yarn = bin.join("yarn"); + fs::write(&yarn, "").expect("write yarn stub"); + + let install = + VoltaInstall::from_candidates(None, Some(home.path())).expect("VOLTA_HOME is evidence"); + assert!(install.is_shim(&yarn)); + } + + #[test] + fn from_candidates_none_without_evidence() { + assert!(VoltaInstall::from_candidates(None, None).is_none()); + } + + #[test] + fn is_shim_requires_exact_parent_dir() { + let dir = TempDir::new("volta-exact-parent"); + let volta = dir.path().join("volta"); + fs::write(&volta, "").expect("write volta stub"); + let nested_dir = dir.path().join("nested"); + fs::create_dir_all(&nested_dir).expect("create nested dir"); + let nested = nested_dir.join("npm"); + fs::write(&nested, "").expect("write nested stub"); + + let install = + VoltaInstall::from_candidates(Some(&volta), None).expect("volta bin is evidence"); + assert!(!install.is_shim(&nested), "no prefix matching: {nested:?}"); + } + + #[test] + fn classify_which_output_resolves_trimmed_path() { + let resolved = classify_which_output(true, b"C:\\Volta\\image\\npm\\11.6.2\\npm.cmd\r\n"); + assert_eq!( + resolved, + ShimResolution::Resolved(PathBuf::from("C:\\Volta\\image\\npm\\11.6.2\\npm.cmd")), + ); + } + + #[test] + fn classify_which_output_nonzero_is_not_provisioned() { + assert_eq!( + classify_which_output(false, b""), + ShimResolution::NotProvisioned, + ); + } + + #[test] + fn classify_which_output_empty_stdout_is_unknown() { + assert_eq!( + classify_which_output(true, b" \n"), + ShimResolution::Unknown + ); + } + + /// Availability-gated smoke test: only meaningful on a host with + /// Volta installed; skips (with a note) elsewhere, mirroring the + /// `just`-gated integration tests. + #[test] + fn volta_which_smoke() { + let Some(path) = std::env::var_os("PATH") else { + eprintln!("skipping: no PATH"); + return; + }; + if crate::resolver::probe_path_for_doctor( + "volta", + &path, + std::env::var_os("PATHEXT").as_deref(), + ) + .is_none() + { + eprintln!("skipping: `volta` not found on PATH"); + return; + } + let cwd = std::env::current_dir().expect("cwd exists"); + // Any variant is acceptable; the assertion is "does not panic + // and answers something classifiable". + let _ = super::resolve_shim("node", &cwd); + } +} diff --git a/src/types.rs b/src/types.rs index 113ae49..ee05ca2 100644 --- a/src/types.rs +++ b/src/types.rs @@ -236,6 +236,20 @@ pub(crate) enum DetectionWarning { /// the user can spot their typo without re-reading the file. raw: String, }, + /// An env-var override (`RUNNER_PM`, `RUNNER_RUNNER`) held a value + /// that doesn't parse, and the command chose to report it instead + /// of dying — `runner doctor` must be able to diagnose the broken + /// environment it exists to diagnose. Strict commands still treat + /// the same condition as a fatal error. + InvalidEnvOverride { + /// The variable that carried the value (`"RUNNER_PM"`). + var: &'static str, + /// The offending value, pre-sanitized for display (control + /// chars escaped, truncated). + raw: String, + /// Rendered parse error, already source-prefixed. + message: String, + }, } impl DetectionWarning { @@ -252,6 +266,7 @@ impl DetectionWarning { | Self::UnparseablePackageManager { .. } => "package.json", Self::PathProbeFallback { .. } | Self::LegacyNpmFallbackUsed { .. } => "resolver", Self::TaskListUnreadable { source, .. } => source, + Self::InvalidEnvOverride { .. } => "env", } } @@ -317,6 +332,9 @@ impl DetectionWarning { (expected one of npm|pnpm|yarn|bun|deno, optionally followed by @); \ declaration ignored, falling back to lockfile / PATH probe", ), + Self::InvalidEnvOverride { var, message, .. } => { + format!("{var} is set but invalid and was ignored for this report: {message}") + } } } } @@ -643,35 +661,56 @@ impl TaskSource { } } -/// Loose semver prefix match. +/// Does `current` satisfy the `expected` version constraint? /// -/// Strips leading range operators (`>=`, `~`, `^`, etc.) and checks whether -/// `current` starts with the cleaned `expected` value. A bare major version -/// like `"20"` matches `"20.x.y"`. +/// `expected` accepts the node-semver range grammar found in `.nvmrc`, +/// `.node-version`, `.tool-versions`, and `package.json` `engines.node`: +/// comparator sets (`>=22.22.2`, `>=18 <21`), caret/tilde ranges +/// (`^20.11`, `~18.15`), `||` unions, hyphen ranges (`18 - 20`), and +/// wildcards (`20.x`). Evaluation is delegated to the `semver` crate +/// after normalizing node's grammar into the comma-separated comparator +/// form it parses. /// -/// This intentionally ignores operator semantics — use the `semver` crate -/// for precise constraint evaluation. +/// Bare versions (`"20"`, `"20.11"`) keep prefix-at-segment-boundary +/// semantics — a `.nvmrc` saying `20.11` means "any 20.11.x", which is +/// narrower than the caret default the `semver` crate would apply. +/// +/// Anything unevaluable (`lts/*`, malformed ranges, a non-version +/// `current`) falls back to the historical prefix match, so this never +/// panics and never rejects input it used to accept. +/// +/// A prerelease `current` (e.g. `23.0.0-nightly`) only matches a +/// comparator that pins the same triple with a prerelease tag — the +/// `semver` crate's gate, mirroring node-semver's default behavior. pub(crate) fn version_matches(expected: &str, current: &str) -> bool { let expected = expected.trim(); let current = current.trim(); - let expected_clean = expected + if bare_version(expected) { + return prefix_version_matches(expected, current); + } + range_matches(expected, current).unwrap_or_else(|| prefix_version_matches(expected, current)) +} + +/// The historical loose prefix match, kept as the fallback for inputs +/// the range path can't evaluate and as the primary semantics for bare +/// versions. +/// +/// Strips leading range operators (`>=`, `~`, `^`, etc.) and checks +/// whether `current` starts with the cleaned `expected` value at a +/// segment boundary. A bare major version like `"20"` matches `"20.x.y"`. +fn prefix_version_matches(expected: &str, current: &str) -> bool { + let after_ops = expected + .trim() .trim_start_matches(">=") .trim_start_matches("<=") .trim_start_matches('>') .trim_start_matches('<') + .trim_start_matches('=') .trim_start_matches('~') .trim_start_matches('^') - .trim_start_matches('v') - .trim(); - - if !expected_clean.contains('.') { - return current.starts_with(expected_clean) - && current[expected_clean.len()..] - .chars() - .next() - .is_none_or(|c| c == '.'); - } + .trim_start(); + let expected_clean = strip_v(after_ops).trim(); current.starts_with(expected_clean) && current[expected_clean.len()..] @@ -680,6 +719,117 @@ pub(crate) fn version_matches(expected: &str, current: &str) -> bool { .is_none_or(|c| c == '.') } +/// True when `s` is a plain version literal — optionally `v`-prefixed, +/// then nothing but ASCII digits and dots (`20`, `20.11`, `v20.11.0`). +/// Operators, wildcards, and named aliases (`lts/*`) all return false. +fn bare_version(s: &str) -> bool { + let stripped = strip_v(s); + !stripped.is_empty() && stripped.chars().all(|c| c.is_ascii_digit() || c == '.') +} + +/// Strip a single leading `v` (`v18` → `18`) per nvm/Corepack convention. +fn strip_v(s: &str) -> &str { + s.strip_prefix('v').unwrap_or(s) +} + +/// Evaluate `expected` as a node-semver range against `current`. +/// +/// Returns `None` when the outcome could not be determined — `current` +/// isn't a version, or no `||` group both parsed and matched while at +/// least one group was unparseable — so the caller can fall back to the +/// prefix match. A parsed-and-matching group wins immediately, letting +/// `">=18 || lts/*"` succeed on the evaluable half. +fn range_matches(expected: &str, current: &str) -> Option { + let cur = parse_current_version(current)?; + let mut any_unparseable = false; + for group in expected.split("||") { + let group = group.trim(); + if group.is_empty() { + any_unparseable = true; + continue; + } + let req = normalize_range_group(group) + .and_then(|normalized| semver::VersionReq::parse(&normalized).ok()); + match req { + Some(req) if req.matches(&cur) => return Some(true), + Some(_) => {} + None => any_unparseable = true, + } + } + if any_unparseable { None } else { Some(false) } +} + +/// Rewrite one `||`-free node-semver comparator group into the +/// comma-separated grammar `semver::VersionReq::parse` accepts. +/// +/// Handles hyphen ranges (`18 - 20` → `>=18, <=20`; a partial upper +/// bound is already inclusive of its whole segment in the crate's +/// grammar), whitespace-separated AND comparators, operators detached +/// from their version (`>= 18`), and per-token `v` prefixes. Bare +/// digit-leading tokens get an `=` operator — the crate would otherwise +/// default them to caret, which is looser than node's exact-partial +/// semantics. Wildcard tokens (`*`, `x`) pass through untouched because +/// `=*` does not parse. +fn normalize_range_group(group: &str) -> Option { + let group = group.replace(',', " "); + let tokens: Vec<&str> = group.split_whitespace().collect(); + if tokens.is_empty() { + return None; + } + + if let [low, "-", high] = tokens.as_slice() { + return Some(format!(">={}, <={}", strip_v(low), strip_v(high))); + } + if tokens.contains(&"-") { + return None; + } + + let mut parts: Vec = Vec::with_capacity(tokens.len()); + let mut iter = tokens.iter(); + while let Some(token) = iter.next() { + let (op, rest) = split_operator(token); + if op.is_empty() { + let rest = strip_v(rest); + if rest.starts_with(|c: char| c.is_ascii_digit()) { + parts.push(format!("={rest}")); + } else { + parts.push(rest.to_string()); + } + } else if rest.is_empty() { + let version = iter.next()?; + parts.push(format!("{op}{}", strip_v(version))); + } else { + parts.push(format!("{op}{}", strip_v(rest))); + } + } + Some(parts.join(", ")) +} + +/// Split a leading range operator off a comparator token. Returns +/// `(op, rest)` with `op` ∈ {`>=`, `<=`, `>`, `<`, `=`, `~`, `^`, ``""``}. +fn split_operator(token: &str) -> (&str, &str) { + for op in [">=", "<=", ">", "<", "=", "~", "^"] { + if let Some(rest) = token.strip_prefix(op) { + return (op, rest); + } + } + ("", token) +} + +/// Parse `current` (a `node --version`-style string with the `v` +/// already stripped by detection) into a full [`semver::Version`], +/// padding bare `major`/`major.minor` forms to a triple. Deliberately +/// duplicates the padding in `tool::node::normalize_version` — `types` +/// must not grow a dependency on `tool`. +fn parse_current_version(current: &str) -> Option { + let padded = match current.split('.').count() { + 1 => format!("{current}.0.0"), + 2 => format!("{current}.0"), + _ => current.to_string(), + }; + semver::Version::parse(&padded).ok() +} + #[cfg(test)] mod tests { use super::version_matches; @@ -691,6 +841,131 @@ mod tests { assert!(!version_matches("20.11", "20.110.0")); } + #[test] + fn gte_range_matches_higher_versions() { + // Regression: ">=22.22.2" used to prefix-match as "=22.22.2", + // warning on 22.22.3 and 25.9.0 — both satisfy the range. + assert!(version_matches(">=22.22.2", "22.22.3")); + assert!(version_matches(">=22.22.2", "25.9.0")); + assert!(!version_matches(">=22.22.2", "22.22.1")); + } + + #[test] + fn operator_with_space_before_version() { + assert!(version_matches(">= 18", "20.0.0")); + assert!(!version_matches(">= 18", "17.9.0")); + } + + #[test] + fn partial_comparator_bounds() { + assert!(version_matches(">=18", "18.0.0")); + // node desugars ">22" to ">=23.0.0": 22.x never qualifies. + assert!(!version_matches(">22", "22.5.0")); + assert!(version_matches(">22", "23.0.0")); + assert!(version_matches("<21", "20.99.0")); + assert!(!version_matches("<21", "21.0.0")); + assert!(version_matches("<=20", "20.99.0")); + } + + #[test] + fn caret_ranges() { + // The case bare-prefix semantics must reject but caret accepts. + assert!(version_matches("^20.11", "20.12.0")); + assert!(!version_matches("^20.11", "20.10.9")); + assert!(!version_matches("^20.11", "21.0.0")); + assert!(version_matches("^0.3", "0.3.9")); + assert!(!version_matches("^0.3", "0.4.0")); + } + + #[test] + fn tilde_ranges() { + assert!(version_matches("~18.15", "18.15.7")); + assert!(!version_matches("~18.15", "18.16.0")); + assert!(version_matches("~18.15.0", "18.15.3")); + } + + #[test] + fn space_separated_and_conjunction() { + assert!(version_matches(">=18 <21", "20.5.1")); + assert!(!version_matches(">=18 <21", "21.0.0")); + assert!(!version_matches(">=18 <21", "17.0.0")); + } + + #[test] + fn or_unions() { + assert!(version_matches("18||20", "20.4.2")); + assert!(!version_matches("18||20", "19.0.0")); + assert!(version_matches(">=18 <19 || >=20", "18.5.0")); + assert!(!version_matches(">=18 <19 || >=20", "19.5.0")); + assert!(version_matches(">=18 <19 || >=20", "25.9.0")); + } + + #[test] + fn hyphen_ranges() { + assert!(version_matches("18 - 20", "19.0.0")); + // Inclusive partial upper bound: node treats "- 20" as "<21". + assert!(version_matches("18 - 20", "20.9.9")); + assert!(!version_matches("18 - 20", "21.0.0")); + assert!(!version_matches("18 - 20", "17.9.9")); + } + + #[test] + fn wildcard_ranges() { + assert!(version_matches("20.x", "20.5.1")); + assert!(!version_matches("20.x", "21.0.0")); + assert!(version_matches("20.*", "20.0.0")); + assert!(version_matches("*", "99.0.0")); + } + + #[test] + fn bare_versions_keep_prefix_semantics() { + // Regression guard for the caret trap: the semver crate would + // read a bare "20.11" as "^20.11" and accept 20.12. + assert!(!version_matches("20.11", "20.12.0")); + assert!(version_matches("20", "20.11.0")); + assert!(!version_matches("2", "20.11.0")); + assert!(version_matches("v20", "20.1.0")); + assert!(version_matches("20.11.0", "20.11.0")); + } + + #[test] + fn exact_operator_partial_equality() { + assert!(version_matches("=20.11", "20.11.5")); + assert!(!version_matches("=20.11", "20.12.0")); + } + + #[test] + fn operator_with_v_prefix() { + assert!(version_matches(">=v18", "18.0.0")); + } + + #[test] + fn unparseable_expected_falls_back_to_prefix() { + assert!(!version_matches("lts/*", "22.0.0")); + assert!(!version_matches("lts/jod", "22.0.0")); + assert!(!version_matches("", "20.0.0")); + } + + #[test] + fn unparseable_or_group_does_not_block_parsed_match() { + assert!(version_matches(">=18 || lts/*", "20.0.0")); + } + + #[test] + fn unparseable_current_falls_back_to_prefix() { + assert!(!version_matches(">=18", "not-a-version")); + } + + #[test] + fn prefix_fallback_strips_equals_and_spaced_v() { + // An unparseable `current` forces the prefix fallback; the + // cleaned expected value must survive a bare `=` operator and + // whitespace between the operator and a `v`-prefixed version. + assert!(version_matches("=20.11", "20.11.beta")); + assert!(!version_matches("=20.11", "20.12.beta")); + assert!(version_matches(">= v18", "18.unknown")); + } + #[test] fn detection_warning_can_be_hashed() { use std::collections::HashSet; diff --git a/tests/doctor_env.rs b/tests/doctor_env.rs new file mode 100644 index 0000000..98b57d5 --- /dev/null +++ b/tests/doctor_env.rs @@ -0,0 +1,152 @@ +//! Integration coverage for doctor's lenient handling of invalid +//! env-sourced overrides. +//! +//! `runner doctor` exists to diagnose a misconfigured environment, so a +//! garbage `RUNNER_PM` (e.g. PowerShell's unquoted `$env:RUNNER_PM=deno` +//! capturing deno's REPL banner) must degrade to a warning on the +//! report instead of killing the command. Every other command — and an +//! explicit `--pm` flag, even on doctor — stays strict. +//! +//! Env vars are injected per spawned child (never `std::env::set_var`), +//! so these tests are safe under the parallel test runner. + +use std::path::{Path, PathBuf}; +use std::process::Command; +use std::sync::atomic::{AtomicU32, Ordering}; + +fn runner_binary() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_runner")) +} + +/// Command for the runner binary with every inherited `RUNNER_*` +/// variable scrubbed, so only what a test sets explicitly reaches the +/// child. A dev box exporting e.g. `RUNNER_NO_WARNINGS` or +/// `RUNNER_FALLBACK` would otherwise flip these assertions. Matched +/// case-insensitively — Windows env lookups ignore case. +fn runner_command() -> Command { + let mut cmd = Command::new(runner_binary()); + for (key, _) in std::env::vars_os() { + if key + .to_string_lossy() + .to_ascii_uppercase() + .starts_with("RUNNER_") + { + cmd.env_remove(&key); + } + } + cmd +} + +/// Minimal self-cleaning temp project: a directory holding only a +/// `Cargo.toml`, so detection finds exactly one PM (cargo) and the +/// doctor report is deterministic. +struct TempProject { + path: PathBuf, +} + +static NEXT_ID: AtomicU32 = AtomicU32::new(0); + +impl TempProject { + fn new(prefix: &str) -> Self { + let id = NEXT_ID.fetch_add(1, Ordering::Relaxed); + let path = std::env::temp_dir().join(format!( + "runner-doctor-env-{prefix}-{}-{id}", + std::process::id() + )); + std::fs::create_dir_all(&path).expect("create temp project dir"); + std::fs::write( + path.join("Cargo.toml"), + "[package]\nname = \"fixture\"\nversion = \"0.0.0\"\n", + ) + .expect("write Cargo.toml"); + Self { path } + } + + fn path(&self) -> &Path { + &self.path + } +} + +impl Drop for TempProject { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.path); + } +} + +/// A value shaped like the PowerShell footgun output: multi-line, with +/// an ANSI escape. +const CAPTURED_BANNER: &str = "Deno 2.8.2 exit using ctrl+d\n\u{1b}[33mREPL is running\u{1b}[0m"; + +#[test] +fn doctor_survives_env_pm_garbage_and_reports_it() { + let project = TempProject::new("doctor-lenient"); + let output = runner_command() + .args(["--dir", project.path().to_str().unwrap(), "doctor"]) + .env("RUNNER_PM", CAPTURED_BANNER) + .output() + .expect("runner binary spawns"); + + assert!( + output.status.success(), + "doctor must survive env garbage. stderr: {}", + String::from_utf8_lossy(&output.stderr), + ); + let combined = format!( + "{}{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); + assert!( + combined.contains("RUNNER_PM"), + "the report must attribute the invalid override. output: {combined}", + ); + assert!( + combined.contains("ignored"), + "the report must say the value was ignored. output: {combined}", + ); +} + +#[test] +fn other_commands_stay_strict_on_env_pm_garbage() { + let project = TempProject::new("list-strict"); + let output = runner_command() + .args(["--dir", project.path().to_str().unwrap(), "list"]) + .env("RUNNER_PM", CAPTURED_BANNER) + .output() + .expect("runner binary spawns"); + + assert!( + !output.status.success(), + "non-doctor commands must keep failing on env garbage", + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("RUNNER_PM"), + "the error must name the env source. stderr: {stderr}", + ); +} + +#[test] +fn doctor_with_cli_pm_garbage_still_errors() { + let project = TempProject::new("doctor-cli-strict"); + let output = runner_command() + .args([ + "--dir", + project.path().to_str().unwrap(), + "--pm", + "zoot", + "doctor", + ]) + .output() + .expect("runner binary spawns"); + + assert!( + !output.status.success(), + "an explicit bad --pm flag must stay fatal, even on doctor", + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("unknown package manager"), + "stderr: {stderr}", + ); +}