fix(desktop): bind Side Conversation events to Host admission - #3716
fix(desktop): bind Side Conversation events to Host admission#3716Astro-Han wants to merge 29 commits into
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.
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
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 |
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.