fix(python): compare workspace paths canonically and read the workspace only under a declaration (#15031)

Follow-up to pnpm/pnpm#15019 for the review items that had no inline
thread when it merged.

pnpm run and pnpm exec compare the configured workspace path and the
command's directory with links resolved, so a workspace reached through
a link still finds the environment its members share.

An add without --filter reads the workspace around the edited project
only when a [tool.uv.workspace] is declared at or above it, which is
where a project can take siblings from the repository or share their
environment. A project outside any declared workspace is read alone, as
it was before, so the common add does not walk the whole workspace.

Shared membership reuses the workspace each project belongs to from the
scope reading, rather than walking ancestors again per member. The
disagreement search compares distinct ranges rather than every member's
requirement, so many members asking for one range add no pairs.

Related to pnpm/pnpm#15015.

pnpm/crates/cli/src/ecosystem_add.rs

@@ -84,9 +84,10 @@ async fn cargo_add_task<Reporter: pnpm_reporter::Reporter + 'static>(
 /// The Python half of the add, and the projects it was resolved against.
 ///
 /// Without a `--filter` selection the add acts on the project the command
-/// was run in, the way the npm add does. In a workspace it still reads the
-/// other projects: what that project may take from the repository, and
-/// whether it shares an environment, is declared around it.
+/// was run in, the way the npm add does. Under a declared uv workspace it
+/// still reads the other projects: what that project may take from the
+/// repository, and whether it shares an environment, is declared around
+/// it. A project outside any is read alone.
 async fn python_add_task<Reporter: pnpm_reporter::Reporter + 'static>(
     context: InstallContext,
     root: &Path,
@@ -108,12 +109,12 @@ async fn python_add_task<Reporter: pnpm_reporter::Reporter + 'static>(
     } else {
         let project = pnpm_python_installer::writable_project(root)?;
         let discovery = match config.workspace_dir.clone() {
-            Some(workspace_root) => {
+            Some(workspace_root)
+                if pnpm_python_installer::in_declared_workspace(&workspace_root, &project) =>
+            {
                 python::discover_around(config, &inventory(workspace_root), &project).await?
             }
-            None => {
-                pnpm_python_installer::discover(config, &[project.join("pyproject.toml")]).await?
-            }
+            _ => pnpm_python_installer::discover(config, &[project.join("pyproject.toml")]).await?,
         };
         (discovery, BTreeSet::from([project]))
     };

pnpm/crates/cli/tests/suite/python/shared.rs

@@ -117,6 +117,23 @@ async fn members_that_cannot_be_installed_together_are_refused() {
         ),
     );
     assert!(!root.path().join("pylock.toml").exists());
+
+    // A member asking both ranges does not hide the other member asking
+    // one of them.
+    python_project(
+        &root.path().join("packages/a"),
+        "a",
+        "dependencies = ['alpha==1.0', 'alpha==2.0']",
+    );
+    python_project(&root.path().join("packages/b"), "b", "dependencies = ['alpha==1.0']");
+    assert_failure_contains(
+        pacquet_in(root.path()).arg("install"),
+        &format!(
+            "{} requires `alpha==1.0` and {} requires `alpha==2.0`",
+            packages.join("b").display(),
+            packages.join("a").display(),
+        ),
+    );
 }
 
 #[tokio::test]
@@ -284,3 +301,58 @@ async fn a_shared_members_metadata_is_prepared_with_the_interpreter_the_root_ask
         .success()
         .stdout(line("0.0.1"));
 }
+
+#[tokio::test]
+async fn an_add_outside_the_declared_workspace_reads_the_project_alone() {
+    let root = tempfile::tempdir().unwrap();
+    let mut server = mockito::Server::new_async().await;
+    let _alpha = serve(&mut server, "alpha", &[("1.0", wheel("alpha", "1.0", "", &[]))]).await;
+    project(root.path(), &server.url(), &[]);
+    fs::remove_file(root.path().join("pyproject.toml")).unwrap();
+    let plain = |name: &str| {
+        format!(
+            "[project]\nname = '{name}'\nversion = '1.0'\nrequires-python = '>=3.10'\n\
+             dependencies = []\n",
+        )
+    };
+    for (directory, manifest) in [
+        (
+            "api",
+            "[tool.uv.workspace]\nmembers = ['providers/*']\n\n[tool.pnpm.python]\n\
+         shared-environment = true\n"
+                .to_string(),
+        ),
+        ("api/providers/good", plain("good")),
+        ("api/providers/broken", "[project\n".to_string()),
+        ("tools/x", plain("x")),
+    ] {
+        fs::create_dir_all(root.path().join(directory)).unwrap();
+        fs::write(
+            root.path()
+                .join(directory)
+                .join("pyproject.toml"),
+            manifest,
+        )
+        .unwrap();
+    }
+    let outside = root.path().join("tools/x");
+
+    pacquet_in(&outside)
+        .args(["add", "pypi:alpha"])
+        .assert()
+        .success();
+
+    assert!(fs::read_to_string(outside.join("pyproject.toml")).unwrap().contains("alpha>=1.0"));
+    assert!(outside.join("pylock.toml").is_file(), "an environment of its own");
+    assert!(
+        !root
+            .path()
+            .join("api/pylock.toml")
+            .exists(),
+        "the workspace was not installed",
+    );
+    pacquet_in(&root.path().join("api/providers/good"))
+        .args(["add", "pypi:alpha"])
+        .assert()
+        .failure();
+}

pnpm/crates/python-installer/src/lib.rs

@@ -1,6 +1,7 @@
 pub use add::{AddOptions, plan_add, writable_project};
 pub use discovery::{Discovery, PythonProject, discover};
 pub use manifest::DependencySelection;
+pub use workspace::members::in_declared_workspace;
 
 mod add;
 mod build;

pnpm/crates/python-installer/src/projects/disagreement.rs

@@ -4,7 +4,9 @@
 //!
 //! pubgrub reports the conflict as a chain of terms, which names the
 //! project as one root. The members are what the reader has to change, so
-//! the two that disagree are named ahead of that report.
+//! the two that disagree are named ahead of that report. The search is
+//! bounded by the distinct ranges the members ask for, not by how many
+//! members ask them.
 
 use super::Member;
 use crate::registry::Resolution;
@@ -25,28 +27,53 @@ pub(crate) fn disagreement(
     }
 }
 
-/// A version requirement one member declares on this environment.
+/// One version range members declare on this environment, and what
+/// each member asking it wrote, by member.
 struct Asked<'a> {
-    root: &'a Path,
-    requirement: &'a Requirement,
     specifiers: &'a VersionSpecifiers,
+    by_root: BTreeMap<&'a Path, &'a Requirement>,
+}
+
+impl<'a> Asked<'a> {
+    /// A member asking this range and a different member asking `other`,
+    /// each with the requirement it wrote, or `None` when one member
+    /// alone asks both.
+    fn distinct_roots(&self, other: &Self) -> Option<[(&'a Path, &'a Requirement); 2]> {
+        self.by_root
+            .iter()
+            .find_map(|(root, requirement)| {
+                other.by_root
+                    .iter()
+                    .find(|(second, _)| *second != root)
+                    .map(|(second, theirs)| [(*root, *requirement), (*second, *theirs)])
+            })
+    }
 }
 
 fn find(resolution: &Resolution, members: &[Member]) -> Option<String> {
     let environment = &resolution.target.environment;
-    let mut by_name = BTreeMap::<&PackageName, Vec<Asked<'_>>>::new();
+    // One entry per distinct range, with every member asking it: two
+    // members asking the same range cannot disagree with each other, so
+    // the pairs compared are of distinct ranges, each reported for a
+    // member of its own.
+    let mut by_name = BTreeMap::<&PackageName, BTreeMap<String, Asked<'_>>>::new();
     for member in members {
         for requirement in &member.requirements.all {
             let Some(VersionOrUrl::VersionSpecifier(specifiers)) = &requirement.version_or_url
             else {
                 continue;
             };
-            if requirement.marker.evaluate(environment, &[]) {
-                by_name
-                    .entry(&requirement.name)
-                    .or_default()
-                    .push(Asked { root: &member.root, requirement, specifiers });
+            if !requirement.marker.evaluate(environment, &[]) {
+                continue;
             }
+            by_name
+                .entry(&requirement.name)
+                .or_default()
+                .entry(specifiers.to_string())
+                .or_insert_with(|| Asked { specifiers, by_root: BTreeMap::new() })
+                .by_root
+                .entry(&member.root)
+                .or_insert(requirement);
         }
     }
     by_name
@@ -55,6 +82,7 @@ fn find(resolution: &Resolution, members: &[Member]) -> Option<String> {
             let offered = resolution.packages.candidates
                 .get(name)
                 .filter(|offered| !offered.is_empty())?;
+            let asked = asked.into_values().collect::<Vec<_>>();
             conflicting_pair(name, &asked, &offered.keys().collect::<Vec<_>>())
         })
 }
@@ -72,8 +100,8 @@ fn conflicting_pair(
         .find_map(|(position, first)| {
             asked[position + 1..]
                 .iter()
-                .filter(|second| second.root != first.root)
-                .find(|second| {
+                .filter_map(|second| Some((second, first.distinct_roots(second)?)))
+                .find(|(second, _)| {
                     !offered
                         .iter()
                         .any(|version| {
@@ -81,15 +109,13 @@ fn conflicting_pair(
                                 && second.specifiers.contains(version)
                         })
                 })
-                .map(|second| {
+                .map(|(_, [(root, requirement), (other, theirs)])| {
                     format!(
                         "the Python projects sharing one environment cannot be installed together: \
-                         {} requires `{}` and {} requires `{}`, and no version of {name} the index \
-                         offers satisfies both",
-                        first.root.display(),
-                        first.requirement,
-                        second.root.display(),
-                        second.requirement,
+                         {} requires `{requirement}` and {} requires `{theirs}`, and no version of \
+                         {name} the index offers satisfies both",
+                        root.display(),
+                        other.display(),
                     )
                 })
         })

pnpm/crates/python-installer/src/tests.rs

@@ -334,7 +334,10 @@ const UNSHARED_ROOT: &str = "[tool.uv.workspace]\nmembers = ['libs/*']\n";
 #[test]
 fn a_command_uses_the_environment_its_project_shares_or_its_own() {
     let workspace = tempfile::tempdir().expect("workspace directory");
-    let root = workspace.path();
+    // The lookup answers with links resolved, and a temporary directory
+    // may be reached through one.
+    let root = dunce::canonicalize(workspace.path()).expect("canonical workspace");
+    let root = root.as_path();
     let environment_of = |dir: &std::path::Path| super::environment_dir(Some(root), dir);
     for project in ["packages/app", "packages/tool", "packages/nested/libs/x", "packages/inner"] {
         std::fs::create_dir_all(root.join(project).join("src")).expect("project directory");
@@ -366,3 +369,20 @@ fn a_command_uses_the_environment_its_project_shares_or_its_own() {
     let inner = root.join("packages/inner");
     assert_eq!(environment_of(&inner), inner.join(".venv"), "its own workspace root");
 }
+
+#[test]
+fn a_workspace_reached_through_a_link_still_shares_its_environment() {
+    let outside = tempfile::tempdir().expect("outside directory");
+    let real = outside.path().join("real");
+    let member = real.join("packages/app");
+    std::fs::create_dir_all(&member).expect("member directory");
+    std::fs::write(real.join("pyproject.toml"), SHARED_ROOT).expect("shared workspace");
+    std::fs::write(member.join("pyproject.toml"), MEMBER).expect("member");
+    let link = outside.path().join("link");
+    pnpm_fs::force_symlink_dir(&real, &link).expect("workspace link");
+    let canonical = dunce::canonicalize(&real).expect("canonical workspace");
+
+    assert_eq!(super::environment_dir(Some(&link), &member), canonical.join(".venv"));
+    assert!(super::in_declared_workspace(&link, &member));
+    assert!(!super::in_declared_workspace(&link, outside.path()), "outside the workspace");
+}

pnpm/crates/python-installer/src/workspace/members.rs

@@ -61,7 +61,7 @@ impl Workspace {
     ) -> Result<()> {
         let mut declared = BTreeMap::<&PackageName, &Path>::new();
         for (member, manifest) in member_projects(root, declaration, projects) {
-            if manifest.project.is_none() || !Self::declared_in(member, projects, root) {
+            if manifest.project.is_none() || self.declared_by(member) != Some(root) {
                 continue;
             }
             if let Some(name) = manifest.distribution()
@@ -80,11 +80,16 @@ impl Workspace {
         Ok(())
     }
 
-    /// Whether `root` is the workspace the project at `member` belongs
-    /// to: the nearest one declared at or above it.
-    fn declared_in(member: &Path, projects: &[(PathBuf, Arc<Manifest>)], root: &Path) -> bool {
-        Self::declaring_root(member, projects)
-            .is_some_and(|(declared_in, _, _)| declared_in == root)
+    /// The workspace the project at `member` belongs to: the nearest one
+    /// declared at or above it, which reading the scopes already found.
+    fn declared_by<'a>(&'a self, member: &'a Path) -> Option<&'a Path> {
+        if let Some((declared_in, _)) = self.inherited.get(member) {
+            return Some(declared_in);
+        }
+        self.manifests
+            .get(member)
+            .filter(|manifest| manifest.tool.uv.workspace.is_some())
+            .map(|_| member)
     }
 
     /// The directory whose lockfile and environment the project at `root`
@@ -189,14 +194,18 @@ impl Patterns {
 /// parse counts for nothing here and is reported by the next install.
 pub(crate) fn environment_root_of(workspace: Option<&Path>, dir: &Path) -> PathBuf {
     let Some(stop) = workspace else { return dir.to_path_buf() };
+    // The workspace may be configured by a path that reaches it through
+    // a link, while the command's directory is canonical.
+    let stop = canonical(stop);
+    let dir = canonical(dir);
     let project = dir
         .ancestors()
-        .take_while(|ancestor| ancestor.starts_with(stop))
+        .take_while(|ancestor| ancestor.starts_with(&stop))
         .find(|ancestor| ancestor.join("pyproject.toml").is_file())
-        .unwrap_or(dir);
+        .unwrap_or(&dir);
     let declared = project
         .ancestors()
-        .take_while(|ancestor| ancestor.starts_with(stop))
+        .take_while(|ancestor| ancestor.starts_with(&stop))
         .find_map(|ancestor| Some((ancestor, workspace_declaration(ancestor)?)));
     match declared {
         Some((root, (true, patterns))) if patterns.contain(root, project) => root.to_path_buf(),
@@ -204,6 +213,25 @@ pub(crate) fn environment_root_of(workspace: Option<&Path>, dir: &Path) -> PathB
     }
 }
 
+/// Whether a `[tool.uv.workspace]` is declared at or above `project`, up
+/// to `workspace`. A project in one may take its siblings from the
+/// repository and may share their environment, so an add there reads
+/// the workspace around it; a project outside any reads itself alone.
+#[must_use]
+pub fn in_declared_workspace(workspace: &Path, project: &Path) -> bool {
+    let stop = canonical(workspace);
+    canonical(project)
+        .ancestors()
+        .take_while(|ancestor| ancestor.starts_with(&stop))
+        .any(|ancestor| workspace_declaration(ancestor).is_some())
+}
+
+/// The path with links resolved, or as given where that fails: a path
+/// that cannot be resolved is still the one the caller means.
+fn canonical(path: &Path) -> PathBuf {
+    dunce::canonicalize(path).unwrap_or_else(|_| path.to_path_buf())
+}
+
 /// Whether the manifest at `root` shares its environment, and which
 /// projects it contains, for one that declares a workspace.
 fn workspace_declaration(root: &Path) -> Option<(bool, Patterns)> {