feat(tool/bluebubbles): send_attachment tool for iMessage media

[maxtongwang]

Summary

• Base branch target: main
• Problem: ZeroClaw's BlueBubbles channel cannot send media files — the LLM agent has no way to attach images, audio, or documents to iMessages.
• Why it matters: Sending attachments is a core messaging capability required for feature parity with OpenClaw's BlueBubbles extension.
• What changed: New bluebubbles_send_attachment tool that POSTs multipart form-data to the BlueBubbles /api/v1/message/attachment private-API endpoint. Tool accepts chat_guid, filename, data_base64, mime_type, optional caption, and optional as_voice flag. Registered in the tool factory alongside the existing group tool when a BB config is present.
• What did not change: No changes to the BlueBubbles channel receive path, config schema, gateway, or any existing tools.

Label Snapshot (required)

• Risk label: risk: medium
• Size label: size: S
• Scope labels: tool
• Module labels: tool: bluebubbles
• Contributor tier label: (auto-managed)
• If any auto-label is incorrect, note requested correction: N/A

Change Metadata

• Change type: feature
• Primary scope: tool

Linked Issue

• Depends on feat(tool/bluebubbles): group management tool — rename, add/remove participants, icon, leave #2532
• Linear issue key(s): external contributor — no Linear key

Supersede Attribution (required when Supersedes # is used)

N/A — this is a new feature, not superseding any PR.

Validation Evidence (required)

cargo fmt --all -- --check   # pass
cargo clippy --all-targets -- -D warnings  # pass (delta scope)
cargo test   # pass

• Evidence: fork PR [LOW] Bearer tokens not zeroized in memory after use #17 merged to maxtongwang/zeroclaw after full CI green + CodeRabbit review + human review.
• Build verified clean on feat/bb-issue4-upstream-pr (stacked on feat(tool/bluebubbles): group management tool — rename, add/remove participants, icon, leave #2532).

Security Impact (required)

• New permissions/capabilities? Yes — allows the LLM agent to send arbitrary files to iMessage contacts via BlueBubbles server.
• New external network calls? Yes — POST to BlueBubbles server /api/v1/message/attachment.
• Secrets/tokens handling changed? No — uses the existing password field from BB config, same as group tool.
• File system access scope changed? No — data comes from the LLM tool call argument (base64), never touches the filesystem.
• Risk: Agent could send unexpected media. Mitigated by the existing dm_policy/group_policy allowlist gating on inbound messages and by the fact that the agent only acts on explicit user instruction.

Privacy and Data Hygiene (required)

• Data-hygiene status: pass
• Redaction/anonymization notes: attachment bytes flow in memory only; not logged.
• Neutral wording confirmation: pass — no identity-like wording in code or tests.

Compatibility / Migration

• Backward compatible? Yes — new tool only registered when BB config is present; no changes to existing tools or config keys.
• Config/env changes? No
• Migration needed? No

i18n Follow-Through (required when docs or user-facing wording changes)

• i18n follow-through triggered? No — this PR adds a new tool file and wires it in mod.rs; no user-facing docs updated.

Human Verification (required)

• Verified scenarios: tool schema validated against BB private-API docs; multipart construction tested; base64 decode/size guard tested.
• Edge cases checked: empty chat_guid, missing filename, oversized payload (>25 MB base64), invalid base64, as_voice flag on non-audio mime type.
• What was not verified: live iMessage send on physical hardware (requires active BB server).

Side Effects / Blast Radius (required)

• Affected subsystems/workflows: src/tools/ (new file + mod.rs registration), src/gateway/mod.rs (no logic change, tool factory call unchanged).
• Potential unintended effects: None — purely additive.
• Guardrails/monitoring: BB server returns HTTP error on invalid payloads; tool returns ToolResult { success: false } with sanitized error.

Agent Collaboration Notes (recommended)

• Agent tools used: Claude Code
• Workflow summary: Fork PR [LOW] Bearer tokens not zeroized in memory after use #17 implemented, reviewed (CodeRabbit + human), merged. This is the upstream PR carrying forward only the issue-4-specific commits cherry-picked onto the stacked upstream PR branch.
• Confirmation: naming + architecture boundaries followed per AGENTS.md + CONTRIBUTING.md.

Rollback Plan (required)

• Fast rollback: revert src/tools/bluebubbles_send_attachment.rs + two-line mod.rs change.
• Feature flags: none — controlled entirely by presence of [channels_config.bluebubbles] in config.
• Observable failure symptoms: BlueBubblesSendAttachmentTool absent from zeroclaw tools list; multipart POST failures surface as ToolResult { success: false }.

Risks and Mitigations

• Risk: Large base64 payload could OOM the agent process.
  • Mitigation: 25 MiB hard cap on data_base64 input checked before decoding; returns ToolResult { success: false } on oversize.

Summary by CodeRabbit

• New Features

  • BlueBubbles iMessage: group management, send attachments, reactions, read receipts, per-channel DM/group policies, background webhook processing (202 accepted) with concurrency limits (503 when busy).
  • Local audio transcription support (Whisper) that can augment messages.
  • Google OAuth linking for external services.
  • Tools to manage BlueBubbles groups and to send media attachments.
  • Improved web fetch redirect handling.

• Documentation

  • Expanded config, channel docs, examples, and localized guides for BlueBubbles and related features.

Review Fixes (Round 2)

Propagated docs fix from base branch feat/bluebubbles-transcription-tapback via rebase:

Chinese command text in English docs: docs/channels-reference.md natural-language approval command examples updated from 授权工具 shell to approve tool shell.

Review Fixes (Round 3)

No-panic constructor + trim string inputs: bluebubbles_send_attachment.rs replaced .expect("valid reqwest client config") with .unwrap_or_else(|_| reqwest::Client::new()) to prevent panic on TLS misconfiguration. Added .trim() to chat_guid and filename validation to consistently reject whitespace-only inputs.

Review Fixes (Round 6)

• oauth.rs: propagate HTTP client build failure — no silent timeout loss
• oauth.rs: validate PKCE state before consuming session file (CSRF fix)
• oauth.rs: use POST form body for token revocation (RFC 7009 compliance)
• transcription.rs: explicit fail on non-UTF-8 temp paths, not empty-string passthrough
• transcription.rs: clean up partial ffmpeg WAV on conversion failure
• bluebubbles.rs: remove attachment GUID from error log
• claude-review.yml: fix step-level PR condition (!= '' → != null)

Review Fixes (Round 7)

• bluebubbles_send_attachment.rs: explicit anyhow::Result<Self> constructor
• bluebubbles_send_attachment.rs: attachment cap corrected from 50 MiB to 34 MiB b64 (≈ 25 MiB decoded, matching stated guardrail)
• bluebubbles_send_attachment.rs: transport error returns Ok(ToolResult { success: false }) instead of propagating Err
• src/tools/mod.rs: registration updated to handle anyhow::Result constructor

Review Fixes (Round 8)

• bluebubbles_send_attachment.rs: inject Arc<SecurityPolicy> — added can_act() and record_action() checks matching the pattern used by all other side-effect tools
• bluebubbles_send_attachment.rs: data_base64 trimmed via .map(str::trim) before emptiness check — whitespace-only inputs now fail early with a clear error rather than a confusing base64 decode error
• bluebubbles_send_attachment.rs: mime_type trimmed via .map(str::trim).filter(|v| !v.is_empty()) — whitespace-only mime_type now falls back to application/octet-stream correctly

Review Fixes (cargo fmt)

• Applied cargo fmt --all to files changed in this PR's diff — collapsed single-use iterator chains and reformatted match arm bodies to comply with project rustfmt settings

[maxtongwang]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[maxtongwang]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[maxtongwang]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

♻️ Duplicate comments (6)

src/tools/bluebubbles_send_attachment.rs (1)

93-107: ⚠️ Potential issue | 🟠 Major

Charge rate-limit only after attachment inputs are fully validated.

At Line 101, record_action() is called before validating chat_guid, filename, data_base64, and as_voice/mime_type consistency. Bad payloads can burn quota without any external send.

🔧 Suggested reordering

         if !self.security.can_act() {
             return Ok(ToolResult {
                 success: false,
                 output: String::new(),
                 error: Some("Action blocked: read-only autonomy level".into()),
             });
         }
-        if !self.security.record_action() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Rate limit exceeded: too many actions in the last hour".into()),
-            });
-        }
         let chat_guid = ...
         let filename = ...
         let data_b64 = ...
         let mime_type = ...
         let as_voice = ...
+        if !self.security.record_action() {
+            return Ok(ToolResult {
+                success: false,
+                output: String::new(),
+                error: Some("Rate limit exceeded: too many actions in the last hour".into()),
+            });
+        }

         let resp = match self
             .client

As per coding guidelines src/tools/**/*.rs: “Validate and sanitize all inputs. Return structured ToolResult; avoid panics in runtime path.”

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/tools/bluebubbles_send_attachment.rs` around lines 93 - 107, The current
execute method charges the rate-limit via self.security.record_action() before
input validation, which can consume quota for malformed requests; move the
record_action() call to after full validation and sanitization of args (check
presence and types of chat_guid, filename, data_base64, and validate consistency
between as_voice and mime_type) and only call self.security.record_action() if
validation passes; keep the early can_act() check, return the same structured
ToolResult error responses for validation failures, and ensure record_action()
failure still returns the rate-limit ToolResult as before.

src/tools/bluebubbles_group.rs (1)

99-113: ⚠️ Potential issue | 🟠 Major

Move rate-limit charging after action-specific input validation.

record_action() at Line 107 executes before validating per-action required fields (display_name, address, icon_base64). Invalid payloads can consume quota without any outbound mutation.

🔧 Representative fix pattern

-        if !self.security.record_action() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Rate limit exceeded: too many actions in the last hour".into()),
-            });
-        }
         let action = match args.get("action").and_then(|v| v.as_str()) {
             ...
         };
         let chat_guid = ...
         let encoded_guid = ...

         match action.as_str() {
             "rename_group" => {
                 let name = ...
+                if !self.security.record_action() {
+                    return Ok(ToolResult {
+                        success: false,
+                        output: String::new(),
+                        error: Some("Rate limit exceeded: too many actions in the last hour".into()),
+                    });
+                }
                 let url = self.api_url(&format!("/api/v1/chat/{encoded_guid}"));
                 ...
             }
             // repeat same placement for add/remove/leave/set_group_icon branches

As per coding guidelines src/tools/**/*.rs: “Validate and sanitize all inputs. Return structured ToolResult; avoid panics in runtime path.”

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/tools/bluebubbles_group.rs` around lines 99 - 113, The execute method
currently calls self.security.record_action() before validating action-specific
inputs, allowing invalid payloads to consume rate-limit quota; update execute
(in BlueBubblesGroup::execute) to first check self.security.can_act(), then
validate and sanitize the incoming serde_json::Value for required fields (e.g.,
display_name, address, icon_base64) and return a structured ToolResult error for
missing/invalid fields, and only after all validations pass call
self.security.record_action(); ensure validation logic is performed before any
quota-affecting call and that failing validation returns Ok(ToolResult {
success: false, error: Some(...), output: "" }) without calling record_action().

src/gateway/mod.rs (3)

2377-2378: ⚠️ Potential issue | 🟡 Minor

Fix stale inline status-code comment (200 vs 202).

Line 2377 says “Returning 200 immediately…”, but the handler returns StatusCode::ACCEPTED (202) at Line 2485. Please keep the comment aligned with runtime behavior.

Also applies to: 2484-2486

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/gateway/mod.rs` around lines 2377 - 2378, The inline comment that says
“Returning 200 immediately…” is out of sync with the actual returned status
(StatusCode::ACCEPTED / 202); update that comment to reflect 202/ACCEPTED and
any wording about HTTP lifecycle/timeout budget accordingly, and make the same
correction for the duplicated comment near the block that returns
StatusCode::ACCEPTED so both comments align with the runtime behavior (reference
StatusCode::ACCEPTED in the comment).

---

2381-2482: ⚠️ Potential issue | 🟠 Major

Add local timeouts in the background BlueBubbles worker to prevent permit starvation.

The semaphore permit is held for the full spawned task, but transcription/LLM/send calls in this block have no local timeout. A stuck downstream call can pin permits and force prolonged 503 responses.

🛡️ Suggested timeout guards

+const BB_WORKER_STEP_TIMEOUT_SECS: u64 = 120;
 ...
     tokio::spawn(async move {
         let _permit = permit; // released when this task completes
-        let messages = bb.parse_webhook_payload_with_transcription(&payload).await;
+        let messages = match tokio::time::timeout(
+            Duration::from_secs(BB_WORKER_STEP_TIMEOUT_SECS),
+            bb.parse_webhook_payload_with_transcription(&payload),
+        )
+        .await
+        {
+            Ok(messages) => messages,
+            Err(_) => {
+                tracing::error!("BlueBubbles webhook timed out during payload parsing");
+                return;
+            }
+        };
 ...
-            match run_gateway_chat_with_tools(&state_bg, &msg.content, Some(&session_id)).await {
+            let llm_result = tokio::time::timeout(
+                Duration::from_secs(BB_WORKER_STEP_TIMEOUT_SECS),
+                run_gateway_chat_with_tools(&state_bg, &msg.content, Some(&session_id)),
+            )
+            .await;
+            match llm_result {
+                Ok(Ok(response)) => {
+                    ...
+                }
+                Ok(Err(e)) => {
+                    ...
+                }
+                Err(_) => {
+                    let _ = bb.stop_typing(&msg.reply_target).await;
+                    tracing::error!("BlueBubbles message processing timed out");
+                    let _ = bb.send(&SendMessage::new(
+                        "Sorry, processing timed out. Please try again.",
+                        &msg.reply_target,
+                    )).await;
+                }
+            }

As per coding guidelines src/{security,runtime,gateway,tools}/**/*.rs: “For security/runtime/gateway/tools changes, include at least one boundary/failure-mode validation.”

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/gateway/mod.rs` around lines 2381 - 2482, The spawned BlueBubbles worker
currently holds the semaphore permit for the entire task and awaits long-running
calls (parse_webhook_payload_with_transcription, run_gateway_chat_with_tools,
bb.send, bb.mark_read, start_typing/stop_typing) with no timeouts, which can
deadlock permits; wrap each external/LLM/transcription/send call with
tokio::time::timeout (e.g., a configurable small boundary like 5–30s) and handle
timeout errors explicitly (log, increment a metric, return early or send a
pragmatic failure reply) so the permit is dropped promptly; specifically, apply
timeout guards around
bb.parse_webhook_payload_with_transcription(&payload).await,
bb.mark_read(...).await, bb.start_typing/stop_typing,
run_gateway_chat_with_tools(...).await, and bb.send(...).await, and ensure any
timeout path stops further processing for that message and returns a clear
failure-mode behavior (log and optional user-facing notice) to satisfy the
boundary/failure-mode validation.

---

562-591: ⚠️ Potential issue | 🟠 Major

Don’t silently disable BlueBubbles when initialization fails.

Line 590-Line 591 (.ok().flatten()) swallows BlueBubbles init errors and continues startup with the channel disabled, even when it is configured.

🔧 Fail-fast patch

-    let bluebubbles_channel: Option<Arc<BlueBubblesChannel>> = config
+    let bluebubbles_channel: Option<Arc<BlueBubblesChannel>> = config
         .channels_config
         .bluebubbles
         .as_ref()
         .map(|bb| {
             ...
         })
         .transpose()
         .map_err(|e| {
             tracing::error!("Failed to initialize BlueBubbles channel: {e}");
             e
-        })
-        .ok()
-        .flatten();
+        })?;

As per coding guidelines src/**/*.rs: “Prefer explicit bail!/errors for unsupported or unsafe states; never silently broaden permissions/capabilities.”

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/gateway/mod.rs` around lines 562 - 591, The initialization currently
swallows BlueBubbles errors via `.ok().flatten()`, so change the tail of the
chain to propagate failures instead of silencing them: replace
`.transpose().map_err(|e| { tracing::error!("Failed to initialize BlueBubbles
channel: {e}"); e }).ok().flatten()` with a `?`-based propagation such as
`.transpose().map_err(|e| { tracing::error!("Failed to initialize BlueBubbles
channel: {e}"); e })?` so that errors from BlueBubblesChannel::new /
with_policies bubble up (causing startup to fail) while preserving the
tracing::error log; this targets the bluebubbles_channel initialization
expression (BlueBubblesChannel, bluebubbles_channel, transpose/map_err chain).

src/channels/bluebubbles.rs (1)

639-646: ⚠️ Potential issue | 🟠 Major

Enforce attachment size cap before appending each chunk.

Line 644 appends chunk bytes first, then Line 645 checks the cap. A single oversized chunk can still push memory over the intended limit before bailing.

🔧 Proposed fix

         while let Some(chunk) = resp
             .chunk()
             .await
             .map_err(|e| anyhow::anyhow!("BB attachment body read failed: {}", e.without_url()))?
         {
-            buf.extend_from_slice(&chunk);
-            if buf.len() > MAX_BYTES {
+            if buf.len().saturating_add(chunk.len()) > MAX_BYTES {
                 anyhow::bail!("BB attachment too large (>{MAX_BYTES} bytes)");
             }
+            buf.extend_from_slice(&chunk);
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/channels/bluebubbles.rs` around lines 639 - 646, Issue: the loop that
reads response chunks appends chunk data to buf before checking the MAX_BYTES
limit, allowing a single large chunk to exceed memory cap; fix by checking the
chunk size plus current buf.len() before calling buf.extend_from_slice. Locate
the while loop using resp.chunk().await and the variables buf and MAX_BYTES, and
change the logic to compute prospective_len = buf.len() + chunk.len() and bail
with anyhow::bail! if prospective_len > MAX_BYTES (keeping the same error
message), only calling buf.extend_from_slice(&chunk) after that check.

🤖 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/channels/bluebubbles.rs`:
- Around line 639-646: Issue: the loop that reads response chunks appends chunk
data to buf before checking the MAX_BYTES limit, allowing a single large chunk
to exceed memory cap; fix by checking the chunk size plus current buf.len()
before calling buf.extend_from_slice. Locate the while loop using
resp.chunk().await and the variables buf and MAX_BYTES, and change the logic to
compute prospective_len = buf.len() + chunk.len() and bail with anyhow::bail! if
prospective_len > MAX_BYTES (keeping the same error message), only calling
buf.extend_from_slice(&chunk) after that check.

In `@src/gateway/mod.rs`:
- Around line 2377-2378: The inline comment that says “Returning 200
immediately…” is out of sync with the actual returned status
(StatusCode::ACCEPTED / 202); update that comment to reflect 202/ACCEPTED and
any wording about HTTP lifecycle/timeout budget accordingly, and make the same
correction for the duplicated comment near the block that returns
StatusCode::ACCEPTED so both comments align with the runtime behavior (reference
StatusCode::ACCEPTED in the comment).
- Around line 2381-2482: The spawned BlueBubbles worker currently holds the
semaphore permit for the entire task and awaits long-running calls
(parse_webhook_payload_with_transcription, run_gateway_chat_with_tools, bb.send,
bb.mark_read, start_typing/stop_typing) with no timeouts, which can deadlock
permits; wrap each external/LLM/transcription/send call with
tokio::time::timeout (e.g., a configurable small boundary like 5–30s) and handle
timeout errors explicitly (log, increment a metric, return early or send a
pragmatic failure reply) so the permit is dropped promptly; specifically, apply
timeout guards around
bb.parse_webhook_payload_with_transcription(&payload).await,
bb.mark_read(...).await, bb.start_typing/stop_typing,
run_gateway_chat_with_tools(...).await, and bb.send(...).await, and ensure any
timeout path stops further processing for that message and returns a clear
failure-mode behavior (log and optional user-facing notice) to satisfy the
boundary/failure-mode validation.
- Around line 562-591: The initialization currently swallows BlueBubbles errors
via `.ok().flatten()`, so change the tail of the chain to propagate failures
instead of silencing them: replace `.transpose().map_err(|e| {
tracing::error!("Failed to initialize BlueBubbles channel: {e}"); e
}).ok().flatten()` with a `?`-based propagation such as
`.transpose().map_err(|e| { tracing::error!("Failed to initialize BlueBubbles
channel: {e}"); e })?` so that errors from BlueBubblesChannel::new /
with_policies bubble up (causing startup to fail) while preserving the
tracing::error log; this targets the bluebubbles_channel initialization
expression (BlueBubblesChannel, bluebubbles_channel, transpose/map_err chain).

In `@src/tools/bluebubbles_group.rs`:
- Around line 99-113: The execute method currently calls
self.security.record_action() before validating action-specific inputs, allowing
invalid payloads to consume rate-limit quota; update execute (in
BlueBubblesGroup::execute) to first check self.security.can_act(), then validate
and sanitize the incoming serde_json::Value for required fields (e.g.,
display_name, address, icon_base64) and return a structured ToolResult error for
missing/invalid fields, and only after all validations pass call
self.security.record_action(); ensure validation logic is performed before any
quota-affecting call and that failing validation returns Ok(ToolResult {
success: false, error: Some(...), output: "" }) without calling record_action().

In `@src/tools/bluebubbles_send_attachment.rs`:
- Around line 93-107: The current execute method charges the rate-limit via
self.security.record_action() before input validation, which can consume quota
for malformed requests; move the record_action() call to after full validation
and sanitization of args (check presence and types of chat_guid, filename,
data_base64, and validate consistency between as_voice and mime_type) and only
call self.security.record_action() if validation passes; keep the early
can_act() check, return the same structured ToolResult error responses for
validation failures, and ensure record_action() failure still returns the
rate-limit ToolResult as before.

---

ℹ️ Review info

Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 94a10cff-cedd-4246-af73-68d5239ab122

📥 Commits

Reviewing files that changed from the base of the PR and between 0289808 and 86c512b.

📒 Files selected for processing (5)

• src/channels/bluebubbles.rs
• src/gateway/mod.rs
• src/tools/bluebubbles_group.rs
• src/tools/bluebubbles_send_attachment.rs
• src/tools/mod.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• src/tools/mod.rs

[maxtongwang]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (4)

src/tools/bluebubbles_group.rs (1)

134-143: ⚠️ Potential issue | 🟠 Major

Move quota charging after action-specific validation.

At Line 135, record_action() runs before required per-action inputs are validated (e.g., display_name, address, icon_base64), so invalid payloads still consume rate-limit budget.

🔧 Suggested fix

-        // All common inputs validated; charge rate-limit only before mutation.
-        if !self.security.record_action() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Rate limit exceeded: too many actions in the last hour".into()),
-            });
-        }
-
         match action.as_str() {
             "rename_group" => {
                 let name = match args.get("display_name").and_then(|v| v.as_str()) {
                     Some(n) if !n.trim().is_empty() => n.trim().to_string(),
                     _ => {
@@
                         })
                     }
                 };
+                if !self.security.record_action() {
+                    return Ok(ToolResult {
+                        success: false,
+                        output: String::new(),
+                        error: Some("Rate limit exceeded: too many actions in the last hour".into()),
+                    });
+                }
                 let url = self.api_url(&format!("/api/v1/chat/{encoded_guid}"));

Apply the same placement in each mutating action branch after that branch’s required-input validation.

As per coding guidelines src/tools/**/*.rs: “Validate and sanitize all inputs” and return structured ToolResult paths.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/tools/bluebubbles_group.rs` around lines 134 - 143, The current
rate-limit call self.security.record_action() is executed before action-specific
required-input validation, causing invalid payloads to consume quota; update the
code so that record_action() is only invoked inside each mutating action branch
(the branches of match action.as_str()) immediately after that branch's
required-input validation (e.g., after checks for display_name, address,
icon_base64) and before performing the mutation and returning a ToolResult;
ensure you keep the same structured ToolResult error paths for validation
failures and only call record_action() on the successful-per-validation path
within functions/methods handling the mutating actions.

src/gateway/mod.rs (2)

585-591: ⚠️ Potential issue | 🟠 Major

Don’t swallow BlueBubbles initialization failures.

At Lines 590-591, .ok().flatten() converts a configured-channel init error into None, leaving startup running with integration silently disabled.

🔧 Suggested fix

-        .transpose()
-        .map_err(|e| {
-            tracing::error!("Failed to initialize BlueBubbles channel: {e}");
-            e
-        })
-        .ok()
-        .flatten();
+        .transpose()
+        .map_err(|e| {
+            tracing::error!("Failed to initialize BlueBubbles channel: {e}");
+            e
+        })?;

As per coding guidelines src/**/*.rs: prefer explicit error paths and avoid silent fallback that broadens/changes capability behavior.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/gateway/mod.rs` around lines 585 - 591, The BlueBubbles channel init is
currently swallowing errors by ending the chain with .ok().flatten(); instead,
propagate failures explicitly: remove the trailing .ok().flatten() on the
expression that calls .transpose().map_err(|e| { tracing::error!("Failed to
initialize BlueBubbles channel: {e}"); e }) and return or propagate the Result
(e.g. use ? on the Result or convert into an explicit Err path) so
initialization errors are surfaced to the caller instead of being silently
turned into None.

---

2381-2489: ⚠️ Potential issue | 🟠 Major

Bound worker steps with local timeouts to avoid permit starvation.

The semaphore permit is held for the full spawned task, but Lines 2383 and 2463 run without local timeout guards. A stalled transcription/LLM step can pin permits and keep /bluebubbles in prolonged 503 mode.

⏱️ Suggested hardening

+const BB_WORKER_STEP_TIMEOUT_SECS: u64 = 120;
 ...
     tokio::spawn(async move {
         let _permit = permit; // released when this task completes
-        let messages = bb.parse_webhook_payload_with_transcription(&payload).await;
+        let messages = match tokio::time::timeout(
+            Duration::from_secs(BB_WORKER_STEP_TIMEOUT_SECS),
+            bb.parse_webhook_payload_with_transcription(&payload),
+        )
+        .await
+        {
+            Ok(messages) => messages,
+            Err(_) => {
+                tracing::error!("BlueBubbles webhook parsing timed out");
+                return;
+            }
+        };
 ...
-            match run_gateway_chat_with_tools(&state_bg, &msg.content, Some(&session_id)).await {
-                Ok(response) => {
+            match tokio::time::timeout(
+                Duration::from_secs(BB_WORKER_STEP_TIMEOUT_SECS),
+                run_gateway_chat_with_tools(&state_bg, &msg.content, Some(&session_id)),
+            )
+            .await
+            {
+                Ok(Ok(response)) => {
                     ...
                 }
-                Err(e) => {
+                Ok(Err(e)) => {
                     ...
                 }
+                Err(_) => {
+                    let _ = bb.stop_typing(&msg.reply_target).await;
+                    tracing::error!("BlueBubbles message processing timed out");
+                }
             }
         }
     });

Based on learnings: for high-risk gateway/runtime flows, include boundary/failure-mode protection and validation.

src/tools/bluebubbles_send_attachment.rs (1)

175-211: ⚠️ Potential issue | 🟡 Minor

Charge rate limit after full local validation.

record_action() on Line 175 runs before base64 decode (Line 184) and MIME validation (Line 201), so invalid payloads still burn quota. Move it to immediately before the outbound .send() call.

🔧 Suggested reordering

-        // All inputs validated; charge rate-limit only if we are about to mutate.
-        if !self.security.record_action() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Rate limit exceeded: too many actions in the last hour".into()),
-            });
-        }
-
         let file_bytes =
             match base64::Engine::decode(&base64::engine::general_purpose::STANDARD, data_b64) {
                 Ok(b) => b,
                 Err(e) => {
                     return Ok(ToolResult {
@@
         if as_voice {
             form = form.text("isAudioMessage", "true");
         }
+
+        // Charge rate-limit only once all local validation has passed.
+        if !self.security.record_action() {
+            return Ok(ToolResult {
+                success: false,
+                output: String::new(),
+                error: Some("Rate limit exceeded: too many actions in the last hour".into()),
+            });
+        }
 
         let resp = match self
             .client

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/tools/bluebubbles_send_attachment.rs` around lines 175 - 211, The current
flow calls self.security.record_action() before validating the payload, causing
invalid requests to consume quota; move the record_action() invocation from its
current position (before base64::Engine::decode) to just prior to the outbound
HTTP send (immediately before the reqwest client .send() call that submits the
multipart built with reqwest::multipart::Part::bytes and mime_str), so
base64::Engine::decode and the reqwest::multipart::Part creation/mime validation
run first and only successful, fully validated requests decrement the rate
limit.

🧹 Nitpick comments (2)

src/channels/transcription.rs (1)

478-482: Make the new test assert a real invariant.

At Line 479, the test currently only verifies “no panic,” so it can’t fail for regressions in behavior.

♻️ Optional tightening

 #[test]
 fn resolve_whisper_bin_returns_str_or_none() {
-    // Just assert the function doesn't panic; result depends on local install.
-    let _ = resolve_whisper_bin();
+    let resolved = resolve_whisper_bin();
+    assert!(resolved.is_none() || resolved.is_some_and(|path| !path.trim().is_empty()));
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/channels/transcription.rs` around lines 478 - 482, The test
resolve_whisper_bin_returns_str_or_none currently only ensures no panic; change
it to assert a real invariant by matching on the result of
resolve_whisper_bin(): if it returns Some(path) assert the Path exists and is a
file (e.g. path.exists() && path.is_file()), otherwise allow None; keep the test
name resolve_whisper_bin_returns_str_or_none and use resolve_whisper_bin() as
the unique symbol to locate and update the assertion logic so regressions where
the function returns an invalid path will fail the test.

src/tools/bluebubbles_send_attachment.rs (1)

154-221: Trim caption before emptiness check.

Whitespace-only captions currently pass through as a message. Trim first, then include only if non-empty.

🧹 Suggested tweak

-        let caption = args
+        let caption = args
             .get("caption")
             .and_then(|v| v.as_str())
             .unwrap_or("")
+            .trim()
             .to_string();

As per coding guidelines `src/tools/**/*.rs`: “validate and sanitize all inputs.”

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/tools/bluebubbles_send_attachment.rs` around lines 154 - 221, Trim the
caption input immediately after extraction so whitespace-only captions are
treated as empty: when you build caption from args.get("caption") update it to
be trimmed (e.g., use .trim()/trimmed string) and then use that trimmed value in
the subsequent emptiness check and when adding the "message" field to the
multipart form (replace uses of caption in the if !caption.is_empty() branch and
form.text("message", caption) with the trimmed caption variable).

🤖 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/gateway/mod.rs`:
- Around line 2377-2378: Update the inline comment that currently says
“Returning 200 immediately” to reflect the actual status code returned (202
Accepted) by the handler; locate the comment near the response handling block in
src/gateway/mod.rs that explains returning immediately to keep the HTTP
lifecycle within the timeout budget and change the text to “Returning 202
Accepted immediately” (or equivalent) so the comment matches the handler's
returned status.

---

Duplicate comments:
In `@src/gateway/mod.rs`:
- Around line 585-591: The BlueBubbles channel init is currently swallowing
errors by ending the chain with .ok().flatten(); instead, propagate failures
explicitly: remove the trailing .ok().flatten() on the expression that calls
.transpose().map_err(|e| { tracing::error!("Failed to initialize BlueBubbles
channel: {e}"); e }) and return or propagate the Result (e.g. use ? on the
Result or convert into an explicit Err path) so initialization errors are
surfaced to the caller instead of being silently turned into None.

In `@src/tools/bluebubbles_group.rs`:
- Around line 134-143: The current rate-limit call self.security.record_action()
is executed before action-specific required-input validation, causing invalid
payloads to consume quota; update the code so that record_action() is only
invoked inside each mutating action branch (the branches of match
action.as_str()) immediately after that branch's required-input validation
(e.g., after checks for display_name, address, icon_base64) and before
performing the mutation and returning a ToolResult; ensure you keep the same
structured ToolResult error paths for validation failures and only call
record_action() on the successful-per-validation path within functions/methods
handling the mutating actions.

In `@src/tools/bluebubbles_send_attachment.rs`:
- Around line 175-211: The current flow calls self.security.record_action()
before validating the payload, causing invalid requests to consume quota; move
the record_action() invocation from its current position (before
base64::Engine::decode) to just prior to the outbound HTTP send (immediately
before the reqwest client .send() call that submits the multipart built with
reqwest::multipart::Part::bytes and mime_str), so base64::Engine::decode and the
reqwest::multipart::Part creation/mime validation run first and only successful,
fully validated requests decrement the rate limit.

---

Nitpick comments:
In `@src/channels/transcription.rs`:
- Around line 478-482: The test resolve_whisper_bin_returns_str_or_none
currently only ensures no panic; change it to assert a real invariant by
matching on the result of resolve_whisper_bin(): if it returns Some(path) assert
the Path exists and is a file (e.g. path.exists() && path.is_file()), otherwise
allow None; keep the test name resolve_whisper_bin_returns_str_or_none and use
resolve_whisper_bin() as the unique symbol to locate and update the assertion
logic so regressions where the function returns an invalid path will fail the
test.

In `@src/tools/bluebubbles_send_attachment.rs`:
- Around line 154-221: Trim the caption input immediately after extraction so
whitespace-only captions are treated as empty: when you build caption from
args.get("caption") update it to be trimmed (e.g., use .trim()/trimmed string)
and then use that trimmed value in the subsequent emptiness check and when
adding the "message" field to the multipart form (replace uses of caption in the
if !caption.is_empty() branch and form.text("message", caption) with the trimmed
caption variable).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 562fd4f1-65ba-46a0-a9ea-1652b312f01c

📥 Commits

Reviewing files that changed from the base of the PR and between 86c512b and aa806a2.

📒 Files selected for processing (7)

• docs/i18n/el/channels-reference.md
• docs/i18n/vi/channels-reference.md
• src/channels/transcription.rs
• src/gateway/mod.rs
• src/gateway/oauth.rs
• src/tools/bluebubbles_group.rs
• src/tools/bluebubbles_send_attachment.rs

🚧 Files skipped from review as they are similar to previous changes (2)

• docs/i18n/vi/channels-reference.md
• src/gateway/oauth.rs

[maxtongwang]

Closing — no longer needed.