Fix unknown-spdx identifier - #301
Conversation
|
Warning Review limit reached
Next review available in: 39 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe ScanCode parsers now resolve unknown licenses from SPDX declarations, preserve expression operators, suppress overlapping references, and rebuild detected-license comments. Tests and fixtures cover current and legacy parser behavior. ChangesScanCode license parsing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SPDXDeclaration
participant ScanCodeParser
participant ExpressionResolver
participant MatchedLicense
SPDXDeclaration->>ScanCodeParser: provide SPDX-License-Identifier text
ScanCodeParser->>ExpressionResolver: tokenize expression and preserve operators
ExpressionResolver->>MatchedLicense: resolve unknown tokens and suppress overlaps
MatchedLicense->>ScanCodeParser: return normalized detections
ScanCodeParser->>ScanCodeParser: rebuild detected-license comments
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
Signed-off-by: Park Wonjae <wonjae.park@lge.com>
Signed-off-by: Park Wonjae <wonjae.park@lge.com>
542f1bd to
8969424
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/fosslight_source/_parsing_scancode_file_item.py (1)
155-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare SPDX header extraction between both parser paths.
parsing_scancode_32_earlierandparsing_scancode_32_latercontain the same extraction and prefix-removal logic. Move it to one helper and cover it with a regression test for mixed-caseLicenseRef-values. This keeps both ScanCode result formats aligned.Also applies to: 285-292
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/fosslight_source/_parsing_scancode_file_item.py` around lines 155 - 162, Extract the duplicated SPDX header matching and case-insensitive LicenseRef- removal into a shared helper used by both parsing_scancode_32_earlier and parsing_scancode_32_later. Preserve the existing empty-result behavior when no header matches, and add a regression test covering mixed-case LicenseRef- values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/fosslight_source/_parsing_scancode_file_item.py`:
- Around line 155-162: Extract the duplicated SPDX header matching and
case-insensitive LicenseRef- removal into a shared helper used by both
parsing_scancode_32_earlier and parsing_scancode_32_later. Preserve the existing
empty-result behavior when no header matches, and add a regression test covering
mixed-case LicenseRef- values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e052d72-e04f-492f-875a-3bd7d1d3a031
📒 Files selected for processing (1)
src/fosslight_source/_parsing_scancode_file_item.py
Parse SPDX-License-Identifier when ScanCode reports unknown-spdx (including compound expressions), suppress redundant unknown-license-reference, and rebuild comments from the detected expression while preserving AND/OR operators.
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (4)
src/fosslight_source/_parsing_scancode_file_item.py (4)
383-395: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis substitution block is unreachable and can be removed.
The guard tests
KEYWORD_SCANCODE_UNKNOWN not in expr.lower(), whereKEYWORD_SCANCODE_UNKNOWNis"unknown-spdx". The SPDX formlicenseref-scancode-unknown-spdxcontains that substring, so the guard isFalseexactly when the firstre.subwould have work to do. The secondre.subis also unreachable for the same reason whenever anunknown-spdxtoken is present.The token tests at Lines 414 and 416 use
inon the lowercased token, so they already match theLicenseRef-scancode-forms. The normalization is not needed.🔧 Proposed removal
expr = detected_expression - if KEYWORD_SCANCODE_UNKNOWN not in expr.lower(): - expr = re.sub( - r'licenseref-scancode-unknown-spdx', - KEYWORD_SCANCODE_UNKNOWN, - expr, - flags=re.IGNORECASE, - ) - expr = re.sub( - r'licenseref-scancode-unknown-license-reference', - KEYWORD_UNKNOWN_LICENSE_REFERENCE, - expr, - flags=re.IGNORECASE, - ) - replacements = _build_unknown_spdx_replacement_queue(matches)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/fosslight_source/_parsing_scancode_file_item.py` around lines 383 - 395, Remove the unreachable substitution block guarded by KEYWORD_SCANCODE_UNKNOWN in the surrounding parsing function, including both re.sub calls; retain the existing token checks and normalization flow because they already handle the LicenseRef-scancode forms.
188-199: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe 32-earlier path does not normalize the resolved token.
The 32-later path applies
_normalize_license_tokento the resolved value at Line 529. This path assigns the raw declaration text tolicense_valueand only stripsreplace_wordcharacters. The same input can therefore produce two different license strings depending on the ScanCode result format.Consider applying
_normalize_license_tokenhere as well.🔧 Proposed change
if KEYWORD_SCANCODE_UNKNOWN in (key or ""): matched = SPDX_LICENSE_IDENTIFIER_PATTERN.search(matched_txt) if matched: - license_value = LICENSE_REF_PREFIX_PATTERN.sub( - '', matched.group(1).strip() - ) + declared = LICENSE_REF_PREFIX_PATTERN.sub( + '', matched.group(1).strip() + ) + license_value = _normalize_license_token(declared) or declared🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/fosslight_source/_parsing_scancode_file_item.py` around lines 188 - 199, Apply _normalize_license_token to the resolved license value in the KEYWORD_SCANCODE_UNKNOWN branch after extracting and stripping matched.group(1), matching the normalization used by the later path and ensuring consistent license strings across ScanCode result formats.
522-528: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis per-token resolution is unreachable.
Lines 503-511 already search the same
matched_txtforSPDX-License-Identifier. If the search succeeds,found_lic_listno longer containsunknown-spdx, so no token here matches. If the search fails, this second search fails for the same input. The block never changesfound_lic.Remove it once the resolution at Lines 503-511 is corrected.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/fosslight_source/_parsing_scancode_file_item.py` around lines 522 - 528, Remove the unreachable per-token resolution block guarded by KEYWORD_SCANCODE_UNKNOWN in the found_lic loop. Keep the earlier matched_txt resolution logic around resolved_unknown_spdx as the sole path for resolving unknown SPDX values, without changing the surrounding token processing.
160-166: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse a shared helper for the "other licenses" set.
This inline comprehension duplicates the intent of
_matched_texts_with_other_licensesat Lines 71-79. The two paths use different fields (keyhere,license_expressionthere), so the suppression policy can drift.Consider extracting a single helper that takes the license-token accessor as a parameter. The
unknown-spdxexclusion noted on Lines 61-79 applies to this path as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/fosslight_source/_parsing_scancode_file_item.py` around lines 160 - 166, Replace the inline matched_texts_with_other_licenses comprehension in the surrounding parsing flow with the shared _matched_texts_with_other_licenses helper. Refactor that helper to accept the license-token accessor needed by each path, using key here and license_expression in the existing path, while consistently excluding KEYWORD_UNKNOWN_LICENSE_REFERENCE tokens.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/fosslight_source/_parsing_scancode_file_item.py`:
- Around line 555-572: Preserve any truncation notice added by the
ScanItem.licenses setter when assigning comments in the detected-expression
handling block. Update the unknown-SPDX and “OR” branches around
result_item.comment so generated license text is appended to the existing
comment rather than replacing it, while retaining the current behavior when no
prior comment exists.
- Around line 333-337: Update the unknown-index handling in
build_comment_from_detected_expression so iteration stops once declared_tokens
is exhausted instead of reusing declared_tokens[-1]. Match the existing
mixed-branch behavior by only enqueueing licenses for available declarations,
allowing downstream processing to skip unmatched unknown-spdx tokens safely.
- Around line 512-531: Update the license filtering flow in the loop handling
split_spdx_expression so _normalize_license_token runs before the REMOVE_LICENSE
membership check. Preserve the existing unknown-license handling, then skip any
token whose normalized value is empty or appears in REMOVE_LICENSE, matching the
ordering used by build_comment_from_detected_expression.
- Around line 501-511: Update the unknown SPDX handling in the shown parsing
flow so it replaces only matching unknown-license tokens within found_lic_list,
preserving known licenses and compound operators. Reuse the per-token
substitution behavior from _build_unknown_spdx_replacement_queue rather than
assigning the entire expression to declared, and keep resolved_unknown_spdx set
when a replacement occurs.
- Around line 19-22: Update SPDX_LICENSE_IDENTIFIER_PATTERN to exclude trailing
comment terminators from captured license values, then move
LICENSE_REF_PREFIX_PATTERN application into _declared_licenses_from_matched_text
after splitting the declaration so every token is stripped independently. Apply
the same per-token stripping in the direct call sites around the license parsing
logic, while preserving existing tokenization and MatchedLicense construction.
- Around line 61-79: Update _expression_has_non_unknown_license_reference so
unknown-spdx is not treated as a real license when determining matched texts
with other licenses. Ensure _matched_texts_with_other_licenses only marks text
for suppression when the expression contains an actual non-unknown license,
preserving unknown-license-reference when no replacement license is resolved.
- Around line 429-434: Update the operator handling around the kept-licenses
accumulation so skipped tokens do not cause the next kept license to use
ops[token_idx - 1]. Carry operators across suppressed tokens and, when merging
skipped-token relations, preserve the safer OR relation over AND; ensure
expressions such as MIT OR unknown-license-reference AND Apache-2.0 produce MIT
OR Apache-2.0.
- Around line 303-356: Update build_comment_from_detected_expression to assign
unknown-spdx replacements by their originating ScanCode match rather than
consuming the file-level queue sequentially with repl_idx. Change
_build_unknown_spdx_replacement_queue to return a source-keyed mapping, such as
match index or (license_expression, matched_text), and use that key while
walking expression tokens so each unknown-spdx token receives its corresponding
detected license regardless of ordering.
In `@tests/test_files/dual_unknow.py`:
- Around line 40-54: Update the option-parsing flow around getopt.getopt so the
-h branch prints help and immediately returns before run_scan(); catch
getopt.GetoptError specifically, print the help message, and return without
scanning or generating reports. Preserve normal processing for valid non-help
options.
In `@tests/test_files/dual.txt`:
- Line 4: Update the SPDX license expression in the dual-license fixture to use
the uppercase OR operator, while preserving the existing license identifiers and
expression structure.
---
Nitpick comments:
In `@src/fosslight_source/_parsing_scancode_file_item.py`:
- Around line 383-395: Remove the unreachable substitution block guarded by
KEYWORD_SCANCODE_UNKNOWN in the surrounding parsing function, including both
re.sub calls; retain the existing token checks and normalization flow because
they already handle the LicenseRef-scancode forms.
- Around line 188-199: Apply _normalize_license_token to the resolved license
value in the KEYWORD_SCANCODE_UNKNOWN branch after extracting and stripping
matched.group(1), matching the normalization used by the later path and ensuring
consistent license strings across ScanCode result formats.
- Around line 522-528: Remove the unreachable per-token resolution block guarded
by KEYWORD_SCANCODE_UNKNOWN in the found_lic loop. Keep the earlier matched_txt
resolution logic around resolved_unknown_spdx as the sole path for resolving
unknown SPDX values, without changing the surrounding token processing.
- Around line 160-166: Replace the inline matched_texts_with_other_licenses
comprehension in the surrounding parsing flow with the shared
_matched_texts_with_other_licenses helper. Refactor that helper to accept the
license-token accessor needed by each path, using key here and
license_expression in the existing path, while consistently excluding
KEYWORD_UNKNOWN_LICENSE_REFERENCE tokens.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3437e1f4-51b2-4b4c-98fc-0dd43d3e15fb
📒 Files selected for processing (6)
src/fosslight_source/_parsing_scancode_file_item.pytests/test_files/dual.txttests/test_files/dual_unknow.pytests/test_files/run_scancode.pytests/test_files/temp.cpptests/test_files/test_unknown_spdx.txt
Strip trailing */ and --> from SPDX-License-Identifier captures, and remove LicenseRef- on each AND/OR token instead of only the first.
Parse detected expressions as a tree so skipped tokens respect parentheses, and without parens use left-associative AND/OR (keep the operator before the next kept token).
Keep grouping parentheses when three or more licenses are present, but drop redundant outer parens for simple dual-license comments like (A OR B).
Collapse per-subdir test annotations into a single tests/** override.
tests/** is annotated as Apache-2.0, so MIT and LicenseRef-MIT-like were unused and failed REUSE lint.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_parsing_unknown_spdx.py (1)
179-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest compound declarations through
parsing_scancode_32_earlier.The legacy integration test uses one declared token only. Compound declaration tests currently exercise the helper or
parsing_scancode_32_later.Add legacy parser cases for both
ANDandOR. Assert the individual restored license tokens.Proposed test expansion
+@pytest.mark.parametrize( + ("matched_text", "expected_licenses"), + [ + ( + "/* SPDX-License-Identifier: LicenseRef-MIT-like */", + ["MIT-like"], + ), + ( + "/* SPDX-License-Identifier: LicenseRef-Foo AND LicenseRef-Bar */", + ["Foo", "Bar"], + ), + ( + "/* SPDX-License-Identifier: LicenseRef-Foo OR LicenseRef-Bar */", + ["Foo", "Bar"], + ), + ], +) -def test_legacy_unknown_spdx_uses_declared_identifier(): +def test_legacy_unknown_spdx_uses_declared_identifier( + matched_text, expected_licenses +): scancode_file_list = [{ "path": "example.sol", "type": "file", "licenses": [{ "key": "unknown-spdx", - "matched_text": "/* SPDX-License-Identifier: LicenseRef-MIT-like */", + "matched_text": matched_text, }], "copyrights": [], }] success, results, _messages, _ = parsing_scancode_32_earlier(scancode_file_list) assert success is True - assert results[0].licenses == ["MIT-like"] + assert results[0].licenses == expected_licenses🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_parsing_unknown_spdx.py` around lines 179 - 194, Add legacy integration test cases through parsing_scancode_32_earlier for compound SPDX declarations using both AND and OR. Build each case with the corresponding declared identifier, assert successful parsing, and verify results[0].licenses contains the individual restored license tokens rather than a combined expression.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@REUSE.toml`:
- Around line 11-14: Update the tests/** annotation in REUSE.toml so precedence
= "override" applies only to files requiring the 2021 LG Electronics metadata,
while preserving declared copyright notices such as the 2026 notice in
tests/test_parsing_unknown_spdx.py.
---
Nitpick comments:
In `@tests/test_parsing_unknown_spdx.py`:
- Around line 179-194: Add legacy integration test cases through
parsing_scancode_32_earlier for compound SPDX declarations using both AND and
OR. Build each case with the corresponding declared identifier, assert
successful parsing, and verify results[0].licenses contains the individual
restored license tokens rather than a combined expression.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 500af39b-9474-44bc-a3a4-c040a95af404
📒 Files selected for processing (6)
LICENSES/LicenseRef-MIT-like.txtLICENSES/MIT.txtREUSE.tomlsrc/fosslight_source/_parsing_scancode_file_item.pytests/test_files/run_scancode.pytests/test_parsing_unknown_spdx.py
💤 Files with no reviewable changes (2)
- LICENSES/LicenseRef-MIT-like.txt
- LICENSES/MIT.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_files/run_scancode.py
- src/fosslight_source/_parsing_scancode_file_item.py
Align unknown-spdx comment rebuild with the existing dual-license rule: only write Comment when the detected expression includes OR.
soimkim
added
bug fix
2 participants
Summary
LicenseRef-prefixes, regardless of capitalization.ANDandORin license expressions.