Skip to content

fix(native-chat): tie each rejected message to the failed start that rejected it - #23681

Closed
brennanb2025 wants to merge 43 commits into
mainfrom
brennanb2025/chat-rejection-start-link
Closed

brennanb2025 wants to merge 43 commits into
mainfrom
brennanb2025/chat-rejection-start-link

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 11 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​623 $\color{#cf222e}{\Huge{\mathbf{−}}}$​52 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​571
Prod 18 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​315 $\color{#cf222e}{\Huge{\mathbf{−}}}$​205 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​110

ELI5

When an agent fails to start, the chat shows one error row saying why, and each message that start was meant to carry shows a short "Your message was not sent." instead of repeating the reason. Until now the chat guessed which messages that row explained by comparing the failure they described. This PR records which start rejected each message, so the short notice only appears under the row that actually explains it.

What Changed

Before: a message rejected for some other reason could show only "Your message was not sent." whenever an unrelated start-failure row in the chat happened to state the same kind of failure, so its real reason was only written somewhere far above, about something else. Separately, when a Claude child disappeared after it was published but before it proved its start, each queued message was rejected on its own as "couldn't start", and the child's later exit wrote the start's row as "stopped before it finished starting": the row and the messages disagreed, and there was no row at all until the exit arrived.

After: the short notice appears only on a message the host recorded as rejected by a failed start, and only when that start's row is loaded; every other rejected message keeps its full notice. A start that fails at handover writes one row immediately, and every message it was for (the one handed over and all those queued behind it) is rejected with the same words and the same start.

Mechanism: a rejected message's record gains an optional rejectedByStartKey: the key of the failed start that rejected it, the same key its row is named by (start-failure:<key>). It is saved on the message's rejected delivery record in the chat history, beside the typed reason, and set only by the two writers that write a start's row together with its rejections: the delivery loop's start-failure writer (recordStructuredAgentSessionStartFailure) and the exit settlement for a child that never proved its start. Every other rejection writer sets none. The row itself states only the failure; its key is its name.

Handover: when a dispatch throws while the child is still starting, dispatchSafely now returns that start failure to the delivery loop instead of rejecting the one message. The loop records it through the start-failure writer, which writes the row and rejects the handed message plus everything queued behind it. The deliberate per-message hostFault rejection for a missing or malformed message body is unchanged.

Command handover: a waiting /compact (or another conversation command) whose child cannot take it at handover now goes through the same writer. Before, the handover rejected the command on its own under a separate command-result:<id> row, with no start key, and the loop went on to hand the next queued message to the same child whose start had failed, so a second row could follow. Now the handover returns the failed start to the delivery loop like any message: one start-failure:<key> row, and the command and every message behind it are rejected naming that key. The command's own turn then ends as failed with no second row. A command the provider itself refuses still gets its own result row, since that is the compaction failing, not a start. On screen, for that failure: the chat shows the start's one error row outside the command's turn instead of a result row inside it, the /compact message shows the short "Your message was not sent." instead of repeating the reason, and a message queued behind it is rejected with it instead of being handed to the child that failed.

First report wins: the start's row was already first-writer-wins. Both writers now read the start's row before rejecting; once it exists they reject with the row's own words and fact and leave the row as it is, so the short notice never sits under a row giving a different reason.

Renderer: the desktop pane builds the set of start keys of the loaded start-failure rows (from each row's name) and shows "Your message was not sent." only when the message's rejectedByStartKey is in that set. The fact-equality comparison is removed.

Why

Identity rather than equality: two failures can be equal without being the same event. A malformed-body hostFault can equal an earlier start's hostFault, and two separate starts can fail with identical facts. Only the writer knows which start rejected which message, so it records that link. Deriving it from journal adjacency (sequence or time) would be another guess, since other writes interleave and replays reorder.

Why the start key and not a row sequence: the start key already is the row's identity. It survives replay and dedupe (a second report of the same start lands on the same row), and it is what first-writer-wins keys on, so the link cannot disagree with the row it points to.

Where the link lives: which start rejected a message is a fact about that message's delivery, not about the failure, so it sits on the message's record rather than inside the failure fact, where every reader of a failure (the rejection classifier, turn verdicts, the copy table) would have to know to ignore it. On the row it would only repeat the row's own name, so the row does not carry it. It is saved once, by the writer that decides it, and changes no status, notification or wording.

Mixed versions: rejectedByStartKey is a new optional field on an existing record. A host from before this change writes no key, so this client shows the full notice for its rejections; that includes chats that failed before the update, whose messages now repeat the reason their start's row states. This is the honest fallback, and it slightly widens full-notice use on old chats compared with the equality match it replaces. The submission schema admits the field in any form, and the host drops a malformed one when reading, never the record. Released clients ignore the unknown field, and since the failure facts are unchanged, a client with the earlier fact-equality match keeps matching as before. The desktop outbox's saved copy of a rejection keeps only its kind and attachment problem, so it never stores the key. Mobile hides rejected messages and has no schema of its own for the record, so mobile rendering is unchanged.

Two records: a failed start is recorded as a conversation row, which outlives the queue, and as a rejection on each message it refused, for per-message recovery. The common pattern keeps the same pair: a per-message failure and a conversation-level error. So this is not a deviation. Orca needs both today: the per-message rejection is the delivery state every status reader uses (sidebar verdict, notifications, worktree ps, restart resume, orchestration preambles, send replay). The row is the only visible trace on mobile, which hides rejected messages, for messages this desktop did not send, and for a start with nothing queued, which writes only a row.

One start, one row: the exit of a child that never started now writes the start's row whenever it rejects what the child was handed, keyed by the child's generation, which is how the delivery loop keys the same start. Whichever reports first writes the one row, and the other rejects in its words, so a row and the messages it rejected can no longer disagree. When the exit was handed nothing and messages are still queued, the row stays the delivery loop's, as on main, so a Stop that withdraws them first leaves no row.

Differences from the common pattern

  • Intended: the start that rejected a message is stored on that message (rejectedByStartKey), and one start can reject several messages. The common pattern saves no such link; it pairs a failure with its message by order, from the one writer that drains the queue. That cannot work in Orca: the row and the rejections come from two different writers (the delivery loop and the exit of a child that never started), each rejection is saved as its own record, and one start rejects several messages. The host test that pins the pairing fails on main for that reason. The field is optional, so older hosts and clients are unaffected (see Mixed versions).
  • Temporary: one failed start rejects every message queued behind it, where the common pattern keeps them queued and retries them. That behaviour predates this PR, and this PR applies it on the exit path too, so the two records stay consistent. Replacing it with per-message retry needs a new saved message state; it is planned as its own change (on the review board, no PR yet).

Linked Issue

No linked issue; follow-up to #23608, which introduced the short "not sent" notice this makes exact.

Visual Proof

Live Electron QA ran on a separate Mac against dev builds with isolated profiles. BEFORE is main at fe7e13daf2c; AFTER is this PR at 9037e507cca, with the first flow re-checked at the current head bce2c9aef8e after the start's key moved onto the message's record. A stand-in for the Claude CLI made the agent's start fail on demand. On screen the two builds match in these flows, which is the point: a start that fails with messages queued still reads the cause once on its row and "Your message was not sent." under each message. What changed is underneath: on this PR each rejected message's record names the start that rejected it, equal to the key of the row start-failure:<key>; on main it names none.

A start that fails with two messages queued. One start for both messages; one row states the cause; each message reads "Your message was not sent." with its own Retry. On this PR both rejected messages name the row's start.

Before After
main-startqueue-final.jpg pr-startqueue-final.jpg

Re-check at bce2c9aef8e: the same screen; the row's failure carries no key, and both rejected messages' records carry rejectedByStartKey equal to the row's key.

pr-startqueue-final-recheck.jpg

An earlier failed start, then a second one that rejects two messages. Two start-failure rows with equal causes are loaded. On main the short notice was chosen by comparing causes, which either row satisfies; on this PR both messages name the second row, the start that actually rejected them.

Before After
main-failqueue-final.jpg pr-failqueue-final.jpg

Pressing one message's Retry. Only that message is sent again (the rig runs the real Claude CLI with no sign-in, so it fails as not signed in and its new start writes its own row); the other message keeps its notice.

Before After
main-retry-one.jpg pr-retry-one.jpg

A chat that failed before updating, opened on this PR's build. Its rejections name no start, so each message shows its full reason, which the start's row below them also states. This is the fallback described under Mixed versions; new failures read the short notice again.

pr-opens-main-written-chat.jpg

A /compact and a message behind it, whose start fails (at 81f942c5d78, this PR only). A live regression check of the merged command flow: a chat at rest after an earlier failed start, then /compact and a message sent while the new start is still in progress, and that start stops before it finishes. Result: one new start row reads "Claude stopped before it finished starting. Run /compact again.", the message behind shows "Your message was not sent." with Retry, and the chat's history shows both the /compact and that message rejected with rejectedByStartKey equal to the row's key. This start stops while both messages still wait, a path that already linked them before the command-handover change, so main shows the same screen here; the difference is in the saved history (on main neither message names the start). This build draws a rejected /compact as no bubble of its own, and shows "Conversation operation was not confirmed." above the composer, on main and on this PR alike.

Waiting on the start After the start failed
pr-compact2681g-00-queued.jpg pr-compact2681g-01-final.jpg

Not exercised live: a start that fails at the handover itself, for a message or for a /compact (the child is still held as starting and then throws on what it was handed; this race is covered by the host tests below only, which were red before the fix and turn red again when it is undone), a rejected message whose start row is not in the loaded history (covered by the pane tests), a delivered reply after Retry.

Testing

  • Host test written first and shown red on the previous code: a child whose start settles with no reason and then cannot take the handed message. The old code rejected each message on its own as startFailed, and the later exit wrote the row as providerStartFailed. Now there is one row, both messages carry its fact and rejectedByStartKey: 'generation-2', dispatch runs once, and the exit leaves the row alone.

  • Tests for the exit settlement naming its start, and for a later report of the same start rejecting in the first row's words.

  • Renderer unit and pane tests: short notice only on a start-key match with a loaded row; full notice for an equal failure that names no start (an older host), for a different start, and when the row is not loaded.

  • Mixed-version tests: schema admission of well-formed and malformed keys, the host reader dropping malformed ones, the classifier unchanged by the key, the outbox copy never storing it; the cross-version wire suite passes.

  • Command handover, written first and shown red on the merged code: a waiting /compact whose child cannot take it is rejected with rejectedByStartKey: 'generation-2', the message behind it carries the same key, there is exactly one start row and no command result row, and the command's turn does not stay running. It holds when the exit lands after the loop's report, when the exit lands while the command still waits, and when the child exits after taking the command. Undoing the change turns it red: the command is rejected with no start key.

  • One start, one row, both orders: the exit reported first and the loop first each leave exactly one row, and every rejected message carries the row's words and the same start key. When the exit was handed nothing, the row stays the loop's.

  • Ablations, each turning the tests red: the writer, the history row builder or the reader dropping the key; the reader keeping a malformed one; handover rejecting per message; a later report keeping its own words; the renderer reading the key from the failure fact, skipping the loaded-row check, or going short whenever any start row is loaded; the exit settlement not writing its row, or writing it when it rejected nothing.

  • Targeted vitest (native-chat main, Claude, renderer native-chat, shared), all oxlint configs with --deny-warnings, oxfmt, and the changed-code gate pass. tc:node, tc:web and mobile typecheck pass.

  • The Claude queued-stop and control-actions tests that expect withdrawn messages to carry {kind: 'cancelled'} were updated on main by test(claude): fix queued-send cancellation CI expectations #23675; a real-CLI test depends on the installed Claude CLI and was not run.

  • Tested on macOS only: unit, and live in the Visual Proof above.

  • I manually tested these changes locally

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

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

Security: no new input surface; the key is an opaque string that the reader keeps only when it is a non-empty string. Cross-platform: no platform-specific code. SSH: the key is written by the execution host with its rows and travels with them. Mobile: rendering unchanged. Backwards compatibility: covered under mixed versions above. Performance: the pane collects start keys only while a rejected message exists, and holds the same list while it is unchanged.

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)

…its reason

History, subscribe, snapshot and options reads reach a chat through one accessor, whose open had no
catch: a journal that would not open reached every client as a runtime error carrying the storage's
own text (a path, "file is not a database"). The accessor, and the options read's own open, now throw
the classified journal refusal: journalCorrupt when SQLite reports damage, journalUnavailable
otherwise. The storage text goes to the log only.

The wire code stays runtime_error and the message becomes the bare code, as for every thrown
refusal; the reason rides in the error's data.
…no hand-written reason

The sweep passed an English sentence as the stop's reason. It lived only in memory and nothing read
it: the delivery loop words the error row and the rejection from the hostStopped fact. Dropped, with
the display-name lookup that built it.
…never its message

A thrown agent-session refusal reaches the client as runtime_error with the bare code as its message
and the typed refusal in error.data. Stop, answers, options and goals, a launch's held option pick,
the option picker's failure toast, and the Retry line of a chat that could not start now word it
from that refusal through the shared notice table. What each caller decides about the outcome is
unchanged: only the words move.

The Retry line of a failed start no longer prints the host's message or a thrown error's text; it
keeps the refusal as a fact and says the cause and step its reason names, or only that the chat
could not be started. The option toast keeps a local option surface's own sentence.
…nd damage stops the retry

The structured chat's read failure showed the host's text on the status line, and the pane always
said Orca keeps trying. The read transport now takes the refusal from the error's data (a stream
payload or a thrown RPC error), the reducer keeps it beside the failure text, and the pane and the
status line word it through the notice table, once: on the pane when the failure took it, else
beside the transcript that stays.

A damaged journal says "Unable to load this chat." and the read stops reconnecting for that run;
reopening the chat reads again. An open that can clear names its cause without "Try again", since
the pane retries on its own. A failure that names no reason keeps today's generic line. Finality
comes from the refusal's reason, never its message, which is the bare code for both.
A message the host recorded and then rejected keeps the host's typed fact beside its reason, but
the Retry words re-read the reason alone. Now the fact decides: a hand-over failure says Orca
couldn't reach the agent, a kind whose reason may be a legacy marker gets its fact's own sentence
(a full queue now says so instead of only "not sent"), and any other kind shows the sentence the
host wrote for it, which carries the agent's name and any words the provider wrote for a person. A
row with no fact reads as before.
…s own row

The structured chat showed one Retry strip under the transcript for whichever single entry it
picked, so a second failed message had no reason and no Retry of its own. The terminal-backed
chat's existing per-row delivery marker now carries a notice and an optional Retry, and the
structured pane derives one per message from the outbox on each render: every rejected message,
and the one the queue stopped on (read through the drain's own rule, so a Retry never names a
message waiting behind it). Each is worded from that message's stored failure. The single strip is
deleted. Nothing new is stored, and the shared message projection is untouched.
… already acts

Three wording rules for the desktop chat:

- A chat that could not start shows Retry beside its reason, so the reason stops at its cause
  where the Retry is the step: a reason whose action is to retry, and a start failure's "send
  your message again". Any other step stays (quit the terminal agent, start a new chat). The
  start-failure sentences take a retryControl context for this; what the host writes is unchanged.
- A history that couldn't open right now still reconnects, so the pane keeps "Orca keeps trying
  to load it" under its cause. Only a damaged history, which no retry reads past, drops it.
- A read failure that names no reason while the transcript is shown is only the pane
  reconnecting: the status line says "Reconnecting to this chat…" in muted text, not an error.
  New key components.native-chat.state.reconnecting, hand-translated for es/fr/ja/ko/zh.
… is moving

Each rejected message's row offered its own Retry even while the queue was stopped on another
message. Any Retry clears the stopped queue, so pressing a rejected message's Retry also sent the
message the queue was holding, which the user had not retried; behind a message whose delivery is
unconfirmed, the retried one instead went back into the queue with no notice and waited there.

While the queue is stopped, only the message it stopped on offers Retry, as the single Retry strip
this replaced did. A rejected message keeps its words on its row and gets its Retry back once the
queue moves.
…n every reconnect

A reader reconnects every 750 ms while a journal open can clear, and each attempt logged the
failure with its full stack. The read door now logs a session's failure once until that session
opens, closes, or fails differently; every attempt is still refused with its reason.
…tice stops at the cause

A rejected message offering Retry read "Claude stopped before it finished starting. Send your
message to try again." beside that button. Its row now takes the rule the launch strip already
follows: beside its own Retry the words leave out sending or trying again, worded from the stored
fact with the chat's agent name. The stored fact keeps less than the host wrote from (a refusal,
the provider's words), so a reason it cannot rebuild exactly is kept as written. A rejected
message without a Retry, while the queue is held, keeps the step. What the host writes and the
phone's notice are unchanged.
…e one the queue is held on

Retry released the queue's refusal hold whichever message it was pressed on. While a queued message waited ahead of a held one, every rejected message offered Retry, and pressing it also sent the held message the person had not retried. Retrying an unconfirmed message ahead of a held one did the same. Retry now releases the hold only for its own message.
…sign in first

Beside a Retry the notice dropped the whole next step, so a chat that could not start because the agent was not signed in read only the cause. Pressing Retry without signing in fails the same way again. The words now keep the sign-in step and leave out only the resend, which the Retry button is.
…ep the same way

A message the queue stopped on worded its refusal with no agent name and with its retry step, beside its own Retry, while a rejected message next to it named the agent and left the step to the button. The launch strip and the history pane each had their own copy of the same rule. One wording context now goes through the one notice table for every surface: a Retry beside the words, or a pane that reconnects on its own, is the step for a reason whose action is to retry, and every other step stays. What the phone and the host write is unchanged.
… fact, never by comparing sentences

A message the host recorded and then rejected kept only the rejection's kind on the message, so its notice was reworded from that smaller copy only when it rebuilt the host's sentence word for word. A different agent name, an older host's wording, or anything the copy dropped (why a start failed, the provider's own words) left the host's sentence in place, beside a Retry that repeated its resend step. The notice now reads the journal's own rejection for that message, found by id, with the pane's agent name and Retry, and shows the provider's words only when they were written for a person. The message's smaller copy words it only when that journal row is not loaded. Nothing new is stored.
… new id the first time

Whether a Retry needed a new message id was remembered in memory for one message, or read from the journal row when it was loaded. After a restart, or for an older message whose row was not loaded, the first Retry resent under the old id, the host answered with the same settled rejection, and nothing visibly happened. The message now says so itself: one the host recorded and rejected always retries under a new id, including after a restart. A refusal that already gave the message a fresh id, and a message whose delivery is unconfirmed or in flight, keep their id as before.
…eps the provider's words

When the journal row that rejected a message is not loaded, the message's own copy of the
fact has no provider detail or start refusal. For the kinds worded from those, the row now
shows the sentence the host wrote for the person instead of a thinner rebuilt one.
…aded journal row

Nothing covered the pane handing the journal's rows to the per-message notices, so a pane that stopped passing them would quietly fall back to the message's smaller copy of the rejection and show the host's sentence, resend step and all. The new case renders the pane with a rejected message whose journal row is loaded and checks that it reads that row's refusal in the chat's own agent name.
A read the host refused for a named reason put its sentence under the generic
"Could not load conversation" title, so a damaged history read as two lines
saying the same thing. The pane's own sentence now takes the title's place; a
history that can come back keeps its line saying Orca keeps trying. A failure
that names nothing keeps the generic title.
… was not sent

When an agent stopped before it finished starting, the chat showed the start's
red row ("Claude stopped before it finished starting. Send your message to try
again.") and then repeated that cause under every message the start rejected.
Each of those messages now reads "Your message was not sent." beside its Retry.

The match is made on typed facts, not on the words: the host writes the start's
row and the rejection of its queued messages from the same failure fact, and the
row is keyed by the start. The pane finds the loaded start-failure rows by that
key and shortens a message's notice only when its loaded journal submission was
rejected with the same fact. Any other rejection, or one whose submission or row
is not loaded, keeps its full notice. The row key moves to a shared module so the
host that writes it and the pane that reads it use one definition.
…less often

A read the host kept refusing (its history store could not be opened right now)
reopened every 750 ms for as long as the chat stayed open, about 40 opens every
30 seconds. Each reconnect now waits twice as long as the last, from 750 ms up
to 30 seconds, and never gives up; the first read that delivers anything starts
the wait over at 750 ms. A damaged history still stops reconnecting at once.

Reset happens on a delivered read, not on connect: a local subscribe resolves
before the host's open refuses, so resetting there would keep the 750 ms loop.
The read retry doubles its wait up to 30 s during an outage, and nothing
reset it when the remote runtime reconnected, so the transcript could
lag the reconnect by up to 30 s. The read now watches the runtime
status store's contact-regained edges (hostContactEpoch for a
same-runtime return, connectionGeneration for a new runtime session)
and, when one lands, runs a waiting retry immediately with the wait
reset to its base.
A failure fact gains an optional startKey: the key of the start-failure row whose start rejected
the message. Readers keep it when it is a non-empty string and drop it otherwise; the classifier,
the copy table and the outbox's saved copy ignore it, and the journal schema admits it however a
host wrote it, so a row with or without one reads as before.
…name that start

The one start-failure writer already keyed its row by the start and rejected every queued message
with the same words; it now also puts that key on the fact it writes to both, so a reader can tell
which row explains a rejected message without comparing the failures.
… for every message

When the child the delivery loop waited on had not proven its start and could not take the message
handed to it, each message was rejected on its own as a start that failed, and the child's later
exit wrote the start's row as the provider stopping: the row and the messages disagreed, and no
row existed until the exit. The handover now returns that start failure to the loop, whose one
start-failure writer writes the row and rejects the handed message and every one queued behind it
with the same words and start key.
…row is loaded

The short notice applied whenever a loaded start-failure row stated a failure equal to the
message's, so a rejection no start wrote could borrow an unrelated start's row. It now applies only
when the rejection names its start and that start's row is loaded; a rejection from an older host,
which names none, keeps its full notice.
…'s words

The start's row is first-writer-wins, but a second report of the same start rejected its messages
in its own words under the same start key, so the short notice could sit under a row giving a
different reason. Both writers of a start's row and its paired rejections, the delivery loop's and
the exit settlement's, now read the start's row: once it is written they reject with its words and
fact and leave it as it is; otherwise they write it. The exit settlement also names the start on
the row and the messages it rejects, with the key that forms the row's identity.
…esktop' into brennanb2025/chat-rejection-start-link
…ection-start-link

# Conflicts:
#	src/main/native-chat/agent-session-wire/structured-agent-session-start-failure-row.ts
#	src/renderer/src/components/native-chat/NativeChatStructuredSession.start-failure-notice.test.tsx
#	src/renderer/src/components/native-chat/NativeChatStructuredSession.tsx
#	src/renderer/src/components/native-chat/structured-agent-session-delivery-notices.test.ts
#	src/renderer/src/components/native-chat/structured-agent-session-delivery-notices.ts
#	src/shared/structured-agent-session-start-failure-row-key.ts
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Review status

Where it stands: draft, ready for the owner's call. The pushed head 9037e507cca is green in CI and merges cleanly. No code changes came out of this review.

Review loops: one loop, clean, with no changes. The reviewer checked:

  • Every rejection writer. Only the two writers that write a start's row together with its rejections set startKey, and it equals the row's own key.
  • The handover path. It writes one row, rejects the handed message and everything queued behind it with the same words and key, and dispatches once.
  • First report wins. There is no race: the exit settlement and the delivery loop run in one per-session queue.
  • The renderer match. The short notice needs a key that matches a loaded row.
  • Status readers (sidebar, notifications, worktree ps, restart resume, orchestration). None of them compares whole failure facts, so the key changes none of them.
  • Mixed versions and the per-render cost.

Tests: 311 tests in 25 files passed. Loosening the renderer match turns the tests red. Lint and the changed-code gate are clean.

Readiness checks: no code blockers at the start of the review. At the end, all items pass.

Live check on a separate Mac, main against this PR, with screenshots in the description:

  • A start that fails with two queued messages: both builds show the same screen. One row gives the cause, and each message reads "Your message was not sent." with its own Retry. On this PR both saved rejections carry the row's start key.
  • Two loaded start rows with equal causes: on this PR, both messages name the start that rejected them.
  • Retry on one message sends only that message.

Behaviour to know about: a chat that failed before the update shows the full reason under each message, because those rejections name no start. This is the stated fallback for rejections without a key.

Remaining gaps, disclosed:

  • The exit settlement can reject messages before the delivery loop writes the row. In practice both word the same exit the same way.
  • If a child has no generation, the two writers can key the same start differently. The only effect is the full notice instead of the short one.
  • Not exercised live: a start that fails at the handover (covered by a host test), and a delivered reply after Retry.

… on its own record

The start that rejected a message is who rejected it, a delivery fact, not part of the failure. It
moves off the failure fact onto the submission as `rejectedByStartKey`, persisted on the rejected
dispatch row beside `rejection`; the start's row stays keyed by it and states only the failure.

The exit of a child that never started now writes the start's row whenever it rejects what the
child was handed, keyed by the child's generation as the delivery loop keys it, so the row and
those messages never wait on the loop to agree.
…rt named

Also says in the exit settlement's input that it keys a failed start as the delivery loop does.
…ry loop when it was handed nothing

A start that rejected nothing it was handed has only queued messages, which the delivery loop reports; the exit writes no row for them.
…ild's generation only

An exit is only settled for the child it names, and that event always carries the child's
generation, so the settlement never met a start without one. Drop the fallback key and the
condition and test for that case; the delivery loop keys the same start by the same
generation.
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Review status (update)

Where it stands: draft at bce2c9aef8e, pushed. Typechecks pass: node, web and mobile at 2d4235123f1, and node at bce2c9aef8e; the later commits change only src/main.

What changed since the last status:

  • Where the start is recorded. The start that rejected a message is now recorded on the message's own record, as rejectedByStartKey on its rejected delivery entry in the chat history. Before, it was a field inside the failure fact. Which start rejected a message is about that message's delivery, not about the failure, so it no longer sits where every reader of a failure had to ignore it. The start-failure row states only the failure, and its key is its name. The failure fact is back to its shape on main.
  • The earlier known gap is closed. When a child that never started exits, it now writes the start's row whenever it rejects what it was handed. It keys the row by the child's generation, the same way the delivery loop does. Whichever reports first writes the one row, and the other rejects in its words. Tests cover both orders.
  • Removed an unreachable path. The exit always carries a generation, because its only caller matches the event's generation to the child's. So the fallback key for a missing generation, and the test for it, were removed.

Review loops (three):

  1. Clean.
  2. After the change: no bugs. Added a test for the rule that the exit leaves the row to the delivery loop when it was handed nothing.
  3. Clean, apart from the removal of the unreachable path.

The whole diff was reviewed each time. The native-chat, renderer and shared suites pass (447 files), the cross-version wire suite passes, ablations of each change turn their tests red, and lint and the changed-code gate are clean.

Live check on a separate Mac at bce2c9aef8e, for a start that fails with two queued messages:

  • The screen is the same as before: one row gives the reason, and each message reads "Your message was not sent." with its own Retry.
  • In the chat's history, the row's failure carries no key, and both rejected messages carry the row's key.

Known behaviour: a chat that failed before the update shows the full reason under each message, because those rejections name no start. This is the stated fallback.

…ection-start-link

# Conflicts:
#	src/main/native-chat/agent-session-journal/journal-store-contracts.ts
#	src/main/native-chat/agent-session-wire/structured-agent-session-delivery-loop.ts
#	src/main/native-chat/agent-session-wire/structured-agent-session-start-failure-row.ts
#	src/main/native-chat/agent-session-wire/structured-agent-session-turns.ts
#	src/shared/agent-session-journal-schemas.ts
… any message

The command handover rejected a waiting /compact with the start-failure words but no start key,
under its own result row. It now returns the failed start to the delivery loop, which records the
start's one row and rejects the command and every message behind it with that row's key; the
command's turn then ends failed with no second row.
@brennanb2025
brennanb2025 marked this pull request as ready for review September 30, 2026 02:22
@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

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

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: edd2b166-905b-4183-acb7-95eafc2b2005

📥 Commits

Reviewing files that changed from the base of the PR and between 5b3366f and f8ad84c.

📒 Files selected for processing (29)
  • src/main/native-chat/agent-session-journal/journal-dispatch-reducer.ts
  • src/main/native-chat/agent-session-journal/journal-pending-submission-recovery.ts
  • src/main/native-chat/agent-session-journal/journal-reducer-rejection-fact.test.ts
  • src/main/native-chat/agent-session-journal/journal-row-builders.ts
  • src/main/native-chat/agent-session-journal/journal-row-schema.ts
  • src/main/native-chat/agent-session-journal/journal-store-contracts.ts
  • src/main/native-chat/agent-session-journal/journal-store.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-accept-then-deliver.test.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-command-turn.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-dead-generation-settlement.test.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-dead-generation-settlement.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-delivery-loop.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-provider-child-record.test.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-start-failure-row.test.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-start-failure-row.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-start-failure-writer.test.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-turns.ts
  • src/renderer/src/components/native-chat/NativeChatStructuredSession.start-failure-notice.test.tsx
  • src/renderer/src/components/native-chat/NativeChatStructuredSession.tsx
  • src/renderer/src/components/native-chat/structured-agent-session-delivery-notices.test.ts
  • src/renderer/src/components/native-chat/structured-agent-session-delivery-notices.ts
  • src/renderer/src/components/native-chat/use-structured-agent-session-start-failure-facts.ts
  • src/renderer/src/components/native-chat/use-structured-agent-session-start-failure-keys.test.ts
  • src/renderer/src/components/native-chat/use-structured-agent-session-start-failure-keys.ts
  • src/shared/agent-session-journal-schemas.test.ts
  • src/shared/agent-session-journal-schemas.ts
  • src/shared/agent-session-journal-types.ts
  • src/shared/structured-agent-session-send-disposition.test.ts
  • src/shared/structured-agent-session-start-failure-row-key.ts
💤 Files with no reviewable changes (1)
  • src/renderer/src/components/native-chat/use-structured-agent-session-start-failure-facts.ts

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


📝 Walkthrough

Walkthrough

Rejected submissions can now record the start key associated with a failed start. Host settlement writes and reuses start-failure rows while rejecting queued or handed-over submissions, and settles affected command turns. The renderer uses matching keys from loaded status rows to choose delivery-notice text. Journal parsing and submission validation preserve rows when start-key values are malformed.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to f8ad8

The change links rejected messages to their specific failed start while preserving full notices for older or unmatched records. No actionable merge-blocking issue remains; merge after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to f8ad8

The change makes failure attribution more precise without giving messages or agents additional permissions. The main remaining uncertainty concerns recovery when saving the failure and finishing the associated work are interrupted partway through.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The traced mutation scope is one session journal: the handed-over submission and queued submissions associated with its failed start. The renderer's downstream effect is notice wording, not increased agent or tool authority. Complete external caller exposure was not independently enumerated.

Trust Boundaries and Controls

  • observed — The shared schema's permissive metadata acceptance is not an authorization control. The host reducer removes unusable associations, and the renderer requires a rejected submission plus a matching loaded start-row identity before shortening its notice. Retry availability continues to derive from outbox admission rather than this key.

Resilience and Maintainability Implications

  • observed — Individual journal writes are transactional, but failure-row creation, dispatch rejection, queued rejection and command-turn completion are separate operations. The base already separated dispatch rejection from command completion. Pending recovery preserves delivery uncertainty rather than returning handed-over work to the queue, and dead-generation settlement revises running turns; the evidence does not prove identical command outcomes after every interruption point.
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 40 functions across 28 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description is detailed and covers the required ELI5, changes, rationale, testing, visual proof, compatibility, and checklist sections. However, the required Linked Issue section states “No linked… Add the actual linked issue under “Linked Issue” and replace “No linked issue” with the required issue reference. Confirm the final CI checklist item if the full commands were run or CI will cover them.
✅ Passed checks (3 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title clearly and concisely identifies the main change: associating each rejected message with the failed start that caused the rejection.
Full details: Description check

Explanation

The description is detailed and covers the required ELI5, changes, rationale, testing, visual proof, compatibility, and checklist sections. However, the required Linked Issue section states “No linked issue” instead of providing an actual issue link.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • 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.

@greptile-apps

greptile-apps Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Medium risk] Refactors how rejected messages track their failed start.

The PR appears safe to merge; no actionable new issue was identified.

Summary

The PR records which failed start rejected each message, uses that identity to choose the desktop notice, and brings message and command handover failures through the shared start-failure writer. The changes since the previous review merge unrelated updates from main; they do not change the PR’s native-chat implementation.

Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Failed agent start] --> B[Start-failure row keyed by start]
  A --> C[Rejected message records start key]
  B --> D[Loaded chat history]
  C --> E[Desktop delivery notice]
  D --> E
  E --> F[Short notice only when keys match]
Loading

Reviews (3) · Last reviewed commit: "Merge remote-tracking branch 'origin/mai..."

@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

  • Rejection identity on the message record — a rejected submission now carries rejectedByStartKey, the start key its start-failure:<key> row is named by; the dispatch row, reducer, and submission schema (z.unknown(), malformed dropped by the reader) were threaded for it. The renderer's short "Your message was not sent." notice is now gated on that key matching a loaded start-failure row, replacing the fact-equality match (sameAgentSessionFailureFact and the facts hook removed).
  • Handover records the start once — dispatchSafely / handOverSubmission return the start failure to the delivery loop instead of rejecting the single message inline; the loop's fail writes one start-failure:<generation> row and rejects the handed message plus everything queued behind it.
  • Command handover — a /compact whose child cannot take it returns the failure the same way; the start's row is the command's one row and endStructuredAgentSessionCommandStartFailure ends its turn, so no command-result: row and no second handover to the failed child.
  • Exit settlement — keys the start by the child's generation, reuses an already-written row's words (first-writer-wins), writes the row when it rejected a handed message or nothing is queued, else leaves it to the loop.
  • Shared schema refactor — TurnRecordFields extracted; no behavior change.

Verified the touched suites locally: start-failure row/writer, dead-generation settlement, reducer rejection fact, renderer notices/keys, accept-then-deliver, provider-child-record, schemas, send-disposition, and the pane start-failure notice all pass. I read the complete diff and the surrounding handover, dead-generation settlement, reducer, and renderer code; the identity-based pairing is coherent, the two writers agree on one row in both orders, and the mixed-version fallback is honest.

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

…ection-start-link

# Conflicts:
#	src/main/native-chat/agent-session-wire/structured-agent-session-start-failure-writer.test.ts

@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

Changes since the prior pullfrog review (81f942c5d78) are the merge of origin/main (5b3366f78e) into the branch. No PR-authored code changed; the delta is main's code arriving in five files the PR touches, and the merge resolution keeps the branch's identity-based pairing intact.

  • Merged origin/main into the branch — the branch's rejectedByStartKey pairing, handover-to-loop recording, and command-start-failure path are unchanged; the merge only folds in main's code where the two branches touched the same files.
  • Renderer notices compose cleanly — main's attemptedAcrossStop guard now precedes the start-key short-notice check in deliveryNoticeText, so an interrupted send across a Stop still reads "delivery is unconfirmed" and does not fall through to the start-key branch.
  • Journal/roster plumbing from main — JournalItemLinkageVisitor / visitItemsWithLinkage and AgentJournalProducerLinkage arrive alongside the PR's JournalDispatchRejectionInput, rejectedByStartKey, and structuredAgentSessionStartFailureRejection without interaction.
  • Pane gains main's queued-message cards — queueFollowUps, NativeChatQueuedMessageList, and steerQueued are woven into NativeChatStructuredSession.tsx around the existing useStructuredAgentSessionStartFailureKeys wiring.

Re-ran the renderer delivery-notice and start-failure-notice suites plus the dead-generation-settlement, start-failure-writer, and accept-then-deliver suites (49 tests, all pass) and pnpm tc (clean) at the merged head. The merge is a pure sync; no new concerns.

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

@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

Changes since the prior pullfrog review (f8ad84ca7f) are a merge of origin/main (7afa4ee3dc) into the branch. No PR-authored code changed: the delta is main's code arriving, and it leaves every file this PR touches untouched.

  • Merged origin/main into the branch — the branch's rejectedByStartKey pairing, handover-to-loop recording, command-start-failure path, and renderer start-key gating are byte-for-byte unchanged (git diff f8ad84ca7f..0795638b78 over the PR's paths is empty).
  • No interacting upstream changes — main changed no native-chat rejection file, journal schema, or journal type between 5b3366f78e and 7afa4ee3dc, so the sync cannot shadow or conflict with this PR's identity-based pairing.

Verified at the merged head: pnpm tc is clean and the ten touched suites (149 tests) pass.

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

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Superseded by #24340. Brennan asked for this to be rebuilt on the common design: the delivery loop is the only writer of a failed start, the failure is recorded on the message that was waiting for it, and messages sent after it go ahead while it retries on a bounded schedule (15 s, 1 min, 5 min, then not sent with Retry). That removes this PR's shared start row, its stored start link and its second writer, and it closes the gap where a message already handed to an agent whose start never finished could end up unconfirmed. Its QA and review history are kept here for reference.

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