From eeeef7c2766069cebdc3addac1f1b2456758a551 Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Sun, 26 Jul 2026 14:43:16 -0400 Subject: [PATCH 1/2] feat(acp): title ACP sessions via session/new _meta MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex sessions all showed up unnamed in the thread list, and one agent active in several channels produced several indistinguishable rows because sessions are keyed per channel. The title travels out of band in `session/new` `_meta.sessionTitle`, so it costs no tokens and cannot perturb the prompt contract. `_meta` is omitted entirely when there is no title, since an adapter may distinguish an absent member from a null one. Sanitization and `Agent · #channel` composition live in buzz-acp: the desktop spawn path only resolves `display_name`-or-`name` and exports `BUZZ_ACP_SESSION_TITLE`. Only the channel part is truncated to the 80-char cap, so the agent name always survives. DMs, unresolved channels, and heartbeat sessions keep the bare agent name. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-acp/src/acp.rs | 69 ++++++++- crates/buzz-acp/src/config.rs | 135 ++++++++++++++++++ crates/buzz-acp/src/lib.rs | 5 +- crates/buzz-acp/src/pool.rs | 39 ++++- .../src-tauri/src/managed_agents/runtime.rs | 12 +- .../src/managed_agents/runtime/metadata.rs | 47 ++++++ 6 files changed, 300 insertions(+), 7 deletions(-) diff --git a/crates/buzz-acp/src/acp.rs b/crates/buzz-acp/src/acp.rs index 78db7ff718..49e4d6cce0 100644 --- a/crates/buzz-acp/src/acp.rs +++ b/crates/buzz-acp/src/acp.rs @@ -558,6 +558,9 @@ impl AcpClient { /// `cwd` must be an absolute path. `mcp_servers` may be empty. /// `system_prompt` is included in the request when `Some` — agents that /// support the field will use it; others ignore unknown fields per JSON-RPC. + /// `session_title` rides in `_meta.sessionTitle` when `Some`; `_meta` is + /// omitted entirely otherwise, since adapters may distinguish an absent + /// member from a null one. /// Callers use [`extract_model_config_options`] and [`extract_model_state`] /// to pull model info from the raw result. pub async fn session_new_full( @@ -565,6 +568,7 @@ impl AcpClient { cwd: &str, mcp_servers: Vec, system_prompt: Option<&str>, + session_title: Option<&str>, ) -> Result { let mut params = serde_json::json!({ "cwd": cwd, @@ -573,6 +577,9 @@ impl AcpClient { if let Some(sp) = system_prompt { params["systemPrompt"] = serde_json::Value::String(sp.to_owned()); } + if let Some(title) = session_title { + params["_meta"] = serde_json::json!({ "sessionTitle": title }); + } let result = self.send_request("session/new", params).await?; let session_id = result["sessionId"] .as_str() @@ -594,9 +601,10 @@ impl AcpClient { cwd: &str, mcp_servers: Vec, system_prompt: Option<&str>, + session_title: Option<&str>, ) -> Result { Ok(self - .session_new_full(cwd, mcp_servers, system_prompt) + .session_new_full(cwd, mcp_servers, system_prompt, session_title) .await? .session_id) } @@ -2954,7 +2962,7 @@ mod tests { .expect("initialize should succeed"); let resp = client - .session_new_full("/tmp", vec![], Some("Custom system prompt")) + .session_new_full("/tmp", vec![], Some("Custom system prompt"), None) .await .expect("session_new_full should succeed"); @@ -3039,7 +3047,7 @@ mod tests { .expect("initialize should succeed"); let resp = client - .session_new_full("/tmp", vec![], None) + .session_new_full("/tmp", vec![], None, None) .await .expect("session_new_full should succeed"); @@ -3051,6 +3059,61 @@ mod tests { ); } + #[tokio::test] + async fn session_new_full_sends_session_title_in_meta_when_some() { + let script = r#" + read -t 2 _init + echo '{"jsonrpc":"2.0","id":0,"result":{"protocolVersion":1,"agentCapabilities":{}}}' + read -t 2 REQ + echo '{"jsonrpc":"2.0","id":1,"result":{"sessionId":"ses_test","_receivedRequest":'"$REQ"'}}' + sleep 1 + "#; + let mut client = spawn_script(script).await; + client + .initialize() + .await + .expect("initialize should succeed"); + + let resp = client + .session_new_full("/tmp", vec![], None, Some("Fizz · #buzz-dev")) + .await + .expect("session_new_full should succeed"); + + let received = &resp.raw["_receivedRequest"]; + assert_eq!( + received["params"]["_meta"]["sessionTitle"].as_str(), + Some("Fizz · #buzz-dev"), + "title should ride in _meta.sessionTitle, out of band from the prompt" + ); + } + + #[tokio::test] + async fn session_new_full_omits_meta_when_session_title_none() { + let script = r#" + read -t 2 _init + echo '{"jsonrpc":"2.0","id":0,"result":{"protocolVersion":1,"agentCapabilities":{}}}' + read -t 2 REQ + echo '{"jsonrpc":"2.0","id":1,"result":{"sessionId":"ses_test","_receivedRequest":'"$REQ"'}}' + sleep 1 + "#; + let mut client = spawn_script(script).await; + client + .initialize() + .await + .expect("initialize should succeed"); + + let resp = client + .session_new_full("/tmp", vec![], None, None) + .await + .expect("session_new_full should succeed"); + + let received = &resp.raw["_receivedRequest"]; + assert!( + received["params"].get("_meta").is_none(), + "_meta should be absent entirely, not an empty object or null" + ); + } + // ── Goose-native steer scaffold (PR follow-up to #1160) ────────────── /// Helper: spawn an inert `cat` subprocess so we have a real AcpClient diff --git a/crates/buzz-acp/src/config.rs b/crates/buzz-acp/src/config.rs index a38d6faa14..9a1b74c276 100644 --- a/crates/buzz-acp/src/config.rs +++ b/crates/buzz-acp/src/config.rs @@ -423,6 +423,12 @@ pub struct CliArgs { #[arg(long, env = "BUZZ_ACP_MODEL")] pub model: Option, + /// Title for the agent's ACP sessions, passed out-of-band in `session/new` + /// `_meta`. Adapters that recognize it name the session after this value; + /// others ignore it. Never enters the prompt. + #[arg(long, env = "BUZZ_ACP_SESSION_TITLE")] + pub session_title: Option, + /// Permission mode for agents that support `session/set_config_option` /// with `configId: "mode"` (e.g. `claude-agent-acp`). /// @@ -522,6 +528,9 @@ pub struct Config { pub memory_enabled: bool, /// Desired LLM model ID. Applied after every `session_new_full()`. pub model: Option, + /// Sanitized session title, sent as `_meta.sessionTitle` on `session/new`. + /// `None` when unset or when the configured value sanitized to empty. + pub session_title: Option, /// Permission mode to apply after session creation. `Default` = skip. pub permission_mode: PermissionMode, /// Inbound author gate mode. @@ -554,6 +563,68 @@ pub struct Config { pub base_prompt_content: Option, } +/// Maximum length, in characters, of a session title sent to the adapter. +const SESSION_TITLE_MAX_CHARS: usize = 80; + +/// Normalize a configured session title into something safe to hand an adapter. +/// +/// Control characters are dropped, runs of whitespace collapse to a single +/// space, and the result is trimmed and capped at +/// [`SESSION_TITLE_MAX_CHARS`]. Returns `None` when nothing printable is left. +/// +/// Buzz is the only guard here: Codex's own `normalize_thread_name` merely +/// trims, so an unbounded display name would be persisted verbatim into its +/// thread store. +fn sanitize_session_title(raw: &str) -> Option { + let collapsed = raw + .split_whitespace() + .map(|word| word.chars().filter(|c| !c.is_control()).collect::()) + .filter(|word| !word.is_empty()) + .collect::>() + .join(" "); + // Truncate by chars, not bytes, so a multi-byte name can't be cut mid-UTF-8. + let title: String = collapsed + .chars() + .take(SESSION_TITLE_MAX_CHARS) + .collect::() + .trim_end() + .to_string(); + if title.is_empty() { + None + } else { + Some(title) + } +} + +/// Separator between the agent name and the channel in a composed title. +/// U+00B7 MIDDLE DOT, spaces on both sides. +const SESSION_TITLE_SEPARATOR: &str = " · "; + +/// Compose a per-session title as `Agent · #channel`. +/// +/// One agent in five channels gets five sessions; a bare agent name would show +/// five identical rows in the adapter's thread list. Only the channel part is +/// truncated to fit [`SESSION_TITLE_MAX_CHARS`], so the agent name always +/// survives. Returns the bare agent name when there is no channel, the channel +/// name is blank, or no room is left for it. +pub(crate) fn compose_session_title(agent: &str, channel_name: Option<&str>) -> String { + let Some(channel) = channel_name.and_then(sanitize_session_title) else { + return agent.to_string(); + }; + // Reserve the separator and the `#` sigil alongside the agent name. + let reserved = agent.chars().count() + SESSION_TITLE_SEPARATOR.chars().count() + 1; + let channel: String = channel + .chars() + .take(SESSION_TITLE_MAX_CHARS.saturating_sub(reserved)) + .collect::() + .trim_end() + .to_string(); + if channel.is_empty() { + return agent.to_string(); + } + format!("{agent}{SESSION_TITLE_SEPARATOR}#{channel}") +} + /// Validate and deduplicate allowlist entries: each must be exactly 64 hex chars. fn validate_allowlist(entries: &[String]) -> Result, ConfigError> { let mut validated = HashSet::new(); @@ -992,6 +1063,10 @@ impl Config { typing_enabled: !args.no_typing, memory_enabled: args.memory && !args.no_memory, model, + session_title: args + .session_title + .as_deref() + .and_then(sanitize_session_title), permission_mode: args.permission_mode, respond_to: args.respond_to, respond_to_allowlist, @@ -1361,6 +1436,7 @@ mod tests { typing_enabled: true, memory_enabled: true, model: None, + session_title: None, permission_mode: PermissionMode::BypassPermissions, respond_to: RespondTo::Anyone, respond_to_allowlist: HashSet::new(), @@ -2706,4 +2782,63 @@ channels = "ALL" assert!(MAX_TURN_DURATION_CEILING_SECS < u64::MAX - 100); } } + + #[test] + fn sanitize_session_title_collapses_whitespace_and_strips_control_chars() { + assert_eq!( + sanitize_session_title(" Fizz\t\tthe\n Bot\u{7} "), + Some("Fizz the Bot".to_string()) + ); + } + + #[test] + fn sanitize_session_title_returns_none_when_nothing_printable_remains() { + assert_eq!(sanitize_session_title(" \n\t "), None); + assert_eq!(sanitize_session_title(""), None); + assert_eq!(sanitize_session_title("\u{1}\u{2}"), None); + } + + #[test] + fn sanitize_session_title_caps_length_without_splitting_multibyte_chars() { + let raw = "\u{1f41d}".repeat(SESSION_TITLE_MAX_CHARS + 10); + let title = sanitize_session_title(&raw).expect("emoji title survives sanitizing"); + assert_eq!(title.chars().count(), SESSION_TITLE_MAX_CHARS); + assert!(title.chars().all(|c| c == '\u{1f41d}')); + } + + #[test] + fn sanitize_session_title_does_not_leave_a_trailing_space_after_the_cap() { + // The cap lands mid-word, so trimming must not leave a dangling space. + let raw = format!("{} tail", "a".repeat(SESSION_TITLE_MAX_CHARS - 1)); + let title = sanitize_session_title(&raw).expect("title survives sanitizing"); + assert_eq!(title, "a".repeat(SESSION_TITLE_MAX_CHARS - 1)); + } + + #[test] + fn compose_session_title_qualifies_the_agent_name_with_the_channel() { + assert_eq!( + compose_session_title("Fizz", Some("buzz-dev")), + "Fizz · #buzz-dev" + ); + } + + #[test] + fn compose_session_title_falls_back_to_bare_agent_name_without_a_channel() { + assert_eq!(compose_session_title("Fizz", None), "Fizz"); + assert_eq!(compose_session_title("Fizz", Some(" ")), "Fizz"); + } + + #[test] + fn compose_session_title_truncates_the_channel_and_keeps_the_agent_name() { + let channel = "c".repeat(200); + let title = compose_session_title("Fizz", Some(&channel)); + assert_eq!(title.chars().count(), SESSION_TITLE_MAX_CHARS); + assert!(title.starts_with("Fizz · #c")); + } + + #[test] + fn compose_session_title_drops_the_channel_when_the_agent_name_fills_the_cap() { + let agent = "a".repeat(SESSION_TITLE_MAX_CHARS); + assert_eq!(compose_session_title(&agent, Some("buzz-dev")), agent); + } } diff --git a/crates/buzz-acp/src/lib.rs b/crates/buzz-acp/src/lib.rs index 0230ea0875..4b6db6a5b0 100644 --- a/crates/buzz-acp/src/lib.rs +++ b/crates/buzz-acp/src/lib.rs @@ -1535,6 +1535,7 @@ async fn tokio_main() -> Result<()> { turn_liveness_interval: Duration::from_secs(config.turn_liveness_secs), dedup_mode: config.dedup_mode, system_prompt: config.system_prompt.clone(), + session_title: config.session_title.clone(), team_instructions: config.team_instructions.clone(), base_prompt: if config.no_base_prompt { None @@ -4026,7 +4027,7 @@ async fn run_models(args: ModelsArgs) -> Result<()> { // so shutdown() runs on all paths (success, error, timeout). let protocol_result = tokio::time::timeout(MODELS_TIMEOUT, async { let init = client.initialize().await?; - let session = client.session_new_full(&cwd, vec![], None).await?; + let session = client.session_new_full(&cwd, vec![], None, None).await?; Ok::<_, acp::AcpError>((init, session)) }) .await; @@ -4973,6 +4974,7 @@ mod build_mcp_servers_tests { typing_enabled: true, memory_enabled: false, model: None, + session_title: None, permission_mode: config::PermissionMode::BypassPermissions, respond_to: config::RespondTo::Anyone, respond_to_allowlist: std::collections::HashSet::new(), @@ -5139,6 +5141,7 @@ mod error_outcome_emission_tests { typing_enabled: true, memory_enabled: false, model: None, + session_title: None, permission_mode: config::PermissionMode::BypassPermissions, respond_to: config::RespondTo::Anyone, respond_to_allowlist: HashSet::new(), diff --git a/crates/buzz-acp/src/pool.rs b/crates/buzz-acp/src/pool.rs index cc537f8683..15486f8656 100644 --- a/crates/buzz-acp/src/pool.rs +++ b/crates/buzz-acp/src/pool.rs @@ -32,7 +32,7 @@ use crate::acp::{ extract_model_config_options, extract_model_state, model_in_catalog, resolve_model_switch_method, AcpClient, AcpError, McpServer, ModelSwitchMethod, StopReason, }; -use crate::config::{DedupMode, PermissionMode}; +use crate::config::{compose_session_title, DedupMode, PermissionMode}; use crate::observer; use crate::queue::{ CancelReason, ContextMessage, ConversationContext, FlushBatch, PromptChannelInfo, @@ -490,6 +490,9 @@ pub struct PromptContext { pub turn_liveness_interval: Duration, pub dedup_mode: DedupMode, pub system_prompt: Option, + /// Sanitized title for each new ACP session, sent as `_meta.sessionTitle` + /// on `session/new`. Never part of the prompt. + pub session_title: Option, pub team_instructions: Option, pub heartbeat_prompt: Option, /// Base prompt content, or `None` if `--no-base-prompt` was passed. @@ -806,6 +809,7 @@ async fn create_session_and_apply_model( ctx: &PromptContext, agent_core: Option<&str>, agent_canvas: Option<&str>, + channel_name: Option<&str>, ) -> Result { // Build base_prompt + system_prompt + agent core + canvas metadata into a // single prompt. Standard protocol-v2 agents receive it in `session/new`; @@ -825,6 +829,11 @@ async fn create_session_and_apply_model( agent_canvas, ); + let session_title = ctx + .session_title + .as_deref() + .map(|agent_name| compose_session_title(agent_name, channel_name)); + let resp = agent .acp .session_new_full( @@ -835,6 +844,7 @@ async fn create_session_and_apply_model( agent.protocol_version, combined_system_prompt.as_deref(), ), + session_title.as_deref(), ) .await?; @@ -1470,12 +1480,25 @@ pub async fn run_prompt_task( if let Some(sid) = agent.state.sessions.get(cid) { (sid.clone(), false) } else { + // Channel-qualify the title (`Agent · #channel`) so one agent in + // several channels doesn't produce identical session rows. + // Cache-first; DM or unresolved channels keep the bare name. + let channel_name = if ctx.session_title.is_some() { + ctx.channel_info + .resolve(*cid) + .await + .filter(|ci| ci.channel_type != "dm") + .map(|ci| ci.name) + } else { + None + }; // Create new session with model application. match create_session_and_apply_model( &mut agent, &ctx, agent_core.as_deref(), agent_canvas.as_deref(), + channel_name.as_deref(), ) .await { @@ -1523,7 +1546,7 @@ pub async fn run_prompt_task( if let Some(sid) = &agent.state.heartbeat_session { (sid.clone(), false) } else { - match create_session_and_apply_model(&mut agent, &ctx, None, None).await { + match create_session_and_apply_model(&mut agent, &ctx, None, None, None).await { Ok(sid) => { tracing::info!( target: "pool::session", @@ -3801,6 +3824,17 @@ mod tests { assert_eq!(framed, "[System]\npersona text"); } + #[test] + fn test_framed_system_prompt_never_carries_a_session_title_section() { + // Regression against #2372: the title travels out of band in + // `_meta.sessionTitle`, so no `[Session]` section may appear in the + // prompt and the base framing must stay byte-identical. + let framed = framed_system_prompt("/", Some("base text"), Some("persona text")) + .expect("both present yields Some"); + assert_eq!(framed, "[Base]\nbase text\n\n[System]\npersona text"); + assert!(!framed.contains("[Session]")); + } + #[test] fn test_framed_system_prompt_neither_is_none() { assert!(framed_system_prompt("/", None, None).is_none()); @@ -5280,6 +5314,7 @@ mod tests { turn_liveness_interval: Duration::ZERO, dedup_mode: DedupMode::Drop, system_prompt: None, + session_title: None, team_instructions: None, heartbeat_prompt: None, base_prompt: None, diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index 32b3b328e9..5329aff185 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -21,7 +21,9 @@ pub(crate) use path::should_skip_claude_executable; pub(crate) use path::should_use_inherited; mod metadata; -pub(crate) use metadata::{resolve_effective_prompt_model_provider, runtime_metadata_env_vars}; +pub(crate) use metadata::{ + resolve_effective_prompt_model_provider, resolve_session_title, runtime_metadata_env_vars, +}; mod stop; pub(crate) use stop::managed_agent_runtime_keys; @@ -766,6 +768,14 @@ pub fn spawn_agent_child( } else { command.env_remove("BUZZ_ACP_MODEL"); } + // Session title for the harness to pass out-of-band on `session/new`. The + // adapter names the session after it; it never reaches the prompt, so this + // is display metadata only. + if let Some(title) = resolve_session_title(record.display_name.as_deref(), &record.name) { + command.env("BUZZ_ACP_SESSION_TITLE", title); + } else { + command.env_remove("BUZZ_ACP_SESSION_TITLE"); + } build_buzz_agent_provider_defaults(&mut command); if let Some(meta) = runtime_meta { for (key, value) in runtime_metadata_env_vars( diff --git a/desktop/src-tauri/src/managed_agents/runtime/metadata.rs b/desktop/src-tauri/src/managed_agents/runtime/metadata.rs index ac9e3a0a3f..f0c5e59ae4 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/metadata.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/metadata.rs @@ -24,6 +24,24 @@ pub(crate) fn runtime_metadata_env_vars<'a>( vars } +/// Resolve the session title for an agent: its `display_name` when it has one, +/// otherwise its unique `name` handle. `None` when both are blank, so the +/// caller clears the env var rather than exporting an empty title. +/// +/// Sanitization (control chars, whitespace, length cap) and channel +/// qualification are the harness's job — see `sanitize_session_title` and +/// `compose_session_title` in `buzz-acp`. +pub(crate) fn resolve_session_title<'a>( + display_name: Option<&'a str>, + name: &'a str, +) -> Option<&'a str> { + [display_name, Some(name)] + .into_iter() + .flatten() + .map(str::trim) + .find(|value| !value.is_empty()) +} + /// Resolve effective prompt/model/provider using definition-authoritative /// semantics for linked instances. /// @@ -49,3 +67,32 @@ pub(crate) fn resolve_effective_prompt_model_provider( None => (record_prompt, record_model, record_provider), } } + +#[cfg(test)] +mod tests { + use super::resolve_session_title; + + #[test] + fn resolve_session_title_prefers_display_name() { + assert_eq!(resolve_session_title(Some("Fizz"), "fizz-1"), Some("Fizz")); + } + + #[test] + fn resolve_session_title_falls_back_to_name_when_display_name_blank() { + assert_eq!(resolve_session_title(None, "fizz-1"), Some("fizz-1")); + assert_eq!(resolve_session_title(Some(" "), "fizz-1"), Some("fizz-1")); + } + + #[test] + fn resolve_session_title_returns_none_when_both_are_blank() { + assert_eq!(resolve_session_title(Some(""), " "), None); + } + + #[test] + fn resolve_session_title_trims_surrounding_whitespace() { + assert_eq!( + resolve_session_title(Some(" Fizz "), "fizz-1"), + Some("Fizz") + ); + } +} From d793039768a77614113ccfbdf8058ff0231ae14c Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Sun, 26 Jul 2026 15:36:40 -0400 Subject: [PATCH 2/2] fix(acp): harden session titling against spawn, drift, and I/O defects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-1 review found four correctness defects in the session-title path. An interior NUL in a display name cannot cross the environment boundary at all, so `Command::env` failed the whole spawn — the child-side sanitizer ran far too late. Control characters are now stripped at the desktop boundary, with a fallback to the unique handle when a display name is nothing else. `spawn_config_hash` did not cover the title, so an agent rename left the running process silently stale with no restart badge. It now hashes the same resolve the spawn writes, and skips it when an explicit `BUZZ_ACP_SESSION_TITLE` override shadows the record (the override already reaches the hash through the effective env). The channel lookup ran twice on a new session. `ChannelInfoResolver` caches only `Some`, so an unresolvable channel paid the full `fetch_channel_info` retry sequence twice — ~6.5s each — in front of `session/new`, exactly when the relay is degraded. One resolve now serves both the canvas DM check and the title. `fetch_channel_info` substitutes a literal `"unknown"` name for a metadata event with no `name` tag; that sailed past the DM filter and titled every unnamed channel `Agent · #unknown` — the same collision the suffix exists to remove. It is now treated as absent. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-acp/src/pool.rs | 238 +++++++++++++++--- desktop/scripts/check-file-sizes.mjs | 9 +- .../src-tauri/src/managed_agents/runtime.rs | 8 +- .../src/managed_agents/runtime/metadata.rs | 76 +++++- .../src/managed_agents/spawn_hash.rs | 11 + .../src/managed_agents/spawn_hash/tests.rs | 66 +++++ 6 files changed, 353 insertions(+), 55 deletions(-) diff --git a/crates/buzz-acp/src/pool.rs b/crates/buzz-acp/src/pool.rs index 15486f8656..0c51fe954f 100644 --- a/crates/buzz-acp/src/pool.rs +++ b/crates/buzz-acp/src/pool.rs @@ -798,6 +798,49 @@ const CONTROL_CANCEL_GRACE: Duration = Duration::from_secs(5); /// Timeout for permission-mode requests (`session/set_config_option` with `configId: "mode"`). const PERMISSION_MODE_TIMEOUT: Duration = Duration::from_secs(5); +/// Placeholder [`fetch_channel_info`] substitutes when a channel's metadata +/// event carries no `name` tag. Not a real channel name — consumers that need +/// an identifying name must treat it as absent. +const UNKNOWN_CHANNEL_NAME: &str = "unknown"; + +/// Channel-derived inputs for a new session — `(is_dm, title_channel)` — from +/// **one** metadata resolve. +/// +/// Both new-session consumers need the same lookup: the canvas block skips DMs +/// (and fails closed when the channel type can't be determined), and the +/// session title is qualified with the channel name. Resolving once is +/// load-bearing rather than tidy: [`ChannelInfoResolver`] caches only `Some`, +/// so two calls against an unresolvable channel pay the whole +/// [`fetch_channel_info`] retry sequence twice — two `CONTEXT_FETCH_TIMEOUT` +/// attempts plus `CONTEXT_FETCH_RETRY_DELAY` each, in front of `session/new`, +/// precisely when the relay is already degraded. +/// +/// `title_channel` is `None` whenever the channel can't usefully identify the +/// session: an unresolved channel, a DM (no meaningful name), or the literal +/// `"unknown"` that [`fetch_channel_info`] substitutes for a metadata event +/// with no `name` tag. Composing that sentinel would title every unnamed +/// channel identically (`Agent · #unknown`) — reintroducing the collision the +/// suffix exists to remove, while naming a channel something it isn't. The +/// startup cache already refuses `channel_type == "unknown"` for the same +/// reason. +/// +/// Renames do not retitle live sessions, and a **channel** rename is stickier +/// than an agent rename: `invalidate_channel` drops the session but not the +/// resolver's cached entry, so a renamed channel keeps its old suffix until the +/// process restarts. An agent rename lands on the next spawn (the desktop +/// restart badge covers it — see `spawn_config_hash`). +async fn resolve_new_session_channel_context( + channel_info: &ChannelInfoResolver, + channel_id: Uuid, +) -> (bool, Option) { + let Some(info) = channel_info.resolve(channel_id).await else { + return (true, None); + }; + let is_dm = info.channel_type == "dm"; + let title_channel = (!is_dm && info.name != UNKNOWN_CHANNEL_NAME).then_some(info.name); + (is_dm, title_channel) +} + /// Create a new ACP session via `session_new_full()`, populate model capabilities /// on the agent (first session only), and apply `desired_model` if set. /// @@ -1437,18 +1480,20 @@ pub async fn run_prompt_task( // prevents a stale revision A surviving a failed create and being re-used by // the next attempt after the canvas was cleared. let mut pending_canvas: Option<(Uuid, String)> = None; + // Channel name for the session title, from the same single resolve the + // canvas DM check uses — see `resolve_new_session_channel_context`. + let mut title_channel: Option = None; if let PromptSource::Channel(cid) = &source { let is_new_channel_session = !agent.state.sessions.contains_key(cid); - if is_new_channel_session && !agent.state.canvas_sections.contains_key(cid) { - // Resolve DM status: prefer the startup cache, lazy-fetch as fallback. - // Unknown → treat as DM (fail-closed). - let is_dm = ctx - .channel_info - .resolve(*cid) - .await - .map(|ci| ci.channel_type == "dm") - .unwrap_or(true); - if !is_dm { + let needs_canvas = is_new_channel_session && !agent.state.canvas_sections.contains_key(cid); + let needs_title = is_new_channel_session && ctx.session_title.is_some(); + if needs_canvas || needs_title { + let (is_dm, resolved_channel) = + resolve_new_session_channel_context(&ctx.channel_info, *cid).await; + title_channel = resolved_channel; + // A confirmed DM never receives a canvas section; an undeterminable + // channel type fails closed as a DM for the same reason. + if needs_canvas && !is_dm { if let Some(section) = fetch_canvas_section(*cid, &ctx.rest_client).await { pending_canvas = Some((*cid, section)); } @@ -1480,25 +1525,16 @@ pub async fn run_prompt_task( if let Some(sid) = agent.state.sessions.get(cid) { (sid.clone(), false) } else { - // Channel-qualify the title (`Agent · #channel`) so one agent in - // several channels doesn't produce identical session rows. - // Cache-first; DM or unresolved channels keep the bare name. - let channel_name = if ctx.session_title.is_some() { - ctx.channel_info - .resolve(*cid) - .await - .filter(|ci| ci.channel_type != "dm") - .map(|ci| ci.name) - } else { - None - }; - // Create new session with model application. + // The title is channel-qualified (`Agent · #channel`) so one + // agent in several channels doesn't produce identical session + // rows; `title_channel` comes from the single resolve above and + // is `None` for DM, unresolved, and unnamed channels. match create_session_and_apply_model( &mut agent, &ctx, agent_core.as_deref(), agent_canvas.as_deref(), - channel_name.as_deref(), + title_channel.as_deref(), ) .await { @@ -2291,7 +2327,7 @@ pub(crate) async fn fetch_channel_info( } let channel_type = crate::relay::channel_type_from_tags(tags); Some(PromptChannelInfo { - name: name.unwrap_or("unknown").to_string(), + name: name.unwrap_or(UNKNOWN_CHANNEL_NAME).to_string(), channel_type, }) } @@ -3804,6 +3840,9 @@ mod tests { #[test] fn test_framed_system_prompt_both_present_carries_both_headers() { + // Also the regression guard against #2372: the session title travels + // out of band in `_meta.sessionTitle`, so this exact-bytes assertion is + // what pins the framing against a `[Session]` section reappearing here. let framed = framed_system_prompt("/", Some("base text"), Some("persona text")) .expect("both present yields Some"); assert_eq!(framed, "[Base]\nbase text\n\n[System]\npersona text"); @@ -3824,17 +3863,6 @@ mod tests { assert_eq!(framed, "[System]\npersona text"); } - #[test] - fn test_framed_system_prompt_never_carries_a_session_title_section() { - // Regression against #2372: the title travels out of band in - // `_meta.sessionTitle`, so no `[Session]` section may appear in the - // prompt and the base framing must stay byte-identical. - let framed = framed_system_prompt("/", Some("base text"), Some("persona text")) - .expect("both present yields Some"); - assert_eq!(framed, "[Base]\nbase text\n\n[System]\npersona text"); - assert!(!framed.contains("[Session]")); - } - #[test] fn test_framed_system_prompt_neither_is_none() { assert!(framed_system_prompt("/", None, None).is_none()); @@ -5652,4 +5680,142 @@ mod tests { "timestamp must not use +00:00 offset" ); } + + // ── new-session channel context (one resolve, two consumers) ───────────── + + /// A [`ChannelInfoResolver`] whose lazy REST fallback is served by a local + /// HTTP server, plus a counter of the requests that actually reached it. + /// Counting real requests is the point: the composition tests are pure and + /// cannot see duplicated I/O. + async fn counting_resolver( + response: serde_json::Value, + ) -> ( + ChannelInfoResolver, + std::sync::Arc, + tokio::task::JoinHandle<()>, + ) { + use std::sync::atomic::{AtomicUsize, Ordering}; + use tokio::io::{AsyncReadExt, AsyncWriteExt}; + + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("bind test HTTP server"); + let base_url = format!("http://{}", listener.local_addr().unwrap()); + let requests = std::sync::Arc::new(AtomicUsize::new(0)); + let server_requests = requests.clone(); + let body = response.to_string(); + let server = tokio::spawn(async move { + while let Ok((mut socket, _)) = listener.accept().await { + let mut buf = vec![0; 8192]; + let _ = socket.read(&mut buf).await; + server_requests.fetch_add(1, Ordering::SeqCst); + let response = format!( + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{}", + body.len(), + body + ); + let _ = socket.write_all(response.as_bytes()).await; + } + }); + let rest = crate::relay::RestClient { + http: reqwest::Client::new(), + base_url, + keys: nostr::Keys::generate(), + auth_tag_json: None, + }; + ( + ChannelInfoResolver::new(std::collections::HashMap::new(), rest), + requests, + server, + ) + } + + fn channel_metadata_response(id: Uuid, tags: &[[&str; 2]]) -> serde_json::Value { + let mut event_tags = vec![json!(["d", id.to_string()])]; + event_tags.extend(tags.iter().map(|[k, v]| json!([k, v]))); + json!([{ "tags": event_tags }]) + } + + /// A normal channel yields a non-DM (canvas allowed) and its name for the + /// title suffix — and the second consumer reads it from cache, not the wire. + #[tokio::test] + async fn test_new_session_channel_context_qualifies_a_normal_channel() { + use std::sync::atomic::Ordering; + + let id = Uuid::new_v4(); + let response = channel_metadata_response(id, &[["name", "buzz-dev"], ["t", "stream"]]); + let (resolver, requests, server) = counting_resolver(response).await; + + let (is_dm, title_channel) = resolve_new_session_channel_context(&resolver, id).await; + assert!(!is_dm, "a stream channel is not a DM"); + assert_eq!(title_channel.as_deref(), Some("buzz-dev")); + assert_eq!(requests.load(Ordering::SeqCst), 1); + + let (_, again) = resolve_new_session_channel_context(&resolver, id).await; + assert_eq!(again.as_deref(), Some("buzz-dev")); + assert_eq!( + requests.load(Ordering::SeqCst), + 1, + "a resolved channel is cached — no second lookup" + ); + server.abort(); + } + + /// A DM carries no useful name, so it gets the bare agent title (and no + /// canvas section). + #[tokio::test] + async fn test_new_session_channel_context_leaves_a_dm_unqualified() { + let id = Uuid::new_v4(); + let response = channel_metadata_response(id, &[["name", "DM"], ["t", "dm"]]); + let (resolver, _requests, server) = counting_resolver(response).await; + + let (is_dm, title_channel) = resolve_new_session_channel_context(&resolver, id).await; + assert!(is_dm); + assert_eq!( + title_channel, None, + "a DM name must never reach the session title" + ); + server.abort(); + } + + /// The `"unknown"` placeholder `fetch_channel_info` substitutes for a + /// metadata event with no `name` tag is not a channel name: qualifying with + /// it would title every unnamed channel `Agent · #unknown`. + #[tokio::test] + async fn test_new_session_channel_context_treats_the_unknown_name_as_absent() { + let id = Uuid::new_v4(); + let response = channel_metadata_response(id, &[["t", "stream"]]); + let (resolver, _requests, server) = counting_resolver(response).await; + + let (is_dm, title_channel) = resolve_new_session_channel_context(&resolver, id).await; + assert!(!is_dm, "a nameless stream channel is still not a DM"); + assert_eq!( + title_channel, None, + "the `unknown` placeholder must yield a bare title" + ); + server.abort(); + } + + /// An unresolvable channel yields the bare title, fails closed as a DM, and + /// costs exactly ONE `fetch_channel_info` sequence — two attempts, because + /// `fetch_with_retry` retries once. `resolve()` caches only `Some`, so a + /// second resolve for the title would double this in front of `session/new`, + /// exactly when the relay is already degraded. + #[tokio::test] + async fn test_new_session_channel_context_attempts_an_unresolved_channel_once() { + use std::sync::atomic::Ordering; + + let (resolver, requests, server) = counting_resolver(json!([])).await; + + let (is_dm, title_channel) = + resolve_new_session_channel_context(&resolver, Uuid::new_v4()).await; + assert!(is_dm, "an undeterminable channel type must fail closed"); + assert_eq!(title_channel, None, "unresolved channels get a bare title"); + assert_eq!( + requests.load(Ordering::SeqCst), + 2, + "one fetch_channel_info sequence (initial attempt + single retry)" + ); + server.abort(); + } } diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index 8bad876429..8de72ec1a8 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -189,10 +189,11 @@ const overrides = new Map([ // ownership (valid_agent_runtime_receipt uses buzz_sweep_owns_process). ["src-tauri/src/managed_agents/runtime/tests.rs", 1320], // runtime.rs re-entered the list after the #1968 merge: main's - // definition-authoritative resolver comments grew it to 982, and this PR's - // typed harness-descriptor resolution in spawn_agent_child (+38) lands on - // top. Queued to shrink with the next runtime split pass (#2974 follow-up). - ["src-tauri/src/managed_agents/runtime.rs", 1020], + // definition-authoritative resolver comments grew it to 982, and the BYOH + // typed harness-descriptor resolution in spawn_agent_child landed on top at + // 1020. This PR's session-title env write in spawn_agent_child adds 12. + // Queued to shrink with the next runtime split pass (#2974 follow-up). + ["src-tauri/src/managed_agents/runtime.rs", 1032], // applyWorkspace reposDir parameter plus the validateReposDir binding, // threaded through Tauri invokes for configurable repos_dir, plus the // harness-persona-sync `harnessOverride` create-input bit — load-bearing diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index 5329aff185..3658f48a95 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -23,6 +23,7 @@ pub(crate) use path::should_use_inherited; mod metadata; pub(crate) use metadata::{ resolve_effective_prompt_model_provider, resolve_session_title, runtime_metadata_env_vars, + SESSION_TITLE_ENV_VAR, }; mod stop; @@ -770,11 +771,12 @@ pub fn spawn_agent_child( } // Session title for the harness to pass out-of-band on `session/new`. The // adapter names the session after it; it never reaches the prompt, so this - // is display metadata only. + // is display metadata only. `spawn_config_hash` hashes the same resolve, so + // a rename raises the restart badge instead of leaving the process stale. if let Some(title) = resolve_session_title(record.display_name.as_deref(), &record.name) { - command.env("BUZZ_ACP_SESSION_TITLE", title); + command.env(SESSION_TITLE_ENV_VAR, title); } else { - command.env_remove("BUZZ_ACP_SESSION_TITLE"); + command.env_remove(SESSION_TITLE_ENV_VAR); } build_buzz_agent_provider_defaults(&mut command); if let Some(meta) = runtime_meta { diff --git a/desktop/src-tauri/src/managed_agents/runtime/metadata.rs b/desktop/src-tauri/src/managed_agents/runtime/metadata.rs index f0c5e59ae4..288ce06b0a 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/metadata.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/metadata.rs @@ -24,21 +24,36 @@ pub(crate) fn runtime_metadata_env_vars<'a>( vars } +/// Env var carrying the session title to the harness. Shared with +/// `spawn_hash` so the restart badge hashes the same key the spawn writes. +pub(crate) const SESSION_TITLE_ENV_VAR: &str = "BUZZ_ACP_SESSION_TITLE"; + /// Resolve the session title for an agent: its `display_name` when it has one, /// otherwise its unique `name` handle. `None` when both are blank, so the /// caller clears the env var rather than exporting an empty title. /// -/// Sanitization (control chars, whitespace, length cap) and channel -/// qualification are the harness's job — see `sanitize_session_title` and -/// `compose_session_title` in `buzz-acp`. -pub(crate) fn resolve_session_title<'a>( - display_name: Option<&'a str>, - name: &'a str, -) -> Option<&'a str> { +/// Control characters are stripped **here**, not left to the harness: an +/// interior NUL cannot cross the environment boundary at all, so +/// `Command::env` fails the whole spawn rather than passing it through (see +/// the same guard applied to user-supplied env in `env_vars::merged_user_env`). +/// A display name that is nothing but control characters therefore falls back +/// to `name` instead of turning display chrome into a spawn failure. +/// +/// The harness still owns whitespace collapsing, the length cap, and channel +/// qualification — see `sanitize_session_title` and `compose_session_title` in +/// `buzz-acp`. +pub(crate) fn resolve_session_title(display_name: Option<&str>, name: &str) -> Option { [display_name, Some(name)] .into_iter() .flatten() - .map(str::trim) + .map(|value| { + value + .chars() + .filter(|c| !c.is_control()) + .collect::() + .trim() + .to_string() + }) .find(|value| !value.is_empty()) } @@ -74,13 +89,22 @@ mod tests { #[test] fn resolve_session_title_prefers_display_name() { - assert_eq!(resolve_session_title(Some("Fizz"), "fizz-1"), Some("Fizz")); + assert_eq!( + resolve_session_title(Some("Fizz"), "fizz-1").as_deref(), + Some("Fizz") + ); } #[test] fn resolve_session_title_falls_back_to_name_when_display_name_blank() { - assert_eq!(resolve_session_title(None, "fizz-1"), Some("fizz-1")); - assert_eq!(resolve_session_title(Some(" "), "fizz-1"), Some("fizz-1")); + assert_eq!( + resolve_session_title(None, "fizz-1").as_deref(), + Some("fizz-1") + ); + assert_eq!( + resolve_session_title(Some(" "), "fizz-1").as_deref(), + Some("fizz-1") + ); } #[test] @@ -91,8 +115,36 @@ mod tests { #[test] fn resolve_session_title_trims_surrounding_whitespace() { assert_eq!( - resolve_session_title(Some(" Fizz "), "fizz-1"), + resolve_session_title(Some(" Fizz "), "fizz-1").as_deref(), Some("Fizz") ); } + + /// An interior NUL cannot cross the env boundary — `Command::env` returns + /// `Err` for the whole spawn. Stripping it here keeps a corrupted record + /// from turning display chrome into a spawn failure. + #[test] + fn resolve_session_title_strips_control_chars_that_would_fail_the_spawn() { + let title = resolve_session_title(Some("Fi\u{0}zz\u{7}"), "fizz-1") + .expect("a name with strippable controls still yields a title"); + assert_eq!(title, "Fizz"); + assert!(!title.contains('\u{0}')); + } + + /// A display name that is *only* control characters is not a title, so the + /// unique handle takes over rather than exporting an empty value. + #[test] + fn resolve_session_title_falls_back_to_name_when_display_name_is_all_control_chars() { + assert_eq!( + resolve_session_title(Some("\u{0}\u{1}"), "fizz-1").as_deref(), + Some("fizz-1") + ); + } + + /// Both candidates unusable — the caller clears the env var instead of + /// exporting a NUL-bearing or empty title. + #[test] + fn resolve_session_title_returns_none_when_both_candidates_are_control_chars_only() { + assert_eq!(resolve_session_title(Some("\u{0}"), "\u{0}"), None); + } } diff --git a/desktop/src-tauri/src/managed_agents/spawn_hash.rs b/desktop/src-tauri/src/managed_agents/spawn_hash.rs index e2884bfa15..648cc62bbe 100644 --- a/desktop/src-tauri/src/managed_agents/spawn_hash.rs +++ b/desktop/src-tauri/src/managed_agents/spawn_hash.rs @@ -31,6 +31,7 @@ use super::{ effective_config::{resolve_effective_config, EffectiveConfigResult}, known_acp_runtime, normalize_agent_args, persona_events::preview_prospective_persona_snapshot, + runtime::{resolve_session_title, SESSION_TITLE_ENV_VAR}, types::{AgentDefinition, ManagedAgentRecord, TeamRecord}, GlobalAgentConfig, }; @@ -124,6 +125,16 @@ pub(crate) fn spawn_config_hash( resolved_prompt.hash(&mut hasher); resolved_model.hash(&mut hasher); resolved_provider.hash(&mut hasher); + // Session title: the same resolve `spawn_agent_child` performs for its env + // write, so a rename raises the restart badge. Skipped when a user env + // override shadows it — spawn writes the title BEFORE the user env layer, + // so the override is what actually runs, and it already reaches this hash + // through `descriptor.env` above. Hashing the record-derived value under an + // override would badge a rename that changes nothing. + let effective_session_title = (!descriptor.env.contains_key(SESSION_TITLE_ENV_VAR)) + .then(|| resolve_session_title(record.display_name.as_deref(), &record.name)) + .flatten(); + effective_session_title.hash(&mut hasher); record.auth_tag.hash(&mut hasher); record.respond_to.as_str().hash(&mut hasher); // The allowlist is hashed as the env receives it: spawn sets diff --git a/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs b/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs index 292cdbbb79..686ad52d4f 100644 --- a/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs +++ b/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs @@ -543,6 +543,72 @@ fn linked_instance_stale_prompt_bytes_are_inert_at_hash_time() { ); } +#[test] +fn display_name_edit_changes_hash() { + // The spawn writes BUZZ_ACP_SESSION_TITLE from display_name-or-name, so a + // rename must trip the badge: the running process keeps the old title + // until it restarts, and the operator has to be told that. + let rec = record(); + let mut renamed = record(); + renamed.display_name = Some("Fizz".into()); + assert_ne!( + spawn_config_hash(&rec, &[], &[], "wss://ws.example", &Default::default()), + spawn_config_hash(&renamed, &[], &[], "wss://ws.example", &Default::default()), + "a display-name rename changes the spawned session title and must badge" + ); +} + +#[test] +fn name_edit_changes_hash_when_display_name_is_absent() { + // With no display_name the title falls back to the unique handle, so the + // handle is what the env write carries and what must be hashed. + let rec = record(); + let mut renamed = record(); + renamed.name = "agent-2".into(); + assert_ne!( + spawn_config_hash(&rec, &[], &[], "wss://ws.example", &Default::default()), + spawn_config_hash(&renamed, &[], &[], "wss://ws.example", &Default::default()), + "the fallback title source must reach the hash too" + ); +} + +#[test] +fn display_name_edit_does_not_change_hash_under_an_explicit_title_override() { + // User env is written AFTER the Buzz-set title (last-wins), so an explicit + // BUZZ_ACP_SESSION_TITLE is what the child actually runs with. Renaming the + // record changes nothing about the spawned process, so badging it would be + // a false restart prompt. The override itself still reaches the hash + // through the effective env. + let mut rec = record(); + rec.env_vars + .insert("BUZZ_ACP_SESSION_TITLE".into(), "Pinned Title".into()); + let mut renamed = rec.clone(); + renamed.display_name = Some("Fizz".into()); + assert_eq!( + spawn_config_hash(&rec, &[], &[], "wss://ws.example", &Default::default()), + spawn_config_hash(&renamed, &[], &[], "wss://ws.example", &Default::default()), + "a rename shadowed by an explicit title override must not badge" + ); +} + +#[test] +fn title_override_edit_changes_hash() { + // Counterpart to the test above: the override is not inert — editing it + // changes what the child runs with and must badge. + let mut rec = record(); + rec.env_vars + .insert("BUZZ_ACP_SESSION_TITLE".into(), "Pinned Title".into()); + let mut edited = record(); + edited + .env_vars + .insert("BUZZ_ACP_SESSION_TITLE".into(), "Other Title".into()); + assert_ne!( + spawn_config_hash(&rec, &[], &[], "wss://ws.example", &Default::default()), + spawn_config_hash(&edited, &[], &[], "wss://ws.example", &Default::default()), + "editing an explicit title override must badge" + ); +} + #[test] fn linked_instance_prompt_model_provider_resolve_from_one_call() { // The prompt for a linked instance must track the definition, exactly