diff --git a/crates/socket-patch-cli/src/commands/update.rs b/crates/socket-patch-cli/src/commands/update.rs index 5f49e21ef..37df27f47 100644 --- a/crates/socket-patch-cli/src/commands/update.rs +++ b/crates/socket-patch-cli/src/commands/update.rs @@ -90,19 +90,6 @@ async fn fail(args: &UpdateArgs, code: &str, message: &str) -> i32 { 1 } -/// A stable machine name for `channel` (telemetry), unlike the -/// human-facing [`channel_label`] (`cargo install`, `the RubyGems launcher`). -fn channel_id(channel: InstallChannel) -> &'static str { - match channel { - InstallChannel::Standalone => "standalone", - InstallChannel::Npm => "npm", - InstallChannel::Pypi => "pypi", - InstallChannel::Cargo => "cargo", - InstallChannel::LauncherCache => "launcher_cache", - InstallChannel::Homebrew => "homebrew", - } -} - /// The no-op message when there is nothing to install: a pin already /// satisfied, the latest release already running, or a build newer than /// the latest release (`latest` never downgrades). @@ -167,33 +154,15 @@ fn installed_message( } } -/// The status line shown while the release downloads and installs. -fn download_status(target: &semver::Version, asset: &str) -> String { - format!("Downloading socket-patch {target} ({asset})...") -} - /// Record a non-fatal advisory: stderr for humans, `warnings[]` on the -/// envelope for machines. `--json` suppresses the stderr line (stdout is -/// the machine channel and stderr must stay clean), so a warning that only -/// ever went to stderr would vanish entirely for JSON consumers — the -/// managed-install override in particular is the "your package manager -/// will silently revert this" signal, and a silent override is the bug -/// class the channel suite exists to catch. Same stderr-or-envelope -/// split `vendor`/`remove` use for their run-level advisories (the -/// rendered stderr line keeps update's own `Warning: ` wording). +/// envelope for machines, which never see stderr under `--json`. fn note_warning(warnings: &mut Vec, quiet: bool, code: &str, detail: String) { if !quiet { eprintln!("Warning: {}", crate::ui::sentence_case(&detail)); } - warnings.push(RunWarning { - code: code.to_string(), - detail, - }); + warnings.push(RunWarning::new(code, detail)); } -/// The status line while `--update` asks which release is the latest. -const CHECKING_LATEST: &str = "Checking for the latest socket-patch release..."; - pub async fn run(args: UpdateArgs) -> i32 { apply_env_toggles(&args.common); let quiet = args.common.json || args.common.silent; @@ -268,7 +237,7 @@ pub async fn run(args: UpdateArgs) -> i32 { // each with its own connect and read budget): say what is // happening instead of a blank terminal. let mut status = crate::ui::StatusLine::stderr(args.common.json, args.common.silent); - status.set(CHECKING_LATEST); + status.set("Checking for the latest socket-patch release..."); let latest = fetch_latest_version(&endpoints, &timeouts).await; status.finish(); match latest { @@ -369,7 +338,9 @@ pub async fn run(args: UpdateArgs) -> i32 { // download can take a while (300 s budget), so a terminal gets a // status line instead of a silent pause after the prompt. let mut status = crate::ui::StatusLine::stderr(args.common.json, args.common.silent); - status.set(download_status(&target_version, &asset)); + status.set(format!( + "Downloading socket-patch {target_version} ({asset})..." + )); let result = core_update::perform_update(UpdateRequest { target_triple: UPDATE_TARGET, version: &target_version, @@ -400,7 +371,7 @@ pub async fn run(args: UpdateArgs) -> i32 { // Before the result prints, like `fail`'s failure event. track_cli_updated( &target_version.to_string(), - channel_id(channel), + channel.id(), pinned, args.force, &args.common.telemetry_auth(), @@ -441,14 +412,6 @@ pub async fn run(args: UpdateArgs) -> i32 { mod tests { use super::*; - #[test] - fn checking_latest_status_line() { - assert_eq!( - CHECKING_LATEST, - "Checking for the latest socket-patch release..." - ); - } - fn v(s: &str) -> semver::Version { semver::Version::parse(s).unwrap() } @@ -529,21 +492,6 @@ mod tests { ); } - #[test] - fn download_status_and_error_capitalization() { - assert_eq!( - download_status(&v("9.9.9"), "socket-patch-x.tar.gz"), - "Downloading socket-patch 9.9.9 (socket-patch-x.tar.gz)..." - ); - assert_eq!( - crate::ui::sentence_case("could not check for updates: x"), - "Could not check for updates: x" - ); - assert_eq!(crate::ui::sentence_case(""), ""); - assert_eq!(crate::ui::sentence_case("éclair"), "Éclair"); - assert_eq!(crate::ui::sentence_case("Already"), "Already"); - } - #[test] fn help_describes_the_update_flag_not_the_internal_subcommand() { use clap::CommandFactory; diff --git a/crates/socket-patch-cli/src/commands/user_patches.rs b/crates/socket-patch-cli/src/commands/user_patches.rs index 7ac74dcb6..a273a3be0 100644 --- a/crates/socket-patch-cli/src/commands/user_patches.rs +++ b/crates/socket-patch-cli/src/commands/user_patches.rs @@ -33,6 +33,16 @@ pub(crate) async fn inspect(common: &GlobalArgs) -> UserPatchIndex { } } +/// The project-patch sources protecting `purl`, if its ecosystem is selected +/// and it has any. +fn protecting(common: &GlobalArgs, index: &UserPatchIndex, purl: &str) -> Option> { + if !common.purl_ecosystem_selected(purl) { + return None; + } + let sources = index.conflicts(purl); + (!sources.is_empty()).then_some(sources) +} + pub(crate) fn detail(purl: &str, sources: &[String], allowed: bool) -> String { let sources = sources.join(", "); if allowed { @@ -76,13 +86,9 @@ pub(crate) async fn partition( let mut conflicts = Vec::new(); for purl in &purls { let purl = purl.as_str(); - if !common.purl_ecosystem_selected(purl) { + let Some(sources) = protecting(common, &index, purl) else { continue; - } - let sources = index.conflicts(purl); - if sources.is_empty() { - continue; - } + }; let message = detail(purl, &sources, common.allow_patch_conflicts); if common.allow_patch_conflicts { warnings.push((ALLOWED.into(), message)); @@ -106,11 +112,7 @@ pub(crate) async fn evidence( } let index = inspect(common).await; for &purl in purls { - if !common.purl_ecosystem_selected(purl) { - continue; - } - let sources = index.conflicts(purl); - if !sources.is_empty() { + if let Some(sources) = protecting(common, &index, purl) { found.insert(purl.to_string(), sources); } } @@ -149,13 +151,9 @@ pub(crate) async fn skip_protected( } let index = inspect(common).await; patches.retain(|purl, record| { - if !common.purl_ecosystem_selected(purl) { + let Some(sources) = protecting(common, &index, purl) else { return true; - } - let sources = index.conflicts(purl); - if sources.is_empty() { - return true; - } + }; let message = detail(purl, &sources, common.allow_patch_conflicts); if common.allow_patch_conflicts { warnings.push((ALLOWED.into(), message)); diff --git a/crates/socket-patch-cli/src/commands/vendor_orphans.rs b/crates/socket-patch-cli/src/commands/vendor_orphans.rs index abe61651f..eb6eaaa63 100644 --- a/crates/socket-patch-cli/src/commands/vendor_orphans.rs +++ b/crates/socket-patch-cli/src/commands/vendor_orphans.rs @@ -1,7 +1,7 @@ //! The orphan sweep `vendor --revert`, `scan --prune`, `rollback` and //! `remove` share: uuid dirs under `.socket/vendor//` that no ledger //! entry owns (a hand-edited state file, an interrupted run, the losing -//! side of a git merge of two vendor runs, #1610). +//! side of a git merge of two vendor runs). use std::collections::HashSet; use std::path::Path; @@ -24,10 +24,9 @@ pub(crate) struct OrphanSweep { pub(crate) failed: Vec<(vendor::path::SweptVendorDir, String)>, } -/// Uuid dirs under `.socket/vendor//` with no owning `(eco, uuid)` -/// ledger entry (a hand-edited state file, or artifacts left by an -/// interrupted run). Unparseable dirs are never returned (and never -/// deleted). Returns the orphans so callers can emit events / counts. +/// Sweep the uuid dirs under `.socket/vendor//` with no owning +/// `(eco, uuid)` ledger entry. Unparseable dirs are never returned (and +/// never deleted). /// /// A missing ledger entry does NOT prove missing wiring: lockfiles can /// still point into `.socket/vendor/` after a deleted state.json or a @@ -44,7 +43,7 @@ pub(crate) async fn sweep_orphan_vendor_dirs( } /// [`sweep_orphan_vendor_dirs`] limited to the orphans `select` accepts: -/// `rollback` and `remove` sweep only the ones in their scope (#1610). +/// `rollback` and `remove` sweep only the ones in their scope. pub(crate) async fn sweep_orphan_vendor_dirs_where( cwd: &Path, state: &VendorState, @@ -69,12 +68,11 @@ pub(crate) async fn sweep_orphan_vendor_dirs_where( if candidates.is_empty() { return out; } - let wired: HashSet<(String, String)> = - crate::commands::vendored_backend::repair::scan_vendor_references(cwd) - .await - .into_iter() - .map(|(eco, uuid, _path)| (eco, uuid)) - .collect(); + let references = crate::commands::vendored_backend::repair::scan_vendor_references(cwd).await; + let wired: HashSet<(&str, &str)> = references + .iter() + .map(|(eco, uuid, _path)| (eco.as_str(), uuid.as_str())) + .collect(); // Each removal also prunes the `/` and `vendor/` levels it // emptied (never `.socket/` — the lock guard owns that level): the // sweep runs AFTER the per-entry reverts and their ledger saves, so it @@ -82,7 +80,7 @@ pub(crate) async fn sweep_orphan_vendor_dirs_where( // `.socket/vendor//` husks. let stop_dir = cwd.join(SOCKET_DIR); for unit in candidates { - if wired.contains(&(unit.eco.clone(), unit.uuid.clone())) { + if wired.contains(&(unit.eco.as_str(), unit.uuid.as_str())) { out.still_wired.push(unit); continue; } @@ -183,7 +181,7 @@ mod tests { /// `rollback ` / `remove ` scope their orphan sweep /// with [`orphan_in_target`]: a package target matches by the leaves' /// purls, a uuid target by the dir's own uuid, even when no leaf - /// parses (#1610). + /// parses. #[test] fn orphan_in_target_matches_package_and_uuid_targets() { let uuid = "aaaaaaaa-1111-4111-8111-aaaaaaaaaaaa"; @@ -216,8 +214,7 @@ mod tests { /// A kept, still-wired orphan points the user at a remedy that works /// in v5: `repair` no longer rebuilds ledger entries from lockfiles, - /// so the advisory names the ledger restore (and the merge case, - /// #991) or the lockfile restore instead. + /// so the advisory names the ledger restore (and the merge case). #[test] fn orphan_still_wired_names_a_working_remedy() { let unit = vendor::path::SweptVendorDir { @@ -228,15 +225,15 @@ mod tests { }; let warning = orphan_still_wired(&unit); assert_eq!(warning.code, "vendor_orphan_still_wired"); - let detail = &warning.detail; - assert!( - detail.contains(".socket/vendor/npm/aaaaaaaa-1111-4111-8111-aaaaaaaaaaaa"), - "{detail}" + // The advisory follows a revert that rewrote the lockfile, so it + // names no lockfile checkout: that would re-wire what it removed. + assert_eq!( + warning.detail, + "a project lockfile still points at \ + .socket/vendor/npm/aaaaaaaa-1111-4111-8111-aaaaaaaaaaaa, which no ledger \ + entry owns; the artifacts were kept (restore the missing entries in \ + .socket/vendor/state.json from version control (if a git merge kept one \ + side of state.json, merge both sides' `entries`), then revert again)" ); - assert!(!detail.contains("socket-patch repair"), "{detail}"); - assert!(detail.contains("entries"), "{detail}"); - // The advisory follows a revert that rewrote the lockfile: - // checking the lockfile out again would re-wire what it removed. - assert!(!detail.contains("git checkout"), "{detail}"); } } diff --git a/crates/socket-patch-cli/src/commands/vlt_heal.rs b/crates/socket-patch-cli/src/commands/vlt_heal.rs index dc96bd0da..64e4992de 100644 --- a/crates/socket-patch-cli/src/commands/vlt_heal.rs +++ b/crates/socket-patch-cli/src/commands/vlt_heal.rs @@ -29,11 +29,11 @@ pub(crate) struct RewriteHeal { /// Set when the heal ran (it may invalidate store entries): the store's /// bundled copies after it, discovery's only read of the installed tree /// (`Discovery::vlt_bundled_copies`). `None` when nothing was healed. - pub(crate) healed_store: Option>, + pub(crate) healed_store: Option>, } pub(crate) const REINSTALL_REQUIRED: &str = "redirect_vlt_reinstall_required"; -/// A bundled copy of a redirected package that no rewire reaches (#471). +/// A bundled copy of a redirected package that no rewire reaches. pub(crate) const BUNDLED_INSTANCE_SKIPPED: &str = "redirect_vlt_bundled_instance_skipped"; /// The uuids of `deps` whose purl a vlt vendored ledger entry claims: a @@ -85,7 +85,7 @@ pub(crate) async fn artifact_preflight( /// The origins the vlt heal treats as Socket-owned in the final lock: the /// patch server plus `--api-url`. Unlike discovery's /// [`crate::commands::hosted_unwind::patch_server_origins`], it also counts the -/// `--api-url` origin (as it has since vlt support landed, #269), so a +/// `--api-url` origin, so a /// setup serving the hosted tarballs from the API origin is healed too. /// Discovery-facing code must use the `hosted_unwind` helper, not this one. fn vlt_heal_origins(common: &crate::args::GlobalArgs) -> Vec { @@ -98,10 +98,6 @@ fn vlt_heal_origins(common: &crate::args::GlobalArgs) -> Vec { .collect() } -fn cleanup_disabled(common: &crate::args::GlobalArgs) -> bool { - common.dry_run || common.no_vlt_install_cleanup -} - /// How a heal ended, for the advisory wording. #[derive(Debug, Default, PartialEq, Eq)] struct HealTally { @@ -146,7 +142,7 @@ async fn heal_targets( if stale.is_empty() { return tally; } - if cleanup_disabled(common) { + if common.dry_run || common.no_vlt_install_cleanup { tally.stale_left = stale.len(); tally .stale_uuids @@ -392,7 +388,7 @@ pub(crate) async fn heal_after_rewrite( out.stale_purls.insert(purl.clone()); } } - // A bundled copy of a patched package (#471) has no lock node, so the + // A bundled copy of a patched package has no lock node, so the // redirect never reaches it: say so, and keep its purl out of the // in-run attestation (lockfile discovery contests it the same way). if inputs.final_lock.is_some() { @@ -423,12 +419,8 @@ pub(crate) async fn heal_after_rewrite( out.stale_purls.insert(purl.clone()); } } - // A heal implies a final lock, so the block above recorded the store; - // never report a heal without it. - if healed && out.healed_store.is_none() { - out.healed_store = - Some(socket_patch_core::vendor::vlt_bundled::bundled_copies(&common.cwd).await); - } + // Heal targets come from the final lock, so the block above saw it. + debug_assert!(!healed || out.healed_store.is_some()); out } diff --git a/crates/socket-patch-core/src/update/channel.rs b/crates/socket-patch-core/src/update/channel.rs index 7794e6526..ae878b5d7 100644 --- a/crates/socket-patch-core/src/update/channel.rs +++ b/crates/socket-patch-core/src/update/channel.rs @@ -524,6 +524,21 @@ fn is_prefix_global_install(path: &Path) -> bool { } } +impl InstallChannel { + /// A stable machine name for telemetry, unlike the human-facing + /// [`channel_label`] (`cargo install`, `the RubyGems launcher`). + pub fn id(self) -> &'static str { + match self { + InstallChannel::Standalone => "standalone", + InstallChannel::Npm => "npm", + InstallChannel::Pypi => "pypi", + InstallChannel::Cargo => "cargo", + InstallChannel::LauncherCache => "launcher_cache", + InstallChannel::Homebrew => "homebrew", + } + } +} + /// Short human label for refusal messages ("managed by npm"); see /// [`channel_label_for`] for the npm-family manager of a given path. pub fn channel_label(channel: InstallChannel) -> &'static str { @@ -841,6 +856,20 @@ mod tests { ); } + #[test] + fn channel_ids_are_the_telemetry_names() { + for (channel, id) in [ + (InstallChannel::Standalone, "standalone"), + (InstallChannel::Npm, "npm"), + (InstallChannel::Pypi, "pypi"), + (InstallChannel::Cargo, "cargo"), + (InstallChannel::LauncherCache, "launcher_cache"), + (InstallChannel::Homebrew, "homebrew"), + ] { + assert_eq!(channel.id(), id); + } + } + #[test] fn channel_label_covers_every_channel() { // Exact strings: these feed the "--update refused: managed by