fix(brainbar): coverage counts terminal statuses (item F-1)#335
Conversation
|
@coderabbitai review |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
@macroscope-app review |
|
@codex review |
✅ Actions performedReview triggered.
|
|
You need to increase your spend limit or enable usage-based billing to run background agents. Go to Cursor |
|
Code review triggered. The Approvability Check and Correctness Check are now running. Results will be posted when complete. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
📝 WalkthroughWalkthroughDatabase statistics methods ChangesEnrichment Coverage Semantics
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
ApprovabilityVerdict: Would Approve This is a self-contained fix to coverage metric calculations, changing the definition of 'enriched' to include all terminal statuses rather than just 'success'. The change is limited in scope, well-documented with comments, and accompanied by comprehensive test coverage. The author owns all modified files. Macroscope would have approved this PR. Enable approvability here. |
9cfb1cc to
28c3e53
Compare
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Macroscope high-severity thread fixed in 28c3e53: #335 (comment) I added a red assertion proving the category invariant was broken: skippedTooShort was 2, and the category sum was 6 vs total 4. Then I narrowed skippedTooShort to only enrich_status IS NULL AND enriched_at IS NULL AND char_count < 50. Verification after the fix: targeted test passed, full swift test --package-path brain-bar passed with 474 tests and 0 failures, and the pre-push gate passed again. |
|
@macroscope-app review |
|
@coderabbitai review |
|
@codex review |
|
You need to increase your spend limit or enable usage-based billing to run background agents. Go to Cursor |
✅ Actions performedReview triggered.
|
|
Code review triggered. The Approvability Check and Correctness Check are now running. Results will be posted when complete. |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
/tmp/codex-mandate-item-F.md.enrich_status = 'success'toenrich_status IS NOT NULLso terminal statuses likeduplicate,noise, and future terminal outcomes count as covered.Diagnosis cited
duplicaterows were terminal but excluded from the numerator.337,573 / 383,333 = 88.06%; counting terminal statuses gives350,841 / 383,333 = 91.52%immediately.enrich_status IS NULL; non-NULL statuses are terminal coverage.TDD / Test plan
DashboardTests/testDashboardStatsCountsTerminalEnrichmentStatusesAsCoveredfailed at 1 enriched / 33.33%.DashboardTests/testEnrichmentStatsCountsAnyTerminalStatusAsCoveredfailed at 1 enriched / 25.0%.DatabaseTests/testRecallStatsCoverageCountsAnyTerminalStatusAsEnrichedfailed at 1 enriched / 25.0%.swift test --package-path brain-bar: 474 tests, 0 failures.git diff --check: passed.Review note
IS NOT NULLcould count a future persistedpendingstatus. I investigated and found pending enrichment is represented as NULL in this codebase;/tmp/codex-mandate-item-F.mdexplicitly requestsIS NOT NULLto include future terminal statuses.Note
Low Risk
Read-only SQL and metric semantics in BrainBar stats; no auth or write-path changes. Main risk is dashboard percentages shifting upward for duplicate/noise rows, which is intentional.
Overview
Enrichment coverage in
BrainDatabasenow treats any non-nullenrich_statusas covered (terminal outcomes likeduplicateandnoise), with NULL still meaning pending. Dashboard totals,enrichmentStats, andrecallStatsnumerators useenrich_status IS NOT NULLinstead ofenrich_status = 'success'.skippedTooShortinenrichmentStatsis narrowed to chunks that are still pending and below the char threshold, so terminal statuses are no longer folded into that bucket.Recent enrichment activity (last enriched time, sparkline buckets, rate, last-24h count) still filters
successonly, so live pipeline metrics stay tied to successful enrichments.Tests were updated/added in
DashboardTestsandDatabaseTeststo lock in terminal-status coverage and the split between coverage vs success-only activity.Reviewed by Cursor Bugbot for commit 28c3e53. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix coverage counts in BrainBar to include all terminal enrichment statuses
enrich_status IS NOT NULLas the predicate for coverage inBrainDatabase.enrichmentStatsandBrainDatabase.recallStats, replacing the previousenrich_status = 'success'check.duplicateornoiseare now counted as covered, alongsidesuccess.skippedTooShortnow only counts chunks where bothenrich_statusandenriched_atare NULL andchar_count < 50.enriched_chunks,enrichedChunkCount, and derived percentages will be higher for databases containing non-successterminal statuses.Macroscope summarized 28c3e53.
Summary by CodeRabbit
Bug Fixes