From 8ae77c3ae319106e7fefcdced01c0928dfe871af Mon Sep 17 00:00:00 2001 From: dtzp555 Date: Sun, 24 May 2026 11:24:12 +1000 Subject: [PATCH] =?UTF-8?q?fix(cache)+docs(adr-0005):=20D15=20=E2=80=94=20?= =?UTF-8?q?expand=20cache=20key=20composition=20to=20include=20max=5Ftoken?= =?UTF-8?q?s/top=5Fp/stop/tool=5Fchoice?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/adr/0005-cache-cross-provider.md | 17 +++++++++- lib/cache/keys.mjs | 20 +++++++++--- test-features.mjs | 47 +++++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 5 deletions(-) diff --git a/docs/adr/0005-cache-cross-provider.md b/docs/adr/0005-cache-cross-provider.md index 02c351f..a9cfc08 100644 --- a/docs/adr/0005-cache-cross-provider.md +++ b/docs/adr/0005-cache-cross-provider.md @@ -5,6 +5,16 @@ - **Authors:** project maintainer (with AI drafting assistance) - **Related:** OLP v0.1 spec §4.4; ADR 0002 (plugin architecture); ADR 0003 (IR — cache keys are computed over IR shape); ADR 0004 (fallback — cross-provider cache misses are correct on fallback) +## Amendments + +### Amendment 2 — 2026-05-24: Expand cache key composition to include `max_tokens`, `top_p`, `stop`, `tool_choice` (D15) + +- **Finding:** Cold-audit Finding 7 (P2 cache correctness) — the v1.0 cache key composition listed in § "Cache key composition (v1.0)" omitted four IR fields that materially affect model output: `max_tokens` (output length truncation), `top_p` (sampling distribution), `stop` (stop sequences that terminate generation), and `tool_choice` (`'auto' | 'none' | 'required' | {type, function:{name}}` — fundamentally changes whether/which tool the model calls). Two requests identical except for one of these four fields produce different model outputs but collided on the same cache key under v1.0. Consequence: a request with `max_tokens: 100` could receive a cached response originally generated by a `max_tokens: 4000` request — wrong content (truncated or unexpectedly extended). +- **Change:** Expand cache key composition to include `max_tokens`, `top_p`, `stop`, and `tool_choice` in addition to the existing seven fields. The four new fields are appended after the existing set so that the ordering of existing fields is stable (pre-D15 cache entries are invalidated on first request — a forced miss — but the schema is forward-compatible). Field serialization follows the existing `?? null` null-coalescing pattern: a request with `max_tokens: undefined` serializes as `null` and is treated as equivalent to an absent field; `max_tokens: 100` serializes as `100`. This is consistent with how `temperature` and `response_format` are handled. +- **Rationale:** ADR 0005's own stated invariant — "different output → different cache entry" — requires that any IR field affecting output be included in the cache key. The original v1.0 key composition was correct for the seven named fields but provided incomplete coverage of ADR 0003's IR field set. The four omitted fields are all defined in ADR 0003 § Optional fields and are all sourced directly from the OpenAI `/v1/chat/completions` spec parameters that affect generation. +- **Forward-looking note:** Future IR field additions (e.g., `seed`, `frequency_penalty`, `presence_penalty`, `logit_bias`, `logprobs`, `n`) must be evaluated for cache-key inclusion at the time they are added to the IR. The default is to **include** the field in the cache key unless an explicit rationale documents why omitting it is safe (e.g., the field has no effect on the provider's output for any value, or it is a purely client-side metadata field). This evaluation must appear in the amending ADR per ALIGNMENT.md Rule 2(c) spirit. +- **Procedural mechanism:** CC 开发铁律 v1.6 § 10.x (Cold Audit caught it). This follows the calibration-loop precedent established by D11 / Amendment 1 of ADR 0002: the diff-review pass that approved the original ADR 0005 did not cross-reference all ADR 0003 optional fields; the cold-audit pass on 2026-05-23 caught the gap as Finding 7. + ## Context OCP shipped a four-layer cache hardening (D1+D2+D3+D4) in v3.13.0 (2026-05-07 per the MEMORY.md entry). The four layers: @@ -31,7 +41,7 @@ The third design point: `cache_control` (D2) is Anthropic-specific in v1.0. Othe Per spec §4.4, OLP's cache layer: -**Cache key composition (v1.0).** +**Cache key composition (v1.0).** (amended 2026-05-24, see Amendment 2) ``` key = sha256(JSON.stringify({ provider, // e.g., 'anthropic', 'openai', 'mistral' @@ -41,6 +51,11 @@ key = sha256(JSON.stringify({ temperature, // included if non-default response_format, // included if present cache_control, // Anthropic prompt-caching markers (the markers themselves; the bypass logic is separate) + // Added by Amendment 2 (D15) — fields from ADR 0003 § Optional fields that affect output: + max_tokens, // output length truncation; null if absent + top_p, // sampling distribution; null if absent + stop, // stop sequences; null if absent + tool_choice, // 'auto' | 'none' | 'required' | {type, function:{name}}; null if absent })) ``` diff --git a/lib/cache/keys.mjs b/lib/cache/keys.mjs index 297cfc1..d115973 100644 --- a/lib/cache/keys.mjs +++ b/lib/cache/keys.mjs @@ -1,7 +1,7 @@ /** * lib/cache/keys.mjs — Cache key generation for OLP * - * Authority: ADR 0005 § Cache key composition (v1.0) + * Authority: ADR 0005 § Cache key composition (v1.0) — amended 2026-05-24 by Amendment 2 (D15) * * Cache keys are computed over the IR (ADR 0003), not over the raw OpenAI * request shape, so key stability is governed by OLP's own release cadence. @@ -15,6 +15,11 @@ * temperature: ir.temperature ?? null, * response_format: ir.response_format ?? null, * cache_control: extractCacheControlMarkers(ir.messages), + * // Added by D15 (Amendment 2) — ADR 0003 § Optional fields that affect output: + * max_tokens: ir.max_tokens ?? null, + * top_p: ir.top_p ?? null, + * stop: ir.stop ?? null, + * tool_choice: ir.tool_choice ?? null, * })) * * Per ADR 0005 § Per-model isolation: (provider, model) is part of the key. @@ -162,9 +167,10 @@ export function hasCacheControl(ir) { /** * Computes a content-addressed SHA-256 cache key over the IR request. * - * Per ADR 0005 § Cache key composition (v1.0): + * Per ADR 0005 § Cache key composition (v1.0) — amended 2026-05-24 by Amendment 2 (D15): * key = sha256(JSON.stringify({ - * provider, model, messages, tools, temperature, response_format, cache_control + * provider, model, messages, tools, temperature, response_format, cache_control, + * max_tokens, top_p, stop, tool_choice * })) * * The key is deterministic: same inputs → same key. No random, no timestamp. @@ -174,7 +180,7 @@ export function hasCacheControl(ir) { * * @param {string} provider — provider name, e.g. 'anthropic' * @param {string} model — model string, e.g. 'claude-haiku-4-5' - * @param {object} ir — IR request (ADR 0003): { messages, tools?, temperature?, response_format? } + * @param {object} ir — IR request (ADR 0003): { messages, tools?, temperature?, response_format?, cache_control?, max_tokens?, top_p?, stop?, tool_choice? } * @param {object} [_options] — reserved for future options (not used at D5) * @returns {string} — 64-character hex SHA-256 digest */ @@ -202,6 +208,12 @@ export function computeCacheKey(provider, model, ir, _options = {}) { temperature: ir.temperature ?? null, response_format: ir.response_format ?? null, cache_control: cacheControlMarkers.length > 0 ? cacheControlMarkers : null, + // D15 (ADR 0005 Amendment 2): IR fields from ADR 0003 § Optional fields that affect output. + // Appended after the existing seven fields to avoid invalidating ordering-dependent keys. + max_tokens: ir.max_tokens ?? null, + top_p: ir.top_p ?? null, + stop: ir.stop ?? null, + tool_choice: ir.tool_choice ?? null, }; return createHash('sha256').update(JSON.stringify(keyObj)).digest('hex'); diff --git a/test-features.mjs b/test-features.mjs index 020ed90..a5c193c 100644 --- a/test-features.mjs +++ b/test-features.mjs @@ -1206,6 +1206,53 @@ describe('Cache layer — computeCacheKey (Suite 9)', () => { const k2 = computeCacheKey('anthropic', 'claude-haiku-4-5', ir2); assert.equal(k1, k2); }); + + // ── D15 Tests: Amendment 2 — max_tokens, top_p, stop, tool_choice ──── + + // ── Test 9: max_tokens distinguishes ───────────────────────────────── + it('computeCacheKey differs when max_tokens differs (D15 Amendment 2)', () => { + const ir1 = makeIR({ messages: [{ role: 'user', content: 'x' }], max_tokens: 100 }); + const ir2 = makeIR({ messages: [{ role: 'user', content: 'x' }], max_tokens: 4000 }); + const k1 = computeCacheKey('anthropic', 'claude-haiku-4-5', ir1); + const k2 = computeCacheKey('anthropic', 'claude-haiku-4-5', ir2); + assert.notEqual(k1, k2); + }); + + // ── Test 10: top_p distinguishes ────────────────────────────────────── + it('computeCacheKey differs when top_p differs (D15 Amendment 2)', () => { + const ir1 = makeIR({ messages: [{ role: 'user', content: 'x' }], top_p: 0.5 }); + const ir2 = makeIR({ messages: [{ role: 'user', content: 'x' }], top_p: 0.9 }); + const k1 = computeCacheKey('anthropic', 'claude-haiku-4-5', ir1); + const k2 = computeCacheKey('anthropic', 'claude-haiku-4-5', ir2); + assert.notEqual(k1, k2); + }); + + // ── Test 11: stop distinguishes ─────────────────────────────────────── + it('computeCacheKey differs when stop sequences differ (D15 Amendment 2)', () => { + const ir1 = makeIR({ messages: [{ role: 'user', content: 'x' }], stop: ['\n'] }); + const ir2 = makeIR({ messages: [{ role: 'user', content: 'x' }], stop: ['END', '\n'] }); + const k1 = computeCacheKey('anthropic', 'claude-haiku-4-5', ir1); + const k2 = computeCacheKey('anthropic', 'claude-haiku-4-5', ir2); + assert.notEqual(k1, k2); + }); + + // ── Test 12: tool_choice distinguishes ─────────────────────────────── + it('computeCacheKey differs when tool_choice differs (D15 Amendment 2)', () => { + const ir1 = makeIR({ messages: [{ role: 'user', content: 'x' }], tool_choice: 'auto' }); + const ir2 = makeIR({ messages: [{ role: 'user', content: 'x' }], tool_choice: 'none' }); + const k1 = computeCacheKey('anthropic', 'claude-haiku-4-5', ir1); + const k2 = computeCacheKey('anthropic', 'claude-haiku-4-5', ir2); + assert.notEqual(k1, k2); + }); + + // ── Test 13: undefined max_tokens stable (both absent → same key) ──── + it('computeCacheKey is identical when max_tokens absent in both requests (D15 Amendment 2)', () => { + const ir1 = makeIR({ messages: [{ role: 'user', content: 'x' }] }); + const ir2 = makeIR({ messages: [{ role: 'user', content: 'x' }] }); + const k1 = computeCacheKey('anthropic', 'claude-haiku-4-5', ir1); + const k2 = computeCacheKey('anthropic', 'claude-haiku-4-5', ir2); + assert.equal(k1, k2); + }); }); describe('Cache layer — extractCacheControlMarkers + hasCacheControl (Suite 9 cont.)', () => {