[RKS (rksharma-owg)]

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 correction 7102ad7a8c8f3525fc3c5941b34a54d28e199365 merged with current main 2f00ab9b7def197310deec187f87ad0c52825f57.

• Test-first proof: 11 new behavioral cases failed on the previous implementation; all 50 focused pnpm tests pass after the correction.
• Full tests including Integration and Cobertura: 1,512 passed and eight existing platform skips on Linux and Windows. Build/analyzers report zero warnings/errors; recorded coverage does not decrease against current main.
• Snapshot verification: all six tests passed with no skips on Linux, Windows, and macOS.
• C# CodeQL completed; downloaded SARIF contains zero findings. The upstream smoke command succeeded across its twelve repositories. Empty fixture lockfiles are rejected as intended; existing malformed-fixture diagnostics remain visible.

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

• Regression tests added or updated
• Build and analyzers passed
• Applicable fork validation completed for the published commit
• Related issue linked

[Copilot]

🟡 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.

[Copilot]

🔵 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 Package and its dev metadata. The later document is therefore not registered, and ProcessDependencyList also 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

[Copilot]

🟡 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

[Copilot]

🟢 Approval recommended

All reviewed changes are covered by tests with no unresolved blocking issues.

Review details

• Files reviewed: 10/10 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Ryan Brandenburg (ryanbrandenburg)]

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.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved Swift API compatibility, detector category compatibility, and public detector type compatibility issues remain.

Review effort: Lite
Findings: 2 · 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…

[RKS (rksharma-owg)]

Ryan Brandenburg (@ryanbrandenburg) could you please approve the upstream CI runs for current head da6bc69, or restart them if needed? All five are action_required with zero jobs executed:

• Build
• Smoke Tests
• Verify snapshot of test scan
• CodeQL
• Zizmor

The upstream build, tests and analysis have not run for this commit.

[Ryan Brandenburg (ryanbrandenburg)]

RKS (@rksharma-owg) it seems we're not able to merge because of some weird state WRT the merge base having "changed" (even though it didn't). Other people who have run into this said that it was fixed when they closed the PR and then immediately reopened it (I guess this makes it re-calculate the commit parentage), could you please try that? It might clean up the diff showing those unrelated changes too.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.0%. Comparing base (2f00ab9) to head (da6bc69).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@     Coverage Diff      @@
##   main   #1871   +/-   ##
============================
============================

☔ View full report in Codecov by Harness.
📢 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.

[github-actions]

👋 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 🙂