diff --git a/Cargo.lock b/Cargo.lock index 60372ea..85cf8b7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -83,6 +83,7 @@ dependencies = [ "tokio-util", "tracing", "unicode-general-category", + "url", ] [[package]] diff --git a/docs/slack.md b/docs/slack.md index ea785e8..2be64b7 100644 --- a/docs/slack.md +++ b/docs/slack.md @@ -342,6 +342,8 @@ Durable hosts can also delegate routing and update payloads to `agentchat.channels.slack.rendering.message_request`. It validates the channel and timestamps, renders the reply, selects `chat.postMessage` or `chat.update`, and clears old attachments on updates. It does not send, authenticate or retry: +Posting as a custom username or avatar uses Slack's `chat:write.customize` bot +scope; `username` and `icon_url` apply only to new posts, not updates. ```python from agentchat import RichReply diff --git a/python/agentchat/channels/slack/rendering.pyi b/python/agentchat/channels/slack/rendering.pyi index cdd4da2..4a2c2f3 100644 --- a/python/agentchat/channels/slack/rendering.pyi +++ b/python/agentchat/channels/slack/rendering.pyi @@ -13,5 +13,6 @@ def rich_payload(reply: RichReply) -> dict[str, object]: ... def message_request( reply: RichReply, *, channel_id: str, thread_ts: str | None = ..., message_ts: str | None = ..., metadata: Mapping[str, object] | None = ..., + username: str | None = ..., icon_url: str | None = ..., fallback_blocks: bool = ..., ) -> SlackMessageRequest: ... diff --git a/src/agentchat-slack/Cargo.toml b/src/agentchat-slack/Cargo.toml index b69e2b5..bda1d53 100644 --- a/src/agentchat-slack/Cargo.toml +++ b/src/agentchat-slack/Cargo.toml @@ -19,6 +19,7 @@ tokio-util = { version = "0.7", features = ["rt"] } tracing = "0.1" unicode-general-category = "1" regex = "1" +url = "2" [dev-dependencies] axum = { version = "0.8", features = ["ws"] } diff --git a/src/agentchat-slack/src/events.rs b/src/agentchat-slack/src/events.rs index 79e2540..e4ab335 100644 --- a/src/agentchat-slack/src/events.rs +++ b/src/agentchat-slack/src/events.rs @@ -116,7 +116,9 @@ pub(crate) fn history_record(record: &Value, bot_user_id: Option<&str>) -> Optio let input = mirror.unwrap_or_else(|| super::rendering::MirroredInput { sender: Sender { id: sender.into(), - display_name: None, + display_name: (Some(sender) == bot_user_id) + .then(|| nonempty(&record["username"]).map(str::to_owned)) + .flatten(), }, text: text.into(), }); diff --git a/src/agentchat-slack/src/host.rs b/src/agentchat-slack/src/host.rs index bcd5b9d..36626af 100644 --- a/src/agentchat-slack/src/host.rs +++ b/src/agentchat-slack/src/host.rs @@ -377,7 +377,15 @@ impl ThreadContextReader { } else { input.reference }; - self.store(ts.as_str(), &input.sender.id, source); + self.store( + ts.as_str(), + input + .sender + .display_name + .as_deref() + .unwrap_or(&input.sender.id), + source, + ); } fn store(&mut self, ts: &str, author: &str, source: Value) { if source.as_object().is_none_or(|v| v.is_empty()) { diff --git a/src/agentchat-slack/src/message_request.rs b/src/agentchat-slack/src/message_request.rs index cb94e46..2b779db 100644 --- a/src/agentchat-slack/src/message_request.rs +++ b/src/agentchat-slack/src/message_request.rs @@ -4,6 +4,7 @@ use crate::{policy::ReplyDestination, rendering, route::SlackTimestamp}; use agentchat_core::{Error, Result, RichReply}; use serde::Serialize; use serde_json::{json, Value}; +use url::Url; #[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize)] pub enum MessageMethod { @@ -13,6 +14,47 @@ pub enum MessageMethod { Update, } +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct SlackAuthor { + pub username: String, + pub icon_url: Option, +} + +impl SlackAuthor { + fn validated(&self) -> Result { + let username = self + .username + .split_whitespace() + .collect::>() + .join(" "); + if username.is_empty() || username.chars().count() > 80 { + return Err(Error::InvalidInput( + "Slack author username must contain 1 to 80 characters".into(), + )); + } + if let Some(icon_url) = &self.icon_url { + let parsed = Url::parse(icon_url).map_err(|_| { + Error::InvalidInput("Slack author icon_url must be an HTTPS URL".into()) + })?; + if icon_url.chars().count() > 2048 + || parsed.scheme() != "https" + || parsed.host_str().is_none() + || !parsed.username().is_empty() + || parsed.password().is_some() + { + return Err(Error::InvalidInput( + "Slack author icon_url must be an HTTPS URL without userinfo, at most 2048 characters" + .into(), + )); + } + } + Ok(Self { + username, + icon_url: self.icon_url.clone(), + }) + } +} + /// A prepared request, with no transport, retry, authorization or queue policy. #[derive(Clone, Debug, Serialize)] pub struct MessageRequest { @@ -27,6 +69,7 @@ impl MessageRequest { thread_ts: Option<&str>, message_ts: Option<&str>, metadata: Option<&Value>, + author: Option<&SlackAuthor>, fallback_blocks: bool, ) -> Result { let destination = ReplyDestination::validate(channel_id, thread_ts)?; @@ -41,6 +84,7 @@ impl MessageRequest { "Slack message metadata must be an object".into(), )); } + let author = author.map(SlackAuthor::validated).transpose()?; let mut payload = rendering::rich_payload_with_fallback_blocks(reply, fallback_blocks)?; payload["channel"] = json!(destination.channel_id); let method = if let Some(timestamp) = timestamp { @@ -55,6 +99,12 @@ impl MessageRequest { if let Some(thread) = destination.thread_ts { payload["thread_ts"] = json!(thread); } + if let Some(author) = author { + payload["username"] = json!(author.username); + if let Some(icon_url) = author.icon_url { + payload["icon_url"] = json!(icon_url); + } + } MessageMethod::Post }; if let Some(metadata) = metadata { @@ -85,6 +135,7 @@ mod tests { Some("123.001"), None, Some(&metadata), + None, true, ) .unwrap(); @@ -107,6 +158,7 @@ mod tests { Some("123.001"), Some("123.002"), None, + None, true, ) .unwrap(); @@ -124,16 +176,81 @@ mod tests { ("C123", Some("bad"), None), ("C123", None, Some("bad")), ] { - assert!(MessageRequest::new(&reply(), channel, thread, message, None, true).is_err()); + assert!( + MessageRequest::new(&reply(), channel, thread, message, None, None, true).is_err() + ); } - assert!(MessageRequest::new(&reply(), "C123", None, None, Some(&json!([])), true).is_err()); + assert!( + MessageRequest::new(&reply(), "C123", None, None, Some(&json!([])), None, true) + .is_err() + ); } #[test] fn preserves_explicit_empty_blocks_for_card_only_and_plain_updates() { let request = - MessageRequest::new(&reply(), "C123", None, Some("123.002"), None, false).unwrap(); + MessageRequest::new(&reply(), "C123", None, Some("123.002"), None, None, false) + .unwrap(); assert_eq!(request.payload["blocks"], json!([])); assert_eq!(request.payload["attachments"], json!([])); } + + #[test] + fn posts_with_a_validated_author_override() { + let author = SlackAuthor { + username: " Ishaan \n Jaffer ".into(), + icon_url: Some("https://example.com/avatar.png".into()), + }; + let request = + MessageRequest::new(&reply(), "C123", None, None, None, Some(&author), true).unwrap(); + assert_eq!(request.payload["username"], "Ishaan Jaffer"); + assert_eq!( + request.payload["icon_url"], + "https://example.com/avatar.png" + ); + } + + #[test] + fn updates_ignore_author_overrides() { + let author = SlackAuthor { + username: "Ishaan".into(), + icon_url: Some("https://example.com/avatar.png".into()), + }; + let request = MessageRequest::new( + &reply(), + "C123", + None, + Some("123.002"), + None, + Some(&author), + true, + ) + .unwrap(); + assert_eq!(request.method, MessageMethod::Update); + assert!(request.payload.get("username").is_none()); + assert!(request.payload.get("icon_url").is_none()); + } + + #[test] + fn rejects_invalid_author_overrides() { + for author in [ + SlackAuthor { + username: " \n\t ".into(), + icon_url: None, + }, + SlackAuthor { + username: "Ishaan".into(), + icon_url: Some("http://example.com/avatar.png".into()), + }, + SlackAuthor { + username: "Ishaan".into(), + icon_url: Some("https://user:pass@example.com/avatar.png".into()), + }, + ] { + assert!( + MessageRequest::new(&reply(), "C123", None, None, None, Some(&author), true) + .is_err() + ); + } + } } diff --git a/src/bindings/python/src/providers/slack_policy.rs b/src/bindings/python/src/providers/slack_policy.rs index 0155c6e..cc9d17d 100644 --- a/src/bindings/python/src/providers/slack_policy.rs +++ b/src/bindings/python/src/providers/slack_policy.rs @@ -65,7 +65,7 @@ fn slack_rich_payload(py: Python<'_>, content: &Bound<'_, PyAny>) -> PyResult, reply: &Bound<'_, PyAny>, @@ -73,15 +73,25 @@ fn slack_message_request( thread_ts: Option<&str>, message_ts: Option<&str>, metadata: Option<&Bound<'_, PyAny>>, + username: Option, + icon_url: Option, fallback_blocks: bool, ) -> PyResult> { let metadata = metadata.map(convert::to_json).transpose()?; + if username.is_none() && icon_url.is_some() { + return Err(PyValueError::new_err( + "Slack message icon_url requires a username", + )); + } + let author = username + .map(|username| agentchat_slack::message_request::SlackAuthor { username, icon_url }); let request = agentchat_slack::message_request::MessageRequest::new( &convert::from_rich_reply(reply)?, channel_id, thread_ts, message_ts, metadata.as_ref(), + author.as_ref(), fallback_blocks, ) .map_err(policy_error)?; diff --git a/tests/channels/slack/test_host.py b/tests/channels/slack/test_host.py index 30fbef1..7c2085e 100644 --- a/tests/channels/slack/test_host.py +++ b/tests/channels/slack/test_host.py @@ -123,6 +123,18 @@ async def request(params): ] +@pytest.mark.asyncio +async def test_custom_bot_username_is_attributed_in_thread_context(): + async def request(_): + return page([ + message("1780000000.000005", text="continue", user=BOT, username="Ishaan Jaffer"), + message(ORIGIN["message_ts"], text="What next?"), + ]) + + context = await HOST.thread_context(ORIGIN, request) + assert context["messages"][0]["author"] == "Ishaan Jaffer" + + @pytest.mark.asyncio @pytest.mark.parametrize("first_page", [True, False]) async def test_provider_failure_retains_good_pages_and_reports_coverage(first_page): diff --git a/tests/channels/slack/test_message_request.py b/tests/channels/slack/test_message_request.py index 7a9540e..8fbd423 100644 --- a/tests/channels/slack/test_message_request.py +++ b/tests/channels/slack/test_message_request.py @@ -47,11 +47,30 @@ def test_card_only_reply_does_not_repeat_its_accessible_fallback(): assert request["payload"]["attachments"] == [card] +def test_author_override_is_used_for_posts_and_ignored_for_updates(): + posted = message_request( + RichReply("Continue"), channel_id="C123", + username="Ishaan Jaffer", icon_url="https://example.com/avatar.png", + ) + assert posted["payload"]["username"] == "Ishaan Jaffer" + assert posted["payload"]["icon_url"] == "https://example.com/avatar.png" + + updated = message_request( + RichReply("Continue"), channel_id="C123", message_ts="123.002", + username="Ishaan Jaffer", icon_url="https://example.com/avatar.png", + ) + assert "username" not in updated["payload"] + assert "icon_url" not in updated["payload"] + + @pytest.mark.parametrize("kwargs", [ {"channel_id": "invalid"}, {"channel_id": "C123", "thread_ts": "invalid"}, {"channel_id": "C123", "message_ts": "invalid"}, {"channel_id": "C123", "metadata": []}, + {"channel_id": "C123", "username": " "}, + {"channel_id": "C123", "username": "Ishaan", "icon_url": "http://example.com/avatar.png"}, + {"channel_id": "C123", "icon_url": "https://example.com/avatar.png"}, ]) def test_request_rejects_invalid_provider_inputs(kwargs): with pytest.raises(ValueError):