Skip to content

Fix pnpm v9 snapshot collisions, optionalDependencies, and env labeling#1669

Draft
spatten wants to merge 20 commits intomasterfrom
pnpm-9-cleanup
Draft

Fix pnpm v9 snapshot collisions, optionalDependencies, and env labeling#1669
spatten wants to merge 20 commits intomasterfrom
pnpm-9-cleanup

Conversation

@spatten
Copy link
Contributor

@spatten spatten commented Mar 14, 2026

Overview

This PR builds on #1668 with additional correctness fixes for pnpm lockfile analysis, based on a spec review of pnpm lockfile v6 and v9 against the parser.

  1. Snapshot peer dep suffix collisions: When multiple peer dependency variants of the same package exist (e.g. button@1.0.0(react@16.0.0) and button@1.0.0(react@17.0.0)), their snapshot dependency lists are now merged using fromListWith (<>) instead of silently dropping one. Previously, HashMap.mapKeys withoutPeerDepSuffix was non-injective — if two snapshot keys mapped to the same stripped key, one overwrote the other non-deterministically.

  2. `optionalDependencies` silently dropped everywhere: The pnpm spec defines optionalDependencies as a valid field in importers, in the packages section (v6), and in snapshots (v9). The parser never read any of them, so edges from packages to their optional dependencies were missing from the graph. This is a pre-existing issue across all lockfile versions, not a v9 regression. Optional deps are treated as production because they may be shipped (e.g., platform-specific packages like fsevents).

  3. Environment labeling correctness (from base PR, included here):

    • Hydration runs before local package removal so transitive deps of workspace packages inherit environments.
    • Dependency identity matching (type + name + version) replaces name-only matching, preventing cross-version co-labeling and correctly handling git/tarball deps.
    • Match guards replaced with if expressions per Haskell style guidelines.

Acceptance criteria

  • When a pnpm v9 lockfile contains packages with multiple peer dependency variants, all transitive dependencies from each variant are included in the analysis.
  • When a pnpm v6 or v9 project declares optionalDependencies, those packages appear in the dependency graph as production dependencies.
  • When a pnpm v9 workspace has a file: local package dependency, the transitive deps of that local package correctly inherit the environment (prod/dev) of the importer.
  • When different workspace packages depend on different versions of the same package in different environments, each version receives only its correct environment label.

Testing plan

First, build and install the CLI from this branch:

make install-dev

Then run each test:

1. Optional dependencies

mkdir /tmp/test-optional-deps && cd /tmp/test-optional-deps
cat > package.json <<'EOF'
{
  "name": "test-optional-deps",
  "version": "1.0.0",
  "dependencies": { "uri-js": "^4.4.1" },
  "optionalDependencies": { "fsevents": "^2.3.3" },
  "devDependencies": { "colorjs": "^0.1.9" }
}
EOF
pnpm install
fossa-dev analyze --output .

Verify that fsevents, uri-js, and punycode appear in the output (fossa-dev analyze --output only shows production deps). colorjs (dev) should be absent. Without this fix, fsevents would be missing entirely.

2. Local dep environment propagation

mkdir -p /tmp/test-local-dep/packages/local-pkg && cd /tmp/test-local-dep
cat > package.json <<'EOF'
{
  "name": "test-local-dep",
  "version": "1.0.0",
  "dependencies": { "local-pkg": "file:packages/local-pkg" }
}
EOF
cat > packages/local-pkg/package.json <<'EOF'
{
  "name": "local-pkg",
  "version": "1.0.0",
  "dependencies": { "sax": "^1.4.1" }
}
EOF
pnpm install
fossa-dev analyze --output .

Verify that sax appears in the pnpm source unit output. Without this fix, transitive deps of file: local packages would lose their environment labels and could be excluded.

3. Multi-version environment labeling

mkdir -p /tmp/test-multi-version/packages/{app-a,app-b} && cd /tmp/test-multi-version
cat > package.json <<'EOF'
{ "name": "test-multi-version", "version": "1.0.0", "private": true }
EOF
cat > pnpm-workspace.yaml <<'EOF'
packages:
  - 'packages/*'
EOF
cat > packages/app-a/package.json <<'EOF'
{ "name": "app-a", "version": "1.0.0", "dependencies": { "sax": "1.2.1" } }
EOF
cat > packages/app-b/package.json <<'EOF'
{ "name": "app-b", "version": "1.0.0", "devDependencies": { "sax": "1.4.1" } }
EOF
pnpm install
fossa-dev analyze --output .

Verify that only sax@1.2.1 appears in the output (production dep from app-a). sax@1.4.1 (dev dep from app-b) should be absent. Without this fix, both versions would be co-labeled as production.

4. Peer dep suffix collision

This scenario requires a lockfile where two snapshot entries share the same base package key but have different peer dep suffixes (e.g. button@1.0.0(react@16.0.0) and button@1.0.0(react@17.0.0)) with different dependency lists. This is difficult to reproduce with pnpm install and is covered by the peer suffix collision unit test with a hand-crafted lockfile fixture.

Risks

This PR fixes both risks noted in #1668:

  • Name-only identity matching: labelV9DirectDeps previously matched by dependencyName only, which could co-label multiple versions of the same package. Now uses full (type, name, version) identity.
  • `optionalDependencies` not parsed: Previously a pre-existing gap across all lockfile versions. Now parsed from importers, packages, and snapshots.

Remaining risks:

  • The fromListWith (<>) merge for snapshot peer dep suffixes assumes that merging dependency lists is the correct behavior when multiple peer variants exist. In practice this is safe because it is equivalent to saying "this package transitively depends on the union of all variants' deps."

Metrics

N/A — correctness fix, no new telemetry needed.

References

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.

spatten and others added 6 commits March 13, 2026 15:58
pnpm lockfile v9 removed the `dev: true/false` flag from packages.
The previous `isPnpm9Dev` only checked direct deps, missing transitive
dev deps entirely. Replace with the `hydrateDepEnvs` pattern (matching
Yarn V1/V2/Poetry): all v9 deps start with empty environments, direct
deps get labeled from importer sections, then labels propagate via
graph reachability. Shared deps correctly get both environments.

- Remove `devOverride` param from `toDependency`/`toResolvedDependency`
- Remove `isPnpm9Dev`, `isPnpm9ProjectDev`, `isPnpm9ProjectDep`
- Add `maybeHydrate`, `labelV9DirectDeps`, `v9ProdDirectNames`, `v9DevDirectNames`
- `toEnv` returns `mempty` for v9 (hydration sets envs instead)
- Non-v9 behavior unchanged (`maybeHydrate = id`, `toEnv` uses `isDev`)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
sax and xmlbuilder are transitive deps of xml2js (a devDependency),
so they should be marked as dev deps, not prod deps.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
New test fixture where sax is a transitive dep of both uri-js (prod)
and xml2js (dev). With hydrateDepEnvs, sax correctly gets both
EnvProduction and EnvDevelopment. punycode stays prod-only, xmlbuilder
stays dev-only.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Reorder buildGraph pipeline to run maybeHydrate before
  withoutLocalPackages, so transitive deps of local (file:) packages
  inherit environment labels before the local nodes are removed.
- Replace name-only matching in labelV9DirectDeps with full dependency
  identity (type + name + version) so multiple versions of the same
  package get distinct environments and git/tarball deps match correctly.
- Replace match guard in toEnv with an if expression per style guidelines.
- Add regression tests for local dep env propagation and multi-version
  env labeling.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@spatten spatten changed the base branch from master to ane-2582-pnpm-lockfile-and-dev-deps March 14, 2026 00:07
@spatten spatten changed the title Pnpm 9 cleanup Fix pnpm v9 snapshot collisions, optionalDependencies, and env labeling Mar 14, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 14, 2026

Caution

Review failed

Failed to post review comments

Walkthrough

This change extends PNPM lockfile parsing to support optional dependencies and introduces PNPM v9-specific environment labeling for direct dependencies. Core updates include expanding the Dependency data constructor with name and version fields, adding optionalDependencies fields to ProjectMap and PackageData, and updating JSON parsing to read these new fields. Graph construction logic incorporates optional dependencies into direct and deep dependency aggregation. PNPM v9 logic introduces environment-aware labeling of direct dependencies via new helpers. Function signatures for toDependency and toResolvedDependency are simplified. Test coverage includes new test data files and test cases validating optional dependency and v9 environment propagation scenarios.

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is empty template boilerplate with no concrete details about the changes, intent, acceptance criteria, or testing steps provided. Fill in all template sections with specific implementation details, acceptance criteria, testing steps, risks, and relevant references for reviewers to understand the changes.
Title check ❓ Inconclusive The title 'Pnpm 9 cleanup' is vague and generic, using non-descriptive language that doesn't convey meaningful information about the changeset's primary objectives. Consider a more specific title that captures the main change, such as 'Add optional dependencies support and refactor pnpm v9 environment labeling' or similar.
✅ Passed checks (1 passed)
Check name Status Explanation
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.

📝 Coding Plan
  • Generate coding plan for human review comments

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

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

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
spatten and others added 3 commits March 16, 2026 11:23
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
spatten and others added 9 commits March 16, 2026 11:54
When two snapshot entries share the same base key but differ in peer dep
suffix (e.g. button@1.0.0(react@16) vs button@1.0.0(react@17)),
HashMap.mapKeys silently drops one entry. Use fromListWith (<>) to merge
their dependency lists instead.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Test verifies that when two snapshot entries share the same base key
(e.g. button@1.0.0(react@16) and button@1.0.0(react@17)), their
dependency lists are merged rather than one silently dropped.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Parse optionalDependencies from importer sections and the non-workspace
fallback. Optional direct deps are included in the graph as production
dependencies alongside regular dependencies, and added to
v9ProdDirectNames for correct environment labeling in v9 lockfiles.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Parse optionalDependencies from the packages section (v6) and include
them as edges in the dependency graph alongside regular and peer
dependencies. This ensures transitive optional deps like fsevents are
not silently dropped from the graph.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Parse optionalDependencies from snapshot entries alongside regular
dependencies, so transitive optional deps are included in the v9
dependency graph.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Test fixtures verify that:
- Optional direct deps appear as production direct dependencies
- Edges from packages to their optional deps are present (v6 via
  PackageData, v9 via snapshot optionalDependencies)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… fixes

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Base automatically changed from ane-2582-pnpm-lockfile-and-dev-deps to master March 20, 2026 19:31
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.

1 participant