Repository navigation
fix(native-chat): resolve a conversation command against its terminal frame - #20658
brennanb2025 wants to merge 27 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds a claim-based lifecycle for structured conversation commands. Claims wait for terminal journal frames, track unresolved deadlines, and reset with session fences. Session close and cancellation now use control lanes when a command parks the main lane. Local runtime calls now enforce optional deadlines through a shared helper, while structured conversation commands rely on terminal frames and remote calls still receive forwarded timeouts. Tests cover command claims, control lanes, session close, and runtime deadlines. Priority: ➖ Normal Merge Risk: 🟠 High · up to This change reworks how /compact and /clear commands are tracked and how session close/cancel interact with them. Two credible defects remain: a delayed but eventually successful command reply can leave the UI permanently believing a command is still pending (requiring a restart or manual interrupt to recover), and a failed cancellation can let the app allow a new command while the previous one is still running on the backend, risking overlapping mutations. A minor, previously flagged stale-error display issue also appears to persist. These should be addressed before merging. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 23.08% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 26 functions across 20 files. (1 skipped: 1 unsupported.) Full details: Description checkExplanation The description explains the implementation, rationale, scope, and validation, but it omits most required template sections. It does not include an ELI5 section, a linked issue, visual proof or an explicit N/A, the required testing checklist, AI disclosure, review section, agent skill boundary section, notes, or the final checklist. Resolution Complete the required template sections. Add an ELI5 summary, explain why this approach was chosen, provide the issue reference under “Fixes #”, add before/after visual proof or state N/A with a reason, complete the Testing and checklist items, provide AI disclosure if applicable, complete Review and Agent skill upstream boundary, and document security, cross-platform, SSH, mobile, compatibility, and performance considerations.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 7317bc29-6cb5-48ad-ad1e-f7384b6f3747
📒 Files selected for processing (8)
src/main/native-chat/agent-session-wire/structured-agent-session-host-types.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host.tssrc/main/native-chat/agent-session-wire/structured-agent-session-task-queue.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-task-queue.tssrc/main/runtime/structured-agent-session-runtime.tssrc/renderer/src/runtime/local-runtime-call-deadline.tssrc/renderer/src/runtime/runtime-rpc-client-local-deadline.test.tssrc/renderer/src/runtime/runtime-rpc-client.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The local-deadline fix is sound and correctly scoped, but the new stall watchdog reports into a channel that production never supplies, so the observability half of this PR has no runtime effect. Details inline.
Reviewed changes
- Local RPC deadline applied on desktop. New
callLocalRuntimeWithDeadlineraces the localwindow.api.runtime.callagainst a timer only when the caller declared atimeoutMs, with no default;runtime-rpc-clientroutes its local branch through it. This is the real fix —runtime:callinsrc/main/ipc/runtime.tsnever acceptedtimeoutMs, so the declared 195sconversationCommandguard never reached main. agentSession.senddeliberately stays unbounded. No deadline is passed for send, preserving the delivery-unknown classification. Verified: every othercallRuntimeRpc(..., { timeoutMs })site is gated ontarget.kind === 'environment', soconversationCommandis genuinely the only local-reachable caller that declares one.- Per-session task-queue stall watchdog.
StructuredAgentSessionTaskQueuewraps each serialized task in awatchthat reports once past a 240s threshold (derived from the 180s compaction bound) without cancelling or reordering, exposed asstalledTasks()and wired throughonSessionTaskStalled→deps.onError.
ℹ️ Nitpicks
- The test titled
reports a rejected parked task only for the session that stalledexercises a never-settling task, not a rejected one; the schema and ordering assertions are fine, only the name misleads.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Made the stall report reach a production sink:
onSessionTaskStallednow falls back toconsole.errorwhendeps.onErroris absent, so a parked session is observable on desktop and orcad instead of disappearing into an unset optional dep. This closes the one open finding from the prior review. - Hardened the queue's reporter:
StructuredAgentSessionTaskQueue.reportwrapsonStalledin try/catch, so a throwing reporter cannot escape the timer callback or perturb the parked chain. - Added coverage for both: a queue test asserting a throwing reporter is contained without releasing the chain, and a runtime test asserting the no-reporter fallback logs the scoped error. Both fail with the production lever reverted.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Visual proof / Electron smoke (PR #20658)Hidden-renderer CDP validation of the desktop app. This change is timeout and stall-diagnostic plumbing: screenshots prove the app at this branch renders, not that a deadline or queue watchdog fired. SHAs
The extra commit landed during the run. It is a main-process extract of stall reporting ( Commandsgit rev-parse HEAD
# begin: 5ef096ef6c7e08becd7e36e65a707759d471a7ca
ORCA_BACKGROUND_LAUNCH=1 pnpm test \
src/renderer/src/runtime/runtime-rpc-client-local-deadline.test.ts \
src/main/native-chat/agent-session-wire/structured-agent-session-task-queue.test.ts \
src/main/runtime/structured-agent-session-runtime.test.ts
# 3 files, 31 tests, all passed (also re-run at cf94910a19: 31 passed)
# Isolated throwaway profile + folder workspace only (not the user profile).
ORCA_BACKGROUND_LAUNCH=1 REMOTE_DEBUGGING_PORT=9333 \
ORCA_DEV_USER_DATA_PATH=/Users/brennanbenson/.orca-qa/pr20658-profile \
node config/scripts/run-electron-vite-dev.mjs
# CDP identity before any interaction:
# window.api.app.getIdentity()
# name: Orca: brennanb2025/native-chat-rpc-guard
# isDev: true
# devBranch: brennanb2025/native-chat-rpc-guard
# devWorktreeName: native-chat-rpc-guard
# devRepoRoot: /Users/brennanbenson/orca/workspaces/orca/native-chat-rpc-guard
# CDP target: page "Orca" http://localhost:5173/Launch used ScreenshotsEmpty isolated app (this branch renderer):
Throwaway folder workspace with a live terminal pane (folder workspace, not a git worktree):
New-tab menu showing Claude/Codex/Grok structured-chat launch entries (not launched):
Experimental settings: Chat UI on, default view Chat UI, Use updated structured native chat on:
What was visually verified
What a screenshot cannot prove (gaps)
Tests (vitest, not e2e)At both
31 passed, 0 failed. These are the guards; this Electron pass is rendered-app smoke only. |
Visual proof (anchored to final head)Hidden-renderer CDP recapture after coordinator SHA Exact SHA
Worktree was clean and matched CDP identity (verified before interaction){
"name": "Orca: brennanb2025/native-chat-rpc-guard",
"isDev": true,
"devLabel": "native-chat-rpc-guard",
"devBranch": "brennanb2025/native-chat-rpc-guard",
"devWorktreeName": "native-chat-rpc-guard",
"devRepoRoot": "/Users/brennanbenson/orca/workspaces/orca/native-chat-rpc-guard"
}CDP target: page Commandsgit rev-parse HEAD
# cf94910a19916bc186fc93b73a59e521c966697b
ORCA_BACKGROUND_LAUNCH=1 pnpm test \
src/renderer/src/runtime/runtime-rpc-client-local-deadline.test.ts \
src/main/native-chat/agent-session-wire/structured-agent-session-task-queue.test.ts \
src/main/runtime/structured-agent-session-runtime.test.ts
# 3 files, 31 tests, all passed at this SHA
ORCA_BACKGROUND_LAUNCH=1 REMOTE_DEBUGGING_PORT=9333 \
ORCA_DEV_USER_DATA_PATH=/Users/brennanbenson/.orca-qa/pr20658-profile \
node config/scripts/run-electron-vite-dev.mjsPlaywright CDP only. No computer-use / OS UI automation. No agent session launched. No product files edited or committed. Screenshots (this head)Throwaway folder workspace with Terminal 1:
New-tab menu with Claude/Codex structured-chat launch entries (not launched):
Experimental settings: Chat UI on, default view Chat UI, Use updated structured native chat on:
Visually verified
Not proven visually (gaps)
Tests at
|
Review status: clean at
|
… frame A conversation command runs on the host long after the request that started it, so the reply is a receipt rather than a completion. The client raced the transport promise against a 195s timer, which reported an outcome nobody had established and left no residue: the next attempt silently raced a live first one. The client now holds one claim per session and resolves it against the frame the host revises when the command finishes, carried on the session's own journal stream; the reply is one way that frame can be learned, not the answer. When neither arrives inside the deadline the claim becomes a marker that refuses the next attempt and says the command may still be running, and the marker retires on a late frame, a session restart, or an interrupt. Also: - callRuntimeRpc dropped a declared timeoutMs on the desktop branch while forwarding it on the remote one, so every local caller that asked for a deadline silently got none. The local path now honours it, and stays a synchronous pass-through when no deadline is declared: an extra async frame inserts a microtask that reorders calls the caller sequences. - Cancel already escaped to its own lane while a compaction held the session's mutation lane. close() did not, so closing a session queued behind the very wait that closing ends. Both now take one named control lane, chosen by re-deriving from the durable record whether a command still parks the main lane.
cf94910 to
ebf408c
Compare
brennanb2025
changed the title
There was a problem hiding this comment.
Important
The terminal-frame resolution has two gaps: the host's own "unconfirmed" compaction frame is read as success, and an orphaned /clear has no recovery. Details inline.
Reviewed changes
- Re-keyed user controls onto a control lane. New
structured-agent-session-control-lane.tsre-derives from the durable record whether aprepared/unknownconversation command parks the session's main lane, andclose()pluscancelStructuredAgentSessionTurnnow serialize onsession-control:<id>while it does. - Added a client-side conversation-command claim.
StructuredConversationCommandClaimholds one command per session, settlescompacton the host's terminal journal frame andclearon its committed reply, arms a refusal marker past the 195s deadline, and retires on a late frame, a fence change, or an interrupt. - Rewired the renderer mutation seam.
runConversationCommandnow mints its own operation id and feeds the claim the journal stream;useStructuredAgentSessionMutatetakes{ operationId, onUnresolved }so a host refusal is distinguishable from a missing reply. - Dropped the transport deadline for structured-session commands and deleted the previous revision's stall watchdog and
structured-conversation-command-send.ts.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/native-chat/agent-session-wire/structured-agent-session-host.ts (1)
201-201: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAssert that attach proceeds before resolving the parked command.
runStructuredConversationCommand()holds the mainsessionIdqueue while it awaitscompact(). The current test resolvescompact()throughfinish({})afterclose()and does not attemptattach()before that resolution. It would not detect a regression that leaves the main queue blocked. Startattach()before callingfinish({}), then assert that it completes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: d02c5d9c-1013-4f6e-8ce1-c18ceed21b59
📒 Files selected for processing (15)
src/main/native-chat/agent-session-wire/structured-agent-session-control-lane.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-control-lane.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host-mutations.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host.tssrc/main/native-chat/agent-session-wire/structured-conversation-command.test.tssrc/renderer/src/components/native-chat/structured-conversation-command-claim.test.tssrc/renderer/src/components/native-chat/structured-conversation-command-claim.tssrc/renderer/src/components/native-chat/structured-conversation-command-send.tssrc/renderer/src/components/native-chat/use-structured-agent-session-mutate.tssrc/renderer/src/components/native-chat/use-structured-agent-session.tssrc/renderer/src/components/native-chat/use-structured-conversation-command.tssrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/runtime/local-runtime-call-deadline.tssrc/renderer/src/runtime/runtime-rpc-client-local-deadline.test.tssrc/renderer/src/runtime/structured-agent-session-client.ts
💤 Files with no reviewable changes (1)
- src/renderer/src/components/native-chat/structured-conversation-command-send.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
| const message = error instanceof Error ? error.message : 'Request was not sent' | ||
| if (stateRef.current.fence === targetFence) { | ||
| setWriteError(error instanceof Error ? error.message : 'Request was not sent') | ||
| setWriteError(message) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,110p' src/renderer/src/components/native-chat/use-structured-agent-session-mutate.ts
rg -n "setWriteError|writeError" src/renderer/src/components/native-chat -A3 -B3
sed -n '1,75p' src/renderer/src/components/native-chat/use-structured-conversation-command.tsRepository: stablyai/orca
Length of output: 12586
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- claim implementation ---'
sed -n '1,280p' src/renderer/src/components/native-chat/structured-conversation-command-claim.ts
printf '%s\n' '--- session command and mutation call sites ---'
rg -n "useStructuredConversationCommand|mutate<|mutate\\(|applyStreamSnapshot|writeError|setWriteError" src/renderer/src/components/native-chat -g '*.ts' -g '*.tsx' -A5 -B5Repository: stablyai/orca
Length of output: 32151
Do not persist an error when onUnresolved owns the result.
The transport-error path sets writeError before calling onUnresolved. A later terminal frame settles StructuredConversationCommandClaim through applyStreamSnapshot; it does not call setWriteError(null). Only a later successful mutate call clears the error. The terminal-frame path can therefore leave a stale error visible.
Set writeError only when options?.onUnresolved is absent, or clear it when the claim settles.
Proposed fix
- if (stateRef.current.fence === targetFence) {
+ if (stateRef.current.fence === targetFence && !options?.onUnresolved) {
setWriteError(message)
}…hat-rpc-guard # Conflicts: # src/renderer/src/components/native-chat/use-structured-agent-session-mutate.ts # src/renderer/src/components/native-chat/use-structured-agent-session.ts
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: c9a218f5-5b30-4235-8623-baf7b3c7124a
📒 Files selected for processing (16)
src/main/native-chat/agent-session-wire/structured-agent-session-control-lane.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-control-lane.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host-mutations.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host.tssrc/main/native-chat/agent-session-wire/structured-conversation-command.test.tssrc/renderer/src/components/native-chat/structured-conversation-command-claim.test.tssrc/renderer/src/components/native-chat/structured-conversation-command-claim.tssrc/renderer/src/components/native-chat/structured-conversation-command-send.tssrc/renderer/src/components/native-chat/use-structured-agent-session-mutate.tssrc/renderer/src/components/native-chat/use-structured-agent-session.tssrc/renderer/src/components/native-chat/use-structured-conversation-command.tssrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/runtime/local-runtime-call-deadline.tssrc/renderer/src/runtime/runtime-rpc-client-local-deadline.test.tssrc/renderer/src/runtime/runtime-rpc-client.tssrc/renderer/src/runtime/structured-agent-session-client.ts
💤 Files with no reviewable changes (1)
- src/renderer/src/components/native-chat/structured-conversation-command-send.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/src/i18n/locales/en.json
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
There was a problem hiding this comment.
️ No critical issues — both prior findings are addressed; one minor suggestion inline and one open question below.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review (ebf408c): the current head rebuilds conversation-command ownership on top of the main merge.
- Reconciled live command ownership with the durable record.
requestControlreports whether a pending command actually holds the main lane; only a compact parks it, and a compact admitted before its provider call is now cancelled by close rather than parking close. - Gave
/cleara recovery path. The hook retains a clear's operation id across the deadline and a fence change, and the host adopts a prepared clear left under a previous fence (interruptedClear,rerunWhenReplayMissing,settleSupersededOperation). - Made the claim read the terminal frame, not a timestamp. The host's unconfirmed item is excluded from
terminalFrameOutcome(by text and now by a running lifecycle), and provider failures are carried through instead of reading as success. - Threaded refusal-vs-missing-reply through the mutation seam with
clearWriteError/onReconciled, scoped to the reconciled operation id.
ℹ️ close() now parks behind an in-flight clear
requestControl returns false for any pending clear, so close() runs on the session's main lane and waits for the clear's replacement attach to complete. That ordering is deliberate — the replacement must commit before the source is retired, and it is covered by the new finishes clear before a concurrent close test — but it is the same shape of wait this PR removed for compaction: closing can no longer escape the work it is closing. Raising it only because the invariant stated in structured-agent-session-control-lane.ts is that closing must not queue behind the operation it stops, and for clear the thing it waits on is a provider acquire.
Technical details
# Close blocks on a clear's replacement attach
## Affected sites
- `src/main/native-chat/agent-session-wire/structured-conversation-command-controller.ts:28-44` — `requestControl` returns `false` for a pending `clear` regardless of `providerCallStarted`, and arms no cancellation.
- `src/main/native-chat/agent-session-wire/structured-agent-session-host.ts:194-209` — `close()` therefore serializes on `sessionId`.
- `src/main/native-chat/agent-session-wire/structured-agent-session-acquisition.ts` — no deadline found on the acquire path.
## Open questions for the human
- Is the replacement attach bounded? If it is, close is bounded by it and this is fine; if it can hang, close is unbounded on this path.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No new issues in the two follow-up commits. Both are targeted, tested fixes; posting this as non-approving only because the earlier inline note on the refusal message is still open in its thread.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review (000aa7d).
- Preserved the newest mutation's error.
useStructuredAgentSessionMutatestamps each request with an increasing sequence and only lets the newest settled mutation set or clearwriteError, so a late reply from an older command can no longer wipe a newer mutation's error. - Admitted a cancel before it may interrupt a command.
cancelStructuredAgentSessionTurnpicks its lane from the new read-onlymainLaneParked(so a stale or conflicting cancel no longer takes the control lane) and signals the pending command only inside the admitted plan; the renderer retires a claim only when the host confirmscancelled, so a failed cancel leaves the command's claim intact. - Retired an expired obligation on its late reply. The claim's
applyReplyhands a late definitive reply toonLateReply, which drops the retained operation id so the next attempt gets a fresh identity.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No new issues — a small host/client compatibility alignment.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review (9f3cd91).
- Stopped the unconfirmed compaction row from reading as an active turn. The host now writes
{ kind: 'status', text: 'Compaction completion is unconfirmed.' }with noturnLifecycle, soactiveStructuredAgentSessionTurnIdno longer treats the row as a live turn; the claim still excludes the row by its'Compaction completion is unconfirmed.'text, and the tests now assert the lifecycle is absent.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
Important
The new abandon/claim design has two gaps: a host refusal now permanently retains the operation id, and abandon can orphan a clear replacement session.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review (81ad1c2): 59aaeae replaces the control-lane approach with an execution module that releases the session lane during provider work.
- Removed the control lane and released the session lane during commands.
StructuredConversationCommandExecutionruns the provider call outside the session lane, andclose()/cancelnow callabandon()on the lane instead of taking a separatesession-control:<id>lane. - Split durable admission from execution.
admitAgentSessionMutationRequestadmits and returns arundecision;prepareStructuredConversationCommandpersists the prepared record before any provider work starts. - Gave both commands a typed lifecycle frame.
publishConversationCommandLifecyclewritesrunning/completed/interrupted/unverifiablestatus items under<command>:<opId>, and the claim reads those instead of a wall-clock deadline. - Dropped the transport deadline for conversation commands and deleted
structured-conversation-command-send.tsand the control-lane module.
ℹ️ The PR description no longer matches the implementation
The body still describes the ebf408c revision: it names structured-agent-session-control-lane.ts (deleted), a 195s client claim deadline (removed), and an onUnresolved distinction between a refusal and a missing reply that the code does not actually make (see the inline finding). It also states a clear has no stream frame to wait for, but the host now writes a lifecycle frame for clear too. Worth rewriting so the next reader is not led to the old design.
Technical details
# Stale PR description
## Affected sites
- PR body — "Change" and "Deliberately not done" sections reference the removed control lane, the removed 195s deadline, and the `clear`-has-no-frame claim.
## Required outcome
- The description should describe the current abandon/lifecycle design.ℹ️ Nitpicks
components.native-chat.conversationCommand.mayStillBeRunning(src/renderer/src/i18n/locales/en.json:17384) has no remaining reference — the claim'sunresolvedMessagewas removed in this commit.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No new issues in this delta — the follow-up commit strengthens the abandon/interrupt escape paths. Two threads from the prior review remain open and still apply, which is why this is not an approval.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review (59aaeae): 12e2a7a preserves the conversation-command escape paths once a command is abandoned or interrupted.
- Broadened the host's command guard to every lane mutation except cancel.
mutate()now refusessend,setOption, andrespondToPromptwhile the durable record holds aprepared/unknownconversation command, andcancelonly abandons the command when the provider confirmscancelled. - Gave the compaction latch a confirmed-interrupt path.
StructuredSessionCompaction.interrupted()deletes the pending attempt and rejects it; the Claude and Codex cancel paths call it on a confirmed cancel, and a provider failure that may still settle late now publishes arunninglifecycle instead of retiring the claim. - Settled the client claim on a retired host lifecycle. An
unverifiable(or legacy unconfirmed-text) frame now finishes the claim with restart guidance, and the hook keeps a clear's operation id across a fence change while resetting on a session switch.
ℹ️ The cancel/interrupt settle path is only covered through mocks
Two new mechanisms now meet in one race — a cancel that abandons the command on the session lane, and a provider compaction rejection that tries to publish a running lifecycle — and no test exercises them together. The compaction unit test drives interrupted() directly and the host test mocks adapter.cancelTurn/adapter.compact, so the ordering argument that abandon always wins against the queued markUnknown is reasoned rather than pinned.
Technical details
# Cancel/interrupt integration is untested
## Affected sites
- `src/main/native-chat/agent-session-wire/structured-session-compaction.test.ts:82-95` — drives `interrupted()` against a real tracker, but with no command and no lane.
- `src/main/native-chat/agent-session-wire/structured-conversation-command.test.ts` — mocks `adapter.cancelTurn` and `adapter.compact`, so the real `StructuredSessionCompaction` never participates.
- `src/main/native-chat/agent-session-wire/structured-conversation-command-execution.ts:211-259` — `markUnknown`/`complete` are the other half of the race.
## Required outcome
- One case that runs a `compact` through a real pending `StructuredSessionCompaction` while a cancel abandons the command, asserting exactly one durable `conversationCommand` record, exactly one terminal lifecycle, and a command promise that resolves once.
## Open questions for the human
- Is the lane-ordering guarantee documented anywhere, or should it be a comment on `markUnknown`'s `ownsExecution` guard?DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No new issues in this delta — the two prior pullfrog threads (host refusal permanently retaining the operation id, and an abandoned clear orphaning its replacement) remain open and were not touched by
707d21e, which is why this is not an approval.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review (12e2a7a): 707d21e bounds conversation-command recovery.
- Re-added a client-side completion deadline.
StructuredConversationCommandClaimarms a 195s timer; on expiry the live claim becomes anunresolvedobligation that refuses the next attempt with restart guidance and keeps the operation id, retiring on a matching terminal frame, a late definitive reply, or a session/fence reset. - Settled out-of-lane commands during whole-host teardown. A new
abandon-conversation-commandsteardown phase runsabandonAllover every pending session on its lane, so a command still running when the host tears down resolves instead of hanging. - Bounded a missing provider request receipt.
StructuredSessionCompaction.runnow races the completion window against the invocation, so its 180s timeout applies even when thecompact()call itself never returns, while a late provider notification can still settle it. - Discarded a late mutation result from a previous request scope.
useStructuredAgentSessionMutatenow guards every late settle on arequestScope(target + session) in addition to the fence, and resets the operation ids, error owner, and newest-sequence watermark when the scope changes.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
Important
8316448 makes recovery replay-based, but two paths discard the retained operation id exactly where the UI tells the user to retry, so the promised replay becomes a fresh id the host refuses. Details inline.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review (707d21e): 8316448 makes conversation-command recovery replayable.
- Released interaction at the deadline and kept the operation id as a bounded observer.
StructuredConversationCommandClaimnow hands the id back to the hook viaretrySameOperationso a retry replays the same durable operation, instead of holding a refusing marker with restart guidance. - Typed the mutation reply.
mutateWithDispositiondistinguishescompleted/refused/unresolved; a definitive refusal settles withoutretrySameOperationand releases the id, closing the prior refusal-retains-id finding. - Gated the abandoned clear's replacement close on
canSettle, closing the prior orphaned-replacement finding. - Reworded the recovery copy to "Retry the command to check its status."
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| .then(({ retrySameOperation, ...outcome }) => { | ||
| if (!retrySameOperation && operationIds.current.get(command) === operationId) { | ||
| operationIds.current.delete(command) | ||
| onReconciled(operationId) | ||
| } | ||
| return outcome | ||
| }) |
There was a problem hiding this comment.
A retry after finishUnconfirmed/expire usually arrives while blocked is true: the host's own markUnknown(retire=false) publishes a turnLifecycle: { state: 'running' } item for the unconfirmed compact, so activeStructuredAgentSessionTurnId reports compact:<opId> and blocked is set. StructuredConversationCommandClaim.run then returns the pendingWork outcome before calling send and without retrySameOperation, so this .then deletes the retained id. The next retry mints a fresh id, which the host refuses (conversationCommandBlocked → "The previous conversation operation is unconfirmed.") — even though the user was just told to "Retry the command to check its status."
Technical details
# A blocked retry discards the retained operation id
## Affected sites
- `src/renderer/src/components/native-chat/use-structured-conversation-command.ts:118-124` — deletes the retained id whenever the outcome lacks `retrySameOperation`.
- `src/renderer/src/components/native-chat/structured-conversation-command-claim.ts:112-122` — the `blocked` early return never calls `send` and never sets `retrySameOperation`.
- `src/renderer/src/components/native-chat/use-structured-agent-session.ts:78-83` — `blocked` includes `transportState.turnId`.
- `src/main/native-chat/agent-session-wire/structured-conversation-command-lifecycle.ts:43-55` — every lifecycle frame carries `turnLifecycle`, so a compact that may settle late reads as the active turn.
- `src/main/native-chat/agent-session-wire/structured-agent-session-mutation-admission.ts:93-126` — a same-id replay is resolved before `conversationCommandBlocked` runs, so the host would accept the replay even while its own turn is "running".
## Required outcome
- An unresolved command must keep its operation id across a `blocked` attempt so the eventual retry replays the same durable operation, instead of minting a fresh id the host refuses.
## Suggested approach (optional)
- In `run`, prefer the retained id over the `blocked` gate (attempt the replay and let the host answer), or have the claim report `retrySameOperation` when it short-circuits without sending while an operation is still outstanding.
- The hook harness hardcodes `blocked: false`; add a case that expires a command and then retries while `blocked`.| applyStreamSnapshot(items: readonly AgentJournalRenderItem[]): string[] { | ||
| const settledOperationIds: string[] = [] | ||
| if (this.live) { | ||
| const outcome = terminalFrameOutcome(items, this.live) | ||
| if (outcome) { | ||
| settledOperationIds.push(this.live.operationId) | ||
| this.finish(this.live, outcome) | ||
| } |
There was a problem hiding this comment.
An unverifiable frame settles the command without retrySameOperation, so the hook's stream effect deletes the id — yet the copy tells the user to "Retry the command to check its status." A fresh id is then refused by the host ("The previous conversation operation is unconfirmed."), while the retained id would at least replay the durable unknown outcome.
Technical details
# An unverifiable terminal frame retires the replay id
## Affected sites
- `src/renderer/src/components/native-chat/structured-conversation-command-claim.ts:65-70` — `terminalFrameOutcome` returns a flag-less outcome for `unverifiable`.
- `src/renderer/src/components/native-chat/structured-conversation-command-claim.ts:140-147` — `applyStreamSnapshot` calls `finish` with that outcome; `finish` (224-231) cannot set `retrySameOperation`.
- `src/renderer/src/components/native-chat/use-structured-conversation-command.ts:42-50` — the stream effect deletes every operation id the claim reports settled, and the observer branch drops it the same way.
- `src/main/native-chat/agent-session-wire/structured-conversation-command-admission.ts:20-25` — a fresh command is refused while the durable record is `prepared`/`unknown`.
## Required outcome
- Retain the id on an `unverifiable` terminal frame (settle like `finishUnconfirmed`), or reword the copy so it does not promise a retry the host will refuse.
There was a problem hiding this comment.
ℹ️ No new issues in this delta.
ce54738,67e5164, and799faa1harden command framing and clean up the compaction generation rather than change the recovery contract; the two prior pullfrog threads (ablockedretry discarding the retained id, and anunverifiableframe retiring it) are untouched, which is why this is not an approval.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review (8316448): the three follow-up commits fence stale command state.
- Fenced stale command replies by generation.
StructuredConversationCommandClaimstamps each live claim with a generation, bumps it inreset(), and drops any reply from an earlier generation, so a pre-reset reply can no longer settle a same-operation replay. - Keyed the controller's coalescing on the whole request.
run's pending key now includesexpectedRuntimeFenceandpayloadFingerprint, andsendno longer short-circuits on a pending command, so a legitimate send replay is served from the durable ledger while a mismatched duplicate is refused. - Tightened the clear replacement's authority.
executeClearcommits only when the replacement lease isliveand this host actually holds the session; a bare reservation now surfaces as unverifiable instead of a false commit. - Quarantined an interrupted compaction generation.
StructuredSessionCompactionretains the interrupted generation until the provider's terminal frame, refuses a new compaction withThe interrupted compaction is still settling., andownsTurnno longer reports the interrupted generation as owning its turn.
ℹ️ A quarantined compaction generation has no deadline
StructuredSessionCompaction.run keeps an interrupted generation in pending until the provider's terminal frame, but the 180s timer cannot remove it (its callback only rejects a promise that already rejected) and run's finally clears it anyway, so the quarantine has no time bound. That is fine while the provider eventually emits result/turn/completed (or ended fires), but a provider that confirms the cancel and then goes silent leaves the session unable to compact again — every retry returns The interrupted compaction is still settling. — while hasPending keeps suppressing compaction content from the translator.
Technical details
# Quarantined compaction generation has no expiry
## Affected sites
- `src/main/native-chat/agent-session-wire/structured-session-compaction.ts:62-70` — `interrupt()` rejects the waiter but leaves the generation in `pending`.
- `src/main/native-chat/agent-session-wire/structured-session-compaction.ts:73-101` — the timeout only rejects; the `finally` clears it, and `finish` (the terminal-frame path) is the sole removal.
- `src/main/claude/claude-structured-compaction.ts:46-54` — `hasPending` suppresses compaction content while a generation is retained.
## Open questions for the human
- Once the provider confirms the cancel, is a terminal frame (or `ended`) guaranteed? If not, should the quarantined generation carry its own expiry, or be cleared on re-attach, so a silent provider cannot wedge compaction permanently?DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Run failed. View the logs →
|
There was a problem hiding this comment.
ℹ️ No new issues in this delta —
846fcb7makes transport-level doubt non-terminal so the claim always defers to the host's lifecycle. One informational note inline. The two prior threads (ablockedretry discarding the retained id, and anunverifiableframe retiring it) are untouched by this commit, which is why this is not an approval.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review (799faa1): 846fcb7 makes a conversation command await the remote lifecycle rather than its transport reply.
- Stopped settling a conversation command on transport doubt.
StructuredConversationCommandClaimno longer treats a rejectedsendor anunresolvedreply as a terminal outcome, so completion is decided only by the host's terminal journal frame or the 195s deadline. - Kept the host-confirmed
unknownreply on its old path. A reply whoseresult.stateisunknownstill routes throughfinishUnconfirmed, retaining the operation id for same-operation replay. - Reconciled the transport error on the unresolved path.
useStructuredConversationCommandnow clears the write error as soon as a disposition isunresolved, leaving the claim's own outcome (or the lifecycle frame) to own the messaging. - Updated the guards for the new contract: the claim test now asserts it keeps waiting when the transport returns no reply, and a new case pins same-operation replay for a host-confirmed unknown.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| // Transport doubt cannot settle host-owned work; the session lifecycle or claim deadline does. | ||
| void input.send().then( | ||
| (reply) => this.applyReply(claim, reply), | ||
| () => undefined |
There was a problem hiding this comment.
run no longer lets a rejected send settle the claim, and applyReply no longer settles on an unresolved reply, so a command whose request failed at the transport layer now stays live until the host's terminal frame or the 195s deadline. For a request that never reached the host there is no frame to wait for, and use-structured-conversation-command.ts:98-101 clears the transport error while use-structured-agent-session.ts:104 gates sends on isRunning(), so the user sees neither the error nor a usable composer until the deadline. Confirm this is the intended trade-off for a fast local transport failure (the first hook test now asserts exactly this state).
Technical details
# A transport failure with no lifecycle frame holds the composer for the full deadline
## Affected sites
- `src/renderer/src/components/native-chat/structured-conversation-command-claim.ts:137-139` — a rejected `send` is swallowed; nothing settles the claim early.
- `src/renderer/src/components/native-chat/structured-conversation-command-claim.ts:201-203` — an `unresolved` reply no longer calls `finishUnconfirmed`.
- `src/renderer/src/components/native-chat/use-structured-conversation-command.ts:98-101` — the `unresolved` disposition calls `onReconciled`, clearing the write error `mutateWithDisposition` just set.
- `src/renderer/src/components/native-chat/use-structured-agent-session-mutate.ts:118-129` — a thrown call maps to `{ status: 'unresolved' }` and sets `writeError`.
- `src/renderer/src/components/native-chat/use-structured-agent-session.ts:104` — `send` is gated on `!conversationCommand.isRunning()`.
## Required outcome
- Decide whether a transport-level failure with no durable host record should surface immediately (as before `846fcb7`) or hold interaction for `CONVERSATION_COMMAND_DEADLINE_MS` (195s).
## Open questions for the human
- For a local `window.api.runtime.call` rejection the request may never have reached the host; is the full 195s wait intended there, or should the error and retry affordance return earlier?
- If the wait is intended, should anything tell the user the command is still outstanding rather than merely blocking the composer?
There was a problem hiding this comment.
ℹ️ No new issues in this delta —
258917ais a targeted retirement fix with guards that can fail. No inline comments. The earlier threads (ablockedretry discarding the retained id, anunverifiableframe retiring it) and the transport-failure latency note are untouched by this commit, which is why this is not an approval.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review (846fcb7): 258917a retires interrupted compaction ownership instead of letting it hold a waiter or suppression.
- Resolved a waiter whose execution lost ownership.
markUnknownInLaneno longer returns early when the journal/fence has moved on; if the entry is still current it finishes it with anunknownresult, so a command superseded by provider recovery resolves instead of hanging. The clear error is sliced to 4096 before it is published or returned. - Released an interrupted compaction generation on the next dispatched turn.
StructuredSessionCompaction.claudeTurnStartedfinishes an interrupted generation (and now runs itsonLateResult, which was previously only wired to expiry) when a later Orca-dispatched turn starts, while a/compactreplay keeps transcript suppression. - Wired the Claude release through the observation path.
observeClaudeCompactionnow callsclaudeTurnStartedfor turn-opening messages before deciding whether to translate, and accepts a narrower translator shape. - Added guards: a concurrency case where provider recovery advances the session generation, plus Claude cases for the later-turn release and for keeping a
/compactreplay suppressed.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No new issues in this delta —
e9690a7closes the remaining silent-return holes with guards that can fail. No inline comments. The earlier threads (ablockedretry discarding the retained id, anunverifiableframe retiring it) and the transport-failure latency note are untouched, which is why this is not an approval.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review (258917a): e9690a7 retires stale command completions instead of dropping them.
- Resolved a completion that lost its generation.
completenow callsmarkUnknownInLane(entry, STALE_COMMAND_COMPLETION, true)whencanSettleis false, so a provider result that lands after recovery advanced the fence resolves the waiter asunknownrather than returning silently. - Retired a superseded clear replacement.
executeClearmarks the command unknown before returning when its execution can no longer settle, instead of leaving the waiter to the next attempt. - Added guards for both paths: a compact whose provider result arrives after recovery, and a clear whose replacement attach finishes after recovery, each asserting the old command resolves
unknownand a fresh command under the recovered fence completes.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Visual evidence (commit
|
There was a problem hiding this comment.
ℹ️ No new issues in this delta —
274a9dereleases a provider's compaction state when close or replacement ends the child, and re-checks command currency against the durable lease. No inline comments. The earlier threads (ablockedretry discarding the retained id, anunverifiableframe retiring it) and the transport-failure latency note are untouched by this commit, which is why this is not an approval.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review (e9690a7, with 48dd3fb/f65ac0c read in the same pass): 274a9de retires compaction state when the provider ends and fences command execution on the durable lease.
- Released compaction ownership when the provider ends. The Claude adapter now routes the
closeSession/releaseAcquisitiononEventthrough its ownemit, soobserveClaudeCompactionsees theendedframe; the Codex adapter addedforwardProviderEventto callcompactions.endedbefore forwarding todeps.onEvent, wired into teardown and acquire. - Fenced command execution on the durable lease, not just the command record.
conversationCommandExecutionIsCurrentnow also requiresrecord.lease.runtimeFence === fence, and the extractedretireStaleretires a command whose lease advanced before the provider call or during the event flush. - Extracted
retireStalefrom the four duplicatedcanSettle/markUnknownpairs, adding a check aftercompact()returns. - Guards added: a concurrency case pinning that provider work never starts after the lease advances, plus Claude and Codex cases for compaction ownership releasing on close or replacement.
Both levers were ablated after checkout: removing record.lease.runtimeFence === fence fails the new concurrency case, and reverting either provider's ended routing makes its releases compaction ownership test time out. The store's lease renewal preserves the existing fence, so the new lease check does not trip during normal operation.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Visual evidence (post-merge, current head
|
There was a problem hiding this comment.
ℹ️ No new issues in this delta —
eedf606scopes the transport change back down by deleting the now-vestigial local deadline module. One documentation note below; the three prior threads remain untouched, which is why this is not an approval.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review (c571bc4): eedf606 keeps runtime timeout behavior scoped.
- Deleted the local deadline helper.
local-runtime-call-deadline.tsandruntime-rpc-client-local-deadline.test.tsare gone, so the PR no longer bounds a local call that declarestimeoutMs. - Reverted the local branch to the plain transport call.
runtime-rpc-client.ts:85again callswindow.api.runtime.call({ method, params })directly, restoring the documented contract that the local branch ignorestimeoutMs(use-native-chat-skills.ts:232already supplies a renderer-side backstop for exactly that). - No dangling references. The only exporter and importers of the helper were the two deleted files plus
runtime-rpc-client.ts; a repo-wide grep finds none remaining, and no live test pins a local timeout.
ℹ️ The description still claims the removed transport fix
The PR body's "Problem" item 1, its "Change → Transport (1)" paragraph, its "Tests" list, and two ablation-table rows all describe local-runtime-call-deadline.ts and the runtime-rpc-client.ts deadline forwarding. That code is deleted at head eedf606, so the description now advertises a fix the diff does not contain. Worth trimming so the next reader is not misled about what this PR changes.
Technical details
# The PR body describes a transport change the diff no longer contains
## Affected sites
- PR body, "Problem" item 1 — "A declared deadline that never applied. `callRuntimeRpc` forwarded `timeoutMs` on the remote branch only. The local branch ... dropped it."
- PR body, "Change" → "Transport (1)" — "`local-runtime-call-deadline.ts` applies an explicitly requested `timeoutMs` on the local branch ..."
- PR body, "Tests" — "Guards in `runtime-rpc-client-local-deadline.test.ts` ...", plus the ablation rows keyed on `runtime-rpc-client.ts` (deadline forwarding) and on re-adding the `async` frame to `local-runtime-call-deadline.ts`.
- `src/renderer/src/runtime/local-runtime-call-deadline.ts` and `src/renderer/src/runtime/runtime-rpc-client-local-deadline.test.ts` — deleted at head `eedf606`; neither appears anywhere in the current full diff.
## Required outcome
- Problem/Change/Tests should drop the local-deadline fix (and its guard file and the two ablation rows), since `conversationCommand` already declares no transport deadline and the local branch is back to ignoring `timeoutMs`.
## Open questions for the human
- Is the local `timeoutMs` gap tracked elsewhere? The body's "Deliberately not done" only rules out a default local deadline for `agentSession.send`; it does not note that no local-reachable caller honors a declared `timeoutMs` at all (e.g. `automation-host-client.ts` passes `timeoutMs: 15_000` to a target that can be local).DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Visual evidence (exact final head
|
Final review status: readyReviewed exact head
Remaining coverage gaps are a live SSH run, live Windows interaction, and visual capture of a successful long-running compaction or the 195-second unresolved state. The platform-neutral implementation, focused lifecycle tests, Windows/package CI, and wire-compatibility CI make these non-blocking for this PR. Mobile emulator QA is inapplicable because no mobile UI or protocol surface changed. |
|
Closing as superseded by the newer command/message architecture on main. I reviewed the full production diff, its changed test intents, and the current receipt, provider-terminal and clear paths; this is not based on the old timeout/watchdog comments, branch age, or conflicts.
Evidence is pinned to main The original reliability goal remains valid and is carried by the current durable-message and in-place-clear paths. This is architectural supersession, not a claim that the two implementations have identical internal lifecycle rules. Branch preserved. |
What this changes
Native Chat conversation commands were treated as complete when their request returned. That is the wrong boundary for
/compactand/clear: provider work can continue after transport settlement, so a client deadline could report failure while the original command was still live and allow a retry to race it.The host now owns each command through its terminal provider outcome. It durably admits the operation, releases the serialized session lane before the long provider call, then re-enters under the same session/generation fence to commit the result before publishing it. The renderer observes that lifecycle and can replay the same operation after an unresolved request; it does not create a second provider command.
Implementation
/clearuses an explicit replacement-session lifecycle so the old session cannot publish after replacement.Scope
This PR is limited to structured Native Chat conversation-command ownership, provider correlation, recovery, and the supporting lifecycle tests. It does not change generic local RPC timeout behavior.
Validation
pnpm tcpnpm run check:code-quality:changedwith zero findingspnpm run verify:localization-runtime-catalog