From a9f2e45532a23e91c990756e7888a58729ccca2b Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Fri, 14 Aug 2026 07:10:21 -0700 Subject: [PATCH 1/2] fix(scan): scope takeover cleanup per package, warn on hosted --prune MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The mode-takeover warning told users to delete the whole redirect ledger (vendored direction) or the whole .socket/vendor// tree (hosted direction). Following it destroyed live data for packages the takeover never touched: other packages' VEX records, the only recorded pre-redirect lockfile originals, and still-live vendored artifacts. The remediation is now per-package and non-destructive — run `socket-patch remove ` per named package (hosted direction) or delete only the named records from the redirect ledger (vendored direction) — and explicitly warns against whole-file/tree deletion and blanket reverts. `scan --mode hosted --prune` silently dropped --prune: both hosted terminals return before the GC blocks, so a bot migrating from `--mode agent --prune` stopped pruning forever with exit 0 and no signal. The flag stays accepted (an orthogonal knob per the CLI contract), but the run now emits an explicit `redirect_prune_ignored` warning on stderr and in the JSON `redirect.warnings[]`, including the zero-discovery envelope. Two takeover blind spots are also closed. A redirect ledger whose every record fetch failed (edits present, records empty) was invisible to overlap detection; the vendored purls are now matched against the recorded edit keys so the stale ledger is still flagged. And hosted liveness was proved only by a hardcoded patch.socket.dev inventory URL — structurally unprovable for yarn-berry (inventory resolved is always None), bun (URL 3-tuples are skipped), and any non-default patch host. It is now proved by the record's patch uuid: in the inventory URL on any host, or in the redirect-edited lockfile text outside a .socket/vendor// path (vendored wiring embeds the same uuid, so only non-vendored occurrences count). Co-authored-by: Claude Fable 5 --- crates/socket-patch-cli/CLI_CONTRACT.md | 2 +- .../src/commands/scan/hosted.rs | 20 +- .../socket-patch-cli/src/commands/scan/mod.rs | 518 ++++++++++++++++-- .../tests/in_process_redirect.rs | 63 +++ 4 files changed, 564 insertions(+), 39 deletions(-) diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index b77faae2..73ef5cfe 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -71,7 +71,7 @@ Beyond the globals above, each subcommand defines a small set of local arguments | `vendor` | `--revert` | `SOCKET_VENDOR_REVERT` | Undo vendoring: restore recorded original lockfile fragments + remove `.socket/vendor/` artifacts. Works without a manifest | | `apply`, `scan`, `vendor` | `--vex` | `SOCKET_VEX` | Generate an OpenVEX 0.2.0 document at this path on a successful run; see "embedded VEX" below | | `apply`, `scan`, `vendor` | `--vex-product`, `--vex-no-verify`, `--vex-doc-id`, `--vex-compact` | `SOCKET_VEX_PRODUCT`, `SOCKET_VEX_NO_VERIFY`, `SOCKET_VEX_DOC_ID`, `SOCKET_VEX_COMPACT` | Passthrough to the embedded VEX builder; mirror the standalone `vex` knobs. Inert unless `--vex` is set | -| `scan` | `--mode ` | — | The documented selector for the three patch-application modes. Each value is equivalent to one legacy boolean spelling: `hosted` == `--redirect`, `vendored` == `--vendor`, `agent` == `--apply` (`--sync` counts as an agent spelling). Combining `--mode` with a boolean of a DIFFERENT mode is a usage error (exit 2, enforced in `resolve_mode_flags` — clap's `conflicts_with` is value-independent); the same mode spelled both ways is accepted. `--prune` is an orthogonal GC knob and never conflicts | +| `scan` | `--mode ` | — | The documented selector for the three patch-application modes. Each value is equivalent to one legacy boolean spelling: `hosted` == `--redirect`, `vendored` == `--vendor`, `agent` == `--apply` (`--sync` counts as an agent spelling). Combining `--mode` with a boolean of a DIFFERENT mode is a usage error (exit 2, enforced in `resolve_mode_flags` — clap's `conflicts_with` is value-independent); the same mode spelled both ways is accepted. `--prune` is an orthogonal GC knob and never conflicts — but hosted mode runs no GC, so `--mode hosted --prune` emits an explicit `redirect_prune_ignored` warning (JSON `redirect.warnings[]` + stderr) instead of silently dropping the flag | | `scan` | `--redirect` | — | Hosted mode's legacy boolean spelling (**hidden from `--help`** and **deprecated** — `--mode hosted` is the documented spelling; this alias is scheduled for removal in v4): rewrite lockfiles / registry configs so ONLY the patched dependencies resolve to Socket's hosted patch server; no artifact bytes land in the repo. Conflicts with `--apply`/`--sync`/`--vendor` | | `scan` | `--apply` / `--prune` / `--sync` | — | Mode selectors (sync = apply + prune); `--apply` == `--mode agent` | | `scan` | `--vendor` / `--detached` | — | Vendor every patched dependency instead of applying in place (`--vendor` == `--mode vendored`; conflicts with `--apply`/`--sync`, combines with `--prune`); `--detached` additionally skips all manifest writes — the vendor ledger embeds the patch records (requires vendored mode in either spelling) | diff --git a/crates/socket-patch-cli/src/commands/scan/hosted.rs b/crates/socket-patch-cli/src/commands/scan/hosted.rs index 340bddf3..3f9ee2e2 100644 --- a/crates/socket-patch-cli/src/commands/scan/hosted.rs +++ b/crates/socket-patch-cli/src/commands/scan/hosted.rs @@ -522,14 +522,16 @@ pub(super) async fn run_redirect( // may still claim package(s) this project also has a hosted redirect ledger // for — their tarballs would then be orphaned and that ledger stale. But the // overlap alone does NOT prove hosted won: only warn for the package(s) the - // LIVE lockfile actually routes to `patch.socket.dev` (see + // LIVE lockfile actually routes to the hosted patch server (see // `classify_overlap_takeover`), so a dry-run / no-op over a lock that still // points at the vendored files stays silent instead of pointing cleanup at // the live vendored ledger. Warn (JSON `warnings[]` and stderr) WITHOUT // deleting the other mode's ledger; reconciliation is deferred (see PR Scope). // Read after the ledger write above so a non-dry-run reflects this run. let mut takeover_warnings: Vec = Vec::new(); - let superseded = super::classify_overlap_takeover(&args.common.cwd).await.redirect; + let superseded = super::classify_overlap_takeover(&args.common.cwd) + .await + .redirect; if !superseded.is_empty() { takeover_warnings.push(serde_json::json!({ "code": super::REDIRECT_SUPERSEDES_VENDORED, @@ -537,6 +539,19 @@ pub(super) async fn run_redirect( })); } + // `--prune` is a no-op in hosted mode (both hosted terminals return + // before the GC blocks): make that explicit in the JSON `warnings[]` + // rather than silently dropping the flag — a bot migrating from + // `--mode agent --prune` must see WHY it stopped pruning. The human + // path warns once up front in `run` (before this flow is entered). + let mut prune_warnings: Vec = Vec::new(); + if args.prune || args.sync { + prune_warnings.push(serde_json::json!({ + "code": super::REDIRECT_PRUNE_IGNORED, + "detail": super::REDIRECT_PRUNE_IGNORED_DETAIL, + })); + } + // Emit an OpenVEX attestation when `--vex` was requested. The redirected // bytes are fetched from the hosted patch server at install time, so the // PURLs CONFIRMED REDIRECTED BY THIS RUN are attested from the ledger @@ -579,6 +594,7 @@ pub(super) async fn run_redirect( warnings.extend(rush_warnings.iter().cloned()); warnings.extend(pnpm_warnings.iter().cloned()); warnings.extend(takeover_warnings.iter().cloned()); + warnings.extend(prune_warnings.iter().cloned()); // Nest the redirect result under `redirect` inside the classic scan // object (built by `run`, threaded in via `scan_result`), mirroring // vendored mode's nested `vendor` block. This keeps the hosted `--json` diff --git a/crates/socket-patch-cli/src/commands/scan/mod.rs b/crates/socket-patch-cli/src/commands/scan/mod.rs index b76700ab..a51af057 100644 --- a/crates/socket-patch-cli/src/commands/scan/mod.rs +++ b/crates/socket-patch-cli/src/commands/scan/mod.rs @@ -93,6 +93,9 @@ impl ScanMode { /// * `--sync` implies `--apply`, so it counts as an agent-mode spelling; /// `--prune` is an orthogonal GC knob and never conflicts. (`--sync`'s /// prune half is orthogonal too, and stays a separate read in `run`.) +/// Hosted mode runs no GC, so `--mode hosted --prune` stays accepted but +/// emits an explicit `redirect_prune_ignored` warning in `run` rather +/// than silently dropping the flag. /// * `--detached` requires vendored mode in either spelling. The former /// clap-level `requires = "vendor"` couldn't see `--mode vendored`, so /// the requirement moved here too. @@ -167,7 +170,8 @@ pub struct ScanArgs { /// blob, diff, and package-archive files from `.socket/`. Off by /// default to preserve manifest state across temporary uninstalls; /// pair with `--apply` (or use `--sync`) for the auto-update - /// workflow. + /// workflow. No effect in hosted mode (which runs no GC): the run + /// proceeds with an explicit `redirect_prune_ignored` warning. #[arg(long, default_value_t = false)] pub prune: bool, @@ -446,6 +450,19 @@ pub(super) const REDIRECT_SUPERSEDES_VENDORED: &str = "redirect_supersedes_vendo /// a committed hosted redirect ledger still claims. pub(super) const VENDOR_SUPERSEDES_REDIRECT: &str = "vendor_supersedes_redirect"; +/// Warning code + detail emitted when `--prune` is combined with +/// `--mode hosted`: both hosted terminals return before the GC blocks, so +/// the flag would otherwise be silently dropped — a bot migrating its sync +/// job from `--mode agent --prune` to `--mode hosted --prune` would stop +/// pruning forever with exit 0 and no signal. `--prune` stays accepted +/// (CLI_CONTRACT.md: an orthogonal GC knob, never a usage error), but the +/// no-op must be explicit in both the JSON `warnings[]` and stderr. +pub(super) const REDIRECT_PRUNE_IGNORED: &str = "redirect_prune_ignored"; +pub(super) const REDIRECT_PRUNE_IGNORED_DETAIL: &str = + "--prune has no effect with --mode hosted: the hosted flow rewrites lockfiles only and \ + runs no GC sweep of `.socket/` state; run `scan --prune` (agent mode) or \ + `scan --mode vendored --prune` to garbage-collect"; + /// The PURLs claimed by BOTH the hosted redirect ledger /// (`.socket/vendor/redirect-state.json`) and the vendored state ledger /// (`.socket/vendor/state.json`) in `cwd`, sorted. A non-empty result means @@ -462,31 +479,76 @@ pub(super) async fn overlapping_ledger_purls(cwd: &Path) -> Vec { let Ok(vendor) = socket_patch_core::vendor::load_state(cwd).await else { return Vec::new(); }; - if redirect.records.is_empty() || vendor.entries.is_empty() { + if vendor.entries.is_empty() { return Vec::new(); } // Canonicalize both sides (drop qualifiers, percent-decode) so the API // purl form the redirect records carry matches the vendor entry's base // purl — mirrors `vendored_ledger_supplement`. let canon = |p: &str| normalize_purl(strip_purl_qualifiers(p)).into_owned(); - let redirect_purls: std::collections::BTreeSet = - redirect.records.keys().map(|p| canon(p)).collect(); let mut vendor_purls: std::collections::BTreeSet = std::collections::BTreeSet::new(); for (key, entry) in &vendor.entries { vendor_purls.insert(canon(key)); vendor_purls.insert(canon(&entry.base_purl)); } - redirect_purls - .intersection(&vendor_purls) - .cloned() + if !redirect.records.is_empty() { + let redirect_purls: std::collections::BTreeSet = + redirect.records.keys().map(|p| canon(p)).collect(); + return redirect_purls + .intersection(&vendor_purls) + .cloned() + .collect(); + } + // The records map can be EMPTY while the ledger still asserts stale lock + // wiring: a run where every per-uuid record fetch failed persists its + // edits with no records (`record_fetch_failed`). Deriving the redirect + // side of the overlap from record keys alone would leave the takeover + // machinery blind to exactly that degraded ledger, so fall back to + // matching the vendored purls against the recorded edit keys — npm + // `node_modules/` (possibly nested), pnpm/yarn/cargo/uv + // `@`, bun `/`, gem/composer/pypi bare + // ``. Name-level matching can over-claim across versions, but the + // direction gate in `classify_overlap_takeover` still requires the live + // lock to prove one side before anything is reported. + if redirect.edits.is_empty() { + return Vec::new(); + } + vendor_purls + .into_iter() + .filter(|purl| { + let Some((name, version)) = purl_name_version(purl) else { + return false; + }; + redirect + .edits + .iter() + .filter_map(|e| e.key.as_deref()) + .any(|key| { + key == name + || key == format!("{name}@{version}") + || key.ends_with(&format!("/{name}")) + }) + }) .collect() } +/// `pkg:/@` → `(, )`; the name keeps any +/// namespace slashes (`@scope/pkg`, `vendor/pkg`). `None` when either part +/// is missing. Input is already canonicalized by the caller. +fn purl_name_version(purl: &str) -> Option<(&str, &str)> { + let rest = strip_purl_qualifiers(purl).strip_prefix("pkg:")?; + let (_, coord) = rest.split_once('/')?; + let at = coord.rfind('@').filter(|&i| i > 0)?; + Some((&coord[..at], &coord[at + 1..])) +} + /// The overlapping PURLs split by which mode the LIVE lockfile actually wires /// them to right now — the truth source for takeover direction. /// /// `redirect` holds the overlap PURLs the lock currently routes to the hosted -/// patch server (`patch.socket.dev`): hosted genuinely won the lockfile, so the +/// patch server (see [`hosted_wiring_live`] — proved by the record's patch +/// uuid on any host, not a hardcoded hostname): hosted genuinely won the +/// lockfile, so the /// vendored ledger entry (and its now-orphaned tarball) is the stale one and /// `redirect_supersedes_vendored` is truthful. `vendored` holds the PURLs the /// lock currently routes to a committed `.socket/vendor//` artifact: @@ -520,21 +582,38 @@ pub(super) async fn classify_overlap_takeover(cwd: &Path) -> OverlapTakeover { return out; }; let canon = |p: &str| normalize_purl(strip_purl_qualifiers(p)).into_owned(); - let mut vendor_by_purl: std::collections::HashMap = - std::collections::HashMap::new(); + let mut vendor_by_purl: std::collections::HashMap< + String, + &socket_patch_core::vendor::VendorEntry, + > = std::collections::HashMap::new(); for (key, entry) in &vendor.entries { vendor_by_purl.entry(canon(key)).or_insert(entry); - vendor_by_purl.entry(canon(&entry.base_purl)).or_insert(entry); + vendor_by_purl + .entry(canon(&entry.base_purl)) + .or_insert(entry); + } + // The hosted proof needs the redirect ledger too: each record's patch + // uuid (embedded in every hosted artifact URL, whatever the host) and + // the lockfiles the redirect actually edited. + let redirect_state = socket_patch_core::patch::redirect::load_redirect_state(cwd).await; + let mut redirect_uuid_by_purl: std::collections::HashMap = + std::collections::HashMap::new(); + let mut redirect_files: Vec<&str> = Vec::new(); + if let Some(redirect) = &redirect_state { + for (key, record) in &redirect.records { + redirect_uuid_by_purl + .entry(canon(key)) + .or_insert(record.uuid.as_str()); + } + redirect_files = redirect.edits.iter().map(|e| e.path.as_str()).collect(); + redirect_files.sort(); + redirect_files.dedup(); } - // The scan inventory keeps only http(s) `resolved` URLs and DROPS our own - // `file:.socket/vendor/…` specs (see `lock_inventory`), so a - // `patch.socket.dev` resolved for a purl is a purl-scoped proof the lock now - // points at hosted. let inventory = socket_patch_core::vendor::lock_inventory::inventory_project(cwd).await; for purl in overlap { - let hosted_live = socket_patch_core::vendor::lock_inventory::lookup(&inventory, &purl) - .and_then(|e| e.resolved.as_deref()) - .is_some_and(|r| r.contains("patch.socket.dev")); + let record_uuid = redirect_uuid_by_purl.get(&purl).copied(); + let hosted_live = + hosted_wiring_live(cwd, &purl, record_uuid, &redirect_files, &inventory).await; let vendored_live = match vendor_by_purl.get(&purl) { Some(entry) => vendored_wiring_live(cwd, entry).await, None => false, @@ -552,6 +631,81 @@ pub(super) async fn classify_overlap_takeover(cwd: &Path) -> OverlapTakeover { out } +/// Whether the LIVE lockfile provably wires `purl` to a HOSTED patch +/// artifact. Two proofs, tried in order: +/// +/// 1. Inventory: the lock's `resolved` URL for this exact purl carries the +/// redirect record's patch uuid — every hosted artifact URL embeds it, +/// on ANY patch-server host (staging, self-hosted `--patch-server-url` +/// deployments), so this is not pinned to the default `patch.socket.dev` +/// hostname (kept only as a fallback for a ledger with no record). +/// 2. Text: the record's patch uuid appears in a lockfile the redirect +/// ledger recorded editing, OUTSIDE a committed `.socket/vendor//` +/// path. This covers the flavors the inventory structurally cannot see — +/// yarn-berry (its inventory `resolved` is always `None`; the hosted URL +/// lives percent-encoded in the `::__archiveUrl=` binding) and bun (the +/// inventory skips the URL 3-tuples hosted mode writes). The vendored +/// wiring embeds the SAME uuid in its `.socket/vendor///` +/// path, so a bare containment check would prove the wrong mode — only +/// non-vendored-path occurrences count. +/// +/// No record uuid and no default-host inventory match ⇒ `false` (the caller +/// then stays silent rather than guess). +async fn hosted_wiring_live( + cwd: &Path, + purl: &str, + record_uuid: Option<&str>, + redirect_files: &[&str], + inventory: &[socket_patch_core::vendor::lock_inventory::LockfileEntry], +) -> bool { + if let Some(resolved) = socket_patch_core::vendor::lock_inventory::lookup(inventory, purl) + .and_then(|e| e.resolved.as_deref()) + { + if resolved.contains("patch.socket.dev") + || record_uuid.is_some_and(|uuid| resolved.contains(uuid)) + { + return true; + } + } + let Some(uuid) = record_uuid else { + return false; + }; + let Some(eco) = strip_purl_qualifiers(purl) + .strip_prefix("pkg:") + .and_then(|rest| rest.split_once('/')) + .map(|(eco, _)| eco) + else { + return false; + }; + let vendored_prefix = format!("vendor/{eco}/"); + for file in redirect_files { + if !is_safe_project_rel_file(file) { + continue; + } + let Ok(text) = tokio::fs::read_to_string(cwd.join(file)).await else { + continue; + }; + let mut search_from = 0; + while let Some(pos) = text[search_from..].find(uuid) { + let idx = search_from + pos; + if !text[..idx].ends_with(&vendored_prefix) { + return true; + } + search_from = idx + uuid.len(); + } + } + false +} + +/// The ledgers are tamper-able: only ever READ a plain in-project relative +/// lockfile name recorded in them — never one that could climb out of `cwd`. +fn is_safe_project_rel_file(file: &str) -> bool { + !(file.is_empty() + || file.starts_with('/') + || file.starts_with('\\') + || file.split(['/', '\\']).any(|c| c == "..")) +} + /// Whether the LIVE lockfile still wires `entry` to its committed /// `.socket/vendor//` artifact. Reads the lockfile(s) this entry /// recorded editing (the same set `--revert` walks) and looks for that exact @@ -568,13 +722,7 @@ async fn vendored_wiring_live(cwd: &Path, entry: &socket_patch_core::vendor::Ven files.sort(); files.dedup(); for file in files { - // state.json is tamper-able: only ever READ a plain in-project relative - // lockfile name — never one that could climb out of `cwd`. - if file.is_empty() - || file.starts_with('/') - || file.starts_with('\\') - || file.split(['/', '\\']).any(|c| c == "..") - { + if !is_safe_project_rel_file(file) { continue; } if let Ok(text) = tokio::fs::read_to_string(cwd.join(file)).await { @@ -590,6 +738,15 @@ async fn vendored_wiring_live(cwd: &Path, entry: &socket_patch_core::vendor::Ven /// package(s). `current_is_hosted` selects the direction: `true` when a /// hosted redirect displaced a vendored ledger, `false` when a vendored run /// displaced a hosted redirect ledger. +/// +/// The warning fires PER PACKAGE (the direction is proved per purl by the +/// live lockfile), so the remediation must be per-package and non-destructive +/// too. It must never tell the user to delete a whole ledger file or a whole +/// `.socket/vendor//` tree: both may still carry LIVE data for packages +/// this takeover did not touch — the redirect ledger holds other packages' +/// records (VEX reads them) plus the recorded pre-redirect lockfile originals +/// (the only revert data), and the `/` tree holds every vendored uuid +/// dir, including packages the hosted run skipped. pub(super) fn mode_takeover_detail(superseded: &[String], current_is_hosted: bool) -> String { let list = superseded.join(", "); if current_is_hosted { @@ -598,19 +755,25 @@ pub(super) fn mode_takeover_detail(superseded: &[String], current_is_hosted: boo `.socket/vendor/state.json` still claims these package(s) and their \ committed tarball(s) under `.socket/vendor/` are now orphaned — the \ lockfile points at the hosted patch server, not the vendored files. \ - Remove the stale vendored ledger and orphaned artifacts (run \ - `socket-patch vendor --revert` before redirecting, or delete the \ - orphaned `.socket/vendor//` tree) so audits and VEX do not read \ - superseded wiring." + Clean up per package: run `socket-patch remove ` for each \ + package listed above (it drops only that entry and its own \ + `.socket/vendor///` artifact directory), so audits and \ + VEX do not read superseded wiring. Do not delete the whole \ + `.socket/vendor//` tree and do not run `vendor --revert`: \ + other vendored package(s) may still be live in the lockfile and \ + would break or be mass-reverted." ) } else { format!( "vendored artifacts superseded the hosted redirect ledger for: {list}. \ `.socket/vendor/redirect-state.json` still records a hosted redirect for \ these package(s), but the lockfile now points at the committed \ - `.socket/vendor/` files. Remove the stale redirect ledger \ - (`.socket/vendor/redirect-state.json`) so audits and VEX do not read \ - superseded wiring." + `.socket/vendor/` files. Clean up per package: edit \ + `.socket/vendor/redirect-state.json` and delete only these package(s)' \ + entries under `records`, so audits and VEX do not read superseded \ + wiring. Do not delete the ledger file itself: it may still hold live \ + redirect records for other package(s), plus the recorded pre-redirect \ + lockfile originals (`edits`) a future revert needs." ) } } @@ -670,6 +833,15 @@ pub async fn run(mut args: ScanArgs) -> i32 { let hosted = args.mode == Some(ScanMode::Hosted); let prune = args.prune || args.sync; + // Hosted mode runs no GC (both hosted terminals return before the GC + // blocks): say so ONCE up front on the human path instead of silently + // dropping the flag. The `--json` path carries the same warning in the + // `redirect.warnings[]` array (see `run_redirect` and the zero-discovery + // envelope below). + if hosted && prune && !args.common.json && !args.common.silent { + eprintln!("Warning ({REDIRECT_PRUNE_IGNORED}): {REDIRECT_PRUNE_IGNORED_DETAIL}"); + } + // A zero batch size would panic the API-query loop below: both // `all_purls.len().div_ceil(batch_size)` and `all_purls.chunks(batch_size)` // abort the process on a divisor/chunk-size of 0. `--batch-size 0` @@ -821,14 +993,23 @@ pub async fn run(mut args: ScanArgs) -> i32 { }); // Hosted mode: keep the `--json` envelope schema-consistent with // the ≥1-package path by including a (no-op) nested `redirect` - // block — nothing was discovered, so nothing is redirected. + // block — nothing was discovered, so nothing is redirected. The + // prune-ignored warning still rides along: hosted runs no GC even + // when the crawl is empty. if hosted { + let mut warnings: Vec = Vec::new(); + if prune { + warnings.push(serde_json::json!({ + "code": REDIRECT_PRUNE_IGNORED, + "detail": REDIRECT_PRUNE_IGNORED_DETAIL, + })); + } result["redirect"] = serde_json::json!({ "mode": "hosted", "redirected": 0, "rewrittenFiles": [], "skipped": [], - "warnings": [], + "warnings": warnings, "dryRun": args.common.dry_run, }); } @@ -2095,7 +2276,10 @@ mod tests { "hosted flow must not warn when the lock is vendored: {takeover:?}" ); // Truthful direction: vendored won ⇒ the redirect ledger is the stale one. - assert_eq!(takeover.vendored, vec!["pkg:npm/minimist@1.2.2".to_string()]); + assert_eq!( + takeover.vendored, + vec!["pkg:npm/minimist@1.2.2".to_string()] + ); // Pre-fix the hosted flow keyed off the raw overlap, which is non-empty // — it WOULD have wrongly told the user to delete the live ledger. assert!(!overlapping_ledger_purls(root).await.is_empty()); @@ -2119,7 +2303,10 @@ mod tests { "vendored flow must not warn when the lock is hosted: {takeover:?}" ); // Truthful direction: hosted won ⇒ the vendored ledger is the stale one. - assert_eq!(takeover.redirect, vec!["pkg:npm/minimist@1.2.2".to_string()]); + assert_eq!( + takeover.redirect, + vec!["pkg:npm/minimist@1.2.2".to_string()] + ); } #[tokio::test] @@ -2142,4 +2329,263 @@ mod tests { vec!["pkg:npm/minimist@1.2.2".to_string()] ); } + + // ---- remediation is per-package and non-destructive --------------------- + + #[test] + fn takeover_detail_remediation_is_per_package_and_non_destructive() { + // Regression: the remediation used to instruct whole-ledger / + // whole-tree deletion, destroying live data for packages the takeover + // did not touch — the redirect ledger holds OTHER packages' records + // (VEX reads them) plus the only recorded pre-redirect originals, and + // the `.socket/vendor//` tree holds EVERY vendored uuid dir. + // Cleanup must be scoped per named package. + let purls = vec!["pkg:npm/minimist@1.2.2".to_string()]; + + let hosted = mode_takeover_detail(&purls, /*current_is_hosted=*/ true); + // The sanctioned per-purl cleanup command… + assert!( + hosted.contains("socket-patch remove "), + "hosted remediation must be per-package: {hosted}" + ); + // …never whole-tree deletion, and never a blanket revert (which would + // mass-revert unrelated still-live vendored packages). + assert!( + !hosted.contains("delete the orphaned"), + "hosted remediation must not advise tree deletion: {hosted}" + ); + assert!( + hosted.contains("Do not delete the whole"), + "hosted remediation must warn against tree deletion: {hosted}" + ); + assert!( + !hosted.contains("vendor --revert` before redirecting"), + "hosted remediation must not advise a blanket revert: {hosted}" + ); + + let vendored = mode_takeover_detail(&purls, /*current_is_hosted=*/ false); + // Only the named packages' records — never the whole ledger file. + assert!( + vendored.contains("only these package(s)"), + "vendored remediation must be per-package: {vendored}" + ); + assert!( + !vendored.contains("Remove the stale redirect ledger"), + "vendored remediation must not advise deleting the ledger: {vendored}" + ); + assert!( + vendored.contains("Do not delete the ledger file"), + "vendored remediation must warn against file deletion: {vendored}" + ); + } + + // ---- takeover blind spots: degraded ledgers and hosted-proof gaps ------ + + fn redirect_edit(path: &str, key: &str) -> socket_patch_core::patch::redirect::FileEdit { + socket_patch_core::patch::redirect::FileEdit { + path: path.to_string(), + kind: "redirect_npm_lock_entry".to_string(), + action: "modified".to_string(), + key: Some(key.to_string()), + original: None, + new: None, + } + } + + /// Like [`write_redirect_ledger`] but with explicit `edits` (and possibly + /// NO records — the degraded shape a run with failed record fetches + /// persists). + async fn write_redirect_ledger_with_edits( + root: &Path, + purls: &[&str], + edits: Vec, + ) { + use socket_patch_core::patch::redirect::RedirectState; + let mut state = RedirectState::new(); + for purl in purls { + state.records.insert((*purl).to_string(), takeover_record()); + } + state.edits = edits; + let dir = root.join(".socket/vendor"); + tokio::fs::create_dir_all(&dir).await.unwrap(); + tokio::fs::write( + dir.join("redirect-state.json"), + serde_json::to_string_pretty(&state).unwrap(), + ) + .await + .unwrap(); + } + + #[tokio::test] + async fn overlap_detected_when_redirect_ledger_has_edits_but_no_records() { + // A hosted run where every per-uuid record fetch failed persists a + // ledger with edits but an EMPTY records map (`record_fetch_failed`). + // That ledger still asserts stale lock wiring, so a vendored takeover + // of the same package must still be flagged — deriving the overlap + // from record keys alone was blind to exactly this ledger. + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + write_redirect_ledger_with_edits( + root, + &[], + vec![redirect_edit("package-lock.json", "node_modules/minimist")], + ) + .await; + write_vendor_ledger_wired(root, &["pkg:npm/minimist@1.2.2"]).await; + write_lock_pointing_at_vendored(root, "minimist", "1.2.2").await; + + assert_eq!( + overlapping_ledger_purls(root).await, + vec!["pkg:npm/minimist@1.2.2".to_string()], + "an edits-only redirect ledger must still count as overlapping" + ); + let takeover = classify_overlap_takeover(root).await; + assert_eq!( + takeover.vendored, + vec!["pkg:npm/minimist@1.2.2".to_string()], + "the vendored takeover of a degraded redirect ledger must be flagged" + ); + assert!(takeover.redirect.is_empty(), "{takeover:?}"); + } + + /// A grant token as it appears between the host and the patch uuid in + /// hosted artifact URLs. + const TAKEOVER_TOKEN: &str = "33333333-3333-4333-8333-333333333333"; + + #[tokio::test] + async fn hosted_direction_provable_on_non_default_patch_host() { + // Hosted artifact URLs embed the record's patch uuid on ANY host + // (staging / self-hosted `--patch-server-url` deployments), so the + // liveness proof must not be pinned to the `patch.socket.dev` + // hostname. + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + write_redirect_ledger(root, &["pkg:npm/minimist@1.2.2"]).await; + write_vendor_ledger_wired(root, &["pkg:npm/minimist@1.2.2"]).await; + let lock = serde_json::json!({ + "name": "app", + "lockfileVersion": 3, + "requires": true, + "packages": { + "": { "name": "app", "version": "0.0.0" }, + "node_modules/minimist": { + "version": "1.2.2", + "resolved": format!( + "https://patches.example.com/patch/npm/{TAKEOVER_TOKEN}/{TAKEOVER_UUID}/minimist-1.2.2.tgz" + ), + "integrity": format!("sha512-{}", "a".repeat(86)), + }, + }, + }); + tokio::fs::write( + root.join("package-lock.json"), + serde_json::to_string_pretty(&lock).unwrap(), + ) + .await + .unwrap(); + + let takeover = classify_overlap_takeover(root).await; + assert_eq!( + takeover.redirect, + vec!["pkg:npm/minimist@1.2.2".to_string()], + "a non-default patch host must still prove hosted is live" + ); + assert!(takeover.vendored.is_empty(), "{takeover:?}"); + } + + #[tokio::test] + async fn hosted_direction_provable_for_bun_url_tuple() { + // The bun inventory skips the URL 3-tuples hosted mode writes, so + // hosted liveness must be provable from the redirect-edited lockfile + // text (the record's uuid outside any vendored path). + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + write_redirect_ledger_with_edits( + root, + &["pkg:npm/minimist@1.2.2"], + vec![redirect_edit("bun.lock", "minimist")], + ) + .await; + write_vendor_ledger_wired(root, &["pkg:npm/minimist@1.2.2"]).await; + tokio::fs::write( + root.join("bun.lock"), + format!( + "{{\n \"lockfileVersion\": 1,\n \"packages\": {{\n \ + \"minimist\": [\"minimist@https://patch.socket.dev/patch/npm/{TAKEOVER_TOKEN}/{TAKEOVER_UUID}/minimist-1.2.2.tgz\", {{}}, \"sha512-AAA\"],\n \ + }}\n}}\n" + ), + ) + .await + .unwrap(); + + let takeover = classify_overlap_takeover(root).await; + assert_eq!( + takeover.redirect, + vec!["pkg:npm/minimist@1.2.2".to_string()], + "a bun URL 3-tuple must prove hosted is live" + ); + assert!(takeover.vendored.is_empty(), "{takeover:?}"); + } + + #[tokio::test] + async fn hosted_direction_provable_for_berry_archive_url() { + // The berry inventory always emits `resolved: None`; the hosted URL + // lives percent-encoded in the `::__archiveUrl=` binding. The uuid + // survives encoding verbatim, so the text proof must see it. + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + write_redirect_ledger_with_edits( + root, + &["pkg:npm/minimist@1.2.2"], + vec![redirect_edit("yarn.lock", "minimist@1.2.2")], + ) + .await; + write_vendor_ledger_wired(root, &["pkg:npm/minimist@1.2.2"]).await; + tokio::fs::write( + root.join("yarn.lock"), + format!( + "__metadata:\n version: 8\n cacheKey: 10c0\n\n\ + \"minimist@npm:1.2.2\":\n version: 1.2.2\n \ + resolution: \"minimist@npm:1.2.2::__archiveUrl=https%3A%2F%2Fpatch.socket.dev%2Fpatch%2Fnpm%2F{TAKEOVER_TOKEN}%2F{TAKEOVER_UUID}%2Fminimist-1.2.2.tgz\"\n" + ), + ) + .await + .unwrap(); + + let takeover = classify_overlap_takeover(root).await; + assert_eq!( + takeover.redirect, + vec!["pkg:npm/minimist@1.2.2".to_string()], + "a berry __archiveUrl binding must prove hosted is live" + ); + assert!(takeover.vendored.is_empty(), "{takeover:?}"); + } + + #[tokio::test] + async fn vendored_path_uuid_does_not_prove_hosted() { + // The vendored wiring embeds the SAME patch uuid in its + // `.socket/vendor///` path. When the redirect ledger + // names the same lockfile, those occurrences must NOT read as + // hosted proof — the lock points at the vendored files. + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + write_redirect_ledger_with_edits( + root, + &["pkg:npm/minimist@1.2.2"], + vec![redirect_edit("package-lock.json", "node_modules/minimist")], + ) + .await; + write_vendor_ledger_wired(root, &["pkg:npm/minimist@1.2.2"]).await; + write_lock_pointing_at_vendored(root, "minimist", "1.2.2").await; + + let takeover = classify_overlap_takeover(root).await; + assert!( + takeover.redirect.is_empty(), + "a vendored-path uuid must not prove hosted: {takeover:?}" + ); + assert_eq!( + takeover.vendored, + vec!["pkg:npm/minimist@1.2.2".to_string()] + ); + } } diff --git a/crates/socket-patch-cli/tests/in_process_redirect.rs b/crates/socket-patch-cli/tests/in_process_redirect.rs index d302aa48..420269cc 100644 --- a/crates/socket-patch-cli/tests/in_process_redirect.rs +++ b/crates/socket-patch-cli/tests/in_process_redirect.rs @@ -1970,3 +1970,66 @@ async fn redirect_json_mode_write_failures_emit_error_envelope() { let out = run_leg(tmp.path(), &server).await; assert_error_envelope(&out, "ledger-write failure"); } + +/// `scan --mode hosted --prune` must not silently drop `--prune`: both +/// hosted terminals return before the GC blocks, so a bot migrating its sync +/// job from `--mode agent --prune` would otherwise stop pruning forever with +/// exit 0 and no signal. The envelope must carry an explicit +/// `redirect_prune_ignored` warning (and, unchanged, no `gc` object — +/// hosted mode runs no GC). +#[tokio::test] +#[serial] +async fn hosted_prune_emits_explicit_ignored_warning() { + let server = MockServer::start().await; + mock_discovery(&server).await; + mock_reference(&server).await; + mock_view(&server).await; + + let tmp = tempfile::tempdir().unwrap(); + write_project(tmp.path()); + + let out = scrubbed_cli() + .args([ + "scan", + "--mode", + "hosted", + "--prune", + "--json", + "--yes", + "--cwd", + tmp.path().to_str().unwrap(), + "--api-url", + &server.uri(), + "--org", + ORG, + "--api-token", + "fake", + ]) + .output() + .expect("run socket-patch"); + assert_eq!( + out.status.code(), + Some(0), + "scan --mode hosted --prune must still succeed; stdout=\n{}\nstderr=\n{}", + String::from_utf8_lossy(&out.stdout), + String::from_utf8_lossy(&out.stderr), + ); + let env_json: serde_json::Value = serde_json::from_slice(&out.stdout).unwrap_or_else(|e| { + panic!( + "--json stdout must be a JSON envelope: {e}\nstdout:\n{}", + String::from_utf8_lossy(&out.stdout) + ) + }); + assert!( + warning_codes(&env_json) + .iter() + .any(|c| c == "redirect_prune_ignored"), + "hosted --prune must warn that the flag is ignored; envelope: {env_json}" + ); + assert!( + env_json.get("gc").is_none(), + "hosted mode must not run (or claim to run) GC: {env_json}" + ); + // The redirect itself is unaffected by the ignored flag. + assert_eq!(env_json["redirect"]["redirected"], 1, "{env_json}"); +} From d52e29bcd6c7ba45d03a64d316eed5577732eea5 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Fri, 14 Aug 2026 20:11:58 -0400 Subject: [PATCH 2/2] fix(scan): make takeover cleanup advice complete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both mode-takeover remediations were incomplete in ways that hurt the operator who followed them. The vendored-direction text named only the stale package's `records` entries. When the takeover cleared the LAST record, the leftover `edits` still matched the package through the degraded-ledger fallback, so the identical warning fired on every later run — repeating advice that could no longer be carried out, since `records` was already empty. The text now names the matching `edits` entries too (that package's stale pre-redirect originals, which a later redirect revert would replay over the live vendored wiring), and says why both halves matter. Following it in full now leaves nothing to warn about. The hosted-direction text said `socket-patch remove ` "drops only that entry and its own artifact directory". It also deletes the package's `.socket/manifest.json` entry, so a reader budgeting for a ledger-scoped edit — a bot passing --yes especially — was mis-told the blast radius. The text now states that, places the live hosted patch (recorded in the redirect ledger, which `remove` never touches), notes that in-place file rollback is skipped for vendor-owned packages, and suggests previewing with --dry-run. Detection is unchanged: the degraded-ledger blind spot stays closed, because a hand-cleaned ledger and one left by failed record fetches are indistinguishable from their contents. Co-authored-by: Claude Fable 5 --- .../socket-patch-cli/src/commands/scan/mod.rs | 116 ++++++++++++++++-- 1 file changed, 109 insertions(+), 7 deletions(-) diff --git a/crates/socket-patch-cli/src/commands/scan/mod.rs b/crates/socket-patch-cli/src/commands/scan/mod.rs index e7701ced..31ef48e4 100644 --- a/crates/socket-patch-cli/src/commands/scan/mod.rs +++ b/crates/socket-patch-cli/src/commands/scan/mod.rs @@ -776,6 +776,20 @@ async fn vendored_wiring_live(cwd: &Path, entry: &socket_patch_core::vendor::Ven /// records (VEX reads them) plus the recorded pre-redirect lockfile originals /// (the only revert data), and the `/` tree holds every vendored uuid /// dir, including packages the hosted run skipped. +/// +/// Per package also has to mean COMPLETE per package, or the remediation does +/// not converge: +/// +/// * The vendored direction names the package's `edits` entry alongside its +/// `records` entry. `overlapping_ledger_purls` falls back to matching edit +/// KEYS once `records` is empty (the degraded-ledger blind spot), so a +/// records-only cleanup that happened to delete the last record left the +/// package still matching and this warning firing on every later run — +/// repeating advice the operator had already carried out. +/// * The hosted direction describes `socket-patch remove`'s full blast radius. +/// It deletes the package's `.socket/manifest.json` entry too, not just the +/// vendor ledger entry and artifact dir, and a reader who budgeted for a +/// ledger-only edit needs to know that before running it with `--yes`. pub(super) fn mode_takeover_detail(superseded: &[String], current_is_hosted: bool) -> String { let list = superseded.join(", "); if current_is_hosted { @@ -785,9 +799,15 @@ pub(super) fn mode_takeover_detail(superseded: &[String], current_is_hosted: boo committed tarball(s) under `.socket/vendor/` are now orphaned — the \ lockfile points at the hosted patch server, not the vendored files. \ Clean up per package: run `socket-patch remove ` for each \ - package listed above (it drops only that entry and its own \ - `.socket/vendor///` artifact directory), so audits and \ - VEX do not read superseded wiring. Do not delete the whole \ + package listed above, so audits and VEX do not read superseded \ + wiring. It drops that package's vendored ledger entry and its own \ + `.socket/vendor///` artifact directory, AND deletes that \ + package's now-superseded `.socket/manifest.json` entry — that entry \ + describes the vendored delivery, while the live hosted patch is \ + recorded in `.socket/vendor/redirect-state.json`, which `remove` \ + never touches. In-place file rollback is skipped for vendor-owned \ + package(s), so the installed tree is left as the lockfile wires it; \ + preview with `--dry-run` first. Do not delete the whole \ `.socket/vendor//` tree and do not run `vendor --revert`: \ other vendored package(s) may still be live in the lockfile and \ would break or be mass-reverted." @@ -799,10 +819,16 @@ pub(super) fn mode_takeover_detail(superseded: &[String], current_is_hosted: boo these package(s), but the lockfile now points at the committed \ `.socket/vendor/` files. Clean up per package: edit \ `.socket/vendor/redirect-state.json` and delete only these package(s)' \ - entries under `records`, so audits and VEX do not read superseded \ - wiring. Do not delete the ledger file itself: it may still hold live \ - redirect records for other package(s), plus the recorded pre-redirect \ - lockfile originals (`edits`) a future revert needs." + entries under `records` AND their matching entries under `edits`, so \ + audits and VEX do not read superseded wiring. Both halves matter: the \ + leftover `edits` are that package's stale pre-redirect originals, \ + which a later redirect revert would replay over the live vendored \ + wiring — and an `edits` entry left behind still names the package, so \ + a ledger whose last record you just deleted keeps reading as \ + superseded and this warning keeps firing. Do not delete the ledger \ + file itself: it may still hold live redirect records for other \ + package(s), plus the recorded pre-redirect lockfile originals \ + (`edits`) a future revert needs for them." ) } } @@ -2425,6 +2451,32 @@ mod tests { ); } + #[test] + fn hosted_remediation_states_removes_full_blast_radius() { + // Regression: the hosted text said `socket-patch remove ` "drops + // only that entry and its own `.socket/vendor///` artifact + // directory". It also deletes the package's `.socket/manifest.json` + // entry, so a reader budgeting for a ledger-scoped edit — a bot passing + // `--yes`, especially — was mis-told what the command does. + let purls = vec!["pkg:npm/minimist@1.2.2".to_string()]; + let hosted = mode_takeover_detail(&purls, /*current_is_hosted=*/ true); + + assert!( + !hosted.contains("drops only that entry"), + "hosted remediation must not understate `remove`: {hosted}" + ); + assert!( + hosted.contains("`.socket/manifest.json`"), + "hosted remediation must name the manifest entry `remove` deletes: {hosted}" + ); + // …and must place the LIVE hosted patch, so "manifest entry deleted" + // does not read as "the hosted patch was dropped too". + assert!( + hosted.contains("redirect-state.json"), + "hosted remediation must say where the live hosted patch lives: {hosted}" + ); + } + // ---- takeover blind spots: degraded ledgers and hosted-proof gaps ------ fn redirect_edit(path: &str, key: &str) -> socket_patch_core::patch::redirect::FileEdit { @@ -2494,6 +2546,56 @@ mod tests { assert!(takeover.redirect.is_empty(), "{takeover:?}"); } + #[tokio::test] + async fn following_the_vendored_remediation_clears_the_warning() { + // Regression (sticky warning): the vendored remediation used to name + // only the `records` entries. When the takeover cleared the LAST + // record, the leftover `edits` still matched the package through the + // degraded-ledger fallback above, so the identical warning fired on + // every later run — and repeated advice that could no longer be + // followed, since `records` was already empty. The remediation now + // names the matching `edits` entries too; carrying it out in full has + // to leave nothing to warn about. + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + write_redirect_ledger_with_edits( + root, + &["pkg:npm/minimist@1.2.2"], + vec![redirect_edit("package-lock.json", "node_modules/minimist")], + ) + .await; + write_vendor_ledger_wired(root, &["pkg:npm/minimist@1.2.2"]).await; + write_lock_pointing_at_vendored(root, "minimist", "1.2.2").await; + + let before = classify_overlap_takeover(root).await; + assert_eq!( + before.vendored, + vec!["pkg:npm/minimist@1.2.2".to_string()], + "the vendored takeover must be flagged first: {before:?}" + ); + let detail = mode_takeover_detail(&before.vendored, /*current_is_hosted=*/ false); + assert!( + detail.contains("`edits`"), + "the remediation must name the edits entries: {detail}" + ); + + // Exactly what the remediation prescribes for this ledger: the + // package's `records` entry AND its matching `edits` entry gone, the + // ledger file itself left in place. + write_redirect_ledger_with_edits(root, &[], Vec::new()).await; + + let after = classify_overlap_takeover(root).await; + assert_eq!( + after, + OverlapTakeover::default(), + "following the remediation must clear the warning: {after:?}" + ); + assert!( + overlapping_ledger_purls(root).await.is_empty(), + "no residue may keep the ledgers reading as overlapping" + ); + } + /// A grant token as it appears between the host and the patch uuid in /// hosted artifact URLs. const TAKEOVER_TOKEN: &str = "33333333-3333-4333-8333-333333333333";