Cargo: Fix transitive dev-dependency classification

[spatten]

Overview

The cargo strategy was labeling transitive dependencies by the kind of the single edge directly pointing at them. This caused deps reachable only via a dev-dep subtree to be reported as Production whenever the intermediate edges happened to be normal-kind — for example workspace → approx (dev) → num-traits (normal) leaked num-traits into Production.

The fix in src/Strategy/Cargo.hs replaces per-edge labeling with two edge-kind-aware reachability passes over the resolved graph:

• Production: reachable from workspace members through paths of null-kind edges only. A "build" or "dev" edge breaks the release chain, so its target (and everything transitively below) is not Production via that path.
• Development: reachable (via any edge) from the target of any non-null-kind edge. A build or dev edge marks the root of a subtree that is never linked into the release binary, and every descendant of that subtree inherits Development.

A package reachable by both kinds of paths carries both labels — LabeledGrapher already supports multi-labels, so no structural changes were needed.

Why we don't need a separate rule for transitive dev-deps

Cargo only resolves [dev-dependencies] for workspace members. Dev-deps declared inside a non-workspace crate are not part of cargo metadata's resolve graph, so the only non-null kind we ever see on a non-workspace edge is "build". The reachability rule above handles it uniformly.

Acceptance criteria

• Transitive deps reached only via dev-dep or build-dep roots are labeled Development (previously leaked to Production when intermediate edges were normal-kind).
• Build-dependencies of production deps (e.g. autocfg, cc, version_check) and their transitives are labeled Development — they're needed to compile the project but are never linked into the release artifact.
• A dep reachable from both a Production root and a Development root gets both labels (multi-label preserved).
• Projects with non-trivial dev-dep or build-dep trees will see their Production set shrink and their Development set grow. This is a correctness fix but is a visible behavior change, called out in the changelog.

Testing plan

Minimal reproduction

The Cargo.toml includes two dependencies. itoa has no transitive deps. It is included as a prod dependency, so it should be the only production dependency in the graph.

approx is being included as a dev dependency. It has two transitive dependencies: approx requires num-traits as a normal dependency, and num-traits requires autocfg as a build-script dependency.

Before this fix, fossa analyze reported both itoa and num-traits as production dependencies. After the fix, it correctly reports only itoa as a production dependency.

1. Create /tmp/cargo-devdep-test/Cargo.toml:

   [package]
   name = "cargo-devdep-test"
   version = "0.1.0"
   edition = "2021"
   
   [dependencies]
   itoa = "1"
   
   [dev-dependencies]
   approx = "0.5"

2. Create /tmp/cargo-devdep-test/src/lib.rs:

   pub fn add(a: u64, b: u64) -> u64 { a + b }

3. Generate the lockfile:

   cd /tmp/cargo-devdep-test && cargo generate-lockfile

4. Run fossa (released) and fossa-dev (this branch):

   cd /tmp/cargo-devdep-test
   fossa analyze --output | jq
   make install-dev
   fossa-dev analyze --output | jq

Compare the two outputs. The output from fossa-dev only includes cargo+itoa. The output from fossa includes both cargo+itoa and cargo+num-traits.

Run again with --include-unused-deps. All four deps appear in the graph; the difference is that approx, num-traits, and autocfg are now labeled Development instead of leaking into Production.

Real-world verification: sparkle and ficus

This is where the build-dep correctness is visible — the minimal num-traits-style repro above doesn't differentiate the fix from released fossa (both classify autocfg as Development, just by different paths). The sparkle/ficus scans are the evidence that the fix handles the build-dep case at scale without regressing any Production deps.

I scanned both projects with released fossa (3.16.6) and fossa-dev from this branch, then diffed sourceUnits.Build.Dependencies:

Project	Released	This PR	Delta
ficus	370	355	−15
sparkle	724	693	−31

All movement is Production → Development — no deps incorrectly leaked the other direction. The reclassified deps are exactly the categories you'd expect:

• Test frameworks: proptest, wiremock, tokio-test, axum-test, simple_test_case, test-log, tracing-test-macro, assert-json-diff
• Build-script utilities: autocfg, cc, cfg_aliases, pkg-config, rustc_version, vcpkg, version_check, jobserver, shlex, find-msvc-tools
• Dev utilities: xshell, xshell-macros, cargo_metadata, cargo-platform, env_logger, env_filter, tempfile
• Platform-specific deps of dev tooling: winapi-i686-pc-windows-gnu, winapi-x86_64-pc-windows-gnu, hermit-abi

Automated coverage

New MetadataSpec cases:

• Transitive deps of dev-deps labeled Development (the original bug).
• Dep reachable from both prod and dev roots gets both labels.
• Direct dep declared as both normal and dev gets both labels.
• Build-deps of prod-deps and their transitives are Development (catches the regression that a naïve propagation approach would reintroduce).
• Dep reachable via a normal prod path and a build-edge gets both labels (multi-label under the edge-kind-aware algorithm).

Risks

Correctness fix with a visible behavior change: Production dep sets will shrink for any project that has dev-dep or build-dep trees with non-trivial transitive closures. This was the intent — previously those trees were partially leaking into Production. Flagged in the changelog. No deps move in the opposite direction (Dev → Prod), so there's no risk of over-reporting.

Metrics

Not tracked.

References

• Plan: cargo-dev-prod-labeling-fix.md

Checklist

• I added tests for this PR's change (or explained in the PR description why tests don't make sense).
• If this PR introduced a user-visible change, I added documentation into docs/.
• If this PR added docs, I added links as appropriate to the user manual's ToC in docs/README.ms and gave consideration to how discoverable or not my documentation is.
• If this change is externally visible, I updated Changelog.md. If this PR did not mark a release, I added my changes into an ## Unreleased section at the top.
• If I made changes to .fossa.yml or fossa-deps.{json.yml}, I updated docs/references/files/*.schema.json AND I have updated example files used by fossa init command. You may also need to update these if you have added/removed new dependency type (e.g. pip) or analysis target type (e.g. poetry).
• If I made changes to a subcommand's options, I updated docs/references/subcommands/<subcommand>.md.

[coderabbitai]

Walkthrough

This pull request refactors the Cargo strategy to correctly classify dependencies reachable only via dev-dependencies or build-dependencies as "Development" rather than "Production". The implementation replaces per-edge labeling with a graph reachability analysis approach. It introduces adjacency maps to distinguish production-only paths from paths including development dependencies, computes reachable node sets for each classification, and labels packages based on their membership in these sets. A package can receive both labels when reachable via both path types. Test coverage is added for transitive dev-dependency labeling scenarios and packages with dual reachability paths.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely summarizes the main fix: correcting how transitive dev-dependencies are classified in the Cargo strategy.
Description check	✅ Passed	The description comprehensively covers all required template sections: overview, acceptance criteria, testing plan with concrete steps, risks, and references. All checklist items are marked complete.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/Strategy/Cargo.hs`:
- Around line 560-568: The function reachable uses guarded equations in the
helper go (the clause "go visited (x : xs) | Set.member x visited = ..." and the
otherwise branch); replace that guard-based pattern with a single if-then-else
expression inside the "go visited (x : xs)" equation: compute children as
before, then use if Set.member x visited then go visited xs else go (Set.insert
x visited) (children ++ xs). Keep the same names (reachable, go, visited,
children) and preserve semantics and types.

In `@test/Cargo/MetadataSpec.hs`:
- Around line 209-340: Add a new test in MetadataSpec (near
devDepTransitiveLabelingSpec) that constructs a multi-member workspace using
CargoMetadata with two workspace members (e.g., member-a and member-b) where
member-a has a NodeDependency on member-b and member-b depends on an external
crate; call buildGraph False meta and assert that the external crate appears in
expectDeps/expectDirect but the internal workspace member (member-b) does not
surface as an external dependency. Use the existing helpers/constructors from
the file (PackageId, mkPkgId, ResolveNode, NodeDependency, devKind/nullKind,
mkDep, expectDeps, expectDirect) so the test covers the regression of labeling
before shrinkRoots.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro

Run ID: 86ddddd6-6146-422b-9f41-2800c9ccba59

📥 Commits

Reviewing files that changed from the base of the PR and between d855f0e and db4fe4e.

📒 Files selected for processing (3)

• Changelog.md
• src/Strategy/Cargo.hs
• test/Cargo/MetadataSpec.hs

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Check if the file exists and examine the specific lines
if [ -f "src/Strategy/Cargo.hs" ]; then
  echo "=== File exists. Checking lines 560-568 ==="
  sed -n '560,568p' src/Strategy/Cargo.hs
else
  echo "File not found at src/Strategy/Cargo.hs"
  # Try to find it
  find . -name "Cargo.hs" -type f 2>/dev/null | head -5
fi

Repository: fossas/fossa-cli

Length of output: 461

---

🏁 Script executed:

# Get more context around lines 560-568
sed -n '555,575p' src/Strategy/Cargo.hs

Repository: fossas/fossa-cli

Length of output: 765

---

🏁 Script executed:

# Check the overall structure of the file to understand imports and pattern
head -100 src/Strategy/Cargo.hs | tail -50

Repository: fossas/fossa-cli

Length of output: 1368

---

🏁 Script executed:

# Search for any Haskell coding guidelines in the repo
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "GUIDELINES*" -o -name ".editorconfig" \) | xargs grep -l "match guard\|pattern guard\|avoid.*guard" 2>/dev/null | head -10

Repository: fossas/fossa-cli

Length of output: 106

---

🏁 Script executed:

# Check the coding guidelines files
cat ./CLAUDE.md

Repository: fossas/fossa-cli

Length of output: 1497

---

🏁 Script executed:

# Also check the style guide
cat ./docs/contributing/STYLE-GUIDE.md

Repository: fossas/fossa-cli

Length of output: 12249

---

🏁 Script executed:

# Verify the proposed fix would be syntactically correct by checking if similar patterns exist elsewhere
rg "if.*then.*else" src/Strategy/Cargo.hs | head -10

Repository: fossas/fossa-cli

Length of output: 42

---

Replace the guarded equations in reachable with an if-then-else expression.

This code uses match guards, which the repository explicitly avoids per the style guide. Replace with conditional expressions as shown below.

♻️ Proposed fix

 reachable :: Map.Map PackageId [PackageId] -> Set PackageId -> Set PackageId
 reachable adj = go Set.empty . Set.toList
   where
     go visited [] = visited
-    go visited (x : xs)
-      | Set.member x visited = go visited xs
-      | otherwise =
-          let children = fromMaybe [] (Map.lookup x adj)
-           in go (Set.insert x visited) (children ++ xs)
+    go visited (x : xs) =
+      if Set.member x visited
+        then go visited xs
+        else
+          let children = fromMaybe [] (Map.lookup x adj)
+           in go (Set.insert x visited) (children ++ xs)

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	reachable :: Map.Map PackageId [PackageId] -> Set PackageId -> Set PackageId
	reachable adj = go Set.empty . Set.toList
	where
	go visited [] = visited
	go visited (x : xs)
	| Set.member x visited = go visited xs
	| otherwise =
	let children = fromMaybe [] (Map.lookup x adj)
	in go (Set.insert x visited) (children ++ xs)
	reachable :: Map.Map PackageId [PackageId] -> Set PackageId -> Set PackageId
	reachable adj = go Set.empty . Set.toList
	where
	go visited [] = visited
	go visited (x : xs) =
	if Set.member x visited
	then go visited xs
	else
	let children = fromMaybe [] (Map.lookup x adj)
	in go (Set.insert x visited) (children ++ xs)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Strategy/Cargo.hs` around lines 560 - 568, The function reachable uses
guarded equations in the helper go (the clause "go visited (x : xs) | Set.member
x visited = ..." and the otherwise branch); replace that guard-based pattern
with a single if-then-else expression inside the "go visited (x : xs)" equation:
compute children as before, then use if Set.member x visited then go visited xs
else go (Set.insert x visited) (children ++ xs). Keep the same names (reachable,
go, visited, children) and preserve semantics and types.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

Add a multi-member workspace regression case.

These scenarios all use a single workspace root. Since buildGraph now labels workspace members before shrinkRoots, please add a member-a -> member-b -> dep case to ensure internal workspace crates still do not surface as dependencies.

🧪 Suggested coverage

     Test.it "dep reachable via a normal prod path and a build-edge gets both labels" $ do
       -- X is a direct normal dep of the workspace (Production) AND a build-dep
       -- of a prod dep (Development via the build-edge split). Both labels
       -- should be applied.
@@
       expectDeps [aDep, xDep] graph
       expectDirect [aDep, xDep] graph
+
+    Test.it "workspace members do not surface as dependencies" $ do
+      let appId = PackageId "app" "0.1.0" "path+file:///path/to/app"
+          libId = PackageId "lib" "0.1.0" "path+file:///path/to/lib"
+          depId = mkPkgId "dep" "1.0.0"
+
+          appNode = ResolveNode appId [NodeDependency libId [nullKind]]
+          libNode = ResolveNode libId [NodeDependency depId [nullKind]]
+          depNode = ResolveNode depId []
+
+          meta = CargoMetadata [] [appId, libId] $ Resolve [appNode, libNode, depNode]
+          graph = buildGraph False meta
+
+          dep = mkDep "dep" "1.0.0" CargoType [EnvProduction]
+          libDep = mkDep "/path/to/lib" "0.1.0" UnresolvedPathType [EnvProduction]
+
+      expectDeps [dep] graph
+      expectDirect [dep] graph
+      Graphing.vertexList graph `shouldSatisfy` notElem libDep

As per coding guidelines, **/*.{hs,rs}: Code should be thoroughly tested with appropriate unit tests.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/Cargo/MetadataSpec.hs` around lines 209 - 340, Add a new test in
MetadataSpec (near devDepTransitiveLabelingSpec) that constructs a multi-member
workspace using CargoMetadata with two workspace members (e.g., member-a and
member-b) where member-a has a NodeDependency on member-b and member-b depends
on an external crate; call buildGraph False meta and assert that the external
crate appears in expectDeps/expectDirect but the internal workspace member
(member-b) does not surface as an external dependency. Use the existing
helpers/constructors from the file (PackageId, mkPkgId, ResolveNode,
NodeDependency, devKind/nullKind, mkDep, expectDeps, expectDirect) so the test
covers the regression of labeling before shrinkRoots.