fix(cli): Phase 1 quick wins — env-bool crashes, remove --dry-run, one-off stub, mode-fold inversion

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.
 
@@ -614,7 +614,7 @@ All v3.0 env vars use the `SOCKET_*` prefix. Three legacy `SOCKET_PATCH_*` names
 | `SOCKET_FORCE` | `apply --force` / `-f` | `false` | Local to `apply`. |
 | `SOCKET_BATCH_SIZE` | `scan --batch-size` | `100` | Local to `scan`. |
 | `SOCKET_SAVE_ONLY` | `get --save-only` | `false` | Local to `get`. |
-| `SOCKET_ONE_OFF` | `get --one-off` / `rollback --one-off` | `false` | Local to `get`/`rollback`. |
+| `SOCKET_ONE_OFF` | `get --one-off` / `rollback --one-off` | `false` | Local to `get`/`rollback`. Both are **not yet implemented**: the flag parses (boolishly, empty-tolerant) and the command fails up front with a "not yet implemented" error, before any network or disk activity. |
 | `SOCKET_SKIP_ROLLBACK` | `remove --skip-rollback` | `false` | Local to `remove`. |
 | `SOCKET_DOWNLOAD_ONLY` | `repair --download-only` | `false` | Local to `repair`. |
 | `SOCKET_SETUP_EXCLUDE` | `setup --exclude` | (none) | Local to `setup`; comma-separated workspace-member paths, persisted to `setup.exclude`. |
@@ -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)
 

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,31 +575,38 @@ 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", "");
             std::env::set_var("SOCKET_GLOBAL_PREFIX", "");
             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);
+            }
+        });
+    }
 }

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,
 

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,
 }
@@ -1277,6 +1293,15 @@ pub async fn run(args: GetArgs) -> 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