feat: content filter routing - #1936
Conversation
WalkthroughAdds gateway content-filter–aware routing: gateway evaluates content-filter responses early, marks providers with Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Gateway
participant ContentFilter
participant ProviderSelector
participant Provider
participant Logger
Client->>Gateway: send chat request
Gateway->>ContentFilter: evaluate request
ContentFilter-->>Gateway: content-filter result
alt content-filter matched
Gateway->>Gateway: identify providers with contentFilter=true
Gateway->>ProviderSelector: request available providers excluding content-filter providers
ProviderSelector->>ProviderSelector: score candidates (mark excluded providers)
ProviderSelector-->>Gateway: preferred provider + excluded list
Gateway->>Provider: forward request to preferred provider
Provider-->>Gateway: response
Gateway->>Logger: log routingMetadata (contentFilterMatched=true, contentFilterRerouted=true, contentFilterExcludedProviders)
else no content-filter match
Gateway->>ProviderSelector: request available providers (all eligible)
ProviderSelector-->>Gateway: selected provider
Gateway->>Provider: forward request
Provider-->>Gateway: response
Gateway->>Logger: log routingMetadata (contentFilterMatched=false)
end
Gateway-->>Client: return response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 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 docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
Adds gateway routing behavior and observability around “content filter” matches, enabling rerouting away from providers marked as content-filtering when alternatives exist, and introduces tooling/docs to analyze moderation signals offline.
Changes:
- Add
contentFilterflag to provider catalog and implement content-filter-aware provider selection + routing metadata in the gateway. - Extend routing metadata schema/types (DB JSON, API zod, generated
v1.d.ts) and update UI log views to surface content-filter exclusions. - Add a scripts package CLI (
analyze-content-filter) and commit a findings report.
Reviewed changes
Copilot reviewed 13 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/scripts/src/analyze-content-filter.ts | New CLI script to analyze moderation payloads vs content_filter finish reason and sweep thresholds. |
| packages/scripts/package.json | Adds analyze-content-filter script entry. |
| packages/models/src/providers.ts | Adds contentFilter?: boolean to ProviderDefinition; marks obsidian as contentFilter: true. |
| packages/db/src/schema.ts | Extends routingMetadata JSON typing for log and videoJob with content-filter routing fields. |
| packages/actions/src/get-cheapest-from-available-providers.ts | Extends RoutingMetadata interface to include content-filter routing fields. |
| findings.md | Adds documented results from running the analyzer on local datasets. |
| apps/gateway/src/chat/chat.ts | Implements content-filter routing decision and injects routing metadata for excluded providers. |
| apps/gateway/src/fallback.spec.ts | Adds integration tests for reroute vs monitor mode behavior; refactors fixture setup/reset. |
| apps/gateway/src/lib/costs.spec.ts | Updates discount test expectations to match seeded global OpenAI discount and uses azure as “no-discount” provider. |
| apps/api/src/routes/logs.ts | Extends Zod schema for routing metadata fields returned by logs endpoints. |
| ee/admin/src/lib/api/v1.d.ts | Updates generated API types for new routing metadata fields. |
| apps/ui/src/lib/api/v1.d.ts | Updates generated API types for new routing metadata fields. |
| apps/playground/src/lib/api/v1.d.ts | Updates generated API types for new routing metadata fields. |
| apps/code/src/lib/api/v1.d.ts | Updates generated API types for new routing metadata fields. |
| ee/admin/src/components/log-card.tsx | Displays content-filter exclusion badges and makes provider score keys region-safe. |
| apps/ui/src/components/dashboard/log-card.tsx | Displays content-filter exclusion badge in provider score list. |
| apps/ui/src/app/dashboard/[orgId]/[projectId]/activity/[logId]/log-detail-client.tsx | Displays content-filter exclusion badge in log detail provider score list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function parseGatewayResponses(value: string): ModerationResponse[] | null { | ||
| const trimmed = value.trim(); | ||
| if (trimmed.length === 0 || trimmed === "null") { | ||
| return null; | ||
| } | ||
|
|
||
| try { | ||
| const parsed = JSON.parse(trimmed) as unknown; | ||
| if (Array.isArray(parsed)) { | ||
| return parsed as ModerationResponse[]; | ||
| } | ||
|
|
||
| if (typeof parsed === "string") { | ||
| const nested = JSON.parse(parsed) as unknown; | ||
| if (Array.isArray(nested)) { | ||
| return nested as ModerationResponse[]; | ||
| } | ||
| } | ||
| } catch { | ||
| return null; | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
parseGatewayResponses() only returns an array when the parsed JSON is already an array (or a JSON-encoded string containing an array). The header comment and expected CSV format say the field may be a single moderation payload object as well; in that case this function returns null and the row is treated as having no payload (breaking tuning/inspection). Consider accepting a single object by wrapping it in an array (and similarly handling a stringified object).
| File: | ||
|
|
||
| ```text | ||
| /Users/steebchen/projects/llmgateway/llmgateway/contentfiltered.csv |
There was a problem hiding this comment.
This doc includes absolute local file paths (e.g. /Users/...); that’s machine/user-specific and can leak local usernames. Suggest replacing with a repo-relative example path or a placeholder (e.g. /abs/path/to/contentfiltered.csv) and keeping any personal paths out of committed docs.
| /Users/steebchen/projects/llmgateway/llmgateway/contentfiltered.csv | |
| /abs/path/to/contentfiltered.csv |
| price: getProviderSelectionPrice(provider), | ||
| contentFilterProvider: true, | ||
| excludedByContentFilter: true, | ||
| }; | ||
| }), |
There was a problem hiding this comment.
contentFilterProvider is only set on the synthetic score entries created for excludedProviders here. The existing routingMetadata.providerScores entries (including the selected provider) are never annotated, so logs/UI can’t tell which scored providers are content-filter providers unless they were excluded. Consider mapping over providerScores to set contentFilterProvider based on getProviderDefinition(providerId)?.contentFilter, and separately marking excludedByContentFilter when applicable.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/routes/logs.ts (1)
111-151:⚠️ Potential issue | 🟠 MajorKeep
routingMetadataAPI schema fully aligned with DB shape.The updated schema adds content-filter fields, but it still omits existing routing metadata fields (
originalProvider,originalProviderUptime,originalProviderRateLimited,noFallback) that are present in the DB type. This creates API contract drift for OpenAPI/typed consumers.🔧 Proposed fix
contentFilterMatched: z.boolean().optional(), contentFilterRerouted: z.boolean().optional(), contentFilterExcludedProviders: z.array(z.string()).optional(), + originalProvider: z.string().optional(), + originalProviderUptime: z.number().optional(), + originalProviderRateLimited: z.boolean().optional(), + noFallback: z.boolean().optional(), routing: z🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/logs.ts` around lines 111 - 151, The routingMetadata Zod schema is missing DB fields causing API drift; update the routingMetadata object (the schema defined as routingMetadata: z.object({...})) to include the missing fields from the DB: add originalProvider (z.string().optional()), originalProviderUptime (z.number().optional()), originalProviderRateLimited (z.boolean().optional()), and noFallback (z.boolean().optional()) with the same optionality and basic types as in the DB type so the API shape matches persisted routing metadata.
🧹 Nitpick comments (2)
ee/admin/src/components/log-card.tsx (1)
38-68: LocalRoutingMetadatatype is missing fields from the authoritative interface.The relevant code snippet shows that
packages/actions/src/get-cheapest-from-available-providers.tsexports aRoutingMetadatainterface with additional fields not present in this local copy:
selectedProvider(required string)originalProvider(optional string)originalProviderUptime(optional number)originalProviderRateLimited(optional boolean)noFallback(optional boolean)While making all fields optional is sensible for UI display of persisted data, the type should include all fields to avoid future maintenance issues when the UI needs to display them.
♻️ Suggested additions to the local interface
interface RoutingMetadata { + selectedProvider?: string; selectionReason?: string; availableProviders?: string[]; providerScores?: Array<{ // ... existing fields ... }>; + originalProvider?: string; + originalProviderUptime?: number; + originalProviderRateLimited?: boolean; + noFallback?: boolean; contentFilterMatched?: boolean; contentFilterRerouted?: boolean; contentFilterExcludedProviders?: string[]; routing?: Array<{ // ... existing fields ... }>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/admin/src/components/log-card.tsx` around lines 38 - 68, The local RoutingMetadata interface is missing fields present in the authoritative interface (packages/actions/src/get-cheapest-from-available-providers.ts); update the local interface (RoutingMetadata) to include selectedProvider: string, originalProvider?: string, originalProviderUptime?: number, originalProviderRateLimited?: boolean, and noFallback?: boolean (make them optional except selectedProvider if the authoritative type requires it, or make all fields optional for UI safety) so the UI type matches the source-of-truth and avoids future mismatches.packages/scripts/src/analyze-content-filter.ts (1)
381-404: JSON parsing silently ignores single moderation response objects.If
gateway_content_filter_responsecontains a singleModerationResponseobject (not wrapped in an array), this function returnsnullrather than wrapping it in an array. This could cause valid data to be silently ignored.♻️ Handle single object responses
function parseGatewayResponses(value: string): ModerationResponse[] | null { const trimmed = value.trim(); if (trimmed.length === 0 || trimmed === "null") { return null; } try { const parsed = JSON.parse(trimmed) as unknown; if (Array.isArray(parsed)) { return parsed as ModerationResponse[]; } if (typeof parsed === "string") { const nested = JSON.parse(parsed) as unknown; if (Array.isArray(nested)) { return nested as ModerationResponse[]; } + if (typeof nested === "object" && nested !== null) { + return [nested as ModerationResponse]; + } } + + if (typeof parsed === "object" && parsed !== null) { + return [parsed as ModerationResponse]; + } } catch { return null; } return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/scripts/src/analyze-content-filter.ts` around lines 381 - 404, The parseGatewayResponses function currently returns null when the parsed JSON is a single ModerationResponse object; update parseGatewayResponses to detect when parsed (or nested parsed from a string) is an object matching a single ModerationResponse (i.e., typeof parsed === "object" && parsed !== null and not an array) and return [parsed as ModerationResponse]; do the same for the nested variable path so single-object strings are also wrapped into an array; keep existing array handling and catch behavior intact and ensure the return type remains ModerationResponse[] | null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 1961-1969: Compute getContentFilterRoutingDecision (using
availableModelProviders and shouldRerouteContentFilter) before the implicit
multi-provider "auto" selection and use its candidates/excludedProviders to
influence the auto-router's provider choice; specifically, apply
contentFilterRoutingDecision.excludedProviders to filter availableModelProviders
and prefer contentFilterRoutingDecision.candidates when determining usedProvider
in the auto-routing code path, and ensure contentFilterRoutingApplied is set
from the decision so later logic sees the exclusion.
- Around line 1528-1530: The gating for applying the gateway content filter
currently uses requestedModel, which can be the raw token and bypasses or
mis-evaluates auto-routed selections; update the shouldApplyGatewayContentFilter
condition to use the resolved model id from modelInfo (e.g., modelInfo.id)
instead of requestedModel so shouldApplyContentFilterToModel(...) is invoked
against the concrete model chosen by the auto-router (ensure modelInfo is
defined/available in scope before calling).
- Around line 2629-2631: The current block decision uses contentFilterMode ===
"enabled" && contentFilterMatched && !contentFilterRoutingApplied which treats
"no implicit reroute occurred" as equivalent to "request must be blocked";
instead, tie blocking to the actual selected provider or to the lack of any
non-filter candidate. Update the logic so that you only mark the request blocked
when contentFilterMode === "enabled" && contentFilterMatched && (the
chosen/selected provider is a content-filter provider OR there were zero
non-content-filter candidate providers), rather than relying on
!contentFilterRoutingApplied; use the actual selected provider identifier (e.g.,
selectedProvider or chosenProvider) or inspect the candidateProviders list to
determine if any non-filter provider existed, and ensure explicit-provider and
single-provider flows are allowed when they resolved to a non-contentFilter
provider.
In `@apps/gateway/src/lib/costs.spec.ts`:
- Around line 136-148: The two tests call calculateCosts("gpt-4", "openai", 100,
50, null) but expect different results; fix by making expectations consistent
with seeded global 10% OpenAI discount: update the earlier test that expects
inputCost 0.001, outputCost 0.0015, totalCost 0.0025 to expect discounted values
(inputCost 0.0009, outputCost 0.00135, totalCost 0.00225 and discount 0.1) OR
modify the test setup to isolate/remove the global discount (e.g., stub
getEffectiveDiscount or set organizationId so getEffectiveDiscount returns 0) so
both calculateCosts calls produce consistent results.
In `@findings.md`:
- Around line 24-28: The file contains a workstation-specific absolute path
"/Users/steebchen/projects/llmgateway/llmgateway/contentfiltered.csv" (and
similar entries referenced later) that leaks a local username and is not
portable; replace these with a repo-relative or placeholder path such as
"./content/contentfiltered.csv" or "<REPO_ROOT>/contentfiltered.csv" throughout
the document so the example works on any machine and does not expose local user
information, updating every occurrence of the absolute path accordingly.
---
Outside diff comments:
In `@apps/api/src/routes/logs.ts`:
- Around line 111-151: The routingMetadata Zod schema is missing DB fields
causing API drift; update the routingMetadata object (the schema defined as
routingMetadata: z.object({...})) to include the missing fields from the DB: add
originalProvider (z.string().optional()), originalProviderUptime
(z.number().optional()), originalProviderRateLimited (z.boolean().optional()),
and noFallback (z.boolean().optional()) with the same optionality and basic
types as in the DB type so the API shape matches persisted routing metadata.
---
Nitpick comments:
In `@ee/admin/src/components/log-card.tsx`:
- Around line 38-68: The local RoutingMetadata interface is missing fields
present in the authoritative interface
(packages/actions/src/get-cheapest-from-available-providers.ts); update the
local interface (RoutingMetadata) to include selectedProvider: string,
originalProvider?: string, originalProviderUptime?: number,
originalProviderRateLimited?: boolean, and noFallback?: boolean (make them
optional except selectedProvider if the authoritative type requires it, or make
all fields optional for UI safety) so the UI type matches the source-of-truth
and avoids future mismatches.
In `@packages/scripts/src/analyze-content-filter.ts`:
- Around line 381-404: The parseGatewayResponses function currently returns null
when the parsed JSON is a single ModerationResponse object; update
parseGatewayResponses to detect when parsed (or nested parsed from a string) is
an object matching a single ModerationResponse (i.e., typeof parsed === "object"
&& parsed !== null and not an array) and return [parsed as ModerationResponse];
do the same for the nested variable path so single-object strings are also
wrapped into an array; keep existing array handling and catch behavior intact
and ensure the return type remains ModerationResponse[] | null.
🪄 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: 5eb01cb0-22d6-48b7-94b2-9ec08ea3bc5c
⛔ Files ignored due to path filters (4)
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.ts
📒 Files selected for processing (13)
apps/api/src/routes/logs.tsapps/gateway/src/chat/chat.tsapps/gateway/src/fallback.spec.tsapps/gateway/src/lib/costs.spec.tsapps/ui/src/app/dashboard/[orgId]/[projectId]/activity/[logId]/log-detail-client.tsxapps/ui/src/components/dashboard/log-card.tsxee/admin/src/components/log-card.tsxfindings.mdpackages/actions/src/get-cheapest-from-available-providers.tspackages/db/src/schema.tspackages/models/src/providers.tspackages/scripts/package.jsonpackages/scripts/src/analyze-content-filter.ts
| const shouldApplyGatewayContentFilter = | ||
| contentFilterMode !== "disabled" && | ||
| shouldApplyContentFilterToModel(requestedModel); |
There was a problem hiding this comment.
Use the resolved model for gateway filter gating.
At this point modelInfo can already hold the concrete model chosen by the auto-router, but this condition still keys off requestedModel (the raw input token). That means auto requests can skip the content-filter match/reroute path entirely, or be evaluated against the wrong model. Gate shouldApplyContentFilterToModel(...) off modelInfo.id here instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/src/chat/chat.ts` around lines 1528 - 1530, The gating for
applying the gateway content filter currently uses requestedModel, which can be
the raw token and bypasses or mis-evaluates auto-routed selections; update the
shouldApplyGatewayContentFilter condition to use the resolved model id from
modelInfo (e.g., modelInfo.id) instead of requestedModel so
shouldApplyContentFilterToModel(...) is invoked against the concrete model
chosen by the auto-router (ensure modelInfo is defined/available in scope before
calling).
| const contentFilterRoutingDecision = getContentFilterRoutingDecision( | ||
| availableModelProviders, | ||
| shouldRerouteContentFilter, | ||
| ); | ||
| const contentFilterPreferredProviders = | ||
| contentFilterRoutingDecision.candidates; | ||
| contentFilterRoutingExcludedProviders = | ||
| contentFilterRoutingDecision.excludedProviders; | ||
| contentFilterRoutingApplied = contentFilterRoutingDecision.rerouted; |
There was a problem hiding this comment.
Apply the exclusion before provider selection is finalized.
This reroute only runs in the implicit multi-provider branch. auto can already pin usedProvider above, so a matched auto request can still land on a contentFilter provider even when a non-filter alternative exists. The same preferred/excluded candidate split needs to participate in the auto-router too, not just here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/src/chat/chat.ts` around lines 1961 - 1969, Compute
getContentFilterRoutingDecision (using availableModelProviders and
shouldRerouteContentFilter) before the implicit multi-provider "auto" selection
and use its candidates/excludedProviders to influence the auto-router's provider
choice; specifically, apply contentFilterRoutingDecision.excludedProviders to
filter availableModelProviders and prefer
contentFilterRoutingDecision.candidates when determining usedProvider in the
auto-routing code path, and ensure contentFilterRoutingApplied is set from the
decision so later logic sees the exclusion.
| contentFilterMode === "enabled" && | ||
| contentFilterMatched && | ||
| !contentFilterRoutingApplied; |
There was a problem hiding this comment.
Don't equate “no reroute happened” with “this request must be blocked.”
contentFilterRoutingApplied is only set by the implicit reroute path, so this will also block explicit-provider and single-provider requests that ended up on a non-contentFilter provider. Blocking needs to be tied to the selected provider (or to the fact that no non-filter candidate existed), not just !contentFilterRoutingApplied.
Minimal safeguard
const contentFilterBlocked =
contentFilterMode === "enabled" &&
contentFilterMatched &&
- !contentFilterRoutingApplied;
+ !contentFilterRoutingApplied &&
+ isContentFilterProvider(usedProvider);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/src/chat/chat.ts` around lines 2629 - 2631, The current block
decision uses contentFilterMode === "enabled" && contentFilterMatched &&
!contentFilterRoutingApplied which treats "no implicit reroute occurred" as
equivalent to "request must be blocked"; instead, tie blocking to the actual
selected provider or to the lack of any non-filter candidate. Update the logic
so that you only mark the request blocked when contentFilterMode === "enabled"
&& contentFilterMatched && (the chosen/selected provider is a content-filter
provider OR there were zero non-content-filter candidate providers), rather than
relying on !contentFilterRoutingApplied; use the actual selected provider
identifier (e.g., selectedProvider or chosenProvider) or inspect the
candidateProviders list to determine if any non-filter provider existed, and
ensure explicit-provider and single-provider flows are allowed when they
resolved to a non-contentFilter provider.
| File: | ||
|
|
||
| ```text | ||
| /Users/steebchen/projects/llmgateway/llmgateway/contentfiltered.csv | ||
| ``` |
There was a problem hiding this comment.
Replace workstation-specific paths with portable examples.
These absolute paths only work on one machine and leak a local username into the repo. Please switch them to a repo-relative example or a placeholder path so the document stays portable.
Also applies to: 133-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@findings.md` around lines 24 - 28, The file contains a workstation-specific
absolute path
"/Users/steebchen/projects/llmgateway/llmgateway/contentfiltered.csv" (and
similar entries referenced later) that leaks a local username and is not
portable; replace these with a repo-relative or placeholder path such as
"./content/contentfiltered.csv" or "<REPO_ROOT>/contentfiltered.csv" throughout
the document so the example works on any machine and does not expose local user
information, updating every occurrence of the absolute path accordingly.
![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: 73d4adbf65
ℹ️ 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".
| const openAIContentFilterResult = | ||
| shouldApplyGatewayContentFilter && contentFilterMethod === "openai" | ||
| ? await checkOpenAIContentFilter( |
There was a problem hiding this comment.
This now executes checkOpenAIContentFilter(...) before downstream hard-fail checks (for example provider RPM 429 with x-no-fallback, insufficient retention credits, or missing token), so prompts can be sent to OpenAI moderation and billed even when the request is guaranteed to be rejected locally. In the previous flow moderation happened later, after those gates. This is a behavioral regression in cost/privacy for LLM_CONTENT_FILTER_METHOD=openai and can be avoided by moving moderation evaluation behind non-recoverable rejection paths.
Useful? React with 👍 / 👎.
| const contentFilterRoutingDecision = getContentFilterRoutingDecision( | ||
| availableModelProviders, | ||
| shouldRerouteContentFilter, | ||
| ); |
There was a problem hiding this comment.
Content-filter rerouting is only computed in the auto-routing branch, so requests with an explicitly selected provider never get rerouted away from contentFilter providers even when alternates exist and fallback is otherwise allowed. In enabled mode those requests remain blocked (contentFilterRoutingApplied stays false), which makes the new reroute behavior unavailable for direct-provider requests despite existing fallback behavior for other failure modes (rate-limit/uptime).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/gateway/src/fallback.spec.ts (2)
227-239:⚠️ Potential issue | 🟠 MajorScope the “root” metrics update to the root mapping only.
When
options.regionis omitted, the update on Line 239 only filters bymodelId+providerId, so a write intended for the root mapping also rewrites every regional row for that provider. That makes this helper unable to seed root and regional scores independently, and it amplifies the stale-fixture problem above. Add an explicit null-region condition in the no-region branch so root stats do not bleed intocn-beijing/singaporerows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/fallback.spec.ts` around lines 227 - 239, The update is currently scoped only by modelId + providerId when options.region is omitted, causing root metrics to overwrite regional rows; modify the branch where options?.region is falsy to push a null-region condition (e.g. push(isNull(tables.modelProviderMapping.region)) or push(eq(tables.modelProviderMapping.region, null))) into the conditions array before calling db.update(...).where(and(...conditions)) so the update only targets the root mapping row and does not bleed into regional rows.
89-118:⚠️ Potential issue | 🟠 MajorClean up the custom model mappings between tests.
Line 92 clears routing stats, but
resetTestState()still leaves the test-createdmodel/modelProviderMappingrows behind. That is risky here because the earlierglm-4.6cases insert analibaba/cn-beijingmapping withoutmaxOutput, and the latermax_tokenscase inserts anotheralibaba/cn-beijingrow withmaxOutput: 16384. If both rows survive, the later test can still route through the stale unbounded mapping and pass for the wrong reason. Please delete those per-test fixtures or make each case use unique model IDs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/fallback.spec.ts` around lines 89 - 118, The resetTestState() helper currently clears routing stats but does not remove test-created model or mapping rows; update resetTestState() to also delete entries from tables.modelProviderMapping and the models table (e.g., tables.model) so per-test model and modelProviderMapping fixtures are removed between tests and avoid stale mappings (refer to resetTestState(), tables.modelProviderMapping and the models table name used in your schema).
🧹 Nitpick comments (2)
apps/worker/src/index.ts (1)
54-61: Path comparison may fail with relative invocations.
fileURLToPath(import.meta.url)returns an absolute path, butprocess.argv[1]can be relative (e.g.,node dist/index.js). The comparison would then fail, preventing the worker from starting.♻️ Proposed fix to normalize paths
-import { fileURLToPath } from "node:url"; +import { resolve } from "node:path"; +import { fileURLToPath } from "node:url";function isDirectExecution() { const entrypoint = process.argv[1]; if (!entrypoint) { return false; } - return fileURLToPath(import.meta.url) === entrypoint; + return fileURLToPath(import.meta.url) === resolve(entrypoint); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/src/index.ts` around lines 54 - 61, The current isDirectExecution comparison can fail when process.argv[1] is relative; change isDirectExecution to normalize entrypoint to an absolute, normalized path before comparing to fileURLToPath(import.meta.url). Specifically, in isDirectExecution, take the raw entrypoint (process.argv[1]) and resolve it against process.cwd() (e.g., via path.resolve or path.join + path.normalize) so that the comparison uses two absolute, normalized paths; keep using fileURLToPath(import.meta.url) for the other side and return the equality check. Ensure you reference the isDirectExecution function, process.argv[1], and fileURLToPath(import.meta.url) when making the change.apps/gateway/src/fallback.spec.ts (1)
1502-1519: Isolate the provider/env overrides behind a helper.
packages/models/src/providers.tsLines 560-564 returns the live registry entry, sotogetherProvider.contentFilter = trueis mutating shared module state here, and each test also patches severalprocess.envkeys. Thefinallyblocks help, but this is still fragile and duplicates a lot of cleanup code. A small helper that snapshots/restores both the provider flag and env vars would make these cases safer and easier to maintain.Also applies to: 1592-1609
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/fallback.spec.ts` around lines 1502 - 1519, Extract the provider/env override logic into a helper that snapshots and restores the provider flag and the specific env vars instead of mutating shared state inline: create a function (e.g., withProviderEnvOverride or snapshotAndRestoreProviderEnv) that accepts a providerName (used with getProviderDefinition) and an overrides object ({ contentFilter: boolean, env: { LLM_CONTENT_FILTER_MODE, LLM_CONTENT_FILTER_METHOD, LLM_CONTENT_FILTER_MODELS, LLM_CONTENT_FILTER_KEYWORDS } }), inside the helper clone or copy the provider entry returned by getProviderDefinition("together.ai") (or snapshot its contentFilter), save the current process.env values for the listed keys, apply the overrides (set provider.contentFilter and process.env values), run the test callback, and in a finally block restore the provider.contentFilter and the saved process.env values; replace the inline mutation of togetherProvider.contentFilter and manual env saves/restores with calls to this helper for both occurrences (around lines where getProviderDefinition, togetherProvider.contentFilter, and the LLM_CONTENT_FILTER_* env keys are used).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/gateway/src/fallback.spec.ts`:
- Around line 227-239: The update is currently scoped only by modelId +
providerId when options.region is omitted, causing root metrics to overwrite
regional rows; modify the branch where options?.region is falsy to push a
null-region condition (e.g. push(isNull(tables.modelProviderMapping.region)) or
push(eq(tables.modelProviderMapping.region, null))) into the conditions array
before calling db.update(...).where(and(...conditions)) so the update only
targets the root mapping row and does not bleed into regional rows.
- Around line 89-118: The resetTestState() helper currently clears routing stats
but does not remove test-created model or mapping rows; update resetTestState()
to also delete entries from tables.modelProviderMapping and the models table
(e.g., tables.model) so per-test model and modelProviderMapping fixtures are
removed between tests and avoid stale mappings (refer to resetTestState(),
tables.modelProviderMapping and the models table name used in your schema).
---
Nitpick comments:
In `@apps/gateway/src/fallback.spec.ts`:
- Around line 1502-1519: Extract the provider/env override logic into a helper
that snapshots and restores the provider flag and the specific env vars instead
of mutating shared state inline: create a function (e.g.,
withProviderEnvOverride or snapshotAndRestoreProviderEnv) that accepts a
providerName (used with getProviderDefinition) and an overrides object ({
contentFilter: boolean, env: { LLM_CONTENT_FILTER_MODE,
LLM_CONTENT_FILTER_METHOD, LLM_CONTENT_FILTER_MODELS,
LLM_CONTENT_FILTER_KEYWORDS } }), inside the helper clone or copy the provider
entry returned by getProviderDefinition("together.ai") (or snapshot its
contentFilter), save the current process.env values for the listed keys, apply
the overrides (set provider.contentFilter and process.env values), run the test
callback, and in a finally block restore the provider.contentFilter and the
saved process.env values; replace the inline mutation of
togetherProvider.contentFilter and manual env saves/restores with calls to this
helper for both occurrences (around lines where getProviderDefinition,
togetherProvider.contentFilter, and the LLM_CONTENT_FILTER_* env keys are used).
In `@apps/worker/src/index.ts`:
- Around line 54-61: The current isDirectExecution comparison can fail when
process.argv[1] is relative; change isDirectExecution to normalize entrypoint to
an absolute, normalized path before comparing to fileURLToPath(import.meta.url).
Specifically, in isDirectExecution, take the raw entrypoint (process.argv[1])
and resolve it against process.cwd() (e.g., via path.resolve or path.join +
path.normalize) so that the comparison uses two absolute, normalized paths; keep
using fileURLToPath(import.meta.url) for the other side and return the equality
check. Ensure you reference the isDirectExecution function, process.argv[1], and
fileURLToPath(import.meta.url) when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 35c1e080-8201-470b-920a-a2c34cdf2e65
📒 Files selected for processing (3)
apps/gateway/src/fallback.spec.tsapps/gateway/src/lib/costs.spec.tsapps/worker/src/index.ts
✅ Files skipped from review due to trivial changes (1)
- apps/gateway/src/lib/costs.spec.ts
Summary by CodeRabbit
New Features
UI
Tests
Documentation