diff --git a/devlog/_plan/260817_wave5_execution/040_thought_signature_scope.md b/devlog/_plan/260817_wave5_execution/040_thought_signature_scope.md index 47990c37f7..a1e81e9e0c 100644 --- a/devlog/_plan/260817_wave5_execution/040_thought_signature_scope.md +++ b/devlog/_plan/260817_wave5_execution/040_thought_signature_scope.md @@ -103,3 +103,77 @@ implementation, not left to the guard's default. File the issue with these conditions, implement, close citing the merge SHA and the isolation + restart test output. If the discovery step shows no live caller, the issue records that finding instead of claiming a fix. +## Outcome (executed) — discovery done, implementation deferred with reasons + +**The caller-discovery step resolved the open question, and the answer changes the +shape of the work.** The round-1 audit could find no `src/` caller of the remember +API and concluded the emit-before-commit defect might not be live. It is live. The +callers are in `src/bridge.ts`, and they discard the durable promise explicitly: + +```ts +void rememberExtraContentForReplay(currentToolCall.callId, currentToolCall.providerMetadata, replayCacheScope); +...(rememberAndSerializeExtraContent(...).extra ?? {}), // durable dropped +emit("response.output_item.done", { output_index: currentToolCall.outputIndex, item }); +``` + +in the streaming close path (freeform and function-call branches) and again on the +non-streaming `pushOutput` path. + +**Why this is not a one-line fix.** `closeCurrentToolCall` is a *synchronous* closure +writing into a `ReadableStream` controller. There is no `await` at that point, so +"await durability before emit" requires either an async close path or a pre-emit +barrier — a change to the streaming core, which `AGENTS.md` gates behind the full +suite and which sits next to the subagent-fallback synchrony invariant documented in +the repository root. That is its own work-phase, not a rider on a scope fix. + +**Filed as #1926** with both halves, the restart-stability constraint, the +OAuth/key/local credential-identity split, the `version: 2` → `version: 3` migration +requirement, and the exact caller locations. Terminal outcome for this cycle: +**NEEDS_HUMAN on sequencing** — the fix is well-specified and the constraint that +blocked the naive version is written down, but landing it means touching the +streaming emit path, which deserves a maintainer's call on scheduling rather than an +agent slipping it into a wave. +## Amendment after the WP4 audit — the destination half landed + +The reviewer pushed back on deferring the whole key-scope fix, and was right. The +restart-stability blocker binds to the **credential** component only: +`provider.baseUrl` is configuration, stable across restarts by construction, and the +only reason today's `providerDestinationIdentity` is unstable is that it runs through +the random-keyed HMAC. A plain digest of the same normalized URL is equally +non-reversible here. This document already sanctioned that fallback — *"scope the fix +to destination only"* — and then deferred the branch it had pre-authorized. + +Landed in `ebab9d253`: + +- `durableReplayDestinationIdentity()` beside the process-local form (not replacing it — + the in-memory cache is right to prefer the random-keyed version). +- `providerDestinationDurableIdentity` threaded through the identity type and `core.ts`. +- The durable `keyFor` includes it, closing cross-endpoint collisions under one provider name. +- `load()` reads the store version for the first time; `STORE_VERSION = 3` drops v2 + entries explicitly instead of letting them go dead and age out invisibly. + +Ablation: removing the destination component fails the new cross-endpoint test while the +restart test stays green — the pair that matters, since the naive fix would have passed +the first and broken the second. + +### Corrections to this document and to #1926 + +The audit found four errors in what I wrote, all corrected on the issue rather than +edited away: + +1. **A third call site.** `failCurrentToolCall` discards the durable promise too, at two + more sites, reached from six places including the stall watchdog. The writeup named + only `closeCurrentToolCall` and `flushToolCall`. +2. **The `local` claim was backwards.** Those providers already never remember — `core.ts` + only binds `scope.current` when credential *and* destination exist. There was no + regression to protect against. +3. **The version mechanism was mis-stated.** `version: 2` was written and never read, so a + bump would have invalidated nothing. Fixed by actually reading it. +4. **A fourth auth mode.** Codex pool auth rides a rotating bearer — the worst + restart-stability story of the four, and absent from the table. + +And one correction to the deferral reasoning itself: the streaming call sites are already +inside an `async` loop, so the obstacle there is the reentrancy guard, not the absence of +an await point. The real blocker is `buildResponseJSON`, a synchronous public export with +three callers. Awaiting a disk write per tool call would also put fsync latency on the hot +path, which argues for a turn-end barrier rather than per-item awaits. diff --git a/src/responses/reasoning-replay-cache.ts b/src/responses/reasoning-replay-cache.ts index f09930475c..4d2e49698f 100644 --- a/src/responses/reasoning-replay-cache.ts +++ b/src/responses/reasoning-replay-cache.ts @@ -19,7 +19,7 @@ * long-lived proxy cannot grow without limit. */ -import { createHmac, randomBytes } from "node:crypto"; +import { createHash, createHmac, randomBytes } from "node:crypto"; import type { OcxProviderConfig, OcxReasoningReplayIdentity, @@ -108,6 +108,21 @@ export function reasoningReplayDestinationIdentity(baseUrl: string | undefined): return `destination:${processLocalIdentity("destination", canonical)}`; } +/** + * The same destination identity, but stable across restarts. + * + * The process-local form above is keyed by `randomBytes(32)` minted at module load, which + * is correct for an in-memory cache and fatal for a durable one: every key would change on + * restart and the store would silently stop matching anything. A plain digest of the same + * canonical URL is equally non-reversible for this purpose — the input is a configured + * endpoint, not a secret — and needs no persisted salt or new on-disk state. + */ +export function durableReplayDestinationIdentity(baseUrl: string | undefined): string | undefined { + if (!nonEmpty(baseUrl)) return undefined; + const canonical = baseUrl.trim().replace(/\/+$/, ""); + return `destination:${createHash("sha256").update("destination\0").update(canonical).digest("hex")}`; +} + /** Produce a non-reversible process-local identity for credential material. */ export function reasoningReplayCredentialIdentity( kind: "key" | "oauth" | "codex", diff --git a/src/responses/thought-signature-replay.ts b/src/responses/thought-signature-replay.ts index 421305cde4..b4ffbab837 100644 --- a/src/responses/thought-signature-replay.ts +++ b/src/responses/thought-signature-replay.ts @@ -27,6 +27,11 @@ import type { OcxProviderOpaqueToolCallMetadata, OcxReasoningReplayScopeRef } fr import { isCarryableSignature, responsesExtraContentFromProviderMetadata } from "./provider-opaque-metadata"; const STORE_FILE_NAME = "thought-signature-replay.json"; +/** + * Bumped whenever `keyFor` changes shape. v3 added the durable destination identity, so a + * v2 file's keys can never match and are dropped on load instead of aging out invisibly. + */ +const STORE_VERSION = 3; /** Bound on remembered entries; real signatures are a few hundred bytes, so this stays small. */ const MAX_ENTRIES = 16_384; @@ -83,6 +88,11 @@ function keyFor(callId: string, scope: OcxReasoningReplayScopeRef | undefined): return JSON.stringify([ scope.clientThreadId, identity.providerName, + // Destination, unlike the credential identity, has a restart-stable form: it is a + // configured endpoint rather than a secret, so a plain digest works where the + // reasoning cache's randomBytes-keyed HMAC cannot. Without it, one provider NAME + // serving two endpoints shares signatures across both. + identity.providerDestinationDurableIdentity ?? "destination:unknown", identity.adapterName, identity.modelId, callId, @@ -103,6 +113,12 @@ function load(): void { if (typeof parsed !== "object" || parsed === null || !Array.isArray((parsed as { entries?: unknown }).entries)) { return; } + // The version was written but never read, so a key-shape change could not be + // announced — old entries simply went dead and aged out on TTL, which is silent and + // indistinguishable from a store that is not working. Reading it makes a shape change + // an explicit drop: entries keyed by an older scheme are discarded on load rather than + // lingering as permanent misses. + if ((parsed as { version?: unknown }).version !== STORE_VERSION) return; const nowMs = Date.now(); for (const entry of (parsed as { entries: unknown[] }).entries) { if (typeof entry !== "object" || entry === null) continue; @@ -141,7 +157,7 @@ function persist(): Promise { persistChain = persistChain .then(async () => { const snapshot = JSON.stringify({ - version: 2, + version: STORE_VERSION, entries: [...entries].map(([key, entry]) => ({ key, sig: entry.sig, savedAt: entry.savedAt })), }); await atomicWriteFileAsync(storePath(), snapshot); diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index 2df5160984..a5ad0191fc 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -14,6 +14,7 @@ import { bindReasoningReplayScope, reasoningReplayCodexCredentialIdentity, reasoningReplayDestinationIdentity, + durableReplayDestinationIdentity, reasoningReplayKeyCredentialIdentity, reasoningReplayOAuthCredentialIdentity, } from "../../responses/reasoning-replay-cache"; @@ -336,6 +337,7 @@ function bindRouteReasoningReplayScope(args: { ? { providerName, providerDestinationIdentity, + providerDestinationDurableIdentity: durableReplayDestinationIdentity(provider.baseUrl), adapterName, modelId: parsed.modelId, credentialIdentity, diff --git a/src/types.ts b/src/types.ts index 24c8faa6aa..8ae883d23d 100644 --- a/src/types.ts +++ b/src/types.ts @@ -5,6 +5,11 @@ export interface OcxReasoningReplayIdentity { providerName: string; /** Opaque process-local digest of the exact upstream destination. */ providerDestinationIdentity: string; + /** + * The same destination, digested WITHOUT the process-local random key, so it can key a + * durable store. Absent when no base URL was resolvable. + */ + providerDestinationDurableIdentity?: string; adapterName: string; modelId: string; /** Opaque process-local credential identity; never a raw token or API key. */ diff --git a/tests/google-signature-history-roundtrip.test.ts b/tests/google-signature-history-roundtrip.test.ts index 049ea50f36..d7bf8ecf0d 100644 --- a/tests/google-signature-history-roundtrip.test.ts +++ b/tests/google-signature-history-roundtrip.test.ts @@ -15,6 +15,7 @@ import { rememberThoughtSignatureForReplay, resetThoughtSignatureReplayForTests, } from "../src/responses/thought-signature-replay"; +import { durableReplayDestinationIdentity } from "../src/responses/reasoning-replay-cache"; import type { AdapterEvent, OcxParsedRequest, OcxProviderConfig } from "../src/types"; import { withTestTranslatorBudget } from "./helpers/translator-budget"; @@ -38,12 +39,18 @@ const provider = { * client-visible call_id is not unique across threads, accounts, providers or models, * so keying on it alone let one conversation's signature reach another's turn. */ -function scopeFor(threadId = "thread-a", modelId = MODEL, providerName = "google") { +function scopeFor( + threadId = "thread-a", + modelId = MODEL, + providerName = "google", + destination = "https://generativelanguage.googleapis.com", +) { return { clientThreadId: threadId, current: { providerName, providerDestinationIdentity: `dest-${providerName}`, + providerDestinationDurableIdentity: durableReplayDestinationIdentity(destination), adapterName: "google", modelId, credentialIdentity: `cred-${providerName}`, @@ -298,4 +305,34 @@ describe("#1735 thought signature survives history replay", () => { resetThoughtSignatureReplayForTests(); expect(lookupReplayThoughtSignature("call_disk_1", scopeFor())).toBe(SIGNATURE); }); + + test("one provider name serving two endpoints does not share signatures", () => { + // The gap the durable key closes. providerName, adapterName, modelId and thread can all + // be identical across two upstreams — a gateway and a direct endpoint under one config + // name — and an opaque signature minted by one is meaningless to the other. + const primary = scopeFor("thread-a", MODEL, "google", "https://generativelanguage.googleapis.com"); + const secondary = scopeFor("thread-a", MODEL, "google", "https://gateway.internal.example/v1beta"); + + rememberThoughtSignatureForReplay("call_dest", SIGNATURE, primary); + + expect(lookupReplayThoughtSignature("call_dest", primary)).toBe(SIGNATURE); + expect(lookupReplayThoughtSignature("call_dest", secondary)).toBeUndefined(); + }); + + test("the durable destination identity is stable across restarts, unlike the process-local one", async () => { + // The reason this is a separate digest rather than the sibling cache's HMAC: that one is + // keyed by randomBytes minted at module load, so reusing it here would change every key + // on restart and the store would silently stop matching — a worse failure than the + // over-broad key it replaced, because it looks like it is working. + const url = "https://generativelanguage.googleapis.com"; + expect(durableReplayDestinationIdentity(url)).toBe(durableReplayDestinationIdentity(url)); + expect(durableReplayDestinationIdentity(url)).not.toBe(durableReplayDestinationIdentity("https://other.example")); + // Trailing-slash normalization matches the process-local form. + expect(durableReplayDestinationIdentity(`${url}/`)).toBe(durableReplayDestinationIdentity(url)); + + rememberThoughtSignatureForReplay("call_dest_restart", SIGNATURE, scopeFor()); + await flushThoughtSignatureReplayForTests(); + resetThoughtSignatureReplayForTests(); + expect(lookupReplayThoughtSignature("call_dest_restart", scopeFor())).toBe(SIGNATURE); + }); });