From 372f80ce5f06babc8a91869f25a0bc5ae82e7ce4 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Wed, 7 Oct 2026 10:56:00 -0400 Subject: [PATCH 1/6] Keep test children from sending telemetry to production Test child processes inherited no telemetry opt-out: the hermetic helper kept SOCKET_TELEMETRY_DISABLED out of its scrub but never set it, and .cargo/config.toml [env] only carried SOCKET_NO_CONFIG and SOCKET_NO_UPDATE_CHECK. An unauthenticated run with no mocked proxy URL POSTs its events to patches-api.socket.dev (audit B69). Set SOCKET_TELEMETRY_DISABLED=1 in the workspace [env] table and force it in hermetic::command. Suites that assert telemetry already opt back in with SOCKET_TELEMETRY_DISABLED=0 (or remove it) and a wiremock endpoint, and caller env still lands last. Co-Authored-By: Claude Opus 5.5 (1M context) --- .cargo/config.toml | 7 +++ .../socket-patch-cli/tests/common/hermetic.rs | 10 +++- .../tests/spawn_env_hygiene.rs | 55 +++++++++++++++++++ 3 files changed, 70 insertions(+), 2 deletions(-) diff --git a/.cargo/config.toml b/.cargo/config.toml index e0acd1cd4..e9b0e1391 100644 --- a/.cargo/config.toml +++ b/.cargo/config.toml @@ -32,3 +32,10 @@ SOCKET_NO_CONFIG = "1" # tests opt back in with SOCKET_NO_UPDATE_CHECK=0 plus a wiremock # SOCKET_UPDATE_BASE_URL. SOCKET_NO_UPDATE_CHECK = "1" + +# No test child (and no dev `cargo run`) may POST telemetry to the real +# public proxy: an unauthenticated run sends its events to +# patches-api.socket.dev, and most spawning suites point no proxy URL at a +# mock. Suites that assert telemetry events opt back in with +# SOCKET_TELEMETRY_DISABLED=0 (or remove it) plus a wiremock endpoint. +SOCKET_TELEMETRY_DISABLED = "1" diff --git a/crates/socket-patch-cli/tests/common/hermetic.rs b/crates/socket-patch-cli/tests/common/hermetic.rs index 31822f99c..b7f324009 100644 --- a/crates/socket-patch-cli/tests/common/hermetic.rs +++ b/crates/socket-patch-cli/tests/common/hermetic.rs @@ -48,8 +48,8 @@ const HOSTILE_SEEDS: &[(&str, &str)] = &[ /// A `Command` for `bin` with the hermetic `SOCKET_*` environment: the /// hostile seeds scrubbed, every other ambient `SOCKET_*` removed (removing -/// `SOCKET_API_TOKEN` also forces the public proxy), and the two opt-outs -/// forced on. Callers add args, cwd and their own env afterwards; caller env +/// `SOCKET_API_TOKEN` also forces the public proxy), and the three opt-outs +/// (config layer, update notifier, telemetry) forced on. Callers add args, cwd and their own env afterwards; caller env /// lands last, so explicit injections survive the scrub. pub fn command(bin: &Path) -> Command { let mut cmd = Command::new(bin); @@ -72,6 +72,12 @@ pub fn command(bin: &Path) -> Command { // this force-set is the layer that holds there. Notifier tests opt back // in via caller env (which lands last). cmd.env("SOCKET_NO_UPDATE_CHECK", "1"); + // And for telemetry: an unauthenticated child POSTs its events to the + // real public proxy unless a suite points it at a mock. The workspace + // `[env]` default can be overridden by the developer's shell, so force + // it here too. Telemetry suites opt back in via caller env with + // `SOCKET_TELEMETRY_DISABLED=0` and a wiremock endpoint. + cmd.env("SOCKET_TELEMETRY_DISABLED", "1"); cmd } diff --git a/crates/socket-patch-cli/tests/spawn_env_hygiene.rs b/crates/socket-patch-cli/tests/spawn_env_hygiene.rs index 057700a3b..58c5146fb 100644 --- a/crates/socket-patch-cli/tests/spawn_env_hygiene.rs +++ b/crates/socket-patch-cli/tests/spawn_env_hygiene.rs @@ -272,6 +272,61 @@ fn command_scrubs_ambient_socket_vars_and_forces_the_opt_outs() { ); } +/// B69: a test child must never POST telemetry to the real public proxy. +/// An ambient opt-in (`SOCKET_TELEMETRY_DISABLED=0`, or a developer shell +/// that removed the `.cargo/config.toml` default) must not reach the child; +/// a suite that asserts telemetry sets its own value after `command`. +#[test] +#[serial] +fn command_forces_telemetry_off() { + for ambient in ["0", ""] { + with_ambient(&[("SOCKET_TELEMETRY_DISABLED", ambient)], || { + let env = effective_env(&hermetic::command(Path::new("socket-patch"))); + assert_eq!( + get(&env, "SOCKET_TELEMETRY_DISABLED"), + Some("1"), + "ambient SOCKET_TELEMETRY_DISABLED={ambient:?} must not re-enable telemetry" + ); + }); + } + let mut cmd = hermetic::command(Path::new("socket-patch")); + cmd.env("SOCKET_TELEMETRY_DISABLED", "0"); + assert_eq!( + get(&effective_env(&cmd), "SOCKET_TELEMETRY_DISABLED"), + Some("0"), + "a telemetry suite's own opt-in lands last" + ); +} + +/// B69: every process `cargo test` / `cargo run` starts (and every child it +/// spawns without a scrub) inherits the three opt-outs from the workspace +/// `.cargo/config.toml` `[env]` table. +#[test] +fn cargo_config_env_carries_the_opt_outs() { + let config = Path::new(env!("CARGO_MANIFEST_DIR")).join("../../.cargo/config.toml"); + let text = std::fs::read_to_string(&config) + .unwrap_or_else(|e| panic!("read {}: {e}", config.display())) + .replace("\r\n", "\n"); + let env_table = text + .split("\n[env]\n") + .nth(1) + .unwrap_or_else(|| panic!("{} has no [env] table", config.display())); + let env_table = env_table.split("\n[").next().unwrap_or(env_table); + for var in [ + "SOCKET_NO_CONFIG", + "SOCKET_NO_UPDATE_CHECK", + "SOCKET_TELEMETRY_DISABLED", + ] { + assert!( + env_table + .lines() + .any(|line| line.trim() == format!("{var} = \"1\"")), + "{} [env] must set {var} = \"1\"", + config.display() + ); + } +} + #[test] #[serial] fn command_never_leaks_its_hostile_seeds() { From 1c788ccdae2dd82307cb5b96b5e79063515acf15 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Wed, 7 Oct 2026 10:56:06 -0400 Subject: [PATCH 2/6] Fail every e2e leg that runs no test, and run npm on Windows The e2e runner failed a suite on "0 passed" only in Gradle legs, so a renamed module, a dropped #[ignore] under the default --ignored selector or a stale test_filter turned any other leg into a green 0/0. Apply the check to every suite in every leg (e2e and e2e-full share the steps). Every e2e and e2e-full leg on recent runs passes at least one test. The Windows e2e_redirect_npm_build leg skipped every test: the suite spawned a bare "npm", which Command::new cannot resolve to the npm.cmd shim, and the leg had no npm_required. Resolve npm from PATH with utils::process::resolve_tool (PATHEXT-aware, the CLI's own lookup) and set npm_required on the Windows row too (audit B70). Drops the now-unused E2E_JVM_TOOL env. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/workflows/ci.yml | 17 +++++++++-------- .../tests/npm_e2e_common/mod.rs | 13 +++++++++++-- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e0b14d8d4..f300adcdc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -956,9 +956,8 @@ jobs: # setup-node step, no version change. # # `npm_required` turns the npm suites' "npm not installed" soft-skip - # into a hard failure. Not on Windows: `Command::new("npm")` cannot - # resolve `npm.cmd` there, so that leg still skips (a known gap; see - # docs/testing/npm-compatibility.md). + # into a hard failure, on every OS: the suite resolves npm through + # PATHEXT, so Windows finds the `npm.cmd` shim instead of skipping. - os: ubuntu-latest suite: e2e_redirect_npm_build npm_required: '1' @@ -967,6 +966,7 @@ jobs: npm_required: '1' - os: windows-latest suite: e2e_redirect_npm_build + npm_required: '1' - os: ubuntu-latest suite: e2e_redirect_rush_sim - os: macos-latest @@ -1617,14 +1617,15 @@ jobs: SOCKET_PATCH_SBT_E2E_VERSION: ${{ matrix.sbt }} E2E_SUITE: ${{ matrix.suite }} E2E_TEST_FILTER: ${{ matrix.test_filter || '--ignored' }} - E2E_JVM_TOOL: ${{ matrix.jvm_tool }} E2E_ALLOW_EMPTY: ${{ matrix.allow_empty }} shell: bash # Runs from the package root with CARGO_MANIFEST_DIR set, as # `cargo test` would. `suite` may name several binaries; an # `allow_empty` row skips the ones whose test file has not landed. - # Every suite a Gradle leg runs must run at least one test (per - # suite, so one suite's tests never hide another's empty filter). + # Every suite a leg runs must run at least one test (per suite, so + # one suite's tests never hide another's empty filter): a renamed + # module, a dropped `#[ignore]` under the default `--ignored` + # selector or a stale `test_filter` would otherwise pass as 0/0. run: | set -uo pipefail exe='' @@ -1644,8 +1645,8 @@ jobs: continue fi passed=$(sed -n 's/^test result: .* \([0-9][0-9]*\) passed;.*/\1/p' "$log" | head -n 1) - if [ "$E2E_JVM_TOOL" = gradle ] && [ "${passed:-0}" = 0 ]; then - echo "::error::$suite ran no test in this Gradle leg ($E2E_TEST_FILTER)" + if [ "${passed:-0}" = 0 ]; then + echo "::error::$suite ran no test in this leg (filter: $E2E_TEST_FILTER). Fix the row's suite or test_filter so it selects the tests it is meant to run." status=1 fi done diff --git a/crates/socket-patch-cli/tests/npm_e2e_common/mod.rs b/crates/socket-patch-cli/tests/npm_e2e_common/mod.rs index 60c2fc028..03796292f 100644 --- a/crates/socket-patch-cli/tests/npm_e2e_common/mod.rs +++ b/crates/socket-patch-cli/tests/npm_e2e_common/mod.rs @@ -62,7 +62,16 @@ fn env_set(name: &str) -> Option { /// The npm executable under test. pub fn npm_program() -> OsString { - env_set(BIN_ENV).unwrap_or_else(|| "npm".into()) + env_set(BIN_ENV).unwrap_or_else(path_npm) +} + +/// `npm` from `PATH`, resolved the way the CLI resolves its own tools +/// (`PATHEXT` on Windows). A bare `Command::new("npm")` cannot spawn the +/// `npm.cmd` shim Windows installs, so without this every Windows run +/// skipped as "npm not installed". +fn path_npm() -> OsString { + socket_patch_core::utils::process::resolve_tool("npm") + .map_or_else(|| "npm".into(), PathBuf::into_os_string) } /// `SOCKET_PATCH_NPM_E2E_REQUIRED` is set (CI matrix legs). @@ -137,7 +146,7 @@ pub fn npm_version() -> Option { /// `SOCKET_PATCH_NPM_E2E_LOCK_WRITER_BIN`, else `npm` from `PATH` when it /// is not the npm under test and reports major >= 7. pub fn modern_npm_writer() -> Option { - let candidate = env_set("SOCKET_PATCH_NPM_E2E_LOCK_WRITER_BIN").unwrap_or_else(|| "npm".into()); + let candidate = env_set("SOCKET_PATCH_NPM_E2E_LOCK_WRITER_BIN").unwrap_or_else(path_npm); let probe = tempfile::tempdir().unwrap(); let out = npm_command_for(&candidate, probe.path()) .arg("--version") From 89fa42ea7c9ae9587ba13a8a103c5f580be42c6f Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Wed, 7 Oct 2026 10:56:11 -0400 Subject: [PATCH 3/6] Make the pending-list ratchets say what to fix The digest ratchet failed with a bare assert_eq of two lists, so a stale entry (three of them reddened main after a merge burst, B01) read the same as a new inline copy. Split it the way the spawn-hygiene ratchets already do, and make all three messages name the files, the list constant and its source file, and the fix: use the shared helper for a new copy, delete the entry (after a rebase) for a stale one. The stale-entry check stays. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../tests/spawn_env_hygiene.rs | 25 +++++++++++----- crates/socket-patch-core/src/utils/digest.rs | 30 ++++++++++++++++--- 2 files changed, 43 insertions(+), 12 deletions(-) diff --git a/crates/socket-patch-cli/tests/spawn_env_hygiene.rs b/crates/socket-patch-cli/tests/spawn_env_hygiene.rs index 58c5146fb..6a268d22f 100644 --- a/crates/socket-patch-cli/tests/spawn_env_hygiene.rs +++ b/crates/socket-patch-cli/tests/spawn_env_hygiene.rs @@ -515,8 +515,10 @@ fn no_new_private_scrub_socket_env_copies() { .collect(); assert!( unexpected.is_empty(), - "spawn through `common/hermetic.rs` (hermetic::command + scrub_extra) \ - instead of a private scrub_socket_env: {unexpected:?}" + "these test files define a private scrub_socket_env: {unexpected:?}. \ + Spawn through `tests/common/hermetic.rs` instead (hermetic::command \ + for the binary, scrub_socket_vars + scrub_extra for package-manager \ + children). Do not add them to PENDING_SCRUB_COPIES." ); let stale: Vec<&&str> = PENDING_SCRUB_COPIES .iter() @@ -524,8 +526,10 @@ fn no_new_private_scrub_socket_env_copies() { .collect(); assert!( stale.is_empty(), - "these files no longer carry a scrub_socket_env copy; drop them from \ - PENDING_SCRUB_COPIES: {stale:?}" + "these files no longer carry a scrub_socket_env copy: {stale:?}. Delete \ + them from PENDING_SCRUB_COPIES in \ + crates/socket-patch-cli/tests/spawn_env_hygiene.rs (another PR may \ + have migrated them; rebase and drop the entries)." ); } @@ -542,8 +546,11 @@ fn no_new_bare_binary_spawns() { .collect(); assert!( unexpected.is_empty(), - "spawn the binary through `hermetic::command` / `common::run*`, not a \ - bare Command::new: {unexpected:?}" + "these test files spawn the binary with a bare Command::new: \ + {unexpected:?}. Use `hermetic::command(&bin)` / \ + `hermetic::binary_command()` or `common::run*` (tests/common/) so the \ + child gets the hermetic SOCKET_* environment. Do not add them to \ + PENDING_RAW_SPAWNS." ); let stale: Vec<&&str> = PENDING_RAW_SPAWNS .iter() @@ -551,7 +558,9 @@ fn no_new_bare_binary_spawns() { .collect(); assert!( stale.is_empty(), - "these files no longer spawn the binary bare; drop them from \ - PENDING_RAW_SPAWNS: {stale:?}" + "these files no longer spawn the binary bare: {stale:?}. Delete them \ + from PENDING_RAW_SPAWNS in \ + crates/socket-patch-cli/tests/spawn_env_hygiene.rs (another PR may \ + have migrated them; rebase and drop the entries)." ); } diff --git a/crates/socket-patch-core/src/utils/digest.rs b/crates/socket-patch-core/src/utils/digest.rs index 630adefa1..0ef654449 100644 --- a/crates/socket-patch-core/src/utils/digest.rs +++ b/crates/socket-patch-core/src/utils/digest.rs @@ -194,9 +194,31 @@ mod tests { } } inline.sort(); - assert_eq!( - inline, PENDING_INLINE_DIGESTS, - "production files computing digests inline differ from the pending list" - ); + let new: Vec<&String> = inline + .iter() + .filter(|f| !PENDING_INLINE_DIGESTS.contains(&f.as_str())) + .collect(); + let stale: Vec<&&str> = PENDING_INLINE_DIGESTS + .iter() + .filter(|f| !inline.iter().any(|g| g == *f)) + .collect(); + let mut problems = Vec::new(); + if !new.is_empty() { + problems.push(format!( + "these files compute a digest inline: {new:?}. Call the `*_of` \ + helpers in crates/socket-patch-core/src/utils/digest.rs instead \ + (sha256_hex_of, sha1_hex_of, sha512_base64_of, sha512_sri_of). \ + Do not add them to PENDING_INLINE_DIGESTS." + )); + } + if !stale.is_empty() { + problems.push(format!( + "these files no longer compute a digest inline: {stale:?}. Delete \ + them from PENDING_INLINE_DIGESTS in \ + crates/socket-patch-core/src/utils/digest.rs (another PR may \ + have moved them onto the helpers; rebase and drop the entries)." + )); + } + assert!(problems.is_empty(), "{}", problems.join("\n")); } } From 23065c9dda7d5375ec9506f711653acd99c68d37 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Wed, 7 Oct 2026 11:33:46 -0400 Subject: [PATCH 4/6] Keep the apply and rollback parse tests off the ambient env clap reads every env-bound flag at parse time, and these two suites parsed argv with whatever SOCKET_* the test process carried. With the workspace SOCKET_TELEMETRY_DISABLED=1 default every parse came back with --no-telemetry set. Add hermetic::scrub_process_socket_env, the in-process half of hermetic::command (one-shot, so no parse reads the env while it changes), and route every parse in both suites through it. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../socket-patch-cli/tests/cli_parse_apply.rs | 22 +++++++++++++---- .../tests/cli_parse_rollback.rs | 24 +++++++++++++++---- .../socket-patch-cli/tests/common/hermetic.rs | 19 +++++++++++++++ 3 files changed, 56 insertions(+), 9 deletions(-) diff --git a/crates/socket-patch-cli/tests/cli_parse_apply.rs b/crates/socket-patch-cli/tests/cli_parse_apply.rs index f68ae72d5..564f3c56e 100644 --- a/crates/socket-patch-cli/tests/cli_parse_apply.rs +++ b/crates/socket-patch-cli/tests/cli_parse_apply.rs @@ -12,13 +12,25 @@ use clap::Parser; use socket_patch_cli::commands::apply::ApplyArgs; use socket_patch_cli::{Cli, Commands}; +#[path = "common/hermetic.rs"] +mod hermetic; + +/// `Cli::try_parse_from` with the ambient `SOCKET_*` environment removed +/// first: clap reads the env-bound flags at parse time, so without it the +/// workspace `SOCKET_TELEMETRY_DISABLED=1` default (or a developer's shell) +/// would set `--no-telemetry` and friends in every parse. +fn try_parse(argv: &[&str]) -> Result { + hermetic::scrub_process_socket_env(); + Cli::try_parse_from(argv) +} + /// Parse `socket-patch apply ` and return the inner `ApplyArgs`. /// Panics if parsing fails or yields a non-`Apply` subcommand — tests for -/// the failure path call `Cli::try_parse_from` directly. +/// the failure path call [`try_parse`] directly. fn parse_apply(extra: &[&str]) -> ApplyArgs { let mut argv: Vec<&str> = vec!["socket-patch", "apply"]; 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::Apply(a) => a, _ => panic!("expected Apply"), @@ -305,7 +317,7 @@ fn no_telemetry_long() { /// the trailing token is rejected as an unknown argument. #[test] fn bare_bool_does_not_consume_next_token() { - match Cli::try_parse_from(["socket-patch", "apply", "--force", "stray"]) { + match try_parse(&["socket-patch", "apply", "--force", "stray"]) { Ok(_) => panic!("`--force stray` must reject the stray positional"), Err(err) => assert_eq!(err.kind(), clap::error::ErrorKind::UnknownArgument), } @@ -563,7 +575,7 @@ fn download_mode_values_are_not_normalized() { /// expectation to assert an `InvalidValue` error. #[test] fn download_mode_invalid_value_is_only_caught_at_runtime() { - match Cli::try_parse_from(["socket-patch", "apply", "--download-mode", "totally-bogus"]) { + match try_parse(&["socket-patch", "apply", "--download-mode", "totally-bogus"]) { Ok(cli) => match cli.command { Commands::Apply(a) => assert_eq!( a.common.download_mode, "totally-bogus", @@ -588,7 +600,7 @@ fn download_mode_invalid_value_is_only_caught_at_runtime() { fn unknown_flag_fails_with_unknown_argument() { // `Cli` doesn't implement `Debug`, so we can't use `.expect_err()` — // match the Result by hand. - match Cli::try_parse_from(["socket-patch", "apply", "--unknown-flag"]) { + match try_parse(&["socket-patch", "apply", "--unknown-flag"]) { Ok(_) => panic!("--unknown-flag must be rejected"), Err(err) => { assert_eq!(err.kind(), clap::error::ErrorKind::UnknownArgument); diff --git a/crates/socket-patch-cli/tests/cli_parse_rollback.rs b/crates/socket-patch-cli/tests/cli_parse_rollback.rs index c8b77af5e..f78c814be 100644 --- a/crates/socket-patch-cli/tests/cli_parse_rollback.rs +++ b/crates/socket-patch-cli/tests/cli_parse_rollback.rs @@ -12,10 +12,22 @@ use socket_patch_cli::commands::rollback::RollbackArgs; use socket_patch_cli::{Cli, Commands}; use std::path::PathBuf; +#[path = "common/hermetic.rs"] +mod hermetic; + +/// `Cli::try_parse_from` with the ambient `SOCKET_*` environment removed +/// first: clap reads the env-bound flags at parse time, so without it the +/// workspace `SOCKET_TELEMETRY_DISABLED=1` default (or a developer's shell) +/// would set `--no-telemetry` and friends in every parse. +fn try_parse(argv: &[&str]) -> Result { + hermetic::scrub_process_socket_env(); + Cli::try_parse_from(argv) +} + fn parse_rollback(extra: &[&str]) -> RollbackArgs { let mut argv = vec!["socket-patch", "rollback"]; 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::Rollback(a) => a, _ => panic!("expected Rollback"), @@ -366,7 +378,11 @@ fn bare_bool_does_not_consume_next_token() { /// relied on the rejection get a test-visible flip instead of a silent one. #[test] fn multiple_targets_parse_in_order() { - let args = parse_rollback(&["pkg:npm/foo@1", "packages/api/**", "b0630680-4da6-45f9-bba8-b888e0ffd58c"]); + let args = parse_rollback(&[ + "pkg:npm/foo@1", + "packages/api/**", + "b0630680-4da6-45f9-bba8-b888e0ffd58c", + ]); assert_eq!( args.targets, vec![ @@ -398,7 +414,7 @@ fn preserve_state_does_not_consume_next_token() { #[test] fn unknown_flag_fails() { - let err = match Cli::try_parse_from(["socket-patch", "rollback", "--unknown-flag"]) { + let err = match try_parse(&["socket-patch", "rollback", "--unknown-flag"]) { Ok(_) => panic!("expected parse failure"), Err(e) => e, }; @@ -407,7 +423,7 @@ fn unknown_flag_fails() { #[test] fn removed_one_off_flag_is_unknown() { - let err = match Cli::try_parse_from(["socket-patch", "rollback", "--one-off"]) { + let err = match try_parse(&["socket-patch", "rollback", "--one-off"]) { Ok(_) => panic!("expected parse error for the v5-removed --one-off"), Err(e) => e, }; diff --git a/crates/socket-patch-cli/tests/common/hermetic.rs b/crates/socket-patch-cli/tests/common/hermetic.rs index b7f324009..89f126dc9 100644 --- a/crates/socket-patch-cli/tests/common/hermetic.rs +++ b/crates/socket-patch-cli/tests/common/hermetic.rs @@ -103,6 +103,25 @@ pub fn scrub_socket_vars(cmd: &mut Command) { } } +/// The in-process half, for test binaries that only parse argv with clap: +/// clap reads every env-bound `SOCKET_*` flag at parse time, so an ambient +/// value (or a `.cargo/config.toml` `[env]` default such as +/// `SOCKET_TELEMETRY_DISABLED=1`) changes what a parse yields. Removes every +/// `SOCKET_*` var from this process, once; later calls wait for the first. +/// Nothing is restored, so call it only from binaries where no test needs a +/// `SOCKET_*` var, and call it before every parse so no parse reads the +/// environment while it is being changed. +pub fn scrub_process_socket_env() { + static ONCE: std::sync::Once = std::sync::Once::new(); + ONCE.call_once(|| { + for (key, _) in std::env::vars_os() { + if key.to_string_lossy().starts_with("SOCKET_") { + std::env::remove_var(&key); + } + } + }); +} + /// Opt-in scrubs for the ambient config of the tools a suite drives. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Extra { From 4c4ee5d854e1d5c97633301e66572165f9fcbc87 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Wed, 7 Oct 2026 12:23:45 -0400 Subject: [PATCH 5/6] Turn telemetry off in CI legs that run test binaries directly The e2e, e2e-full, cargo-vex-matrix(-full) jobs and the nine *-compatibility workflows launch the prebuilt test binaries themselves, so .cargo/config.toml [env] never reaches them; their env blocks copy that table by hand and were missing SOCKET_TELEMETRY_DISABLED. Add it to every copy, and add a test that fails when a workflow env block sets one of the three opt-outs without the other two (B69). Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/workflows/ci.yml | 4 + .github/workflows/composer-compatibility.yml | 1 + .github/workflows/go-compatibility.yml | 1 + .github/workflows/gradle-compatibility.yml | 1 + .github/workflows/npm-compatibility.yml | 1 + .github/workflows/pdm-compatibility.yml | 1 + .github/workflows/pipenv-compatibility.yml | 1 + .github/workflows/pnpm-compatibility.yml | 1 + .github/workflows/poetry-compatibility.yml | 1 + .github/workflows/sbt-compatibility.yml | 1 + .../tests/spawn_env_hygiene.rs | 76 +++++++++++++++++++ 11 files changed, 89 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f300adcdc..e580f1565 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1252,6 +1252,7 @@ jobs: env: SOCKET_NO_CONFIG: '1' SOCKET_NO_UPDATE_CHECK: '1' + SOCKET_TELEMETRY_DISABLED: '1' # e2e-full runs these same steps over its rows. steps: &e2e-steps - name: Checkout @@ -1737,6 +1738,7 @@ jobs: env: SOCKET_NO_CONFIG: '1' SOCKET_NO_UPDATE_CHECK: '1' + SOCKET_TELEMETRY_DISABLED: '1' steps: *e2e-steps # ---------------------------------------------------------------------- @@ -1945,6 +1947,7 @@ jobs: env: SOCKET_NO_CONFIG: '1' SOCKET_NO_UPDATE_CHECK: '1' + SOCKET_TELEMETRY_DISABLED: '1' strategy: fail-fast: false matrix: @@ -2016,6 +2019,7 @@ jobs: env: SOCKET_NO_CONFIG: '1' SOCKET_NO_UPDATE_CHECK: '1' + SOCKET_TELEMETRY_DISABLED: '1' strategy: fail-fast: false matrix: diff --git a/.github/workflows/composer-compatibility.yml b/.github/workflows/composer-compatibility.yml index e19c35340..f0fb21e4e 100644 --- a/.github/workflows/composer-compatibility.yml +++ b/.github/workflows/composer-compatibility.yml @@ -75,6 +75,7 @@ concurrency: env: SOCKET_NO_CONFIG: '1' SOCKET_NO_UPDATE_CHECK: '1' + SOCKET_TELEMETRY_DISABLED: '1' jobs: native: diff --git a/.github/workflows/go-compatibility.yml b/.github/workflows/go-compatibility.yml index 3f0e6b9b7..9210fd496 100644 --- a/.github/workflows/go-compatibility.yml +++ b/.github/workflows/go-compatibility.yml @@ -35,6 +35,7 @@ concurrency: env: SOCKET_NO_CONFIG: '1' SOCKET_NO_UPDATE_CHECK: '1' + SOCKET_TELEMETRY_DISABLED: '1' jobs: go: diff --git a/.github/workflows/gradle-compatibility.yml b/.github/workflows/gradle-compatibility.yml index 9b17fe9b1..1d5903bdd 100644 --- a/.github/workflows/gradle-compatibility.yml +++ b/.github/workflows/gradle-compatibility.yml @@ -94,6 +94,7 @@ concurrency: env: SOCKET_NO_CONFIG: '1' SOCKET_NO_UPDATE_CHECK: '1' + SOCKET_TELEMETRY_DISABLED: '1' jobs: build: diff --git a/.github/workflows/npm-compatibility.yml b/.github/workflows/npm-compatibility.yml index 40d7a25f7..aef8b5632 100644 --- a/.github/workflows/npm-compatibility.yml +++ b/.github/workflows/npm-compatibility.yml @@ -117,6 +117,7 @@ jobs: SOCKET_PATCH_NPM_E2E_REQUIRED: '1' SOCKET_NO_CONFIG: '1' SOCKET_NO_UPDATE_CHECK: '1' + SOCKET_TELEMETRY_DISABLED: '1' run: | chmod +x target/debug/socket-patch target/debug/e2e_redirect_npm_build target/debug/e2e_vendor_npm_build target/debug/e2e_redirect_npm_build --include-ignored --test-threads 4 diff --git a/.github/workflows/pdm-compatibility.yml b/.github/workflows/pdm-compatibility.yml index 6f760d7c8..1013948c6 100644 --- a/.github/workflows/pdm-compatibility.yml +++ b/.github/workflows/pdm-compatibility.yml @@ -56,6 +56,7 @@ concurrency: env: SOCKET_NO_CONFIG: '1' SOCKET_NO_UPDATE_CHECK: '1' + SOCKET_TELEMETRY_DISABLED: '1' PDM_CHECK_UPDATE: 'false' jobs: diff --git a/.github/workflows/pipenv-compatibility.yml b/.github/workflows/pipenv-compatibility.yml index c36335391..68c87964b 100644 --- a/.github/workflows/pipenv-compatibility.yml +++ b/.github/workflows/pipenv-compatibility.yml @@ -86,6 +86,7 @@ jobs: env: SOCKET_NO_CONFIG: '1' SOCKET_NO_UPDATE_CHECK: '1' + SOCKET_TELEMETRY_DISABLED: '1' # The runners carry whatever patch release uv ships; the harness # only needs one 3.8 and one 3.12. BACKTEST_PY38: '3.8' diff --git a/.github/workflows/pnpm-compatibility.yml b/.github/workflows/pnpm-compatibility.yml index f742364c4..37da0e08f 100644 --- a/.github/workflows/pnpm-compatibility.yml +++ b/.github/workflows/pnpm-compatibility.yml @@ -118,6 +118,7 @@ jobs: SOCKET_PATCH_PNPM_E2E_REQUIRED: '1' SOCKET_NO_CONFIG: '1' SOCKET_NO_UPDATE_CHECK: '1' + SOCKET_TELEMETRY_DISABLED: '1' run: | chmod +x bin/socket-patch bin/pnpm-e2e bin/pnpm-vendor-e2e export SOCKET_PATCH_PNPM_E2E_SOCKET_BIN="$PWD/bin/socket-patch" diff --git a/.github/workflows/poetry-compatibility.yml b/.github/workflows/poetry-compatibility.yml index 3b397b89f..fcad29e07 100644 --- a/.github/workflows/poetry-compatibility.yml +++ b/.github/workflows/poetry-compatibility.yml @@ -49,6 +49,7 @@ concurrency: env: SOCKET_NO_CONFIG: '1' SOCKET_NO_UPDATE_CHECK: '1' + SOCKET_TELEMETRY_DISABLED: '1' jobs: build: diff --git a/.github/workflows/sbt-compatibility.yml b/.github/workflows/sbt-compatibility.yml index 22ea35660..2b646370a 100644 --- a/.github/workflows/sbt-compatibility.yml +++ b/.github/workflows/sbt-compatibility.yml @@ -65,6 +65,7 @@ concurrency: env: SOCKET_NO_CONFIG: '1' SOCKET_NO_UPDATE_CHECK: '1' + SOCKET_TELEMETRY_DISABLED: '1' jobs: image: diff --git a/crates/socket-patch-cli/tests/spawn_env_hygiene.rs b/crates/socket-patch-cli/tests/spawn_env_hygiene.rs index 6a268d22f..2447d2091 100644 --- a/crates/socket-patch-cli/tests/spawn_env_hygiene.rs +++ b/crates/socket-patch-cli/tests/spawn_env_hygiene.rs @@ -327,6 +327,82 @@ fn cargo_config_env_carries_the_opt_outs() { } } +/// B69: CI legs that run the prebuilt test binaries directly (not through +/// cargo) do not get the `[env]` table, so their workflow env blocks copy it +/// by hand. Any env block that copies one opt-out must copy all three. +#[test] +fn workflow_env_copies_carry_every_opt_out() { + const OPT_OUTS: [&str; 3] = [ + "SOCKET_NO_CONFIG", + "SOCKET_NO_UPDATE_CHECK", + "SOCKET_TELEMETRY_DISABLED", + ]; + let dir = Path::new(env!("CARGO_MANIFEST_DIR")).join("../../.github/workflows"); + let mut entries: Vec<_> = std::fs::read_dir(&dir) + .unwrap_or_else(|e| panic!("read {}: {e}", dir.display())) + .map(|e| e.expect("workflow dir entry").path()) + .filter(|p| p.extension().is_some_and(|x| x == "yml" || x == "yaml")) + .collect(); + entries.sort(); + let mut copies = 0; + let mut missing = Vec::new(); + for path in entries { + let text = std::fs::read_to_string(&path) + .unwrap_or_else(|e| panic!("read {}: {e}", path.display())) + .replace("\r\n", "\n"); + let lines: Vec<&str> = text.lines().collect(); + // An env block is a run of `KEY: value` lines at one indent; find + // every run that sets any opt-out and check it sets them all. + let mut i = 0; + while i < lines.len() { + let line = lines[i]; + let Some(var) = OPT_OUTS + .iter() + .find(|v| line.trim_start().starts_with(&format!("{v}:"))) + else { + i += 1; + continue; + }; + let indent = line.len() - line.trim_start().len(); + let same_block = |l: &&str| { + let t = l.trim_start(); + l.len() - t.len() == indent && !t.is_empty() && !t.starts_with('-') + }; + let mut start = i; + while start > 0 && same_block(&lines[start - 1]) { + start -= 1; + } + let mut end = i; + while end + 1 < lines.len() && same_block(&lines[end + 1]) { + end += 1; + } + let block = &lines[start..=end]; + copies += 1; + for want in OPT_OUTS { + let set = block.iter().any(|l| l.trim() == format!("{want}: '1'")); + if !set { + missing.push(format!( + "{}:{} (block with {var}) lacks {want}: '1'", + path.display(), + start + 1 + )); + } + } + i = end + 1; + } + } + assert!( + copies > 0, + "found no workflow env copy of the [env] opt-outs" + ); + assert!( + missing.is_empty(), + "workflow env blocks that copy .cargo/config.toml [env] must set all \ + three opt-outs:\n{}", + missing.join("\n") + ); +} + #[test] #[serial] fn command_never_leaks_its_hostile_seeds() { From a076303eb39a0d54646f0dcf87e806be3556cb6d Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Wed, 7 Oct 2026 12:23:45 -0400 Subject: [PATCH 6/6] Share the parse suites' try_parse through hermetic.rs cli_parse_apply and cli_parse_rollback each defined the same scrubbed Cli::try_parse_from wrapper; keep one copy in common/hermetic.rs. Also rewrap the hermetic::command doc comment. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/socket-patch-cli/tests/cli_parse_apply.rs | 12 ++---------- .../socket-patch-cli/tests/cli_parse_rollback.rs | 12 ++---------- crates/socket-patch-cli/tests/common/hermetic.rs | 15 +++++++++++++-- 3 files changed, 17 insertions(+), 22 deletions(-) diff --git a/crates/socket-patch-cli/tests/cli_parse_apply.rs b/crates/socket-patch-cli/tests/cli_parse_apply.rs index 564f3c56e..642c70db3 100644 --- a/crates/socket-patch-cli/tests/cli_parse_apply.rs +++ b/crates/socket-patch-cli/tests/cli_parse_apply.rs @@ -8,21 +8,13 @@ use std::path::PathBuf; -use clap::Parser; use socket_patch_cli::commands::apply::ApplyArgs; -use socket_patch_cli::{Cli, Commands}; +use socket_patch_cli::Commands; #[path = "common/hermetic.rs"] mod hermetic; -/// `Cli::try_parse_from` with the ambient `SOCKET_*` environment removed -/// first: clap reads the env-bound flags at parse time, so without it the -/// workspace `SOCKET_TELEMETRY_DISABLED=1` default (or a developer's shell) -/// would set `--no-telemetry` and friends in every parse. -fn try_parse(argv: &[&str]) -> Result { - hermetic::scrub_process_socket_env(); - Cli::try_parse_from(argv) -} +use hermetic::try_parse; /// Parse `socket-patch apply ` and return the inner `ApplyArgs`. /// Panics if parsing fails or yields a non-`Apply` subcommand — tests for diff --git a/crates/socket-patch-cli/tests/cli_parse_rollback.rs b/crates/socket-patch-cli/tests/cli_parse_rollback.rs index f78c814be..8e4b7dcde 100644 --- a/crates/socket-patch-cli/tests/cli_parse_rollback.rs +++ b/crates/socket-patch-cli/tests/cli_parse_rollback.rs @@ -7,22 +7,14 @@ //! these tests is a breaking change and requires a MAJOR bump per //! `crates/socket-patch-cli/CLI_CONTRACT.md`. -use clap::Parser; use socket_patch_cli::commands::rollback::RollbackArgs; -use socket_patch_cli::{Cli, Commands}; +use socket_patch_cli::Commands; use std::path::PathBuf; #[path = "common/hermetic.rs"] mod hermetic; -/// `Cli::try_parse_from` with the ambient `SOCKET_*` environment removed -/// first: clap reads the env-bound flags at parse time, so without it the -/// workspace `SOCKET_TELEMETRY_DISABLED=1` default (or a developer's shell) -/// would set `--no-telemetry` and friends in every parse. -fn try_parse(argv: &[&str]) -> Result { - hermetic::scrub_process_socket_env(); - Cli::try_parse_from(argv) -} +use hermetic::try_parse; fn parse_rollback(extra: &[&str]) -> RollbackArgs { let mut argv = vec!["socket-patch", "rollback"]; diff --git a/crates/socket-patch-cli/tests/common/hermetic.rs b/crates/socket-patch-cli/tests/common/hermetic.rs index 89f126dc9..12c67c3ec 100644 --- a/crates/socket-patch-cli/tests/common/hermetic.rs +++ b/crates/socket-patch-cli/tests/common/hermetic.rs @@ -49,8 +49,9 @@ const HOSTILE_SEEDS: &[(&str, &str)] = &[ /// A `Command` for `bin` with the hermetic `SOCKET_*` environment: the /// hostile seeds scrubbed, every other ambient `SOCKET_*` removed (removing /// `SOCKET_API_TOKEN` also forces the public proxy), and the three opt-outs -/// (config layer, update notifier, telemetry) forced on. Callers add args, cwd and their own env afterwards; caller env -/// lands last, so explicit injections survive the scrub. +/// (config layer, update notifier, telemetry) forced on. Callers add args, +/// cwd and their own env afterwards; caller env lands last, so explicit +/// injections survive the scrub. pub fn command(bin: &Path) -> Command { let mut cmd = Command::new(bin); for (k, v) in HOSTILE_SEEDS { @@ -122,6 +123,16 @@ pub fn scrub_process_socket_env() { }); } +/// `Cli::try_parse_from` with the ambient `SOCKET_*` environment removed +/// first (see [`scrub_process_socket_env`]): clap reads the env-bound flags +/// at parse time, so without it the workspace `SOCKET_TELEMETRY_DISABLED=1` +/// default (or a developer's shell) would set `--no-telemetry` and friends +/// in every parse. +pub fn try_parse(argv: &[&str]) -> Result { + scrub_process_socket_env(); + ::try_parse_from(argv) +} + /// Opt-in scrubs for the ambient config of the tools a suite drives. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Extra {