feat: global daily aggregation + admin stats page#2195
Merged
Conversation
Adds globalDailyModelStats and globalDailySourceStats with the full hourly-stats column set, aggregated from log every 5 min by a new worker loop. Skips current-day rescans when no new logs since the last refresh. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replaces the 5-min full-day rescan with an hourly walk over closed hours. A singleton globalDailyAggregationState row tracks the lastProcessedHour watermark; each tick aggregates one hour of log rows and ADDs the result to the day's totals via a col=col+EXCLUDED.col upsert, then advances the watermark in the same transaction. Once a day, the previous day is fully recomputed as a safety net for any late-arriving logs. Per-tick work is now O(rows-in-1-hour) regardless of where in the day we are, scaling cleanly to 200+ r/s. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds GET /admin/global-daily-stats with range (7d|30d|90d|365d) and groupBy (model|source) query params, returning totals + per-day timeseries + per-key breakdown summed from the new globalDailyModelStats / globalDailySourceStats tables. Adds an admin UI page at /global-daily-stats with: - range + groupBy selectors - 4 stat cards (requests, cost, tokens, distinct keys) - daily line chart with metric toggle (cost / requests / tokens) - pie chart of cost share (top 10 + Other) plus a top-25 table Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds global daily stats: DB tables and migration, an incremental aggregation worker with safety-net, worker loop wiring, an admin API GET /global-stats, org-deletion Resend cleanup, and an admin UI page with charts and sidebar navigation. ChangesGlobal Daily Statistics Pipeline
Sequence Diagram(s)sequenceDiagram
participant AdminUI as Admin Dashboard
participant AdminAPI as GET /admin/global-stats
participant AggWorker as Global Stats Worker
participant DB as Database
AdminUI->>AdminAPI: query range, groupBy
AdminAPI->>DB: select from globalModelStats or globalSourceStats
DB-->>AdminAPI: daily aggregated rows
AdminAPI-->>AdminUI: timeseries, totals, breakdown
AggWorker->>DB: read lastProcessedHour watermark
loop per bucket until max cap
AggWorker->>DB: select logs in window
AggWorker->>DB: upsert daily totals (model/source)
AggWorker->>DB: advance lastProcessedHour
end
AggWorker->>DB: check if safety net needed
alt recompute yesterday
AggWorker->>DB: delete yesterday daily rows
AggWorker->>DB: re-aggregate 24 one-hour windows
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b5503c121
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+279
to
+280
| await recomputeDayFully(yesterdayStart); | ||
| await setLastSafetyNetDay(yesterdayStart); |
There was a problem hiding this comment.
When the worker is catching up (for example on first deployment, where nextHour starts 30 days back and only 100 hours are processed per tick), this safety-net recomputes yesterday before the incremental watermark has reached yesterday. Later ticks still process those same yesterday hours through the additive upsert path, so yesterday's global totals are added on top of the rows rebuilt here and become permanently inflated. Gate the safety-net on lastProcessedHour being past the end of the recomputed day, or make the later incremental path skip/replace already recomputed hours.
Useful? React with 👍 / 👎.
Comment on lines
+315
to
+317
| await db.transaction(async (tx) => { | ||
| await aggregateHourIntoDailyStats(tx, hour); | ||
| await setLastProcessedHour(tx, hour); |
There was a problem hiding this comment.
If two worker processes run this loop at the same time, they can both read the same lastProcessedHour, enter this transaction for the same hour, and execute aggregateHourIntoDailyStats, whose conflict handler adds to existing totals. The shared state update does not claim or lock the hour before aggregation, so the same hour is double-counted; use the existing distributed lock pattern or a row-level/state claim before running the additive upserts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds cross-organization daily usage aggregation (by (usedModel, usedProvider) or x-source) and exposes it via a new admin API endpoint plus an admin UI page for visualization.
Changes:
- Introduces new DB tables for global daily aggregates and a singleton watermark table for incremental processing.
- Adds a worker loop/service to incrementally fold closed hours into daily totals, with a daily safety-net recomputation pass.
- Adds
GET /admin/global-daily-statsand a new/global-daily-statsadmin UI page (timeseries + breakdown charts).
Reviewed changes
Copilot reviewed 11 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/db/src/schema.ts | Adds global daily model/source stats tables and aggregation state table to the Drizzle schema. |
| packages/db/migrations/meta/_journal.json | Registers the new migrations in the migration journal. |
| packages/db/migrations/1778152537_sudden_blue_marvel.sql | Creates global_daily_model_stats and global_daily_source_stats plus supporting indexes. |
| packages/db/migrations/1778164250_fresh_blazing_skull.sql | Creates global_daily_aggregation_state for incremental watermarking. |
| ee/admin/src/lib/api/v1.d.ts | Adds OpenAPI/typed client definitions for /admin/global-daily-stats. |
| ee/admin/src/components/admin-shell.tsx | Adds sidebar navigation entry for the new global daily stats page. |
| ee/admin/src/app/global-daily-stats/page.tsx | Adds the server page wrapper that requires a session. |
| ee/admin/src/app/global-daily-stats/client.tsx | Implements the admin UI (cards, line chart, pie chart, top-25 table). |
| apps/worker/src/worker.ts | Starts the new global daily stats loop in the worker. |
| apps/worker/src/services/project-stats-aggregator.ts | Exports shared UTC timestamp formatting + common aggregation field builder for reuse. |
| apps/worker/src/services/global-stats-aggregator.ts | Implements incremental closed-hour processing + daily safety-net recompute. |
| apps/api/src/routes/admin.ts | Adds GET /admin/global-daily-stats endpoint aggregating totals/timeseries/breakdown. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+206
to
+230
| const [row] = await db | ||
| .select() | ||
| .from(globalDailyAggregationState) | ||
| .where(eq(globalDailyAggregationState.id, STATE_ROW_ID)) | ||
| .limit(1); | ||
| return row; | ||
| } | ||
|
|
||
| async function setLastProcessedHour(database: Tx, hour: Date): Promise<void> { | ||
| await database | ||
| .insert(globalDailyAggregationState) | ||
| .values({ id: STATE_ROW_ID, lastProcessedHour: hour }) | ||
| .onConflictDoUpdate({ | ||
| target: globalDailyAggregationState.id, | ||
| set: { lastProcessedHour: hour, updatedAt: new Date() }, | ||
| }); | ||
| } | ||
|
|
||
| async function setLastSafetyNetDay(day: Date): Promise<void> { | ||
| await db | ||
| .insert(globalDailyAggregationState) | ||
| .values({ id: STATE_ROW_ID, lastSafetyNetDay: day }) | ||
| .onConflictDoUpdate({ | ||
| target: globalDailyAggregationState.id, | ||
| set: { lastSafetyNetDay: day, updatedAt: new Date() }, |
Comment on lines
+269
to
+283
|
|
||
| const state = await readState(); | ||
| if (state?.lastSafetyNetDay && state.lastSafetyNetDay >= yesterdayStart) { | ||
| return; | ||
| } | ||
|
|
||
| logger.info( | ||
| `[global-safety-net] Recomputing ${formatUTCTimestamp(yesterdayStart)} from logs`, | ||
| ); | ||
|
|
||
| await recomputeDayFully(yesterdayStart); | ||
| await setLastSafetyNetDay(yesterdayStart); | ||
|
|
||
| logger.info( | ||
| `[global-safety-net] Recompute complete for ${formatUTCTimestamp(yesterdayStart)}`, |
Comment on lines
+883
to
+947
| const dayMs = 24 * 60 * 60 * 1000; | ||
| const startDate = new Date(); | ||
| startDate.setUTCHours(0, 0, 0, 0); | ||
| const startMs = startDate.getTime() - (days - 1) * dayMs; // eslint-disable-line no-mixed-operators | ||
| startDate.setTime(startMs); | ||
|
|
||
| const sourceTable = | ||
| groupBy === "model" ? globalDailyModelStats : globalDailySourceStats; | ||
|
|
||
| const metricSums = { | ||
| requestCount: | ||
| sql<number>`COALESCE(SUM(${sourceTable.requestCount}), 0)::int`.as( | ||
| "requestCount", | ||
| ), | ||
| errorCount: | ||
| sql<number>`COALESCE(SUM(${sourceTable.errorCount}), 0)::int`.as( | ||
| "errorCount", | ||
| ), | ||
| cacheCount: | ||
| sql<number>`COALESCE(SUM(${sourceTable.cacheCount}), 0)::int`.as( | ||
| "cacheCount", | ||
| ), | ||
| inputTokens: | ||
| sql<number>`COALESCE(SUM(CAST(${sourceTable.inputTokens} AS NUMERIC)), 0)::float8`.as( | ||
| "inputTokens", | ||
| ), | ||
| cachedTokens: | ||
| sql<number>`COALESCE(SUM(CAST(${sourceTable.cachedTokens} AS NUMERIC)), 0)::float8`.as( | ||
| "cachedTokens", | ||
| ), | ||
| outputTokens: | ||
| sql<number>`COALESCE(SUM(CAST(${sourceTable.outputTokens} AS NUMERIC)), 0)::float8`.as( | ||
| "outputTokens", | ||
| ), | ||
| totalTokens: | ||
| sql<number>`COALESCE(SUM(CAST(${sourceTable.totalTokens} AS NUMERIC)), 0)::float8`.as( | ||
| "totalTokens", | ||
| ), | ||
| cost: sql<number>`COALESCE(SUM(${sourceTable.cost}), 0)::float8`.as("cost"), | ||
| inputCost: | ||
| sql<number>`COALESCE(SUM(${sourceTable.inputCost}), 0)::float8`.as( | ||
| "inputCost", | ||
| ), | ||
| cachedInputCost: | ||
| sql<number>`COALESCE(SUM(${sourceTable.cachedInputCost}), 0)::float8`.as( | ||
| "cachedInputCost", | ||
| ), | ||
| outputCost: | ||
| sql<number>`COALESCE(SUM(${sourceTable.outputCost}), 0)::float8`.as( | ||
| "outputCost", | ||
| ), | ||
| }; | ||
|
|
||
| const dateExpr = | ||
| sql<string>`to_char(${sourceTable.dayTimestamp}, 'YYYY-MM-DD')`.as("date"); | ||
|
|
||
| const timeseriesRows = await db | ||
| .select({ | ||
| date: dateExpr, | ||
| ...metricSums, | ||
| }) | ||
| .from(sourceTable) | ||
| .where(gte(sourceTable.dayTimestamp, startDate)) | ||
| .groupBy(sourceTable.dayTimestamp) | ||
| .orderBy(asc(sourceTable.dayTimestamp)); |
| breakdownRows.map((row) => { | ||
| const isModel = "usedModel" in row; | ||
| const key = isModel ? `${row.usedProvider}/${row.usedModel}` : row.source; | ||
| const label = isModel ? row.usedModel : row.source; |
Comment on lines
+237
to
+263
| async function recomputeDayFully(day: Date): Promise<void> { | ||
| const dayStr = formatUTCTimestamp(day); | ||
|
|
||
| await db.transaction(async (tx) => { | ||
| await tx | ||
| .delete(globalDailyModelStats) | ||
| .where(sql`${globalDailyModelStats.dayTimestamp} = ${dayStr}::timestamp`); | ||
| await tx | ||
| .delete(globalDailySourceStats) | ||
| .where( | ||
| sql`${globalDailySourceStats.dayTimestamp} = ${dayStr}::timestamp`, | ||
| ); | ||
| }); | ||
|
|
||
| for (let h = 0; h < 24; h++) { | ||
| if (isStopRequested()) { | ||
| logger.info( | ||
| `[global-safety-net] Stop requested mid-recompute of ${dayStr}, leaving lastSafetyNetDay unchanged so next start retries`, | ||
| ); | ||
| return; | ||
| } | ||
| const hour = new Date(day.getTime() + h * HOUR_MS); // eslint-disable-line no-mixed-operators | ||
| await db.transaction(async (tx) => { | ||
| await aggregateHourIntoDailyStats(tx, hour); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 300447879c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
|
|
||
| await recomputeDayFully(yesterdayStart); | ||
| await setLastSafetyNetDay(yesterdayStart); |
There was a problem hiding this comment.
If the worker is asked to stop during recomputeDayFully, that helper logs that lastSafetyNetDay should remain unchanged and returns normally after deleting/rebuilding only part of the day. This line still advances lastSafetyNetDay, so a deploy/shutdown during the safety-net pass can leave yesterday's global stats partially rebuilt and prevent the next start from retrying the day.
Useful? React with 👍 / 👎.
Comment on lines
+32
to
+33
| const INITIAL_LOOKBACK_DAYS = | ||
| Number(process.env.GLOBAL_DAILY_STATS_INITIAL_LOOKBACK_DAYS) || 30; |
There was a problem hiding this comment.
With the default seed window capped at 30 days, a deployment that already has older logs will never populate the older portions of the new 90d/365d admin ranges exposed by /admin/global-daily-stats; after the watermark is initialized it only moves forward, and the safety-net only recomputes yesterday. Unless operators know to override this env var before first startup, those charts return zeros for days 31–365 even though source logs exist.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/worker/src/services/global-stats-aggregator.ts (1)
151-202: ⚡ Quick winReplace the per-row upsert loops with a single batch
INSERT … ON CONFLICT DO UPDATE.Each iteration of the two
forloops issues a separate DB round-trip. For a 30-day initial catch-up (720 hours × N model+source combos), this can amount to tens of thousands of individual statements. Drizzle supports passing an array to.values()on multi-row upserts, whereexcludedcorrectly refers to the specific row that caused each individual conflict.♻️ Proposed refactor — batch upsert for model rows (apply the same pattern to source rows)
- for (const row of modelRows) { - const { usedModel, usedProvider, ...stats } = row; - await database - .insert(globalDailyModelStats) - .values({ - dayTimestamp: sql`${dayTimestamp}::timestamp`, - usedModel, - usedProvider, - ...stats, - }) - .onConflictDoUpdate({ - target: [ - globalDailyModelStats.dayTimestamp, - globalDailyModelStats.usedModel, - globalDailyModelStats.usedProvider, - ], - set: { - ...MODEL_ADD_SET, - updatedAt: new Date(), - }, - }); - } + if (modelRows.length > 0) { + await database + .insert(globalDailyModelStats) + .values( + modelRows.map(({ usedModel, usedProvider, ...stats }) => ({ + dayTimestamp: sql`${dayTimestamp}::timestamp`, + usedModel, + usedProvider, + ...stats, + })), + ) + .onConflictDoUpdate({ + target: [ + globalDailyModelStats.dayTimestamp, + globalDailyModelStats.usedModel, + globalDailyModelStats.usedProvider, + ], + set: { + ...MODEL_ADD_SET, + updatedAt: new Date(), + }, + }); + }🤖 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. In `@apps/worker/src/services/global-stats-aggregator.ts` around lines 151 - 202, The code issues one DB round-trip per row in the two for-loops (iterating modelRows and sourceRows) — replace each per-row upsert with a single batch upsert: map modelRows into an array of value-objects (setting dayTimestamp as sql`${dayTimestamp}::timestamp`, usedModel/usedProvider and rest of stats) and call database.insert(globalDailyModelStats).values(array).onConflictDoUpdate(...) using the same target and set (MODEL_ADD_SET plus updatedAt) so the DB performs a multi-row INSERT ... ON CONFLICT DO UPDATE; do the same for sourceRows with globalDailySourceStats and SOURCE_ADD_SET. Ensure the mapped arrays match the column order/types expected by the insert so excluded references work per-row.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@apps/api/src/routes/admin.ts`:
- Around line 894-904: The COALESCE(SUM(...), 0)::int casts in the global daily
stats block (the sql`COALESCE(SUM(${sourceTable.requestCount}),
0)::int`.as("requestCount"), and analogs for "errorCount" and "cacheCount")
force 32-bit ints and can overflow; replace those casts by either removing the
cast entirely or changing to ::bigint so the SUM results remain 64-bit (e.g.,
change the sql expressions for requestCount, errorCount, and cacheCount to not
cast to int or to cast to bigint).
In `@apps/worker/src/services/global-stats-aggregator.ts`:
- Around line 265-285: runSafetyNetIfNeeded currently unconditionally calls
setLastSafetyNetDay even when recomputeDayFully returned early due to
isStopRequested(), which marks the day as complete incorrectly; change the
contract so recomputeDayFully signals success (e.g., return boolean completed or
throw on stop) and update runSafetyNetIfNeeded to check that signal and only
call setLastSafetyNetDay(yesterdayStart) when recomputeDayFully indicates a full
successful recompute; reference the functions runSafetyNetIfNeeded,
recomputeDayFully and setLastSafetyNetDay when making the change.
---
Nitpick comments:
In `@apps/worker/src/services/global-stats-aggregator.ts`:
- Around line 151-202: The code issues one DB round-trip per row in the two
for-loops (iterating modelRows and sourceRows) — replace each per-row upsert
with a single batch upsert: map modelRows into an array of value-objects
(setting dayTimestamp as sql`${dayTimestamp}::timestamp`, usedModel/usedProvider
and rest of stats) and call
database.insert(globalDailyModelStats).values(array).onConflictDoUpdate(...)
using the same target and set (MODEL_ADD_SET plus updatedAt) so the DB performs
a multi-row INSERT ... ON CONFLICT DO UPDATE; do the same for sourceRows with
globalDailySourceStats and SOURCE_ADD_SET. Ensure the mapped arrays match the
column order/types expected by the insert so excluded references work per-row.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2472c427-875e-4a07-807f-7115bd800ab0
⛔ Files ignored due to path filters (1)
ee/admin/src/lib/api/v1.d.tsis excluded by!**/v1.d.ts
📒 Files selected for processing (13)
apps/api/src/routes/admin.tsapps/worker/src/services/global-stats-aggregator.tsapps/worker/src/services/project-stats-aggregator.tsapps/worker/src/worker.tsee/admin/src/app/global-daily-stats/client.tsxee/admin/src/app/global-daily-stats/page.tsxee/admin/src/components/admin-shell.tsxpackages/db/migrations/1778152537_sudden_blue_marvel.sqlpackages/db/migrations/1778164250_fresh_blazing_skull.sqlpackages/db/migrations/meta/1778152537_snapshot.jsonpackages/db/migrations/meta/1778164250_snapshot.jsonpackages/db/migrations/meta/_journal.jsonpackages/db/src/schema.ts
Comment on lines
+894
to
+904
| sql<number>`COALESCE(SUM(${sourceTable.requestCount}), 0)::int`.as( | ||
| "requestCount", | ||
| ), | ||
| errorCount: | ||
| sql<number>`COALESCE(SUM(${sourceTable.errorCount}), 0)::int`.as( | ||
| "errorCount", | ||
| ), | ||
| cacheCount: | ||
| sql<number>`COALESCE(SUM(${sourceTable.cacheCount}), 0)::int`.as( | ||
| "cacheCount", | ||
| ), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the admin.ts file structure and the specific lines mentioned
head -n 910 apps/api/src/routes/admin.ts | tail -n 50Repository: theopenco/llmgateway
Length of output: 1453
🏁 Script executed:
# Let's get a broader view of the function containing these lines
head -n 920 apps/api/src/routes/admin.ts | tail -n 150Repository: theopenco/llmgateway
Length of output: 4079
🏁 Script executed:
# Search for all similar SUM casts in the file to see if this is a pattern
rg "SUM\(\$\{" apps/api/src/routes/admin.ts -A 1Repository: theopenco/llmgateway
Length of output: 46
🏁 Script executed:
# Check the schema to understand the source data types for these count fields
rg "requestCount|errorCount|cacheCount" apps/api/src/routes/admin.ts -B 2 -A 2 | head -n 80Repository: theopenco/llmgateway
Length of output: 2169
🏁 Script executed:
# Find imports related to SQL and the sourceTable to understand types
head -n 50 apps/api/src/routes/admin.tsRepository: theopenco/llmgateway
Length of output: 1090
🏁 Script executed:
# Check the database schema definition for these tables
rg "globalDailyModelStats|globalDailySourceStats" --type ts -A 5 -B 2 | head -n 100Repository: theopenco/llmgateway
Length of output: 6610
🏁 Script executed:
# Look for schema/table definitions in the db module
fd -e ts -path "*/db/*" | head -20Repository: theopenco/llmgateway
Length of output: 233
🏁 Script executed:
# Search for table schema definitions
rg "export const globalDaily" -A 10 | head -n 50Repository: theopenco/llmgateway
Length of output: 2027
🏁 Script executed:
# Get the full column definitions for globalDailyModelStats and globalDailySourceStats
rg "export const globalDailyModelStats" packages/db/src/schema.ts -A 30Repository: theopenco/llmgateway
Length of output: 1348
🏁 Script executed:
# Also check the sourceStats schema
rg "export const globalDailySourceStats" packages/db/src/schema.ts -A 30Repository: theopenco/llmgateway
Length of output: 1401
🏁 Script executed:
# Look for the column definitions - search for requestCount, errorCount, cacheCount
rg "requestCount|errorCount|cacheCount" packages/db/src/schema.ts -B 2 -A 2Repository: theopenco/llmgateway
Length of output: 3453
🏁 Script executed:
# Search for other instances of SUM on these count fields in the codebase
rg "SUM.*requestCount|SUM.*errorCount|SUM.*cacheCount" --type ts -B 2 -A 2Repository: theopenco/llmgateway
Length of output: 8047
🏁 Script executed:
# Check if there are any other aggregate queries on the daily stats tables
rg "globalDailyModelStats|globalDailySourceStats" apps/api/src/routes/admin.ts | grep -i "select\|sum\|aggregate"Repository: theopenco/llmgateway
Length of output: 46
Remove ::int casts on count aggregates in global daily stats endpoint
At your stated scale (1000 r/s), summing request/error/cache counts over 365 days can exceed the 32-bit integer limit (~2.1B). These casts on lines 894, 898, and 902 force PostgreSQL SUM results back to int, causing data loss. Use bigint instead. Note that other SUM() aggregates in this file (e.g., projectHourlyStats queries) omit the cast and work correctly.
Suggested fix
- requestCount:
- sql<number>`COALESCE(SUM(${sourceTable.requestCount}), 0)::int`.as(
+ requestCount:
+ sql<number>`COALESCE(SUM(${sourceTable.requestCount}), 0)::bigint`.as(
"requestCount",
),
- errorCount:
- sql<number>`COALESCE(SUM(${sourceTable.errorCount}), 0)::int`.as(
+ errorCount:
+ sql<number>`COALESCE(SUM(${sourceTable.errorCount}), 0)::bigint`.as(
"errorCount",
),
- cacheCount:
- sql<number>`COALESCE(SUM(${sourceTable.cacheCount}), 0)::int`.as(
+ cacheCount:
+ sql<number>`COALESCE(SUM(${sourceTable.cacheCount}), 0)::bigint`.as(
"cacheCount",
),🤖 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.
In `@apps/api/src/routes/admin.ts` around lines 894 - 904, The COALESCE(SUM(...),
0)::int casts in the global daily stats block (the
sql`COALESCE(SUM(${sourceTable.requestCount}), 0)::int`.as("requestCount"), and
analogs for "errorCount" and "cacheCount") force 32-bit ints and can overflow;
replace those casts by either removing the cast entirely or changing to ::bigint
so the SUM results remain 64-bit (e.g., change the sql expressions for
requestCount, errorCount, and cacheCount to not cast to int or to cast to
bigint).
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a centered Product Hunt call-to-action in the landing-page hero that opens the Product Hunt listing in a new tab. * CTA includes a dedicated Product Hunt icon alongside an arrow for clearer visual emphasis and improved click-through. * **Style** * CTA is styled as an amber, button-like link to stand out in the hero area. <!-- end of auto-generated comment: release notes by coderabbit.ai --> <img width="1622" height="929" alt="image" src="https://github.com/user-attachments/assets/eb11853a-fbba-4b99-a468-3cba890b7b65" /> --------- Co-authored-by: Ismail Ghallou <ismai23l@hotmail.com>
## Summary - When admins ban (status=deleted) an organization via the admin dashboard, also remove each member's Resend contact via the Resend API - Errors from Resend are logged but ignored, matching the existing `createResendContact` / `updateResendContact` pattern - Only fires on the deleted transition; re-enabling an org is unchanged ## Test plan - [ ] Ban an org via the admin UI and confirm members' contacts are removed from the configured Resend audience - [ ] Ban an org with `RESEND_API_KEY` unset and confirm the request still succeeds - [ ] Ban an org whose member email was already removed from Resend and confirm no failure (warning is logged) 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Chores** * Organization deletion now automatically removes associated member email contacts from the contact system. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
## Summary - The devpass subscribe flow swallowed the API error in `handleSubscribe`, so users always saw a generic "Failed to start subscription" toast — even when the API returned a clear, actionable message like "Email verification required". - The catch block now extracts `error.message` from the rejected mutation and surfaces it, falling back to the generic message only when no message is present. ## Test plan - [ ] Sign up for a new devpass account and, before verifying email, click subscribe — toast should now read "Email verification required". - [ ] Verify email and subscribe — flow should still redirect to Stripe Checkout as before. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved error messaging for dev plan subscription failures by displaying specific API error details instead of generic notifications, providing users with clearer feedback when issues occur. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
## Summary - Stripe checkout cancel for devpass redirected to `/dashboard/plans?canceled=true`, which 404s. Point it at `/dashboard?canceled=true` on the same host instead. ## Test plan - [ ] Start a devpass checkout, cancel, verify the browser lands on `/dashboard?canceled=true` 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Updated subscription checkout cancellation to redirect to the main dashboard instead of the plans page. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
[//]: # (dependabot-start)⚠️ **Dependabot is rebasing this PR**⚠️ Rebasing might not happen immediately, so don't worry if this takes some time. Note: if you make any changes to this PR yourself, they will take precedence over the rebase. --- [//]: # (dependabot-end) Bumps the all/@semantic-release/npm group with 1 update: [@semantic-release/npm](https://github.com/semantic-release/npm). Updates `@semantic-release/npm` from 12.0.2 to 13.1.5 <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/semantic-release/npm/releases"><code>@semantic-release/npm</code>'s releases</a>.</em></p> <blockquote> <h2>v13.1.5</h2> <h2><a href="https://github.com/semantic-release/npm/compare/v13.1.4...v13.1.5">13.1.5</a> (2026-03-01)</h2> <h3>Bug Fixes</h3> <ul> <li><strong>deps:</strong> update dependency normalize-url to v9 (<a href="https://redirect.github.com/semantic-release/npm/issues/1095">#1095</a>) (<a href="https://github.com/semantic-release/npm/commit/daec492f715f00a6ce61ed1e942d93c735c8fba8">daec492</a>)</li> </ul> <h2>v13.1.4</h2> <h2><a href="https://github.com/semantic-release/npm/compare/v13.1.3...v13.1.4">13.1.4</a> (2026-02-06)</h2> <h3>Bug Fixes</h3> <ul> <li><strong>deps:</strong> update dependency <code>@actions/core</code> to v3 (<a href="https://redirect.github.com/semantic-release/npm/issues/1085">#1085</a>) (<a href="https://github.com/semantic-release/npm/commit/17abfe1be0864e57be6488061e3d4ecb11189b67">17abfe1</a>)</li> </ul> <h2>v13.1.3</h2> <h2><a href="https://github.com/semantic-release/npm/compare/v13.1.2...v13.1.3">13.1.3</a> (2025-12-12)</h2> <h3>Bug Fixes</h3> <ul> <li><strong>deps:</strong> update dependency <code>@actions/core</code> to v2 (<a href="https://redirect.github.com/semantic-release/npm/issues/1055">#1055</a>) (<a href="https://github.com/semantic-release/npm/commit/fa4a3abc402b32c5ac0cd595958d60b5ac2571fe">fa4a3ab</a>)</li> </ul> <h2>v13.1.2</h2> <h2><a href="https://github.com/semantic-release/npm/compare/v13.1.1...v13.1.2">13.1.2</a> (2025-11-14)</h2> <h3>Bug Fixes</h3> <ul> <li><strong>deps:</strong> update dependency read-pkg to v10 (<a href="https://redirect.github.com/semantic-release/npm/issues/1032">#1032</a>) (<a href="https://github.com/semantic-release/npm/commit/f3f1a00b02c3dfde2ca9a2ec1c3a2b80aee23aeb">f3f1a00</a>)</li> </ul> <h2>v13.1.1</h2> <h2><a href="https://github.com/semantic-release/npm/compare/v13.1.0...v13.1.1">13.1.1</a> (2025-10-19)</h2> <h3>Bug Fixes</h3> <ul> <li><strong>publish-dry-run:</strong> temporarily remove the addition of dry-running the publish step (<a href="https://github.com/semantic-release/npm/commit/30bd1763e404f2b0a789336ade6748a20c9d3138">30bd176</a>)</li> </ul> <h2>v13.1.0</h2> <h1><a href="https://github.com/semantic-release/npm/compare/v13.0.0...v13.1.0">13.1.0</a> (2025-10-19)</h1> <h3>Features</h3> <ul> <li><strong>trusted-publishing:</strong> verify auth, considering OIDC vs tokens from various registries (<a href="https://github.com/semantic-release/npm/commit/e3319f1b2cb07eef8f61f9fa613552fa33bc92ae">e3319f1</a>), closes <a href="https://redirect.github.com/semantic-release/npm/issues/958">#958</a></li> <li><strong>trusted-publishing:</strong> refine the messages for related errors (<a href="https://github.com/semantic-release/npm/commit/316ce2161e1a7894ce9775b4c8788b966c4dc0db">316ce21</a>), closes <a href="https://redirect.github.com/semantic-release/npm/issues/958">#958</a></li> <li><strong>trusted-publishing:</strong> make request to verify if OIDC token exchange can succeed (<a href="https://github.com/semantic-release/npm/commit/c80ecb0404f44fa60c5d9edb1d3424adf8a336f0">c80ecb0</a>), closes <a href="https://redirect.github.com/semantic-release/npm/issues/958">#958</a></li> <li><strong>trusted-publishing:</strong> pass id-token as bearer header for github actions (<a href="https://github.com/semantic-release/npm/commit/d83b727440121f4e029d5c2885e9cb55f8aab839">d83b727</a>), closes <a href="https://redirect.github.com/semantic-release/npm/issues/958">#958</a></li> <li><strong>trusted-publishing:</strong> pass id-token as bearer header for gitlab pipelines (<a href="https://github.com/semantic-release/npm/commit/6d1c3cf9b3c9a9c44a855ab7fc16b7b9e76026ca">6d1c3cf</a>), closes <a href="https://redirect.github.com/semantic-release/npm/issues/958">#958</a></li> </ul> <!-- raw HTML omitted --> </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/semantic-release/npm/commit/daec492f715f00a6ce61ed1e942d93c735c8fba8"><code>daec492</code></a> fix(deps): update dependency normalize-url to v9 (<a href="https://redirect.github.com/semantic-release/npm/issues/1095">#1095</a>)</li> <li><a href="https://github.com/semantic-release/npm/commit/af8362f874f817b11d960e3350561fff45286db1"><code>af8362f</code></a> chore(deps): update dependency strip-ansi to v7.2.0 (<a href="https://redirect.github.com/semantic-release/npm/issues/1098">#1098</a>)</li> <li><a href="https://github.com/semantic-release/npm/commit/7354e1152590de8a8020ac208351a8005add18a3"><code>7354e11</code></a> chore(deps): update node.js to v24 (<a href="https://redirect.github.com/semantic-release/npm/issues/1027">#1027</a>)</li> <li><a href="https://github.com/semantic-release/npm/commit/2c4d2c94c819ec1fced1ea5cbdff4019ae0f7fc5"><code>2c4d2c9</code></a> chore(deps): update npm to v11.11.0 (<a href="https://redirect.github.com/semantic-release/npm/issues/1097">#1097</a>)</li> <li><a href="https://github.com/semantic-release/npm/commit/95ba68947e3d7d2b22a3989727cd700692f2035c"><code>95ba689</code></a> chore(deps): update dependency c8 to v11 (<a href="https://redirect.github.com/semantic-release/npm/issues/1096">#1096</a>)</li> <li><a href="https://github.com/semantic-release/npm/commit/5d32912ab7e1593a329a0aa1a101544b04df1346"><code>5d32912</code></a> chore(deps): update npm to v11.10.1 (<a href="https://redirect.github.com/semantic-release/npm/issues/1094">#1094</a>)</li> <li><a href="https://github.com/semantic-release/npm/commit/54f908c81a8b8d836653d0eb840e1fb55f259e0c"><code>54f908c</code></a> chore(deps): lock file maintenance (<a href="https://redirect.github.com/semantic-release/npm/issues/1093">#1093</a>)</li> <li><a href="https://github.com/semantic-release/npm/commit/91e1f14a87086fbb5d783d8684f87e07f7f2a16b"><code>91e1f14</code></a> chore(deps): update npm to v11.10.0 (<a href="https://redirect.github.com/semantic-release/npm/issues/1092">#1092</a>)</li> <li><a href="https://github.com/semantic-release/npm/commit/0d7d37ef14c6442a5da0ca146f25a81ceabab293"><code>0d7d37e</code></a> chore(deps): lock file maintenance (<a href="https://redirect.github.com/semantic-release/npm/issues/1089">#1089</a>)</li> <li><a href="https://github.com/semantic-release/npm/commit/c5411cc2d0158e2bca5b8fb01c3ba31bcfbea490"><code>c5411cc</code></a> chore(deps): update npm to v11.9.0 (<a href="https://redirect.github.com/semantic-release/npm/issues/1088">#1088</a>)</li> <li>Additional commits viewable in <a href="https://github.com/semantic-release/npm/compare/v12.0.2...v13.1.5">compare view</a></li> </ul> </details> <details> <summary>Maintainer changes</summary> <p>This version was pushed to npm by [GitHub Actions](<a href="https://www.npmjs.com/~GitHub">https://www.npmjs.com/~GitHub</a> Actions), a new releaser for <code>@semantic-release/npm</code> since your current version.</p> </details> <br /> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore <dependency name> major version` will close this group update PR and stop Dependabot creating any more for the specific dependency's major version (unless you unignore this specific dependency's major version or upgrade to it yourself) - `@dependabot ignore <dependency name> minor version` will close this group update PR and stop Dependabot creating any more for the specific dependency's minor version (unless you unignore this specific dependency's minor version or upgrade to it yourself) - `@dependabot ignore <dependency name>` will close this group update PR and stop Dependabot creating any more for the specific dependency (unless you unignore this specific dependency or upgrade to it yourself) - `@dependabot unignore <dependency name>` will remove all of the ignore conditions of the specified dependency - `@dependabot unignore <dependency name> <ignore condition>` will remove the ignore condition of the specified dependency and ignore conditions </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: steebchen-bot <bot@luca-steeb.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
## Summary - Add `EmailVerificationBanner` to the devpass dashboard (`apps/code`), mirroring the existing UI banner in `apps/ui` - Reuses the same `/auth/send-verification-email` endpoint, so the existing exponential backoff rate limit (60s base, capped at 24h) applies automatically - Adapted to the code app's stack: `sonner` for toasts and the local shadcn `Button` ## Test plan - [ ] Sign in to devpass dashboard with an unverified email — banner appears above main content - [ ] Click "Resend Email" — toast confirms send, email arrives - [ ] Verify email — banner disappears - [ ] Resend repeatedly — backend rate limit returns an error and toast surfaces it 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Email verification banner added to the dashboard for users with unverified emails. * Users can resend verification emails from the banner; button shows "Sending…" while processing. * **Improvements** * Verification emails now return users to the correct dashboard URL after verification. * Resend flow shows clearer success/error notifications. * **Bug Fixes** * Subscription error handling now surfaces specific error messages in toasts. * **Behavior** * Cancelling a subscription checkout now redirects to the dashboard cancel state. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Bumps [hono](https://github.com/honojs/hono) from 4.12.7 to 4.12.16. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/honojs/hono/releases">hono's releases</a>.</em></p> <blockquote> <h2>v4.12.16</h2> <h2>Security fixes</h2> <p>This release includes fixes for the following security issues:</p> <h3>Unvalidated JSX Tag Names in hono/jsx May Allow HTML Injection</h3> <p>Affects: hono/jsx. Fixes missing validation of JSX tag names when using <code>jsx()</code> or <code>createElement()</code>, which could allow HTML injection if untrusted input is used as the tag name. GHSA-69xw-7hcm-h432</p> <h3>bodyLimit() can be bypassed for chunked / unknown-length requests</h3> <p>Affects: Body Limit Middleware. Fixes late enforcement for request bodies without a reliable Content-Length (e.g. chunked requests), where oversized requests could reach handlers and return successful responses before being rejected. GHSA-9vqf-7f2p-gf9v</p> <h2>v4.12.15</h2> <h2>What's Changed</h2> <ul> <li>fix(jwt): support single-line PEM keys by <a href="https://github.com/hiendv"><code>@hiendv</code></a> in <a href="https://redirect.github.com/honojs/hono/pull/4889">honojs/hono#4889</a></li> </ul> <h2>New Contributors</h2> <ul> <li><a href="https://github.com/hiendv"><code>@hiendv</code></a> made their first contribution in <a href="https://redirect.github.com/honojs/hono/pull/4889">honojs/hono#4889</a></li> </ul> <p><strong>Full Changelog</strong>: <a href="https://github.com/honojs/hono/compare/v4.12.14...v4.12.15">https://github.com/honojs/hono/compare/v4.12.14...v4.12.15</a></p> <h2>v4.12.14</h2> <h2>Security fixes</h2> <p>This release includes fixes for the following security issues:</p> <h3>Improper handling of JSX attribute names in hono/jsx SSR</h3> <p>Affects: hono/jsx. Fixes missing validation of JSX attribute names during server-side rendering, which could allow malformed attribute keys to corrupt the generated HTML output and inject unintended attributes or elements. GHSA-458j-xx4x-4375</p> <h2>Other changes</h2> <ul> <li>fix(aws-lambda): handle invalid header names in request processing (<a href="https://redirect.github.com/honojs/hono/issues/4883">#4883</a>) fa2c74fe</li> </ul> <h2>v4.12.13</h2> <h2>What's Changed</h2> <ul> <li>fix(types): infer response type from last handler in app.on 9-/10-handler overloads by <a href="https://github.com/T4ko0522"><code>@T4ko0522</code></a> in <a href="https://redirect.github.com/honojs/hono/pull/4865">honojs/hono#4865</a></li> <li>feat(trailing-slash): add <code>skip</code> option by <a href="https://github.com/yusukebe"><code>@yusukebe</code></a> in <a href="https://redirect.github.com/honojs/hono/pull/4862">honojs/hono#4862</a></li> <li>feat(cache): add <code>onCacheNotAvailable</code> option by <a href="https://github.com/yusukebe"><code>@yusukebe</code></a> in <a href="https://redirect.github.com/honojs/hono/pull/4876">honojs/hono#4876</a></li> </ul> <h2>New Contributors</h2> <ul> <li><a href="https://github.com/T4ko0522"><code>@T4ko0522</code></a> made their first contribution in <a href="https://redirect.github.com/honojs/hono/pull/4865">honojs/hono#4865</a></li> </ul> <p><strong>Full Changelog</strong>: <a href="https://github.com/honojs/hono/compare/v4.12.12...v4.12.13">https://github.com/honojs/hono/compare/v4.12.12...v4.12.13</a></p> <h2>v4.12.12</h2> <h2>Security fixes</h2> <p>This release includes fixes for the following security issues:</p> <!-- raw HTML omitted --> </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/honojs/hono/commit/90d4182aabd328e2ec6af3f25ec62ddc574ad8cb"><code>90d4182</code></a> 4.12.16</li> <li><a href="https://github.com/honojs/hono/commit/db05b96d7a4de569ba9e965052b0593663b164fc"><code>db05b96</code></a> Merge commit from fork</li> <li><a href="https://github.com/honojs/hono/commit/614b834551378bffff70c810b2295495bc6e4d55"><code>614b834</code></a> Merge commit from fork</li> <li><a href="https://github.com/honojs/hono/commit/027e3dfca9d58bbd6b2e11adef724d26bb4f4123"><code>027e3df</code></a> fix(method-override): handle Content-Type with charset parameter (<a href="https://redirect.github.com/honojs/hono/issues/4894">#4894</a>)</li> <li><a href="https://github.com/honojs/hono/commit/f774f8df49e7ec7e205f15c5076a37132c515ebf"><code>f774f8d</code></a> 4.12.15</li> <li><a href="https://github.com/honojs/hono/commit/18fe604c8cefc2628240651b1af219692e1918c1"><code>18fe604</code></a> fix(jwt): support single-line PEM keys (<a href="https://redirect.github.com/honojs/hono/issues/4889">#4889</a>)</li> <li><a href="https://github.com/honojs/hono/commit/cf2d2b7edcf07adef2db7614557f4d7f9e2be7ba"><code>cf2d2b7</code></a> 4.12.14</li> <li><a href="https://github.com/honojs/hono/commit/66daa2edef8965544c04fcad82c596ab2acdb5ee"><code>66daa2e</code></a> Merge commit from fork</li> <li><a href="https://github.com/honojs/hono/commit/fa2c74fe5c3ce996d025d9d97bf5670c207bb82e"><code>fa2c74f</code></a> fix(aws-lambda): handle invalid header names in request processing (<a href="https://redirect.github.com/honojs/hono/issues/4883">#4883</a>)</li> <li><a href="https://github.com/honojs/hono/commit/3779927c17201dc6bfd20697f0e1ec65407da779"><code>3779927</code></a> 4.12.13</li> <li>Additional commits viewable in <a href="https://github.com/honojs/hono/compare/v4.12.7...v4.12.16">compare view</a></li> </ul> </details> <br /> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) You can disable automated security fix PRs for this repo from the [Security Alerts page](https://github.com/theopenco/llmgateway/network/alerts). </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Luca Steeb <contact@luca-steeb.com>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
## What changed This PR scopes routing health and retry decisions to the unit they actually describe (model + region + key) and recovers from a class of provider failures that previously slipped past retry. - **Retry on invalid-key 400 payloads.** Some providers surface invalid credentials with status `400` and a body like `API key not valid`. These are now classified as auth failures, so same-provider retry rotates to the next configured key and the bad credential is excluded for the rest of the request. Centralized in a new `provider-auth-errors.ts`. - **Per-model API key health.** Tracked-key health and env-based round-robin penalties are now scoped per `(key, model)` instead of per key. A bad key on `gpt-4` no longer fences `claude-3-5-sonnet` traffic on the same key. - **Region-consistent direct-provider key selection.** The two early region-lock lookups in `chat.ts` now go through the same model-scoped `findProviderKey(...)` path used at request time, so `usedRegion ??=` cannot pin one key's region to another key's token after failover. - **Same-provider, same-region retry aggregation.** Worker mapping aggregation now suppresses a failed attempt from `model_provider_mapping_history` only when the retry recovers on the same provider AND same region. A `singapore` failure recovered by `cn-beijing` is no longer dropped from `singapore`'s mapping uptime. - **Spec hardening.** Mock servers wait until they're listening before tests issue requests; touched gateway specs are less flaky under local and CI timing variance. ## Impact - Explicit-provider requests recover from invalid-key payloads by retrying another configured key for the same provider. - Key selection and uptime penalties are isolated per model. - Region-aware routing stays consistent with the scoped key actually selected after failover. - Provider mapping uptime no longer hides failed regional attempts when the recovery happens in a different region. ## Main areas changed - `apps/gateway/src/chat/chat.ts` - `apps/gateway/src/chat/tools/get-finish-reason-from-error.ts` - `apps/gateway/src/chat/tools/get-provider-env.ts` - `apps/gateway/src/chat/tools/resolve-provider-context.ts` - `apps/gateway/src/chat/tools/retry-with-fallback.ts` - `apps/gateway/src/lib/api-key-health.ts` - `apps/gateway/src/lib/cached-queries.ts` - `apps/gateway/src/lib/provider-auth-errors.ts` (new) - `apps/gateway/src/lib/round-robin-env.ts` - `apps/worker/src/services/stats-calculator.ts` - Gateway and worker specs covering invalid-key retries, model-scoped key health, region-scoped direct-provider routing, and retry aggregation Diff against `origin/main`: ~854 insertions / ~79 deletions across 21 files. ## Validation - `pnpm vitest run apps/gateway/src/fallback.spec.ts apps/gateway/src/chat/tools/retry-with-fallback.spec.ts apps/worker/src/services/stats-calculator.spec.ts --no-file-parallelism` - `pnpm format`, `pnpm lint`, `pnpm build` - `pnpm test:unit` — all PR-related tests pass <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Key-health and token selection can be scoped by model, isolating selection per model/region. * New credential-detection and retry helper to better identify invalid provider credentials. * **Bug Fixes** * Improved alternate-key retry logic for authentication and credential-error cases. * More reliable regional failover and routing with model-scoped key health and logging. * **Tests** * Expanded unit/integration tests for key-health, failover, retry, routing, and shutdown behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
## Summary OpenAI's API has no numeric reasoning-budget primitive — only `reasoning.effort` (none/minimal/low/medium/high/xhigh). The `reasoningMaxTokens: true` flag on `gpt-5.5` and `gpt-5.5-pro` (added in #2076) was unsupported upstream: a request with `reasoning.max_tokens: 1024` is rejected by OpenAI with `Unknown parameter: 'reasoning.max_tokens'`. The flag is dropped from those provider mappings; the gateway now correctly returns 400 instead of silently sending a budget that gets stripped or rejected. The earlier e2e `test.each(reasoningMaxTokensModels)` round-tripped a real completion against every capable model just to inspect the upstream request body — wasteful (real reasoning tokens are not free) and the response-shape assertions duplicated `basic reasoning`. Replaced with focused unit tests on `prepareRequestBody` covering each provider that maps the budget: - `anthropic` → `thinking.budget_tokens` - `aws-bedrock` → `additionalModelRequestFields.thinking.budget_tokens` - `google-ai-studio` → `generationConfig.thinkingConfig.thinkingBudget` - `google-vertex` → `generationConfig.thinkingConfig.thinkingBudget` The negative e2e in `api-individual.e2e.ts` (gateway returns 400 when `reasoning.max_tokens` is sent to a non-capable model) is kept — it short-circuits at validation, no provider call. ## Test plan - [x] `pnpm vitest run packages/actions/src/prepare-request-body.spec.ts` — 35/35 pass, including 4 new forwarding tests. - [x] `pnpm vitest run -c vitest/vitest.e2e.config.mts apps/gateway/src/api-individual.e2e.ts -t "reasoning.max_tokens error"` — gateway 400 path still verified. - [x] `pnpm format` clean; full build succeeds. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Corrected reasoning.max_tokens support status for gpt-5.5 and gpt-5.5-pro models on OpenAI and Azure. * Enhanced validation to reject reasoning.max_tokens requests on models that don't support this feature with appropriate error messaging. * **Tests** * Added end-to-end tests for reasoning.max_tokens handling and validation across providers. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
## Summary - Move the unauthenticated check on `/dashboard` from the client-side `useUser` redirect to a server-side guard. The page now `await`s `/user/me` via `fetchServerData` and `redirect()`s to `/login?returnUrl=/dashboard` when there is no session, so `DashboardClient` never hydrates while logged out and the client-side `useUser` → 401 → `router.push` cycle that produced the `/me` fetch loop can't trigger. - Mirrors the existing pattern in `apps/ui/src/app/dashboard/page.tsx`. ## Test plan - [ ] Visit http://localhost:3004/dashboard while logged out — server redirects to `/login?returnUrl=/dashboard`, no repeated `/user/me` calls in the network tab. - [ ] Visit `/dashboard` while logged in — page renders normally. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Dashboard now enforces user authentication requirements, automatically redirecting unauthenticated users to the login page with return URL handling to seamlessly return to the dashboard after successful authentication. * Dashboard now retrieves and displays current user information on page load, providing a personalized dashboard experience. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
If the incremental walker hasn't yet reached today (e.g. during a long backfill that spans multiple ticks), the daily safety-net pass must not wipe + recompute yesterday — the walker would later ADD yesterday's hours on top, doubling the totals. Skip when lastProcessedHour < todayStart. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/gateway/src/videos/videos.ts (1)
1220-1222:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass
selectionScopein the hybrid env fallback path.Line 1220 currently omits
selectionScopewhen selecting env keys, so hybrid mode (without provider keys) can bypass scope-aware health/routing and pick a key from the wrong scope.🔧 Proposed fix
const env = getProviderEnv(providerId, { excludedIndices: getVideoExcludedConfigIndices(providerId), + selectionScope, });🤖 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. In `@apps/gateway/src/videos/videos.ts` around lines 1220 - 1222, The call that builds env for hybrid fallback currently omits the selectionScope, causing getProviderEnv to pick keys without scope awareness; update the env construction so getProviderEnv(providerId, { excludedIndices: getVideoExcludedConfigIndices(providerId), selectionScope }) includes the selectionScope value (same selectionScope variable used elsewhere in this module) so hybrid-mode env selection is scope-aware for health/routing.packages/models/src/models/anthropic.ts (1)
380-399:⚠️ Potential issue | 🔴 CriticalRemove
cacheWriteInputPrice1hfrom AWS Bedrock entries for Sonnet 4.6, Opus 4.6, and Opus 4.7AWS Bedrock's 1-hour TTL prompt caching is only supported on Claude Sonnet 4.5, Haiku 4.5, and Opus 4.5 (GA Jan 26, 2026). The 4.6+ variants (
claude-sonnet-4-6,claude-opus-4-6,claude-opus-4-7) do not support 1h TTL on Bedrock as of May 2026.Remove
cacheWriteInputPrice1handcacheWriteInputPricefrom these three Bedrock model entries to prevent incorrect cost calculations that would overcharge users attempting to use 1h caches on unsupported models.🤖 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. In `@packages/models/src/models/anthropic.ts` around lines 380 - 399, In the AWS Bedrock model entries for modelName "claude-sonnet-4-6", "claude-opus-4-6", and "claude-opus-4-7" (objects with providerId "aws-bedrock" in packages/models/src/models/anthropic.ts), remove the cacheWriteInputPrice and cacheWriteInputPrice1h properties so the entries no longer advertise 1-hour TTL prompt caching; locate the corresponding objects (e.g., the object that currently contains cacheWriteInputPrice and cacheWriteInputPrice1h for claude-sonnet-4-6) and delete those two keys from each of the three model definitions.apps/gateway/src/moderations/moderations.ts (1)
385-420:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope-aware key selection is only partially applied
Line 389 and Line 400/415 scope key selection by
upstreamModel, but later health writes are still unscoped. That can blacklist/penalize a key across unrelated models and defeat scoped failover.Suggested fix
- reportKeyError(envVarName, configIndex, 0); + reportKeyError(envVarName, configIndex, 0, undefined, upstreamModel); - reportTrackedKeyError(providerKey.id, 0); + reportTrackedKeyError(providerKey.id, 0, undefined, upstreamModel); - reportKeyError(envVarName, configIndex, upstreamResponse.status, upstreamText); + reportKeyError( + envVarName, + configIndex, + upstreamResponse.status, + upstreamText, + upstreamModel, + ); - reportTrackedKeyError(providerKey.id, upstreamResponse.status, upstreamText); + reportTrackedKeyError( + providerKey.id, + upstreamResponse.status, + upstreamText, + upstreamModel, + ); - reportKeySuccess(envVarName, configIndex); + reportKeySuccess(envVarName, configIndex, upstreamModel); - reportTrackedKeySuccess(providerKey.id); + reportTrackedKeySuccess(providerKey.id, upstreamModel);🤖 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. In `@apps/gateway/src/moderations/moderations.ts` around lines 385 - 420, The provider key selection uses upstreamModel in findProviderKey and getProviderEnv but subsequent health/blacklist updates remain unscoped; update the health-write calls (e.g., wherever writeProviderKeyHealth, markProviderKeyFailed, or similar are invoked) to include the upstreamModel/selectionScope so health penalties apply per model, and ensure any fallback logic in the hybrid branch records health against the same scoped key returned by findProviderKey/getProviderEnv; reference findProviderKey, getProviderEnv and the health update functions to make the change.apps/gateway/src/chat/chat.ts (1)
4569-4592:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't consume fallback quota before the retry can actually run.
These branches call
checkProviderRateLimit(...)beforeresolveProviderContextForRetry(...). If context resolution fails or the candidate is skipped locally, the provider's RPM/RPD slot has already been consumed even though no upstream request was sent. Under retry churn, that can artificially exhaust fallback capacity. Move the consume step to just beforefetch, or add rollback semantics for locally-aborted retries.Also applies to: 8148-8169
🤖 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. In `@apps/gateway/src/chat/chat.ts` around lines 4569 - 4592, The code currently calls checkProviderRateLimit(...) (consuming RPM/RPD quota) before resolveProviderContextForRetry(...) and before actually issuing the upstream request, which can consume fallback slots even when the retry is aborted locally; move the rate-limit consumption to just before the actual fetch/send of the retry (i.e., after resolveProviderContextForRetry(...) and after any local-skips) or implement rollback semantics to release the consumed slot when a local abort occurs; update the retry branches that call providerRetryKey(...) and checkProviderRateLimit(...) (also the duplicate code at the other reported location) so that rate-limit checks are performed only immediately prior to the network call or are undone if resolveProviderContextForRetry(...) fails or the candidate is skipped locally.
♻️ Duplicate comments (1)
apps/api/src/routes/admin.ts (1)
895-904:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent 32-bit overflow in global daily count aggregates.
SUM(...)::intcan overflow for larger ranges/traffic and return corruptedrequestCount,errorCount, andcacheCount. Keep these aggregates 64-bit.Suggested fix
- requestCount: - sql<number>`COALESCE(SUM(${sourceTable.requestCount}), 0)::int`.as( + requestCount: + sql<number>`COALESCE(SUM(${sourceTable.requestCount}), 0)::bigint`.as( "requestCount", ), - errorCount: - sql<number>`COALESCE(SUM(${sourceTable.errorCount}), 0)::int`.as( + errorCount: + sql<number>`COALESCE(SUM(${sourceTable.errorCount}), 0)::bigint`.as( "errorCount", ), - cacheCount: - sql<number>`COALESCE(SUM(${sourceTable.cacheCount}), 0)::int`.as( + cacheCount: + sql<number>`COALESCE(SUM(${sourceTable.cacheCount}), 0)::bigint`.as( "cacheCount", ),#!/bin/bash # Verify remaining 32-bit casts for global daily count sums in this route. rg -nP 'COALESCE\(SUM\(\$\{sourceTable\.(requestCount|errorCount|cacheCount)\}\), 0\)::int' apps/api/src/routes/admin.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. In `@apps/api/src/routes/admin.ts` around lines 895 - 904, The SQL sum casts for the global daily aggregates are using 32-bit (::int) which can overflow; update the three aggregate expressions that build requestCount, errorCount, and cacheCount (the sql`COALESCE(SUM(${sourceTable.requestCount}), 0)::int`.as("requestCount") etc.) to use 64-bit casts (e.g. ::bigint or ::int8) instead of ::int and adjust any TypeScript/sql generic if necessary to reflect a 64-bit numeric type so the returned types line up with requestCount, errorCount, and cacheCount.
🧹 Nitpick comments (6)
apps/gateway/src/graceful-shutdown.spec.ts (1)
31-41: ⚡ Quick win
waitForServerListeningwill hang indefinitely if the server errors before emittinglistening.If the server emits
errorbeforelistening(e.g., due to OS resource exhaustion), the returnedPromisenever settles and the test runner will time out with no useful diagnostic.🛡️ Proposed fix — reject on server error
const waitForServerListening = (server: ServerType): Promise<void> => { const httpServer = server as Server; if (httpServer.listening) { return Promise.resolve(); } - return new Promise((resolve) => { - httpServer.once("listening", () => resolve()); + return new Promise((resolve, reject) => { + const onError = (err: Error) => { + httpServer.off("listening", onListening); + reject(err); + }; + const onListening = () => { + httpServer.off("error", onError); + resolve(); + }; + httpServer.once("listening", onListening); + httpServer.once("error", onError); }); };🤖 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. In `@apps/gateway/src/graceful-shutdown.spec.ts` around lines 31 - 41, waitForServerListening currently only waits for "listening" and will hang if the server emits "error"; update waitForServerListening (and the local httpServer variable of type ServerType/Server) to also attach a one-time "error" listener that rejects the returned Promise with the error, and ensure you clean up the other listener on settle (e.g., remove the "listening" listener when an error occurs and remove the "error" listener when listening occurs) so the Promise always settles and no listeners leak.apps/api/src/routes/dev-plans.ts (1)
957-1023: ⚡ Quick winSkip project query when no caching fields were supplied, and avoid silent success if no project exists.
Two related concerns in this block:
- The
db.query.project.findFirstruns on every settings update even when neithercachingEnablednorcacheDurationSecondswas provided (e.g. only updatingretentionLevelordevPlanAllowAllModels). Gate the lookup onprojectUpdateactually having keys.- If the project lookup returns nothing (e.g. data inconsistency for an active dev plan) but the caller passed caching fields, the request still returns
success: trueand echoes back the requested values without persisting them. Either treat this as 404 or at least log/return the original org-side updates honestly.♻️ Proposed change
- const project = await db.query.project.findFirst({ - where: { - organizationId: { - eq: personalOrg.id, - }, - }, - orderBy: { - createdAt: "asc", - }, - }); - const projectUpdate: { cachingEnabled?: boolean; cacheDurationSeconds?: number; } = {}; if (cachingEnabled !== undefined) { projectUpdate.cachingEnabled = cachingEnabled; } if (cacheDurationSeconds !== undefined) { projectUpdate.cacheDurationSeconds = cacheDurationSeconds; } - if (project && Object.keys(projectUpdate).length > 0) { + let project: typeof tables.project.$inferSelect | undefined; + if (Object.keys(projectUpdate).length > 0) { + project = await db.query.project.findFirst({ + where: { organizationId: { eq: personalOrg.id } }, + orderBy: { createdAt: "asc" }, + }); + if (!project) { + throw new HTTPException(404, { + message: "Default project not found", + }); + } await db .update(tables.project) .set(projectUpdate) .where(eq(tables.project.id, project.id));🤖 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. In `@apps/api/src/routes/dev-plans.ts` around lines 957 - 1023, Only query and update the project when there are caching-related fields to persist: wrap the db.query.project.findFirst(...) and subsequent db.update(tables.project) logic behind a check that Object.keys(projectUpdate).length > 0 so you skip the lookup when no caching fields were supplied; additionally, if projectUpdate has keys but db.query.project.findFirst(...) returns null, do not return success echoing unpersisted values—return a 404 (or an error) indicating missing project and skip writing the success response/logAuditEvent, ensuring you reference project, projectUpdate, cachingEnabled, cacheDurationSeconds, db.query.project.findFirst, and db.update(tables.project) when making the change.apps/gateway/src/lib/costs.spec.ts (1)
203-237: 💤 Low valueTest couples to hardcoded model pricing — consider extracting expected rates.
The expected costs are derived from inline literals (
1.0/1M,5.0/1M,1.25/1M,2.0/1M,discount=0.2). If Bedrock Claude Haiku 4.5 pricing changes, this test fails alongside the actual behavioral regression you'd want to catch, but the failure message will look like a math mismatch rather than a pricing-data change. Pre-existing pattern in this file though, so not a blocker.🤖 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. In `@apps/gateway/src/lib/costs.spec.ts` around lines 203 - 237, The test "should calculate AWS Bedrock Claude cache write costs" couples expected rates and discount to hardcoded literals; update the spec to reference a single source of truth instead of inline numbers by extracting the Bedrock Claude Haiku 4.5 per-token rates and discount into named constants or reading them from the same pricing lookup used by calculateCosts (e.g., use constants like CLAUDE_HAIKU_INPUT_RATE, CLAUDE_HAIKU_OUTPUT_RATE, CLAUDE_HAIKU_5M_WRITE_RATE, CLAUDE_HAIKU_1H_WRITE_RATE, DISCOUNT_MULTIPLIER) and compute expected values from those constants so the test asserts derived values rather than magic numbers.packages/actions/src/prepare-request-body.spec.ts (1)
1076-1208: 💤 Low valueReasonable forwarding tests; consider adding a negative case.
Tests verify the happy path for
reasoning_max_tokensforwarding acrossanthropic,aws-bedrock,google-ai-studio, andgoogle-vertex. One small gap worth filling later: there's no assertion that providers cap or clamp the budget (Anthropic clamps to[1024, 128000]pergetThinkingBudget). A test passingbudget = 200and assertingbudget_tokens === 1024, plus one withbudget = 200000asserting clamp to128000, would lock in that boundary behavior.🤖 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. In `@packages/actions/src/prepare-request-body.spec.ts` around lines 1076 - 1208, Add negative boundary tests to prepare-request-body.spec.ts to assert clamping behavior: call prepareRequestBody (same signature as existing tests) with reasoning_max_tokens set to 200 and assert the resulting anthropic requestBody.thinking.budget_tokens equals 1024, and call it with reasoning_max_tokens set to 200000 and assert budget_tokens equals 128000; reference prepareRequestBody and the getThinkingBudget clamping behavior (Anthropic path / requestBody.thinking) so reviewers can locate the anthropic branch to verify the expected clamped values.apps/gateway/src/lib/api-key-health.ts (1)
449-461: 💤 Low valueConsider also resetting
consecutiveErrorson the scoped record when permanently blacklisting.When a permanent auth error triggers, you set
health.permanentlyBlacklisted = trueand push to history, butconsecutiveErrorsis not reset/touched. Most call paths (isKeyHealthy) short-circuit onpermanentlyBlacklistedso this is benign today, butKeyHealth.consecutiveErrorscould be stale (still showing pre-auth errors) for any future code path that inspectsgetKeyHealth()directly for diagnostics. Same observation inreportTrackedKeyErrorat lines 504–513.Minor / debug-surface only — feel free to skip.
🤖 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. In `@apps/gateway/src/lib/api-key-health.ts` around lines 449 - 461, When permanently blacklisting a key (the block that calls markUnscopedPermanentBlacklist(getHealthKey(envVarName, keyIndex)) and sets health.permanentlyBlacklisted = true), also reset the scoped KeyHealth.consecutiveErrors to 0 on the same scoped record (the local variable health) before pushing history/pruning and returning; make the same change in reportTrackedKeyError (the block around lines 504–513) so any scoped health returned by getKeyHealth() has consecutiveErrors cleared when permanentlyBlacklisted is set.apps/gateway/src/chat/tools/retry-with-fallback.ts (1)
46-58: ⚡ Quick win
shouldRetryAlternateKey's second clause is dead code
isRetryableErrorTypealready includes"gateway_error"(line 34), so whenevererrorType === "gateway_error"the first operand of||is alreadytrueand short-circuits. The(errorType === "gateway_error" && ...)clause can never be reached, making the function equivalent toreturn isRetryableErrorType(errorType).The docstring intent — "Auth failures are not eligible for cross-provider fallback, but should rotate to another configured key" — is never enforced as written, because
shouldRetryRequestalso passesgateway_errorthroughisRetryableErrorTypeunchanged.Two consistent fixes depending on intent:
Option A – Simplify (if 401/403 cross-provider fallback is acceptable):
♻️ Proposed refactor
-import { hasInvalidProviderCredentialError } from "@/lib/provider-auth-errors.js"; - export const MAX_RETRIES = 2; -/** - * Determines whether a failed request should be retried against another key - * for the same provider. - * - * Auth failures (401/403) are not eligible for cross-provider fallback, but - * they should still rotate to another configured key for the current provider - * because the failure is often isolated to a single credential. - */ -export function shouldRetryAlternateKey( - errorType: string, - statusCode?: number, - errorText?: string, -): boolean { - return ( - isRetryableErrorType(errorType) || - (errorType === "gateway_error" && - ((statusCode !== undefined && - (statusCode === 401 || statusCode === 403)) || - hasInvalidProviderCredentialError(errorText))) - ); -} +export function shouldRetryAlternateKey(errorType: string): boolean { + return isRetryableErrorType(errorType); +}Option B – Align with the docstring (if 401/403 should NOT do cross-provider fallback):
Remove
"gateway_error"fromisRetryableErrorTypesoshouldRetryRequestgates it out, whileshouldRetryAlternateKey's second clause becomes the only path that allows same-provider key rotation on auth errors. This is a more intentional design change and should be validated against existing fallback tests.🤖 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. In `@apps/gateway/src/chat/tools/retry-with-fallback.ts` around lines 46 - 58, shouldRetryAlternateKey contains a dead branch because isRetryableErrorType already treats "gateway_error" as retryable, so the (errorType === "gateway_error" && ...) clause never runs; fix by either A) simplifying shouldRetryAlternateKey to just return isRetryableErrorType(errorType) if 401/403 cross-provider fallback is acceptable, or B) make auth errors behave differently by removing "gateway_error" from isRetryableErrorType and keeping the existing gateway-specific clause in shouldRetryAlternateKey (and then run tests that exercise shouldRetryRequest/shouldRetryAlternateKey to ensure fallback behavior remains correct); locate and edit the functions isRetryableErrorType, shouldRetryAlternateKey, and any callers like shouldRetryRequest to implement the chosen option.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@apps/api/src/routes/admin.ts`:
- Around line 4496-4504: The current unbounded Promise.all over members.map
calling deleteResendContact can spawn too many concurrent requests; change the
fan-out to process members in bounded batches (e.g., chunk members into groups
of N, such as 10) and run Promise.all for each chunk sequentially or use a
concurrency limiter (p-limit) to ensure only N concurrent deleteResendContact
calls at a time; update the code around the members.map(...) /
deleteResendContact usage to implement batching or a concurrency limiter so
deletions are rate-limited.
In `@apps/code/src/app/page.tsx`:
- Around line 110-114: Replace the raw anchor element used for the Product Hunt
badge with Next.js' Link component and add the missing opener protection: import
and use Link (from next/link) for the element currently rendered by the <a ...>
starting at the snippet and move the href, target="_blank" and className into
the Link; update the rel value to include "noopener" alongside "noreferrer"
(i.e. rel="noopener noreferrer") to harden external-link opener protection while
preserving the target and styling.
In `@apps/code/src/components/EmailVerificationBanner.tsx`:
- Around line 19-38: The handleResendVerification function can leave isResending
true if sendVerificationEmail throws; wrap the await call in a try/catch/finally
so setIsResending(true) stays at the top, await sendVerificationEmail(...)
inside try, show toast.success on success and toast.error in catch (using
error.message when available), and always call setIsResending(false) in the
finally block; reference handleResendVerification, sendVerificationEmail, and
setIsResending when making the change.
In `@apps/gateway/src/chat/chat.ts`:
- Around line 2900-2905: When blacklisting failed credentials, use
trackedKeyHealthId (not providerKey?.id) so we only exclude the key that was
actually attempted; update calls to rememberFailedKey(...) and any manipulation
of excludedProviderKeyIds to pass trackedKeyHealthId when trackedKeyHealthId is
set (fall back to providerKey?.id only if trackedKeyHealthId is undefined).
Search for uses in the retry/exclusion paths (e.g., rememberFailedKey,
excludedProviderKeyIds updates and tryResolveAlternateKeyForCurrentProvider) and
change them to prefer trackedKeyHealthId so env-overrides that cleared
trackedKeyHealthId do not incorrectly blacklist DB keys.
- Around line 2945-2960: The current region-env override replaces the tenant/DB
BYOK token (usedToken) with a gateway env credential, which breaks BYOK
isolation; change the if-block that checks regionEnvVarName/regionToken to only
perform the override when the original provider credential is a shared/credits
gateway credential (i.e., detect the provider key origin instead of always
overriding). Concretely, before assigning
usedToken/envVarName/configIndex/trackedKeyHealthId, add a guard that verifies
the original provider key is not a BYOK/DB/org-scoped key (use whatever
identifying flag on the providerKey or its metadata the code uses to mark
BYOK/shared keys), and only then allow the env override; alternatively, if the
provider key cannot serve the requested region, reject the region early rather
than falling back—apply the same check in both the api-keys and hybrid DB-key
branches where usedRegion/usedProvider/usedToken are modified.
In `@apps/gateway/src/chat/tools/extract-token-usage.ts`:
- Around line 34-57: The function extractBedrockCacheCreationDetails currently
accepts an untyped usage: any; replace that with a minimal typed shape (e.g., an
interface or inline type) describing usage.cacheDetails as an array of objects
with optional ttl (string literal union like '5m'|'1h'|string) and optional
inputTokens: number, and update local variables to use number types
(fiveMinuteTokens: number, oneHourTokens: number) and narrow detail to that
typed element (e.g., const cacheDetails: Array<{ ttl?: '5m'|'1h'|string;
inputTokens?: number }> = Array.isArray(usage?.cacheDetails) ?
usage.cacheDetails : []); keep the existing return type and logic in
extractBedrockCacheCreationDetails unchanged.
In `@apps/gateway/src/native-anthropic-cache.e2e.ts`:
- Around line 592-597: The comment describing the test references the wrong
model name ("Haiku 4.5"); update that comment to reference the actual model used
in the test (`aws-bedrock/claude-sonnet-4-6`) so the description for the 1h
cache TTL Bedrock test (the block that mentions "cache write rate" and
"prompt_tokens_details.cache_creation.ephemeral_1h_input_tokens") accurately
matches the test body.
In `@apps/ui/src/components/email-verification-banner.tsx`:
- Around line 19-41: The handleResendVerification function can leave isResending
true if sendVerificationEmail throws (network/runtime error); wrap the await
sendVerificationEmail call in try/catch/finally (use the existing
setIsResending(true) at start and move setIsResending(false) into a finally
block) so isResending is always reset, catch optional errors to show the toast
(use error?.message or a generic message), and reference
handleResendVerification, sendVerificationEmail, and setIsResending when
applying the change.
---
Outside diff comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 4569-4592: The code currently calls checkProviderRateLimit(...)
(consuming RPM/RPD quota) before resolveProviderContextForRetry(...) and before
actually issuing the upstream request, which can consume fallback slots even
when the retry is aborted locally; move the rate-limit consumption to just
before the actual fetch/send of the retry (i.e., after
resolveProviderContextForRetry(...) and after any local-skips) or implement
rollback semantics to release the consumed slot when a local abort occurs;
update the retry branches that call providerRetryKey(...) and
checkProviderRateLimit(...) (also the duplicate code at the other reported
location) so that rate-limit checks are performed only immediately prior to the
network call or are undone if resolveProviderContextForRetry(...) fails or the
candidate is skipped locally.
In `@apps/gateway/src/moderations/moderations.ts`:
- Around line 385-420: The provider key selection uses upstreamModel in
findProviderKey and getProviderEnv but subsequent health/blacklist updates
remain unscoped; update the health-write calls (e.g., wherever
writeProviderKeyHealth, markProviderKeyFailed, or similar are invoked) to
include the upstreamModel/selectionScope so health penalties apply per model,
and ensure any fallback logic in the hybrid branch records health against the
same scoped key returned by findProviderKey/getProviderEnv; reference
findProviderKey, getProviderEnv and the health update functions to make the
change.
In `@apps/gateway/src/videos/videos.ts`:
- Around line 1220-1222: The call that builds env for hybrid fallback currently
omits the selectionScope, causing getProviderEnv to pick keys without scope
awareness; update the env construction so getProviderEnv(providerId, {
excludedIndices: getVideoExcludedConfigIndices(providerId), selectionScope })
includes the selectionScope value (same selectionScope variable used elsewhere
in this module) so hybrid-mode env selection is scope-aware for health/routing.
In `@packages/models/src/models/anthropic.ts`:
- Around line 380-399: In the AWS Bedrock model entries for modelName
"claude-sonnet-4-6", "claude-opus-4-6", and "claude-opus-4-7" (objects with
providerId "aws-bedrock" in packages/models/src/models/anthropic.ts), remove the
cacheWriteInputPrice and cacheWriteInputPrice1h properties so the entries no
longer advertise 1-hour TTL prompt caching; locate the corresponding objects
(e.g., the object that currently contains cacheWriteInputPrice and
cacheWriteInputPrice1h for claude-sonnet-4-6) and delete those two keys from
each of the three model definitions.
---
Duplicate comments:
In `@apps/api/src/routes/admin.ts`:
- Around line 895-904: The SQL sum casts for the global daily aggregates are
using 32-bit (::int) which can overflow; update the three aggregate expressions
that build requestCount, errorCount, and cacheCount (the
sql`COALESCE(SUM(${sourceTable.requestCount}), 0)::int`.as("requestCount") etc.)
to use 64-bit casts (e.g. ::bigint or ::int8) instead of ::int and adjust any
TypeScript/sql generic if necessary to reflect a 64-bit numeric type so the
returned types line up with requestCount, errorCount, and cacheCount.
---
Nitpick comments:
In `@apps/api/src/routes/dev-plans.ts`:
- Around line 957-1023: Only query and update the project when there are
caching-related fields to persist: wrap the db.query.project.findFirst(...) and
subsequent db.update(tables.project) logic behind a check that
Object.keys(projectUpdate).length > 0 so you skip the lookup when no caching
fields were supplied; additionally, if projectUpdate has keys but
db.query.project.findFirst(...) returns null, do not return success echoing
unpersisted values—return a 404 (or an error) indicating missing project and
skip writing the success response/logAuditEvent, ensuring you reference project,
projectUpdate, cachingEnabled, cacheDurationSeconds, db.query.project.findFirst,
and db.update(tables.project) when making the change.
In `@apps/gateway/src/chat/tools/retry-with-fallback.ts`:
- Around line 46-58: shouldRetryAlternateKey contains a dead branch because
isRetryableErrorType already treats "gateway_error" as retryable, so the
(errorType === "gateway_error" && ...) clause never runs; fix by either A)
simplifying shouldRetryAlternateKey to just return
isRetryableErrorType(errorType) if 401/403 cross-provider fallback is
acceptable, or B) make auth errors behave differently by removing
"gateway_error" from isRetryableErrorType and keeping the existing
gateway-specific clause in shouldRetryAlternateKey (and then run tests that
exercise shouldRetryRequest/shouldRetryAlternateKey to ensure fallback behavior
remains correct); locate and edit the functions isRetryableErrorType,
shouldRetryAlternateKey, and any callers like shouldRetryRequest to implement
the chosen option.
In `@apps/gateway/src/graceful-shutdown.spec.ts`:
- Around line 31-41: waitForServerListening currently only waits for "listening"
and will hang if the server emits "error"; update waitForServerListening (and
the local httpServer variable of type ServerType/Server) to also attach a
one-time "error" listener that rejects the returned Promise with the error, and
ensure you clean up the other listener on settle (e.g., remove the "listening"
listener when an error occurs and remove the "error" listener when listening
occurs) so the Promise always settles and no listeners leak.
In `@apps/gateway/src/lib/api-key-health.ts`:
- Around line 449-461: When permanently blacklisting a key (the block that calls
markUnscopedPermanentBlacklist(getHealthKey(envVarName, keyIndex)) and sets
health.permanentlyBlacklisted = true), also reset the scoped
KeyHealth.consecutiveErrors to 0 on the same scoped record (the local variable
health) before pushing history/pruning and returning; make the same change in
reportTrackedKeyError (the block around lines 504–513) so any scoped health
returned by getKeyHealth() has consecutiveErrors cleared when
permanentlyBlacklisted is set.
In `@apps/gateway/src/lib/costs.spec.ts`:
- Around line 203-237: The test "should calculate AWS Bedrock Claude cache write
costs" couples expected rates and discount to hardcoded literals; update the
spec to reference a single source of truth instead of inline numbers by
extracting the Bedrock Claude Haiku 4.5 per-token rates and discount into named
constants or reading them from the same pricing lookup used by calculateCosts
(e.g., use constants like CLAUDE_HAIKU_INPUT_RATE, CLAUDE_HAIKU_OUTPUT_RATE,
CLAUDE_HAIKU_5M_WRITE_RATE, CLAUDE_HAIKU_1H_WRITE_RATE, DISCOUNT_MULTIPLIER) and
compute expected values from those constants so the test asserts derived values
rather than magic numbers.
In `@packages/actions/src/prepare-request-body.spec.ts`:
- Around line 1076-1208: Add negative boundary tests to
prepare-request-body.spec.ts to assert clamping behavior: call
prepareRequestBody (same signature as existing tests) with reasoning_max_tokens
set to 200 and assert the resulting anthropic requestBody.thinking.budget_tokens
equals 1024, and call it with reasoning_max_tokens set to 200000 and assert
budget_tokens equals 128000; reference prepareRequestBody and the
getThinkingBudget clamping behavior (Anthropic path / requestBody.thinking) so
reviewers can locate the anthropic branch to verify the expected clamped values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e0cfe632-b8ff-4563-98bd-eeb73dcd92b0
⛔ Files ignored due to path filters (5)
apps/code/src/lib/api/v1.d.tsis excluded by!**/v1.d.tsapps/playground/src/lib/api/v1.d.tsis excluded by!**/v1.d.tsapps/ui/src/lib/api/v1.d.tsis excluded by!**/v1.d.tsee/admin/src/lib/api/v1.d.tsis excluded by!**/v1.d.tspnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (62)
apps/api/package.jsonapps/api/src/auth/config.tsapps/api/src/routes/admin.tsapps/api/src/routes/dev-plans.tsapps/code/src/app/dashboard/DashboardClient.tsxapps/code/src/app/dashboard/components/DevPlanSettings.tsxapps/code/src/app/dashboard/page.tsxapps/code/src/app/page.tsxapps/code/src/components/EmailVerificationBanner.tsxapps/code/src/components/ProductHuntIcon.tsxapps/code/src/lib/auth-client.tsapps/gateway/package.jsonapps/gateway/src/api.spec.tsapps/gateway/src/chat/chat.tsapps/gateway/src/chat/tools/extract-token-usage.spec.tsapps/gateway/src/chat/tools/extract-token-usage.tsapps/gateway/src/chat/tools/get-finish-reason-from-error.spec.tsapps/gateway/src/chat/tools/get-finish-reason-from-error.tsapps/gateway/src/chat/tools/get-provider-env.spec.tsapps/gateway/src/chat/tools/get-provider-env.tsapps/gateway/src/chat/tools/parse-provider-response.spec.tsapps/gateway/src/chat/tools/parse-provider-response.tsapps/gateway/src/chat/tools/resolve-provider-context.tsapps/gateway/src/chat/tools/retry-with-fallback.spec.tsapps/gateway/src/chat/tools/retry-with-fallback.tsapps/gateway/src/chat/tools/transform-streaming-to-openai.spec.tsapps/gateway/src/chat/tools/transform-streaming-to-openai.tsapps/gateway/src/fallback.spec.tsapps/gateway/src/graceful-shutdown.spec.tsapps/gateway/src/lib/anthropic-pricing.spec.tsapps/gateway/src/lib/api-key-health.spec.tsapps/gateway/src/lib/api-key-health.tsapps/gateway/src/lib/cached-queries.spec.tsapps/gateway/src/lib/cached-queries.tsapps/gateway/src/lib/costs.spec.tsapps/gateway/src/lib/provider-auth-errors.tsapps/gateway/src/lib/provider-rate-limit.spec.tsapps/gateway/src/lib/provider-rate-limit.tsapps/gateway/src/lib/round-robin-env.spec.tsapps/gateway/src/lib/round-robin-env.tsapps/gateway/src/moderations/moderations.tsapps/gateway/src/native-anthropic-cache.e2e.tsapps/gateway/src/test-utils/mock-openai-server.tsapps/gateway/src/videos/videos.tsapps/ui/src/components/email-verification-banner.tsxapps/ui/src/components/settings/caching-settings.tsxapps/ui/src/components/settings/organization-retention-settings.tsxapps/ui/src/lib/auth-client.tsapps/worker/src/services/global-stats-aggregator.tsapps/worker/src/services/stats-calculator.spec.tsapps/worker/src/services/stats-calculator.tspackage.jsonpackages/actions/src/prepare-request-body.spec.tspackages/actions/src/prepare-request-body.tspackages/db/migrations/1778166992_rare_plazm.sqlpackages/db/migrations/meta/1778166992_snapshot.jsonpackages/db/migrations/meta/_journal.jsonpackages/instrumentation/package.jsonpackages/models/src/models/anthropic.tspackages/models/src/models/minimax.tspackages/models/src/models/openai.tspackages/models/src/provider.ts
💤 Files with no reviewable changes (1)
- packages/models/src/models/openai.ts
✅ Files skipped from review due to trivial changes (5)
- apps/gateway/package.json
- packages/instrumentation/package.json
- apps/gateway/src/chat/tools/retry-with-fallback.spec.ts
- apps/gateway/src/lib/provider-auth-errors.ts
- apps/code/src/components/ProductHuntIcon.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/db/migrations/meta/_journal.json
- apps/worker/src/services/global-stats-aggregator.ts
Comment on lines
+4496
to
+4504
| if (status === "deleted" && memberUserIds.length > 0) { | ||
| const members = await db.query.user.findMany({ | ||
| where: { id: { in: memberUserIds } }, | ||
| columns: { email: true }, | ||
| }); | ||
|
|
||
| await Promise.all( | ||
| members.map((member) => deleteResendContact(member.email)), | ||
| ); |
Contributor
There was a problem hiding this comment.
Bound external deletion fan-out to avoid request-path spikes.
Deleting all contacts with one unbounded Promise.all can trigger large concurrent calls to Resend and hurt reliability (429s/timeouts). Batch concurrency.
Suggested fix
if (status === "deleted" && memberUserIds.length > 0) {
const members = await db.query.user.findMany({
where: { id: { in: memberUserIds } },
columns: { email: true },
});
- await Promise.all(
- members.map((member) => deleteResendContact(member.email)),
- );
+ const batchSize = 25;
+ for (let i = 0; i < members.length; i += batchSize) {
+ const batch = members.slice(i, i + batchSize);
+ await Promise.all(
+ batch.map((member) => deleteResendContact(member.email)),
+ );
+ }
}🤖 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.
In `@apps/api/src/routes/admin.ts` around lines 4496 - 4504, The current unbounded
Promise.all over members.map calling deleteResendContact can spawn too many
concurrent requests; change the fan-out to process members in bounded batches
(e.g., chunk members into groups of N, such as 10) and run Promise.all for each
chunk sequentially or use a concurrency limiter (p-limit) to ensure only N
concurrent deleteResendContact calls at a time; update the code around the
members.map(...) / deleteResendContact usage to implement batching or a
concurrency limiter so deletions are rate-limited.
Comment on lines
+110
to
+114
| <a | ||
| href="https://www.producthunt.com/products/devpass-by-llm-gateway?launch=devpass-by-llm-gateway" | ||
| target="_blank" | ||
| rel="noreferrer" | ||
| className="inline-flex items-center gap-2.5 rounded-full border border-amber-500/40 bg-amber-500/10 px-5 py-2.5 text-sm font-semibold text-amber-700 transition-colors hover:bg-amber-500/20 dark:text-amber-300" |
Contributor
There was a problem hiding this comment.
Use next/link here and harden external-link rel
At Line 110, this should use next/link per project rules, and at Line 113 please include noopener with noreferrer for explicit opener protection.
Suggested patch
- <div className="mt-6 flex justify-center">
- <a
+ <div className="mt-6 flex justify-center">
+ <Link
href="https://www.producthunt.com/products/devpass-by-llm-gateway?launch=devpass-by-llm-gateway"
target="_blank"
- rel="noreferrer"
+ rel="noopener noreferrer"
className="inline-flex items-center gap-2.5 rounded-full border border-amber-500/40 bg-amber-500/10 px-5 py-2.5 text-sm font-semibold text-amber-700 transition-colors hover:bg-amber-500/20 dark:text-amber-300"
>
<ProductHuntIcon className="h-6 w-6" />
@@
- </a>
+ </Link>
</div>As per coding guidelines, "Use next/link for links and next/navigation's router for programmatic navigation."
🤖 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.
In `@apps/code/src/app/page.tsx` around lines 110 - 114, Replace the raw anchor
element used for the Product Hunt badge with Next.js' Link component and add the
missing opener protection: import and use Link (from next/link) for the element
currently rendered by the <a ...> starting at the snippet and move the href,
target="_blank" and className into the Link; update the rel value to include
"noopener" alongside "noreferrer" (i.e. rel="noopener noreferrer") to harden
external-link opener protection while preserving the target and styling.
Comment on lines
+19
to
+38
| const handleResendVerification = async () => { | ||
| setIsResending(true); | ||
|
|
||
| const { error } = await sendVerificationEmail({ | ||
| email: user.email, | ||
| callbackURL: `${window.location.origin}/dashboard?emailVerified=true`, | ||
| }); | ||
|
|
||
| if (error) { | ||
| toast.error("Error", { | ||
| description: error.message ?? "Failed to send verification email", | ||
| }); | ||
| } else { | ||
| toast.success("Verification email sent", { | ||
| description: "Please check your inbox for the verification email.", | ||
| }); | ||
| } | ||
|
|
||
| setIsResending(false); | ||
| }; |
Contributor
There was a problem hiding this comment.
Wrap the async call in try/finally to avoid stuck isResending state.
If sendVerificationEmail throws (network failure, runtime error, etc.), setIsResending(false) will never run and the button stays disabled. Same concern as the ui-app counterpart.
🛡️ Proposed fix
const handleResendVerification = async () => {
setIsResending(true);
-
- const { error } = await sendVerificationEmail({
- email: user.email,
- callbackURL: `${window.location.origin}/dashboard?emailVerified=true`,
- });
-
- if (error) {
- toast.error("Error", {
- description: error.message ?? "Failed to send verification email",
- });
- } else {
- toast.success("Verification email sent", {
- description: "Please check your inbox for the verification email.",
- });
+ try {
+ const { error } = await sendVerificationEmail({
+ email: user.email,
+ callbackURL: `${window.location.origin}/dashboard?emailVerified=true`,
+ });
+
+ if (error) {
+ toast.error("Error", {
+ description: error.message ?? "Failed to send verification email",
+ });
+ } else {
+ toast.success("Verification email sent", {
+ description: "Please check your inbox for the verification email.",
+ });
+ }
+ } finally {
+ setIsResending(false);
}
-
- setIsResending(false);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleResendVerification = async () => { | |
| setIsResending(true); | |
| const { error } = await sendVerificationEmail({ | |
| email: user.email, | |
| callbackURL: `${window.location.origin}/dashboard?emailVerified=true`, | |
| }); | |
| if (error) { | |
| toast.error("Error", { | |
| description: error.message ?? "Failed to send verification email", | |
| }); | |
| } else { | |
| toast.success("Verification email sent", { | |
| description: "Please check your inbox for the verification email.", | |
| }); | |
| } | |
| setIsResending(false); | |
| }; | |
| const handleResendVerification = async () => { | |
| setIsResending(true); | |
| try { | |
| const { error } = await sendVerificationEmail({ | |
| email: user.email, | |
| callbackURL: `${window.location.origin}/dashboard?emailVerified=true`, | |
| }); | |
| if (error) { | |
| toast.error("Error", { | |
| description: error.message ?? "Failed to send verification email", | |
| }); | |
| } else { | |
| toast.success("Verification email sent", { | |
| description: "Please check your inbox for the verification email.", | |
| }); | |
| } | |
| } finally { | |
| setIsResending(false); | |
| } | |
| }; |
🤖 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.
In `@apps/code/src/components/EmailVerificationBanner.tsx` around lines 19 - 38,
The handleResendVerification function can leave isResending true if
sendVerificationEmail throws; wrap the await call in a try/catch/finally so
setIsResending(true) stays at the top, await sendVerificationEmail(...) inside
try, show toast.success on success and toast.error in catch (using error.message
when available), and always call setIsResending(false) in the finally block;
reference handleResendVerification, sendVerificationEmail, and setIsResending
when making the change.
Comment on lines
+2900
to
+2905
| // ID for tracked-key health attribution. Equal to providerKey.id when the | ||
| // DB-provided key is what's actually sent. Cleared when a region-specific | ||
| // env var override replaces the token, so health failures route to the env | ||
| // credential via envVarName instead of blaming an unused DB key. Endpoint | ||
| // and option resolution still use providerKey for BYOK base URLs/options. | ||
| let trackedKeyHealthId: string | undefined; |
Contributor
There was a problem hiding this comment.
Wire trackedKeyHealthId into retry exclusion, not just health reporting.
trackedKeyHealthId is cleared when an env override wins, but the retry path still blacklists providerKey?.id via rememberFailedKey(...). A failure on the env credential will therefore still exclude the DB key in excludedProviderKeyIds, even though that key was never attempted, and tryResolveAlternateKeyForCurrentProvider() can stop early for the wrong reason.
Suggested fix
- providerKeyId: providerKey?.id,
+ providerKeyId: trackedKeyHealthId,Also applies to: 2943-2960, 3042-3059, 4240-4240
🤖 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.
In `@apps/gateway/src/chat/chat.ts` around lines 2900 - 2905, When blacklisting
failed credentials, use trackedKeyHealthId (not providerKey?.id) so we only
exclude the key that was actually attempted; update calls to
rememberFailedKey(...) and any manipulation of excludedProviderKeyIds to pass
trackedKeyHealthId when trackedKeyHealthId is set (fall back to providerKey?.id
only if trackedKeyHealthId is undefined). Search for uses in the retry/exclusion
paths (e.g., rememberFailedKey, excludedProviderKeyIds updates and
tryResolveAlternateKeyForCurrentProvider) and change them to prefer
trackedKeyHealthId so env-overrides that cleared trackedKeyHealthId do not
incorrectly blacklist DB keys.
Comment on lines
+2945
to
+2960
| // Override with region-specific env var if the DB key doesn't match the requested region. | ||
| // When we do override, route health attribution to the regional env credential. | ||
| // providerKey stays set so endpoint/options/baseUrl construction keeps the BYOK context; | ||
| // only trackedKeyHealthId is cleared so reportTrackedKey* doesn't blame the unused DB key. | ||
| if (usedRegion) { | ||
| const regionToken = getRegionSpecificEnvValue(usedProvider, usedRegion); | ||
| if (regionToken && regionToken !== usedToken) { | ||
| usedToken = regionToken; | ||
| const regionEnvVarName = getRegionSpecificEnvVarName( | ||
| usedProvider, | ||
| usedRegion, | ||
| ); | ||
| if (regionEnvVarName) { | ||
| const regionToken = process.env[regionEnvVarName]; | ||
| if (regionToken && regionToken !== usedToken) { | ||
| usedToken = regionToken; | ||
| envVarName = regionEnvVarName; | ||
| configIndex = 0; | ||
| trackedKeyHealthId = undefined; |
Contributor
There was a problem hiding this comment.
Don't fall back to gateway env credentials in BYOK flows.
In api-keys and in the DB-key branch of hybrid, this now replaces the organization's providerKey.token with a region-specific process.env[...] token whenever one exists. That means a BYOK request can leave under the shared gateway credential instead of the tenant's key, which breaks isolation/billing expectations and can hide a misconfigured regional DB key. Restrict this override to credits-only flows, or reject the region when the configured provider key cannot serve it.
Also applies to: 3044-3059
🤖 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.
In `@apps/gateway/src/chat/chat.ts` around lines 2945 - 2960, The current
region-env override replaces the tenant/DB BYOK token (usedToken) with a gateway
env credential, which breaks BYOK isolation; change the if-block that checks
regionEnvVarName/regionToken to only perform the override when the original
provider credential is a shared/credits gateway credential (i.e., detect the
provider key origin instead of always overriding). Concretely, before assigning
usedToken/envVarName/configIndex/trackedKeyHealthId, add a guard that verifies
the original provider key is not a BYOK/DB/org-scoped key (use whatever
identifying flag on the providerKey or its metadata the code uses to mark
BYOK/shared keys), and only then allow the env override; alternatively, if the
provider key cannot serve the requested region, reject the region early rather
than falling back—apply the same check in both the api-keys and hybrid DB-key
branches where usedRegion/usedProvider/usedToken are modified.
Comment on lines
+34
to
+57
| export function extractBedrockCacheCreationDetails(usage: any): { | ||
| cacheCreation5mTokens: number | null; | ||
| cacheCreation1hTokens: number | null; | ||
| } { | ||
| let fiveMinuteTokens = 0; | ||
| let oneHourTokens = 0; | ||
|
|
||
| const cacheDetails = Array.isArray(usage?.cacheDetails) | ||
| ? usage.cacheDetails | ||
| : []; | ||
| for (const detail of cacheDetails) { | ||
| const inputTokens = detail?.inputTokens ?? 0; | ||
| if (detail?.ttl === "1h") { | ||
| oneHourTokens += inputTokens; | ||
| } else if (detail?.ttl === "5m") { | ||
| fiveMinuteTokens += inputTokens; | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| cacheCreation5mTokens: fiveMinuteTokens > 0 ? fiveMinuteTokens : null, | ||
| cacheCreation1hTokens: oneHourTokens > 0 ? oneHourTokens : null, | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Avoid introducing new any in the Bedrock cache-details helper
Line 34 adds a new any parameter. Please type the minimal usage shape and keep narrowing local.
Suggested fix
-export function extractBedrockCacheCreationDetails(usage: any): {
+type BedrockUsageCacheDetail = {
+ ttl?: "5m" | "1h" | string;
+ inputTokens?: number;
+};
+
+type BedrockUsage = {
+ cacheDetails?: BedrockUsageCacheDetail[];
+};
+
+export function extractBedrockCacheCreationDetails(usage: BedrockUsage): {
cacheCreation5mTokens: number | null;
cacheCreation1hTokens: number | null;
} {As per coding guidelines, "Do not use any type in TypeScript; use strict typing instead".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function extractBedrockCacheCreationDetails(usage: any): { | |
| cacheCreation5mTokens: number | null; | |
| cacheCreation1hTokens: number | null; | |
| } { | |
| let fiveMinuteTokens = 0; | |
| let oneHourTokens = 0; | |
| const cacheDetails = Array.isArray(usage?.cacheDetails) | |
| ? usage.cacheDetails | |
| : []; | |
| for (const detail of cacheDetails) { | |
| const inputTokens = detail?.inputTokens ?? 0; | |
| if (detail?.ttl === "1h") { | |
| oneHourTokens += inputTokens; | |
| } else if (detail?.ttl === "5m") { | |
| fiveMinuteTokens += inputTokens; | |
| } | |
| } | |
| return { | |
| cacheCreation5mTokens: fiveMinuteTokens > 0 ? fiveMinuteTokens : null, | |
| cacheCreation1hTokens: oneHourTokens > 0 ? oneHourTokens : null, | |
| }; | |
| } | |
| type BedrockUsageCacheDetail = { | |
| ttl?: "5m" | "1h" | string; | |
| inputTokens?: number; | |
| }; | |
| type BedrockUsage = { | |
| cacheDetails?: BedrockUsageCacheDetail[]; | |
| }; | |
| export function extractBedrockCacheCreationDetails(usage: BedrockUsage): { | |
| cacheCreation5mTokens: number | null; | |
| cacheCreation1hTokens: number | null; | |
| } { | |
| let fiveMinuteTokens = 0; | |
| let oneHourTokens = 0; | |
| const cacheDetails = Array.isArray(usage?.cacheDetails) | |
| ? usage.cacheDetails | |
| : []; | |
| for (const detail of cacheDetails) { | |
| const inputTokens = detail?.inputTokens ?? 0; | |
| if (detail?.ttl === "1h") { | |
| oneHourTokens += inputTokens; | |
| } else if (detail?.ttl === "5m") { | |
| fiveMinuteTokens += inputTokens; | |
| } | |
| } | |
| return { | |
| cacheCreation5mTokens: fiveMinuteTokens > 0 ? fiveMinuteTokens : null, | |
| cacheCreation1hTokens: oneHourTokens > 0 ? oneHourTokens : null, | |
| }; | |
| } |
🤖 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.
In `@apps/gateway/src/chat/tools/extract-token-usage.ts` around lines 34 - 57, The
function extractBedrockCacheCreationDetails currently accepts an untyped usage:
any; replace that with a minimal typed shape (e.g., an interface or inline type)
describing usage.cacheDetails as an array of objects with optional ttl (string
literal union like '5m'|'1h'|string) and optional inputTokens: number, and
update local variables to use number types (fiveMinuteTokens: number,
oneHourTokens: number) and narrow detail to that typed element (e.g., const
cacheDetails: Array<{ ttl?: '5m'|'1h'|string; inputTokens?: number }> =
Array.isArray(usage?.cacheDetails) ? usage.cacheDetails : []); keep the existing
return type and logic in extractBedrockCacheCreationDetails unchanged.
Comment on lines
+592
to
+597
| // 1h cache TTL via Bedrock /v1/chat/completions: opts into Bedrock's 1h | ||
| // cache write rate (2x base) on a model that supports it (Haiku 4.5) and | ||
| // asserts the gateway forwards ttl:"1h" to the Converse API cachePoint and | ||
| // surfaces the response breakdown | ||
| // (prompt_tokens_details.cache_creation.ephemeral_1h_input_tokens) so SDK | ||
| // clients can attribute spend across rates. |
Contributor
There was a problem hiding this comment.
Stale model name in comment — says "Haiku 4.5" but the test targets claude-sonnet-4-6
The comment at Line 594 reads "a model that supports it (Haiku 4.5)", but the test body uses aws-bedrock/claude-sonnet-4-6. This is a copy-paste artifact from the Anthropic test above.
📝 Proposed fix
- // 1h cache TTL via Bedrock /v1/chat/completions: opts into Bedrock's 1h
- // cache write rate (2x base) on a model that supports it (Haiku 4.5) and
- // asserts the gateway forwards ttl:"1h" to the Converse API cachePoint and
+ // 1h cache TTL via Bedrock /v1/chat/completions: opts into Bedrock's 1h
+ // cache write rate (2x base) on a model that supports it (Sonnet 4.6) and
+ // asserts the gateway forwards ttl:"1h" to the Converse API cachePoint and📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // 1h cache TTL via Bedrock /v1/chat/completions: opts into Bedrock's 1h | |
| // cache write rate (2x base) on a model that supports it (Haiku 4.5) and | |
| // asserts the gateway forwards ttl:"1h" to the Converse API cachePoint and | |
| // surfaces the response breakdown | |
| // (prompt_tokens_details.cache_creation.ephemeral_1h_input_tokens) so SDK | |
| // clients can attribute spend across rates. | |
| // 1h cache TTL via Bedrock /v1/chat/completions: opts into Bedrock's 1h | |
| // cache write rate (2x base) on a model that supports it (Sonnet 4.6) and | |
| // asserts the gateway forwards ttl:"1h" to the Converse API cachePoint and | |
| // surfaces the response breakdown | |
| // (prompt_tokens_details.cache_creation.ephemeral_1h_input_tokens) so SDK | |
| // clients can attribute spend across rates. |
🤖 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.
In `@apps/gateway/src/native-anthropic-cache.e2e.ts` around lines 592 - 597, The
comment describing the test references the wrong model name ("Haiku 4.5");
update that comment to reference the actual model used in the test
(`aws-bedrock/claude-sonnet-4-6`) so the description for the 1h cache TTL
Bedrock test (the block that mentions "cache write rate" and
"prompt_tokens_details.cache_creation.ephemeral_1h_input_tokens") accurately
matches the test body.
Comment on lines
19
to
41
| const handleResendVerification = async () => { | ||
| setIsResending(true); | ||
|
|
||
| try { | ||
| const response = await fetch( | ||
| `${config.apiUrl}/auth/send-verification-email`, | ||
| { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify({ | ||
| email: user.email, | ||
| callbackURL: `${window.location.origin}/dashboard?emailVerified=true`, | ||
| }), | ||
| }, | ||
| ); | ||
| const { error } = await sendVerificationEmail({ | ||
| email: user.email, | ||
| callbackURL: `${window.location.origin}/dashboard?emailVerified=true`, | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error("Failed to send verification email"); | ||
| } | ||
|
|
||
| toast({ | ||
| title: "Verification email sent", | ||
| description: "Please check your inbox for the verification email.", | ||
| }); | ||
| } catch (error) { | ||
| if (error) { | ||
| toast({ | ||
| title: "Error", | ||
| description: | ||
| error instanceof Error | ||
| ? error.message | ||
| : "Failed to send verification email", | ||
| description: error.message ?? "Failed to send verification email", | ||
| variant: "destructive", | ||
| }); | ||
| } finally { | ||
| setIsResending(false); | ||
| } else { | ||
| toast({ | ||
| title: "Verification email sent", | ||
| description: "Please check your inbox for the verification email.", | ||
| }); | ||
| } | ||
|
|
||
| setIsResending(false); | ||
| }; |
Contributor
There was a problem hiding this comment.
Restore finally to reset isResending on thrown errors.
While sendVerificationEmail returns { error } for handled API failures, it can still throw on network/runtime errors (better-auth uses better-fetch under the hood). The previous implementation wrapped this in try/catch/finally; without it, an unexpected throw leaves isResending stuck at true and the button permanently disabled.
🛡️ Proposed fix
const handleResendVerification = async () => {
setIsResending(true);
-
- const { error } = await sendVerificationEmail({
- email: user.email,
- callbackURL: `${window.location.origin}/dashboard?emailVerified=true`,
- });
-
- if (error) {
- toast({
- title: "Error",
- description: error.message ?? "Failed to send verification email",
- variant: "destructive",
- });
- } else {
- toast({
- title: "Verification email sent",
- description: "Please check your inbox for the verification email.",
- });
+ try {
+ const { error } = await sendVerificationEmail({
+ email: user.email,
+ callbackURL: `${window.location.origin}/dashboard?emailVerified=true`,
+ });
+
+ if (error) {
+ toast({
+ title: "Error",
+ description: error.message ?? "Failed to send verification email",
+ variant: "destructive",
+ });
+ } else {
+ toast({
+ title: "Verification email sent",
+ description: "Please check your inbox for the verification email.",
+ });
+ }
+ } finally {
+ setIsResending(false);
}
-
- setIsResending(false);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleResendVerification = async () => { | |
| setIsResending(true); | |
| try { | |
| const response = await fetch( | |
| `${config.apiUrl}/auth/send-verification-email`, | |
| { | |
| method: "POST", | |
| headers: { | |
| "Content-Type": "application/json", | |
| }, | |
| body: JSON.stringify({ | |
| email: user.email, | |
| callbackURL: `${window.location.origin}/dashboard?emailVerified=true`, | |
| }), | |
| }, | |
| ); | |
| const { error } = await sendVerificationEmail({ | |
| email: user.email, | |
| callbackURL: `${window.location.origin}/dashboard?emailVerified=true`, | |
| }); | |
| if (!response.ok) { | |
| throw new Error("Failed to send verification email"); | |
| } | |
| toast({ | |
| title: "Verification email sent", | |
| description: "Please check your inbox for the verification email.", | |
| }); | |
| } catch (error) { | |
| if (error) { | |
| toast({ | |
| title: "Error", | |
| description: | |
| error instanceof Error | |
| ? error.message | |
| : "Failed to send verification email", | |
| description: error.message ?? "Failed to send verification email", | |
| variant: "destructive", | |
| }); | |
| } finally { | |
| setIsResending(false); | |
| } else { | |
| toast({ | |
| title: "Verification email sent", | |
| description: "Please check your inbox for the verification email.", | |
| }); | |
| } | |
| setIsResending(false); | |
| }; | |
| const handleResendVerification = async () => { | |
| setIsResending(true); | |
| try { | |
| const { error } = await sendVerificationEmail({ | |
| email: user.email, | |
| callbackURL: `${window.location.origin}/dashboard?emailVerified=true`, | |
| }); | |
| if (error) { | |
| toast({ | |
| title: "Error", | |
| description: error.message ?? "Failed to send verification email", | |
| variant: "destructive", | |
| }); | |
| } else { | |
| toast({ | |
| title: "Verification email sent", | |
| description: "Please check your inbox for the verification email.", | |
| }); | |
| } | |
| } finally { | |
| setIsResending(false); | |
| } | |
| }; |
🤖 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.
In `@apps/ui/src/components/email-verification-banner.tsx` around lines 19 - 41,
The handleResendVerification function can leave isResending true if
sendVerificationEmail throws (network/runtime error); wrap the await
sendVerificationEmail call in try/catch/finally (use the existing
setIsResending(true) at start and move setIsResending(false) into a finally
block) so isResending is always reset, catch optional errors to show the toast
(use error?.message or a generic message), and reference
handleResendVerification, sendVerificationEmail, and setIsResending when
applying the change.
…egation # Conflicts: # apps/gateway/src/lib/costs.spec.ts
Drop "daily" from table names and adjacent identifiers since the
aggregation cadence is configurable via GLOBAL_STATS_BUCKET_SECONDS.
The rows themselves are still day-bucketed (dayTimestamp column is
unchanged), but the table names no longer imply a specific update
frequency.
Renames:
table global_daily_model_stats -> global_model_stats
global_daily_source_stats -> global_source_stats
global_daily_aggregation_state -> global_aggregation_state
env GLOBAL_DAILY_STATS_* -> GLOBAL_STATS_*
MAX_HOURS_PER_TICK -> MAX_BUCKETS_PER_TICK
api /admin/global-daily-stats -> /admin/global-stats
page /global-daily-stats -> /global-stats
fn aggregateWindowIntoDailyStats -> aggregateWindowIntoStats
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41f19fb567
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+893
to
+895
| requestCount: | ||
| sql<number>`COALESCE(SUM(${sourceTable.requestCount}), 0)::int`.as( | ||
| "requestCount", |
There was a problem hiding this comment.
For high-traffic ranges this SUM(... )::int can make /admin/global-stats fail with integer out of range: PostgreSQL's SUM(integer) can exceed 2,147,483,647 over 30/90/365 days, and the commit explicitly targets up to ~1000 r/s where 30 days is ~2.6B requests. Keep these count sums as bigint/numeric or cast to float8 before returning; the same applies to the adjacent errorCount and cacheCount sums.
Useful? React with 👍 / 👎.
getTableColumns() returns the JS-side camelCase identifier in col.name when no explicit name was passed. Drizzle's snake_case casing only applies at SQL emission time, so excluded.requestCount was referencing a column that doesn't exist (the actual column is request_count). Convert the JS key to snake_case before passing to sql.identifier(). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/worker/src/services/global-stats-aggregator.ts (1)
308-313:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
setLastSafetyNetDayis still called unconditionally after a potentially partialrecomputeDayFully.This issue was raised in a previous review and remains unresolved. When
recomputeDayFullyreturns early becauseisStopRequested()fired mid-loop, the DELETE has already run but not all 24 hours have been re-inserted.setLastSafetyNetDaythen marks yesterday as fully recomputed, so the safety net will never retry, and the incremental walker (whose watermark is already past yesterday) will also never backfill the missing hours.🐛 Proposed fix (same as previous review)
await recomputeDayFully(yesterdayStart); - await setLastSafetyNetDay(yesterdayStart); - - logger.info( - `[global-safety-net] Recompute complete for ${formatUTCTimestamp(yesterdayStart)}`, - ); + if (!isStopRequested()) { + await setLastSafetyNetDay(yesterdayStart); + logger.info( + `[global-safety-net] Recompute complete for ${formatUTCTimestamp(yesterdayStart)}`, + ); + }🤖 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. In `@apps/worker/src/services/global-stats-aggregator.ts` around lines 308 - 313, recomputeDayFully may return early when isStopRequested() fires, but setLastSafetyNetDay(yesterdayStart) is still called unconditionally; change the flow so setLastSafetyNetDay is only called when recomputeDayFully actually completed all hours. Have recomputeDayFully return a boolean (e.g., completedFully) or throw on unrecoverable failure, update the caller to await const completed = await recomputeDayFully(yesterdayStart) and only call await setLastSafetyNetDay(yesterdayStart) (and log the success message) when completed is true; keep existing isStopRequested checks inside recomputeDayFully and ensure the returned value accurately reflects a full recompute.
🧹 Nitpick comments (1)
apps/worker/src/services/global-stats-aggregator.ts (1)
114-126: ⚡ Quick winAdd a runtime guard for unknown keys in
buildAddUpsertSet.
cols[key]is typed as non-undefined due to theas Record<…>cast, but if any entry inAGGREGATE_KEYSis absent from the table (e.g. after a schema rename),colisundefinedat runtime. SinceMODEL_ADD_SET/SOURCE_ADD_SETare built once at module load and reused for every upsert, a single stale key would silently corrupt the SET clause for all incremental writes with no TypeScript warning.🛡️ Proposed defensive guard
function buildAddUpsertSet(table: AnyTable) { const cols = getTableColumns(table) as Record< string, { name: string } & object >; const set: Record<string, ReturnType<typeof sql>> = {}; for (const key of AGGREGATE_KEYS) { const col = cols[key]; + if (!col) { + throw new Error( + `buildAddUpsertSet: column "${key}" not found in table – AGGREGATE_KEYS is out of sync with the schema`, + ); + } const snakeName = toSnakeCase(key); set[key] = sql`${col} + excluded.${sql.identifier(snakeName)}`; } return set; }🤖 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. In `@apps/worker/src/services/global-stats-aggregator.ts` around lines 114 - 126, The buildAddUpsertSet function currently assumes every key in AGGREGATE_KEYS exists in cols; add a runtime guard after const col = cols[key] to detect missing columns (cols[key] === undefined) and throw a clear Error (or fail fast) naming the missing key and the table to prevent constructing an invalid SET clause; update callers that build MODEL_ADD_SET / SOURCE_ADD_SET to rely on this early failure so stale keys are caught at module load instead of silently corrupting upserts.
🤖 Prompt for all review comments with 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.
Duplicate comments:
In `@apps/worker/src/services/global-stats-aggregator.ts`:
- Around line 308-313: recomputeDayFully may return early when isStopRequested()
fires, but setLastSafetyNetDay(yesterdayStart) is still called unconditionally;
change the flow so setLastSafetyNetDay is only called when recomputeDayFully
actually completed all hours. Have recomputeDayFully return a boolean (e.g.,
completedFully) or throw on unrecoverable failure, update the caller to await
const completed = await recomputeDayFully(yesterdayStart) and only call await
setLastSafetyNetDay(yesterdayStart) (and log the success message) when completed
is true; keep existing isStopRequested checks inside recomputeDayFully and
ensure the returned value accurately reflects a full recompute.
---
Nitpick comments:
In `@apps/worker/src/services/global-stats-aggregator.ts`:
- Around line 114-126: The buildAddUpsertSet function currently assumes every
key in AGGREGATE_KEYS exists in cols; add a runtime guard after const col =
cols[key] to detect missing columns (cols[key] === undefined) and throw a clear
Error (or fail fast) naming the missing key and the table to prevent
constructing an invalid SET clause; update callers that build MODEL_ADD_SET /
SOURCE_ADD_SET to rely on this early failure so stale keys are caught at module
load instead of silently corrupting upserts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 58a2fa9f-5d74-4b50-a6b4-5a0029829975
📒 Files selected for processing (1)
apps/worker/src/services/global-stats-aggregator.ts
The pie chart and breakdown table now respond to the same cost/requests/total tokens toggle as the line chart, so the metric selector in the pie card switches both the slice values and the percentages in the tooltip + the column shown in the breakdown table. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1aea710e53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+1033
to
+1036
| .from(globalSourceStats) | ||
| .where(gte(globalSourceStats.dayTimestamp, startDate)) | ||
| .groupBy(globalSourceStats.source) | ||
| .orderBy(desc(metricSums.requestCount)); |
There was a problem hiding this comment.
When groupBy=source, this query materializes and returns every distinct x-source value in the selected range. Since x-source is supplied on gateway requests and only character-validated, a workload that sends many unique sources over 90d/365d can make the admin stats endpoint group, sort, serialize, and render an unbounded response even though the UI only displays the top slices/rows. Consider limiting in SQL and computing an aggregated “Other” bucket server-side.
Useful? React with 👍 / 👎.
recomputeDayFully returned early on isStopRequested() but the caller unconditionally called setLastSafetyNetDay, causing the day to be flagged as fully recomputed even though only a partial subset of its 24 hour-chunks had been re-aggregated. Next worker start would skip the safety net for that day, leaving the partial data in place. Change recomputeDayFully to return a boolean (true on full completion, false on stop) and gate setLastSafetyNetDay on it. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
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
globalDailyModelStats+globalDailySourceStatstables aggregating cross-org usage by day, grouped by(usedModel, usedProvider)andcoalesce(source, 'unknown')respectively. Same metric column set as the existing project-hourly tables.globalDailyAggregationState. Each tick reads ~1h oflogrows and ADDs the result to the day's totals viacol = col + EXCLUDED.colupserts; per-tick work is constant regardless of where in the day we are. Once per day, the previous day is fully recomputed as a safety net for late-arriving logs.GET /admin/global-daily-stats?range=7d|30d|90d|365d&groupBy=model|sourcereturning totals + per-day timeseries + per-key breakdown./global-daily-statswith range + groupBy selectors, 4 stat cards, daily line chart with metric toggle, and a pie chart of cost share with a top-25 table.Scaling
At 50 r/s (4.3M rows/day), the previous "rescan whole day every 5 min" would be ~600M row-reads/day. The hourly-incremental design is O(rows-in-1-hour) per tick — comfortable to ~1000 r/s. Trade-off: the current day's stats lag by ~1–2h until each closed hour is processed.
Test plan
pnpm buildpasses (17/17)pnpm test:unitpasses (1151/1153)/global-daily-statswith admin auth, switch range + groupBy, verify charts render🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores