Sanitize the licence expressions an SPDX 3 document actually has - #413
Open
aurangzaib048 wants to merge 100 commits into
Open
aurangzaib048 wants to merge 100 commits into
aurangzaib048 wants to merge 100 commits into
Conversation
A document that validates clean against our own bundled spdx-3.0.1.schema.json came back with 12 schema errors after parse_spdx3_file plus write_spdx3_file. Every element in the output was invalid, which is why AUGMENT=true or ENRICH=true on any SPDX 3 input was an unconditional exit 1: both re-validate their own output. Four defects, all of them spdx-tools 0.8.5 modelling a pre-3.0.1 draft. CreationInfo still carries profile and data_license there. 3.0.1 moved dataLicense to SpdxDocument and replaced profile with Element.profileConformance, and CreationInfo_props is unevaluatedProperties: false, so either key invalidates every element carrying that CreationInfo. Both are now stripped on write. profileConformance was read nowhere, so every rewrite silently dropped it. Listing a profile claims every contained element meets its restrictions, so losing it weakens the document without saying so. It now rides on the payload and lands back on the SpdxDocument. A document that declared none does not gain one. The writer emitted externalReference, externalReferenceType and the type ExternalReference. 3.0.1 renamed all three to externalRef, externalRefType and ExternalRef, and rejects the old names. The parser defaults data_license to the bare id "CC0-1.0" when a document declares none. Writing that back invented a licence claim its author never made, and failed the schema besides, which wants a licence IRI rather than a short identifier. It is now read from the raw document only, from SpdxDocument first and the CreationInfo second, and is absent when the input was. Not fixed by upgrading spdx-tools: its SPDX 3 support is write-only and the maintainers say not to use it in production. The repo had no conformant SPDX 3 fixture to measure against. spdx3_minimal.json uses @id rather than spdxId, carries no @context and fails the schema eight times on its own, so a round-trip through it proved nothing. The new fixture validates before anything touches it, and the first test asserts that, because the rest are worthless without it. The mypy hook is skipped: it fails on master in two files this change does not touch, because its isolated env resolves two return types as Any.
Two more places the draft model spdx-tools 0.8.5 implements cost us data the producer wrote. The @context was hardcoded to 3.0.1 and none of the eight write call sites overrode it, so a 3.0 document came back labelled 3.0.1 while its creationInfo still said 3.0.0. Nobody wrote that document and no consumer can resolve it. 3.0 shipped April 2024 and 3.0.1 that December; syft, Microsoft sbom-tool, JFrog Xray and Zephyr all emit 3.0. The context now rides on the payload from parse to write, so every call site preserves it without knowing it has to, and a document built from nothing still gets the current release. An explicit argument still wins. SoftwarePurpose in that library is the pre-3.0.1 enum: no specification, no filesystemImage, both of which 3.0.1 defines and Yocto emits. The parser logged "Unrecognized primaryPurpose value; omitting" and dropped the field. Measured on the published Yocto 6.0.3 core-image-minimal SBOM, which validates clean against our own schema: 38 packages lost their primaryPurpose, and the one filesystemImage in the document is the image itself, the package a Yocto user cares about most. The 78 that survived did so by accident, having failed to parse for an unrelated reason and been passed through verbatim, so the elements we handled correctly were the ones that lost data. A value the producer wrote and the schema accepts is not dropped because a library is a version behind. Unmodelled purposes ride on the payload keyed by spdxId and are written back. That document now round-trips with 0 schema errors, 0 of 3049 elements lost and no field lost on any element. Before this branch it was 0 errors in and thousands out.
…pes were renamed 3.0.1 has no declaredLicense or concludedLicense property. It states a licence as a Relationship from the artifact to a licensing element. spdx-tools 0.8.5 still models both as fields, and every properties block is unevaluatedProperties: false, so the field invalidated the whole package. Invalid is the smaller half: sbomify's own reader follows the relationship, so a licence the action enriched in was one we could not read back. The same draft model costs more on the way in. Its RelationshipType enum holds 62 values and 37 of 3.0.1's 59 are not among them, both licence relationships included. The parser maps anything it does not recognise to `other`, which is itself legal, so the rewritten document passed the schema while saying something different from what the producer wrote. Measured on the published Yocto SBOMs, plain round-trip, no augment or enrich: 6.0.3 core-image-minimal 708 hasDeclaredLicense -> 708 other 6.0.3 core-image-full-cmdline 1667 hasDeclaredLicense -> 1667 other 5.1 core-image-minimal 547 hasConcludedLicense -> 547 other Every licence in the image, gone, and the file still validated. The raw value now rides on the payload beside the purposes and goes back verbatim. Two more from the same source. An artifact has one suppliedBy in 3.0.1 and originatedBy is the set; spdx-tools models both as lists, so a supplier the action worked out was written as a one-element array the schema refuses. And CreationInfo_props requires createdBy with minItems 1, while make_spdx3_creation_info left it empty, so each Tool, Organization and Person augmentation and enrichment add failed outright. Those now name a SoftwareAgent for the action, which the writer puts in the graph. A CreationInfo that arrived without a createdBy is left as it arrived: the action does not know who created that element, and naming itself there would state provenance nobody wrote. Enrichment and augmentation also stop second-guessing a package that already declared a licence. declared_license is unset on any package whose author stated one as a relationship, and reading that as "no licence" is how a contradicting second declaration got added. Overriding replaces the declaration rather than leaving two that disagree.
…ough SPDX_SCHEMAS held 2.2, 2.3 and 3.0.1. A 3.0 document matched nothing, so it validated as "skipped", logged a warning and went to the upload unchecked, while the README says generated SBOMs are validated against their JSON schemas. 3.0 is what syft, Microsoft sbom-tool, JFrog Xray and Zephyr emit, and what the published Yocto 5.1 image SBOM declares. CycloneDX has bundled every version it accepts, 1.3 through 1.7, all along. The two are separate documents rather than a version label on one, so aliasing 3.0 onto the 3.0.1 schema would have been wrong in both directions: 3.0.1 renamed File.software_contentType to contentType, added IndividualElement and renamed SpdxDocument.imports and Build.build_parameters. A document correct under either schema fails the other, and a test asserts all four cases. spdx-3.0.0.schema.json is the official shacl2code output from spdx.org/schema/3.0.0/, the same source as the 3.0.1 copy beside it. An SPDX 3.x version with no bundled schema now fails, naming the versions there are, rather than skipping. A skip is how every SPDX 3 document reached the upload unchecked in the first place, and for a format still adding versions a loud failure is the safer default. SPDX 2.x keeps the old skip: nothing claims to check 2.1, so refusing it now would be a new rejection rather than a fix. Five of the tests added here fail without the change. Two of them only discriminate because they assert `valid is False`: a skip reports valid=None, which is falsy, so assertFalse would have passed on the very behaviour they exist to rule out. Measured on the published Yocto 5.1 core-image-minimal, 2066 elements: skipped before, valid=True against the 3.0.0 schema now. Left alone: https://spdx.org/rdf/3.0/spdx-context.jsonld resolves (byte identical to 3.0.1 today) but carries no patch version, so a document using it still reports "could not detect spec version". That is a moving alias whose meaning depends on when the document was written, nothing in the corpus uses it, and guessing a version for it would be worse than refusing.
…ument states
A 3.1 document was accepted as SPDX 3 and then failed somewhere unhelpful.
`/rdf/3.1/` carries no patch number and the version regex wanted three dotted
numbers, so it found nothing and the document failed as "Could not detect spdx
spec version", naming neither SPDX nor 3.1. `/rdf/3.1.0/` fell through to the
unvalidated-skip path. Both now say so by name, in the backend's own words, so
a user who hits the action and the upload hears one answer rather than two:
SPDX 3.1 is not supported. sbomify accepts SPDX 2.2, 2.3 and 3.0.x;
send 3.0.1 to also satisfy the BSI TR-03183-2 floor.
Rejection is the right answer: 3.1 is at RC1, BSI accepts released versions
only, and the backend refuses it, so shipping it here would need a backend
change first.
extract_spdx3_version now reads the document's own specVersion before its
@context. CreationInfo_props requires specVersion and every Element requires a
creationInfo, so a conformant document always states its version, and that is
the normative claim rather than a hint. Checked on all six SPDX 3 documents in
the corpus: the two agree in every one. The context regex also accepts a
two-part version now, so an unversioned context is read rather than ignored.
The specVersion is read from a CreationInfo only, either one that names its
type or one reached as a creationInfo value. This answer chooses the schema
the whole document is held to, so a specVersion sitting on some other element
must not speak for it.
That order matters because https://spdx.org/rdf/3.0/spdx-context.jsonld is
real. It serves 200 and is byte-identical to the 3.0.1 context today, which
makes it a moving alias whose meaning depends on when the document was
written. Reading specVersion first resolves it for every document that states
one, which is every conformant document.
A document carrying only the alias still cannot validate, and that is SPDX's
own ruling rather than a choice made here: both official schemas pin @context
with a `const` to their fully qualified URL. What changes is where such a
document lands. It now gets the schema's own message naming the exact context
to write, instead of "could not detect spec version", and it is deliberately
not reported as an unsupported version, because 3.0.0 is supported and the
alias is the thing to fix. A test pins the const in both schemas so this
reasoning fails loudly if a future schema drops it.
Nine of the tests added here fail without the change. A tenth passes on the
old reader and fails on a looser one that took specVersion from anywhere,
which is the version I wrote first.
…efully
The --spec-version help read "e.g., '1.6', '2.3', '3.0.1'". Nothing here
generates SPDX 3: syft is the only SPDX generator that answers a plain version
and it caps at 2.2/2.3. So the one example that was not CycloneDX or the
default was the one that cannot work.
The config guard that catches it was already good, and pointed at SBOM_FILE
and additional-packages-only mode. Two things were wrong with it. It keyed on
the literal string "3.0.1", so SPEC_VERSION=3.0.0 fell through to the bare
"Supported: 2.2, 2.3" with no hint, and 3.0 is what syft, Microsoft sbom-tool,
JFrog Xray and Yocto 5.x emit. And it offered SBOM_FILE for any 3.x, including
versions the reader refuses, which would have sent a 3.1 user to a second
route to hear the same no. Now every version it names is one that route
accepts, and the empty-SBOM route says it writes 3.0.1 whatever was asked for,
which it does.
Below that, the registry's own failure rendered a Python dict into user-facing
text:
No generator found for input. Requested: format=spdx, version=3.0.1.
Available formats: {'cyclonedx': ['1.2', ...], 'spdx': ['2.2', '2.3',
'SPDX-2.3']}
'SPDX-2.3' is not a version anyone can set. Five generators declare it as an
internal marker meaning "reachable by converting", and three declare the plain
spelling, so the list showed one version twice and invited a value that selects
nothing. The message now names the format, the version asked for, and the
versions from the canonical tuples, which is where "what can be generated" is
already defined:
Nothing here generates SPDX 3.0.1. Generatable versions: 2.2, 2.3. SPDX 3
is read, validated and written, but no generator produces one, so supply
an existing document with SBOM_FILE instead of generating from a lock file
or an image.
The marker spelling itself is left alone and filed separately: normalising it
would make cyclonedx-cargo, gomod, maven, gradle and sbt candidates for a
plain SPDX 2.3 request, changing which tool produces the SBOM for every Rust,
Go, Java and Scala project.
action.yml gains sbom-format and spec-version as first-class inputs, so
neither has to be smuggled through a raw env: block, and a test asserts every
declared input reaches an environment variable, since one that reaches nothing
looks supported and is silently dropped. The README gains the input table it
never had; none of the existing five were in it either.
SUPPORTED_SPDX_VERSIONS gains 3.0.0, which is read, validated and written back
since the 3.0.0 schema was bundled, and validate_spdx_version's docstring no
longer claims SPDX 3.0 support has yet to be added.
…over SPDX 3
Both older SPDX 3 fixtures failed the schema this repo ships. Every Element
used `@id` where 3.0.1 wants `spdxId`, and the minimal one also carried an
empty `createdBy`, which is required with minItems 1, and a `profile` on its
CreationInfo, which 3.0.1 replaced with Element.profileConformance. The one
test that looked at a fixture's validity declined to assert it:
# Our minimal test fixture may not pass the strict 3.0.1 schema,
# but format/version detection must always succeed (valid is not None).
self.assertIsNotNone(result.valid)
A test that declines to check is why documents failing every element shipped
unnoticed. It now asserts `valid is True`, and a sibling holds all three
conformance-claiming fixtures to the same bar.
Converting the fixtures wholesale would have met the letter of that and
quietly deleted something worth keeping. `@id` on an Element is legal JSON-LD,
unprefixed software properties are what several producers emit, and the parser
reads both deliberately. So the old minimal fixture is kept verbatim as
spdx3_legacy_spellings.json, with tests that say what it is for, and one that
asserts it is still the non-conformant one, because these tests stop meaning
anything the moment somebody tidies it. spdx3_multi_type.json keeps the
blank-node `_:CreationInfo0` as `@id`, which the writer preserves on purpose
since spdxId must be an IRI.
The full-flow compliance test now runs for 3.0.0 and 3.0.1 beside 2.2 and 2.3.
It needs a sibling rather than a parametrize: the 2.x body builds a document
through spdx-tools, and nothing generates SPDX 3, so the 3.x leg starts from
the conformant fixture retargeted to the version under test. Both the context
and every specVersion move together, because the official schemas pin @context
with a const to their own fully qualified URL.
It validates the fixture before touching it, then augments, enriches, and
holds the result to the schema for the version it declares. Two further
assertions: the version it arrived as is the version it leaves as, and the
declared licence survives as a relationship, which is the writer defect in
miniature.
Reverting the writer fails it. Checked, not assumed: with spdx3.py,
enrichment.py, augmentation.py and additional_packages.py reset to
upstream/master, both legs fail on the `dataLicense` the draft model emits.
…standard The format list said "generate and process" per format, which reads as one capability and is two. Everything here is read, validated, augmented, enriched and written back; only some of it can be produced from a lock file or an image in the first place, and the two lists do not match in either direction. SPDX 3 is the loud gap: nothing generates it, syft being the only SPDX generator and stopping at 2.3. CycloneDX 1.2 is the quiet one in the other direction, generatable with no bundled schema, so it reaches the upload unchecked. The table now has a Generate column and a Validate column and they disagree in both rows, which is the point. Added a short standards note, because a procurement clause citing "the ISO SBOM standard" is a real way to end up on the wrong version, and this repo mentioned 5962 nowhere. Every claim in it is from a primary source, not from the issue text. ISO/IEC 5962:2021 is SPDX 2.2.1: the SPDX project's own v2.2.1 release notes say the release "includes all updates for the final ISO/IEC 5962:2021 SPDX specification", and spdx.dev dates it 2021/08. SPDX 3.0 is OMG formal/24-11-01, March 2025, read off omg.org/spec/SPDX. BSI TR-03183-2 v2.1.0 asks for SPDX 3.0.1 or CycloneDX 1.6 and up, which this repo's own BSI plugin already enforces. The issue also asserted that ISO/IEC DIS 5962 edition 2 remains unpublished. iso.org sits behind Cloudflare and returns 403, so that could not be checked and is not claimed here. What is said instead is that SPDX 3.0 is not covered by 5962:2021, which follows from what was verified. omg.org/spec/SPDX/3.0.1/ is a 404 and is not linked; the versionless page is.
…lly has sanitize_spdx_licenses walked packages[], files[] and snippets[] for licenseConcluded, licenseDeclared and licenseInfoFromFiles. An SPDX 3 document has none of those. It states a licence as a Relationship to a simplelicensing_LicenseExpression element in @graph, so the function returned 0 having looked at nothing, and zero reads as "nothing to fix" at both call sites: on ingest for any SPDX input, and again before validating generated SPDX. The SPDX 3 walk is additive rather than an early return, so a document carrying both shapes gets both, and the 2.x path is untouched either way. The issue this closes says RPM-style strings matter most for the Yocto path. They do not appear there. Every licence expression in the three published Yocto SPDX 3 images, run through this repo's own _sanitize_spdx_license_expression: 200 expressions across the three documents, 0 would be rewritten Yocto normalises licences itself, compound ones included: ( GPL-2.0-or-later OR LGPL-3.0-or-later ) AND GPL-3.0-or-later bzip2-1.0.6 AND GPL-3.0-or-later AND Apache-2.0 AND MS-PL AND BSD-3-Clause AND Zlib GPL-2.0-only AND LicenseRef-bzip2-1.0.4 So this is worth having for producers that do emit junk, and it is not the Yocto fix the issue frames it as. All five real Yocto documents in the corpus, SPDX 2.2 and 3 alike, come out byte identical, and a test pins that: the risk this code carries is rewriting somebody's correct licence, not missing a bad one. Five of the tests added here fail without the change.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
It introduces a non-portable hardcoded local filesystem path in a new test and contains user-facing advice logic that will incorrectly treat the SPDX “3.0” context alias as a readable SBOM_FILE version.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR updates SPDX 3 handling so that license-expression sanitization and schema validation operate on the fields an SPDX 3 JSON-LD document actually uses (relationships and simplelicensing_LicenseExpression elements in @graph), and tightens fixtures/tests/docs around SPDX 3.0.0 vs 3.0.1 behavior.
Changes:
- Extend SPDX license sanitization to walk SPDX 3’s JSON-LD
@graphlicensing elements (in addition to the existing SPDX 2.x shapes). - Improve SPDX 3 version detection/validation behavior (separate 3.0.0 vs 3.0.1 schemas; fail unknown SPDX 3 versions rather than silently skipping).
- Add/refresh SPDX 3 fixtures and broaden test coverage to pin schema conformance and prevent unintended license rewrites.
File summaries
| File | Description |
|---|---|
| tests/test-data/spdx3_multi_type.json | Updates fixture identifiers/arrays toward schema-conformant SPDX 3 JSON-LD. |
| tests/test-data/spdx3_minimal.json | Makes the “minimal” SPDX 3 fixture conformant (spdxId, CreationInfo typing, etc.). |
| tests/test-data/spdx3_legacy_spellings.json | Adds a deliberately non-conformant fixture to cover legacy producer spellings. |
| tests/test-data/spdx3_conformant.json | Adds a conformant SPDX 3 fixture used as a validation/control document. |
| tests/test_validation.py | Adds tests for SPDX 3.0.0 validation (not skipped) and version extraction behavior. |
| tests/test_spdx3.py | Tightens schema-validation assertions and adds fixture conformance checks. |
| tests/test_spdx3_conformance.py | Adds extensive round-trip conformance tests for SPDX 3 parse/write paths. |
| tests/test_serialization.py | Adds tests ensuring SPDX 3 license expressions in @graph are sanitized correctly. |
| tests/test_schema_compliance.py | Extends full-flow compliance tests to SPDX 3.0.0 and 3.0.1 fixtures. |
| tests/test_generation_plugin.py | Updates expectations for “no generator” errors to be actionable/user-facing. |
| tests/test_config.py | Adds tests ensuring SPEC_VERSION advice is routable and inputs are wired in action.yml. |
| sbomify_action/validation.py | Adds SPDX 3.0.0 schema mapping and fails unknown SPDX 3 versions instead of skipping. |
| sbomify_action/spdx3.py | Enhances SPDX 3 parsing/writing to preserve raw fields and normalize draft-vs-final spellings. |
| sbomify_action/serialization.py | Updates sanitize_spdx_licenses to also sanitize SPDX 3 license-expression elements in @graph. |
| sbomify_action/enrichment.py | Avoids adding a second declared license when SPDX 3 already declared via relationships. |
| sbomify_action/cli/main.py | Improves SPEC_VERSION validation messaging for SPDX 3 requests. |
| sbomify_action/augmentation.py | Avoids duplicating/contradicting SPDX 3 declared license relationships; supports override semantics. |
| sbomify_action/additional_packages.py | Reuses shared SPDX 3 CreationInfo creation to ensure schema-valid empty SPDX 3 docs. |
| sbomify_action/_generation/registry.py | Provides a clearer “no generator” error message using canonical version tuples. |
| README.md | Clarifies generate-vs-validate support matrix and explains SPDX 3 ingest-only behavior. |
| action.yml | Exposes sbom-format and spec-version as first-class inputs wired to env vars. |
Review details
- Files reviewed: 21/22 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Three from Copilot's review, all of them the same shape as the bugs this code exists to fix. A package listing both a purpose the library knows and one it does not came back with only the one it does not. Capture kept the strangers alone, restore overwrites the serialized list wholesale, so ["library", "specification"] became ["specification"]. The whole list is kept now whenever any of it is unknown. A document whose @context is a list or an object lost it. is_spdx3 and extract_spdx3_version both read every shape JSON-LD allows; only this one took the bare string, so context_url stayed unset and the writer fell back to the current release. That is the relabelling this branch added context_url to prevent, surviving in the one shape nobody tested. The minted agent stamped the wall clock, so two runs over one input differed by a line. It takes the timestamp from a CreationInfo that names it, which is both the stable answer and the honest one: the agent describes elements created at the document's own moment. Only a document stating no time at all falls back to now.
GitHub Actions passes `${{ inputs.x }}` as "" when the input is not set, so
SBOM_FORMAT arrived empty rather than unset and the CLI's click.Choice refused
it instead of falling back to cyclonedx. sbom-format declares its default the
way bom-type already does. spec-version declares an empty one on purpose,
since its real default is per format and lives in the CLI: 1.6 for CycloneDX,
2.3 for SPDX.
Separately, SPEC_VERSION=3.0 was told to pass an existing 3.0 document as
SBOM_FILE. That route fails too. "3.0" is a key in SPDX_SCHEMAS so an
alias-context document reaches a schema at all, but both official schemas pin
@context with a const to their fully qualified URL, so a document declaring
the bare line fails whichever one it is held to. It now gets the same
"nor is it read" answer as 3.1, naming the versions that do work.
It pinned /Users/<name>/PycharmProjects/... into a public repo, which is both unportable and a local path nobody needed to publish. The skip was therefore meaningless everywhere except the machine it was written on, which is the opposite of what a regression guard is for. Reads SBOMIFY_YOCTO_CORPUS now, falling back to tests/test-data/yocto. The documents are release artefacts of tens of megabytes and stay out of the repo, so the skip stays, but it names the variable to set.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are a couple of concrete correctness gaps (SPDX 3 @type handling in the sanitizer and a Yocto-guard test that can pass without processing any files) that should be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
sbomify_action/serialization.py:1348
- SPDX 3 JSON-LD commonly uses "@type" as well as "type". sanitize_spdx_licenses currently only matches element.get("type") == "simplelicensing_LicenseExpression", so it will miss (and report 0 fixes for) SPDX 3 documents that use "@type" for the licensing elements.
- Files reviewed: 21/22 changed files
- Comments generated: 1
- Review effort level: Lite
Two test classes were appended below `if __name__ == "__main__": unittest.main()`,
so running the file directly executed unittest at that line and exited before
they were ever defined. pytest imports the module and never notices, which is
why it stayed hidden.
python tests/test_config.py before: Ran 96 tests
python tests/test_config.py after: Ran 101 tests
Guard moved to the end, where it can see the whole file.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
SPDX 3 license set serialization currently risks changing license semantics by flattening nested AND/OR structures without parentheses.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
sbomify_action/spdx3.py:1080
- When
_license_expression_textcomposes a nestedConjunctiveLicenseSet/DisjunctiveLicenseSet, it joins member strings without parentheses. For nested sets this can change SPDX expression semantics due to AND/OR precedence (e.g.(MIT OR Apache-2.0) AND GPL-2.0would serialize asMIT OR Apache-2.0 AND GPL-2.0). Wrap nested set members in parentheses when their operator differs from the current joiner to preserve meaning.
- Files reviewed: 21/22 changed files
- Comments generated: 0 new
- Review effort level: Lite
SPDX binds AND tighter than OR, and set members were joined with no parentheses, so a disjunction inside a conjunction came back as "MIT OR Apache-2.0 AND GPL-2.0-only". That parses as MIT OR (Apache-2.0 AND GPL-2.0-only), which grants MIT on its own: a different licence claim from the one the producer wrote, published as though they had written it. A member is now parenthesised when it carries an operator the joiner does not, counted outside any parentheses it already has. The same operator still nests flat, because AND and OR are each associative and the parentheses would be noise. A member that is a bare expression string rather than a set is read the same way, which is the shape spdx3_license_from_string produces.
…ther Both the action.yml description and the README input row said SPDX 3 is not generated. Additional-packages-only mode does generate it: LOCK_FILE=none with SPEC_VERSION=3.0.1 builds a 3.0.1 document from ADDITIONAL_PACKAGES, and the error you get for asking a generator plugin for 3.0.1 already points there. Neither said what happens on the path most SPDX 3 users take. SPEC_VERSION is ignored outright when SBOM_FILE names a real file: the document is read, checked and written at the version it declares. Verified by running the same input at SPEC_VERSION unset, 2.2 and 1.6, which produce identical output.
Contributor
Author
|
Confirmed and fixed in 1f4a2da, which rides on #411. Reproduced exactly as described. A ConjunctiveLicenseSet holding a DisjunctiveLicenseSet serialised to A member is now parenthesised when it carries an operator the joiner does not, counted at paren depth zero. Two cases beyond the one you named are covered too: a member that is a bare expression string rather than a set, which is the shape Note for review: this defect is on master, not introduced by this PR. It shipped with #407. |
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The functional SPDX 3 sanitizer fix is well-covered by new tests and the only issue found is a docstring mismatch in a test guard (non-blocking).
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tests/test_serialization.py:2617
- The Yocto corpus used by this test defaults to
tests/test-data/yocto/*.spdx.json, and those fixtures are SPDX 2.2 (they start with"spdxVersion": "SPDX-2.2"). The docstring here currently describes it as the published Yocto SPDX 3 images and references counts from that external corpus, which makes the in-repo guard misleading.
Update the docstring to describe the actual default corpus (SPDX 2.x in-repo) and optionally note that SPDX 3 corpora can be supplied via SBOMIFY_YOCTO_CORPUS.
- Files reviewed: 21/22 changed files
- Comments generated: 0 new
- Review effort level: Lite
… version _three_part_spec_version scanned the graph for a producer-stated version, so in a merged document the element copied in from somewhere else could answer for the whole thing. That answer picks the @context the writer emits and the schema the result is then held to, so a document whose own CreationInfo says 3.0.1 could come back relabelled 3.0.0 and checked against the wrong schema. The SpdxDocument is asked first now, inline or referenced, which is the order extract_spdx3_version already uses for the same reason. A fragment with no SpdxDocument still falls back to the graph, and what the action minted is still skipped.
The docstring said 200 expressions across three published SPDX 3 images. That measurement is real but it is not what runs: widening the glob to match the in-repo fixtures pointed the default at six per-recipe SPDX 2.2 documents carrying six expressions between them, and the prose was never updated to match. Both are named now, along with the environment variable that switches to the published images. The guard itself is unchanged and still meaningful: the sanitizer walks 2.x as well as 3, and these are real Yocto output.
Contributor
Author
|
Right, and the mismatch is mine: fixed in 8ae542e. The 200-expressions-across-three-SPDX-3-images measurement is real, but it is not what the test runs. Widening the glob so the guard would actually find the in-repo fixtures pointed the default at six per-recipe SPDX 2.2 documents, and I left the prose describing the corpus it no longer uses. Counted rather than guessed: the six in-repo documents carry six licence expressions between them, in base-files, busybox and zlib. Thin, but real Yocto output rather than something written to pass, and the sanitizer walks 2.x as well as 3, so the guard still holds. The docstring now names what runs by default, what the quoted measurement came from, and the environment variable that switches to the published images. |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
sanitize_spdx_licenses still misses SPDX 3 license expressions when @graph is a single object (valid JSON-LD shape), so sanitization can incorrectly no-op in that case.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
sbomify_action/serialization.py:1354
- The SPDX 3 walk assumes
@graphis always a list (for element in data.get("@graph", [])). JSON-LD allows@graphto be a single object too, and this repo already handles that shape elsewhere (e.g.spdx3._document_spec_versionwraps dict graphs). In the dict case this loop iterates keys, sanitizes nothing, andsanitize_spdx_licensescan still incorrectly return 0 for SPDX 3 documents.
- Files reviewed: 21/22 changed files
- Comments generated: 0 new
- Review effort level: Lite
The SPDX 3 walk iterated data["@graph"] directly. JSON-LD allows that to be a single node object rather than an array, and iterating a dict walks its keys, so the sanitizer returned 0 for a document it had not looked at. The SPDX 3 schemas pin @graph to an array, so such a document fails validation whatever happens here, and this changes no verdict. It changes which answer is honest: "nothing to fix" about a document nobody read is the one number this function must never return, since every call site reads it as a clean bill.
Contributor
Author
|
Fixed in 28dfff6, with one correction to the premise. Reproduced: a document whose The correction: JSON-LD allows that shape, but SPDX 3 does not. Both shipped schemas pin Fixed it anyway, because the wrong part is the number rather than the outcome. Every call site reads a 0 as a clean bill, and "nothing to fix" about a document nobody looked at is the one answer this function must not give. One line, and it costs nothing if SPDX ever relaxes the constraint. Worth noting the inconsistency you spotted is real and points the other way too: |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
SPDX 3 license sanitization currently misses JSON-LD elements using @type, causing some valid SPDX 3 documents to be skipped and incorrectly reported as having no fixes.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 21/22 changed files
- Comments generated: 1
- Review effort level: Lite
JSON-LD states a type as `type` under the SPDX 3 context and as `@type` expanded. The licence walk read only `type`, so a conforming document using the expanded spelling was skipped and the sanitizer returned 0, which reads as "nothing to fix" at both call sites. spdx3.py already reads both spellings, and the component id two lines below already reads both spellings of the id.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes align sanitization with the real SPDX 3 data model and are backed by extensive schema-conformance and regression tests covering the updated behavior.
Review details
- Files reviewed: 21/22 changed files
- Comments generated: 0 new
- Review effort level: Lite
27 tasks
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
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.
Closes #334. Phase 3 of the SPDX 3 epic, sbomify/sbomify#1324.
Stacked on #412 → #411 → #410 → #409 → #408 → #407. Only the last commit belongs to this PR.
The no-op
sanitize_spdx_licenseswalkedpackages[],files[]andsnippets[]forlicenseConcluded,licenseDeclaredandlicenseInfoFromFiles. An SPDX 3 document has none of those: it states a licence as a Relationship to asimplelicensing_LicenseExpressionelement in@graph.So the function returned 0 having looked at nothing, and zero reads as "nothing to fix" at both call sites: on ingest for any SPDX input (
cli/main.py:2288), and again before validating generated SPDX (_generation/registry.py:707).The SPDX 3 walk is additive rather than an early return, so a document carrying both shapes gets both and the 2.x path is untouched either way.
The issue's justification did not survive checking
It says RPM-style strings "matter most for the Yocto path". They do not appear there. Every licence expression in the three published Yocto SPDX 3 images, run through this repo's own
_sanitize_spdx_license_expression:Yocto normalises licences itself, compound ones included:
No
GPLv2+, noASL 2.0. This is worth having for producers that do emit junk, and it is not the Yocto fix the issue frames it as. Anyone picking up related work should not go looking for those strings in Yocto output.What the risk actually is
Not missing a bad licence. Rewriting a good one. All five real Yocto documents in the corpus, SPDX 2.2 and 3 alike, come out byte identical, and a test pins that so a future change to the sanitizer cannot quietly start editing correct licences.
Tests
Five of the nine cases added here fail without the change. Full suite 3785 passed, 4 skipped, 0 failed.
ruffandmypyclean.Closes #334