Repository navigation
Stream findings live, and notify the triggering user via browser push - #150
Conversation
…unning GetDiffReviewStatus now resolves the diff/hunk structure even before a review completes, and the webui polls batch events to merge in streamed comments live, instead of hiding Findings behind the Events tab until the whole review finishes. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Notify the person who triggered a review, even with the tab or browser closed, when the first finding is ready and again when the review completes or fails - via Web Push + a service worker, opt-in from a toast shown right after triggering a review. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Catches up schema.sql with every migration already applied to the dev database but never dumped (push notifications, plus the pre-existing user time_zone migration), after fixing the local pg_dump/server version mismatch by running pg_dump inside the matching-version Postgres container. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
dbmate dump refuses to run when the host's pg_dump is older than the Postgres server (ours runs Postgres 18 in Docker, hosts often still have an older postgresql-client). scripts/dbmate-dump.sh shims pg_dump to run inside the livereview_pg container instead, so the dump always matches the server without requiring a matching client install on every machine. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
…ia Docker dbmate-dump.sh previously forwarded pg_dump into the livereview_pg container to sidestep a host/server version mismatch. That made schema regeneration depend on Docker being up. Replace it with a version check that fails fast with install instructions (scripts/install-pgdg-client.sh, installing the matching major from the official PGDG apt repo) when the host's pg_dump is older than the server, so the fix is a one-time local client upgrade rather than a per-dump workaround. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
shrsv
changed the title
Add Push Notifications and Stream Review FindingsOverviewThis change introduces browser push notifications for review events. It also enables real-time streaming of findings into the UI. New database tables, API endpoints, and a service worker support these features. Technical Highlights
Impact
|
| CREATE TABLE IF NOT EXISTS push_subscriptions ( | ||
| id BIGSERIAL PRIMARY KEY, | ||
| user_id BIGINT NOT NULL REFERENCES users(id) ON DELETE CASCADE, | ||
| endpoint TEXT NOT NULL UNIQUE, |
There was a problem hiding this comment.
Severity: warning
Ensure endpoint, p256dh, and auth_key are handled securely. While standard for web push, review overall data-at-rest encryption policies.
Suggestions:
- Confirm if these fields are encrypted at rest or if the push service provides sufficient security guarantees.
- Ensure these values are not logged excessively.
| default_org_id bigint, | ||
| review_level integer DEFAULT 1 NOT NULL | ||
| review_level integer DEFAULT 1 NOT NULL, | ||
| time_zone text |
There was a problem hiding this comment.
Severity: info
The time_zone column was added to the users table. This is a good addition for internationalization and personalized time display.
Suggestions:
- Consider how existing users will be migrated or assigned a default time zone.
- Ensure application logic correctly uses this time zone for display and operations.
| return JSONErrorWithEnvelope(c, http.StatusNotFound, "review not found") | ||
| } | ||
|
|
||
| if reviewRecord.Status != "completed" { |
There was a problem hiding this comment.
Severity: critical
Removing this block changes the API contract for GetDiffReviewStatus. Previously, it returned a 200 OK with status and metadata for non-completed reviews.
Suggestions:
- Reintroduce the status check and early return for non-completed reviews if previous behavior is desired.
- Update API documentation to reflect this breaking change if the new behavior is intentional.
- Consider returning an appropriate HTTP error (e.g.,
404 Not Foundor400 Bad Request) for non-completed reviews if the endpoint is now strictly for completed reviews.
| return JSONErrorWithEnvelope(c, http.StatusNotFound, "review not found") | ||
| } | ||
|
|
||
| if reviewRecord.Status != "completed" { |
There was a problem hiding this comment.
Severity: critical
Without the early return, the code will now attempt to load diffFileRecords and findingRecords for non-completed reviews, which might be empty or incomplete.
Suggestions:
- Add a check for
reviewRecord.Status == "completed"before attempting to load full review data. - Ensure subsequent logic handles incomplete data gracefully if this new behavior is intended.
| return JSONErrorWithEnvelope(c, http.StatusNotFound, "review not found") | ||
| } | ||
|
|
||
| if reviewRecord.Status != "completed" { |
There was a problem hiding this comment.
Severity: warning
The logic for applying envelope usage, operation ID, and idempotency key from metadata for non-completed reviews is lost.
Suggestions:
- Re-evaluate if billing/accounting metadata should still be applied for non-completed review statuses.
- Restore the metadata application logic if it is still required for auditing or billing purposes.
| if _, baseURL, parseErr := validateAndParseURL(reviewRecord.PrMrURL); parseErr == nil { | ||
| if token, tokenErr := s.findIntegrationToken(baseURL, orgID); tokenErr == nil { | ||
| connectorID = &token.ID | ||
| if updErr := rm.UpdateReviewConnector(reviewRecord.ID, token.ID); updErr != nil { |
There was a problem hiding this comment.
Severity: info
The UpdateReviewConnector call is a side effect within a function primarily designed for reading/resolving diffs. Consider if this backfill logic could be triggered elsewhere, or if resolvePreloadedChanges should be renamed to reflect its mutation aspect.
Suggestions:
- Extract the
UpdateReviewConnectorlogic into a separate, explicit backfill or migration function that runs once per review, perhaps triggered by the initial review creation or a background job. - Rename
resolvePreloadedChangesto indicate it performs data mutations, e.g.,ResolveAndBackfillReviewChanges.
| } | ||
|
|
||
| userAgent := c.Request().UserAgent() | ||
| _, err = h.db.ExecContext(c.Request().Context(), ` |
There was a problem hiding this comment.
Severity: warning
The ON CONFLICT (endpoint) DO UPDATE SET user_id = EXCLUDED.user_id clause is a critical security and correctness feature. It ensures that if an endpoint is re-registered, it's correctly associated with the currently authenticated user, preventing a stale subscription from being tied to the wrong user.
| return JSONErrorWithEnvelope(c, http.StatusBadRequest, "endpoint query param is required") | ||
| } | ||
|
|
||
| if _, err := h.db.ExecContext(c.Request().Context(), ` |
There was a problem hiding this comment.
Severity: warning
Deleting by endpoint AND user_id is crucial for security, preventing one user from deleting another's push subscription.
| // a failed push must never fail the review itself. | ||
| func (n *Notifier) NotifyReviewEvent(ctx context.Context, reviewID, orgID int64, kind string) { | ||
| go func() { | ||
| dispatchCtx, cancel := context.WithTimeout(context.Background(), 20*time.Second) |
There was a problem hiding this comment.
Severity: warning
Consider adding the reviewID and kind to the log message for better traceability if the context times out.
Suggestions:
- Modify the log message to include
reviewIDandkindfrom the outer scope.
| } | ||
|
|
||
| type reviewInfo struct { | ||
| userEmail string |
There was a problem hiding this comment.
Severity: warning
The orgID parameter is passed to NotifyReviewEvent but never used in dispatch or any other function within this package. Remove it.
Suggestions:
- Remove the
orgIDparameter fromNotifyReviewEventsignature and its calls.
| return "Your review" | ||
| } | ||
|
|
||
| func buildPayload(kind Kind, reviewID int64, info *reviewInfo, prodURL string) pushPayload { |
There was a problem hiding this comment.
Severity: info
The prodURL parameter is retrieved from the database in dispatch then passed here. Consider passing n *Notifier directly or caching prodURL if it's static.
Suggestions:
- Pass the
Notifierinstance tobuildPayloadinstead ofprodURLto allow internal access togetProductionURL. - Alternatively, consider caching
prodURLin theNotifierstruct if it's expected to be static.
|
|
||
| func (n *Notifier) getProductionURL(ctx context.Context) string { | ||
| var url sql.NullString | ||
| if err := n.db.QueryRowContext(ctx, `SELECT livereview_prod_url FROM instance_details LIMIT 1`).Scan(&url); err != nil { |
There was a problem hiding this comment.
Severity: warning
This function queries livereview_prod_url on every dispatch. If this URL is static, cache it in the Notifier struct to avoid repeated DB lookups.
Suggestions:
- Cache the
livereview_prod_urlin theNotifierstruct after the first successful retrieval.
| return nil | ||
| } | ||
|
|
||
| vapidPublic, vapidPrivate, err := loadOrCreateVAPIDKeys(ctx, n.db) |
There was a problem hiding this comment.
Severity: critical
The loadOrCreateVAPIDKeys function is not defined in this file. Ensure it's part of the overall change and properly handles secure storage of the private key.
Suggestions:
- Ensure
loadOrCreateVAPIDKeysis implemented in a separate, secure utility file and that the VAPID private key is stored encrypted or with appropriate access controls.
| } | ||
|
|
||
| options := &webpush.Options{ | ||
| Subscriber: "mailto:support@hexmos.com", |
There was a problem hiding this comment.
Severity: warning
The Subscriber email mailto:REDACTED_EMAIL is hardcoded. Consider making this configurable or deriving it from system settings.
Suggestions:
- Move the subscriber email to a configuration setting or retrieve it from
instance_detailstable.
| log.Printf("[WARN] push send failed for subscription %d: %v", sub.id, sendErr) | ||
| continue | ||
| } | ||
| resp.Body.Close() |
There was a problem hiding this comment.
Severity: critical
The resp.Body.Close() call is inside the if sendErr != nil block. It should be deferred right after webpush.SendNotificationWithContext to ensure it's always closed.
Suggestions:
- Move
defer resp.Body.Close()immediately after thewebpush.SendNotificationWithContextcall to ensure the response body is always closed, preventing potential resource leaks.
| t.Cleanup(func() { _, _ = db.Exec(`DELETE FROM system_settings WHERE name = 'push_vapid'`) }) | ||
| _, _ = db.Exec(`DELETE FROM system_settings WHERE name = 'push_vapid'`) | ||
|
|
||
| first, err := GetVAPIDPublicKey(context.Background(), db) |
There was a problem hiding this comment.
Severity: critical
The GetVAPIDPublicKey function is not defined in the provided pushnotify.go file. This test implies its existence and functionality.
Suggestions:
- Ensure
GetVAPIDPublicKeyis defined and available, likely in a related VAPID utility file. If it's part ofloadOrCreateVAPIDKeys, consider exposing it properly or testingloadOrCreateVAPIDKeysdirectly.
|
|
||
| // vapidKeys is stored as the 'push_vapid' row in system_settings, the same | ||
| // name/data JSONB pattern internal/api/system_settings.go already uses for | ||
| // SMTP credentials — no new secret-storage mechanism invented here. |
There was a problem hiding this comment.
Severity: info
Clarify that system_settings is for system-wide settings. VAPID keys are global, not user-specific.
Suggestions:
- Add 'system-wide' to the comment for clarity.
|
|
||
| func readVAPIDKeys(ctx context.Context, db *sql.DB) (*vapidKeys, error) { | ||
| var data []byte | ||
| err := db.QueryRowContext(ctx, `SELECT data FROM system_settings WHERE name = 'push_vapid'`).Scan(&data) |
There was a problem hiding this comment.
Severity: warning
Consider adding a FOR UPDATE clause to the SELECT query if strong consistency is needed to prevent another transaction from inserting keys between the SELECT and a potential INSERT. However, given the ON CONFLICT DO NOTHING and re-read pattern, this might be overkill and could lead to deadlocks if not carefully managed.
Suggestions:
- Evaluate if
FOR UPDATEis necessary for stronger consistency, or if the currentON CONFLICTand re-read is sufficient given thepush_vapidkey is immutable once set.
| } | ||
| var keys vapidKeys | ||
| if err := json.Unmarshal(data, &keys); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Severity: warning
Add context to the error message for JSON unmarshalling failures.
Suggestions:
- Change to
fmt.Errorf("unmarshal VAPID keys: %w", err).
| } | ||
| data, err := json.Marshal(vapidKeys{PublicKey: newPublic, PrivateKey: newPrivate}) | ||
| if err != nil { | ||
| return "", "", err |
There was a problem hiding this comment.
Severity: warning
Add context to the error message for JSON marshalling failures.
Suggestions:
- Change to
fmt.Errorf("marshal VAPID keys: %w", err).
| // concurrently, so which one is actually "first" is decided by | ||
| // pushnotify.Notifier's atomic claim, not by this call site. See | ||
| // reviewprocessor.SetNotifier's doc comment. | ||
| if status == "completed" && hasComments(comments) { |
There was a problem hiding this comment.
Severity: warning
If comments is always []*models.ReviewComment, consider passing a len(comments) > 0 boolean directly to CreateBatchEvent instead of using reflection. This improves type safety and performance.
Suggestions:
- Refactor
CreateBatchEventto accept ahasCommentsboolean directly, avoiding reflection. - Alternatively, if the concrete type is always
[]*models.ReviewComment, pass the slice directly and perform a type assertion insidehasComments.
| if comments == nil { | ||
| return false | ||
| } | ||
| v := reflect.ValueOf(comments) |
There was a problem hiding this comment.
Severity: warning
Using reflect.ValueOf(comments) on an interface{} can be slower and less type-safe than direct type checks. Ensure comments is consistently a slice type.
Suggestions:
- Add a comment explaining the expected types for
commentsto prevent misuse. - Consider if a more specific interface or generic type could avoid reflection if
commentsis always a known slice type.
| // NotifyReviewEvent should be non-blocking (fire-and-forget) — it is | ||
| // called from hot paths (batch-completion event persistence, review | ||
| // status updates) that must never wait on a push send. kind is one of | ||
| // "first_finding", "completed", "failed". |
There was a problem hiding this comment.
Severity: info
Consider adding ctx to NotifyReviewEvent signature to allow for context-aware logging or tracing within the notifier implementation.
Suggestions:
- Update
ReviewNotifierinterface andNotifyReviewEventto acceptcontext.Context.
| return c.JSON(http.StatusInternalServerError, map[string]string{"error": "Failed to save review AI settings: " + err.Error()}) | ||
| } | ||
|
|
||
| perfSettings, err := reviewperf.Load(c.Request().Context(), s.db) |
There was a problem hiding this comment.
Severity: warning
Perf settings are loaded after Upsert. If Load fails, the handler returns 500 after the preference was already persisted, so the client sees failure for a successful mutation. Load perf settings before Upsert, or make the write conditional on a successful load.
Suggestions:
- Move reviewperf.Load before store.Upsert.
- If a Load error occurs after Upsert, still return the saved state or explicitly indicate the save succeeded.
| return JSONErrorWithEnvelope(c, http.StatusNotFound, "review not found") | ||
| } | ||
|
|
||
| if reviewRecord.Status != "completed" { |
There was a problem hiding this comment.
Severity: warning
Removing this branch sends non-completed reviews through the completed-review path. Ensure the remaining code branches on reviewRecord.Status and preserves failure_reason/message; otherwise pending/failed reviews are returned as completed-shaped responses. Keep an explicit status branch or add a regression test for queued/running/failed states.
Suggestions:
- Add an explicit status switch in GetDiffReviewStatus for non-completed and completed reviews.
- Add handler tests for pending, running, and failed reviews to verify response shape.
| switch { | ||
| case err == nil: | ||
| t.limiter.ReportSuccess() | ||
| case errclass.Classify(t.providerType, err) == errclass.RateLimited: |
There was a problem hiding this comment.
Severity: warning
Rate-limit classification depends on providerType, but SetLimiter does not enforce SetProviderType. An empty value silently skews limiter self-tuning.
Suggestions:
- Validate in Execute that providerType is non-empty whenever limiter is non-nil, or default to a safe classification.
- Document SetProviderType as required before SetLimiter is used.
| // iteration near-instantly once ctx was done, racking up well beyond | ||
| // what a properly-bailing loop would attempt. A generous upper bound | ||
| // catches the regression without being timing-flaky. | ||
| if got := started.Load(); got > 6 { |
There was a problem hiding this comment.
Severity: warning
The cancel goroutine is independent of the retry loop. If the scheduler delays it past 10ms, even fixed code can exceed 6 attempts and fail on a correct implementation.
Suggestions:
- Trigger cancellation deterministically, e.g. from a watcher that cancels once started reaches 3.
- Or make retry delay and attempts injectable with a fake clock so no real time is needed.
| // Bounded by the single slowest task (300ms) plus scheduling slop, not | ||
| // by any serialization - this is the ~2x+ measured improvement over the | ||
| // old model's >= 450ms floor. | ||
| if elapsed > 400*time.Millisecond { |
There was a problem hiding this comment.
Severity: warning
The 400ms wall-clock bound is tight for a shared CI runner. A scheduling pause can push elapsed time over the limit without a code regression.
Suggestions:
- Assert concurrency deterministically, e.g. record max simultaneous Execute calls and require it to reach 7.
- Or use a larger margin combined with a minimum-concurrency check.
livereviewbot
left a comment
livereviewbot
left a comment
There was a problem hiding this comment.
❌ LiveReview policy: Not approved (blocked at Risk score) · Review #13539
| Stage | Result | Detail |
|---|---|---|
| Eligibility | ✅ | author level 1 >= required 1 |
| Static rules | ✅ | no blocking rulesets configured |
| Risk score | ❌ | Risk score 1.00 (P100) is above the cutoff of 1.00 (P20) |
| LLM classification | ✅ | class "logic-change", confidence 9 |
| Final decision | ❌ | risk: Risk score 1.00 (P100) is above the cutoff of 1.00 (P20) |
…tale push-worker icon Review-detail page: - Elapsed timer no longer resets on page refresh (seed from review.startedAt instead of mount time). - Add a live "Elapsed" header stat alongside "Last activity". - Capture and show commit messages (new review_commits.message column, GitHub/GitLab providers now keep the message instead of discarding it). - Events tab label shows a live event-count badge. - Group duplicate circuit-breaker pills into one expandable pill per cause. - Raw Events tab gets a "Tail Log" auto-scroll toggle. - Findings tab shows total comment count above the summary panel. - Fix "Batches: 0" getting stuck - the review summary now refreshes on the existing 15s poll instead of only fetching once on mount. Reviews/PR tables: - Add opt-in TanStack column resizing to ClientTable, used by the Reviews and Merge Requests tables, without changing existing visuals/layout. Push notifications: - Service worker now calls skipWaiting()/clients.claim() on install/activate so existing subscribers pick up sw.js changes (like the notification icon fix) without needing to fully close every open tab first. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
|
Error generating summary: request timeout: API call exceeded deadline This review completed with partial results: 5 out of 7 batches processed successfully. Failed batches: [batch-3 batch-6] The comments and analysis shown are based only on the files that were successfully reviewed. Some files may have been skipped due to batch processing errors. |
livereviewbot
left a comment
livereviewbot
left a comment
There was a problem hiding this comment.
❌ LiveReview policy: Not approved (blocked at Risk score) · Review #13542
| Stage | Result | Detail |
|---|---|---|
| Eligibility | ✅ | author level 1 >= required 1 |
| Static rules | ✅ | no blocking rulesets configured |
| Risk score | ❌ | Risk score 1.00 (P100) is above the cutoff of 1.00 (P20) |
| LLM classification | ❌ | no diff classification available |
| Final decision | ❌ | risk: Risk score 1.00 (P100) is above the cutoff of 1.00 (P20); llm: no diff classification available |
|
Error generating summary: request timeout: API call exceeded deadline |
| // Collect batch results - TOLERATE PARTIAL FAILURES. One bad batch (a | ||
| // transient LLM error, a parse failure, ...) must not discard every other | ||
| // batch's already-good comments and fail the whole review; only abort if | ||
| // every batch failed. Mirrors internal/ai/langchain/provider.go's |
There was a problem hiding this comment.
Severity: warning
This behavior mirrors the langchain provider instead of reusing shared code; the two implementations can drift.
Suggestions:
- Extract partial-failure batch aggregation into a shared helper used by both gemini and langchain providers.
| return nil, fmt.Errorf("batch %s not found in results", batchID) | ||
| fmt.Printf("[PARTIAL FAILURE] Batch %s not found in results, skipping\n", batchID) | ||
| } else { | ||
| fmt.Printf("[PARTIAL FAILURE] Batch %s failed: %v, skipping\n", batchID, taskResult.Error) |
There was a problem hiding this comment.
Severity: warning
Per-batch error text is printed to stdout then dropped; the final warning/error only knows batch IDs. Callers cannot see why a batch failed.
Suggestions:
- Store per-batch failures as a slice of {ID, Err} and include the error text in the partial-failure warning and all-failed error.
- If a batch failed due to context cancellation, wrap/return ctx.Err so callers can distinguish cancellation from other failures.
|
|
||
| // If NO batches succeeded, fail the entire review - there's nothing to aggregate. | ||
| if len(batchResults) == 0 { | ||
| return nil, fmt.Errorf("all %d batches failed: %v", len(batchInput.Batches), failedBatches) |
There was a problem hiding this comment.
Severity: warning
If batchInput.Batches is empty, this returns "all 0 batches failed" when there was nothing to process. Empty input should not be reported as a failure.
Suggestions:
- Handle len(batchInput.Batches) == 0 before this check, e.g., return an empty result or a clear no-op result.
| switch { | ||
| case err == nil: | ||
| t.limiter.ReportSuccess() | ||
| case errclass.Classify(t.providerType, err) == errclass.RateLimited: |
There was a problem hiding this comment.
Severity: warning
An empty providerType silently reaches this classification when SetLimiter is used without SetProviderType. That can mislabel rate limits and skew limiter adaptation.
Suggestions:
- Validate in Execute that limiter != nil implies providerType is set, or log a warning once.
- Make providerType a required parameter of NewBatchTask or SetLimiter instead of a separate optional setter.
| t.logger.Info("Processing batch task %s with %d files", t.id, len(t.batch)) | ||
| } | ||
|
|
||
| if t.limiter != nil { |
There was a problem hiding this comment.
Severity: warning
No test covers the new limiter path. Add a fake limiter covering Acquire failure, deferred release, and all three report branches.
Suggestions:
- Add a fake limiter capturing Acquire/Release/Report calls.
- Assert release happens on processor success and error, and that the correct report branch is chosen.
| // Bounded by the single slowest task (300ms) plus scheduling slop, not | ||
| // by any serialization - this is the ~2x+ measured improvement over the | ||
| // old model's >= 450ms floor. | ||
| if elapsed > 400*time.Millisecond { |
There was a problem hiding this comment.
Severity: warning
A 400ms wall-clock ceiling for 300ms of work is timing-flaky on loaded CI. Prefer deterministic concurrency assertions.
Suggestions:
- Assert that all tasks started before any slow task finishes, using sync primitives rather than elapsed time.
- If wall-clock timing is kept, increase the margin substantially or use a controllable fake clock.
| // iteration near-instantly once ctx was done, racking up well beyond | ||
| // what a properly-bailing loop would attempt. A generous upper bound | ||
| // catches the regression without being timing-flaky. | ||
| if got := started.Load(); got > 6 { |
There was a problem hiding this comment.
Severity: warning
This upper-bound check passes vacuously if cancellation happens before ProcessAll starts the task. Require evidence the task actually ran.
Suggestions:
- Assert started.Load() >= 1 to ensure the task began before cancellation.
- Use a channel to signal the first Execute, then cancel only after receiving it.
livereviewbot
left a comment
livereviewbot
left a comment
There was a problem hiding this comment.
❌ LiveReview policy: Not approved (blocked at Risk score) · Review #13543
| Stage | Result | Detail |
|---|---|---|
| Eligibility | ✅ | author level 1 >= required 1 |
| Static rules | ✅ | no blocking rulesets configured |
| Risk score | ❌ | Risk score 1.00 (P100) is above the cutoff of 1.00 (P20) |
| LLM classification | ✅ | class "logic-change", confidence 9 |
| Final decision | ❌ | risk: Risk score 1.00 (P100) is above the cutoff of 1.00 (P20) |
…r, and risk engine bugs - Table resize/width/tooltip/header-centering fixes across Reviews, PR list, and Repositories (all on the TanStack resizable table now), plus search-icon focus on click. - Move total comment count from the diff viewer panel up into the review header stats. - Collapse duplicate circuit-breaker pills in the Events tab (group by failure type instead of exact message text, which never matched across Go's wrapped-error/cause logging). - Make Merge Requests the primary/default Explore destination over Repositories (nav links, mega menu, in-page tabs, default route). - Force-resolve stuck pending/in-progress stages in the review progress bar once the review's own status is terminal, so a completed review whose event history exceeds the frontend's fetch window doesn't show a stale in-progress stage. - Fix the Auto-Approve risk score message: a tied score/cutoff no longer reads as "above", and the cutoff's percentile label is computed the same way as the score's (both via PercentileOf) instead of echoing the configured threshold, so the two numbers are directly comparable. Add a minimum-sample guard to the percentile cutoff calculation so a handful of historical scores can't produce a degenerate cutoff that collapses onto the distribution's max. - Fix the Auto-Approve tab's "Configure" links not switching the target settings sub-tab when Settings is already mounted (stale one-shot useState instead of reacting to URL changes). LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
…gmoid br_sum (a raw SUM across all blast-radius hunks, real production values 1700-4840) was scored against a weight calibrated for an assumed 0-100 range, pushing the sigmoid input past the point float64 can represent 1+exp(-z) as anything but exactly 1.0. Every other unbounded count feature (critical/warning/security counts, taxonomy counts) shared the same structural flaw. Log1p-compress every unbounded feature, retune Prior() weights to match, add a z-clamp safety net, add regression tests built from real production feature vectors, add a migration to reset the stale seed model, and surface saturation stats in the Simulate endpoint and Settings UI so this is visible going forward instead of only discoverable by hand-querying production. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
|
Error generating summary: request timeout: API call exceeded deadline This review completed with partial results: 6 out of 7 batches processed successfully. Failed batches: [batch-4] The comments and analysis shown are based only on the files that were successfully reviewed. Some files may have been skipped due to batch processing errors. |
| partialFailure = true | ||
| if !ok { | ||
| return nil, fmt.Errorf("batch %s not found in results", batchID) | ||
| fmt.Printf("[PARTIAL FAILURE] Batch %s not found in results, skipping\n", batchID) |
There was a problem hiding this comment.
Severity: warning
A missing result from ProcessAll can mean the context was canceled or the queue stopped early, not that the batch failed. This branch can turn cancellation into a partial success when at least one batch completes.
Suggestions:
- Check ctx.Err() in the !ok branch and return it before treating the missing batch as a partial failure.
- Consider having ProcessAll return the cancellation cause so callers can distinguish abort from per-batch failure.
| batchResult, ok := taskResult.Result.(*batch.BatchResult) | ||
| if !ok { | ||
| return nil, fmt.Errorf("invalid result type for batch %s", batchID) | ||
| failedBatches = append(failedBatches, batchID) |
There was a problem hiding this comment.
Severity: warning
Invalid result type is an internal invariant violation, not a transient LLM or parse failure. Skipping it can hide a programming bug and produce an incomplete review with only a warning.
Suggestions:
- Keep returning an error when taskResult.Result is not *batch.BatchResult; only taskResult.Error should be treated as a partial failure.
|
|
||
| // If NO batches succeeded, fail the entire review - there's nothing to aggregate. | ||
| if len(batchResults) == 0 { | ||
| return nil, fmt.Errorf("all %d batches failed: %v", len(batchInput.Batches), failedBatches) |
There was a problem hiding this comment.
Severity: warning
An empty batch list also reaches this path and reports 'all 0 batches failed: []', turning a no-op review into an error.
Suggestions:
- If len(batchInput.Batches) == 0, return an empty result before this all-failed check.
- If ctx.Err() != nil, return ctx.Err() before returning this generic all-batches-failed error.
livereviewbot
left a comment
livereviewbot
left a comment
There was a problem hiding this comment.
✅ LiveReview policy: Approved · Review #13545
| Stage | Result | Detail |
|---|---|---|
| Eligibility | ✅ | author level 1 >= required 1 |
| Static rules | ✅ | no blocking rulesets configured |
| Risk score | ✅ | Risk score 1.00 (P0) is below the cutoff of 1.00 (P100), configured for P20 |
| LLM classification | ✅ | class "logic-change", confidence 9 |
| Final decision | ✅ | approved |
The "Risk assessment" header stat is visible on every tab, but its value started from an empty/default score list until the Findings tab was opened and its diff data finished loading. In between, riskAssessment() silently fell back to a different, structural-only blast-radius score to compute a confident tier label, then recomputed once Findings handed over the real (finding-aware) scores -- visibly flashing e.g. "High Risk" to "Moderate risk". DiffViewerPanel had a second, smaller version of the same issue: it could report an empty score list before its own diff fetch even resolved. Distinguish "not loaded yet" (null) from "loaded, confirmed zero" ([]) for the risk-scores state, show a neutral "Calculating..." state while the findings-aware score isn't ready instead of guessing, and don't report scores out of DiffViewerPanel until its diff data has actually loaded. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
… scan GHSA-3g3m-2fjr-6pw4 (glob) and GHSA-g3hv-75wg-m249 (minimatch) were disclosed in the last day and have no patched release in the version lines this repo's overrides pin (glob 9.x-13.x, minimatch 3.x/9.x/10.x) - only glob 14.0.2+/minimatch 11.0.1+ fix them. Every occurrence in ui/package-lock.json and extension/livereview/package-lock.json is dev-only build/test tooling (webpack plugins, eslint, jest, mocha, @vscode/test-cli); `npm ls glob/minimatch --omit=dev` is empty in both, so neither ever ships. Bumping either across its major version would mean bumping every dependent toolchain package too, which is a separate change from what this PR is about. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
internal/riskscore/learning_cycle_test.go built its br_sum feature value by hand (bypassing ExtractFeatures), using the same raw-scale convention ExtractFeatures used before the log1p rescaling fix. Fixed the same way as the earlier referencemix.go/TestScoreOverrideEffect fixes: apply math.Log1p to the br_sum component while leaving br_max/br_weighted_avg raw (they were never log1p'd). internal/policyengine/testdata/scenarios.json's two threshold-dial scenarios used a blast_hunks combined value (8) that, under the corrected model, now scores above their fixed P80 reference cutoff instead of below it -- a real, intended effect of the recalibration (log1p compression is milder at small-to-moderate magnitudes than it is at the extreme values the retuned weights were calibrated against, so a small-but-present blast radius now registers somewhat more than before). Lowered combined to 3 so both scenarios land back in the "moderately risky" ~0.6 score band their rationale already describes, preserving the actual thing they test (same score, different percentile dial, different outcome) rather than its incidental exact number. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
🔍 LiveReview
⚙️ Customize risk-based approval policy for this repository or your organization.
This PR bundles two related review-visibility improvements, a dev-tooling fix found along the way, and a round of production bug fixes found while verifying the push-notification feature end to end.
Part 1: Stream findings into the Findings tab while a review is running
Use case
Today, when you hit Trigger Review or Start a Review, the webui bounces you to the Events tab (a log/timeline view) and keeps the Findings tab empty until the entire review finishes. For a large diff spanning many files, that means staring at a scrolling log with no sense of which files have problems until everything is done.
This PR makes the Findings tab usable the whole time a review is running:
Out of scope for this PR: posting comments to GitHub/GitLab/etc. PR threads is untouched and still happens as a single batch at the end. Making that incremental too is a separate, larger change (it would need to handle the helper "polish" stage potentially rewording a comment after it's already been posted) and is intentionally left for a future PR.
Algorithm / data structures
The backend has quietly been capable of this for a while: every time a batch of files finishes its LLM call,
internal/ai/langchain/provider.goemits abatch/completedevent carrying that batch's raw[]*models.ReviewCommentinto thereview_eventstable (EmitBatchComplete→DatabaseEventSink.EmitBatchEvent→CreateBatchEvent). The webui simply never read that payload — the Events tab only ever rendered it as a log line. git-lrc's local review UI already solved exactly this by polling the same events endpoint and merging streamed comments into its file view; this PR ports that logic into LiveReview's hosted webui.New module:
ui/src/components/reviews/diffviewer/reviewStreamState.ts(ported ~1:1 fromgit-lrc:internal/staticserve/static/components/review_stream_state.mjs, unit-tested inreviewStreamState.test.ts):extractNewEvents): maintains aSet<string>of seen event ids across polls (falling back to a stable composite key — type/time/batchId/stringified-data — when the server omits an id), so re-polling with overlap never double-applies a batch's comments.inferReviewStatusFromEvents): scans events backwards for the lateststatusevent or acompletionevent, to detectcompleted/failedfrom the event stream itself without waiting on a separateGET /reviews/:idpoll.extractExternalCommentsFromEvents/normalizeStreamedComment): filters fortype === 'batch' && data.status === 'completed', readsdata.comments[], and normalizes field casing —models.ReviewCommenthas no JSON tags so it marshals PascalCase (FilePath,Line, ...) over the wire, whileDiffReviewComment(the shapeGetDiffReviewStatusreturns) is lower camelCase. DropsIsInternalcomments, which are synthesis-only and never meant to reach a user.appendStreamedCommentsToFiles): an O(files + comments) merge — builds aMap<file_path, index>once, then appends each incoming comment to its file'scomments[]by path lookup, immutably (returns new file objects, doesn't mutate the input array react state depends on).DiffViewerPanel.tsxwires this into two effects:getDiffReviewfetch on mount/reviewId change (unchanged).data.statusiscreated/in_progress: pollsGetReviewEvents(reviewId, since, limit=1000)every 1s (matching git-lrc's cadence), runs the response through dedup → extract → merge, and accumulates streamed comments in state. The moment inferred status flips tocompleted/failed, it fires one finalgetDiffReviewcall and clears the streamed-comment accumulator — that response already carries the real (possibly helper-polished) comments, so there's nothing left to merge on top of it. Rendered files =appendStreamedCommentsToFiles(data.files, streamedComments), memoized.Deeper technical details
Backend change was required, not just a frontend port.
GetDiffReviewStatus(internal/api/diff_review.go) used to early-return{status, review_id}with nofileskey at all wheneverreviewRecord.Status != "completed"— the diff/hunk structure (fetchPreloadedChanges+buildDiffFiles) was only ever computed in the completed-path below. That meant there was nothing for the frontend to merge streamed comments into before completion. Fixed by extracting the trigger-type-aware diff-resolution logic (blob storage lookup, falling back to a live re-fetch from the PR/MR or fromscheduled-trigger metadata depending onTriggerType) into a new helper,resolvePreloadedChanges, called unconditionally before branching on completion status. The non-completed response path now also returnsfiles: buildDiffFiles(preloaded, nil)— hunks with emptycomments, to be filled in by the frontend's event-driven merge. The completed path is otherwise unchanged (still callsbuildDiffFiles(preloaded, result.Comments)with the real, final comments fromreview_resultmetadata).Why the final reconciliation fetch is safe, not just convenient. The helper "polish" stage (
internal/review/service.golines ~259-264) only ever rewords findings the leader model already produced — per the code's own comment, it never adds, drops, or relocates one. So a comment shown early via the batch-event stream is already correct in substance and position; the only thing the finalgetDiffReviewfetch can change is wording. That's why clearingstreamedCommentsand trusting the final fetch wholesale, rather than trying to diff/patch it against what was already shown, is the right trade — verified by reading git-lrc's own reason for being "fast": its local CLI-diff reviews skip PR-posting entirely (Provider.Type == "cli") and normally run without the helper stage at all, so there was nothing to reconcile there; LiveReview's hosted reviews can have the helper stage, hence the explicit reconciliation step this PR adds that git-lrc's UI never needed.Tab-switching changes in
ReviewDetail.tsx. Removed the one-time-per-review effect that force-switchedactiveTabto'events'wheneverreview.status !== 'completed'— Findings is now reachable (and useful) the entire time a review runs. The existing effect that jumps back to Findings on completion is kept as a courtesy. The existingkey={review?.status}on<DiffViewerPanel>(forcing a clean remount on each status transition) is left untouched and composes fine with the new polling — each remount just starts a fresh poll cycle for whatever status it mounts under.Gating/empty-states in
DiffViewerPanel.tsxwere restructured aroundfiles.lengthinstead ofdata.status === 'completed': no diff data at all → unchanged "no diff data" empty state; a failure with zero streamed findings → unchanged failure empty state; a failure with some findings already streamed in → now shown with a warning banner instead of being discarded; still-running with no files yet (diff not resolved server-side yet) → new "Preparing the diff…" empty state; still-running or failed with files present → full diff/findings view, with an info or warning banner noting the in-progress/failed state and the running finding count.Part 2: Browser push notifications for review progress
Use case
Once you trigger a review, the only way to know it's progressing or done used to be keeping the tab open (helped by Part 1 above). There was no way to walk away and get pinged. This adds real OS-level browser notifications — via the Web Push API + a service worker, so they arrive even if the tab or browser is closed — for two moments per review: the first finding becoming available, and the review completing (or failing).
PushNotificationBanner) on the review detail page — not a toast, which turned out to never render action buttons at all (see Part 4) — with an inline "Enable notifications" button, a "Settings" link, and guidance if the browser has already blocked notifications for the site.Algorithm / data structures
Two different trigger points, for a good reason:
First finding available. Batches run concurrently (
internal/batch/task_queue.goworker pool), so "first" needs an atomic claim, not an in-memory flag, or a goroutine race would double-notify.pushnotify.ClaimFirstFindingrunsUPDATE reviews SET first_finding_notified_at = now() WHERE id = $1 AND first_finding_notified_at IS NULL— only the caller that gets a row back (verified under 20-goroutine concurrency inpushnotify_test.go) proceeds to notify. This is wired into both of the codebase's independent event-sink implementations (internal/review_processor/events.go'sCreateBatchEvent, used by the async job-queue path, andinternal/api/polling_event_service.go'sCreateBatchEvent, used by the synchronous CLI-diff API path) — missing either one would silently drop notifications for half of LiveReview's review-triggering paths.Completion/failure. Terminal-status writes are scattered across
internal/jobqueue/review_worker.go,scheduled_review_worker.go, andinternal/review_processor/manual.go— too many call sites to safely hook individually. Centralized instead insideReviewManager.UpdateReviewStatus(internal/review_processor/reviews.go): after the status row update succeeds for a terminal status, it calls a package-level notifier hook (review_processor.SetNotifier(n)/ a nil-safe package var), set once at server startup ininternal/api/server.go. A package-level hook, not a constructor param, becauseNewReviewManager(db)is called fresh at many unrelated call sites — threading a notifier through all of them would recreate the same "touch everything" problem this avoids.Delivery (
internal/pushnotify): a VAPID keypair is generated once at first use and stored insystem_settings(same pattern as SMTP credentials).NotifyReviewEventresolvesreviews.user_email→ ausersrow (skips silently if empty — the "no identifiable human" case), checks that user'suser_notification_preferencesrow (missing row = both enabled, matching the "on by default" decision with no backfill needed), loads theirpush_subscriptionsrows (one per browser/device), and sends viawebpush-go; a 410/404 response prunes that subscription inline.Deeper technical details
New schema (
db/migrations/20261030134721_create_push_notifications.sql):push_subscriptions(id, user_id FK, endpoint UNIQUE, p256dh, auth_key, user_agent, created_at, last_used_at),user_notification_preferences(user_id PK FK, notify_first_finding, notify_completion, updated_at), andreviews.first_finding_notified_at TIMESTAMPTZ(the claim-once marker above).New endpoints, session-only (
authMiddleware.RequireAuth(), never API-key — a push subscription is tied to one specific browser's real endpoint, not something automation should manage on a user's behalf):GET /push/vapid-public-key,POST/DELETE /push/subscriptions,GET/PUT /users/me/notification-preferences. The three mutating routes are wired throughs.audited(...)with newauditlog.Specentries/action constants, documented indocs/audit-events.mdper this repo's audit-logging rules.user_idis always read from the authenticated session context, never the request body.Frontend: a minimal push-only service worker (
ui/public/sw.js— justpush/notificationclick, not a full PWA/offline cache, usingnotification-icon.png—favicon.svgrasterized at 192x192 viavl-convert svg2png, since.icoisn't reliably rendered as a notification icon),pushNotifications.tswrappingpushManager.subscribe/getSubscription,PushNotificationBanneron the review detail page, and a new Settings → Notifications tab — plus the required mega-menu entry androutes_guidedoc per this repo's navigation/RAG-doc conventions.Part 3: Fix
db/schema.sqldrift and the dump tooling behind itWhile working on Part 2, found
db/schema.sqlhad silently drifted from the actual migration history —dbmate dumpwas failing locally because the host'spg_dump(16.13, from Ubuntu's own repo) was older than the dev Postgres server (18.6), andpg_dumprefuses to dump a server newer than itself. This wasn't new or caused by this PR; it meant at least one prior migration (users.time_zone, already merged in an earlier PR) had also never made it intoschema.sql.Fixed properly rather than routed around:
scripts/install-pgdg-client.shinstalls a matching-majorpostgresql-clientfrom the official PGDG apt repo (one-time, needs sudo).scripts/dbmate-dump.sh(wired tomake db-dump) now checks the localpg_dumpmajor version against the live server'sserver_version_numbefore dumping, and fails fast with the exact fix command instead of a confusing dbmate error or a silent workaround.db/schema.sqlregenerated and verified byte-identical to a known-good Docker-container-produced dump except for the dumper-package label — confirming the catch-up is accurate, not just "a diff happened."Part 4: Bugs found verifying Part 2 against a real deployment
reviewprocessor.SetNotifier(...)was only called insideNewServer()(theapiprocess). The backgroundworkerprocess (ecosystem.config.js'slivereview-worker/-2,cmd/worker.go→api.WorkerContext()) is where queued reviews actually run and where batch-completion/status events fire — so in that process the notifier stayednil, and everyNotifyReviewEventcall silently no-op'd with no error or log line. A subscribed user's completed review with findings never setfirst_finding_notified_atand never sent a push. Fixed by movingSetNotifierinto the sharedappContext()bothNewServerandWorkerContextcall.GET /reviews/:id/accounting500'd on every freshly-triggered review.GetReviewAccountingTotals'stoken_tracked_opscolumn is a bareSUM(CASE WHEN ... THEN 1 ELSE 0 END), unliketotal_billable_loc'sCOALESCE(SUM(...), 0)right next to it —SUM()over zero matchingloc_usage_ledgerrows (every review, until its first LLM call is accounted) isNULL, and scanning that into a non-nullableint64failed the whole query. Fixed with a matchingCOALESCE; regression test confirmed to fail against the old query and pass against the fix.#<id>,owner/repo, PR/MR title) in the body instead of a bare repository URL, a proper PNG icon instead of.ico, and "Review completed with errors" instead of the blunter "Review failed" (most failures here are one erroring batch, not total loss).PUT /users/time-zone) — couldn't reproduce a bad zone name server-side, but this guards against stray whitespace some browser/OS combinations are known to append.GET /github-app/demo404s are expected on boxes withoutGITHUB_APP_IDconfigured, not a bug —useGitHubAppConnectedalready documents and handles a 404 there as "not connected."Part 5: Risk model saturation fix, found while reading the auto-approval table above
While verifying Part 4 end to end, the auto-approval table this PR's own bot comment renders (above) surfaced something off: review #13543's risk message read "Risk score 1.00 (P100) is above the cutoff of 1.00 (P20)" — a score pinned at exactly 1.00 against a cutoff also exactly 1.00. Traced it back to a real scoring bug, not a display bug.
Root cause
internal/riskscore/features.go'sbr_sum(a raw, unboundedSUM(combined)across every blast-radius hunk in a diff) was scored byinternal/riskscore/model.go'sPrior()with a weight calibrated for an assumed 0-100 range. Querying production (read-only) showed realbr_sumvalues running 1700-4840 — at the default blast-radius override (2.0x), that pushes the sigmoid's inputzto roughly 190, and in float64,1 + exp(-z)rounds to exactly1.0for anyzpast ~37, well before it would ever literally overflow. Org 3's entire 10-review history scored exactly1.0; org 677 showed a clean before/after (normal varying scores whilebr_sum = 0, pinned to1.0the instant it crossed ~150).Checked whether this was
br_sum-specific via the category-override sliders (Settings → Risk Model, 0-10x per category): every other raw count feature (critical_count,warning_count,security_count,br_high_count, and the 9 finding-taxonomy counts) shared the identical structural flaw — real production data already showswarning_countup to 37 andcritical_countup to 12, both alone enough to saturate even without any override.Fix
internal/riskscore/features.go: every structurally-unbounded count/sum feature is nowmath.Log1p-compressed before scoring (matching the convention already used forfiles_changed/lines_added), not just the two that happened to be compressed before.internal/riskscore/model.go:Prior()'s weights retuned for the new log1p'd scale (new_weight = intended_contribution_at_reference / log1p(reference), preserving the existing priority ordering: critical > security > correctness/reliability > performance > the rest). Added a hardzClampAbssafety net (25 — comfortably inside float64's real precision-collapse zone around|z| > ~37, but with enough headroom that genuinely different high-risk reviews stay distinguishable rather than all collapsing to the clamp boundary) around the sigmoid input in bothScoreandscoreRaw, so no future weight/override/feature-scale mistake can reproduce this class of bug.internal/riskscore/saturation_test.go(new): regression fixtures built from the real production feature vectors found above (org 3 and org 677), confirming they no longer score bit-identical or pinned at the ceiling; a sweep of every category override 0.1x-10x against a realistic battery, asserting the score always moves monotonically and never jumps straight to the clamp; a fuzz test confirming the score is always strictly inside(0, 1). Also fixedinternal/riskscore/referencemix/referencemix.go(shared test fixture data, built by hand rather than throughExtractFeatures) and two existing tests (TestScoreOverrideEffect,TestPriorScoreMonotonicInEachFeature) that had silently drifted out of sync with the new encoding or sampled unrealistically small feature ranges — exactly why they never caught this.db/migrations/20261031140000_reset_risk_model_seed_for_feature_rescale.sql(new): deletes the single stale shared seed row soEnsureSeedModel's existing idempotent bootstrap reinserts it from the correctedPrior()on next use. Safe: the entire production DB has exactly onerisk_model_versionsrow and zero per-org trained models, so there's no fitted-model data at risk.internal/api/auto_approve_policy_handlers.go/ Settings UI: theSimulateendpoint now returns asummary(scored/skipped counts, how many scores are pinned at either extreme, score min/max/mean), rendered as a warning banner in Settings → Risk Model when a slider configuration is producing non-differentiating scores — the same signal that previously required hand-querying production to notice.Part 6: Risk assessment header flickering between risk tiers on tab switch
Found while testing Part 5 in the browser: the always-visible "Risk assessment" header stat would briefly show one risk tier (e.g. "High Risk", red) then correct to a different one (e.g. "Moderate risk", amber) a moment after switching into the Findings tab.
Cause: the header stat is driven by a score that starts empty until the Findings tab's diff data loads. In between, it silently fell back to a structural-only blast-radius score (no finding severity folded in) to show something immediately, then recomputed once Findings handed over the real, finding-aware scores — which can land in a different tier.
DiffViewerPanelhad a smaller version of the same issue, reporting an empty score list before its own diff fetch had even resolved.Fixed by distinguishing "not loaded yet" (
null) from "loaded, confirmed zero" ([]) for the risk-scores state: the header now shows a neutral "Calculating..." state while the real score isn't ready yet, instead of guessing and correcting later.Testing
go build ./...andgo test ./internal/riskscore/... ./internal/policyengine/... ./internal/api/...(and the full repo suite) pass clean, including the new saturation/regression tests built from real production feature vectors.npx tsc --noEmitpasses clean after the Simulate-summary and risk-assessment-flicker UI changes.make security-osv,make security-govulncheck,make security-semgrep,make security-gitleaks. (OSV required two new ignore-list entries,config/osv-scanner.toml, for glob/minimatch CVEs disclosed in the last day with no patched release in the version lines this repo's dev tooling uses — confirmed dev-only vianpm ls --omit=devin bothui/andextension/livereview/, so neither ever ships.)ExtractFeatures) using pre-rescaling raw values:internal/riskscore/learning_cycle_test.go'sbr_sumcomponent, andinternal/policyengine/testdata/scenarios.json's twothreshold-dialscenarios (their fixedblast_hunks.combinedvalue scored just above the new model's recalibrated P80 reference cutoff instead of below it — lowered it so both land back in the "moderately risky" band their own rationale already describes).govulncheck,gitleaks,osv-scan,semgrep/ci) against this branch viagh workflow runand confirmed green before merging.TestAIMD_ConvergesToSecretCapacity(internal/llm/adaptivelimiter, added earlier in this same PR for unrelated LLM-call concurrency control) is flaky undergo test ./...— converges to capacity 16 against a[2, 15]band in most local and CI runs, passing occasionally. Left as-is since fixing an unrelated concurrency-limiter's convergence tolerance is out of scope here; worth a follow-up.Testing (Parts 1-4)
go build ./...andmake testall(full repo Go test suite, matching CI'slivereview-tests.yml) pass clean, includinginternal/pushnotify(claim-once race under concurrency, preference-filtering, empty-user-email skip, the new notification-content/label tests) and the newstorage/licenseaccounting regression test.npx tsc --noEmitpasses; full jest suite passes, includingPushNotificationBanner's tests (eligibility/blocked-state/subscribe-failure-retry).make security-osv,make security-govulncheck,make security-semgrep,make security-gitleaks.livereview.hexmos.com): triggered a review, confirmed the notification arrives with the tab closed, confirmed the icon/body/click-through fixes in Part 4, confirmed the accounting-poll 500 is gone.LiveReview auto-approval decisions