Compare commits

...

1 Commits

Author SHA1 Message Date
diegosouzapw
69647b3b94 fix(combo): ignore benign empty error fields in streaming quality validation
isStreamingUpstreamError used a key-presence check (parsed.error != null)
which false-positives on benign values some backends emit on every chunk
({}, '', false, 0). When opencode issues a tool-call turn, the upstream SSE
opens with role-only frames (no recognized content) and a later chunk that
carries real tool_calls content PLUS a benign empty error field. The error
gate runs BEFORE content recognizers, so that single frame short-circuits
to 'error' -> 502 'streaming upstream error'. Same combo via kilocode works
because its wire format never emits the empty error field.

Fix: isSubstantiveError() helper — only treat error as real when it carries
non-empty string, non-empty object, or explicit true. Empty object {}, empty
string '', false, and 0 are benign.

TDD: tests/unit/quality-validation-benign-error.test.ts proves tool_calls
chunk with error:{} or error:'' is valid (was 502), while a real error
{message, code} still correctly fails.
2026-08-08 11:03:11 -03:00
2 changed files with 184 additions and 1 deletions

View File

@@ -190,10 +190,26 @@ function isRecord(value: unknown): value is Record<string, unknown> {
return !!value && typeof value === "object" && !Array.isArray(value);
}
/**
* Whether an `error` field carries a real failure signal. A key-presence check
* (`!= null`) false-positives on benign values some backends emit on every
* chunk (`{}`, `""`, `false`, `0`) — e.g. tool-call turns where a chunk with
* real tool_calls content also carries `"error": {}`. Only substantive values
* are treated as upstream failures.
*/
function isSubstantiveError(value: unknown): boolean {
if (value === null || value === undefined) return false;
if (typeof value === "string") return value.trim().length > 0;
if (typeof value === "object" && !Array.isArray(value)) {
return Object.keys(value as Record<string, unknown>).length > 0;
}
return value === true;
}
function isStreamingUpstreamError(parsed: unknown, eventType: string): boolean {
if (eventType === "response.failed" || eventType === "error") return true;
if (!isRecord(parsed)) return false;
if (parsed.error != null) return true;
if (isSubstantiveError(parsed.error)) return true;
const nestedResponse = isRecord(parsed.response) ? parsed.response : null;
return nestedResponse?.status === "failed" && nestedResponse.error != null;

View File

@@ -0,0 +1,167 @@
/**
* TDD regression guard — quality validation false-positive on benign `error`
* fields in streaming SSE chunks.
*
* `isStreamingUpstreamError` treats ANY non-null `error` field as an upstream
* failure: `parsed.error != null` is true for `{}`, `""`, `false`, and `0`.
* When a client like opencode issues a tool-call turn, the upstream SSE opens
* with role-only frames (no recognized content) and a later chunk that carries
* real tool_calls content PLUS a benign empty `error` field (a field some
* backends emit on every chunk). The error gate runs BEFORE the content
* recognizers, so that single frame short-circuits to "error" → 502
* "streaming upstream error" — while the same combo via kilocode (different
* wire format) never emits the empty `error` field and works fine.
*/
import test from "node:test";
import assert from "node:assert/strict";
const { validateResponseQuality } = await import("../../open-sse/services/combo.ts");
const encoder = new TextEncoder();
const silentLog = { warn: () => {} };
function openAiSseStream(events: string[]): ReadableStream<Uint8Array> {
const body = events.join("\n") + "\n";
return new ReadableStream<Uint8Array>({
start(controller) {
controller.enqueue(encoder.encode(body));
controller.close();
},
});
}
/**
* OpenAI-compatible tool-call stream that ALSO carries a benign empty `error`
* field on the tool_calls chunk. Some backends emit `"error": {}` or
* `"error": ""` alongside every chunk; that is not a real upstream failure.
* The frame must be treated as CONTENT (valid), not ERROR.
*/
function makeToolCallStreamWithBenignError(): Response {
const events = [
// role-only first chunk — no recognized content, widens the peek window
`data: ${JSON.stringify({
id: "chatcmpl_1",
object: "chat.completion.chunk",
created: 123,
model: "gpt-4o",
choices: [{ index: 0, delta: { role: "assistant" }, finish_reason: null }],
})}`,
"",
// tool_calls delta + benign empty `error` field (the bug trigger)
`data: ${JSON.stringify({
id: "chatcmpl_2",
object: "chat.completion.chunk",
created: 123,
model: "gpt-4o",
choices: [
{
index: 0,
delta: {
tool_calls: [
{ index: 0, id: "call_1", type: "function", function: { name: "Bash", arguments: "" } },
],
},
finish_reason: null,
},
],
error: {},
})}`,
"",
`data: [DONE]`,
"",
];
return new Response(openAiSseStream(events), {
status: 200,
headers: { "content-type": "text/event-stream" },
});
}
test("OpenAI stream with tool_calls + benign empty error:{} field is VALID (not 502)", async () => {
const res = makeToolCallStreamWithBenignError();
const out = await validateResponseQuality(res, true, silentLog);
assert.equal(
out.valid,
true,
`expected valid for tool_calls chunk with benign error:{}, got valid=false (reason: ${out.reason})`
);
assert.ok(out.clonedResponse, "clonedResponse must be present for valid streaming response");
});
test("OpenAI stream with tool_calls + benign empty error:'' field is VALID", async () => {
const events = [
`data: ${JSON.stringify({
id: "chatcmpl_3",
object: "chat.completion.chunk",
created: 123,
model: "gpt-4o",
choices: [{ index: 0, delta: { role: "assistant" }, finish_reason: null }],
})}`,
"",
`data: ${JSON.stringify({
id: "chatcmpl_4",
object: "chat.completion.chunk",
created: 123,
model: "gpt-4o",
choices: [
{
index: 0,
delta: {
tool_calls: [
{ index: 0, id: "call_2", type: "function", function: { name: "Read", arguments: "" } },
],
},
finish_reason: null,
},
],
error: "",
})}`,
"",
`data: [DONE]`,
"",
];
const res = new Response(openAiSseStream(events), {
status: 200,
headers: { "content-type": "text/event-stream" },
});
const out = await validateResponseQuality(res, true, silentLog);
assert.equal(
out.valid,
true,
`expected valid for tool_calls chunk with benign error:"", got valid=false (reason: ${out.reason})`
);
});
test("Stream with a REAL non-empty error object is still flagged as invalid", async () => {
const events = [
`data: ${JSON.stringify({
id: "chatcmpl_5",
object: "chat.completion.chunk",
created: 123,
model: "gpt-4o",
choices: [{ index: 0, delta: { role: "assistant" }, finish_reason: null }],
})}`,
"",
`data: ${JSON.stringify({
id: "chatcmpl_6",
object: "chat.completion.chunk",
created: 123,
model: "gpt-4o",
choices: [{ index: 0, delta: {}, finish_reason: null }],
error: { message: "upstream quota exceeded", code: "rate_limit_exceeded" },
})}`,
"",
`data: [DONE]`,
"",
];
const res = new Response(openAiSseStream(events), {
status: 200,
headers: { "content-type": "text/event-stream" },
});
const out = await validateResponseQuality(res, true, silentLog);
assert.equal(
out.valid,
false,
`expected invalid for real error object, got valid=true (reason: ${out.reason})`
);
assert.match(out.reason ?? "", /streaming upstream error/, "reason should mention the upstream error");
});