diff --git a/crates/socket-patch-cli/tests/cli_global_args.rs b/crates/socket-patch-cli/tests/cli_global_args.rs index e2b69c8d9..f7cb2c710 100644 --- a/crates/socket-patch-cli/tests/cli_global_args.rs +++ b/crates/socket-patch-cli/tests/cli_global_args.rs @@ -20,6 +20,8 @@ use clap::Parser; use socket_patch_cli::args::GlobalArgs; use socket_patch_cli::Cli; +#[path = "common/clap_parse.rs"] +mod clap_parse; #[path = "common/hermetic.rs"] mod hermetic; @@ -158,18 +160,15 @@ fn try_parse(subcommand: &str, extra: &[&str]) -> Result { for &arg in extra { argv.push(arg.into()); } - Cli::try_parse_from(&argv) + hermetic::try_parse(&argv.iter().map(String::as_str).collect::>()) } #[test] #[serial_test::serial] fn every_global_flag_parses_on_every_subcommand() { - // Serial + env-isolated: clap validates a field's `env` value during parse - // even when the field is not on the CLI (an invalid `SOCKET_OFFLINE` will - // abort a parse that never mentions `--offline`). So any ambient or - // concurrently-set `SOCKET_*` value can break this matrix — the old - // "CLI args win so it's deterministic" comment was wrong. Clear the slate. - let saved = save_and_clear_global_env(); + // Serial: clap validates a field's `env` value during parse even when the + // field is not on the CLI, so a concurrently-set `SOCKET_*` value from + // the env tests below can break this matrix. let cases = global_flag_cases(); let all_subcommands: Vec<&str> = SUBCOMMANDS_NO_POSITIONAL .iter() @@ -191,13 +190,10 @@ fn every_global_flag_parses_on_every_subcommand() { ) }); // Not just "parsed" — the value must actually land in the - // matching GlobalArgs field on this subcommand. With the env - // cleared above, the only source for the field is the CLI flag. + // matching GlobalArgs field on this subcommand. verify(common_of(&cli)); } } - - restore_global_env(saved); } /// Tripwire: the long-flag matrix in `global_flag_cases()` must have exactly @@ -210,8 +206,7 @@ fn every_global_flag_parses_on_every_subcommand() { #[test] #[serial_test::serial] fn global_flag_cases_cover_every_global_field() { - let saved = save_and_clear_global_env(); - let cli = Cli::try_parse_from(["socket-patch", "list"]).expect("parse"); + let cli = hermetic::try_parse(&["socket-patch", "list"]).expect("parse"); let common = common_of(&cli).clone(); // Exhaustive: every field must be named here. `_`-binding keeps it honest // (we only care that the set of fields matches), and a `..` rest pattern is @@ -253,8 +248,6 @@ fn global_flag_cases_cover_every_global_field() { 27, "every GlobalArgs field needs a long-flag case in global_flag_cases()", ); - - restore_global_env(saved); } /// Tripwire for the tripwire above: the manual field count can be bumped @@ -343,9 +336,7 @@ fn all_subcommands_are_covered() { #[test] #[serial_test::serial] fn every_global_short_form_parses_on_every_subcommand() { - // Serial + env-isolated for the same reason as the long-flag matrix: an - // ambient/concurrent invalid `SOCKET_*` bool would abort these parses. - let saved = save_and_clear_global_env(); + // Serial for the same reason as the long-flag matrix. // (short, value-or-None, verifier) — only flags that actually have a // short. The verifier proves the short maps to the *intended* GlobalArgs // field, not just that it parses (a short silently rebound to a different @@ -388,8 +379,6 @@ fn every_global_short_form_parses_on_every_subcommand() { verify(common_of(&cli)); } } - - restore_global_env(saved); } /// `-d` and `-m` were intentionally dropped (formerly aliases for @@ -400,10 +389,9 @@ fn every_global_short_form_parses_on_every_subcommand() { #[test] #[serial_test::serial] fn reserved_short_forms_are_not_assigned() { - // Env-isolated: an invalid ambient `SOCKET_*` bool would make clap fail + // Serial: an invalid concurrent `SOCKET_*` bool would make clap fail // with ValueValidation *before* it ever reports UnknownArgument for the - // reserved short, turning this assertion into a false positive/negative. - let saved = save_and_clear_global_env(); + // reserved short. let all_subcommands: Vec<&str> = SUBCOMMANDS_NO_POSITIONAL .iter() .chain(SUBCOMMANDS_WITH_IDENTIFIER.iter()) @@ -431,8 +419,6 @@ fn reserved_short_forms_are_not_assigned() { ); } } - - restore_global_env(saved); } /// Locks the env-var bindings: setting a SOCKET_* env var must populate @@ -442,7 +428,6 @@ fn reserved_short_forms_are_not_assigned() { #[test] #[serial_test::serial] fn env_vars_populate_global_args() { - // Save then clear any env vars we set, then verify clap picks them up. let pairs = [ ("SOCKET_CWD", "/env/cwd"), ("SOCKET_MANIFEST_PATH", "env-manifest.json"), @@ -468,18 +453,7 @@ fn env_vars_populate_global_args() { ("SOCKET_TELEMETRY_DISABLED", "true"), ]; - // Save originals. - let saved: Vec<(String, Option)> = pairs - .iter() - .map(|(k, _)| (k.to_string(), std::env::var(k).ok())) - .collect(); - - // Set test values. - for (k, v) in &pairs { - std::env::set_var(k, v); - } - - let cli = Cli::try_parse_from(["socket-patch", "list"]).expect("parse"); + let cli = clap_parse::try_parse_with_env(&pairs, &["socket-patch", "list"]).expect("parse"); if let socket_patch_cli::Commands::List(args) = cli.command { assert_eq!(args.common.cwd, std::path::PathBuf::from("/env/cwd")); assert_eq!(args.common.manifest_path, "env-manifest.json"); @@ -524,24 +498,11 @@ fn env_vars_populate_global_args() { } else { panic!("expected List"); } - - // Restore originals. - for (k, orig) in saved { - match orig { - Some(v) => std::env::set_var(&k, v), - None => std::env::remove_var(&k), - } - } } /// Bool env vars accept "1"/"yes" (the conventional truthy strings), not /// just clap's strict "true"/"false": `SOCKET_OFFLINE=1` (or /// `SOCKET_DEBUG=1`) must not fail with `invalid value '1' for '--offline'`. -/// -/// `#[serial]` because env-var state is process-global; without it -/// these tests race each other (and the existing -/// `env_vars_populate_global_args`) when cargo runs them in -/// parallel. #[test] #[serial_test::serial] fn bool_env_vars_accept_one_and_yes() { @@ -559,15 +520,7 @@ fn bool_env_vars_accept_one_and_yes() { ("SOCKET_TELEMETRY_DISABLED", "1"), ]; - let saved: Vec<(String, Option)> = cases - .iter() - .map(|(k, _)| (k.to_string(), std::env::var(k).ok())) - .collect(); - for (k, v) in cases { - std::env::set_var(k, v); - } - - let cli = Cli::try_parse_from(["socket-patch", "list"]).expect("parse"); + let cli = clap_parse::try_parse_with_env(cases, &["socket-patch", "list"]).expect("parse"); if let socket_patch_cli::Commands::List(args) = cli.command { assert!(args.common.offline, "SOCKET_OFFLINE=1 must parse as true"); assert!(args.common.strict, "SOCKET_STRICT=on must parse as true"); @@ -585,28 +538,14 @@ fn bool_env_vars_accept_one_and_yes() { } else { panic!("expected List"); } - - for (k, orig) in saved { - match orig { - Some(v) => std::env::set_var(&k, v), - None => std::env::remove_var(&k), - } - } } /// Defensive: "0", "false", "no", "off" must NOT engage a bool. Otherwise /// an operator unsetting via `SOCKET_OFFLINE=0` would still get airgap mode /// (and various subtler shell idioms). /// -/// The original version of this test was vacuous: every assertion expected -/// `false`, which is *also* the field default. A regression that dropped the -/// `env = "SOCKET_*"` binding (or replaced `BoolishValueParser` with a parser -/// that silently ignored the var) would leave the fields at their default -/// `false` and the test would stay green — it never actually exercised the -/// env binding. We now first PROVE the binding is live by setting the var -/// truthy and asserting the field flips to `true`; only then is the -/// falsey-resolves-to-false assertion meaningful. Env is fully cleared and -/// isolated per iteration so no leaked `SOCKET_*` value can taint a parse. +/// Each var is first set truthy to prove the binding is live; otherwise the +/// falsey checks would pass on a dead binding, since `false` is the default. #[test] #[serial_test::serial] fn bool_env_vars_reject_zero_and_falsey() { @@ -617,39 +556,29 @@ fn bool_env_vars_reject_zero_and_falsey() { ("SOCKET_JSON", |c| c.json), ]; - let saved = save_and_clear_global_env(); - - let parse_list = || { - let cli = Cli::try_parse_from(["socket-patch", "list"]); - cli.map(|cli| match cli.command { - socket_patch_cli::Commands::List(args) => args.common, - _ => panic!("expected List"), - }) + let parse_list = |var: &str, value: &str| { + let cli = clap_parse::try_parse_with_env(&[(var, value)], &["socket-patch", "list"]) + .unwrap_or_else(|e| panic!("{var}={value} should parse, got: {e}")); + common_of(&cli).clone() }; for &(var, get) in fields { // Liveness proof: a truthy value MUST flip the field to true. If this // fails, the env binding is dead and the falsey checks below would be // vacuous. - std::env::set_var(var, "1"); - let common = parse_list().unwrap_or_else(|e| panic!("{var}=1 should parse: {e}")); assert!( - get(&common), + get(&parse_list(var, "1")), "{var}=1 must engage the bool (proves binding is live)" ); - std::env::remove_var(var); // Each falsey idiom must resolve to false — not true, not a parse error. for falsey in ["0", "false", "no", "off"] { - std::env::set_var(var, falsey); - let common = - parse_list().unwrap_or_else(|e| panic!("{var}={falsey} should parse, got: {e}")); - assert!(!get(&common), "{var}={falsey} must NOT engage the bool"); - std::env::remove_var(var); + assert!( + !get(&parse_list(var, falsey)), + "{var}={falsey} must NOT engage the bool" + ); } } - - restore_global_env(saved); } /// An **empty** boolean env var resolves to `false` — it must NOT crash. @@ -685,12 +614,8 @@ fn empty_bool_env_var_resolves_to_false_not_crash() { }), ]; - let saved = save_and_clear_global_env(); - for (var, accessor) in bool_vars { - std::env::set_var(var, ""); - let result = Cli::try_parse_from(["socket-patch", "list"]); - std::env::remove_var(var); + let result = clap_parse::try_parse_with_env(&[(var, "")], &["socket-patch", "list"]); let cli = result.unwrap_or_else(|e| panic!("{var}= (empty) must parse cleanly, got error: {e}")); @@ -699,16 +624,8 @@ fn empty_bool_env_var_resolves_to_false_not_crash() { "{var}= (empty) must resolve to false", ); } - - restore_global_env(saved); } -/// Every `SOCKET_*` env var that `GlobalArgs` binds, so tests that need a -/// clean slate can save/clear/restore them in one place. This is the -/// production list itself, so it cannot drift from the bound flags; -/// `save_and_clear_covers_every_bound_global_env_var` below pins that. -use socket_patch_cli::args::GLOBAL_ARG_ENV_VARS as GLOBAL_ENV_VARS; - /// An exported-but-**empty** non-bool env var must mean "unset", not crash. /// /// `parse_bool_flag` gave the *bool* globals the empty-means-false semantic, @@ -774,68 +691,6 @@ fn empty_nonbool_env_vars_do_not_crash_the_binary() { ); } -/// `save_and_clear_global_env` must clear **every** env var `GlobalArgs` -/// binds — the production list, not a private copy that can go stale. A var -/// that survives the clear taints every parse in this file: an ambient -/// `SOCKET_STRICT=garbage` in a developer's shell would abort all of them -/// with a `ValueValidation` error that looks nothing like the real cause. -#[test] -#[serial_test::serial] -fn save_and_clear_covers_every_bound_global_env_var() { - use socket_patch_cli::args::GLOBAL_ARG_ENV_VARS; - - // Snapshot the full bound set so this test is hermetic even on failure. - let originals: Vec<(&str, Option)> = GLOBAL_ARG_ENV_VARS - .iter() - .map(|&k| (k, std::env::var(k).ok())) - .collect(); - for &(k, _) in &originals { - std::env::set_var(k, "ambient-poison"); - } - - // Discard the snapshot it returns — it captured the poison values. - let _ = save_and_clear_global_env(); - - let leaked: Vec<&str> = GLOBAL_ARG_ENV_VARS - .iter() - .copied() - .filter(|k| std::env::var(k).is_ok()) - .collect(); - - // Restore before asserting so a failure can't poison later serial tests. - for (k, orig) in originals { - match orig { - Some(v) => std::env::set_var(k, v), - None => std::env::remove_var(k), - } - } - - assert!( - leaked.is_empty(), - "save_and_clear_global_env left GlobalArgs-bound env vars set: {leaked:?}", - ); -} - -fn save_and_clear_global_env() -> Vec<(&'static str, Option)> { - let saved: Vec<(&'static str, Option)> = GLOBAL_ENV_VARS - .iter() - .map(|&k| (k, std::env::var(k).ok())) - .collect(); - for &k in GLOBAL_ENV_VARS { - std::env::remove_var(k); - } - saved -} - -fn restore_global_env(saved: Vec<(&'static str, Option)>) { - for (k, orig) in saved { - match orig { - Some(v) => std::env::set_var(k, v), - None => std::env::remove_var(k), - } - } -} - /// Regression for the documented precedence (`CLI arg > env var > default`, /// see the module header in `args.rs`): when both a CLI flag and its env var /// are set, the CLI value must win. Covers a string field (`--api-url`) and a @@ -844,16 +699,17 @@ fn restore_global_env(saved: Vec<(&'static str, Option)>) { #[test] #[serial_test::serial] fn cli_arg_overrides_env_var() { - let saved = save_and_clear_global_env(); - + let api_url_env = [("SOCKET_API_URL", "https://env-api.example.com")]; // String field: env set, CLI overrides. - std::env::set_var("SOCKET_API_URL", "https://env-api.example.com"); - let cli = Cli::try_parse_from([ - "socket-patch", - "list", - "--api-url", - "https://cli-api.example.com", - ]) + let cli = clap_parse::try_parse_with_env( + &api_url_env, + &[ + "socket-patch", + "list", + "--api-url", + "https://cli-api.example.com", + ], + ) .expect("parse"); let socket_patch_cli::Commands::List(args) = cli.command else { panic!("expected List"); @@ -865,7 +721,8 @@ fn cli_arg_overrides_env_var() { ); // Sanity: with the CLI flag absent, the env value resolves through. - let cli = Cli::try_parse_from(["socket-patch", "list"]).expect("parse"); + let cli = + clap_parse::try_parse_with_env(&api_url_env, &["socket-patch", "list"]).expect("parse"); let socket_patch_cli::Commands::List(args) = cli.command else { panic!("expected List"); }; @@ -876,8 +733,11 @@ fn cli_arg_overrides_env_var() { ); // Bool field: CLI `--offline` wins over a falsey env value. - std::env::set_var("SOCKET_OFFLINE", "0"); - let cli = Cli::try_parse_from(["socket-patch", "list", "--offline"]).expect("parse"); + let cli = clap_parse::try_parse_with_env( + &[("SOCKET_OFFLINE", "0")], + &["socket-patch", "list", "--offline"], + ) + .expect("parse"); let socket_patch_cli::Commands::List(args) = cli.command else { panic!("expected List"); }; @@ -885,8 +745,6 @@ fn cli_arg_overrides_env_var() { args.common.offline, "CLI --offline must win over SOCKET_OFFLINE=0" ); - - restore_global_env(saved); } /// Regression: with neither CLI flags nor env vars set, clap must populate the @@ -898,28 +756,15 @@ fn cli_arg_overrides_env_var() { #[test] #[serial_test::serial] fn production_defaults_populate_when_unset() { - let saved = save_and_clear_global_env(); - - let cli = Cli::try_parse_from(["socket-patch", "list"]).expect("parse"); + let cli = hermetic::try_parse(&["socket-patch", "list"]).expect("parse"); let socket_patch_cli::Commands::List(args) = cli.command else { panic!("expected List"); }; let c = &args.common; - assert_eq!(c.cwd, std::path::PathBuf::from(".")); - assert_eq!(c.manifest_path, ".socket/manifest.json"); - assert_eq!(c.api_url, None, "no clap default — resolved in core"); - assert_eq!(c.proxy_url, None, "no clap default — resolved in core"); - assert_eq!(c.vendor_source, "service"); - assert!(c.vendor_url.is_none()); - assert!(c.patch_server_url.is_none()); - assert!(c.api_token.is_none()); - assert!(c.org.is_none()); - assert!(c.ecosystems.is_none()); - assert!(!c.strict); - assert!(!c.offline && !c.global && !c.json && !c.verbose && !c.silent); - assert!(!c.dry_run && !c.yes && !c.debug && !c.no_telemetry); - assert!(c.lock_timeout.is_none()); - assert!(c.global_prefix.is_none()); + assert_eq!( + clap_parse::Globals(c.clone()), + clap_parse::global_defaults() + ); // With no flag and no env var the overrides stay `None`, so the core // resolver (env → socket-cli config → documented default) decides. @@ -928,6 +773,4 @@ fn production_defaults_populate_when_unset() { assert!(o.proxy_url.is_none(), "unset --proxy-url must not override"); assert!(o.api_token.is_none()); assert!(o.org_slug.is_none()); - - restore_global_env(saved); } diff --git a/crates/socket-patch-cli/tests/cli_parse_get.rs b/crates/socket-patch-cli/tests/cli_parse_get.rs index f856580cc..e48849df2 100644 --- a/crates/socket-patch-cli/tests/cli_parse_get.rs +++ b/crates/socket-patch-cli/tests/cli_parse_get.rs @@ -5,148 +5,35 @@ //! removed in v5 and must stay parse errors. Changing any assertion here is a //! breaking change to the CLI surface — see //! `crates/socket-patch-cli/CLI_CONTRACT.md`. -//! -//! ## Hermeticity -//! -//! Every flag and default below is also wired to an `#[arg(env = "SOCKET_*")]` -//! source. clap reads those env vars during `try_parse_from`, so an ambient -//! `SOCKET_*` variable in the developer's shell or in CI would silently -//! satisfy these assertions even if the corresponding CLI default -//! (`default_value`/`default_value_t`) regressed or a flag's action broke — -//! the env value would mask the bug and the test would pass for the wrong -//! reason. To make the assertions test *argv parsing* rather than the -//! ambient environment, every parse runs with the full set of `SOCKET_*` -//! vars scrubbed (see [`EnvScrub`]). Because the environment is process- -//! global, every test is `#[serial_test::serial]` so the scrub/restore -//! dance can't race a concurrent parse. - -use clap::Parser; + use socket_patch_cli::commands::get::GetArgs; -use socket_patch_cli::{Cli, Commands}; +use socket_patch_cli::Commands; use std::path::PathBuf; -/// Every `SOCKET_*` env var that clap consults while parsing `get` (its own -/// flags plus the flattened `GlobalArgs`). If any of these leaks in from the -/// ambient environment it can mask a broken default or a regressed flag, so -/// the parse helpers below remove them for the duration of the parse. -const SOCKET_ENV_VARS: &[&str] = &[ - // GlobalArgs - "SOCKET_CWD", - "SOCKET_MANIFEST_PATH", - "SOCKET_API_URL", - "SOCKET_API_TOKEN", - "SOCKET_ORG_SLUG", - "SOCKET_PROXY_URL", - "SOCKET_ECOSYSTEMS", - "SOCKET_VENDOR_SOURCE", - "SOCKET_VENDOR_URL", - "SOCKET_PATCH_SERVER_URL", - "SOCKET_OFFLINE", - "SOCKET_STRICT", - "SOCKET_GLOBAL", - "SOCKET_GLOBAL_PREFIX", - "SOCKET_JSON", - "SOCKET_VERBOSE", - "SOCKET_SILENT", - "SOCKET_DRY_RUN", - "SOCKET_YES", - "SOCKET_LOCK_TIMEOUT", - "SOCKET_DEBUG", - "SOCKET_TELEMETRY_DISABLED", - "SOCKET_NO_TRUST_LOCKFILE_CONFIG", - "SOCKET_NO_NPM_ALLOW_REMOTE_CONFIG", - "SOCKET_NO_VLT_INSTALL_CLEANUP", - // GetArgs-specific - "SOCKET_SAVE_ONLY", - "SOCKET_ALL_RELEASES", -]; - -/// RAII guard that removes every [`SOCKET_ENV_VARS`] entry on construction and -/// restores the prior value on drop. Holding one of these around a clap parse -/// guarantees the parse sees only what's on the argv, not the developer's -/// shell. Pair with `#[serial_test::serial]` so the global env mutation never -/// races another test. -struct EnvScrub(Vec<(&'static str, Option)>); - -impl EnvScrub { - fn new() -> Self { - let saved = SOCKET_ENV_VARS - .iter() - .map(|&k| { - let prev = std::env::var(k).ok(); - std::env::remove_var(k); - (k, prev) - }) - .collect(); - EnvScrub(saved) - } -} +#[path = "common/clap_parse.rs"] +mod clap_parse; +#[path = "common/hermetic.rs"] +mod hermetic; -impl Drop for EnvScrub { - fn drop(&mut self) { - for (k, v) in &self.0 { - match v { - Some(val) => std::env::set_var(k, val), - None => std::env::remove_var(k), - } - } - } -} +use clap_parse::Globals; +use hermetic::try_parse; -/// Parse `socket-patch get ` and return the `GetArgs`, with the -/// ambient `SOCKET_*` environment scrubbed so the result reflects only the -/// argv. The scrub guard is held across the parse and dropped before the -/// caller's assertions run (which only inspect the returned struct). fn parse_get(extra: &[&str]) -> GetArgs { - let _scrub = EnvScrub::new(); let mut argv = vec!["socket-patch", "get"]; argv.extend_from_slice(extra); - let cli = Cli::try_parse_from(&argv).expect("parse"); + let cli = try_parse(&argv).expect("parse"); match cli.command { Commands::Get(a) => a, _ => panic!("expected Get"), } } -/// Owned, comparable snapshot of *every* parsed field in `GetArgs` — its own -/// flags plus every field of the flattened `GlobalArgs`. `GetArgs` itself does -/// not derive `PartialEq` (it's production code we may not touch), so this -/// mirror exists purely so a single `assert_eq!` can police the entire parsed -/// surface at once. -/// -/// This is what makes the per-flag tests honest. A field-at-a-time assertion -/// (`assert!(a.package)`) only proves the flag set *its* field; it says nothing -/// about whether the same flag also flipped an unrelated one. A clap-derive -/// copy/paste regression (e.g. `--package` accidentally wired to `save_only`) -/// would set both and still pass a single-field check. Comparing the whole -/// snapshot against the independently-declared defaults — with only the field -/// under test mutated — fails loudly the instant any other field moves. +/// Every parsed field of `GetArgs`. The per-flag tests compare the whole +/// snapshot, so a flag wired to a second field fails too. #[derive(Debug, Clone, PartialEq)] struct Snap { identifier: String, - cwd: PathBuf, - manifest_path: String, - api_url: Option, - api_token: Option, - org: Option, - proxy_url: Option, - ecosystems: Option>, - vendor_source: String, - vendor_url: Option, - patch_server_url: Option, - offline: bool, - strict: bool, - allow_patch_conflicts: bool, - global: bool, - global_prefix: Option, - json: bool, - verbose: bool, - silent: bool, - dry_run: bool, - yes: bool, - lock_timeout: Option, - debug: bool, - no_telemetry: bool, + common: Globals, id: bool, cve: bool, ghsa: bool, @@ -159,29 +46,7 @@ struct Snap { fn snapshot(a: &GetArgs) -> Snap { Snap { identifier: a.identifier.clone(), - cwd: a.common.cwd.clone(), - manifest_path: a.common.manifest_path.clone(), - api_url: a.common.api_url.clone(), - api_token: a.common.api_token.clone(), - org: a.common.org.clone(), - proxy_url: a.common.proxy_url.clone(), - ecosystems: a.common.ecosystems.clone(), - vendor_source: a.common.vendor_source.clone(), - vendor_url: a.common.vendor_url.clone(), - patch_server_url: a.common.patch_server_url.clone(), - offline: a.common.offline, - strict: a.common.strict, - allow_patch_conflicts: a.common.allow_patch_conflicts, - global: a.common.global, - global_prefix: a.common.global_prefix.clone(), - json: a.common.json, - verbose: a.common.verbose, - silent: a.common.silent, - dry_run: a.common.dry_run, - yes: a.common.yes, - lock_timeout: a.common.lock_timeout, - debug: a.common.debug, - no_telemetry: a.common.no_telemetry, + common: Globals(a.common.clone()), id: a.id, cve: a.cve, ghsa: a.ghsa, @@ -192,39 +57,11 @@ fn snapshot(a: &GetArgs) -> Snap { } } -/// Independent oracle: the snapshot a correct parse of `get ` (with -/// no other flags) must produce. The values are transcribed by hand from the -/// `default_value`/`default_value_t` declarations on `GetArgs`/`GlobalArgs` and -/// the `DEFAULT_*` constants in `socket-patch-core` — NOT read back from a live -/// parse — so this can actually disagree with the implementation if a default -/// regresses. Every per-flag test starts from this and mutates exactly the one -/// field the flag is supposed to touch. +/// The snapshot of `get ` with no other flag, written by hand. fn expected_defaults(identifier: &str) -> Snap { Snap { identifier: identifier.to_string(), - cwd: PathBuf::from("."), - manifest_path: ".socket/manifest.json".to_string(), - api_url: None, // no clap default — resolved in core - api_token: None, - org: None, - proxy_url: None, // no clap default — resolved in core - ecosystems: None, - vendor_source: "service".to_string(), - vendor_url: None, - patch_server_url: None, - offline: false, - strict: false, - allow_patch_conflicts: false, - global: false, - global_prefix: None, - json: false, - verbose: false, - silent: false, - dry_run: false, - yes: false, - lock_timeout: None, - debug: false, - no_telemetry: false, + common: clap_parse::global_defaults(), id: false, cve: false, ghsa: false, @@ -238,32 +75,22 @@ fn expected_defaults(identifier: &str) -> Snap { // --- Defaults ---------------------------------------------------------------- #[test] -#[serial_test::serial] fn defaults_with_only_required_identifier() { let a = parse_get(&["some-id"]); - // Pin the *entire* default surface in one shot against the independent - // oracle. This covers every field (manifest_path, - // proxy_url, offline, verbose, silent, dry_run, lock_timeout, - // debug, no_telemetry, ecosystems) — any of which could regress to a - // non-default and go unnoticed under a field-cherry-picked assertion. assert_eq!(snapshot(&a), expected_defaults("some-id")); } #[test] -#[serial_test::serial] fn all_releases_flag_sets_all_releases() { let a = parse_get(&["some-id", "--all-releases"]); let mut want = expected_defaults("some-id"); want.all_releases = true; - // Full-snapshot equality: proves the flag set `all_releases` AND left every - // other field at its default (env scrubbed, so the `true` is the flag's). assert_eq!(snapshot(&a), want); } // --- Positional -------------------------------------------------------------- #[test] -#[serial_test::serial] fn positional_identifier_stored() { let a = parse_get(&["pkg:npm/foo@1.0"]); // The positional lands in `identifier` and nothing else shifts. @@ -273,7 +100,6 @@ fn positional_identifier_stored() { // --- Short flags ------------------------------------------------------------- #[test] -#[serial_test::serial] fn short_p_sets_package() { let a = parse_get(&["some-id", "-p"]); let mut want = expected_defaults("some-id"); @@ -282,7 +108,6 @@ fn short_p_sets_package() { } #[test] -#[serial_test::serial] fn long_package_sets_package() { let a = parse_get(&["some-id", "--package"]); let mut want = expected_defaults("some-id"); @@ -291,63 +116,56 @@ fn long_package_sets_package() { } #[test] -#[serial_test::serial] fn short_y_sets_yes() { let a = parse_get(&["some-id", "-y"]); let mut want = expected_defaults("some-id"); - want.yes = true; + want.common.yes = true; assert_eq!(snapshot(&a), want); } #[test] -#[serial_test::serial] fn long_yes_sets_yes() { let a = parse_get(&["some-id", "--yes"]); let mut want = expected_defaults("some-id"); - want.yes = true; + want.common.yes = true; assert_eq!(snapshot(&a), want); } #[test] -#[serial_test::serial] fn short_g_sets_global() { let a = parse_get(&["some-id", "-g"]); let mut want = expected_defaults("some-id"); - want.global = true; + want.common.global = true; assert_eq!(snapshot(&a), want); } #[test] -#[serial_test::serial] fn long_global_sets_global() { let a = parse_get(&["some-id", "--global"]); let mut want = expected_defaults("some-id"); - want.global = true; + want.common.global = true; assert_eq!(snapshot(&a), want); } // --- Long-only flags --------------------------------------------------------- #[test] -#[serial_test::serial] fn cwd_flag_sets_cwd() { let a = parse_get(&["some-id", "--cwd", "/tmp/project"]); let mut want = expected_defaults("some-id"); - want.cwd = PathBuf::from("/tmp/project"); + want.common.cwd = PathBuf::from("/tmp/project"); assert_eq!(snapshot(&a), want); } #[test] -#[serial_test::serial] fn org_flag_sets_org() { let a = parse_get(&["some-id", "--org", "acme"]); let mut want = expected_defaults("some-id"); - want.org = Some("acme".to_string()); + want.common.org = Some("acme".to_string()); assert_eq!(snapshot(&a), want); } #[test] -#[serial_test::serial] fn id_flag_sets_id() { let a = parse_get(&["some-id", "--id"]); let mut want = expected_defaults("some-id"); @@ -356,7 +174,6 @@ fn id_flag_sets_id() { } #[test] -#[serial_test::serial] fn cve_flag_sets_cve() { let a = parse_get(&["some-id", "--cve"]); let mut want = expected_defaults("some-id"); @@ -365,7 +182,6 @@ fn cve_flag_sets_cve() { } #[test] -#[serial_test::serial] fn ghsa_flag_sets_ghsa() { let a = parse_get(&["some-id", "--ghsa"]); let mut want = expected_defaults("some-id"); @@ -374,47 +190,42 @@ fn ghsa_flag_sets_ghsa() { } #[test] -#[serial_test::serial] fn api_url_flag_sets_api_url() { let a = parse_get(&["some-id", "--api-url", "https://api.example.com"]); let mut want = expected_defaults("some-id"); - want.api_url = Some("https://api.example.com".to_string()); + want.common.api_url = Some("https://api.example.com".to_string()); assert_eq!(snapshot(&a), want); } #[test] -#[serial_test::serial] fn api_token_flag_sets_api_token() { let a = parse_get(&["some-id", "--api-token", "sktsec_abc"]); let mut want = expected_defaults("some-id"); - want.api_token = Some("sktsec_abc".to_string()); + want.common.api_token = Some("sktsec_abc".to_string()); assert_eq!(snapshot(&a), want); } #[test] -#[serial_test::serial] fn global_prefix_flag_sets_global_prefix() { let a = parse_get(&["some-id", "--global-prefix", "/usr/local/lib"]); let mut want = expected_defaults("some-id"); - want.global_prefix = Some(PathBuf::from("/usr/local/lib")); + want.common.global_prefix = Some(PathBuf::from("/usr/local/lib")); // `--global-prefix` must NOT imply `--global`; full-snapshot equality keeps // `global` pinned at its default. assert_eq!(snapshot(&a), want); } #[test] -#[serial_test::serial] fn json_flag_sets_json() { let a = parse_get(&["some-id", "--json"]); let mut want = expected_defaults("some-id"); - want.json = true; + want.common.json = true; assert_eq!(snapshot(&a), want); } // --- save-only ------------------------------------------------------------------ #[test] -#[serial_test::serial] fn save_only_flag_sets_save_only() { let a = parse_get(&["some-id", "--save-only"]); let mut want = expected_defaults("some-id"); @@ -423,12 +234,10 @@ fn save_only_flag_sets_save_only() { } #[test] -#[serial_test::serial] fn removed_no_apply_alias_is_a_usage_error() { // v5 removed the hidden `--no-apply` alias; `--save-only` (or // SOCKET_SAVE_ONLY) is the only spelling. - let _scrub = EnvScrub::new(); - let err = match Cli::try_parse_from(["socket-patch", "get", "some-id", "--no-apply"]) { + let err = match try_parse(&["socket-patch", "get", "some-id", "--no-apply"]) { Ok(_) => panic!("--no-apply should no longer parse"), Err(e) => e, }; @@ -445,11 +254,8 @@ fn removed_no_apply_alias_is_a_usage_error() { // enforced inside `run()`, not by clap — pinned in get_modes_e2e.rs). // The hidden VALUE aliases (`host`/`redirect` for hosted, `vendor` for // vendored) ride along from the shared enum and are part of the contract. -// `--mode` deliberately has no `SOCKET_*` env binding (matching scan), so -// no scrub-list entry is needed. #[test] -#[serial_test::serial] fn mode_defaults_to_none() { // No `--mode` → `None` (agent behavior). Whole-snapshot: the absent // flag must leave every field at its default, `mode` included. @@ -459,7 +265,6 @@ fn mode_defaults_to_none() { } #[test] -#[serial_test::serial] fn mode_hosted_parses() { let a = parse_get(&["some-id", "--mode", "hosted"]); let mut want = expected_defaults("some-id"); @@ -471,7 +276,6 @@ fn mode_hosted_parses() { } #[test] -#[serial_test::serial] fn mode_vendored_parses() { let a = parse_get(&["some-id", "--mode", "vendored"]); let mut want = expected_defaults("some-id"); @@ -480,7 +284,6 @@ fn mode_vendored_parses() { } #[test] -#[serial_test::serial] fn mode_agent_parses() { let a = parse_get(&["some-id", "--mode", "agent"]); let mut want = expected_defaults("some-id"); @@ -489,13 +292,11 @@ fn mode_agent_parses() { } #[test] -#[serial_test::serial] fn removed_mode_value_aliases_are_rejected() { // v5.0 dropped the hidden `host` / `redirect` / `vendor` value aliases // (shared with `scan --mode`); only the canonical names parse. - let _scrub = EnvScrub::new(); for alias in ["host", "redirect", "vendor"] { - let err = match Cli::try_parse_from(["socket-patch", "get", "some-id", "--mode", alias]) { + let err = match try_parse(&["socket-patch", "get", "some-id", "--mode", alias]) { Ok(_) => panic!("--mode {alias} should fail to parse"), Err(e) => e, }; @@ -511,14 +312,12 @@ fn removed_mode_value_aliases_are_rejected() { } #[test] -#[serial_test::serial] fn mode_rejects_unknown_value() { // The shared value_enum restricts `--mode` to the three known names. // `apply` is not a scan-mode name anywhere, so it must be rejected // exactly like a bogus value. - let _scrub = EnvScrub::new(); for bad in ["bogus", "apply"] { - let err = match Cli::try_parse_from(["socket-patch", "get", "some-id", "--mode", bad]) { + let err = match try_parse(&["socket-patch", "get", "some-id", "--mode", bad]) { Ok(_) => panic!("--mode {bad} should fail to parse"), Err(e) => e, }; @@ -536,11 +335,9 @@ fn mode_rejects_unknown_value() { // --- removed `download` alias ------------------------------------------------- #[test] -#[serial_test::serial] fn removed_download_alias_is_a_usage_error() { // v5 removed the `download` alias for `get`. - let _scrub = EnvScrub::new(); - let err = match Cli::try_parse_from(["socket-patch", "download", "some-id"]) { + let err = match try_parse(&["socket-patch", "download", "some-id"]) { Ok(_) => panic!("`download` should no longer parse"), Err(e) => e, }; @@ -550,10 +347,8 @@ fn removed_download_alias_is_a_usage_error() { // --- Error paths ------------------------------------------------------------- #[test] -#[serial_test::serial] fn missing_required_identifier_errors() { - let _scrub = EnvScrub::new(); - let err = match Cli::try_parse_from(["socket-patch", "get"]) { + let err = match try_parse(&["socket-patch", "get"]) { Err(e) => e, Ok(_) => panic!("expected parse error for missing required positional"), }; @@ -561,10 +356,8 @@ fn missing_required_identifier_errors() { } #[test] -#[serial_test::serial] fn unknown_flag_errors() { - let _scrub = EnvScrub::new(); - let err = match Cli::try_parse_from(["socket-patch", "get", "some-id", "--bogus"]) { + let err = match try_parse(&["socket-patch", "get", "some-id", "--bogus"]) { Err(e) => e, Ok(_) => panic!("expected parse error for unknown flag"), }; @@ -572,65 +365,19 @@ fn unknown_flag_errors() { } #[test] -#[serial_test::serial] fn removed_one_off_flag_is_unknown() { - let _scrub = EnvScrub::new(); - let err = match Cli::try_parse_from(["socket-patch", "get", "some-id", "--one-off"]) { + let err = match try_parse(&["socket-patch", "get", "some-id", "--one-off"]) { Err(e) => e, Ok(_) => panic!("expected parse error for the v5-removed --one-off"), }; assert_eq!(err.kind(), clap::error::ErrorKind::UnknownArgument); } -// --- Hermeticity of the scrub itself ------------------------------------------- - -#[test] -#[serial_test::serial] -fn scrub_covers_every_global_env_var_clap_consults() { - // [`SOCKET_ENV_VARS`] claims to list "every SOCKET_* env var that clap - // consults while parsing `get`". `GlobalArgs` is flattened in whole, so - // the production `GLOBAL_ARG_ENV_VARS` list is the oracle — a flag added - // to `GlobalArgs` with an env binding is consulted here the moment it - // lands, and if the scrub list lags behind, an ambient value either - // aborts every parse in this file (validated flags: bools, ints, - // `--ecosystems`, `--vendor-source`) or silently leaks into the parsed - // args (string flags), voiding the hermeticity the module doc promises. - // `garbage` is rejected by every validating parser and visibly non-default - // for every string/path/option flag, so a missing scrub entry fails - // loudly either way. - for &var in socket_patch_cli::args::GLOBAL_ARG_ENV_VARS { - let prev = std::env::var(var).ok(); - std::env::set_var(var, "garbage"); - let parsed = { - let _scrub = EnvScrub::new(); - Cli::try_parse_from(["socket-patch", "get", "some-id"]) - }; - match prev { - Some(v) => std::env::set_var(var, v), - None => std::env::remove_var(var), - } - let a = match parsed { - Ok(cli) => match cli.command { - Commands::Get(a) => a, - _ => panic!("expected Get"), - }, - Err(e) => panic!("ambient {var}=garbage aborted the scrubbed parse: {e}"), - }; - assert_eq!( - snapshot(&a), - expected_defaults("some-id"), - "ambient {var}=garbage leaked into the scrubbed parse", - ); - } -} - #[test] -#[serial_test::serial] fn no_vlt_install_cleanup_flag_parses_for_hosted_get() { - assert!(!parse_get(&["some-id"]).common.no_vlt_install_cleanup); - assert!( - parse_get(&["some-id", "--mode", "hosted", "--no-vlt-install-cleanup"]) - .common - .no_vlt_install_cleanup - ); + let a = parse_get(&["some-id", "--mode", "hosted", "--no-vlt-install-cleanup"]); + let mut want = expected_defaults("some-id"); + want.mode = Some(socket_patch_cli::commands::scan::ScanMode::Hosted); + want.common.no_vlt_install_cleanup = true; + assert_eq!(snapshot(&a), want); } diff --git a/crates/socket-patch-cli/tests/cli_parse_list.rs b/crates/socket-patch-cli/tests/cli_parse_list.rs index e5c5d7682..f4e1883c3 100644 --- a/crates/socket-patch-cli/tests/cli_parse_list.rs +++ b/crates/socket-patch-cli/tests/cli_parse_list.rs @@ -13,13 +13,16 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; -use clap::Parser; use socket_patch_cli::commands::list::{run, ListArgs}; +#[path = "common/clap_parse.rs"] +mod clap_parse; #[path = "common/mod.rs"] mod common; -use common::hermetic::binary_command; -use socket_patch_cli::{Cli, Commands}; +use clap_parse::try_parse_keeping_opt_outs as try_parse; +use common::hermetic; +use hermetic::binary_command; +use socket_patch_cli::Commands; use socket_patch_core::manifest::schema::{ PatchFileInfo, PatchManifest, PatchRecord, VulnerabilityInfo, }; @@ -31,7 +34,7 @@ use socket_patch_core::manifest::schema::{ fn parse_list(extra: &[&str]) -> ListArgs { let mut argv = vec!["socket-patch", "list"]; argv.extend_from_slice(extra); - let cli = Cli::try_parse_from(&argv).expect("parse"); + let cli = try_parse(&argv).expect("parse"); match cli.command { Commands::List(a) => a, _ => panic!("expected List"), @@ -44,10 +47,12 @@ fn parse_list(extra: &[&str]) -> ListArgs { #[test] fn defaults_match_contract() { + // The `run()` tests below need the forced opt-outs, so parses here see + // `SOCKET_TELEMETRY_DISABLED=1`. let args = parse_list(&[]); - assert_eq!(args.common.cwd, PathBuf::from(".")); - assert_eq!(args.common.manifest_path, ".socket/manifest.json"); - assert!(!args.common.json); + let mut want = clap_parse::global_defaults(); + want.no_telemetry = true; + assert_eq!(clap_parse::Globals(args.common), want); } #[test] @@ -70,7 +75,7 @@ fn json_flag_sets_true() { #[test] fn unknown_flag_is_rejected() { - let err = match Cli::try_parse_from(["socket-patch", "list", "--nope"]) { + let err = match try_parse(&["socket-patch", "list", "--nope"]) { Ok(_) => panic!("unknown flag must fail"), Err(e) => e, }; diff --git a/crates/socket-patch-cli/tests/cli_parse_main.rs b/crates/socket-patch-cli/tests/cli_parse_main.rs index 9310736fa..3109d214a 100644 --- a/crates/socket-patch-cli/tests/cli_parse_main.rs +++ b/crates/socket-patch-cli/tests/cli_parse_main.rs @@ -23,6 +23,7 @@ mod hermetic; /// the real CLI. Routing through the wrapper means each name/alias/error-kind /// assertion below also exercises the code path users actually hit. fn parse(argv: &[&str]) -> Result { + hermetic::scrub_process_socket_env(); parse_argv_with_shortcuts(argv.iter().map(|s| s.to_string()).collect()) } diff --git a/crates/socket-patch-cli/tests/cli_parse_remove.rs b/crates/socket-patch-cli/tests/cli_parse_remove.rs index ee4db08b8..b836d922d 100644 --- a/crates/socket-patch-cli/tests/cli_parse_remove.rs +++ b/crates/socket-patch-cli/tests/cli_parse_remove.rs @@ -7,24 +7,36 @@ //! These tests deliberately avoid spawning the binary so they run in the //! default `cargo test` set (no `--ignored` required) and stay fast. -use clap::Parser; use socket_patch_cli::commands::remove::{run, RemoveArgs}; -use socket_patch_cli::{Cli, Commands}; +use socket_patch_cli::Commands; use std::path::PathBuf; +#[path = "common/clap_parse.rs"] +mod clap_parse; #[path = "common/hermetic.rs"] mod hermetic; +use clap_parse::try_parse_keeping_opt_outs as try_parse; +use clap_parse::{global_defaults, Globals}; + fn parse_remove(extra: &[&str]) -> RemoveArgs { let mut argv = vec!["socket-patch", "remove"]; argv.extend_from_slice(extra); - let cli = Cli::try_parse_from(&argv).expect("parse"); + let cli = try_parse(&argv).expect("parse"); match cli.command { Commands::Remove(a) => a, _ => panic!("expected Remove"), } } +/// The `run()` tests below need the forced opt-outs, so parses here see +/// `SOCKET_TELEMETRY_DISABLED=1`. +fn opt_out_defaults() -> Globals { + let mut want = global_defaults(); + want.no_telemetry = true; + want +} + // --------------------------------------------------------------------------- // Defaults // --------------------------------------------------------------------------- @@ -33,13 +45,8 @@ fn parse_remove(extra: &[&str]) -> RemoveArgs { fn defaults_with_purl_positional() { let args = parse_remove(&["pkg:npm/foo@1"]); assert_eq!(args.identifier, "pkg:npm/foo@1"); - assert_eq!(args.common.cwd, PathBuf::from(".")); - assert_eq!(args.common.manifest_path, ".socket/manifest.json"); assert!(!args.skip_rollback); - assert!(!args.common.yes); - assert!(!args.common.global); - assert_eq!(args.common.global_prefix, None); - assert!(!args.common.json); + assert_eq!(Globals(args.common), opt_out_defaults()); } #[test] @@ -49,13 +56,8 @@ fn positional_uuid_stored_in_identifier() { // Everything else still at default — `remove` does not auto-detect the // identifier shape at parse time; the runtime branch on `pkg:` happens // inside `run()`. - assert_eq!(args.common.cwd, PathBuf::from(".")); - assert_eq!(args.common.manifest_path, ".socket/manifest.json"); assert!(!args.skip_rollback); - assert!(!args.common.yes); - assert!(!args.common.global); - assert_eq!(args.common.global_prefix, None); - assert!(!args.common.json); + assert_eq!(Globals(args.common), opt_out_defaults()); } // --------------------------------------------------------------------------- @@ -153,7 +155,7 @@ fn all_flags_combined() { #[test] fn missing_required_positional_is_error() { - let result = Cli::try_parse_from(["socket-patch", "remove"]); + let result = try_parse(&["socket-patch", "remove"]); let err = match result { Ok(_) => panic!("remove without identifier must fail"), Err(e) => e, @@ -163,7 +165,7 @@ fn missing_required_positional_is_error() { #[test] fn unknown_flag_is_error() { - let result = Cli::try_parse_from([ + let result = try_parse(&[ "socket-patch", "remove", "pkg:npm/foo@1", diff --git a/crates/socket-patch-cli/tests/cli_parse_repair.rs b/crates/socket-patch-cli/tests/cli_parse_repair.rs index 554dd42bb..4e7270787 100644 --- a/crates/socket-patch-cli/tests/cli_parse_repair.rs +++ b/crates/socket-patch-cli/tests/cli_parse_repair.rs @@ -6,206 +6,51 @@ //! //! See `crates/socket-patch-cli/CLI_CONTRACT.md` for the full repair table. //! -//! ## Hermeticity -//! -//! Every flag and default below is also wired to an `#[arg(env = "SOCKET_*")]` -//! source. clap reads those env vars during `try_parse_from`, so an ambient -//! `SOCKET_*` variable in the developer's shell or in CI would silently -//! satisfy these assertions even if the corresponding CLI default -//! (`default_value`/`default_value_t`) regressed or a flag's action broke — -//! the env value would mask the bug and the test would pass for the wrong -//! reason (e.g. an exported `SOCKET_DOWNLOAD_ONLY=true` keeps a flag -//! assertion green even if the flag's action broke). -//! To make the assertions test *argv parsing* rather than the ambient -//! environment, every parse runs with the full set of `SOCKET_*` vars scrubbed -//! (see [`EnvScrub`]). Because the environment is process-global, every test is -//! `#[serial_test::serial]` so the scrub/restore dance can't race a concurrent -//! parse. This mirrors the hardening in `cli_parse_get.rs`. +//! Two tests set `SOCKET_DOWNLOAD_ONLY`, and clap reads it during every parse, +//! so every test that parses is `#[serial]`. use std::path::PathBuf; -use clap::Parser; use socket_patch_cli::commands::repair::RepairArgs; -use socket_patch_cli::{Cli, Commands}; +use socket_patch_cli::Commands; -/// Every `SOCKET_*` env var that clap consults while parsing `repair` (its own -/// `--download-only` flag plus the flattened `GlobalArgs`). If any leaks in -/// from the ambient environment it can mask a broken default or a regressed -/// flag, so the parse helpers below remove them for the duration of the parse. -const SOCKET_ENV_VARS: &[&str] = &[ - // GlobalArgs - "SOCKET_CWD", - "SOCKET_MANIFEST_PATH", - "SOCKET_API_URL", - "SOCKET_API_TOKEN", - "SOCKET_ORG_SLUG", - "SOCKET_PROXY_URL", - "SOCKET_ECOSYSTEMS", - "SOCKET_VENDOR_SOURCE", - "SOCKET_VENDOR_URL", - "SOCKET_PATCH_SERVER_URL", - "SOCKET_OFFLINE", - "SOCKET_STRICT", - "SOCKET_GLOBAL", - "SOCKET_GLOBAL_PREFIX", - "SOCKET_JSON", - "SOCKET_VERBOSE", - "SOCKET_SILENT", - "SOCKET_DRY_RUN", - "SOCKET_YES", - "SOCKET_LOCK_TIMEOUT", - "SOCKET_DEBUG", - "SOCKET_TELEMETRY_DISABLED", - "SOCKET_NO_TRUST_LOCKFILE_CONFIG", - "SOCKET_NO_NPM_ALLOW_REMOTE_CONFIG", - "SOCKET_NO_VLT_INSTALL_CLEANUP", - // RepairArgs-specific - "SOCKET_DOWNLOAD_ONLY", -]; +#[path = "common/clap_parse.rs"] +mod clap_parse; +#[path = "common/hermetic.rs"] +mod hermetic; -/// RAII guard that removes every [`SOCKET_ENV_VARS`] entry on construction and -/// restores the prior value on drop. Holding one of these around a clap parse -/// guarantees the parse sees only what's on the argv, not the developer's -/// shell. Pair with `#[serial_test::serial]` so the global env mutation never -/// races another test. -struct EnvScrub(Vec<(&'static str, Option)>); - -impl EnvScrub { - fn new() -> Self { - let saved = SOCKET_ENV_VARS - .iter() - .map(|&k| { - let prev = std::env::var(k).ok(); - std::env::remove_var(k); - (k, prev) - }) - .collect(); - EnvScrub(saved) - } -} - -impl Drop for EnvScrub { - fn drop(&mut self) { - for (k, v) in &self.0 { - match v { - Some(val) => std::env::set_var(k, val), - None => std::env::remove_var(k), - } - } - } -} +use clap_parse::{try_parse_with_env, Globals}; +use hermetic::try_parse; fn parse_repair(extra: &[&str]) -> RepairArgs { - let _scrub = EnvScrub::new(); let mut argv = vec!["socket-patch", "repair"]; argv.extend_from_slice(extra); - let cli = Cli::try_parse_from(&argv).expect("parse"); + let cli = try_parse(&argv).expect("parse"); match cli.command { Commands::Repair(a) => a, _ => panic!("expected Repair"), } } -/// Owned, comparable snapshot of *every* parsed field in `RepairArgs` — its own -/// `download_only` flag plus every field of the flattened `GlobalArgs`. -/// `RepairArgs`/`GlobalArgs` are production types we may not touch and don't -/// derive `PartialEq`, so this mirror exists purely so a single `assert_eq!` -/// can police the entire parsed surface at once. -/// -/// This is what makes the defaults/alias tests honest. A field-at-a-time -/// assertion only proves the one field it inspects; it says nothing about -/// whether some *other* default silently regressed to a non-default value, or -/// whether a flag flipped an unrelated field (a clap-derive copy/paste bug). -/// Comparing the whole snapshot against the independently-declared defaults -/// fails loudly the instant any field moves. +/// Every parsed field of `RepairArgs`. The tests compare the whole snapshot, +/// so a flag wired to a second field fails too. #[derive(Debug, Clone, PartialEq)] struct Snap { - cwd: PathBuf, - manifest_path: String, - api_url: Option, - api_token: Option, - org: Option, - proxy_url: Option, - ecosystems: Option>, - vendor_source: String, - vendor_url: Option, - patch_server_url: Option, - offline: bool, - strict: bool, - allow_patch_conflicts: bool, - global: bool, - global_prefix: Option, - json: bool, - verbose: bool, - silent: bool, - dry_run: bool, - yes: bool, - lock_timeout: Option, - debug: bool, - no_telemetry: bool, + common: Globals, download_only: bool, } fn snapshot(a: &RepairArgs) -> Snap { Snap { - cwd: a.common.cwd.clone(), - manifest_path: a.common.manifest_path.clone(), - api_url: a.common.api_url.clone(), - api_token: a.common.api_token.clone(), - org: a.common.org.clone(), - proxy_url: a.common.proxy_url.clone(), - ecosystems: a.common.ecosystems.clone(), - vendor_source: a.common.vendor_source.clone(), - vendor_url: a.common.vendor_url.clone(), - patch_server_url: a.common.patch_server_url.clone(), - offline: a.common.offline, - strict: a.common.strict, - allow_patch_conflicts: a.common.allow_patch_conflicts, - global: a.common.global, - global_prefix: a.common.global_prefix.clone(), - json: a.common.json, - verbose: a.common.verbose, - silent: a.common.silent, - dry_run: a.common.dry_run, - yes: a.common.yes, - lock_timeout: a.common.lock_timeout, - debug: a.common.debug, - no_telemetry: a.common.no_telemetry, + common: Globals(a.common.clone()), download_only: a.download_only, } } -/// Independent oracle: the snapshot a correct parse of bare `repair` (no flags) -/// must produce. The values are transcribed BY HAND from the -/// `default_value`/`default_value_t` declarations on `RepairArgs`/`GlobalArgs` -/// and the `DEFAULT_*` constants in `socket-patch-core` — NOT read back from a -/// live parse — so this can actually disagree with the implementation if a -/// default regresses. +/// The snapshot of bare `repair`, written by hand. fn expected_defaults() -> Snap { Snap { - cwd: PathBuf::from("."), - manifest_path: ".socket/manifest.json".to_string(), - api_url: None, // no clap default — resolved in core - api_token: None, - org: None, - proxy_url: None, // no clap default — resolved in core - ecosystems: None, - vendor_source: "service".to_string(), - vendor_url: None, - patch_server_url: None, - offline: false, - strict: false, - allow_patch_conflicts: false, - global: false, - global_prefix: None, - json: false, - verbose: false, - silent: false, - dry_run: false, - yes: false, - lock_timeout: None, - debug: false, - no_telemetry: false, + common: clap_parse::global_defaults(), download_only: false, } } @@ -214,13 +59,6 @@ fn expected_defaults() -> Snap { #[serial_test::serial] fn repair_defaults_match_contract() { let args = parse_repair(&[]); - - // Pin the *entire* default surface in one shot against the independent - // oracle. The previous version only checked cwd, - // manifest_path, dry_run, offline, download_only and json — leaving - // api_url, proxy_url, verbose, silent, yes, lock_timeout, - // debug, no_telemetry, global, global_prefix, ecosystems, api_token and - // org free to regress unnoticed. assert_eq!(snapshot(&args), expected_defaults()); } @@ -231,7 +69,7 @@ fn repair_dry_run_long_flag() { // The flag flips dry_run and *nothing else* — anything but this exact // one-field delta from the defaults is a regression. let mut expected = expected_defaults(); - expected.dry_run = true; + expected.common.dry_run = true; assert_eq!(snapshot(&args), expected); } @@ -240,7 +78,7 @@ fn repair_dry_run_long_flag() { fn repair_manifest_path_long_flag() { let args = parse_repair(&["--manifest-path", "custom.json"]); let mut expected = expected_defaults(); - expected.manifest_path = "custom.json".to_string(); + expected.common.manifest_path = "custom.json".to_string(); assert_eq!(snapshot(&args), expected); } @@ -249,7 +87,7 @@ fn repair_manifest_path_long_flag() { fn repair_cwd_flag() { let args = parse_repair(&["--cwd", "/tmp/x"]); let mut expected = expected_defaults(); - expected.cwd = PathBuf::from("/tmp/x"); + expected.common.cwd = PathBuf::from("/tmp/x"); assert_eq!(snapshot(&args), expected); } @@ -258,7 +96,7 @@ fn repair_cwd_flag() { fn repair_offline_flag() { let args = parse_repair(&["--offline"]); let mut expected = expected_defaults(); - expected.offline = true; + expected.common.offline = true; assert_eq!(snapshot(&args), expected); } @@ -276,7 +114,7 @@ fn repair_download_only_flag() { fn repair_json_flag() { let args = parse_repair(&["--json"]); let mut expected = expected_defaults(); - expected.json = true; + expected.common.json = true; assert_eq!(snapshot(&args), expected); } @@ -284,7 +122,7 @@ fn repair_json_flag() { #[test] #[serial_test::serial] fn repair_download_mode_flag_is_removed() { - match Cli::try_parse_from(["socket-patch", "repair", "--download-mode", "file"]) { + match try_parse(&["socket-patch", "repair", "--download-mode", "file"]) { Ok(_) => panic!("--download-mode must be rejected"), Err(err) => assert_eq!(err.kind(), clap::error::ErrorKind::UnknownArgument), } @@ -298,14 +136,11 @@ fn repair_download_mode_flag_is_removed() { /// `--download-only` env binding must match. (`main`'s empty-var scrub also /// removes a blank `SOCKET_DOWNLOAD_ONLY` via `LOCAL_ARG_ENV_VARS`, but the /// parser itself must not depend on it — library callers of `Cli::parse` -/// never run the scrub, as this test's direct `try_parse_from` shows.) +/// never run the scrub, and neither does this test's parse.) #[test] #[serial_test::serial] fn empty_download_only_env_var_parses_as_false_not_crash() { - let _scrub = EnvScrub::new(); - std::env::set_var("SOCKET_DOWNLOAD_ONLY", ""); - let parsed = Cli::try_parse_from(["socket-patch", "repair"]); - std::env::remove_var("SOCKET_DOWNLOAD_ONLY"); + let parsed = try_parse_with_env(&[("SOCKET_DOWNLOAD_ONLY", "")], &["socket-patch", "repair"]); let cli = parsed.expect("empty SOCKET_DOWNLOAD_ONLY must not abort the parse"); match cli.command { Commands::Repair(a) => assert!( @@ -321,10 +156,10 @@ fn empty_download_only_env_var_parses_as_false_not_crash() { #[test] #[serial_test::serial] fn truthy_download_only_env_var_sets_flag() { - let _scrub = EnvScrub::new(); - std::env::set_var("SOCKET_DOWNLOAD_ONLY", "1"); - let parsed = Cli::try_parse_from(["socket-patch", "repair"]); - std::env::remove_var("SOCKET_DOWNLOAD_ONLY"); + let parsed = try_parse_with_env( + &[("SOCKET_DOWNLOAD_ONLY", "1")], + &["socket-patch", "repair"], + ); let cli = parsed.expect("SOCKET_DOWNLOAD_ONLY=1 must parse"); match cli.command { Commands::Repair(a) => assert!(a.download_only), @@ -332,53 +167,10 @@ fn truthy_download_only_env_var_sets_flag() { } } -// --- Hermeticity of the scrub itself ------------------------------------------- - -#[test] -#[serial_test::serial] -fn scrub_covers_every_global_env_var_clap_consults() { - // [`SOCKET_ENV_VARS`] claims to list "every SOCKET_* env var that clap - // consults while parsing `repair`". `GlobalArgs` is flattened in whole, - // so the production `GLOBAL_ARG_ENV_VARS` list is the oracle — a flag - // added to `GlobalArgs` with an env binding is consulted here the moment - // it lands, and if the scrub list lags behind, an ambient value either - // aborts every parse in this file (validated flags: bools, ints, - // `--ecosystems`, `--vendor-source`) or silently leaks into the parsed - // args (string flags), voiding the hermeticity the module doc promises. - // `garbage` is rejected by every validating parser and visibly - // non-default for every string/path/option flag, so a missing scrub - // entry fails loudly either way. Mirrors `cli_parse_get.rs`. - for &var in socket_patch_cli::args::GLOBAL_ARG_ENV_VARS { - let prev = std::env::var(var).ok(); - std::env::set_var(var, "garbage"); - let parsed = { - let _scrub = EnvScrub::new(); - Cli::try_parse_from(["socket-patch", "repair"]) - }; - match prev { - Some(v) => std::env::set_var(var, v), - None => std::env::remove_var(var), - } - let a = match parsed { - Ok(cli) => match cli.command { - Commands::Repair(a) => a, - _ => panic!("expected Repair"), - }, - Err(e) => panic!("ambient {var}=garbage aborted the scrubbed parse: {e}"), - }; - assert_eq!( - snapshot(&a), - expected_defaults(), - "ambient {var}=garbage leaked into the scrubbed parse", - ); - } -} - #[test] #[serial_test::serial] fn repair_unknown_flag_is_unknown_argument_error() { - let _scrub = EnvScrub::new(); - let err = match Cli::try_parse_from(["socket-patch", "repair", "--nope"]) { + let err = match try_parse(&["socket-patch", "repair", "--nope"]) { Ok(_) => panic!("unknown flag should fail to parse"), Err(e) => e, }; @@ -392,8 +184,7 @@ fn repair_unknown_flag_is_unknown_argument_error() { // clean up without an apply pass. v5 removed its `gc` alias. fn top_level_help() -> String { - let _scrub = EnvScrub::new(); - match Cli::try_parse_from(["socket-patch", "--help"]) { + match try_parse(&["socket-patch", "--help"]) { Ok(_) => panic!("--help should return a clap error (DisplayHelp)"), Err(e) => format!("{e}"), } @@ -416,8 +207,7 @@ fn repair_appears_in_top_level_help() { fn removed_gc_alias_is_a_usage_error() { let help = top_level_help(); assert!(!help.contains("[aliases: gc]"), "{help}"); - let _scrub = EnvScrub::new(); - match Cli::try_parse_from(["socket-patch", "gc"]) { + match try_parse(&["socket-patch", "gc"]) { Ok(_) => panic!("`gc` should no longer parse"), Err(e) => assert_eq!(e.kind(), clap::error::ErrorKind::InvalidSubcommand), } @@ -426,10 +216,10 @@ fn removed_gc_alias_is_a_usage_error() { #[test] #[serial_test::serial] fn no_vlt_install_cleanup_is_accepted_silently_by_repair() { - assert!(!parse_repair(&[]).common.no_vlt_install_cleanup); - assert!( - parse_repair(&["--no-vlt-install-cleanup"]) - .common - .no_vlt_install_cleanup + let mut expected = expected_defaults(); + expected.common.no_vlt_install_cleanup = true; + assert_eq!( + snapshot(&parse_repair(&["--no-vlt-install-cleanup"])), + expected ); } diff --git a/crates/socket-patch-cli/tests/cli_parse_scan.rs b/crates/socket-patch-cli/tests/cli_parse_scan.rs index 49d2e8d7c..c3bbaf623 100644 --- a/crates/socket-patch-cli/tests/cli_parse_scan.rs +++ b/crates/socket-patch-cli/tests/cli_parse_scan.rs @@ -8,91 +8,23 @@ //! `--batch-size` has no parse-time default: unset, `scan` picks it per //! endpoint at run time (500 on the authenticated API, 100 on the public //! proxy), so an explicit value must stay distinguishable from none. +//! +//! Several tests set `SOCKET_*` vars, and clap reads them during every +//! parse, so every test that parses is `#[serial]`. +#[path = "common/clap_parse.rs"] +mod clap_parse; #[path = "common/hermetic.rs"] mod hermetic; -use clap::Parser; +use hermetic::try_parse; use socket_patch_cli::commands::scan::{resolve_mode_flags, ScanArgs, ScanMode}; use socket_patch_cli::{Cli, Commands}; -/// Every `ScanArgs`/`GlobalArgs`/`VexEmbedArgs` field that has an `env = -/// "SOCKET_*"` binding. clap reads these at parse time whenever the matching -/// flag is absent, so an ambient value silently overrides the code-level -/// `default_value`. That defeats the entire purpose of these snapshot tests: -/// a regression that flips a `default_value` (e.g. `--batch-size` → `50`) would stay GREEN on any machine -/// whose shell/CI happens to export the old value, and the "default" tests -/// would be asserting the environment, not the parser. We therefore clear -/// the whole set before every parse and restore it after, under `#[serial]` -/// so the process-global mutation can't race a concurrent test. -/// -/// Keep this list in sync with `env = "SOCKET_*"` attrs in -/// `src/args.rs`, `src/commands/scan/mod.rs`, and `src/commands/vex.rs`. -const SCAN_ENV_VARS: &[&str] = &[ - "SOCKET_ALL_RELEASES", - "SOCKET_API_TOKEN", - "SOCKET_API_URL", - "SOCKET_BATCH_SIZE", - "SOCKET_CWD", - "SOCKET_SCAN_PACKAGES", - "SOCKET_DEBUG", - "SOCKET_DRY_RUN", - "SOCKET_ECOSYSTEMS", - "SOCKET_GLOBAL", - "SOCKET_GLOBAL_PREFIX", - "SOCKET_JSON", - "SOCKET_LOCK_TIMEOUT", - "SOCKET_MANIFEST_PATH", - "SOCKET_MAX_NEW_PATCHES", - "SOCKET_MIN_SEVERITY", - "SOCKET_NO_SOCKET_YML", - "SOCKET_NO_TRUST_LOCKFILE_CONFIG", - "SOCKET_NO_NPM_ALLOW_REMOTE_CONFIG", - "SOCKET_NO_VLT_INSTALL_CLEANUP", - "SOCKET_OFFLINE", - "SOCKET_ORG_SLUG", - "SOCKET_PATCH_SERVER_URL", - "SOCKET_PROXY_URL", - "SOCKET_SILENT", - "SOCKET_STRICT", - "SOCKET_TELEMETRY_DISABLED", - "SOCKET_VENDOR_SOURCE", - "SOCKET_VENDOR_URL", - "SOCKET_VERBOSE", - "SOCKET_VEX", - "SOCKET_VEX_COMPACT", - "SOCKET_VEX_DOC_ID", - "SOCKET_VEX_NO_VERIFY", - "SOCKET_VEX_OUTPUT", - "SOCKET_VEX_PRODUCT", - "SOCKET_YES", -]; - -/// Run `f` with every `SOCKET_*` var removed from the environment, then -/// restore the originals. Must be called only from `#[serial]` tests — -/// env state is process-global. -fn with_clean_env(f: impl FnOnce() -> T) -> T { - let saved: Vec<(&str, Option)> = SCAN_ENV_VARS - .iter() - .map(|k| (*k, std::env::var(k).ok())) - .collect(); - for k in SCAN_ENV_VARS { - std::env::remove_var(k); - } - let result = f(); - for (k, orig) in saved { - match orig { - Some(v) => std::env::set_var(k, v), - None => std::env::remove_var(k), - } - } - result -} - fn parse_scan(extra: &[&str]) -> ScanArgs { let mut argv = vec!["socket-patch", "scan"]; argv.extend_from_slice(extra); - let cli = with_clean_env(|| Cli::try_parse_from(&argv)).expect("parse"); + let cli = try_parse(&argv).expect("parse"); match cli.command { Commands::Scan(a) => a, _ => panic!("expected Scan"), @@ -102,7 +34,7 @@ fn parse_scan(extra: &[&str]) -> ScanArgs { fn try_parse_scan(extra: &[&str]) -> Result { let mut argv = vec!["socket-patch", "scan"]; argv.extend_from_slice(extra); - let cli = with_clean_env(|| Cli::try_parse_from(&argv))?; + let cli = try_parse(&argv)?; match cli.command { Commands::Scan(a) => Ok(a), _ => panic!("expected Scan"), @@ -121,22 +53,16 @@ fn defaults_match_contract() { ); // All other defaults from the scan table. - assert_eq!(args.common.cwd, std::path::PathBuf::from(".")); - assert_eq!(args.common.org, None); - assert!(!args.common.json); - assert!(!args.common.yes); - assert!(!args.common.global); - assert_eq!(args.common.global_prefix, None); - assert_eq!(args.common.api_url, None); // default applied in core resolver - assert_eq!(args.common.api_token, None); - assert_eq!(args.common.ecosystems, None); + assert_eq!( + clap_parse::Globals(args.common.clone()), + clap_parse::global_defaults() + ); assert!( !args.prune, "--prune default is false (GC is opt-in in v3.0)" ); assert!(!args.sync, "--sync default is false"); assert_eq!(args.mode, None, "--mode default is None (no mode selector)"); - assert!(!args.common.dry_run, "--dry-run default is false"); assert!( !args.all_releases, "--all-releases default is false (narrow — installed-dist variant only)" @@ -656,74 +582,6 @@ fn removed_mode_booleans_are_usage_errors() { } } -// --- scrub-list completeness guard ----------------------------------------- - -/// Full-surface snapshot of a parsed `ScanArgs`, for env-leak detection. -/// The flattened `GlobalArgs` participates via its `Debug` impl so every -/// field — including ones added after this file was written — is covered -/// without being named here; the scan-local and vex-embed fields (no -/// `Debug` derive) are formatted individually. -fn snap(a: &ScanArgs) -> String { - format!( - "{:?} paths={:?} batch_size={:?} prune={} sync={} \ - mode={:?} all_releases={} vex={:?} vex_product={:?} \ - vex_no_verify={} vex_doc_id={:?} vex_compact={}", - a.common, - a.paths, - a.batch_size, - a.prune, - a.sync, - a.mode, - a.all_releases, - a.vex.vex, - a.vex.vex_product, - a.vex.vex_no_verify, - a.vex.vex_doc_id, - a.vex.vex_compact, - ) -} - -#[test] -#[serial_test::serial] -fn scrub_covers_every_scan_env_var_clap_consults() { - // `SCAN_ENV_VARS` claims to cover every `SOCKET_*` var clap consults - // while parsing `scan`. The production oracles are - // `GLOBAL_ARG_ENV_VARS` (the flattened `GlobalArgs`, consulted by every - // subcommand the moment a binding lands) and `LOCAL_ARG_ENV_VARS` - // (subcommand-local bindings — scan's own plus other subcommands', - // which the scan parse never reads, so probing them is harmless). For - // each var: plant `garbage`, parse under the scrub, and require the - // exact clean-parse result. A var missing from the scrub fails loudly - // either way — validated flags (bools, ints, `--ecosystems`, - // `--vendor-source`) abort the parse; free-form strings/paths leak a - // visibly non-default value into the snapshot. - let baseline = snap(&parse_scan(&[])); - for &var in socket_patch_cli::args::GLOBAL_ARG_ENV_VARS - .iter() - .chain(socket_patch_cli::args::LOCAL_ARG_ENV_VARS) - { - let prev = std::env::var(var).ok(); - std::env::set_var(var, "garbage"); - let parsed = with_clean_env(|| Cli::try_parse_from(["socket-patch", "scan"])); - match prev { - Some(v) => std::env::set_var(var, v), - None => std::env::remove_var(var), - } - let args = match parsed { - Ok(cli) => match cli.command { - Commands::Scan(a) => a, - _ => panic!("expected Scan"), - }, - Err(e) => panic!("ambient {var}=garbage aborted the scrubbed parse: {e}"), - }; - assert_eq!( - snap(&args), - baseline, - "ambient {var}=garbage leaked into the scrubbed parse", - ); - } -} - #[test] #[serial_test::serial] fn non_canonical_mode_values_are_rejected() { @@ -731,8 +589,7 @@ fn non_canonical_mode_values_are_rejected() { // and the v3/v4 value aliases `host` / `redirect` / `vendor` were // removed in v5.0. for value in ["apply", "host", "redirect", "vendor"] { - let parsed = - with_clean_env(|| Cli::try_parse_from(["socket-patch", "scan", "--mode", value])); + let parsed = try_parse(&["socket-patch", "scan", "--mode", value]); let Err(err) = parsed else { panic!("--mode {value} must be rejected"); }; @@ -750,7 +607,7 @@ fn non_canonical_mode_values_are_rejected() { #[serial_test::serial] fn removed_legacy_scan_flags_are_rejected() { for flag in ["--redirect", "--detached"] { - let parsed = with_clean_env(|| Cli::try_parse_from(["socket-patch", "scan", flag])); + let parsed = try_parse(&["socket-patch", "scan", flag]); let Err(err) = parsed else { panic!("{flag} must be rejected"); }; @@ -768,14 +625,11 @@ fn mode_help_lists_only_canonical_names() { use clap::CommandFactory; // clap embeds live env values into help as `[env: VAR=value]` at // command-build/render time, so the whole build+render runs clean. - let (short, long) = with_clean_env(|| { - let mut cmd = Cli::command(); - let scan = cmd.find_subcommand_mut("scan").expect("scan subcommand"); - ( - scan.render_help().to_string(), - scan.render_long_help().to_string(), - ) - }); + hermetic::scrub_process_socket_env(); + let mut cmd = Cli::command(); + let scan = cmd.find_subcommand_mut("scan").expect("scan subcommand"); + let short = scan.render_help().to_string(); + let long = scan.render_long_help().to_string(); // Short help (`scan -h`) renders the compact bracketed list. Assert on // the exact rendered segment so any extra value inside the brackets @@ -808,12 +662,10 @@ fn no_vlt_install_cleanup_flag_and_env_parse() { .common .no_vlt_install_cleanup ); - let from_env = with_clean_env(|| { - std::env::set_var("SOCKET_NO_VLT_INSTALL_CLEANUP", "1"); - let cli = Cli::try_parse_from(["socket-patch", "scan", "--mode", "hosted"]); - std::env::remove_var("SOCKET_NO_VLT_INSTALL_CLEANUP"); - cli - }) + let from_env = clap_parse::try_parse_with_env( + &[("SOCKET_NO_VLT_INSTALL_CLEANUP", "1")], + &["socket-patch", "scan", "--mode", "hosted"], + ) .expect("parse"); match from_env.command { Commands::Scan(a) => assert!(a.common.no_vlt_install_cleanup), @@ -880,22 +732,13 @@ fn max_new_patches_env_is_read_at_run_time() { let got = flag.resolve(Some("7"), None).unwrap(); assert_eq!((got.value, got.source), (Some(1), MaxNewSource::Flag)); } -/// Parse `scan` under a clean env plus `env`, restoring it afterwards. + fn parse_scan_with_env(extra: &[&str], env: &[(&str, &str)]) -> Result { - with_clean_env(|| { - for (k, v) in env { - std::env::set_var(k, v); - } - let mut argv = vec!["socket-patch", "scan"]; - argv.extend_from_slice(extra); - let cli = Cli::try_parse_from(&argv); - for (k, _) in env { - std::env::remove_var(k); - } - cli.map(|c| match c.command { - Commands::Scan(a) => a, - _ => panic!("expected Scan"), - }) + let mut argv = vec!["socket-patch", "scan"]; + argv.extend_from_slice(extra); + clap_parse::try_parse_with_env(env, &argv).map(|c| match c.command { + Commands::Scan(a) => a, + _ => panic!("expected Scan"), }) } @@ -922,17 +765,16 @@ fn no_socket_yml_flag_and_env() { fn min_severity_flag_and_env() { use socket_patch_core::policy::OverrideSource; let overrides = |extra: &[&str], env: &[(&str, &str)]| { + // `overrides` reads `SOCKET_MIN_SEVERITY` itself, at run time. let args = parse_scan_with_env(extra, env).expect("parse"); - with_clean_env(|| { - for (k, v) in env { - std::env::set_var(k, v); - } - let out = args.socket_yml.overrides(); - for (k, _) in env { - std::env::remove_var(k); - } - out - }) + for (k, v) in env { + std::env::set_var(k, v); + } + let out = args.socket_yml.overrides(); + for (k, _) in env { + std::env::remove_var(k); + } + out }; assert_eq!(parse_scan(&[]).socket_yml.min_severity, None); assert_eq!(overrides(&[], &[]).unwrap().min_severity, None); diff --git a/crates/socket-patch-cli/tests/cli_parse_vendor.rs b/crates/socket-patch-cli/tests/cli_parse_vendor.rs index 515ff2bbe..35da21ce3 100644 --- a/crates/socket-patch-cli/tests/cli_parse_vendor.rs +++ b/crates/socket-patch-cli/tests/cli_parse_vendor.rs @@ -9,173 +9,46 @@ //! `vendor`. Changing any assertion here is a breaking change to the CLI //! surface — see `crates/socket-patch-cli/CLI_CONTRACT.md`. //! -//! ## Hermeticity -//! -//! Every flag and default below is also wired to an `#[arg(env = "SOCKET_*")]` -//! source. clap reads those env vars during `try_parse_from`, so an ambient -//! `SOCKET_*` variable in the developer's shell or in CI would silently -//! satisfy these assertions even if the corresponding CLI default -//! (`default_value`/`default_value_t`) regressed or a flag's action broke — -//! the env value would mask the bug and the test would pass for the wrong -//! reason. To make the assertions test *argv parsing* rather than the -//! ambient environment, every parse runs with the full set of `SOCKET_*` -//! vars scrubbed (see [`EnvScrub`]). Because the environment is process- -//! global, every test is `#[serial_test::serial]` so the scrub/restore -//! dance can't race a concurrent parse. This mirrors `cli_parse_get.rs` / -//! `cli_parse_repair.rs`. - -use clap::Parser; +//! The env-wiring tests set `SOCKET_*` vars, and clap reads them during every +//! parse, so every test that parses is `#[serial]`. + use socket_patch_cli::commands::vendor::VendorArgs; use socket_patch_cli::{parse_argv_with_shortcuts, Cli, Commands}; use std::path::PathBuf; -/// Every `SOCKET_*` env var that clap consults while parsing `vendor` (its -/// own flags, the flattened `GlobalArgs`, and the flattened `VexEmbedArgs`). -/// If any of these leaks in from the ambient environment it can mask a -/// broken default or a regressed flag, so the parse helpers below remove -/// them for the duration of the parse. -const SOCKET_ENV_VARS: &[&str] = &[ - // GlobalArgs - "SOCKET_CWD", - "SOCKET_MANIFEST_PATH", - "SOCKET_API_URL", - "SOCKET_API_TOKEN", - "SOCKET_ORG_SLUG", - "SOCKET_PROXY_URL", - "SOCKET_ECOSYSTEMS", - "SOCKET_VENDOR_SOURCE", - "SOCKET_VENDOR_URL", - "SOCKET_PATCH_SERVER_URL", - "SOCKET_OFFLINE", - "SOCKET_STRICT", - "SOCKET_GLOBAL", - "SOCKET_GLOBAL_PREFIX", - "SOCKET_JSON", - "SOCKET_VERBOSE", - "SOCKET_SILENT", - "SOCKET_DRY_RUN", - "SOCKET_YES", - "SOCKET_LOCK_TIMEOUT", - "SOCKET_DEBUG", - "SOCKET_TELEMETRY_DISABLED", - "SOCKET_NO_TRUST_LOCKFILE_CONFIG", - "SOCKET_NO_NPM_ALLOW_REMOTE_CONFIG", - "SOCKET_NO_VLT_INSTALL_CLEANUP", - // VendorArgs-specific (`SOCKET_FORCE` is retired but still scrubbed so a - // stale export can't leak into the "is ignored" test) - "SOCKET_FORCE", - "SOCKET_VENDOR_REVERT", - // VexEmbedArgs (flattened embedded-VEX passthrough) - "SOCKET_VEX", - "SOCKET_VEX_PRODUCT", - "SOCKET_VEX_NO_VERIFY", - "SOCKET_VEX_DOC_ID", - "SOCKET_VEX_COMPACT", -]; - -/// RAII guard that removes every [`SOCKET_ENV_VARS`] entry on construction and -/// restores the prior value on drop. Holding one of these around a clap parse -/// guarantees the parse sees only what's on the argv, not the developer's -/// shell. Pair with `#[serial_test::serial]` so the global env mutation never -/// races another test. -struct EnvScrub(Vec<(&'static str, Option)>); - -impl EnvScrub { - fn new() -> Self { - let saved = SOCKET_ENV_VARS - .iter() - .map(|&k| { - let prev = std::env::var(k).ok(); - std::env::remove_var(k); - (k, prev) - }) - .collect(); - EnvScrub(saved) - } -} +#[path = "common/clap_parse.rs"] +mod clap_parse; +#[path = "common/hermetic.rs"] +mod hermetic; -impl Drop for EnvScrub { - fn drop(&mut self) { - for (k, v) in &self.0 { - match v { - Some(val) => std::env::set_var(k, val), - None => std::env::remove_var(k), - } - } - } -} +use clap_parse::Globals; +use hermetic::try_parse; -/// Parse `socket-patch vendor ` and return the `VendorArgs`, with -/// the ambient `SOCKET_*` environment scrubbed so the result reflects only -/// the argv. The scrub guard is held across the parse and dropped before the -/// caller's assertions run (which only inspect the returned struct). fn parse_vendor(extra: &[&str]) -> VendorArgs { - let _scrub = EnvScrub::new(); let mut argv = vec!["socket-patch", "vendor"]; argv.extend_from_slice(extra); - let cli = Cli::try_parse_from(&argv).expect("parse"); + let cli = try_parse(&argv).expect("parse"); match cli.command { Commands::Vendor(a) => a, _ => panic!("expected Vendor"), } } -/// Parse `socket-patch vendor ` with `env` injected into an -/// otherwise fully-scrubbed `SOCKET_*` environment. Returns the raw clap -/// result so env-wiring tests can assert both the success and the failure -/// shapes. The injected vars are removed before the scrub guard restores -/// the ambient values. fn parse_vendor_with_env(env: &[(&str, &str)], extra: &[&str]) -> Result { - let _scrub = EnvScrub::new(); - for (k, v) in env { - std::env::set_var(k, v); - } let mut argv = vec!["socket-patch", "vendor"]; argv.extend_from_slice(extra); - let result = Cli::try_parse_from(&argv); - for (k, _) in env { - std::env::remove_var(k); - } - result.map(|cli| match cli.command { + clap_parse::try_parse_with_env(env, &argv).map(|cli| match cli.command { Commands::Vendor(a) => a, _ => panic!("expected Vendor"), }) } -/// Owned, comparable snapshot of *every* parsed field in `VendorArgs` — its -/// own flags (`force`, `revert`), every field of the flattened `GlobalArgs`, -/// and every field of the flattened `VexEmbedArgs`. `VendorArgs` is -/// production code that doesn't derive `PartialEq`, so this mirror exists -/// purely so a single `assert_eq!` can police the entire parsed surface at -/// once. -/// -/// This is what makes the per-flag tests honest. A field-at-a-time assertion -/// (`assert!(a.force)`) only proves the flag set *its* field; it says nothing -/// about whether the same flag also flipped an unrelated one. A clap-derive -/// copy/paste regression (e.g. `--revert` accidentally wired to `force`) -/// would set both and still pass a single-field check. Comparing the whole -/// snapshot against the independently-declared defaults — with only the -/// field under test mutated — fails loudly the instant any other field moves. +/// Every parsed field of `VendorArgs`, including the flattened `GlobalArgs` +/// and `VexEmbedArgs`. The per-flag tests compare the whole snapshot, so a +/// flag wired to a second field fails too. #[derive(Debug, Clone, PartialEq)] struct Snap { - cwd: PathBuf, - manifest_path: String, - api_url: Option, - api_token: Option, - org: Option, - proxy_url: Option, - ecosystems: Option>, - offline: bool, - global: bool, - global_prefix: Option, - json: bool, - verbose: bool, - silent: bool, - dry_run: bool, - yes: bool, - lock_timeout: Option, - debug: bool, - no_telemetry: bool, + common: Globals, force: bool, revert: bool, vex: Option, @@ -187,24 +60,7 @@ struct Snap { fn snapshot(a: &VendorArgs) -> Snap { Snap { - cwd: a.common.cwd.clone(), - manifest_path: a.common.manifest_path.clone(), - api_url: a.common.api_url.clone(), - api_token: a.common.api_token.clone(), - org: a.common.org.clone(), - proxy_url: a.common.proxy_url.clone(), - ecosystems: a.common.ecosystems.clone(), - offline: a.common.offline, - global: a.common.global, - global_prefix: a.common.global_prefix.clone(), - json: a.common.json, - verbose: a.common.verbose, - silent: a.common.silent, - dry_run: a.common.dry_run, - yes: a.common.yes, - lock_timeout: a.common.lock_timeout, - debug: a.common.debug, - no_telemetry: a.common.no_telemetry, + common: Globals(a.common.clone()), force: a.force, revert: a.revert, vex: a.vex.vex.clone(), @@ -215,34 +71,10 @@ fn snapshot(a: &VendorArgs) -> Snap { } } -/// Independent oracle: the snapshot a correct parse of bare `vendor` (no -/// flags) must produce. The values are transcribed BY HAND from the -/// `default_value`/`default_value_t` declarations on `VendorArgs` / -/// `GlobalArgs` / `VexEmbedArgs` and the `DEFAULT_*` constants in -/// `socket-patch-core` — NOT read back from a live parse — so this can -/// actually disagree with the implementation if a default regresses. Every -/// per-flag test starts from this and mutates exactly the one field the flag -/// is supposed to touch. +/// The snapshot of bare `vendor`, written by hand. fn expected_defaults() -> Snap { Snap { - cwd: PathBuf::from("."), - manifest_path: ".socket/manifest.json".to_string(), - api_url: None, // no clap default — resolved in core - api_token: None, - org: None, - proxy_url: None, // no clap default — resolved in core - ecosystems: None, - offline: false, - global: false, - global_prefix: None, - json: false, - verbose: false, - silent: false, - dry_run: false, - yes: false, - lock_timeout: None, - debug: false, - no_telemetry: false, + common: clap_parse::global_defaults(), force: false, revert: false, vex: None, @@ -355,7 +187,7 @@ fn vex_passthrough_knobs_each_set_their_field() { fn json_long_sets_json() { let a = parse_vendor(&["--json"]); let mut want = expected_defaults(); - want.json = true; + want.common.json = true; assert_eq!(snapshot(&a), want); } @@ -364,7 +196,7 @@ fn json_long_sets_json() { fn json_short_sets_json() { let a = parse_vendor(&["-j"]); let mut want = expected_defaults(); - want.json = true; + want.common.json = true; assert_eq!(snapshot(&a), want); } @@ -373,7 +205,7 @@ fn json_short_sets_json() { fn dry_run_sets_dry_run() { let a = parse_vendor(&["--dry-run"]); let mut want = expected_defaults(); - want.dry_run = true; + want.common.dry_run = true; // `--dry-run` is the preview contract ("verifies and writes nothing"); // it must NOT be cross-wired into `--force` or `--revert`. assert_eq!(snapshot(&a), want); @@ -384,7 +216,7 @@ fn dry_run_sets_dry_run() { fn cwd_flag_sets_cwd() { let a = parse_vendor(&["--cwd", "/tmp/project"]); let mut want = expected_defaults(); - want.cwd = PathBuf::from("/tmp/project"); + want.common.cwd = PathBuf::from("/tmp/project"); assert_eq!(snapshot(&a), want); } @@ -393,7 +225,7 @@ fn cwd_flag_sets_cwd() { fn manifest_path_flag_sets_manifest_path() { let a = parse_vendor(&["--manifest-path", "custom.json"]); let mut want = expected_defaults(); - want.manifest_path = "custom.json".to_string(); + want.common.manifest_path = "custom.json".to_string(); assert_eq!(snapshot(&a), want); } @@ -402,7 +234,7 @@ fn manifest_path_flag_sets_manifest_path() { fn offline_flag_sets_offline() { let a = parse_vendor(&["--offline"]); let mut want = expected_defaults(); - want.offline = true; + want.common.offline = true; assert_eq!(snapshot(&a), want); } @@ -411,7 +243,7 @@ fn offline_flag_sets_offline() { fn lock_timeout_flag_sets_lock_timeout() { let a = parse_vendor(&["--lock-timeout", "30"]); let mut want = expected_defaults(); - want.lock_timeout = Some(30); + want.common.lock_timeout = Some(30); assert_eq!(snapshot(&a), want); } @@ -420,7 +252,7 @@ fn lock_timeout_flag_sets_lock_timeout() { fn ecosystems_csv_splits_into_vec() { let a = parse_vendor(&["--ecosystems", "npm,cargo"]); let mut want = expected_defaults(); - want.ecosystems = Some(vec!["npm".to_string(), "cargo".to_string()]); + want.common.ecosystems = Some(vec!["npm".to_string(), "cargo".to_string()]); assert_eq!(snapshot(&a), want); } @@ -432,7 +264,7 @@ fn ecosystems_csv_splits_into_vec() { /// v5 retired `SOCKET_FORCE` (#615): `--force` is flag-only, so a stale /// export leaves `force` at its default instead of bypassing the variant -/// probe. The var stays in the scrub list so this test controls it. +/// probe. #[test] #[serial_test::serial] fn env_socket_force_is_ignored() { @@ -513,9 +345,7 @@ fn env_socket_vex_sets_embedded_vex_path() { // --- Subcommand routing ---------------------------------------------------------- #[test] -#[serial_test::serial] fn vendor_appears_in_subcommand_list() { - let _scrub = EnvScrub::new(); use clap::CommandFactory; let cmd = Cli::command(); assert!( @@ -530,8 +360,7 @@ fn vendor_appears_in_subcommand_list() { #[test] #[serial_test::serial] fn vendor_appears_in_top_level_help() { - let _scrub = EnvScrub::new(); - let err = match Cli::try_parse_from(["socket-patch", "--help"]) { + let err = match try_parse(&["socket-patch", "--help"]) { Ok(_) => panic!("--help should return a clap error (DisplayHelp)"), Err(e) => e, }; @@ -550,7 +379,7 @@ fn vendor_appears_in_top_level_help() { #[test] #[serial_test::serial] fn bare_uuid_fallback_still_routes_to_get_not_vendor() { - let _scrub = EnvScrub::new(); + hermetic::scrub_process_socket_env(); const UUID: &str = "9f6b2c4e-1d3a-4f6b-8c2d-7e5a9b1c3d5f"; let cli = parse_argv_with_shortcuts(vec!["socket-patch".to_string(), UUID.to_string()]) .expect("bare uuid must parse via the get fallback"); @@ -561,32 +390,12 @@ fn bare_uuid_fallback_still_routes_to_get_not_vendor() { } } -// --- Harness invariants -------------------------------------------------------- - -/// Drift guard: [`EnvScrub`] must cover every env var `GlobalArgs` binds — -/// the production `GLOBAL_ARG_ENV_VARS` list is the source of truth. A -/// `GlobalArgs` flag whose env var is missing from [`SOCKET_ENV_VARS`] -/// escapes the scrub, so an ambient value in the developer's shell or CI -/// (e.g. `SOCKET_STRICT=garbage`) aborts every parse in this file — -/// exactly the wrong-reason failure mode the hermeticity contract at the -/// top of this file promises away. -#[test] -fn env_scrub_covers_every_global_arg_env_var() { - for var in socket_patch_cli::args::GLOBAL_ARG_ENV_VARS { - assert!( - SOCKET_ENV_VARS.contains(var), - "{var} is bound by GlobalArgs but missing from SOCKET_ENV_VARS — EnvScrub won't scrub it", - ); - } -} - // --- Error paths ------------------------------------------------------------- #[test] #[serial_test::serial] fn unknown_flag_errors() { - let _scrub = EnvScrub::new(); - let err = match Cli::try_parse_from(["socket-patch", "vendor", "--bogus"]) { + let err = match try_parse(&["socket-patch", "vendor", "--bogus"]) { Err(e) => e, Ok(_) => panic!("expected parse error for unknown flag"), }; @@ -599,8 +408,7 @@ fn unknown_flag_errors() { #[test] #[serial_test::serial] fn bare_force_does_not_consume_next_token() { - let _scrub = EnvScrub::new(); - match Cli::try_parse_from(["socket-patch", "vendor", "--force", "stray"]) { + match try_parse(&["socket-patch", "vendor", "--force", "stray"]) { Ok(_) => panic!("`vendor --force stray` must reject the stray positional"), Err(err) => assert_eq!(err.kind(), clap::error::ErrorKind::UnknownArgument), } @@ -609,12 +417,10 @@ fn bare_force_does_not_consume_next_token() { #[test] #[serial_test::serial] fn no_vlt_install_cleanup_is_accepted_silently_by_vendor() { - assert!( - parse_vendor(&["--no-vlt-install-cleanup"]) - .common - .no_vlt_install_cleanup - ); + let mut want = expected_defaults(); + want.common.no_vlt_install_cleanup = true; + assert_eq!(snapshot(&parse_vendor(&["--no-vlt-install-cleanup"])), want); let from_env = parse_vendor_with_env(&[("SOCKET_NO_VLT_INSTALL_CLEANUP", "true")], &[]).expect("parse"); - assert!(from_env.common.no_vlt_install_cleanup); + assert_eq!(snapshot(&from_env), want); } diff --git a/crates/socket-patch-cli/tests/cli_parse_vex.rs b/crates/socket-patch-cli/tests/cli_parse_vex.rs index 500314716..4340b6876 100644 --- a/crates/socket-patch-cli/tests/cli_parse_vex.rs +++ b/crates/socket-patch-cli/tests/cli_parse_vex.rs @@ -16,104 +16,62 @@ //! `LOCAL_ARG_ENV_VARS`), but these library-level parses bypass `main`, so //! the value parser must accept the empty string itself. //! -//! ## Hermeticity -//! -//! Every parse runs with the full set of `SOCKET_*` vars scrubbed (see -//! [`EnvScrub`]) and each test is `#[serial_test::serial]` because the -//! process environment is global. This mirrors `cli_parse_repair.rs`. +//! Most tests set a `SOCKET_*` var, and clap reads them during every parse, +//! so every test that parses is `#[serial]`. use std::path::PathBuf; -use clap::Parser; use socket_patch_cli::commands::vex::VexArgs; use socket_patch_cli::{Cli, Commands}; -/// Every `SOCKET_*` env var clap consults while parsing `vex`, `apply`, or -/// `scan` (their own flags plus the flattened `GlobalArgs` and -/// `VexEmbedArgs`). Scrubbed around each parse so ambient shell/CI values -/// can't mask or fabricate a result. -const SOCKET_ENV_VARS: &[&str] = &[ - // GlobalArgs - "SOCKET_CWD", - "SOCKET_MANIFEST_PATH", - "SOCKET_API_URL", - "SOCKET_API_TOKEN", - "SOCKET_ORG_SLUG", - "SOCKET_PROXY_URL", - "SOCKET_ECOSYSTEMS", - "SOCKET_VENDOR_SOURCE", - "SOCKET_VENDOR_URL", - "SOCKET_PATCH_SERVER_URL", - "SOCKET_OFFLINE", - "SOCKET_STRICT", - "SOCKET_GLOBAL", - "SOCKET_GLOBAL_PREFIX", - "SOCKET_JSON", - "SOCKET_VERBOSE", - "SOCKET_SILENT", - "SOCKET_DRY_RUN", - "SOCKET_YES", - "SOCKET_LOCK_TIMEOUT", - "SOCKET_DEBUG", - "SOCKET_TELEMETRY_DISABLED", - "SOCKET_NO_TRUST_LOCKFILE_CONFIG", - "SOCKET_NO_NPM_ALLOW_REMOTE_CONFIG", - "SOCKET_NO_VLT_INSTALL_CLEANUP", - // VexArgs / VexEmbedArgs - "SOCKET_VEX", - "SOCKET_VEX_OUTPUT", - "SOCKET_VEX_PRODUCT", - "SOCKET_VEX_NO_VERIFY", - "SOCKET_VEX_DOC_ID", - "SOCKET_VEX_COMPACT", - // Retired in v5 (#615); still scrubbed for hermeticity - "SOCKET_FORCE", - // ScanArgs-specific - "SOCKET_BATCH_SIZE", - "SOCKET_SCAN_PACKAGES", - "SOCKET_ALL_RELEASES", -]; +#[path = "common/clap_parse.rs"] +mod clap_parse; +#[path = "common/hermetic.rs"] +mod hermetic; -/// RAII guard that removes every [`SOCKET_ENV_VARS`] entry on construction and -/// restores the prior value on drop. Holding one of these around a clap parse -/// guarantees the parse sees only what's on the argv (plus whatever the test -/// itself sets), not the developer's shell. Pair with `#[serial_test::serial]` -/// so the global env mutation never races another test. -struct EnvScrub(Vec<(&'static str, Option)>); +use clap_parse::{try_parse_with_env, Globals}; +use hermetic::try_parse; -impl EnvScrub { - fn new() -> Self { - let saved = SOCKET_ENV_VARS - .iter() - .map(|&k| { - let prev = std::env::var(k).ok(); - std::env::remove_var(k); - (k, prev) - }) - .collect(); - EnvScrub(saved) +fn parse_vex(cli: Result) -> VexArgs { + match cli.expect("parse").command { + Commands::Vex(a) => a, + _ => panic!("expected Vex"), } } -impl Drop for EnvScrub { - fn drop(&mut self) { - for (k, v) in &self.0 { - match v { - Some(val) => std::env::set_var(k, val), - None => std::env::remove_var(k), - } - } +/// Every parsed field of `VexArgs`. The tests compare the whole snapshot, so +/// a value landing in a second field fails too. +#[derive(Debug, Clone, PartialEq)] +struct Snap { + common: Globals, + output: Option, + product: Option, + no_verify: bool, + doc_id: Option, + compact: bool, +} + +fn snapshot(a: &VexArgs) -> Snap { + Snap { + common: Globals(a.common.clone()), + output: a.output.clone(), + product: a.product.clone(), + no_verify: a.no_verify, + doc_id: a.doc_id.clone(), + compact: a.compact, } } -/// Scrub the env, set `var=value`, parse `argv`, restore. Returns the parse -/// result so callers can assert on success or failure. -fn parse_with_env(var: &str, value: &str, argv: &[&str]) -> Result { - let _scrub = EnvScrub::new(); - std::env::set_var(var, value); - let parsed = Cli::try_parse_from(argv); - std::env::remove_var(var); - parsed +/// The snapshot of bare `vex`, written by hand. +fn expected_defaults() -> Snap { + Snap { + common: clap_parse::global_defaults(), + output: None, + product: None, + no_verify: false, + doc_id: None, + compact: false, + } } /// The truthy env spellings must work: `SOCKET_VEX_NO_VERIFY=1` must set @@ -121,12 +79,13 @@ fn parse_with_env(var: &str, value: &str, argv: &[&str]) -> Result assert!(a.no_verify, "SOCKET_VEX_NO_VERIFY=1 must set --no-verify"), - _ => panic!("expected Vex"), - } + let a = parse_vex(try_parse_with_env( + &[("SOCKET_VEX_NO_VERIFY", "1")], + &["socket-patch", "vex"], + )); + let mut want = expected_defaults(); + want.no_verify = true; + assert_eq!(snapshot(&a), want); } /// An exported-but-empty `SOCKET_VEX_NO_VERIFY=` — the shell/CI idiom for @@ -135,24 +94,24 @@ fn truthy_vex_no_verify_env_sets_flag_on_vex() { #[test] #[serial_test::serial] fn empty_vex_no_verify_env_parses_as_false_on_vex() { - let cli = parse_with_env("SOCKET_VEX_NO_VERIFY", "", &["socket-patch", "vex"]) - .expect("empty SOCKET_VEX_NO_VERIFY must not abort the parse"); - match cli.command { - Commands::Vex(a) => assert!(!a.no_verify, "empty SOCKET_VEX_NO_VERIFY must be false"), - _ => panic!("expected Vex"), - } + let a = parse_vex(try_parse_with_env( + &[("SOCKET_VEX_NO_VERIFY", "")], + &["socket-patch", "vex"], + )); + assert_eq!(snapshot(&a), expected_defaults()); } /// `SOCKET_VEX_COMPACT=1` must set `vex --compact`. #[test] #[serial_test::serial] fn truthy_vex_compact_env_sets_flag_on_vex() { - let cli = parse_with_env("SOCKET_VEX_COMPACT", "1", &["socket-patch", "vex"]) - .expect("SOCKET_VEX_COMPACT=1 must parse, not abort"); - match cli.command { - Commands::Vex(a) => assert!(a.compact, "SOCKET_VEX_COMPACT=1 must set --compact"), - _ => panic!("expected Vex"), - } + let a = parse_vex(try_parse_with_env( + &[("SOCKET_VEX_COMPACT", "1")], + &["socket-patch", "vex"], + )); + let mut want = expected_defaults(); + want.compact = true; + assert_eq!(snapshot(&a), want); } /// `VexEmbedArgs` shares the env var names with the standalone flags, so an @@ -162,7 +121,7 @@ fn truthy_vex_compact_env_sets_flag_on_vex() { #[test] #[serial_test::serial] fn truthy_vex_no_verify_env_sets_embedded_flag_on_apply() { - let cli = parse_with_env("SOCKET_VEX_NO_VERIFY", "1", &["socket-patch", "apply"]) + let cli = try_parse_with_env(&[("SOCKET_VEX_NO_VERIFY", "1")], &["socket-patch", "apply"]) .expect("SOCKET_VEX_NO_VERIFY=1 must not abort `apply`"); match cli.command { Commands::Apply(a) => assert!( @@ -178,7 +137,7 @@ fn truthy_vex_no_verify_env_sets_embedded_flag_on_apply() { #[test] #[serial_test::serial] fn empty_vex_compact_env_parses_as_false_on_scan() { - let cli = parse_with_env("SOCKET_VEX_COMPACT", "", &["socket-patch", "scan"]) + let cli = try_parse_with_env(&[("SOCKET_VEX_COMPACT", "")], &["socket-patch", "scan"]) .expect("empty SOCKET_VEX_COMPACT must not abort `scan`"); match cli.command { Commands::Scan(a) => assert!(!a.vex.vex_compact, "empty SOCKET_VEX_COMPACT must be false"), @@ -186,192 +145,31 @@ fn empty_vex_compact_env_parses_as_false_on_scan() { } } -/// Owned, comparable snapshot of *every* parsed field in `VexArgs` — its own -/// five flags plus every field of the flattened `GlobalArgs`. `VexArgs` / -/// `GlobalArgs` are production types that don't derive `PartialEq`, so this -/// mirror exists purely so a single `assert_eq!` can police the entire -/// parsed surface at once. Mirrors `cli_parse_repair.rs`. -#[derive(Debug, Clone, PartialEq)] -struct Snap { - cwd: PathBuf, - manifest_path: String, - api_url: Option, - api_token: Option, - org: Option, - proxy_url: Option, - ecosystems: Option>, - vendor_source: String, - vendor_url: Option, - patch_server_url: Option, - offline: bool, - strict: bool, - allow_patch_conflicts: bool, - global: bool, - global_prefix: Option, - json: bool, - verbose: bool, - silent: bool, - dry_run: bool, - yes: bool, - lock_timeout: Option, - debug: bool, - no_telemetry: bool, - no_trust_lockfile_config: bool, - no_npm_allow_remote_config: bool, - no_vlt_install_cleanup: bool, - output: Option, - product: Option, - no_verify: bool, - doc_id: Option, - compact: bool, -} - -fn snapshot(a: &VexArgs) -> Snap { - Snap { - cwd: a.common.cwd.clone(), - manifest_path: a.common.manifest_path.clone(), - api_url: a.common.api_url.clone(), - api_token: a.common.api_token.clone(), - org: a.common.org.clone(), - proxy_url: a.common.proxy_url.clone(), - ecosystems: a.common.ecosystems.clone(), - vendor_source: a.common.vendor_source.clone(), - vendor_url: a.common.vendor_url.clone(), - patch_server_url: a.common.patch_server_url.clone(), - offline: a.common.offline, - strict: a.common.strict, - allow_patch_conflicts: a.common.allow_patch_conflicts, - global: a.common.global, - global_prefix: a.common.global_prefix.clone(), - json: a.common.json, - verbose: a.common.verbose, - silent: a.common.silent, - dry_run: a.common.dry_run, - yes: a.common.yes, - lock_timeout: a.common.lock_timeout, - debug: a.common.debug, - no_telemetry: a.common.no_telemetry, - no_trust_lockfile_config: a.common.no_trust_lockfile_config, - no_npm_allow_remote_config: a.common.no_npm_allow_remote_config, - no_vlt_install_cleanup: a.common.no_vlt_install_cleanup, - output: a.output.clone(), - product: a.product.clone(), - no_verify: a.no_verify, - doc_id: a.doc_id.clone(), - compact: a.compact, - } -} - -/// Independent oracle: the snapshot a correct parse of bare `vex` (no flags, -/// no env) must produce. The values are transcribed BY HAND from the -/// `default_value`/`default_value_t` declarations on `VexArgs`/`GlobalArgs` -/// and the `DEFAULT_*` constants in `socket-patch-core` — NOT read back from -/// a live parse — so this can actually disagree with the implementation if a -/// default regresses. -fn expected_defaults() -> Snap { - Snap { - cwd: PathBuf::from("."), - manifest_path: ".socket/manifest.json".to_string(), - api_url: None, // no clap default — resolved in core - api_token: None, - org: None, - proxy_url: None, // no clap default — resolved in core - ecosystems: None, - vendor_source: "service".to_string(), - vendor_url: None, - patch_server_url: None, - offline: false, - strict: false, - allow_patch_conflicts: false, - global: false, - global_prefix: None, - json: false, - verbose: false, - silent: false, - dry_run: false, - yes: false, - lock_timeout: None, - debug: false, - no_telemetry: false, - no_trust_lockfile_config: false, - no_npm_allow_remote_config: false, - no_vlt_install_cleanup: false, - output: None, - product: None, - no_verify: false, - doc_id: None, - compact: false, - } -} - -/// [`SOCKET_ENV_VARS`] claims to list "every `SOCKET_*` env var clap consults -/// while parsing `vex`, `apply`, or `scan`". `GlobalArgs` is flattened in -/// whole, so the production `GLOBAL_ARG_ENV_VARS` list is the oracle — a flag -/// added to `GlobalArgs` with an env binding is consulted here the moment it -/// lands, and if the scrub list lags behind, an ambient value either aborts -/// every parse in this file (validated flags: bools, ints, `--ecosystems`, -/// `--vendor-source`) or silently leaks into the parsed args (string flags), -/// voiding the hermeticity the module doc promises. `garbage` is rejected by -/// every validating parser and visibly non-default for every -/// string/path/option flag, so a missing scrub entry fails loudly either way. -/// Mirrors `cli_parse_repair.rs` / `cli_parse_get.rs`. -#[test] -#[serial_test::serial] -fn scrub_covers_every_global_env_var_clap_consults() { - for &var in socket_patch_cli::args::GLOBAL_ARG_ENV_VARS { - let prev = std::env::var(var).ok(); - std::env::set_var(var, "garbage"); - let parsed = { - let _scrub = EnvScrub::new(); - Cli::try_parse_from(["socket-patch", "vex"]) - }; - match prev { - Some(v) => std::env::set_var(var, v), - None => std::env::remove_var(var), - } - let a = match parsed { - Ok(cli) => match cli.command { - Commands::Vex(a) => a, - _ => panic!("expected Vex"), - }, - Err(e) => panic!("ambient {var}=garbage aborted the scrubbed parse: {e}"), - }; - assert_eq!( - snapshot(&a), - expected_defaults(), - "ambient {var}=garbage leaked into the scrubbed parse", - ); - } -} - /// The explicit CLI flags keep working through the env fix (the custom /// value parser must not change flag-only usage). #[test] #[serial_test::serial] fn bare_flags_still_parse_without_env() { - let _scrub = EnvScrub::new(); - let cli = Cli::try_parse_from(["socket-patch", "vex", "--no-verify", "--compact"]) - .expect("bare flags must parse"); - match cli.command { - Commands::Vex(a) => { - assert!(a.no_verify); - assert!(a.compact); - } - _ => panic!("expected Vex"), - } + let a = parse_vex(try_parse(&[ + "socket-patch", + "vex", + "--no-verify", + "--compact", + ])); + let mut want = expected_defaults(); + want.no_verify = true; + want.compact = true; + assert_eq!(snapshot(&a), want); } #[test] #[serial_test::serial] fn no_vlt_install_cleanup_env_is_accepted_silently_by_vex() { - let cli = parse_with_env( - "SOCKET_NO_VLT_INSTALL_CLEANUP", - "1", + let a = parse_vex(try_parse_with_env( + &[("SOCKET_NO_VLT_INSTALL_CLEANUP", "1")], &["socket-patch", "vex"], - ) - .expect("SOCKET_NO_VLT_INSTALL_CLEANUP=1 must parse on vex"); - match cli.command { - Commands::Vex(a) => assert!(a.common.no_vlt_install_cleanup), - _ => panic!("expected Vex"), - } + )); + let mut want = expected_defaults(); + want.common.no_vlt_install_cleanup = true; + assert_eq!(snapshot(&a), want); } diff --git a/crates/socket-patch-cli/tests/common/clap_parse.rs b/crates/socket-patch-cli/tests/common/clap_parse.rs new file mode 100644 index 000000000..6c0d42c9e --- /dev/null +++ b/crates/socket-patch-cli/tests/common/clap_parse.rs @@ -0,0 +1,113 @@ +//! Shared pieces of the `cli_parse_*` suites. Includers also declare +//! `mod hermetic` (`common/hermetic.rs`) at their root. + +#![allow(dead_code)] + +use socket_patch_cli::args::GlobalArgs; +use socket_patch_cli::Cli; + +/// [`crate::hermetic::try_parse`] with `env` set for this one parse and +/// removed after it. Every test in a binary that calls this must be +/// `#[serial]`: a concurrent parse would read the vars. +pub fn try_parse_with_env(env: &[(&str, &str)], argv: &[&str]) -> Result { + crate::hermetic::scrub_process_socket_env(); + for (k, v) in env { + std::env::set_var(k, v); + } + let parsed = crate::hermetic::try_parse(argv); + for (k, _) in env { + std::env::remove_var(k); + } + parsed +} + +/// `Cli::try_parse_from` for binaries that also call a command's `run()` +/// in-process, which reads the process env. Once per process, removes every +/// `SOCKET_*` var except the `hermetic::FORCED_OPT_OUTS`, and forces those +/// on, so `run()` reads no developer config and sends no telemetry. An +/// opt-out is never removed, so a concurrent `run()` can't see it missing. +/// `SOCKET_TELEMETRY_DISABLED` binds `--no-telemetry`, so every parse here +/// yields `no_telemetry: true`. +pub fn try_parse_keeping_opt_outs(argv: &[&str]) -> Result { + use crate::hermetic::FORCED_OPT_OUTS; + static ONCE: std::sync::Once = std::sync::Once::new(); + ONCE.call_once(|| { + for (key, _) in std::env::vars_os() { + let name = key.to_string_lossy(); + if name.starts_with("SOCKET_") && !FORCED_OPT_OUTS.iter().any(|(k, _)| *k == name) { + std::env::remove_var(&key); + } + } + for (k, v) in FORCED_OPT_OUTS { + std::env::set_var(k, v); + } + }); + ::try_parse_from(argv) +} + +/// The `GlobalArgs` a parse with no global flag must yield, written out by +/// hand so it can disagree with the `default_value`s. A struct literal: a new +/// global field fails to compile here until its default is added. +pub fn global_defaults() -> Globals { + Globals(GlobalArgs { + cwd: ".".into(), + manifest_path: ".socket/manifest.json".to_string(), + // No clap default: resolved in core. + api_url: None, + api_token: None, + org: None, + // No clap default: resolved in core. + proxy_url: None, + ecosystems: None, + vendor_source: "service".to_string(), + maven_config: None, + vendor_url: None, + patch_server_url: None, + offline: false, + strict: false, + allow_patch_conflicts: false, + global: false, + global_prefix: None, + json: false, + verbose: false, + silent: false, + dry_run: false, + yes: false, + lock_timeout: None, + debug: false, + no_telemetry: false, + no_trust_lockfile_config: false, + no_npm_allow_remote_config: false, + no_vlt_install_cleanup: false, + }) +} + +/// `GlobalArgs` compared through its `Debug` output (it has no `PartialEq`), +/// so a suite's snapshot struct can hold it and still derive `PartialEq`. +#[derive(Clone)] +pub struct Globals(pub GlobalArgs); + +impl PartialEq for Globals { + fn eq(&self, other: &Self) -> bool { + format!("{:?}", self.0) == format!("{:?}", other.0) + } +} + +impl std::fmt::Debug for Globals { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.0.fmt(f) + } +} + +impl std::ops::Deref for Globals { + type Target = GlobalArgs; + fn deref(&self) -> &GlobalArgs { + &self.0 + } +} + +impl std::ops::DerefMut for Globals { + fn deref_mut(&mut self) -> &mut GlobalArgs { + &mut self.0 + } +}