diff --git a/Packages/macOS/CmuxNext/Sources/CmuxNextAgentPane/AgentPaneTransport+Check.swift b/Packages/macOS/CmuxNext/Sources/CmuxNextAgentPane/AgentPaneTransport+Check.swift index 344848abfca2..742b503a84db 100644 --- a/Packages/macOS/CmuxNext/Sources/CmuxNextAgentPane/AgentPaneTransport+Check.swift +++ b/Packages/macOS/CmuxNext/Sources/CmuxNextAgentPane/AgentPaneTransport+Check.swift @@ -62,8 +62,10 @@ extension AgentPaneTransport { } /// What the rules that need no main-actor state make of one frame: refused (a ticket in it, if - /// any, is to be spent), or the checked frame and its facts. No socket and no request ids, so the - /// shared parity cases run it (cmux-agent-pane-policy `check_frame`, tests/cases/check.json). + /// any, is to be spent), or the checked frame and its facts. No socket and no request ids. The + /// Rust copy of these rules (cmux-agent-pane-policy `check_frame`) runs the cases in its + /// tests/cases/check.json; no Swift test reads that file, so a rule here needs its own Swift + /// test (the transport tests, for example `AgentPaneForkRelayTests`). nonisolated enum Checked: Sendable { case refuse(AcpmuxPaneMethods.Decision, spend: String?) case frame(Facts, FrameBox) diff --git a/Packages/macOS/CmuxNext/Tests/CmuxNextAgentPaneTests/AgentPaneForkRelayTests.swift b/Packages/macOS/CmuxNext/Tests/CmuxNextAgentPaneTests/AgentPaneForkRelayTests.swift index 2efee78420e7..dd657dadad32 100644 --- a/Packages/macOS/CmuxNext/Tests/CmuxNextAgentPaneTests/AgentPaneForkRelayTests.swift +++ b/Packages/macOS/CmuxNext/Tests/CmuxNextAgentPaneTests/AgentPaneForkRelayTests.swift @@ -29,4 +29,62 @@ struct AgentPaneForkRelayTests { func aForkCarriesNothingElse(_ params: String) { #expect(AcpmuxPaneMethods.breaksParamsRule(Self.object(params), modeFields: []), "\(params)") } + + static func frame(id: Int, params: String) -> String { + #"{"jsonrpc":"2.0","id":\#(id),"method":"acp.session.fork","params":\#(params)}"# + } + + /// A fork of a chat outside the pane's scope waits for the click's scope credit, as an attach the + /// person makes (``AcpmuxPaneMethods/sourceScoped``, `AgentPaneTransport+Frames.swift` + /// `consumeScope`). `live` names no other source and needs the click too; one click brings one + /// fork. The pane's own chat forks with no click. The Rust copy of this rule is + /// cmux-agent-pane-policy tests/cases/check.json ("live fork from outside the scope uses scope + /// credit"); this test runs the rule in the host that serves the pane. + @MainActor + @Test(.timeLimit(.minutes(1))) func aForkFromOutsideTheScopeGoesOutOnlyWithTheClick() async throws { + let server = AcpmuxStandInServer() + try await server.start() + defer { server.stop() } + server.answer("acp.session.fork", with: #"{"sessionId":"f1"}"#) + let inbox = AgentPaneRelayedAllowTests.PageInbox() + let transport = AgentPaneTransport() + transport.deliver = { event, done in inbox.deliver(event.frames); done() } + let id = try await transport.open(AcpmuxConnection(url: server.url, dashboardToken: "t", localAppToken: nil)) + #expect(await transport.send(connection: id, frames: [#"{"jsonrpc":"2.0","id":0,"method":"initialize","params":{}}"#]) == nil) + transport.sessions.add("s") + let sent = { server.peers.first?.frames.filter { $0.contains("acp.session.fork") } ?? [] } + func params(_ frame: String) throws -> NSDictionary { + let object = try JSONSerialization.jsonObject(with: Data(frame.utf8)) as? [String: Any] + return try #require(object?["params"] as? [String: Any]) as NSDictionary + } + + // No click: a chat outside the scope, with `live` and through a turn. + #expect(await transport.send(connection: id, frames: [Self.frame(id: 1, params: #"{"sessionId":"other","live":true}"#)]) == .gestureRequired, + "a live fork of a chat outside the pane's scope, with no click") + #expect(await transport.send(connection: id, frames: [Self.frame(id: 2, params: #"{"sessionId":"other","throughSeq":3}"#)]) == .gestureRequired, + "a fork through a turn of a chat outside the pane's scope, with no click") + #expect(sent().isEmpty, "a refused fork reached acpmux: \(sent())") + + // The pane's own chat forks with no click. + #expect(await transport.send(connection: id, frames: [Self.frame(id: 3, params: #"{"sessionId":"s","live":true}"#)]) == nil, + "the relay refused a live fork of the pane's own chat") + _ = await inbox.frame(containing: #""id":3"#) + #expect(try sent().map(params) == [["sessionId": "s", "live": true] as NSDictionary]) + + // A click brings one fork of a chat outside the scope in. + transport.gestures.record(click: true) + #expect(await transport.send(connection: id, frames: [Self.frame(id: 4, params: #"{"sessionId":"other","live":true}"#)]) == nil, + "the relay refused a live fork the person clicked for") + _ = await inbox.frame(containing: #""id":4"#) + #expect(try sent().dropFirst().map(params) == [["sessionId": "other", "live": true] as NSDictionary]) + + // The click's credit is spent: the next fork outside the scope needs its own click. + #expect(await transport.send(connection: id, frames: [Self.frame(id: 5, params: #"{"sessionId":"other","throughSeq":3}"#)]) == .gestureRequired) + #expect(sent().count == 2, "\(sent())") + transport.gestures.record(click: true) + #expect(await transport.send(connection: id, frames: [Self.frame(id: 6, params: #"{"sessionId":"other","throughSeq":3}"#)]) == nil, + "the relay refused a fork through a turn the person clicked for") + _ = await inbox.frame(containing: #""id":6"#) + #expect(try sent().dropFirst(2).map(params) == [["sessionId": "other", "throughSeq": 3] as NSDictionary]) + } } diff --git a/cmux-tui/crates/acpmux/src/server/fork_through.rs b/cmux-tui/crates/acpmux/src/server/fork_through.rs index a5c72d58b21f..5d6e3fcfd8de 100644 --- a/cmux-tui/crates/acpmux/src/server/fork_through.rs +++ b/cmux-tui/crates/acpmux/src/server/fork_through.rs @@ -50,23 +50,33 @@ pub(super) fn through(params: &Value) -> Option { params.pointer("/_meta/acpmux/throughSeq").and_then(Value::as_u64) } +/// Why a chat on another computer (a peer's) cannot be forked here: a `live` +/// fork names no turn. Daemon error text, in English as acpmux's other +/// errors; the pane never sends a fork of a peer's chat (webviews +/// `messageMenu.ts` `forkServed`), so only a raw client reads it. +fn remote_refusal(live: bool) -> &'static str { + if live { + "forking a chat on another computer is not supported yet" + } else { + "forking a chat on another computer through a turn is not supported yet" + } +} + /// The `session/fork` params for an `acp.session.fork` request, or why the /// fork cannot happen at that turn. async fn to_session_fork(hub: &Hub, params: &Value) -> Result { let key = session_key(params)?; + // `live`: the whole chat as it is now (Fork Conversation), its running turn cut off + // there (`hub/fork.rs`). + let live = params.get("live").and_then(Value::as_bool).unwrap_or(false); let session = hub.resolve(key).map_err(|e| { if hub.resolve_remote(key).is_some() { - RpcError::invalid_params( - "forking a chat on another computer through a turn is not supported yet", - ) + RpcError::invalid_params(remote_refusal(live)) } else { e } })?; let working = matches!(session.meta().status, SessionStatus::Running | SessionStatus::Waiting); - // `live`: the whole chat as it is now (Fork Conversation), its running turn cut off - // there (`hub/fork.rs`). - let live = params.get("live").and_then(Value::as_bool).unwrap_or(false); let can_cut = { let harness = session.meta().harness; let cfg = hub.config.read().await; @@ -134,3 +144,58 @@ async fn to_session_fork(hub: &Hub, params: &Value) -> Result { out.set("_meta", json!({"acpmux": mux})); Ok(out) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::config::{Config, StoreMode}; + use crate::session_identity::SessionReceipt; + + /// A hub with no chat of its own, and peer hb (listed, never run) listing a working chat: + /// `Hub::resolve_remote` needs the peer as well as its row (`hub/own_host.rs` `add_peer`). + fn hub_with_a_peer_chat() -> Arc { + let mut config: Config = serde_json::from_value(json!({})).unwrap(); + config.store.mode = StoreMode::Memory; + let store = + crate::store::open(&config.store, std::path::Path::new("/nonexistent")).unwrap(); + let hub = Hub::new(config, store); + let row = crate::hub::RemoteSession { + peer: "hb".into(), + summary: json!({"sessionId": "peer-chat-1", "name": "hb/chat", "status": "running"}), + receipt: SessionReceipt { + host_id: "hb".into(), + mux_session_id: "peer-chat-1".into(), + agent_session_id: None, + generation: 1, + }, + }; + hub.remote_sessions.lock().unwrap().insert("hb/peer-chat-1".into(), row); + let (tx, _rx) = tokio::sync::mpsc::channel(8); + let peer = crate::peer::Peer::new("hb", "ws://127.0.0.1:9", None, None, tx); + hub.peers.lock().unwrap().insert("hb".to_owned(), peer); + hub + } + + /// Review of #28332: a `live` fork (Fork Conversation of a working chat) names no turn, so + /// its refusal for a peer's chat does not say "through a turn"; a fork through a turn does. + #[tokio::test] + async fn a_peer_chats_fork_is_refused_with_what_it_asked_for() { + let hub = hub_with_a_peer_chat(); + assert!( + hub.resolve_remote("hb/peer-chat-1").is_some(), + "the fixture lists the peer's chat" + ); + let live = to_session_fork(&hub, &json!({"sessionId": "hb/peer-chat-1", "live": true})) + .await + .unwrap_err(); + assert_eq!(live.message, "forking a chat on another computer is not supported yet"); + let through = + to_session_fork(&hub, &json!({"sessionId": "hb/peer-chat-1", "throughSeq": 3})) + .await + .unwrap_err(); + assert_eq!( + through.message, + "forking a chat on another computer through a turn is not supported yet" + ); + } +} diff --git a/cmux-tui/crates/cmux-agent-pane-policy/tests/cases/check.json b/cmux-tui/crates/cmux-agent-pane-policy/tests/cases/check.json index 6c0be8866fc1..f0d1f208c021 100644 --- a/cmux-tui/crates/cmux-agent-pane-policy/tests/cases/check.json +++ b/cmux-tui/crates/cmux-agent-pane-policy/tests/cases/check.json @@ -514,6 +514,47 @@ } } }, + { + "name": "live fork from outside the scope uses scope credit", + "text": "{\"jsonrpc\":\"2.0\",\"id\":13,\"method\":\"acp.session.fork\",\"params\":{\"sessionId\":\"s9\",\"live\":true}}", + "state": { + "first": false, + "token": null, + "mode_fields": null, + "sessions": [ + "s1" + ], + "denies": [] + }, + "expect": { + "frame": { + "jsonrpc": "2.0", + "id": 13, + "method": "acp.session.fork", + "params": { + "sessionId": "s9", + "live": true + } + }, + "facts": { + "is_first": false, + "method": "acp.session.fork", + "page_id": "13", + "ticket": null, + "other_meta": false, + "pick": null, + "session_id": null, + "needs_gesture": false, + "needs_path_check": false, + "setting": null, + "attach_session": null, + "foreign_source": true, + "handoff_id": null, + "harness_enable": false, + "free": false + } + } + }, { "name": "fork from the scope is free", "text": "{\"jsonrpc\":\"2.0\",\"id\":12,\"method\":\"acp.session.fork\",\"params\":{\"sessionId\":\"s1\"}}", diff --git a/cmux-tui/crates/cmux-agent-pane-policy/tests/cases/params.json b/cmux-tui/crates/cmux-agent-pane-policy/tests/cases/params.json index 34fb4b622f19..eb9319552109 100644 --- a/cmux-tui/crates/cmux-agent-pane-policy/tests/cases/params.json +++ b/cmux-tui/crates/cmux-agent-pane-policy/tests/cases/params.json @@ -870,7 +870,8 @@ "method": "acp.session.fork", "params": { "sessionId": 1, - "throughSeq": 1 + "throughSeq": 1, + "live": true } }, "mode_fields": null, diff --git a/cmux-tui/crates/cmux-agent-pane-policy/tests/parity.rs b/cmux-tui/crates/cmux-agent-pane-policy/tests/parity.rs index 40f0d9618ba4..a8ac70c72044 100644 --- a/cmux-tui/crates/cmux-agent-pane-policy/tests/parity.rs +++ b/cmux-tui/crates/cmux-agent-pane-policy/tests/parity.rs @@ -109,12 +109,13 @@ fn facts_json(facts: &cmux_agent_pane_policy::Facts) -> Value { }) } -/// The full check in Swift's order (tests/cases/check.json; Swift runs the -/// same file against `AgentPaneTransport.checkOne`). +/// The full check in Swift's order (tests/cases/check.json). Only this crate +/// runs the file: no Swift test reads it, so `AgentPaneTransport.checkOne` has +/// its own Swift tests (for example `AgentPaneForkRelayTests`). #[test] fn full_check_order() { let all = cases("check.json"); - assert_eq!(all.as_array().unwrap().len(), 54, "check.json: the full order's 54 cases"); + assert_eq!(all.as_array().unwrap().len(), 55, "check.json: the full order's 55 cases"); for c in all.as_array().unwrap() { let s = &c["state"]; let modes: Option> = s["mode_fields"] @@ -125,8 +126,8 @@ fn full_check_order() { scope.add(session.as_str().unwrap()); } let options = PermissionOptions::new(); - // A question (state `questions`, crate only: Swift does not read it - // yet) is one pending request with its denies and the question mark. + // A question (state `questions`) is one pending request with its + // denies and the question mark. let questions: BTreeSet<&str> = s["questions"] .as_array() .map(|a| a.iter().map(|q| q.as_str().unwrap()).collect()) @@ -145,9 +146,9 @@ fn full_check_order() { for d in s["denies"].as_array().unwrap().iter().filter(|d| d[0] == *q) { listed.push(serde_json::json!({"optionId": d[1], "kind": "reject_once"})); } - // The shared cases keep the question body out of `state`. Mirror the Swift - // parity harness by deriving the pending question's item ids from this frame's - // `answers` object, so answer cases exercise the same key allowlist in both hosts. + // The shared cases keep the question body out of `state`: the pending + // question's item ids come from this frame's `answers` object, so the + // answer cases exercise the key allowlist. let keys: Vec = c["text"] .as_str() .and_then(|text| serde_json::from_str::(text).ok()) @@ -215,8 +216,8 @@ fn debug_output_is_redacted() { assert!(!shown.contains(&token), "{shown}"); } -/// The pane's scope and the source rule (tests/cases/sources.json; Swift runs -/// it against `AcpmuxPaneSessions`). +/// The pane's scope and the source rule (tests/cases/sources.json). Only this +/// crate runs the file; no Swift test reads it. #[test] fn scope_and_sources() { for c in cases("sources.json").as_array().unwrap() {