fix+test: D35 — pre-Phase-2 batch #1 (issues #4 #9 #10 #11 #12)

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 <noreply@anthropic.com>
2026-07-19 09:45:07 +00:00 · 2026-05-24 20:20:49 +10:00
parent 9db4266bc5
commit 2600185edb
4 changed files with 302 additions and 5 deletions
@@ -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'

@@ -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') {
@@ -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.
@@ -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.