diff --git a/src/discover/registry.rs b/src/discover/registry.rs index e16da48dca..eb4c82316f 100644 --- a/src/discover/registry.rs +++ b/src/discover/registry.rs @@ -77,6 +77,13 @@ static ENV_PREFIX: LazyLock = LazyLock::new(|| { static GIT_GLOBAL_OPT: LazyLock = LazyLock::new(|| { Regex::new(r"^(?:(?:-C\s+\S+|-c\s+\S+|--git-dir(?:=\S+|\s+\S+)|--work-tree(?:=\S+|\s+\S+)|--no-pager|--no-optional-locks|--bare|--literal-pathspecs)\s+)+").unwrap() }); +// Strip pnpm global options that precede the subcommand so `pnpm -r install`, +// `pnpm --filter @app install`, `pnpm -w list` route to the same rules as their +// bare forms. Only a fixed, known set is stripped — never an unknown `-x`, so a +// non-install flag-first command can't be mis-rewritten into a filter with savings. +static PNPM_GLOBAL_OPT: LazyLock = LazyLock::new(|| { + Regex::new(r"^(?:(?:-r|--recursive|-w|--workspace-root|--filter(?:=\S+|\s+\S+)|-F(?:=\S+|\s+\S+))\s+)+").unwrap() +}); // Issue #1362: each capture expects a SINGLE file argument (`\S+$`). Multi-file // invocations like `head -3 a b c` fail to match so the segment is passed through // to the native `head`/`tail` binary — which already handles multi-file with @@ -143,6 +150,21 @@ pub fn classify_command(cmd: &str) -> Classification { // Strip golangci-lint global options before `run` so classify/rewrite stays // aligned with the runtime wrapper behavior. let cmd_normalized = strip_golangci_global_opts(&cmd_normalized); + // Strip pnpm global options (-r, --filter, -w) before the subcommand so + // `pnpm -r install` classifies like `pnpm install` — but only adopt the + // stripped form when it routes to the `rtk pnpm` rule itself. For the tool + // rules reachable via `pnpm exec`/`pnpm run` (`pnpm -r exec vitest`, + // `pnpm -r lint`, …) the rewrite matches the original flag-first text and + // never fires, so classifying the stripped form there would report a + // Supported saving the hook can't deliver — misleading `rtk discover` and + // `rtk session`, which count `Supported` as covered. See #3275. + let cmd_pnpm_stripped = strip_pnpm_global_opts(&cmd_normalized); + let cmd_normalized = + if cmd_pnpm_stripped != cmd_normalized && matches_pnpm_rule(&cmd_pnpm_stripped) { + cmd_pnpm_stripped + } else { + cmd_normalized + }; let cmd_clean = cmd_normalized.as_str(); // Exclude cat/head/tail with redirect operators — these are writes, not reads (#315) @@ -366,6 +388,39 @@ fn strip_git_global_opts(cmd: &str) -> String { format!("git {}", stripped.trim()) } +/// Strip pnpm global options before the subcommand (mirror of `strip_git_global_opts`). +/// `pnpm -r install` → `pnpm install`; `pnpm --filter @app list` → `pnpm list`. +/// Classification only — the rewrite re-emits the ORIGINAL command, so the stripped +/// flags are preserved (e.g. `pnpm -r install` → `rtk pnpm -r install`). +/// Returns the original string unchanged if not a pnpm command. +fn strip_pnpm_global_opts(cmd: &str) -> String { + // Require a single ASCII space after `pnpm` — the exact boundary the rewrite's + // `strip_word_prefix` enforces — so classify and rewrite can never diverge on a + // tab or other whitespace separator (that would resurrect the class of bug + // #3275 closes: Supported on one side, un-rewritable on the other). Extra + // spaces are still tolerated via `trim_start` (`pnpm -r install`), since + // `PNPM_GLOBAL_OPT` is `^`-anchored and a leading space would skip the strip. + if !cmd.starts_with("pnpm ") { + return cmd.to_string(); + } + let after_pnpm = cmd[5..].trim_start(); // skip "pnpm ", then any extra spaces + let stripped = PNPM_GLOBAL_OPT.replace(after_pnpm, ""); + format!("pnpm {}", stripped.trim()) +} + +/// True when `cmd` (already normalized) routes to the `rtk pnpm` rule rather than +/// a tool rule reachable through `pnpm exec`/`pnpm run`. Gates the pnpm +/// global-option strip in `classify_command`: adopting the stripped form for a +/// tool rule would diverge from the rewrite, which matches the original +/// flag-first text and never fires there. See #3275. +fn matches_pnpm_rule(cmd: &str) -> bool { + REGEX_SET + .matches(cmd) + .into_iter() + .next_back() + .is_some_and(|idx| RULES[idx].rtk_cmd == "rtk pnpm") +} + /// Strip golangci-lint global options before the `run` subcommand. /// `golangci-lint --color never run ./...` → `golangci-lint run ./...` /// Returns the original string unchanged if this is not a supported compact `run` invocation. @@ -2911,6 +2966,179 @@ mod tests { ); } + // --- pnpm global option stripping (-r / --filter / -w) --- + + #[test] + fn test_rewrite_pnpm_recursive_install() { + assert_eq!( + rewrite_command_no_prefixes("pnpm -r install", &[]), + Some("rtk pnpm -r install".into()) + ); + } + + #[test] + fn test_rewrite_pnpm_filter_install() { + assert_eq!( + rewrite_command_no_prefixes("pnpm --filter @app install", &[]), + Some("rtk pnpm --filter @app install".into()) + ); + } + + #[test] + fn test_rewrite_pnpm_filter_short_install() { + assert_eq!( + rewrite_command_no_prefixes("pnpm -F @app install", &[]), + Some("rtk pnpm -F @app install".into()) + ); + } + + #[test] + fn test_rewrite_pnpm_filter_eq_install() { + assert_eq!( + rewrite_command_no_prefixes("pnpm --filter=@app install", &[]), + Some("rtk pnpm --filter=@app install".into()) + ); + } + + #[test] + fn test_rewrite_pnpm_workspace_root_install() { + assert_eq!( + rewrite_command_no_prefixes("pnpm -w install", &[]), + Some("rtk pnpm -w install".into()) + ); + } + + #[test] + fn test_rewrite_pnpm_recursive_filter_combo() { + assert_eq!( + rewrite_command_no_prefixes("pnpm -r --filter @app list", &[]), + Some("rtk pnpm -r --filter @app list".into()) + ); + } + + // No-regression: bare forms behave exactly as before. + #[test] + fn test_rewrite_pnpm_bare_install_unchanged() { + assert_eq!( + rewrite_command_no_prefixes("pnpm install", &[]), + Some("rtk pnpm install".into()) + ); + } + + #[test] + fn test_rewrite_pnpm_run_build_unchanged() { + assert_eq!( + rewrite_command_no_prefixes("pnpm run build", &[]), + Some("rtk pnpm run build".into()) + ); + } + + // Bare `pnpm build` is still NOT rewritten: it would only hit the passthrough + // (no output parser), so rewriting it would add false-positive surface for zero + // savings. Stripping global opts must not change this. + #[test] + fn test_rewrite_pnpm_bare_build_none() { + assert_eq!(rewrite_command_no_prefixes("pnpm build", &[]), None); + } + + // False-positive guards. + #[test] + fn test_rewrite_pnpm_filter_no_subcommand_none() { + // A filter with no subcommand must not be rewritten. + assert_eq!(rewrite_command_no_prefixes("pnpm --filter @app", &[]), None); + } + + #[test] + fn test_rewrite_pnpm_unknown_flag_not_stripped() { + // `-x` is not a known global opt → not stripped → no subcommand → None. + assert_eq!(rewrite_command_no_prefixes("pnpm -x build", &[]), None); + // Load-bearing case for the fixed-set design: `install` IS a routed + // subcommand, so if `-x` were stripped this would rewrite to + // `rtk pnpm -x install`, which reaches clap and dies. Only the fixed + // allowlist keeps it a safe passthrough (None). + assert_eq!(rewrite_command_no_prefixes("pnpm -x install", &[]), None); + } + + #[test] + fn test_rewrite_pnpm_recursive_lint_safe_noop() { + // `pnpm lint` classifies as Supported, but the ORIGINAL `pnpm -r lint` + // matches no lint rewrite-prefix → safe no-op (never a malformed rewrite). + assert_eq!(rewrite_command_no_prefixes("pnpm -r lint", &[]), None); + } + + #[test] + fn test_classify_pnpm_flag_first_tool_stays_unsupported() { + // #3275 blocker: the strip must not make a tool rule reachable via + // `pnpm exec`/`pnpm run` classify as Supported — its rewrite matches the + // original flag-first text and never fires, so a Supported verdict would + // advertise savings `rtk discover`/`rtk session` can never deliver. These + // must classify exactly as on develop: Unsupported(pnpm). + for cmd in [ + "pnpm -r lint", + "pnpm -r exec eslint .", + "pnpm --filter @app exec vitest run", + "pnpm -F web exec playwright test", + "pnpm -r exec tsc --noEmit", + "pnpm -w exec next build", + ] { + assert!( + matches!(classify_command(cmd), Classification::Unsupported { .. }), + "{cmd} must stay Unsupported (rewrite can't fire), got: {:?}", + classify_command(cmd) + ); + } + } + + #[test] + fn test_classify_pnpm_flag_first_install_still_supported() { + // The gate keeps everything the PR claims: flag-first forms that route to + // the `rtk pnpm` rule stay Supported. + for cmd in [ + "pnpm -r install", + "pnpm --filter @app list", + "pnpm -w install", + "pnpm -r outdated", + ] { + assert!( + matches!( + classify_command(cmd), + Classification::Supported { + rtk_equivalent: "rtk pnpm", + .. + } + ), + "{cmd} must classify as rtk pnpm, got: {:?}", + classify_command(cmd) + ); + } + } + + #[test] + fn test_rewrite_pnpm_extra_whitespace() { + // Extra spaces before the global flag must not skip the strip + // (`PNPM_GLOBAL_OPT` is `^`-anchored, so the slice is trimmed first). + assert_eq!( + rewrite_command_no_prefixes("pnpm -r install", &[]), + Some("rtk pnpm -r install".into()) + ); + } + + #[test] + fn test_pnpm_tab_separator_no_classify_rewrite_divergence() { + // A non-space separator must NOT be stripped: the rewrite's + // `strip_word_prefix` only accepts an ASCII space, so classify has to + // agree and stay Unsupported. If the strip tolerated `\t` (or any other + // whitespace), classify would say Supported(rtk pnpm) while rewrite + // returned None — the exact classify/rewrite divergence #3275 closes. + let cmd = "pnpm\t-r install"; + assert!( + matches!(classify_command(cmd), Classification::Unsupported { .. }), + "tab-separated pnpm must stay Unsupported, got: {:?}", + classify_command(cmd) + ); + assert_eq!(rewrite_command_no_prefixes(cmd, &[]), None); + } + #[test] fn test_rewrite_cargo_test() { assert_eq!( diff --git a/src/main.rs b/src/main.rs index 4ac0df0a28..792672fe9b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -219,6 +219,14 @@ enum Commands { #[arg(long, short = 'F')] filter: Vec, + /// Recursive across workspace packages (pnpm -r) + #[arg(long, short = 'r')] + recursive: bool, + + /// Run in the workspace root (pnpm -w) + #[arg(long = "workspace-root", short = 'w')] + workspace_root: bool, + #[command(subcommand)] command: PnpmCommands, }, @@ -1722,20 +1730,58 @@ fn build_k8s_logs_args(pod: String, container: Option) -> Vec { args } -/// Merge pnpm global filters args with other ones for standard String-based commands -fn merge_pnpm_args(filters: &[String], args: &[String]) -> Vec { - filters - .iter() - .map(|filter| format!("--filter={}", filter)) +/// Leading pnpm global flags (recursive / workspace-root), forwarded to pnpm. +/// +/// These are appended *after* the subcommand by the callers (e.g. `run_install` +/// builds `pnpm install `), i.e. `pnpm -r install` typed by the user runs +/// as `pnpm install -r`. This is behavior-preserving: `-r`/`-w`/`--filter` are +/// root-level pnpm options accepted in either position (same established pattern +/// already used for `--filter`). Verified against pnpm 9.15.4: `pnpm install +/// --help` lists `-r, --recursive`, `-w, --workspace-root` and `--filter` as +/// options of `install` itself, and `pnpm ` vs `pnpm ` +/// produce byte-identical output on `ls` and `outdated` for all three flags. If a +/// future pnpm makes position significant, emit these before the subcommand +/// instead (as the passthrough path already does via merge order). +fn pnpm_global_flags(recursive: bool, workspace_root: bool) -> Vec { + let mut flags = Vec::new(); + if recursive { + flags.push("-r".to_string()); + } + if workspace_root { + flags.push("-w".to_string()); + } + flags +} + +/// Merge pnpm global flags + filters with the subcommand args (String-based commands). +fn merge_pnpm_args( + filters: &[String], + recursive: bool, + workspace_root: bool, + args: &[String], +) -> Vec { + pnpm_global_flags(recursive, workspace_root) + .into_iter() + .chain(filters.iter().map(|filter| format!("--filter={}", filter))) .chain(args.iter().cloned()) .collect() } -/// Merge pnpm global filters args with other ones, using OsString for passthrough compatibility -fn merge_pnpm_args_os(filters: &[String], args: &[OsString]) -> Vec { - filters - .iter() - .map(|filter| OsString::from(format!("--filter={}", filter))) +/// Same as `merge_pnpm_args` but OsString-based, for passthrough compatibility. +fn merge_pnpm_args_os( + filters: &[String], + recursive: bool, + workspace_root: bool, + args: &[OsString], +) -> Vec { + pnpm_global_flags(recursive, workspace_root) + .into_iter() + .map(OsString::from) + .chain( + filters + .iter() + .map(|filter| OsString::from(format!("--filter={}", filter))), + ) .chain(args.iter().cloned()) .collect() } @@ -1763,6 +1809,33 @@ fn validate_pnpm_filters(filters: &[String], command: &PnpmCommands) -> Option Option { + match command { + PnpmCommands::Typecheck { .. } if recursive || workspace_root => { + let mut flags = Vec::new(); + if recursive { + flags.push("-r"); + } + if workspace_root { + flags.push("-w"); + } + Some(format!( + "[rtk] warning: pnpm tsc does not support {} — ignored", + flags.join(", ") + )) + } + _ => None, + } +} + fn main() { // Reset SIGPIPE to default handler so writing to a closed pipe // e.g `rtk git log | head` exits silently instead of panicking. @@ -2071,32 +2144,41 @@ fn run_cli() -> Result { Commands::Psql { args } => psql_cmd::run(&args, cli.verbose)?, - Commands::Pnpm { filter, command } => { + Commands::Pnpm { + filter, + recursive, + workspace_root, + command, + } => { // Warns user if filters are used with unsupported subcommands like typecheck if let Some(warning) = validate_pnpm_filters(&filter, &command) { eprintln!("{}", warning); } + if let Some(warning) = validate_pnpm_globals(recursive, workspace_root, &command) { + eprintln!("{}", warning); + } match command { PnpmCommands::List { depth, args } => pnpm_cmd::run( pnpm_cmd::PnpmCommand::List { depth }, - &merge_pnpm_args(&filter, &args), + &merge_pnpm_args(&filter, recursive, workspace_root, &args), cli.verbose, )?, PnpmCommands::Outdated { args } => pnpm_cmd::run( pnpm_cmd::PnpmCommand::Outdated, - &merge_pnpm_args(&filter, &args), + &merge_pnpm_args(&filter, recursive, workspace_root, &args), cli.verbose, )?, PnpmCommands::Install { args } => pnpm_cmd::run( pnpm_cmd::PnpmCommand::Install, - &merge_pnpm_args(&filter, &args), + &merge_pnpm_args(&filter, recursive, workspace_root, &args), cli.verbose, )?, PnpmCommands::Typecheck { args } => tsc_cmd::run(Some("pnpm"), &args, cli.verbose)?, - PnpmCommands::Other(args) => { - pnpm_cmd::run_passthrough(&merge_pnpm_args_os(&filter, &args), cli.verbose)? - } + PnpmCommands::Other(args) => pnpm_cmd::run_passthrough( + &merge_pnpm_args_os(&filter, recursive, workspace_root, &args), + cli.verbose, + )?, } } @@ -3238,6 +3320,46 @@ mod tests { } } + #[test] + fn test_pnpm_recursive_install_parsing() { + // The rewriter emits `rtk pnpm -r install` for `pnpm -r install`; it must parse + // to Install with recursive=true (not error, not Other) so `-r` is forwarded. + let cli = Cli::try_parse_from(["rtk", "pnpm", "-r", "install"]).unwrap(); + match cli.command { + Commands::Pnpm { + recursive, + workspace_root, + command, + .. + } => { + assert!(recursive); + assert!(!workspace_root); + assert!(matches!(command, PnpmCommands::Install { .. })); + } + _ => panic!("Expected Pnpm command"), + } + } + + #[test] + fn test_pnpm_workspace_root_filter_install_parsing() { + let cli = + Cli::try_parse_from(["rtk", "pnpm", "-w", "--filter", "@app", "install"]).unwrap(); + match cli.command { + Commands::Pnpm { + filter, + recursive, + workspace_root, + command, + } => { + assert!(!recursive); + assert!(workspace_root); + assert_eq!(filter, vec!["@app".to_string()]); + assert!(matches!(command, PnpmCommands::Install { .. })); + } + _ => panic!("Expected Pnpm command"), + } + } + #[test] fn test_git_commit_long_flag_multiple() { let cli = Cli::try_parse_from([ @@ -3865,7 +3987,10 @@ mod tests { let filters = vec![]; let args = vec!["--depth=0".to_string(), "--no-verbose".to_string()]; let expected_args = vec!["--depth=0", "--no-verbose"]; - assert_eq!(merge_pnpm_args(&filters, &args), expected_args); + assert_eq!( + merge_pnpm_args(&filters, false, false, &args), + expected_args + ); } #[test] @@ -3883,7 +4008,10 @@ mod tests { "--depth=0", "--no-verbose", ]; - assert_eq!(merge_pnpm_args(&filters, &args), expected_args); + assert_eq!( + merge_pnpm_args(&filters, false, false, &args), + expected_args + ); } #[test] @@ -3891,7 +4019,10 @@ mod tests { let filters = vec![]; let args = vec![OsString::from("--depth=0")]; let expected_args = vec![OsString::from("--depth=0")]; - assert_eq!(merge_pnpm_args_os(&filters, &args), expected_args); + assert_eq!( + merge_pnpm_args_os(&filters, false, false, &args), + expected_args + ); } #[test] @@ -3902,7 +4033,39 @@ mod tests { OsString::from("--filter=@app1"), OsString::from("--depth=0"), ]; - assert_eq!(merge_pnpm_args_os(&filters, &args), expected_args); + assert_eq!( + merge_pnpm_args_os(&filters, false, false, &args), + expected_args + ); + } + + #[test] + fn test_merge_recursive_workspace_root_ordering() { + // Global flags come first, then filters, then the subcommand args — + // executed as `pnpm -r -w --filter=@app ` (position-independent, + // verified empirically; see pnpm_global_flags docs). + let filters = vec!["@app".to_string()]; + let args = vec!["--depth=0".to_string()]; + assert_eq!( + merge_pnpm_args(&filters, true, true, &args), + vec!["-r", "-w", "--filter=@app", "--depth=0"] + ); + // No flags → unchanged from the filters-only behavior. + assert_eq!(merge_pnpm_args(&[], false, false, &args), vec!["--depth=0"]); + } + + #[test] + fn test_validate_pnpm_globals_warns_on_typecheck() { + let cmd = PnpmCommands::Typecheck { args: vec![] }; + assert!(validate_pnpm_globals(true, false, &cmd).is_some()); + assert!(validate_pnpm_globals(false, true, &cmd).is_some()); + // Both flags at once: single warning naming both. + let both = validate_pnpm_globals(true, true, &cmd).unwrap(); + assert!(both.contains("-r") && both.contains("-w")); + assert!(validate_pnpm_globals(false, false, &cmd).is_none()); + // Non-typecheck subcommands forward the flags → no warning. + let install = PnpmCommands::Install { args: vec![] }; + assert!(validate_pnpm_globals(true, true, &install).is_none()); } #[test] @@ -3916,6 +4079,7 @@ mod tests { Commands::Pnpm { filter, command: PnpmCommands::List { depth, args }, + .. } => { assert_eq!(depth, 0); assert_eq!(filter, vec!["@app1", "@app2"]); @@ -3976,7 +4140,9 @@ mod tests { ]) .unwrap(); match cli.command { - Commands::Pnpm { filter, command } => { + Commands::Pnpm { + filter, command, .. + } => { let warning = validate_pnpm_filters(&filter, &command); assert!(filter.is_empty()); @@ -4003,7 +4169,9 @@ mod tests { ]) .unwrap(); match cli.command { - Commands::Pnpm { filter, command } => { + Commands::Pnpm { + filter, command, .. + } => { let warning = validate_pnpm_filters(&filter, &command).unwrap(); assert_eq!(filter, vec!["@app1", "@app2"]);