-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(responses): keep the whole conversation when a continuation replay misses #4683
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| import { effectiveAdapterContract, getAdapterDefinition, type AdapterWire } from "../adapters/registry"; | ||
|
|
||
| /** | ||
| * Wires whose upstream holds the conversation itself, so a turn may reference history this | ||
| * process no longer has. | ||
| * | ||
| * The set is empty, and that is the finding rather than an oversight. The three wires that look | ||
| * like they belong here do not: | ||
| * | ||
| * - devin sends `mapOcxMessagesToDevin(parsed)` — the whole conversation — on every turn | ||
| * (`src/adapters/devin.ts`). Its session/thread id buys prompt caching, not remembered context. | ||
| * - cursor continues from `_providerContinuation.cursor.checkpointRef`, which is read out of the | ||
| * very store that just expired; without it `resolveCursorCheckpoint` returns a reason and the | ||
| * request falls back to `continuationMode: "full-replay"` over `parsed.context.messages` | ||
| * (`src/adapters/cursor/request-builder.ts`). | ||
| * - kiro builds `conversationState.history` from the parsed turns it was given | ||
| * (`src/adapters/kiro/payload.ts`); a conversation id alone reconstructs nothing. | ||
| * | ||
| * So for every translated wire a replay miss means the delta travels alone. Only the native | ||
| * Responses passthrough, which forwards `previous_response_id` untouched to a backend that stored | ||
| * the chain, can answer a turn whose history this process lost. | ||
| */ | ||
| export const PROVIDER_OWNED_CONTINUATION_WIRES: ReadonlySet<AdapterWire> = new Set<AdapterWire>(); | ||
|
|
||
| /** The wire an adapter id resolves to through contract inheritance, or undefined if unknown. */ | ||
| export function resolvedAdapterWire(adapterId: unknown): AdapterWire | undefined { | ||
| if (typeof adapterId !== "string" || !getAdapterDefinition(adapterId)) return undefined; | ||
| return effectiveAdapterContract(adapterId).wire; | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -331,7 +331,20 @@ no new destination-based migration. The existing stateless pass sets `store: fal | |||||||
| stored continuation parameters, and repairs orphan calls/results without claiming execution | ||||||||
| success. A local replay-cache hit supplies history; a miss cannot reconstruct it, so callers | ||||||||
| receive `previous_response_not_found` before upstream dispatch and must resend complete history | ||||||||
| without `previous_response_id`. Routed custom-tool lowering requires the same recovery when a delta | ||||||||
| without `previous_response_id`. That refusal is not specific to the stateless flag: it covers every | ||||||||
| destination that cannot see the prefix this process failed to restore, which is every destination | ||||||||
| except the native Responses passthrough. The passthrough forwards the id and keeps its | ||||||||
| upstream-owned state. `PROVIDER_OWNED_CONTINUATION_WIRES` in | ||||||||
| `src/responses/continuation-ownership.ts` is deliberately empty and records why the three | ||||||||
| candidates do not qualify: devin re-sends the whole conversation each turn, cursor reads its | ||||||||
| `checkpointRef` out of the same expired store and otherwise falls back to `full-replay`, and kiro | ||||||||
| rebuilds `conversationState.history` from the turns it was handed. A missed expansion on any of | ||||||||
| them would forward the current turn alone under a normal 200 — the whole conversation replaced by | ||||||||
| one line, with nothing in the response saying so. This also replaces kiro's former | ||||||||
| `invalid_request_error`, which told the client to start a new session and therefore skipped the | ||||||||
| recovery Codex performs on `previous_response_not_found`. Retention is the other half: local | ||||||||
| continuation state is held for `RESPONSE_TTL_MS` (24 hours), long enough that an ordinary idle gap | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Qualify the 24-hour retention statement.
Proposed documentation correction-continuation state is held for `RESPONSE_TTL_MS` (24 hours), long enough that an ordinary idle gap
+continuation state is eligible for retention for up to `RESPONSE_TTL_MS` (24 hours), subject to
+storage limits and eviction. When the state remains available, an ordinary idle gap📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||
| resumes by expansion rather than by asking the client to replay. Routed custom-tool lowering requires the same recovery when a delta | ||||||||
| custom result has no local call, because its original wire type cannot be established and guessing it | ||||||||
| would send an unmatched result upstream. The check resolves the selected wire protocol and the | ||||||||
| request's own tool declarations after final route selection, so stateful destinations keep their | ||||||||
|
|
||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Describe replay retention as a maximum age in both guides.
RESPONSE_TTL_MSallows retention for 24 hours, butpruneResponsesandenforceSpilledResponseBudget()can evict state earlier when entry, memory, or disk ceilings apply. The current English wording, “retained for 24 hours,” and Korean wording, “24시간 보존하며,” can imply a guaranteed 24-hour retention period.At
docs-site/src/content/docs/guides/codex-integration.md:368-369, state that replayed state is retained for up to 24 hours and may be evicted earlier by the memory, disk, or entry ceilings. Apply the equivalent wording atdocs-site/src/content/docs/ko/guides/codex-integration.md:209-210.🤖 Prompt for AI Agents