fix(si_server): skip embedded server autostart on non-macOS (OPENHUMAN-TAURI-50)#1542
Open
oxoxDev wants to merge 2 commits into
Open
Conversation
Add a runtime check `if !cfg!(target_os = "macos")` in `start_if_enabled` to skip `si_server` autostart on non-macOS platforms. Adjust logging to use `info!` for "macOS-only" errors on those platforms to prevent Sentry noise. Sentry: OPENHUMAN-TAURI-50 Co-authored-by: oxoxDev <164490987+oxoxDev@users.noreply.github.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe server classifies known "macOS-only" session-start failures and logs them at info on non-macOS, skips embedded autostart on non-macOS, and includes tests that validate the classifier and platform-specific autostart behavior. ChangesCross-Platform macOS-Only Error Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🧹 Nitpick comments (1)
src/openhuman/screen_intelligence/server.rs (1)
159-163: ⚡ Quick winDeduplicate the macOS-only error classification check.
The same
!cfg!(target_os = "macos") && e.contains("macOS-only")logic is repeated in two paths. Extract a small helper to keep behavior consistent over time.Proposed refactor
+fn is_macos_only_error(err: &str) -> bool { + err.contains("macOS-only") +} ... - if !cfg!(target_os = "macos") && e.contains("macOS-only") { + if !cfg!(target_os = "macos") && is_macos_only_error(&e) { info!("{LOG_PREFIX} failed to start session: {e}"); } else { error!("{LOG_PREFIX} failed to start session: {e}"); } ... - if !cfg!(target_os = "macos") && e.contains("macOS-only") { + if !cfg!(target_os = "macos") && is_macos_only_error(&e) { info!("{LOG_PREFIX} embedded server autostart skipped: {e}"); } else { error!("{LOG_PREFIX} embedded server exited with error: {e}"); }Also applies to: 366-370
🤖 Prompt for 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. In `@src/openhuman/screen_intelligence/server.rs` around lines 159 - 163, The duplicated condition `!cfg!(target_os = "macos") && e.contains("macOS-only")` should be extracted into a small helper (e.g., fn is_macos_only_error(e: &str) -> bool) in server.rs and used wherever that check appears (replace the inline condition in the session-start error branch around the LOG_PREFIX handling and the similar branch at the 366-370 area); update the two branches to call is_macos_only_error(e) and keep the existing info/error logging behavior unchanged.
🤖 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/screen_intelligence/server.rs`:
- Around line 462-475: The test should assert the no-op contract more strictly
by verifying no global server was created after calling start_if_enabled:
instead of only checking an existing server's state, assert that
try_global_server() returns None (i.e., no server instance exists) after
start_if_enabled(&config). Update the test function (which currently calls
start_if_enabled and then inspects try_global_server and ServerState::Stopped)
to assert try_global_server().is_none() so regressions that create-then-stop are
caught.
---
Nitpick comments:
In `@src/openhuman/screen_intelligence/server.rs`:
- Around line 159-163: The duplicated condition `!cfg!(target_os = "macos") &&
e.contains("macOS-only")` should be extracted into a small helper (e.g., fn
is_macos_only_error(e: &str) -> bool) in server.rs and used wherever that check
appears (replace the inline condition in the session-start error branch around
the LOG_PREFIX handling and the similar branch at the 366-370 area); update the
two branches to call is_macos_only_error(e) and keep the existing info/error
logging behavior unchanged.
🪄 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: aa4b1506-c141-4bc0-a5b5-fb4c4bd66354
📒 Files selected for processing (1)
src/openhuman/screen_intelligence/server.rs
…st (OPENHUMAN-TAURI-50) Two adjustments per CodeRabbit review on PR tinyhumansai#1542: 1. Deduplicate `!cfg!(target_os = "macos") && e.contains("macOS-only")` — the same predicate appeared in both `SiServer::run`'s start_session error arm and `start_if_enabled`'s spawned `server.run` future. Extracted to a single private `is_expected_macos_only_failure(&str)` helper so the log-level decision (info! vs error!) stays in sync if the engine ever revises its marker text. Plus one unit test covering the platform-gated branches. 2. Strengthen the non-macOS `start_if_enabled_skips_on_non_macos` test to assert the strict no-op contract. The previous shape only checked `if let Some(server)... state == Stopped`, which would silently pass if the implementation regressed to a "create-then-stop" pattern. The test now records pre-state and asserts `try_global_server()` stays `None` post-call when the singleton was untouched at entry. `cargo test --lib openhuman::screen_intelligence::server` — 7/7 pass (local on macOS; the non-macOS-only test is cfg-gated out — CI matrix covers the Windows / Linux leg). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
si_serverautostart on non-macOS at the spawn site so cores no longer crash withaccessibility automation is macOS-only in V1. Sentry issue OPENHUMAN-TAURI-50 (~15 events).error!toinfo!for defense-in-depth.Problem
screen_intelligence::server::start_if_enabledspawns the embedded screen-intelligence server on every platform during core boot. The underlying accessibility engine is macOS-only in V1 (per the existing check atengine.rs:95-96), so on Windows/Linux the spawned task immediately errors and surfaces as Sentry OPENHUMAN-TAURI-50 (~15 events). The error log is the noise — the actual feature gap is expected and documented.Solution
src/openhuman/screen_intelligence/server.rs::start_if_enabled: runtimeif !cfg!(target_os = "macos")guard returns early with aninfo!log explaining the V1 platform constraint. Avoids#[cfg]-gating the function symbol so non-mac callers still compile.start_sessionor spawnedrun()does fail with a macOS-only message (defense-in-depth — covers any future caller that bypassesstart_if_enabled), demoteerror!→info!so any residual non-mac entrypoint doesn't pollute Sentry.engine.rs:95-96left intact.Submission Checklist
start_if_enabledreturns without panic and server isStopped; macOS cfg-gated test asserts global server initializesN/A: platform-gating fix, no new feature row## Related—N/A: behaviour-only changeN/A: no release-cut surface touchedClosesin the## RelatedsectionImpact
info!line per non-mac core boot.screen_intelligence.enabled = trueon non-mac now no-ops cleanly instead of erroring. Feature availability identical to V1 reality (mac-only).cfg!()check is a compile-time constant; negligible.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix-si-server-non-macos-crash-13903582644506769746Validation Run
pnpm --filter openhuman-app format:check— N/A: noapp/changespnpm typecheck— N/A: no TS changescargo test --lib openhuman::screen_intelligence::server::tests::start_if_enabled_skips_on_non_macos(non-mac targets)cargo test --lib openhuman::screen_intelligence::server::tests::start_if_enabled_continues_on_macos(mac targets)app/src-tauri/changesValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Parity Contract
engine.rs:95-96left intact as last line of defense.Duplicate / Superseded PR Handling
Summary by CodeRabbit
Bug Fixes
Tests