Repository navigation
daemon: command-response replies return staged cancel-queued images - #348
Conversation
|
I'll fix CI failures and address comments from users with write access that start with 'Devin'.
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 45 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (15)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. Summary by CodeRabbit
WalkthroughThe change adds request-correlated command replies to the daemon protocol and clients. Queued-message cancellation can return staged text and images. The UI uses those responses and session-scoped composer inserts to restore queued message content. ChangesCommand replies and queued-message editing
Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant UI
participant RemoteDaemon
participant DaemonServer
participant DaemonCore
UI->>RemoteDaemon: Send correlated cancellation request
RemoteDaemon->>DaemonServer: Forward request envelope
DaemonServer->>DaemonCore: Dispatch cancellation
DaemonCore-->>DaemonServer: Return cancelled message content
DaemonServer-->>RemoteDaemon: Send request-correlated reply
RemoteDaemon-->>UI: Deliver command result
UI->>UI: Restore composer content for the matching session
Merge Risk: ⚪ Minimal · up to The queued-message restoration changes are mergeable after normal checks. Session-scoped replies survive switching sessions; the annotation-routing concern is pre-existing and unchanged. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Replies remain confined to the requesting connection and preserve the existing authentication checks. The main concern is client/daemon version skew: an older daemon rejects the new edit request while the client removes the queued row, potentially leaving recalled work eligible to run. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. A rabbit sends a request down the wire, Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 747efcc764
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Good catch — fixed in |
|
Fixed in |
|
Fixed in On the client, Also bounded the wait while I was in there: |
|
On the retained security-review concerns:
|
Summary
Follow-up to #346: the wire had no way to return a command's result, so a remote
cancel-queuedrestored only the staged text (recovered from the optimistic echo) and silently dropped staged images. This adds a point-to-point request/reply channel soCancelQueuedMessagecan deliver the full staged payload back to the caller, plus a journaled recovery event so a disconnect can't eat the answer.Wire (
threadlane-protocol) — new frame shapes alongside the existing ones:CommandRequest { request_id, command }— aSessionCommandsent as{"request_id": N, "command": {...}}instead of bare. Disjoint from a bare command by construction (no top-leveltypetag), and a protocol test asserts neither shape deserializes as the other.CommandReply { request_id, result: Result<CommandResponse, String> }— sent as{"response": ...}on the requesting connection only: never journaled, never broadcast, so?since=replays don't re-deliver it.CommandResponseis the payload:Ack(every command that returns nothing) andCancelledQueuedMessage { session_id, entry_id, text, images }.SessionEvent::QueuedEntryCancelled { session_id, entry_id, request_id, text, images }— journaled on every successful cancel with the same payload: the recovery path when a reply is lost to a disconnect (a replayed tail re-delivers it and the requester correlates byrequest_id), and how every client learns the entry went away.SessionEvent::CommandResult { request_id, result }— synthesized by client transports for the requester; never appears in a journal tail.Daemon —
DaemonCore::dispatchreturnsResult<CommandResponse, String>(DaemonError broadcast unchanged);dispatch_with_request_idadditionally threads the caller'srequest_idinto the journaledQueuedEntryCancelled.dispatch_innershort-circuitsCancelQueuedMessagetowork_handle.cancel_queued_entry, mapping the staged(text, images)into the response; all other commands funnel throughdispatch_effect→Ack.server.rstries theCommandRequestparse first; on success it dispatches and sends the{"response"}frame back down the same connection's ordered outbound channel (reply_tx, a clone ofout_tx).Client —
DaemonClientgainscommand_request(CommandRequest) -> Result<CommandResponse, String>; caller-chosenrequest_idcorrelates the reply.LocalDaemonjust returns the dispatch result.RemoteDaemonkeysrequest_id → PendingRequestinpending_requests: a reconnect fails every waiter exceptreplayableones (journaled-payload commands), which get one reconnect for the tail replay to resolve them viaQueuedEntryCancelled; a 30s timeout fails callers whose reply will never arrive. Replies and journaled recovery both resolve the same waiter.App —
AppStategains anext_command_request_idcounter andpending_queued_restores: HashMap<u64, PendingQueuedRestore>. The queued row's edit button now callsedit_queued_message(new sibling ofcancel_queued_message, which keeps fire-and-forget for plain removal): it parks the intent, issues the cancel as aCommandRequest, and the reply re-enters viastream_txasCommandResult— handled insidedrain_chat_streamlike every other daemon outcome. Either aCommandResultor a journaledQueuedEntryCancelledresolves the intent throughapply_queued_cancel_restore:requested_composer_insertsgains a session-scoped insert (RequestedComposerInsert.session_id) that lands in the composer once that session is active — retained across a session switch rather than dropped or delivered to a foreign composer (text only if the echo hadn't already restored it).Validation
cargo check -p threadlane-gpuiclean;git diff --checkclean.cargo nextest run -p threadlane-protocol -p threadlane-client -p threadlane-daemon -p threadlane-ui-state: 154/154 pass, including newcommand_request_and_reply_round_trip_through_json(shape disjointness + all reply variants),remote_command_request_round_trips(real server: dispatch error resolves the caller'sErrAND still broadcastsDaemonError;GetProjectState→Ok(Ack)),remote_edit_queued_message_reply_survives_a_session_switch, andremote_edit_queued_message_recovers_via_journaled_cancel_event(journal-tail recovery path).cargo nextest run -p threadlane-ui-chat: 123/130 — the 7 failures (composer_at_completion_escape_dismisses_and_keeps_query,queued_panel_tracks_active_messages_and_generation, etc.) fail identically onmainin this environment.Link to Devin session: https://app.devin.ai/sessions/e1a52aee996148b6a500b6a489bcb5fa
Open in Devin Desktop: https://app.devin.ai/desktop/session/e1a52aee996148b6a500b6a489bcb5fa?variant=devin
Requested by: @wheregmis