feat(lockfile): add BlockLocation + LocationRole=lockfile to npm-lock (pilot)#141
Merged
Conversation
The npm-lock extractor previously set FilePosition on intermediate types (NpmLockPackage/NpmLockDependency) via InJSON but never transferred those positions to PackageDetails.BlockLocation. This meant transitive deps had no lockfile position data. Changes: - Thread sourceFile parameter through all npm-lock processing functions - Copy FilePosition to BlockLocation in both processPackage() and parseNpmLockDependencies() - Fix matcher de-duplication check in match-package-json.go and match-composer.go: compare BlockLocation.Filename against the source file path instead of checking Line.Start != 0, since extractors now set BlockLocation from the lockfile for all packages - Add BlockLocation assertion tests for npm v1 and v2 Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Switch strict ExpectPackages to ExpectPackagesWithoutLocations in tests that are not specifically testing location data. Dedicated BlockLocation assertion tests (v1 and v2) verify positions are set. Update WorkspacesComplex test to expect lockfile-sourced BlockLocation for the transitive dep [email protected]. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Two fixes for non-deterministic output after adding lockfile positions: - Sort evidence occurrences by location string in cyclonedx.go so that packages with multiple locations (lockfile + source file) always emit occurrences in a consistent order. - In npmPackageDetailsMap.add(), prefer the earlier lockfile position when the same name@version appears at multiple node_modules paths, ensuring deterministic location selection regardless of map iteration order. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…e main - Add LocationRole to ignored fields in HasPackage when ignoreLocations=true - Add LocationRole: lockfile to transitive package test expectations - Use assert.Positive instead of assert.Greater per linter - Regenerate integration test snapshots Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
This comment has been minimized.
This comment has been minimized.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 561a8d604e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (depMap.RootType == typeDevDependencies || depMap.RootType == typeOptionalDependencies) && pkg.BlockLocation.Line.Start != 0 { | ||
| // If it is a dev or optional dependency definition and we already found a package location, | ||
| // we skip it to prioritize non-dev dependencies | ||
| if (depMap.RootType == typeDevDependencies || depMap.RootType == typeOptionalDependencies) && pkg.BlockLocation.Filename == depMap.FilePath { |
There was a problem hiding this comment.
In workspace projects, Match builds packagesWithoutKnownLocationIndices before root matching, then still passes those same packages to every workspace matcher. If a root dependency has already been matched in the root package.json and a workspace package.json also lists the same name/version under devDependencies or optionalDependencies, this filename-only guard is false for the workspace file, so UpdatePackageDetails overwrites the root prod manifest location with the workspace dev/optional location. The old Line.Start check avoided that; please distinguish lockfile locations from any prior manifest location, not just same-file ones.
Useful? React with 👍 / 👎.
…write Replace the BlockLocation.Filename==depMap.FilePath check with LocationRole==manifest so that a root package.json prod-dependency match is preserved when a workspace package.json later processes the same package under devDependencies or optionalDependencies. The old Line.Start!=0 check implicitly protected against this, but broke once extractors started populating BlockLocation from the lockfile. Filename comparison fixed the same-file case but missed the cross-file workspace scenario. Checking LocationRole captures the intent precisely: only skip overwrite when a manifest match already exists, regardless of which file set it. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
anderruiz
commented
May 7, 2026
anderruiz
left a comment
anderruiz
left a comment
Contributor
Author
There was a problem hiding this comment.
Fixed in 84abd50: changed the guard from pkg.BlockLocation.Filename == depMap.FilePath to pkg.LocationRole == models.LocationRoleManifest.
The filename check only covered the same-file case (same package.json dev→prod section within one file). The cross-workspace scenario — root package.json prod match followed by a workspace package.json dev section — has a different FilePath, so the guard was false and the overwrite happened.
Checking LocationRole == manifest captures the intent exactly: skip the overwrite whenever a manifest location is already set, regardless of which file set it. A lockfile-originated location (LocationRole == lockfile) still allows the first manifest match through.
cespio
approved these changes
May 11, 2026
anderruiz
added a commit
that referenced
this pull request
May 13, 2026
Snapshots now reflect BlockLocation data from all parsers added in ander/transitive-dep-locations, merged with the npm-lock positions already shipped in PR #141. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
anderruiz
added a commit
that referenced
this pull request
May 22, 2026
Snapshots now reflect BlockLocation data from all parsers added in ander/transitive-dep-locations, merged with the npm-lock positions already shipped in PR #141. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
anderruiz
added a commit
that referenced
this pull request
May 22, 2026
…ross all parsers (#127) * Set BlockLocation for all packages in gradle-lock extractor Add line counter to bufio.Scanner loop and set BlockLocation with line number and column range for each parsed package. Column.Start is 1 (start of line), Column.End is the raw line length + 1 (before TrimSpace, preserving original positions). Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * Add BlockLocation assertion test for gradle-lock extractor Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * Set BlockLocation for all packages in mix-lock extractor Add line counter to bufio.Scanner loop and set BlockLocation with line number and full line column range for each parsed package. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * Add BlockLocation test and update mix-lock test expectations Add dedicated BlockLocation assertion test. Switch strict ExpectPackages to ExpectPackagesWithoutLocations for non-location tests. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * Set BlockLocation for all packages in gemfile-lock extractor Add line number and source file tracking to the gemfileLockfileParser state machine. Each package gets BlockLocation set with the line where it appears in the Gemfile.lock file. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * Add BlockLocation test and update gemfile-lock test expectations Add dedicated BlockLocation assertion test for gemfile-lock extractor verifying line numbers and filenames. Switch existing tests to ExpectPackagesWithoutLocations since dedicated test covers location assertions. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * Set BlockLocation for all packages in cargo-lock extractor Use the InTOML utility to compute block positions for each [[package]] section in Cargo.lock. Read file content into buffer for both TOML decode and line-based position computation. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * Add BlockLocation test and update cargo-lock test expectations Add dedicated BlockLocation assertion test for cargo-lock extractor verifying multi-line block positions from InTOML utility. Switch existing tests to ExpectPackagesWithoutLocations. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * Set BlockLocation for all packages in poetry-lock extractor Use the InTOML utility to compute block positions for each [[package]] section in poetry.lock. Uses [metadata] as the otherKey to properly close the last package block. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * Add BlockLocation test and update poetry-lock test expectations Add dedicated BlockLocation assertion test for poetry-lock extractor verifying multi-line block positions from InTOML utility. Switch existing tests to ExpectPackagesWithoutLocations. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * Set BlockLocation for all packages in pdm-lock extractor Use the InTOML utility to compute block positions for each [[package]] section in pdm.lock. Buffer file content with io.ReadAll for both TOML decode and line-based position computation. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * Add BlockLocation test and update pdm-lock test expectations Add dedicated BlockLocation assertion test for pdm-lock extractor verifying multi-line block positions from InTOML utility. Switch existing tests to ExpectPackagesWithoutLocations. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * Set BlockLocation for all packages in uv-lock extractor Use the InTOML utility to compute block positions for all [[package]] sections including root, then assign positions only to non-root packages. Handles the root package skip correctly by computing positions for all TOML entries and indexing by position. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * Add BlockLocation test and update uv-lock test expectations Add dedicated BlockLocation assertion test for uv-lock extractor verifying that root package is skipped while non-root packages get correct positions. Switch existing tests to ExpectPackagesWithoutLocations. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * Set BlockLocation for all packages in composer-lock extractor Add custom JSON array scanner to compute block positions for each package object in the "packages" and "packages-dev" arrays. Tracks brace depth to find start/end lines of each package block. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * Add BlockLocation test and update composer-lock test expectations Add dedicated BlockLocation assertion test for composer-lock extractor verifying JSON block positions for both packages and packages-dev arrays. Switch existing tests to ExpectPackagesWithoutLocations. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * feat(lockfile): add BlockLocation to pipenv-lock extractor Buffer content with io.ReadAll, use InJSON utility for "default" and "develop" map sections to compute block positions for each package. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * test(lockfile): update pipenv-lock tests with BlockLocation assertions Switch 5 existing tests to ExpectPackagesWithoutLocations and add dedicated TestParsePipenvLock_TwoPackages_BlockLocation test verifying positions for packages in both "default" and "develop" sections. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * feat(lockfile): add BlockLocation to renv-lock extractor Buffer content with io.ReadAll, use InJSON utility for "Packages" map section to compute block positions for each package. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * test(lockfile): update renv-lock tests with BlockLocation assertions Switch 4 existing tests to ExpectPackagesWithoutLocations and add dedicated TestParseRenvLock_TwoPackages_BlockLocation test verifying positions for packages in the "Packages" section. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * feat(lockfile): add BlockLocation to nuget-lock extractor Buffer content with io.ReadAll, use InJSON utility per framework target (e.g. "net6.0") to compute block positions for each package within the nested "dependencies" structure. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * test(lockfile): update nuget-lock tests with BlockLocation assertions Switch 6 existing tests to ExpectPackagesWithoutLocations, update MultipleVersionsNonDeterministicOrder to expect lockfile positions for packages not matched by .csproj, and add dedicated BlockLocation test for one-framework-two-packages fixture. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * Add BlockLocation to conan-lock extractor for both V1 and V2 formats V1 (nodes map): Uses InJSON with "nodes" group key to get positions for each node block, keyed by node ID. V2 (string arrays): Uses ExtractDelimitedStringPositionInBlock to find the line containing each reference string within quotes. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * test(lockfile): update conan-lock tests with BlockLocation assertions Switch existing v1-revisions and v2 tests from ExpectPackages to ExpectPackagesWithoutLocations, and add dedicated BlockLocation tests for both v1-revisions and v2 formats. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * feat(lockfile): add BlockLocation to gradle-verification-metadata extractor Buffer XML content and scan for <component> elements to extract line positions. Handles duplicate group:name:version entries (multiple versions) by consuming positions sequentially. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * test(lockfile): update gradle-verification-metadata tests with BlockLocation assertions Switch existing tests to ExpectPackagesWithoutLocations and add dedicated BlockLocation test for two-packages fixture verifying line ranges for each <component> block. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * feat(lockfile): add BlockLocation to pubspec-lock extractor Buffer YAML content and scan for package name entries at 2-space indent under the "packages:" key. Track block boundaries from package name line to the line before the next package or end of section. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * test(lockfile): update pubspec-lock tests with BlockLocation assertions Switch existing tests to ExpectPackagesWithoutLocations and add dedicated BlockLocation test for two-packages fixture verifying line ranges for shelf and shelf_web_socket packages. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * feat(lockfile): add BlockLocation to pnpm-lock extractor Add lockfile position tracking for both pnpm v9+ and legacy (<v7) formats. Buffer file content before YAML decode, scan for package entries under "packages:" section using indent-based detection, and set BlockLocation on all returned PackageDetails. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * test(lockfile): update pnpm-lock tests with BlockLocation assertions Add dedicated BlockLocation tests for v9 one-package, v9 mixed-groups, legacy one-package, and legacy multiple-packages fixtures. Update workspace-complex test to expect lockfile position for transitive dependency ([email protected]). Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * feat(lockfile): add BlockLocation to yarn-lock extractor Add lockfile position tracking for both yarn v1-v3 (text format) and v4+ (JSON format). For text format, track line numbers during block grouping. For JSON format, scan lines for entry keys within "entries" object. Buffer full content for both formats to enable line scanning. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * test(lockfile): update yarn-lock tests with BlockLocation assertions Add dedicated BlockLocation tests for v1 one-package, v1 two-packages, v2 one-package, and v2 two-packages fixtures. Update workspace-complex tests for both v1 and v2 to expect lockfile positions for transitive dependency ([email protected]). Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * test: update snapshots and fix lint warnings in BlockLocation tests Update integration test snapshots to reflect new lockfile positions for transitive dependencies. Fix testifylint warnings in new test code: - Use assert.Len instead of assert.Equal with len() - Use assert.Positive instead of assert.Greater with 0 Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * feat(swift): add BlockLocation to Package.resolved extractor Buffer file content with io.ReadAll, scan pin blocks by identity field using pinPositionsByIdentity, and attach BlockLocation to each PackageDetails entry for v2/v3 lockfiles. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * test(swift): update Package.resolved tests with BlockLocation assertions Switch metadata tests to ExpectPackagesWithoutLocations and add a dedicated TestParsePackageResolved_TwoPackagesV2_BlockLocation test that verifies exact line/column ranges for each pin block. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * test: regenerate integration snapshots after rebase onto main (#141) Snapshots now reflect BlockLocation data from all parsers added in ander/transitive-dep-locations, merged with the npm-lock positions already shipped in PR #141. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * fix(javascript): fix matcher dev-dep skipping when BlockLocation set from lockfile - Change dev/optional dep skip condition from BlockLocation.Line.Start != 0 to LocationRole == LocationRoleManifest - Add pkg/models import to match-package-json.go Rationale: The original condition intended to skip updating a package with a dev-dependency location if it had already been matched in the regular dependencies section. It used BlockLocation.Line.Start != 0 as a proxy for "already found in manifest". After extractors started setting BlockLocation from the lockfile itself, every package had a non-zero BlockLocation from the start, causing the matcher to incorrectly skip all dev/optional dependency packages, leaving them without a manifest location even when present in package.json. Using LocationRole == LocationRoleManifest directly expresses the intent. This commit made by [/dd:git:commit:atomic](https://github.com/DataDog/claude-marketplace/tree/main/dd/commands/git/commit/atomic.md) * feat(lockfile): set LocationRole=lockfile in all extractors that set BlockLocation - Set LocationRole = LocationRoleLockfile alongside BlockLocation in all lockfile extractors: cargo, gradle, gradle-verification-metadata, composer, nuget, gemfile, poetry, pipenv, pnpm (v9 and legacy), yarn - Update unit test expectations to assert LocationRole=lockfile for packages whose location comes from the lockfile (dotnet, pnpm-v9, yarn-v1, yarn-v2) - Regenerate integration snapshots to include role=lockfile in evidence occurrences Rationale: The invariant test TestLocationRoleSetWhenBlockLocationIsValid (added to main in 153b51a) requires that any PackageDetails with a valid BlockLocation must also have a non-empty LocationRole. This branch added BlockLocation extraction to many parsers but predated that invariant, so none of them set LocationRole. Setting LocationRoleLockfile makes the data complete and consistent: downstream consumers (e.g. vulnerability_result.go) can rely on the role field always being present when a location is emitted. This commit made by [/dd:git:commit:atomic](https://github.com/DataDog/claude-marketplace/tree/main/dd/commands/git/commit/atomic.md) --------- Co-authored-by: Claude Sonnet 4.6 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Pilot PR to validate
BlockLocationextraction from npm lockfiles before rolling out to all 17 parsers in PR #127.Changes:
BlockLocation(line/column/filename) from the lockfile for all npm packages during extraction (v1 and v2)LocationRole: "lockfile"on all extracted npm packages (matchers override to"manifest"for direct dependencies)LocationRolefield interaction withBlockLocationCherry-picked from
ander/transitive-dep-locations(PR #127):fd8cd67— Set BlockLocation from lockfile for all npm packages during extraction769af54— Update npm-lock tests for BlockLocation extractionda74b0e— fix(sbom): sort evidence occurrences and fix npm location determinismPlus one adaptation commit to reconcile with
LocationRole(merged via #138 on main).Next steps: Once this pilot is validated, PR #127 will extend
BlockLocationextraction to the remaining 16 parsers.Test plan