fix(desktop): bind Side Conversation events to Host admission - #3716
Conversation
41aa14c to
f51928a
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found blocking issues.
[P2] Queued steer Stop retracts the queued turn but drops UI ownership of the still-running turn
use-quote-companion.ts:312-320,723-758 with runtime-host-session-execution-ipc-main.ts:816-840. When a Side Conversation owns active A and steered S is still queued, Stop retracts only S without interrupting A. The renderer restores A but abandonAdmission() clears turnInFlight, so later A deltas render while the Composer appears non-streaming.
[P2] Ordinary Desktop observation now requires a catalog preflight that may not recover
runtime-host-session-execution-ipc-main.ts:192-206 with preload.ts:1722-1775. If Host reconnect interrupts getSession before registry registration, no observation is registered and live events are lost until remount. Previously sessions:observe registered directly.
Additional standards note: UI changes should include before/after visual evidence per CONTRIBUTING.md:85.
Checks on 364dd1a96 are test: success but mergeable: dirty — needs rebase.
简体中文
存在两项状态机/观测阻断,需修复后再审。695ddfa to
e896f46
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found a blocking issue.
[P2] Queued Stop succeeds on the Host but the retraction event is lost before observation
use-quote-companion.ts:719-736 keeps the admission pending after a queued Stop, waiting for message_admission(retracted). The Main path confirms and awaits the exact queue retraction, then returns void (runtime-host-session-execution-ipc-main.ts:822-829). The projector recovery seed only replays current admissions, so with an empty queue no terminal fact is projected and replacement does not compare old/new queues. The UI stays pending with processing=true, blocking further sends/steers, and stopRequestRef suppresses future Stops.
Fix: have sessions:stop return a typed retracted | interrupted(turnId) outcome and consume it, or persist a replayable terminal admission fact; cover queued send/steer → Stop success → dropped live retraction → empty-queue reseed.
Checks on e896f46483 are test: success.
简体中文
该头仍有排队停止后身份丢失导致的永久 pending。|
Addressed in 1a5d92d. The failure was at the existing Host command boundary: Main had already awaited the exact queued-entry retraction, but sessions:stop erased that fact into a void reply, leaving the renderer dependent on a lossy live message_admission(retracted) event. The Stop reply now carries only the confirmed queued retraction identity ({ kind: retracted, messageId }). The Side Conversation hook releases admission only when that identity matches its current pending message. A rejected Stop still means unknown and preserves admission; live admission events remain the idempotent observation path for other observers. No durable terminal state or second authority was added. Regression coverage now drops the live retraction entirely for queued send and queued steer, verifies the admission clears, preserves the active Host Turn streaming state, and verifies the next Stop targets that active Turn. Local verification: Desktop typecheck plus 46 focused Side Conversation, Runtime Host IPC, Workbar adapter, and WorkHub adapter tests. Exact-head CI is running. |
M4n5ter
left a comment
There was a problem hiding this comment.
English
I reviewed exact head 1a5d92d87229f873bf0f540da29f3e8eaf2c37e5 and found one blocking issue: a Host-retracted busy-raced send can still permanently consume its staged quotes even though the send returns false and keeps the rest of the draft for retry.
The exact-head test check is green. I also ran 238 focused Runtime Host, runtime-mapper, Desktop execution/observation, and Side Conversation ownership tests; the existing suite passes because its retraction probes use no staged quotes. The inline comment includes a production-hook reproduction and the repair direction.
简体中文
我审查了精确提交 1a5d92d87229f873bf0f540da29f3e8eaf2c37e5,发现一项阻断问题:Host 撤回忙碌竞态中的发送后,发送会返回 false 并保留草稿的其他部分,但已暂存的引用仍会被永久消费。
当前提交的 test 检查为绿色。我还运行了 238 项 Runtime Host、runtime mapper、Desktop 执行/观测与 Side Conversation 所有权相关测试;现有测试会通过,是因为撤回探针没有携带已暂存引用。行内评论包含生产 Hook 复现与修复方向。
jackwener
left a comment
There was a problem hiding this comment.
I reviewed exact head 6dc0af38a512ca97c903da95793118e1c1566cc2 and found two P1 blockers and one P2 reconnect defect.
[P1] Re-resolve the admission against the final Stop snapshot
runtime-host-session-execution-ipc-main.ts:819-862 can acknowledge Stop without stopping the Turn that now owns the ticket. The first snapshot can bind expectedAdmissionId to Turn A, then production beforeStop() awaits Computer Use resource release. If the Host assigns the same ticket to successor Turn B during that await, the final snapshot sees B and returns undefined; no interrupt is sent. The renderer treats every fulfilled pending-admission Stop as confirmed and leaves its Stop request fence set unless it receives an explicit retraction, so B keeps running and later Stop clicks are ignored until settlement or panel recreation.
I reproduced this through the registered production IPC handler: beforeStop changed A/in-flight to B/running with the ticket in rootTurnSourceMessageIds; the call fulfilled with undefined, B remained running, and interruptTurn was called zero times. Existing tests use static snapshots and cannot hit the ownership change inside the await. After the final snapshot, resolve the same admission ticket again. If it now belongs to B, interrupt B; if disposition cannot be proven, reject as unknown rather than reporting a successful no-op.
[P1] Do not publish a revocable in-flight lease as final Turn ownership
session-projector.ts:514 treats every transition into in_flight as message_admission(ticket, A), but that lease is still revocable. The backend can fail attachment materialization, abort before durable consumption, or lose its consumer, and drainSteeringInto() then nacks it back to queued. The renderer has already consumed its sole pending ticket and bound A; the projector emits no retraction for in-flight -> queued, and when the ticket is later folded into successor B, the second admission is ignored because no pending ticket remains. B's live answer is therefore invisible, and on this head staged quotes are also consumed at the false A admission.
A production Hook probe performed A start -> ticket admission A -> A completion -> the same ticket admitted to B plus B text. The final live Turn id stayed empty and B's text never rendered. Current tests cover queued -> in-flight and queued -> successor independently, not in-flight -> nack -> queued -> successor. Publish final ownership only after durable ack/root-source admission, or keep ticket ownership rebindable until the Host's durable admission is known. Stop must likewise distinguish a revocable lease from durable Turn ownership.
[P2] Retry a dispatched queued retraction with the same identity
At runtime-host-session-execution-ipc-main.ts:825-832, queued Stop calls retractQueueEntry directly even though this PR adds idempotent reconnect retry for Side Conversation start/submit. If the Host commits the retract and the connection closes before the reply, RuntimeHostRequestInterruptedError(dispatch='dispatched') is propagated. The renderer correctly keeps the admission unknown, but the replacement seed cannot replay the retraction because the ticket is now absent; the panel remains processing, and a retry cannot find the deleted ticket. Recreating the panel is the only recovery.
A production-handler probe recorded the first retraction and then threw a dispatched interruption. The handler rejected immediately and made no reconnect/retry, while retrying with the original id would be safe. Generate retractId once, wait for reconnect, and retry the exact operation with that same identity; add the dispatched-retract plus dropped-live-event case.
The current head does close the previous staged-quote retraction issue, and the compatibility epoch/strict snapshot decoding are correct. I materialized the current-main merge ref c0f73f9cd27bddf0a1ac8bb25d0e2c8694e3f860: the full repository and Desktop renderer build passed, along with 143 focused merge-result tests. Those green checks do not exercise the three interleavings above. Exact-head hosted test is currently queued.
6dc0af3 to
738c33d
Compare
|
Addressed the three findings from the exact-head The common root was that Desktop treated the Host queue's revocable
Regression coverage includes revocable lease -> no admission, live durable transcript -> exact admission, admission Stop racing A -> B replacement, same-ID dispatched retraction retry, and production Hook/Workbar Stop identity behavior. Validation after rebasing onto current
CI has been triggered for |
jackwener
left a comment
There was a problem hiding this comment.
I re-reviewed exact head 738c33d08f96bda88dc48b7b348a16b4cfe3b15e. All three findings from 6dc0af38a are resolved, and I found no new P0–P3 issue in the repair delta.
- A revocable
in_flightlease no longer publishes or reseeds final message ownership. Admission now comes from the existing durable authorities:rootTurnSourceMessageIdsor a persisted steering transcript message with its exact Turn identity. - Stop now carries a typed admission-vs-Turn target. If an admission moves while
beforeStop()is awaiting teardown, the operation rejects as outcome-unknown instead of acknowledging a no-op or interrupting the replacement Turn; the renderer keeps the admission retryable. - A dispatched queued retraction reconnects and retries with the same
retractId, preserving the Host's idempotency identity.
I verified the rebased history with git range-diff: the first 27 commits are patch-equivalent to the previously reviewed series, followed only by the two repair commits. The affected Core, Runtime, Runtime Host, UI, CLI, and Desktop builds passed, as did 190 focused projector, observer, protocol, runtime-mapper, execution-IPC, production Side Conversation hook, and Workbar adapter tests. Scoped Biome and git diff --check also passed. Current main is the PR base, and the materialized merge tree is identical to the reviewed head tree.
The hosted test check for this exact head is still queued, so this is a correctness GO rather than an approval; approval still waits for that required check to complete successfully.
M4n5ter
left a comment
There was a problem hiding this comment.
English
I re-reviewed exact head 738c33d08f96bda88dc48b7b348a16b4cfe3b15e and found no remaining P0–P3 issues.
The staged-quote regression is closed: quotes are now consumed only when the exact Host admission binds a Turn, while a retracted or confirmed-stopped ticket returns false without consuming its snapshot. I verified the regression test by temporarily restoring the early-consumption behavior; it failed with one unexpected consumption, then passed again with this head.
I also verified the current-head ownership repairs: a revocable in_flight lease no longer becomes final Turn ownership, Stop preserves whether its target is an admission or a Turn, a replacement Turn cannot be interrupted by a stale admission Stop, and a dispatched queued retraction retries with the same idempotency identity.
The exact-head test check completed successfully. Runtime Host, Desktop main/preload/renderer builds and 91 affected projector, observer, execution IPC, Workbar, and production Hook tests passed locally. The only review thread is resolved, there are no stale approvals, and current main has no changed-path overlap with this PR; the merge tree is clean.
简体中文
我重新审查了精确提交 738c33d08f96bda88dc48b7b348a16b4cfe3b15e,没有发现仍未解决的 P0–P3 问题。
暂存引用丢失问题已经闭合:引用只会在精确的 Host admission 绑定 Turn 后消费;ticket 被撤回或确认停止时,发送返回 false 且保留引用快照。我临时恢复了提前消费行为验证回归测试,测试会以一次意外消费失败;恢复当前实现后重新通过。
我也验证了当前提交的所有权修复:可撤销的 in_flight lease 不再成为最终 Turn 所有权;Stop 会保留目标是 admission 还是 Turn;过期 admission Stop 不会中断替换后的 Turn;已派发的排队撤回会使用同一个幂等身份重试。
精确提交的 test 检查已成功完成。Runtime Host、Desktop main/preload/renderer 构建以及 91 项 projector、observer、execution IPC、Workbar 和生产 Hook 相关测试均在本地通过。唯一的 review thread 已解决,没有过期 approval;当前 main 与该 PR 没有变更路径重叠,merge tree 干净。
738c33d to
1afbd33
Compare
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Derive admission observation from the Host-owned Session mode and keep ordinary Session streams unchanged. Remove the duplicate renderer send intent and tighten continuity decoding. Generated-by: Codex
Keep Session event listeners bound to the current validated Host target epoch instead of filtering replacement events through the original host identity. Generated-by: Codex
Reconcile buffered Host admission events when Stop outcome is unknown and bind successor/retraction fixtures to the actual submitted ticket. Generated-by: Codex
Follow replacement target epochs within the owning profile while rejecting same-named Session channels from other Hosts. Generated-by: Codex
Assert that replaying admission and text after an unknown Stop restores the real Host Turn streaming projection. Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
1afbd33 to
6f9608c
Compare
Summary
message_admissionfact; unrelated terminal, steering, and queue events never infer ownership.No durable message schema, settlement/recovery policy, CLI projection, or Desktop primary-conversation projection is changed.
Review follow-up
retractedevent instead of treating a successful Stop RPC as global idle. The still-running Turn remains owned and interruptible, and a second Stop can target it.message_admissionfact as successor admission; observation reseeding replays that fact while the in-flight queue entry remains authoritative.UserMessage; no renderer recovery state or new protocol field is needed.sessions:observeis registered as one reconnectable read operation, so Runtime Host replacement retries catalog preflight and observer registration together.turnInFlight=falseclosure.Behavior evidence
The production-shaped React hook + Workbar seam covers:
Protocol boundary
A full Host replacement can discard an old in-memory queued ticket before it reaches either an active Turn or a durable successor admission. Current Host facts can then prove neither
admittednorretracted; the existing submit contract intentionally reportsoutcome_unknown. This PR preserves that ticket instead of inventing cancellation. Tickets that reached an active Turn are recovered from the durable steering message. Fully settling the never-admitted queue-only case requires a prior Host protocol fact for the durable terminal disposition of a queued admission; it is outside this PR's storage/recovery scope.Verification
git diff --checkExact pushed head:
e896f4648, rebased onto currentorigin/main. Exact-head CI is green, including Desktop e2e, Runtime Host, Browser/Storybook smoke, alignment, and installed CLI candidate validation. Earlier pre-rebase runs reached 64/66 Desktop e2e tests before the slash-command menu DOM-stability assertion failed; the rebase removed that failure and the full five-test Slash menu spec also passes locally.AI use
Codex contributed the implementation, regression tests, verification, and adversarial/simplification review. No commit from Draft PR #3633 was cherry-picked or copied; its old branch/review was used only as failure evidence.