feat(core): gate the desktop-automation cluster behind a default-ON feature (#5049) - #5061
Conversation
…eature (tinyhumansai#5049) Add a default-ON `desktop-automation` Cargo feature compiling out the accessibility / screen_intelligence / autocomplete / desktop_companion / computer cluster (~24.5k LOC) when off, via the voice/web3 facade+stub pattern and the skills type carve-out (inert types stay ungated; stubs re-export the one real definition). The accessibility stub is voice-aware: it mirrors the non-macOS real arms so a `voice`-on / `desktop-automation`-off build behaves like a non-desktop platform. Sheds the exclusive `uiautomation` dependency (proven via `cargo tree -i`). Wires the `DomainSet::desktop_automation` runtime axis and retags the three cluster controllers; the 15 `screen_intelligence_*` and 4 `computer` tools become absent (not error-degraded) when off. Both-ways gate tests pin both directions. Forwarded to the desktop shell. With the gate on the build is byte-identical.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds a ChangesDesktop automation feature and runtime gating
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
…inyhumansai#5049) The `GlobeHotkey*` type carve-out left `PermissionState` (the type) named only by the macOS listener's permission check; the non-macOS stubs read `detect_permissions().input_monitoring` without naming the type. Gate the import to macOS so the Linux/Windows build is warning-clean under `cargo clippy -D warnings` (the core-crate clippy CI lane, which was red on this).
…urface (tinyhumansai#5049) The desktop-automation gate unregisters the autocomplete / screen_intelligence controllers when off, but three pre-existing core tests hard-asserted that surface and were not #[cfg]-gated in lockstep, so `cargo test --lib --no-default-features --features tokenjuice-treesitter core::` failed (the testing gotcha AGENTS.md calls out; the CI smoke lane runs `cargo check` only and never compiled these tests). - legacy_aliases::is_compiled_out_method now also treats autocomplete_ / screen_intelligence_ / companion_ methods as compiled-out when the feature is off (composed with the existing mcp handling), so the frontend-catalog drift test keeps asserting on everything else. - grouped_schemas_contains_migrated_namespaces gates its `autocomplete` assertion. - invoke_autocomplete_status_rejects_unknown_param is gated on the feature. Disabled-build core:: suite now 656 passed / 0 failed (was 654 / 3). Default build unchanged: all three run + pass; clippy -D warnings clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30a4476355
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/accessibility/stub.rs`:
- Around line 27-229: Add namespaced debug!/trace! diagnostics to disabled
accessibility flows in src/openhuman/accessibility/stub.rs (lines 27-229),
covering entry, disabled branches, state changes, returned errors, and outcomes
for validate_focused_target, focused-text and paste functions,
globe_listener_start/poll/stop, permission functions, automation-state
functions, and automate::run; do not log user text or PII. Also add equivalent
grep-friendly diagnostics in src/openhuman/autocomplete/stub.rs (lines 21-79)
for omitted registration and disabled engine status/start/stop behavior,
including relevant entry/exit and error or state results.
In `@src/openhuman/screen_intelligence/stub.rs`:
- Around line 77-89: Add grep-friendly diagnostic logging before each disabled
rejection: log the rejected accessibility_capture_image_ref RPC in
src/openhuman/screen_intelligence/stub.rs lines 77-89, the rejected
screen-intelligence CLI command in src/openhuman/screen_intelligence/stub.rs
lines 122-124, and the rejected autocomplete start command in
src/core/autocomplete_cli_adapter.rs lines 99-113. Preserve each existing
disabled outcome and use the project’s established logging mechanism.
In `@src/openhuman/tools/ops_tests.rs`:
- Around line 2168-2201: Update the DESKTOP_ALWAYS_ON expectations in the
desktop_tools_are_registered test so screen_intelligence_status and
screen_intelligence_capture_now are included only under the desktop-automation
feature, matching the existing per-entry cfg gating and feature-off absence
contract.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38fd8d38-df19-4776-a883-f7fa574108f4
📒 Files selected for processing (28)
Cargo.tomlapp/src-tauri/Cargo.tomlsrc/core/all.rssrc/core/all_tests.rssrc/core/autocomplete_cli_adapter.rssrc/core/cli_tests.rssrc/core/jsonrpc_tests.rssrc/core/legacy_aliases.rssrc/core/runtime/builder.rssrc/openhuman/accessibility/globe.rssrc/openhuman/accessibility/mod.rssrc/openhuman/accessibility/stub.rssrc/openhuman/accessibility/types.rssrc/openhuman/autocomplete/core/engine.rssrc/openhuman/autocomplete/core/mod.rssrc/openhuman/autocomplete/core/text.rssrc/openhuman/autocomplete/mod.rssrc/openhuman/autocomplete/stub.rssrc/openhuman/autocomplete/types.rssrc/openhuman/desktop_companion/mod.rssrc/openhuman/desktop_companion/stub.rssrc/openhuman/screen_intelligence/mod.rssrc/openhuman/screen_intelligence/stub.rssrc/openhuman/tools/impl/mod.rssrc/openhuman/tools/impl/system/mod.rssrc/openhuman/tools/mod.rssrc/openhuman/tools/ops.rssrc/openhuman/tools/ops_tests.rs
…bled paths (tinyhumansai#5049) From CodeRabbit review + a disabled-build test audit: - Gate the desktop-automation entries in the DESKTOP_ALWAYS_ON / DESKTOP_DEFAULT_OFF test constants and the all_tools_includes_computer_control_when_enabled test with #[cfg(feature = "desktop-automation")] (matching the existing mcp gating). Without this the disabled-build tools::ops::tests suite expected screen_intelligence_* / mouse / keyboard tools that the gate compiles out — the AGENTS.md testing gotcha, invisible to CI's cargo-check-only smoke lane. - Add grep-friendly debug diagnostics to the disabled entry points: the screen_intelligence capture-image-ref RPC + CLI rejections, the autocomplete-start CLI rejection, and the accessibility paste / globe-start / automate::run disabled actions (never logging user text or PII). Disabled-build tools::ops::tests now pass except three pre-existing knowledge_* failures (skills/flows-gated tools asserted ungated — unrelated to this gate; they fail on main too, since CI never runs the disabled-build test suite).
…lake Under cfg(test) the crate's ~12.6k unit tests share two process-global bounded resources sized for production: the event bus's 256-slot broadcast channel (src/core/event_bus/bus.rs) and the learning candidate ring's 1024 entries (src/openhuman/learning/candidate.rs, whose drainer does not run in the unit suite). Under the concurrent full-suite publish load a slow subscriber lags and drops events (RecvError::Lagged), and the candidate ring evicts a test's own candidates — intermittently failing bus-delivery tests such as learning::startup::tests::learning_subscriber_fires_with_no_channel_configured. Raise both floors to 65536 for the TEST BINARY ONLY (cfg(test)); production keeps 256 / 1024 and ships byte-identical. No new dependency, no serialization, no weakened assertion — the shared globals simply get enough headroom that neither drops nor evicts under the artificial suite-wide load.
…s with DA off (tinyhumansai#5049) Codex review: in a slim build with `voice` enabled but `desktop-automation` disabled, `voice::audio_capture::record_on_thread` reads mic permission through `accessibility::detect_microphone_permission` and treats `Unknown` as "request-then-recheck, else fail". The stub returned `Unknown` and `request_microphone_access` is a no-op, so dictation failed before ever opening the mic — even where the mic is usable. The real Linux / non-macOS implementation cpal-probes and returns `Granted` when an input device exists. Match that permissive non-desktop arm in the stub (`detect_microphone_permission` + the `microphone` field of `detect_permissions` now return `Granted`) so voice proceeds and any real capture error surfaces from cpal, instead of a preemptive permission failure.
📌 Note for reviewers — the event-bus change here is an unrelated, self-healing flake fixThis PR carries a small test-only change that is not part of its objective:
They fix two pre-existing full-suite flakes — Root cause (corrected): not a capacity problem. Both tests asserted against the process-global broadcast bus, so under the ~12,600-test parallel suite they received events published by other concurrent tests — cross-test interference, which no channel-capacity bump can fix. (An earlier capacity-floor attempt was pushed and then reverted here once CI proved it ineffective.) Fix — real isolation:
Production ships byte-identical — the diff is confined to Why it's in this PR: this PR changes Pre-existing CI red unrelated to this PR (not merge-blocking — not in
|
…city floor The earlier test-only capacity floor (5c49a96) did not fix the flake: the two failing tests share the process-global broadcast bus, so under the parallel suite they receive events published by *other* tests — a capacity bump cannot fix cross-test interference. Replace it with real isolation. - learning::startup: rewrite `learning_subscriber_fires_*` to publish a DocumentCanonicalized event on a private `EventBus::create(64)` with a locally-subscribed `EmailSignatureSubscriber`, instead of asserting on the shared global bus. Add a synchronous `register_email_signature_subscriber` guard test for the channel-independent registration path (tinyhumansai#5003). - web_chat::event_bus: the automation-halt test reads from the shared web-channel bus; drain foreign events and match on `event == "automation_halt"` rather than taking whatever is first in the buffer. - Revert the `#[cfg(test)]` capacity floors in event_bus/bus.rs and learning/candidate.rs back to origin/main — no longer needed.
CodeGhost21
left a comment
There was a problem hiding this comment.
Reviewed the desktop-automation gate. Overall this is high-quality, idiomatic work: the facade + stub.rs pattern is applied cleanly, the type carve-out (GlobeHotkey* → accessibility::types) is correct with zero duplication, both-directions tests pin the gate, and the dependency-shed claim is disciplined (only uiautomation is exclusive). The detect_microphone_permission() → Granted and PermissionState import fixes are genuine catches.
I verified there are no callers of the macOS-only permission exports (detect_screen_recording_permission, etc.) outside the gated tree, so the stub omitting them is safe on a slim macOS build.
Approve-with-follow-up. The one thing worth pushing on before this pattern proliferates is a slim-build CI lane (see the Cargo.toml comment) — the rest are non-blocking nits filed inline.
| # | ||
| # NOTE: only `uiautomation` is exclusive and thus shed. `enigo` is shared with the | ||
| # `voice` domain; `rdev` / `arboard` are voice-owned — none of those are dropped here. | ||
| desktop-automation = ["dep:uiautomation"] |
There was a problem hiding this comment.
(Structural — highest concern) CI never compiles or tests the disabled build.
The safety of every stub↔real signature parity here rests entirely on a manual local --no-default-features build. The PR body itself notes CI's cargo check-only smoke lane never caught the six pre-existing test-gating bugs CodeRabbit surfaced, and that three knowledge_* disabled-build tests already fail on main undetected.
This gate multiplies the feature combinations that can silently rot. Strongly recommend a follow-up (or this PR) adding a slim cargo check --no-default-features --features <minimal> lane — otherwise the next contributor who adds an always-on caller of a gated symbol breaks the slim build with no signal. Good candidate to file alongside the Phase 2 extraction.
There was a problem hiding this comment.
Agreed, and I think keeping the full slim test lane out of this PR is the right scope call. You're correct that the Rust Feature-Gate Smoke (gates off) lane only cargo checks the disabled build and never runs its test suite — which is exactly how the pre-existing gated-test breakage went unnoticed.
What this PR does add on that front: it extends the self-maintaining gated-test guard so the allowlist can't silently drift as the gate surface grows — desktop-automation is now in the guard regex and the new core/autocomplete_cli_adapter.rs is in EXPECTED (8e9ce8b). That's still a compile-time guard, not a disabled-build test run.
A dedicated slim cargo test --no-default-features --features <minimal> lane belongs with the Phase 2 extraction follow-up rather than expanding this PR — noting it here so it's tracked alongside that work.
| /// (screen-recording / accessibility / input-monitoring) are unknown when the | ||
| /// gate is off; microphone mirrors `detect_microphone_permission` above so the | ||
| /// `voice`-on / `desktop-automation`-off build keeps a usable mic state. | ||
| pub fn detect_permissions() -> PermissionStatus { |
There was a problem hiding this comment.
(Minor nit) Unknown vs the real non-macOS Unsupported.
The real non-macOS detect_permissions returns PermissionState::Unsupported for screen_recording / accessibility / input_monitoring, but this stub returns Unknown. Elsewhere the stub docs claim the disabled build "behaves like a non-desktop platform" — this slightly contradicts that.
I confirmed no always-on / voice caller branches on these three fields (only microphone is read outside the gated tree), so it's cosmetic. For exactness, Unsupported would better match the stated intent.
There was a problem hiding this comment.
Good catch — fixed in 8be6f19. The disabled-build stub detect_permissions() now returns PermissionState::Unsupported for screen_recording / accessibility / input_monitoring, matching the real non-macOS detect_permissions (permissions.rs:267, pinned by detect_permissions_non_macos_reports_unsupported_for_desktop_perms) and the sibling struct that already used Unsupported. microphone stays Granted. Confirmed no always-on/voice caller branches on those three fields, and the disabled build (--no-default-features --features tokenjuice-treesitter) compiles clean.
| // channel runtime is ever constructed in this test. | ||
| let _ = crate::openhuman::memory::global::init(tmp.path().join("workspace")); | ||
| register_learning_subscribers(tmp.path().to_path_buf()); | ||
| async fn learning_subscriber_fires_on_document_event() { |
There was a problem hiding this comment.
(Scope) Unrelated flaky-test fix riding along.
This isolated-bus refactor is a test-only flake fix, not gate work. Not blocking, but it inflates the review surface — ideally it lands on its own PR (the sibling web_chat/event_bus.rs change is at least disclosed as cherry-picked from #5063 and self-drops on rebase).
There was a problem hiding this comment.
Fair point on scope. I've kept the isolated-bus refactor here deliberately: it fixes a full-suite flake that this PR's own gates-off test + coverage lanes would otherwise hit intermittently, so dropping it would destabilize this branch's checks. It's test-only and low-risk. Happy to extract it to a standalone PR if you'd prefer the clean separation — say the word and I'll pull it out and rebase.
| // suite other tests publish to it too, so take THIS subscriber's next | ||
| // `automation_halt` event rather than whatever happens to be first in the | ||
| // buffer (which flaked as an "event name mismatch"). | ||
| fn next_automation_halt( |
There was a problem hiding this comment.
(Very minor) Narrow flake window remains.
In the "resumed" block, a concurrent test publishing an automation_halt event between the first drain and the resumed publish could be returned instead of the intended event. Much less likely than the bug this fixes, and test-only — noting for completeness.
There was a problem hiding this comment.
Tightened in 88f7b94. next_automation_halt now filters on the engaged payload (true for halted, false for resumed) in addition to the event name, so a concurrent test's halt/resume on the shared web-channel bus can no longer be returned in place of the intended event. The 6 web_chat::event_bus tests pass. (The helper is cherry-picked from #5063; the same tightening can land there on rebase.)
…A off The disabled-build stub returned PermissionState::Unknown for screen_recording / accessibility / input_monitoring, but the real non-macOS detect_permissions reports these as Unsupported. Align the stub so the desktop-automation-off build matches the documented "behaves like a non-desktop platform" intent. Cosmetic: no always-on or voice caller branches on these three fields (only microphone is read outside the gated tree), which stays Granted.
The self-maintaining gated-test guard grepped only (voice|media|web3|meet|mcp|skills|flows), so the new desktop-automation gate's test files were invisible to it — the lane would stay green while silently under-covering the gate as its tests grow. Add desktop-automation to the regex and add the now-matched core/autocomplete_cli_adapter.rs to the EXPECTED allowlist.
next_automation_halt filtered only on the event name, but automation_halt is emitted for both engaged=true (halted) and engaged=false (resumed). A concurrent test publishing to the shared web-channel bus between the drain and the resumed publish could be returned in place of the intended event. Filter on the expected engaged payload so each phase takes only its own event.
…tion-gate-5049 # Conflicts: # src/openhuman/learning/startup.rs # src/openhuman/web_chat/event_bus.rs
The tinyhumansai#5084 tui gate added `#[cfg(feature = "tui")]` guards, the optional ratatui/crossterm deps, the forwarding allowlist entry, and the docs — but never declared the `tui` feature in [features] or added it to `default`. That left `feature = "tui"` an unknown cfg value, so `cargo clippy -p openhuman -- -D warnings` failed with unexpected_cfgs (Rust Quality lane). Declare `tui = ["dep:ratatui", "dep:crossterm"]` and add it to default, matching the documented default-ON intent (already allowlisted in check-feature-forwarding.mjs).
…ell self-test The 'real shell manifest forwards every real core default' self-test asserted every default-ON gate is forwarded to the desktop shell, ignoring the INTENTIONALLY_NOT_FORWARDED allowlist that the CI checker respects. Once the tui gate (tinyhumansai#5084) became default-ON — a terminal subcommand deliberately not shipped in the Tauri shell — the two disagreed and Scripts Self-Tests failed with 'core default gate not forwarded to the shell: tui'. Move the allowlist into scripts/lib/feature-forwarding.mjs (one source of truth) and have the self-test skip allowlisted gates, matching the checker. The lane is path-gated so this was dormant on main; it surfaced here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e13c1806f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub fn globe_listener_start() -> Result<GlobeHotkeyStatus, String> { | ||
| log::debug!("[accessibility] globe_listener_start disabled: desktop-automation off"); | ||
| Ok(disabled_globe_status()) |
There was a problem hiding this comment.
Preserve Fn dictation when voice is kept
In a macOS slim build with voice enabled but desktop-automation disabled, the voice server still uses the Globe/Fn listener for its default Fn hotkey: voice::server::start_globe_hotkey_listener calls globe_listener_start() and treats !status.running as a startup error. This stub always returns a non-running status, so voice_server.auto_start or the standalone voice server fails before dictation can start in that feature combination. The Globe listener needs the same voice carve-out as the microphone path, or voice needs a working fallback when desktop automation is compiled out.
Useful? React with 👍 / 👎.
Reconcile dep-gating with main (now incl. tinyhumansai#5052/tinyhumansai#5060/tinyhumansai#5061/tinyhumansai#5062): - Cargo.toml: union gating (optional axum/sentry/ppt-rs + http-server/ inference/documents/crash-reporting features) with main's tower + tui deps and desktop-automation/tui features; default = union of both feature sets. - app/src-tauri: shell forwards crash-reporting/http-server/desktop-automation (feature-forwarding gate passes; tui allowlisted). - all_tests.rs: union of http-server + desktop-automation gate tests. - types.rs: keep main's redirect_links security denylist entry (tinyhumansai#5052). Cargo.lock + app/src-tauri/Cargo.lock resynced (--locked passes); cargo fmt, feature-forwarding gate, and clippy -D warnings all clean locally.
Summary
desktop-automationCargo feature that compiles out the ~24.5k-LOC desktop-automation cluster —accessibility,screen_intelligence,autocomplete,desktop_companion, and thecomputeragent-tool family — when disabled.stub.rspattern (voice/web3) and the skills type carve-out rule: inert type modules stay ungated so enabled/disabled builds share one definition.DomainSet::desktop_automationaxis (feat(core): DomainSet on CoreBuilder + DomainRegistration filter seam (feature-gate prerequisite) #4796); the three cluster controllers unregister and the 15screen_intelligence_*+ 4computeragent tools become absent (not error-degraded) when off.uiautomationdependency (proven viacargo tree -i); forwarded to the desktop shell.Problem
None of this cluster belongs in a headless or embedded core — it is desktop-only by nature and the second-largest ungated family after channels. Keeping it unconditional inflates the embedded
openhuman_corebinary and RAM footprint (#5046) and keeps OS-automation dependencies in every build. The cluster is not a leaf: it is reached from always-on code and, critically, theaccessibilitymodule is consumed by the independently-toggledvoicegate — so the gate must be built carefully to keep every feature combination compiling.Solution
accessibility→ facade + a rich, voice-aware stub. The stub reproduces the exact public surface reached byvoice,text_input,app_state, and siblings, byte-for-byte; where the real module has a non-macOS arm it mirrors that arm (e.g. overlay/validate returnOk), so avoice-on /desktop-automation-off build behaves like a non-desktop platform.automate::run<B>is generic so the caller's&RealBackendbinds without stubbing the backend trait.autocomplete/screen_intelligence/desktop_companion→ facade + minimal stubs (empty registration aggregators + the always-on caller surface; carved status types re-exported).computer→ leaf gate; per-element#[cfg]on the tool registrations (themcp/web3idiom).GlobeHotkey*moved into ungatedaccessibility::types;AutocompleteStatus/AccessibilityStatuskept ungated. Zero type duplication.DomainGroup::DesktopAutomation+ field +full/harness/nonepresets +allowsarm + preset tests; the three aggregators retagged fromPlatform.core/all_tests.rs+tools/ops_tests.rspin registration/tool presence in both directions.enigo/arboard/rdevdeps are not claimed as shed —enigois shared withvoice,rdev/arboardare voice-owned. Onlyuiautomationis exclusive.Verified locally across all four combinations (on/on, off/on-voice, off/off) plus fmt, clippy, feature-forwarding, and the
uiautomationshed proof.Submission Checklist
desktop_automation_controllers_registered_when_feature_on/..._absent_when_feature_offandscreen_intelligence_tools_absent_when_feature_off(asserts the tools are absent, not degraded).N/A: compile-time gate over existing behaviour; no new user-facing feature row.N/A.uiautomationis made optional, not added.N/A: gate-on build is byte-identical to the shipped app.Closes #NNN—N/A: this is Phase 1 (the gate). Phase 2 (extraction into its own crate) is a separate follow-up; see## Related.Impact
uiautomationdependency. The shipped desktop app is unchanged (default-ON, forwarded to the shell)./rpcmethods become unknown-method and its tools are absent by design.Related
DomainSetseam).Changes since initial submission (review-driven)
6544a6b1b) — gate thePermissionStateimport inglobe.rsto macOS; theGlobeHotkey*type carve-out left it used only by macOS code, so it was an unused import (Linux-D warnings).299e9ec65, CodeRabbit surfaced a real bug) — six pre-existing tests hard-asserted the now-gated surface and were not#[cfg]-gated in lockstep (the AGENTS.md testing gotcha; CI'scargo check-only smoke lane never caught them): theDESKTOP_ALWAYS_ON/DESKTOP_DEFAULT_OFFtest consts,all_tools_includes_computer_control_when_enabled, and threecore::tests (cli/jsonrpc/legacy_aliases). Disabled-buildcore::is now green.299e9ec65, CodeRabbit) — grep-friendlydebug!on the screen-intelligence RPC/CLI + autocomplete-start rejections and the accessibility disabled actions (no user text/PII).0b76e1b05, Codex — real bug) — the accessibility stub returnedUnknownmic permission, which brokevoicedictation preemptively in a voice-on / desktop-automation-off build. The stub now returnsGranted, matching the real Linux/non-macOS cpal arm.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
Summary by CodeRabbit
New Features
Bug Fixes