feat(channel/bluebubbles): dm_policy, group_policy, and send_read_receipts

[maxtongwang]

Summary

• Add dm_policy and group_policy config fields to [channels_config.bluebubbles] with open, allowlist, and block variants
• Add send_read_receipts bool (default true) to control iMessage read-receipt delivery
• Add group_allow_from allowlist for group_policy = "allowlist" mode
• Extend OAuth module (src/gateway/oauth.rs) with Google OAuth flow, PKCE, token storage, and revocation
• Add operational runbook and troubleshooting docs for BlueBubbles, with VI locale parity

Label Snapshot

Label	Value
size	XL
risk	high
scope	channel, config, gateway, docs

Change Metadata

Field	Value
Linear key	external contributor
Scope	src/channels/bluebubbles.rs, src/config/schema.rs, src/gateway/mod.rs, src/gateway/oauth.rs, docs
Compatibility	Additive — new optional config fields with safe defaults
Breaking	No

Linked Issue

No upstream Linear key — external contributor.

Validation Evidence

• cargo fmt --all -- --check: pass
• cargo clippy --all-targets -- -D warnings: pass
• cargo test: pass
• ./scripts/ci/rust_quality_gate.sh: pass
• ./scripts/ci/rust_strict_delta_gate.sh: pass
• ./scripts/ci/docs_quality_gate.sh: pass

Security Impact

• dm_policy / group_policy gate inbound message processing — deny-by-default paths added
• OAuth PKCE flow: session files use mode(0o600), directory mode(0o700); token revocation on logout
• send_read_receipts: no security impact; controlled by operator config
• allowed_senders / group_allow_from validated at startup — empty allowlist with allowlist policy is a startup error

Privacy and Data Hygiene

• Raw sender identifiers removed from debug/warn logs
• Chat GUIDs redacted from mark_read logs
• No PII in test fixtures or commit messages

Compatibility

• All new config fields have safe defaults (dm_policy = "open", group_policy = "open", send_read_receipts = true)
• Existing deployments continue to work without config changes
• oauth.rs is a new module; no existing behavior changed

i18n Follow-Through

• VI channels-reference.md and docs/README.md updated for BlueBubbles §4.15 parity
• EL channels-reference.md updated
• Deferred: Translation of the new "BlueBubbles: webhook not receiving messages" troubleshooting section (docs/troubleshooting.md lines 185-211) to zh-CN, ja, ru, fr, vi, el is deferred to a follow-up PR. Content is ops/troubleshooting only and does not block functionality. Follow-up will add translated copies per docs/i18n-guide.md checklist item 3.

Human Verification

• dm_policy / group_policy enforcement tested with actual BlueBubbles server
• OAuth flow tested end-to-end (authorize → callback → token stored → revoke)
• send_read_receipts = false disables read receipts in group chats

Side Effects

• oauth.rs adds /auth/google, /auth/google/callback, /auth/status, /auth/revoke routes to the gateway router
• New config fields add startup validation — misconfigured allowlist policy with empty list is now a hard startup error

Agent Notes

• Multiple CodeRabbit review rounds completed (Rounds 1–11)
• claude-review.yml workflow removed from branch (fork-only, not for upstream)
• cargo fmt --all applied after each fix round

Rollback Plan

Revert the PR. All changes are additive/isolated — no data migrations required.

Risks

• risk: high label — gateway + security surface (OAuth, policy enforcement)
• OAuth PKCE state file is per-service (not per-request) in this PR — concurrent /auth/google starts could race; acceptable for current usage (single-user deployment)

Review Fixes (Round 1–11)

Multiple rounds of CodeRabbit findings addressed including: OAuth security hardening, PII removal from logs, allowlist validation, CSRF state token, token revocation, chunking docs, format fixes, VI nav parity, troubleshooting section, and runbook additions.

Review Fixes (i18n Deferral)

i18n follow-through for BlueBubbles webhook troubleshooting section (docs/troubleshooting.md lines 185-211):

The English section "BlueBubbles: webhook not receiving messages" added in this PR has not been translated to the supported locales (zh-CN, ja, ru, fr, vi, el).

Deferral: Translation deferred to a follow-up PR. Content is troubleshooting/ops and does not block functionality. Follow-up will add translated copies per docs/i18n-guide.md checklist item 3.

Summary by CodeRabbit

Release Notes

• New Features

  • BlueBubbles (iMessage) channel support with configurable message policies and read receipts.
  • Audio transcription for BlueBubbles messages.
  • Message reaction/tapback support.
  • Google OAuth2 authentication integration.

• Improvements

  • Enhanced HTTP redirect handling in web requests.

• Documentation

  • Comprehensive BlueBubbles integration guides and troubleshooting.
  • Expanded channel configuration and operational documentation.

[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 (5)

src/gateway/mod.rs (3)

2377-2378: ⚠️ Potential issue | 🟡 Minor

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

Line 2377 says “Returning 200 immediately” while the handler returns StatusCode::ACCEPTED at Line 2485.

🤖 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, Update the stale comment that
says "Returning 200 immediately" so it correctly reflects the actual status
being returned (StatusCode::ACCEPTED / 202); locate the comment near the handler
that ultimately returns StatusCode::ACCEPTED and change the wording to mention
202/ACCEPTED and that the request is accepted for background processing,
ensuring the comment matches the code's behavior.

---

2366-2374: ⚠️ Potential issue | 🟠 Major

Add boundary tests for Accepted vs exhausted worker-pool paths.

The new semaphore branch introduces a critical boundary (202 accepted vs 503 exhausted), but there’s no corresponding validation in the provided tests for this handler behavior.

Based on learnings: "Applies to src/gateway/**/*.rs : Include threat/risk notes and rollback strategy. Add/update tests or validation evidence for failure modes and boundaries."

Also applies to: 2484-2487

🤖 Prompt for AI Agents

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

In `@src/gateway/mod.rs` around lines 2366 - 2374, Add unit/integration tests for
the handler that acquires BB_WORKER_SEMAPHORE to assert both success (when
try_acquire_owned returns Ok) yields StatusCode::ACCEPTED (202) and failure
(when semaphore is exhausted / try_acquire_owned returns Err) yields
StatusCode::SERVICE_UNAVAILABLE (503) with the JSON error body; mock or
configure BB_WORKER_SEMAPHORE to a controllable semaphore in tests, exercise the
code path that calls Arc::clone(&*BB_WORKER_SEMAPHORE).try_acquire_owned() (the
handler/function containing that match) for both scenarios, and include
assertions for response status and body; add a test case documenting the failure
mode and a brief rollback note in comments for reverting the change if boundary
behavior regresses.

---

585-591: ⚠️ Potential issue | 🟠 Major

Don’t silently disable BlueBubbles on init failure.

Line 590 converts initialization errors into None, which hides misconfiguration and weakens fail-fast startup behavior.

Suggested fix

-    let bluebubbles_channel: Option<Arc<BlueBubblesChannel>> = config
+    let bluebubbles_channel: Option<Arc<BlueBubblesChannel>> = config
         .channels_config
         .bluebubbles
         .as_ref()
         .map(|bb| {
             BlueBubblesChannel::new(
                 bb.server_url.clone(),
                 bb.password.clone(),
                 bb.allowed_senders.clone(),
                 bb.ignore_senders.clone(),
             )
             .map(|ch| {
                 Arc::new(
                     ch.with_policies(
                         bb.dm_policy,
                         bb.group_policy,
                         bb.group_allow_from.clone(),
                         bb.send_read_receipts,
                     )
                     .with_transcription(config.transcription.clone()),
                 )
             })
         })
         .transpose()
         .map_err(|e| {
             tracing::error!("Failed to initialize BlueBubbles channel: {e}");
             e
-        })
-        .ok()
-        .flatten();
+        })?;

As per coding guidelines: "Prefer explicit bail!/errors for unsupported or unsafe states; never silently broaden permissions/capabilities (Fail Fast principle)".

🤖 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 current chain ending with
`.transpose().map_err(|e| { tracing::error!("Failed to initialize BlueBubbles
channel: {e}"); e }).ok().flatten()` swallows initialization failures by
converting errors to None; instead propagate the error (fail-fast). Remove the
`.ok().flatten()` and return/propagate the error from the `map_err` (or replace
`map_err` with a `map_err(|e| { tracing::error!(...); anyhow::bail!(...); })` or
simply use `...?` on the result) so the calling function receives a Result::Err;
keep the trace log but do not convert the error into None. Ensure the enclosing
function's signature returns Result and update callers if necessary.

src/channels/bluebubbles.rs (2)

271-287: ⚠️ Potential issue | 🟠 Major

Enforce allowed_senders in group Allowlist mode to avoid permission widening.

BlueBubblesGroupPolicy::Allowlist currently checks only group_allow_from, so configured sender restrictions are bypassed in that mode. This broadens access relative to existing sender-gated behavior.

🔧 Suggested fix

 fn is_group_allowed(&self, chat_guid: &str, sender: &str) -> bool {
+    let sender_allowed = self.is_sender_allowed(sender);
     match self.group_policy {
         BlueBubblesGroupPolicy::Disabled => false,
         BlueBubblesGroupPolicy::Allowlist => self
             .group_allow_from
             .iter()
-            .any(|g| g == "*" || g.eq_ignore_ascii_case(chat_guid)),
+            .any(|g| g == "*" || g.eq_ignore_ascii_case(chat_guid))
+            && sender_allowed,
         BlueBubblesGroupPolicy::Open => {
-            if self.allowed_senders.is_empty() {
-                true
-            } else {
-                self.allowed_senders
-                    .iter()
-                    .any(|a| a == "*" || a.eq_ignore_ascii_case(sender))
-            }
+            sender_allowed
         }
     }
 }

As per coding guidelines: “Do not silently weaken security policy or access constraints; never silently broaden permissions/capabilities.”

🤖 Prompt for AI Agents

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

In `@src/channels/bluebubbles.rs` around lines 271 - 287, is_group_allowed's
BlueBubblesGroupPolicy::Allowlist branch currently only checks group_allow_from
and thus ignores allowed_senders; change the Allowlist branch to require both
that the chat_guid matches group_allow_from AND that the sender is permitted by
allowed_senders (treating an empty allowed_senders as "no sender restriction"
same as Open). In other words, inside is_group_allowed, update the Allowlist arm
to return self.group_allow_from.iter().any(... for chat_guid) &&
(self.allowed_senders.is_empty() || self.allowed_senders.iter().any(... for
sender)) so both group and sender checks are enforced.

---

303-339: ⚠️ Potential issue | 🟠 Major

mark_read blocks despite the “fire-and-forget” contract.

mark_read awaits the HTTP call, so BB slowness can delay the caller up to the request timeout. If non-blocking is intended, dispatch the POST in a detached task (or enforce a short per-request timeout).

🤖 Prompt for AI Agents

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

In `@src/channels/bluebubbles.rs` around lines 303 - 339, The mark_read function
currently awaits the HTTP POST which violates the intended “fire-and-forget”
behavior; change it to dispatch the POST in a detached task so it won’t block
the caller (or, alternatively, wrap the request in a short
tokio::time::timeout). Concretely, move the request logic that builds the URL
and makes client.post(...).query(...).send().await into a tokio::spawn async
move closure (cloning self.client, self.password and the computed
url/encoded/chat_guid as needed) and perform the same logging inside that
spawned task; if you prefer a safety bound, apply tokio::time::timeout around
the send() inside the task to enforce a short per-request timeout. Ensure
mark_read returns immediately after spawning the task.

🤖 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 271-287: is_group_allowed's BlueBubblesGroupPolicy::Allowlist
branch currently only checks group_allow_from and thus ignores allowed_senders;
change the Allowlist branch to require both that the chat_guid matches
group_allow_from AND that the sender is permitted by allowed_senders (treating
an empty allowed_senders as "no sender restriction" same as Open). In other
words, inside is_group_allowed, update the Allowlist arm to return
self.group_allow_from.iter().any(... for chat_guid) &&
(self.allowed_senders.is_empty() || self.allowed_senders.iter().any(... for
sender)) so both group and sender checks are enforced.
- Around line 303-339: The mark_read function currently awaits the HTTP POST
which violates the intended “fire-and-forget” behavior; change it to dispatch
the POST in a detached task so it won’t block the caller (or, alternatively,
wrap the request in a short tokio::time::timeout). Concretely, move the request
logic that builds the URL and makes client.post(...).query(...).send().await
into a tokio::spawn async move closure (cloning self.client, self.password and
the computed url/encoded/chat_guid as needed) and perform the same logging
inside that spawned task; if you prefer a safety bound, apply
tokio::time::timeout around the send() inside the task to enforce a short
per-request timeout. Ensure mark_read returns immediately after spawning the
task.

In `@src/gateway/mod.rs`:
- Around line 2377-2378: Update the stale comment that says "Returning 200
immediately" so it correctly reflects the actual status being returned
(StatusCode::ACCEPTED / 202); locate the comment near the handler that
ultimately returns StatusCode::ACCEPTED and change the wording to mention
202/ACCEPTED and that the request is accepted for background processing,
ensuring the comment matches the code's behavior.
- Around line 2366-2374: Add unit/integration tests for the handler that
acquires BB_WORKER_SEMAPHORE to assert both success (when try_acquire_owned
returns Ok) yields StatusCode::ACCEPTED (202) and failure (when semaphore is
exhausted / try_acquire_owned returns Err) yields
StatusCode::SERVICE_UNAVAILABLE (503) with the JSON error body; mock or
configure BB_WORKER_SEMAPHORE to a controllable semaphore in tests, exercise the
code path that calls Arc::clone(&*BB_WORKER_SEMAPHORE).try_acquire_owned() (the
handler/function containing that match) for both scenarios, and include
assertions for response status and body; add a test case documenting the failure
mode and a brief rollback note in comments for reverting the change if boundary
behavior regresses.
- Around line 585-591: The current chain ending with `.transpose().map_err(|e| {
tracing::error!("Failed to initialize BlueBubbles channel: {e}"); e
}).ok().flatten()` swallows initialization failures by converting errors to
None; instead propagate the error (fail-fast). Remove the `.ok().flatten()` and
return/propagate the error from the `map_err` (or replace `map_err` with a
`map_err(|e| { tracing::error!(...); anyhow::bail!(...); })` or simply use
`...?` on the result) so the calling function receives a Result::Err; keep the
trace log but do not convert the error into None. Ensure the enclosing
function's signature returns Result and update callers if necessary.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2732a11 and b065e69.

📒 Files selected for processing (2)

• src/channels/bluebubbles.rs
• src/gateway/mod.rs

[maxtongwang]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

Actionable comments posted: 4

♻️ Duplicate comments (12)

docs/i18n/el/channels-reference.md (1)

40-50: ⚠️ Potential issue | 🟠 Major

Split native iMessage and BlueBubbles rows in the EL delivery table.

At Line 49, iMessage is currently documented as BlueBubbles webhook mode, which misstates native iMessage behavior. Please separate them into distinct rows (native AppleScript bridge vs BlueBubbles webhook) to keep receive-mode and inbound-port semantics accurate.

Based on learnings: "Applies to docs/channels-reference.md : Treat as runtime-contract reference. Track behavior changes to channel integrations. Update corresponding localized docs for supported locales when wording changes."

🤖 Prompt for AI Agents

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

In `@docs/i18n/el/channels-reference.md` around lines 40 - 50, The EL docs table
incorrectly conflates native iMessage and BlueBubbles into one row; split the
single "iMessage" row into two rows: one for native iMessage (describe mechanism
as "AppleScript bridge" or "Native bridge", set Port Forwarding to "Όχι" and
appropriate receive-mode semantics) and a separate "BlueBubbles" row (mechanism
"BlueBubbles webhook (`/bluebubbles`)", Port Forwarding "Ναι" and note that the
BlueBubbles server must be reachable). Update the table header alignment and
ensure the new rows preserve the same column order and formatting as the other
rows (use the unique labels "iMessage" and "BlueBubbles webhook
(`/bluebubbles`)").

docs/channels-reference.md (1)

555-593: ⚠️ Potential issue | 🟠 Major

BlueBubbles runtime-contract expansion still needs full locale follow-through.

This section adds new shared user-facing contract content (dm_policy, group_policy, group_allow_from, send_read_receipts) but the update set shown here does not cover all supported locales. Please sync remaining localized channel references (or explicitly document deferral with a tracked follow-up).

As per coding guidelines, "If a change touches docs IA, runtime-contract references, or user-facing wording in shared docs, perform i18n follow-through for currently supported locales (en, zh-CN, ja, ru, fr, vi, el) in the same PR."

🤖 Prompt for AI Agents

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

In `@docs/channels-reference.md` around lines 555 - 593, The BlueBubbles channel
docs add new runtime-contract fields (dm_policy, group_policy, group_allow_from,
send_read_receipts) but the PR only updates the English doc; update the
corresponding localized channel reference files for all supported locales (en,
zh-CN, ja, ru, fr, vi, el) to include the new fields and policy table for
BlueBubbles, or if you cannot update them now, add a clear note in each locale
file pointing to a tracked follow-up issue/PR number that defers i18n
follow-through; ensure you update the same section/title used for BlueBubbles so
the runtime-contract wording is consistent across locales.

docs/i18n/vi/channels-reference.md (1)

118-118: ⚠️ Potential issue | 🟠 Major

BlueBubbles runtime-contract coverage in VI doc is still incomplete.

Line 118 adds BlueBubbles to the delivery table, but the VI reference still lacks matching BlueBubbles policy/config detail and a BlueBubbles diagnostics keyword row in the appendix table. Please sync this section with the EN runtime-contract coverage so behavior and triage docs stay consistent.

Based on learnings: "Applies to docs/channels-reference.md : Treat as runtime-contract reference. Track behavior changes to channel integrations. Update corresponding localized docs for supported locales when wording changes."

Also applies to: 415-430

🤖 Prompt for AI Agents

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

In `@docs/i18n/vi/channels-reference.md` at line 118, The Vietnamese
channels-reference entry added "BlueBubbles" to the delivery table but lacks the
corresponding runtime-contract policy/config details and the diagnostics keyword
row present in the English runtime-contract docs; update the VI
channels-reference.md by copying the BlueBubbles policy/config paragraph and the
diagnostics keyword row from the EN runtime-contract coverage
(channels-reference.md EN version) into the Vietnamese document, ensure the
delivery table entry text matches the EN wording and add a localized diagnostics
keyword row in the appendix table so behavior, triage, and configuration
guidance are consistent across locales.

src/channels/transcription.rs (1)

90-93: ⚠️ Potential issue | 🟠 Major

ffmpeg timeout/spawn failures can leak temporary WAV files.

At Line 90-93, timeout/spawn errors return early via ? before cleanup of tmp, so conversion artifacts can accumulate on failure paths.

♻️ Cleanup-safe conversion handling

-        let conv = tokio::time::timeout(std::time::Duration::from_secs(120), ffmpeg_cmd.output())
-            .await
-            .map_err(|_| anyhow::anyhow!("ffmpeg CAF→WAV conversion timed out after 120s"))?
-            .context("ffmpeg CAF→WAV conversion failed")?;
+        let conv = match tokio::time::timeout(std::time::Duration::from_secs(120), ffmpeg_cmd.output()).await {
+            Ok(Ok(o)) => o,
+            Ok(Err(e)) => {
+                let _ = tokio::fs::remove_file(&tmp).await;
+                return Err(anyhow::anyhow!("ffmpeg CAF→WAV conversion failed: {e}"));
+            }
+            Err(_) => {
+                let _ = tokio::fs::remove_file(&tmp).await;
+                anyhow::bail!("ffmpeg CAF→WAV conversion timed out after 120s");
+            }
+        };

Based on learnings: "Applies to **/*.rs : Keep error paths obvious and localized; prefer explicit bail!/errors for unsupported or unsafe states."

🤖 Prompt for AI Agents

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

In `@src/channels/transcription.rs` around lines 90 - 93, The ffmpeg timeout/spawn
failure path can return early and leak the temporary WAV file `tmp`; change the
`tokio::time::timeout(...).await.map_err(...)?` usage around
`ffmpeg_cmd.output()` so errors always run cleanup: either use a scoped
match/if-let to handle the timeout/error (capturing the `Output` or error into
`conv`), remove the `tmp` file (or rely on a `tempfile::NamedTempFile`/RAII
wrapper) before returning/bailing, and ensure `tmp` is deleted in every error
branch (timeout, spawn failure, and conversion failure) so `conv`, `ffmpeg_cmd`,
and `tmp` cleanup is explicit and not bypassed by `?`.

src/gateway/mod.rs (3)

2377-2379: ⚠️ Potential issue | 🟡 Minor

Stale inline comment: status code says 200 but handler returns 202.

Please align the comment text with the actual StatusCode::ACCEPTED response.

🤖 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 - 2379, The inline comment above the
background-task spawn is stale: it references returning HTTP 200 but the handler
actually returns StatusCode::ACCEPTED (202). Update the comment text near the
Arc::clone(bluebubbles) / background task block to mention 202
(StatusCode::ACCEPTED) instead of 200 so the comment matches the actual response
code and intent.

---

562-591: ⚠️ Potential issue | 🟠 Major

BlueBubbles init failures are swallowed and silently disable the channel.

The .ok().flatten() conversion drops initialization errors after logging, which weakens fail-fast startup behavior for a configured integration.

🔧 Propagate initialization failures

     let bluebubbles_channel: Option<Arc<BlueBubblesChannel>> = config
         .channels_config
         .bluebubbles
         .as_ref()
         .map(|bb| {
             BlueBubblesChannel::new(
                 bb.server_url.clone(),
                 bb.password.clone(),
                 bb.allowed_senders.clone(),
                 bb.ignore_senders.clone(),
             )
             .map(|ch| {
                 Arc::new(
                     ch.with_policies(
                         bb.dm_policy,
                         bb.group_policy,
                         bb.group_allow_from.clone(),
                         bb.send_read_receipts,
                     )
                     .with_transcription(config.transcription.clone()),
                 )
             })
         })
         .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; document fallback behavior when fallback is intentional and safe”.

🤖 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 code currently logs
BlueBubblesChannel initialization errors but then swallows them via
.ok().flatten(), silently disabling the channel; change the chain to propagate
the error instead. Replace the trailing .ok().flatten() with a propagated Result
by using the existing .transpose().map_err(|e| { tracing::error!( "Failed to
initialize BlueBubbles channel: {e}"); e })? so the function returns an Err on
failure; keep the use of BlueBubblesChannel::new, ch.with_policies and
.with_transcription and ensure bluebubbles_channel remains an
Option<Arc<BlueBubblesChannel>> after the ? propagation.

---

2358-2487: ⚠️ Potential issue | 🟠 Major

Please add direct tests for 202 accepted vs 503 saturation behavior.

This new async boundary is critical: one test should assert 202 + {"status":"accepted"} when a permit exists, and another should assert 503 + {"error":"worker pool exhausted, retry later"} when permits are exhausted.

Based on learnings: “Applies to src/gateway/**/*.rs : Include threat/risk notes and rollback strategy. Add/update tests or validation evidence for failure modes and boundaries. Keep observability useful but non-sensitive.”

🤖 Prompt for AI Agents

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

In `@src/gateway/mod.rs` around lines 2358 - 2487, Add two direct tests around the
BlueBubbles webhook path that exercise the try_acquire_owned branch on
BB_WORKER_SEMAPHORE: one test (e.g.,
test_bluebubbles_returns_accepted_when_permit_exists) should ensure a permit is
available (ensure BB_WORKER_SEMAPHORE has at least one free permit), invoke the
webhook handler with a minimal payload, and assert the HTTP response is
StatusCode::ACCEPTED and body JSON {"status":"accepted"}; the other test (e.g.,
test_bluebubbles_returns_503_when_semaphore_exhausted) should exhaust
BB_WORKER_SEMAPHORE (by acquiring all permits or otherwise blocking
try_acquire_owned), invoke the same handler, and assert
StatusCode::SERVICE_UNAVAILABLE and JSON {"error":"worker pool exhausted, retry
later"}; make sure tests cleanup/release any acquired permits so other tests are
unaffected and reference the BB_WORKER_SEMAPHORE, try_acquire_owned, and the
handler invocation that returns the ACCEPTED/SERVICE_UNAVAILABLE pair.

src/gateway/oauth.rs (5)

557-561: ⚠️ Potential issue | 🟠 Major

Revocation transport failures are still silently swallowed.

let _ = revoke_google_token(...) drops network/TLS errors entirely, so the endpoint can report success while remote revocation never happened.

🔧 Minimal hardening patch

     if service == "google" {
         if let Some(token) = read_token(&path).await {
-            let _ = revoke_google_token(&token.access_token).await;
+            if let Err(e) = revoke_google_token(&token.access_token).await {
+                tracing::warn!("Google token revoke request failed before local cleanup: {e}");
+            }
         }
     }

🤖 Prompt for AI Agents

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

In `@src/gateway/oauth.rs` around lines 557 - 561, The revocation call currently
swallows all errors via `let _ =
revoke_google_token(&token.access_token).await;`; update the `if service ==
"google"` block that uses `read_token(&path).await` to handle the Result from
`revoke_google_token` instead of discarding it: await the call into a `match` or
`if let Err(e)` and either log the error (including `e` and context like
`path`/`service`) or propagate it to the caller so transport/TLS failures are
not ignored; ensure you reference `revoke_google_token` and `read_token` in the
change and do not lose the original success path.

---

116-127: ⚠️ Potential issue | 🟡 Minor

callback_url hardcodes http://, which breaks HTTPS-exposed OAuth deployments.

When the gateway is exposed via an HTTPS tunnel/proxy, this can produce redirect_uri mismatches with provider settings.

🤖 Prompt for AI Agents

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

In `@src/gateway/oauth.rs` around lines 116 - 127, The callback_url function
currently hardcodes "http://" causing redirect_uri mismatches for HTTPS
deployments; update callback_url (involving AppState and callback_url) to derive
the URL scheme from configuration instead of hardcoding: read a scheme field
(e.g., cfg.gateway.scheme) or a TLS flag (e.g., cfg.gateway.tls_enabled) from
cfg and set scheme = "https" when TLS is enabled (fallback to "http" otherwise),
keep the existing display_host logic for localhost normalization, and format the
URL as "{scheme}://{display_host}:{port}/auth/{service}/callback" so
HTTPS-exposed gateways produce correct redirect_uri values.

---

76-92: ⚠️ Potential issue | 🟠 Major

Pre-existing OAuth directory/token files may keep permissive Unix permissions.

mode(0o700) / mode(0o600) protects create-time only. If the path already exists with broader permissions, this code path does not re-harden it.

#!/bin/bash
# Verify whether oauth path hardening includes post-create chmod for existing paths.
# Expected: explicit set_permissions logic near ensure_oauth_dir/write_token.
rg -n -C4 'async fn ensure_oauth_dir|mode\(0o700\)|set_permissions|async fn write_token|mode\(0o600\)' src/gateway/oauth.rs

As per coding guidelines: src/{gateway,security,tools,runtime}/**/*.rs — “Deny-by-default for access and exposure boundaries; never log secrets, raw tokens, or sensitive payloads; keep network/filesystem/shell scope as narrow as possible unless explicitly justified”.

Also applies to: 99-113

🤖 Prompt for AI Agents

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

In `@src/gateway/oauth.rs` around lines 76 - 92, ensure_oauth_dir currently only
sets mode during creation, so pre-existing directories with permissive
permissions are not re-hardened; update ensure_oauth_dir to always enforce
restrictive permissions on the path after creation (and on errors when path
already exists) by applying std::fs::set_permissions (or set_permissions via
PermissionsExt on unix to 0o700) inside tokio::task::spawn_blocking, and
likewise ensure write_token enforces file permissions (0o600) after writing the
token (again using spawn_blocking and PermissionsExt on unix); locate and modify
the functions ensure_oauth_dir and write_token to add these explicit post-create
chmod steps so existing files/dirs are hardened regardless of creation path.

---

129-132: ⚠️ Potential issue | 🟠 Major

OAuth management endpoints are effectively unauthenticated when pairing is off.

auth_check currently returns true when pairing is disabled, which opens /auth/{service}, /auth/status, and DELETE /auth/{service} without any auth boundary.

As per coding guidelines: src/{gateway,security,tools,runtime}/**/*.rs — “Deny-by-default for access and exposure boundaries; never log secrets, raw tokens, or sensitive payloads; keep network/filesystem/shell scope as narrow as possible unless explicitly justified”.

🤖 Prompt for AI Agents

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

In `@src/gateway/oauth.rs` around lines 129 - 132, auth_check currently returns
true when state.pairing.require_pairing() is false, effectively disabling auth;
change auth_check so it is deny-by-default: if pairing is enabled keep current
behavior, but when pairing is disabled do NOT return true unconditionally —
instead validate the incoming headers using the service's normal auth validation
path (e.g., call the existing token/session validation routine or an AppState
method that verifies auth headers) and only return true when that validation
succeeds; update auth_check (and any callers of auth_check) to reference
AppState’s proper auth validator rather than short-circuiting on
pairing.require_pairing().

---

705-738: ⚠️ Potential issue | 🟠 Major

Security-critical handler failure boundaries still need direct tests.

Current coverage is helper-level only; please add handler-level tests for unauthorized management access, missing/mismatched OAuth state, and revoke failure behavior.

Based on learnings: “Applies to src/gateway/**/*.rs : Include threat/risk notes and rollback strategy. Add/update tests or validation evidence for failure modes and boundaries. Keep observability useful but non-sensitive.”

🤖 Prompt for AI Agents

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

In `@src/gateway/oauth.rs` around lines 705 - 738, The repo lacks handler-level
tests for failure boundaries: add tests that exercise the management access
handler, the OAuth callback state validation, and the revoke flow to cover
unauthorized access, missing/mismatched state, and revoke errors. Specifically,
add tests that call the management handler (e.g., handle_manage or
manage_authorization) with an unauthenticated/unauthorized request and assert
the handler returns the expected 401/403 and does not perform privileged
actions; call the OAuth callback handler (e.g., oauth_callback or
handle_oauth_callback) with missing and with mismatched state values and assert
it rejects the request and does not create sessions; and mock the revocation
backend for the revoke handler (e.g., revoke_token or handle_revoke) to return
an error and assert the handler surfaces the failure, performs any defined
rollback/cleanup, and emits the expected observable logs/metrics. Ensure tests
use the same public interfaces/handlers in src/gateway/oauth.rs, mock external
dependencies (token store, HTTP client), and assert both HTTP response codes and
side-effect (no session persisted, no management change) to validate boundaries.

🧹 Nitpick comments (1)

src/config/schema.rs (1)

14325-14369: Add whitespace-only allowlist regression tests.

Current tests assert empty vectors, but Line 7635 and Line 7643 also reject whitespace-only entries. Adding explicit vec![" "] cases for both DM and group allowlists would lock that branch behavior.

🧪 Suggested test additions

+    #[test]
+    async fn validation_rejects_bluebubbles_dm_allowlist_with_whitespace_only_senders() {
+        let mut config = Config::default();
+        config.channels_config.bluebubbles = Some(BlueBubblesConfig {
+            server_url: "http://localhost:1234".to_string(),
+            password: String::new(),
+            allowed_senders: vec!["   ".to_string()],
+            webhook_secret: None,
+            ignore_senders: vec![],
+            dm_policy: BlueBubblesDmPolicy::Allowlist,
+            group_policy: BlueBubblesGroupPolicy::Open,
+            group_allow_from: vec![],
+            send_read_receipts: true,
+        });
+        let err = config.validate().expect_err(
+            "allowlist dm_policy with whitespace-only allowed_senders should fail validation",
+        );
+        assert!(err.to_string().contains("dm_policy"));
+    }
+
+    #[test]
+    async fn validation_rejects_bluebubbles_group_allowlist_with_whitespace_only_entries() {
+        let mut config = Config::default();
+        config.channels_config.bluebubbles = Some(BlueBubblesConfig {
+            server_url: "http://localhost:1234".to_string(),
+            password: String::new(),
+            allowed_senders: vec![],
+            webhook_secret: None,
+            ignore_senders: vec![],
+            dm_policy: BlueBubblesDmPolicy::Open,
+            group_policy: BlueBubblesGroupPolicy::Allowlist,
+            group_allow_from: vec!["   ".to_string()],
+            send_read_receipts: true,
+        });
+        let err = config.validate().expect_err(
+            "allowlist group_policy with whitespace-only group_allow_from should fail validation",
+        );
+        assert!(err.to_string().contains("group_policy"));
+    }

🤖 Prompt for AI Agents

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

In `@src/config/schema.rs` around lines 14325 - 14369, Add two regression tests
mirroring validation_rejects_bluebubbles_dm_allowlist_with_empty_senders and
validation_rejects_bluebubbles_group_allowlist_with_empty_list but use
whitespace-only entries (e.g., allowed_senders: vec!["   "] and
group_allow_from: vec!["   "]) in the BlueBubblesConfig; call Config::default(),
set channels_config.bluebubbles with those whitespace-only vectors, call
config.validate().expect_err(...), and assert the error string contains
"dm_policy" for the DM test and "group_policy" for the group test to ensure
whitespace-only allowlist entries are rejected the same as empty vectors.

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@docs/config-reference.md`:
- Line 1247: Update the `send_read_receipts` config description so it reflects
actual runtime behavior: instead of saying receipts are posted "after each
processed message", state that read receipts are sent only after a successful
reply is sent (i.e., when the outbound response completes successfully). Locate
the `send_read_receipts` entry in the config reference and change the wording to
mention "after successful replies" and keep the default value (`true`)
unchanged.

In `@src/channels/bluebubbles.rs`:
- Around line 818-821: In the Err(e) branch of the audio transcription flow (the
match arm that currently does tracing::warn!("BB audio transcription failed:
{e}")), avoid logging raw error text which may contain attachment GUIDs or
filenames from transcribe_local / download_attachment_bytes; instead log a
sanitized message and only non-sensitive context (e.g., "BB audio transcription
failed" plus error kind or a scrubbed e.to_string() with GUID/filename patterns
removed) and then continue to call self.parse_webhook_payload(payload); update
the tracing::warn call in that Err(e) arm to use the sanitized/error-kind form
rather than printing the full error chain.

In `@src/channels/transcription.rs`:
- Around line 114-119: The owner-only permission hardening currently swallows
errors for the set_permissions call on out_dir and is not applied to the second
temp directory; change the unix-only permission block so the
tokio::fs::set_permissions(&out_dir, Permissions::from_mode(0o700)).await result
is checked and propagated (use anyhow::bail! or propagate the Err) instead of
ignoring it, apply the same Permissions::from_mode(0o700) hardening to the other
temp directory created later (the second temp dir variable), and make this
fail-closed on non-unix targets (return an explicit error or bail! if permission
hardening cannot be enforced) so neither out_dir nor the later temp directory
continues with broader access when hardening fails.

---

Duplicate comments:
In `@docs/channels-reference.md`:
- Around line 555-593: The BlueBubbles channel docs add new runtime-contract
fields (dm_policy, group_policy, group_allow_from, send_read_receipts) but the
PR only updates the English doc; update the corresponding localized channel
reference files for all supported locales (en, zh-CN, ja, ru, fr, vi, el) to
include the new fields and policy table for BlueBubbles, or if you cannot update
them now, add a clear note in each locale file pointing to a tracked follow-up
issue/PR number that defers i18n follow-through; ensure you update the same
section/title used for BlueBubbles so the runtime-contract wording is consistent
across locales.

In `@docs/i18n/el/channels-reference.md`:
- Around line 40-50: The EL docs table incorrectly conflates native iMessage and
BlueBubbles into one row; split the single "iMessage" row into two rows: one for
native iMessage (describe mechanism as "AppleScript bridge" or "Native bridge",
set Port Forwarding to "Όχι" and appropriate receive-mode semantics) and a
separate "BlueBubbles" row (mechanism "BlueBubbles webhook (`/bluebubbles`)",
Port Forwarding "Ναι" and note that the BlueBubbles server must be reachable).
Update the table header alignment and ensure the new rows preserve the same
column order and formatting as the other rows (use the unique labels "iMessage"
and "BlueBubbles webhook (`/bluebubbles`)").

In `@docs/i18n/vi/channels-reference.md`:
- Line 118: The Vietnamese channels-reference entry added "BlueBubbles" to the
delivery table but lacks the corresponding runtime-contract policy/config
details and the diagnostics keyword row present in the English runtime-contract
docs; update the VI channels-reference.md by copying the BlueBubbles
policy/config paragraph and the diagnostics keyword row from the EN
runtime-contract coverage (channels-reference.md EN version) into the Vietnamese
document, ensure the delivery table entry text matches the EN wording and add a
localized diagnostics keyword row in the appendix table so behavior, triage, and
configuration guidance are consistent across locales.

In `@src/channels/transcription.rs`:
- Around line 90-93: The ffmpeg timeout/spawn failure path can return early and
leak the temporary WAV file `tmp`; change the
`tokio::time::timeout(...).await.map_err(...)?` usage around
`ffmpeg_cmd.output()` so errors always run cleanup: either use a scoped
match/if-let to handle the timeout/error (capturing the `Output` or error into
`conv`), remove the `tmp` file (or rely on a `tempfile::NamedTempFile`/RAII
wrapper) before returning/bailing, and ensure `tmp` is deleted in every error
branch (timeout, spawn failure, and conversion failure) so `conv`, `ffmpeg_cmd`,
and `tmp` cleanup is explicit and not bypassed by `?`.

In `@src/gateway/mod.rs`:
- Around line 2377-2379: The inline comment above the background-task spawn is
stale: it references returning HTTP 200 but the handler actually returns
StatusCode::ACCEPTED (202). Update the comment text near the
Arc::clone(bluebubbles) / background task block to mention 202
(StatusCode::ACCEPTED) instead of 200 so the comment matches the actual response
code and intent.
- Around line 562-591: The code currently logs BlueBubblesChannel initialization
errors but then swallows them via .ok().flatten(), silently disabling the
channel; change the chain to propagate the error instead. Replace the trailing
.ok().flatten() with a propagated Result by using the existing
.transpose().map_err(|e| { tracing::error!( "Failed to initialize BlueBubbles
channel: {e}"); e })? so the function returns an Err on failure; keep the use of
BlueBubblesChannel::new, ch.with_policies and .with_transcription and ensure
bluebubbles_channel remains an Option<Arc<BlueBubblesChannel>> after the ?
propagation.
- Around line 2358-2487: Add two direct tests around the BlueBubbles webhook
path that exercise the try_acquire_owned branch on BB_WORKER_SEMAPHORE: one test
(e.g., test_bluebubbles_returns_accepted_when_permit_exists) should ensure a
permit is available (ensure BB_WORKER_SEMAPHORE has at least one free permit),
invoke the webhook handler with a minimal payload, and assert the HTTP response
is StatusCode::ACCEPTED and body JSON {"status":"accepted"}; the other test
(e.g., test_bluebubbles_returns_503_when_semaphore_exhausted) should exhaust
BB_WORKER_SEMAPHORE (by acquiring all permits or otherwise blocking
try_acquire_owned), invoke the same handler, and assert
StatusCode::SERVICE_UNAVAILABLE and JSON {"error":"worker pool exhausted, retry
later"}; make sure tests cleanup/release any acquired permits so other tests are
unaffected and reference the BB_WORKER_SEMAPHORE, try_acquire_owned, and the
handler invocation that returns the ACCEPTED/SERVICE_UNAVAILABLE pair.

In `@src/gateway/oauth.rs`:
- Around line 557-561: The revocation call currently swallows all errors via
`let _ = revoke_google_token(&token.access_token).await;`; update the `if
service == "google"` block that uses `read_token(&path).await` to handle the
Result from `revoke_google_token` instead of discarding it: await the call into
a `match` or `if let Err(e)` and either log the error (including `e` and context
like `path`/`service`) or propagate it to the caller so transport/TLS failures
are not ignored; ensure you reference `revoke_google_token` and `read_token` in
the change and do not lose the original success path.
- Around line 116-127: The callback_url function currently hardcodes "http://"
causing redirect_uri mismatches for HTTPS deployments; update callback_url
(involving AppState and callback_url) to derive the URL scheme from
configuration instead of hardcoding: read a scheme field (e.g.,
cfg.gateway.scheme) or a TLS flag (e.g., cfg.gateway.tls_enabled) from cfg and
set scheme = "https" when TLS is enabled (fallback to "http" otherwise), keep
the existing display_host logic for localhost normalization, and format the URL
as "{scheme}://{display_host}:{port}/auth/{service}/callback" so HTTPS-exposed
gateways produce correct redirect_uri values.
- Around line 76-92: ensure_oauth_dir currently only sets mode during creation,
so pre-existing directories with permissive permissions are not re-hardened;
update ensure_oauth_dir to always enforce restrictive permissions on the path
after creation (and on errors when path already exists) by applying
std::fs::set_permissions (or set_permissions via PermissionsExt on unix to
0o700) inside tokio::task::spawn_blocking, and likewise ensure write_token
enforces file permissions (0o600) after writing the token (again using
spawn_blocking and PermissionsExt on unix); locate and modify the functions
ensure_oauth_dir and write_token to add these explicit post-create chmod steps
so existing files/dirs are hardened regardless of creation path.
- Around line 129-132: auth_check currently returns true when
state.pairing.require_pairing() is false, effectively disabling auth; change
auth_check so it is deny-by-default: if pairing is enabled keep current
behavior, but when pairing is disabled do NOT return true unconditionally —
instead validate the incoming headers using the service's normal auth validation
path (e.g., call the existing token/session validation routine or an AppState
method that verifies auth headers) and only return true when that validation
succeeds; update auth_check (and any callers of auth_check) to reference
AppState’s proper auth validator rather than short-circuiting on
pairing.require_pairing().
- Around line 705-738: The repo lacks handler-level tests for failure
boundaries: add tests that exercise the management access handler, the OAuth
callback state validation, and the revoke flow to cover unauthorized access,
missing/mismatched state, and revoke errors. Specifically, add tests that call
the management handler (e.g., handle_manage or manage_authorization) with an
unauthenticated/unauthorized request and assert the handler returns the expected
401/403 and does not perform privileged actions; call the OAuth callback handler
(e.g., oauth_callback or handle_oauth_callback) with missing and with mismatched
state values and assert it rejects the request and does not create sessions; and
mock the revocation backend for the revoke handler (e.g., revoke_token or
handle_revoke) to return an error and assert the handler surfaces the failure,
performs any defined rollback/cleanup, and emits the expected observable
logs/metrics. Ensure tests use the same public interfaces/handlers in
src/gateway/oauth.rs, mock external dependencies (token store, HTTP client), and
assert both HTTP response codes and side-effect (no session persisted, no
management change) to validate boundaries.

---

Nitpick comments:
In `@src/config/schema.rs`:
- Around line 14325-14369: Add two regression tests mirroring
validation_rejects_bluebubbles_dm_allowlist_with_empty_senders and
validation_rejects_bluebubbles_group_allowlist_with_empty_list but use
whitespace-only entries (e.g., allowed_senders: vec!["   "] and
group_allow_from: vec!["   "]) in the BlueBubblesConfig; call Config::default(),
set channels_config.bluebubbles with those whitespace-only vectors, call
config.validate().expect_err(...), and assert the error string contains
"dm_policy" for the DM test and "group_policy" for the group test to ensure
whitespace-only allowlist entries are rejected the same as empty vectors.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: cd5ccaac-6f20-496b-a262-69e2dbbaff2f

📥 Commits

Reviewing files that changed from the base of the PR and between 10b7838 and b065e69.

📒 Files selected for processing (12)

• Cargo.toml
• docs/actions-source-policy.md
• docs/channels-reference.md
• docs/config-reference.md
• docs/i18n/el/channels-reference.md
• docs/i18n/vi/channels-reference.md
• src/channels/bluebubbles.rs
• src/channels/transcription.rs
• src/config/schema.rs
• src/gateway/mod.rs
• src/gateway/oauth.rs
• src/tools/web_fetch.rs

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Clarify send_read_receipts trigger timing to match runtime behavior.

At Line 1247, the description says receipts are posted after each processed message, but runtime wiring is after successful replies. Tightening this wording avoids false expectations during incident triage.

✏️ Suggested doc correction

-| `send_read_receipts` | Optional | Post read receipt after each processed message. Default: `true`                            |
+| `send_read_receipts` | Optional | Post read receipt after each successful reply. Default: `true`                             |

Based on learnings: "Applies to docs/config-reference.md : Treat as runtime-contract reference. Track behavior changes to configuration schema."

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	| `send_read_receipts` | Optional | Post read receipt after each processed message. Default: `true` |
	| `send_read_receipts` | Optional | Post read receipt after each successful reply. Default: `true` |

🤖 Prompt for AI Agents

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

In `@docs/config-reference.md` at line 1247, Update the `send_read_receipts`
config description so it reflects actual runtime behavior: instead of saying
receipts are posted "after each processed message", state that read receipts are
sent only after a successful reply is sent (i.e., when the outbound response
completes successfully). Locate the `send_read_receipts` entry in the config
reference and change the wording to mention "after successful replies" and keep
the default value (`true`) unchanged.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Sanitize error before logging to avoid leaking attachment identifiers.

Line 819 logs {e} directly, which may contain attachment GUIDs or filenames from transcribe_local error chain (e.g., from download_attachment_bytes). This could expose identifiers in logs.

🔧 Suggested fix

             Err(e) => {
-                tracing::warn!("BB audio transcription failed: {e}");
+                tracing::warn!("BB audio transcription failed");
+                tracing::debug!("Transcription error details: {e}");
                 self.parse_webhook_payload(payload)
             }

As per coding guidelines: "Never commit personal or sensitive data in … logs."

🤖 Prompt for AI Agents

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

In `@src/channels/bluebubbles.rs` around lines 818 - 821, In the Err(e) branch of
the audio transcription flow (the match arm that currently does
tracing::warn!("BB audio transcription failed: {e}")), avoid logging raw error
text which may contain attachment GUIDs or filenames from transcribe_local /
download_attachment_bytes; instead log a sanitized message and only
non-sensitive context (e.g., "BB audio transcription failed" plus error kind or
a scrubbed e.to_string() with GUID/filename patterns removed) and then continue
to call self.parse_webhook_payload(payload); update the tracing::warn call in
that Err(e) arm to use the sanitized/error-kind form rather than printing the
full error chain.

[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 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/channels/bluebubbles.rs (3)

271-288: ⚠️ Potential issue | 🟠 Major

group_policy = Allowlist currently bypasses sender allowlist checks.

In Allowlist mode, only group_allow_from is enforced. A sender not in allowed_senders can still pass if chat GUID is allowlisted.

🔒 Suggested fix

     fn is_group_allowed(&self, chat_guid: &str, sender: &str) -> bool {
+        let sender_allowed = self.is_sender_allowed(sender);
         match self.group_policy {
             BlueBubblesGroupPolicy::Disabled => false,
             BlueBubblesGroupPolicy::Allowlist => self
                 .group_allow_from
                 .iter()
-                .any(|g| g == "*" || g.eq_ignore_ascii_case(chat_guid)),
+                .any(|g| g == "*" || g.eq_ignore_ascii_case(chat_guid))
+                && sender_allowed,
             BlueBubblesGroupPolicy::Open => {
-                if self.allowed_senders.is_empty() {
-                    true
-                } else {
-                    self.allowed_senders
-                        .iter()
-                        .any(|a| a == "*" || a.eq_ignore_ascii_case(sender))
-                }
+                sender_allowed
             }
         }
     }

As per coding guidelines, “Do not silently weaken security policy or access constraints.”

🤖 Prompt for AI Agents

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

In `@src/channels/bluebubbles.rs` around lines 271 - 288, The Allowlist branch in
is_group_allowed currently returns true if chat_guid is in group_allow_from
without checking allowed_senders; change BlueBubblesGroupPolicy::Allowlist logic
to require both the chat GUID be allowlisted (group_allow_from contains "*" or
equals chat_guid) AND the sender is allowed (allowed_senders is empty or
contains "*" or equals sender), so only when both conditions hold return true;
update the block inside is_group_allowed to perform these two checks and return
false otherwise.

---

303-338: ⚠️ Potential issue | 🟠 Major

mark_read is not actually fire-and-forget and can block up to HTTP timeout.

The method is documented as fire-and-forget, but it awaits the POST. During BB outages this can stall processing per message.

⚙️ Non-blocking pattern

-        match self
-            .client
-            .post(&url)
-            .query(&[("password", &self.password)])
-            .send()
-            .await
-        {
-            Err(e) => tracing::warn!("BlueBubbles mark_read failed: {}", e.without_url()),
-            Ok(resp) => {
-                let status = resp.status();
-                if status.is_success() {
-                    tracing::info!("BlueBubbles mark_read ok status={status}");
-                } else {
-                    tracing::warn!("BlueBubbles mark_read got {status}");
-                }
-            }
-        }
+        let client = self.client.clone();
+        let password = self.password.clone();
+        tokio::spawn(async move {
+            match client.post(&url).query(&[("password", password.as_str())]).send().await {
+                Err(e) => tracing::warn!("BlueBubbles mark_read failed: {}", e.without_url()),
+                Ok(resp) => {
+                    let status = resp.status();
+                    if status.is_success() {
+                        tracing::info!("BlueBubbles mark_read ok status={status}");
+                    } else {
+                        tracing::warn!("BlueBubbles mark_read got {status}");
+                    }
+                }
+            }
+        });

🤖 Prompt for AI Agents

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

In `@src/channels/bluebubbles.rs` around lines 303 - 338, mark_read currently
awaits the POST and can block; make it truly fire-and-forget by moving the HTTP
call into a spawned background task. Keep the early check of send_read_receipts
and compute normalised/encoded/url synchronously, then clone/self-contain all
values needed for the request (clone self.client, password string, url) and call
tokio::spawn(async move { ... }) to perform the post and logging (use the same
tracing.warn/info logic inside the task). Reference mark_read, self.client,
self.password, self.api_url and ensure no await on the spawned task so the
function returns immediately.

---

818-822: ⚠️ Potential issue | 🟠 Major

Avoid logging raw transcription error chains at warn level.

error = %e can leak attachment/file identifiers from nested error messages. Emit sanitized summary instead.

🧹 Suggested sanitization

             Err(e) => {
-                tracing::warn!(error = %e, "BB audio transcription failed");
+                let sanitized = crate::providers::sanitize_api_error(&e.to_string());
+                tracing::warn!("BB audio transcription failed: {sanitized}");
                 self.parse_webhook_payload(payload)
             }

As per coding guidelines, “Never commit personal or sensitive data in … logs.”

🤖 Prompt for AI Agents

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

In `@src/channels/bluebubbles.rs` around lines 818 - 822, The current
tracing::warn!(error = %e, "BB audio transcription failed") can leak
attachment/file identifiers from nested error chains; replace this direct error
interpolation with a sanitized summary: remove the %e from the warn call and
instead pass a short, scrubbed string (e.g., the error type or a call to a
helper sanitize_error(e) that strips file paths/IDs and sensitive tokens) or use
std::any::type_name_of_val(&e) to log only the error class; keep the existing
fallback to self.parse_webhook_payload(payload) and ensure no raw error chain is
emitted at warn level.

src/channels/transcription.rs (1)

90-94: ⚠️ Potential issue | 🟠 Major

Clean up temp WAV on ffmpeg timeout/spawn failures.

The timeout/spawn-error path returns before removing tmp, so failed conversions can leak temporary files.

♻️ Cleanup-safe conversion pattern

-        let conv = tokio::time::timeout(std::time::Duration::from_secs(120), ffmpeg_cmd.output())
-            .await
-            .map_err(|_| anyhow::anyhow!("ffmpeg CAF→WAV conversion timed out after 120s"))?
-            .context("ffmpeg CAF→WAV conversion failed")?;
+        let conv = match tokio::time::timeout(
+            std::time::Duration::from_secs(120),
+            ffmpeg_cmd.output(),
+        )
+        .await
+        {
+            Ok(Ok(output)) => output,
+            Ok(Err(e)) => {
+                let _ = tokio::fs::remove_file(&tmp).await;
+                return Err(anyhow::anyhow!("ffmpeg CAF→WAV conversion failed: {e}"));
+            }
+            Err(_) => {
+                let _ = tokio::fs::remove_file(&tmp).await;
+                anyhow::bail!("ffmpeg CAF→WAV conversion timed out after 120s");
+            }
+        };

As per coding guidelines, “Keep error paths obvious and localized; prefer explicit bail!/errors for unsupported or unsafe states.”

🤖 Prompt for AI Agents

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

In `@src/channels/transcription.rs` around lines 90 - 94, The ffmpeg
timeout/spawn-error branch can return early and leak the temporary WAV file
`tmp`; update the conversion call around
tokio::time::timeout(std::time::Duration::from_secs(120), ffmpeg_cmd.output())
so that any early error path removes the temp file before returning—e.g., in the
map_err closure and any spawn/output error handling, call
std::fs::remove_file(&tmp).unwrap_or_else(|_| ()) (or use a small drop
guard/TempDir wrapper) and then return the anyhow error, ensuring the cleanup
happens for both timeout and ffmpeg spawn/output failures that currently return
before the subsequent success check on `conv`.

docs/config-reference.md (2)

1247-1247: ⚠️ Potential issue | 🟡 Minor

send_read_receipts description still mismatches runtime trigger.

The text says receipts are sent after each processed incoming message, but behavior is after successful replies. Please align wording to avoid operator confusion.

✏️ Suggested wording fix

-| `send_read_receipts` | Optional | Send read receipt to the sender after each incoming message is processed. Default: `true`  |
+| `send_read_receipts` | Optional | Send read receipt after each successful reply. Default: `true`                              |

Based on learnings: “Applies to docs/config-reference.md : Treat as runtime-contract reference. Track behavior changes to configuration schema.”

🤖 Prompt for AI Agents

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

In `@docs/config-reference.md` at line 1247, Update the `send_read_receipts`
config description in the table row (the entry currently reading "Send read
receipt to the sender after each incoming message is processed") to reflect the
actual runtime trigger: state that read receipts are sent after a reply is
successfully sent (e.g., "Send a read receipt to the sender after a reply is
successfully sent. Default: `true`"). Locate the `send_read_receipts` table row
and replace the sentence so wording matches runtime behavior.

---

1233-1254: ⚠️ Potential issue | 🟠 Major

Update related runtime-contract docs for BlueBubbles operations/troubleshooting.

This adds new channel behavior/config surface, but related operator references should be updated in the same PR (or explicitly deferred): docs/operations-runbook.md and docs/troubleshooting.md (plus localized counterparts for supported locales).

As per coding guidelines, “When runtime surfaces change, update related references (commands/providers/channels/config/runbook/troubleshooting).”

🤖 Prompt for AI Agents

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

In `@docs/config-reference.md` around lines 1233 - 1254, The new
channels_config.bluebubbles config keys (server_url, password, allowed_senders,
ignore_senders, webhook_secret, dm_policy, group_policy, group_allow_from,
send_read_receipts) were added but related runtime/operator docs weren’t
updated; update docs/operations-runbook.md and docs/troubleshooting.md (and all
localized equivalents) to document BlueBubbles webhook endpoint (POST
/bluebubbles), authentication (webhook_secret / Authorization: Bearer), policies
(dm_policy, group_policy and group_allow_from semantics),
allowed_senders/ignore_senders behavior, send_read_receipts default, and common
troubleshooting steps for connectivity/auth failures and message routing.

🤖 Prompt for all review comments with AI agents

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

Duplicate comments:
In `@docs/config-reference.md`:
- Line 1247: Update the `send_read_receipts` config description in the table row
(the entry currently reading "Send read receipt to the sender after each
incoming message is processed") to reflect the actual runtime trigger: state
that read receipts are sent after a reply is successfully sent (e.g., "Send a
read receipt to the sender after a reply is successfully sent. Default:
`true`"). Locate the `send_read_receipts` table row and replace the sentence so
wording matches runtime behavior.
- Around line 1233-1254: The new channels_config.bluebubbles config keys
(server_url, password, allowed_senders, ignore_senders, webhook_secret,
dm_policy, group_policy, group_allow_from, send_read_receipts) were added but
related runtime/operator docs weren’t updated; update docs/operations-runbook.md
and docs/troubleshooting.md (and all localized equivalents) to document
BlueBubbles webhook endpoint (POST /bluebubbles), authentication (webhook_secret
/ Authorization: Bearer), policies (dm_policy, group_policy and group_allow_from
semantics), allowed_senders/ignore_senders behavior, send_read_receipts default,
and common troubleshooting steps for connectivity/auth failures and message
routing.

In `@src/channels/bluebubbles.rs`:
- Around line 271-288: The Allowlist branch in is_group_allowed currently
returns true if chat_guid is in group_allow_from without checking
allowed_senders; change BlueBubblesGroupPolicy::Allowlist logic to require both
the chat GUID be allowlisted (group_allow_from contains "*" or equals chat_guid)
AND the sender is allowed (allowed_senders is empty or contains "*" or equals
sender), so only when both conditions hold return true; update the block inside
is_group_allowed to perform these two checks and return false otherwise.
- Around line 303-338: mark_read currently awaits the POST and can block; make
it truly fire-and-forget by moving the HTTP call into a spawned background task.
Keep the early check of send_read_receipts and compute normalised/encoded/url
synchronously, then clone/self-contain all values needed for the request (clone
self.client, password string, url) and call tokio::spawn(async move { ... }) to
perform the post and logging (use the same tracing.warn/info logic inside the
task). Reference mark_read, self.client, self.password, self.api_url and ensure
no await on the spawned task so the function returns immediately.
- Around line 818-822: The current tracing::warn!(error = %e, "BB audio
transcription failed") can leak attachment/file identifiers from nested error
chains; replace this direct error interpolation with a sanitized summary: remove
the %e from the warn call and instead pass a short, scrubbed string (e.g., the
error type or a call to a helper sanitize_error(e) that strips file paths/IDs
and sensitive tokens) or use std::any::type_name_of_val(&e) to log only the
error class; keep the existing fallback to self.parse_webhook_payload(payload)
and ensure no raw error chain is emitted at warn level.

In `@src/channels/transcription.rs`:
- Around line 90-94: The ffmpeg timeout/spawn-error branch can return early and
leak the temporary WAV file `tmp`; update the conversion call around
tokio::time::timeout(std::time::Duration::from_secs(120), ffmpeg_cmd.output())
so that any early error path removes the temp file before returning—e.g., in the
map_err closure and any spawn/output error handling, call
std::fs::remove_file(&tmp).unwrap_or_else(|_| ()) (or use a small drop
guard/TempDir wrapper) and then return the anyhow error, ensuring the cleanup
happens for both timeout and ffmpeg spawn/output failures that currently return
before the subsequent success check on `conv`.

---

ℹ️ Review info

Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d67ab25a-7808-43e5-bc3b-3ac5fe182d57

📥 Commits

Reviewing files that changed from the base of the PR and between b065e69 and 9e63f0b.

📒 Files selected for processing (3)

• docs/config-reference.md
• src/channels/bluebubbles.rs
• src/channels/transcription.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.

[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

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@docs/troubleshooting.md`:
- Around line 185-211: The new BlueBubbles webhook troubleshooting section added
to docs/troubleshooting.md must be synchronized to all localized troubleshooting
docs (zh-CN, ja, ru, fr, vi, el) or explicitly deferred in the PR; either add
translated copies of the section into each locale file (matching title
"BlueBubbles: webhook not receiving messages" and the same
symptom/diagnosis/fixes content adapted for language) or, if translations cannot
be completed in this PR, update the PR description to include a clear deferral
note naming the owner responsible and a follow-up issue/PR reference that will
add the localized versions (per docs/i18n-guide.md checklist item 3). Ensure the
change references the exact English section in docs/troubleshooting.md so
reviewers can verify synchronization or the deferral plan.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9cc4c863-edea-4492-a921-0c1b79feeff0

📥 Commits

Reviewing files that changed from the base of the PR and between 9e63f0b and 0cdc5ba.

📒 Files selected for processing (3)

• docs/i18n/vi/channels-reference.md
• docs/operations-runbook.md
• docs/troubleshooting.md

✅ Files skipped from review due to trivial changes (1)

• docs/operations-runbook.md

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

• docs/i18n/vi/channels-reference.md

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== 1. List all changed files to understand PR scope ==="
git diff --name-only HEAD~1..HEAD 2>/dev/null | head -30 || echo "(Could not get git diff)"

echo
echo "=== 2. Explore docs directory structure ==="
find docs -type d -maxdepth 2 | sort | head -20

echo
echo "=== 3. Check for locale-specific directory patterns ==="
fd -t d -H '^(i18n|locales|zh-CN|ja|ru|fr|vi|el)$' docs 2>/dev/null || echo "(No locale dirs found)"

echo
echo "=== 4. List troubleshooting files across all locations ==="
fd -H 'troubleshooting' docs 2>/dev/null | sort || echo "(No troubleshooting files found)"

echo
echo "=== 5. Check for i18n-related docs ==="
fd -H 'i18n|locali' docs -type f 2>/dev/null | head -10 || true

echo
echo "=== 6. Search for deferral notes in common locations ==="
rg -l 'deferr|follow.?up|future.*localiz|i18n.*follow' docs README.md 2>/dev/null | head -10 || echo "(No deferral notes found)"

echo
echo "=== 7. Check docs/troubleshooting.md history ==="
git log --oneline docs/troubleshooting.md 2>/dev/null | head -5 || echo "(Could not get git log)"

Repository: zeroclaw-labs/zeroclaw

Length of output: 1487

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== 1. Read docs/i18n-guide.md (mandatory completion checklist) ==="
head -100 docs/i18n-guide.md

echo
echo "=== 2. Check if BlueBubbles section exists in EN troubleshooting ==="
grep -n "BlueBubbles" docs/troubleshooting.md | head -5

echo
echo "=== 3. Check BlueBubbles presence in each locale ==="
for locale in zh-CN ja ru fr vi el; do
  count=$(grep -c "BlueBubbles" "docs/i18n/$locale/troubleshooting.md" 2>/dev/null || echo "0")
  echo "  $locale: $count occurrences"
done

echo
echo "=== 4. Show line counts for comparison (EN vs locales) ==="
wc -l docs/troubleshooting.md docs/i18n/*/troubleshooting.md | tail -10

echo
echo "=== 5. Search for deferral note in PR-related docs ==="
rg -n "BlueBubbles|deferr.*localiz|follow.*PR" docs/pr-workflow.md docs/README.md docs/SUMMARY.md 2>/dev/null | head -20 || echo "(No deferral found)"

Repository: zeroclaw-labs/zeroclaw

Length of output: 6094

---

Sync BlueBubbles troubleshooting section to all localized docs or document deferral in PR.

The BlueBubbles webhook troubleshooting section (lines 185–211) was added to the English docs but is missing from all 7 localized troubleshooting files (zh-CN, ja, ru, fr, vi, el). Per the i18n-guide.md mandatory completion checklist (item 3), top-level runtime docs changes must sync to all locales in the same PR. If localization cannot complete in this PR, add an explicit deferral note to the PR description with owner and follow-up issue/PR reference; silent deferral is not permitted. Currently neither sync nor deferral is present.

🤖 Prompt for AI Agents

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

In `@docs/troubleshooting.md` around lines 185 - 211, The new BlueBubbles webhook
troubleshooting section added to docs/troubleshooting.md must be synchronized to
all localized troubleshooting docs (zh-CN, ja, ru, fr, vi, el) or explicitly
deferred in the PR; either add translated copies of the section into each locale
file (matching title "BlueBubbles: webhook not receiving messages" and the same
symptom/diagnosis/fixes content adapted for language) or, if translations cannot
be completed in this PR, update the PR description to include a clear deferral
note naming the owner responsible and a follow-up issue/PR reference that will
add the localized versions (per docs/i18n-guide.md checklist item 3). Ensure the
change references the exact English section in docs/troubleshooting.md so
reviewers can verify synchronization or the deferral plan.

[theonlyhennygod]

Blocking merge for now: this PR is currently in a merge-conflict state with main (DIRTY/CONFLICTING). Please rebase or resolve conflicts against main, then I can re-review and merge while preserving contributor attribution.

[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/gateway/mod.rs (2)

2377-2379: ⚠️ Potential issue | 🟡 Minor

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

The comment says “Returning 200 immediately,” but this handler returns 202 Accepted (Line 2496).

🤖 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 - 2379, Update the stale comment that
reads "Returning 200 immediately" to reflect the actual response code sent by
this handler (202 Accepted); locate the comment near the Arc::clone(bluebubbles)
usage (e.g., the bb = Arc::clone(bluebubbles) context) and change the wording to
something like "Returning 202 Accepted immediately keeps the HTTP lifecycle
within the timeout budget while the background task handles transcription, LLM
inference, and reply." Ensure the comment aligns with the response value
returned by the handler.

---

585-591: ⚠️ Potential issue | 🟠 Major

Don’t silently drop BlueBubbles when initialization fails.

Line 590–Line 591 (.ok().flatten()) converts a real init error into None, so startup proceeds with the channel silently disabled.

✅ Fail-fast 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 should prefer explicit errors over silent fallback.

🤖 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 current chain
(.transpose().map_err(|e| { tracing::error!("Failed to initialize BlueBubbles
channel: {e}"); e }).ok().flatten()) swallows initialization errors by turning
Err into None; instead remove the .ok().flatten() and propagate the error so
startup fails fast—i.e., keep the .transpose().map_err(...) and then use the ?
operator (or return the Result) to propagate the error from the BlueBubbles init
call; ensure the surrounding function returns a Result so the error can bubble
up rather than being silently ignored.

src/channels/transcription.rs (1)

90-93: ⚠️ Potential issue | 🟠 Major

Timeout/spawn failure path can leak the temp WAV file.

Line 90–Line 93 returns early via ? before tmp cleanup, so partial conversion output can remain on disk.

♻️ Cleanup-safe handling

-        let conv = tokio::time::timeout(std::time::Duration::from_secs(120), ffmpeg_cmd.output())
-            .await
-            .map_err(|_| anyhow::anyhow!("ffmpeg CAF→WAV conversion timed out after 120s"))?
-            .context("ffmpeg CAF→WAV conversion failed")?;
+        let conv = match tokio::time::timeout(
+            std::time::Duration::from_secs(120),
+            ffmpeg_cmd.output(),
+        )
+        .await
+        {
+            Ok(Ok(o)) => o,
+            Ok(Err(e)) => {
+                let _ = tokio::fs::remove_file(&tmp).await;
+                return Err(anyhow::anyhow!("ffmpeg CAF→WAV conversion failed: {e}"));
+            }
+            Err(_) => {
+                let _ = tokio::fs::remove_file(&tmp).await;
+                anyhow::bail!("ffmpeg CAF→WAV conversion timed out after 120s");
+            }
+        };

As per coding guidelines, src/**/*.rs should keep error paths obvious and localized.

🤖 Prompt for AI Agents

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

In `@src/channels/transcription.rs` around lines 90 - 93, The timeout/spawn
failure path can leave the temporary WAV file (tmp) on disk because the early
return from the tokio::time::timeout / ffmpeg_cmd.output() block (`let conv =
...?;`) occurs before tmp is cleaned up; ensure tmp is removed on all
error/timeout paths by either using a RAII guard (e.g., wrap tmp in a type that
deletes on Drop or use tempfile::NamedTempFile) or explicitly remove the temp
file before returning when the timeout returns Err or when ffmpeg_cmd.output()
fails; update the code around ffmpeg_cmd.output(), timeout handling, and the tmp
variable to guarantee cleanup on both timeout and conversion error paths.

src/gateway/oauth.rs (1)

126-137: ⚠️ Potential issue | 🟡 Minor

OAuth routes remain open when pairing is disabled.

When require_pairing() is false, is_authenticated returns true for any token (including empty), leaving OAuth connect/status/revoke endpoints accessible without authentication. While this respects the operator's choice, OAuth credential management is sensitive and should arguably require explicit opt-in for unpaired access.

As per coding guidelines: "Deny-by-default for access and exposure boundaries."

🤖 Prompt for AI Agents

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

In `@src/gateway/oauth.rs` around lines 126 - 137, auth_check currently treats
empty tokens as valid when PairingGuard.is_authenticated returns true for
unpaired mode; change it to deny-by-default for OAuth endpoints by rejecting
empty or missing Bearer tokens unless there is an explicit operator opt-in flag.
Update auth_check to (1) extract the token as now, (2) if token is empty, return
false unless a dedicated config/flag (e.g. state.pairing.allow_unpaired_oauth or
state.config.allow_unpaired_oauth) is true, and (3) otherwise call
state.pairing.is_authenticated(token); reference auth_check,
state.pairing.is_authenticated, and add or use a clear opt-in symbol like
allow_unpaired_oauth to gate unpaired OAuth access.

🧹 Nitpick comments (2)

src/gateway/oauth.rs (2)

73-89: Consider hardening pre-existing directories.

DirBuilder::mode(0o700) only applies when creating a new directory. If the OAuth directory already exists with broader permissions (e.g., from a previous version or manual creation), those permissions remain unchanged. This was noted in a previous review cycle.

🔒 Optional: Re-apply permissions after creation

     #[cfg(unix)]
     {
+        use std::os::unix::fs::PermissionsExt;
         use std::os::unix::fs::DirBuilderExt;
         let dir = dir.clone();
         tokio::task::spawn_blocking(move || {
-            std::fs::DirBuilder::new().recursive(true).mode(0o700).create(&dir)
+            std::fs::DirBuilder::new().recursive(true).mode(0o700).create(&dir)?;
+            // Harden pre-existing directory
+            std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o700))
         })
         .await
         .map_err(|e| anyhow::anyhow!("spawn_blocking panicked creating oauth dir: {e}"))??;
     }

🤖 Prompt for AI Agents

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

In `@src/gateway/oauth.rs` around lines 73 - 89, The current ensure_oauth_dir
function uses DirBuilder::mode(0o700) which only affects newly created
directories and won't tighten permissions on an already-existing oauth dir;
update ensure_oauth_dir to, after creation (or if create fails because dir
exists), explicitly set the directory permissions to 0o700 (on Unix) using
std::fs::set_permissions or nix/chmod wrapper so pre-existing loose permissions
are hardened; keep the existing spawn_blocking approach for the blocking ops and
apply the permission change inside that Unix branch (referencing
ensure_oauth_dir, DirBuilder::mode, and the create call) and skip/adjust for
non-Unix platforms accordingly.

---

711-716: Consider escaping single quotes for defense-in-depth.

The html_escape function handles &, <, >, " but not single quotes ('). While current template usage appears safe, unescaped single quotes can be an XSS vector in attribute contexts using single-quoted values.

🛡️ Add single quote escaping

 fn html_escape(s: &str) -> String {
     s.replace('&', "&")
         .replace('<', "&lt;")
         .replace('>', "&gt;")
         .replace('"', "&quot;")
+        .replace('\'', "&#x27;")
 }

🤖 Prompt for AI Agents

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

In `@src/gateway/oauth.rs` around lines 711 - 716, The html_escape function
currently replaces &, <, >, and " but omits single quotes; update the
html_escape function to also replace the single-quote character (') with an HTML
entity (e.g., &#x27; or &#39;) so single-quoted attribute contexts are safe—add
the .replace for '\'' -> "&#x27;" (or "&#39;") alongside the existing replaces
in the html_escape function to ensure single quotes are escaped.

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@docs/i18n/vi/config-reference.md`:
- Around line 479-480: Update the enum documentation for dm_policy and
group_policy to match the new schema by replacing the outdated `"disabled"`
option with the correct `"block"` value; locate the table rows that mention
`dm_policy` and `group_policy` and change the enum list to `"open"` (default) |
`"allowlist"` | `"block"` so the docs reflect the runtime config contract and
prevent invalid configurations.

---

Duplicate comments:
In `@src/channels/transcription.rs`:
- Around line 90-93: The timeout/spawn failure path can leave the temporary WAV
file (tmp) on disk because the early return from the tokio::time::timeout /
ffmpeg_cmd.output() block (`let conv = ...?;`) occurs before tmp is cleaned up;
ensure tmp is removed on all error/timeout paths by either using a RAII guard
(e.g., wrap tmp in a type that deletes on Drop or use tempfile::NamedTempFile)
or explicitly remove the temp file before returning when the timeout returns Err
or when ffmpeg_cmd.output() fails; update the code around ffmpeg_cmd.output(),
timeout handling, and the tmp variable to guarantee cleanup on both timeout and
conversion error paths.

In `@src/gateway/mod.rs`:
- Around line 2377-2379: Update the stale comment that reads "Returning 200
immediately" to reflect the actual response code sent by this handler (202
Accepted); locate the comment near the Arc::clone(bluebubbles) usage (e.g., the
bb = Arc::clone(bluebubbles) context) and change the wording to something like
"Returning 202 Accepted immediately keeps the HTTP lifecycle within the timeout
budget while the background task handles transcription, LLM inference, and
reply." Ensure the comment aligns with the response value returned by the
handler.
- Around line 585-591: The current chain (.transpose().map_err(|e| {
tracing::error!("Failed to initialize BlueBubbles channel: {e}"); e
}).ok().flatten()) swallows initialization errors by turning Err into None;
instead remove the .ok().flatten() and propagate the error so startup fails
fast—i.e., keep the .transpose().map_err(...) and then use the ? operator (or
return the Result) to propagate the error from the BlueBubbles init call; ensure
the surrounding function returns a Result so the error can bubble up rather than
being silently ignored.

In `@src/gateway/oauth.rs`:
- Around line 126-137: auth_check currently treats empty tokens as valid when
PairingGuard.is_authenticated returns true for unpaired mode; change it to
deny-by-default for OAuth endpoints by rejecting empty or missing Bearer tokens
unless there is an explicit operator opt-in flag. Update auth_check to (1)
extract the token as now, (2) if token is empty, return false unless a dedicated
config/flag (e.g. state.pairing.allow_unpaired_oauth or
state.config.allow_unpaired_oauth) is true, and (3) otherwise call
state.pairing.is_authenticated(token); reference auth_check,
state.pairing.is_authenticated, and add or use a clear opt-in symbol like
allow_unpaired_oauth to gate unpaired OAuth access.

---

Nitpick comments:
In `@src/gateway/oauth.rs`:
- Around line 73-89: The current ensure_oauth_dir function uses
DirBuilder::mode(0o700) which only affects newly created directories and won't
tighten permissions on an already-existing oauth dir; update ensure_oauth_dir
to, after creation (or if create fails because dir exists), explicitly set the
directory permissions to 0o700 (on Unix) using std::fs::set_permissions or
nix/chmod wrapper so pre-existing loose permissions are hardened; keep the
existing spawn_blocking approach for the blocking ops and apply the permission
change inside that Unix branch (referencing ensure_oauth_dir, DirBuilder::mode,
and the create call) and skip/adjust for non-Unix platforms accordingly.
- Around line 711-716: The html_escape function currently replaces &, <, >, and
" but omits single quotes; update the html_escape function to also replace the
single-quote character (') with an HTML entity (e.g., &#x27; or &#39;) so
single-quoted attribute contexts are safe—add the .replace for '\'' -> "&#x27;"
(or "&#39;") alongside the existing replaces in the html_escape function to
ensure single quotes are escaped.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 09bf57a8-0aa5-40e6-bf01-e1f8af34d24b

📥 Commits

Reviewing files that changed from the base of the PR and between 0cdc5ba and df41efe.

📒 Files selected for processing (6)

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

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

• docs/i18n/el/channels-reference.md
• docs/i18n/vi/channels-reference.md

[coderabbitai]

⚠️ Potential issue | 🟠 Major

dm_policy / group_policy enum docs are inconsistent with the new schema.

These rows document "disabled", but this PR’s config contract introduces "block". Using "disabled" from docs will lead to invalid configuration.

🛠️ Suggested doc fix

-| `dm_policy`          | Tùy chọn | `"open"` (mặc định) \| `"allowlist"` \| `"disabled"` — kiểm soát tin nhắn trực tiếp                 |
-| `group_policy`       | Tùy chọn | `"open"` (mặc định) \| `"allowlist"` \| `"disabled"` — kiểm soát chat nhóm                          |
+| `dm_policy`          | Tùy chọn | `"open"` (mặc định) \| `"allowlist"` \| `"block"` — kiểm soát tin nhắn trực tiếp                    |
+| `group_policy`       | Tùy chọn | `"open"` (mặc định) \| `"allowlist"` \| `"block"` — kiểm soát chat nhóm                             |

Based on learnings: Applies to docs/config-reference.md — treat as runtime-contract reference and track configuration schema behavior changes.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	| `dm_policy` | Tùy chọn | `"open"` (mặc định) \| `"allowlist"` \| `"disabled"` — kiểm soát tin nhắn trực tiếp |
	| `group_policy` | Tùy chọn | `"open"` (mặc định) \| `"allowlist"` \| `"disabled"` — kiểm soát chat nhóm |
	| `dm_policy` | Tùy chọn | `"open"` (mặc định) \| `"allowlist"` \| `"block"` — kiểm soát tin nhắn trực tiếp |
	| `group_policy` | Tùy chọn | `"open"` (mặc định) \| `"allowlist"` \| `"block"` — kiểm soát chat nhóm |

🤖 Prompt for AI Agents

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

In `@docs/i18n/vi/config-reference.md` around lines 479 - 480, Update the enum
documentation for dm_policy and group_policy to match the new schema by
replacing the outdated `"disabled"` option with the correct `"block"` value;
locate the table rows that mention `dm_policy` and `group_policy` and change the
enum list to `"open"` (default) | `"allowlist"` | `"block"` so the docs reflect
the runtime config contract and prevent invalid configurations.

[maxtongwang]

Closing — no longer needed.