feat(organizations): spending alerts with monthly spending threshold - #5457
Open
jrf0110 wants to merge 6 commits into
Open
feat(organizations): spending alerts with monthly spending threshold#5457jrf0110 wants to merge 6 commits into
jrf0110 wants to merge 6 commits into
Conversation
Contributor
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Executive SummaryLow-balance crossings that land at a negative org balance cannot be claimed, and the new Measure/Group selects render behind the alerts drawer so group-scoped monthly spending cannot be configured in the UI. Overview
Fix these issues in Kilo Cloud Issue Details (click to expand)WARNING
Files Reviewed (40 files)
Previous Review Summaries (2 snapshots, latest commit 5286814)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 5286814)Status: No Issues Found | Recommendation: Merge Executive SummaryReviewed the monthly spending alerts implementation (schema, delivery/claim, API/auth, UI, and spec alignment) at Files Reviewed (49 files)
Previous review (commit 11bf108)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Reviewed by grok-4.6 · Input: 204.6K · Output: 21.4K · Cached: 881.9K Review guidance: REVIEW.md from base branch |
Implements the monthly spending threshold PRD as a general, extensible Alerts surface rather than a single per-organization setting. An organization may create any number of alerts. Each alert has an immutable type, a versioned type-specific configuration, an enabled/ disabled/archived lifecycle, and a configuration version used for optimistic concurrency. Delivery is claimed durably per alert, period occurrence, recipient and channel, so two alerts never suppress each other's email and no recipient is notified twice for one period. - persistence: `organization_alerts` and `organization_alert_deliveries`, with recipient identity stored only as a keyed digest - domain: typed alert union, versioned `calendar_month_utc` period resolution, exact whole-cent threshold conversion, recipient normalization and a 1-10 per-alert bound - API: cursor-paginated list plus create/update/setEnabled/archive, gated by organization billing authority; Enterprise entitlement is required to create, enable or expand, while disable, archive and recipient removal stay available after entitlement loss - UI: Account > Alerts page with a DrawerStack editor, an alert type dropdown defaulting to Monthly Spending with a disabled "More coming soon" option, and a type registry so a second type needs no shell changes; the existing low balance alert is surfaced alongside it - delivery: hourly cron evaluates enabled alerts in bounded keyset pages, aggregates spend once per organization-period, claims under a per-alert-period advisory lock, re-reads eligibility before sending, and distinguishes retryable pre-submission failures from ambiguous provider outcomes that must never be retried - lifecycle: leaving Enterprise disables alerts and cancels claimed work; organization deletion removes recipient addresses
Brings `main` up to date on this branch and adds the implementation of the monthly spending threshold spec. The spec itself is revised in the merged commit: alerts are a collection rather than one setting per organization, so delivery identity, recipient admission and the editor UX are all alert-scoped.
jrf0110
changed the title
The per-alert-period advisory lock only protected the 10-recipient admission cap. At-most-once delivery per recipient and period is guaranteed by the unique index on delivery identity, which needs no serialization: a concurrent evaluator's overlapping insert is absorbed by ON CONFLICT. The cap is a fanout bound on mid-period recipient churn rather than a safety invariant, and the configuration schema already limits an alert to 10 recipients, so a single sweep cannot exceed it. Overlapping evaluation may now admit a few extra addresses for one period, which the spec records as acceptable. Claiming is a single statement, so the surrounding transaction is gone too. Adds a test that concurrent claims produce exactly one row per recipient, since that invariant now stands alone.
Resolves conflicts from concurrent work on main: - `packages/db/src/schema.ts`: main added `organization_domain_claims` directly after the organizations table, where the alert tables also landed. Both are kept. - migrations: main took slots 0231 and 0232, so the branch-local migration and its snapshot were discarded and regenerated as 0233_condemned_hardball against the merged schema. The generated SQL contains only the two alert tables, with no drift and no destructive statements. - `.kilo/skills/specs/SKILL.md`: keeps both new index entries, spending alerts and verified-domain auto-join.
…spending - Recreate the legacy low-balance alert as a new low_balance type in the collection-backed alerts system (organization_alerts), evaluated just-in-time from every usage-mutation call site (token usage ingest, Exa, AI Gateway) rather than a periodic sweep, since a balance change has exactly one call site unlike accumulated spend. Coexists with the legacy organizations.settings-based low-balance alert; nothing about it changes. - Generalize organization_alert_deliveries.measured_spend_microdollars to measured_value_microdollars (direction-agnostic) so low_balance's below-threshold semantics can share the claim/dispatch machinery with monthly_spending's at-or-above semantics. Two migrations split the rename into an unambiguous add then drop. - Add scope to monthly spending alerts: measure the whole organization or a specific organization group, via a live-membership join against organization_group_memberships at evaluation time. A scope naming a deleted group is treated as invalid rather than silently measured as zero spend. - Extract shared threshold parsing/formatting into alert-thresholds.ts so both alert types use the same USD<->microdollars contract.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| recipients, | ||
| configurationVersion: alert.configuration_version, | ||
| thresholdMicrodollars, | ||
| measuredValueMicrodollars: newBalanceMicrodollars, |
Contributor
There was a problem hiding this comment.
WARNING: Negative org balances cannot be claimed, so a crossing that lands below zero never emails
claimAlertDeliveries inserts measuredValueMicrodollars: newBalanceMicrodollars, but organization_alert_deliveries enforces measured_value_microdollars >= 0. Org balances are allowed to go negative. A single debit from at/above the threshold to below zero is a real crossing (previous >= threshold && new < threshold) and then the INSERT raises check_violation. There is no per-alert try/catch, so the throw aborts the rest of this org's alerts, no delivery row is written, and the dispatch sweep cannot retry.
Clamp to Math.max(0, newBalanceMicrodollars) at claim time, or relax the check to allow negative measured values for low_balance.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| <SelectTrigger id={SCOPE_ID} className="w-full"> | ||
| <SelectValue /> | ||
| </SelectTrigger> | ||
| <SelectContent> |
Contributor
There was a problem hiding this comment.
WARNING: Measure dropdown renders behind the alerts drawer
SelectContent defaults to z-50 and portals to document. The drawer overlay is z-[60] and the panel is z-index: 62. The type picker in AlertEditorPanel already sets z-[70] for this. Without it, the new Measure options are not clickable.
Suggested change
| <SelectContent> | |
| <SelectContent className="z-[70]"> |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| > | ||
| <SelectValue placeholder="Choose a group" /> | ||
| </SelectTrigger> | ||
| <SelectContent> |
Contributor
There was a problem hiding this comment.
WARNING: Group picker renders behind the alerts drawer
Same stacking issue as the Measure select: default z-50 is below the drawer (z-[60]/62). Group-scoped monthly spending cannot be chosen in the UI.
Suggested change
| <SelectContent> | |
| <SelectContent className="z-[70]"> |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
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
1 participant
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
Implements the monthly spending threshold spec, and revises the spec itself in one
important way: an organization may create as many alerts as it wants instead of
a single monthly threshold setting.
The surface is a general Account → Alerts page. Creating an alert starts from an
alert-type dropdown that defaults to Monthly Spending and shows a disabled
More coming soon option; changing the type swaps in that type's editor. As with
the Group Policy editor, a type registry plus
DrawerStackmeans a second alerttype needs no changes to the shell.
Why the multiplicity change matters
Allowing many alerts is not just a UI change. It invalidates the spec's original
storage and deduplication model:
alert + period occurrence + recipient digest + channel), otherwise two independently configured alerts suppress eachother's email
tables rather than
organizations.settingsWhat's implemented
Persistence —
organization_alertsandorganization_alert_deliveries(migration
0231). Delivery rows never store an address, only a keyed digest; theaddress is re-read from configuration at send time.
Domain — typed alert union, versioned
calendar_month_utcperiod resolution toa stable occurrence identity and half-open UTC interval, exact whole-cent threshold
conversion with no floating-point drift, recipient normalization/dedup with a 1–10
bound (0 allowed while disabled so a disclosure can always be removed).
API —
organizations.alerts.{list,create,update,setEnabled,archive}behind theexisting organization billing authority, including inherited parent-over-child
authority. Enterprise entitlement is required to create, enable, or expand; disable,
archive, and recipient removal deliberately remain available after entitlement loss.
Edits use optimistic concurrency on configuration version. Audit entries carry the
alert ID and recipient counts, never addresses.
UI — billing-gated sidebar entry and server-authorized route, paginated alert
list, the Monthly Spending editor (threshold, up to 10 recipients, UTC month
explanation, informational/no-cap framing, required disclosure confirmation, inline
errors), and inline disable/archive confirmation inside the drawer. The existing low
balance alert is presented alongside as a clearly separate card, reusing its
implementation.
Delivery — hourly cron evaluates enabled alerts of live Enterprise organizations
in bounded keyset pages, aggregates spend once per organization-period and
compares each alert independently, claims delivery under a per-alert-period advisory
lock before any provider call, re-reads eligibility before sending, and cancels stale
work. A definitive pre-submission failure stays retryable; an ambiguous provider
outcome is retained and never retried automatically.
Lifecycle — leaving Enterprise disables alerts and cancels claimed work without
re-enabling on a later upgrade; organization deletion removes recipient addresses,
which matters because deletion is soft and no FK cascade fires.
Validation
pnpm format,pnpm typecheck,pnpm dependency-cycle-checkcleanpackages/dbschema/migration-consistency suite anddrizzle-kit checkpnpm lintreports only 2 pre-existing errors in untouchedservices/user-data-export/src/index.tsmigration 0204case inpackages/db/src/schema.test.tsreads a migration file that was renumbered awayFollow-ups
provider_message_idis not yet written;send()does not surface a Mailgun IDMAX_DISPATCH_PER_RUNsends serially, worth confirming against the route'seffective
maxDurationonce real volume exists