feat: listen-only mode toggle + cross-platform invoke handling#288
Open
deanfluencebot wants to merge 42 commits intospacedriveapp:mainfrom
Open
feat: listen-only mode toggle + cross-platform invoke handling#288deanfluencebot wants to merge 42 commits intospacedriveapp:mainfrom
deanfluencebot wants to merge 42 commits intospacedriveapp:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds channel-level listen-only mode and UI, persists and exposes it via API/config, wires runtime/settings to honor explicit or persisted per-channel overrides, integrates listen-only enforcement into message handling and platform metadata, and updates reply tooling to include agent identity. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
deanfluencebot
force-pushed
the
codex/quiet-mode-v022
branch
from
1269121 to
eafc13d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
interface/src/routes/AgentConfig.tsx (1)
68-70: Prefer deriving valid tabs fromSECTIONSto avoid drift.Line 68 through Line 70 duplicates section ids manually. If a section is added/renamed later, URL validation can silently go stale.
Suggested refactor
- const validSections: SectionId[] = ["soul", "identity", "user", "routing", "tuning", "compaction", "cortex", "coalesce", "memory", "browser", "channel", "sandbox"]; + const validSections: SectionId[] = SECTIONS.map((section) => section.id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/AgentConfig.tsx` around lines 68 - 70, The code builds a hard-coded validSections array which can drift; replace that manual list by deriving validSections from the existing SECTIONS constant (e.g., use Object.keys(SECTIONS) or SECTIONS.map(...) depending on SECTIONS shape) and keep the SectionId typing, then keep the existing check that if (validSections.includes(search.tab as SectionId)) setActiveSection(search.tab as SectionId); so URL tab validation always reflects the canonical SECTIONS definition (refer to SECTIONS, validSections, setActiveSection, and search.tab to locate the change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agent/channel.rs`:
- Around line 1191-1212: The match arm that sets invoked_by_mention for
message.source is missing a "twitch" case, so Twitch explicit invokes are
treated as false and suppressed in quiet/listen-only mode; add a "twitch" arm to
the match that reads the appropriate metadata flag (e.g.,
message.metadata.get("twitch_mentions_or_replies_to_bot").and_then(|v|
v.as_bool()).unwrap_or(false)) or otherwise checks the Twitch-specific mention
field, mirroring the pattern used for "discord" and "slack" in the
invoked_by_mention assignment to ensure Twitch mentions are honored.
- Around line 254-255: The code currently snapshots listen_only_mode into
default_listen_only_mode when constructing Channel (Self) and later mutates only
the channel-local state (see the toggles around lines 377-389), causing drift
from the persisted runtime_config.channel_config; change behavior so mutations
update the shared/persisted config instead of only the local field: read
listen_only_mode from deps.runtime_config.channel_config when initializing and
whenever it should reflect persisted state, and when the channel toggles
listen-only (the methods that currently mutate the local flag) call the
runtime_config.channel_config update/store API to persist the new value so the
in-memory channel and persisted/API config stay synchronized across restarts.
- Around line 1171-1180: Batched turns in handle_message_batch are skipping the
built-in command paths, so slash commands bypass try_handle_builtin_ops_commands
and try_handle_builtin_digest; modify handle_message_batch to either (a) detect
per-message raw_text starting with command markers and call
self.try_handle_builtin_ops_commands(&raw_text, &message).await? and/or
self.try_handle_builtin_digest(&raw_text, &message).await? for each message
before coalescing, or (b) prevent command messages from being coalesced by
changing the coalescing logic (e.g., in coalesce_messages or wherever messages
are aggregated) to exclude messages that look like commands; ensure you
reference and invoke try_handle_builtin_ops_commands and
try_handle_builtin_digest from handle_message_batch so built-in handlers run for
batched turns.
In `@src/config.rs`:
- Line 5737: The reload_config path currently unconditionally stores
resolved.channel into self.channel_config (via ArcSwap) and can clobber a
DB-persisted listen-only setting; update reload_config so channel resolution
honors precedence env > DB > default: first check for an environment override
(existing env-based config), then check the DB-persisted channel/listen-only
value, and only if neither exist use resolved.channel, and finally call
self.channel_config.store(Arc::new(final_channel)); use the existing resolver
helpers (per-subsystem resolve() methods) or add a small resolver that queries
the DB-persisted value before falling back to resolved.channel to ensure the
stored channel respects env then DB then default precedence.
In `@src/messaging/slack.rs`:
- Around line 264-274: The current metadata flag
slack_mentions_or_replies_to_bot only checks for the literal mention via
mentioned_bot and misses replies; update the predicate that builds mentioned_bot
to also detect replies to the bot by inspecting the message event's reply
indicators (e.g., fields on msg_event.content such as parent_user_id, thread_ts,
is_reply, or any reply-to/user-in-thread field your Slack parser provides) and
set the boolean true if either the mention check or a reply-to-bot check
(matching adapter_state.bot_user_id against parent_user_id or equivalent) is
true before inserting into metadata["slack_mentions_or_replies_to_bot"].
In `@src/tools/reply.rs`:
- Around line 455-465: The code currently downgrades to OutboundResponse::Text
when source == "telegram" || source == "discord", silently discarding
args.cards, args.interactive_elements, and args.poll; instead update the logic
in reply.rs where OutboundResponse::Text / OutboundResponse::RichMessage are
chosen so that when a rich payload was requested (check args.cards,
args.interactive_elements, args.poll) and the adapter (source) cannot support
it, you return a structured error Result::Err (or the crate's ToolError type)
describing "unsupported rich payload for source" and include which fields were
requested (args.cards, args.interactive_elements, args.poll) so the LLM sees the
failure and can recover, rather than returning OutboundResponse::Text silently.
---
Nitpick comments:
In `@interface/src/routes/AgentConfig.tsx`:
- Around line 68-70: The code builds a hard-coded validSections array which can
drift; replace that manual list by deriving validSections from the existing
SECTIONS constant (e.g., use Object.keys(SECTIONS) or SECTIONS.map(...)
depending on SECTIONS shape) and keep the SectionId typing, then keep the
existing check that if (validSections.includes(search.tab as SectionId))
setActiveSection(search.tab as SectionId); so URL tab validation always reflects
the canonical SECTIONS definition (refer to SECTIONS, validSections,
setActiveSection, and search.tab to locate the change).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
interface/src/api/client.tsinterface/src/routes/AgentConfig.tsxsrc/agent/channel.rssrc/agent/channel_prompt.rssrc/api/agents.rssrc/api/config.rssrc/config.rssrc/messaging/slack.rssrc/messaging/telegram.rssrc/tools.rssrc/tools/reply.rs
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (6)
src/tools/reply.rs (1)
453-466:⚠️ Potential issue | 🟠 MajorReturn a structured error instead of silently dropping rich payloads.
Line 455 currently converts rich requests to plain text success for telegram/discord, which hides unsupported fields (
cards,interactive_elements,poll) from the LLM.As per coding guidelines `src/tools/**/*.rs`: "Tool errors must be returned as structured results, not panics; the LLM must see the error and be able to recover".🛠️ Suggested fix
- } else if args.cards.is_some() || args.interactive_elements.is_some() || args.poll.is_some() - { - if source == "telegram" || source == "discord" { - // Force plain text on adapters where rich payloads can fail silently. - OutboundResponse::Text(converted_content.clone()) - } else { - OutboundResponse::RichMessage { - text: converted_content.clone(), - blocks: vec![], // No block generation for now; Slack adapters will fall back to text - cards: args.cards.unwrap_or_default(), - interactive_elements: args.interactive_elements.unwrap_or_default(), - poll: args.poll, - } - } + } else if args.cards.is_some() || args.interactive_elements.is_some() || args.poll.is_some() { + let requested_fields = [ + args.cards + .as_ref() + .filter(|cards| !cards.is_empty()) + .map(|_| "cards"), + args.interactive_elements + .as_ref() + .filter(|elements| !elements.is_empty()) + .map(|_| "interactive_elements"), + args.poll.as_ref().map(|_| "poll"), + ] + .into_iter() + .flatten() + .collect::<Vec<_>>(); + + if (source == "telegram" || source == "discord") && !requested_fields.is_empty() { + return Err(ReplyError(format!( + "unsupported rich payload for source '{source}': {}", + requested_fields.join(", ") + ))); + } + + OutboundResponse::RichMessage { + text: converted_content.clone(), + blocks: vec![], + cards: args.cards.unwrap_or_default(), + interactive_elements: args.interactive_elements.unwrap_or_default(), + poll: args.poll, + } } else { OutboundResponse::Text(converted_content.clone()) };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/reply.rs` around lines 453 - 466, The code currently silences unsupported rich fields by returning OutboundResponse::Text when source == "telegram" or "discord" while args.cards / args.interactive_elements / args.poll are set; change this to return a structured error instead so the LLM can see and handle it: detect the same condition (source variable and args.cards.is_some() || args.interactive_elements.is_some() || args.poll.is_some()), and return a Result::Err containing a descriptive tool error (e.g., an UnsupportedRichPayload or ToolError with fields describing which of args.cards / args.interactive_elements / args.poll were provided) rather than OutboundResponse::Text; update the function signature/return type accordingly and adjust call sites to propagate/handle the Err; reference OutboundResponse, args.cards, args.interactive_elements, args.poll, and source when making the change.src/config.rs (1)
5830-5830:⚠️ Potential issue | 🟠 MajorPreserve DB-persisted listen-only state on reload.
At Line 5830,
self.channel_config.store(Arc::new(resolved.channel));unconditionally applies file-resolved config and can overwrite settings-backed listen-only state after hot reload.Use the same precedence on reload (
env > DB > default) before storingchannel_config(via an existing/new per-subsystem resolver) so persisted runtime intent is not lost.As per coding guidelines,
src/config.rs:Config resolution must follow: env > DB > default with per-subsystem resolve() methods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.rs` at line 5830, The current line self.channel_config.store(Arc::new(resolved.channel)); blindly replaces the in-memory channel config with the file-resolved config and can wipe DB-persisted listen-only state on reload; change this to run per-subsystem precedence resolution (env > DB > default) for the channel subsystem before storing: fetch the DB-stored channel entry (the persisted listen-only flag), apply environment overrides, and merge those into the resolved.channel (or call/implement a ChannelResolver.resolve(env, db, default) helper) so that listen-only from DB is preserved unless env overrides it, then store the merged Arc (use the existing/new resolver function name like ChannelResolver::resolve or a per-subsystem resolve() method and update the call site that currently uses resolved.channel).src/agent/channel.rs (4)
1171-1180:⚠️ Potential issue | 🟡 MinorBuilt-in command handling is still bypassed for coalesced batches.
The built-in handlers are only called in
handle_message; batched turns (handle_message_batch) can still let slash commands fall through to normal LLM processing when coalescing is active.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 1171 - 1180, The built-in command handlers are not invoked for coalesced batches, so update handle_message_batch to call the same built-in checks used in handle_message: invoke try_handle_builtin_ops_commands(&raw_text, &message).await? and try_handle_builtin_digest(&raw_text, &message).await? (or their async equivalents) before falling back to normal LLM processing; if either returns true, short-circuit and return Ok(()) to prevent slash commands from being processed by the LLM. Ensure you reuse the same logic/ordering as in handle_message so behavior is consistent.
1191-1212:⚠️ Potential issue | 🟠 MajorAdd Twitch explicit-invoke handling in quiet mode mention gating.
invoked_by_mentionhas no"twitch"match arm, so Twitch mention-based invokes currently evaluate to false and can be suppressed in listen-only mode.Suggested patch
invoked_by_mention = match message.source.as_str() { "telegram" => message @@ "slack" => message .metadata .get("slack_mentions_or_replies_to_bot") .and_then(|v| v.as_bool()) .unwrap_or(false), + "twitch" => message + .metadata + .get("twitch_mentions_or_replies_to_bot") + .and_then(|v| v.as_bool()) + .unwrap_or(false), _ => false, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 1191 - 1212, The match for invoked_by_mention is missing a "twitch" arm so Twitch messages never count as mentions; add a "twitch" branch in the match on message.source that reads the appropriate metadata flag (e.g., message.metadata.get("twitch_mentions_or_replies_to_bot").and_then(|v| v.as_bool()).unwrap_or(false)) or the Twitch-specific username key if present, and return that boolean so invoked_by_mention is true when Twitch explicitly invokes the bot in quiet/listen-only gating logic.
254-255:⚠️ Potential issue | 🟠 MajorPersist
/quietand/activechanges instead of mutating only channel-local state.
listen_only_modeis initialized from runtime config once (Line 254) but toggles at Lines 378 and 388 only update the local field. This can drift from persisted/API config and reset unexpectedly after restart or reload.Also applies to: 377-389
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 254 - 255, The channel's listen_only_mode is only read once into default_listen_only_mode and later toggled by updating the Channel struct field, which drifts from the persisted/runtime config; change the toggle handlers (the methods that flip listen_only_mode for the Channel struct — where `/quiet` and `/active` are handled) to atomically update the persistent runtime config entry (the same store used to initialize default_listen_only_mode, e.g., deps.runtime_config.channel_config) and then set the Channel.listen_only_mode field from the persisted result; use the config store's update/modify API (or write-and-load) and handle/log any errors so the change survives restarts/reloads.
396-458:⚠️ Potential issue | 🟠 MajorMove
/tasks,/today, and/digestbehind channel tools (and remove direct in-channel LLM call).These paths directly query task/conversation data, and
/digestspins a separateAgentBuilderflow, bypassing the channel tool/process contract.As per coding guidelines
src/agent/channel.rs: “Channel processes must use tools: reply, branch, spawn_worker, route, cancel, skip, react; they must NOT execute tasks directly or search memories themselves” andsrc/agent/**/*.rs: “Every LLM process must be a RigAgent<SpacebotModel, SpacebotHook>with system prompt, tools, history, and hooks”.Also applies to: 524-557
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 396 - 458, The handlers for "/tasks", "/today", and "/digest" are violating the channel-tool contract by calling deps.task_store.list/list_ready and spinning an AgentBuilder/LLM directly (see symbols "/tasks", "/today", "/digest", deps.task_store.list_ready, deps.task_store.list, AgentBuilder, send_builtin_text); change these handlers to not query task_store or create Agents inline but instead call channel tools (reply/route/spawn_worker) that you register to perform the actual task/conversation queries and formatting: create worker/tool endpoints (e.g., "tasks.summary", "tasks.today", "tasks.digest") that encapsulate the task_store calls and return the formatted body, then replace the inline logic with a tool invocation (spawn_worker or route) and send the returned text via send_builtin_text (or let the tool call reply) so all data access and LLM usage happens inside proper Agent tool processes.
🧹 Nitpick comments (3)
interface/src/routes/AgentConfig.tsx (2)
68-70: Derive valid tabs fromSECTIONSinstead of hardcoding them.Line 68 duplicates section IDs and can drift over time. Prefer deriving valid tabs from
SECTIONSso URL validation stays in sync automatically.♻️ Suggested change
- const validSections: SectionId[] = ["soul", "identity", "user", "routing", "tuning", "compaction", "cortex", "coalesce", "memory", "browser", "channel", "sandbox"]; - if (validSections.includes(search.tab as SectionId)) { + const validSections = new Set<SectionId>(SECTIONS.map((section) => section.id)); + if (validSections.has(search.tab as SectionId)) { setActiveSection(search.tab as SectionId); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/AgentConfig.tsx` around lines 68 - 70, Replace the hardcoded validSections array with a derived list from the existing SECTIONS constant so URL tab validation stays in sync; specifically, compute validSections (or rename) by mapping SECTIONS (the exported/defined constant) to extract each SectionId, then use that derived array in the includes check before calling setActiveSection(search.tab as SectionId) — update the logic around validSections, SECTIONS, setActiveSection, and search.tab to ensure types align and no hardcoded strings remain.
421-442: Consider centralizing section→values mapping to avoid switch drift.The same mapping logic is repeated in initializer/reset/revert. A small helper would reduce maintenance risk when adding future sections.
Also applies to: 452-483, 497-527
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/AgentConfig.tsx` around lines 421 - 442, The switch-based mapping from sectionId to its ConfigValues (seen in the switch block returning {...config.routing}, {...config.tuning}, etc. and duplicated in other ranges) should be centralized: add a helper function (e.g., getSectionValues(sectionId: string, config: AgentConfig, channel: ChannelType, sandbox: SandboxType): ConfigValues) that contains the single switch or a lookup map for keys ("routing","tuning","compaction","cortex","coalesce","memory","browser","channel","sandbox") and returns the appropriate object (including the sandbox special-case), then replace the duplicated switch blocks in the initializer/reset/revert sites with calls to this helper (reference the existing symbols config, channel, sandbox and the switch logic currently in AgentConfig.tsx).src/api/config.rs (1)
698-709: Add focused tests forupdate_channel_tableand round-trip config behavior.The new channel subtable write path is untested in this file, unlike existing warmup helpers; adding parity tests here would make this safer to evolve.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/config.rs` around lines 698 - 709, Add unit tests that exercise update_channel_table and ensure round-trip TOML behavior: create a mutable toml_edit::DocumentMut with an agent table (use get_agent_table_mut/get_or_create_subtable semantics), call update_channel_table with a ChannelUpdate that sets listen_only_mode, assert the channel subtable contains the expected listen_only_mode value, and then serialize/parse back (or apply reverse reader) to confirm the value round-trips. Add at least two tests: one for setting listen_only_mode=true and one for leaving it None to ensure no key is written; reference update_channel_table, get_agent_table_mut, and get_or_create_subtable to locate the code under test. Ensure tests live alongside other config tests in this file’s test module and use the same helper patterns as existing warmup tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/agent/channel.rs`:
- Around line 1171-1180: The built-in command handlers are not invoked for
coalesced batches, so update handle_message_batch to call the same built-in
checks used in handle_message: invoke try_handle_builtin_ops_commands(&raw_text,
&message).await? and try_handle_builtin_digest(&raw_text, &message).await? (or
their async equivalents) before falling back to normal LLM processing; if either
returns true, short-circuit and return Ok(()) to prevent slash commands from
being processed by the LLM. Ensure you reuse the same logic/ordering as in
handle_message so behavior is consistent.
- Around line 1191-1212: The match for invoked_by_mention is missing a "twitch"
arm so Twitch messages never count as mentions; add a "twitch" branch in the
match on message.source that reads the appropriate metadata flag (e.g.,
message.metadata.get("twitch_mentions_or_replies_to_bot").and_then(|v|
v.as_bool()).unwrap_or(false)) or the Twitch-specific username key if present,
and return that boolean so invoked_by_mention is true when Twitch explicitly
invokes the bot in quiet/listen-only gating logic.
- Around line 254-255: The channel's listen_only_mode is only read once into
default_listen_only_mode and later toggled by updating the Channel struct field,
which drifts from the persisted/runtime config; change the toggle handlers (the
methods that flip listen_only_mode for the Channel struct — where `/quiet` and
`/active` are handled) to atomically update the persistent runtime config entry
(the same store used to initialize default_listen_only_mode, e.g.,
deps.runtime_config.channel_config) and then set the Channel.listen_only_mode
field from the persisted result; use the config store's update/modify API (or
write-and-load) and handle/log any errors so the change survives
restarts/reloads.
- Around line 396-458: The handlers for "/tasks", "/today", and "/digest" are
violating the channel-tool contract by calling deps.task_store.list/list_ready
and spinning an AgentBuilder/LLM directly (see symbols "/tasks", "/today",
"/digest", deps.task_store.list_ready, deps.task_store.list, AgentBuilder,
send_builtin_text); change these handlers to not query task_store or create
Agents inline but instead call channel tools (reply/route/spawn_worker) that you
register to perform the actual task/conversation queries and formatting: create
worker/tool endpoints (e.g., "tasks.summary", "tasks.today", "tasks.digest")
that encapsulate the task_store calls and return the formatted body, then
replace the inline logic with a tool invocation (spawn_worker or route) and send
the returned text via send_builtin_text (or let the tool call reply) so all data
access and LLM usage happens inside proper Agent tool processes.
In `@src/config.rs`:
- Line 5830: The current line
self.channel_config.store(Arc::new(resolved.channel)); blindly replaces the
in-memory channel config with the file-resolved config and can wipe DB-persisted
listen-only state on reload; change this to run per-subsystem precedence
resolution (env > DB > default) for the channel subsystem before storing: fetch
the DB-stored channel entry (the persisted listen-only flag), apply environment
overrides, and merge those into the resolved.channel (or call/implement a
ChannelResolver.resolve(env, db, default) helper) so that listen-only from DB is
preserved unless env overrides it, then store the merged Arc (use the
existing/new resolver function name like ChannelResolver::resolve or a
per-subsystem resolve() method and update the call site that currently uses
resolved.channel).
In `@src/tools/reply.rs`:
- Around line 453-466: The code currently silences unsupported rich fields by
returning OutboundResponse::Text when source == "telegram" or "discord" while
args.cards / args.interactive_elements / args.poll are set; change this to
return a structured error instead so the LLM can see and handle it: detect the
same condition (source variable and args.cards.is_some() ||
args.interactive_elements.is_some() || args.poll.is_some()), and return a
Result::Err containing a descriptive tool error (e.g., an UnsupportedRichPayload
or ToolError with fields describing which of args.cards /
args.interactive_elements / args.poll were provided) rather than
OutboundResponse::Text; update the function signature/return type accordingly
and adjust call sites to propagate/handle the Err; reference OutboundResponse,
args.cards, args.interactive_elements, args.poll, and source when making the
change.
---
Nitpick comments:
In `@interface/src/routes/AgentConfig.tsx`:
- Around line 68-70: Replace the hardcoded validSections array with a derived
list from the existing SECTIONS constant so URL tab validation stays in sync;
specifically, compute validSections (or rename) by mapping SECTIONS (the
exported/defined constant) to extract each SectionId, then use that derived
array in the includes check before calling setActiveSection(search.tab as
SectionId) — update the logic around validSections, SECTIONS, setActiveSection,
and search.tab to ensure types align and no hardcoded strings remain.
- Around line 421-442: The switch-based mapping from sectionId to its
ConfigValues (seen in the switch block returning {...config.routing},
{...config.tuning}, etc. and duplicated in other ranges) should be centralized:
add a helper function (e.g., getSectionValues(sectionId: string, config:
AgentConfig, channel: ChannelType, sandbox: SandboxType): ConfigValues) that
contains the single switch or a lookup map for keys
("routing","tuning","compaction","cortex","coalesce","memory","browser","channel","sandbox")
and returns the appropriate object (including the sandbox special-case), then
replace the duplicated switch blocks in the initializer/reset/revert sites with
calls to this helper (reference the existing symbols config, channel, sandbox
and the switch logic currently in AgentConfig.tsx).
In `@src/api/config.rs`:
- Around line 698-709: Add unit tests that exercise update_channel_table and
ensure round-trip TOML behavior: create a mutable toml_edit::DocumentMut with an
agent table (use get_agent_table_mut/get_or_create_subtable semantics), call
update_channel_table with a ChannelUpdate that sets listen_only_mode, assert the
channel subtable contains the expected listen_only_mode value, and then
serialize/parse back (or apply reverse reader) to confirm the value round-trips.
Add at least two tests: one for setting listen_only_mode=true and one for
leaving it None to ensure no key is written; reference update_channel_table,
get_agent_table_mut, and get_or_create_subtable to locate the code under test.
Ensure tests live alongside other config tests in this file’s test module and
use the same helper patterns as existing warmup tests.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
interface/src/api/client.tsinterface/src/routes/AgentConfig.tsxsrc/agent/channel.rssrc/agent/channel_prompt.rssrc/api/agents.rssrc/api/config.rssrc/config.rssrc/messaging/slack.rssrc/messaging/telegram.rssrc/tools.rssrc/tools/reply.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- src/tools.rs
- src/api/agents.rs
- src/messaging/telegram.rs
- interface/src/api/client.ts
- src/messaging/slack.rs
- src/agent/channel_prompt.rs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (6)
src/agent/channel.rs (6)
1062-1069:⚠️ Potential issue | 🟡 MinorNormalize Telegram mention matching consistently (case-insensitive).
The text is normalized in one path, but mention tokens are not consistently normalized, which can miss valid invokes for mixed-case bot usernames.
Case-normalization fix
- let mention = message + let mention = message .metadata .get("telegram_bot_username") .and_then(|v| v.as_str()) - .map(|u| format!("@{u}")) + .map(|u| format!("@{u}").to_lowercase()) .unwrap_or_default(); let has_mention = !mention.is_empty() && text.contains(&mention);- let text = raw_text.trim(); + let text = raw_text.trim().to_lowercase(); @@ - let mention = format!("@{username}"); + let mention = format!("@{username}").to_lowercase(); text.contains(&mention)Also applies to: 1204-1212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 1062 - 1069, The mention matching uses a lowercased text variable but leaves the mention token unhashed, causing case-sensitive misses; update the logic that constructs mention (from message.metadata "telegram_bot_username") to normalize it the same way as raw_text (trim and to_lowercase and prefix with '@' as needed) before checking contains, and make the same change in the other occurrence that builds has_mention (e.g., the blocks creating mention/has_mention near the usages of raw_text, text, mention, and has_mention) so the contains check is consistently case-insensitive.
1203-1224:⚠️ Potential issue | 🟠 MajorAdd Twitch mention detection to quiet-mode invoke matching.
Twitch currently falls into the default
_ => falsebranch for mention-based invoke detection, so explicit Twitch mentions can be suppressed in listen-only mode.Minimal fix
invoked_by_mention = match message.source.as_str() { @@ "slack" => message .metadata .get("slack_mentions_or_replies_to_bot") .and_then(|v| v.as_bool()) .unwrap_or(false), + "twitch" => message + .metadata + .get("twitch_mentions_or_replies_to_bot") + .and_then(|v| v.as_bool()) + .unwrap_or(false), _ => false, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 1203 - 1224, The match for invoked_by_mention lacks a "twitch" arm so Twitch messages always fall through to false; add a "twitch" branch in the same match block that reads message.metadata.get("twitch_mentions_or_replies_to_bot").and_then(|v| v.as_bool()).unwrap_or(false) (mirroring the "discord"/"slack" arms) so Twitch mention/reply flags suppress invokes in listen-only/quiet mode; update the match handling around invoked_by_mention in the same function to include this new arm.
664-679:⚠️ Potential issue | 🟠 MajorBatched message flow bypasses command handling and quiet-mode invoke gating.
When coalescing produces a batch,
handle_message_batchskips the built-in command path and listen-only suppression logic used inhandle_message, so behavior diverges under load.Possible guard to prevent command/quiet-mode bypass via coalescing
fn should_coalesce( &self, message: &InboundMessage, config: &crate::config::CoalesceConfig, ) -> bool { @@ if message.source == "system" { return false; } + if let crate::MessageContent::Text(text) = &message.content + && text.trim_start().starts_with('/') + { + return false; + } + if self.listen_only_mode { + return false; + } if config.multi_user_only && self.is_dm() { return false; } true }Also applies to: 761-942, 1183-1192, 1198-1244
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 664 - 679, The coalescing path allows batched messages to bypass the command handling and quiet-mode gating present in handle_message; update should_coalesce and the batch entry points (e.g., handle_message_batch) to refuse coalescing or to short-circuit batch processing when a message would trigger built-in commands or be suppressed by quiet/listen-only logic: detect command invocations (same checks used by handle_message) and the quiet-mode/listen-only conditions before returning true from should_coalesce (or at the start of handle_message_batch), and if any of those checks match, skip coalescing and fall back to the single-message handle_message flow so command parsing and suppression still run.
307-314:⚠️ Potential issue | 🟠 MajorPersist
/quietand/activeto runtime config, not only local state.
/quietand/activecurrently mutate onlyself.listen_only_mode, but subsequent runtime sync calls can overwrite that local value, so the mode flip can silently revert.Also applies to: 386-403, 1014-1015, 1890-1891
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 307 - 314, The handlers that toggle listen-only (the `/quiet` and `/active` code paths which currently only set self.listen_only_mode) must persist the change to the shared runtime config so it isn’t overwritten by later syncs; update those handlers to read the current deps.runtime_config.channel_config.load(), modify the listen_only_mode field, and write the updated config back via the channel_config store/update API (matching your runtime_config type/API) in addition to setting self.listen_only_mode. Also update any places that mirror runtime→local (e.g., sync_listen_only_mode_from_runtime) to remain read-only and rely on the persisted runtime_config for truth; reference functions/fields: sync_listen_only_mode_from_runtime, self.listen_only_mode, deps.runtime_config.channel_config.load()/store() (or the equivalent update method) and apply the same persist-on-toggle fix to the other affected handlers mentioned.
1225-1229:⚠️ Potential issue | 🟡 MinorTighten Telegram reply invoke detection to this bot, not any bot.
Using only
reply_to_is_botcan incorrectly treat replies to other bots as explicit invokes.Targeted Telegram check
- invoked_by_reply = message - .metadata - .get("reply_to_is_bot") - .and_then(|v| v.as_bool()) - .unwrap_or(false); + invoked_by_reply = match message.source.as_str() { + "telegram" => { + let reply_to_is_bot = message + .metadata + .get("reply_to_is_bot") + .and_then(|v| v.as_bool()) + .unwrap_or(false); + let reply_to_matches_this_bot = match ( + message.metadata.get("reply_to_username").and_then(|v| v.as_str()), + message.metadata.get("telegram_bot_username").and_then(|v| v.as_str()), + ) { + (Some(reply_to), Some(bot_username)) => { + reply_to.eq_ignore_ascii_case(bot_username) + } + _ => true, + }; + reply_to_is_bot && reply_to_matches_this_bot + } + _ => message + .metadata + .get("reply_to_is_bot") + .and_then(|v| v.as_bool()) + .unwrap_or(false), + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 1225 - 1229, The current invoked_by_reply logic uses only message.metadata.get("reply_to_is_bot") which treats replies to any bot as an invoke; change it to require the replied-to user ID match this bot's ID. Update the invoked_by_reply assignment (the code referencing message.metadata.get("reply_to_is_bot")) to first confirm reply_to_is_bot is true and then compare the metadata's reply_to_user_id (or reply_to_from_id) to this instance's bot_user_id (e.g., self.bot_user_id or a similar field in the surrounding struct/context); only set invoked_by_reply = true when the IDs match, otherwise false.
405-466:⚠️ Potential issue | 🟠 MajorBuilt-in
/tasks,/today, and/digestcurrently bypass the channel tool/process contract.These paths directly read task/conversation data in channel code and
/digestbuilds a separate local LLM call path instead of going through the channel tool flow.As per coding guidelines
src/agent/channel.rs: “Channel processes must use tools: reply, branch, spawn_worker, route, cancel, skip, react; they must NOT execute tasks directly or search memories themselves”, andsrc/agent/**/*.rs: “Every LLM process must be a RigAgent<SpacebotModel, SpacebotHook>with system prompt, tools, history, and hooks.”Also applies to: 503-565, 538-553
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 405 - 466, Handlers for "/tasks", "/today" (and "/digest") are directly accessing deps.task_store and performing local LLM work (via send_builtin_text and inlined logic using deps.task_store.list_ready, deps.task_store.list, TaskStatus::InProgress), which violates the channel/tool and Agent requirements. Replace the direct reads and local formatting/LLM call by invoking the channel tool flow: instead of calling deps.task_store.list_ready/list or send_builtin_text, route or spawn the appropriate channel tool (use the channel methods like route or spawn_worker and tools such as reply/branch) that will perform the task-store access and return the formatted result; for "/digest" create a Rig Agent<SpacebotModel, SpacebotHook> process (with system prompt, tools, history, hooks) to run the LLM step rather than executing it inline. Ensure the handlers only call the tool/process (route/spawn_worker) and then forward the tool response (via reply/react) rather than touching task_store or calling the LLM directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/agent/channel.rs`:
- Around line 1062-1069: The mention matching uses a lowercased text variable
but leaves the mention token unhashed, causing case-sensitive misses; update the
logic that constructs mention (from message.metadata "telegram_bot_username") to
normalize it the same way as raw_text (trim and to_lowercase and prefix with '@'
as needed) before checking contains, and make the same change in the other
occurrence that builds has_mention (e.g., the blocks creating
mention/has_mention near the usages of raw_text, text, mention, and has_mention)
so the contains check is consistently case-insensitive.
- Around line 1203-1224: The match for invoked_by_mention lacks a "twitch" arm
so Twitch messages always fall through to false; add a "twitch" branch in the
same match block that reads
message.metadata.get("twitch_mentions_or_replies_to_bot").and_then(|v|
v.as_bool()).unwrap_or(false) (mirroring the "discord"/"slack" arms) so Twitch
mention/reply flags suppress invokes in listen-only/quiet mode; update the match
handling around invoked_by_mention in the same function to include this new arm.
- Around line 664-679: The coalescing path allows batched messages to bypass the
command handling and quiet-mode gating present in handle_message; update
should_coalesce and the batch entry points (e.g., handle_message_batch) to
refuse coalescing or to short-circuit batch processing when a message would
trigger built-in commands or be suppressed by quiet/listen-only logic: detect
command invocations (same checks used by handle_message) and the
quiet-mode/listen-only conditions before returning true from should_coalesce (or
at the start of handle_message_batch), and if any of those checks match, skip
coalescing and fall back to the single-message handle_message flow so command
parsing and suppression still run.
- Around line 307-314: The handlers that toggle listen-only (the `/quiet` and
`/active` code paths which currently only set self.listen_only_mode) must
persist the change to the shared runtime config so it isn’t overwritten by later
syncs; update those handlers to read the current
deps.runtime_config.channel_config.load(), modify the listen_only_mode field,
and write the updated config back via the channel_config store/update API
(matching your runtime_config type/API) in addition to setting
self.listen_only_mode. Also update any places that mirror runtime→local (e.g.,
sync_listen_only_mode_from_runtime) to remain read-only and rely on the
persisted runtime_config for truth; reference functions/fields:
sync_listen_only_mode_from_runtime, self.listen_only_mode,
deps.runtime_config.channel_config.load()/store() (or the equivalent update
method) and apply the same persist-on-toggle fix to the other affected handlers
mentioned.
- Around line 1225-1229: The current invoked_by_reply logic uses only
message.metadata.get("reply_to_is_bot") which treats replies to any bot as an
invoke; change it to require the replied-to user ID match this bot's ID. Update
the invoked_by_reply assignment (the code referencing
message.metadata.get("reply_to_is_bot")) to first confirm reply_to_is_bot is
true and then compare the metadata's reply_to_user_id (or reply_to_from_id) to
this instance's bot_user_id (e.g., self.bot_user_id or a similar field in the
surrounding struct/context); only set invoked_by_reply = true when the IDs
match, otherwise false.
- Around line 405-466: Handlers for "/tasks", "/today" (and "/digest") are
directly accessing deps.task_store and performing local LLM work (via
send_builtin_text and inlined logic using deps.task_store.list_ready,
deps.task_store.list, TaskStatus::InProgress), which violates the channel/tool
and Agent requirements. Replace the direct reads and local formatting/LLM call
by invoking the channel tool flow: instead of calling
deps.task_store.list_ready/list or send_builtin_text, route or spawn the
appropriate channel tool (use the channel methods like route or spawn_worker and
tools such as reply/branch) that will perform the task-store access and return
the formatted result; for "/digest" create a Rig Agent<SpacebotModel,
SpacebotHook> process (with system prompt, tools, history, hooks) to run the LLM
step rather than executing it inline. Ensure the handlers only call the
tool/process (route/spawn_worker) and then forward the tool response (via
reply/react) rather than touching task_store or calling the LLM directly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
src/agent/channel.rs (6)
1225-1238:⚠️ Potential issue | 🟡 MinorTelegram reply invoke can still trigger on replies to other bots.
Using generic
reply_to_is_botfor Telegram may incorrectly treat replies to unrelated bots as explicit invokes; prefer matchingreply_to_usernameagainst the bot username when available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 1225 - 1238, The Telegram arm for computing invoked_by_reply should not rely solely on the generic "reply_to_is_bot" metadata; update the "telegram" match branch (where invoked_by_reply is set) to first check message.metadata.get("reply_to_username") and compare it case-insensitively to the running bot username (e.g., self.bot_username or the appropriate field holding the bot's username), returning true only on a match, and only fall back to the boolean "reply_to_is_bot" if "reply_to_username" is missing; keep the existing Discord branch unchanged.
1062-1069:⚠️ Potential issue | 🟡 MinorTelegram mention detection is case-mismatched.
textis lowercased, butmentionis not, so mixed-case bot usernames can fail mention detection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 1062 - 1069, The mention detection lowercases raw_text into variable text but leaves mention (built from message.metadata "telegram_bot_username") in original case, causing missed matches for mixed-case usernames; update the mention construction so the username is lowercased before formatting (i.e., lowercase the metadata string used to build mention) or perform a case-insensitive comparison between text and mention; adjust the code around raw_text/text, mention, and has_mention to use the lowercased mention (or a case-insensitive check) so text.contains(mention) reliably detects mentions.
1203-1224:⚠️ Potential issue | 🟠 MajorTwitch explicit mention invoke is still missing in listen-only mode.
The mention-source match does not include a
"twitch"arm, so Twitch explicit invokes can be suppressed as unsolicited.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 1203 - 1224, The match that sets invoked_by_mention omits a "twitch" arm so Twitch explicit mentions get treated as unsolicited; add a "twitch" branch to the match (alongside "telegram", "discord", "slack") that checks the appropriate metadata boolean (e.g., message.metadata.get("twitch_mentioned_bot").and_then(|v| v.as_bool()).unwrap_or(false)) and returns that value so explicit Twitch mentions are honored; update the invoked_by_mention match in channel.rs accordingly.
1183-1192:⚠️ Potential issue | 🟠 MajorBuilt-in command handling is still bypassed for coalesced batches.
Built-ins are invoked in
handle_message, but batched turns can skip this path and fall through to normal LLM processing, which breaks deterministic command behavior under coalescing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 1183 - 1192, Built-in commands are being skipped for coalesced batches; ensure the same checks performed in handle_message are run for batched turns by invoking try_handle_builtin_ops_commands and try_handle_builtin_digest in the coalesced-batch processing path (or factor them into a shared helper that both handle_message and the coalesced batch handler call). Locate the coalescing handler (the function that processes batched/coalesced turns) and call try_handle_builtin_ops_commands(&raw_text, &message).await? and try_handle_builtin_digest(&raw_text, &message).await? before falling through to normal LLM processing so built-ins remain deterministic under coalescing.
386-403:⚠️ Potential issue | 🟠 Major
/quietand/activecurrently revert on the next turn.These commands only mutate
self.listen_only_mode, but each message/event re-syncs from runtime config, so the toggle is effectively ephemeral unless persisted to runtime config as the source of truth.Also applies to: 1014-1016
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 386 - 403, The /quiet and /active handlers only flip self.listen_only_mode in memory so their change is lost when the runtime config is reloaded; update the runtime-config source of truth when toggling instead of only mutating the field. In the command handling block (where self.listen_only_mode is set and send_builtin_text is called) set the equivalent field on the runtime config object and call the runtime-config persist/save method (e.g., set runtime_config.listen_only_mode = true/false and invoke its save/persist function) so the change survives restarts/re-syncs; apply the same fix to the other occurrence around the referenced lines (1014–1016).
405-565:⚠️ Potential issue | 🟠 MajorBuilt-in
/tasks,/today, and/digestbypass the required channel tool/agent contract.This path directly queries task/conversation data and runs a separate LLM call inside
Channel, instead of going through the channel tool flow / branch-worker flow and standard channel agent lifecycle.
As per coding guidelines,src/agent/channel.rs: "Channel processes must use tools: reply, branch, spawn_worker, route, cancel, skip, react; they must NOT execute tasks directly or search memories themselves" andsrc/agent/**/*.rs: "Every LLM process must be a Rig Agent<SpacebotModel, SpacebotHook> with system prompt, tools, history, and hooks".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/messaging/discord.rs`:
- Around line 952-959: build_metadata now inserts split flags
discord_mentioned_bot and discord_reply_to_bot, but interaction event paths
still only set the older combined flag, causing channel listen-only checks to
misclassify component interactions; update the interaction handling code that
constructs event metadata (the place creating interaction events/metadata) to
set both discord_mentioned_bot and discord_reply_to_bot consistently — derive
their values from the existing combined/old flag or from the interaction payload
(whichever is authoritative) so component interactions populate the same split
keys as build_metadata. Ensure the same symbol names discord_mentioned_bot and
discord_reply_to_bot are used so downstream checks read the expected keys.
---
Duplicate comments:
In `@src/agent/channel.rs`:
- Around line 1225-1238: The Telegram arm for computing invoked_by_reply should
not rely solely on the generic "reply_to_is_bot" metadata; update the "telegram"
match branch (where invoked_by_reply is set) to first check
message.metadata.get("reply_to_username") and compare it case-insensitively to
the running bot username (e.g., self.bot_username or the appropriate field
holding the bot's username), returning true only on a match, and only fall back
to the boolean "reply_to_is_bot" if "reply_to_username" is missing; keep the
existing Discord branch unchanged.
- Around line 1062-1069: The mention detection lowercases raw_text into variable
text but leaves mention (built from message.metadata "telegram_bot_username") in
original case, causing missed matches for mixed-case usernames; update the
mention construction so the username is lowercased before formatting (i.e.,
lowercase the metadata string used to build mention) or perform a
case-insensitive comparison between text and mention; adjust the code around
raw_text/text, mention, and has_mention to use the lowercased mention (or a
case-insensitive check) so text.contains(mention) reliably detects mentions.
- Around line 1203-1224: The match that sets invoked_by_mention omits a "twitch"
arm so Twitch explicit mentions get treated as unsolicited; add a "twitch"
branch to the match (alongside "telegram", "discord", "slack") that checks the
appropriate metadata boolean (e.g.,
message.metadata.get("twitch_mentioned_bot").and_then(|v|
v.as_bool()).unwrap_or(false)) and returns that value so explicit Twitch
mentions are honored; update the invoked_by_mention match in channel.rs
accordingly.
- Around line 1183-1192: Built-in commands are being skipped for coalesced
batches; ensure the same checks performed in handle_message are run for batched
turns by invoking try_handle_builtin_ops_commands and try_handle_builtin_digest
in the coalesced-batch processing path (or factor them into a shared helper that
both handle_message and the coalesced batch handler call). Locate the coalescing
handler (the function that processes batched/coalesced turns) and call
try_handle_builtin_ops_commands(&raw_text, &message).await? and
try_handle_builtin_digest(&raw_text, &message).await? before falling through to
normal LLM processing so built-ins remain deterministic under coalescing.
- Around line 386-403: The /quiet and /active handlers only flip
self.listen_only_mode in memory so their change is lost when the runtime config
is reloaded; update the runtime-config source of truth when toggling instead of
only mutating the field. In the command handling block (where
self.listen_only_mode is set and send_builtin_text is called) set the equivalent
field on the runtime config object and call the runtime-config persist/save
method (e.g., set runtime_config.listen_only_mode = true/false and invoke its
save/persist function) so the change survives restarts/re-syncs; apply the same
fix to the other occurrence around the referenced lines (1014–1016).
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (6)
src/agent/channel.rs (6)
585-600:⚠️ Potential issue | 🟠 Major
/digestuses an ad-hoc LLM call path outside the required channel agent contract.This
AgentBuilderpath does not follow the normal channel process contract (tools/hooks/history flow), which can diverge behavior and observability fromrun_agent_turn.As per coding guidelines
src/agent/**/*.rs: "Every LLM process must be a RigAgent<SpacebotModel, SpacebotHook>with system prompt, tools, history, and hooks" and "History must be external, passed on each call viaagent.prompt().with_history(&mut history), not stored in the agent itself".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 585 - 600, The /digest handler constructs an ad-hoc Agent via AgentBuilder and calls agent.prompt(&prompt).await, bypassing the standard channel agent contract; instead refactor to use the canonical Rig agent flow by invoking run_agent_turn (or the same Agent<SpacebotModel, SpacebotHook> pipeline) so system prompt, tools, hooks and external history are applied; create or reuse a Spacebot Agent instance and call agent.prompt(...).with_history(&mut history) (or delegate to run_agent_turn which does this) passing the transcript as input while ensuring history is external and not baked into AgentBuilder-created state.
1133-1140:⚠️ Potential issue | 🟡 MinorNormalize Telegram mention matching to a common case.
textis lowercased, butmentionis not. If the bot username contains uppercase characters, mention detection can miss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 1133 - 1140, The mention detection lowercases raw_text into text but constructs mention without normalizing case, so has_mention can miss usernames with uppercase letters; update the mention creation (the expression using message.metadata.get("telegram_bot_username") ... .map(|u| format!("@{u}")) ) to normalize to a common case (e.g., .map(|u| format!("@{u}").to_lowercase()) or call u.to_lowercase() before formatting) and then use text.contains(&mention) as before so text, mention, and has_mention all use the same case.
452-514:⚠️ Potential issue | 🟠 Major
/tasksand/todayare still executing task queries directly in channel code.These command paths bypass the channel tool flow contract.
As per coding guidelines
src/agent/channel.rs: "Channel processes must use tools: reply, branch, spawn_worker, route, cancel, skip, react; they must NOT execute tasks directly or search memories themselves".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 452 - 514, The handlers for the "/tasks" and "/today" command branches in channel.rs are directly calling deps.task_store.{list_ready, list} (and then calling send_builtin_text), which violates the channel tool contract; instead, change these branches to invoke the appropriate channel tool flow (e.g., use the channel's reply/route/spawn_worker tool API) to request task queries and rendering from the task/tool layer rather than querying task_store directly. Locate the "/tasks" and "/today" match arms and replace the direct deps.task_store calls and send_builtin_text usage with calls that dispatch a tool action (referencing the channel tool names used in this repo such as reply/route/spawn_worker) that will perform list_ready/list and return a formatted body to be sent by the channel; keep the same resulting body/format but move all task_store access into the tool implementation. Ensure no direct deps.task_store.* calls remain in channel.rs.
711-726:⚠️ Potential issue | 🟠 MajorSlash commands can still bypass built-ins in coalesced batches.
should_coalesceallows command messages, andhandle_message_batchdoes not invoketry_handle_builtin_ops_commands/try_handle_builtin_digest. Batched commands can fall through to regular LLM handling.Possible guard
fn should_coalesce( &self, message: &InboundMessage, config: &crate::config::CoalesceConfig, ) -> bool { @@ if message.source == "system" { return false; } + if let crate::MessageContent::Text(text) = &message.content + && text.trim_start().starts_with('/') + { + return false; + } if config.multi_user_only && self.is_dm() { return false; } true }Also applies to: 808-963
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 711 - 726, should_coalesce currently allows slash/command messages to be batched which bypasses builtin handlers; update should_coalesce to return false when the inbound message represents a command (e.g. check message.kind == Command or message.content.starts_with('/') or message.is_command()) so command messages are never coalesced, and/or ensure handle_message_batch calls try_handle_builtin_ops_commands and try_handle_builtin_digest for each message in the batch before falling through to LLM handling so builtins run for batched inputs (refer to should_coalesce, handle_message_batch, try_handle_builtin_ops_commands, try_handle_builtin_digest).
327-348:⚠️ Potential issue | 🟠 MajorAdd a Twitch arm in mention-based invoke detection.
compute_listen_mode_invocationstill has no"twitch"case forinvoked_by_mention, so explicit Twitch invokes can be suppressed in listen-only mode.Suggested patch
let invoked_by_mention = match message.source.as_str() { @@ "slack" => message .metadata .get("slack_mentions_or_replies_to_bot") .and_then(|v| v.as_bool()) .unwrap_or(false), + "twitch" => message + .metadata + .get("twitch_mentions_or_replies_to_bot") + .and_then(|v| v.as_bool()) + .unwrap_or(false), _ => false, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 327 - 348, compute_listen_mode_invocation's invoked_by_mention match in src/agent/channel.rs is missing a "twitch" arm; add a "twitch" branch similar to the "telegram" case that looks up the bot username in message.metadata (e.g., "twitch_bot_username"), converts it to a string, formats a mention like format!("@{username}") and checks if text.contains(&mention), with a fallback to false via unwrap_or(false), so explicit Twitch mentions are detected and not suppressed in listen-only mode.
433-445:⚠️ Potential issue | 🟠 MajorPersist
/quietand/activeto runtime config, not just local state.
self.listen_only_modeis toggled locally here, but Line 307-314 re-syncs from runtime config on each entry path, so the toggle is effectively reset on subsequent turns/events.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 433 - 445, The handlers for the "/quiet" and "/active" commands currently only flip self.listen_only_mode locally, but because the runtime config is re-synced on each entry path the change is lost; update the runtime configuration store when toggling these commands (e.g. call the runtime config setter/update method to persist listen_only_mode = true/false, await the persist operation, and handle/log errors), and keep self.listen_only_mode in sync with the persisted value after a successful save; apply this to both the "/quiet" and "/active" match arms and ensure send_builtin_text behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/agent/channel.rs`:
- Around line 585-600: The /digest handler constructs an ad-hoc Agent via
AgentBuilder and calls agent.prompt(&prompt).await, bypassing the standard
channel agent contract; instead refactor to use the canonical Rig agent flow by
invoking run_agent_turn (or the same Agent<SpacebotModel, SpacebotHook>
pipeline) so system prompt, tools, hooks and external history are applied;
create or reuse a Spacebot Agent instance and call
agent.prompt(...).with_history(&mut history) (or delegate to run_agent_turn
which does this) passing the transcript as input while ensuring history is
external and not baked into AgentBuilder-created state.
- Around line 1133-1140: The mention detection lowercases raw_text into text but
constructs mention without normalizing case, so has_mention can miss usernames
with uppercase letters; update the mention creation (the expression using
message.metadata.get("telegram_bot_username") ... .map(|u| format!("@{u}")) ) to
normalize to a common case (e.g., .map(|u| format!("@{u}").to_lowercase()) or
call u.to_lowercase() before formatting) and then use text.contains(&mention) as
before so text, mention, and has_mention all use the same case.
- Around line 452-514: The handlers for the "/tasks" and "/today" command
branches in channel.rs are directly calling deps.task_store.{list_ready, list}
(and then calling send_builtin_text), which violates the channel tool contract;
instead, change these branches to invoke the appropriate channel tool flow
(e.g., use the channel's reply/route/spawn_worker tool API) to request task
queries and rendering from the task/tool layer rather than querying task_store
directly. Locate the "/tasks" and "/today" match arms and replace the direct
deps.task_store calls and send_builtin_text usage with calls that dispatch a
tool action (referencing the channel tool names used in this repo such as
reply/route/spawn_worker) that will perform list_ready/list and return a
formatted body to be sent by the channel; keep the same resulting body/format
but move all task_store access into the tool implementation. Ensure no direct
deps.task_store.* calls remain in channel.rs.
- Around line 711-726: should_coalesce currently allows slash/command messages
to be batched which bypasses builtin handlers; update should_coalesce to return
false when the inbound message represents a command (e.g. check message.kind ==
Command or message.content.starts_with('/') or message.is_command()) so command
messages are never coalesced, and/or ensure handle_message_batch calls
try_handle_builtin_ops_commands and try_handle_builtin_digest for each message
in the batch before falling through to LLM handling so builtins run for batched
inputs (refer to should_coalesce, handle_message_batch,
try_handle_builtin_ops_commands, try_handle_builtin_digest).
- Around line 327-348: compute_listen_mode_invocation's invoked_by_mention match
in src/agent/channel.rs is missing a "twitch" arm; add a "twitch" branch similar
to the "telegram" case that looks up the bot username in message.metadata (e.g.,
"twitch_bot_username"), converts it to a string, formats a mention like
format!("@{username}") and checks if text.contains(&mention), with a fallback to
false via unwrap_or(false), so explicit Twitch mentions are detected and not
suppressed in listen-only mode.
- Around line 433-445: The handlers for the "/quiet" and "/active" commands
currently only flip self.listen_only_mode locally, but because the runtime
config is re-synced on each entry path the change is lost; update the runtime
configuration store when toggling these commands (e.g. call the runtime config
setter/update method to persist listen_only_mode = true/false, await the persist
operation, and handle/log errors), and keep self.listen_only_mode in sync with
the persisted value after a successful save; apply this to both the "/quiet" and
"/active" match arms and ensure send_builtin_text behavior remains unchanged.
600cf11 to
408748b
Compare
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
src/messaging/discord.rs (1)
952-959:⚠️ Potential issue | 🟠 MajorSplit invoke metadata is still missing on interaction events.
Lines 952–959 add split keys for message events, but
interaction_createstill only sets the combined flag. That can misclassify component interactions when downstream checks readdiscord_mentioned_bot/discord_reply_to_bot.Proposed fix
--- a/src/messaging/discord.rs +++ b/src/messaging/discord.rs @@ - metadata.insert( - "discord_mentions_or_replies_to_bot".into(), - serde_json::Value::Bool(true), - ); + let invokes_bot = true; + metadata.insert( + "discord_mentions_or_replies_to_bot".into(), + serde_json::Value::Bool(invokes_bot), + ); + metadata.insert( + "discord_mentioned_bot".into(), + serde_json::Value::Bool(invokes_bot), + ); + metadata.insert( + "discord_reply_to_bot".into(), + serde_json::Value::Bool(invokes_bot), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/discord.rs` around lines 952 - 959, The interaction_create handler currently only sets the combined flag and omits the split keys, causing downstream checks for "discord_mentioned_bot" and "discord_reply_to_bot" to miss interaction events; update the interaction_create branch to insert the same two metadata keys as the message branch by calling metadata.insert("discord_mentioned_bot".into(), is_mention_to_bot(interaction, bot_user_id).into()) and metadata.insert("discord_reply_to_bot".into(), is_reply_to_bot(interaction, bot_user_id).into()), using the same helper functions (is_mention_to_bot and is_reply_to_bot) so component and other interactions get the per-key flags in addition to the combined flag.src/config.rs (1)
1324-1325:⚠️ Potential issue | 🟠 MajorPreserve settings-backed listen-only precedence during reload.
Line 1324 resolves channel config using only agent/default, and Line 5830 stores that directly into runtime. This can overwrite DB-persisted listen-only intent after config reload, which breaks the intended precedence for this subsystem.
As per coding guidelines,
src/config.rs: Config resolution must follow:env > DB > defaultwith per-subsystemresolve()methods.Also applies to: 5830-5830
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.rs` around lines 1324 - 1325, The config resolution for channel (using self.channel.unwrap_or(defaults.channel) and resolve_mcp_configs) overwrites DB-persisted listen-only intent during reload; update resolution to follow env > DB > default by invoking the subsystem resolve() semantics instead of blindly using defaults—e.g., call the channel-specific resolver that accepts the current runtime/DB value, the incoming settings value (self.channel), and the defaults so that the resolved channel honors an existing DB-backed listen-only flag; similarly ensure when storing into runtime you write the resolved value rather than the direct unwrap result and apply the same pattern for MCP via resolve_mcp_configs/its resolve variant.src/agent/channel.rs (4)
338-346:⚠️ Potential issue | 🟡 MinorNormalize Telegram mention matching to avoid case-sensitive misses.
Current matching can miss explicit invokes when username casing differs from message casing, causing unintended suppression in listen-only mode.
Proposed fix
- "telegram" => message + "telegram" => message .metadata .get("telegram_bot_username") .and_then(|v| v.as_str()) .map(|username| { - let mention = format!("@{username}"); - text.contains(&mention) + let mention = format!("@{}", username.to_lowercase()); + text.to_lowercase().contains(&mention) }) .unwrap_or(false),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 338 - 346, The Telegram mention check is case-sensitive and can miss mentions; update the "telegram" match arm (the code using message.metadata.get("telegram_bot_username"), username, mention, and text.contains) to do a case-insensitive comparison by normalizing both the message text and the constructed mention to the same case (e.g., call to_lowercase() on text and on the formatted "@{username}") before calling contains, so mentions match regardless of casing.
364-376:⚠️ Potential issue | 🟠 MajorTighten Telegram reply invoke checks to “replying to this bot”.
Using generic
reply_to_is_botfor non-Discord sources can trigger false invokes when replying to other bots, which weakens listen-only suppression.Proposed fix
- _ => message - .metadata - .get("reply_to_is_bot") - .and_then(|v| v.as_bool()) - .unwrap_or(false), + "telegram" => { + let reply_to_is_bot = message + .metadata + .get("reply_to_is_bot") + .and_then(|v| v.as_bool()) + .unwrap_or(false); + let reply_to_matches_this_bot = match ( + message.metadata.get("reply_to_username").and_then(|v| v.as_str()), + message.metadata.get("telegram_bot_username").and_then(|v| v.as_str()), + ) { + (Some(reply_to), Some(bot_username)) => { + reply_to.eq_ignore_ascii_case(bot_username) + } + _ => true, + }; + reply_to_is_bot && reply_to_matches_this_bot + } + _ => message + .metadata + .get("reply_to_is_bot") + .and_then(|v| v.as_bool()) + .unwrap_or(false),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 364 - 376, The current invoked_by_reply logic uses the generic "reply_to_is_bot" for non-Discord sources which can cause false invokes; update the match in channel.rs so "telegram" is handled explicitly: read a Telegram-specific metadata key (e.g. "telegram_reply_to_bot_id" or "telegram_reply_to_this_bot") from message.metadata and compare it against this agent's bot id/identity (use the existing agent identity variable or accessor such as self.bot_id or whatever identity getter exists) to set invoked_by_reply, falling back to false if the metadata or id is missing; leave the generic "reply_to_is_bot" branch only for other unknown sources and ensure invoked_by_reply remains false by default.
467-529:⚠️ Potential issue | 🟠 MajorRoute
/tasksand/todaythrough channel tools instead of direct store access.These command paths execute task retrieval directly inside channel logic, bypassing the required channel tool flow.
As per coding guidelines
src/agent/channel.rs: "Channel processes must use tools: reply, branch, spawn_worker, route, cancel, skip, react; they must NOT execute tasks directly or search memories themselves".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 467 - 529, The match arms for "/tasks" and "/today" directly call self.deps.task_store.list_ready and self.deps.task_store.list (with TaskStatus::InProgress) which bypasses channel tools; replace these direct store accesses by invoking the channel tool flow (use the channel's route tool) to request task data, await the tool's response, then format and call send_builtin_text as before; update the "/tasks" and "/today" branches to call the route tool (instead of deps.task_store.list_ready / deps.task_store.list / TaskStatus::InProgress) and handle the routed response to produce the same lines passed to send_builtin_text.
565-603:⚠️ Potential issue | 🟠 Major
/digestuses an ad-hoc LLM path outside the required channel agent contract.This creates a separate agent call without the standard channel tool/history/hook wiring, which breaks the LLM process contract for
src/agent/**/*.rs.As per coding guidelines
src/agent/**/*.rs: "Every LLM process must be a RigAgent<SpacebotModel, SpacebotHook>with system prompt, tools, history, and hooks".Also applies to: 614-627
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 565 - 603, The current /digest flow creates an ad-hoc model+agent using SpacebotModel::make and AgentBuilder::new which bypasses the required channel agent contract; replace the SpacebotModel::make + AgentBuilder::new usage with constructing a Rig Agent<SpacebotModel, SpacebotHook> that includes the standard system prompt, tools, history wiring and hooks used elsewhere in the channel agent code. Concretely, remove the direct AgentBuilder::preamble/default_max_turns build and instead instantiate the channel-compatible agent (the same pattern used for other channel LLM calls) by creating the SpacebotModel via SpacebotModel::make, then building an Agent<SpacebotModel, SpacebotHook> with the channel system prompt, attach the channel history tool, register the SpacebotHook implementation, and preserve routing via with_routing so the LLM call follows the standard tools/history/hooks contract.
🧹 Nitpick comments (1)
src/api/config.rs (1)
698-709: Add a unit test forupdate_channel_table.This helper is new but currently uncovered in the local test module. A focused test (write + assert
channel.listen_only_mode) would prevent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/config.rs` around lines 698 - 709, Add a unit test that constructs a toml_edit::DocumentMut representing an agent, creates a ChannelUpdate with listen_only_mode set (true/false), calls update_channel_table(doc_mut, agent_idx, &channel) and then asserts that the resulting document has the "channel" subtable with "listen_only_mode" set to the expected toml value; use the same helper symbols from the diff (update_channel_table, ChannelUpdate, get_agent_table_mut/get_or_create_subtable indirectly via the function) and return Result<(), StatusCode> handling in the test by unwrapping or mapping errors to test failures so the test fails on error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tools/reply.rs`:
- Around line 449-460: When thread_name is provided alongside cards_requested,
interactive_requested, or poll_requested the code currently drops those rich
payloads and still returns OutboundResponse::ThreadReply; instead detect this
conflict (check thread_name together with cards_requested ||
interactive_requested || poll_requested) and return a structured tool error
Result indicating that thread replies do not support
cards/interactive_elements/polls (rather than constructing
OutboundResponse::ThreadReply with converted_content); update the function's
error return path to produce the appropriate error variant so the LLM can
observe and recover from the conflict (refer to thread_name,
OutboundResponse::ThreadReply, cards_requested, interactive_requested,
poll_requested, and converted_content to locate the logic).
- Around line 460-479: The new early-return Err paths in the
cards/interactive/poll check can cause log_bot_message_with_name to be called
even when no message is sent; update the flow so that logging occurs only after
response_tx.send(...) has returned Ok — move or call log_bot_message_with_name
and any associated success bookkeeping (currently happening before or regardless
of the send) to after the successful response_tx.send(...) call in the same
function (refer to response_tx.send, log_bot_message_with_name, and ReplyError)
so that on Err(...) you return immediately without logging a sent reply.
---
Duplicate comments:
In `@src/agent/channel.rs`:
- Around line 338-346: The Telegram mention check is case-sensitive and can miss
mentions; update the "telegram" match arm (the code using
message.metadata.get("telegram_bot_username"), username, mention, and
text.contains) to do a case-insensitive comparison by normalizing both the
message text and the constructed mention to the same case (e.g., call
to_lowercase() on text and on the formatted "@{username}") before calling
contains, so mentions match regardless of casing.
- Around line 364-376: The current invoked_by_reply logic uses the generic
"reply_to_is_bot" for non-Discord sources which can cause false invokes; update
the match in channel.rs so "telegram" is handled explicitly: read a
Telegram-specific metadata key (e.g. "telegram_reply_to_bot_id" or
"telegram_reply_to_this_bot") from message.metadata and compare it against this
agent's bot id/identity (use the existing agent identity variable or accessor
such as self.bot_id or whatever identity getter exists) to set invoked_by_reply,
falling back to false if the metadata or id is missing; leave the generic
"reply_to_is_bot" branch only for other unknown sources and ensure
invoked_by_reply remains false by default.
- Around line 467-529: The match arms for "/tasks" and "/today" directly call
self.deps.task_store.list_ready and self.deps.task_store.list (with
TaskStatus::InProgress) which bypasses channel tools; replace these direct store
accesses by invoking the channel tool flow (use the channel's route tool) to
request task data, await the tool's response, then format and call
send_builtin_text as before; update the "/tasks" and "/today" branches to call
the route tool (instead of deps.task_store.list_ready / deps.task_store.list /
TaskStatus::InProgress) and handle the routed response to produce the same lines
passed to send_builtin_text.
- Around line 565-603: The current /digest flow creates an ad-hoc model+agent
using SpacebotModel::make and AgentBuilder::new which bypasses the required
channel agent contract; replace the SpacebotModel::make + AgentBuilder::new
usage with constructing a Rig Agent<SpacebotModel, SpacebotHook> that includes
the standard system prompt, tools, history wiring and hooks used elsewhere in
the channel agent code. Concretely, remove the direct
AgentBuilder::preamble/default_max_turns build and instead instantiate the
channel-compatible agent (the same pattern used for other channel LLM calls) by
creating the SpacebotModel via SpacebotModel::make, then building an
Agent<SpacebotModel, SpacebotHook> with the channel system prompt, attach the
channel history tool, register the SpacebotHook implementation, and preserve
routing via with_routing so the LLM call follows the standard
tools/history/hooks contract.
In `@src/config.rs`:
- Around line 1324-1325: The config resolution for channel (using
self.channel.unwrap_or(defaults.channel) and resolve_mcp_configs) overwrites
DB-persisted listen-only intent during reload; update resolution to follow env >
DB > default by invoking the subsystem resolve() semantics instead of blindly
using defaults—e.g., call the channel-specific resolver that accepts the current
runtime/DB value, the incoming settings value (self.channel), and the defaults
so that the resolved channel honors an existing DB-backed listen-only flag;
similarly ensure when storing into runtime you write the resolved value rather
than the direct unwrap result and apply the same pattern for MCP via
resolve_mcp_configs/its resolve variant.
In `@src/messaging/discord.rs`:
- Around line 952-959: The interaction_create handler currently only sets the
combined flag and omits the split keys, causing downstream checks for
"discord_mentioned_bot" and "discord_reply_to_bot" to miss interaction events;
update the interaction_create branch to insert the same two metadata keys as the
message branch by calling metadata.insert("discord_mentioned_bot".into(),
is_mention_to_bot(interaction, bot_user_id).into()) and
metadata.insert("discord_reply_to_bot".into(), is_reply_to_bot(interaction,
bot_user_id).into()), using the same helper functions (is_mention_to_bot and
is_reply_to_bot) so component and other interactions get the per-key flags in
addition to the combined flag.
---
Nitpick comments:
In `@src/api/config.rs`:
- Around line 698-709: Add a unit test that constructs a toml_edit::DocumentMut
representing an agent, creates a ChannelUpdate with listen_only_mode set
(true/false), calls update_channel_table(doc_mut, agent_idx, &channel) and then
asserts that the resulting document has the "channel" subtable with
"listen_only_mode" set to the expected toml value; use the same helper symbols
from the diff (update_channel_table, ChannelUpdate,
get_agent_table_mut/get_or_create_subtable indirectly via the function) and
return Result<(), StatusCode> handling in the test by unwrapping or mapping
errors to test failures so the test fails on error.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
interface/src/api/client.tsinterface/src/routes/AgentConfig.tsxsrc/agent/channel.rssrc/agent/channel_prompt.rssrc/api/agents.rssrc/api/config.rssrc/config.rssrc/messaging/discord.rssrc/messaging/slack.rssrc/messaging/telegram.rssrc/messaging/twitch.rssrc/tools.rssrc/tools/reply.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- src/messaging/telegram.rs
- interface/src/api/client.ts
- src/messaging/slack.rs
- src/api/agents.rs
- src/agent/channel_prompt.rs
Author
|
Addressed review pass in commit 5287912.\n\nFixed:\n- reply tool now errors on thread_name + rich payload conflict (cards/interactive/poll) instead of silently dropping rich payloads.\n- reply tool now logs bot messages only after successful outbound send.\n- Telegram mention matching in listen-only flow is now case-insensitive.\n- Telegram reply invoke detection now matches reply_to_username against telegram_bot_username to avoid false invokes.\n- Discord interaction metadata now includes split keys: discord_mentioned_bot and discord_reply_to_bot.\n- Added unit test coverage for update_channel_table listen_only_mode true/false writes.\n\nIntentionally not changed in this pass:\n- Reworking /tasks and /today to route through tools (architectural change; current direct store access remains stable and testable).\n- Rebuilding /digest on full channel-agent tooling stack (large contract change; not required for this bugfix pass).\n- Config env>DB>default channel precedence in reload path as phrased by bot comment (does not map to current persisted config model in this branch).\n\nBuild/validation:\n- cargo fmt\n- cargo check\n- interface npm run build\nall passing. |
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/agent/channel.rs (2)
1176-1183:⚠️ Potential issue | 🟡 MinorMake Telegram mention matching truly case-insensitive.
textis lowercased on Line [1176], butmentionis not lowercased on Line [1181]-Line [1182], so mixed-case bot usernames can miss pings.Suggested fix
let mention = message .metadata .get("telegram_bot_username") .and_then(|v| v.as_str()) - .map(|u| format!("@{u}")) + .map(|u| format!("@{u}").to_lowercase()) .unwrap_or_default();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 1176 - 1183, The mention check lowercases raw_text into text but leaves mention as-is, so mixed-case Telegram usernames can be missed; update the code that builds mention (the expression using message.metadata.get("telegram_bot_username").and_then(|v| v.as_str()).map(|u| format!("@{u}"))) to lowercase the username (or call to_lowercase()) before formatting, or alternatively lowercase the constructed mention, and then use that normalized mention when computing has_mention so the contains check is truly case-insensitive (affecting variables mention and has_mention).
484-546:⚠️ Potential issue | 🟠 MajorBuilt-in
/tasks/today/digestbypass the channel tool/agent contract.These paths directly query task/conversation data and
/digestruns a separate prompt flow in channel code, bypassing the required channel tool path.Run this to verify the direct data-access and ad-hoc LLM path presence in this file (expected: matches in built-in handlers, separate from
run_agent_turnpath):#!/bin/bash set -euo pipefail # Direct task/memory reads inside built-ins rg -n -C2 '"/tasks"|"/today"|task_store\.(list|list_ready)\(' src/agent/channel.rs rg -n -C2 '"/digest"|conversation_logger\.load_recent\(' src/agent/channel.rs # Ad-hoc LLM call in digest handler vs normal channel turn path rg -n -C3 'try_handle_builtin_digest|AgentBuilder::new\(model\)|agent\.prompt\(&prompt\)\.await' src/agent/channel.rs rg -n -C3 'run_agent_turn\(' src/agent/channel.rsAs per coding guidelines
src/agent/channel.rs: "Channel processes must use tools: reply, branch, spawn_worker, route, cancel, skip, react; they must NOT execute tasks directly or search memories themselves" andsrc/agent/**/*.rs: "Every LLM process must be a RigAgent<SpacebotModel, SpacebotHook>with system prompt, tools, history, and hooks".Also applies to: 582-645
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 484 - 546, The built-in handlers for "/tasks", "/today", and "/digest" directly call deps.task_store.list/list_ready and run an ad-hoc LLM flow (conversation_logger.load_recent / try_handle_builtin_digest) instead of using the channel tool/agent contract; refactor these handlers (the match arms that call send_builtin_text for "/tasks" and "/today" and the digest code that uses AgentBuilder/agent.prompt) to stop direct data access and LLM invocation and instead invoke the approved channel tools or agent path: either call the existing run_agent_turn flow or create/route to a tool that encapsulates task_store.list/list_ready and memory reads, then call that tool via the channel’s routing/spawn/branch API so the LLM runs as an Agent (AgentBuilder/agent.prompt flow lives inside run_agent_turn or the tool), and replace any direct send_builtin_text with replies emitted by the routed tool/agent; locate code by the symbols send_builtin_text, deps.task_store.list, deps.task_store.list_ready, try_handle_builtin_digest, conversation_logger.load_recent, AgentBuilder::new, and run_agent_turn to make these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agent/channel.rs`:
- Around line 399-407: The bot currently logs outgoing messages via
conversation_logger.log_bot_message_with_name before attempting the outbound
send in send_builtin_text, causing “delivered” transcript entries even when
response_tx.send fails; change send_builtin_text to attempt
response_tx.send(OutboundResponse::Text(text.clone())).await first and only call
conversation_logger.log_bot_message_with_name(&self.state.channel_id, &text,
Some(self.agent_display_name())) after a successful send, and log an error (as
now) on send failure without persisting the transcript. Also update other send
sites (the /agent-id and ping-ack paths and the other occurrences noted) to
route through this unified send_builtin_text so all built-in replies follow the
same send-then-log semantics.
- Around line 1209-1218: The code computes `directed` using legacy keys
(`discord_mentions_or_replies_to_bot` and `reply_to_is_bot`) which diverges from
the invoke metadata contract; update the `directed` computation to read the
split Discord keys used elsewhere (`discord_mentioned_bot` and
`discord_reply_to_bot`) from `message.metadata` (via the same
.get(...).and_then(|v| v.as_bool()).unwrap_or(false) pattern) so the routing
uses the same metadata schema as the invoke gating; ensure you only reference
`discord_mentioned_bot` and `discord_reply_to_bot` when building `directed`.
---
Duplicate comments:
In `@src/agent/channel.rs`:
- Around line 1176-1183: The mention check lowercases raw_text into text but
leaves mention as-is, so mixed-case Telegram usernames can be missed; update the
code that builds mention (the expression using
message.metadata.get("telegram_bot_username").and_then(|v| v.as_str()).map(|u|
format!("@{u}"))) to lowercase the username (or call to_lowercase()) before
formatting, or alternatively lowercase the constructed mention, and then use
that normalized mention when computing has_mention so the contains check is
truly case-insensitive (affecting variables mention and has_mention).
- Around line 484-546: The built-in handlers for "/tasks", "/today", and
"/digest" directly call deps.task_store.list/list_ready and run an ad-hoc LLM
flow (conversation_logger.load_recent / try_handle_builtin_digest) instead of
using the channel tool/agent contract; refactor these handlers (the match arms
that call send_builtin_text for "/tasks" and "/today" and the digest code that
uses AgentBuilder/agent.prompt) to stop direct data access and LLM invocation
and instead invoke the approved channel tools or agent path: either call the
existing run_agent_turn flow or create/route to a tool that encapsulates
task_store.list/list_ready and memory reads, then call that tool via the
channel’s routing/spawn/branch API so the LLM runs as an Agent
(AgentBuilder/agent.prompt flow lives inside run_agent_turn or the tool), and
replace any direct send_builtin_text with replies emitted by the routed
tool/agent; locate code by the symbols send_builtin_text, deps.task_store.list,
deps.task_store.list_ready, try_handle_builtin_digest,
conversation_logger.load_recent, AgentBuilder::new, and run_agent_turn to make
these changes.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/agent/channel.rssrc/api/config.rssrc/messaging/discord.rssrc/tools/reply.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/api/config.rs
Author
|
Final stabilization pass complete on commit 11b2445. What I verified:
Full cargo test -q note:
Additional fixes just landed in this pass:
Requesting fresh bot review on latest head. @coderabbitai review |
Contributor
|
The integration test failures in I'll trigger a fresh review of the latest commit now. 🧠 Learnings used✅ Actions performedReview triggered.
|
Author
|
Final stabilization pass complete on commit 11b2445.\n\nWhat I verified:\n- ✅\n- ✅\n- running 0 tests test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s running 0 tests test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 4 filtered out; finished in 0.00s running 0 tests test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 3 filtered out; finished in 0.00s running 0 tests test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 4 filtered out; finished in 0.00s running 0 tests test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 18 filtered out; finished in 0.00s running 0 tests test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 1 filtered out; finished in 0.00s ✅\n- : ✅\n\nFull running 0 tests test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s running 4 tests running 3 tests failures: ---- test_bulletin_generation stdout ---- thread 'test_bulletin_generation' (8879922) panicked at tests/bulletin.rs:194:39: Caused by: ---- test_memory_recall_returns_results stdout ---- thread 'test_memory_recall_returns_results' (8879924) panicked at tests/bulletin.rs:167:39: Caused by: failures: test result: FAILED. 1 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.20s note:\n- Core/unit suites passed, but 2 integration tests in fail locally due to an external runtime lock ( already open under ) rather than PR logic changes.\n\nAdditional fixes just landed in this pass:\n- Telegram liveness mention check now lowercases mention text too (prevents case mismatch misses).\n- Quiet-mode suppression now happens before attachment download IO in .\n- Slack mention predicate cleanup (same semantics, less overhead).\n\nRequesting fresh bot review on latest head.\n\n@coderabbitai review\n@tembo please re-review on latest commit. |
Contributor
|
✅ Build & formatting verified:
✅ Test suite results:
✅ Additional fixes in this pass:
I'll trigger a fresh review of commit 11b2445 now. 🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/agent/channel.rs (3)
399-408:⚠️ Potential issue | 🟠 MajorPersist bot replies only after successful outbound send.
The current implementation logs the bot message before attempting to send it via
response_tx.send(). If the send fails, the conversation log will contain a "delivered" entry for a message that was never actually sent.Suggested fix: send first, then log on success
async fn send_builtin_text(&mut self, text: String, log_label: &str) { - self.state.conversation_logger.log_bot_message_with_name( - &self.state.channel_id, - &text, - Some(self.agent_display_name()), - ); - if let Err(error) = self.response_tx.send(OutboundResponse::Text(text)).await { + if let Err(error) = self + .response_tx + .send(OutboundResponse::Text(text.clone())) + .await + { tracing::error!(%error, channel_id = %self.id, %log_label, "failed to send built-in reply"); + return; } + self.state.conversation_logger.log_bot_message_with_name( + &self.state.channel_id, + &text, + Some(self.agent_display_name()), + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 399 - 408, In send_builtin_text, reverse the order so the outbound send is attempted before persisting: call self.response_tx.send(OutboundResponse::Text(text.clone())).await and only on Ok(()) call self.state.conversation_logger.log_bot_message_with_name(&self.state.channel_id, &text, Some(self.agent_display_name())); on Err(error) do not log the message and instead emit the existing tracing::error!(%error, channel_id = %self.id, %log_label, "failed to send built-in reply"); ensure you clone or otherwise retain the text for logging after the send attempt if needed.
1208-1219:⚠️ Potential issue | 🟡 MinorUse consistent metadata keys for Discord ping detection.
This block uses
discord_mentions_or_replies_to_botand genericreply_to_is_bot, butcompute_listen_mode_invocationuses the split keysdiscord_mentioned_botanddiscord_reply_to_bot. Using different metadata contracts can cause inconsistent behavior.Suggested fix: reuse compute_listen_mode_invocation
if message.source == "discord" && self.listen_only_mode && message.source != "system" { let text = raw_text.trim().to_lowercase(); - let directed = message - .metadata - .get("discord_mentions_or_replies_to_bot") - .and_then(|v| v.as_bool()) - .unwrap_or(false) - || message - .metadata - .get("reply_to_is_bot") - .and_then(|v| v.as_bool()) - .unwrap_or(false); + let (_, invoked_by_mention, invoked_by_reply) = + self.compute_listen_mode_invocation(&message, &raw_text); + let directed = invoked_by_mention || invoked_by_reply; let looks_like_ping = ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 1208 - 1219, The metadata keys used to detect Discord pings in the if block (checking message.metadata for "discord_mentions_or_replies_to_bot" and "reply_to_is_bot") are inconsistent with compute_listen_mode_invocation which expects "discord_mentioned_bot" and "discord_reply_to_bot"; update the logic in the message handling branch to reuse compute_listen_mode_invocation (or at minimum check the same "discord_mentioned_bot" and "discord_reply_to_bot" keys) so the listening-direction detection for message (and its directed/directed flag) is consistent across compute_listen_mode_invocation and the branch that computes directed from message.metadata.
647-658:⚠️ Potential issue | 🟠 MajorLog-before-send pattern: same issue as
send_builtin_text.This block logs the digest reply before attempting to send it. Apply the same fix pattern: send first, log only on success.
Suggested fix
- self.state.conversation_logger.log_bot_message_with_name( - &self.state.channel_id, - &reply_text, - Some(self.agent_display_name()), - ); if let Err(error) = self .response_tx - .send(OutboundResponse::Text(reply_text)) + .send(OutboundResponse::Text(reply_text.clone())) .await { tracing::error!(%error, channel_id = %self.id, "failed to send builtin /digest reply"); + return Ok(true); } + self.state.conversation_logger.log_bot_message_with_name( + &self.state.channel_id, + &reply_text, + Some(self.agent_display_name()), + ); Ok(true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 647 - 658, The digest reply is being logged before sending; change the order so response_tx.send(OutboundResponse::Text(reply_text)).await is performed first and only upon Ok(()) call conversation_logger.log_bot_message_with_name(&self.state.channel_id, &reply_text, Some(self.agent_display_name())); on Err(error) keep the existing tracing::error!(%error, channel_id = %self.id, "failed to send builtin /digest reply") behavior; ensure you reference the same symbols (response_tx.send, OutboundResponse::Text, conversation_logger.log_bot_message_with_name, agent_display_name, self.state.channel_id, self.id) and do not drop the reply_text ownership until after send completes.
🧹 Nitpick comments (2)
src/agent/channel.rs (2)
1240-1249: Potentially dead code:/digestrewrite is superseded by built-in handler.This rewrite is executed before
try_handle_builtin_digest(line 1299), which handles alltelegram+/digestcases and returns early. The rewritten text is never used since:
- For telegram
/digest: built-in handler catches it first- For non-telegram: the rewrite doesn't apply
Consider removing this block or moving the built-in check earlier to clarify the flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 1240 - 1249, The `/digest` rewrite in the rewritten_text block is dead because try_handle_builtin_digest handles telegram `/digest` earlier; remove the special-case mapping for "/digest" (the branch that returns the long hardcoded string) from the rewritten_text construction (which references message.source and raw_text) so the code no longer creates an unused rewritten_text for that command, or alternatively move the try_handle_builtin_digest call (function try_handle_builtin_digest) to run before building rewritten_text so the built-in handler short-circuits before this logic—pick one fix and apply it consistently.
1155-1171: Refactor to usesend_builtin_textfor consistency.This
/agent-idhandler duplicates the send-and-log pattern. Oncesend_builtin_textis fixed to log after successful send, refactor this to use it:self.send_builtin_text(self.deps.agent_id.to_string(), "agent-id").await; return Ok(());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 1155 - 1171, Refactor the /agent-id handler to call the existing send_builtin_text helper instead of duplicating send-and-log logic: replace the manual use of self.deps.agent_id, the call to self.state.conversation_logger.log_bot_message_with_name, and the send via self.response_tx with a single await call to self.send_builtin_text(self.deps.agent_id.to_string(), "agent-id") and then return Ok(()); ensure send_builtin_text is used so logging occurs after a successful send and remove the duplicated tracing::error branch in this block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/agent/channel.rs`:
- Around line 399-408: In send_builtin_text, reverse the order so the outbound
send is attempted before persisting: call
self.response_tx.send(OutboundResponse::Text(text.clone())).await and only on
Ok(()) call
self.state.conversation_logger.log_bot_message_with_name(&self.state.channel_id,
&text, Some(self.agent_display_name())); on Err(error) do not log the message
and instead emit the existing tracing::error!(%error, channel_id = %self.id,
%log_label, "failed to send built-in reply"); ensure you clone or otherwise
retain the text for logging after the send attempt if needed.
- Around line 1208-1219: The metadata keys used to detect Discord pings in the
if block (checking message.metadata for "discord_mentions_or_replies_to_bot" and
"reply_to_is_bot") are inconsistent with compute_listen_mode_invocation which
expects "discord_mentioned_bot" and "discord_reply_to_bot"; update the logic in
the message handling branch to reuse compute_listen_mode_invocation (or at
minimum check the same "discord_mentioned_bot" and "discord_reply_to_bot" keys)
so the listening-direction detection for message (and its directed/directed
flag) is consistent across compute_listen_mode_invocation and the branch that
computes directed from message.metadata.
- Around line 647-658: The digest reply is being logged before sending; change
the order so response_tx.send(OutboundResponse::Text(reply_text)).await is
performed first and only upon Ok(()) call
conversation_logger.log_bot_message_with_name(&self.state.channel_id,
&reply_text, Some(self.agent_display_name())); on Err(error) keep the existing
tracing::error!(%error, channel_id = %self.id, "failed to send builtin /digest
reply") behavior; ensure you reference the same symbols (response_tx.send,
OutboundResponse::Text, conversation_logger.log_bot_message_with_name,
agent_display_name, self.state.channel_id, self.id) and do not drop the
reply_text ownership until after send completes.
---
Nitpick comments:
In `@src/agent/channel.rs`:
- Around line 1240-1249: The `/digest` rewrite in the rewritten_text block is
dead because try_handle_builtin_digest handles telegram `/digest` earlier;
remove the special-case mapping for "/digest" (the branch that returns the long
hardcoded string) from the rewritten_text construction (which references
message.source and raw_text) so the code no longer creates an unused
rewritten_text for that command, or alternatively move the
try_handle_builtin_digest call (function try_handle_builtin_digest) to run
before building rewritten_text so the built-in handler short-circuits before
this logic—pick one fix and apply it consistently.
- Around line 1155-1171: Refactor the /agent-id handler to call the existing
send_builtin_text helper instead of duplicating send-and-log logic: replace the
manual use of self.deps.agent_id, the call to
self.state.conversation_logger.log_bot_message_with_name, and the send via
self.response_tx with a single await call to
self.send_builtin_text(self.deps.agent_id.to_string(), "agent-id") and then
return Ok(()); ensure send_builtin_text is used so logging occurs after a
successful send and remove the duplicated tracing::error branch in this block.
Author
|
Applied the latest CodeRabbit channel.rs feedback in commit baa3264. Updated:
Validation:
@coderabbitai review |
Contributor
|
✅ Send-then-log semantics consolidated:
✅ Metadata contract consistency:
✅ Code cleanup:
✅ Validation confirmed:
I'll trigger a review of commit baa3264 now. 🧠 Learnings used✅ Actions performedReview triggered.
|
Author
|
Applied follow-up hardening in 485ec55:
@coderabbitai review |
Contributor
|
✅ Telegram ping mention — unified matcher:
✅ Telegram reply invoke — stricter identity checks:
✅ Twitch reply detection — bot-targeted check:
These changes further harden the invoke detection surface across both platforms. I'll trigger a fresh review of commit 485ec55 now. 🧠 Learnings used✅ Actions performedReview triggered.
|
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
|
✅ Telegram mention matching — boundary-aware:
✅ Twitch combined invoke flag — reply coverage:
I'll trigger a fresh review of commit 3c72735 now. 🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agent/channel.rs`:
- Around line 255-256: The listen-only mode is currently stored and loaded from
a shared runtime_config.channel_config and a single persisted key
("channel_listen_only_mode"), which causes one channel's /quiet or /active to
affect all channels; update the logic in set_listen_only_mode and the
Channel::new/default initialization (where default_listen_only_mode is read) to
key the persisted value by ChannelId (e.g.,
"channel_listen_only_mode:{channel_id}") and to read/write per-channel state
instead of using the shared channel_config snapshot; ensure handlers resync
using a per-ChannelId lookup (not the shared channel_config.listen_only_mode) so
each Channel instance resolves its listen-only mode from the channel-qualified
persisted key and ChannelId.
In `@src/config/runtime.rs`:
- Around line 218-223: The code falls back to current.as_ref().listen_only_mode
which can mask a new default; change the fallback so listen_only_mode resolves
as env (configured_listen_only) > DB (persisted_listen_only) > default from
resolved_channel, i.e. use resolved_channel.listen_only_mode as the final
fallback instead of current.as_ref().listen_only_mode inside the
channel_config.rcu closure where resolved_channel, configured_listen_only,
persisted_listen_only and listen_only_mode are handled.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8ea40bb-433a-4daf-adc6-227da018883a
📒 Files selected for processing (14)
src/agent/channel.rssrc/api/agents.rssrc/config/load.rssrc/config/runtime.rssrc/config/toml_schema.rssrc/config/types.rssrc/main.rssrc/messaging/discord.rssrc/messaging/slack.rssrc/messaging/twitch.rssrc/settings.rssrc/settings/store.rssrc/tools.rssrc/tools/reply.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/messaging/twitch.rs
- src/api/agents.rs
- src/messaging/slack.rs
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/config/runtime.rs (1)
218-223:⚠️ Potential issue | 🟠 MajorPreserve the current RCU snapshot during reload merge.
At Line 218, the closure ignores
current. If reload races with an in-memory listen-only toggle, this path can overwrite the newer value with a stale/default one when no configured/DB override is present.Suggested fix
- self.channel_config.rcu(move |_current| { + self.channel_config.rcu(move |current| { let mut next = resolved_channel; next.listen_only_mode = configured_listen_only .or(persisted_listen_only) - .unwrap_or(next.listen_only_mode); + .unwrap_or(current.as_ref().listen_only_mode); Arc::new(next) });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/runtime.rs` around lines 218 - 223, The closure passed to channel_config.rcu ignores the current snapshot and uses next.listen_only_mode as the fallback, which can overwrite a newer in-memory toggle during reload; change the closure to accept the current snapshot (use |current| instead of |_current|) and set next.listen_only_mode = configured_listen_only.or(persisted_listen_only).unwrap_or_else(|| current.listen_only_mode) so the merge preserves the live/current listen_only_mode when no configured or persisted override is present (references: channel_config.rcu closure, resolved_channel, listen_only_mode, configured_listen_only, persisted_listen_only).
🧹 Nitpick comments (1)
src/tools/reply.rs (1)
75-78: Optional cleanup: document whyagent_idis stored while style enforcement is a no-op.
enforce_agent_stylecurrently ignoresagent_id; a short comment linking this to planned behavior (or a tracking issue) would reduce future confusion.Also applies to: 49-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/reply.rs` around lines 75 - 78, The function enforce_agent_style currently accepts _agent_id but ignores it; add a short explanatory comment inside enforce_agent_style (and the similar occurrence around where agent_id is stored) stating that agent-specific style/voice transformations are a planned feature and reference the tracking issue or TODO (e.g., "TODO: implement per-agent style transforms, see ISSUE-XXXX") so future readers know why agent_id is captured while the function is a no-op; include the function name enforce_agent_style and the variable _agent_id in the comment to make the intent explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agent/channel.rs`:
- Around line 255-275: The merge currently lets persisted_listen_only_mode
override runtime config (default_listen_only_mode); change the logic in the
channel construction so configured/env value from
runtime_config.channel_config.listen_only_mode takes highest precedence, then
persisted_listen_only_mode, then the hard default: i.e., if
runtime_config.channel_config provides an explicit listen_only_mode use that for
the channel, else if persisted_listen_only_mode.is_some() use that, otherwise
fall back to default_listen_only_mode; update the code around the variables
default_listen_only_mode, persisted_listen_only_mode and the channel field
assignment to enforce this precedence.
---
Duplicate comments:
In `@src/config/runtime.rs`:
- Around line 218-223: The closure passed to channel_config.rcu ignores the
current snapshot and uses next.listen_only_mode as the fallback, which can
overwrite a newer in-memory toggle during reload; change the closure to accept
the current snapshot (use |current| instead of |_current|) and set
next.listen_only_mode =
configured_listen_only.or(persisted_listen_only).unwrap_or_else(||
current.listen_only_mode) so the merge preserves the live/current
listen_only_mode when no configured or persisted override is present
(references: channel_config.rcu closure, resolved_channel, listen_only_mode,
configured_listen_only, persisted_listen_only).
---
Nitpick comments:
In `@src/tools/reply.rs`:
- Around line 75-78: The function enforce_agent_style currently accepts
_agent_id but ignores it; add a short explanatory comment inside
enforce_agent_style (and the similar occurrence around where agent_id is stored)
stating that agent-specific style/voice transformations are a planned feature
and reference the tracking issue or TODO (e.g., "TODO: implement per-agent style
transforms, see ISSUE-XXXX") so future readers know why agent_id is captured
while the function is a no-op; include the function name enforce_agent_style and
the variable _agent_id in the comment to make the intent explicit.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2294541f-e317-4cbe-b5df-061d9b99389a
📒 Files selected for processing (6)
src/agent/channel.rssrc/config/runtime.rssrc/messaging/discord.rssrc/messaging/twitch.rssrc/settings/store.rssrc/tools/reply.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/messaging/discord.rs
- src/messaging/twitch.rs
Contributor
|
✅ Telegram reply invoke fallback:
✅
✅ Poll support matrix alignment:
✅ Twitch mention micro-nit:
✅ Discord combined-flag derivation:
I'll trigger a fresh review of commit 4fa37ba now. 🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
|
✅ Telegram ping mention — unified matcher:
✅ Telegram reply invoke — stricter identity checks:
✅ Twitch reply detection — bot-targeted check:
These changes further harden the invoke detection surface across both platforms. I'll trigger a fresh review of commit 485ec55 now. 🧠 Learnings used✅ Actions performedReview triggered.
|
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
1 similar comment
Contributor
✅ Actions performedReview triggered.
|
Contributor
|
✅ Telegram mention matching — boundary-aware:
✅ Twitch combined invoke flag — reply coverage:
I'll trigger a fresh review of commit 3c72735 now. 🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/messaging/slack.rs (1)
272-311: Avoid unnecessary API lookup when mention already confirms bot invoke.On Lines 272–273,
tokenandsessionare created unconditionally. Then on Line 274–311,replied_to_botevaluation and the API call toconversations_replies()execute unconditionally, even whenmentioned_botis alreadytrue(Lines 266–271). Since the final metadata usesmentioned_bot || replied_to_bot(Line 314), the entire thread lookup is redundant when the bot is mentioned.♻️ Proposed refactor
- let token = SlackApiToken::new(SlackApiTokenValue(adapter_state.bot_token.clone())); - let session = client.open_session(&token); - let replied_to_bot = if let Some(thread_ts) = msg_event.origin.thread_ts.as_ref() { + let replied_to_bot = if mentioned_bot { + false + } else if let Some(thread_ts) = msg_event.origin.thread_ts.as_ref() { // For threaded replies, treat as explicit invoke only when the thread // root message belongs to this bot. if thread_ts.0 != ts { + let token = SlackApiToken::new(SlackApiTokenValue(adapter_state.bot_token.clone())); + let session = client.open_session(&token); let thread_replies_request = SlackApiConversationsRepliesRequest::new( SlackChannelId(channel_id.clone()), thread_ts.clone(), ) .with_limit(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/slack.rs` around lines 272 - 311, The code currently always creates a SlackApiToken and session and performs a conversations_replies lookup to compute replied_to_bot even when mentioned_bot is already true; change the logic to short-circuit: if mentioned_bot is true then set replied_to_bot = false (or skip computing it) and avoid constructing SlackApiToken/SlackApiTokenValue and calling client.open_session or session.conversations_replies; only create the token/session and perform the timeout+session.conversations_replies call when mentioned_bot is false and msg_event.origin.thread_ts is Some and thread_ts.0 != ts, keeping the existing timeout/error handling and comparisons against adapter_state.bot_user_id so replied_to_bot is computed only when needed.src/tools/reply.rs (2)
81-112: Add targeted tests for card-only fallback and empty-content defaulting.The new
cards_to_text+"noted."fallback path changes user-visible behavior; add unit tests for card-only input, whitespace-only cards, and empty rich payloads to lock regressions down.Also applies to: 406-420
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/reply.rs` around lines 81 - 112, Add unit tests covering the new cards_to_text behavior: 1) a card-only case where cards_to_text returns the card's concatenated text and the higher-level fallback appends "noted." as expected; 2) whitespace-only cards where title/description/fields/footer are whitespace and should be treated as empty resulting in the empty-content default path; and 3) an empty rich payload (no cards or all-empty cards) that triggers the empty-content defaulting. Place tests exercising the cards_to_text function and the caller behavior that appends "noted." (where applicable), using the Card struct instances with title/description/fields/footer variants to assert exact output strings and ensure regressions are caught.
55-63: MakeReplyTool::newidentity params type-safe to prevent accidental swaps.
agent_idandagent_display_nameare both string-like inputs, so argument-order mistakes will compile silently. Consider using a typedAgentIdinput (or a small identity struct) for stronger call-site safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/reply.rs` around lines 55 - 63, Change ReplyTool::new to accept a type-safe identity for agent_id instead of a plain string: introduce a small newtype (e.g., struct AgentId(String) with From<String>/From<&str> and common derives) and change the signature of ReplyTool::new to take agent_id: AgentId while keeping agent_display_name: impl Into<String>; update any internal uses of agent_id to call .0 or provide accessor methods, and update all call sites to pass AgentId::from(...) (or rely on From impl) to prevent accidental argument swaps between agent_id and agent_display_name.src/settings/store.rs (1)
171-185: Consider extracting shared bool-read parsing/error mapping into one helper.Both listen-only getters repeat the same parse/error conversion flow; a small private helper would reduce drift risk.
♻️ Optional DRY refactor
impl SettingsStore { + fn get_optional_bool(&self, key: &str) -> Result<Option<bool>> { + match self.get_raw(key) { + Ok(raw) => raw.parse::<bool>().map(Some).map_err(|error| { + SettingsError::ReadFailed { + key: key.to_string(), + details: format!("invalid boolean value '{raw}': {error}"), + } + .into() + }), + Err(crate::error::Error::Settings(settings_error)) => match *settings_error { + SettingsError::NotFound { .. } => Ok(None), + other => Err(other.into()), + }, + Err(other) => Err(other), + } + } + pub fn channel_listen_only_mode(&self) -> Result<Option<bool>> { - match self.get_raw(CHANNEL_LISTEN_ONLY_MODE_KEY) { - Ok(raw) => raw.parse::<bool>().map(Some).map_err(|error| { - SettingsError::ReadFailed { - key: CHANNEL_LISTEN_ONLY_MODE_KEY.to_string(), - details: format!("invalid boolean value '{raw}': {error}"), - } - .into() - }), - Err(crate::error::Error::Settings(settings_error)) => match *settings_error { - SettingsError::NotFound { .. } => Ok(None), - other => Err(other.into()), - }, - Err(other) => Err(other), - } + self.get_optional_bool(CHANNEL_LISTEN_ONLY_MODE_KEY) } pub fn channel_listen_only_mode_for(&self, channel_id: &str) -> Result<Option<bool>> { let key = Self::channel_listen_only_mode_key(channel_id); - match self.get_raw(&key) { - Ok(raw) => raw.parse::<bool>().map(Some).map_err(|error| { - SettingsError::ReadFailed { - key: key.clone(), - details: format!("invalid boolean value '{raw}': {error}"), - } - .into() - }), - Err(crate::error::Error::Settings(settings_error)) => match *settings_error { - SettingsError::NotFound { .. } => Ok(None), - other => Err(other.into()), - }, - Err(other) => Err(other), - } + self.get_optional_bool(&key) } }Also applies to: 197-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/settings/store.rs` around lines 171 - 185, Extract the repeated "get raw string and parse optional bool with SettingsError mapping" logic into a small private helper (e.g., fn parse_optional_bool(&self, key: &str) -> Result<Option<bool>>), then replace channel_listen_only_mode and the similar getter at 197-212 to call that helper; the helper should call self.get_raw(key), map Ok(raw) -> raw.parse::<bool>().map(Some).map_err(|error| SettingsError::ReadFailed { key: key.to_string(), details: format!("invalid boolean value '{raw}': {error}"), }.into()), handle Err(SettingsError::NotFound) -> Ok(None), and propagate other Errs unchanged so the existing behavior and error mapping (including SettingsError::ReadFailed) is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/messaging/slack.rs`:
- Around line 272-311: The code currently always creates a SlackApiToken and
session and performs a conversations_replies lookup to compute replied_to_bot
even when mentioned_bot is already true; change the logic to short-circuit: if
mentioned_bot is true then set replied_to_bot = false (or skip computing it) and
avoid constructing SlackApiToken/SlackApiTokenValue and calling
client.open_session or session.conversations_replies; only create the
token/session and perform the timeout+session.conversations_replies call when
mentioned_bot is false and msg_event.origin.thread_ts is Some and thread_ts.0 !=
ts, keeping the existing timeout/error handling and comparisons against
adapter_state.bot_user_id so replied_to_bot is computed only when needed.
In `@src/settings/store.rs`:
- Around line 171-185: Extract the repeated "get raw string and parse optional
bool with SettingsError mapping" logic into a small private helper (e.g., fn
parse_optional_bool(&self, key: &str) -> Result<Option<bool>>), then replace
channel_listen_only_mode and the similar getter at 197-212 to call that helper;
the helper should call self.get_raw(key), map Ok(raw) ->
raw.parse::<bool>().map(Some).map_err(|error| SettingsError::ReadFailed { key:
key.to_string(), details: format!("invalid boolean value '{raw}': {error}"),
}.into()), handle Err(SettingsError::NotFound) -> Ok(None), and propagate other
Errs unchanged so the existing behavior and error mapping (including
SettingsError::ReadFailed) is preserved.
In `@src/tools/reply.rs`:
- Around line 81-112: Add unit tests covering the new cards_to_text behavior: 1)
a card-only case where cards_to_text returns the card's concatenated text and
the higher-level fallback appends "noted." as expected; 2) whitespace-only cards
where title/description/fields/footer are whitespace and should be treated as
empty resulting in the empty-content default path; and 3) an empty rich payload
(no cards or all-empty cards) that triggers the empty-content defaulting. Place
tests exercising the cards_to_text function and the caller behavior that appends
"noted." (where applicable), using the Card struct instances with
title/description/fields/footer variants to assert exact output strings and
ensure regressions are caught.
- Around line 55-63: Change ReplyTool::new to accept a type-safe identity for
agent_id instead of a plain string: introduce a small newtype (e.g., struct
AgentId(String) with From<String>/From<&str> and common derives) and change the
signature of ReplyTool::new to take agent_id: AgentId while keeping
agent_display_name: impl Into<String>; update any internal uses of agent_id to
call .0 or provide accessor methods, and update all call sites to pass
AgentId::from(...) (or rely on From impl) to prevent accidental argument swaps
between agent_id and agent_display_name.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a5df3756-ef92-4afd-ab22-849b0673352e
📒 Files selected for processing (14)
src/agent/channel.rssrc/api/agents.rssrc/config/load.rssrc/config/runtime.rssrc/config/toml_schema.rssrc/config/types.rssrc/main.rssrc/messaging/discord.rssrc/messaging/slack.rssrc/messaging/twitch.rssrc/settings.rssrc/settings/store.rssrc/tools.rssrc/tools/reply.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/messaging/twitch.rs
- src/api/agents.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/config/runtime.rs (1)
210-232:⚠️ Potential issue | 🟠 MajorUse
resolved_channel.listen_only_modeas the fallback to apply config defaults on reload.The RCU merge at lines 228-230 uses
current.as_ref().listen_only_modeas the final fallback, which preserves the in-memory state but prevents changed defaults from applying during reload. Sinceresolved_channelis created fromagent.resolve(&config.defaults), it contains the properly resolved default value and should be the fallback instead.Current precedence:
configured_listen_only > persisted_listen_only > current (in-memory)Should be:
configured_listen_only > persisted_listen_only > resolved_channel (default)This ensures the config resolution order
env/config > DB > defaultis followed consistently, matching the pattern used during initial setup inset_settings().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/runtime.rs` around lines 210 - 232, The RCU merge in the closure passed to self.channel_config.rcu uses current.as_ref().listen_only_mode as the final fallback, preventing updated defaults from taking effect on reload; change the fallback to use resolved_channel.listen_only_mode instead so the precedence becomes configured_listen_only > persisted_listen_only > resolved_channel (default) — update the closure where next.listen_only_mode is set (referencing resolved_channel, configured_listen_only, persisted_listen_only, and current) to use resolved_channel.listen_only_mode rather than current.as_ref().listen_only_mode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agent/channel.rs`:
- Around line 331-333: The command responses are checking persisted settings but
not the effective mode when runtime/env explicitly pins listen-only, causing
mismatches; update the logic in sync_listen_only_mode_from_runtime and any
command-response paths that read listen_only_mode (also the similar block around
the code at 603-619) to compute and return the effective mode by honoring
explicit_listen_only/runtime_default when explicit_listen_only.is_some(), i.e.,
derive the reported state from the same expression used to set
self.listen_only_mode (using explicit_listen_only presence to prefer
runtime_default) so responses reflect the mode that will be restored on the next
sync.
---
Duplicate comments:
In `@src/config/runtime.rs`:
- Around line 210-232: The RCU merge in the closure passed to
self.channel_config.rcu uses current.as_ref().listen_only_mode as the final
fallback, preventing updated defaults from taking effect on reload; change the
fallback to use resolved_channel.listen_only_mode instead so the precedence
becomes configured_listen_only > persisted_listen_only > resolved_channel
(default) — update the closure where next.listen_only_mode is set (referencing
resolved_channel, configured_listen_only, persisted_listen_only, and current) to
use resolved_channel.listen_only_mode rather than
current.as_ref().listen_only_mode.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a7f389ba-4f92-46f5-8217-7b248f6a707f
📒 Files selected for processing (14)
src/agent/channel.rssrc/api/agents.rssrc/config/load.rssrc/config/runtime.rssrc/config/toml_schema.rssrc/config/types.rssrc/main.rssrc/messaging/discord.rssrc/messaging/slack.rssrc/messaging/twitch.rssrc/settings.rssrc/settings/store.rssrc/tools.rssrc/tools/reply.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/messaging/discord.rs
- src/tools.rs
Contributor
|
✅ Telegram ping mention — unified matcher:
✅ Telegram reply invoke — stricter identity checks:
✅ Twitch reply detection — bot-targeted check:
These changes further harden the invoke detection surface across both platforms. I'll trigger a fresh review of commit 485ec55 now. 🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/config/runtime.rs (1)
226-231:⚠️ Potential issue | 🟠 MajorReload merge fallback currently violates
env > DB > defaultfor listen-only mode.When
configured_listen_onlyandpersisted_listen_onlyare bothNone, falling back tocurrent.listen_only_modecan preserve stale runtime state and ignore a newly resolved default.Suggested patch
self.channel_config.rcu(move |current| { let mut next = resolved_channel; next.listen_only_mode = configured_listen_only .or(persisted_listen_only) - .unwrap_or(current.as_ref().listen_only_mode); + .unwrap_or(next.listen_only_mode); Arc::new(next) });Based on learnings: "Config resolution must follow:
env > DB > defaultwith per-subsystemresolve()methods".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/runtime.rs` around lines 226 - 231, The reload merge is currently using current.as_ref().listen_only_mode as the final fallback which preserves stale runtime state; update the closure inside channel_config.rcu so next.listen_only_mode is set to configured_listen_only.or(persisted_listen_only).or(Some(resolved_channel.listen_only_mode)).unwrap() (i.e., prefer configured > persisted > resolved default) instead of falling back to current.as_ref().listen_only_mode; keep the rest of the closure (Arc::new(next)) unchanged and reference the variables configured_listen_only, persisted_listen_only, resolved_channel, and current.as_ref().listen_only_mode to locate the change.src/agent/channel.rs (1)
602-619:⚠️ Potential issue | 🟡 Minor
/quietand/activecan still report the wrong effective mode when listen-only is explicitly pinned.These responses only use
persisted, butsync_listen_only_mode_from_runtime()will restore the explicit runtime/env value on the next turn. This can acknowledge a mode change that is not actually effective.Suggested patch
"/quiet" => { + let explicitly_configured = + (**self.deps.runtime_config.channel_listen_only_explicit.load()).is_some(); let persisted = self.set_listen_only_mode(true); - let body = if persisted { + self.sync_listen_only_mode_from_runtime(); + let body = if explicitly_configured { + "quiet mode is pinned by runtime/env config for this channel; toggle saved but not currently effective.".to_string() + } else if persisted { "quiet mode enabled. i'll only reply to commands, `@mentions`, or replies to my message." .to_string() } else { @@ "/active" => { + let explicitly_configured = + (**self.deps.runtime_config.channel_listen_only_explicit.load()).is_some(); let persisted = self.set_listen_only_mode(false); - let body = if persisted { + self.sync_listen_only_mode_from_runtime(); + let body = if explicitly_configured { + "active/quiet mode is pinned by runtime/env config for this channel; toggle saved but not currently effective.".to_string() + } else if persisted { "active mode enabled. i'll respond normally in this chat.".to_string() } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 602 - 619, The responses for "/quiet" and "/active" currently build their reply using only the boolean returned by set_listen_only_mode (persisted) which can be misleading if a runtime/env pin (reconciled by sync_listen_only_mode_from_runtime) will immediately override the change; after calling set_listen_only_mode(true/false) call sync_listen_only_mode_from_runtime() or read the authoritative current flag (e.g., get_listen_only_mode or the internal state updated by sync_listen_only_mode_from_runtime) and use that effective mode value (not just persisted) when composing the body passed to send_builtin_text so the message accurately reflects the actual effective mode.
🧹 Nitpick comments (2)
src/api/agents.rs (1)
680-682:explicit_listen_onlyis currently alwaysNonein this path.Line 680 derives from
raw_config.channel, but this handler setsraw_config.channeltoNoneon Line 565. Consider inliningNoneuntilCreateAgentRequestcarries channel settings.Small clarity cleanup
- let explicit_listen_only = raw_config.channel.map(|channel| channel.listen_only_mode); - runtime_config.set_settings(settings_store.clone(), explicit_listen_only); + runtime_config.set_settings(settings_store.clone(), None);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/agents.rs` around lines 680 - 682, The computed explicit_listen_only is always None here because raw_config.channel was set to None earlier; replace the map expression with an explicit None and call runtime_config.set_settings(settings_store.clone(), None) (or otherwise propagate channel data from CreateAgentRequest once it carries channel settings). Update the use of explicit_listen_only (symbol) so it no longer relies on raw_config.channel being present and keep runtime_config.set_settings and CreateAgentRequest changes in sync.src/settings/store.rs (1)
171-185: Consider deduplicating bool-setting decode logic.Both getters implement the same read/parse/error mapping flow. A small private helper would reduce drift risk.
♻️ Suggested refactor
+ fn get_optional_bool(&self, key: &str) -> Result<Option<bool>> { + match self.get_raw(key) { + Ok(raw) => raw.parse::<bool>().map(Some).map_err(|error| { + SettingsError::ReadFailed { + key: key.to_string(), + details: format!("invalid boolean value '{raw}': {error}"), + } + .into() + }), + Err(crate::error::Error::Settings(settings_error)) => match *settings_error { + SettingsError::NotFound { .. } => Ok(None), + other => Err(other.into()), + }, + Err(other) => Err(other), + } + } + pub fn channel_listen_only_mode(&self) -> Result<Option<bool>> { - match self.get_raw(CHANNEL_LISTEN_ONLY_MODE_KEY) { - ... - } + self.get_optional_bool(CHANNEL_LISTEN_ONLY_MODE_KEY) } pub fn channel_listen_only_mode_for(&self, channel_id: &str) -> Result<Option<bool>> { let key = Self::channel_listen_only_mode_key(channel_id); - match self.get_raw(&key) { - ... - } + self.get_optional_bool(&key) }Also applies to: 197-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/settings/store.rs` around lines 171 - 185, The channel_listen_only_mode getter duplicates the read/parse/error mapping logic; add a small private helper (e.g., a method like parse_optional_bool_setting or parse_bool_setting) that takes the setting key (&str) and returns Result<Option<bool>>, calling get_raw(key), parsing with raw.parse::<bool>(), and mapping parse errors into SettingsError::ReadFailed referencing the key and raw value; then replace channel_listen_only_mode (and the other similar getter) to call this helper, keeping existing error types (SettingsError::NotFound => Ok(None)) and preserving the same error conversion behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/agent/channel.rs`:
- Around line 602-619: The responses for "/quiet" and "/active" currently build
their reply using only the boolean returned by set_listen_only_mode (persisted)
which can be misleading if a runtime/env pin (reconciled by
sync_listen_only_mode_from_runtime) will immediately override the change; after
calling set_listen_only_mode(true/false) call
sync_listen_only_mode_from_runtime() or read the authoritative current flag
(e.g., get_listen_only_mode or the internal state updated by
sync_listen_only_mode_from_runtime) and use that effective mode value (not just
persisted) when composing the body passed to send_builtin_text so the message
accurately reflects the actual effective mode.
In `@src/config/runtime.rs`:
- Around line 226-231: The reload merge is currently using
current.as_ref().listen_only_mode as the final fallback which preserves stale
runtime state; update the closure inside channel_config.rcu so
next.listen_only_mode is set to
configured_listen_only.or(persisted_listen_only).or(Some(resolved_channel.listen_only_mode)).unwrap()
(i.e., prefer configured > persisted > resolved default) instead of falling back
to current.as_ref().listen_only_mode; keep the rest of the closure
(Arc::new(next)) unchanged and reference the variables configured_listen_only,
persisted_listen_only, resolved_channel, and current.as_ref().listen_only_mode
to locate the change.
---
Nitpick comments:
In `@src/api/agents.rs`:
- Around line 680-682: The computed explicit_listen_only is always None here
because raw_config.channel was set to None earlier; replace the map expression
with an explicit None and call
runtime_config.set_settings(settings_store.clone(), None) (or otherwise
propagate channel data from CreateAgentRequest once it carries channel
settings). Update the use of explicit_listen_only (symbol) so it no longer
relies on raw_config.channel being present and keep runtime_config.set_settings
and CreateAgentRequest changes in sync.
In `@src/settings/store.rs`:
- Around line 171-185: The channel_listen_only_mode getter duplicates the
read/parse/error mapping logic; add a small private helper (e.g., a method like
parse_optional_bool_setting or parse_bool_setting) that takes the setting key
(&str) and returns Result<Option<bool>>, calling get_raw(key), parsing with
raw.parse::<bool>(), and mapping parse errors into SettingsError::ReadFailed
referencing the key and raw value; then replace channel_listen_only_mode (and
the other similar getter) to call this helper, keeping existing error types
(SettingsError::NotFound => Ok(None)) and preserving the same error conversion
behavior.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8bf3cc00-364c-4cfc-bd66-e02229aaf6f6
📒 Files selected for processing (14)
src/agent/channel.rssrc/api/agents.rssrc/config/load.rssrc/config/runtime.rssrc/config/toml_schema.rssrc/config/types.rssrc/main.rssrc/messaging/discord.rssrc/messaging/slack.rssrc/messaging/twitch.rssrc/settings.rssrc/settings/store.rssrc/tools.rssrc/tools/reply.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tools.rs
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/config/runtime.rs (1)
226-231:⚠️ Potential issue | 🟠 MajorReload fallback can mask updated defaults when both configured and DB values are absent.
At Line 230, falling back to
current.as_ref().listen_only_modemeans a newly changed default from config may not take effect after reload.🔧 Proposed fix
- self.channel_config.rcu(move |current| { + self.channel_config.rcu(move |_current| { let mut next = resolved_channel; next.listen_only_mode = configured_listen_only .or(persisted_listen_only) - .unwrap_or(current.as_ref().listen_only_mode); + .unwrap_or(resolved_channel.listen_only_mode); Arc::new(next) });Based on learnings: "Config resolution must follow:
env > DB > defaultwith per-subsystemresolve()methods".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/runtime.rs` around lines 226 - 231, The closure passed to channel_config.rcu is falling back to the existing loaded value (current.as_ref().listen_only_mode) which masks newly computed defaults; change the fallback to use the resolved/default value from resolved_channel (or next) instead. In other words, inside the channel_config.rcu closure (where next is initialized from resolved_channel and listen_only_mode is set), replace unwrap_or(current.as_ref().listen_only_mode) with unwrap_or(next.listen_only_mode) (or resolved_channel.listen_only_mode) so resolution follows env > DB > default as intended.src/agent/channel.rs (1)
602-620:⚠️ Potential issue | 🟡 Minor
/quietand/activecan still misreport effective mode when runtime/env is explicitly pinned.At Line [603] and Line [614], response text only branches on persistence success. But
sync_listen_only_mode_from_runtime()(Line [331]) will restore the explicit runtime mode on the next turn, so users can see “enabled” even when it is not currently effective.Suggested adjustment
"/quiet" => { + let explicitly_configured = + (**self.deps.runtime_config.channel_listen_only_explicit.load()).is_some(); let persisted = self.set_listen_only_mode(true); - let body = if persisted { + let body = match (explicitly_configured, persisted) { + (true, true) => "quiet mode is pinned by runtime/env config for this channel; toggle was saved but is not currently effective.".to_string(), + (true, false) => "quiet mode is pinned by runtime/env config for this channel; toggle is session-only and not currently effective.".to_string(), + (false, true) => "quiet mode enabled. i'll only reply to commands, `@mentions`, or replies to my message.".to_string(), + (false, false) => "quiet mode enabled for this session, but persistence failed; it may revert after restart.".to_string(), + }; - "quiet mode enabled. i'll only reply to commands, `@mentions`, or replies to my message." - .to_string() - } else { - "quiet mode enabled for this session, but persistence failed; it may revert after restart.".to_string() - }; self.send_builtin_text(body, "quiet").await; return Ok(true); } "/active" => { + let explicitly_configured = + (**self.deps.runtime_config.channel_listen_only_explicit.load()).is_some(); let persisted = self.set_listen_only_mode(false); - let body = if persisted { - "active mode enabled. i'll respond normally in this chat.".to_string() - } else { - "active mode enabled for this session, but persistence failed; it may revert after restart.".to_string() - }; + let body = match (explicitly_configured, persisted) { + (true, true) => "active mode is pinned by runtime/env config for this channel; toggle was saved but is not currently effective.".to_string(), + (true, false) => "active mode is pinned by runtime/env config for this channel; toggle is session-only and not currently effective.".to_string(), + (false, true) => "active mode enabled. i'll respond normally in this chat.".to_string(), + (false, false) => "active mode enabled for this session, but persistence failed; it may revert after restart.".to_string(), + }; self.send_builtin_text(body, "active").await; return Ok(true); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 602 - 620, The reply messages for "/quiet" and "/active" currently only consider whether persistence succeeded (set_listen_only_mode) and can misreport the actual effective mode when the runtime/env is explicitly pinned; call or consult sync_listen_only_mode_from_runtime() (or the runtime pin flag it uses) after set_listen_only_mode to determine the true effective mode and use that to build the body text passed to send_builtin_text, while still including the persisted boolean in a secondary clause when persistence failed; update the branches in the command handlers (the "/quiet" and "/active" blocks) to reflect the effective mode from sync_listen_only_mode_from_runtime() rather than relying solely on persisted.
🧹 Nitpick comments (1)
src/main.rs (1)
2165-2170: Optional: precompute explicit listen-only lookups once.This per-agent
.find(...)adds an extra linear scan in the initialization loop. A small precomputed map would reduce startup-time overhead when agent counts grow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 2165 - 2170, The per-agent linear scan uses config.agents.iter().find(...) to compute explicit_listen_only; precompute a lookup map (e.g., HashMap<AgentId, bool/Option<bool>>) from config.agents mapping agent.id -> channel.listen_only_mode before the agent initialization loop, then replace the find call with a constant-time lookup using agent_config.id and pass the resulting value into runtime_config.set_settings(settings_store.clone(), explicit_listen_only); this removes the repeated scan and reduces startup overhead as agent counts grow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/agent/channel.rs`:
- Around line 602-620: The reply messages for "/quiet" and "/active" currently
only consider whether persistence succeeded (set_listen_only_mode) and can
misreport the actual effective mode when the runtime/env is explicitly pinned;
call or consult sync_listen_only_mode_from_runtime() (or the runtime pin flag it
uses) after set_listen_only_mode to determine the true effective mode and use
that to build the body text passed to send_builtin_text, while still including
the persisted boolean in a secondary clause when persistence failed; update the
branches in the command handlers (the "/quiet" and "/active" blocks) to reflect
the effective mode from sync_listen_only_mode_from_runtime() rather than relying
solely on persisted.
In `@src/config/runtime.rs`:
- Around line 226-231: The closure passed to channel_config.rcu is falling back
to the existing loaded value (current.as_ref().listen_only_mode) which masks
newly computed defaults; change the fallback to use the resolved/default value
from resolved_channel (or next) instead. In other words, inside the
channel_config.rcu closure (where next is initialized from resolved_channel and
listen_only_mode is set), replace unwrap_or(current.as_ref().listen_only_mode)
with unwrap_or(next.listen_only_mode) (or resolved_channel.listen_only_mode) so
resolution follows env > DB > default as intended.
---
Nitpick comments:
In `@src/main.rs`:
- Around line 2165-2170: The per-agent linear scan uses
config.agents.iter().find(...) to compute explicit_listen_only; precompute a
lookup map (e.g., HashMap<AgentId, bool/Option<bool>>) from config.agents
mapping agent.id -> channel.listen_only_mode before the agent initialization
loop, then replace the find call with a constant-time lookup using
agent_config.id and pass the resulting value into
runtime_config.set_settings(settings_store.clone(), explicit_listen_only); this
removes the repeated scan and reduces startup overhead as agent counts grow.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 30711d7a-f749-4e47-a1b0-630e63ecef58
📒 Files selected for processing (14)
src/agent/channel.rssrc/api/agents.rssrc/config/load.rssrc/config/runtime.rssrc/config/toml_schema.rssrc/config/types.rssrc/main.rssrc/messaging/discord.rssrc/messaging/slack.rssrc/messaging/twitch.rssrc/settings.rssrc/settings/store.rssrc/tools.rssrc/tools/reply.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/messaging/slack.rs
- src/tools.rs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/agent/channel.rs (1)
311-315:⚠️ Potential issue | 🟡 Minor
/quietand/activecan still report a mode that is not the effective runtime mode.Line 312 applies
listen_only_session_overridebefore checking explicit config, while Line 603 and Line 614 build responses frompersistedonly. In explicitly pinned setups, user-facing mode confirmations can drift from what will be applied on the next sync.Proposed adjustment
fn sync_listen_only_mode_from_runtime(&mut self) { + let runtime_default = self + .deps + .runtime_config + .channel_config + .load() + .listen_only_mode; + let explicit_listen_only = **self.deps.runtime_config.channel_listen_only_explicit.load(); + if explicit_listen_only.is_some() { + self.listen_only_session_override = None; + self.listen_only_mode = runtime_default; + return; + } if let Some(override_mode) = self.listen_only_session_override { self.listen_only_mode = override_mode; return; } - let runtime_default = self - .deps - .runtime_config - .channel_config - .load() - .listen_only_mode; - let explicit_listen_only = **self.deps.runtime_config.channel_listen_only_explicit.load(); @@ "/quiet" => { + let explicitly_configured = + (**self.deps.runtime_config.channel_listen_only_explicit.load()).is_some(); let persisted = self.set_listen_only_mode(true); - let body = if persisted { + let body = if explicitly_configured { + "quiet mode is pinned by runtime/env config for this channel; chat toggle may be stored but is not currently effective.".to_string() + } else if persisted { "quiet mode enabled. i'll only reply to commands, `@mentions`, or replies to my message." .to_string() } else { @@ "/active" => { + let explicitly_configured = + (**self.deps.runtime_config.channel_listen_only_explicit.load()).is_some(); let persisted = self.set_listen_only_mode(false); - let body = if persisted { + let body = if explicitly_configured { + "active mode is pinned by runtime/env config for this channel; chat toggle may be stored but is not currently effective.".to_string() + } else if persisted { "active mode enabled. i'll respond normally in this chat.".to_string() } else {Also applies to: 331-333, 602-620
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 311 - 315, The code applies listen_only_session_override before considering the explicit persisted config, causing user-facing confirmations to mismatch the actual runtime mode; create a single helper (e.g., get_effective_listen_only_mode) that computes the effective mode by reconciling persisted (explicit/pinned) config and listen_only_session_override with a clear precedence rule (persisted explicit/pinned should override session override when present, otherwise honor the session override), update sync_listen_only_mode_from_runtime to set self.listen_only_mode from that helper, and make the response builders that currently read persisted (places building confirmations) use the same helper so confirmations always reflect the effective runtime mode.src/config/runtime.rs (1)
226-231:⚠️ Potential issue | 🟠 MajorReload fallback currently preserves stale runtime value instead of resolved default.
Line 230 falls back to
current.as_ref().listen_only_mode, so when both configured and persisted values are absent, updated defaults from reload may never apply. This breaks the intendedconfigured/env > persisted(DB) > defaultorder.🔧 Suggested fix
self.channel_config.rcu(move |current| { let mut next = resolved_channel; next.listen_only_mode = configured_listen_only .or(persisted_listen_only) - .unwrap_or(current.as_ref().listen_only_mode); + .unwrap_or(next.listen_only_mode); Arc::new(next) });Based on learnings: "Config resolution must follow:
env > DB > defaultwith per-subsystemresolve()methods".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/runtime.rs` around lines 226 - 231, The reload fallback is using the current runtime value (current.as_ref().listen_only_mode) instead of the resolved default, preventing updated defaults from applying; inside the channel_config.rcu closure (the block that defines let mut next = resolved_channel), change the fallback for next.listen_only_mode so it uses the resolved default (from resolved_channel) rather than current.as_ref(); i.e. compute next.listen_only_mode = configured_listen_only.or(persisted_listen_only).unwrap_or(resolved_channel.listen_only_mode) so resolution follows env/config > DB > default (and ensure other subsystem resolve() patterns follow the same change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/agent/channel.rs`:
- Around line 311-315: The code applies listen_only_session_override before
considering the explicit persisted config, causing user-facing confirmations to
mismatch the actual runtime mode; create a single helper (e.g.,
get_effective_listen_only_mode) that computes the effective mode by reconciling
persisted (explicit/pinned) config and listen_only_session_override with a clear
precedence rule (persisted explicit/pinned should override session override when
present, otherwise honor the session override), update
sync_listen_only_mode_from_runtime to set self.listen_only_mode from that
helper, and make the response builders that currently read persisted (places
building confirmations) use the same helper so confirmations always reflect the
effective runtime mode.
In `@src/config/runtime.rs`:
- Around line 226-231: The reload fallback is using the current runtime value
(current.as_ref().listen_only_mode) instead of the resolved default, preventing
updated defaults from applying; inside the channel_config.rcu closure (the block
that defines let mut next = resolved_channel), change the fallback for
next.listen_only_mode so it uses the resolved default (from resolved_channel)
rather than current.as_ref(); i.e. compute next.listen_only_mode =
configured_listen_only.or(persisted_listen_only).unwrap_or(resolved_channel.listen_only_mode)
so resolution follows env/config > DB > default (and ensure other subsystem
resolve() patterns follow the same change).
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fe7beadd-65bc-46a2-8c6e-af4a901e1ab7
📒 Files selected for processing (14)
src/agent/channel.rssrc/api/agents.rssrc/config/load.rssrc/config/runtime.rssrc/config/toml_schema.rssrc/config/types.rssrc/main.rssrc/messaging/discord.rssrc/messaging/slack.rssrc/messaging/twitch.rssrc/settings.rssrc/settings/store.rssrc/tools.rssrc/tools/reply.rs
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/agent/channel.rs (2)
500-520:⚠️ Potential issue | 🟠 MajorTelegram reply invoke check is too strict when username metadata is missing.
Current logic requires both
reply_to_usernameandtelegram_bot_username; if either is absent, replies to the bot can be treated as non-invokes in listen-only mode.Suggested fix
"telegram" => { let reply_to_is_bot = message .metadata .get("reply_to_is_bot") .and_then(|v| v.as_bool()) .unwrap_or(false); let bot_username = message .metadata .get("telegram_bot_username") .and_then(|v| v.as_str()) .map(str::to_lowercase); let reply_username = message .metadata .get("reply_to_username") .and_then(|v| v.as_str()) .map(str::to_lowercase); - reply_to_is_bot - && reply_username - .zip(bot_username) - .is_some_and(|(reply, bot)| bot == reply) + let reply_to_matches_this_bot = match (reply_username.as_deref(), bot_username.as_deref()) { + (Some(reply), Some(bot)) => reply == bot, + _ => true, + }; + reply_to_is_bot && reply_to_matches_this_bot }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 500 - 520, The telegram branch currently requires both reply_to_is_bot and matching reply/usernames to consider a reply an invoke; change the logic to treat reply_to_is_bot as sufficient when the username metadata is missing by returning true if reply_to_is_bot is true, otherwise fall back to comparing reply_username and bot_username; update the expression using the local variables reply_to_is_bot, bot_username, and reply_username in the "telegram" match arm so it returns reply_to_is_bot || (reply_username.zip(bot_username).is_some_and(|(reply, bot)| bot == reply)).
602-620:⚠️ Potential issue | 🟡 Minor
/quietand/activecan acknowledge a mode that is not currently effective.When listen-only is explicitly pinned via runtime/env config, these handlers report success based on persistence only, which can mislead users about actual active mode.
Suggested fix
"/quiet" => { + let explicitly_configured = + (**self.deps.runtime_config.channel_listen_only_explicit.load()).is_some(); + let effective_mode = self.deps.runtime_config.channel_config.load().listen_only_mode; let persisted = self.set_listen_only_mode(true); - let body = if persisted { + let body = if explicitly_configured && !effective_mode { + "quiet mode is pinned by runtime/env config for this channel; your toggle was saved but is not currently effective.".to_string() + } else if persisted { "quiet mode enabled. i'll only reply to commands, `@mentions`, or replies to my message." .to_string() } else { "quiet mode enabled for this session, but persistence failed; it may revert after restart.".to_string() }; self.send_builtin_text(body, "quiet").await; return Ok(true); } "/active" => { + let explicitly_configured = + (**self.deps.runtime_config.channel_listen_only_explicit.load()).is_some(); + let effective_mode = self.deps.runtime_config.channel_config.load().listen_only_mode; let persisted = self.set_listen_only_mode(false); - let body = if persisted { + let body = if explicitly_configured && effective_mode { + "active mode is pinned by runtime/env config for this channel; your toggle was saved but is not currently effective.".to_string() + } else if persisted { "active mode enabled. i'll respond normally in this chat.".to_string() } else { "active mode enabled for this session, but persistence failed; it may revert after restart.".to_string() }; self.send_builtin_text(body, "active").await; return Ok(true); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 602 - 620, The handlers for "/quiet" and "/active" currently base their user message solely on the boolean returned by set_listen_only_mode (persistence) and can therefore claim a mode is enabled even when a runtime/env pin prevents it from taking effect; update these branches (the code that calls set_listen_only_mode(...) and then send_builtin_text(...)) to query the actual effective listen-only state after attempting to persist (e.g., call an existing checker like is_listen_only_pinned or get_effective_listen_only(), or add a small helper that reads the runtime/env pin) and compose the response based on that effective state (and include persistence failure detail if needed) before calling send_builtin_text so the acknowledgement reflects the real active mode.
🧹 Nitpick comments (1)
src/main.rs (1)
2165-2170: Optional: precompute explicit listen-only lookup once.This works, but Line 2165 currently scans
config.agentsfor each resolved agent. Precomputing a map before the loop avoids O(n²) behavior when agent count grows.♻️ Proposed refactor
+ let explicit_listen_only_by_agent: std::collections::HashMap<String, bool> = config + .agents + .iter() + .filter_map(|agent| { + agent + .channel + .map(|channel_config| (agent.id.clone(), channel_config.listen_only_mode)) + }) + .collect(); for agent_config in &resolved_agents { @@ - let explicit_listen_only = config - .agents - .iter() - .find(|agent| agent.id == agent_config.id) - .and_then(|agent| agent.channel.map(|channel| channel.listen_only_mode)); + let explicit_listen_only = explicit_listen_only_by_agent + .get(&agent_config.id) + .copied(); runtime_config.set_settings(settings_store.clone(), explicit_listen_only);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 2165 - 2170, The current code repeatedly scans config.agents inside the agent loop to compute explicit_listen_only (using config.agents.iter().find(|agent| agent.id == agent_config.id).and_then(|agent| agent.channel.map(|channel| channel.listen_only_mode))), causing O(n²) cost; precompute a HashMap (or similar) from agent id to channel.listen_only_mode once before iterating agents, then replace the repeated find with a simple lookup by agent_config.id and pass that result to runtime_config.set_settings(settings_store.clone(), explicit_listen_only) to avoid repeated scans; refer to config.agents, agent_config.id, explicit_listen_only, runtime_config.set_settings, and settings_store when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/agent/channel.rs`:
- Around line 500-520: The telegram branch currently requires both
reply_to_is_bot and matching reply/usernames to consider a reply an invoke;
change the logic to treat reply_to_is_bot as sufficient when the username
metadata is missing by returning true if reply_to_is_bot is true, otherwise fall
back to comparing reply_username and bot_username; update the expression using
the local variables reply_to_is_bot, bot_username, and reply_username in the
"telegram" match arm so it returns reply_to_is_bot ||
(reply_username.zip(bot_username).is_some_and(|(reply, bot)| bot == reply)).
- Around line 602-620: The handlers for "/quiet" and "/active" currently base
their user message solely on the boolean returned by set_listen_only_mode
(persistence) and can therefore claim a mode is enabled even when a runtime/env
pin prevents it from taking effect; update these branches (the code that calls
set_listen_only_mode(...) and then send_builtin_text(...)) to query the actual
effective listen-only state after attempting to persist (e.g., call an existing
checker like is_listen_only_pinned or get_effective_listen_only(), or add a
small helper that reads the runtime/env pin) and compose the response based on
that effective state (and include persistence failure detail if needed) before
calling send_builtin_text so the acknowledgement reflects the real active mode.
---
Nitpick comments:
In `@src/main.rs`:
- Around line 2165-2170: The current code repeatedly scans config.agents inside
the agent loop to compute explicit_listen_only (using
config.agents.iter().find(|agent| agent.id == agent_config.id).and_then(|agent|
agent.channel.map(|channel| channel.listen_only_mode))), causing O(n²) cost;
precompute a HashMap (or similar) from agent id to channel.listen_only_mode once
before iterating agents, then replace the repeated find with a simple lookup by
agent_config.id and pass that result to
runtime_config.set_settings(settings_store.clone(), explicit_listen_only) to
avoid repeated scans; refer to config.agents, agent_config.id,
explicit_listen_only, runtime_config.set_settings, and settings_store when
making the change.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53517576-d4b5-473f-b756-c259c2fb8a28
📒 Files selected for processing (14)
src/agent/channel.rssrc/api/agents.rssrc/config/load.rssrc/config/runtime.rssrc/config/toml_schema.rssrc/config/types.rssrc/main.rssrc/messaging/discord.rssrc/messaging/slack.rssrc/messaging/twitch.rssrc/settings.rssrc/settings/store.rssrc/tools.rssrc/tools/reply.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/messaging/discord.rs
- src/tools.rs
- src/messaging/slack.rs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/config/runtime.rs (1)
226-231:⚠️ Potential issue | 🟠 MajorReload merge fallback can mask new default listen-only values.
At
Line [230], falling back tocurrent.as_ref().listen_only_modemeans config default changes inresolved_channel.listen_only_modemay never apply when both configured and persisted values are absent.Suggested fix
- self.channel_config.rcu(move |current| { + let resolved_default_listen_only = resolved_channel.listen_only_mode; + self.channel_config.rcu(move |_current| { let mut next = resolved_channel; next.listen_only_mode = configured_listen_only .or(persisted_listen_only) - .unwrap_or(current.as_ref().listen_only_mode); + .unwrap_or(resolved_default_listen_only); Arc::new(next) });Based on learnings: "Config resolution must follow:
env > DB > defaultwith per-subsystemresolve()methods".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/runtime.rs` around lines 226 - 231, The fallback to current.as_ref().listen_only_mode inside the channel_config.rcu closure masks updated defaults in resolved_channel; change the merge logic in the closure that sets next.listen_only_mode (where resolved_channel, configured_listen_only, persisted_listen_only are used) so that when both configured_listen_only and persisted_listen_only are None it falls back to resolved_channel.listen_only_mode (the new default) instead of current.as_ref().listen_only_mode; keep the same pattern for other per-subsystem resolves to ensure env > DB > default ordering.src/agent/channel.rs (1)
602-619:⚠️ Potential issue | 🟡 Minor
/quietand/activecan still misreport effective mode under explicit runtime pinning.At
Line [603]andLine [614], response text depends only onpersisted. Ifchannel_listen_only_explicitis set,sync_listen_only_mode_from_runtime(seeLine [331]) will restore configured mode on the next sync, so the command can acknowledge a mode that is not currently effective.Suggested fix
"/quiet" => { + let explicitly_configured = + (**self.deps.runtime_config.channel_listen_only_explicit.load()).is_some(); let persisted = self.set_listen_only_mode(true); - let body = if persisted { + let body = if explicitly_configured { + "quiet mode is pinned by runtime/env config for this channel; toggle was persisted but is not currently effective.".to_string() + } else if persisted { "quiet mode enabled. i'll only reply to commands, `@mentions`, or replies to my message." .to_string() } else { @@ "/active" => { + let explicitly_configured = + (**self.deps.runtime_config.channel_listen_only_explicit.load()).is_some(); let persisted = self.set_listen_only_mode(false); - let body = if persisted { + let body = if explicitly_configured { + "active mode is pinned by runtime/env config for this channel; toggle was persisted but is not currently effective.".to_string() + } else if persisted { "active mode enabled. i'll respond normally in this chat.".to_string() } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 602 - 619, The response messages for the "/quiet" and "/active" branches only reflect whether persistence succeeded (persisted) and can misreport the actual or eventual mode when a runtime pin is set; after calling set_listen_only_mode(...) compute the effective state and whether the runtime pin (channel_listen_only_explicit) will override it (see sync_listen_only_mode_from_runtime) and update the body passed to send_builtin_text(...) to accurately state both the current effective mode and, if channel_listen_only_explicit is true, that the mode will be restored/overridden on the next sync; use the instance flags/state (or a helper like an effective_listen_only() query) and mention the persistence result in the message accordingly.
🧹 Nitpick comments (3)
src/messaging/discord.rs (1)
959-966: Optional: compute split booleans once before metadata inserts.Current logic is correct; this just avoids repeated helper calls and keeps combined/split fields obviously in sync from one source.
♻️ Suggested refactor
- metadata.insert( - "discord_mentions_or_replies_to_bot".into(), - is_mention_or_reply_to_bot(message, bot_user_id).into(), - ); - metadata.insert( - "discord_mentioned_bot".into(), - is_mention_to_bot(message, bot_user_id).into(), - ); - metadata.insert( - "discord_reply_to_bot".into(), - is_reply_to_bot(message, bot_user_id).into(), - ); + let discord_mentioned_bot = is_mention_to_bot(message, bot_user_id); + let discord_reply_to_bot = is_reply_to_bot(message, bot_user_id); + metadata.insert("discord_mentioned_bot".into(), discord_mentioned_bot.into()); + metadata.insert("discord_reply_to_bot".into(), discord_reply_to_bot.into()); + metadata.insert( + "discord_mentions_or_replies_to_bot".into(), + (discord_mentioned_bot || discord_reply_to_bot).into(), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/discord.rs` around lines 959 - 966, Compute the boolean results once before inserting into metadata to avoid duplicate helper calls and keep the derived fields in sync: call is_mention_to_bot(message, bot_user_id) and is_reply_to_bot(message, bot_user_id) into local variables (e.g., mentioned and replied), then use those variables in the two metadata.insert calls instead of calling the helpers twice; update any related combined/split fields to reference these same locals so they remain consistent.src/messaging/slack.rs (1)
272-311: Short-circuit reply lookup when the bot is already mentioned.You can skip
conversations_replies(and session creation) whenmentioned_botistrue; this reduces Slack API load on hot paths.♻️ Proposed refactor
- let token = SlackApiToken::new(SlackApiTokenValue(adapter_state.bot_token.clone())); - let session = client.open_session(&token); - let replied_to_bot = if let Some(thread_ts) = msg_event.origin.thread_ts.as_ref() { + let replied_to_bot = if mentioned_bot { + false + } else if let Some(thread_ts) = msg_event.origin.thread_ts.as_ref() { // For threaded replies, treat as explicit invoke only when the thread // root message belongs to this bot. if thread_ts.0 != ts { + let token = SlackApiToken::new(SlackApiTokenValue(adapter_state.bot_token.clone())); + let session = client.open_session(&token); let thread_replies_request = SlackApiConversationsRepliesRequest::new( SlackChannelId(channel_id.clone()), thread_ts.clone(), ) .with_limit(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/slack.rs` around lines 272 - 311, The reply-thread lookup and session creation should be skipped when mentioned_bot is already true: move the check for mentioned_bot before creating SlackApiToken/SlackApiTokenValue and before calling client.open_session or session.conversations_replies; only construct token and open a session and call conversations_replies inside the branch where msg_event.origin.thread_ts is Some(...) AND mentioned_bot is false, so replied_to_bot is computed via the existing timeout/conversations_replies logic only when needed (leave the existing error handling and timeouts intact for the conversations_replies call).src/settings/store.rs (1)
171-219: Consider extracting a shared bool-setting reader helper.
channel_listen_only_modeandchannel_listen_only_mode_forduplicate the same parse/error mapping logic; a small helper would reduce drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/settings/store.rs` around lines 171 - 219, Extract the duplicated parsing and SettingsError handling into a private helper like read_bool_setting(&self, key: &str) -> Result<Option<bool>> that calls self.get_raw(key), maps Ok(raw) to raw.parse::<bool>().map(Some).map_err(...) using the same SettingsError::ReadFailed construction (include key and formatted error), treats Error::Settings(SettingsError::NotFound{..}) as Ok(None), and forwards other errors; then replace the bodies of channel_listen_only_mode and channel_listen_only_mode_for to compute their key (for the former use CHANNEL_LISTEN_ONLY_MODE_KEY, for the latter Self::channel_listen_only_mode_key(channel_id)) and return read_bool_setting(&key). Ensure the helper clones/borrows key appropriately so existing error messages still include the key string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/agent/channel.rs`:
- Around line 602-619: The response messages for the "/quiet" and "/active"
branches only reflect whether persistence succeeded (persisted) and can
misreport the actual or eventual mode when a runtime pin is set; after calling
set_listen_only_mode(...) compute the effective state and whether the runtime
pin (channel_listen_only_explicit) will override it (see
sync_listen_only_mode_from_runtime) and update the body passed to
send_builtin_text(...) to accurately state both the current effective mode and,
if channel_listen_only_explicit is true, that the mode will be
restored/overridden on the next sync; use the instance flags/state (or a helper
like an effective_listen_only() query) and mention the persistence result in the
message accordingly.
In `@src/config/runtime.rs`:
- Around line 226-231: The fallback to current.as_ref().listen_only_mode inside
the channel_config.rcu closure masks updated defaults in resolved_channel;
change the merge logic in the closure that sets next.listen_only_mode (where
resolved_channel, configured_listen_only, persisted_listen_only are used) so
that when both configured_listen_only and persisted_listen_only are None it
falls back to resolved_channel.listen_only_mode (the new default) instead of
current.as_ref().listen_only_mode; keep the same pattern for other per-subsystem
resolves to ensure env > DB > default ordering.
---
Nitpick comments:
In `@src/messaging/discord.rs`:
- Around line 959-966: Compute the boolean results once before inserting into
metadata to avoid duplicate helper calls and keep the derived fields in sync:
call is_mention_to_bot(message, bot_user_id) and is_reply_to_bot(message,
bot_user_id) into local variables (e.g., mentioned and replied), then use those
variables in the two metadata.insert calls instead of calling the helpers twice;
update any related combined/split fields to reference these same locals so they
remain consistent.
In `@src/messaging/slack.rs`:
- Around line 272-311: The reply-thread lookup and session creation should be
skipped when mentioned_bot is already true: move the check for mentioned_bot
before creating SlackApiToken/SlackApiTokenValue and before calling
client.open_session or session.conversations_replies; only construct token and
open a session and call conversations_replies inside the branch where
msg_event.origin.thread_ts is Some(...) AND mentioned_bot is false, so
replied_to_bot is computed via the existing timeout/conversations_replies logic
only when needed (leave the existing error handling and timeouts intact for the
conversations_replies call).
In `@src/settings/store.rs`:
- Around line 171-219: Extract the duplicated parsing and SettingsError handling
into a private helper like read_bool_setting(&self, key: &str) ->
Result<Option<bool>> that calls self.get_raw(key), maps Ok(raw) to
raw.parse::<bool>().map(Some).map_err(...) using the same
SettingsError::ReadFailed construction (include key and formatted error), treats
Error::Settings(SettingsError::NotFound{..}) as Ok(None), and forwards other
errors; then replace the bodies of channel_listen_only_mode and
channel_listen_only_mode_for to compute their key (for the former use
CHANNEL_LISTEN_ONLY_MODE_KEY, for the latter
Self::channel_listen_only_mode_key(channel_id)) and return
read_bool_setting(&key). Ensure the helper clones/borrows key appropriately so
existing error messages still include the key string.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 914f12fe-47e3-4c07-9cfd-328f0d7846f4
📒 Files selected for processing (14)
src/agent/channel.rssrc/api/agents.rssrc/config/load.rssrc/config/runtime.rssrc/config/toml_schema.rssrc/config/types.rssrc/main.rssrc/messaging/discord.rssrc/messaging/slack.rssrc/messaging/twitch.rssrc/settings.rssrc/settings/store.rssrc/tools.rssrc/tools/reply.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/api/agents.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/agent/channel.rs (1)
311-315:⚠️ Potential issue | 🟠 MajorSession override currently bypasses explicit runtime/env pinning.
When
listen_only_session_overrideis set,sync_listen_only_mode_from_runtimereturns before checkingchannel_listen_only_explicit. A temporary persistence failure can therefore override an explicit configured mode until process restart or another manual toggle, and/quiet//activefeedback becomes misleading downstream.Suggested fix
fn sync_listen_only_mode_from_runtime(&mut self) { - if let Some(override_mode) = self.listen_only_session_override { - self.listen_only_mode = override_mode; - return; - } + let explicit_listen_only = **self.deps.runtime_config.channel_listen_only_explicit.load(); + if explicit_listen_only.is_some() { + self.listen_only_mode = self + .deps + .runtime_config + .channel_config + .load() + .listen_only_mode; + return; + } + if let Some(override_mode) = self.listen_only_session_override { + self.listen_only_mode = override_mode; + return; + } let runtime_default = self .deps .runtime_config .channel_config .load() .listen_only_mode; - let explicit_listen_only = **self.deps.runtime_config.channel_listen_only_explicit.load();Also applies to: 331-348
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 311 - 315, The current early return in sync_listen_only_mode_from_runtime when listen_only_session_override is set bypasses the explicit runtime/env pin (channel_listen_only_explicit); change the logic in sync_listen_only_mode_from_runtime (and the similar block around the other occurrence) to first check channel_listen_only_explicit and, if it is set, apply channel_listen_only_explicit to listen_only_mode, otherwise apply listen_only_session_override, and only if neither is present fall back to the runtime/env value — ensure you update the code paths using the symbols sync_listen_only_mode_from_runtime, listen_only_session_override, channel_listen_only_explicit, and listen_only_mode so explicit pinning always takes precedence over temporary session overrides.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agent/channel.rs`:
- Around line 1168-1173: The substring check in the looks_like_ping logic
wrongly matches words like “shipping”; update the detection to match whole words
only (case-insensitive) instead of using text.contains("ping") — e.g., use a
word-boundary regex or tokenize text and check tokens for
equals_ignore_ascii_case("ping") inside the looks_like_ping expression; apply
the same fix to the duplicate liveness-check block in the same file referenced
by the reviewer so both checks only trigger on standalone "ping".
---
Duplicate comments:
In `@src/agent/channel.rs`:
- Around line 311-315: The current early return in
sync_listen_only_mode_from_runtime when listen_only_session_override is set
bypasses the explicit runtime/env pin (channel_listen_only_explicit); change the
logic in sync_listen_only_mode_from_runtime (and the similar block around the
other occurrence) to first check channel_listen_only_explicit and, if it is set,
apply channel_listen_only_explicit to listen_only_mode, otherwise apply
listen_only_session_override, and only if neither is present fall back to the
runtime/env value — ensure you update the code paths using the symbols
sync_listen_only_mode_from_runtime, listen_only_session_override,
channel_listen_only_explicit, and listen_only_mode so explicit pinning always
takes precedence over temporary session overrides.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ceb2b016-1086-4ea0-80d0-30f8dadec6af
📒 Files selected for processing (2)
src/agent/channel.rssrc/tools.rs
Comment on lines
+1168
to
+1173
| let looks_like_ping = text.contains("you here") | ||
| || text.contains("ping") | ||
| || text.ends_with(" yo") | ||
| || text == "yo" | ||
| || text.contains("alive") | ||
| || text.contains("there?"); |
Contributor
There was a problem hiding this comment.
contains("ping") can hijack normal messages as liveness checks.
This substring match also catches words like “shipping”/“mapping”, which can trigger an early “yeah i'm here” and skip normal handling.
Suggested hardening
- let looks_like_ping = text.contains("you here")
- || text.contains("ping")
- || text.ends_with(" yo")
- || text == "yo"
- || text.contains("alive")
- || text.contains("there?");
+ let has_token = |needle: &str| {
+ text.split(|character: char| !(character.is_ascii_alphanumeric() || character == '_'))
+ .any(|token| token == needle)
+ };
+ let looks_like_ping = text.contains("you here")
+ || has_token("ping")
+ || has_token("yo")
+ || has_token("alive")
+ || text.contains("there?");Also applies to: 1189-1194
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/channel.rs` around lines 1168 - 1173, The substring check in the
looks_like_ping logic wrongly matches words like “shipping”; update the
detection to match whole words only (case-insensitive) instead of using
text.contains("ping") — e.g., use a word-boundary regex or tokenize text and
check tokens for equals_ignore_ascii_case("ping") inside the looks_like_ping
expression; apply the same fix to the duplicate liveness-check block in the same
file referenced by the reviewer so both checks only trigger on standalone
"ping".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds settings-backed listen-only mode, extends invoke detection to Slack/Twitch, and removes org-specific hardcoding.