Skip to content

Conversation

@RushabhBhansali
Copy link
Contributor

@RushabhBhansali RushabhBhansali commented Jan 28, 2026

Summary:

This pull request improves the handling of dependency relationships in the NpmLockfile3Detector. It updates the logic to accurately record cases where the same component appears under multiple parents and ensures that circular dependencies are processed without causing infinite loops. This closely matches the npm cli behavior

Dependency graph handling improvements:

  • Updated NpmLockfile3Detector.cs to track component-parent pairs instead of just component IDs, ensuring that the same component appearing under different parents is recorded for each unique relationship. This prevents missing dependencies in cases where a component is shared across multiple parents or involved in circular references. [1] [2]

Test coverage enhancements:

  • Added two new tests in NpmLockfile3DetectorTests.cs:
    • One test verifies that the detector correctly records a component (e.g., c) as a dependency of multiple parents (e.g., both a and b), ensuring both paths are captured in the dependency graph.
    • Another test ensures that circular dependencies (e.g., abca) are handled gracefully without causing infinite loops or crashes, and all relationships are properly recorded.

This addresses the bug #1602

@codecov
Copy link

codecov bot commented Jan 28, 2026

Codecov Report

❌ Patch coverage is 98.37838% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.1%. Comparing base (6a25122) to head (cb55fcf).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...entDetection.Detectors/npm/NpmLockfile3Detector.cs 72.7% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##            main   #1617    +/-   ##
======================================
  Coverage   90.1%   90.1%            
======================================
  Files        435     435            
  Lines      37375   37556   +181     
  Branches    2310    2311     +1     
======================================
+ Hits       33682   33863   +181     
- Misses      3217    3219     +2     
+ Partials     476     474     -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Improves npm lockfile v3 dependency-graph recording so shared components under multiple parents (and cycles) are captured more accurately, aligning behavior more closely with npm CLI.

Changes:

  • Update NpmLockfile3Detector traversal de-duplication to track (component, parent) pairs rather than only component IDs.
  • Add unit tests covering multi-parent dependency paths and circular dependencies.
  • Add a minimal package.json fixture.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/Microsoft.ComponentDetection.Detectors/npm/NpmLockfile3Detector.cs Adjusts traversal bookkeeping to record repeated components under different parents and avoid infinite loops.
test/Microsoft.ComponentDetection.Detectors.Tests/NpmLockfile3DetectorTests.cs Adds tests for multi-parent dependency edges and circular dependency handling.
npm-test/package.json Adds an npm fixture package.json (currently placed at repo root).

Comment on lines 589 to 595
componentRecorder.IsDependencyOfExplicitlyReferencedComponents<NpmComponent>(
componentBId,
parentComponent => parentComponent.Name == componentA.Name);

componentRecorder.IsDependencyOfExplicitlyReferencedComponents<NpmComponent>(
componentCId,
parentComponent => parentComponent.Name == componentA.Name || parentComponent.Name == componentB.Name);
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These calls return a bool but the result isn’t asserted, so the test will pass even if the expected dependency relationships are not recorded. Please assert the return value (e.g., with .Should().BeTrue()) or use an assertion helper that throws on failure.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot open a new pull request to apply changes based on this feedback

Copy link
Contributor

Copilot AI commented Jan 28, 2026

@RushabhBhansali I've opened a new pull request, #1618, to work on those changes. Once the pull request is ready, I'll request review from you.

@github-actions
Copy link

github-actions bot commented Jan 28, 2026

👋 Hi! It looks like you modified some files in the Detectors folder.
You may need to bump the detector versions if any of the following scenarios apply:

  • The detector detects more or fewer components than before
  • The detector generates different parent/child graph relationships than before
  • The detector generates different devDependencies values than before

If none of the above scenarios apply, feel free to ignore this comment 🙂

…alls (#1618)

* Initial plan

* Add assertions to IsDependencyOfExplicitlyReferencedComponents calls in circular dependency test

Co-authored-by: RushabhBhansali <24465841+RushabhBhansali@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: RushabhBhansali <24465841+RushabhBhansali@users.noreply.github.com>
Co-authored-by: Rushabh <rbhansali@microsoft.com>
Copilot AI review requested due to automatic review settings January 28, 2026 23:26
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@RushabhBhansali
Copy link
Contributor Author

snapshot verification failure is expected.

@RushabhBhansali RushabhBhansali merged commit dbff730 into main Jan 29, 2026
30 of 33 checks passed
@RushabhBhansali RushabhBhansali deleted the users/rbhansali/fix-npm-transitive-dependency branch January 29, 2026 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants