story-kit: merge 352_bug_ambient_on_off_command_not_intercepted_by_bot_after_refactors

This commit is contained in:
Dave
2026-03-20 11:16:48 +00:00
parent 4268db8641
commit d7e814c02c
10 changed files with 43 additions and 39 deletions

View File

@@ -817,7 +817,6 @@ async fn on_room_message(
agents: &ctx.agents, agents: &ctx.agents,
ambient_rooms: &ctx.ambient_rooms, ambient_rooms: &ctx.ambient_rooms,
room_id: &room_id_str, room_id: &room_id_str,
is_addressed,
}; };
if let Some(response) = super::commands::try_handle_command(&dispatch, &user_message) { if let Some(response) = super::commands::try_handle_command(&dispatch, &user_message) {
slog!("[matrix-bot] Handled bot command from {sender}"); slog!("[matrix-bot] Handled bot command from {sender}");

View File

@@ -5,12 +5,11 @@ use crate::matrix::config::save_ambient_rooms;
/// Toggle ambient mode for this room. /// Toggle ambient mode for this room.
/// ///
/// Only acts when the message directly addressed the bot (`is_addressed=true`) /// Works whether or not the message directly addressed the bot — the user can
/// to prevent accidental toggling via ambient-mode traffic. /// say "timmy ambient on", "@timmy ambient on", or just "ambient on" in an
/// ambient-mode room. The command is specific enough (must be the first word
/// after any bot-mention prefix) that accidental triggering is very unlikely.
pub(super) fn handle_ambient(ctx: &CommandContext) -> Option<String> { pub(super) fn handle_ambient(ctx: &CommandContext) -> Option<String> {
if !ctx.is_addressed {
return None;
}
let enable = match ctx.args { let enable = match ctx.args {
"on" => true, "on" => true,
"off" => false, "off" => false,
@@ -50,8 +49,12 @@ mod tests {
Arc::new(AgentPool::new_test(3000)) Arc::new(AgentPool::new_test(3000))
} }
// Bug 352: ambient commands were being forwarded to LLM after refactors
// 328/330 because handle_ambient required is_addressed=true, but
// mentions_bot() only matches @-prefixed mentions, not bare bot names.
// "timmy ambient off" sets is_addressed=false even though it names the bot.
#[test] #[test]
fn ambient_on_requires_addressed() { fn ambient_on_works_when_unaddressed() {
let ambient_rooms = test_ambient_rooms(); let ambient_rooms = test_ambient_rooms();
let room_id = "!myroom:example.com".to_string(); let room_id = "!myroom:example.com".to_string();
let agents = test_agents(); let agents = test_agents();
@@ -62,14 +65,42 @@ mod tests {
agents: &agents, agents: &agents,
ambient_rooms: &ambient_rooms, ambient_rooms: &ambient_rooms,
room_id: &room_id, room_id: &room_id,
is_addressed: false, // not addressed
}; };
let result = try_handle_command(&dispatch, "@timmy ambient on"); // "timmy ambient on" — bot name mentioned but not @-prefixed, so
// Should fall through to LLM when not addressed // is_addressed is false; strip_bot_mention still strips "timmy ".
assert!(result.is_none(), "ambient should not fire in non-addressed mode"); let result = try_handle_command(&dispatch, "timmy ambient on");
assert!(result.is_some(), "ambient on should fire even when is_addressed=false");
assert!(
ambient_rooms.lock().unwrap().contains(&room_id),
"room should be in ambient_rooms after ambient on"
);
}
#[test]
fn ambient_off_works_bare_in_ambient_room() {
let ambient_rooms = test_ambient_rooms();
let room_id = "!myroom:example.com".to_string();
ambient_rooms.lock().unwrap().insert(room_id.clone());
let agents = test_agents();
let dispatch = CommandDispatch {
bot_name: "Timmy",
bot_user_id: "@timmy:homeserver.local",
project_root: std::path::Path::new("/tmp"),
agents: &agents,
ambient_rooms: &ambient_rooms,
room_id: &room_id,
};
// Bare "ambient off" in an ambient room (is_addressed=false).
let result = try_handle_command(&dispatch, "ambient off");
assert!(result.is_some(), "bare ambient off should be handled without LLM");
let output = result.unwrap();
assert!(
output.contains("Ambient mode off"),
"response should confirm ambient off: {output}"
);
assert!( assert!(
!ambient_rooms.lock().unwrap().contains(&room_id), !ambient_rooms.lock().unwrap().contains(&room_id),
"ambient_rooms should not be modified when not addressed" "room should be removed from ambient_rooms after ambient off"
); );
} }
@@ -85,7 +116,6 @@ mod tests {
agents: &agents, agents: &agents,
ambient_rooms: &ambient_rooms, ambient_rooms: &ambient_rooms,
room_id: &room_id, room_id: &room_id,
is_addressed: true,
}; };
let result = try_handle_command(&dispatch, "@timmy ambient on"); let result = try_handle_command(&dispatch, "@timmy ambient on");
assert!(result.is_some(), "ambient on should produce a response"); assert!(result.is_some(), "ambient on should produce a response");
@@ -115,7 +145,6 @@ mod tests {
agents: &agents, agents: &agents,
ambient_rooms: &ambient_rooms, ambient_rooms: &ambient_rooms,
room_id: &room_id, room_id: &room_id,
is_addressed: true,
}; };
let result = try_handle_command(&dispatch, "@timmy ambient off"); let result = try_handle_command(&dispatch, "@timmy ambient off");
assert!(result.is_some(), "ambient off should produce a response"); assert!(result.is_some(), "ambient off should produce a response");

View File

@@ -144,7 +144,6 @@ mod tests {
agents: &agents, agents: &agents,
ambient_rooms: &ambient_rooms, ambient_rooms: &ambient_rooms,
room_id: &room_id, room_id: &room_id,
is_addressed: true,
}; };
try_handle_command(&dispatch, "@timmy cost") try_handle_command(&dispatch, "@timmy cost")
} }

View File

@@ -121,7 +121,6 @@ mod tests {
agents: &agents, agents: &agents,
ambient_rooms: &ambient_rooms, ambient_rooms: &ambient_rooms,
room_id: &room_id, room_id: &room_id,
is_addressed: true,
}; };
let result = try_handle_command(&dispatch, "@timmy git"); let result = try_handle_command(&dispatch, "@timmy git");
assert!(result.is_some(), "git command should always return Some"); assert!(result.is_some(), "git command should always return Some");
@@ -142,7 +141,6 @@ mod tests {
agents: &agents, agents: &agents,
ambient_rooms: &ambient_rooms, ambient_rooms: &ambient_rooms,
room_id: &room_id, room_id: &room_id,
is_addressed: true,
}; };
let output = try_handle_command(&dispatch, "@timmy git").unwrap(); let output = try_handle_command(&dispatch, "@timmy git").unwrap();
assert!( assert!(
@@ -166,7 +164,6 @@ mod tests {
agents: &agents, agents: &agents,
ambient_rooms: &ambient_rooms, ambient_rooms: &ambient_rooms,
room_id: &room_id, room_id: &room_id,
is_addressed: true,
}; };
let output = try_handle_command(&dispatch, "@timmy git").unwrap(); let output = try_handle_command(&dispatch, "@timmy git").unwrap();
assert!( assert!(
@@ -190,7 +187,6 @@ mod tests {
agents: &agents, agents: &agents,
ambient_rooms: &ambient_rooms, ambient_rooms: &ambient_rooms,
room_id: &room_id, room_id: &room_id,
is_addressed: true,
}; };
let output = try_handle_command(&dispatch, "@timmy git").unwrap(); let output = try_handle_command(&dispatch, "@timmy git").unwrap();
assert!( assert!(

View File

@@ -52,9 +52,6 @@ pub struct CommandDispatch<'a> {
pub ambient_rooms: &'a Arc<Mutex<HashSet<String>>>, pub ambient_rooms: &'a Arc<Mutex<HashSet<String>>>,
/// The room this message came from (needed by ambient). /// The room this message came from (needed by ambient).
pub room_id: &'a str, pub room_id: &'a str,
/// Whether the message directly addressed the bot (mention/reply).
/// Some commands (e.g. ambient) only operate when directly addressed.
pub is_addressed: bool,
} }
/// Context passed to individual command handlers. /// Context passed to individual command handlers.
@@ -71,9 +68,6 @@ pub struct CommandContext<'a> {
pub ambient_rooms: &'a Arc<Mutex<HashSet<String>>>, pub ambient_rooms: &'a Arc<Mutex<HashSet<String>>>,
/// The room this message came from (needed by ambient). /// The room this message came from (needed by ambient).
pub room_id: &'a str, pub room_id: &'a str,
/// Whether the message directly addressed the bot (mention/reply).
/// Some commands (e.g. ambient) only operate when directly addressed.
pub is_addressed: bool,
} }
/// Returns the full list of registered bot commands. /// Returns the full list of registered bot commands.
@@ -171,7 +165,6 @@ pub fn try_handle_command(dispatch: &CommandDispatch<'_>, message: &str) -> Opti
agents: dispatch.agents, agents: dispatch.agents,
ambient_rooms: dispatch.ambient_rooms, ambient_rooms: dispatch.ambient_rooms,
room_id: dispatch.room_id, room_id: dispatch.room_id,
is_addressed: dispatch.is_addressed,
}; };
commands() commands()
@@ -291,7 +284,6 @@ pub(crate) mod tests {
bot_user_id: &str, bot_user_id: &str,
message: &str, message: &str,
ambient_rooms: &Arc<Mutex<HashSet<String>>>, ambient_rooms: &Arc<Mutex<HashSet<String>>>,
is_addressed: bool,
) -> Option<String> { ) -> Option<String> {
let agents = test_agents(); let agents = test_agents();
let room_id = "!test:example.com".to_string(); let room_id = "!test:example.com".to_string();
@@ -302,13 +294,12 @@ pub(crate) mod tests {
agents: &agents, agents: &agents,
ambient_rooms, ambient_rooms,
room_id: &room_id, room_id: &room_id,
is_addressed,
}; };
try_handle_command(&dispatch, message) try_handle_command(&dispatch, message)
} }
pub fn try_cmd_addressed(bot_name: &str, bot_user_id: &str, message: &str) -> Option<String> { pub fn try_cmd_addressed(bot_name: &str, bot_user_id: &str, message: &str) -> Option<String> {
try_cmd(bot_name, bot_user_id, message, &test_ambient_rooms(), true) try_cmd(bot_name, bot_user_id, message, &test_ambient_rooms())
} }
// Re-export commands() for submodule tests // Re-export commands() for submodule tests

View File

@@ -138,7 +138,6 @@ mod tests {
agents: &agents, agents: &agents,
ambient_rooms: &ambient_rooms, ambient_rooms: &ambient_rooms,
room_id: &room_id, room_id: &room_id,
is_addressed: true,
}; };
try_handle_command(&dispatch, &format!("@timmy move {args}")) try_handle_command(&dispatch, &format!("@timmy move {args}"))
} }

View File

@@ -238,7 +238,6 @@ mod tests {
agents: &agents, agents: &agents,
ambient_rooms: &ambient_rooms, ambient_rooms: &ambient_rooms,
room_id: &room_id, room_id: &room_id,
is_addressed: true,
}; };
try_handle_command(&dispatch, &format!("@timmy overview {args}")) try_handle_command(&dispatch, &format!("@timmy overview {args}"))
} }

View File

@@ -87,7 +87,6 @@ mod tests {
agents: &agents, agents: &agents,
ambient_rooms: &ambient_rooms, ambient_rooms: &ambient_rooms,
room_id: &room_id, room_id: &room_id,
is_addressed: true,
}; };
try_handle_command(&dispatch, &format!("@timmy show {args}")) try_handle_command(&dispatch, &format!("@timmy show {args}"))
} }

View File

@@ -678,7 +678,6 @@ pub async fn slash_command_receive(
agents: &ctx.agents, agents: &ctx.agents,
ambient_rooms: &ctx.ambient_rooms, ambient_rooms: &ctx.ambient_rooms,
room_id: &payload.channel_id, room_id: &payload.channel_id,
is_addressed: true,
}; };
let response_text = try_handle_command(&dispatch, &synthetic_message) let response_text = try_handle_command(&dispatch, &synthetic_message)
@@ -711,8 +710,6 @@ async fn handle_incoming_message(
agents: &ctx.agents, agents: &ctx.agents,
ambient_rooms: &ctx.ambient_rooms, ambient_rooms: &ctx.ambient_rooms,
room_id: channel, room_id: channel,
// Slack messages in configured channels are always "addressed" to the bot.
is_addressed: true,
}; };
if let Some(response) = try_handle_command(&dispatch, message) { if let Some(response) = try_handle_command(&dispatch, message) {
@@ -1429,7 +1426,6 @@ mod tests {
agents: &agents, agents: &agents,
ambient_rooms: &ambient_rooms, ambient_rooms: &ambient_rooms,
room_id: &room_id, room_id: &room_id,
is_addressed: true,
}; };
let result = try_handle_command(&dispatch, &synthetic); let result = try_handle_command(&dispatch, &synthetic);
@@ -1457,7 +1453,6 @@ mod tests {
agents: &agents, agents: &agents,
ambient_rooms: &ambient_rooms, ambient_rooms: &ambient_rooms,
room_id: &room_id, room_id: &room_id,
is_addressed: true,
}; };
let result = try_handle_command(&dispatch, &synthetic); let result = try_handle_command(&dispatch, &synthetic);

View File

@@ -722,8 +722,6 @@ async fn handle_incoming_message(
agents: &ctx.agents, agents: &ctx.agents,
ambient_rooms: &ctx.ambient_rooms, ambient_rooms: &ctx.ambient_rooms,
room_id: sender, room_id: sender,
// WhatsApp messages are always "addressed" to the bot (1:1 or bot-specific).
is_addressed: true,
}; };
if let Some(response) = try_handle_command(&dispatch, message) { if let Some(response) = try_handle_command(&dispatch, message) {