feat(security): implement end-to-end security pillar#34
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements the security pillar end to end: adds alert and vulnerability contracts, persistence schema, dedicated mappers, scanning services, REST endpoints, exception handling, a scheduled worker, and related tests. ChangesSecurity Pillar: Code Scan Alerts and Vulnerability Priority
Sequence Diagram(s)sequenceDiagram
participant Client
participant VulnerabilityController
participant VulnerabilityService
participant VulnerabilityScanner
participant PriorityCalculator
participant VulnerabilityRepository
Client->>VulnerabilityController: POST /api/v1/vulnerabilities
VulnerabilityController->>VulnerabilityService: create(requestDto)
VulnerabilityService->>VulnerabilityScanner: processAndSave(entity)
VulnerabilityScanner->>PriorityCalculator: calculate(Vulnerability)
PriorityCalculator-->>VulnerabilityScanner: Priority
VulnerabilityScanner->>VulnerabilityRepository: save(entity with priority)
VulnerabilityScanner-->>VulnerabilityService: updated entity
VulnerabilityService-->>VulnerabilityController: VulnerabilityResponseDto
VulnerabilityController-->>Client: 201 Created
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 19
🧹 Nitpick comments (4)
backend/apps/api/src/test/java/dev/cleat/api/CodeScanAlertControllerTest.java (1)
33-43: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the response contract, not only the status.
This test currently passes even if
CodeScanAlertController.create(...)returns the wrong body, because the mock is unstubbed and only201is asserted. Given the PR objective is to return the expected contract shapes, stubcodeScanAlertService.create(...), verify delegation, and assert the JSON fields you expect back.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/apps/api/src/test/java/dev/cleat/api/CodeScanAlertControllerTest.java` around lines 33 - 43, The test for CodeScanAlertController.create is only checking the HTTP status, so it can pass even if the response body is wrong. In CodeScanAlertControllerTest, stub codeScanAlertService.create(...) with the expected DTO, verify the controller delegates to that service method, and add JSON assertions on the returned payload to validate the response contract instead of only asserting 201.backend/apps/api/src/test/java/dev/cleat/api/SecretFindingControllerTest.java (1)
33-43: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the returned DTO shape as well.
SecretFindingController.createSecret(...)returns a created body, but this test only checks the status code. StubsecretFindingService.create(...), verify it was called, and assert the response JSON so this actually guards the contract introduced in this PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/apps/api/src/test/java/dev/cleat/api/SecretFindingControllerTest.java` around lines 33 - 43, The SecretFindingControllerTest currently only verifies the 201 status and does not validate the returned DTO contract. Update whenCreateSecret_thenShouldReturn201() to stub secretFindingService.create(...), verify that create(...) is invoked, and assert the response JSON/body shape from SecretFindingController.createSecret(...) so the test covers the created response payload as well as the status.backend/apps/api/src/test/java/dev/cleat/api/VulnerabilityControllerTest.java (1)
33-43: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the vulnerability payload, not just
201 Created.This endpoint’s response now includes security-specific fields such as priority, but the test never checks them. Stub
vulnerabilityService.create(...), verify the call, and assert the returned JSON so the contract change is actually protected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/apps/api/src/test/java/dev/cleat/api/VulnerabilityControllerTest.java` around lines 33 - 43, The create-vulnerability test currently only checks for 201 Created and does not verify the response contract. Update whenCreateVulnerability_thenShouldReturn201 in VulnerabilityControllerTest to stub vulnerabilityService.create(...), verify that the controller invokes it, and assert the returned JSON payload includes the security-specific fields such as priority. Use the existing MockMvc setup and ObjectMapper to validate the response body instead of only the status.backend/libs/scanning/src/main/java/dev/cleat/scanning/CodeScanAlertScanner.java (1)
22-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the dead severity branch or implement the missing mapping.
Lines 22-27 set
Status.OPENin both paths, so severity currently has no effect. If all new alerts should start open, collapse this to one assignment; otherwise the low/medium rule is missing here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/libs/scanning/src/main/java/dev/cleat/scanning/CodeScanAlertScanner.java` around lines 22 - 27, The severity check in CodeScanAlertScanner is dead code because both branches set the same Status.OPEN value. Update the scan alert mapping logic in the method that processes CodeScanAlertEntity so either all severities are assigned OPEN with a single assignment, or add the missing low/medium severity-specific status mapping if that was intended. Use the Severity, Status, and CodeScanAlertEntity symbols to locate and simplify the conditional.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@backend/apps/api/src/main/java/dev/cleat/api/exception/GlobalExceptionHandler.java`:
- Around line 18-21: The RuntimeException handler in GlobalExceptionHandler is
exposing raw exception text in the 500 response; change handleRuntimeException
to return a generic client-facing message instead of ex.getMessage(), and add
server-side logging of the exception details for debugging. Keep the response
body consistent and update the ResponseEntity/Map.of usage so only a safe
generic error string is returned.
In
`@backend/apps/api/src/test/java/dev/cleat/api/CodeScanAlertControllerTest.java`:
- Line 34: Rename the test method in CodeScanAlertControllerTest to satisfy the
MethodName Checkstyle rule by removing underscores from
whenCreateCodeScanAlert_thenShouldBeReturn201; keep the same test behavior, but
use a camelCase method name so the class continues to compile and CI passes.
In
`@backend/apps/api/src/test/java/dev/cleat/api/SecretFindingControllerTest.java`:
- Line 34: The test method name in SecretFindingControllerTest does not match
the configured MethodName pattern, causing Checkstyle failure. Rename
whenCreateSecret_thenShouldReturn201 to a valid lowerCamelCase test name that
fits the project’s naming rules, and keep the behavior and assertions unchanged.
In
`@backend/apps/api/src/test/java/dev/cleat/api/VulnerabilityControllerTest.java`:
- Line 34: Rename the test method whenCreateVulnerability_thenShouldReturn201 to
match the configured camelCase test naming rule used by Checkstyle, keeping it
in VulnerabilityControllerTest and preserving the same test behavior. Update
only the method identifier so the name uses standard lowerCamelCase while still
clearly describing the create vulnerability success case.
In
`@backend/apps/worker/src/main/java/dev/cleat/worker/CleatWorkerApplication.java`:
- Line 7: `CleatWorkerApplication` is not scanning the worker package, so
`VulnerabilityWorker` is never discovered and its scheduled job won’t run.
Update the `@SpringBootApplication` component scan on `CleatWorkerApplication`
to include `dev.cleat.worker` (or broaden it to `dev.cleat`) alongside the
existing `dev.cleat.persistence` and `dev.cleat.scanning` packages so the worker
bean is registered.
In `@backend/apps/worker/src/main/java/dev/cleat/worker/VulnerabilityWorker.java`:
- Around line 10-15: The VulnerabilityWorker constructor is incorrectly
injecting VulnerabilityEntity, which is a JPA entity and not a Spring-managed
bean. Update VulnerabilityWorker so it only injects the service/dependency that
provides scan input, and create or fetch the VulnerabilityEntity inside
runSecurityScan() or via a proper repository/service instead of constructor
injection. Keep the constructor signature aligned with the actual Spring bean
dependencies and ensure the worker no longer depends on a directly injected
entity instance.
In
`@backend/libs/common/src/main/java/dev/cleat/common/dto/request/VulnerabilityRequestDto.java`:
- Line 3: Remove client-write access to priority from the vulnerability
create/update flow: VulnerabilityRequestDto should no longer expose or carry a
writable priority field, since it must be derived server-side. Update
VulnerabilityMapper.toVulnerabilityEntity(...) so it stops copying getPriority()
from the request DTO into VulnerabilityEntity, and compute/set priority in the
server-side mapping/service path based on CVSS/KEV/severity. Also remove any
unused Priority import and related DTO accessors/constructors tied to
request-supplied priority.
In `@backend/libs/domain/src/main/java/dev/cleat/domain/model/Vulnerability.java`:
- Line 5: The Vulnerability record currently allows nullable cvss and severity
values, but PriorityCalculator and VulnerabilityScanner assume both are present
for comparisons and switching. Update Vulnerability to enforce its invariants in
the canonical constructor, or adjust PriorityCalculator to handle missing
cvss/severity explicitly before doing numeric checks or the severity switch,
using the Vulnerability record, VulnerabilityScanner, and PriorityCalculator
symbols to locate the flow.
In `@backend/libs/domain/src/main/java/dev/cleat/domain/PriorityCalculator.java`:
- Around line 9-21: PriorityCalculator.calculate currently dereferences nullable
Vulnerability fields before the priority decision, causing failures instead of a
safe priority result. Update calculate(Vulnerability) to guard cvss() and
severity() up front: avoid auto-unboxing cvss() in the range check and handle
null severity before the switch, returning a sensible default priority when
either field is missing. Keep the existing URGENT/HIGH/MEDIUM/LOW mapping in
PriorityCalculator, but make the logic null-safe before evaluating the CVSS
threshold or switching on severity.
In
`@backend/libs/persistence/src/main/java/dev/cleat/persistence/entity/CodeScanAlertEntity.java`:
- Around line 56-57: The `detectedAt` field on `CodeScanAlertEntity` is
currently left unset before persistence, so `CodeScanAlertScanner` can save a
null value instead of relying on the timestamp default. Populate this field
either in `CodeScanAlertMapper` when building the entity or in a `@PrePersist`
hook on `CodeScanAlertEntity`, and ensure the `detectedAt` column is marked
non-null if the timestamp is required. Keep the fix centered on the entity
mapping/save flow so every alert has a timestamp before insert.
In
`@backend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/ActivityEventMapper.java`:
- Around line 15-24: ActivityEventMapper is not null-safe when mapping the repo
field, so activityEventEntity.getRepo().getName() can throw if repo is absent.
Update the mapping logic in the mapper’s ActivityEventResponseDto conversion to
handle a null repo gracefully, either by guarding the getRepo() access before
calling getName() or by applying a required-field change consistently across the
entity/schema and mapper.
In
`@backend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/SecretFindingMapper.java`:
- Around line 28-42: The SecretFindingMapper.toSecretFindingEntity create-flow
mapping is missing the account/repo data that SecretFindingService later needs
for the POST response. Update toSecretFindingEntity to also copy the account
identifier and repo association from SecretFindingRequestDto into
SecretFindingEntity so toSecretFindingDto can safely read getAccountId() and
getRepo().getName() without relying on scanner-side hydration.
In
`@backend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/UsageMapper.java`:
- Around line 20-30: The UsageMapper.toUsageResponseDto mapping dereferences
usageEntity.getSeries() directly, which can fail when
DashboardService.getDataset(...) supplies a fallback new UsageEntity() with a
null series. Update the mapper to safely handle a null series by defaulting to
an empty list before streaming, or initialize the series field in the
UsageEntity fallback path, so the UsageResponseDto mapping remains valid for
empty-usage accounts.
In
`@backend/libs/persistence/src/main/resources/db/migration/V3_alter_vulnerability_table.sql`:
- Line 1: The V3_alter_vulnerability_table migration only adds the priority
column and leaves existing vulnerability rows with NULL values. Update the
rollout so the vulnerability table change is paired with a backfill or explicit
post-migration rescan, using the migration script or the related vulnerability
persistence/rescan flow, so existing records get populated with priority instead
of remaining incomplete.
- Line 1: Rename the Flyway migration file to use the default versioned format
with a double underscore so it is picked up automatically. Update the migration
named V3_alter_vulnerability_table.sql to V3__alter_vulnerability_table.sql,
keeping the ALTER TABLE vulnerability statement unchanged.
In
`@backend/libs/persistence/src/main/resources/db/migration/V4__create_code_scan_alerts_table.sql`:
- Line 7: The schema for code scan alerts is hardcoding branch to "main" in the
migration, which can store the wrong value when ingest data omits branch. Remove
the default from the branch column in V4__create_code_scan_alerts_table.sql so
it stays nullable, and ensure the real branch is set earlier in the ingest flow
instead of being synthesized in the database schema.
In
`@backend/libs/scanning/src/main/java/dev/cleat/scanning/VulnerabilityScanner.java`:
- Around line 23-37: Move the null/input validation in
VulnerabilityScanner.processAndSave out of the try block so invalid inputs are
rejected before any persistence or priority calculation work starts. Keep the
null guard for VulnerabilityEntity at the top of the method, then run the
Vulnerability/priorityCalculator flow inside the try, and ensure the catch only
handles downstream failures. Also avoid calling vulnerabilityEntity.getId() in
the error path unless the entity is known non-null, so a null payload doesn’t
trigger a second exception and hide the original failure.
In
`@backend/libs/scanning/src/test/java/dev/cleat/scanning/VulnerabilityScannerTest.java`:
- Around line 39-53: The test in VulnerabilityScannerTest is asserting against a
stubbed repository lookup instead of the actual save behavior. Update the
verification around VulnerabilityScanner.processAndSave(...) to assert directly
on vulnerabilityEntity after the call and verify that
vulnerabilityRepository.save(vulnerabilityEntity) was invoked. Keep the existing
priority expectations tied to the saved entity so the test validates persistence
rather than a mocked findById(...) path.
- Around line 60-67: The current test in VulnerabilityScannerTest only verifies
exception wrapping because priorityCalculator.calculate(...) is mocked to throw,
so it does not actually cover invalid CVSS handling. Update the test to either
use the real PriorityCalculator path with an out-of-range CVSS value in
VulnerabilityScanner.processAndSave, or rename the test/method to reflect that
it is testing downstream exception propagation rather than CVSS validation.
---
Nitpick comments:
In
`@backend/apps/api/src/test/java/dev/cleat/api/CodeScanAlertControllerTest.java`:
- Around line 33-43: The test for CodeScanAlertController.create is only
checking the HTTP status, so it can pass even if the response body is wrong. In
CodeScanAlertControllerTest, stub codeScanAlertService.create(...) with the
expected DTO, verify the controller delegates to that service method, and add
JSON assertions on the returned payload to validate the response contract
instead of only asserting 201.
In
`@backend/apps/api/src/test/java/dev/cleat/api/SecretFindingControllerTest.java`:
- Around line 33-43: The SecretFindingControllerTest currently only verifies the
201 status and does not validate the returned DTO contract. Update
whenCreateSecret_thenShouldReturn201() to stub secretFindingService.create(...),
verify that create(...) is invoked, and assert the response JSON/body shape from
SecretFindingController.createSecret(...) so the test covers the created
response payload as well as the status.
In
`@backend/apps/api/src/test/java/dev/cleat/api/VulnerabilityControllerTest.java`:
- Around line 33-43: The create-vulnerability test currently only checks for 201
Created and does not verify the response contract. Update
whenCreateVulnerability_thenShouldReturn201 in VulnerabilityControllerTest to
stub vulnerabilityService.create(...), verify that the controller invokes it,
and assert the returned JSON payload includes the security-specific fields such
as priority. Use the existing MockMvc setup and ObjectMapper to validate the
response body instead of only the status.
In
`@backend/libs/scanning/src/main/java/dev/cleat/scanning/CodeScanAlertScanner.java`:
- Around line 22-27: The severity check in CodeScanAlertScanner is dead code
because both branches set the same Status.OPEN value. Update the scan alert
mapping logic in the method that processes CodeScanAlertEntity so either all
severities are assigned OPEN with a single assignment, or add the missing
low/medium severity-specific status mapping if that was intended. Use the
Severity, Status, and CodeScanAlertEntity symbols to locate and simplify the
conditional.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8972a510-5ec3-459d-a880-7f6bdf28e730
📒 Files selected for processing (50)
backend/apps/api/src/main/java/dev/cleat/api/CleatApiApplication.javabackend/apps/api/src/main/java/dev/cleat/api/controller/CodeScanAlertController.javabackend/apps/api/src/main/java/dev/cleat/api/controller/SecretFindingController.javabackend/apps/api/src/main/java/dev/cleat/api/controller/VulnerabilityController.javabackend/apps/api/src/main/java/dev/cleat/api/exception/GlobalExceptionHandler.javabackend/apps/api/src/test/java/dev/cleat/api/CodeScanAlertControllerTest.javabackend/apps/api/src/test/java/dev/cleat/api/SecretFindingControllerTest.javabackend/apps/api/src/test/java/dev/cleat/api/VulnerabilityControllerTest.javabackend/apps/worker/build.gradle.ktsbackend/apps/worker/src/main/java/dev/cleat/worker/CleatWorkerApplication.javabackend/apps/worker/src/main/java/dev/cleat/worker/VulnerabilityWorker.javabackend/libs/common/src/main/java/dev/cleat/common/dto/request/CodeScanAlertRequestDto.javabackend/libs/common/src/main/java/dev/cleat/common/dto/request/VulnerabilityRequestDto.javabackend/libs/common/src/main/java/dev/cleat/common/dto/response/CodeScanAlertResponseDto.javabackend/libs/common/src/main/java/dev/cleat/common/dto/response/DatasetDto.javabackend/libs/common/src/main/java/dev/cleat/common/dto/response/VulnerabilityResponseDto.javabackend/libs/common/src/main/java/dev/cleat/common/enums/Priority.javabackend/libs/common/src/main/java/dev/cleat/common/enums/Status.javabackend/libs/domain/build.gradle.ktsbackend/libs/domain/src/main/java/dev/cleat/domain/PriorityCalculator.javabackend/libs/domain/src/main/java/dev/cleat/domain/model/Vulnerability.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/DashboardService.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/entity/CodeScanAlertEntity.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/entity/VulnerabilityEntity.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/AccountMapper.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/ActivityEventMapper.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/CleatMapper.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/CodeScanAlertMapper.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/MemberMapper.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/RepoMapper.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/ScorecardCheckMapper.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/SecretFindingMapper.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/UsageMapper.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/UsagePointMapper.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/VulnerabilityMapper.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/repository/CodeScanAlertRepository.javabackend/libs/persistence/src/main/resources/db/migration/V2__update_repo_and_add_tables.sqlbackend/libs/persistence/src/main/resources/db/migration/V3_alter_vulnerability_table.sqlbackend/libs/persistence/src/main/resources/db/migration/V4__create_code_scan_alerts_table.sqlbackend/libs/persistence/src/main/resources/db/migration/V5__alter_secret_finding_table.sqlbackend/libs/scanning/build.gradle.ktsbackend/libs/scanning/src/main/java/dev/cleat/scanning/CodeScanAlertScanner.javabackend/libs/scanning/src/main/java/dev/cleat/scanning/SecretFindingScanner.javabackend/libs/scanning/src/main/java/dev/cleat/scanning/VulnerabilityScanner.javabackend/libs/scanning/src/main/java/dev/cleat/scanning/service/CodeScanAlertService.javabackend/libs/scanning/src/main/java/dev/cleat/scanning/service/SecretFindingService.javabackend/libs/scanning/src/main/java/dev/cleat/scanning/service/VulnerabilityService.javabackend/libs/scanning/src/test/java/dev/cleat/scanning/CodeScanAlertScannerTest.javabackend/libs/scanning/src/test/java/dev/cleat/scanning/SecretFindingScannerTest.javabackend/libs/scanning/src/test/java/dev/cleat/scanning/VulnerabilityScannerTest.java
💤 Files with no reviewable changes (1)
- backend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/CleatMapper.java
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/apps/worker/src/main/java/dev/cleat/worker/VulnerabilityWorker.java (1)
19-21: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftAvoid rescanning the entire table on every tick.
findAll()loads every vulnerability, andprocessAndSave(...)writes each entity back after recalculating priority. Once priorities are populated, this job turns into a full-table read/write every 10 minutes. Prefer paging plus a narrower query (for example, only rows with missing/stale priority) so the worker cost does not grow with table size.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/apps/worker/src/main/java/dev/cleat/worker/VulnerabilityWorker.java` around lines 19 - 21, The scheduled scan in VulnerabilityWorker.runSecurityScan is doing a full-table read with vulnerabilityRepository.findAll() and then rewriting every row via vulnerabilityScanner.processAndSave, so it will keep rescanning everything on each tick. Update this job to use a narrower repository query and paging/batching, targeting only vulnerabilities with missing or stale priority instead of all rows, and keep the work bounded as the table grows.backend/libs/scanning/src/main/java/dev/cleat/scanning/CodeScanAlertScanner.java (1)
22-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse or complete the severity branch.
Lines 22-27 ignore
severityand always setStatus.OPEN, so the conditional currently adds no behavior and can hide a missed mapping. Either remove the branch or implement the intended non-OPENpath.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/libs/scanning/src/main/java/dev/cleat/scanning/CodeScanAlertScanner.java` around lines 22 - 27, The severity check in CodeScanAlertScanner is redundant because both branches in the status assignment set Status.OPEN. Update the logic in the scanner method that handles CodeScanAlertEntity so the Severity.CRITICAL/Severity.HIGH branch has distinct behavior, or collapse the conditional entirely if all severities should map to OPEN. Use the existing Severity and Status handling in CodeScanAlertScanner to keep the mapping explicit and avoid a no-op branch.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/apps/worker/src/main/java/dev/cleat/worker/VulnerabilityWorker.java`:
- Around line 19-21: The batch scan in VulnerabilityWorker.runSecurityScan
currently stops on the first exception because
vulnerabilityScanner.processAndSave is called directly inside the forEach. Wrap
each per-row call in its own try/catch inside runSecurityScan so a failure for
one Vulnerability does not abort the scheduled run, and continue processing the
remaining entities while preserving the existing scheduler behavior.
---
Nitpick comments:
In `@backend/apps/worker/src/main/java/dev/cleat/worker/VulnerabilityWorker.java`:
- Around line 19-21: The scheduled scan in VulnerabilityWorker.runSecurityScan
is doing a full-table read with vulnerabilityRepository.findAll() and then
rewriting every row via vulnerabilityScanner.processAndSave, so it will keep
rescanning everything on each tick. Update this job to use a narrower repository
query and paging/batching, targeting only vulnerabilities with missing or stale
priority instead of all rows, and keep the work bounded as the table grows.
In
`@backend/libs/scanning/src/main/java/dev/cleat/scanning/CodeScanAlertScanner.java`:
- Around line 22-27: The severity check in CodeScanAlertScanner is redundant
because both branches in the status assignment set Status.OPEN. Update the logic
in the scanner method that handles CodeScanAlertEntity so the
Severity.CRITICAL/Severity.HIGH branch has distinct behavior, or collapse the
conditional entirely if all severities should map to OPEN. Use the existing
Severity and Status handling in CodeScanAlertScanner to keep the mapping
explicit and avoid a no-op branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a27bb90a-2681-473c-9e0c-18c0cdc9fe76
📒 Files selected for processing (5)
backend/apps/api/src/test/java/dev/cleat/api/CleatApiTests.javabackend/apps/worker/src/main/java/dev/cleat/worker/VulnerabilityWorker.javabackend/libs/scanning/src/main/java/dev/cleat/scanning/CodeScanAlertScanner.javabackend/libs/scanning/src/main/java/dev/cleat/scanning/SecretFindingScanner.javabackend/libs/scanning/src/main/java/dev/cleat/scanning/VulnerabilityScanner.java
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/libs/scanning/src/main/java/dev/cleat/scanning/VulnerabilityScanner.java
- backend/libs/scanning/src/main/java/dev/cleat/scanning/SecretFindingScanner.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@backend/apps/worker/src/test/java/dev/cleat/worker/CleatWorkerApplicationTests.java`:
- Around line 8-12: Disable background scheduling in CleatWorkerApplicationTests
so the smoke test does not trigger VulnerabilityWorker.runSecurityScan() or
other scheduled work; `@MockitoBean` on VulnerabilityScanner is not enough because
the scheduled path can still reach VulnerabilityRepository.findAll(). Update the
test setup around `@SpringBootTest/CleatWorkerApplicationTests` to either exclude
scheduling or load a narrower configuration so only the intended smoke-test
context starts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6088706b-686e-431c-8819-2282234b23ed
📒 Files selected for processing (1)
backend/apps/worker/src/test/java/dev/cleat/worker/CleatWorkerApplicationTests.java
…to fix context loading
…dundant exclude configurations
…-configuration for CI
…manage datasource
…tasource in test configuration
…at) to CleatWorkerApllicationTests(basePackages=dev.cleat)
…eatWorkerApplication in worker module
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@backend/apps/api/src/main/java/dev/cleat/api/exception/GlobalExceptionHandler.java`:
- Around line 22-26: The broad RuntimeException handler in
GlobalExceptionHandler is swallowing framework/client errors that should
preserve their 4xx responses. Narrow handleRuntimeException to only unexpected
server-side failures, and add explicit exception handlers for Spring MVC
exceptions you want to keep unchanged, such as ResponseStatusException and
request conversion/binding failures, so those paths don’t get converted into a
generic 500.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b2eeb002-a8eb-4545-b2e2-bb020ba6893f
📒 Files selected for processing (1)
backend/apps/api/src/main/java/dev/cleat/api/exception/GlobalExceptionHandler.java
…o prevent client manipulation
… enforce non-null cvss and severity
…lid,non-null arguments for Vulnerability constructor
…st entities to satisfy Vulnerability constructor constraints
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@backend/libs/scanning/src/test/java/dev/cleat/scanning/VulnerabilityScannerTest.java`:
- Around line 61-62: The test setup in VulnerabilityScannerTest has broken
entity construction because the VulnerabilityEntity initialization is malformed
and leaves an invalid statement in the same block. Fix the setup around
VulnerabilityEntity and the subsequent Severity assignment so the entity is
built with valid Java syntax and the severity is applied to the intended
variable, keeping the test compileable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b22fc870-c2ca-46b0-8d5f-e43307901d5c
📒 Files selected for processing (1)
backend/libs/scanning/src/test/java/dev/cleat/scanning/VulnerabilityScannerTest.java
…o resolve test problem
…d batch processing
Member
Author
|
Hi, @martian56 Could you please review the PR when you have a moment? #34 Thanks! |
martian56
approved these changes
Jun 30, 2026
martian56
left a comment
martian56
left a comment
Member
There was a problem hiding this comment.
Thanks Arzu, and yes, I went through it. This is a big PR and it holds together well. Approving.
First, on the CodeRabbit feedback, since you said you cleared all of it: I checked, all 22 threads are resolved, and I spot-checked the ones that actually mattered rather than trusting the green checkmarks. The fixes are real, not just closed:
- GlobalExceptionHandler no longer echoes the raw exception in the 500 body. It returns a generic message and logs the real one server side, and it rethrows ResponseStatusException so those still surface properly. That was the security one and it is handled correctly.
- PriorityCalculator validates the CVSS range and the non-null invariants now live in the Vulnerability record's canonical constructor, so the nullable-input concern is closed at construction time, which is the right place for it.
- The client-supplied priority is gone from VulnerabilityRequestDto, the migration no longer hardcodes 'main' as a column default, and the per-row failure isolation in the worker is in place.
A few things I checked on my own that look good:
- SecurityScanWorker wraps each record in its own try/catch, so one bad row logs and the batch keeps going. It also reads scalar fields and saves rather than walking lazy associations, so it sidesteps the detached-entity LazyInitializationException trap that came up on the workflow-audit PR. Nice.
- DatasetDto picked up codeAlerts, so the contract is growing pillar by pillar as each one lands. That is the right way to close the gap toward the frontend's full Dataset shape.
None of the following block the merge, but worth a look:
-
Migration numbering is going to collide with the other open backend PRs. This branch has V3__alter_vulnerability_table, while the workflow-audit PR has a V3__update_repo_and_add_tables, and the API-contract PR also touches V2__update_repo_and_add_tables. Flyway versions have to be unique and ordered, so whichever of these merges second will fail on a duplicate version or apply things out of order. Please sort the sequencing with Nurlan before the second one lands, this is the one I would not want to discover at deploy time.
-
SecurityScanWorker re-reads and reprocesses every finding in the tables every ten minutes. Fine for now, but it will not scale, so at some point this wants to be incremental (only new or changed rows) or at least paged.
-
Tiny one: GlobalExceptionHandler has 'occured' twice, should be 'occurred'. And the fixedDelay of 600000 could move into config like the other scan intervals, just for consistency.
Good work getting the whole security pillar in and grinding through that long tail of test-context wiring. Approving, just please lock down the migration ordering.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this change?
Related issue
Closes #8
Area
Screenshots
If this touches the UI, drop before and after screenshots here.
Checklist
bun run typecheck && bun run buildinapps/web(for frontend changes)Summary by CodeRabbit