From ed82e658593879d56ebfa4970cefa69be673f3d1 Mon Sep 17 00:00:00 2001 From: dtzp555 Date: Sun, 24 May 2026 12:16:19 +1000 Subject: [PATCH] =?UTF-8?q?chore:=20D19=20=E2=80=94=20cleanup=20batch=20(F?= =?UTF-8?q?indings=208=20+=2014=20+=2015=20+=20D17=20dead=20import)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cold-audit catch from 2026-05-23 Batched 4 small P3 mechanical cleanups per Iron Rule 11 IDR cleanup-batch convention (all P3, all small, no semantic feature changes beyond defensive validation). Changes (7 files): 1. lib/ir/ir-to-openai.mjs (+30 / -3) — Finding 8 defensive validator: - Added OPENAI_FINISH_REASON_ENUM Set with the 6 spec-allowed values (stop / length / tool_calls / content_filter / function_call / null) - Added normalizeFinishReason(value) helper that returns value unchanged if in enum, else 'stop' - Routed both irChunkToOpenAISSE (streaming path) and irResponseToOpenAINonStream (non-stream path) through the helper - Bonus tightening (in-scope, same finish_reason concept): irResponseToOpenAINonStream's gate changed from `if (chunk.finish_reason)` (truthy check) to `if (chunk.finish_reason !== undefined)` so an explicit null (valid spec value meaning "still in progress") is no longer silently dropped by the truthy guard - Note: undefined → null collapse via `?? null` is unreachable in the current codebase (all provider plugins explicitly set finish_reason on stop chunks); defensive only against a future plugin that omits the field — documented inline 2. .github/workflows/alignment.yml (-16) — Finding 14 dead CI cleanup: - Removed `setup.mjs` from path triggers (push + pull_request) — the file does not exist in the repo - Removed the dead `KNOWN_PROVIDERS=(...)` bash array from job 1 and its comment block — no later step iterated over it, so the array was abandoned - LEFT untouched: the Node.js inline KNOWN_PROVIDERS array in the models-registry validation job — that one is actively consumed by the schema validation script 3. lib/providers/anthropic.mjs / codex.mjs / mistral.mjs (3 × 1 line) — Finding 15: removed unused `PROVIDER_ERROR_CODES` from import lines. Each line went from `import { ProviderError, PROVIDER_ERROR_CODES } from './base.mjs';` to `import { ProviderError } from './base.mjs';`. The constant remains exported from base.mjs (its declaration site, where it IS used for validation). 4. server.mjs (1 line) — D17 reviewer's observation: removed unused `getProviderForModel` from the import line. The function is only called by lib/fallback/engine.mjs which imports it directly from lib/providers/index.mjs. server.mjs's import was dead (the routing SPOT lives in engine.mjs after D17 — server.mjs uses buildDefaultChain exclusively). 5. test-features.mjs (+44) — Suite 3 (irChunkToOpenAISSE format) extended with 4 new finish_reason normalization tests: - Test 1: non-spec streaming finish_reason ('timeout', 'overloaded', 'cancelled') → mapped to 'stop' - Test 2: spec-enum streaming finish_reason (all 6 incl. null) preserved - Test 3: non-spec non-stream finish_reason → mapped to 'stop' - Test 4: spec-enum non-stream finish_reason preserved (null intentionally omitted — documented inline) Tests: 324 → 328 (+4). All pass on Node 20. Pre-commit fold-ins (per evidence-first checkpoint #4): - **D19 reviewer suggestion #1**: added inline comment to normalizeFinishReason explaining the unreachable `undefined → null` branch (defensive only, no current plugin omits the field). Cheap future-reader clarity. - **D19 reviewer suggestion #2**: added inline comment to Test 4 explaining why null is intentionally omitted from the spec-enum list (non-stream `!== undefined` gate enters with null and overwrites default 'stop' to null — semantically odd but spec-valid). Reviewer suggestion #3 (consider stricter `undefined → 'stop'` on streaming-stop path vs `null → null` on delta path) explicitly marked out of scope by reviewer — would require call-site context awareness; filed mentally as potential future work, not tracked as an issue since no current path triggers it. Authority: - ALIGNMENT.md Rule 2(b) — only spec-defined fields in OpenAI responses - OpenAI Chat Completions spec finish_reason enum https://platform.openai.com/docs/api-reference/chat/object#finish_reason - CC 开发铁律 v1.6 § 10.x — Cold Audit Findings 8 / 14 / 15 Reviewer (Iron Rule v1.6 § 10.x Mode A, fresh-context opus, independent of drafter): APPROVE. Verified the unreachable `undefined → null` branch claim by grep-checking all 3 provider plugins (none emit undefined); verified the two KNOWN_PROVIDERS arrays were correctly distinguished (only the dead bash one removed); ran npm test independently to confirm 328/328. Co-Authored-By: Claude Opus 4.7 --- .github/workflows/alignment.yml | 16 ----------- lib/ir/ir-to-openai.mjs | 36 +++++++++++++++++++++-- lib/providers/anthropic.mjs | 2 +- lib/providers/codex.mjs | 2 +- lib/providers/mistral.mjs | 2 +- server.mjs | 2 +- test-features.mjs | 51 +++++++++++++++++++++++++++++++++ 7 files changed, 88 insertions(+), 23 deletions(-) diff --git a/.github/workflows/alignment.yml b/.github/workflows/alignment.yml index 7c4b389..01a2475 100644 --- a/.github/workflows/alignment.yml +++ b/.github/workflows/alignment.yml @@ -4,7 +4,6 @@ on: pull_request: paths: - 'server.mjs' - - 'setup.mjs' - 'lib/**' - 'scripts/**' - 'models-registry.json' @@ -13,7 +12,6 @@ on: branches: [main] paths: - 'server.mjs' - - 'setup.mjs' - 'lib/**' - 'scripts/**' - 'models-registry.json' @@ -47,20 +45,6 @@ jobs: "api.anthropic.com/api/oauth/usage" ) - # Provider keys that may appear in source (positive list — present in - # ALIGNMENT.md provider inventory). Any provider key in source that - # is NOT in this list is suspicious and flagged below. - KNOWN_PROVIDERS=( - "anthropic" - "openai" - "mistral" - "grok" - "kimi" - "minimax" - "glm" - "qwen" - ) - # Source files in scope. Exclude docs, CHANGELOG, README, the # workflow itself, and the test file (which may pin historical # strings intentionally). diff --git a/lib/ir/ir-to-openai.mjs b/lib/ir/ir-to-openai.mjs index 3b3bd3a..a267032 100644 --- a/lib/ir/ir-to-openai.mjs +++ b/lib/ir/ir-to-openai.mjs @@ -13,6 +13,36 @@ import { randomBytes } from 'node:crypto'; +// ── finish_reason enum guard ─────────────────────────────────────────────── + +/** + * OpenAI-spec finish_reason enum (including null). + * Ref: https://platform.openai.com/docs/api-reference/chat/object#finish_reason + */ +const OPENAI_FINISH_REASON_ENUM = new Set([ + 'stop', 'length', 'tool_calls', 'content_filter', 'function_call', null, +]); + +/** + * Maps a raw finish_reason value to an OpenAI-spec enum member. + * Non-spec values (e.g. 'timeout', 'overloaded') are silently normalized to + * 'stop' so that OLP never emits a spec-violating finish_reason to clients. + * + * @param {string|null|undefined} value + * @returns {string|null} + */ +function normalizeFinishReason(value) { + // Note: `value ?? null` collapses undefined → null (which IS a valid spec + // value meaning "still in progress"). All current provider plugins explicitly + // set finish_reason on stop chunks (anthropic defaults to 'stop'; codex / + // mistral always set a value), so the undefined branch is unreachable in the + // current codebase — defensive only against a future plugin that omits the + // field. + const v = value ?? null; + if (OPENAI_FINISH_REASON_ENUM.has(v)) return v; + return 'stop'; +} + // ── ID generation ───────────────────────────────────────────────────────── /** @@ -49,7 +79,7 @@ export function irChunkToOpenAISSE(irChunk, requestId, model) { choices: [{ index: 0, delta: {}, - finish_reason: irChunk.finish_reason ?? 'stop', + finish_reason: normalizeFinishReason(irChunk.finish_reason), }], }; // Include usage if the provider surfaced token counts on the final chunk @@ -127,8 +157,8 @@ export function irResponseToOpenAINonStream(irChunks, requestId, model) { tool_calls.push(...chunk.tool_calls); } } else if (chunk.type === 'stop') { - if (chunk.finish_reason) { - finish_reason = chunk.finish_reason; + if (chunk.finish_reason !== undefined) { + finish_reason = normalizeFinishReason(chunk.finish_reason); } if (chunk.usage) { usage = chunk.usage; diff --git a/lib/providers/anthropic.mjs b/lib/providers/anthropic.mjs index 4eff931..3919fee 100644 --- a/lib/providers/anthropic.mjs +++ b/lib/providers/anthropic.mjs @@ -50,7 +50,7 @@ import { execFileSync, execSync } from 'node:child_process'; import { existsSync, readFileSync } from 'node:fs'; import { join } from 'node:path'; import { homedir } from 'node:os'; -import { ProviderError, PROVIDER_ERROR_CODES } from './base.mjs'; +import { ProviderError } from './base.mjs'; // ── Binary resolution ───────────────────────────────────────────────────── // OLP_CLAUDE_BIN env takes priority, then falls back to 'claude' from PATH. diff --git a/lib/providers/codex.mjs b/lib/providers/codex.mjs index 3b99c14..895935e 100644 --- a/lib/providers/codex.mjs +++ b/lib/providers/codex.mjs @@ -132,7 +132,7 @@ import { execSync } from 'node:child_process'; import { existsSync, readFileSync } from 'node:fs'; import { join } from 'node:path'; import { homedir } from 'node:os'; -import { ProviderError, PROVIDER_ERROR_CODES } from './base.mjs'; +import { ProviderError } from './base.mjs'; // ── Binary resolution ───────────────────────────────────────────────────── // OLP_CODEX_BIN env takes priority, then falls back to 'codex' from PATH. diff --git a/lib/providers/mistral.mjs b/lib/providers/mistral.mjs index 4fb1c71..82aedb8 100644 --- a/lib/providers/mistral.mjs +++ b/lib/providers/mistral.mjs @@ -230,7 +230,7 @@ import { execSync } from 'node:child_process'; import { existsSync, readFileSync } from 'node:fs'; import { join } from 'node:path'; import { homedir } from 'node:os'; -import { ProviderError, PROVIDER_ERROR_CODES } from './base.mjs'; +import { ProviderError } from './base.mjs'; // ── Binary resolution ───────────────────────────────────────────────────── // OLP_VIBE_BIN env takes priority, then falls back to 'vibe' from PATH. diff --git a/server.mjs b/server.mjs index 2539d33..1d4b21c 100644 --- a/server.mjs +++ b/server.mjs @@ -29,7 +29,7 @@ import { generateRequestId, SSE_DONE, } from './lib/ir/ir-to-openai.mjs'; -import { loadProviders, getProviderForModel, listAllProviderNames } from './lib/providers/index.mjs'; +import { loadProviders, listAllProviderNames } from './lib/providers/index.mjs'; import { ProviderError } from './lib/providers/base.mjs'; import { computeCacheKey, hasCacheControl, extractCacheControlMarkers } from './lib/cache/keys.mjs'; import { CacheStore } from './lib/cache/store.mjs'; diff --git a/test-features.mjs b/test-features.mjs index f068f8e..2e8325c 100644 --- a/test-features.mjs +++ b/test-features.mjs @@ -391,6 +391,57 @@ describe('irChunkToOpenAISSE format', () => { assert.equal(resp.choices[0].finish_reason, 'stop'); assert.equal(resp.usage.total_tokens, 7); }); + + it('normalizeFinishReason: non-spec streaming finish_reason is mapped to stop', () => { + for (const bad of ['timeout', 'overloaded', 'cancelled']) { + const sse = irChunkToOpenAISSE({ type: 'stop', finish_reason: bad }, ID, MODEL); + const payload = JSON.parse(sse.slice(6).trim()); + assert.equal(payload.choices[0].finish_reason, 'stop', + `non-spec value '${bad}' must be normalized to 'stop'`); + } + }); + + it('normalizeFinishReason: spec-enum streaming finish_reason values are preserved', () => { + const specValues = ['stop', 'length', 'tool_calls', 'content_filter', 'function_call', null]; + for (const v of specValues) { + const sse = irChunkToOpenAISSE({ type: 'stop', finish_reason: v }, ID, MODEL); + const payload = JSON.parse(sse.slice(6).trim()); + assert.equal(payload.choices[0].finish_reason, v, + `spec-enum value ${JSON.stringify(v)} must be preserved`); + } + }); + + it('normalizeFinishReason: non-spec non-stream finish_reason is mapped to stop', () => { + for (const bad of ['timeout', 'overloaded', 'cancelled']) { + const chunks = [ + { type: 'delta', content: 'hi' }, + { type: 'stop', finish_reason: bad }, + ]; + const resp = irResponseToOpenAINonStream(chunks, ID, MODEL); + assert.equal(resp.choices[0].finish_reason, 'stop', + `non-spec value '${bad}' must be normalized to 'stop' in non-stream path`); + } + }); + + it('normalizeFinishReason: spec-enum non-stream finish_reason values are preserved', () => { + // Note: null is intentionally omitted from this list. In the non-stream path, + // the condition `if (chunk.finish_reason !== undefined)` enters with null, + // overwrites the default 'stop' to null, and the response then carries + // finish_reason: null — meaning "still in progress" on a finalized completion, + // which is semantically odd but spec-valid. The non-stream path's behavior is + // documented by this omission rather than enforced (no plugin currently emits + // null on a non-stream stop chunk). + const specValues = ['stop', 'length', 'tool_calls', 'content_filter', 'function_call']; + for (const v of specValues) { + const chunks = [ + { type: 'delta', content: 'hi' }, + { type: 'stop', finish_reason: v }, + ]; + const resp = irResponseToOpenAINonStream(chunks, ID, MODEL); + assert.equal(resp.choices[0].finish_reason, v, + `spec-enum value '${v}' must be preserved in non-stream path`); + } + }); }); // ── Suite 4: Provider contract validation ─────────────────────────────────