Repository navigation
Support multi-document pnpm lockfiles - #1871
RKS (rksharma-owg) wants to merge 16 commits into
Conversation
- Use YamlDotNet Parser to deserialize all documents from multi-document pnpm lockfiles - Validate lockfile version consistency across documents, failing clearly on conflicting versions - Record components and dependencies across all documents in Pnpm9, Pnpm6, and Pnpm5 detectors - Support packageManagerDependencies and configDependencies from environment lockfile documents - Bump PnpmComponentDetectorFactory version to 9
There was a problem hiding this comment.
🟡 Changes recommended
Add dependency-edge assertions and focused v5 multi-document test coverage.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds multi-document pnpm lockfile support, version validation, cross-document dependency handling, and related tests.
Changes:
- Parses and validates all YAML documents.
- Aggregates components and dependency metadata across documents.
- Adds environment dependency fields for v6 and v9.
- Updates v5/v6/v9 detectors and bumps the detector version.
File summaries
| File | Summary |
|---|---|
test/Microsoft.ComponentDetection.Detectors.Tests/PnpmParsingUtilitiesTest.cs |
Tests multi-document parsing and version validation. |
test/Microsoft.ComponentDetection.Detectors.Tests/PnpmDetectorTests.cs |
Tests multi-document detector scenarios. |
src/Microsoft.ComponentDetection.Detectors/pnpm/PnpmComponentDetectorFactory.cs |
Bumps the detector version. |
src/Microsoft.ComponentDetection.Detectors/pnpm/Pnpm9Detector.cs |
Processes v9 documents and environment dependencies. |
src/Microsoft.ComponentDetection.Detectors/pnpm/Pnpm6Detector.cs |
Processes v6 documents and dependencies. |
src/Microsoft.ComponentDetection.Detectors/pnpm/Pnpm5Detector.cs |
Processes v5 documents. |
src/Microsoft.ComponentDetection.Detectors/pnpm/ParsingUtilities/PnpmParsingUtilitiesFactory.cs |
Parses versions across YAML documents. |
src/Microsoft.ComponentDetection.Detectors/pnpm/ParsingUtilities/PnpmParsingUtilitiesBase.cs |
Adds multi-document deserialization. |
src/Microsoft.ComponentDetection.Detectors/pnpm/Contracts/V9/PnpmHasDependenciesV9.cs |
Adds v9 environment dependency properties. |
src/Microsoft.ComponentDetection.Detectors/pnpm/Contracts/V6/PnpmHasDependenciesV6.cs |
Adds v6 environment dependency properties. |
Review details
Suppressed comments (2)
src/Microsoft.ComponentDetection.Detectors/pnpm/Pnpm5Detector.cs:19
- This changes the v5 detector to consume every YAML document, but there is no multi-document v5 detector test. Add a focused fixture that verifies components and dependency edges from both documents; otherwise a regression in this new loop would not be detected.
foreach (var yaml in yamls)
{
foreach (var packageKeyValue in yaml?.Packages ?? Enumerable.Empty<KeyValuePair<string, Package>>())
src/Microsoft.ComponentDetection.Detectors/pnpm/Pnpm6Detector.cs:75
- The v6 multi-document test only checks component count and names. Since packages are registered in the earlier pass, it would pass even if this new importer dependency-processing loop were removed; add assertions for explicit references from both documents to cover the behavior introduced here.
foreach (var yaml in yamls)
{
// "dedicated shrinkwrap" (single package) case:
this.ProcessDependencySet(singleFileComponentRecorder, components, yaml);
// "shared shrinkwrap" (workspace / mono-repos) case:
foreach (var (_, package) in yaml.Importers ?? Enumerable.Empty<KeyValuePair<string, PnpmHasDependenciesV6>>())
{
this.ProcessDependencySet(singleFileComponentRecorder, components, package);
}
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| this.ProcessDependencyList(singleFileComponentRecorder, components, item.PackageManagerDependencies); | ||
| this.ProcessDependencyList(singleFileComponentRecorder, components, item.ConfigDependencies); |
| foreach (var yaml in yamls) | ||
| { | ||
| this.ProcessDependencySets(singleFileComponentRecorder, components, package); | ||
| foreach (var (_, package) in yaml.Importers ?? Enumerable.Empty<KeyValuePair<string, PnpmHasDependenciesV9>>()) | ||
| { | ||
| this.ProcessDependencySets(singleFileComponentRecorder, components, package); |
There was a problem hiding this comment.
🔵 Needs a closer look
Fix duplicate v6 package-path handling across documents before approval.
Review details
Suppressed comments (1)
src/Microsoft.ComponentDetection.Detectors/pnpm/Pnpm6Detector.cs:46
- When the same v6 package path occurs in both YAML documents, this guard keeps only the first document's
Packageand itsdevmetadata. The later document is therefore not registered, andProcessDependencyListalso looks up the first tuple, so an environment/project overlap that is production in one document and development in the other can be classified incorrectly (and later package metadata/dependency edges are lost). Preserve/merge package metadata per document rather than dropping repeated paths; in particular, the effective dev value should let a production occurrence win.
if (!components.ContainsKey(pnpmDependencyPath))
{
var parentDetectedComponent = this.pnpmParsingUtilities.CreateDetectedComponentFromPnpmPath(pnpmPackagePath: pnpmDependencyPath);
components.Add(pnpmDependencyPath, (parentDetectedComponent, package));
// Register the component.
// It should get registered again with with additional information (what depended on it) later,
// but registering it now ensures nothing is missed due to a limitation in dependency traversal
// like skipping local dependencies which might have transitively depended on this.
singleFileComponentRecorder.RegisterUsage(parentDetectedComponent, isDevelopmentDependency: this.pnpmParsingUtilities.IsPnpmPackageDevDependency(package));
}
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
…pendencies assertions - In Pnpm6Detector, merge package metadata across documents so production occurrences take precedence over development occurrences, and merge declared child dependencies across documents. - In Pnpm9Detector, merge snapshot dependencies across documents when the same snapshot path occurs across multiple documents. - In PnpmDetectorTests, exercise non-empty configDependencies for v6 and v9 multi-document scenarios and assert explicit reference status. - Add TestPnpmDetector_V6_MultiDocumentLockfile_DuplicatePackage_ProductionWinsAsync to verify dev vs prod resolution and merged dependency edges.
There was a problem hiding this comment.
🟡 Changes recommended
Critical unresolved dependency references may be silently dropped in the pnpm v6 and v9 detectors.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
src/Microsoft.ComponentDetection.Detectors/pnpm/Pnpm6Detector.cs:130
- A missing dependency path is now ignored, so a non-local dependency that cannot be resolved from any document is omitted while the detector still succeeds. The
file:/link:cases are filtered immediately above, and the old indexed lookup intentionally surfaced other misses; please retain that failure or report the unresolved reference instead of silently producing an incomplete graph.
if (!components.TryGetValue(pnpmDependencyPath, out var componentAndPackage))
{
continue;
src/Microsoft.ComponentDetection.Detectors/pnpm/Pnpm9Detector.cs:161
- This change makes unresolved non-local transitive dependencies disappear silently. The
file:,link:, and URL cases are filtered before this lookup, so a miss here means the graph cannot resolve a declared dependency; returning success without the edge causes an incomplete SBOM. Please keep the previous failing lookup (or explicitly surface unresolved references) rather than continuing.
if (!components.TryGetValue(pnpmDependencyPath, out var componentAndPackage))
{
continue;
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Lite
…d v9 Retain direct indexed dictionary lookup components[pnpmDependencyPath] when walking non-local package and indirect dependencies in Pnpm6Detector and Pnpm9Detector so that unresolved references in malformed lockfiles continue to fail fast rather than silently producing an incomplete dependency graph.
* Promote MSBuildBinaryLog Detector to prod
* Make Swift Detector experimental * Expand Package.resolved data reporting * Serialization constructor * Fix test constructors * Fix serialization issue * PR feedback * Emit SwiftComponent afterall * Fix test expectations * Make the SwiftComponent Id truely representative * Don't include extranious ID information
* Fix Go direct dependency classification Mark go.mod requirements as explicitly referenced unless they carry the // indirect marker. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b9f67746-b363-43ac-9c8c-764b1890c63c * Bump Go detector version Increment the detector version because explicit-reference metadata now changes for go.mod dependencies. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b9f67746-b363-43ac-9c8c-764b1890c63c --------- Co-authored-by: Aayush Maini <aamaini@microsoft.com> Copilot-Session: b9f67746-b363-43ac-9c8c-764b1890c63c
* Fix Cargo.lock Git source dependency matching Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Skip fork-only detector reminder failure and bump Rust detector version Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Don't report Pip for conda component * Cleanup * Fix outdated comment
* Resolve Maven properties in component coordinates Resolve Maven property references in group IDs and artifact IDs as well as versions when using static POM parsing. Skip components whose coordinates remain unresolved and bump the detector version for the output change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Resolve nested Maven property references Recursively resolve coordinate properties with memoization and cycle detection. Add coverage for nested local and inherited properties and cyclic definitions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Bound Maven property expansion depth Limit recursive coordinate property expansion to prevent deeply nested POM properties from exhausting the process stack. Leave over-depth coordinates unresolved so they are skipped safely. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Reject malformed Maven property references Treat any residual Maven interpolation marker as unresolved so malformed references are skipped rather than emitted as component coordinates. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Aayush Maini <aamaini@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Add exception handling for non-Dockerfile formats and add tests for non-dockerfiles * Don't check for dockerfiles in `node_modules` or subfolders of it. * Add logging verification for Shiki language definition test
* Swift relies on Version, not hash * PR feedback
Ryan Brandenburg (ryanbrandenburg)
left a comment
There was a problem hiding this comment.
Some questions and comments. I don't have a pnpm background so some of this may just be me misunderstanding the intricacies, feel free to correct me with some docs if so.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved Swift API compatibility, detector category compatibility, and public detector type compatibility issues remain.
Review effort: Lite
Findings: 2
Open (4)
Preserve the public SwiftComponent constructor for compatibility · New Retain public detector types for API compatibility · New The new multi-document v9 test only checks that both components exist, but both are registered… Could we add a graph assertion for the environment-only dependency lists (including a non-empty…
| public class SwiftComponent : TypedComponent | ||
| { | ||
| private readonly Uri packageUrl; | ||
| [JsonPropertyName("name")] |
| services.AddSingleton<IComponentDetector, NuGetComponentDetector>(); | ||
| services.AddSingleton<IComponentDetector, NuGetPackagesConfigDetector>(); | ||
| services.AddSingleton<IComponentDetector, NuGetProjectModelProjectCentricComponentDetector>(); | ||
| services.AddSingleton<IComponentDetector, MSBuildBinaryLogComponentDetector>(); |


Description
Valid pnpm lockfiles can contain an environment document followed by a project document. The single-document parser rejects the second document and skips the lockfile. Both inventories are now parsed and their components and dependency references recorded.
The shared parser accepts one or two nonempty documents in source order and returns an ordered read-only collection. Version detection uses that parser and rejects inconsistent declared versions; the unused internal single-document method is removed. Tests verify explicit environment dependencies and duplicate-package production classification and graph edges across v5/v6/v9; the new v5/v9 controls cover both document orders. Legacy versionless shrinkwrap handling is retained. Detector version is incremented to 9.
Fixes #1864.
Validation
Validated publication commit
da6bc69d33a7dc0990fd53cdc1ec91bbf323441e, containing correction7102ad7a8c8f3525fc3c5941b34a54d28e199365merged with current main2f00ab9b7def197310deec187f87ad0c52825f57.Older-base snapshot failures involve four unrelated DotNet/NuGet components and reproduce identically on the untouched previous PR head. The published current-main merge passes all snapshot checks. Full local Apple Silicon tests retain one Vcpkg path-normalization failure, also reproduced on untouched current main; required hosted Linux/Windows tests pass. The fork runs contain those older-base/control failures, so the successful final-commit jobs are linked individually. Upstream review, Codecov status, and protected orchestration remain pending.
Checklist