From 68e50da68a11691daf7c9ac37c53218d00308765 Mon Sep 17 00:00:00 2001 From: dtzp555-max Date: Mon, 25 May 2026 16:32:59 +1000 Subject: [PATCH] =?UTF-8?q?fix+test+docs:=20D53=20=E2=80=94=20tried=5Fprov?= =?UTF-8?q?iders=20schema=20semantic=20fix=20(D45=20P2=20deferral=20closed?= =?UTF-8?q?)=20(#30)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sixth Phase 3 D-day. Small focused fix for the D45 fresh-context opus reviewer P2 finding that was deferred: auditCtx.tried_providers on the key_no_provider_access 403 path was being stamped with the ORIGINAL chain (which was filtered out, never dispatched), distorting downstream audit queries like "which providers did key X actually call". CHANGES: - server.mjs handleChatCompletions ~L815: on key_no_provider_access 403, auditCtx.tried_providers = [] (was _originalChainProviders). The configured-but-blocked chain still appears in the human- readable error message body — the audit just doesn't claim those providers were "tried" when the server's filter dispatched zero. - docs/adr/0007-multi-key-auth.md § 8 amendment: new paragraph spelling out the tried_providers semantic. "The list of providers the server actually dispatched a spawn against. A provider that was configured in the chain but filtered out by providers_enabled gating is NOT included — the key didn't try the provider, the gate did. On the 403 path tried_providers is the empty array." Plus a forward note that audit log rotation moved to Phase 3 / ADR 0008 § 5. - test-features.mjs Suite 20h-extra-audit (+1 test — 600 → 601): creates guest key with providers_enabled: ['mistral']; fires request for Anthropic-routed model; asserts 403 key_no_provider_access; reads audit row from audit.ndjson; asserts tried_providers === []. Pins the D53 semantic against regression. - CHANGELOG.md: D53 entry under Unreleased. NOT IN D53: - E2E + docs polish (D54) - Phase 3 close → v0.3.0 (D55; maintainer-triggered) Test count: 600 → 601 (+1). Verified locally via npm test. AUTHORITY: - ADR 0007 § 8 amendment (D53, 2026-05-25). - D45 fresh-context opus reviewer P2 deferral note. - CLAUDE.md release_kit overlay phase_rolling_mode — under Unreleased. - Standing autopilot grant. ALIGNMENT.md scope check: small entry-surface change (audit context field assignment on one error path) + ADR amendment + new test. Per ALIGNMENT.md Rule 1 the ADR amendment is the authority citation for the server change. No provider plugin / IR / models-registry change. Co-authored-by: dtzp555 Co-authored-by: Claude Opus 4.7 --- CHANGELOG.md | 11 +++++++++++ docs/adr/0007-multi-key-auth.md | 4 +++- server.mjs | 10 +++++++++- test-features.mjs | 29 +++++++++++++++++++++++++++++ 4 files changed, 52 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ec53b4..b07ea88 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,17 @@ All notable changes to OLP land here. Per `CLAUDE.md` release_kit overlay, this ## Unreleased +### D53 — `tried_providers` schema semantic fix (D45 P2 deferral closed) + +Sixth Phase 3 D-day. Small focused fix for the D45 fresh-context opus reviewer P2 finding that was deferred: `auditCtx.tried_providers` on the `key_no_provider_access` 403 path was being stamped with the ORIGINAL chain (which was filtered out, never dispatched), distorting downstream audit queries like "which providers did key X actually call". + +- **`server.mjs` 403 path fix** (around L815): `auditCtx.tried_providers = []` (was `_originalChainProviders`). The configured-but-blocked chain still appears in the human-readable error message body — the audit just doesn't claim those providers were "tried" when the server's filter dispatched zero. +- **ADR 0007 § 8 amendment**: new paragraph spelling out the `tried_providers` semantic — "the list of providers the server actually dispatched a spawn against. A provider that was configured in the chain but filtered out by `providers_enabled` gating is NOT included — the key didn't try the provider, the gate did. On the 403 path `tried_providers` is the empty array." Plus a forward note that audit log rotation moved to Phase 3 / ADR 0008 § 5. +- **Suite 20h-extra-audit (+1 test — 600 → 601):** creates a guest key with `providers_enabled: ['mistral']`; fires a request for an Anthropic-routed model; asserts 403 `key_no_provider_access`; reads the audit row from `audit.ndjson`; asserts `tried_providers === []`. This pins the D53 semantic against regression — if a future change reverts to stamping the original chain, the test fails. +- **Documentation:** CHANGELOG D53 entry; ADR 0007 § 8 amendment. +- **Test count:** 600 → 601 (+1 D53 regression test). +- **Authority:** ADR 0007 § 8 amendment (D53, 2026-05-25); D45 fresh-context opus reviewer P2 deferral note; CLAUDE.md `release_kit overlay phase_rolling_mode` — under Unreleased; standing autopilot grant. + ### D52 — Daily audit rotation (`lib/audit.mjs` extension + `bin/olp-audit-rotate.mjs`) Fifth Phase 3 D-day. Adds daily UTC-aware rotation to `lib/audit.mjs` per ADR 0008 § 5 + ships an external cron tool. Rotation is **synchronous** at v0.3.0 (Lane 3 = B daily rotation; synchronous design eliminates the race that an async wrapper would create between date-change-detection and the append). diff --git a/docs/adr/0007-multi-key-auth.md b/docs/adr/0007-multi-key-auth.md index fd2641a..60020ad 100644 --- a/docs/adr/0007-multi-key-auth.md +++ b/docs/adr/0007-multi-key-auth.md @@ -263,7 +263,9 @@ Field origin: **No PII.** Audit deliberately captures NO request body, NO response body, NO IR-message content. Hash + shape only. This is a personal/family deployment property; do not relax without a separate ADR amendment. -**Rotation.** Phase 2 does NOT rotate `audit.ndjson`. Rotation policy lands in Phase 3 alongside Dashboard / audit query work (§ 12). +**`tried_providers` semantics (clarification, D53 / 2026-05-25).** The field captures the list of providers the server **actually dispatched a spawn against** for this request. A provider that was configured in the chain but filtered out by `providers_enabled` gating (resulting in 403 `key_no_provider_access`) is NOT included — the key didn't try the provider, the gate did. On the 403 path `tried_providers` is the empty array. The configured-but-blocked chain providers appear in the human-readable error message returned to the client but are intentionally NOT surfaced in the audit event, so downstream queries like "which providers did key X actually call" stay accurate. This semantic was implicit in the D45 implementation (where the field was set to the original chain on 403, misrepresenting "tried"); D53 corrects the implementation + amends this section to spell out the intent. + +**Rotation.** Phase 2 does NOT rotate `audit.ndjson`. Rotation policy ships in Phase 3 — daily rotation via `lib/audit.mjs` `_maybeRotateAudit` synchronous trigger on first append after UTC date change + optional `bin/olp-audit-rotate.mjs` external cron. See ADR 0008 § 5. --- diff --git a/server.mjs b/server.mjs index a6c34fd..304fff9 100644 --- a/server.mjs +++ b/server.mjs @@ -813,7 +813,15 @@ async function handleChatCompletions(req, res) { chain = chain.filter(hop => isProviderEnabled(olpIdentity, hop.provider)); if (chain.length === 0) { auditCtx.error_code = 'key_no_provider_access'; - auditCtx.tried_providers = _originalChainProviders; + // D53 (D45 P2 deferral fix): tried_providers semantic per ADR 0007 § 8 is + // "providers the server actually dispatched a spawn against". On 403 the + // filtered chain is empty — no provider was dispatched, so the audit + // value is []. The configured-but-blocked chain providers go into the + // human-readable error message + a separate _diagnostic field we don't + // surface in audit (would distort downstream queries like "which + // providers did key X actually call"). ADR 0007 § 8 amended in this + // D-day's CHANGELOG entry to spell the semantic. + auditCtx.tried_providers = []; const allowed = olpIdentity.providers_enabled === '*' ? '*' : (olpIdentity.providers_enabled ?? []).join(', ') || '(none)'; return sendError( res, 403, diff --git a/test-features.mjs b/test-features.mjs index 030d7fa..5f61140 100644 --- a/test-features.mjs +++ b/test-features.mjs @@ -10146,6 +10146,35 @@ describe('Suite 20 — server.mjs auth integration (D45, ADR 0007)', () => { }); assert.equal(r.status, 200); }); + + it('20h-extra-audit: D53 — key_no_provider_access 403 audit row has tried_providers = []', async () => { + // Per ADR 0007 § 8 tried_providers semantics clarification: the field + // captures providers the server actually dispatched. On 403 (filter + // rejected the whole chain) the server dispatched zero providers, so + // tried_providers MUST be []. The configured-but-blocked chain only + // appears in the human-readable error message, not in the audit. + // D45 P2 deferral fix — was previously stamping the original chain + // which distorted "which providers did key X actually call" queries. + const { id, plaintext_token } = createKey({ name: '20h-extra-audit', owner_tier: 'guest', providers_enabled: ['mistral'], olpHome: TMP }); + const r = await fetch({ + port, method: 'POST', path: '/v1/chat/completions', + headers: { Authorization: `Bearer ${plaintext_token}` }, + body: { model: 'claude-sonnet-4-6', messages: [{ role: 'user', content: '20h-extra-audit' }] }, + }); + assert.equal(r.status, 403); + assert.equal(JSON.parse(r.body).error.type, 'key_no_provider_access'); + // Wait for the res.on('finish') audit append + await new Promise(resolve => setTimeout(resolve, 25)); + const auditPath = _pathJoinForSetup(TMP, 'logs', 'audit.ndjson'); + const lines = fsReadFileSync(auditPath, 'utf-8').trim().split('\n').filter(Boolean); + const row = lines.map(l => JSON.parse(l)).find(r => + r.path === '/v1/chat/completions' && r.status_code === 403 && r.key_id === id, + ); + assert.ok(row, '403 audit row must be present'); + assert.equal(row.error_code, 'key_no_provider_access'); + assert.deepEqual(row.tried_providers, [], + 'tried_providers MUST be [] on key_no_provider_access — D53 semantic fix per ADR 0007 § 8 clarification'); + }); }); // ── 20i: per-key cache namespace isolation (criterion #1) ────────────────