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>
…dev#11465) ## 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.md` and `specs/QUALITY-726/TECH.md` for the full design. User-visible changes: - A manually shared local orchestrator now drives orchestration discovery in its viewer's pill bar (previously the gate matched only `AmbientAgent`, so manually-shared local orchestrators got no pill bar). - Local children of a manually-shared local orchestrator inherit the share automatically, and the share's children pane shows them. Children that existed before the share started are caught up via a one-shot cascade. Stopping the host's share cascades to those children. - Local-to-driver children of a remote orchestrator now surface in the parent's pill bar — the local client reports the freshly-minted `session_id` back to warp-server on share so the viewer's REST descendant fetch can locate it. - Agent-id references inside conversation bodies resolve to the correct display name on first poll (previously the viewer-side index was never populated, so references rendered as "Unknown"). - Non-orchestrator user shares stop polling after their first empty descendant fetch and only resume on the next `AppendedExchange` on the orchestrator. Wire-format notes: - `SessionSourceType::User` stays a strict unit variant on the wire. A new `source_task_id: Option<String>` sidecar travels alongside `source_type` on `InitPayload` and `JoinedSuccessfully` (and the server's `SessionManifest`), carrying the conversation's `ai_tasks` row id when the user shares an orchestrator. This avoids the wire-incompatibility an earlier iteration introduced when it turned `User` into a struct variant. - A new `BlocklistAIHistoryEvent::LocalSharedSessionEstablished` lifecycle event drives a sidecar model that links the local share's session id to its `ai_tasks` row via `update_agent_task`. ## Demos - local → local: https://www.loom.com/share/7cab1a115706404c9869a41d28dc1efd - local → remote: https://www.loom.com/share/e06c7d4da56f4923b9304f94e4982a29 - remote → local: https://www.loom.com/share/57ae334e3d5f494393709ac87606755b - remote → remote: https://www.loom.com/share/b66e778549a14108be6d9d440be1b3cf ## Known issues / follow-up - **Child-link sibling resolution deferred.** Opening a *child* agent's share link directly does not yet resolve sibling agent IDs in the child transcript — they fall back to the existing "Unknown agent" placeholder. The original design fetched siblings on join and registered them via `start_new_conversation`, which polluted `live_conversation_ids_for_terminal_view`, emitted `StartedNewConversation` events 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 on `BlocklistAIHistoryModel` that doesn't go through `start_new_conversation`. - **Pre-`StreamInit` share with concurrent viewer join.** If the user shares a brand-new local conversation before the first response (so `task_id` is still `None`) and a viewer joins in that window, the sharer's later `StreamInit` upgrade mutates only the local `TerminalModel.shared_session_source.source_task_id`. Already-joined viewers received `source_task_id: None` on `JoinedSuccessfully` and have no push channel for the upgraded value, so their `OrchestrationViewerModel` never 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→viewer `DownstreamMessage::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. - **Catch-up cascade dispatch test coverage.** The `inherit_share_for_local_child` rule itself has tests; the pane-group dispatch loop does not yet. ## Related PRs (must land together) - session-sharing-protocol wire-format change: warpdotdev/session-sharing-protocol#15 - session-sharing-server vendored-protocol mirror: warpdotdev/session-sharing-server#449 - warp-server session-link gate relaxation: warpdotdev/warp-server#11372 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 on `main`. ### Agent Mode - [x] This PR was created by Warp Agent Mode Co-Authored-By: Oz <oz-agent@warp.dev> --------- 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