Skip to content

fix(client-core): keep image prompts in place after conversation reseed - #464

Open
lucas77778 wants to merge 1 commit into
masterfrom
chenyu/code-602
Open

fix(client-core): keep image prompts in place after conversation reseed#464
lucas77778 wants to merge 1 commit into
masterfrom
chenyu/code-602

Conversation

@lucas77778

Copy link
Copy Markdown
Member

Summary

  • enrich lossy seeded user prompts with image blocks from their richer live echoes
  • preserve the seeded message identity, branch cursor, timestamp, and timeline position
  • consume repeated matching prompts in order so image messages are not duplicated at the bottom

CODE-602

Verification

  • pnpm check:ci
  • pnpm test — 2974 passed, 1 skipped
  • Playwright acceptance recording: after history reseed and after switching threads away/back, DOM contained 1 target message, 1 loaded image, and 0 bottom duplicates; console/page/network/HTTP errors were 0

Checklist

  • pnpm check:ci and pnpm test both pass (plus cargo fmt / clippy / test for Rust changes)
  • I ran the affected surface and observed the change working
  • If a wire message changed: WIRE_PROTOCOL_VERSION is bumped (not applicable; no wire change)
  • New code and assets are my own work, or their origin and license compatibility are noted above
  • Docs and comments are updated where behavior changed (no docs change required)

Copilot AI lite review requested due to automatic review settings August 21, 2026 07:10
@linear-code

linear-code Bot commented Aug 21, 2026

Copy link
Copy Markdown

CODE-602

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The enrichment advance registers a second replay entry under the seed's messageId, which breaks conversation-rewind for exactly the prompts this PR fixes. Reproduced locally — details inline.

Reviewed changes — the coveredBySeedfoldPreCutEvent inversion in packages/client/core/src/conversation-store.ts plus the new integration test, reviewed against the four agent adapters' history-read paths.

  • Coverage check invertedcoveredBySeed(event, …) => boolean (consulted by the caller) becomes foldPreCutEvent(builder, event, receivedAt, …) => void, which calls builder.advance itself per branch. I traced this branch by branch and it is semantically faithful: default: still advances, and the seq > uptoSeq short-circuit still skips seed-queue consumption for post-cut events.
  • Seed rows retained, not countedMap<string, number> becomes Map<string, SeedUserMessageQueue> ({ messages, nextIndex }), so the matched seed event itself is recoverable. In-order consumption for repeated identical prompts is preserved.
  • Image enrichment — a live user-message carrying an image block whose full content key misses retries with the image-filtered content, then re-advances the live content under the seed's messageId and branchCursor. Omitting receivedAt is what keeps the seed timestamp (the projection does receivedAt ?? item.receivedAt) — subtle, and correct.
  • Test — asserts the enriched message keeps the seed id/cursor, carries the image blocks, and that no duplicate lands at the bottom. It genuinely fails with the bug present.

I confirmed the premise holds where it needs to: textHistoryEvent (packages/host/agent-adapter/src/history-util.ts:67) funnels user history rows through textFromUnknown and emits exactly [textBlock(text)], so claude-code and opencode do drop image blocks, and the filtered live content serializes identically to the seed row. codex preserves images (native/codex/history.ts:153), so its first lookup hits and the fallback never fires. No adapter substitutes a placeholder block, and none drops non-image block kinds — so the image-only filter is the right scope.

ℹ️ Image-only prompts still land at the bottom

textHistoryEvent returns undefined when the text is empty (history-util.ts:76), so a prompt that is only an image produces no seed row at all on claude-code and opencode. Neither the full-content key nor the image-filtered key ("[]") can match, so the live echo falls through to the plain builder.advance and is appended below the reply it prompted. The composer permits this input, so the reported symptom survives for that case.

This isn't fixable from conversation-store.ts — there is nothing to enrich — so it's a scope question rather than a defect in this diff. Worth deciding explicitly whether it's out of scope for CODE-602 or whether the real fix belongs in the adapters.

Technical details
# Image-only prompts have no seed row to enrich

## Affected sites
- `packages/host/agent-adapter/src/history-util.ts:76``if (text.trim().length === 0) return undefined;` drops the entire history row for a prompt whose only content was an image, so no `user-message` seed event is emitted.
- `packages/client/core/src/conversation-store.ts:71-85` — both lookups miss (full key has an image the seed never had; filtered key is `"[]"` and no seed row exists), so control reaches `builder.advance(event, receivedAt)` at line 86 and the prompt is appended at the tail.
- `packages/presentation/ui/src/shell/composer.tsx:1053` — send is enabled on `hasReadyAttachment` with empty text, so an image-only prompt is reachable from the UI.

## Required outcome
- Either an explicit decision that image-only prompts are out of scope for this change (with the residual behavior noted), or a history-read that emits a `user-message` row whenever the prompt had *any* content, so there is a seed row for the client to enrich.

## Open questions for the human
- Is the intended long-term fix client-side reconstruction, or should `textHistoryEvent` stop discarding non-text blocks so the seed is faithful and the `JSON.stringify` matching becomes unnecessary? The current approach is a value-equality workaround for a lossy upstream read, and it only works while the two construction sites happen to serialize identically.

ℹ️ Nitpicks

  • The deleted coveredBySeed JSDoc carried the reason the default: branch must advance (ephemeral events — a pending permission request — are never in history.read, so cutting them erases them outright). The replacement one-liner drops that, leaving a bare builder.advance in default: with nothing to stop a future reader from "optimizing" it. A one-line note on that branch would preserve the trap without restoring the essay.
  • Reflowing the createConversationStore JSDoc left conversation-store.ts:105 over-long (… (see {@link foldPreCutEvent}). The sync is idempotent and monotone with a stable snapshot identity). Formatters don't rewrap comments, so it needs a manual wrap.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment on lines +78 to +82
builder.advance({
...event,
messageId: seedMessage.messageId,
branchCursor: seedMessage.branchCursor,
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This advance registers a second entries row under the seed's messageId, which breaks conversation-rewind for enriched prompts. createConversationBuilder.advance appends every event to a replay log, and the rewind handler scans that log backwards for the first matching user-message id (conversation.ts:211-219) — so the cut lands on this enriched row instead of the seed row, leaving the rewound prompt and the agent reply between them on screen.

I reproduced it: with a lossy seed the timeline after a rewind + replacement is ['provider-prompt', 'reply', 'replacement']; with a faithful seed (so the fallback never fires) the same sequence correctly yields ['replacement'].

Technical details
# Enrichment advance corrupts the `conversation-rewind` replay log

## Affected sites
- `packages/client/core/src/conversation-store.ts:78-82` — advances the live content under `seedMessage.messageId`, after the seed fold at line 158 already advanced the seed event under that same id. `entries` now holds two `user-message` rows sharing one `messageId`.
- `packages/client/core/src/conversation.ts:211-219` — the rewind scan walks `entries` from the end and cuts at the first id match, i.e. the enriched row. `entries.slice(0, cut)` therefore retains the original lossy seed row and every event that landed between the two.

## Reachability
- `packages/presentation/ui/src/chat/user-message.tsx:98``onEditPrompt(item.id, item.branchCursor, …)`. After enrichment `item.id` *is* the seed `messageId` and `item.branchCursor` is the seed cursor, so edit is enabled and keyed on the enriched identity.
- `packages/client/workbench/src/surface/workbench.tsx:346` — forwards that id as `sourceMessageId`; the engine echoes it back as `conversation-rewind` with the same `messageId`.

## Reproduction
Seed `[{ user-message 'provider-prompt', content: [text] }, { agent-message 'reply' }]`, `uptoSeq: 2`; live buffer holds the image-bearing echo `'host-prompt'` plus the same reply. Enrichment fires. Then send `conversation-rewind` for `'provider-prompt'` followed by a replacement prompt:

```
lossy seed (fallback fires):    ['provider-prompt', 'reply', 'replacement']   // wrong
faithful seed (no fallback):   ['replacement']                               // correct
```

## Required outcome
- After a rewind targeting an enriched prompt, that prompt and every later item must leave the timeline, exactly as they do when the fallback never fires.

## Suggested approach (optional)
- Enrich the seed event's `content` *before* the initial fold rather than re-advancing afterwards, so `entries` keeps one row per message identity. The live buffer is already reachable from `sync()`, which folds the seed and then walks the live events on the same call.
- Alternatively, make the rewind scan cut at the *first* matching entry instead of the last. Since a user `messageId` is a single identity, forward and backward scans differ only when one id was folded twice — precisely this case.

## Open questions for the human
- A prompt rewrite also emits `session-ref` with a new `historyId`, which re-keys `useSeededConversation` and would build a fresh store from a fresh seed. How reliably does that reseed land? It probably bounds this to a transient window rather than a permanent wrong state, but it depends on the reseed arriving and on SWR yielding a new seed identity, so I would not lean on it.

id: 'provider-prompt',
role: 'user',
blocks: livePrompt.content,
branchCursor: 'provider-cursor',

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The seed row here has no ts, so nothing in this test pins timestamp preservation even though the PR description claims it. That matters because the preservation is implicit: omitting receivedAt from the enrichment advance is the only reason the seed timestamp survives (receivedAt ?? item.receivedAt). A future refactor that threads receivedAt through would silently stamp the live receive time onto a history message with no test failing.

Giving the seed row a ts and asserting receivedAt alongside the existing fields would close that.

Suggested change
branchCursor: 'provider-cursor',
branchCursor: 'provider-cursor',
receivedAt: 1_700_000_000_000,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants