taodeng/olp
1
0
Fork 0
mirror of https://github.com/dtzp555-max/olp.git synced 2026-07-19 09:45:07 +00:00
Code Issues Packages Projects Releases Wiki Activity
111 Commits 6 Branches 14 Tags
main
Commit Graph
8 Commits
Author SHA1 Message Date
dtzp555-maxGitHubtaodengClaude Opus 4.7
 		1062e88e77 feat+test: D57 — cache layer streaming singleflight (ADR 0005 Amendment 8, issue #16) (#36) 
* feat+test: D57 — cache layer streaming singleflight (ADR 0005 Amendment 8, issue #16)

First of three D-days implementing the v1.x roadmap #1 streaming-path
singleflight. D57 lands the cache-layer coordination primitive only;
server.mjs wiring is D58, docs polish is D59.

## What

lib/cache/store.mjs — new method `getOrComputeStreaming(keyId, cacheKey,
sourceFactory, opts) → { stream, isFirst, role }`. Three outcomes per
Amendment 8 §1: cache_hit (no spawn), attached (joins existing inflight),
source (first caller, spawns via factory). Backed by `_streamingInflight`
Map keyed by `${keyId}\\0${cacheKey}` with synchronous check+insert per
Amendment 8 §1 + §6 atomicity invariant.

Internals (Amendment 8 §§2-10, §14):
- StreamingInflightEntry + AttachedClient typedefs
- Tee fan-out loop: single reader drains source, pushes to accumulatedChunks
  + every client's queue, fires per-client resolveNext promises
- Late-joiner replay buffer (synchronous drain on attach; reject with
  synthetic STREAM_BACKPRESSURE terminator if drain exceeds cap)
- Per-client backpressure (PER_CLIENT_QUEUE_CAP=1MB default, overridable)
- Replay buffer cap (ACCUMULATED_REPLAY_CAP=10MB default, overridable;
  cache write skipped if exceeded)
- AbortController propagation: when attachedClients.size === 0 after
  client iterator return(), source.return() + abort.signal fire
- D38 coordination via sourceFactory closure (factory wraps tryAcquireSpawn
  internally; cache layer just invokes it once)

lib/providers/base.mjs — `'STREAM_BACKPRESSURE'` added to
PROVIDER_ERROR_CODES per Amendment 8 §8. NOT in HARD_TRIGGER_CODES (engine
update lands in D58; whitelist-only map gives correct default).

test-features.mjs Suite 27 — 12 new tests (27a-27l) covering: solo stream,
2-concurrent dedup, mid-stream join + post-completion cache_hit, per-client
disconnect with other clients continuing, full disconnect → abort, source
error propagation, per-client backpressure, replay cap, TTL race during
inflight, sourceFactory throw, stats accuracy, composite key isolation.

## Scope

Strictly cache layer + base.mjs PROVIDER_ERROR_CODES entry. Untouched:
server.mjs, providers/{anthropic,codex,mistral}.mjs, fallback/engine.mjs,
IR, dashboard.html, README, CHANGELOG, package.json. D58 will wire server.

## Authority

- docs/adr/0005-cache-cross-provider.md Amendment 8 (2026-05-25, design
  ratified at D42, implementation gated on maintainer "go" — fired
  2026-05-25 post-v0.3.1)
- docs/v1x-roadmap.md #1 (streaming SF + TOCTOU close)
- GitHub issue #16 (round-6 cold-audit F13 sibling TOCTOU window)
- ADR 0002 Amendment 6 (D38 tryAcquireSpawn/releaseSpawn — invoked via
  sourceFactory closure at server layer, not directly by cache)

## Test count

603 → 615 (+12 D57 tests). Local: 615/615 pass.

## Iron Rule 11 (IDR)

D57 is the cache-layer minimum reviewable unit. D58 wires server.mjs +
adds X-OLP-Streaming-Inflight header + integration tests through HTTP
layer. D59 polishes README + closes issue #16.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix: D57 reviewer follow-ups — P2-2 (constant cleanup) + P2-3 (join-event deferral note)

Fold-in for D57 PR #36 fresh-context opus reviewer findings (APPROVE WITH
MINOR — 0 P0/P1, 3 P2). P2-1 is the D58 split (already planned); this
commit addresses P2-2 + P2-3.

P2-2 (cosmetic) — replace `Object.freeze({ value: X }).value` baroque
declaration of PER_CLIENT_QUEUE_CAP_DEFAULT + ACCUMULATED_REPLAY_CAP_DEFAULT
with a plain `export const X = 1*1024*1024`. The freeze-then-extract pattern
freezes a throwaway wrapper, which the `.value` immediately discards — does
nothing useful. Const declaration already gives binding immutability.

P2-3 (observability event parity deferral) — ADR 0005 Amendment 8 §11
lists `streaming_inflight_join` as one of four log events. The cache
layer cannot emit it correctly because provider/model identity lives in
the sourceFactory closure (server-layer concern). Added TODO note in
`_attachClient` pointing at D58 server wiring where the event will fire
on the consumer of `role: 'attached'`. The other three §11 events
(stream_backpressure_disconnect / streaming_inflight_source_done /
streaming_inflight_abort) ARE emitted from the cache layer with
{client_id, composite_key, ...} payloads; provider/model is enriched at
the server-side wrapper.

No test-surface change. 615/615 still pass.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: dtzp555 <dtzp555@gmail.com>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-25 20:35:03 +10:00
taodengandClaude Opus 4.7 b0c080db13 docs: D42 — streaming singleflight design ADR + v1.x safeguards (issue #16) 
ADR 0005 Amendment 6 (D34) deferred streaming-path D4 singleflight to
v1.x with the note "the design alone warrants a dedicated ADR." Round-6
cold-audit F13 (issue #16) raised the sibling TOCTOU window between
server.mjs's preCheckHit peek and the streaming-branch spawn. D42
fulfils Amendment 6's deferral note by ratifying the v1.x design as
ADR 0005 Amendment 8 — design only, no implementation.

**Design ratified (ADR 0005 Amendment 8, 14 sections):**

1. New `cacheStore.getOrComputeStreaming(keyId, cacheKey, sourceFactory)`
   API with `{ stream, isFirst }` return shape. Mirrors the buffered
   path's `getOrCompute` to keep the cache API surface coherent.
2. `StreamingInflightEntry` shape — source iterator + AbortController +
   accumulated-chunks replay buffer + attached-clients Set + state
   flags + D38 spawn-slot tracker.
3. `AttachedClient` shape — per-client tee buffer + byte-size meter +
   late-joiner replay flag + done/resolveNext/rejectNext for the
   single-reader-multi-writer tee.
4. Tee fan-out loop (one reader drains source, fans chunks to all
   attached clients).
5. Late-joiner replay policy — accumulated chunks burst-drained on
   attach.
6. Cache TTL race during inflight — late joiners see the inflight entry
   and join; expired cache slot is overwritten by inflight completion.
7. D38 maxConcurrent coordination — only first caller acquires; release
   fires once on source-complete/error/abort.
8. New `STREAM_BACKPRESSURE` error code. NOT a hard trigger. Affected
   client gets synthetic `{ type: 'stop', finish_reason: 'length' }` +
   `[DONE]` (matches D35 #10 truncation marker).
9. Mid-stream disconnect — remove from attached set; if 0 remaining,
   abort source via AbortController.
10. Replay buffer cap (10 MB, matches D23 cache-entry cap) — over cap
    marks entry not cacheable, late joiners get backpressure error.
11. Observability — 4 new log events
    (streaming_inflight_join / source_done / abort,
    stream_backpressure_disconnect) + new
    `X-OLP-Streaming-Inflight: source | attached | solo` header.
12. Server.mjs wiring — replaces the current peek+spawn pattern;
    TOCTOU window closes because Map check+insert is synchronous.
13. Test surface (when implementation lands) — 10 scenarios covering
    single-client / 2-concurrent / 3-concurrent / mid-stream join /
    first-disconnect / all-disconnect / source-error / backpressure /
    D38 coordination / TTL race / replay cap / X-OLP-* header values.
14. Defaults — PER_CLIENT_QUEUE_CAP=1MB, ACCUMULATED_REPLAY_CAP=10MB,
    STREAM_BACKPRESSURE not in HARD_TRIGGER_CODES.

**Multi-layer safeguards (the maintainer asked: "保证后面这一块会被处理而不会被忽略"):**

1. **`docs/v1x-roadmap.md` (NEW)** — single living landing page for
   every Phase-1 deferral. 7 items at D42:
   - #1 Streaming SF (this amendment)
   - #2 Multi-key auth (lib/keys.mjs)
   - #3 Soft trigger reactivation (ADR 0004 Amendment 2)
   - #4 /health activeSpawns integration (D38)
   - #5 Provider-level cacheKeyFields mask (ADR 0005 Amendment 7)
   - #6 Streaming-path SPAWN_FAILED salvage
   - #7 AUTH_MISSING tuple test coverage (D40 follow-up)
   Each entry names the ratifying ADR, load-bearing code anchor
   (file:line), GitHub issue (if any), concrete start trigger, and
   estimated effort. Maintainer's session-startup discipline grep
   this file at sprint kickoff.

2. **Issue #16 STAYS OPEN** — not closed in D42. Body updated post-
   commit to reference Amendment 8 with status "design ratified;
   implementation pending." Do not close until §13 test surface is
   green on actual implementation.

3. **`lib/cache/store.mjs#getOrCompute` JSDoc** — TODO comment for the
   sibling streaming API pointing at Amendment 8 + v1x-roadmap.md #1.

4. **`server.mjs` streaming-branch entry (~line 810)** — TODO comment
   block citing Amendment 8 + issue #16 + roadmap.md #1, naming the
   exact code lines the v1.x impl will replace.

5. **`README.md § Known limitations` section** — new subsection
   surfaces 4 limitations to users (streaming SF / soft triggers /
   multi-key auth / cacheKeyFields mask), each linking to the
   v1x-roadmap.md entry.

6. **Amendment 8 § "Cross-references and safeguards"** — explicit
   cross-link block enumerating the above 4 anchors so a future
   ADR-only reader knows every breadcrumb.

**Maintainer decision recorded:** Option 1 (design ADR only) chosen
over Option 2 (design + implementation now). Rationale: 200-400 lines
of concurrency primitives + 15-20 tests is not "pre-Phase-2 cleanup"
in shape — it is real v1.x feature work. Shipping streaming SF in a
v0.1.1 patch release would muddy the Phase 1 / Phase 2 contract that
v0.1.0 ratified. Personal/family-scale load makes the deferral safe
at v0.1.

Changes (6 files, +165 / -0):

- `docs/adr/0005-cache-cross-provider.md` — Amendment 8 prepended
  (133 lines).
- `docs/v1x-roadmap.md` — NEW file (148 lines).
- `lib/cache/store.mjs` — getOrCompute JSDoc gains TODO block (8 lines).
- `server.mjs` — streaming-branch entry gains TODO block (6 lines).
- `README.md` — Known limitations section (9 lines).
- `CHANGELOG.md` — D42 sub-entry under Unreleased (9 lines).

No code-behavior change. No new tests. No package.json bump
(phase_rolling_mode).

Authority:
- ADR 0005 Amendment 8 (this commit) — design ratification
- ADR 0005 Amendment 6 (D34) — original deferral with "design ADR
  needed" note that this commit fulfils
- GitHub issue #16 (round-6 F13) — sibling TOCTOU; STAYS OPEN
- ADR 0002 Amendment 6 (D38) — tryAcquireSpawn semantics
- ADR 0004 Amendment 5 (D40) — observability pattern extension
- CC 开发铁律 v1.6 § 10.x — design-only amendment; fresh-context
  reviewer not required per Iron Rule 10 implementation-phase scope
- CLAUDE.md release_kit_overlay phase_rolling_mode — under Unreleased

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-25 06:58:45 +10:00
taodengandClaude Opus 4.7 bdfea6884b feat+docs+test: D39 — D16 follow-ups (issue #3, 4 parts) 
D16 reviewer (commit `bafa6d1`) left 4 non-blocking suggestions
batched into issue #3 as a tracker. D39 closes all 4.

**Part 1 — CacheStore.delete(keyId, cacheKey) API** (lib/cache/store.mjs)

D16 originally evicted truncated entries via
`cacheStore.set(keyId, hopCacheKey, result, ttlMs=0)` — a TTL=0
tombstone purged lazily on next access. D39 introduces an explicit
delete primitive that removes the entry immediately.

- Synchronous: `delete(keyId, cacheKey) → boolean`. Returns true if
  the entry was present and removed, false if absent. Sync (not
  async) for the simplest in-memory Map contract — mirrors clear().
  Other CacheStore methods are async to leave room for a Phase 2
  file-backed adapter; delete being sync was a deliberate choice.
- Namespace cleanup: when the inner Map becomes empty after delete,
  the outer Map's per-keyId entry is also removed (memory hygiene;
  mirrors the _activeSpawns cleanup pattern from D38).
- Behavior: peek/get/getOrCompute see no trace after delete; the
  subsequent getOrCompute triggers a fresh compute.

**Part 2 — `cache_evicted_truncated` observability log** (server.mjs)

After the D16 eviction call in collectAllChunks, emit:
```js
logEvent('info', 'cache_evicted_truncated', {
  provider, model, cache_eviction_hit,
});
```

Dashboard sees salvage frequency per (provider, model). The
cache_eviction_hit boolean distinguishes "we evicted an entry" (true)
from "we tried to evict but it was already gone" (false — race with
concurrent eviction or TTL purge), preserving observability accuracy
under concurrency.

**Part 3 — Sticky-cache regression test** (test-features.mjs)

Defense-in-depth around the eviction path. Two consecutive identical
buffered requests; the first triggers SPAWN_FAILED after partial
chunks → Case B salvage returns `{ chunks..., finish_reason: 'length' }`
to the client and evicts via delete(). The second identical request
must trigger a fresh spawn (NOT serve the salvaged response from a
stale cache entry).

Asserts on BOTH invariants for defense-in-depth:
- Mock provider spawn count == 2 across the 2 identical requests
- Second request's X-OLP-Cache header is 'miss'

If eviction silently breaks in a future regression, both assertions
catch it independently.

**Part 4 — SPAWN_TIMEOUT salvage parity: DOCUMENT ASYMMETRY**
(docs/adr/0004-fallback-engine.md)

Maintainer decision: SPAWN_TIMEOUT is NOT salvaged. Document the
asymmetry rather than implementing parity. ADR 0004 Amendment 1 is
extended with a new section "Why SPAWN_TIMEOUT is excluded from
salvage" with 4-point rationale:

1. SPAWN_FAILED indicates the provider crashed mid-stream — there's
   nothing more coming; partial > nothing. Next-hop spawn has no
   advantage (same input may crash same way).
2. SPAWN_TIMEOUT indicates the provider was slow (deadline exceeded
   per `hints.maxSpawnTimeMs`). Fallback advancement to a DIFFERENT
   provider is more likely to give a complete response than salvaging
   a partial from a slow provider.
3. The "user paid for partial content" framing from D16 captures only
   SPAWN_FAILED. For SPAWN_TIMEOUT the user actually paid for "result
   within time T" — partial-at-time-T is not what was paid for;
   "full result soon after T" via fallback is closer.
4. Code-level inspection confirms the asymmetry: collectAllChunks
   catch matches ONLY `code === 'SPAWN_FAILED'` (server.mjs:563).
   SPAWN_TIMEOUT propagates via re-throw and hits evaluateHardTriggers.

v1.x re-evaluation trigger: if real usage shows users want partial-
on-timeout for very long deadlines, add a v1.x design ADR.

Stale comment fix: `lib/providers/anthropic.mjs:369` previously said
"SPAWN_TIMEOUT salvage parity is tracked in issue #3". D39 closes
that issue, so the comment is updated to point at ADR 0004 Amendment 1.

**Tests** (test-features.mjs): 447 → 452 (+5):
- 3 unit tests on CacheStore.delete (Suite 9): present-key true, absent-key
  false, namespace cleanup at empty
- 1 D16 integration test: cache_evicted_truncated log fires with
  correct fields during salvage
- 1 sticky-cache regression: spawn count 2 across 2 identical requests,
  X-OLP-Cache miss on second

Pre-commit fold-ins (per evidence-first checkpoint #4):

- **Reviewer Suggestion #1**: cacheStore.delete() return value was
  discarded at the call site → log inflated salvage metric under
  concurrent-eviction race. Folded: captured `evicted` boolean and
  added to log payload as `cache_eviction_hit`.
- **Reviewer Suggestion #2**: anthropic.mjs:369 stale comment
  pointing at now-closed issue #3. Folded: rewrote to point at
  ADR 0004 Amendment 1 § "Why SPAWN_TIMEOUT is excluded from
  salvage".
- **Reviewer Suggestion #3**: ADR 0004 attribution ambiguity —
  parenthetical "(per Amendment 3 — SPAWN_TIMEOUT is one of the 4
  live hard-trigger codes alongside SPAWN_FAILED, CLI_NOT_FOUND, and
  CONCURRENCY_LIMIT from Amendment 4)" could mis-parse as Amendment 3
  covering all four. Folded: split to
  "(per Amendment 3: SPAWN_FAILED, CLI_NOT_FOUND, SPAWN_TIMEOUT;
  per Amendment 4: CONCURRENCY_LIMIT)".

**CHANGELOG**: D39 sub-entry appended under the existing D38 entry
in Unreleased section. No package.json bump (phase_rolling_mode).

Authority:
- ADR 0005 § Cache layer — CacheStore API extension (Part 1)
- ADR 0004 Amendment 1 update — SPAWN_TIMEOUT asymmetry rationale (Part 4)
- GitHub issue #3 — closed by this commit
- D16 commit bafa6d1 non-blocking suggestions — batched here
- CC 开发铁律 v1.6 § 10.x — fresh-context opus reviewer independent
- CLAUDE.md release_kit_overlay phase_rolling_mode — under Unreleased

Reviewer (fresh-context opus, Iron Rule 10): APPROVE_WITH_MINOR.
Verified: delete() sync + call-site no-await correct; namespace
cleanup guarded on (had && ns.size === 0); SPAWN_TIMEOUT NOT in
salvage catch; ADR section 4-point rationale internally consistent
with code state; hygiene clean; 452/452 tests pass independently.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-24 21:13:57 +10:00
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>
 2026-05-24 19:33:48 +10:00
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>
 2026-05-24 16:40:30 +10:00
taodengandClaude Opus 4.7 7ef5510837 feat(cache): D23 — implement hints.cacheable + 10MB size cap (round-2 F3) 
cold-audit catch from 2026-05-24

Round-2 cold-audit Finding 3 (P2 cache correctness). ADR 0005 § "Cache
write conditions" items 3 and 4 were documented but never wired in
code:
- Item 3: "The provider's hints.cacheable flag is not false"
- Item 4: "The response is below a size cap (default 10 MB; configurable)"

Grep verified zero matches for `cacheable` / `10485760` / size-cap
patterns in lib/ or server.mjs pre-D23.

Changes (9 files, +391 / -12):

1. docs/adr/0002-plugin-architecture.md — Amendment 3 adds `cacheable`
   to the Provider contract hints list (after D11's Amendment 1 added
   maxSpawnTimeMs). Authority chain cites ADR 0005 § Cache write
   conditions item 3 as the field's origin.

2. docs/adr/0005-cache-cross-provider.md — Amendment 3 documents the
   D23 implementation of items 3 + 4 + the D16-interaction edge case
   (truncated > 10MB → no-op eviction, structurally bounded since
   responses > 10MB are anomalous by ADR's own rationale).

3. lib/providers/base.mjs — ProviderHints typedef gains
   `[cacheable]` (optional boolean); validateProvider rejects non-
   boolean non-undefined values. Omission accepted (default = true).

4. 3 plugins (anthropic / codex / mistral) each declare
   `cacheable: true` explicitly with citation comment.

5. lib/cache/store.mjs — CacheStore constructor accepts
   `maxEntryBytes` (default 10 * 1024 * 1024 = 10_485_760) +
   injectable `_warnFn`. `set()` computes
   `Buffer.byteLength(JSON.stringify(value))`; if exceeded, warns via
   `_warnFn` and returns undefined (no persistence). `getOrCompute`
   still returns the computed value to caller — cache write skipped
   but caller gets data; subsequent identical requests re-spawn.

6. server.mjs — 4 sites coordinated for cacheable opt-out:
   - `executeHopFn`: cacheable check before D13 shouldBypassCacheForHop
     (permanent provider policy precedes per-request bypass condition)
   - `cacheStore.peek` gate at line ~504: `cacheableForFirstHop`
     short-circuit
   - Real-streaming branch entry condition at line ~522:
     `cacheableForFirstHop` added (so cacheable: false + stream falls
     through to buffered path which honors the opt-out via executeHopFn)
   - Both `cacheStore.set` sites in streaming branch wrapped in
     `if (cacheableForFirstHop)` defensive guards (post-D23
     restructure these are unreachable for cacheable: false, but the
     guards make intent explicit and survive future refactors)

7. test-features.mjs — 13 new tests:
   - 5 validator tests (Suite 4): explicit true/false, omitted, string
     rejected, number rejected
   - 5 size-cap unit tests (Suite 9): default 10MB, custom override,
     oversize skip + warn capture, within-limit normal persistence,
     getOrCompute oversize returns-but-doesn't-cache + re-spawn
   - 3 cacheable integration tests (Suite 9e): non-streaming opt-out,
     streaming opt-out (the regression case that pre-fold-in failed),
     X-OLP-Cache header consistency on both paths

Tests: 335 → 348 (+13). All pass on Node 20.

Pre-commit fold-in (per evidence-first checkpoint #4):

- **D23 reviewer flagged 2 blocking issues**: (1) the cacheable opt-out
  in initial implementation was only in `executeHopFn` (buffered path);
  the D10 real-streaming branch in server.mjs bypassed the check
  entirely — calling streamPlugin.spawn() directly and writing to
  cacheStore.set() at 2 sites without consulting cacheable. (2) Suite
  9e integration tests didn't cover stream: true so the leak wasn't
  caught.

  Both diff-review and the implementer focused on `executeHopFn`
  because that's where the cold-audit reviewer pointed for Finding 3.
  Same class of "narrow attention" miss as several earlier D-days.

  Fold-in: compute `cacheableForFirstHop` once at request entry; add
  `!cacheableForFirstHop` short-circuit to peek gate; add
  `cacheableForFirstHop` to streaming-branch entry condition (forces
  fall-through to buffered path which has the opt-out); add defensive
  guards on both `cacheStore.set` call sites. Added a 3rd Suite 9e
  test covering stream: true + cacheable: false (which pre-fold-in
  would have failed by serving the second request from cache).

  This is now the FOURTH D-day where a doc-vs-code or path-coverage
  gap was caught by the reviewer rather than the implementer. The
  v1.6 § 10.x diff-review discipline continues to pay off.

Default behavior unchanged for 3 shipped plugins (all explicitly
`cacheable: true` → cache path identical to pre-D23).

Authority:
- ADR 0002 Amendment 3 (in-place) — establishes cacheable in contract
- ADR 0005 Amendment 3 (in-place) — documents implementation of items
  3 + 4
- ADR 0005 § Cache write conditions items 3 + 4 — the original
  authority for both rules
- CC 开发铁律 v1.6 § 10.x — Round-2 Cold Audit caught the missing
  implementation; diff-review Mode A caught the streaming-path gap

Reviewer (Iron Rule v1.6 § 10.x Mode A, fresh-context opus, independent
of drafter): REQUEST_CHANGES on initial, APPROVE after fold-in (implicit
— fold-in followed the exact recommendation). Verified:
- ADR amendment placement + structure
- Validator typedef + checks
- Size cap implementation in CacheStore + inflight slot release on
  oversize-skip
- All 4 interaction cases (cacheable × cache_control × D16
  × ordering) coherent post-fold-in
- 13 new tests including the regression test that would have failed
  on pre-fold-in code

Follow-up items (reviewer's non-blocking notes, NOT in this PR):
- ADR 0005 Amendment 3 could add one sentence on the prior-write-also-
  oversize case (file as docs polish)
- Consider extracting `shouldUseCacheForHop(hopProvider, ir)` helper
  combining D13 + D23 logic — reduces miss-risk for next reviewer
- Test 30 could add `assert.equal(store._inflight.size, 0)` as
  inflight-slot leak regression guard

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-24 14:45:08 +10:00
taodengandClaude Opus 4.7 8ae77c3ae3 fix(cache)+docs(adr-0005): D15 — expand cache key composition to include max_tokens/top_p/stop/tool_choice 
cold-audit catch from 2026-05-23

Cold-audit Finding 7 (P2 cache correctness). ADR 0005 § Cache key
composition (v1.0) listed 7 fields. ADR 0003 § Optional fields added
`max_tokens`, `top_p`, `stop`, `tool_choice` as IR-carried fields
that affect model output. Pre-D15 cache key omitted those 4 —
identical IRs differing only in those fields collided on the same
cache key but produced legitimately different outputs. Concrete
hazard: a request with `max_tokens: 100` could receive a cached
response generated by `max_tokens: 4000` — wrong content (truncated
or unexpectedly extended).

Coordinated change across two layers, single commit per the ADR-with-
code pattern established by D11:

1. docs/adr/0005-cache-cross-provider.md — Amendment 2 (top of doc,
   matching D11 Amendment 1 placement convention)
   - Documents the 4 missing fields + concrete failure mode
   - Expands the v1.0 cache key composition spec
   - Documents the `?? null` collapsing semantics (explicit-null
     equals absent — consistent with existing temperature/response_format)
   - Adds forward-looking note: future IR field additions must be
     evaluated for cache-key inclusion at addition time; default is
     include unless explicit rationale documents safe omission

2. lib/cache/keys.mjs — append the 4 fields to `keyObj` after the
   existing 7 (preserves field ordering; pre-D15 cache entries are
   forced misses on first request — schema forward-compatible)
   - `max_tokens: ir.max_tokens ?? null`
   - `top_p: ir.top_p ?? null`
   - `stop: ir.stop ?? null`
   - `tool_choice: ir.tool_choice ?? null`
   - Updated file-level docblock + function-level JSDoc + @param ir
     enumeration to reflect new schema (the @param fix also closes a
     pre-existing partial-list issue that omitted cache_control)

3. test-features.mjs — 5 new tests in Suite 9:
   - max_tokens differs → different key
   - top_p differs → different key
   - stop differs → different key
   - tool_choice differs → different key (covers string-form
     `'auto'` vs `'none'`; object-form coverage left for future
     defense-in-depth — slot existence is verified by the string case)
   - Both-absent stability check (`?? null` collapsing produces
     identical keys for omitted-vs-omitted)

Tests: 292 → 297 (+5). No hash constants pinned in existing tests
(grep verified) so no pre-D15 tests required updating; assertions
were already shape-based (`assert.equal` / `assert.notEqual` on
hash strings, never specific hex values).

Authority:
- ADR 0003 § Optional fields — source of the 4 IR field definitions
  https://github.com/dtzp555-max/olp/blob/main/docs/adr/0003-intermediate-representation.md
- ADR 0005's stated invariant: "different output → different cache
  entry" — the addition restores compliance with this invariant
- OpenAI /v1/chat/completions spec — confirms the 4 fields affect
  output:
  https://platform.openai.com/docs/api-reference/chat/create
- ALIGNMENT.md Rule 2(c) spirit — ADR amendment + code change land
  in same merge (D11 precedent established this pattern for
  Provider-contract changes; D15 applies same pattern for cache-key
  schema changes)
- CC 开发铁律 v1.6 § 10.x — Cold Audit caught this; diff-review pass
  that approved original ADR 0005 did not cross-reference all ADR
  0003 optional fields

Reviewer (Iron Rule v1.6 § 10.x Mode A, fresh-context opus, independent
of drafter): APPROVE_WITH_MINOR. Folded the one minor (`@param ir`
JSDoc stale partial list) before commit — same JSDoc precision logic
that drove D11's B1 fold-in. Two remaining non-blocking suggestions
(tool_choice object-form test coverage; D11-vs-D15 amendment structure
template) tracked as defense-in-depth opportunities, not required for
spec compliance.

Reviewer's highest-value verification: field ordering preserved.
keyObj at lib/cache/keys.mjs:203-217 reads exactly:
`{provider, model, messages, tools, temperature, response_format,
cache_control, max_tokens, top_p, stop, tool_choice}` — existing 7
in unchanged positions, 4 new appended at end. JSON.stringify on
Node 18+ preserves insertion order; SHA-256 hash deterministic
across runs.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-24 11:24:12 +10:00
taodengandClaude Opus 4.7 (noreply@anthropic.com) 8dd02e77ac feat(phase-1): land cache layer (D1+D4) + Anthropic E2E gate (D5) 
Phase 1 Day 3. ADR 0005 cache layer ships: content-hash key over (provider,
model, IR), D1 per-key isolation, D4 singleflight. server.mjs wires cache
into the dispatch path with X-OLP-Cache: hit|miss|bypass header. Suite 10
adds a gated real Anthropic spawn E2E (OLP_RUN_E2E=1) — orchestrator ran
it once on Mac mini: 7.2s wall-clock, real claude-haiku-4-5 spawn via
keychain OAuth, returned "OK", asserted X-OLP-Provider-Used: anthropic +
X-OLP-Cache: miss on first request. The plugin chain (IR to claude -p to
IR to OpenAI) now works end-to-end on a real binary.

Files:
  NEW:  lib/cache/keys.mjs (199 lines) — computeCacheKey + cache_control
        extraction. sha256(stable-JSON(provider, model, normalized messages,
        tools, temperature, response_format, cache_control_markers)).
  NEW:  lib/cache/store.mjs (348 lines, includes peek() fold-in) — in-memory
        CacheStore with D1 per-key isolation (nested Maps), D4 singleflight
        via getOrCompute(keyId, cacheKey, computeFn), TTL expiry,
        injectable _nowFn for deterministic tests, stats reporting,
        scoped clear(keyId?).
  MOD:  server.mjs (+136/-26 lines) — cache lookup before spawn, bypass on
        cache_control markers, getOrCompute for D4 singleflight, header
        annotation. authContext changed from {} to null so providers
        correctly fall back to readAuthArtifact.
  MOD:  test-features.mjs (+614 lines, 30 new Suite 9 tests + 1 gated
        Suite 10 E2E).

Test count: 98 → 128 (+30) at default. With OLP_RUN_E2E=1: 129/129
(verified by orchestrator on Mac mini, Node 25.8.0).

Authority citations:
  Cache key composition: ADR 0005 § Cache key composition (v1.0). All 7
    spec fields present (provider, model, normalized messages, tools,
    temperature, response_format, cache_control markers).
  Per-key isolation D1: ADR 0005 § D1 + OCP keys.mjs precedent (nested
    Map keyId → cacheKey → entry).
  Singleflight D4: ADR 0005 § D4 + OCP server.mjs inflight Promise
    precedent. getOrCompute synchronously registers the inflight promise
    BEFORE any await, so concurrent callers cannot observe an empty
    inflight slot — JavaScript event-loop guarantee on this is the
    correctness anchor.
  cache_control bypass D2 basic structure: ADR 0005 § D2. Full marker-
    strip-for-non-Anthropic-provider behaviour deferred (only anthropic
    plugin exists at D4, so the marker-strip branch is unreachable).
  Chunked stream replay D3 basic structure: ADR 0005 § D3. Full timing-
    accurate replay deferred per orchestrator spec; D5 stores collected
    chunks and replays sequentially without timing fidelity.

Architectural decisions:
  1. In-memory cache at D5. ADR 0005 § Cache directory structure shows
     ~/.olp/cache/<keyId>/... as the eventual layout; D5 ships the
     structural equivalent (nested Maps) in memory. File backing lands
     in a later Phase. The Map structure is identical to the eventual
     filesystem layout; migration is a serializer/deserializer pair.
  2. keyId = '__anonymous__' at D5. Per-OLP-API-key namespacing infra
     lands in Phase 2 multi-key. The constant is hardcoded in
     server.mjs with a comment explaining the Phase 2 transition.
  3. authContext changed from {} to null. {} ?? readAuthArtifact()
     never falls back (empty object is truthy under ??); null
     correctly triggers the fallback. Confirmed by D5 E2E test which
     used the keychain OAuth path end-to-end.
  4. cache_control side-channel via raw body. The IR translator
     (lib/ir/openai-to-ir.mjs) strips cache_control because it is not
     an IR v1.0 field. server.mjs bypass check uses both hasCacheControl
     (ir) and extractCacheControlMarkers(body?.messages) to compensate.
     The proper fix is an ADR 0003 amendment to preserve cache_control
     in IR; tracked as a Phase 2 backlog item. Suite 9 Test 30 verifies
     the dual-check works end-to-end over HTTP.
  5. collectAllChunks throws ProviderError on type:error chunks. This
     prevents cache_store.set() from being called on error-terminated
     responses per ADR 0005 § Cache write conditions item 1. Anthropic
     plugin currently never emits type:error chunks (throws instead),
     so this is defensive code for future provider plugins.

Reviewer chain (Iron Rule 10):
  Implementer: sonnet (general-purpose).
  Fresh-context reviewer: opus (ecc:code-reviewer). Verdict
    APPROVE_WITH_MINOR.
  Reviewer ran npm test (128/128 pass with Suite 10 skipped), opened
    ADR 0005 end-to-end, opened OCP keys.mjs + server.mjs to verify
    singleflight precedent, verified the singleflight invariant by code
    inspection (5-concurrent Test 23 plus event-loop guarantee proof).

Reviewer non-blocking findings folded in this commit:
  1. peek() added to CacheStore — stats-neutral existence check.
     server.mjs preCheck now uses peek() instead of has(), fixing the
     hit/miss counter double-count bug. Documentation on has() updated
     to point callers at peek() for stats-sensitive paths.
  2. collectAllChunks now throws ProviderError on type:error chunks
     instead of returning an error-terminated array, preventing cache
     pollution per ADR 0005 § Cache write conditions item 1.
  3. Cache key composition comment clarifies that cache_control slot is
     forward-compat infrastructure for the future ADR 0003 amendment;
     v1.0 IR strips cache_control so the slot is always null at the
     key-composition site, with the D2 bypass side-channeling through
     the raw body in server.mjs.

Reviewer findings deferred:
  - IR amendment to preserve cache_control as first-class IR field
    (would let server.mjs drop the dual-check). Tracked as Phase 2
    backlog: "amend ADR 0003 to preserve cache_control markers in IR".
  - LRU-by-linked-list eviction at maxEntriesPerKey scale. Current
    O(n log n) sort-on-evict is fine at personal/family scale.
    Tracked for revisit if cap is raised significantly.

Verification:
  node --check on all touched files: clean.
  npm test on Node 25.8.0: 128/128 pass in 210ms (default mode).
  OLP_RUN_E2E=1 npm test on Mac mini: 129/129 pass in 7.4s (real
    claude-haiku-4-5 spawn, "OK" response, all OLP headers correct).
  hygiene grep: no personal names, no /Users literal paths, no real
    OAuth tokens (test fixtures use "<fake-token>" placeholders).

Co-Authored-By: Claude Opus 4.7 (noreply@anthropic.com)
 2026-05-23 18:48:37 +10:00