fix: apply [agents.channel] config when spawning platform channels#550
fix: apply [agents.channel] config when spawning platform channels#550
Conversation
The agent_default argument to ResolvedConversationSettings::resolve() was always None for platform channels (Slack, Discord, etc.) and the idle-worker resume path, so [agents.channel] settings (response_mode, save_attachments) were loaded from TOML but silently discarded at runtime. Adds ChannelConfig::to_conversation_settings() to centralise the conversion, and passes the result as agent_default in all six resolve() call sites (new-message path and idle-worker resume path). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
WalkthroughA new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| pub fn to_conversation_settings( | ||
| &self, | ||
| ) -> Option<crate::conversation::ConversationSettings> { | ||
| if self.response_mode.is_none() && !self.save_attachments { |
There was a problem hiding this comment.
to_conversation_settings() currently only treats response_mode = None as “default”, so response_mode = "active" still injects a no-op layer. Also, when response_mode is set, this will always write save_attachments: Some(false) even though it's the default.
| if self.response_mode.is_none() && !self.save_attachments { | |
| let response_mode = self.response_mode.unwrap_or_default(); | |
| let save_attachments = self.save_attachments.then_some(true); | |
| if response_mode == crate::conversation::settings::ResponseMode::Active | |
| && save_attachments.is_none() | |
| { | |
| return None; | |
| } | |
| Some(crate::conversation::ConversationSettings { | |
| response_mode, | |
| save_attachments, | |
| ..Default::default() | |
| }) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main.rs (1)
1963-2020:⚠️ Potential issue | 🟠 MajorResume path still skips binding-level defaults.
Every branch here resolves with
Noneas the channel layer, while the normal spawn path later in this file passesbinding_settings.as_ref(). Once the resumed channel is inserted intoactive_channels, later inbound messages reuse it instead of re-resolving settings, so a restart can silently drop binding-levelresponse_mode/save_attachmentsdefaults for channels that have idle workers but no DB override. Please persist or reconstruct the matched binding settings before creating the resumed channel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 1963 - 2020, The resume branch currently calls ResolvedConversationSettings::resolve with None for the channel/binding settings, dropping binding-level defaults; locate the resume block that builds resolved_settings (the match on PortalConversationStore::get / ChannelSettingsStore::get) and reconstruct or look up the matched binding settings (the same binding_settings used in the normal spawn path and referenced later as binding_settings.as_ref()), then pass binding_settings.as_ref() into ResolvedConversationSettings::resolve instead of None for every branch (including the Ok(None) and Err branches), and ensure the resumed channel inserted into active_channels carries those resolved binding-level defaults.
🤖 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/config/types.rs`:
- Around line 955-970: The ChannelConfig::to_conversation_settings helper
currently ignores the deprecated listen_only_mode when deciding whether to
return None and when filling response_mode; compute an effective_response_mode
first (use self.response_mode if Some, otherwise set to listen-only mode when
self.listen_only_mode is true, otherwise default), use that to decide the early
return (return None only when effective_response_mode is default and
save_attachments is false), and set response_mode to that effective value when
constructing ConversationSettings (replace the current unwrap_or_default usage).
---
Outside diff comments:
In `@src/main.rs`:
- Around line 1963-2020: The resume branch currently calls
ResolvedConversationSettings::resolve with None for the channel/binding
settings, dropping binding-level defaults; locate the resume block that builds
resolved_settings (the match on PortalConversationStore::get /
ChannelSettingsStore::get) and reconstruct or look up the matched binding
settings (the same binding_settings used in the normal spawn path and referenced
later as binding_settings.as_ref()), then pass binding_settings.as_ref() into
ResolvedConversationSettings::resolve instead of None for every branch
(including the Ok(None) and Err branches), and ensure the resumed channel
inserted into active_channels carries those resolved binding-level defaults.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab726b17-f76a-4688-b043-a1794a2955f8
📒 Files selected for processing (2)
src/config/types.rssrc/main.rs
| impl ChannelConfig { | ||
| /// Convert to a `ConversationSettings` for use as the agent-level default in | ||
| /// `ResolvedConversationSettings::resolve`. Returns `None` when both fields | ||
| /// match their system defaults (avoids injecting a no-op layer). | ||
| pub fn to_conversation_settings( | ||
| &self, | ||
| ) -> Option<crate::conversation::ConversationSettings> { | ||
| if self.response_mode.is_none() && !self.save_attachments { | ||
| return None; | ||
| } | ||
| Some(crate::conversation::ConversationSettings { | ||
| response_mode: self.response_mode.unwrap_or_default(), | ||
| save_attachments: Some(self.save_attachments), | ||
| ..Default::default() | ||
| }) | ||
| } |
There was a problem hiding this comment.
Honor listen_only_mode when deriving the agent default.
This helper ignores the deprecated compatibility field completely. If a config still uses listen_only_mode = true with no explicit response_mode, Line 962 returns None, so the channel default is still dropped instead of producing the effective mode promised by the struct docs. Please derive the effective response mode from both fields before the early return.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/config/types.rs` around lines 955 - 970, The
ChannelConfig::to_conversation_settings helper currently ignores the deprecated
listen_only_mode when deciding whether to return None and when filling
response_mode; compute an effective_response_mode first (use self.response_mode
if Some, otherwise set to listen-only mode when self.listen_only_mode is true,
otherwise default), use that to decide the early return (return None only when
effective_response_mode is default and save_attachments is false), and set
response_mode to that effective value when constructing ConversationSettings
(replace the current unwrap_or_default usage).
1 participant
Summary
[agents.channel]settings (response_mode,save_attachments) were loaded from TOML but silently discarded at runtime —agent_defaultwas alwaysNonein everyResolvedConversationSettings::resolve()call for platform channels and the idle-worker resume pathChannelConfig::to_conversation_settings()inconfig/types.rsto centralise the conversion logicagent_defaultacross all sixresolve()call sites (new-message spawn path + idle-worker resume path)response_mode.is_some(), which would silently drop a configuredsave_attachments = truewhen noresponse_modewas setTest plan
response_mode = "quiet"under[agents.channel]now suppresses responses without needing a/quietslash commandsave_attachments = trueunder[agents.channel]now applies even whenresponse_modeis unset[agents.channel]defaults correctly/quiet,/active) still take precedence over config🤖 Generated with Claude Code