[ANE-2852] pnpm v9 lockfile and dev deps#1668
Conversation
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>
WalkthroughbuildGraph now runs hydrateDepEnvs and constructs the graph inside withLabeling applyLabels so environments are assigned via PnpmLabel labels rather than embedded in Dependency values. Direct production and development deps are emitted separately; for PNPM v9 direct deps are labeled EnvProduction or EnvDevelopment. Deep edges use unified toDependency/toResolvedDependency (no PNPM-v9 dev-override). For PNPM v9 toEnv returns mempty. Tests add mkBothEnvDep and new v9 shared, local, and multi-version specs. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Strategy/Node/Pnpm/PnpmLock.hs`:
- Around line 500-514: The two definitions v9ProdDirectNames and
v9DevDirectNames use list comprehensions over toList lockFile.importers and
directDependencies/directDevDependencies; replace each comprehension with a
foldMap (or mconcat . map) to build the Set directly (e.g. foldMap over toList
lockFile.importers, mapping each importer to the Set of its dependency names via
Set.singleton or Set.fromList) so you avoid list comprehensions and produce the
same Set result; update both v9ProdDirectNames and v9DevDirectNames to use
foldMap and reference lockFile.importers, directDependencies and
directDevDependencies accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0e636c47-5b7b-451d-8043-9bda098f58b2
⛔ Files ignored due to path filters (1)
test/Pnpm/testdata/pnpm-9-shared-dep/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
src/Strategy/Node/Pnpm/PnpmLock.hstest/Pnpm/PnpmLockSpec.hs
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Strategy/Node/Pnpm/PnpmLock.hs`:
- Line 307: The buildGraph pipeline currently calls withoutLocalPackages before
maybeHydrate which causes PNPM v9 hydration to run after UserType nodes are
removed; change the composition so maybeHydrate runs first and then
withoutLocalPackages (i.e., reorder the calls in buildGraph to maybeHydrate .
withoutLocalPackages -> withoutLocalPackages . maybeHydrate reversed), and apply
the same reordering to the other affected call sites around the blocks
identified (the similar sequences at the locations corresponding to lines
487-490 and 506-507) so that maybeHydrate (the v9 environment propagation)
always runs prior to dropping UserType/local package nodes.
- Around line 458-462: The current seeding/labeling uses dependencyName only,
which causes cross-version co-labeling and misses URL/git-based direct deps;
update the logic so toDependency / PackageData handling (including GitResolve,
TarballResolve, DirectoryResolve in toDependency) produces a canonical resolved
dependency identity (combine type + resolved name + version where available) and
use that identity as the key instead of dependencyName; then change
labelV9DirectDeps to seed/check using depKey(dep) (i.e., Set.member (depKey
dep)) for prod/dev labeling so git/tarball deps resolve to the same key as
importers and multiple versions are distinct.
- Around line 479-480: The function toEnv currently uses a match guard on isV9;
replace that guard with an explicit if expression: check isV9 first and return
mempty when true, otherwise evaluate the boolean isNotRequired and return
Set.singleton EnvDevelopment if true or Set.singleton EnvProduction if false;
update the toEnv definition and ensure it still references isV9, isNotRequired,
EnvDevelopment and EnvProduction accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: f3ced69d-986b-4698-a6b4-7ebdbf8fe3f1
📒 Files selected for processing (1)
src/Strategy/Node/Pnpm/PnpmLock.hs
- 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>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Strategy/Node/Pnpm/PnpmLock.hs`:
- Around line 489-492: The definition of maybeHydrate uses match guards; replace
them with an if expression to follow guidelines: keep the same type signature
(maybeHydrate :: Graphing Dependency) and change the implementation so it
returns hydrateDepEnvs . labelV9DirectDeps when isV9 is True and id when False,
referencing the existing identifiers isV9, hydrateDepEnvs and labelV9DirectDeps;
ensure semantics remain identical and formatting matches surrounding code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: c55543d9-a48b-47b4-91d2-4b9b63eb61dd
⛔ Files ignored due to path filters (2)
test/Pnpm/testdata/pnpm-9-local-dep/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamltest/Pnpm/testdata/pnpm-9-multi-version/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
src/Strategy/Node/Pnpm/PnpmLock.hstest/Pnpm/PnpmLockSpec.hs
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Strategy/Node/Pnpm/PnpmLock.hs`:
- Around line 458-461: The tests are missing v9 fixtures for non-registry
git/tarball resolutions that exercise the new depIdentity path used by
toDependency (pattern-matching on PackageData with GitResolve and
TarballResolve); add v9-format fixtures in the test/Pnpm/PnpmLockSpec.hs suite
(around lines 100-137) that include GitResolve (GitResolution url rev) and
TarballResolve (TarballResolution url) cases so depIdentity and toDependency
(the branches handling GitType/URLType) are covered for direct non-registry
dependencies; ensure the new fixtures mirror the v9 structure used by
shared/local/multi-version tests and assert the expected depIdentity outputs for
those git/tarball entries.
- Around line 43-44: Remove the unused unqualified import of shrink: the module
currently imports both "shrink" and "Graphing qualified", but all uses call
Graphing.shrink; update the imports by deleting the plain "shrink" from the
import list (leave the qualified Graphing import intact) so the -Wunused-imports
warning is resolved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1577d67e-1f2f-4511-baf3-5886b838e366
📒 Files selected for processing (1)
src/Strategy/Node/Pnpm/PnpmLock.hs
| toDependency _ _ (PackageData isDev _ (GitResolve (GitResolution url rev)) _ _) = | ||
| toDep GitType url (Just rev) isDev | ||
| toDependency _ _ (PackageData isDev _ (TarballResolve (TarballResolution url)) _ _) = | ||
| toDep URLType url Nothing isDev |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add a v9 git/tarball fixture for the new identity path.
depIdentity now relies on the GitResolve and TarballResolve branches here, but test/Pnpm/PnpmLockSpec.hs:100-137 only adds shared/local/multi-version v9 coverage. A direct non-registry case is still unpinned.
As per coding guidelines: **/*.{hs,rs}: "Code should be thoroughly tested with appropriate unit tests"
Also applies to: 495-529
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Strategy/Node/Pnpm/PnpmLock.hs` around lines 458 - 461, The tests are
missing v9 fixtures for non-registry git/tarball resolutions that exercise the
new depIdentity path used by toDependency (pattern-matching on PackageData with
GitResolve and TarballResolve); add v9-format fixtures in the
test/Pnpm/PnpmLockSpec.hs suite (around lines 100-137) that include GitResolve
(GitResolution url rev) and TarballResolve (TarballResolution url) cases so
depIdentity and toDependency (the branches handling GitType/URLType) are covered
for direct non-registry dependencies; ensure the new fixtures mirror the v9
structure used by shared/local/multi-version tests and assert the expected
depIdentity outputs for those git/tarball entries.
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
force-pushed
the
ane-2582-pnpm-lockfile-and-dev-deps
branch
from
fc11bc8 to
232cab1
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Strategy/Node/Pnpm/PnpmLock.hs (1)
429-438:⚠️ Potential issue | 🟠 MajorNon-registry direct seeding can miss v9 local deps when
nameis absent.
toResolvedDependencyseeds non-registry deps with importerdepName(Line 437), butDirectoryResolvewithPackageData.name = Nothinguses the passed name as identity (Line 464-Line 465). In package traversal, that name is the package key; in direct seeding, it is the importer key. Identity mismatch preventslabelV9DirectDepsfrom seeding envs, so hydration can leave downstream deps unlabeled.💡 Proposed fix
- let maybeNonRegistrySrcPackage = - Map.lookup strippedVersion (packages lockFile) - <|> Map.lookup depVersion (packages lockFile) + let maybeNonRegistrySrcPackage = + fmap (strippedVersion,) (Map.lookup strippedVersion (packages lockFile)) + <|> fmap (depVersion,) (Map.lookup depVersion (packages lockFile)) @@ - case (maybeNonRegistrySrcPackage, maybeRegistrySrcPackage) of + case (maybeNonRegistrySrcPackage, maybeRegistrySrcPackage) of (Nothing, Nothing) -> Nothing - (Just nonRegistryPkg, _) -> Just $ toDependency depName Nothing nonRegistryPkg + (Just (pkgKey, nonRegistryPkg), _) -> Just $ toDependency pkgKey Nothing nonRegistryPkg (Nothing, Just (version, registryPkg)) -> Just $ toDependency depName (Just version) registryPkgAlso applies to: 462-465, 501-508
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Strategy/Node/Pnpm/PnpmLock.hs` around lines 429 - 438, The non-registry seeding currently calls toDependency with the importer key (depName), causing identity mismatches when PackageData.name is Nothing; change the toDependency calls for non-registry packages (the branch that uses maybeNonRegistrySrcPackage) to use the package's own name when present (PackageData.name) as the identity argument, falling back to depName only if the package name is absent, and apply the same change to the other similar spots mentioned (around the other toDependency usages at the referenced locations) so DirectoryResolve and labelV9DirectDeps see a consistent package identity.
♻️ Duplicate comments (1)
src/Strategy/Node/Pnpm/PnpmLock.hs (1)
43-44:⚠️ Potential issue | 🟠 MajorRemove the unqualified
shrinkimport to avoid warnings-as-errors breakage.
shrinkis imported unqualified but onlyGraphing.shrinkis used. This will trip-Wunused-importswhen warnings are enforced.As per coding guidelines
**/*.{hs,rs}: "Compiler warnings must be treated as errors and fixed, not ignored" and "Linter warnings must be treated as errors and fixed, not ignored".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Strategy/Node/Pnpm/PnpmLock.hs` around lines 43 - 44, Remove the unqualified import of shrink and rely only on the qualified Graphing import to eliminate the unused-imports warning: delete the standalone "shrink" import and ensure all uses refer to Graphing.shrink (symbols: shrink, Graphing, Graphing.shrink) so the module compiles without warnings-as-errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/Strategy/Node/Pnpm/PnpmLock.hs`:
- Around line 429-438: The non-registry seeding currently calls toDependency
with the importer key (depName), causing identity mismatches when
PackageData.name is Nothing; change the toDependency calls for non-registry
packages (the branch that uses maybeNonRegistrySrcPackage) to use the package's
own name when present (PackageData.name) as the identity argument, falling back
to depName only if the package name is absent, and apply the same change to the
other similar spots mentioned (around the other toDependency usages at the
referenced locations) so DirectoryResolve and labelV9DirectDeps see a consistent
package identity.
---
Duplicate comments:
In `@src/Strategy/Node/Pnpm/PnpmLock.hs`:
- Around line 43-44: Remove the unqualified import of shrink and rely only on
the qualified Graphing import to eliminate the unused-imports warning: delete
the standalone "shrink" import and ensure all uses refer to Graphing.shrink
(symbols: shrink, Graphing, Graphing.shrink) so the module compiles without
warnings-as-errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 13d8a6fd-971b-4ea1-b14d-4f9b5c018213
📒 Files selected for processing (1)
src/Strategy/Node/Pnpm/PnpmLock.hs
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Strategy/Node/Pnpm/PnpmLock.hs (1)
429-438:⚠️ Potential issue | 🟠 MajorPreserve the matched non-registry key for local package identities.
When
DirectoryResolvehas noname, Line 437 builds the direct node from the importer alias, but Lines 464-465 build the package node from the matchedfile:key. Those nodes stop unifying, so v9 hydration labels the local root but never reaches its transitive dependencies beforewithoutLocalPackagesremoves it.💡 Proposed fix
- let maybeNonRegistrySrcPackage = - Map.lookup strippedVersion (packages lockFile) - <|> Map.lookup depVersion (packages lockFile) + let maybeNonRegistrySrcPackage = + (\pkg -> (strippedVersion, pkg)) <$> Map.lookup strippedVersion (packages lockFile) + <|> (\pkg -> (depVersion, pkg)) <$> Map.lookup depVersion (packages lockFile) @@ - (Just nonRegistryPkg, _) -> Just $ toDependency depName Nothing nonRegistryPkg + (Just (resolvedKey, nonRegistryPkg), _) -> Just $ toDependency resolvedKey Nothing nonRegistryPkgAlso applies to: 462-465
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Strategy/Node/Pnpm/PnpmLock.hs` around lines 429 - 438, The code currently discards the actual map key when matching local (non-registry) packages so the created package node uses a different identity than the importer alias; change maybeNonRegistrySrcPackage to capture the matched key like maybeRegistrySrcPackage (e.g. produce fmap (matchedKey, pkg) from Map.lookup using strippedVersion and depVersion), then when handling (Just (key, nonRegistryPkg)) call toDependency with that matched key (pass it similarly to the registry case) instead of Nothing; update the case branches that reference maybeNonRegistrySrcPackage and maybeRegistrySrcPackage to use the new pair shape and pass the preserved key into toDependency (referencing maybeNonRegistrySrcPackage, maybeRegistrySrcPackage, toDependency, mkPkgKey, packages, strippedVersion, depVersion, depName).
♻️ Duplicate comments (1)
src/Strategy/Node/Pnpm/PnpmLock.hs (1)
43-44:⚠️ Potential issue | 🟠 MajorDrop the unused unqualified
shrinkimport.Line 43 still imports
shrink, but the file only callsGraphing.shrinkon Line 533. That warning will fail the build here.As per coding guidelines,
**/*.{hs,rs}: "Compiler warnings must be treated as errors and fixed, not ignored".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Strategy/Node/Pnpm/PnpmLock.hs` around lines 43 - 44, Remove the unused unqualified shrink import: drop shrink from the unqualified import list (the "import Graphing (Graphing, shrink)" entry) so only the needed Graphing symbol(s) remain or remove that unqualified import entirely and rely on the existing "import Graphing qualified" usage; this eliminates the unused `shrink` symbol while preserving calls to Graphing.shrink.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/Strategy/Node/Pnpm/PnpmLock.hs`:
- Around line 429-438: The code currently discards the actual map key when
matching local (non-registry) packages so the created package node uses a
different identity than the importer alias; change maybeNonRegistrySrcPackage to
capture the matched key like maybeRegistrySrcPackage (e.g. produce fmap
(matchedKey, pkg) from Map.lookup using strippedVersion and depVersion), then
when handling (Just (key, nonRegistryPkg)) call toDependency with that matched
key (pass it similarly to the registry case) instead of Nothing; update the case
branches that reference maybeNonRegistrySrcPackage and maybeRegistrySrcPackage
to use the new pair shape and pass the preserved key into toDependency
(referencing maybeNonRegistrySrcPackage, maybeRegistrySrcPackage, toDependency,
mkPkgKey, packages, strippedVersion, depVersion, depName).
---
Duplicate comments:
In `@src/Strategy/Node/Pnpm/PnpmLock.hs`:
- Around line 43-44: Remove the unused unqualified shrink import: drop shrink
from the unqualified import list (the "import Graphing (Graphing, shrink)"
entry) so only the needed Graphing symbol(s) remain or remove that unqualified
import entirely and rely on the existing "import Graphing qualified" usage; this
eliminates the unused `shrink` symbol while preserving calls to Graphing.shrink.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: a2703406-acd1-416c-9bf7-4efa7e742a94
📒 Files selected for processing (1)
src/Strategy/Node/Pnpm/PnpmLock.hs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Strategy/Node/Pnpm/PnpmLock.hs`:
- Around line 510-530: Add v9 fixtures covering non-registry dependencies (git
and tarball) to exercise the depIdentity path handled by
toDependency/toResolvedDependency; create test lockfile/importer entries that
include GitResolve and TarballResolve variants and ensure they appear in
v9ProdDirectDeps and v9DevDirectDeps sets so the depIdentity extraction is
exercised and assertions verify the expected identities and environment labels.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9f6c202f-cebc-4277-a8bf-151c3852157e
📒 Files selected for processing (1)
src/Strategy/Node/Pnpm/PnpmLock.hs
src/Strategy/Node/Pnpm/PnpmLock.hs
Outdated
| v9ProdDirectDeps :: Set.Set (DepType, Text, Maybe VerConstraint) | ||
| v9ProdDirectDeps = | ||
| foldMap | ||
| ( \proj -> | ||
| Set.fromList $ | ||
| mapMaybe | ||
| (\(depName, ProjectMapDepMetadata depVersion) -> depIdentity <$> toResolvedDependency depName depVersion) | ||
| (Map.toList $ directDependencies proj) | ||
| ) | ||
| lockFile.importers | ||
|
|
||
| v9DevDirectDeps :: Set.Set (DepType, Text, Maybe VerConstraint) | ||
| v9DevDirectDeps = | ||
| foldMap | ||
| ( \proj -> | ||
| Set.fromList $ | ||
| mapMaybe | ||
| (\(depName, ProjectMapDepMetadata depVersion) -> depIdentity <$> toResolvedDependency depName depVersion) | ||
| (Map.toList $ directDevDependencies proj) | ||
| ) | ||
| lockFile.importers |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM!
The v9ProdDirectDeps and v9DevDirectDeps sets correctly collect resolved dependency identities from all importers. Using foldMap with mapMaybe cleanly handles multi-workspace projects and skips unresolvable entries. Dependencies appearing in both production and development contexts will correctly receive both environment labels.
Consider adding v9 git/tarball test fixtures.
The depIdentity logic now handles GitResolve and TarballResolve branches via toDependency, but existing tests only cover shared/local/multi-version registry scenarios. A direct non-registry dependency fixture would pin this path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Strategy/Node/Pnpm/PnpmLock.hs` around lines 510 - 530, Add v9 fixtures
covering non-registry dependencies (git and tarball) to exercise the depIdentity
path handled by toDependency/toResolvedDependency; create test lockfile/importer
entries that include GitResolve and TarballResolve variants and ensure they
appear in v9ProdDirectDeps and v9DevDirectDeps sets so the depIdentity
extraction is exercised and assertions verify the expected identities and
environment labels.
src/Strategy/Node/Pnpm/PnpmLock.hs
Outdated
| (\(depName, ProjectMapDepMetadata depVersion) -> depIdentity <$> toResolvedDependency depName depVersion) | ||
| (Map.toList $ directDevDependencies proj) | ||
| ) | ||
| lockFile.importers |
There was a problem hiding this comment.
This looks OK, but if you haven't I'd recommend looking at the Graphing module to see if there are any prebuilt functions, (maybe reachableSuccessorsWithCondition?) that might make the code a bit clearer if you haven't already
There was a problem hiding this comment.
I couldn't find anything direct in the Graphing module, but I did find how YarnV1 does it with withLabeling and changed it to be more like that (https://github.com/fossas/fossa-cli/blob/ane-2582-pnpm-lockfile-and-dev-deps/src/Strategy/Node/YarnV1/YarnLock.hs/#L78-L108)
What do you think? I think it's an improvement. It's nicer that it's iterating over directDependencies and directDevDependencies and setting the environment directly instead of doing it in two passes.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Strategy/Node/Pnpm/PnpmLock.hs (1)
465-468: 🧹 Nitpick | 🔵 TrivialConsider adding v9 git/tarball test fixtures.
The
toDependencyfunction handlesGitResolveandTarballResolvevariants at lines 465-468, but existing tests only cover shared/local/multi-version registry scenarios. A direct non-registry dependency fixture would pin this code path. This was noted in a prior review and remains unaddressed.As per coding guidelines:
**/*.{hs,rs}: "Code should be thoroughly tested with appropriate unit tests"
,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Strategy/Node/Pnpm/PnpmLock.hs` around lines 465 - 468, Add unit tests covering v9 non-registry dependencies to exercise the toDependency branch that handles GitResolve and TarballResolve; create fixtures representing PackageData values with GitResolve (using GitResolution url rev) and TarballResolve (using TarballResolution url) and assert that calling toDependency yields the expected toDep result (GitType with Just rev for git, URLType with Nothing for tarball) and preserves isDev; target the toDependency function and its handling of PackageData/GitResolve/TarballResolve so these code paths are directly validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Strategy/Node/Pnpm/PnpmLock.hs`:
- Around line 465-468: Add unit tests covering v9 non-registry dependencies to
exercise the toDependency branch that handles GitResolve and TarballResolve;
create fixtures representing PackageData values with GitResolve (using
GitResolution url rev) and TarballResolve (using TarballResolution url) and
assert that calling toDependency yields the expected toDep result (GitType with
Just rev for git, URLType with Nothing for tarball) and preserves isDev; target
the toDependency function and its handling of
PackageData/GitResolve/TarballResolve so these code paths are directly
validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: d1e98a2c-afec-4246-8fac-4a4e2393c0f3
📒 Files selected for processing (1)
src/Strategy/Node/Pnpm/PnpmLock.hs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Strategy/Node/Pnpm/PnpmLock.hs (1)
468-471: 🧹 Nitpick | 🔵 TrivialConsider adding v9 git/tarball test fixtures.
The
GitResolveandTarballResolvebranches intoDependencyhandle non-registry dependencies. Based on the PR's test additions (shared/local/multi-version v9 specs), these code paths for direct git/tarball dependencies in v9 lockfiles appear to lack dedicated test coverage.As per coding guidelines:
**/*.{hs,rs}: "Code should be thoroughly tested with appropriate unit tests"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Strategy/Node/Pnpm/PnpmLock.hs` around lines 468 - 471, The GitResolve and TarballResolve branches in toDependency (pattern-matching on PackageData with GitResolution and TarballResolution) lack unit tests for v9 lockfiles; add v9 test fixtures and unit tests that exercise toDependency for non-registry dependencies by creating v9 lockfile fixtures containing git and tarball entries and asserting that toDependency returns the expected toDep results (use GitType/URLType, the GitResolution/TarballResolution shapes, and isDev variations) so the GitResolve and TarballResolve code paths are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Strategy/Node/Pnpm/PnpmLock.hs`:
- Around line 468-471: The GitResolve and TarballResolve branches in
toDependency (pattern-matching on PackageData with GitResolution and
TarballResolution) lack unit tests for v9 lockfiles; add v9 test fixtures and
unit tests that exercise toDependency for non-registry dependencies by creating
v9 lockfile fixtures containing git and tarball entries and asserting that
toDependency returns the expected toDep results (use GitType/URLType, the
GitResolution/TarballResolution shapes, and isDev variations) so the GitResolve
and TarballResolve code paths are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 073a3ac3-e1ab-4244-8ce4-1f851ee022f5
📒 Files selected for processing (1)
src/Strategy/Node/Pnpm/PnpmLock.hs
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Changelog.md`:
- Line 9: Update the capitalization of Apple's OS in the changelog entry: change
the lowercase "macos" in the Changelog.md bugfix line ("Bugfix: revert caching
changes as they caused a problem with missing libs on macos (...)") to the
correct "macOS" so the entry reads "...missing libs on macOS...".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0f955823-01e5-4554-be14-dc25ae7178e2
📒 Files selected for processing (1)
Changelog.md
|
|
||
| ## 3.16.4 | ||
|
|
||
| - Bugfix: revert caching changes as they caused a problem with missing libs on macos ([#1675](https://github.com/fossas/fossa-cli/pull/1675)) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider capitalizing "macOS" for consistency.
The standard capitalization for Apple's operating system is "macOS" rather than "macos".
📝 Suggested capitalization fix
-- Bugfix: revert caching changes as they caused a problem with missing libs on macos ([`#1675`](https://github.com/fossas/fossa-cli/pull/1675))
+- Bugfix: revert caching changes as they caused a problem with missing libs on macOS ([`#1675`](https://github.com/fossas/fossa-cli/pull/1675))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Bugfix: revert caching changes as they caused a problem with missing libs on macos ([#1675](https://github.com/fossas/fossa-cli/pull/1675)) | |
| - Bugfix: revert caching changes as they caused a problem with missing libs on macOS ([`#1675`](https://github.com/fossas/fossa-cli/pull/1675)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Changelog.md` at line 9, Update the capitalization of Apple's OS in the
changelog entry: change the lowercase "macos" in the Changelog.md bugfix line
("Bugfix: revert caching changes as they caused a problem with missing libs on
macos (...)") to the correct "macOS" so the entry reads "...missing libs on
macOS...".
2 participants
Overview
pnpm v9 removed the
dev: booleanflag from the packages section of the lockfile. In v5/v6, every package entry told you directly whether it was a dev dependency:The code relied on this flag via
PackageData.isDev, and had a workaround calledisPnpm9Devthat tried to patch it for v9 by checking if a dependency name appeared indirectDevDependenciesbut notdirectDependenciesacross importers. That only worked for direct deps — any transitive dep of a dev dependency would still getisDev = False(since it's not in any importer's dev list) and be classified as production.This PR replaces the
isPnpm9Devname-matching approach (which only worked for direct deps) with graph-reachability-based environment propagation viahydrateDepEnvs, matching the pattern already used by Yarn V1, Yarn V2, and Poetry strategies.Before: All transitive deps of v9 devDependencies were marked
EnvProduction.After: Transitive deps inherit the environment of their ancestors. Deps reachable from both prod and dev get both
{EnvProduction, EnvDevelopment}.Acceptance criteria
When users run
fossa analyzeon a project with a pnpm v9 lockfile:Testing plan
First, build and install the CLI from this branch:
1. Manual test with a pnpm v9 project
Create a minimal pnpm v9 project with both prod and dev deps:
Verify the lockfile is v9:
head -1 pnpm-lock.yaml # Should show: lockfileVersion: '9.0'Verify in the output JSON that only production deps appear —
fossa analyze --outputexcludes dev deps:uri-jsandpunycodeare present (production deps)xml2js,sax, andxmlbuilderare absent (correctly classified as dev and excluded)2. Regression check with a pnpm v6 project
Verify the lockfile is v6:
head -1 pnpm-lock.yaml # Should show: lockfileVersion: '6.0' or similarVerify that only
uri-jsandpunycodeappear (same as v9). Dev deps (xml2js,sax,xmlbuilder) should be absent.Risks
labelV9DirectDepsmatches nodes bydependencyNamerather than full(name, version)identity. If a lockfile contains two versions of the same package where one is a direct dep and the other is transitive-only, both would be seeded with the direct dep's environment. This is unlikely in practice and errs conservative (over-classifying as prod rather than missing it).optionalDependenciesare not parsed from importers, packages, or snapshots. This is a pre-existing gap across all lockfile versions, not a regression from this PR. Tracked separately.Metrics
N/A
References
hydrateDepEnvspattern from Yarn V2 (src/Strategy/Node/YarnV2/YarnLock.hs)Checklist
docs/.docs/README.msand gave consideration to how discoverable or not my documentation is.Changelog.md. If this PR did not mark a release, I added my changes into an## Unreleasedsection at the top..fossa.ymlorfossa-deps.{json.yml}, I updateddocs/references/files/*.schema.jsonAND I have updated example files used byfossa initcommand. 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).docs/references/subcommands/<subcommand>.md.Follow-up