Mscasso/feat/sp 3838 update go.api to support the new scanning parameters #58
Conversation
📝 WalkthroughWalkthroughAdds server-side scanning tuning fields and defaults; introduces per-request ScanningServiceConfig merged from server defaults, base64 JSON Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTP as "HTTP Server"
participant Service as "ScanningService"
participant Storage as "SBOM/Storage"
participant Engine as "Scan Engine"
Client->>HTTP: POST /scan/direct (multipart + optional Scanoss-Settings header)
HTTP->>Service: forward request and headers
Service->>Service: DefaultScanningServiceConfig(server defaults)
Service->>Service: getConfigFromRequest (decode base64 JSON -> UpdateScanningServiceConfigDTO)
alt SBOM provided
Service->>Storage: validate/store SBOM (config.sbomFile / config.sbomType)
Storage-->>Service: sbomFile path
end
Service->>Engine: start scan(s) with ScanningServiceConfig + sbomFile
Engine->>Engine: workerScan threads use config (flags, ranking, snippet opts)
Engine-->>Service: aggregated scan results
Service->>HTTP: respond with results/status
HTTP->>Client: HTTP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (2)pkg/service/scanning_service_config.go (1)
pkg/service/scanning_service.go (2)
🔇 Additional comments (6)
✏️ Tip: You can disable this entire section by setting 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
tests/scanning_test.go (1)
136-222: Good test coverage for the new scan settings header feature.The test covers key scenarios: valid base64-encoded settings, invalid base64 handling, and integration with legacy flags. Consider adding a test case for malformed JSON (valid base64 but invalid JSON content) to verify that the server handles this gracefully and uses defaults.
🔎 Optional: Additional test case for malformed JSON
{ name: "Test Invalid ScanSettings - Malformed JSON", filename: "../pkg/service/tests/fingers.wfp", shortName: "fingers.wfp", // Base64 of "{invalid json}" scanSettingsB64: "e2ludmFsaWQganNvbn0=", extraFields: map[string]string{}, want: http.StatusOK, description: "Should handle malformed JSON gracefully and continue with defaults", },pkg/service/scanning_service_config.go (2)
74-74: Address the TODO or create an issue to track it.The TODO indicates a warning should be added when ranking settings are rejected due to
RankingAllowedbeing false. This would improve observability for API consumers.Would you like me to open an issue to track adding a warning log when ranking settings are rejected?
100-123: Simplify redundant empty string checks.The conditions
len(x) > 0 && x != ""are redundant since an empty string always has length 0. Either check is sufficient.🔎 Proposed fix
- if len(dbName) > 0 && dbName != "" { + if dbName != "" { currentConfig.dbName = dbName s.Debugf("Updated DbName to %s", currentConfig.dbName) } - if len(flags) > 0 && flags != "" { + if flags != "" { flagsInt, err := strconv.Atoi(flags) if err != nil { s.Errorf("Error converting flags to integer: %v", err) } else { currentConfig.flags = flagsInt s.Debugf("Updated Flags to %d", currentConfig.flags) } } - if len(scanType) > 0 && scanType != "" { + if scanType != "" { currentConfig.sbomType = scanType s.Debugf("Updated SbomType to %s", currentConfig.sbomType) } - if len(sbom) > 0 && sbom != "" { + if sbom != "" { currentConfig.sbomFile = sbom s.Debugf("Updated SbomFile to %s", currentConfig.sbomFile) }pkg/service/scanning_service.go (1)
93-95: Extract "identify" and "blacklist" as constants.Static analysis flagged that the string
"identify"appears 3 times. Extract these SBOM type strings as constants for maintainability.🔎 Proposed fix - Add constants at package level
+const ( + sbomTypeIdentify = "identify" + sbomTypeBlacklist = "blacklist" +) + // scanDirect handles WFP scanning requests from a client.Then update usages:
- if scanConfig.sbomType != "identify" && scanConfig.sbomType != "blacklist" { + if scanConfig.sbomType != sbomTypeIdentify && scanConfig.sbomType != sbomTypeBlacklist { ... - case "identify": + case sbomTypeIdentify: args = append(args, "-s") - case "blacklist": + case sbomTypeBlacklist: args = append(args, "-b") default: args = append(args, "-s") // Default to identifyAlso applies to: 414-421
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/config/server_config.gopkg/service/kb_details.gopkg/service/scanning_service.gopkg/service/scanning_service_config.gopkg/service/scanning_service_config_test.gotests/scanning_test.go
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/service/kb_details.go (1)
pkg/service/scanning_service_config.go (1)
DefaultScanningServiceConfig(39-52)
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(39-52)ScanningServiceConfig(26-37)UpdateScanningServiceConfigDTO(54-126)
pkg/service/scanning_service.go (2)
pkg/service/utils_service.go (1)
APIService(71-73)pkg/service/scanning_service_config.go (3)
ScanningServiceConfig(26-37)DefaultScanningServiceConfig(39-52)UpdateScanningServiceConfigDTO(54-126)
🪛 GitHub Check: build
pkg/config/server_config.go
[warning] 77-77:
comment-spacings: no space between comment delimiter and comment text (revive)
[warning] 73-73:
comment-spacings: no space between comment delimiter and comment text (revive)
[warning] 131-131:
comment-spacings: no space between comment delimiter and comment text (revive)
pkg/service/scanning_service.go
[failure] 94-94:
string identify has 3 occurrences, make it a constant (goconst)
[failure] 197-197:
elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocritic)
🔇 Additional comments (13)
pkg/service/kb_details.go (1)
88-89: LGTM!The refactor correctly uses
DefaultScanningServiceConfigto create a configuration object from server defaults before passing it toscanWfp. This aligns with the new unified configuration flow introduced across the scanning service.tests/scanning_test.go (2)
51-51: LGTM - Expected behavior change documented.The change from
http.StatusInternalServerErrortohttp.StatusOKreflects that the engine now handles invalid KB names gracefully with a fallback, as noted in the comment.
93-101: LGTM - Assets format updated to proper JSON structure.The assets field now uses proper JSON format with
componentsarray containing PURLs, which aligns with the expected SBOM structure.pkg/service/scanning_service_config_test.go (5)
27-64: LGTM - Comprehensive test for default config creation.The test properly validates that all fields from
ServerConfig.Scanningare correctly mapped toScanningServiceConfig.
66-123: LGTM - Good coverage for JSON-based settings updates.The test validates that all JSON settings fields are correctly parsed and applied when
rankingAllowedis true.
125-162: LGTM - Important test for the RankingAllowed guard.This test verifies the critical security/business logic that ranking settings are ignored when
RankingAllowedis false at the server level.
164-222: LGTM - Good coverage for legacy parameters and invalid input handling.Tests properly validate backward compatibility with legacy string parameters and graceful error handling for invalid inputs.
224-286: LGTM - Combined update test validates precedence.Good test ensuring both JSON settings and legacy parameters can be applied together in a single update.
pkg/service/scanning_service_config.go (2)
26-37: LGTM - Well-structured configuration type.The
ScanningServiceConfigstruct cleanly encapsulates all scanning parameters with appropriate types. Using unexported fields enforces controlled access through the provided functions.
39-52: LGTM - Clean mapping from server config to service config.
DefaultScanningServiceConfigcorrectly initializes all fields from the server configuration, providing sensible empty defaults forsbomTypeandsbomFile.pkg/service/scanning_service.go (3)
90-109: LGTM - Clean integration with new config flow.The scan direct handler now correctly uses
getConfigFromRequestto obtain configuration and accesses SBOM settings through the config object.
224-226: LGTM - Updated method signatures.The
singleScanmethod correctly accepts the newScanningServiceConfigparameter and passes it through toscanWfp.
425-443: LGTM - New scan arguments are properly conditionally added.The new ranking and snippet parameters are correctly added to the command arguments when their conditions are met. Good to log that
honourFileExtsis not yet implemented in the engine.
agustingroh
left a comment
agustingroh
left a comment
There was a problem hiding this comment.
Please, add changes to CHANGELOG.md file
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/config/server_config.go (1)
131-138: Typo in comment on line 137.The comment for
MinSnippetLinessays "minimum snippet hits" but should say "minimum snippet lines".🔎 Proposed fix
- cfg.Scanning.MinSnippetLines = 0 // Lets the engine decide on minimum snippet hits on the file total lines + cfg.Scanning.MinSnippetLines = 0 // Lets the engine decide on minimum snippet lines based on the file total linespkg/service/scanning_service.go (1)
411-414: Flag format inconsistency.This was flagged in a previous review: the flag uses
"-F %v"with a space, but other single-character flags in this file use no space (e.g.,"-n%s"on line 408,"-r%d"on line 431).🔎 Proposed fix
- args = append(args, fmt.Sprintf("-F %v", config.flags)) + args = append(args, fmt.Sprintf("-F%v", config.flags))
🧹 Nitpick comments (1)
pkg/service/scanning_service_config.go (1)
108-131: Simplify redundant empty string checks.The checks
len(x) > 0 && x != ""are redundant since a non-empty string always has length > 0.🔎 Proposed fix
- if len(dbName) > 0 && dbName != "" { + if dbName != "" { currentConfig.dbName = dbName s.Debugf("Updated DbName to %s", currentConfig.dbName) } - if len(flags) > 0 && flags != "" { + if flags != "" { flagsInt, err := strconv.Atoi(flags) if err != nil { s.Errorf("Error converting flags to integer: %v", err) } else { currentConfig.flags = flagsInt s.Debugf("Updated Flags to %d", currentConfig.flags) } } - if len(scanType) > 0 && scanType != "" { + if scanType != "" { currentConfig.sbomType = scanType s.Debugf("Updated SbomType to %s", currentConfig.sbomType) } - if len(sbom) > 0 && sbom != "" { + if sbom != "" { currentConfig.sbomFile = sbom s.Debugf("Updated SbomFile to %s", currentConfig.sbomFile) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.mdpkg/config/server_config.gopkg/service/scanning_service.gopkg/service/scanning_service_config.gopkg/service/scanning_service_config_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/service/scanning_service_config.go (1)
pkg/config/server_config.go (1)
ServerConfig(37-93)
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(39-52)ScanningServiceConfig(26-37)UpdateScanningServiceConfigDTO(54-134)
🔇 Additional comments (11)
pkg/config/server_config.go (1)
73-80: LGTM!The new scanning configuration fields are well-structured with appropriate environment variable tags and clear inline documentation.
pkg/service/scanning_service_config.go (3)
26-37: LGTM!The struct encapsulates all scanning configuration fields appropriately. Using unexported fields enforces controlled access through the provided functions.
39-52: LGTM!The default config factory correctly maps all server configuration fields to the service config.
54-73: Consider whether early return on JSON error is the desired behavior.When JSON unmarshalling fails, the function logs an error and returns the original config unchanged. This silently ignores potentially malformed input. Consider whether returning an error or setting a flag to indicate partial configuration would be more appropriate for debugging/observability.
pkg/service/scanning_service_config_test.go (3)
27-64: LGTM!Comprehensive test for default config creation, validating all fields are correctly mapped from server config.
125-162: LGTM!Good test ensuring the
RankingAllowedguard prevents ranking settings from being modified when not allowed.
197-222: LGTM!Important error handling tests that verify the config remains unchanged when given invalid input.
pkg/service/scanning_service.go (4)
40-43: LGTM!Good practice defining constants for SBOM types to avoid magic strings.
95-114: LGTM!Clean integration of the new config-based approach for SBOM validation and file handling.
164-209: LGTM!Well-structured function that consolidates config extraction from form values, headers, and base64-encoded settings. The else-if style issue from previous review has been addressed.
429-447: LGTM!Good handling of the new scanning parameters. The
honourFileExtslogging with a note about engine support is a pragmatic approach for future-proofing.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/service/scanning_service.go (1)
411-414: Fix flag formatting inconsistency.The flag formatting uses
fmt.Sprintf("-F %v", config.flags)with a space, but other single-character flags in the same function consistently use no space:-n%s(line 408) and-r%d(line 431). Change to-F%vto maintain consistency.🔎 Proposed fix
// Scanning flags if config.flags > 0 { - args = append(args, fmt.Sprintf("-F %v", config.flags)) + args = append(args, fmt.Sprintf("-F%v", config.flags)) }
🧹 Nitpick comments (1)
pkg/service/scanning_service_config.go (1)
54-134: Consider simplifying the parameter/return pattern.The function takes a pointer parameter, modifies it in-place, and then returns the dereferenced value. Since callers use the return value (line 208 in scanning_service.go), the pointer modification is redundant. Consider either:
- Taking the config by value and returning the modified value (no pointer needed), OR
- Modifying the pointer and returning nothing (void)
The current pattern works but may confuse future maintainers about whether to use the modified pointer or the returned value.
🔎 Proposed refactor (pass by value)
-func UpdateScanningServiceConfigDTO(s *zap.SugaredLogger, currentConfig *ScanningServiceConfig, +func UpdateScanningServiceConfigDTO(s *zap.SugaredLogger, currentConfig ScanningServiceConfig, flags, scanType, sbom, dbName string, inputSettings []byte) ScanningServiceConfig { // ScanSettings represents the scanning parameters that can be configured type scanSettings struct { RankingEnabled *bool `json:"ranking_enabled,omitempty"` RankingThreshold *int `json:"ranking_threshold,omitempty"` MinSnippetHits *int `json:"min_snippet_hits,omitempty"` MinSnippetLines *int `json:"min_snippet_lines,omitempty"` HonourFileExts *bool `json:"honour_file_exts,omitempty"` } // Parse scan settings from JSON if provided var newSettings scanSettings if len(inputSettings) > 0 { err := json.Unmarshal(inputSettings, &newSettings) if err != nil { s.Errorf("Error unmarshalling scanning service config input: %v", err) - return *currentConfig + return currentConfig } } if newSettings.RankingEnabled != nil { if currentConfig.rankingAllowed { currentConfig.rankingEnabled = *newSettings.RankingEnabled s.Debugf("Updated RankingEnabled to %v", currentConfig.rankingEnabled) } else { s.Warnf("RankingEnabled setting ignored as RankingAllowed is false") } } // ... rest of the function remains the same ... - return *currentConfig + return currentConfig }And update the caller at line 208 in scanning_service.go:
- return UpdateScanningServiceConfigDTO(zs, &scanConfig, flags, scanType, sbom, dbName, decoded) + return UpdateScanningServiceConfigDTO(zs, scanConfig, flags, scanType, sbom, dbName, decoded)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.mdpkg/service/scanning_service.gopkg/service/scanning_service_config.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/service/scanning_service.go (2)
pkg/service/utils_service.go (1)
APIService(71-73)pkg/service/scanning_service_config.go (3)
ScanningServiceConfig(26-37)DefaultScanningServiceConfig(39-52)UpdateScanningServiceConfigDTO(54-134)
pkg/service/scanning_service_config.go (1)
pkg/config/server_config.go (1)
ServerConfig(37-93)
🔇 Additional comments (8)
CHANGELOG.md (1)
11-15: LGTM!The version entry is properly formatted with a recent release date and clear descriptions of the new scanning configuration features. The comparison link that was previously missing has been added at line 169.
pkg/service/scanning_service_config.go (1)
39-52: LGTM!The function correctly initializes default scanning configuration from server-wide settings, with appropriate defaults for SBOM fields.
pkg/service/scanning_service.go (6)
40-43: LGTM!The SBOM type constants are well-defined and improve maintainability by centralizing these string literals.
164-209: LGTM!The function cleanly extracts configuration from both form values and headers, with proper base64 decoding of the scan settings header. The fallback logic ensures flexibility in how clients can provide parameters.
95-115: LGTM!The config-driven approach is well-integrated here, with proper validation of SBOM types using the defined constants. The configuration flows correctly to the scanning functions.
228-228: LGTM!The function signatures have been consistently updated to accept the scanning configuration. Passing the config by value is appropriate since these functions only read from it.
Also applies to: 248-248, 340-340, 379-379
416-427: LGTM!The SBOM configuration logic correctly maps the SBOM type to the appropriate command-line flags using the defined constants, with a sensible default fallback.
444-447: Update the stale comment or remove the unsupported flag.The
--ignore-file-extflag is added to scan arguments whenhonourFileExtsis false (lines 445-446), but the comment states it's "not yet implemented in scanoss engine." The mock test engine does not recognize this flag, and no engine documentation confirms support. Either:
- Update the comment to reflect current engine support if now implemented, or
- Remove or gate this flag behind a feature flag until engine support is confirmed
Running scans with an unrecognized flag risks failures depending on the engine version in use.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pkg/service/scanning_service.go (1)
406-414: Fix flag formatting inconsistency.Line 413 formats the
-Fflag with a space ("-F %v"), while other single-character flags in the same function use no space (e.g., line 408:"-n%s", line 431:"-r%d"). This creates an inconsistent argument format.🔎 Proposed fix
// Scanning flags if config.flags > 0 { - args = append(args, fmt.Sprintf("-F %v", config.flags)) + args = append(args, fmt.Sprintf("-F%v", config.flags)) }pkg/config/server_config.go (1)
132-140: Fix minor typo in comment.Line 138 has a small typo: "on the file total lines" should be "based on the file total lines" to match the comment on line 137.
🔎 Proposed fix
- cfg.Scanning.MinSnippetLines = 0 // Lets the engine decide on minimum snippet hits on the file total lines + cfg.Scanning.MinSnippetLines = 0 // Lets the engine decide on minimum snippet lines based on the file total linesAlso note: the comment says "minimum snippet hits" but should say "minimum snippet lines" to match the field name.
🧹 Nitpick comments (2)
pkg/config/server_config.go (1)
73-81: Good addition of scanning configuration fields.The new ranking and snippet matching configuration fields are well-documented and properly grouped. The defaults set later in the file are reasonable.
💡 Optional: Consider using full field name for consistency
The field
SnippetRangeTolabbreviates "Tolerance" to "Tol", while other fields use full words (e.g.,MinSnippetHits,MinSnippetLines). For consistency, consider renaming toSnippetRangeTolerance:- SnippetRangeTol int `env:"SCANOSS_SNIPPET_RANGE_TOL"` // Snippet range tolerance for matching + SnippetRangeTolerance int `env:"SCANOSS_SNIPPET_RANGE_TOL"` // Snippet range tolerance for matchingNote: This would require updating all references in
scanning_service_config.gowhere it's currently mapped assnippetRangeTolerance.pkg/service/scanning_service_config.go (1)
26-38: Consider field visibility for better encapsulation.All fields in
ScanningServiceConfigare private (unexported). While this works since the accessing code is in the same package, it limits flexibility and testability. Consider either:
- Making fields public (exported) if external packages need access
- Adding getter methods for better encapsulation
The current approach is acceptable for internal package use, but adding getters would improve maintainability if the struct evolves.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/config/server_config.gopkg/service/scanning_service.gopkg/service/scanning_service_config.gopkg/service/scanning_service_config_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/service/scanning_service_config_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/service/scanning_service_config.go (1)
pkg/config/server_config.go (1)
ServerConfig(37-94)
pkg/service/scanning_service.go (2)
pkg/service/utils_service.go (1)
APIService(71-73)pkg/service/scanning_service_config.go (3)
ScanningServiceConfig(26-38)DefaultScanningServiceConfig(40-54)UpdateScanningServiceConfigDTO(56-142)
🪛 GitHub Actions: Go Unit Test
pkg/service/scanning_service_config.go
[error] TestUpdateScanningServiceConfigDTO_JSONSettings failed: Expected SnippetRangeTol to be 10, got 0.
🔇 Additional comments (5)
pkg/service/scanning_service.go (4)
40-43: Good addition of SBOM constants.Defining constants for the SBOM types improves code maintainability and reduces the risk of typos.
95-115: Clean refactoring to config-driven SBOM handling.The SBOM validation logic has been successfully refactored to use the new
ScanningServiceConfig. The validation remains consistent with the previous implementation while improving code organization.
164-209: Well-structured config extraction logic.The function cleanly handles form values, header fallbacks, and base64-decoded scan settings. The configuration flow is logical and the error handling is appropriate.
228-228: Consistent signature updates across all scanning functions.All scanning functions have been successfully updated to accept the new
ScanningServiceConfigparameter. The signatures are consistent and all call sites have been properly updated.Also applies to: 248-248, 340-340, 379-379
pkg/service/scanning_service_config.go (1)
40-54: Clean initialization from server config.The function correctly initializes a
ScanningServiceConfigfrom the server's default configuration, mapping all relevant fields appropriately.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @pkg/service/scanning_service_config.go:
- Around line 98-109: Add a nil check before dereferencing currentConfig: if
currentConfig == nil { s.Errorf("nil currentConfig passed to update scanning
service config") ; return ScanningServiceConfig{} } so you avoid the panic when
doing updatedConfig := *currentConfig; then proceed to create updatedConfig :=
*currentConfig and continue with unmarshalling inputSettings into newSettings.
Ensure you reference currentConfig and updatedConfig exactly as in the diff and
keep the existing error log style (s.Errorf).
🧹 Nitpick comments (1)
pkg/service/scanning_service_config.go (1)
41-55: Consider adding nil check for defensive programming.If
serverDefaultConfigisnil, this function will panic. While this may be controlled by callers, a nil check would provide better resilience and clearer error messaging.🔎 Proposed defensive check
func DefaultScanningServiceConfig(serverDefaultConfig *cfg.ServerConfig) ScanningServiceConfig { + if serverDefaultConfig == nil { + return ScanningServiceConfig{} + } return ScanningServiceConfig{ flags: serverDefaultConfig.Scanning.ScanFlags,
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Makefilepkg/service/scanning_service_config.go
🔇 Additional comments (6)
Makefile (2)
36-39: LGTM!The new
unit_test_covertarget correctly mirrors the existingunit_testtarget with the addition of the-coverflag for coverage reporting. The implementation is consistent with the existing Makefile conventions.
74-79: LGTM!The
e2e_test_covertarget correctly follows the same docker-compose workflow ase2e_testwhile adding coverage reporting. The structure is clean and consistent.pkg/service/scanning_service_config.go (4)
27-39: LGTM!The struct design with unexported fields provides good encapsulation. The field types are appropriate for their purposes.
154-162: Error handling is inconsistent but acceptable.JSON unmarshal errors (line 107) cause an early return, while flags conversion errors (line 157) only log and continue processing other fields. This difference is reasonable—a malformed JSON blob invalidates all settings, whereas a bad flags value shouldn't prevent other valid updates from applying.
If you prefer consistent behavior, consider documenting this explicitly or applying the same "continue on error" approach to JSON parsing for partial updates.
111-147: LGTM!The conditional updates with pointer nil checks correctly distinguish between "not provided" and "zero value". The ranking settings are properly guarded by
rankingAllowedwith appropriate warning logs when ignored.
164-172: LGTM!The sbomType and sbomFile updates are straightforward with proper debug logging.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test-support/scanoss.sh (1)
32-32: Consider writing error to stderr for consistency.Line 65 writes its error message to stderr (
>&2), but this error message goes to stdout. While not critical for a test script, directing all error messages to stderr would improve consistency.♻️ Optional improvement
- echo "Error: Invalid MD5 hash format: $md5" + echo "Error: Invalid MD5 hash format: $md5" >&2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test-support/scanoss.sh
🔇 Additional comments (2)
test-support/scanoss.sh (2)
30-34: LGTM! MD5 validation logic is correct.The regex pattern correctly validates that MD5 hashes are exactly 32 hexadecimal characters, and the error handling appropriately exits with a non-zero status for invalid inputs.
59-68: LGTM! KB name validation logic is sound.The validation correctly enforces that the KB name must be attached to the
-nflag (e.g.,-nMyKBrather than-n MyKB). By checking for standalone-n, the code effectively ensures a non-empty KB name is provided, since any attached name would make the argument not equal to-n.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @pkg/service/scanning_service_config.go:
- Line 101: Update the fmt.Errorf messages that start with a capitalized
"Default" to use a lowercase "default" to follow Go error string conventions
(ST1005); locate the fmt.Errorf call returning ScanningServiceConfig{} (the
message "Default server scanning service config is undefined") and the similar
fmt.Errorf at the other occurrence and change their messages to start with
"default" instead.
In @pkg/service/scanning_service.go:
- Line 202: The error string returned when decoding scan settings is
capitalized; change the fmt.Errorf call that currently uses "Error decoding scan
settings from base64: %v" to use a lowercase leading word (e.g., "error decoding
scan settings from base64: %v") so it follows Go ST1005 conventions; locate the
return that returns scanConfig and fmt.Errorf in scanning_service.go (the
decode/parse routine that handles base64 scan settings) and update the message
accordingly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/service/scanning_service.gopkg/service/scanning_service_config.gopkg/service/scanning_service_config_test.gopkg/service/scanning_service_test.gotest-support/scanoss.shtests/scanning_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/service/scanning_service_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/service/scanning_service_config_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/service/scanning_service_config.go (1)
pkg/config/server_config.go (1)
ServerConfig(37-94)
🪛 GitHub Check: build
pkg/service/scanning_service_config.go
[failure] 110-110:
ST1005: error strings should not be capitalized (stylecheck)
[failure] 101-101:
ST1005: error strings should not be capitalized (stylecheck)
pkg/service/scanning_service.go
[failure] 202-202:
ST1005: error strings should not be capitalized (stylecheck)
🔇 Additional comments (15)
test-support/scanoss.sh (3)
30-34: LGTM: MD5 validation is correct.The regex pattern properly validates MD5 hash format (32 hexadecimal characters), and the error handling with exit code 1 is appropriate for test simulation.
59-68: LGTM: KB name validation is correct.The validation properly detects the bare
-nflag without an attached KB name and provides a clear error message. This aligns with the expected engine behavior.
71-79: LGTM: -w flag detection works correctly.The loop properly detects the
-wflag anywhere in the arguments and produces appropriate simulated output for test purposes.tests/scanning_test.go (3)
93-93: LGTM: Assets payload format improved.The change from plain PURL strings to structured JSON with a
componentsarray is a good improvement that aligns with standard SBOM formats like CycloneDX and SPDX.Also applies to: 100-100
136-222: LGTM: Comprehensive test coverage for scan settings header.The new
TestScanSettingsHeadertest function provides excellent coverage:
- Valid multi-parameter scan settings with proper base64 encoding
- Invalid base64 handling with appropriate error response
- Integration with legacy flags parameter
The inline comments documenting the decoded JSON payloads enhance readability and maintainability.
46-52: Test expectation is well-documented and appears correct.The test change from
http.StatusInternalServerErrortohttp.StatusOKis clearly explained by the inline comment: the engine now handles invalid KB names gracefully with fallback behavior. The API correctly passes thedb_nameparameter through to the scanning engine without validation—this is the appropriate responsibility boundary since the engine itself determines whether the KB name is valid. Debug logging is already in place for tracking DB name configuration updates.pkg/service/scanning_service_config.go (3)
28-40: LGTM: Well-structured configuration type.The
ScanningServiceConfigstruct has a comprehensive set of fields for scanning configuration, and the use of unexported fields is appropriate for internal service configuration.
42-56: LGTM: Proper default configuration initialization.The function correctly initializes the scanning configuration from server defaults while appropriately leaving per-request fields (sbomType, sbomFile) empty.
87-167: LGTM: Well-implemented configuration update logic.The function properly:
- Checks for nil config and returns an error
- Creates a copy to avoid modifying the original
- Gates ranking updates based on
rankingAllowedwith appropriate warnings- Handles JSON unmarshalling errors
- Validates and converts flags with error handling
The immutable update pattern (copy, modify, return) is a good design choice.
pkg/service/scanning_service.go (6)
40-43: LGTM: SBOM type constants improve code clarity.The constants eliminate magic strings and make the code more maintainable. As noted in past reviews, these represent the two SBOM scan types supported by the engine: "identify" (include matching components) and "blacklist" (exclude matching components).
95-120: LGTM: Proper config extraction and error handling.The refactored code correctly:
- Obtains scanning configuration from the request
- Handles configuration errors with appropriate HTTP status
- Validates SBOM types using the new constants
- Uses config fields consistently
169-208: LGTM: Well-implemented config extraction with proper fallbacks.The function correctly:
- Extracts scanning parameters from both form values and headers
- Implements a proper fallback mechanism
- Decodes and validates base64-encoded scan settings
- Returns errors to the caller for proper HTTP error responses
- Guards trace logging appropriately
The past review concern about else-if style has been addressed.
227-227: LGTM: Consistent config-driven refactoring.The updated function signatures consistently use
ScanningServiceConfiginstead of multiple individual parameters, resulting in cleaner, more maintainable code across all scanning functions.Also applies to: 247-247, 339-339, 378-378
398-446: LGTM: Well-structured argument building logic.The command argument construction is well-organized:
- Consistent flag formatting for single-character flags (
-n%s,-F%v,-r%d)- Proper use of long-form flags with equals signs for snippet parameters
- Ranking threshold correctly gated by both
rankingEnabledand positive threshold value- SBOM configuration properly uses constants with switch statement
The past review concern about flag format consistency has been addressed.
447-450: Verify--ignore-file-extflag support before release.As flagged in a previous review, the
--ignore-file-extflag may not be supported by the SCANOSS engine. The comment on line 447 states "(not yet implemented in scanoss engine)". This creates a risk:
- If the flag is not recognized, the scan command will fail with "Unknown command option"
- The simulator script (test-support/scanoss.sh) doesn't handle this flag, so E2E tests won't catch the issue
Run the following script to check if the flag is handled by the test simulator and verify its usage in the real engine:
#!/bin/bash # Check if --ignore-file-ext is handled in the simulator echo "=== Check test simulator for --ignore-file-ext handling ===" grep -n "ignore-file-ext" test-support/scanoss.sh echo "" echo "=== Check for any documentation or comments about this flag ===" rg -n "ignore-file-ext|honourFileExts" --type=go -B 2 -A 2 echo "" echo "=== Check if there's version detection for this flag ===" rg -n "version|engine.*version" pkg/service/scanning_service.go -B 2 -A 2 | grep -i "detect\|check"If the flag is not yet supported, consider:
- Remove lines 447-450 until engine support is confirmed
- Add version detection to conditionally pass the flag
- Update the test simulator to handle/ignore this flag gracefully
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/service/scanning_service.go (1)
428-430: Consider addingrankingAllowedcheck for defensive coding.While
UpdateScanningServiceConfigDTOprevents clients from enabling ranking whenrankingAllowedis false, the condition here doesn't verifyrankingAllowed. If the server configuration itself is misconfigured (e.g.,rankingAllowed=falsebutrankingEnabled=true), the ranking flag would still be passed to the engine.For defensive coding, consider changing the condition to:
if config.rankingAllowed && config.rankingEnabled && config.rankingThreshold > 0 {This ensures the flag is only passed when ranking is both allowed and enabled.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/service/scanning_service.gopkg/service/scanning_service_config.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/service/scanning_service_config.go (1)
pkg/config/server_config.go (1)
ServerConfig(37-94)
pkg/service/scanning_service.go (2)
pkg/service/utils_service.go (1)
APIService(71-73)pkg/service/scanning_service_config.go (3)
ScanningServiceConfig(28-40)DefaultScanningServiceConfig(42-56)UpdateScanningServiceConfigDTO(87-167)
🔇 Additional comments (5)
pkg/service/scanning_service_config.go (1)
1-167: LGTM! Well-structured config management implementation.The file introduces a clean, immutable configuration update pattern. All past review concerns have been addressed:
- Nil check prevents panics
- Error strings follow Go conventions (lowercase)
- JSON field names match correctly
- Comprehensive documentation explains the immutable copy semantics
The conditional logic properly restricts ranking modifications when
rankingAllowedis false, and the logging provides good observability.pkg/service/scanning_service.go (4)
40-43: Good use of constants for SBOM type checking.Using
sbomIdentifyandsbomBlackListconstants eliminates string literal repetition and reduces the risk of typos when checking SBOM types throughout the code.
169-208: LGTM! Clean config extraction with proper error handling.The function correctly extracts configuration from both form values and headers, with appropriate fallback logic. The base64 decoding error handling follows Go conventions, and all past review concerns have been addressed.
104-119: LGTM! Proper SBOM validation.The validation correctly uses the new constants and ensures only valid SBOM types (
identifyorblacklist) are accepted before proceeding with scan operations.
404-445: LGTM! Consistent flag formatting and conditional logic.The command argument construction properly:
- Uses consistent formatting:
-n%s,-F%v,-r%d(no spaces) and--long-form=%d(with equals)- Applies conditional checks to only add flags when values are positive
- Handles SBOM flags correctly with the switch statement
| if flags != "" { | ||
| flagsInt, err := strconv.Atoi(flags) | ||
| if err != nil { | ||
| s.Errorf("Error converting flags to integer: %v", err) |
There was a problem hiding this comment.
do you want this to fail or just ignore it?
There was a problem hiding this comment.
It´s not failing, this is just a log, right?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CHANGELOG.md (1)
11-21: Consider consistent spelling forhonourFileExts.The parameter name uses British English spelling (
honour). Ensure this spelling is consistent across the entire codebase (code, configuration, and documentation) to avoid confusion. If the codebase uses American English elsewhere, consider standardizing tohonorFileExts.pkg/service/scanning_service_config.go (1)
87-179: Refactor to reduce cognitive complexity and verify validation logic.The function has high cognitive complexity (32 > 25) due to multiple conditional branches. Consider extracting validation logic into separate helper functions.
Additionally, verify the validation inconsistency:
MinSnippetHitsallows>= 0(Line 130)MinSnippetLinesrequires> 0(Line 138)SnippetRangeToleranceallows>= 0(Line 146)Are these different bounds intentional? If so, document why
MinSnippetHitscan be 0 butMinSnippetLinescannot. If not, standardize the validation logic.♻️ Example refactoring to reduce complexity
Extract validation into helper functions:
func validateMinSnippetHits(value int) bool { return value >= 0 } func validateMinSnippetLines(value int) bool { return value > 0 } func validateSnippetRangeTolerance(value int) bool { return value >= 0 } func (c *ScanningServiceConfig) applyRankingSetting(s *zap.SugaredLogger, enabled *bool, threshold *int) { if !c.rankingAllowed { s.Warnf("Ranking settings ignored as RankingAllowed is false") return } if enabled != nil { c.rankingEnabled = *enabled s.Debugf("Updated RankingEnabled to %v", c.rankingEnabled) } if threshold != nil { c.rankingThreshold = *threshold s.Debugf("Updated RankingThreshold to %d", c.rankingThreshold) } }Then simplify the main function by calling these helpers.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.mdMakefilepkg/service/scanning_service.gopkg/service/scanning_service_config.go
🧰 Additional context used
🪛 GitHub Check: build
pkg/service/scanning_service_config.go
[failure] 87-87:
cognitive complexity 32 of func UpdateScanningServiceConfigDTO is high (> 25) (gocognit)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~20-~20: Ensure spelling is correct
Context: ...inSnippetLines - snippetRangeTolerance - honourFileExts ## [1.5.2] - 2025-11-07 ### Added - Added Cu...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (9)
Makefile (1)
36-38: LGTM! Coverage test targets properly implemented.The new coverage targets correctly extend the existing test workflow by adding the
-coverflag to unit, integration, and end-to-end tests. The implementation follows the existing patterns and provides explicit coverage reporting without modifying the base test targets.Also applies to: 44-46, 78-83
CHANGELOG.md (1)
176-176: LGTM! Version link properly added.The version comparison link is correctly formatted and follows the existing pattern.
pkg/service/scanning_service_config.go (2)
28-40: LGTM! Well-structured configuration type.The
ScanningServiceConfigstruct is well-organized with unexported fields appropriate for internal service use. The logical grouping of related fields (SBOM, ranking, snippet settings) enhances readability.
42-56: LGTM! Default configuration properly initialized.The function correctly initializes all configuration fields from the server defaults, with appropriate empty string defaults for SBOM-related fields that are request-specific.
pkg/service/scanning_service.go (5)
40-43: LGTM! SBOM constants improve code clarity.The SBOM type constants enhance code readability and reduce the risk of typos in string comparisons throughout the codebase.
95-100: LGTM! Proper error handling for configuration.The error handling for invalid scanning configuration is appropriate and provides clear feedback to the client with a 400 Bad Request status.
168-207: LGTM! Config extraction well-implemented with proper fallbacks.The function correctly:
- Extracts parameters from form values with header fallbacks
- Decodes base64-encoded scan settings with appropriate error handling
- Creates default config and applies updates via the DTO pattern
- Provides comprehensive debug logging when trace is enabled
398-444: LGTM! Config-driven command building properly implemented.The refactored command-building logic correctly derives all scanning options from the
ScanningServiceConfig:
- Database name, flags, and SBOM handling
- Ranking controls with proper permission checks
- Snippet threshold parameters
- File extension handling
The implementation maintains backward compatibility while enabling flexible per-request configuration.
439-441: Clarify the status of the--ignore-file-extflag support in the SCANOSS engine.The code adds the
--ignore-file-extflag based on thehonourFileExtsconfiguration (lines 440-441), but the comment explicitly states this feature is "not yet implemented in scanoss engine." This creates ambiguity: either the comment is outdated and the engine now supports this flag, or the configuration option has no actual effect.Please confirm:
- Is
--ignore-file-extcurrently supported by the SCANOSS engine?- If not supported, remove this conditional logic or add a note explaining it's a placeholder for future functionality.
- If now supported, remove or update the "not yet implemented" comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @pkg/service/scanning_service_config.go:
- Around line 42-56: DefaultScanningServiceConfig currently dereferences
serverDefaultConfig and will panic if it's nil; add a nil guard at the start of
DefaultScanningServiceConfig (similar to UpdateScanningServiceConfigDTO) that
returns a safe ScanningServiceConfig (zero-values or explicit defaults) when
serverDefaultConfig is nil, otherwise proceed to read fields like
serverDefaultConfig.Scanning.ScanFlags, ScanKbName, RankingAllowed,
RankingEnabled, RankingThreshold, MinSnippetHits, MinSnippetLines,
SnippetRangeTol and HonourFileExts to populate the ScanningServiceConfig struct.
🧹 Nitpick comments (1)
pkg/service/scanning_service_config.go (1)
119-142: Consider returning validation errors for consistency.Unlike
applySnippetSettingswhich returns invalid settings, this function silently logs and ignores invalidflagsvalues. The caller has no indication that the flags update was skipped. Consider returning validation errors for consistency.♻️ Optional improvement
-func applyDirectParameters(s *zap.SugaredLogger, config *ScanningServiceConfig, flags, scanType, sbom, dbName string) { +func applyDirectParameters(s *zap.SugaredLogger, config *ScanningServiceConfig, flags, scanType, sbom, dbName string) []string { + var invalidParams []string if dbName != "" { config.dbName = dbName s.Debugf("Updated DbName to %s", config.dbName) } if flags != "" { flagsInt, err := strconv.Atoi(flags) if err == nil { config.flags = flagsInt s.Debugf("Updated Flags to %d", config.flags) } else { s.Errorf("Error converting flags to integer: %v", err) + invalidParams = append(invalidParams, fmt.Sprintf("flags: %s", flags)) } } // ... rest unchanged + return invalidParams }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/service/scanning_service_config.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/service/scanning_service_config.go (1)
pkg/config/server_config.go (1)
ServerConfig(37-94)
🔇 Additional comments (6)
pkg/service/scanning_service_config.go (6)
1-26: LGTM!License header and imports are appropriate. All imported packages are used in the implementation.
28-40: LGTM!The struct with unexported fields provides good encapsulation. The fields correctly mirror the server-side scanning configuration from
ServerConfig.Scanning.
58-66: LGTM!Good use of pointer types for optional JSON fields, enabling proper distinction between unset values and zero values.
68-83: LGTM!Proper guard logic ensures ranking settings are only applied when
rankingAllowedis true. The warning log provides good visibility when settings are ignored.
85-117: LGTM!Validation logic is sensible:
MinSnippetHitsandSnippetRangeToleranceallow zero (disabling the constraint), whileMinSnippetLinesrequires at least 1 line. Returning invalid settings for logging is a clean pattern.
144-196: LGTM!Well-documented function with clear immutability guarantees. The nil check, copy-before-modify pattern, and delegated helper functions provide a clean and safe implementation. Error handling for JSON unmarshalling correctly returns the unmodified copy.
| func DefaultScanningServiceConfig(serverDefaultConfig *cfg.ServerConfig) ScanningServiceConfig { | ||
| return ScanningServiceConfig{ | ||
| flags: serverDefaultConfig.Scanning.ScanFlags, | ||
| sbomType: "", | ||
| sbomFile: "", | ||
| dbName: serverDefaultConfig.Scanning.ScanKbName, | ||
| rankingAllowed: serverDefaultConfig.Scanning.RankingAllowed, | ||
| rankingEnabled: serverDefaultConfig.Scanning.RankingEnabled, | ||
| rankingThreshold: serverDefaultConfig.Scanning.RankingThreshold, | ||
| minSnippetHits: serverDefaultConfig.Scanning.MinSnippetHits, | ||
| minSnippetLines: serverDefaultConfig.Scanning.MinSnippetLines, | ||
| snippetRangeTolerance: serverDefaultConfig.Scanning.SnippetRangeTol, | ||
| honourFileExts: serverDefaultConfig.Scanning.HonourFileExts, | ||
| } | ||
| } |
There was a problem hiding this comment.
Add nil check for serverDefaultConfig parameter.
If serverDefaultConfig is nil, this function will panic with a nil pointer dereference. Consider adding a guard similar to what's done in UpdateScanningServiceConfigDTO.
🛡️ Proposed fix
func DefaultScanningServiceConfig(serverDefaultConfig *cfg.ServerConfig) ScanningServiceConfig {
+ if serverDefaultConfig == nil {
+ return ScanningServiceConfig{}
+ }
return ScanningServiceConfig{
flags: serverDefaultConfig.Scanning.ScanFlags,📝 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.
| func DefaultScanningServiceConfig(serverDefaultConfig *cfg.ServerConfig) ScanningServiceConfig { | |
| return ScanningServiceConfig{ | |
| flags: serverDefaultConfig.Scanning.ScanFlags, | |
| sbomType: "", | |
| sbomFile: "", | |
| dbName: serverDefaultConfig.Scanning.ScanKbName, | |
| rankingAllowed: serverDefaultConfig.Scanning.RankingAllowed, | |
| rankingEnabled: serverDefaultConfig.Scanning.RankingEnabled, | |
| rankingThreshold: serverDefaultConfig.Scanning.RankingThreshold, | |
| minSnippetHits: serverDefaultConfig.Scanning.MinSnippetHits, | |
| minSnippetLines: serverDefaultConfig.Scanning.MinSnippetLines, | |
| snippetRangeTolerance: serverDefaultConfig.Scanning.SnippetRangeTol, | |
| honourFileExts: serverDefaultConfig.Scanning.HonourFileExts, | |
| } | |
| } | |
| func DefaultScanningServiceConfig(serverDefaultConfig *cfg.ServerConfig) ScanningServiceConfig { | |
| if serverDefaultConfig == nil { | |
| return ScanningServiceConfig{} | |
| } | |
| return ScanningServiceConfig{ | |
| flags: serverDefaultConfig.Scanning.ScanFlags, | |
| sbomType: "", | |
| sbomFile: "", | |
| dbName: serverDefaultConfig.Scanning.ScanKbName, | |
| rankingAllowed: serverDefaultConfig.Scanning.RankingAllowed, | |
| rankingEnabled: serverDefaultConfig.Scanning.RankingEnabled, | |
| rankingThreshold: serverDefaultConfig.Scanning.RankingThreshold, | |
| minSnippetHits: serverDefaultConfig.Scanning.MinSnippetHits, | |
| minSnippetLines: serverDefaultConfig.Scanning.MinSnippetLines, | |
| snippetRangeTolerance: serverDefaultConfig.Scanning.SnippetRangeTol, | |
| honourFileExts: serverDefaultConfig.Scanning.HonourFileExts, | |
| } | |
| } |
🤖 Prompt for AI Agents
In @pkg/service/scanning_service_config.go around lines 42 - 56,
DefaultScanningServiceConfig currently dereferences serverDefaultConfig and will
panic if it's nil; add a nil guard at the start of DefaultScanningServiceConfig
(similar to UpdateScanningServiceConfigDTO) that returns a safe
ScanningServiceConfig (zero-values or explicit defaults) when
serverDefaultConfig is nil, otherwise proceed to read fields like
serverDefaultConfig.Scanning.ScanFlags, ScanKbName, RankingAllowed,
RankingEnabled, RankingThreshold, MinSnippetHits, MinSnippetLines,
SnippetRangeTol and HonourFileExts to populate the ScanningServiceConfig struct.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @pkg/service/scanning_service.go:
- Around line 95-100: getConfigFromRequest can return detailed errors but the
caller drops err; update the error path after calling getConfigFromRequest in
scanning_service.go to log the actual err before returning the 400.
Specifically, when err != nil, write the error details to the service logger
(e.g., using s.logger or a structured log call) and include the err text in the
tracing error via setSpanError(span, ...) in addition to the existing http.Error
call so the actual parsing/validation failure is recorded for debugging.
- Around line 423-426: The condition that appends the ranking flag only checks
config.rankingEnabled and config.rankingThreshold but omits
config.rankingAllowed; update the if in the block that builds args (the code
referencing args and fmt.Sprintf("-r%d", config.rankingThreshold)) to also
require config.rankingAllowed so it becomes enabled && allowed && threshold>0,
ensuring ScanningServiceConfig.rankingAllowed is enforced when deciding to
append the ranking argument.
- Around line 168-207: In getConfigFromRequest: stop logging r.Header and r.Form
directly—replace that Debugf with a minimal, non-sensitive summary (e.g., log
presence/lengths or whitelist keys) and explicitly redact Authorization/Cookie;
do not log the decoded settings string (remove zs.Debugf("Decoded scan settings:
%s"...)). Add a header size guard before decoding Scanoss-Settings (e.g., reject
or truncate if len(scanSettings) exceeds a safe threshold) and continue using
base64.StdEncoding for decoding; finally, ensure UpdateScanningServiceConfigDTO
and DefaultScanningServiceConfig usage remains unchanged and return errors as
currently done.
- Around line 398-442: The scan argument builder appends flags (-r,
--min-snippet-hits, --min-snippet-lines, --range-tolerance, --ignore-file-ext)
without checking engine support, causing failures; update the builder to consult
explicit support booleans on the config (add EngineSupportsRanking,
EngineSupportsSnippetFlags, EngineSupportsIgnoreFileExt to the config used by
the function) and only append ranking flags when config.EngineSupportsRanking &&
config.rankingEnabled, only append snippet-related flags when
config.EngineSupportsSnippetFlags && (config.minSnippetHits > 0 ||
config.minSnippetLines > 0 || config.snippetRangeTolerance > 0), and only append
"--ignore-file-ext" when !config.honourFileExts &&
config.EngineSupportsIgnoreFileExt; ensure any new fields are set based on the
tracked engineVersion before this function is called and default to false (omit
flags) if support is unknown.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/service/scanning_service.gotests/scanning_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/scanning_test.go
🔇 Additional comments (6)
pkg/service/scanning_service.go (6)
3-3: Copyright header year bump is fine.
22-23: Newencoding/base64import is appropriate forScanoss-Settings.
40-43: SBOM type constants reduce stringly-typed usage—good.
103-119: SBOM validation + temp-file lifecycle look correct. Nice to see type validation before writing and conditional cleanup viaTmpFileDelete.
135-138: Config threading/plumbing is consistent across scan paths. Signatures and call sites line up cleanly (single-threaded + worker model).Also applies to: 226-229, 246-247, 263-264, 338-339, 352-353, 377-377
393-396: Good: close the temp file on write error. Prevents fd leaks on early returns.
| scanConfig, err := s.getConfigFromRequest(r, zs) | ||
| if err != nil { | ||
| http.Error(w, "ERROR invalid scanning configuration", http.StatusBadRequest) | ||
| setSpanError(span, "Invalid scanning configuration.") | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
Log the config parsing error before returning 400. getConfigFromRequest can fail for multiple reasons, but the caller currently drops err (harder to debug).
Proposed tweak
scanConfig, err := s.getConfigFromRequest(r, zs)
if err != nil {
+ zs.Warnf("Invalid scanning configuration: %v", err)
http.Error(w, "ERROR invalid scanning configuration", http.StatusBadRequest)
setSpanError(span, "Invalid scanning configuration.")
return 0
}📝 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.
| scanConfig, err := s.getConfigFromRequest(r, zs) | |
| if err != nil { | |
| http.Error(w, "ERROR invalid scanning configuration", http.StatusBadRequest) | |
| setSpanError(span, "Invalid scanning configuration.") | |
| return 0 | |
| } | |
| scanConfig, err := s.getConfigFromRequest(r, zs) | |
| if err != nil { | |
| zs.Warnf("Invalid scanning configuration: %v", err) | |
| http.Error(w, "ERROR invalid scanning configuration", http.StatusBadRequest) | |
| setSpanError(span, "Invalid scanning configuration.") | |
| return 0 | |
| } |
🤖 Prompt for AI Agents
In @pkg/service/scanning_service.go around lines 95 - 100, getConfigFromRequest
can return detailed errors but the caller drops err; update the error path after
calling getConfigFromRequest in scanning_service.go to log the actual err before
returning the 400. Specifically, when err != nil, write the error details to the
service logger (e.g., using s.logger or a structured log call) and include the
err text in the tracing error via setSpanError(span, ...) in addition to the
existing http.Error call so the actual parsing/validation failure is recorded
for debugging.
| // getConfigFromRequest extracts the form values from a request and returns the scanning configuration. | ||
| func (s APIService) getConfigFromRequest(r *http.Request, zs *zap.SugaredLogger) (ScanningServiceConfig, error) { | ||
| flags := strings.TrimSpace(r.FormValue("flags")) // Check form for scanning flags | ||
| scanType := strings.TrimSpace(r.FormValue("type")) // Check form for SBOM type | ||
| sbom := strings.TrimSpace(r.FormValue("assets")) // Check form for SBOM contents | ||
| dbName := strings.TrimSpace(r.FormValue("db_name")) // Check form for db name | ||
| // TODO is it necessary to check the header also for these values? | ||
| // Fall back to headers if form values are empty | ||
| if len(flags) == 0 { | ||
| flags = strings.TrimSpace(r.Header.Get("flags")) // Check header for Scanning flags | ||
| flags = strings.TrimSpace(r.Header.Get("flags")) | ||
| } | ||
| if len(scanType) == 0 { | ||
| scanType = strings.TrimSpace(r.Header.Get("type")) // Check header for SBOM type | ||
| scanType = strings.TrimSpace(r.Header.Get("type")) | ||
| } | ||
| if len(sbom) == 0 { | ||
| sbom = strings.TrimSpace(r.Header.Get("assets")) // Check header for SBOM contents | ||
| sbom = strings.TrimSpace(r.Header.Get("assets")) | ||
| } | ||
| if len(dbName) == 0 { | ||
| dbName = strings.TrimSpace(r.Header.Get("db_name")) // Check header for SBOM contents | ||
| dbName = strings.TrimSpace(r.Header.Get("db_name")) | ||
| } | ||
| scanSettings := strings.TrimSpace(r.Header.Get("Scanoss-Settings")) // Check header for scan settings | ||
| if s.config.App.Trace { | ||
| zs.Debugf("Header: %v, Form: %v, flags: %v, type: %v, assets: %v, db_name %v", r.Header, r.Form, flags, scanType, sbom, dbName) | ||
| 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) | ||
| } | ||
| // Create default configuration from server config | ||
| scanConfig := DefaultScanningServiceConfig(s.config) | ||
| // Decode scan settings from base64 if provided | ||
| var decoded []byte | ||
| if len(scanSettings) > 0 { | ||
| var err error | ||
| decoded, err = base64.StdEncoding.DecodeString(scanSettings) | ||
| if err != nil { | ||
| zs.Errorf("Error decoding scan settings from base64: %v", err) | ||
| return scanConfig, fmt.Errorf("error decoding scan settings from base64: %v", err) | ||
| } else if s.config.App.Trace { | ||
| zs.Debugf("Decoded scan settings: %s", string(decoded)) | ||
| } | ||
| } | ||
| return flags, scanType, sbom, dbName | ||
| return UpdateScanningServiceConfigDTO(zs, &scanConfig, flags, scanType, sbom, dbName, decoded) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
scanoss client Scanoss-Settings header base64 encoding format padding
💡 Result:
Short answer: SCANOSS docs and public client code don't document the exact byte/charset or base64 variant used for the Scanoss-Settings HTTP header. The safe assumptions and recommended checks are below.
What I could verify
- Official SCANOSS docs describe the settings JSON (scanoss.json) but do not specify how it is encoded into a Scanoss-Settings HTTP header. [1][2].
- The SCANOSS Python client repository is public (scanoss.py) — you should inspect the code that builds HTTP headers to see how it encodes settings. [3].
Recommended actions
- Inspect your client code: search the client repo (or your installed package) for where "Scanoss-Settings" is set — that code will show whether it uses:
- standard Base64 (RFC 4648) with '=' padding, or
- URL-safe base64 ( '-' and '_' ) and whether it strips padding.
RFC 4648 explains the standard/base64 padding behavior. [4]
- Capture a request (curl, tcpdump, browser DevTools, mitm) from your client and inspect the Scanoss-Settings header value to confirm the exact encoding and whether '=' padding is present.
- If you want me to check the client source for you, tell me which client (scanoss.py, scanoss-java, scanoss.js, or a specific version) and I will search the repository and point to the exact file/line that encodes the header.
Sources
- SCANOSS settings documentation (settings JSON; no header encoding info). [1]
- SCANOSS project / scanoss.py GitHub repository. [3]
- RFC 4648 — The Base16, Base32, and Base64 Data Encodings (describes padding). [4]
References
[1] https://scanoss.readthedocs.io/projects/scanoss-py/en/latest/
[2] https://scanoss.readthedocs.io/projects/settings-file-docs/en/latest/
[3] https://github.com/scanoss/scanoss.py
[4] https://tools.ietf.org/html/rfc4648
Would you like me to search the scanoss client repo for the exact header-encoding code (and return the file/line)?
🏁 Script executed:
# Search for Scanoss-Settings usage in the codebase
rg "Scanoss-Settings" -iRepository: scanoss/api.go
Length of output: 349
🏁 Script executed:
# Search for base64 decoding patterns in the codebase
rg "base64\." --type go -B 2 -A 5Repository: scanoss/api.go
Length of output: 682
🏁 Script executed:
# Look for scanning_service tests
fd "scanning_service" -type fRepository: scanoss/api.go
Length of output: 228
🏁 Script executed:
# Check the scanning_service.go file for related functions
rg "ScanningServiceConfig|UpdateScanningServiceConfigDTO" -A 3 -B 1Repository: scanoss/api.go
Length of output: 16023
🏁 Script executed:
# Look at the scanning test to see how scanSettingsB64 is created
cat -n tests/scanning_test.go | head -100Repository: scanoss/api.go
Length of output: 3940
🏁 Script executed:
# Search for scanSettingsB64 initialization
rg "scanSettingsB64" -B 5 -A 5Repository: scanoss/api.go
Length of output: 2942
🏁 Script executed:
# Look for any size limit configuration or constants
rg "1024|8192|16\*1024|65536|HeaderSize|MaxHeader" --type goRepository: scanoss/api.go
Length of output: 120
🏁 Script executed:
# Check if there are other places in the codebase that handle base64 decoding
rg "base64\.(Std|Raw|URLEncoding)" --type go -B 2 -A 5Repository: scanoss/api.go
Length of output: 682
Avoid logging full headers/forms; remove decoded settings logging; add header size guard. Current trace log (lines 189–190) prints r.Header and r.Form verbatim, which can leak credentials (Authorization/Cookie headers) into logs. Additionally, logging decoded settings as a string (line 197) can expose sensitive configuration JSON. No documented header size limit exists, so a guard is recommended.
The base64 encoding format is not documented in official SCANOSS specs, but test cases confirm the server currently expects only base64.StdEncoding with padding; no evidence of raw or URL-safe variants is present in the codebase.
Safer logging without full headers + size guard
scanSettings := strings.TrimSpace(r.Header.Get("Scanoss-Settings")) // Check header for scan settings
if s.config.App.Trace {
- 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.Debugf("flags: %v, type: %v, assets_len: %d, db_name: %v, scanSettings_present: %t, scanSettings_len: %d",
+ flags, scanType, len(sbom), dbName, len(scanSettings) > 0, len(scanSettings))
}
// Create default configuration from server config
scanConfig := DefaultScanningServiceConfig(s.config)
// Decode scan settings from base64 if provided
var decoded []byte
if len(scanSettings) > 0 {
+ // Prevent pathologically large headers (tune limit as needed)
+ if len(scanSettings) > 16*1024 {
+ return scanConfig, fmt.Errorf("Scanoss-Settings header too large")
+ }
var err error
decoded, err = base64.StdEncoding.DecodeString(scanSettings)
if err != nil {
zs.Errorf("Error decoding scan settings from base64: %v", err)
return scanConfig, fmt.Errorf("error decoding scan settings from base64: %v", err)
- } else if s.config.App.Trace {
- zs.Debugf("Decoded scan settings: %s", string(decoded))
}
}
return UpdateScanningServiceConfigDTO(zs, &scanConfig, flags, scanType, sbom, dbName, decoded)📝 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.
| // getConfigFromRequest extracts the form values from a request and returns the scanning configuration. | |
| func (s APIService) getConfigFromRequest(r *http.Request, zs *zap.SugaredLogger) (ScanningServiceConfig, error) { | |
| flags := strings.TrimSpace(r.FormValue("flags")) // Check form for scanning flags | |
| scanType := strings.TrimSpace(r.FormValue("type")) // Check form for SBOM type | |
| sbom := strings.TrimSpace(r.FormValue("assets")) // Check form for SBOM contents | |
| dbName := strings.TrimSpace(r.FormValue("db_name")) // Check form for db name | |
| // TODO is it necessary to check the header also for these values? | |
| // Fall back to headers if form values are empty | |
| if len(flags) == 0 { | |
| flags = strings.TrimSpace(r.Header.Get("flags")) // Check header for Scanning flags | |
| flags = strings.TrimSpace(r.Header.Get("flags")) | |
| } | |
| if len(scanType) == 0 { | |
| scanType = strings.TrimSpace(r.Header.Get("type")) // Check header for SBOM type | |
| scanType = strings.TrimSpace(r.Header.Get("type")) | |
| } | |
| if len(sbom) == 0 { | |
| sbom = strings.TrimSpace(r.Header.Get("assets")) // Check header for SBOM contents | |
| sbom = strings.TrimSpace(r.Header.Get("assets")) | |
| } | |
| if len(dbName) == 0 { | |
| dbName = strings.TrimSpace(r.Header.Get("db_name")) // Check header for SBOM contents | |
| dbName = strings.TrimSpace(r.Header.Get("db_name")) | |
| } | |
| scanSettings := strings.TrimSpace(r.Header.Get("Scanoss-Settings")) // Check header for scan settings | |
| if s.config.App.Trace { | |
| zs.Debugf("Header: %v, Form: %v, flags: %v, type: %v, assets: %v, db_name %v", r.Header, r.Form, flags, scanType, sbom, dbName) | |
| 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) | |
| } | |
| // Create default configuration from server config | |
| scanConfig := DefaultScanningServiceConfig(s.config) | |
| // Decode scan settings from base64 if provided | |
| var decoded []byte | |
| if len(scanSettings) > 0 { | |
| var err error | |
| decoded, err = base64.StdEncoding.DecodeString(scanSettings) | |
| if err != nil { | |
| zs.Errorf("Error decoding scan settings from base64: %v", err) | |
| return scanConfig, fmt.Errorf("error decoding scan settings from base64: %v", err) | |
| } else if s.config.App.Trace { | |
| zs.Debugf("Decoded scan settings: %s", string(decoded)) | |
| } | |
| } | |
| return flags, scanType, sbom, dbName | |
| return UpdateScanningServiceConfigDTO(zs, &scanConfig, flags, scanType, sbom, dbName, decoded) | |
| } | |
| // getConfigFromRequest extracts the form values from a request and returns the scanning configuration. | |
| func (s APIService) getConfigFromRequest(r *http.Request, zs *zap.SugaredLogger) (ScanningServiceConfig, error) { | |
| flags := strings.TrimSpace(r.FormValue("flags")) // Check form for scanning flags | |
| scanType := strings.TrimSpace(r.FormValue("type")) // Check form for SBOM type | |
| sbom := strings.TrimSpace(r.FormValue("assets")) // Check form for SBOM contents | |
| dbName := strings.TrimSpace(r.FormValue("db_name")) // Check form for db name | |
| // Fall back to headers if form values are empty | |
| if len(flags) == 0 { | |
| flags = strings.TrimSpace(r.Header.Get("flags")) | |
| } | |
| if len(scanType) == 0 { | |
| scanType = strings.TrimSpace(r.Header.Get("type")) | |
| } | |
| if len(sbom) == 0 { | |
| sbom = strings.TrimSpace(r.Header.Get("assets")) | |
| } | |
| if len(dbName) == 0 { | |
| dbName = strings.TrimSpace(r.Header.Get("db_name")) | |
| } | |
| scanSettings := strings.TrimSpace(r.Header.Get("Scanoss-Settings")) // Check header for scan settings | |
| if s.config.App.Trace { | |
| zs.Debugf("flags: %v, type: %v, assets_len: %d, db_name: %v, scanSettings_present: %t, scanSettings_len: %d", | |
| flags, scanType, len(sbom), dbName, len(scanSettings) > 0, len(scanSettings)) | |
| } | |
| // Create default configuration from server config | |
| scanConfig := DefaultScanningServiceConfig(s.config) | |
| // Decode scan settings from base64 if provided | |
| var decoded []byte | |
| if len(scanSettings) > 0 { | |
| // Prevent pathologically large headers (tune limit as needed) | |
| if len(scanSettings) > 16*1024 { | |
| return scanConfig, fmt.Errorf("Scanoss-Settings header too large") | |
| } | |
| var err error | |
| decoded, err = base64.StdEncoding.DecodeString(scanSettings) | |
| if err != nil { | |
| zs.Errorf("Error decoding scan settings from base64: %v", err) | |
| return scanConfig, fmt.Errorf("error decoding scan settings from base64: %v", err) | |
| } | |
| } | |
| return UpdateScanningServiceConfigDTO(zs, &scanConfig, flags, scanType, sbom, dbName, decoded) | |
| } |
🤖 Prompt for AI Agents
In @pkg/service/scanning_service.go around lines 168 - 207, In
getConfigFromRequest: stop logging r.Header and r.Form directly—replace that
Debugf with a minimal, non-sensitive summary (e.g., log presence/lengths or
whitelist keys) and explicitly redact Authorization/Cookie; do not log the
decoded settings string (remove zs.Debugf("Decoded scan settings: %s"...)). Add
a header size guard before decoding Scanoss-Settings (e.g., reject or truncate
if len(scanSettings) exceeds a safe threshold) and continue using
base64.StdEncoding for decoding; finally, ensure UpdateScanningServiceConfigDTO
and DefaultScanningServiceConfig usage remains unchanged and return errors as
currently done.
| // Build command arguments | ||
| var args []string | ||
| if s.config.Scanning.ScanDebug { | ||
| args = append(args, "-d") // Set debug mode | ||
| } | ||
| if len(dbName) > 0 && dbName != "" { // we want to prefer request over the local config | ||
| args = append(args, fmt.Sprintf("-n%s", dbName)) | ||
| } else if s.config.Scanning.ScanKbName != "" { // Set scanning KB name | ||
| args = append(args, fmt.Sprintf("-n%s", s.config.Scanning.ScanKbName)) | ||
| // Database name | ||
| if len(config.dbName) > 0 { | ||
| args = append(args, fmt.Sprintf("-n%s", config.dbName)) | ||
| } | ||
| if s.config.Scanning.ScanFlags > 0 { // Set system flags if enabled | ||
| args = append(args, fmt.Sprintf("-F %v", s.config.Scanning.ScanFlags)) | ||
| } else if len(flags) > 0 && flags != "0" { // Set user supplied flags if enabled | ||
| args = append(args, fmt.Sprintf("-F %s", flags)) | ||
| // Scanning flags | ||
| if config.flags > 0 { | ||
| args = append(args, fmt.Sprintf("-F%v", config.flags)) | ||
| } | ||
| if len(sbomFile) > 0 && len(sbomType) > 0 { // Add SBOM to scanning process | ||
| switch sbomType { | ||
| case "identify": | ||
| // SBOM configuration | ||
| if len(sbomFile) > 0 && len(config.sbomType) > 0 { | ||
| switch config.sbomType { | ||
| case sbomIdentify: | ||
| args = append(args, "-s") | ||
| case "blacklist": | ||
| case sbomBlackList: | ||
| args = append(args, "-b") | ||
| default: | ||
| args = append(args, "-s") // Default to identify | ||
| } | ||
| args = append(args, sbomFile) | ||
| } | ||
| // Ranking threshold (only if ranking is enabled and allowed) | ||
| if config.rankingEnabled && config.rankingThreshold > 0 { | ||
| args = append(args, fmt.Sprintf("-r%d", config.rankingThreshold)) | ||
| } | ||
| // Minimum snippet hits | ||
| if config.minSnippetHits > 0 { | ||
| args = append(args, fmt.Sprintf("--min-snippet-hits=%d", config.minSnippetHits)) | ||
| } | ||
| // Minimum snippet lines | ||
| if config.minSnippetLines > 0 { | ||
| args = append(args, fmt.Sprintf("--min-snippet-lines=%d", config.minSnippetLines)) | ||
| } | ||
| // Snippet range tolerance | ||
| if config.snippetRangeTolerance > 0 { | ||
| args = append(args, fmt.Sprintf("--range-tolerance=%d", config.snippetRangeTolerance)) | ||
| } | ||
| // Honour file extensions (not yet implemented in scanoss engine) | ||
| if !config.honourFileExts { | ||
| args = append(args, "--ignore-file-ext") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
scanoss engine version support --ignore-file-ext flag history
💡 Result:
Do you mean the Scanoss "engine" (the C CLI) or the Python CLI/package (scanoss.py)? Also, do you want a full history across all releases or support for a specific version range?
🌐 Web query:
scanoss engine ranking threshold -r flag version support
💡 Result:
Short answer: The rank-threshold option (to filter by file/component "rank") is supported in the Python CLI (scanoss-py) as the --rank-threshold argument; the native C "engine" documents the ranking algorithm but does not list a -r/--rank-threshold flag in its CLI. Sources: [scanoss-py docs][1] (shows --rank-threshold), [SCANOSS Engine docs][2] (describes ranking logic; engine CLI options don’t include -r).
References
- scanoss-py CLI docs — --rank-threshold option. [1]
- SCANOSS Engine docs — file ranking algorithm and engine CLI options. [2]
🌐 Web query:
scanoss engine --min-snippet-hits --min-snippet-lines --range-tolerance support versions
💡 Result:
Do you mean the SCANOSS CLI flags (scanoss / scanoss-py) or the scanoss-engine API/container? And do you want the minimum engine/CLI version that added support for --min-snippet-hits, --min-snippet-lines and --range-tolerance?
🏁 Script executed:
# First, verify the actual code in the repository
find . -name "scanning_service.go" -type fRepository: scanoss/api.go
Length of output: 91
🏁 Script executed:
# Check if there's any version/compatibility handling for the scanoss engine
grep -r "EngineSupports\|engineVersion\|version.*engine" --include="*.go" | head -20Repository: scanoss/api.go
Length of output: 471
🏁 Script executed:
# Read the scanning_service.go file to see the full context
cat -n pkg/service/scanning_service.go | head -100Repository: scanoss/api.go
Length of output: 4440
🏁 Script executed:
# Look for the command execution and which tool is being invoked
grep -n "scanoss\|exec\|Command" pkg/service/scanning_service.go | head -20Repository: scanoss/api.go
Length of output: 861
🏁 Script executed:
# Check if there are any version checks before appending flags
grep -B5 -A5 "ignore-file-ext\|min-snippet\|range-tolerance" pkg/service/scanning_service.goRepository: scanoss/api.go
Length of output: 1084
🏁 Script executed:
# Look at the broader context around the flag construction
sed -n '350,450p' pkg/service/scanning_service.goRepository: scanoss/api.go
Length of output: 3600
🌐 Web query:
scanoss engine CLI flags documentation complete list supported options
💡 Result:
Here are the SCANOSS Engine / scanoss CLI flags and options gathered from the official docs & repositories:
- --format / -f : result output format (default: plain). [1]
- --threads / -T : number of threads to use while scanning (default ~5–10, max varies). [1][2]
- --flags / -F : scanning engine flags (advanced engine configuration). [1][2]
- --post-size / -P : limit POST payload size in KB (default ~32–64). [1][2]
- --timeout / -M : timeout (seconds) for API communication. [1][2]
- --retry / -R : retry limit for API communication. [2]
- --no-wfp-output : skip WFP file generation. [1][2]
- --skip-snippets / -S : skip generation of snippets. [0][1]
- --all-folders : scan all folders. [1]
- --all-extensions : scan all file extensions. [1]
- --all-hidden : include hidden files/folders. [1]
- --obfuscate : obfuscate fingerprints. [1]
- --dependencies / -D : enable dependency scanning. [1]
- --dependencies-only : run only dependency scanning. [1]
- --sc-command : Scancode command/path (default: scancode). [1]
- --sc-timeout : timeout for Scancode (seconds). [1]
- --apiurl : SCANOSS API URL (default https://api.osskb.org/api/scan/direct). [1]
- --api2url : SCANOSS gRPC API 2.0 URL (default similar to apiurl). [1]
- --grpc-proxy : gRPC proxy URL. [1]
- --proxy : HTTP/HTTPS proxy (can use HTTPS_PROXY env var). [1]
- --pac : proxy auto-configuration. [1]
- --ca-cert : alternative CA PEM file (or use REQUEST_CA_BUNDLE / GRPC_DEFAULT_SSL_ROOTS_FILE_PATH env vars). [1]
- --ignore-cert-errors : ignore certificate errors. [1]
- --key / -k : SCANOSS API key token (optional for default API). [1]
Sources / reference pages:
- SCANOSS CLI/docs (scanoss-py / tools CLI reference). [ScanOSS docs]. [1]
- scanoss code / package definitions (repo and package metadata showing flags). [GitHub / pkg]. [2][3]
If you want, I can (a) fetch and paste the exact --help output for the latest scanoss binary you care about, or (b) provide direct links to each source.
🌐 Web query:
scanoss-c engine binary command line options -r --min-snippet-hits
💡 Result:
I can't find any published docs for the specific flags "-r" or "--min-snippet-hits" in the public SCANOSS repositories or docs. The official engine README documents general options and how to get help from the binary but does not list those two flags. [1]
Please run the engine's local help to see exact, current CLI options on your installation:
- scanoss --help
- scanoss -h
If you want, paste the help output here and I’ll explain the meaning of -r and/or --min-snippet-hits. Reference: scanoss engine README. [1]
Sources:
[1] scanoss/engine README (GitHub).
🏁 Script executed:
# Check if there's documentation about which scanoss binary is used
find . -name "*.md" -o -name "*.txt" -o -name "*.rst" | xargs grep -l "scanoss\|ScanBinary" 2>/dev/null | head -5Repository: scanoss/api.go
Length of output: 174
🏁 Script executed:
# Look for any configuration or defaults related to ScanBinary
grep -r "ScanBinary" --include="*.go" -B2 -A2Repository: scanoss/api.go
Length of output: 9764
🏁 Script executed:
# Check the config structure to see what ScanBinary might default to
grep -r "type.*Config\|ScanBinary\|scanning" --include="*.go" | grep -i "scan.*binary\|config.*scanning" | head -20Repository: scanoss/api.go
Length of output: 2301
Unsupported engine flags cause scan failures. The code passes -r, --min-snippet-hits, --min-snippet-lines, --range-tolerance, and --ignore-file-ext without verifying engine support. Official SCANOSS engine documentation does not list these flags, and passing unknown flags will cause the scanoss binary to reject them and fail scans. While the codebase tracks engineVersion, it is never consulted before appending flags.
Remove or conditionally guard these flags based on explicit engine version checks:
- Add
EngineSupportsRanking,EngineSupportsSnippetFlags,EngineSupportsIgnoreFileExtconfig options. - Only append flags when the deployed engine version explicitly supports them.
- Consider defaulting to safer behavior: omit flags entirely unless confirmed as supported.
🤖 Prompt for AI Agents
In @pkg/service/scanning_service.go around lines 398 - 442, The scan argument
builder appends flags (-r, --min-snippet-hits, --min-snippet-lines,
--range-tolerance, --ignore-file-ext) without checking engine support, causing
failures; update the builder to consult explicit support booleans on the config
(add EngineSupportsRanking, EngineSupportsSnippetFlags,
EngineSupportsIgnoreFileExt to the config used by the function) and only append
ranking flags when config.EngineSupportsRanking && config.rankingEnabled, only
append snippet-related flags when config.EngineSupportsSnippetFlags &&
(config.minSnippetHits > 0 || config.minSnippetLines > 0 ||
config.snippetRangeTolerance > 0), and only append "--ignore-file-ext" when
!config.honourFileExts && config.EngineSupportsIgnoreFileExt; ensure any new
fields are set based on the tracked engineVersion before this function is called
and default to false (omit flags) if support is unknown.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @pkg/service/kb_details_test.go:
- Around line 88-114: TestEngineVersionBelowMinimum mutates package-level
engineVersion, uses time.Sleep and has no assertions; fix by injecting or
mocking the engine version into the APIService (avoid setting the package var
directly), invoke the KB validation logic synchronously or expose a test hook
(e.g., call the method that validates engine version or provide a
SetEngineVersion/GetEngineVersion on APIService) and capture logs or return an
error to assert that the "critical" condition occurred; replace time.Sleep by
waiting on a channel/WaitGroup or calling the validation function directly, and
add explicit assertions (e.g., check captured logger output contains the
expected message or the function returns the expected error) so the test is
deterministic and isolated from other tests.
🧹 Nitpick comments (1)
pkg/service/scanning_service.go (1)
202-204: Consider masking decoded settings in trace logs.Logging the raw decoded scan settings could expose sensitive configuration data in trace logs. If these settings may contain sensitive values, consider logging only field names or a sanitized summary.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
go.modpkg/config/server_config.gopkg/service/kb_details.gopkg/service/kb_details_test.gopkg/service/scanning_service.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/service/kb_details.go
- pkg/config/server_config.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/service/kb_details_test.go (1)
pkg/service/utils_service.go (1)
NewAPIService(76-79)
pkg/service/scanning_service.go (1)
pkg/service/scanning_service_config.go (3)
ScanningServiceConfig(28-40)DefaultScanningServiceConfig(42-56)UpdateScanningServiceConfigDTO(173-196)
🔇 Additional comments (7)
pkg/service/scanning_service.go (6)
40-43: Good extraction of SBOM type constants.These constants improve readability and reduce magic string usage throughout the file.
226-228: Good refactoring to config-driven scanning.The function signature updates consistently propagate the
ScanningServiceConfigand SBOM file path through the call chain. The separation ofsbomFile(temp file path) fromconfig.sbomFile(content) is appropriate.
95-100: Proper error handling for config parsing.The error handling returns an appropriate HTTP 400 status without leaking internal details, and correctly sets the span error for observability.
103-118: SBOM handling correctly validates type and stores content.The validation against
sbomIdentifyandsbomBlackListconstants is properly implemented, and the flow correctly writes SBOM content to a temp file for engine consumption.
411-422: SBOM argument construction is correct.The switch statement properly maps
sbomTypeto engine flags (-sfor identify,-bfor blacklist), with a sensible default fallback.
439-442: The inline comment stating the flag is "not yet implemented in scanoss engine" is outdated. The--ignore-file-extflag is fully implemented and integrated into the codebase. Evidence shows:
HonourFileExtsis a valid, tested configuration option added in v1.6.0- The minimum required engine version (5.4.20) supports this flag
- The test support binary successfully processes the flag without errors
- The flag is treated consistently with other conditional engine options (ranking, snippet settings)
Update the inline comment to reflect that the feature is implemented:
// Honour file extensions when enabled (requires scanoss engine 5.4.20+) if !config.honourFileExts { args = append(args, "--ignore-file-ext") }No code changes are required; the implementation is correct.
Likely an incorrect or invalid review comment.
go.mod (1)
37-37: Appropriate dependency for version parsing.The
hashicorp/go-versionlibrary is a well-established choice for semantic version comparison, which is needed for the newvalidateEngineVersionfunctionality. The v1.8.0 release is available and the implementation inpkg/service/kb_details.gocorrectly uses the library's API for version parsing and comparison.
| func TestEngineVersionBelowMinimum(t *testing.T) { | ||
| err := zlog.NewSugaredDevLogger() | ||
| if err != nil { | ||
| t.Fatalf("an error '%s' was not expected when opening a sugared logger", err) | ||
| } | ||
| defer zlog.SyncZap() | ||
|
|
||
| myConfig := setupConfig(t) | ||
| myConfig.App.Trace = true | ||
| myConfig.Scanning.LoadKbDetails = true | ||
| myConfig.Scanning.MinEngineVersion = "5.4.19" | ||
|
|
||
| apiService := NewAPIService(myConfig) | ||
|
|
||
| // Simulate engine version below minimum | ||
| engineVersion = "5.4.0" | ||
|
|
||
| // Setup cron which will call loadKBDetails | ||
| apiService.SetupKBDetailsCron() | ||
|
|
||
| // Wait for the cron to execute | ||
| time.Sleep(time.Duration(3) * time.Second) | ||
|
|
||
| // The critical error should have been logged | ||
| // (we can't easily assert on log output without capturing it, | ||
| // but the function will execute and log the error) | ||
| fmt.Println("Engine version validation test completed - check logs for CRITICAL error") |
There was a problem hiding this comment.
Test lacks assertions and may cause test pollution.
This test has several issues:
-
No assertions: The test doesn't verify any behavior - it only logs a message asking to "check logs for CRITICAL error". This provides no automated validation in CI.
-
Package-level variable mutation: Directly setting
engineVersion = "5.4.0"(line 103) modifies shared state that could affect other tests running in parallel or subsequently. -
Flaky synchronization: Using
time.Sleep(3 * time.Second)is unreliable for waiting on async operations.
Consider capturing log output to assert the critical error was logged, or use a mock/interface to verify the validation behavior.
Suggested approach for proper test isolation and assertion
func TestEngineVersionBelowMinimum(t *testing.T) {
+ // Save and restore original engineVersion to prevent test pollution
+ originalVersion := engineVersion
+ defer func() { engineVersion = originalVersion }()
+
err := zlog.NewSugaredDevLogger()
if err != nil {
t.Fatalf("an error '%s' was not expected when opening a sugared logger", err)
}
defer zlog.SyncZap()
myConfig := setupConfig(t)
myConfig.App.Trace = true
myConfig.Scanning.LoadKbDetails = true
myConfig.Scanning.MinEngineVersion = "5.4.19"
apiService := NewAPIService(myConfig)
// Simulate engine version below minimum
engineVersion = "5.4.0"
- // Setup cron which will call loadKBDetails
- apiService.SetupKBDetailsCron()
-
- // Wait for the cron to execute
- time.Sleep(time.Duration(3) * time.Second)
-
- // The critical error should have been logged
- // (we can't easily assert on log output without capturing it,
- // but the function will execute and log the error)
- fmt.Println("Engine version validation test completed - check logs for CRITICAL error")
+ // Directly call loadKBDetails for synchronous testing
+ apiService.loadKBDetails()
+
+ // TODO: Add proper assertion by either:
+ // 1. Capturing log output using a test hook
+ // 2. Making validateEngineVersion return an error that can be checked
+ // 3. Adding a callback/channel to signal validation failure
}🤖 Prompt for AI Agents
In @pkg/service/kb_details_test.go around lines 88 - 114,
TestEngineVersionBelowMinimum mutates package-level engineVersion, uses
time.Sleep and has no assertions; fix by injecting or mocking the engine version
into the APIService (avoid setting the package var directly), invoke the KB
validation logic synchronously or expose a test hook (e.g., call the method that
validates engine version or provide a SetEngineVersion/GetEngineVersion on
APIService) and capture logs or return an error to assert that the "critical"
condition occurred; replace time.Sleep by waiting on a channel/WaitGroup or
calling the validation function directly, and add explicit assertions (e.g.,
check captured logger output contains the expected message or the function
returns the expected error) so the test is deterministic and isolated from other
tests.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @pkg/service/scanning_service.go:
- Around line 440-443: The code appends an unsupported "--ignore-file-ext" flag
when config.honourFileExts is false, which causes the ScanOSS engine to fail;
remove the args = append(args, "--ignore-file-ext") line and instead either (a)
pre-filter the file list before building args (use the same variable that
constructs the scan target paths so only desired extensions are passed to the
scan command), or (b) if you need to pass file-type constraints, switch to using
an SBOM input with the supported "-s" or "-b" options when constructing args;
update the logic that uses config.honourFileExts and the args slice so no
unsupported flag is ever appended.
- Around line 424-427: The conditional uses s.config.Scanning.RankingAllowed
instead of the locally-initialized field config.rankingAllowed; update the if to
check config.rankingAllowed so the check is consistent with
applyRankingSettings() and the rest of this function (i.e., ensure the condition
reads: config.rankingEnabled && config.rankingThreshold > 0 &&
config.rankingAllowed before appending the "-r" arg via args = append(...)).
🧹 Nitpick comments (1)
pkg/service/scanning_service.go (1)
104-119: Consider renamingsbomFilefield for clarity.The
scanConfig.sbomFilefield contains SBOM content (passed towriteSbomFilewhich writes it as a string), but the name suggests it's a filename. This is confusing sincesbomFilename(line 118) is the actual temp file path. Consider renaming tosbomContentor similar inScanningServiceConfigto improve clarity.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/config/server_config.gopkg/service/kb_details.gopkg/service/kb_details_test.gopkg/service/scanning_service.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/service/kb_details_test.go
- pkg/config/server_config.go
- pkg/service/kb_details.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/service/scanning_service.go (1)
pkg/service/scanning_service_config.go (3)
ScanningServiceConfig(28-40)DefaultScanningServiceConfig(42-56)UpdateScanningServiceConfigDTO(173-196)
🔇 Additional comments (6)
pkg/service/scanning_service.go (6)
3-3: LGTM!Copyright year update is appropriate.
227-229: LGTM!Function signature properly updated to support the config-driven scanning approach.
247-264: LGTM!The threaded scanning function correctly propagates the new config parameters to worker goroutines.
339-353: LGTM!Worker scan function correctly updated to pass configuration to the scanning engine.
444-458: LGTM!Command execution with timeout and error handling follows existing patterns and is implemented correctly.
40-44: minEngineVersion constant usage is appropriate.The
minEngineVersionconstant defined at scanning_service.go:43 is correctly used in kb_details.go:153 where it's passed to thevalidateEngineVersion()function call. As a package-level constant, it's accessible across files in theservicepackage without requiring an import. No issues detected.
| if s.config.App.Trace { | ||
| zs.Debugf("Header: %v, Form: %v, flags: %v, type: %v, assets: %v, db_name %v", r.Header, r.Form, flags, scanType, sbom, dbName) | ||
| 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) | ||
| } |
There was a problem hiding this comment.
Potential sensitive data in debug logs.
The debug log at lines 190-191 outputs the full assets (SBOM content) and scanSettings which may contain sensitive information. While gated by Trace flag, consider redacting or truncating these fields in logs to avoid accidentally exposing sensitive data in trace-enabled environments.
Suggested mitigation
if s.config.App.Trace {
- 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.Debugf("Header: %v, Form: %v, flags: %v, type: %v, assets_len: %v, db_name: %v, scanSettings_len: %v",
+ r.Header, r.Form, flags, scanType, len(sbom), dbName, len(scanSettings))
}📝 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.
| if s.config.App.Trace { | |
| zs.Debugf("Header: %v, Form: %v, flags: %v, type: %v, assets: %v, db_name %v", r.Header, r.Form, flags, scanType, sbom, dbName) | |
| 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) | |
| } | |
| if s.config.App.Trace { | |
| zs.Debugf("Header: %v, Form: %v, flags: %v, type: %v, assets_len: %v, db_name: %v, scanSettings_len: %v", | |
| r.Header, r.Form, flags, scanType, len(sbom), dbName, len(scanSettings)) | |
| } |
| // Ranking threshold (only if ranking is enabled and allowed) | ||
| if config.rankingEnabled && config.rankingThreshold > 0 && s.config.Scanning.RankingAllowed { | ||
| args = append(args, fmt.Sprintf("-r%d", config.rankingThreshold)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how rankingAllowed is used in the config vs how it's applied
rg -n -A2 -B2 'rankingAllowed|RankingAllowed' --type goRepository: scanoss/api.go
Length of output: 6402
Use config.rankingAllowed instead of s.config.Scanning.RankingAllowed on line 425.
Line 425 checks s.config.Scanning.RankingAllowed instead of config.rankingAllowed. Since config.rankingAllowed is already initialized from the server config and gated by applyRankingSettings(), use the local config field for consistency with the rest of the function. This aligns the check with the gating logic applied elsewhere in the config processing.
🤖 Prompt for AI Agents
In @pkg/service/scanning_service.go around lines 424 - 427, The conditional uses
s.config.Scanning.RankingAllowed instead of the locally-initialized field
config.rankingAllowed; update the if to check config.rankingAllowed so the check
is consistent with applyRankingSettings() and the rest of this function (i.e.,
ensure the condition reads: config.rankingEnabled && config.rankingThreshold > 0
&& config.rankingAllowed before appending the "-r" arg via args = append(...)).
| // Honour file extensions (not yet implemented in scanoss engine) | ||
| if !config.honourFileExts { | ||
| args = append(args, "--ignore-file-ext") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
scanoss engine --ignore-file-ext flag support documentation
💡 Result:
Short answer: The ScanOSS engine does not document a --ignore-file-ext flag. The engine's CLI options (from its GitHub README and ReadTheDocs) list -w, -s, -b, -t, -v, -h and -d but no --ignore-file-ext option. [1][2]
Workarounds:
- Pre-filter files before calling scanoss (e.g., find ... -name '*.ext' -prune or use shell globs) and pass the remaining paths to scanoss.
- Use an SBOM with -s or -b to include/ignore assets at the SBOM level if that fits your case. [1][2]
Sources:
- ScanOSS engine README (GitHub). [1]
- ScanOSS Engine docs (ReadTheDocs). [2]
🏁 Script executed:
# Let's also examine the context of the code in question and search for scanoss-related information in the repo
grep -r "ignore-file-ext" --include="*.go" --include="*.md" --include="*.txt" || echo "No matches found in repo"Repository: scanoss/api.go
Length of output: 132
🏁 Script executed:
# Search for any scanoss-related documentation or references
fd -e md -e txt | xargs grep -l "scanoss" 2>/dev/null | head -20Repository: scanoss/api.go
Length of output: 177
🏁 Script executed:
# Check the scanning_service.go file for more context around those lines
cat -n pkg/service/scanning_service.go | sed -n '430,455p'Repository: scanoss/api.go
Length of output: 1434
🏁 Script executed:
# Search for where this scanning service is called or configured
rg "scanning_service|honourFileExts" --type go -B 2 -A 2 | head -50Repository: scanoss/api.go
Length of output: 2976
Remove the unsupported --ignore-file-ext flag or implement the feature in a different way.
The --ignore-file-ext flag is not supported by the ScanOSS engine (verified against official engine documentation—supported options are -w, -s, -b, -t, -v, -h, -d only). When config.honourFileExts is false, this code passes an unsupported flag to the scan command, causing it to fail with an unknown flag error. Either remove this flag passing, pre-filter files before scanning, or use an SBOM with -s or -b flags if applicable.
🤖 Prompt for AI Agents
In @pkg/service/scanning_service.go around lines 440 - 443, The code appends an
unsupported "--ignore-file-ext" flag when config.honourFileExts is false, which
causes the ScanOSS engine to fail; remove the args = append(args,
"--ignore-file-ext") line and instead either (a) pre-filter the file list before
building args (use the same variable that constructs the scan target paths so
only desired extensions are passed to the scan command), or (b) if you need to
pass file-type constraints, switch to using an SBOM input with the supported
"-s" or "-b" options when constructing args; update the logic that uses
config.honourFileExts and the args slice so no unsupported flag is ever
appended.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/scanning_test.go`:
- Around line 192-197: Update the failing test error messages to fix the
grammar: replace the awkward phrases in the s.Failf calls so they read "an error
was not expected when creating multipart form data" for the multipart creation
error and "an error was not expected when creating the HTTP request" for the
http.NewRequest error (refer to the s.Failf calls near the multipart creation
and the http.NewRequest(fmt.Sprintf("%v/scan/direct", hostPort), &b) usage).
♻️ Duplicate comments (4)
pkg/service/scanning_service.go (3)
428-439: Verify engine support for snippet and file-extension flags.The flags
--min-snippet-hits,--min-snippet-lines, and--ignore-file-extare passed without version checking. TheminEngineVersionconstant (line 43) suggests version validation was planned. Consider gating these flags based on engine version to avoid failures with older engines.What SCANOSS engine version supports --min-snippet-hits --min-snippet-lines --ignore-file-ext flags?
189-192: Potential sensitive data exposure in debug logs.The trace log outputs full
r.Header,r.Form,sbomcontent, andscanSettingswhich may contain sensitive information. Consider logging only non-sensitive metadata:if s.config.App.Trace { - 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.Debugf("flags: %v, type: %v, assets_len: %d, db_name: %v, scanSettings_present: %t", + flags, scanType, len(sbom), dbName, len(scanSettings) > 0) }
424-427: Useconfig.rankingAllowedinstead ofs.config.Scanning.RankingAllowed.Line 425 checks server config directly while all other conditions in this function use the local
configparameter. This creates inconsistency - the config-driven approach should be used throughout.Proposed fix
// Ranking threshold (only if ranking is enabled and allowed) -if config.rankingEnabled && config.rankingThreshold > 0 && s.config.Scanning.RankingAllowed { +if config.rankingEnabled && config.rankingThreshold > 0 && config.rankingAllowed { args = append(args, fmt.Sprintf("-r%d", config.rankingThreshold)) }pkg/service/scanning_service_config.go (1)
41-54: Add nil check forserverDefaultConfigparameter.If
serverDefaultConfigis nil, this function will panic with a nil pointer dereference on line 43. A guard similar toUpdateScanningServiceConfigDTO(line 164) should be added for consistency.Proposed fix
func DefaultScanningServiceConfig(serverDefaultConfig *cfg.ServerConfig) ScanningServiceConfig { + if serverDefaultConfig == nil { + return ScanningServiceConfig{} + } return ScanningServiceConfig{ flags: serverDefaultConfig.Scanning.ScanFlags,
🧹 Nitpick comments (2)
pkg/service/scanning_service_config_test.go (1)
27-64: Consider adding test for nil serverConfig input.The test covers the happy path well. Consider adding a test case for
DefaultScanningServiceConfig(nil)to verify behavior when server config is nil (once the nil check is implemented in the function).pkg/service/scanning_service.go (1)
96-101: Consider logging the config error before returning 400.The error from
getConfigFromRequestis discarded. For debugging, consider logging it:scanConfig, err := s.getConfigFromRequest(r, zs) if err != nil { + zs.Warnf("Invalid scanning configuration: %v", err) http.Error(w, "ERROR invalid scanning configuration", http.StatusBadRequest)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.mdpkg/config/server_config.gopkg/service/scanning_service.gopkg/service/scanning_service_config.gopkg/service/scanning_service_config_test.gotests/scanning_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/config/server_config.go
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/service/scanning_service_config.go (1)
pkg/config/server_config.go (1)
ServerConfig(37-93)
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-185)
pkg/service/scanning_service.go (1)
pkg/service/scanning_service_config.go (3)
ScanningServiceConfig(28-39)DefaultScanningServiceConfig(41-54)UpdateScanningServiceConfigDTO(162-185)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~19-~19: Ensure spelling is correct
Context: ...d - minSnippetHits - minSnippetLines - honourFileExts ## [1.5.2] - 2025-11-07 ### Added - Added Cu...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (18)
CHANGELOG.md (1)
11-21: LGTM!The changelog entry is well-structured, properly documenting the new scanning configuration parameters with a clear sub-list as requested. The version comparison link at line 175 completes the entry.
tests/scanning_test.go (2)
93-101: LGTM!The assets field correctly uses JSON format with components array, aligning with the new SBOM handling approach in the scanning service.
136-222: Good test coverage for the new Scanoss-Settings header.The test cases cover the essential scenarios: valid settings, invalid base64, and combination with legacy parameters. The inline comments documenting the decoded JSON payloads are helpful for understanding test intent.
pkg/service/scanning_service_config.go (5)
28-39: LGTM - struct design is clean.The unexported fields provide good encapsulation. The struct correctly maps to server configuration fields.
56-106: LGTM - well-structured settings parsing with appropriate validation.The use of pointer types in
scanSettingscorrectly handles optional JSON fields. The validation inapplySnippetSettingsproperly distinguishes between invalid negative values and valid zero/positive values.
108-131: LGTM!The function correctly handles empty strings as "no change" and appropriately logs errors for invalid flags without failing the entire update, matching the documented behavior.
162-185: LGTM - UpdateScanningServiceConfigDTO implementation is solid.The nil check, copy semantics (preserving original config), and error handling are all correct. The function properly delegates to helper functions for modular updates.
145-153: Documentation references non-existent fieldsnippet_range_tolerance.Line 151 documents
snippet_range_toleranceas a valid JSON field, but this field doesn't exist in thescanSettingsstruct (lines 57-63) orScanningServiceConfig. Either remove this from the documentation or implement the field if it's intended.Proposed fix (remove from documentation)
// "ranking_enabled": bool, // Enable/disable ranking (requires ranking_allowed=true) // "ranking_threshold": int, // Ranking threshold value (requires ranking_allowed=true) // "min_snippet_hits": int, // Minimum snippet hits to consider a match // "min_snippet_lines": int, // Minimum snippet lines to consider a match -// "snippet_range_tolerance": int, // Snippet range tolerance for matching // "honour_file_exts": bool // Honor file extensions when filtering snippetsLikely an incorrect or invalid review comment.
pkg/service/scanning_service_config_test.go (6)
66-126: LGTM!Comprehensive test for JSON settings parsing, correctly using pointer types to match the
scanSettingsstruct behavior.
128-168: LGTM!Good test verifying that ranking settings are properly gated by
rankingAllowedflag.
170-204: LGTM!Thorough test for legacy parameter handling.
206-234: LGTM!Good test coverage for invalid inputs, correctly distinguishing between recoverable (invalid flags) and non-recoverable (invalid JSON) errors.
236-301: LGTM!Excellent integration test verifying that JSON settings and legacy parameters work correctly together.
303-313: LGTM!Essential edge case test for nil config handling.
pkg/service/scanning_service.go (4)
104-119: LGTM!Clean refactoring using the config-driven approach with proper SBOM type validation.
227-229: LGTM - consistent config-driven method signatures.The refactored signatures properly separate the temporary SBOM file path from the configuration object.
339-375: LGTM - worker functions properly propagate config.The
workerScanfunction correctly passes the config and sbomFile toscanWfp.
40-44: LGTM - constants improve readability.The SBOM type constants (
sbomIdentify,sbomBlackList) eliminate magic strings and are properly utilized in validation and case handling. TheminEngineVersionconstant is used inkb_details.gofor engine version validation via thevalidateEngineVersionfunction.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| if err != nil { | ||
| s.Failf("an error was not creating multipart form data.", "error: %v", err) | ||
| } | ||
| req, err := http.NewRequest(http.MethodPost, fmt.Sprintf("%v/scan/direct", hostPort), &b) | ||
| if err != nil { | ||
| s.Failf("an error was not creating request.", "error: %v", err) |
There was a problem hiding this comment.
Minor grammatical issue in error messages.
The error messages contain awkward phrasing: "an error was not creating" should be "an error was not expected when creating".
Proposed fix
b, w, err := createMultipartFormData("file", test.filename, test.shortName, test.extraFields)
if err != nil {
- s.Failf("an error was not creating multipart form data.", "error: %v", err)
+ s.Failf("an error was not expected when creating multipart form data.", "error: %v", err)
}
req, err := http.NewRequest(http.MethodPost, fmt.Sprintf("%v/scan/direct", hostPort), &b)
if err != nil {
- s.Failf("an error was not creating request.", "error: %v", err)
+ s.Failf("an error was not expected when creating request.", "error: %v", err)
}📝 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.
| if err != nil { | |
| s.Failf("an error was not creating multipart form data.", "error: %v", err) | |
| } | |
| req, err := http.NewRequest(http.MethodPost, fmt.Sprintf("%v/scan/direct", hostPort), &b) | |
| if err != nil { | |
| s.Failf("an error was not creating request.", "error: %v", err) | |
| if err != nil { | |
| s.Failf("an error was not expected when creating multipart form data.", "error: %v", err) | |
| } | |
| req, err := http.NewRequest(http.MethodPost, fmt.Sprintf("%v/scan/direct", hostPort), &b) | |
| if err != nil { | |
| s.Failf("an error was not expected when creating request.", "error: %v", err) |
🤖 Prompt for AI Agents
In `@tests/scanning_test.go` around lines 192 - 197, Update the failing test error
messages to fix the grammar: replace the awkward phrases in the s.Failf calls so
they read "an error was not expected when creating multipart form data" for the
multipart creation error and "an error was not expected when creating the HTTP
request" for the http.NewRequest error (refer to the s.Failf calls near the
multipart creation and the http.NewRequest(fmt.Sprintf("%v/scan/direct",
hostPort), &b) usage).
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.