fix: correctness bug sweep + hardening tests across crawlers, patch, and vex

crates/socket-patch-cli/src/args.rs

@@ -578,6 +578,88 @@ mod tests {
         );
     }
 
+    /// `parse_supported_ecosystem` accepts every name this build compiles in
+    /// and returns it verbatim.
+    #[test]
+    fn parse_supported_ecosystem_accepts_compiled_in_names() {
+        for e in Ecosystem::all() {
+            let name = e.cli_name();
+            assert_eq!(
+                parse_supported_ecosystem(name),
+                Ok(name.to_string()),
+                "{name:?} is compiled in and must be accepted",
+            );
+        }
+    }
+
+    /// Unsupported / misspelled ecosystem names are rejected with a message
+    /// that names the offending token and lists the supported set.
+    #[test]
+    fn parse_supported_ecosystem_rejects_unknown_names() {
+        for bad in ["bogus", "NPM", "py-pi", ""] {
+            let err = parse_supported_ecosystem(bad)
+                .expect_err("unsupported ecosystem name must be rejected");
+            assert!(err.contains(bad), "error should echo the bad token: {err:?}");
+            assert!(
+                err.contains("supported:"),
+                "error should list the supported set: {err:?}",
+            );
+        }
+    }
+
+    /// End-to-end through clap: `--ecosystems` splits on commas, validates each
+    /// token, and rejects the whole parse if any token is unsupported.
+    #[test]
+    #[serial_test::serial]
+    fn ecosystems_flag_splits_and_validates() {
+        with_clean_socket_env(|| {
+            let cli = TestCli::try_parse_from(["socket-patch", "--ecosystems", "npm,pypi"])
+                .expect("comma-separated supported ecosystems must parse");
+            assert_eq!(
+                cli.common.ecosystems,
+                Some(vec!["npm".to_string(), "pypi".to_string()]),
+            );
+
+            // One bad token in the list aborts the whole parse.
+            assert!(
+                TestCli::try_parse_from(["socket-patch", "--ecosystems", "npm,bogus"]).is_err(),
+                "an unsupported token must fail the parse",
+            );
+        });
+    }
+
+    /// Precedence contract: a CLI value wins over the env var for a string flag.
+    #[test]
+    #[serial_test::serial]
+    fn cli_arg_overrides_env_var() {
+        with_clean_socket_env(|| {
+            std::env::set_var("SOCKET_MANIFEST_PATH", "from-env.json");
+            let cli =
+                TestCli::try_parse_from(["socket-patch", "--manifest-path", "from-cli.json"])
+                    .unwrap();
+            assert_eq!(cli.common.manifest_path, "from-cli.json");
+            std::env::remove_var("SOCKET_MANIFEST_PATH");
+        });
+    }
+
+    /// Precedence contract: the env var is honored when no CLI value is given,
+    /// and the clap-declared default applies when neither is set.
+    #[test]
+    #[serial_test::serial]
+    fn env_var_used_then_default_applies() {
+        with_clean_socket_env(|| {
+            std::env::set_var("SOCKET_MANIFEST_PATH", "from-env.json");
+            let cli = TestCli::try_parse_from(["socket-patch"]).unwrap();
+            assert_eq!(cli.common.manifest_path, "from-env.json");
+            std::env::remove_var("SOCKET_MANIFEST_PATH");
+
+            let cli = TestCli::try_parse_from(["socket-patch"]).unwrap();
+            assert_eq!(cli.common.manifest_path, DEFAULT_PATCH_MANIFEST_PATH);
+            assert_eq!(cli.common.download_mode, "diff");
+            assert_eq!(cli.common.cwd, PathBuf::from("."));
+        });
+    }
+
     /// `apply_env_toggles` mirrors `--debug` / `--no-telemetry` into the env
     /// vars core code reads directly, and is a no-op when the flags are off.
     /// `#[serial]` because it mutates process-global env state.

crates/socket-patch-cli/src/commands/apply.rs

@@ -1235,25 +1235,43 @@ async fn apply_patches_inner(
                     // hash-mismatch and were skipped above), so this
                     // applies a single variant for them; Maven's coexisting
                     // classifier jars each get patched.
+                } else {
+                    // A variant that reached apply IS the installed
+                    // distribution, so a failure here is a real apply
+                    // failure — flag it even if a *sibling* variant of the
+                    // same base succeeds (Maven's coexisting classifier
+                    // jars, or any base where `--force` attempts every
+                    // variant). Mirrors the npm branch below and the
+                    // rollback loop, which mark `has_errors` on every failed
+                    // result; without this a partial multi-variant failure
+                    // would leave a `failed` event in the envelope while the
+                    // command still reported `success` / exit 0.
+                    has_errors = true;
+                    if !args.common.silent && !args.common.json {
+                        eprintln!(
+                            "Failed to patch {}: {}",
+                            variant_purl,
+                            result.error.as_deref().unwrap_or("unknown error")
+                        );
+                    }
                 }
                 results.push(result);
             }
 
             if applied {
                 applied_base_purls.insert(base_purl.clone());
             } else {
+                // Nothing applied for this base. `has_errors` was already set
+                // per-variant above when a variant was attempted-but-failed;
+                // set it here too for the no-variant-attempted case so both
+                // paths fail the command.
                 has_errors = true;
-                if !args.common.silent && !args.common.json {
-                    if attempted {
-                        // The installed variant was found but its patch could
-                        // not be applied (e.g. a later file mismatched) — a
-                        // genuine apply failure, not a missing package.
-                        eprintln!(
-                            "Failed to patch {base_purl}: the installed variant could not be patched"
-                        );
-                    } else {
-                        eprintln!("Failed to patch {base_purl}: no matching variant found");
-                    }
+                if !attempted && !args.common.silent && !args.common.json {
+                    // No variant matched the installed distribution at all —
+                    // the package on disk isn't any known release variant.
+                    // (Attempted-but-failed variants already printed their own
+                    // per-variant failure line above.)
+                    eprintln!("Failed to patch {base_purl}: no matching variant found");
                 }
             }
         } else {

crates/socket-patch-cli/src/commands/get.rs

@@ -103,6 +103,13 @@ pub(crate) fn max_vuln_severity(
     vulns
         .values()
         .max_by_key(|v| severity_rank(&v.severity))
+        // `max_by_key` only yields `None` for an empty map; a non-empty
+        // map of exclusively unrecognized severities (all rank 0) would
+        // otherwise leak a garbage label like "" or "unknown". Drop it so
+        // the documented "every entry unrecognized → None" contract holds
+        // and `patch_event_metadata` omits `severity` rather than emitting
+        // a meaningless value.
+        .filter(|v| severity_rank(&v.severity) > 0)
         .map(|v| v.severity.clone())
 }
 
@@ -1928,6 +1935,94 @@ mod tests {
         assert_eq!(max_vuln_severity(&HashMap::new()), None);
     }
 
+    #[test]
+    fn max_vuln_severity_returns_none_when_all_unrecognized() {
+        // Non-empty map but every severity is off-canon (rank 0). Per the
+        // doc contract this must be `None` — NOT `Some("")`/`Some("unknown")`.
+        // Regression guard: `max_by_key` alone returns the element for any
+        // non-empty map, leaking a garbage severity label.
+        let mut vulns = HashMap::new();
+        vulns.insert(
+            "GHSA-a".into(),
+            VulnerabilityResponse {
+                cves: Vec::new(),
+                summary: String::new(),
+                severity: "informational".into(),
+                description: String::new(),
+            },
+        );
+        vulns.insert(
+            "GHSA-b".into(),
+            VulnerabilityResponse {
+                cves: Vec::new(),
+                summary: String::new(),
+                severity: String::new(),
+                description: String::new(),
+            },
+        );
+        assert_eq!(max_vuln_severity(&vulns), None);
+    }
+
+    #[test]
+    fn max_vuln_severity_recognized_wins_over_unrecognized() {
+        // A single recognized severity alongside unrecognized ones must
+        // surface — the rank-0 filter only suppresses the all-unrecognized
+        // case, never a real label.
+        let mut vulns = HashMap::new();
+        vulns.insert(
+            "GHSA-junk".into(),
+            VulnerabilityResponse {
+                cves: Vec::new(),
+                summary: String::new(),
+                severity: "unknown".into(),
+                description: String::new(),
+            },
+        );
+        vulns.insert(
+            "GHSA-real".into(),
+            VulnerabilityResponse {
+                cves: Vec::new(),
+                summary: String::new(),
+                severity: "low".into(),
+                description: String::new(),
+            },
+        );
+        assert_eq!(max_vuln_severity(&vulns).as_deref(), Some("low"));
+    }
+
+    #[test]
+    fn patch_event_metadata_omits_severity_when_all_unrecognized() {
+        // The consumer-facing contract: a patch whose vulnerabilities all
+        // carry non-canonical severities must NOT emit a `severity` key
+        // (it would otherwise be `""`), while still listing the vulns.
+        let mut vulns = HashMap::new();
+        vulns.insert(
+            "GHSA-aaaa-bbbb-cccc".into(),
+            VulnerabilityResponse {
+                cves: vec!["CVE-2024-0001".into()],
+                summary: "Something".into(),
+                severity: "informational".into(),
+                description: String::new(),
+            },
+        );
+        let patch = PatchResponse {
+            uuid: String::new(),
+            purl: String::new(),
+            published_at: "ts".into(),
+            files: HashMap::new(),
+            vulnerabilities: vulns,
+            description: "desc".into(),
+            license: "MIT".into(),
+            tier: "free".into(),
+        };
+        let meta = patch_event_metadata(&patch);
+        assert!(meta.as_object().unwrap().get("severity").is_none());
+        // The vulnerability itself is still surfaced (with its raw label).
+        let vulns_out = meta["vulnerabilities"].as_array().unwrap();
+        assert_eq!(vulns_out.len(), 1);
+        assert_eq!(vulns_out[0]["severity"], "informational");
+    }
+
     #[test]
     fn patch_event_metadata_includes_all_keys() {
         let mut vulns = HashMap::new();

crates/socket-patch-cli/src/commands/list.rs

@@ -96,15 +96,14 @@ fn emit_error(args: &ListArgs, code: &str, message: String) {
 pub async fn run(args: ListArgs) -> i32 {
     let manifest_path = args.common.resolved_manifest_path();
 
-    if tokio::fs::metadata(&manifest_path).await.is_err() {
-        emit_error(
-            &args,
-            "manifest_not_found",
-            format!("Manifest not found at {}", manifest_path.display()),
-        );
-        return 1;
-    }
-
+    // `read_manifest` is the single source of truth for the three error
+    // states: `Ok(None)` (file absent), `Err(InvalidData)` (present but
+    // unparseable), and any other `Err` (genuine I/O failure). We deliberately
+    // do NOT stat the path first: a `metadata` pre-check is both redundant and
+    // wrong — it reports *any* stat failure (e.g. an unreadable parent dir) as
+    // `manifest_not_found`, masking real I/O errors that owe a
+    // `manifest_unreadable`, and it opens a TOCTOU window where a file removed
+    // between the stat and the read lands in the wrong error arm.
     match read_manifest(&manifest_path).await {
         Ok(Some(manifest)) => {
             // Sort by PURL so both the JSON envelope and the human-readable
@@ -170,11 +169,16 @@ pub async fn run(args: ListArgs) -> i32 {
             0
         }
         Ok(None) => {
-            // Defensive: `read_manifest` only returns `Ok(None)` for a
-            // missing file, which the metadata pre-check above already
-            // turned into `manifest_not_found`. Kept so a future loader
-            // change can't silently fall through without an envelope.
-            emit_error(&args, "manifest_invalid", "Invalid manifest".to_string());
+            // `read_manifest` returns `Ok(None)` only when the file does not
+            // exist (its documented contract), so this is the missing-manifest
+            // path — `manifest_not_found`, NOT `manifest_invalid` (which means
+            // the file is present but corrupt). See CLI_CONTRACT.md error-code
+            // table.
+            emit_error(
+                &args,
+                "manifest_not_found",
+                format!("Manifest not found at {}", manifest_path.display()),
+            );
             1
         }
         Err(e) => {

crates/socket-patch-cli/src/commands/lock_cli.rs

@@ -104,7 +104,14 @@ pub fn acquire_or_emit(
         match acquire(socket_dir, Duration::ZERO) {
             Ok(guard) => drop(guard),
             Err(LockError::Held) => {
-                let msg = held_message(timeout);
+                // The probe above is a *non-blocking* try-once
+                // (`Duration::ZERO`), so report a zero wait. Threading
+                // the caller's `timeout` here would claim a "(waited …)"
+                // that never happened — the probe refuses a live holder
+                // immediately, it does not wait out the budget first.
+                // `break_probe_held_message` takes no timeout precisely so
+                // the wrong value can't be passed back in.
+                let msg = break_probe_held_message();
                 emit(command, json, silent, dry_run, "lock_held", &msg, Some(socket_dir));
                 return Err(1);
             }
@@ -192,6 +199,16 @@ pub fn record_lock_broken(env: &mut Envelope, socket_dir: &Path) {
     env.record(lock_broken_event(socket_dir));
 }
 
+/// Contention message for the `--break-lock` pre-acquire probe. That
+/// probe is hard-wired to a non-blocking try-once (`Duration::ZERO`), so
+/// the message must never claim a wait, regardless of the caller's
+/// `--lock-timeout`. Kept timeout-free on purpose: the call site cannot
+/// thread the full budget back in and fabricate a "(waited …)" clause
+/// for time that was never spent.
+fn break_probe_held_message() -> String {
+    held_message(Duration::ZERO)
+}
+
 /// Human-readable description of a `lock_held` contention for the given
 /// wait budget. A zero budget means the historical non-blocking
 /// try-once, so we omit the "(waited …)" clause entirely.
@@ -492,6 +509,26 @@ mod tests {
         assert!(!msg.contains("waited"), "zero budget should not claim a wait: {msg}");
     }
 
+    /// Regression: the `--break-lock` pre-acquire probe is a non-blocking
+    /// try-once, so its `lock_held` refusal must NEVER claim a wait — even
+    /// when the caller passes a positive `--lock-timeout`. The earlier
+    /// code threaded the full `timeout` into the probe's message, so a
+    /// `--break-lock --lock-timeout 250ms` against a live holder reported
+    /// `(waited 250ms)` despite refusing immediately. The probe message is
+    /// now timeout-free by construction; this pins that it carries no wait
+    /// clause.
+    #[test]
+    fn break_probe_held_message_never_claims_a_wait() {
+        let msg = break_probe_held_message();
+        assert!(
+            !msg.contains("waited"),
+            "break-lock probe refuses immediately and must not claim a wait: {msg}"
+        );
+        // It is still the same identity sentence the rest of the code
+        // emits for contention, just without the trailing budget clause.
+        assert_eq!(msg, held_message(Duration::ZERO));
+    }
+
     /// The `--json` failure envelope (previously emitted only via
     /// `println!`, so untested) has the stable error shape downstream
     /// consumers pattern-match on: top-level `status: "error"` and

crates/socket-patch-cli/src/commands/remove.rs

@@ -295,8 +295,18 @@ pub async fn run(args: RemoveArgs) -> i32 {
                 // is non-zero so the `rolledBack` count is still reported
                 // even when no blobs happened to be swept (e.g. the removed
                 // patch's afterHash blobs are still referenced elsewhere).
+                //
+                // Pushed directly rather than via `env.record`: this is a
+                // purl-less metadata carrier, not a removed manifest entry.
+                // The per-purl events above are the authoritative
+                // patch-removal count, so `summary.removed` must equal the
+                // number of entries deleted (`removed.len()`) — letting this
+                // carrier bump `removed` too would double-count, reporting
+                // e.g. `removed: 2` for a single-patch removal that happened
+                // to sweep an orphan blob. Consumers read the blob/rollback
+                // totals from `details`, never from `summary.removed`.
                 if blobs_removed > 0 || rollback_count > 0 {
-                    env.record(
+                    env.events.push(
                         PatchEvent::artifact(PatchAction::Removed).with_details(serde_json::json!({
                             "blobsRemoved": blobs_removed,
                             "rolledBack": rollback_count,