huskies: merge 1205 bug compact STILL swallowed after 1192 — interception is placed AFTER the registry dispatch, not before

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<Vec<(String, String)>>,
+    }
+
+    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<MessageId, String> {
+            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<crate::services::Services>,
+        transport: std::sync::Arc<dyn ChatTransport>,
+    ) -> 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"
+        );
+    }
 }