Skip to content

fix(native-chat): an unfinished /clear or refused Codex rewind no longer locks the chat - #23524

Merged
brennanb2025 merged 236 commits into
mainfrom
brennanb2025/chat-clear-latch-dies
Sep 29, 2026
Merged

brennanb2025 merged 236 commits into
mainfrom
brennanb2025/chat-clear-latch-dies

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 6 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​536 $\color{#cf222e}{\Huge{\mathbf{−}}}$​61 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​475
Prod 11 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​148 $\color{#cf222e}{\Huge{\mathbf{−}}}$​95 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​53

ELI5

The problem: if a /clear in a native chat got stuck halfway, that chat stopped accepting anything. Every message, /compact, another /clear and rewind was refused, and the only way out was a retry of that same /clear that only the open window knew how to send. After a restart, nothing could unlock it; the person had to start a new chat.

Now a /clear that never finished counts as never having happened, so the chat keeps working, and running /clear again from the same window finishes it without leaving a second new conversation behind. A separate stuck state after a Codex rewind that didn't take effect is fixed the same way.

Scope: native (structured) Claude and Codex chats on any host, local or over SSH. Terminal agent tabs are not affected. Normal /clear, /compact and rewind behave as before. The bug is pre-existing on main; it was found in review of #23059, which this PR is stacked on.

What Changed

Before: if starting the new conversation for /clear failed without a definite answer (for example "too many operations"), or Orca quit mid-clear, the chat refused every message, /compact, another /clear and rewind, permanently. A message got "The last /clear didn't finish. Your message was not sent. Start a new chat to continue." Separately, a Codex rewind whose recovery found the rewound turn still in Codex's history could be left "in doubt" forever if Codex had acknowledged the request, and then every attempt to start that chat's agent failed, so every send failed too.

After: a /clear that never finished changes nothing and the chat keeps working. Running /clear again from the same window finishes the clear onto the new conversation the unfinished try already started, including after a restart, whether the window resends the same request or sends a new one. If that start failed or never answered, the new /clear simply starts afresh. While one window's unfinished /clear still has its new conversation's agent running, another window's /clear waits ("A /compact or /clear is still running"); that ends when the agent stops, at the latest after the 30-minute idle timeout. A Codex rewind that did not take effect is marked refused, and the chat starts normally.

No new messages. A /clear whose new conversation definitely failed to start still says why (for example "Codex is not signed in for the selected account. Sign in, then run /clear again."), as #23059 already does.

Mechanism, /clear: it used to save an "in progress, outcome unknown" record on the chat before starting the replacement conversation, and every send, /compact, /clear and rewind checked for it. It now writes nothing the chat reads until it commits the replacement, and those checks refuse only a chat that has actually been cleared. Records of this kind left by older builds are ignored.

Which replacement a /clear uses is decided by the host, not by the window. The host keeps a durable log of each request (the operation ledger) with its outcome and the start of any conversation it launched. For a /clear it looks for that window's earliest /clear on this chat since its last completed one whose new conversation started successfully, and reuses that try's replacement id; if there is none, the id comes from this request. So a second /clear from the same window, under a new or the same request id, lands on the same replacement: when that conversation is at rest (its agent stopped by a restart or the idle timeout) the /clear commits onto it like any normal /clear, and its first message starts its agent; when it is running the /clear attaches to it. A try whose start failed or never answered left nothing to finish and is skipped. A /clear after a completed one (including a completed failure, such as "not signed in") gets a new replacement. Another window's /clear is refused only while such a replacement's agent is running here; one Orca merely can't prove stopped gates nothing.

Mechanism, rewind: the chat's own history is replaced only after Codex proves the rewind. Recovery reports "refused" only when it reads the rewound turn still in Codex's history, so at that point both sides still agree the turn is there, and the rewind is settled as refused whether or not Codex acknowledged the request. It used to settle only when the request was never acknowledged, and otherwise threw inside every agent start. The saved "Codex acknowledged" flag had no other reader, so it is no longer written; older records that carry it still load.

Why

The saved "clear in progress" record guarded nothing the running app didn't already enforce through the per-session lock and the command controller. It only mattered after a failure, where all it did was lock the chat for good. Removing it removes that state; a stale record can no longer lock the chat.

Alternative considered: keep the record and add recovery that finishes or abandons it when the chat is next opened. Finishing is only safe while the record still blocks the chat, so the lock would stay durable, and it would still stick in a running app whenever the recovery's own write failed.

Why the host decides the replacement id: windows are moving to a fresh request id per press (a separate PR), so an id derived from the request would give every retry a new replacement and leave the earlier one unused. Deriving it from the host's own log keeps one replacement per unfinished /clear, needs no new saved field and no protocol change, and an older window that resends the same request id still lands on the same replacement. An earlier version of this PR instead answered a retry after a restart with a new "This /clear didn't finish ... Run /clear again" message; that was chosen from a generic "owner unproven" refusal that in fact meant the new conversation was already stopped and usable, so it is gone.

A crash after the replacement conversation started but before the commit, with no later /clear from that window, leaves an unused replacement that nothing points at. Verified in code and pinned by tests: it has no tab and is not in the replacement list; restart opens and starts nothing for it and releases its lease like any dead owner's; in a running app the idle sweep stops its agent like any quiet agent. Its record stays on disk, as every closed chat's record does; the store deletes none. This leftover is pre-existing (the old code started the replacement before its commit too).

Unverified: whether a provider can write a transcript for a thread that never received a message, and whether a provider child can outlive an Orca crash (it runs over stdin, which closes when Orca dies), in which case that unused replacement's leftover ownership is only settled by a later /clear from that window; until the log entry ages out (about a day) such a /clear reports "not done" instead of clearing, which a new chat works around. The rewind "acknowledged but still in history" case is a guard: the Codex revert swaps its history before it replies, so it has not been seen with a real Codex; tests drive it.

Follow-up: starting the replacement conversation only on its first message, with the /clear itself a single saved switch, would remove the unused-replacement leftover, the retry bookkeeping above and the other-window wait altogether; it changes when a start failure shows (on the first message instead of at /clear), so it is left for its own PR.

Known, not changed here (pre-existing): a refused Codex rewind does not record its outcome, so retrying that same rewind reads "outcome unknown"; a follow-up records the refusal as the operation's outcome. A rewind whose recovery fails for other reasons (history proof mismatch, unreadable history) still stays "in doubt" with no expiry.

Differences from the common pattern

  • Matches: interrupted work is settled from what the host recorded, not kept behind a durable "in progress" flag that blocks sends.
  • Temporary: the /clear starts the replacement conversation before it commits. The common pattern makes a clear a single saved switch and starts the next agent on the first message. Starting first is why a crash between the two can leave an unused replacement, and why the host needs the retry bookkeeping above (which earlier try to finish, and the other-window wait). The follow-up that removes it is the lazy-start change described under Why; it has no PR or ticket yet.
  • Intended: a refused rewind is settled from the provider's own history listing, not from whether the request was acknowledged. It is safe because the chat's history is replaced only after the provider proves the rewind, so when the provider still lists the turn, both sides agree it is there.

Linked Issue

N/A. Found in review of #23059; pre-existing on main.

Visual Proof

Live QA on a separate Mac with a background-launched development build of b67389a (the later commit changes only which earlier try a second /clear picks, not a first /clear), its own Orca profile and home folder, and a scripted Claude.

A normal /clear moves the chat to a fresh conversation, and the next message is answered there:

normal clear: the fresh conversation shows only Say HELLO. and its answer

A /clear whose new Claude dies while starting: Claude's start returns before its agent finishes starting, so the clear itself completes, and the dying agent shows "Claude stopped before it finished starting. Send your message to try again." The chat is not locked; the next message is answered right away:

send after a clear whose new agent died while starting: the red row, then Say AFTER. answered

Running /clear again afterwards clears normally:

second clear: only Say FRESH. and its answer

Live QA on this laptop with the real, signed-in Codex and a background-launched development build of this PR (before the review fixes, which don't touch these paths) using its own Orca profile. The chat asked for ALPHA, then BRAVO; the BRAVO turn was rewound; then CHARLIE was asked. Asked to list every word it had been asked for, Codex answers "ALPHA, CHARLIE", so the rewind took effect in Codex as well as in the chat, and the chat kept working after it:

Codex rewind: the chat shows ALPHA and CHARLIE, and Codex lists only ALPHA, CHARLIE

Then /clear in the same Codex chat: the chat moved to a fresh conversation, and the next message (DELTA) is the only exchange shown and was answered there:

Codex after /clear: only the DELTA exchange, answered in the fresh conversation

Not shown live, driven by tests instead: a /clear whose start is refused without a definite answer or cut off by a restart (Claude's start returns before its agent finishes, so neither can be forced without editing source), a second /clear finishing the first one's replacement, the other-window wait, and a Codex rewind that Codex acknowledges but does not keep.

Testing

  • I manually tested these changes locally
  • Automated tests added/updated, or explained why not below

Host tests, each red first or red with its fix removed:

  • After a /clear whose replacement start is refused without a definite answer: rewind gets past the conversation checks, /compact is accepted, a send is accepted, and a /clear under a new request id commits.
  • A second /clear from the same window under a NEW request id, while the first try's replacement runs, lands on that replacement with no second one and nothing left over.
  • The same after a restart: it commits onto the replacement the first try made, with no second start; the old chat then refuses sends as cleared, and the replacement starts its agent on its first message. Also within one app session once the idle sweep stopped it.
  • Another window's /clear is refused while that replacement's agent runs and runs once it stops; it is not refused when the replacement is merely not proven stopped.
  • A /clear after a completed one (in either window), and after a completed failure whose log settlement was lost, gets a new replacement.
  • A second /clear after a start that definitely failed ("not signed in"), or one a restart cut off before it answered, starts afresh instead of repeating that failure.
  • A same-request resend (older windows) still commits with exactly one replacement start, and a same-request retry after a definite start failure still reads that failure.
  • A restarted host holding an older build's unconfirmed /clear accepts a send and commits a new /clear; the command admission check ignores an uncommitted /clear and refuses a committed one.
  • The unused replacement: no tab, not in the replacement list, its agent stopped by the idle sweep, and after a crash and relaunch its lease released.
  • Codex rewind acknowledged but not kept: the agent start succeeds, the chat history is unchanged, the rewind reads refused, and a send is accepted.

At 4974af9: the /clear suite (26 tests) and 8 neighbouring clear, compaction, rewind, send-check and failure-words suites pass; tc:node passes (and tc:web at the last shared-file change). macOS only; no platform-specific code touched.

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

Stacked on #23059. A separate PR makes windows send a fresh request id per /clear press; it relies on the host-side replacement choice here and lands after this PR.

Messages this host no longer sends, kept for older hosts: the send refusal with reason clearUnconfirmed ("The last /clear didn't finish. Your message was not sent. Start a new chat to continue.") and the command refusal with reason conversationCommandUnconfirmed ("The command didn't run."). A host with this PR never sends them; a host without it still does, where their words are true, so both reasons and their words stay in the client tables.

Author X: @BrennanKB5

Backwards compatibility: records written by older builds with an unconfirmed /clear become inert, not an error; their replacement id is derived the same way, so the next /clear from that window lands on the same replacement. Downgrading to an older build after a stuck /clear written by that older build brings its lock back, as on that build today. No wire or persisted shape changes: the committed record is the same as any normal /clear's, and the dropped rewind field was optional. SSH: host-side only; the same code runs on remote hosts. Mobile: no code change; the phone prints the host's sentence for a /clear result.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

brennanb2025 and others added 30 commits September 24, 2026 23:36
No client ever called agentSession.requestHandoff or mounted the handoff
chrome. Delete the handoff coordinator, the terminal-owner runtime, the
proof write path and the unmounted UI. Keep agentSession.handoffStatus,
which released desktop clients read for worktree activation, and let
records an older build left mid handoff reconcile through the ordinary
restart and recovery paths.
Eviction now drains delivered events before quit's resume-offer snapshot. An
unbounded wait there sits ahead of the provider stop, so a sink whose journal
write stalls kept the child running until the step deadline aborted the
eviction. The offer is advisory: bound the drain and stop the child regardless.

Co-Authored-By: Claude <noreply@anthropic.com>
`claudeAuthEnvCarriedForward`, `isPathWithinDirectory` and
`queryWindowsProcessRowsFresh` lost their last caller with the handoff. The
fresh-scan tests now go through `queryWindowsProcessDescendants({ fresh: true })`,
the teardown path that still depends on that contract.

Co-Authored-By: Claude <noreply@anthropic.com>
Six comments still named the handoff coordinator, a handoff suspend, or a
terminal-owned session as live participants in the flows they describe.

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
…ement

The removed restart handoff test pinned this branch; nothing else did.

Co-Authored-By: Claude <noreply@anthropic.com>
The handoff removal dropped the per-session queue from `handoffStatus`, so a
read landing mid-start reported the reservation (no owner) instead of the
settled chat owner, and shipped desktop clients blocked worktree activation on
it. The read is queued again, as it was before the removal.

Co-Authored-By: Claude <noreply@anthropic.com>
The gate only refused a write when a PTY had been bound to a chat session, and the
only code that ever bound one was the terminal handoff this branch removes. With it
gone, every admit/readmit returned "admitted" unconditionally, so the checks on the
renderer write path, the runtime controller backstop, terminal.send, agent prompts,
preview input and orchestration pointers, the refusal fields on terminal.send and
worker-start receipts, the plugin and CLI refusal copy, and the adopted-pane
orchestration routing could no longer run. Ordinary writes take the same path in
the same order as before.

Co-Authored-By: Claude <noreply@anthropic.com>
…alled

appendLegacyTranscriptMessages fed the terminal transcript catch-up and
proveClaudeTranscriptBranch backed the terminal owner's exit proof. Both lost
their last caller with the handoff. Their tests now go through the live entry
points instead: the roster bounds through the legacy import, the pinned-read and
growth tests through the ancestry replay the history window uses, and the marker
rules through the string proof in their own file rather than the session-file
resolver's.

Co-Authored-By: Claude <noreply@anthropic.com>
A send refused because the chat's owner is not settled showed "The session is
mid-handoff (<stage>)." in the composer. With the handoff gone, the stages that
reach it are a chat that is still starting, or one whose previous agent process
has not yet been confirmed stopped. The message now says which of the two it is.
The refusal code is unchanged.

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
With the terminal handoff gone, the module named codex-tui-rollout-proof holds
only the pinned rollout lookup that structured Codex launches use to resume a
thread, so the name described code that no longer exists. Rename the module and
its options type. Also drop a mobile allowlist assertion that pinned the
removed agentSession.requestHandoff method, which no longer exists to allow.
The handoffStatus reply type still listed the terminal handoff's fields and
states (terminal placement, host label, proof retry, queued and waiting phases,
the to-terminal direction). No host writes them any more and the only client
reader parses the reply as unknown, so they described nothing. The reply on the
wire is unchanged.
…t decode

Nothing in this build writes a terminal owner (`runtimeKind: 'tui'`) or the
handoff's `preparing` / `old-owner-stopped` stages, but the in-memory types
still admitted them, so readers across the host kept branches for values no
path produces and the compiler could not point at them.

The store now validates the on-disk shape, which still accepts those values so
an older record is not quarantined, and maps them once while parsing:

- `preparing` and `old-owner-stopped` become `recovering`
- a `tui` lease becomes `native`; when it records a process it also becomes
  `conflicted`, the claim every build probes but never stops. A plain native
  owner would be stopped by restart recovery, here and in older builds.

Revisions are taken over the normalized state on both sides of every compare,
and the mapped record reaches disk with the store's first transaction, the
same way the tab-id backfill does.

The in-memory types narrow to what this build writes, and the branches that
existed only for the removed values go. Structured-worker identity keeps its
verdict for a former terminal owner by refusing a conflicted claim rather
than a non-native kind.
…ation

A reservation only ever names a native owner now, so the request no longer
carries a kind and the reserved lease records `native` directly. The attach
params keep `runtimeKind`: agentSession.ensure and create accept it, and the
operation fingerprint stored in the ledger covers it.
…at changes nothing else

Hiding a tab also committed the visibility index, so the no-op transaction
wrote the file even when its open-time revision was wrong. Committing the index
first leaves the pending rewrite as the only reason to write.
…ration

A write carried the fence of the last frame the pane read, and the host refused it
unless that fence was still current. An idle release and the restart after it each
move the fence, and the release publishes nothing, so a send after a release was
refused "Expected runtime fence 1; the session is at 3", and a Stop queued behind a
cold start was refused as stale.

Every write already names what it acts on: a send its conversation, a cancel its
turn, a prompt answer its item revision, a rewind its epoch; an option is
last-writer-wins. So admission stops comparing the client's fence, and the rebase
that papered over one restart (admitAtResumedFence, resumedFromFence) goes with it.
The writer-lease check stays, and so does the attach's compare-and-swap.

Frames now stamp the fence read when each frame is sent instead of a copy each
subscriber kept, which went stale on the same release.
A journal write and its delivery to open readers were two calls, and some
writers made only the first. A failed start whose lease could not be handed
back, a provider revision with no frame behind it, and eviction's settlement
were all journaled without reaching an open chat.

A journal handle now reports every durable change, and the host's session map
binds that report to the session's readers when the handle is set. Writers no
longer publish what they append; the per-writer publish calls are deleted.
…ackfill cannot supply its rewrite

The seeded record had no surface tab id, so the next open backfilled one and
that rewrite alone made the no-op transaction write. The test passed with the
legacy-lease rewrite signal removed.
The handoff removal deleted it alongside the terminal-owner tests, but it
covers the surfaced-PTY block that still guards resume, including an agent
whose ownership is unknown.
Each commit now delivers itself, so the publish a provider frame still sends
afterwards found every reader caught up but still read rows and rebuilt the
timeline for each one. A caught-up reader now skips the read.
# Conflicts:
#	src/main/native-chat/agent-session-wire/structured-agent-session-subscribers.ts
A send to a chat with no running agent restarted the agent inside the send
call, before the message was recorded, so the client waited for the whole
start and a failed restart refused the message. Claude held prompts sent
during startup, and those could settle as "unconfirmed".

A send is now accepted inside the session's serialized queue: one ledger row
and one submission row marked handoverRecorded, published, answered pending.
A per-session delivery loop exists while a message is queued. It starts the
agent through the same serialized attach a hold uses, waits outside the queue
for a Claude child to prove its start, and hands the oldest queued message
over as its own serialized step, writing dispatch{pending} before the adapter
call. A start it needed and did not get writes one error-tone row and rejects
every queued message with the same words; a start Stop cancelled writes none.

Settlement follows from the rows. A queued message is provably unwritten, so a
close, an eviction or an exit rejects it. A handed-over message stays in doubt.
A queued row at or below the sequence a handle found when it opened was left
by an earlier process and is rejected at open, with no latch. Stop withdraws
queued messages with no writer lease and no fence. An attach failure keeps the
conversation open, and the attach adopts its journal. Owed work counts the
loop and queued rows.

A compaction or rewind found prepared when a conversation opens was started
under a child this process no longer has, so the open settles it rather than
leaving it to refuse every send until a view attaches. The open cursor is
scoped to its epoch, because sequences restart when an epoch is replaced.

Deleted: restart-before-admission, recordFailedRestart, the fence rebase,
Claude's startup gate, the attach's forget on failure and its own crash
boundary. Clients without agent-session.accepted-send.v1 get their reply held
until the handover; the desktop and paired desktop lists advertise it.
A child that proved its start and then exited before its message was handed
over left the message queued: the exit settlement returned early when nothing
else was in flight. Delivery then started another child for it, and a child
that died the same way started another, without end and without a row.

A retried settlement for an earlier generation, run by the attach that
delivery started, did the opposite: with that generation's turn unfinished it
rejected the message queued for the child being attached.

The settlement now takes the rejection for queued messages from its caller.
The unexpected exit and the eviction pass one, and it applies even with no
other work in flight; the retry for an earlier generation passes none.
@brennanb2025
brennanb2025 marked this pull request as ready for review September 29, 2026 12:00
@brennanb2025

brennanb2025 commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

Review status at 4974af9: ready for review (not merged)

The problem. If a /clear in a native Claude or Codex chat was cut off before it finished (a start that ended without a definite answer, or Orca quitting mid-clear), the chat refused every message, /compact, /clear and rewind for good, with "The last /clear didn't finish… Start a new chat to continue." Separately, a Codex rewind whose recovery found the turn still in Codex's history could stay "in doubt" forever, which made every agent start, and so every send, fail.

What changes for you. An unfinished /clear leaves the chat usable. Running /clear again from the same window finishes it onto the new conversation the unfinished try already started, including after a restart and whether the window resends the same request or a new one, so no second leftover conversation is created. If that start failed or never answered, the new /clear starts afresh. While one window's unfinished /clear still has its new agent running, another window's /clear waits with the existing "A /compact or /clear is still running" message, at most until the 30-minute idle timeout. A Codex rewind that didn't take effect reads refused and the chat keeps working.

Changed during review (5 commits):

  • 84fc7ff: a retried /clear finishes onto its at-rest replacement instead of a new "This /clear didn't finish" message, which had been chosen from a mislabelled refusal. That extra failure type is gone.
  • 8995771: the rewind record's now-unread "Codex acknowledged" flag is no longer written.
  • 1e087ec, per a coordinator ruling: the host, not the window, decides which replacement a /clear uses, from its own request log. This is needed because windows are moving to a fresh request id per press in a separate PR. No new saved field and no protocol change.
  • b67389a and 4974af9 fix three review findings in that change. A retry no longer repeats an old "not signed in" failure. Another window is no longer refused for up to a day by a leftover merely not proven stopped. A try cut off mid-start is skipped rather than replayed.

Deferred (not blocking):

  • Follow-up design: start the new conversation only on its first message, with /clear a single saved switch. That removes the leftover unused conversation after a crash (pre-existing), the retry bookkeeping and the other-window wait. It moves a start failure from /clear to the first message, so it belongs in its own PR.
  • P3: if an unfinished /clear's new agent can't be proven stopped after a restart, that window's next /clears report "not done" until the log entry ages out (about a day); a new chat works around it. Low reachability.
  • P3: the retry reuses the first try's account and options if they changed in between.
  • Pre-existing: a rewind whose recovery fails for other reasons (history proof mismatch, unreadable history) stays "in doubt"; a refused rewind's retry reads "outcome unknown".
  • Not this PR (fix(native-chat): turn facts come from the turn record, and /compact is a message the chat sends #23059's wording): after a cleared chat's new Claude dies while starting, "Send your message to try again" shows with an empty composer, and the red row stays above later answers.

Verified:

  • Two full review rounds. The second came after the architecture change.
  • Narrow reviews of each fix commit and two readiness checklists: no P0/P1 caused by the PR remains open.
  • The /clear suite (26 tests, including red-first tests for each ruling case and each fix) and neighbouring suites pass; tc:node passes.
  • Live QA on the M4Air at b67389a (fresh profile, isolated home, scripted Claude) passed a normal /clear, a /clear whose new agent dies while starting (not locked; the next message is answered), and a second /clear. The last commit only changes which earlier try a second /clear picks.

Not verified live:

  • A /clear refused or cut off mid-start, a second /clear finishing the first one's leftover, and the other-window wait. Tests drive all three.
  • The real-Codex rewind shots predate the review fixes, which don't touch that path.
  • No SSH or mobile run: the change is host-side only.

@greptile-apps

greptile-apps Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Medium risk] Fixes chat lock when rewind or clear command fails to complete.

No new blocking issue was identified in the changes since the previous review.

Summary

The PR lets a native chat continue after an uncommitted /clear and settles a Codex rewind as refused when provider history still contains the target turn.

  • /clear retries use the host’s operation ledger to reuse a replacement conversation that was successfully started before an interrupted commit.
  • Sends and commands no longer treat an older build’s uncommitted clear record as a durable lock.
  • The merge since the previous review leaves the PR’s 17-file patch unchanged.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A["/clear request"] --> B["Find earliest unfinished successful start for this caller"]
  B --> C["Attach to or start replacement"]
  C --> D{"Replacement usable?"}
  D -- Yes --> E["Commit clear to original chat"]
  D -- Definite start failure --> F["Commit failure; keep original chat"]
  D -- Outcome unknown --> G["Leave original chat usable for retry"]
Loading

Reviews (4) · Last reviewed commit: "Merge origin/main (28073327354, #23923) ..."

@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

  • /clear stops writing a prepared record — structured-conversation-command.ts now writes nothing the source chat reads until it commits the replacement, so a clear cut short by a crash or an unproven refusal leaves no durable state behind. An older build's prepared/unconfirmed record is inert.
  • Retries land on the same replacement — the replacement id is always the deterministic clear-<sha256([sessionId, callerKey, operationId])> (the prior?.replacementSessionId ?? fallback is gone), and rerunWhenReplayMissing now reruns any uncommitted clear under the same op id.
  • startedAndAtRest finishes a retry onto an existing replacement — when re-attach is not ok, the clear commits anyway if the replacement record exists, its lease is released, and the attach operation row for the derived attach id succeeded. This covers both restart and idle-sweep.
  • Admission and send block now refuse only a committed clear with a replacement id — conversationCommandBlocked and structuredAgentSessionSendBlock treat prepared/unconfirmed records as inert; clearUnconfirmed / conversationCommandUnconfirmed stay in the shared tables with no producers, labelled "older hosts only".
  • Codex rewind drops providerApplied — onReverted is removed from the adapter and the persisted schema; recoverStructuredRewind settles provider-refused whenever the phase is prepared, since the journal is replaced only after the provider proves the revert.
  • Tests — new host-level coverage for a never-committed clear (rewind/compact/send all run, new-op clear commits, same-op rerun starts exactly one replacement, retry after a crash and after the idle sweep, definite failure keeps its words, orphan has no tab and is released) plus rewind recovery tests for the acknowledged-but-not-kept case.

I ran the focused suites (structured-conversation-command, structured-agent-session-send-block, structured-conversation-command-admission, structured-agent-session-rewind, structured-agent-session-rewind-at-rest, codex-structured-rewind, structured-agent-session-send-preparation — 94 tests), pnpm tc:node, pnpm tc:web, and oxlint on the changed source files; all pass. The deferrals called out in the PR body (orphan-replacement cleanup, no outcome recorded for a refused rewind, unverified "acknowledged but not kept" provider-sync guard) are reasonable and correctly scoped as non-blocking.

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

@brennanb2025
brennanb2025 marked this pull request as draft September 29, 2026 17:37
…the same replacement

A /clear's replacement id came from the client's operation id, so a retry the client sent
under a fresh id started a second replacement and orphaned the first. The host now derives
it from this caller's oldest /clear since its last commit whose replacement start reached
the operation ledger, so any retry from that caller finishes the same replacement, including
after a restart. A /clear after a committed one starts a new replacement. Another caller's
/clear is refused only while such a replacement is running or not proven stopped. An older
client that resends the same operation id still lands on the same replacement.
…s on an unproven stop

A retry under a new operation id could pick an earlier try whose replacement start had already
failed, replay that failure and commit it again, so a user who had since signed in was told
they were still signed out. Such a try is now skipped, and the retry starts afresh.

Another window's /clear was refused while the first window's leftover replacement was merely
not proven stopped. Nothing but the first window's own retry would settle that, so the refusal
could last until its ledger row expired a day later. It now waits only on a replacement whose
agent is running.
…rted

A retry picked an earlier try whose replacement start never answered, because a crash left
that start unsettled. Replaying it could only repeat "couldn't start" or, with the old agent
unproven, refuse every /clear from that window. Only a start that succeeded left a
conversation to finish; any other try is skipped and the retry starts afresh.
@brennanb2025
brennanb2025 marked this pull request as ready for review September 29, 2026 19:07

@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

Since the prior approval at 8995771d, three commits replaced the per-request replacement derivation with a durable ledger scan, so a /clear retried under a fresh operation id lands on the same replacement an earlier try started. Re-verified against the full PR at 4974af98; the ledger selection and the rewind refusal were each traced independently.

  • clearTryToFinish selects the try to finish — scans listOperationRows() in admission order and returns the caller's oldest clear row since its last completed one whose derived attach row is succeeded, resetting on a succeeded row or the record's committed op. A retry under a new operation id therefore reuses the earlier replacement instead of starting a second one.
  • otherCallersClearIsLive bounds the cross-window wait — refuses a different caller's /clear only while some other caller's replacement has a lease that agentSessionLeaseAdmitsWriter, so a merely unproven stop gates nothing.
  • startedAndAtRest repairs onto a stopped replacement — when re-attach is not ok, the clear commits anyway if the replacement record exists, its lease is released, and the attach row for the derived id succeeded (covers restart and idle sweep).
  • Prior changes still hold — the commit is the clear's only durable write (prepared/unconfirmed records inert), and Codex providerApplied/onReverted are gone with recoverStructuredRewind settling provider-refused whenever phase is prepared.
  • Tests — the 26-case /clear suite plus the rewind, rewind-at-rest, send-block and admission suites pass (79 tests across 6 files); pnpm tc:node passes.

The deferrals in the PR body (orphan-replacement cleanup, no outcome recorded for a refused rewind, the test-only "acknowledged but not kept" guard) remain correctly scoped as non-blocking.

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

@brennanb2025
brennanb2025 changed the base branch from brennanb2025/chat-turn-facts-from-record to main September 29, 2026 20:18
#23059 landed as a squash identical in content to the branch this PR was built
on (273 files). Merged with #23059's branch tip as the base, so only main's other
changes since then come in.
@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 1a88051b-9f61-4d5d-9071-335d2535ee42

📥 Commits

Reviewing files that changed from the base of the PR and between dc4ca58 and c9b008b.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 52a5d285-2ce4-472c-94f2-3f03b40a1221

📥 Commits

Reviewing files that changed from the base of the PR and between d60f999 and dc4ca58.

📒 Files selected for processing (17)
  • src/main/codex/codex-structured-rewind.test.ts
  • src/main/codex/codex-structured-rewind.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-adapter.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-rewind-at-rest.test.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-rewind.test.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-rewind.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-send-block.test.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-send-preparation.ts
  • src/main/native-chat/agent-session-wire/structured-conversation-command-admission.test.ts
  • src/main/native-chat/agent-session-wire/structured-conversation-command-admission.ts
  • src/main/native-chat/agent-session-wire/structured-conversation-command-controller.ts
  • src/main/native-chat/agent-session-wire/structured-conversation-command.test.ts
  • src/main/native-chat/agent-session-wire/structured-conversation-command.ts
  • src/main/native-chat/agent-session-wire/structured-rewind-recovery.ts
  • src/shared/agent-session-refusal-details.ts
  • src/shared/agent-session-refusal-notice.ts
  • src/shared/agent-session-rewind.ts
💤 Files with no reviewable changes (4)
  • src/main/native-chat/agent-session-wire/structured-agent-session-rewind.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-adapter.ts
  • src/shared/agent-session-rewind.ts
  • src/main/codex/codex-structured-rewind.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.


📝 Walkthrough

Walkthrough

Rewind processing no longer uses an onReverted callback to mark provider mutation. Prepared rewind recovery now settles provider-refused outcomes as refused. Clear-command admission no longer blocks on prepared, unknown clear records, and clear retries use replacement-session records and attach outcomes to determine command settlement. Tests cover rewind recovery and clear retries across failures, restarts, and competing callers.

Priority: ➖ Normal

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to dc4ca

The recovery design removes a persistent chat lock and includes safeguards against duplicate visible conversations. One ownership case remains uncertain: a second window can start a replacement when the first replacement’s owner cannot be verified as stopped.

Retained concerns

  • Medium · reliability · inferred: A cross-window clear can start a different replacement when the prior replacement’s owner is indeterminate. The old replacement is not exposed through the visible-session mapping, but termination of its provider child is unproven; this leaves a failure-containment and cleanup question for restarted or remote hosts.
Security review details

Security Blast Radius

  • inferred — The changed admission and recovery behavior acts within native structured sessions and their provider children. The examined paths establish no new cross-tenant, IAM, secret or network authority; the maximum lifetime of an unreferenced provider child remains unproven.

Trust Boundaries and Controls

  • observed — The controller’s changed refusal construction does not remove its pending-session checks. Accepted operations still carry the caller into mutation admission; Codex recovery checks provider history before treating a rewind as applied.

Resilience and Maintainability Implications

  • inferred — Committing visibility only after replacement creation contains an interrupted clear’s user-visible effects. This is not proof that an unreferenced provider child has stopped, particularly when its owner verdict is indeterminate.

Hardening Proposals

  • proposed — Establish and verify a bounded cleanup or reconciliation guarantee for unreferenced replacement children before relying on the indeterminate-owner cross-window retry path.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 13 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely identifies the two main fixes: unfinished /clear operations and refused Codex rewinds no longer lock native chat.
Description check ✅ Passed The description is comprehensive and covers the user impact, behavior changes, implementation approach, rationale, testing, visual proof, compatibility, and known limitations. It references the relate…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@brennanb2025
brennanb2025 merged commit 134a220 into main Sep 29, 2026
36 of 37 checks passed
brennanb2025 added a commit that referenced this pull request Sep 29, 2026
#23524 landed as the squash 134a220, whose change equals the branch
this PR was stacked on, so the merge uses that branch head (c9b008b)
as its base.

Resolved against #23726 (host-owned message queue):
- /clear's draft carry now runs after the commit and opens the new
  conversation only when there are drafts to carry, since the new
  conversation has no transcript until something opens it. It stays
  bookkeeping: a failure, or a crash before it, leaves the drafts on the
  cleared source, where nothing sends them.
- The subscriber hook serves the remembered command list alongside the
  queue publication.
brennanb2025 added a commit that referenced this pull request Sep 29, 2026
…ow is

Three queue tests from #23726 are red on main 29c49ae itself:
- Two expected an older build's unconfirmed ("prepared") /clear to hold
  sends and drafts back. #23524 made such a clear inert, because it
  changed nothing; Send-now and the drain now go past it, like any send.
- One held /clear in flight by holding the new conversation's start,
  which /clear no longer makes. It now holds the one store write, and
  still sees a send refused and no draft left on either conversation.
brennanb2025 added a commit that referenced this pull request Sep 29, 2026
Brings in the host-owned queue for mid-turn messages (#23726) and the /clear and
Codex rewind unlock (#23524), expressed on the one host journal database:

- Queue transactions run through runJournalTransaction on the shared connection,
  and every appended row keeps the draft hooks inside that same transaction.
- The open-time draft repair writes nothing for a chat with no draft, so a chat
  whose first-use copy is owed stays uncopied until its first real write.
- A newer Orca's journal takes no new draft, and Send-now, Delete and Resume on
  it refuse with the same update words a send gets.
- Main's queue tests open the shared database; a table-healing case restarts
  the host around its edit, since the table is ensured when the database opens.
brennanb2025 added a commit that referenced this pull request Sep 29, 2026
…hed clear holds nothing (#23940)

#23726 and #23524 merged minutes apart. #23726's queue gate takes its "blocked"
answer from the shared send check, which #23524 changed: a /clear that never
committed changed nothing the chat reads, so it refuses no send. Three #23726
tests still seeded a "prepared" /clear record and expected it to hold the queue,
and the /clear carry passed a possibly-undefined replacement id where #23726's
helper takes a string, so main failed its unit tests and tc:node.

The gate code already follows the new rule; the tests now say it: a /clear an
older build left prepared holds no draft and Send-now sends it, and a send made
while a /clear is starting its replacement is still told to wait. The carry reads
the replacement id from the ids it already checked.
brennanb2025 added a commit that referenced this pull request Sep 29, 2026
…3789, #23812, #23852) into brennanb2025/codex-home-single-writer

#23744 moves every agent's folder pre-trust into the PTY spawn hook (and
the structured Codex chat create), removing Codex's launch-prep and
resume-prep trust writes. markCodexProjectTrusted now takes the config
files to write.

Conflicts:
- agent-trust-presets.ts: #23744's configFiles signature and nested lane
  acquisition, with this branch's lane rationale (hook installs read and
  rewrite across awaits); main's "capture->restore window" wording
  described the grant snapshot this branch removed.
- agent-trust-write-deadline.ts: the Codex writer waits only on
  managed-home installs' inline grant; the real-home background approval
  holds no lane. #23744's SSH/SFTP writer is gone, so it is no longer
  named; its 1.5 s short deadline is kept.
- codex-trust-config-concurrent-launch.test.ts: this branch's tests
  (a launch's write lands while a grant session runs; the renamed
  sibling-failure case) with #23744's new call signature.

Outside the conflicts: codex-launch-during-real-home-grant.test.ts passed
launchAgent/workspacePath into launch prep, which #23744 removed; its
launch helper now runs launch prep and then applyAgentWorkspaceTrust, as
the spawn builder does, so it still proves a launch and its project trust
land while the real-home approval hangs.
brennanb2025 added a commit that referenced this pull request Sep 30, 2026
Main now settles a Codex send from the turn it was answered into
(#23618, #23679, #23850): an interrupted or failed turn that never
echoed a send withdraws or refuses it, and a completed turn leaves it
to its echo, which Codex records before turn/completed. That replaces
this branch's own mechanism, so every conflict resolves to main:

- journal-level terminal settlement (ownerEndedClientMessageIds,
  multi-row enqueueMany, terminalTurnFences, canResolveDispatch,
  hasTerminalTurn, turn_settled_before_acknowledgement) and the sink's
  onCommitted/onAbandoned/onDiscarded plumbing
- the dispatch-echo ownership model (start candidates, started
  evidence, terminal snapshots) and the late-settlement retry
- the connection's onResult response observer
- the shared-predicate command admission, which main expresses with
  its own fence and handover rules (#23524)

Codex's turn/steer, the part the host-owned queue still needs, is
re-expressed on main's model in the following commit.
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