From 2362a4a6abdc94116317f9fa3d57148f7cd3bb4d Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 07:22:39 +0000 Subject: [PATCH 1/3] Start fix for #655 Assisted-by: Claude Code:claude-opus-5-5 From 821bb2b929f2473cbc5ccfdf623699f26d985c1c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 07:33:34 +0000 Subject: [PATCH 2/3] Patch Maven deps named via ${prop} coordinates A reactor module that declares the patched library as ${ct.group} / ${ct.artifact} was skipped by the vendored Maven planner, which compared the raw element text with the patch coordinates. Maven interpolates those elements before matching, so the module kept its literal base version and built Central's unpatched jar while vendor, vendor --check and vex all reported it patched. The planner, the external-management check and the revert now match declarations on interpolated coordinates. A coordinate whose property is not defined in the checkout gets a property_unresolved warning instead of passing silently. Fixes #655 Assisted-by: Claude Code:claude-opus-5-5 --- .../tests/e2e_vendor_jvm_build.rs | 84 ++++++++++ .../src/vendor/jvm/maven_reactor.rs | 156 ++++++++++++++++-- 2 files changed, 229 insertions(+), 11 deletions(-) diff --git a/crates/socket-patch-cli/tests/e2e_vendor_jvm_build.rs b/crates/socket-patch-cli/tests/e2e_vendor_jvm_build.rs index 2e1f00754..36abe8385 100644 --- a/crates/socket-patch-cli/tests/e2e_vendor_jvm_build.rs +++ b/crates/socket-patch-cli/tests/e2e_vendor_jvm_build.rs @@ -544,6 +544,90 @@ fn maven_reactor_vendor_fresh_checkout_offline_build_and_byte_exact_revert() { ); } +/// #655: `a` declares the library through `${ct.group}:${ct.artifact}`, +/// properties the local corp parent defines. Maven interpolates them before +/// matching, so vendor must rewrite `a`'s literal version too, or `a` builds +/// Central's unpatched jar. +#[test] +#[ignore = "real Maven + Maven Central (fixture); run with --ignored"] +fn maven_reactor_property_coordinates_vendor_fresh_offline_build_and_revert() { + const SUITE: &str = "e2e_vendor_jvm_build::maven_reactor_property_coordinates"; + let Some(mvn) = Mvn::detect(SUITE) else { + return; + }; + let tmp = tempfile::tempdir().unwrap(); + let root = fixture_root(&tmp); + let m2 = root.join("m2"); + let proj = root.join("proj"); + let settings = root.join("settings.xml"); + write_settings(&settings, &[]); + let Some((jar, _)) = warm_fixture(SUITE, &mvn, &root.join("warm"), &m2, &settings) else { + return; + }; + write_reactor(&proj); + let corp = CORP_PARENT_POM.replace( + " ", + " org.apache.commons\n \ + commons-text\n ", + ); + std::fs::write(proj.join("corp-parent/pom.xml"), corp).unwrap(); + let a = a_pom() + .replace( + "org.apache.commons", + "${ct.group}", + ) + .replace( + "commons-text", + "${ct.artifact}", + ); + std::fs::write(proj.join("a/pom.xml"), &a).unwrap(); + let out = with_central_fallback(SUITE, "pre-vendor reactor build", &settings, |s, flags| { + mvn_classpath(&mvn, &proj, &m2, s, flags) + }); + assert!(ok(&out), "pre-vendor reactor build:\n{}", dump(&out)); + assert_eq!( + std::fs::read(classpath_entry(&proj.join("a"))).unwrap(), + jar, + "pre-vendor `a` resolves Central's jar through its property coordinates" + ); + for dir in [ + "target", + "corp-parent/target", + "a/target", + "b/target", + ".mvn/target", + ] { + let _ = std::fs::remove_dir_all(proj.join(dir)); + } + let _ = std::fs::remove_dir(proj.join(".mvn")); + + let (orig, patched) = patched_member(&jar, UUID); + stage_manifest(&proj, &orig, &patched); + let before = snapshot(&proj); + let env = vendor(&proj, &m2); + assert_eq!(env["summary"]["applied"], 1, "{env}"); + let vendored = snapshot(&proj); + assert_eq!( + text(&vendored["a/pom.xml"]), + a.replace( + "1.10.0", + &format!("{SV}") + ), + "`a`'s literal version is rewritten" + ); + + let fresh = root.join("fresh"); + fresh_checkout_all(&proj, &fresh); + purge(&m2); + let out = mvn_classpath(&mvn, &fresh, &m2, &settings, &["-o"]); + assert!(ok(&out), "fresh offline reactor build:\n{}", dump(&out)); + assert_vendored_on_classpath(&fresh.join("a"), &patched, "module a"); + assert_vendored_on_classpath(&fresh.join("b"), &patched, "module b"); + + revert(&proj, &m2); + assert_restored(&proj, &before); +} + // ── P2: Gradle multi-project with dependency locking ──────────────────── const GRADLE_SETTINGS: &str = r#"buildscript { diff --git a/crates/socket-patch-core/src/vendor/jvm/maven_reactor.rs b/crates/socket-patch-core/src/vendor/jvm/maven_reactor.rs index db447fd6c..067769e40 100644 --- a/crates/socket-patch-core/src/vendor/jvm/maven_reactor.rs +++ b/crates/socket-patch-core/src/vendor/jvm/maven_reactor.rs @@ -350,8 +350,9 @@ pub fn unplan(read: ReadFn<'_>, c: &Coords<'_>, records: &[WiringRecord]) -> Jvm .filter(|w| w.kind == POM_FRAGMENT_KIND) .map(|w| w.file.clone()) .collect(); - if let Ok(reactor) = Reactor::discover(read) { - files.extend(reactor.scope); + let reactor = Reactor::discover(read).ok(); + if let Some(reactor) = &reactor { + files.extend(reactor.scope.iter().cloned()); } let mut before: BTreeMap> = files .into_iter() @@ -369,7 +370,7 @@ pub fn unplan(read: ReadFn<'_>, c: &Coords<'_>, records: &[WiringRecord]) -> Jvm .filter(|w| w.kind == POM_FRAGMENT_KIND && w.file == *rel) .collect(); *t = cut_pin(rel, t, c, &recs, &mut drifted); - *t = restore_versions(rel, t, c, &sv, &recs, &mut drifted); + *t = restore_versions(rel, t, c, &sv, &recs, reactor.as_ref(), &mut drifted); for w in recs .iter() .filter(|w| w.key.as_deref() == Some("pin_section")) @@ -530,13 +531,15 @@ fn cut_pin( } /// `text` with every g:a declaration at `sv` back at its recorded original -/// (else the base version). +/// (else the base version). `reactor` interpolates `${...}` coordinates in +/// `rel`'s context, as the plan matched them. fn restore_versions( rel: &str, text: &str, c: &Coords<'_>, sv: &str, recs: &[&WiringRecord], + reactor: Option<&Reactor>, drifted: &mut Vec, ) -> String { if !text.contains(sv) { @@ -548,8 +551,13 @@ fn restore_versions( )); return text.to_string(); }; + let resolve = |v: &str| { + reactor + .filter(|r| r.poms.contains_key(rel)) + .and_then(|r| r.interpolate(rel, v, 0)) + }; let mut edits = Vec::new(); - for (dep, key) in doc.keyed_declarations(c.group_id, c.artifact_id) { + for (dep, key) in doc.keyed_declarations(c.group_id, c.artifact_id, &resolve) { let Some(version) = doc.child(dep, "version") else { continue; }; @@ -911,7 +919,7 @@ impl Reactor { 'modules: for rel in self.scope.iter().filter(|rel| self.local_root(rel) == root) { let locally_versioned = self.chain(rel).any(|p| { let doc = &self.poms[p].doc; - doc.keyed_declarations(g, a) + self.keyed_declarations(p, g, a) .iter() .any(|(dep, _)| doc.child(*dep, "version").is_some()) }); @@ -1497,6 +1505,14 @@ impl Reactor { roots } + /// The g:a declarations of `rel`, with `${...}` coordinates + /// interpolated in its context. + fn keyed_declarations(&self, rel: &str, g: &str, a: &str) -> Vec<(usize, String)> { + self.poms[rel] + .doc + .keyed_declarations(g, a, &|v| self.interpolate(rel, v, 0)) + } + /// Interpolate every `${name}` in `value` in the context of `rel`; /// `None` when any name is undefined in the checkout. fn interpolate(&self, rel: &str, value: &str, depth: usize) -> Option { @@ -1564,7 +1580,8 @@ impl Reactor { ) { let doc = &self.poms[rel].doc; let ga = format!("{}:{}", patch.group_id, patch.artifact_id); - for (dep, key) in doc.keyed_declarations(patch.group_id, patch.artifact_id) { + self.warn_unresolved_coordinates(rel, patch, warnings); + for (dep, key) in self.keyed_declarations(rel, patch.group_id, patch.artifact_id) { if doc .child_text(dep, "type") .is_some_and(|t| !t.is_empty() && t != "jar") @@ -1663,6 +1680,54 @@ impl Reactor { } } +impl Reactor { + /// Warn for each declaration of `rel` that could be `patch`'s g:a but + /// names a `${...}` groupId or artifactId undefined in the checkout: + /// it is left as is, probably unpatched. + fn warn_unresolved_coordinates( + &self, + rel: &str, + patch: &JvmPatch<'_>, + warnings: &mut Vec, + ) { + let doc = &self.poms[rel].doc; + let resolve = |v: &str| self.interpolate(rel, v, 0); + for dep in doc.declarations() { + let (Some(g), Some(a)) = ( + doc.child_text(dep, "groupId"), + doc.child_text(dep, "artifactId"), + ) else { + continue; + }; + let unresolved: Vec<&str> = [&g, &a] + .into_iter() + .filter(|c| c.contains("${") && resolve(c).is_none()) + .map(String::as_str) + .collect(); + let could_be = |raw: &str, want: &str| { + raw == want || (raw.contains("${") && resolve(raw).is_none_or(|v| v == want)) + }; + if unresolved.is_empty() + || !could_be(&g, patch.group_id) + || !could_be(&a, patch.artifact_id) + { + continue; + } + let at = format!("{rel}:{}", doc.line_of(doc.nodes[dep].start)); + warnings.push(degraded( + "property_unresolved", + format!( + "{at}: {g}:{a} may be {}:{} but {} is not defined in the checkout; \ + left as is, probably unpatched", + patch.group_id, + patch.artifact_id, + unresolved.join(", ") + ), + )); + } + } +} + /// The base version, or a suffixed version of it from any patch uuid. fn is_base_like(version: &str, base: &str) -> bool { version == base @@ -2487,13 +2552,19 @@ impl Doc { } /// The g:a declarations that are not pins, with their record key - /// `version:::
:`. - fn keyed_declarations(&self, g: &str, a: &str) -> Vec<(usize, String)> { + /// `version:::
:`. `resolve` interpolates a + /// `${...}` groupId or artifactId, which Maven does before matching. + fn keyed_declarations( + &self, + g: &str, + a: &str, + resolve: &dyn Fn(&str) -> Option, + ) -> Vec<(usize, String)> { let mut seen: BTreeMap = BTreeMap::new(); let mut out = Vec::new(); for dep in self.declarations() { - if self.child_text(dep, "groupId").as_deref() != Some(g) - || self.child_text(dep, "artifactId").as_deref() != Some(a) + if self.coordinate(dep, "groupId", resolve).as_deref() != Some(g) + || self.coordinate(dep, "artifactId", resolve).as_deref() != Some(a) || self.pin_at(dep).is_some() { continue; @@ -2506,6 +2577,22 @@ impl Doc { out } + /// The `name` child of `dep`, interpolated through `resolve` when it + /// holds a `${...}` expression; `None` when that is undefined. + fn coordinate( + &self, + dep: usize, + name: &str, + resolve: &dyn Fn(&str) -> Option, + ) -> Option { + let raw = self.child_text(dep, name)?; + if raw.contains("${") { + resolve(&raw) + } else { + Some(raw) + } + } + /// `dependencies` / `dependencyManagement`, prefixed with /// `profile::` inside a profile. fn section_of(&self, dep: usize) -> String { @@ -4618,6 +4705,53 @@ mod tests { assert_eq!(a.1.as_deref(), Some(module("a", cdata).as_bytes())); } + #[test] + fn property_coordinates_are_interpolated_and_restored() { + // #655: Maven interpolates groupId/artifactId before matching, so a + // `${ct.group}:${ct.artifact}` declaration is the patched artifact. + let root = ROOT.replace( + " ", + " \n org.apache.commons\n \ + commons-text\n \n ", + ); + let decl = "${ct.group}\ + ${ct.artifact}1.10.0"; + let a = module("a", decl); + let files = fs(&[ + ("pom.xml", &root), + ("a/pom.xml", &a), + ("b/pom.xml", &module("b", "")), + ]); + let (plan, after) = vendor(&files); + assert!(plan.warnings.is_empty(), "{:?}", plan.warnings); + assert_eq!(text(&after, "a/pom.xml"), a.replace("1.10.0", SV)); + let read = |rel: &str| after.get(rel).cloned(); + let undo = unplan(&read, &patch().coords(), &plan.records); + assert!(undo.drifted.is_empty(), "{:?}", undo.drifted); + let a_undo = undo.changes.iter().find(|(r, _)| r == "a/pom.xml").unwrap(); + assert_eq!(a_undo.1.as_deref(), Some(a.as_bytes())); + } + + #[test] + fn unresolvable_property_coordinate_warns() { + let decl = "${remote.group}\ + commons-text1.10.0"; + let a = module("a", decl); + let files = fs(&[ + ("pom.xml", ROOT), + ("a/pom.xml", &a), + ("b/pom.xml", &module("b", "")), + ]); + let (plan, after) = vendor(&files); + assert_eq!(reasons(&plan), ["property_unresolved"]); + assert!( + plan.warnings[0].detail.contains("${remote.group}"), + "{}", + plan.warnings[0].detail + ); + assert_eq!(text(&after, "a/pom.xml"), a); + } + #[test] fn a_comment_before_the_anchor_is_not_indentation() { let root = ROOT.replace(" ", " "); From 16e930e92c3cad504fca623f7ed1f72a4ce4da2d Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 08:15:04 +0000 Subject: [PATCH 3/3] Guard ${prop} Maven coordinates edge cases Follow-ups from review of the #655 fix: - A module that inherits a ${prop}-named declaration and overrides the property names a different artifact; vendor now leaves the declaration alone and warns coordinates_overridden instead of pointing that module at a version that does not exist. - Declarations matched through properties get their own record-key ordinals, so revert of a project vendored by an older release still finds each literal declaration's original version. - ${project.groupId} and ${project.artifactId} resolve. - The unresolved-coordinate warning respects literal text around the property and skips classifier and non-jar variants, so unrelated dependencies no longer warn. Assisted-by: Claude Code:claude-opus-5-5 --- .../src/vendor/jvm/maven_reactor.rs | 184 +++++++++++++++++- 1 file changed, 180 insertions(+), 4 deletions(-) diff --git a/crates/socket-patch-core/src/vendor/jvm/maven_reactor.rs b/crates/socket-patch-core/src/vendor/jvm/maven_reactor.rs index 067769e40..7918d0b9a 100644 --- a/crates/socket-patch-core/src/vendor/jvm/maven_reactor.rs +++ b/crates/socket-patch-core/src/vendor/jvm/maven_reactor.rs @@ -1542,7 +1542,12 @@ impl Reactor { "project.version" | "pom.version" => { return pom.effective_version().map(str::to_string); } + "project.groupId" | "pom.groupId" => { + return pom.effective_group().map(str::to_string); + } + "project.artifactId" | "pom.artifactId" => return pom.artifact.clone(), "project.parent.version" => return pom.parent.as_ref()?.version.clone(), + "project.parent.groupId" => return pom.parent.as_ref()?.group.clone(), _ => {} } if let Some(v) = self.cli_props.get(name) { @@ -1599,8 +1604,30 @@ impl Reactor { )); continue; } - let top_managed = doc.is_top_level_managed(dep); let root = self.local_root(rel); + // Maven interpolates after inheritance: a pom that inherits this + // declaration may name another artifact through it, which the + // literal SV must not reach. + if doc.has_property_coordinate(dep) { + let other = self.inheritors(rel).find(|p| { + let resolve = |v: &str| self.interpolate(p, v, 0); + doc.coordinate(dep, "groupId", &resolve).as_deref() != Some(patch.group_id) + || doc.coordinate(dep, "artifactId", &resolve).as_deref() + != Some(patch.artifact_id) + }); + if let Some(p) = other { + warnings.push(degraded( + "coordinates_overridden", + format!( + "{at}: {ga} is named through properties that {p} resolves \ + differently; left as is and {root} is not pinned" + ), + )); + unpinned.insert(root); + continue; + } + } + let top_managed = doc.is_top_level_managed(dep); if top_managed && root == rel { managed_roots.insert(root.clone()); } @@ -1704,10 +1731,18 @@ impl Reactor { .filter(|c| c.contains("${") && resolve(c).is_none()) .map(String::as_str) .collect(); - let could_be = |raw: &str, want: &str| { - raw == want || (raw.contains("${") && resolve(raw).is_none_or(|v| v == want)) + let could_be = |raw: &str, want: &str| match resolve(raw) { + Some(v) => v == want, + None => may_interpolate_to(raw, want), }; + let other_variant = doc + .child_text(dep, "type") + .is_some_and(|t| !t.is_empty() && t != "jar") + || doc + .child_text(dep, "classifier") + .is_some_and(|c| !c.is_empty()); if unresolved.is_empty() + || other_variant || !could_be(&g, patch.group_id) || !could_be(&a, patch.artifact_id) { @@ -1728,6 +1763,33 @@ impl Reactor { } } +/// Whether `raw`, with each `${...}` standing for any text, can read +/// `want`. +fn may_interpolate_to(raw: &str, want: &str) -> bool { + let mut literals = Vec::new(); + let mut rest = raw; + while let Some(at) = rest.find("${") { + literals.push(&rest[..at]); + let Some(close) = rest[at..].find('}') else { + return false; + }; + rest = &rest[at + close + 1..]; + } + let Some((first, tail)) = literals.split_first() else { + return raw == want; + }; + let Some(mut left) = want.strip_prefix(first) else { + return false; + }; + for middle in tail { + let Some(i) = left.find(middle) else { + return false; + }; + left = &left[i + middle.len()..]; + } + left.ends_with(rest) +} + /// The base version, or a suffixed version of it from any patch uuid. fn is_base_like(version: &str, base: &str) -> bool { version == base @@ -2569,7 +2631,13 @@ impl Doc { { continue; } - let section = self.section_of(dep); + // Declarations named through properties count apart, so literal + // declarations keep the keys of records written before they + // were matched. + let mut section = self.section_of(dep); + if self.has_property_coordinate(dep) { + section.push_str(":$"); + } let ordinal = seen.entry(section.clone()).or_default(); out.push((dep, format!("version:{g}:{a}:{section}:{ordinal}"))); *ordinal += 1; @@ -2593,6 +2661,13 @@ impl Doc { } } + /// Whether `dep` names its groupId or artifactId through `${...}`. + fn has_property_coordinate(&self, dep: usize) -> bool { + ["groupId", "artifactId"] + .iter() + .any(|n| self.child_text(dep, n).is_some_and(|t| t.contains("${"))) + } + /// `dependencies` / `dependencyManagement`, prefixed with /// `profile::` inside a profile. fn section_of(&self, dep: usize) -> String { @@ -4752,6 +4827,107 @@ mod tests { assert_eq!(text(&after, "a/pom.xml"), a); } + #[test] + fn property_coordinates_keep_literal_record_keys() { + // Records written before #655 keyed the literal declaration as + // ordinal 0; a `${...}` declaration ahead of it must not shift that. + let deps = format!( + "${{ct.group}}\ + commons-text1.10.0{}", + dep("1.10.0") + ); + let doc = Doc::parse(module("a", &deps)).unwrap(); + let resolve = |_: &str| Some("org.apache.commons".to_string()); + let keys: Vec = doc + .keyed_declarations("org.apache.commons", "commons-text", &resolve) + .into_iter() + .map(|(_, k)| k) + .collect(); + assert_eq!( + keys, + [ + "version:org.apache.commons:commons-text:dependencies:$:0", + "version:org.apache.commons:commons-text:dependencies:0", + ] + ); + } + + #[test] + fn property_coordinates_another_module_overrides_are_left_alone() { + let root = ROOT.replace( + " ", + " \n commons-text\n \n \ + org.apache.commons\ + ${lib.artifact}1.10.0\ + \n ", + ); + let b = module("b", "").replace( + " ", + " commons-lang3\n ", + ); + let files = fs(&[ + ("pom.xml", &root), + ("a/pom.xml", &module("a", "")), + ("b/pom.xml", &b), + ]); + let plan = run(&files).expect("plan"); + assert_eq!(reasons(&plan), ["coordinates_overridden"]); + let after = applied(&files, &plan); + let out = text(&after, "pom.xml"); + assert!(!out.contains(SV), "{out}"); + assert!( + out.contains("${lib.artifact}1.10.0"), + "{out}" + ); + } + + #[test] + fn project_group_id_coordinate_resolves() { + let root = ROOT.replace( + "t", + "org.apache.commons", + ); + let decl = "${project.groupId}\ + commons-text1.10.0"; + let a = module("a", decl).replace( + "t", + "org.apache.commons", + ); + let b = module("b", "").replace( + "t", + "org.apache.commons", + ); + let files = fs(&[("pom.xml", &root), ("a/pom.xml", &a), ("b/pom.xml", &b)]); + let (plan, after) = vendor(&files); + assert!(plan.warnings.is_empty(), "{:?}", plan.warnings); + assert_eq!(text(&after, "a/pom.xml"), a.replace("1.10.0", SV)); + } + + #[test] + fn unresolvable_coordinate_of_another_artifact_does_not_warn() { + let decls = "org.apache.commons\ + ${remote.x}-lang33.0\ + ${remote.group}commons-text\ + sources1.10.0"; + let files = fs(&[ + ("pom.xml", ROOT), + ("a/pom.xml", &module("a", decls)), + ("b/pom.xml", &module("b", "")), + ]); + let (plan, _) = vendor(&files); + assert!(plan.warnings.is_empty(), "{:?}", plan.warnings); + } + + #[test] + fn may_interpolate_to_keeps_literal_text() { + assert!(may_interpolate_to("${g}", "org.apache.commons")); + assert!(may_interpolate_to("org.${x}.commons", "org.apache.commons")); + assert!(may_interpolate_to("${a}-${b}", "commons-text")); + assert!(!may_interpolate_to("${x}-lang3", "commons-text")); + assert!(!may_interpolate_to("com.${x}", "org.apache.commons")); + assert!(!may_interpolate_to("${x", "commons-text")); + } + #[test] fn a_comment_before_the_anchor_is_not_indentation() { let root = ROOT.replace(" ", " ");