From 68a19d2e98163b84b9023f857d3eb046a7c90d62 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Thu, 2 Jul 2026 17:47:41 -0400 Subject: [PATCH 1/4] =?UTF-8?q?fix(cli):=20unify=20env-bool=20parsing=20?= =?UTF-8?q?=E2=80=94=20one=20parser=20policy,=20scrub=20covers=20local=20v?= =?UTF-8?q?ars?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit parse_bool_flag everywhere: get --save-only/--one-off and remove --skip-rollback had clap's default bool-from-env (SOCKET_X=1 or an exported-but-empty var aborted the parse — same bug class fixed on rollback/repair/unlock/vex earlier); apply --check, setup --check/--remove, and get/scan --all-releases dropped BoolishValueParser (rejects VAR=). SOCKET_STRICT was missing from GLOBAL_ARG_ENV_VARS despite the list's 'every env var GlobalArgs binds' contract. New LOCAL_ARG_ENV_VARS mirrors that contract for subcommand flags; the main-run scrub and the test harness's clean-env helper now cover both lists. Two table-driven invariant tests parse every binding against its owning subcommand with '', '1', 'yes', 'garbage' so the policy self-enforces (RED-verified against the pre-fix get.rs). Co-Authored-By: Claude Fable 5 --- crates/socket-patch-cli/src/args.rs | 169 +++++++++++++++--- crates/socket-patch-cli/src/commands/apply.rs | 2 +- crates/socket-patch-cli/src/commands/get.rs | 22 ++- .../socket-patch-cli/src/commands/remove.rs | 9 +- crates/socket-patch-cli/src/commands/scan.rs | 2 +- crates/socket-patch-cli/src/commands/setup.rs | 4 +- crates/socket-patch-cli/src/main.rs | 9 +- .../socket-patch-cli/tests/cli_global_args.rs | 2 +- 8 files changed, 184 insertions(+), 35 deletions(-) diff --git a/crates/socket-patch-cli/src/args.rs b/crates/socket-patch-cli/src/args.rs index 0652fb94..1194a6e3 100644 --- a/crates/socket-patch-cli/src/args.rs +++ b/crates/socket-patch-cli/src/args.rs @@ -359,7 +359,7 @@ pub(crate) fn apply_env_toggles(common: &GlobalArgs) { } /// Every env var `GlobalArgs` binds (one per `env = "..."` attribute above). -/// Single source of truth for [`scrub_empty_global_env_vars`] and the +/// Single source of truth for [`scrub_empty_env_vars`] and the /// clean-environment test harnesses. pub const GLOBAL_ARG_ENV_VARS: &[&str] = &[ "SOCKET_CWD", @@ -374,6 +374,7 @@ pub const GLOBAL_ARG_ENV_VARS: &[&str] = &[ "SOCKET_VENDOR_URL", "SOCKET_PATCH_SERVER_URL", "SOCKET_OFFLINE", + "SOCKET_STRICT", "SOCKET_GLOBAL", "SOCKET_GLOBAL_PREFIX", "SOCKET_JSON", @@ -387,21 +388,48 @@ pub const GLOBAL_ARG_ENV_VARS: &[&str] = &[ "SOCKET_TELEMETRY_DISABLED", ]; -/// Remove exported-but-**empty** `GlobalArgs` env vars before clap parses. +/// Every env var a **subcommand-local** flag binds (one per `env = "..."` +/// attribute in `commands/*.rs`). Same contract as [`GLOBAL_ARG_ENV_VARS`]: +/// single source of truth for [`scrub_empty_env_vars`] and the +/// clean-environment test harnesses. A flag added with an `env` binding but +/// missing here escapes the empty-var scrub — the invariant tests below +/// parse every entry against its owning subcommand to keep this honest. +pub const LOCAL_ARG_ENV_VARS: &[&str] = &[ + "SOCKET_FORCE", + "SOCKET_SAVE_ONLY", + "SOCKET_ONE_OFF", + "SOCKET_ALL_RELEASES", + "SOCKET_SKIP_ROLLBACK", + "SOCKET_DOWNLOAD_ONLY", + "SOCKET_SETUP_EXCLUDE", + "SOCKET_VENDOR_REVERT", + "SOCKET_UNLOCK_RELEASE", + "SOCKET_BATCH_SIZE", + "SOCKET_VEX", + "SOCKET_VEX_OUTPUT", + "SOCKET_VEX_PRODUCT", + "SOCKET_VEX_NO_VERIFY", + "SOCKET_VEX_DOC_ID", + "SOCKET_VEX_COMPACT", +]; + +/// Remove exported-but-**empty** flag-bound env vars before clap parses. /// /// `SOCKET_CWD=` — the conventional shell/CI idiom for blanking a variable /// without unsetting it — must mean "unset, fall back to the default", not /// abort the command. [`parse_bool_flag`] already gives the bool flags that /// semantic, but clap rejects an empty `SOCKET_CWD` / `SOCKET_GLOBAL_PREFIX` -/// ("a value is required"), `SOCKET_LOCK_TIMEOUT` ("cannot parse integer -/// from empty string") and `SOCKET_ECOSYSTEMS` (the per-token validator) -/// outright — a single stray blank var crashed every subcommand — and an -/// empty `SOCKET_DOWNLOAD_MODE` / `SOCKET_MANIFEST_PATH` leaked `""` past -/// the documented defaults. Called from `main` after legacy-name promotion -/// and before clap runs. Only exactly-empty values are scrubbed; whitespace -/// is significant in paths, so it is left for the parsers to judge. -pub fn scrub_empty_global_env_vars() { - for &var in GLOBAL_ARG_ENV_VARS { +/// ("a value is required"), `SOCKET_LOCK_TIMEOUT` / `SOCKET_BATCH_SIZE` +/// ("cannot parse integer from empty string") and `SOCKET_ECOSYSTEMS` (the +/// per-token validator) outright — a single stray blank var crashed every +/// subcommand — and an empty `SOCKET_DOWNLOAD_MODE` / `SOCKET_MANIFEST_PATH` +/// (or `SOCKET_VEX_OUTPUT`, which would silently target `""`) leaked `""` +/// past the documented defaults. Called from `main` after legacy-name +/// promotion and before clap runs. Only exactly-empty values are scrubbed; +/// whitespace is significant in paths, so it is left for the parsers to +/// judge. +pub fn scrub_empty_env_vars() { + for &var in GLOBAL_ARG_ENV_VARS.iter().chain(LOCAL_ARG_ENV_VARS) { if matches!(std::env::var(var).as_deref(), Ok("")) { std::env::remove_var(var); } @@ -483,11 +511,14 @@ mod tests { } } - /// Clear every env var `GlobalArgs` reads (the production list, so the - /// scrub and the harness can't drift), giving each clap-parse test a - /// known-clean environment with no ambient `SOCKET_*` bleed-through. + /// Clear every env var a flag reads — global and subcommand-local (the + /// production lists, so the scrub and the harness can't drift), giving + /// each clap-parse test a known-clean environment with no ambient + /// `SOCKET_*` bleed-through. fn with_clean_socket_env(f: impl FnOnce()) { - with_env_cleared(GLOBAL_ARG_ENV_VARS, f); + with_env_cleared(GLOBAL_ARG_ENV_VARS, || { + with_env_cleared(LOCAL_ARG_ENV_VARS, f); + }); } /// Clear the extra env the core telemetry gate reads beyond the @@ -544,14 +575,14 @@ mod tests { }); } - /// `scrub_empty_global_env_vars` removes exactly-empty `SOCKET_*` globals - /// (the `VAR=` blank-without-unsetting idiom) and nothing else: set, - /// non-empty values — even whitespace-only ones, which are significant in - /// paths — survive, and the previously-crashing parse then sees plain - /// defaults. + /// `scrub_empty_env_vars` removes exactly-empty `SOCKET_*` flag vars + /// (the `VAR=` blank-without-unsetting idiom) — global and local — and + /// nothing else: set, non-empty values — even whitespace-only ones, + /// which are significant in paths — survive, and the + /// previously-crashing parse then sees plain defaults. #[test] #[serial_test::serial] - fn scrub_empty_global_env_vars_unsets_only_empties() { + fn scrub_empty_env_vars_unsets_only_empties() { with_clean_socket_env(|| { std::env::set_var("SOCKET_CWD", ""); std::env::set_var("SOCKET_LOCK_TIMEOUT", ""); @@ -559,16 +590,23 @@ mod tests { std::env::set_var("SOCKET_ECOSYSTEMS", ""); std::env::set_var("SOCKET_DOWNLOAD_MODE", ""); std::env::set_var("SOCKET_VENDOR_SOURCE", ""); + std::env::set_var("SOCKET_BATCH_SIZE", ""); + std::env::set_var("SOCKET_VEX_OUTPUT", ""); std::env::set_var("SOCKET_MANIFEST_PATH", "keep.json"); std::env::set_var("SOCKET_ORG_SLUG", " "); - scrub_empty_global_env_vars(); + scrub_empty_env_vars(); assert!( std::env::var("SOCKET_CWD").is_err(), "empty var is scrubbed" ); assert!(std::env::var("SOCKET_LOCK_TIMEOUT").is_err()); + assert!( + std::env::var("SOCKET_BATCH_SIZE").is_err(), + "empty subcommand-local vars are scrubbed too" + ); + assert!(std::env::var("SOCKET_VEX_OUTPUT").is_err()); assert_eq!( std::env::var("SOCKET_MANIFEST_PATH").as_deref(), Ok("keep.json"), @@ -985,4 +1023,91 @@ mod tests { ); }); } + + /// Policy invariant: EVERY env-bound boolean flag on every subcommand + /// parses with [`parse_bool_flag`] semantics — the boolish vocabulary is + /// accepted, exported-but-empty means false, garbage is a parse error. + /// Table-driven against the real `Cli` so a new flag added with clap's + /// default bool-from-env parser (accepts only `true`/`false` — the + /// recurring "`SOCKET_X=1` aborts the parse" bug class) or with + /// `BoolishValueParser` (rejects `VAR=`) fails here, not in the field. + #[test] + #[serial_test::serial] + fn every_env_bound_bool_flag_parses_boolishly_and_tolerates_empty() { + use clap::Parser as _; + + // (env var, argv of a subcommand that binds it) — every bool entry + // of `LOCAL_ARG_ENV_VARS`, on each subcommand that binds it. + const BOOL_BINDINGS: &[(&str, &[&str])] = &[ + ("SOCKET_FORCE", &["socket-patch", "apply"]), + ("SOCKET_FORCE", &["socket-patch", "vendor"]), + ("SOCKET_SAVE_ONLY", &["socket-patch", "get", "x"]), + ("SOCKET_ONE_OFF", &["socket-patch", "get", "x"]), + ("SOCKET_ONE_OFF", &["socket-patch", "rollback"]), + ("SOCKET_ALL_RELEASES", &["socket-patch", "get", "x"]), + ("SOCKET_ALL_RELEASES", &["socket-patch", "scan"]), + ("SOCKET_SKIP_ROLLBACK", &["socket-patch", "remove", "x"]), + ("SOCKET_DOWNLOAD_ONLY", &["socket-patch", "repair"]), + ("SOCKET_VENDOR_REVERT", &["socket-patch", "vendor"]), + ("SOCKET_UNLOCK_RELEASE", &["socket-patch", "unlock"]), + ("SOCKET_VEX_NO_VERIFY", &["socket-patch", "vex"]), + ("SOCKET_VEX_COMPACT", &["socket-patch", "vex"]), + // The embedded `--vex-*` twins share the same env vars and must + // not abort host commands (e.g. apply from a postinstall hook). + ("SOCKET_VEX_NO_VERIFY", &["socket-patch", "apply"]), + ("SOCKET_VEX_COMPACT", &["socket-patch", "scan"]), + ]; + + with_clean_socket_env(|| { + for &(var, argv) in BOOL_BINDINGS { + for (val, should_parse) in + [("", true), ("1", true), ("yes", true), ("garbage", false)] + { + std::env::set_var(var, val); + let result = crate::Cli::try_parse_from(argv.iter().copied()); + assert_eq!( + result.is_ok(), + should_parse, + "{var}={val:?} on {argv:?} — expected parse {}: {:?}", + if should_parse { "success" } else { "failure" }, + result.err().map(|e| e.to_string()), + ); + std::env::remove_var(var); + } + } + }); + } + + /// Companion invariant for the **value-typed** local env vars: an + /// exported-but-empty value (`VAR=`) must not crash its subcommand — + /// [`scrub_empty_env_vars`] (run by `main` before clap) removes it, and + /// the parse then sees plain defaults. + #[test] + #[serial_test::serial] + fn empty_value_typed_local_env_vars_are_rescued_by_the_scrub() { + use clap::Parser as _; + + const VALUE_BINDINGS: &[(&str, &[&str])] = &[ + ("SOCKET_BATCH_SIZE", &["socket-patch", "scan"]), + ("SOCKET_SETUP_EXCLUDE", &["socket-patch", "setup"]), + ("SOCKET_VEX", &["socket-patch", "apply"]), + ("SOCKET_VEX_OUTPUT", &["socket-patch", "vex"]), + ("SOCKET_VEX_PRODUCT", &["socket-patch", "vex"]), + ("SOCKET_VEX_DOC_ID", &["socket-patch", "vex"]), + ]; + + with_clean_socket_env(|| { + for &(var, argv) in VALUE_BINDINGS { + std::env::set_var(var, ""); + scrub_empty_env_vars(); + let result = crate::Cli::try_parse_from(argv.iter().copied()); + assert!( + result.is_ok(), + "{var}= (exported empty) on {argv:?} must be scrubbed, not abort: {:?}", + result.err().map(|e| e.to_string()), + ); + std::env::remove_var(var); + } + }); + } } diff --git a/crates/socket-patch-cli/src/commands/apply.rs b/crates/socket-patch-cli/src/commands/apply.rs index 13dd4837..5fe7b61e 100644 --- a/crates/socket-patch-cli/src/commands/apply.rs +++ b/crates/socket-patch-cli/src/commands/apply.rs @@ -154,7 +154,7 @@ pub struct ApplyArgs { #[arg( long = "check", default_value_t = false, - value_parser = clap::builder::BoolishValueParser::new(), + value_parser = crate::args::parse_bool_flag, )] pub check: bool, diff --git a/crates/socket-patch-cli/src/commands/get.rs b/crates/socket-patch-cli/src/commands/get.rs index 590955b1..bb5835d7 100644 --- a/crates/socket-patch-cli/src/commands/get.rs +++ b/crates/socket-patch-cli/src/commands/get.rs @@ -404,16 +404,32 @@ pub struct GetArgs { pub package: bool, /// Download patch without applying it. + /// + /// `value_parser = parse_bool_flag` matches the `GlobalArgs` bool flags: + /// clap's default bool parser accepts only the literal strings + /// `true`/`false` from the env binding, so `SOCKET_SAVE_ONLY=1` (or an + /// exported-but-empty `SOCKET_SAVE_ONLY=`) aborted every `get` + /// invocation. #[arg( long = "save-only", alias = "no-apply", env = "SOCKET_SAVE_ONLY", - default_value_t = false + default_value_t = false, + value_parser = crate::args::parse_bool_flag, )] pub save_only: bool, /// Apply patch immediately without saving to .socket folder. - #[arg(long = "one-off", env = "SOCKET_ONE_OFF", default_value_t = false)] + /// + /// `value_parser = parse_bool_flag`: same env-crash fix as `--save-only` + /// above — and `SOCKET_ONE_OFF` is shared with `rollback --one-off`, + /// which already parses boolishly; the two must not diverge. + #[arg( + long = "one-off", + env = "SOCKET_ONE_OFF", + default_value_t = false, + value_parser = crate::args::parse_bool_flag, + )] pub one_off: bool, /// Download patches for every release/distribution variant of a @@ -426,7 +442,7 @@ pub struct GetArgs { long = "all-releases", env = "SOCKET_ALL_RELEASES", default_value_t = false, - value_parser = clap::builder::BoolishValueParser::new(), + value_parser = crate::args::parse_bool_flag, )] pub all_releases: bool, } diff --git a/crates/socket-patch-cli/src/commands/remove.rs b/crates/socket-patch-cli/src/commands/remove.rs index 16a7bf25..67c965b3 100644 --- a/crates/socket-patch-cli/src/commands/remove.rs +++ b/crates/socket-patch-cli/src/commands/remove.rs @@ -88,10 +88,17 @@ pub struct RemoveArgs { pub common: GlobalArgs, /// Skip rolling back files before removing (only update manifest). + /// + /// `value_parser = parse_bool_flag` matches the `GlobalArgs` bool flags: + /// clap's default bool parser accepts only the literal strings + /// `true`/`false` from the env binding, so `SOCKET_SKIP_ROLLBACK=1` (or + /// an exported-but-empty `SOCKET_SKIP_ROLLBACK=`) aborted every + /// `remove` invocation. #[arg( long = "skip-rollback", env = "SOCKET_SKIP_ROLLBACK", - default_value_t = false + default_value_t = false, + value_parser = crate::args::parse_bool_flag, )] pub skip_rollback: bool, } diff --git a/crates/socket-patch-cli/src/commands/scan.rs b/crates/socket-patch-cli/src/commands/scan.rs index fa772b59..459f77f2 100644 --- a/crates/socket-patch-cli/src/commands/scan.rs +++ b/crates/socket-patch-cli/src/commands/scan.rs @@ -730,7 +730,7 @@ pub struct ScanArgs { long = "all-releases", env = "SOCKET_ALL_RELEASES", default_value_t = false, - value_parser = clap::builder::BoolishValueParser::new(), + value_parser = crate::args::parse_bool_flag, )] pub all_releases: bool, diff --git a/crates/socket-patch-cli/src/commands/setup.rs b/crates/socket-patch-cli/src/commands/setup.rs index c11899f1..d8d5a2f5 100644 --- a/crates/socket-patch-cli/src/commands/setup.rs +++ b/crates/socket-patch-cli/src/commands/setup.rs @@ -72,7 +72,7 @@ pub struct SetupArgs { long = "check", conflicts_with = "remove", default_value_t = false, - value_parser = clap::builder::BoolishValueParser::new(), + value_parser = crate::args::parse_bool_flag, )] pub check: bool, @@ -82,7 +82,7 @@ pub struct SetupArgs { #[arg( long = "remove", default_value_t = false, - value_parser = clap::builder::BoolishValueParser::new(), + value_parser = crate::args::parse_bool_flag, )] pub remove: bool, diff --git a/crates/socket-patch-cli/src/main.rs b/crates/socket-patch-cli/src/main.rs index 995a7267..de52a08a 100644 --- a/crates/socket-patch-cli/src/main.rs +++ b/crates/socket-patch-cli/src/main.rs @@ -8,10 +8,11 @@ async fn main() { // names. A one-shot deprecation warning fires per legacy name set. promote_legacy_env_vars(); - // Then drop exported-but-empty SOCKET_* globals (`SOCKET_CWD=` means - // "unset", not "crash the parse"). Must run after the promotion so a - // blanked legacy name is scrubbed too. - socket_patch_cli::args::scrub_empty_global_env_vars(); + // Then drop exported-but-empty SOCKET_* flag vars — global and + // subcommand-local (`SOCKET_CWD=` means "unset", not "crash the + // parse"). Must run after the promotion so a blanked legacy name is + // scrubbed too. + socket_patch_cli::args::scrub_empty_env_vars(); // The parser surface is `String`-typed, but argv is raw bytes on Unix — // `std::env::args()` would *panic* on a non-Unicode argument. Collect diff --git a/crates/socket-patch-cli/tests/cli_global_args.rs b/crates/socket-patch-cli/tests/cli_global_args.rs index 527a0fc3..8ddb0a2b 100644 --- a/crates/socket-patch-cli/tests/cli_global_args.rs +++ b/crates/socket-patch-cli/tests/cli_global_args.rs @@ -663,7 +663,7 @@ const GLOBAL_ENV_VARS: &[&str] = &[ /// "cannot parse integer from empty string"), and empty /// `SOCKET_DOWNLOAD_MODE=` / `SOCKET_MANIFEST_PATH=` leaked `""` past the /// documented defaults. The binary now scrubs empty `GlobalArgs` env vars -/// before clap parses (`args::scrub_empty_global_env_vars` in `main`), +/// before clap parses (`args::scrub_empty_env_vars` in `main`), /// restoring the documented CLI > env > default precedence for blank vars. /// This spawns the real binary because the scrub is `main` wiring. #[test] From a870d16146e79850ff8d7a905aa8dc7d2ff4cecd Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Thu, 2 Jul 2026 17:54:04 -0400 Subject: [PATCH 2/4] =?UTF-8?q?fix(remove):=20implement=20--dry-run=20?= =?UTF-8?q?=E2=80=94=20preview,=20no=20mutations?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit remove was the one mutating command that silently ignored the global --dry-run (contract row: 'Preview, no mutations') and deleted anyway. Now: lock/emit carry dry_run; the confirmation prompt is skipped (nothing to confirm); rollback and vendor-revert run their existing dry-run modes; the manifest removal is simulated in memory so the blob sweep previews against the post-removal reference set; the envelope reports dryRun:true with would-be Removed events flipped to Verified previews (the apply/vendor/repair convention) so summary.removed stays 'entries actually deleted'. Telemetry does not fire on previews. Contract lines 103/763 updated. +2 invariant tests (RED-verified against the old code). Co-Authored-By: Claude Fable 5 --- crates/.DS_Store | Bin 0 -> 6148 bytes crates/socket-patch-cli/.DS_Store | Bin 0 -> 6148 bytes crates/socket-patch-cli/CLI_CONTRACT.md | 4 +- .../socket-patch-cli/src/commands/remove.rs | 109 +++++++++++++++--- .../tests/remove_invariants.rs | 89 ++++++++++++++ crates/socket-patch-core/.DS_Store | Bin 0 -> 6148 bytes 6 files changed, 182 insertions(+), 20 deletions(-) create mode 100644 crates/.DS_Store create mode 100644 crates/socket-patch-cli/.DS_Store create mode 100644 crates/socket-patch-core/.DS_Store diff --git a/crates/.DS_Store b/crates/.DS_Store new file mode 100644 index 0000000000000000000000000000000000000000..68a0380967b756ea5b4b4e3eb6ca5e5c2420c7af GIT binary patch literal 6148 zcmeHKy-ve05I&a_rCg4PO#gD!BZ`SAf-`oO(R^S$&pv0JFv63@zhg`n zv`G~m0oDG30&@B1WGSaps^4Eu1@1DZl1|Fw-XOP=$_KRdas`a(8_vXL*|4-jtc}+8 zi|gCBr`>kE(L%hcsGV{9j*q(frCKc3H6;K753M|QE z%=v%s^ZLI@(kE3w75G;Qm~J-6`UolH*16y~*Cy~oI2*5Xf>R1QJ{4mvr{ZOPBif=Li literal 0 HcmV?d00001 diff --git a/crates/socket-patch-cli/.DS_Store b/crates/socket-patch-cli/.DS_Store new file mode 100644 index 0000000000000000000000000000000000000000..7efe91bbff08dd4fea37ba70cfe4e0ff1514752e GIT binary patch literal 6148 zcmeHKy-EW?5S}p+4n#<4=QctLo0Jy98qO!k3rG^tBF6<2`)h6NEqw)F!WXcz@)>Mx z{AOn~w|H2J=nU+Bv-7ia_rdMn5D|~B=Ody45fvDMEJ}xnxzn{}!Qy1uV>Fr7)iPOk z&G*|up1nsax}w?QqW%2)cN?NoAgXoBhhK-`^BKDLH^dEslh)>SI4>0f!Hmpf4 literal 0 HcmV?d00001 diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index 28b1e425..a1db1a67 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -100,7 +100,7 @@ The rewriter reads a fixed set of candidate files from the project root: the npm * `.socket/vendor/state.json` — the **vendored**-mode ledger (see "Ownership, state, and reversal" below): wiring edits with verbatim pre-vendor originals, artifact fingerprints, optional `detached` records. * `.socket/vendor/redirect-state.json` — the **hosted**-mode ledger (`RedirectState` in `socket-patch-core/src/patch/redirect/state.rs`): `{ version, mode: "hosted", edits[], records{} }`. `edits` are recorded `FileEdit`s (append-only across re-runs — merge, never clobber: the pre-redirect originals a future revert needs live here); `records` maps PURL → the full manifest `PatchRecord` so a post-install `vex` can attest redirected patches with no manifest entry. The `mode` string is opaque to the loader (pre-rename ledgers carrying `"redirect"` still load; a hosted re-run normalizes them to `"hosted"`). Written identically by this CLI and by the depscan backend's hosted PR flow (`github-patch-pr-hosted.ts`). -`--dry-run` previews what `apply` / `rollback` / `scan --apply` / `repair` would do without mutating disk. In JSON mode, the envelope is populated with would-be actions and counts. +`--dry-run` previews what `apply` / `rollback` / `scan --apply` / `repair` / `remove` would do without mutating disk. In JSON mode, the envelope is populated with would-be actions and counts (`remove --dry-run` skips the confirmation prompt — there is nothing to confirm — and flips its would-be `Removed` events to `Verified` previews, so `summary.removed` stays "entries actually deleted"). The hidden alias `--no-apply` on `get --save-only` is **part of the contract** — it does not appear in `--help` but is widely used in existing scripts. @@ -760,7 +760,7 @@ Every `--json` invocation emits a single JSON object that follows the **unified | `vendor` | `Applied` (= vendored; `command` routes) · `Skipped` (refusals, warnings, unsupported ecosystems) · `Failed` · `Removed` (reconcile + `--revert`) · `Verified` (dry-run) | | `list` | `Discovered` (with `details.vulnerabilities`, `details.tier`, `details.license`, `details.description`, `details.exportedAt`) | | `repair`/`gc`| `Downloaded` (or `Verified` on dry-run) · `Rebuilt` (vendored artifacts; `Verified` previews on dry-run) · `Skipped` (vendor_uuid_mismatch) · `Removed` (or `Verified`) · `Failed` events | -| `remove` | `Removed` (per purl) · artifact-level `Removed` event (with `details.blobsRemoved`, `details.rolledBack`) | +| `remove` | `Removed` (per purl; `Verified` on dry-run) · artifact-level `Removed`/`Verified` event (with `details.blobsRemoved`, `details.rolledBack`) | ### Migration status (v3.0) diff --git a/crates/socket-patch-cli/src/commands/remove.rs b/crates/socket-patch-cli/src/commands/remove.rs index 67c965b3..5d5c010a 100644 --- a/crates/socket-patch-cli/src/commands/remove.rs +++ b/crates/socket-patch-cli/src/commands/remove.rs @@ -132,7 +132,7 @@ pub async fn run(args: RemoveArgs) -> i32 { Command::Remove, args.common.json, args.common.silent, - false, // remove has no --dry-run + args.common.dry_run, Duration::from_secs(args.common.lock_timeout.unwrap_or(0)), args.common.break_lock, ) { @@ -228,8 +228,11 @@ pub async fn run(args: RemoveArgs) -> i32 { eprintln!(); } + // `--dry-run` previews without mutating, so there is nothing to + // confirm — skip the prompt (matching the global contract row: + // "Preview, no mutations"). let prompt = format!("Remove {} patch(es) and rollback files?", matching.len()); - if !confirm(&prompt, true, args.common.yes, args.common.json) { + if !args.common.dry_run && !confirm(&prompt, true, args.common.yes, args.common.json) { if !args.common.json && !args.common.silent { println!("Removal cancelled."); } @@ -246,7 +249,7 @@ pub async fn run(args: RemoveArgs) -> i32 { &args.common.cwd, &manifest_path, Some(&args.identifier), - false, + args.common.dry_run, args.common.json || args.common.silent, args.common.offline, args.common.global, @@ -360,7 +363,8 @@ pub async fn run(args: RemoveArgs) -> i32 { } } else { for (key, entry) in &vendored_matches { - let outcome = dispatch_revert_one(entry, &args.common.cwd, false).await; + let outcome = + dispatch_revert_one(entry, &args.common.cwd, args.common.dry_run).await; for w in &outcome.warnings { if !args.common.json { eprintln!("Warning ({}): {}", w.code, w.detail); @@ -388,6 +392,20 @@ pub async fn run(args: RemoveArgs) -> i32 { ); return 1; } + if args.common.dry_run { + if !args.common.json { + println!("Would revert vendoring for {key}"); + } + // Dry-run flips the would-be Removed to a Verified + // preview, same convention as apply/vendor/repair. + vendor_reverted_events.push( + PatchEvent::new(PatchAction::Verified, key.clone()).with_reason( + "vendor_would_revert", + "vendoring would be reverted on remove", + ), + ); + continue; + } vendor_state.entries.remove(key); if let Err(e) = save_state(&args.common.cwd, &vendor_state).await { emit_error_envelope( @@ -408,8 +426,18 @@ pub async fn run(args: RemoveArgs) -> i32 { } } - // Now remove from manifest - match remove_patch_from_manifest(&args.identifier, &manifest_path).await { + // Now remove from manifest. On --dry-run the removal is simulated in + // memory (manifest untouched) so the blob sweep below can still + // preview against the post-removal reference set. + let removal = if args.common.dry_run { + let removed: Vec = matching.iter().map(|(purl, _)| (*purl).clone()).collect(); + let mut simulated = manifest.clone(); + simulated.patches.retain(|purl, _| !removed.contains(purl)); + Ok((removed, simulated)) + } else { + remove_patch_from_manifest(&args.identifier, &manifest_path).await + }; + match removal { Ok((removed, manifest)) => { if removed.is_empty() { emit_not_found( @@ -423,25 +451,48 @@ pub async fn run(args: RemoveArgs) -> i32 { } if !args.common.json && !args.common.silent { - println!("Removed {} patch(es) from manifest:", removed.len()); + if args.common.dry_run { + println!("Would remove {} patch(es) from manifest:", removed.len()); + } else { + println!("Removed {} patch(es) from manifest:", removed.len()); + } for purl in &removed { println!(" - {purl}"); } - println!("\nManifest updated at {}", manifest_path.display()); + if args.common.dry_run { + println!("\nDry run — nothing was changed."); + } else { + println!("\nManifest updated at {}", manifest_path.display()); + } } - // Clean up unused blobs + // Clean up unused blobs (previewed, not deleted, on --dry-run). let blobs_path = socket_dir.join("blobs"); let mut blobs_removed = 0; - if let Ok(cleanup_result) = cleanup_unused_blobs(&manifest, &blobs_path, false).await { + if let Ok(cleanup_result) = + cleanup_unused_blobs(&manifest, &blobs_path, args.common.dry_run).await + { blobs_removed = cleanup_result.blobs_removed; if !args.common.json && !args.common.silent && cleanup_result.blobs_removed > 0 { - println!("\n{}", format_cleanup_result(&cleanup_result, false)); + println!( + "\n{}", + format_cleanup_result(&cleanup_result, args.common.dry_run) + ); } } if args.common.json { let mut env = Envelope::new(Command::Remove); + env.dry_run = args.common.dry_run; + // Dry-run flips would-be Removed events to Verified + // previews (the apply/vendor/repair convention), so + // `summary.removed` stays "manifest entries actually + // deleted" — zero on a preview. + let removal_action = if args.common.dry_run { + PatchAction::Verified + } else { + PatchAction::Removed + }; if lock_was_broken { env.record(lock_broken_event(socket_dir)); } @@ -457,9 +508,10 @@ pub async fn run(args: RemoveArgs) -> i32 { for ev in vendor_skipped_events { env.record(ev); } - // One Removed event per purl whose manifest entry was deleted. + // One Removed event per purl whose manifest entry was + // deleted (Verified on --dry-run). for purl in &removed { - env.record(PatchEvent::new(PatchAction::Removed, purl.clone())); + env.record(PatchEvent::new(removal_action, purl.clone())); } // One artifact-level Removed event carrying the // blob-sweep and rollback counts. Emitted whenever either @@ -478,7 +530,7 @@ pub async fn run(args: RemoveArgs) -> i32 { // totals from `details`, never from `summary.removed`. if blobs_removed > 0 || rollback_count > 0 { env.events - .push(PatchEvent::artifact(PatchAction::Removed).with_details( + .push(PatchEvent::artifact(removal_action).with_details( serde_json::json!({ "blobsRemoved": blobs_removed, "rolledBack": rollback_count, @@ -488,7 +540,10 @@ pub async fn run(args: RemoveArgs) -> i32 { println!("{}", env.to_pretty_json()); } - track_patch_removed(removed.len(), api_token.as_deref(), org_slug.as_deref()).await; + if !args.common.dry_run { + track_patch_removed(removed.len(), api_token.as_deref(), org_slug.as_deref()) + .await; + } 0 } Err(e) => { @@ -535,11 +590,12 @@ async fn remove_detached_only( } eprintln!(); } + // `--dry-run` previews without mutating — nothing to confirm. let prompt = format!( "Remove {} vendored patch(es) and revert their vendoring?", detached.len() ); - if !confirm(&prompt, true, args.common.yes, args.common.json) { + if !args.common.dry_run && !confirm(&prompt, true, args.common.yes, args.common.json) { if !args.common.json { println!("Removal cancelled."); } @@ -547,11 +603,12 @@ async fn remove_detached_only( } let mut env = Envelope::new(Command::Remove); + env.dry_run = args.common.dry_run; if lock_was_broken { env.record(lock_broken_event(socket_dir)); } for (key, entry) in &detached { - let outcome = dispatch_revert_one(entry, &args.common.cwd, false).await; + let outcome = dispatch_revert_one(entry, &args.common.cwd, args.common.dry_run).await; for w in &outcome.warnings { if !args.common.json { eprintln!("Warning ({}): {}", w.code, w.detail); @@ -578,6 +635,20 @@ async fn remove_detached_only( ); return 1; } + if args.common.dry_run { + if !args.common.json { + println!("Would revert vendoring for {key}"); + } + // Verified preview (the dry-run convention); still recorded + // so `summary.verified` counts the would-be removals. + env.record( + PatchEvent::new(PatchAction::Verified, key.clone()).with_reason( + "vendor_would_revert", + "vendoring would be reverted on remove", + ), + ); + continue; + } state.entries.remove(key); if let Err(e) = save_state(&args.common.cwd, &state).await { emit_error_envelope(args.common.json, "vendor_state_write_failed", e.to_string()); @@ -594,7 +665,9 @@ async fn remove_detached_only( if args.common.json { println!("{}", env.to_pretty_json()); } - track_patch_removed(detached.len(), api_token, org_slug).await; + if !args.common.dry_run { + track_patch_removed(detached.len(), api_token, org_slug).await; + } 0 } diff --git a/crates/socket-patch-cli/tests/remove_invariants.rs b/crates/socket-patch-cli/tests/remove_invariants.rs index fe2a593a..5263dbf9 100644 --- a/crates/socket-patch-cli/tests/remove_invariants.rs +++ b/crates/socket-patch-cli/tests/remove_invariants.rs @@ -432,3 +432,92 @@ fn remove_honors_manifest_path_override() { "remove must not create a default .socket manifest when --manifest-path is given" ); } + +// --------------------------------------------------------------------------- +// --dry-run (global contract row: "Preview, no mutations") +// --------------------------------------------------------------------------- + +/// `remove --dry-run` must mutate NOTHING — the manifest keeps every entry — +/// while the envelope reports the preview: `dryRun: true`, per-purl +/// `Verified` events (the apply/vendor/repair dry-run convention), and +/// `summary.removed` stays 0 because no entry was actually deleted. +#[test] +fn remove_dry_run_keeps_manifest_and_emits_verified_previews() { + let tmp = tempfile::tempdir().unwrap(); + let socket = make_socket_dir(tmp.path()); + + let (code, stdout) = run_remove( + tmp.path(), + "pkg:npm/__remove_test_a__@1.0.0", + &["--dry-run"], + ); + assert_eq!(code, 0, "stdout=\n{stdout}"); + let v: serde_json::Value = serde_json::from_str(&stdout).expect("valid JSON"); + assert_eq!(v["command"], "remove"); + assert_eq!(v["dryRun"], true); + assert_eq!( + v["summary"]["removed"], 0, + "a preview must not count as a removal" + ); + + let events = v["events"].as_array().expect("events array"); + assert!( + events.iter().any(|e| e["action"] == "verified" + && e["purl"] == "pkg:npm/__remove_test_a__@1.0.0"), + "expected a Verified preview event for the matched purl: {events:?}" + ); + assert!( + events.iter().all(|e| e["action"] != "removed"), + "dry-run must not emit Removed events: {events:?}" + ); + + // The on-disk manifest is untouched: both entries survive. + let manifest = read_manifest(&socket); + let patches = manifest["patches"].as_object().unwrap(); + assert_eq!(patches.len(), 2, "dry-run must not delete manifest entries"); + assert!(patches.contains_key("pkg:npm/__remove_test_a__@1.0.0")); + assert!(patches.contains_key("pkg:npm/__remove_test_b__@2.0.0")); +} + +/// The blob sweep runs in preview mode on `--dry-run`: the artifact-level +/// carrier event reports how many blobs WOULD be swept (as `Verified`, +/// with `details.blobsRemoved`), but the blob files stay on disk. +#[test] +fn remove_dry_run_previews_blob_sweep_without_deleting() { + let tmp = tempfile::tempdir().unwrap(); + let socket = make_socket_dir(tmp.path()); + + // A's afterHash blob: referenced only by entry A, so removing A + // makes it sweepable. + let blobs = socket.join("blobs"); + std::fs::create_dir_all(&blobs).unwrap(); + let blob_a = + blobs.join("1111111111111111111111111111111111111111111111111111111111111111"); + std::fs::write(&blob_a, b"patched contents").unwrap(); + + let (code, stdout) = run_remove( + tmp.path(), + "pkg:npm/__remove_test_a__@1.0.0", + &["--dry-run"], + ); + assert_eq!(code, 0, "stdout=\n{stdout}"); + let v: serde_json::Value = serde_json::from_str(&stdout).expect("valid JSON"); + assert_eq!(v["dryRun"], true); + + let events = v["events"].as_array().expect("events array"); + let carrier = events + .iter() + .find(|e| e["action"] == "verified" && e["details"]["blobsRemoved"].is_number()) + .unwrap_or_else(|| panic!("expected a Verified blob-sweep carrier event: {events:?}")); + assert_eq!( + carrier["details"]["blobsRemoved"], 1, + "the preview must count A's now-unreferenced blob" + ); + + assert!( + blob_a.exists(), + "dry-run must not delete blobs from disk" + ); + let manifest = read_manifest(&socket); + assert_eq!(manifest["patches"].as_object().unwrap().len(), 2); +} diff --git a/crates/socket-patch-core/.DS_Store b/crates/socket-patch-core/.DS_Store new file mode 100644 index 0000000000000000000000000000000000000000..8b63f6a44d6cbc4c43e40b6eee8b080bbc4e5fb0 GIT binary patch literal 6148 zcmeHKy-EW?5S}$r4s5Ow#6tEFOkrVnhVuzh3v!7>A;$$1`+Wu9#3xWY!FRB<^a*VI zW@j|Fcvy-gGqC&3&d<)>2e*4eL~eLH84%?}lwmMBRyu^uovtknW~bvl2BUFP&!hEN zeg7T!*?Y908ye51?dN}cU3u!JEUQ^l!IB+b?^Sp2&!=tg`}b+fA79g)<}pSZq>F<=ZB1IEB+Gk`mrCEFFW*%&Ye zjDc?k_O%!UJ)V3Y1i*EryeH*uBOTia}7) z$!YW9wDMQ;;$n5|?_)T*LeOSoz!*pw=*cqY`~M7|OlOmyrr63DFb39)0oN}_lTwt(pTwnzPJ^2a1g3pot=oG(a9dU(X5aeCh9@ByLBM=F($r$(p20j6m CDMz>f literal 0 HcmV?d00001 From 7c745628dc486c8afc44f86ec19b487a9988936c Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Thu, 2 Jul 2026 17:56:12 -0400 Subject: [PATCH 3/4] fix(get): --one-off fails honestly instead of silently saving anyway MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The flag parsed but was never read past the --save-only conflict check — the patch was saved to the manifest regardless, lying about persistence. It now errors 'One-off get mode is not yet implemented' up front (before any network/disk activity), mirroring rollback --one-off's contract. Contract env-table row documents both stubs; the in_process_get caveat test now pins the honest error instead of the accidental behavior. Co-Authored-By: Claude Fable 5 --- crates/.DS_Store | Bin 6148 -> 6148 bytes crates/socket-patch-cli/.DS_Store | Bin 6148 -> 6148 bytes crates/socket-patch-cli/CLI_CONTRACT.md | 2 +- crates/socket-patch-cli/src/commands/get.rs | 9 +++++++++ .../socket-patch-cli/tests/in_process_get.rs | 16 ++++++++-------- 5 files changed, 18 insertions(+), 9 deletions(-) diff --git a/crates/.DS_Store b/crates/.DS_Store index 68a0380967b756ea5b4b4e3eb6ca5e5c2420c7af..26ecd7bd5778d302d45335d0d2a7175ec32349d8 100644 GIT binary patch delta 43 ycmZoMXffDe$Ha6~d9nkO0mm*2!)IN;k2?Z6YLn+NxiBqYnB2##wwa6hmk0n;QV%8o delta 34 qcmZoMXffDe$Ha84ak2xG!Q>T8I+N!xxiL*(nB2##wwa6hmk0pM%?m*Q diff --git a/crates/socket-patch-cli/.DS_Store b/crates/socket-patch-cli/.DS_Store index 7efe91bbff08dd4fea37ba70cfe4e0ff1514752e..f2206edcf42764c2ff79bec241ff7584197de4e8 100644 GIT binary patch delta 37 tcmZoMXffEJ!pNjpI$4b|gz>}VYDPK6Lz8DQO0h2b{_9@X=ADe*VgT$(4ch i32 { ); return 1; } + if args.one_off { + // Honest failure instead of the historical silent no-op: the flag + // parsed but was never implemented, so the patch was saved to the + // manifest anyway — lying to the user about persistence. Mirrors + // `rollback --one-off`'s not-yet-implemented contract; rejected + // before any network or disk activity. + report_error(args.common.json, "One-off get mode is not yet implemented"); + return 1; + } apply_env_toggles(&args.common); // `--silent` is "errors only" (CLI_CONTRACT.md): every informational diff --git a/crates/socket-patch-cli/tests/in_process_get.rs b/crates/socket-patch-cli/tests/in_process_get.rs index 7c822c85..e646f242 100644 --- a/crates/socket-patch-cli/tests/in_process_get.rs +++ b/crates/socket-patch-cli/tests/in_process_get.rs @@ -555,13 +555,13 @@ async fn get_one_off_with_save_only_errors() { #[tokio::test] #[serial] -async fn get_one_off_without_identifier_validation() { - // CAVEAT: `--one-off` is NOT specially handled in the UUID path — there - // is no "not yet implemented" stub (the original comment here was wrong). - // With the API unreachable, the UUID fetch fails and `report_fetch_failure` - // returns exit 1. So this test really exercises the network-failure path - // with one_off set, not a one-off stub. We pin the observable contract: - // exit 1 and nothing written. +async fn get_one_off_is_an_honest_not_implemented_error() { + // `--one-off` was a silent no-op for three majors: the flag parsed but + // was never read past the `--save-only` conflict check, so the patch + // was saved to the manifest anyway — lying about persistence. It now + // fails honestly, BEFORE any network or disk activity (the unreachable + // API here proves no fetch is attempted: a fetch would produce the + // same exit code but only after a connect timeout). let tmp = tempfile::tempdir().unwrap(); let mut args = default_args(UUID, tmp.path()); args.common.api_url = "http://127.0.0.1:1".to_string(); @@ -569,7 +569,7 @@ async fn get_one_off_without_identifier_validation() { args.save_only = false; let code = run(args).await; - assert_eq!(code, 1, "unreachable API fetch must exit 1"); + assert_eq!(code, 1, "--one-off must fail as not-yet-implemented"); assert_no_manifest(tmp.path()); } From 06a9b1024766b910013696f9f15d2587bc7b1798 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Thu, 2 Jul 2026 18:02:30 -0400 Subject: [PATCH 4/4] =?UTF-8?q?refactor(scan):=20invert=20the=20mode=20fol?= =?UTF-8?q?d=20=E2=80=94=20ScanMode=20is=20the=20source=20of=20truth?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolve_mode_flags previously folded --mode INTO the legacy booleans (--redirect/--vendor/--apply), leaving the deprecated spellings as the internal truth. Now the booleans fold into args.mode and are never read past the fold; run() derives apply/vendor/hosted locals from the enum. Error messages are byte-identical (pinned by scan_vendor_e2e). Legacy boolean help text now says 'Deprecated spelling of --mode X'. Contract tests updated to pin the new direction (modeless scan stays read-only, --sync == --mode agent --prune). Co-Authored-By: Claude Fable 5 --- crates/.DS_Store | Bin 6148 -> 6148 bytes crates/socket-patch-cli/.DS_Store | Bin 6148 -> 6148 bytes crates/socket-patch-cli/src/commands/scan.rs | 93 ++++++++++-------- .../socket-patch-cli/tests/cli_parse_scan.rs | 75 +++++++------- 4 files changed, 92 insertions(+), 76 deletions(-) diff --git a/crates/.DS_Store b/crates/.DS_Store index 26ecd7bd5778d302d45335d0d2a7175ec32349d8..8861cdbd8c86b4396576ef06934d3df0a2b9fb65 100644 GIT binary patch delta 15 WcmZoMXffDe$Ha8+%4P?qBccE=Qx0H6{IQvd(} delta 32 ocmZoMXffEJ!pNjpI$4deh{^KM Result<(), String> { mode.cli_name(), )); } - match mode { - ScanMode::Hosted => args.redirect = true, - ScanMode::Vendored => args.vendor = true, - ScanMode::Agent => args.apply = true, - } + } else if args.redirect { + args.mode = Some(ScanMode::Hosted); + } else if args.vendor { + args.mode = Some(ScanMode::Vendored); + } else if args.apply || args.sync { + args.mode = Some(ScanMode::Agent); } - if args.detached && !args.vendor { + if args.detached && args.mode != Some(ScanMode::Vendored) { // "required" phrasing matches clap's requires errors — the // scan_vendor_e2e contract test accepts exactly that shape. return Err( @@ -632,14 +636,14 @@ pub struct ScanArgs { #[arg(long = "batch-size", env = "SOCKET_BATCH_SIZE", default_value_t = DEFAULT_BATCH_SIZE)] pub batch_size: usize, - /// Download and apply selected patches in JSON mode (non-interactive). - /// Without this flag, `scan --json` is read-only — it lists available - /// patches plus an `updates` array but does not mutate the manifest. - /// Designed for unattended workflows (cron jobs, bots that open PRs); - /// pair with `--yes` for clarity though `--json` already implies non- - /// interactive confirmation. No effect outside `--json` mode (the - /// non-JSON path always prompts the user). `--mode agent` is the - /// documented spelling of this mode. + /// Deprecated spelling of `--mode agent` (kept for compatibility; + /// prefer `--mode`). Download and apply selected patches in JSON mode + /// (non-interactive). Without a mode, `scan --json` is read-only — it + /// lists available patches plus an `updates` array but does not mutate + /// the manifest. Designed for unattended workflows (cron jobs, bots + /// that open PRs); pair with `--yes` for clarity though `--json` + /// already implies non-interactive confirmation. No effect outside + /// `--json` mode (the non-JSON path always prompts the user). #[arg(long, default_value_t = false)] pub apply: bool, @@ -659,17 +663,17 @@ pub struct ScanArgs { #[arg(long, default_value_t = false)] pub sync: bool, - /// Vendor every patched dependency into the committable - /// `.socket/vendor/` tree instead of applying patches in place: - /// download the selected patches, record them in the manifest, then - /// build + wire the vendored artifacts (the whole manifest is + /// Deprecated spelling of `--mode vendored` (kept for compatibility; + /// prefer `--mode`). Vendor every patched dependency into the + /// committable `.socket/vendor/` tree instead of applying patches in + /// place: download the selected patches, record them in the manifest, + /// then build + wire the vendored artifacts (the whole manifest is /// vendored, so a package vendored at an older patch uuid is /// re-vendored automatically). Conflicts with `--apply`/`--sync` /// (vendoring replaces the in-place apply); combine with `--prune` /// to drop uninstalled entries before they fail vendoring. JSON mode /// is non-interactive like `--apply`; the interactive path prompts - /// before downloading. `--mode vendored` is the documented spelling - /// of this mode. + /// before downloading. #[arg(long, default_value_t = false, conflicts_with_all = ["apply", "sync"])] pub vendor: bool, @@ -894,7 +898,7 @@ fn download_params(args: &ScanArgs, save_only: bool, json: bool, silent: bool) - api_overrides: args.common.api_client_overrides(), all_releases: args.all_releases, strict: args.common.strict, - persist_blobs: !args.vendor, + persist_blobs: args.mode != Some(ScanMode::Vendored), } } @@ -1945,9 +1949,10 @@ async fn run_redirect( pub async fn run(mut args: ScanArgs) -> i32 { apply_env_toggles(&args.common); - // Fold `--mode` into the legacy mode booleans before anything reads - // them, so every branch below keeps a single source of truth. Cross- - // mode combinations get a usage-style error (exit 2, matching clap's + // Fold the legacy mode booleans into `args.mode` before anything reads + // it, so every branch below keeps a single source of truth (the enum; + // the booleans are never consulted past this point). Cross-mode + // combinations get a usage-style error (exit 2, matching clap's // conflict exit code) — see `resolve_mode_flags` for why clap itself // can't express them. if let Err(message) = resolve_mode_flags(&mut args) { @@ -1955,11 +1960,13 @@ pub async fn run(mut args: ScanArgs) -> i32 { return 2; } - // `--sync` is sugar for `--apply --prune`. Derive locals once and + // `--sync` is sugar for `--mode agent --prune`. Derive locals once and // use them everywhere downstream so the flag interactions are // expressed in one place. `--apply --prune --sync` is redundant - // but legal (all three end up true). - let apply = args.apply || args.sync; + // but legal. + let apply = args.mode == Some(ScanMode::Agent); + let vendor = args.mode == Some(ScanMode::Vendored); + let hosted = args.mode == Some(ScanMode::Hosted); let prune = args.prune || args.sync; // A zero batch size would panic the API-query loop below: both @@ -2342,7 +2349,7 @@ pub async fn run(mut args: ScanArgs) -> i32 { // Registry-redirect mode is a distinct, self-contained flow (rewrite // lockfiles → hosted vendored patches). It reuses discovery above, then // returns — it must NOT fall through to the apply/vendor branches. - if args.redirect { + if hosted { return run_redirect( &args, &api_client, @@ -2507,7 +2514,7 @@ pub async fn run(mut args: ScanArgs) -> i32 { } } // --- Vendor path (if requested; conflicts with --apply/--sync) --- - } else if args.vendor { + } else if vendor { // Extracted into its own boxed fn — and it must STAY extracted: // this branch's temporaries (json! trees, DownloadParams, the // engine dispatch) live in the enclosing poll frame in debug @@ -2779,7 +2786,7 @@ pub async fn run(mut args: ScanArgs) -> i32 { // no-op — re-vendoring a stale uuid is exactly what the flag is for. let is_vendored = |p: &str| vendored_purls.contains(p) || vendored_purls.contains(strip_purl_qualifiers(p)); - let (vendored_selected, selected): (Vec<_>, Vec<_>) = if args.vendor { + let (vendored_selected, selected): (Vec<_>, Vec<_>) = if vendor { (Vec::new(), selected) } else { selected.into_iter().partition(|p| is_vendored(&p.purl)) @@ -2796,7 +2803,7 @@ pub async fn run(mut args: ScanArgs) -> i32 { // Lockfile-only purls leave the in-place apply selection (calm skip, // mirrors the JSON path). In `--vendor` mode they stay: the vendor // engine fetches lockfile-resolved packages pristine. - let (selected, not_installed_selected): (Vec<_>, Vec) = if args.vendor { + let (selected, not_installed_selected): (Vec<_>, Vec) = if vendor { (selected, Vec::new()) } else { let (kept, skipped) = partition_skipped_selected( @@ -2824,7 +2831,7 @@ pub async fn run(mut args: ScanArgs) -> i32 { } } - if selected.is_empty() && !args.vendor { + if selected.is_empty() && !vendor { if !args.common.silent { println!("No patches selected."); } @@ -2834,7 +2841,7 @@ pub async fn run(mut args: ScanArgs) -> i32 { // Vendor mode: pre-verify baselines so a content mismatch surfaces // BEFORE the confirm prompt (vendoring still proceeds for these — // the stage force-applies the verified patched content). - let mismatched_baselines: HashSet = if args.vendor && !args.common.silent { + let mismatched_baselines: HashSet = if vendor && !args.common.silent { preverify_vendor_baselines( &api_client, effective_org_slug, @@ -2850,7 +2857,7 @@ pub async fn run(mut args: ScanArgs) -> i32 { // Display detailed summary of selected patches before confirming // (presentational only — skipped wholesale under --silent). if !args.common.silent { - if args.vendor { + if vendor { println!("\nPatches to vendor:\n"); } else { println!("\nPatches to apply:\n"); @@ -2925,7 +2932,7 @@ pub async fn run(mut args: ScanArgs) -> i32 { // of which mutate the manifest and `.socket/` on disk. if args.common.dry_run { if !args.common.silent { - let action = if args.vendor { + let action = if vendor { "download and vendor" } else { "download and apply" @@ -2939,7 +2946,7 @@ pub async fn run(mut args: ScanArgs) -> i32 { } // Prompt to download - let verb = if args.vendor { "vendor" } else { "apply" }; + let verb = if vendor { "vendor" } else { "apply" }; let prompt = format!("Download and {verb} {} patch(es)?", selected.len()); if !confirm(&prompt, true, args.common.yes, args.common.json) { if !args.common.silent { @@ -2954,12 +2961,12 @@ pub async fn run(mut args: ScanArgs) -> i32 { // download only saves and the vendor step below does the rest). let params = download_params( &args, - /*save_only=*/ args.vendor, + /*save_only=*/ vendor, /*json=*/ false, args.common.silent, ); - let code = if args.vendor { + let code = if vendor { // Extracted + boxed for the same Windows-1-MiB-frame reason as the // JSON path (see `run_vendor_json_path`). boxed_vendor_interactive_path( @@ -2985,7 +2992,7 @@ pub async fn run(mut args: ScanArgs) -> i32 { // beyond what `--apply` added — users wanting to clean up should // run `socket-patch gc` (or `repair`) explicitly. (Vendor mode // already ran its GC before the vendor step.) - if prune && !args.vendor { + if prune && !vendor { let gc = run_apply_gc( &args.common, &manifest_path, diff --git a/crates/socket-patch-cli/tests/cli_parse_scan.rs b/crates/socket-patch-cli/tests/cli_parse_scan.rs index e9ec112c..e7f41924 100644 --- a/crates/socket-patch-cli/tests/cli_parse_scan.rs +++ b/crates/socket-patch-cli/tests/cli_parse_scan.rs @@ -561,15 +561,16 @@ fn scan_json_empty_cwd_emits_updates_key() { // --- `--mode` selector (documented spelling of the mode booleans) ---------- // // `--mode ` is the RELEASED spelling of the three -// mode flags. The clap parser only fills `args.mode`; `resolve_mode_flags` -// (run at the top of `scan::run`, exercised directly here) folds it into the -// legacy `--redirect`/`--vendor`/`--apply` booleans and enforces the -// cross-mode rules clap can't express (a value-dependent conflict). These -// tests lock both the fold and the legacy aliases. +// mode flags. `resolve_mode_flags` (run at the top of `scan::run`, +// exercised directly here) makes `args.mode` the single source of truth: +// the legacy `--redirect`/`--vendor`/`--apply`/`--sync` booleans fold INTO +// the enum (they are input spellings, never read downstream), and the +// cross-mode rules clap can't express (a value-dependent conflict) are +// enforced. These tests lock both the fold and the legacy aliases. /// Parse `extra` (must parse cleanly at the clap level), then run the mode -/// fold — mirroring exactly what `scan::run` does before it reads any mode -/// boolean. +/// fold — mirroring exactly what `scan::run` does before it reads the +/// resolved mode. fn parse_and_resolve(extra: &[&str]) -> Result { let mut args = parse_scan(extra); resolve_mode_flags(&mut args)?; @@ -578,35 +579,39 @@ fn parse_and_resolve(extra: &[&str]) -> Result { #[test] #[serial_test::serial] -fn mode_hosted_folds_to_redirect() { - // The parser records the enum verbatim... - assert_eq!( - parse_scan(&["--mode", "hosted"]).mode, - Some(ScanMode::Hosted) - ); - // ...and the fold turns it into the redirect boolean, exclusively. +fn mode_hosted_is_the_source_of_truth() { + // The parser records the enum verbatim and the fold leaves it as the + // single source of truth (the booleans are inputs, not outputs). let folded = parse_and_resolve(&["--mode", "hosted"]).expect("fold ok"); - assert!(folded.redirect, "--mode hosted sets --redirect"); - assert!(!folded.vendor); - assert!(!folded.apply); + assert_eq!(folded.mode, Some(ScanMode::Hosted)); + // ...and the legacy boolean spelling folds INTO the enum. + let folded = parse_and_resolve(&["--redirect"]).expect("fold ok"); + assert_eq!(folded.mode, Some(ScanMode::Hosted), "--redirect == --mode hosted"); } #[test] #[serial_test::serial] -fn mode_vendored_folds_to_vendor() { +fn mode_vendored_is_the_source_of_truth() { let folded = parse_and_resolve(&["--mode", "vendored"]).expect("fold ok"); - assert!(folded.vendor, "--mode vendored sets --vendor"); - assert!(!folded.redirect); - assert!(!folded.apply); + assert_eq!(folded.mode, Some(ScanMode::Vendored)); + let folded = parse_and_resolve(&["--vendor"]).expect("fold ok"); + assert_eq!(folded.mode, Some(ScanMode::Vendored), "--vendor == --mode vendored"); } #[test] #[serial_test::serial] -fn mode_agent_folds_to_apply() { +fn mode_agent_is_the_source_of_truth() { let folded = parse_and_resolve(&["--mode", "agent"]).expect("fold ok"); - assert!(folded.apply, "--mode agent sets --apply"); - assert!(!folded.redirect); - assert!(!folded.vendor); + assert_eq!(folded.mode, Some(ScanMode::Agent)); + let folded = parse_and_resolve(&["--apply"]).expect("fold ok"); + assert_eq!(folded.mode, Some(ScanMode::Agent), "--apply == --mode agent"); + // --sync counts as an agent-mode spelling (its prune half is orthogonal). + let folded = parse_and_resolve(&["--sync"]).expect("fold ok"); + assert_eq!(folded.mode, Some(ScanMode::Agent), "--sync == --mode agent --prune"); + assert!(folded.sync, "the prune half of --sync stays readable"); + // No mode selected at all: scan stays read-only. + let folded = parse_and_resolve(&[]).expect("fold ok"); + assert_eq!(folded.mode, None, "modeless scan is read-only"); } #[test] @@ -645,15 +650,15 @@ fn mode_hosted_with_vendor_boolean_errors() { fn mode_agent_with_apply_boolean_is_allowed() { // Same mode spelled both ways is redundant but legal. let folded = parse_and_resolve(&["--mode", "agent", "--apply"]).expect("same-mode ok"); - assert!(folded.apply); + assert_eq!(folded.mode, Some(ScanMode::Agent)); } #[test] #[serial_test::serial] fn mode_agent_with_sync_boolean_is_allowed() { - // --sync implies --apply, so it counts as an agent-mode spelling. + // --sync implies agent mode, so it counts as an agent-mode spelling. let folded = parse_and_resolve(&["--mode", "agent", "--sync"]).expect("same-mode ok"); - assert!(folded.apply); + assert_eq!(folded.mode, Some(ScanMode::Agent)); assert!(folded.sync); } @@ -662,7 +667,7 @@ fn mode_agent_with_sync_boolean_is_allowed() { fn mode_vendored_with_detached_ok() { // --detached is legal under vendored mode selected via --mode. let folded = parse_and_resolve(&["--mode", "vendored", "--detached"]).expect("fold ok"); - assert!(folded.vendor); + assert_eq!(folded.mode, Some(ScanMode::Vendored)); assert!(folded.detached); } @@ -682,13 +687,17 @@ fn detached_without_vendored_mode_errors() { #[test] #[serial_test::serial] fn legacy_mode_spellings_still_parse() { - // The boolean aliases keep working with no `--mode` given; the fold is - // then a no-op and leaves the booleans exactly as parsed. + // The boolean aliases keep working with no `--mode` given; the fold + // derives the mode enum from them (the inverse of the historical + // direction — `args.mode` is now the single source of truth). assert!(parse_scan(&["--redirect"]).redirect); assert!(parse_scan(&["--vendor"]).vendor); assert!(parse_scan(&["--apply"]).apply); let folded = parse_and_resolve(&["--vendor", "--detached"]).expect("legacy fold ok"); - assert!(folded.vendor); assert!(folded.detached); - assert_eq!(folded.mode, None, "no --mode ⇒ selector stays None"); + assert_eq!( + folded.mode, + Some(ScanMode::Vendored), + "legacy --vendor folds into the mode selector" + ); }