feat(emails): transactional emails for top-up and KiloClaw purchase#2851
Open
kilo-code-bot[bot] wants to merge 11 commits intomainfrom
Open
feat(emails): transactional emails for top-up and KiloClaw purchase#2851kilo-code-bot[bot] wants to merge 11 commits intomainfrom
kilo-code-bot[bot] wants to merge 11 commits intomainfrom
Conversation
Send exactly-once transactional emails for two purchase events: - Credit top-up (manual Checkout or auto-top-up), hooked into processTopUp's idempotent post-processing block and gated by the existing unique constraint on credit_transactions.stripe_payment_id. - KiloClaw subscription started (first paid billing period only), hooked into applyStripeFundedKiloClawPeriod and gated by an insert-before-send into kiloclaw_email_log so renewals are skipped.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| }); | ||
| } | ||
|
|
||
| if (resolvedInstanceId && amountMicrodollars > 0) { |
Contributor
Author
There was a problem hiding this comment.
WARNING: Existing subscriptions can receive a misleading "started" email
This only checks for a missing kiloclaw_subscription_started log row. Existing Stripe-funded/hybrid subscriptions won't have that new marker, so their first paid renewal after this deploy will insert it and send "Your KiloClaw subscription is active" even though it's a renewal. Gate this on the pre-settlement subscription state/first activation, or backfill markers for already-active paid subscriptions before relying on the log as the only first-period signal.
| const actual = jest.requireActual<typeof emailModule>('@/lib/email'); | ||
| return { | ||
| ...actual, | ||
| send: jest.fn((params: unknown) => sendMock(params)), |
Contributor
Author
There was a problem hiding this comment.
WARNING: This mock does not intercept the helper sends
sendCreditsTopUpEmail is returned from jest.requireActual, and inside email.ts it calls the module-local send function, not this mocked send export. As a result processTopUp still goes through the real email path and sendMock.mock.calls remains empty, so these tests won't verify the new top-up email. Mock sendCreditsTopUpEmail directly or mock the lower-level Mailgun transport instead.
Contributor
There was a problem hiding this comment.
The root cause is that sendCreditsTopUpEmail calls the module-local send function inside email.ts — so replacing the exported send with a mock has no effect on internal calls. Mocking sendCreditsTopUpEmail directly via jest.mock('@/lib/email', factory) also doesn't reliably intercept calls from credits.ts with @swc/jest, because the factory mock doesn't get picked up by transitive module references.
The fix is to mock at the transport boundary: replace sendViaMailgun from @/lib/email-mailgun and verifyEmail from @/lib/email-neverbounce, which are separate modules accessed via property lookup at call time. Assertions can then check the mailgun spy — e.g. verify the subject is subjects.creditsTopUp for manual top-ups and 'Kilo auto top-up successful' for auto top-ups.
Contributor
Author
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Resolved Since Last Review
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (12 files)
Fix these issues in Kilo Cloud Reviewed by gpt-5.5-2026-04-23 · 1,076,070 tokens |
…onStarted in email-testing-router
…eriod Existing paid subscribers have no kiloclaw_email_log row for the new email_type, so their first renewal after deploy would insert a marker and send a misleading 'subscription is active' email. Gate the send on isFirstPaidPeriod — true only when the subscription was previously in 'trialing' status (trial → paid transition). Renewals and reactivations have before.status === 'active' and are skipped.
… settlement Adds best-effort duplicate-settlement email recovery for the KiloClaw subscription-started email when a webhook replay hits the duplicate-credit path. Eligibility is derived from a durable subscription change-log entry proving a prior paid activation for the same subscription, plan, and period, with a 31-day window guard. Also broadens activation eligibility to cover canceled rows (including canceled paid rows that resubscribe), and strengthens tests to exercise the production settlement path end-to-end with realistic kiloclaw_email_log idempotency shapes.
… identity Replaces the created_at ordering used in didStripeSubscriptionCreatedRecordEligibleActivation with a direct plan + period_start + period_end identity match against the stripe_subscription_created row's after_state. created_at defaults to now() (transaction-start), which under concurrent webhook transactions can reorder relative to commit chronology and let a stale activation log re-fire the email on a later renewal when the email-log marker is absent (e.g. after the intentional rollback in maybeSendKiloClawSubscriptionStartedEmail error path). The handler in stripe-handlers.ts already stamps the Stripe-derived plan and period boundaries onto both the subscription row and the change-log after_state, so an activation log can only match the specific period it activated. A later renewal covers a different period and cannot match. Mirrors the existing identity-match approach in didPriorSettlementRecordPaidActivation. Tests updated to stamp period boundaries when simulating handleKiloClawSubscriptionCreated and to seed the prior-activation renewal case with a different period than the current settlement.
…subscription_id The 'stale duplicate recovery guard' test backdated every matching period_advanced / stripe_invoice_settlement row without scoping to the test's subscription, mutating shared DB state from earlier tests in the file and potentially corrupting later assertions. seedSubscription already returns the subscription row; destructure it and add eq(kiloclaw_subscription_change_log.subscription_id, subscription.id) to the WHERE clause alongside the existing action / reason predicates.
… period
## What changed
Replaces the per-instance-lifetime idempotency key on `kiloclaw_email_log`
with a per-activation key so that users who cancel and resubscribe on the
same KiloClaw instance actually receive a second subscription-started email.
- `packages/db/src/schema.ts` + migration `0106_noisy_pete_wisdom.sql`:
add `period_start timestamptz NOT NULL DEFAULT 'epoch'` to
`kiloclaw_email_log`; drop `UQ_kiloclaw_email_log_user_instance_type`;
add `UQ_kiloclaw_email_log_user_instance_type_period` on
`(user_id, instance_id, email_type, period_start)` WHERE
`instance_id IS NOT NULL`.
- `apps/web/src/lib/kiloclaw/credit-billing.ts`: the insert-before-send
in `maybeSendKiloClawSubscriptionStartedEmail` now writes `period_start`,
and the delete-on-error branch now scopes by `period_start` so a failed
send only clears its own marker.
- `apps/web/src/lib/purchase-emails.test.ts`: rewrote the unique-index
unit test for the new shape, added a sibling test proving the index
admits a second row for a new period, and added an end-to-end
regression test that activates, cancels, and resubscribes on the same
instance and asserts two sends and two log rows.
## Why this matters
The KiloClaw transactional email work introduced a subscription-started
email gated on `kiloclaw_email_log` as the durable idempotency surface.
The existing unique index on `(user_id, instance_id, email_type)` was
designed for one-per-instance-lifetime emails (e.g. `claw_instance_ready`)
and is wrong for activation-event emails: after the first activation
wrote a row, every future resubscribe on the same instance would conflict
on the index, `onConflictDoNothing` would return `rowCount=0`, and the
function would exit early without sending. Users who cancel and rejoin
— a normal lifecycle — silently lost their activation email.
The adversarial review captured this as cloud-ib7 with the explicit
instruction to pick one of two product semantics and make the code
consistent with the test expectation. We picked "one email per activation
event" because (a) the existing test at purchase-emails.test.ts:440
already asserts a canceled-paid resubscribe should send, and (b) removing
canceled-paid rows from eligibility would deprive returning customers of
a confirmation they reasonably expect.
## Decisions
**Why a `period_start` column and not a `subscription_id` column.**
Resubscribe (both Stripe checkout and credit enrollment) UPDATEs the
existing `kiloclaw_subscriptions` row in place rather than inserting a
new one — Stripe path at `stripe-handlers.ts:870+` (allowed when
`existingRow.status === 'canceled'`), credit path at
`credit-billing.ts:1147` via `onConflictDoUpdate` on `instance_id`. Our
internal `kiloclaw_subscriptions.id` is therefore stable across every
activation on the instance and adds no discriminative power as a dedupe
key. `stripe_subscription_id` is NULL for pure-credit subscriptions
(`enrollWithCredits` explicitly writes `stripe_subscription_id: null`),
so it cannot serve as the key either without special-casing. What
actually differs across activations on both paths is the period
boundary: Stripe stamps fresh `current_period_start` from the invoice
line item; credits stamp `nowIso` on every enrollment. One column
handles both paths.
**Why `NOT NULL DEFAULT 'epoch'` instead of nullable.** Postgres treats
`NULL` as distinct in unique indexes by default, which would let any
other email type that omits `period_start` insert multiple rows and
break the existing one-per-instance-lifetime contract for
`claw_instance_ready`, `claw_suspended_*`, and friends. Drizzle's
`nullsNotDistinct()` is only available on `unique()` constraints, which
do not support partial `WHERE`. Defaulting to `'epoch'` lets every
existing writer keep working unchanged — they all collapse onto the
same `(user, instance, type, 'epoch')` index row — while only the
subscription-started email path opts in to per-activation keying by
explicitly writing `periodStart`.
**Why not a synthetic `dedupe_key text`.** A natural timestamp column
is queryable, self-documenting, and makes admin tooling easier
("show me all activation emails for period X"). A synthetic string key
forces every reader to parse it.
**Why the delete-on-error also got tightened.** The previous delete
cleared every row for `(user, instance, type)`, which was fine when
only one row could exist. With per-activation keying it would be a
foot-gun: a failed send on activation N could erase activation N-1's
durable marker. The new scope is `(user, instance, type, periodStart)`
so a failure only touches its own insert.
## Impact and side effects
**Other writers of `kiloclaw_email_log` are unaffected.** The kiloclaw
billing worker (`services/kiloclaw-billing/src/lifecycle.ts`), the
KiloClaw router instance-destroy cleanup, the admin trial-reset flow,
and the admin instance-reset flow all write and delete rows without
referencing `period_start`. The `DEFAULT 'epoch'` fills in a stable
value so their inserts still collapse one-per-(user, instance, type)
and their deletes (filtered by `email_type IN (...)`) still match every
relevant row regardless of `period_start`.
**Existing production rows get `period_start='epoch'` on backfill.**
For `kiloclaw_subscription_started` rows written before the migration,
this means the first post-deploy activation on the same instance will
write a row with a real `periodStart` and succeed. For renewals that is
correctly suppressed upstream by
`shouldSendSubscriptionStartedEmailForActivation` (before we ever reach
the email helper), so existing active subscribers do not get duplicate
emails. For resubscribes — the exact cohort this fix exists for — a
second email correctly fires.
**Coordinates with adjacent beads.**
- cloud-4lb (enrollWithCredits does not send subscription-started on
credit activation) becomes trivial to land: pass the new period start
to `maybeSendKiloClawSubscriptionStartedEmail` and per-activation
dedupe already works for the credit path.
- cloud-j1o (template copy hard-codes "first billing period") was
previously moot because resubscribes never received the email. It is
now a real product bug and should be addressed.
**Does not touch.** Email rendering or templates, credit accounting,
Stripe webhook parsing, subscription lifecycle state machine, top-up
email flow (cloud-0zq), `softDeleteUser`/GDPR retention (the new
column is a billing-period boundary, not PII; the retention test at
`user.test.ts:1536` still passes).
## Verification
Manually verified:
- `pnpm --filter web typecheck` passes
- `pnpm --filter kiloclaw-billing typecheck` passes
- `pnpm --filter web test -- purchase-emails` — 20/20 pass, including
the new per-period admit-second-row test and the end-to-end
activate→cancel→resubscribe test
- `pnpm --filter web test -- user.test` — 59/59 pass, including the
GDPR retention test
- `pnpm --filter kiloclaw-billing test` — 60/60 pass
- `pnpm format` clean
Closes cloud-ib7.
Per KiloClaw billing spec (Stripe-Funded Credit Settlement rule 10), $0 invoices must still run settlement and transition the row into the activated hybrid state. The subscription-started email is an activation notification, not a revenue side effect, so it must fire regardless of invoice amount. Revenue side effects (analytics, affiliate sale events) apply their own amount_paid > 0 guard in stripe-handlers.ts. Drops the amountMicrodollars > 0 gate on the email so users activated by a full coupon or promo still receive the activation notification. The existing '$0 Stripe settlement' test in purchase-emails.test.ts locks in this behavior.
…der not configured
maybeSendKiloClawSubscriptionStartedEmail inserted the kiloclaw_email_log
marker before calling sendKiloClawSubscriptionStartedEmail and only
deleted it if the send threw. When the provider returned
{sent: false, reason: 'provider_not_configured'} without throwing (e.g.
Mailgun env missing in a preview environment), the marker persisted and
permanently suppressed the email on future webhook retries via the
unique index guard.
Inspect the SendResult and clear the marker on provider_not_configured
so a retry can re-attempt. Mirrors the proven pattern in
services/kiloclaw-billing/src/lifecycle.ts:879-884.
neverbounce_rejected is deliberately left in place: the verdict is
terminal for that address (invalid / disposable), so retrying would
loop forever. Leaving the row keeps the outcome idempotent — we tried
once, the address was rejected, we do not try again.
Refactored the delete branch into deleteSubscriptionStartedEmailLog,
reused by both the non-throwing failure path and the existing catch.
Tests: one asserting the log row is cleared on provider_not_configured
so a retry can re-send, one asserting the row persists on
neverbounce_rejected so we do not retry a terminally invalid address.
Widened sendMock's return type to SendResult so mockImplementationOnce
can return {sent: false, reason: ...}.
…mport in purchase-emails test oxlint's consistent-type-imports rule forbids inline import() type annotations. Convert to a top-level 'import type * as creditBillingModule' at the file header, matching the existing pattern used for emailModule.
| @@ -0,0 +1,3 @@ | |||
| DROP INDEX "UQ_kiloclaw_email_log_user_instance_type";--> statement-breakpoint | |||
| ALTER TABLE "kiloclaw_email_log" ADD COLUMN "period_start" timestamp with time zone DEFAULT 'epoch' NOT NULL;--> statement-breakpoint | |||
| CREATE UNIQUE INDEX "UQ_kiloclaw_email_log_user_instance_type_period" ON "kiloclaw_email_log" USING btree ("user_id","instance_id","email_type","period_start") WHERE "kiloclaw_email_log"."instance_id" is not null; No newline at end of file | |||
Contributor
Author
There was a problem hiding this comment.
WARNING: Rebuilds the unique index with a blocking migration path
This drops the old instance-scoped unique index before creating the replacement, and the replacement CREATE UNIQUE INDEX is not CONCURRENTLY. On a populated kiloclaw_email_log table this can block writes while the index scans, and there is also a window with no instance-scoped dedupe enforced for email-log inserts. Prefer a phased online migration: add the column, create the new unique index concurrently while the old one still exists, then drop the old index concurrently (or explicitly schedule downtime for the lock window).
Refactors purchase-emails.test.ts to mock sendViaMailgun and verifyEmail so every test exercises the real sendCreditsTopUpEmail and sendKiloClawSubscriptionStartedEmail code paths — including formatUsd rounding, formatDate formatting, subjectOverride selection, and credits_url / manage_url / receipt_section construction. Previously the helpers themselves were mocked with synthetic implementations, so a rename like receipt_url → receipt_section would ship green. Adds direct payload tests for both helpers covering the happy path, neverbounce rejection, provider_not_configured, null receipt URL, and the zero-cent price case.
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
2 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
Adds two exactly-once transactional emails covering the remaining purchase
events that currently send no confirmation:
processTopUpsuccessfullycompletes, covering both manual Stripe Checkout top-ups and
auto-top-ups. The email copy switches between a manual and an auto
variant via a
CreditsTopUpVariantflag.period only, hooked into
applyStripeFundedKiloClawPeriodandskipped on every subsequent renewal.
Hook points
apps/web/src/lib/credits.ts— email send added to theprocessPostTopUpFreeStuffblock insideprocessTopUp. That block isnaturally gated by the unique constraint on
credit_transactions.stripe_payment_id, so Stripe webhook replaysreturn early (
false) before reaching the send and duplicate emailsare impossible. Paths that pass
skipPostTopUpFreeStuff: true(Kilo Pass issuance, KiloClaw credit settlement) intentionally do NOT
trigger the top-up email, since they are not user-initiated top-ups.
apps/web/src/lib/kiloclaw/credit-billing.ts— email send added toapplyStripeFundedKiloClawPeriodafter the ledger transactioncommits. First-paid-period detection uses two layers:
before.status === 'trialing'(the subscriptionrow before this settlement) guards against sending to existing paid
subscribers who have no
kiloclaw_email_logmarker yet — renewalshave
before.status === 'active'and are skipped entirely.kiloclaw_email_logtable,keyed by
(user_id, instance_id, email_type='kiloclaw_subscription_started')protected by
UQ_kiloclaw_email_log_user_instance_type, handlesretry idempotency: if a webhook replays after the state check passes
but before the email is delivered, the duplicate insert is a no-op.
On send failure the marker is rolled back so the next retry can
re-send — same pattern used by
clawInstanceReady.Idempotency approach
credit_transactions.stripe_payment_id. The inserting webhook is theonly one that enters the post-processing block, so the email is sent
at most once per Stripe charge/invoice/payment-intent id. No new
table needed.
before.status === 'trialing'(only the trial→paid transition qualifies). The
kiloclaw_email_loginsert then ensures Stripe webhook retries within the same first period
are also deduplicated.
Templates added
apps/web/src/emails/creditsTopUp.html— variant-aware heading &intro, amount / credits / date detail box, Stripe receipt link when
available, CTA to
/credits.apps/web/src/emails/kiloClawSubscriptionStarted.html— plan name,price, billing period, next billing date, CTA to
/claw.Both templates follow the Customer.io-aligned style defined in
apps/web/src/emails/AGENTS.md(520px, Mailgun-rendered, light-mode,standard branding footer). AGENTS.md template table is updated with
the new variables.
New env vars
None. Uses the existing
@/lib/emailMailgun wrapper and existingNEXTAUTH_URLfor CTAs.Verification
renderTemplate('creditsTopUp', …)andrenderTemplate('kiloClawSubscriptionStarted', …)via unit tests andconfirmed required fields interpolate.
processTopUpmanually with the samestripe_payment_idtwice and confirmed the second call returns
falsebefore the emailsend helper runs.
kiloclaw_email_logunique index with duplicateinserts and confirmed the second insert reports
rowCount === 0.Visual Changes
Reviewer Notes
top-up (charge / invoice / payment-intent depending on id prefix).
It's wrapped in try/catch and skipped entirely under
IS_IN_AUTOMATED_TESTso Jest suites that construct fakech_*/pi_*ids don't hit the live API.applyStripeFundedKiloClawPeriodafter the DB transaction commitsbut before the
logInfo, so any email failure is captured viaSentry without rolling back the paid period.
skipPostTopUpFreeStuff: truecallers intentionally suppress thecredit top-up email. If a future caller uses
processTopUpfor auser-initiated flow and passes that flag, they will need to send
their own confirmation.
Built for Job Rietbergen by Kilo for Slack