Chore/refactor inspect module #161
Conversation
|
Caution Review failedFailed to post review comments WalkthroughRefactors inspection into Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI / Caller
participant Check as PolicyCheck (ABC)
participant Proc as ScanResultProcessor
participant Formatter as Formatter (JSON/MD/JIRA)
participant Out as Output/File
CLI->>Check: run()
alt needs scan results
Check->>Proc: get_results() / get_components_data(...) / convert_components_to_list(...)
Proc-->>Check: components, dependencies
end
Check->>Formatter: _generate_formatter_report(components)
Formatter-->>Check: PolicyOutput(details, summary)
Check->>Out: write(policy_output.details)
Check-->>CLI: return (status, policy_output)
note right of Formatter #DDEBF7: PolicyOutput (namedtuple) replaces dict {'details','summary'}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
src/scanoss/inspection/policy_check/policy_check.py (1)
188-220: Unreachable branch and inconsistent fallback in _generate_formatter_report.
- _get_formatter() raises on invalid format, so “if formatter is None” is dead code.
- Fallback returns {} (not a PolicyOutput), diverging from the new API.
Handle the error path consistently and drop the unreachable check.
- formatter = self._get_formatter() - if formatter is None: - return PolicyStatus.ERROR.value, {} + try: + formatter = self._get_formatter() + except ValueError as e: + self.print_stderr(f'Error: {e}') + return PolicyStatus.ERROR.value, PolicyOutput(details='', summary='')src/scanoss/inspection/utils/scan_result_processor.py (2)
25-33: Fix CI lint: remove unused import and sort imports.abstractmethod is unused; ruff flags I001 (import order) and F401 (unused). Remove and order imports.
-from abc import abstractmethod -from enum import Enum -from typing import Any, Dict, TypeVar - -from ..utils.file_utils import load_json_file -from ..utils.license_utils import LicenseUtil -from ...scanossbase import ScanossBase +from enum import Enum +from typing import Any, Dict + +from ...scanossbase import ScanossBase +from ..utils.file_utils import load_json_file +from ..utils.license_utils import LicenseUtil
271-279: Bug: assigning missing dependency version to the wrong object.When dependency['version'] is missing, code sets c['version'] instead of dependency['version'].
- if not version: - self.print_debug(f'WARNING: Result missing version. Setting it to unknown: {c}') - version = 'unknown' - c['version'] = version # Set an 'unknown' version to the current component + if not version: + self.print_debug(f'WARNING: Dependency missing version. Setting it to unknown: {dependency}') + version = 'unknown' + dependency['version'] = version # Set 'unknown' on the dependency itselfsrc/scanoss/inspection/policy_check/dependency_track/project_violation.py (2)
379-401: Fix Markdown/Jira heading inconsistency in _md_summary_generatorNone-path uses Jira “h3.” while normal path uses “###”. Make header conditional on table_generator.
def _md_summary_generator(self, project_violations: list[PolicyViolationDict], table_generator) -> PolicyOutput: + header_prefix = 'h3. ' if table_generator is generate_jira_table else '### ' @@ - if project_violations is None: + if project_violations is None: self.print_stderr('Warning: No project violations found. Returning empty results.') return PolicyOutput( - details= "h3. Dependency Track Project Violations\n\nNo policy violations found.\n", + details= f"{header_prefix}Dependency Track Project Violations\n\nNo policy violations found.\n", summary= "0 policy violations were found.\n", ) @@ - return PolicyOutput( - details= f'### Dependency Track Project Violations\n{table_generator(headers, rows, c_cols)}\n\n' + return PolicyOutput( + details= f'{header_prefix}Dependency Track Project Violations\n' + f'{table_generator(headers, rows, c_cols)}\n\n' f'View project in Dependency Track [here]({self.url}/projects/{self.project_id}).\n', summary= f'{len(project_violations)} policy violations were found.\n' )Also applies to: 426-430
432-479: Align run() with base class: return tuple[int, PolicyOutput] instead of intThe base class PolicyCheck.run() returns tuple[int, PolicyOutput], but DependencyTrackProjectViolationPolicyCheck.run() returns only int across all five return paths. This breaks the API contract. All return statements (lines 438, 442, 447, 475, 477) must be wrapped to return (status_code, PolicyOutput). PolicyOutput is already imported and constructed throughout the file.
Apply the proposed diff to all five return statements, ensuring error paths also construct appropriate PolicyOutput instances with meaningful error messages.
src/scanoss/inspection/policy_check/scanoss/copyleft.py (1)
133-169: Fix centered column index and make Jira/MD headers consistentIndex 4 doesn’t exist for 4 columns; also prefer Jira header when generating Jira tables.
- headers = ['Component', 'License', 'URL', 'Copyleft'] - centered_columns = [1, 4] + headers = ['Component', 'License', 'URL', 'Copyleft'] + centered_columns = [1, 3] @@ - return PolicyOutput( - details= f'### Copyleft Licenses\n{table_generator(headers, rows, centered_columns)}', + header_prefix = 'h3. ' if table_generator is generate_jira_table else '### ' + return PolicyOutput( + details= f'{header_prefix}Copyleft Licenses\n{table_generator(headers, rows, centered_columns)}', summary= f'{len(component_licenses)} component(s) with copyleft licenses were found.\n', )src/scanoss/inspection/policy_check/scanoss/undeclared_component.py (2)
94-108: Avoid KeyError when stripping countersdel on missing keys will throw. Use pop with default.
- # Remove unused keys - del component['count'] - del component['declared'] - del component['undeclared'] + # Remove unused keys defensively + component.pop('count', None) + component.pop('declared', None) + component.pop('undeclared', None)
136-146: Jira summary ends with literal “\n”Use a newline escape, not a literal backslash-n.
- return f'{len(components)} undeclared component(s) were found.\\n' + return f'{len(components)} undeclared component(s) were found.\n'
🧹 Nitpick comments (15)
src/scanoss/inspection/policy_check/policy_check.py (4)
86-101: Docstring return shape is outdated (now returns PolicyOutput).Update to reflect tuple[int, PolicyOutput]; current text mentions “named tuple” and “PolicyStatus enum value”. Keep consistent across the project.
104-116: Formatter method docstrings still say “dict”; they return PolicyOutput.Align _json/_markdown/_jira_markdown docstrings with the PolicyOutput return type to avoid confusion for implementors.
Also applies to: 118-142
144-159: Typing polish for formatter selection.Use the generic type var in the callable signature and avoid mixing List/dict with builtin generics.
- def _get_formatter(self) -> Callable[[List[dict]], PolicyOutput]: + def _get_formatter(self) -> Callable[[list[T]], PolicyOutput]:
69-85: Remove unused dependency from base.self.license_util is never used in the base class; instantiate in subclasses that need it.
src/scanoss/inspection/summary/component_summary.py (1)
27-33: Type hints and unused generic T.These methods are placeholders; importing T from policy_check just for hints couples modules unnecessarily. Prefer Any or local TypeVar if needed.
-from ..policy_check.policy_check import T +from typing import Any @@ - def _markdown(self, data: list[T]) -> dict[str, Any]: + def _markdown(self, data: list[Any]) -> dict[str, Any]: @@ - def _jira_markdown(self, data: list[T]) -> dict[str, Any]: + def _jira_markdown(self, data: list[Any]) -> dict[str, Any]:Also applies to: 77-100
tests/test_policy_inspect.py (1)
58-62: Strengthen assertion semantics for empty-path case.assertTrue(success, 2) doesn’t assert the status code; assert against PolicyStatus to lock behavior.
- success, results = copyleft.run() - self.assertTrue(success, 2) + status, _ = copyleft.run() + self.assertEqual(status, PolicyStatus.ERROR.value)src/scanoss/inspection/utils/scan_result_processor.py (3)
82-84: get_results typing can be Optional.Method can return None on JSON parse errors. Adjust hint to Optional[Dict[str, Any]].
- def get_results(self) -> Dict[str, Any]: + def get_results(self) -> dict[str, Any] | None:
392-401: Dedup check uses spdxid instead of composite key.To enforce uniqueness per component-license pair, check key existence rather than spdxid.
- spdxid = lic.get('spdxid', 'unknown') - if spdxid not in component_licenses: - key = f'{purl}-{spdxid}' + spdxid = lic.get('spdxid', 'unknown') + key = f'{purl}-{spdxid}' + if key not in component_licenses: component_licenses[key] = {
53-60: Remove unused TypeVar T and tidy constructor docs.T isn’t used in this module; drop it and keep docstring concise.
-T = TypeVar('T') class ScanResultProcessor(ScanossBase): @@ - def __init__( # noqa: PLR0913 + def __init__( # noqa: PLR0913Also applies to: 66-81
src/scanoss/inspection/policy_check/dependency_track/project_violation.py (2)
268-270: Comment vs logic mismatch on zero timestampsText says “all timestamps are zero” but condition triggers when any is zero. Clarify or change to all().
- if last_vulnerability_analysis == 0 or last_occurrence == 0 or last_import == 0: + if all(v == 0 for v in (last_vulnerability_analysis, last_occurrence, last_import)): self.print_stderr(f'Warning: Some project data appears to be unset. Returning False: {dt_project}') return False
360-377: Optional: stable sort with secondary keysCurrent sort uses only type. Consider secondary keys (e.g., violationState, timestamp desc) for deterministic output.
- type_priority = {'SECURITY': 3, 'LICENSE': 2, 'OTHER': 1} - return sorted( - violations, - key=lambda x: -type_priority.get(x.get('type', 'OTHER'), 1) - ) + type_priority = {'SECURITY': 3, 'LICENSE': 2, 'OTHER': 1} + state_priority = {'FAIL': 2, 'WARN': 1} + return sorted( + violations, + key=lambda x: ( + -type_priority.get(x.get('type', 'OTHER'), 1), + -state_priority.get(x.get('policyCondition', {}).get('policy', {}).get('violationState', ''), 0), + -x.get('timestamp', 0), + ), + )src/scanoss/inspection/summary/license_summary.py (2)
29-31: Type var import likely unnecessary/fragileImporting T from policy_check ties this module to a generic param it doesn’t use. Replace list[T] with concrete types to avoid NameError if T isn’t exported.
-from ..policy_check.policy_check import T +from typing import AnyAlso update annotations below:
- def _markdown(self, data: list[T]) -> dict[str, Any]: + def _markdown(self, data: list[dict[str, Any]]) -> dict[str, Any]: @@ - def _jira_markdown(self, data: list[T]) -> dict[str, Any]: + def _jira_markdown(self, data: list[dict[str, Any]]) -> dict[str, Any]:If T is exported and intended, please confirm and ignore this suggestion.
45-77: Doc default mismatch and constructor clarityDocstring says “trace default True” but default is False; adjust text to avoid confusion. Consider documenting that ScanResultProcessor loads file at init (may raise early).
src/scanoss/inspection/policy_check/scanoss/copyleft.py (1)
207-214: Unify error-path returns with base typeOn error you return (status, {}), but base expects PolicyOutput. Return an empty PolicyOutput for consistency.
- if components is None: - return PolicyStatus.ERROR.value, {} + if components is None: + return PolicyStatus.ERROR.value, PolicyOutput(details='', summary='Error: no components.') @@ - return self._generate_formatter_report(copyleft_components) + return self._generate_formatter_report(copyleft_components)Confirm downstream callers don’t rely on dict; tests should assert on PolicyOutput.details/summary.
Also applies to: 229-236
src/scanoss/inspection/policy_check/scanoss/undeclared_component.py (1)
185-201: Minor: include headings in Markdown/Jira detailsCurrent Markdown details have a header; Jira details don’t. Consider adding header_prefix logic as done in Copyleft for consistency.
Also applies to: 203-219
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
CHANGELOG.md(2 hunks)Makefile(1 hunks)src/scanoss/__init__.py(1 hunks)src/scanoss/cli.py(1 hunks)src/scanoss/inspection/policy_check/dependency_track/project_violation.py(9 hunks)src/scanoss/inspection/policy_check/policy_check.py(8 hunks)src/scanoss/inspection/policy_check/scanoss/copyleft.py(8 hunks)src/scanoss/inspection/policy_check/scanoss/undeclared_component.py(8 hunks)src/scanoss/inspection/summary/component_summary.py(3 hunks)src/scanoss/inspection/summary/license_summary.py(4 hunks)src/scanoss/inspection/utils/scan_result_processor.py(9 hunks)tests/test_policy_inspect.py(21 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
src/scanoss/inspection/policy_check/dependency_track/project_violation.py (4)
src/scanoss/services/dependency_track_service.py (1)
DependencyTrackService(31-132)src/scanoss/inspection/utils/markdown_utils.py (2)
generate_jira_table(52-63)generate_table(25-50)src/scanoss/inspection/policy_check/policy_check.py (6)
PolicyCheck(55-219)PolicyOutput(49-51)PolicyStatus(33-44)_json(105-116)_markdown(119-129)_jira_markdown(132-142)src/scanoss/scanossbase.py (2)
print_to_file_or_stdout(83-94)print_to_file_or_stderr(96-107)
src/scanoss/inspection/policy_check/policy_check.py (3)
src/scanoss/scanossbase.py (3)
ScanossBase(28-107)print_to_file_or_stdout(83-94)print_to_file_or_stderr(96-107)src/scanoss/inspection/utils/license_utils.py (1)
LicenseUtil(52-132)src/scanoss/inspection/policy_check/dependency_track/project_violation.py (2)
run(432-478)_json(174-187)
src/scanoss/inspection/summary/license_summary.py (2)
src/scanoss/scanossbase.py (1)
ScanossBase(28-107)src/scanoss/inspection/utils/scan_result_processor.py (6)
ScanResultProcessor(54-402)group_components_by_license(353-402)get_results(82-83)get_components_data(196-247)get_dependencies_data(249-284)convert_components_to_list(299-311)
src/scanoss/inspection/policy_check/scanoss/undeclared_component.py (3)
src/scanoss/inspection/policy_check/policy_check.py (6)
PolicyCheck(55-219)PolicyOutput(49-51)PolicyStatus(33-44)_json(105-116)_markdown(119-129)_jira_markdown(132-142)src/scanoss/inspection/utils/markdown_utils.py (2)
generate_jira_table(52-63)generate_table(25-50)src/scanoss/inspection/utils/scan_result_processor.py (5)
ScanResultProcessor(54-402)group_components_by_license(353-402)get_results(82-83)get_components_data(196-247)convert_components_to_list(299-311)
src/scanoss/inspection/policy_check/scanoss/copyleft.py (3)
src/scanoss/inspection/policy_check/policy_check.py (6)
PolicyCheck(55-219)PolicyOutput(49-51)PolicyStatus(33-44)_json(105-116)_markdown(119-129)_jira_markdown(132-142)src/scanoss/inspection/utils/markdown_utils.py (2)
generate_jira_table(52-63)generate_table(25-50)src/scanoss/inspection/utils/scan_result_processor.py (6)
ScanResultProcessor(54-402)group_components_by_license(353-402)get_results(82-83)get_components_data(196-247)get_dependencies_data(249-284)convert_components_to_list(299-311)
src/scanoss/cli.py (6)
src/scanoss/inspection/policy_check/dependency_track/project_violation.py (1)
DependencyTrackProjectViolationPolicyCheck(122-478)src/scanoss/inspection/policy_check/scanoss/copyleft.py (1)
Copyleft(48-235)src/scanoss/inspection/policy_check/scanoss/undeclared_component.py (1)
UndeclaredComponent(47-307)src/scanoss/inspection/summary/component_summary.py (1)
ComponentSummary(32-167)src/scanoss/inspection/summary/license_summary.py (1)
LicenseSummary(33-188)src/scanoss/inspection/summary/match_summary.py (1)
MatchSummary(63-287)
src/scanoss/inspection/utils/scan_result_processor.py (2)
src/scanoss/scanossbase.py (1)
ScanossBase(28-107)src/scanoss/inspection/utils/license_utils.py (2)
LicenseUtil(52-132)init(68-105)
tests/test_policy_inspect.py (6)
src/scanoss/inspection/policy_check/dependency_track/project_violation.py (2)
DependencyTrackProjectViolationPolicyCheck(122-478)run(432-478)src/scanoss/inspection/policy_check/policy_check.py (2)
PolicyStatus(33-44)run(87-102)src/scanoss/inspection/policy_check/scanoss/copyleft.py (2)
Copyleft(48-235)run(215-235)src/scanoss/inspection/policy_check/scanoss/undeclared_component.py (2)
UndeclaredComponent(47-307)run(285-307)src/scanoss/inspection/summary/component_summary.py (2)
ComponentSummary(32-167)run(162-167)src/scanoss/inspection/summary/license_summary.py (2)
LicenseSummary(33-188)run(183-188)
src/scanoss/inspection/summary/component_summary.py (2)
src/scanoss/scanossbase.py (1)
ScanossBase(28-107)src/scanoss/inspection/utils/scan_result_processor.py (5)
ScanResultProcessor(54-402)group_components_by_license(353-402)get_results(82-83)get_components_data(196-247)convert_components_to_list(299-311)
🪛 GitHub Actions: Lint
src/scanoss/inspection/utils/scan_result_processor.py
[error] 25-25: I001 [ruff] Import block is un-sorted or un-formatted. Organize imports.
[error] 25-25: F401 [ruff] 'abc.abstractmethod' imported but unused. Remove unused import.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
src/scanoss/__init__.py (1)
25-25: LGTM! Version bump aligns with the changelog.The patch version increment is appropriate for this refactoring PR.
CHANGELOG.md (2)
12-12: Verify the release date.The release date is set to 2025-10-29, which is tomorrow. Please confirm this is intentional for a planned release, or update to today's date (2025-10-28) if this was unintentional.
13-24: LGTM! Changelog entries are comprehensive and well-structured.The changelog accurately documents the refactoring changes and the new Makefile targets. The version comparison links are correctly updated.
Also applies to: 726-727
src/scanoss/cli.py (1)
72-79: Import path refactor looks correct.CLI now targets the new inspection.* locations. No issues spotted.
tests/test_policy_inspect.py (2)
72-77: Good switch to PolicyOutput in tests.Parsing details via json.loads and asserting summary text exercises the new contract.
441-471: DT: formatter unit tests look solid.They validate both summary count and details content/headers for json/md.
agustingroh
force-pushed
the
chore/refactor-inspect-module
branch
from
89b45cf to
ccc40e6
Compare
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
agustingroh
force-pushed
the
chore/refactor-inspect-module
branch
from
ccc40e6 to
e6c5aa2
Compare
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
src/scanoss/inspection/policy_check/policy_check.py (2)
188-220: Unreachable branch and inconsistent return type in_generate_formatter_report.
_get_formatternever returns None now; also fallback returns{}instead ofPolicyOutput.- def _generate_formatter_report(self, components: list[Dict]): + def _generate_formatter_report(self, components: list[Dict]) -> tuple[int, PolicyOutput]: @@ - formatter = self._get_formatter() - if formatter is None: - return PolicyStatus.ERROR.value, {} + formatter = self._get_formatter() @@ - policy_output = formatter(components) + policy_output = formatter(components) @@ - if len(components) > 0: - return PolicyStatus.POLICY_FAIL.value, policy_output - return PolicyStatus.POLICY_SUCCESS.value, policy_output + if len(components) > 0: + return PolicyStatus.POLICY_FAIL.value, policy_output + return PolicyStatus.POLICY_SUCCESS.value, policy_output
144-160: Remove unreachable None checks from two _get_formatter call sites.Since
_get_formatter()now raisesValueErrorinstead of returningNone, the defensive checks at both call sites are dead code:
- src/scanoss/inspection/policy_check/policy_check.py:209 – Remove
if formatter is None:block- src/scanoss/inspection/policy_check/dependency_track/project_violation.py:468 – Remove
if formatter is None:blockEither let the exception propagate or wrap calls in try-except ValueError blocks to handle invalid format gracefully.
src/scanoss/cli.py (1)
1276-1279: Fix incorrect membership check on subparser argument to use tuple membership instead of substring matching.Line 1276 uses
args.subparser in 'inspect', which performs a substring test rather than checking if the value is a member of valid subparser names. While this currently works for the defined aliases ('inspect', 'insp', 'ins') since they are all substrings of 'inspect', it is semantically incorrect and fragile. Change to tuple membership check:elif ( - (args.subparser in 'inspect') + (args.subparser in ('inspect', 'insp', 'ins')) and (args.subparsercmd in ('raw', 'dt', 'glc', 'gitlab')) and (args.subparser_subcmd is None) ):src/scanoss/inspection/utils/scan_result_processor.py (2)
271-276: Bug: setting version on the wrong object in dependency processing.When dependency.version is missing, you assign to c['version'] instead of dependency['version'] causing KeyError later in _append_component.
- if not version: - self.print_debug(f'WARNING: Result missing version. Setting it to unknown: {c}') - version = 'unknown' - c['version'] = version # Set an 'unknown' version to the current component + if not version: + self.print_debug(f'WARNING: Dependency missing version. Setting it to unknown: {dependency}') + version = 'unknown' + dependency['version'] = version # Set 'unknown' on the dependency itself
389-399: Incorrect membership check when de-duplicating component-license pairs.You compute key = f'{purl}-{spdxid}' but check if spdxid not in component_licenses. This can overwrite entries and defeats dedupe intent.
- spdxid = lic.get('spdxid', 'unknown') - if spdxid not in component_licenses: - key = f'{purl}-{spdxid}' + spdxid = lic.get('spdxid', 'unknown') + key = f'{purl}-{spdxid}' + if key not in component_licenses: component_licenses[key] = { 'purl': purl, 'spdxid': spdxid, 'status': status, 'copyleft': lic['copyleft'], 'url': lic['url'], }src/scanoss/inspection/policy_check/dependency_track/project_violation.py (2)
199-211: Jira vs Markdown formatting inconsistencies (headers and links).empty results use Jira-style “h3.” header; non-empty uses “###”. Link in details always Markdown “here” even for Jira. Standardize based on output flavor.
- def _markdown(self, project_violations: list[PolicyViolationDict]) -> PolicyOutput: + def _markdown(self, project_violations: list[PolicyViolationDict]) -> PolicyOutput: @@ - return self._md_summary_generator(project_violations, generate_table) + return self._md_summary_generator( + project_violations, + table_generator=generate_table, + heading='###', + link_formatter=lambda u: f'[here]({u})', + ) @@ - def _jira_markdown(self, data: list[PolicyViolationDict]) -> PolicyOutput: + def _jira_markdown(self, data: list[PolicyViolationDict]) -> PolicyOutput: @@ - return self._md_summary_generator(data, generate_jira_table) + return self._md_summary_generator( + data, + table_generator=generate_jira_table, + heading='h3.', + link_formatter=lambda u: f'[here|{u}]', + ) @@ - def _md_summary_generator(self, project_violations: list[PolicyViolationDict], table_generator) -> PolicyOutput: + def _md_summary_generator( + self, + project_violations: list[PolicyViolationDict], + table_generator, + heading: str = '###', + link_formatter=lambda u: f'[here]({u})', + ) -> PolicyOutput: @@ - if project_violations is None: + if project_violations is None: self.print_stderr('Warning: No project violations found. Returning empty results.') - return PolicyOutput( - details= "h3. Dependency Track Project Violations\n\nNo policy violations found.\n", - summary= "0 policy violations were found.\n", - ) + return PolicyOutput( + details= f"{heading} Dependency Track Project Violations\n\nNo policy violations found.\n", + summary= "0 policy violations were found.\n", + ) @@ - return PolicyOutput( - details= f'### Dependency Track Project Violations\n{table_generator(headers, rows, c_cols)}\n\n' - f'View project in Dependency Track [here]({self.url}/projects/{self.project_id}).\n', - summary= f'{len(project_violations)} policy violations were found.\n' - ) + return PolicyOutput( + details= ( + f'{heading} Dependency Track Project Violations\n' + f'{table_generator(headers, rows, c_cols)}\n\n' + f'View project in Dependency Track {link_formatter(f"{self.url}/projects/{self.project_id}")}.\n' + ), + summary= f'{len(project_violations)} policy violations were found.\n' + )Also applies to: 379-431
432-479: Fix return type contract violation in DependencyTrackProjectViolationPolicyCheck.run().The
run()method violates the basePolicyCheck.run()contract by returningintinstead oftuple[int, PolicyOutput]. Currently at lines 432–478, all five return statements return onlyPolicyStatusenum values, breaking polymorphic callers and type safety.- def run(self) -> int: + def run(self) -> tuple[int, PolicyOutput]: @@ - if not dt_project: - return PolicyStatus.ERROR.value + if not dt_project: + return PolicyStatus.ERROR.value, PolicyOutput( + details="{}\n", + summary="Error: Project processing failed or timed out.\n", + ) @@ - if formatter is None: - self.print_stderr('Error: Invalid format specified.') - return PolicyStatus.ERROR.value + if formatter is None: + self.print_stderr('Error: Invalid format specified.') + return PolicyStatus.ERROR.value, PolicyOutput(details="{}\n", summary="Error: Invalid format specified.\n") @@ - if len(dt_project_violations) > 0: - return PolicyStatus.POLICY_FAIL.value - return PolicyStatus.POLICY_SUCCESS.value + if len(dt_project_violations) > 0: + return PolicyStatus.POLICY_FAIL.value, policy_output + return PolicyStatus.POLICY_SUCCESS.value, policy_outputsrc/scanoss/inspection/policy_check/scanoss/undeclared_component.py (1)
143-144: Accidental literal “\n” in Jira summary.This renders a backslash-n instead of a newline.
- return f'{len(components)} undeclared component(s) were found.\\n' + return f'{len(components)} undeclared component(s) were found.\n'
♻️ Duplicate comments (3)
Makefile (1)
53-58: Guard against “no Python files changed.”Current pipeline can invoke ruff with no args. Add an explicit guard. This was flagged previously; re-surfacing with a ready-to-apply fix.
-linter: ## Run ruff linter with docker - docker run --rm -v $(PWD):/src -w /src ghcr.io/astral-sh/ruff:0.14.2 check $$(git diff --name-only --diff-filter=ACMR | grep '\.py$$' | xargs) + .PHONY: linter fix-linter +linter: ## Run ruff linter with docker + @files=$$(git diff --name-only --diff-filter=ACMR | grep '\.py$$' || true); \ + if [ -n "$$files" ]; then \ + docker run --rm -v "$(PWD)":/src -w /src ghcr.io/astral-sh/ruff:0.14.2 check $$files; \ + else \ + echo "No Python files to lint"; \ + fi @@ -fix-linter: ## Run ruff linter with docker - docker run --rm -v $(PWD):/src -w /src ghcr.io/astral-sh/ruff:0.14.2 check $$(git diff --name-only --diff-filter=ACMR | grep '\.py$$' | xargs) --fix +fix-linter: ## Run ruff linter with docker + @files=$$(git diff --name-only --diff-filter=ACMR | grep '\.py$$' || true); \ + if [ -n "$$files" ]; then \ + docker run --rm -v "$(PWD)":/src -w /src ghcr.io/astral-sh/ruff:0.14.2 check $$files --fix; \ + else \ + echo "No Python files to lint"; \ + fisrc/scanoss/inspection/summary/component_summary.py (1)
149-156: Prefer graceful empty result over raising on missing results.Raising aborts the flow; other modules handle “no results” by proceeding with empty summaries. Recommend returning an empty list and logging instead.
- if self.results_processor.get_results() is None: - raise ValueError(f'Error: No results found in {self.filepath}') + if self.results_processor.get_results() is None: + self.print_debug(f'Warning: No results found in {self.filepath}') + return []src/scanoss/inspection/policy_check/scanoss/undeclared_component.py (1)
296-304: Return PolicyOutput on error paths for consistency with PolicyCheck.Current returns
(status, {})violate the annotated contract and break consumers expecting PolicyOutput.- if components is None: - return PolicyStatus.ERROR.value, {} + if components is None: + return PolicyStatus.ERROR.value, PolicyOutput( + details='{}\n', + summary='Error: No scan results to process.\n', + ) @@ - if undeclared_components is None: - return PolicyStatus.ERROR.value, {} + if undeclared_components is None: + return PolicyStatus.ERROR.value, PolicyOutput( + details='{}\n', + summary='Error: Component extraction failed.\n', + )
🧹 Nitpick comments (12)
Makefile (1)
53-58: Handle filenames with spaces/newlines (null-delimited).To be robust, prefer -z/-0 to preserve filenames safely in CI.
-linter: ## Run ruff linter with docker - @files=$$(git diff --name-only --diff-filter=ACMR | grep '\.py$$' || true); \ - if [ -n "$$files" ]; then \ - docker run --rm -v "$(PWD)":/src -w /src ghcr.io/astral-sh/ruff:0.14.2 check $$files; \ +linter: ## Run ruff linter with docker + @git diff --name-only --diff-filter=ACMR -z | grep -z '\.py$$' | \ + xargs -0 sh -c 'set -e; if [ "$$#" -gt 0 ]; then docker run --rm -v "$(PWD)":/src -w /src ghcr.io/astral-sh/ruff:0.14.2 check "$$@"; else echo "No Python files to lint"; fi' -- @@ -fix-linter: ## Run ruff linter with docker - @files=$$(git diff --name-only --diff-filter=ACMR | grep '\.py$$' || true); \ - if [ -n "$$files" ]; then \ - docker run --rm -v "$(PWD)":/src -w /src ghcr.io/astral-sh/ruff:0.14.2 check $$files --fix; \ +fix-linter: ## Run ruff linter with docker + @git diff --name-only --diff-filter=ACMR -z | grep -z '\.py$$' | \ + xargs -0 sh -c 'set -e; if [ "$$#" -gt 0 ]; then docker run --rm -v "$(PWD)":/src -w /src ghcr.io/astral-sh/ruff:0.14.2 check "$$@" --fix; else echo "No Python files to lint"; fi' --src/scanoss/inspection/policy_check/policy_check.py (5)
86-101: Updaterundocstring to reflectPolicyOutput(details, summary).Doc still references a generic “tuple containing the policy results.”
- :return: A named tuple containing two elements: - - First element: PolicyStatus enum value (SUCCESS, FAIL, or ERROR) - - Second element: PolicyOutput A tuple containing the policy results. + :return: Tuple[int, PolicyOutput] + - First: PolicyStatus value (SUCCESS, FAIL, ERROR) + - Second: PolicyOutput(details: str, summary: str)
105-116: Fix_jsondocstring: references markdown and dict keys.Return type is
PolicyOutput, not a dict; remove stale text.- Format the policy checks results as JSON. - This method should be implemented by subclasses to create a Markdown representation - of the policy check results. + Format policy check results as JSON. @@ - :return: A dictionary containing two keys: - - 'results': A JSON-formatted string with the full list of components - - 'summary': A string summarizing the number of components found + :return: PolicyOutput(details: JSON string, summary: str)
119-129: Fix_markdowndocstring toPolicyOutput.- :return: A dictionary representing the Markdown output. + :return: PolicyOutput(details: Markdown string, summary: str)
132-142: Fix_jira_markdowndocstring toPolicyOutput.- :return: A dictionary representing the Markdown output. + :return: PolicyOutput(details: Jira-Markdown string, summary: str)
63-68: Minor: class/doc comments drift.Mentions “InspectBase” and only 'md/json' formats; class actually inherits
ScanossBaseand supportsjira_md.- InspectBase: A base class providing common functionality for SCANOSS-related operations. + ScanossBase: A base class providing common functionality for SCANOSS-related operations. @@ - VALID_FORMATS (set): A set of valid output formats ('md', 'json'). + VALID_FORMATS (set): Valid output formats ('json', 'md', 'jira_md').tests/test_policy_inspect.py (2)
59-62: Assert explicit status codes instead of truthiness.Use PolicyStatus constants to assert ERROR on empty path for clarity.
- copyleft = Copyleft(filepath='', format_type='json') - success, results = copyleft.run() - self.assertTrue(success, 2) + copyleft = Copyleft(filepath='', format_type='json') + status, _ = copyleft.run() + self.assertEqual(status, PolicyStatus.ERROR.value) @@ - undeclared = UndeclaredComponent(filepath='', format_type='json') - success, results = undeclared.run() - self.assertTrue(success, 2) + undeclared = UndeclaredComponent(filepath='', format_type='json') + status, _ = undeclared.run() + self.assertEqual(status, PolicyStatus.ERROR.value)Also applies to: 171-175
417-417: Remove stray print to keep test output clean.
print(component_summary)is noisy in CI.- print(component_summary)src/scanoss/inspection/summary/license_summary.py (2)
169-176: Handle missing results gracefully instead of raising.Returning an empty list allows summaries to render with zero counts and avoids hard failures in callers.
- if self.results_processor.get_results() is None: - raise ValueError(f'Error: No results found in {self.filepath}') + if self.results_processor.get_results() is None: + self.print_debug(f'Warning: No results found in {self.filepath}') + return []
42-44: Unused constant.REQUIRED_LICENSE_FIELDS is defined but not used. Either enforce these fields during build-time checks or remove to avoid confusion.
src/scanoss/inspection/policy_check/scanoss/undeclared_component.py (1)
96-106: Non-destructive cleanup of component fields.Use pop with default instead of del to avoid KeyError if shapes vary.
- # Remove unused keys - del component['count'] - del component['declared'] - del component['undeclared'] - for lic in component['licenses']: - lic.pop('count', None) # None is default value if key doesn't exist - lic.pop('source', None) # None is default value if key doesn't exist + # Remove unused keys safely + component.pop('count', None) + component.pop('declared', None) + component.pop('undeclared', None) + for lic in component['licenses']: + lic.pop('count', None) + lic.pop('source', None)src/scanoss/inspection/policy_check/scanoss/copyleft.py (1)
81-95: Consider removing redundant assignments.Lines 86-87 reassign
self.outputandself.statusafter they've already been set bysuper().__init__()on line 81-82. While not incorrect, these assignments are redundant.Apply this diff to remove the redundant assignments:
super().__init__( debug, trace, quiet, format_type, status, name='Copyleft Policy', output=output ) self.license_util.init(include, exclude, explicit) self.filepath = filepath - self.output = output - self.status = status self.results_processor = ScanResultProcessor(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
CHANGELOG.md(2 hunks)Makefile(1 hunks)src/scanoss/__init__.py(1 hunks)src/scanoss/cli.py(1 hunks)src/scanoss/inspection/policy_check/dependency_track/project_violation.py(9 hunks)src/scanoss/inspection/policy_check/policy_check.py(8 hunks)src/scanoss/inspection/policy_check/scanoss/copyleft.py(8 hunks)src/scanoss/inspection/policy_check/scanoss/undeclared_component.py(8 hunks)src/scanoss/inspection/summary/component_summary.py(3 hunks)src/scanoss/inspection/summary/license_summary.py(4 hunks)src/scanoss/inspection/utils/scan_result_processor.py(9 hunks)tests/test_policy_inspect.py(21 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/scanoss/init.py
🧰 Additional context used
🧬 Code graph analysis (9)
src/scanoss/inspection/policy_check/policy_check.py (5)
src/scanoss/scanossbase.py (3)
ScanossBase(28-107)print_to_file_or_stdout(83-94)print_to_file_or_stderr(96-107)src/scanoss/inspection/utils/license_utils.py (1)
LicenseUtil(52-132)src/scanoss/inspection/policy_check/dependency_track/project_violation.py (4)
run(432-478)_json(174-187)_markdown(189-199)_jira_markdown(201-211)src/scanoss/inspection/policy_check/scanoss/copyleft.py (4)
run(214-234)_json(97-112)_markdown(114-121)_jira_markdown(123-130)src/scanoss/inspection/policy_check/scanoss/undeclared_component.py (4)
run(284-306)_json(167-182)_markdown(184-200)_jira_markdown(202-218)
src/scanoss/inspection/summary/license_summary.py (2)
src/scanoss/scanossbase.py (1)
ScanossBase(28-107)src/scanoss/inspection/utils/scan_result_processor.py (6)
ScanResultProcessor(53-401)group_components_by_license(352-401)get_results(81-82)get_components_data(195-246)get_dependencies_data(248-283)convert_components_to_list(298-310)
src/scanoss/inspection/summary/component_summary.py (2)
src/scanoss/scanossbase.py (1)
ScanossBase(28-107)src/scanoss/inspection/utils/scan_result_processor.py (5)
ScanResultProcessor(53-401)group_components_by_license(352-401)get_results(81-82)get_components_data(195-246)convert_components_to_list(298-310)
src/scanoss/inspection/utils/scan_result_processor.py (3)
src/scanoss/scanossbase.py (1)
ScanossBase(28-107)src/scanoss/inspection/utils/license_utils.py (2)
LicenseUtil(52-132)init(68-105)src/scanoss/cli.py (2)
results(2577-2614)print_stderr(94-98)
tests/test_policy_inspect.py (6)
src/scanoss/inspection/policy_check/dependency_track/project_violation.py (2)
DependencyTrackProjectViolationPolicyCheck(122-478)run(432-478)src/scanoss/inspection/policy_check/policy_check.py (2)
PolicyStatus(33-44)run(87-102)src/scanoss/inspection/policy_check/scanoss/copyleft.py (2)
Copyleft(48-234)run(214-234)src/scanoss/inspection/policy_check/scanoss/undeclared_component.py (2)
UndeclaredComponent(47-306)run(284-306)src/scanoss/inspection/summary/component_summary.py (2)
ComponentSummary(32-167)run(162-167)src/scanoss/inspection/summary/license_summary.py (2)
LicenseSummary(33-188)run(183-188)
src/scanoss/inspection/policy_check/scanoss/undeclared_component.py (3)
src/scanoss/inspection/policy_check/policy_check.py (6)
PolicyCheck(55-219)PolicyOutput(49-51)PolicyStatus(33-44)_json(105-116)_markdown(119-129)_jira_markdown(132-142)src/scanoss/inspection/utils/markdown_utils.py (2)
generate_jira_table(52-63)generate_table(25-50)src/scanoss/inspection/utils/scan_result_processor.py (5)
ScanResultProcessor(53-401)group_components_by_license(352-401)get_results(81-82)get_components_data(195-246)convert_components_to_list(298-310)
src/scanoss/inspection/policy_check/dependency_track/project_violation.py (3)
src/scanoss/services/dependency_track_service.py (1)
DependencyTrackService(31-132)src/scanoss/inspection/utils/markdown_utils.py (2)
generate_jira_table(52-63)generate_table(25-50)src/scanoss/inspection/policy_check/policy_check.py (6)
PolicyCheck(55-219)PolicyOutput(49-51)PolicyStatus(33-44)_json(105-116)_markdown(119-129)_jira_markdown(132-142)
src/scanoss/inspection/policy_check/scanoss/copyleft.py (4)
src/scanoss/inspection/policy_check/policy_check.py (6)
PolicyCheck(55-219)PolicyOutput(49-51)PolicyStatus(33-44)_json(105-116)_markdown(119-129)_jira_markdown(132-142)src/scanoss/inspection/utils/markdown_utils.py (2)
generate_jira_table(52-63)generate_table(25-50)src/scanoss/inspection/utils/scan_result_processor.py (6)
ScanResultProcessor(53-401)group_components_by_license(352-401)get_results(81-82)get_components_data(195-246)get_dependencies_data(248-283)convert_components_to_list(298-310)src/scanoss/inspection/policy_check/scanoss/undeclared_component.py (4)
Component(41-45)_json(167-182)_markdown(184-200)_jira_markdown(202-218)
src/scanoss/cli.py (6)
src/scanoss/inspection/policy_check/dependency_track/project_violation.py (1)
DependencyTrackProjectViolationPolicyCheck(122-478)src/scanoss/inspection/policy_check/scanoss/copyleft.py (1)
Copyleft(48-234)src/scanoss/inspection/policy_check/scanoss/undeclared_component.py (1)
UndeclaredComponent(47-306)src/scanoss/inspection/summary/component_summary.py (1)
ComponentSummary(32-167)src/scanoss/inspection/summary/license_summary.py (1)
LicenseSummary(33-188)src/scanoss/inspection/summary/match_summary.py (1)
MatchSummary(63-287)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (12)
CHANGELOG.md (2)
12-24: Release date is in the future relative to PR creation.Consider keeping this under “Unreleased” until tag/merge, or update the date on release.
726-727: Compare links look good; ensure they’re updated on tag.No action required; just confirm these auto-update as part of the release process.
src/scanoss/cli.py (1)
72-79: Import path reorg LGTM.New inspection modules resolved under policy_check/summary; no issues spotted.
tests/test_policy_inspect.py (1)
72-77: Migration toPolicyOutputlooks correct.Accessing
policy_output.details/summaryand parsing JSON where needed is consistent with the refactor.Consider adding a small assert on
statusalongside details/summary to harden tests.Also applies to: 87-90, 109-110, 122-139, 150-163, 185-209, 220-253, 264-300, 311-338, 345-375, 382-390, 441-473, 501-508, 523-527, 539-543, 553-558, 602-606
src/scanoss/inspection/policy_check/scanoss/copyleft.py (8)
27-31: LGTM! Imports reflect the refactored module structure.The imports correctly reference the new module paths and include all necessary types (
PolicyCheck,PolicyOutput,PolicyStatus) and utilities (ScanResultProcessor, markdown generators).
48-48: LGTM! Correct use of generic PolicyCheck base class.
97-112: LGTM! Correctly returns PolicyOutput.The method properly uses
self.results_processor.group_components_by_license()and returns aPolicyOutputnamed tuple with JSON-formatted details and summary.
114-130: LGTM! Clean delegation pattern.Both methods correctly delegate to
_md_summary_generatorwith appropriate table generators, maintaining DRY principles.
132-167: LGTM! Correctly implements PolicyOutput return.The method properly uses
self.results_processor.group_components_by_license()and returns a well-structuredPolicyOutputwith markdown-formatted details and summary.
195-212: LGTM! Clean delegation to ScanResultProcessor.The method correctly delegates component extraction and processing to the
ScanResultProcessorutility, following the refactored architecture.
214-234: LGTM! Correct orchestration and error handling.The method properly orchestrates the copyleft license inspection workflow and handles error cases by returning
PolicyStatus.ERROR.valuewhen components cannot be retrieved.
169-193: The review comment is incorrect and should be dismissed.The keys
count,declared, andundeclaredare guaranteed to exist in components returned by_get_components(). Every component is created through_append_component()which initializes all three keys (lines 130-135 in scan_result_processor.py), and bothget_components_data()andget_dependencies_data()use this method. Theconvert_components_to_list()method preserves these keys—it only converts the licenses dictionary to a list. Therefore, thedelstatements on lines 181-183 are safe and will not raise aKeyError.Likely an incorrect or invalid review comment.
agustingroh
force-pushed
the
chore/refactor-inspect-module
branch
from
e6c5aa2 to
9494df2
Compare
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/scanoss/inspection/policy_check/scanoss/undeclared_component.py (1)
284-306: Error paths should return PolicyOutput for type consistency.Lines 299 and 303 return
(PolicyStatus.ERROR.value, {}), which violates the type contracttuple[int, PolicyOutput]. To maintain consistency with the abstract base class signature and other formatter methods, wrap error responses in PolicyOutput.Apply this diff to fix the type contract violations:
components = self._get_components() if components is None: - return PolicyStatus.ERROR.value, {} + return PolicyStatus.ERROR.value, PolicyOutput(details="{}\n", summary="Error: No components found.\n") # Get an undeclared component summary (if any) undeclared_components = self._get_undeclared_components(components) if undeclared_components is None: - return PolicyStatus.ERROR.value, {} + return PolicyStatus.ERROR.value, PolicyOutput(details="{}\n", summary="Error: Failed to process components.\n")Note: This issue was already flagged in a previous review at lines 167-183.
🧹 Nitpick comments (3)
src/scanoss/inspection/policy_check/dependency_track/project_violation.py (1)
360-377: Note: _sort_project_violations changed from staticmethod to instance method.The method signature changed from
@staticmethodto an instance method by addingself. While this doesn't introduce issues (the method doesn't useself), consider whether this change was intentional or if the@staticmethoddecorator should be retained for clarity.If the method truly doesn't need instance state, restore the decorator:
+ @staticmethod - def _sort_project_violations(self,violations: List[PolicyViolationDict]) -> List[PolicyViolationDict]: + def _sort_project_violations(violations: List[PolicyViolationDict]) -> List[PolicyViolationDict]: """ Sort project violations by priority.And update the call site at line 472:
- policy_output = formatter(self._sort_project_violations(dt_project_violations)) + policy_output = formatter(DependencyTrackProjectViolationPolicyCheck._sort_project_violations(dt_project_violations))src/scanoss/inspection/summary/license_summary.py (1)
42-43: Consider using or removing REQUIRED_LICENSE_FIELDS constant.The
REQUIRED_LICENSE_FIELDSconstant is defined but doesn't appear to be used in the visible code. If it's intended for validation or documentation, consider adding validation logic; otherwise, remove it to reduce clutter.src/scanoss/inspection/policy_check/scanoss/copyleft.py (1)
86-87: Remove redundant attribute assignments.These assignments are unnecessary since
super().__init__()already setsself.outputandself.status(see PolicyCheck base class constructor). The parent class assigns these from the parameters passed on line 82.Apply this diff to remove the redundant lines:
self.license_util.init(include, exclude, explicit) self.filepath = filepath - self.output = output - self.status = status self.results_processor = ScanResultProcessor(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
CHANGELOG.md(2 hunks)Makefile(1 hunks)src/scanoss/__init__.py(1 hunks)src/scanoss/cli.py(1 hunks)src/scanoss/inspection/policy_check/dependency_track/project_violation.py(9 hunks)src/scanoss/inspection/policy_check/policy_check.py(8 hunks)src/scanoss/inspection/policy_check/scanoss/copyleft.py(8 hunks)src/scanoss/inspection/policy_check/scanoss/undeclared_component.py(8 hunks)src/scanoss/inspection/summary/component_summary.py(3 hunks)src/scanoss/inspection/summary/license_summary.py(4 hunks)src/scanoss/inspection/utils/scan_result_processor.py(9 hunks)tests/test_policy_inspect.py(21 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Makefile
- CHANGELOG.md
- src/scanoss/cli.py
🧰 Additional context used
🧬 Code graph analysis (8)
src/scanoss/inspection/summary/component_summary.py (2)
src/scanoss/scanossbase.py (1)
ScanossBase(28-107)src/scanoss/inspection/utils/scan_result_processor.py (5)
ScanResultProcessor(53-401)group_components_by_license(352-401)get_results(81-82)get_components_data(195-246)convert_components_to_list(298-310)
src/scanoss/inspection/policy_check/scanoss/undeclared_component.py (4)
src/scanoss/inspection/policy_check/policy_check.py (6)
PolicyCheck(55-219)PolicyOutput(49-51)PolicyStatus(33-44)_json(105-116)_markdown(119-129)_jira_markdown(132-142)src/scanoss/inspection/utils/markdown_utils.py (2)
generate_jira_table(52-63)generate_table(25-50)src/scanoss/inspection/utils/scan_result_processor.py (5)
ScanResultProcessor(53-401)group_components_by_license(352-401)get_results(81-82)get_components_data(195-246)convert_components_to_list(298-310)src/scanoss/inspection/policy_check/scanoss/copyleft.py (4)
Component(42-46)_json(97-112)_markdown(114-121)_jira_markdown(123-130)
src/scanoss/inspection/policy_check/dependency_track/project_violation.py (3)
src/scanoss/services/dependency_track_service.py (1)
DependencyTrackService(31-132)src/scanoss/inspection/utils/markdown_utils.py (2)
generate_jira_table(52-63)generate_table(25-50)src/scanoss/inspection/policy_check/policy_check.py (5)
PolicyOutput(49-51)PolicyStatus(33-44)_json(105-116)_markdown(119-129)_jira_markdown(132-142)
src/scanoss/inspection/policy_check/scanoss/copyleft.py (2)
src/scanoss/inspection/policy_check/policy_check.py (6)
PolicyCheck(55-219)PolicyOutput(49-51)PolicyStatus(33-44)_json(105-116)_markdown(119-129)_jira_markdown(132-142)src/scanoss/inspection/utils/scan_result_processor.py (6)
ScanResultProcessor(53-401)group_components_by_license(352-401)get_results(81-82)get_components_data(195-246)get_dependencies_data(248-283)convert_components_to_list(298-310)
tests/test_policy_inspect.py (4)
src/scanoss/inspection/policy_check/dependency_track/project_violation.py (2)
DependencyTrackProjectViolationPolicyCheck(122-478)run(432-478)src/scanoss/inspection/policy_check/policy_check.py (2)
PolicyStatus(33-44)run(87-102)src/scanoss/inspection/policy_check/scanoss/copyleft.py (2)
Copyleft(48-234)run(214-234)src/scanoss/inspection/policy_check/scanoss/undeclared_component.py (2)
UndeclaredComponent(47-306)run(284-306)
src/scanoss/inspection/summary/license_summary.py (2)
src/scanoss/scanossbase.py (1)
ScanossBase(28-107)src/scanoss/inspection/utils/scan_result_processor.py (6)
ScanResultProcessor(53-401)group_components_by_license(352-401)get_results(81-82)get_components_data(195-246)get_dependencies_data(248-283)convert_components_to_list(298-310)
src/scanoss/inspection/policy_check/policy_check.py (4)
src/scanoss/scanossbase.py (3)
ScanossBase(28-107)print_to_file_or_stdout(83-94)print_to_file_or_stderr(96-107)src/scanoss/inspection/policy_check/dependency_track/project_violation.py (4)
run(432-478)_json(174-187)_markdown(189-199)_jira_markdown(201-211)src/scanoss/inspection/policy_check/scanoss/copyleft.py (4)
run(214-234)_json(97-112)_markdown(114-121)_jira_markdown(123-130)src/scanoss/inspection/policy_check/scanoss/undeclared_component.py (4)
run(284-306)_json(167-182)_markdown(184-200)_jira_markdown(202-218)
src/scanoss/inspection/utils/scan_result_processor.py (3)
src/scanoss/scanossbase.py (1)
ScanossBase(28-107)src/scanoss/inspection/utils/license_utils.py (2)
LicenseUtil(52-132)init(68-105)src/scanoss/cli.py (2)
results(2577-2614)print_stderr(94-98)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (18)
src/scanoss/__init__.py (1)
25-25: LGTM: Version bump aligns with the refactor.The version increment to 1.39.1 is appropriate for this refactor of the inspect module.
src/scanoss/inspection/utils/scan_result_processor.py (3)
53-83: LGTM: ScanResultProcessor initialization is well-structured.The constructor properly initializes the processor with file path and license configuration parameters, and preloads results via
_load_input_file().
195-246: LGTM: Component data extraction is thorough.The method properly handles missing fields, filters by component type, and provides helpful debug warnings for malformed data.
28-30: Import is already present and verified.The import statement
from ..utils.file_utils import load_json_fileis already present on line 29, as confirmed by the file. This import is also visible in the code snippet provided in the review comment itself (second line of the imports section). The function call on line 293 is properly supported by this import.Likely an incorrect or invalid review comment.
tests/test_policy_inspect.py (2)
31-38: LGTM: Test imports updated to new module structure.The imports correctly reference the reorganized inspection modules under
policy_check.*andsummary.*.
72-76: LGTM: Tests properly consume PolicyOutput.The test correctly accesses
policy_output.detailsandpolicy_output.summaryinstead of dictionary keys, aligning with the new API.src/scanoss/inspection/summary/component_summary.py (2)
32-56: LGTM: Class successfully migrated to processor-based flow.The ComponentSummary now properly inherits from ScanossBase and delegates data processing to ScanResultProcessor. The docstring correctly identifies the class being initialized.
149-155: LGTM: Error handling and data extraction properly updated.The method now correctly checks
results_processor.get_results()and uses the processor's data extraction methods. The f-string in the ValueError correctly interpolatesself.filepath.src/scanoss/inspection/policy_check/policy_check.py (3)
49-51: LGTM: PolicyOutput provides type-safe structured output.The introduction of PolicyOutput as a NamedTuple ensures consistent structure across all policy check outputs with
detailsandsummaryfields.
55-102: LGTM: PolicyCheck is now explicitly abstract.Making PolicyCheck inherit from ABC and marking methods as abstract improves type safety and enforces implementation in subclasses. The updated return type
tuple[int, PolicyOutput]is clear and consistent.
144-159: LGTM: Improved error handling in formatter selection.Raising
ValueErroron invalid format is clearer than returning None, and the return type annotation correctly specifiesCallable[[List[dict]], PolicyOutput].src/scanoss/inspection/policy_check/dependency_track/project_violation.py (1)
174-187: LGTM: JSON formatter returns PolicyOutput.The method correctly constructs and returns a PolicyOutput with JSON-serialized details and a summary string.
src/scanoss/inspection/summary/license_summary.py (2)
45-77: LGTM: LicenseSummary successfully migrated to processor-based flow.The constructor properly initializes the ScanResultProcessor with all license configuration parameters (include, exclude, explicit), and the class correctly delegates data processing to the processor.
169-176: LGTM: Error handling and processor delegation work correctly.The method properly checks
results_processor.get_results()and the f-string correctly interpolatesself.filepathwithout the extraneous$.src/scanoss/inspection/policy_check/scanoss/undeclared_component.py (2)
76-84: LGTM: Constructor properly initializes processor.The constructor correctly calls
super().__init__with named arguments and initializesresults_processorwith the appropriate parameters.
167-182: LGTM: JSON formatter returns PolicyOutput.The method correctly uses
results_processor.group_components_by_licenseand returns a properly structured PolicyOutput.src/scanoss/inspection/policy_check/scanoss/copyleft.py (2)
97-167: LGTM! Clean migration to PolicyOutput.The formatter methods (
_json,_markdown,_jira_markdown,_md_summary_generator) correctly returnPolicyOutputnamed tuples and delegate data processing toself.results_processor.group_components_by_license(), aligning with the refactored architecture.
195-212: LGTM! Proper delegation to ScanResultProcessor.The component extraction logic correctly delegates to
results_processormethods (get_components_data,get_dependencies_data,convert_components_to_list), maintaining clean separation of concerns.
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/scanoss/inspection/summary/match_summary.py (2)
187-220: Fix line length violation on line 219.Line 219 exceeds the project's 120-character limit (156 characters), causing a linter failure.
Apply this diff to fix the line length:
- self.print_debug(f"Match summary complete: {len(gitlab_matches_summary.files)} file matches, {len(gitlab_matches_summary.snippet)} snippet matches") + self.print_debug( + f"Match summary complete: {len(gitlab_matches_summary.files)} file matches, " + f"{len(gitlab_matches_summary.snippet)} snippet matches" + )
234-293: Fix line length violation on line 240.Line 240 exceeds the project's 120-character limit (146 characters), causing a linter failure.
Apply this diff to fix the line length:
- self.print_trace(f"Formatting {len(gitlab_matches_summary.files)} file matches and {len(gitlab_matches_summary.snippet)} snippet matches") + self.print_trace( + f"Formatting {len(gitlab_matches_summary.files)} file matches and " + f"{len(gitlab_matches_summary.snippet)} snippet matches" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/scanoss/gitlabqualityreport.py(5 hunks)src/scanoss/inspection/summary/match_summary.py(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/scanoss/inspection/summary/match_summary.py (2)
src/scanoss/scanossbase.py (3)
print_trace(65-70)print_stdout(73-81)print_to_file_or_stdout(83-94)src/scanoss/utils/scanoss_scan_results_utils.py (1)
get_lines(25-41)
src/scanoss/gitlabqualityreport.py (3)
src/scanoss/scanossbase.py (1)
print_trace(65-70)src/scanoss/utils/scanoss_scan_results_utils.py (1)
get_lines(25-41)src/scanoss/cli.py (2)
print_stderr(94-98)results(2577-2614)
🪛 GitHub Actions: Lint
src/scanoss/inspection/summary/match_summary.py
[error] 219-219: E501: Line too long (156 > 120).
[error] 240-240: E501: Line too long (146 > 120).
[error] 307-307: E501: Line too long (154 > 120).
src/scanoss/gitlabqualityreport.py
[error] 144-144: F541: f-string without any placeholders. Remove extraneous f prefix.
[error] 187-187: F541: f-string without any placeholders. Remove extraneous f prefix.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (9)
src/scanoss/gitlabqualityreport.py (6)
77-77: Useful initialization logging.The trace logging of initialization parameters aids debugging and follows the established pattern from the parent class.
81-130: Enhanced observability for code quality extraction.The trace and debug logging additions provide useful diagnostic information. The local variable assignments before returns enable trace logging without changing behavior.
134-142: Good logging coverage for output operations.The trace and debug logging effectively tracks the output writing process and destination.
151-169: Comprehensive logging for JSON processing.The logging additions effectively track the processing flow and provide useful diagnostic information, including enhanced context for skipped results at line 161.
180-180: Effective logging for string parsing.The trace and debug statements appropriately track the JSON parsing process and provide useful metrics.
Also applies to: 184-184, 188-188
202-213: Well-structured file processing with logging.The logging additions track the file reading process effectively. Reading the file content into a variable (lines 212-213) improves code clarity.
src/scanoss/inspection/summary/match_summary.py (3)
97-97: LGTM!The initialization logging helps track object instantiation for debugging purposes.
112-134: LGTM!The trace logging appropriately differentiates between snippet and file matches, providing valuable execution details without altering the functional logic.
310-329: LGTM!The logging throughout the
runmethod provides excellent workflow visibility, tracking all major stages from loading to output generation.
agustingroh
force-pushed
the
chore/refactor-inspect-module
branch
from
aee5cd8 to
4f8fe31
Compare
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tools/linter.sh (1)
44-50: Quote variables to prevent word-splitting issues.Unquoted variable expansion in lines 46 and 49 can cause unexpected behavior if filenames or flags contain special characters. While
$FIX_FLAGis safe (only--fixor empty) and git-diff output typically avoids spaces, defensive quoting is best practice.# Run linter if [ "$USE_DOCKER" = true ]; then # Run with Docker - docker run --rm -v "$(pwd)":/src -w /src ghcr.io/astral-sh/ruff:0.14.2 check $files $FIX_FLAG + docker run --rm -v "$(pwd)":/src -w /src ghcr.io/astral-sh/ruff:0.14.2 check $files $FIX_FLAG else # Run locally - python3 -m ruff check $files $FIX_FLAG + python3 -m ruff check $files $FIX_FLAG fiAlternatively, if splitting arguments is preferred for robustness:
- docker run --rm -v "$(pwd)":/src -w /src ghcr.io/astral-sh/ruff:0.14.2 check $files $FIX_FLAG + docker run --rm -v "$(pwd)":/src -w /src ghcr.io/astral-sh/ruff:0.14.2 check ${FIX_FLAG:+$FIX_FLAG} $filesThis way,
$FIX_FLAGis only expanded if non-empty, preserving intended behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.md(2 hunks)Makefile(1 hunks)src/scanoss/__init__.py(1 hunks)src/scanoss/gitlabqualityreport.py(5 hunks)src/scanoss/inspection/summary/match_summary.py(9 hunks)tools/linter.sh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/scanoss/init.py
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- src/scanoss/gitlabqualityreport.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/scanoss/inspection/summary/match_summary.py (2)
src/scanoss/scanossbase.py (3)
print_trace(65-70)print_stdout(73-81)print_to_file_or_stdout(83-94)src/scanoss/utils/scanoss_scan_results_utils.py (1)
get_lines(25-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
Makefile (1)
53-63: Linting targets are well-structured and address prior feedback.The new targets correctly split docker/local variants and delegate cleanly to the shell script. This directly addresses the previous reviewer's request for
linter/linter-fix(local) alongsidelinter-docker/linter-docker-fix(docker), and the empty-file handling is now in the script.src/scanoss/inspection/summary/match_summary.py (2)
313-316: Line length issue resolved.The previously flagged line length violation (154 characters on old line 307) has been correctly fixed by splitting the log statement across multiple lines.
97-338: LGTM! Logging additions improve observability.The trace and debug logging additions throughout the class provide valuable observability without modifying any functional behavior. The logging follows a consistent pattern:
print_debugfor high-level operations andprint_tracefor detailed step-by-step execution tracking.
agustingroh
force-pushed
the
chore/refactor-inspect-module
branch
2 times, most recently
from
de0092d to
1b22cdc
Compare
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Makefile(2 hunks)src/scanoss/gitlabqualityreport.py(5 hunks)src/scanoss/inspection/summary/match_summary.py(9 hunks)tools/linter.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Makefile
- src/scanoss/inspection/summary/match_summary.py
- src/scanoss/gitlabqualityreport.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
agustingroh
force-pushed
the
chore/refactor-inspect-module
branch
from
1b22cdc to
e6922bd
Compare
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tools/linter.sh (1)
35-50: Handle file list with array expansion to support filenames with spaces and special characters.This issue was flagged in the previous review and remains unresolved. The variables
${files}are unquoted and will undergo word-splitting if filenames contain spaces, newlines, or special shell characters. For example, a file namedmy file.pywould be incorrectly split into separate arguments.Convert
filesto an array and use proper array expansion syntax:# Get all changed Python files since merge base -files=$(git diff --name-only "$merge_base" HEAD | grep '\.py$' || true) +mapfile -t files_array < <(git diff --name-only "$merge_base" HEAD | grep '\.py$' || true) # Check if there are any Python files changed -if [ -z "$files" ]; then +if [ ${#files_array[@]} -eq 0 ]; then echo "No Python files changed" exit 0 fi # Run linter if [ "$USE_DOCKER" = true ]; then # Run with Docker - docker run --rm -v "$(pwd)":/src -w /src ghcr.io/astral-sh/ruff:0.14.2 check ${files} ${FIX_FLAG} + docker run --rm -v "$(pwd)":/src -w /src ghcr.io/astral-sh/ruff:0.14.2 check "${files_array[@]}" ${FIX_FLAG} else # Run locally - python3 -m ruff check ${files} ${FIX_FLAG} + python3 -m ruff check "${files_array[@]}" ${FIX_FLAG} fi
🧹 Nitpick comments (1)
src/scanoss/inspection/summary/match_summary.py (1)
112-134: Consider consistent spacing around log statements.The empty lines after trace statements (lines 113, 120, 134) add visual separation, but this pattern isn't applied uniformly throughout the file. For consistency, either apply this spacing after all similar log statements or remove these extra lines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Makefile(2 hunks)src/scanoss/gitlabqualityreport.py(5 hunks)src/scanoss/inspection/summary/match_summary.py(9 hunks)tools/linter.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
🧰 Additional context used
🧬 Code graph analysis (2)
src/scanoss/inspection/summary/match_summary.py (2)
src/scanoss/scanossbase.py (3)
print_trace(65-70)print_stdout(73-81)print_to_file_or_stdout(83-94)src/scanoss/utils/scanoss_scan_results_utils.py (1)
get_lines(25-41)
src/scanoss/gitlabqualityreport.py (3)
src/scanoss/scanossbase.py (1)
print_trace(65-70)src/scanoss/utils/scanoss_scan_results_utils.py (1)
get_lines(25-41)src/scanoss/cli.py (2)
print_stderr(94-98)results(2577-2614)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (9)
src/scanoss/inspection/summary/match_summary.py (4)
97-97: LGTM: Clear initialization logging.The debug log on initialization is helpful for tracing object lifecycle.
187-224: LGTM: Comprehensive processing logs.The debug and trace logs effectively track the scan result processing flow, including file counts and match categorization. The multi-line format keeps lines within the length limit.
237-299: LGTM: Well-structured markdown generation logs.The logging appropriately marks each stage of markdown generation (empty check, table building, completion) and maintains line-length compliance with multi-line formatting.
312-338: LGTM: Excellent observability and previous issue resolved.The run method now has comprehensive logging that tracks the entire workflow. The multi-line formatting on lines 313-316 successfully addresses the previous line-length violation flagged in the earlier review (line 307 was 154 characters). The configuration details and execution steps are now clearly logged within the 120-character limit.
src/scanoss/gitlabqualityreport.py (5)
77-82: Excellent observability improvements.The trace and debug logging additions provide valuable insights into the processing flow. The refactoring to use local
code_qualityvariables before returning (lines 91-104, 117-130) makes debugging easier without changing behavior.Minor note: Line 82 logs the entire result dictionary at trace level, which could be large. This is acceptable since trace logging is opt-in for debugging purposes.
Also applies to: 89-104, 110-130
134-148: Output logging looks good; past issue resolved.The trace and debug logging additions help track the output writing process. The past f-string issue on line 144 has been correctly resolved—the f-prefix has been removed.
150-171: Comprehensive processing visibility.The trace and debug logging additions throughout
_produce_from_jsonprovide excellent visibility into the processing pipeline. The enhanced skip message on line 161 now includes file name and result id context, which is helpful for debugging.
173-192: Parsing visibility improved; past issue resolved.The logging additions help track JSON parsing and validation. The past f-string issue on line 187 has been correctly resolved—the f-prefix has been removed.
195-214: File processing improvements look good.The trace and debug logging additions provide clear visibility into file operations. The refactoring to use a local
json_contentvariable (line 212) improves readability without changing behavior.
agustingroh
force-pushed
the
chore/refactor-inspect-module
branch
from
e6922bd to
e6ac2e3
Compare
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
agustingroh
force-pushed
the
chore/refactor-inspect-module
branch
from
e6ac2e3 to
1b45692
Compare
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
| @@ -0,0 +1,50 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
I don't think we need this script
On one hand we're already doing this in the lint workflow
Besides that, if what we want is to run the linter before a commit, we could just add ruff to the pre commit config file
| @./tools/linter.sh | ||
|
|
||
| linter-fix: ## Run ruff linter locally with auto-fix | ||
| @./tools/linter.sh --fix |
There was a problem hiding this comment.
If we use ruff's pre-commit hook we wouldn't need all these make commands
| :return: A tuple containing: | ||
| :return: A named tuple containing two elements: | ||
| - First element: PolicyStatus enum value (SUCCESS, FAIL, or ERROR) |
There was a problem hiding this comment.
Here it says it returns PolicyStatus enum but in line 87 is returning an int
| """ | ||
| Initialize the ComponentSummary class. | ||
| :param debug: Enable debug mode |
There was a problem hiding this comment.
Across all these new/modified files, remember to use Google-style docstring with Args Returns and Raise
| :param result: SCANOSS scan result dictionary containing match details | ||
| :return: Populated match summary item with all relevant information | ||
| """ | ||
| self.print_trace(f"Creating match summary item for file: {file_name}, id: {result.get('id')}") |
There was a problem hiding this comment.
Maybe this and the one below are more debug logs rather than trace?
| - If `self.results` is `None`, simply return `None`. | ||
| """ | ||
| pass | ||
| def get_results(self) -> Dict[str, Any]: |
There was a problem hiding this comment.
In some of the files you're checking if this is equal to None
I would add to the types here that it could also be None
| """ | ||
| try: | ||
| return load_json_file(self.filepath) | ||
| return load_json_file(self.result_file_path) |
There was a problem hiding this comment.
Remember we have also this util src/scanoss/utils/file.py that checks multiple errors while loading and parsing a json file
| ) | ||
| ) | ||
| ) | ||
| self.print_trace(f"Created file CodeQuality object: {code_quality}") |
There was a problem hiding this comment.
Do we need this to be traced?
Summary by CodeRabbit
New Features
Changed
Tests