From 92630055336f74d898c59a1e2124f7cd907e171f Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 15:00:36 +0000 Subject: [PATCH 1/5] Start refactor for #594 Assisted-by: Claude Code:claude-opus-5-5 From 3f929fd3a95e122588353464aad9171a24f9a2a2 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 15:16:29 +0000 Subject: [PATCH 2/5] Wire vendored nuget.config via formats::nuget Vendored NuGet now reads the source keys and finds the , and anchors through formats::nuget::parse_config, the reader that hosted, upstream restore and VEX already use. The private substring scanner (blank_comments, parse_config_source_keys, attr_value, self_closing_package_sources, insert_at_line) is deleted. User impact: - A close tag written with whitespace () is now the section that gets extended; vendor used to append a second section NuGet ignores, so restore failed NU1100/NU1403 (#685). - An empty is expanded in place instead of left beside a second mapping section. - A section opened and closed on one line receives the source inside it, not before its open tag. - Catch-all keys are written XML-encoded, so a key with & or a quote keeps its identity. - Malformed XML or a repeated section is refused with "malformed XML or a repeated section; not wired" instead of being spliced at the first substring match, as hosted already does. Output bytes for well-formed configs are unchanged. Fixes #685 Refs #594 Assisted-by: Claude Code:claude-opus-5-5 --- .../src/formats/nuget/mod.rs | 14 + .../src/vendor/nuget_feed.rs | 529 +++++++++++------- 2 files changed, 335 insertions(+), 208 deletions(-) diff --git a/crates/socket-patch-core/src/formats/nuget/mod.rs b/crates/socket-patch-core/src/formats/nuget/mod.rs index fdf41875c..1499a27d2 100644 --- a/crates/socket-patch-core/src/formats/nuget/mod.rs +++ b/crates/socket-patch-core/src/formats/nuget/mod.rs @@ -291,6 +291,20 @@ fn decode_entities(raw: &str) -> String { out } +/// Encode `value` for a double-quoted attribute: the inverse of +/// [`parse_config`]'s decoding, so a key read as `a&b` is written back as +/// `a&b` and keeps its identity. +pub(crate) fn xml_attribute(value: &str) -> String { + value + .replace('&', "&") + .replace('"', """) + .replace('<', "<") + // Literal XML attribute whitespace would be normalized to spaces. + .replace('\t', " ") + .replace('\n', " ") + .replace('\r', " ") +} + #[cfg(test)] mod tests { #[test] diff --git a/crates/socket-patch-core/src/vendor/nuget_feed.rs b/crates/socket-patch-core/src/vendor/nuget_feed.rs index 70d6be479..13c335489 100644 --- a/crates/socket-patch-core/src/vendor/nuget_feed.rs +++ b/crates/socket-patch-core/src/vendor/nuget_feed.rs @@ -256,7 +256,8 @@ async fn nuget_prelude( // key merely mentioned elsewhere — is not wiring NuGet reads. let config_wired = config_text .as_deref() - .is_some_and(|t| parse_config_source_keys(&blank_comments(t)).contains(&source_key)); + .and_then(crate::formats::nuget::parse_config) + .is_some_and(|parsed| parsed.sources.iter().any(|(key, _)| *key == source_key)); let in_sync = config_wired && { // One guarded read of the committed nupkg serves both the member-hash // check and the lock's content-hash pin. @@ -891,17 +892,24 @@ fn build_config_edit( }) } Some(text) => { - // Every anchor find and source scan runs against the - // comment-blanked view (same length, so offsets splice into - // `text`). NuGet never reads a comment: a commented-out section - // must not capture an insert (the wired source would be invisible - // and restore would silently serve the UNPATCHED package), and a - // commented-out `` must not become a catch-all target (the - // mapping would fan `*` out to a source that does not exist). - let visible = blank_comments(text); + // Keys and anchors come from the one `nuget.config` reader that + // hosted, restore and VEX use. NuGet never reads a comment, CDATA + // or an element outside `configuration/
`: a commented-out + // section must not capture an insert (the wired source would be + // invisible and restore would silently serve the UNPATCHED + // package), and a commented-out `` must not become a + // catch-all target (the mapping would fan `*` out to a source that + // does not exist). Malformed XML or a repeated section has no + // single live anchor, so it is refused rather than guessed at. + let parsed = parse_wirable_config(text)?; // Whether we are about to CREATE the mapping section (vs. extend an - // existing one) — decided against the pre-edit text. - let creating_mapping = !visible.contains(""); + // existing one) — decided against the pre-edit text. An empty + // self-closing `` maps nothing, so it is + // created (expanded in place) too. + let creating_mapping = parsed + .source_mapping + .as_ref() + .is_none_or(|section| section.close_start.is_none()); // The pre-existing sources the catch-all fans `*` out to. When the // config has NONE and we are creating a mapping from scratch, a // socket-only mapping would NU1100 every other package, so seed the @@ -912,7 +920,12 @@ fn build_config_edit( // suppressing the seed on it recreates the exact socket-only // mapping the seed exists to prevent. Mirrors // redirect::add_nuget_source. - let mut catch_all_keys = parse_config_source_keys(&visible); + let mut catch_all_keys: Vec = Vec::new(); + for (key, _) in &parsed.sources { + if !catch_all_keys.contains(key) { + catch_all_keys.push(key.clone()); + } + } let seed_nuget_org = creating_mapping && catch_all_keys.is_empty(); let source_add = format!(" \n"); @@ -931,45 +944,59 @@ fn build_config_edit( // self-closing `` carries no children, so // expand it in place into an open/close pair rather than leaving // it dangling beside a duplicate element. - let with_source = if let Some((start, end)) = self_closing_package_sources(&visible) { - let mut expanded = String::with_capacity(text.len() + injected_sources.len() + 40); - expanded.push_str(&text[..start]); - expanded.push_str(&format!( - "\n{injected_sources} " - )); - expanded.push_str(&text[end..]); - expanded - } else if let Some(at) = visible.find("") { - insert_at_line(text, at, &injected_sources) - } else if let Some(at) = visible.find("") { - let block = format!(" \n{injected_sources} \n"); - insert_at_line(text, at, &block) - } else { - return Err("nuget.config has no to edit".to_string()); + let no_root = || "nuget.config has no to edit".to_string(); + let with_source = match &parsed.package_sources { + Some(section) => { + insert_children(text, section, "packageSources", &injected_sources) + } + None => { + let root = parsed.configuration.as_ref().ok_or_else(no_root)?; + let block = + format!(" \n{injected_sources} \n"); + insert_before_close(text, root, &block).ok_or_else(no_root)? + } }; // 2. Mapping: extend an existing section, or create one over the - // pre-existing sources (the load-bearing catch-all). The blanked - // view is recomputed — step 1 shifted the offsets. - let visible_ws = blank_comments(&with_source); + // pre-existing sources (the load-bearing catch-all). Step 1 + // shifted the offsets, so the edited text is re-read through + // the same tokenizer. + let updated = parse_wirable_config(&with_source)?; let new_text = if !creating_mapping { - let at = visible_ws.find("").ok_or_else(|| { + let section = updated.source_mapping.as_ref().ok_or_else(|| { "could not locate to insert the mapping".to_string() })?; - insert_at_line(&with_source, at, &mapping_fragment) + insert_children( + &with_source, + section, + "packageSourceMapping", + &mapping_fragment, + ) } else { - let mut block = String::from(" \n"); + let mut inner = String::new(); for key in &catch_all_keys { - block.push_str(&format!( - " \n \n \n" + inner.push_str(&format!( + " \n \n \n", + crate::formats::nuget::xml_attribute(key) )); } - block.push_str(&mapping_fragment); - block.push_str(" \n"); - let at = visible_ws.find("").ok_or_else(|| { - "could not locate to insert a packageSourceMapping section" - .to_string() - })?; - insert_at_line(&with_source, at, &block) + inner.push_str(&mapping_fragment); + match &updated.source_mapping { + Some(section) => { + insert_children(&with_source, section, "packageSourceMapping", &inner) + } + None => { + let block = + format!(" \n{inner} \n"); + updated + .configuration + .as_ref() + .and_then(|root| insert_before_close(&with_source, root, &block)) + .ok_or_else(|| { + "could not locate to insert a packageSourceMapping section" + .to_string() + })? + } + } }; Ok(ConfigEdit { new_text, @@ -979,121 +1006,58 @@ fn build_config_edit( } } -/// `text` with every `` comment blanked to spaces (newlines kept), -/// preserving length so offsets found in the blanked view splice into the -/// original. NuGet never reads a comment, so anchors and source keys inside -/// one must be invisible to the wiring logic — the nuget twin of maven's -/// `find_wireable_anchor` comment masking. An unterminated comment blanks -/// through EOF (fail-closed). -fn blank_comments(text: &str) -> String { - let mut out = text.as_bytes().to_vec(); - let mut from = 0; - while let Some(rel) = text[from..].find("") { - Some(rel_end) => start + 4 + rel_end + 3, - None => text.len(), - }; - for b in &mut out[start..end] { - if *b != b'\n' { - *b = b' '; - } - } - from = end; - } - // Every replaced byte became ASCII space; newlines are never continuation - // bytes, so the result is valid UTF-8. - String::from_utf8(out).expect("blanking preserves UTF-8") -} - -/// Insert `insertion` (already newline-terminated) at the start of the line -/// containing byte offset `at` — the offset comes from the comment-blanked -/// view, which shares offsets with `text`. -fn insert_at_line(text: &str, at: usize, insertion: &str) -> String { - let line_start = text[..at].rfind('\n').map(|n| n + 1).unwrap_or(0); - let mut out = String::with_capacity(text.len() + insertion.len()); - out.push_str(&text[..line_start]); - out.push_str(insertion); - out.push_str(&text[line_start..]); - out +/// `text` through [`crate::formats::nuget::parse_config`], or the refusal +/// when it has no single live layout to wire into. +fn parse_wirable_config(text: &str) -> Result { + crate::formats::nuget::parse_config(text) + .filter(|parsed| !parsed.repeated_sections) + .ok_or_else(|| { + "nuget.config has malformed XML or a repeated section; not wired".to_string() + }) } -/// Extract the `key` attribute of every `` element inside -/// ``. Deliberately minimal (no XML parser dependency): scans -/// the packageSources span for `` elements. These are -/// the "pre-existing sources" the catch-all maps `*` to. Callers pass the -/// comment-blanked text so a commented-out source never contributes a key. -fn parse_config_source_keys(text: &str) -> Vec { - let mut out = Vec::new(); - let Some(start) = text.find("` - // (valid, common) or a malformed config NuGet itself would reject. Scanning - // to EOF instead would harvest `` entries from unrelated - // sections (``, ``, …) as phantom catch-all - // sources — mapping `*` to a key NuGet has no source for hard-fails every - // restore. - let Some(end) = text[start..].find("").map(|e| start + e) else { +/// Insert `children` (newline-terminated lines) as the last children of +/// `section`, or expand a self-closing `section` in place (its attributes +/// kept) into an open/close pair holding them. +fn insert_children( + text: &str, + section: &crate::formats::nuget::ConfigSection, + name: &str, + children: &str, +) -> String { + if let Some(out) = insert_before_close(text, section, children) { return out; - }; - let span = &text[start..end]; - let mut rest = span; - while let Some(add_at) = rest.find("'. - let elem_end = after.find('>').unwrap_or(after.len()); - let elem = &after[..elem_end]; - if let Some(key) = attr_value(elem, "key") { - if !out.contains(&key) { - out.push(key); - } - } - rest = &after[elem_end..]; } + let head = text[section.open.start..section.open.end - 2].trim_end(); + let mut out = String::with_capacity(text.len() + children.len() + 2 * name.len() + 8); + out.push_str(&text[..section.open.start]); + out.push_str(&format!("{head}>\n{children} ")); + out.push_str(&text[section.open.end..]); out } -/// The value of `="..."` inside an element's attribute text, if present. -/// Tolerates whitespace around `=` (`key = "nuget.org"` is valid XML NuGet -/// parses): a real source the scan misses would read as "no sources", -/// triggering a duplicate nuget.org seed and leaving the missed source out of -/// the catch-all fan-out. -fn attr_value(elem: &str, attr: &str) -> Option { - let mut rest = elem; - loop { - let at = rest.find(attr)?; - let after = rest[at + attr.len()..].trim_start(); - if let Some(eq) = after.strip_prefix('=') { - // NuGet accepts either XML quote style; tolerate both, like the - // redirect twin (patch/redirect/mod.rs nuget key harvesting). - let val = eq.trim_start(); - for quote in ['"', '\''] { - if let Some(quoted) = val.strip_prefix(quote) { - let close = quoted.find(quote)?; - return Some(quoted[..close].to_string()); - } - } - } - rest = &rest[at + attr.len()..]; - } -} - -/// The `[start, end)` byte span of a self-closing `` element -/// (any whitespace before `/>`), or `None` if the config has no such element. -/// Deliberately minimal (no XML parser dependency), matching the rest of this -/// module's scanning style. -fn self_closing_package_sources(text: &str) -> Option<(usize, usize)> { - let start = text.find("` for a - // self-closing element — anything else (`>` or an attribute) is a normal - // open tag, which the caller handles separately. - let after_name = &text[start + "`, so a - // `` open tag or `")?; - let end = text.len() - rest.len(); - Some((start, end)) +/// Insert `insertion` (newline-terminated lines) at the start of the line +/// holding `section`'s close tag, so it lands indented like its siblings, or +/// right before the close tag when other markup shares its line (a section +/// opened and closed on one line still receives it inside). `None` for a +/// self-closing section. +fn insert_before_close( + text: &str, + section: &crate::formats::nuget::ConfigSection, + insertion: &str, +) -> Option { + let close = section.close_start?; + let line_start = text[..close].rfind('\n').map(|n| n + 1).unwrap_or(0); + let at = if text[line_start..close].trim().is_empty() { + line_start + } else { + close + }; + let mut out = String::with_capacity(text.len() + insertion.len()); + out.push_str(&text[..at]); + out.push_str(insertion); + out.push_str(&text[at..]); + Some(out) } /// Revert our `nuget.config` wiring. `Ok(true)` = reverted (or would be on dry @@ -1547,12 +1511,30 @@ mod tests { assert_eq!(t.matches("").count(), 1); } + /// A section opened and closed on one line receives the insert inside + /// it: the line-start anchor never reaches back before the open tag. #[test] - fn parse_config_source_keys_reads_adds() { + fn one_line_sections_receive_their_children_inside() { let text = "\ \ "; - assert_eq!(parse_config_source_keys(text), vec!["a", "b"]); + let edit = build_config_edit( + Some(text), + &source_key(), + &format!(".socket/vendor/nuget/{UUID}"), + "Newtonsoft.Json", + ) + .unwrap(); + let parsed = crate::formats::nuget::parse_config(&edit.new_text).unwrap(); + let keys: Vec<&str> = parsed.sources.iter().map(|(k, _)| k.as_str()).collect(); + assert_eq!(keys, ["a", "b", source_key().as_str()], "{}", edit.new_text); + let mapped: Vec<&str> = parsed.mappings.iter().map(|(k, _)| k.as_str()).collect(); + assert_eq!( + mapped, + ["a", "b", source_key().as_str()], + "{}", + edit.new_text + ); } #[test] @@ -1570,9 +1552,11 @@ mod tests { \x20 \n\ \x20 \n\ \n"; - assert_eq!( - parse_config_source_keys(orig), - Vec::::new(), + assert!( + crate::formats::nuget::parse_config(orig) + .unwrap() + .sources + .is_empty(), "a self-closing packageSources carries no source keys" ); let edit = build_config_edit( @@ -2107,7 +2091,11 @@ mod tests { .await .unwrap(); assert!( - parse_config_source_keys(&blank_comments(&rewired)).contains(&source_key()), + crate::formats::nuget::parse_config(&rewired) + .unwrap() + .sources + .iter() + .any(|(key, _)| *key == source_key()), "the re-run wires a live source: {rewired}" ); } @@ -3460,7 +3448,7 @@ mod tests { .error .as_deref() .unwrap_or("") - .contains("no "), + .contains("malformed XML"), "{:?}", result.error ); @@ -3515,10 +3503,10 @@ mod tests { assert!(t.trim_end().ends_with("")); } - /// A config whose only anchor is `` (step 1 lands) but - /// with no `` fails creating the mapping section. + /// A `` outside a `` root is not a section + /// NuGet reads, so it is no anchor: the edit fails on the missing root. #[test] - fn config_without_configuration_close_errs_on_mapping_section() { + fn config_without_configuration_root_errs() { let orig = "\n\n"; let err = build_config_edit( Some(orig), @@ -3527,13 +3515,13 @@ mod tests { "Newtonsoft.Json", ) .err() - .expect("a config without must fail the mapping insert"); - assert!(err.contains("packageSourceMapping section"), "{err}"); + .expect("a config without must fail the edit"); + assert!(err.contains("no to edit"), "{err}"); } - /// No usable anchor at all: fail-closed with the `` error. + /// An unclosed root is malformed XML: fail-closed before any splice. #[test] - fn config_without_any_anchor_errs() { + fn config_with_unclosed_root_errs() { let err = build_config_edit( Some(""), &source_key(), @@ -3542,7 +3530,7 @@ mod tests { ) .err() .expect("an anchorless config must fail the edit"); - assert!(err.contains("no to edit"), "{err}"); + assert!(err.contains("malformed XML"), "{err}"); } // ── marker write failure is a warning, not a failure ─────────────────── @@ -4236,32 +4224,180 @@ mod tests { ); } - // ── comment blanking + key scan edges ────────────────────────────────── + // ── shared-reader edges (formats::nuget::parse_config) ───────────────── + + fn wire(text: &str) -> Result { + build_config_edit( + Some(text), + &source_key(), + &format!(".socket/vendor/nuget/{UUID}"), + "Newtonsoft.Json", + ) + } + /// An unterminated comment, a mismatched close tag or a repeated section + /// has no single live anchor: the writer refuses instead of splicing into + /// whichever copy a substring search finds first. #[test] - fn blank_comments_unterminated_blanks_through_eof() { - let input = "keep \n\ + \x20 \n \n\n", + // commented before the real one + "\n \n \n\ + \x20 \n \n\n", + // commented + "\n \n \n\ + \x20 \n \n\ + \n", + // self-closing sections + "\n \n \n\n", + // single-quoted and spaced attributes, CRLF + "\r\n \r\n \r\n\ + \x20 \r\n\r\n", + // in a lookalike section outside packageSources + "\n \n \n\ + \x20 \n\n", + ]; + for text in cases { + let before = crate::formats::nuget::parse_config(text).unwrap(); + let t = wire(text) + .unwrap_or_else(|e| panic!("{e}: {text:?}")) + .new_text; + let after = crate::formats::nuget::parse_config(&t) + .unwrap_or_else(|| panic!("unparseable output: {t:?}")); + assert!(!after.repeated_sections, "{t}"); + let mut expected: Vec = before.sources.iter().map(|(k, _)| k.clone()).collect(); + if expected.is_empty() + && before + .source_mapping + .is_none_or(|m| m.close_start.is_none()) + { + expected.push(NUGET_ORG_SOURCE_KEY.to_string()); + } + let wired = after + .sources + .iter() + .map(|(k, _)| k.clone()) + .filter(|k| *k != source_key()) + .collect::>(); + let mut wired_sorted = wired.clone(); + wired_sorted.sort(); + let mut expected_sorted = expected.clone(); + expected_sorted.sort(); + assert_eq!(wired_sorted, expected_sorted, "{t}"); + assert!(after.sources.iter().any(|(k, _)| *k == source_key()), "{t}"); + let catch_all: Vec<&str> = after + .mappings + .iter() + .filter(|(_, p)| p == &["*"]) + .map(|(k, _)| k.as_str()) + .collect(); + assert_eq!(catch_all.len(), expected.len(), "{t}"); + assert!( + after + .mappings + .iter() + .any(|(k, p)| *k == source_key() && p == &["Newtonsoft.Json"]), + "{t}" + ); + } } // ── permission-failure unwinds (unix) ────────────────────────────────── @@ -4450,29 +4586,6 @@ mod tests { // ── remaining prod arms ──────────────────────────────────────────────── - /// `attr_value` scanning edges: a substring hit on the attribute NAME - /// (`keyring`) and a malformed unquoted value both advance the scan to the - /// next occurrence instead of aborting the harvest, and a config with no - /// properly quoted attribute at all terminates with `None`. - #[test] - fn attr_value_skips_name_lookalikes_and_unquoted_values() { - // "keyring" contains "key" but is not the attribute: the real quoted - // `key` later in the element must still be harvested. - assert_eq!( - attr_value(" keyring=\"x\" key=\"real\" /", "key").as_deref(), - Some("real") - ); - // An unquoted value (malformed XML NuGet would reject anyway) is not - // harvested; the scan moves on to the next, properly quoted match. - assert_eq!( - attr_value("key=bare key='q2'", "key").as_deref(), - Some("q2") - ); - // Lookalikes only ("keyring", "monkeys") and no quoted value → None, - // not an infinite loop. - assert_eq!(attr_value("keyring monkeys", "key"), None); - } - /// Tier A (service prebuilt) write failure: the served bytes cannot land /// because a regular FILE squats the uuid dir path → the hard /// `vendor_prebuilt_write_failed` refusal, before any wiring. On this From d4dffe7bd9719fc7cdb9fdaeb435a1bcdd385885 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Fri, 9 Oct 2026 12:46:05 -0400 Subject: [PATCH 3/5] Start NuGet fix: nuget-crlf-revert Draft placeholder while the fix is written. Co-Authored-By: Claude Opus 5.5 (1M context) From fb711dfd1bda96074e1b48418dc650926dcb5f2b Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Fri, 9 Oct 2026 13:30:19 -0400 Subject: [PATCH 4/5] Revert vendored NuGet on autocrlf checkouts After a core.autocrlf checkout (Git for Windows' default) nuget.config comes back CRLF. vendor --revert, remove and rollback compared it with the LF text vendor recorded, treated it as drift and left it wired, but had already put packages.lock.json back to the upstream contentHash, so every later restore failed NU1403 while --revert exited 0. The config restore now compares and excises line-ending-insensitively and writes the original back in the checkout's line endings. And the lock pin is only reverted when the config stops routing to the vendored feed: a drift-kept config keeps its lock pin too, so the project stays consistently vendored instead of half-reverted. Fixes #537. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/socket-patch-cli/CLI_CONTRACT.md | 6 +- .../src/vendor/nuget_feed.rs | 230 +++++++++++++++++- 2 files changed, 222 insertions(+), 14 deletions(-) diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index bb4592f4b..1c53ec792 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -817,7 +817,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 13c335489..bbd3aa8e8 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_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::{ @@ -720,10 +721,47 @@ 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() { let restored = match w.kind.as_str() { + LOCK_WIRING_KIND if config_still_routes => { + warnings.push(VendorWarning::new( + "vendor_lock_entry_drifted", + format!( + "{} still routes {} to the vendored feed, so its {PACKAGES_LOCK} 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("") + ), + )); + continue; + } LOCK_WIRING_KIND => { revert_lock_record(&project_root.join(PACKAGES_LOCK), w, dry_run).await } @@ -1107,17 +1145,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()))?; } @@ -1135,8 +1184,9 @@ 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); + let nl = terminator(&live); + let source_add = format!(" {nl}"); + let mapping_block = excise_source_mapping(&live, source_key, nl); if !live.contains(&source_add) && mapping_block.is_none() { // (c) Neither authored element is present verbatim → drift, leave alone. return Ok(false); @@ -1159,16 +1209,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()) } @@ -2265,6 +2331,144 @@ 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: 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. @@ -2321,13 +2525,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] From 5eebde23936411e542f05f2a99b0528df2d59259 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Fri, 9 Oct 2026 14:40:39 -0400 Subject: [PATCH 5/5] Find LF NuGet fragments in a mixed CRLF config Vendor inserts LF lines even into a CRLF nuget.config, which stays mixed until git converts it. A sibling edit then sent the revert down the excision path, where the CRLF-majority spelling missed our LF fragments and drift-kept the package (review on #1342). The excision now tries the LF spelling first, then the file's own terminator. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../src/vendor/nuget_feed.rs | 47 +++++++++++++++++-- 1 file changed, 44 insertions(+), 3 deletions(-) diff --git a/crates/socket-patch-core/src/vendor/nuget_feed.rs b/crates/socket-patch-core/src/vendor/nuget_feed.rs index bbd3aa8e8..6d81a2222 100644 --- a/crates/socket-patch-core/src/vendor/nuget_feed.rs +++ b/crates/socket-patch-core/src/vendor/nuget_feed.rs @@ -1184,9 +1184,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 nl = terminator(&live); - let source_add = format!(" {nl}"); - let mapping_block = excise_source_mapping(&live, source_key, nl); + // 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); @@ -2428,6 +2441,34 @@ mod tests { ); } + /// #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.