feat(api): enhance reasoning content checks - #813
Conversation
Conditionally enforce reasoning content validation for OpenAI responses when `USE_RESPONSES_API` is enabled. Prevent unnecessary checks for other providers.
WalkthroughE2E tests updated to cast the selected reasoning provider to ProviderModelMapping, use optional chaining for reasoning fields, and gate assertions so reasoning_content is skipped for OpenAI when USE_RESPONSES_API is not enabled or when reasoningOutput === "omit". (46 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test Runner
participant G as Gateway E2E Test
participant P as Provider Config
participant E as Env (USE_RESPONSES_API)
T->>G: Run reasoning (non-streaming/streaming) test
G->>P: Find provider where reasoning === true
Note right of G: Cast to ProviderModelMapping\nUse optional chaining for reasoning fields
G->>E: Read USE_RESPONSES_API
alt Assert reasoning_content
Note over G,P: reasoningOutput !== "omit"\nand (NOT (providerId === "openai" AND USE_RESPONSES_API !== "true"))
G->>G: Assert `reasoning_content` present
else Skip assertion
G->>G: Do not assert `reasoning_content`
end
G-->>T: Test result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/gateway/src/api.e2e.ts (1)
869-884: Extend the same gating to “reasoning + tool calls” for consistency.Currently this block enforces reasoning_content regardless of USE_RESPONSES_API; bring it in line with the other tests.
- if ( - (reasoningProvider as ProviderModelMapping)?.reasoningOutput !== - "omit" - ) { + const useResponsesApi = process.env.USE_RESPONSES_API === "true"; + const isOpenAI = reasoningProvider?.providerId === "openai"; + if ( + reasoningProvider?.reasoningOutput !== "omit" && + (!useResponsesApi || isOpenAI) + ) {
🧹 Nitpick comments (2)
apps/gateway/src/api.e2e.ts (2)
632-636: Fix misleading inline comment.The comment says “only enforce ... for openai on the responses API” but the surrounding condition negated it. After applying the corrected logic, keep the comment as “When using the Responses API, only enforce ... for OpenAI.”
Also applies to: 750-754
626-629: Prefer a type guard over assertion casts.Use a predicate type guard to avoid
asand keep types sound.Example:
const reasoningProvider = providers?.find( (p: ProviderModelMapping): p is ProviderModelMapping & { reasoning: true } => p.reasoning === true, );Also applies to: 744-747
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/gateway/src/api.e2e.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use top-level import; never use require() or dynamic import()
Always use top-level import; never use require or dynamic imports
Files:
apps/gateway/src/api.e2e.ts
{apps/{api,gateway}/**/*.ts,packages/db/**/*.ts}
📄 CodeRabbit inference engine (CLAUDE.md)
.findMany() or db().query.
{apps/{api,gateway}/**/*.ts,packages/db/**/*.ts}: For database reads, use Drizzle’s db().query..findFirst()
Use Drizzle ORM with the latest object syntaxFiles:
apps/gateway/src/api.e2e.ts**/*.e2e.ts
📄 CodeRabbit inference engine (AGENTS.md)
Name end-to-end test files with the .e2e.ts suffix
Files:
apps/gateway/src/api.e2e.tsapps/{api,gateway}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/{api,gateway}/**/*.{ts,tsx}: Use Drizzle ORM with the latest object syntax for database access
For reads, use db().query..findMany() or db().query.
.findFirst()
After API route changes, run pnpm generate to update OpenAPI schemasFiles:
apps/gateway/src/api.e2e.ts🧬 Code graph analysis (1)
apps/gateway/src/api.e2e.ts (1)
packages/models/src/models.ts (1)
ProviderModelMapping(23-100)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build / run
- GitHub Check: e2e / run
🔇 Additional comments (1)
apps/gateway/src/api.e2e.ts (1)
626-638: Fix inverted gating: only enforce reasoning checks for OpenAI when USE_RESPONSES_API=trueCurrent condition inverts intent — it enforces checks for non‑OpenAI when USE_RESPONSES_API="true". Apply the following change (apps/gateway/src/api.e2e.ts ~lines 626–638):
- ) as ProviderModelMapping; + ); - if ( - reasoningProvider?.reasoningOutput !== "omit" && - !( - // only enforce reasoning content checks for openai on the responses API - ( - process.env.USE_RESPONSES_API === "true" && - reasoningProvider?.providerId === "openai" - ) - ) - ) { + const useResponsesApi = process.env.USE_RESPONSES_API === "true"; + const isOpenAI = reasoningProvider?.providerId === "openai"; + // When using the Responses API, only enforce reasoning_content checks for OpenAI. + if ( + reasoningProvider?.reasoningOutput !== "omit" && + (!useResponsesApi || isOpenAI) + ) {Remove the unnecessary
as ProviderModelMappingcast (nit). Tests couldn't be executed in the sandbox (ENOENT: /home/jailuser/apps). Run locally to confirm:USE_RESPONSES_API=true pnpm -C apps/gateway test -t "reasoning"
USE_RESPONSES_API=false pnpm -C apps/gateway test -t "reasoning"
| const reasoningProvider = providers?.find( | ||
| (p: ProviderModelMapping) => p.reasoning === true, | ||
| ); | ||
| ) as ProviderModelMapping; | ||
| if ( | ||
| (reasoningProvider as ProviderModelMapping)?.reasoningOutput !== "omit" | ||
| reasoningProvider?.reasoningOutput !== "omit" && | ||
| !( | ||
| // only enforce reasoning content checks for openai on the responses API | ||
| ( | ||
| process.env.USE_RESPONSES_API === "true" && | ||
| reasoningProvider?.providerId === "openai" | ||
| ) | ||
| ) | ||
| ) { |
There was a problem hiding this comment.
Same inversion in streaming reasoning test; align gating and remove unsafe cast.
Mirror the corrected logic from the non‑streaming test so we only enforce for OpenAI when Responses API is enabled.
- ) as ProviderModelMapping;
- if (
- reasoningProvider?.reasoningOutput !== "omit" &&
- !(
- // only enforce reasoning content checks for openai on the responses API
- (
- process.env.USE_RESPONSES_API === "true" &&
- reasoningProvider?.providerId === "openai"
- )
- )
- ) {
+ );
+ const useResponsesApi = process.env.USE_RESPONSES_API === "true";
+ const isOpenAI = reasoningProvider?.providerId === "openai";
+ // When using the Responses API, only enforce reasoning_content checks for OpenAI.
+ if (
+ reasoningProvider?.reasoningOutput !== "omit" &&
+ (!useResponsesApi || isOpenAI)
+ ) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const reasoningProvider = providers?.find( | |
| (p: ProviderModelMapping) => p.reasoning === true, | |
| ); | |
| ) as ProviderModelMapping; | |
| if ( | |
| (reasoningProvider as ProviderModelMapping)?.reasoningOutput !== "omit" | |
| reasoningProvider?.reasoningOutput !== "omit" && | |
| !( | |
| // only enforce reasoning content checks for openai on the responses API | |
| ( | |
| process.env.USE_RESPONSES_API === "true" && | |
| reasoningProvider?.providerId === "openai" | |
| ) | |
| ) | |
| ) { | |
| const reasoningProvider = providers?.find( | |
| (p: ProviderModelMapping) => p.reasoning === true, | |
| ); | |
| const useResponsesApi = process.env.USE_RESPONSES_API === "true"; | |
| const isOpenAI = reasoningProvider?.providerId === "openai"; | |
| // When using the Responses API, only enforce reasoning_content checks for OpenAI. | |
| if ( | |
| reasoningProvider?.reasoningOutput !== "omit" && | |
| (!useResponsesApi || isOpenAI) | |
| ) { |
🤖 Prompt for AI Agents
In apps/gateway/src/api.e2e.ts around lines 744 to 756, the streaming reasoning
test uses an inverted gating condition and an unsafe cast; change the logic to
mirror the non-streaming test so reasoning content checks are enforced only when
process.env.USE_RESPONSES_API === "true" AND the selected providerId ===
"openai", and remove the unsafe "as ProviderModelMapping" cast by properly
typing or narrowing the result of providers?.find (e.g., check for undefined
before accessing properties or use a typed predicate) so the condition becomes:
if (reasoningProvider?.reasoningOutput !== "omit" &&
process.env.USE_RESPONSES_API === "true" && reasoningProvider?.providerId ===
"openai") { ... }.
Refactor conditional checks for reasoning content validation by introducing reusable variables and improving readability. Applies consistent behavior for "USE_RESPONSES_API" and "providerId" conditions.
Update conditional logic for enforcing reasoning content checks to properly handle `reasoningOutput` and `USE_RESPONSES_API` variables. Prevent unintended validation skips for OpenAI responses.
1 participant
Conditionally enforce reasoning content validation for OpenAI responses when
USE_RESPONSES_APIis enabled. Prevent unnecessary checks for other providers.Summary by CodeRabbit