lisa/feat-ai-diagnostics#423
Conversation
Signed-off-by: Gustavo Carvalho <gustavo.carvalho@container-solutions.com>
gusfcarvalho
left a comment
There was a problem hiding this comment.
Findings inline (both Low / non-blocking). Solid PR — the substantive correctness issues were already caught and fixed in the two prior self-review passes (rate-limit snooze budget on persisted count, pending-only pressure check, recent-failure window on cell.completed_at, live cell-level totals), each with a real regression test. Remaining notes: loadQueue's success path is untested because the integration DB has no river_job table, and a cosmetic meta:{} always-emitted nit. go build/vet/gofmt clean; check-diff green.
gusfcarvalho
left a comment
There was a problem hiding this comment.
Approving. The new commit (7a45620) addresses the Low finding from the prior round: TestAiDiagnosticsSummaryQueueSuccessPath seeds a real Postgres river_job table across every state plus an 'other' queue, and asserts the per-state counts, 24h completed/discarded sums, and oldestAvailableAt — exercising the previously-untested state::text GROUP BY, the SUM(CASE...) query, and the min(CASE WHEN state IN ? ...) placeholder binding. The meta:{} nit was a cosmetic acknowledgement. go build / vet / gofmt clean and all four CI checks (check-diff, unit-tests, integration-tests, lint) are green. Nice work.
|
PR approved. Marking as ready for e2e. |
automated implementation by lisa.