From 2600185edb0cc7246646d7104d22b4ad6a342e28 Mon Sep 17 00:00:00 2001 From: dtzp555 Date: Sun, 24 May 2026 20:20:49 +1000 Subject: [PATCH] =?UTF-8?q?fix+test:=20D35=20=E2=80=94=20pre-Phase-2=20bat?= =?UTF-8?q?ch=20#1=20(issues=20#4=20#9=20#10=20#11=20#12)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First batch of pre-Phase-2 cleanup work. 5 GitHub issues closed in one cohesive commit covering streaming-path correctness, IR validator hardening, and CI path-trigger hygiene. Changes (4 files, +302 / -5): **Code fixes** 1. **#9 — Streaming empty-then-clean-exit headers** (server.mjs) Pre-D35: when a provider's streaming spawn finished cleanly with zero chunks (e.g. spec-degenerate stop with no content), the response went out the SSE_DONE / res.end path without ever calling writeHead. Result: client saw stream open + close with no headers, no status code path applied. Now: zero-chunk branch guards `!res.headersSent` and emits Content-Type + Cache-Control + Connection + X-Accel-Buffering + all 5 X-OLP-* headers via olpHeaders (provider attempted, cache miss) before writing the terminator. Zero-chunk path correctly does NOT cache (cache write remains gated on irChunk.type === 'stop'). 2. **#10 — Streaming post-first-chunk error truncation marker** (server.mjs, two sibling sites) Pre-D35: if a provider yielded an error AFTER first content chunk was emitted, the SSE stream was abandoned with raw socket close. Client SDKs that wait for finish_reason hung. Now: - Catch-block firstChunkEmitted=true path: emit synthetic `{type:'stop', finish_reason:'length'}` via irChunkToOpenAISSE, write SSE_DONE, end. Per ADR 0004 § Fallback safety: post-first- chunk truncation surfaces as `length` finish, not a hang. - Sibling fix in error-chunk path (provider yields `type:'error'` chunk AFTER first content chunk): same recovery (marker + DONE + end). Scope-creep acknowledged but identical semantic; clean to fix together. Comments cross-reference D26 F19 and D35 #10. 3. **#11 — validateIRRequest irVersion strict check** (lib/ir/types.mjs) ADR 0003 IR contract pins irVersion to '1.0'. Validator pre-D35 accepted ANY value (including no value, undefined, '2.0', numeric 1.0). Now: `obj.irVersion !== undefined && obj.irVersion !== '1.0'` → rejection. Strict string match. `undefined` still accepted (pre-D35 IRs without the field remain valid — back-compat with sites that haven't yet been migrated to emit it). Error message uses JSON.stringify for safe rendering. 4. **#12 — alignment.yml scripts/** trigger removal** (.github/workflows/alignment.yml) Pre-D35 push.paths and pull_request.paths listed scripts/**. The scripts/ directory does not currently exist (per AGENTS.md note: scripts/migrate-from-ocp.mjs is planned for Phase 7). A path filter referencing a non-existent directory has no effect on trigger evaluation BUT misleads readers about the workflow's intent. Removed from both push.paths and pull_request.paths. When scripts/ lands in Phase 7, the trigger should be re-added at the same time (see release_kit_overlay.bootstrap_quirk_policy). **Verification — #4 (uniform X-OLP-Latency-Ms across error paths)** #4 was found to already be correct via D32. Re-audit of all 7 in-handler sendError sites in handleChatCompletions confirmed all attach a 5-header set via olpHeaders or olpErrorHeaders: - 360-361 (415 wrong Content-Type) → olpErrorHeaders - 368-369 (400 bad JSON) → olpErrorHeaders - 378-379 (400 BadRequestError IR translation) → olpErrorHeaders - 402-407 (503 no chain) → olpErrorHeaders - 617-618 (503 provider disappeared) → olpErrorHeaders - 760-761 (502 streaming pre-first-chunk error) → olpHeaders - 778-779 (500 fallback engine error) → olpErrorHeaders The 404 (line 922) and outer 500 (line 926) are router-level paths without startMs context and correctly lack OLP headers. D35 adds the #4-audit regression test pinning the 5-header invariant on the 503 no-provider response so future drift is caught immediately. **Tests** (test-features.mjs): 416 → 424 (+8): - #4-audit ×1 (5-header invariant on 503 no-provider sendError) - #9 ×1 (zero-chunk streaming → 200 + Content-Type=text/event-stream + 5 X-OLP-* headers + [DONE]) - #10 ×1 (catch-throw after first chunk → marker + length finish + DONE) - #10b ×1 (provider error chunk after first chunk → same recovery) - #11a ×1 (irVersion undefined accepted) - #11b ×1 (irVersion '1.0' accepted) - #11c ×1 (irVersion '2.0' rejected) - #11d ×1 (irVersion numeric 1.0 rejected) Pre-commit fold-in (per evidence-first checkpoint #4): - **D35 reviewer flagged JSDoc/validator drift on irVersion** (Suggestion #1, non-blocking). The @property typedef at lib/ir/types.mjs:43 said `{string} irVersion - always IR_VERSION` but the validator at lines 185-186 accepts `undefined`. Future reader who scans the @property alone sees contradiction without the rationale comment 140 lines below. Folded: typedef marked `[irVersion]` (optional) and description updated to "optional; when present must equal IR_VERSION ('1.0'). Pre-D35 IRs lack this field and remain valid." Two other non-blocking reviewer suggestions not folded (out of scope for D35; tracked as future polish): - Distinct event names for the two streaming_error_after_first_chunk log sites (provider-emitted string vs JS exception message). - Phase 7 TODO: re-add scripts/** trigger to alignment.yml when scripts/migrate-from-ocp.mjs lands. Authority: - ADR 0004 § Fallback safety — post-first-chunk truncation surfaces as `length` finish (#10 + #10b) - ADR 0003 § IR contract — irVersion pinned to '1.0' (#11) - AGENTS.md § Implementation status — scripts/ planned for Phase 7 (#12) - CLAUDE.md release_kit_overlay phase_rolling_mode — D35 lands under "Unreleased" against Phase 2; no version bump - CC 开发铁律 v1.6 § 10.x — independent fresh-context reviewer required for code change Reviewer (Iron Rule v1.6 § 10.x Mode A, fresh-context opus, independent of drafter): APPROVE_WITH_MINOR. Critical depth checks: - Verified all 7 sendError sites in handleChatCompletions attach 5-header set (cited line numbers reconciled with current state) - Verified writeHead block guarded by !res.headersSent; correctly placed AFTER optional truncation marker, BEFORE SSE_DONE - Verified irVersion validator strict-equality semantics across all 4 cases (undefined / '1.0' / '2.0' / numeric 1.0) - Verified scripts/** removed from both push and pull_request paths - Verified hygiene: 0 hits for personal markers, home paths, OAuth tokens, internal IPs - 424/424 tests pass independently in reviewer's run Co-Authored-By: Claude Opus 4.7 --- .github/workflows/alignment.yml | 2 - lib/ir/types.mjs | 11 +- server.mjs | 30 +++- test-features.mjs | 264 ++++++++++++++++++++++++++++++++ 4 files changed, 302 insertions(+), 5 deletions(-) diff --git a/.github/workflows/alignment.yml b/.github/workflows/alignment.yml index 01a2475..1efa8fa 100644 --- a/.github/workflows/alignment.yml +++ b/.github/workflows/alignment.yml @@ -5,7 +5,6 @@ on: paths: - 'server.mjs' - 'lib/**' - - 'scripts/**' - 'models-registry.json' - '.github/workflows/alignment.yml' push: @@ -13,7 +12,6 @@ on: paths: - 'server.mjs' - 'lib/**' - - 'scripts/**' - 'models-registry.json' - '.github/workflows/alignment.yml' diff --git a/lib/ir/types.mjs b/lib/ir/types.mjs index 39c568d..a340709 100644 --- a/lib/ir/types.mjs +++ b/lib/ir/types.mjs @@ -40,7 +40,7 @@ export const VALID_ROLES = ['system', 'user', 'assistant', 'tool']; /** * @typedef {Object} IRRequest - * @property {string} irVersion - always IR_VERSION + * @property {string} [irVersion] - optional; when present must equal IR_VERSION ('1.0'). Pre-D35 IRs lack this field and remain valid. * @property {IRMessage[]} messages * @property {string} model * @property {boolean} stream @@ -177,6 +177,15 @@ export function validateIRRequest(obj) { } } + // Optional: irVersion — must be '1.0' if present; undefined accepted for pre-existing IRs + // Per ADR 0003 § Required fields: analogous to contractVersion='1.0' in base.mjs. + // Decision: undefined accepted because openai-to-ir.mjs sets irVersion on construction; + // pre-existing IRs without it still validate. Strict '1.0' rejection only when explicitly + // set wrong. + if (obj.irVersion !== undefined && obj.irVersion !== '1.0') { + errors.push(`irVersion must be '1.0' (got: ${JSON.stringify(obj.irVersion)})`); + } + // Optional: tool_choice — 'auto' | 'none' | 'required' | {type:'function', function:{name}} if (obj.tool_choice !== undefined) { if (typeof obj.tool_choice === 'string') { diff --git a/server.mjs b/server.mjs index 4407357..7b8f353 100644 --- a/server.mjs +++ b/server.mjs @@ -637,12 +637,16 @@ async function handleChatCompletions(req, res) { if (irChunk.type === 'error') { // Error chunk from provider if (firstChunkEmitted) { - // Past first-chunk boundary — can't fallback; truncate stream. + // Past first-chunk boundary — can't fallback; emit truncation marker + [DONE] + // so clients can detect the incomplete response in-band (aligns D26 F19 + // stop-less exhaustion behaviour and the catch-block fix in D35 #10). logEvent('warn', 'streaming_error_after_first_chunk', { provider: streamProvider, model: streamModel, error: irChunk.error, }); + res.write(irChunkToOpenAISSE({ type: 'stop', finish_reason: 'length' }, requestId, ir.model)); + res.write(SSE_DONE); res.end(); return; } @@ -701,6 +705,23 @@ async function handleChatCompletions(req, res) { const truncMarker = { type: 'stop', finish_reason: 'length' }; res.write(irChunkToOpenAISSE(truncMarker, requestId, ir.model)); } + + // D35 #9: Zero-chunk empty-stream path — writeHead is still deferred + // (firstChunkEmitted===false) when the generator yields no chunks at all and + // exits cleanly. Without an explicit writeHead Node auto-emits 200 with the + // default Content-Type and none of the X-OLP-* headers. + // A provider that yielded nothing still constitutes an attempted call, so we + // emit the full olpHeaders (provider WAS attempted, just yielded zero chunks). + if (!res.headersSent) { + res.writeHead(200, { + 'Content-Type': 'text/event-stream', + 'Cache-Control': 'no-cache', + Connection: 'keep-alive', + 'X-Accel-Buffering': 'no', + ...streamHeaders, + }); + } + res.write(SSE_DONE); res.end(); // Loop exhausted without stop chunk = truncation. The stop-chunk completion @@ -716,12 +737,17 @@ async function handleChatCompletions(req, res) { } } catch (e) { if (firstChunkEmitted) { - // Past first-chunk boundary — truncate silently. + // Past first-chunk boundary — can't fallback; emit truncation marker + [DONE] + // so clients can detect the incomplete response in-band (aligns with D26 F19 + // stop-less exhaustion behaviour). ADR 0004 § Fallback safety: no fallback + // after first-chunk boundary; truncation is the correct recovery. logEvent('warn', 'streaming_error_after_first_chunk', { provider: streamProvider, model: streamModel, error: e.message, }); + res.write(irChunkToOpenAISSE({ type: 'stop', finish_reason: 'length' }, requestId, ir.model)); + res.write(SSE_DONE); res.end(); } else { // No bytes written — surface a clean JSON error. diff --git a/test-features.mjs b/test-features.mjs index 415fac6..8d847ab 100644 --- a/test-features.mjs +++ b/test-features.mjs @@ -5840,6 +5840,270 @@ describe('Streaming cache-miss real-time (Suite 15)', () => { }); +// ── D35 batch: issues #4 / #9 / #10 / #11 / #12 ───────────────────────────── +// +// #4 — Uniform X-OLP-* headers on in-handler error paths (audit confirms D32 complete) +// #9 — Zero-chunk empty stream → writeHead fires with text/event-stream + X-OLP-* headers +// #10 — Post-first-chunk error emits truncation marker (finish_reason:'length') + [DONE] +// #11 — validateIRRequest irVersion enforcement +// #12 — alignment.yml scripts/** path removed (CI only, no test needed) +// +// Authority: ADR 0004 § Observability headers; ADR 0003 IR contract + +// ── D35-#11 unit tests (no server needed) ──────────────────────────────────── + +describe('D35 #11 — validateIRRequest irVersion enforcement', () => { + + it('#11a: undefined irVersion is accepted (omitted field)', () => { + const ir = makeIR(); + delete ir.irVersion; + const r = validateIRRequest(ir); + assert.equal(r.valid, true, `Expected valid when irVersion is absent, got errors: ${r.errors.join('; ')}`); + }); + + it("#11b: irVersion '1.0' is accepted (correct version)", () => { + const r = validateIRRequest(makeIR({ irVersion: '1.0' })); + assert.equal(r.valid, true, `Expected valid for irVersion '1.0'`); + }); + + it("#11c: irVersion '2.0' is rejected (wrong version string)", () => { + const r = validateIRRequest(makeIR({ irVersion: '2.0' })); + assert.equal(r.valid, false, "Expected invalid for irVersion '2.0'"); + assert.ok( + r.errors.some(e => e.includes('irVersion')), + `Expected an irVersion error, got: ${r.errors.join('; ')}` + ); + }); + + it('#11d: irVersion 1.0 (number, not string) is rejected', () => { + const r = validateIRRequest(makeIR({ irVersion: 1.0 })); + assert.equal(r.valid, false, 'Expected invalid for irVersion as number 1.0'); + assert.ok( + r.errors.some(e => e.includes('irVersion')), + `Expected an irVersion error, got: ${r.errors.join('; ')}` + ); + }); + +}); + +// ── D35-#4/#9/#10 integration tests ────────────────────────────────────────── + +describe('D35 #4/#9/#10 — Streaming error-path header + truncation-marker fixes', () => { + let serverD35; + let portD35; + let savedTokenD35; + + before(async () => { + savedTokenD35 = process.env.CLAUDE_CODE_OAUTH_TOKEN; + process.env.CLAUDE_CODE_OAUTH_TOKEN = 'fake-token-d35'; + __setProvidersEnabled({ anthropic: true }); + + const { createOlpServer: sD35, __clearCache: ccD35 } = await import('./server.mjs'); + ccD35(); + serverD35 = sD35(); + await new Promise((resolve, reject) => { + serverD35.listen(0, '127.0.0.1', resolve); + serverD35.once('error', reject); + }); + portD35 = serverD35.address().port; + }); + + after(async () => { + __resetProvidersEnabled(); + __resetSpawnImpl(); + if (savedTokenD35 !== undefined) { + process.env.CLAUDE_CODE_OAUTH_TOKEN = savedTokenD35; + } else { + delete process.env.CLAUDE_CODE_OAUTH_TOKEN; + } + if (!serverD35) return; + return new Promise(r => serverD35.close(r)); + }); + + it('#4-audit: 503 no_enabled_provider carries all 5 X-OLP-* headers (D32 already correct)', async () => { + // Verifies that the 503 path (no chain built) emits the full 5-header set + // via olpErrorHeaders — confirming D32 coverage. D35 audit found D32 complete; + // this test pins the invariant. + __setProvidersEnabled({}); + const { createOlpServer: s4, __clearCache: cc4 } = await import('./server.mjs'); + cc4(); + const tmpServer = s4(); + await new Promise((resolve, reject) => { + tmpServer.listen(0, '127.0.0.1', resolve); + tmpServer.once('error', reject); + }); + const tmpPort = tmpServer.address().port; + try { + const r = await fetch({ + port: tmpPort, + method: 'POST', + path: '/v1/chat/completions', + body: { + model: 'claude-sonnet-4-6', + messages: [{ role: 'user', content: 'test' }], + stream: false, + }, + }); + assert.equal(r.status, 503, `Expected 503, got ${r.status}`); + assert.ok(r.headers['x-olp-provider-used'], 'Must have X-OLP-Provider-Used'); + assert.ok(r.headers['x-olp-model-used'], 'Must have X-OLP-Model-Used'); + assert.ok(r.headers['x-olp-fallback-hops'] !== undefined, 'Must have X-OLP-Fallback-Hops'); + assert.ok(r.headers['x-olp-cache'], 'Must have X-OLP-Cache'); + assert.ok(r.headers['x-olp-latency-ms'] !== undefined, 'Must have X-OLP-Latency-Ms'); + assert.equal(r.headers['x-olp-provider-used'], 'none', 'X-OLP-Provider-Used must be "none"'); + assert.equal(r.headers['x-olp-model-used'], 'claude-sonnet-4-6', 'X-OLP-Model-Used must be the requested model'); + } finally { + __setProvidersEnabled({ anthropic: true }); + await new Promise(r => tmpServer.close(r)); + } + }); + + it('#9: zero-chunk clean exit → 200 + text/event-stream + all 5 X-OLP-* headers + [DONE]', async () => { + // Provider spawn yields zero chunks and exits cleanly (empty generator). + // Before D35 fix: writeHead never fired → Node auto-emitted 200 with default + // Content-Type + no X-OLP-* headers. + // After D35 fix: writeHead fires before SSE_DONE with text/event-stream + all 5 headers. + const { loadedProviders: lpD35_9, __clearCache: ccD35_9 } = await import('./server.mjs'); + const savedProvider = lpD35_9.get('anthropic'); + const emptyProvider = { + ...savedProvider, + spawn: async function* () { + // yields nothing — clean generator exit + }, + }; + lpD35_9.set('anthropic', emptyProvider); + ccD35_9(); + + try { + const r = await fetch({ + port: portD35, + method: 'POST', + path: '/v1/chat/completions', + body: { + model: 'claude-sonnet-4-6', + messages: [{ role: 'user', content: 'empty-stream-test' }], + stream: true, + }, + }); + + assert.equal(r.status, 200, `Expected 200, got ${r.status}`); + + // Must have text/event-stream Content-Type + assert.ok( + (r.headers['content-type'] ?? '').includes('text/event-stream'), + `Expected text/event-stream, got: ${r.headers['content-type']}` + ); + + // Must carry all 5 X-OLP-* headers + assert.ok(r.headers['x-olp-provider-used'], 'Must have X-OLP-Provider-Used'); + assert.ok(r.headers['x-olp-model-used'], 'Must have X-OLP-Model-Used'); + assert.ok(r.headers['x-olp-fallback-hops'] !== undefined, 'Must have X-OLP-Fallback-Hops'); + assert.ok(r.headers['x-olp-cache'], 'Must have X-OLP-Cache'); + assert.ok(r.headers['x-olp-latency-ms'] !== undefined, 'Must have X-OLP-Latency-Ms'); + + // Body must contain [DONE] terminator + assert.ok(r.body.includes('[DONE]'), `Expected [DONE] in body, got: ${r.body.slice(0, 200)}`); + } finally { + if (savedProvider !== undefined) { + lpD35_9.set('anthropic', savedProvider); + } else { + lpD35_9.delete('anthropic'); + } + } + }); + + it('#10: post-first-chunk throw → truncation marker (finish_reason:length) + [DONE] in response', async () => { + // Provider spawn yields 1 delta chunk then throws. + // Before D35 fix: catch block called res.end() silently — no [DONE], no truncation marker. + // After D35 fix: emits {type:'stop', finish_reason:'length'} SSE chunk + [DONE] before res.end(). + const { loadedProviders: lpD35_10, __clearCache: ccD35_10 } = await import('./server.mjs'); + const savedProvider = lpD35_10.get('anthropic'); + const oneChunkThenThrow = { + ...savedProvider, + spawn: async function* () { + yield { type: 'delta', role: 'assistant', content: 'partial-content' }; + throw new Error('post-first-chunk provider error'); + }, + }; + lpD35_10.set('anthropic', oneChunkThenThrow); + ccD35_10(); + + try { + const r = await fetch({ + port: portD35, + method: 'POST', + path: '/v1/chat/completions', + body: { + model: 'claude-sonnet-4-6', + messages: [{ role: 'user', content: 'post-chunk-error-test' }], + stream: true, + }, + }); + + // Status must be 200 (headers already sent with first chunk) + assert.equal(r.status, 200, `Expected 200 (headers sent at first chunk), got ${r.status}`); + + // Body must contain the [DONE] terminator + assert.ok(r.body.includes('[DONE]'), `Expected [DONE] in body, got: ${r.body.slice(0, 400)}`); + + // Body must contain the finish_reason:'length' truncation marker + assert.ok( + r.body.includes('"length"'), + `Expected finish_reason:"length" truncation marker in body, got: ${r.body.slice(0, 400)}` + ); + } finally { + if (savedProvider !== undefined) { + lpD35_10.set('anthropic', savedProvider); + } else { + lpD35_10.delete('anthropic'); + } + } + }); + + it('#10b: post-first-chunk error-chunk → truncation marker + [DONE] (error-chunk variant)', async () => { + // Provider spawn yields 1 delta then yields an error irChunk (type='error'). + // The error-chunk branch inside the for-await loop mirrors the catch-block fix. + const { loadedProviders: lpD35_10b, __clearCache: ccD35_10b } = await import('./server.mjs'); + const savedProvider = lpD35_10b.get('anthropic'); + const oneChunkThenErrorChunk = { + ...savedProvider, + spawn: async function* () { + yield { type: 'delta', role: 'assistant', content: 'partial-content-b' }; + yield { type: 'error', error: 'provider emitted error chunk after first' }; + }, + }; + lpD35_10b.set('anthropic', oneChunkThenErrorChunk); + ccD35_10b(); + + try { + const r = await fetch({ + port: portD35, + method: 'POST', + path: '/v1/chat/completions', + body: { + model: 'claude-sonnet-4-6', + messages: [{ role: 'user', content: 'error-chunk-after-first-test' }], + stream: true, + }, + }); + + assert.equal(r.status, 200, `Expected 200, got ${r.status}`); + assert.ok(r.body.includes('[DONE]'), `Expected [DONE] in body, got: ${r.body.slice(0, 400)}`); + assert.ok( + r.body.includes('"length"'), + `Expected finish_reason:"length" in body, got: ${r.body.slice(0, 400)}` + ); + } finally { + if (savedProvider !== undefined) { + lpD35_10b.set('anthropic', savedProvider); + } else { + lpD35_10b.delete('anthropic'); + } + } + }); + +}); + // ── Suite 17: D18 — /v1/models population + X-OLP-* headers on errors ──────── // // Finding 10: /v1/models returned empty data regardless of loaded providers.