fix(macos): restart sidecar so TCC permission state updates after System Settings (#133)#182
Merged
senamakel merged 3 commits intotinyhumansai:mainfrom Apr 1, 2026
Conversation
…ttings Fixes tinyhumansai#133 - Tauri: restart core after kill+wait; fail fast when port held by non-managed process - ACL: allow core_rpc_url and restart_core_process (IPC was blocked in Tauri 2) - Dev: default_core_bin falls through to release search when binaries/ empty - Core: expose permission_check_process_path on accessibility status - App: Restart & refresh UX, extractError for invoke, Vitest for RPC + slice - Stage script: optional dev codesign helper for stable TCC identity Made-with: Cursor
Contributor
📝 WalkthroughWalkthroughAdds a restart-and-refresh flow for the core sidecar so macOS permission grants take effect: new Tauri command and permission, core process restart logic, backend RPC for re-detection, frontend thunk/UI to restart and retry permission checks, macOS codesign helper, and types/tests updated to surface process path info. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI
participant Redux
participant Tauri
participant Core
participant OSAPI
User->>UI: Grants permission in System Settings\nReturns and clicks "Restart & Refresh Permissions"
UI->>Redux: Dispatch refreshPermissionsWithRestart()
Redux->>Tauri: invoke restart_core_process
Tauri->>Core: request restart
Core->>Core: acquire restart lock\nshutdown child (wait/reap)
Core->>Core: poll RPC port until free\nspawn new sidecar
Core->>Tauri: return restart result
Redux->>Redux: set isRestartingCore = true
Redux->>Tauri: call core RPC openhuman.accessibility_status (retry loop)
Tauri->>Core: forward RPC to screen_intelligence
Core->>OSAPI: AXIsProcessTrusted / CGPreflight / IOHIDCheckAccess
OSAPI->>Core: return updated permissions
Core->>Tauri: return PermissionStatus
Redux->>Redux: set isRestartingCore = false\nupdate status & lastError
UI->>User: render updated permission status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 3
🧹 Nitpick comments (2)
app/src/pages/onboarding/steps/ScreenPermissionsStep.tsx (1)
82-88: Consider aligning button label with other panels for consistency.This button shows "Refresh Status" while
AccessibilityPanelandScreenIntelligencePaneluse "Restart & Refresh Permissions". The inconsistency might confuse users about what the button actually does (core restart).💡 Suggested label alignment
- {isRestartingCore ? 'Restarting core...' : 'Refresh Status'} + {isRestartingCore ? 'Restarting core…' : 'Restart & Refresh'}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/pages/onboarding/steps/ScreenPermissionsStep.tsx` around lines 82 - 88, Change the button label in ScreenPermissionsStep.tsx to match the other panels by replacing "Refresh Status" with "Restart & Refresh Permissions" so it clearly reflects the action triggered by dispatch(refreshPermissionsWithRestart()); ensure the text still toggles to "Restarting core..." when isRestartingCore is true and keep existing disabled logic that uses isRestartingCore and isLoading.scripts/setup-dev-codesign.sh (1)
24-25: Remove dead variableKEY_DER.
KEY_DERis declared but never used, which adds noise and can confuse maintenance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup-dev-codesign.sh` around lines 24 - 25, Remove the unused variable KEY_DER from the script: delete the line assigning KEY_DER="$TMPDIR_CERT/openhuman-dev-key.der" so only variables actually used remain (e.g., TMPDIR_CERT). Ensure no other references to KEY_DER exist elsewhere in the script before removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src-tauri/src/lib.rs`:
- Around line 99-105: The restart_core_process command must be serialized to
prevent overlapping restarts; modify the code so invocations acquire an async
restart gate before calling CoreProcessHandle::restart. Add an async
Mutex/Semaphore field (e.g., restart_lock) to the CoreProcessHandle or to the
tauri state and have restart_core_process await that lock, then call
state.inner().restart().await while holding the lock, releasing it afterwards;
reference the restart_core_process function and CoreProcessHandle::restart to
locate where to add the gate.
In `@scripts/setup-dev-codesign.sh`:
- Around line 68-72: The security add-trusted-cert invocation is currently
adding root trust broadly; modify the command that calls security
add-trusted-cert (the one using "$KEYCHAIN" and "$CERT") to include the -p
codeSign policy flag so the trust is scoped only to code signing rather than all
certificate policies.
- Around line 52-64: The script fails under set -u because the variables P12 and
P12_PASS are used before being defined; initialize and validate them before the
openssl pkcs12 and security import calls (e.g., compute P12 from CERT/KEY or set
a default tempfile name, generate or read P12_PASS from an environment variable
or prompt), ensure P12 and P12_PASS are non-empty (exit with a clear error if
not), and then proceed to run openssl pkcs12 and security import using those
validated variables so expansions do not trigger unbound-variable errors.
---
Nitpick comments:
In `@app/src/pages/onboarding/steps/ScreenPermissionsStep.tsx`:
- Around line 82-88: Change the button label in ScreenPermissionsStep.tsx to
match the other panels by replacing "Refresh Status" with "Restart & Refresh
Permissions" so it clearly reflects the action triggered by
dispatch(refreshPermissionsWithRestart()); ensure the text still toggles to
"Restarting core..." when isRestartingCore is true and keep existing disabled
logic that uses isRestartingCore and isLoading.
In `@scripts/setup-dev-codesign.sh`:
- Around line 24-25: Remove the unused variable KEY_DER from the script: delete
the line assigning KEY_DER="$TMPDIR_CERT/openhuman-dev-key.der" so only
variables actually used remain (e.g., TMPDIR_CERT). Ensure no other references
to KEY_DER exist elsewhere in the script before removal.
🪄 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: 18138ec5-5962-4a4f-934f-f69d568301a5
📒 Files selected for processing (18)
app/src-tauri/capabilities/default.jsonapp/src-tauri/permissions/allow-core-process.tomlapp/src-tauri/src/core_process.rsapp/src-tauri/src/lib.rsapp/src/components/settings/panels/AccessibilityPanel.tsxapp/src/components/settings/panels/ScreenIntelligencePanel.tsxapp/src/components/settings/panels/__tests__/AccessibilityPanel.test.tsxapp/src/pages/onboarding/steps/ScreenPermissionsStep.tsxapp/src/services/__tests__/coreRpcClient.test.tsapp/src/store/__tests__/accessibilitySlice.test.tsapp/src/store/accessibilitySlice.tsapp/src/utils/tauriCommands.tsscripts/setup-dev-codesign.shscripts/stage-core-sidecar.mjssrc/openhuman/screen_intelligence/engine.rssrc/openhuman/screen_intelligence/ops.rssrc/openhuman/screen_intelligence/schemas.rssrc/openhuman/screen_intelligence/types.rs
… process restart handling - Changed button text in ScreenPermissionsStep from 'Refresh Status' to 'Restart & Refresh Permissions' for better user understanding. - Introduced a restart lock in CoreProcessHandle to serialize overlapping restart requests, ensuring smoother core process management. - Updated restart_core_process function to acquire the restart lock before initiating a restart, improving reliability during the process.
…ePanel - Adjusted text formatting in AccessibilityPanel for better readability. - Enhanced text clarity in ScreenIntelligencePanel regarding permission refresh instructions. - Standardized the formatting of permission_check_process_path in test files for consistency.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src-tauri/src/core_process.rs (1)
350-366:⚠️ Potential issue | 🟡 MinorSort binaries and prefer the current target triple to ensure deterministic sidecar selection.
The code returns the first
openhuman-core-*entry fromread_dir(), which has no guaranteed order. In debug builds with multiple target triples, the selected binary can differ between runs, causing flaky macOS permission tests. Collect all matches, sort them deterministically (e.g., by filename), and prefer the current target triple before falling back to the first available candidate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src-tauri/src/core_process.rs` around lines 350 - 366, The function that scans binaries_dir (using std::fs::read_dir and checking path.file_name() into file_name with the matches logic) should not return the first match directly; instead collect all candidate paths into a Vec (e.g., candidates), sort them deterministically (by file_name or display string), then determine the current target triple (via env!("TARGET") or std::env::var("TARGET") as appropriate) and prefer the candidate whose filename contains that target triple; if none match the current triple, return the first sorted candidate. Update the early return in the loop to push to candidates and add the sorting/preference logic before returning an Option<PathBuf>.
🧹 Nitpick comments (3)
app/src/pages/onboarding/steps/ScreenPermissionsStep.tsx (1)
89-98: Consider simplifying the process path message for onboarding users.The raw executable path (
permission_check_process_path) may be confusing for non-technical users during onboarding. Consider either:
- Showing only the app/binary name (e.g.,
openhuman-core) instead of the full path, or- Adding a brief explanation like "Look for 'OpenHuman' or this app in System Settings"
This is less critical than in the settings panel since users reaching this step may need more guidance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/pages/onboarding/steps/ScreenPermissionsStep.tsx` around lines 89 - 98, The UI is showing the raw executable path (status.permission_check_process_path) which is confusing for onboarding users; update the ScreenPermissionsStep rendering logic to display a simplified app name instead: derive a basename from permission_check_process_path (e.g., take the final path segment like "openhuman-core") and display that, and add a short helper string such as "Look for 'OpenHuman' or this app in System Settings" alongside it; keep lastError handling unchanged but show the friendly name (fallback to the full path if basename extraction fails) so locate the JSX block that references lastError and status.permission_check_process_path and replace the displayed text accordingly.app/src/components/settings/panels/AccessibilityPanel.tsx (1)
148-153: Consider hiding dev-specific instructions from production builds.The
yarn core:stageinstruction is helpful for developers but may confuse end users in production. Consider either:
- Conditionally rendering this paragraph based on
import.meta.env.DEV, or- Moving it to a separate "Developer Troubleshooting" section
Example conditional rendering
- <p className="opacity-75"> - Still stuck? Remove the old entry for this app in System Settings → Privacy, then - click “Request” again. For dev, run{' '} - <span className="font-mono text-xs">yarn core:stage</span> so the sidecar matches - the staged binary name. - </p> + <p className="opacity-75"> + Still stuck? Remove the old entry for this app in System Settings → Privacy, then + click “Request” again. + </p> + {import.meta.env.DEV && ( + <p className="opacity-75"> + For dev, run <span className="font-mono text-xs">yarn core:stage</span> so the + sidecar matches the staged binary name. + </p> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/settings/panels/AccessibilityPanel.tsx` around lines 148 - 153, The paragraph in AccessibilityPanel.tsx that includes the dev-only command "yarn core:stage" should not be shown in production; update the JSX in the AccessibilityPanel component to conditionally render that dev instruction only when import.meta.env.DEV is true (or alternatively extract it into a separate "Developer Troubleshooting" component/section and render that component only in DEV). Locate the paragraph node containing the <span className="font-mono text-xs">yarn core:stage</span> and wrap it with a conditional (e.g., {import.meta.env.DEV && ( ... )}) or move its content into a new DevTroubleshooting component and render that component conditionally in AccessibilityPanel.app/src-tauri/src/core_process.rs (1)
344-370: Add trace logs around the dev-sidecar fallback.This branch changes which executable identity gets used for permission checks, but it never logs whether a staged sidecar was selected or when lookup fell back to the packaged search. A small
info!/debug!pair here would make TCC issues much easier to trace end-to-end.As per coding guidelines, "Add substantial, development-oriented logs while implementing features or fixes so issues are easy to trace end-to-end on new/changed flows".Possible logging hook
let binaries_dir = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("binaries"); if let Ok(entries) = std::fs::read_dir(&binaries_dir) { for entry in entries.flatten() { let path = entry.path(); if !path.is_file() { continue; } let Some(file_name) = path.file_name().and_then(|n| n.to_str()) else { continue; }; #[cfg(windows)] let matches = file_name.starts_with("openhuman-core-") && file_name.ends_with(".exe"); #[cfg(not(windows))] let matches = file_name.starts_with("openhuman-core-"); if matches { + log::info!("[core] using staged debug core binary {}", file_name); return Some(path); } } + log::debug!( + "[core] no staged debug core binary found in src-tauri/binaries; falling back to packaged lookup" + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src-tauri/src/core_process.rs` around lines 344 - 370, Add tracing around the dev-sidecar fallback in the cfg(debug_assertions) block in core_process.rs: log the resolved binaries_dir path at debug level, log an info! message when you select/return a staged sidecar (include the chosen path/file_name), and log a debug! message when a candidate is skipped or when the directory read yields no matching files so the code clearly shows whether the staged sidecar was used or lookup will fall back to the packaged search; insert these logs inside the existing loop and after the read_dir check (use the same tracing/log macros used elsewhere in this file).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src-tauri/src/core_process.rs`:
- Around line 42-45: The restart mutex accessor restart_lock() is public which
makes synchronization opt-in; make the mutex acquisition internal by removing
the pub from restart_lock (or deleting the accessor) in core_process.rs and
instead acquire the lock inside the public lifecycle methods (e.g., restart(),
shutdown(), any permission refresh methods) by calling
self.restart_lock.lock().await at the top of each method; update callers to stop
calling restart_lock() directly and ensure shutdown() and other lifecycle entry
points hold the mutex for their entire critical section so quitting cannot
interleave with permission refresh or restart.
- Around line 266-285: The shutdown path currently uses the restart-specific
reap-and-wait logic (the timeout(Duration::from_secs(12)) around child.wait())
which causes app-exit to block; split the behavior by extracting the timed wait
into a new helper (e.g., wait_for_child_reap_with_timeout or
reap_child_with_timeout) that contains the existing
timeout(Duration::from_secs(12)) { child.wait() } logic and call that helper
only from restart(), while changing shutdown() to perform the kill without
invoking the 12s timed wait (either not waiting or using a much shorter
non-blocking check), updating references to child.wait(), timeout, restart(),
and shutdown() in core_process.rs accordingly.
---
Outside diff comments:
In `@app/src-tauri/src/core_process.rs`:
- Around line 350-366: The function that scans binaries_dir (using
std::fs::read_dir and checking path.file_name() into file_name with the matches
logic) should not return the first match directly; instead collect all candidate
paths into a Vec (e.g., candidates), sort them deterministically (by file_name
or display string), then determine the current target triple (via env!("TARGET")
or std::env::var("TARGET") as appropriate) and prefer the candidate whose
filename contains that target triple; if none match the current triple, return
the first sorted candidate. Update the early return in the loop to push to
candidates and add the sorting/preference logic before returning an
Option<PathBuf>.
---
Nitpick comments:
In `@app/src-tauri/src/core_process.rs`:
- Around line 344-370: Add tracing around the dev-sidecar fallback in the
cfg(debug_assertions) block in core_process.rs: log the resolved binaries_dir
path at debug level, log an info! message when you select/return a staged
sidecar (include the chosen path/file_name), and log a debug! message when a
candidate is skipped or when the directory read yields no matching files so the
code clearly shows whether the staged sidecar was used or lookup will fall back
to the packaged search; insert these logs inside the existing loop and after the
read_dir check (use the same tracing/log macros used elsewhere in this file).
In `@app/src/components/settings/panels/AccessibilityPanel.tsx`:
- Around line 148-153: The paragraph in AccessibilityPanel.tsx that includes the
dev-only command "yarn core:stage" should not be shown in production; update the
JSX in the AccessibilityPanel component to conditionally render that dev
instruction only when import.meta.env.DEV is true (or alternatively extract it
into a separate "Developer Troubleshooting" component/section and render that
component only in DEV). Locate the paragraph node containing the <span
className="font-mono text-xs">yarn core:stage</span> and wrap it with a
conditional (e.g., {import.meta.env.DEV && ( ... )}) or move its content into a
new DevTroubleshooting component and render that component conditionally in
AccessibilityPanel.
In `@app/src/pages/onboarding/steps/ScreenPermissionsStep.tsx`:
- Around line 89-98: The UI is showing the raw executable path
(status.permission_check_process_path) which is confusing for onboarding users;
update the ScreenPermissionsStep rendering logic to display a simplified app
name instead: derive a basename from permission_check_process_path (e.g., take
the final path segment like "openhuman-core") and display that, and add a short
helper string such as "Look for 'OpenHuman' or this app in System Settings"
alongside it; keep lastError handling unchanged but show the friendly name
(fallback to the full path if basename extraction fails) so locate the JSX block
that references lastError and status.permission_check_process_path and replace
the displayed text accordingly.
🪄 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: b0b7a47e-ff00-4792-ad4c-a9a5a58127a3
📒 Files selected for processing (8)
app/src-tauri/src/core_process.rsapp/src-tauri/src/lib.rsapp/src/components/settings/panels/AccessibilityPanel.tsxapp/src/components/settings/panels/ScreenIntelligencePanel.tsxapp/src/pages/onboarding/steps/ScreenPermissionsStep.tsxapp/src/services/__tests__/coreRpcClient.test.tsapp/src/store/__tests__/accessibilitySlice.test.tsscripts/setup-dev-codesign.sh
✅ Files skipped from review due to trivial changes (2)
- scripts/setup-dev-codesign.sh
- app/src/services/tests/coreRpcClient.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src-tauri/src/lib.rs
- app/src/components/settings/panels/ScreenIntelligencePanel.tsx
Comment on lines
+42
to
+45
| /// Acquire the restart lock to serialize overlapping restart requests. | ||
| pub async fn restart_lock(&self) -> tokio::sync::MutexGuard<'_, ()> { | ||
| self.restart_lock.lock().await | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t expose the restart mutex as a public call-step.
This makes correct synchronization opt-in. app/src-tauri/src/lib.rs (Line 245-Line 251) already bypasses it by calling shutdown() directly, so quitting during a permission refresh can still interleave lifecycle operations. Keep the mutex private and acquire it inside the public lifecycle methods instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src-tauri/src/core_process.rs` around lines 42 - 45, The restart mutex
accessor restart_lock() is public which makes synchronization opt-in; make the
mutex acquisition internal by removing the pub from restart_lock (or deleting
the accessor) in core_process.rs and instead acquire the lock inside the public
lifecycle methods (e.g., restart(), shutdown(), any permission refresh methods)
by calling self.restart_lock.lock().await at the top of each method; update
callers to stop calling restart_lock() directly and ensure shutdown() and other
lifecycle entry points hold the mutex for their entire critical section so
quitting cannot interleave with permission refresh or restart.
Comment on lines
+266
to
+285
| // Wait for the process to exit so the RPC listen socket is released before restart | ||
| // checks the port (otherwise we can spuriously hit "port still in use"). | ||
| match timeout( | ||
| Duration::from_secs(12), | ||
| child.wait(), | ||
| ) | ||
| .await | ||
| { | ||
| Ok(Ok(status)) => { | ||
| log::debug!("[core] child core process reaped after kill: {status}"); | ||
| } | ||
| Ok(Err(e)) => { | ||
| log::warn!("[core] wait on child core process after kill: {e}"); | ||
| } | ||
| Err(_) => { | ||
| log::warn!( | ||
| "[core] timed out waiting for child core process to exit after kill (12s)" | ||
| ); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Split restart shutdown from app-exit shutdown.
The reap-and-wait path is useful for restart(), but shutdown() is also used by the app exit handler in app/src-tauri/src/lib.rs (Line 245-Line 251). That means closing the app now pays a restart-specific slow path and can block quit for up to 12 seconds. Use a separate wait-on-exit helper for restart only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src-tauri/src/core_process.rs` around lines 266 - 285, The shutdown path
currently uses the restart-specific reap-and-wait logic (the
timeout(Duration::from_secs(12)) around child.wait()) which causes app-exit to
block; split the behavior by extracting the timed wait into a new helper (e.g.,
wait_for_child_reap_with_timeout or reap_child_with_timeout) that contains the
existing timeout(Duration::from_secs(12)) { child.wait() } logic and call that
helper only from restart(), while changing shutdown() to perform the kill
without invoking the 12s timed wait (either not waiting or using a much shorter
non-blocking check), updating references to child.wait(), timeout, restart(),
and shutdown() in core_process.rs accordingly.
This was referenced Apr 2, 2026
This was referenced Apr 9, 2026
This was referenced Apr 23, 2026
Merged
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
2 participants
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
openhuman-coresidecar after macOS privacy grants so APIs likeAXIsProcessTrusted/CGPreflightScreenCaptureAccessrun in a fresh process (per-process TCC cache).core_rpc_urlandrestart_core_processin Tauri 2 ACL so IPC is not denied.wait()on the child afterkill, fail fast when nothing we spawned holds the port but something else does.default_core_binno longer returns early without searching next to the app / Resources whenbinaries/is empty.permission_check_process_pathso the UI matches System Settings to the real executable.coreRpcClient+accessibilitySlicefor fetched status shape.Problem
Fixes #133 — Screen Recording / Accessibility / Input Monitoring showed DENIED after granting in System Settings because checks run in the sidecar and macOS does not refresh grants for a running process.
Solution
restart_core_process→ kill + wait + wait for port +ensure_running.refreshPermissionsWithRestartthunk + ACL permission file.Submission Checklist
coreRpcClient+accessibilitySlice//////!, TS JSDoc where touchedImpact
permission_check_process_pathonAccessibilityStatus.Related
Fixes #133
Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores