feat: add email messaging adapter and setup docs#244
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds first-class Email support: new EmailConfig and EmailAdapter (IMAP polling + SMTP sending), API and frontend changes for email bindings, prompt/template and tooling adjustments for adapter-aware behavior, conversation metadata for email, and accompanying documentation and tests. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 |
![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: 8
🧹 Nitpick comments (3)
src/messaging/target.rs (1)
213-231: Consider reusing the existing email target normalizer to avoid drift.There is parallel email normalization logic in
src/messaging/email.rs(Line 1223-Line 1239). Keeping two parsers will drift on edge cases (mailbox lists, encoded names, etc.).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/target.rs` around lines 213 - 231, The normalize_email_target function duplicates parsing already implemented in src/messaging/email.rs (the email normalizer around lines 1223–1239); replace the local parsing with a call to that existing normalizer: remove the duplicated logic inside normalize_email_target and delegate to the email module's function (make it public or adjust the import path if necessary), returning its Option<String> result and preserving the same signature; ensure you update imports to reference the existing function and run tests to confirm behavior matches the canonical normalizer.src/messaging/email.rs (2)
521-524: Use.ok()for channel send instead oflet _ =.The coding guidelines state: "Only exception is
.ok()on channel sends where the receiver may be dropped." Line 523 useslet _ =which doesn't align with this pattern.♻️ Suggested fix
if let Some(shutdown_tx) = self.shutdown_tx.write().await.take() { - let _ = shutdown_tx.send(true); + shutdown_tx.send(true).ok(); }As per coding guidelines: "Don't silently discard errors; no let _ = on Results. Handle, log, or propagate errors. 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/email.rs` around lines 521 - 524, In the async shutdown method, replace the silent discard of the channel send result (the use of "let _ = shutdown_tx.send(true);") with the approved pattern for dropped receivers by calling .ok() on the Result; locate the shutdown function and the shutdown_tx.send(true) call and change it so the send result uses .ok() instead of using let _ = to discard the Result.
1224-1268: json_value_to_string is duplicated identically; normalize_email_target implementations differ by design.
json_value_to_string(lines 1257–1268) is identical to the version insrc/messaging/target.rs(line 242) and could be extracted to a shared utility to avoid duplication.However,
normalize_email_target(lines 1224–1240) uses a different implementation thantarget.rs. Theemail.rsversion prioritizesparse_primary_mailbox()for extracting addresses from headers, whiletarget.rsusesrsplit_once('<')to handle angle-bracket-formatted targets. These different parsing strategies appear intentional for their respective modules, so consolidation is not applicable here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/email.rs` around lines 1224 - 1268, Duplicate function json_value_to_string should be centralized: create a single shared utility function (e.g., json_value_to_string) in a common module and replace the duplicated implementations in both email.rs and target.rs with a call/import to that shared function; update all callers to use the new shared symbol and remove the duplicate definitions from email.rs and target.rs so only the new centralized json_value_to_string remains.
🤖 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:
- Line 634: Update the bindings docs row for the `channel` field to include all
currently supported platforms by adding `slack` and `twitch` to the list
alongside the existing `discord`, `telegram`, `email`, and `webhook`; locate the
table entry that defines the `channel` parameter (the line containing |
`channel` | string | **required** | ...) and edit the platform list so it reads
something like `discord, telegram, email, webhook, slack, twitch` to reflect
current support.
In `@interface/src/components/ChannelSettingCard.tsx`:
- Around line 228-244: The port parsing for email_imap_port/email_smtp_port
(variables parsedImapPort/parsedSmtpPort) is too permissive; replace the current
Number.parseInt usage with a strict numeric validation (e.g., ensure
credentialInputs.email_imap_port/email_smtp_port are non-empty trimmed strings
matching /^\d+$/), convert to Number, then enforce range checks (1..65535)
before assigning into request.platform_credentials.email_imap_port and
email_smtp_port; if validation fails, set the port fields to undefined or
surface a validation error rather than silently accepting partial/invalid input.
In `@src/api/bindings.rs`:
- Around line 292-315: The password fields email_imap_password and
email_smtp_password are being trimmed which can corrupt intentional
leading/trailing whitespace; update the binding logic in bindings.rs to preserve
these password values exactly by removing the .trim() call (keep the
as_deref().unwrap_or("").to_string() flow) for email_imap_password and
email_smtp_password while leaving other fields unchanged.
In `@src/config.rs`:
- Around line 1527-1548: The Debug implementation for EmailConfig is leaking
email identifiers (imap_username, smtp_username, from_address, allowed_senders);
update the fmt method (impl std::fmt::Debug for EmailConfig -> fn fmt) to redact
or replace those fields with a placeholder like "[REDACTED]" (or omit them)
instead of logging their real values, leaving other fields unchanged and keeping
imap_password/smtp_password already redacted.
- Around line 3908-3948: The email field resolution currently prefers TOML
values then falls back to env vars; change each field resolution in the closure
that builds email (imap_host, imap_username, imap_password, smtp_host,
smtp_username, smtp_password, from_address) to check environment variables first
(std::env::var("EMAIL_*").ok()) and only if missing, use the TOML value
processed with as_deref().and_then(resolve_env_value). Preserve the existing
fallback/unwrap_or_else behavior for smtp_username/smtp_password/from_address
(e.g., keep using
imap_username.clone()/imap_password.clone()/smtp_username.clone()), but flip the
order so env var wins before the toml.email.* lookup.
In `@src/messaging/email.rs`:
- Around line 369-379: OutboundResponse::ScheduledMessage currently discards the
send_at timestamp and calls send_email immediately; update the handler for
OutboundResponse::ScheduledMessage to explicitly handle unsupported scheduling
by logging a warning that includes the send_at timestamp (and recipient/subject
for context) before calling send_email, or alternatively return an Err to
surface that scheduled delivery is unsupported; locate the match arm for
OutboundResponse::ScheduledMessage and modify it to emit a clear warning via the
existing logger/tracing facility and then either proceed to call
self.send_email(...) or return an appropriate error result so the behavior is
explicit.
- Around line 539-554: In build_smtp_transport: do not use
AsyncSmtpTransport::builder_dangerous when smtp_use_starttls is false because it
creates an unencrypted connection; instead call
AsyncSmtpTransport::relay(&config.smtp_host) (similar to starttls_relay) so the
client uses implicit TLS/SMTPS and performs certificate validation, propagate
any errors with with_context (e.g., same message used for starttls_relay) and
keep setting port/credentials as before (respecting config.smtp_port and
smtp_username/smtp_password).
In `@src/messaging/target.rs`:
- Around line 104-117: The fallback currently returns reply_to.or(from)? which
accepts malformed reply addresses; update the "email" arm so reply_to is only
used when it contains a syntactically valid email. After extracting reply_to and
from (variables reply_to and from produced via json_value_to_string),
filter/validate reply_to with your email validation helper (e.g., is_valid_email
or validate_email_address) before OR-ing with from, i.e. use reply_to.filter(|s|
is_valid_email(s)).or(from)? so malformed email_reply_to values are skipped and
the valid email_from is used as fallback.
---
Nitpick comments:
In `@src/messaging/email.rs`:
- Around line 521-524: In the async shutdown method, replace the silent discard
of the channel send result (the use of "let _ = shutdown_tx.send(true);") with
the approved pattern for dropped receivers by calling .ok() on the Result;
locate the shutdown function and the shutdown_tx.send(true) call and change it
so the send result uses .ok() instead of using let _ = to discard the Result.
- Around line 1224-1268: Duplicate function json_value_to_string should be
centralized: create a single shared utility function (e.g.,
json_value_to_string) in a common module and replace the duplicated
implementations in both email.rs and target.rs with a call/import to that shared
function; update all callers to use the new shared symbol and remove the
duplicate definitions from email.rs and target.rs so only the new centralized
json_value_to_string remains.
In `@src/messaging/target.rs`:
- Around line 213-231: The normalize_email_target function duplicates parsing
already implemented in src/messaging/email.rs (the email normalizer around lines
1223–1239); replace the local parsing with a call to that existing normalizer:
remove the duplicated logic inside normalize_email_target and delegate to the
email module's function (make it public or adjust the import path if necessary),
returning its Option<String> result and preserving the same signature; ensure
you update imports to reference the existing function and run tests to confirm
behavior matches the canonical normalizer.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lock,!**/*.lockCargo.tomlis excluded by!**/*.tomldocs/content/docs/(messaging)/meta.jsonis excluded by!**/*.json
📒 Files selected for processing (16)
docs/content/docs/(configuration)/config.mdxdocs/content/docs/(deployment)/roadmap.mdxdocs/content/docs/(messaging)/email-setup.mdxdocs/content/docs/(messaging)/messaging.mdxinterface/src/api/client.tsinterface/src/components/ChannelEditModal.tsxinterface/src/components/ChannelSettingCard.tsxinterface/src/routes/Settings.tsxsrc/api/bindings.rssrc/api/messaging.rssrc/config.rssrc/conversation/channels.rssrc/main.rssrc/messaging.rssrc/messaging/email.rssrc/messaging/target.rs
💤 Files with no reviewable changes (1)
- docs/content/docs/(deployment)/roadmap.mdx
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
src/messaging/email.rs
Outdated
| } | ||
| } | ||
|
|
||
| if let Err(error) = session.uid_store(&uid_sequence, "+FLAGS (\\Seen)") { |
Contributor
There was a problem hiding this comment.
Looks like we mark the UID as \\Seen even when parse_inbound_email returns Err. That means a transient parse bug or unexpected MIME can permanently drop the message. Consider only marking \\Seen after a successful parse (or intentional skip), and leaving it unseen on parse errors so it retries.
| ) | ||
| .await?; | ||
| } | ||
| OutboundResponse::File { |
Contributor
There was a problem hiding this comment.
max_attachment_bytes is stored on the adapter but not enforced here (or in broadcast). If OutboundResponse::File can be user-influenced, it’s easy to end up sending multi-MB payloads and/or hitting SMTP limits—worth checking data.len() and returning an error (or skipping the attachment) when it exceeds the configured max.
src/messaging/email.rs
Outdated
| continue; | ||
| } | ||
|
|
||
| let criterion = format!("HEADER Message-ID \"{search_id}\""); |
Contributor
There was a problem hiding this comment.
message_id ultimately comes from untrusted headers; building an IMAP SEARCH string with it unescaped can break the query (quotes/newlines) and makes the search criterion harder to reason about. Might be worth either escaping \"/\\/CRLF or just skipping IDs with those characters before calling uid_search.
src/api/messaging.rs
Outdated
| .and_then(|v| v.as_str()) | ||
| .is_some_and(|s| !s.is_empty()); | ||
|
|
||
| let configured = has_imap_host |
Contributor
There was a problem hiding this comment.
Minor mismatch with Config::load_from_path: smtp_username/smtp_password/from_address can be omitted in the TOML and default to the IMAP creds / SMTP username, but configured here requires them to be explicitly present. That will show Email as “not configured” in the UI even though runtime config resolves fine.
|
|
||
| request.platform_credentials = { | ||
| email_imap_host: credentialInputs.email_imap_host.trim(), | ||
| email_imap_port: Number.isFinite(parsedImapPort) && parsedImapPort > 0 ? parsedImapPort : undefined, |
Contributor
There was a problem hiding this comment.
email_imap_port deserializes into a Rust u16 on the server; clamping here avoids sending values like 999999 that will fail request parsing.
Suggested change
| email_imap_port: Number.isFinite(parsedImapPort) && parsedImapPort > 0 ? parsedImapPort : undefined, | |
| email_imap_port: Number.isFinite(parsedImapPort) && parsedImapPort > 0 && parsedImapPort <= 65535 ? parsedImapPort : undefined, |
| email_imap_username: credentialInputs.email_imap_username.trim(), | ||
| email_imap_password: credentialInputs.email_imap_password.trim(), | ||
| email_smtp_host: credentialInputs.email_smtp_host.trim(), | ||
| email_smtp_port: Number.isFinite(parsedSmtpPort) && parsedSmtpPort > 0 ? parsedSmtpPort : undefined, |
Contributor
There was a problem hiding this comment.
Same for email_smtp_port.
Suggested change
| email_smtp_port: Number.isFinite(parsedSmtpPort) && parsedSmtpPort > 0 ? parsedSmtpPort : undefined, | |
| email_smtp_port: Number.isFinite(parsedSmtpPort) && parsedSmtpPort > 0 && parsedSmtpPort <= 65535 ? parsedSmtpPort : undefined, |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/messaging/email.rs (2)
1223-1239:normalize_email_targetoverlaps withsrc/messaging/target.rsbut differs in implementation.This version uses
parse_primary_mailboxfor more robust parsing, whiletarget.rsuses string manipulation. Consider consolidating—this implementation appears more correct for handling display names like"Alice <alice@example.com>".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/email.rs` around lines 1223 - 1239, The normalize_email_target implementation is more robust (it uses parse_primary_mailbox to handle display-name forms like "Alice <alice@example.com>") and overlaps with the simpler string-based implementation in target.rs; consolidate by centralizing email normalization into a single helper (keep normalize_email_target and its use of parse_primary_mailbox) and update the duplicate function in src/messaging/target.rs to call normalize_email_target (or remove the duplicate and export normalize_email_target for reuse), ensuring all callers (including any functions in target.rs) use the same behavior and parsing logic.
1256-1267: Duplicate helper:json_value_to_stringalready exists insrc/messaging/target.rs.This function is identical to the one in
target.rs(lines 241-252). Consider extracting to a shared utility module or importing fromtarget.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/email.rs` around lines 1256 - 1267, The function json_value_to_string is duplicated in src/messaging/email.rs and src/messaging/target.rs; remove the duplicate and consolidate it into a single shared location (e.g., create a new util module messaging::utils with pub fn json_value_to_string or make the existing json_value_to_string in target.rs pub and re-export it), then update email.rs to import and call that shared function instead of defining its own; ensure the symbol name stays json_value_to_string so callers in both email.rs and target.rs compile unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/messaging/email.rs`:
- Around line 1223-1239: The normalize_email_target implementation is more
robust (it uses parse_primary_mailbox to handle display-name forms like "Alice
<alice@example.com>") and overlaps with the simpler string-based implementation
in target.rs; consolidate by centralizing email normalization into a single
helper (keep normalize_email_target and its use of parse_primary_mailbox) and
update the duplicate function in src/messaging/target.rs to call
normalize_email_target (or remove the duplicate and export
normalize_email_target for reuse), ensuring all callers (including any functions
in target.rs) use the same behavior and parsing logic.
- Around line 1256-1267: The function json_value_to_string is duplicated in
src/messaging/email.rs and src/messaging/target.rs; remove the duplicate and
consolidate it into a single shared location (e.g., create a new util module
messaging::utils with pub fn json_value_to_string or make the existing
json_value_to_string in target.rs pub and re-export it), then update email.rs to
import and call that shared function instead of defining its own; ensure the
symbol name stays json_value_to_string so callers in both email.rs and target.rs
compile unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
src/config.rs (1)
3908-3963: Add focused regression tests for email resolution precedence/fallbacks.This resolver now has multiple branches (env-over-TOML plus SMTP/from fallbacks). Add unit tests around those branches to prevent future precedence regressions.
Based on learnings, "Use config resolution order: env > DB > default, with per-subsystem resolve() methods".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.rs` around lines 3908 - 3963, Add focused unit tests for EmailConfig resolution to cover all precedence/fallback branches: test that environment variables (EMAIL_IMAP_HOST, EMAIL_IMAP_USERNAME, EMAIL_IMAP_PASSWORD, EMAIL_SMTP_HOST, EMAIL_SMTP_USERNAME, EMAIL_SMTP_PASSWORD, EMAIL_FROM_ADDRESS, EMAIL_FROM_NAME) override TOML values (toml.messaging.email), that smtp_username/smtp_password fall back to imap_username/imap_password when missing, that from_address falls back to smtp_username, that folders defaults to ["INBOX"] when empty, and that other fields (imap_port/imap_use_tls/smtp_port/smtp_use_starttls/poll_interval_secs/allowed_senders/max_body_bytes/max_attachment_bytes) are preserved from TOML; call into the same resolution logic used in the code (the EmailConfig construction path that uses resolve_env_value and the toml.messaging.email branch) so tests exercise resolve_env_value, EmailConfig, and the env-over-TOML precedence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/src/components/ChannelSettingCard.tsx`:
- Around line 217-226: The silent early return in ChannelSettingCard when
required email fields (credentialInputs.email_imap_host, email_imap_username,
email_imap_password, email_smtp_host, email_smtp_username, email_smtp_password,
email_from_address) are missing should instead surface a visible validation
error; locate the if-block that checks these credentialInputs and replace the
bare return with logic that sets a user-visible error (e.g., call an existing
error/toast helper or add a local state like setValidationError and populate it
with a clear message such as "Please fill in all required email fields"), ensure
the submit/save path remains blocked when the validation error exists, and make
sure the component UI renders that validation message so users see why the
action was prevented.
In `@src/api/bindings.rs`:
- Around line 581-591: The code currently swallows adapter build/start failures
(in the EmailAdapter::from_config and manager.register_and_start paths) and only
logs errors, causing the API to return "Binding created and active."; change the
control flow so that on Err(error) from EmailAdapter::from_config or when
manager.register_and_start(adapter).await returns Err(error) you do not mark the
binding as active — instead propagate the error into the API response (e.g., set
success=false and an appropriate message/status field indicating startup failed
or return an Err result) so clients receive a non-active/failure response;
update the matching logic around EmailAdapter::from_config and the
manager.register_and_start call (and the analogous block at lines 625-629) to
construct and return the failure response rather than only logging with
tracing::error!.
- Around line 280-362: The code currently ignores partial email credential
payloads; update the block that reads email fields (email_imap_host,
email_imap_username, email_imap_password, email_smtp_host, email_smtp_username,
email_smtp_password, email_from_address) to explicitly validate: compute a count
or boolean flags for which required fields are present (non-empty) and if some
but not all required fields are provided, return a Bad Request error
(StatusCode::BAD_REQUEST) instead of silently skipping; otherwise proceed to
configure the toml as before and set new_email_configured = true. Locate and
modify the logic around the existing variables/email table handling (e.g., the
variables named email_imap_host, email_imap_username, email_imap_password,
email_smtp_host, email_smtp_username, email_smtp_password, email_from_address
and the new_email_configured assignment) to implement this explicit
partial-payload rejection.
In `@src/api/messaging.rs`:
- Around line 427-438: The current toggle handling for "email" logs errors from
EmailAdapter::from_config and manager.register_and_start but still proceeds to
the success response; update the "email" arm so that on Err from
EmailAdapter::from_config or on Err from
manager.register_and_start(adapter).await you short-circuit and return an
appropriate error response (or propagate the error) instead of falling through
to the success path—modify the match arms around EmailAdapter::from_config and
the inner register_and_start call to return early (e.g., Err(...) or an HTTP
500/error response) when failures occur so the UI/config state reflects the
actual runtime result.
In `@src/messaging/email.rs`:
- Line 982: Replace the silent discard of the logout Result at the
session.logout() call with explicit error logging: instead of
session.logout().ok(), check the Result (e.g., if let Err(e) = session.logout()
or match session.logout()) and call the project's logger (e.g., error! or
tracing::error!) to record a descriptive message including the error value;
update all similar session.logout() occurrences in src/messaging/email.rs so
logout failures are logged rather than ignored.
- Line 639: Replace the silent discard of session.logout() with explicit error
logging: instead of session.logout().ok(); call session.logout() and if it
returns Err(e) log the error (e.g., using the project's logger macro like error!
or tracing::error!) with context such as "email session logout failed" and the
error variable; locate the session.logout() call in the same function where
health_check() had the similar issue and use the same logging style used
elsewhere in src/messaging/email.rs to keep consistency.
- Line 526: The call session.logout().ok() silently discards any logout error;
change it to capture the Result returned by session.logout() and log failures
instead of ignoring them (e.g., if let Err(e) = session.logout() {
<logger>.error!("IMAP logout failed: {}", e); }), keeping successful path
unchanged; update the same location where session.logout().ok() appears so you
reference the session.logout() call and use the existing logging facility in
this module (tracing/log or the module's logger) to record the error.
---
Nitpick comments:
In `@src/config.rs`:
- Around line 3908-3963: Add focused unit tests for EmailConfig resolution to
cover all precedence/fallback branches: test that environment variables
(EMAIL_IMAP_HOST, EMAIL_IMAP_USERNAME, EMAIL_IMAP_PASSWORD, EMAIL_SMTP_HOST,
EMAIL_SMTP_USERNAME, EMAIL_SMTP_PASSWORD, EMAIL_FROM_ADDRESS, EMAIL_FROM_NAME)
override TOML values (toml.messaging.email), that smtp_username/smtp_password
fall back to imap_username/imap_password when missing, that from_address falls
back to smtp_username, that folders defaults to ["INBOX"] when empty, and that
other fields
(imap_port/imap_use_tls/smtp_port/smtp_use_starttls/poll_interval_secs/allowed_senders/max_body_bytes/max_attachment_bytes)
are preserved from TOML; call into the same resolution logic used in the code
(the EmailConfig construction path that uses resolve_env_value and the
toml.messaging.email branch) so tests exercise resolve_env_value, EmailConfig,
and the env-over-TOML precedence.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/content/docs/(configuration)/config.mdxinterface/src/components/ChannelSettingCard.tsxsrc/api/bindings.rssrc/api/messaging.rssrc/config.rssrc/messaging/email.rssrc/messaging/target.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/messaging/target.rs
- docs/content/docs/(configuration)/config.mdx
Comment on lines
+217
to
+226
| if ( | ||
| !credentialInputs.email_imap_host?.trim() || | ||
| !credentialInputs.email_imap_username?.trim() || | ||
| !credentialInputs.email_imap_password || | ||
| !credentialInputs.email_smtp_host?.trim() || | ||
| !credentialInputs.email_smtp_username?.trim() || | ||
| !credentialInputs.email_smtp_password || | ||
| !credentialInputs.email_from_address?.trim() | ||
| ) | ||
| return; |
Contributor
There was a problem hiding this comment.
Show a validation error when required email fields are missing.
This branch exits silently when a required field is blank, so the action can appear broken to users.
Proposed fix
} else if (platform === "email") {
- if (
+ const missingRequired =
!credentialInputs.email_imap_host?.trim() ||
!credentialInputs.email_imap_username?.trim() ||
!credentialInputs.email_imap_password ||
!credentialInputs.email_smtp_host?.trim() ||
!credentialInputs.email_smtp_username?.trim() ||
!credentialInputs.email_smtp_password ||
!credentialInputs.email_from_address?.trim()
- )
+ ;
+ if (missingRequired) {
+ setMessage({
+ text: "Please provide IMAP/SMTP host, username, password, and From Address.",
+ type: "error",
+ });
return;
+ }📝 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
| if ( | |
| !credentialInputs.email_imap_host?.trim() || | |
| !credentialInputs.email_imap_username?.trim() || | |
| !credentialInputs.email_imap_password || | |
| !credentialInputs.email_smtp_host?.trim() || | |
| !credentialInputs.email_smtp_username?.trim() || | |
| !credentialInputs.email_smtp_password || | |
| !credentialInputs.email_from_address?.trim() | |
| ) | |
| return; | |
| const missingRequired = | |
| !credentialInputs.email_imap_host?.trim() || | |
| !credentialInputs.email_imap_username?.trim() || | |
| !credentialInputs.email_imap_password || | |
| !credentialInputs.email_smtp_host?.trim() || | |
| !credentialInputs.email_smtp_username?.trim() || | |
| !credentialInputs.email_smtp_password || | |
| !credentialInputs.email_from_address?.trim() | |
| ; | |
| if (missingRequired) { | |
| setMessage({ | |
| text: "Please provide IMAP/SMTP host, username, password, and From Address.", | |
| type: "error", | |
| }); | |
| return; | |
| } |
🤖 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 217 - 226, The
silent early return in ChannelSettingCard when required email fields
(credentialInputs.email_imap_host, email_imap_username, email_imap_password,
email_smtp_host, email_smtp_username, email_smtp_password, email_from_address)
are missing should instead surface a visible validation error; locate the
if-block that checks these credentialInputs and replace the bare return with
logic that sets a user-visible error (e.g., call an existing error/toast helper
or add a local state like setValidationError and populate it with a clear
message such as "Please fill in all required email fields"), ensure the
submit/save path remains blocked when the validation error exists, and make sure
the component UI renders that validation message so users see why the action was
prevented.
Comment on lines
+280
to
+362
| let email_imap_host = credentials | ||
| .email_imap_host | ||
| .as_deref() | ||
| .unwrap_or("") | ||
| .trim() | ||
| .to_string(); | ||
| let email_imap_username = credentials | ||
| .email_imap_username | ||
| .as_deref() | ||
| .unwrap_or("") | ||
| .trim() | ||
| .to_string(); | ||
| let email_imap_password = credentials | ||
| .email_imap_password | ||
| .as_deref() | ||
| .unwrap_or("") | ||
| .to_string(); | ||
| let email_smtp_host = credentials | ||
| .email_smtp_host | ||
| .as_deref() | ||
| .unwrap_or("") | ||
| .trim() | ||
| .to_string(); | ||
| let email_smtp_username = credentials | ||
| .email_smtp_username | ||
| .as_deref() | ||
| .unwrap_or("") | ||
| .trim() | ||
| .to_string(); | ||
| let email_smtp_password = credentials | ||
| .email_smtp_password | ||
| .as_deref() | ||
| .unwrap_or("") | ||
| .to_string(); | ||
| let email_from_address = credentials | ||
| .email_from_address | ||
| .as_deref() | ||
| .unwrap_or("") | ||
| .trim() | ||
| .to_string(); | ||
|
|
||
| if !email_imap_host.is_empty() | ||
| && !email_imap_username.is_empty() | ||
| && !email_imap_password.is_empty() | ||
| && !email_smtp_host.is_empty() | ||
| && !email_smtp_username.is_empty() | ||
| && !email_smtp_password.is_empty() | ||
| && !email_from_address.is_empty() | ||
| { | ||
| if doc.get("messaging").is_none() { | ||
| doc["messaging"] = toml_edit::Item::Table(toml_edit::Table::new()); | ||
| } | ||
| let messaging = doc["messaging"] | ||
| .as_table_mut() | ||
| .ok_or(StatusCode::INTERNAL_SERVER_ERROR)?; | ||
| if !messaging.contains_key("email") { | ||
| messaging["email"] = toml_edit::Item::Table(toml_edit::Table::new()); | ||
| } | ||
| let email = messaging["email"] | ||
| .as_table_mut() | ||
| .ok_or(StatusCode::INTERNAL_SERVER_ERROR)?; | ||
| email["enabled"] = toml_edit::value(true); | ||
| email["imap_host"] = toml_edit::value(email_imap_host); | ||
| email["imap_port"] = | ||
| toml_edit::value(i64::from(credentials.email_imap_port.unwrap_or(993))); | ||
| email["imap_username"] = toml_edit::value(email_imap_username); | ||
| email["imap_password"] = toml_edit::value(email_imap_password); | ||
| email["smtp_host"] = toml_edit::value(email_smtp_host); | ||
| email["smtp_port"] = | ||
| toml_edit::value(i64::from(credentials.email_smtp_port.unwrap_or(587))); | ||
| email["smtp_username"] = toml_edit::value(email_smtp_username); | ||
| email["smtp_password"] = toml_edit::value(email_smtp_password); | ||
| email["from_address"] = toml_edit::value(email_from_address); | ||
|
|
||
| if let Some(from_name) = &credentials.email_from_name { | ||
| let from_name = from_name.trim(); | ||
| if !from_name.is_empty() { | ||
| email["from_name"] = toml_edit::value(from_name); | ||
| } | ||
| } | ||
|
|
||
| new_email_configured = true; | ||
| } |
Contributor
There was a problem hiding this comment.
Reject partial email credential payloads explicitly.
Right now, partial email inputs are silently ignored and the request still succeeds. That makes misconfiguration hard to detect from the client side.
Suggested fix
let email_from_address = credentials
.email_from_address
.as_deref()
.unwrap_or("")
.trim()
.to_string();
+
+ let any_email_field_supplied = [
+ &credentials.email_imap_host,
+ &credentials.email_imap_username,
+ &credentials.email_imap_password,
+ &credentials.email_smtp_host,
+ &credentials.email_smtp_username,
+ &credentials.email_smtp_password,
+ &credentials.email_from_address,
+ ]
+ .iter()
+ .any(|value| value.as_deref().is_some_and(|v| !v.trim().is_empty()));
+
+ let email_payload_complete = !email_imap_host.is_empty()
+ && !email_imap_username.is_empty()
+ && !email_imap_password.is_empty()
+ && !email_smtp_host.is_empty()
+ && !email_smtp_username.is_empty()
+ && !email_smtp_password.is_empty()
+ && !email_from_address.is_empty();
+
+ if any_email_field_supplied && !email_payload_complete {
+ return Ok(Json(CreateBindingResponse {
+ success: false,
+ restart_required: false,
+ message: "Incomplete email credentials.".to_string(),
+ }));
+ }
- if !email_imap_host.is_empty()
- && !email_imap_username.is_empty()
- && !email_imap_password.is_empty()
- && !email_smtp_host.is_empty()
- && !email_smtp_username.is_empty()
- && !email_smtp_password.is_empty()
- && !email_from_address.is_empty()
- {
+ if email_payload_complete {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/bindings.rs` around lines 280 - 362, The code currently ignores
partial email credential payloads; update the block that reads email fields
(email_imap_host, email_imap_username, email_imap_password, email_smtp_host,
email_smtp_username, email_smtp_password, email_from_address) to explicitly
validate: compute a count or boolean flags for which required fields are present
(non-empty) and if some but not all required fields are provided, return a Bad
Request error (StatusCode::BAD_REQUEST) instead of silently skipping; otherwise
proceed to configure the toml as before and set new_email_configured = true.
Locate and modify the logic around the existing variables/email table handling
(e.g., the variables named email_imap_host, email_imap_username,
email_imap_password, email_smtp_host, email_smtp_username, email_smtp_password,
email_from_address and the new_email_configured assignment) to implement this
explicit partial-payload rejection.
Comment on lines
+581
to
+591
| match crate::messaging::email::EmailAdapter::from_config(email_config) { | ||
| Ok(adapter) => { | ||
| if let Err(error) = manager.register_and_start(adapter).await { | ||
| tracing::error!(%error, "failed to hot-start email adapter"); | ||
| } | ||
| } | ||
| Err(error) => { | ||
| tracing::error!(%error, "failed to build email adapter"); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid returning “active” when email adapter startup failed.
If email adapter build/start fails, the API still returns success: true with "Binding created and active.", which is inaccurate and breaks client expectations.
Suggested fix
if new_email_configured {
let Some(email_config) = new_config.messaging.email.as_ref() else {
tracing::error!("email config missing despite credentials being provided");
return Err(StatusCode::INTERNAL_SERVER_ERROR);
};
match crate::messaging::email::EmailAdapter::from_config(email_config) {
Ok(adapter) => {
if let Err(error) = manager.register_and_start(adapter).await {
tracing::error!(%error, "failed to hot-start email adapter");
+ return Ok(Json(CreateBindingResponse {
+ success: false,
+ restart_required: true,
+ message: format!(
+ "Binding created, but email adapter failed to start: {error}"
+ ),
+ }));
}
}
Err(error) => {
tracing::error!(%error, "failed to build email adapter");
+ return Ok(Json(CreateBindingResponse {
+ success: false,
+ restart_required: true,
+ message: format!(
+ "Binding created, but email configuration is invalid: {error}"
+ ),
+ }));
}
}
}Also applies to: 625-629
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/bindings.rs` around lines 581 - 591, The code currently swallows
adapter build/start failures (in the EmailAdapter::from_config and
manager.register_and_start paths) and only logs errors, causing the API to
return "Binding created and active."; change the control flow so that on
Err(error) from EmailAdapter::from_config or when
manager.register_and_start(adapter).await returns Err(error) you do not mark the
binding as active — instead propagate the error into the API response (e.g., set
success=false and an appropriate message/status field indicating startup failed
or return an Err result) so clients receive a non-active/failure response;
update the matching logic around EmailAdapter::from_config and the
manager.register_and_start call (and the analogous block at lines 625-629) to
construct and return the failure response rather than only logging with
tracing::error!.
Comment on lines
+427
to
+438
| "email" => { | ||
| if let Some(email_config) = &new_config.messaging.email { | ||
| match crate::messaging::email::EmailAdapter::from_config(email_config) { | ||
| Ok(adapter) => { | ||
| if let Err(error) = manager.register_and_start(adapter).await { | ||
| tracing::error!(%error, "failed to start email adapter on toggle"); | ||
| } | ||
| } | ||
| Err(error) => { | ||
| tracing::error!(%error, "failed to build email adapter on toggle"); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t report enable success when email adapter failed to build/start.
If EmailAdapter::from_config or register_and_start fails, the handler still reaches the success response, leaving UI/config state inconsistent with runtime state.
Suggested fix
"email" => {
if let Some(email_config) = &new_config.messaging.email {
match crate::messaging::email::EmailAdapter::from_config(email_config) {
Ok(adapter) => {
if let Err(error) = manager.register_and_start(adapter).await {
tracing::error!(%error, "failed to start email adapter on toggle");
+ return Ok(Json(serde_json::json!({
+ "success": false,
+ "message": format!("failed to enable {platform}: {error}")
+ })));
}
}
Err(error) => {
tracing::error!(%error, "failed to build email adapter on toggle");
+ return Ok(Json(serde_json::json!({
+ "success": false,
+ "message": format!("invalid {platform} configuration: {error}")
+ })));
}
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/messaging.rs` around lines 427 - 438, The current toggle handling for
"email" logs errors from EmailAdapter::from_config and
manager.register_and_start but still proceeds to the success response; update
the "email" arm so that on Err from EmailAdapter::from_config or on Err from
manager.register_and_start(adapter).await you short-circuit and return an
appropriate error response (or propagate the error) instead of falling through
to the success path—modify the match arms around EmailAdapter::from_config and
the inner register_and_start call to return early (e.g., Err(...) or an HTTP
500/error response) when failures occur so the UI/config state reflects the
actual runtime result.
| session | ||
| .select(&folder) | ||
| .with_context(|| format!("failed to select IMAP folder '{folder}'"))?; | ||
| session.logout().ok(); |
Contributor
There was a problem hiding this comment.
Log the logout error instead of silently discarding it.
The coding guidelines restrict .ok() to channel sends only. While IMAP logout failures after a successful health check are unlikely to be actionable, the error should be logged for observability.
🔧 Suggested fix
- session.logout().ok();
+ if let Err(error) = session.logout() {
+ tracing::debug!(%error, "IMAP logout failed during health check");
+ }As per coding guidelines: "Don't silently discard errors; no let _ = on Results. Handle, log, or propagate errors. Only exception is .ok() on channel sends where the receiver may be dropped"
📝 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
| session.logout().ok(); | |
| if let Err(error) = session.logout() { | |
| tracing::debug!(%error, "IMAP logout failed during health check"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/messaging/email.rs` at line 526, The call session.logout().ok() silently
discards any logout error; change it to capture the Result returned by
session.logout() and log failures instead of ignoring them (e.g., if let Err(e)
= session.logout() { <logger>.error!("IMAP logout failed: {}", e); }), keeping
successful path unchanged; update the same location where session.logout().ok()
appears so you reference the session.logout() call and use the existing logging
facility in this module (tracing/log or the module's logger) to record the
error.
| } | ||
| } | ||
|
|
||
| session.logout().ok(); |
Contributor
There was a problem hiding this comment.
Log the logout error instead of silently discarding it.
Same issue as in health_check(): the coding guidelines restrict .ok() to channel sends only.
🔧 Suggested fix
- session.logout().ok();
+ if let Err(error) = session.logout() {
+ tracing::debug!(%error, "IMAP logout failed after polling");
+ }As per coding guidelines: "Don't silently discard errors; no let _ = on Results. Handle, log, or propagate errors. Only exception is .ok() on channel sends where the receiver may be dropped"
📝 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
| session.logout().ok(); | |
| if let Err(error) = session.logout() { | |
| tracing::debug!(%error, "IMAP logout failed after polling"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/messaging/email.rs` at line 639, Replace the silent discard of
session.logout() with explicit error logging: instead of session.logout().ok();
call session.logout() and if it returns Err(e) log the error (e.g., using the
project's logger macro like error! or tracing::error!) with context such as
"email session logout failed" and the error variable; locate the
session.logout() call in the same function where health_check() had the similar
issue and use the same logging style used elsewhere in src/messaging/email.rs to
keep consistency.
| } | ||
| } | ||
|
|
||
| session.logout().ok(); |
Contributor
There was a problem hiding this comment.
Log the logout error instead of silently discarding it.
Same issue as the other session.logout().ok() calls.
🔧 Suggested fix
- session.logout().ok();
+ if let Err(error) = session.logout() {
+ tracing::debug!(%error, "IMAP logout failed after history fetch");
+ }As per coding guidelines: "Don't silently discard errors; no let _ = on Results. Handle, log, or propagate errors. Only exception is .ok() on channel sends where the receiver may be dropped"
📝 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
| session.logout().ok(); | |
| if let Err(error) = session.logout() { | |
| tracing::debug!(%error, "IMAP logout failed after history fetch"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/messaging/email.rs` at line 982, Replace the silent discard of the logout
Result at the session.logout() call with explicit error logging: instead of
session.logout().ok(), check the Result (e.g., if let Err(e) = session.logout()
or match session.logout()) and call the project's logger (e.g., error! or
tracing::error!) to record a descriptive message including the error value;
update all similar session.logout() occurrences in src/messaging/email.rs so
logout failures are logged rather than ignored.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/agent/channel.rs (1)
1043-1055:⚠️ Potential issue | 🟠 MajorEmail loses all outbound reply paths when
allow_direct_replyis false.Line 1043 sets
allow_direct_reply = !self.suppress_plaintext_fallback(), which becomesfalsefor email (line 302). This gatesReplyToolregistration inadd_channel_tools(line 245), and plaintext fallback is also suppressed at lines 1188, 1247, and 1299 with no actual reply sent. Email turns where the LLM produces plain text have no reply mechanism at all—neither the explicitReplyToolnor fallback text output.Recommended fix
- let allow_direct_reply = !self.suppress_plaintext_fallback(); + // Keep explicit reply tool available; adapter policy only suppresses raw-text fallback. + let allow_direct_reply = true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 1043 - 1055, The email flows currently disable all outbound reply paths because allow_direct_reply is set from !self.suppress_plaintext_fallback(), which prevents ReplyTool registration in add_channel_tools and also suppresses plaintext output; to fix, decouple "direct reply" capability from plaintext-fallback suppression by ensuring add_channel_tools always registers the ReplyTool (or set allow_direct_reply = true for email channels) while keeping suppress_plaintext_fallback() behavior for plaintext output; locate the decision in channel.rs around allow_direct_reply, the call to add_channel_tools, and the ReplyTool registration within add_channel_tools and change the condition so email still gets a ReplyTool (or pass an explicit flag like is_email_channel) even when plaintext fallback is suppressed.
🧹 Nitpick comments (2)
src/tools.rs (1)
246-251: Consider deduplicating agent display-name resolution.This logic mirrors
src/agent/channel.rsand could drift over time. Consider a shared helper to keep one canonical resolution path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools.rs` around lines 246 - 251, The agent display-name resolution in tools.rs (the agent_display_name assignment using state.deps.agent_names.get(...).cloned().unwrap_or_else(...)) is duplicated with the logic in src/agent/channel.rs; extract this into a single helper (e.g., resolve_agent_display_name or a method on the deps struct like AgentDeps::display_name_for) that accepts the agent_names map and an agent_id and returns the canonical display string, then replace the inline logic in both tools.rs and src/agent/channel.rs to call that helper so they share one implementation.tests/context_dump.rs (1)
215-225: Add a context-dump test path withallow_direct_reply = false.These updates cover the new argument wiring, but not the behavior change itself. Add one assertion path that verifies
replyis absent from tool defs when direct replies are disabled.Also applies to: 434-444
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/context_dump.rs` around lines 215 - 225, The new test should add a case calling spacebot::tools::add_channel_tools with allow_direct_reply (the boolean argument currently passed as true) set to false and then assert that the generated tool definitions do not include any "reply" entry; specifically, in tests/context_dump.rs add a branch that invokes add_channel_tools with the allow_direct_reply flag false and then checks the returned/inspected tool defs (from the same helper used in the existing tests) to ensure there is no reply field or reply-capable tool present. Use the same helper variables (tool_server, state, response_tx, "test-conversation", skip_flag, replied_flag, None, None) but pass false for the last boolean and add an assertion mirroring the existing positive case that verifies absence of reply in the tool list.
🤖 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/`(messaging)/email-setup.mdx:
- Around line 126-129: Update the verification steps so they don't assume
replies are enabled: change the third step ("Reply in the same thread and
confirm Spacebot replies in-thread") to instead instruct the tester to either
(A) if reply-enabled is explicitly configured, reply in-thread and confirm
Spacebot responds in-thread, or (B) by default (inbound reply-disabled) reply
in-thread and confirm that the message is only ingested/triaged (no automated
reply) and that a new channel or intake record is created; reference the
numbered verification list where the steps begin and the note about "inbound
email channels are reply-disabled by default" so the behavior is validated
appropriately.
In `@docs/content/docs/`(messaging)/messaging.mdx:
- Line 147: Update the sentence that currently reads "Changes to bindings and
permissions (channel filters, DM allowed users) take effect within a couple
seconds — no restart needed." to use formal phrasing by inserting "of": change
"a couple seconds" to "a couple of seconds" so the sentence reads "take effect
within a couple of seconds"; make the same adjustment in the sentence that
follows if duplicated.
In `@src/prompts/engine.rs`:
- Around line 448-451: The code currently swallows errors by calling .ok() on
self.render_static(template_name); replace that silence with explicit error
handling: match on the Result from render_static(template_name), and on Err(e)
log the error with context (include template_name and the error) and return None
(or propagate the error by changing the surrounding function signature to return
Result<Option<String>, E>); on Ok(value) continue to trim, to_string and filter
empty as before. Use the existing logger (e.g., tracing::error or whatever
project logger is used) and reference the render_static(template_name) call and
template_name in the log message to aid diagnosis.
---
Outside diff comments:
In `@src/agent/channel.rs`:
- Around line 1043-1055: The email flows currently disable all outbound reply
paths because allow_direct_reply is set from
!self.suppress_plaintext_fallback(), which prevents ReplyTool registration in
add_channel_tools and also suppresses plaintext output; to fix, decouple "direct
reply" capability from plaintext-fallback suppression by ensuring
add_channel_tools always registers the ReplyTool (or set allow_direct_reply =
true for email channels) while keeping suppress_plaintext_fallback() behavior
for plaintext output; locate the decision in channel.rs around
allow_direct_reply, the call to add_channel_tools, and the ReplyTool
registration within add_channel_tools and change the condition so email still
gets a ReplyTool (or pass an explicit flag like is_email_channel) even when
plaintext fallback is suppressed.
---
Nitpick comments:
In `@src/tools.rs`:
- Around line 246-251: The agent display-name resolution in tools.rs (the
agent_display_name assignment using
state.deps.agent_names.get(...).cloned().unwrap_or_else(...)) is duplicated with
the logic in src/agent/channel.rs; extract this into a single helper (e.g.,
resolve_agent_display_name or a method on the deps struct like
AgentDeps::display_name_for) that accepts the agent_names map and an agent_id
and returns the canonical display string, then replace the inline logic in both
tools.rs and src/agent/channel.rs to call that helper so they share one
implementation.
In `@tests/context_dump.rs`:
- Around line 215-225: The new test should add a case calling
spacebot::tools::add_channel_tools with allow_direct_reply (the boolean argument
currently passed as true) set to false and then assert that the generated tool
definitions do not include any "reply" entry; specifically, in
tests/context_dump.rs add a branch that invokes add_channel_tools with the
allow_direct_reply flag false and then checks the returned/inspected tool defs
(from the same helper used in the existing tests) to ensure there is no reply
field or reply-capable tool present. Use the same helper variables (tool_server,
state, response_tx, "test-conversation", skip_flag, replied_flag, None, None)
but pass false for the last boolean and add an assertion mirroring the existing
positive case that verifies absence of reply in the tool list.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
docs/content/docs/(messaging)/email-setup.mdxdocs/content/docs/(messaging)/messaging.mdxprompts/en/adapters/email.md.j2prompts/en/channel.md.j2src/agent/channel.rssrc/prompts/engine.rssrc/prompts/text.rssrc/tools.rssrc/tools/reply.rstests/context_dump.rs
Comment on lines
+126
to
+129
| 1. Send an email to the configured mailbox from an allowed sender. | ||
| 2. Confirm a new channel appears in Spacebot with a subject-based name. | ||
| 3. Reply in the same thread and confirm Spacebot replies in-thread. | ||
|
|
Contributor
There was a problem hiding this comment.
Verification step conflicts with inbound-only default behavior.
Line 128 says to confirm Spacebot replies in-thread, but Line 88 says inbound email channels are reply-disabled by default. This should be rewritten to validate intake/triage behavior unless direct reply is explicitly enabled.
📝 Proposed doc fix
-3. Reply in the same thread and confirm Spacebot replies in-thread.
+3. Reply in the same thread and confirm the message is ingested into the same conversation thread.
+4. If direct email replies are explicitly enabled for your setup, verify outbound replies preserve threading headers.📝 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
| 1. Send an email to the configured mailbox from an allowed sender. | |
| 2. Confirm a new channel appears in Spacebot with a subject-based name. | |
| 3. Reply in the same thread and confirm Spacebot replies in-thread. | |
| 1. Send an email to the configured mailbox from an allowed sender. | |
| 2. Confirm a new channel appears in Spacebot with a subject-based name. | |
| 3. Reply in the same thread and confirm the message is ingested into the same conversation thread. | |
| 4. If direct email replies are explicitly enabled for your setup, verify outbound replies preserve threading headers. | |
🧰 Tools
🪛 LanguageTool
[style] ~126-~126: Consider using “email”.
Context: ...AP polling. ## Verify it's working 1. Send an email to the configured mailbox from an allowed ...
(SEND_AN_EMAIL)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/content/docs/`(messaging)/email-setup.mdx around lines 126 - 129, Update
the verification steps so they don't assume replies are enabled: change the
third step ("Reply in the same thread and confirm Spacebot replies in-thread")
to instead instruct the tester to either (A) if reply-enabled is explicitly
configured, reply in-thread and confirm Spacebot responds in-thread, or (B) by
default (inbound reply-disabled) reply in-thread and confirm that the message is
only ingested/triaged (no automated reply) and that a new channel or intake
record is created; reference the numbered verification list where the steps
begin and the note about "inbound email channels are reply-disabled by default"
so the behavior is validated appropriately.
| ## Hot Reloading | ||
|
|
||
| Changes to bindings and permissions (channel filters, DM allowed users) take effect within a couple seconds — no restart needed. Token changes require a restart, or you can re-save from the dashboard which reconnects automatically. | ||
| Changes to bindings and permissions (channel filters, DM allowed users) take effect within a couple seconds — no restart needed. Token and credential changes are applied by reconnecting the adapter. |
Contributor
There was a problem hiding this comment.
Use formal phrasing in docs copy.
Line 147 should use “a couple of seconds” instead of “a couple seconds.”
🧰 Tools
🪛 LanguageTool
[style] ~147-~147: It’s considered informal to use ‘a couple’ without the preposition ‘of’ before a noun.
Context: ...s, DM allowed users) take effect within a couple seconds — no restart needed. Token and ...
(A_COUPLE_OF)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/content/docs/`(messaging)/messaging.mdx at line 147, Update the sentence
that currently reads "Changes to bindings and permissions (channel filters, DM
allowed users) take effect within a couple seconds — no restart needed." to use
formal phrasing by inserting "of": change "a couple seconds" to "a couple of
seconds" so the sentence reads "take effect within a couple of seconds"; make
the same adjustment in the sentence that follows if duplicated.
Comment on lines
+448
to
+451
| self.render_static(template_name) | ||
| .ok() | ||
| .map(|value| value.trim().to_string()) | ||
| .filter(|value| !value.is_empty()) |
Contributor
There was a problem hiding this comment.
Don’t silently swallow adapter prompt render failures.
Line 448 drops template-render errors with .ok(), which can hide prompt misconfiguration and make behavior hard to diagnose. Prefer logging and returning None, or returning Result<Option<String>>.
As per coding guidelines, "Don't silently discard errors; no let _ = on Results. Handle, log, or propagate errors. 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/prompts/engine.rs` around lines 448 - 451, The code currently swallows
errors by calling .ok() on self.render_static(template_name); replace that
silence with explicit error handling: match on the Result from
render_static(template_name), and on Err(e) log the error with context (include
template_name and the error) and return None (or propagate the error by changing
the surrounding function signature to return Result<Option<String>, E>); on
Ok(value) continue to trim, to_string and filter empty as before. Use the
existing logger (e.g., tracing::error or whatever project logger is used) and
reference the render_static(template_name) call and template_name in the log
message to aid diagnosis.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
docs/content/docs/(messaging)/messaging.mdx (1)
147-147:⚠️ Potential issue | 🟡 MinorUse formal phrasing: "a couple of seconds".
The phrase "a couple seconds" is considered informal. Add "of" for consistent documentation tone.
📝 Proposed fix
-Changes to bindings and permissions (channel filters, DM allowed users) take effect within a couple seconds — no restart needed. +Changes to bindings and permissions (channel filters, DM allowed users) take effect within a couple of seconds — no restart needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/docs/`(messaging)/messaging.mdx at line 147, Update the sentence "Changes to bindings and permissions (channel filters, DM allowed users) take effect within a couple seconds — no restart needed." to use formal phrasing by inserting "of" so it reads "take effect within a couple of seconds — no restart needed." Modify the exact line containing that sentence in the messaging documentation (the string shown in the diff) to ensure consistent tone across docs.docs/content/docs/(messaging)/email-setup.mdx (1)
133-137:⚠️ Potential issue | 🟡 MinorVerification step conflicts with inbound-only default behavior.
Line 137 instructs users to "confirm Spacebot replies in-thread," but line 88 states that inbound email channels are reply-disabled by default. The verification step should validate intake/triage behavior unless direct reply is explicitly enabled.
📝 Proposed doc fix
-3. Reply in the same thread and confirm Spacebot replies in-thread. +3. Reply in the same thread and confirm the message is ingested into the existing conversation. +4. If outbound email replies are explicitly enabled, verify responses preserve threading headers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/docs/`(messaging)/email-setup.mdx around lines 133 - 137, The verification step under the "## Verify it's working" heading incorrectly asks users to "confirm Spacebot replies in-thread" despite the doc stating inbound email channels are "reply-disabled by default"; update the verification step to instead validate intake/triage behavior (e.g., confirm a new channel is created with the subject-based name and inbound messages are recorded) or add a conditional note telling readers to enable replies first if they want to test in-thread replies—adjust the "Verify it's working" section text to either remove the reply confirmation or add instructions to enable replies before testing so the doc is consistent with the inbound email default behavior.
🧹 Nitpick comments (1)
src/tools/send_message_to_another_channel.rs (1)
179-194: Consider edge case: channel names containing@.The bare email detection on line 189 uses
contains('@')as the heuristic. If a channel name ever contains@(e.g., a Slack channel namedteam@project), it would be incorrectly routed to email parsing. This is likely rare, but could cause unexpected behavior.Consider adding basic email validation (e.g., checking for a valid domain pattern) before treating it as an email target.
♻️ Optional: Add minimal email validation
if !trimmed.contains('@') { return None; } + + // Basic validation: must have format local@domain.tld + let at_pos = trimmed.find('@')?; + let domain = &trimmed[at_pos + 1..]; + if !domain.contains('.') || domain.starts_with('.') || domain.ends_with('.') { + return None; + } crate::messaging::target::parse_delivery_target(&format!("email:{trimmed}"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/send_message_to_another_channel.rs` around lines 179 - 194, The heuristic in parse_explicit_email_target currently treats any string containing '@' as an email and hands it to parse_delivery_target, which misclassifies channel names like "team@project"; update parse_explicit_email_target to perform a minimal email validation before that fallback (for example verify the trimmed string has exactly one '@', non-empty local part, a domain part containing at least one '.' and no whitespace) and only call crate::messaging::target::parse_delivery_target(&format!("email:{trimmed}")) when the validation passes; reference parse_explicit_email_target and parse_delivery_target when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/content/docs/`(messaging)/email-setup.mdx:
- Around line 133-137: The verification step under the "## Verify it's working"
heading incorrectly asks users to "confirm Spacebot replies in-thread" despite
the doc stating inbound email channels are "reply-disabled by default"; update
the verification step to instead validate intake/triage behavior (e.g., confirm
a new channel is created with the subject-based name and inbound messages are
recorded) or add a conditional note telling readers to enable replies first if
they want to test in-thread replies—adjust the "Verify it's working" section
text to either remove the reply confirmation or add instructions to enable
replies before testing so the doc is consistent with the inbound email default
behavior.
In `@docs/content/docs/`(messaging)/messaging.mdx:
- Line 147: Update the sentence "Changes to bindings and permissions (channel
filters, DM allowed users) take effect within a couple seconds — no restart
needed." to use formal phrasing by inserting "of" so it reads "take effect
within a couple of seconds — no restart needed." Modify the exact line
containing that sentence in the messaging documentation (the string shown in the
diff) to ensure consistent tone across docs.
---
Nitpick comments:
In `@src/tools/send_message_to_another_channel.rs`:
- Around line 179-194: The heuristic in parse_explicit_email_target currently
treats any string containing '@' as an email and hands it to
parse_delivery_target, which misclassifies channel names like "team@project";
update parse_explicit_email_target to perform a minimal email validation before
that fallback (for example verify the trimmed string has exactly one '@',
non-empty local part, a domain part containing at least one '.' and no
whitespace) and only call
crate::messaging::target::parse_delivery_target(&format!("email:{trimmed}"))
when the validation passes; reference parse_explicit_email_target and
parse_delivery_target when making the change.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/content/docs/(messaging)/email-setup.mdxdocs/content/docs/(messaging)/messaging.mdxprompts/en/tools/send_message_description.md.j2src/tools/send_message_to_another_channel.rs
✅ Files skipped from review due to trivial changes (1)
- prompts/en/tools/send_message_description.md.j2
This was referenced Feb 27, 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
docs/content/docs/(messaging)/email-setup.mdxand updates to messaging/configuration docs plus roadmap statusTesting
cargo checkcargo test messaging::email --libnpm run build(ininterface/)npm run types:check(indocs/) currently fails in this environment because Next.js requires Node>=20.9.0but local Node is18.18.0Note
Added email messaging adapter implementation (1344 lines) with full lifecycle management: configurable IMAP polling, SMTP delivery, sender filtering, attachment handling, and graceful shutdown. Integrated across the system with EmailConfig parameters, API endpoints for credential binding, and UI components in Settings for platform configuration and messaging status. Documentation includes comprehensive setup guide for IMAP/SMTP configuration. Dependencies updated for email library support.
Written by Tembo for commit a114f22. This will update automatically on new commits.