fix: avoid filtered providers on moderation failure#1985
fix: avoid filtered providers on moderation failure#1985
Conversation
|
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:
WalkthroughDetects OpenAI moderation outages and propagates that state into content-filter routing: providers relying on provider-side filtering can be excluded via Changes
Sequence DiagramsequenceDiagram
participant Client
participant Gateway as Gateway (chat.ts)
participant Moderation as OpenAI Moderation
participant Selector as Provider Selector
participant Upstream as Upstream Provider
Client->>Gateway: Send chat request
Gateway->>Moderation: Request moderation check
alt Moderation unavailable
Moderation-->>Gateway: { unavailable: true }
Gateway->>Gateway: set contentFilterUnavailable, mark providerScores.excludedByModerationFailure
Gateway->>Selector: Request provider list (exclude provider-side filters)
alt Alternative providers exist
Selector-->>Gateway: choose non-filter provider
Gateway->>Upstream: Route request
Upstream-->>Gateway: Response
Gateway-->>Client: 200 + routing metadata (contentFilterUnavailable, excluded providers)
else No alternatives
Gateway->>Gateway: persist moderation outage block log (503)
Gateway-->>Client: 503 Service Unavailable
end
else Moderation available
Moderation-->>Gateway: { flagged: true/false }
Gateway->>Selector: Provide provider scores (mark excludedByContentFilter if flagged)
Selector-->>Gateway: chosen provider
Gateway->>Upstream: Route request
Upstream-->>Gateway: Response
Gateway-->>Client: Response + routing metadata
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates gateway routing to treat OpenAI moderation outages as a signal to avoid providers marked as contentFilter (provider-side filtering), rerouting to other eligible providers when possible and returning a 503 when no non-content-filter provider is available. It also surfaces this decision in routing metadata, updates the shared log UI to display it, and adds regression coverage for reroute vs. block behavior.
Changes:
- Add moderation-unavailable signals to routing metadata (
contentFilterUnavailable,excludedByModerationFailure) and use them in routing/provider retry selection. - Update gateway routing logic to avoid
contentFilterproviders when moderation is unavailable (enabled mode), and hard-block with 503 if no alternatives exist. - Extend UI log card and tests to reflect and validate the new moderation-unavailable behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/components/log-card.tsx | Displays provider-score annotations for moderation-unavailable exclusions and adds metadata fields. |
| packages/actions/src/get-cheapest-from-available-providers.ts | Extends RoutingMetadata typings to carry moderation-unavailable exclusion fields. |
| apps/gateway/src/fallback.spec.ts | Adds regression tests for rerouting away from content-filter providers and blocking when only such providers exist. |
| apps/gateway/src/chat/tools/retry-with-fallback.ts | Ensures retry selection skips providers excluded due to moderation failure as well as content-filter matches. |
| apps/gateway/src/chat/tools/openai-content-filter.ts | Adds unavailable flag to moderation check results and sets it based on per-request success. |
| apps/gateway/src/chat/tools/openai-content-filter.spec.ts | Updates expected results to include the new unavailable field. |
| apps/gateway/src/chat/chat.ts | Implements the core routing change: avoid content-filter providers when moderation is unavailable, add routing metadata, and block with 503 when unavoidable. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const sorted = [...providerScores].sort((a, b) => a.score - b.score); | ||
| for (const score of sorted) { | ||
| if (score.excludedByContentFilter) { | ||
| if (score.excludedByContentFilter || score.excludedByModerationFailure) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
selectNextProvider() now skips scores marked excludedByModerationFailure, but there’s no accompanying unit test verifying this new exclusion behavior (the existing spec only covers excludedByContentFilter). Please add a test case to ensure providers excluded due to moderation unavailability are never selected during fallback retries.
| if (moderationRequests.length === 0) { | ||
| logModerationResult(context, { | ||
| durationMs: Date.now() - startTime, | ||
| flagged: false, | ||
| model: OPENAI_MODERATION_MODEL, | ||
| upstreamRequestId: null, | ||
| hasImages: false, | ||
| requestCount: 0, | ||
| imageRequestCount: 0, | ||
| flaggedCategories: [], | ||
| results: [], | ||
| }); | ||
|
|
||
| return createFailedOpenAIContentFilterResult(); | ||
| return createFailedOpenAIContentFilterResult(null, false); | ||
| } |
There was a problem hiding this comment.
In the moderationRequests.length === 0 path, the code returns createFailedOpenAIContentFilterResult(null, false). Since this isn’t actually a failure case, consider introducing a separate helper (or renaming the existing one) to represent a successful “no moderation needed” result, to avoid confusing semantics around unavailable vs. failure.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/gateway/src/chat/tools/openai-content-filter.ts (2)
523-537: Includeunavailablein the aggregate moderation log.Routing now branches on this flag, but the structured
gateway_content_filterevent still looks like a normal success. Emitting it here will make reroutes and 503s much easier to diagnose without correlating separate error logs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/tools/openai-content-filter.ts` around lines 523 - 537, The structured moderation log emitted by logModerationResult currently omits the aggregate "unavailable" flag, so add unavailable: moderationResults.some((r) => !r.success) to the object passed into logModerationResult (the call near logModerationResult(...) in openai-content-filter.ts) so the gateway_content_filter event includes the overall unavailable state; keep the rest of the payload unchanged and ensure the value is computed the same way as the function's return value to keep logs and routing consistent.
341-344: Split the empty-result path from the failure helper.
createFailedOpenAIContentFilterResult(null, false)is now the "nothing to moderate" case. A separate empty-result helper would make theunavailablecontract easier to read and harder to misuse later.Also applies to: 483-483
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/tools/openai-content-filter.ts` around lines 341 - 344, The helper createFailedOpenAIContentFilterResult conflates "service unavailable" and "nothing to moderate" by using upstreamRequestId=null and unavailable=false; split these concerns by keeping createFailedOpenAIContentFilterResult(upstreamRequestId: string | null, unavailable = true) solely for true failures/unavailable states and add a new helper (e.g., createEmptyOpenAIContentFilterResult() or createNoContentToModerateResult()) that returns the explicit "nothing to moderate" OpenAIContentFilterCheckResult; update all call sites that currently call createFailedOpenAIContentFilterResult(null, false) to call the new empty-result helper, and ensure any callers that represent real failures continue to use createFailedOpenAIContentFilterResult with unavailable=true so the contract is clear and not misused.apps/gateway/src/fallback.spec.ts (1)
1764-1854: Assert the persisted log on the 503 branch too.The reroute case verifies
contentFilterUnavailable/excludedByModerationFailurein the DB log, but the no-eligible-provider case only checks the HTTP response. Adding the log assertion here would protect the log-card path as well.🤖 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 1764 - 1854, In the "openai moderation failure blocks when only content-filter providers are available" test, after asserting the 503 response and body, also fetch the persisted request log from the DB and assert it contains the contentFilterUnavailable and excludedByModerationFailure flags (the same fields checked in the reroute test) so the "log-card" path is covered; locate this test (the async function that calls app.request and uses togetherProvider/fetchSpy) and add a DB lookup for the created request log (using your existing test helper used by other tests to read stored logs) and assert the persisted log includes contentFilterUnavailable: true and excludedByModerationFailure: true before restoring fetchSpy and environment toggles.packages/shared/src/components/log-card.tsx (1)
50-85: Avoid maintaining a secondRoutingMetadatacontract here.This local copy already had to be manually updated for the moderation-outage fields. Moving the type into a shared DTO/module would remove the drift risk between
packages/actions/src/get-cheapest-from-available-providers.tsand this component.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/components/log-card.tsx` around lines 50 - 85, The RoutingMetadata interface in this component duplicates the DTO used elsewhere and causes drift; remove the local RoutingMetadata declaration and instead import the shared type exported from the common DTO/module used by get-cheapest-from-available-providers.ts (or its defining module), update all references in this file (e.g., props or variables typed as RoutingMetadata) to use the imported type, and ensure the shared DTO is extended there with the moderation/outage fields so this component no longer carries a separate contract.
🤖 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 1906-1910: The moderation-outage avoidance (the logic computing
contentFilterUnavailable / shouldAvoidContentFilterProviders and calling
getContentFilterRoutingDecision()/addContentFilterRoutingMetadata()) must run
before any code sets usedProvider so auto-routing and specific-provider
fallbacks honor it; move or duplicate this check so that before usedProvider is
assigned in the auto routing and fallback paths you evaluate contentFilterMode,
contentFilterMatched, and openAIContentFilterResult?.unavailable and then call
getContentFilterRoutingDecision() and addContentFilterRoutingMetadata() as
appropriate; update the branches that set usedProvider (including the auto route
and the specific-provider fallback code paths referenced) to respect the
computed shouldAvoidContentFilterProviders value instead of running the check
later.
- Around line 2996-2999: Move the moderation-outage guard (the
contentFilterSensitiveProviderBlocked check) to execute before
checkProviderRateLimit() so we don't consume a provider slot for requests
blocked by content-moderation outage; when blocking, explicitly write the
request log (call the existing request-log insertion routine used for
successful/other-failed requests) including routing metadata and then throw the
HTTPException(503) so the blocked request appears in logs; also add a clear log
entry (logger.warn/error) describing the moderation outage and reference this
same change for the duplicate guard around the 3140-3145 area.
---
Nitpick comments:
In `@apps/gateway/src/chat/tools/openai-content-filter.ts`:
- Around line 523-537: The structured moderation log emitted by
logModerationResult currently omits the aggregate "unavailable" flag, so add
unavailable: moderationResults.some((r) => !r.success) to the object passed into
logModerationResult (the call near logModerationResult(...) in
openai-content-filter.ts) so the gateway_content_filter event includes the
overall unavailable state; keep the rest of the payload unchanged and ensure the
value is computed the same way as the function's return value to keep logs and
routing consistent.
- Around line 341-344: The helper createFailedOpenAIContentFilterResult
conflates "service unavailable" and "nothing to moderate" by using
upstreamRequestId=null and unavailable=false; split these concerns by keeping
createFailedOpenAIContentFilterResult(upstreamRequestId: string | null,
unavailable = true) solely for true failures/unavailable states and add a new
helper (e.g., createEmptyOpenAIContentFilterResult() or
createNoContentToModerateResult()) that returns the explicit "nothing to
moderate" OpenAIContentFilterCheckResult; update all call sites that currently
call createFailedOpenAIContentFilterResult(null, false) to call the new
empty-result helper, and ensure any callers that represent real failures
continue to use createFailedOpenAIContentFilterResult with unavailable=true so
the contract is clear and not misused.
In `@apps/gateway/src/fallback.spec.ts`:
- Around line 1764-1854: In the "openai moderation failure blocks when only
content-filter providers are available" test, after asserting the 503 response
and body, also fetch the persisted request log from the DB and assert it
contains the contentFilterUnavailable and excludedByModerationFailure flags (the
same fields checked in the reroute test) so the "log-card" path is covered;
locate this test (the async function that calls app.request and uses
togetherProvider/fetchSpy) and add a DB lookup for the created request log
(using your existing test helper used by other tests to read stored logs) and
assert the persisted log includes contentFilterUnavailable: true and
excludedByModerationFailure: true before restoring fetchSpy and environment
toggles.
In `@packages/shared/src/components/log-card.tsx`:
- Around line 50-85: The RoutingMetadata interface in this component duplicates
the DTO used elsewhere and causes drift; remove the local RoutingMetadata
declaration and instead import the shared type exported from the common
DTO/module used by get-cheapest-from-available-providers.ts (or its defining
module), update all references in this file (e.g., props or variables typed as
RoutingMetadata) to use the imported type, and ensure the shared DTO is extended
there with the moderation/outage fields so this component no longer carries a
separate contract.
🪄 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: 2b4f4198-8fce-45e8-9040-d71e265c5a56
📒 Files selected for processing (7)
apps/gateway/src/chat/chat.tsapps/gateway/src/chat/tools/openai-content-filter.spec.tsapps/gateway/src/chat/tools/openai-content-filter.tsapps/gateway/src/chat/tools/retry-with-fallback.tsapps/gateway/src/fallback.spec.tspackages/actions/src/get-cheapest-from-available-providers.tspackages/shared/src/components/log-card.tsx
![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: e8d62dec40
ℹ️ 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 contentFilterSensitiveProviderBlocked = | ||
| contentFilterMode === "enabled" && | ||
| contentFilterUnavailable && | ||
| isContentFilterProvider(usedProvider); |
There was a problem hiding this comment.
This new contentFilterSensitiveProviderBlocked gate can return 503 even when a non-content-filter provider is still eligible, because the requested-provider fallback paths (rate-limit/low-uptime rerouting in chat.ts) select the cheapest provider without applying getContentFilterRoutingDecision. In a moderation outage, that can pick a content-filter provider first and then hard-block here, causing avoidable outages instead of routing to a safe alternative. Please re-run content-filter-aware candidate selection (or reselect) before throwing this 503.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/gateway/src/chat/chat.ts (1)
2319-2322:⚠️ Potential issue | 🟠 MajorModeration-outage rerouting is still skipped when
usedProvideris preselected.At Line 2319,
getContentFilterRoutingDecision()only runs in theif (!usedProvider)flow. Auto-routing and specific-provider resolution can setusedProviderearlier, so moderation outages can still fall through tocontentFilterSensitiveProviderBlockedand return 503 even when a non-sensitive alternative exists.🤖 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 2319 - 2322, The code only computes contentFilterRoutingDecision via getContentFilterRoutingDecision when usedProvider is falsy, causing moderation-outage rerouting to be skipped if usedProvider was preselected; update the flow to always call getContentFilterRoutingDecision(availableModelProviders, shouldAvoidContentFilterProviders) regardless of usedProvider and then use its result to override or reroute when the provider is blocked by contentFilterSensitiveProviderBlocked (and similar checks) so a non-sensitive alternative can be selected; locate the usedProvider assignment and the contentFilterSensitiveProviderBlocked check in chat.ts and ensure contentFilterRoutingDecision is computed before those checks and applied to choose an alternative provider when the current usedProvider is impacted.
🤖 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/fallback.spec.ts`:
- Around line 1658-1764: Add assertions to ensure the sensitive upstream
(mockServerUrl) is not contacted by checking the spied fetch calls: after
sending the request in the "openai moderation failure reroutes away from
content-filter providers" test (the test function in which fetchSpy is created),
assert that fetch was called exactly once for mockServerUrl in the reroute case
(expect(fetchSpy.mock.calls.filter(c => c[0] && (typeof c[0] === "string" ? c[0]
: c[0].url) === mockServerUrl).length).toBe(1)) and in the related block-case
test assert the same filter count is 0; place these assertions after the
response checks and before fetchSpy.mockRestore(), referencing the existing
fetchSpy and mockServerUrl identifiers so the tests prove the upstream was
skipped.
---
Duplicate comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 2319-2322: The code only computes contentFilterRoutingDecision via
getContentFilterRoutingDecision when usedProvider is falsy, causing
moderation-outage rerouting to be skipped if usedProvider was preselected;
update the flow to always call
getContentFilterRoutingDecision(availableModelProviders,
shouldAvoidContentFilterProviders) regardless of usedProvider and then use its
result to override or reroute when the provider is blocked by
contentFilterSensitiveProviderBlocked (and similar checks) so a non-sensitive
alternative can be selected; locate the usedProvider assignment and the
contentFilterSensitiveProviderBlocked check in chat.ts and ensure
contentFilterRoutingDecision is computed before those checks and applied to
choose an alternative provider when the current usedProvider is impacted.
🪄 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: b01596c1-73aa-4696-aaf3-fd9f6efbeb1d
📒 Files selected for processing (2)
apps/gateway/src/chat/chat.tsapps/gateway/src/fallback.spec.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fe0c9bc98
ℹ️ 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".
| if (contentFilterSensitiveProviderBlocked) { | ||
| const moderationOutageMessage = | ||
| "OpenAI moderation is unavailable and no eligible provider without provider-side content filtering is available."; | ||
| const baseLogEntry = createLogEntry( |
There was a problem hiding this comment.
This new early-return path handles only contentFilterSensitiveProviderBlocked, but regular contentFilterBlocked requests are now evaluated later (after provider RPM consumption and retention-credit checks). In the contentFilterMatched && !contentFilterRoutingApplied case, the request can now decrement provider limits or fail with 402/429 before reaching the gateway content-filter response path, which is a behavior regression from the previous ordering and can incorrectly throttle/deny blocked prompts.
Useful? React with 👍 / 👎.
| const contentFilterSensitiveProviderBlocked = | ||
| contentFilterMode === "enabled" && | ||
| contentFilterUnavailable && | ||
| isContentFilterProvider(usedProvider); |
There was a problem hiding this comment.
contentFilterSensitiveProviderBlocked is triggered whenever moderation is unavailable and the selected provider is content-filter-sensitive, even if moderation also returned flagged: true. checkOpenAIContentFilter can produce both states (e.g., one moderation sub-request fails while another flags), so this branch can return 503 before the normal contentFilterBlocked flow, changing a definite filter decision into an outage error for mixed-success moderation responses.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/gateway/src/fallback.spec.ts (1)
1681-1811:⚠️ Potential issue | 🟡 MinorAssert that the filtered upstream is never contacted.
These cases still only prove the final reroute/503 outcome. They will pass if the gateway briefly calls the content-filter-sensitive upstream before rerouting or blocking, which is the core regression this PR is trying to prevent.
🧪 Tighten the regression assertions
expect(res.status).not.toBe(503); expect(fetchSpy).toHaveBeenCalled(); + const rerouteUpstreamCalls = fetchSpy.mock.calls.filter(([input]) => { + const url = + typeof input === "string" + ? input + : input instanceof URL + ? input.toString() + : input.url; + return url.startsWith(mockServerUrl); + }); + expect(rerouteUpstreamCalls).toHaveLength(1); ... expect(res.status).toBe(503); + const blockedUpstreamCalls = fetchSpy.mock.calls.filter(([input]) => { + const url = + typeof input === "string" + ? input + : input instanceof URL + ? input.toString() + : input.url; + return url.startsWith(mockServerUrl); + }); + expect(blockedUpstreamCalls).toHaveLength(0);Repeat the reroute assertion in the auto-routing and low-uptime-fallback cases as well.
Also applies to: 1813-1918, 1920-2067, 2069-2197
🤖 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 1681 - 1811, The test "openai moderation failure reroutes away from content-filter providers" currently only checks final routing/logs but doesn't assert the content-filtered upstream was never contacted; update the test to assert the filtered provider was not called by inspecting fetchSpy calls (e.g., ensure no fetch call made to the together.ai endpoint or togetherProvider.baseUrl) — use the existing fetchSpy variable and togetherProvider identifier to check fetchSpy.mock.calls contain no request URLs matching together.ai (and add the same non-call assertion to the other affected tests named for auto-routing and low-uptime-fallback so they also verify the filtered upstream was never contacted).
🤖 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 2139-2167: The current routing logic can switch away from the
active usedProvider when every fallback candidate is moderation-sensitive;
instead, if a usedProvider exists and is eligible (not excluded, not
rate-limited, and not itself moderation-sensitive), prefer staying on it rather
than routing into a moderation-sensitive fallback pool. Modify the
candidatesForRouting selection (the block that uses
contentFilterPreferredNonRateLimitedAlternatives,
contentFilterPreferredProviders, nonRateLimitedAlternatives,
availableModelProviders) to first check usedProvider eligibility (check
usedProvider against contentFilterRoutingExcludedProviders,
rateLimitedAlternatives, and whether it appears in
contentFilterPreferredProviders or otherwise is non-moderation-sensitive) and,
if eligible, place usedProvider as the sole/first candidate or return
[usedProvider]; otherwise fall back to the existing multi-branch selection.
Ensure this same check is applied at the other location mentioned (the similar
block at the later lines).
---
Duplicate comments:
In `@apps/gateway/src/fallback.spec.ts`:
- Around line 1681-1811: The test "openai moderation failure reroutes away from
content-filter providers" currently only checks final routing/logs but doesn't
assert the content-filtered upstream was never contacted; update the test to
assert the filtered provider was not called by inspecting fetchSpy calls (e.g.,
ensure no fetch call made to the together.ai endpoint or
togetherProvider.baseUrl) — use the existing fetchSpy variable and
togetherProvider identifier to check fetchSpy.mock.calls contain no request URLs
matching together.ai (and add the same non-call assertion to the other affected
tests named for auto-routing and low-uptime-fallback so they also verify the
filtered upstream was never contacted).
🪄 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: 3c7d43cc-96a3-4a6a-93a0-facb76f2900f
📒 Files selected for processing (2)
apps/gateway/src/chat/chat.tsapps/gateway/src/fallback.spec.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 290557d94d
ℹ️ 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".
| return { | ||
| excludedProviders, | ||
| routingApplied: existingRoutingApplied || decision.rerouted, | ||
| }; |
There was a problem hiding this comment.
routingApplied is set from decision.rerouted before a replacement provider is actually chosen. In the rate-limit and low-uptime fallback paths, getCheapestFromAvailableProviders(...) can still return null (for example when preferred alternatives are filtered out as unstable), leaving usedProvider unchanged, but this flag stays true. Because contentFilterBlocked later gates on !contentFilterRoutingApplied, a matched prompt can skip the gateway content-filter block and continue to the original provider even though no reroute occurred.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/gateway/src/fallback.spec.ts (1)
1745-1750: ReusegetFetchCallUrl()inside the fetch spies.The same URL-normalization branch is copied four times right below the shared helper. Reusing the helper keeps that parsing logic in one place.
♻️ Deduplicate the URL extraction
- const url = - typeof input === "string" - ? input - : input instanceof URL - ? input.toString() - : input.url; + const url = getFetchCallUrl(input);Also applies to: 1885-1890, 1998-2003, 2167-2172
🤖 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 1745 - 1750, The duplicated URL-normalization logic inside the fetch spies should be replaced by reusing the existing helper getFetchCallUrl(); locate each instance in the spec where the code computes url via typeof input === "string" ? input : input instanceof URL ? input.toString() : input.url (seen in the fetch spy blocks around the failing tests) and call getFetchCallUrl(input) instead. Update the four spots (the fetch spy callbacks/assertions) to call getFetchCallUrl rather than repeating the branch so all URL parsing lives in one place and tests remain consistent.
🤖 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/fallback.spec.ts`:
- Line 873: Replace the weak status assertion expect(res.status).not.toBe(503)
with a strict success assertion expect(res.status).toBe(200) in the failing
test(s) so the success path is pinned to HTTP 200; update the same pattern at
the other occurrences mentioned (the other expect(res.status).not.toBe(503)
instances) to expect 200 instead.
- Around line 1804-1811: The tests currently only assert routing and token
selection (fetchSpy, getMockServerTokens) but don't assert that the gateway
actually attempted a call to https://api.openai.com/v1/moderations; add an
assertion that one of fetchSpy.mock.calls targeted that exact moderation URL
(e.g., examine the first argument(s) from fetchSpy.mock.calls with
getMockServerCalls(fetchSpy.mock.calls) or inspect call[0].url) to ensure the
moderation request was made; update the same pattern in the other failing blocks
(lines around where getMockServerTokens is checked: the groups at 1919-1926,
2072-2083, 2206-2209) so each outage-path test explicitly expects a call to the
OpenAI moderations endpoint.
---
Nitpick comments:
In `@apps/gateway/src/fallback.spec.ts`:
- Around line 1745-1750: The duplicated URL-normalization logic inside the fetch
spies should be replaced by reusing the existing helper getFetchCallUrl();
locate each instance in the spec where the code computes url via typeof input
=== "string" ? input : input instanceof URL ? input.toString() : input.url (seen
in the fetch spy blocks around the failing tests) and call
getFetchCallUrl(input) instead. Update the four spots (the fetch spy
callbacks/assertions) to call getFetchCallUrl rather than repeating the branch
so all URL parsing lives in one place and tests remain consistent.
🪄 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: e41b3177-aafe-4534-b290-865913b9f306
📒 Files selected for processing (1)
apps/gateway/src/fallback.spec.ts
| }); | ||
|
|
||
| expect(res.status).toBe(200); | ||
| expect(res.status).not.toBe(503); |
There was a problem hiding this comment.
Keep the success-path assertions pinned to HTTP 200.
not.toBe(503) only excludes one failure mode. A generic 4xx/5xx regression can still satisfy the status check, which weakens these fallback/reroute tests unnecessarily.
🧪 Keep the contract strict
- expect(res.status).not.toBe(503);
+ expect(res.status).toBe(200);Also applies to: 1673-1673, 1803-1803
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/src/fallback.spec.ts` at line 873, Replace the weak status
assertion expect(res.status).not.toBe(503) with a strict success assertion
expect(res.status).toBe(200) in the failing test(s) so the success path is
pinned to HTTP 200; update the same pattern at the other occurrences mentioned
(the other expect(res.status).not.toBe(503) instances) to expect 200 instead.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54bb162bde
ℹ️ 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 contentFilterSensitiveProviderBlocked = | ||
| contentFilterMode === "enabled" && | ||
| contentFilterUnavailable && | ||
| isContentFilterProvider(usedProvider); |
There was a problem hiding this comment.
This outage gate blocks as soon as the selected provider is content-filter-sensitive, but for direct-provider requests we only evaluate alternative providers in the rate-limit/low-uptime branches. In the common case where the requested sensitive provider is healthy, contentFilterSensitiveProviderBlocked becomes true and returns 503 even when noFallback is not set and a non-sensitive provider is available, creating avoidable outages during moderation API incidents.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e26c302510
ℹ️ 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".
| // Log the filtered request | ||
| try { | ||
| await insertLog({ | ||
| await insertLogEntry({ |
There was a problem hiding this comment.
Route this branch through the local insertLog(...) wrapper instead of calling insertLogEntry(...) directly; the wrapper is the only place that attaches gatewayContentFilterResponse (and content-filter tagging) to log records. With the current call site, requests blocked by gateway content filtering are persisted without the moderation payload, which regresses observability/debugging for blocked prompts and can hide why the filter fired in downstream log consumers.
Useful? React with 👍 / 👎.
This change stops gateway routing from falling through to content-filter-sensitive providers when the OpenAI moderation check is unavailable. If another eligible provider exists, routing now prefers it; if not, the request returns 503 instead of using the sensitive provider. The PR also records moderation-unavailable routing metadata, updates the shared log card, and adds regression coverage for reroute and block behavior. Verification: targeted Vitest coverage passed, then pnpm format and pnpm build passed.
Summary by CodeRabbit
Bug Fixes
New Features
UI
Tests
Chore