Skip to content

fix(native-chat): move queued messages on /clear only after it commits, fixing main's typecheck - #23939

Closed
brennanb2025 wants to merge 1 commit into
mainfrom
fix-clear-carry-replacement-id
Closed

brennanb2025 wants to merge 1 commit into
mainfrom
fix-clear-carry-replacement-id

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 3 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​115 $\color{#cf222e}{\Huge{\mathbf{−}}}$​62 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​53
Prod 4 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​26 $\color{#cf222e}{\Huge{\mathbf{−}}}$​24 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​2

ELI5

main is red: its typecheck fails, so every open PR's CI fails, and two queue tests about /clear fail. Two PRs landed back to back without being tested together: #23524 (an unfinished /clear no longer locks the chat) and #23726 (the mid-turn message queue). #23524 changed how /clear works underneath, and #23726's queue code for /clear was written against the old way. This PR brings the queue in line with #23524, which fixes main.

It also fixes a real bug that the combination created: a /clear that failed partway could make your queued messages disappear from the chat you were still in.

What Changed

Scope. Native (structured) Claude and Codex chats, only when a /clear runs while messages are queued. The queue is still switched off (the host doesn't advertise it), so no user can reach this yet. Terminal agent tabs, /compact, rewind and ordinary sends are not affected.

Before (on main now):

  • The typecheck fails at structured-conversation-command.ts:273: the queue's /clear hand-over reads the new conversation's id through a variable TypeScript can't narrow (string | undefined).
  • The queue moved your queued messages to the new conversation after its agent started but before the /clear was saved. That order was safe only while the old "clear in progress" record locked the chat. fix(native-chat): an unfinished /clear or refused Codex rewind no longer locks the chat #23524 removed that record: a /clear that doesn't finish now counts as never having happened, and you stay in the old chat. So if saving the /clear failed, or Orca quit at that moment, your queued messages had already been moved to a conversation nothing points to. They were gone from the chat you were still using.
  • Three queue tests still set up the removed "in progress" record, so they fail or test a situation that can no longer happen.

After:

  • The queue moves the messages only after the /clear is saved. A /clear that never finished leaves your queued messages where they were, still under whatever pause they already had. Running /clear again from the same window saves it and then moves them to the new conversation, paused with "Queue paused after you cleared the conversation", as before.
  • A normal /clear behaves exactly as before.
  • Tests now describe fix(native-chat): an unfinished /clear or refused Codex rewind no longer locks the chat #23524's model:
    • a send during a running /clear is still refused and no card lands on the old chat;
    • an older build's leftover "clear in progress" record holds nothing, and the queue sends normally;
    • a /clear whose new conversation definitely failed to start leaves the messages on the old chat under the Stop pause, not a "cleared" one;
    • a /clear whose save fails moves nothing, and a retried /clear saves and moves them;
    • Send-now on a card left behind on a chat that a /clear replaced is refused, because that chat was cleared.
  • Four comments that still described the removed record are corrected.

Why

Moving the messages after the /clear is saved makes the move depend on the one fact that decides which conversation you are in. The alternative, keeping the earlier order and undoing the move when the save fails, needs an undo step that can itself fail, and would bring back the "clear in progress" state #23524 removed.

Known gap, temporary. If Orca crashes in the moment between saving the /clear and moving the messages, they stay on the replaced chat, hidden and never sent. Nothing re-derives them later. That's the same outcome the queue already has when the move itself fails (reported, never blocking the /clear). The old order had the opposite gap, and a larger one: messages lost whenever the save failed. The follow-up that closes it: when a chat that a /clear replaced still holds queued messages, move them again (the move is already safe to repeat). I'll do it as a separate PR before the queue is switched on.

Linked Issue

None; main breakage from #23524 + #23726.

Visual Proof

N/A: host-side ordering and tests only; no screen changes, and the queue is still switched off.

Testing

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

  • tc:node passes (it fails on main).

  • 17 test files: the queue's /clear, gate and command tests, every test file that imports a changed file, and all of fix(native-chat): an unfinished /clear or refused Codex rewind no longer locks the chat #23524's test files. All pass.

  • Ablation: moving the hand-over back before the save makes the new save-failure test fail with the messages gone from the old chat.

  • structured-chat-coordinator-mail.test.ts failed twice under heavy local load on fixed-time checks. It passes alone and doesn't touch the queue or /clear.

AI Disclosure

Review

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

The host-owned queue (#23726) landed right after #23524, which made a /clear
write nothing the chat reads until it commits. The queue still assumed the
old flow: its /clear carry moved the source's drafts to the replacement
before the commit, so a commit that failed left the drafts on a replacement
nothing points at while the source, not cleared, kept working without them.
Carry after the commit instead, so an unfinished /clear leaves the drafts on
the source, unpaused by it, and a retried /clear that commits carries them.

Fix the /clear carry call's unnarrowed replacement id (main's typecheck
error). Rewrite the tests that waited on or planted the removed "prepared"
clear record: a send during a /clear is refused by the command controller
with no record written, an older build's unfinished record holds no draft,
and Send-now's blocked case uses a source a committed clear superseded. Add
tests for a /clear that fails to start or to commit.
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Closing in favour of #23940 (from the owner of #23524), which fixes main the same way. The one extra change here, handing queued messages over only after a /clear is saved, arrives with #23935.

@greptile-apps

greptile-apps Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

[Medium risk] Fixes message queue handling during conversation clear operations.

The PR appears safe to merge while the queue remains disabled, but committed clears need draft-carry recovery before the queue is enabled.

Findings

  1. P2 Drafts stranded after clear ▶

Summary

The PR moves queued-draft carry until after a /clear commits, fixing the typecheck error and keeping drafts on the original chat when the commit fails. It also updates tests and comments for the current clear lifecycle. The new ordering leaves a recovery gap if the process exits between commit and carry.

Reviews (1) · Last reviewed commit: "fix(native-chat): carry queued drafts on..."

Comment on lines 275 to +281
await store.setConversationCommand(sessionId, ctx.fence, completed)
if (ids) {
// Carry the source's drafts only once the clear committed: an unfinished clear
// changed nothing, so its drafts stay on the source. The same for every client
// version, and no text rides the wire. Bookkeeping — a failure is reported and never
// fails the clear.
await carryQueuedMessagesToClearReplacement(ctx, {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Drafts stranded after clear If Orca exits after /clear commits but before the queued drafts are moved, they remain on the old chat. That chat can no longer send them, and retrying the committed command does not move them, so the drafts never appear in the replacement chat. This recovery gap needs to be closed before the queue is enabled.

Knowledge Base Used: Agent workspaces and sessions

@pullfrog pullfrog Bot 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.

✅ No new issues found.

Reviewed changes

  • The queue carry now runs after the clear's commit (structured-conversation-command.ts): carryQueuedMessagesToClearReplacement moved out of the attach block to after setConversationCommand(sessionId, ctx.fence, completed), so the move mirrors the fact that decides which conversation you are in. An unfinished clear now changes nothing and leaves the drafts on the source.
  • The tc:node failure is fixed by narrowing, not by a cast: the carry passes ids.sessionId (narrowed by if (ids)) instead of the outer replacementSessionId (string | undefined). Confirmed locally — pnpm tc:node exits 0.
  • Gate and command tests realigned to #23524's model: the "clear in progress" prepared record is gone from setup; two new command tests pin the save-failure and definitive-attach-failure outcomes, and the gate's Send-now test now drives a real clear that fails mid-carry.
  • Comment-only updates in structured-agent-session-queued-mutations.ts, structured-agent-session-queued-messages.ts, and the command controller, dropping the removed "clear in doubt" vocabulary.

I verified the touched/relevant suites (structured-agent-session-queued-command, -gate, -messages, and structured-conversation-command) pass: 72 tests, and tc:node is clean. The save-failure test genuinely fails on the old order (where the commit throws after the carry has already moved the drafts), so it is a real regression pin rather than theatre.

ℹ️ Post-commit interruption has no user-reachable recovery path

The PR documents the crash window between the committed clear and the carry as a known gap, and that is accurate — but worth sharpening the follow-up's scope: once the clear has committed, neither a fresh operator/clear nor a replay of the same operation recovers the stranded drafts. A new /clear from the superseded source is refused conversationCleared, and a same-operation replay is short-circuited because replay returns the committed record through matching() without re-entering run — so the carry is never re-run and nothing re-derives the rows. This is only reachable while the queue is advertised (it is not, yet), so it is fine to land now; the follow-up should scan superseded sources for unsettled drafts rather than assume a retry will re-carry them.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

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.

1 participant