feat: named messaging adapter instances#249
Merged
Conversation
Implement the named-messaging-adapters design doc: allow multiple credential instances per messaging platform (Discord, Slack, Telegram, Twitch, Email, Webhook), with backward-compatible config, runtime adapter identity, binding resolution by adapter selector, instance CRUD API, and a two-column Settings UI. - Config: adapter field on bindings, per-platform instance config structs, validation (no duplicate/empty/reserved names, binding-adapter consistency) - Runtime: adapters keyed by runtime key (e.g. telegram, telegram:support), inbound messages tagged with adapter identity, outbound routed by adapter key - API: GET /api/messaging/status returns instance list, POST/DELETE /api/messaging/instances for instance CRUD, adapter field on binding payloads - UI: two-column layout with platform catalog and expandable instance cards, per-instance binding management, credential forms for all six platforms - 23 new tests covering config parsing, binding matching, validation, and adapter identity
Contributor
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (3)
✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. WalkthroughAdds support for named/multiple messaging adapter instances per platform. Changes include config/TOML schema updates, API endpoints for instance lifecycle, frontend UI and API client updates for instance management, runtime adapter keys in adapter initialization, and propagation of adapter identity through inbound messages and routing. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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 |
jamiepine
force-pushed
the
named-messaging-adapters
branch
from
e4f8ae9 to
14e4a0c
Compare
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
![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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/config.rs (1)
5266-5298:⚠️ Potential issue | 🟠 MajorNamed adapter permission handles are not updated on config reload.
Default adapters receive permission updates because they hold a persistent
Arc<ArcSwap>that is updated via.store()at lines 5271, 5279, 5288, 5296. Named instance adapters break this pattern:
- During reload, a fresh
Arc<ArcSwap>is created for each named instance (lines 5343, 5398, 5452, 5531)- If the adapter already exists, the entire construction block is skipped (
continueat lines 5335, 5390, 5444, 5514)- The new
Arc<ArcSwap>is never created or passed to the adapter- Without a persistent handle registry, existing named adapters retain their initial
Arc<ArcSwap>reference and never receive updatesWhen bindings change and permissions are recalculated, named instance adapters will use stale permissions until they are manually restarted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.rs` around lines 5266 - 5298, Named adapter instances don't get their ArcSwap handles updated on reload because the code creates a fresh Arc<ArcSwap> only when constructing new adapters but skips construction (via the continue path) for existing named adapters; as a result existing adapters keep stale ArcSwap references. Fix by keeping a persistent handle registry for named adapters and, during config reload, lookup the existing ArcSwap for that named instance and call .store(Arc::new(...)) on it using the new permissions produced by DiscordPermissions::from_config, SlackPermissions::from_config, TelegramPermissions::from_config, and TwitchPermissions::from_config instead of creating a new Arc only when building a new adapter; if no existing handle exists then create and register the Arc<ArcSwap> as before. Ensure the code paths that currently do continue when an adapter exists update the registry handle rather than skipping updates.
🧹 Nitpick comments (4)
src/messaging/telegram.rs (1)
591-604: Consider deduplicating runtime conversation-id rewriting logic.This helper now exists in multiple adapters (
src/messaging/discord.rs,src/messaging/slack.rs,src/messaging/twitch.rs, and here). Centralizing it in a shared messaging utility would reduce drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/telegram.rs` around lines 591 - 604, The runtime conversation-id rewriting logic implemented in apply_runtime_adapter_to_conversation_id should be centralized: extract the function into a shared messaging utility module (e.g., messaging::conversation_id or messaging::util) and export it for reuse, then replace the duplicate implementations in the adapters (the functions currently in discord.rs, slack.rs, twitch.rs and telegram.rs) to call the shared function; ensure the extracted function retains the same signature/behavior (accepts runtime_key: &str and base_conversation_id: String and returns String) and update adapter imports/usages accordingly so there is a single authoritative implementation.src/messaging/discord.rs (1)
790-803: Consider movingapply_runtime_adapter_to_conversation_idto shared messaging utils.Same logic now exists across multiple adapters; centralizing it will reduce divergence bugs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/discord.rs` around lines 790 - 803, The function apply_runtime_adapter_to_conversation_id duplicates logic across adapters—move it into a shared messaging utilities module (e.g., messaging::utils or messaging_utils) and replace local copies with a single exported function; keep the same signature (runtime_key: &str, base_conversation_id: String) and behavior (split_once on ':' and return base_conversation_id if no split or same platform, otherwise format!("{runtime_key}:{remainder}")), update all callers in adapter modules to import and call the shared apply_runtime_adapter_to_conversation_id, and ensure the function is pub(crate) or pub as needed so adapters can access it.src/messaging/twitch.rs (1)
256-260: Consider centralizing runtime conversation-id rewriting.This helper now appears in multiple adapters with identical logic. A shared utility would reduce drift risk.
Also applies to: 467-480
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/twitch.rs` around lines 256 - 260, Duplicate runtime conversation-id rewriting logic (building base_conversation_id and calling apply_runtime_adapter_to_conversation_id) appears across adapters; extract and centralize it into a shared helper (e.g., a new function in a common utils/module) and replace the inline logic in twitch adapter and other adapters with a single call. Specifically, create a utility that accepts runtime_key and channel_login (or other base components), constructs the base_conversation_id like "twitch:{channel_login}", applies apply_runtime_adapter_to_conversation_id, and returns the final conversation_id; then update occurrences that currently build base_conversation_id and call apply_runtime_adapter_to_conversation_id to use this new helper.src/config.rs (1)
5316-5327: Use full variable names instead ofpermsin new hot-start paths.Please rename
permstopermissionsin these new blocks for consistency with repository naming rules.As per coding guidelines, "Don't abbreviate variable names. Use
queuenotq,messagenotmsg,channelnotch. Common abbreviations likeconfigare fine."Also applies to: 5363-5375, 5425-5436, 5487-5506, 5530-5544
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.rs` around lines 5316 - 5327, Rename the abbreviated local variable perms to permissions in the new hot-start code paths: replace the let perms = match discord_permissions { ... } block and its inner bindings (including the Arc::new(arc_swap::ArcSwap::from_pointee(...) result) and the variable passed into DiscordAdapter::new) so the variable is called permissions; update all uses in the nearby blocks that call DiscordPermissions::from_config(discord_config, &config.bindings) and pass the value into crate::messaging::discord::DiscordAdapter::new("discord", &discord_config.token, permissions). Also apply the same renaming in the other indicated blocks (around the DiscordPermissions::from_config and DiscordAdapter::new usages at the listed ranges) for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/content/docs/`(configuration)/config.mdx:
- Around line 612-671: The docs currently add [[messaging.*.instances]] for
Discord/Slack/Telegram/Twitch but omit matching instance sections for Email and
Webhook; add corresponding table blocks for [messaging.email] and
[[messaging.email.instances]] and for [messaging.webhook] and
[[messaging.webhook.instances]] following the same pattern (keys: name, enabled,
plus adapter-specific required fields like smtp config or url/token and
dm_allowed_users/channels where applicable) so the config reference covers named
instances for Email and Webhook just like Discord/Slack/Telegram/Twitch.
In `@docs/content/docs/`(messaging)/messaging.mdx:
- Around line 86-87: The sentence explaining default vs named adapters is
incomplete; update the text around the `adapter = "<name>"` example to
explicitly list which platforms participate in default-adapter behavior — either
expand the list to include Email and Webhook if they also support default
adapters in this PR or reword to clarify that only certain platforms (e.g.,
`discord`, `slack`, `telegram`, `twitch`) use the platform default adapter while
others (Email, Webhook) do not; ensure the `adapter = "<name>"` example remains
and the sentence clearly states which platforms are included.
In `@interface/src/components/ChannelSettingCard.tsx`:
- Around line 328-332: The header uses a div with role="button", which is not
fully keyboard-accessible; update the ChannelSettingCard header to use a
semantic <button> element instead of the div: replace the element that currently
has role="button" and onClick={onToggleExpand} with a <button type="button">
preserving the existing className ("flex w-full items-center gap-3 p-3 text-left
cursor-pointer"), move the onClick handler to the button, remove the role
attribute, and add an aria-expanded attribute (e.g., aria-expanded={isExpanded}
or the component's expansion state) to reflect expansion state for screen
readers.
In `@interface/src/routes/AgentDetail.tsx`:
- Around line 247-250: The header renders an empty title when displayName is an
empty string; update the fallback logic in the AgentDetail header to use a
trimmed truthy check instead of the nullish coalescing operator—replace usages
of displayName ?? agentId with a conditional that returns displayName when
displayName?.trim() is truthy, otherwise agentId, and change the conditional
rendering of the secondary span from displayName && (...) to displayName?.trim()
&& (...) so empty/whitespace names are treated as absent.
In `@src/api/bindings.rs`:
- Around line 419-421: Normalize and validate request.adapter before persisting:
trim whitespace and only write to binding_table["adapter"] if the trimmed string
is non-empty, matching the same normalization used in the other block around
lines 749-753; update the code that currently does binding_table["adapter"] =
toml_edit::value(adapter.as_str()) (referencing request.adapter and
binding_table["adapter"]) to use the trimmed value and skip writing when empty
so create/update behaviors are consistent with delete/matching logic.
In `@src/api/messaging.rs`:
- Around line 45-50: TogglePlatformRequest.adapter is declared but never used,
causing unused-field warnings and incorrect platform-wide toggles; update the
toggle_platform handler (and the other two places that currently perform
platform-wide enable/disable) to read request.adapter and branch: if adapter is
Some(name) perform an adapter/named-instance scoped enable/disable (use the
existing instance-level toggle/update functions or lookup that accept an adapter
identifier and pass the adapter string), otherwise perform the existing
platform-wide behavior; also include the adapter in logs/keys so the adapter
field is actually referenced and named-instance semantics are enforced.
- Around line 1473-1492: In delete_messaging_instance, the default-instance
branch only removes credentials for discord/slack/telegram/twitch, leaving
Email/Webhook secrets in config.toml; update the match on platform.as_str()
inside delete_messaging_instance to also remove the Email and Webhook keys
(e.g., "email" / "smtp_username" / "smtp_password" or whatever keys are used for
Email, and "webhook" / "url" / "secret" for Webhook) from the table before
saving so the root credentials are cleared; locate the match block around
table.remove calls and add the appropriate table.remove(...) calls for
Email/Webhook entries to ensure the instance is fully deleted.
In `@src/config.rs`:
- Around line 1299-1307: The binding_runtime_adapter_key currently treats
Some("") as None, but the adapter selector validation elsewhere treats Some("")
as a named value and rejects it; normalize the adapter selector before any
validation and key building so empty strings are treated as None in both places.
Update binding_runtime_adapter_key to normalize the incoming adapter (e.g., map
empty strings to None) and change the adapter selector validation logic that
rejects Some("") to use the same normalization (or accept empty as default),
ensuring both use the same check (adapter.as_ref().and_then(|s| if s.is_empty()
{ None } else { Some(s) }) or equivalent) so behavior is consistent across
functions.
- Around line 4480-4484: The current Discord token resolution evaluates the
DB/TOML value before the environment; change the precedence to environment-first
so it checks std::env::var("DISCORD_BOT_TOKEN") (or resolve_env_value on env)
before falling back to d.token and then any default. Update the token resolution
logic around the token variable (the use of d.token, resolve_env_value, and
DISCORD_BOT_TOKEN) to evaluate env > DB > default; apply the same pattern to the
similar blocks at the other locations mentioned (near the code ranges for
Slack/Telegram/Twitch).
- Around line 4465-4478: The current use of filter_map with a `?` inside the
closure silently drops instances whose credentials fail to resolve; replace this
silent filtering with explicit error propagation or logging: iterate over
d.instances (e.g., with map) attempt to resolve the token via resolve_env_value
for each instance, and if resolution fails return or propagate a descriptive
error (or collect Results and use collect::<Result<Vec<_>, _>>() so the caller
sees which instance failed) instead of skipping it; construct
DiscordInstanceConfig only on success and include the instance name in the error
message to aid debugging.
In `@src/main.rs`:
- Around line 1466-1487: The loop registers enabled Discord instances
unconditionally, causing adapters to start without required credentials; before
constructing the runtime_key, perms, and calling DiscordAdapter::new/register,
validate that the instance has required credential fields (e.g., token/non-empty
strings or other platform-specific secrets) and skip logging/registering if
missing; update the block around discord_config.instances iteration to perform a
guard check (using instance.token.is_empty() or equivalent) and emit a clear
warning via the logger instead of creating
DiscordPermissions::from_instance_config, binding_runtime_adapter_key,
DiscordAdapter::new, and new_messaging_manager.register when credentials are
absent.
- Around line 1660-1667: The token filename generation for token_file_name
currently normalizes instance.name by replacing non-alphanumerics with '_' which
can collide (e.g., "ops-prod" vs "ops_prod"); change the strategy in the
token_file_name construction to include a collision-resistant suffix derived
from instance.name such as a short hex or base64url hash (e.g., sha256/blake3
digest truncated to e.g. 8-12 chars) or use percent-encoding of the raw name,
ensuring the suffix uses filesystem-safe characters; update the code that builds
token_file_name so it still produces a safe filename but appends the
hash/encoded instance.name to guarantee uniqueness.
In `@src/messaging/email.rs`:
- Around line 837-841: The InboundMessage creation sets adapter to a constant
"email" which prevents instance-scoped routing; update the adapter field in the
InboundMessage construction (the adapter property set alongside id/message_id
and conversation_id) to use the instance runtime key (e.g., format
"email:<instance_name>" or the existing runtime_key variable) instead of the
hardcoded "email" so that adapter-specific bindings and outbound replies resolve
to the correct named instance.
In `@src/messaging/webhook.rs`:
- Around line 268-272: The InboundMessage construction is hardcoding adapter to
"webhook" which loses named-instance identity; change the adapter field in the
InboundMessage initializer (the block that creates InboundMessage) to use the
runtime adapter key/variable (e.g., adapter_key, runtime_adapter, or similar)
instead of the literal "webhook", and wrap it in Some(...) so it carries values
like "webhook:ops" for named instances; update any call-sites that pass the
adapter value into this scope to supply the correct runtime adapter key if
needed.
---
Outside diff comments:
In `@src/config.rs`:
- Around line 5266-5298: Named adapter instances don't get their ArcSwap handles
updated on reload because the code creates a fresh Arc<ArcSwap> only when
constructing new adapters but skips construction (via the continue path) for
existing named adapters; as a result existing adapters keep stale ArcSwap
references. Fix by keeping a persistent handle registry for named adapters and,
during config reload, lookup the existing ArcSwap for that named instance and
call .store(Arc::new(...)) on it using the new permissions produced by
DiscordPermissions::from_config, SlackPermissions::from_config,
TelegramPermissions::from_config, and TwitchPermissions::from_config instead of
creating a new Arc only when building a new adapter; if no existing handle
exists then create and register the Arc<ArcSwap> as before. Ensure the code
paths that currently do continue when an adapter exists update the registry
handle rather than skipping updates.
---
Nitpick comments:
In `@src/config.rs`:
- Around line 5316-5327: Rename the abbreviated local variable perms to
permissions in the new hot-start code paths: replace the let perms = match
discord_permissions { ... } block and its inner bindings (including the
Arc::new(arc_swap::ArcSwap::from_pointee(...) result) and the variable passed
into DiscordAdapter::new) so the variable is called permissions; update all uses
in the nearby blocks that call DiscordPermissions::from_config(discord_config,
&config.bindings) and pass the value into
crate::messaging::discord::DiscordAdapter::new("discord", &discord_config.token,
permissions). Also apply the same renaming in the other indicated blocks (around
the DiscordPermissions::from_config and DiscordAdapter::new usages at the listed
ranges) for consistency.
In `@src/messaging/discord.rs`:
- Around line 790-803: The function apply_runtime_adapter_to_conversation_id
duplicates logic across adapters—move it into a shared messaging utilities
module (e.g., messaging::utils or messaging_utils) and replace local copies with
a single exported function; keep the same signature (runtime_key: &str,
base_conversation_id: String) and behavior (split_once on ':' and return
base_conversation_id if no split or same platform, otherwise
format!("{runtime_key}:{remainder}")), update all callers in adapter modules to
import and call the shared apply_runtime_adapter_to_conversation_id, and ensure
the function is pub(crate) or pub as needed so adapters can access it.
In `@src/messaging/telegram.rs`:
- Around line 591-604: The runtime conversation-id rewriting logic implemented
in apply_runtime_adapter_to_conversation_id should be centralized: extract the
function into a shared messaging utility module (e.g.,
messaging::conversation_id or messaging::util) and export it for reuse, then
replace the duplicate implementations in the adapters (the functions currently
in discord.rs, slack.rs, twitch.rs and telegram.rs) to call the shared function;
ensure the extracted function retains the same signature/behavior (accepts
runtime_key: &str and base_conversation_id: String and returns String) and
update adapter imports/usages accordingly so there is a single authoritative
implementation.
In `@src/messaging/twitch.rs`:
- Around line 256-260: Duplicate runtime conversation-id rewriting logic
(building base_conversation_id and calling
apply_runtime_adapter_to_conversation_id) appears across adapters; extract and
centralize it into a shared helper (e.g., a new function in a common
utils/module) and replace the inline logic in twitch adapter and other adapters
with a single call. Specifically, create a utility that accepts runtime_key and
channel_login (or other base components), constructs the base_conversation_id
like "twitch:{channel_login}", applies apply_runtime_adapter_to_conversation_id,
and returns the final conversation_id; then update occurrences that currently
build base_conversation_id and call apply_runtime_adapter_to_conversation_id to
use this new helper.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
README.mddocs/content/docs/(configuration)/config.mdxdocs/content/docs/(messaging)/messaging.mdxinterface/src/api/client.tsinterface/src/components/ChannelSettingCard.tsxinterface/src/components/Sidebar.tsxinterface/src/routes/AgentDetail.tsxinterface/src/routes/Settings.tsxsrc/agent/channel.rssrc/api/bindings.rssrc/api/messaging.rssrc/api/server.rssrc/api/webchat.rssrc/config.rssrc/cron/scheduler.rssrc/lib.rssrc/main.rssrc/messaging/discord.rssrc/messaging/email.rssrc/messaging/manager.rssrc/messaging/slack.rssrc/messaging/telegram.rssrc/messaging/twitch.rssrc/messaging/webhook.rs
- Tag email/webhook inbound messages with runtime adapter key - Use semantic <button> for expandable instance card header (a11y) - Add email/webhook credential removal in delete_messaging_instance - Warn and auto-disable instances with unresolvable credentials - Reverse credential precedence to env > TOML for all platforms - Guard named instances for empty credentials before registration - Fix twitch token-path collisions with hash suffix - Normalize adapter field before persisting in bindings - Guard adapter_key() and validation against empty/whitespace strings - Case-insensitive 'default' rejection in create_messaging_instance - Deduplicate apply_runtime_adapter_to_conversation_id into traits.rs - Rename perms -> permissions in hot-start paths - Guard blank displayName in AgentDetail header - Update docs default-adapter platform list
Email: add full [[messaging.email.instances]] support — config structs, TOML parsing, validation, EmailAdapter::from_instance_config(), startup registration, hot-reload, and docs. Twitch: load_token() no longer returns expires_at. The twitch-irc library's RefreshingLoginCredentials spin-loops with no backoff when OAuth refresh fails (e.g. invalid client_secret). By never returning expires_at from load_token, the library treats the access token as non-expiring on load. Real expiry is only set via update_token() after a successful library-driven refresh.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/api/messaging.rs (1)
1092-1096:⚠️ Potential issue | 🟠 MajorDisabling a named instance shuts down all platform adapters.
When
request.adapterisSomeandrequest.enabledisfalse, lines 768-797 correctly update only that specific instance in TOML, but the disable branch here unconditionally callsremove_platform_adapters(platform), shutting down all adapters for the platform instead of just the targeted instance.🐛 Suggested fix to scope the shutdown
} else if let Some(manager) = manager - && let Err(error) = manager.remove_platform_adapters(platform).await { - tracing::warn!(%error, platform = %platform, "failed to shut down adapter on toggle"); + if let Some(adapter_name) = &request.adapter { + let adapter_name = adapter_name.trim(); + let runtime_key = crate::config::binding_runtime_adapter_key(platform, Some(adapter_name)); + if let Err(error) = manager.remove_adapter(&runtime_key).await { + tracing::warn!(%error, adapter = %runtime_key, "failed to shut down adapter on toggle"); + } + } else if let Err(error) = manager.remove_platform_adapters(platform).await { + tracing::warn!(%error, platform = %platform, "failed to shut down adapter on toggle"); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/messaging.rs` around lines 1092 - 1096, The disable branch currently unconditionally calls manager.remove_platform_adapters(platform), shutting down all adapters; instead, when request.adapter is Some(...) you must call the per-instance shutdown API and only remove that adapter. Change the conditional to check request.adapter and, in that branch, call the single-adapter shutdown (e.g., manager.remove_platform_adapter(platform, adapter).await) with the same error handling/logging; keep the existing call to remove_platform_adapters(platform) only for the case where request.adapter is None so the whole platform is torn down.src/config.rs (1)
1178-1181:⚠️ Potential issue | 🟠 MajorNormalize
binding.adapteronce at ingest to prevent validation/runtime mismatch.
Line 1356normalizes adapter values for validation, butLine 4918stores raw values and runtime helpers compare/use raw selectors (Lines 1299, 1310, 2210).
A binding likeadapter = " support "can pass validation yet fail matching/routing at runtime.Proposed fix
fn binding_runtime_adapter_key(platform: &str, adapter: Option<&str>) -> String { - if let Some(name) = adapter - && !name.is_empty() + if let Some(name) = adapter.map(str::trim).filter(|value| !value.is_empty()) { return format!("{platform}:{name}"); } platform.to_string() } @@ fn binding_adapter_matches(binding: &Binding, message: &crate::InboundMessage) -> bool { - match (&binding.adapter, message.adapter_selector()) { + let binding_adapter = binding + .adapter + .as_deref() + .map(str::trim) + .filter(|value| !value.is_empty()); + match (binding_adapter, message.adapter_selector()) { (None, None) => true, - (Some(expected), Some(actual)) => expected == actual, + (Some(expected), Some(actual)) => expected == actual, _ => false, } } @@ fn binding_adapter_selector_matches(binding: &Binding, adapter_selector: Option<&str>) -> bool { - match (binding.adapter.as_deref(), adapter_selector) { + let binding_selector = binding + .adapter + .as_deref() + .map(str::trim) + .filter(|value| !value.is_empty()); + match (binding_selector, adapter_selector) { (None, None) => true, (Some(binding_selector), Some(requested_selector)) => { binding_selector == requested_selector } _ => false, @@ let bindings: Vec<Binding> = toml .bindings .into_iter() .map(|b| Binding { agent_id: b.agent_id, channel: b.channel, - adapter: b.adapter, + adapter: b + .adapter + .map(|value| value.trim().to_string()) + .filter(|value| !value.is_empty()), guild_id: b.guild_id, workspace_id: b.workspace_id, chat_id: b.chat_id,Also applies to: 1299-1307, 1310-1315, 1356-1360, 2210-2218, 4918-4925
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.rs` around lines 1178 - 1181, The binding.adapter value must be normalized once when the binding is ingested and stored normalized so validation and runtime matching use the same canonical value: update the code path that constructs/stores the binding to trim and canonicalize adapter (e.g., lowercase and remove surrounding whitespace) before assigning to the binding.adapter field, and ensure uses_default_adapter() (the pub fn uses_default_adapter(&self) -> bool) and any runtime helper that compares adapter selectors operate on that normalized value (or call a single normalization helper). Locate the code that currently normalizes adapter for validation and extract that normalization logic into a shared function (e.g., normalize_adapter) and apply it both at ingest time and in helpers to prevent mismatches. Ensure tests cover inputs like " support " to verify stored adapter is normalized and matching/routing works.
🧹 Nitpick comments (2)
interface/src/components/ChannelSettingCard.tsx (2)
41-43: Unused helper function.
platformSupportsNamedAdaptersis defined but never called within this file. If it's intended for external use, consider exporting it; otherwise, it can be removed to reduce dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/components/ChannelSettingCard.tsx` around lines 41 - 43, The helper function platformSupportsNamedAdapters is defined but unused in ChannelSettingCard.tsx; either remove it to eliminate dead code or make it available for reuse by exporting it (export function platformSupportsNamedAdapters) and updating any import sites that should call it. If you choose to export, ensure the function is included in the module's public API and run a grep/IDE search for places that should consume it (or add tests); if you remove it, delete the platformSupportsNamedAdapters definition and any related unused references or imports in this file.
508-509: Minor: unnecessary grouping parentheses.The
{(wrapping around the remove instance section is redundant. The JSX fragment works without it.♻️ Suggested simplification
- {( - <div className="border-t border-app-line/50 pt-3"> + <div className="border-t border-app-line/50 pt-3"> ... - )} + </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/components/ChannelSettingCard.tsx` around lines 508 - 509, In ChannelSettingCard.tsx inside the JSX for the remove-instance section, there are redundant grouping parentheses around the fragment that starts with <div className="border-t border-app-line/50 pt-3">; remove the leading "{(" and the matching ")}" so the <div ...> block is returned directly by the ChannelSettingCard component, leaving the JSX unchanged otherwise.
🤖 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.rs`:
- Around line 1473-1483: The current default_present check only looks at
email.imap_host, allowing a default adapter even if credentials/SMTP are
missing; update the default_present boolean in the messaging.email handling to
require all required default-email fields are non-empty (e.g. check
email.imap_host.trim().is_empty() && email.imap_username.trim().is_empty() &&
email.imap_password.trim().is_empty() && email.smtp_host.trim().is_empty() —
invert appropriately so default_present is true only when none of those are
empty), then pass the same named_instances into validate_runtime_keys("email",
default_present, &named_instances) as before; this ensures validate_runtime_keys
will treat the adapter as present only when host and credentials are provided.
- Around line 1963-1985: EmailInstanceConfig currently derives Debug which may
print sensitive IMAP/SMTP credentials; remove the automatic Debug derive and
instead implement Debug manually for EmailInstanceConfig so that sensitive
fields (at minimum imap_password and smtp_password, and ideally imap_username
and smtp_username and any host fields you deem sensitive) are redacted (e.g.
"<redacted>") in the Debug output; keep Clone derived and implement fmt::Debug
for the EmailInstanceConfig struct to format non-sensitive fields normally and
replace credential field values with the redaction placeholder.
---
Duplicate comments:
In `@src/api/messaging.rs`:
- Around line 1092-1096: The disable branch currently unconditionally calls
manager.remove_platform_adapters(platform), shutting down all adapters; instead,
when request.adapter is Some(...) you must call the per-instance shutdown API
and only remove that adapter. Change the conditional to check request.adapter
and, in that branch, call the single-adapter shutdown (e.g.,
manager.remove_platform_adapter(platform, adapter).await) with the same error
handling/logging; keep the existing call to remove_platform_adapters(platform)
only for the case where request.adapter is None so the whole platform is torn
down.
In `@src/config.rs`:
- Around line 1178-1181: The binding.adapter value must be normalized once when
the binding is ingested and stored normalized so validation and runtime matching
use the same canonical value: update the code path that constructs/stores the
binding to trim and canonicalize adapter (e.g., lowercase and remove surrounding
whitespace) before assigning to the binding.adapter field, and ensure
uses_default_adapter() (the pub fn uses_default_adapter(&self) -> bool) and any
runtime helper that compares adapter selectors operate on that normalized value
(or call a single normalization helper). Locate the code that currently
normalizes adapter for validation and extract that normalization logic into a
shared function (e.g., normalize_adapter) and apply it both at ingest time and
in helpers to prevent mismatches. Ensure tests cover inputs like " support " to
verify stored adapter is normalized and matching/routing works.
---
Nitpick comments:
In `@interface/src/components/ChannelSettingCard.tsx`:
- Around line 41-43: The helper function platformSupportsNamedAdapters is
defined but unused in ChannelSettingCard.tsx; either remove it to eliminate dead
code or make it available for reuse by exporting it (export function
platformSupportsNamedAdapters) and updating any import sites that should call
it. If you choose to export, ensure the function is included in the module's
public API and run a grep/IDE search for places that should consume it (or add
tests); if you remove it, delete the platformSupportsNamedAdapters definition
and any related unused references or imports in this file.
- Around line 508-509: In ChannelSettingCard.tsx inside the JSX for the
remove-instance section, there are redundant grouping parentheses around the
fragment that starts with <div className="border-t border-app-line/50 pt-3">;
remove the leading "{(" and the matching ")}" so the <div ...> block is returned
directly by the ChannelSettingCard component, leaving the JSX unchanged
otherwise.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
docs/content/docs/(configuration)/config.mdxdocs/content/docs/(messaging)/messaging.mdxinterface/src/components/ChannelSettingCard.tsxinterface/src/routes/AgentDetail.tsxsrc/api/bindings.rssrc/api/messaging.rssrc/config.rssrc/lib.rssrc/main.rssrc/messaging.rssrc/messaging/discord.rssrc/messaging/email.rssrc/messaging/slack.rssrc/messaging/telegram.rssrc/messaging/traits.rssrc/messaging/twitch.rssrc/messaging/webhook.rs
- Email default_present checks all 4 required fields (imap_host, imap_username, imap_password, smtp_host) - EmailInstanceConfig manual Debug impl with credential redaction - toggle_platform disable: per-instance shutdown via remove_adapter() when adapter is Some - Extract normalize_adapter() helper; apply at binding ingest time; add test - Remove unused platformSupportsNamedAdapters function from ChannelSettingCard.tsx - Remove redundant grouping parens in remove-instance JSX - Instance name badge: both default and named use same gray badge style
This was referenced Mar 1, 2026
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.
Summary
Implements
docs/design-docs/named-messaging-adapters.md— support for multiple named credential instances per messaging platform while keeping legacy single-token config working.adapter: Option<String>on bindings, per-platformInstanceConfigstructs withinstances: Vec<...>on all six platform configs, validation (no duplicate/empty/reserved names, binding-adapter reference checks)telegramfor default,telegram:supportfor named), inbound messages tagged with adapter identity, outbound routed by captured adapter keyGET /api/messaging/statusreturnsinstances: Vec<AdapterInstanceStatus>,POST/DELETE /api/messaging/instancesfor instance CRUD, optionaladapterfield on all binding CRUD payloads● Active/○ DisabledindicatorsFiles changed (25)
Rust backend:
src/config.rs,src/lib.rs,src/main.rs,src/api/messaging.rs,src/api/bindings.rs,src/api/server.rs,src/api/webchat.rs,src/messaging/manager.rs,src/messaging/discord.rs,src/messaging/slack.rs,src/messaging/telegram.rs,src/messaging/twitch.rs,src/messaging/email.rs,src/messaging/webhook.rs,src/agent/channel.rs,src/cron/scheduler.rsFrontend:
interface/src/api/client.ts,interface/src/components/ChannelSettingCard.tsx,interface/src/routes/Settings.tsx,interface/src/components/Sidebar.tsx,interface/src/routes/AgentDetail.tsxDocs:
README.md,docs/content/docs/(configuration)/config.mdx,docs/content/docs/(messaging)/messaging.mdx,docs/design-docs/named-messaging-adapters.mdNote
Core changes enable multiple adapter instances per platform with backward compatibility. Configuration supports named instances (e.g.,
telegram:support) alongside the default instance. Messaging bindings are enhanced to specify target adapters, and the UI provides a two-column interface for managing platform credentials and their associated bindings. All changes maintain existing single-token configurations.Written by Tembo for commit e4f8ae9. This will update automatically on new commits.