mirror of
https://github.com/dtzp555-max/olp.git
synced 2026-07-21 21:15:10 +00:00
60570ef0743360465d999c7ab71cb67da058cb86
7
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  taodengandClaude Opus 4.7
|
60570ef074 |
fix+docs: D34 — FINAL batch (F1+F4+F7+F8); audit cadence stops
cold-audit catch from 2026-05-24 (round 6 — FINAL)
This is the closing D-day of a 24-day round-1→round-6 audit cycle.
After this commit + the 9 round-6 follow-up issue filings, no more
audit rounds. Trajectory R1=17 → R2=13 → R3=13 → R4=10 → R5=12 →
R6=14 — the method did not converge; owner chose Option A (focused
batch of most consequential items, then STOP).
Changes (6 files, +138 / -47):
**Code changes**
1. lib/cache/keys.mjs (+14/-?) — F4 P2 cache key array-field
normalization:
- New `normalizeArrayField` helper: `(Array.isArray(v) && v.length === 0) ? null : (v ?? null)`
- Applied to `tools` and `stop` in computeCacheKey
- Now `tools: []` and `tools` omitted produce IDENTICAL cache
keys (and same for `stop: []` vs omitted). ADR 0005 Amendment 2's
own claim that "[] and undefined share a cache entry" was
empirically FALSE pre-D34; round-6 reviewer verified hashes
differ. The fix makes the claim literally true at the
key-composition layer.
2. lib/providers/base.mjs (+16/-?) — F7 P2 dead error code removal:
- `QUOTA_EXHAUSTED` removed from PROVIDER_ERROR_CODES
- `RATE_LIMITED` removed from PROVIDER_ERROR_CODES
- v0.1 live codes: SPAWN_FAILED, CLI_NOT_FOUND, AUTH_MISSING,
SPAWN_TIMEOUT
- Comment block documents removal + cites ADR 0004 Amendment 3
3. lib/fallback/engine.mjs (+29/-?) — F7 P2 sibling:
- HARD_TRIGGER_CODES: QUOTA_EXHAUSTED + RATE_LIMITED removed
- SPAWN_FAILED, CLI_NOT_FOUND, AUTH_MISSING(false), SPAWN_TIMEOUT
remain
- evaluateHardTriggers HTTP-status branches KEPT (option (b)) with
forward-compat comment: "v0.1 plugins never attach statusCode;
branches reserved for v1.x when plugin gains HTTP-status parsing"
4. test-features.mjs (+93) — F4 + F7 test work:
- 4 new F4 regression tests (tools:[] vs undefined, stop:[] vs
undefined, tools:[] vs null, tools:non-empty vs undefined sanity)
- ~14 integration test code-swap edits (QUOTA_EXHAUSTED →
SPAWN_FAILED, RATE_LIMITED → SPAWN_FAILED) preserving original
hard-trigger semantic
- 2 dead unit tests for QUOTA_EXHAUSTED/RATE_LIMITED removed
(tombstone comment retained for audit trail)
**ADR amendments (docs-only, no code change)**
5. docs/adr/0004-fallback-engine.md (+12) — F7 Amendment 3:
- Documents the v0.1 hard-trigger code narrowing
- 4 live codes listed explicitly
- Captures evaluateHardTriggers HTTP-status branch retention rationale
- v1.x re-activation path: plugin gains HTTP-status parsing →
re-add codes → branches activate naturally
6. docs/adr/0005-cache-cross-provider.md (+21) — TWO amendments + 1
prior-amendment update:
- **Amendment 6 (F1 P1)**: Formal v1.x deferral of D4 streaming
singleflight. Buffered path (executeHopFn) uses cacheStore.getOrCompute
and participates in D4 fully. Streaming cache-miss path
(server.mjs:609-741) bypasses singleflight — N concurrent identical
streamers each spawn fresh. v0.1 trade-off accepted for
personal/family scale; v1.x design ADR needed for tee-streaming +
per-key inflight Map. Cross-references CLAUDE.md release_kit.
phase_rolling_mode as the deferral pattern precedent.
- **Amendment 7 (F8 P2)**: Documents the v0.1 conservative cache-key
posture: includes all IR fields including those plugins discard
(anthropic/codex/mistral drop temperature/max_tokens/top_p/stop/
tools/tool_choice at spawn). Consequence: 2 requests with different
temperature produce identical CLI output (CLI ignores) but
different cache keys → spurious miss. Trade-off justified:
spurious miss > spurious hit. v1.x forward path:
per-plugin cacheKeyFields contract extension (ADR 0002 amendment
needed). 3 implementation subtasks enumerated for the v1.x PR.
- **Amendment 2 update (F4)**: heading renamed to "Note on
null-coalescing AND array normalization"; body documents the
new normalizeArrayField helper; quotes the regression test name.
Tests: 414 → 416 (+4 F4 regression, -2 F7 dead, +0 net from F7
integration rewrites).
Pre-commit fold-in: NONE — D34 reviewer APPROVE with all 4 suggestions
non-blocking/cosmetic.
Authority:
- ADR 0005 Amendment 2 invariant restored at code level (F4)
- ADR 0005 Amendment 6 formalizes F1 streaming-singleflight deferral
per the same pattern as D22 ADR 0004 Amendment 2 soft-trigger
deferral
- ADR 0005 Amendment 7 documents F8 conservative posture as v0.1
intentional design (not an accident)
- ADR 0004 Amendment 3 narrows v0.1 trigger taxonomy (F7)
- Round-6 cold audit findings F2 / F3 / F6 / F9 / F10 / F11 / F12 /
F13 / F14 filed as GitHub issues after this commit (NOT in scope)
- CC 开发铁律 v1.6 § 10.x — final round of the audit cadence
Reviewer (Iron Rule v1.6 § 10.x Mode A, fresh-context opus, independent
of drafter): APPROVE. Critical depth checks:
- B5 over-normalization: verified `normalizeArrayField` only applies
to `tools` and `stop`; `response_format: {}` and `tool_choice: ''`
unaffected (Array.isArray guard)
- C10 test cleanup: 21 references reconciled (3 tombstone, 18
rewrites/removals); integration test rewrites preserve hard-trigger
semantics (QUOTA_EXHAUSTED → SPAWN_FAILED is also a hard trigger,
so fallback advancement behavior unchanged)
---
**End of audit cycle.** 24 D-days shipped from D10 (P1 hardening) through
D34 (final batch). 6 cold-audit rounds executed; 78+ findings raised;
~50 closed via implementation; ~28 deferred to GitHub issues / v1.x
ADR amendments. v0.1 tag remains explicit-maintainer-action per
phase_rolling_mode policy.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
||
|
taodengandClaude Opus 4.7
|
30de965e8e |
fix+docs: D32 — round-4 cleanup batch (F2/F3/F4/F5/F7/F8/F9)
cold-audit catch from 2026-05-24 (round 4) Round-4 cold-audit cleanup batch. 7 items grouped per IDR cleanup-batch convention (D19/D20/D25/D26/D30/D31 precedent). 2 P2 + 3 ADR amendments + 2 small docs/code cleanups. F10 (P3 semantic edge case) filed separately as GitHub issue #8. Changes (8 files, +200 / -38): **P2 fixes** 1. **F3 — README missing 6 provider auth env vars** (README.md): D30 fixed the `OLP_*` table but missed the auth-bearing env vars actually read by plugin code: - `CLAUDE_CODE_OAUTH_TOKEN` (anthropic.mjs:84) — highest-precedence override; bypasses keychain + .credentials.json file lookup - `OPENAI_CODEX_AUTH_PATH` (codex.mjs:163) — overrides full auth file path; when set, no other path tried - `CODEX_HOME` (codex.mjs:176) — overrides base dir; default auth path becomes `$CODEX_HOME/auth.json` - `MISTRAL_API_KEY` (mistral.mjs:260) — directly supplies API key; highest precedence per Mistral DOCS-2 - `MISTRAL_VIBE_AUTH_PATH` (mistral.mjs:265) — overrides full .env path; evaluated only when MISTRAL_API_KEY absent - `VIBE_HOME` (mistral.mjs:277) — overrides Vibe base dir; default auth path becomes `$VIBE_HOME/.env` Real onboarding-blocker fix: new users couldn't run OLP without knowing about these env vars; README now documents them in a "Per-provider auth env vars" subsection. 2. **F8 — Early-return error paths missing X-OLP-* headers** (server.mjs): ADR 0004 § Observability + README claim "every response carries the 5 X-OLP-* headers" but 503 (no-enabled-providers), 415 (wrong Content-Type), and early 400s (bad JSON, IR validation) emitted only Content-Type + Content-Length + X-OLP-Latency-Ms (D18 only wired the chain-exhausted path). Operators debugging these paths got less info than the ADR promised. New helper `olpErrorHeaders({ startMs, model })` emits the canonical "no provider attempted" defaults: X-OLP-Provider-Used: 'none' X-OLP-Model-Used: model ?? 'unknown' X-OLP-Fallback-Hops: '0' X-OLP-Cache: 'bypass' X-OLP-Latency-Ms: <delta> 6 sendError call sites updated: 415 / 400-bad-JSON / 400-IR-parse (model: undefined → 'unknown') + 503-no-providers / 503-provider- disappeared / 500-engine-programming (model: ir.model). The 502 streaming-error-before-first-chunk path correctly stays on `olpHeaders(...)` since a provider WAS attempted. **ADR amendments** 3. **F2 — ADR 0003 model-mapping example correction** (docs/adr/0003, Amendment 2): the § Required fields example claimed `claude-sonnet-4-6` → `claude-sonnet-4-6-20260301` mapping happens in the provider plugin. This was wrong: per D17 SPOT decision (commit |
||
|
taodengandClaude Opus 4.7
|
4a238c9cc4 |
feat(fallback)+chore(cache): D28 — per-hop log observability fields (round-3 F2)
cold-audit catch from 2026-05-24 (round 3)
Round-3 cold-audit Finding 2 (P2 observability gap). ADR 0004 §
Observability headers requires per-hop structured log events to carry
7 fields: timestamp, chain id, hop index, failed provider, trigger type,
IR request hash, and downstream provider that was tried next. Pre-D28
events only carried timestamp/hop/provider/model/error/code — operators
could not:
- correlate hops across one logical request (no chain_id)
- pivot per-prompt via fingerprint (no ir_request_hash)
- tell from one log line which bucket classified the error (no
trigger_type)
- see lookahead routing (no next_provider)
Changes (3 files, +431 / -3):
1. lib/cache/keys.mjs (+35):
- New export `computeIRRequestHash(ir)` returning a 16-char SHA-256
prefix over the IR fields that define request semantics
(messages-normalized, tools, temperature, max_tokens, top_p, stop,
tool_choice, response_format). Excludes provider/model/cache_control
so the hash is provider-agnostic — identical prompts across a
fallback chain produce identical fingerprints, enabling log
correlation. 16-char prefix gives 2^64 collision resistance —
astronomically safe for the use case while keeping log lines compact.
2. lib/fallback/engine.mjs (+86 / -3):
- New helper `classifyTrigger(error)` returning one of:
'hard' / 'auth_missing' / 'client_error' / 'non_trigger' / null.
ProviderError with HARD_TRIGGER_CODES → 'hard'; AUTH_MISSING → its
own bucket; HTTP 400/401/403/404/422 → 'client_error' (per ADR 0004
§ "No fallback for client-side errors"); HTTP 5xx or non-client 4xx
(e.g. 429/529) → 'hard'; other → 'non_trigger'. 'soft' is reserved
in JSDoc for forward-compat per D22 Amendment 2's deferral of soft
triggers to v1.x.
- `executeWithFallback` now generates `chainId` (crypto.randomBytes(8)
.toString('hex')) and `irRequestHash` (computeIRRequestHash(ir))
ONCE at entry — both stay constant across all hops in the chain.
- All 8 logEvent call sites augmented with the 4 new fields:
fallback_soft_trigger, fallback_hop_success, fallback_hop_error,
fallback_client_error_no_fallback, fallback_auth_missing_no_fallback,
fallback_hard_trigger, fallback_non_trigger_error,
fallback_chain_exhausted.
- `next_provider` is `chain[i + 1]?.provider ?? null` on advancement
events; null on terminal events (success / client_error /
auth_missing / chain_exhausted).
- chain_id NOT reused from generateRequestId() since that produces
OpenAI-response-scoped `chatcmpl-<base64url>` IDs; chain_id is
internal log correlation and uses a clean 16-char hex token.
3. test-features.mjs (+313): 24 new tests covering:
- computeIRRequestHash determinism + 16-char hex shape + 8 per-field
sensitivity tests (one per IR input field)
- Provider-agnosticism (same hash across different provider/model)
- chain_id uniqueness across calls + consistency across hops
- trigger_type for each branch (hard / auth_missing / client_error /
non_trigger / null)
- next_provider correctness (chain[i+1] on advance; null at
exhaustion)
Tests: 376 → 400 (+24). All pass on Node 20.
Pure observation-only change: no fallback decision logic touched.
`evaluateHardTriggers`, `evaluateSoftTriggers`, `HARD_TRIGGER_CODES`,
`CLIENT_ERROR_STATUSES` are only READ by `classifyTrigger`; the existing
decision branches are textually unchanged — only the trailing logEvent
object literals are expanded.
Authority:
- ADR 0004 § Observability headers (the 7-field requirement)
https://github.com/dtzp555-max/olp/blob/main/docs/adr/0004-fallback-engine.md
- ADR 0004 § "No fallback for client-side errors" — 401/403/404/422
bucket (used by classifyTrigger's CLIENT_ERROR_STATUSES branch)
- ADR 0005 Amendment 2 (D15) — cache key composition fields, mirrored
in computeIRRequestHash minus provider/model/cache_control
- D22 Amendment 2 (ADR 0004) — soft triggers deferred to v1.x;
'soft' reserved in JSDoc but unreturnable
- CC 开发铁律 v1.6 § 10.x — Round-3 Cold Audit caught this
Reviewer (Iron Rule v1.6 § 10.x Mode A, fresh-context opus, independent
of drafter): APPROVE. Traced all 8 classifyTrigger branches against
ADR 0004 wording; verified 401/403/404/422 correctly classified as
'client_error' (never 'hard') per the ordering of CLIENT_ERROR_STATUSES
check before the generic status>=400 fallthrough; confirmed observation-
only via diff inspection (only logEvent object literals expanded, no
control-flow changes); independently ran git-stash to verify pre-D28
baseline 376 + 24 = 400.
3 non-blocking suggestions noted (JSDoc 'soft' union vs unreachable
return; future cross-correlation of chain_id with chatcmpl response IDs;
log-line redundancy between fallback_hop_error + fallback_hard_trigger
for same hop) — all cosmetic, not folded.
Note on D27 CI: D27 first CI run failed 1/376 on a Suite 17 port-
collision flake (overlapping random port ranges across Suite 17 tests +
Linux TIME_WAIT timing). Re-run was green. The flake is pre-existing
from D18-era test design; tracked as D29 follow-up to switch Suite 17
to OS-assigned port 0. D28 doesn't touch Suite 17 and is unaffected.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
||
|
taodengandClaude Opus 4.7
|
e10b7d7cb9 |
docs(adr-0004)+chore: D22 — defer soft triggers to v1.x (round-2 F2)
cold-audit catch from 2026-05-24
Round-2 cold-audit Finding 2 (P2 feature-surface vs data-ingestion drift).
ADR 0004 § Trigger taxonomy documented soft triggers (credit_pool_percent,
daily_request_count, five_hour_window_percent) as a live, configurable
feature category. But `evaluateSoftTriggers` in lib/fallback/engine.mjs
is functionally inert at v0.1:
- buildDefaultChain hardcodes quotaSnapshot: null on every hop
- No call site for provider.quotaStatus() in server.mjs or engine.mjs
(only definitions in the 3 provider plugins, all currently stubs)
- evaluateSoftTriggers correctly short-circuits to false on null snapshot
A user populating routing.soft_triggers in ~/.olp/config.json gets zero
runtime behavior. Round-1 cold audit + D5/D9 diff-review reviewers all
focused on evaluation correctness; nobody traced the production data
path end-to-end.
Owner decision (after considering wire-vs-defer): defer to v1.x. Wiring
quotaStatus() polling requires per-hop async I/O before each spawn
decision, error handling for providers without quota endpoints (all 3
current providers fall in this bucket: claude -p, codex exec --json,
vibe --prompt — none expose quota), a caching layer to avoid re-polling,
and a latency budget for a pre-spawn network call. Implementation cost
high; v0.1 value zero.
Strategy: defer the FEATURE (data ingestion path), not the CODE (evaluation
logic). evaluateSoftTriggers is small, well-tested via unit tests that
inject snapshots directly (test-features.mjs:3617-3665), and
architecturally correct. v1.x reactivation requires only wiring the
data path — evaluation stays untouched.
Changes (3 files, +25 / -1):
1. docs/adr/0004-fallback-engine.md +15 — new Amendment 2 block at top
of doc (after Amendment 1, before § Context, matching D11/D15/D16/
Amendment 1 placement convention):
- Finding (cold-audit round-2 F2 + 3 concrete code-state facts)
- Decision (defer; keep evaluation code inert-but-correct)
- Rationale (3-point cost/value analysis)
- Effect on § Trigger taxonomy (inline deferral note appended; design
prose preserved)
- What v1.x reactivation looks like (3 concrete steps; no rewrite needed)
- Procedural mechanism (CC 开发铁律 v1.6 § 10.x — round-2 caught it)
Plus an inline "📋 Deferred to v1.x (Amendment 2)" sub-bullet in
§ Trigger taxonomy → Soft triggers entry. The 3 threshold descriptions
are preserved verbatim — the architectural design remains the v1.x
intent.
2. lib/fallback/engine.mjs +8 / -1 — comment-only updates on the 2
`quotaSnapshot: null` lines in buildDefaultChain. Each now reads:
```
// quotaSnapshot stays null at v0.1 — soft triggers deferred to v1.x per
// ADR 0004 Amendment 2. evaluateSoftTriggers correctly short-circuits to
// false when quotaSnapshot is null. The polling path is not wired in v0.1.
```
No code logic changed. The previous misleading comment "populated at
runtime if provider.quotaStatus() is called" (which falsely implied a
live wiring) is replaced.
3. README.md +3 — two additions:
- Deferral callout immediately after the routing.soft_triggers config
example block, naming Amendment 2
- Implementation status table row: "Soft trigger data path
(quotaStatus() polling) | 📋 Planned (v1.x) | Evaluation logic
shipped + tested; data ingestion deferred per ADR 0004 Amendment 2"
Tests: 335/335 unchanged — pure docs + comment deferral, no behavior
change. Existing unit tests for evaluateSoftTriggers (which inject
snapshots directly) continue to validate the evaluation correctness
independent of the production ingestion path.
Authority:
- ADR 0004 self-amendment (Amendment 2 in-place)
- ALIGNMENT.md Rule 1 (Cite First) + CLAUDE.md § "Hard requirements"
item 1 — contract status changes require ADR amendment
- CC 开发铁律 v1.6 § 10.x — Round-2 Cold Audit caught this
Reviewer (Iron Rule v1.6 § 10.x Mode A, fresh-context opus, independent
of drafter): APPROVE. Verified all 3 framing claims independently:
(a) buildDefaultChain hardcodes null on both branches — confirmed at
engine.mjs:421 and :444; (b) zero quotaStatus() call sites in production
— `grep -rn "quotaStatus("` found only 3 definitions in provider plugins
+ JSDoc mentions; (c) evaluateSoftTriggers correctly short-circuits at
engine.mjs:139. Reactivation realism check: all 3 v1.x steps map to
existing surfaces (insertion points, hop shape, test fixtures).
Follow-up items (reviewer's non-blocking notes, NOT in this PR):
- README line 154 still reads "328-test suite"; current is 335 — pre-
existing doc-sync drift to pick up in D25 P3 batch
- v1.x ADR 0002 amendment may need to formally define authContext shape
(currently informal in the contract); pre-note as a dependency
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
||
|
taodengandClaude Opus 4.7
|
cb86807009 |
refactor(routing): D17 — alias-aware getProviderForModel as routing SPOT (Findings 12 + 13)
cold-audit catch from 2026-05-23
Cold-audit Findings 12 + 13 (both P3, both routing layer). F12: mistral
plugin's models[] included canonical IDs + aliases while anthropic/codex
were canonical-only — inconsistent routing surface (request "sonnet"
returned 503 from anthropic but request "devstral" worked for mistral).
F13: getProviderForModel imported but never called; buildDefaultChain
duplicated the lookup loop — two SPOT-candidate code paths.
Per cold-audit reviewer's option (a): standardize models[] on canonical-
only across all 3 plugins; getProviderForModel becomes alias-aware via
models-registry.json; buildDefaultChain uses getProviderForModel as SPOT.
Changes (4 files, +242 / -50):
1. lib/providers/index.mjs:
- At module load, builds `_aliasMap: Map<aliasString, {providerName,
canonicalModel}>` by walking models-registry.json's providers[*].aliases
- getProviderForModel signature extended: returns `{provider, name,
canonicalModel}` (was `{provider, name}`). The canonicalModel field is
the resolved canonical ID for callers that need to use it downstream
(cache key, observability headers, log events)
- 3-step resolution order: (1) alias map lookup → if hit AND provider
loaded → return with canonicalModel; (2) direct canonical scan → return
with canonicalModel = modelString; (3) null
- "Alias known but provider not loaded" case correctly falls through
to step 2 (which will also miss for an alias string) → returns null
2. lib/providers/mistral.mjs:
- _registryModels stripped to canonical-only: removed the
`...Object.keys(_registryEntry.aliases ?? {})` spread
- mistral.models[] now matches anthropic/codex shape (canonical IDs only)
- Comment block updated to point future readers at getProviderForModel
as the SPOT
3. lib/fallback/engine.mjs:
- buildDefaultChain's inline `for ([name, provider] of loadedProviders)`
scan loop replaced with a single getProviderForModel() call
- Chain hop's `model` field is now `match.canonicalModel` (was `modelString`)
— so downstream consumers receive canonical
- Explicit-chain config path unchanged (kept its pre-existing behavior;
alias resolution in routing.chains config is a known limitation, tracked
as a future improvement)
4. test-features.mjs:
- 17 new tests in `D17 — alias-aware getProviderForModel` describe block:
anthropic alias coverage (sonnet/opus/haiku/claude), openai aliases
(codex/codex-spark/gpt5/gpt5-mini), mistral aliases (devstral/
devstral-2/devstral-small/devstral-small-2), canonical pass-through,
unknown model → null, alias-to-disabled-provider → null,
buildDefaultChain integration with alias resolution
- 3 existing mistral tests rewritten — they were asserting the pre-D17
inconsistent shape (aliases in mistral.models[]). Now assert the
post-D17 invariant (aliases NOT in models[]; routing via
getProviderForModel instead)
Tests: 300 → 317 (+17 new). All pass on Node 20.
Pre-commit fold-in (per evidence-first checkpoint #4 — fold-ins themselves
need second-pass review):
- **D17 reviewer flagged C10**: original engine.mjs comment said
"downstream (cache key, X-OLP-Model-Used, provider.spawn) receives the
canonical ID rather than the alias string". The provider.spawn portion
is incorrect — spawn reads `irRequest.model` (the user's original input),
which is NEVER rewritten to canonical. Each provider CLI accepts its own
aliases natively (claude accepts sonnet/opus/haiku; vibe accepts
devstral/devstral-2; codex accepts its model IDs), so runtime behavior
is fine, but the comment overstated what D17 actually changes.
Folded in: corrected comment to honestly describe the canonical flow
(cache key + X-OLP-Model-Used + logs receive canonical; spawn continues
to receive irRequest.model). Same class of doc-code drift fix as D11's
B1 (false maxConcurrent enforcement claim) and D16's "bypasses
getOrCompute" drift — the discipline is maturing across D-days.
Authority:
- ADR 0002 § Provider contract (`models: string[]` is "models this provider
serves")
https://github.com/dtzp555-max/olp/blob/main/docs/adr/0002-plugin-architecture.md
- models-registry.json — single source of truth for (provider, model)
metadata + alias→canonical mappings per AGENTS.md SPOT policy
- AGENTS.md § Project-specific constraints — "models-registry.json is the
only place to add/edit (provider, model) metadata"
- CC 开发铁律 v1.6 § 10.x — Cold Audit Findings 12 + 13
Reviewer (Iron Rule v1.6 § 10.x Mode A, fresh-context opus, independent
of drafter): APPROVE_WITH_MINOR. Highest-value verification: walked
chain[0].model from buildDefaultChain through all downstream consumers
(computeCacheKey, X-OLP-Model-Used, log events) to confirm canonical
flow; then verified provider.spawn paths in anthropic.mjs:231 and
codex.mjs:248 read irRequest.model (user input), confirming the comment
overstatement (now fixed). Verified no alias-canonical collision exists
in current registry (12 aliases vs 10 canonical IDs, zero intersection).
Verified empty-registry edge case + provider-with-model-not-in-registry
edge case.
Follow-up items (reviewer's non-blocking observations, NOT in this PR):
- server.mjs:32 dead import of getProviderForModel — defer to D19 cleanup
- explicit-chain config path doesn't run alias resolution (routing.chains
in ~/.olp/config.json) — pre-existing limitation, file as future issue
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
||
|
taodengandClaude Opus 4.7
|
2cfd0b194e |
feat(phase-1): D10 P1 hardening — providers.enabled wiring + real streaming + spawn timeout
Folds in three production-blocking defects from external Codex review
round-3 of Phase 1 (D5+D6+D8+D9 already merged). Without these, OLP
returns 503 on every request even when `npm start` succeeds, streams
arrive as a single buffered burst after the spawn completes, and hung
CLIs block the engine forever despite ADR 0004 declaring spawn-timeout
a hard trigger.
P1.1 providers.enabled config wiring (ADR 0002 § Disable model):
- loadFallbackConfigSync() returns tri-field {chains, soft_triggers,
providersEnabled}; server.mjs reads _startupConfig.providersEnabled
and passes to loadProviders() at startup
- Empty / missing config → 0 enabled providers → 503 no_enabled_provider
(matches v0.1 0-Enabled posture per ALIGNMENT.md § Provider Inventory)
- __setProvidersEnabled / __resetProvidersEnabled test seams; in-place
Map mutation preserves existing direct-mutation patterns in Suite 13
P1.2 Real SSE streaming on single-hop cache-miss (ADR 0003 entry adapter
pattern, OpenAI /v1/chat/completions stream=true contract):
- New handleChatCompletions branch when ir.stream === true && chain.length
=== 1 && !bypassCache && !preCheckHit
- for await (const irChunk of provider.spawn(...)) writes SSE per chunk
via res.write(irChunkToOpenAISSE(...)); accumulates streamedChunks for
cacheStore.set on stop
- First-chunk rule preserved: error-before-first-chunk → sendError(502);
error-after-first-chunk → truncated res.end(), no fallback
- Multi-hop chains (chain.length > 1) continue to use buffered
executeWithFallback to keep fallback safety semantics
P1.3 Spawn timeout hard trigger (ADR 0004 § Trigger taxonomy bullet 4):
- SPAWN_TIMEOUT added to PROVIDER_ERROR_CODES (lib/providers/base.mjs)
and HARD_TRIGGER_CODES (lib/fallback/engine.mjs)
- All three plugins (anthropic.mjs / codex.mjs / mistral.mjs) wrap drain
loop with setTimeout (default 600_000ms, configurable via
hints.maxSpawnTimeMs); on fire: proc.kill('SIGTERM') + reject pending
drain promise with ProviderError(..., 'SPAWN_TIMEOUT')
- Timer cleared in finally; resolveNext/rejectNext atomically nulled in
push() + timer-fire path to prevent late-fire double-settle
Tests 277 → 288 (+11). Suite 14 (4 providers.enabled), Suite 15
(3 streaming cache-miss real-time, including arrival-count >= 2
assertion that architecturally proves real streaming), Suite 16
(4 spawn timeout, including 2-hop chain advancement from timed-out
primary). 288/288 pass on Node 20.20.2 + Node 25.8.0.
Authorities:
- ADR 0002 § Disable model — config toggle, not plugin-removal
https://github.com/dtzp555-max/olp/blob/main/docs/adr/0002-plugin-architecture.md
- ADR 0003 § Translation direction model — entry adapter for await pattern
https://github.com/dtzp555-max/olp/blob/main/docs/adr/0003-intermediate-representation.md
- ADR 0004 § Trigger taxonomy + § Fallback safety (first-chunk rule)
https://github.com/dtzp555-max/olp/blob/main/docs/adr/0004-fallback-engine.md
- OpenAI /v1/chat/completions stream=true (server-sent events,
data: {chunk} per delta, terminator data: [DONE])
https://platform.openai.com/docs/api-reference/chat/streaming
Reviewer (Iron Rule 10): fresh-context opus, independent of drafter.
Verdict: APPROVE_WITH_MINOR. Folded the one cheap minor before commit
(Suite 15a arrival-count assertion strengthened from chunks.length > 0
to arrivalTimestamps.length >= 2 — the prior assertion would have
admitted a buffered impl). Two remaining non-blocking notes deferred:
optional writeHead deferral (low value; single-hop guard makes pre-
content 200 + empty body safe), and version bump (Phase 1 ships as
v0.1.0 aggregate when D11–D16 land).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
||
|
taodengandClaude Opus 4.7 (noreply@anthropic.com)
|
5960486542 |
feat(phase-1): land fallback engine (D9)
Phase 1 Day 6 — the architectural milestone of Phase 1. OLP is no longer
a single-provider proxy with cache; it is now a multi-provider router
with idempotent-failure-safe fallback. ADR 0004 ratified in v0.1 governance
is fully implemented for D9 scope. Configuration-driven chains activate
when a user populates ~/.olp/config.json routing.chains; at v0.1 default
(empty chains), behaviour is single-hop pass-through identical to D5.
Files:
NEW: lib/fallback/engine.mjs (~470 lines) — Trigger taxonomy, chain
advancement, first-chunk safety, observability annotation.
MOD: server.mjs (+80 lines net) — wires executeWithFallback between IR
construction and provider.spawn. Per-hop cache key isolation
(ADR 0005 § Cross-provider fallback). Test seams added:
__setFallbackConfig, __resetFallbackConfig, __clearCache.
MOD: test-features.mjs (+845 lines, 6 new suites = +55 tests).
Authority citations:
ADR 0004 § Decision § Trigger taxonomy — Hard / Soft / Deterministic
(deferred) / Cost-aware (deferred) implemented exactly per spec.
ADR 0004 § Decision § Fallback safety — first-chunk rule satisfied at
D9 by buffering composition (executeHopFn = collectAllChunks; server
writes to res only after engine returns).
ADR 0004 § Decision § Chain advancement — one-at-a-time iteration;
originalError preserved from FIRST hop (not last) on exhaustion.
ADR 0004 § Decision § No fallback for client-side errors — HTTP 400/
401/403/404/422 stop the chain immediately; AUTH_MISSING also stops
immediately (user-config failure, not provider-quota).
ADR 0004 § Decision § Observability headers — X-OLP-Fallback-Hops set
from engine return value; X-OLP-Fallback-Exhausted lists tried
providers on exhaustion.
ADR 0005 § Cross-provider fallback cache behavior — each fallback hop
computes a fresh cache key with the hop's (provider, model) tuple;
primary's cache entries cannot leak to secondary's hop.
Reviewer chain (Iron Rule 10):
Implementer: sonnet (general-purpose).
Fresh-context reviewer: opus. Verdict APPROVE_WITH_MINOR.
Reviewer ran npm test (277/277 pass), read ADR 0004 + ADR 0005 end-
to-end, verified first-chunk-safety composition, checked all
trigger taxonomy mappings.
Reviewer non-blocking findings folded in this commit:
1. Test label at test-features.mjs:3756 clarified. Original title
"client error (400) → does NOT fall back" was misleading because
the test actually exercises both-fail SPAWN_FAILED → exhausted
path (400-no-fallback semantic is covered at unit level instead).
Renamed + commented to match actual behaviour.
2. triedProviders semantic (includes soft-skipped) documented in
engine.mjs comment near the soft-trigger branch.
3. SOFT_TRIGGER synthetic code documented in engine.mjs as engine-
internal, NOT a member of base.mjs PROVIDER_ERROR_CODES.
Reviewer non-blocking suggestions deferred (open D-later questions):
- Q1 cacheStatus on fallback hops: conservative 'miss', acceptable
per ADR 0005 § Per-model isolation.
- Q2 X-OLP-Fallback-Exhausted only when triedProviders > 1:
acceptable per ADR 0004 spec.
- Q3 soft-trigger quotaSnapshot always null at D9: acceptable per
ADR 0004 § Consequences/Negative graceful-degrade clause. Phase 2
will add quota poll-worker.
- Q4 loadFallbackConfigSync sync-only: acceptable for bounded
startup I/O.
Test count: 222 (D8) → 277 (D9). 6 new test suites cover trigger
taxonomy, engine chain advancement (including the load-bearing
originalError-from-FIRST-hop property), HTTP integration through
real server.mjs, soft trigger pre-fetch path, exhausted-chain
header emission.
Verification:
node --check on all touched files: clean.
npm test on Node 25.8.0: 277/277 pass in 266ms.
Hygiene grep: zero personal-name/path/token hits.
No new external npm deps.
Co-Authored-By: Claude Opus 4.7 (noreply@anthropic.com)
|