test(lockfile): add invariant — LocationRole must be set when BlockLocation is valid#149
Conversation
…n is set Any extractor that populates a valid BlockLocation must also set LocationRole. Without it, the reporter emits a blank role field in the SBOM output even though the location is meaningful. The test walks pkg/lockfile/fixtures/, runs the registered extractor for each fixture file, and fails if any PackageDetails has a valid BlockLocation (file + line/column all non-zero) but an empty LocationRole. It is a no-op for files without a registered extractor and silently skips fixtures that intentionally fail to parse. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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: c15246aca6
ℹ️ 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".
| return err | ||
| } | ||
|
|
||
| extractor, _ := lockfile.FindExtractor(path, map[string]bool{}) |
There was a problem hiding this comment.
In this test, FindExtractor only returns an extractor when enabledParsers[name] is true (see pkg/lockfile/extract.go), so passing an empty map disables every registered parser. As a result extractor is nil for every fixture and the new invariant test passes without exercising any extractor, including the future package.json case it is meant to catch. Populate the map from the registered extractor names before walking fixtures, or use a helper that matches all registered extractors.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4be1fa3. The test now populates allParsers from lockfile.ListSupportedExtractors() before passing it to FindExtractor, so every registered extractor is exercised against its fixture files.
Pass allParsers populated from ListSupportedExtractors() to FindExtractor instead of an empty map. An empty map causes FindExtractor to return nil for every fixture, making the test pass vacuously without exercising any extractor. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4be1fa3a63
ℹ️ 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".
| } | ||
| defer f.Close() | ||
|
|
||
| packages, err := extractor.Extract(f, context) |
There was a problem hiding this comment.
For extractors that add source-file locations via matchers, this check is bypassed because it calls extractor.Extract directly. Production extraction then runs any ExtractorWithMatcher matchers (see ExtractFromFileWithContext), and those matchers are exactly where several manifest BlockLocation/LocationRole fields are populated. If a matcher regresses and starts setting a valid BlockLocation without LocationRole, this invariant still passes, so use the same extraction path that applies matchers for each fixture.
Useful? React with 👍 / 👎.
2 participants
Summary
Motivation
Identified while reviewing PR #126 (`PackageJSONExtractor`): the extractor sets `BlockLocation` for every package but omits `LocationRole`, so the `role` field in the SBOM output is blank. Since `LocationRole` is a plain `string` field, the compiler cannot enforce it — a test is the right safety net.
How the test catches PR #126 as-is: when that PR merges, `PackageJSONExtractor` is registered and its `package.json` fixtures are picked up by this test. The extractor sets a valid `BlockLocation` but leaves `LocationRole` empty → the test fails. After adding `LocationRole: models.LocationRoleManifest` (the fix) the test passes again.
Test plan
🤖 Generated with Claude Code