QUALITY-726: session sharing for orchestrated agent sessions#11465
Conversation
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR extends shared-session orchestration discovery across local/remote parent and child topologies, adds server-side session-id linking for local shared sessions, and adds specs/tests for the new behavior.
Concerns
- The committed
Cargo.tomllocal path patch makes the branch unreproducible outside a sibling checkout and overrides the pinned protocol revision. - A manual share that starts before the conversation task id exists only upgrades the local model source type; existing viewers remain on
User { task_id: None }and never start orchestration discovery. - Child-link sibling preload creates normal live conversations for siblings, which can expose sibling navigation/metadata in a view that is supposed to stay child-scoped.
Verdict
Found: 0 critical, 3 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| # on `main`; patch to the sibling local worktree until the upstream PR lands. | ||
| # Remove this table once that happens. | ||
| [patch."https://github.com/warpdotdev/session-sharing-protocol"] | ||
| session-sharing-protocol = { path = "../session-sharing-protocol" } |
There was a problem hiding this comment.
../session-sharing-protocol, and it overrides the pinned git rev. Remove the patch and commit the upstream protocol rev once the protocol PR lands before merging.
There was a problem hiding this comment.
Will fix when protocol PR lands
| return; | ||
| }; | ||
|
|
||
| model |
There was a problem hiding this comment.
TerminalModel leaves already-joined viewers with User { task_id: None }, so a share opened before StreamInit never constructs OrchestrationViewerModel after the task id arrives. Propagate this source-type upgrade to active viewers or avoid advertising the pre-StreamInit path as parent-scoped until reconnect.
There was a problem hiding this comment.
Narrow blast radius - will follow up.
| // Standalone conversation: there's no local parent on a | ||
| // child-link viewer, so we don't try to wire one up. The | ||
| // conversation is purely a name-resolution placeholder. | ||
| let conversation_id = history.start_new_conversation( |
There was a problem hiding this comment.
start_new_conversation registers each sibling in live_conversation_ids_for_terminal_view and emits StartedNewConversation, so a child-link viewer can gain real sibling conversations even though child links must stay scoped to the joined child. Populate the agent-id/name index with metadata-only placeholders or otherwise keep these sibling entries non-navigable.
There was a problem hiding this comment.
fixed - by removing the sibling related code for now
## What
Adds a `source_task_id: Option<String>` sidecar field to
`sharer::InitPayload` and
`viewer::DownstreamMessage::JoinedSuccessfully`.
`SessionSourceType::User` stays a strict unit variant (no struct
fields), matching `main`. `AmbientAgent { task_id }` is unchanged.
Both new fields are gated on `#[serde(default)]` so older clients and
viewers ignore them silently. The sidecar carries the conversation's
server-side `ai_tasks` row id so downstream orchestration discovery can
enumerate descendants without re-shaping the variant.
## Why
Required by the warp client to support session sharing of
*manually-shared* local orchestrator conversations.
## Related
- session-sharing-server vendored-protocol mirror:
warpdotdev/session-sharing-server#449
- warp client: warpdotdev/warp#11465
Co-Authored-By: Oz <oz-agent@warp.dev>
---------
Co-authored-by: Oz <oz-agent@warp.dev>
advait-m
left a comment
There was a problem hiding this comment.
nice, the cascade approach makes sense - cool to see it working across all the permutations!
| }); | ||
| } | ||
|
|
||
| // Suppress unused-import warning on wasm builds where the helper |
There was a problem hiding this comment.
nit: does this mean non-wasm? though, in general, not sure if I see why this or the use crate::ai::blocklist::orchestration_topology; import is needed.
| return; | ||
| }; | ||
| let Some(host_source) = | ||
| crate::pane_group::pane::terminal_pane::host_terminal_shared_session_source_type( |
|
|
||
| // Emit for a conversation that was never registered: the subscriber | ||
| // must early-return without firing an RPC. | ||
| let bogus_conversation_id = crate::ai::agent::conversation::AIConversationId::new(); |
| ## Parallelization | ||
| The four threads touch largely independent code areas. They run as parallel local sub-agents with separate worktrees once Thread A0 lands, then merge into a single PR (or 1-PR-per-thread). The strict sequencing is: A0 (protocol crate) → A/B/C/D in parallel. |
There was a problem hiding this comment.
This was cool to read - it sounds like the parallelization worked well in practice? @cephalonaut
There was a problem hiding this comment.
I think it went pretty well! Would definitely be interested in feedback if you try it yourself.
Address Oz auto-review concern #1 on PR #11465: the temporary `[patch."https://github.com/warpdotdev/session-sharing-protocol"]` override pointing at `../session-sharing-protocol` made the branch unbuildable outside a sibling checkout and shadowed the pinned git rev. QUALITY-726 has now landed on session-sharing-protocol `main` as 5a0ad6135809feee9da2e9efae8bd6b54b89172e (PR #15). This commit: - Bumps the workspace `session-sharing-protocol` dep to that rev. - Removes both `[patch]` tables that overrode the upstream URL with the local sibling worktree. Co-Authored-By: Oz <oz-agent@warp.dev>
…al and remote topologies Net effect of the QUALITY-726 integration branch, squashed. - Threads a `source_task_id` orchestrator-task-id sidecar through the shared-session protocol (`InitPayload`, `JoinedSuccessfully`, `SessionManifest`) and through the warp client so: - Manually-shared local orchestrator panes carry a task id when one is available, enabling orchestration discovery for their viewers. - Local-spawned children inherit the host's share with the child's task id stamped on the sidecar. - Bundles `source_type` + `source_task_id` into a single `SharedSessionSource` carried on `IsSharedSessionCreator::Yes`, `SharedSessionStatus::SharePendingPreBootstrap`, and `TerminalModel`. - Adds a catch-up cascade so child agent panes that existed before the parent started sharing also get auto-shared, via `BlocklistAIHistoryEvent::LocalSharedSessionEstablished` and `transitively_share_existing_local_children`. Stops auto-shared children when the host stops sharing. - Adds a driver-side session-id link: `LocalSharedSessionLinkModel` pings `update_agent_task` with the new `session_id` for local orchestrator conversations so the warp-server can surface the joinable link. - Drops a per-conversation dedupe `HashSet` from `LocalSharedSessionLinkModel` (server is now the source of truth). - Pins `session-sharing-protocol` to the merged main rev 5a0ad61 (PR #15) — removed the temporary `[patch]` override to the sibling worktree. - Hoists `host_terminal_shared_session_source_type` and `inherit_share_for_local_child` to a top-of-file cfg-gated `use` in `pane_group/mod.rs` and removes a dead `orchestration_topology` import + suppression line. Adds `specs/QUALITY-726/PRODUCT.md` and `TECH.md` documenting the design and parallelization approach (Threads A/B/C/D ran as parallel sub-agents after the protocol crate landed). - Pre-StreamInit viewer-join edge case: `source_type` upgrade on the host only mutates the local `TerminalModel`; viewers that joined before `StreamInit` stay on `User { task_id: None }` until reconnect. Tracked as a follow-up. - `updateAgentConversationIDQuery` still gates strictly on `state=RUNNING` server-side; same-class latent bug as the session-link case. To be addressed by the planned follow-up that starts local executions in RUNNING. Co-Authored-By: Oz <oz-agent@warp.dev>
0136abb to
83ea458
Compare
When the sharer builds historical scrollback for a late-joining viewer, `reconstruct_response_events_from_conversations` was unconditionally emitting a Finished event after every exchange, including ones still in-flight. The viewer would take its `current_response_id` slot on the replayed Finished and then drop every live ClientAction arriving for the same still-running stream — the conversation appeared stuck on the initial prompt until the next request started. Gate the synthetic Finished on `exchange.output_status.is_finished()` so in-flight exchanges end the scrollback with their accumulated messages only; the live wire's real Finished closes the stream naturally. Co-Authored-By: Oz <oz-agent@warp.dev>
`BlocklistAIHistoryEvent` (pane_group/mod.rs) and `SharedSessionSource` (pane_group/pane/terminal_pane.rs) are only consumed inside `#[cfg(not(target_family = "wasm"))]` blocks, so the wasm build flags them as unused. `PaneGroup::stop_transitively_shared_child_shares` is only invoked from a non-wasm dispatch arm, so the wasm build flags it as dead. Gate each with the matching cfg attribute to keep the wasm clippy build warning-clean. Co-Authored-By: Oz <oz-agent@warp.dev>
97d5d3f to
e23b5e9
Compare
The rebase ran the workspace's default rustfmt against several files whose import blocks then drifted from master's organization, producing 800+ lines of import-only churn in the PR diff. Re-formatting these four files with rustfmt's stable form (`imports_granularity=Module`, `group_imports=StdExternalCrate`) restores their original master-side import layout. No semantic changes — diff vs master shrinks by ~800 lines on these four files alone: app/src/terminal/view.rs 863 → 38 lines app/src/terminal/model/terminal_model.rs 166 → 46 lines app/src/terminal/local_tty/terminal_manager.rs 238 → 119 lines app/src/workspace/view_tests.rs 101 → 13 lines Co-Authored-By: Oz <oz-agent@warp.dev>
What
Makes the orchestration pill bar work for shared-session viewers across every orchestrator/child topology (local-local, local-remote, remote-local, remote-remote × share-parent / share-child). See
specs/QUALITY-726/PRODUCT.mdandspecs/QUALITY-726/TECH.mdfor the full design.User-visible changes:
AmbientAgent, so manually-shared local orchestrators got no pill bar).session_idback to warp-server on share so the viewer's REST descendant fetch can locate it.AppendedExchangeon the orchestrator.Wire-format notes:
SessionSourceType::Userstays a strict unit variant on the wire. A newsource_task_id: Option<String>sidecar travels alongsidesource_typeonInitPayloadandJoinedSuccessfully(and the server'sSessionManifest), carrying the conversation'sai_tasksrow id when the user shares an orchestrator. This avoids the wire-incompatibility an earlier iteration introduced when it turnedUserinto a struct variant.BlocklistAIHistoryEvent::LocalSharedSessionEstablishedlifecycle event drives a sidecar model that links the local share's session id to itsai_tasksrow viaupdate_agent_task.Demos
Known issues / follow-up
start_new_conversation, which pollutedlive_conversation_ids_for_terminal_view, emittedStartedNewConversationevents that cascaded into session-restoration persistence, and exposed siblings as navigable conversations in pickers. Stripped from this PR; a follow-up should add a name-only resolution path onBlocklistAIHistoryModelthat doesn't go throughstart_new_conversation.StreamInitshare with concurrent viewer join. If the user shares a brand-new local conversation before the first response (sotask_idis stillNone) and a viewer joins in that window, the sharer's laterStreamInitupgrade mutates only the localTerminalModel.shared_session_source.source_task_id. Already-joined viewers receivedsource_task_id: NoneonJoinedSuccessfullyand have no push channel for the upgraded value, so theirOrchestrationViewerModelnever constructs and they never see the pill bar for that share. Workaround: the affected viewer can rejoin to pick up the upgraded value. Follow-up: add a sharer→viewerDownstreamMessage::UpdateSourceTaskId(or piggyback on an existing channel) and re-run viewer-side orchestration-discovery construction on receipt. Narrow blast radius — common-case shares and late-joining viewers are unaffected.inherit_share_for_local_childrule itself has tests; the pane-group dispatch loop does not yet.Related PRs (must land together)
The
Cargo.toml[patch]override in this PR currently points the protocol crate at a local sibling worktree; remove the override and bump the rev pin once warpdotdev/session-sharing-protocol#15 lands onmain.Agent Mode
Co-Authored-By: Oz oz-agent@warp.dev