(MOT-3961) feat: expose model-specific reasoning efforts - #473
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis change adds provider-native reasoning-effort metadata and forwarding, introduces OpenAI Responses API support with SSE handling, updates Codex reasoning validation, and redesigns console model and permission pickers. ChangesReasoning metadata and forwarding
OpenAI Responses support
Supporting updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Console
participant RealBackend
participant Harness
participant Router
participant OpenAI
Console->>RealBackend: Select model and reasoning effort
RealBackend->>Harness: harness::send with provider_options
Harness->>Router: router::chat with provider_options
Router->>OpenAI: Responses or Chat Completions request
OpenAI-->>Router: SSE response events
Router-->>Console: Streamed assistant events
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
skill-check — worker0 verified, 41 skipped (no docs/).
Four for four. Nicely done. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
harness/src/turn_loop.rs (1)
335-339: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilent fallback to
Noneon serialization failure.
serde_json::to_value(options).ok()swallows any serialization error, silently omittingprovider_optionsfrom the request instead of surfacing it. Givenoptionsis aBTreeMap<String, Value>, serialization is practically infallible today, so this is low risk, but atracing::warn!on theErrbranch would make a future regression (e.g. a key type change) visible instead of silently dropping provider-native options from the call.♻️ Optional: surface serialization failures
- provider_options: record - .options - .provider_options - .as_ref() - .and_then(|options| serde_json::to_value(options).ok()), + provider_options: record.options.provider_options.as_ref().and_then(|options| { + serde_json::to_value(options) + .inspect_err(|e| tracing::warn!(error = %e, "failed to serialize provider_options")) + .ok() + }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@harness/src/turn_loop.rs` around lines 335 - 339, In the provider_options construction within the turn-loop request, replace the silent serde_json::to_value(options).ok() fallback with explicit error handling that emits a tracing::warn! containing the serialization error, while still returning None on failure; keep successful serialization unchanged.harness/Makefile (1)
81-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared Cargo worker loop.
checkmakeflags this new target for exceeding the maximum body length, and its validation/iteration logic duplicatesbuild. Move the common worker-manifest loop into a Make macro or helper script so both targets remain concise and consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@harness/Makefile` around lines 81 - 93, Extract the duplicated worker validation and iteration logic from cargo-clean and build into a shared Make macro or helper script. Preserve manifest existence checks, worker reporting, and command execution behavior, then have both targets invoke the shared helper so each target stays within checkmake’s body-length limit.Source: Linters/SAST tools
provider-openai-codex/src/reasoning.rs (1)
212-250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding edge-case tests for
native_reasoning_effort.The current test covers the happy path (supported effort accepted) and the rejection path (unsupported effort rejected). Several branches lack coverage:
provider_optionsisNone,reasoning_effortkey absent, non-string value, empty/whitespace string, andmodel_metaisNone(cold-start permissive path). These are the input-validation boundaries most likely to regress.🧪 Suggested additional tests
#[test] fn native_effort_absent_when_no_provider_options() { assert_eq!(native_reasoning_effort(None, None), Ok(None)); } #[test] fn native_effort_absent_when_key_missing() { assert_eq!( native_reasoning_effort(Some(&serde_json::json!({})), None), Ok(None) ); } #[test] fn native_effort_rejects_non_string() { assert!(native_reasoning_effort( Some(&serde_json::json!({ "reasoning_effort": 42 })), None, ) .unwrap_err() .contains("non-empty string")); } #[test] fn native_effort_rejects_empty_or_whitespace() { assert!(native_reasoning_effort( Some(&serde_json::json!({ "reasoning_effort": " " })), None, ) .is_err()); } #[test] fn native_effort_permissive_without_catalog() { assert_eq!( native_reasoning_effort( Some(&serde_json::json!({ "reasoning_effort": "anything" })), None, ), Ok(Some("anything".into())) ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@provider-openai-codex/src/reasoning.rs` around lines 212 - 250, Add edge-case unit tests alongside native_effort_uses_model_specific_catalog for native_reasoning_effort: cover None provider_options, a missing reasoning_effort key, non-string values, empty or whitespace-only strings, and valid arbitrary efforts when model_meta is None. Assert the expected Ok(None), validation errors, or permissive Ok(Some(...)) results for each case.provider-openai/src/stream_fn.rs (1)
213-238: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test case for
effort = None.The test only covers
Some("high"). Add a case forNoneto verify the guard does not activate when no reasoning effort is present.♻️ Proposed additional test assertions
assert_eq!( compatible_reasoning_effort( ApiMode::ChatCompletions, "gpt-5.6-luna", false, Some("high") ), (Some("high"), false) ); + // None effort should not trigger the guard + assert_eq!( + compatible_reasoning_effort( + ApiMode::ChatCompletions, + "gpt-5.6-luna", + true, + None, + ), + (None, false) + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@provider-openai/src/stream_fn.rs` around lines 213 - 238, Add an assertion in the existing luna_tools_disable_effort_only_on_chat_completions test for compatible_reasoning_effort with ApiMode::ChatCompletions, model "gpt-5.6-luna", tools enabled, and effort None; verify it returns (None, false), confirming the guard does not activate without a reasoning effort.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@console/web/src/hooks/use-model-picker-source.ts`:
- Around line 73-97: Update the initial snapshot logic in the useEffect using
fetchProviderList so a provider event arriving during the request does not
discard the fetched provider list. When the snapshot resolves, reconcile it with
the current presentProviders by provider id, preserving the freshest
event-driven available state while using snapshot metadata such as display_name
and supports_model_listing; ensure newly event-created entries are replaced or
enriched rather than retaining fabricated values. Keep cancellation handling
intact and apply the same reconciliation on successful snapshot completion even
when providerEventVersion.current differs from snapshotVersion.
In `@provider-openai/src/sse.rs`:
- Around line 484-510: Handle the “content_filter” reason in the
response.incomplete branch of the SSE event handler as a graceful completion,
matching the Chat Completions behavior: set StopReason::End, emit the
appropriate warning, close any open block, and push Stop and Done events instead
of AssistantMessageEvent::Error. Preserve the existing max_output_tokens
handling and error behavior for other reasons.
- Around line 58-60: The has_content method must match build_content by counting
only function_calls entries with a non-empty function_id, preventing empty
entries from being treated as content. Also update response.incomplete handling
to recognize content_filter separately from unknown reasons, returning the
documented non-hard-failure behavior instead of Error; use the relevant
build_content and incomplete-response matching logic in
provider-openai/src/sse.rs.
In `@provider-openai/src/stream_fn.rs`:
- Around line 24-40: Update compatible_reasoning_effort so needs_luna_guard only
activates when effort.is_some() and the requested effort is not "none"; preserve
the existing ChatCompletions, tools, and Luna model checks, and continue
returning the original None unchanged when no effort was requested.
---
Nitpick comments:
In `@harness/Makefile`:
- Around line 81-93: Extract the duplicated worker validation and iteration
logic from cargo-clean and build into a shared Make macro or helper script.
Preserve manifest existence checks, worker reporting, and command execution
behavior, then have both targets invoke the shared helper so each target stays
within checkmake’s body-length limit.
In `@harness/src/turn_loop.rs`:
- Around line 335-339: In the provider_options construction within the turn-loop
request, replace the silent serde_json::to_value(options).ok() fallback with
explicit error handling that emits a tracing::warn! containing the serialization
error, while still returning None on failure; keep successful serialization
unchanged.
In `@provider-openai-codex/src/reasoning.rs`:
- Around line 212-250: Add edge-case unit tests alongside
native_effort_uses_model_specific_catalog for native_reasoning_effort: cover
None provider_options, a missing reasoning_effort key, non-string values, empty
or whitespace-only strings, and valid arbitrary efforts when model_meta is None.
Assert the expected Ok(None), validation errors, or permissive Ok(Some(...))
results for each case.
In `@provider-openai/src/stream_fn.rs`:
- Around line 213-238: Add an assertion in the existing
luna_tools_disable_effort_only_on_chat_completions test for
compatible_reasoning_effort with ApiMode::ChatCompletions, model "gpt-5.6-luna",
tools enabled, and effort None; verify it returns (None, false), confirming the
guard does not activate without a reasoning effort.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 157d79b4-bc16-4aca-bdbb-96e9690f1bd7
⛔ Files ignored due to path filters (1)
provider-openai/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (63)
console/web/src/components/chat/Composer.stories.tsxconsole/web/src/components/chat/ModelPicker.tsxconsole/web/src/components/permissions/PermissionModePicker.tsxconsole/web/src/hooks/use-model-picker-source.tsconsole/web/src/lib/backend/harness-send.tsconsole/web/src/lib/backend/real-metadata.test.tsconsole/web/src/lib/backend/real.tsconsole/web/src/lib/backend/types.tsconsole/web/src/lib/models-catalog.test.tsconsole/web/src/lib/models-catalog.tsconsole/web/src/types/chat.tsharness/Makefileharness/src/clients/router.rsharness/src/functions/send.rsharness/src/subagent.rsharness/src/turn_loop.rsharness/src/types/mod.rsharness/src/types/model.rsharness/src/types/turn.rsharness/tests/golden/schemas/harness.send.jsonllm-router/src/catalog/queries.rsllm-router/src/types/model.rsllm-router/tests/golden/schemas/router.models.get.jsonllm-router/tests/golden/schemas/router.models.list.jsonllm-router/tests/golden/schemas/router.models.reconcile.jsonllm-router/tests/golden/schemas/router.provider.register.jsonprovider-anthropic/src/discovery.rsprovider-anthropic/src/thinking.rsprovider-anthropic/tests/golden/schemas/provider.anthropic.stream.jsonprovider-llamacpp/src/discovery.rsprovider-llamacpp/src/sse.rsprovider-llamacpp/tests/golden/schemas/provider.llamacpp.stream.jsonprovider-openai-codex/src/discovery.rsprovider-openai-codex/src/reasoning.rsprovider-openai-codex/src/request.rsprovider-openai-codex/src/stream_fn.rsprovider-openai-codex/tests/golden/schemas/provider.openai-codex.stream.jsonprovider-openai/Cargo.tomlprovider-openai/README.mdprovider-openai/iii.worker.yamlprovider-openai/src/config.rsprovider-openai/src/curated.rsprovider-openai/src/discovery.rsprovider-openai/src/lib.rsprovider-openai/src/main.rsprovider-openai/src/manifest.rsprovider-openai/src/reasoning.rsprovider-openai/src/request.rsprovider-openai/src/sse.rsprovider-openai/src/stream_fn.rsprovider-openai/src/surface.rsprovider-openai/src/upstream.rsprovider-openai/src/wire/messages.rsprovider-openai/src/wire/mod.rsprovider-openai/src/wire/tools.rsprovider-openai/tests/golden/schemas/provider.openai.stream.jsonprovider-openai/tests/integration.rsprovider-xai/src/curated.rsprovider-xai/tests/golden/schemas/provider.xai.stream.jsonprovider-zai/src/curated.rsprovider-zai/tests/golden/schemas/provider.zai.stream.jsontech-specs/2026-06-agentic/README.mdtech-specs/2026-06-agentic/llm-router.md
| // One initial snapshot. Subsequent availability changes are applied from | ||
| // `router::provider::changed`, so model refreshes never re-read this list. | ||
| useEffect(() => { | ||
| if (backendId !== 'real' || !harnessAvailable) { | ||
| setPresentProviders([]) | ||
| return | ||
| } | ||
| let cancelled = false | ||
| const snapshotVersion = providerEventVersion.current | ||
| void fetchProviderList() | ||
| .then((providers) => { | ||
| if (!cancelled && providerEventVersion.current === snapshotVersion) { | ||
| setPresentProviders(providers) | ||
| } | ||
| }) | ||
| .catch(() => { | ||
| if (!cancelled && providerEventVersion.current === snapshotVersion) { | ||
| setPresentProviders([]) | ||
| } | ||
| }) | ||
| return () => { | ||
| cancelled = true | ||
| } | ||
| }, [backendId, harnessAvailable]) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Snapshot is discarded (not merged) if any provider event lands first — can leave presentProviders incomplete.
The staleness guard on Lines 84-90 compares providerEventVersion.current to the version captured before the fetch, but on mismatch it drops the entire fetched snapshot rather than merging it with whatever partial updates already landed. Since subscribeProviderChanges is wired up asynchronously in a sibling effect (Lines 121-145) and both effects fire on mount, a provider::changed event that arrives before the snapshot promise resolves will bump the version and cause the full list from fetchProviderList() to be thrown away — leaving presentProviders with only the one entry the event patched (built from an empty starting array), not the complete provider list.
Compounding this, the "add new provider" branch (Lines 132-140) fabricates display_name: provider (raw id) and supports_model_listing: true for any provider not yet known, and there's no other path that refreshes this metadata after mount — so an event-created entry can persist with guessed data indefinitely.
Consider merging the snapshot into current state (e.g. reconcile by id, preferring the freshest available flag per entry) instead of an all-or-nothing overwrite/discard, so a late-arriving event never causes the base list to vanish.
💡 Sketch: merge instead of discard
void fetchProviderList()
.then((providers) => {
- if (!cancelled && providerEventVersion.current === snapshotVersion) {
- setPresentProviders(providers)
- }
+ if (cancelled) return
+ setPresentProviders((current) => {
+ // Merge: snapshot supplies full/accurate entries; keep any
+ // `available` flips applied by events that arrived after the
+ // snapshot version was captured.
+ const byId = new Map(current.map((p) => [p.id, p]))
+ return providers.map((p) => {
+ const patched = byId.get(p.id)
+ return patched && providerEventVersion.current !== snapshotVersion
+ ? { ...p, available: patched.available }
+ : p
+ })
+ })
})📝 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.
| // One initial snapshot. Subsequent availability changes are applied from | |
| // `router::provider::changed`, so model refreshes never re-read this list. | |
| useEffect(() => { | |
| if (backendId !== 'real' || !harnessAvailable) { | |
| setPresentProviders([]) | |
| return | |
| } | |
| let cancelled = false | |
| const snapshotVersion = providerEventVersion.current | |
| void fetchProviderList() | |
| .then((providers) => { | |
| if (!cancelled && providerEventVersion.current === snapshotVersion) { | |
| setPresentProviders(providers) | |
| } | |
| }) | |
| .catch(() => { | |
| if (!cancelled && providerEventVersion.current === snapshotVersion) { | |
| setPresentProviders([]) | |
| } | |
| }) | |
| return () => { | |
| cancelled = true | |
| } | |
| }, [backendId, harnessAvailable]) | |
| // One initial snapshot. Subsequent availability changes are applied from | |
| // `router::provider::changed`, so model refreshes never re-read this list. | |
| useEffect(() => { | |
| if (backendId !== 'real' || !harnessAvailable) { | |
| setPresentProviders([]) | |
| return | |
| } | |
| let cancelled = false | |
| const snapshotVersion = providerEventVersion.current | |
| void fetchProviderList() | |
| .then((providers) => { | |
| if (cancelled) return | |
| setPresentProviders((current) => { | |
| // Merge: snapshot supplies full/accurate entries; keep any | |
| // `available` flips applied by events that arrived after the | |
| // snapshot version was captured. | |
| const byId = new Map(current.map((p) => [p.id, p])) | |
| return providers.map((p) => { | |
| const patched = byId.get(p.id) | |
| return patched && providerEventVersion.current !== snapshotVersion | |
| ? { ...p, available: patched.available } | |
| : p | |
| }) | |
| }) | |
| }) | |
| .catch(() => { | |
| if (!cancelled && providerEventVersion.current === snapshotVersion) { | |
| setPresentProviders([]) | |
| } | |
| }) | |
| return () => { | |
| cancelled = true | |
| } | |
| }, [backendId, harnessAvailable]) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@console/web/src/hooks/use-model-picker-source.ts` around lines 73 - 97,
Update the initial snapshot logic in the useEffect using fetchProviderList so a
provider event arriving during the request does not discard the fetched provider
list. When the snapshot resolves, reconcile it with the current presentProviders
by provider id, preserving the freshest event-driven available state while using
snapshot metadata such as display_name and supports_model_listing; ensure newly
event-created entries are replaced or enriched rather than retaining fabricated
values. Keep cancellation handling intact and apply the same reconciliation on
successful snapshot completion even when providerEventVersion.current differs
from snapshotVersion.
| pub fn has_content(&self) -> bool { | ||
| !self.text.is_empty() || !self.thinking.is_empty() || !self.function_calls.is_empty() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant definitions and call sites.
rg -n "has_content\\(|build_content\\(|ensure_function_call\\(|function_id|response\\.incomplete|incomplete_details|Done vs synthetic error|synthetic error|handle_responses_event" provider-openai/src -S
# Show the surrounding code for the relevant file.
sed -n '1,220p' provider-openai/src/sse.rs
# Show the upstream fallback logic if present.
sed -n '1,260p' provider-openai/src/upstream.rsRepository: iii-hq/workers
Length of output: 19889
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find exact locations in provider-openai/src where has_content is used.
rg -n "has_content\\(" provider-openai/src -S
# Show the specific surrounding sections from upstream.rs if found.
grep -n "has_content()" -n provider-openai/src/upstream.rs || trueRepository: iii-hq/workers
Length of output: 320
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== sse.rs =="
nl -ba provider-openai/src/sse.rs | sed -n '1,220p'
echo
echo "== upstream.rs matches =="
rg -n "has_content\\(|Done|synthetic error|response\\.incomplete|incomplete_details|content_filter|max_output_tokens|function_id|function_calls" provider-openai/src/upstream.rs provider-openai/src -SRepository: iii-hq/workers
Length of output: 205
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the Responses-event branch and its tests around response.incomplete.
sed -n '340,540p' provider-openai/src/sse.rs
echo
echo "== tests around response.incomplete =="
sed -n '780,860p' provider-openai/src/sse.rsRepository: iii-hq/workers
Length of output: 10492
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact section around handle_responses_event and response.incomplete.
sed -n '350,520p' provider-openai/src/sse.rs
echo
echo "== response.incomplete tests =="
sed -n '820,860p' provider-openai/src/sse.rsRepository: iii-hq/workers
Length of output: 8051
🌐 Web query:
OpenAI Responses API incomplete_details.reason documented values max_output_tokens content_filter
💡 Result:
In the OpenAI Responses API, the incomplete_details.reason field provides the specific reason why a response has terminated with a status of "incomplete" [1][2]. According to official API documentation, the documented string values for this field are [1]: - max_output_tokens: Indicates that the response was truncated because the model reached the configured max_output_tokens limit [1][2]. Note that this limit applies to all tokens generated, including non-visible tokens like formatting, delimiters, and reasoning tokens [3]. - content_filter: Indicates that the response was terminated due to a content policy violation [1][2]. When a response status is "incomplete," the incomplete_details object will contain this reason [1]. Developers are advised to inspect this field to determine the appropriate handling logic (e.g., whether to retry with adjusted parameters) [4][5][6]. In some cases, particularly with "max_output_tokens," a response may contain reasoning items but no message items, resulting in an empty output_text [5][7]. Additionally, while these are the primary documented reasons, client implementations should be prepared for potential null values or future-added enum members if using older SDK versions [8][9].
Citations:
- 1: https://developers.openai.com/api/reference/resources/responses/methods/create/
- 2: https://community.openai.com/t/responses-api-streaming-the-simple-guide-to-events/1363122
- 3: https://developers.openai.com/api/docs/guides/token-counting
- 4: Background responses failures lack a stable code/name that maps to an exception class openai/openai-python#3212
- 5: https://community.openai.com/t/responses-api-empty-output-text-no-message-item-when-status-incomplete-due-to-max-output-tokens-reasoning-only-output/1373609
- 6: Responses streaming structured output parses incomplete JSON before terminal incomplete status openai/openai-python#3263
- 7: gpt-5-mini Returns Empty
output_textwith Responses API openai/openai-python#2546 - 8: https://javadoc.io/static/com.openai/openai-java/4.6.1/com/openai/models/responses/Response.IncompleteDetails.Reason.html
- 9: https://community.openai.com/t/empty-incomplete-details-from-gpt-5-with-max-output-tokens/1355725
Align has_content() with build_content() and avoid hard-errors for documented content_filter incompletes
has_content()still counts any non-emptyfunction_callsvec, butbuild_content()drops entries with an emptyfunction_id. That can make the stream-end fallback emitDonewith no content instead of the intended error.response.incompletecurrently treats every non-max_output_tokensreason asError, butcontent_filteris a documented Responses reason. Handle it separately instead of converting it into a hard failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@provider-openai/src/sse.rs` around lines 58 - 60, The has_content method must
match build_content by counting only function_calls entries with a non-empty
function_id, preventing empty entries from being treated as content. Also update
response.incomplete handling to recognize content_filter separately from unknown
reasons, returning the documented non-hard-failure behavior instead of Error;
use the relevant build_content and incomplete-response matching logic in
provider-openai/src/sse.rs.
| "response.incomplete" => { | ||
| responses_usage(event, state); | ||
| let reason = event | ||
| .pointer("/response/incomplete_details/reason") | ||
| .and_then(Value::as_str) | ||
| .unwrap_or("incomplete"); | ||
| if reason == "max_output_tokens" { | ||
| state.stop_reason = StopReason::Length; | ||
| state.native_stop_reason = Some(reason.to_string()); | ||
| close_open_block(state, model, &mut events); | ||
| events.push(AssistantMessageEvent::Stop { | ||
| stop_reason: StopReason::Length, | ||
| error_message: None, | ||
| error_kind: None, | ||
| }); | ||
| events.push(AssistantMessageEvent::Done { | ||
| message: build_final(state, model), | ||
| }); | ||
| } else { | ||
| let message = format!("OpenAI response incomplete: {reason}"); | ||
| state.stop_reason = StopReason::Error; | ||
| state.error_message = Some(message.clone()); | ||
| let mut error = build_final(state, model); | ||
| error.error_kind = Some(classify(None, &message)); | ||
| events.push(AssistantMessageEvent::Error { error }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle content_filter as a graceful stop
incomplete_details.reason can be content_filter on the Responses API, so this branch should mirror the Chat Completions path (StopReason::End + warning) instead of turning it into AssistantMessageEvent::Error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@provider-openai/src/sse.rs` around lines 484 - 510, Handle the
“content_filter” reason in the response.incomplete branch of the SSE event
handler as a graceful completion, matching the Chat Completions behavior: set
StopReason::End, emit the appropriate warning, close any open block, and push
Stop and Done events instead of AssistantMessageEvent::Error. Preserve the
existing max_output_tokens handling and error behavior for other reasons.
| fn compatible_reasoning_effort( | ||
| api_mode: ApiMode, | ||
| model: &str, | ||
| has_tools: bool, | ||
| effort: Option<&'static str>, | ||
| ) -> (Option<&'static str>, bool) { | ||
| let needs_luna_guard = api_mode == ApiMode::ChatCompletions | ||
| && has_tools | ||
| && model.to_ascii_lowercase().contains("luna") | ||
| && effort != Some("none"); | ||
| if needs_luna_guard { | ||
| (Some("none"), true) | ||
| } else { | ||
| (effort, false) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard activates when effort is None, adding reasoning_effort: "none" unnecessarily.
When effort is None (no reasoning effort requested), effort != Some("none") evaluates to true, so the guard activates and returns (Some("none"), true). This adds reasoning_effort: "none" to the request body and emits a misleading "reasoning effort disabled" warning even though no reasoning effort was requested. For reasoning models with an empty effort ladder (where reasoning_effort_for returned None), this could cause an API rejection since the model doesn't accept the parameter at all.
🐛 Proposed fix: add `effort.is_some()` to the guard condition
fn compatible_reasoning_effort(
api_mode: ApiMode,
model: &str,
has_tools: bool,
effort: Option<&'static str>,
) -> (Option<&'static str>, bool) {
let needs_luna_guard = api_mode == ApiMode::ChatCompletions
&& has_tools
&& model.to_ascii_lowercase().contains("luna")
- && effort != Some("none");
+ && effort.is_some()
+ && effort != Some("none");
if needs_luna_guard {
(Some("none"), true)
} else {
(effort, false)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn compatible_reasoning_effort( | |
| api_mode: ApiMode, | |
| model: &str, | |
| has_tools: bool, | |
| effort: Option<&'static str>, | |
| ) -> (Option<&'static str>, bool) { | |
| let needs_luna_guard = api_mode == ApiMode::ChatCompletions | |
| && has_tools | |
| && model.to_ascii_lowercase().contains("luna") | |
| && effort != Some("none"); | |
| if needs_luna_guard { | |
| (Some("none"), true) | |
| } else { | |
| (effort, false) | |
| } | |
| } | |
| fn compatible_reasoning_effort( | |
| api_mode: ApiMode, | |
| model: &str, | |
| has_tools: bool, | |
| effort: Option<&'static str>, | |
| ) -> (Option<&'static str>, bool) { | |
| let needs_luna_guard = api_mode == ApiMode::ChatCompletions | |
| && has_tools | |
| && model.to_ascii_lowercase().contains("luna") | |
| && effort.is_some() | |
| && effort != Some("none"); | |
| if needs_luna_guard { | |
| (Some("none"), true) | |
| } else { | |
| (effort, false) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@provider-openai/src/stream_fn.rs` around lines 24 - 40, Update
compatible_reasoning_effort so needs_luna_guard only activates when
effort.is_some() and the requested effort is not "none"; preserve the existing
ChatCompletions, tools, and Luna model checks, and continue returning the
original None unchanged when no effort was requested.
1 participant
Summary
cargo-cleantarget for all workers or an explicit worker subsetImpact
Users see only supported reasoning efforts for the selected model, can move directly from model selection to effort selection, and no longer hit the Luna function-tools plus reasoning-effort failure on the default OpenAI transport.
Validation
pnpm test --run— 72 files, 927 testspnpm exec biome checkandpnpm typecheckcargo test— 79 library, 2 binary, 6 integration, and 4 schema testscargo clippy --all-targets -- -D warningsandcargo fmt --checkmake -n cargo-cleanand selected-worker dry runFixes MOT-3961
Summary by CodeRabbit