Fix Maven 4 implicit subprojects in vendor (#622)

crates/socket-patch-cli/tests/e2e_vendor_jvm_build.rs

@@ -575,6 +575,171 @@ fn maven_reactor_vendor_fresh_checkout_offline_build_and_byte_exact_revert() {
     );
 }
 
+/// A Maven 4 (model 4.1.0) aggregator that lists no subprojects: Maven 4
+/// discovers `a` and `b` itself.
+const IMPLICIT_ROOT_POM: &str = r#"<?xml version="1.0" encoding="UTF-8"?>
+<project xmlns="http://maven.apache.org/POM/4.1.0" root="true">
+  <modelVersion>4.1.0</modelVersion>
+  <groupId>com.example</groupId>
+  <artifactId>root</artifactId>
+  <version>1.0.0</version>
+  <packaging>pom</packaging>
+</project>
+"#;
+
+const IMPLICIT_A_POM: &str = r#"<?xml version="1.0" encoding="UTF-8"?>
+<project xmlns="http://maven.apache.org/POM/4.1.0">
+  <modelVersion>4.1.0</modelVersion>
+  <parent/>
+  <artifactId>a</artifactId>
+  <dependencies>
+    <dependency>
+      <groupId>org.apache.commons</groupId>
+      <artifactId>commons-text</artifactId>
+      <version>1.10.0</version>
+    </dependency>
+  </dependencies>
+</project>
+"#;
+
+const IMPLICIT_B_POM: &str = r#"<?xml version="1.0" encoding="UTF-8"?>
+<project xmlns="http://maven.apache.org/POM/4.1.0">
+  <modelVersion>4.1.0</modelVersion>
+  <parent/>
+  <artifactId>b</artifactId>
+  <dependencies>
+    <dependency>
+      <groupId>com.example</groupId>
+      <artifactId>a</artifactId>
+      <version>${project.version}</version>
+    </dependency>
+  </dependencies>
+</project>
+"#;
+
+/// #622: Maven 4 builds `a` and `b` of a 4.1.0 root that lists no
+/// `<subprojects>`. Vendoring must plan them as the reactor (suffixed
+/// version in `a`, the root pinned), so the developer's own warm `~/.m2`
+/// build, which already holds Central's 1.10.0, resolves the patched jar,
+/// and so does a fresh offline checkout. Before the fix the root was a
+/// reactor of one: `a` kept the base version and built Central's jar.
+#[test]
+#[ignore = "real Maven + Maven Central (fixture); run with --ignored"]
+fn maven_reactor_implicit_subprojects_warm_and_offline_build_and_revert() {
+    const SUITE: &str = "e2e_vendor_jvm_build::maven_reactor_implicit_subprojects";
+    let Some(mvn) = Mvn::detect(SUITE) else {
+        return;
+    };
+    if mvn.major() < 4 {
+        println!(
+            "{SUITE}: SKIP (Maven {} has no subproject discovery)",
+            mvn.version
+        );
+        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;
+    };
+    for (rel, body) in [
+        ("pom.xml", IMPLICIT_ROOT_POM),
+        ("a/pom.xml", IMPLICIT_A_POM),
+        ("b/pom.xml", IMPLICIT_B_POM),
+    ] {
+        let path = proj.join(rel);
+        std::fs::create_dir_all(path.parent().unwrap()).unwrap();
+        std::fs::write(path, body).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("b"))).unwrap(),
+        jar,
+        "pre-vendor `b` resolves Central's jar through the discovered `a`"
+    );
+    for dir in ["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"]),
+        IMPLICIT_A_POM.replace(
+            "<version>1.10.0</version>",
+            &format!("<version>{SV}</version>")
+        ),
+        "the discovered module's declaration is suffixed"
+    );
+    assert_eq!(text(&vendored["b/pom.xml"]), IMPLICIT_B_POM);
+    assert!(
+        text(&vendored["pom.xml"]).contains(&format!("<version>{SV}</version>")),
+        "the root is pinned"
+    );
+
+    // The developer's own tree: `~/.m2` still holds Central's 1.10.0.
+    let out = mvn_classpath(&mvn, &proj, &m2, &settings, &["-o"]);
+    assert!(ok(&out), "warm offline reactor build:\n{}", dump(&out));
+    assert_vendored_on_classpath(&proj.join("a"), &patched, "warm build, module a");
+    assert_vendored_on_classpath(&proj.join("b"), &patched, "warm build, module b");
+    for dir in ["target", "a/target", "b/target", ".mvn/target"] {
+        let _ = std::fs::remove_dir_all(proj.join(dir));
+    }
+
+    let (code, doc, stderr) = socket_in(
+        &proj,
+        &m2,
+        None,
+        &[
+            "vex",
+            "--cwd",
+            proj.to_str().unwrap(),
+            "--product",
+            "pkg:maven/com.example/root@1.0.0",
+        ],
+    );
+    assert_eq!(code, Some(0), "vex: {doc}\n{stderr}");
+    assert_eq!(doc["statements"][0]["status"], "not_affected", "{doc}");
+    let (code, env, stderr) = socket_in(
+        &proj,
+        &m2,
+        None,
+        &[
+            "vendor",
+            "--check",
+            "--json",
+            "--offline",
+            "--cwd",
+            proj.to_str().unwrap(),
+        ],
+    );
+    assert_eq!(code, Some(0), "vendor --check: {env}\n{stderr}");
+
+    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, "fresh build, module a");
+    assert_vendored_on_classpath(&fresh.join("b"), &patched, "fresh build, module b");
+
+    revert(&proj, &m2);
+    assert_restored(&proj, &before);
+    assert!(!proj.join(".mvn").exists(), ".mvn residue");
+}
+
 /// #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

crates/socket-patch-core/src/vendor/jvm/apply.rs

@@ -30,7 +30,7 @@ use super::super::state::{VendorEntry, WiringAction, WiringRecord};
 use super::super::{RevertOpts, RevertOutcome, VendorWarning};
 use super::{
     coursier_tree, gradle, layout, maven_reactor, op_of, op_str, sbt, scala_cli, sha256_hex,
-    Coords, JvmPlan, JvmUnplan, ReadFn, Shape, CONFIG_LINE_KIND, COURSIER_INDEX_KIND,
+    Coords, JvmPlan, JvmUnplan, ListFn, ReadFn, Shape, CONFIG_LINE_KIND, COURSIER_INDEX_KIND,
     CREATED_DIR_KIND, DERIVED_METADATA_KIND, KINDS, OWNED_FILE_KIND, POM_FRAGMENT_KIND,
     SBT_FRAGMENT_KIND, SETTINGS_FRAGMENT_KIND, TREE_KIND, VERIFICATION_FRAGMENT_KIND,
 };
@@ -198,15 +198,31 @@ impl ProjectReader {
     /// The children of the project directory `rel` (`""` = the root) for
     /// the script graph: names, directories ending in `/`. Resolved like
     /// [`Self::read`]; anything unsafe or unreadable lists as empty.
+    ///
+    /// A directory reached through a symlink that leaves the checkout is
+    /// only peeked at: its files are listed, its subdirectories are not,
+    /// and no escape is recorded. A probe (Maven 4 subproject discovery
+    /// asking whether a linked `target/` holds a `pom.xml`) then refuses
+    /// nothing for a link it never reads through, while reading any file
+    /// under the link still records the escape the caller refuses.
     pub fn list(&self, rel: &str) -> Vec<String> {
-        let path = if rel.is_empty() {
+        let (path, outside) = if rel.is_empty() {
             match &self.canonical {
-                Some(c) => c.clone(),
+                Some(c) => (c.clone(), false),
                 None => return Vec::new(),
             }
         } else {
-            match self.resolve(rel.trim_end_matches('/')) {
-                Ok(path) => path,
+            let rel = rel.trim_end_matches('/');
+            let before = self.escaped.borrow().clone();
+            match self.resolve(rel) {
+                Ok(path) => (path, false),
+                Err(e) if e.contains(LEAVES_PROJECT) => {
+                    *self.escaped.borrow_mut() = before;
+                    match self.outside_target(rel) {
+                        Some(target) => (target, true),
+                        None => return Vec::new(),
+                    }
+                }
                 Err(_) => return Vec::new(),
             }
         };
@@ -219,13 +235,28 @@ impl ProjectReader {
                 // Followed like `read` (in-checkout links only).
                 let dir = e.metadata().ok()?.is_dir()
                     || (e.file_type().ok()?.is_symlink() && e.path().is_dir());
-                Some(if dir { format!("{name}/") } else { name })
+                match (dir, outside) {
+                    (true, true) => None,
+                    (true, false) => Some(format!("{name}/")),
+                    (false, _) => Some(name),
+                }
             })
             .collect();
         out.sort();
         out
     }
 
+    /// The directory a project-relative `rel` names once its symlinks are
+    /// followed wherever they lead (for [`Self::list`]'s peek). `None` for
+    /// an unsafe path or one that does not resolve to a directory.
+    fn outside_target(&self, rel: &str) -> Option<PathBuf> {
+        if !safe_rel(rel) {
+            return None;
+        }
+        let target = std::fs::canonicalize(self.canonical.as_ref()?.join(rel)).ok()?;
+        target.is_dir().then_some(target)
+    }
+
     /// The first path that resolved outside the checkout.
     pub fn escaped(&self) -> Option<String> {
         self.escaped.borrow().clone()
@@ -269,7 +300,7 @@ impl ProjectReader {
                         let at = segments[..=i].join("/");
                         self.escaped.borrow_mut().get_or_insert(at.clone());
                         return Err(format!(
-                            "{at} is a symlink that leaves the project (or dangles)"
+                            "{at} is a symlink that {LEAVES_PROJECT} (or dangles)"
                         ));
                     };
                     cur = target;
@@ -281,6 +312,9 @@ impl ProjectReader {
     }
 }
 
+/// How [`ProjectReader`] words a symlink resolving outside the checkout.
+const LEAVES_PROJECT: &str = "leaves the project";
+
 /// A path that does not exist, including one that runs through a regular
 /// file (`pom.xml/pom.xml`, probed when a `relativePath` or `<module>`
 /// names a pom file rather than its directory, #534).
@@ -593,6 +627,7 @@ pub async fn revert(root: &Path, entry: &VendorEntry, opts: RevertOpts) -> Rever
     }
     let reader = ProjectReader::new(root);
     let read = |rel: &str| reader.read(rel);
+    let list = |dir: &str| reader.list(dir);
     let peers = match super::super::state::load_state(root).await {
         Ok(state) => state,
         Err(e) => return RevertOutcome::failed(format!("vendor_state_unreadable: {e}")),
@@ -625,7 +660,10 @@ pub async fn revert(root: &Path, entry: &VendorEntry, opts: RevertOpts) -> Rever
             unplan = merge_unplans(unplan, gradle::unplan(&read, &c, &records));
         }
         if maven {
-            unplan = merge_unplans(unplan, maven_reactor::unplan(&read, &c, &entry.wiring));
+            unplan = merge_unplans(
+                unplan,
+                maven_reactor::unplan(&read, &list, &c, &entry.wiring),
+            );
         }
         unplan
     };
@@ -837,13 +875,14 @@ pub fn entry_references(root: &Path, entry: &VendorEntry) -> bool {
     };
     let reader = ProjectReader::new(root);
     let read = |rel: &str| reader.read(rel);
+    let list = |dir: &str| reader.list(dir);
     let referenced = if sbt::owns(&entry.wiring) {
         sbt::wired_checked(&read, &c).unwrap_or(true)
     } else if scala_cli::owns(&entry.wiring) {
         scala_cli::wired_checked(&read, &c).unwrap_or(true)
     } else {
         let (maven, gradle) = sides(&entry.wiring);
-        (maven && maven_reactor::wired_checked(&read, &c).unwrap_or(true))
+        (maven && maven_reactor::wired_checked(&read, &list, &c).unwrap_or(true))
             || (gradle && gradle::references_checked(&read, &c).unwrap_or(true))
     };
     // `read` answers `None` for an unreadable file as for a missing one;
@@ -878,7 +917,7 @@ pub fn entry_wired_checked(root: &Path, entry: &VendorEntry) -> Result<bool, Str
             wired = gradle::wired_checked(&read, &list, &c).map_err(|e| e.detail);
         }
         if maven && wired == Ok(true) {
-            wired = maven_reactor::attestable_checked(&read, &c).map_err(|e| e.detail);
+            wired = maven_reactor::attestable_checked(&read, &list, &c).map_err(|e| e.detail);
         }
         wired
     };
@@ -892,12 +931,13 @@ pub fn entry_wired_checked(root: &Path, entry: &VendorEntry) -> Result<bool, Str
 /// repository `local_repo` alone (`None` entries: not there).
 fn local_external_poms(
     read: ReadFn<'_>,
+    list: ListFn<'_>,
     patch: &super::JvmPatch<'_>,
     local_repo: Option<&Path>,
 ) -> maven_reactor::ExternalPoms {
     let mut known = maven_reactor::ExternalPoms::new();
     loop {
-        let need = maven_reactor::external_poms_needed(read, patch, &known);
+        let need = maven_reactor::external_poms_needed(read, list, patch, &known);
         if need.is_empty() {
             return known;
         }
@@ -1000,7 +1040,7 @@ pub fn check_entry(
     // sync: the pin it wrote when it could read it, or none.
     let shape = shape_of(&entry.wiring);
     let external = if matches!(shape, Shape::MavenReactor | Shape::Mixed) {
-        Some(local_external_poms(&read, &patch, local_repo))
+        Some(local_external_poms(&read, &list, &patch, local_repo))
     } else {
         None
     };
@@ -1020,7 +1060,7 @@ pub fn check_entry(
     let referenced = if sbt_entry || scala_cli_entry {
         entry_wired_checked(root, entry)?
     } else {
-        (!maven || maven_reactor::wired_checked(&read, &c).map_err(|e| e.detail)?)
+        (!maven || maven_reactor::wired_checked(&read, &list, &c).map_err(|e| e.detail)?)
             && (!gradle || gradle::references(&read, &c))
     };
     if !referenced {

crates/socket-patch-core/src/vendor/jvm/gradle.rs

@@ -2443,6 +2443,7 @@ fn line_start(text: &str, at: usize) -> usize {
 mod tests {
     use std::collections::BTreeMap;
 
+    use super::super::testing::list_of;
     use super::super::{apply, detect, Shape};
     use super::*;
 
@@ -2495,29 +2496,6 @@ mod tests {
         plan(&read, &|dir: &str| list_of(files, dir), p)
     }
 
-    /// The children of `dir` in an in-memory tree (the planner's ListFn).
-    fn list_of(files: &BTreeMap<String, Vec<u8>>, dir: &str) -> Vec<String> {
-        let prefix = if dir.is_empty() {
-            String::new()
-        } else {
-            format!("{}/", dir.trim_end_matches('/'))
-        };
-        let mut out: Vec<String> = Vec::new();
-        for key in files.keys() {
-            let Some(rest) = key.strip_prefix(&prefix) else {
-                continue;
-            };
-            let child = match rest.find('/') {
-                Some(i) => format!("{}/", &rest[..i]),
-                None => rest.to_string(),
-            };
-            if !out.contains(&child) {
-                out.push(child);
-            }
-        }
-        out
-    }
-
     fn vendored_line(kotlin: bool, prefix: &str) -> String {
         apply_line(&WiringTarget::vendored(), kotlin, prefix)
     }