Add Stripe credit plans, tenant billing, and /admin. - #230
Conversation
![mergestorm-vortex[bot]](https://avatars.githubusercontent.com/in/3924858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review progress ██████████ 35/35 files
Actionable comments posted: 13
🧠 Thinking traces
Traced the new billing/credit flow end-to-end: meter queries vs the usage ledger schema, webhook watermarking, checkout/portal guards, admin allowlist, and which X post-read entry points are actually gated. The core Stripe plumbing is sound (signature-verified webhook, watermark ordering, migration runner tracks applied files), but credit enforcement only covers Scout/search — one user-triggerable X read endpoint bypasses the paywall entirely, and the webhook silently keeps the old plan when a subscription price isn't in env.
Verified the new billing/admin frontend against the server: endpoint contracts (billing/me, stripe checkout/portal/confirm, admin/tenants), auth /me isAdmin, scout 402 surfacing, and webhook/confirm watermark idempotency all line up. Found one billing-flow dead-end for non-active subscription states in BillingPanel.
Reviewed the four new test files against billingStore.ts, stripeConfig.ts, stripeGuards.ts, plans.ts, db.ts, usageMeter.ts, index.ts and the webhook dispatch in stripeHttp.ts. The tests are accurate: watermark semantics (shouldApplyStripeEvent(stored, event)), admin-horizon override, free-pool exhaustion, guard behaviors, and dev/prod price resolution all match the implementation, and the test-DB/ADMIN_EMAILS isolation is sound. One silent-fallback gap in the webhook flow that the stripeConfig test pins is worth noting.
Reviewed the /admin pass (adminHttp.ts, adminEmails.ts, adminEmails.test.ts, AdminPanel.tsx) and traced cross-file flows: auth-gate ordering in index.ts, fail-closed ADMIN_EMAILS semantics (and that user emails are only set from Google-verified OAuth), AdminTenantUsage vs AdminTenantRow shape, and UTC-month usage counting consistency with the 402 gate. No defects found; the pass is sound.
Reviewed the billing/tenant pass: credit gate placement in index.ts, request-context tenant attribution, webhook watermark/activation logic, admin allowlist, and migration runner. Found two enforcement gaps in the credit paywall (an ungated paid-read endpoint, and entry-only ceiling enforcement) that contradict the stated "hard monthly ceiling, no overage" guarantee.
Verified the 402/credits_exhausted soft-gate wiring (client scoutStages + server sendCreditsExhausted), the public webhook path vs authGuard source, env docs, and the new stripe dependency. One credit-enforcement gap found: X post-read entry points other than /api/search and /api/scout/run are not gated.
This pass adds only three plan-card PNG images (public/images/plan-*.png) with no code changes to review. No defects found.
🤖 Prompt for AI agents · Core + specialists
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate. Do not add new
features, refactors, or architecture beyond the fix; prefer the smallest diff.
Findings to address (aggregated from Vortex Core and specialist fleet lanes):
1. In `@server/src/billingStore.ts` (line 272, Important):
This UPDATE sets plan_key/status/period/watermark but not `stripe_subscription_id` (or `stripe_customer_id`). The only place the sub id is stored is `activateSubscription` (checkout.session.completed webhook / checkout/confirm). If `customer.subscription.updated` is processed before `checkout.session.completed` (Stripe retries make delivery order non-deterministic) — or if both the completed event and the browser's checkout/confirm are lost (sidecar down past Stripe's retry window) — the row ends with plan_key='pulse', status='active', stripe_subscription_id=NULL. From then on `getUserBillingBySubscriptionId(sub.id)` misses, so `customer.subscription.deleted` → `cancelSubscriptionByStripeSubscriptionId` is a no-op: the DB keeps a phantom paid plan forever with no reconciliation path, and a user who actually paid stays stuck at the free pool because `effectivePlanKey` requires a live sub id. Persist the subscription id (and customer id) from the `Stripe.Subscription` object in this update.
2. In `@server/src/billingStore.ts` (line 292, Important):
`const planKey = input.planKey ?? row.planKey;` — when `customer.subscription.updated` arrives for a price not present in STRIPE_PRICE_{PULSE,RADAR,HORIZON} (e.g. a mergestorm.ai price from the shared account, since the portal config is optional), `planKeyFromStripePriceId` returns null and the old `plan_key` is kept while `stripe_last_event_created` still advances. A downgrade to an unmapped price keeps granting the pricier plan's credits; an upgrade never takes effect; and because the watermark moved, a later Stripe redelivery after env is fixed is rejected as stale. Log/error loudly (or skip advancing the watermark) when the price can't be mapped.
3. In `@server/src/billingStore.ts` (line 339, Important):
`countPostsReadThisUtcMonth` (and `creditsExhaustedResponse`) counts every `/tweets` post read for the tenant, but `sendCreditsExhausted` is only invoked in the `/api/search` (index.ts:437) and `/api/scout/run` (index.ts:508) routes. `POST /api/interacted/detect` (index.ts:835) → `detectOwnReplyToThreadWithRetry` → `searchTimelinePages` → `xApiGet` runs in the request's tenant context (enterRequestContext at index.ts:292), so each call records up to 20 posts_read per search (3 attempts with the retry variant) into this meter — yet the route never returns the 402. A free-tier user who exhausts the 250/month allotment can keep polling detect indefinitely to read posts beyond the hard ceiling (real X API per-read cost to the operator), and the Usage page will show used > limit while reads keep flowing. Gate detect (or exclude it from the metered pool) so the ceiling is actually enforced.
4. In `@server/src/index.ts` (line 437, Important):
`if (sendCreditsExhausted(req, res)) return;`
The 402 gate is only wired into `/api/search` and `/api/scout/run`. `POST /api/interacted/detect` (line ~835) calls `detectOwnReplyToThreadWithRetry` → `searchTimelinePages` → `xApiGet('/2/tweets/search/recent')`, which is a metered, paid post-read: `countPostsRead` counts these hits and `recordUsageEvent` attributes them to the request tenant (path contains `/tweets`). A user whose 250-credit free pool (or any plan pool) is exhausted can call `/api/interacted/detect` with an arbitrary `threadId` and keep consuming the shared paid X bearer — up to 3 searches × 20 posts per request with the retry path — and the meter only records, never blocks. This bypasses the hard-ceiling/no-overage guarantee the billing feature exists to enforce. The same applies to any other user-triggerable X-read handler; suggest gating this endpoint (or all metered read paths) with the same check.
5. In `@server/src/index.ts` (line 437, Important):
`sendCreditsExhausted` gates only `/api/search` and the Scout path, but `POST /api/interacted/detect` (index.ts:835) reaches `detectOwnReplyToThreadWithRetry` → `searchTimelinePages` → `searchTimeline` → `xApiGet({ path: '/tweets/search/recent' })` without any credit check. These are paid X post reads metered to the session tenant (up to 3 searches × 20 posts per call, no cooldown gate, callable any number of times). A tenant whose monthly pool (250/1500/6000/20000) is exhausted can keep generating reads at the operator's X wallet cost, defeating the 'hard ceiling, no overage' guarantee of this PR. Add the `sendCreditsExhausted` gate to the detect handler (and any other user-facing path that reaches `xApiGet` post reads).
6. In `@server/src/index.ts` (line 437, Important):
`sendCreditsExhausted` is only wired into `/api/search` and `/api/scout/run`. `POST /api/interacted/detect` (index.ts:835) calls `detectOwnReplyToThread(WithRetry)` → `searchTimelinePages` → `searchTimeline` → `xApiGet('/tweets/search/recent')`, and every call records postsRead into the tenant ledger via `recordUsageEvent` — but there is no 402 gate on this handler.
Failing sequence: a tenant exhausts their 250-credit free pool, then clicks "Mark interacted" — the UI's `runMarkDetect` poll loop (`src/App.tsx`) keeps POSTing `/api/interacted/detect` (with server-side 3-attempt retry when `once` is omitted), each attempt consuming up to 20 X API post reads. Scout/search now return 402, but this path keeps spending the shared Pay-Per-Use balance at Mergestorm's cost past the ceiling, and an authed user can repeat it with arbitrary `conversationId`s. Reads are counted in the ledger but never blocked, so the advertised hard ceiling is bypassable. Add the same `sendCreditsExhausted` check to the `/api/interacted/detect` handler (and consider gating the client poll on `credits.can_use`).
7. In `@server/src/index.ts` (line 437, Important):
`sendCreditsExhausted` is only wired into `POST /api/search` (here) and `POST /api/scout/run`. `POST /api/interacted/detect` (index.ts:835) calls `detectOwnReplyToThread` / `detectOwnReplyToThreadWithRetry`, which run `/2/tweets/search/recent` searches (post reads) with retry/backoff — those reads are metered to the tenant through the request-context fallback in `recordUsageEvent` (xApi.ts:76), so they consume the credit pool, but the endpoint never checks `creditsExhaustedResponse`. An exhausted free tenant can keep calling detect (`once:false` fires up to N searches per call) and keep spending the platform's Pay-Per-Use X API budget past the advertised 'hard monthly ceiling, no overage'. Add the `sendCreditsExhausted` gate to detect as well, and audit for any other user-reachable `xApiGet` paths (e.g. tweetLookup hydration reachable outside scout).
8. In `@server/src/index.ts` (line 508, Important):
`if (sendCreditsExhausted(req, res)) return;`
The gate is checked once before `tryBeginScout()`, but `runScoutCollect`'s refill loop keeps searching/cycling until the cool target is hit or supply is exhausted, with each `searchTimelinePages` call recording ~20 posts. A user with, say, 5 credits left in a 250-credit free pool can start a run that reads 100–400+ posts before it ends, exceeding the 'hard monthly ceiling, no overage' contract. Since reads are metered per-call inside the run, either enforce the limit at each metered read (stop the run when the pool empties) or document that the ceiling applies per run-start only.
9. In `@server/src/index.ts` (line 508, Important):
`sendCreditsExhausted` is evaluated before `tryBeginScout`, but `recordUsageEvent` accrues continuously during the run (scoutCollect's refill loop keeps searching/paging until the cool target is hit or supply is exhausted). A free tenant at 249/250 used — or any run that crosses the limit mid-flight — can complete a run that reads far more than the remaining credits. The stated 'hard monthly ceiling, no overage' is therefore exceeded by up to one full run per month per tenant, with the overage billed to the operator's shared X wallet. Re-check credits (or enforce remaining-budget) inside the refill loop rather than only at run start.
10. In `@server/src/stripeConfig.test.ts` (line 40, Important):
`planKeyFromStripePriceId("price_other")` returning null is fine for this function, but trace what the webhook does with that null: in `dispatchWebhook` (stripeHttp.ts:434-444), `customer.subscription.updated` passes `planKey: null` into `updateSubscriptionFromStripe`, which falls back to `input.planKey ?? row.planKey` (billingStore.ts:292) — the user's plan stays unchanged while `subscription_status`/`current_period_end` AND `stripe_last_event_created` are still updated. Because the watermark advances, a later, correctly-recognized event for the same subscription (same or older `event.created`) is then dropped by `shouldApplyStripeEvent`, so the mismatch never self-heals even after the env is fixed. Trigger: a price ID on the Stripe subscription that isn't in `STRIPE_PRICE_*` env (one plan missing from the prod .env, or a price swapped in the Stripe dashboard/Portal). Result is a paying user billed for one tier while credited for another, silently. Consider logging/alerting loudly on the unknown-price path instead of silently keeping the old plan.
11. In `@src/BillingPanel.tsx` (line 63, Important):
`const usePortal = live && (billing?.plan_state === "subscription_active" || paymentFailed);`
The server's `hasLiveStripeSubscription` treats any non-`canceled`/`incomplete_expired` status as live, so a subscription stuck in `incomplete` (first invoice unpaid, e.g. async payment method or failed card retry) or `paused` reports `has_stripe_subscription: true` with `plan_state: "free"` and `subscription_status` neither `past_due` nor `unpaid`. In that state `usePortal` is false and `paymentFailed` is false, so the UI shows the Subscribe buttons but no "Manage billing"/"Update payment" control. Clicking Subscribe hits `checkoutBlockedByExistingSubscription` (stripeGuards.ts: status not in NON_LIVE, `stripe_subscription_id` set) → 409 `subscription_exists`, leaving the user with no in-app way to complete payment or switch plans (for `incomplete`, Stripe only auto-resolves to `incomplete_expired` after ~24h). Consider treating `incomplete`/`paused` like `past_due` in `usePortal` (or having the server surface them distinctly) so the portal/retry path is always reachable when a live subscription exists.
12. In `@src/lib/scoutStages.ts` (line 48, Important):
`status === 402` / `credits_exhausted` soft-gating matches `sendCreditsExhausted` in `server/src/index.ts`, but that gate is only wired into `/api/search` and `/api/scout/run`. `POST /api/interacted/detect` (index.ts:835) runs `detectOwnReplyToThreadWithRetry` → `searchTimelinePages` → X `/2/tweets/search/recent` (up to 3 retry attempts × 20 tweets each), and that path matches `path LIKE '%/tweets%'` in `countPostsReadThisUtcMonth`/`listAdminTenantUsage`, so those reads are counted against the tenant's credit pool but never blocked. A user whose 250/1500/… credits are exhausted can keep calling detect (the UI auto-fires it via mark-detect polling after each Scout) and burn X Pay-Per-Use reads past the promised "hard ceiling, no overage". Consider gating detect (and any other user-initiated X-read endpoint) with the same `sendCreditsExhausted` check, or excluding it from the credit ledger.
13. In `@server/src/stripeHttp.ts` (line 434, Suggestion):
`planKey = planKeyFromStripePriceId(priceId)` returns null when the subscription's price ID is not in STRIPE_PRICE_{PULSE,RADAR,HORIZON} (or _DEV), and `updateSubscriptionFromStripe` then does `input.planKey ?? row.planKey` (billingStore.ts:292) — keeping the previous plan_key. If a user changes plans in the Stripe portal to any price not listed in env (legacy/retired price, test-mode mix, or a price added to Stripe but not yet to .env), a downgrade keeps the higher credit limit for the rest of the UTC month and an upgrade silently never takes effect while the user is charged the new amount. Consider failing closed (treat unknown price as a webhook error → 500 so Stripe retries) or at least logging loudly instead of preserving the stale plan.
Vortex specialist fleet
Security — completed · 3 findings
📋 Security findings (3)
Credit paywall bypassed via /api/interacted/detect
server/src/index.ts:437
`sendCreditsExhausted` gates only `/api/search` and the Scout path, but `POST /api/interacted/detect` (index.ts:835) reaches `detectOwnReplyToThreadWithRetry` → `searchTimelinePages` → `searchTimeline` → `xApiGet({ path: '/tweets/search/recent' })` without any credit check. These are paid X post reads metered to the session tenant (up to 3 searches × 20 posts per call, no cooldown gate, callable any number of times). A tenant whose monthly pool (250/1500/6000/20000) is exhausted can keep generating reads at the operator's X wallet cost, defeating the 'hard ceiling, no overage' guarantee of this PR. Add the `sendCreditsExhausted` gate to the detect handler (and any other user-facing path that reaches `xApiGet` post reads).
Unmapped Stripe price silently keeps old plan but advances the watermark
server/src/billingStore.ts:292
`const planKey = input.planKey ?? row.planKey;` — when `customer.subscription.updated` arrives for a price not present in STRIPE_PRICE_{PULSE,RADAR,HORIZON} (e.g. a mergestorm.ai price from the shared account, since the portal config is optional), `planKeyFromStripePriceId` returns null and the old `plan_key` is kept while `stripe_last_event_created` still advances. A downgrade to an unmapped price keeps granting the pricier plan's credits; an upgrade never takes effect; and because the watermark moved, a later Stripe redelivery after env is fixed is rejected as stale. Log/error loudly (or skip advancing the watermark) when the price can't be mapped.
Credit check runs once before the run, so a single run can exceed the ceiling
server/src/index.ts:508
`sendCreditsExhausted` is evaluated before `tryBeginScout`, but `recordUsageEvent` accrues continuously during the run (scoutCollect's refill loop keeps searching/paging until the cool target is hit or supply is exhausted). A free tenant at 249/250 used — or any run that crosses the limit mid-flight — can complete a run that reads far more than the remaining credits. The stated 'hard monthly ceiling, no overage' is therefore exceeded by up to one full run per month per tenant, with the overage billed to the operator's shared X wallet. Re-check credits (or enforce remaining-budget) inside the refill loop rather than only at run start.
🧠 Security thinking traces
Security review of Stripe billing/tenant/admin PR. Webhook signature verification, admin allowlist (fail-closed), checkout-session ownership checks, and tenant-context usage attribution all check out. Found three concrete billing-enforcement/billing-correctness gaps: an un-gated paid post-read endpoint, and two ceiling/plan-accounting mismatches.
Performance — completed · 1 finding
📋 Performance findings (1)
Credit gate misses /api/interacted/detect (and the client's mark-detect polling)
server/src/index.ts:437
`sendCreditsExhausted` is only wired into `/api/search` and `/api/scout/run`. `POST /api/interacted/detect` (index.ts:835) calls `detectOwnReplyToThread(WithRetry)` → `searchTimelinePages` → `searchTimeline` → `xApiGet('/tweets/search/recent')`, and every call records postsRead into the tenant ledger via `recordUsageEvent` — but there is no 402 gate on this handler.
Failing sequence: a tenant exhausts their 250-credit free pool, then clicks "Mark interacted" — the UI's `runMarkDetect` poll loop (`src/App.tsx`) keeps POSTing `/api/interacted/detect` (with server-side 3-attempt retry when `once` is omitted), each attempt consuming up to 20 X API post reads. Scout/search now return 402, but this path keeps spending the shared Pay-Per-Use balance at Mergestorm's cost past the ceiling, and an authed user can repeat it with arbitrary `conversationId`s. Reads are counted in the ledger but never blocked, so the advertised hard ceiling is bypassable. Add the same `sendCreditsExhausted` check to the `/api/interacted/detect` handler (and consider gating the client poll on `credits.can_use`).
🧠 Performance thinking traces
Traced the new credit-enforcement gate across all X post-read entry points and the tenant/usage plumbing. The scout/search paths are correctly gated, but one read path — POST /api/interacted/detect — performs X recent-search reads that are metered into the ledger yet never blocked by the credit ceiling, so the "hard monthly ceiling, no overage" guarantee doesn't hold and the shared Pay-Per-Use balance keeps draining after a tenant's pool is empty.
Architecture — completed · 2 findings
📋 Architecture findings (2)
Credit gate only covers scout/search — /api/interacted/detect is an ungated X post-read path
server/src/index.ts:437
`sendCreditsExhausted` is only wired into `POST /api/search` (here) and `POST /api/scout/run`. `POST /api/interacted/detect` (index.ts:835) calls `detectOwnReplyToThread` / `detectOwnReplyToThreadWithRetry`, which run `/2/tweets/search/recent` searches (post reads) with retry/backoff — those reads are metered to the tenant through the request-context fallback in `recordUsageEvent` (xApi.ts:76), so they consume the credit pool, but the endpoint never checks `creditsExhaustedResponse`. An exhausted free tenant can keep calling detect (`once:false` fires up to N searches per call) and keep spending the platform's Pay-Per-Use X API budget past the advertised 'hard monthly ceiling, no overage'. Add the `sendCreditsExhausted` gate to detect as well, and audit for any other user-reachable `xApiGet` paths (e.g. tweetLookup hydration reachable outside scout).
updateSubscriptionFromStripe never persists stripe_subscription_id — deletion events become no-ops
server/src/billingStore.ts:272
This UPDATE sets plan_key/status/period/watermark but not `stripe_subscription_id` (or `stripe_customer_id`). The only place the sub id is stored is `activateSubscription` (checkout.session.completed webhook / checkout/confirm). If `customer.subscription.updated` is processed before `checkout.session.completed` (Stripe retries make delivery order non-deterministic) — or if both the completed event and the browser's checkout/confirm are lost (sidecar down past Stripe's retry window) — the row ends with plan_key='pulse', status='active', stripe_subscription_id=NULL. From then on `getUserBillingBySubscriptionId(sub.id)` misses, so `customer.subscription.deleted` → `cancelSubscriptionByStripeSubscriptionId` is a no-op: the DB keeps a phantom paid plan forever with no reconciliation path, and a user who actually paid stays stuck at the free pool because `effectivePlanKey` requires a live sub id. Persist the subscription id (and customer id) from the `Stripe.Subscription` object in this update.
🧠 Architecture thinking traces
Reviewed Stripe billing/tenant/admin flow end-to-end: webhook watermarking, checkout/portal guards, credit metering (usageMeter/requestContext), and the 402 gate placement in index.ts. Found two issues: the credit paywall only covers scout/search while /api/interacted/detect is an ungated X post-read surface, and updateSubscriptionFromStripe never persists the Stripe subscription id, leaving the billing row unable to reconcile deletions.
| ); | ||
| } | ||
|
|
||
| export function updateSubscriptionFromStripe(input: { |
There was a problem hiding this comment.
Architecture specialist
updateSubscriptionFromStripe never persists stripe_subscription_id — deletion events become no-ops
This UPDATE sets plan_key/status/period/watermark but not stripe_subscription_id (or stripe_customer_id). The only place the sub id is stored is activateSubscription (checkout.session.completed webhook / checkout/confirm). If customer.subscription.updated is processed before checkout.session.completed (Stripe retries make delivery order non-deterministic) — or if both the completed event and the browser's checkout/confirm are lost (sidecar down past Stripe's retry window) — the row ends with plan_key='pulse', status='active', stripe_subscription_id=NULL. From then on getUserBillingBySubscriptionId(sub.id) misses, so customer.subscription.deleted → cancelSubscriptionByStripeSubscriptionId is a no-op: the DB keeps a phantom paid plan forever with no reconciliation path, and a user who actually paid stays stuck at the free pool because effectivePlanKey requires a live sub id. Persist the subscription id (and customer id) from the Stripe.Subscription object in this update.
| row.stripeLastEventCreated, | ||
| input.stripeEventCreated, | ||
| ); | ||
| const planKey = input.planKey ?? row.planKey; |
There was a problem hiding this comment.
Security specialist
Unmapped Stripe price silently keeps old plan but advances the watermark
const planKey = input.planKey ?? row.planKey; — when customer.subscription.updated arrives for a price not present in STRIPE_PRICE_{PULSE,RADAR,HORIZON} (e.g. a mergestorm.ai price from the shared account, since the portal config is optional), planKeyFromStripePriceId returns null and the old plan_key is kept while stripe_last_event_created still advances. A downgrade to an unmapped price keeps granting the pricier plan's credits; an upgrade never takes effect; and because the watermark moved, a later Stripe redelivery after env is fixed is rejected as stale. Log/error loudly (or skip advancing the watermark) when the price can't be mapped.
| .run(watermark, nowIso(), row.userId); | ||
| } | ||
|
|
||
| export function countPostsReadThisUtcMonth(tenantId: string): number { |
There was a problem hiding this comment.
Credit ceiling only enforced on /api/search and /api/scout/run — /api/interacted/detect bypasses it
countPostsReadThisUtcMonth (and creditsExhaustedResponse) counts every /tweets post read for the tenant, but sendCreditsExhausted is only invoked in the /api/search (index.ts:437) and /api/scout/run (index.ts:508) routes. POST /api/interacted/detect (index.ts:835) → detectOwnReplyToThreadWithRetry → searchTimelinePages → xApiGet runs in the request's tenant context (enterRequestContext at index.ts:292), so each call records up to 20 posts_read per search (3 attempts with the retry variant) into this meter — yet the route never returns the 402. A free-tier user who exhausts the 250/month allotment can keep polling detect indefinitely to read posts beyond the hard ceiling (real X API per-read cost to the operator), and the Usage page will show used > limit while reads keep flowing. Gate detect (or exclude it from the metered pool) so the ceiling is actually enforced.
| ? body.queries.filter((q): q is string => typeof q === "string") | ||
| : []; | ||
| const filters = parseScoutFilters(body.filters); | ||
| if (sendCreditsExhausted(req, res)) return; |
There was a problem hiding this comment.
Credit gate misses /api/interacted/detect — paid X reads keep flowing after the pool is empty
if (sendCreditsExhausted(req, res)) return;
The 402 gate is only wired into /api/search and /api/scout/run. POST /api/interacted/detect (line ~835) calls detectOwnReplyToThreadWithRetry → searchTimelinePages → xApiGet('/2/tweets/search/recent'), which is a metered, paid post-read: countPostsRead counts these hits and recordUsageEvent attributes them to the request tenant (path contains /tweets). A user whose 250-credit free pool (or any plan pool) is exhausted can call /api/interacted/detect with an arbitrary threadId and keep consuming the shared paid X bearer — up to 3 searches × 20 posts per request with the retry path — and the meter only records, never blocks. This bypasses the hard-ceiling/no-overage guarantee the billing feature exists to enforce. The same applies to any other user-triggerable X-read handler; suggest gating this endpoint (or all metered read paths) with the same check.
| ? body.queries.filter((q): q is string => typeof q === "string") | ||
| : []; | ||
| const filters = parseScoutFilters(body.filters); | ||
| if (sendCreditsExhausted(req, res)) return; |
There was a problem hiding this comment.
Security specialist
Credit paywall bypassed via /api/interacted/detect
sendCreditsExhausted gates only /api/search and the Scout path, but POST /api/interacted/detect (index.ts:835) reaches detectOwnReplyToThreadWithRetry → searchTimelinePages → searchTimeline → xApiGet({ path: '/tweets/search/recent' }) without any credit check. These are paid X post reads metered to the session tenant (up to 3 searches × 20 posts per call, no cooldown gate, callable any number of times). A tenant whose monthly pool (250/1500/6000/20000) is exhausted can keep generating reads at the operator's X wallet cost, defeating the 'hard ceiling, no overage' guarantee of this PR. Add the sendCreditsExhausted gate to the detect handler (and any other user-facing path that reaches xApiGet post reads).
| const targetCool = clampTargetCool(body.targetCool); | ||
| const bucketSize = clampBucketSize(body.bucketSize); | ||
|
|
||
| if (sendCreditsExhausted(req, res)) return; |
There was a problem hiding this comment.
Security specialist
Credit check runs once before the run, so a single run can exceed the ceiling
sendCreditsExhausted is evaluated before tryBeginScout, but recordUsageEvent accrues continuously during the run (scoutCollect's refill loop keeps searching/paging until the cool target is hit or supply is exhausted). A free tenant at 249/250 used — or any run that crosses the limit mid-flight — can complete a run that reads far more than the remaining credits. The stated 'hard monthly ceiling, no overage' is therefore exceeded by up to one full run per month per tenant, with the overage billed to the operator's shared X wallet. Re-check credits (or enforce remaining-budget) inside the refill loop rather than only at run start.
| assert.equal(resolveStripePriceId("pulse"), "price_dev"); | ||
| assert.equal(planKeyFromStripePriceId("price_dev"), "pulse"); | ||
| assert.equal(planKeyFromStripePriceId("price_prod"), "pulse"); | ||
| assert.equal(planKeyFromStripePriceId("price_other"), null); |
There was a problem hiding this comment.
Unknown Stripe price silently keeps the old plan and advances the watermark
planKeyFromStripePriceId("price_other") returning null is fine for this function, but trace what the webhook does with that null: in dispatchWebhook (stripeHttp.ts:434-444), customer.subscription.updated passes planKey: null into updateSubscriptionFromStripe, which falls back to input.planKey ?? row.planKey (billingStore.ts:292) — the user's plan stays unchanged while subscription_status/current_period_end AND stripe_last_event_created are still updated. Because the watermark advances, a later, correctly-recognized event for the same subscription (same or older event.created) is then dropped by shouldApplyStripeEvent, so the mismatch never self-heals even after the env is fixed. Trigger: a price ID on the Stripe subscription that isn't in STRIPE_PRICE_* env (one plan missing from the prod .env, or a price swapped in the Stripe dashboard/Portal). Result is a paying user billed for one tier while credited for another, silently. Consider logging/alerting loudly on the unknown-price path instead of silently keeping the old plan.
| live && | ||
| (billing?.subscription_status === "past_due" || | ||
| billing?.subscription_status === "unpaid"); | ||
| const usePortal = live && (billing?.plan_state === "subscription_active" || paymentFailed); |
There was a problem hiding this comment.
Subscriptions in incomplete/paused state leave the user with no way to pay or manage
const usePortal = live && (billing?.plan_state === "subscription_active" || paymentFailed);
The server's hasLiveStripeSubscription treats any non-canceled/incomplete_expired status as live, so a subscription stuck in incomplete (first invoice unpaid, e.g. async payment method or failed card retry) or paused reports has_stripe_subscription: true with plan_state: "free" and subscription_status neither past_due nor unpaid. In that state usePortal is false and paymentFailed is false, so the UI shows the Subscribe buttons but no "Manage billing"/"Update payment" control. Clicking Subscribe hits checkoutBlockedByExistingSubscription (stripeGuards.ts: status not in NON_LIVE, stripe_subscription_id set) → 409 subscription_exists, leaving the user with no in-app way to complete payment or switch plans (for incomplete, Stripe only auto-resolves to incomplete_expired after ~24h). Consider treating incomplete/paused like past_due in usePortal (or having the server surface them distinctly) so the portal/retry path is always reachable when a live subscription exists.
| ): boolean { | ||
| return ( | ||
| status === 429 || | ||
| status === 402 || |
There was a problem hiding this comment.
Credits gate only covers /api/search and /api/scout/run — /api/interacted/detect bypasses the hard ceiling
status === 402 / credits_exhausted soft-gating matches sendCreditsExhausted in server/src/index.ts, but that gate is only wired into /api/search and /api/scout/run. POST /api/interacted/detect (index.ts:835) runs detectOwnReplyToThreadWithRetry → searchTimelinePages → X /2/tweets/search/recent (up to 3 retry attempts × 20 tweets each), and that path matches path LIKE '%/tweets%' in countPostsReadThisUtcMonth/listAdminTenantUsage, so those reads are counted against the tenant's credit pool but never blocked. A user whose 250/1500/… credits are exhausted can keep calling detect (the UI auto-fires it via mark-detect polling after each Scout) and burn X Pay-Per-Use reads past the promised "hard ceiling, no overage". Consider gating detect (and any other user-initiated X-read endpoint) with the same sendCreditsExhausted check, or excluding it from the credit ledger.
| const stored = existing?.stripeLastEventCreated ?? 0; | ||
| if (!shouldApplyStripeEvent(stored, event.created)) break; | ||
| const priceId = priceIdFromSubscription(sub); | ||
| const planKey = planKeyFromStripePriceId(priceId); |
There was a problem hiding this comment.
💡 Suggestion · Suggestion
Unrecognized price ID silently keeps the old plan on subscription changes
planKey = planKeyFromStripePriceId(priceId) returns null when the subscription's price ID is not in STRIPE_PRICE_{PULSE,RADAR,HORIZON} (or _DEV), and updateSubscriptionFromStripe then does input.planKey ?? row.planKey (billingStore.ts:292) — keeping the previous plan_key. If a user changes plans in the Stripe portal to any price not listed in env (legacy/retired price, test-mode mix, or a price added to Stripe but not yet to .env), a downgrade keeps the higher credit limit for the rest of the UTC month and an upgrade silently never takes effect while the user is charged the new amount. Consider failing closed (treat unknown price as a webhook error → 500 so Stripe retries) or at least logging loudly instead of preserving the stale plan.
|
@mergestorm-cyclone cyclone-outcome: fixed sha=c3884be batch=review-4932229441 verify=passed Cyclone applied fixes and pushed to |
There was a problem hiding this comment.
Review progress ██████████ 36/36 files
Actionable comment posted: 1
🧠 Thinking traces
Traced the Stripe billing/credit flow end-to-end: request-context tenant attribution, watermark/event ordering, checkout/portal guards, admin allowlist, and the credit gates. The prior gaps (ungated /api/interacted/detect, updateSubscriptionFromStripe not persisting the sub id, mid-run ceiling overspend, incomplete/paused portal path) are all addressed in this pass. One new client-side handling gap remains for the 402 the detect gate now returns.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate. Do not add new
features, refactors, or architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@server/src/index.ts` (line 888, Important):
`if (sendCreditsExhausted(req, res)) return;`
This gate correctly stops metered X reads when the pool is empty, but the client that consumes this endpoint never surfaces the 402. `runMarkDetect` in `src/App.tsx` (line ~1359) special-cases only 401/503; any other non-ok status — including this new 402 with `error: "credits_exhausted"` — is treated as a soft `search_failed`, so `shouldContinueMarkDetectPoll` keeps the loop polling every 5s until the 30s `MARK_DETECT_TIMEOUT_MS`, then falls back to the misleading "Couldn't find your reply — paste the URL" message. The useful body (`"This month's N credits are used. Upgrade on Usage & Billing…"`) is discarded. An exhausted free tenant clicking Mark interacted gets ~6 wasted polls and no indication the real cause is an empty credit pool. Handle 402/`credits_exhausted` in the poll loop (stop and show the message), or the gate's message never reaches the user.
| "Set X_OPERATOR_USERNAME in .env so Mark detect can resolve your handle.", | ||
| }); | ||
| } | ||
| if (sendCreditsExhausted(req, res)) return; |
There was a problem hiding this comment.
Detect 402 credits_exhausted is dropped by the client poll loop
if (sendCreditsExhausted(req, res)) return;
This gate correctly stops metered X reads when the pool is empty, but the client that consumes this endpoint never surfaces the 402. runMarkDetect in src/App.tsx (line ~1359) special-cases only 401/503; any other non-ok status — including this new 402 with error: "credits_exhausted" — is treated as a soft search_failed, so shouldContinueMarkDetectPoll keeps the loop polling every 5s until the 30s MARK_DETECT_TIMEOUT_MS, then falls back to the misleading "Couldn't find your reply — paste the URL" message. The useful body ("This month's N credits are used. Upgrade on Usage & Billing…") is discarded. An exhausted free tenant clicking Mark interacted gets ~6 wasted polls and no indication the real cause is an empty credit pool. Handle 402/credits_exhausted in the poll loop (stop and show the message), or the gate's message never reaches the user.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate. Do not add new
features, refactors, or architecture beyond the fix; prefer the smallest diff.
In `@server/src/index.ts` (line 888, Important):
`if (sendCreditsExhausted(req, res)) return;`
This gate correctly stops metered X reads when the pool is empty, but the client that consumes this endpoint never surfaces the 402. `runMarkDetect` in `src/App.tsx` (line ~1359) special-cases only 401/503; any other non-ok status — including this new 402 with `error: "credits_exhausted"` — is treated as a soft `search_failed`, so `shouldContinueMarkDetectPoll` keeps the loop polling every 5s until the 30s `MARK_DETECT_TIMEOUT_MS`, then falls back to the misleading "Couldn't find your reply — paste the URL" message. The useful body (`"This month's N credits are used. Upgrade on Usage & Billing…"`) is discarded. An exhausted free tenant clicking Mark interacted gets ~6 wasted polls and no indication the real cause is an empty credit pool. Handle 402/`credits_exhausted` in the poll loop (stop and show the message), or the gate's message never reaches the user.
|
@mergestorm-cyclone cyclone-outcome: fixed sha=194483c batch=review-4932283793 verify=passed Cyclone applied fixes and pushed to |
There was a problem hiding this comment.
Review progress ██████████ 36/36 files
Approve — reviewed files look good at 194483c.
✅ All clear — nothing to fix.
🧠 Thinking traces
Reviewed src/App.tsx billing/admin wiring: 402 credits_exhausted handling now terminates the mark-detect poll and surfaces the message (server gate for /api/interacted/detect is present), checkout/portal redirects reconcile with the server confirm endpoint, and BillingPanel/AdminPanel prop usage matches their components. No remaining defects in the touched path.
Wrap X post-read usage in Pulse/Radar/Horizon pools with Checkout/Portal, fail-closed operator admin, and Mergestorm, Inc. legal. Closes #229. Co-authored-by: Cursor <cursoragent@cursor.com>
A shared Mergestorm, Inc. Stripe account would otherwise list mergestorm.ai plans in this desk's billing portal. Co-authored-by: Cursor <cursoragent@cursor.com>
- Gate /api/interacted/detect with sendCreditsExhausted so paid X post reads cannot bypass the monthly ceiling (multiple reviewer findings) - Re-check the credit pool inside the scout refill loop so a single run stops when credits run out mid-flight - Persist stripe_subscription_id (and customer id) in updateSubscriptionFromStripe so out-of-order/updated webhooks leave a live sub id and deleted events reconcile - Log loudly when a subscription price is not mapped in STRIPE_PRICE_* env instead of silently keeping the old plan - Expose the portal/manage path in BillingPanel for incomplete/paused subscriptions instead of a Subscribe button that 409s Cyclone-Source: mergestorm-vortex[bot] Cyclone-Batch: review-4932229441
Cyclone-Source: mergestorm-vortex[bot] Cyclone-Batch: review-4932283793
194483c to
3d6d7c0
Compare
There was a problem hiding this comment.
Review progress ███░░░░░░░ 12/36 files
Actionable comment posted: 1
🧠 Thinking traces
Traced the new billing/admin wiring end-to-end: credit gates on search/scout/detect, the in-loop scout creditGate, Stripe webhook watermarking and sub-id persistence, admin authz, and migration/query column alignment all check out. One remaining issue: a mid-run credit stop is reported to the user as a generic "supply exhausted" rather than a credits-exhausted signal.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate. Do not add new
features, refactors, or architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@server/src/index.ts` (line 557, Important):
`creditGate: async () => creditsExhaustedResponse(...) === null` — when the pool empties mid-run, `runScoutCollect` breaks the search loop at scoutCollect.ts:471-473, but nothing records that credits were the cause: `stopReason` falls through to `"exhausted"` (scoutCollect.ts:791-795) and the terminal `done` event says "Scout finished — N cool thread(s) (supply exhausted)". A tenant who hits their monthly ceiling mid-flight therefore sees a run that just ends with fewer results and no actionable "credits used — upgrade on Usage & Billing" message (unlike the run-start 402 at index.ts:509, which does surface it). Suggest emitting a credits_exhausted stop reason/event from the creditGate path so the UI and scout log reflect the real cause.
| deps: { | ||
| // Re-check the ceiling as reads accrue so a run cannot overspend | ||
| // the remaining monthly pool once it empties mid-flight. | ||
| creditGate: async () => |
There was a problem hiding this comment.
Mid-run credit stop is reported as "supply exhausted", not credits exhausted
creditGate: async () => creditsExhaustedResponse(...) === null — when the pool empties mid-run, runScoutCollect breaks the search loop at scoutCollect.ts:471-473, but nothing records that credits were the cause: stopReason falls through to "exhausted" (scoutCollect.ts:791-795) and the terminal done event says "Scout finished — N cool thread(s) (supply exhausted)". A tenant who hits their monthly ceiling mid-flight therefore sees a run that just ends with fewer results and no actionable "credits used — upgrade on Usage & Billing" message (unlike the run-start 402 at index.ts:509, which does surface it). Suggest emitting a credits_exhausted stop reason/event from the creditGate path so the UI and scout log reflect the real cause.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate. Do not add new
features, refactors, or architecture beyond the fix; prefer the smallest diff.
In `@server/src/index.ts` (line 557, Important):
`creditGate: async () => creditsExhaustedResponse(...) === null` — when the pool empties mid-run, `runScoutCollect` breaks the search loop at scoutCollect.ts:471-473, but nothing records that credits were the cause: `stopReason` falls through to `"exhausted"` (scoutCollect.ts:791-795) and the terminal `done` event says "Scout finished — N cool thread(s) (supply exhausted)". A tenant who hits their monthly ceiling mid-flight therefore sees a run that just ends with fewer results and no actionable "credits used — upgrade on Usage & Billing" message (unlike the run-start 402 at index.ts:509, which does surface it). Suggest emitting a credits_exhausted stop reason/event from the creditGate path so the UI and scout log reflect the real cause.
Cyclone-Source: mergestorm-vortex[bot] Cyclone-Batch: review-4934192661
|
@mergestorm-cyclone cyclone-outcome: fixed sha=5d49dc0 batch=review-4934192661 verify=passed Cyclone applied fixes and pushed to |
There was a problem hiding this comment.
Review progress █████░░░░░ 17/37 files
Actionable comment posted: 1
🧠 Thinking traces
Traced the credit-gate flow (scoutCollect creditGate/stopReason, 402 pre-checks on search/scout/detect, new client 402 handling), the new stripe config/guard tests, and request-context tenant attribution. The mid-run credits_exhausted stop and the client-side 402 surfacing are correctly wired; one cross-request ALS context leak remains in requestContext.ts.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate. Do not add new
features, refactors, or architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@server/src/requestContext.ts` (line 16, Important):
`als.enterWith(ctx)` mutates the current async context's store instead of scoping a new one, and Node's http server does not create a per-request async context — every request on one keep-alive connection runs in the socket's shared context. Two failure modes follow: (1) the store set by one request persists into later requests on the same connection, and (2) a concurrent request's `enterRequestContext` overwrites the store while the first request's handler is still awaiting. Concrete sequence: tenant A starts a long `/api/scout/run` (searches spread over minutes); tenant B's request is multiplexed onto the same pooled origin connection (prod sits behind Cloudflare per wrangler.toml/PUBLIC_DEPLOY.md) and calls `enterRequestContext({tenantId: B})` at index.ts:293 — from then on, tenant A's `xApiGet` → `recordUsageEvent` (xApi.ts:76 passes no tenantId and relies on `getRequestTenantId()`, usageMeter.ts:108) writes A's X reads into B's ledger: A gets free reads and B's credit pool drains until B is 402'd. Since this file already exports `runWithRequestContext`, wrap the request handling in `als.run(ctx, ...)` (per-request scope that restores afterward), or pass `tenantId` explicitly into `recordUsageEvent`.
| const als = new AsyncLocalStorage<RequestContext>(); | ||
|
|
||
| export function enterRequestContext(ctx: RequestContext): void { | ||
| als.enterWith(ctx); |
There was a problem hiding this comment.
enterWith leaks/clobbers the tenant store across requests on a shared connection
als.enterWith(ctx) mutates the current async context's store instead of scoping a new one, and Node's http server does not create a per-request async context — every request on one keep-alive connection runs in the socket's shared context. Two failure modes follow: (1) the store set by one request persists into later requests on the same connection, and (2) a concurrent request's enterRequestContext overwrites the store while the first request's handler is still awaiting. Concrete sequence: tenant A starts a long /api/scout/run (searches spread over minutes); tenant B's request is multiplexed onto the same pooled origin connection (prod sits behind Cloudflare per wrangler.toml/PUBLIC_DEPLOY.md) and calls enterRequestContext({tenantId: B}) at index.ts:293 — from then on, tenant A's xApiGet → recordUsageEvent (xApi.ts:76 passes no tenantId and relies on getRequestTenantId(), usageMeter.ts:108) writes A's X reads into B's ledger: A gets free reads and B's credit pool drains until B is 402'd. Since this file already exports runWithRequestContext, wrap the request handling in als.run(ctx, ...) (per-request scope that restores afterward), or pass tenantId explicitly into recordUsageEvent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate. Do not add new
features, refactors, or architecture beyond the fix; prefer the smallest diff.
In `@server/src/requestContext.ts` (line 16, Important):
`als.enterWith(ctx)` mutates the current async context's store instead of scoping a new one, and Node's http server does not create a per-request async context — every request on one keep-alive connection runs in the socket's shared context. Two failure modes follow: (1) the store set by one request persists into later requests on the same connection, and (2) a concurrent request's `enterRequestContext` overwrites the store while the first request's handler is still awaiting. Concrete sequence: tenant A starts a long `/api/scout/run` (searches spread over minutes); tenant B's request is multiplexed onto the same pooled origin connection (prod sits behind Cloudflare per wrangler.toml/PUBLIC_DEPLOY.md) and calls `enterRequestContext({tenantId: B})` at index.ts:293 — from then on, tenant A's `xApiGet` → `recordUsageEvent` (xApi.ts:76 passes no tenantId and relies on `getRequestTenantId()`, usageMeter.ts:108) writes A's X reads into B's ledger: A gets free reads and B's credit pool drains until B is 402'd. Since this file already exports `runWithRequestContext`, wrap the request handling in `als.run(ctx, ...)` (per-request scope that restores afterward), or pass `tenantId` explicitly into `recordUsageEvent`.
als.enterWith mutates the shared async context of an http keep-alive connection, so the tenant store leaked between requests and concurrent requests clobbered each other's store. Wrap the handler body in als.run (runWithRequestContext) so the tenant scope is per-request and restored afterward. Cyclone-Source: mergestorm-vortex[bot] Cyclone-Batch: review-4934226565
|
@mergestorm-cyclone cyclone-outcome: fixed sha=0fca759 batch=review-4934226565 verify=passed Cyclone applied fixes and pushed to |
There was a problem hiding this comment.
Review progress ██████████ 37/37 files
Approve — reviewed files look good at 0fca759.
✅ All clear — nothing to fix.
🧠 Thinking traces
Reviewed the lockfile change (stripe ^17.7.0) and the two added plan images. Lockfile is consistent: manifest range, resolved version, and every transitive dependency (call-bound/get-intrinsic/side-channel*/qs/hasown etc.) are present with matching ranges, and stripe's @types/node >=8.1.0 requirement is satisfied by the hoisted entry. The plan images match the paths referenced in plans.ts. No defects found in this pass.
1 participant
Summary
Test plan