From 92630055336f74d898c59a1e2124f7cd907e171f Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 15:00:36 +0000 Subject: [PATCH 1/2] 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/2] 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