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)), };