From 4b1a9c88086c04d60aecbc069c56df03ba398fdb Mon Sep 17 00:00:00 2001 From: dtzp555 Date: Sun, 24 May 2026 10:28:38 +1000 Subject: [PATCH] =?UTF-8?q?fix(ir):=20D12=20=E2=80=94=20remove=20invented?= =?UTF-8?q?=20top-level=20`error`=20field=20on=20OpenAI=20response=20shape?= =?UTF-8?q?s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cold-audit catch from 2026-05-23 Cold-audit Finding 3 (P2 ALIGNMENT.md Rule 2(b) violation). The IR→OpenAI translator was inventing a top-level `error: {message, type}` field on chat.completion / chat.completion.chunk objects. OpenAI's spec defines no such field — errors surface via HTTP 4xx/5xx with `{error: {...}}` body, not as in-band SSE annotations. The pre-D12 code comments acknowledged Rule 2(b) for `finish_reason` enum values then proceeded to invent the top-level error field anyway. Changes (3 files, +25 / -50 net): 1. lib/ir/ir-to-openai.mjs — both invention sites removed: - irChunkToOpenAISSE: removed `if (irChunk.type === 'error')` branch that emitted `{...spec fields, error: {message, type: 'provider_error'}}` - irResponseToOpenAINonStream: removed `else if (chunk.type === 'error')` branch + dead `errorChunk` local var + the `if (errorChunk)` block that appended top-level `response.error = {...}` - Added explanatory comments at both sites citing ALIGNMENT.md Rule 2(b) + OpenAI spec URLs so future readers see the rationale 2. server.mjs:581 — burst-replay loop break condition simplified: - Pre: `if (irChunk.type === 'stop' || irChunk.type === 'error') break;` - Post: `if (irChunk.type === 'stop') break;` - Cache writes (server.mjs:456, :470) only append to streamedChunks after the error-chunk guard fires, so cached chunks structurally cannot contain type === 'error'. The removed clause was dead. 3. test-features.mjs — one test rewritten (test count unchanged 288 → 288): - Old test asserted `payload.error.type === 'provider_error'` exists (i.e., it tested the violation as if it were correct) - New test asserts `payload.error === undefined` (the correct Rule 2(b) invariant) and that object stays `chat.completion.chunk` with finish_reason in the OpenAI enum IR contract decision (verified by both implementer and reviewer): - IR keeps `'error'` in IRResponseChunk typedef — providers legitimately emit error chunks (e.g., anthropic.mjs) - server.mjs intercepts error chunks BEFORE translation: · collectAllChunks() throws ProviderError on type === 'error' · Real-streaming branch throws ProviderError (no first chunk) or truncates res.end() (after first chunk per ADR 0004 first-chunk rule) - The translator is downstream of both guards; error chunks structurally cannot reach it in normal operation - Therefore the invention sites were dead-code paths handling an impossible case — safe to remove - Pass-through behavior on the impossible path: produces an empty SSE delta `{choices: [{index: 0, delta: {}, finish_reason: null}]}` (verified manually by reviewer); the error message text does NOT leak to the wire 288/288 tests pass on Node 20.20.2. Authority: - ALIGNMENT.md Rule 2(b) — no invented OpenAI-spec fields - OpenAI Chat Completions object spec https://platform.openai.com/docs/api-reference/chat/object https://platform.openai.com/docs/api-reference/chat/streaming - ADR 0004 § Fallback safety — first-chunk rule (preserves the truncation semantics for post-first-chunk errors) Reviewer (Iron Rule v1.6 § 10.x Mode A, fresh-context opus, independent of drafter): APPROVE. Verified error chunks never reach translator via three independent grep passes (collectAllChunks guard, real-streaming guard, no third caller exists), verified cache cannot contain error chunks (streamedChunks.push is post-guard), verified Rule 2(b) compliance by direct read of ALIGNMENT.md line 29 + OpenAI spec citations. Three non-blocking suggestions (defensive log on impossible path; defensive guard at server.mjs:587; comment placement nit) explicitly "Not for D12" per the reviewer — preserved as separate cleanup opportunities if ever needed. Co-Authored-By: Claude Opus 4.7 --- lib/ir/ir-to-openai.mjs | 55 +++++++++++------------------------------ server.mjs | 2 +- test-features.mjs | 18 ++++++++------ 3 files changed, 25 insertions(+), 50 deletions(-) diff --git a/lib/ir/ir-to-openai.mjs b/lib/ir/ir-to-openai.mjs index 856cc87..3b3bd3a 100644 --- a/lib/ir/ir-to-openai.mjs +++ b/lib/ir/ir-to-openai.mjs @@ -59,29 +59,14 @@ export function irChunkToOpenAISSE(irChunk, requestId, model) { return `data: ${JSON.stringify(chunk)}\n\n`; } - if (irChunk.type === 'error') { - // SSE error chunk. ALIGNMENT.md Rule 2 (b) forbids inventing - // `finish_reason` values not in OpenAI's enum - // (https://platform.openai.com/docs/api-reference/chat/streaming - // enumerates: stop, length, tool_calls, content_filter, function_call, - // null). Surface the error via the top-level `error` object and use - // finish_reason: 'stop' on the choice — clients that respect the - // enum see a valid terminator; clients that read the `error` field - // see the failure detail. - const chunk = { - id: requestId, - object: 'chat.completion.chunk', - created, - model, - choices: [{ - index: 0, - delta: { content: '' }, - finish_reason: 'stop', - }], - error: { message: irChunk.error ?? 'Unknown provider error', type: 'provider_error' }, - }; - return `data: ${JSON.stringify(chunk)}\n\n`; - } + // Note: type === 'error' is intentionally not handled here. + // Error chunks are converted to thrown ProviderError in server.mjs before + // they reach this translator (see collectAllChunks and the real-streaming + // error path). Per ALIGNMENT.md Rule 2 (b), adding a top-level `error` + // field on chat.completion.chunk objects is a spec violation — OpenAI errors + // surface via HTTP 4xx/5xx with {error: {...}} body, not in-band SSE fields. + // If an error chunk somehow reaches here it falls through to the delta path + // which is a no-op (no content/role/tool_calls), producing an empty delta. // type === 'delta' const delta = {}; @@ -131,7 +116,6 @@ export function irResponseToOpenAINonStream(irChunks, requestId, model) { let content = ''; let finish_reason = 'stop'; let usage = null; - let errorChunk = null; const tool_calls = []; for (const chunk of irChunks) { @@ -149,17 +133,13 @@ export function irResponseToOpenAINonStream(irChunks, requestId, model) { if (chunk.usage) { usage = chunk.usage; } - } else if (chunk.type === 'error') { - // Surface provider errors via the top-level `error` annotation on the - // response object below + an inline content marker. `finish_reason` - // stays 'stop' because ALIGNMENT.md Rule 2 (b) forbids inventing - // enum values OpenAI's spec does not define - // (https://platform.openai.com/docs/api-reference/chat/object — - // finish_reason ∈ {stop, length, tool_calls, content_filter, - // function_call, null}). - content += chunk.error ? `[provider error: ${chunk.error}]` : '[provider error]'; - errorChunk = chunk; } + // Note: type === 'error' is intentionally not handled here. + // Error chunks are converted to thrown ProviderError in server.mjs + // (collectAllChunks) before this function is ever called. Adding a + // top-level `error` field on chat.completion objects violates + // ALIGNMENT.md Rule 2 (b) — OpenAI errors surface via HTTP 4xx/5xx + // with {error: {...}} body, not as invented response fields. } const message = { @@ -187,12 +167,5 @@ export function irResponseToOpenAINonStream(irChunks, requestId, model) { response.usage = usage; } - if (errorChunk) { - response.error = { - message: errorChunk.error ?? 'Unknown provider error', - type: 'provider_error', - }; - } - return response; } diff --git a/server.mjs b/server.mjs index 8f764c0..d983816 100644 --- a/server.mjs +++ b/server.mjs @@ -578,7 +578,7 @@ async function handleChatCompletions(req, res) { for (const irChunk of chunks) { res.write(irChunkToOpenAISSE(irChunk, requestId, ir.model)); - if (irChunk.type === 'stop' || irChunk.type === 'error') break; + if (irChunk.type === 'stop') break; } res.write(SSE_DONE); res.end(); diff --git a/test-features.mjs b/test-features.mjs index f3a8b3f..ccd02ca 100644 --- a/test-features.mjs +++ b/test-features.mjs @@ -357,17 +357,19 @@ describe('irChunkToOpenAISSE format', () => { assert.deepEqual(payload.choices[0].delta, {}); }); - it('formats an error chunk with finish_reason within the OpenAI enum', () => { - // ALIGNMENT.md Rule 2 (b): finish_reason must stay within the OpenAI spec - // enum (stop|length|tool_calls|content_filter|function_call|null). - // Provider errors surface via the top-level `error` object, not via an - // invented finish_reason value. + it('does not invent a top-level error field on error chunks (ALIGNMENT.md Rule 2(b))', () => { + // ALIGNMENT.md Rule 2 (b): OLP must not introduce OpenAI-spec fields that + // OpenAI's /v1/chat/completions specification does not document. + // OpenAI chat.completion.chunk objects have no top-level `error` field. + // Provider errors surface via HTTP 4xx/5xx, not as in-band SSE fields. + // Error chunks should not reach the translator in normal operation — + // server.mjs converts them to thrown ProviderError before translation. + // If one somehow does reach here, no `error` field must be invented. const sse = irChunkToOpenAISSE({ type: 'error', error: 'spawn failed' }, ID, MODEL); const payload = JSON.parse(sse.slice(6).trim()); - assert.ok(payload.error); - assert.equal(payload.error.type, 'provider_error'); + assert.equal(payload.error, undefined, 'translator must not invent top-level error field'); + assert.equal(payload.object, 'chat.completion.chunk'); assert.ok(['stop', 'length', 'tool_calls', 'content_filter', 'function_call', null].includes(payload.choices[0].finish_reason)); - assert.equal(payload.choices[0].finish_reason, 'stop'); }); it('SSE_DONE is the [DONE] terminator', () => {