diff --git a/crates/cli/src/app.rs b/crates/cli/src/app.rs index 1e1df9ff..e874db2a 100644 --- a/crates/cli/src/app.rs +++ b/crates/cli/src/app.rs @@ -40496,6 +40496,9 @@ mod tests { allowed_channel_count: 0, allowed_workspaces: Vec::new(), allowed_channels: Vec::new(), + progress: None, + follow_up: None, + thread_context: None, attached_to: Some(name.to_string()), publication: None, }], @@ -40862,6 +40865,9 @@ mod tests { allowed_channel_count: 0, allowed_workspaces: Vec::new(), allowed_channels: Vec::new(), + progress: None, + follow_up: None, + thread_context: None, attached_to: None, publication: None, }) @@ -41839,6 +41845,262 @@ mod tests { server.abort(); } + /// Open the editor on a fresh Slack channel, which is where the behavior + /// options come seeded with the defaults a definition would get. + async fn slack_channel_editor() -> (App, tempfile::TempDir, tokio::task::JoinHandle<()>) { + let (mut app, dir, server) = captured_app().await; + app.services.push(service_summary_for_test("assistant")); + app.service_channel_catalog = app.services[0].channels.clone(); + app.open_edit_service_view("assistant"); + app.open_new_service_channel(); + app.service_dialog + .as_mut() + .unwrap() + .channel_editor + .as_mut() + .unwrap() + .selected_field = 1; + app.on_key(KeyEvent::new(KeyCode::Char(' '), KeyModifiers::NONE)) + .await; + (app, dir, server) + } + + fn channel_editor(app: &App) -> &crate::app::ServiceChannelDialog { + app.service_dialog + .as_ref() + .unwrap() + .channel_editor + .as_ref() + .unwrap() + } + + #[tokio::test] + async fn slack_channel_editor_cycles_the_behavior_options() { + use crate::app::service_dialog::{ + SLACK_FIELD_FOLLOW_UP, SLACK_FIELD_PROGRESS, SLACK_FIELD_STATE, + }; + + let (mut app, _dir, server) = slack_channel_editor().await; + // Switching a new channel to Slack seeds what its definition would get, + // so the editor never offers three blanks to save. + assert_eq!( + channel_editor(&app).channel.progress.as_deref(), + Some(construct_protocol::SLACK_PROGRESS_DEFAULT) + ); + assert_eq!( + channel_editor(&app).channel.follow_up.as_deref(), + Some(construct_protocol::SLACK_FOLLOW_UP_DEFAULT) + ); + assert_eq!( + channel_editor(&app).channel.thread_context, + Some(construct_protocol::SLACK_THREAD_CONTEXT_DEFAULT) + ); + + // The options sit below the allowlists and above State, so a Slack + // channel has ten fields and Ctrl+N wraps from the last back to the ID. + app.service_dialog + .as_mut() + .unwrap() + .channel_editor + .as_mut() + .unwrap() + .selected_field = SLACK_FIELD_STATE; + app.on_key(KeyEvent::new(KeyCode::Char('n'), KeyModifiers::CONTROL)) + .await; + assert_eq!(channel_editor(&app).selected_field, 0); + + // Space walks the published order and wraps; ← steps the other way. + app.service_dialog + .as_mut() + .unwrap() + .channel_editor + .as_mut() + .unwrap() + .selected_field = SLACK_FIELD_PROGRESS; + let mut seen = vec![channel_editor(&app) + .channel + .progress + .clone() + .expect("seeded")]; + for _ in 1..construct_protocol::SLACK_PROGRESS_VALUES.len() { + app.on_key(KeyEvent::new(KeyCode::Char(' '), KeyModifiers::NONE)) + .await; + seen.push(channel_editor(&app).channel.progress.clone().unwrap()); + } + let mut expected = construct_protocol::SLACK_PROGRESS_VALUES.to_vec(); + expected.sort_unstable(); + let mut walked = seen.clone(); + walked.sort_unstable(); + assert_eq!( + walked, expected, + "cycling visits every published value exactly once: {seen:?}" + ); + app.on_key(KeyEvent::new(KeyCode::Char(' '), KeyModifiers::NONE)) + .await; + assert_eq!( + channel_editor(&app).channel.progress.as_deref(), + Some(construct_protocol::SLACK_PROGRESS_DEFAULT), + "one full turn returns to where it started" + ); + app.on_key(KeyEvent::new(KeyCode::Left, KeyModifiers::NONE)) + .await; + let values = construct_protocol::SLACK_PROGRESS_VALUES; + let default_index = values + .iter() + .position(|value| *value == construct_protocol::SLACK_PROGRESS_DEFAULT) + .expect("the default is one of the published values"); + assert_eq!( + channel_editor(&app).channel.progress.as_deref(), + Some(values[(default_index + values.len() - 1) % values.len()]), + "← steps back one, wrapping at the start" + ); + + app.service_dialog + .as_mut() + .unwrap() + .channel_editor + .as_mut() + .unwrap() + .selected_field = SLACK_FIELD_FOLLOW_UP; + app.on_key(KeyEvent::new(KeyCode::Right, KeyModifiers::NONE)) + .await; + let after_right = channel_editor(&app).channel.follow_up.clone().unwrap(); + assert_ne!(after_right, construct_protocol::SLACK_FOLLOW_UP_DEFAULT); + assert!(construct_protocol::SLACK_FOLLOW_UP_VALUES.contains(&after_right.as_str())); + app.on_key(KeyEvent::new(KeyCode::Left, KeyModifiers::NONE)) + .await; + assert_eq!( + channel_editor(&app).channel.follow_up.as_deref(), + Some(construct_protocol::SLACK_FOLLOW_UP_DEFAULT), + "← undoes →" + ); + + // Cycling never reaches the state toggle, and the toggle never reaches + // the options. + app.service_dialog + .as_mut() + .unwrap() + .channel_editor + .as_mut() + .unwrap() + .selected_field = SLACK_FIELD_STATE; + let before = channel_editor(&app).channel.follow_up.clone(); + app.on_key(KeyEvent::new(KeyCode::Char(' '), KeyModifiers::NONE)) + .await; + assert!(!channel_editor(&app).channel.enabled); + assert_eq!(channel_editor(&app).channel.follow_up, before); + + server.abort(); + } + + #[tokio::test] + async fn slack_channel_editor_takes_a_typed_thread_context() { + use crate::app::service_dialog::SLACK_FIELD_THREAD_CONTEXT; + + let (mut app, _dir, server) = slack_channel_editor().await; + app.service_dialog + .as_mut() + .unwrap() + .channel_editor + .as_mut() + .unwrap() + .selected_field = SLACK_FIELD_THREAD_CONTEXT; + + // Backspacing the seeded default all the way out reads as none rather + // than as "leave it alone" — 0 is a setting an operator chooses. + for _ in 0..8 { + app.on_key(KeyEvent::new(KeyCode::Backspace, KeyModifiers::NONE)) + .await; + } + assert_eq!(channel_editor(&app).channel.thread_context, Some(0)); + + for ch in "120".chars() { + app.on_key(KeyEvent::new(KeyCode::Char(ch), KeyModifiers::NONE)) + .await; + } + assert_eq!(channel_editor(&app).channel.thread_context, Some(120)); + + // Past Slack's own page limit the field holds at the cap instead of + // letting the operator save a number the daemon would refuse. + for ch in "000".chars() { + app.on_key(KeyEvent::new(KeyCode::Char(ch), KeyModifiers::NONE)) + .await; + } + assert_eq!( + channel_editor(&app).channel.thread_context, + Some(construct_protocol::SLACK_THREAD_CONTEXT_MAX) + ); + + server.abort(); + } + + #[tokio::test] + async fn slack_channel_editor_renders_the_behavior_options() { + use crate::app::service_dialog::SLACK_FIELD_THREAD_CONTEXT; + + let (mut app, _dir, server) = slack_channel_editor().await; + let backend = ratatui::backend::TestBackend::new(160, 24); + let mut term = ratatui::Terminal::new(backend).expect("terminal"); + // Drawn on its own rather than through the whole frame: the pane the + // editor shares with the animated panel repaints rows every frame, and + // this test is about the editor. + let mut draw = |app: &mut App| { + let editor = channel_editor(app).clone(); + term.draw(|f| { + crate::ui::render_service_channel_editor(f, f.area(), app, &editor); + }) + .expect("draw"); + rendered_text(term.backend().buffer()) + }; + + let text = draw(&mut app); + for label in ["Progress", "Follow-up", "Thread context"] { + assert!(text.contains(label), "{label} is missing: {text}"); + } + assert!( + text.contains(construct_protocol::SLACK_PROGRESS_DEFAULT), + "the seeded progress value is shown: {text}" + ); + assert!( + text.contains(construct_protocol::SLACK_FOLLOW_UP_DEFAULT), + "{text}" + ); + assert!(text.contains("50 messages"), "{text}"); + + // Selecting the field explains what it needs from the Slack app, and + // what admitting other people's text into a session costs. + app.service_dialog + .as_mut() + .unwrap() + .channel_editor + .as_mut() + .unwrap() + .selected_field = SLACK_FIELD_THREAD_CONTEXT; + let text = draw(&mut app); + assert!(text.contains("channels:history"), "{text}"); + + // Zero is a setting, not an empty field, and says so. + app.service_dialog + .as_mut() + .unwrap() + .channel_editor + .as_mut() + .unwrap() + .channel + .thread_context = Some(0); + let text = draw(&mut app); + assert!(text.contains("0 · none"), "{text}"); + + // An HTTP channel has no such options and is not asked about them. + app.open_new_service_channel(); + let text = draw(&mut app); + assert_eq!(channel_editor(&app).channel.kind, "http"); + assert!(!text.contains("Follow-up"), "{text}"); + assert_eq!(channel_editor(&app).channel.progress, None); + + server.abort(); + } + #[tokio::test] async fn service_view_lists_available_and_owned_catalog_channels() { let (mut app, _dir, server) = captured_app().await; @@ -41857,6 +42119,9 @@ mod tests { allowed_channel_count: 0, allowed_workspaces: Vec::new(), allowed_channels: Vec::new(), + progress: None, + follow_up: None, + thread_context: None, attached_to: None, publication: None, }, @@ -41872,6 +42137,9 @@ mod tests { allowed_channel_count: 0, allowed_workspaces: Vec::new(), allowed_channels: Vec::new(), + progress: None, + follow_up: None, + thread_context: None, attached_to: Some("other-service".to_string()), publication: None, }, diff --git a/crates/cli/src/app/service_dialog.rs b/crates/cli/src/app/service_dialog.rs index fae07416..604614d5 100644 --- a/crates/cli/src/app/service_dialog.rs +++ b/crates/cli/src/app/service_dialog.rs @@ -233,14 +233,37 @@ pub enum ServiceChannelAction { CopyAddress, } +/// Field indexes shared by the Slack channel editor's navigation, rendering, +/// and key handling. State stays last so it sits where an HTTP channel's does. +pub(crate) const SLACK_FIELD_PROGRESS: usize = 6; +pub(crate) const SLACK_FIELD_FOLLOW_UP: usize = 7; +pub(crate) const SLACK_FIELD_THREAD_CONTEXT: usize = 8; +pub(crate) const SLACK_FIELD_STATE: usize = 9; +const HTTP_FIELD_STATE: usize = 3; + fn channel_field_count(editor: &ServiceChannelDialog) -> usize { if editor.channel.kind == "slack" { - 7 + SLACK_FIELD_STATE + 1 } else { - 4 + HTTP_FIELD_STATE + 1 } } +/// Step through a fixed list of option values, wrapping in both directions. +/// The list is the protocol's, so the editor can only offer what the daemon +/// will accept. +fn cycle_option(values: &[&str], current: Option<&str>, forward: bool) -> String { + let index = current + .and_then(|current| values.iter().position(|value| *value == current)) + .unwrap_or(0); + let next = if forward { + (index + 1) % values.len() + } else { + index.checked_sub(1).unwrap_or(values.len() - 1) + }; + values[next].to_string() +} + fn canonical_service_model(model: &str) -> String { construct_protocol::published_model::decode_published_model_id(model) .ok() @@ -681,6 +704,11 @@ impl App { allowed_channel_count: 0, allowed_workspaces: Vec::new(), allowed_channels: Vec::new(), + // Filled in when the kind is switched to Slack; an HTTP channel + // has no behavior options to show. + progress: None, + follow_up: None, + thread_context: None, attached_to: Some(dialog.service.name.clone()), publication: None, }, @@ -1063,6 +1091,25 @@ impl App { channel.channel.allowed_channels = split_allowlist(&value); channel.channel.allowed_channel_count = channel.channel.allowed_channels.len(); } + SLACK_FIELD_THREAD_CONTEXT if channel.channel.kind == "slack" => { + let mut value = channel + .channel + .thread_context + .map(|count| count.to_string()) + .unwrap_or_default(); + edit(&mut value); + // An emptied field reads as none rather than as "unchanged": + // the operator is typing a number, and 0 is a real setting. + channel.channel.thread_context = if value.is_empty() { + Some(0) + } else { + value + .parse::() + .ok() + .map(|count| count.min(construct_protocol::SLACK_THREAD_CONTEXT_MAX)) + .or(channel.channel.thread_context) + }; + } _ => return, } channel.note = None; @@ -1078,6 +1125,7 @@ impl App { return; }; let editor_snapshot = editor.clone(); + let slack = editor_snapshot.channel.kind == "slack"; let valid_id = valid_service_name(&editor_snapshot.channel.id); let validation_error = if !valid_id { Some("Channel ID must be 1–32 lowercase letters, digits, or interior hyphens.") @@ -1119,6 +1167,13 @@ impl App { .then_some(editor_snapshot.bot_token), allowed_workspaces: editor_snapshot.channel.allowed_workspaces, allowed_channels: editor_snapshot.channel.allowed_channels, + // Slack-only: sending these for an HTTP channel is refused, + // and the editor never shows them there. + progress: slack.then_some(editor_snapshot.channel.progress).flatten(), + follow_up: slack.then_some(editor_snapshot.channel.follow_up).flatten(), + thread_context: slack + .then_some(editor_snapshot.channel.thread_context) + .flatten(), }, rotate_secret, }) @@ -1869,15 +1924,26 @@ impl App { "http" } .to_string(); - editor.channel.port = (editor.channel.kind == "http").then_some(8787); + let slack = editor.channel.kind == "slack"; + editor.channel.port = (!slack).then_some(8787); + // Show a new Slack channel the values it will be saved + // with rather than three blanks. + editor.channel.progress = slack + .then(|| construct_protocol::SLACK_PROGRESS_DEFAULT.to_string()); + editor.channel.follow_up = slack + .then(|| construct_protocol::SLACK_FOLLOW_UP_DEFAULT.to_string()); + editor.channel.thread_context = + slack.then_some(construct_protocol::SLACK_THREAD_CONTEXT_DEFAULT); editor.selected_field = 1; editor.note = None; } } } KeyCode::Left | KeyCode::Right | KeyCode::Char(' ') - if (snapshot.channel.kind == "http" && snapshot.selected_field == 3) - || (snapshot.channel.kind == "slack" && snapshot.selected_field == 6) => + if (snapshot.channel.kind == "http" + && snapshot.selected_field == HTTP_FIELD_STATE) + || (snapshot.channel.kind == "slack" + && snapshot.selected_field == SLACK_FIELD_STATE) => { if let Some(dialog) = self.service_dialog.as_mut() { if let Some(editor) = dialog.channel_editor.as_mut() { @@ -1885,6 +1951,34 @@ impl App { } } } + KeyCode::Left | KeyCode::Right | KeyCode::Char(' ') + if snapshot.channel.kind == "slack" + && matches!( + snapshot.selected_field, + SLACK_FIELD_PROGRESS | SLACK_FIELD_FOLLOW_UP + ) => + { + // More than two values, so Left has to mean the other way. + let forward = key.code != KeyCode::Left; + if let Some(dialog) = self.service_dialog.as_mut() { + if let Some(editor) = dialog.channel_editor.as_mut() { + if editor.selected_field == SLACK_FIELD_PROGRESS { + editor.channel.progress = Some(cycle_option( + construct_protocol::SLACK_PROGRESS_VALUES, + editor.channel.progress.as_deref(), + forward, + )); + } else { + editor.channel.follow_up = Some(cycle_option( + construct_protocol::SLACK_FOLLOW_UP_VALUES, + editor.channel.follow_up.as_deref(), + forward, + )); + } + editor.note = None; + } + } + } KeyCode::Backspace => self.edit_service_channel_text(|value| { value.pop(); }), diff --git a/crates/cli/src/ui.rs b/crates/cli/src/ui.rs index d57a9c56..83744efe 100644 --- a/crates/cli/src/ui.rs +++ b/crates/cli/src/ui.rs @@ -8638,7 +8638,7 @@ fn render_service_view(f: &mut Frame, area: Rect, app: &mut App, name: &str, foc } } -fn render_service_channel_editor( +pub(crate) fn render_service_channel_editor( f: &mut Frame, area: Rect, app: &mut App, @@ -8664,6 +8664,28 @@ fn render_service_channel_editor( ), ("Workspaces", editor.channel.allowed_workspaces.join(",")), ("Channels", editor.channel.allowed_channels.join(",")), + ( + "Progress", + editor.channel.progress.clone().unwrap_or_default(), + ), + ( + "Follow-up", + editor.channel.follow_up.clone().unwrap_or_default(), + ), + ( + "Thread context", + editor + .channel + .thread_context + .map(|count| { + if count == 0 { + "0 · none".to_string() + } else { + format!("{count} messages") + } + }) + .unwrap_or_default(), + ), ("State", state), ] } else { @@ -8705,7 +8727,7 @@ fn render_service_channel_editor( Style::default().fg(app.theme.text) }; fields.push(Line::from(Span::styled( - format!("{marker} {label:<12} {value}"), + format!("{marker} {label:<14} {value}"), style, ))); } @@ -8731,7 +8753,10 @@ fn render_service_channel_editor( 3 => ("Bot token", "Slack bot token used only to post final replies into the originating thread.", "Paste an xoxb- token; blank preserves the current token."), 4 => ("Workspace allowlist", "Optional comma-separated Slack team IDs. Empty accepts configured events from any workspace.", "Type workspace IDs separated by commas."), 5 => ("Channel allowlist", "Optional comma-separated Slack channel or DM IDs. Empty accepts every channel delivered to the app.", "Type channel IDs separated by commas."), - 6 => ("State", "Disabled Slack channels close their Socket Mode connection while preserving configuration.", "Space or ←/→ toggles · applies immediately."), + 6 => ("Progress", "What the thread shows while a turn is still running: off says nothing, placeholder posts a message that becomes the answer, reaction marks the triggering message, both does each. Reactions need the reactions:write scope.", "Space or → next · ← previous · reconnects on save."), + 7 => ("Follow-up", "Where the bot keeps answering once addressed: off is mentions and DMs only, thread answers later messages in a thread it was mentioned in, channel answers everything in a channel it has been mentioned in. Anything past off needs the message.channels subscription.", "Space or → next · ← previous · reconnects on save."), + 8 => ("Thread context", "Earlier messages of a thread to read when first pulled into one; 0 reads none. Needs channels:history. This is text written by everyone in the thread, put in front of a session that holds tools — keep it at 0 where the participants are not people you would let instruct the agent.", "Type a number up to 1000 · reconnects on save."), + 9 => ("State", "Disabled Slack channels close their Socket Mode connection while preserving configuration.", "Space or ←/→ toggles · applies immediately."), _ => ("Channel", "", ""), } } else { diff --git a/crates/daemon/assets/index.html b/crates/daemon/assets/index.html index 1c1057ba..d105de95 100644 --- a/crates/daemon/assets/index.html +++ b/crates/daemon/assets/index.html @@ -4786,6 +4786,19 @@

}; } +// Mirrors the protocol's published option lists; the daemon rejects anything +// outside them, so the form must not offer anything else. +const SLACK_PROGRESS_VALUES = ["off", "placeholder", "reaction", "both"]; +const SLACK_FOLLOW_UP_VALUES = ["off", "thread", "channel"]; +const SLACK_PROGRESS_DEFAULT = "placeholder"; +const SLACK_FOLLOW_UP_DEFAULT = "thread"; +const SLACK_THREAD_CONTEXT_DEFAULT = 50; +const SLACK_THREAD_CONTEXT_MAX = 1000; + +function slackOptionMarkup(values, selected) { + return values.map((value) => ``).join(""); +} + function nextServiceChannelId(service) { const channels = service?.channels || []; if (!channels.some((channel) => channel.id === "http")) return "http"; @@ -4818,6 +4831,9 @@

bot_token: "", allowed_workspaces: [], allowed_channels: [], + progress: SLACK_PROGRESS_DEFAULT, + follow_up: SLACK_FOLLOW_UP_DEFAULT, + thread_context: SLACK_THREAD_CONTEXT_DEFAULT, }; renderServiceView(); const input = serviceActivityEl.querySelector('[data-channel-field="id"]'); @@ -4878,6 +4894,14 @@

}; if (kind === "slack" && draft.app_token) channel.app_token = draft.app_token; if (kind === "slack" && draft.bot_token) channel.bot_token = draft.bot_token; + // Slack-only, and omitting one keeps the stored value. + if (kind === "slack") { + if (draft.progress) channel.progress = draft.progress; + if (draft.follow_up) channel.follow_up = draft.follow_up; + if (Number.isFinite(Number(draft.thread_context))) { + channel.thread_context = Math.max(0, Math.min(SLACK_THREAD_CONTEXT_MAX, Number(draft.thread_context))); + } + } const result = await rpc("service.channel.put", { service_name: draft.serviceName, channel, @@ -5096,7 +5120,7 @@

}).join("") : `
No channels in the catalog.
`; const channelSpecificFields = channelDraft?.kind === "slack" - ? `` + ? `` : ``; const channelEditor = channelDraft ? `
${channelSpecificFields}
` : ""; const attachedCount = channels.filter((channel) => channel.attached_to === name).length; @@ -5135,6 +5159,7 @@

if (!state.serviceChannelDraft) return; const key = control.dataset.channelField; if (key === "port") state.serviceChannelDraft.port = Number(control.value) || 0; + else if (key === "thread_context") state.serviceChannelDraft.thread_context = Math.max(0, Math.min(SLACK_THREAD_CONTEXT_MAX, Number(control.value) || 0)); else if (key === "enabled") state.serviceChannelDraft.enabled = control.value === "true"; else if (key === "allowed_workspaces" || key === "allowed_channels") state.serviceChannelDraft[key] = control.value.split(",").map((value) => value.trim()).filter(Boolean); else state.serviceChannelDraft[key] = control.value; diff --git a/crates/daemon/src/service.rs b/crates/daemon/src/service.rs index ef265379..c7e6181b 100644 --- a/crates/daemon/src/service.rs +++ b/crates/daemon/src/service.rs @@ -9,6 +9,13 @@ mod ingress; mod slack; use anyhow::{anyhow, Context, Result}; +// The accepted values for a channel's behavior options are published by the +// protocol so that what a client offers and what the daemon accepts cannot +// drift apart. +use construct_protocol::{ + SLACK_FOLLOW_UP_VALUES as FOLLOW_UP_VALUES, SLACK_PROGRESS_VALUES as PROGRESS_VALUES, + SLACK_THREAD_CONTEXT_DEFAULT, SLACK_THREAD_CONTEXT_MAX as THREAD_CONTEXT_MAX, +}; use serde::{Deserialize, Serialize}; use std::collections::{BTreeMap, HashMap}; use std::path::PathBuf; @@ -159,6 +166,25 @@ impl SlackProgress { pub(crate) fn reacts(self) -> bool { matches!(self, Self::Reaction | Self::Both) } + + pub(crate) fn as_str(self) -> &'static str { + match self { + Self::Off => "off", + Self::Placeholder => "placeholder", + Self::Reaction => "reaction", + Self::Both => "both", + } + } + + fn parse(value: &str) -> Result { + match value { + "off" => Ok(Self::Off), + "placeholder" => Ok(Self::Placeholder), + "reaction" => Ok(Self::Reaction), + "both" => Ok(Self::Both), + other => Err(unknown_option("progress", other, PROGRESS_VALUES)), + } + } } /// Where a Slack channel keeps listening after it has been addressed. @@ -184,8 +210,34 @@ pub enum SlackFollowUp { Channel, } +impl SlackFollowUp { + pub(crate) fn as_str(self) -> &'static str { + match self { + Self::Off => "off", + Self::Thread => "thread", + Self::Channel => "channel", + } + } + + fn parse(value: &str) -> Result { + match value { + "off" => Ok(Self::Off), + "thread" => Ok(Self::Thread), + "channel" => Ok(Self::Channel), + other => Err(unknown_option("follow_up", other, FOLLOW_UP_VALUES)), + } + } +} + +fn unknown_option(field: &str, value: &str, accepted: &[&str]) -> anyhow::Error { + anyhow::anyhow!( + "unknown {field} value {value:?}; expected one of {}", + accepted.join(", ") + ) +} + fn default_thread_context() -> usize { - 50 + SLACK_THREAD_CONTEXT_DEFAULT } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -526,32 +578,57 @@ pub fn put_channel( if params.channel.kind == "slack" { validate_slack_token("app", app_token.as_deref(), "xapp-")?; validate_slack_token("bot", bot_token.as_deref(), "xoxb-")?; + } else if params.channel.progress.is_some() + || params.channel.follow_up.is_some() + || params.channel.thread_context.is_some() + { + // Accepting these on an HTTP channel would store an option nothing + // reads, and report it back as though it were in effect. + return Err(anyhow!( + "progress, follow_up, and thread_context apply to Slack channels only" + )); } - let config = ServiceChannelConfig { - kind: Some(params.channel.kind), - enabled: params.channel.enabled, - port, - token, - app_token, - bot_token, - allowed_workspaces: normalize_allowlist(params.channel.allowed_workspaces), - allowed_channels: normalize_allowlist(params.channel.allowed_channels), - // Not editable through this path yet, so carry the operator's choice - // forward. Defaulting here would silently reset a definition every - // time an unrelated field (a rotated token, an allowlist) was saved. - progress: existing + // An omitted option keeps what is stored: a client that does not offer + // these fields must not reset them by saving an unrelated one. + let progress = match params.channel.progress.as_deref() { + Some(value) => SlackProgress::parse(value)?, + None => existing .as_ref() .map(|channel| channel.progress) .unwrap_or_default(), - follow_up: existing + }; + let follow_up = match params.channel.follow_up.as_deref() { + Some(value) => SlackFollowUp::parse(value)?, + None => existing .as_ref() .map(|channel| channel.follow_up) .unwrap_or_default(), - thread_context: existing + }; + let thread_context = match params.channel.thread_context { + Some(value) if value > THREAD_CONTEXT_MAX => { + return Err(anyhow!( + "thread_context must be at most {THREAD_CONTEXT_MAX}; Slack returns no more in one page" + )); + } + Some(value) => value, + None => existing .as_ref() .map(|channel| channel.thread_context) .unwrap_or_else(default_thread_context), }; + let config = ServiceChannelConfig { + kind: Some(params.channel.kind), + enabled: params.channel.enabled, + port, + token, + app_token, + bot_token, + allowed_workspaces: normalize_allowlist(params.channel.allowed_workspaces), + allowed_channels: normalize_allowlist(params.channel.allowed_channels), + progress, + follow_up, + thread_context, + }; service .channels .insert(params.channel.id.clone(), config.clone()); @@ -797,6 +874,9 @@ fn channel_summary( config: &ServiceChannelConfig, attached_to: Option, ) -> construct_protocol::ServiceChannelSummary { + // Behavior options belong to Slack, and a client that cannot see the stored + // value cannot show what an omitted field is preserving. + let slack = channel_kind(&id, config) == "slack"; construct_protocol::ServiceChannelSummary { id: id.clone(), kind: channel_kind(&id, config), @@ -827,6 +907,9 @@ fn channel_summary( allowed_channel_count: config.allowed_channels.len(), allowed_workspaces: config.allowed_workspaces.clone(), allowed_channels: config.allowed_channels.clone(), + progress: slack.then(|| config.progress.as_str().to_string()), + follow_up: slack.then(|| config.follow_up.as_str().to_string()), + thread_context: slack.then_some(config.thread_context), attached_to, publication: None, } @@ -1575,6 +1658,9 @@ mod tests { bot_token: None, allowed_workspaces: Vec::new(), allowed_channels: Vec::new(), + progress: None, + follow_up: None, + thread_context: None, }, rotate_secret: false, }, @@ -1655,6 +1741,9 @@ mod tests { bot_token: None, allowed_workspaces: Vec::new(), allowed_channels: Vec::new(), + progress: None, + follow_up: None, + thread_context: None, }, rotate_secret: false, }, @@ -1673,6 +1762,9 @@ mod tests { bot_token: None, allowed_workspaces: Vec::new(), allowed_channels: Vec::new(), + progress: None, + follow_up: None, + thread_context: None, }, rotate_secret: false, }, @@ -1789,6 +1881,9 @@ mod tests { bot_token: None, allowed_workspaces: Vec::new(), allowed_channels: Vec::new(), + progress: None, + follow_up: None, + thread_context: None, }, rotate_secret: false, }, @@ -1873,6 +1968,9 @@ mod tests { bot_token: None, allowed_workspaces: Vec::new(), allowed_channels: Vec::new(), + progress: None, + follow_up: None, + thread_context: None, }, rotate_secret: false, }, @@ -1892,11 +1990,10 @@ mod tests { } #[test] - fn editing_a_channel_keeps_the_progress_affordance_it_was_given() { - // The channel put path cannot express `progress` yet, so a save that - // touched anything else — rotating a token, editing an allowlist — - // would reset an operator's choice back to the default without ever - // saying so. + fn an_omitted_option_keeps_the_value_the_channel_was_given() { + // Absent means unchanged, never default: a client that does not offer + // these fields must be able to save an allowlist without resetting an + // operator's choice behind their back. let config = tempfile::tempdir().unwrap(); let services = config.path().join("services"); std::fs::create_dir_all(&services).unwrap(); @@ -1922,6 +2019,9 @@ mod tests { bot_token: None, allowed_workspaces: vec!["T9".into()], allowed_channels: Vec::new(), + progress: None, + follow_up: None, + thread_context: None, }, rotate_secret: false, }, @@ -1935,6 +2035,163 @@ mod tests { assert_eq!(stored.allowed_workspaces, vec!["T9".to_string()]); } + /// A Slack channel with every behavior option left at its default. + fn slack_channel_fixture() -> (tempfile::TempDir, PathBuf) { + let config = tempfile::tempdir().unwrap(); + let services = config.path().join("services"); + std::fs::create_dir_all(&services).unwrap(); + std::fs::write( + services.join("chat.toml"), + "harness = \"codex\"\n\ + [channels.bot]\nkind = \"slack\"\n\ + app_token = \"xapp-1\"\nbot_token = \"xoxb-1\"\n", + ) + .unwrap(); + (config, services) + } + + fn slack_option_put( + progress: Option<&str>, + follow_up: Option<&str>, + thread_context: Option, + ) -> construct_protocol::ServiceChannelPutParams { + construct_protocol::ServiceChannelPutParams { + service_name: "chat".into(), + channel: construct_protocol::ServiceChannelPut { + id: "bot".into(), + kind: "slack".into(), + enabled: true, + port: None, + app_token: None, + bot_token: None, + allowed_workspaces: Vec::new(), + allowed_channels: Vec::new(), + progress: progress.map(ToString::to_string), + follow_up: follow_up.map(ToString::to_string), + thread_context, + }, + rotate_secret: false, + } + } + + #[test] + fn a_channel_put_sets_the_slack_options_and_reports_them_back() { + let (_config, services) = slack_channel_fixture(); + + let result = put_channel( + &services, + slack_option_put(Some("both"), Some("channel"), Some(12)), + ) + .unwrap(); + + // Reported back, so a client can show what it is preserving. + assert_eq!(result.channel.progress.as_deref(), Some("both")); + assert_eq!(result.channel.follow_up.as_deref(), Some("channel")); + assert_eq!(result.channel.thread_context, Some(12)); + + let stored = &load_definitions(&services).unwrap()["chat"].channels["bot"]; + assert_eq!(stored.progress, SlackProgress::Both); + assert_eq!(stored.follow_up, SlackFollowUp::Channel); + assert_eq!(stored.thread_context, 12); + } + + #[test] + fn an_unknown_option_value_is_refused_rather_than_defaulted() { + let (_config, services) = slack_channel_fixture(); + + for params in [ + slack_option_put(Some("loud"), None, None), + slack_option_put(None, Some("everywhere"), None), + ] { + let error = put_channel(&services, params).unwrap_err().to_string(); + assert!(error.contains("expected one of"), "unexpected: {error}"); + } + // A refused edit leaves the stored definition untouched. + let stored = &load_definitions(&services).unwrap()["chat"].channels["bot"]; + assert_eq!(stored.progress, SlackProgress::default()); + assert_eq!(stored.follow_up, SlackFollowUp::default()); + } + + #[test] + fn a_thread_context_past_slacks_own_page_limit_is_refused() { + let (_config, services) = slack_channel_fixture(); + + let error = put_channel( + &services, + slack_option_put(None, None, Some(THREAD_CONTEXT_MAX + 1)), + ) + .unwrap_err() + .to_string(); + + assert!(error.contains("at most"), "unexpected: {error}"); + assert_eq!( + put_channel(&services, slack_option_put(None, None, Some(0))) + .unwrap() + .channel + .thread_context, + Some(0), + "0 is a real setting, not an absent one" + ); + } + + #[test] + fn slack_options_are_refused_on_a_channel_that_cannot_read_them() { + let config = tempfile::tempdir().unwrap(); + let services = config.path().join("services"); + std::fs::create_dir_all(&services).unwrap(); + std::fs::write(services.join("alerts.toml"), "harness = \"codex\"\n").unwrap(); + + let error = put_channel( + &services, + construct_protocol::ServiceChannelPutParams { + service_name: "alerts".into(), + channel: construct_protocol::ServiceChannelPut { + id: "http".into(), + kind: "http".into(), + enabled: true, + port: Some(8787), + app_token: None, + bot_token: None, + allowed_workspaces: Vec::new(), + allowed_channels: Vec::new(), + follow_up: Some("channel".into()), + progress: None, + thread_context: None, + }, + rotate_secret: false, + }, + ) + .unwrap_err() + .to_string(); + + assert!(error.contains("Slack channels only"), "unexpected: {error}"); + // An HTTP channel reports no options rather than defaults it ignores. + let summary = &list_channel_catalog(&services).unwrap(); + assert!(summary.is_empty(), "the refused put must not have stored"); + } + + #[test] + fn the_published_option_defaults_match_the_ones_a_definition_gets() { + // Clients seed a new channel from the published defaults, so a drift + // here would show the operator a value the daemon would not store. + assert_eq!( + SlackProgress::default().as_str(), + construct_protocol::SLACK_PROGRESS_DEFAULT + ); + assert_eq!( + SlackFollowUp::default().as_str(), + construct_protocol::SLACK_FOLLOW_UP_DEFAULT + ); + assert_eq!(default_thread_context(), SLACK_THREAD_CONTEXT_DEFAULT); + // Every value a client can offer is one the daemon accepts. + for value in PROGRESS_VALUES { + assert!(SlackProgress::parse(value).is_ok(), "{value}"); + } + for value in FOLLOW_UP_VALUES { + assert!(SlackFollowUp::parse(value).is_ok(), "{value}"); + } + } + #[test] fn slack_credentials_are_persisted_but_never_returned_in_summaries() { let config = tempfile::tempdir().unwrap(); @@ -1970,6 +2227,9 @@ mod tests { bot_token: Some("xoxb-secret".into()), allowed_workspaces: vec![" T2 ".into(), "T1".into(), "T1".into()], allowed_channels: vec!["C1".into()], + progress: None, + follow_up: None, + thread_context: None, }, rotate_secret: false, }, diff --git a/crates/protocol/src/lib.rs b/crates/protocol/src/lib.rs index c6abe443..e713b2e8 100644 --- a/crates/protocol/src/lib.rs +++ b/crates/protocol/src/lib.rs @@ -3288,6 +3288,14 @@ pub struct ServiceChannelSummary { pub allowed_workspaces: Vec, #[serde(default, skip_serializing_if = "Vec::is_empty")] pub allowed_channels: Vec, + /// Slack behavior options, reported so a client can show what it is about + /// to preserve. `None` on a channel whose kind has no such option. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub progress: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub follow_up: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub thread_context: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub attached_to: Option, /// Live public exposure, when this channel has been explicitly published. @@ -3443,6 +3451,9 @@ pub enum ServiceField { ChannelPort, ChannelState, ChannelCredential, + ChannelProgress, + ChannelFollowUp, + ChannelThreadContext, } impl ServiceField { @@ -3460,15 +3471,24 @@ impl ServiceField { ServiceField::ChannelPort, ServiceField::ChannelState, ServiceField::ChannelCredential, + ServiceField::ChannelProgress, + ServiceField::ChannelFollowUp, + ServiceField::ChannelThreadContext, ]; pub fn propagation(self) -> PropagationClass { match self { - // Channel shape is listener state: saving it binds or unbinds. + // Channel shape is listener state: saving it binds or unbinds. A + // Slack channel has no port, but its behavior options are held by + // the running connection, so saving one replaces that connection + // exactly as a changed allowlist does. ServiceField::ChannelAttachment | ServiceField::ChannelPort | ServiceField::ChannelState - | ServiceField::ChannelCredential => PropagationClass::Immediate, + | ServiceField::ChannelCredential + | ServiceField::ChannelProgress + | ServiceField::ChannelFollowUp + | ServiceField::ChannelThreadContext => PropagationClass::Immediate, // Consulted while handling a request, before any session is touched. ServiceField::Routing | ServiceField::Paused | ServiceField::ApprovalTimeout => { PropagationClass::NextRequest @@ -3562,8 +3582,32 @@ pub struct ServiceChannelPut { pub allowed_workspaces: Vec, #[serde(default, skip_serializing_if = "Vec::is_empty")] pub allowed_channels: Vec, + /// Slack behavior options. An omitted option keeps the stored value, so a + /// client that does not offer these fields never resets them by saving an + /// unrelated one. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub progress: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub follow_up: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub thread_context: Option, } +/// Accepted values for a Slack channel's behavior options, in the order a +/// client cycles them. Clients and the daemon's validator read the same lists, +/// so an option added here is offered and accepted by the same change. +pub const SLACK_PROGRESS_VALUES: &[&str] = &["off", "placeholder", "reaction", "both"]; +pub const SLACK_FOLLOW_UP_VALUES: &[&str] = &["off", "thread", "channel"]; +/// Slack's own ceiling on one `conversations.replies` page. +pub const SLACK_THREAD_CONTEXT_MAX: usize = 1000; + +/// What a channel created without these options gets. Published so a client +/// can show a new channel the values it will be saved with, rather than three +/// blanks that mean "whatever the daemon decides". +pub const SLACK_PROGRESS_DEFAULT: &str = "placeholder"; +pub const SLACK_FOLLOW_UP_DEFAULT: &str = "thread"; +pub const SLACK_THREAD_CONTEXT_DEFAULT: usize = 50; + #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ServiceChannelPutResult { pub channel: ServiceChannelSummary, diff --git a/docs/services.md b/docs/services.md index 39268c84..ce6740ae 100644 --- a/docs/services.md +++ b/docs/services.md @@ -43,6 +43,14 @@ follow_up = "thread" # off | thread | channel thread_context = 50 # earlier thread messages to read on joining; 0 = none ``` +The last three are also editable where the channel is. Select a Slack channel +in the service view and press `e`: the editor lists **Progress**, **Follow-up**, +and **Thread context** below the allowlists, with what each one needs from your +Slack app in the help column. Space or `→` steps an option forward and `←` back; +thread context is typed. The web client offers the same fields. Saving any of +them reconnects that channel's Socket Mode connection, the same as changing an +allowlist does. + ### Answering without being mentioned A bot you must `@`-mention for every message cannot hold a conversation. DMs diff --git a/specs/0173-service-definitions-apply-without-restart.md b/specs/0173-service-definitions-apply-without-restart.md index 0cee9d82..7e094064 100644 --- a/specs/0173-service-definitions-apply-without-restart.md +++ b/specs/0173-service-definitions-apply-without-restart.md @@ -16,7 +16,7 @@ property of where the daemon reads that field: | Change | Applies | |---|---| -| Channel attachment, port, enabled state, credential | immediately | +| Channel attachment, port, enabled state, credential, behavior options | immediately | | Routing rule, paused, approval timeout | on the next request | | Instruction, harness, model, working directory, sandbox | on new sessions | @@ -43,6 +43,12 @@ used, so a rotated credential, a changed routing rule, or a paused service take effect without disturbing the listener. When sockets must move, every stop completes before any start, so two channels can exchange ports. +An outbound channel is the mirror image: it holds its configuration in the +connection rather than reading it per request, so any change to that +configuration replaces the connection. That is why a channel's behavior options +sit in the immediate class — not because they touch a port, but because saving +one is what makes the running connection adopt it. + **Requests in flight are never interrupted.** Stopping a listener stops it accepting; a request already being served runs to completion, because a service request has an agent turn behind it. Connections drain, sockets rebind. diff --git a/specs/0180-channel-options-are-editable-where-channels-are.md b/specs/0180-channel-options-are-editable-where-channels-are.md new file mode 100644 index 00000000..b817784e --- /dev/null +++ b/specs/0180-channel-options-are-editable-where-channels-are.md @@ -0,0 +1,75 @@ +# 0180-channel-options-are-editable-where-channels-are + +Status: accepted +Date: 2026-08-02 +Area: ux +Scope: An option that exists in a channel definition is offered by the clients that edit channels, not only by the configuration file. + +## Decision + +Adding an option to a channel definition is not finished until the clients that +edit channels can set it. A definition field reachable only by hand-editing +configuration is an incomplete feature, not a smaller one. + +Three rules keep that honest as options accumulate: + +- **The accepted values are published once.** The set a client offers and the + set the daemon accepts come from the same declaration. A client cannot offer + a value that would be rejected, and an option gains its editor and its + validator in the same change. +- **An omitted option preserves the stored value.** A client that does not know + about an option must be able to save an unrelated field without resetting it. + Absent means "unchanged", never "default". +- **An option is reported back.** A client that is about to preserve a value + has to be able to show it. Write-only credentials are the sole exception, and + they are reported as present rather than returned. + +An option that applies to one channel kind is offered only for that kind, and +submitting it for another kind is refused rather than stored unread. + +## Reason + +Two consecutive changes each added a Slack option to the definition and to the +documentation, and neither added it to a client. The result was an operator who +could see in the docs that a bot can answer untagged messages, find the channel +editor in front of them, and still have to go find a TOML file — while the +editor sat one field away, silently preserving a value it would not show. + +The three rules address how that happens rather than the two instances of it. +Duplicating the accepted values in each client is what lets an editor drift out +of step with the validator. Defaulting on absence is what makes adding an +option to one client silently reset it from another. Not reporting the value is +what leaves a client unable to render a field even once someone wants to. + +## Consequences + +- Adding a channel option means touching the wire type, the daemon's + validation, and every client that edits channels — in one change. That is + more work per option than a configuration-only field, and it is the point. +- The option's propagation class must be declared, per + [0173](0173-service-definitions-apply-without-restart.md). For an outbound + channel that holds its configuration in a live connection, saving an option + replaces that connection. +- A client written against an older protocol keeps working: it sends no option, + and preserves every value it cannot show. +- Options whose safe use depends on context — anything that widens who can put + text in front of a session — carry that caveat in the client, not only in the + documentation. The operator making the decision is the one looking at the + field. + +## Non-Goals + +- This does not say every service definition field must be editable from every + client. It is about channel options specifically, and about the client that + already edits that channel. +- It does not require a CLI subcommand for each option. "The clients that edit + channels" means the surfaces that already present a channel editor. + +## Examples + +- A Slack channel's editor lists its behavior options beside its allowlists, + with what each needs from the Slack app stated in the same view. +- Saving that editor from a client that offers only the allowlists leaves the + behavior options exactly as they were. +- Submitting a Slack-only option for an HTTP channel is refused, rather than + stored and reported back as though it were in effect.