Skip to content

Mscasso/feat/sp 3838 update go.api to support the new scanning parameters#64

Merged
mscasso-scanoss merged 34 commits intomainfrom
mscasso/feat/SP-3838-Update-go.api-to-support-the-new-scanning-parameters
Jan 20, 2026
Merged

Mscasso/feat/sp 3838 update go.api to support the new scanning parameters#64
mscasso-scanoss merged 34 commits intomainfrom
mscasso/feat/SP-3838-Update-go.api-to-support-the-new-scanning-parameters

Conversation

@mscasso-scanoss
Copy link
Copy Markdown
Contributor

@mscasso-scanoss mscasso-scanoss commented Jan 15, 2026

Summary by CodeRabbit

  • New Features

    • Enhanced scanning: Scanoss-Settings header & scanoss.json support, SBOM identify/blacklist modes, engine-version validation, and runtime tuning for ranking, snippet thresholds, match-config control, and file-extension handling; dev/prod configs updated.
  • Tests

    • Added coverage-enabled test targets and new tests for scan settings/header and engine-version handling; updated tests to use JSON payloads and skip on 403 where appropriate.
  • Chores

    • Dependency upgrades, docker-compose env var for match-config, and changelog entry for v1.6.0.

✏️ Tip: You can customize this high-level summary in your review settings.

@mscasso-scanoss mscasso-scanoss self-assigned this Jan 15, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jan 15, 2026

📝 Walkthrough

Walkthrough

Introduce a ScanningServiceConfig DTO and header-based scan settings; route configuration through scanning flows and SBOM handling; add server scanning parameters/defaults; validate engine version during KB load; add tests, coverage Makefile targets, dependency bumps, script tweaks, and config/docker changes.

Changes

Cohort / File(s) Summary
Release & Changelog
CHANGELOG.md
Add v1.6.0 release block and version-history link.
Build & Dependencies
Makefile, go.mod
Add coverage targets (unit_test_cover, int_test_cover, e2e_test_cover); add github.com/hashicorp/go-version; bump OpenTelemetry and multiple transitive deps; zap patch.
Server Config
pkg/config/server_config.go
Add scanning fields and defaults: MatchConfigAllowed, RankingAllowed, RankingEnabled, RankingThreshold, MinSnippetHits, MinSnippetLines, HonourFileExts.
Scanning Config Model & Tests
pkg/service/scanning_service_config.go, pkg/service/scanning_service_config_test.go
New ScanningServiceConfig DTO, DefaultScanningServiceConfig, UpdateScanningServiceConfigDTO, parsing/apply helpers for ranking/snippets/direct params; comprehensive unit tests covering merges, validation, and error paths.
Scanning Service Refactor
pkg/service/scanning_service.go, pkg/service/scanning_service_test.go
Replace per-request params with ScanningServiceConfig; add Scanoss-Settings base64 header decoding; SBOM identify/blacklist handling; propagate ranking/snippet/honour-file-exts into engine args; update scanning function signatures.
Engine Version Enforcement
pkg/service/kb_details.go, pkg/service/kb_details_test.go
Add validateEngineVersion (uses hashicorp/go-version), call during KB details load to enforce minEngineVersion; add test exercising below-minimum behavior.
Tests & E2E adjustments
tests/...
tests/scanning_test.go, tests/charset_detection_test.go, tests/file_contents_test.go
Update tests to send JSON asset payloads; add TestScanSettingsHeader for Base64 header cases; skip some tests on 403 responses; adjust suite embedding.
Test Utilities & Scripts
test-support/scanoss.sh
Strengthen argument validation (MD5 check), robust -w handling, stderr error outputs; simulate invalid KB in -n case.
Config Files & Compose
config/app-config-dev.json, config/app-config-prod.json, docker-compose.yml
Add scanning fields (ScanKbName, MatchConfigAllowed, RankingAllowed, RankingEnabled, RankingThreshold, MinSnippetHits, MinSnippetLines, HonourFileExts); set SCANOSS_MATCH_CONFIG_ALLOWED env var.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant HTTP as "HTTP Handler"
    participant Config as "ScanningServiceConfig"
    participant KB as "KB Loader"
    participant Engine as "Scan Engine"

    Client->>HTTP: POST /scan/direct (form + Scanoss-Settings header)
    HTTP->>Config: getConfigFromRequest(params, decoded header)
    Config->>Config: UpdateScanningServiceConfigDTO (merge JSON/header/params)
    HTTP->>KB: loadKBDetails()
    KB->>KB: validateEngineVersion(current, min)
    KB-->>HTTP: KB info
    HTTP->>Engine: run engine with args (sbomFile, sbomType, ranking, snippets, honour-file-exts)
    Engine-->>HTTP: scan results
    HTTP-->>Client: HTTP response with results
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐇 I munched through headers, JSON crumbs and flags,
I stitched a config cloak from legacy rags.
Engines checked their teeth, versions all aligned,
SBOMs, rankings, snippets — neat and well defined.
Hop! The scanner hums; new settings all combined.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main objective of the PR: adding support for new scanning parameters to go.api, which is evident from the widespread config additions, new ScanningServiceConfig implementation, and parameter propagation throughout the codebase.
Docstring Coverage ✅ Passed Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Fix all issues with AI agents
In `@pkg/service/kb_details.go`:
- Around line 47-68: Fix the typo in validateEngineVersion: update the zs.Errorf
call that logs when currentVersion.LessThan(minVersion) to include a space after
the period in the message (change "'%s'.Some" to "'%s'. Some" or better "'%s'.
Some features..." ), ensuring the log still references currentEngineVersion and
minEngineVersion in the same zs.Errorf invocation.

In `@pkg/service/scanning_service_config.go`:
- Around line 145-153: The doc comment lists a "snippet_range_tolerance" JSON
field that is not implemented; update the scanSettings struct or remove the doc
entry. Either add a field named snippetRangeTolerance (or SnippetRangeTolerance)
of type int to the scanSettings struct and wire any deserialization/validation
logic where scanSettings is parsed, or remove the "snippet_range_tolerance" line
from the comment so documentation matches the implemented scanSettings fields.

In `@pkg/service/scanning_service.go`:
- Around line 436-439: The pull request adds an unsupported "--ignore-file-ext"
flag when config.honourFileExts is false which causes the SCANOSS engine to
error; remove the args append that injects "--ignore-file-ext" in
scanning_service.go (the block that checks config.honourFileExts and calls args
= append(args, "--ignore-file-ext")), or alternatively add a version check for
the engine binary and return a clear error from the same function if the
requested flag is not supported; in short, stop passing "--ignore-file-ext" to
the scanoss engine (or validate engine support and fail fast with a descriptive
message) referencing the config.honourFileExts branch and the args slice
construction.
🧹 Nitpick comments (7)
Makefile (2)

36-39: Minor inconsistency: Missing -v flag compared to unit_test.

The unit_test task uses -v for verbose output, but unit_test_cover omits it. Consider adding -v for consistency:

♻️ Suggested fix
 unit_test_cover:  ## Run all unit tests in the pkg folder
 	`@echo` "Running unit test framework with coverage..."
-	go test -cover ./pkg/...
+	go test -cover -v ./pkg/...

44-47: Minor: Echo message doesn't mention coverage.

The echo message should indicate coverage is enabled for clarity.

♻️ Suggested fix
 int_test_cover: clean_testcache  ## Run all integration tests in the tests folder
-	`@echo` "Running integration test framework..."
+	`@echo` "Running integration test framework with coverage..."
 	go test -cover -v ./tests
pkg/service/scanning_service_config.go (1)

41-54: Consider adding nil check for serverDefaultConfig.

If serverDefaultConfig is nil, this function will panic on the first field access. While callers may ensure this never happens, a defensive check would improve robustness.

♻️ Proposed fix
 func DefaultScanningServiceConfig(serverDefaultConfig *cfg.ServerConfig) ScanningServiceConfig {
+	if serverDefaultConfig == nil {
+		return ScanningServiceConfig{}
+	}
 	return ScanningServiceConfig{
 		flags:            serverDefaultConfig.Scanning.ScanFlags,
pkg/service/kb_details_test.go (1)

87-114: Test lacks proper assertions and modifies global state.

This test has several quality concerns:

  1. Global state modification: Directly setting engineVersion = "5.4.0" (line 102) modifies package-level state, which could cause test flakiness in parallel execution.
  2. No actual assertion: The test acknowledges it "can't easily assert on log output" (lines 110-112) and relies on manual log inspection.
  3. Fragile timing: time.Sleep(3 * time.Second) is unreliable and slows down the test suite.

Consider refactoring to either:

  • Use a log capture mechanism to assert on logged messages
  • Refactor validateEngineVersion to return validation status for testability
  • At minimum, add t.Parallel() avoidance comment to prevent parallel test issues
♻️ Suggested improvement - extract testable validation

One approach would be to have validateEngineVersion return a boolean or error that can be tested directly:

// In kb_details.go - make validation testable
func validateEngineVersion(zs *zap.SugaredLogger, currentEngineVersion, minEngineVersion string) bool {
    // ... existing logic ...
    if currentVersion.LessThan(minVersion) {
        zs.Errorf("...")
        return false
    }
    zs.Infof("...")
    return true
}

// In test file - direct unit test
func TestValidateEngineVersion(t *testing.T) {
    logger, _ := zap.NewDevelopment()
    sugar := logger.Sugar()
    
    if validateEngineVersion(sugar, "5.4.0", "6.0.0") {
        t.Error("Expected validation to fail for version below minimum")
    }
    if !validateEngineVersion(sugar, "6.1.0", "6.0.0") {
        t.Error("Expected validation to pass for version meeting minimum")
    }
}
pkg/service/scanning_service.go (1)

412-423: Consider removing unreachable default case.

The default case on line 419-420 is unreachable because sbomType is already validated at lines 104-109 in scanDirect, ensuring only sbomIdentify or sbomBlackList reach this point. While defensive coding is good practice, unreachable code can cause confusion.

♻️ Optional: Remove unreachable default case
 	// SBOM configuration
 	if len(sbomFile) > 0 && len(config.sbomType) > 0 {
 		switch config.sbomType {
 		case sbomIdentify:
 			args = append(args, "-s")
 		case sbomBlackList:
 			args = append(args, "-b")
-		default:
-			args = append(args, "-s") // Default to identify
 		}
 		args = append(args, sbomFile)
 	}
tests/scanning_test.go (2)

136-187: Consider generating base64 strings at test time for maintainability.

The hardcoded base64 strings with comments work, but generating them at test time would make the tests more maintainable and self-documenting:

♻️ Optional: Generate base64 from JSON for clarity
import "encoding/base64"
import "encoding/json"

// Helper to encode settings for tests
func encodeSettings(settings map[string]interface{}) string {
    data, _ := json.Marshal(settings)
    return base64.StdEncoding.EncodeToString(data)
}

// Then in test:
scanSettingsB64: encodeSettings(map[string]interface{}{
    "ranking_enabled":    true,
    "ranking_threshold":  85,
    "min_snippet_hits":   3,
    "min_snippet_lines":  8,
    "honour_file_exts":   false,
}),

206-219: Close response body to prevent resource leaks.

The resp.Body should be closed after reading. While Go's GC will eventually handle it, explicit closure is best practice for E2E tests that may run many iterations.

♻️ Proposed fix
 			resp, err := c.Do(req)
 			if err != nil {
 				s.Failf("an error was not expected when sending request.", "error: %v", err)
 			}
+			defer resp.Body.Close()

 			s.Equal(test.want, resp.StatusCode, test.description)

Note: The same pattern exists in TestScanning at line 119 - consider applying there as well for consistency.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0dcc097 and 8299112.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (12)
  • CHANGELOG.md
  • Makefile
  • go.mod
  • pkg/config/server_config.go
  • pkg/service/kb_details.go
  • pkg/service/kb_details_test.go
  • pkg/service/scanning_service.go
  • pkg/service/scanning_service_config.go
  • pkg/service/scanning_service_config_test.go
  • pkg/service/scanning_service_test.go
  • test-support/scanoss.sh
  • tests/scanning_test.go
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/service/scanning_service_config_test.go (2)
pkg/config/server_config.go (1)
  • ServerConfig (37-93)
pkg/service/scanning_service_config.go (3)
  • DefaultScanningServiceConfig (41-54)
  • ScanningServiceConfig (28-39)
  • UpdateScanningServiceConfigDTO (162-183)
pkg/service/kb_details_test.go (1)
pkg/service/utils_service.go (1)
  • NewAPIService (76-79)
pkg/service/kb_details.go (1)
pkg/service/scanning_service_config.go (1)
  • DefaultScanningServiceConfig (41-54)
🔇 Additional comments (29)
pkg/service/scanning_service_test.go (1)

3-3: LGTM!

Copyright year update is appropriate maintenance.

CHANGELOG.md (2)

11-21: LGTM!

The changelog entries accurately document the new scanoss.json configuration support and the server-side scanning parameters introduced in this PR. The format follows the existing Keep a Changelog conventions.


175-175: LGTM!

Version link correctly references the comparison from v1.5.2 to v1.6.0.

Makefile (1)

78-84: LGTM!

The e2e_test_cover task correctly mirrors e2e_test while adding the -cover flag for coverage reporting.

test-support/scanoss.sh (3)

30-34: LGTM!

The MD5 validation correctly ensures the hash is exactly 32 hexadecimal characters using a proper regex pattern. Clear error message and appropriate exit code.


59-68: LGTM!

The validation correctly rejects -n as a standalone argument while allowing -n<name> (attached format). The error message clearly indicates the expected usage.


71-79: Last argument assumption is valid given actual usage pattern.

The script captures the last argument as the WFP file, which works correctly because scanning_service.go always appends -w and the file path at the end of the argument list with no subsequent arguments. The concern about additional flags following the WFP file is theoretically sound but does not apply to the current invocation pattern.

go.mod (1)

37-37: LGTM! Dependency addition supports version validation.

The hashicorp/go-version library is a well-established choice for semantic version parsing and comparison. Version 1.8.0 is current (released November 2025) and correctly supports the engine version validation implemented in pkg/service/kb_details.go.

pkg/config/server_config.go (2)

73-80: LGTM! Well-structured configuration additions.

The new scanning parameters are properly documented with inline comments, follow the existing naming conventions, and have appropriate environment variable tags for external configuration.


131-138: LGTM! Sensible defaults.

The defaults follow a good principle: ranking is allowed but disabled by default (opt-in), and zero values for thresholds delegate decision-making to the engine. The HonourFileExts = true default is appropriately conservative.

pkg/service/scanning_service_config.go (6)

28-39: LGTM! Good encapsulation with unexported fields.

The struct properly encapsulates scanning configuration with unexported fields, enforcing access control at the package level.


56-63: LGTM! Proper use of pointer types for optional JSON fields.

Using pointers allows distinguishing between "field not provided" (nil) and "field explicitly set to zero/false", which is essential for partial updates.


65-80: LGTM! Proper guard for ranking settings.

The function correctly checks rankingAllowed before applying ranking changes and logs a warning when settings are ignored, providing good debugging visibility.


82-106: LGTM! Appropriate validation for snippet settings.

The different validation thresholds make sense: MinSnippetHits >= 0 allows disabling the check (0 = engine decides), while MinSnippetLines > 0 correctly requires a positive value since 0 lines would be meaningless.


108-131: LGTM! Proper handling of legacy string parameters.

The function correctly treats empty strings as "no change" and gracefully handles invalid flag values by logging an error without failing the entire update.


162-182: LGTM! Clean copy-on-write implementation.

The implementation correctly creates a shallow copy (safe since all fields are value types), applies updates sequentially, and maintains immutability of the original config.

pkg/service/scanning_service_config_test.go (7)

27-64: LGTM! Comprehensive validation of default config creation.

The test thoroughly validates that all fields are correctly mapped from ServerConfig.Scanning to ScanningServiceConfig.


66-126: LGTM! Thorough JSON settings test.

The test validates all JSON-configurable fields and properly uses pointer types to ensure the JSON payload is correctly structured.


128-168: LGTM! Good negative test for ranking guard.

The test correctly validates that ranking settings are ignored when rankingAllowed is false, ensuring the security guard works as intended.


170-204: LGTM! Validates legacy parameter handling.

The test ensures backward compatibility by validating that legacy string parameters (flags, scanType, sbom, dbName) are correctly applied.


206-234: LGTM! Proper error handling validation.

The test correctly validates two error scenarios: graceful degradation for invalid flags (keeps original) and proper error return for malformed JSON.


236-301: LGTM! Good integration test for combined updates.

The test validates that both JSON settings and legacy string parameters can be applied in a single call, which is the expected real-world usage pattern.


303-313: LGTM! Validates nil config error handling.

The test ensures that passing a nil config returns an appropriate error rather than panicking.

pkg/service/kb_details.go (1)

113-114: LGTM! Clean integration with new config model.

Using DefaultScanningServiceConfig(s.config) properly initializes the scanning config from server defaults for the KB details probe. The minEngineVersion constant is correctly defined in the package and properly used for engine version validation.

pkg/service/scanning_service.go (4)

40-44: LGTM! Constants defined for SBOM types and engine version.

The constants are well-named and provide clear documentation of their purpose. Note that minEngineVersion is declared here but used in kb_details.go for validation.


169-208: LGTM! Well-structured configuration extraction with proper fallback handling.

The function properly:

  • Extracts from form values first, falling back to headers
  • Handles base64 decoding with appropriate error propagation
  • Creates defaults from server config before applying request-specific overrides

227-229: LGTM! Consistent refactoring to use ScanningServiceConfig.

The function signatures have been cleanly updated to accept the configuration struct, maintaining a consistent pattern across singleScan, scanThreaded, workerScan, and scanWfp.


102-120: LGTM! Proper SBOM type validation and file handling.

The validation correctly checks against the defined constants and provides clear error messages. The temporary file cleanup via defer removeFile ensures resources are properly released.

tests/scanning_test.go (1)

93-101: LGTM! Updated asset format to JSON structure.

The assets field now uses the proper JSON format with components array, which aligns with the SBOM structure expected by the scanning service.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Comment thread pkg/service/kb_details.go
Comment thread pkg/service/scanning_service_config.go
Comment thread pkg/service/scanning_service.go
Comment thread pkg/service/scanning_service.go
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Fix all issues with AI agents
In `@pkg/service/scanning_service.go`:
- Around line 169-191: In getConfigFromRequest replace the debug log that prints
r.Header and r.Form with a redacted/whitelist-only log: update the zs.Debugf
call in function getConfigFromRequest to omit r.Header and r.Form entirely and
only log the safe, trimmed variables (flags, scanType, sbom, dbName,
scanSettings) and optionally a small whitelist of specific headers (e.g.,
"Authorization" excluded; only include header names you explicitly permit) or
indicate "<redacted>" for headers; ensure you modify the zs.Debugf invocation so
it no longer interpolates r.Header or r.Form to avoid leaking credentials.

In `@test-support/scanoss.sh`:
- Around line 30-34: The MD5 validation block that checks the variable md5
currently prints the error with echo to stdout; update the if block that tests
[[ ! "$md5" =~ ^[a-fA-F0-9]{32}$ ]] so the error message for "Error: Invalid MD5
hash format: $md5" is written to stderr (use the same redirection pattern as
other errors in this script, e.g., append >&2) and then exit 1; ensure only the
echo destination changes, leaving the condition and exit intact.

In `@tests/charset_detection_test.go`:
- Around line 81-83: Add the same 403 check and skip logic to the
TestFileContentsWithMissingMD5 test: detect when the HTTP response (resp) from
the file_contents endpoint has StatusCode == http.StatusForbidden and call
s.T().Skip("skipping test: file_contents endpoint returned 403 Forbidden") so
the test is skipped in restricted environments; modify the
TestFileContentsWithMissingMD5 function to mirror the existing check used
elsewhere.
♻️ Duplicate comments (1)
pkg/service/scanning_service.go (1)

399-439: Do not pass --ignore-file-ext unless the engine explicitly supports it.
The inline comment says it’s not implemented; passing it risks breaking scans. Gate by engine version or remove the flag.

Option A: remove the unsupported flag
-    // Honour file extensions (not yet implemented in scanoss engine)
-    if !config.honourFileExts {
-        args = append(args, "--ignore-file-ext")
-    }

Please verify current engine support in the official docs:

SCANOSS engine "--ignore-file-ext" flag support (v5.4.20+)?
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e3f84b6 and d93293d.

📒 Files selected for processing (4)
  • pkg/service/scanning_service.go
  • test-support/scanoss.sh
  • tests/charset_detection_test.go
  • tests/file_contents_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/service/scanning_service.go (2)
pkg/service/utils_service.go (1)
  • APIService (71-73)
pkg/service/scanning_service_config.go (3)
  • ScanningServiceConfig (28-39)
  • DefaultScanningServiceConfig (41-54)
  • UpdateScanningServiceConfigDTO (161-182)
🔇 Additional comments (8)
tests/file_contents_test.go (1)

41-43: LGTM - Graceful handling of disabled/restricted endpoints.

Skipping on 403 is reasonable for E2E tests that may run against environments with different access configurations. The skip message is descriptive.

tests/charset_detection_test.go (1)

42-44: LGTM - Consistent with other file_contents tests.

test-support/scanoss.sh (2)

59-71: LGTM - KB validation logic is clear and well-documented.

The distinction between -n (bare, invalid) and -n<name> (attached, valid but checked for test_kb) is correctly implemented with helpful comments.


74-82: LGTM - Position-independent -w detection is more robust.

The refactor makes the script handle -w regardless of its position in the argument list, which is more flexible for varying invocation patterns.

pkg/service/scanning_service.go (4)

3-3: Copyright year update looks good.


22-44: SBOM constants/min engine version addition is clear.


96-139: Scan config extraction + SBOM validation integration looks solid.


227-265: Config propagation through scan workers is consistent.

Also applies to: 338-353

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Comment thread pkg/service/scanning_service.go
Comment thread test-support/scanoss.sh
Comment thread tests/charset_detection_test.go
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@go.mod`:
- Around line 14-22: The go.mod pins OTLP exporter modules
(go.opentelemetry.io/otel/exporters/otlp/otlptrace,
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc,
go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc) to v1.38.0
while core otel packages are v1.39.0; either bump these three exporter module
versions to v1.39.0 in go.mod for consistency, or add a short comment in the
file/PR confirming that keeping v1.38.0 is intentional for compatibility (and
document which incompatibility requires the pin). Ensure you update the module
lines for otlptrace, otlptracegrpc, and otlpmetricgrpc or add the confirmation
comment accordingly.
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d93293d and f13e2e3.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (1)
  • go.mod
🔇 Additional comments (2)
go.mod (2)

23-23: LGTM!

The zap logger patch update from v1.27.0 to v1.27.1 is a safe change.


37-37: No action required. Security verification confirms all critical indirect dependencies are in patched versions:

  • google.golang.org/grpc v1.78.0: No CVEs; CVE-2023-44487 fixed in v1.56.3+
  • google.golang.org/protobuf v1.36.11: No CVEs; prior vulnerabilities fixed in earlier versions
  • golang.org/x/net v0.47.0: Exceeds minimum patched versions for CVE-2025-22870 (v0.36.0) and CVE-2025-22872 (v0.38.0)
  • golang.org/x/sys v0.39.0: No active security advisories

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Comment thread go.mod
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@pkg/service/scanning_service_config.go`:
- Around line 101-107: The validation currently rejects a client-provided zero
for MinSnippetLines even though server default 0 is a valid sentinel; update the
check in the block handling settings.MinSnippetLines so that it accepts zero
(use >= 0 as MinSnippetHits does) and only treat negative values as
invalid—i.e., when settings.MinSnippetLines != nil set config.minSnippetLines =
*settings.MinSnippetLines for values >= 0, otherwise append to invalidSettings;
reference the symbols settings.MinSnippetLines, config.minSnippetLines, and
invalidSettings to locate and adjust the logic.
🧹 Nitpick comments (3)
pkg/service/scanning_service_config.go (3)

42-56: Add nil check for serverDefaultConfig parameter.

If serverDefaultConfig is nil, accessing its fields will cause a panic. Consider adding a defensive nil check, similar to the check in UpdateScanningServiceConfigDTO.

🛡️ Proposed fix
 func DefaultScanningServiceConfig(serverDefaultConfig *cfg.ServerConfig) ScanningServiceConfig {
+	if serverDefaultConfig == nil {
+		return ScanningServiceConfig{
+			honourFileExts: true, // Safe default
+		}
+	}
 	return ScanningServiceConfig{
 		flags:              serverDefaultConfig.Scanning.ScanFlags,

67-82: Inconsistent error handling between ranking and snippet settings.

When rankingAllowed is false, ranking settings are silently ignored with a warning. However, applySnippetSettings returns an error when matchConfigAllowed is false. This asymmetry may confuse API clients who receive different responses for similar scenarios.

Consider either:

  1. Returning an error here as well for consistency, or
  2. Documenting why the behaviors intentionally differ (e.g., ranking is a "soft" preference while snippet matching is stricter)

183-187: Partial configuration returned on error.

When applySnippetSettings returns an error, the function returns updatedConfig which already has ranking settings applied (line 183). This means the caller receives a partially updated configuration along with the error.

Consider whether this is the intended behavior. If not, you could defer all mutations until validation passes, or return the original *currentConfig on error.

+	// Validate snippet settings first before applying any changes
+	if len(inputSettings) > 0 {
+		_, err := applySnippetSettings(s, &ScanningServiceConfig{matchConfigAllowed: currentConfig.matchConfigAllowed}, &newSettings)
+		if err != nil {
+			return *currentConfig, err
+		}
+	}
 	applyRankingSettings(s, &updatedConfig, &newSettings)
-	invalidSettings, err := applySnippetSettings(s, &updatedConfig, &newSettings)
-	if err != nil {
-		return updatedConfig, err
-	}
+	invalidSettings, _ := applySnippetSettings(s, &updatedConfig, &newSettings)

Comment thread pkg/service/scanning_service_config.go
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@pkg/service/scanning_service.go`:
- Around line 196-201: The guard that returns an error when
s.config.Scanning.MatchConfigAllowed is false is too aggressive because it
rejects all scanoss-settings (including ranking-only requests); instead, remove
that early return and still decode scanSettings into decoded and call
UpdateScanningServiceConfigDTO (or the corresponding function that applies
settings) so that the DTO/enforcer can validate and reject only match-config
changes while allowing ranking when s.config.Scanning.RankingAllowed is true; in
short, delete the block that errors on MatchConfigAllowed, keep the decoding of
scanSettings and pass the decoded payload to
UpdateScanningServiceConfigDTO/scanConfig handling to enforce match vs ranking
rules.
♻️ Duplicate comments (3)
pkg/service/scanning_service_config.go (1)

97-103: Allow zero for MinSnippetLines to match server default.
Line 98 rejects explicit 0 even though the server default uses 0 as a sentinel. This is inconsistent with MinSnippetHits.

♻️ Proposed fix
-	if *settings.MinSnippetLines > 0 {
+	if *settings.MinSnippetLines >= 0 {
 		config.minSnippetLines = *settings.MinSnippetLines
 		zs.Debugf("Updated MinSnippetLines to %d", config.minSnippetLines)
 	} else {
pkg/service/scanning_service.go (2)

189-192: Avoid logging full headers/forms to prevent credential leakage.
Line 190 logs r.Header and r.Form, which can include auth tokens or cookies. Please redact/whitelist.

🛡️ Proposed fix (redacted logging)
-		zs.Debugf("Header: %v, Form: %v, flags: %v, type: %v, assets: %v, db_name: %v, scanSettings: %v",
-			r.Header, r.Form, flags, scanType, sbom, dbName, scanSettings)
+		zs.Debugw("Scan request params",
+			"flags", flags,
+			"type", scanType,
+			"assets_len", len(sbom),
+			"db_name", dbName,
+			"scanSettings_present", len(scanSettings) > 0,
+		)

440-443: Do not pass --ignore-file-ext without verified engine support.
Line 441 still appends an engine flag that is not guaranteed to exist; scans will fail on unsupported engine versions. Gate this by a verified engine version or remove it until confirmed.

🧩 Safe default: remove the flag
-	// Honour file extensions (not yet implemented in scanoss engine)
-	if !config.honourFileExts {
-		args = append(args, "--ignore-file-ext")
-	}

Comment thread pkg/service/scanning_service.go
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@config/app-config-dev.json`:
- Around line 12-20: The dev config currently forces zero-value thresholds by
setting "RankingThreshold", "MinSnippetHits", and "MinSnippetLines" to 0 which
overrides server defaults; if you want dev to inherit production/server
defaults, remove these three keys from the JSON (or set them to null/omit them)
in config/app-config-dev.json so the server's default behavior is used instead,
leaving other keys like "RankingEnabled" and "HonourFileExts" unchanged.

Comment thread config/app-config-dev.json
mscasso-scanoss and others added 3 commits January 20, 2026 00:28
Updated the release date for version 1.6.0 and added new configuration parameters.
@mscasso-scanoss mscasso-scanoss merged commit 53edf9b into main Jan 20, 2026
2 checks passed
@mscasso-scanoss mscasso-scanoss deleted the mscasso/feat/SP-3838-Update-go.api-to-support-the-new-scanning-parameters branch January 20, 2026 10:59
@scanoss scanoss deleted a comment from coderabbitai Bot Jan 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants