From 5243854129549f9b0d42ceb6ed5ef16c5429bae2 Mon Sep 17 00:00:00 2001 From: Huskies Agent Date: Sat, 18 Jul 2026 01:15:10 +0000 Subject: [PATCH] =?UTF-8?q?huskies:=20merge=201205=20bug=20compact=20STILL?= =?UTF-8?q?=20swallowed=20after=201192=20=E2=80=94=20interception=20is=20p?= =?UTF-8?q?laced=20AFTER=20the=20registry=20dispatch,=20not=20before?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- server/src/chat/commands/mod.rs | 17 +- .../matrix/bot/messages/on_room_message.rs | 311 ++++++++++++++++-- 2 files changed, 288 insertions(+), 40 deletions(-) diff --git a/server/src/chat/commands/mod.rs b/server/src/chat/commands/mod.rs index ab65096b..308757db 100644 --- a/server/src/chat/commands/mod.rs +++ b/server/src/chat/commands/mod.rs @@ -425,15 +425,14 @@ fn handle_reset_fallback(_ctx: &CommandContext) -> Option { None } -/// Fallback handler for the `compact` command. -/// -/// This IS called on every `compact` dispatch — `try_handle_command` always -/// invokes the matched handler, so this runs before the async `compact` -/// check each transport performs afterward (`on_room_message` for Matrix; an -/// inline check in `handle_incoming_message` for Discord/Slack/WhatsApp). It -/// deliberately always returns `None` so that check gets a chance to run the -/// real handler instead of the LLM. The entry exists in the registry so -/// `help` lists it. +/// Fallback handler for the `compact` command when it is not intercepted by +/// the async handler beforehand. For Matrix, `on_room_message` detects and +/// handles `compact` before `try_handle_command` is invoked, so this is +/// never called. Discord/Slack/WhatsApp check for `compact` only after their +/// own `try_handle_command` dispatch, so this handler does run there — it +/// deliberately always returns `None` so their subsequent async check gets a +/// chance to run the real handler instead of the LLM. The entry exists in +/// the registry so `help` lists it. /// /// Returns `None` to prevent the LLM from receiving "compact" as a prompt. fn handle_compact_fallback(_ctx: &CommandContext) -> Option { diff --git a/server/src/chat/transport/matrix/bot/messages/on_room_message.rs b/server/src/chat/transport/matrix/bot/messages/on_room_message.rs index 36431baa..5e5a1ca6 100644 --- a/server/src/chat/transport/matrix/bot/messages/on_room_message.rs +++ b/server/src/chat/transport/matrix/bot/messages/on_room_message.rs @@ -247,6 +247,54 @@ pub(super) async fn eval_switch_command( } } +/// Attempt to handle an addressed message as a `compact` command. +/// +/// Returns `true` when `user_message` was recognised as `compact` and the +/// real handler ran (a reply was sent via `ctx.transport`, clearing the +/// room's session and setting `pending_seed`). Returns `false` so the caller +/// falls through to the sync command registry and, eventually, the LLM. +/// +/// Must be called before the registry dispatch (`try_handle_command_with_html`): +/// the registry's `compact` entry (`handle_compact_fallback`) always returns +/// `None` so `help` can list it, which means this call site — not the +/// registry — is what makes `compact` reachable. Extracted into its own +/// function so it is directly testable without a live Matrix `Room`/`Client`. +async fn try_handle_compact_command( + ctx: &BotContext, + sender: &str, + user_message: &str, + room_id_str: &str, + incoming_room_id: &matrix_sdk::ruma::OwnedRoomId, +) -> bool { + if super::super::super::compact::extract_compact_command( + user_message, + &ctx.services.bot_name, + ctx.matrix_user_id.as_str(), + ) + .is_none() + { + return false; + } + slog!("[matrix-bot] Handling compact command from {sender}"); + let response = super::super::super::compact::handle_compact( + incoming_room_id, + &ctx.history, + &ctx.services.project_root, + ctx.compact_seed_max_bytes, + ) + .await; + let html = markdown_to_html(&response); + if let Ok(msg_id) = ctx + .transport + .send_message(room_id_str, &response, &html) + .await + && let Ok(event_id) = msg_id.parse() + { + ctx.bot_sent_event_ids.lock().await.insert(event_id); + } + true +} + pub(in crate::chat::transport::matrix::bot) async fn on_room_message( ev: OriginalSyncRoomMessageEvent, room: Room, @@ -946,6 +994,23 @@ pub(in crate::chat::transport::matrix::bot) async fn on_room_message( return; } + // Check for the compact command, which requires async access to the + // shared conversation history and cannot be handled by the sync command + // registry. Must run before the registry dispatch below — the registry's + // `compact` entry exists only so `help` lists it and always returns + // `None`, so placement here (not after) is what makes this reachable. + if try_handle_compact_command( + &ctx, + &sender, + &user_message, + &room_id_str, + &incoming_room_id, + ) + .await + { + return; + } + // Check for bot-level commands (help, status, ambient, …) before invoking // the LLM. All commands are registered in commands.rs — no special-casing // needed here. @@ -1158,36 +1223,6 @@ pub(in crate::chat::transport::matrix::bot) async fn on_room_message( return; } - // Check for the compact command, which requires async access to the - // shared conversation history and cannot be handled by the sync command - // registry. - if super::super::super::compact::extract_compact_command( - &user_message, - &ctx.services.bot_name, - ctx.matrix_user_id.as_str(), - ) - .is_some() - { - slog!("[matrix-bot] Handling compact command from {sender}"); - let response = super::super::super::compact::handle_compact( - &incoming_room_id, - &ctx.history, - &ctx.services.project_root, - ctx.compact_seed_max_bytes, - ) - .await; - let html = markdown_to_html(&response); - if let Ok(msg_id) = ctx - .transport - .send_message(&room_id_str, &response, &html) - .await - && let Ok(event_id) = msg_id.parse() - { - ctx.bot_sent_event_ids.lock().await.insert(event_id); - } - return; - } - // In gateway mode, intercept "rebuild gateway" and route it through the // detached trampoline so the process swap survives any bash-tool kill cascade. if ctx.gateway_active_project.is_some() @@ -1420,8 +1455,13 @@ pub(in crate::chat::transport::matrix::bot) async fn on_room_message( #[cfg(test)] mod tests { - use super::{eval_gateway_overview_command, eval_gateway_status_command, eval_switch_command}; + use super::{ + eval_gateway_overview_command, eval_gateway_status_command, eval_switch_command, + try_handle_compact_command, + }; + use crate::chat::{ChatTransport, MessageId}; use crate::service::gateway::config::ProjectEntry; + use async_trait::async_trait; use std::collections::BTreeMap; use tokio::sync::RwLock; @@ -1729,4 +1769,213 @@ mod tests { "numeric arg should proxy to the active project, reporting no URL: {resp}" ); } + + // ── compact (story 1205: still swallowed after 1192) ──────────────── + + /// Captures every message sent through it, for assertion in tests. + struct CapturingTransport { + sent: std::sync::Mutex>, + } + + impl CapturingTransport { + fn new() -> Self { + Self { + sent: std::sync::Mutex::new(Vec::new()), + } + } + } + + #[async_trait] + impl ChatTransport for CapturingTransport { + async fn send_message( + &self, + room_id: &str, + plain: &str, + _html: &str, + ) -> Result { + self.sent + .lock() + .unwrap() + .push((room_id.to_string(), plain.to_string())); + Ok("msg-id".to_string()) + } + + async fn edit_message( + &self, + _room_id: &str, + _original_message_id: &str, + _plain: &str, + _html: &str, + ) -> Result<(), String> { + Ok(()) + } + + async fn send_typing(&self, _room_id: &str, _typing: bool) -> Result<(), String> { + Ok(()) + } + } + + /// Build a minimal [`super::BotContext`] for driving [`try_handle_compact_command`] + /// directly in tests, without a live Matrix `Room`/`Client`. + fn make_test_ctx( + services: std::sync::Arc, + transport: std::sync::Arc, + ) -> super::BotContext { + use std::collections::HashSet; + use std::sync::Arc; + use std::sync::atomic::AtomicI64; + use tokio::sync::Mutex as TokioMutex; + + super::BotContext { + services, + matrix_user_id: "@bot:example.com".parse().unwrap(), + target_room_ids: vec![], + allowed_users: vec![], + history: Arc::new(TokioMutex::new(std::collections::HashMap::new())), + history_size: 20, + bot_sent_event_ids: Arc::new(TokioMutex::new(HashSet::new())), + htop_sessions: Arc::new(TokioMutex::new(std::collections::HashMap::new())), + transport, + timer_store: Arc::new(crate::service::timer::TimerStore::load( + std::path::PathBuf::from("/tmp/timers-on-room-message-compact-test.json"), + )), + gateway_active_project: None, + gateway_projects_store: None, + gateway_channels_store: None, + handled_incoming_event_ids: Arc::new(TokioMutex::new( + crate::chat::transport::matrix::bot::context::SeenEventIds::new( + crate::chat::transport::matrix::bot::context::SEEN_EVENT_IDS_CAP, + ), + )), + gateway_port: None, + last_matrix_event_ms: Arc::new(AtomicI64::new(chrono::Utc::now().timestamp_millis())), + model: None, + compact_seed_max_bytes: 8_000, + cache_read_suggest_threshold: 50_000, + compact_suggest_cooldown_secs: 3_600, + } + } + + /// Regression test for story 1205: `compact` must not be swallowed by the + /// registry placeholder. Drives `try_handle_compact_command` — the same + /// entry point `on_room_message` calls before the sync command registry — + /// with the literal message `"compact"` (never calling `handle_compact` + /// directly), and asserts the real handler ran: the captured transport + /// message reports the before/after byte-size confirmation (never a + /// generic "no response" placeholder), and the room's session_id/entries + /// were cleared with pending_seed set. + #[tokio::test] + async fn compact_command_runs_through_full_dispatch_and_clears_session() { + use crate::chat::transport::matrix::bot::{ConversationEntry, ConversationRole}; + use std::sync::Arc; + + let room_id: matrix_sdk::ruma::OwnedRoomId = "!test:example.com".parse().unwrap(); + let session_id = "sess-matrix-compact-1205"; + + let project_root_dir = tempfile::tempdir().unwrap(); + let project_root = project_root_dir.path().to_path_buf(); + let home = tempfile::tempdir().unwrap(); + // SAFETY: this test owns HOME for its duration; no other test in this + // process reads HOME concurrently with this call. + unsafe { + std::env::set_var("HOME", home.path()); + } + let transcript_dir = + crate::chat::compact::transcript::transcript_path(&project_root, session_id) + .parent() + .unwrap() + .to_path_buf(); + std::fs::create_dir_all(&transcript_dir).unwrap(); + let jsonl = r#"{"type":"user","message":{"role":"user","content":"hello"}} +{"type":"assistant","message":{"role":"assistant","content":[{"type":"text","text":"hi there"}]}}"#; + std::fs::write(transcript_dir.join(format!("{session_id}.jsonl")), jsonl).unwrap(); + + let services = + crate::services::Services::new_test(project_root.clone(), "Huskies".to_string()); + let transport = Arc::new(CapturingTransport::new()); + let ctx = make_test_ctx(services, transport.clone()); + + { + let mut guard = ctx.history.lock().await; + guard.insert( + room_id.clone(), + crate::chat::transport::matrix::bot::RoomConversation { + session_id: Some(session_id.to_string()), + entries: vec![ConversationEntry { + role: ConversationRole::User, + sender: "@alice:example.com".to_string(), + content: "hi".to_string(), + }], + pending_seed: None, + last_compact_suggested_at_ms: None, + }, + ); + } + + let handled = try_handle_compact_command( + &ctx, + "@alice:example.com", + "compact", + room_id.as_str(), + &room_id, + ) + .await; + + assert!(handled, "compact must be recognized and handled"); + + let sent = transport.sent.lock().unwrap().clone(); + assert_eq!(sent.len(), 1, "exactly one reply must be sent"); + assert!( + sent[0].1.contains("Compacted session context"), + "reply must confirm compaction with before/after byte sizes, not a generic \ + placeholder like 'Command succeeded with no response text': {}", + sent[0].1 + ); + + let guard = ctx.history.lock().await; + let conv = guard.get(&room_id).unwrap(); + assert!( + conv.session_id.is_none(), + "session_id must be cleared after compact" + ); + assert!( + conv.entries.is_empty(), + "entries must be cleared after compact" + ); + assert!( + conv.pending_seed.is_some(), + "pending_seed must be set after compact" + ); + } + + /// A message that is not `compact` must fall through untouched, so the + /// caller can continue to the sync command registry / LLM. + #[tokio::test] + async fn non_compact_message_falls_through() { + use std::sync::Arc; + + let room_id: matrix_sdk::ruma::OwnedRoomId = "!test:example.com".parse().unwrap(); + let project_root_dir = tempfile::tempdir().unwrap(); + let services = crate::services::Services::new_test( + project_root_dir.path().to_path_buf(), + "Huskies".to_string(), + ); + let transport = Arc::new(CapturingTransport::new()); + let ctx = make_test_ctx(services, transport.clone()); + + let handled = try_handle_compact_command( + &ctx, + "@alice:example.com", + "status", + room_id.as_str(), + &room_id, + ) + .await; + + assert!(!handled, "non-compact messages must fall through"); + assert!( + transport.sent.lock().unwrap().is_empty(), + "no reply should be sent for a message that isn't compact" + ); + } }