wip

Author: mxcl

TESTING.md

@@ -10,7 +10,29 @@ This document describes the test suite for `cargox`, particularly focusing on sa
 - `split_spec_with_version` - Verifies parsing of crate names with `@version` syntax
 - `split_spec_rejects_empty` - Ensures invalid specs are rejected
 
-### 2. Install Directory Tests
+### 2. Argument Parsing Tests
+
+These tests verify that command-line arguments are correctly parsed and that arguments intended for the target binary are not intercepted by cargox.
+
+#### Integration Tests (`tests/help_flag.rs`)
+
+**Critical regression prevention tests** for ensuring `cargox bat --help` shows bat's help, not cargox's help:
+
+- `test_help_flag_passed_to_binary` - **Most critical test**: Verifies that flags like `--help` after the crate spec are passed to the target binary and not intercepted by cargox's argument parser. Ensures we don't regress the fix for this common use case.
+
+- `test_cargox_help_still_works` - Verifies that `cargox --help` (without a crate spec) still shows cargox's own help text.
+
+- `test_cargox_flags_before_crate_spec` - Verifies that flags before the crate spec (like `cargox --force bat`) are correctly parsed by cargox.
+
+- `test_bin_flag_parsing` - Verifies that the `--bin` flag works correctly and that subsequent arguments are passed to the binary.
+
+#### Unit Tests (`src/cli.rs`)
+
+- `parse_args_separates_binary_args_correctly` - Unit test demonstrating the difference between standard clap parsing and custom argument separation
+- `parse_args_handles_bin_flag` - Verifies `--bin` flag parsing
+- `parse_args_handles_force_flag` - Verifies `-f`/`--force` flag parsing
+
+### 3. Install Directory Tests
 
 These tests verify that `cargox` uses the correct, sandboxed installation directories:
 

src/cli.rs

@@ -1,4 +1,6 @@
+use anyhow::{anyhow, Result};
 use clap::Parser;
+use std::env;
 use std::ffi::OsString;
 
 /// Run Cargo binaries on demand, installing them via `cargo-binstall` when missing.
@@ -29,3 +31,110 @@ pub struct Cli {
     #[arg(trailing_var_arg = true, value_name = "binary-args")]
     pub args: Vec<OsString>,
 }
+
+impl Cli {
+    /// Parse arguments, ensuring that arguments after the crate spec are passed to the binary
+    /// rather than being intercepted by clap. This allows `cargox bat --help` to show bat's
+    /// help rather than cargox's help.
+    pub fn parse_args() -> Result<Self> {
+        let mut args: Vec<OsString> = env::args_os().collect();
+
+        // Skip the program name
+        if args.is_empty() {
+            return Err(anyhow!("no program name in arguments"));
+        }
+        args.remove(0);
+
+        // Find the first positional argument (crate spec) by iterating through args
+        // and stopping at the first argument that doesn't start with `-` and isn't a value for a flag
+        let mut crate_spec_idx = None;
+        let mut i = 0;
+        let mut skip_next = false;
+
+        while i < args.len() {
+            if skip_next {
+                skip_next = false;
+                i += 1;
+                continue;
+            }
+
+            let arg = args[i].to_string_lossy();
+
+            // Check if this is a flag that takes a value
+            if arg == "--bin" {
+                skip_next = true;
+                i += 1;
+                continue;
+            }
+
+            // If it doesn't start with `-`, it's the crate spec
+            if !arg.starts_with('-') {
+                crate_spec_idx = Some(i);
+                break;
+            }
+
+            i += 1;
+        }
+
+        // If we found a crate spec, split args at that point
+        let (cargox_args, binary_args) = if let Some(idx) = crate_spec_idx {
+            let mut cargox_args = args[..idx].to_vec();
+            // Add the crate spec to cargox args
+            cargox_args.push(args[idx].clone());
+            // Everything after crate spec goes to the binary
+            let binary_args = args[idx + 1..].to_vec();
+            (cargox_args, binary_args)
+        } else {
+            // No crate spec found, let clap handle it (will show help or error)
+            (args, vec![])
+        };
+
+        // Parse cargox arguments with clap
+        let mut cli = match Cli::try_parse_from(
+            std::iter::once(OsString::from("cargox")).chain(cargox_args),
+        ) {
+            Ok(cli) => cli,
+            Err(e) => {
+                // Let clap print the error/help message and exit
+                e.exit();
+            }
+        };
+
+        // Set the binary arguments
+        cli.args = binary_args;
+
+        Ok(cli)
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn parse_args_separates_binary_args_correctly() {
+        // Simulate: cargox bat --help
+        let result = Cli::try_parse_from(["cargox", "bat", "--help"]);
+        // This would fail because clap would see --help and try to show cargox's help
+        // Our custom parse_args should prevent this by separating the args
+        assert!(result.is_err()); // Without custom parsing, this intercepts --help
+
+        // The custom parse needs to be tested differently since it reads from env::args_os()
+        // We'll add an integration test for this behavior
+    }
+
+    #[test]
+    fn parse_args_handles_bin_flag() {
+        let cli = Cli::try_parse_from(["cargox", "--bin", "foo", "mycrate"]).unwrap();
+        assert_eq!(cli.crate_spec, "mycrate");
+        assert_eq!(cli.bin, Some("foo".to_string()));
+        assert_eq!(cli.args.len(), 0);
+    }
+
+    #[test]
+    fn parse_args_handles_force_flag() {
+        let cli = Cli::try_parse_from(["cargox", "-f", "mycrate"]).unwrap();
+        assert_eq!(cli.crate_spec, "mycrate");
+        assert!(cli.force);
+    }
+}

src/main.rs

@@ -8,7 +8,6 @@ use std::path::PathBuf;
 use std::process::{ExitStatus, exit};
 
 use anyhow::{Context, Result};
-use clap::Parser;
 
 use cli::Cli;
 use executor::execute_binary;
@@ -24,7 +23,7 @@ fn main() {
 }
 
 fn run_application() -> Result<ExitStatus> {
-    let cli = parse_arguments();
+    let cli = parse_arguments()?;
     let target = parse_target_from_cli(&cli)?;
 
     if should_use_existing_binary(&cli, &target) {
@@ -34,8 +33,8 @@ fn run_application() -> Result<ExitStatus> {
     install_and_run_binary(&target, &cli)
 }
 
-fn parse_arguments() -> Cli {
-    Cli::parse()
+fn parse_arguments() -> Result<Cli> {
+    Cli::parse_args()
 }
 
 fn parse_target_from_cli(cli: &Cli) -> Result<Target> {

tests/help_flag.rs

@@ -0,0 +1,115 @@
+use std::process::Command;
+
+/// This test ensures that flags like --help after the crate spec are passed to the target binary
+/// and not intercepted by cargox's own argument parser.
+///
+/// Regression test for: cargox bat --help should show bat's help, not cargox's help
+#[test]
+fn test_help_flag_passed_to_binary() {
+    // Build the binary first
+    let binary_path = env!("CARGO_BIN_EXE_cargox");
+
+    // Run: cargox bat --help
+    // We can't actually install bat in the test, but we can verify that:
+    // 1. cargox doesn't show its own help (which would exit with code 0)
+    // 2. The --help flag gets passed through to the execution logic
+    //
+    // Since bat isn't installed, this will fail trying to find/install bat,
+    // but importantly it won't show cargox's help text
+    let output = Command::new(binary_path)
+        .args(["bat", "--help"])
+        .output()
+        .expect("Failed to execute cargox");
+
+    let stderr = String::from_utf8_lossy(&output.stderr);
+    let stdout = String::from_utf8_lossy(&output.stdout);
+
+    // The key assertion: cargox's help should NOT appear
+    // If the help was intercepted, we'd see "Run Cargo binaries on demand" in the output
+    assert!(
+        !stdout.contains("Run Cargo binaries on demand"),
+        "cargox help text appeared in stdout, indicating --help was intercepted.\nStdout:\n{}",
+        stdout
+    );
+    assert!(
+        !stderr.contains("Run Cargo binaries on demand"),
+        "cargox help text appeared in stderr, indicating --help was intercepted.\nStderr:\n{}",
+        stderr
+    );
+
+    // We should see some error about bat not being found/installed
+    // or if it's already on PATH, it would run bat's help
+    // Either way, we shouldn't see cargox's help
+}
+
+/// Test that cargox's own help still works when invoked without a crate spec
+#[test]
+fn test_cargox_help_still_works() {
+    let binary_path = env!("CARGO_BIN_EXE_cargox");
+
+    let output = Command::new(binary_path)
+        .args(["--help"])
+        .output()
+        .expect("Failed to execute cargox");
+
+    let stdout = String::from_utf8_lossy(&output.stdout);
+
+    // When --help comes before the crate spec, it should show cargox's help
+    assert!(
+        stdout.contains("Run Cargo binaries on demand"),
+        "cargox help should appear when --help is used without a crate spec.\nStdout:\n{}",
+        stdout
+    );
+    assert!(output.status.success(), "cargox --help should exit successfully");
+}
+
+/// Test that flags before the crate spec are still parsed by cargox
+#[test]
+fn test_cargox_flags_before_crate_spec() {
+    let binary_path = env!("CARGO_BIN_EXE_cargox");
+
+    // Run: cargox --force bat
+    // The --force flag should be recognized by cargox
+    let output = Command::new(binary_path)
+        .args(["--force", "bat"])
+        .output()
+        .expect("Failed to execute cargox");
+
+    let stderr = String::from_utf8_lossy(&output.stderr);
+
+    // If --force was parsed correctly, cargox would proceed with installation
+    // We should not see cargox's help text
+    assert!(
+        !stderr.contains("Run Cargo binaries on demand"),
+        "cargox help text appeared, indicating argument parsing failed.\nStderr:\n{}",
+        stderr
+    );
+}
+
+/// Test that --bin flag works correctly
+#[test]
+fn test_bin_flag_parsing() {
+    let binary_path = env!("CARGO_BIN_EXE_cargox");
+
+    // Run: cargox --bin custom mycrate --help
+    // The --help should be passed to the binary, not cargox
+    let output = Command::new(binary_path)
+        .args(["--bin", "custom", "mycrate", "--help"])
+        .output()
+        .expect("Failed to execute cargox");
+
+    let stderr = String::from_utf8_lossy(&output.stderr);
+    let stdout = String::from_utf8_lossy(&output.stdout);
+
+    // Should not show cargox help
+    assert!(
+        !stdout.contains("Run Cargo binaries on demand"),
+        "cargox help appeared when it shouldn't.\nStdout:\n{}",
+        stdout
+    );
+    assert!(
+        !stderr.contains("Run Cargo binaries on demand"),
+        "cargox help appeared when it shouldn't.\nStderr:\n{}",
+        stderr
+    );
+}