refactor!: harden the workspace-root descriptor before a second ecosystem uses it

crates/dependable-core/src/manifest.rs

@@ -33,7 +33,10 @@ pub struct AlternateRegistryDecl {
 /// Distinguishes manifest files. Every variant has a parser; the mapping to
 /// [`Ecosystem`] is many-to-one, since several manifest formats can belong to one
 /// registry.
-#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+///
+/// `Hash` because a kind is part of a cache key: a path alone does not say which parser
+/// read it, and two kinds can name the same file.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 #[non_exhaustive]
 pub enum ManifestKind {
     CargoToml,
@@ -138,8 +141,7 @@ impl ManifestKind {
     pub fn workspace_roots(self) -> Option<WorkspaceRoots> {
         match self {
             ManifestKind::CargoToml => Some(WorkspaceRoots {
-                root_names: &["Cargo.toml"],
-                root_kind: ManifestKind::CargoToml,
+                root_names: &[("Cargo.toml", ManifestKind::CargoToml)],
                 self_governing: true,
             }),
             _ => None,
@@ -203,18 +205,42 @@ impl ManifestKind {
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 #[non_exhaustive]
 pub struct WorkspaceRoots {
-    /// Candidate file names for a root, tried in order within each directory —
-    /// the same precedence rule [`ManifestKind::lockfiles`] uses, so a root beside
-    /// the manifest always beats one further up whichever name it goes by.
-    pub root_names: &'static [&'static str],
-    /// The kind a located root is parsed as. Not necessarily the kind that went
-    /// looking: an ecosystem may keep its central declarations in a different file
-    /// format from the manifests that inherit them.
-    pub root_kind: ManifestKind,
+    /// Candidate roots, each a path relative to a directory being searched, paired
+    /// with the kind that candidate is recognized and parsed as.
+    ///
+    /// They are tried in order within each directory — the same precedence rule
+    /// [`ManifestKind::lockfiles`] uses, so a root beside the manifest always beats
+    /// one further up whichever name it goes by.
+    ///
+    /// Each name carries its **own** kind rather than the list sharing one, because a
+    /// single ecosystem's roots need not share a file format: a pnpm member roots at
+    /// `pnpm-workspace.yaml`, a Bun member at the workspace root's own `package.json`,
+    /// and one kind over both names would run the wrong parser over one of them. The
+    /// paired kind is also not necessarily the kind that went looking — an ecosystem may
+    /// keep its central declarations in a format none of its members are written in.
+    ///
+    /// A candidate is a *path*, not just a file name, and the `dir.join()` the walk
+    /// performs already resolves the separator. A Gradle descriptor would name
+    /// `gradle/libs.versions.toml` — illustrative of a descriptor a future ecosystem
+    /// would write, not one that exists, since [`ManifestKind::GradleVersionCatalog`]
+    /// declares no roots today and no shipped candidate has more than one segment.
+    ///
+    /// Only the directory a candidate is joined onto is symlink-resolved; the
+    /// candidate's own segments are not, so two members reaching one physical root by
+    /// two spellings of a symlinked segment would parse it twice rather than share it.
+    pub root_names: &'static [(&'static str, ManifestKind)],
     /// Whether a manifest may be its own root. Cargo's root-that-is-also-a-package
     /// writes `serde.workspace = true` against its own table, so walking past
     /// itself would leave exactly those entries unresolved; a kind whose central
     /// declarations always live in a separate file sets this `false`.
+    ///
+    /// A self-governing manifest is recognized **and** parsed as its own kind, so this
+    /// may only be set on a kind that appears among its own [`root_names`] kinds.
+    /// Setting it on a kind that is only ever read by a different parser would accept a
+    /// manifest as its own root with one parser and then read it with another, which
+    /// answers `Err`, and an unparseable root silently declares nothing.
+    ///
+    /// [`root_names`]: WorkspaceRoots::root_names
     pub self_governing: bool,
 }
 
@@ -468,30 +494,82 @@ mod tests {
         assert!(!ManifestKind::GoMod.has_lockfile_support());
     }
 
+    /// Every manifest kind, so an invariant that has to hold for *all* of them can be
+    /// asserted over the whole set rather than over whichever ones a test remembered.
+    const ALL_KINDS: [ManifestKind; 12] = [
+        ManifestKind::CargoToml,
+        ManifestKind::GoMod,
+        ManifestKind::PackageJson,
+        ManifestKind::DenoJson,
+        ManifestKind::PnpmWorkspaceYaml,
+        ManifestKind::ComposerJson,
+        ManifestKind::RequirementsTxt,
+        ManifestKind::PyprojectToml,
+        ManifestKind::PubspecYaml,
+        ManifestKind::MixExs,
+        ManifestKind::Csproj,
+        ManifestKind::GradleVersionCatalog,
+    ];
+
+    /// The match is what makes the compiler care that [`ALL_KINDS`] is complete: adding a
+    /// [`ManifestKind`] variant makes it non-exhaustive. It maps each kind to its
+    /// *position* in the array and the round trip is asserted, so the arm a new variant
+    /// needs has no position it can honestly name — every index the array has is already
+    /// claimed by another kind — and `ALL_KINDS` and its length annotation have to grow
+    /// with the enum before one is free.
+    ///
+    /// This narrows the door rather than closing it: the loop visits what the array
+    /// holds, so an arm naming a position the array does not have is never evaluated.
+    /// Nothing in the language forces a variant into the array without either an
+    /// unstable `variant_count` or a derive dependency, which `dependable-core` does not
+    /// take.
+    #[test]
+    fn all_kinds_lists_every_variant_once() {
+        let mut seen = Vec::new();
+        for kind in ALL_KINDS {
+            let position = match kind {
+                ManifestKind::CargoToml => 0,
+                ManifestKind::GoMod => 1,
+                ManifestKind::PackageJson => 2,
+                ManifestKind::DenoJson => 3,
+                ManifestKind::PnpmWorkspaceYaml => 4,
+                ManifestKind::ComposerJson => 5,
+                ManifestKind::RequirementsTxt => 6,
+                ManifestKind::PyprojectToml => 7,
+                ManifestKind::PubspecYaml => 8,
+                ManifestKind::MixExs => 9,
+                ManifestKind::Csproj => 10,
+                ManifestKind::GradleVersionCatalog => 11,
+            };
+            assert_eq!(
+                ALL_KINDS.get(position),
+                Some(&kind),
+                "{kind:?} claims position {position} of ALL_KINDS"
+            );
+            assert!(!seen.contains(&kind), "{kind:?} listed twice");
+            seen.push(kind);
+        }
+        assert_eq!(
+            seen.len(),
+            ALL_KINDS.len(),
+            "every position is claimed once"
+        );
+    }
+
     /// Every kind but Cargo declares its versions in place, so the upward walk is
     /// skipped for all of them — the behaviour the boolean gate this replaced had.
     #[test]
     fn only_cargo_looks_for_a_workspace_root() {
         let cargo = ManifestKind::CargoToml
             .workspace_roots()
             .expect("Cargo inherits");
-        assert_eq!(cargo.root_names, ["Cargo.toml"]);
-        assert_eq!(cargo.root_kind, ManifestKind::CargoToml);
+        assert_eq!(cargo.root_names, [("Cargo.toml", ManifestKind::CargoToml)]);
         assert!(cargo.self_governing, "a Cargo root may be a package too");
 
-        for kind in [
-            ManifestKind::GoMod,
-            ManifestKind::PackageJson,
-            ManifestKind::DenoJson,
-            ManifestKind::PnpmWorkspaceYaml,
-            ManifestKind::ComposerJson,
-            ManifestKind::RequirementsTxt,
-            ManifestKind::PyprojectToml,
-            ManifestKind::PubspecYaml,
-            ManifestKind::MixExs,
-            ManifestKind::Csproj,
-            ManifestKind::GradleVersionCatalog,
-        ] {
+        for kind in ALL_KINDS {
+            if kind == ManifestKind::CargoToml {
+                continue;
+            }
             assert!(kind.workspace_roots().is_none(), "{kind:?}");
             assert!(
                 !kind.declares_workspace("[workspace]\nmembers = []\n"),
@@ -500,6 +578,29 @@ mod tests {
         }
     }
 
+    /// The invariant [`WorkspaceRoots::self_governing`] states, checked against every
+    /// descriptor that exists.
+    ///
+    /// A self-governing manifest is accepted as its own root by its own kind's
+    /// `declares_workspace` and then read by its own kind's parser. A kind that claimed
+    /// to govern itself while only ever appearing under a *different* root kind would be
+    /// recognised with one parser and read with another: the read fails, the root
+    /// declares nothing, and every entry inheriting from it is silently reported as
+    /// unresolved rather than as wrong.
+    #[test]
+    fn a_self_governing_kind_is_one_of_its_own_root_kinds() {
+        for kind in ALL_KINDS {
+            let Some(roots) = kind.workspace_roots() else {
+                continue;
+            };
+            assert!(
+                !roots.self_governing || roots.root_names.iter().any(|(_, root)| *root == kind),
+                "{kind:?} governs itself but is not among its own root kinds: {:?}",
+                roots.root_names
+            );
+        }
+    }
+
     /// Recognition has to stay the same predicate the walk used before, or a root
     /// that used to govern a member would be walked straight past.
     #[test]

crates/dependable-fetch/src/cache.rs

@@ -8,7 +8,7 @@ use std::path::PathBuf;
 use std::sync::Arc;
 use std::time::{Duration, SystemTime, UNIX_EPOCH};
 
-use dependable_core::Item;
+use dependable_core::{Item, ManifestKind};
 use moka::future::Cache;
 
 use crate::registries::PackageMetadata;
@@ -56,13 +56,21 @@ pub fn metadata_cache() -> MetadataCache {
         .build()
 }
 
-/// Caches a workspace root's `[workspace.dependencies]`, keyed by the root's path.
+/// Caches a workspace root's central declarations, keyed by the root's path **and** the
+/// kind it was parsed as.
 ///
 /// A monorepo asks the same question once per member — "what does the root declare" —
 /// and the answer is one file read plus a full `toml_edit` parse of bytes that have not
 /// changed. Without this, a 500-crate workspace pays for both 500 times over. The `Arc`
 /// is what makes a hit free rather than a clone of every declaration.
-pub type WorkspaceCache = Cache<PathBuf, Arc<Vec<Item>>>;
+///
+/// The kind is in the key because the path alone does not determine the parser: a name
+/// can be a candidate root for more than one kind
+/// ([`WorkspaceRoots::root_names`](dependable_core::WorkspaceRoots::root_names) pairs
+/// each name with its own), and a root cached under one member's kind would then be
+/// served to a member of the other. That failure is order-dependent — whichever kind was
+/// checked first wins — and so shows up intermittently rather than in a test.
+pub type WorkspaceCache = Cache<(PathBuf, ManifestKind), Arc<Vec<Item>>>;
 
 /// A fresh workspace-declaration cache with a 5-minute TTL, matching the versions cache:
 /// a root edited mid-run is a rare enough thing to be worth one stale answer, and a
@@ -276,6 +284,38 @@ mod tests {
         );
     }
 
+    /// A root's path does not say which parser read it: one name can be a candidate root
+    /// for two kinds, and serving one kind's parse to a member of the other is a wrong
+    /// answer that depends on which member was checked first. Keying on the pair is what
+    /// keeps that from being possible.
+    #[tokio::test]
+    async fn a_root_cached_under_one_kind_is_not_served_to_another() {
+        let cache = workspace_cache();
+        let root = PathBuf::from("/repo/package.json");
+
+        cache
+            .insert(
+                (root.clone(), ManifestKind::PackageJson),
+                Arc::new(Vec::new()),
+            )
+            .await;
+
+        assert!(
+            cache
+                .get(&(root.clone(), ManifestKind::CargoToml))
+                .await
+                .is_none(),
+            "a different kind read the same path and must parse it itself"
+        );
+        assert!(
+            cache
+                .get(&(root, ManifestKind::PackageJson))
+                .await
+                .is_some(),
+            "the kind that wrote the entry still hits"
+        );
+    }
+
     #[test]
     fn cache_root_prefers_xdg_then_localappdata_then_home() {
         // `$XDG_CACHE_HOME` wins outright.

crates/dependable-fetch/src/check.rs

@@ -505,22 +505,27 @@ impl Checker {
     /// difference that the answer is on local disk, so it only ever showed up as CPU. A
     /// 500-crate workspace parsed one root manifest 500 times.
     ///
-    /// Locating the root is still done per manifest: it is the walk that produces the key
-    /// this is cached on, and it is the cheap half.
+    /// Locating the root is still done per manifest: it is the walk that produces the
+    /// `(path, kind)` key this is cached on, and it is the cheap half.
     async fn workspace_source(
         &self,
         path: &Path,
         kind: ManifestKind,
         manifest: &str,
     ) -> Option<(PathBuf, Arc<Vec<Item>>)> {
-        let (root, root_content) = crate::discover::workspace_root_of(path, kind, manifest)?;
-        if let Some(hit) = self.workspace_cache.get(&root).await {
+        let (root, root_kind, root_content) =
+            crate::discover::workspace_root_of(path, kind, manifest)?;
+        // Keyed on the kind as well as the path: the same file can be a candidate root
+        // for two kinds, and the declarations are whatever that kind's parser saw.
+        let key = (root.clone(), root_kind);
+        if let Some(hit) = self.workspace_cache.get(&key).await {
             return Some((root, hit));
         }
-        let declarations = Arc::new(crate::discover::workspace_declarations(kind, &root_content));
-        self.workspace_cache
-            .insert(root.clone(), declarations.clone())
-            .await;
+        let declarations = Arc::new(crate::discover::workspace_declarations(
+            root_kind,
+            &root_content,
+        ));
+        self.workspace_cache.insert(key, declarations.clone()).await;
         Some((root, declarations))
     }