Feat/sp 3099 EPSS info #29
Conversation
…erability response
📝 WalkthroughWalkthroughAdds EPSS support and DTO fields, refactors OSV to a configurable worker-pool with logger injection, migrates PURL parsing to an external helper, removes All-URLs and Golang-project models/tests, adds API worker config, bumps papi, and changes Makefile to build linux-arm64. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant H as Handler
participant US as VulnerabilityUseCase
participant OU as OSVUseCase
participant WK as Worker
participant API as OSV API
participant EP as EPSSModel
H->>US: Request vulnerabilities
US->>OU: processRequests(ctx, requests)
Note over OU,WK: spawn N workers (MaxAPIWorkers) and create jobs/results channels
OU->>WK: enqueue OSVRequest (jobs channel)
WK->>API: POST /vulns (package/version payload)
API-->>WK: OSV response
WK->>OU: send result (results channel)
OU-->>US: aggregated OSV vulnerabilities
US->>EP: GetEPSSByCVEs(ctx, cveList)
EP-->>US: EPSS records
US-->>H: VulnerabilityOutput (OSV + EPSS merged)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
🤖 Fix all issues with AI Agents
In @pkg/usecase/OSV_use_case.go:
- Around line 182-186: Replace the fmt.Println call inside the ctx.Done() select
case with the project's structured logger: call zlog.S().Info("Worker:
cancellation signal received, stopping.") (or Infof if you want formatted
fields) so it uses structured logging and log levels; then remove the unused
"fmt" import from the file; locate the select case handling <-ctx.Done() in
OSV_use_case.go to make the change.
- Around line 53-62: NewOSVUseCase currently assigns MaxAPIWorkers directly from
config.Source.OSV.APIWorkers which can be zero or negative and will deadlock
processRequests; update NewOSVUseCase to validate and enforce a safe minimum
(e.g., if config.Source.OSV.APIWorkers <= 0 then set MaxAPIWorkers = 1) and
optionally cap to a sane maximum, so OSVUseCase.MaxAPIWorkers is never zero or
negative before being used by processRequests.
- Around line 174-179: The code currently logs a failed resp.Body.Close() and
sends an empty response (results <- response; continue) even when json.Decode
succeeded, discarding valid data; modify the block after json decoding so that
if resp.Body.Close() returns an error you log or warn about the close failure
but still send the decoded response into the results channel (i.e., remove the
early continue that returns an empty response), and ideally move
resp.Body.Close() into a deferred call right after a successful response read
start to ensure close is attempted regardless of later errors; reference the
variables and symbols used here: resp.Body.Close(), json.Decode, results channel
and response variable to implement the change.
In @pkg/usecase/vulnerability_use_case.go:
- Around line 86-90: Replace the global logger call zlog.S.Infof with the
injected logger on the use case (us.s) for consistency: in the block that checks
us.config.Source.OSV.Enabled and constructs NewOSVUseCase (where
osvUseCase.Execute sets osvVulnerabilities), change the log call to use us.s
(e.g., us.s.Infof) so the method consistently uses the injected logger instance.
🧹 Nitpick comments (6)
pkg/utils/purl.go (1)
33-56: Consider adding support for additional ecosystem types.The
ProjectURLfunction handles common ecosystems well. Consider whether additional types likenuget,cargo, orcomposershould be supported for completeness, or document that unsupported types return an error intentionally.pkg/dtos/vulnerability_output.go (1)
69-69: Consider addingomitemptyfor EPSS field.The
Epssfield is not a pointer and lacksomitempty, so it will serialize as{"percentile": 0, "probability": 0}when EPSS data is unavailable. If empty EPSS should be omitted from the response, consider using a pointer type or addingomitempty.🔎 Proposed fix if omission is desired
- Epss EPSS `json:"epss"` + Epss *EPSS `json:"epss,omitempty"`pkg/models/vulns_purl.go (1)
62-63: Minor typo in comment.The comment says "valid" instead of "validate".
🔎 Proposed fix
- // used to valid the PURL + // used to validate the PURL _, err := purlhelper.PurlFromString(purl)pkg/config/server_config.go (1)
79-79: Consider adding validation forAPIWorkersbounds.The
APIWorkersfield lacks validation inIsValidConfig. If set to 0 or a negative value via theVULN_OSV_API_WORKERSenvironment variable, it could cause unexpected behavior in the OSV worker pool.🔎 Suggested validation in IsValidConfig
// Check OSV source config if cfg.Source.OSV.Enabled { if cfg.Source.OSV.APIBaseURL == "" { return errors.New("OSV API Base URL cannot be empty") } if cfg.Source.OSV.InfoBaseURL == "" { return errors.New("OSV Info Base URL cannot be empty") } + if cfg.Source.OSV.APIWorkers < 1 { + return errors.New("OSV API Workers must be at least 1") + } }pkg/usecase/vulnerability_use_case_test.go (1)
43-43: Logger extracted from context without attached zap logger returns a no-op logger.
ctxzap.Extract(ctx)on a barecontext.Background()returns a no-op logger since no logger was attached to the context. While the test will still run, any logging within the use case will be silently discarded.Consider using
zlog.Sdirectly (the global sugared logger initialized on line 38) or attaching a logger to the context viactxzap.ToContext.🔎 Option 1: Use global logger directly
- s := ctxzap.Extract(ctx).Sugar() + s := zlog.S🔎 Option 2: Attach logger to context
+ ctx = ctxzap.ToContext(ctx, zlog.L) s := ctxzap.Extract(ctx).Sugar()pkg/usecase/OSV_use_case.go (1)
156-165: Consider usingdeferfor response body cleanup to prevent leaks on early returns.While each error path closes the body, using
deferafter the successfulclient.Docall is more idiomatic and safer against future code changes.🔎 Proposed fix pattern
resp, err := us.client.Do(req) if err != nil { zlog.S.Errorf("HTTP request failed: %s", err) results <- response continue } + defer resp.Body.Close() // Check for non-200 HTTP responses if resp.StatusCode != http.StatusOK { zlog.S.Errorf("Unexpected HTTP status: %d", resp.StatusCode) - err = resp.Body.Close() - if err != nil { - zlog.S.Errorf("Failed to close response body: %s", err) - } results <- response continue }Note: With
deferinside theforloop'sselectcase, each iteration's defer will execute when the function returns, not at the end of each iteration. For a cleaner approach, consider extracting the HTTP call logic into a separate function wheredeferworks as expected per call.
📜 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 (22)
CHANGELOG.mdMakefilego.modpkg/adapters/vulnerability_support.gopkg/config/server_config.gopkg/dtos/vulnerability_output.gopkg/models/all_urls.gopkg/models/all_urls_test.gopkg/models/cpe_purl.gopkg/models/epss.gopkg/models/epss_test.gopkg/models/golang_projects.gopkg/models/golang_projects_test.gopkg/models/tests/epss.sqlpkg/models/vulns_purl.gopkg/service/vulnerability_service.gopkg/usecase/OSV_use_case.gopkg/usecase/OSV_use_case_test.gopkg/usecase/vulnerability_use_case.gopkg/usecase/vulnerability_use_case_test.gopkg/utils/purl.gopkg/utils/purl_test.go
💤 Files with no reviewable changes (6)
- pkg/models/golang_projects_test.go
- pkg/models/all_urls.go
- pkg/utils/purl_test.go
- pkg/models/all_urls_test.go
- pkg/models/golang_projects.go
- Makefile
🧰 Additional context used
🧬 Code graph analysis (7)
pkg/dtos/vulnerability_output.go (1)
pkg/models/epss.go (1)
EPSS(22-26)
pkg/service/vulnerability_service.go (1)
pkg/usecase/vulnerability_use_case.go (1)
NewVulnerabilityUseCase(45-51)
pkg/usecase/vulnerability_use_case_test.go (1)
pkg/usecase/vulnerability_use_case.go (1)
NewVulnerabilityUseCase(45-51)
pkg/models/epss_test.go (3)
pkg/models/common.go (1)
CloseDB(79-87)pkg/config/server_config.go (1)
NewServerConfig(88-102)pkg/models/epss.go (1)
NewEPSSModel(29-36)
pkg/models/epss.go (2)
pkg/config/server_config.go (1)
ServerConfig(32-85)pkg/dtos/vulnerability_output.go (1)
EPSS(54-57)
pkg/usecase/OSV_use_case_test.go (2)
pkg/config/server_config.go (1)
NewServerConfig(88-102)pkg/usecase/OSV_use_case.go (2)
OSVUseCase(46-51)NewOSVUseCase(53-62)
pkg/usecase/vulnerability_use_case.go (5)
pkg/config/server_config.go (1)
ServerConfig(32-85)pkg/dtos/component_dto.go (1)
ComponentDTO(19-23)pkg/dtos/vulnerability_output.go (2)
VulnerabilityOutput(28-30)EPSS(54-57)pkg/usecase/OSV_use_case.go (1)
NewOSVUseCase(53-62)pkg/models/epss.go (3)
EPSSModel(15-20)NewEPSSModel(29-36)EPSS(22-26)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~16-~16: Ensure spelling is correct
Context: ...y response ### Changed - Refactored OSV usecase - Upgraded scanoss/papi to v0.28.0 ## [...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (28)
CHANGELOG.md (1)
12-17: LGTM!The changelog entry follows the Keep a Changelog format correctly and documents the EPSS integration, OSV refactor, and PAPI upgrade appropriately.
pkg/utils/purl.go (1)
24-31: LGTM!The simplified PURL utility correctly retains
PurlRemoveFromVersionComponentwhich is still used by the model layer. The regex pattern correctly matches the PURL spec components (version@, qualifiers?, and subpath#).pkg/dtos/vulnerability_output.go (1)
54-57: LGTM - EPSS struct correctly models the scoring data.The field types (
float32) are appropriate for EPSS scores which range from 0 to 1.pkg/models/cpe_purl.go (3)
26-26: LGTM!Clean import of the external PURL helper package with a clear alias.
53-53: LGTM - Migration to external helper is correct.The migration from
utils.PurlFromStringtopurlhelper.PurlFromStringmaintains the same error handling pattern.
62-62: LGTM!The
GetVersionFromReqfunction is correctly migrated to use the external helper.pkg/adapters/vulnerability_support.go (2)
25-25: LGTM!Consistent use of the
purlhelperalias matching other files in this PR.
37-37: LGTM - Validation-only usage is correct.The discarded return value is intentional since only PURL validation is needed here. The error is properly handled to categorize invalid components.
pkg/models/vulns_purl.go (1)
25-25: LGTM!Consistent import of the external PURL helper package.
go.mod (1)
19-19: PAPI version upgrade to v0.28.0 is compatible and safe.The upgrade from v0.17.0 to v0.28.0 is confirmed as a stable release with no breaking changes. Release notes for intermediate versions (v0.25.0, v0.24.0, v0.28.0) show only additive changes: new EPSS fields (probability, percentile), error handling enhancements, and Semgrep endpoints. The codebase already supports EPSS data structures and is well-positioned to leverage the new functionality in v0.28.0 without any compatibility issues.
pkg/config/server_config.go (1)
125-125: LGTM!The default value of 5 workers is a reasonable starting point for concurrent OSV API requests.
pkg/usecase/vulnerability_use_case_test.go (1)
63-63: LGTM!The constructor call correctly passes the logger, config, and database in the expected order.
pkg/service/vulnerability_service.go (3)
56-56: LGTM!The logger extraction and use case initialization in
GetVulnerabilitiescorrectly follows the new pattern and aligns with the existing middleware setup.Also applies to: 71-71
195-195: LGTM!Consistent with the updated constructor signature.
226-226: LGTM!Consistent with the updated constructor signature.
pkg/models/tests/epss.sql (1)
1-10: LGTM!The test fixture correctly defines the
epss_dataschema and provides realistic sample data for the EPSS model tests.pkg/usecase/OSV_use_case_test.go (1)
33-56: LGTM!The test correctly adopts the new config-based constructor pattern. Initializing
OSVUseCaseonce before the test loop is efficient since the same configuration applies to all test cases.pkg/models/epss_test.go (4)
34-71: Good test coverage for the happy path.The test correctly validates that all three CVEs are returned. Same note as in the vulnerability use case test:
ctxzap.Extract(ctx)oncontext.Background()yields a no-op logger.
73-108: Good edge case coverage for empty input.This validates the guard clause in
GetEPSSByCVEsthat returns early for empty input.
110-146: Good edge case coverage for non-existent CVEs.This validates graceful handling when queried CVEs don't exist in the database.
148-187: Good single-result validation.The test correctly verifies both the count and the actual CVE value returned.
pkg/models/epss.go (3)
15-26: LGTM!The model and struct definitions follow established patterns in the codebase. The field names correctly map to the database columns.
28-36: LGTM!The constructor properly initializes all dependencies, including the database query context with trace configuration.
38-55: LGTM!The implementation correctly handles edge cases (empty input), uses
sqlx.Infor safe parameterized queries, and properly rebinds for database driver compatibility.pkg/usecase/vulnerability_use_case.go (2)
39-51: LGTM!The struct definition and constructor follow the same pattern used in
EPSSModel(logger, config, db). The dependency injection pattern is consistent with the codebase.
111-144: EPSS enrichment logic looks correct with graceful error handling.The method correctly:
- Collects CVEs from vulnerabilities
- Returns early if no CVEs exist
- Handles EPSS fetch errors gracefully (logs and continues)
- Uses a map for O(1) lookup when enriching
One minor observation: consider pre-allocating the
cvesslice if you have an estimate of the expected count, though this is optional for typical workloads.pkg/usecase/OSV_use_case.go (2)
86-111: Worker pool implementation looks solid.The pattern correctly:
- Creates buffered channels sized to the number of requests
- Spawns workers before sending jobs
- Closes the jobs channel after sending all work
- Collects exactly
len(requests)resultsThe context with cancel is set up properly but note that
cancel()is only called viadeferwhen the function returns. This is fine for cleanup.
190-236: LGTM!The
mapOSVVulnerabilitiesfunction correctly maps OSV response data to the internal DTO structure with proper handling of aliases, severity, and CVSS scores.
agustingroh
force-pushed
the
feat/SP-3099-epss-info
branch
from
b6d8aa6 to
6dbb490
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @pkg/usecase/OSV_use_case_test.go:
- Around line 31-42: The test creates a global sugared logger via
zlog.NewSugaredDevLogger() but never injects it into the context, so
ctxzap.Extract(ctx).Sugar() returns a no-op logger; fix by attaching the created
logger to the context before extraction (e.g. after zlog.NewSugaredDevLogger()
call do ctx = ctxzap.ToContext(ctx, <the zap logger or zap.Logger from zlog>)
and then call ctxzap.Extract(ctx).Sugar(), ensuring you use the concrete logger
instance returned/accessible from zlog.
In @pkg/usecase/OSV_use_case.go:
- Around line 55-64: The constructor NewOSVUseCase accepts s *zap.SugaredLogger
but never assigns it to the OSVUseCase.s field, causing nil dereference when
processRequest calls us.s.Errorf; update NewOSVUseCase to set the logger field
(OSVUseCase.s = s) when returning the struct so processRequest and any other
methods can safely use us.s.
🧹 Nitpick comments (2)
pkg/config/server_config.go (1)
79-79: Consider adding validation for APIWorkers inIsValidConfig.The
APIWorkersfield has no validation to ensure it's positive. If set to 0 or negative via environment variable, it could cause issues in the OSV worker pool. Adding validation here would catch misconfigurations early.🔎 Proposed fix
func IsValidConfig(cfg *ServerConfig) error { // Check vulnerability source if !cfg.Source.SCANOSS.Enabled && !cfg.Source.OSV.Enabled { return errors.New("at least one vulnerability source provider (SCANOSS or OSV) must be enabled") } // Check OSV source config if cfg.Source.OSV.Enabled { if cfg.Source.OSV.APIBaseURL == "" { return errors.New("OSV API Base URL cannot be empty") } if cfg.Source.OSV.InfoBaseURL == "" { return errors.New("OSV Info Base URL cannot be empty") } + if cfg.Source.OSV.APIWorkers <= 0 { + return errors.New("OSV API Workers must be greater than 0") + } } return nil }pkg/usecase/OSV_use_case.go (1)
209-213: Inconsistent logger usage: global logger vs injected logger.Line 211 uses
zlog.S.Warnfwhile the rest of the file uses the injectedus.slogger. This should be consistent.🔎 Proposed fix
cvssResult, err := utils.GetCVSS(s.Score) if err != nil { - zlog.S.Warnf("Failed to get CVSS severity and score from: %v, %v", s, err) + us.s.Warnf("Failed to get CVSS severity and score from: %v, %v", s, err) continue }After this change, the
zlogimport on line 33 can likely be removed.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
CHANGELOG.mdpkg/adapters/vulnerability_support.gopkg/config/server_config.gopkg/models/all_urls.gopkg/models/all_urls_test.gopkg/models/cpe_purl.gopkg/models/golang_projects.gopkg/models/golang_projects_test.gopkg/models/vulns_purl.gopkg/usecase/OSV_use_case.gopkg/usecase/OSV_use_case_test.gopkg/usecase/vulnerability_use_case.gopkg/utils/purl.gopkg/utils/purl_test.go
💤 Files with no reviewable changes (5)
- pkg/models/all_urls_test.go
- pkg/models/all_urls.go
- pkg/utils/purl_test.go
- pkg/models/golang_projects.go
- pkg/models/golang_projects_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/utils/purl.go
- pkg/models/vulns_purl.go
- pkg/adapters/vulnerability_support.go
- pkg/models/cpe_purl.go
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/usecase/OSV_use_case_test.go (2)
pkg/config/server_config.go (1)
NewServerConfig(88-102)pkg/usecase/OSV_use_case.go (2)
OSVUseCase(47-53)NewOSVUseCase(55-64)
pkg/usecase/OSV_use_case.go (3)
pkg/config/server_config.go (1)
ServerConfig(32-85)pkg/dtos/vulnerability_output.go (2)
VulnerabilityComponentOutput(35-40)VulnerabilityOutput(28-30)pkg/dtos/osv_response_dto.go (2)
Package(168-184)OSVResponseDTO(21-23)
pkg/usecase/vulnerability_use_case.go (6)
pkg/config/server_config.go (1)
ServerConfig(32-85)pkg/dtos/component_dto.go (1)
ComponentDTO(19-23)pkg/dtos/vulnerability_output.go (2)
VulnerabilityOutput(28-30)EPSS(54-57)pkg/usecase/OSV_use_case.go (1)
NewOSVUseCase(55-64)pkg/helpers/vulnerabilty_helper.go (1)
MergeOSVAndLocalVulnerabilities(68-74)pkg/models/epss.go (3)
EPSSModel(15-20)NewEPSSModel(29-36)EPSS(22-26)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~16-~16: Ensure spelling is correct
Context: ...y response ### Changed - Refactored OSV usecase - Upgraded scanoss/papi to v0.28.0 ## [...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (9)
CHANGELOG.md (1)
12-17: LGTM!The changelog entry accurately documents the new EPSS feature, OSV use case refactor, and papi dependency upgrade. The format follows the Keep a Changelog convention.
Note: The static analysis hint about "usecase" spelling is a false positive - it's a common compound term in software development.
pkg/config/server_config.go (1)
125-125: LGTM!Default of 5 workers is a reasonable starting point for the worker pool.
pkg/usecase/OSV_use_case_test.go (1)
61-61: LGTM!The constructor call correctly uses the new
NewOSVUseCase(s, serverConfig)signature.pkg/usecase/vulnerability_use_case.go (3)
37-49: LGTM!The struct and constructor properly implement dependency injection for the logger, config, and database connection. This follows good practices for testability and separation of concerns.
84-88: LGTM!The inconsistent logger usage flagged in the previous review has been addressed. All logging now consistently uses the injected
us.slogger.
109-142: LGTM!The EPSS enrichment implementation is well-structured:
- Graceful error handling that doesn't fail the main response
- Efficient map-based lookup for CVE matching
- Correct slice modification by index
pkg/usecase/OSV_use_case.go (3)
88-113: LGTM!The worker pool implementation is well-structured:
- Buffered channels sized appropriately to prevent blocking
- Workers exit gracefully when jobs channel closes
- Result collection correctly waits for all responses
176-181: LGTM!The previous issue with discarding valid data on body close failure has been addressed. The code now logs the close error but still sends the decoded response.
182-185: LGTM!The previous issue with
fmt.Printlnhas been resolved. Cancellation now uses the structured logger (us.s.Debugf).
agustingroh
force-pushed
the
feat/SP-3099-epss-info
branch
from
6dbb490 to
ed91025
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @CHANGELOG.md:
- Line 16: Update the changelog entry that reads "Refactored OSV usecase" to use
the standard two-word phrasing "Refactored OSV use case" to match documentation
conventions and avoid static analysis flags; edit the string in CHANGELOG.md
accordingly.
In @pkg/usecase/OSV_use_case_test.go:
- Around line 31-42: You’re extracting a logger from an empty context
(ctxzap.Extract(ctx)) which returns a no-op; after calling
zlog.NewSugaredDevLogger() obtain the actual sugared logger directly from the
zlog package and assign it to s (instead of using ctxzap.Extract), e.g. use the
zlog package’s accessor for the sugared logger and set s :=
<zlog-sugar-accessor>() and then remove the unused ctx variable and context
import; replace the ctxzap.Extract(ctx).Sugar() usage with the direct zlog
sugared logger and keep zlog.SyncZap() deferred.
🧹 Nitpick comments (2)
pkg/utils/purl.go (2)
24-25: Comment is slightly misleading.The comment says "regex to extract version" but
PurlRemoveFromVersionComponentuses this regex to remove (not extract) everything from the version component onwards. Consider rewording for clarity:-// regex to extract version from requirement field. +// regex to match version, qualifiers, and subpath components for removal. var fromVRegex = regexp.MustCompile(`(@|\?|#).+`)
27-31: Typo and incomplete comment.Line 27 has "proceds" (should be "proceeds" or "follows") and only mentions
@while the function also strips?(qualifiers) and#(subpath).Suggested fix
-// PurlRemoveFromVersionComponent From a purlString removes everything that proceds @. +// PurlRemoveFromVersionComponent removes version, qualifiers, and subpath from a purl string. // See purl specs scheme:type/namespace/name@version?qualifiers#subpath.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
CHANGELOG.mdpkg/adapters/vulnerability_support.gopkg/config/server_config.gopkg/models/all_urls.gopkg/models/all_urls_test.gopkg/models/cpe_purl.gopkg/models/golang_projects.gopkg/models/golang_projects_test.gopkg/models/vulns_purl.gopkg/usecase/OSV_use_case.gopkg/usecase/OSV_use_case_test.gopkg/usecase/vulnerability_use_case.gopkg/utils/purl.gopkg/utils/purl_test.go
💤 Files with no reviewable changes (5)
- pkg/models/all_urls_test.go
- pkg/utils/purl_test.go
- pkg/models/golang_projects_test.go
- pkg/models/all_urls.go
- pkg/models/golang_projects.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/models/cpe_purl.go
- pkg/models/vulns_purl.go
- pkg/adapters/vulnerability_support.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/usecase/OSV_use_case_test.go (2)
pkg/config/server_config.go (1)
NewServerConfig(88-102)pkg/usecase/OSV_use_case.go (2)
OSVUseCase(47-53)NewOSVUseCase(55-65)
pkg/usecase/vulnerability_use_case.go (4)
pkg/config/server_config.go (1)
ServerConfig(32-85)pkg/dtos/component_dto.go (1)
ComponentDTO(19-23)pkg/dtos/vulnerability_output.go (2)
VulnerabilityOutput(28-30)EPSS(54-57)pkg/models/epss.go (3)
EPSSModel(15-20)NewEPSSModel(29-36)EPSS(22-26)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~16-~16: Ensure spelling is correct
Context: ...y response ### Changed - Refactored OSV usecase - Upgraded scanoss/papi to v0.28.0 ## [...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (7)
pkg/utils/purl.go (2)
33-56: LGTM!The
ProjectURLfunction correctly validates inputs and returns appropriate browsable URLs for each supported package type with proper error handling for unknown types.
19-22: Functions were migrated to external helper, not removed.The functions mentioned were not removed from the codebase—they were migrated to the
purlhelperexternal package. All existing callers properly reference them through the new namespace (e.g.,purlhelper.PurlFromString,purlhelper.GetVersionFromReq), with no dangling references found.Likely an incorrect or invalid review comment.
pkg/config/server_config.go (1)
79-79: LGTM: APIWorkers configuration field added correctly.The new
APIWorkersfield with a sensible default of 5 is appropriate for the OSV worker pool. The env tag follows the existing naming convention.Consider adding validation in
IsValidConfigto ensureAPIWorkers > 0when OSV is enabled, preventing potential deadlocks if set to 0 via environment variable.Also applies to: 125-125
pkg/usecase/vulnerability_use_case.go (2)
43-49: LGTM: Constructor properly injects logger and config.The updated constructor signature properly accepts the logger and config, storing them for consistent use throughout the use case.
109-142: LGTM: EPSS enrichment implementation is well-designed.The enrichment logic is correctly implemented:
- Gracefully handles errors without failing the main response
- Uses an efficient map-based lookup for O(1) CVE matching
- Properly iterates by index to modify the slice in-place
pkg/usecase/OSV_use_case.go (2)
89-114: Worker pool implementation is well-structured.The worker pool pattern is correctly implemented:
- Buffered channels prevent goroutine leaks
- Closing
jobschannel signals workers to exit- Results collection is deterministic (one result per request)
116-188: Worker function handles all paths correctly.The worker implementation properly:
- Handles channel closure to terminate gracefully
- Sends responses on all error paths to maintain result count
- Uses the injected logger consistently
- Handles context cancellation
agustingroh
force-pushed
the
feat/SP-3099-epss-info
branch
from
ed91025 to
07f7a37
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/usecase/OSV_use_case.go (1)
211-211: Inconsistent logger usage: uses global logger instead of injected logger.Line 211 uses
zlog.S.Warnfwhile the rest of the file uses the injectedus.slogger. This should be consistent.🔎 Proposed fix
- zlog.S.Warnf("Failed to get CVSS severity and score from: %v, %v", s, err) + us.s.Warnf("Failed to get CVSS severity and score from: %v, %v", s, err)After applying this fix, you may be able to remove the
zlogimport if it's no longer used elsewhere in the file.
🤖 Fix all issues with AI Agents
In @pkg/usecase/OSV_use_case_test.go:
- Around line 31-37: The test stores zlog.L into the context before the logger
is initialized, causing ctxzap.Extract(ctx).Sugar() to panic; fix by
initializing the logger first via zlog.NewSugaredDevLogger(), defer
zlog.SyncZap(), then create the context with
ctxzap.ToContext(context.Background(), zlog.L) and finally call
ctxzap.Extract(ctx).Sugar(); update the order of calls involving
zlog.NewSugaredDevLogger, zlog.SyncZap, ctxzap.ToContext, and ctxzap.Extract to
ensure zlog.L is non-nil when added to the context.
In @pkg/usecase/OSV_use_case.go:
- Around line 55-65: NewOSVUseCase currently assigns MaxAPIWorkers directly from
config.Source.OSV.APIWorkers which can be 0 or negative and cause the worker
loop to deadlock; validate and clamp that value when constructing the OSVUseCase
(in NewOSVUseCase) so MaxAPIWorkers is at least 1 (e.g., if
config.Source.OSV.APIWorkers <= 0 then set MaxAPIWorkers = 1) or otherwise apply
a safe min/max clamp before assigning to the OSVUseCase.MaxAPIWorkers field so
the worker-creation logic never receives zero workers.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
CHANGELOG.mdpkg/adapters/vulnerability_support.gopkg/config/server_config.gopkg/models/all_urls.gopkg/models/all_urls_test.gopkg/models/cpe_purl.gopkg/models/golang_projects.gopkg/models/golang_projects_test.gopkg/models/vulns_purl.gopkg/usecase/OSV_use_case.gopkg/usecase/OSV_use_case_test.gopkg/usecase/vulnerability_use_case.gopkg/utils/purl.gopkg/utils/purl_test.go
💤 Files with no reviewable changes (5)
- pkg/models/all_urls_test.go
- pkg/models/all_urls.go
- pkg/models/golang_projects.go
- pkg/utils/purl_test.go
- pkg/models/golang_projects_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/models/cpe_purl.go
- pkg/models/vulns_purl.go
- pkg/config/server_config.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/usecase/OSV_use_case_test.go (2)
pkg/config/server_config.go (1)
NewServerConfig(88-102)pkg/usecase/OSV_use_case.go (2)
OSVUseCase(47-53)NewOSVUseCase(55-65)
pkg/usecase/OSV_use_case.go (2)
pkg/dtos/vulnerability_output.go (2)
VulnerabilityComponentOutput(35-40)VulnerabilityOutput(28-30)pkg/dtos/osv_response_dto.go (2)
Package(168-184)OSVResponseDTO(21-23)
🪛 GitHub Actions: Go Unit Test
pkg/usecase/OSV_use_case_test.go
[error] 37-37: OSVUseCase test panicked: runtime error: invalid memory address or nil pointer dereference
🔇 Additional comments (10)
CHANGELOG.md (2)
12-18: ✅ Changelog entry properly structured and accurately reflects PR changes.The 0.8.0 section is well-formatted and aligns with the PR objectives. The "use case" spelling has been correctly updated to two words (addressing the previous review feedback). The entries accurately capture the EPSS addition and OSV refactoring work, along with the papi dependency bump to v0.28.0.
91-93: ✅ Version link references properly maintained.The updated version links follow the established format and correctly reference the version comparisons for 0.6.2, 0.7.0, and 0.8.0.
pkg/adapters/vulnerability_support.go (1)
25-25: LGTM! Refactor is clean with consistent dependency management.The migration to the external
go-purl-helperpackage is well-executed. The import aliaspurlhelperis consistently applied across the codebase (vulns_purl.go, cpe_purl.go, and vulnerability_support.go), the dependency is properly managed in go.mod, and all instances of the oldutils.PurlFromStringhave been successfully migrated.pkg/utils/purl.go (1)
23-30: Refactor successfully completed with full migration verified.This change properly addresses the previous review feedback by moving PURL utilities to the external
go-purl-helperpackage. All six removed functions have been successfully migrated with zero orphaned usages:
purlhelper.PurlFromStringnow handles validation invulns_purl.go,cpe_purl.go, andvulnerability_support.gopurlhelper.GetVersionFromReqnow handles version extraction incpe_purl.goThe retained
PurlRemoveFromVersionComponentfunction is appropriately kept as domain-specific logic actively used invulns_purl.goandcpe_purl.go.pkg/usecase/OSV_use_case_test.go (1)
39-69: Test structure looks good after the logger fix.Once the logger initialization order is corrected, the test properly wires the
OSVUseCasewith the config-based constructor and validates that the execution returns components.pkg/usecase/vulnerability_use_case.go (3)
106-137: EPSS enrichment implementation looks solid.The method efficiently collects CVEs, uses a map for O(1) lookup, and gracefully handles errors without failing the entire request. The pointer receiver pattern correctly allows in-place mutation of the vulnerabilities.
83-87: Logger usage is now consistent.The OSV use case construction properly passes the injected logger and config. The log level was appropriately changed to
Debugfas suggested in past reviews.
56-62: > Likely an incorrect or invalid review comment.pkg/usecase/OSV_use_case.go (2)
89-114: Worker pool implementation is well-structured.The implementation correctly:
- Stores
len(requests)once asnumJobs- Uses buffered channels sized to prevent blocking
- Limits workers to
min(MaxAPIWorkers, numJobs)to avoid excess goroutines- Adds a 2-minute context timeout for the overall operation
116-188: Worker function implementation is correct.The worker properly:
- Handles channel closure to terminate gracefully
- Uses the injected logger consistently (
us.s)- Closes response bodies on error paths
- No longer discards valid data on body close failure (line 176-179)
- Handles context cancellation with structured logging
agustingroh
force-pushed
the
feat/SP-3099-epss-info
branch
2 times, most recently
from
abf7e23 to
2384d95
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @pkg/config/server_config.go:
- Line 125: Add validation in IsValidConfig to ensure the OSV API worker count
is positive: check cfg.Source.OSV.APIWorkers (and any exported alias
MaxAPIWorkers if present) and return an error when it is <= 0 so downstream code
like usecase.OSVUseCase (which uses min(us.MaxAPIWorkers, numJobs)) cannot spawn
zero workers; update the validation message to mention the invalid APIWorkers
value and require a positive integer.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
CHANGELOG.mdpkg/adapters/vulnerability_support.gopkg/config/server_config.gopkg/models/all_urls.gopkg/models/all_urls_test.gopkg/models/cpe_purl.gopkg/models/golang_projects.gopkg/models/golang_projects_test.gopkg/models/vulns_purl.gopkg/usecase/OSV_use_case.gopkg/usecase/OSV_use_case_test.gopkg/usecase/vulnerability_use_case.gopkg/utils/purl.gopkg/utils/purl_test.go
💤 Files with no reviewable changes (5)
- pkg/utils/purl_test.go
- pkg/models/golang_projects_test.go
- pkg/models/golang_projects.go
- pkg/models/all_urls.go
- pkg/models/all_urls_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/adapters/vulnerability_support.go
- pkg/models/vulns_purl.go
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/usecase/OSV_use_case_test.go (2)
pkg/config/server_config.go (1)
NewServerConfig(88-102)pkg/usecase/OSV_use_case.go (2)
OSVUseCase(47-53)NewOSVUseCase(55-65)
pkg/usecase/OSV_use_case.go (3)
pkg/config/server_config.go (1)
ServerConfig(32-85)pkg/dtos/vulnerability_output.go (2)
VulnerabilityComponentOutput(35-40)VulnerabilityOutput(28-30)pkg/dtos/osv_response_dto.go (2)
Package(168-184)OSVResponseDTO(21-23)
pkg/usecase/vulnerability_use_case.go (3)
pkg/config/server_config.go (1)
ServerConfig(32-85)pkg/usecase/OSV_use_case.go (1)
NewOSVUseCase(55-65)pkg/models/epss.go (3)
EPSSModel(15-20)NewEPSSModel(29-36)EPSS(22-26)
🔇 Additional comments (14)
CHANGELOG.md (2)
12-18: Changelog entries look good; previous spelling issue resolved.The [0.8.0] section is well-structured and clearly documents the EPSS addition and OSV refactor. The spelling of "use case" (line 16) has been properly corrected from the previous review feedback flagging "usecase" as non-standard—this is now fixed.
91-93: Version links properly maintained.The comparison links for [0.6.2], [0.7.0], and [0.8.0] follow the established pattern and are correctly ordered with appropriate commit ranges.
pkg/models/cpe_purl.go (2)
26-26: LGTM: External PURL helper library adopted.The migration to the external
go-purl-helperpackage consolidates PURL parsing logic as suggested in previous reviews.
53-53: Verify external library behavior matches internal implementation.Ensure that
purlhelper.PurlFromStringandpurlhelper.GetVersionFromReqhave identical behavior, error handling, and return signatures to the removed internal implementations. Check that all call sites have been migrated to use the new purlhelper functions and that no references to the old utils PURL functions remain in the codebase.pkg/config/server_config.go (1)
79-79: LGTM: APIWorkers field added for configurable concurrency.The new configuration field allows runtime control of OSV API worker pool size, which aligns well with the worker pool refactoring in the OSV use case.
pkg/usecase/OSV_use_case_test.go (3)
31-37: LGTM: Logger initialization order is correct.The logger is now properly initialized before being added to the context, which resolves the previous nil pointer dereference issue. The sequence is correct: initialize logger → defer sync → add to context → extract from context.
39-42: LGTM: Configuration loading with proper error handling.The server configuration is correctly loaded and error handling is in place for test reliability.
61-61: LGTM: Constructor updated to use injected logger and config.The OSV use case constructor now correctly receives the logger and server configuration, aligning with the dependency injection pattern introduced in this PR.
pkg/usecase/vulnerability_use_case.go (4)
26-27: LGTM: Logger injection properly integrated.The sugared logger is correctly added to the struct and constructor signature, enabling structured logging throughout the use case.
Also applies to: 40-40, 47-47
52-52: LGTM: Consistent use of injected logger.All logging calls now use the injected logger
us.sinstead of the global logger, maintaining consistency and enabling better test isolation.Also applies to: 56-56, 59-59, 73-73, 84-84, 95-95
85-85: LGTM: OSV use case instantiation updated with dependency injection.The OSV use case constructor now receives the injected logger and config, matching the updated signature.
106-137: LGTM: EPSS enrichment implemented with graceful error handling.The
enrichWithEPSSmethod properly:
- Collects unique CVEs from all vulnerabilities
- Handles empty CVE lists efficiently (early return)
- Fetches EPSS data and builds a lookup map
- Enriches vulnerabilities with EPSS probability and percentile
- Logs errors without blocking the main response flow
The graceful degradation on errors ensures that EPSS data unavailability doesn't break the API response.
pkg/usecase/OSV_use_case.go (2)
89-114: LGTM: Worker pool pattern correctly implemented.The refactored
processRequestsmethod properly:
- Creates buffered channels sized to the number of jobs
- Spawns an appropriate number of workers using
min(MaxAPIWorkers, numJobs)- Feeds jobs into the channel and closes it to signal completion
- Collects exactly
numJobsresults from the results channel- Uses a context with timeout for cancellation
The pattern is sound assuming
MaxAPIWorkersis validated to be positive (see critical issue raised above).
116-188: LGTM: Worker function correctly implements request processing.The
processRequestworker function properly:
- Reads jobs from the channel until it's closed
- Respects context cancellation for graceful shutdown
- Uses the shared HTTP client to avoid resource overhead
- Handles errors at each stage (marshal, request creation, HTTP call, decode)
- Sends a default response on errors to prevent blocking the results channel
- Closes the response body after reading
- Uses structured logging throughout
The error handling ensures that individual request failures don't block the entire batch, and the response body is properly closed even on non-200 status codes.
agustingroh
force-pushed
the
feat/SP-3099-epss-info
branch
2 times, most recently
from
e803271 to
15d2ff0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @pkg/config/server_config.go:
- Line 79: The config field APIWorkers (int) can be zero or negative and
IsValidConfig currently doesn't check it; add validation in the IsValidConfig
function to ensure APIWorkers (a.k.a. MaxAPIWorkers where referenced by the use
case) is > 0 and return an error if not so that callers (e.g., usecase.OSV use
of min(us.MaxAPIWorkers, numJobs)) never receive non-positive worker counts;
apply the same positive-check pattern for any other related fields noted (the
duplicate at the second occurrence).
In @pkg/usecase/OSV_use_case.go:
- Around line 55-65: The constructor NewOSVUseCase sets OSVUseCase.MaxAPIWorkers
directly from config.Source.OSV.APIWorkers which can be zero or negative and
cause processRequests to deadlock; update NewOSVUseCase to validate and
normalize APIWorkers (e.g., if config.Source.OSV.APIWorkers <= 0 set
MaxAPIWorkers to 1 or a sensible default and optionally clamp to a reasonable
max) so that workers := min(us.MaxAPIWorkers, numJobs) always yields at least
one worker.
🧹 Nitpick comments (2)
pkg/utils/purl.go (2)
23-24: Clarify the comment - it describes removal, not extraction.The comment states "regex to extract version from requirement field," but the regex is actually used in
PurlRemoveFromVersionComponent(line 29) to remove everything that follows@,?, or#symbols, not to extract the version. The comment should reflect this removal behavior.🔎 Proposed fix
-// regex to extract version from requirement field. +// regex to match the version, qualifiers, and subpath components in a PURL string (starting from @, ?, or #). var fromVRegex = regexp.MustCompile(`(@|\?|#).+`)
28-30: Consider completing the migration to the shared library.Based on a previous review comment, most PURL-related functions have been migrated to the external
go-purl-helperlibrary. However,PurlRemoveFromVersionComponentremains in this internal utils package. Consider whether this function should also be migrated to the shared library for consistency and to avoid maintaining duplicate PURL manipulation logic across repositories.Based on past review comments questioning whether this should be in the shared library.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
CHANGELOG.mdpkg/adapters/vulnerability_support.gopkg/config/server_config.gopkg/models/all_urls.gopkg/models/all_urls_test.gopkg/models/cpe_purl.gopkg/models/golang_projects.gopkg/models/golang_projects_test.gopkg/models/vulns_purl.gopkg/usecase/OSV_use_case.gopkg/usecase/OSV_use_case_test.gopkg/usecase/vulnerability_use_case.gopkg/utils/purl.gopkg/utils/purl_test.go
💤 Files with no reviewable changes (5)
- pkg/models/all_urls_test.go
- pkg/utils/purl_test.go
- pkg/models/all_urls.go
- pkg/models/golang_projects_test.go
- pkg/models/golang_projects.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/adapters/vulnerability_support.go
- CHANGELOG.md
- pkg/usecase/OSV_use_case_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/usecase/OSV_use_case.go (3)
pkg/config/server_config.go (1)
ServerConfig(32-85)pkg/dtos/vulnerability_output.go (2)
VulnerabilityComponentOutput(35-40)VulnerabilityOutput(28-30)pkg/dtos/osv_response_dto.go (2)
Package(168-184)OSVResponseDTO(21-23)
pkg/usecase/vulnerability_use_case.go (6)
pkg/config/server_config.go (1)
ServerConfig(32-85)pkg/dtos/component_dto.go (1)
ComponentDTO(19-23)pkg/dtos/vulnerability_output.go (2)
VulnerabilityOutput(28-30)EPSS(54-57)pkg/usecase/OSV_use_case.go (1)
NewOSVUseCase(55-65)pkg/helpers/vulnerabilty_helper.go (1)
MergeOSVAndLocalVulnerabilities(68-74)pkg/models/epss.go (3)
EPSSModel(15-20)NewEPSSModel(29-36)EPSS(22-26)
🔇 Additional comments (8)
pkg/usecase/vulnerability_use_case.go (3)
43-49: LGTM!The constructor refactor correctly implements dependency injection for the logger and config, consistent with patterns used elsewhere in the codebase (e.g.,
EPSSModel).
52-52: Logger usage is now consistent.All logging statements correctly use the injected logger (
us.s) instead of the global logger. This addresses the previous review concern about inconsistent logger usage.Also applies to: 56-56, 59-59, 73-73, 84-84, 95-95
106-137: LGTM!The
enrichWithEPSSmethod is well-structured:
- Efficiently collects CVEs and returns early if none exist
- Uses a map for O(1) lookup when patching vulnerability records
- Handles errors gracefully with logging
- Correctly mutates the input pointer to enrich vulnerability data
pkg/usecase/OSV_use_case.go (3)
89-114: Worker pool implementation is well-structured.The refactor from semaphore to worker pool pattern is correct:
- Buffered channels are appropriately sized for
numJobs- Context with 2-minute timeout enables graceful cancellation
- Jobs channel is closed after all jobs are sent, allowing workers to terminate
- Result collection loop correctly waits for exactly
numJobsresponsesmin(us.MaxAPIWorkers, numJobs)prevents spawning unnecessary workersThe implementation will work correctly once the constructor validates
MaxAPIWorkers > 0.
116-188: Worker function correctly implements concurrent request processing.The worker implementation is sound:
- Properly handles both job processing and context cancellation via select
- Returns gracefully when the jobs channel is closed or context is cancelled
- Uses the shared HTTP client to avoid overhead
- Explicitly closes response bodies to prevent resource leaks
- Sends a default response on errors to prevent result channel deadlock
- Uses the injected logger consistently
60-60: Timeout configuration is reasonable.The HTTP client timeout of 15 seconds per request combined with the 2-minute context timeout for the overall batch provides a good balance between responsiveness and reliability for concurrent API calls.
Also applies to: 92-92
pkg/models/vulns_purl.go (1)
25-25: Migration to external PURL helper is complete and correct.The change from
utils.PurlFromStringtopurlhelper.PurlFromStringhas been fully migrated across the codebase (confirmed at line 63). The externalgo-purl-helperlibrary is properly imported and used. No remaining references to the old function signature exist.pkg/models/cpe_purl.go (1)
26-26: PURL helper migration is complete and consistent across the codebase.The migration from the internal
utils.PurlFromStringandutils.GetVersionFromReqfunctions to theirpurlhelperequivalents is properly executed:
- Old utility functions removed entirely from
pkg/utils/- New functions actively used in three locations (cpe_purl.go, vulns_purl.go, vulnerability_support.go)
- Usage patterns confirm expected function signatures (PurlFromString returns object with
.Versionfield; GetVersionFromReq returns string)- External dependency properly declared in go.mod (v0.2.1)
The external library
github.com/scanoss/go-purl-helperis not publicly documented. To fully confirm API compatibility, review the private repository or library documentation directly.
| APIBaseURL string `env:"VULN_OSV_API_BASE_URL"` | ||
| InfoBaseURL string `env:"VULN_OSV_INFO_BASE_URL"` | ||
| Enabled bool `env:"VULN_OSV_SOURCE_ENABLED"` | ||
| APIWorkers int `env:"VULN_OSV_API_WORKERS"` |
There was a problem hiding this comment.
Add validation to prevent zero or negative APIWorkers.
The configuration lacks validation in IsValidConfig to ensure APIWorkers is positive. Line 95 in pkg/usecase/OSV_use_case.go uses min(us.MaxAPIWorkers, numJobs), which would spawn zero workers and cause a deadlock if MaxAPIWorkers is 0 or negative.
🔎 Proposed fix
Add validation in the IsValidConfig function:
// Check OSV source config
if cfg.Source.OSV.Enabled {
if cfg.Source.OSV.APIBaseURL == "" {
return errors.New("OSV API Base URL cannot be empty")
}
if cfg.Source.OSV.InfoBaseURL == "" {
return errors.New("OSV Info Base URL cannot be empty")
}
+ if cfg.Source.OSV.APIWorkers <= 0 {
+ return errors.New("OSV API Workers must be greater than 0")
+ }
}Also applies to: 125-125
🤖 Prompt for AI Agents
In @pkg/config/server_config.go at line 79, The config field APIWorkers (int)
can be zero or negative and IsValidConfig currently doesn't check it; add
validation in the IsValidConfig function to ensure APIWorkers (a.k.a.
MaxAPIWorkers where referenced by the use case) is > 0 and return an error if
not so that callers (e.g., usecase.OSV use of min(us.MaxAPIWorkers, numJobs))
never receive non-positive worker counts; apply the same positive-check pattern
for any other related fields noted (the duplicate at the second occurrence).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @pkg/usecase/OSV_use_case.go:
- Around line 55-65: NewOSVUseCase assigns MaxAPIWorkers from
config.Source.OSV.APIWorkers without validation, which can be zero/negative and
cause processRequests to deadlock when no workers are spawned; update
NewOSVUseCase to validate and coerce config.Source.OSV.APIWorkers into a sane
positive value (e.g., set a minimum of 1 and optionally cap to a reasonable max)
before assigning to OSVUseCase.MaxAPIWorkers, and ensure any invalid config
triggers a warning via the provided logger (s) or falls back to the default to
avoid spawning zero workers.
🧹 Nitpick comments (1)
pkg/utils/purl.go (1)
23-30: Consider consolidating remaining PURL utility to external library.Most PURL utilities have been successfully migrated to the external
go-purl-helperpackage. For consistency, evaluate whetherPurlRemoveFromVersionComponentshould also be part of the external library, especially since the past review comments suggested consolidating PURL utilities into a shared library.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
CHANGELOG.mdpkg/adapters/vulnerability_support.gopkg/config/server_config.gopkg/models/all_urls.gopkg/models/all_urls_test.gopkg/models/cpe_purl.gopkg/models/golang_projects.gopkg/models/golang_projects_test.gopkg/models/vulns_purl.gopkg/usecase/OSV_use_case.gopkg/usecase/OSV_use_case_test.gopkg/usecase/vulnerability_use_case.gopkg/utils/purl.gopkg/utils/purl_test.go
💤 Files with no reviewable changes (5)
- pkg/models/all_urls_test.go
- pkg/models/golang_projects.go
- pkg/utils/purl_test.go
- pkg/models/all_urls.go
- pkg/models/golang_projects_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/config/server_config.go
- pkg/models/vulns_purl.go
- CHANGELOG.md
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/usecase/OSV_use_case.go (3)
pkg/config/server_config.go (1)
ServerConfig(32-85)pkg/dtos/vulnerability_output.go (2)
VulnerabilityComponentOutput(35-40)VulnerabilityOutput(28-30)pkg/dtos/osv_response_dto.go (2)
Package(168-184)OSVResponseDTO(21-23)
pkg/usecase/OSV_use_case_test.go (2)
pkg/config/server_config.go (1)
NewServerConfig(88-102)pkg/usecase/OSV_use_case.go (2)
OSVUseCase(47-53)NewOSVUseCase(55-65)
pkg/usecase/vulnerability_use_case.go (6)
pkg/config/server_config.go (1)
ServerConfig(32-85)pkg/dtos/component_dto.go (1)
ComponentDTO(19-23)pkg/dtos/vulnerability_output.go (2)
VulnerabilityOutput(28-30)EPSS(54-57)pkg/usecase/OSV_use_case.go (1)
NewOSVUseCase(55-65)pkg/helpers/vulnerabilty_helper.go (1)
MergeOSVAndLocalVulnerabilities(68-74)pkg/models/epss.go (3)
EPSSModel(15-20)NewEPSSModel(29-36)EPSS(22-26)
🔇 Additional comments (10)
pkg/adapters/vulnerability_support.go (2)
37-37: Migration to external PURL helper looks good.The refactoring consolidates PURL parsing to the external helper library, addressing the past review concerns about using a shared library.
25-25: The external PURL helper librarygithub.com/scanoss/go-purl-helper v0.2.1is available ingo.modand thePurlFromStringfunction is properly imported and used consistently across the codebase with correct error handling. No issues found.pkg/models/cpe_purl.go (1)
26-26: PURL parsing migration implemented correctly.The migration to the external PURL helper library is applied consistently across both
PurlFromStringandGetVersionFromReqfunction calls.Also applies to: 53-53, 62-62
pkg/usecase/OSV_use_case_test.go (2)
31-42: Logger initialization and test setup look good.The logger initialization order has been corrected—
zlog.NewSugaredDevLogger()is called before adding the logger to the context. The server config is properly loaded with error handling, and the test correctly uses the new constructor signature.
61-61: OSV use case instantiation follows the new constructor pattern.The test correctly instantiates the OSV use case with the sugared logger and server config.
pkg/usecase/OSV_use_case.go (2)
89-114: Worker pool implementation follows correct patterns.The worker pool correctly:
- Spawns workers bounded by both
MaxAPIWorkersandnumJobs- Closes the jobs channel after feeding all requests
- Collects exactly
numJobsresults from the results channel- Uses context with a 3-minute timeout for cancellation
116-188: Worker function is well-implemented with proper error handling.The worker function:
- Includes clear documentation
- Correctly handles channel closure and context cancellation
- Uses the injected logger for all error cases
- Ensures a response is always sent to the results channel, preventing deadlock
- Properly closes the response body after decoding
pkg/usecase/vulnerability_use_case.go (3)
43-49: Constructor correctly updated with logger dependency.The constructor now accepts a
SugaredLoggerand properly initializes the logger field, enabling consistent structured logging throughout the use case.
52-95: Consistent logger usage throughout Execute method.All logging statements now use the injected logger
us.srather than the global logger, maintaining consistency. The OSV use case is correctly instantiated with the logger and config.
106-137: EPSS enrichment implemented efficiently.The
enrichWithEPSSmethod:
- Collects all CVEs in a single pass
- Returns early if no CVEs are found, avoiding unnecessary database calls
- Uses a map for O(1) lookups when matching EPSS data to vulnerabilities
- Logs errors but doesn't fail the entire request if EPSS data is unavailable, maintaining resilience
The implementation follows good practices for optional enrichment.
agustingroh
force-pushed
the
feat/SP-3099-epss-info
branch
from
15d2ff0 to
0326017
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @pkg/usecase/OSV_use_case.go:
- Around line 55-65: NewOSVUseCase assigns MaxAPIWorkers from
config.Source.OSV.APIWorkers without validation which can be zero or negative
and cause processRequests to deadlock; update NewOSVUseCase to validate the
value and set OSVUseCase.MaxAPIWorkers to a sane default (e.g., 1 or
runtime.NumCPU()) when config.Source.OSV.APIWorkers <= 0 so that at least one
worker is spawned; ensure the change references OSVUseCase.MaxAPIWorkers and
NewOSVUseCase and mention processRequests in the comment or log so future
reviewers understand why the validation exists.
🧹 Nitpick comments (1)
pkg/usecase/OSV_use_case.go (1)
92-92: Consider making the context timeout configurable.The hardcoded 3-minute timeout may be insufficient for large batches of vulnerability queries, especially under slow network conditions or high API latency. Consider making this configurable via
config.Source.OSVor dynamically scaling it based onnumJobs.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
CHANGELOG.mdpkg/adapters/vulnerability_support.gopkg/config/server_config.gopkg/models/all_urls.gopkg/models/all_urls_test.gopkg/models/cpe_purl.gopkg/models/golang_projects.gopkg/models/golang_projects_test.gopkg/models/vulns_purl.gopkg/usecase/OSV_use_case.gopkg/usecase/OSV_use_case_test.gopkg/usecase/vulnerability_use_case.gopkg/usecase/vulnerability_use_case_test.gopkg/utils/purl.gopkg/utils/purl_test.go
💤 Files with no reviewable changes (5)
- pkg/models/all_urls_test.go
- pkg/models/all_urls.go
- pkg/models/golang_projects_test.go
- pkg/utils/purl_test.go
- pkg/models/golang_projects.go
🚧 Files skipped from review as they are similar to previous changes (3)
- CHANGELOG.md
- pkg/models/vulns_purl.go
- pkg/models/cpe_purl.go
🧰 Additional context used
🧬 Code graph analysis (4)
pkg/usecase/OSV_use_case_test.go (2)
pkg/config/server_config.go (1)
NewServerConfig(88-102)pkg/usecase/OSV_use_case.go (2)
OSVUseCase(47-53)NewOSVUseCase(55-65)
pkg/usecase/OSV_use_case.go (2)
pkg/dtos/vulnerability_output.go (2)
VulnerabilityComponentOutput(35-40)VulnerabilityOutput(28-30)pkg/dtos/osv_response_dto.go (2)
Package(168-184)OSVResponseDTO(21-23)
pkg/usecase/vulnerability_use_case.go (4)
pkg/config/server_config.go (1)
ServerConfig(32-85)pkg/dtos/vulnerability_output.go (2)
VulnerabilityOutput(28-30)EPSS(54-57)pkg/usecase/OSV_use_case.go (1)
NewOSVUseCase(55-65)pkg/models/epss.go (3)
EPSSModel(15-20)NewEPSSModel(29-36)EPSS(22-26)
pkg/usecase/vulnerability_use_case_test.go (1)
pkg/usecase/vulnerability_use_case.go (1)
NewVulnerabilityUseCase(43-49)
🔇 Additional comments (12)
pkg/adapters/vulnerability_support.go (1)
25-25: LGTM! Clean migration to external PURL helper.The import of the external
go-purl-helperpackage and the substitution ofpurlhelper.PurlFromStringfor the internalutils.PurlFromStringis a straightforward refactor that consolidates PURL parsing logic into a shared library.Also applies to: 37-37
pkg/utils/purl.go (1)
23-30: LGTM! Appropriate cleanup after migrating to external PURL helper.The removal of PURL parsing utilities that are now provided by the external
go-purl-helperlibrary is appropriate. RetainingPurlRemoveFromVersionComponentmakes sense as it appears to be a domain-specific utility for stripping version components from PURL strings.pkg/usecase/vulnerability_use_case_test.go (2)
24-24: LGTM! Logger context setup is correct.The logger initialization sequence is correct: the sugared logger is initialized first, then added to the context, and finally extracted for use. This properly addresses the past review comment.
Also applies to: 42-43
63-63: LGTM! Constructor call matches new signature.The call to
NewVulnerabilityUseCase(s, serverConfig, db)correctly matches the updated constructor signature that accepts a sugared logger, server config, and database connection in that order.pkg/usecase/OSV_use_case_test.go (3)
36-37: LGTM! Logger initialization order is correct.The logger is properly initialized before being added to the context, preventing the nil pointer dereference that was flagged in past reviews. The sequence (initialize logger → add to context → extract sugared logger) is correct.
39-42: LGTM! Config loading with proper error handling.Loading the server configuration with error handling ensures that the OSV use case is initialized with valid configuration values rather than hard-coded URLs.
61-61: LGTM! Constructor call matches new signature.The call to
NewOSVUseCase(s, serverConfig)correctly passes the sugared logger and server configuration, aligning with the refactored constructor that supports config-driven initialization and logger injection.pkg/config/server_config.go (1)
79-79: Add validation to prevent zero or negative APIWorkers.The configuration lacks validation to ensure
APIWorkersis positive. Without this check, settingVULN_OSV_API_WORKERS=0via environment variables would cause the OSV worker pool to spawn zero workers, leading to a deadlock when jobs are submitted.🔎 Proposed fix
Add validation in the
IsValidConfigfunction:// Check OSV source config if cfg.Source.OSV.Enabled { if cfg.Source.OSV.APIBaseURL == "" { return errors.New("OSV API Base URL cannot be empty") } if cfg.Source.OSV.InfoBaseURL == "" { return errors.New("OSV Info Base URL cannot be empty") } + if cfg.Source.OSV.APIWorkers <= 0 { + return errors.New("OSV API Workers must be greater than 0") + } }Also applies to: 125-125
Likely an incorrect or invalid review comment.
pkg/usecase/OSV_use_case.go (1)
120-188: Worker implementation looks solid.The worker correctly:
- Handles context cancellation (lines 182-186)
- Always sends a response to prevent deadlock (lines 141, 147, 156, 166, 173, 181)
- Uses the shared HTTP client (line 153)
- Closes response bodies on error paths (lines 162-165)
- Logs errors with the injected logger
The error-handling pattern (sending partial responses on failure) ensures the result channel receives exactly
numJobsresults, which is essential for the collection logic at lines 109-111.pkg/usecase/vulnerability_use_case.go (3)
43-49: Constructor updated correctly.The updated constructor properly injects the logger and stores the config reference. The signature change aligns with the OSV use case wiring at line 85.
106-137: EPSS enrichment implementation is well-structured.The method correctly:
- Collects CVEs from all components and vulnerabilities (lines 108-115)
- Short-circuits if no CVEs are found (lines 116-118)
- Uses a map for O(1) lookup during enrichment (lines 125-128)
- Logs errors without failing the entire request (lines 121-124)
- Mutates the vulnerabilities in place (lines 132-133)
The CVE list may contain duplicates, but the database query will handle deduplication, so this is acceptable.
52-95: Logger usage is now consistent.All logging calls now use the injected logger
us.sinstead of the globalzlog.S. This addresses the inconsistency flagged in previous reviews (line 84 specifically).
| func NewOSVUseCase(s *zap.SugaredLogger, config *config.ServerConfig) *OSVUseCase { | ||
| return &OSVUseCase{ | ||
| OSVAPIBaseURL: osvAPIBaseURL, | ||
| OSVInfoBaseURL: osvInfoBaseURL, | ||
| semaphore: make(chan struct{}, 4), | ||
| OSVAPIBaseURL: config.Source.OSV.APIBaseURL, | ||
| OSVInfoBaseURL: config.Source.OSV.InfoBaseURL, | ||
| client: &http.Client{ | ||
| Timeout: 10 * time.Second, | ||
| Timeout: 15 * time.Second, | ||
| }, | ||
| MaxAPIWorkers: config.Source.OSV.APIWorkers, | ||
| s: s, | ||
| } | ||
| } |
There was a problem hiding this comment.
Add validation for MaxAPIWorkers to prevent deadlock.
The constructor still assigns MaxAPIWorkers directly from config.Source.OSV.APIWorkers without validation. If this value is 0 or negative, line 96 will spawn zero workers, causing processRequests to deadlock at lines 109-111 when waiting for results that no worker will produce.
This critical issue has been flagged twice in previous reviews but remains unaddressed.
🔒 Proposed fix to validate MaxAPIWorkers
func NewOSVUseCase(s *zap.SugaredLogger, config *config.ServerConfig) *OSVUseCase {
+ maxWorkers := config.Source.OSV.APIWorkers
+ if maxWorkers <= 0 {
+ s.Warnf("Invalid OSV API workers config (%d), defaulting to 5", maxWorkers)
+ maxWorkers = 5
+ }
return &OSVUseCase{
OSVAPIBaseURL: config.Source.OSV.APIBaseURL,
OSVInfoBaseURL: config.Source.OSV.InfoBaseURL,
client: &http.Client{
Timeout: 15 * time.Second,
},
- MaxAPIWorkers: config.Source.OSV.APIWorkers,
+ MaxAPIWorkers: maxWorkers,
s: s,
}
}📝 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 NewOSVUseCase(s *zap.SugaredLogger, config *config.ServerConfig) *OSVUseCase { | |
| return &OSVUseCase{ | |
| OSVAPIBaseURL: osvAPIBaseURL, | |
| OSVInfoBaseURL: osvInfoBaseURL, | |
| semaphore: make(chan struct{}, 4), | |
| OSVAPIBaseURL: config.Source.OSV.APIBaseURL, | |
| OSVInfoBaseURL: config.Source.OSV.InfoBaseURL, | |
| client: &http.Client{ | |
| Timeout: 10 * time.Second, | |
| Timeout: 15 * time.Second, | |
| }, | |
| MaxAPIWorkers: config.Source.OSV.APIWorkers, | |
| s: s, | |
| } | |
| } | |
| func NewOSVUseCase(s *zap.SugaredLogger, config *config.ServerConfig) *OSVUseCase { | |
| maxWorkers := config.Source.OSV.APIWorkers | |
| if maxWorkers <= 0 { | |
| s.Warnf("Invalid OSV API workers config (%d), defaulting to 5", maxWorkers) | |
| maxWorkers = 5 | |
| } | |
| return &OSVUseCase{ | |
| OSVAPIBaseURL: config.Source.OSV.APIBaseURL, | |
| OSVInfoBaseURL: config.Source.OSV.InfoBaseURL, | |
| client: &http.Client{ | |
| Timeout: 15 * time.Second, | |
| }, | |
| MaxAPIWorkers: maxWorkers, | |
| s: s, | |
| } | |
| } |
🤖 Prompt for AI Agents
In @pkg/usecase/OSV_use_case.go around lines 55 - 65, NewOSVUseCase assigns
MaxAPIWorkers from config.Source.OSV.APIWorkers without validation which can be
zero or negative and cause processRequests to deadlock; update NewOSVUseCase to
validate the value and set OSVUseCase.MaxAPIWorkers to a sane default (e.g., 1
or runtime.NumCPU()) when config.Source.OSV.APIWorkers <= 0 so that at least one
worker is spawned; ensure the change references OSVUseCase.MaxAPIWorkers and
NewOSVUseCase and mention processRequests in the comment or log so future
reviewers understand why the validation exists.
Summary by CodeRabbit
New Features
Changed
Removed
Tests
✏️ Tip: You can customize this high-level summary in your review settings.