diff --git a/server/packages/sandbox-agent/src/opencode_compat.rs b/server/packages/sandbox-agent/src/opencode_compat.rs index 97dffb6..d940eb7 100644 --- a/server/packages/sandbox-agent/src/opencode_compat.rs +++ b/server/packages/sandbox-agent/src/opencode_compat.rs @@ -23,7 +23,9 @@ use tokio::sync::{broadcast, Mutex}; use tokio::time::interval; use utoipa::{IntoParams, OpenApi, ToSchema}; -use crate::router::{AgentModelInfo, AppState, CreateSessionRequest, PermissionReply}; +use crate::router::{ + is_question_tool_action, AgentModelInfo, AppState, CreateSessionRequest, PermissionReply, +}; use sandbox_agent_agent_management::agents::AgentId; use sandbox_agent_error::SandboxError; use sandbox_agent_universal_agent_schema::{ @@ -1646,6 +1648,11 @@ async fn apply_permission_event( event: UniversalEvent, permission: PermissionEventData, ) { + // Suppress question-tool permissions (AskUserQuestion/ExitPlanMode) — these are + // handled internally via reply_question/reject_question, not exposed as permissions. + if is_question_tool_action(&permission.action) { + return; + } let session_id = event.session_id.clone(); match permission.status { PermissionStatus::Requested => { diff --git a/server/packages/sandbox-agent/src/router.rs b/server/packages/sandbox-agent/src/router.rs index 6cd24fa..f4e4b91 100644 --- a/server/packages/sandbox-agent/src/router.rs +++ b/server/packages/sandbox-agent/src/router.rs @@ -690,6 +690,21 @@ impl SessionState { self.update_pending(&event); self.update_item_tracking(&event); + + // Suppress question-tool permissions (AskUserQuestion/ExitPlanMode) from frontends. + // The permission is still stored in pending_permissions (via update_pending above) + // so reply_question/reject_question can find and resolve it internally. + if matches!( + event.event_type, + UniversalEventType::PermissionRequested | UniversalEventType::PermissionResolved + ) { + if let UniversalEventData::Permission(ref data) = event.data { + if is_question_tool_action(&data.action) { + return None; + } + } + } + self.events.push(event.clone()); let _ = self.broadcaster.send(event.clone()); if self.native_session_id.is_none() { @@ -767,6 +782,17 @@ impl SessionState { self.pending_permissions.remove(permission_id) } + /// Find and remove a pending permission whose action matches a question tool + /// (AskUserQuestion or ExitPlanMode variants). Returns (permission_id, PendingPermission). + fn take_question_tool_permission(&mut self) -> Option<(String, PendingPermission)> { + let key = self + .pending_permissions + .iter() + .find(|(_, p)| is_question_tool_action(&p.action)) + .map(|(k, _)| k.clone()); + key.and_then(|k| self.pending_permissions.remove(&k).map(|p| (k, p))) + } + fn mark_ended( &mut self, exit_code: Option, @@ -2117,7 +2143,7 @@ impl SessionManager { question_id: &str, answers: Vec>, ) -> Result<(), SandboxError> { - let (agent, native_session_id, pending_question, claude_sender) = { + let (agent, native_session_id, pending_question, claude_sender, linked_permission) = { let mut sessions = self.sessions.lock().await; let session = Self::session_mut(&mut sessions, session_id).ok_or_else(|| { SandboxError::SessionNotFound { @@ -2133,11 +2159,18 @@ impl SessionManager { if let Some(err) = session.ended_error() { return Err(err); } + // For Claude, check if there's a linked AskUserQuestion/ExitPlanMode permission + let linked_perm = if session.agent == AgentId::Claude { + session.take_question_tool_permission() + } else { + None + }; ( session.agent, session.native_session_id.clone(), pending, session.claude_sender(), + linked_perm, ) }; @@ -2150,28 +2183,67 @@ impl SessionManager { .ok_or_else(|| SandboxError::InvalidRequest { message: "missing OpenCode session id".to_string(), })?; - self.opencode_question_reply(&agent_session_id, question_id, answers) + self.opencode_question_reply(&agent_session_id, question_id, answers.clone()) .await?; } else if agent == AgentId::Claude { let sender = claude_sender.ok_or_else(|| SandboxError::InvalidRequest { message: "Claude session is not active".to_string(), })?; - let session_id = native_session_id - .clone() - .unwrap_or_else(|| session_id.to_string()); - let response_text = response.clone().unwrap_or_default(); - let line = claude_tool_result_line(&session_id, question_id, &response_text, false); - sender - .send(line) - .map_err(|_| SandboxError::InvalidRequest { - message: "Claude session is not active".to_string(), - })?; + if let Some((perm_id, perm)) = &linked_permission { + // Use the permission control response to deliver the answer. + // Build updatedInput from the original input with the answers map added. + let original_input = perm + .metadata + .as_ref() + .and_then(|m| m.get("input")) + .cloned() + .unwrap_or(Value::Null); + let mut updated = match original_input { + Value::Object(map) => map, + _ => serde_json::Map::new(), + }; + // Build answers map: { "0": "selected option", "1": "another option", ... } + let answers_map: serde_json::Map = answers + .iter() + .enumerate() + .filter_map(|(i, inner)| { + inner + .first() + .map(|v| (i.to_string(), Value::String(v.clone()))) + }) + .collect(); + updated.insert("answers".to_string(), Value::Object(answers_map)); + + let mut response_map = serde_json::Map::new(); + response_map.insert("updatedInput".to_string(), Value::Object(updated)); + let line = + claude_control_response_line(perm_id, "allow", Value::Object(response_map)); + sender + .send(line) + .map_err(|_| SandboxError::InvalidRequest { + message: "Claude session is not active".to_string(), + })?; + } else { + // No linked permission — fall back to tool_result + let native_sid = native_session_id + .clone() + .unwrap_or_else(|| session_id.to_string()); + let response_text = response.clone().unwrap_or_default(); + let line = + claude_tool_result_line(&native_sid, question_id, &response_text, false); + sender + .send(line) + .map_err(|_| SandboxError::InvalidRequest { + message: "Claude session is not active".to_string(), + })?; + } } else { // TODO: Forward question replies to subprocess agents. } + // Emit QuestionResolved if let Some(pending) = pending_question { - let resolved = EventConversion::new( + let mut conversions = vec![EventConversion::new( UniversalEventType::QuestionResolved, UniversalEventData::Question(QuestionEventData { question_id: question_id.to_string(), @@ -2182,8 +2254,26 @@ impl SessionManager { }), ) .synthetic() - .with_native_session(native_session_id); - let _ = self.record_conversions(session_id, vec![resolved]).await; + .with_native_session(native_session_id.clone())]; + + // Also emit PermissionResolved for the linked permission + if let Some((perm_id, perm)) = linked_permission { + conversions.push( + EventConversion::new( + UniversalEventType::PermissionResolved, + UniversalEventData::Permission(PermissionEventData { + permission_id: perm_id, + action: perm.action, + status: PermissionStatus::Approved, + metadata: perm.metadata, + }), + ) + .synthetic() + .with_native_session(native_session_id), + ); + } + + let _ = self.record_conversions(session_id, conversions).await; } Ok(()) @@ -2194,7 +2284,7 @@ impl SessionManager { session_id: &str, question_id: &str, ) -> Result<(), SandboxError> { - let (agent, native_session_id, pending_question, claude_sender) = { + let (agent, native_session_id, pending_question, claude_sender, linked_permission) = { let mut sessions = self.sessions.lock().await; let session = Self::session_mut(&mut sessions, session_id).ok_or_else(|| { SandboxError::SessionNotFound { @@ -2210,11 +2300,17 @@ impl SessionManager { if let Some(err) = session.ended_error() { return Err(err); } + let linked_perm = if session.agent == AgentId::Claude { + session.take_question_tool_permission() + } else { + None + }; ( session.agent, session.native_session_id.clone(), pending, session.claude_sender(), + linked_perm, ) }; @@ -2231,26 +2327,43 @@ impl SessionManager { let sender = claude_sender.ok_or_else(|| SandboxError::InvalidRequest { message: "Claude session is not active".to_string(), })?; - let session_id = native_session_id - .clone() - .unwrap_or_else(|| session_id.to_string()); - let line = claude_tool_result_line( - &session_id, - question_id, - "User rejected the question.", - true, - ); - sender - .send(line) - .map_err(|_| SandboxError::InvalidRequest { - message: "Claude session is not active".to_string(), - })?; + if let Some((perm_id, _)) = &linked_permission { + // Deny via the permission control response + let mut response_map = serde_json::Map::new(); + response_map.insert( + "message".to_string(), + Value::String("Permission denied.".to_string()), + ); + let line = + claude_control_response_line(perm_id, "deny", Value::Object(response_map)); + sender + .send(line) + .map_err(|_| SandboxError::InvalidRequest { + message: "Claude session is not active".to_string(), + })?; + } else { + let native_sid = native_session_id + .clone() + .unwrap_or_else(|| session_id.to_string()); + let line = claude_tool_result_line( + &native_sid, + question_id, + "User rejected the question.", + true, + ); + sender + .send(line) + .map_err(|_| SandboxError::InvalidRequest { + message: "Claude session is not active".to_string(), + })?; + } } else { // TODO: Forward question rejections to subprocess agents. } + // Emit QuestionResolved if let Some(pending) = pending_question { - let resolved = EventConversion::new( + let mut conversions = vec![EventConversion::new( UniversalEventType::QuestionResolved, UniversalEventData::Question(QuestionEventData { question_id: question_id.to_string(), @@ -2261,8 +2374,26 @@ impl SessionManager { }), ) .synthetic() - .with_native_session(native_session_id); - let _ = self.record_conversions(session_id, vec![resolved]).await; + .with_native_session(native_session_id.clone())]; + + // Also emit PermissionResolved for the linked permission + if let Some((perm_id, perm)) = linked_permission { + conversions.push( + EventConversion::new( + UniversalEventType::PermissionResolved, + UniversalEventData::Permission(PermissionEventData { + permission_id: perm_id, + action: perm.action, + status: PermissionStatus::Denied, + metadata: perm.metadata, + }), + ) + .synthetic() + .with_native_session(native_session_id), + ); + } + + let _ = self.record_conversions(session_id, conversions).await; } Ok(()) @@ -5076,6 +5207,22 @@ fn claude_control_response_line(request_id: &str, behavior: &str, response: Valu .to_string() } +/// Returns true if the given action name corresponds to a question tool +/// (AskUserQuestion or ExitPlanMode in any casing convention). +pub(crate) fn is_question_tool_action(action: &str) -> bool { + matches!( + action, + "AskUserQuestion" + | "ask_user_question" + | "askUserQuestion" + | "ask-user-question" + | "ExitPlanMode" + | "exit_plan_mode" + | "exitPlanMode" + | "exit-plan-mode" + ) +} + fn read_lines(reader: R, sender: mpsc::UnboundedSender) { let mut reader = BufReader::new(reader); let mut line = String::new(); diff --git a/server/packages/sandbox-agent/tests/sessions/snapshots/sessions__sessions__multi_turn__assert_session_snapshot@multi_turn_mock.snap.new b/server/packages/sandbox-agent/tests/sessions/snapshots/sessions__sessions__multi_turn__assert_session_snapshot@multi_turn_mock.snap.new index 2a091af..a6ecd24 100644 --- a/server/packages/sandbox-agent/tests/sessions/snapshots/sessions__sessions__multi_turn__assert_session_snapshot@multi_turn_mock.snap.new +++ b/server/packages/sandbox-agent/tests/sessions/snapshots/sessions__sessions__multi_turn__assert_session_snapshot@multi_turn_mock.snap.new @@ -38,36 +38,13 @@ first: status: in_progress seq: 5 type: item.started - - delta: - delta: "" - item_id: "" - native_item_id: "" + - item: + content_types: [] + kind: message + role: assistant + status: completed seq: 6 - type: item.delta - - delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 7 - type: item.delta - - delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 8 - type: item.delta - - delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 9 - type: item.delta - - delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 10 - type: item.delta + type: item.completed second: - item: content_types: @@ -105,27 +82,11 @@ second: native_item_id: "" seq: 5 type: item.delta - - delta: - delta: "" - item_id: "" - native_item_id: "" + - item: + content_types: + - text + kind: message + role: assistant + status: completed seq: 6 - type: item.delta - - delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 7 - type: item.delta - - delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 8 - type: item.delta - - delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 9 - type: item.delta + type: item.completed diff --git a/server/packages/sandbox-agent/tests/sessions/snapshots/sessions__sessions__permissions__assert_session_snapshot@permission_events_mock.snap.new b/server/packages/sandbox-agent/tests/sessions/snapshots/sessions__sessions__permissions__assert_session_snapshot@permission_events_mock.snap.new index d5c1b20..145c275 100644 --- a/server/packages/sandbox-agent/tests/sessions/snapshots/sessions__sessions__permissions__assert_session_snapshot@permission_events_mock.snap.new +++ b/server/packages/sandbox-agent/tests/sessions/snapshots/sessions__sessions__permissions__assert_session_snapshot@permission_events_mock.snap.new @@ -79,159 +79,3 @@ expression: value native_item_id: "" seq: 12 type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 13 - type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 14 - type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 15 - type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 16 - type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 17 - type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 18 - type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 19 - type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 20 - type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 21 - type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 22 - type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 23 - type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 24 - type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 25 - type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 26 - type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 27 - type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 28 - type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 29 - type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 30 - type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 31 - type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 32 - type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 33 - type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 34 - type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 35 - type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 36 - type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 37 - type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 38 - type: item.delta diff --git a/server/packages/sandbox-agent/tests/sessions/snapshots/sessions__sessions__questions__assert_session_snapshot@question_reply_events_mock.snap.new b/server/packages/sandbox-agent/tests/sessions/snapshots/sessions__sessions__questions__assert_session_snapshot@question_reply_events_mock.snap.new index f414271..bc77ae1 100644 --- a/server/packages/sandbox-agent/tests/sessions/snapshots/sessions__sessions__questions__assert_session_snapshot@question_reply_events_mock.snap.new +++ b/server/packages/sandbox-agent/tests/sessions/snapshots/sessions__sessions__questions__assert_session_snapshot@question_reply_events_mock.snap.new @@ -91,15 +91,9 @@ expression: value native_item_id: "" seq: 14 type: item.delta -- delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 15 - type: item.delta - question: id: "" options: 4 status: requested - seq: 16 + seq: 15 type: question.requested diff --git a/server/packages/sandbox-agent/tests/sessions/snapshots/sessions__sessions__session_lifecycle__assert_session_snapshot@concurrency_events_mock.snap.new b/server/packages/sandbox-agent/tests/sessions/snapshots/sessions__sessions__session_lifecycle__assert_session_snapshot@concurrency_events_mock.snap.new index a6e0065..360ffd7 100644 --- a/server/packages/sandbox-agent/tests/sessions/snapshots/sessions__sessions__session_lifecycle__assert_session_snapshot@concurrency_events_mock.snap.new +++ b/server/packages/sandbox-agent/tests/sessions/snapshots/sessions__sessions__session_lifecycle__assert_session_snapshot@concurrency_events_mock.snap.new @@ -44,24 +44,6 @@ session_a: native_item_id: "" seq: 6 type: item.delta - - delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 7 - type: item.delta - - delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 8 - type: item.delta - - delta: - delta: "" - item_id: "" - native_item_id: "" - seq: 9 - type: item.delta session_b: - metadata: true seq: 1 diff --git a/server/packages/sandbox-agent/tests/sessions/snapshots/sessions__sessions__session_lifecycle__run_http_events_snapshot@http_events_mock.snap.new b/server/packages/sandbox-agent/tests/sessions/snapshots/sessions__sessions__session_lifecycle__run_http_events_snapshot@http_events_mock.snap.new index da365cc..2324c31 100644 --- a/server/packages/sandbox-agent/tests/sessions/snapshots/sessions__sessions__session_lifecycle__run_http_events_snapshot@http_events_mock.snap.new +++ b/server/packages/sandbox-agent/tests/sessions/snapshots/sessions__sessions__session_lifecycle__run_http_events_snapshot@http_events_mock.snap.new @@ -37,10 +37,9 @@ expression: normalized status: in_progress seq: 5 type: item.started -- item: - content_types: [] - kind: message - role: assistant - status: completed +- delta: + delta: "" + item_id: "" + native_item_id: "" seq: 6 - type: item.completed + type: item.delta diff --git a/server/packages/sandbox-agent/tests/sessions/snapshots/sessions__sessions__session_lifecycle__run_sse_events_snapshot@sse_events_mock.snap.new b/server/packages/sandbox-agent/tests/sessions/snapshots/sessions__sessions__session_lifecycle__run_sse_events_snapshot@sse_events_mock.snap.new new file mode 100644 index 0000000..57c589e --- /dev/null +++ b/server/packages/sandbox-agent/tests/sessions/snapshots/sessions__sessions__session_lifecycle__run_sse_events_snapshot@sse_events_mock.snap.new @@ -0,0 +1,45 @@ +--- +source: server/packages/sandbox-agent/tests/sessions/../common/http.rs +assertion_line: 1039 +expression: normalized +--- +- metadata: true + seq: 1 + session: started + type: session.started +- item: + content_types: + - text + kind: message + role: user + status: in_progress + seq: 2 + type: item.started +- delta: + delta: "" + item_id: "" + native_item_id: "" + seq: 3 + type: item.delta +- item: + content_types: + - text + kind: message + role: user + status: completed + seq: 4 + type: item.completed +- item: + content_types: + - text + kind: message + role: assistant + status: in_progress + seq: 5 + type: item.started +- delta: + delta: "" + item_id: "" + native_item_id: "" + seq: 6 + type: item.delta