fix(client-core): preserve conversation identity across history reseeds - #483
fix(client-core): preserve conversation identity across history reseeds#483Zerlight wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR tightens conversation reconciliation in client-core to preserve stable conversation identity across history reseeds by restricting prompt/rewind aliasing to provenance-checked cases, and by making “fresh session” provenance dependent on uninterrupted all-event delivery.
Changes:
- Add an explicit transport capability flag for “transparent reconnect” and reject such transports in
LinkCodeClientto keep event continuity provable. - Refine conversation-store reseed merging to avoid ambiguous content-based prompt aliasing, preserve duplicate prompts/branch cursors, and keep ephemeral/uncovered events.
- Add/expand regression integration tests for reseeds, reconnect boundaries, duplicate prompts, branch cursors, rewinds, and provenance continuity.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/foundation/transport/src/ws.ts | Exposes whether the WebSocket transport can reconnect without emitting onClose. |
| packages/foundation/transport/src/transport.ts | Extends Transport with optional reconnectsTransparently metadata for continuity-sensitive consumers. |
| packages/client/core/src/client.ts | Tracks fresh-session provenance across subscription continuity; rejects transparently reconnecting transports. |
| packages/client/core/src/conversation-store.ts | Reworks reseed coverage and prompt aliasing to be provenance-safe and cursor/rewind-safe. |
| packages/client/core/src/conversation.ts | Adjusts rewind cutting to anchor on the first occurrence of a message id (creation) rather than later updates. |
| packages/client/core/src/tests/connection.test.ts | Adds coverage ensuring LinkCodeClient rejects transparently reconnecting transports. |
| packages/client/core/tests/integration/control-client.test.ts | Adds coverage for provenance invalidation across subscription-mode interruptions. |
| packages/client/core/tests/integration/conversation-store.test.ts | Adds extensive reseed/aliasing/cursor/rewind regression coverage and updates prior attachment/echo expectations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| get reconnectsTransparently(): boolean { | ||
| return this.reconnectOpts !== null && !(this.attempt >= this.reconnectOpts.maxRetries); | ||
| } |
| setSubscriptionMode(mode: SessionSubscriptionMode): Promise<RequestAck> { | ||
| return this.control.setSubscriptionMode(mode); | ||
| if (mode === 'attached') { | ||
| this.subscriptionMode = mode; | ||
| this.subscriptionEpoch += 1; | ||
| this.freshSessionIds.clear(); |
Greptile SummaryThis PR makes history/live conversation reconciliation provenance-aware and prevents hidden carrier reconnects from retaining continuity assumptions.
Confidence Score: 5/5The PR appears safe to merge, with no concrete correctness or security defects identified in the changed behavior. The continuity checks, conservative aliasing rules, identity-aware coverage, and first-entry rewind semantics align with current transport ordering and production prompt-rewrite identity flows.
|
| Filename | Overview |
|---|---|
| packages/client/core/src/client.ts | Adds generation-scoped fresh-session provenance, invalidates it across narrowed delivery, and rejects transparently reconnecting transports. |
| packages/client/core/src/conversation-store.ts | Reworks seed/live reconciliation around counted prompt rows, provenance-gated aliases, identity-based coverage, and conservative rewind translation. |
| packages/client/core/src/conversation.ts | Changes rewinds to cut at the first matching user-message entry so later same-ID cursor updates do not replay the prompt. |
| packages/foundation/transport/src/transport.ts | Extends the transport contract with optional transparent-reconnect capability metadata. |
| packages/foundation/transport/src/ws.ts | Reports whether configured WebSocket recovery can transparently replace the physical connection. |
| packages/client/core/tests/integration/conversation-store.test.ts | Adds extensive regression coverage for duplicate prompts, reseeds, cursor preservation, provenance boundaries, and rewind behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Start fresh session] --> B{All-event delivery continuous?}
B -->|No| C[Invalidate fresh provenance]
B -->|Yes| D[Record fresh-session provenance]
E[Provider history seed] --> F[Conversation reconciliation]
G[Live event buffer] --> F
D --> F
F --> H{Prompt relationship trusted?}
H -->|Yes| I[Alias echo and translate rewind]
H -->|No| J[Preserve distinct live and seeded rows]
F --> K[Keep uncovered and ephemeral events]
I --> L[Conversation projection]
J --> L
K --> L
Reviews (1): Last reviewed commit: "fix(client-core): restrict prompt binds ..." | Re-trigger Greptile
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Recommended changes before merge.
The core of this PR is right, and I want to say that first: narrowing a content-derived bind from "shared identity" to "display dedupe only", and gating the two destructive/visible powers (cursor fill, rewind re-aim) behind a four-conjunct provenance predicate, is the correct shape for this problem. I tried to break the predicate and could not — traced independently twice, no false-trust path found. Repeated identical prompt content within one fresh run is blocked by firstUserRowId; stop→resume is blocked by cleanBuffer (both stopSession/deleteSession and a conversation-rewind ingest wipe the buffer, so events[0].seq === 1 is a sound "no wipe" proxy given EventBuffer has no size cap); racing setSubscriptionMode calls fail safe toward distrust. And it is not test theatre: replacing the predicate with const trusted = true fails exactly the 5 new negative tests.
Three things I'd want addressed:
- Attachment prompts will render twice after a reseed — guaranteed, not hypothetical. Inline on
conversation-store.ts. - The rewind cut-direction fix ships with zero coverage — reverting it passes all 110
packages/client/coretests. Inline onconversation.ts. - The transparent-reconnect guard misses the one production transport that reconnects transparently. Inline on
transport.ts.
⚠️ Scope question
WsTransport's reconnect option is fully implemented and tested, but the new constructor throw makes it unreachable from LinkCodeClient. I checked: no production site constructs WsTransport with reconnect today (only tests), so nothing breaks now — but that leaves the feature with no reachable consumer. Is it being retired, or is a non-client consumer planned? Worth stating either way so the next person doesn't "fix" the throw.
ℹ️ Nitpicks
The PR body's Verification checklist leaves pnpm check:ci and pnpm test both pass and I ran the affected surface and observed the change working unticked, while the prose Verification section says both were done. Worth reconciling. (For what it's worth, packages/client/core + packages/foundation/transport = 159 tests pass on d9bd53da.)
Claude Opus ｜ 𝕏
| return; | ||
| } | ||
| } | ||
| builder.advance(event, receivedAt); |
There was a problem hiding this comment.
Dropping the image-stripped fallback means every attachment prompt renders twice after a reseed — and this is structural, not a race.
textHistoryEvent at packages/host/agent-adapter/src/history-util.ts:84 emits content: [textBlock(text)], and every adapter (claude-code, codex, pi, opencode) routes its history user rows through it. So a provider history user row is text-only by construction. An echo carrying an image block can therefore never exact-match its seed row via takeSeedPrompt's JSON.stringify key, and always falls through to this line as its own live item — landing after the agent reply that answered it, with its own Edit button (packages/presentation/ui/src/chat/user-message.tsx:66 gates canEdit on branchCursor !== undefined).
The PR's own test encodes this outcome: the expected message count went from 2 to 3.
Technical details
Required outcome: an attachment prompt appears exactly once in the transcript after a reseed, in its original position, with its image intact.
Why the old fallback wasn't the bug: the pre-PR code already kept the seed row's own branchCursor on a stripped match — it never let the echo's cursor overwrite the row's. The cursor-corruption vector this PR is fixing was the unconditional exact-content bind granting shared identity, not the stripped-content enrichment. Those are separable.
Suggested approach: restore the stripped-content match as a dedupe-only bind (trusted: false), so the echo consumes its row and is not re-folded, but gains none of the trusted powers. Optionally require the stripped match to be unambiguous (exactly one candidate row) so it can't mis-bind across repeated text.
| // in-place updates (echo cursor merges), and anchoring on one would replay the rewound item. | ||
| let cut = -1; | ||
| for (let index = entries.length - 1; index >= 0; index -= 1) { | ||
| for (let index = 0; index < entries.length; index += 1) { |
There was a problem hiding this comment.
This forward scan is a real fix on a destructive code path, but it ships with zero coverage — I reverted it to the backward scan (findLastIndex-style) and all 110 packages/client/core tests still pass. Nothing in the suite distinguishes the two directions.
Repro that does distinguish them
No seed. Feed the buffer:
user-messagehost-m1, text'old prompt'- re-echo
host-m1carrying abranchCursor(the in-place update that appends the secondentriesrow with the same id) agent-messageagent-r1conversation-rewindcitinghost-m1user-messagehost-m2, text'rewritten prompt'
Forward scan → ['rewritten prompt']. Backward scan → ['old prompt', 'rewritten prompt'] — the rewound prompt survives the cut and the transcript shows both.
Given this governs a destructive truncation, I'd want that case pinned before merge — otherwise the next person refactoring the scan has no signal.
| */ | ||
| export interface Transport { | ||
| /** True when carrier recovery can replace the physical connection without emitting `onClose`. */ | ||
| readonly reconnectsTransparently?: boolean; |
There was a problem hiding this comment.
Making this optional means every transport that doesn't declare it is treated as safe by default — and the one production transport that actually reconnects transparently is in exactly that bucket.
TunnelTransport (packages/foundation/transport/src/tunnel.ts:19) documents itself as "onClose fires on permanent closure only — transient drops reconnect internally", and does not declare reconnectsTransparently. It is handed straight to new LinkCodeClient for every cloud host: apps/mobile/src/runtime/create-host-transport.ts:13 → apps/mobile/src/runtime/host-connection-pool.ts:47. So the constructor throw never fires there, while the hazard it guards against is live: across a hidden reconnect both freshSessionIds and the buffer's seq-1 head survive, so cleanBuffer stays true and a post-gap bare echo can still be trusted.
Technical details
Required outcome: a transport whose reconnects are invisible to the client cannot be used to license a content-derived trusted bind.
Suggested approach: make the property required, so every Transport implementation is forced to answer the question rather than defaulting into the safe bucket by omission. That surfaces TunnelTransport at compile time, at which point it's a deliberate decision — declare it true and let the mobile path handle the throw, or narrow the client-side guard to something the tunnel can satisfy.
Summary
Make conversation reconciliation provenance-safe when combining provider-history snapshots with live host events.
Tracks CODE-621.
Existing limitations and follow-up
The current client still reconstructs one logical conversation by merging provider-owned history snapshots with a host-owned live-event buffer. User prompts receive different identities from the host and the provider, so reconciliation must infer relationships that the data model does not represent explicitly.
This PR narrows that inference to cases with verified provenance and fixes the resulting correctness regressions. It does not attempt to turn content matching or client-side reconciliation into the long-term architecture.
A follow-up structural refactor will introduce daemon-owned conversation identities and an immutable graph built from Session, Turn, PromptRevision, Branch, and provider-checkpoint bindings. Prompt editing, branch switching, session forking, and attachments will then operate on explicit durable references instead of reconstructing identity from provider history and event timing.
Verification
pnpm check:cipnpm testWIRE_PROTOCOL_VERSIONdoes not need to be bumped.Checklist
pnpm check:ciandpnpm testboth pass (pluscargo fmt/clippy/testfor Rust changes)WIRE_PROTOCOL_VERSIONis bumped — N/A, no wire message changed