feat(security-agent): add audit report backend#4081
Open
jeanduplessis wants to merge 3 commits into
Open
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Contributor
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Executive SummaryIncremental changes address review feedback: Overview
Issue Details (click to expand)WARNING
Files Reviewed (8 changed, 73 total)
(plus 65 unchanged files from previous review) Fix these issues in Kilo Cloud Previous Review Summaries (2 snapshots, latest commit ab27592)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit ab27592)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryMigration creates indexes without Overview
Issue Details (click to expand)WARNING
Files Reviewed (73 files)
Fix these issues in Kilo Cloud Previous review (commit 2497209)Status: 3 Issues Found | Recommendation: Address before merge Executive SummaryMigration creates indexes without Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (71 files)
Reviewed by deepseek-v4-pro-20260423 · 529,301 tokens Review guidance: REVIEW.md from base branch |
St0rmz1
reviewed
Jun 17, 2026
St0rmz1
reviewed
Jun 17, 2026
St0rmz1
reviewed
Jun 17, 2026
St0rmz1
approved these changes
Jun 17, 2026
pandemicsyn
requested changes
Jun 18, 2026
pandemicsyn
left a comment
pandemicsyn
left a comment
Contributor
There was a problem hiding this comment.
Found a few audit report SLA correctness issues that should be fixed before merge.
| return { status: 'unknown', deadline, reason: 'invalid_terminal_timestamp' }; | ||
| } | ||
| return { | ||
| status: fixedAtMs <= deadlineMs ? 'terminal_met' : 'terminal_missed', |
Contributor
There was a problem hiding this comment.
Must-fix: This treats fixed_at === sla_due_at as terminal_met. The spec defines SLA report evidence as before vs at/after the recorded deadline, and SLA breach semantics make equality breached/missed. Use fixedAtMs < deadlineMs for met, otherwise terminal_missed, and add an equality regression test.
| } | ||
|
|
||
| return { | ||
| status: cutoffMs <= deadlineMs ? 'open_within_deadline' : 'open_past_deadline', |
Contributor
There was a problem hiding this comment.
Must-fix: This reports dataThrough === sla_due_at as open_within_deadline. At the exact deadline an open finding is already at/after the deadline, so the report should show it as past deadline. Use cutoffMs < deadlineMs for within-deadline and cover the equality case in tests.
| firstDetectedAt: stringFromSnapshot(evidenceSnapshot, 'first_detected_at'), | ||
| canonicalFindingId: stringFromSnapshot(latestSnapshot, 'canonical_finding_id'), | ||
| deleted, | ||
| sla: buildSlaEvidence(evidenceSnapshot, reportParams.dataThrough), |
Contributor
There was a problem hiding this comment.
Must-fix: Deleted findings still derive SLA evidence from the latest snapshot. If the final deletion evidence still has status: "open", the report can show deleted: true and sla.status: "open_past_deadline", which is contradictory and not trustworthy evidence. Pass deletion evidence into SLA derivation and return unknown unless a terminal timestamp is recorded.
| (left, right) => | ||
| left.effective_at.localeCompare(right.effective_at) || left.id.localeCompare(right.id) | ||
| ); | ||
| const latestSnapshot = latestFindingSnapshot(rows); |
Contributor
There was a problem hiding this comment.
Must-fix: SLA state is derived only from in-period snapshots, but open/terminal status is evaluated against dataThrough. A report can include an in-period open snapshot, miss a later out-of-period fixed event, and incorrectly report the finding as open/past deadline. Load latest state through dataThrough for included findings, or return unknown when the period rows do not prove cutoff state.
pandemicsyn
approved these changes
Jun 18, 2026
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
3 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.
Summary
Security Agent now has the backend/API foundation for durable Security Finding Activity Events and owner-scoped Audit Reports, split out so it can land before the web UI.
Why this change is needed
Security owners need period-bounded evidence of material Security Finding actions and outcomes for investigation and compliance work without implying complete legacy reconstruction or aggregate historical SLA compliance. The schema, writers, and API need to land first because deployed services will write audit evidence before the UI consumes it.
How this is addressed
Verification
Visual Changes
N/A
Reviewer Notes
Human Reviewer Flags
Code Reviewer Agent
Code Reviewer Notes