feat(messaging): add Signal adapter via signal-cli JSON-RPC daemon#347
Open
ibhagwan wants to merge 2 commits intospacedriveapp:mainfrom
Open
feat(messaging): add Signal adapter via signal-cli JSON-RPC daemon#347ibhagwan wants to merge 2 commits intospacedriveapp:mainfrom
ibhagwan wants to merge 2 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: new Signal messaging adapter (SSE inbound, JSON‑RPC outbound), config schema/types/permissions, prompt fragments and docs, per-turn adapter propagation into agent tooling and send-message flows, Signal target parsing/normalization, and hot-reload wiring for Signal adapters and permissions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Member
|
Woah! Thank you, huge. |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
Author
|
Ty @jamiepine for this wonderful project, although lesser known ATM I believe this one has huge potential, I've been personally testing every decent project out there and so far I like this one and IronClaw the most. Btw, this isn't my first signal support PR, I've added the same to ZeroClaw, IronClaw, NullClaw and Hermes-agent: |
![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: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/secrets/store.rs (1)
1103-1119:⚠️ Potential issue | 🟠 MajorSignal secret registration is still incomplete.
This only helps for whatever
SignalConfig::secret_fields()exposes, and that implementation currently declareshttp_urlbut notaccount. As a result,SIGNAL_ACCOUNTandSIGNAL_<INSTANCE>_ACCOUNTwill still auto-categorize as Tool secrets and can leak into worker env vars.🧩 Required follow-up in
src/config/types.rsfn secret_fields() -> &'static [SecretField] { - &[SecretField { - toml_key: "http_url", - secret_name: "SIGNAL_HTTP_URL", - instance_pattern: Some(InstancePattern { - platform_prefix: "SIGNAL", - field_suffix: "HTTP_URL", - }), - }] + &[ + SecretField { + toml_key: "http_url", + secret_name: "SIGNAL_HTTP_URL", + instance_pattern: Some(InstancePattern { + platform_prefix: "SIGNAL", + field_suffix: "HTTP_URL", + }), + }, + SecretField { + toml_key: "account", + secret_name: "SIGNAL_ACCOUNT", + instance_pattern: Some(InstancePattern { + platform_prefix: "SIGNAL", + field_suffix: "ACCOUNT", + }), + }, + ] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/secrets/store.rs` around lines 1103 - 1119, Signal secret registration misses the Signal account key: update SignalConfig::secret_fields (in the SignalConfig implementation in src/config/types.rs) to include the account field alongside http_url so SIGNAL_ACCOUNT and SIGNAL_<INSTANCE>_ACCOUNT are treated as secret; modify the SignalConfig struct/impl to return the account secret name (matching naming used elsewhere for instance-scoped secrets) in secret_fields so the secrets store registers it correctly.src/agent/channel.rs (1)
1789-1800:⚠️ Potential issue | 🟠 MajorThis still drops named-instance context before it reaches
SendMessageTool.
self.current_adapter()only derives fromsource_adapter/conversation_id, and both are populated from the base source (message.source). A conversation running onsignal:supporttherefore still passes justsignalhere, so explicit Signal sends can escape onto the default instance instead of the active one. Capture and preferInboundMessage.adapterwhen establishing the channel adapter context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 1789 - 1800, The code currently passes self.current_adapter() into add_channel_tools which is derived from source_adapter/conversation_id and can lose named-instance context; change the adapter argument to prefer the InboundMessage.adapter (or equivalent inbound message field available in this scope) when present, falling back to self.current_adapter() only if InboundMessage.adapter is None, so the channel context reaching SendMessageTool preserves the explicit inbound adapter instance.
🧹 Nitpick comments (3)
src/agent/channel_history.rs (1)
320-328: Minor: Double space whensender_contextis empty.For non-Signal adapters (Slack, Discord, Telegram, Twitch),
sender_contextis not set, resulting in an empty string. The format string" {sender_context} "then produces consecutive spaces in the output (e.g.,"Alice [timestamp]: text").Consider conditionally including the leading space only when
sender_contextis non-empty:🔧 Proposed fix
let sender_context = message .metadata .get("sender_context") .and_then(|v| v.as_str()) - .unwrap_or(""); + .filter(|s| !s.is_empty()) + .map(|s| format!(" {s}")) + .unwrap_or_default(); format!( - "{display_name}{bot_tag}{reply_context} {sender_context} [{timestamp_text}]: {text_content}" + "{display_name}{bot_tag}{reply_context}{sender_context} [{timestamp_text}]: {text_content}" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel_history.rs` around lines 320 - 328, The formatted string currently always inserts a space before and after sender_context, causing a double space when sender_context is empty; update the code around the retrieval of sender_context (the variable named sender_context and the format! call that builds the final line) to conditionally include the leading space only when sender_context is non-empty (e.g., compute a sender_context_prefixed that is either "" or " "+sender_context and use that in the format string) so the output has no extra spaces for non-Signal adapters.src/api/bindings.rs (1)
35-60: Keep the update endpoint symmetric with the new group permission fields.
CreateBindingRequestnow acceptsgroup_ids/group_allowed_users, butUpdateBindingRequestbelow still cannot modify them. That leaves API clients with delete-and-recreate as the only way to change group-based binding permissions on an existing record.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/bindings.rs` around lines 35 - 60, UpdateBindingRequest is missing the new group permission fields present on CreateBindingRequest, preventing updates to group-based permissions; add group_ids: Vec<String> with #[serde(default)] and group_allowed_users: Vec<String> with #[serde(default)] to the UpdateBindingRequest definition (use the same types and serde defaults as CreateBindingRequest) so the update endpoint can modify group_ids and group_allowed_users symmetrically with creation.src/config/load.rs (1)
2031-2031: Renamestosignal_configin this loader block.This closure is long enough that the single-letter binding makes the field mapping harder to scan than it needs to be.
As per coding guidelines, "Use non-abbreviated variable names in Rust code:
queuenotq,messagenotmsg,channelnotch; common abbreviations likeconfigare acceptable".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/load.rs` at line 2031, The closure mapping toml.messaging.signal currently binds its argument as a single-letter `s`, which reduces readability; rename that binding to `signal_config` in the closure used to build `signal:` (i.e., change `toml.messaging.signal.and_then(|s| { ... }` to `and_then(|signal_config| { ... }`) and update all references inside the closure (field mappings and method calls) to use `signal_config` so the intent is clearer while preserving behavior.
🤖 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/agent/channel.rs`:
- Around line 2134-2139: The send of the user-visible error via
self.response_tx.send(OutboundResponse::Text(error_msg)).await currently ignores
the Result with `let _ =`, which silently discards failures; update the code in
channel.rs around the error path where error_msg is built to handle the send
result explicitly—either call .ok() to mark it as best-effort
(self.response_tx.send(...).await.ok()) or log the error when send returns Err
(e.g., capture the Result and call a logger with context including error_msg and
the send Err); reference the send call, response_tx, OutboundResponse::Text, and
error_msg when making the change.
In `@src/config/permissions.rs`:
- Around line 339-343: SignalPermissions currently mixes DM and group allowlists
causing group wildcards to grant DM access; preserve them separately by keeping
dm_allowed_users as the canonical DM allowlist and
group_filter/group_allowed_users as the group-scoped allowlist. Update the code
paths that set ["*"] for group IDs (the logic around group_filter and any places
that assign to dm_allowed_users when encountering a group wildcard) so they only
mutate group_filter or group_allowed_users and never overwrite or merge into
dm_allowed_users; likewise remove any folding of group_allowed_users into
dm_allowed_users in the code that consolidates permissions (the blocks
referenced around the current group handling and the consolidation sections),
and ensure authorization checks use dm_allowed_users for direct messages and
group_filter/group_allowed_users for group messages only.
- Around line 388-428: The group_filter construction currently returns
Some(all_group_ids) even when all_group_ids is empty; change it so that after
aggregating and filtering seed_group_ids and signal_bindings (using
is_valid_base64) you return None when all_group_ids.is_empty() to represent the
"block all" case; keep the existing early returns for wildcard ("*") that
produce Self with group_filter Some(vec!["*"]) and dm_allowed_users vec!["*"],
but replace the final Some(all_group_ids) with None when no valid IDs remain so
consumers don’t receive Some(vec![]); update the block where group_filter is set
(the let group_filter = { ... } scope) to perform this empty-check before
returning.
In `@src/config/types.rs`:
- Around line 1330-1339: Binding::matches currently ignores Signal-specific
metadata so resolve_agent_for_message will incorrectly select the first
channel="signal" binding; update Binding::matches to inspect the message's
Signal fields (e.g., signal_chat_type, signal_group_id, sender_id) and enforce
the new binding fields: check that signal_chat_type (or equivalent) matches the
binding, that for group chats signal_group_id is allowed by group_ids or
sender_id is allowed by group_allowed_users, and that for DMs sender_id is
allowed by dm_allowed_users; also respect require_mention and channel_ids when
matching Signal messages so resolve_agent_for_message can correctly route to the
appropriate agent.
- Around line 2341-2374: The SignalConfig treats account as sensitive in Debug
but SystemSecrets.secret_fields only includes http_url; add a SecretField for
the account inside the SignalConfig impl of SystemSecrets (update
secret_fields() to return both SecretField entries) using toml_key "account", a
sensible secret_name like "SIGNAL_ACCOUNT", and the same InstancePattern style
used for http_url (platform_prefix "SIGNAL", field_suffix "ACCOUNT") so the
account/phone number is resolved as a secret consistently with Debug redaction.
In `@src/config/watcher.rs`:
- Around line 233-239: The reload only replaces a local Arc (new_perms) into an
ephemeral ArcSwap when signal_permissions was None or when creating a named
instance, so adapters started after boot keep an unreachable permissions handle
and won't get updates; change the logic to allocate and store a shared
ArcSwap<SignalPermissions> that lives in the watcher's state and hand clones of
that ArcSwap (or Arc<ArcSwap<...>>) to any hot-started Signal adapters instead
of creating a fresh, untracked Arc each time. Specifically, where
signal_permissions is initialized or a named Signal instance is created (the
places that call SignalPermissions::from_config / perms.store now and the
named-instance startup blocks), create or reuse a single ArcSwap container held
in the watcher and update that container on reload (call store with the new
Arc<SignalPermissions>) so all adapters receive updates.
In `@src/conversation/worker_transcript.rs`:
- Around line 451-458: The change currently maps UserContent::Text to
TranscriptStep::Action which causes user messages to be labeled as agent text;
instead, update the branch that matches rig::message::UserContent::Text in
worker_transcript.rs to emit TranscriptStep::UserText (preserving the same inner
text payload) rather than TranscriptStep::Action so worker_inspect.rs will
render it as "**User:**". Locate the match arm handling UserContent::Text and
replace the Action emission with the UserText variant, keeping the text
cloning/empty-and-system checks intact.
In `@src/messaging/signal.rs`:
- Around line 581-585: The tracing::info calls (e.g., the one using
sender_display, text.len(), is_group and the other similar calls around the
file) are logging raw Signal identifiers and URLs; update those log statements
to redact or obfuscate sensitive fields instead of printing them raw: replace
sender_display with a redacted/hashed form (e.g., mask or hash the phone/UUID)
and replace any http_url or full URLs with a redacted or sanitized version (keep
only domain or a fixed placeholder), and ensure any helper used
(redact_identifier/redact_url or similar) is applied consistently to the
tracing::info calls at this site and the other occurrences you flagged (around
lines ~677-680 and ~899-900) so no PII/embedded credentials are emitted.
- Around line 287-295: The current logic checks response_body.is_empty() before
validating the HTTP status, allowing non-success statuses with empty bodies to
be treated as Ok(None); update the control flow in the function that reads
response_body and status so the status.is_success() check runs before returning
on an empty body (i.e., if !status.is_success() keep the existing truncated_body
+ anyhow::bail! behavior even when response_body.is_empty()), ensuring failed
send/sendTyping calls propagate an error rather than Ok(None).
- Around line 480-485: The sender resolution currently only checks
envelope.source_number and envelope.source and thus returns None for
privacy-mode envelopes; update the chain to also consider envelope.source_uuid
(e.g., prefer envelope.source_number, then envelope.source, then
envelope.source_uuid) before mapping to String so the new UUID allowlist path
works; reference the existing sender binding and the envelope.source_number /
envelope.source / envelope.source_uuid fields and ensure the final
.map(String::from)? remains.
- Around line 397-405: The code currently joins untrusted `filename` into
`self.tmp_dir`, allowing path traversal or nested paths; fix by extracting and
sanitizing only the final path component before joining: use
Path::new(&filename).file_name().and_then(|s|
s.to_str()).unwrap_or("attachment") (or fallback to the UUID alone) then
optionally replace or strip unsafe characters to produce `safe_name`, build
`unique_name` with Uuid::new_v4() and `safe_name`, and use that when creating
`tmp_path` (still `self.tmp_dir.join(&unique_name)`); ensure you handle the
fallback case so `tmp_path` cannot escape `tmp_dir` and avoid relying on
intermediate directories from the original filename.
- Around line 821-889: The cancel sender is being dropped immediately which
closes cancel_rx and causes the spawned typing task (created in the block using
cancel_tx/cancel_rx and inserted into self.typing_tasks) to exit immediately;
instead of drop(cancel_tx), retain and store the sender so stop_typing() can
call send() to signal cancellation. Modify the storage for typing tasks (e.g.,
replace the current map entry of just the JoinHandle inserted by
typing_tasks.insert(conversation_id, handle) with a small struct or tuple
containing both the JoinHandle and the cancel_tx Sender) or add a separate map
for cancel senders, ensure you insert the cancel_tx alongside the handle, and
remove the explicit drop(cancel_tx); update stop_typing() to use the stored
cancel_tx to signal cancellation before/without aborting the handle.
In `@src/prompts/engine.rs`:
- Around line 476-479: The match in PromptEngine::new() selects
"adapters/signal" for the "signal" branch, but PromptEngine::new() only
registers the "adapters/email" template so "adapters/signal" falls through to
the default None; update PromptEngine::new() (or the environment/template
registration code it calls) to register the "adapters/signal" template alongside
"adapters/email" so that the match on template_name ("adapters/signal") finds a
registered template and returns the proper adapter-specific guidance.
In `@src/tools/send_message_to_another_channel.rs`:
- Around line 249-284: The current hyphen check in parse_explicit_signal_target
misclassifies ordinary hyphenated names as Signal UUIDs; update the hyphen
branch to only accept a true UUID shape (or require the explicit "signal:uuid:"
prefix) before calling parse_delivery_target. Concretely, in
parse_explicit_signal_target replace the loose trimmed.contains('-') &&
trimmed.len() > 8 check with a strict UUID validation (e.g., test with a UUID
regex or attempt to parse as a UUID) and only then call
parse_delivery_target(&format!("signal:uuid:{trimmed}")), otherwise fall through
to channel lookup.
- Around line 141-166: The code currently unconditionally replaces
explicit_target.adapter with self.current_adapter, which causes explicit Signal
targets (e.g., parse_explicit_signal_target returning "signal:+1555...") to be
sent via the wrong adapter; change the logic in the explicit-target branch so
you only assign self.current_adapter to explicit_target.adapter when the parsed
adapter is empty/unspecified (i.e., do not overwrite when
parse_explicit_signal_target returned a concrete adapter like "signal" or
"signal:..."); update the block around parse_explicit_signal_target,
explicit_target.adapter, and the subsequent messaging_manager.broadcast call to
use the parsed adapter unless it was empty.
---
Outside diff comments:
In `@src/agent/channel.rs`:
- Around line 1789-1800: The code currently passes self.current_adapter() into
add_channel_tools which is derived from source_adapter/conversation_id and can
lose named-instance context; change the adapter argument to prefer the
InboundMessage.adapter (or equivalent inbound message field available in this
scope) when present, falling back to self.current_adapter() only if
InboundMessage.adapter is None, so the channel context reaching SendMessageTool
preserves the explicit inbound adapter instance.
In `@src/secrets/store.rs`:
- Around line 1103-1119: Signal secret registration misses the Signal account
key: update SignalConfig::secret_fields (in the SignalConfig implementation in
src/config/types.rs) to include the account field alongside http_url so
SIGNAL_ACCOUNT and SIGNAL_<INSTANCE>_ACCOUNT are treated as secret; modify the
SignalConfig struct/impl to return the account secret name (matching naming used
elsewhere for instance-scoped secrets) in secret_fields so the secrets store
registers it correctly.
---
Nitpick comments:
In `@src/agent/channel_history.rs`:
- Around line 320-328: The formatted string currently always inserts a space
before and after sender_context, causing a double space when sender_context is
empty; update the code around the retrieval of sender_context (the variable
named sender_context and the format! call that builds the final line) to
conditionally include the leading space only when sender_context is non-empty
(e.g., compute a sender_context_prefixed that is either "" or " "+sender_context
and use that in the format string) so the output has no extra spaces for
non-Signal adapters.
In `@src/api/bindings.rs`:
- Around line 35-60: UpdateBindingRequest is missing the new group permission
fields present on CreateBindingRequest, preventing updates to group-based
permissions; add group_ids: Vec<String> with #[serde(default)] and
group_allowed_users: Vec<String> with #[serde(default)] to the
UpdateBindingRequest definition (use the same types and serde defaults as
CreateBindingRequest) so the update endpoint can modify group_ids and
group_allowed_users symmetrically with creation.
In `@src/config/load.rs`:
- Line 2031: The closure mapping toml.messaging.signal currently binds its
argument as a single-letter `s`, which reduces readability; rename that binding
to `signal_config` in the closure used to build `signal:` (i.e., change
`toml.messaging.signal.and_then(|s| { ... }` to `and_then(|signal_config| { ...
}`) and update all references inside the closure (field mappings and method
calls) to use `signal_config` so the intent is clearer while preserving
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7ebd32c-d659-4f3e-b98b-298d5a3dd4f0
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lock,!**/*.lockCargo.tomlis excluded by!**/*.toml
📒 Files selected for processing (23)
prompts/en/adapters/signal.md.j2prompts/en/tools/send_message_description.md.j2src/agent/channel.rssrc/agent/channel_history.rssrc/api/bindings.rssrc/config.rssrc/config/load.rssrc/config/permissions.rssrc/config/toml_schema.rssrc/config/types.rssrc/config/watcher.rssrc/conversation/worker_transcript.rssrc/llm/model.rssrc/main.rssrc/messaging.rssrc/messaging/signal.rssrc/messaging/target.rssrc/prompts/engine.rssrc/prompts/text.rssrc/secrets/scrub.rssrc/secrets/store.rssrc/tools.rssrc/tools/send_message_to_another_channel.rs
Author
|
This needs some work, I'll change it to draft until I respond to all the review comments |
Member
This is wonderful to hear, thank you. I'm putting a load of effort into making this the best I can. Hoping soon it will pick up traction wise, but so far the contributions have been amazing and we're moving quickly to stability. If you want to chat please join the Discord and/or DM me anytime! |
ibhagwan
force-pushed
the
feat/signal-adapter
branch
from
90cc30d to
b7b7218
Compare
ibhagwan
added a commit
to ibhagwan/spacebot
that referenced
this pull request
Mar 7, 2026
…eapp#347) Fixes critical issues identified in code review: **Critical:** - conversation/worker_transcript.rs: Restore TranscriptStep::UserText emission to prevent user messages being mislabeled as agent text - config/permissions.rs: Separate DM and group permissions to prevent group wildcards from granting DM access - messaging/signal.rs: Move HTTP status check before empty body check to properly fail on non-2xx responses - messaging/signal.rs: Sanitize attachment filenames to prevent path traversal vulnerabilities - messaging/signal.rs: Add source_uuid fallback for sender resolution in privacy-mode envelopes **Medium:** - agent/channel.rs: Use .ok() instead of let _ = for best-effort error channel sends - config/types.rs: Add SecretField for Signal account to treat phone number as sensitive - prompts/engine.rs: Register adapters/signal template for proper Signal-specific guidance - tools/send_message_to_another_channel.rs: Only overwrite adapter when explicit target has no adapter specified All changes pass cargo check.
ibhagwan
added a commit
to ibhagwan/spacebot
that referenced
this pull request
Mar 7, 2026
…eapp#347) Fixes critical issues identified in code review: **Critical:** - conversation/worker_transcript.rs: Restore TranscriptStep::UserText emission to prevent user messages being mislabeled as agent text - config/permissions.rs: Separate DM and group permissions to prevent group wildcards from granting DM access - messaging/signal.rs: Move HTTP status check before empty body check to properly fail on non-2xx responses - messaging/signal.rs: Sanitize attachment filenames to prevent path traversal vulnerabilities - messaging/signal.rs: Add source_uuid fallback for sender resolution in privacy-mode envelopes **Medium:** - agent/channel.rs: Use .ok() instead of let _ = for best-effort error channel sends - config/types.rs: Add SecretField for Signal account to treat phone number as sensitive - prompts/engine.rs: Register adapters/signal template for proper Signal-specific guidance - tools/send_message_to_another_channel.rs: Only overwrite adapter when explicit target has no adapter specified **Low** - agent/channel_history.rs: Condition space before sender_context on non-empty value to prevent double spacing All changes pass cargo check.
ibhagwan
force-pushed
the
feat/signal-adapter
branch
from
e741a28 to
b2509ac
Compare
ibhagwan
added a commit
to ibhagwan/spacebot
that referenced
this pull request
Mar 7, 2026
…eapp#347) Fixes critical issues identified in code review: **Critical:** - conversation/worker_transcript.rs: Restore TranscriptStep::UserText emission to prevent user messages being mislabeled as agent text - config/permissions.rs: Separate DM and group permissions to prevent group wildcards from granting DM access - messaging/signal.rs: Move HTTP status check before empty body check to properly fail on non-2xx responses - messaging/signal.rs: Sanitize attachment filenames to prevent path traversal vulnerabilities - messaging/signal.rs: Add source_uuid fallback for sender resolution in privacy-mode envelopes **Medium:** - agent/channel.rs: Use .ok() instead of let _ = for best-effort error channel sends - config/types.rs: Add SecretField for Signal account to treat phone number as sensitive - prompts/engine.rs: Register adapters/signal template for proper Signal-specific guidance - tools/send_message_to_another_channel.rs: Only overwrite adapter when explicit target has no adapter specified **Low:** - agent/channel_history.rs: Condition space before sender_context on non-empty value to prevent double spacing - api/bindings.rs: Add group_ids and group_allowed_users to UpdateBindingRequest for symmetric updates All changes pass cargo check.
ibhagwan
force-pushed
the
feat/signal-adapter
branch
from
b2509ac to
22b4717
Compare
ibhagwan
added a commit
to ibhagwan/spacebot
that referenced
this pull request
Mar 7, 2026
…eapp#347) Fixes critical issues identified in code review: **Critical:** - conversation/worker_transcript.rs: Restore TranscriptStep::UserText emission to prevent user messages being mislabeled as agent text - config/permissions.rs: Separate DM and group permissions to prevent group wildcards from granting DM access - messaging/signal.rs: Move HTTP status check before empty body check to properly fail on non-2xx responses - messaging/signal.rs: Sanitize attachment filenames to prevent path traversal vulnerabilities - messaging/signal.rs: Add source_uuid fallback for sender resolution in privacy-mode envelopes **Medium:** - agent/channel.rs: Use .ok() instead of let _ = for best-effort error channel sends - config/types.rs: Add SecretField for Signal account to treat phone number as sensitive - prompts/engine.rs: Register adapters/signal template for proper Signal-specific guidance - tools/send_message_to_another_channel.rs: Only overwrite adapter when explicit target has no adapter specified - agent/channel.rs: Add adapter parameter to run_agent_turn, prefer InboundMessage.adapter to preserve named-instance context - api/bindings.rs: Add group_ids and group_allowed_users to UpdateBindingRequest for symmetric updates **Low:** - agent/channel_history.rs: Condition space before sender_context on non-empty value to prevent double spacing All changes pass cargo check.
ibhagwan
force-pushed
the
feat/signal-adapter
branch
from
22b4717 to
dd419d7
Compare
ibhagwan
added a commit
to ibhagwan/spacebot
that referenced
this pull request
Mar 7, 2026
…eapp#347) Fixes critical issues identified in code review: **Critical:** - conversation/worker_transcript.rs: Restore TranscriptStep::UserText emission to prevent user messages being mislabeled as agent text - config/permissions.rs: Separate DM and group permissions to prevent group wildcards from granting DM access - messaging/signal.rs: Move HTTP status check before empty body check to properly fail on non-2xx responses - messaging/signal.rs: Sanitize attachment filenames to prevent path traversal vulnerabilities - messaging/signal.rs: Add source_uuid fallback for sender resolution in privacy-mode envelopes **Medium:** - agent/channel.rs: Use .ok() instead of let _ = for best-effort error channel sends - config/types.rs: Add SecretField for Signal account to treat phone number as sensitive - prompts/engine.rs: Register adapters/signal template for proper Signal-specific guidance - tools/send_message_to_another_channel.rs: Only overwrite adapter when explicit target has no adapter specified - agent/channel.rs: Add adapter parameter to run_agent_turn, prefer InboundMessage.adapter to preserve named-instance context - api/bindings.rs: Add group_ids and group_allowed_users to UpdateBindingRequest for symmetric updates **Low:** - agent/channel_history.rs: Condition space before sender_context on non-empty value to prevent double spacing All changes pass cargo check.
ibhagwan
force-pushed
the
feat/signal-adapter
branch
5 times, most recently
from
aba09a9 to
0d2bf8f
Compare
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
1 similar comment
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
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)
1325-1333:⚠️ Potential issue | 🟠 MajorPreserve the concrete adapter on batched turns.
Line 1332 still passes
self.current_adapter(), which is derived from the channel/source and can lose the per-message adapter instance. The single-message path at Lines 1625-1628 now prefersmessage.adapter, but the coalesced path does not, so a batched Signal conversation on a non-default instance can still send explicitsignal:*targets through the wrong adapter inSendMessageTool.Suggested fix
- let (result, skip_flag, replied_flag, _) = self + let adapter = messages + .iter() + .find_map(|message| message.adapter.as_deref()) + .or_else(|| self.current_adapter()); + let (result, skip_flag, replied_flag, _) = self .run_agent_turn( &combined_text, &system_prompt, &conversation_id, attachment_parts, false, // not a retrigger - self.current_adapter(), + adapter, ) .await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 1325 - 1333, The batched/coalesced turn is still calling run_agent_turn(..., self.current_adapter()) which can lose a per-message adapter; change the coalesced-path call site to preserve and pass the concrete adapter instance used for those messages (the same adapter you use for single-message handling via message.adapter) into run_agent_turn (or into the coalescing routine) so SendMessageTool receives the correct adapter for signal:* targets; update the coalescing logic that builds combined_text/attachment_parts to carry the chosen adapter and pass that adapter instead of self.current_adapter().
♻️ Duplicate comments (4)
src/config/watcher.rs (1)
233-239:⚠️ Potential issue | 🟠 MajorHot-started Signal adapters still get untracked permission handles.
When Signal is enabled after boot, or when a named instance starts here, this code allocates a fresh
ArcSwapand passes it straight into the adapter. Later reloads only updatesignal_permissions, so those adapters keep stale DM/group filters until restart. The watcher needs one shared, stored handle per live Signal adapter.Also applies to: 541-547, 562-574
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/watcher.rs` around lines 233 - 239, The current watcher creates a fresh Arc/ArcSwap and hands it directly to newly started Signal adapters (via the code path that calls SignalPermissions::from_config and perms.store(...)), so adapters started after boot keep their own Arc and never see subsequent reloads; instead ensure the watcher maintains one shared ArcSwap<SignalPermissions> per live Signal adapter and always pass a clone of that shared ArcSwap handle into adapter constructors. Concretely: stop allocating a new ArcSwap for each adapter start, centralize the ArcSwap (the existing signal_permissions variable) so adapters receive signal_permissions.clone() (or an Arc clone of the ArcSwap/holder) and update SignalPermissions::from_config -> perms.store(Arc::new(new_perms)) on reloads; apply the same change where Signal adapters are instantiated (the branches that touch signal_permissions, SignalPermissions::from_config, and perms.store) so adapters observe reloads instead of keeping stale filters.src/tools/send_message_to_another_channel.rs (1)
147-156:⚠️ Potential issue | 🟠 MajorDon't overwrite explicitly selected Signal instances.
This still rewrites any explicit Signal target onto
current_adapteras long as the current conversation came from Signal.signal:work:+1555...sent from asignal:personalthread will go out on the wrong account. Only the bare shorthand forms should inherit the current adapter.Suggested guard
- if let Some(current_adapter) = self - .current_adapter - .as_ref() - .filter(|adapter| adapter.starts_with("signal")) - { - explicit_target.adapter = current_adapter.clone(); - } + if !args.target.trim_start().starts_with("signal:") + && let Some(current_adapter) = self + .current_adapter + .as_ref() + .filter(|adapter| adapter.starts_with("signal")) + { + explicit_target.adapter = current_adapter.clone(); + }🤖 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 147 - 156, The code currently overwrites any Signal target with self.current_adapter when the conversation is from Signal; change this so only bare shorthand targets inherit the current adapter. In the block that handles parse_explicit_signal_target(&args.target) (and sets explicit_target.adapter), first detect whether the caller provided an explicit adapter (e.g., args.target contains an adapter prefix like "signal:" or parse_explicit_signal_target produced a non-empty adapter); only if the target is a bare shorthand (no adapter specified) assign explicit_target.adapter = current_adapter.clone() — otherwise leave the explicit adapter untouched. Use the existing symbols parse_explicit_signal_target, explicit_target.adapter, args.target and self.current_adapter to implement the guard.src/messaging/signal.rs (2)
559-579:⚠️ Potential issue | 🔴 CriticalKeep DM and group sender scopes separate.
This re-merges
dm_allowed_usersinto group authorization. Withdm_allowed_users = ["*"], every sender in an allowed group passes even whengroup_allowed_usersis empty, so the new group-specific allowlist never actually narrows access.🔒 Proposed fix
- let all_group_users: Vec<&String> = permissions - .dm_allowed_users - .iter() - .chain(permissions.group_allowed_users.iter()) - .collect(); - - let sender_allowed = if all_group_users.is_empty() { + let sender_allowed = if permissions.group_allowed_users.is_empty() { false // Empty = block all - } else if all_group_users.iter().any(|u| u.as_str() == "*") { + } else if permissions + .group_allowed_users + .iter() + .any(|user| user.as_str() == "*") + { true // Wildcard = allow all } else { - all_group_users.iter().any(|allowed| { - allowed.as_str() == sender + permissions.group_allowed_users.iter().any(|allowed| { + allowed.as_str() == sender.as_str() || envelope .source_uuid .as_deref()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/signal.rs` around lines 559 - 579, The group authorization logic incorrectly merges dm_allowed_users into the group allowlist; update the check so it uses only permissions.group_allowed_users (remove dm_allowed_users from all_group_users) when computing sender_allowed for groups, keeping dm_allowed_users separate for DM checks; preserve the semantics for empty (deny all), wildcard "*" (allow all), and matching by sender or envelope.source_uuid in the group_allowed_users-based logic (referencing dm_allowed_users, group_allowed_users, all_group_users, sender_allowed, and envelope.source_uuid to locate the change).
105-111:⚠️ Potential issue | 🟠 Major
redact_url()still exposes URL userinfo.Keeping the first three
/-separated components preservesuser:pass@host:port, so a credentialedhttp_urlstill lands in logs. Parse the URL and emit onlyscheme://host[:port], or a fixed placeholder if parsing fails.🛡️ Proposed fix
fn redact_url(url: &str) -> String { - url.split('/') - .take(3) // scheme://domain - .collect::<Vec<_>>() - .join("/") - .to_string() + match reqwest::Url::parse(url) { + Ok(parsed) => match parsed.host_str() { + Some(host) => { + let mut redacted = format!("{}://{}", parsed.scheme(), host); + if let Some(port) = parsed.port() { + redacted.push_str(&format!(":{port}")); + } + redacted + } + None => "[redacted-url]".to_string(), + }, + Err(_) => "[redacted-url]".to_string(), + } }As per coding guidelines, "Compliance/privacy risks (PII retention, logging sensitive data -- like emails and other user identifiers, GDPR/CCPA violations)" are major issues.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/signal.rs` around lines 105 - 111, The current redact_url function still leaks URL userinfo because it naively keeps the first three slash-separated components; update redact_url to properly parse the input URL (using a URL parser) and reconstruct only the scheme and host (including port if present) in the form "scheme://host[:port]", explicitly omitting any userinfo, and return a fixed placeholder (e.g., "redacted://") if parsing fails or the scheme/host are missing; reference the redact_url function to locate and replace the naive split/join logic.
🤖 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/bindings.rs`:
- Around line 157-166: The update_binding() path currently ignores the new
Signal permission fields so updates never persist group_ids and
group_allowed_users; modify update_binding() to copy these fields from the
incoming binding DTO into the in-memory binding (assign binding.group_ids =
dto.group_ids and binding.group_allowed_users = dto.group_allowed_users) and
ensure the updated config is written back to TOML using the same persistence
path used elsewhere (the existing save/write config routine in this module) so
changes to group_ids and group_allowed_users are persisted; keep types as
Vec<String> to match toml_schema.rs and respect serde defaults.
In `@src/config/permissions.rs`:
- Around line 397-419: The wildcard-handling branches inside the permission
assembly prematurely return a new Self (seen in the branches that set
group_filter to Some(vec!["*".to_string()])) which prevents merging the rest of
the allowlists (dm_allowed_users and group_allowed_users) from signal_bindings
and seed lists; change those branches to set only the relevant field (e.g., set
group_filter = Some(vec!["*".to_string()]) or dm_allowed_users =
vec!["*".to_string()]) and continue processing instead of returning, then after
iterating all signal_bindings build and return the final Self using the merged
dm_allowed_users, group_allowed_users, and group_filter so wildcards finalize
only their own field while allowing other lists to be combined (refer to
symbols: all_group_ids, signal_bindings, group_filter, dm_allowed_users,
group_allowed_users).
In `@src/config/types.rs`:
- Around line 1504-1511: Signal DM handling in SignalConfig is inverted: update
the block that checks self.dm_allowed_users for DMs so that when
dm_allowed_users is empty DMs are ignored, and when non-empty only listed
sender_ids are allowed. Specifically, in the Signal DM branch (SignalConfig /
method handling message routing that currently references self.dm_allowed_users
and message.sender_id) change the condition to return false if
self.dm_allowed_users.is_empty() or if
!self.dm_allowed_users.contains(&message.sender_id), so DMs are blocked when the
list is empty and otherwise only allowed for contained sender_ids.
In `@src/messaging/signal.rs`:
- Around line 887-897: The typing-indicator loop currently only handles
transport errors from client.post(...).send().await and keeps retrying on HTTP
4xx/5xx; change the block in the typing task that calls
client.post(&rpc_url).timeout(RPC_REQUEST_TIMEOUT).header(...).json(&body).send().await
to capture the Response on success, check response.status().is_success(), and if
not successful log the status and response body (or error) and break the
loop—follow the same status-checking pattern used by the rpc_request() function
so server-side failures stop the typing loop instead of retrying.
In `@src/messaging/target.rs`:
- Around line 335-374: parse_signal_target_parts currently handles e164 and
phone branches itself, causing inconsistent parsing vs the shared normalizer;
update parse_signal_target_parts to call the existing normalize_signal_target()
for any phone/e164 branches (both default and named-adapter arms) and use its
returned canonical phone string (or None to reject) when building
BroadcastTarget, keeping the adapter construction logic (adapter: "signal" or
format!("signal:{instance}")) and the uuid/group handling unchanged; this
ensures parse_signal_target_parts mirrors parse_delivery_target() behavior and
accepts bare numeric forms that normalize_signal_target accepts.
In `@src/tools/send_message_to_another_channel.rs`:
- Around line 288-291: The bare-digit branch (checking trimmed.len() >= 7 && all
digits) preempts ChannelStore lookup and wrongly treats numeric channel IDs as
Signal numbers; change the logic so that you only call
crate::messaging::target::parse_delivery_target for Signal when the input
explicitly indicates Signal (e.g., original input starts with "signal:" or an
explicit "+"/international prefix), otherwise fall through to the ChannelStore
resolution path. Update the condition around the trimmed check (and any use of
trimmed) to require an explicit signal indicator, and ensure ChannelStore lookup
remains the default resolution for plain numeric strings.
---
Outside diff comments:
In `@src/agent/channel.rs`:
- Around line 1325-1333: The batched/coalesced turn is still calling
run_agent_turn(..., self.current_adapter()) which can lose a per-message
adapter; change the coalesced-path call site to preserve and pass the concrete
adapter instance used for those messages (the same adapter you use for
single-message handling via message.adapter) into run_agent_turn (or into the
coalescing routine) so SendMessageTool receives the correct adapter for signal:*
targets; update the coalescing logic that builds combined_text/attachment_parts
to carry the chosen adapter and pass that adapter instead of
self.current_adapter().
---
Duplicate comments:
In `@src/config/watcher.rs`:
- Around line 233-239: The current watcher creates a fresh Arc/ArcSwap and hands
it directly to newly started Signal adapters (via the code path that calls
SignalPermissions::from_config and perms.store(...)), so adapters started after
boot keep their own Arc and never see subsequent reloads; instead ensure the
watcher maintains one shared ArcSwap<SignalPermissions> per live Signal adapter
and always pass a clone of that shared ArcSwap handle into adapter constructors.
Concretely: stop allocating a new ArcSwap for each adapter start, centralize the
ArcSwap (the existing signal_permissions variable) so adapters receive
signal_permissions.clone() (or an Arc clone of the ArcSwap/holder) and update
SignalPermissions::from_config -> perms.store(Arc::new(new_perms)) on reloads;
apply the same change where Signal adapters are instantiated (the branches that
touch signal_permissions, SignalPermissions::from_config, and perms.store) so
adapters observe reloads instead of keeping stale filters.
In `@src/messaging/signal.rs`:
- Around line 559-579: The group authorization logic incorrectly merges
dm_allowed_users into the group allowlist; update the check so it uses only
permissions.group_allowed_users (remove dm_allowed_users from all_group_users)
when computing sender_allowed for groups, keeping dm_allowed_users separate for
DM checks; preserve the semantics for empty (deny all), wildcard "*" (allow
all), and matching by sender or envelope.source_uuid in the
group_allowed_users-based logic (referencing dm_allowed_users,
group_allowed_users, all_group_users, sender_allowed, and envelope.source_uuid
to locate the change).
- Around line 105-111: The current redact_url function still leaks URL userinfo
because it naively keeps the first three slash-separated components; update
redact_url to properly parse the input URL (using a URL parser) and reconstruct
only the scheme and host (including port if present) in the form
"scheme://host[:port]", explicitly omitting any userinfo, and return a fixed
placeholder (e.g., "redacted://") if parsing fails or the scheme/host are
missing; reference the redact_url function to locate and replace the naive
split/join logic.
In `@src/tools/send_message_to_another_channel.rs`:
- Around line 147-156: The code currently overwrites any Signal target with
self.current_adapter when the conversation is from Signal; change this so only
bare shorthand targets inherit the current adapter. In the block that handles
parse_explicit_signal_target(&args.target) (and sets explicit_target.adapter),
first detect whether the caller provided an explicit adapter (e.g., args.target
contains an adapter prefix like "signal:" or parse_explicit_signal_target
produced a non-empty adapter); only if the target is a bare shorthand (no
adapter specified) assign explicit_target.adapter = current_adapter.clone() —
otherwise leave the explicit adapter untouched. Use the existing symbols
parse_explicit_signal_target, explicit_target.adapter, args.target and
self.current_adapter to implement the guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a757d08-c636-441b-a7e1-bc51616e900f
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lock,!**/*.lockCargo.tomlis excluded by!**/*.toml
📒 Files selected for processing (23)
prompts/en/adapters/signal.md.j2prompts/en/tools/send_message_description.md.j2src/agent/channel.rssrc/agent/channel_history.rssrc/api/bindings.rssrc/config.rssrc/config/load.rssrc/config/permissions.rssrc/config/toml_schema.rssrc/config/types.rssrc/config/watcher.rssrc/conversation/worker_transcript.rssrc/llm/model.rssrc/main.rssrc/messaging.rssrc/messaging/signal.rssrc/messaging/target.rssrc/prompts/engine.rssrc/prompts/text.rssrc/secrets/scrub.rssrc/secrets/store.rssrc/tools.rssrc/tools/send_message_to_another_channel.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- src/agent/channel_history.rs
- src/config/load.rs
- src/prompts/engine.rs
- src/secrets/store.rs
- src/conversation/worker_transcript.rs
- src/tools.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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)
1325-1333:⚠️ Potential issue | 🟠 MajorPreserve the full runtime adapter key for named instances.
These call sites still fall back to
self.current_adapter(), but that value is derived frommessage.source/ the firstconversation_idsegment, so a named adapter likesignal:supportgets collapsed tosignal. Batched turns and synthetic retriggers will then register channel tools against the default adapter and can send follow-up messages through the wrong account. Please store/propagatemessage.adapteras the canonical per-channel adapter key instead of reconstructing it fromsource.Also applies to: 1625-1637
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 1325 - 1333, The call is using self.current_adapter() which collapses named adapters (e.g., "signal:support") to their transport only; instead propagate and use the full per-channel adapter key stored on the message (message.adapter) when invoking run_agent_turn and any other places that reconstruct adapters (also at the other call site around run_agent_turn lines ~1625-1637). Replace usages of self.current_adapter() for per-message operations with the message.adapter (or a function that returns the full adapter key from the Message struct) so named instances are preserved for registering channel tools and sending follow-ups.
♻️ Duplicate comments (10)
src/messaging/target.rs (2)
335-374:⚠️ Potential issue | 🟠 MajorReuse
normalize_signal_target()inparse_signal_target_parts().This helper still special-cases
e164/phone forms instead of normalizing them through the shared Signal normalizer. That leaves it inconsistent withparse_delivery_target():signal:e164:1234567890becomes1234567890here instead of canonical+1234567890, and bare numeric forms accepted bynormalize_signal_target()are still rejected on this path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/target.rs` around lines 335 - 374, parse_signal_target_parts currently special-cases e164/phone forms instead of reusing normalize_signal_target, causing inconsistent normalization; update parse_signal_target_parts to call normalize_signal_target for branches that handle phone/e164 or single-phone inputs (both default adapter and named adapter cases) and only construct BroadcastTarget when normalize_signal_target returns Some(normalized_target), using adapter "signal" or format!("signal:{instance}") as before; remove the duplicated phone/e164 normalization logic so numeric-only inputs accepted by normalize_signal_target are handled consistently.
104-122:⚠️ Potential issue | 🟠 MajorKeep the named Signal adapter when resolving stored channels.
This branch reparses metadata as
signal:{signal_target}, which hard-codes the default adapter. Sincesrc/messaging/signal.rs:1329-1333only persists the recipient undersignal_target, any channel whose ID encodes a named adapter (for examplesignal:<instance>:...) will resolve back tosignalhere and outbound sends can jump to the wrong daemon/account. Use the adapter encoded inchannel.idfor adapter selection and metadata only for the recipient payload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/target.rs` around lines 104 - 122, When resolving Signal channels in this branch, do not hard-code the adapter as "signal" by calling parse_delivery_target with "signal:{signal_target}"; instead extract the adapter name from channel.id (the first part before the first ':') and combine that adapter with the recipient from platform_meta["signal_target"] so adapter selection uses the channel.id while the metadata supplies only the recipient payload; update the logic around parse_delivery_target and parse_signal_target_parts (and the parts extraction from channel.id) so that when signal_target exists you call parse_delivery_target with "<adapter>:<signal_target>" (adapter taken from channel.id) and otherwise continue to parse parts from channel.id via parse_signal_target_parts.src/tools/send_message_to_another_channel.rs (2)
288-290:⚠️ Potential issue | 🟠 MajorPlain numeric targets still hijack channel IDs.
This parser runs before
ChannelStore::find_by_name(), so"1234567890"will still be coerced intosignal:+1234567890instead of resolving a numeric channel ID. Require an explicit Signal marker here (signal:or+) and leave bare digits for normal channel lookup.Minimal fix
- // Bare phone number (7+ digits) - if trimmed.len() >= 7 && trimmed.chars().all(|c| c.is_ascii_digit()) { - return crate::messaging::target::parse_delivery_target(&format!("signal:+{trimmed}")); - } - // Group ID format: group:xxx (might be passed directly)🤖 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 288 - 290, The current bare-digit check in send_message_to_another_channel.rs incorrectly converts plain numeric strings to a Signal delivery target; change the logic so only explicit Signal markers are accepted: only call parse_delivery_target for inputs that either start with "signal:" or start with '+' followed by digits (e.g., "+1234567"); leave purely numeric trimmed strings untouched so ChannelStore::find_by_name() can resolve numeric channel IDs. Ensure you still construct the "signal:+{trimmed_digits}" form when handling a leading '+' case and keep parse_delivery_target usage for the explicit-signal branch.
150-156:⚠️ Potential issue | 🟠 MajorDon't overwrite explicitly named Signal adapters.
If
parse_explicit_signal_target()resolves something likesignal:work:+1555, this branch still replaces it withself.current_adapterwhenever the current conversation is on Signal. That sends through the wrong Signal account. Only inherit the current adapter for generic targets that resolved to the default"signal"adapter.Suggested guard
- if let Some(current_adapter) = self - .current_adapter - .as_ref() - .filter(|adapter| adapter.starts_with("signal")) - { - explicit_target.adapter = current_adapter.clone(); + if explicit_target.adapter == "signal" { + if let Some(current_adapter) = self + .current_adapter + .as_ref() + .filter(|adapter| adapter.starts_with("signal")) + { + explicit_target.adapter = current_adapter.clone(); + } }🤖 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 150 - 156, The current logic overwrites explicitly resolved Signal adapters (e.g., "signal:work:+1555") by checking only that self.current_adapter starts_with("signal"); instead, only inherit the current adapter when the parsed target was the generic "signal". Change the guard in the block that sets explicit_target.adapter to ensure explicit_target.adapter == "signal" (and still optionally verify self.current_adapter.starts_with("signal")) before assigning self.current_adapter.clone(); reference the symbols explicit_target.adapter and self.current_adapter to locate and update the condition within the send_message_to_another_channel logic.src/messaging/signal.rs (2)
887-897:⚠️ Potential issue | 🟠 MajorStop the typing loop on HTTP failures too.
This branch only handles transport errors.
reqwest::send().awaitstill returnsOk(Response)for 4xx/5xx responses, so the typing task keeps retrying every 4 seconds even when Signal is rejecting the request.rpc_request()already handles statuses correctly.In reqwest, does RequestBuilder::send().await return Err for HTTP 4xx/5xx, or do callers need to inspect Response::status() / call error_for_status() themselves?Minimal fix
- if let Err(error) = client - .post(&rpc_url) - .timeout(RPC_REQUEST_TIMEOUT) - .header("Content-Type", "application/json") - .json(&body) - .send() - .await - { - tracing::debug!(%error, "failed to send signal typing indicator"); - break; - } + match client + .post(&rpc_url) + .timeout(RPC_REQUEST_TIMEOUT) + .header("Content-Type", "application/json") + .json(&body) + .send() + .await + { + Ok(response) if response.status().is_success() => {} + Ok(response) => { + tracing::debug!( + status = %response.status(), + "failed to send signal typing indicator" + ); + break; + } + Err(error) => { + tracing::debug!(%error, "failed to send signal typing indicator"); + break; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/signal.rs` around lines 887 - 897, The current send() branch only treats transport errors as failures, but reqwest::RequestBuilder::send().await returns Ok(Response) for HTTP 4xx/5xx so the typing loop keeps retrying; after awaiting client.post(&rpc_url)...send().await, inspect the Response and treat non-success statuses as errors (e.g., call response.error_for_status() or check !response.status().is_success()) and log the error and break the loop similarly to the Err branch; update the block around client.post(&rpc_url)...timeout(RPC_REQUEST_TIMEOUT)...json(&body).send().await to handle and break on non-2xx responses (same behavior as rpc_request()).
105-112:⚠️ Potential issue | 🟠 Major
redact_url()still leaks embedded credentials.
https://user:pass@host/pathcurrently becomeshttps://user:pass@host, so the health-check debug log can still expose daemon credentials. Rebuild the redacted value from scheme/host/port instead of splitting on/.Suggested fix
fn redact_url(url: &str) -> String { - url.split('/') - .take(3) // scheme://domain - .collect::<Vec<_>>() - .join("/") - .to_string() + match reqwest::Url::parse(url) { + Ok(parsed) => { + let scheme = parsed.scheme(); + let host = parsed.host_str().unwrap_or("[invalid]"); + match parsed.port() { + Some(port) => format!("{scheme}://{host}:{port}"), + None => format!("{scheme}://{host}"), + } + } + Err(_) => "[invalid-url]".to_string(), + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/signal.rs` around lines 105 - 112, redact_url currently leaks embedded credentials by splitting on '/'; update redact_url to parse the input with a URL parser (e.g., Url::parse) and reconstruct the redacted string using the scheme and host (and port if present) only, explicitly omitting username/password and path/query/fragment; locate the function redact_url and replace the split/join logic with parsing and building "scheme://host" (including ":port" when Url::port_or_known_default indicates a non-default port) to ensure credentials are never included.src/config/watcher.rs (1)
233-239:⚠️ Potential issue | 🟠 MajorKeep Signal permissions on a shared
ArcSwap.When
signal_permissionsisNoneor a named instance starts here, these branches allocate a freshArcSwapand pass it only to the new adapter. The watcher never stores that handle, so later reloads stop updating that adapter's DM/group filters.Also applies to: 541-546, 572-574
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/watcher.rs` around lines 233 - 239, The watcher currently creates a fresh ArcSwap for signal permissions and only gives it to the new adapter, so later reloads don't update that adapter; change the logic around signal_permissions and SignalPermissions::from_config so that when you create a new ArcSwap (the ArcSwap holding SignalPermissions produced by SignalPermissions::from_config) you also store that ArcSwap into the shared watcher state (the signal_permissions Option) instead of only passing it to the adapter; ensure the code path that calls perms.store(Arc::new(new_perms)) always operates on the ArcSwap instance that is kept in signal_permissions (create-and-insert into signal_permissions when it was None, and reuse the existing ArcSwap when Some) so subsequent reloads update all adapters.src/api/bindings.rs (1)
157-166:⚠️ Potential issue | 🟠 MajorWire the new Signal group fields through
update_binding().These fields are still dead on the update path.
create_binding()andlist_bindings()handlegroup_ids/group_allowed_users, butupdate_binding()never writes or clears them, so Signal binding edits silently no-op.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/bindings.rs` around lines 157 - 166, The Signal-specific fields group_ids and group_allowed_users are not persisted in update_binding(), so edits to those properties silently no-op; modify update_binding() to read the incoming request's group_ids and group_allowed_users (respecting empty-to-clear semantics) and write them to the stored Binding record just like create_binding() and list_bindings() do, ensuring you handle serde/default-empty Vecs and retain require_mention/dm_allowed_users behavior consistent with the create path.src/config/types.rs (1)
1504-1510:⚠️ Potential issue | 🟠 MajorAn empty Signal DM allowlist still matches every DM.
This still behaves as allow-all when
dm_allowed_usersis empty, which contradictsSignalConfig.dm_allowed_usersand the adapter-level permission checks. It also only comparesmessage.sender_id, so a binding written with the sender's phone number won't match onceSignalAdapternormalizedsender_idto the UUID.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/types.rs` around lines 1504 - 1510, The current DM allowlist logic treats an empty dm_allowed_users as allow-all and only compares message.sender_id (a normalized UUID), which lets mismatched binding entries (e.g., phone numbers) slip through; change the logic so an empty self.dm_allowed_users results in denying the DM (return false) and, when checking membership, compare using the same normalized identifier as SignalAdapter produces (or compare both the normalized UUID and the original raw phone identifier used by bindings) so that contains(&message.sender_id) actually matches real binding entries; update the check around self.dm_allowed_users and the membership test that references message.sender_id to use the normalized id(s).src/config/permissions.rs (1)
393-419:⚠️ Potential issue | 🟠 MajorWildcard branches shouldn't short-circuit the rest of permission merging.
Each
return Self { ... }here exits before the remaining binding-derived allowlists are collected. For example,group_ids = ["*"]skips laterdm_allowed_users/group_allowed_users, anddm_allowed_users = ["*"]drops the configured group-user list. Set the wildcard for that field, then keep merging the others.Also applies to: 449-504
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/permissions.rs` around lines 393 - 419, The current wildcard handling in the group_filter/dm_allowed_users/group_allowed_users logic returns early (using return Self { ... }) when encountering "*" in seed_group_ids or binding.group_ids or dm_allowed_users, which prevents subsequent merging of allowlists; instead, modify the logic in the functions that build group_filter, dm_allowed_users, and group_allowed_users (look for usages of seed_group_ids, signal_bindings, and binding.group_ids / binding.dm_allowed_users) to set the respective field to a wildcard marker (e.g., set group_filter to Some(vec!["*".to_string())] or mark dm_allowed_users/group_allowed_users as wildcard) and then continue processing remaining bindings so other allowlists are still collected and merged; apply the same fix to the other identical block around the 449-504 region so wildcards no longer short-circuit merging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config/types.rs`:
- Around line 1481-1502: The group-matching logic currently treats
self.group_ids and self.group_allowed_users as alternatives; change it so each
non-empty selector is required (cumulative narrowing): for the "group" branch in
the matching function, compute group_allowed only if self.group_ids is non-empty
and return false if that check fails, and likewise compute sender_allowed only
if self.group_allowed_users is non-empty and return false if that fails; use
message.metadata.get("signal_group_id") and message.sender_id as the inputs and
ensure both non-empty selectors must pass instead of using a single OR check.
In `@src/tools/send_message_to_another_channel.rs`:
- Around line 355-366: The tests use a non-canonical fake UUID so
parse_explicit_signal_target returns None; update the fixtures to use a real
canonical UUID string (for example "123e4567-e89b-12d3-a456-426655440000").
Change the parses_signal_bare_uuid test to call parse_explicit_signal_target
with that bare UUID and assert target.adapter == "signal" and target.target ==
"uuid:123e4567-e89b-12d3-a456-426655440000", and also update
parses_signal_uuid_prefixed to use
"signal:uuid:123e4567-e89b-12d3-a456-426655440000" so both tests exercise the
canonical-UUID parsing path in parse_explicit_signal_target.
---
Outside diff comments:
In `@src/agent/channel.rs`:
- Around line 1325-1333: The call is using self.current_adapter() which
collapses named adapters (e.g., "signal:support") to their transport only;
instead propagate and use the full per-channel adapter key stored on the message
(message.adapter) when invoking run_agent_turn and any other places that
reconstruct adapters (also at the other call site around run_agent_turn lines
~1625-1637). Replace usages of self.current_adapter() for per-message operations
with the message.adapter (or a function that returns the full adapter key from
the Message struct) so named instances are preserved for registering channel
tools and sending follow-ups.
---
Duplicate comments:
In `@src/api/bindings.rs`:
- Around line 157-166: The Signal-specific fields group_ids and
group_allowed_users are not persisted in update_binding(), so edits to those
properties silently no-op; modify update_binding() to read the incoming
request's group_ids and group_allowed_users (respecting empty-to-clear
semantics) and write them to the stored Binding record just like
create_binding() and list_bindings() do, ensuring you handle serde/default-empty
Vecs and retain require_mention/dm_allowed_users behavior consistent with the
create path.
In `@src/config/permissions.rs`:
- Around line 393-419: The current wildcard handling in the
group_filter/dm_allowed_users/group_allowed_users logic returns early (using
return Self { ... }) when encountering "*" in seed_group_ids or
binding.group_ids or dm_allowed_users, which prevents subsequent merging of
allowlists; instead, modify the logic in the functions that build group_filter,
dm_allowed_users, and group_allowed_users (look for usages of seed_group_ids,
signal_bindings, and binding.group_ids / binding.dm_allowed_users) to set the
respective field to a wildcard marker (e.g., set group_filter to
Some(vec!["*".to_string())] or mark dm_allowed_users/group_allowed_users as
wildcard) and then continue processing remaining bindings so other allowlists
are still collected and merged; apply the same fix to the other identical block
around the 449-504 region so wildcards no longer short-circuit merging.
In `@src/config/types.rs`:
- Around line 1504-1510: The current DM allowlist logic treats an empty
dm_allowed_users as allow-all and only compares message.sender_id (a normalized
UUID), which lets mismatched binding entries (e.g., phone numbers) slip through;
change the logic so an empty self.dm_allowed_users results in denying the DM
(return false) and, when checking membership, compare using the same normalized
identifier as SignalAdapter produces (or compare both the normalized UUID and
the original raw phone identifier used by bindings) so that
contains(&message.sender_id) actually matches real binding entries; update the
check around self.dm_allowed_users and the membership test that references
message.sender_id to use the normalized id(s).
In `@src/config/watcher.rs`:
- Around line 233-239: The watcher currently creates a fresh ArcSwap for signal
permissions and only gives it to the new adapter, so later reloads don't update
that adapter; change the logic around signal_permissions and
SignalPermissions::from_config so that when you create a new ArcSwap (the
ArcSwap holding SignalPermissions produced by SignalPermissions::from_config)
you also store that ArcSwap into the shared watcher state (the
signal_permissions Option) instead of only passing it to the adapter; ensure the
code path that calls perms.store(Arc::new(new_perms)) always operates on the
ArcSwap instance that is kept in signal_permissions (create-and-insert into
signal_permissions when it was None, and reuse the existing ArcSwap when Some)
so subsequent reloads update all adapters.
In `@src/messaging/signal.rs`:
- Around line 887-897: The current send() branch only treats transport errors as
failures, but reqwest::RequestBuilder::send().await returns Ok(Response) for
HTTP 4xx/5xx so the typing loop keeps retrying; after awaiting
client.post(&rpc_url)...send().await, inspect the Response and treat non-success
statuses as errors (e.g., call response.error_for_status() or check
!response.status().is_success()) and log the error and break the loop similarly
to the Err branch; update the block around
client.post(&rpc_url)...timeout(RPC_REQUEST_TIMEOUT)...json(&body).send().await
to handle and break on non-2xx responses (same behavior as rpc_request()).
- Around line 105-112: redact_url currently leaks embedded credentials by
splitting on '/'; update redact_url to parse the input with a URL parser (e.g.,
Url::parse) and reconstruct the redacted string using the scheme and host (and
port if present) only, explicitly omitting username/password and
path/query/fragment; locate the function redact_url and replace the split/join
logic with parsing and building "scheme://host" (including ":port" when
Url::port_or_known_default indicates a non-default port) to ensure credentials
are never included.
In `@src/messaging/target.rs`:
- Around line 335-374: parse_signal_target_parts currently special-cases
e164/phone forms instead of reusing normalize_signal_target, causing
inconsistent normalization; update parse_signal_target_parts to call
normalize_signal_target for branches that handle phone/e164 or single-phone
inputs (both default adapter and named adapter cases) and only construct
BroadcastTarget when normalize_signal_target returns Some(normalized_target),
using adapter "signal" or format!("signal:{instance}") as before; remove the
duplicated phone/e164 normalization logic so numeric-only inputs accepted by
normalize_signal_target are handled consistently.
- Around line 104-122: When resolving Signal channels in this branch, do not
hard-code the adapter as "signal" by calling parse_delivery_target with
"signal:{signal_target}"; instead extract the adapter name from channel.id (the
first part before the first ':') and combine that adapter with the recipient
from platform_meta["signal_target"] so adapter selection uses the channel.id
while the metadata supplies only the recipient payload; update the logic around
parse_delivery_target and parse_signal_target_parts (and the parts extraction
from channel.id) so that when signal_target exists you call
parse_delivery_target with "<adapter>:<signal_target>" (adapter taken from
channel.id) and otherwise continue to parse parts from channel.id via
parse_signal_target_parts.
In `@src/tools/send_message_to_another_channel.rs`:
- Around line 288-290: The current bare-digit check in
send_message_to_another_channel.rs incorrectly converts plain numeric strings to
a Signal delivery target; change the logic so only explicit Signal markers are
accepted: only call parse_delivery_target for inputs that either start with
"signal:" or start with '+' followed by digits (e.g., "+1234567"); leave purely
numeric trimmed strings untouched so ChannelStore::find_by_name() can resolve
numeric channel IDs. Ensure you still construct the "signal:+{trimmed_digits}"
form when handling a leading '+' case and keep parse_delivery_target usage for
the explicit-signal branch.
- Around line 150-156: The current logic overwrites explicitly resolved Signal
adapters (e.g., "signal:work:+1555") by checking only that self.current_adapter
starts_with("signal"); instead, only inherit the current adapter when the parsed
target was the generic "signal". Change the guard in the block that sets
explicit_target.adapter to ensure explicit_target.adapter == "signal" (and still
optionally verify self.current_adapter.starts_with("signal")) before assigning
self.current_adapter.clone(); reference the symbols explicit_target.adapter and
self.current_adapter to locate and update the condition within the
send_message_to_another_channel logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da324a68-d864-43e9-b033-6679c394fc6b
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lock,!**/*.lockCargo.tomlis excluded by!**/*.toml
📒 Files selected for processing (23)
prompts/en/adapters/signal.md.j2prompts/en/tools/send_message_description.md.j2src/agent/channel.rssrc/agent/channel_history.rssrc/api/bindings.rssrc/config.rssrc/config/load.rssrc/config/permissions.rssrc/config/toml_schema.rssrc/config/types.rssrc/config/watcher.rssrc/conversation/worker_transcript.rssrc/llm/model.rssrc/main.rssrc/messaging.rssrc/messaging/signal.rssrc/messaging/target.rssrc/prompts/engine.rssrc/prompts/text.rssrc/secrets/scrub.rssrc/secrets/store.rssrc/tools.rssrc/tools/send_message_to_another_channel.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- src/tools.rs
- src/llm/model.rs
- src/prompts/text.rs
- src/agent/channel_history.rs
- src/messaging.rs
- src/conversation/worker_transcript.rs
- src/secrets/store.rs
- src/secrets/scrub.rs
- prompts/en/adapters/signal.md.j2
- prompts/en/tools/send_message_description.md.j2
ibhagwan
force-pushed
the
feat/signal-adapter
branch
from
0d2bf8f to
aa8342e
Compare
ibhagwan
added a commit
to ibhagwan/spacebot
that referenced
this pull request
Mar 7, 2026
…eapp#347) Fixes critical issues identified in code review: **Critical:** - conversation/worker_transcript.rs: Restore TranscriptStep::UserText emission to prevent user messages being mislabeled as agent text - config/permissions.rs: Separate DM and group permissions to prevent group wildcards from granting DM access - messaging/signal.rs: Move HTTP status check before empty body check to properly fail on non-2xx responses - messaging/signal.rs: Sanitize attachment filenames to prevent path traversal vulnerabilities - messaging/signal.rs: Add source_uuid fallback for sender resolution in privacy-mode envelopes **Medium:** - agent/channel.rs: Use .ok() instead of let _ = for best-effort error channel sends - config/types.rs: Add SecretField for Signal account to treat phone number as sensitive - prompts/engine.rs: Register adapters/signal template for proper Signal-specific guidance - tools/send_message_to_another_channel.rs: Only overwrite adapter when explicit target has no adapter specified - agent/channel.rs: Add adapter parameter to run_agent_turn, prefer InboundMessage.adapter to preserve named-instance context - api/bindings.rs: Add group_ids and group_allowed_users to UpdateBindingRequest for symmetric updates **Low:** - agent/channel_history.rs: Condition space before sender_context on non-empty value to prevent double spacing All changes pass cargo check.
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
src/agent/channel.rs (1)
2566-2571:⚠️ Potential issue | 🟠 MajorKeep the user-facing LLM failure message generic.
Line 2567 sends the provider/toolchain error text straight to the user. Those strings can include internal details that should stay in logs only. Please return a generic failure message here and keep
%errorin thetracing::error!call.Suggested fix
- let error_msg = format!("I encountered an error: {}", error); + let error_msg = + "Sorry — something went wrong while generating a response. Please try again." + .to_string();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 2566 - 2571, Replace the user-facing error text sent via self.response_tx.send(OutboundResponse::Text(...)).await.ok() with a generic message (e.g., "Sorry, something went wrong.") and do not include the provider/toolchain error string there; instead keep the original error variable in the tracing::error! call so the detailed error remains in logs (ensure tracing::error!(%error, "context...") is present) while the OutboundResponse::Text uses only the generic text.src/config/watcher.rs (1)
244-249:⚠️ Potential issue | 🟠 MajorHot-started Signal adapters still miss future permission reloads.
The reload path only updates the watcher-owned
signal_permissionshandle, but the hot-start path still creates freshArcSwaps for Signal when that handle isNoneand for every named instance. Those adapters keep their own unreachable permission handle forever, so later config reloads never update their DM/group filters.Please keep watcher-owned shared handles for the default adapter and each named runtime key, and reuse/store into those handles instead of allocating ad-hoc
ArcSwaps during hot-start.Also applies to: 261-262, 552-557, 573-585
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/watcher.rs` around lines 244 - 249, The hot-start code path creates new ad-hoc ArcSwap handles for Signal adapters causing those adapters to never see later updates; instead, keep and reuse watcher-owned shared ArcSwap handles for the default adapter and each named runtime key so reloads update all adapters. Locate the hot-start branch that constructs Signal adapters (places creating new ArcSwap/Arc::new for signal permissions) and change it to fetch-or-insert into the existing watcher-owned map/option (signal_permissions and the per-key handles) and store the Arc<SignalPermissions> produced by SignalPermissions::from_config into those shared handles rather than allocating fresh ArcSwap instances per adapter; ensure the same update logic used in the reload path (perms.store(Arc::new(...))) is applied to the reused handles for both default and named adapters.src/messaging/target.rs (2)
341-355:⚠️ Potential issue | 🟠 MajorPreserve the named Signal adapter when parsing tracked channel IDs.
For
signal:{instance}:uuid:{id}andsignal:{instance}:group:{id},split(':')makes the third component"uuid"/"group", so this helper falls back to"signal". Metadata-backed replies from named Signal channels will route through the default adapter.Suggested fix
fn extract_signal_adapter_from_channel_id(channel_id: &str) -> String { let parts: Vec<&str> = channel_id.split(':').collect(); match parts.as_slice() { - // Named adapter: signal:{instance}:{target} - // target must start with "uuid:", "group:", or "+" to confirm it's a real target - ["signal", instance, target_part, ..] - if target_part.starts_with("uuid:") - || target_part.starts_with("group:") - || target_part.starts_with('+') => - { - format!("signal:{instance}") - } + ["signal", instance, "uuid", _] | ["signal", instance, "group", _] => { + format!("signal:{instance}") + } + ["signal", instance, phone] if phone.starts_with('+') => format!("signal:{instance}"), // Default adapter: signal:{target} _ => "signal".to_string(), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/target.rs` around lines 341 - 355, The helper extract_signal_adapter_from_channel_id currently mis-parses named Signal channels like "signal:{instance}:uuid:{id}" because split(':') makes the third element "uuid"/"group" and the match falls back to "signal"; update the pattern matching to detect the two-part target forms by checking for ["signal", instance, "uuid", id, ..] and ["signal", instance, "group", id, ..] (or the equivalent check that parts[2] == "uuid" or "group" and parts.len() > 3) and preserve the named adapter by returning format!("signal:{instance}") for those cases (also keep the existing handling for a third part that starts_with('+') returning format!("signal:{instance}")); leave default behavior returning "signal".
365-413:⚠️ Potential issue | 🟠 MajorMake
parse_signal_target_parts()accept the same Signal forms as the shared normalizer.This helper still rejects bare numeric and UUID-like targets even though
normalize_signal_target()accepts them, so fallback resolution fromchannel.iddepends on which parsing path produced the target.Suggested refactor
pub fn parse_signal_target_parts(parts: &[&str]) -> Option<BroadcastTarget> { - match parts { - // Default adapter: signal:uuid:xxx, signal:group:xxx, signal:e164:+xxx, signal:+xxx - ["uuid", uuid] => Some(BroadcastTarget { - adapter: "signal".to_string(), - target: format!("uuid:{uuid}"), - }), - ["group", group_id] => Some(BroadcastTarget { - adapter: "signal".to_string(), - target: format!("group:{group_id}"), - }), - // Use normalize_signal_target for phone/e164 to ensure consistent parsing - ["e164", phone] => { - normalize_signal_target(&format!("e164:{phone}")).map(|target| BroadcastTarget { - adapter: "signal".to_string(), - target, - }) - } - [phone] if phone.starts_with('+') => { - normalize_signal_target(phone).map(|target| BroadcastTarget { - adapter: "signal".to_string(), - target, - }) - } - // Named adapter: signal:instance:uuid:xxx, signal:instance:group:xxx - [instance, "uuid", uuid] => Some(BroadcastTarget { - adapter: format!("signal:{instance}"), - target: format!("uuid:{uuid}"), - }), - [instance, "group", group_id] => Some(BroadcastTarget { - adapter: format!("signal:{instance}"), - target: format!("group:{group_id}"), - }), - // Named adapter: signal:instance:e164:+xxx - use normalize_signal_target - [instance, "e164", phone] => { - normalize_signal_target(&format!("e164:{phone}")).map(|target| BroadcastTarget { - adapter: format!("signal:{instance}"), - target, - }) - } - // Named adapter: signal:instance:+xxx - use normalize_signal_target - [instance, phone] if phone.starts_with('+') => { - normalize_signal_target(phone).map(|target| BroadcastTarget { - adapter: format!("signal:{instance}"), - target, - }) - } - _ => None, - } + let (adapter, raw_target) = match parts { + ["uuid", uuid] => ("signal".to_string(), format!("uuid:{uuid}")), + ["group", group_id] => ("signal".to_string(), format!("group:{group_id}")), + ["e164", phone] => ("signal".to_string(), format!("e164:{phone}")), + [target] => ("signal".to_string(), (*target).to_string()), + [instance, "uuid", uuid] => (format!("signal:{instance}"), format!("uuid:{uuid}")), + [instance, "group", group_id] => { + (format!("signal:{instance}"), format!("group:{group_id}")) + } + [instance, "e164", phone] => (format!("signal:{instance}"), format!("e164:{phone}")), + [instance, target] => (format!("signal:{instance}"), (*target).to_string()), + _ => return None, + }; + + Some(BroadcastTarget { + adapter, + target: normalize_signal_target(&raw_target)?, + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/target.rs` around lines 365 - 413, parse_signal_target_parts currently only accepts explicit "uuid" and "group" forms and rejects bare numeric or UUID-like tokens that normalize_signal_target accepts; update parse_signal_target_parts to delegate to normalize_signal_target for single-part targets (e.g., a bare phone, numeric, or UUID-like string) and for instance-prefixed single tokens (e.g., ["instance", token]) so that if normalize_signal_target returns Some(target) you construct BroadcastTarget with adapter "signal" or format!("signal:{instance}") and that target; keep the existing explicit ["uuid", uuid] and ["group", group_id] arms unchanged but ensure all other phone/uuid-like cases use normalize_signal_target to produce consistent parsing with normalize_signal_target.src/messaging/signal.rs (1)
573-594:⚠️ Potential issue | 🟠 Major
dm_allowed_userscurrently weakens group-specific sender restrictions.If
dm_allowed_userscontains"*"or a broad allowlist, this merged check admits senders thatgroup_allowed_userswas supposed to exclude. In practice, a permissive DM config can nullify group-specific restrictions.♻️ Minimal fix
- let all_group_users: Vec<&String> = permissions - .dm_allowed_users - .iter() - .chain(permissions.group_allowed_users.iter()) - .collect(); - - let sender_allowed = if all_group_users.is_empty() { + let allowed_users = if permissions.group_allowed_users.is_empty() { + &permissions.dm_allowed_users + } else { + &permissions.group_allowed_users + }; + + let sender_allowed = if allowed_users.is_empty() { false // Empty = block all - } else if all_group_users.iter().any(|u| u.as_str() == "*") { + } else if allowed_users.iter().any(|u| u.as_str() == "*") { true // Wildcard = allow all } else { - all_group_users.iter().any(|allowed| { + allowed_users.iter().any(|allowed| { allowed.as_str() == sender || envelope .source_uuid🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/signal.rs` around lines 573 - 594, The merged check over dm_allowed_users and group_allowed_users lets a permissive DM list override stricter group rules; change the logic so group_allowed_users takes precedence for group messages: compute sender_allowed by first evaluating group_allowed_users (empty => block all, ["*"] => allow all, otherwise check sender or envelope.source_uuid), and only if group_allowed_users is empty fall back to the dm_allowed_users rules; update the code around all_group_users/sender_allowed (and references to permissions.dm_allowed_users, permissions.group_allowed_users, and envelope.source_uuid) to implement this 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 `@src/config/toml_schema.rs`:
- Around line 494-495: The TOML schema added Signal but the binding-level Signal
group filters aren't deserialized or propagated: update TomlBinding to include
Option<Vec<...>> group_ids and Option<Vec<...>> group_allowed_users (matching
the types used by TomlSignalConfig), modify the deserialization mapping so
TomlBinding can parse those fields, and then in Config::from_toml (the code that
constructs Binding instances) populate Binding { group_ids: b.group_ids,
group_allowed_users: b.group_allowed_users, ... } so the Binding struct receives
the parsed values; ensure types and nullability match Binding's existing
group_ids/group_allowed_users fields and any schema validation logic is
preserved.
In `@src/messaging/signal.rs`:
- Around line 1142-1189: The UTF-8 error handling currently treats the entire
suffix after a detected invalid byte as utf8_carry and defers re-decoding, which
can drop valid SSE data; change the logic around
std::str::from_utf8(&decode_buf) so that when error.error_len() returns
Some(bad_len) you skip exactly the bad byte(s) (increment an index by
valid_up_to + bad_len), append the preceding valid slice
(decode_buf[..valid_up_to]) to buffer (respecting MAX_SSE_BUFFER_SIZE and
current_data), and then continue decoding the remaining bytes in the same chunk
(re-run the UTF-8 parse/handling on decode_buf[index..]) instead of setting
carry_start to include the whole suffix; only set utf8_carry from a trailing
incomplete sequence (error_len() == None) and preserve the existing buffer
overflow/reset behavior.
- Around line 1077-1094: The unconditional tokio::time::sleep(retry_delay).await
calls make shutdown/reload stall; replace those sleeps (the two shown after the
response error and Err(error) and the similar sleep after stream end at lines
~1231-1233) with a cancelable wait using tokio::select! that waits on either
tokio::time::sleep(retry_delay) or the module's shutdown signal (e.g., a
CancellationToken/CancellationFuture or notified() on the shutdown receiver used
elsewhere in this module); if shutdown fires, break/return immediately,
otherwise proceed to double retry_delay = (retry_delay * 2).min(SSE_MAX_BACKOFF)
and continue the reconnect loop.
In `@src/tools/send_message_to_another_channel.rs`:
- Around line 151-184: The code currently skips broadcasting when
parse_explicit_signal_target(...) returns an explicit_target with adapter ==
"signal", causing valid explicit Signal forms to fall through and fail; change
the branch so that when parse_explicit_signal_target returns
Some(explicit_target) you treat explicit Signal targets as immediate explicit
targets if the explicit_target.target is a Signal-form identifier (e.g., starts
with "uuid:", "group:", or "+"): call self.messaging_manager.broadcast(...) and
return the SendMessageOutput (same as the non-signal branch), redact the
broadcast_target for adapters that start with "signal", and only fall through to
use current_adapter when you intentionally want to prefer a named non-default
adapter; reference parse_explicit_signal_target, explicit_target.adapter,
explicit_target.target, self.messaging_manager.broadcast, and SendMessageOutput
to locate and modify the logic.
---
Duplicate comments:
In `@src/agent/channel.rs`:
- Around line 2566-2571: Replace the user-facing error text sent via
self.response_tx.send(OutboundResponse::Text(...)).await.ok() with a generic
message (e.g., "Sorry, something went wrong.") and do not include the
provider/toolchain error string there; instead keep the original error variable
in the tracing::error! call so the detailed error remains in logs (ensure
tracing::error!(%error, "context...") is present) while the
OutboundResponse::Text uses only the generic text.
In `@src/config/watcher.rs`:
- Around line 244-249: The hot-start code path creates new ad-hoc ArcSwap
handles for Signal adapters causing those adapters to never see later updates;
instead, keep and reuse watcher-owned shared ArcSwap handles for the default
adapter and each named runtime key so reloads update all adapters. Locate the
hot-start branch that constructs Signal adapters (places creating new
ArcSwap/Arc::new for signal permissions) and change it to fetch-or-insert into
the existing watcher-owned map/option (signal_permissions and the per-key
handles) and store the Arc<SignalPermissions> produced by
SignalPermissions::from_config into those shared handles rather than allocating
fresh ArcSwap instances per adapter; ensure the same update logic used in the
reload path (perms.store(Arc::new(...))) is applied to the reused handles for
both default and named adapters.
In `@src/messaging/signal.rs`:
- Around line 573-594: The merged check over dm_allowed_users and
group_allowed_users lets a permissive DM list override stricter group rules;
change the logic so group_allowed_users takes precedence for group messages:
compute sender_allowed by first evaluating group_allowed_users (empty => block
all, ["*"] => allow all, otherwise check sender or envelope.source_uuid), and
only if group_allowed_users is empty fall back to the dm_allowed_users rules;
update the code around all_group_users/sender_allowed (and references to
permissions.dm_allowed_users, permissions.group_allowed_users, and
envelope.source_uuid) to implement this precedence.
In `@src/messaging/target.rs`:
- Around line 341-355: The helper extract_signal_adapter_from_channel_id
currently mis-parses named Signal channels like "signal:{instance}:uuid:{id}"
because split(':') makes the third element "uuid"/"group" and the match falls
back to "signal"; update the pattern matching to detect the two-part target
forms by checking for ["signal", instance, "uuid", id, ..] and ["signal",
instance, "group", id, ..] (or the equivalent check that parts[2] == "uuid" or
"group" and parts.len() > 3) and preserve the named adapter by returning
format!("signal:{instance}") for those cases (also keep the existing handling
for a third part that starts_with('+') returning format!("signal:{instance}"));
leave default behavior returning "signal".
- Around line 365-413: parse_signal_target_parts currently only accepts explicit
"uuid" and "group" forms and rejects bare numeric or UUID-like tokens that
normalize_signal_target accepts; update parse_signal_target_parts to delegate to
normalize_signal_target for single-part targets (e.g., a bare phone, numeric, or
UUID-like string) and for instance-prefixed single tokens (e.g., ["instance",
token]) so that if normalize_signal_target returns Some(target) you construct
BroadcastTarget with adapter "signal" or format!("signal:{instance}") and that
target; keep the existing explicit ["uuid", uuid] and ["group", group_id] arms
unchanged but ensure all other phone/uuid-like cases use normalize_signal_target
to produce consistent parsing with normalize_signal_target.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eddb8954-3793-4692-8b3f-451f53b2df08
📒 Files selected for processing (21)
prompts/en/adapters/signal.md.j2prompts/en/tools/send_message_description.md.j2src/agent/channel.rssrc/agent/channel_history.rssrc/config.rssrc/config/load.rssrc/config/permissions.rssrc/config/toml_schema.rssrc/config/types.rssrc/config/watcher.rssrc/main.rssrc/messaging.rssrc/messaging/signal.rssrc/messaging/target.rssrc/prompts/engine.rssrc/prompts/text.rssrc/secrets/scrub.rssrc/secrets/store.rssrc/tools.rssrc/tools/send_message_to_another_channel.rstests/context_dump.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- src/agent/channel_history.rs
- src/prompts/engine.rs
- src/secrets/scrub.rs
- src/secrets/store.rs
- tests/context_dump.rs
- src/prompts/text.rs
Contributor
|
✅ Actions performedFull review triggered. |
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/config.rs (1)
523-532:⚠️ Potential issue | 🟡 MinorUse the helper’s
dm_allowed_usersargument.This helper always returns an empty config allowlist, so the merge/dedup tests below never exercise config-provided Slack DM users and the expectations with
U001/U002become wrong.Suggested fix
fn slack_config_with_dm_users(dm_allowed_users: Vec<String>) -> SlackConfig { SlackConfig { enabled: true, bot_token: "xoxb-test".into(), app_token: "xapp-test".into(), instances: vec![], - dm_allowed_users: vec![], + dm_allowed_users, commands: vec![], } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.rs` around lines 523 - 532, The helper slack_config_with_dm_users currently ignores its dm_allowed_users parameter and always sets SlackConfig.dm_allowed_users to an empty vec; update the function to assign the passed-in dm_allowed_users parameter to the SlackConfig field (i.e., set dm_allowed_users: dm_allowed_users) so tests exercise config-provided Slack DM users when constructing SlackConfig in slack_config_with_dm_users.
♻️ Duplicate comments (6)
src/messaging/target.rs (1)
341-356:⚠️ Potential issue | 🟠 MajorNamed Signal adapter extraction still broken for UUID/group channels.
For a channel ID like
"signal:myinstance:uuid:abc-123-def", splitting on:yields["signal", "myinstance", "uuid", "abc-123-def"]. The guard condition checks iftarget_part.starts_with("uuid:"), buttarget_parthere is"uuid"(a literal segment), not"uuid:...". This means UUID and group channels for named adapters fall through to the default"signal"adapter instead of returning"signal:myinstance".🛠️ Suggested fix
fn extract_signal_adapter_from_channel_id(channel_id: &str) -> String { let parts: Vec<&str> = channel_id.split(':').collect(); match parts.as_slice() { - // Named adapter: signal:{instance}:{target} - // target must start with "uuid:", "group:", or "+" to confirm it's a real target - ["signal", instance, target_part, ..] - if target_part.starts_with("uuid:") - || target_part.starts_with("group:") - || target_part.starts_with('+') => - { + // Named adapter: signal:{instance}:uuid:{uuid} or signal:{instance}:group:{id} + ["signal", instance, "uuid", ..] | ["signal", instance, "group", ..] => { format!("signal:{instance}") } + // Named adapter: signal:{instance}:+{phone} + ["signal", instance, phone, ..] if phone.starts_with('+') => { + format!("signal:{instance}") + } // Default adapter: signal:{target} _ => "signal".to_string(), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/target.rs` around lines 341 - 356, The function extract_signal_adapter_from_channel_id is mis-detecting named adapters because splitting on ':' breaks targets like "uuid:abc-123" into separate segments; change the logic in extract_signal_adapter_from_channel_id to reconstruct the target portion (join parts from index 2 onward into a single target_part string) or use a splitn that preserves the remainder, then test target_part.starts_with("uuid:"), "group:", or '+' and return format!("signal:{instance}") when matched, otherwise fall back to "signal".src/config/load.rs (1)
2150-2166:⚠️ Potential issue | 🟠 MajorGate the default Signal adapter on top-level credentials.
The early return fixes the “nothing runnable” case, but when only named instances are valid this still builds
SignalConfig { enabled: true, http_url: "", account: "" }. That keeps a phantom default adapter alive and can make startup fail even though the named instances are fine.Suggested fix
let http_url = std::env::var("SIGNAL_HTTP_URL") .ok() .or_else(|| s.http_url.as_deref().and_then(resolve_env_value)); let account = std::env::var("SIGNAL_ACCOUNT") .ok() .or_else(|| s.account.as_deref().and_then(resolve_env_value)); + let has_default_credentials = http_url.is_some() && account.is_some(); - if (http_url.is_none() || account.is_none()) - && !instances.iter().any(|inst| inst.enabled) - { + if !has_default_credentials + && !instances.iter().any(|instance| instance.enabled) + { return None; } Some(SignalConfig { - enabled: s.enabled, + enabled: s.enabled && has_default_credentials, http_url: http_url.unwrap_or_default(), account: account.unwrap_or_default(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/load.rs` around lines 2150 - 2166, The code currently constructs a default SignalConfig with empty strings when top-level credentials are missing (http_url/account), leaving a phantom adapter alive; update the gating logic so you only build SignalConfig when both http_url and account are present: change the conditional around SignalConfig creation to return None whenever http_url.is_none() || account.is_none() (remove the instances-enabled check), ensuring you don’t produce a SignalConfig with empty values; keep using http_url.unwrap() and account.unwrap() (or unwrap_or_default after the presence check) so the default adapter is only created when top-level creds exist (refer to http_url, account, instances, SignalConfig, resolve_env_value, and s.enabled).src/agent/channel.rs (1)
2566-2571:⚠️ Potential issue | 🟠 MajorDon't send raw
PromptErrortext back to the user.
PromptErrorformatting can include upstream/provider details. Keep the chat message generic and leave%errorin logs.Minimal fix
- let error_msg = format!("I encountered an error: {}", error); + let error_msg = + "Sorry — something went wrong while generating a response. Please try again." + .to_string();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 2566 - 2571, The handler currently formats and sends the raw PromptError to the user (via error_msg and self.response_tx.send(OutboundResponse::Text(...))), which may leak provider details; instead send a generic user-facing message like "I encountered an internal error; please try again later." through OutboundResponse::Text, and log the original PromptError (preserving `%error`) to your logger (e.g., process or module logger) without exposing it to the response path; update the branch around self.response_tx.send and the PromptError handling to separate user message vs internal log while keeping PromptError only in logs.src/tools/send_message_to_another_channel.rs (1)
150-186:⚠️ Potential issue | 🟠 MajorKeep unscoped Signal targets on the active named adapter.
In a
signal:workturn,signal:+...,signal:uuid:..., bare UUIDs, and bare+targets still broadcast through the defaultsignaladapter because this branch never upgradesexplicit_target.adapter == "signal"toself.current_adapter. That regresses named-instance routing the channel now threads into the tool.Suggested fix
- if let Some(explicit_target) = parse_explicit_signal_target(&args.target) { + if let Some(mut explicit_target) = parse_explicit_signal_target(&args.target) { + if explicit_target.adapter == "signal" + && let Some(current_adapter) = self + .current_adapter + .as_ref() + .filter(|adapter| adapter.starts_with("signal")) + { + explicit_target.adapter = current_adapter.clone(); + } + // Check if this is an explicit Signal target form let is_explicit_signal_target = explicit_target.target.starts_with("uuid:") || explicit_target.target.starts_with("group:") || explicit_target.target.starts_with("e164:") || explicit_target.target.starts_with('+');🤖 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 150 - 186, The branch treating explicit Signal targets never upgrades explicit_target.adapter == "signal" to the active named adapter, so explicit signal forms in a named adapter turn into the generic "signal" adapter; fix by computing an effective_adapter (use self.current_adapter when explicit_target.adapter == "signal" and is_explicit_signal_target is true, otherwise keep explicit_target.adapter) and use that effective_adapter for messaging_manager.broadcast, the redaction check, tracing::info, and the returned SendMessageOutput.platform and target as appropriate (keep returned target as explicit_target.target but platform should be effective_adapter).src/messaging/signal.rs (2)
573-594:⚠️ Potential issue | 🟠 MajorDon't let DM allowlists bypass group sender restrictions.
This still unions
dm_allowed_userswithgroup_allowed_usersfor group messages, sodm_allowed_users = ["*"]makes every sender in an allowed group pass even whengroup_allowed_usersis intended to narrow group access.SignalPermissionstreats those lists as DM-only vs. group-only; for groups,group_allowed_usersshould be authoritative when configured, withdm_allowed_usersonly as a fallback if the group list is empty.Suggested fix
- let all_group_users: Vec<&String> = permissions - .dm_allowed_users - .iter() - .chain(permissions.group_allowed_users.iter()) - .collect(); - - let sender_allowed = if all_group_users.is_empty() { + let allowed_group_users = if permissions.group_allowed_users.is_empty() { + &permissions.dm_allowed_users + } else { + &permissions.group_allowed_users + }; + + let sender_allowed = if allowed_group_users.is_empty() { false // Empty = block all - } else if all_group_users.iter().any(|u| u.as_str() == "*") { + } else if allowed_group_users.iter().any(|u| u.as_str() == "*") { true // Wildcard = allow all } else { - all_group_users.iter().any(|allowed| { + allowed_group_users.iter().any(|allowed| { allowed.as_str() == sender || envelope .source_uuid🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/signal.rs` around lines 573 - 594, The current logic combines permissions.dm_allowed_users and permissions.group_allowed_users into all_group_users for group messages, allowing DM allowlists to override group restrictions; change the computation of sender_allowed in the group path to prefer permissions.group_allowed_users when it is non-empty and only fall back to permissions.dm_allowed_users if group_allowed_users is empty: implement this by checking if permissions.group_allowed_users.is_empty() ? use dm_allowed_users : use group_allowed_users, then apply the existing empty/wildcard/specific checks (matching against sender and envelope.source_uuid) to that chosen list; update references in the block that sets sender_allowed and keep the same semantics for "*" and [] (empty = block all).
1152-1201:⚠️ Potential issue | 🟠 MajorContinue decoding after invalid UTF-8 instead of deferring the whole suffix.
The
Some(bad_len)path still stores everything after the invalid sequence inutf8_carry. That means valid bytes later in the same chunk are only retried if another chunk arrives, and they are dropped if the stream ends or reconnects first. To preserve SSE data, skip the bad byte(s) and keep decoding the remaining suffix in the current chunk;utf8_carryshould only hold a trailing incomplete codepoint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/signal.rs` around lines 1152 - 1201, The current UTF-8 handling in the SSE decoder incorrectly defers the entire suffix after a detected invalid sequence to utf8_carry; instead, when std::str::from_utf8() returns Err with Some(bad_len) you should skip the bad bytes and continue decoding the remainder of decode_buf in the same chunk rather than moving it all to utf8_carry. Update the logic around decode_buf, valid_len, carry_start and utf8_carry so that only a trailing incomplete multi-byte sequence (when error.error_len() is None) is preserved in utf8_carry, while the bytes after a skipped invalid sequence are re-decoded/processed immediately into buffer (respecting MAX_SSE_BUFFER_SIZE and current_data), e.g., by looping over decode_buf advancing by valid_up_to + bad_len until the whole slice is handled or an incomplete suffix remains; keep references to utf8_carry, buffer, decode_buf, carry_start, and MAX_SSE_BUFFER_SIZE to locate where to change the code.
🧹 Nitpick comments (1)
src/tools/send_message_to_another_channel.rs (1)
89-118: Advertise Signal targeting based on configured availability, not the current turn.
call()accepts explicit Signal sends even from non-Signal conversations, butdefinition()only documents them whencurrent_adapteralready starts withsignal. That makes a working capability invisible to the LLM on Discord/Slack/etc.🤖 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 89 - 118, The definition() currently advertises Signal targets only when the current_adapter starts with "signal"; change it to detect configured Signal capability like email does by using self.messaging_manager.has_adapter("signal"). Replace the signal_adapter_available computation that references self.current_adapter with a check against self.messaging_manager.has_adapter("signal"). Keep the rest of the description/target_description appends the same so definition() documents Signal targeting whenever the messaging manager reports a Signal adapter is available (matching how email_adapter_available is derived), while call() can continue to accept explicit Signal sends.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/config.rs`:
- Around line 523-532: The helper slack_config_with_dm_users currently ignores
its dm_allowed_users parameter and always sets SlackConfig.dm_allowed_users to
an empty vec; update the function to assign the passed-in dm_allowed_users
parameter to the SlackConfig field (i.e., set dm_allowed_users:
dm_allowed_users) so tests exercise config-provided Slack DM users when
constructing SlackConfig in slack_config_with_dm_users.
---
Duplicate comments:
In `@src/agent/channel.rs`:
- Around line 2566-2571: The handler currently formats and sends the raw
PromptError to the user (via error_msg and
self.response_tx.send(OutboundResponse::Text(...))), which may leak provider
details; instead send a generic user-facing message like "I encountered an
internal error; please try again later." through OutboundResponse::Text, and log
the original PromptError (preserving `%error`) to your logger (e.g., process or
module logger) without exposing it to the response path; update the branch
around self.response_tx.send and the PromptError handling to separate user
message vs internal log while keeping PromptError only in logs.
In `@src/config/load.rs`:
- Around line 2150-2166: The code currently constructs a default SignalConfig
with empty strings when top-level credentials are missing (http_url/account),
leaving a phantom adapter alive; update the gating logic so you only build
SignalConfig when both http_url and account are present: change the conditional
around SignalConfig creation to return None whenever http_url.is_none() ||
account.is_none() (remove the instances-enabled check), ensuring you don’t
produce a SignalConfig with empty values; keep using http_url.unwrap() and
account.unwrap() (or unwrap_or_default after the presence check) so the default
adapter is only created when top-level creds exist (refer to http_url, account,
instances, SignalConfig, resolve_env_value, and s.enabled).
In `@src/messaging/signal.rs`:
- Around line 573-594: The current logic combines permissions.dm_allowed_users
and permissions.group_allowed_users into all_group_users for group messages,
allowing DM allowlists to override group restrictions; change the computation of
sender_allowed in the group path to prefer permissions.group_allowed_users when
it is non-empty and only fall back to permissions.dm_allowed_users if
group_allowed_users is empty: implement this by checking if
permissions.group_allowed_users.is_empty() ? use dm_allowed_users : use
group_allowed_users, then apply the existing empty/wildcard/specific checks
(matching against sender and envelope.source_uuid) to that chosen list; update
references in the block that sets sender_allowed and keep the same semantics for
"*" and [] (empty = block all).
- Around line 1152-1201: The current UTF-8 handling in the SSE decoder
incorrectly defers the entire suffix after a detected invalid sequence to
utf8_carry; instead, when std::str::from_utf8() returns Err with Some(bad_len)
you should skip the bad bytes and continue decoding the remainder of decode_buf
in the same chunk rather than moving it all to utf8_carry. Update the logic
around decode_buf, valid_len, carry_start and utf8_carry so that only a trailing
incomplete multi-byte sequence (when error.error_len() is None) is preserved in
utf8_carry, while the bytes after a skipped invalid sequence are
re-decoded/processed immediately into buffer (respecting MAX_SSE_BUFFER_SIZE and
current_data), e.g., by looping over decode_buf advancing by valid_up_to +
bad_len until the whole slice is handled or an incomplete suffix remains; keep
references to utf8_carry, buffer, decode_buf, carry_start, and
MAX_SSE_BUFFER_SIZE to locate where to change the code.
In `@src/messaging/target.rs`:
- Around line 341-356: The function extract_signal_adapter_from_channel_id is
mis-detecting named adapters because splitting on ':' breaks targets like
"uuid:abc-123" into separate segments; change the logic in
extract_signal_adapter_from_channel_id to reconstruct the target portion (join
parts from index 2 onward into a single target_part string) or use a splitn that
preserves the remainder, then test target_part.starts_with("uuid:"), "group:",
or '+' and return format!("signal:{instance}") when matched, otherwise fall back
to "signal".
In `@src/tools/send_message_to_another_channel.rs`:
- Around line 150-186: The branch treating explicit Signal targets never
upgrades explicit_target.adapter == "signal" to the active named adapter, so
explicit signal forms in a named adapter turn into the generic "signal" adapter;
fix by computing an effective_adapter (use self.current_adapter when
explicit_target.adapter == "signal" and is_explicit_signal_target is true,
otherwise keep explicit_target.adapter) and use that effective_adapter for
messaging_manager.broadcast, the redaction check, tracing::info, and the
returned SendMessageOutput.platform and target as appropriate (keep returned
target as explicit_target.target but platform should be effective_adapter).
---
Nitpick comments:
In `@src/tools/send_message_to_another_channel.rs`:
- Around line 89-118: The definition() currently advertises Signal targets only
when the current_adapter starts with "signal"; change it to detect configured
Signal capability like email does by using
self.messaging_manager.has_adapter("signal"). Replace the
signal_adapter_available computation that references self.current_adapter with a
check against self.messaging_manager.has_adapter("signal"). Keep the rest of the
description/target_description appends the same so definition() documents Signal
targeting whenever the messaging manager reports a Signal adapter is available
(matching how email_adapter_available is derived), while call() can continue to
accept explicit Signal sends.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8dfa7b47-e02d-41f3-b125-c03e09c535b2
📒 Files selected for processing (21)
prompts/en/adapters/signal.md.j2prompts/en/tools/send_message_description.md.j2src/agent/channel.rssrc/agent/channel_history.rssrc/config.rssrc/config/load.rssrc/config/permissions.rssrc/config/toml_schema.rssrc/config/types.rssrc/config/watcher.rssrc/main.rssrc/messaging.rssrc/messaging/signal.rssrc/messaging/target.rssrc/prompts/engine.rssrc/prompts/text.rssrc/secrets/scrub.rssrc/secrets/store.rssrc/tools.rssrc/tools/send_message_to_another_channel.rstests/context_dump.rs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/messaging/target.rs (1)
341-356:⚠️ Potential issue | 🟠 MajorNamed Signal adapter extraction fails for UUID and group channel IDs.
When channel ID is
signal:work:uuid:abc-123, splitting by:yields["signal", "work", "uuid", "abc-123"]. The condition checkstarget_part.starts_with("uuid:")wheretarget_part = "uuid", but"uuid"does not start with"uuid:"(missing colon after split). This causes named adapters with UUID or group targets to fall through to the default"signal"adapter, breaking routing for named Signal instances.Phone numbers with
+work only because the check isstarts_with('+')which matches the actual part value.🐛 Proposed fix
fn extract_signal_adapter_from_channel_id(channel_id: &str) -> String { let parts: Vec<&str> = channel_id.split(':').collect(); match parts.as_slice() { - // Named adapter: signal:{instance}:{target} - // target must start with "uuid:", "group:", or "+" to confirm it's a real target - ["signal", instance, target_part, ..] - if target_part.starts_with("uuid:") - || target_part.starts_with("group:") - || target_part.starts_with('+') => - { - format!("signal:{instance}") - } + // Named adapter: signal:{instance}:uuid:{uuid} or signal:{instance}:group:{id} + ["signal", instance, "uuid", _] | ["signal", instance, "group", _] => { + format!("signal:{instance}") + } + // Named adapter: signal:{instance}:+{phone} + ["signal", instance, phone] if phone.starts_with('+') => { + format!("signal:{instance}") + } // Default adapter: signal:{target} _ => "signal".to_string(), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/target.rs` around lines 341 - 356, The function extract_signal_adapter_from_channel_id mis-identifies named adapters because splitting on ':' yields "uuid" and "group" without their following colon-prefixed data; update the match guard in extract_signal_adapter_from_channel_id to treat target_part == "uuid" or target_part == "group" (in addition to starts_with("uuid:")/starts_with("group:") and starts_with('+')) as valid named targets, so that inputs like "signal:work:uuid:abc-123" or "signal:team:group:xyz" return format!("signal:{instance}") using the instance variable.
🧹 Nitpick comments (2)
src/messaging/signal.rs (1)
1154-1202: UTF-8 recovery could process more data in the same chunk, but current behavior is acceptable.When encountering an invalid UTF-8 byte, the code carries remaining bytes to the next chunk rather than continuing to decode them immediately. This could delay processing of valid SSE data following an invalid byte until the next chunk arrives. However, since invalid UTF-8 in SSE streams is rare and the data is preserved (not dropped), this is a minor optimization opportunity rather than a correctness issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/signal.rs` around lines 1154 - 1202, The code currently defers decoding bytes after an invalid UTF-8 sequence to the next chunk; update the invalid-byte branch so that when error.error_len() returns Some(bad_len) you skip the bad bytes (as now) but then immediately attempt to decode the remainder of decode_buf in the same iteration (recomputing valid_len/carry_start for the remainder and appending any newly-valid text to buffer), taking care to reuse the same symbols (decode_buf, valid_len, carry_start, utf8_carry, buffer, MAX_SSE_BUFFER_SIZE) and preserve existing buffer overflow protection and carry logic so trailing incomplete sequences are still saved to utf8_carry.src/tools/send_message_to_another_channel.rs (1)
344-346: Dead code: redundantsignal:check.This branch is unreachable. If the input starts with
signal:, line 318 already processes it viaparse_signal_target_parts. If that returnsNone(invalid format), callingparse_delivery_targethere would also returnNone. This is harmless but could be removed for clarity.♻️ Suggested removal
- // Bare phone numbers (7+ digits) require explicit Signal indicator - // to avoid treating numeric channel IDs as Signal numbers. - // Only parse as Signal if input explicitly starts with "signal:" - if trimmed.starts_with("signal:") { - return crate::messaging::target::parse_delivery_target(trimmed); - } - // Group ID format: group:xxx (might be passed directly)🤖 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 344 - 346, The branch that re-checks if trimmed.starts_with("signal:") and then calls crate::messaging::target::parse_delivery_target is redundant and unreachable because parse_signal_target_parts already handles inputs starting with "signal:"; remove this conditional (the if trimmed.starts_with("signal:") { return crate::messaging::target::parse_delivery_target(trimmed); } block) so only parse_signal_target_parts is relied on for "signal:" inputs, leaving parse_delivery_target for non-signal inputs.
🤖 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/messaging/target.rs`:
- Around line 341-356: The function extract_signal_adapter_from_channel_id
mis-identifies named adapters because splitting on ':' yields "uuid" and "group"
without their following colon-prefixed data; update the match guard in
extract_signal_adapter_from_channel_id to treat target_part == "uuid" or
target_part == "group" (in addition to
starts_with("uuid:")/starts_with("group:") and starts_with('+')) as valid named
targets, so that inputs like "signal:work:uuid:abc-123" or
"signal:team:group:xyz" return format!("signal:{instance}") using the instance
variable.
---
Nitpick comments:
In `@src/messaging/signal.rs`:
- Around line 1154-1202: The code currently defers decoding bytes after an
invalid UTF-8 sequence to the next chunk; update the invalid-byte branch so that
when error.error_len() returns Some(bad_len) you skip the bad bytes (as now) but
then immediately attempt to decode the remainder of decode_buf in the same
iteration (recomputing valid_len/carry_start for the remainder and appending any
newly-valid text to buffer), taking care to reuse the same symbols (decode_buf,
valid_len, carry_start, utf8_carry, buffer, MAX_SSE_BUFFER_SIZE) and preserve
existing buffer overflow protection and carry logic so trailing incomplete
sequences are still saved to utf8_carry.
In `@src/tools/send_message_to_another_channel.rs`:
- Around line 344-346: The branch that re-checks if
trimmed.starts_with("signal:") and then calls
crate::messaging::target::parse_delivery_target is redundant and unreachable
because parse_signal_target_parts already handles inputs starting with
"signal:"; remove this conditional (the if trimmed.starts_with("signal:") {
return crate::messaging::target::parse_delivery_target(trimmed); } block) so
only parse_signal_target_parts is relied on for "signal:" inputs, leaving
parse_delivery_target for non-signal inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 973fad31-8bb9-452e-beb8-05569d63f019
📒 Files selected for processing (4)
src/config.rssrc/messaging/signal.rssrc/messaging/target.rssrc/tools/send_message_to_another_channel.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/config.rs
ibhagwan
force-pushed
the
feat/signal-adapter
branch
from
55c1741 to
130f858
Compare
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
ibhagwan
force-pushed
the
feat/signal-adapter
branch
2 times, most recently
from
a363fad to
8dc6b44
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/config.rs (1)
523-532:⚠️ Potential issue | 🟠 MajorPass the helper input through to
SlackConfig.
slack_config_with_dm_users()ignores itsdm_allowed_usersargument and always returns an empty list. The merge/dedup tests below then stop exercising config-level DM users, and several assertions become wrong.🛠️ Minimal fix
fn slack_config_with_dm_users(dm_allowed_users: Vec<String>) -> SlackConfig { SlackConfig { enabled: true, bot_token: "xoxb-test".into(), app_token: "xapp-test".into(), instances: vec![], - dm_allowed_users: vec![], + dm_allowed_users, commands: vec![], } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.rs` around lines 523 - 532, The helper slack_config_with_dm_users currently ignores its dm_allowed_users parameter and sets SlackConfig.dm_allowed_users to an empty vec; update the SlackConfig construction in slack_config_with_dm_users to assign the passed-in dm_allowed_users (possibly cloned or moved as appropriate) to the dm_allowed_users field so tests exercise config-level DM users (ensure the function signature and ownership match how SlackConfig is constructed).
♻️ Duplicate comments (4)
src/messaging/target.rs (1)
104-118:⚠️ Potential issue | 🟠 MajorNormalize metadata-backed Signal targets before returning them.
signal_targetis returned verbatim here, but tracked Signal DMs can store a bare UUID in metadata.SignalAdapter::broadcast()only acceptsuuid:...,group:..., or+..., so channel-based sends for privacy-mode DMs will fail on this path. Runsignal_targetthrough the shared normalizer before constructing theBroadcastTarget.🛠️ Minimal fix
"signal" => { // Signal channels store target in signal_target metadata if let Some(signal_target) = channel .platform_meta .as_ref() .and_then(|meta| meta.get("signal_target")) .and_then(json_value_to_string) { - // signal_target is already normalized (e.g., "uuid:xxxx", "group:xxxx", "+123...") // Determine adapter from channel.id: named if format is "signal:{name}:..." let adapter = extract_signal_adapter_from_channel_id(&channel.id); + let target = normalize_signal_target(&signal_target)?; return Some(BroadcastTarget { adapter, - target: signal_target, + target, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/target.rs` around lines 104 - 118, The metadata-backed Signal target (signal_target) is returned verbatim and must be normalized before constructing the BroadcastTarget; update the "signal" arm in the function that builds BroadcastTarget to pass signal_target through the shared Signal target normalizer (the same normalization used elsewhere before calling SignalAdapter::broadcast) prior to creating BroadcastTarget, keeping the adapter extraction via extract_signal_adapter_from_channel_id(&channel.id) and the existing json_value_to_string logic intact so tracked DMs that store bare UUIDs become valid "uuid:...", "group:..." or "+..." targets.src/messaging/signal.rs (2)
573-594:⚠️ Potential issue | 🟠 Major
dm_allowed_users = ["*"]currently bypassesgroup_allowed_users.This merge makes the group-specific sender allowlist ineffective: as soon as
dm_allowed_userscontains"*", every sender in an allowed group passes, even whengroup_allowed_usersis configured to restrict that group. Treatgroup_allowed_usersas authoritative when it is non-empty, and only fall back todm_allowed_userswhen no group-specific sender list is configured.🛠️ Minimal fix
- let all_group_users: Vec<&String> = permissions - .dm_allowed_users - .iter() - .chain(permissions.group_allowed_users.iter()) - .collect(); - - let sender_allowed = if all_group_users.is_empty() { + let allowed_users = if permissions.group_allowed_users.is_empty() { + &permissions.dm_allowed_users + } else { + &permissions.group_allowed_users + }; + + let sender_allowed = if allowed_users.is_empty() { false // Empty = block all - } else if all_group_users.iter().any(|u| u.as_str() == "*") { + } else if allowed_users.iter().any(|user| user.as_str() == "*") { true // Wildcard = allow all } else { - all_group_users.iter().any(|allowed| { + allowed_users.iter().any(|allowed| { allowed.as_str() == sender || envelope .source_uuid🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/signal.rs` around lines 573 - 594, The current merge of dm_allowed_users and group_allowed_users lets a wildcard in dm_allowed_users override group restrictions; change the logic so group_allowed_users is authoritative when non-empty: first build a list from permissions.group_allowed_users and, if that list is non-empty, compute sender_allowed using only that list (apply empty => block all, ["*"] => allow all, otherwise match allowed == sender or allowed == envelope.source_uuid); only if group_allowed_users is empty fall back to evaluating permissions.dm_allowed_users with the same empty/wildcard/match rules (use the same matching including envelope.source_uuid).
1154-1202:⚠️ Potential issue | 🟠 MajorKeep decoding after invalid UTF-8 bytes instead of parking the whole suffix.
When
from_utf8()reportserror_len() = Some(_),decode_buf[carry_start..]can already contain valid UTF-8 and complete SSE lines after the bad byte(s). Stashing that whole suffix inutf8_carrymeans it is only retried if another chunk arrives; if the stream ends or reconnects first, that valid event data is lost. Continue decoding the remainder in the same chunk and reserveutf8_carryonly for a trailing incomplete sequence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/signal.rs` around lines 1154 - 1202, The current logic stores the entire suffix after a detected invalid UTF-8 byte into utf8_carry, losing any valid UTF-8 that follows; instead, change the decoding to iterate through decode_buf: when std::str::from_utf8 returns Err with Some(bad_len), treat the bytes up to valid_up_to as valid, skip the bad_len bytes, append the valid fragment to buffer (respecting MAX_SSE_BUFFER_SIZE and current_data logic), then continue decoding the remainder of decode_buf in the same chunk; only when from_utf8 reports None (incomplete sequence) should you stash the trailing bytes into utf8_carry. Use the existing identifiers (decode_buf, valid_up_to/error.error_len()/bad_len, carry_start/utf8_carry, buffer, MAX_SSE_BUFFER_SIZE, current_data) to locate and implement this looped-skip-and-continue behavior.src/tools/send_message_to_another_channel.rs (1)
147-198:⚠️ Potential issue | 🟠 MajorBare Signal shorthands still preempt normal channel resolution.
This branch runs before
ChannelStore::find_by_name(), so a bare UUID orgroup:...target gets routed as Signal even in non-Signal conversations. That can hijack legitimate channel IDs/names and send through the wrong adapter. Only honor those implicit Signal shorthands whencurrent_adapteralready starts withsignal; otherwise require an explicitsignal:prefix and let normal channel lookup run first.
🧹 Nitpick comments (1)
src/config/load.rs (1)
2123-2123: Renamesto a descriptive config name.This block is dense enough already that the single-letter closure arg hurts readability more than it helps.
As per coding guidelines, "Don't abbreviate variable names. Use
queuenotq,messagenotmsg,channelnotch. Common abbreviations likeconfigare fine"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/load.rs` at line 2123, Rename the single-letter closure argument in the toml.messaging.signal.and_then(...) call to a descriptive name (e.g., signal_config or signal_settings) to improve readability; update all references inside the closure accordingly so toml.messaging.signal.and_then(|signal_config| { ... }) (and any uses of the old `s` identifier inside the closure) compile and retain the same behavior, keeping the mapping assigned to the signal field intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config/load.rs`:
- Around line 2156-2166: The current logic disables the entire SignalConfig when
default credentials are missing (enabled: s.enabled && has_default_credentials),
which prevents named instances from starting; change the enabled expression to
keep the config enabled when either default credentials exist or at least one
named instance is enabled (e.g., set SignalConfig.enabled to s.enabled &&
(has_default_credentials || instances.iter().any(|instance| instance.enabled))).
Keep has_default_credentials, http_url.unwrap_or_default(), and other fields
unchanged.
---
Outside diff comments:
In `@src/config.rs`:
- Around line 523-532: The helper slack_config_with_dm_users currently ignores
its dm_allowed_users parameter and sets SlackConfig.dm_allowed_users to an empty
vec; update the SlackConfig construction in slack_config_with_dm_users to assign
the passed-in dm_allowed_users (possibly cloned or moved as appropriate) to the
dm_allowed_users field so tests exercise config-level DM users (ensure the
function signature and ownership match how SlackConfig is constructed).
---
Duplicate comments:
In `@src/messaging/signal.rs`:
- Around line 573-594: The current merge of dm_allowed_users and
group_allowed_users lets a wildcard in dm_allowed_users override group
restrictions; change the logic so group_allowed_users is authoritative when
non-empty: first build a list from permissions.group_allowed_users and, if that
list is non-empty, compute sender_allowed using only that list (apply empty =>
block all, ["*"] => allow all, otherwise match allowed == sender or allowed ==
envelope.source_uuid); only if group_allowed_users is empty fall back to
evaluating permissions.dm_allowed_users with the same empty/wildcard/match rules
(use the same matching including envelope.source_uuid).
- Around line 1154-1202: The current logic stores the entire suffix after a
detected invalid UTF-8 byte into utf8_carry, losing any valid UTF-8 that
follows; instead, change the decoding to iterate through decode_buf: when
std::str::from_utf8 returns Err with Some(bad_len), treat the bytes up to
valid_up_to as valid, skip the bad_len bytes, append the valid fragment to
buffer (respecting MAX_SSE_BUFFER_SIZE and current_data logic), then continue
decoding the remainder of decode_buf in the same chunk; only when from_utf8
reports None (incomplete sequence) should you stash the trailing bytes into
utf8_carry. Use the existing identifiers (decode_buf,
valid_up_to/error.error_len()/bad_len, carry_start/utf8_carry, buffer,
MAX_SSE_BUFFER_SIZE, current_data) to locate and implement this
looped-skip-and-continue behavior.
In `@src/messaging/target.rs`:
- Around line 104-118: The metadata-backed Signal target (signal_target) is
returned verbatim and must be normalized before constructing the
BroadcastTarget; update the "signal" arm in the function that builds
BroadcastTarget to pass signal_target through the shared Signal target
normalizer (the same normalization used elsewhere before calling
SignalAdapter::broadcast) prior to creating BroadcastTarget, keeping the adapter
extraction via extract_signal_adapter_from_channel_id(&channel.id) and the
existing json_value_to_string logic intact so tracked DMs that store bare UUIDs
become valid "uuid:...", "group:..." or "+..." targets.
---
Nitpick comments:
In `@src/config/load.rs`:
- Line 2123: Rename the single-letter closure argument in the
toml.messaging.signal.and_then(...) call to a descriptive name (e.g.,
signal_config or signal_settings) to improve readability; update all references
inside the closure accordingly so toml.messaging.signal.and_then(|signal_config|
{ ... }) (and any uses of the old `s` identifier inside the closure) compile and
retain the same behavior, keeping the mapping assigned to the signal field
intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f6e7c281-63c2-4e09-91d9-0d986678d5fa
📒 Files selected for processing (5)
src/config.rssrc/config/load.rssrc/messaging/signal.rssrc/messaging/target.rssrc/tools/send_message_to_another_channel.rs
ibhagwan
force-pushed
the
feat/signal-adapter
branch
2 times, most recently
from
1454fb6 to
2e5d500
Compare
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
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)
377-469:⚠️ Potential issue | 🔴 CriticalThese Signal tests won't compile against the current helpers.
The test module still imports/calls
parse_explicit_signal_target, but this file now exposesparse_explicit_signal_prefix()andparse_implicit_signal_shorthand()instead. Renaming the import is not enough either: the bare UUID / bare+phonecases now belong to the implicit-shorthand helper, so this module needs to be updated to match the new split API.🤖 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 377 - 469, Tests currently call the removed parse_explicit_signal_target; update the test module to import the new helpers parse_explicit_signal_prefix and parse_implicit_signal_shorthand and split cases accordingly: keep explicit-prefixed checks (e.g. "signal:uuid:...", "signal:group:...", "signal:+...", "signal:e164:+...") using parse_explicit_signal_prefix and assert adapter/target as before, and move bare UUID and bare +phone test cases to use parse_implicit_signal_shorthand (asserting they return a signal target), while leaving negative cases to call both helpers as appropriate; adjust imports to reference parse_explicit_signal_prefix and parse_implicit_signal_shorthand and update any test names/comments to reflect the new API.
♻️ Duplicate comments (2)
src/tools/send_message_to_another_channel.rs (1)
147-170:⚠️ Potential issue | 🟠 MajorNamed Signal instances are still routed incorrectly here.
signal:+.../signal:uuid:...in asignal:{instance}conversation still go out through the default"signal"adapter, and the shorthand helper buildssignal:{instance}:...strings before callingparse_delivery_target(), which cannot parse named adapters because it splits at the first:. That means the documented Signal forms either use the wrong account or fail entirely on named Signal adapters.Suggested direction
- if let Some(target) = parse_explicit_signal_prefix(&args.target) { + if let Some(mut target) = parse_explicit_signal_prefix(&args.target) { + if target.adapter == "signal" + && let Some(current_adapter) = self + .current_adapter + .as_ref() + .filter(|adapter| adapter.starts_with("signal")) + { + target.adapter = current_adapter.clone(); + } self.messaging_manager .broadcast( &target.adapter, &target.target, crate::OutboundResponse::Text(args.message), )And for
parse_implicit_signal_shorthand(), build theBroadcastTargetviaparse_signal_target_parts()(or directly) instead of round-tripping throughparse_delivery_target("signal:{instance}:...").Also applies to: 175-203, 322-352
🤖 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 147 - 170, The explicit/implicit Signal handling incorrectly routes named Signal instances because parse_explicit_signal_prefix/parse_implicit_signal_shorthand currently round-trip through parse_delivery_target which splits at the first ':' and loses the named adapter; instead construct the BroadcastTarget using parse_signal_target_parts (or directly) so the adapter/instance are preserved, and pass that target into messaging_manager.broadcast (and return target.target/target.adapter) rather than using parse_delivery_target; update the branches that call parse_explicit_signal_prefix and parse_implicit_signal_shorthand (and anywhere shorthand builds "signal:{instance}:...") to build a BroadcastTarget via parse_signal_target_parts to fix routing for named Signal adapters.src/messaging/signal.rs (1)
1152-1202:⚠️ Potential issue | 🟠 MajorInvalid UTF-8 recovery still drops valid SSE bytes after the bad sequence.
When
error_len()isSome(_), this path skips the bad byte(s) but still pushes the entire remaining suffix intoutf8_carry. Any validdata:lines after the invalid byte are then deferred until a future chunk, and they are lost outright if the stream ends/reconnects first. Keep decoding the remainder of the current chunk aftervalid_up_to + bad_leninstead of treating it as carry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/signal.rs` around lines 1152 - 1202, The current UTF-8 error handling in the decode loop incorrectly treats the bytes after a detected invalid sequence as carry; instead when error.error_len() is Some(bad_len) you should skip the bad_len bytes but continue decoding the remainder of decode_buf in the same iteration (not push it into utf8_carry). Update the logic around decode_buf/valid_len/carry_start (the match on std::str::from_utf8 and subsequent from_utf8(&decode_buf[..valid_len])) so that when Some(bad_len) you set valid_len = valid_up_to, advance an index = valid_up_to + bad_len, then attempt to decode the tail decode_buf[index..] (or loop-process the remainder) appending any valid UTF-8 to buffer and only place an unfinished trailing multi-byte suffix into utf8_carry; keep using the existing names decode_buf, utf8_carry, carry_start, buffer, and MAX_SSE_BUFFER_SIZE to locate where to implement this.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/tools/send_message_to_another_channel.rs`:
- Around line 377-469: Tests currently call the removed
parse_explicit_signal_target; update the test module to import the new helpers
parse_explicit_signal_prefix and parse_implicit_signal_shorthand and split cases
accordingly: keep explicit-prefixed checks (e.g. "signal:uuid:...",
"signal:group:...", "signal:+...", "signal:e164:+...") using
parse_explicit_signal_prefix and assert adapter/target as before, and move bare
UUID and bare +phone test cases to use parse_implicit_signal_shorthand
(asserting they return a signal target), while leaving negative cases to call
both helpers as appropriate; adjust imports to reference
parse_explicit_signal_prefix and parse_implicit_signal_shorthand and update any
test names/comments to reflect the new API.
---
Duplicate comments:
In `@src/messaging/signal.rs`:
- Around line 1152-1202: The current UTF-8 error handling in the decode loop
incorrectly treats the bytes after a detected invalid sequence as carry; instead
when error.error_len() is Some(bad_len) you should skip the bad_len bytes but
continue decoding the remainder of decode_buf in the same iteration (not push it
into utf8_carry). Update the logic around decode_buf/valid_len/carry_start (the
match on std::str::from_utf8 and subsequent from_utf8(&decode_buf[..valid_len]))
so that when Some(bad_len) you set valid_len = valid_up_to, advance an index =
valid_up_to + bad_len, then attempt to decode the tail decode_buf[index..] (or
loop-process the remainder) appending any valid UTF-8 to buffer and only place
an unfinished trailing multi-byte suffix into utf8_carry; keep using the
existing names decode_buf, utf8_carry, carry_start, buffer, and
MAX_SSE_BUFFER_SIZE to locate where to implement this.
In `@src/tools/send_message_to_another_channel.rs`:
- Around line 147-170: The explicit/implicit Signal handling incorrectly routes
named Signal instances because
parse_explicit_signal_prefix/parse_implicit_signal_shorthand currently
round-trip through parse_delivery_target which splits at the first ':' and loses
the named adapter; instead construct the BroadcastTarget using
parse_signal_target_parts (or directly) so the adapter/instance are preserved,
and pass that target into messaging_manager.broadcast (and return
target.target/target.adapter) rather than using parse_delivery_target; update
the branches that call parse_explicit_signal_prefix and
parse_implicit_signal_shorthand (and anywhere shorthand builds
"signal:{instance}:...") to build a BroadcastTarget via
parse_signal_target_parts to fix routing for named Signal adapters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b2c92200-f21e-4f25-9499-f79b6f402985
📒 Files selected for processing (4)
src/config.rssrc/messaging/signal.rssrc/messaging/target.rssrc/tools/send_message_to_another_channel.rs
ibhagwan
force-pushed
the
feat/signal-adapter
branch
from
e53dafa to
f4a5e6a
Compare
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
When an LLM call fails (e.g., provider error like StepFun's 'Unrecognized chat message'), the channel was only logging the error and sending nothing to the user. This led to confusing silent failures where the user received no response and had to check logs to understand what happened. Changes: - src/agent/channel.rs: Modified error handler to send error to user - Formats error message and sends via response_tx - User now sees: 'I encountered an error: ...' - Still logs full error for debugging This ensures users always receive feedback when something goes wrong, even if it's an internal/provider error rather than a channel bug.
ibhagwan
force-pushed
the
feat/signal-adapter
branch
from
f4a5e6a to
051e16e
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/tools/send_message_to_another_channel.rs (1)
254-262: Usechannelinstead ofchper coding guidelines.The coding guidelines specify: "Don't abbreviate variable names...
channelnotch".Suggested fix
let channel = match channel_result { - Some(ch) => ch, + Some(channel) => channel, None => { return Err(SendMessageError(format!(🤖 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 254 - 262, The match on channel_result binds the Some variant to the abbreviated name ch; change the binding to the full name channel (i.e. replace Some(ch) => ch with Some(channel) => channel) so the returned variable follows the coding guideline; keep the rest of the match (the None branch that constructs SendMessageError using args.target) 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/tools/send_message_to_another_channel.rs`:
- Around line 254-262: The match on channel_result binds the Some variant to the
abbreviated name ch; change the binding to the full name channel (i.e. replace
Some(ch) => ch with Some(channel) => channel) so the returned variable follows
the coding guideline; keep the rest of the match (the None branch that
constructs SendMessageError using args.target) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 873527f3-f620-4901-99bd-3352fe0f0464
📒 Files selected for processing (1)
src/tools/send_message_to_another_channel.rs
ibhagwan
force-pushed
the
feat/signal-adapter
branch
3 times, most recently
from
fd8409f to
301a0fb
Compare
Implements Signal messaging support using the signal-cli daemon HTTP API,
following the existing adapter architecture (Telegram, Discord, Slack, Twitch).
- Inbound: SSE stream with automatic reconnection and exponential backoff
(2s → 60s), UTF-8 chunk boundary handling, buffer overflow protection.
- Outbound: JSON-RPC send calls. DM recipients must be a JSON array.
- Typing indicators: JSON-RPC sendTyping with ~5s expiry.
- Attachments: Temp files in {instance_dir}/tmp/, auto-cleaned after send.
- Streaming: Not supported (Signal can't edit messages).
- Permissions: DM allowlist + group filter (None = block all groups).
Config types in types.rs, toml_schema.rs, load.rs. SignalPermissions in
permissions.rs with from_config/from_instance_config. Hot-reload support in
watcher.rs. 23 unit tests.
Add to config.toml:
```toml
[messaging.signal]
enabled = true
http_url = "http://127.0.0.1:8686"
account = "+1234567890"
dm_allowed_users = ["+0987654321"]
group_ids = ["<uuid1>", "<uuid2>"]
group_allowed_users = ["+5566778899", "+1122334455"]
ignore_stories = true
[[messaging.signal.instances]]
name = "work"
enabled = true
http_url = "http://127.0.0.1:8687"
account = "+1122334455"
dm_allowed_users = ["+5566778899"]
group_ids = ["<uuid1>", "<uuid2>"]
group_allowed_users = ["+1122334455"]
```
Requires signal-cli daemon running: `signal-cli daemon --http`
Closes spacedriveapp#310
ibhagwan
force-pushed
the
feat/signal-adapter
branch
from
301a0fb to
dc2c3e7
Compare
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.
Implements Signal messaging support using the signal-cli daemon HTTP API,
following the existing adapter architecture (Telegram, Discord, Slack, Twitch).
(2s → 60s), UTF-8 chunk boundary handling, buffer overflow protection.
Config types in types.rs, toml_schema.rs, load.rs. SignalPermissions in
permissions.rs with from_config/from_instance_config. Hot-reload support in
watcher.rs. 23 unit tests.
Add to config.toml:
Requires signal-cli daemon running:
signal-cli daemon --httpCloses #310
Note
Summary: Complete Signal adapter implementation (1953 lines) following the existing messaging adapter pattern. Adds SSE-based inbound stream with exponential backoff reconnection, JSON-RPC outbound messaging, typing indicators, file attachment handling, and fine-grained permission controls (DM allowlist + group filters). Configuration schema extended for Signal with multi-instance support. Includes 23 unit tests covering JSON parsing, permissions, and SSE edge cases. No changes to core agent or Rig integration—purely additive messaging module.
Written by Tembo for commit 83b8f72. This will update automatically on new commits.