feat(provider-openai): standalone OpenAI Chat Completions provider worker - #252
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 24 minutes and 53 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (31)
📝 WalkthroughWalkthroughAdds a new Rust worker "provider-openai" implementing llm-router provider protocol: provider registration, per-request credential/config resolution, OpenAI chat completions streaming (SSE → router frames), model discovery/curation and reconciliation, error classification, request/wire adapters, and an integration test suite. ChangesOpenAI Provider Implementation
Sequence Diagram(s) sequenceDiagram
participant Router
participant ProviderWorker
participant OpenAI_Upstream
participant IIIState
Router->>ProviderWorker: trigger provider::openai::stream (ProviderStreamInput)
ProviderWorker->>IIIState: load_token / store_token (registration token)
ProviderWorker->>Router: iii.trigger resolve (resolve credentials/config)
ProviderWorker->>OpenAI_Upstream: POST /v1/chat/completions (body, headers)
OpenAI_Upstream->>ProviderWorker: SSE data chunks -> parse -> AssistantMessageEvent
ProviderWorker->>Router: write frame to FrameSink (AssistantMessageEvent)
Router->>ProviderWorker: router ready (subscribe) -> provider re-declare/refresh
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
08b852d to
55c8add
Compare
skill-check — worker0 verified, 18 skipped (no docs/).
Four for four. Nicely done. |
55c8add to
952738c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (1)
provider-openai/tests/integration.rs (1)
196-223: ⚡ Quick winValidate the outbound chat request in the stub.
Right now every non-
GET /v1/modelsrequest gets a success/error fixture regardless of method, path, headers, or body. That means regressions in the OpenAI wire contract can slip through this suite whilechat_streams_end_to_end_with_cost_fill()still passes. Capture the raw request and assert the minimal contract here before sending the fixture.🤖 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/tests/integration.rs` around lines 196 - 223, The stub_upstream currently returns fixture responses for any non-GET request without validating the actual outbound request; update stub_upstream to read and preserve the full raw request (request line, headers, and body) for non-GET traffic, parse and assert the minimal OpenAI chat contract — e.g., method is POST, path is "/v1/chat/completions", Content-Type contains "application/json", and the JSON body contains a "messages" field — before choosing to return messages_response (leave GET /v1/models returning STUB_MODELS). Ensure these validations occur inside the spawned connection handler (the closure that reads from sock) and only send the fixture after assertions pass so failing contract expectations will surface test failures.
🤖 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 `@provider-openai/src/config.rs`:
- Around line 37-40: The match on resolved.credential currently treats any
Some(credential) as configured even when credential_parts(credential) yields an
empty string; change the logic in the block handling resolved.credential so that
you call credential_parts(credential), trim/check the resulting string and if
it's empty return Err(NotConfigured) instead of assigning credential_value.
Update the branch that currently sets credential_value to perform this
empty-check (use the existing credential_parts helper and the NotConfigured
error) so empty bearer tokens are treated as not configured.
In `@provider-openai/src/curated.rs`:
- Around line 37-43: family_meta currently has no entry for the bare "gpt-5"
alias, so models retained by is_legacy_generation() and
discovery::parse_live_models() fall through to defaults; add an explicit "gpt-5"
match arm in family_meta (e.g., map to ("GPT-5", same token limits/flags/pricing
as the intended current GPT‑5 family member) so it gets proper
max_output_tokens, structured-output/thinking flags, and pricing), or
alternatively normalize the alias to a known family (e.g., rewrite "gpt-5" to
"gpt-5.1" or "gpt-5.2") before calling enrich(); update family_meta (and/or the
normalization location) to reference the chosen family consistently.
In `@provider-openai/src/register.rs`:
- Around line 100-106: register_provider currently builds a reqwest::Client with
only connect_timeout and reuses it for streaming and discovery, which lets
provider::openai::refresh_models / provider-openai::discovery::fetch_live_models
hang if the upstream stalls; fix by creating a separate reqwest client for model
refresh (e.g., new client in register_provider or inside
refresh_models/fetch_live_models) that sets a full request timeout via
.timeout(...) or .read_timeout(...), or alternatively wrap the GET/json calls in
tokio::time::timeout, and ensure register_provider continues to use the original
client for streaming while the discovery path uses the timeout-enabled client.
In `@provider-openai/src/router_client.rs`:
- Around line 47-55: The function models_get currently swallows call errors via
`.await.ok()?`, making timeouts/auth/bus failures look like "no model"; change
models_get to return a Result<Option<Model>, IIIError> (instead of
Option<Model>) and propagate the call error instead of discarding it: await the
`call(...)` and `?` the Result to return the underlying IIIError, then handle
the case where `raw.get("model")` is None by returning Ok(None) and convert
serde deserialization failures into an IIIError (or map them to an appropriate
error variant) rather than silencing them; use the existing symbols models_get,
call, IIIError, and PROVIDER_ID to locate and update the logic.
In `@provider-openai/src/sse.rs`:
- Around line 247-250: The code reads an upstream-controlled index via
tc.get("index") and grows state.function_calls to that size, which allows
unbounded allocation; change this by validating and capping the index before
resizing: parse the index from tc.get("index"), check it against a safe maximum
(e.g., a MAX_FUNCTION_CALLS constant) and if it’s >= MAX_FUNCTION_CALLS or
otherwise invalid, either ignore the request or return/log an error instead of
growing the vector; only perform the while loop/resizing when the
validated_index is within the allowed bound so
state.function_calls.push(PartialFunctionCall::default()) cannot be driven by an
attacker to trigger OOM.
In `@provider-openai/src/state.rs`:
- Around line 10-20: The function load_token currently swallows all errors from
iii.trigger by using .await.ok()?; change load_token to return a
Result<Option<String>, E> (e.g., anyhow::Error or the III trigger error type)
instead of Option<String>, propagate the trigger error with ? (or map_err to add
context) when calling iii.trigger(TriggerRequest { function_id: "state::get",
payload: json!({ "scope": STATE_SCOPE, "key": TOKEN_KEY }), ... }), and only
convert a successful value into Option<String>
(value.as_str().map(String::from)); this preserves transient/state errors while
still returning None when the token is genuinely absent.
In `@provider-openai/src/stream_fn.rs`:
- Around line 92-97: The reasoning_effort and related checks use the original
input model variable (e.g., calls to is_reasoning_model and reasoning_effort_for
with &model and input.thinking_level) but the request actually sends cfg.model
(the resolved model/alias) upstream; update the logic to compute
reasoning_effort and the warning branch using the resolved model in cfg (use
cfg.model or the resolved model variable) so the classification/mapping and
warning behavior reflect the actual target model sent in the API call, and
ensure any other nearby checks (the ones around lines where reasoning_effort is
used and where cfg.model is sent) are changed consistently.
In `@provider-openai/src/wire/messages.rs`:
- Around line 145-153: The function to_wire_messages currently always creates
and pushes an assistant entry (entry = json!({ "role": "assistant" })) even when
both text and tool_calls are empty, which can cause OpenAI validation errors;
change the logic in to_wire_messages so you only construct/push the assistant
entry (the entry variable and the out.push(entry) call) when either text is
non-empty or tool_calls is non-empty, and ensure the existing comment/behavior
about placing placeholders for orphans "directly after the assistant" is
preserved by only inserting those placeholders after an actual assistant entry
has been emitted.
In `@provider-openai/src/wire/names.rs`:
- Around line 4-10: The current encode_tool_name/decode_tool_name is lossy for
names containing "__"; replace the ad-hoc replace scheme with a reversible
encoding (e.g., base64-url or percent-encoding) so encode_tool_name(name: &str)
returns a collision-free encoded string and decode_tool_name reverses it back;
update both functions (encode_tool_name and decode_tool_name) to use the chosen
reversible encoder/decoder so decode(encode(x)) == x for any input, and add a
short unit test exercising names with "__" and "::" to verify round-trip
correctness.
In `@provider-openai/tests/integration.rs`:
- Line 377: The test currently ignores the result of
tokio::time::timeout(Duration::from_secs(5), pump).await, which can hide a
timeout or task panic; update both chat_streams_end_to_end_with_cost_fill() and
upstream_401_surfaces_as_auth_expired_error_frame() to assert the timeout
succeeded (fail the test on Err from timeout) and then separately assert the
JoinHandle completed successfully (surface JoinError from awaiting the
JoinHandle) before proceeding to read frames. In practice replace the ignored
let _ = ... with checking the timeout Result (expect or assert on Err) to fail
on elapsed, then await the returned JoinHandle and fail/assert on its JoinError,
and only after that continue to read frames.
- Around line 42-48: The helper free_port() is vulnerable to a TOCTOU race
because it relinquishes the ephemeral port before spawn_engine() starts iii;
change the test startup to reserve the port (keep the TcpListener open) until
the engine is confirmed listening or implement a retry loop that picks a fresh
port and retries spawn_engine() on failure; specifically modify free_port()
usage or replace it with a reserve_port() that returns a TcpListener (or a
(port, listener) pair) and ensure spawn_engine() is invoked while the listener
is held, or wrap spawn_engine() in a small retry/backoff that retries on
bind/address-in-use errors to eliminate the intermittent race.
---
Nitpick comments:
In `@provider-openai/tests/integration.rs`:
- Around line 196-223: The stub_upstream currently returns fixture responses for
any non-GET request without validating the actual outbound request; update
stub_upstream to read and preserve the full raw request (request line, headers,
and body) for non-GET traffic, parse and assert the minimal OpenAI chat contract
— e.g., method is POST, path is "/v1/chat/completions", Content-Type contains
"application/json", and the JSON body contains a "messages" field — before
choosing to return messages_response (leave GET /v1/models returning
STUB_MODELS). Ensure these validations occur inside the spawned connection
handler (the closure that reads from sock) and only send the fixture after
assertions pass so failing contract expectations will surface test failures.
🪄 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: 990ef76b-3c78-4b17-ba56-4f85770e70ef
⛔ Files ignored due to path filters (1)
provider-openai/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
llm-router/README.mdprovider-openai/.gitignoreprovider-openai/Cargo.tomlprovider-openai/README.mdprovider-openai/build.rsprovider-openai/iii-permissions.yamlprovider-openai/iii.worker.yamlprovider-openai/src/config.rsprovider-openai/src/curated.rsprovider-openai/src/discovery.rsprovider-openai/src/errors.rsprovider-openai/src/lib.rsprovider-openai/src/main.rsprovider-openai/src/manifest.rsprovider-openai/src/reasoning.rsprovider-openai/src/register.rsprovider-openai/src/request.rsprovider-openai/src/router_client.rsprovider-openai/src/sse.rsprovider-openai/src/state.rsprovider-openai/src/stream_fn.rsprovider-openai/src/upstream.rsprovider-openai/src/wire/messages.rsprovider-openai/src/wire/mod.rsprovider-openai/src/wire/names.rsprovider-openai/src/wire/tools.rsprovider-openai/tests/integration.rs
| let credential_value = match &resolved.credential { | ||
| Some(credential) => credential_parts(credential).to_string(), | ||
| None => return Err(NotConfigured), | ||
| }; |
There was a problem hiding this comment.
Treat empty credential secrets as NotConfigured too.
Line 37–40 currently accepts Some(Credential) even when the extracted bearer token is empty, which defers a deterministic config error into an upstream 401 path.
Suggested patch
- let credential_value = match &resolved.credential {
- Some(credential) => credential_parts(credential).to_string(),
- None => return Err(NotConfigured),
- };
+ let credential_value = match &resolved.credential {
+ Some(credential) => {
+ let v = credential_parts(credential).trim();
+ if v.is_empty() {
+ return Err(NotConfigured);
+ }
+ v.to_string()
+ }
+ None => return Err(NotConfigured),
+ };📝 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.
| let credential_value = match &resolved.credential { | |
| Some(credential) => credential_parts(credential).to_string(), | |
| None => return Err(NotConfigured), | |
| }; | |
| let credential_value = match &resolved.credential { | |
| Some(credential) => { | |
| let v = credential_parts(credential).trim(); | |
| if v.is_empty() { | |
| return Err(NotConfigured); | |
| } | |
| v.to_string() | |
| } | |
| None => return Err(NotConfigured), | |
| }; |
🤖 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/config.rs` around lines 37 - 40, The match on
resolved.credential currently treats any Some(credential) as configured even
when credential_parts(credential) yields an empty string; change the logic in
the block handling resolved.credential so that you call
credential_parts(credential), trim/check the resulting string and if it's empty
return Err(NotConfigured) instead of assigning credential_value. Update the
branch that currently sets credential_value to perform this empty-check (use the
existing credential_parts helper and the NotConfigured error) so empty bearer
tokens are treated as not configured.
| fn family_meta(base: &str) -> Option<(&'static str, u64, u64, bool, Pricing)> { | ||
| match base { | ||
| "gpt-5.2" => Some(("GPT-5.2", 400_000, 128_000, true, price(1.75, 14.0))), | ||
| "gpt-5.1" => Some(("GPT-5.1", 400_000, 128_000, false, price(1.25, 10.0))), | ||
| "gpt-5-mini" => Some(("GPT-5 Mini", 400_000, 128_000, false, price(0.25, 2.0))), | ||
| "gpt-5-nano" => Some(("GPT-5 Nano", 400_000, 128_000, false, price(0.05, 0.40))), | ||
| _ => None, |
There was a problem hiding this comment.
Add metadata for the bare gpt-5 alias.
is_legacy_generation() explicitly keeps gpt-5, and discovery::parse_live_models() will publish it if /v1/models returns that alias, but family_meta() falls through to the unknown-family defaults here. The router would then advertise gpt-5 with max_output_tokens: 16_384, no structured-output/thinking flags, and no pricing even though this code treats it as a current chat model. Add an explicit gpt-5 row or normalize that alias before calling enrich().
🤖 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/curated.rs` around lines 37 - 43, family_meta currently
has no entry for the bare "gpt-5" alias, so models retained by
is_legacy_generation() and discovery::parse_live_models() fall through to
defaults; add an explicit "gpt-5" match arm in family_meta (e.g., map to
("GPT-5", same token limits/flags/pricing as the intended current GPT‑5 family
member) so it gets proper max_output_tokens, structured-output/thinking flags,
and pricing), or alternatively normalize the alias to a known family (e.g.,
rewrite "gpt-5" to "gpt-5.1" or "gpt-5.2") before calling enrich(); update
family_meta (and/or the normalization location) to reference the chosen family
consistently.
| pub fn models_url(api_url: &str) -> String { | ||
| match api_url.strip_suffix("/chat/completions") { | ||
| Some(base) => format!("{base}/models"), | ||
| None => "https://api.openai.com/v1/models".to_string(), | ||
| } |
There was a problem hiding this comment.
Keep /v1/models on the configured host.
If resolved.api_url points at a proxy or custom base and does not literally end with /chat/completions, this helper silently falls back to https://api.openai.com/v1/models. refresh_models() then sends the resolved Bearer credential there, which both breaks catalog refresh for custom deployments and leaks the secret to the wrong upstream. Return an error/skip refresh instead of changing hosts, or derive /models from the same origin/path prefix.
| pub async fn register_provider(iii: III) -> Result<(), IIIError> { | ||
| // Streaming uses no total timeout (the router owns stream budgets); | ||
| // connect failures surface fast. | ||
| let http = reqwest::Client::builder() | ||
| .connect_timeout(Duration::from_secs(10)) | ||
| .build() | ||
| .expect("reqwest client"); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In reqwest's ClientBuilder, if only connect_timeout is set and no timeout/read_timeout is configured, do requests and response body reads have an infinite default timeout?
💡 Result:
Yes, if you only set the connect_timeout in reqwest's ClientBuilder, there is no default timeout for the rest of the request (sending the request and reading the response body) [1][2]. In the async implementation, the default timeout for the overall request and for individual read operations is None [2], meaning these operations will not time out automatically unless specifically configured [1][2]. It is important to distinguish between the different types of timeouts available in reqwest: 1. connect_timeout: Specifies a limit only for the TCP connection establishment phase [3][1]. 2. timeout: Specifies a deadline for the entire request lifecycle (from connection start until the response body has finished) [3][1]. 3. read_timeout: Specifies a timeout for each individual read operation, which is useful for detecting stalled connections during body streaming [1][4][2]. If you do not set the global timeout or a read_timeout, requests will wait indefinitely for the server to respond and send the full response body [1][2]. Note that the blocking client in reqwest differs from the async client in its default behavior, often including a 30-second default for its general timeout [5][6], but for the standard async ClientBuilder, no such automatic limit exists when only connect_timeout is set [1][2].
Citations:
- 1: https://deepwiki.com/seanmonstar/reqwest/3.1-client-configuration
- 2: https://github.com/seanmonstar/reqwest/blob/master/src/async_impl/client.rs
- 3: https://arcmutex.com/content/reqwest-timeout-vs-connect-timeout-request-lifecycle
- 4: Add read_timeout option seanmonstar/reqwest#2237
- 5: Misleading description for ClientBuilder timeout in the docs seanmonstar/reqwest#1799
- 6: https://github.com/seanmonstar/reqwest/blob/5397d2cf8eaecc9f964ab4d2b40b8f2ad24ce26d/src/blocking/client.rs
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and display the relevant code in register.rs around the provided line range
rg -n "pub async fn register_provider" -S provider-openai/src/register.rs
sed -n '80,140p' provider-openai/src/register.rs
# Find where fetch_live_models / refresh_models are implemented and inspect timeout usage
rg -n "fetch_live_models|refresh_models" -S provider-openai/src
# Show discovery.rs relevant sections
DISCOVERY_FILE=$(fd -t f "discovery.rs" provider-openai/src | head -n 1 || true)
echo "Discovery file: $DISCOVERY_FILE"
if [ -n "${DISCOVERY_FILE:-}" ]; then
rg -n "fetch_live_models" "$DISCOVERY_FILE"
rg -n "v1/models|/v1/models" "$DISCOVERY_FILE"
# print a reasonable window around the functions
# (use sed windows around matches if possible)
# First, show entire function blocks by grabbing lines around occurrences
sed -n '1,220p' "$DISCOVERY_FILE" | sed -n '1,220p'
fi
# Also inspect any place where the reqwest client is constructed or passed into discovery
rg -n "reqwest::Client::builder|connect_timeout|timeout\(|read_timeout\(" -S provider-openai/srcRepository: iii-hq/workers
Length of output: 11989
Add a request/response timeout for model refresh (/v1/models) to prevent indefinite hangs
register_provider builds a reqwest::Client with only connect_timeout and reuses it for both streaming and provider::openai::refresh_models. provider-openai/src/discovery.rs::fetch_live_models performs a normal GET /v1/models and then resp.json::<Value>().await without any timeout/read_timeout, so if the upstream stalls after the TCP connection succeeds, the refresh task can wait indefinitely (reqwest async timeout defaults to None unless configured). Create a separate client for discovery/refresh with .timeout(...)/.read_timeout(...) (or wrap send/json in tokio::time::timeout) instead of sharing the stream client.
🤖 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/register.rs` around lines 100 - 106, register_provider
currently builds a reqwest::Client with only connect_timeout and reuses it for
streaming and discovery, which lets provider::openai::refresh_models /
provider-openai::discovery::fetch_live_models hang if the upstream stalls; fix
by creating a separate reqwest client for model refresh (e.g., new client in
register_provider or inside refresh_models/fetch_live_models) that sets a full
request timeout via .timeout(...) or .read_timeout(...), or alternatively wrap
the GET/json calls in tokio::time::timeout, and ensure register_provider
continues to use the original client for streaming while the discovery path uses
the timeout-enabled client.
| pub async fn models_get(iii: &III, model_id: &str) -> Option<Model> { | ||
| let raw = call( | ||
| iii, | ||
| "router::models::get", | ||
| json!({ "provider": PROVIDER_ID, "id": model_id }), | ||
| ) | ||
| .await | ||
| .ok()?; | ||
| serde_json::from_value(raw.get("model")?.clone()).ok() |
There was a problem hiding this comment.
models_get currently masks router call failures as “model absent”.
Line 54 (.await.ok()?) drops all IIIError variants, so timeout/auth/bus failures become indistinguishable from a legitimate miss. Downstream callers then take fallback paths silently.
Suggested fix
-pub async fn models_get(iii: &III, model_id: &str) -> Option<Model> {
- let raw = call(
+pub async fn models_get(iii: &III, model_id: &str) -> Result<Option<Model>, IIIError> {
+ let raw = call(
iii,
"router::models::get",
json!({ "provider": PROVIDER_ID, "id": model_id }),
)
- .await
- .ok()?;
- serde_json::from_value(raw.get("model")?.clone()).ok()
+ .await?;
+ match raw.get("model") {
+ Some(v) if !v.is_null() => serde_json::from_value(v.clone())
+ .map(Some)
+ .map_err(|e| IIIError::Remote {
+ code: "provider/bad_models_get_response".into(),
+ message: e.to_string(),
+ stacktrace: None,
+ }),
+ _ => Ok(None),
+ }
}📝 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.
| pub async fn models_get(iii: &III, model_id: &str) -> Option<Model> { | |
| let raw = call( | |
| iii, | |
| "router::models::get", | |
| json!({ "provider": PROVIDER_ID, "id": model_id }), | |
| ) | |
| .await | |
| .ok()?; | |
| serde_json::from_value(raw.get("model")?.clone()).ok() | |
| pub async fn models_get(iii: &III, model_id: &str) -> Result<Option<Model>, IIIError> { | |
| let raw = call( | |
| iii, | |
| "router::models::get", | |
| json!({ "provider": PROVIDER_ID, "id": model_id }), | |
| ) | |
| .await?; | |
| match raw.get("model") { | |
| Some(v) if !v.is_null() => serde_json::from_value(v.clone()) | |
| .map(Some) | |
| .map_err(|e| IIIError::Remote { | |
| code: "provider/bad_models_get_response".into(), | |
| message: e.to_string(), | |
| stacktrace: None, | |
| }), | |
| _ => Ok(None), | |
| } | |
| } |
🤖 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/router_client.rs` around lines 47 - 55, The function
models_get currently swallows call errors via `.await.ok()?`, making
timeouts/auth/bus failures look like "no model"; change models_get to return a
Result<Option<Model>, IIIError> (instead of Option<Model>) and propagate the
call error instead of discarding it: await the `call(...)` and `?` the Result to
return the underlying IIIError, then handle the case where `raw.get("model")` is
None by returning Ok(None) and convert serde deserialization failures into an
IIIError (or map them to an appropriate error variant) rather than silencing
them; use the existing symbols models_get, call, IIIError, and PROVIDER_ID to
locate and update the logic.
| let reasoning_effort = if is_reasoning_model( | ||
| &model, | ||
| model_meta.as_ref().and_then(|m| m.supports_thinking), | ||
| ) { | ||
| let effort = reasoning_effort_for(input.thinking_level, &model); | ||
| if input.thinking_level.is_some() && effort.is_none() { |
There was a problem hiding this comment.
Reasoning effort is computed from input.model, but the API call uses cfg.model.
Line 115 sends cfg.model upstream, while Lines 92 and 96 classify/map effort using model from input. If resolve remaps aliases, reasoning_effort and warning behavior can be wrong for the actual target model.
Suggested fix
- let model = input.model.clone();
+ let model = input.model.clone();
+ let target_model = cfg.model.clone();
@@
- let model_meta = match input.model_meta {
+ let model_meta = match input.model_meta {
Some(m) => Some(m),
- None => router_client::models_get(iii, &model).await,
+ None => router_client::models_get(iii, &target_model).await,
};
- let reasoning_effort = if is_reasoning_model(
- &model,
+ let reasoning_effort = if is_reasoning_model(
+ &target_model,
model_meta.as_ref().and_then(|m| m.supports_thinking),
) {
- let effort = reasoning_effort_for(input.thinking_level, &model);
+ let effort = reasoning_effort_for(input.thinking_level, &target_model);Also applies to: 114-116
🤖 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 92 - 97, The reasoning_effort
and related checks use the original input model variable (e.g., calls to
is_reasoning_model and reasoning_effort_for with &model and
input.thinking_level) but the request actually sends cfg.model (the resolved
model/alias) upstream; update the logic to compute reasoning_effort and the
warning branch using the resolved model in cfg (use cfg.model or the resolved
model variable) so the classification/mapping and warning behavior reflect the
actual target model sent in the API call, and ensure any other nearby checks
(the ones around lines where reasoning_effort is used and where cfg.model is
sent) are changed consistently.
| let mut entry = json!({ "role": "assistant" }); | ||
| if !text.is_empty() { | ||
| entry["content"] = Value::String(text); | ||
| } | ||
| if !tool_calls.is_empty() { | ||
| entry["tool_calls"] = Value::Array(tool_calls); | ||
| } | ||
| out.push(entry); | ||
| // Placeholders for orphans go directly after the assistant |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show file around the referenced lines
FILE="provider-openai/src/wire/messages.rs"
echo "== $FILE =="
python3 - <<PY
import itertools
path="$FILE"
start=100
end=200
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f,1):
if start<=i<=end:
print(f"{i:4d}: {line.rstrip()}")
PY
# Find all assistant entry constructions and any related skip/filter logic
rg -n "thinking|redacted|Thinking|redacted-thinking|role\"\\s*:\\s*\"assistant\"|tool_calls|out\\.push\\(" "$FILE"
# Check if entry.get("content") pattern already exists elsewhere in the repo (serde_json Value usage)
rg -n "entry\\.get\\(\"content\"\\)|\\.get\\(\"tool_calls\"\\)" provider-openai/src || true
# Look for validation/schema for the "wire" rows / outgoing messages
# (search in provider-openai and maybe common request builders)
rg -n "content.*tool_calls|tool_calls.*content|Messages|wire.*row|invalid.*assistant|assistant row" provider-openai/src || trueRepository: iii-hq/workers
Length of output: 6223
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="provider-openai/src/wire/messages.rs"
echo "== Test section around thinking_blocks_and_result_images_are_dropped() =="
python3 - <<PY
path="$FILE"
start=320
end=410
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f,1):
if start<=i<=end:
print(f"{i:4d}: {line.rstrip()}")
PY
echo "== Look for any filtering of wire messages before sending (provider-openai) =="
rg -n "get\\(\"content\"\\)|get\\(\"tool_calls\"\\)|empty assistant|skip.*assistant|filter.*tool_calls|filter.*assistant|drop.*assistant" provider-openai/src || true
echo "== Where the wire array is used to build OpenAI requests =="
rg -n "wire\\b|messages\\b.*wire|to_wire|from_wire|chat\\b.*messages" provider-openai/src || trueRepository: iii-hq/workers
Length of output: 10457
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="provider-openai/src/request.rs"
echo "== $FILE =="
python3 - <<PY
path="$FILE"
start=1
end=200
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f,1):
if start<=i<=end:
print(f"{i:4d}: {line.rstrip()}")
PY
echo "== Search for any filtering/validation of assistant rows with missing content/tool_calls =="
rg -n "role\"\\s*:\\s*\"assistant\"|\\[\"role\"\\]\\s*==\\s*\"assistant\"|assistant.*(content|tool_calls)|tool_calls.*assistant" provider-openai/src || true
rg -n "get\\(\"content\"\\)|get\\(\"tool_calls\"\\)|content\\s*\\.\\s*is_none|tool_calls\\s*\\.\\s*is_none|content\\s*==\\s*Null" provider-openai/src || true
echo "== Search tests for assistant with only Thinking/RedactedThinking (no Text) =="
rg -n "ContentBlock::Thinking|ContentBlock::RedactedThinking" provider-openai/src/wire/messages.rsRepository: iii-hq/workers
Length of output: 8858
🌐 Web query:
OpenAI Chat Completions API assistant message role requires content or tool_calls validation
💡 Result:
In the Chat Completions API, an assistant message (role = "assistant") must include content unless tool_calls (or the deprecated function_call) is specified. - The assistant message schema documents content as: “Required unless tool_calls or function_call is specified.” [1][2] - The same schema also defines tool_calls as the tool calls generated by the model. [1][2] - This is consistent with the typical function/tool-calling flow: when the model responds with tool_calls, you execute the tools and append tool result messages, then call the API again; the example explicitly notes that tool-role messages must be responses to a preceding message with tool_calls. [3] Practical validation guidance (what to send): - If message.tool_calls is present (non-empty), you should omit content (or allow it to be null/absent per the client type), and rely on tool_calls to carry the structured tool request. [1][2] - If message.tool_calls is absent, then message.content must be provided (text or content parts, depending on your SDK/model). [1][2] Why you may see “requires content or tool_calls”: - Your payload is likely validating against the ChatCompletionAssistantMessageParam type where content is conditionally required. If neither content nor tool_calls is provided on an assistant message, the validator rejects it. [1][2] Sources: [1] https://github.com/openai/openai-python/blob/5e8f09c2/src/openai/types/chat/chat_completion_assistant_message_param.py [2] https://openai-hd4n6.mintlify.app/api-reference/chat/create-chat-completion [3] https://developers.openai.com/cookbook/examples/how_to_call_functions_with_chat_models
Citations:
- 1: https://github.com/openai/openai-python/blob/5e8f09c2/src/openai/types/chat/chat_completion_assistant_message_param.py
- 2: https://openai-hd4n6.mintlify.app/api-reference/chat/create-chat-completion
- 3: https://developers.openai.com/cookbook/examples/how_to_call_functions_with_chat_models
Skip emitting empty assistant turns (no content and no tool_calls)
to_wire_messages unconditionally pushes json!({ "role": "assistant" }) even when text and tool_calls are both empty (e.g., an assistant turn containing only dropped Thinking/RedactedThinking blocks). OpenAI Chat Completions requires content unless tool_calls (or deprecated function_call) is provided, so this can produce a 400 validation error.
Proposed fix
if !tool_calls.is_empty() {
entry["tool_calls"] = Value::Array(tool_calls);
}
- out.push(entry);
+ if entry.get("content").is_some() || entry.get("tool_calls").is_some() {
+ out.push(entry);
+ }🤖 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/wire/messages.rs` around lines 145 - 153, The function
to_wire_messages currently always creates and pushes an assistant entry (entry =
json!({ "role": "assistant" })) even when both text and tool_calls are empty,
which can cause OpenAI validation errors; change the logic in to_wire_messages
so you only construct/push the assistant entry (the entry variable and the
out.push(entry) call) when either text is non-empty or tool_calls is non-empty,
and ensure the existing comment/behavior about placing placeholders for orphans
"directly after the assistant" is preserved by only inserting those placeholders
after an actual assistant entry has been emitted.
| pub fn encode_tool_name(name: &str) -> String { | ||
| name.replace("::", "__") | ||
| } | ||
|
|
||
| pub fn decode_tool_name(name: &str) -> String { | ||
| name.replace("__", "::") | ||
| } |
There was a problem hiding this comment.
Tool-name codec is lossy for ids containing __.
At Lines 8-9, decoding blindly rewrites every __ to ::. That makes the codec non-reversible (decode(encode("a__b")) != "a__b"), which can rewrite function ids and break tool execution routing.
A collision-free encoding/decoding scheme (or a per-request encoded-name map) is needed here.
🤖 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/wire/names.rs` around lines 4 - 10, The current
encode_tool_name/decode_tool_name is lossy for names containing "__"; replace
the ad-hoc replace scheme with a reversible encoding (e.g., base64-url or
percent-encoding) so encode_tool_name(name: &str) returns a collision-free
encoded string and decode_tool_name reverses it back; update both functions
(encode_tool_name and decode_tool_name) to use the chosen reversible
encoder/decoder so decode(encode(x)) == x for any input, and add a short unit
test exercising names with "__" and "::" to verify round-trip correctness.
| fn free_port() -> u16 { | ||
| std::net::TcpListener::bind("127.0.0.1:0") | ||
| .expect("bind ephemeral port") | ||
| .local_addr() | ||
| .expect("local addr") | ||
| .port() | ||
| } |
There was a problem hiding this comment.
Avoid the free-port TOCTOU during engine startup.
free_port() releases the ephemeral listener before spawn_engine() launches iii, so another process can claim that port and make these tests fail intermittently. Retrying startup with a fresh port (or keeping the reservation until the engine is listening, if the engine supports that flow) would remove a real flake source.
🤖 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/tests/integration.rs` around lines 42 - 48, The helper
free_port() is vulnerable to a TOCTOU race because it relinquishes the ephemeral
port before spawn_engine() starts iii; change the test startup to reserve the
port (keep the TcpListener open) until the engine is confirmed listening or
implement a retry loop that picks a fresh port and retries spawn_engine() on
failure; specifically modify free_port() usage or replace it with a
reserve_port() that returns a TcpListener (or a (port, listener) pair) and
ensure spawn_engine() is invoked while the listener is held, or wrap
spawn_engine() in a small retry/backoff that retries on bind/address-in-use
errors to eliminate the intermittent race.
| "cost filled: {res}" | ||
| ); | ||
|
|
||
| let _ = tokio::time::timeout(Duration::from_secs(5), pump).await; |
There was a problem hiding this comment.
Don't ignore timeout(..., pump) failures in chat_streams_end_to_end_with_cost_fill() and upstream_401_surfaces_as_auth_expired_error_frame().
If the 5s timeout fires in either test, dropping the JoinHandle detaches read_all() and the test keeps going. That hides broken stream shutdown and can leave a background task running until later worker teardown. Fail on Err(_), and surface JoinError separately, before reading frames.
🤖 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/tests/integration.rs` at line 377, The test currently ignores
the result of tokio::time::timeout(Duration::from_secs(5), pump).await, which
can hide a timeout or task panic; update both
chat_streams_end_to_end_with_cost_fill() and
upstream_401_surfaces_as_auth_expired_error_frame() to assert the timeout
succeeded (fail the test on Err from timeout) and then separately assert the
JoinHandle completed successfully (surface JoinError from awaiting the
JoinHandle) before proceeding to read frames. In practice replace the ignored
let _ = ... with checking the timeout Result (expect or assert on Err) to fail
on elapsed, then await the returned JoinHandle and fail/assert on its JoinError,
and only after that continue to read frames.
Adds the initial crate scaffold for the provider-openai worker:
Cargo.toml, build.rs, src/{lib,manifest,main}.rs, iii.worker.yaml,
iii-permissions.yaml, and .gitignore. Includes a passing TDD test
(json_roundtrip_has_required_fields) for the manifest module.
…oundary sanitization
…ive structured output
GET /v1/models is now the source of truth for the catalog's id list: the registration declaration carries no static models and a refresh fires right after registering. OpenAI's models API returns bare ids — no capability tree, display names, or limits — so a local metadata table enriches known families and conservative defaults cover the rest; pricing stays local for the same reason. Two filters shape the slice: known legacy generations (gpt-3*/gpt-4*/ chatgpt*/o1/o3/o4 — a name denylist, since there is no capability data to key off; unrecognized future families pass through) are dropped, and dated snapshots fold into their undated alias when both are live. Against the real API this takes the picker from 70 rows to 23, all current-generation.
- main.rs: clap CLI (--config/--url/--manifest), tracing init, WorkerMetadata identity, and shutdown_async on exit; --url honours III_WS_URL as a fallback. A --config file carrying real keys now warns instead of being silently ignored (provider config comes from the llm-router configuration entry). - config.yaml: committed stub documenting where configuration lives. - Release wiring: added to create-tag.yml options and release.yml tag patterns. Version pre-bumped to 0.3.0 above the retired bundled Node provider's tags (provider-openai/v0.1.0–v0.2.1) so the first Create Tag run cannot collide. - README: Modules table row; worker README documents the CLI flags.
bea21b7 to
df52d2f
Compare
2 participants
Summary
Standalone llm-router plugin worker for the OpenAI Chat Completions API, structurally mirroring
provider-anthropic/(#247): token-gated registration withrouter::readyrebind, live model discovery reconciled through the router's single write path, SSE →AssistantMessageEventstreaming with ping watchdog and abort-on-channel-close, and typed upstream error classification.OpenAI-specific surfaces
reasoning.rs):thinking_levelmaps toreasoning_effortper model family — each ladder branch traces to a documented 400 (o1 rejects the param entirely, chat-tuned variants take only the fixed default, pro tiers are high-only, xhigh is gpt-5.2+). Non-reasoning families omit the param and degrade with a warning.response_formatpasses through asjson_schema(the Anthropic provider reports-and-ignores it).GET /v1/modelsreturns bare ids (no capabilities, names, or limits — unlike Anthropic's API), so a local family-metadata table enriches known families and a name denylist drops known legacy generations (gpt-3*/gpt-4*/chatgpt*/o1/o3/o4; unrecognized future families pass through). Dated snapshots fold into their undated alias when both are live. Against the real API: 70 chat models → 23 current-generation rows.Behavior notes
router::provider::listshowsconfigured: false; chats fail loudly with a classified error.OPENAI_API_KEY) resolves in the llm-router process, same as every provider.Test plan
cargo test— 56 unit + 5 engine-backed integration (registration/token persistence, live-only catalog, filtered reconcile, chat with cost fill, upstream 401 classification, router-restart redeclare)engine::workers::listround-trip), multi-turn recall, reasoning effort (thinking high on gpt-5.1 → correct bat-and-ball answer; degrade on gpt-4o-mini pre-filter), mid-stream abort, and the filtered catalog (23 rows, picker clean, chat on gpt-5 mini)Summary by CodeRabbit
New Features
Documentation
Tests
Chores