From aced7046cdee638d848e63896ff0ee3e81c4accb Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 17 Aug 2026 21:59:34 +0900 Subject: [PATCH 1/3] docs(devlog): record what WP4 discovery found, and why it stops there The audit could not find a caller of the remember API and reasonably concluded the emit-before-commit defect might not be live. It is: src/bridge.ts calls it three times and discards the durable promise with void, then emits the item. The reason this does not land in the same cycle is structural rather than effort. closeCurrentToolCall is a synchronous closure writing into a ReadableStream controller, so there is no await to hang the durability on - awaiting the write means an async close path or a pre-emit barrier, which is a change to the streaming core. AGENTS.md gates that behind the full suite and it sits beside the subagent-fallback synchrony invariant, so it deserves its own work-phase and a maintainer's call on sequencing. Filed as #1926 with both halves, the restart-stability constraint that kills the naive key widening, the OAuth/key/local credential split, the version bump requirement, and the exact caller lines. --- .../040_thought_signature_scope.md | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) 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..47ec0cbfb5 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,33 @@ 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. From ebab9d253c9b9ac1154647facb6ed5b53b68c389 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 17 Aug 2026 22:08:08 +0900 Subject: [PATCH 2/3] fix(responses): scope durable thought signatures to the upstream destination The durable replay key used thread, provider name, adapter and model, while the sibling in-memory cache used those plus destination and credential identity. So one provider NAME serving two endpoints - a gateway and a direct endpoint under one config entry - shared opaque signatures between them, and a signature minted by one upstream is meaningless to the other. The reason the durable store left those fields out is real and worth keeping in view: the sibling's identities run through an HMAC keyed by randomBytes minted at module load. Reusing them here would change every key on restart, so the store would stop matching anything while still appearing to work - a worse failure than the over-broad key, because nothing announces it. Destination does not share that constraint. It is a configured endpoint rather than a secret, so a plain digest of the same normalized URL is equally non-reversible for this purpose and needs no persisted salt or new on-disk state. That is durableReplayDestinationIdentity, and it sits beside the process-local one rather than replacing it, because the in-memory cache is right to prefer the random-keyed form. Credential scope is deliberately NOT included here. OAuth has a restart-stable discriminator in accountId and generation, key auth would need a persisted-salt digest of secret material, and Codex pool auth rides a rotating bearer - three different answers that belong with the emit-before-commit work in #1926 rather than smuggled in behind a destination fix. The store version is now read on load, not just written. It was written as 2 and never checked, so a key-shape change could not be announced: old entries simply went dead and aged out on TTL, which is silent and looks exactly like a store that is not working. v3 drops them explicitly instead. Ablation: removing the destination component from the key fails the new cross-endpoint test and leaves the restart test green, which is the pair that matters - the fix must isolate endpoints without breaking restart replay. --- src/responses/reasoning-replay-cache.ts | 17 +++++++- src/responses/thought-signature-replay.ts | 18 ++++++++- src/server/responses/core.ts | 2 + src/types.ts | 5 +++ ...google-signature-history-roundtrip.test.ts | 39 ++++++++++++++++++- 5 files changed, 78 insertions(+), 3 deletions(-) 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); + }); }); From ed74f2b9a5b2cedae41145706d7aab2294208a2a Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 17 Aug 2026 22:09:42 +0900 Subject: [PATCH 3/3] docs(devlog): correct the WP4 record after the audit The reviewer was right that deferring the whole key-scope fix was too much. The restart-stability blocker binds to the credential component alone, and this document had already sanctioned scoping the fix to destination only - then deferred the branch it pre-authorized. Also records the four errors the audit found in what I wrote: a third call site in failCurrentToolCall, an inverted claim about local providers that already never remember, a version mechanism that was written but never read, and a missing fourth auth mode. Plus the correction to my own deferral reasoning - the streaming sites are already inside an async loop, so the real blocker is buildResponseJSON rather than the synchronous closure I named. --- .../040_thought_signature_scope.md | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) 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 47ec0cbfb5..a1e81e9e0c 100644 --- a/devlog/_plan/260817_wave5_execution/040_thought_signature_scope.md +++ b/devlog/_plan/260817_wave5_execution/040_thought_signature_scope.md @@ -133,3 +133,47 @@ requirement, and the exact caller locations. Terminal outcome for this cycle: 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.