From 17fab5107140380dc526beb73a7b57d702ec28a7 Mon Sep 17 00:00:00 2001 From: Shibo Sheng Date: Mon, 12 Oct 2026 16:30:23 +0800 Subject: [PATCH] fix(agent-core): omit empty tools and stop invalid request retries OpenAI-compatible final summaries could send an empty tools list with an automatic tool choice. Strict providers reject that request, and the retry wrapper repeatedly sent the same invalid payload. Build tool parameters once for streaming and whole-response calls, reject missing forced tools locally, and classify remaining HTTP 400 responses as non-retryable invalid requests. Preserve modified-request recovery, fallback providers, and transient-error retries. Add HTTP contract tests and a loopback fixture for packaged-app verification. Verification: - cargo test -p agent_core --lib: 3,772 passed, 4 ignored - cargo clippy --all-targets -- -D warnings: passed - pnpm run lint; pnpm run check:circular; selected frontend tests: passed - node --test scripts/dev/provider-fixture-server.test.cjs: 2 passed - BuildFast packaged app: strict and lenient summaries, 400 fail-fast, 503 recovery, stop, restart, retry, and ordinary tools exercised The separate Org wake/finality stall under near-instant fixture responses remains outside this patch and is recorded in the verification document. Pre-commit hook ran. Total eslint: 0, total circular: 0 --- .../dev/org-request-contract-verification.md | 106 +++++++ scripts/dev/provider-fixture-server.cjs | 134 ++++++++ scripts/dev/provider-fixture-server.test.cjs | 42 +++ .../src/core/providers/openai_adaptive.rs | 8 +- .../core/providers/openai_compat/client.rs | 23 ++ .../src/core/providers/openai_compat/mod.rs | 5 + .../openai_compat/recovery_contract_tests.rs | 168 ++++++++++ .../openai_compat/request_contract_tests.rs | 288 ++++++++++++++++++ .../providers/openai_compat/streaming/chat.rs | 25 +- .../providers/openai_compat/streaming/mod.rs | 1 + .../openai_compat/streaming/sse_stream.rs | 24 +- .../openai_compat/streaming/tool_params.rs | 49 +++ .../src/core/providers/openai_compat/types.rs | 8 +- .../agent-core/src/core/providers/reliable.rs | 6 + .../agent-core/src/core/providers/traits.rs | 15 + .../src/core/session/turn/streaming.rs | 7 + .../crates/agent-core/src/core/side_query.rs | 9 + 17 files changed, 871 insertions(+), 47 deletions(-) create mode 100644 scripts/dev/org-request-contract-verification.md create mode 100644 scripts/dev/provider-fixture-server.cjs create mode 100644 scripts/dev/provider-fixture-server.test.cjs create mode 100644 src-tauri/crates/agent-core/src/core/providers/openai_compat/recovery_contract_tests.rs create mode 100644 src-tauri/crates/agent-core/src/core/providers/openai_compat/request_contract_tests.rs create mode 100644 src-tauri/crates/agent-core/src/core/providers/openai_compat/streaming/tool_params.rs diff --git a/scripts/dev/org-request-contract-verification.md b/scripts/dev/org-request-contract-verification.md new file mode 100644 index 0000000000..2b416b8ef4 --- /dev/null +++ b/scripts/dev/org-request-contract-verification.md @@ -0,0 +1,106 @@ +# Org request contract verification + +## Change and ownership + +Text-only turns omit both `tools` and `tool_choice` after internal tool metadata +and selection markers are removed. Streaming and whole-response requests share +this boundary. Forced selection of a missing tool fails locally before HTTP. + +`ProviderError::InvalidRequest` retains an optional HTTP status, parameter and +message. OpenAI-compatible HTTP 400 responses use this type after existing +special classifications. ReliableProvider does not repeat these requests to the +same provider; configured fallback providers remain eligible. Adaptive endpoint +and parameter repairs and side-query thinking recovery accept the new type. +Frontend conversion uses the existing UNKNOWN code with retryability false. + +No database migration, task mutation, queue policy, tool permission, automatic +summary fallback or runtime dependency was added. Reverting this change restores +the previous request/retry behavior without deleting tasks or certificates. + +| Contract | Owner | Proof | +| -------------------------------------- | ------------------------------ | --------------------------------------------------- | +| Empty tools omitted on both transports | Compatible request adapter | HTTP request contract tests | +| Invalid forced selection never sent | Shared tool parameter builder | Zero HTTP requests | +| HTTP 400 status/parameter retained | Error ingestion | JSON, text and malformed-body cases | +| Deterministic rejection not repeated | Reliable provider wrapper | Request counters and fallback case | +| Modified requests can recover | Adaptive client and side query | HTTP parameter/endpoint/thinking cases | +| Terminal summary evidence retained | Existing Org finality | Existing terminal/restart suites and app inspection | + +Architecture review covered tool authority, wire construction, error consumers, +terminal states and persistence ownership. The routed architecture-audit skill +is absent from this checkout; no formal skill audit is claimed. UI component, +schema migration and cloud/remote topology reviews are outside this change. + +## Local reproduction + +Run `node scripts/dev/provider-fixture-server.cjs 18099 /tmp/provider-requests.jsonl`. +Configure an isolated app account at `http://127.0.0.1:18099/v1`, model +`local-contract-model`, using a disposable fixture token. Send +`ORG_REQUEST_CONTRACT:normal` to the default Org. The fixture scripts model +responses only; real HTTP, task tools, scheduling, SQLite and finality execute. + +POST JSON `{"mode":"summary400"}` to `/control` to reject summaries; +`summary503` fails once then succeeds, `delay` allows Stop, and `strict`/`lenient` +exercise both validation policies. `/records` exposes bounded request records. +Use `FILE_REQUEST_CONTRACT` in an ordinary agent's disposable workspace to write +and read `verification.txt` through the real shell tool. + +## Verification performed (2026-10-12) + +The unmodified `c3480fcf3` baseline BuildFast app sent both `tools: []` and +`tool_choice: "auto"` for its final report. A strict loopback service returned +HTTP 400 eleven times before the report failed. Independent HTTP-boundary tests +against that baseline also failed for both streaming and whole responses. + +The repaired BuildFast app used an isolated profile and the same loopback +service. The Org coordinator created a task, a member completed it through +`task_update`, and `org_run_complete` produced a certificate. The final report +request omitted both tool fields and persisted once. A lenient service also +completed. A forced summary HTTP 400 produced one request and one failed +receipt. A temporary HTTP 503 produced two requests (503, then 200) and a +persisted report. Stopping a delayed report cancelled its HTTP request and +recorded a stopped receipt. The existing retry button created exactly one new +attempt for the same certificate and persisted it. Quitting and reopening the +packaged app preserved the session, task outputs and receipts without issuing a +new report request. An ordinary agent wrote and read `verification.txt` using +the real shell tool. + +Three normal / invalid-parameter / transient-recovery sequences were exercised. +In the second recovery, the scripted fixture returned a generic text response +while the Org reported transient in-flight blockers, so completion required a +new user message. This limits what the scripted run proves and is not counted +as automatic completion. The first and third sequences completed without that +prompt. The Command+5 diagnostics panel showed no recorded API calls; HTTP +records and read-only SQLite queries provide the request and persistence +evidence instead. + +An additional fresh-session repeat reproduced a separate acceptance gap under +the fixture's near-instant responses: after a successful report and one 400 +failure, the next member task completed, but the Org stayed running with no +certificate or final report for over three minutes. Its Coordinator turn ended +as `waiting_for_org_event`; two subsequent resume intents were cancelled. The +user message was marked answered, so this is an Org wake/finality gap rather +than a message left in the sending state. The cause of the cancelled wakes is +not proven. Queue and background mechanism changes are outside this patch; +repeated automatic recovery is therefore **not fully accepted** locally. The +user chose to deliver this request fix and handle the wake/finality gap in a +separate task. + +Automated checks: 3,772 Rust library tests passed (4 ignored); Clippy with +warnings denied, repository lint, circular-import check, 31 selected frontend +tests, the fixture tests and `git diff --check` passed. The company services and +Windows incident database are unavailable locally and are not claimed as +tested. Company testing is deferred to the user and is not a local gate. + +| Performance area | Verdict | Evidence | Change or reason kept | Verification | +| ---------------------- | ------- | ----------------------------------------------------------------------------- | -------------------------------------------------------------------------- | --------------------------------------------------------------------- | +| Background work | Keep | No new timer or poller; retry is request-scoped | Invalid requests exit immediately; transient failures retain bounded retry | One 400 request, two 503 requests, no idle requests after two minutes | +| Memory | Keep | No new retained production collection | Tool parameters are allocated once per request | 121 MB in foreground and hidden idle samples after repeated runs | +| Scope and isolation | Keep | BuildFast instance 99 used separate data and external-history homes and ports | No cache or identity changes | Restart preserved only the isolated test session | +| Rendering and hot path | Keep | No UI or per-delta parser change | Shared helper runs once before HTTP | Full Rust suite and foreground/hidden CPU samples | + +Performance verdict: pass for the changed provider path. Foreground idle was +0.0% CPU and hidden idle 0.7% CPU in point samples; an earlier active +conversation sample was 7.4%. These samples do not establish a long-term memory +trend or make a claim about company services. After shutdown, the isolated app +and test server processes were gone. diff --git a/scripts/dev/provider-fixture-server.cjs b/scripts/dev/provider-fixture-server.cjs new file mode 100644 index 0000000000..68f1e80801 --- /dev/null +++ b/scripts/dev/provider-fixture-server.cjs @@ -0,0 +1,134 @@ +#!/usr/bin/env node +// Loopback-only HTTP fixture: real clients, tools, scheduling and persistence. +const http = require("node:http"); +const fs = require("node:fs"); +const { randomUUID } = require("node:crypto"); + +const MODES = new Set(["strict", "lenient", "summary400", "summary503", "delay"]); +const text = (message) => typeof message.content === "string" ? message.content : + (message.content ?? []).map((block) => block.text ?? "").join("\n"); + +function scriptedResponse(body) { + const messages = body.messages ?? []; + const names = new Set((body.tools ?? []).map((tool) => tool.function?.name)); + const system = messages.filter((m) => m.role === "system").map(text).join("\n"); + const context = messages.filter((m) => m.role === "system" || m.role === "user").map(text).join("\n"); + const lastUser = messages.findLastIndex((m) => m.role === "user" && + !text(m).includes("")); + const user = lastUser < 0 ? "" : text(messages[lastUser]); + const results = messages.slice(lastUser + 1).filter((m) => m.role === "tool"); + const call = (name, args) => ({ role: "assistant", content: null, tool_calls: [{ + id: `fixture-${randomUUID()}`, type: "function", + function: { name, arguments: JSON.stringify(args) }, + }] }); + if (system.includes("You write one final user-facing Agent Org report")) { + return { role: "assistant", content: "Local verification task completed. Its certified text output is preserved." }; + } + if (names.has("org_run_complete") && context.includes("state=`ready`") && !results.length) { + return call("org_run_complete", { candidate_outcome: "delivered", summary: "Local task evidence verified", evidence_task_ids: [] }); + } + const taskId = user.match(/Task ID:\s*([\w-]+)/)?.[1] ?? user.match(/task_id="([\w-]+)"/)?.[1]; + if (taskId && names.has("task_update")) { + if (!results.length) return call("task_update", { operation: "start", id: taskId }); + if (results.length === 1) return call("task_update", { operation: "complete", id: taskId, + output: { summary: "Local HTTP boundary checked", content: "LOCAL_TASK_EVIDENCE: output produced through the real task tool", artifact_ids: [] } }); + } + if (user.includes("ORG_REQUEST_CONTRACT:") && names.has("task_graph_create") && !results.length) { + return call("task_graph_create", { allow_parallel_with_existing_open_tasks: true, + tasks: [{ key: "verification", subject: "Local HTTP contract verification", description: "Produce a text result for certification", + owner_member_id: "sde-reviewer", execution_mode: "build" }] }); + } + if (user.includes("FILE_REQUEST_CONTRACT") && names.has("run_shell") && !results.length) { + return call("run_shell", { command: "printf 'LOCAL_FILE_EVIDENCE\\n' > verification.txt && cat verification.txt" }); + } + return { role: "assistant", content: "Local fixture response complete." }; +} + +function startFixture({ port = 0, logPath = null } = {}) { + let mode = "strict"; + let summaryFailures = 0; + let count = 0; + const timers = new Set(); + const records = []; + const server = http.createServer(async (request, response) => { + const chunks = []; + let size = 0; + for await (const chunk of request) { + size += chunk.length; + if (size > 2 * 1024 * 1024) { response.writeHead(413).end(); return; } + chunks.push(chunk); + } + const json = (status, value) => { + response.writeHead(status, { "content-type": "application/json" }); + response.end(JSON.stringify(value)); + }; + if (request.url === "/v1/models" || request.url === "/models") { + json(200, { object: "list", data: [{ id: "local-contract-model", object: "model", owned_by: "fixture" }] }); return; + } + if (request.url === "/control" && request.method === "POST") { + let value; + try { value = JSON.parse(Buffer.concat(chunks)); } catch { json(400, { error: "Invalid JSON" }); return; } + if (!MODES.has(value.mode)) { json(400, { error: "Unknown mode" }); return; } + mode = value.mode; summaryFailures = 0; + json(200, { mode }); return; + } + if (request.url === "/records") { json(200, { mode, count, records }); return; } + if (!request.url.endsWith("/chat/completions")) { json(404, { error: "Unknown route" }); return; } + let body; + try { body = JSON.parse(Buffer.concat(chunks)); } catch { json(400, { error: { message: "Invalid JSON" } }); return; } + const summary = (body.messages ?? []).some((m) => m.role === "system" && text(m).includes("You write one final user-facing Agent Org report")); + const record = { id: ++count, time: new Date().toISOString(), mode, summary, + toolsPresent: Object.hasOwn(body, "tools"), toolChoicePresent: Object.hasOwn(body, "tool_choice"), + toolNames: (body.tools ?? []).map((tool) => tool.function?.name), body }; + records.push(record); + if (records.length > 200) records.shift(); + const finish = (status) => { record.status = status; if (logPath) fs.appendFileSync(logPath, `${JSON.stringify(record)}\n`); }; + const reject = (status, message, parameter) => { + finish(status); json(status, { error: { message, type: "invalid_request_error", param: parameter } }); + }; + if (mode !== "lenient" && Array.isArray(body.tools) && body.tools.length === 0) { + reject(400, "tools must not be an empty array", "tools"); return; + } + if (mode !== "lenient" && !body.tools?.length && body.tool_choice && body.tool_choice !== "none") { + reject(400, "tool_choice requires tools", "tool_choice"); return; + } + if (summary && mode === "summary400") { reject(400, "Fixture rejects summary parameters", "fixture_parameter"); return; } + if (summary && mode === "summary503" && summaryFailures++ === 0) { reject(503, "Temporarily unavailable", null); return; } + const send = () => { + if (response.destroyed) { finish("cancelled"); return; } + const message = scriptedResponse(body); + finish(200); + const reason = message.tool_calls ? "tool_calls" : "stop"; + if (body.stream) { + response.writeHead(200, { "content-type": "text/event-stream" }); + const delta = { ...message }; + delete delta.role; + if (delta.tool_calls) delta.tool_calls = delta.tool_calls.map((tool, index) => ({ ...tool, index })); + response.end(`data: ${JSON.stringify({ choices: [{ index: 0, delta, finish_reason: reason }] })}\n\ndata: [DONE]\n\n`); + } else { + json(200, { id: `fixture-${count}`, choices: [{ index: 0, message, finish_reason: reason }], usage: { prompt_tokens: 1, completion_tokens: 1 } }); + } + }; + if (summary && mode === "delay") { + const timer = setTimeout(() => { timers.delete(timer); send(); }, 60000); + timers.add(timer); + response.once("close", () => { if (timers.delete(timer)) { clearTimeout(timer); finish("cancelled"); } }); + } else send(); + }); + return new Promise((resolve, reject) => { + server.once("error", reject); + server.listen(port, "127.0.0.1", () => resolve({ + url: `http://127.0.0.1:${server.address().port}`, + close: async () => { for (const timer of timers) clearTimeout(timer); timers.clear(); server.closeAllConnections(); await new Promise((done) => server.close(done)); }, + })); + }); +} + +module.exports = { startFixture, scriptedResponse }; +if (require.main === module) { + startFixture({ port: Number(process.argv[2] ?? 18099), logPath: process.argv[3] ?? null }).then((fixture) => { + console.log(`Provider fixture: ${fixture.url}/v1`); + const stop = async () => { await fixture.close(); process.exit(0); }; + process.on("SIGINT", stop); process.on("SIGTERM", stop); + }); +} diff --git a/scripts/dev/provider-fixture-server.test.cjs b/scripts/dev/provider-fixture-server.test.cjs new file mode 100644 index 0000000000..3d42581bba --- /dev/null +++ b/scripts/dev/provider-fixture-server.test.cjs @@ -0,0 +1,42 @@ +const { test } = require("node:test"); +const assert = require("node:assert/strict"); +const { startFixture, scriptedResponse } = require("./provider-fixture-server.cjs"); + +test("fixture validates actual requests and can switch failure modes", async () => { + const fixture = await startFixture(); + try { + const send = (body) => fetch(`${fixture.url}/v1/chat/completions`, { + method: "POST", body: JSON.stringify(body), headers: { "content-type": "application/json" }, + }); + const mode = (value) => fetch(`${fixture.url}/control`, { method: "POST", body: JSON.stringify({ mode: value }) }); + const summary = { messages: [{ role: "system", content: "You write one final user-facing Agent Org report" }] }; + assert.equal((await send({ ...summary, tools: [] })).status, 400); + assert.equal((await send({ ...summary, tool_choice: "auto" })).status, 400); + assert.equal((await send(summary)).status, 200); + await mode("lenient"); + assert.equal((await send({ ...summary, tools: [] })).status, 200); + await mode("summary400"); + assert.equal((await send(summary)).status, 400); + await mode("summary503"); + assert.equal((await send(summary)).status, 503); + assert.equal((await send(summary)).status, 200); + const records = await (await fetch(`${fixture.url}/records`)).json(); + assert.equal(records.count, 7); + assert.equal(records.records[0].toolsPresent, true); + } finally { await fixture.close(); } +}); + +test("fixture scripts real task tools rather than mutating domain state", () => { + const reminder = { role: "user", content: [{ text: "test environment" }] }; + const result = scriptedResponse({ messages: [{ role: "user", content: "ORG_REQUEST_CONTRACT:normal" }, reminder], + tools: [{ function: { name: "task_graph_create" } }] }); + assert.equal(result.tool_calls[0].function.name, "task_graph_create"); + const args = JSON.parse(result.tool_calls[0].function.arguments); + assert.equal(args.tasks[0].owner_member_id, "sde-reviewer"); + assert.equal(scriptedResponse({ messages: [{ role: "user", content: "Task ID: abc-def" }], + tools: [{ function: { name: "task_update" } }] }).tool_calls[0].function.name, "task_update"); + assert.equal(scriptedResponse({ messages: [ + { role: "user", content: "task done" }, + { role: "user", content: [{ text: "state=`ready`" }] }, + ], tools: [{ function: { name: "org_run_complete" } }] }).tool_calls[0].function.name, "org_run_complete"); +}); diff --git a/src-tauri/crates/agent-core/src/core/providers/openai_adaptive.rs b/src-tauri/crates/agent-core/src/core/providers/openai_adaptive.rs index 3b6a468acf..0e15bc6edf 100644 --- a/src-tauri/crates/agent-core/src/core/providers/openai_adaptive.rs +++ b/src-tauri/crates/agent-core/src/core/providers/openai_adaptive.rs @@ -70,7 +70,9 @@ impl OpenAiAdaptiveClient { fn should_try_responses(&self, err: &ProviderError) -> bool { match err { - ProviderError::RequestFailed(body) | ProviderError::ModelNotFound(body) => { + ProviderError::RequestFailed(body) + | ProviderError::ModelNotFound(body) + | ProviderError::InvalidRequest { message: body, .. } => { openai_error_requires_responses(body) } _ => false, @@ -84,7 +86,9 @@ impl OpenAiAdaptiveClient { current_policy: OpenAiChatWirePolicy, ) -> bool { let body = match err { - ProviderError::RequestFailed(body) | ProviderError::ModelNotFound(body) => body, + ProviderError::RequestFailed(body) + | ProviderError::ModelNotFound(body) + | ProviderError::InvalidRequest { message: body, .. } => body, _ => return false, }; let Some(preference) = openai_chat_wire_preference_from_error(body, current_policy) else { diff --git a/src-tauri/crates/agent-core/src/core/providers/openai_compat/client.rs b/src-tauri/crates/agent-core/src/core/providers/openai_compat/client.rs index 1e3d0bef73..c460d123ae 100644 --- a/src-tauri/crates/agent-core/src/core/providers/openai_compat/client.rs +++ b/src-tauri/crates/agent-core/src/core/providers/openai_compat/client.rs @@ -190,6 +190,7 @@ impl OpenAICompatClient { } return match status { + 400 => Self::invalid_request(body, message), 401 => ProviderError::AuthError(message), 429 => ProviderError::RateLimited { message, @@ -205,6 +206,12 @@ impl OpenAICompatClient { } } + if status == 400 { + return Self::invalid_request( + body, + crate::providers::http_error_body::clean_error_message(status, body), + ); + } ProviderError::RequestFailed(format!( "HTTP {}: {}", status, @@ -212,6 +219,22 @@ impl OpenAICompatClient { )) } + fn invalid_request(body: &str, message: String) -> ProviderError { + let parameter = serde_json::from_str::(body).ok().and_then(|value| { + let error = value.get("error").unwrap_or(&value); + error + .get("param") + .and_then(Value::as_str) + .or_else(|| error.get("parameter").and_then(Value::as_str)) + .map(str::to_owned) + }); + ProviderError::InvalidRequest { + status: Some(400), + parameter, + message, + } + } + /// Reassemble an SSE-streamed body (sent by providers that ignore `stream:false`) /// into a single `LLMResponse`. pub(super) fn reassemble_sse_to_response(body: &str) -> Result { diff --git a/src-tauri/crates/agent-core/src/core/providers/openai_compat/mod.rs b/src-tauri/crates/agent-core/src/core/providers/openai_compat/mod.rs index b305d6baca..1a6b87f690 100644 --- a/src-tauri/crates/agent-core/src/core/providers/openai_compat/mod.rs +++ b/src-tauri/crates/agent-core/src/core/providers/openai_compat/mod.rs @@ -12,6 +12,11 @@ pub mod streaming; pub mod types; pub mod wire_expand; +#[cfg(test)] +mod recovery_contract_tests; +#[cfg(test)] +mod request_contract_tests; + pub use client::OpenAICompatClient; pub use streaming::STREAM_PARSE_ERROR_KEY; // `expand_tool_images_for_openai_wire` is reached only through the diff --git a/src-tauri/crates/agent-core/src/core/providers/openai_compat/recovery_contract_tests.rs b/src-tauri/crates/agent-core/src/core/providers/openai_compat/recovery_contract_tests.rs new file mode 100644 index 0000000000..2e3eac8f21 --- /dev/null +++ b/src-tauri/crates/agent-core/src/core/providers/openai_compat/recovery_contract_tests.rs @@ -0,0 +1,168 @@ +//! Existing request repairs must remain reachable through typed rejections. +use crate::core::side_query::{side_query, SideQueryConfig}; +use crate::providers::openai_adaptive::OpenAiAdaptiveClient; +use crate::providers::registry::{find_by_name, provider_id}; +use crate::providers::traits::{LLMProvider, ProviderConfig, ProviderError}; +use serde_json::{json, Value}; +use wiremock::{ + matchers::{method, path}, + Mock, MockServer, Request, ResponseTemplate, +}; + +fn config(server: &MockServer) -> ProviderConfig { + crate::test_support::install_crypto_provider_for_tests(); + ProviderConfig { + api_key: "fixture-key".into(), + api_base: Some(server.uri()), + extra_headers: Default::default(), + is_azure: false, + } +} + +fn success(request: &Request) -> ResponseTemplate { + let body: Value = request.body_json().unwrap(); + if body["stream"] == true { + ResponseTemplate::new(200).insert_header("content-type", "text/event-stream") + .set_body_string("data: {\"choices\":[{\"delta\":{\"content\":\"ok\"},\"finish_reason\":\"stop\"}]}\n\ndata: [DONE]\n\n") + } else { + ResponseTemplate::new(200).set_body_json(json!({"choices":[{ + "message":{"role":"assistant","content":"ok"},"finish_reason":"stop" + }]})) + } +} + +#[tokio::test] +async fn adaptive_client_changes_unsupported_token_parameter_after_typed_400() { + let _sandbox = test_helpers::test_env::sandbox(); + for stream in [false, true] { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/chat/completions")) + .respond_with(|request: &Request| { + let body: Value = request.body_json().unwrap(); + if body.get("max_tokens").is_some() { + ResponseTemplate::new(400).set_body_json(json!({"error":{ + "message":"Unsupported parameter: max_tokens", "param":"max_tokens" + }})) + } else { + assert_eq!(body["max_completion_tokens"], 64); + success(request) + } + }) + .mount(&server) + .await; + let client = OpenAiAdaptiveClient::new( + config(&server), + find_by_name(provider_id::OPENAI).unwrap(), + "fixture-model".into(), + Some(format!("fixture-{stream}")), + Some(server.uri()), + ); + let messages = [json!({"role":"user","content":"hello"})]; + let response = if stream { + client + .chat_streaming(&messages, None, "fixture-model", 64, 0.0, &|_| {}, None) + .await + } else { + client.chat(&messages, None, "fixture-model", 64, 0.0).await + }; + assert_eq!(response.unwrap().content.as_deref(), Some("ok")); + assert_eq!(server.received_requests().await.unwrap().len(), 2); + } +} + +#[tokio::test] +async fn adaptive_client_can_switch_endpoint_after_typed_400() { + let _sandbox = test_helpers::test_env::sandbox(); + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/chat/completions")) + .respond_with(ResponseTemplate::new(400).set_body_json(json!({"error":{ + "message":"Chat completions not supported with this model; use /responses" + }}))) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/responses")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "id":"response", "status":"completed", "output":[{"type":"message", "role":"assistant", + "content":[{"type":"output_text","text":"ok"}]}], + "usage":{"input_tokens":1,"output_tokens":1,"total_tokens":2} + }))) + .mount(&server) + .await; + let client = OpenAiAdaptiveClient::new( + config(&server), + find_by_name(provider_id::OPENAI).unwrap(), + "fixture-model".into(), + Some("endpoint-fixture".into()), + Some(server.uri()), + ); + let response = client + .chat( + &[json!({"role":"user","content":"hello"})], + None, + "fixture-model", + 64, + 0.0, + ) + .await + .unwrap(); + assert_eq!(response.content.as_deref(), Some("ok")); + assert_eq!(server.received_requests().await.unwrap().len(), 2); +} + +#[tokio::test] +async fn side_query_keeps_thinking_recovery_after_typed_400() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .respond_with(|request: &Request| { + let body: Value = request.body_json().unwrap(); + if body["max_tokens"] == 64 { + ResponseTemplate::new(400) + .set_body_json(json!({"error":{"message":"thinking disabled rejected"}})) + } else { + success(request) + } + }) + .mount(&server) + .await; + let client = super::OpenAICompatClient::new( + config(&server), + find_by_name(provider_id::VLLM).unwrap(), + "fixture-model".into(), + ); + let result = side_query( + &client, + &[json!({"role":"user","content":"hello"})], + &SideQueryConfig { + max_tokens: 64, + ..Default::default() + }, + "fixture-model", + ) + .await + .unwrap(); + assert_eq!(result.content, "ok"); + let requests = server.received_requests().await.unwrap(); + assert_eq!(requests.len(), 2); + let body: Value = requests[1].body_json().unwrap(); + assert_eq!(body["max_tokens"], 2112); +} + +#[test] +fn parameter_rejection_is_not_projected_as_retryable_network_failure() { + let error = crate::core::session::turn::streaming::StreamingError::from( + ProviderError::InvalidRequest { + status: Some(400), + parameter: Some("tools".into()), + message: "invalid tools".into(), + }, + ); + assert_eq!(error.error_code.wire_value(), "UNKNOWN"); + assert!(!error.is_retryable); + assert_eq!( + error.details.unwrap(), + json!({"reason":"invalid_request","status":400,"parameter":"tools"}) + ); +} diff --git a/src-tauri/crates/agent-core/src/core/providers/openai_compat/request_contract_tests.rs b/src-tauri/crates/agent-core/src/core/providers/openai_compat/request_contract_tests.rs new file mode 100644 index 0000000000..6b2a1e5345 --- /dev/null +++ b/src-tauri/crates/agent-core/src/core/providers/openai_compat/request_contract_tests.rs @@ -0,0 +1,288 @@ +//! Contracts at the actual HTTP boundary, including the reliable wrapper. +use super::OpenAICompatClient; +use crate::providers::registry::{find_by_name, provider_id}; +use crate::providers::reliable::ReliableProvider; +use crate::providers::traits::{LLMProvider, LLMResponse, ProviderConfig, ProviderError}; +use serde_json::{json, Value}; +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::sync::Arc; +use wiremock::{matchers::method, Mock, MockServer, Request, ResponseTemplate}; + +fn client(server: &MockServer) -> OpenAICompatClient { + crate::test_support::install_crypto_provider_for_tests(); + OpenAICompatClient::new( + ProviderConfig { + api_key: "fixture-key".into(), + api_base: Some(server.uri()), + extra_headers: Default::default(), + is_azure: false, + }, + find_by_name(provider_id::VLLM).unwrap(), + "fixture-model".into(), + ) +} + +async fn call( + provider: &dyn LLMProvider, + tools: Option<&[Value]>, + stream: bool, +) -> Result { + let messages = [json!({"role":"user","content":"summarize"})]; + if stream { + provider + .chat_streaming(&messages, tools, "fixture-model", 64, 0.0, &|_| {}, None) + .await + } else { + provider + .chat(&messages, tools, "fixture-model", 64, 0.0) + .await + } +} + +fn success(request: &Request) -> ResponseTemplate { + let body: Value = request.body_json().unwrap(); + if body["stream"] == true { + ResponseTemplate::new(200).insert_header("content-type", "text/event-stream") + .set_body_string("data: {\"choices\":[{\"delta\":{\"content\":\"ok\"},\"finish_reason\":\"stop\"}]}\n\ndata: [DONE]\n\n") + } else { + ResponseTemplate::new(200).set_body_json(json!({ + "choices":[{"message":{"role":"assistant","content":"ok"},"finish_reason":"stop"}] + })) + } +} + +fn tool() -> Value { + json!({"type":"function","function":{"name":"emit","parameters":{"type":"object"}}}) +} + +fn choice(value: Value) -> Value { + json!({"_orgii_tool_choice_override":value}) +} + +#[tokio::test] +async fn text_only_requests_omit_both_fields_after_internal_markers_are_removed() { + for stream in [false, true] { + for tools in [None, Some(vec![]), Some(vec![choice(json!("auto"))])] { + let server = MockServer::start().await; + Mock::given(method("POST")) + .respond_with(|request: &Request| { + let body: Value = request.body_json().unwrap(); + if body.get("tools").is_some() || body.get("tool_choice").is_some() { + ResponseTemplate::new(400) + .set_body_json(json!({"error":{"message":"invalid tool parameters"}})) + } else { + success(request) + } + }) + .mount(&server) + .await; + assert_eq!( + call(&client(&server), tools.as_deref(), stream) + .await + .unwrap() + .content + .as_deref(), + Some("ok") + ); + let requests = server.received_requests().await.unwrap(); + assert_eq!(requests.len(), 1); + let body: Value = requests[0].body_json().unwrap(); + assert!(body.get("tools").is_none()); + assert!(body.get("tool_choice").is_none()); + } + } +} + +#[tokio::test] +async fn normal_and_forced_tools_survive_without_internal_metadata() { + for stream in [false, true] { + for forced in [false, true] { + let server = MockServer::start().await; + Mock::given(method("POST")) + .respond_with(success) + .mount(&server) + .await; + let mut definition = tool(); + definition["x_orgii_tool_schema_cache_scope"] = json!("stable_prefix"); + let mut tools = vec![definition]; + if forced { + tools.push(choice(json!({"type":"tool","name":"emit"}))); + } + call(&client(&server), Some(&tools), stream).await.unwrap(); + let requests = server.received_requests().await.unwrap(); + let body: Value = requests[0].body_json().unwrap(); + assert_eq!(body["tools"], json!([tool()])); + assert_eq!( + body["tool_choice"], + if forced { + json!({"type":"function","function":{"name":"emit"}}) + } else { + json!("auto") + } + ); + } + } +} + +#[tokio::test] +async fn missing_forced_tool_fails_locally_without_http() { + for stream in [false, true] { + let server = MockServer::start().await; + for tools in [ + vec![choice(json!({"type":"tool","name":"missing"}))], + vec![tool(), choice(json!({"type":"tool","name":"missing"}))], + vec![choice(json!("required"))], + ] { + assert!(matches!(call(&client(&server), Some(&tools), stream).await, + Err(ProviderError::InvalidRequest { status: None, parameter: Some(parameter), .. }) if parameter == "tool_choice")); + } + assert!(server.received_requests().await.unwrap().is_empty()); + } +} + +#[test] +fn bad_request_preserves_http_status_parameter_and_reason() { + for (body, parameter) in [ + ( + r#"{"error":{"message":"empty tools","param":"tools"}}"#, + Some("tools"), + ), + ( + r#"{"error":{"message":"empty tools","parameter":"tools"}}"#, + Some("tools"), + ), + ( + r#"{"error":{"message":"empty tools","code":"invalid_request_error","param":"tools"}}"#, + Some("tools"), + ), + ("invalid parameters", None), + ( + r#"{"message":"empty tools","parameter":"tools"}"#, + Some("tools"), + ), + ("{broken", None), + ("", None), + ] { + let error = OpenAICompatClient::parse_error_response(400, body, None); + match error { + ProviderError::InvalidRequest { + status, + parameter: actual, + message, + } => { + assert_eq!(status, Some(400)); + assert_eq!(actual.as_deref(), parameter); + assert!(!message.is_empty()); + } + other => panic!("unexpected error: {other}"), + } + } + assert!(matches!( + OpenAICompatClient::parse_error_response( + 400, + r#"{"error":{"message":"maximum context length exceeded"}}"#, + None + ), + ProviderError::ContextTooLong(_) + )); + assert!(matches!( + OpenAICompatClient::parse_error_response( + 400, + r#"{"error":{"code":"model_not_found","message":"unknown model"}}"#, + None + ), + ProviderError::ModelNotFound(_) + )); + assert!(matches!( + OpenAICompatClient::parse_error_response( + 429, + r#"{"error":{"type":"usage_limit_reached","message":"quota exhausted"}}"#, + None + ), + ProviderError::UsageLimitReached(_) + )); +} + +#[tokio::test] +async fn invalid_requests_are_sent_once_and_can_fall_back() { + for stream in [false, true] { + let primary = MockServer::start().await; + Mock::given(method("POST")) + .respond_with( + ResponseTemplate::new(400).set_body_json( + json!({"error":{"message":"invalid parameter","param":"tools"}}), + ), + ) + .mount(&primary) + .await; + let reliable = + ReliableProvider::single("primary".into(), Box::new(client(&primary)), 3, 50); + assert!(matches!( + call(&reliable, None, stream).await, + Err(ProviderError::InvalidRequest { .. }) + )); + assert_eq!(primary.received_requests().await.unwrap().len(), 1); + primary.reset().await; + Mock::given(method("POST")) + .respond_with(ResponseTemplate::new(400).set_body_string("invalid parameters")) + .mount(&primary) + .await; + let fallback = MockServer::start().await; + Mock::given(method("POST")) + .respond_with(success) + .mount(&fallback) + .await; + let reliable = ReliableProvider::with_fallbacks( + vec![ + ("primary".into(), Box::new(client(&primary))), + ("fallback".into(), Box::new(client(&fallback))), + ], + 3, + 50, + ); + assert_eq!( + call(&reliable, None, stream) + .await + .unwrap() + .content + .as_deref(), + Some("ok") + ); + assert_eq!(primary.received_requests().await.unwrap().len(), 1); + assert_eq!(fallback.received_requests().await.unwrap().len(), 1); + } +} + +#[tokio::test] +async fn transient_http_failures_still_retry_and_exhaust_the_budget() { + for stream in [false, true] { + for status in [429, 503] { + let server = MockServer::start().await; + let count = Arc::new(AtomicUsize::new(0)); + let seen = count.clone(); + Mock::given(method("POST")) + .respond_with(move |request: &Request| { + if seen.fetch_add(1, Ordering::SeqCst) == 0 { + ResponseTemplate::new(status) + .set_body_json(json!({"error":{"message":"temporary failure"}})) + } else { + success(request) + } + }) + .mount(&server) + .await; + let reliable = + ReliableProvider::single("fixture".into(), Box::new(client(&server)), 2, 50); + call(&reliable, None, stream).await.unwrap(); + assert_eq!(count.load(Ordering::SeqCst), 2); + } + let server = MockServer::start().await; + Mock::given(method("POST")) + .respond_with(ResponseTemplate::new(503).set_body_string("unavailable")) + .mount(&server) + .await; + let reliable = ReliableProvider::single("fixture".into(), Box::new(client(&server)), 2, 50); + assert!(call(&reliable, None, stream).await.is_err()); + assert_eq!(server.received_requests().await.unwrap().len(), 3); + } +} diff --git a/src-tauri/crates/agent-core/src/core/providers/openai_compat/streaming/chat.rs b/src-tauri/crates/agent-core/src/core/providers/openai_compat/streaming/chat.rs index bfc013b129..0dbc2d8d36 100644 --- a/src-tauri/crates/agent-core/src/core/providers/openai_compat/streaming/chat.rs +++ b/src-tauri/crates/agent-core/src/core/providers/openai_compat/streaming/chat.rs @@ -6,13 +6,12 @@ use tracing::{info, warn}; use super::super::client::OpenAICompatClient; use super::super::types::{ChatCompletionRequest, ChatCompletionResponse, RequestBuilderExt}; use super::think_split::ThinkTagSplitter; -use super::translate_tool_choice_for_openai; +use super::tool_params::build_tool_params; use crate::providers::openai_policy::ChatTokenLimitField; use crate::providers::registry::provider_id; use crate::providers::traits::{finish_reason as finish, LLMResponse, ProviderError}; use crate::providers::wire_sanitize::{ coalesce_system_messages_to_front, sanitize_deepseek_messages, sanitize_openai_compat_messages, - strip_tool_schema_cache_scopes, }; use crate::utils::http_retry::extract_retry_after_secs; use crate::utils::safe_truncate_utf8; @@ -69,29 +68,13 @@ pub(super) async fn run_chat( this.config.is_azure, ); - let wire_tools = tools.map(strip_tool_schema_cache_scopes); - // Extract forced tool_choice from side_query structured output - let (tool_choice_override, clean_wire_tools) = if let Some(ref wt) = wire_tools { - let (ovr, cleaned) = crate::core::side_query::extract_tool_choice_override(wt); - (ovr, Some(cleaned)) - } else { - (None, None) - }; - let wire_tools_final = clean_wire_tools.or(wire_tools); - let wire_policy = this.chat_wire_policy(&base_model); + let (wire_tools, tool_choice) = build_tool_params(tools, wire_policy.send_tool_choice_auto)?; let request_body = ChatCompletionRequest { model: base_model.clone(), messages: wire_messages, - tools: wire_tools_final, - tool_choice: if let Some(ovr) = tool_choice_override { - // Map Anthropic-style {"type":"tool","name":"x"} → OpenAI {"type":"function","function":{"name":"x"}} - Some(translate_tool_choice_for_openai(&ovr)) - } else if wire_policy.send_tool_choice_auto { - tools.map(|_| Value::String("auto".to_string())) - } else { - None - }, + tools: wire_tools, + tool_choice, max_tokens: match wire_policy.token_limit_field { ChatTokenLimitField::MaxTokens => Some(max_tokens), ChatTokenLimitField::MaxCompletionTokens => None, diff --git a/src-tauri/crates/agent-core/src/core/providers/openai_compat/streaming/mod.rs b/src-tauri/crates/agent-core/src/core/providers/openai_compat/streaming/mod.rs index a991227993..85b392b6db 100644 --- a/src-tauri/crates/agent-core/src/core/providers/openai_compat/streaming/mod.rs +++ b/src-tauri/crates/agent-core/src/core/providers/openai_compat/streaming/mod.rs @@ -20,6 +20,7 @@ mod index_resolver; mod parse; mod sse_stream; mod think_split; +mod tool_params; // Re-export the public constant used by `turn_executor::tool_execution::*` // (via `openai_compat::STREAM_PARSE_ERROR_KEY`) to detect the synthetic diff --git a/src-tauri/crates/agent-core/src/core/providers/openai_compat/streaming/sse_stream.rs b/src-tauri/crates/agent-core/src/core/providers/openai_compat/streaming/sse_stream.rs index cd79eb0420..ff73605191 100644 --- a/src-tauri/crates/agent-core/src/core/providers/openai_compat/streaming/sse_stream.rs +++ b/src-tauri/crates/agent-core/src/core/providers/openai_compat/streaming/sse_stream.rs @@ -18,7 +18,7 @@ use super::error_classify::{looks_overloaded, parse_retry_after_ms}; use super::index_resolver::resolve_tool_call_index; use super::parse::{build_stream_parse_error_args, parse_streamed_tool_args, ParsedToolArgs}; use super::think_split::ThinkTagSplitter; -use super::translate_tool_choice_for_openai; +use super::tool_params::build_tool_params; use crate::providers::openai_policy::ChatTokenLimitField; use crate::providers::registry::provider_id; use crate::providers::traits::{ @@ -27,7 +27,6 @@ use crate::providers::traits::{ }; use crate::providers::wire_sanitize::{ coalesce_system_messages_to_front, sanitize_deepseek_messages, sanitize_openai_compat_messages, - strip_tool_schema_cache_scopes, }; use crate::utils::http_retry::extract_retry_after_secs; use crate::utils::safe_truncate_utf8; @@ -109,28 +108,13 @@ pub(super) async fn run_chat_streaming( this.config.is_azure, ); - let wire_tools = tools.map(strip_tool_schema_cache_scopes); - // Extract forced tool_choice from side_query structured output - let (tool_choice_override, clean_wire_tools) = if let Some(ref wt) = wire_tools { - let (ovr, cleaned) = crate::core::side_query::extract_tool_choice_override(wt); - (ovr, Some(cleaned)) - } else { - (None, None) - }; - let wire_tools_final = clean_wire_tools.or(wire_tools); - let wire_policy = this.chat_wire_policy(&base_model); + let (wire_tools, tool_choice) = build_tool_params(tools, wire_policy.send_tool_choice_auto)?; let request_body = ChatCompletionRequest { model: base_model.clone(), messages: wire_messages, - tools: wire_tools_final, - tool_choice: if let Some(ovr) = tool_choice_override { - Some(translate_tool_choice_for_openai(&ovr)) - } else if wire_policy.send_tool_choice_auto { - tools.map(|_| Value::String("auto".to_string())) - } else { - None - }, + tools: wire_tools, + tool_choice, max_tokens: match wire_policy.token_limit_field { ChatTokenLimitField::MaxTokens => Some(max_tokens), ChatTokenLimitField::MaxCompletionTokens => None, diff --git a/src-tauri/crates/agent-core/src/core/providers/openai_compat/streaming/tool_params.rs b/src-tauri/crates/agent-core/src/core/providers/openai_compat/streaming/tool_params.rs new file mode 100644 index 0000000000..237cddf5f8 --- /dev/null +++ b/src-tauri/crates/agent-core/src/core/providers/openai_compat/streaming/tool_params.rs @@ -0,0 +1,49 @@ +//! Tool parameters shared by streaming and whole-response requests. + +use serde_json::Value; + +use super::translate_tool_choice_for_openai; +use crate::providers::traits::ProviderError; +use crate::providers::wire_sanitize::strip_tool_schema_cache_scopes; + +pub(super) fn build_tool_params( + tools: Option<&[Value]>, + send_auto: bool, +) -> Result<(Option>, Option), ProviderError> { + let wire_tools = strip_tool_schema_cache_scopes(tools.unwrap_or_default()); + let (override_value, tools) = + crate::core::side_query::extract_tool_choice_override(&wire_tools); + let choice = override_value + .as_ref() + .map(translate_tool_choice_for_openai); + if let Some(choice) = &choice { + if let Some(name) = choice.pointer("/function/name").and_then(Value::as_str) { + if !tools + .iter() + .any(|tool| tool.pointer("/function/name").and_then(Value::as_str) == Some(name)) + { + return Err(ProviderError::InvalidRequest { + status: None, + parameter: Some("tool_choice".into()), + message: format!("The selected tool '{name}' is absent from the tool list"), + }); + } + } + if tools.is_empty() && choice.as_str() == Some("required") { + return Err(ProviderError::InvalidRequest { + status: None, + parameter: Some("tool_choice".into()), + message: "Required tool selection needs at least one tool".into(), + }); + } + } + // An empty tool list is a valid text-only turn, but an empty tools field + // (or an orphan automatic choice) is rejected by strict compatible APIs. + if tools.is_empty() { + return Ok((None, None)); + } + Ok(( + Some(tools), + choice.or_else(|| send_auto.then(|| Value::String("auto".into()))), + )) +} diff --git a/src-tauri/crates/agent-core/src/core/providers/openai_compat/types.rs b/src-tauri/crates/agent-core/src/core/providers/openai_compat/types.rs index 2e3849c800..bd6b7e1811 100644 --- a/src-tauri/crates/agent-core/src/core/providers/openai_compat/types.rs +++ b/src-tauri/crates/agent-core/src/core/providers/openai_compat/types.rs @@ -338,8 +338,8 @@ pub(super) struct ApiError { pub error_type: Option, /// Google-style status string (e.g. "RESOURCE_EXHAUSTED", "NOT_FOUND") pub status: Option, - /// Google-style numeric error code - pub code: Option, + /// Compatible APIs use numeric codes or strings such as invalid_request_error. + pub code: Option, } impl ApiError { @@ -351,12 +351,12 @@ impl ApiError { } } if let Some(ref status) = self.status { - if let Some(code) = self.code { + if let Some(ref code) = self.code { return format!("{} (code {})", status, code); } return status.clone(); } - if let Some(code) = self.code { + if let Some(ref code) = self.code { return format!("Error code {}", code); } "Unknown error".to_string() diff --git a/src-tauri/crates/agent-core/src/core/providers/reliable.rs b/src-tauri/crates/agent-core/src/core/providers/reliable.rs index 74d2b29d35..910dff887b 100644 --- a/src-tauri/crates/agent-core/src/core/providers/reliable.rs +++ b/src-tauri/crates/agent-core/src/core/providers/reliable.rs @@ -176,6 +176,7 @@ impl ReliableProvider { /// failure. fn error_kind(err: &ProviderError) -> &'static str { match err { + ProviderError::InvalidRequest { .. } => "invalid_request", ProviderError::RateLimited { .. } => "rate_limited", ProviderError::UsageLimitReached(_) => "usage_limit_reached", ProviderError::Overloaded { .. } => "overloaded", @@ -202,6 +203,9 @@ impl ReliableProvider { /// `_ => false` catch-all would have made any new error variant /// silently retryable, including ones that should fail fast. fn is_non_retryable(err: &ProviderError) -> bool { + if matches!(err, ProviderError::InvalidRequest { .. }) { + return true; + } // An explicit server directive (`x-should-retry` response header) // overrides the status-code heuristics below. The >10min rate-limit // fail-fast still applies afterwards: a server "true" on a @@ -211,6 +215,7 @@ impl ReliableProvider { } match err { ProviderError::AuthError(_) + | ProviderError::InvalidRequest { .. } | ProviderError::UsageLimitReached(_) | ProviderError::ModelNotFound(_) | ProviderError::ContextTooLong(_) @@ -282,6 +287,7 @@ impl ReliableProvider { | ProviderError::MediaTooLarge(_) | ProviderError::Cancelled | ProviderError::RequestFailed(_) + | ProviderError::InvalidRequest { .. } | ProviderError::ParseError(_) | ProviderError::Other(_) => None, } diff --git a/src-tauri/crates/agent-core/src/core/providers/traits.rs b/src-tauri/crates/agent-core/src/core/providers/traits.rs index 8b4bd8d9ee..6f87c7140f 100644 --- a/src-tauri/crates/agent-core/src/core/providers/traits.rs +++ b/src-tauri/crates/agent-core/src/core/providers/traits.rs @@ -547,6 +547,12 @@ pub trait LLMProvider: Send + Sync { /// Error type for LLM provider operations. #[derive(Debug)] pub enum ProviderError { + /// A rejected request must be corrected before retrying the same provider. + InvalidRequest { + status: Option, + parameter: Option, + message: String, + }, /// HTTP request failed. RequestFailed(String), /// Response parsing failed. @@ -586,6 +592,15 @@ pub enum ProviderError { impl std::fmt::Display for ProviderError { fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { + ProviderError::InvalidRequest { + status, message, .. + } => { + write!(formatter, "Request failed: ")?; + if let Some(status) = status { + write!(formatter, "HTTP {status}: ")?; + } + write!(formatter, "{message}") + } ProviderError::RequestFailed(msg) => write!(formatter, "Request failed: {}", msg), ProviderError::ParseError(msg) => write!(formatter, "Parse error: {}", msg), ProviderError::AuthError(msg) => write!(formatter, "Auth error: {}", msg), diff --git a/src-tauri/crates/agent-core/src/core/session/turn/streaming.rs b/src-tauri/crates/agent-core/src/core/session/turn/streaming.rs index 7b825110f0..a59687c5f9 100644 --- a/src-tauri/crates/agent-core/src/core/session/turn/streaming.rs +++ b/src-tauri/crates/agent-core/src/core/session/turn/streaming.rs @@ -151,6 +151,13 @@ pub fn classify_streaming_error_message(message: &str) -> StreamingErrorCode { impl From for StreamingError { fn from(err: ProviderError) -> Self { match err { + ProviderError::InvalidRequest { + status, + parameter, + message, + } => Self::new(message, StreamingErrorCode::Unknown).with_details(serde_json::json!({ + "reason": "invalid_request", "status": status, "parameter": parameter + })), ProviderError::AuthError(msg) => Self::new(msg, StreamingErrorCode::AuthError), ProviderError::UsageLimitReached(msg) => { Self::new(msg, StreamingErrorCode::RateLimited).with_details(serde_json::json!({ diff --git a/src-tauri/crates/agent-core/src/core/side_query.rs b/src-tauri/crates/agent-core/src/core/side_query.rs index a10e73d5e7..602487804d 100644 --- a/src-tauri/crates/agent-core/src/core/side_query.rs +++ b/src-tauri/crates/agent-core/src/core/side_query.rs @@ -303,6 +303,15 @@ pub async fn side_query_typed_with_options( warn!("[side-query] thinking:disabled rejected (400), retrying with padded max_tokens"); None } + Err(ProviderError::InvalidRequest { + status: Some(400), + ref message, + .. + }) if message.to_lowercase().contains("thinking") => { + observe_always_on_thinking(config, model); + warn!("[side-query] thinking:disabled rejected (400), retrying with padded max_tokens"); + None + } Err(err) => return Err(SideQueryError::Provider(err)), };