mirror of
https://github.com/dtzp555-max/olp.git
synced 2026-07-21 21:15:10 +00:00
chore: D19 — cleanup batch (Findings 8 + 14 + 15 + D17 dead import)
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 <noreply@anthropic.com>
This commit is contained in:
@@ -4,7 +4,6 @@ on:
|
|||||||
pull_request:
|
pull_request:
|
||||||
paths:
|
paths:
|
||||||
- 'server.mjs'
|
- 'server.mjs'
|
||||||
- 'setup.mjs'
|
|
||||||
- 'lib/**'
|
- 'lib/**'
|
||||||
- 'scripts/**'
|
- 'scripts/**'
|
||||||
- 'models-registry.json'
|
- 'models-registry.json'
|
||||||
@@ -13,7 +12,6 @@ on:
|
|||||||
branches: [main]
|
branches: [main]
|
||||||
paths:
|
paths:
|
||||||
- 'server.mjs'
|
- 'server.mjs'
|
||||||
- 'setup.mjs'
|
|
||||||
- 'lib/**'
|
- 'lib/**'
|
||||||
- 'scripts/**'
|
- 'scripts/**'
|
||||||
- 'models-registry.json'
|
- 'models-registry.json'
|
||||||
@@ -47,20 +45,6 @@ jobs:
|
|||||||
"api.anthropic.com/api/oauth/usage"
|
"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
|
# Source files in scope. Exclude docs, CHANGELOG, README, the
|
||||||
# workflow itself, and the test file (which may pin historical
|
# workflow itself, and the test file (which may pin historical
|
||||||
# strings intentionally).
|
# strings intentionally).
|
||||||
|
|||||||
+33
-3
@@ -13,6 +13,36 @@
|
|||||||
|
|
||||||
import { randomBytes } from 'node:crypto';
|
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 ─────────────────────────────────────────────────────────
|
// ── ID generation ─────────────────────────────────────────────────────────
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -49,7 +79,7 @@ export function irChunkToOpenAISSE(irChunk, requestId, model) {
|
|||||||
choices: [{
|
choices: [{
|
||||||
index: 0,
|
index: 0,
|
||||||
delta: {},
|
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
|
// 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);
|
tool_calls.push(...chunk.tool_calls);
|
||||||
}
|
}
|
||||||
} else if (chunk.type === 'stop') {
|
} else if (chunk.type === 'stop') {
|
||||||
if (chunk.finish_reason) {
|
if (chunk.finish_reason !== undefined) {
|
||||||
finish_reason = chunk.finish_reason;
|
finish_reason = normalizeFinishReason(chunk.finish_reason);
|
||||||
}
|
}
|
||||||
if (chunk.usage) {
|
if (chunk.usage) {
|
||||||
usage = chunk.usage;
|
usage = chunk.usage;
|
||||||
|
|||||||
@@ -50,7 +50,7 @@ import { execFileSync, execSync } from 'node:child_process';
|
|||||||
import { existsSync, readFileSync } from 'node:fs';
|
import { existsSync, readFileSync } from 'node:fs';
|
||||||
import { join } from 'node:path';
|
import { join } from 'node:path';
|
||||||
import { homedir } from 'node:os';
|
import { homedir } from 'node:os';
|
||||||
import { ProviderError, PROVIDER_ERROR_CODES } from './base.mjs';
|
import { ProviderError } from './base.mjs';
|
||||||
|
|
||||||
// ── Binary resolution ─────────────────────────────────────────────────────
|
// ── Binary resolution ─────────────────────────────────────────────────────
|
||||||
// OLP_CLAUDE_BIN env takes priority, then falls back to 'claude' from PATH.
|
// OLP_CLAUDE_BIN env takes priority, then falls back to 'claude' from PATH.
|
||||||
|
|||||||
@@ -132,7 +132,7 @@ import { execSync } from 'node:child_process';
|
|||||||
import { existsSync, readFileSync } from 'node:fs';
|
import { existsSync, readFileSync } from 'node:fs';
|
||||||
import { join } from 'node:path';
|
import { join } from 'node:path';
|
||||||
import { homedir } from 'node:os';
|
import { homedir } from 'node:os';
|
||||||
import { ProviderError, PROVIDER_ERROR_CODES } from './base.mjs';
|
import { ProviderError } from './base.mjs';
|
||||||
|
|
||||||
// ── Binary resolution ─────────────────────────────────────────────────────
|
// ── Binary resolution ─────────────────────────────────────────────────────
|
||||||
// OLP_CODEX_BIN env takes priority, then falls back to 'codex' from PATH.
|
// OLP_CODEX_BIN env takes priority, then falls back to 'codex' from PATH.
|
||||||
|
|||||||
@@ -230,7 +230,7 @@ import { execSync } from 'node:child_process';
|
|||||||
import { existsSync, readFileSync } from 'node:fs';
|
import { existsSync, readFileSync } from 'node:fs';
|
||||||
import { join } from 'node:path';
|
import { join } from 'node:path';
|
||||||
import { homedir } from 'node:os';
|
import { homedir } from 'node:os';
|
||||||
import { ProviderError, PROVIDER_ERROR_CODES } from './base.mjs';
|
import { ProviderError } from './base.mjs';
|
||||||
|
|
||||||
// ── Binary resolution ─────────────────────────────────────────────────────
|
// ── Binary resolution ─────────────────────────────────────────────────────
|
||||||
// OLP_VIBE_BIN env takes priority, then falls back to 'vibe' from PATH.
|
// OLP_VIBE_BIN env takes priority, then falls back to 'vibe' from PATH.
|
||||||
|
|||||||
+1
-1
@@ -29,7 +29,7 @@ import {
|
|||||||
generateRequestId,
|
generateRequestId,
|
||||||
SSE_DONE,
|
SSE_DONE,
|
||||||
} from './lib/ir/ir-to-openai.mjs';
|
} 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 { ProviderError } from './lib/providers/base.mjs';
|
||||||
import { computeCacheKey, hasCacheControl, extractCacheControlMarkers } from './lib/cache/keys.mjs';
|
import { computeCacheKey, hasCacheControl, extractCacheControlMarkers } from './lib/cache/keys.mjs';
|
||||||
import { CacheStore } from './lib/cache/store.mjs';
|
import { CacheStore } from './lib/cache/store.mjs';
|
||||||
|
|||||||
@@ -391,6 +391,57 @@ describe('irChunkToOpenAISSE format', () => {
|
|||||||
assert.equal(resp.choices[0].finish_reason, 'stop');
|
assert.equal(resp.choices[0].finish_reason, 'stop');
|
||||||
assert.equal(resp.usage.total_tokens, 7);
|
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 ─────────────────────────────────
|
// ── Suite 4: Provider contract validation ─────────────────────────────────
|
||||||
|
|||||||
Reference in New Issue
Block a user