refactor: SPDX expression handling is the SDK's, not a local wrapper (phase 2.2) - #428
Merged
Conversation
Phase 2.2, which the train skipped: internal/licenseexpr exposed Valid, ValidateAll, Identifier, Compose, Satisfies and Extract, and bomly-sdk/spdxkit exposes the same six with the same signatures. Two implementations of one specification's semantics is the drift this program exists to remove, and it was sitting directly under the next phase's work. The call sites move to the kit, the package is deleted, and go-spdx drops to indirect. No golden moves: the two Compose implementations agree on every fixture the smoke suite produces, which is the check that mattered, since composition is the one function where parenthesization could have differed. The panic guard survives the move -- the kit carries it -- so the rule it protected changes shape rather than disappearing. It used to be "route through the local wrapper"; it is now "do not import the parser at all", and TestNoDirectSPDXExpressionUse moves to internal/detectors/guards_test.go where the other tree-wide structural guards live. It scans test files too, because a test reaching the parser directly proves the same crash is reachable and is where the temptation lives. Mutation-checked by importing go-spdx into internal/sbom. This is the prerequisite for 2.4 (#410): LicenseRef-* minting, hasExtractedLicensingInfos, and mixed-validity composition are all already in spdxkit -- MintLicenseRef, Classify, Compose -- and its doc comments cite the issue by number. Building that on the local wrapper would have meant hand-rolling identifier sanitization and collision-avoidance beside a library that already owns both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
Warning Review limit reachedNext included review available in 37 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (13)
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 |
Contributor
Bomly Diff SummaryCompared Overview
Dependency ChangesSummary: 0 added, 0 version changed, 1 detail change, 0 removed. Dependency Detail ChangesReview: 0 of 1 detail change needs extra review.
Vulnerabilities✅ No vulnerability changes. License Changes✅ No license changes. Project Posture✅ No project posture changes ( Policy Findings✅ No policy differences were identified. |
bomly-guy
added a commit
that referenced
this pull request
Sep 5, 2026
… (phase 2.4 + SDK v0.9.2) (#429) * feat(sbom): unrecognized licenses export as LicenseRef, not free text Closes #410. Phase 2.4. SPDX 2.3 has no free-text license field: licenseDeclared must hold a valid expression, NOASSERTION, NONE, or a LicenseRef. Bomly wrote an unrecognized value verbatim -- "see LICENSE file" landed in licenseDeclared as-is -- producing a document a strict consumer can reject. Each such value now mints a reference and the original text travels beside it in hasExtractedLicensingInfos, which is what SPDX defines for this case and, unlike NOASSERTION, keeps the information: ingest reads the text back. The quieter half was the mixed-validity fallback. A set of one recognized and one unrecognized license kept only the first and dropped the rest, which a comment described as deliberate because composing free text would produce something that does not parse. A LicenseRef is a valid expression element, so the set composes whole now and nothing a source declared is lost. Minting is bomly-sdk/spdxkit's, not this package's. A reference has to be deterministic, collision-free across components assembled without coordination, and confined to the characters the SPDX idstring grammar allows; MintLicenseRef answers all three by hashing the whitespace-normalized text, and a sanitizer written here would be a second, worse answer to a question the SDK already settled. What stays local is policy: which values classify how, where the extracted texts attach, and how ingest maps them back. Ingest resolves only an atomic reference. A compound expression naming one among other terms has no single text to become, and substituting free text into it would produce something that no longer parses. A foreign document's own reference-to-text pairing is taken as stated rather than re-minted: there the document is the authority, not our hash. docs/SBOM.md and ADR-0035 drop the limitation they recorded; the ADR gets a dated note rather than a rewritten body, since it recorded a decision that was true when made. The note also redirects its references to internal/licenseexpr, which #428 deleted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore: adopt bomly-sdk v0.9.2, retiring two CLI stopgaps v0.9.2 closes both SDK issues this branch's predecessors filed, so the workarounds they left behind can go. sdk#43 landed as NodePURL. The projection existed twice here and deliberately so -- internal/graphview for the renderers and internal/sbom for the codec, because a codec importing the CLI's output layer inverts the layering -- with both commented as converging on the accessor once it shipped. It shipped; both delegate; the question every surface asks of a node has one answer now. graphview keeps ChildrenAmong and TopLevelParentIDs, which are CLI policy rather than model semantics. sdk#39 landed as PEP 440 canonicalization before minting, so two spellings of one PyPI release fold again. TestPythonVersionCaseIsNotFoldedYet existed to make that gap visible rather than silent, and its failure message said to fold the case back into TestEquivalentPythonSpellingsFoldOnInsertion and delete it once a normalizer arrived. Done exactly that: the fold test carries the "1.0.0RC1" spelling again. No golden drift. The fixtures' PyPI versions are already canonical -- pip, poetry and uv all write normalized versions -- so the change reaches only the hand-written requirements case it was filed for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Phase 2.2, which the train skipped — and the prerequisite for 2.4.
What was wrong
internal/licenseexprexposedValid,ValidateAll,Identifier,Compose,Satisfies,Extract.bomly-sdk/spdxkitexposes the same six, with the same signatures. Two implementations of one specification's semantics, and the second one is the authoritative home the SDK program built for exactly this.I found it while starting 2.4: nothing under
internal/importedspdxkitat all.What changes
internal/auditors/license,internal/sbom/{transform,spdx23,cyclonedx}) move to the kit.internal/licenseexpris deleted;github.com/github/go-spdxdrops to indirect.TestNoDirectSPDXExpressionUsemoves tointernal/detectors/guards_test.goalongside the other tree-wide structural guards, and now scans test files too, since a test reaching the parser directly proves the same crash is reachable.CLAUDE.md,AGENTS.md,dev-docs/ARCHITECTURE.md, andtest/assurance/PARSER_FUZZING.md.Why it blocks 2.4
Everything #410 asks for is already in
spdxkit—MintLicenseRef(SHA-256 of whitespace-normalized text, 128-bit hex, SPDX-idstring-safe),Classify,Compose,LicenseRefsIn,ReplaceLicenseRef— and its doc comments cite the issue by number. Building LicenseRef emission on the local wrapper would have meant hand-rolling identifier sanitization and collision-avoidance next to a library that already owns both.Delegation check
LicenseRef-*minting:bomly-sdk/spdxkit— delegated, local duplicate deleted.github.com/github/go-spdx: the kit — enforced tree-wide byTestNoDirectSPDXExpressionUse.Verification
make verify SMOKE=1green, zero golden drift — the check that mattered, sinceComposeis the one function where parenthesization could have differed between the two implementations.make fuzz FUZZTIME=5sclean, includingFuzzSPDXLicenseValue. Guard mutation-checked by importing go-spdx intointernal/sbom.🤖 Generated with Claude Code