feat: DB-driven gateway margin + mount in core#175
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 12 minutes and 58 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughDynamic per-request margin resolution is introduced for the gateway proxy via a new Changes
Sequence Diagram(s)sequenceDiagram
participant Server as Server (bootPlatformServer)
participant Router as RouteMounter (mountRoutes)
participant Gateway as Gateway Module
participant Meter as Metering System
participant Budget as Budget Checker
participant Provider as OpenRouter Provider
Server->>Router: await mountRoutes(container)
Router->>Router: if container.gateway present
Router->>Router: validate productConfig.billing?.marginConfig.default
Router->>Router: create resolveMargin() callback
Router->>Provider: configure using OPENROUTER_API_KEY / base URL
Router->>Gateway: dynamic import ../gateway/index.js
Router->>Gateway: mountGateway(app, { meter, budgetChecker, creditLedger, resolveMargin, resolveServiceKey })
Gateway->>Meter: initialize MeterEmitter (flushIntervalMs: 5000)
Gateway->>Budget: initialize BudgetChecker (cacheTtlMs: 30000)
Router->>Server: return (mountRoutes done)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Coverage Report
File CoverageNo changed files found. |
- Gateway mounted in core mountRoutes (all products get it) - Margin from product_billing_config.margin_config.default (no fallback) - resolveMargin getter for runtime DB changes - MeterEmitter + BudgetChecker in core container Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
TSavo
force-pushed
the
feat/db-driven-gateway
branch
from
c33c17b to
b37f567
Compare
Greptile SummaryThis PR mounts the metered inference gateway centrally in Key findings:
Confidence Score: 3/5Not safe to merge — the advertised live margin feature is a no-op, and the change departs from a team-wide convention without updating it. Two P1 findings block merge: the resolveMargin closure silently reads a stale boot-time snapshot (operators who update the DB margin expecting a live effect will see no change), and the DB-driven margin approach directly contradicts the MARGIN_CONFIG_JSON env-var convention codified in every WOPR codebase rule. The remaining findings are P2 style issues. src/server/mount-routes.ts requires the most attention — both P1 issues originate there.
|
| Filename | Overview |
|---|---|
| src/server/mount-routes.ts | Core of this PR — adds gateway mounting with DB-driven margin; resolveMargin reads a stale boot-time snapshot (P1) and uses the DB rather than MARGIN_CONFIG_JSON env var per convention (P1); minor falsy-guard and comment numbering issues. |
| src/server/container.ts | Constructs MeterEmitter and DrizzleBudgetChecker in the gateway block — correct Drizzle repository wiring; meter typed as concrete class instead of IMeterEmitter interface (P2 style). |
| src/gateway/proxy.ts | Replaces static DEFAULT_MARGIN with a getter-based defaultMargin that calls resolveMargin when provided; the proxy layer change itself is clean. |
| src/gateway/types.ts | Adds optional resolveMargin to GatewayConfig; straightforward type extension with clear doc comments. |
| src/index.ts | Four leftover CI breadcrumb comments appended after the tRPC export; no functional change but should be removed before merge. |
| src/server/index.ts | Adds await to mountRoutes call — necessary now that the function is async; straightforward and correct. |
| src/server/tests/container.test.ts | Updates test container stubs to include the new meter and budgetChecker fields; no logic issues. |
| src/server/routes/tests/admin.test.ts | Adds meter and budgetChecker stubs to existing gateway fixture objects; routine test housekeeping. |
Sequence Diagram
sequenceDiagram
participant Boot as bootPlatformServer
participant BC as buildContainer
participant MR as mountRoutes
participant GW as mountGateway
participant Req as Inbound Request
Boot->>BC: await buildContainer(config)
BC->>BC: new DrizzleMeterEventRepository(db)
BC->>BC: new MeterEmitter(repo, {flushIntervalMs:5000})
BC->>BC: new DrizzleBudgetChecker(db, {cacheTtlMs:30000})
BC-->>Boot: container (productConfig snapshot frozen here)
Boot->>MR: await mountRoutes(app, container, config)
MR->>MR: read container.productConfig.billing.marginConfig.default
MR->>MR: capture initialMargin (boot-time value)
MR->>MR: define resolveMargin() → reads same frozen snapshot
MR->>GW: mountGateway(app, { resolveMargin, meter, budgetChecker })
Req->>GW: POST /v1/chat/completions
GW->>GW: resolveMargin() → returns boot-time margin (not live DB)
GW->>GW: budget check via DrizzleBudgetChecker
GW->>GW: proxy to upstream provider
GW->>GW: MeterEmitter.emit(event)
Comments Outside Diff (1)
-
src/server/mount-routes.ts, line 135-141 (link)Duplicate step number
7in comments; docblock also out of dateAfter inserting the gateway as step 6, both "Product-specific route plugins" and "Tenant proxy middleware" are labelled
// 7.. The top-of-function docblock still lists the old numbering (only 7 items, gateway missing). Suggest:And update the docblock to include the new step 6 (gateway) and renumber 7 → 8 for plugins and tenant proxy.
Prompt To Fix With AI
This is a comment left during a code review. Path: src/server/mount-routes.ts Line: 135-141 Comment: **Duplicate step number `7` in comments; docblock also out of date** After inserting the gateway as step 6, both "Product-specific route plugins" and "Tenant proxy middleware" are labelled `// 7.`. The top-of-function docblock still lists the old numbering (only 7 items, gateway missing). Suggest: And update the docblock to include the new step 6 (gateway) and renumber 7 → 8 for plugins and tenant proxy. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/server/mount-routes.ts
Line: 110-115
Comment:
**`resolveMargin` reads a stale boot-time snapshot, not live DB values**
`container.productConfig` is a plain `ProductConfig` object set once when `buildContainer` runs and is **never mutated or reassigned** afterward. The closure therefore always returns the margin captured at startup — identical to `initialMargin`. The `ProductConfigService` (with its TTL cache) is available as `container.productConfigService`, but it is not used here.
The PR description claims "resolveMargin getter for runtime DB changes without restart", but since `container.productConfig` is a frozen snapshot, this does not work. An operator who updates `margin_config.default` in the DB mid-run will see no effect until the server restarts.
To make the margin truly live the resolver must call `container.productConfigService.getBySlug(slug)` and await it; but since `resolveMargin` is typed as `() => number` (sync), this requires either making the type `() => Promise<number>` throughout the gateway, or adding a separate background-refresh loop that pushes the cached value into a mutable local variable. As-is, the "live margin" feature is a no-op.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/server/mount-routes.ts
Line: 103-108
Comment:
**Margin config convention violated — must use `MARGIN_CONFIG_JSON` env var**
Every WOPR codebase convention rule explicitly states: *"Margin config is stored in `MARGIN_CONFIG_JSON` env var, never in source"* (with `loadMarginConfig()` in `src/monetization/adapters/margin-config.ts` as the canonical loader). This PR moves the margin to `product_billing_config.margin_config.default` in the database instead, which is an intentional but undiscussed departure from the team-wide convention.
If the intent is to permanently migrate to DB-driven margin, the custom instruction rules and `loadMarginConfig()` path should be updated in the same PR so no future author is misled. If this is a product-specific override on top of the env-var baseline, the env-var path should still be respected as a fallback.
**Rule Used:** WOPR codebase conventions:
- Repository pattern is... ([source](https://app.greptile.com/review/custom-context?memory=b986c674-3c9f-4f81-9c16-19366bd8a16f))
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/server/container.ts
Line: 54-55
Comment:
**`meter` typed as concrete class instead of its interface**
`IMeterEmitter` is exported from `src/metering/index.ts` and matches the convention that sub-container fields take repository/emitter *interfaces*, not implementations. `budgetChecker` correctly uses `IBudgetChecker`; `meter` should be consistent:
```suggestion
meter: import("../metering/emitter.js").IMeterEmitter;
budgetChecker: import("../monetization/budget/budget-checker.js").IBudgetChecker;
```
**Rule Used:** WOPR codebase conventions:
- Repository pattern is... ([source](https://app.greptile.com/review/custom-context?memory=158cf84c-c9a3-4390-b104-b5df2c2423d6))
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/index.ts
Line: 51-54
Comment:
**Leftover CI breadcrumb comments in public barrel**
These four lines look like temporary markers left over from debugging the build pipeline and should not be committed to the public export barrel:
```suggestion
// tRPC
export * from "./trpc/index.js";
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/server/mount-routes.ts
Line: 135-141
Comment:
**Duplicate step number `7` in comments; docblock also out of date**
After inserting the gateway as step 6, both "Product-specific route plugins" and "Tenant proxy middleware" are labelled `// 7.`. The top-of-function docblock still lists the old numbering (only 7 items, gateway missing). Suggest:
```suggestion
// 7. Product-specific route plugins
for (const plugin of plugins) {
app.route(plugin.path, plugin.handler(container));
}
// 8. Tenant proxy middleware (catch-all — MUST be last)
```
And update the docblock to include the new step 6 (gateway) and renumber 7 → 8 for plugins and tenant proxy.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/server/mount-routes.ts
Line: 103-108
Comment:
**Falsy guard rejects a `0` margin**
`!marginConfig?.default` is `true` when `default` is `0`, which would throw even though `0` is a valid (if degenerate) numeric value. Prefer an explicit null/undefined check:
```suggestion
if (marginConfig?.default == null) {
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat: DB-driven gateway margin + mount i..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Live margin — reads from productConfig per-request (DB-cached with TTL) | ||
| const initialMargin = marginConfig.default; | ||
| const resolveMargin = (): number => { | ||
| const cfg = container.productConfig.billing?.marginConfig as { default?: number } | null; | ||
| return cfg?.default ?? initialMargin; | ||
| }; |
There was a problem hiding this comment.
resolveMargin reads a stale boot-time snapshot, not live DB values
container.productConfig is a plain ProductConfig object set once when buildContainer runs and is never mutated or reassigned afterward. The closure therefore always returns the margin captured at startup — identical to initialMargin. The ProductConfigService (with its TTL cache) is available as container.productConfigService, but it is not used here.
The PR description claims "resolveMargin getter for runtime DB changes without restart", but since container.productConfig is a frozen snapshot, this does not work. An operator who updates margin_config.default in the DB mid-run will see no effect until the server restarts.
To make the margin truly live the resolver must call container.productConfigService.getBySlug(slug) and await it; but since resolveMargin is typed as () => number (sync), this requires either making the type () => Promise<number> throughout the gateway, or adding a separate background-refresh loop that pushes the cached value into a mutable local variable. As-is, the "live margin" feature is a no-op.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/server/mount-routes.ts
Line: 110-115
Comment:
**`resolveMargin` reads a stale boot-time snapshot, not live DB values**
`container.productConfig` is a plain `ProductConfig` object set once when `buildContainer` runs and is **never mutated or reassigned** afterward. The closure therefore always returns the margin captured at startup — identical to `initialMargin`. The `ProductConfigService` (with its TTL cache) is available as `container.productConfigService`, but it is not used here.
The PR description claims "resolveMargin getter for runtime DB changes without restart", but since `container.productConfig` is a frozen snapshot, this does not work. An operator who updates `margin_config.default` in the DB mid-run will see no effect until the server restarts.
To make the margin truly live the resolver must call `container.productConfigService.getBySlug(slug)` and await it; but since `resolveMargin` is typed as `() => number` (sync), this requires either making the type `() => Promise<number>` throughout the gateway, or adding a separate background-refresh loop that pushes the cached value into a mutable local variable. As-is, the "live margin" feature is a no-op.
How can I resolve this? If you propose a fix, please make it concise.| if (!marginConfig?.default) { | ||
| throw new Error( | ||
| "Gateway enabled but product_billing_config.margin_config.default is not set. " + | ||
| "Seed the DB: INSERT INTO product_billing_config (product_id, margin_config) VALUES ('<id>', '{\"default\": 4.0}')", | ||
| ); | ||
| } |
There was a problem hiding this comment.
MARGIN_CONFIG_JSON env var
Every WOPR codebase convention rule explicitly states: "Margin config is stored in MARGIN_CONFIG_JSON env var, never in source" (with loadMarginConfig() in src/monetization/adapters/margin-config.ts as the canonical loader). This PR moves the margin to product_billing_config.margin_config.default in the database instead, which is an intentional but undiscussed departure from the team-wide convention.
If the intent is to permanently migrate to DB-driven margin, the custom instruction rules and loadMarginConfig() path should be updated in the same PR so no future author is misled. If this is a product-specific override on top of the env-var baseline, the env-var path should still be respected as a fallback.
Rule Used: WOPR codebase conventions:
- Repository pattern is... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/server/mount-routes.ts
Line: 103-108
Comment:
**Margin config convention violated — must use `MARGIN_CONFIG_JSON` env var**
Every WOPR codebase convention rule explicitly states: *"Margin config is stored in `MARGIN_CONFIG_JSON` env var, never in source"* (with `loadMarginConfig()` in `src/monetization/adapters/margin-config.ts` as the canonical loader). This PR moves the margin to `product_billing_config.margin_config.default` in the database instead, which is an intentional but undiscussed departure from the team-wide convention.
If the intent is to permanently migrate to DB-driven margin, the custom instruction rules and `loadMarginConfig()` path should be updated in the same PR so no future author is misled. If this is a product-specific override on top of the env-var baseline, the env-var path should still be respected as a fallback.
**Rule Used:** WOPR codebase conventions:
- Repository pattern is... ([source](https://app.greptile.com/review/custom-context?memory=b986c674-3c9f-4f81-9c16-19366bd8a16f))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| meter: import("../metering/emitter.js").MeterEmitter; | ||
| budgetChecker: import("../monetization/budget/budget-checker.js").IBudgetChecker; |
There was a problem hiding this comment.
meter typed as concrete class instead of its interface
IMeterEmitter is exported from src/metering/index.ts and matches the convention that sub-container fields take repository/emitter interfaces, not implementations. budgetChecker correctly uses IBudgetChecker; meter should be consistent:
| meter: import("../metering/emitter.js").MeterEmitter; | |
| budgetChecker: import("../monetization/budget/budget-checker.js").IBudgetChecker; | |
| meter: import("../metering/emitter.js").IMeterEmitter; | |
| budgetChecker: import("../monetization/budget/budget-checker.js").IBudgetChecker; |
Rule Used: WOPR codebase conventions:
- Repository pattern is... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/server/container.ts
Line: 54-55
Comment:
**`meter` typed as concrete class instead of its interface**
`IMeterEmitter` is exported from `src/metering/index.ts` and matches the convention that sub-container fields take repository/emitter *interfaces*, not implementations. `budgetChecker` correctly uses `IBudgetChecker`; `meter` should be consistent:
```suggestion
meter: import("../metering/emitter.js").IMeterEmitter;
budgetChecker: import("../monetization/budget/budget-checker.js").IBudgetChecker;
```
**Rule Used:** WOPR codebase conventions:
- Repository pattern is... ([source](https://app.greptile.com/review/custom-context?memory=158cf84c-c9a3-4390-b104-b5df2c2423d6))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| // monorepo e2e cutover test | ||
| // hybrid dockerfile e2e | ||
| // sequential build test | ||
| // lockfile build |
There was a problem hiding this comment.
These four lines look like temporary markers left over from debugging the build pipeline and should not be committed to the public export barrel:
| // monorepo e2e cutover test | |
| // hybrid dockerfile e2e | |
| // sequential build test | |
| // lockfile build | |
| // tRPC | |
| export * from "./trpc/index.js"; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/index.ts
Line: 51-54
Comment:
**Leftover CI breadcrumb comments in public barrel**
These four lines look like temporary markers left over from debugging the build pipeline and should not be committed to the public export barrel:
```suggestion
// tRPC
export * from "./trpc/index.js";
```
How can I resolve this? If you propose a fix, please make it concise.| if (!marginConfig?.default) { | ||
| throw new Error( | ||
| "Gateway enabled but product_billing_config.margin_config.default is not set. " + | ||
| "Seed the DB: INSERT INTO product_billing_config (product_id, margin_config) VALUES ('<id>', '{\"default\": 4.0}')", | ||
| ); | ||
| } |
There was a problem hiding this comment.
0 margin
!marginConfig?.default is true when default is 0, which would throw even though 0 is a valid (if degenerate) numeric value. Prefer an explicit null/undefined check:
| if (!marginConfig?.default) { | |
| throw new Error( | |
| "Gateway enabled but product_billing_config.margin_config.default is not set. " + | |
| "Seed the DB: INSERT INTO product_billing_config (product_id, margin_config) VALUES ('<id>', '{\"default\": 4.0}')", | |
| ); | |
| } | |
| if (marginConfig?.default == null) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/server/mount-routes.ts
Line: 103-108
Comment:
**Falsy guard rejects a `0` margin**
`!marginConfig?.default` is `true` when `default` is `0`, which would throw even though `0` is a valid (if degenerate) numeric value. Prefer an explicit null/undefined check:
```suggestion
if (marginConfig?.default == null) {
```
How can I resolve this? If you propose a fix, please make it concise.
TSavo
force-pushed
the
feat/db-driven-gateway
branch
from
45979ab to
a69f3ff
Compare
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| ); | ||
| } | ||
|
|
||
| // Live margin — reads from productConfig per-request (DB-cached with TTL) |
There was a problem hiding this comment.
🟡 Medium server/mount-routes.ts:110
The resolveMargin closure reads from container.productConfig.billing?.marginConfig, which is a static snapshot captured at startup. Because the comment explicitly claims "Live margin — reads from productConfig per-request (DB-cached with TTL)", the implementation contradicts its own documentation: margin changes in the database will never be reflected during the process lifetime. Consider reading from container.productConfigService.getBySlug() instead to actually leverage the TTL-cached service.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file src/server/mount-routes.ts around line 110:
The `resolveMargin` closure reads from `container.productConfig.billing?.marginConfig`, which is a static snapshot captured at startup. Because the comment explicitly claims "Live margin — reads from productConfig per-request (DB-cached with TTL)", the implementation contradicts its own documentation: margin changes in the database will never be reflected during the process lifetime. Consider reading from `container.productConfigService.getBySlug()` instead to actually leverage the TTL-cached service.
Evidence trail:
src/server/mount-routes.ts lines 110-115 (REVIEWED_COMMIT): comment claims 'Live margin — reads from productConfig per-request (DB-cached with TTL)' but resolveMargin reads `container.productConfig.billing?.marginConfig`. src/product-config/boot.ts lines 39-45: platformBoot returns `{ service, config }` where config is `await service.getBySlug(slug)` - a one-time fetch. src/server/container.ts line 144: `const { config: productConfig, service: productConfigService } = await platformBoot(...)`. src/product-config/service.ts lines 37-49: ProductConfigService.getBySlug() has TTL-cached DB reads (default 60s). container.productConfig is static; container.productConfigService.getBySlug() is TTL-cached.
| const marginConfig = billingConfig?.marginConfig as { default?: number } | null; | ||
| if (!marginConfig?.default) { |
There was a problem hiding this comment.
🟢 Low server/mount-routes.ts:102
The check !marginConfig?.default at line 103 treats a margin of 0 as "not set" and throws an error, even though 0 is a valid configuration value (e.g., for testing or promotional pricing). Consider using typeof marginConfig?.default !== 'number' to properly distinguish between unset and zero values.
const marginConfig = billingConfig?.marginConfig as { default?: number } | null;
- if (!marginConfig?.default) {
+ if (typeof marginConfig?.default !== 'number') {🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file src/server/mount-routes.ts around lines 102-103:
The check `!marginConfig?.default` at line 103 treats a margin of `0` as "not set" and throws an error, even though `0` is a valid configuration value (e.g., for testing or promotional pricing). Consider using `typeof marginConfig?.default !== 'number'` to properly distinguish between unset and zero values.
Evidence trail:
src/server/mount-routes.ts lines 102-107 at REVIEWED_COMMIT: The check `if (!marginConfig?.default)` on line 103 uses JavaScript falsy check which treats 0 as falsy, causing a valid margin value of 0 to trigger the error. The type annotation `{ default?: number }` on line 102 confirms that `default` is expected to be a number, and 0 is a valid number value.
|
🎉 This PR is included in version 1.75.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
Summary
product_billing_config.margin_config.defaultresolveMargingetter for runtime DB changes without restartTest plan
🤖 Generated with Claude Code
Note
Mount DB-driven gateway routes with live margin resolution in core server
meter(MeterEmitter) andbudgetChecker(DrizzleBudgetChecker) toPlatformContainer.gatewayin container.ts, initialized with Drizzle-backed repositories and cache/flush intervals.container.gatewayis present, wiring in live margin resolution viaresolveMargin()that readsproduct_billing_config.margin_config.defaultper request.DEFAULT_MARGINtoTEST_ONLY_MARGINin proxy.ts and addsresolveMargin?: () => numbertoGatewayConfigso production always uses a live resolver instead of the static fallback.mountRoutescall.product_billing_config.margin_config.defaultis missing when the gateway feature is enabled.📊 Macroscope summarized 45979ab. 1 file reviewed, 1 issue evaluated, 0 issues filtered, 1 comment posted
🗂️ Filtered Issues
Summary by CodeRabbit
New Features
Documentation
Tests