Skip to content

Commit 7c7d809

Browse files
Sweep update, user_patches, vendor_orphans, vlt_heal (#1929)
* Start sweep of update, user_patches, orphans Assisted-by: Claude Code:claude-opus-5-5 * Sweep commands/update.rs Inline download_status and the CHECKING_LATEST const into their one call site each, move channel_id to core as InstallChannel::id beside channel_label, build warnings with RunWarning::new and trim the note_warning essay. Delete the test that compared the const to its own literal and the sentence_case asserts that ui/text.rs already covers. Output is unchanged. Assisted-by: Claude Code:claude-opus-5-5 * Share one gate in commands/user_patches.rs partition, evidence and skip_protected each open-coded the same selected-ecosystem and conflicts check; they now call one private protecting(). Behavior is unchanged. Assisted-by: Claude Code:claude-opus-5-5 * Sweep commands/vendor_orphans.rs Borrow the wired (eco, uuid) keys instead of cloning both strings per candidate, drop bare issue refs, and fix the still-wired test doc that claimed the advisory names a lockfile restore. The test now pins the whole advisory instead of three .contains checks. Assisted-by: Claude Code:claude-opus-5-5 * Sweep commands/vlt_heal.rs Inline vlt_vendored_uuids and cleanup_disabled into their one caller each, and delete the healed_store fallback in heal_after_rewrite: a heal needs a heal target, which needs a final lock, so the bundled-copy block above it has always set healed_store. Drop bare issue refs and a full std path. Assisted-by: Claude Code:claude-opus-5-5 * Address review on the update and vlt_heal sweep Keep vlt_vendored_uuids as a named helper, assert the healed-store invariant in debug builds instead of the deleted fallback, and pin InstallChannel::id's telemetry names in a table test. Assisted-by: Claude Code:claude-opus-5-5 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 28eed29 commit 7c7d809

5 files changed

Lines changed: 80 additions & 116 deletions

File tree

‎crates/socket-patch-cli/src/commands/update.rs‎

Lines changed: 7 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -90,19 +90,6 @@ async fn fail(args: &UpdateArgs, code: &str, message: &str) -> i32 {
9090
1
9191
}
9292

93-
/// A stable machine name for `channel` (telemetry), unlike the
94-
/// human-facing [`channel_label`] (`cargo install`, `the RubyGems launcher`).
95-
fn channel_id(channel: InstallChannel) -> &'static str {
96-
match channel {
97-
InstallChannel::Standalone => "standalone",
98-
InstallChannel::Npm => "npm",
99-
InstallChannel::Pypi => "pypi",
100-
InstallChannel::Cargo => "cargo",
101-
InstallChannel::LauncherCache => "launcher_cache",
102-
InstallChannel::Homebrew => "homebrew",
103-
}
104-
}
105-
10693
/// The no-op message when there is nothing to install: a pin already
10794
/// satisfied, the latest release already running, or a build newer than
10895
/// the latest release (`latest` never downgrades).
@@ -167,33 +154,15 @@ fn installed_message(
167154
}
168155
}
169156

170-
/// The status line shown while the release downloads and installs.
171-
fn download_status(target: &semver::Version, asset: &str) -> String {
172-
format!("Downloading socket-patch {target} ({asset})...")
173-
}
174-
175157
/// Record a non-fatal advisory: stderr for humans, `warnings[]` on the
176-
/// envelope for machines. `--json` suppresses the stderr line (stdout is
177-
/// the machine channel and stderr must stay clean), so a warning that only
178-
/// ever went to stderr would vanish entirely for JSON consumers — the
179-
/// managed-install override in particular is the "your package manager
180-
/// will silently revert this" signal, and a silent override is the bug
181-
/// class the channel suite exists to catch. Same stderr-or-envelope
182-
/// split `vendor`/`remove` use for their run-level advisories (the
183-
/// rendered stderr line keeps update's own `Warning: <detail>` wording).
158+
/// envelope for machines, which never see stderr under `--json`.
184159
fn note_warning(warnings: &mut Vec<RunWarning>, quiet: bool, code: &str, detail: String) {
185160
if !quiet {
186161
eprintln!("Warning: {}", crate::ui::sentence_case(&detail));
187162
}
188-
warnings.push(RunWarning {
189-
code: code.to_string(),
190-
detail,
191-
});
163+
warnings.push(RunWarning::new(code, detail));
192164
}
193165

194-
/// The status line while `--update` asks which release is the latest.
195-
const CHECKING_LATEST: &str = "Checking for the latest socket-patch release...";
196-
197166
pub async fn run(args: UpdateArgs) -> i32 {
198167
apply_env_toggles(&args.common);
199168
let quiet = args.common.json || args.common.silent;
@@ -268,7 +237,7 @@ pub async fn run(args: UpdateArgs) -> i32 {
268237
// each with its own connect and read budget): say what is
269238
// happening instead of a blank terminal.
270239
let mut status = crate::ui::StatusLine::stderr(args.common.json, args.common.silent);
271-
status.set(CHECKING_LATEST);
240+
status.set("Checking for the latest socket-patch release...");
272241
let latest = fetch_latest_version(&endpoints, &timeouts).await;
273242
status.finish();
274243
match latest {
@@ -369,7 +338,9 @@ pub async fn run(args: UpdateArgs) -> i32 {
369338
// download can take a while (300 s budget), so a terminal gets a
370339
// status line instead of a silent pause after the prompt.
371340
let mut status = crate::ui::StatusLine::stderr(args.common.json, args.common.silent);
372-
status.set(download_status(&target_version, &asset));
341+
status.set(format!(
342+
"Downloading socket-patch {target_version} ({asset})..."
343+
));
373344
let result = core_update::perform_update(UpdateRequest {
374345
target_triple: UPDATE_TARGET,
375346
version: &target_version,
@@ -400,7 +371,7 @@ pub async fn run(args: UpdateArgs) -> i32 {
400371
// Before the result prints, like `fail`'s failure event.
401372
track_cli_updated(
402373
&target_version.to_string(),
403-
channel_id(channel),
374+
channel.id(),
404375
pinned,
405376
args.force,
406377
&args.common.telemetry_auth(),
@@ -441,14 +412,6 @@ pub async fn run(args: UpdateArgs) -> i32 {
441412
mod tests {
442413
use super::*;
443414

444-
#[test]
445-
fn checking_latest_status_line() {
446-
assert_eq!(
447-
CHECKING_LATEST,
448-
"Checking for the latest socket-patch release..."
449-
);
450-
}
451-
452415
fn v(s: &str) -> semver::Version {
453416
semver::Version::parse(s).unwrap()
454417
}
@@ -529,21 +492,6 @@ mod tests {
529492
);
530493
}
531494

532-
#[test]
533-
fn download_status_and_error_capitalization() {
534-
assert_eq!(
535-
download_status(&v("9.9.9"), "socket-patch-x.tar.gz"),
536-
"Downloading socket-patch 9.9.9 (socket-patch-x.tar.gz)..."
537-
);
538-
assert_eq!(
539-
crate::ui::sentence_case("could not check for updates: x"),
540-
"Could not check for updates: x"
541-
);
542-
assert_eq!(crate::ui::sentence_case(""), "");
543-
assert_eq!(crate::ui::sentence_case("éclair"), "Éclair");
544-
assert_eq!(crate::ui::sentence_case("Already"), "Already");
545-
}
546-
547495
#[test]
548496
fn help_describes_the_update_flag_not_the_internal_subcommand() {
549497
use clap::CommandFactory;

‎crates/socket-patch-cli/src/commands/user_patches.rs‎

Lines changed: 15 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,16 @@ pub(crate) async fn inspect(common: &GlobalArgs) -> UserPatchIndex {
3333
}
3434
}
3535

36+
/// The project-patch sources protecting `purl`, if its ecosystem is selected
37+
/// and it has any.
38+
fn protecting(common: &GlobalArgs, index: &UserPatchIndex, purl: &str) -> Option<Vec<String>> {
39+
if !common.purl_ecosystem_selected(purl) {
40+
return None;
41+
}
42+
let sources = index.conflicts(purl);
43+
(!sources.is_empty()).then_some(sources)
44+
}
45+
3646
pub(crate) fn detail(purl: &str, sources: &[String], allowed: bool) -> String {
3747
let sources = sources.join(", ");
3848
if allowed {
@@ -76,13 +86,9 @@ pub(crate) async fn partition(
7686
let mut conflicts = Vec::new();
7787
for purl in &purls {
7888
let purl = purl.as_str();
79-
if !common.purl_ecosystem_selected(purl) {
89+
let Some(sources) = protecting(common, &index, purl) else {
8090
continue;
81-
}
82-
let sources = index.conflicts(purl);
83-
if sources.is_empty() {
84-
continue;
85-
}
91+
};
8692
let message = detail(purl, &sources, common.allow_patch_conflicts);
8793
if common.allow_patch_conflicts {
8894
warnings.push((ALLOWED.into(), message));
@@ -106,11 +112,7 @@ pub(crate) async fn evidence(
106112
}
107113
let index = inspect(common).await;
108114
for &purl in purls {
109-
if !common.purl_ecosystem_selected(purl) {
110-
continue;
111-
}
112-
let sources = index.conflicts(purl);
113-
if !sources.is_empty() {
115+
if let Some(sources) = protecting(common, &index, purl) {
114116
found.insert(purl.to_string(), sources);
115117
}
116118
}
@@ -149,13 +151,9 @@ pub(crate) async fn skip_protected(
149151
}
150152
let index = inspect(common).await;
151153
patches.retain(|purl, record| {
152-
if !common.purl_ecosystem_selected(purl) {
154+
let Some(sources) = protecting(common, &index, purl) else {
153155
return true;
154-
}
155-
let sources = index.conflicts(purl);
156-
if sources.is_empty() {
157-
return true;
158-
}
156+
};
159157
let message = detail(purl, &sources, common.allow_patch_conflicts);
160158
if common.allow_patch_conflicts {
161159
warnings.push((ALLOWED.into(), message));

‎crates/socket-patch-cli/src/commands/vendor_orphans.rs‎

Lines changed: 22 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
//! The orphan sweep `vendor --revert`, `scan --prune`, `rollback` and
22
//! `remove` share: uuid dirs under `.socket/vendor/<eco>/` that no ledger
33
//! entry owns (a hand-edited state file, an interrupted run, the losing
4-
//! side of a git merge of two vendor runs, #1610).
4+
//! side of a git merge of two vendor runs).
55
66
use std::collections::HashSet;
77
use std::path::Path;
@@ -24,10 +24,9 @@ pub(crate) struct OrphanSweep {
2424
pub(crate) failed: Vec<(vendor::path::SweptVendorDir, String)>,
2525
}
2626

27-
/// Uuid dirs under `.socket/vendor/<eco>/` with no owning `(eco, uuid)`
28-
/// ledger entry (a hand-edited state file, or artifacts left by an
29-
/// interrupted run). Unparseable dirs are never returned (and never
30-
/// deleted). Returns the orphans so callers can emit events / counts.
27+
/// Sweep the uuid dirs under `.socket/vendor/<eco>/` with no owning
28+
/// `(eco, uuid)` ledger entry. Unparseable dirs are never returned (and
29+
/// never deleted).
3130
///
3231
/// A missing ledger entry does NOT prove missing wiring: lockfiles can
3332
/// still point into `.socket/vendor/` after a deleted state.json or a
@@ -44,7 +43,7 @@ pub(crate) async fn sweep_orphan_vendor_dirs(
4443
}
4544

4645
/// [`sweep_orphan_vendor_dirs`] limited to the orphans `select` accepts:
47-
/// `rollback` and `remove` sweep only the ones in their scope (#1610).
46+
/// `rollback` and `remove` sweep only the ones in their scope.
4847
pub(crate) async fn sweep_orphan_vendor_dirs_where(
4948
cwd: &Path,
5049
state: &VendorState,
@@ -69,20 +68,19 @@ pub(crate) async fn sweep_orphan_vendor_dirs_where(
6968
if candidates.is_empty() {
7069
return out;
7170
}
72-
let wired: HashSet<(String, String)> =
73-
crate::commands::vendored_backend::repair::scan_vendor_references(cwd)
74-
.await
75-
.into_iter()
76-
.map(|(eco, uuid, _path)| (eco, uuid))
77-
.collect();
71+
let references = crate::commands::vendored_backend::repair::scan_vendor_references(cwd).await;
72+
let wired: HashSet<(&str, &str)> = references
73+
.iter()
74+
.map(|(eco, uuid, _path)| (eco.as_str(), uuid.as_str()))
75+
.collect();
7876
// Each removal also prunes the `<eco>/` and `vendor/` levels it
7977
// emptied (never `.socket/` — the lock guard owns that level): the
8078
// sweep runs AFTER the per-entry reverts and their ledger saves, so it
8179
// is the last thing that can leave a fully reverted project with empty
8280
// `.socket/vendor/<eco>/` husks.
8381
let stop_dir = cwd.join(SOCKET_DIR);
8482
for unit in candidates {
85-
if wired.contains(&(unit.eco.clone(), unit.uuid.clone())) {
83+
if wired.contains(&(unit.eco.as_str(), unit.uuid.as_str())) {
8684
out.still_wired.push(unit);
8785
continue;
8886
}
@@ -183,7 +181,7 @@ mod tests {
183181
/// `rollback <target>` / `remove <target>` scope their orphan sweep
184182
/// with [`orphan_in_target`]: a package target matches by the leaves'
185183
/// purls, a uuid target by the dir's own uuid, even when no leaf
186-
/// parses (#1610).
184+
/// parses.
187185
#[test]
188186
fn orphan_in_target_matches_package_and_uuid_targets() {
189187
let uuid = "aaaaaaaa-1111-4111-8111-aaaaaaaaaaaa";
@@ -216,8 +214,7 @@ mod tests {
216214

217215
/// A kept, still-wired orphan points the user at a remedy that works
218216
/// in v5: `repair` no longer rebuilds ledger entries from lockfiles,
219-
/// so the advisory names the ledger restore (and the merge case,
220-
/// #991) or the lockfile restore instead.
217+
/// so the advisory names the ledger restore (and the merge case).
221218
#[test]
222219
fn orphan_still_wired_names_a_working_remedy() {
223220
let unit = vendor::path::SweptVendorDir {
@@ -228,15 +225,15 @@ mod tests {
228225
};
229226
let warning = orphan_still_wired(&unit);
230227
assert_eq!(warning.code, "vendor_orphan_still_wired");
231-
let detail = &warning.detail;
232-
assert!(
233-
detail.contains(".socket/vendor/npm/aaaaaaaa-1111-4111-8111-aaaaaaaaaaaa"),
234-
"{detail}"
228+
// The advisory follows a revert that rewrote the lockfile, so it
229+
// names no lockfile checkout: that would re-wire what it removed.
230+
assert_eq!(
231+
warning.detail,
232+
"a project lockfile still points at \
233+
.socket/vendor/npm/aaaaaaaa-1111-4111-8111-aaaaaaaaaaaa, which no ledger \
234+
entry owns; the artifacts were kept (restore the missing entries in \
235+
.socket/vendor/state.json from version control (if a git merge kept one \
236+
side of state.json, merge both sides' `entries`), then revert again)"
235237
);
236-
assert!(!detail.contains("socket-patch repair"), "{detail}");
237-
assert!(detail.contains("entries"), "{detail}");
238-
// The advisory follows a revert that rewrote the lockfile:
239-
// checking the lockfile out again would re-wire what it removed.
240-
assert!(!detail.contains("git checkout"), "{detail}");
241238
}
242239
}

‎crates/socket-patch-cli/src/commands/vlt_heal.rs‎

Lines changed: 7 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -29,11 +29,11 @@ pub(crate) struct RewriteHeal {
2929
/// Set when the heal ran (it may invalidate store entries): the store's
3030
/// bundled copies after it, discovery's only read of the installed tree
3131
/// (`Discovery::vlt_bundled_copies`). `None` when nothing was healed.
32-
pub(crate) healed_store: Option<std::collections::BTreeMap<String, String>>,
32+
pub(crate) healed_store: Option<BTreeMap<String, String>>,
3333
}
3434

3535
pub(crate) const REINSTALL_REQUIRED: &str = "redirect_vlt_reinstall_required";
36-
/// A bundled copy of a redirected package that no rewire reaches (#471).
36+
/// A bundled copy of a redirected package that no rewire reaches.
3737
pub(crate) const BUNDLED_INSTANCE_SKIPPED: &str = "redirect_vlt_bundled_instance_skipped";
3838

3939
/// The uuids of `deps` whose purl a vlt vendored ledger entry claims: a
@@ -85,7 +85,7 @@ pub(crate) async fn artifact_preflight(
8585
/// The origins the vlt heal treats as Socket-owned in the final lock: the
8686
/// patch server plus `--api-url`. Unlike discovery's
8787
/// [`crate::commands::hosted_unwind::patch_server_origins`], it also counts the
88-
/// `--api-url` origin (as it has since vlt support landed, #269), so a
88+
/// `--api-url` origin, so a
8989
/// setup serving the hosted tarballs from the API origin is healed too.
9090
/// Discovery-facing code must use the `hosted_unwind` helper, not this one.
9191
fn vlt_heal_origins(common: &crate::args::GlobalArgs) -> Vec<String> {
@@ -98,10 +98,6 @@ fn vlt_heal_origins(common: &crate::args::GlobalArgs) -> Vec<String> {
9898
.collect()
9999
}
100100

101-
fn cleanup_disabled(common: &crate::args::GlobalArgs) -> bool {
102-
common.dry_run || common.no_vlt_install_cleanup
103-
}
104-
105101
/// How a heal ended, for the advisory wording.
106102
#[derive(Debug, Default, PartialEq, Eq)]
107103
struct HealTally {
@@ -146,7 +142,7 @@ async fn heal_targets(
146142
if stale.is_empty() {
147143
return tally;
148144
}
149-
if cleanup_disabled(common) {
145+
if common.dry_run || common.no_vlt_install_cleanup {
150146
tally.stale_left = stale.len();
151147
tally
152148
.stale_uuids
@@ -392,7 +388,7 @@ pub(crate) async fn heal_after_rewrite(
392388
out.stale_purls.insert(purl.clone());
393389
}
394390
}
395-
// A bundled copy of a patched package (#471) has no lock node, so the
391+
// A bundled copy of a patched package has no lock node, so the
396392
// redirect never reaches it: say so, and keep its purl out of the
397393
// in-run attestation (lockfile discovery contests it the same way).
398394
if inputs.final_lock.is_some() {
@@ -423,12 +419,8 @@ pub(crate) async fn heal_after_rewrite(
423419
out.stale_purls.insert(purl.clone());
424420
}
425421
}
426-
// A heal implies a final lock, so the block above recorded the store;
427-
// never report a heal without it.
428-
if healed && out.healed_store.is_none() {
429-
out.healed_store =
430-
Some(socket_patch_core::vendor::vlt_bundled::bundled_copies(&common.cwd).await);
431-
}
422+
// Heal targets come from the final lock, so the block above saw it.
423+
debug_assert!(!healed || out.healed_store.is_some());
432424
out
433425
}
434426

‎crates/socket-patch-core/src/update/channel.rs‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -518,6 +518,21 @@ fn is_prefix_global_install(path: &Path) -> bool {
518518
}
519519
}
520520

521+
impl InstallChannel {
522+
/// A stable machine name for telemetry, unlike the human-facing
523+
/// [`channel_label`] (`cargo install`, `the RubyGems launcher`).
524+
pub fn id(self) -> &'static str {
525+
match self {
526+
InstallChannel::Standalone => "standalone",
527+
InstallChannel::Npm => "npm",
528+
InstallChannel::Pypi => "pypi",
529+
InstallChannel::Cargo => "cargo",
530+
InstallChannel::LauncherCache => "launcher_cache",
531+
InstallChannel::Homebrew => "homebrew",
532+
}
533+
}
534+
}
535+
521536
/// Short human label for refusal messages ("managed by npm"); see
522537
/// [`channel_label_for`] for the npm-family manager of a given path.
523538
pub fn channel_label(channel: InstallChannel) -> &'static str {
@@ -853,6 +868,20 @@ mod tests {
853868
);
854869
}
855870

871+
#[test]
872+
fn channel_ids_are_the_telemetry_names() {
873+
for (channel, id) in [
874+
(InstallChannel::Standalone, "standalone"),
875+
(InstallChannel::Npm, "npm"),
876+
(InstallChannel::Pypi, "pypi"),
877+
(InstallChannel::Cargo, "cargo"),
878+
(InstallChannel::LauncherCache, "launcher_cache"),
879+
(InstallChannel::Homebrew, "homebrew"),
880+
] {
881+
assert_eq!(channel.id(), id);
882+
}
883+
}
884+
856885
#[test]
857886
fn channel_label_covers_every_channel() {
858887
// Exact strings: these feed the "--update refused: managed by <label>"

0 commit comments

Comments
 (0)