diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index ef6d6b8be8..8780a0aa47 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -1986,6 +1986,25 @@ impl Session { state.clear_connector_selection(); } + pub(crate) async fn record_slack_channel_name( + &self, + connector_id: Option<&str>, + channel_id: String, + channel_name: String, + ) { + let mut state = self.state.lock().await; + state.record_slack_channel_name(connector_id, channel_id, channel_name); + } + + pub(crate) async fn slack_channel_name( + &self, + connector_id: Option<&str>, + channel_id: &str, + ) -> Option { + let state = self.state.lock().await; + state.slack_channel_name(connector_id, channel_id) + } + async fn record_initial_history(&self, conversation_history: InitialHistory) { let turn_context = self.new_default_turn().await; let is_subagent = { diff --git a/codex-rs/core/src/consequential_tool_message_templates.json b/codex-rs/core/src/consequential_tool_message_templates.json index 83e11c79a2..0014c18e19 100644 --- a/codex-rs/core/src/consequential_tool_message_templates.json +++ b/codex-rs/core/src/consequential_tool_message_templates.json @@ -591,6 +591,10 @@ "server_name": "codex_apps", "tool_title": "slack_send_message", "template_params": [ + { + "name": "to", + "label": "To" + }, { "name": "channel_id", "label": "Conversation" diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 51ea3f47ea..cea6d53957 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -69,6 +69,7 @@ mod sandbox_tags; pub mod sandboxing; mod session_prefix; mod shell_detect; +mod slack_channel_names; mod stream_events_utils; pub mod test_support; mod text_encoding; diff --git a/codex-rs/core/src/mcp_tool_approval_templates.rs b/codex-rs/core/src/mcp_tool_approval_templates.rs index f8fbad3ede..6f16f435da 100644 --- a/codex-rs/core/src/mcp_tool_approval_templates.rs +++ b/codex-rs/core/src/mcp_tool_approval_templates.rs @@ -310,6 +310,48 @@ mod tests { assert_eq!(CONSEQUENTIAL_TOOL_MESSAGE_TEMPLATES.is_some(), true); } + #[test] + fn bundled_slack_send_message_template_renders_to_before_conversation() { + let rendered = render_mcp_tool_approval_template( + "codex_apps", + Some("asdk_app_69a1d78e929881919bba0dbda1f6436d"), + Some("Slack"), + Some("slack_send_message"), + Some(&json!({ + "channel_id": "U123", + "message": "hello", + "to": "@mzeng", + })), + ); + + assert_eq!( + rendered, + Some(RenderedMcpToolApprovalTemplate { + question: "Allow Slack to send a message?".to_string(), + elicitation_message: "Allow Slack to send a message?".to_string(), + tool_params: Some(json!({ + "To": "@mzeng", + "Conversation": "U123", + "Message": "hello", + })), + tool_params_display: vec![ + RenderedMcpToolApprovalParam { + name: "To".to_string(), + value: json!("@mzeng"), + }, + RenderedMcpToolApprovalParam { + name: "Conversation".to_string(), + value: json!("U123"), + }, + RenderedMcpToolApprovalParam { + name: "Message".to_string(), + value: json!("hello"), + }, + ], + }) + ); + } + #[test] fn renders_literal_template_without_connector_substitution() { let templates = vec![ConsequentialToolMessageTemplate { diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 888d86c5ff..8cc98d3596 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -32,6 +32,9 @@ use crate::protocol::EventMsg; use crate::protocol::McpInvocation; use crate::protocol::McpToolCallBeginEvent; use crate::protocol::McpToolCallEndEvent; +use crate::slack_channel_names::slack_channel_name_from_profile_result; +use crate::slack_channel_names::slack_send_message_channel_id; +use crate::slack_channel_names::translated_slack_send_message_tool_params; use crate::state_db; use codex_protocol::mcp::CallToolResult; use codex_protocol::openai_models::InputModality; @@ -170,6 +173,12 @@ pub(crate) async fn handle_mcp_tool_call( tool_call_end_event.clone(), ) .await; + maybe_record_slack_channel_name_from_tool_result( + sess.as_ref(), + metadata.as_ref(), + &result, + ) + .await; maybe_track_codex_app_used( sess.as_ref(), turn_context.as_ref(), @@ -257,6 +266,8 @@ pub(crate) async fn handle_mcp_tool_call( tool_call_end_event.clone(), ) .await; + maybe_record_slack_channel_name_from_tool_result(sess.as_ref(), metadata.as_ref(), &result) + .await; maybe_track_codex_app_used(sess.as_ref(), turn_context.as_ref(), &server, &tool_name).await; let status = if result.is_ok() { "ok" } else { "error" }; @@ -516,17 +527,19 @@ async fn maybe_request_mcp_tool_approval( tool_call_mcp_elicitation_enabled, ); let question_id = format!("{MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX}_{call_id}"); + let approval_tool_params = + build_mcp_tool_approval_tool_params(sess.as_ref(), invocation, metadata).await; let rendered_template = render_mcp_tool_approval_template( &invocation.server, metadata.and_then(|metadata| metadata.connector_id.as_deref()), metadata.and_then(|metadata| metadata.connector_name.as_deref()), metadata.and_then(|metadata| metadata.tool_title.as_deref()), - invocation.arguments.as_ref(), + approval_tool_params.as_ref(), ); let tool_params_display = rendered_template .as_ref() .map(|rendered_template| rendered_template.tool_params_display.clone()) - .or_else(|| build_mcp_tool_approval_display_params(invocation.arguments.as_ref())); + .or_else(|| build_mcp_tool_approval_display_params(approval_tool_params.as_ref())); let mut question = build_mcp_tool_approval_question( question_id.clone(), &invocation.server, @@ -552,7 +565,7 @@ async fn maybe_request_mcp_tool_approval( tool_params: rendered_template .as_ref() .and_then(|rendered_template| rendered_template.tool_params.as_ref()) - .or(invocation.arguments.as_ref()), + .or(approval_tool_params.as_ref()), tool_params_display: tool_params_display.as_deref(), question, message_override: rendered_template.as_ref().and_then(|rendered_template| { @@ -676,6 +689,55 @@ fn build_guardian_mcp_tool_review_request( } } +async fn build_mcp_tool_approval_tool_params( + sess: &Session, + invocation: &McpInvocation, + metadata: Option<&McpToolApprovalMetadata>, +) -> Option { + let connector_name = metadata.and_then(|metadata| metadata.connector_name.as_deref()); + let tool_title = metadata.and_then(|metadata| metadata.tool_title.as_deref()); + let connector_id = metadata.and_then(|metadata| metadata.connector_id.as_deref()); + let channel_name = match slack_send_message_channel_id( + connector_name, + tool_title, + invocation.arguments.as_ref(), + ) { + Some(channel_id) => sess.slack_channel_name(connector_id, channel_id).await, + None => None, + }; + + translated_slack_send_message_tool_params( + connector_name, + tool_title, + invocation.arguments.as_ref(), + channel_name.as_deref(), + ) +} + +async fn maybe_record_slack_channel_name_from_tool_result( + sess: &Session, + metadata: Option<&McpToolApprovalMetadata>, + result: &Result, +) { + let Ok(result) = result else { + return; + }; + let Some(channel_name) = slack_channel_name_from_profile_result( + metadata.and_then(|metadata| metadata.connector_name.as_deref()), + metadata.and_then(|metadata| metadata.tool_title.as_deref()), + result, + ) else { + return; + }; + + sess.record_slack_channel_name( + metadata.and_then(|metadata| metadata.connector_id.as_deref()), + channel_name.channel_id, + channel_name.channel_name, + ) + .await; +} + fn mcp_tool_approval_decision_from_guardian(decision: ReviewDecision) -> McpToolApprovalDecision { match decision { ReviewDecision::Approved @@ -1408,6 +1470,84 @@ mod tests { ); } + #[tokio::test] + async fn slack_profile_tool_result_records_channel_name_with_global_fallback() { + let (session, _turn_context) = make_session_and_context().await; + let metadata = approval_metadata( + Some("connector_slack_profile"), + Some("Slack Codex App"), + None, + Some("get_profile"), + None, + ); + let result = Ok(CallToolResult { + content: Vec::new(), + structured_content: Some(serde_json::json!({ + "result": { + "id": "U123", + "name": "Mason Zeng", + "nickname": "mzeng", + } + })), + is_error: Some(false), + meta: None, + }); + + maybe_record_slack_channel_name_from_tool_result(&session, Some(&metadata), &result).await; + + assert_eq!( + session + .slack_channel_name(Some("connector_slack_profile"), "U123") + .await, + Some("@mzeng".to_string()) + ); + assert_eq!( + session + .slack_channel_name(Some("connector_slack_send"), "U123") + .await, + Some("@mzeng".to_string()) + ); + } + + #[tokio::test] + async fn slack_send_message_approval_tool_params_include_translated_to_field() { + let (session, _turn_context) = make_session_and_context().await; + session + .record_slack_channel_name( + Some("connector_slack_profile"), + "U123".to_string(), + "@mzeng".to_string(), + ) + .await; + let invocation = McpInvocation { + server: CODEX_APPS_MCP_SERVER_NAME.to_string(), + tool: "slack_slack_send_message".to_string(), + arguments: Some(serde_json::json!({ + "channel_id": "U123", + "message": "hi", + })), + }; + let metadata = approval_metadata( + Some("connector_slack_send"), + Some("Slack"), + None, + Some("slack_send_message"), + None, + ); + + let tool_params = + build_mcp_tool_approval_tool_params(&session, &invocation, Some(&metadata)).await; + + assert_eq!( + tool_params, + Some(serde_json::json!({ + "channel_id": "U123", + "message": "hi", + "to": "@mzeng", + })) + ); + } + #[test] fn custom_mcp_tool_question_mentions_server_name() { let question = build_mcp_tool_approval_question( diff --git a/codex-rs/core/src/slack_channel_names.rs b/codex-rs/core/src/slack_channel_names.rs new file mode 100644 index 0000000000..79da51a3b2 --- /dev/null +++ b/codex-rs/core/src/slack_channel_names.rs @@ -0,0 +1,389 @@ +use codex_protocol::mcp::CallToolResult; +use serde_json::Map; +use serde_json::Value; + +const CHANNEL_ID_FIELD_NAME: &str = "channel_id"; +const ID_FIELD_NAME: &str = "id"; +const NAME_FIELD_NAME: &str = "name"; +const NICKNAME_FIELD_NAME: &str = "nickname"; +const RESULT_FIELD_NAME: &str = "result"; +const TO_FIELD_NAME: &str = "to"; + +const SLACK_GET_PROFILE_TOOL_TITLE: &str = "get_profile"; +const SLACK_READ_USER_PROFILE_TOOL_TITLE: &str = "slack_read_user_profile"; +const SLACK_SEND_MESSAGE_TOOL_TITLE: &str = "slack_send_message"; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct SlackChannelName { + pub(crate) channel_id: String, + pub(crate) channel_name: String, +} + +pub(crate) fn slack_channel_name_from_profile_result( + connector_name: Option<&str>, + tool_title: Option<&str>, + result: &CallToolResult, +) -> Option { + if !is_slack_profile_tool(connector_name, tool_title) { + return None; + } + + if let Some(channel_name) = result + .structured_content + .as_ref() + .filter(|value| !value.is_null()) + .and_then(slack_channel_name_from_payload) + { + return Some(channel_name); + } + + let parsed_text_payload = parse_payload_from_text_content(&result.content); + if let Some(channel_name) = parsed_text_payload + .as_ref() + .and_then(slack_channel_name_from_payload) + { + return Some(channel_name); + } + + raw_text_content(&result.content).and_then(parse_slack_channel_name_from_text) +} + +pub(crate) fn slack_send_message_channel_id<'a>( + connector_name: Option<&str>, + tool_title: Option<&str>, + tool_params: Option<&'a Value>, +) -> Option<&'a str> { + if !is_slack_send_message_tool(connector_name, tool_title) { + return None; + } + + tool_params? + .as_object()? + .get(CHANNEL_ID_FIELD_NAME) + .and_then(nonempty_string) +} + +pub(crate) fn translated_slack_send_message_tool_params( + connector_name: Option<&str>, + tool_title: Option<&str>, + tool_params: Option<&Value>, + channel_name: Option<&str>, +) -> Option { + let tool_params = tool_params?; + if !is_slack_send_message_tool(connector_name, tool_title) { + return Some(tool_params.clone()); + } + + let Some(channel_name) = channel_name.map(str::trim).filter(|name| !name.is_empty()) else { + return Some(tool_params.clone()); + }; + let Value::Object(tool_params) = tool_params else { + return Some(tool_params.clone()); + }; + + let mut translated = tool_params.clone(); + translated.insert( + TO_FIELD_NAME.to_string(), + Value::String(channel_name.to_string()), + ); + Some(Value::Object(translated)) +} + +fn is_slack_profile_tool(connector_name: Option<&str>, tool_title: Option<&str>) -> bool { + is_slack_connector(connector_name) + && matches!( + tool_title.map(str::trim), + Some(SLACK_GET_PROFILE_TOOL_TITLE) | Some(SLACK_READ_USER_PROFILE_TOOL_TITLE) + ) +} + +fn is_slack_send_message_tool(connector_name: Option<&str>, tool_title: Option<&str>) -> bool { + is_slack_connector(connector_name) + && matches!( + tool_title.map(str::trim), + Some(SLACK_SEND_MESSAGE_TOOL_TITLE) + ) +} + +fn is_slack_connector(connector_name: Option<&str>) -> bool { + connector_name + .map(str::trim) + .is_some_and(|connector_name| connector_name.starts_with("Slack")) +} + +fn parse_payload_from_text_content(content: &[Value]) -> Option { + let text = raw_text_content(content)?; + serde_json::from_str(text).ok() +} + +fn raw_text_content(content: &[Value]) -> Option<&str> { + let [content_block] = content else { + return None; + }; + let content_block = content_block.as_object()?; + if content_block.get("type").and_then(Value::as_str) != Some("text") { + return None; + } + + content_block.get("text").and_then(nonempty_string) +} + +fn slack_channel_name_from_payload(payload: &Value) -> Option { + let payload = payload.as_object()?; + if let Some(result_text) = payload.get(RESULT_FIELD_NAME).and_then(nonempty_string) { + return parse_slack_channel_name_from_text(result_text); + } + if let Some(result_object) = payload.get(RESULT_FIELD_NAME).and_then(Value::as_object) { + return slack_channel_name_from_profile_object(result_object); + } + + slack_channel_name_from_profile_object(payload) +} + +fn slack_channel_name_from_profile_object( + profile: &Map, +) -> Option { + let channel_id = profile + .get(ID_FIELD_NAME) + .and_then(nonempty_string)? + .to_string(); + let channel_name = readable_slack_channel_name(profile)?; + + Some(SlackChannelName { + channel_id, + channel_name, + }) +} + +fn readable_slack_channel_name(profile: &Map) -> Option { + if let Some(nickname) = profile.get(NICKNAME_FIELD_NAME).and_then(nonempty_string) { + return Some(format_slack_handle(nickname)); + } + + profile + .get(NAME_FIELD_NAME) + .and_then(nonempty_string) + .map(ToString::to_string) +} + +fn format_slack_handle(handle: &str) -> String { + if handle.starts_with('@') { + handle.to_string() + } else { + format!("@{handle}") + } +} + +fn parse_slack_channel_name_from_text(text: &str) -> Option { + let lines = text + .lines() + .map(str::trim) + .filter(|line| !line.is_empty()) + .collect::>(); + let channel_id = labeled_multiline_value(&lines, "User ID")?; + let channel_name = + handle_from_profile_lines(&lines).or_else(|| labeled_multiline_value(&lines, "Name"))?; + + Some(SlackChannelName { + channel_id, + channel_name, + }) +} + +fn handle_from_profile_lines(lines: &[&str]) -> Option { + let header = *lines.first()?; + let open_index = header.rfind('(')?; + let close_index = header.rfind(')')?; + if close_index <= open_index { + return None; + } + + let candidate = header[open_index + 1..close_index].trim(); + is_plausible_slack_handle(candidate).then(|| format_slack_handle(candidate)) +} + +fn is_plausible_slack_handle(candidate: &str) -> bool { + !candidate.is_empty() + && candidate + .chars() + .all(|ch| ch.is_ascii_alphanumeric() || matches!(ch, '_' | '-' | '.')) +} + +fn labeled_multiline_value(lines: &[&str], label: &str) -> Option { + let prefix = format!("{label}:"); + for (index, line) in lines.iter().enumerate() { + if let Some(value) = line.strip_prefix(&prefix) { + let mut combined = value.trim().to_string(); + let mut next_index = index + 1; + while next_index < lines.len() && !looks_like_field_label(lines[next_index]) { + if !combined.is_empty() { + combined.push(' '); + } + combined.push_str(lines[next_index]); + next_index += 1; + } + + return Some(combined); + } + } + + None +} + +fn looks_like_field_label(line: &str) -> bool { + let Some((label, _)) = line.split_once(':') else { + return false; + }; + !label.trim().is_empty() + && label + .chars() + .all(|ch| ch.is_ascii_alphabetic() || ch == ' ' || ch == '#') +} + +fn nonempty_string(value: &Value) -> Option<&str> { + value + .as_str() + .map(str::trim) + .filter(|value| !value.is_empty()) +} + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + use serde_json::json; + + #[test] + fn extracts_slack_channel_name_from_profile_result() { + let result = CallToolResult { + content: Vec::new(), + structured_content: Some(json!({ + "result": { + "id": "U123", + "name": "Mason Zeng", + "nickname": "mzeng", + } + })), + is_error: Some(false), + meta: None, + }; + + assert_eq!( + slack_channel_name_from_profile_result( + Some("Slack Codex App"), + Some("get_profile"), + &result + ), + Some(SlackChannelName { + channel_id: "U123".to_string(), + channel_name: "@mzeng".to_string(), + }) + ); + } + + #[test] + fn falls_back_to_text_content_when_structured_content_is_missing() { + let result = CallToolResult { + content: vec![json!({ + "type": "text", + "text": "{\"result\":\"mzeng (mzeng)\\nOpenAI\\nCodexing\\nUsers (1 results)\\n### Result 1 of 1\\nName: Matthew\\nZeng\\nUser ID: U07B9LBRPST\\nTitle: Codexing\"}", + })], + structured_content: None, + is_error: Some(false), + meta: None, + }; + + assert_eq!( + slack_channel_name_from_profile_result( + Some("Slack"), + Some("slack_read_user_profile"), + &result + ), + Some(SlackChannelName { + channel_id: "U07B9LBRPST".to_string(), + channel_name: "@mzeng".to_string(), + }) + ); + } + + #[test] + fn extracts_slack_channel_name_from_structured_text_result() { + let result = CallToolResult { + content: Vec::new(), + structured_content: Some(json!({ + "result": "mzeng (mzeng)\nOpenAI\nCodexing\nUsers (1 results)\n### Result 1 of 1\nName: Matthew\nZeng\nUser ID: U07B9LBRPST\nTitle: Codexing", + })), + is_error: Some(false), + meta: None, + }; + + assert_eq!( + slack_channel_name_from_profile_result( + Some("Slack"), + Some("slack_read_user_profile"), + &result + ), + Some(SlackChannelName { + channel_id: "U07B9LBRPST".to_string(), + channel_name: "@mzeng".to_string(), + }) + ); + } + + #[test] + fn ignores_non_slack_profile_tools() { + let result = CallToolResult { + content: Vec::new(), + structured_content: Some(json!({ + "result": { + "id": "U123", + "name": "Mason Zeng", + } + })), + is_error: Some(false), + meta: None, + }; + + assert_eq!( + slack_channel_name_from_profile_result(Some("Linear"), Some("get_profile"), &result), + None + ); + } + + #[test] + fn translates_slack_send_message_tool_params() { + assert_eq!( + translated_slack_send_message_tool_params( + Some("Slack"), + Some("slack_send_message"), + Some(&json!({ + "channel_id": "U123", + "message": "hi", + })), + Some("@mzeng"), + ), + Some(json!({ + "channel_id": "U123", + "message": "hi", + "to": "@mzeng", + })) + ); + } + + #[test] + fn leaves_non_slack_send_message_tool_params_unchanged() { + assert_eq!( + translated_slack_send_message_tool_params( + Some("Slack"), + Some("slack_search_channels"), + Some(&json!({ + "query": "eng", + })), + Some("#eng"), + ), + Some(json!({ + "query": "eng", + })) + ); + } +} diff --git a/codex-rs/core/src/state/session.rs b/codex-rs/core/src/state/session.rs index a40405d1d1..7e09ce2001 100644 --- a/codex-rs/core/src/state/session.rs +++ b/codex-rs/core/src/state/session.rs @@ -33,6 +33,8 @@ pub(crate) struct SessionState { /// Startup regular task pre-created during session initialization. pub(crate) startup_regular_task: Option>>, pub(crate) active_connector_selection: HashSet, + slack_channel_names: HashMap, + slack_channel_names_by_connector_id: HashMap>, pub(crate) pending_session_start_source: Option, granted_permissions: Option, } @@ -51,6 +53,8 @@ impl SessionState { previous_turn_settings: None, startup_regular_task: None, active_connector_selection: HashSet::new(), + slack_channel_names: HashMap::new(), + slack_channel_names_by_connector_id: HashMap::new(), pending_session_start_source: None, granted_permissions: None, } @@ -194,6 +198,44 @@ impl SessionState { self.active_connector_selection.clear(); } + pub(crate) fn record_slack_channel_name( + &mut self, + connector_id: Option<&str>, + channel_id: String, + channel_name: String, + ) { + self.slack_channel_names + .insert(channel_id.clone(), channel_name.clone()); + if let Some(connector_id) = connector_id.map(str::trim).filter(|id| !id.is_empty()) { + self.slack_channel_names_by_connector_id + .entry(connector_id.to_string()) + .or_default() + .insert(channel_id, channel_name); + } + } + + pub(crate) fn slack_channel_name( + &self, + connector_id: Option<&str>, + channel_id: &str, + ) -> Option { + let channel_id = channel_id.trim(); + if channel_id.is_empty() { + return None; + } + + connector_id + .map(str::trim) + .filter(|id| !id.is_empty()) + .and_then(|connector_id| { + self.slack_channel_names_by_connector_id + .get(connector_id)? + .get(channel_id) + .cloned() + }) + .or_else(|| self.slack_channel_names.get(channel_id).cloned()) + } + pub(crate) fn set_pending_session_start_source( &mut self, value: Option, @@ -272,6 +314,41 @@ mod tests { assert_eq!(state.get_connector_selection(), HashSet::new()); } + #[tokio::test] + async fn slack_channel_lookup_prefers_connector_match_and_falls_back_to_global() { + let session_configuration = make_session_configuration_for_tests().await; + let mut state = SessionState::new(session_configuration); + state.record_slack_channel_name( + Some("connector_a"), + "U123".to_string(), + "@alice".to_string(), + ); + + assert_eq!( + state.slack_channel_name(Some("connector_a"), "U123"), + Some("@alice".to_string()) + ); + assert_eq!( + state.slack_channel_name(Some("connector_b"), "U123"), + Some("@alice".to_string()) + ); + + state.record_slack_channel_name( + Some("connector_b"), + "U123".to_string(), + "@ally".to_string(), + ); + + assert_eq!( + state.slack_channel_name(Some("connector_a"), "U123"), + Some("@alice".to_string()) + ); + assert_eq!( + state.slack_channel_name(Some("connector_b"), "U123"), + Some("@ally".to_string()) + ); + } + #[tokio::test] async fn set_rate_limits_defaults_limit_id_to_codex_when_missing() { let session_configuration = make_session_configuration_for_tests().await;