diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index 3caf2fd0f..4c9a51a9f 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -878,7 +878,11 @@ worse, lets a warm cache silently serve unpatched bytes): whole-file wiring cannot tell a converged fragment from a drifted one, keep the artifact exactly while the live `composer.lock` / `pom.xml` / `nuget.config` still names its `.socket/vendor//` dir — a file that no longer references it is warned about and the - artifact removed; in the npm family (npm, yarn classic and berry, pnpm, bun) a recorded lock entry + artifact removed; nuget (v5.0, #537) compares `nuget.config` line-ending-insensitively, so a + `core.autocrlf` checkout of the file vendor wrote is not drift (the original is restored in the + checkout's line endings), and while `nuget.config` is drift-kept the `packages.lock.json` pin is + kept too (`vendor_lock_entry_drifted`), never reverted under a config that still routes the id to + the vendored feed; in the npm family (npm, yarn classic and berry, pnpm, bun) a recorded lock entry that no longer exists at all — the user removed the dependency — is not drift: it warns `vendor_lock_entry_removed` and the artifact and entry are kept unless every wired file that exists was read and none mentions the uuid in any spelling (an unreadable lock keeps them), so `rollback` / `remove` / `scan --prune` clean up diff --git a/crates/socket-patch-core/src/vendor/nuget_feed.rs b/crates/socket-patch-core/src/vendor/nuget_feed.rs index 620973346..8d8f7df40 100644 --- a/crates/socket-patch-core/src/vendor/nuget_feed.rs +++ b/crates/socket-patch-core/src/vendor/nuget_feed.rs @@ -11,6 +11,7 @@ use crate::patch::path_safety::{is_safe_multi_segment, is_safe_single_segment}; use crate::utils::fs::{ atomic_write_artifact, atomic_write_bytes_preserving_mode, read_regular_to_string, }; +use crate::utils::line_endings::{eol_eq, respell, terminator}; use crate::utils::purl::{build_nuget_purl, parse_nuget_purl}; use super::common::{ @@ -730,6 +731,27 @@ pub async fn revert_nuget_opts( }; let mut warnings = Vec::new(); + // The lock pin may only go back to the upstream contentHash when + // nuget.config stops routing the id to the vendored feed. A config that + // will be left wired (drift-kept: it still names the feed) with the lock + // reverted under it fails every restore NU1403 (#537), so its lock pin + // is kept and the package stays consistently vendored. Decided up front + // (a read-only preview of the config restore), so the lock still goes + // first and a lock failure leaves the config wired for the retry. + let mut config_still_routes = false; + for w in entry + .wiring + .iter() + .filter(|w| w.kind == CONFIG_SOURCE_WIRING_KIND) + { + if matches!( + revert_config_record(project_root, &uuid_dir_rel, w, true).await, + Ok(false) + ) && config_references(project_root, &w.file, &uuid_dir_rel).await + { + config_still_routes = true; + } + } // Reverse application order: lock pin, then the (no-op) mapping audit // record, then the authoritative config restore. for w in entry.wiring.iter().rev() { @@ -742,6 +764,22 @@ pub async fn revert_nuget_opts( "refusing revert: unsafe wiring file path {:?}", w.file )), + LOCK_WIRING_KIND if config_still_routes => { + warnings.push(VendorWarning::new( + "vendor_lock_entry_drifted", + format!( + "{} still routes {} to the vendored feed, so its {} pin is kept", + entry + .wiring + .iter() + .find(|c| c.kind == CONFIG_SOURCE_WIRING_KIND) + .map_or("nuget.config", |c| c.file.as_str()), + w.key.as_deref().unwrap_or(""), + w.file + ), + )); + continue; + } LOCK_WIRING_KIND => revert_lock_record(&project_root.join(&w.file), w, dry_run).await, // Audit-only: the whole-file config restore lives on the source // record, so there is nothing to undo here. @@ -1123,17 +1161,28 @@ async fn revert_config_record( Err(e) => return Err(format!("unreadable {}: {e}", config_path.display())), }; - // (a) Byte-identical to what we wrote → the whole-file restore/delete is - // provably safe (nothing changed since vendoring). - let new_matches = matches!(&w.new, Some(Value::String(n)) if *n == live); + // (a) What we wrote, up to line endings → the whole-file restore/delete + // is provably safe (nothing changed since vendoring). A + // `core.autocrlf` checkout (Git for Windows' default) hands back + // our LF text as CRLF; that is git's encoding, not an edit (#537). + let new_matches = + matches!(&w.new, Some(Value::String(n)) if eol_eq(n.as_bytes(), live.as_bytes())); if new_matches { if dry_run { return Ok(true); } match &w.original { - // Pre-existed → restore the verbatim original bytes. + // Pre-existed → restore the original, verbatim when the live + // file still has the line endings we wrote, else spelled in the + // live file's (the checkout converted it). Some(Value::String(orig)) => { - atomic_write_bytes_preserving_mode(&config_path, orig.as_bytes()) + let wrote_lf = matches!(&w.new, Some(Value::String(n)) if *n == live); + let restored = if wrote_lf { + orig.clone() + } else { + respell(orig, terminator(&live)) + }; + atomic_write_bytes_preserving_mode(&config_path, restored.as_bytes()) .await .map_err(|e| format!("failed to restore {}: {e}", config_path.display()))?; } @@ -1151,8 +1200,22 @@ async fn revert_config_record( // two authored elements. Both are reproduced verbatim from the source // key + uuid dir (the source ``) and matched structurally by our // source key (the mapping ``). - let source_add = format!(" \n"); - let mapping_block = excise_source_mapping(&live, source_key); + // Vendor inserts LF lines, even into a CRLF file (which then has + // mixed endings until git converts it), so the LF spelling is tried + // first, then the file's own terminator (a `core.autocrlf` checkout). + let spelled = |nl: &str| { + ( + format!(" {nl}"), + excise_source_mapping(&live, source_key, nl), + ) + }; + let lf = spelled("\n"); + let (source_add, mapping_block) = + if live.contains(&lf.0) || lf.1.is_some() || terminator(&live) == "\n" { + lf + } else { + spelled(terminator(&live)) + }; if !live.contains(&source_add) && mapping_block.is_none() { // (c) Neither authored element is present verbatim → drift, leave alone. return Ok(false); @@ -1175,16 +1238,32 @@ async fn revert_config_record( Ok(true) } +/// Whether the project-root config `file` (a recorded wiring basename) +/// still names the vendored feed dir `uuid_dir_rel`. An unsafe or +/// unreadable file answers yes: when in doubt the lock pin is kept with +/// the config that may route to it. +async fn config_references(project_root: &Path, file: &str, uuid_dir_rel: &str) -> bool { + if !is_safe_single_segment(file) { + return true; + } + match read_regular_to_string(&project_root.join(file)).await { + Ok(text) => text.contains(uuid_dir_rel), + Err(e) => e.kind() != std::io::ErrorKind::NotFound, + } +} + /// The exact ` … \n` block we /// authored in the mapping section, if present verbatim in `config`. Anchored on /// our source key and closed at the first `` after it, then /// extended through the trailing newline so the excision leaves no blank line. /// `None` when our mapping block is absent (already reverted, or edited). -fn excise_source_mapping(config: &str, source_key: &str) -> Option { - let open = format!(" \n"); +/// `nl` is the config's line terminator ([`terminator`]): a `core.autocrlf` +/// checkout spells our LF block in CRLF. +fn excise_source_mapping(config: &str, source_key: &str, nl: &str) -> Option { + let open = format!(" {nl}"); let open_at = config.find(&open)?; - let close = " \n"; - let rel_close = config[open_at..].find(close)?; + let close = format!(" {nl}"); + let rel_close = config[open_at..].find(&close)?; let end = open_at + rel_close + close.len(); Some(config[open_at..end].to_string()) } @@ -2513,6 +2592,172 @@ mod tests { assert!(after.contains("key=\"nuget.org\"")); } + /// `path` rewritten with CRLF line endings, as a `core.autocrlf=true` + /// checkout (Git for Windows' default) hands it back. + async fn autocrlf(path: &Path) { + let text = tokio::fs::read_to_string(path).await.unwrap(); + tokio::fs::write(path, text.replace("\r\n", "\n").replace('\n', "\r\n")) + .await + .unwrap(); + } + + /// #537: after an autocrlf checkout, revert restores BOTH the + /// pre-existing config (in the checkout's CRLF) and the lock, with no + /// drift warning and the feed removed. + #[tokio::test] + async fn revert_on_an_autocrlf_checkout_restores_config_and_lock() { + let orig_cfg = "\n\ + \n\ + \x20 \n\ + \x20 \n\ + \x20 \n\ + \n"; + let (dir, blobs, installed, record) = fixture(true, Some(orig_cfg)).await; + let root = dir.path(); + let lock_before = tokio::fs::read_to_string(root.join(PACKAGES_LOCK)) + .await + .unwrap(); + let (_r, entry, _w) = + unwrap_done(run_vendor(root, &blobs, &installed, &record, false).await); + let entry = entry.unwrap(); + autocrlf(&root.join("nuget.config")).await; + autocrlf(&root.join(PACKAGES_LOCK)).await; + + let outcome = revert_nuget(&entry, root, false).await; + assert!(outcome.success, "{:?}", outcome.error); + assert!(outcome.warnings.is_empty(), "{:?}", outcome.warnings); + assert!(!outcome.kept_artifact); + assert_eq!( + tokio::fs::read_to_string(root.join("nuget.config")) + .await + .unwrap(), + orig_cfg.replace('\n', "\r\n"), + "the original config, in the checkout's line endings" + ); + assert_eq!( + tokio::fs::read_to_string(root.join(PACKAGES_LOCK)) + .await + .unwrap(), + lock_before.replace('\n', "\r\n") + ); + assert!(!root.join(format!(".socket/vendor/nuget/{UUID}")).exists()); + } + + /// #537: a config we created is deleted on an autocrlf checkout too. + #[tokio::test] + async fn revert_on_an_autocrlf_checkout_deletes_a_created_config() { + let (dir, blobs, installed, record) = fixture(true, None).await; + let root = dir.path(); + let (_r, entry, _w) = + unwrap_done(run_vendor(root, &blobs, &installed, &record, false).await); + let entry = entry.unwrap(); + autocrlf(&root.join("nuget.config")).await; + let outcome = revert_nuget(&entry, root, false).await; + assert!(outcome.success, "{:?}", outcome.error); + assert!(outcome.warnings.is_empty(), "{:?}", outcome.warnings); + assert!(!root.join("nuget.config").exists()); + } + + /// #537: the fragment excision (a sibling's wiring made the file differ + /// from what we wrote) finds our CRLF-spelled elements and keeps the + /// file's CRLF. + #[tokio::test] + async fn revert_excises_our_crlf_fragments_beside_a_sibling() { + let (dir, blobs, installed, record) = fixture(true, None).await; + let root = dir.path(); + let (_r, entry, _w) = + unwrap_done(run_vendor(root, &blobs, &installed, &record, false).await); + let entry = entry.unwrap(); + let cfg = root.join("nuget.config"); + let wired = tokio::fs::read_to_string(&cfg).await.unwrap(); + let sibling = wired.replacen( + "", + " \n ", + 1, + ); + tokio::fs::write(&cfg, &sibling).await.unwrap(); + autocrlf(&cfg).await; + let outcome = revert_nuget(&entry, root, false).await; + assert!(outcome.success, "{:?}", outcome.error); + assert!(outcome.warnings.is_empty(), "{:?}", outcome.warnings); + let after = tokio::fs::read_to_string(&cfg).await.unwrap(); + assert!(!after.contains(UUID), "{after}"); + assert!(after.contains("key=\"corp\""), "{after}"); + assert!( + !after.replace("\r\n", "").contains('\n'), + "CRLF kept: {after:?}" + ); + } + + /// #537 review: an existing CRLF config gets our LF lines (mixed until + /// git converts it); a CRLF sibling edit sends the revert down the + /// excision path, which must still find our LF fragments. + #[tokio::test] + async fn revert_excises_lf_fragments_from_a_mixed_crlf_config() { + let orig = "\r\n\r\n \r\n \r\n \r\n \r\n \r\n \r\n \r\n \r\n\r\n"; + let (dir, blobs, installed, record) = fixture(true, Some(orig)).await; + let root = dir.path(); + let (_r, entry, _w) = + unwrap_done(run_vendor(root, &blobs, &installed, &record, false).await); + let entry = entry.unwrap(); + let cfg = root.join("nuget.config"); + let wired = tokio::fs::read_to_string(&cfg).await.unwrap(); + let sibling = wired.replacen( + " ", + " \r\n ", + 1, + ); + tokio::fs::write(&cfg, &sibling).await.unwrap(); + let outcome = revert_nuget(&entry, root, false).await; + assert!(outcome.success, "{:?}", outcome.error); + assert!(outcome.warnings.is_empty(), "{:?}", outcome.warnings); + assert!(!outcome.kept_artifact); + let after = tokio::fs::read_to_string(&cfg).await.unwrap(); + assert!(!after.contains(UUID), "{after}"); + assert!(after.contains("key=\"corp\""), "{after}"); + } + + /// #537: a config left wired (drift-kept, it still routes to the feed) + /// keeps its lock pin too, so restore never sees an upstream lock under + /// a vendored mapping. + #[tokio::test] + async fn drift_kept_config_keeps_the_lock_pin() { + let (dir, blobs, installed, record) = fixture(true, None).await; + let root = dir.path(); + let (_r, entry, _w) = + unwrap_done(run_vendor(root, &blobs, &installed, &record, false).await); + let entry = entry.unwrap(); + let pinned = tokio::fs::read_to_string(root.join(PACKAGES_LOCK)) + .await + .unwrap(); + // Tooling re-serialized the config: our elements no longer match + // verbatim, but the source still points at the feed. + let cfg = root.join("nuget.config"); + let wired = tokio::fs::read_to_string(&cfg).await.unwrap(); + tokio::fs::write(&cfg, wired.replace(" <", "\t\t<")) + .await + .unwrap(); + let outcome = revert_nuget(&entry, root, false).await; + assert!(outcome.success, "{:?}", outcome.error); + assert!(outcome.kept_artifact); + assert_eq!( + tokio::fs::read_to_string(root.join(PACKAGES_LOCK)) + .await + .unwrap(), + pinned, + "the lock keeps the vendored pin while the config routes to it" + ); + assert!( + outcome + .warnings + .iter() + .any(|w| w.code == "vendor_lock_entry_drifted" + && w.detail.contains("still routes")), + "{:?}", + outcome.warnings + ); + } + #[tokio::test] async fn revert_warns_when_our_source_key_already_gone() { // The user regenerated nuget.config, dropping our source entirely. @@ -2569,13 +2814,13 @@ mod tests { \x20 \n\ \x20 \n\ \x20 \n"; - let block = excise_source_mapping(cfg, "socket-patch-abc").unwrap(); + let block = excise_source_mapping(cfg, "socket-patch-abc", "\n").unwrap(); assert!(block.contains("key=\"socket-patch-abc\"")); assert!(block.contains("Newtonsoft.Json")); // Does not swallow the sibling nuget.org block. assert!(!block.contains("nuget.org")); // Absent key → None. - assert!(excise_source_mapping(cfg, "socket-patch-missing").is_none()); + assert!(excise_source_mapping(cfg, "socket-patch-missing", "\n").is_none()); } #[tokio::test]