Merge remote-tracking branch 'origin/agent/v5-nuget-member-locks' into agent/v5-nuget-gpf-shadow

Author: mikolalysenko

crates/socket-patch-core/src/hosted/engine.rs

@@ -633,32 +633,32 @@ pub async fn read_candidate_files(
     }
 
     // NuGet: the root config routes every project under the root, so each
-    // project's lock is pinned with it (#353, #514). The project files ride
-    // along as advisory input (never rewritten) so the pure rewriter
-    // re-derives which keys are locks; a walk that cannot see the whole
-    // tree rides a synthetic key and the rewriter skips the redirect.
-    // NuGet is disk-only (the in-memory engine refuses it).
+    // project's lock is pinned with it (#353, #514). The walk's answer rides
+    // a synthetic key (the lock paths, an unevaluable NuGetLockFilePath, or
+    // why the tree could not be listed) and every lock is read. The project
+    // files themselves stay out of the candidate texts. NuGet is disk-only
+    // (the in-memory engine refuses it).
     if candidates.iter().any(|c| c.dep.ecosystem == "nuget") {
         if let Some(root) = view.disk_root() {
-            match crate::vendor::nuget_config::project_files(root) {
-                Ok(projects) => {
-                    let governed = crate::formats::nuget::lock::governed_locks(&projects, |rel| {
-                        crate::vendor::nuget_config::lock_present(root, rel)
-                    });
-                    for (rel, text) in projects {
-                        out.files.entry(rel).or_insert(text);
+            let mut lines: Vec<String> = Vec::new();
+            match crate::vendor::nuget_config::governed_locks_on_disk(root) {
+                Ok(governed) => {
+                    for (project, detail) in &governed.unresolved {
+                        lines.push(format!("unresolved\t{project}\t{detail}"));
                     }
                     for rel in governed.locks {
                         if !out.files.contains_key(&rel) {
                             out.read(view, unreadable, &rel).await;
                         }
+                        lines.push(format!("lock\t{rel}"));
                     }
                 }
-                Err(why) => {
-                    out.files
-                        .insert(crate::patch::redirect::NUGET_LOCK_WALK_KEY.to_string(), why);
-                }
+                Err(why) => lines.push(format!("error\t{why}")),
             }
+            out.files.insert(
+                crate::patch::redirect::NUGET_LOCKS_KEY.to_string(),
+                lines.join("\n"),
+            );
         }
     }
 

crates/socket-patch-core/src/patch/redirect/mod.rs

@@ -5760,11 +5760,13 @@ fn nuget_xml_attribute(value: &str) -> String {
         .replace('\r', "
")
 }
 
-/// The synthetic candidate key carrying why the engine could not list the
-/// project's NuGet locks (an unreadable directory or project file): the
-/// rewriter then skips the nuget redirect rather than leave a lock it never
-/// saw on its upstream hash. Never a path (see [`sbt::SYNTHETIC_KEY_PREFIX`]).
-pub const NUGET_LOCK_WALK_KEY: &str = "<socket-patch:nuget-lock-walk>";
+/// The synthetic candidate key carrying the locks the engine found every
+/// project under the root restoring into (#353, #514), one per line:
+/// `lock\t<rel>`, `unresolved\t<project>\t<why>` for a `NuGetLockFilePath`
+/// it cannot evaluate, or `error\t<why>` when it could not list the tree.
+/// Absent (the in-memory engine, unit tests), the root `packages.lock.json`
+/// is the one lock. Never a path (see [`sbt::SYNTHETIC_KEY_PREFIX`]).
+pub const NUGET_LOCKS_KEY: &str = "<socket-patch:nuget-locks>";
 
 fn rewrite_nuget(
     files: &BTreeMap<String, String>,
@@ -5808,41 +5810,45 @@ fn rewrite_nuget(
     // routes them all, so each is pinned with it (#353, #514). The engine
     // reads the project files and their locks; the same pure discovery
     // re-derives which keys are locks here.
-    if let Some(why) = files.get(NUGET_LOCK_WALK_KEY) {
-        result.warnings.push(RewriteWarning {
-            code: "redirect_nuget_lock_unreadable".into(),
-            detail: format!(
-                "cannot list the project's NuGet locks ({why}); nuget redirect skipped"
-            ),
-        });
-        return;
-    }
-    let projects: Vec<(String, String)> = files
-        .iter()
-        .filter(|(rel, _)| {
-            !sbt::is_synthetic_key(rel)
-                && crate::formats::nuget::lock::is_project_file(
-                    rel.rsplit('/').next().unwrap_or(rel),
-                )
-        })
-        .map(|(rel, text)| (rel.clone(), text.clone()))
-        .collect();
-    let governed =
-        crate::formats::nuget::lock::governed_locks(&projects, |rel| files.contains_key(rel));
-    if let Some((project, detail)) = governed.unresolved.first() {
-        result.warnings.push(RewriteWarning {
-            code: "redirect_nuget_lock_path_unresolved".into(),
-            detail: format!(
-                "{project}: {detail}; the lock it restores into cannot be pinned, so nuget \
-                 redirect is skipped (set a literal NuGetLockFilePath, or remove it)"
-            ),
-        });
-        return;
-    }
+    let lock_rels: Vec<String> = match files.get(NUGET_LOCKS_KEY) {
+        None => vec![crate::formats::nuget::lock::PACKAGES_LOCK.to_string()],
+        Some(found) => {
+            let mut rels = Vec::new();
+            for line in found.lines() {
+                let mut fields = line.splitn(3, '\t');
+                match (fields.next(), fields.next(), fields.next()) {
+                    (Some("lock"), Some(rel), None) => rels.push(rel.to_string()),
+                    (Some("unresolved"), Some(project), Some(detail)) => {
+                        result.warnings.push(RewriteWarning {
+                            code: "redirect_nuget_lock_path_unresolved".into(),
+                            detail: format!(
+                                "{project}: {detail}; the lock it restores into cannot be \
+                                 pinned, so nuget redirect is skipped (set a literal \
+                                 NuGetLockFilePath, or remove it)"
+                            ),
+                        });
+                        return;
+                    }
+                    (Some("error"), Some(why), _) => {
+                        result.warnings.push(RewriteWarning {
+                            code: "redirect_nuget_lock_unreadable".into(),
+                            detail: format!(
+                                "cannot list the project's NuGet locks ({why}); nuget redirect \
+                                 skipped"
+                            ),
+                        });
+                        return;
+                    }
+                    _ => {}
+                }
+            }
+            rels
+        }
+    };
     // Read past a UTF-8 BOM the way dotnet does (#623); the write below
     // keeps it.
     let mut locks: Vec<(String, &String, Value, bool)> = Vec::new();
-    for rel in governed.locks {
+    for rel in lock_rels {
         let Some(text) = files.get(&rel) else {
             continue;
         };
@@ -22644,21 +22650,24 @@ packages:
     }
 
     /// #353 / #514: member-project locks and named locks the root config
-    /// governs are re-pinned under their own paths with the config.
+    /// governs (the engine's walk, carried by [`NUGET_LOCKS_KEY`]) are
+    /// re-pinned under their own paths with the config.
     #[test]
     fn nuget_member_and_named_locks_are_repinned() {
         let mut files = BTreeMap::new();
         files.insert("nuget.config".to_string(), default_nuget_config());
-        files.insert("src/App/App.csproj".to_string(), "<Project />".to_string());
         files.insert(
             "src/App/packages.lock.json".to_string(),
             simple_lock("ORIGINALHASH=="),
         );
-        files.insert("src/Lib/Lib.csproj".to_string(), "<Project />".to_string());
         files.insert(
             "src/Lib/packages.Lib.lock.json".to_string(),
             simple_lock("ORIGINALHASH=="),
         );
+        files.insert(
+            NUGET_LOCKS_KEY.to_string(),
+            "lock\tsrc/App/packages.lock.json\nlock\tsrc/Lib/packages.Lib.lock.json".to_string(),
+        );
         let r = rewrite_registry_redirect(&files, &[nuget_override()]);
         assert!(r.warnings.is_empty(), "{:?}", r.warnings);
         for rel in [
@@ -22671,7 +22680,6 @@ packages:
                 "{rel}"
             );
         }
-        assert!(!r.files.contains_key("src/App/App.csproj"));
         let lock_paths: Vec<&str> = r
             .edits
             .iter()
@@ -22685,35 +22693,31 @@ packages:
                 "src/Lib/packages.Lib.lock.json"
             ]
         );
+        // Without the walk (in memory, unit tests) only the root lock is one:
+        // a member lock is never mistaken for one.
+        files.remove(NUGET_LOCKS_KEY);
+        let r = rewrite_registry_redirect(&files, &[nuget_override()]);
+        assert!(!r.files.contains_key("src/App/packages.lock.json"));
     }
 
     /// #514: an unresolvable `NuGetLockFilePath`, or a project tree the
     /// engine could not list, skips the nuget redirect with nothing written.
     #[test]
     fn nuget_unknowable_locks_skip_the_redirect() {
-        let mut files = BTreeMap::new();
-        files.insert("nuget.config".to_string(), default_nuget_config());
-        files.insert(
-            "app.csproj".to_string(),
-            "<Project><PropertyGroup><NuGetLockFilePath>$(X)/l.json</NuGetLockFilePath></PropertyGroup></Project>"
-                .to_string(),
-        );
-        let r = rewrite_registry_redirect(&files, &[nuget_override()]);
-        assert!(r.files.is_empty() && r.edits.is_empty(), "{:?}", r.files);
-        assert_eq!(
-            warning_codes(&r),
-            vec!["redirect_nuget_lock_path_unresolved"]
-        );
-
-        let mut files = BTreeMap::new();
-        files.insert("nuget.config".to_string(), default_nuget_config());
-        files.insert(
-            NUGET_LOCK_WALK_KEY.to_string(),
-            "unreadable src".to_string(),
-        );
-        let r = rewrite_registry_redirect(&files, &[nuget_override()]);
-        assert!(r.files.is_empty() && r.edits.is_empty(), "{:?}", r.files);
-        assert_eq!(warning_codes(&r), vec!["redirect_nuget_lock_unreadable"]);
+        for (walk, code) in [
+            (
+                "lock\tpackages.lock.json\nunresolved\tapp.csproj\tNuGetLockFilePath `$(X)/l.json` references MSBuild properties or items",
+                "redirect_nuget_lock_path_unresolved",
+            ),
+            ("error\tunreadable src", "redirect_nuget_lock_unreadable"),
+        ] {
+            let mut files = BTreeMap::new();
+            files.insert("nuget.config".to_string(), default_nuget_config());
+            files.insert(NUGET_LOCKS_KEY.to_string(), walk.to_string());
+            let r = rewrite_registry_redirect(&files, &[nuget_override()]);
+            assert!(r.files.is_empty() && r.edits.is_empty(), "{:?}", r.files);
+            assert_eq!(warning_codes(&r), vec![code]);
+        }
     }
 
     /// #593: a multi-targeting lock resolving the patched id at another

crates/socket-patch-core/src/vendor/nuget_config.rs

@@ -86,22 +86,41 @@ pub(crate) fn project_files(root: &std::path::Path) -> Result<Vec<(String, Strin
             std::fs::read_dir(&dir).map_err(|e| format!("unreadable {}: {e}", dir.display()))?;
         for entry in entries {
             let entry = entry.map_err(|e| format!("unreadable {}: {e}", dir.display()))?;
-            let Some(name) = entry.file_name().to_str().map(str::to_string) else {
+            let raw = entry.file_name();
+            let Some(name) = raw.to_str().map(str::to_string) else {
+                // A project (or a directory that may hold one) this walk
+                // cannot name is a lock it would miss: fail closed.
+                let lossy = raw.to_string_lossy();
+                if crate::formats::nuget::lock::is_project_file(&lossy)
+                    || entry.file_type().is_ok_and(|k| k.is_dir())
+                {
+                    return Err(format!(
+                        "{} has a name that is not UTF-8",
+                        dir.join(&raw).display()
+                    ));
+                }
                 continue;
             };
             let child = if rel.is_empty() {
                 name.clone()
             } else {
                 format!("{rel}/{name}")
             };
-            let Ok(kind) = entry.file_type() else {
-                continue;
-            };
+            let kind = entry
+                .file_type()
+                .map_err(|e| format!("unreadable {}: {e}", entry.path().display()))?;
+            // A symlinked directory is not entered (it may lead outside the
+            // tree, or back into it); a symlinked project file is read
+            // through the link, like MSBuild opens it.
+            let is_project = crate::formats::nuget::lock::is_project_file(&name)
+                && (kind.is_file()
+                    || (kind.is_symlink()
+                        && std::fs::metadata(entry.path()).is_ok_and(|m| m.is_file())));
             if kind.is_dir() {
                 if !name.starts_with('.') && !SKIPPED_DIRS.contains(&name.as_str()) {
                     pending.push(child);
                 }
-            } else if kind.is_file() && crate::formats::nuget::lock::is_project_file(&name) {
+            } else if is_project {
                 let path = entry.path();
                 if std::fs::metadata(&path).is_ok_and(|m| m.len() > MAX_PROJECT_BYTES) {
                     return Err(format!("{} is too large to read", path.display()));
@@ -167,4 +186,38 @@ mod tests {
         std::fs::create_dir(&dir).unwrap();
         assert_eq!(same_file(&dir, &dir).await, cfg!(unix));
     }
+
+    /// The walk reads a symlinked project file, never enters build output
+    /// or hidden dirs, and finds projects at any depth.
+    #[cfg(unix)]
+    #[test]
+    fn project_walk_follows_project_links_and_skips_output() {
+        let tmp = tempfile::tempdir().unwrap();
+        let root = tmp.path();
+        for dir in ["src/App/obj", "src/Lib", ".git", "shared"] {
+            std::fs::create_dir_all(root.join(dir)).unwrap();
+        }
+        std::fs::write(root.join("src/App/App.csproj"), "<Project />").unwrap();
+        std::fs::write(root.join("src/App/obj/Gen.csproj"), "<Project />").unwrap();
+        std::fs::write(root.join(".git/X.csproj"), "<Project />").unwrap();
+        std::fs::write(root.join("shared/Lib.csproj"), "<Project>lib</Project>").unwrap();
+        std::os::unix::fs::symlink(
+            root.join("shared/Lib.csproj"),
+            root.join("src/Lib/Lib.csproj"),
+        )
+        .unwrap();
+        let found: Vec<String> = super::project_files(root)
+            .unwrap()
+            .into_iter()
+            .map(|(rel, _)| rel)
+            .collect();
+        assert_eq!(
+            found,
+            [
+                "shared/Lib.csproj",
+                "src/App/App.csproj",
+                "src/Lib/Lib.csproj"
+            ]
+        );
+    }
 }

crates/socket-patch-core/src/vendor/nuget_feed.rs

@@ -444,6 +444,10 @@ pub async fn vendor_nuget(
             // re-attaches the untouched config records.
             let mut wiring: Vec<WiringRecord> = Vec::new();
             let new_hash = sha512_base64_of(&bytes);
+            // Locks already re-pinned, put back if a later one fails: the
+            // projects must agree on one nupkg (the rebuilt artifact stays,
+            // the config routes to it).
+            let mut written: Vec<(&LockFile, &str)> = Vec::new();
             for lock in &locks {
                 match edit_lock(&lock.text, name, &version_norm, &new_hash) {
                     Ok(Some(edit)) => {
@@ -452,10 +456,12 @@ pub async fn vendor_nuget(
                             atomic_write_bytes_preserving_mode(&lock.path, edit.text.as_bytes())
                                 .await
                         {
+                            unwind_locks(&written).await;
                             result.success = false;
                             result.error = Some(format!("failed to rewrite {}: {e}", lock.rel));
                             return done(result, None, warnings);
                         }
+                        written.push((lock, lock.text.as_str()));
                         wiring.push(WiringRecord {
                             file: lock.rel.clone(),
                             kind: LOCK_WIRING_KIND.to_string(),
@@ -467,6 +473,7 @@ pub async fn vendor_nuget(
                     }
                     Ok(None) => {}
                     Err(detail) => {
+                        unwind_locks(&written).await;
                         result.success = false;
                         result.error = Some(format!("{}: {detail}", lock.rel));
                         return done(result, None, warnings);