feat(vulnerability-response):SP-4015 include status in vulnerability…#32
Conversation
📝 WalkthroughWalkthroughAdds a new exported Changes
Sequence DiagramsequenceDiagram
autonumber
participant Client as Client
participant Adapter as Adapter
participant Helper as Helper (Sanitize / Version)
participant UseCase as UseCase (OSV/CPE/Local)
participant SCANOSS as SCANOSS API/DB
Client->>Adapter: submit request (ComponentDTOs)
Adapter->>Helper: SanitizeComponents(DTOs)
Helper-->>Adapter: []entities.Component (with ComponentStatus)
Adapter->>UseCase: []entities.Component
UseCase->>Helper: GetComponentsVersion(ctx, components)
Helper->>SCANOSS: concurrent GetComponent calls (worker pool)
SCANOSS-->>Helper: version responses
Helper-->>UseCase: []entities.Component (versions resolved)
UseCase->>SCANOSS: query vulnerabilities / CPEs
SCANOSS-->>UseCase: results
UseCase-->>Client: response (items include ComponentStatus)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
agustingroh
force-pushed
the
chore/SP-4015-include-status-in-vulnerability-response
branch
from
e23d647 to
f9627cf
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
go.mod (1)
3-5:⚠️ Potential issue | 🟠 MajorUpdate
golangci-lint.ymlto use Go 1.24.x.The
golangci-lint.ymlworkflow is pinned togo-version: 1.22.x(line 22), which lags behind thego 1.24.0requirement ingo.mod. This inconsistency will cause linting to run against an older Go version than the build. Update togo-version: 1.24.xto matchgo-ci.yml,release.yml, and the Dockerfile.pkg/helpers/vulnerabilty_helper.go (1)
30-45:⚠️ Potential issue | 🔴 CriticalBug:
purlOutput.ComponentStatusis never populated before the check, making the preserve-existing-status branch dead code.
purlOutputis constructed at lines 30-35 without copyingComponentStatusfromvulnerabilityComponent, sopurlOutput.ComponentStatus.Messageis always"". The condition on line 37 is always false, and line 38 never executes. The "No vulnerabilities found" default on lines 40-43 will always be applied, even when a meaningful status (e.g.,InvalidPurl) was already set upstream.🐛 Proposed fix — copy ComponentStatus during construction
purlOutput := dtos.VulnerabilityComponentOutput{ Purl: vulnerabilityComponent.Purl, Requirement: vulnerabilityComponent.Requirement, Version: vulnerabilityComponent.Version, Vulnerabilities: vulnerabilityComponent.Vulnerabilities, + ComponentStatus: vulnerabilityComponent.ComponentStatus, } if len(purlOutput.Vulnerabilities) == 0 { - if purlOutput.ComponentStatus.Message != "" { - purlOutput.ComponentStatus = vulnerabilityComponent.ComponentStatus - } else { + if purlOutput.ComponentStatus.Message == "" { purlOutput.ComponentStatus = domain.ComponentStatus{ Message: "No vulnerabilities found for " + purlOutput.Purl, StatusCode: domain.ComponentWithoutInfo, } } }pkg/service/vulnerability_service_test.go (1)
706-716:⚠️ Potential issue | 🟠 MajorDB-closed test may not actually test the DB-closed path.
badReqhas empty purls ({"purls": []}), which likely fails validation before any DB access occurs. The error returned would be a validation error, not a DB-closed error. Other DB-closed tests in this file (e.g., line 591) were updated to usesuccessReqfor exactly this reason. This test should also usesuccessReqto ensure the DB-closed code path is exercised.🐛 Proposed fix
sWithClosedDB := NewVulnerabilityServer(dbClosed, myConfig) models.CloseDB(dbClosed) - _, err = sWithClosedDB.GetVulnerabilities(ctx, &badReq) //nolint:staticcheck // SA1019: keeping deprecated GetVulnerabilities method for backward compatibility + _, err = sWithClosedDB.GetVulnerabilities(ctx, &successReq) //nolint:staticcheck // SA1019: keeping deprecated GetVulnerabilities method for backward compatibility if err == nil { t.Errorf("DB is closed, an error was expected, s.GetVulnerabilities()") }pkg/usecase/OSV_use_case.go (1)
222-243:⚠️ Potential issue | 🟠 MajorFallback check
fallbackVulns != nilshould belen(fallbackVulns) > 0— status may remainSuccesswhen no vulnerabilities are found.
queryOSVreturns a non-nil empty slice on successful queries with no results (viamapOSVVulnerabilitiesusingmake). So when the fallback query succeeds but finds zero vulnerabilities,fallbackVulns != nilistrue,response.Vulnerabilitiesis set to the empty slice, and the status staysSuccess(from line 214). This is inconsistent with the non-fallback path at line 238 which correctly setsComponentWithoutInfofor empty results.Proposed fix
fallbackVulns := us.queryOSV(ctx, fallbackReq) - if fallbackVulns != nil { + if len(fallbackVulns) > 0 { response.Vulnerabilities = fallbackVulns } else { response.ComponentStatus = domain.ComponentStatus{pkg/usecase/vulnerability_use_case.go (1)
58-64:⚠️ Potential issue | 🟠 MajorRemove unused database connection —
connis obtained but never referenced.Line 59 obtains a connection from the pool with
us.db.Connx(ctx), but it is never used. The refactoredhelpers.GetComponentsVersion(line 65) takes the pool directly and manages its own connections. The explicit connection wastes a pool slot for the entire function duration, only to be closed without use in the defer statement.Proposed fix
- us.s.Debugf("Getting DB Connection from pool: %v", ctx) - conn, err := us.db.Connx(ctx) // Get a connection from the pool - if err != nil { - us.s.Errorf("Failed to get a database connection from the pool: %v", err) - return dtos.VulnerabilityOutput{}, errors.New("problem getting database pool connection") - } - defer models.CloseConn(conn)
🤖 Fix all issues with AI agents
In `@pkg/helpers/component_helper_test.go`:
- Around line 44-48: The test "Invalid Purl with semver" expects InvalidPurl but
SanitizeComponents is designed to parse semver operators into Requirement and
strip the version from the PURL (see SanitizeComponents), so update the test to
expect domain.Success instead of domain.InvalidPurl and add an assertion that
the resulting ComponentDTO.Requirement equals ">=4.17.21" (or that the
requirement was set) to validate the intended behavior.
In `@pkg/helpers/component_helper.go`:
- Around line 111-114: The warning log in the loop that calls GetComponent is
discarding the returned error; update the s.Warnf call in the same block (the
branch that currently logs "Failed to get component: %s, %s") to include the err
value (e.g., append the error to the formatted message) so the actual error from
GetComponent is recorded; keep the existing behavior of sending
processedComponent on results and continuing.
- Around line 126-149: The code can end up with zero workers when
config.Source.SCANOSS.MaxWorkers is set to 0; update validation or add a guard
so GetComponentsVersion always spawns at least one worker. Either add a check in
IsValidConfig() to return an error when SCANOSS is enabled and MaxWorkers <= 0
(reference config.Source.SCANOSS.MaxWorkers and IsValidConfig), or modify
GetComponentsVersion to compute numWorkers = max(1,
min(config.Source.SCANOSS.MaxWorkers, numJobs)) (reference GetComponentsVersion,
numWorkers, min and componentVersionWorker) so the worker pool is never zero and
components are processed.
- Around line 37-84: SanitizeComponents currently leaves an embedded version in
dto.Purl when purlParts == 2 but dto.Requirement is non-empty; always strip the
version from the PURL whenever len(purlParts) == 2 by setting dto.Purl =
purlParts[0]; if dto.Requirement is empty then set dto.Requirement =
purlParts[1], otherwise preserve the existing dto.Requirement (do not overwrite
it), and keep the existing logic that moves semver operators to Requirement when
utils.HasSemverOperator(purlParts[1]) returns true; update the branches around
purlParts, utils.HasSemverOperator, and the later conditional that assigns
Requirement so the PURL stored in the returned entities.Component never contains
an embedded version.
In `@pkg/service/vulnerability_service_test.go`:
- Line 374: The test "Get Component CPES succeeded with warnings" fails because
GetComponentsCpes currently always uses createSuccessStatusResponse(), which
returns StatusCode_SUCCESS with message "Success" and doesn't surface a custom
"Invalid components supplied" message; update GetComponentsCpes to detect
invalid/missing components during request processing and, when such invalid
components are found, construct and return a StatusResponse with
StatusCode_SUCCESS and Message "Invalid components supplied" (or set the message
on the response before returning) instead of calling
createSuccessStatusResponse() unconditionally so the test expectation matches
the service behavior.
In `@pkg/usecase/cpe.go`:
- Around line 76-84: The output for valid components incorrectly assigns
item.Version = c.Requirement instead of the resolved c.Version; update the code
in pkg/usecase/cpe.go where dtos.CpeComponentOutput is populated (for both
validComponents loop and the invalid-components case) to set Version to
c.Version (falling back to c.Requirement only if c.Version is empty), ensuring
consistency with the CPE lookup call d.cpePurl.GetCpeByPurl(c.Purl, c.Version)
and matching the behavior used in vulnerability_use_case.go.
🧹 Nitpick comments (6)
pkg/helpers/component_helper_test.go (1)
10-75: Consider asserting more fields (Purl, Requirement) beyond just StatusCode.Several test cases describe specific behaviors (e.g., "Component with empty requirement gets extracted from purl", semver operator extraction) but only assert on
StatusCode. Verifying thatPurlandRequirementare correctly populated after sanitization would strengthen these tests and catch regressions in the extraction logic.pkg/usecase/local_use_case_test.go (1)
62-72: Components missingStatusfield — may not match production flow.These
entities.Componentinstances don't haveStatusset (zero-valueComponentStatus), whereas in production they'd come throughSanitizeComponentswhich populatesStatus. IfGetVulnerabilitiesnow relies onStatusbeing pre-populated (e.g., to skip invalid components), this test may not reflect real behavior. Consider addingStatusfields consistent with whatSanitizeComponentswould produce, especially for the empty-purl component on line 67.pkg/entities/component.go (1)
21-26:Statusfield lacks ajsontag, unlike the other fields.If
Componentis ever serialized to JSON,Statuswill appear as"Status"(uppercase) while other fields use lowercase. Consider adding a json tag for consistency:- Status domain.ComponentStatus + Status domain.ComponentStatus `json:"status,omitempty"`pkg/dtos/vulnerability_output.go (1)
41-41:ComponentStatuslacks a JSON struct tag — consider addingjson:"-".Without an explicit tag, Go will marshal this field as
"ComponentStatus"in JSON output. IfVulnerabilityComponentOutputis ever serialized directly (e.g., in a REST response), this leaks internal status details. Since the status is mapped separately toErrorCode/ErrorMessagein the adapter layer viabuildComponentStatusMap, the field should be explicitly excluded from JSON.The same applies to
CpeComponentOutput.ComponentStatuson line 49.Proposed fix
type VulnerabilityComponentOutput struct { Purl string `json:"purl"` Version string `json:"version"` Requirement string `json:"requirement"` - ComponentStatus domain.ComponentStatus + ComponentStatus domain.ComponentStatus `json:"-"` Vulnerabilities []VulnerabilitiesOutput `json:"vulnerabilities"` } type CpeComponentOutput struct { Purl string `json:"purl"` Version string `json:"version"` Requirement string `json:"requirement"` Cpes []string `json:"cpes"` - ComponentStatus domain.ComponentStatus + ComponentStatus domain.ComponentStatus `json:"-"` }pkg/adapters/vulnerability_support_test.go (1)
357-366: Test case name "Mixed valid and invalid components" is now misleading.Since
FromComponentsRequestToComponentDTOno longer validates PURLs (validation moved toSanitizeComponents), all components are treated as valid at this layer. The test correctly expectsexpectError: false, but the name suggests invalid components are present and handled differently. Consider renaming to something like"Components with various purl formats".pkg/helpers/component_helper.go (1)
93-98: Unnecessary manual struct copy.Since
entities.Componentis a value type (no pointer fields that need deep-copying based on the entity definition), you can simplify:- processedComponent := entities.Component{ - Purl: j.Purl, - Requirement: j.Requirement, - Version: j.Version, - Status: j.Status, - } + processedComponent := jThis also avoids needing to update this code when fields are added to
Component.
agustingroh
force-pushed
the
chore/SP-4015-include-status-in-vulnerability-response
branch
from
f9627cf to
f07ad1b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/usecase/OSV_use_case.go (1)
222-243:⚠️ Potential issue | 🟠 MajorBug: fallback returning zero vulnerabilities leaves status as
Successinstead ofComponentWithoutInfo.
queryOSVreturns a non-nil empty slice (viamake([]..., 0, ...)inmapOSVVulnerabilities) when the API responds successfully but with no vulnerabilities. This means thefallbackVulns != nilbranch (line 230) is taken,response.Vulnerabilitiesis set to an empty slice, andComponentStatusstaysSuccess. Theelseblock (line 232–237) only triggers on actual HTTP/decode errors (whenqueryOSVreturnsnil), which arguably shouldn't be labeledComponentWithoutInfoeither — it's an error, not "no info."The same issue applies to the non-fallback path at lines 238–242: if the initial
queryOSVreturns a non-nil empty slice,len(response.Vulnerabilities) == 0is true butresponse.Vulnerabilitiesis not nil, so control reaches line 238 and correctly setsComponentWithoutInfo. However, in the fallback path, the empty-but-non-nil result silently hides the "no vulns found" state.🐛 Proposed fix — check length after fallback, not just nil
if len(response.Vulnerabilities) == 0 && j.FallbackPackage != nil { us.s.Debugf("No vulnerabilities found for GIT ecosystem, falling back to PURL query for: %s", j.OriginalPurl) fallbackReq := OSVRequest{ Version: j.Version, Package: *j.FallbackPackage, OriginalPurl: j.OriginalPurl, } fallbackVulns := us.queryOSV(ctx, fallbackReq) - if fallbackVulns != nil { + if len(fallbackVulns) > 0 { response.Vulnerabilities = fallbackVulns } else { response.ComponentStatus = domain.ComponentStatus{ Message: "No vulnerabilities found for: " + j.OriginalPurl, StatusCode: domain.ComponentWithoutInfo, } } } else if len(response.Vulnerabilities) == 0 {
🤖 Fix all issues with AI agents
In `@pkg/usecase/vulnerability_use_case.go`:
- Around line 113-121: The current code appends notValidComponents to
vulnerabilities.Components after merging valid results which breaks the original
request order; to fix, build the final vulnerabilities.Components in the
original input order instead of appending invalids at the end — for example,
create a map of outputs keyed by component identifier from the merged valid
results and notValidComponents (using the existing notValidComponents slice and
the merged outputs produced earlier), then iterate the original incoming
components slice and push the corresponding entry into
vulnerabilities.Components in that sequence; update the logic around
notValidComponents and vulnerabilities.Components to populate via this ordered
iteration so order is preserved.
🧹 Nitpick comments (3)
pkg/adapters/vulnerability_support.go (1)
145-159: Composite key collision risk with string concatenation.The key
Purl + Requirement + Versionuses raw concatenation without a separator, which could theoretically produce collisions (e.g.,purl="a", req="bc"vspurl="ab", req="c"). While practically unlikely given PURL formats, a delimiter would make this robust.Suggested fix
- statusByKey := buildComponentStatusMap(input, - func(c dtos.CpeComponentOutput) string { return c.Purl + c.Requirement + c.Version }, + statusByKey := buildComponentStatusMap(input, + func(c dtos.CpeComponentOutput) string { return c.Purl + "|" + c.Requirement + "|" + c.Version },Apply the same pattern to the key lookup and the vulnerability variant.
pkg/adapters/vulnerability_support_test.go (1)
532-623: Consider adding test cases with non-emptyComponentStatusto verify error code propagation.
TestGetComponentCPEInfoandTestGetComponentVulnerabilityInfoonly test with default (zero-value)ComponentStatus. The new status-to-error-code mapping logic (viabuildComponentStatusMapanddomain.StatusCodeToErrorCode) is untested here.pkg/usecase/vulnerability_use_case.go (1)
96-101: Shadowederrcaptured in goroutine — use a local variable instead.Line 97 assigns to the outer
err(from line 59) inside a goroutine closure. While there's no data race today (the OSV goroutine doesn't toucherr), this is fragile — any future change that readserrconcurrently would introduce a race. The dedicatedlocalErrvariable exists but isn't used for the initial assignment.♻️ Proposed fix
go func() { defer wg.Done() localVulUc := NewLocalVulnerabilitiesUseCase(ctx, us.s, us.config, us.db) - localVulnerabilities, err = localVulUc.GetVulnerabilities(ctx, validComponents) - if err != nil { - us.s.Errorf("Failed to get Vulnerabilities: %v", err) + var lErr error + localVulnerabilities, lErr = localVulUc.GetVulnerabilities(ctx, validComponents) + if lErr != nil { + us.s.Errorf("Failed to get Vulnerabilities: %v", lErr) localErr = errors.New("problems encountered extracting vulnerability data") return } }()
agustingroh
force-pushed
the
chore/SP-4015-include-status-in-vulnerability-response
branch
from
f07ad1b to
16237a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/service/vulnerability_service_test.go (2)
706-716:⚠️ Potential issue | 🟡 MinorClosed-DB test uses
badReqwhich fails at input validation, not at DB access.
badReqhas empty purls, soFromVulnerabilityRequestToComponentDTOreturns an error before any DB connection is attempted. This means the closed-DB scenario is never actually exercised. Other tests in this file (e.g.,TestVulnerabilityServer_GetComponentVulnerabilitiesat line 591) were correctly updated to usesuccessReqfor the closed-DB path.Proposed fix
sWithClosedDB := NewVulnerabilityServer(dbClosed, myConfig) models.CloseDB(dbClosed) - _, err = sWithClosedDB.GetVulnerabilities(ctx, &badReq) //nolint:staticcheck // SA1019: keeping deprecated GetVulnerabilities method for backward compatibility + _, err = sWithClosedDB.GetVulnerabilities(ctx, &successReq) //nolint:staticcheck // SA1019: keeping deprecated GetVulnerabilities method for backward compatibility if err == nil { t.Errorf("DB is closed, an error was expected, s.GetVulnerabilities()") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/service/vulnerability_service_test.go` around lines 706 - 716, The closed-DB test is using badReq which fails input validation via FromVulnerabilityRequestToComponentDTO before the DB is touched; change the request used when calling sWithClosedDB.GetVulnerabilities to successReq (the same pattern used in TestVulnerabilityServer_GetComponentVulnerabilities) so the code path reaches the DB after creating sWithClosedDB via NewVulnerabilityServer and closing it with models.CloseDB; keep the existing nolint comment on the GetVulnerabilities call.
367-376:⚠️ Potential issue | 🟡 MinorTest
want.Status.Messageis misleading — it's never asserted.Line 374 sets
Message: "Invalid components supplied", but the assertion at line 386 only comparesgot.Status.Status(theStatusCodeenum), notMessage. The currentGetComponentsCpesimplementation returns"Success"as the message viacreateSuccessStatusResponse(). The test passes, but thewantstruct is misleading to readers about expected behavior.Either update the assertion to also check
Message, or change the want toMessage: "Success"to match reality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/service/vulnerability_service_test.go` around lines 367 - 376, The test case "Get Component CPES succeeded with warnings" has a misleading expected message in the want struct; update the test to either assert the Status.Message returned by GetComponentsCpes or change the want.Status.Message to match the actual implementation (use "Success" as produced by createSuccessStatusResponse()). Locate the test case using the name "Get Component CPES succeeded with warnings" and the requestInvalidComponents arg, then either add an assertion comparing got.Status.Message to want.Status.Message or modify want to set Message: "Success" so the expected value aligns with createSuccessStatusResponse().pkg/usecase/OSV_use_case.go (1)
222-243:⚠️ Potential issue | 🟡 MinorInconsistent "no vulnerabilities" status between fallback and non-fallback paths.
When there's no fallback package and 0 vulnerabilities, the status is set to
ComponentWithoutInfo(line 239). But when the fallback query succeeds with 0 vulnerabilities (fallbackVulns != nilbut empty),response.Vulnerabilitiesis set to the empty slice and the status remainsSuccess(line 231 executes, else-if on line 238 is skipped).If the intent is to mark components with no known vulnerabilities as
ComponentWithoutInfo, the fallback-success-with-empty-results case should be handled consistently:Proposed fix
fallbackVulns := us.queryOSV(ctx, fallbackReq) if fallbackVulns != nil { response.Vulnerabilities = fallbackVulns + if len(fallbackVulns) == 0 { + response.ComponentStatus = domain.ComponentStatus{ + Message: "No vulnerabilities found for: " + j.OriginalPurl, + StatusCode: domain.ComponentWithoutInfo, + } + } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/usecase/OSV_use_case.go` around lines 222 - 243, The code treats an empty vulnerability slice differently when set via the fallback path; after calling queryOSV and assigning fallbackVulns to response.Vulnerabilities in the fallback branch (the OSVRequest/fallback block that calls queryOSV), add a check: if response.Vulnerabilities is non-nil but has length 0, set response.ComponentStatus to domain.ComponentStatus{Message: "No vulnerabilities found for: " + j.OriginalPurl, StatusCode: domain.ComponentWithoutInfo}; this makes the fallback-success-empty case consistent with the non-fallback branch that sets ComponentWithoutInfo when len(response.Vulnerabilities) == 0. Ensure you modify the code around the queryOSV/fallback handling (where fallbackReq, fallbackVulns and response.Vulnerabilities are referenced).
♻️ Duplicate comments (5)
pkg/helpers/component_helper.go (3)
111-114:errfromGetComponentis silently discarded in the warning log.Previously flagged and still unresolved. The warning on line 112 omits
err, losing the actual failure reason.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helpers/component_helper.go` around lines 111 - 114, The warning for a failed GetComponent call currently discards the actual error; update the log in the block handling GetComponent (the s.Warnf call near where GetComponent is invoked and processedComponent/results are used) to include the err value (e.g., append the error as a %v placeholder or similar) so the real failure reason is logged while still sending processedComponent to results and continuing.
126-149:MaxWorkers = 0silently drops all components.Previously flagged and still unresolved.
numWorkers = min(0, numJobs) = 0means no workers are spawned;wg.Wait()returns immediately;resultsis closed empty; the function returnsnileven thoughnumJobs > 0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helpers/component_helper.go` around lines 126 - 149, GetComponentsVersion currently uses numWorkers := min(config.Source.SCANOSS.MaxWorkers, numJobs) which allows numWorkers==0 and causes no component processing; change the logic to ensure at least one worker when there are jobs (e.g., set numWorkers = max(1, min(config.Source.SCANOSS.MaxWorkers, numJobs)) or treat a MaxWorkers of 0 as 1), so componentVersionWorker goroutines are spawned, jobs/results channels are consumed, and processedComponents is populated; update any related variable names (numWorkers, MaxWorkers) accordingly and keep the existing wg/join and channel closing behavior.
70-73: PURL is not stripped whenRequirementis already populated.This was flagged in a previous review and remains unresolved. When
dto.Requirement != ""andlen(purlParts) == 2(version embedded in PURL but no semver operator), line 71 is never reached, sodto.Purlstill contains the trailing@version. The resultingentities.Componentis inconsistently shaped compared to components whose version was extracted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helpers/component_helper.go` around lines 70 - 73, The dto.Purl still contains the trailing `@version` when len(purlParts) == 2 but dto.Requirement is already set; change the logic so you always strip the version from dto.Purl when purlParts has two elements, and only assign dto.Requirement from purlParts[1] if dto.Requirement is empty. In other words, in the block handling purlParts (the code referencing dto.Purl, dto.Requirement and purlParts), set dto.Purl = purlParts[0] unconditionally when len(purlParts) == 2, and then set dto.Requirement = purlParts[1] only if dto.Requirement == "" so entities.Component shapes are consistent.pkg/usecase/cpe.go (1)
76-84:⚠️ Potential issue | 🔴 Critical
item.Versionis still set toc.Requirementinstead ofc.Versionfor valid components.Line 78 assigns
c.Requirementtoitem.Version, discarding the resolved version fromGetComponentsVersion. The CPE lookup on line 84 correctly usesc.Version. The response will show the requirement string instead of the actual resolved version.Proposed fix
for _, c := range validComponents { var item dtos.CpeComponentOutput - item.Version = c.Requirement + item.Version = c.Version item.Requirement = c.Requirement item.Purl = c.Purl🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/usecase/cpe.go` around lines 76 - 84, In the loop over validComponents when building dtos.CpeComponentOutput, item.Version is incorrectly set from c.Requirement; change the assignment to use the resolved version (c.Version) instead so the output shows the actual resolved version returned by GetComponentsVersion—update the item.Version assignment in the loop that constructs dtos.CpeComponentOutput (the same block that later calls d.cpePurl.GetCpeByPurl with c.Version).pkg/usecase/vulnerability_use_case.go (1)
113-121: Invalid components appended at the end — response order differs from request order.This was flagged in a prior review.
notValidComponentsare appended after the merged valid results, so the output order won't match the input order. If callers depend on positional correspondence, this is a problem.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/usecase/vulnerability_use_case.go` around lines 113 - 121, The current code appends notValidComponents after valid results causing output order to differ; instead, preserve the request order by building a lookup of merged valid component outputs (e.g., map keyed by Purl) and then iterate the original input components slice in order, for each component push either the merged entry from that map into vulnerabilities.Components or create the dtos.VulnerabilityComponentOutput placeholder (with Purl, Requirement, Version, empty Vulnerabilities, ComponentStatus) for invalid ones; update the code that currently appends notValidComponents so it no longer appends them en masse but relies on the ordered iteration to populate vulnerabilities.Components.
🧹 Nitpick comments (6)
pkg/helpers/component_helper.go (1)
105-106: Stale comment does not describe the surrounding code.
// Set by default version = requirementimplies a default assignment, but no such assignment exists — the code queries the SCANOSS API and only updatesVersionif a non-empty result is returned. The comment should be removed or replaced.♻️ Proposed fix
- // Set by default version = requirement - var component types.ComponentResponse - component, err := sc.Component.GetComponent(ctx, types.ComponentRequest{ + component, err := sc.Component.GetComponent(ctx, types.ComponentRequest{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helpers/component_helper.go` around lines 105 - 106, The inline comment "// Set by default version = requirement" is stale and misleading; update or remove it near the declaration of the local variable component (types.ComponentResponse). Either delete the comment entirely or replace it with a brief accurate note such as "initialize component; Version will be populated from ScanOSS API response if present" so it correctly reflects that Version is only updated after a non-empty API result.pkg/dtos/vulnerability_output.go (1)
38-49:ComponentStatusis missing a JSON struct tag, inconsistent with all other fields.Every sibling field uses an explicit snake_case
json:"..."tag.ComponentStatushas no tag, so it serialises as"ComponentStatus"(PascalCase) if these DTOs are ever marshalled directly (logging, debugging, future serialisation paths). Either addjson:"-"to make the intentional exclusion explicit, or add a snake_case tag matching the wire name expected by consumers.♻️ Proposed fix
type VulnerabilityComponentOutput struct { Purl string `json:"purl"` Version string `json:"version"` Requirement string `json:"requirement"` - ComponentStatus domain.ComponentStatus + ComponentStatus domain.ComponentStatus `json:"-"` Vulnerabilities []VulnerabilitiesOutput `json:"vulnerabilities"` } type CpeComponentOutput struct { Purl string `json:"purl"` Version string `json:"version"` Requirement string `json:"requirement"` Cpes []string `json:"cpes"` - ComponentStatus domain.ComponentStatus + ComponentStatus domain.ComponentStatus `json:"-"` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/dtos/vulnerability_output.go` around lines 38 - 49, The ComponentStatus fields in the VulnerabilitiesOutput (struct containing Purl, Version, Requirement, ComponentStatus, Vulnerabilities) and CpeComponentOutput structs lack json tags and will serialize as "ComponentStatus"; update both declarations to include a snake_case json tag (e.g., json:"component_status") to match the other fields' naming convention so marshalled output stays consistent.pkg/adapters/vulnerability_support_test.go (1)
532-624: New status-propagation path has no test coverage.
TestGetComponentCPEInfoandTestGetComponentVulnerabilityInfoonly exercise the happy path (emptyComponentStatus). The new logic ingetComponentCPEInfo/getComponentVulnerabilityInfo— building the status map, translatingStatusCodetoErrorCode, and settingErrorMessage— is never exercised by the test suite.Consider adding a test case for each function that provides a
CpeComponentOutput/VulnerabilityComponentOutputwith a non-SuccessComponentStatusand asserts that the correspondingpb.ComponentCpesInfo/pb.ComponentVulnerabilityInfocarries the expectedErrorCodeandErrorMessage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/adapters/vulnerability_support_test.go` around lines 532 - 624, The tests only cover the success/empty status paths; add unit tests that exercise status propagation by passing CpeComponentOutput and VulnerabilityComponentOutput with a non-Success ComponentStatus (set StatusCode and Message) into getComponentCPEInfo and getComponentVulnerabilityInfo and assert the resulting pb.ComponentCpesInfo/pb.ComponentVulnerabilityInfo include the mapped ErrorCode (from StatusCode) and the ErrorMessage. Specifically, add one test case per TestGetComponentCPEInfo and TestGetComponentVulnerabilityInfo that supplies a ComponentStatus with a non-success StatusCode and Message, call getComponentCPEInfo/getComponentVulnerabilityInfo, then verify len(componentsInfo) matches and that componentsInfo[0].ErrorCode equals the expected mapping and componentsInfo[0].ErrorMessage equals the provided status Message. Ensure you reference the input types CpeComponentOutput, VulnerabilityComponentOutput and the output pb types when writing assertions.pkg/usecase/vulnerability_use_case.go (1)
94-104: Goroutine captures outererrvariable — potential for subtle bugs.The goroutine at line 94 writes to the outer
erron line 97, which shadows the intent. AlthoughlocalErris the variable checked afterwg.Wait(), any future code additions below line 105 that referenceerrwould silently pick up the goroutine's last write. Consider using a local variable inside the goroutine.Proposed fix
go func() { defer wg.Done() localVulUc := NewLocalVulnerabilitiesUseCase(ctx, us.s, us.config, us.db) - localVulnerabilities, err = localVulUc.GetVulnerabilities(ctx, validComponents) - if err != nil { - us.s.Errorf("Failed to get Vulnerabilities: %v", err) + var localGetErr error + localVulnerabilities, localGetErr = localVulUc.GetVulnerabilities(ctx, validComponents) + if localGetErr != nil { + us.s.Errorf("Failed to get Vulnerabilities: %v", localGetErr) localErr = errors.New("problems encountered extracting vulnerability data") return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/usecase/vulnerability_use_case.go` around lines 94 - 104, The goroutine is closing over the outer variable err which can lead to races or accidental later uses; inside the anonymous func used with wg create a new local error variable (e.g., errLocal) and assign the result of localVulUc.GetVulnerabilities to that local variable, then set localVulnerabilities and localErr explicitly from those local values before returning; reference NewLocalVulnerabilitiesUseCase, GetVulnerabilities, localVulnerabilities, localErr, err and wg to locate the code and avoid writing to the outer err from within the goroutine.pkg/usecase/cpe_test.go (1)
73-84: Consider asserting per-component status in the output.The test checks for an error but doesn't verify that the new
ComponentStatusfield is correctly populated on each output component (e.g., the empty-purl component should carryInvalidPurlstatus). This would improve confidence in the new status propagation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/usecase/cpe_test.go` around lines 73 - 84, When GetCpes returns an error, assert that each returned component in cpes has the correct ComponentStatus set (use the same identifiers from the test input to map outputs to inputs); specifically check that the component with an empty PURL is marked InvalidPurl and other components have the expected statuses. Update the test after the err branch to iterate components/cpes and assert status values (referencing cpeUc.GetCpes, components, ComponentStatus, and InvalidPurl) and fail the test if any status mismatches, ensuring the number of returned entries matches expectations for status checks.pkg/usecase/local_use_case.go (1)
69-72: Defensive empty-purl check sends a zero-value output, losing status context.With
SanitizeComponentsnow marking empty purls asInvalidPurland the caller (vulnerability_use_case.go) filtering them out before passing toGetVulnerabilities, this branch should be unreachable for normal flows. If it's kept as a safety net, consider propagatingc.Status(or at minimumc.Purl) so the output item isn't completely blank.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/usecase/local_use_case.go` around lines 69 - 72, The guard that sends a zero-value dtos.VulnerabilityComponentOutput when c.Purl is empty loses status context; update this safety branch in GetVulnerabilities (local_use_case.go) to propagate the component's metadata instead of an empty struct—e.g., include c.Status and/or c.Purl in the sent dtos.VulnerabilityComponentOutput so callers can see it was InvalidPurl (SanitizeComponents) or at least which Purl was skipped; this keeps downstream filtering and logging meaningful while retaining the defensive check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/adapters/vulnerability_support.go`:
- Around line 106-108: The function FromComponentsRequestToComponentDTO
currently accesses request.Components before verifying request is non-nil, which
can panic; add a nil guard at the top (like in
FromVulnerabilityRequestToComponentDTO) that checks if request == nil and
returns a clear error before any field access, ensuring subsequent code can
safely reference request.Components and maintaining consistent error messaging.
- Around line 131-158: The map key is built by concatenating Purl, Requirement
and Version without a delimiter, risking collisions; update the key construction
in buildComponentStatusMap usage inside getComponentCPEInfo and its lookup to
use a delimiter (e.g. "@" like the aggregator uses) — change the keyFunc lambda
passed to buildComponentStatusMap to return c.Purl + "@" + c.Requirement + "@" +
c.Version and update the lookup in the loop that computes key := info.Purl +
info.Requirement + info.Version to the same delimiter form (info.Purl + "@" +
info.Requirement + "@" + info.Version); ensure both creation and lookup use the
identical separator so statusByKey keys match correctly.
In `@pkg/helpers/vulnerabilty_helper.go`:
- Around line 36-45: The current conditional checks
purlOutput.ComponentStatus.Message which is always empty because purlOutput is
newly constructed, so it mistakenly overwrites real error statuses; change the
check to inspect the source vulnerabilityComponent.ComponentStatus.Message and,
if non-empty, copy vulnerabilityComponent.ComponentStatus into
purlOutput.ComponentStatus, otherwise set the generic "No vulnerabilities found
for "+purlOutput.Purl with StatusCode domain.ComponentWithoutInfo; update the
conditional around purlOutput assignment that references
purlOutput.ComponentStatus to instead reference
vulnerabilityComponent.ComponentStatus to preserve original error statuses like
InvalidPurl.
In `@pkg/usecase/cpe.go`:
- Around line 57-74: The current GetCpes in CpeUseCase splits
processedComponents into validComponents and out (invalid) and appends invalid
entries first, breaking input order; instead, iterate processedComponents once
in original order and append a CpeComponentOutput for each entry (populating
Cpes for valid ones and empty slice for invalid ones) so response preserves
request order; update the logic in GetCpes (and mirror the same single-pass
ordered append fix in the analogous function in vulnerability_use_case.go that
currently collects notValidComponents separately) so outputs are emitted in the
same sequence as processedComponents.
---
Outside diff comments:
In `@pkg/service/vulnerability_service_test.go`:
- Around line 706-716: The closed-DB test is using badReq which fails input
validation via FromVulnerabilityRequestToComponentDTO before the DB is touched;
change the request used when calling sWithClosedDB.GetVulnerabilities to
successReq (the same pattern used in
TestVulnerabilityServer_GetComponentVulnerabilities) so the code path reaches
the DB after creating sWithClosedDB via NewVulnerabilityServer and closing it
with models.CloseDB; keep the existing nolint comment on the GetVulnerabilities
call.
- Around line 367-376: The test case "Get Component CPES succeeded with
warnings" has a misleading expected message in the want struct; update the test
to either assert the Status.Message returned by GetComponentsCpes or change the
want.Status.Message to match the actual implementation (use "Success" as
produced by createSuccessStatusResponse()). Locate the test case using the name
"Get Component CPES succeeded with warnings" and the requestInvalidComponents
arg, then either add an assertion comparing got.Status.Message to
want.Status.Message or modify want to set Message: "Success" so the expected
value aligns with createSuccessStatusResponse().
In `@pkg/usecase/OSV_use_case.go`:
- Around line 222-243: The code treats an empty vulnerability slice differently
when set via the fallback path; after calling queryOSV and assigning
fallbackVulns to response.Vulnerabilities in the fallback branch (the
OSVRequest/fallback block that calls queryOSV), add a check: if
response.Vulnerabilities is non-nil but has length 0, set
response.ComponentStatus to domain.ComponentStatus{Message: "No vulnerabilities
found for: " + j.OriginalPurl, StatusCode: domain.ComponentWithoutInfo}; this
makes the fallback-success-empty case consistent with the non-fallback branch
that sets ComponentWithoutInfo when len(response.Vulnerabilities) == 0. Ensure
you modify the code around the queryOSV/fallback handling (where fallbackReq,
fallbackVulns and response.Vulnerabilities are referenced).
---
Duplicate comments:
In `@pkg/helpers/component_helper.go`:
- Around line 111-114: The warning for a failed GetComponent call currently
discards the actual error; update the log in the block handling GetComponent
(the s.Warnf call near where GetComponent is invoked and
processedComponent/results are used) to include the err value (e.g., append the
error as a %v placeholder or similar) so the real failure reason is logged while
still sending processedComponent to results and continuing.
- Around line 126-149: GetComponentsVersion currently uses numWorkers :=
min(config.Source.SCANOSS.MaxWorkers, numJobs) which allows numWorkers==0 and
causes no component processing; change the logic to ensure at least one worker
when there are jobs (e.g., set numWorkers = max(1,
min(config.Source.SCANOSS.MaxWorkers, numJobs)) or treat a MaxWorkers of 0 as
1), so componentVersionWorker goroutines are spawned, jobs/results channels are
consumed, and processedComponents is populated; update any related variable
names (numWorkers, MaxWorkers) accordingly and keep the existing wg/join and
channel closing behavior.
- Around line 70-73: The dto.Purl still contains the trailing `@version` when
len(purlParts) == 2 but dto.Requirement is already set; change the logic so you
always strip the version from dto.Purl when purlParts has two elements, and only
assign dto.Requirement from purlParts[1] if dto.Requirement is empty. In other
words, in the block handling purlParts (the code referencing dto.Purl,
dto.Requirement and purlParts), set dto.Purl = purlParts[0] unconditionally when
len(purlParts) == 2, and then set dto.Requirement = purlParts[1] only if
dto.Requirement == "" so entities.Component shapes are consistent.
In `@pkg/usecase/cpe.go`:
- Around line 76-84: In the loop over validComponents when building
dtos.CpeComponentOutput, item.Version is incorrectly set from c.Requirement;
change the assignment to use the resolved version (c.Version) instead so the
output shows the actual resolved version returned by GetComponentsVersion—update
the item.Version assignment in the loop that constructs dtos.CpeComponentOutput
(the same block that later calls d.cpePurl.GetCpeByPurl with c.Version).
In `@pkg/usecase/vulnerability_use_case.go`:
- Around line 113-121: The current code appends notValidComponents after valid
results causing output order to differ; instead, preserve the request order by
building a lookup of merged valid component outputs (e.g., map keyed by Purl)
and then iterate the original input components slice in order, for each
component push either the merged entry from that map into
vulnerabilities.Components or create the dtos.VulnerabilityComponentOutput
placeholder (with Purl, Requirement, Version, empty Vulnerabilities,
ComponentStatus) for invalid ones; update the code that currently appends
notValidComponents so it no longer appends them en masse but relies on the
ordered iteration to populate vulnerabilities.Components.
---
Nitpick comments:
In `@pkg/adapters/vulnerability_support_test.go`:
- Around line 532-624: The tests only cover the success/empty status paths; add
unit tests that exercise status propagation by passing CpeComponentOutput and
VulnerabilityComponentOutput with a non-Success ComponentStatus (set StatusCode
and Message) into getComponentCPEInfo and getComponentVulnerabilityInfo and
assert the resulting pb.ComponentCpesInfo/pb.ComponentVulnerabilityInfo include
the mapped ErrorCode (from StatusCode) and the ErrorMessage. Specifically, add
one test case per TestGetComponentCPEInfo and TestGetComponentVulnerabilityInfo
that supplies a ComponentStatus with a non-success StatusCode and Message, call
getComponentCPEInfo/getComponentVulnerabilityInfo, then verify
len(componentsInfo) matches and that componentsInfo[0].ErrorCode equals the
expected mapping and componentsInfo[0].ErrorMessage equals the provided status
Message. Ensure you reference the input types CpeComponentOutput,
VulnerabilityComponentOutput and the output pb types when writing assertions.
In `@pkg/dtos/vulnerability_output.go`:
- Around line 38-49: The ComponentStatus fields in the VulnerabilitiesOutput
(struct containing Purl, Version, Requirement, ComponentStatus, Vulnerabilities)
and CpeComponentOutput structs lack json tags and will serialize as
"ComponentStatus"; update both declarations to include a snake_case json tag
(e.g., json:"component_status") to match the other fields' naming convention so
marshalled output stays consistent.
In `@pkg/helpers/component_helper.go`:
- Around line 105-106: The inline comment "// Set by default version =
requirement" is stale and misleading; update or remove it near the declaration
of the local variable component (types.ComponentResponse). Either delete the
comment entirely or replace it with a brief accurate note such as "initialize
component; Version will be populated from ScanOSS API response if present" so it
correctly reflects that Version is only updated after a non-empty API result.
In `@pkg/usecase/cpe_test.go`:
- Around line 73-84: When GetCpes returns an error, assert that each returned
component in cpes has the correct ComponentStatus set (use the same identifiers
from the test input to map outputs to inputs); specifically check that the
component with an empty PURL is marked InvalidPurl and other components have the
expected statuses. Update the test after the err branch to iterate
components/cpes and assert status values (referencing cpeUc.GetCpes, components,
ComponentStatus, and InvalidPurl) and fail the test if any status mismatches,
ensuring the number of returned entries matches expectations for status checks.
In `@pkg/usecase/local_use_case.go`:
- Around line 69-72: The guard that sends a zero-value
dtos.VulnerabilityComponentOutput when c.Purl is empty loses status context;
update this safety branch in GetVulnerabilities (local_use_case.go) to propagate
the component's metadata instead of an empty struct—e.g., include c.Status
and/or c.Purl in the sent dtos.VulnerabilityComponentOutput so callers can see
it was InvalidPurl (SanitizeComponents) or at least which Purl was skipped; this
keeps downstream filtering and logging meaningful while retaining the defensive
check.
In `@pkg/usecase/vulnerability_use_case.go`:
- Around line 94-104: The goroutine is closing over the outer variable err which
can lead to races or accidental later uses; inside the anonymous func used with
wg create a new local error variable (e.g., errLocal) and assign the result of
localVulUc.GetVulnerabilities to that local variable, then set
localVulnerabilities and localErr explicitly from those local values before
returning; reference NewLocalVulnerabilitiesUseCase, GetVulnerabilities,
localVulnerabilities, localErr, err and wg to locate the code and avoid writing
to the outer err from within the goroutine.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
CHANGELOG.mdgo.modpkg/adapters/vulnerability_support.gopkg/adapters/vulnerability_support_test.gopkg/dtos/vulnerability_output.gopkg/entities/component.gopkg/helpers/component_helper.gopkg/helpers/component_helper_test.gopkg/helpers/vulnerabilty_helper.gopkg/service/vulnerability_service.gopkg/service/vulnerability_service_test.gopkg/usecase/OSV_use_case.gopkg/usecase/OSV_use_case_test.gopkg/usecase/cpe.gopkg/usecase/cpe_test.gopkg/usecase/local_use_case.gopkg/usecase/local_use_case_test.gopkg/usecase/vulnerability_use_case.gopkg/utils/semver.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/usecase/local_use_case_test.go
- pkg/entities/component.go
- pkg/usecase/OSV_use_case_test.go
- pkg/helpers/component_helper_test.go
agustingroh
force-pushed
the
chore/SP-4015-include-status-in-vulnerability-response
branch
from
16237a6 to
becc6c3
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 (3)
pkg/helpers/vulnerabilty_helper.go (1)
65-77:⚠️ Potential issue | 🟠 MajorPre-existing bug:
removeDuplicatedVulnerabilitiesis effectively a no-op.
for _, vulnerabilityComponent := range vulnerabilityComponentMapproduces a value copy. ReassigningvulnerabilityComponent.Vulnerabilitieson lines 72 and 74 modifies the local copy only; the map entry is never updated. All duplicate vulnerabilities pass through toconvertToVulnerabilityOutputunchanged.🐛 Proposed fix
func removeDuplicatedVulnerabilities(vulnerabilityComponentMap map[string]dtos.VulnerabilityComponentOutput) { - for _, vulnerabilityComponent := range vulnerabilityComponentMap { + for key, vulnerabilityComponent := range vulnerabilityComponentMap { vulnerabilityMap := make(map[string]dtos.VulnerabilitiesOutput) for _, vulnerability := range vulnerabilityComponent.Vulnerabilities { key := vulnerability.Source + vulnerability.Cve vulnerabilityMap[key] = vulnerability } vulnerabilityComponent.Vulnerabilities = make([]dtos.VulnerabilitiesOutput, 0, len(vulnerabilityMap)) for _, vulnerability := range vulnerabilityMap { vulnerabilityComponent.Vulnerabilities = append(vulnerabilityComponent.Vulnerabilities, vulnerability) } + vulnerabilityComponentMap[key] = vulnerabilityComponent } }Note: the inner loop variable
keyshadows the outerkeyparameter — rename one of them (e.g.,compKey) to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helpers/vulnerabilty_helper.go` around lines 65 - 77, The function removeDuplicatedVulnerabilities currently iterates with "for _, vulnerabilityComponent := range vulnerabilityComponentMap" which copies map values so assigning to vulnerabilityComponent.Vulnerabilities is a no-op; change the loop to iterate by key (e.g., "for compKey, vulnerabilityComponent := range vulnerabilityComponentMap") or retrieve the value, deduplicate into a new slice, then assign it back into vulnerabilityComponentMap[compKey].Also rename the inner dedupe map key variable (the concatenated key) to avoid shadowing the outer compKey (e.g., use "vulnKey"), and ensure the updated vulnerabilityComponent.Vulnerabilities replaces the map entry so convertToVulnerabilityOutput receives deduplicated data.pkg/service/vulnerability_service_test.go (1)
374-388:⚠️ Potential issue | 🟡 MinorTest declares an expected message that is never verified by the assertion.
The
wantat line 374 specifiesMessage: "Invalid components supplied", but the assertion at line 386 only comparesgot.Status.Status(the status code). The message contract is entirely unverified — any message (including"Success") passes this test.Either assert the full
Statusor remove the misleadingMessagefield fromwant:- if err == nil && !reflect.DeepEqual(got.Status.Status, tt.want.Status.Status) { + if err == nil && !reflect.DeepEqual(got.Status, tt.want.Status) { t.Errorf("service.GetComponentsCpes() = %v, want %v", got, tt.want) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/service/vulnerability_service_test.go` around lines 374 - 388, The test's expected response `want` includes a Status.Message but the assertion only compares `got.Status.Status` (status code), so the message is never verified; update the test in Test for service.GetComponentsCpes to either compare the full status object (e.g., assert reflect.DeepEqual(got.Status, tt.want.Status) or compare both Status.Status and Status.Message) or remove the misleading Message field from the `want` value (pb.ComponentsCpesResponse) so expectations match what the assertion actually checks.pkg/usecase/OSV_use_case.go (1)
228-243:⚠️ Potential issue | 🟠 MajorFallback nil-check misses the "no vulnerabilities" case when the HTTP request succeeds but returns zero results.
queryOSVreturns a non-nil empty slice on a successful HTTP call with no vulnerabilities (viamapOSVVulnerabilities'smake(…, 0, …)), and returnsnilonly on errors. The conditionif fallbackVulns != nil(line 230) is true even when the fallback found nothing, keepingComponentStatusasSuccessinstead ofComponentWithoutInfo.When a GIT PURL has zero vulnerabilities in both the primary and fallback queries, the fallback branch executes, receives a non-nil empty slice, and silently assigns it to
response.Vulnerabilitieswithout updating the status — leaving it asSuccess.Proposed fix — check length instead of nil
fallbackVulns := us.queryOSV(ctx, fallbackReq) - if fallbackVulns != nil { + if len(fallbackVulns) > 0 { response.Vulnerabilities = fallbackVulns } else { response.ComponentStatus = domain.ComponentStatus{ Message: "No vulnerabilities found for: " + j.OriginalPurl, StatusCode: domain.ComponentWithoutInfo, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/usecase/OSV_use_case.go` around lines 228 - 243, The fallback branch currently checks only for nil (fallbackVulns != nil) so an empty but non-nil slice from queryOSV is treated as a success; change the check to verify length (len(fallbackVulns) > 0) before assigning to response.Vulnerabilities and otherwise set response.ComponentStatus to domain.ComponentWithoutInfo with the message referencing j.OriginalPurl; update the same len-based check where response.Vulnerabilities is inspected so zero-length slices are handled consistently (see queryOSV, fallbackVulns, response.Vulnerabilities, mapOSVVulnerabilities, and ComponentStatus).
♻️ Duplicate comments (8)
pkg/helpers/component_helper.go (3)
111-114:⚠️ Potential issue | 🟡 Minor
errfromGetComponentis silently discarded in the warning log.The error is never included in the
Warnfcall, making the log useless for diagnosing API failures.🐛 Proposed fix
- s.Warnf("Failed to get component: %s, %s", j.Purl, j.Requirement) + s.Warnf("Failed to get component %s@%s: %v", j.Purl, j.Requirement, err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helpers/component_helper.go` around lines 111 - 114, The warning log in the GetComponent error path discards the actual error; update the error handling where GetComponent fails (the branch that calls s.Warnf("Failed to get component: %s, %s", j.Purl, j.Requirement) and then sends processedComponent to results) to include the err value in the log message (e.g., add an %v/%s placeholder and err) so the log shows the API error details while preserving the existing send to results and continue behavior.
126-149:⚠️ Potential issue | 🟡 Minor
MaxWorkers = 0silently returns an empty result; fan-out/fan-in also loses input order.Two issues:
Zero workers:
numWorkers = min(0, numJobs) = 0. No workers are spawned, the jobs channel is closed immediately, and the function returns an empty slice — silently discarding all components. This is reproducible by settingVULN_SCANOSS_WORKERS=0.Order not preserved: The fan-out/fan-in pattern sends results to a buffered channel in worker-completion order, not input order. Callers that depend on the output order matching the request order will observe non-deterministic component ordering.
For (1), either add a guard in
IsValidConfigor clampnumWorkersto at least 1:-numWorkers := min(config.Source.SCANOSS.MaxWorkers, numJobs) +numWorkers := max(1, min(config.Source.SCANOSS.MaxWorkers, numJobs))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helpers/component_helper.go` around lines 126 - 149, GetComponentsVersion currently sets numWorkers = min(config.Source.SCANOSS.MaxWorkers, numJobs) which allows zero workers and drops all input and also returns results in worker-completion order; fix by clamping numWorkers to at least 1 (e.g., numWorkers = max(1, min(...))) so a worker is always spawned, and preserve input order by sending an indexed job (include the component index with each entities.Component or a small job struct) and collecting results into a preallocated slice using that index before returning; update componentVersionWorker signature/usage accordingly to accept/send the index so ordering is stable.
70-73:⚠️ Potential issue | 🟡 MinorPURL embedded version not stripped when
Requirementis already populated.When
dto.Requirementis non-empty,len(purlParts) == 2, andpurlParts[1]is not a semver operator (lines 54–57 don't fire), the condition on line 70 is false. The resulting component carriesdto.Purlwith the version still embedded (e.g.,"pkg:npm/foo@1.0") whileRequirementholds a different value — an inconsistent state.🐛 Proposed fix
+ if len(purlParts) == 2 { + dto.Purl = purlParts[0] + } if dto.Requirement == "" && len(purlParts) == 2 { - dto.Purl = purlParts[0] dto.Requirement = purlParts[1] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helpers/component_helper.go` around lines 70 - 73, The code leaves a version embedded in dto.Purl when purlParts has two elements and the second part is a non-semver operator but dto.Requirement is already set; update the logic so that whenever len(purlParts) == 2 and the second part is not a semver operator you always set dto.Purl = purlParts[0] (strip the embedded version) while leaving dto.Requirement unchanged (do not skip the Purl assignment just because dto.Requirement is non-empty); locate the block that examines purlParts, the semver operator check, and dto.Requirement/dto.Purl assignment and change it to unconditionally strip the version into dto.Purl in that non-semver case.pkg/usecase/vulnerability_use_case.go (1)
113-121:⚠️ Potential issue | 🟡 MinorInvalid components appended at the end — original request order is lost.
notValidComponentsare appended after the aggregated valid results. Clients that expect response components in the same order as the request will observe a reordering. This is further compounded by the non-deterministic ordering already introduced byGetComponentsVersion's fan-out/fan-in pattern and the map iteration inconvertToVulnerabilityOutput.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/usecase/vulnerability_use_case.go` around lines 113 - 121, The current code appends notValidComponents after assembling valid results, which reorders responses and breaks client expectations; modify the assembly so the final vulnerabilities.Components preserves the original request order by mapping each input component identifier (or original slice index) to its output and then iterating the original request slice to append outputs in that order; update the logic around GetComponentsVersion and convertToVulnerabilityOutput to populate a map (keyed by component Purl or input index) for both valid and invalid results and then build vulnerabilities.Components by walking the original components sequence so ordering is deterministic and stable.pkg/adapters/vulnerability_support.go (2)
106-116:FromComponentsRequestToComponentDTOstill dereferencesrequestbefore checking for nil.Line 107 accesses
request.Componentswithout first guarding against a nilrequest, which will panic on a nil input. This was flagged in a previous review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/adapters/vulnerability_support.go` around lines 106 - 116, The function FromComponentsRequestToComponentDTO dereferences request.Components before checking for a nil request; add an explicit nil check at the start of FromComponentsRequestToComponentDTO (e.g., if request == nil) and return a descriptive error (such as "'components request' is nil" or similar) before accessing request.Components, then proceed with the existing request.Components nil check and convertViaJSON call.
145-158: Key concatenation without delimiter still risks silent status misassignment on collisions.Both
getComponentCPEInfo(lines 146, 151) andgetComponentVulnerabilityInfo(lines 196, 201) build map keys by raw concatenation ofPurl + Requirement + Versionwith no separator. This was flagged in a previous review.Also applies to: 195-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/adapters/vulnerability_support.go` around lines 145 - 158, The map key creation using raw concatenation (Purl + Requirement + Version) in getComponentCPEInfo and getComponentVulnerabilityInfo risks collisions; change the key to use an unambiguous delimiter or a composite key type instead. Locate the key construction sites (statusByKey and the local variable key in the loops inside getComponentCPEInfo and getComponentVulnerabilityInfo) and replace the concatenation with a stable format (e.g., fmt.Sprintf("%s|%s|%s", Purl, Requirement, Version) or a struct/map key) and update all corresponding lookups so keys match exactly. Ensure both places use the same key scheme so statusByKey lookups succeed without collisions.pkg/usecase/cpe.go (2)
57-74: Response order still does not match request order.Invalid components are collected first (lines 67–73), then valid components are appended (lines 76–103). This means the output order diverges from the input order. This was flagged in a previous review.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/usecase/cpe.go` around lines 57 - 74, GetCpes currently collects invalid components into out immediately but accumulates valid ones into validComponents and appends them later, which breaks input order; modify GetCpes so you preserve the original order by producing an output entry for each processedComponents element in the same loop (replace the validComponents accumulation with creating and appending a dtos.CpeComponentOutput for success cases as you do for failures), or alternatively allocate out with length len(processedComponents) and write each result into the same index while iterating processedComponents; reference symbols: function GetCpes, processedComponents, validComponents, out, and dtos.CpeComponentOutput.
67-73:item.Versionis still set toc.Requirementinstead ofc.Versionfor valid components.On line 78, valid components set
item.Version = c.Requirement, discarding the resolved version fromGetComponentsVersion. The CPE lookup on line 84 correctly usesc.Version. This was flagged in a previous review.The same applies to invalid components on line 69 (
Version: c.Requirement), though for thosec.Versionmay legitimately be empty.Also applies to: 76-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/usecase/cpe.go` around lines 67 - 73, The CpeComponentOutput struct is being populated with Version: c.Requirement instead of the resolved version; update the assignments that construct dtos.CpeComponentOutput so that for valid/resolved components you set Version = c.Version (the value produced by GetComponentsVersion) while leaving invalid/unresolved components using Version = c.Requirement as before; specifically fix the Version field in the dtos.CpeComponentOutput constructions where c.Requirement is currently used for resolved components and ensure Purl, Cpes, Requirement, and ComponentStatus remain unchanged.
🧹 Nitpick comments (3)
pkg/helpers/component_helper_test.go (1)
32-48: Tests only verifyStatusCode— core extraction behavior is unasserted.Cases "Component with empty requirement gets extracted from purl" and "Invalid Purl with semver" exercise non-trivial transformations (version extraction into
Requirement, PURL stripping) but never assert the resultingPurlandRequirementvalues. A bug in those code paths would go undetected.♻️ Suggested additions
for i, expectedCode := range tt.expectedStatus { if result[i].Status.StatusCode != expectedCode { t.Errorf("component[%d]: expected status %s, got %s", i, expectedCode, result[i].Status.StatusCode) } } +// Example: for the semver case, also assert: +// if result[0].Requirement != ">=4.17.21" { t.Errorf(...) } +// if result[0].Purl != "pkg:npm/lodash" { t.Errorf(...) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helpers/component_helper_test.go` around lines 32 - 48, The tests only assert StatusCode but not the transformed Purl/Requirement; update the two cases "Component with empty requirement gets extracted from purl" and "Invalid Purl with semver" to include expected Purl and Requirement values and assert them after calling the function under test (operate on the returned/updated dtos.ComponentDTO). Specifically, for the empty-requirement case expect Purl "pkg:npm/lodash" and Requirement "4.17.21", and for the semver case expect Purl "pkg:npm/lodash" and Requirement ">=4.17.21"; add fields like expectedPurl/expectedRequirement to the table-driven cases and assert equality against the ComponentDTO.Purl and ComponentDTO.Requirement (refer to ComponentDTO, Requirement, Purl and the test helper that invokes the extraction).pkg/usecase/vulnerability_use_case.go (1)
93-103: Goroutine reassigns the outererrvariable unnecessarily.The goroutine at line 97 uses
=to write back to theerrvariable declared at line 59 (outer scope). The error is immediately re-encoded intolocalErrfor propagation; the outererris never read afterwg.Wait(). This creates confusion and could silently introduce a race if future code is added that readserrafterwg.Add.♻️ Proposed fix
go func() { defer wg.Done() localVulUc := NewLocalVulnerabilitiesUseCase(ctx, us.s, us.config, us.db) - localVulnerabilities, err = localVulUc.GetVulnerabilities(ctx, validComponents) - if err != nil { - us.s.Errorf("Failed to get Vulnerabilities: %v", err) + var localErr2 error + localVulnerabilities, localErr2 = localVulUc.GetVulnerabilities(ctx, validComponents) + if localErr2 != nil { + us.s.Errorf("Failed to get Vulnerabilities: %v", localErr2) localErr = errors.New("problems encountered extracting vulnerability data") return } }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/usecase/vulnerability_use_case.go` around lines 93 - 103, The goroutine is reassigning the outer err variable (declared at line 59) which is unnecessary and risky; change the call inside the goroutine to capture its error into a new local variable (e.g., lErr or use := to create a shadowed err) when calling localVulUc.GetVulnerabilities, then only set localErr (or wrap/convert lErr) for propagation — do not reassign the outer err. Update the anonymous func so it uses a local error variable for the GetVulnerabilities result and assigns to localErr (or wraps lErr) on failure; keep references to NewLocalVulnerabilitiesUseCase, localVulUc, localVulnerabilities, and localErr to locate the change.pkg/usecase/cpe_test.go (1)
78-84: Uset.Logf/t.Logfinstead offmt.Printffor test output.
fmt.Printfalways writes to stdout regardless of test verbosity flags.t.Logfis the idiomatic alternative — output is only shown when the test fails or-vis passed.♻️ Proposed fix
- fmt.Printf("Test completed - GetCpes properly returned error for problematic data\n") + t.Logf("Test completed - GetCpes properly returned error for problematic data") return } - fmt.Printf("cpes response: %+v\n", cpes) + t.Logf("cpes response: %+v", cpes)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/usecase/cpe_test.go` around lines 78 - 84, Replace the fmt.Printf calls in the test (the snippet that prints "Test completed - GetCpes..." and "cpes response: ...") with t.Logf so test output respects verbosity and only shows on failure or with -v; update the two fmt.Printf invocations in pkg/usecase/cpe_test.go (inside the TestGetCpes test where variables t and cpes are in scope) to use t.Logf with the same format strings and arguments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 16: The gRPC-gateway JSON marshaler in the updated dependency now omits
unset protobuf fields (they no longer appear as null), which affects responses
produced by pkg/protocol/rest/server.go; either ensure REST clients tolerate
missing fields or restore prior behavior by configuring the gateway marshaler to
emit defaults (e.g., set the JSON marshaler option to include default/zero
values when creating the gateway/runtime marshaler used in functions that
register handlers such as the Register* or New* server setup in
pkg/protocol/rest/server.go) or pin go-grpc-helper to the previous version;
update the marshaler configuration or dependency accordingly and run API
integration tests to validate client compatibility.
In `@pkg/dtos/vulnerability_output.go`:
- Around line 37-50: The ComponentStatus field in both
VulnerabilityComponentOutput and CpeComponentOutput lacks a JSON tag causing it
to marshal as "ComponentStatus"; update the struct tags for the ComponentStatus
field in both types—either add json:"component_status" to include it in API
responses using snake_case to match other fields, or mark it json:"-" if it is
internal-only used by protobuf adapters—then run tests/serialization checks to
confirm behavior.
---
Outside diff comments:
In `@pkg/helpers/vulnerabilty_helper.go`:
- Around line 65-77: The function removeDuplicatedVulnerabilities currently
iterates with "for _, vulnerabilityComponent := range vulnerabilityComponentMap"
which copies map values so assigning to vulnerabilityComponent.Vulnerabilities
is a no-op; change the loop to iterate by key (e.g., "for compKey,
vulnerabilityComponent := range vulnerabilityComponentMap") or retrieve the
value, deduplicate into a new slice, then assign it back into
vulnerabilityComponentMap[compKey].Also rename the inner dedupe map key variable
(the concatenated key) to avoid shadowing the outer compKey (e.g., use
"vulnKey"), and ensure the updated vulnerabilityComponent.Vulnerabilities
replaces the map entry so convertToVulnerabilityOutput receives deduplicated
data.
In `@pkg/service/vulnerability_service_test.go`:
- Around line 374-388: The test's expected response `want` includes a
Status.Message but the assertion only compares `got.Status.Status` (status
code), so the message is never verified; update the test in Test for
service.GetComponentsCpes to either compare the full status object (e.g., assert
reflect.DeepEqual(got.Status, tt.want.Status) or compare both Status.Status and
Status.Message) or remove the misleading Message field from the `want` value
(pb.ComponentsCpesResponse) so expectations match what the assertion actually
checks.
In `@pkg/usecase/OSV_use_case.go`:
- Around line 228-243: The fallback branch currently checks only for nil
(fallbackVulns != nil) so an empty but non-nil slice from queryOSV is treated as
a success; change the check to verify length (len(fallbackVulns) > 0) before
assigning to response.Vulnerabilities and otherwise set response.ComponentStatus
to domain.ComponentWithoutInfo with the message referencing j.OriginalPurl;
update the same len-based check where response.Vulnerabilities is inspected so
zero-length slices are handled consistently (see queryOSV, fallbackVulns,
response.Vulnerabilities, mapOSVVulnerabilities, and ComponentStatus).
---
Duplicate comments:
In `@pkg/adapters/vulnerability_support.go`:
- Around line 106-116: The function FromComponentsRequestToComponentDTO
dereferences request.Components before checking for a nil request; add an
explicit nil check at the start of FromComponentsRequestToComponentDTO (e.g., if
request == nil) and return a descriptive error (such as "'components request' is
nil" or similar) before accessing request.Components, then proceed with the
existing request.Components nil check and convertViaJSON call.
- Around line 145-158: The map key creation using raw concatenation (Purl +
Requirement + Version) in getComponentCPEInfo and getComponentVulnerabilityInfo
risks collisions; change the key to use an unambiguous delimiter or a composite
key type instead. Locate the key construction sites (statusByKey and the local
variable key in the loops inside getComponentCPEInfo and
getComponentVulnerabilityInfo) and replace the concatenation with a stable
format (e.g., fmt.Sprintf("%s|%s|%s", Purl, Requirement, Version) or a
struct/map key) and update all corresponding lookups so keys match exactly.
Ensure both places use the same key scheme so statusByKey lookups succeed
without collisions.
In `@pkg/helpers/component_helper.go`:
- Around line 111-114: The warning log in the GetComponent error path discards
the actual error; update the error handling where GetComponent fails (the branch
that calls s.Warnf("Failed to get component: %s, %s", j.Purl, j.Requirement) and
then sends processedComponent to results) to include the err value in the log
message (e.g., add an %v/%s placeholder and err) so the log shows the API error
details while preserving the existing send to results and continue behavior.
- Around line 126-149: GetComponentsVersion currently sets numWorkers =
min(config.Source.SCANOSS.MaxWorkers, numJobs) which allows zero workers and
drops all input and also returns results in worker-completion order; fix by
clamping numWorkers to at least 1 (e.g., numWorkers = max(1, min(...))) so a
worker is always spawned, and preserve input order by sending an indexed job
(include the component index with each entities.Component or a small job struct)
and collecting results into a preallocated slice using that index before
returning; update componentVersionWorker signature/usage accordingly to
accept/send the index so ordering is stable.
- Around line 70-73: The code leaves a version embedded in dto.Purl when
purlParts has two elements and the second part is a non-semver operator but
dto.Requirement is already set; update the logic so that whenever len(purlParts)
== 2 and the second part is not a semver operator you always set dto.Purl =
purlParts[0] (strip the embedded version) while leaving dto.Requirement
unchanged (do not skip the Purl assignment just because dto.Requirement is
non-empty); locate the block that examines purlParts, the semver operator check,
and dto.Requirement/dto.Purl assignment and change it to unconditionally strip
the version into dto.Purl in that non-semver case.
In `@pkg/usecase/cpe.go`:
- Around line 57-74: GetCpes currently collects invalid components into out
immediately but accumulates valid ones into validComponents and appends them
later, which breaks input order; modify GetCpes so you preserve the original
order by producing an output entry for each processedComponents element in the
same loop (replace the validComponents accumulation with creating and appending
a dtos.CpeComponentOutput for success cases as you do for failures), or
alternatively allocate out with length len(processedComponents) and write each
result into the same index while iterating processedComponents; reference
symbols: function GetCpes, processedComponents, validComponents, out, and
dtos.CpeComponentOutput.
- Around line 67-73: The CpeComponentOutput struct is being populated with
Version: c.Requirement instead of the resolved version; update the assignments
that construct dtos.CpeComponentOutput so that for valid/resolved components you
set Version = c.Version (the value produced by GetComponentsVersion) while
leaving invalid/unresolved components using Version = c.Requirement as before;
specifically fix the Version field in the dtos.CpeComponentOutput constructions
where c.Requirement is currently used for resolved components and ensure Purl,
Cpes, Requirement, and ComponentStatus remain unchanged.
In `@pkg/usecase/vulnerability_use_case.go`:
- Around line 113-121: The current code appends notValidComponents after
assembling valid results, which reorders responses and breaks client
expectations; modify the assembly so the final vulnerabilities.Components
preserves the original request order by mapping each input component identifier
(or original slice index) to its output and then iterating the original request
slice to append outputs in that order; update the logic around
GetComponentsVersion and convertToVulnerabilityOutput to populate a map (keyed
by component Purl or input index) for both valid and invalid results and then
build vulnerabilities.Components by walking the original components sequence so
ordering is deterministic and stable.
---
Nitpick comments:
In `@pkg/helpers/component_helper_test.go`:
- Around line 32-48: The tests only assert StatusCode but not the transformed
Purl/Requirement; update the two cases "Component with empty requirement gets
extracted from purl" and "Invalid Purl with semver" to include expected Purl and
Requirement values and assert them after calling the function under test
(operate on the returned/updated dtos.ComponentDTO). Specifically, for the
empty-requirement case expect Purl "pkg:npm/lodash" and Requirement "4.17.21",
and for the semver case expect Purl "pkg:npm/lodash" and Requirement
">=4.17.21"; add fields like expectedPurl/expectedRequirement to the
table-driven cases and assert equality against the ComponentDTO.Purl and
ComponentDTO.Requirement (refer to ComponentDTO, Requirement, Purl and the test
helper that invokes the extraction).
In `@pkg/usecase/cpe_test.go`:
- Around line 78-84: Replace the fmt.Printf calls in the test (the snippet that
prints "Test completed - GetCpes..." and "cpes response: ...") with t.Logf so
test output respects verbosity and only shows on failure or with -v; update the
two fmt.Printf invocations in pkg/usecase/cpe_test.go (inside the TestGetCpes
test where variables t and cpes are in scope) to use t.Logf with the same format
strings and arguments.
In `@pkg/usecase/vulnerability_use_case.go`:
- Around line 93-103: The goroutine is reassigning the outer err variable
(declared at line 59) which is unnecessary and risky; change the call inside the
goroutine to capture its error into a new local variable (e.g., lErr or use :=
to create a shadowed err) when calling localVulUc.GetVulnerabilities, then only
set localErr (or wrap/convert lErr) for propagation — do not reassign the outer
err. Update the anonymous func so it uses a local error variable for the
GetVulnerabilities result and assigns to localErr (or wraps lErr) on failure;
keep references to NewLocalVulnerabilitiesUseCase, localVulUc,
localVulnerabilities, and localErr to locate the change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
CHANGELOG.mdgo.modpkg/adapters/vulnerability_support.gopkg/adapters/vulnerability_support_test.gopkg/dtos/vulnerability_output.gopkg/entities/component.gopkg/helpers/component_helper.gopkg/helpers/component_helper_test.gopkg/helpers/vulnerabilty_helper.gopkg/service/vulnerability_service.gopkg/service/vulnerability_service_test.gopkg/usecase/OSV_use_case.gopkg/usecase/OSV_use_case_test.gopkg/usecase/cpe.gopkg/usecase/cpe_test.gopkg/usecase/local_use_case.gopkg/usecase/local_use_case_test.gopkg/usecase/vulnerability_use_case.gopkg/utils/semver.go
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/entities/component.go
- pkg/usecase/OSV_use_case_test.go
- pkg/utils/semver.go
- pkg/usecase/local_use_case_test.go
agustingroh
deleted the
chore/SP-4015-include-status-in-vulnerability-response
branch
1 participant
… response
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores