Fix human scan skipping re-apply of recorded patches (#732)

crates/socket-patch-cli/src/commands/scan/mod.rs

@@ -35,17 +35,17 @@ use crate::ui::{self, plural, print_json, StatusLine};
 
 use super::get::{download_and_apply_patches_with, DownloadParams, DownloadRun};
 
-pub use self::socket_yml_args::{SocketYmlArgs, MIN_SEVERITY_ENV};
 use self::policy::{load_invocation_policy, InvocationPolicy, PolicyLoadError, ScanPolicy};
+pub use self::socket_yml_args::{SocketYmlArgs, MIN_SEVERITY_ENV};
 
 mod discovery;
 mod gc;
 pub(crate) mod hosted;
 pub(crate) mod policy;
-mod socket_yml_args;
 pub(crate) mod render;
 pub(crate) mod rollout;
 pub mod rollout_args;
+mod socket_yml_args;
 pub(crate) mod vendor_flow;
 
 use self::discovery::{
@@ -65,13 +65,13 @@ use self::gc::gc_json;
 pub(crate) use self::hosted::boxed_run_redirect_selected;
 use self::hosted::run_redirect;
 pub(crate) use self::hosted::{vlt_rollback_heal, vlt_takeover_heal};
-pub(crate) use self::vendor_flow::{
-    boxed_vendor_step, preview_vendor_json, print_dry_run_refusals, VendorStep,
-};
 use self::vendor_flow::{
     boxed_vendor_interactive_path, boxed_vendor_json_path, fold_vendored_skips_into_apply,
     partition_skipped_selected,
 };
+pub(crate) use self::vendor_flow::{
+    boxed_vendor_step, preview_vendor_json, print_dry_run_refusals, VendorStep,
+};
 
 /// Packages per batch request on the authenticated API when `--batch-size`
 /// is not given: the server's own per-request maximum
@@ -318,11 +318,7 @@ pub struct ScanArgs {
     /// `requests`), or a purl with or without its version
     /// (`pkg:npm/lodash`, `pkg:pypi/requests@2.31.0`). Repeat the flag or
     /// separate with commas
-    #[arg(
-        long = "package",
-        env = "SOCKET_SCAN_PACKAGES",
-        value_delimiter = ','
-    )]
+    #[arg(long = "package", env = "SOCKET_SCAN_PACKAGES", value_delimiter = ',')]
     pub packages: Vec<String>,
 
     /// On a successful scan, also generate an OpenVEX 0.2.0 document.
@@ -500,9 +496,10 @@ async fn discover_selected(
     telemetry.flush().await;
     let error_count = failures.len();
     if error_count > 0 && error_count == packages.len() {
-        let err = failures
-            .last()
-            .map_or_else(|| "all patch-detail queries failed".to_string(), |(_, e)| e.clone());
+        let err = failures.last().map_or_else(
+            || "all patch-detail queries failed".to_string(),
+            |(_, e)| e.clone(),
+        );
         let message = format!("all {error_count} patch-detail queries failed: {err}");
         if detail_error_line {
             eprintln!("{}", render::fetch_details_failed(&failures));
@@ -568,7 +565,11 @@ fn classified_rows(
     packages: &[BatchPackagePatches],
     result: Option<&mut serde_json::Value>,
 ) -> Vec<rollout::Row> {
-    let failed: Vec<String> = discovered.failed.iter().map(|(purl, _)| purl.clone()).collect();
+    let failed: Vec<String> = discovered
+        .failed
+        .iter()
+        .map(|(purl, _)| purl.clone())
+        .collect();
     stage.incomplete = rollout::lookup_incomplete(&recorded.index, &failed, batch_failed);
     let rows = rollout::classify(&discovered.offers, &recorded.index, &stage.project);
     if let Some(result) = result {
@@ -1317,7 +1318,8 @@ fn project_dirs(cwd: &Path, paths: &[String]) -> Result<Vec<(PathBuf, bool)>, St
         let joined = cwd.join(raw);
         if raw.contains(['*', '?', '[']) {
             let pattern = joined.to_string_lossy().into_owned();
-            let matches = glob::glob(&pattern).map_err(|e| format!("invalid path pattern `{raw}`: {e}"))?;
+            let matches =
+                glob::glob(&pattern).map_err(|e| format!("invalid path pattern `{raw}`: {e}"))?;
             let before = dirs.len();
             dirs.extend(
                 matches
@@ -1390,7 +1392,10 @@ async fn run_project_dirs(
     }
     // One budget per invocation (§5.2): the directories spend it in sorted
     // order, and a package admitted in one is admitted free in the next.
-    let configured = match args.rollout.resolve_from_env(invocation.policy.max_new_patches()) {
+    let configured = match args
+        .rollout
+        .resolve_from_env(invocation.policy.max_new_patches())
+    {
         Ok(max) => max,
         Err(message) => {
             eprintln!("Error: {message}");
@@ -1491,19 +1496,19 @@ async fn run_scan(
     // error.
     let configured_cap = match args.rollout.carry.as_ref() {
         Some(carry) => carry.lock().configured,
-        None => match args.rollout.resolve_from_env(invocation.policy.max_new_patches()) {
+        None => match args
+            .rollout
+            .resolve_from_env(invocation.policy.max_new_patches())
+        {
             Ok(max) => max,
             Err(message) => {
                 eprintln!("Error: {message}");
                 return 2;
             }
         },
     };
-    let mut stage = rollout::Stage::new(
-        configured_cap,
-        args.rollout.carry.clone(),
-        &args.common.cwd,
-    );
+    let mut stage =
+        rollout::Stage::new(configured_cap, args.rollout.carry.clone(), &args.common.cwd);
 
     // Strict airgap (CLI_CONTRACT.md `--offline`): scan's patch discovery
     // is remote data, so refuse before the crawl and before the API client
@@ -1704,8 +1709,11 @@ async fn run_scan(
         .filter(|pkg| args.common.purl_ecosystem_selected(&pkg.purl))
         .collect();
 
-    let package_specs: Vec<&String> =
-        args.packages.iter().filter(|s| !s.trim().is_empty()).collect();
+    let package_specs: Vec<&String> = args
+        .packages
+        .iter()
+        .filter(|s| !s.trim().is_empty())
+        .collect();
     let filtered_crawled: Vec<_> = if package_specs.is_empty() {
         filtered_crawled
     } else {
@@ -1860,13 +1868,12 @@ async fn run_scan(
                 // `redirectState` rides the empty-discovery envelope too
                 // (same rule as the ≥1-package path). `wiringLive` is empty
                 // by construction: this run covered zero packages.
-                let redirect_state = (!args.common.is_global()).then_some(
-                    crate::commands::hosted_state_from_pins(
+                let redirect_state =
+                    (!args.common.is_global()).then_some(crate::commands::hosted_state_from_pins(
                         &socket_patch_core::patch::redirect::upstream::HostedPin::all(
                             ctx.discovery().await,
                         ),
-                    ),
-                );
+                    ));
                 if let Some(state) = redirect_state_json(redirect_state.as_ref(), &[]) {
                     result["redirectState"] = state;
                 }
@@ -2222,7 +2229,8 @@ async fn run_scan(
         // A report-only run selects nothing, but a severity floor or
         // `enabled: false` still hides candidates; report them like the
         // human arm does (the detail fetch runs only then).
-        if !apply && !vendor && policy.reports_selection() && !all_packages_with_patches.is_empty() {
+        if !apply && !vendor && policy.reports_selection() && !all_packages_with_patches.is_empty()
+        {
             if let Err((code, message)) = discover_selected(
                 &api_client,
                 &all_packages_with_patches,
@@ -2515,12 +2523,7 @@ async fn run_scan(
                     &all_packages_with_patches,
                     None,
                 );
-                updates = offer_updates(
-                    &rows,
-                    &discovered,
-                    &recorded,
-                    &all_packages_with_patches,
-                );
+                updates = offer_updates(&rows, &discovered, &recorded, &all_packages_with_patches);
                 rows
             }
             // `discover_selected` already printed the failure to stderr.
@@ -2730,8 +2733,11 @@ async fn run_scan(
         plan_kept_rows(&mut stage, rows, selected)
     };
 
-    // Drop selections the manifest already records at the same uuid.
-    // Agent mode only: vendored mode never reads the manifest.
+    // Set aside selections the manifest already records at the same uuid.
+    // A wet agent run still hands them to the download step below, whose
+    // nested apply re-applies them after a reinstall (#454, #732); a
+    // preview only names them. Agent mode only: vendored mode never reads
+    // the manifest.
     let recorded = |p: &PatchSearchResult| {
         existing_manifest
             .as_ref()
@@ -2743,17 +2749,18 @@ async fn run_scan(
     } else {
         selected.into_iter().partition(|p| recorded(p))
     };
+    let reapply = !report_only && !args.common.dry_run;
     if !silent {
         for p in &already_recorded {
             open_paragraph(&mut skip_paragraph);
             println!(
                 "{}",
-                render::already_recorded_line(&normalize_purl(&p.purl), &p.uuid)
+                render::already_recorded_line(&normalize_purl(&p.purl), &p.uuid, reapply)
             );
         }
     }
 
-    if selected.is_empty() {
+    if selected.is_empty() && (!reapply || already_recorded.is_empty()) {
         if !silent {
             open_paragraph(&mut skip_paragraph);
             if !stage.deferred_keys().is_empty() {
@@ -2764,6 +2771,8 @@ async fn run_scan(
                 }
             } else if already_recorded.is_empty() {
                 println!("No patches selected.");
+            } else if args.common.dry_run && !report_only {
+                println!("{}", render::ALL_ALREADY_RECORDED_DRY_RUN);
             } else {
                 println!("{}", render::ALL_ALREADY_RECORDED);
             }
@@ -2773,7 +2782,7 @@ async fn run_scan(
     }
 
     // Display detailed summary of selected patches (skipped under --silent).
-    if !silent {
+    if !silent && !selected.is_empty() {
         if vendor {
             println!("\nPatches to vendor:\n");
         } else {
@@ -2927,9 +2936,16 @@ async fn run_scan(
         )
         .await
     } else {
-        let (code, _) =
-            download_and_apply_patches_with(&selected, &params, &download_run(&args, &api_client))
-                .await;
+        // The recorded selections ride along: the fetch loop skips their
+        // record and the nested apply re-applies them (a no-op on disk
+        // when the installed copy is still patched).
+        let to_download: Vec<_> = selected.iter().chain(&already_recorded).cloned().collect();
+        let (code, _) = download_and_apply_patches_with(
+            &to_download,
+            &params,
+            &download_run(&args, &api_client),
+        )
+        .await;
         code
     };
 
@@ -2982,14 +2998,20 @@ mod tests {
             dirs.iter()
                 .map(|(d, explicit)| {
                     (
-                        d.strip_prefix(tmp.path()).unwrap().to_string_lossy().replace('\\', "/"),
+                        d.strip_prefix(tmp.path())
+                            .unwrap()
+                            .to_string_lossy()
+                            .replace('\\', "/"),
                         *explicit,
                     )
                 })
                 .collect()
         };
-        let got = project_dirs(tmp.path(), &["apps/*".into(), "libs/core".into(), "apps/web".into()])
-            .unwrap();
+        let got = project_dirs(
+            tmp.path(),
+            &["apps/*".into(), "libs/core".into(), "apps/web".into()],
+        )
+        .unwrap();
         // Named literally = explicit (also when a glob matches it too).
         assert_eq!(
             rel(got),

crates/socket-patch-cli/src/commands/scan/render.rs

@@ -300,18 +300,27 @@ pub(super) fn not_installed_skip_line(purl: &str) -> String {
     )
 }
 
-/// `[skip]` line for a selection the manifest already records.
-pub(super) fn already_recorded_line(purl: &str, uuid: &str) -> String {
+/// Line for a selection the manifest already records: `[re-apply]` on a
+/// wet agent run (the nested apply re-applies it), `[skip]` in a preview.
+pub(super) fn already_recorded_line(purl: &str, uuid: &str, reapply: bool) -> String {
+    let tag = if reapply { "re-apply" } else { "skip" };
     format!(
-        "  [skip] {purl} (already recorded: {})",
+        "  [{tag}] {purl} (already recorded: {})",
         super::super::get::short_uuid(uuid)
     )
 }
 
-/// Printed when every selection is already recorded (agent mode).
+/// Printed when every selection is already recorded and nothing is applied
+/// (report-only scan).
 pub(super) const ALL_ALREADY_RECORDED: &str =
     "All selected patches are already recorded in the manifest; run `socket-patch apply` to re-apply them.";
 
+/// [`ALL_ALREADY_RECORDED`] for an agent-mode `--dry-run`. A report-only
+/// dry run keeps [`ALL_ALREADY_RECORDED`]: dropping `--dry-run` there still
+/// only reports.
+pub(super) const ALL_ALREADY_RECORDED_DRY_RUN: &str =
+    "All selected patches are already recorded in the manifest; a run without --dry-run re-applies them.";
+
 /// The terminal error when no package's patch details could be fetched.
 pub(super) fn fetch_details_failed(failed: &[(String, String)]) -> String {
     match failed {
@@ -746,7 +755,10 @@ mod tests {
 
     #[test]
     fn report_only_hint_names_agent_mode() {
-        assert_eq!(report_only_hint()[0], "To apply these patches in place, run:");
+        assert_eq!(
+            report_only_hint()[0],
+            "To apply these patches in place, run:"
+        );
         assert!(report_only_hint()[1].contains("--mode agent"));
     }
 
@@ -760,9 +772,13 @@ mod tests {
         assert!(l.contains("`socket-patch scan --mode vendored`"), "{l}");
         assert!(!l.contains("--vendor`"), "{l}");
         assert_eq!(
-            already_recorded_line("pkg:npm/x@1", "884e9f6d-aaaa"),
+            already_recorded_line("pkg:npm/x@1", "884e9f6d-aaaa", false),
             "  [skip] pkg:npm/x@1 (already recorded: 884e9f6d)"
         );
+        assert_eq!(
+            already_recorded_line("pkg:npm/x@1", "884e9f6d-aaaa", true),
+            "  [re-apply] pkg:npm/x@1 (already recorded: 884e9f6d)"
+        );
     }
 
     #[test]