From eb06ace8eed0cc5ba19c29fcba5b052915e1fa37 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Sun, 6 Sep 2026 12:39:27 -0400 Subject: [PATCH 1/5] feat(fetch): resolve a nested workspace's members against their own root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A scan can span more than one workspace: a `fuzz/` or `examples/` tree with its own `[workspace]` is a root in its own right, and the walk descends into it because Cargo already ignores such a subtree, so nobody lists it in the outer root's `exclude`. The shallow graph resolved every member against one root's tables — the scan root's. Refusing to hand the outer root's `[workspace.package] version` to a nested crate stopped it reporting a number that was never its own, but left it with no version at all, when the version it does have sits one directory up in its own root. The same single-root assumption routed every member's `dep.workspace = true` through the outer root's `[workspace.dependencies]`, so a name that a nested root vendors by path was classified from whatever the outer root happened to say about that name, or from nothing. Carry a `Scope` per workspace root instead — both inheritance tables together, since a member's `version.workspace = true` and its `dep.workspace = true` name the same root — and give each member the index of the nearest `[workspace]` ancestor that governs it. A nested root declaring no version still yields none: an absent table resolves to nothing rather than to some other root's number, so a crate can never borrow a version from a workspace it is not in. The walk's growing state moves onto a `Walk` struct so the recursion carries context rather than an argument list. --- crates/dependable-fetch/src/tree.rs | 231 ++++++++++++++---------- crates/dependable-fetch/tests/tree.rs | 245 ++++++++++++++++++++++++-- 2 files changed, 364 insertions(+), 112 deletions(-) diff --git a/crates/dependable-fetch/src/tree.rs b/crates/dependable-fetch/src/tree.rs index c42e9aa..5ccd3f7 100644 --- a/crates/dependable-fetch/src/tree.rs +++ b/crates/dependable-fetch/src/tree.rs @@ -12,7 +12,7 @@ //! — a manifest declares a constraint, not a resolution — except where the //! constraint names exactly one release, which [`declared_pin`] reads off it. -use std::collections::{HashMap, HashSet}; +use std::collections::{BTreeMap, HashMap, HashSet}; use std::path::{Path, PathBuf}; use dependable_core::{ @@ -109,7 +109,7 @@ pub fn build_workspace_graph( let root_content = read(&root_dir.join("Cargo.toml"))?; let excluded = excluded_dirs(&root_dir, &root_content); - let members = collect_members(&root_dir, &excluded); + let (members, scopes) = collect_members(&root_dir, &root_content, &excluded); let workspace_names: HashSet = members.iter().map(|member| member.name.clone()).collect(); @@ -136,7 +136,7 @@ pub fn build_workspace_graph( }); } - let graph = shallow_graph(&members, &workspace_names, &roots, &root_content); + let graph = shallow_graph(&members, &workspace_names, &roots, &scopes); Ok(WorkspaceGraph { graph, source: GraphSource::Manifests, @@ -169,58 +169,138 @@ fn excluded_dirs(root_dir: &Path, root_content: &str) -> HashSet { .unwrap_or_default() } +/// The authority one `[workspace]` root lends its members: both of Cargo's +/// inheritance tables, read off that root's manifest. +/// +/// A single scan can span more than one workspace — a `fuzz/` or `examples/` tree +/// with its own `[workspace]` is a root in its own right — so "the workspace root" +/// is not a single thing and each member must be read against the one that +/// actually governs it. The two tables travel together because a member's +/// `version.workspace = true` and its `dep.workspace = true` name the *same* root; +/// answering them from different manifests is the bug this type exists to prevent. +struct Scope { + /// `[workspace.package]`, the source of a member's `version.workspace = true`. + package_defaults: BTreeMap, + /// `[workspace.dependencies]`, the source of a member's `dep.workspace = true`. + declarations: Vec, +} + +/// Read a manifest's two workspace inheritance tables. +/// +/// A table that is absent, or a manifest that does not parse, yields an empty one +/// — which is the honest answer rather than a fallback: a root declaring no +/// `[workspace.package] version` resolves its members' `version.workspace = true` +/// to nothing, never to some other root's number. +fn scope_of(content: &str) -> Scope { + Scope { + package_defaults: parse_workspace(content) + .map(|ws| ws.package_defaults) + .unwrap_or_default(), + // A member's `dep.workspace = true` says nothing about what the crate *is* — + // its root's declaration does. Resolving against it is what tells a + // centrally-declared registry crate from a centrally-declared vendored path, + // which the member's own text cannot. + declarations: CargoTomlParser + .parse(content) + .map(|m| m.items) + .unwrap_or_default() + .into_iter() + .filter(|item| item.kind == DependencyKind::Workspace) + .collect(), + } +} + /// A crate manifest found under the scan root. struct Member { /// The crate's `[package] name`. name: String, /// The manifest's text. content: String, - /// Whether the scan root is this crate's **nearest** `[workspace]` ancestor, - /// and so whether the root's `[workspace.package]` table governs it. + /// Index into the scan's [`Scope`] arena: the workspace root that governs this + /// crate, which is its **nearest** `[workspace]` ancestor. /// - /// False for a crate inside a nested, independent workspace — a `fuzz/` or - /// `examples/` directory with its own `[workspace]` table. Cargo resolves - /// those against *their* root; the scan root has no authority over them. - governed_by_root: bool, + /// The scan root is index 0. A crate inside a nested, independent workspace — a + /// `fuzz/` or `examples/` directory with its own `[workspace]` table — points at + /// that nested root instead, because Cargo resolves it against *that* one and the + /// scan root has no authority over it. + scope: usize, } -/// Collect a [`Member`] for every crate under `root_dir`, deduplicated by name. -/// A crate is treated as in-workspace iff its `[package] name` appears here — -/// this sidesteps needing a glob engine. -fn collect_members(root_dir: &Path, excluded: &HashSet) -> Vec { - let mut out = Vec::new(); - let mut seen = HashSet::new(); - walk_members(root_dir, root_dir, excluded, &mut seen, &mut out, 64, true); - out +/// Collect a [`Member`] for every crate under `root_dir`, deduplicated by name, +/// together with the [`Scope`] arena those members index into. A crate is treated +/// as in-workspace iff its `[package] name` appears here — this sidesteps needing +/// a glob engine. +fn collect_members( + root_dir: &Path, + root_content: &str, + excluded: &HashSet, +) -> (Vec, Vec) { + let mut walk = Walk { + root_dir, + excluded, + seen: HashSet::new(), + members: Vec::new(), + // The scan root is index 0, and a nested root can only be pushed after the + // root that contains it — so a smaller index is always the outer scope. + scopes: vec![scope_of(root_content)], + }; + walk.descend(root_dir, 64, 0); + (walk.members, walk.scopes) } -/// Whether `dir` holds a `Cargo.toml` declaring a `[workspace]` table. -fn declares_workspace(dir: &Path) -> bool { - std::fs::read_to_string(dir.join("Cargo.toml")) - .is_ok_and(|content| parse_workspace(&content).is_some()) +/// One run of the member walk. +/// +/// The walk carries state across the whole recursion — the dedup index, the +/// members found, and the [`Scope`] arena that grows as nested workspace roots are +/// met — so it lives here rather than in an argument list threaded through every +/// call. Only what actually varies per directory stays an argument. +struct Walk<'a> { + /// The scan root, the one directory whose `[workspace]` does not open a new scope. + root_dir: &'a Path, + /// Absolute directories named in the scan root's `[workspace] exclude`. + excluded: &'a HashSet, + /// Every `[package] name` already recorded; a crate name yields one member. + seen: HashSet, + members: Vec, + scopes: Vec, } -fn walk_members( - dir: &Path, - root_dir: &Path, - excluded: &HashSet, - seen: &mut HashSet, - out: &mut Vec, - depth_left: usize, - governed: bool, -) { - // A nested `[workspace]` is a workspace root in its own right. Cargo already - // ignores such a subtree, so nobody lists it in `[workspace] exclude`, and the - // walk still descends into it — but the scan root's `[workspace.package]` has - // no authority there, so nothing at or below this manifest may inherit from it. - let governed = governed && (dir == root_dir || !declares_workspace(dir)); - let Ok(entries) = std::fs::read_dir(dir) else { - return; - }; - for entry in entries.flatten() { - let path = entry.path(); - if path.is_dir() { - if depth_left == 0 || excluded.contains(&path) { +impl Walk<'_> { + /// Record `dir`'s crate, if it holds one, then descend into its subdirectories. + fn descend(&mut self, dir: &Path, depth_left: usize, scope: usize) { + // Read once: the same text answers both "is this a workspace root?" and "is + // this a crate?", and a `cargo fuzz` manifest is routinely both. + let manifest = std::fs::read_to_string(dir.join("Cargo.toml")).ok(); + // A nested `[workspace]` is a workspace root in its own right. Cargo already + // ignores such a subtree, so nobody lists it in `[workspace] exclude`, and the + // walk still descends into it — but the outer root's tables have no authority + // there. Everything at or below this manifest resolves against the nested root + // instead. The scope is switched *before* this directory's own `[package]` is + // read, so a manifest that is both a `[workspace]` and a `[package]` resolves + // against itself. + let scope = match manifest.as_deref() { + Some(content) if dir != self.root_dir && parse_workspace(content).is_some() => { + self.scopes.push(scope_of(content)); + self.scopes.len() - 1 + } + _ => scope, + }; + if let Some(content) = manifest + && let Some(name) = parse_package_name(&content) + && self.seen.insert(name.clone()) + { + self.members.push(Member { + name, + content, + scope, + }); + } + let Ok(entries) = std::fs::read_dir(dir) else { + return; + }; + for entry in entries.flatten() { + let path = entry.path(); + if !path.is_dir() || depth_left == 0 || self.excluded.contains(&path) { continue; } if let Some(name) = path.file_name().and_then(|n| n.to_str()) @@ -228,25 +308,7 @@ fn walk_members( { continue; } - walk_members( - &path, - root_dir, - excluded, - seen, - out, - depth_left - 1, - governed, - ); - } else if path.file_name().and_then(|n| n.to_str()) == Some("Cargo.toml") - && let Ok(content) = std::fs::read_to_string(&path) - && let Some(name) = parse_package_name(&content) - && seen.insert(name.clone()) - { - out.push(Member { - name, - content, - governed_by_root: governed, - }); + self.descend(&path, depth_left - 1, scope); } } } @@ -262,42 +324,31 @@ fn walk_members( /// which case the manifest has already resolved it and [`declared_pin`] reads it /// off. /// -/// `version.workspace = true` is resolved against the root's `[workspace.package]` -/// only for a member the root actually governs (see [`Member::governed_by_root`]). -/// A crate in a nested, independent workspace keeps no version at all: reporting -/// nothing is honest, where borrowing an unrelated root's number would not be. +/// Both kinds of `workspace = true` — a member's own `version` and its +/// dependencies — are resolved against [`Member::scope`], the crate's nearest +/// `[workspace]` ancestor, rather than against the scan root. A crate in a nested, +/// independent workspace therefore reports what *its* root declares; where that +/// root declares nothing, nothing is reported, because borrowing an unrelated +/// root's number would be a confidently wrong answer rather than an absent one. fn shallow_graph( members: &[Member], workspace_names: &HashSet, roots: &[String], - root_content: &str, + scopes: &[Scope], ) -> DependencyGraph { - // A member's `dep.workspace = true` says nothing about what the crate *is* — the - // root's declaration does, and the root is already in hand here. Resolving against - // it is what tells a centrally-declared registry crate from a centrally-declared - // vendored path, which the member's own text cannot. - let declarations: Vec = CargoTomlParser - .parse(root_content) - .map(|m| m.items) - .unwrap_or_default() - .into_iter() - .filter(|item| item.kind == DependencyKind::Workspace) - .collect(); - // `[workspace.package]`, the source a member's `version.workspace = true` - // inherits from. A member that inherits its version still has one. - let package_defaults = parse_workspace(root_content) - .map(|ws| ws.package_defaults) - .unwrap_or_default(); let mut member_pkgs: Vec = Vec::new(); let mut external_pkgs: Vec = Vec::new(); let mut external_seen: HashMap = HashMap::new(); for member in members { + // The root that governs *this* crate, which in a scan spanning more than one + // workspace is not necessarily the scan root. + let scope = &scopes[member.scope]; let mut items = CargoTomlParser .parse(&member.content) .map(|m| m.items) .unwrap_or_default(); - let _ = resolve_workspace_inheritance(&mut items, &declarations); + let _ = resolve_workspace_inheritance(&mut items, &scope.declarations); let mut deps: Vec = Vec::new(); for item in &items { deps.push(item.name.clone()); @@ -305,8 +356,8 @@ fn shallow_graph( continue; } // Items are inheritance-resolved by now, so a member's - // `dep.workspace = true` pointing at a root `= "1.0.200"` is read here - // as the pin the root declared. + // `dep.workspace = true` pointing at its own root's `= "1.0.200"` is read + // here as the pin that root declared. let pin = declared_pin(item, Ecosystem::Rust).map(str::to_owned); match external_seen.get(&item.name) { // One node for the name, so a version survives only where every @@ -348,15 +399,7 @@ fn shallow_graph( let version = parse_project(ManifestKind::CargoToml, &member.content) .version .as_ref() - .and_then(|field| { - if member.governed_by_root { - field.resolve(&package_defaults, "version") - } else { - // Not this root's crate to resolve — take only what its own - // manifest states outright. - field.literal() - } - }) + .and_then(|field| field.resolve(&scope.package_defaults, "version")) .map(str::to_owned); member_pkgs.push(LockedPackage::new(member.name.clone(), version, None, deps)); } diff --git a/crates/dependable-fetch/tests/tree.rs b/crates/dependable-fetch/tests/tree.rs index 52e416c..c180769 100644 --- a/crates/dependable-fetch/tests/tree.rs +++ b/crates/dependable-fetch/tests/tree.rs @@ -326,10 +326,12 @@ version.workspace = true /// `examples/` tree with its own `[workspace]` — because Cargo already ignores such /// a subtree, so nobody lists it in `[workspace] exclude`. The outer root has no /// authority over those crates, so its `[workspace.package] version` must not be -/// handed to them: that would turn "no version" into a confidently wrong one. +/// handed to them. The nested root's own table is the one that governs them, and it +/// is one directory up from the crate: reading it is what turns "no version" into +/// the right one rather than a confidently wrong one. /// A version the nested crate states outright is still its own, and still reported. #[test] -fn a_nested_independent_workspace_does_not_inherit_the_outer_roots_version() { +fn a_nested_independent_workspaces_crate_resolves_against_its_own_root() { let tmp = TempDir::new().unwrap(); let dir = tmp.path(); fs::write( @@ -386,26 +388,233 @@ version = "7.7.7" let built = build_workspace_graph(dir, &WorkspaceGraphOptions::default()).unwrap(); assert_eq!(built.source, GraphSource::Manifests); - let version_of = |name: &str| { - built - .graph - .nodes() - .iter() - .find(|n| n.name == name) - .unwrap_or_else(|| panic!("a node for {name}")) - .version - .clone() - }; - assert_eq!(version_of("a").as_deref(), Some("1.0.0")); + let g = &built.graph; + assert_eq!(version_of(g, "a"), Some("1.0.0")); assert_eq!( - version_of("a-fuzz"), - None, - "the outer root does not govern a nested workspace's crate" + version_of(g, "a-fuzz"), + Some("0.0.0"), + "a nested workspace's crate inherits from the nested root, never the outer one" ); assert_eq!( - version_of("a-fuzz-stated").as_deref(), + version_of(g, "a-fuzz-stated"), Some("7.7.7"), - "but a version the crate states outright is still its own" + "and a version the crate states outright is still its own" + ); +} + +/// The guard on the fix above: a nested root that declares a `[workspace]` and no +/// `[workspace.package]` resolves its members' `version.workspace = true` to +/// nothing. Reaching past it to the outer root's `1.0.0` would be the bug the +/// scope stack exists to prevent, dressed up as a fallback. +#[test] +fn a_nested_root_declaring_no_version_leaves_its_crate_without_one() { + let tmp = TempDir::new().unwrap(); + let dir = tmp.path(); + fs::write( + dir.join("Cargo.toml"), + r#" +[workspace] +resolver = "2" +members = ["crates/a"] + +[workspace.package] +version = "1.0.0" +"#, + ) + .unwrap(); + let member = dir.join("crates").join("a"); + fs::create_dir_all(&member).unwrap(); + fs::write( + member.join("Cargo.toml"), + r#" +[package] +name = "a" +version.workspace = true +"#, + ) + .unwrap(); + let silent = dir.join("silent"); + fs::create_dir_all(&silent).unwrap(); + fs::write( + silent.join("Cargo.toml"), + r#" +[workspace] + +[package] +name = "silent" +version.workspace = true +"#, + ) + .unwrap(); + + let built = build_workspace_graph(dir, &WorkspaceGraphOptions::default()).unwrap(); + assert_eq!(built.source, GraphSource::Manifests); + let g = &built.graph; + assert_eq!(version_of(g, "a"), Some("1.0.0")); + assert_eq!( + version_of(g, "silent"), + None, + "its own root declares no version, and the outer root's is not a fallback" + ); +} + +/// Workspaces nest more than one deep — a `fuzz/` tree that itself vendors an +/// example workspace. The governing root is the *nearest* `[workspace]` ancestor, +/// so the innermost one wins, for the crate that declares it and for everything +/// below it. +#[test] +fn the_innermost_workspace_root_governs_when_workspaces_nest_twice() { + let tmp = TempDir::new().unwrap(); + let dir = tmp.path(); + fs::write( + dir.join("Cargo.toml"), + r#" +[workspace] +resolver = "2" + +[workspace.package] +version = "1.0.0" +"#, + ) + .unwrap(); + let outer_member = dir.join("crates").join("a"); + fs::create_dir_all(&outer_member).unwrap(); + fs::write( + outer_member.join("Cargo.toml"), + r#" +[package] +name = "a" +version.workspace = true +"#, + ) + .unwrap(); + let mid = dir.join("mid"); + fs::create_dir_all(&mid).unwrap(); + fs::write( + mid.join("Cargo.toml"), + r#" +[workspace] + +[workspace.package] +version = "2.0.0" + +[package] +name = "mid" +version.workspace = true +"#, + ) + .unwrap(); + let inner = mid.join("inner"); + fs::create_dir_all(&inner).unwrap(); + fs::write( + inner.join("Cargo.toml"), + r#" +[workspace] + +[workspace.package] +version = "3.0.0" + +[package] +name = "inner" +version.workspace = true +"#, + ) + .unwrap(); + let leaf = inner.join("crates").join("leaf"); + fs::create_dir_all(&leaf).unwrap(); + fs::write( + leaf.join("Cargo.toml"), + r#" +[package] +name = "leaf" +version.workspace = true +"#, + ) + .unwrap(); + + let built = build_workspace_graph(dir, &WorkspaceGraphOptions::default()).unwrap(); + assert_eq!(built.source, GraphSource::Manifests); + let g = &built.graph; + assert_eq!(version_of(g, "a"), Some("1.0.0")); + // A manifest that is both a `[workspace]` and a `[package]` resolves against + // itself, not against the root that contains it. + assert_eq!(version_of(g, "mid"), Some("2.0.0")); + assert_eq!(version_of(g, "inner"), Some("3.0.0")); + assert_eq!( + version_of(g, "leaf"), + Some("3.0.0"), + "the nearest [workspace] ancestor governs, not the outermost" + ); +} + +/// `[workspace.dependencies]` is the second thing a root lends its members, and it +/// decides what a `dep.workspace = true` crate *is*. It is scoped exactly like +/// `[workspace.package]`: a nested workspace's crate takes its own root's +/// declaration, so a name that root vendors by path is a path crate, even though +/// the outer root declares the same shape of entry as a registry one. +/// +/// The two names are deliberately different: one node carries one name, so a name +/// declared on both sides could not distinguish which root was consulted. +#[test] +fn a_nested_workspaces_crate_inherits_dependencies_from_its_own_root() { + let tmp = TempDir::new().unwrap(); + let dir = tmp.path(); + fs::write( + dir.join("Cargo.toml"), + r#" +[workspace] +resolver = "2" + +[workspace.dependencies] +outer-dep = "1" +nested-dep = "1" +"#, + ) + .unwrap(); + let outer_member = dir.join("crates").join("outer"); + fs::create_dir_all(&outer_member).unwrap(); + fs::write( + outer_member.join("Cargo.toml"), + r#" +[package] +name = "outer" +version = "0.1.0" + +[dependencies] +outer-dep.workspace = true +"#, + ) + .unwrap(); + let nested = dir.join("nested"); + fs::create_dir_all(&nested).unwrap(); + fs::write( + nested.join("Cargo.toml"), + r#" +[workspace] + +[workspace.dependencies] +nested-dep = { path = "../vendor/nested-dep" } + +[package] +name = "nested" +version = "0.1.0" + +[dependencies] +nested-dep.workspace = true +"#, + ) + .unwrap(); + + let built = build_workspace_graph(dir, &WorkspaceGraphOptions::default()).unwrap(); + assert_eq!(built.source, GraphSource::Manifests); + let g = &built.graph; + assert_eq!(kind_of(g, "outer-dep"), NodeKind::Registry); + // The outer root declares `nested-dep = "1"` too, and consulting it would make + // this a registry crate. The nested root is the one with authority here. + assert_eq!( + kind_of(g, "nested-dep"), + NodeKind::Path, + "a nested workspace's crate inherits from the nested root's declarations" ); } From 84197494f2c30bcf1fcdc581340ea93472992695 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Sun, 6 Sep 2026 12:41:14 -0400 Subject: [PATCH 2/5] fix(fetch): settle a crate name shared across a nested workspace boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The member walk deduplicates by `[package] name` and takes the first crate it meets. Its `read_dir` yields filesystem order, so two crates sharing a name — one in the scanned workspace, one under a nested, independent root — were settled by whichever the filesystem happened to hand back first. That was invisible while both resolved against the same tables. Now that each resolves against its own root, the two answers differ, and the one reported would differ between machines holding identical contents. Descend in sorted order and keep the outer crate: a nested root is only pushed onto the scope arena after the root containing it, so the smaller scope index is the enclosing one. Sorting alone would only make a wrong answer stable — it is here so that the tie between two crates in sibling nested workspaces, where neither encloses the other, is fixed rather than arbitrary. --- crates/dependable-fetch/src/tree.rs | 47 +++++++++++++----- crates/dependable-fetch/tests/tree.rs | 71 +++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 11 deletions(-) diff --git a/crates/dependable-fetch/src/tree.rs b/crates/dependable-fetch/src/tree.rs index 5ccd3f7..d980eb9 100644 --- a/crates/dependable-fetch/src/tree.rs +++ b/crates/dependable-fetch/src/tree.rs @@ -238,7 +238,7 @@ fn collect_members( let mut walk = Walk { root_dir, excluded, - seen: HashSet::new(), + seen: HashMap::new(), members: Vec::new(), // The scan root is index 0, and a nested root can only be pushed after the // root that contains it — so a smaller index is always the outer scope. @@ -259,8 +259,8 @@ struct Walk<'a> { root_dir: &'a Path, /// Absolute directories named in the scan root's `[workspace] exclude`. excluded: &'a HashSet, - /// Every `[package] name` already recorded; a crate name yields one member. - seen: HashSet, + /// `[package] name` -> index into `members`; a crate name yields one member. + seen: HashMap, members: Vec, scopes: Vec, } @@ -287,19 +287,44 @@ impl Walk<'_> { }; if let Some(content) = manifest && let Some(name) = parse_package_name(&content) - && self.seen.insert(name.clone()) { - self.members.push(Member { - name, - content, - scope, - }); + match self.seen.get(&name).copied() { + // Two crates can share a `[package] name` across a nested-workspace + // boundary, and only one node can carry it. The outer scope wins: a + // nested root is only pushed after the root containing it, so a + // smaller index is the enclosing one. Between two crates in *sibling* + // nested workspaces neither encloses the other, and the smaller index + // is then the alphabetically earlier path — arbitrary, but fixed, + // which is the point. Without this the answer would follow whichever + // one the filesystem happened to hand back first. + Some(idx) => { + if scope < self.members[idx].scope { + self.members[idx] = Member { + name, + content, + scope, + }; + } + } + None => { + self.seen.insert(name.clone(), self.members.len()); + self.members.push(Member { + name, + content, + scope, + }); + } + } } let Ok(entries) = std::fs::read_dir(dir) else { return; }; - for entry in entries.flatten() { - let path = entry.path(); + // `read_dir` yields filesystem order, which can differ between machines holding + // identical contents. Descending in a fixed order is what makes the walk — and + // with it the duplicate-name rule above — reproducible. + let mut paths: Vec = entries.flatten().map(|entry| entry.path()).collect(); + paths.sort(); + for path in paths { if !path.is_dir() || depth_left == 0 || self.excluded.contains(&path) { continue; } diff --git a/crates/dependable-fetch/tests/tree.rs b/crates/dependable-fetch/tests/tree.rs index c180769..93114c3 100644 --- a/crates/dependable-fetch/tests/tree.rs +++ b/crates/dependable-fetch/tests/tree.rs @@ -618,6 +618,77 @@ nested-dep.workspace = true ); } +/// Two crates can share a `[package] name` across a nested-workspace boundary, and +/// the graph keys nodes by name, so only one of them survives. The outer one does — +/// it is the crate the scan is actually about — and it does so whichever of the two +/// the directory walk reaches first, since a version that depends on filesystem +/// iteration order is not a resolution of anything. +#[test] +fn a_name_shared_across_a_nested_boundary_keeps_the_outer_crates_version() { + // The same two crates twice, with the directories named so the walk reaches the + // outer crate first in one layout and the nested crate first in the other. + let built_with = |outer_dir: &str, nested_dir: &str| { + let tmp = TempDir::new().unwrap(); + let dir = tmp.path(); + fs::write( + dir.join("Cargo.toml"), + r#" +[workspace] +resolver = "2" + +[workspace.package] +version = "1.0.0" +"#, + ) + .unwrap(); + let outer_member = dir.join(outer_dir); + fs::create_dir_all(&outer_member).unwrap(); + fs::write( + outer_member.join("Cargo.toml"), + r#" +[package] +name = "dup" +version.workspace = true +"#, + ) + .unwrap(); + let nested = dir.join(nested_dir); + fs::create_dir_all(&nested).unwrap(); + fs::write( + nested.join("Cargo.toml"), + r#" +[workspace] + +[workspace.package] +version = "9.9.9" +"#, + ) + .unwrap(); + let nested_member = nested.join("dup"); + fs::create_dir_all(&nested_member).unwrap(); + fs::write( + nested_member.join("Cargo.toml"), + r#" +[package] +name = "dup" +version.workspace = true +"#, + ) + .unwrap(); + + let built = build_workspace_graph(dir, &WorkspaceGraphOptions::default()).unwrap(); + assert_eq!(built.source, GraphSource::Manifests); + version_of(&built.graph, "dup").map(str::to_owned) + }; + + assert_eq!(built_with("aaa", "zzz").as_deref(), Some("1.0.0")); + assert_eq!( + built_with("zzz", "aaa").as_deref(), + Some("1.0.0"), + "and still the outer crate when the walk meets the nested one first" + ); +} + /// Without a lockfile the graph is built from manifests alone, and a member's /// `dep.workspace = true` says nothing about what the crate *is*. The root's declaration /// does, and the root is already read here — so `centrally_declared` is classified from From 528978b0474c3b312776b2f260fbaeaa44c2b502 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Sun, 6 Sep 2026 13:48:19 -0400 Subject: [PATCH 3/5] fix(fetch): treat an unusable nested manifest as an opaque boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The member walk read a directory's `Cargo.toml` with `read_to_string(..).ok()` and asked `parse_workspace` whether it opened a new scope. Both collapse failure into `None`, so a nested root that could not be read — mode 000, or a TOML syntax error — was indistinguishable from a directory with no manifest at all, and every crate beneath it kept the *outer* scope. Such a crate resolved `version.workspace = true` against a root with no authority over it, reporting a number the outer root's `[workspace.package]` happened to carry. A file that exists and cannot be read is not evidence that the enclosing root governs what is below it: it may well declare a `[workspace]`, and nothing can rule that out. Classify the manifest into absent, readable, or opaque, and give an opaque one a scope of its own with both inheritance tables empty. Crates below it now inherit nothing, in either direction, while a directory with no `Cargo.toml` keeps inheriting the enclosing scope as before. --- crates/dependable-fetch/src/tree.rs | 62 ++++++++++++++++++++-- crates/dependable-fetch/tests/tree.rs | 76 +++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 5 deletions(-) diff --git a/crates/dependable-fetch/src/tree.rs b/crates/dependable-fetch/src/tree.rs index d980eb9..a1ffa11 100644 --- a/crates/dependable-fetch/src/tree.rs +++ b/crates/dependable-fetch/src/tree.rs @@ -178,6 +178,11 @@ fn excluded_dirs(root_dir: &Path, root_content: &str) -> HashSet { /// actually governs it. The two tables travel together because a member's /// `version.workspace = true` and its `dep.workspace = true` name the *same* root; /// answering them from different manifests is the bug this type exists to prevent. +/// +/// The [`Default`] scope — both tables empty — is what an *opaque* boundary gets: +/// a root whose manifest cannot be read or parsed lends no authority at all, but +/// still stops the enclosing root's from reaching past it. +#[derive(Default)] struct Scope { /// `[workspace.package]`, the source of a member's `version.workspace = true`. package_defaults: BTreeMap, @@ -210,6 +215,40 @@ fn scope_of(content: &str) -> Scope { } } +/// What a directory's `Cargo.toml` tells the walk about the subtree beneath it. +/// +/// The distinction that matters is between *absent* and *unusable*. A directory +/// with no manifest is plainly still governed by the enclosing workspace root; a +/// directory whose manifest exists but cannot be read or parsed is not — it may +/// well declare a `[workspace]`, and there is no way to tell. Collapsing the two +/// into "not a workspace" is what would let a crate below an unreadable nested +/// root inherit a version from a root with no authority over it. +enum Boundary { + /// No `Cargo.toml` here. The enclosing scope still governs what is below. + Absent, + /// A `Cargo.toml` that was read and parses as TOML. + Manifest(String), + /// A `Cargo.toml` that exists but could not be read, or is not valid TOML. + Opaque, +} + +/// Classify a directory's `Cargo.toml` into a [`Boundary`]. +/// +/// Anything other than "the file is not there" is [`Boundary::Opaque`]: a +/// permission error, an unreadable device, a directory of that name, or a syntax +/// error all leave the manifest's contents unknown, and unknown is not the same as +/// empty. +fn boundary_at(dir: &Path) -> Boundary { + match std::fs::read_to_string(dir.join("Cargo.toml")) { + // `CargoTomlParser` fails only when the TOML itself does not parse, which is + // exactly the question being asked here. + Ok(content) if CargoTomlParser.parse(&content).is_ok() => Boundary::Manifest(content), + Ok(_) => Boundary::Opaque, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Boundary::Absent, + Err(_) => Boundary::Opaque, + } +} + /// A crate manifest found under the scan root. struct Member { /// The crate's `[package] name`. @@ -222,7 +261,8 @@ struct Member { /// The scan root is index 0. A crate inside a nested, independent workspace — a /// `fuzz/` or `examples/` directory with its own `[workspace]` table — points at /// that nested root instead, because Cargo resolves it against *that* one and the - /// scan root has no authority over it. + /// scan root has no authority over it. An unreadable or unparseable manifest in + /// between opens a scope too — an empty one, per [`Boundary::Opaque`]. scope: usize, } @@ -270,7 +310,7 @@ impl Walk<'_> { fn descend(&mut self, dir: &Path, depth_left: usize, scope: usize) { // Read once: the same text answers both "is this a workspace root?" and "is // this a crate?", and a `cargo fuzz` manifest is routinely both. - let manifest = std::fs::read_to_string(dir.join("Cargo.toml")).ok(); + let manifest = boundary_at(dir); // A nested `[workspace]` is a workspace root in its own right. Cargo already // ignores such a subtree, so nobody lists it in `[workspace] exclude`, and the // walk still descends into it — but the outer root's tables have no authority @@ -278,14 +318,26 @@ impl Walk<'_> { // instead. The scope is switched *before* this directory's own `[package]` is // read, so a manifest that is both a `[workspace]` and a `[package]` resolves // against itself. - let scope = match manifest.as_deref() { - Some(content) if dir != self.root_dir && parse_workspace(content).is_some() => { + let scope = match &manifest { + Boundary::Manifest(content) + if dir != self.root_dir && parse_workspace(content).is_some() => + { self.scopes.push(scope_of(content)); self.scopes.len() - 1 } + // A manifest that exists but cannot be read or parsed is an opaque + // boundary, not an absent one: it may declare a `[workspace]`, and nothing + // here can rule that out. Push an empty scope so the crates below it + // resolve their `workspace = true` fields to nothing, rather than silently + // borrowing an enclosing root's — a file that exists and cannot be read is + // not evidence that the outer root governs what is beneath it. + Boundary::Opaque if dir != self.root_dir => { + self.scopes.push(Scope::default()); + self.scopes.len() - 1 + } _ => scope, }; - if let Some(content) = manifest + if let Boundary::Manifest(content) = manifest && let Some(name) = parse_package_name(&content) { match self.seen.get(&name).copied() { diff --git a/crates/dependable-fetch/tests/tree.rs b/crates/dependable-fetch/tests/tree.rs index 93114c3..702f4eb 100644 --- a/crates/dependable-fetch/tests/tree.rs +++ b/crates/dependable-fetch/tests/tree.rs @@ -689,6 +689,82 @@ version.workspace = true ); } +/// The scope stack's guarantee has to hold for a nested root that cannot be read at +/// all. A `Cargo.toml` with a TOML syntax error yields no `[workspace]` table — but +/// "it does not parse" is not "it is not a workspace", and treating the two alike +/// would hand every crate beneath it to the outer root, which is precisely the +/// confidently-wrong number the scope stack exists to prevent. An unusable manifest +/// is an opaque boundary: nothing is inherited across it, in either direction. +#[test] +fn a_crate_under_an_unparseable_nested_root_inherits_nothing() { + let tmp = TempDir::new().unwrap(); + let dir = tmp.path(); + fs::write( + dir.join("Cargo.toml"), + r#" +[workspace] +resolver = "2" +members = ["crates/a"] + +[workspace.package] +version = "1.0.0" + +[workspace.dependencies] +shared-dep = "1" +"#, + ) + .unwrap(); + let member = dir.join("crates").join("a"); + fs::create_dir_all(&member).unwrap(); + fs::write( + member.join("Cargo.toml"), + r#" +[package] +name = "a" +version.workspace = true +"#, + ) + .unwrap(); + // An unterminated table header: the file exists, and no reader can say whether + // it declares a `[workspace]`. + let broken = dir.join("fuzz"); + fs::create_dir_all(&broken).unwrap(); + fs::write(broken.join("Cargo.toml"), "[workspace\n").unwrap(); + let below = broken.join("crates").join("target-crate"); + fs::create_dir_all(&below).unwrap(); + fs::write( + below.join("Cargo.toml"), + r#" +[package] +name = "target-crate" +version.workspace = true + +[dependencies] +shared-dep.workspace = true +"#, + ) + .unwrap(); + + let built = build_workspace_graph(dir, &WorkspaceGraphOptions::default()).unwrap(); + assert_eq!(built.source, GraphSource::Manifests); + let g = &built.graph; + assert_eq!( + version_of(g, "a"), + Some("1.0.0"), + "the outer root still governs its own members" + ); + assert_eq!( + version_of(g, "target-crate"), + None, + "a crate under an unreadable root inherits no version, least of all the outer root's 1.0.0" + ); + assert_eq!( + version_of(g, "shared-dep"), + None, + "and no dependency constraint either — the outer `[workspace.dependencies]` does not reach across" + ); +} + /// Without a lockfile the graph is built from manifests alone, and a member's /// `dep.workspace = true` says nothing about what the crate *is*. The root's declaration /// does, and the root is already read here — so `centrally_declared` is classified from From 9e0c46590e23e716df5b8f97d589702ee8beb7f4 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Sun, 6 Sep 2026 13:48:27 -0400 Subject: [PATCH 4/5] test(fetch): pin which crate wins a name shared inside one scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two crates can also share a `[package] name` without a workspace boundary between them — `crates/dup` and `examples/dup` in one workspace — and the scope comparison cannot settle that: both index the same root, so first-wins decides. What "first" means is the walk's sorted descent, and nothing pinned it. The existing cross-boundary test passes with or without `paths.sort()`, because the scope comparison picks the same winner in either encounter order. Assert that the alphabetically earlier path wins, with the two directories created in reverse alphabetical order so a filesystem reporting entries in creation order hands the walk the wrong crate first. Removing `paths.sort()` fails this test on every run rather than on some machines. --- crates/dependable-fetch/tests/tree.rs | 44 +++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/crates/dependable-fetch/tests/tree.rs b/crates/dependable-fetch/tests/tree.rs index 702f4eb..01b6f6c 100644 --- a/crates/dependable-fetch/tests/tree.rs +++ b/crates/dependable-fetch/tests/tree.rs @@ -689,6 +689,50 @@ version.workspace = true ); } +/// Two crates in the *same* scope can share a `[package] name` too — `crates/dup` +/// and `examples/dup` in one workspace — and there no scope comparison can pick a +/// winner: both index the same root. First-wins then decides, and what "first" +/// means is the walk's own sorted descent, not whatever order `read_dir` happened +/// to return. Without that sort this answer would vary between two machines +/// holding byte-identical checkouts, which is what makes the sort load-bearing +/// rather than tidy. +#[test] +fn a_name_shared_within_one_scope_settles_on_the_alphabetically_earlier_path() { + let tmp = TempDir::new().unwrap(); + let dir = tmp.path(); + fs::write( + dir.join("Cargo.toml"), + r#" +[workspace] +resolver = "2" +members = ["aaa", "zzz"] +"#, + ) + .unwrap(); + // Same name, same (root) scope, different literal versions — so which manifest + // the walk records is visible in the graph. Created in reverse alphabetical + // order deliberately: a filesystem that reports entries in creation order then + // hands back the *wrong* crate first, so the assertion below is answered by the + // walk's sort rather than by the directory happening to agree with it. + for (subdir, version) in [("zzz", "9.9.9"), ("aaa", "1.1.1")] { + let member = dir.join(subdir); + fs::create_dir_all(&member).unwrap(); + fs::write( + member.join("Cargo.toml"), + format!("[package]\nname = \"dup\"\nversion = \"{version}\"\n"), + ) + .unwrap(); + } + + let built = build_workspace_graph(dir, &WorkspaceGraphOptions::default()).unwrap(); + assert_eq!(built.source, GraphSource::Manifests); + assert_eq!( + version_of(&built.graph, "dup"), + Some("1.1.1"), + "the sorted walk reaches `aaa` first, on every filesystem" + ); +} + /// The scope stack's guarantee has to hold for a nested root that cannot be read at /// all. A `Cargo.toml` with a TOML syntax error yields no `[workspace]` table — but /// "it does not parse" is not "it is not a workspace", and treating the two alike From c8e71c90693e82c353382e19cba13fadfc7b922c Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Sun, 6 Sep 2026 13:48:33 -0400 Subject: [PATCH 5/5] docs(fetch): record that the graph walk keeps its own skip list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `tree`'s private `SKIP_DIRS` lists the same four names as `discover::SKIP_DIRS`, and the inline filter beside it is `discover::is_skipped_dir` verbatim, which reads as an oversight worth unifying. It is not: the two bound different scans with different consequences — adding a name to `discover`'s list narrows what `list` and `check` report on, adding one here silently drops crates from the graph. Say so where the next reader will be tempted. --- crates/dependable-fetch/src/tree.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/crates/dependable-fetch/src/tree.rs b/crates/dependable-fetch/src/tree.rs index a1ffa11..feb4a62 100644 --- a/crates/dependable-fetch/src/tree.rs +++ b/crates/dependable-fetch/src/tree.rs @@ -25,6 +25,13 @@ use dependable_core::{ use thiserror::Error; /// Directories never descended into while collecting member manifests. +/// +/// Deliberately **not** [`crate::discover::SKIP_DIRS`], despite listing the same +/// names today. That list bounds what `list` and `check` scan for manifests to +/// report on; this one bounds what the graph walk treats as workspace members. The +/// two answer different questions with different blast radii — adding a name here +/// silently drops crates from the graph, adding one there only narrows a report — +/// so they are free to diverge, and unifying them would couple the two decisions. const SKIP_DIRS: &[&str] = &["target", "node_modules", ".git", "vendor"]; /// Where a workspace graph's edges came from.