Repository navigation
feat(purl): build package urls from the ecosystem they belong to - #1398
alex-powell wants to merge 1 commit into
Conversation
c4823e9 to
2904c36
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Advisory-window evaluation can produce false negatives, and PyPI package URL versions need percent-encoding.
2 open issues
What changed in this PR
Adds ecosystem-aware package URLs while preserving npm serialization and keeping non-npm packages out of SPDX and CycloneDX output. The branch also includes the prerequisite PEP 440 version-handling work from #1397.
Changes:
- Adds PyPI package URL construction and an npm-only SBOM publication helper.
- Updates SBOM generation and SPDX finding grouping to distinguish ecosystems.
- Includes PEP 440 comparison, advisory integration, validation tooling, and regression tests.
| File | Description |
|---|---|
tsconfig.test.json |
Includes the PEP 440 validation script in test compilation. |
tests/utils/pep440.test.ts |
Tests version parsing, ordering, and advisory evaluation. |
tests/utils/pep440-helpers.test.ts |
Tests ecosystem-aware version helpers. |
tests/utils/ecosystem-purl.test.ts |
Tests package URLs and non-npm SBOM exclusion. |
tests/scripts/check-pep440-requirements.test.ts |
Tests requirements parsing and advisory reports. |
tests/scanner/pep440-fix-version.test.ts |
Tests PyPI fix-version selection. |
tests/advisory/pep440-local-db.test.ts |
Tests offline PyPI advisory matching. |
src/utils/versions/scheme.ts |
Defines version-scheme contracts and parsing errors. |
src/utils/versions/registry.ts |
Selects version schemes by ecosystem. |
src/utils/versions/pep440.ts |
Implements PEP 440 parsing and comparison. |
src/utils/versions/advisory.ts |
Adds PyPI advisory evaluation helpers. |
src/utils/version.ts |
Adds ecosystem-aware dispatch while retaining semver behavior. |
src/utils/purl.ts |
Adds ecosystem URL construction and SBOM publication filtering. |
src/scanner.ts |
Uses ecosystem-specific comparisons for fix selection. |
src/output/spdx.ts |
Filters packages and groups findings by ecosystem. |
src/output/cyclonedx.ts |
Excludes non-npm components and vulnerabilities. |
src/advisory/local-db.ts |
Uses ecosystem-specific advisory-boundary comparisons. |
scripts/check-pep440.ts |
Adds requirements reporting and Python comparison tooling. |
Review effort: Balanced
sonukapoor
left a comment
sonukapoor
left a comment
There was a problem hiding this comment.
This does what #1402 asks, @alex-powell. On current main, SPDX and CycloneDX output is byte-identical before and after on nest, analog, yarn-berry, cline and vscode, so npm, pnpm, Yarn and Bun are all covered, and the suite passes. Your tests also discriminate: removing the npm-only gate, the PyPI name rule, the PyPI branch, or the ecosystem in the finding key each fails at least one of them.
One ask: please rebase this onto main on its own instead of stacking it on #1397. Your commit cherry-picks cleanly onto current main and passes there, so it does not need #1397 at all, and as it stands merging this would bring #1397 in with it. That lets this land while #1397 is still in review.
Two small things while you are there. purlForSbom repeats the npm check rather than calling buildEcosystemPurl behind a gate, so the two can drift. And the PyPI version goes into the URL unencoded, so a local label like 1.2.3+local would need its + written as %2B per the purl spec. It is not emitted yet, so a note or a test is enough for now.
An npm URL is still buildPurl. buildEcosystemPurl adds PyPI, where the name is lowercased and underscores become hyphens, and returns null when the ecosystem has no rule. A PyPI version percent-encodes reserved characters, so 1!2.0 is 1%212.0 and 1.2.3+local is 1.2.3%2Blocal. SPDX and CycloneDX publish only npm, through that same builder.
2904c36 to
6cd5ddc
Compare
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Ecosystem PURL rules src/utils/purl.ts, tests/utils/ecosystem-purl.test.ts |
buildEcosystemPurl handles npm and PyPI PURLs. It normalizes PyPI names and encodes PyPI versions. purlForSbom returns a PURL only for npm packages. Tests cover the helper behavior. |
SBOM package and finding emission src/output/cyclonedx.ts, src/output/spdx.ts, tests/utils/ecosystem-purl.test.ts |
CycloneDX and SPDX use purlForSbom and omit packages and findings without a PURL. SPDX finding keys include ecosystem, name, and version. Publication tests check npm inclusion and PyPI and Maven omission. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Feature · Severity of issue fixed: Medium
Sequence Diagram(s)
sequenceDiagram
participant CycloneDX builder
participant purlForSbom
participant SPDX builder
CycloneDX builder->>purlForSbom: Package ecosystem, name, and version
purlForSbom-->>CycloneDX builder: PURL or null
CycloneDX builder->>CycloneDX builder: Emit package or finding only when PURL exists
SPDX builder->>purlForSbom: Package ecosystem, name, and version
purlForSbom-->>SPDX builder: PURL or null
SPDX builder->>SPDX builder: Emit package or finding only when PURL exists
Suggested reviewers: sonukapoor
Merge Risk: 
6cd5d
-
No actionable issue remains from the supplied review; the change is mergeable after normal checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surv… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly identifies the main change: building package URLs based on their ecosystem. Linked Issues check Linked issue #1402has coding requirements only. Its acceptance criteria are: (1) npm SBOM output byte-identical, including%40scoped-name encoding; (2) PyPI names lowercased with_replaced by `…Out of Scope Changes check Each changed area connects to #1402or supports its requirements:
-src/utils/purl.ts:buildEcosystemPurlandpurlForSbomimplement the per-ecosystem rule.encodePurlVersionapplies to PyPI p…Description check The description follows the template's core purpose. It explains what changed and why, and it gives a detailed account of the PyPI name and version rules, the npm behavior, the SPDX and CycloneDX gati… Full details: Linked Issues check
-
Explanation
Linked issue
#1402has coding requirements only. Its acceptance criteria are: (1) npm SBOM output byte-identical, including%40scoped-name encoding; (2) PyPI names lowercased with_replaced by-; (3) no purl for an ecosystem without a rule; (4) tests for a mixed-case PyPI name with underscores, a scoped npm name, and an unknown ecosystem; (5) no new runtime dependency. Evidence at the reviewed head:- Ecosystem dependence and PyPI rule (
#1402):buildEcosystemPurlinsrc/utils/purl.tslowercasesnameand replaces_with-forpypi. It returnsnullfor every other ecosystem except npm, which it sends to the unchangedbuildPurl. - npm preservation (
#1402):buildPurlis unchanged.purlForSbomreturnsbuildEcosystemPurloutput for npm, so npm SBOM purls take the same encoder path as before.src/output/cyclonedx.tsandsrc/output/spdx.tsnow callpurlForSbom. For npm packages the emittedpurl,bom-ref, and SPDXreferenceLocatorvalues are unchanged. The byte-identical result is author-reported (SBOM diff against parent commit). I did not run it. - No purl for unknown ecosystems (
#1402):purlForSbomreturnsnullfor all non-npm ecosystems. Both SBOM builders skip such packages. - Tests (
#1402): the summary states thattests/utils/ecosystem-purl.test.tscovers the three required cases. I could not read that file. The diff forsrc/utils/purl.tswas unavailable, so I read the file at head. That read supports the code behavior described above, but the test contents rely on the summary. - Runtime dependency (
#1402): the PR description and summary state thatpackage.jsonis unchanged. I did not readpackage.json.
The requirements are met by the code at the reviewed head. Test presence and byte-identity rest on the stated author evidence.
- Ecosystem dependence and PyPI rule (
Full details: Out of Scope Changes check
-
Explanation
Each changed area connects to
#1402or supports its requirements:src/utils/purl.ts:buildEcosystemPurlandpurlForSbomimplement the per-ecosystem rule.encodePurlVersionapplies to PyPI purl versions only. It is part of building a valid PyPI purl, not a separate version-parsing change. The issue excludes version parsing as a separate task. This function does not parse versions. npm versions remain unencoded, as before.src/output/cyclonedx.ts: components and findings without an SBOM purl are skipped. This follows#1402("emit no purl rather than a wrong one") and the issue rule that Python stays out of SPDX and CycloneDX.src/output/spdx.ts:findingPackageKeyadds the ecosystem to the finding grouping key. This prevents findings from one ecosystem from attaching to a same-named package in another ecosystem. That behavior would otherwise emit a wrong identifier. The removed doc comment was replaced by the helper. This is a minor, related change.- Tests: the summary states that the new test file covers only the issue's cases and the SBOM exclusion behavior.
I found no change unrelated to
#1402. The file-level evidence forpurl.tswas read directly. Thespdx.tsandcyclonedx.tschanges were read as diffs.
Full details: Description check
-
Explanation
The description follows the template's core purpose. It explains what changed and why, and it gives a detailed account of the PyPI name and version rules, the npm behavior, the SPDX and CycloneDX gating through purlForSbom, and the verification performed. The template's "Closes #" line is filled with "Fixes
#1402", and the description also references#672. It does not use the literal "## What changed and why" heading, but its opening section covers the same content. Review notes and a Done-when section are added beyond the template. These are useful and do not conflict with the template. The description is detailed and specific, so the check passes.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 8 functions across 4 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
sonukapoor
left a comment
sonukapoor
left a comment
There was a problem hiding this comment.
Thanks @alex-powell, this is ready. It sits on main on its own now, the SBOM path goes through one builder behind the npm check, and the epoch and local-label encoding has tests. SPDX and CycloneDX output on nest and yarn-berry is identical to main, and breaking either the encoding or the npm check fails the new tests.
One tiny thing for later: the comment above groupFindingsByPackage in spdx.ts went missing.
Fixes #1402
Refs #672
What changed
buildPurl(name, version)still builds an npm package URL, including the%40encoding of a scoped name.buildEcosystemPurl(ecosystem, name, version)is new. Forpypiit lowercases the name and replaces_with-, and it leaves dots in place, soDjango_RESTbecomespkg:pypi/django-rest@...andpdfminer.sixstayspkg:pypi/pdfminer.six@.... An ecosystem with no rule returns null.A PyPI version is percent-encoded.
1!2.0ispkg:pypi/requests@1%212.0.1.2.3+localispkg:pypi/requests@1.2.3%2Blocal. An npm version is unchanged, including a literal+.SPDX and CycloneDX call
purlForSbom. That returns null unless the ecosystem is npm, and for npm it returnsbuildEcosystemPurl, so the npm URL has one implementation. A PyPI URL can be built. It is not written into either document. Existing tests are unchanged. The new cases are intests/utils/ecosystem-purl.test.ts. No runtime dependency was added.This commit sits on current
main(ff40875). It is the only commit on the branch.Review
mainon its own. Head6cd5ddcb64e645667c3c2add61e7c8eaf7964395, parentff408754310e98245fbe6553c9ff4eb437f814fd.52d1792is not in the history.purlForSbomcallsbuildEcosystemPurlbehind the npm gate. The CycloneDX test expects the lodash SBOM URL to equalbuildEcosystemPurl("npm", "lodash", "4.17.21"), and null forPyPI.pkg:pypi/requests@1!2.0. It now expectspkg:pypi/requests@1%212.0andpkg:pypi/requests@1.2.3%2Blocal, and both pass.pkg:npm/left-pad@1.0.0+buildstays literal.Done when
examples/vscode(1374 packages),examples/yarn-berry(3),examples/cline(1518),examples/astro(2228).tests/utils/ecosystem-purl.test.tsexpectspkg:pypi/django-rest@3.14.0,pkg:npm/%40babel/core@7.0.0, and null for Maven. The same file expectszope.interfaceandpdfminer.sixto keep their dots, and it expects PyPI and Maven packages to be absent from both SBOM documents. It also expects the two encoded PyPI versions above.package.jsonandpackage-lock.jsonare unchanged. No pre-existing test file is modified. Test hygiene passed. The suite passed: 184 suites, 2516 tests.packageurl-python 0.17.6 parsed the PyPI strings below and re-serialized them unchanged. Building from the name and version produced the same string. packageurl-js 2.0.1 recovered the same type, name, and version. For
1!2.0and1.2.3+localit then writes a literal!and+.Verified against a production requirements.txt
I took the 20 exact pins from the pinned
requirements.txtof the production Python backend I mentioned in #672 and ran them throughbuildEcosystemPurl("PyPI", ...)andpurlForSbomfrom this commit (6cd5ddc).purlForSbomreturns null (kept out of SPDX and CycloneDX)The one name the rule changes is
pdfminer.six, which is emitted aspkg:pypi/pdfminer.six@20260107with its dot kept, and OSV resolves that purl to the same result as the ecosystem query. That file has no epoch or local versions, so the encoding is covered by the examples below.Edge-case examples
These are constructed names and versions chosen to exercise the name rule and the version encoding, not pins from a real project.
Django_REST==3.14.0pkg:pypi/django-rest@3.14.0zope.interface==7.0pkg:pypi/zope.interface@7.0pdfminer.six==20221105pkg:pypi/pdfminer.six@20221105pdfminer.six==20260107pkg:pypi/pdfminer.six@20260107requests==1!2.0pkg:pypi/requests@1%212.0requests==1.2.3+localpkg:pypi/requests@1.2.3%2BlocalPillow==10.4.0pkg:pypi/pillow@10.4.0typing_extensions==4.12.2pkg:pypi/typing-extensions@4.12.2python-dateutil==2.9.0.post0pkg:pypi/python-dateutil@2.9.0.post0ruamel.yaml==0.18.6pkg:pypi/ruamel.yaml@0.18.6PyYAML==6.0.1pkg:pypi/pyyaml@6.0.1