mirror of
https://github.com/dtzp555-max/olp.git
synced 2026-07-21 21:15:10 +00:00
main
3
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  taodengandClaude Opus 4.7
|
ed82e65859 |
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>
|
||
|
taodengandClaude Opus 4.7
|
4b1a9c8808 |
fix(ir): D12 — remove invented top-level error field on OpenAI response shapes
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 <noreply@anthropic.com>
|
||
|
taodengandClaude Opus 4.7 (noreply@anthropic.com)
|
e2e67de23a |
feat(phase-1): land IR + plugin loader + server skeleton (D3)
Phase 1 Day 1. First executable code lands. Zero providers wired yet
(per ALIGNMENT.md "v0.1 ships 0 Enabled Providers"); the server starts
clean and POST /v1/chat/completions returns 503 with no_enabled_provider.
Files added:
lib/ir/types.mjs - IR v1.0 schema + validators (ADR 0003)
lib/ir/openai-to-ir.mjs - OpenAI Chat Completions to IR
lib/ir/ir-to-openai.mjs - IR chunks to OpenAI SSE / non-stream
lib/providers/base.mjs - Provider contract + validateProvider + ProviderError
lib/providers/index.mjs - Static empty registry stub (ADR 0002)
server.mjs - HTTP listener with createOlpServer factory + main guard
test-features.mjs - 61 tests across 7 suites (IR / provider / HTTP)
Files modified:
package.json - main and scripts.start/test added back; targets now exist.
Authority citations:
IR fields and translation direction: ADR 0003 sections Decision and
Translation direction model.
Provider contract (9 fields): ADR 0002 section Provider contract v1.0
interface.
Entry surface routes (health, v1/models, v1/chat/completions): OLP v0.1
spec section 4.1 single-protocol entry; ALIGNMENT.md Authority 2.
Zero-Enabled-Providers behaviour: ALIGNMENT.md Provider Inventory.
Architectural decisions worth recording:
1. server.mjs uses a createOlpServer factory plus an import.meta.url
main guard. The factory returns an unbound http.Server; only the
main-script invocation calls .listen(). Tests import the real
server.mjs and exercise the real router. No parallel implementation
in the test file.
This pattern was a fold-in from the orchestration step. The initial
sonnet draft put a top-level server.listen call in server.mjs, which
forced test-features.mjs to reimplement the router inline (a false-
confidence trap because the real server logic would never be tested).
Refactored before reviewer dispatch.
2. lib/providers/index.mjs ships an empty STATIC_REGISTRY array, not a
placeholder with dummy entries. ALIGNMENT.md Provider Inventory says
v0.1 ships zero Enabled Providers; the registry honors that exactly.
Phase 1 Day 2 adds the first import (Anthropic) when its plugin lands.
3. BadRequestError lives in openai-to-ir.mjs and ProviderError in
base.mjs. Reviewer suggested relocating to a shared lib/errors.mjs
once the count exceeds two; deferred to Phase 1 Day 2 to ship with
the third typed error class.
4. contractVersion: '1.0' on each provider plugin: not enforced at D3
because no providers exist yet. Reviewer flagged for Phase 1 Day 2
tightening when the first provider lands.
Reviewer chain (Iron Rule 10):
Initial implementer: sonnet (general-purpose).
Refactor (createOlpServer + main guard) by the orchestrator after
catching the inline-router parallel-implementation issue.
Fresh-context reviewer: opus (ecc:code-reviewer). Verdict
APPROVE_WITH_MINOR.
Reviewer's two non-blocking findings folded in:
F1: removed unused createServer import from test-features.mjs line 12,
left over from the refactor.
F2: replaced finish_reason value 'error' with 'stop' in both the
streaming error chunk path (lib/ir/ir-to-openai.mjs line 72) and
the non-streaming error aggregation path (lib/ir/ir-to-openai.mjs
line 153). The 'error' value is not in OpenAI's documented
finish_reason enum (stop / length / tool_calls / content_filter /
function_call / null), so emitting it would violate ALIGNMENT.md
Rule 2 (b). Provider errors are now surfaced via a top-level
response.error object plus an inline content marker. The matching
test assertion at test-features.mjs line 325 was updated to verify
finish_reason stays within the OpenAI enum.
Note on the F2 fold-in:
Reviewer pointed only at the streaming path (line 72). After applying
that fix I ran grep across lib/ and test-features.mjs for the same
invention pattern and caught a second hit at line 153 (non-streaming
aggregation). This is the "fold-in must grep the full repo, not only
the file the reviewer named" discipline from
~/.cc-rules/memory/feedback/evidence_first_under_speed_pressure.md.
Both hits are fixed in this commit.
Verification:
node --check on all 7 new files plus modified package.json plus
server.mjs plus lib/ir/ir-to-openai.mjs - all clean.
npm test - 61/61 pass in 209ms, no flakes, no skipped.
OLP_PORT=14001 node server.mjs followed by curl /health returns
proper JSON; curl /v1/models returns 200 empty list; server shuts
down cleanly on signal.
grep "finish_reason.*error" returns zero hits across lib/ and tests.
Co-Authored-By: Claude Opus 4.7 (noreply@anthropic.com)
|