From 4c47274eb8165be9683cfb1dff8c907b252b5d61 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 30 Oct 2026 02:57:24 +0000 Subject: [PATCH 1/4] Start sweep of remove.rs and repair.rs Assisted-by: Claude Code:claude-opus-5-5 From 6281ec4363e809f91d35d2dea1ddedc091802127 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 30 Oct 2026 03:13:11 +0000 Subject: [PATCH 2/4] Sweep repair.rs: drop the noun parameter The human formatters took an ArtifactNoun that production always passed as BLOB; they now use BLOB directly. Envelopes print through ui::print_json. Trims history comments and bare issue numbers. Tests: drops the tautological HOSTED_ONLY_REASON assert, the two multibyte runs that format_id_list's own test already covers, and merges the two offline no-download tests into one that covers dry run and wet run. Assisted-by: Claude Code:claude-opus-5-5 --- .../socket-patch-cli/src/commands/repair.rs | 323 +++++------------- 1 file changed, 81 insertions(+), 242 deletions(-) diff --git a/crates/socket-patch-cli/src/commands/repair.rs b/crates/socket-patch-cli/src/commands/repair.rs index 929987f43..650710f12 100644 --- a/crates/socket-patch-cli/src/commands/repair.rs +++ b/crates/socket-patch-cli/src/commands/repair.rs @@ -29,11 +29,7 @@ pub struct RepairArgs { /// Only download missing artifacts; skip the cleanup phase. /// Incompatible with `--offline`. // - // `value_parser = parse_bool_flag` matches the `GlobalArgs` bool flags: - // clap's default bool parser accepts only the literal strings - // `true`/`false` from the env binding, so `SOCKET_DOWNLOAD_ONLY=1` would - // abort every `repair` invocation. (`main`'s empty-var scrub covers an - // exported-but-empty value via `LOCAL_ARG_ENV_VARS`.) + // clap's default bool parser rejects `SOCKET_DOWNLOAD_ONLY=1`. #[arg( long = "download-only", env = "SOCKET_DOWNLOAD_ONLY", @@ -46,8 +42,6 @@ pub struct RepairArgs { pub async fn run(args: RepairArgs) -> i32 { apply_env_toggles(&args.common); - // --offline implies strict airgap: no network calls. `--download-only` - // is the inverse (network-only). The two are now mutually exclusive. if args.common.offline && args.download_only { return crate::json_envelope::usage_error( Command::Repair, @@ -66,13 +60,11 @@ pub async fn run(args: RepairArgs) -> i32 { let mut vendor_references: Option> = None; // Only a missing file is "no manifest"; one repair cannot even stat is - // unreadable, not a project with nothing to repair (#998). + // unreadable, not a project with nothing to repair. let manifest_present = match manifest_exists(&manifest_path).await { Ok(present) => present, Err(e) => { let err = crate::json_envelope::manifest_load_error(&manifest_path, &e); - // Recorded like the load failure in `repair_inner` (no client - // is built before this gate). track_patch_repair_failed(&err.message, &TelemetryAuth::from_credentials(None, None)) .await; if args.common.json { @@ -82,7 +74,7 @@ pub async fn run(args: RepairArgs) -> i32 { &err.code, &err.message, ); - println!("{}", env.to_pretty_json()); + crate::ui::print_json(&env.to_value()); } else { eprintln!("Error: {}", crate::ui::sentence_case(&err.message)); } @@ -99,7 +91,6 @@ pub async fn run(args: RepairArgs) -> i32 { // bare directory would get. Only cheap existence probes (and the // read-only lockfile scans) run before the lock, so a project with // nothing to repair never grows `.socket/`. - // The vendored phase runs at the manifest's project (#745). let state_file = args .common .project_root() @@ -131,10 +122,8 @@ pub async fn run(args: RepairArgs) -> i32 { PatchEvent::artifact(PatchAction::Skipped) .with_reason("redirect_only_project", msg), ); - println!("{}", env.to_pretty_json()); + crate::ui::print_json(&env.to_value()); } else if !args.common.silent { - // A sentence on the terminal; the JSON reason keeps - // its historical, period-less text. println!("{msg}."); } return 0; @@ -147,7 +136,7 @@ pub async fn run(args: RepairArgs) -> i32 { "manifest_not_found", &msg, ); - println!("{}", env.to_pretty_json()); + crate::ui::print_json(&env.to_value()); } else { eprintln!("Error: {msg}"); } @@ -155,13 +144,9 @@ pub async fn run(args: RepairArgs) -> i32 { } } - // Serialize against concurrent socket-patch runs targeting the - // same `.socket/` directory. See `apply_lock`: acquire creates the - // directory when needed (the vendor-only repair of a ledger-less - // project), and the guard's drop removes `apply.lock` — and an - // otherwise-empty `.socket/` — on every exit path, dry-run included. - // A live holder makes repair refuse with `lock_held`; it never steals - // the lock. + // Acquire creates `.socket/` when needed (the vendor-only repair of a + // ledger-less project); the guard's drop removes `apply.lock` and an + // otherwise-empty `.socket/` on every exit path. let socket_dir = crate::args::socket_dir_of(&manifest_path, &args.common.cwd); let _lock = match acquire_or_emit( &socket_dir, @@ -199,16 +184,11 @@ pub async fn run(args: RepairArgs) -> i32 { .as_ref() .and_then(crate::commands::vex_sources::api_auth_fallback_warning); - // Resolve telemetry credentials through the API client the way - // apply/rollback/remove do: passing the raw `--api-token`/`--org` flag - // values meant env-provided SOCKET_API_TOKEN/SOCKET_ORG_SLUG (the - // standard configuration) never reached telemetry, which then fell - // back to the anonymous public-proxy endpoint instead of the - // org-scoped one. Reuse the download phase's client when there was - // one. Otherwise build one only when a token is available (so the - // org auto-resolve still attributes the event, and no proxy advisory - // is printed): without a token telemetry goes to the public endpoint - // whatever the org slug, so `(None, None)` is equivalent. + // Telemetry credentials come from an API client, as in apply/rollback/ + // remove, so env-provided tokens and the org auto-resolve apply. One is + // built only when a token is available: without one telemetry goes to + // the public endpoint whatever the org, and building it would print the + // proxy advisory. if client.is_none() && api_token_available(&args.common) { client = Some( get_api_client_with_overrides(args.common.api_client_overrides()) @@ -223,15 +203,7 @@ pub async fn run(args: RepairArgs) -> i32 { match result { Ok((mut env, counts)) => { - // A token whose org could not be resolved put the download on the - // public proxy (stderr already said so); `warnings[]` is the - // machine channel for it, as in scan / get / apply / vendor. env.warnings.extend(auth_fallback); - // A repair where some artifacts failed to download is marked a - // partial failure inside `repair_inner` (a `Failed` event plus - // `mark_partial_failure`). Mirror `apply`: surface that as a - // non-zero exit and the failure telemetry, so a CI guarding on - // the exit code doesn't treat a half-finished repair as success. let had_failure = matches!(env.status, Status::PartialFailure | Status::Error); if had_failure { track_patch_repair_failed("One or more artifacts failed to download", &telemetry) @@ -246,19 +218,15 @@ pub async fn run(args: RepairArgs) -> i32 { .await; } if args.common.json { - println!("{}", env.to_pretty_json()); - } - if had_failure { - 1 - } else { - 0 + crate::ui::print_json(&env.to_value()); } + i32::from(had_failure) } Err(e) => { track_patch_repair_failed(&e.message, &telemetry).await; if args.common.json { let env = error_envelope(Command::Repair, args.common.dry_run, &e.code, &e.message); - println!("{}", env.to_pretty_json()); + crate::ui::print_json(&env.to_value()); } else { eprintln!("Error: {}", e.message); } @@ -281,13 +249,13 @@ const DRY_RUN_LIST_CAP: usize = 10; /// ` - ` lines for `ids`, sorted (they come from a `HashSet`), at /// most `cap` of them, then ` ... and N more`. -fn format_id_list(ids: &[String], noun: ArtifactNoun, cap: usize) -> Vec { +fn format_id_list(ids: &[String], cap: usize) -> Vec { let mut sorted: Vec<&String> = ids.iter().collect(); sorted.sort(); let mut lines: Vec = sorted .iter() .take(cap) - .map(|id| format!(" - {}", noun.display_id(id))) + .map(|id| format!(" - {}", BLOB.display_id(id))) .collect(); if sorted.len() > cap { lines.push(format!(" ... and {} more", sorted.len() - cap)); @@ -296,8 +264,8 @@ fn format_id_list(ids: &[String], noun: ArtifactNoun, cap: usize) -> Vec } /// `Found 2 missing blobs` / `Found 1 missing blob`. -fn format_found_missing(n: usize, noun: ArtifactNoun) -> String { - format!("Found {}", noun.count(n).replacen(' ', " missing ", 1)) +fn format_found_missing(n: usize) -> String { + format!("Found {}", BLOB.count(n).replacen(' ', " missing ", 1)) } /// Why a hosted-only project has nothing to repair (the JSON skip @@ -311,25 +279,24 @@ const HOSTED_ONLY_REASON: &str = "Hosted patches need no local repair; re-run \ /// one), or that everything is on disk. fn format_nothing_missing( manifest: Option<&socket_patch_core::manifest::schema::PatchManifest>, - noun: ArtifactNoun, ) -> String { match manifest { None => "No manifest; no patch artifacts to download.".to_string(), Some(m) if m.patches.is_empty() => { "No patches in manifest; nothing to download.".to_string() } - Some(_) => format!("All {} are present locally.", noun.many), + Some(_) => format!("All {} are present locally.", BLOB.many), } } /// The `--offline` warning (stderr) for artifacts that cannot be fetched. -fn format_offline_warning(ids: &[String], noun: ArtifactNoun) -> String { +fn format_offline_warning(ids: &[String]) -> String { let verb = if ids.len() == 1 { "is" } else { "are" }; let mut lines = vec![format!( "Warning: {} {verb} missing (offline mode - not downloading):", - noun.count(ids.len()) + BLOB.count(ids.len()) )]; - lines.extend(format_id_list(ids, noun, OFFLINE_LIST_CAP)); + lines.extend(format_id_list(ids, OFFLINE_LIST_CAP)); lines.join("\n") } @@ -359,17 +326,12 @@ fn format_cleanup_summary(results: &[(ArtifactNoun, CleanupResult)], dry_run: bo /// The closing line of a human repair run. `other_failure` is a failure /// recorded elsewhere in the run (a vendored artifact that could not be /// rebuilt): the run exits 1, so it must not close on "Repair complete.". -fn format_final_line( - download_failed: usize, - other_failure: bool, - noun: ArtifactNoun, - dry_run: bool, -) -> String { +fn format_final_line(download_failed: usize, other_failure: bool, dry_run: bool) -> String { if download_failed > 0 { let verb = if download_failed == 1 { "was" } else { "were" }; format!( "Repair finished with errors: {} {verb} not downloaded.", - noun.count(download_failed) + BLOB.count(download_failed) ) } else if other_failure { "Repair finished with errors.".to_string() @@ -400,32 +362,31 @@ async fn download_pass( blobs_path: &Path, ) -> DownloadPass { let quiet = args.common.json || args.common.silent; - let noun = BLOB; let mut pass = DownloadPass { missing: missing.len(), ..DownloadPass::default() }; if args.common.offline { if !quiet { - eprintln!("{}", format_offline_warning(missing, noun)); + eprintln!("{}", format_offline_warning(missing)); } return pass; } if !quiet { - println!("{}", format_found_missing(missing.len(), noun)); + println!("{}", format_found_missing(missing.len())); } if args.common.dry_run { if !quiet { println!(); println!("Would download:"); - for line in format_id_list(missing, noun, DRY_RUN_LIST_CAP) { + for line in format_id_list(missing, DRY_RUN_LIST_CAP) { println!("{line}"); } } return pass; } let mut status = crate::ui::StatusLine::stderr(args.common.json, args.common.silent); - status.set(format!("Downloading {}...", noun.count(missing.len()))); + status.set(format!("Downloading {}...", BLOB.count(missing.len()))); if client.is_none() { *client = Some( get_api_client_with_overrides(args.common.api_client_overrides()) @@ -439,14 +400,14 @@ async fn download_pass( pass.downloaded = fetch_result.downloaded; pass.failed = fetch_result.failed; if !quiet { - for line in format_fetch_successes(&fetch_result, noun) { + for line in format_fetch_successes(&fetch_result, BLOB) { println!("{line}"); } } // Failures are error output: stderr, and not muted by `--silent` // (`--json` runs carry them in the envelope). if !args.common.json { - for (i, line) in format_fetch_failures(&fetch_result, noun) + for (i, line) in format_fetch_failures(&fetch_result, BLOB) .iter() .enumerate() { @@ -488,8 +449,7 @@ async fn repair_inner( // `(eco, uuid, rel)` lockfile vendor references, scanned once by `run`. vendor_references: Vec<(String, String, String)>, ) -> Result<(Envelope, RepairCounts), EnvelopeError> { - // `Ok(None)` = no manifest (vendor-only repair); present-but-invalid - // stays a hard error, under the shared manifest-load code (#931). + // `Ok(None)` = no manifest (vendor-only repair). let manifest = read_manifest(manifest_path) .await .map_err(|e| crate::json_envelope::manifest_load_error(manifest_path, &e))?; @@ -497,12 +457,6 @@ async fn repair_inner( let socket_dir = crate::args::socket_dir_of(manifest_path, &args.common.cwd); let blobs_path = socket_dir.join("blobs"); - // `--silent` ("suppress non-error output") must mute the human-readable - // progress just like `--json` does — otherwise a silent repair still - // floods stdout with "Found N missing", "Downloading…", cleanup - // summaries and "Repair complete.". Gate every informational print on - // both, mirroring `get`/`apply`. (The JSON envelope is emitted by the - // caller, so nothing here depends on `json` alone.) let quiet = args.common.json || args.common.silent; let mut downloaded_count = 0usize; @@ -562,7 +516,6 @@ async fn repair_inner( .into_iter() .collect(), }; - let noun = BLOB; // Whether stdout already carries a line, so the blank separators // between sections never open the output (the offline warning goes // to stderr). @@ -573,7 +526,7 @@ async fn repair_inner( } _ => { if !quiet { - println!("{}", format_nothing_missing(manifest.as_ref(), noun)); + println!("{}", format_nothing_missing(manifest.as_ref())); } DownloadPass::default() } @@ -675,12 +628,7 @@ async fn repair_inner( // so a piped stdout never ends in a stray blank line when the // line itself goes to stderr. let other_failure = matches!(env.status, Status::PartialFailure | Status::Error); - let line = format_final_line( - download_failed_count, - other_failure, - noun, - args.common.dry_run, - ); + let line = format_final_line(download_failed_count, other_failure, args.common.dry_run); if download_failed_count > 0 || other_failure { if stdout_started { eprintln!(); @@ -711,8 +659,7 @@ async fn repair_inner( env.record( PatchEvent::artifact(action).with_details(serde_json::json!({ "count": count, - // Constant since v5 (blobs are the only download); kept so - // the event's shape is unchanged. + // Always "file"; kept for the event's shape. "downloadMode": "file", })), ); @@ -720,7 +667,7 @@ async fn repair_inner( if download_failed_count > 0 { env.record(PatchEvent::artifact(PatchAction::Failed).with_error( "download_failed", - format!("{} failed to download", noun.count(download_failed_count)), + format!("{} failed to download", BLOB.count(download_failed_count)), )); env.mark_partial_failure(); } @@ -773,25 +720,20 @@ mod tests { fn nothing_missing_line_says_why() { use socket_patch_core::manifest::schema::PatchManifest; assert_eq!( - format_nothing_missing(None, DIFF_ARCHIVE), + format_nothing_missing(None), "No manifest; no patch artifacts to download." ); let empty = PatchManifest::new(); assert_eq!( - format_nothing_missing(Some(&empty), DIFF_ARCHIVE), + format_nothing_missing(Some(&empty)), "No patches in manifest; nothing to download." ); let one: PatchManifest = serde_json::from_str(MANIFEST_JSON).expect("fixture manifest parses"); assert!(!one.patches.is_empty()); assert_eq!( - format_nothing_missing(Some(&one), DIFF_ARCHIVE), - "All diff archives are present locally." - ); - assert_eq!( - HOSTED_ONLY_REASON, - "Hosted patches need no local repair; re-run `scan --mode hosted` to refresh \ - the lockfile (it also re-checks for stale pre-hosted installs)" + format_nothing_missing(Some(&one)), + "All blobs are present locally." ); } @@ -865,30 +807,30 @@ mod tests { }) } - /// Regression for the offline + dry-run leak: with `--offline` set, the - /// download phase is skipped entirely, so even in dry-run mode a missing - /// artifact must NOT produce a "would-download" (verified) event, - /// matching the human-readable path (which only warns). + /// `--offline` skips the download phase: a missing artifact is a + /// warning, never a download or would-download event, dry run or not. #[tokio::test] - async fn offline_dry_run_does_not_record_download_event() { - let tmp = tempfile::tempdir().unwrap(); - let socket = make_socket(tmp.path()); - // No blob on disk → the manifest's afterHash is "missing". - let mut args = offline_args(tmp.path()); - args.common.dry_run = true; - - let (env, counts) = - repair_inner(&args, &socket.join("manifest.json"), &mut None, Vec::new()) - .await - .expect("repair_inner"); - - assert!( - !has_download_event(&env), - "offline dry-run must not emit a download/would-download event; events={:?}", - env.events - ); - assert_eq!(counts.downloaded, 0); - assert_eq!(env.status, Status::Success); + async fn offline_missing_artifact_records_no_download_event() { + for dry_run in [false, true] { + let tmp = tempfile::tempdir().unwrap(); + let socket = make_socket(tmp.path()); + // No blob on disk → the manifest's afterHash is "missing". + let mut args = offline_args(tmp.path()); + args.common.dry_run = dry_run; + + let (env, counts) = + repair_inner(&args, &socket.join("manifest.json"), &mut None, Vec::new()) + .await + .expect("repair_inner"); + + assert!( + !has_download_event(&env), + "dry_run={dry_run}: events={:?}", + env.events + ); + assert_eq!(counts.downloaded, 0); + assert_eq!(env.status, Status::Success); + } } /// The online dry-run path *should* still preview the download — this @@ -1089,148 +1031,45 @@ mod tests { .exists()); } - /// A manifest "hash" that is NOT a hex digest: manifest hashes are - /// unvalidated strings (serde only), and byte index 12 of this one lands - /// inside a multibyte char — so a byte slice `&id[..12]` panics on it. - /// 1 ASCII byte + 8×2-byte `é` = 17 bytes; boundaries at 11 and 13. - const MULTIBYTE_HASH: &str = "aéééééééé"; - - /// Write a `.socket/manifest.json` whose afterHash is `MULTIBYTE_HASH`. - fn make_socket_multibyte(root: &Path) -> PathBuf { - let socket = root.join(".socket"); - std::fs::create_dir_all(&socket).unwrap(); - std::fs::write( - socket.join("manifest.json"), - MANIFEST_JSON.replace(REFERENCED_HASH, MULTIBYTE_HASH), - ) - .unwrap(); - socket - } - - /// Regression: the human-readable offline warning truncates each missing - /// artifact id for display. Truncation must be by characters, not bytes — - /// `&id[..12]` panics when byte 12 falls inside a multibyte char, so a - /// corrupt or hand-edited manifest crashed `repair --offline` instead of - /// warning. (Same class as the `format_fetch_result` fix in blob_fetcher.) - #[tokio::test] - async fn offline_warning_survives_multibyte_manifest_hash() { - let tmp = tempfile::tempdir().unwrap(); - let socket = make_socket_multibyte(tmp.path()); - // The truncating print only runs on the human-readable path. - let mut args = offline_args(tmp.path()); - args.common.json = false; - - let (env, counts) = - repair_inner(&args, &socket.join("manifest.json"), &mut None, Vec::new()) - .await - .expect("repair_inner"); - - assert_eq!(counts.downloaded, 0); - assert_eq!(env.status, Status::Success); - } - - /// Regression twin for the dry-run preview print, which truncated ids the - /// same byte-sliced way (its list caps at 10 instead of 5). - #[tokio::test] - async fn dry_run_preview_survives_multibyte_manifest_hash() { - let tmp = tempfile::tempdir().unwrap(); - let socket = make_socket_multibyte(tmp.path()); - let mut args = offline_args(tmp.path()); - args.common.offline = false; - args.common.dry_run = true; - args.common.json = false; - - let (env, _counts) = - repair_inner(&args, &socket.join("manifest.json"), &mut None, Vec::new()) - .await - .expect("repair_inner"); - - // The preview event is still recorded once the print survives. - assert!( - has_download_event(&env), - "dry-run must still preview the download; events={:?}", - env.events - ); - } - - /// Offline mode with a missing artifact: the run must succeed (a warning, - /// not a failure), record NO download event, and report zero downloads — - /// nothing is fetched and the airgap is honoured. Cleanup still runs. - #[tokio::test] - async fn offline_missing_artifact_warns_without_failure() { - let tmp = tempfile::tempdir().unwrap(); - let socket = make_socket(tmp.path()); - // No blob on disk → manifest afterHash is "missing". Not dry-run. - let args = offline_args(tmp.path()); - - let (env, counts) = - repair_inner(&args, &socket.join("manifest.json"), &mut None, Vec::new()) - .await - .expect("repair_inner"); - - assert!( - !has_download_event(&env), - "offline mode must not record a download event; events={:?}", - env.events - ); - assert_eq!(counts.downloaded, 0); - assert_eq!( - env.status, - Status::Success, - "missing artifacts in offline mode are a warning, not a failure" - ); - } - fn ids(list: &[&str]) -> Vec { list.iter().map(|s| s.to_string()).collect() } #[test] fn id_list_is_sorted_capped_and_honest_about_truncation() { - let uuids = ids(&[ - "22222222-2222-4222-8222-222222222222", - "11111111-1111-4111-8111-111111111111", - ]); - // UUID-keyed archive ids print in full, in sorted order. - assert_eq!( - format_id_list(&uuids, DIFF_ARCHIVE, 5), - vec![ - " - 11111111-1111-4111-8111-111111111111", - " - 22222222-2222-4222-8222-222222222222", - ] - ); - // File-mode hashes: 64-hex cut to 12 + "...", a short one kept whole. + // Sorted; a 64-hex hash cut to 12 + "...", a short one kept whole. let hashes = ids(&[&"b".repeat(64), "22"]); assert_eq!( - format_id_list(&hashes, BLOB, 5), + format_id_list(&hashes, 5), vec![" - 22", " - bbbbbbbbbbbb..."] ); let many: Vec = (0..7).map(|i| format!("id{i}")).collect(); - let lines = format_id_list(&many, BLOB, 5); + let lines = format_id_list(&many, 5); assert_eq!(lines.len(), 6); assert_eq!(lines[5], " ... and 2 more"); - assert!(format_id_list(&[], BLOB, 5).is_empty()); - // Multibyte ids never panic and are counted in chars. + assert!(format_id_list(&[], 5).is_empty()); + // Byte 12 of this unvalidated manifest "hash" is inside a char: + // it never panics and is counted in chars. assert_eq!( - format_id_list(&ids(&[MULTIBYTE_HASH]), BLOB, 5), + format_id_list(&ids(&["aéééééééé"]), 5), vec![" - aéééééééé"] ); } #[test] fn found_missing_line() { - assert_eq!(format_found_missing(1, BLOB), "Found 1 missing blob"); - assert_eq!(format_found_missing(12, BLOB), "Found 12 missing blobs"); + assert_eq!(format_found_missing(1), "Found 1 missing blob"); + assert_eq!(format_found_missing(12), "Found 12 missing blobs"); } #[test] fn offline_warning_singular_and_plural() { assert_eq!( - format_offline_warning(&ids(&["a"]), BLOB), + format_offline_warning(&ids(&["a"])), "Warning: 1 blob is missing (offline mode - not downloading):\n - a" ); assert_eq!( - format_offline_warning(&ids(&["b", "a"]), BLOB), + format_offline_warning(&ids(&["b", "a"])), "Warning: 2 blobs are missing (offline mode - not downloading):\n - a\n - b" ); } @@ -1300,26 +1139,26 @@ mod tests { #[test] fn final_line_reflects_failures_and_dry_run() { - assert_eq!(format_final_line(0, false, BLOB, false), "Repair complete."); + assert_eq!(format_final_line(0, false, false), "Repair complete."); assert_eq!( - format_final_line(0, false, BLOB, true), + format_final_line(0, false, true), "Dry run: no changes made." ); assert_eq!( - format_final_line(1, false, BLOB, false), + format_final_line(1, false, false), "Repair finished with errors: 1 blob was not downloaded." ); assert_eq!( - format_final_line(2, true, BLOB, false), + format_final_line(2, true, false), "Repair finished with errors: 2 blobs were not downloaded." ); // A failed vendored rebuild (exit 1) never closes on "complete". assert_eq!( - format_final_line(0, true, BLOB, false), + format_final_line(0, true, false), "Repair finished with errors." ); assert_eq!( - format_final_line(0, true, BLOB, true), + format_final_line(0, true, true), "Repair finished with errors." ); } From 9d0b1f043cb4d69edbb700d2630bc85413a2d75d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 30 Oct 2026 03:13:11 +0000 Subject: [PATCH 3/4] Sweep remove.rs: inline wrappers, dedupe tails Inlines emit_not_found and hosted_unwind_error, and replaces the HostedUnwindError newtype with a String. The hosted_reverted event and the kept-entries exit tail are each built by one helper instead of twice. print_hosted_leg_warnings becomes print_run_warnings, since it also prints the rollback's warnings. Envelopes print through ui::print_json and warnings use RunWarning::new. Output is unchanged. Tests: deletes dry_run_footer_matches_repair, which compared a constant to its own literal, and asserts the not-installed warning's exact lines instead of fragments. Assisted-by: Claude Code:claude-opus-5-5 --- .../socket-patch-cli/src/commands/remove.rs | 282 +++++++----------- 1 file changed, 115 insertions(+), 167 deletions(-) diff --git a/crates/socket-patch-cli/src/commands/remove.rs b/crates/socket-patch-cli/src/commands/remove.rs index ad3e4442f..561bd5c8d 100644 --- a/crates/socket-patch-cli/src/commands/remove.rs +++ b/crates/socket-patch-cli/src/commands/remove.rs @@ -33,8 +33,8 @@ use crate::ui::{plural, sweep_failure}; /// Vendor-ledger entries matching a remove target /// ([`socket_patch_core::ledgers::Ledgers::matching`]), sorted by key for /// deterministic event order. With the manifest, an entry the matched -/// manifest keys claim matches whatever patch generation it recorded -/// (#999); the ledger-only path passes `None`. +/// manifest keys claim matches whatever patch generation it recorded; +/// the ledger-only path passes `None`. fn vendor_entries_matching( state: &VendorState, manifest: Option<&PatchManifest>, @@ -72,29 +72,10 @@ fn remove_matching( removed } -/// Emit the `not_found` envelope (or stderr line) for an identifier that -/// matched nothing in any store, tracking the failure. `dry_run` rides the -/// envelope so a preview's failures still report `dryRun: true` (matching -/// apply's error envelopes and remove's own success envelope). -async fn emit_not_found(json: bool, dry_run: bool, identifier: &str, telemetry: &TelemetryAuth) { - let msg = format!("No patch found matching identifier: {identifier}"); - track_patch_remove_failed(&msg, telemetry).await; - if json { - let mut env = Envelope::new(Command::Remove); - env.dry_run = dry_run; - env.status = Status::NotFound; - env.error = Some(EnvelopeError::new("not_found", msg)); - println!("{}", env.to_pretty_json()); - } else { - eprintln!("Error: {msg}"); - } -} - -/// Print the hosted leg's run-level advisories (`Warning: …`) on -/// stderr — never under `--silent` / `--json` (JSON carries them in the -/// envelope's `warnings[]`). Printed as soon as the leg returns, so a -/// human run that then fails still says what it did to the files. -fn print_hosted_leg_warnings(common: &GlobalArgs, warnings: &[(String, String)]) { +/// Print run-level advisories (`Warning: …`) on stderr — never under +/// `--silent` / `--json` (JSON carries them in the envelope's +/// `warnings[]`). +fn print_run_warnings(common: &GlobalArgs, warnings: &[(String, String)]) { if common.silent || common.json { return; } @@ -124,7 +105,7 @@ fn emit_error_envelope(json: bool, dry_run: bool, code: &str, message: String) { let mut env = Envelope::new(Command::Remove); env.dry_run = dry_run; env.mark_error(EnvelopeError::new(code, message)); - println!("{}", env.to_pretty_json()); + crate::ui::print_json(&env.to_value()); } else { eprintln!("Error: {}", crate::ui::sentence_case(&message)); } @@ -288,11 +269,7 @@ pub struct RemoveArgs { #[command(flatten)] pub common: GlobalArgs, - // `value_parser = parse_bool_flag` matches the `GlobalArgs` bool flags: - // clap's default bool parser accepts only the literal strings - // `true`/`false` from the env binding, so `SOCKET_SKIP_ROLLBACK=1` (or - // an exported-but-empty `SOCKET_SKIP_ROLLBACK=`) aborted every - // `remove` invocation. + // clap's default bool parser rejects `SOCKET_SKIP_ROLLBACK=1`. /// Skip rolling back files before removing (only update the manifest). #[arg( long = "skip-rollback", @@ -318,10 +295,6 @@ pub struct RemoveArgs { pub preserve_state: bool, } -/// The last line of a human `remove --dry-run` (the wording `repair -/// --dry-run` ends with too). -const DRY_RUN_FOOTER: &str = "Dry run: no changes made."; - pub async fn run(args: RemoveArgs) -> i32 { apply_env_toggles(&args.common); // The shared target grammar: a UUID, a purl (versioned or not) or an @@ -366,7 +339,7 @@ pub async fn run(args: RemoveArgs) -> i32 { // only. let project_state = crate::commands::project_state_in_scope(&args.common); // Only a missing file is "no manifest"; one remove cannot even stat is - // unreadable, not a bare project (#998). + // unreadable, not a bare project. let manifest_missing = match manifest_exists(&manifest_path).await { Ok(present) => !present, Err(e) => { @@ -385,7 +358,7 @@ pub async fn run(args: RemoveArgs) -> i32 { } else { Default::default() }; - // Leftover `resolutions` selectors (#1203) unwind like pins: the + // Leftover `resolutions` selectors unwind like pins: the // restore retires them from the manifest. let hosted_pins: Vec = hosted_inventory.unwindable(); if manifest_missing { @@ -469,7 +442,6 @@ pub async fn run(args: RemoveArgs) -> i32 { // the vendored leg. An unreadable ledger degrades to "nothing vendored" // for the rollback and fails closed at the vendored leg — exactly where // the run is about to mutate vendored state. - // From the manifest's project, like the manifest itself (#745). let vendor_state_result = if project_state { load_state(&args.common.project_root()).await } else { @@ -541,13 +513,17 @@ pub async fn run(args: RemoveArgs) -> i32 { return remove_hosted_only(&args, hosted_matches, &telemetry).await; } - emit_not_found( - args.common.json, - args.common.dry_run, - &args.identifier, - &telemetry, - ) - .await; + let msg = format!("No patch found matching identifier: {}", args.identifier); + track_patch_remove_failed(&msg, &telemetry).await; + if args.common.json { + let mut env = Envelope::new(Command::Remove); + env.dry_run = args.common.dry_run; + env.status = Status::NotFound; + env.error = Some(EnvelopeError::new("not_found", msg)); + crate::ui::print_json(&env.to_value()); + } else { + eprintln!("Error: {msg}"); + } return 1; } @@ -616,9 +592,9 @@ pub async fn run(args: RemoveArgs) -> i32 { // ── nested in-place rollback ──────────────────────────────────────── // Vendor-owned purls are excluded from the in-place restore (the // vendored leg below reverts them) unless their Cargo shared-cache copy - // still carries an agent-mode patch (#336); an unreadable ledger degrades to + // still carries an agent-mode patch; an unreadable ledger degrades to // "nothing vendored" here and fails closed at that leg. Under global - // scope the project's ledger still owns its own NuGet install (#489). + // scope the project's ledger still owns its own NuGet install. // The identifier is the scope, so `--ecosystems` does not narrow it. let vendored_keys: HashSet = if project_state { vendor_state_result @@ -639,7 +615,7 @@ pub async fn run(args: RemoveArgs) -> i32 { // `summary.rolledBack` counts them). A dry run restores nothing, so it // records none. let mut rolled_back_events: Vec = Vec::new(); - // Copies a global run left to the project's vendor ledger (#489): each + // Copies a global run left to the project's vendor ledger: each // a `skipped` / `vendored` event, since no vendored leg reports them. let mut project_owned: Vec = Vec::new(); // In-scope manifest entries the nested rollback SKIPPED because the @@ -753,7 +729,7 @@ pub async fn run(args: RemoveArgs) -> i32 { .with_details(serde_json::json!({ "path": r.package_path })) }) .collect(); - print_hosted_leg_warnings(&args.common, &rollback_warnings); + print_run_warnings(&args.common, &rollback_warnings); if loud { // Vendor-owned targets say nothing here: the vendored @@ -885,19 +861,11 @@ pub async fn run(args: RemoveArgs) -> i32 { } // `run_hosted_leg` printed one line per restored purl. printed_progress |= loud && !leg.reverted.is_empty(); - let hosted_action = if args.common.dry_run { - PatchAction::Verified - } else { - PatchAction::Removed - }; - for purl in &leg.reverted { - hosted_reverted_events.push( - PatchEvent::new(hosted_action, purl.clone()).with_reason( - "hosted_reverted", - "hosted lockfile pin restored to the upstream registry on remove", - ), - ); - } + hosted_reverted_events.extend( + leg.reverted + .iter() + .map(|purl| hosted_reverted_event(args.common.dry_run, purl)), + ); } } @@ -952,7 +920,7 @@ pub async fn run(args: RemoveArgs) -> i32 { } env.status = Status::PartialFailure; env.error = Some(EnvelopeError::new("vendor_revert_kept", msg)); - println!("{}", env.to_pretty_json()); + crate::ui::print_json(&env.to_value()); } else { eprintln!("Error: {msg}"); } @@ -1113,7 +1081,7 @@ pub async fn run(args: RemoveArgs) -> i32 { // The dry-run footer closes the whole preview, the blob-cleanup // preview above included. if loud && args.common.dry_run { - println!("\n{DRY_RUN_FOOTER}"); + println!("\nDry run: no changes made."); } if args.common.json { @@ -1186,10 +1154,7 @@ pub async fn run(args: RemoveArgs) -> i32 { .iter() .chain(&hosted_leg_warnings) .chain(&cleanup_warnings) - .map(|(code, detail)| crate::json_envelope::RunWarning { - code: code.clone(), - detail: detail.clone(), - }), + .map(|(code, detail)| crate::json_envelope::RunWarning::new(code, detail)), ); // The in-place restores ran before the manifest mutation. for ev in rolled_back_events { @@ -1226,29 +1191,17 @@ pub async fn run(args: RemoveArgs) -> i32 { if !vendor_leg.kept.is_empty() { env.status = Status::PartialFailure; } - println!("{}", env.to_pretty_json()); + crate::ui::print_json(&env.to_value()); } if !args.common.dry_run { track_patch_removed(removed.len(), &telemetry).await; } - if vendor_leg.kept.is_empty() { - 0 - } else { - // Errors print even under --silent; the per-key drift-keep lines - // above are gated, so name the outcome once here. - if !args.common.json { - eprintln!( - "Error: {}", - kept_error_line(&vendor_leg.kept_causes, "manifest record") - ); - } - 1 - } + kept_exit_code(&args.common, &vendor_leg.kept_causes, "manifest record") } /// The top-level error when every matching vendored entry was kept: the -/// drift wording and its normalize remedy only for drift-keeps (#1184). +/// drift wording and its normalize remedy only for drift-keeps. fn all_kept_message(identifier: &str, causes: &[KeepCause]) -> String { if causes.iter().all(|c| *c == KeepCause::Drift) { return format!( @@ -1312,7 +1265,7 @@ struct RemoveVendorLeg { kept_causes: Vec, /// `Removed`/`vendor_orphan_removed` events (dry runs too, as in /// `vendor --revert`): uuid dirs of the reverted packages that no - /// ledger entry owns, swept after the reverts (#1610). + /// ledger entry owns, swept after the reverts. orphans: Vec, /// Entries actually reverted and dropped from the ledger (wet runs). reverted_count: usize, @@ -1477,7 +1430,7 @@ async fn revert_vendored_matches( /// Sweep the uuid dirs under `.socket/vendor/` that no ledger entry owns /// and `target` names (a git merge of two vendor runs keeps both sides' -/// dirs, #1610), as `vendor --revert` does: into `leg.orphans`, with a +/// dirs), as `vendor --revert` does: into `leg.orphans`, with a /// `skipped` advisory for each one a lockfile still points into (kept) or /// that could not be deleted. Runs only after a vendored revert, past the /// remove's last abort point. `--preserve-state` keeps local state, @@ -1529,63 +1482,71 @@ async fn sweep_target_orphans( leg.orphans = sweep.removed.iter().map(orphan_removed_event).collect(); } -/// Why a hosted unwind stopped: a pin the upstream restore refused (or a -/// write failure). Each caller renders its own message (the -/// manifest-backed path adds that the manifest was not touched). -struct HostedUnwindError { - why: String, -} - /// Restore the hosted pins in `hosted_matches` to their upstream registry /// entries. Nothing is written unless every pin resolved (the restore is -/// all-or-nothing per pin, and a refused pin fails the remove). Shared by -/// the manifest-backed and hosted-only remove paths. +/// all-or-nothing per pin, and a refused pin fails the remove). The error +/// is the first refusal (or write failure), naming the pin and its remedy. async fn unwind_hosted( common: &GlobalArgs, hosted_matches: &[HostedPin], -) -> Result { +) -> Result { let leg = run_hosted_leg(common, hosted_matches).await; // Printed as soon as the leg returns, so a human run that then fails // still says what it did to the files. - print_hosted_leg_warnings(common, &leg.warnings); + print_run_warnings(common, &leg.warnings); if let Some((_, why)) = leg.failed.first().cloned() { - return Err(HostedUnwindError { why }); + return Err(why); } if let Some(warning) = super::rollback::retire_legacy_redirect_ledger(common).await { - print_hosted_leg_warnings(common, std::slice::from_ref(&warning)); + print_run_warnings(common, std::slice::from_ref(&warning)); } Ok(leg) } -/// Error code + message for a stopped hosted unwind. -fn hosted_unwind_error(err: HostedUnwindError, manifest_backed: bool) -> (&'static str, String) { - // `why` already names the pin (the restore's refusal) or the write - // that failed, with its remedy. - let HostedUnwindError { why } = err; - ( - "hosted_revert_failed", - if manifest_backed { - format!("{why}. The manifest was not modified.") - } else { - why - }, - ) -} - /// Report a stopped hosted unwind. JSON carries the whole message in the /// error envelope. A human run already saw `Error: ` — the hosted leg /// prints each refusal as it happens — so only the manifest-backed path's -/// extra fact is added; printing the refusal again would show the same -/// error twice (B77). -fn emit_hosted_unwind_error(common: &GlobalArgs, err: HostedUnwindError, manifest_backed: bool) { - let (code, msg) = hosted_unwind_error(err, manifest_backed); +/// extra fact is added. +fn emit_hosted_unwind_error(common: &GlobalArgs, why: String, manifest_backed: bool) { if common.json { - emit_error_envelope(true, common.dry_run, code, msg); + let msg = if manifest_backed { + format!("{why}. The manifest was not modified.") + } else { + why + }; + emit_error_envelope(true, common.dry_run, "hosted_revert_failed", msg); } else if manifest_backed { eprintln!("The manifest was not modified."); } } +/// The `Removed`/`hosted_reverted` event for a restored hosted pin +/// (`Verified` on a dry run). +fn hosted_reverted_event(dry_run: bool, purl: &str) -> PatchEvent { + let action = if dry_run { + PatchAction::Verified + } else { + PatchAction::Removed + }; + PatchEvent::new(action, purl.to_string()).with_reason( + "hosted_reverted", + "hosted lockfile pin restored to the upstream registry on remove", + ) +} + +/// The exit code of a remove whose vendored revert kept entries (1), or 0. +/// The per-key keep lines are gated by `--silent`, but this error line is +/// not; `retained` names the record kept beside the vendored state. +fn kept_exit_code(common: &GlobalArgs, causes: &[KeepCause], retained: &str) -> i32 { + if causes.is_empty() { + return 0; + } + if !common.json { + eprintln!("Error: {}", kept_error_line(causes, retained)); + } + 1 +} + /// Remove path for identifiers that match ONLY hosted lockfile pins (no /// manifest entry, no vendor-ledger entry): confirm, restore each pin's /// upstream registry entry, and report `Removed`/`hosted_reverted` events. Like the ledger-only vendored path, @@ -1660,34 +1621,29 @@ async fn remove_hosted_only( }; let mut env = Envelope::new(Command::Remove); env.dry_run = args.common.dry_run; - let action = if args.common.dry_run { - PatchAction::Verified - } else { - PatchAction::Removed - }; let mut warnings = leg.warnings.clone(); if args.preserve_state && !leg.reverted.is_empty() { warnings.push(note_hosted_state_not_preservable(&args.common)); } - for (code, detail) in &warnings { - env.warnings.push(crate::json_envelope::RunWarning { - code: code.clone(), - detail: detail.clone(), - }); + for (code, detail) in warnings { + env.warnings + .push(crate::json_envelope::RunWarning::new(code, detail)); } // Human per-purl lines already printed inside `run_hosted_leg`. - for purl in &leg.reverted { - env.record( - PatchEvent::new(action, purl.clone()) - .with_reason( - "hosted_reverted", - "hosted lockfile pin restored to the upstream registry on remove", - ) - .with_details(serde_json::json!({ "mode": crate::commands::HOSTED_MODE_LABEL })), - ); + let mut events: Vec = leg + .reverted + .iter() + .map(|purl| hosted_reverted_event(args.common.dry_run, purl)) + .collect(); + crate::commands::vendored_backend::tag_event_mode( + &mut events, + crate::commands::HOSTED_MODE_LABEL, + ); + for ev in events { + env.record(ev); } if args.common.json { - println!("{}", env.to_pretty_json()); + crate::ui::print_json(&env.to_value()); } if !args.common.dry_run { track_patch_removed(leg.reverted.len(), telemetry).await; @@ -1814,24 +1770,12 @@ async fn remove_ledger_only( } } if args.common.json { - println!("{}", env.to_pretty_json()); + crate::ui::print_json(&env.to_value()); } if !args.common.dry_run { track_patch_removed(leg.reverted_count, telemetry).await; } - if leg.kept.is_empty() { - 0 - } else { - // Errors print even under --silent; the per-key drift-keep lines - // are gated, so name the outcome once here. - if !args.common.json { - eprintln!( - "Error: {}", - kept_error_line(&leg.kept_causes, "ledger record") - ); - } - 1 - } + kept_exit_code(&args.common, &leg.kept_causes, "ledger record") } #[cfg(test)] @@ -1840,11 +1784,6 @@ mod tests { use socket_patch_core::manifest::schema::PatchRecord; use std::collections::HashMap; - #[test] - fn dry_run_footer_matches_repair() { - assert_eq!(DRY_RUN_FOOTER, "Dry run: no changes made."); - } - fn make_record(uuid: &str) -> PatchRecord { PatchRecord { uuid: uuid.to_string(), @@ -1979,8 +1918,8 @@ mod tests { } /// The shared target grammar: a bare name and a versionless purl - /// select every recorded version (`remove six` used to be "No patch - /// found"), and a name stays exact (`six` is not `sixer`). + /// select every recorded version, and a name stays exact (`six` is not + /// `sixer`). #[test] fn remove_by_name_or_versionless_purl_removes_every_version() { for token in ["six", "pkg:pypi/six"] { @@ -2159,14 +2098,23 @@ mod tests { " - pkg:npm/a@1", ] ); - let without = format_not_installed_warning(&[], &["pkg:npm/b@1", "pkg:npm/c@1"]); - assert_eq!(without.len(), 4); - assert!(without[1].starts_with("Warning: 2 removed patches had no matching installed")); - assert!(without[1].contains("their rollback was skipped")); - assert!(without[1].contains("reinstall the packages to restore their original files")); - let one = format_not_installed_warning(&[], &["pkg:npm/b@1"]); - assert!(one[1].contains("reinstall the package to restore its original files")); - assert!(!without[1].contains("was kept in .socket/blobs")); + assert_eq!( + format_not_installed_warning(&[], &["pkg:npm/b@1", "pkg:npm/c@1"]), + vec![ + "", + "Warning: 2 removed patches had no matching installed package, so their \ + rollback was skipped (a crawler miss would look the same); no local revert \ + data exists, so reinstall the packages to restore their original files:", + " - pkg:npm/b@1", + " - pkg:npm/c@1", + ] + ); + assert_eq!( + format_not_installed_warning(&[], &["pkg:npm/b@1"])[1], + "Warning: 1 removed patch had no matching installed package, so its rollback \ + was skipped (a crawler miss would look the same); no local revert data exists, \ + so reinstall the package to restore its original files:" + ); assert!(format_not_installed_warning(&[], &[]).is_empty()); } From b2e8bc9742e81aa9cf8856096b66acb61a06a77a Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 30 Oct 2026 03:23:31 +0000 Subject: [PATCH 4/4] Apply review cleanups to the remove/repair sweep The hosted-only path records its events directly with the hosted mode instead of collecting and tagging them, and stops cloning the leg's warnings. Repair's hosted-only print says why it adds a period. Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-cli/src/commands/remove.rs | 18 ++++++------------ crates/socket-patch-cli/src/commands/repair.rs | 1 + 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/crates/socket-patch-cli/src/commands/remove.rs b/crates/socket-patch-cli/src/commands/remove.rs index 561bd5c8d..eafb44413 100644 --- a/crates/socket-patch-cli/src/commands/remove.rs +++ b/crates/socket-patch-cli/src/commands/remove.rs @@ -1621,7 +1621,7 @@ async fn remove_hosted_only( }; let mut env = Envelope::new(Command::Remove); env.dry_run = args.common.dry_run; - let mut warnings = leg.warnings.clone(); + let mut warnings = leg.warnings; if args.preserve_state && !leg.reverted.is_empty() { warnings.push(note_hosted_state_not_preservable(&args.common)); } @@ -1630,17 +1630,11 @@ async fn remove_hosted_only( .push(crate::json_envelope::RunWarning::new(code, detail)); } // Human per-purl lines already printed inside `run_hosted_leg`. - let mut events: Vec = leg - .reverted - .iter() - .map(|purl| hosted_reverted_event(args.common.dry_run, purl)) - .collect(); - crate::commands::vendored_backend::tag_event_mode( - &mut events, - crate::commands::HOSTED_MODE_LABEL, - ); - for ev in events { - env.record(ev); + for purl in &leg.reverted { + env.record( + hosted_reverted_event(args.common.dry_run, purl) + .with_details(serde_json::json!({ "mode": crate::commands::HOSTED_MODE_LABEL })), + ); } if args.common.json { crate::ui::print_json(&env.to_value()); diff --git a/crates/socket-patch-cli/src/commands/repair.rs b/crates/socket-patch-cli/src/commands/repair.rs index 650710f12..4bbb3008f 100644 --- a/crates/socket-patch-cli/src/commands/repair.rs +++ b/crates/socket-patch-cli/src/commands/repair.rs @@ -124,6 +124,7 @@ pub async fn run(args: RepairArgs) -> i32 { ); crate::ui::print_json(&env.to_value()); } else if !args.common.silent { + // The JSON reason keeps its period-less text. println!("{msg}."); } return 0;