fix(cargo): resolve workspace membership by identity, not name alone - #407
fix(cargo): resolve workspace membership by identity, not name alone#407bomly-guy wants to merge 1 commit into
Conversation
A workspace member and an unrelated crate can share a name. The lock workspace path resolved membership through name-keyed maps, so the last same-named lock record won: the member was emitted under the external crate's version with its ResolvedURL, and the external crate vanished from the graph entirely — misstating first-party code and dropping a real dependency from vulnerability matching. Fixes #399: - isProjectLockRecord/projectLockRecord centralize the membership rule: a project's own lock record matches by name, declared version, and the absence of a source (workspace version inheritance may leave the manifest without a version, in which case the source-less name match stands alone). Both lock paths — workspace partitioning and the single-package root — route through it, and only the claimed records are withheld from the ordinary dependency pass, so a same-named external crate keeps its own node. - parseCargoLockPackages keeps full dependency reference strings instead of truncating to the crate name; Cargo.lock qualifies a reference with version (and source) exactly when a bare name would be ambiguous, and truncation resolved every reference to whichever same-named record came first. Direct manifest edges resolve through the root's own lock record refs at the same precision. - The cargo metadata path inserts workspace members before other records, so on an exact name@version collision the project's own package keeps the plain node ID and the external record becomes the qualified occurrence, not the other way around by sort order. Regression coverage in identity_test.go spans both lock paths and the metadata path; scan-cargo and scan-cargo-workspace smoke goldens are unchanged (the pinned fixtures have no colliding or version-ambiguous records). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 26 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
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. Comment |
Bomly Diff SummaryCompared Overview
Dependency Changes✅ No dependency changes. Vulnerabilities✅ No vulnerability changes. License Changes✅ No license changes. Project Posture✅ No project posture changes ( Policy Findings✅ No policy differences were identified. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1c98db37f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if strings.TrimSpace(pkg.Source) != "" { | ||
| return false | ||
| } | ||
| return manifest.Version == "" || pkg.Version == manifest.Version |
There was a problem hiding this comment.
In the lockfile workspace fallback, a member using version.workspace = true is parsed with an empty manifest.Version; if the lockfile also contains a same-named source-less path dependency at another version, this predicate accepts both records and projectLockRecord claims whichever appears first. For example, a workspace helper inheriting version 1.0.0 alongside a path dependency helper 0.1.0 causes the latter to become the application root while the real member is treated as a dependency, corrupting identity and vulnerability matching whenever cargo metadata is unavailable or fails. Resolve the inherited workspace version before matching, or reject ambiguous source-less candidates rather than treating an empty version as a wildcard.
Useful? React with 👍 / 👎.
1 participant
Fixes #399.
What was wrong
depGraphFromLockWorkspaceresolved workspace membership and lock packages through name-keyed maps, so when a workspace member and an unrelated crate shared a name, the last same-named lock record won: the member node was emitted under the external crate's version, carried itsResolvedURL, and the external crate was absent from the graph entirely — first-party code misreported under someone else's identity, and a real dependency lost to vulnerability matching. The single-package lock path had the same collision against the root package's name, and dependency references were truncated to bare crate names, resolving every reference to whichever same-named record came first in the file.The fix
isProjectLockRecord/projectLockRecordinlock_index.go): a project's own lock record matches by name, declared version, and the absence of asource— a workspace member's record never carries one. Workspace version inheritance may leave the member manifest without a version; the source-less name match then stands alone. Both lock paths (workspace partitioning and the single-package root) route through it, and only the claimed records are withheld from the ordinary dependency pass, so a same-named external crate keeps its own node, origin, and edges.parseCargoLockPackageskeeps the whole reference string ("name","name version","name version (source)") instead of the first field. Cargo.lock qualifies a reference exactly when a bare name would be ambiguous, so version-qualified refs now resolve to the exact record (seeTestCargoLockVersionQualifiedDependencyRefsResolveExactly— previously bothhelper 1.0.0andhelper 2.0.0refs resolved to whichever record came first). Direct manifest edges resolve through each application root's own lock record refs at the same precision.EnsureOccurrence); workspace members now insert first, so on an exact name@version collision the project's own package keeps the plain node ID and the external record becomes the qualified occurrence — not the other way around by accident of sort order.Coverage
New
identity_test.goreproduces the issue's exact scenario (memberhelper 0.1.0+ externalhelper 1.0.0from a git remote) on the lock workspace path, plus: workspace version inheritance, the metadata path (different-version and exact name@version collisions), the single-package root collision, and version-qualified reference resolution.TestCargoWorkspaceMemberTakesNoExternalOrigin's "not fixed here" caveat is updated to point at the new tests.Goldens
scan-cargoandscan-cargo-workspacesmoke cases pass unchanged locally — the pinned fixtures hold no colliding names and no version-ambiguous references, so no graph shape moves there.make test, the cargo fuzz target (FuzzDepGraphFromCargoLock, 10s),gofmt, andgo vetare clean.🤖 Generated with Claude Code