fix(responses): scope durable thought signatures to the upstream destination - #1929
Merged
Conversation
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.
…ination 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.
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.
Contributor
|
✅ Deterministic PR hygiene checks passed. |
Contributor
✅ READY
|
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lidge-jun
marked this pull request as ready for review
August 18, 2026 00:04
This was referenced Aug 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the cross-endpoint half of #1926.
The durable replay key used thread, provider name, adapter and model, while the sibling
in-memory cache (
src/responses/reasoning-replay-cache.ts) used those plus destinationand 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.
Why the durable store left those fields out is a real constraint, not an oversight.
The sibling's identities run through an HMAC keyed by
randomBytes(32)minted at moduleload. 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.
durableReplayDestinationIdentitysits beside the process-local form rather than replacing it — the in-memory cache is
right to prefer the random-keyed version.
Credential scope is deliberately not included. OAuth has a restart-stable discriminator
in
accountId+generation, key auth would need a persisted-salt digest of secretmaterial, 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.
Cross-account and rotation collisions therefore remain open, and they are the more serious
ones.
The store version is now read, not just written. It was written as
2and neverchecked, so a key-shape change could not be announced: old entries simply went dead against
the new keys and aged out on TTL — silent, and indistinguishable from a store that is not
working.
STORE_VERSION = 3drops them explicitly on load.Verification
bun test --isolate tests— 12755 pass, 10 skip, 0 fail, 159251 expect() calls across 823 files (505s).bun run typecheck— passed.google-signature-history-roundtrip,reasoning-replay-identity,reasoning-replay-scope-source,google-antigravity-replay— 80 pass, 0 fail.keyForfails the new cross-endpoint test while the restart test stays green. That pair is the point — the naive fix (reusing the process-local HMAC) would have passed the first and broken the second.Checklist
dev