feat: Add Mattermost channel support#428
Open
unverbraucht wants to merge 8 commits intospacedriveapp:mainfrom
Open
feat: Add Mattermost channel support#428unverbraucht wants to merge 8 commits intospacedriveapp:mainfrom
unverbraucht wants to merge 8 commits intospacedriveapp:mainfrom
Conversation
Implements a full Mattermost adapter using a custom HTTP + WebSocket
client (reqwest + tokio-tungstenite), following the existing adapter
architecture.
Features:
- WebSocket-based inbound messages with exponential backoff reconnection
- Text responses, streaming edits (ZWS placeholder + throttled PUT),
file uploads, reactions, typing indicators, thread replies
- History fetch via /channels/{id}/posts
- Named adapter instances with per-instance permissions
- Hot-reload of permissions via spawn_file_watcher
- Conversation IDs: mattermost:{team_id}:{channel_id} and
mattermost:{team_id}:dm:{user_id}
- Fail-closed permission filtering: messages without a team_id are
rejected when team/channel filters are configured
- URL validation at config load time
Config:
[messaging.mattermost]
enabled = true
base_url = "https://mattermost.example.com"
token = "env:MATTERMOST_TOKEN"
team_id = "optional-default-team-id"
Resolved conflicts in src/config.rs (refactored to submodules), src/main.rs, src/messaging.rs, and src/messaging/target.rs. Distributed Mattermost config additions across the new config submodule structure introduced upstream.
- Add Mattermost platform to ChannelSettingCard, ChannelEditModal, Settings, and platformIcons - Server URL and access token credential inputs - Backend: read mattermost config into instances list in messaging_status - Backend: write base_url and token to TOML via create_messaging_instance - Mattermost instances configured manually in config.toml appear in the UI
Contributor
WalkthroughAdds end-to-end Mattermost support: docs, UI fields/icons, API surface, config schema/types/permissions, hot-reload watcher integration, messaging target normalization, and a full Rust Mattermost adapter implementation with tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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}
Comment on lines
+178
to
+181
| self.typing_tasks | ||
| .write() | ||
| .await | ||
| .insert(channel_id.to_string(), handle); |
Contributor
There was a problem hiding this comment.
Dropping the previous JoinHandle here won’t stop the old typing task, so repeated start_typing() calls can leave multiple loops running for the same channel.
Suggested change
| self.typing_tasks | |
| .write() | |
| .await | |
| .insert(channel_id.to_string(), handle); | |
| let mut typing_tasks = self.typing_tasks.write().await; | |
| if let Some(previous) = typing_tasks.insert(channel_id.to_string(), handle) { | |
| previous.abort(); | |
| } |
src/messaging/mattermost.rs
Outdated
| handle.abort(); | ||
| } | ||
|
|
||
| self.typing_tasks.write().await.clear(); |
Contributor
There was a problem hiding this comment.
clear() drops the JoinHandles without cancelling the spawned loops. Aborting them on shutdown avoids leaking background tasks.
Suggested change
| self.typing_tasks.write().await.clear(); | |
| let mut typing_tasks = self.typing_tasks.write().await; | |
| for (_, handle) in typing_tasks.drain() { | |
| handle.abort(); | |
| } |
| .client | ||
| .get(self.api_url("/users/me")) | ||
| .bearer_auth(self.token.as_ref()) | ||
| .send() |
Contributor
There was a problem hiding this comment.
Minor robustness: for this /users/me call, consider checking response.status() before .json() (like create_post does), so auth/config errors show up as a clear status+body instead of a JSON parse failure.
| .post(self.api_url("/files")) | ||
| .bearer_auth(self.token.as_ref()) | ||
| .multipart(form) | ||
| .send() |
Contributor
There was a problem hiding this comment.
This path parses JSON without checking the HTTP status first. Doing an explicit status.is_success() check (and including the response body on error) makes failures much easier to debug (401/403/413/etc) and avoids misleading “parse file upload response” errors.
src/messaging/target.rs
Outdated
| // or mattermost:{team_id}:dm:{user_id} | ||
| let parts: Vec<&str> = channel.id.split(':').collect(); | ||
| match parts.as_slice() { | ||
| ["mattermost", _team_id, "dm", user_id] => format!("dm:{user_id}"), |
Contributor
There was a problem hiding this comment.
One gotcha: this fallback returns dm:{user_id}, but MattermostAdapter::broadcast() currently treats the target as a channel_id (and validate_id() rejects :), so DM broadcasts will always fail unless platform_meta.mattermost_channel_id is present. Might be worth either returning None here until DM broadcast is implemented, or teaching the adapter to resolve dm:{user_id} → a DM channel id via the API.
src/messaging/mattermost.rs
Outdated
Comment on lines
+75
to
+76
| let base_url = | ||
| Url::parse(base_url).context("invalid mattermost base_url")?; |
Contributor
There was a problem hiding this comment.
If someone configures base_url with a path (e.g. https://mm.example.com/mattermost), api_url()/ws_url() will silently append under that path. It might be worth rejecting non-origin URLs here (or normalizing) so requests go where users expect.
Suggested change
| let base_url = | |
| Url::parse(base_url).context("invalid mattermost base_url")?; | |
| let base_url = Url::parse(base_url).context("invalid mattermost base_url")?; | |
| if base_url.path() != "/" || base_url.query().is_some() || base_url.fragment().is_some() { | |
| return Err(anyhow::anyhow!( | |
| "mattermost base_url must not include path/query/fragment (got: {})", | |
| base_url | |
| )); | |
| } |
Comment on lines
+515
to
+526
| let display_text = if active.accumulated_text.len() > MAX_MESSAGE_LENGTH { | ||
| let end = active | ||
| .accumulated_text | ||
| .floor_char_boundary(MAX_MESSAGE_LENGTH - 3); | ||
| format!("{}...", &active.accumulated_text[..end]) | ||
| } else { | ||
| active.accumulated_text.clone() | ||
| }; | ||
|
|
||
| if let Err(error) = self.edit_post(&active.post_id, &display_text).await { | ||
| tracing::warn!(%error, "failed to edit streaming message"); | ||
| } |
Contributor
There was a problem hiding this comment.
This currently clones accumulated_text on every throttled edit even when it’s under MAX_MESSAGE_LENGTH. Using a borrowed &str for the common case avoids repeated allocations for long streams. Also, StreamEnd may want to apply the same max-length truncation as the in-progress edits to avoid finalizing with an oversized post.
Suggested change
| let display_text = if active.accumulated_text.len() > MAX_MESSAGE_LENGTH { | |
| let end = active | |
| .accumulated_text | |
| .floor_char_boundary(MAX_MESSAGE_LENGTH - 3); | |
| format!("{}...", &active.accumulated_text[..end]) | |
| } else { | |
| active.accumulated_text.clone() | |
| }; | |
| if let Err(error) = self.edit_post(&active.post_id, &display_text).await { | |
| tracing::warn!(%error, "failed to edit streaming message"); | |
| } | |
| let display_text: std::borrow::Cow<'_, str> = | |
| if active.accumulated_text.len() > MAX_MESSAGE_LENGTH { | |
| let end = active | |
| .accumulated_text | |
| .floor_char_boundary(MAX_MESSAGE_LENGTH - 3); | |
| std::borrow::Cow::Owned(format!( | |
| "{}...", | |
| &active.accumulated_text[..end] | |
| )) | |
| } else { | |
| std::borrow::Cow::Borrowed(active.accumulated_text.as_str()) | |
| }; | |
| if let Err(error) = | |
| self.edit_post(&active.post_id, display_text.as_ref()).await | |
| { | |
| tracing::warn!(%error, "failed to edit streaming message"); | |
| } |
src/messaging/mattermost.rs
Outdated
Comment on lines
+555
to
+559
| let bot_user_id = self | ||
| .bot_user_id | ||
| .get() | ||
| .map(|s| s.as_ref().to_string()) | ||
| .unwrap_or_default(); |
Contributor
There was a problem hiding this comment.
Minor: defaulting bot_user_id to "" will produce a confusing server-side error when adding reactions. Since this can only work after start() initialized the ID, it seems better to fail fast.
Suggested change
| let bot_user_id = self | |
| .bot_user_id | |
| .get() | |
| .map(|s| s.as_ref().to_string()) | |
| .unwrap_or_default(); | |
| let bot_user_id = self | |
| .bot_user_id | |
| .get() | |
| .ok_or_else(|| anyhow::anyhow!("bot_user_id not initialized"))? | |
| .as_ref() | |
| .to_string(); |
src/messaging/mattermost.rs
Outdated
Comment on lines
+631
to
+641
| self.client | ||
| .post(self.api_url("/posts")) | ||
| .bearer_auth(self.token.as_ref()) | ||
| .json(&serde_json::json!({ | ||
| "channel_id": channel_id, | ||
| "message": caption.unwrap_or_default(), | ||
| "file_ids": file_ids, | ||
| })) | ||
| .send() | ||
| .await | ||
| .context("failed to create post with file")?; |
Contributor
There was a problem hiding this comment.
This /posts call doesn’t check the HTTP status, so failures (401/403/etc) get silently dropped even though create_post() elsewhere includes a great status+body error.
Suggested change
| self.client | |
| .post(self.api_url("/posts")) | |
| .bearer_auth(self.token.as_ref()) | |
| .json(&serde_json::json!({ | |
| "channel_id": channel_id, | |
| "message": caption.unwrap_or_default(), | |
| "file_ids": file_ids, | |
| })) | |
| .send() | |
| .await | |
| .context("failed to create post with file")?; | |
| let response = self | |
| .client | |
| .post(self.api_url("/posts")) | |
| .bearer_auth(self.token.as_ref()) | |
| .json(&serde_json::json!({ | |
| "channel_id": channel_id, | |
| "message": caption.unwrap_or_default(), | |
| "file_ids": file_ids, | |
| })) | |
| .send() | |
| .await | |
| .context("failed to create post with file")?; | |
| let status = response.status(); | |
| if !status.is_success() { | |
| let body = response.text().await.unwrap_or_default(); | |
| return Err(anyhow::anyhow!( | |
| "mattermost POST /posts (file) failed with status {}: {body}", | |
| status.as_u16() | |
| ) | |
| .into()); | |
| } |
Comment on lines
+839
to
+858
| // "D" = direct message, "G" = group DM | ||
| let conversation_id = if post.channel_type.as_deref() == Some("D") { | ||
| apply_runtime_adapter_to_conversation_id( | ||
| runtime_key, | ||
| format!( | ||
| "mattermost:{}:dm:{}", | ||
| team_id.as_deref().unwrap_or(""), | ||
| post.user_id | ||
| ), | ||
| ) | ||
| } else { | ||
| apply_runtime_adapter_to_conversation_id( | ||
| runtime_key, | ||
| format!( | ||
| "mattermost:{}:{}", | ||
| team_id.as_deref().unwrap_or(""), | ||
| post.channel_id | ||
| ), | ||
| ) | ||
| }; |
Contributor
There was a problem hiding this comment.
MattermostPermissions collects dm_allowed_users, but inbound DM events don’t currently apply that allow-list. The other adapters treat empty dm_allowed_users as “drop all DMs”, so I’d mirror that here to avoid accepting DMs by default.
Suggested change
| // "D" = direct message, "G" = group DM | |
| let conversation_id = if post.channel_type.as_deref() == Some("D") { | |
| apply_runtime_adapter_to_conversation_id( | |
| runtime_key, | |
| format!( | |
| "mattermost:{}:dm:{}", | |
| team_id.as_deref().unwrap_or(""), | |
| post.user_id | |
| ), | |
| ) | |
| } else { | |
| apply_runtime_adapter_to_conversation_id( | |
| runtime_key, | |
| format!( | |
| "mattermost:{}:{}", | |
| team_id.as_deref().unwrap_or(""), | |
| post.channel_id | |
| ), | |
| ) | |
| }; | |
| let is_dm = post.channel_type.as_deref() == Some("D"); | |
| if is_dm { | |
| if permissions.dm_allowed_users.is_empty() { | |
| return None; | |
| } | |
| if !permissions.dm_allowed_users.contains(&post.user_id) { | |
| return None; | |
| } | |
| } | |
| // "D" = direct message, "G" = group DM | |
| let conversation_id = if is_dm { | |
| apply_runtime_adapter_to_conversation_id( | |
| runtime_key, | |
| format!( | |
| "mattermost:{}:dm:{}", | |
| team_id.as_deref().unwrap_or(""), | |
| post.user_id | |
| ), | |
| ) | |
| } else { | |
| apply_runtime_adapter_to_conversation_id( | |
| runtime_key, | |
| format!( | |
| "mattermost:{}:{}", | |
| team_id.as_deref().unwrap_or(""), | |
| post.channel_id | |
| ), | |
| ) | |
| }; |
Empty dm_allowed_users blocks all DMs (fail-closed). Non-empty list allows only the specified Mattermost user IDs, consistent with Slack and Telegram adapter behaviour. All other layers (config, TOML schema, permissions merge from bindings, bindings API, UI TagInput) were already in place.
![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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/config/types.rs (1)
1543-1612:⚠️ Potential issue | 🔴 CriticalChannel-scoped Mattermost bindings never match.
At Line 1543 the generic
channel_idsgate only checks Discord/Slack/Twitch metadata. For a Mattermost binding withchannel_ids, it returnsfalsebefore the Mattermost-specific branch at Line 1604 executes, so channel-scoped Mattermost bindings are effectively dead.🎯 Proposed fix
- if !self.channel_ids.is_empty() { + if !self.channel_ids.is_empty() && self.channel != "mattermost" { let message_channel = message .metadata .get("discord_channel_id") .and_then(|v| v.as_u64()) .map(|v| v.to_string());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/types.rs` around lines 1543 - 1612, The generic channel_ids gate (using message.metadata discord/slack/twitch) prevents Mattermost-specific matching; change the logic so Mattermost bindings are handled by the Mattermost-specific branch instead of being short-circuited. Concretely, update the initial if !self.channel_ids.is_empty() block to skip the generic Discord/Slack/Twitch checks when self.channel == "mattermost" (e.g. make the condition if !self.channel_ids.is_empty() && self.channel != "mattermost") or alternatively include message.metadata.get("mattermost_channel_id").and_then(|v| v.as_str()) in the direct_match calculation; reference self.channel_ids, self.channel, message.metadata and the later mattermost_channel_id check so the Mattermost-specific branch (the block that reads "mattermost_channel_id" and the one guarded by if self.channel == "mattermost") can correctly match.
🧹 Nitpick comments (1)
interface/src/components/ChannelSettingCard.tsx (1)
642-652: Add lightweight client-side URL validation for Mattermost server URL.Line 643 currently only checks non-empty input. Validating URL format here gives faster, clearer feedback before submit.
Suggested patch
} else if (platform === "mattermost") { - if (!credentialInputs.mattermost_base_url?.trim()) { + const baseUrl = credentialInputs.mattermost_base_url?.trim(); + if (!baseUrl) { setMessage({text: "Server URL is required", type: "error"}); return; } + try { + new URL(baseUrl); + } catch { + setMessage({text: "Server URL must be a valid absolute URL", type: "error"}); + return; + } if (!credentialInputs.mattermost_token?.trim()) { setMessage({text: "Access token is required", type: "error"}); return; } - credentials.mattermost_base_url = credentialInputs.mattermost_base_url.trim(); + credentials.mattermost_base_url = baseUrl; credentials.mattermost_token = credentialInputs.mattermost_token.trim(); }🤖 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 642 - 652, In the platform === "mattermost" branch of ChannelSettingCard, replace the simple non-empty check for credentialInputs.mattermost_base_url with a lightweight client-side URL validation: trim the input, attempt to validate that it is a well-formed HTTP/HTTPS URL (for example with new URL(trimmed) and checking protocol is http: or https:), and if invalid call setMessage({text: "Invalid server URL", type: "error"}) and return; only after it passes validation assign credentials.mattermost_base_url = trimmed and proceed; keep the existing token trim/validation logic intact.
🤖 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/api/messaging.rs`:
- Around line 1331-1337: The Mattermost branch writes
credentials.mattermost_base_url into platform_table before validating it, which
can corrupt config if Config::load_from_path() later fails; modify the logic in
the "mattermost" arm to first validate the provided URL (e.g., parse with
url::Url::parse or similar) and only set platform_table["base_url"] when
validation succeeds, and if you still call Config::load_from_path() ensure you
check its Result and rollback/remove the recently-inserted keys from
platform_table (or avoid mutating platform_table until validation passes) so a
bad URL never gets persisted; apply the same pattern to the other occurrences
around the other branches referenced (the sections around the previous
Config::load_from_path() calls).
- Around line 1520-1522: The match arm accepting "mattermost" currently does not
clear the default instance credentials, leaving base_url and token set after a
delete; update the default-instance cleanup code path (the match on platform and
the cleanup block around lines 1578-1619) so that when platform == "mattermost"
you explicitly clear the stored credentials (set base_url and token to
None/empty on the config/struct representing the default Mattermost adapter),
persist the updated configuration/state (or delete those fields in the DB), and
only return success after the changes are saved so the default adapter cannot
reappear on reload.
In `@src/config/load.rs`:
- Around line 2232-2255: The current use of filter_map on mm.instances drops
Mattermost instances with unresolved token/base_url before
validate_named_messaging_adapters() runs; instead iterate over mm.instances
(e.g., with map or map + collect) and always produce a MattermostInstanceConfig
record for each instance, but if token.is_none() || base_url.is_none() set
enabled: false (and keep the original instance.name, team_id, dm_allowed_users,
max_attachment_bytes), emit the existing tracing::warn with context, and
preserve the unresolved token/base_url values (or use empty strings if the
struct requires) so validate_named_messaging_adapters() and the settings UI
still see the named adapter present.
In `@src/config/watcher.rs`:
- Around line 245-251: The watcher currently recreates an ArcSwap for each named
Mattermost adapter and immediately drops it so updates never reach running
adapters; change this to keep a shared ArcSwap handle per adapter runtime key
and update that stored value on reload. Concretely: instead of creating a new
local ArcSwap in the adapter initialization, register its ArcSwap handle in a
central registry/map keyed by the adapter runtime key (the same key used to
identify "mattermost:{instance}"), and in the watcher code (where
mattermost_permissions and MattermostPermissions::from_config are used) iterate
the registry entries for mattermost adapters and call
.store(Arc::new(new_perms)) on each ArcSwap handle; ensure the adapter
construction uses the shared ArcSwap handle from the registry rather than
building and dropping its own.
In `@src/messaging/mattermost.rs`:
- Around line 532-540: StreamEnd currently calls edit_post with the full
active.accumulated_text which can exceed MAX_MESSAGE_LENGTH and cause the final
edit to fail; update the OutboundResponse::StreamEnd handling to finalize
streaming by chunking accumulated_text into slices no larger than
MAX_MESSAGE_LENGTH and apply them safely (e.g., edit_post the first chunk for
the placeholder/post_id via active.post_id and then create_post or append
additional posts for remaining chunks), using the existing MAX_MESSAGE_LENGTH
constant and the edit_post/create_post methods to ensure no single edit exceeds
the limit and to remove the placeholder correctly; reference
OutboundResponse::StreamEnd, active.accumulated_text, active.post_id, edit_post,
create_post and MAX_MESSAGE_LENGTH when implementing the split-and-send logic.
- Around line 821-894: When constructing inbound DMs, enforce
MattermostPermissions.dm_allowed_users and add the directed-message hints to
metadata: if post.channel_type indicates a DM ("D" or "G") check
permissions.dm_allowed_users (or per-team list) and return None if the sender
isn't allowed; also add metadata flags the binding layer expects (e.g., a
direct_message/dm_hint boolean and mention-reply-related fields derived from
permissions.require_mention and post root/mentions) into the same metadata
HashMap before creating the InboundMessage so require_mention and
dm_allowed_users routing work (update logic around post.channel_type,
permissions, metadata, and the InboundMessage construction).
- Around line 490-497: OutboundResponse::StreamStart only uses metadata
"mattermost_root_id" so streamed replies escape threads; change the root_id
resolution to mirror the text path: first try
message.metadata.get("mattermost_root_id").and_then(|v| v.as_str()), then fall
back to the REPLY_TO_MESSAGE_ID metadata (or constant) if the first is None, and
pass that resolved root_id into create_post(channel_id, "\u{200B}", root_id).
Update the root_id variable in the StreamStart branch (around
OutboundResponse::StreamStart and the create_post call) to use this fallback
logic so streamed placeholders remain in the original thread.
- Around line 150-181: start_typing() currently spawns a new background task and
inserts its JoinHandle into typing_tasks without stopping any previous task,
causing leaks; at the top of start_typing() call
self.stop_typing(channel_id).await to abort any existing task for that channel
before spawning a new one, and continue using the same
channel_id/channel_id_owned logic and TYPING_INDICATOR_INTERVAL. In shutdown(),
replace the write().await.clear() on typing_tasks with iterating over the map's
values and calling abort() on each JoinHandle (and optionally await or check
join results) to ensure all spawned tasks are terminated. Finally, replace the
discarded shutdown channel send (the current let _ = ...) with proper error
handling (log or return the error) so failures to notify shutdown are not
silently ignored.
In `@src/messaging/target.rs`:
- Around line 143-161: The Mattermost branch currently drops the
instance/runtime adapter key when parsing channel.id (the match arm under
"mattermost" that builds parts from channel.id), causing named instances like
mattermost:{instance}:... to lose their adapter; update that logic so when
platform_meta lacks mattermost_channel_id you parse parts and, for patterns
["mattermost", instance, "dm", user_id] return a conversation id that preserves
the instance (e.g. format!("{instance}:dm:{user_id}")), and for ["mattermost",
instance, channel_id] return format!("{instance}:{channel_id}") instead of only
the channel_id; keep using json_value_to_string and channel.platform_meta as
before and only fall back to None for non-matching shapes.
- Around line 261-269: normalize_mattermost_target currently returns the full
suffix after strip_repeated_prefix instead of the actual send target; update it
to parse the suffix by splitting on ':' and return the real channel or DM id: if
the stripped target is empty return None, if it contains ':' split into parts
and if the first part is "dm" return "dm:{user_id}" using the last part,
otherwise return the last part (the channel id), and if there is no ':' return
the target as-is; use the existing normalize_mattermost_target and
strip_repeated_prefix symbols to locate and modify the logic accordingly.
---
Outside diff comments:
In `@src/config/types.rs`:
- Around line 1543-1612: The generic channel_ids gate (using message.metadata
discord/slack/twitch) prevents Mattermost-specific matching; change the logic so
Mattermost bindings are handled by the Mattermost-specific branch instead of
being short-circuited. Concretely, update the initial if
!self.channel_ids.is_empty() block to skip the generic Discord/Slack/Twitch
checks when self.channel == "mattermost" (e.g. make the condition if
!self.channel_ids.is_empty() && self.channel != "mattermost") or alternatively
include message.metadata.get("mattermost_channel_id").and_then(|v| v.as_str())
in the direct_match calculation; reference self.channel_ids, self.channel,
message.metadata and the later mattermost_channel_id check so the
Mattermost-specific branch (the block that reads "mattermost_channel_id" and the
one guarded by if self.channel == "mattermost") can correctly match.
---
Nitpick comments:
In `@interface/src/components/ChannelSettingCard.tsx`:
- Around line 642-652: In the platform === "mattermost" branch of
ChannelSettingCard, replace the simple non-empty check for
credentialInputs.mattermost_base_url with a lightweight client-side URL
validation: trim the input, attempt to validate that it is a well-formed
HTTP/HTTPS URL (for example with new URL(trimmed) and checking protocol is http:
or https:), and if invalid call setMessage({text: "Invalid server URL", type:
"error"}) and return; only after it passes validation assign
credentials.mattermost_base_url = trimmed and proceed; keep the existing token
trim/validation logic intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f94c442a-2f2b-48e9-82b0-bb38672e10ae
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lock,!**/*.lockCargo.tomlis excluded by!**/*.toml
📒 Files selected for processing (17)
docs/mattermost.mdinterface/src/api/client.tsinterface/src/components/ChannelEditModal.tsxinterface/src/components/ChannelSettingCard.tsxinterface/src/lib/platformIcons.tsxinterface/src/routes/Settings.tsxsrc/api/messaging.rssrc/config.rssrc/config/load.rssrc/config/permissions.rssrc/config/toml_schema.rssrc/config/types.rssrc/config/watcher.rssrc/main.rssrc/messaging.rssrc/messaging/mattermost.rssrc/messaging/target.rs
SEC2: add sanitize_reaction_name() — resolves unicode emoji to shortcodes
via the emojis crate, strips colon wrappers, lowercases plain names.
FN1: user identity resolution — add user_identity_cache, resolve
display name from /users/{id} API on first encounter, set
sender_display_name metadata and formatted_author.
FN2: channel name resolution — add channel_name_cache, resolve
display_name from /channels/{id} API on first encounter, set
mattermost_channel_name and CHANNEL_NAME metadata.
FN3: fetch_history now returns timestamps parsed from post.create_at
milliseconds instead of always None.
FN4: bot mention detection — check message text for @bot_username,
set mattermost_mentions_or_replies_to_bot metadata flag so
require_mention routing mode works correctly.
Also adds 12 new unit tests covering these fixes.
Contributor
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 (2)
src/config/types.rs (2)
1651-1659:⚠️ Potential issue | 🟠 Major
require_mentiondoes not recognize Mattermost metadata.Mattermost inbound metadata uses
mattermost_mentions_or_replies_to_bot, but the mention-key switch has nomattermostarm, so bindings withrequire_mention = truewill always drop Mattermost messages.💡 Proposed fix
let mention_key = match message.source.as_str() { "discord" => "discord_mentions_or_replies_to_bot", "slack" => "slack_mentions_or_replies_to_bot", "twitch" => "twitch_mentions_or_replies_to_bot", "telegram" => "telegram_mentions_or_replies_to_bot", + "mattermost" => "mattermost_mentions_or_replies_to_bot", // Unknown platforms: if require_mention is set, default to // requiring a mention (safe default). _ => return false, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/types.rs` around lines 1651 - 1659, The match on message.source (which sets mention_key) is missing a branch for Mattermost, causing require_mention checks to drop Mattermost messages; update the match in the code that defines mention_key (the match on message.source.as_str()) to include a "mattermost" arm that maps to "mattermost_mentions_or_replies_to_bot" so Mattermost inbound metadata is recognized the same as other platforms.
1543-1576:⚠️ Potential issue | 🔴 CriticalMattermost
channel_idsbindings are currently blocked by the generic channel filter.Line 1543 applies the generic Discord/Slack/Twitch channel check for all platforms; for Mattermost this returns false before the Mattermost branch at Line 1604 runs.
💡 Proposed fix
- if !self.channel_ids.is_empty() { + if !self.channel_ids.is_empty() && self.channel != "mattermost" { let message_channel = message .metadata .get("discord_channel_id") .and_then(|v| v.as_u64()) .map(|v| v.to_string()); @@ - if !self.channel_ids.is_empty() && self.channel == "mattermost" { + if !self.channel_ids.is_empty() && self.channel == "mattermost" { let message_channel = message .metadata .get("mattermost_channel_id") .and_then(|v| v.as_str()); if !self.channel_ids.iter().any(|id| Some(id.as_str()) == message_channel) { return false; } }Also applies to: 1604-1612
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/types.rs` around lines 1543 - 1576, The generic channel filtering block using self.channel_ids and the computed variables message_channel, slack_channel, and twitch_channel is preventing Mattermost checks from running; update the channel extraction and matching logic to also handle Mattermost by reading message.metadata.get("mattermost_channel_id").and_then(|v| v.as_str()) (and include a parent mattermost id if applicable) and include that value in the direct_match boolean (similar to slack_channel/twitch_channel checks), and ensure the same adjustment is applied to the later Mattermost-specific branch referenced around the parent/channel checks (the code that computes message_channel, parent_channel, direct_match, and parent_match).
♻️ Duplicate comments (8)
src/messaging/mattermost.rs (8)
518-523:⚠️ Potential issue | 🟠 Major
StreamStartmisses thread fallback metadata.This path only uses
mattermost_root_id; it should also fallback toREPLY_TO_MESSAGE_IDlike the text path does.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/mattermost.rs` around lines 518 - 523, OutboundResponse::StreamStart currently extracts only message.metadata["mattermost_root_id"] and calls start_typing without the same thread fallback used elsewhere; update the StreamStart branch to follow the text-path logic by checking message.metadata for "mattermost_root_id" and, if absent, falling back to REPLY_TO_MESSAGE_ID before using that value for threaded behavior (the same metadata lookup pattern used in the text response branch), ensuring start_typing/channel handling uses the resolved thread id from message.metadata.
154-186:⚠️ Potential issue | 🟠 MajorTyping tasks are not safely replaced/terminated.
Line 185 overwrites existing channel task handles without explicit abort, and Line 770 clears handles on shutdown without aborting them.
#!/bin/bash # Verify typing task lifecycle handling in start_typing/shutdown rg -n -C4 'async fn start_typing|typing_tasks.*insert|async fn shutdown|typing_tasks.*clear' src/messaging/mattermost.rs💡 Proposed fix
async fn start_typing(&self, channel_id: &str) { + self.stop_typing(channel_id).await; let Some(user_id) = self.bot_user_id.get().cloned() else { return; }; @@ - self.typing_tasks.write().await.clear(); + let mut typing_tasks = self.typing_tasks.write().await; + for (_, handle) in typing_tasks.drain() { + handle.abort(); + }Also applies to: 770-771
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/mattermost.rs` around lines 154 - 186, The start_typing function currently inserts a new tokio::task::JoinHandle into typing_tasks without stopping any existing task for the same channel; update start_typing to check typing_tasks (by channel_id) for an existing handle, call abort() on it and optionally await its completion or ignore the result before inserting the new handle so you don't leak duplicate tasks; likewise modify the shutdown logic (the code that currently clears typing_tasks) to instead iterate all stored JoinHandles, call abort() on each handle (and await or detach as appropriate) before removing them so all background typing tasks are explicitly terminated — refer to the start_typing method, typing_tasks field and the shutdown function to locate the changes.
659-669:⚠️ Potential issue | 🟠 MajorPost creation after file upload lacks HTTP status handling.
Both paths send
POST /postsand ignore non-success responses, which can silently drop file messages.#!/bin/bash # Verify /posts calls after file upload without explicit status checks rg -n -C6 'failed to create post with file|post\(self\.api_url\("/posts"\)\)' src/messaging/mattermost.rsAlso applies to: 818-829
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/mattermost.rs` around lines 659 - 669, The POST to /posts after file upload (the request built with self.client.post(self.api_url("/posts")).bearer_auth(self.token.as_ref()).json(...).send().await.context("failed to create post with file")?) does not check HTTP status and can silently ignore non-2xx responses; update the call to inspect the Response returned by send() (e.g., assign to a variable), call response.error_for_status() or check status().is_success(), and return a contextual error (including status and body text) if not successful. Apply the same fix for the other /posts call that uses the "failed to create post" context (the second occurrence around the other upload path) so both paths surface non-success HTTP responses.
762-764:⚠️ Potential issue | 🟡 MinorDon’t silently discard shutdown send errors.
Line 763 uses
let _ =on aResult; handle/log it (or use.ok()if intentionally drop-safe for channel send).As per coding guidelines: "Don't silently discard errors. No
let _ =on Results. Handle them, log them, or propagate them. The only exception is.ok()on channel sends where the receiver may be dropped."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/mattermost.rs` around lines 762 - 764, The code currently discards the Result from sending the shutdown signal with "let _ = tx.send(()).await" on the shutdown_tx channel; change this to explicitly handle the Result from tx.send(()).await (e.g., use if let Err(err) = tx.send(()).await { log the error with tracing::warn! or tracing::error! } or, if it's intentionally safe to ignore a dropped receiver, call .ok() on the Result) so you no longer silently discard errors; locate the send call on the shutdown_tx (the shutdown_tx.write().await.take() -> tx.send(()).await expression) and replace the "let _ =" pattern with explicit logging or .ok() as appropriate.
583-587:⚠️ Potential issue | 🟡 MinorFail fast when
bot_user_idis uninitialized for reactions.Defaulting to an empty user ID produces opaque server-side reaction errors.
💡 Proposed fix
- let bot_user_id = self - .bot_user_id - .get() - .map(|s| s.as_ref().to_string()) - .unwrap_or_default(); + let bot_user_id = self + .bot_user_id + .get() + .ok_or_else(|| anyhow::anyhow!("bot_user_id not initialized"))? + .as_ref() + .to_string();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/mattermost.rs` around lines 583 - 587, The code currently defaults bot_user_id to an empty string which causes opaque server errors when sending reactions; replace the unwrap_or_default usage on self.bot_user_id.get().map(|s| s.as_ref().to_string()).unwrap_or_default() with an explicit failure path: either call expect/panic with a clear message like "bot_user_id not initialized for reactions" or, preferably, change the surrounding handler to return a Result and use ok_or_else / map_err to return a descriptive error (e.g., "missing bot_user_id") so callers can handle it instead of sending an empty ID; update any call sites to handle the Result if you choose propagation.
308-317:⚠️ Potential issue | 🟠 MajorCheck
/users/meHTTP status before JSON parsing.On auth/config failures, this currently reports a parse error instead of surfacing status/body details.
#!/bin/bash # Verify /users/me path currently parses JSON directly rg -n -C8 'get\(self\.api_url\("/users/me"\)\)|failed to parse user response' src/messaging/mattermost.rs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/mattermost.rs` around lines 308 - 317, The current call to self.client.get(self.api_url("/users/me")).bearer_auth(self.token.as_ref()).send().await... then .json().await can mask non-2xx responses as JSON parse errors; modify the flow in the /users/me request (the block that assigns to let me: MattermostUser) to inspect the HTTP response status after send() and before json(): await the Response from send(), check response.status().is_success() (or use response.error_for_status()/error_for_status_ref()), and if not successful read the response text (response.text().await) and return an error/context that includes the status code and body; only call response.json().await when the status is successful so parse errors truly reflect JSON problems. Ensure you reference the same calls (self.client.get, api_url("/users/me"), bearer_auth, send, json) when making the change.
560-567:⚠️ Potential issue | 🟠 MajorFinal stream edit can exceed Mattermost max post length.
Line 564 edits with full
accumulated_text; large streams can fail finalization and leave placeholder/truncated content.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/mattermost.rs` around lines 560 - 567, The final stream edit in the OutboundResponse::StreamEnd branch uses active.accumulated_text directly and can exceed Mattermost's max post length; before calling self.edit_post(&active.post_id, &active.accumulated_text).await truncate or otherwise constrain active.accumulated_text to the Mattermost max (use a MAX_POST_LENGTH constant or a helper like truncate_to_max_post_length) and log a warning if truncation occurs, so edit_post receives a length-safe string and finalization won't fail.
1142-1149:⚠️ Potential issue | 🔴 CriticalUnsafe UTF-8 slicing can panic on non-ASCII input.
Line 1142 slices with
[..max_len]before finding a char boundary.#!/bin/bash # Verify split_message uses direct byte slicing before boundary correction rg -n -C4 'let search_region = &remaining\[\.\.max_len\];|floor_char_boundary' src/messaging/mattermost.rs💡 Proposed fix
- let search_region = &remaining[..max_len]; + let search_end = remaining.floor_char_boundary(max_len); + let search_region = &remaining[..search_end]; let break_point = search_region .rfind('\n') .or_else(|| search_region.rfind(' ')) - .unwrap_or(max_len); + .unwrap_or(search_end);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/mattermost.rs` around lines 1142 - 1149, The code currently does unsafe byte slicing with let search_region = &remaining[..max_len]; which can panic on UTF-8 boundaries; change it to compute a safe byte index for the desired maximum by using remaining.char_indices().nth(max_len).map(|(i,_)|i).unwrap_or(remaining.len()) (or similar) and then set search_region = &remaining[..safe_index]; keep using rfind to find break_point and pass that byte index into floor_char_boundary before slicing and pushing into chunks (update the logic around search_region, break_point, floor_char_boundary, and chunks.push to use the computed safe byte index rather than direct ..max_len slicing).
🧹 Nitpick comments (1)
src/messaging/mattermost.rs (1)
538-553: Avoid holdingactive_messageswrite lock across awaited network I/O.Line 552 awaits
edit_postwhile the write lock is held, which can stall other streams and status updates.💡 Proposed refactor
OutboundResponse::StreamChunk(chunk) => { - let mut active_messages = self.active_messages.write().await; - if let Some(active) = active_messages.get_mut(&message.id) { + let mut update: Option<(Arc<str>, String)> = None; + { + let mut active_messages = self.active_messages.write().await; + if let Some(active) = active_messages.get_mut(&message.id) { active.accumulated_text.push_str(&chunk); @@ - if let Err(error) = self.edit_post(&active.post_id, &display_text).await { - tracing::warn!(%error, "failed to edit streaming message"); - } + update = Some((active.post_id.clone(), display_text)); active.last_edit = Instant::now(); } } + if let Some((post_id, display_text)) = update + && let Err(error) = self.edit_post(&post_id, &display_text).await + { + tracing::warn!(%error, "failed to edit streaming message"); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/mattermost.rs` around lines 538 - 553, The code currently holds the active_messages write lock while awaiting self.edit_post, which can block other tasks; fix by extracting/cloning the needed data (e.g., clone active.post_id and active.accumulated_text or compute display_text) while holding the lock, then drop the lock before calling self.edit_post(&post_id, &display_text). After the await, reacquire the write lock to update active.last_edit (and any other state) only if the edit succeeded; use the same symbols active_messages, active.accumulated_text, active.post_id, edit_post, STREAM_EDIT_THROTTLE, and MAX_MESSAGE_LENGTH to locate and implement 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/messaging/mattermost.rs`:
- Around line 1067-1079: The identity lookup swallows network/JSON errors via
multiple .ok()? calls (e.g., the cache.read().await.get(user_id) path,
client.get(url).bearer_auth(token).send().await.ok()?, and
resp.json().await.ok()? when decoding MattermostUser and later in the similar
block at 1091-1103); replace those .ok()? chains with explicit error handling:
check resp.error_for_status() (or inspect resp.status()) and handle send() and
json() Result errors with matches, logging the error context (include user_id
and URL) or propagating a proper Result type instead of silently returning None,
and ensure the parsed MattermostUser (and its display_name()) is only used after
successful JSON deserialization.
---
Outside diff comments:
In `@src/config/types.rs`:
- Around line 1651-1659: The match on message.source (which sets mention_key) is
missing a branch for Mattermost, causing require_mention checks to drop
Mattermost messages; update the match in the code that defines mention_key (the
match on message.source.as_str()) to include a "mattermost" arm that maps to
"mattermost_mentions_or_replies_to_bot" so Mattermost inbound metadata is
recognized the same as other platforms.
- Around line 1543-1576: The generic channel filtering block using
self.channel_ids and the computed variables message_channel, slack_channel, and
twitch_channel is preventing Mattermost checks from running; update the channel
extraction and matching logic to also handle Mattermost by reading
message.metadata.get("mattermost_channel_id").and_then(|v| v.as_str()) (and
include a parent mattermost id if applicable) and include that value in the
direct_match boolean (similar to slack_channel/twitch_channel checks), and
ensure the same adjustment is applied to the later Mattermost-specific branch
referenced around the parent/channel checks (the code that computes
message_channel, parent_channel, direct_match, and parent_match).
---
Duplicate comments:
In `@src/messaging/mattermost.rs`:
- Around line 518-523: OutboundResponse::StreamStart currently extracts only
message.metadata["mattermost_root_id"] and calls start_typing without the same
thread fallback used elsewhere; update the StreamStart branch to follow the
text-path logic by checking message.metadata for "mattermost_root_id" and, if
absent, falling back to REPLY_TO_MESSAGE_ID before using that value for threaded
behavior (the same metadata lookup pattern used in the text response branch),
ensuring start_typing/channel handling uses the resolved thread id from
message.metadata.
- Around line 154-186: The start_typing function currently inserts a new
tokio::task::JoinHandle into typing_tasks without stopping any existing task for
the same channel; update start_typing to check typing_tasks (by channel_id) for
an existing handle, call abort() on it and optionally await its completion or
ignore the result before inserting the new handle so you don't leak duplicate
tasks; likewise modify the shutdown logic (the code that currently clears
typing_tasks) to instead iterate all stored JoinHandles, call abort() on each
handle (and await or detach as appropriate) before removing them so all
background typing tasks are explicitly terminated — refer to the start_typing
method, typing_tasks field and the shutdown function to locate the changes.
- Around line 659-669: The POST to /posts after file upload (the request built
with
self.client.post(self.api_url("/posts")).bearer_auth(self.token.as_ref()).json(...).send().await.context("failed
to create post with file")?) does not check HTTP status and can silently ignore
non-2xx responses; update the call to inspect the Response returned by send()
(e.g., assign to a variable), call response.error_for_status() or check
status().is_success(), and return a contextual error (including status and body
text) if not successful. Apply the same fix for the other /posts call that uses
the "failed to create post" context (the second occurrence around the other
upload path) so both paths surface non-success HTTP responses.
- Around line 762-764: The code currently discards the Result from sending the
shutdown signal with "let _ = tx.send(()).await" on the shutdown_tx channel;
change this to explicitly handle the Result from tx.send(()).await (e.g., use if
let Err(err) = tx.send(()).await { log the error with tracing::warn! or
tracing::error! } or, if it's intentionally safe to ignore a dropped receiver,
call .ok() on the Result) so you no longer silently discard errors; locate the
send call on the shutdown_tx (the shutdown_tx.write().await.take() ->
tx.send(()).await expression) and replace the "let _ =" pattern with explicit
logging or .ok() as appropriate.
- Around line 583-587: The code currently defaults bot_user_id to an empty
string which causes opaque server errors when sending reactions; replace the
unwrap_or_default usage on self.bot_user_id.get().map(|s|
s.as_ref().to_string()).unwrap_or_default() with an explicit failure path:
either call expect/panic with a clear message like "bot_user_id not initialized
for reactions" or, preferably, change the surrounding handler to return a Result
and use ok_or_else / map_err to return a descriptive error (e.g., "missing
bot_user_id") so callers can handle it instead of sending an empty ID; update
any call sites to handle the Result if you choose propagation.
- Around line 308-317: The current call to
self.client.get(self.api_url("/users/me")).bearer_auth(self.token.as_ref()).send().await...
then .json().await can mask non-2xx responses as JSON parse errors; modify the
flow in the /users/me request (the block that assigns to let me: MattermostUser)
to inspect the HTTP response status after send() and before json(): await the
Response from send(), check response.status().is_success() (or use
response.error_for_status()/error_for_status_ref()), and if not successful read
the response text (response.text().await) and return an error/context that
includes the status code and body; only call response.json().await when the
status is successful so parse errors truly reflect JSON problems. Ensure you
reference the same calls (self.client.get, api_url("/users/me"), bearer_auth,
send, json) when making the change.
- Around line 560-567: The final stream edit in the OutboundResponse::StreamEnd
branch uses active.accumulated_text directly and can exceed Mattermost's max
post length; before calling self.edit_post(&active.post_id,
&active.accumulated_text).await truncate or otherwise constrain
active.accumulated_text to the Mattermost max (use a MAX_POST_LENGTH constant or
a helper like truncate_to_max_post_length) and log a warning if truncation
occurs, so edit_post receives a length-safe string and finalization won't fail.
- Around line 1142-1149: The code currently does unsafe byte slicing with let
search_region = &remaining[..max_len]; which can panic on UTF-8 boundaries;
change it to compute a safe byte index for the desired maximum by using
remaining.char_indices().nth(max_len).map(|(i,_)|i).unwrap_or(remaining.len())
(or similar) and then set search_region = &remaining[..safe_index]; keep using
rfind to find break_point and pass that byte index into floor_char_boundary
before slicing and pushing into chunks (update the logic around search_region,
break_point, floor_char_boundary, and chunks.push to use the computed safe byte
index rather than direct ..max_len slicing).
---
Nitpick comments:
In `@src/messaging/mattermost.rs`:
- Around line 538-553: The code currently holds the active_messages write lock
while awaiting self.edit_post, which can block other tasks; fix by
extracting/cloning the needed data (e.g., clone active.post_id and
active.accumulated_text or compute display_text) while holding the lock, then
drop the lock before calling self.edit_post(&post_id, &display_text). After the
await, reacquire the write lock to update active.last_edit (and any other state)
only if the edit succeeded; use the same symbols active_messages,
active.accumulated_text, active.post_id, edit_post, STREAM_EDIT_THROTTLE, and
MAX_MESSAGE_LENGTH to locate and implement the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca599496-9b51-4c85-bc78-65074e5fafde
📒 Files selected for processing (2)
src/config/types.rssrc/messaging/mattermost.rs
mattermost.rs:
- Validate base_url has no path/query/fragment in new()
- Check /users/me HTTP status before .json() to surface auth errors
- start_typing() aborts previous task before spawning to prevent leaks
- shutdown() drains typing_tasks with abort() instead of clear()
- StreamStart now falls back to REPLY_TO_MESSAGE_ID for thread replies
- StreamEnd applies MAX_MESSAGE_LENGTH chunking to avoid oversized edits
- Reaction handler fails fast if bot_user_id not initialized
- respond() file attachment post checks HTTP status
- broadcast() file upload checks HTTP status
- broadcast() resolves dm:{user_id} targets via /channels/direct API
- split_message uses floor_char_boundary before slicing to prevent UTF-8 panics
- Identity/channel lookups log errors via tracing::debug instead of silent None
target.rs:
- resolve_broadcast_target extracts named adapter key from conversation ID
- normalize_mattermost_target strips team/instance prefixes down to bare channel_id or dm:{user_id}
api/messaging.rs:
- Validate mattermost base_url before persisting to config.toml
- Delete default Mattermost instance now clears all credential fields
config/load.rs:
- Named Mattermost instances with missing credentials are preserved but
disabled (map + enabled&&has_credentials) instead of silently dropped
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/messaging/mattermost.rs (2)
840-842:⚠️ Potential issue | 🟡 MinorDon’t silently discard shutdown signal errors.
let _ = tx.send(()).awaithides failure to notify the websocket task.As per coding guidelines: "Don't silently discard errors. No
let _ =on Results. Handle them, log them, or propagate them. The only exception is.ok()on channel sends where the receiver may be dropped"Suggested fix
if let Some(tx) = self.shutdown_tx.write().await.take() { - let _ = tx.send(()).await; + if let Err(error) = tx.send(()).await { + tracing::debug!(%error, adapter = %self.runtime_key, "shutdown receiver already closed"); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/mattermost.rs` around lines 840 - 842, The shutdown send currently discards errors with `let _ = tx.send(()).await;`; update the code around `self.shutdown_tx.write().await.take()` so the Result from `tx.send(()).await` is not silently ignored—handle the error by at minimum logging it (e.g., via the module logger or `error!`) or propagating it upward; ensure the failure to notify the websocket task is visible (include context like "failed to send shutdown signal to websocket task") and keep the existing `take()` behavior on `shutdown_tx`.
916-927:⚠️ Potential issue | 🟠 Major
broadcast()file uploads don’t validate/postsHTTP status.This path can silently report success even when Mattermost rejects the post (401/403/413/etc).
Suggested fix
- self.client + let post_response = self.client .post(self.api_url("/posts")) .bearer_auth(self.token.as_ref()) .json(&serde_json::json!({ "channel_id": target, "message": caption.unwrap_or_default(), "file_ids": file_ids, })) .send() .await .context("failed to create post with file")?; + let post_status = post_response.status(); + if !post_status.is_success() { + let body = post_response.text().await.unwrap_or_default(); + return Err(anyhow::anyhow!( + "mattermost POST /posts (broadcast file) failed with status {}: {body}", + post_status.as_u16() + ).into()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/mattermost.rs` around lines 916 - 927, The POST to /posts in broadcast() currently awaits send() but ignores the HTTP status; change it to capture the Response from self.client.post(self.api_url("/posts")).bearer_auth(...).json(...).send().await, then validate the status (e.g., use resp.error_for_status() or check resp.status().is_success()), and on non-success return a contextual error that includes the HTTP status and response body/text to surface Mattermost errors (401/403/413/etc.); update the error message passed to .context("failed to create post with file") to include these details.
🧹 Nitpick comments (2)
src/api/messaging.rs (2)
518-570: Renamemmto a descriptive variable name.Please avoid abbreviated local names here (e.g.,
mattermost_config) for readability and consistency with repo style.As per coding guidelines
**/*.rs: "Don't abbreviate variable names. Usequeuenotq,messagenotmsg,channelnotch. Common abbreviations likeconfigare fine".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/messaging.rs` around lines 518 - 570, The local variable `mm` in the Mattermost block is an unclear abbreviation; rename it to a descriptive identifier like `mattermost_config` (or similar) wherever it's used in this block (the binding from doc.get("messaging").and_then(|m| m.get("mattermost")), the checks for base_url/token/enabled, the `named_instances` extraction, and the loop that computes `instance_name`, `instance_enabled`, and `instance_configured`) so the code remains readable and consistent; update all references passed into functions such as push_instance_status and calls to normalize_adapter_selector and ensure no other symbol names change.
1331-1347: Deduplicate Mattermost base URL validation.The same validation block appears in both default and named instance paths. Extracting a helper will reduce drift risk.
♻️ Proposed refactor
+fn is_valid_mattermost_origin(base_url: &str) -> bool { + url::Url::parse(base_url) + .map(|parsed_url| { + parsed_url.path() == "/" + && parsed_url.query().is_none() + && parsed_url.fragment().is_none() + }) + .unwrap_or(false) +} ... - if let Some(url) = &credentials.mattermost_base_url { - if url::Url::parse(url) - .map(|u| u.path() != "/" || u.query().is_some() || u.fragment().is_some()) - .unwrap_or(true) - { + if let Some(url) = &credentials.mattermost_base_url { + if !is_valid_mattermost_origin(url) { return Ok(Json(MessagingInstanceActionResponse { success: false, message: format!("invalid mattermost base_url: must be an origin URL (e.g. https://mm.example.com)"), })); } platform_table["base_url"] = toml_edit::value(url.as_str()); } ... - if let Some(url) = &credentials.mattermost_base_url { - if url::Url::parse(url) - .map(|u| u.path() != "/" || u.query().is_some() || u.fragment().is_some()) - .unwrap_or(true) - { + if let Some(url) = &credentials.mattermost_base_url { + if !is_valid_mattermost_origin(url) { return Ok(Json(MessagingInstanceActionResponse { success: false, message: format!("invalid mattermost base_url: must be an origin URL (e.g. https://mm.example.com)"), })); } instance_table["base_url"] = toml_edit::value(url.as_str()); }Also applies to: 1457-1473
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/messaging.rs` around lines 1331 - 1347, Duplicate Mattermost base_url parsing/validation occurs in the "mattermost" branch (checking credentials.mattermost_base_url with url::Url::parse and the path/query/fragment conditions) and should be extracted into a small helper to avoid drift; add a function like validate_mattermost_base_url(url: &str) -> Result<(), String> (or similar) that encapsulates the parse and the condition (path == "/" && query.is_none() && fragment.is_none()), call that helper from both places, and on Err return the same Json(MessagingInstanceActionResponse { success: false, message: ... }) behavior; update places that currently set platform_table["base_url"] to use the validated url after the helper returns Ok.
🤖 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/mattermost.rs`:
- Around line 434-442: The code currently swallows JSON decode errors for
WebSocket posts (the chain that produces post_result from
MattermostWsEvent.data.get("post") and
serde_json::from_str::<MattermostPost>()); change this to explicitly handle and
log failures instead of using .ok(): extract the "post" string, attempt
serde_json::from_str in a match or if let Err(e) pattern, log the error with
context (including the raw text or post string and the error) and only proceed
when Ok(post) yields a MattermostPost; update references to post_result to use
the validated value so malformed payloads are visible in logs rather than
silently dropped.
- Around line 620-635: When handling OutboundResponse::StreamEnd, the overflow
chunks are being posted with root_id = None which breaks the thread; update the
create_post call inside the chunks loop to pass the original thread root (e.g.
Some(active.post_id) or Some(active.root_id) if the active message stores a
separate root_id) instead of None so overflow chunks remain in the same thread
as the edited post (adjust create_post invocation and any active struct field
used to hold the thread root).
In `@src/messaging/target.rs`:
- Around line 289-292: normalize_mattermost_target currently treats a
single-segment "dm" as a valid channel_id; update the normalization to reject
bare "dm" targets by ensuring the single-segment match (the branch handling
[channel_id]) also checks channel_id != "dm" (and non-empty) and returns None
for that case, and keep the existing two-segment ["dm", user_id] branch
unchanged so only "dm:<user_id>" forms are accepted; modify the match in
normalize_mattermost_target to add that explicit check.
- Around line 268-283: The function extract_mattermost_adapter_from_channel_id
is mis-matching default DM IDs (e.g. "mattermost:{team_id}:dm:{user_id}") as
named-instance IDs; update the match arms to inspect the third part (team_id)
instead of the fourth so only true named-instance IDs like ["mattermost",
instance, team_id, channel] (where team_id != "dm") and the 5-part
named-instance DM ["mattermost", instance, team_id, "dm", user_id] map to
"mattermost:{instance}", while default 3- or 4-part IDs with the third part ==
"dm" fall through to the default "mattermost" case; modify
extract_mattermost_adapter_from_channel_id accordingly so
resolve_broadcast_target no longer routes default DMs to a non-existent named
adapter.
---
Duplicate comments:
In `@src/messaging/mattermost.rs`:
- Around line 840-842: The shutdown send currently discards errors with `let _ =
tx.send(()).await;`; update the code around
`self.shutdown_tx.write().await.take()` so the Result from `tx.send(()).await`
is not silently ignored—handle the error by at minimum logging it (e.g., via the
module logger or `error!`) or propagating it upward; ensure the failure to
notify the websocket task is visible (include context like "failed to send
shutdown signal to websocket task") and keep the existing `take()` behavior on
`shutdown_tx`.
- Around line 916-927: The POST to /posts in broadcast() currently awaits send()
but ignores the HTTP status; change it to capture the Response from
self.client.post(self.api_url("/posts")).bearer_auth(...).json(...).send().await,
then validate the status (e.g., use resp.error_for_status() or check
resp.status().is_success()), and on non-success return a contextual error that
includes the HTTP status and response body/text to surface Mattermost errors
(401/403/413/etc.); update the error message passed to .context("failed to
create post with file") to include these details.
---
Nitpick comments:
In `@src/api/messaging.rs`:
- Around line 518-570: The local variable `mm` in the Mattermost block is an
unclear abbreviation; rename it to a descriptive identifier like
`mattermost_config` (or similar) wherever it's used in this block (the binding
from doc.get("messaging").and_then(|m| m.get("mattermost")), the checks for
base_url/token/enabled, the `named_instances` extraction, and the loop that
computes `instance_name`, `instance_enabled`, and `instance_configured`) so the
code remains readable and consistent; update all references passed into
functions such as push_instance_status and calls to normalize_adapter_selector
and ensure no other symbol names change.
- Around line 1331-1347: Duplicate Mattermost base_url parsing/validation occurs
in the "mattermost" branch (checking credentials.mattermost_base_url with
url::Url::parse and the path/query/fragment conditions) and should be extracted
into a small helper to avoid drift; add a function like
validate_mattermost_base_url(url: &str) -> Result<(), String> (or similar) that
encapsulates the parse and the condition (path == "/" && query.is_none() &&
fragment.is_none()), call that helper from both places, and on Err return the
same Json(MessagingInstanceActionResponse { success: false, message: ... })
behavior; update places that currently set platform_table["base_url"] to use the
validated url after the helper returns Ok.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9929b306-caea-415a-ad85-11cf9986a955
📒 Files selected for processing (4)
src/api/messaging.rssrc/config/load.rssrc/messaging/mattermost.rssrc/messaging/target.rs
Comment on lines
+434
to
+442
| if let Ok(event) = serde_json::from_str::<MattermostWsEvent>(&text) { | ||
| if event.event == "posted" { | ||
| // The post is double-encoded as a JSON string in the data field. | ||
| let post_result = event | ||
| .data | ||
| .get("post") | ||
| .and_then(|v| v.as_str()) | ||
| .and_then(|s| serde_json::from_str::<MattermostPost>(s).ok()); | ||
|
|
Contributor
There was a problem hiding this comment.
WebSocket post decode failures are dropped without diagnostics.
The .ok() parse chain at Line 441 silently discards malformed payloads; inbound events are lost with no trace.
As per coding guidelines: "Don't silently discard errors. No let _ = on Results. Handle them, log them, or propagate them. The only exception is .ok() on channel sends where the receiver may be dropped"
Suggested fix
- let post_result = event
- .data
- .get("post")
- .and_then(|v| v.as_str())
- .and_then(|s| serde_json::from_str::<MattermostPost>(s).ok());
+ let post_result = event
+ .data
+ .get("post")
+ .and_then(|v| v.as_str())
+ .map(|raw| serde_json::from_str::<MattermostPost>(raw));
- if let Some(mut post) = post_result {
+ if let Some(mut post) = match post_result {
+ Some(Ok(post)) => Some(post),
+ Some(Err(error)) => {
+ tracing::debug!(%error, "failed to parse Mattermost WS post payload");
+ None
+ }
+ None => None,
+ } {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/messaging/mattermost.rs` around lines 434 - 442, The code currently
swallows JSON decode errors for WebSocket posts (the chain that produces
post_result from MattermostWsEvent.data.get("post") and
serde_json::from_str::<MattermostPost>()); change this to explicitly handle and
log failures instead of using .ok(): extract the "post" string, attempt
serde_json::from_str in a match or if let Err(e) pattern, log the error with
context (including the raw text or post string and the error) and only proceed
when Ok(post) yields a MattermostPost; update references to post_result to use
the validated value so malformed payloads are visible in logs rather than
silently dropped.
Comment on lines
+620
to
+635
| OutboundResponse::StreamEnd => { | ||
| self.stop_typing(channel_id).await; | ||
| if let Some(active) = self.active_messages.write().await.remove(&message.id) { | ||
| let chunks = split_message(&active.accumulated_text, MAX_MESSAGE_LENGTH); | ||
| let mut first = true; | ||
| for chunk in chunks { | ||
| if first { | ||
| first = false; | ||
| if let Err(error) = self.edit_post(&active.post_id, &chunk).await { | ||
| tracing::warn!(%error, "failed to finalize streaming message"); | ||
| } | ||
| } else { | ||
| if let Err(error) = self.create_post(channel_id, &chunk, None).await { | ||
| tracing::warn!(%error, "failed to create overflow chunk for streaming message"); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Overflow stream chunks lose thread context.
At Line 632, additional chunks are posted with root_id = None, so long streamed replies can escape the original thread.
Suggested fix
OutboundResponse::StreamEnd => {
self.stop_typing(channel_id).await;
+ let root_id = message
+ .metadata
+ .get("mattermost_root_id")
+ .and_then(|v| v.as_str())
+ .or_else(|| {
+ message
+ .metadata
+ .get(crate::metadata_keys::REPLY_TO_MESSAGE_ID)
+ .and_then(|v| v.as_str())
+ });
if let Some(active) = self.active_messages.write().await.remove(&message.id) {
let chunks = split_message(&active.accumulated_text, MAX_MESSAGE_LENGTH);
let mut first = true;
for chunk in chunks {
if first {
@@
} else {
- if let Err(error) = self.create_post(channel_id, &chunk, None).await {
+ if let Err(error) = self.create_post(channel_id, &chunk, root_id).await {
tracing::warn!(%error, "failed to create overflow chunk for streaming message");
}
}
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| OutboundResponse::StreamEnd => { | |
| self.stop_typing(channel_id).await; | |
| if let Some(active) = self.active_messages.write().await.remove(&message.id) { | |
| let chunks = split_message(&active.accumulated_text, MAX_MESSAGE_LENGTH); | |
| let mut first = true; | |
| for chunk in chunks { | |
| if first { | |
| first = false; | |
| if let Err(error) = self.edit_post(&active.post_id, &chunk).await { | |
| tracing::warn!(%error, "failed to finalize streaming message"); | |
| } | |
| } else { | |
| if let Err(error) = self.create_post(channel_id, &chunk, None).await { | |
| tracing::warn!(%error, "failed to create overflow chunk for streaming message"); | |
| } | |
| } | |
| OutboundResponse::StreamEnd => { | |
| self.stop_typing(channel_id).await; | |
| let root_id = message | |
| .metadata | |
| .get("mattermost_root_id") | |
| .and_then(|v| v.as_str()) | |
| .or_else(|| { | |
| message | |
| .metadata | |
| .get(crate::metadata_keys::REPLY_TO_MESSAGE_ID) | |
| .and_then(|v| v.as_str()) | |
| }); | |
| if let Some(active) = self.active_messages.write().await.remove(&message.id) { | |
| let chunks = split_message(&active.accumulated_text, MAX_MESSAGE_LENGTH); | |
| let mut first = true; | |
| for chunk in chunks { | |
| if first { | |
| first = false; | |
| if let Err(error) = self.edit_post(&active.post_id, &chunk).await { | |
| tracing::warn!(%error, "failed to finalize streaming message"); | |
| } | |
| } else { | |
| if let Err(error) = self.create_post(channel_id, &chunk, root_id).await { | |
| tracing::warn!(%error, "failed to create overflow chunk for streaming message"); | |
| } | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/messaging/mattermost.rs` around lines 620 - 635, When handling
OutboundResponse::StreamEnd, the overflow chunks are being posted with root_id =
None which breaks the thread; update the create_post call inside the chunks loop
to pass the original thread root (e.g. Some(active.post_id) or
Some(active.root_id) if the active message stores a separate root_id) instead of
None so overflow chunks remain in the same thread as the edited post (adjust
create_post invocation and any active struct field used to hold the thread
root).
Comment on lines
+268
to
+283
| fn extract_mattermost_adapter_from_channel_id(channel_id: &str) -> String { | ||
| // Named instance conv IDs: "mattermost:{instance}:{team_id}:{channel_id}" (4 parts) | ||
| // or: "mattermost:{instance}:{team_id}:dm:{user_id}" (5 parts) | ||
| // Default conv IDs: "mattermost:{team_id}:{channel_id}" (3 parts) | ||
| // or: "mattermost:{team_id}:dm:{user_id}" (4 parts, 3rd part = "dm") | ||
| let parts: Vec<&str> = channel_id.split(':').collect(); | ||
| match parts.as_slice() { | ||
| ["mattermost", instance, _, channel_or_team] if *channel_or_team != "dm" => { | ||
| format!("mattermost:{instance}") | ||
| } | ||
| ["mattermost", instance, _, "dm", _] => { | ||
| format!("mattermost:{instance}") | ||
| } | ||
| _ => "mattermost".to_string(), | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Default Mattermost DM IDs are being routed to a non-existent named adapter.
At Line 275, mattermost:{team_id}:dm:{user_id} matches the named-instance arm and returns mattermost:{team_id}. Downstream, this breaks default-adapter DM broadcasts in resolve_broadcast_target().
Suggested fix
fn extract_mattermost_adapter_from_channel_id(channel_id: &str) -> String {
let parts: Vec<&str> = channel_id.split(':').collect();
match parts.as_slice() {
- ["mattermost", instance, _, channel_or_team] if *channel_or_team != "dm" => {
- format!("mattermost:{instance}")
- }
+ // Default DM: mattermost:{team_id}:dm:{user_id}
+ ["mattermost", _, "dm", _] => "mattermost".to_string(),
+ // Named DM: mattermost:{instance}:{team_id}:dm:{user_id}
["mattermost", instance, _, "dm", _] => {
format!("mattermost:{instance}")
}
+ // Named channel: mattermost:{instance}:{team_id}:{channel_id}
+ ["mattermost", instance, _, _] => format!("mattermost:{instance}"),
_ => "mattermost".to_string(),
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/messaging/target.rs` around lines 268 - 283, The function
extract_mattermost_adapter_from_channel_id is mis-matching default DM IDs (e.g.
"mattermost:{team_id}:dm:{user_id}") as named-instance IDs; update the match
arms to inspect the third part (team_id) instead of the fourth so only true
named-instance IDs like ["mattermost", instance, team_id, channel] (where
team_id != "dm") and the 5-part named-instance DM ["mattermost", instance,
team_id, "dm", user_id] map to "mattermost:{instance}", while default 3- or
4-part IDs with the third part == "dm" fall through to the default "mattermost"
case; modify extract_mattermost_adapter_from_channel_id accordingly so
resolve_broadcast_target no longer routes default DMs to a non-existent named
adapter.
Comment on lines
+289
to
+292
| // Already bare: "channel_id" or "dm:user_id" | ||
| [channel_id] if !channel_id.is_empty() => Some((*channel_id).to_string()), | ||
| ["dm", user_id] if !user_id.is_empty() => Some(format!("dm:{user_id}")), | ||
| // With team prefix: "team_id:channel_id" or "team_id:dm:user_id" |
Contributor
There was a problem hiding this comment.
Reject bare mattermost:dm targets during normalization.
normalize_mattermost_target() currently accepts "dm" as a channel ID (Line 290), which is invalid and later fails at send time.
Suggested fix
- [channel_id] if !channel_id.is_empty() => Some((*channel_id).to_string()),
+ [channel_id] if !channel_id.is_empty() && *channel_id != "dm" => {
+ Some((*channel_id).to_string())
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/messaging/target.rs` around lines 289 - 292, normalize_mattermost_target
currently treats a single-segment "dm" as a valid channel_id; update the
normalization to reject bare "dm" targets by ensuring the single-segment match
(the branch handling [channel_id]) also checks channel_id != "dm" (and
non-empty) and returns None for that case, and keep the existing two-segment
["dm", user_id] branch unchanged so only "dm:<user_id>" forms are accepted;
modify the match in normalize_mattermost_target to add that explicit check.
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
src/messaging/mattermost.rs)config.tomlshow up automaticallyWhat's included
Adapter (
src/messaging/mattermost.rs)data.postfield — parsed correctlyStreamEndmattermost:{team_id}:{channel_id}for channels,mattermost:{team_id}:dm:{user_id}for DMsteam_idis absent but filters are configuredmetadata_keys::MESSAGE_IDon inbound messages for reply threadingArc<ArcSwap<MattermostPermissions>>in the file watcherConfig (
src/config/)MattermostConfig/MattermostInstanceConfigwith token-redactingDebugimplsMattermostPermissionswithfrom_config,from_instance_config,from_bindings_for_adapter[messaging.mattermost]TOML section:base_url,token, optionalteam_id,max_attachment_bytes[[messaging.mattermost.instances]]for named instancesteam_idfield onBindingfor channel-level permission scopingSettings UI (
interface/)src/api/messaging.rs: reads[messaging.mattermost]into the instances list; writes credentials to TOML via the create-instance endpointConfig example
Test plan
config.toml— verify instance appears in Settings UIconfig.toml[[messaging.mattermost.instances]]) connect independentlyteam_idin the event is rejected when filters are configured (fail-closed)I based this on the Slack integrations. Written with the help of both Claude and Gemini. Would appreciate review and test feedback.
Note
Introduces full Mattermost messaging adapter with WebSocket event handling, TOML-based configuration, and hot-reloadable permission filtering. Spans 19 files across adapter implementation, config structures, and UI integration (+3400 lines). Follows existing Slack adapter patterns for consistency.
Written by Tembo for commit b069b56.