feat(messaging): add signal configuration to web app#392
Open
ibhagwan wants to merge 7 commits intospacedriveapp:mainfrom
Open
feat(messaging): add signal configuration to web app#392ibhagwan wants to merge 7 commits intospacedriveapp:mainfrom
ibhagwan wants to merge 7 commits intospacedriveapp:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds first-class Signal support and Mattermost status exposure across frontend types/UI, backend parsing/validation, adapter lifecycle/watcher, cron/tools routing, prompt rendering, runtime state, and tests; extends client API types to accept Signal credentials and expose Mattermost/Signal status. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ibhagwan
force-pushed
the
feat/signal-web-ui-config
branch
from
f9a4efb to
0376290
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
1 similar comment
Contributor
✅ Actions performedReview triggered.
|
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tools/send_message_to_another_channel.rs (1)
187-191:⚠️ Potential issue | 🟡 MinorTighten the Signal adapter check before enabling shorthand parsing.
starts_with("signal")is too broad here.current_adapter()can fall back to the channel ID (src/agent/channel.rs:527-535), so a non-Signal channel likesignal-alertswould incorrectly enter the Signal shorthand path for+123...,group:..., or bare UUID targets. Restrict this to the real adapter shapes ("signal"or"signal:...").Suggested fix
if let Some(current_adapter) = self .current_adapter .as_ref() - .filter(|adapter| adapter.starts_with("signal")) + .filter(|adapter| *adapter == "signal" || adapter.starts_with("signal:")) && let Some(target) = parse_implicit_signal_shorthand(&args.target, current_adapter) {🤖 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 187 - 191, The current check uses adapter.starts_with("signal") which is too permissive; update the predicate in the self.current_adapter.as_ref().filter(...) used alongside parse_implicit_signal_shorthand(&args.target, current_adapter) to only allow exact "signal" or the namespaced form "signal:..." (e.g., adapter == "signal" || adapter.starts_with("signal:")), so non-Signal adapters like "signal-alerts" won't trigger Signal shorthand parsing in current_adapter / parse_implicit_signal_shorthand.
🤖 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 42-47: Remove the "signal" entry from the DOC_LINKS map so the
settings UI doesn't surface a dead docs link; update the
Partial<Record<Platform, string>> DOC_LINKS constant in ChannelSettingCard
(remove the signal: "https://docs.spacebot.sh/signal-setup" key) and ensure any
code that reads DOC_LINKS (e.g., link rendering for platforms) will simply not
find a URL for Platform "signal" until the docs page is live.
In `@src/api/messaging.rs`:
- Around line 1288-1304: The current ok_or_else(...)? branches for
credentials.signal_http_url and credentials.signal_account return a bare
StatusCode::BAD_REQUEST which bypasses the MessagingInstanceActionResponse path;
change these to return an in-band validation response instead (e.g., construct
and return MessagingInstanceActionResponse::ValidationFailure with a descriptive
message like "signal: http_url is required" or "signal: account is required")
from the enclosing handler function rather than using the StatusCode; apply the
same pattern for the other occurrence noted around the invalid-E.164 branch
(lines ~1453-1469) so all user-correctable validation failures produce
MessagingInstanceActionResponse::ValidationFailure with clear messages instead
of StatusCode::BAD_REQUEST.
- Around line 1288-1304: The code that reads signal_account (via signal_account
and signal_http_url) can silently expand dm_allowed_users when the operator
changes account; fix by detecting when the provided signal_account differs from
the previously auto-added account and remove the old auto-added number from
dm_allowed_users (or rebuild permissions) so the allowed-DM set does not grow
forever; update the create/update branch that uses signal_account (and the
related block around lines handling signal_http_url/signal_account) to compare
the incoming account to the existing configured account and if different, call
the logic that removes the prior auto-added entry (or invoke
SignalPermissions::build_from_seed to regenerate permissions) so only the new
account remains authorized.
- Around line 1143-1146: The change allowed creating "signal" but other handlers
still reject it; update the API to treat Signal consistently by adding "signal"
to the platform handling paths: ensure messaging_status() includes Signal
instances in MessagingStatusResponse.instances, implement a start path for
"signal" inside toggle_platform() (mirroring how "telegram"/"discord" are
started) and allow deletion in delete_messaging_instance() so it no longer
rejects "signal"; ensure any platform validation or match arms that previously
excluded "signal" are updated so creation, reporting, toggling, and deletion all
accept the "signal" platform.
- Around line 136-143: The is_valid_e164 function currently allows numbers like
+01234567 and >15 digits; update is_valid_e164 to require the first digit after
'+' to be between '1' and '9' (non-zero) and enforce a maximum length of 15
digits in addition to the existing minimum of 7, while still ensuring all
characters are ASCII digits; update any unit tests referencing is_valid_e164 to
add cases for '+01234567' (should be invalid) and a >15-digit number (should be
invalid) and keep valid examples within 7–15 digits starting with 1–9.
---
Outside diff comments:
In `@src/tools/send_message_to_another_channel.rs`:
- Around line 187-191: The current check uses adapter.starts_with("signal")
which is too permissive; update the predicate in the
self.current_adapter.as_ref().filter(...) used alongside
parse_implicit_signal_shorthand(&args.target, current_adapter) to only allow
exact "signal" or the namespaced form "signal:..." (e.g., adapter == "signal" ||
adapter.starts_with("signal:")), so non-Signal adapters like "signal-alerts"
won't trigger Signal shorthand parsing in current_adapter /
parse_implicit_signal_shorthand.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ac7970c0-8a54-476f-9f64-842e23d52d5b
📒 Files selected for processing (7)
interface/src/api/client.tsinterface/src/components/ChannelEditModal.tsxinterface/src/components/ChannelSettingCard.tsxinterface/src/lib/platformIcons.tsxinterface/src/routes/Settings.tsxsrc/api/messaging.rssrc/tools/send_message_to_another_channel.rs
ibhagwan
changed the title
ibhagwan
force-pushed
the
feat/signal-web-ui-config
branch
6 times, most recently
from
e771158 to
eb9d9f3
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
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 (2)
src/messaging/target.rs (2)
337-340:⚠️ Potential issue | 🟠 MajorValidate bare Signal phone numbers with the same E.164 rules.
This branch still accepts values like
01234567and1234567890123456, because it only checks for ASCII digits andlen >= 7before prepending+. That lets malformed Signal targets passparse_delivery_target()even though the rest of the PR tightened E.164 handling.Suggested fix
- // Check if it's a bare phone number (7+ digits required for E.164) - if target.chars().all(|c| c.is_ascii_digit()) && target.len() >= 7 { - return Some(format!("+{target}")); + // Check if it's a bare phone number and normalize only if it becomes valid E.164 + if target.chars().all(|c| c.is_ascii_digit()) { + let normalized = format!("+{target}"); + if is_valid_e164(&normalized) { + return Some(normalized); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/target.rs` around lines 337 - 340, The bare-digit branch in parse_delivery_target accepts malformed numbers; change the check in the block that currently tests target.chars().all(|c| c.is_ascii_digit()) && target.len() >= 7 to enforce E.164 rules: ensure all characters are ASCII digits, length is between 7 and 15 inclusive, and the first digit is not '0' (reject leading zero country codes) before returning Some(format!("+{target}")). Update the logic around the target variable in parse_delivery_target to use these stricter validations.
403-456:⚠️ Potential issue | 🟠 MajorReject empty Signal instance and target segments here.
These match arms currently build
BroadcastTargets for malformed inputs likesignal:uuid:,signal:group:, andsignal::+15551234567. That moves bad delivery targets past validation and into later send-time failures instead of failing at parse time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/target.rs` around lines 403 - 456, The parsing arms for Signal targets (the match that constructs BroadcastTarget instances using normalize_signal_target and direct formats) currently accept empty path segments like uuid/group/instance parts; update each arm to reject empty segments by returning None when an identifier is empty. Concretely: for the arms ["uuid", uuid], ["group", group_id], [instance, "uuid", uuid], and [instance, "group", group_id] add an early guard returning None if uuid/is_empty()/group_id.is_empty(); for the ["e164", phone], [instance, "e164", phone], [phone] if phone.starts_with('+'), and [instance, phone] if phone.starts_with('+') arms return None when phone.is_empty() (or otherwise invalid) before calling normalize_signal_target; and for the single-part arms [single] and [instance, single] return None when single.is_empty() instead of delegating to normalize_signal_target. Reference symbols: BroadcastTarget, normalize_signal_target and the match arms that use patterns ["uuid", uuid], ["group", group_id], ["e164", phone], [phone] if phone.starts_with('+'), [single], [instance, "uuid", uuid], [instance, "group", group_id], [instance, "e164", phone], [instance, phone] if phone.starts_with('+'), and [instance, single].
♻️ Duplicate comments (1)
src/api/messaging.rs (1)
1204-1207:⚠️ Potential issue | 🟠 MajorSignal creation is enabled here, but re-enable still looks like a no-op.
This allowlist now lets the web flow create Signal instances, but
toggle_platform()still has no"signal"start branch. After a Signal adapter is disabled, turning it back on only flips config; no runtime adapter is registered on the same path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/messaging.rs` around lines 1204 - 1207, The allowlist check in messaging.rs now permits "signal" (platform.as_str()), but toggle_platform() lacks a "signal" start branch so re-enabling Signal only flips config and doesn't register the runtime adapter; update toggle_platform() to handle the "signal" case by instantiating and registering the Signal adapter when starting (mirror the existing branches for "discord"/"slack" etc.), using the same adapter registration path and error handling as other platforms (refer to toggle_platform() and platform.as_str() usage) so disabling and re-enabling Signal properly tears down and recreates the runtime adapter.
🧹 Nitpick comments (1)
src/tools/cron.rs (1)
431-441: Add a different-instance regression test.The suite covers same-instance qualified targets, but not an explicit
signal:other_instance:...target created from a different current instance. That is the case most likely to get silently rewritten if parser behavior changes.Suggested test
+ #[test] + fn test_normalize_explicit_other_instance_no_rewrite() { + let result = normalize_delivery_target( + "signal:personal:uuid:550e8400-e29b-41d4-a716-446655440000", + &Some("signal:work".to_string()), + ); + assert_eq!( + result, + "signal:personal:uuid:550e8400-e29b-41d4-a716-446655440000" + ); + } + #[test] fn test_normalize_already_qualified_no_rewrite() {Also applies to: 530-541
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/cron.rs` around lines 431 - 441, Add a regression test to cover the case where normalize_delivery_target is given a delivery_target that already names a different instance (e.g., "signal:other_instance:...") while current_adapter is a different named instance; ensure the test asserts that normalize_delivery_target does NOT rewrite the other-instance target and returns the original string. Locate normalize_delivery_target in src/tools/cron.rs and add a unit test (or expand the existing test module around the tests covering same-instance qualified targets near the other similar case at lines ~530-541) that passes a parsed-style target produced by crate::messaging::target::parse_delivery_target and a different current_adapter value, then assert equality with the original delivery_target.
🤖 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 651-658: Update the frontend E.164 validation to match the backend
by rejecting numbers that start with 0 and enforcing the 8–15 digit length
(excluding the '+'). Replace the current e164Regex used around
credentialInputs.signal_account with a pattern that requires a '+' then a
non-zero first digit and a total of 8–15 digits (e.g., use a regex equivalent to
/^\+[1-9]\d{7,14}$/), and keep the same setMessage error text; apply the same
change to the other occurrence noted (around lines 975-976) so both client-side
checks mirror the backend rule.
In `@src/tools/send_message_to_another_channel.rs`:
- Around line 198-203: The current check allows default Signal adapters to call
parse_implicit_signal_shorthand for bare +... targets, which uses a laxer
parser; update the call site and the helper so implicit shorthand uses the same
validation/normalization as explicit targets by replacing
parse_implicit_signal_shorthand usage with logic that invokes is_valid_e164()
and normalize_signal_target() (or change parse_implicit_signal_shorthand to
internally call is_valid_e164() then normalize_signal_target()) so inputs like
"+01234567" and overly long numbers are rejected consistently; ensure references
to current_adapter and args.target remain but flow through the stricter
validation/normalization functions (preserve function names
parse_implicit_signal_shorthand, is_valid_e164, normalize_signal_target as
anchors).
- Around line 146-170: The code leaves target.adapter == "signal" when called
outside a Signal conversation and multiple named Signal adapters exist, causing
broadcast() to fail with "no messaging adapter named 'signal'"; update the logic
in send_message_to_another_channel (the block handling target.adapter ==
"signal", referencing current_adapter and messaging_manager.adapter_names()) to
detect the ambiguous case (signal_adapters.len() != 1 and no adapter named
exactly "signal") and return an early Err (or a recoverable error result) with a
clear message about multiple named Signal adapters and asking the caller to
specify one, instead of leaving target.adapter as "signal" and deferring to
broadcast().
---
Outside diff comments:
In `@src/messaging/target.rs`:
- Around line 337-340: The bare-digit branch in parse_delivery_target accepts
malformed numbers; change the check in the block that currently tests
target.chars().all(|c| c.is_ascii_digit()) && target.len() >= 7 to enforce E.164
rules: ensure all characters are ASCII digits, length is between 7 and 15
inclusive, and the first digit is not '0' (reject leading zero country codes)
before returning Some(format!("+{target}")). Update the logic around the target
variable in parse_delivery_target to use these stricter validations.
- Around line 403-456: The parsing arms for Signal targets (the match that
constructs BroadcastTarget instances using normalize_signal_target and direct
formats) currently accept empty path segments like uuid/group/instance parts;
update each arm to reject empty segments by returning None when an identifier is
empty. Concretely: for the arms ["uuid", uuid], ["group", group_id], [instance,
"uuid", uuid], and [instance, "group", group_id] add an early guard returning
None if uuid/is_empty()/group_id.is_empty(); for the ["e164", phone], [instance,
"e164", phone], [phone] if phone.starts_with('+'), and [instance, phone] if
phone.starts_with('+') arms return None when phone.is_empty() (or otherwise
invalid) before calling normalize_signal_target; and for the single-part arms
[single] and [instance, single] return None when single.is_empty() instead of
delegating to normalize_signal_target. Reference symbols: BroadcastTarget,
normalize_signal_target and the match arms that use patterns ["uuid", uuid],
["group", group_id], ["e164", phone], [phone] if phone.starts_with('+'),
[single], [instance, "uuid", uuid], [instance, "group", group_id], [instance,
"e164", phone], [instance, phone] if phone.starts_with('+'), and [instance,
single].
---
Duplicate comments:
In `@src/api/messaging.rs`:
- Around line 1204-1207: The allowlist check in messaging.rs now permits
"signal" (platform.as_str()), but toggle_platform() lacks a "signal" start
branch so re-enabling Signal only flips config and doesn't register the runtime
adapter; update toggle_platform() to handle the "signal" case by instantiating
and registering the Signal adapter when starting (mirror the existing branches
for "discord"/"slack" etc.), using the same adapter registration path and error
handling as other platforms (refer to toggle_platform() and platform.as_str()
usage) so disabling and re-enabling Signal properly tears down and recreates the
runtime adapter.
---
Nitpick comments:
In `@src/tools/cron.rs`:
- Around line 431-441: Add a regression test to cover the case where
normalize_delivery_target is given a delivery_target that already names a
different instance (e.g., "signal:other_instance:...") while current_adapter is
a different named instance; ensure the test asserts that
normalize_delivery_target does NOT rewrite the other-instance target and returns
the original string. Locate normalize_delivery_target in src/tools/cron.rs and
add a unit test (or expand the existing test module around the tests covering
same-instance qualified targets near the other similar case at lines ~530-541)
that passes a parsed-style target produced by
crate::messaging::target::parse_delivery_target and a different current_adapter
value, then assert equality with the original delivery_target.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b389d9e-75c2-482c-8cff-5005a5ba96b1
📒 Files selected for processing (12)
interface/src/api/client.tsinterface/src/components/ChannelEditModal.tsxinterface/src/components/ChannelSettingCard.tsxinterface/src/lib/platformIcons.tsxinterface/src/routes/Settings.tsxprompts/en/adapters/cron.md.j2src/api/messaging.rssrc/cron/scheduler.rssrc/messaging/target.rssrc/tools.rssrc/tools/cron.rssrc/tools/send_message_to_another_channel.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- interface/src/api/client.ts
- interface/src/lib/platformIcons.tsx
- interface/src/routes/Settings.tsx
- interface/src/components/ChannelEditModal.tsx
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/api/messaging.rs (1)
521-586:⚠️ Potential issue | 🟠 Major
messaging_status()still drops Signal’s platform status.This builds
PlatformStatusinto_signal_statusand then throws it away, so the response never exposes Signal’s configured/enabled state. The instances list helps, but the top-level status contract is still incomplete for the web UI.Suggested fix
pub(super) struct MessagingStatusResponse { discord: PlatformStatus, slack: PlatformStatus, telegram: PlatformStatus, email: PlatformStatus, webhook: PlatformStatus, twitch: PlatformStatus, + signal: PlatformStatus, instances: Vec<AdapterInstanceStatus>, } - let (discord, slack, telegram, email, webhook, twitch, instances) = if config_path.exists() { + let (discord, slack, telegram, email, webhook, twitch, signal, instances) = if config_path.exists() { ... - let _signal_status = doc + let signal_status = doc .get("messaging") .and_then(|m| m.get("signal")) ... ( discord_status, slack_status, telegram_status, email_status, webhook_status, twitch_status, + signal_status, instances, ) } else { let default = PlatformStatus { configured: false, enabled: false, }; ( default.clone(), default.clone(), default.clone(), default.clone(), default.clone(), - default, + default.clone(), + default, Vec::new(), ) }; Ok(Json(MessagingStatusResponse { discord, slack, telegram, email, webhook, twitch, + signal, instances, }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/messaging.rs` around lines 521 - 586, The code builds Signal's PlatformStatus into the variable `_signal_status` and then drops it, so change the binding to keep it (e.g., `signal_status` instead of `_signal_status`) and ensure that `signal_status` is included in the function's returned/top-level status map so the web UI sees Signal's configured/enabled state; update the `messaging_status()` logic that constructs PlatformStatus (the closure that currently returns PlatformStatus and calls `push_instance_status`) to return and propagate `signal_status` into the final response alongside the existing instances, not just rely on `instances` populated by `push_instance_status`.
🤖 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 1167-1180: The code currently creates a new ArcSwap and calls
state.set_signal_permissions(...) inside the toggle path (in the block that
constructs SignalPermissions from_config), which causes the API to miss the
startup adapter's swap; instead, seed ApiState.signal_permissions at startup and
reuse that same ArcSwap here: check for an existing ArcSwap in ApiState
(state.signal_permissions) and, if present, update its pointee rather than
creating a new ArcSwap; if not present (startup fallback) then create and set it
as you do now. Refer to ApiState.signal_permissions, set_signal_permissions,
SignalPermissions::from_config and the startup swap created in main.rs when
wiring the swap through to the ApiState so the same arc_swap instance is used by
both the adapter and the API.
In `@src/messaging/target.rs`:
- Around line 273-294: Update the E.164 validator to match the frontend by
requiring at least 8 digits after '+': in is_valid_e164 change the minimum check
from digits.len() < 7 to digits.len() < 8 (so it enforces /^\+[1-9]\d{7,14}$/);
then update downstream expectations that rely on this rule — adjust tests in the
messaging API test suite (references in api messaging tests) and change the
hardcoded shorthand error string used in send_message_to_another_channel to
reflect the new "8 digits" minimum.
In `@src/tools/send_message_to_another_channel.rs`:
- Around line 146-175: The code incorrectly treats the presence of the plain
"signal" adapter as an ambiguity when combined with named adapters; change the
logic in the block that collects signal_adapters (from
self.messaging_manager.adapter_names()) to detect whether the default "signal"
adapter exists and count only named adapters (those starting with "signal:"). If
"signal" exists, do not error (leave target.adapter as "signal"); otherwise, if
exactly one named adapter exists, set target.adapter to that named instance; if
more than one named adapter exists and no plain "signal" present, return the
ambiguity SendMessageError. Update the check around target.adapter,
self.current_adapter, signal_adapters, and the error branch accordingly so
broadcast() can still perform exact lookup when "signal" is present.
---
Duplicate comments:
In `@src/api/messaging.rs`:
- Around line 521-586: The code builds Signal's PlatformStatus into the variable
`_signal_status` and then drops it, so change the binding to keep it (e.g.,
`signal_status` instead of `_signal_status`) and ensure that `signal_status` is
included in the function's returned/top-level status map so the web UI sees
Signal's configured/enabled state; update the `messaging_status()` logic that
constructs PlatformStatus (the closure that currently returns PlatformStatus and
calls `push_instance_status`) to return and propagate `signal_status` into the
final response alongside the existing instances, not just rely on `instances`
populated by `push_instance_status`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5308c193-efb4-4dd4-8f9e-8ccae4d8da39
📒 Files selected for processing (5)
interface/src/components/ChannelSettingCard.tsxsrc/api/messaging.rssrc/api/state.rssrc/messaging/target.rssrc/tools/send_message_to_another_channel.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- interface/src/components/ChannelSettingCard.tsx
ibhagwan
force-pushed
the
feat/signal-web-ui-config
branch
2 times, most recently
from
68aef7a to
cb44a63
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/messaging/target.rs (1)
273-284:⚠️ Potential issue | 🟠 MajorTighten the E.164 lower bound to match the web flow.
Line 283 still accepts numbers with only 7 digits after
+(e.g.+1234567). That is looser than the Signal web validation introduced in this PR, so API/tool parsing can accept values the form rejects.Suggested fix
/// Requirements: /// - Must start with '+' /// - First digit after '+' must be 1-9 (not 0) -/// - Minimum 7 digits total (6 after '+') -/// - Maximum 15 digits total (14 after '+', E.164 standard) +/// - Minimum 8 digits after '+' +/// - Maximum 15 digits after '+' (E.164 standard) /// - All characters after '+' must be ASCII digits pub fn is_valid_e164(phone: &str) -> bool { if let Some(digits) = phone.strip_prefix('+') { - if digits.len() < 7 || digits.len() > 15 { + if digits.len() < 8 || digits.len() > 15 { return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/target.rs` around lines 273 - 284, The is_valid_e164 function currently allows phone numbers with 7 digits after '+', which is too permissive; update the length check in is_valid_e164 from digits.len() < 7 to digits.len() < 8 so it requires at least 8 digits after '+' (and keep the existing upper bound of 15), and update the function doc comment/requirements to reflect the new minimum (minimum 8 digits after '+' / 9 digits total if you prefer consistent wording) so the implementation and docs match the tightened web-flow validation.
🧹 Nitpick comments (1)
src/api/messaging.rs (1)
1439-1445: Avoid abbreviated binding names (acc) in credential parsing.Please use a full variable name for readability and guideline compliance.
Suggested rename
- Some(acc) => acc, + Some(account_value) => account_value,As per coding guidelines "Don't abbreviate variable names. Use
queuenotq,messagenotmsg,channelnotch. Common abbreviations likeconfigare fine".Also applies to: 1636-1642
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/messaging.rs` around lines 1439 - 1445, Replace the abbreviated binding name "acc" used when matching credentials.signal_account with a descriptive name like "signal_account" (or "signal_account_ref") to improve readability and follow guidelines; update the match arm in the parsing logic where credentials.signal_account.as_ref().filter(|s| !s.is_empty()) is matched (and the similar occurrence around the other match at the later location) so the Some(...) binding uses the full variable name and adjust any subsequent uses in that scope accordingly.
🤖 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 1425-1431: The code currently accepts signal_http_url if non-empty
but doesn't reject whitespace-only or malformed URLs; update the handling around
credentials.signal_http_url (used in both the default Signal branch and the
named Signal branch) to first trim() the string, reject if empty after trim,
then attempt to parse/normalize it (e.g., using Url::parse or equivalent) and
ensure it has an allowed scheme (http or https) before assigning/persisting; on
parse or validation failure return or log an explicit error so
malformed/whitespace URLs are never written to config.
---
Duplicate comments:
In `@src/messaging/target.rs`:
- Around line 273-284: The is_valid_e164 function currently allows phone numbers
with 7 digits after '+', which is too permissive; update the length check in
is_valid_e164 from digits.len() < 7 to digits.len() < 8 so it requires at least
8 digits after '+' (and keep the existing upper bound of 15), and update the
function doc comment/requirements to reflect the new minimum (minimum 8 digits
after '+' / 9 digits total if you prefer consistent wording) so the
implementation and docs match the tightened web-flow validation.
---
Nitpick comments:
In `@src/api/messaging.rs`:
- Around line 1439-1445: Replace the abbreviated binding name "acc" used when
matching credentials.signal_account with a descriptive name like
"signal_account" (or "signal_account_ref") to improve readability and follow
guidelines; update the match arm in the parsing logic where
credentials.signal_account.as_ref().filter(|s| !s.is_empty()) is matched (and
the similar occurrence around the other match at the later location) so the
Some(...) binding uses the full variable name and adjust any subsequent uses in
that scope accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c20ecf93-ee6a-4941-b0c1-11da0964f9fb
📒 Files selected for processing (5)
interface/src/components/ChannelSettingCard.tsxsrc/api/messaging.rssrc/api/state.rssrc/messaging/target.rssrc/tools/send_message_to_another_channel.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- interface/src/components/ChannelSettingCard.tsx
- src/api/state.rs
ibhagwan
force-pushed
the
feat/signal-web-ui-config
branch
from
cb44a63 to
6a5ca6f
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
ibhagwan
force-pushed
the
feat/signal-web-ui-config
branch
from
6a5ca6f to
5d946fe
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
interface/src/components/ChannelSettingCard.tsx (1)
975-977:⚠️ Potential issue | 🟡 MinorAlign helper text with the actual non-zero first-digit validation rule.
The field hint currently omits the
first digit must be 1-9rule, which can mislead users entering+0....Suggested text update
- Your Signal phone number in E.164 format (+ followed by 6-14 digits) + Your Signal phone number in E.164 format (+ followed by 6-14 digits, first digit 1-9)🤖 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 975 - 977, Update the helper text shown in the ChannelSettingCard component to reflect the actual validation rule that the first digit after the "+" cannot be zero; specifically change the copy near the phone number hint (the <p> element in ChannelSettingCard.tsx that currently reads "Your Signal phone number in E.164 format (+ followed by 6-14 digits)") to include "first digit must be 1-9" (e.g., "Your Signal phone number in E.164 format (+ followed by 6–14 digits, first digit must be 1–9)") so the displayed hint matches the non-zero first-digit validation enforced by the form.src/api/messaging.rs (1)
1425-1431:⚠️ Potential issue | 🟠 MajorTrim and validate
signal_http_urlbefore persisting.The current checks still accept whitespace-only and malformed values, which can be saved and fail later during adapter start.
Suggested fix pattern (apply in both default + named Signal branches)
- let http_url = match credentials.signal_http_url.as_ref().filter(|s| !s.is_empty()) { + let http_url = match credentials + .signal_http_url + .as_deref() + .map(str::trim) + .filter(|s| !s.is_empty()) + { Some(url) => url, None => { /* validation failure response */ } }; + if !(http_url.starts_with("http://") || http_url.starts_with("https://")) { + return Ok(Json(MessagingInstanceActionResponse { + success: false, + message: "signal: http_url must start with http:// or https://".to_string(), + })); + }Also applies to: 1622-1628
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/messaging.rs` around lines 1425 - 1431, The code allows whitespace-only or malformed signal_http_url to pass through; update the logic that computes http_url from credentials.signal_http_url to first trim the string (e.g., via .trim()) and ensure it is non-empty, then validate/parse it (e.g., Url::parse or reqwest::Url::parse) and only accept the value if parsing succeeds; if parsing fails, treat it as None/error as appropriate. Apply the exact same trimming+parsing fix in both the default and named Signal branches where signal_http_url is used (the code paths that assign to http_url from credentials.signal_http_url).src/messaging/target.rs (1)
273-294:⚠️ Potential issue | 🟠 MajorAlign E.164 bounds with the web form.
is_valid_e164()currently allows+123456and rejects 15-digit numbers, so backend validation diverges from the frontend constraint (^\+[1-9]\d{7,14}$).Suggested fix
- if digits.len() < 6 || digits.len() > 14 { + if digits.len() < 8 || digits.len() > 15 { return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/target.rs` around lines 273 - 294, The validation in is_valid_e164 currently allows 6–14 digits after '+' but the frontend expects 8–15 digits after '+' (regex ^\+[1-9]\d{7,14}$), so update the length bounds: change the length check in is_valid_e164 to require digits.len() >= 8 && digits.len() <= 15 (i.e. return false if digits.len() < 8 || digits.len() > 15), keep the first-digit non-'0' check (digits.chars().next()) and the digits.chars().all(|c| c.is_ascii_digit()) check unchanged.
🧹 Nitpick comments (1)
src/api/messaging.rs (1)
1444-1445: Prefer full variable names in new branches.
accis abbreviated; use a full name (account_value,account_input, etc.) for readability and consistency.As per coding guidelines: "Don't abbreviate variable names. Use
queuenotq,messagenotmsg,channelnotch."Also applies to: 1641-1642
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/messaging.rs` around lines 1444 - 1445, In the match arms where the variable is named `acc` (e.g., the branch showing `Some(acc) => acc, None => {`), rename `acc` to a full, descriptive identifier such as `account_value` or `account_input` to follow the no-abbreviation guideline; update both the `Some(...)` pattern and any downstream uses so the return and pattern remain consistent. Apply the same renaming to the other occurrence around the 1641-1642 region so both match arms and their uses use the same descriptive name (`account_value`/`account_input`) for readability and consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tools/cron.rs`:
- Around line 431-439: normalize_delivery_target currently rewrites
delivery_target whenever current_adapter appears to be a named instance of
parsed.adapter, but parse_delivery_target only preserves named-instance
semantics for Signal, which can misroute other platforms; update
normalize_delivery_target so the rewrite is only applied when parsed.adapter
indicates the Signal platform (e.g., parsed.adapter == "signal" or the SIGNAL
adapter constant) in addition to the existing current_adapter check, so this
function only substitutes named Signal instances until generic named-instance
parsing is implemented in parse_delivery_target.
---
Duplicate comments:
In `@interface/src/components/ChannelSettingCard.tsx`:
- Around line 975-977: Update the helper text shown in the ChannelSettingCard
component to reflect the actual validation rule that the first digit after the
"+" cannot be zero; specifically change the copy near the phone number hint (the
<p> element in ChannelSettingCard.tsx that currently reads "Your Signal phone
number in E.164 format (+ followed by 6-14 digits)") to include "first digit
must be 1-9" (e.g., "Your Signal phone number in E.164 format (+ followed by
6–14 digits, first digit must be 1–9)") so the displayed hint matches the
non-zero first-digit validation enforced by the form.
In `@src/api/messaging.rs`:
- Around line 1425-1431: The code allows whitespace-only or malformed
signal_http_url to pass through; update the logic that computes http_url from
credentials.signal_http_url to first trim the string (e.g., via .trim()) and
ensure it is non-empty, then validate/parse it (e.g., Url::parse or
reqwest::Url::parse) and only accept the value if parsing succeeds; if parsing
fails, treat it as None/error as appropriate. Apply the exact same
trimming+parsing fix in both the default and named Signal branches where
signal_http_url is used (the code paths that assign to http_url from
credentials.signal_http_url).
In `@src/messaging/target.rs`:
- Around line 273-294: The validation in is_valid_e164 currently allows 6–14
digits after '+' but the frontend expects 8–15 digits after '+' (regex
^\+[1-9]\d{7,14}$), so update the length bounds: change the length check in
is_valid_e164 to require digits.len() >= 8 && digits.len() <= 15 (i.e. return
false if digits.len() < 8 || digits.len() > 15), keep the first-digit non-'0'
check (digits.chars().next()) and the digits.chars().all(|c| c.is_ascii_digit())
check unchanged.
---
Nitpick comments:
In `@src/api/messaging.rs`:
- Around line 1444-1445: In the match arms where the variable is named `acc`
(e.g., the branch showing `Some(acc) => acc, None => {`), rename `acc` to a
full, descriptive identifier such as `account_value` or `account_input` to
follow the no-abbreviation guideline; update both the `Some(...)` pattern and
any downstream uses so the return and pattern remain consistent. Apply the same
renaming to the other occurrence around the 1641-1642 region so both match arms
and their uses use the same descriptive name (`account_value`/`account_input`)
for readability and consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8069f225-dbea-4331-b01c-8c0be86d3512
📒 Files selected for processing (13)
interface/src/api/client.tsinterface/src/components/ChannelEditModal.tsxinterface/src/components/ChannelSettingCard.tsxinterface/src/lib/platformIcons.tsxinterface/src/routes/Settings.tsxprompts/en/adapters/cron.md.j2src/api/messaging.rssrc/api/state.rssrc/cron/scheduler.rssrc/messaging/target.rssrc/tools.rssrc/tools/cron.rssrc/tools/send_message_to_another_channel.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- src/cron/scheduler.rs
- interface/src/api/client.ts
- interface/src/routes/Settings.tsx
- prompts/en/adapters/cron.md.j2
- src/tools.rs
ibhagwan
force-pushed
the
feat/signal-web-ui-config
branch
from
5d946fe to
977c175
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/tools/cron.rs (1)
481-492: Consider usingOption<&str>for more idiomatic signature.The function works correctly, but taking
&Option<String>is slightly less idiomatic thanOption<&str>. This is a minor ergonomics improvement.♻️ Optional signature improvement
-fn normalize_delivery_target(delivery_target: &str, current_adapter: &Option<String>) -> String { - if let Some(parsed) = crate::messaging::target::parse_delivery_target(delivery_target) - && let Some(current_adapter) = current_adapter.as_ref() +fn normalize_delivery_target(delivery_target: &str, current_adapter: Option<&str>) -> String { + if let Some(parsed) = crate::messaging::target::parse_delivery_target(delivery_target) + && let Some(current_adapter) = current_adapterAnd update the call site at line 268:
- normalize_delivery_target(&delivery_target, &self.current_adapter) + normalize_delivery_target(&delivery_target, self.current_adapter.as_deref())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/cron.rs` around lines 481 - 492, Change normalize_delivery_target signature from fn normalize_delivery_target(delivery_target: &str, current_adapter: &Option<String>) -> String to fn normalize_delivery_target(delivery_target: &str, current_adapter: Option<&str>) -> String; inside the function use the Option<&str> directly (e.g., if let Some(parsed) = crate::messaging::target::parse_delivery_target(delivery_target) && let Some(current_adapter) = current_adapter { ... }) and adjust formatting/starts_with checks to use the &str; update all call sites that currently pass &Option<String> to pass an Option<&str> instead (e.g., convert an Option<String> with .as_deref() or .map(|s| s.as_str()) before calling normalize_delivery_target).
🤖 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/agents.rs`:
- Around line 840-846: The code must not substitute a fresh MessagingManager
when state.messaging_manager is None; change the logic in the agent creation
path that currently builds messaging_manager (the block reading
state.messaging_manager) to instead fail fast or defer cron wiring until the
shared manager exists: check state.messaging_manager.read().await and if None,
return an Err (or propagate a creation error) rather than calling
MessagingManager::new(), or postpone initializing CronContext/CronTool until
after set_messaging_manager(...) has been called; update the code paths that
construct CronContext/CronTool (and any use of deps.messaging_manager) to rely
on the shared manager being Some and avoid creating a new
Arc::new(crate::messaging::MessagingManager::new()) fallback.
---
Nitpick comments:
In `@src/tools/cron.rs`:
- Around line 481-492: Change normalize_delivery_target signature from fn
normalize_delivery_target(delivery_target: &str, current_adapter:
&Option<String>) -> String to fn normalize_delivery_target(delivery_target:
&str, current_adapter: Option<&str>) -> String; inside the function use the
Option<&str> directly (e.g., if let Some(parsed) =
crate::messaging::target::parse_delivery_target(delivery_target) && let
Some(current_adapter) = current_adapter { ... }) and adjust
formatting/starts_with checks to use the &str; update all call sites that
currently pass &Option<String> to pass an Option<&str> instead (e.g., convert an
Option<String> with .as_deref() or .map(|s| s.as_str()) before calling
normalize_delivery_target).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f1e9d924-def9-4ac2-bc67-8779dc15efb7
📒 Files selected for processing (4)
src/api/agents.rssrc/main.rssrc/tools/cron.rssrc/tools/send_message_to_another_channel.rs
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/agents.rs`:
- Around line 840-846: Fetch and validate the messaging manager once at the
start of create_agent_internal: read state.messaging_manager once, convert to
the Arc (e.g., let messaging_manager = ...), fail early if missing, then reuse
that same Arc when assigning deps.messaging_manager and when constructing the
CronTool instead of re-reading state.messaging_manager later; update uses at
lines referenced (where deps.messaging_manager is set and where CronTool is
created) to use the previously captured messaging_manager to avoid inconsistent
reads and partial persistence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5bee9b74-25ad-4031-b33d-59eb5217ef47
📒 Files selected for processing (4)
src/api/agents.rssrc/main.rssrc/tools/cron.rssrc/tools/send_message_to_another_channel.rs
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/api/agents.rs (1)
777-785:⚠️ Potential issue | 🟠 MajorThis still fails after persistent side effects.
By the time this lookup runs, Line 567 has already rewritten
config.toml, and the agent directories/DB/settings have already been created. Ifstate.messaging_manageris stillNone, the request returns an error after partially creating the agent, and the next retry will just hit "already exists". Please move this check ahead of the config/file/db work, or defer cron wiring until messaging is initialized.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/agents.rs` around lines 777 - 785, The lookup of state.messaging_manager (and the creation of messaging_manager) happens after agent config/files/DB are created, causing partially-created agents on failure; move the state.messaging_manager check earlier in the agent creation flow (before any config.toml rewrite, directory/DB creation, or settings persistence) or alternatively delay any cron or messaging wiring until after messaging_manager is Some; locate the block creating messaging_manager from state.messaging_manager.read().await and either hoist that existence check to the start of the function that handles agent creation or wrap cron/messaging setup behind a post-persistence step that runs only when messaging_manager.is_some().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tools/cron.rs`:
- Around line 245-265: When handling explicit_delivery_target in the block
around parse_delivery_target, don’t silently accept a raw delivery_target string
when parse_delivery_target(&delivery_target) returns None; instead return a
recoverable tool error (Err(CronError(...))). Update the branch so that if
parse_delivery_target returns None you produce Err(CronError("invalid delivery
target" or similar)), and when should_resolve_signal_default is true validate
the target_part produced by delivery_target.strip_prefix("signal:") is non-empty
after trimming—if empty return Err(CronError("invalid Signal delivery target")).
Keep using resolve_signal_adapter(&self.messaging_manager,
self.current_adapter.as_deref()).await for adapter resolution and map
adapter-resolution failures to CronError as currently done.
---
Duplicate comments:
In `@src/api/agents.rs`:
- Around line 777-785: The lookup of state.messaging_manager (and the creation
of messaging_manager) happens after agent config/files/DB are created, causing
partially-created agents on failure; move the state.messaging_manager check
earlier in the agent creation flow (before any config.toml rewrite, directory/DB
creation, or settings persistence) or alternatively delay any cron or messaging
wiring until after messaging_manager is Some; locate the block creating
messaging_manager from state.messaging_manager.read().await and either hoist
that existence check to the start of the function that handles agent creation or
wrap cron/messaging setup behind a post-persistence step that runs only when
messaging_manager.is_some().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 089e0483-9c62-40d7-a220-99ee21209b89
📒 Files selected for processing (4)
src/api/agents.rssrc/main.rssrc/tools/cron.rssrc/tools/send_message_to_another_channel.rs
✅ Files skipped from review due to trivial changes (1)
- src/tools/send_message_to_another_channel.rs
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/tools/cron.rs (2)
265-267:⚠️ Potential issue | 🟠 MajorDon't rewrite default targets through a stale adapter.
The explicit-Signal branch resolves against the live manager, but Line 266 rewrites the conversation default with
normalize_delivery_target()without verifying thatself.current_adapteris still registered. If a thread outlives adapter removal or rename, this persistssignal:<stale_instance>:.../slack:<stale_instance>:...and the cron will only fail later at delivery time. Reuse the same live-adapter presence check before rewriting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/cron.rs` around lines 265 - 267, The code is rewriting conversation defaults with normalize_delivery_target(&delivery_target, &self.current_adapter) even when self.current_adapter may have been removed/renamed; change it to only rewrite the default if the current adapter is still registered by reusing the same live-adapter presence check from the explicit-Signal branch (i.e., verify the manager/registry reports self.current_adapter as present/registered before calling normalize_delivery_target and updating the conversation default), otherwise skip the normalization so stale adapter identifiers are not persisted.
252-261:⚠️ Potential issue | 🟠 MajorReject explicit Signal cron targets when no Signal adapter is running.
resolve_signal_adapter()returns"signal"on the zero-adapter path so the send-message tool can fail later at broadcast time. Reusing it here meanscreate()will save asignal:+...cron even when no Signal adapter exists, so the job is guaranteed to fail on its first run. Turn the zero-adapter case into aCronErrorbefore persisting the config.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/cron.rs` around lines 252 - 261, The code currently uses resolve_signal_adapter(...) in the should_resolve_signal_default branch and persists a "signal:..." cron even when no Signal adapter exists; instead, call resolve_signal_adapter(&self.messaging_manager, self.current_adapter.as_deref()).await and if it returns the zero-adapter sentinel ("signal") treat this as a failure by returning CronError (e.g., map_err or explicit check) so create() aborts; specifically, after getting resolved_adapter from resolve_signal_adapter, if resolved_adapter == "signal" return Err(CronError("No Signal adapter running".into())) rather than formatting and saving a signal target with parsed_delivery_target.target.src/api/agents.rs (1)
531-538:⚠️ Potential issue | 🟠 MajorCapture
messaging_managerhere instead of re-reading it later.This block only proves the manager exists at check time. Lines 786-794 perform a second read after
config.tomlis rewritten and the agent directories/databases are created, so the partial-create race still exists if the manager is cleared between the two reads. Clone theArchere and reuse it fordeps.messaging_manager,CronContext.messaging_manager, andCronTool::new(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/agents.rs` around lines 531 - 538, The current pre-check reads state.messaging_manager to verify existence but does not capture the Arc, allowing a race if it is cleared later; change the check block to clone the Arc from state.messaging_manager (e.g., let messaging_manager = guard.as_ref().cloned().ok_or_else(...)? or clone the Option<Arc<...>> you found) and then use that cloned messaging_manager for deps.messaging_manager, CronContext.messaging_manager, and the CronTool::new(...) call so all subsequent uses share the same captured Arc rather than re-reading state.messaging_manager later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/api/agents.rs`:
- Around line 531-538: The current pre-check reads state.messaging_manager to
verify existence but does not capture the Arc, allowing a race if it is cleared
later; change the check block to clone the Arc from state.messaging_manager
(e.g., let messaging_manager = guard.as_ref().cloned().ok_or_else(...)? or clone
the Option<Arc<...>> you found) and then use that cloned messaging_manager for
deps.messaging_manager, CronContext.messaging_manager, and the
CronTool::new(...) call so all subsequent uses share the same captured Arc
rather than re-reading state.messaging_manager later.
In `@src/tools/cron.rs`:
- Around line 265-267: The code is rewriting conversation defaults with
normalize_delivery_target(&delivery_target, &self.current_adapter) even when
self.current_adapter may have been removed/renamed; change it to only rewrite
the default if the current adapter is still registered by reusing the same
live-adapter presence check from the explicit-Signal branch (i.e., verify the
manager/registry reports self.current_adapter as present/registered before
calling normalize_delivery_target and updating the conversation default),
otherwise skip the normalization so stale adapter identifiers are not persisted.
- Around line 252-261: The code currently uses resolve_signal_adapter(...) in
the should_resolve_signal_default branch and persists a "signal:..." cron even
when no Signal adapter exists; instead, call
resolve_signal_adapter(&self.messaging_manager,
self.current_adapter.as_deref()).await and if it returns the zero-adapter
sentinel ("signal") treat this as a failure by returning CronError (e.g.,
map_err or explicit check) so create() aborts; specifically, after getting
resolved_adapter from resolve_signal_adapter, if resolved_adapter == "signal"
return Err(CronError("No Signal adapter running".into())) rather than formatting
and saving a signal target with parsed_delivery_target.target.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8604c55b-636a-438c-88d2-7abcc797da49
📒 Files selected for processing (4)
src/api/agents.rssrc/main.rssrc/tools/cron.rssrc/tools/send_message_to_another_channel.rs
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/api/agents.rs (1)
531-538: Minor redundancy: messaging_manager is read twice.The early guard (lines 533-538) validates that the messaging manager exists, but doesn't capture the
Arc. Lines 786-794 then read it again to clone. While this works correctly (the first guard ensures we fail early before side effects), a single read-and-clone at line 533 would be cleaner and eliminate any theoretical TOCTOU window.This is low-risk since the manager is unlikely to become
Noneonce set, and the past review comment was marked as addressed, so this is just a minor improvement suggestion.♻️ Optional: Consolidate into a single read
- // Fail early if messaging manager is unavailable — before any config write, - // directory creation, or database init that would leave a half-created agent. - { - let guard = state.messaging_manager.read().await; - guard.as_ref().ok_or_else(|| { - "Messaging manager not initialized. Please ensure messaging adapters are configured before creating agents.".to_string() - })?; - } + // Fail early if messaging manager is unavailable — before any config write, + // directory creation, or database init that would leave a half-created agent. + let messaging_manager = { + let guard = state.messaging_manager.read().await; + guard + .as_ref() + .cloned() + .ok_or_else(|| { + "Messaging manager not initialized. Please ensure messaging adapters are configured before creating agents.".to_string() + })? + };Then later at lines 786-794, simply remove that block and use the already-captured
messaging_manager.Also applies to: 786-794
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/agents.rs` around lines 531 - 538, The code reads state.messaging_manager twice; replace the initial read-and-check block (using state.messaging_manager.read().await) with a single read that captures and clones the Arc into a local variable (e.g., messaging_manager = guard.as_ref().cloned().ok_or_else(...)?), then remove the later redundant read at the second block and use the captured messaging_manager wherever needed; ensure you preserve the original error message and early-fail behavior from the guard.as_ref().ok_or_else(...) call and update references that previously used the second read to use the new local variable (symbols: state.messaging_manager, guard, messaging_manager).src/tools/send_message_to_another_channel.rs (1)
413-449: Make the resolver matrix unit-testable without aMessagingManagerseam.
MessagingManager::new()only exercises the empty-manager path today, so the default/single-named/ambiguous/current-instance branches are still mostly hidden behindadapter_names().awaitand one of them is left as a TODO. Extract the “choose adapter from names + current adapter” logic into a small pure helper, and keepresolve_signal_adapter()as the async wrapper.Also applies to: 752-774
🤖 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 413 - 449, Extract the pure decision logic out of resolve_signal_adapter into a small, synchronous helper (e.g., choose_signal_adapter(names: &[String], current_adapter: Option<&str>) -> Result<String, String>) that takes the adapter name list and optional current_adapter and implements the existing branching (default "signal", single named, ambiguous error, honour current_adapter when it's a specific "signal:<instance>" and behavior when default exists). Keep resolve_signal_adapter as the async wrapper that calls messaging_manager.adapter_names().await to collect names and then delegates to choose_signal_adapter; update any other similar resolver usage (the logic at the other occurrence mentioned) to call the same helper so unit tests can feed name vectors directly without needing a MessagingManager.
🤖 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/api/agents.rs`:
- Around line 531-538: The code reads state.messaging_manager twice; replace the
initial read-and-check block (using state.messaging_manager.read().await) with a
single read that captures and clones the Arc into a local variable (e.g.,
messaging_manager = guard.as_ref().cloned().ok_or_else(...)?), then remove the
later redundant read at the second block and use the captured messaging_manager
wherever needed; ensure you preserve the original error message and early-fail
behavior from the guard.as_ref().ok_or_else(...) call and update references that
previously used the second read to use the new local variable (symbols:
state.messaging_manager, guard, messaging_manager).
In `@src/tools/send_message_to_another_channel.rs`:
- Around line 413-449: Extract the pure decision logic out of
resolve_signal_adapter into a small, synchronous helper (e.g.,
choose_signal_adapter(names: &[String], current_adapter: Option<&str>) ->
Result<String, String>) that takes the adapter name list and optional
current_adapter and implements the existing branching (default "signal", single
named, ambiguous error, honour current_adapter when it's a specific
"signal:<instance>" and behavior when default exists). Keep
resolve_signal_adapter as the async wrapper that calls
messaging_manager.adapter_names().await to collect names and then delegates to
choose_signal_adapter; update any other similar resolver usage (the logic at the
other occurrence mentioned) to call the same helper so unit tests can feed name
vectors directly without needing a MessagingManager.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3649aa92-18cc-48af-9cda-751114c6d8f2
📒 Files selected for processing (4)
src/api/agents.rssrc/main.rssrc/tools/cron.rssrc/tools/send_message_to_another_channel.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tools/cron.rs
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tools/cron.rs`:
- Around line 238-279: The code only special-cases bare "signal" adapters but
leaves explicit bare adapters like "slack:C123" unchanged, causing jobs to be
undeliverable when only a named instance is running; update the
explicit_delivery_target branch so that after parsing (parse_delivery_target) if
parsed_delivery_target.adapter != "signal" and
messaging_manager.has_adapter(parsed_delivery_target.adapter).await returns
false, then if self.current_adapter.is_some() and
messaging_manager.has_adapter(self.current_adapter.as_ref().unwrap()).await is
true call normalize_delivery_target(&delivery_target, &self.current_adapter) to
rewrite the explicit target to the running named instance; if no suitable
adapter is available return a CronError (structured result) instructing the
caller to use platform:<instance>:... instead of leaving the delivery_target
unchanged; keep existing resolve_signal_adapter logic for "signal" as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f5e794b3-9f67-43ef-b60b-11f771929908
📒 Files selected for processing (4)
src/api/agents.rssrc/main.rssrc/tools/cron.rssrc/tools/send_message_to_another_channel.rs
✅ Files skipped from review due to trivial changes (1)
- src/main.rs
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/tools/cron.rs (1)
514-665: Addcreate()regression tests for the adapter-resolution branches.These tests lock down
normalize_delivery_target()and parser behavior, but the failure-prone logic lives inCronTool::create()—resolve_signal_adapter,has_adapter, and the explicit-vs-default split. A small async test matrix around explicitsignal:<target>, explicitsignal:<instance>:<target>, and bare named-only adapters would cover the real regression surface.Based on learnings: named Signal adapter instances are intentionally started independently of the root
signal_config.enabledflag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/cron.rs` around lines 514 - 665, Add async regression tests targeting CronTool::create to exercise adapter-resolution branches: call CronTool::create (or its async test harness) with contexts for (1) explicit signal:<target>, (2) explicit signal:<instance>:<target>, and (3) bare named-only adapters, and assert that resolve_signal_adapter/has_adapter behavior matches normalize_delivery_target/parse_delivery_target expectations (i.e., explicit instance strings resolve to adapter "signal:<instance>", bare "signal" resolves to adapter "signal", and already-qualified targets are unchanged). Also include checks that named Signal adapter instances are started/recognized even when the root signal_config.enabled is false (verify the instance exists/started while root flag is false). Use the same helper functions (resolve_signal_adapter, has_adapter) or CronTool API to locate adapter state in the test.
🤖 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/agents.rs`:
- Around line 531-541: After acquiring the config mutex (_config_guard)
revalidate that the requested agent_id is still unique before mutating
config.toml: read the current agent list from the locked config (or re-parse the
TOML/refresh the agents list) and return an error if agent_id already exists;
only proceed to append/write to config.toml and create the agent after this
second check. Reference the existing symbols agent_id, _config_guard, and the
code that appends to config.toml to locate where to insert the re-check.
In `@src/tools/cron.rs`:
- Around line 271-279: The error message when an adapter isn't running currently
always suggests adding ":<instance>" even when parsed_delivery_target.adapter is
already instance-qualified; update the branch in has_adapter check to detect if
parsed_delivery_target.adapter contains a ':' and only append the "Use
'<adapter>:<instance>:...'" hint when it's a bare platform name (no colon).
Modify the code around messaging_manager.has_adapter(...) /
parsed_delivery_target.adapter to conditionally construct the CronError message:
if adapter contains ':' return a simple "No '<adapter>' adapter running"
CronError, otherwise include the instance hint as before.
---
Nitpick comments:
In `@src/tools/cron.rs`:
- Around line 514-665: Add async regression tests targeting CronTool::create to
exercise adapter-resolution branches: call CronTool::create (or its async test
harness) with contexts for (1) explicit signal:<target>, (2) explicit
signal:<instance>:<target>, and (3) bare named-only adapters, and assert that
resolve_signal_adapter/has_adapter behavior matches
normalize_delivery_target/parse_delivery_target expectations (i.e., explicit
instance strings resolve to adapter "signal:<instance>", bare "signal" resolves
to adapter "signal", and already-qualified targets are unchanged). Also include
checks that named Signal adapter instances are started/recognized even when the
root signal_config.enabled is false (verify the instance exists/started while
root flag is false). Use the same helper functions (resolve_signal_adapter,
has_adapter) or CronTool API to locate adapter state in the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b688f1d-b3a9-4cc6-91c0-69d8f3e74c1c
📒 Files selected for processing (4)
src/api/agents.rssrc/main.rssrc/tools/cron.rssrc/tools/send_message_to_another_channel.rs
✅ Files skipped from review due to trivial changes (1)
- src/tools/send_message_to_another_channel.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main.rs
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/api/agents.rs (1)
531-538:⚠️ Potential issue | 🟠 MajorRe-check the locked TOML, not the stale agent cache.
Line 534 still consults
state.agent_configs, but_config_guardis dropped at Line 596 and the new ID is not published back tostate.agent_configsuntil Line 994. A second create request can therefore pass this check after the first one has already written the sameagent_idtoconfig.toml, and you'll append a duplicate[[agents]]entry anyway. Revalidate against the parseddoc/agents_arrayimmediately beforepush.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/agents.rs` around lines 531 - 538, The current uniqueness re-check uses state.agent_configs.load() (stale cache) after acquiring the lock (_config_guard) which can miss concurrent writes; instead, before appending the new agent to the parsed TOML document, revalidate against the in-memory parsed document (e.g., the local doc and agents_array variables) to ensure no entry with the same agent_id exists, and return the duplicate error if found; perform this check immediately prior to the agents_array.push so the check reflects the locked file state rather than state.agent_configs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tools/cron.rs`:
- Around line 287-293: When building the final delivery target before
persisting, don’t silently accept the raw conversation-derived delivery_target
if current_adapter was captured earlier but is no longer present; instead, after
the existing branch that calls normalize_delivery_target(&delivery_target,
&self.current_adapter) and in the fallback branch where you currently return
delivery_target, run the same validation: parse/normalize the delivery_target
(using normalize_delivery_target or the same parsing routine used in
normalize_delivery_target), check availability via
self.messaging_manager.has_adapter(parsed.adapter).await, and if the adapter is
missing return a CronError from create() rather than persisting an undeliverable
job. Ensure this touches the code paths around current_adapter,
messaging_manager.has_adapter, normalize_delivery_target, delivery_target and
create() so both branches validate adapter availability consistently.
---
Duplicate comments:
In `@src/api/agents.rs`:
- Around line 531-538: The current uniqueness re-check uses
state.agent_configs.load() (stale cache) after acquiring the lock
(_config_guard) which can miss concurrent writes; instead, before appending the
new agent to the parsed TOML document, revalidate against the in-memory parsed
document (e.g., the local doc and agents_array variables) to ensure no entry
with the same agent_id exists, and return the duplicate error if found; perform
this check immediately prior to the agents_array.push so the check reflects the
locked file state rather than state.agent_configs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7699f07d-5bb8-43eb-9bf9-42da7d32e0c6
📒 Files selected for processing (4)
src/api/agents.rssrc/main.rssrc/tools/cron.rssrc/tools/send_message_to_another_channel.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main.rs
- src/tools/send_message_to_another_channel.rs
Extract Signal adapter resolution logic from SendMessageTool into a reusable public function `resolve_signal_adapter()`. This function handles: - Using default 'signal' adapter when present - Falling back to current named adapter when no default exists - Returning error when multiple named adapters exist without default Update CronTool to: - Accept MessagingManager in constructor - Resolve Signal adapters for explicit 'signal:' delivery targets - Prevent cron jobs from failing when deployment has only named Signal instances Add unit tests for resolve_signal_adapter covering default resolution, current adapter fallback, and error cases. Update call sites in main.rs and api/agents.rs to pass MessagingManager.
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/tools/send_message_to_another_channel.rs (1)
325-326: Renamechtochannelper coding guidelines.The variable name
chshould bechannelto follow the project's naming conventions.Proposed fix
let channel = match channel_result { - Some(ch) => ch, + Some(channel) => channel, None => {As per coding guidelines: "Don't abbreviate variable names. Use
queuenotq,messagenotmsg,channelnotch."🤖 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 325 - 326, In the match on channel_result inside send_message_to_another_channel.rs replace the short binding name `ch` with the full `channel` identifier so the code reads `Some(channel) => channel,` (and update any subsequent references within that scope to `channel`) to comply with the project's naming guideline of avoiding abbreviated variable names.
🤖 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/tools/send_message_to_another_channel.rs`:
- Around line 325-326: In the match on channel_result inside
send_message_to_another_channel.rs replace the short binding name `ch` with the
full `channel` identifier so the code reads `Some(channel) => channel,` (and
update any subsequent references within that scope to `channel`) to comply with
the project's naming guideline of avoiding abbreviated variable names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58ff5096-54d1-46a7-93a8-889131558e27
📒 Files selected for processing (4)
src/api/agents.rssrc/main.rssrc/tools/cron.rssrc/tools/send_message_to_another_channel.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main.rs
- src/tools/cron.rs
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
2 participants
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.
As per title, this adds signal configuration to web interface settings/channels.
@jamiepine, couple of questions:
1. Do you want to support multi-instance signal channels from the web?SUPPORTED2. How do I add docs to the spacebot.sh website like we have for telegram: https://docs.spacebot.sh/telegram-setup
Note
Adds Signal messaging platform support to the web configuration UI. Includes frontend input fields for signal-cli HTTP URL and account phone number (E.164 format validation), backend credential handling with auto-population of account to dm_allowed_users, and comprehensive E.164 phone number validation tests.
Written by Tembo for commit 0d763a7. This will update automatically on new commits.
Note
Updated Summary: Comprehensive Signal adapter integration including API endpoints for configuring Signal credentials with E.164 phone number validation, frontend UI components (ChannelEditModal and ChannelSettingCard) for Signal-specific inputs, and enhancements to cron job and message routing systems. Backend validates and normalizes E.164 phone numbers (+ prefix with 6-15 digits, first digit 1-9), auto-populates dm_allowed_users from account number, and includes fallback number parsing for multiple formats.
Written by Tembo for commit 0d4c4f6. This will update automatically on new commits.