Skip to content

fix(agent-core): omit empty tools and stop invalid request retries #2169

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ShiboSheng wants to merge 1 commit into
base: develop
Choose a base branch
from
+871 −47
Open

fix(agent-core): omit empty tools and stop invalid request retries #2169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 106 additions & 0 deletions scripts/dev/org-request-contract-verification.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
134 changes: 134 additions & 0 deletions scripts/dev/provider-fixture-server.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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("<system-reminder>"));
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);
});
}
42 changes: 42 additions & 0 deletions scripts/dev/provider-fixture-server.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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: "<system-reminder>test environment</system-reminder>" }] };
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: "<inbox-batch>task done</inbox-batch>" },
{ role: "user", content: [{ text: "<system-reminder>state=`ready`</system-reminder>" }] },
], tools: [{ function: { name: "org_run_complete" } }] }).tool_calls[0].function.name, "org_run_complete");
});
8 changes: 6 additions & 2 deletions src-tauri/crates/agent-core/src/core/providers/openai_adaptive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 {
Expand Down
Loading
Loading