Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 26 additions & 4 deletions src/cmds/js/vitest_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use regex::Regex;
use serde::Deserialize;
use std::sync::LazyLock;

use crate::core::arg_tokenizer;
use crate::core::stream::exec_capture;
use crate::core::tracking;
use crate::core::utils::{package_manager_exec, strip_ansi};
Expand Down Expand Up @@ -222,8 +223,9 @@ fn vitest_invocation(args: &[String]) -> TestInvocation {
effective.push("--reporter=json".to_string());
}

for arg in args {
if should_skip_vitest_arg(arg) {
let boundary = user_boundary(args);
for (i, arg) in args.iter().enumerate() {
if i < boundary && should_skip_vitest_arg(arg) {
continue;
}
effective.push(arg.clone());
Expand All @@ -243,10 +245,12 @@ fn jest_invocation(args: &[String]) -> TestInvocation {
// Enable JSON structured output
"--json".to_string(),
];
let boundary = user_boundary(args);
effective.extend(
args.iter()
.filter(|arg| !should_skip_jest_arg(arg))
.cloned(),
.enumerate()
.filter(|(i, arg)| *i >= boundary || !should_skip_jest_arg(arg))
.map(|(_, arg)| arg.clone()),
);
TestInvocation {
framework: "jest",
Expand All @@ -255,6 +259,12 @@ fn jest_invocation(args: &[String]) -> TestInvocation {
}
}

/// Index of the user's `--`, or `args.len()`. A `run` or `--json` after it is an argument
/// for the tool, not a mode to strip.
fn user_boundary(args: &[String]) -> usize {
arg_tokenizer::injection_point(&arg_tokenizer::tokenize(args), args.len())
}

fn should_skip_jest_arg(arg: &str) -> bool {
arg == "run"
|| arg.starts_with("--json")
Expand Down Expand Up @@ -456,6 +466,18 @@ mod tests {
);
}

#[test]
fn test_invocation_keeps_skipped_words_after_double_dash() {
assert_eq!(
jest_invocation(&args(&["run", "--", "run", "--json"])).args,
args(&["--no-watch", "--json", "--", "run", "--json"])
);
assert_eq!(
vitest_invocation(&args(&["--", "run"])).args,
args(&["run", "--reporter=json", "--", "run"])
);
}

#[test]
fn test_vitest_parser_json() {
let json = r#"{
Expand Down
18 changes: 15 additions & 3 deletions src/core/args_utils.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
//! Utility functions for argument handling, particularly for restoring "--" escape
//! arguments that clap consumes during parsing.

use std::ffi::{OsStr, OsString};

/// Restores `--` tokens that clap consumed when using `trailing_var_arg = true`.
///
/// Returns `parsed_args` unchanged when `raw_args` has the same or fewer `--` tokens
Expand All @@ -11,6 +13,13 @@ pub fn restore_double_dash(parsed_args: &[String]) -> Vec<String> {
restore_double_dash_with_raw(parsed_args, &raw_args)
}

/// [`restore_double_dash`] for an external subcommand's `Vec<OsString>`. Reads
/// `std::env::args_os()`, since `std::env::args()` panics on a non-UTF-8 argument.
pub fn restore_double_dash_os(parsed_args: &[OsString]) -> Vec<OsString> {
let raw_args: Vec<OsString> = std::env::args_os().collect();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocks merge (item 3): nothing pins this args_os() read. With std::env::args().map(OsString::from).collect() instead, all 5220 tests pass. A test that runs an Other arm with a non-UTF-8 argument (rtk go foo $'\xff.py', which works on head) and asserts the child's argv bytes and exit code would turn that mutation red.

restore_double_dash_with_raw(parsed_args, &raw_args)
}

/// Testable version that takes raw_args explicitly.
///
/// Precondition: all callers use `trailing_var_arg = true`, which guarantees that
Expand All @@ -22,9 +31,12 @@ pub fn restore_double_dash(parsed_args: &[String]) -> Vec<String> {
///
/// Returning `raw_args[user_region_start..]` restores all stripped `--` tokens at
/// their original positions without any value-based matching.
pub fn restore_double_dash_with_raw(parsed_args: &[String], raw_args: &[String]) -> Vec<String> {
let raw_dash_count = raw_args.iter().filter(|a| a.as_str() == "--").count();
let parsed_dash_count = parsed_args.iter().filter(|a| a.as_str() == "--").count();
pub fn restore_double_dash_with_raw<T: AsRef<OsStr> + Clone>(
parsed_args: &[T],
raw_args: &[T],
) -> Vec<T> {
let raw_dash_count = raw_args.iter().filter(|a| a.as_ref() == "--").count();
let parsed_dash_count = parsed_args.iter().filter(|a| a.as_ref() == "--").count();

if raw_dash_count <= parsed_dash_count {
return parsed_args.to_vec();
Expand Down
34 changes: 33 additions & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2498,6 +2498,37 @@ fn pings_telemetry(parsed: &Result<Cli, clap::Error>) -> bool {
)
}

/// clap's `trailing_var_arg` strips a `--` that leads a variant's arguments, so
/// `rtk tsc -- zzz` would forward `tsc zzz` (#4753). Restoring it here, once, lets each
/// module place it after the options it injects, the way it places the rest of `args`.
fn restore_leading_double_dash(command: &mut Commands) {
match command {
Commands::Tsc { args }
| Commands::Prettier { args }
| Commands::Phpunit { args }
| Commands::Phpstan { args }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocks merge (item 1): phpstan appends --error-format json after the user's arguments (phpstan_cmd.rs:76), so with the restored separator rtk phpstan -- analyse src runs phpstan -- analyse src --error-format json. Real phpstan 2.3.4: Path /app/--error-format does not exist, no finding, where develop reported it through the JSON filter. Either insert the format before the user's first -- (as a single --error-format=json token: two tokens in front of the command name make phpstan read json as the command), or leave phpstan out of this list.

| Commands::Pest { args }
| Commands::Mvn { args }
| Commands::Rspec { args }
| Commands::Rubocop { args }
| Commands::Vitest { args }
| Commands::Jest { args } => *args = core::args_utils::restore_double_dash(args),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocks merge (item 2): these 10 arms now call restore_double_dash on every run, and it reads std::env::args(), which panics on a non-UTF-8 argv[0]. Run by absolute path from such a directory, rtk tsc a and rtk jest a exit 101 here and run the tool on develop. Reading args_os() lossily is safe: clap already required every user argument on these arms to be UTF-8, and argv[0] is outside the returned region.

Commands::Go {
command: GoCommands::Other(args),
}
| Commands::Cargo {
command: CargoCommands::Other(args),
}
| Commands::Docker {
command: DockerCommands::Other(args),
}
| Commands::Kubectl {
command: KubectlCommands::Other(args),
} => *args = core::args_utils::restore_double_dash_os(args),
_ => {}
}
}

fn run_cli() -> Result<i32> {
let parsed = try_parse_cli_from(std::env::args_os());

Expand All @@ -2506,7 +2537,7 @@ fn run_cli() -> Result<i32> {
core::telemetry::maybe_ping();
}

let cli = match parsed {
let mut cli = match parsed {
Ok(cli) => cli,
Err(e) => {
if matches!(e.kind(), ErrorKind::DisplayHelp | ErrorKind::DisplayVersion) {
Expand All @@ -2515,6 +2546,7 @@ fn run_cli() -> Result<i32> {
return run_fallback(e);
}
};
restore_leading_double_dash(&mut cli.command);

// Warn if installed hook is outdated/missing (1/day, non-blocking).
// Skip for Gain (shows its own inline warning), Init/Verify (manage the
Expand Down
108 changes: 108 additions & 0 deletions tests/passthrough_double_dash_test.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
//! A leading `--` for the passthrough commands of #4753. clap's `trailing_var_arg` strips a
//! `--` that leads `args` (or an external subcommand), so `rtk tsc -- zzz` forwarded
//! `tsc zzz` and a path after the separator was read as an option or a subcommand. The
//! separator has to reach the tool, after any options rtk injects, and a word after it such
//! as `run` must stay an argument. Stubs each tool on PATH with a script that records its argv.

#![cfg(unix)]

use std::fs;
use std::os::unix::fs::PermissionsExt;

mod common;

fn shell_quote(path: &std::path::Path) -> String {
format!("'{}'", path.display().to_string().replace('\'', "'\\''"))
}

/// Runs `rtk <tool> <args>` against a `<tool>` stub and returns the argv the stub received.
fn tool_argv(tool: &str, args: &[&str]) -> Vec<String> {
let dir = common::tempdir();
let argv_file = dir.path().join("argv.txt");
let stub_path = dir.path().join(tool);

fs::write(
&stub_path,
format!(
"#!/bin/sh\nprintf '%s\\n' \"$@\" > {}\n",
shell_quote(&argv_file)
),
)
.expect("write stub");
fs::set_permissions(&stub_path, fs::Permissions::from_mode(0o755)).expect("chmod stub");

let path_with_stub = format!(
"{}:{}",
dir.path().display(),
std::env::var("PATH").unwrap_or_default()
);

let out = common::rtk_command()
.arg(tool)
.args(args)
.env("PATH", path_with_stub)
.env("LC_ALL", "C")
.current_dir(dir.path())
.output()
.expect("spawn rtk");

fs::read_to_string(&argv_file)
.unwrap_or_else(|e| {
panic!(
"read captured argv for {tool}: {e}; rtk stdout={} stderr={}",
String::from_utf8_lossy(&out.stdout),
String::from_utf8_lossy(&out.stderr)
)
})
.lines()
.map(str::to_string)
.collect()
}

#[test]
fn leading_double_dash_reaches_the_tool() {
let cases: &[(&str, &[&str])] = &[
("tsc", &[]),
("prettier", &[]),
("go", &[]),
("cargo", &[]),
("phpunit", &[]),
("phpstan", &[]),
("mvn", &[]),
("kubectl", &[]),
("docker", &[]),
("rspec", &["--format", "json"]),
("rubocop", &["--format", "json"]),
("vitest", &["run", "--reporter=json"]),
("jest", &["--no-watch", "--json"]),
("pest", &["--no-progress"]),
];
let mut failures = Vec::new();
for (tool, injected) in cases {
let mut expected: Vec<&str> = injected.to_vec();
expected.extend(["--", "zzz"]);
let argv = tool_argv(tool, &["--", "zzz"]);
if argv != expected {
failures.push(format!("{tool}: got {argv:?}, want {expected:?}"));
}
}
assert!(failures.is_empty(), "{}", failures.join("\n"));
}

#[test]
fn a_double_dash_after_an_argument_is_forwarded_once() {
assert_eq!(tool_argv("tsc", &["-p", "--", "zzz"]), ["-p", "--", "zzz"]);
assert_eq!(tool_argv("go", &["zzz", "--", "zzz"]), ["zzz", "--", "zzz"]);
}

#[test]
fn run_after_double_dash_stays_an_argument() {
assert_eq!(
tool_argv("vitest", &["--", "run"]),
["run", "--reporter=json", "--", "run"]
);
assert_eq!(
tool_argv("jest", &["--", "run"]),
["--no-watch", "--json", "--", "run"]
);
}
Loading