Conversation
…ing to a common function: get_scanoss_settings_from_args
…ersisting credentials in checkout in version-tag workflow.
…ing to a common function: get_scanoss_settings_from_args
…ersisting credentials in checkout in version-tag workflow.
…gs-file-load' into feat/alex/SP-4167-improve-settings-file-load
…nto feat/alex/hermine-integration
…nto feat/alex/hermine-integration # Conflicts: # CHANGELOG.md # src/scanoss/cli.py # tests/test_scanoss_settings.py
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Hermine API integration for SBOM export and release-violation inspection, including CLI commands, validation, formatting, tests, and release metadata. It also centralizes FileFilters test fixtures and updates related test inheritance. ChangesHermine Integration
File Filtering Test Updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant HermineExporter
participant HermineService
participant HermineAPI
CLI->>HermineExporter: Upload SPDX SBOM
HermineExporter->>HermineService: Resolve product and release IDs
HermineService->>HermineAPI: Request products or create missing entities
HermineExporter->>HermineService: Upload SBOM contents
HermineService->>HermineAPI: POST upload_spdx
HermineAPI-->>HermineExporter: Upload response
sequenceDiagram
participant CLI
participant HermineViolationsPolicyCheck
participant HermineService
participant HermineAPI
CLI->>HermineViolationsPolicyCheck: Run violations inspection
HermineViolationsPolicyCheck->>HermineService: Resolve product and release IDs
HermineViolationsPolicyCheck->>HermineAPI: GET validation_1 through validation_4
HermineAPI-->>HermineViolationsPolicyCheck: Validation results
HermineViolationsPolicyCheck->>HermineAPI: GET validation_5 and validation_6
HermineAPI-->>HermineViolationsPolicyCheck: Violation data
HermineViolationsPolicyCheck-->>CLI: Formatted report and policy status
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 11
🤖 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/scanoss/cli.py`:
- Around line 1082-1083: The Hermine export CLI currently accepts --product-id
and --release-id but export_hermine still only passes args.product_name and
args.release_name into HermineExporter.upload_sbom_file, so ID-only exports
break with missing names. Update export_hermine to either forward the ID-based
values into an exporter path that supports release_id/product_id, or explicitly
reject these flags for the export command; use the existing export_hermine and
HermineExporter.upload_sbom_file symbols to wire the correct contract.
- Around line 900-912: The new Hermine subparser setup in p_hermine_sub and
p_inspect_hermine_sub can now be selected without a nested command, but there is
no guard for that case, so inspect hermine / inspect hm may fall through to
args.func with no handler. Update the missing-subcommand handling in the inspect
command flow to include the Hermine branch, and ensure the parser either rejects
the bare hermine invocation with a clear message or routes it to an explicit
default handler before args.func is accessed.
- Around line 2348-2367: The selector validation in the CLI flow accepts both
complete name and ID pairs, but it should reject conflicting Hermine selector
sets so only one selector mode is used. Update the validation around the args
checks in the `run()` path to enforce either `--product-name/--release-name` or
`--product-id/--release-id`, and emit an error when both pairs are supplied so
`HermineViolationsPolicyCheck.run()` cannot silently prefer IDs over names.
- Around line 1693-1695: The settings/skip conflict check currently lives in the
`scan` command path, but it should be moved into
`get_scanoss_settings_from_args` so all settings-aware callers handle it
consistently. Update that helper to reject the combination of `args.settings`
and `args.skip_settings_file` before any early return, and keep the error
message aligned with the actual flag name by changing it to
`--skip-settings-file`.
In `@src/scanoss/export/hermine.py`:
- Around line 81-99: `_read_and_validate_sbom` in `Hermine` has a
return/docstring mismatch: it is documented as returning parsed SBOM data, but
it currently only validates and returns nothing useful. Either make
`_read_and_validate_sbom` return `result.data` and thread that value through
`upload_sbom_file`/`_build_file_payload` to avoid re-reading the file, or keep
it validation-only and remove the useless return plus fix the docstring to
reflect that behavior.
In `@src/scanoss/inspection/policy_check/hermine/violations.py`:
- Line 208: Several f-strings in violations.py exceed the 120-character limit
and are causing E501 failures; wrap the long message literals used in the
Hermine violations/reporting flow so each string fits within the limit. Update
the affected formatted messages in the relevant functions around the listed
report/exception strings, keeping the existing text and references to
components/licenses intact while splitting long lines into multiple concatenated
parts or adjacent literals.
- Around line 297-300: _spdx_tokens currently splits SPDX expressions but leaves
surrounding parentheses attached to identifiers, which breaks matching later in
_handle_validation_5 and _handle_validation_6. Update the token normalization in
_spdx_tokens so each token is stripped of leading/trailing parentheses in
addition to whitespace and the existing empty/plus filtering, ensuring
expressions like "(MIT AND Apache-2.0)" produce clean license identifiers that
match lic_by_spdx keys.
- Around line 24-38: Remove the unused imports and constants in violations.py to
address the F401 failures: drop the unused names from the import block (time,
datetime, Any, Dict) and delete the unused constants PROCESSING_RETRY_DELAY and
MILLISECONDS_TO_SECONDS. Keep only the symbols that are actually referenced by
the PolicyCheck/HermineService code path so the module stays clean and
lint-safe.
In `@src/scanoss/services/hermine_service.py`:
- Around line 25-27: Import ordering in the module is incorrect and is causing
the I001/isort check to fail. Reorder the imports in the top-level import block
of the file so that import requests appears before from ..scanossbase import
ScanossBase, keeping the import groups consistent with isort conventions.
- Around line 59-66: In get_ids, replace direct indexing of the Hermine API
response with defensive .get() access for the response containers and nested
lists, so missing keys like results or releases do not raise KeyError. Update
the lookups in get_ids to safely handle absent or malformed data by defaulting
to empty iterables or returning False when expected fields are missing, and keep
the existing behavior consistent for upload_sbom_file and run by preventing
uncaught exceptions from escaping get_ids.
- Around line 148-156: All HTTP calls in HermineService.get_hermine_data
currently lack a timeout, so requests can hang indefinitely; update each
requests.get/request.post path to use the service timeout. Also wire the
existing timeout from HermineViolationsPolicyCheck.__init__ through to
HermineService so the user-configured value is actually honored. Use the
get_hermine_data method and the HermineViolationsPolicyCheck constructor to
locate the changes, and ensure any request helper or client setup consistently
applies the timeout parameter.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 627dcde8-8fa0-45e0-8864-83c7f5b08521
📒 Files selected for processing (8)
CHANGELOG.mdsrc/scanoss/cli.pysrc/scanoss/export/hermine.pysrc/scanoss/inspection/policy_check/hermine/__init__.pysrc/scanoss/inspection/policy_check/hermine/violations.pysrc/scanoss/services/hermine_service.pytests/test_hermine.pytests/test_scanoss_settings.py
| try: | ||
| self.print_debug(f'URL: {uri}, Params: {params}, Data: {data}') | ||
| if data: | ||
| response = requests.post(uri, headers=req_headers, data=data, files=files) | ||
| elif params: | ||
| response = requests.get(uri, headers=req_headers, params=params) | ||
| else: | ||
| response = requests.get(uri, headers=req_headers) | ||
| response.raise_for_status() # Raises an HTTPError for bad responses |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add timeout to all HTTP requests to prevent indefinite hangs.
None of the requests.get/requests.post calls in get_hermine_data specify a timeout. If the Hermine server is unresponsive, the CLI will hang indefinitely — problematic in CI/CD pipelines. Additionally, HermineViolationsPolicyCheck accepts a timeout parameter (default 300s) but never passes it to HermineService, so the user-configured timeout is silently ignored. The unused constants PROCESSING_RETRY_DELAY and MILLISECONDS_TO_SECONDS in violations.py suggest polling/retry logic was planned but not implemented.
⏱️ Proposed fix: add timeout to HermineService
class HermineService(ScanossBase):
def __init__(
self,
api_key: str,
url: str,
+ timeout: float = 30.0,
debug: bool = False,
trace: bool = False,
quiet: bool = False,
):
super().__init__(debug=debug, trace=trace, quiet=quiet)
if not url:
raise ValueError("Error: Hermine URL is required")
self.url = url.strip().rstrip('/')
if not api_key:
raise ValueError("Error: Hermine API key is required")
self.api_key = api_key
+ self.timeout = timeoutThen in get_hermine_data:
if data:
- response = requests.post(uri, headers=req_headers, data=data, files=files)
+ response = requests.post(uri, headers=req_headers, data=data, files=files, timeout=self.timeout)
elif params:
- response = requests.get(uri, headers=req_headers, params=params)
+ response = requests.get(uri, headers=req_headers, params=params, timeout=self.timeout)
else:
- response = requests.get(uri, headers=req_headers)
+ response = requests.get(uri, headers=req_headers, timeout=self.timeout)And in HermineViolationsPolicyCheck.__init__, pass the timeout through:
- self.hm_service = HermineService(self.api_key, self.url, debug=debug, trace=trace, quiet=quiet)
+ self.hm_service = HermineService(self.api_key, self.url, timeout=self.timeout, debug=debug, trace=trace, quiet=quiet)🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 150-150: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.post(uri, headers=req_headers, data=data, files=files)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
[warning] 152-152: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.get(uri, headers=req_headers, params=params)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
[warning] 154-154: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.get(uri, headers=req_headers)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
[info] 150-150: no timeout was given on call to external resource
Context: requests.post(uri, headers=req_headers, data=data, files=files)
Note: [CWE-1088] Synchronous Access of Remote Resource without Timeout.
(requests-timeout)
[info] 152-152: no timeout was given on call to external resource
Context: requests.get(uri, headers=req_headers, params=params)
Note: [CWE-1088] Synchronous Access of Remote Resource without Timeout.
(requests-timeout)
[info] 154-154: no timeout was given on call to external resource
Context: requests.get(uri, headers=req_headers)
Note: [CWE-1088] Synchronous Access of Remote Resource without Timeout.
(requests-timeout)
🤖 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/scanoss/services/hermine_service.py` around lines 148 - 156, All HTTP
calls in HermineService.get_hermine_data currently lack a timeout, so requests
can hang indefinitely; update each requests.get/request.post path to use the
service timeout. Also wire the existing timeout from
HermineViolationsPolicyCheck.__init__ through to HermineService so the
user-configured value is actually honored. Use the get_hermine_data method and
the HermineViolationsPolicyCheck constructor to locate the changes, and ensure
any request helper or client setup consistently applies the timeout parameter.
Sources: Linters/SAST tools, Pipeline failures
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
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/policy_check/hermine/violations.py (2)
230-243: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReturn step 4 action items so output files are not blank.
_handle_validation_4()logs unresolved choices but returns[], sorun()skips writing formatted output even though the policy fails.Proposed fix
- return [] + return [ + { + 'project': item.get('project', ''), + 'scope': item.get('scope', ''), + 'description': item.get('description', '').strip(), + } + for item in items + ]Also applies to: 418-421
🤖 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/scanoss/inspection/policy_check/hermine/violations.py` around lines 230 - 243, `_handle_validation_4()` currently only logs the unresolved license choices and returns an empty list, which causes `run()` to skip output generation. Update `_handle_validation_4` in `violations.py` so it builds and returns the step 4 action items from `response["to_resolve"]` (or an equivalent normalized list) instead of `[]`, and keep the existing stderr message for the failure path. Make sure the returned items preserve the data needed by `run()`/the formatter so the output files are populated when validation 4 fails.
275-279: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDistinguish API/request errors from policy violations.
HermineService.get_hermine_data()returnsNoneon request errors, but this code treatsNoneas a validation failure. That can returnPOLICY_FAILwith empty/misleading results instead ofPOLICY_ERROR.Also applies to: 364-388
🤖 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/scanoss/inspection/policy_check/hermine/violations.py` around lines 275 - 279, Update the Hermine validation flow in violations.py so that request failures from HermineService.get_hermine_data() are handled separately from invalid policy responses. In the loop that calls get_hermine_data() and in the related logic around handlers, treat a None response as a POLICY_ERROR path (with an appropriate error result) instead of falling through to handler(response) and POLICY_FAIL, while keeping only explicit valid=False responses as policy violations. Use the existing symbols get_hermine_data, handlers, and the policy result/return logic in the validation methods to locate and apply the same distinction consistently.
🤖 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/scanoss/inspection/policy_check/hermine/violations.py`:
- Around line 327-330: The is_exempt helper in violations.py currently exempts a
usage when any matched license is derogated, which can hide remaining
non-derogated licenses in a multi-license expression. Update the exemption check
to require every license returned by matched_lics(...) to be derogated for the
given version_id before returning true, and keep the logic centered around
is_exempt, matched_lics, and derogated so the intent is clear.
---
Outside diff comments:
In `@src/scanoss/inspection/policy_check/hermine/violations.py`:
- Around line 230-243: `_handle_validation_4()` currently only logs the
unresolved license choices and returns an empty list, which causes `run()` to
skip output generation. Update `_handle_validation_4` in `violations.py` so it
builds and returns the step 4 action items from `response["to_resolve"]` (or an
equivalent normalized list) instead of `[]`, and keep the existing stderr
message for the failure path. Make sure the returned items preserve the data
needed by `run()`/the formatter so the output files are populated when
validation 4 fails.
- Around line 275-279: Update the Hermine validation flow in violations.py so
that request failures from HermineService.get_hermine_data() are handled
separately from invalid policy responses. In the loop that calls
get_hermine_data() and in the related logic around handlers, treat a None
response as a POLICY_ERROR path (with an appropriate error result) instead of
falling through to handler(response) and POLICY_FAIL, while keeping only
explicit valid=False responses as policy violations. Use the existing symbols
get_hermine_data, handlers, and the policy result/return logic in the validation
methods to locate and apply the same distinction consistently.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04e59591-60f3-4a25-b938-b08a2cf4cec4
📒 Files selected for processing (5)
CHANGELOG.mdsrc/scanoss/cli.pysrc/scanoss/export/hermine.pysrc/scanoss/inspection/policy_check/hermine/violations.pysrc/scanoss/services/hermine_service.py
💤 Files with no reviewable changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/scanoss/export/hermine.py
- src/scanoss/cli.py
…nto feat/alex/hermine-integration # Conflicts: # CHANGELOG.md # src/scanoss/cli.py
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: 1
🧹 Nitpick comments (1)
tests/test_file_filters.py (1)
329-341: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract duplicate test setup into a base class.
The
setUp,tearDown, and_createmethods are duplicated acrossTestSkipPatternOperationType,TestSkipPatternNegation, and the existingTestFilterPathclass. Consider extracting these into a common base class (e.g.,BaseFileFiltersTest(unittest.TestCase)) to keep the test suite DRY.🤖 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_file_filters.py` around lines 329 - 341, Extract the duplicated setUp, tearDown, and _create helpers from TestSkipPatternOperationType, TestSkipPatternNegation, and TestFilterPath into a shared BaseFileFiltersTest(unittest.TestCase), then have each test class inherit from it while preserving their existing test behavior.
🤖 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 `@CHANGELOG.md`:
- Line 14: Update the CHANGELOG entry for the `inspect hm v` subcommand to
describe retrieving Hermine release violations, removing the incorrect reference
to Dependency Track project violations while preserving the Markdown and JSON
format details.
---
Nitpick comments:
In `@tests/test_file_filters.py`:
- Around line 329-341: Extract the duplicated setUp, tearDown, and _create
helpers from TestSkipPatternOperationType, TestSkipPatternNegation, and
TestFilterPath into a shared BaseFileFiltersTest(unittest.TestCase), then have
each test class inherit from it while preserving their existing test behavior.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 316ed95f-6da9-4247-ab94-92893c66581d
📒 Files selected for processing (8)
CHANGELOG.mddocs/source/scanoss_settings_schema.rstsrc/scanoss/__init__.pysrc/scanoss/file_filters.pysrc/scanoss/inspection/policy_check/hermine/violations.pysrc/scanoss/scanner.pytests/test_file_filters.pytests/test_hermine.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/scanoss/inspection/policy_check/hermine/violations.py
| ### Added | ||
| - Added Hermine integration | ||
| - Added `export hm` subcommand to export SBOM files to Hermine | ||
| - Added `inspect hm v` subcommand to retrieve Dependency Track project violations in Markdown and JSON formats |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Describe the Hermine violations source correctly.
inspect hm v inspects Hermine release violations, not Dependency Track project violations.
Proposed correction
-- Added `inspect hm v` subcommand to retrieve Dependency Track project violations in Markdown and JSON formats
+- Added `inspect hm v` subcommand to retrieve Hermine release violations in Markdown and JSON formats📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Added `inspect hm v` subcommand to retrieve Dependency Track project violations in Markdown and JSON formats | |
| - Added `inspect hm v` subcommand to retrieve Hermine release violations in Markdown and JSON formats |
🤖 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 `@CHANGELOG.md` at line 14, Update the CHANGELOG entry for the `inspect hm v`
subcommand to describe retrieving Hermine release violations, removing the
incorrect reference to Dependency Track project violations while preserving the
Markdown and JSON format details.
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
|
Hi @Alex-1089, I'm cancelling this PR, we're not going to support Hermine for now. We might come back to it later if needed. Thanks! |
3 participants
Summary by CodeRabbit
export hmandinspect hm vchangelog-documented subcommands with product/release selection by name or ID and configurable reporting (output/status/format).--settingsand--skip-settings-file.