feat: disable permission auto-prompts + typing indicator on webhook channels#552
Merged
senamakel merged 4 commits intotinyhumansai:mainfrom Apr 14, 2026
Merged
Conversation
…l off Flip defaults so no macOS TCC permission prompt fires on first run: - `dictation.enabled`: `true` → `false` (was auto-starting rdev::listen, which requests Accessibility/Input Monitoring on macOS) - `screen_intelligence.use_vision_model`: `true` → `false` (fewer surprise vision-model calls; Pass 1 Apple Vision OCR still runs) Aligns all permission-gated auto-starts on a consistent opt-in posture: `screen_intelligence.enabled`, `autocomplete.enabled`, and `voice_server.auto_start` already default to `false`. Users must now explicitly flip each toggle (config or JSON-RPC) before the core triggers any OS permission dialog.
Two inbound flows exist today and only one fires typing: - Local bot (`channels_config.telegram.bot_token` set) → dispatch.rs already calls `channel.start_typing` + `spawn_scoped_typing_task` - Backend webhook (Telegram → backend → socket.io → core) → `ChannelInboundSubscriber` had **no typing call** — replies route via backend REST, so the local `Channel` trait isn't reachable. Close the gap by going through the backend: - `api/rest.rs`: add `send_channel_typing(channel, jwt, body)` hitting the new `POST /channels/:id/typing` backend route. - `channels/bus.rs`: extract the agent-wait loop into `run_agent_loop` and wrap it with a typing task that fires immediately on `start_chat` success, refreshes every 4s (beats Telegram's ~5s and Discord's ~10s typing TTLs), and cancels on every exit path (done / error / empty / bus-closed / lagged / timeout). Backend failures log at debug — a flaky typing call must never block the reply flow. Generalises to every channel with a backend adapter; adapters without a native typing API no-op gracefully.
Contributor
📝 WalkthroughWalkthroughThree configuration and test changes: two schema defaults flipped from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/config/schema/accessibility.rs (1)
22-26:⚠️ Potential issue | 🟡 MinorUpdate stale rustdoc default for
use_vision_model.Line 25 still says
Default: true., butdefault_use_vision_model()now returnsfalse. Please update the doc comment to prevent config confusion.Suggested patch
- /// feeds into the text synthesis LLM (Pass 3) — no vision model required. - /// Default: `true`. + /// feeds into the text synthesis LLM (Pass 3) — no vision model required. + /// Default: `false`.As per coding guidelines,
{src/**/*.rs,.github/**/*.md,docs/**/*.md}: Add concise rustdoc / code comments where the flow is not obvious; update ... docs when user-visible behavior change.Also applies to: 70-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/config/schema/accessibility.rs` around lines 22 - 26, Update the stale rustdoc for the accessibility flag so it matches the actual default returned by default_use_vision_model(): change the comment text that reads "Default: `true`." to "Default: `false`." for the use_vision_model field and any other duplicate occurrences (e.g., the similar comment around lines 70-72) so the documentation matches the behavior of default_use_vision_model().
🧹 Nitpick comments (1)
src/openhuman/channels/bus.rs (1)
97-105: Capture request context before spawning the typing loop.Lines 97-105, 175-187, and 193-228 only carry
channelinto the background task, so the typing path losesrequest_idand repeats config/session/client setup every 4 seconds. Passing an owned context intospawn_typing_taskwould keep these logs correlatable and avoid repeating most of that setup work on every refresh.As per coding guidelines, "include correlation fields such as request IDs, method names, and entity IDs".
Also applies to: 175-187, 193-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/channels/bus.rs` around lines 97 - 105, The typing background task currently only receives channel, losing request context (request_id) and forcing repeated setup; capture and clone the minimal owned context (at least request_id plus any correlation/config needed) before you call spawn_typing_task and pass that owned context into spawn_typing_task instead of just channel so the task can log correlated fields and reuse precomputed config/session/client; update the spawn_typing_task signature and its callers (where typing_cancel, typing_handle, spawn_typing_task, and run_agent_loop are used) to accept the owned context and remove duplicate setup inside the task; apply the same change to the other spawn_typing_task usages referenced in this review.
🤖 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/openhuman/channels/bus.rs`:
- Around line 153-155: The RecvError::Lagged branch currently only warns and
continues, which can miss the matching chat_done/chat_error for request_id and
cause bogus timeouts; update the handler in the loop handling inbound channel
events (the branch matching tokio::sync::broadcast::error::RecvError::Lagged
under the "[channel-inbound]" logic) to treat lag as terminal for this request:
after logging, break out of the processing loop or return an error so the caller
can perform cleanup and emit a chat_error (or otherwise mark the request
finished) instead of continuing to wait for events (preventing the 180s bogus
timeout).
---
Outside diff comments:
In `@src/openhuman/config/schema/accessibility.rs`:
- Around line 22-26: Update the stale rustdoc for the accessibility flag so it
matches the actual default returned by default_use_vision_model(): change the
comment text that reads "Default: `true`." to "Default: `false`." for the
use_vision_model field and any other duplicate occurrences (e.g., the similar
comment around lines 70-72) so the documentation matches the behavior of
default_use_vision_model().
---
Nitpick comments:
In `@src/openhuman/channels/bus.rs`:
- Around line 97-105: The typing background task currently only receives
channel, losing request context (request_id) and forcing repeated setup; capture
and clone the minimal owned context (at least request_id plus any
correlation/config needed) before you call spawn_typing_task and pass that owned
context into spawn_typing_task instead of just channel so the task can log
correlated fields and reuse precomputed config/session/client; update the
spawn_typing_task signature and its callers (where typing_cancel, typing_handle,
spawn_typing_task, and run_agent_loop are used) to accept the owned context and
remove duplicate setup inside the task; apply the same change to the other
spawn_typing_task usages referenced in this review.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7318a24-77a8-48e5-9f14-8dc09bc5be15
📒 Files selected for processing (4)
src/api/rest.rssrc/openhuman/channels/bus.rssrc/openhuman/config/schema/accessibility.rssrc/openhuman/config/schema/dictation.rs
Comment on lines
+153
to
+155
| Err(tokio::sync::broadcast::error::RecvError::Lagged(n)) => { | ||
| tracing::warn!("[channel-inbound] event bus lagged, skipped {} events", n); | ||
| } |
Contributor
There was a problem hiding this comment.
Treat Lagged as terminal for this request.
Lines 153-155 only log and continue. After RecvError::Lagged, this receiver may already have skipped the matching chat_done / chat_error event for request_id, so the loop can keep typing until the 180s timeout and then emit a bogus timeout reply.
Minimal fail-closed fix
- Err(tokio::sync::broadcast::error::RecvError::Lagged(n)) => {
- tracing::warn!("[channel-inbound] event bus lagged, skipped {} events", n);
- }
+ Err(tokio::sync::broadcast::error::RecvError::Lagged(n)) => {
+ tracing::error!("[channel-inbound] event bus lagged, skipped {} events", n);
+ send_channel_reply(
+ channel,
+ "Sorry, I lost track of that request. Please try again.",
+ )
+ .await;
+ return;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/channels/bus.rs` around lines 153 - 155, The RecvError::Lagged
branch currently only warns and continues, which can miss the matching
chat_done/chat_error for request_id and cause bogus timeouts; update the handler
in the loop handling inbound channel events (the branch matching
tokio::sync::broadcast::error::RecvError::Lagged under the "[channel-inbound]"
logic) to treat lag as terminal for this request: after logging, break out of
the processing loop or return an error so the caller can perform cleanup and
emit a chat_error (or otherwise mark the request finished) instead of continuing
to wait for events (preventing the 180s bogus timeout).
# Conflicts: # src/api/rest.rs # src/openhuman/channels/bus.rs
…ED_ENV in tests - Added a static Mutex guard to ensure safe concurrent access to the `TRIAGE_DISABLED_ENV` variable during tests, preventing interleaved set_var/remove_var calls that could lead to spurious failures. - Updated relevant test cases to acquire the Mutex lock when accessing the environment variable, ensuring consistent behavior across concurrent test executions.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/composio/bus.rs (1)
515-527: Make env-var cleanup panic-safe to avoid cascading test pollution.Cleanup is currently manual; if an assertion/panic happens before
remove_var, later tests may inherit stale state. Prefer an RAII restore helper.♻️ Proposed refactor (panic-safe env restoration)
#[cfg(test)] mod tests { use super::*; use serde_json::json; use std::sync::Mutex; @@ static TRIAGE_ENV_GUARD: Mutex<()> = Mutex::new(()); + + struct EnvVarRestore { + key: &'static str, + prev: Option<String>, + } + + impl EnvVarRestore { + fn set(key: &'static str, value: &str) -> Self { + let prev = std::env::var(key).ok(); + std::env::set_var(key, value); + Self { key, prev } + } + } + + impl Drop for EnvVarRestore { + fn drop(&mut self) { + if let Some(prev) = &self.prev { + std::env::set_var(self.key, prev); + } else { + std::env::remove_var(self.key); + } + } + } @@ async fn handles_trigger_event_without_panic() { let _guard = TRIAGE_ENV_GUARD.lock().unwrap_or_else(|e| e.into_inner()); - std::env::set_var(TRIAGE_DISABLED_ENV, "1"); + let _env_restore = EnvVarRestore::set(TRIAGE_DISABLED_ENV, "1"); @@ - std::env::remove_var(TRIAGE_DISABLED_ENV); } @@ fn triage_disabled_flag_parser() { let _guard = TRIAGE_ENV_GUARD.lock().unwrap_or_else(|e| e.into_inner()); + let _env_restore = EnvVarRestore { + key: TRIAGE_DISABLED_ENV, + prev: std::env::var(TRIAGE_DISABLED_ENV).ok(), + }; @@ - std::env::remove_var(TRIAGE_DISABLED_ENV); + std::env::remove_var(TRIAGE_DISABLED_ENV); assert!(!triage_disabled(), "unset must default to triage enabled"); } }Also applies to: 531-545
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/composio/bus.rs` around lines 515 - 527, The test sets TRIAGE_DISABLED_ENV manually and removes it later which is not panic-safe; replace the manual set/remove with an RAII helper that saves the previous state and restores it on drop (e.g., an EnvRestore or TriageEnvGuard) and use it around the ComposioTriggerSubscriber::new() / sub.handle(...) sequence; ensure the helper interacts with TRIAGE_DISABLED_ENV and acquires/reuses TRIAGE_ENV_GUARD as needed so removal always happens even if assertions panic, and update both occurrences (the block around lines shown and the similar block at 531-545) to construct this guard instead of calling std::env::set_var/remove_var directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/openhuman/composio/bus.rs`:
- Around line 515-527: The test sets TRIAGE_DISABLED_ENV manually and removes it
later which is not panic-safe; replace the manual set/remove with an RAII helper
that saves the previous state and restores it on drop (e.g., an EnvRestore or
TriageEnvGuard) and use it around the ComposioTriggerSubscriber::new() /
sub.handle(...) sequence; ensure the helper interacts with TRIAGE_DISABLED_ENV
and acquires/reuses TRIAGE_ENV_GUARD as needed so removal always happens even if
assertions panic, and update both occurrences (the block around lines shown and
the similar block at 531-545) to construct this guard instead of calling
std::env::set_var/remove_var directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c81c3526-bbb8-4e20-828c-334a5e6f55ed
📒 Files selected for processing (1)
src/openhuman/composio/bus.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Merged
upstream/main(picking up #549 progressive-edit streaming + typing indicator) into this branch. After the merge, only the privacy-defaults change remains unique to this PR — the typing-indicator work I originally included inrest.rsandchannels/bus.rswas already landed upstream in #549 with a stronger implementation (progressive edits, latched fallback). I took upstream's version of both files during conflict resolution.What's left in this PR
Flip three config defaults to
falseso a fresh install never triggers a macOS TCC permission dialog before the user opts in:dictation.enabled:true→false(was auto-startingrdev::listen, which requests Accessibility / Input Monitoring on macOS)screen_intelligence.use_vision_model:true→false(Pass 1 Apple Vision OCR still runs; skips the surprise vision-model call)screen_intelligence.enabled,autocomplete.enabled, andvoice_server.auto_startalready defaulted tofalse— this alignsdictationwith the same opt-in posture.Companion backend PR
The upstream client (post-#549) already calls
POST /channels/:id/typing, but the backend route isn't deployed yet. https://github.com/tinyhumansai/backend/pull/636 adds it (TelegramsendChatAction+ DiscordTrigger Typing Indicator).Test plan
cargo check --bin openhuman-coreclean.cargo test --lib --bin openhuman-core— 2655 passed / 0 failed.dictation.enabled = trueinconfig.toml, restart — confirm the Accessibility / Input Monitoring prompt now fires again.Summary by CodeRabbit