Repository navigation
Conversation
📝 WalkthroughWalkthroughAdds macOS GUI login-session runtime scopes to daemon handshakes, health checks, startup, adoption, replacement, cleanup, restart, and PTY history. Scope values are resolved from process audit identity and boot-session UUID, persisted for history isolation, and passed through daemon CLI and adapter configuration. Mismatched clients and legacy daemons are rejected or retired. New tests cover parsing, scoped recovery, login transitions, reboot, account isolation, concurrency, and daemon replacement. Documentation and a reliability gate define validation scenarios and assertions. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/daemon/daemon-pty-adapter.ts (1)
1234-1245: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify redundant ternary.
Since
getMacDaemonSystemResolverHealthaccepts an optional fourthruntimeScopeparameter, you can passthis.runtimeScopedirectly and remove the ternary. Passingundefinedyields the exact same behavior as omitting the argument.♻️ Proposed refactor
- const health = this.runtimeScope - ? await getMacDaemonSystemResolverHealth( - this.socketPath, - this.tokenPath, - this.protocolVersion, - this.runtimeScope - ) - : await getMacDaemonSystemResolverHealth( - this.socketPath, - this.tokenPath, - this.protocolVersion - ) + const health = await getMacDaemonSystemResolverHealth( + this.socketPath, + this.tokenPath, + this.protocolVersion, + this.runtimeScope + )config/reliability-gates.jsonc (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest-file references for the new gate are incomplete/inconsistent across the two documentation artifacts. The gate declares 5
testFiles/commandsentries butassertionRefsdocuments only 2 of them, and the reproduction doc's headlineRun:command lists only 3 of the 5, even though it separately cites daemon-init.test.ts assertions.
config/reliability-gates.jsonc#L5487-5576: addassertionRefsentries fordaemon-login-session-history.test.ts,macos-gui-session-scope.test.ts, anddaemon-health.test.tsto match the pattern used by every other gate in this file (one entry pertestFilesitem).docs/bug-reproductions/issue-7936-macos-login-session-daemon.md#L9-14: extend theRun:command to includedaemon-health.test.tsanddaemon-init.test.tsso it matches the gate's registeredcommandsand the daemon-init.test.ts assertions discussed later in the same doc (line 28).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 871c423a-45c9-4150-8bb9-5a2b3e6978e6
📒 Files selected for processing (22)
config/reliability-gates.jsoncdocs/bug-reproductions/issue-7936-macos-login-session-daemon.mdsrc/main/daemon/client.tssrc/main/daemon/daemon-entry.test.tssrc/main/daemon/daemon-entry.tssrc/main/daemon/daemon-health.test.tssrc/main/daemon/daemon-health.tssrc/main/daemon/daemon-hello-protocol.tssrc/main/daemon/daemon-init.test.tssrc/main/daemon/daemon-init.tssrc/main/daemon/daemon-login-session-history.test.tssrc/main/daemon/daemon-login-session-history.tssrc/main/daemon/daemon-main.tssrc/main/daemon/daemon-protocol-version.tssrc/main/daemon/daemon-pty-adapter.tssrc/main/daemon/daemon-pty-provider.tssrc/main/daemon/daemon-server.tssrc/main/daemon/macos-gui-session-scope.test.tssrc/main/daemon/macos-gui-session-scope.tssrc/main/daemon/macos-login-session-lifecycle.test.tssrc/main/daemon/types.tssrc/main/index.ts
bd143ab to
2b705a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 78c086b1-5be9-4762-84ca-acb682d78512
📒 Files selected for processing (22)
config/reliability-gates.jsoncdocs/bug-reproductions/issue-7936-macos-login-session-daemon.mdsrc/main/daemon/client.tssrc/main/daemon/daemon-entry.test.tssrc/main/daemon/daemon-entry.tssrc/main/daemon/daemon-health.test.tssrc/main/daemon/daemon-health.tssrc/main/daemon/daemon-hello-protocol.tssrc/main/daemon/daemon-init.test.tssrc/main/daemon/daemon-init.tssrc/main/daemon/daemon-login-session-history.test.tssrc/main/daemon/daemon-login-session-history.tssrc/main/daemon/daemon-main.tssrc/main/daemon/daemon-protocol-version.tssrc/main/daemon/daemon-pty-adapter.tssrc/main/daemon/daemon-pty-provider.tssrc/main/daemon/daemon-server.tssrc/main/daemon/macos-gui-session-scope.test.tssrc/main/daemon/macos-gui-session-scope.tssrc/main/daemon/macos-login-session-lifecycle.test.tssrc/main/daemon/types.tssrc/main/index.ts
🚧 Files skipped from review as they are similar to previous changes (20)
- src/main/daemon/daemon-entry.test.ts
- src/main/daemon/daemon-health.test.ts
- docs/bug-reproductions/issue-7936-macos-login-session-daemon.md
- src/main/daemon/daemon-login-session-history.test.ts
- src/main/daemon/daemon-main.ts
- src/main/index.ts
- src/main/daemon/daemon-login-session-history.ts
- src/main/daemon/daemon-hello-protocol.ts
- src/main/daemon/daemon-pty-provider.ts
- src/main/daemon/daemon-server.ts
- src/main/daemon/daemon-protocol-version.ts
- src/main/daemon/macos-gui-session-scope.test.ts
- src/main/daemon/client.ts
- src/main/daemon/types.ts
- src/main/daemon/daemon-entry.ts
- src/main/daemon/daemon-health.ts
- src/main/daemon/macos-login-session-lifecycle.test.ts
- src/main/daemon/daemon-pty-adapter.ts
- src/main/daemon/daemon-init.test.ts
- src/main/daemon/daemon-init.ts
| async function readLaunchctlProcessDomain(pid: number): Promise<string> { | ||
| const { execFile } = await import('node:child_process') | ||
| return new Promise((resolve, reject) => { | ||
| execFile( | ||
| '/bin/launchctl', | ||
| ['print', `pid/${pid}`], | ||
| { encoding: 'utf8', timeout: 1_000, maxBuffer: 64 * 1024 }, | ||
| (error, stdout) => { | ||
| if (error) { | ||
| reject(error) | ||
| } else { | ||
| resolve(stdout) | ||
| } | ||
| } | ||
| ) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- file map ---'
git ls-files 'src/main/daemon/macos-gui-session-scope.ts' 'src/main/daemon/*' | sed -n '1,120p'
printf '\n%s\n' '--- outline ---'
ast-grep outline src/main/daemon/macos-gui-session-scope.ts --view expanded
printf '\n%s\n' '--- relevant source lines ---'
nl -ba src/main/daemon/macos-gui-session-scope.ts | sed -n '1,220p'
printf '\n%s\n' '--- search for parser/consumer ---'
rg -n "parseMacosProcessSessionIdentity|resolveMacosGuiSessionScope|readLaunchctlProcessDomain|launchctl print|pid/" src/main/daemon src -g '!**/dist/**' -g '!**/build/**'Repository: stablyai/orca
Length of output: 6381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- source: macos-gui-session-scope.ts ---'
sed -n '1,220p' src/main/daemon/macos-gui-session-scope.ts
printf '\n%s\n' '--- tests: macos-gui-session-scope.test.ts ---'
sed -n '1,260p' src/main/daemon/macos-gui-session-scope.test.ts
printf '\n%s\n' '--- call sites ---'
rg -n "readMacosProcessSessionIdentity|resolveMacosGuiSessionScope|parseMacosProcessSessionIdentity|parseMacosGuiSessionScope|readLaunchctlProcessDomain|readMacosBootSessionId" src/main/daemon -g '!**/dist/**' -g '!**/build/**'Repository: stablyai/orca
Length of output: 12671
🌐 Web query:
launchctl print output documented machine-readable stable contract macOS audit session security context
💡 Result:
The output of the launchctl print command on macOS is not a stable, machine-readable contract [1][2][3]. According to the official launchctl(1) manual, the output is intended solely for human inspection during debugging and profiling [4][1][3]. Apple explicitly warns that the output format is not an API and does not provide any guarantees regarding forward compatibility [1][2][3]. The structure, content, and formatting of the output may change at any time across macOS releases without warning [1][2][3]. Consequently, it is strongly advised against relying on parsing this output in production scenarios or automated scripts [1][3]. While some third-party tools exist to parse this data into structured formats like JSON for ad-hoc automation, they are inherently fragile and subject to breaking whenever Apple modifies the launchctl command output [5].
Citations:
- 1: https://ss64.com/mac/launchctl.html
- 2: https://leancrew.com/all-this/man/man1/launchctl.html
- 3: https://www.manpagez.com/man/1/launchctl/osx-10.12.6.php
- 4: https://keith.github.io/xcode-man-pages/launchctl.1.html
- 5: https://github.com/bluuuk/launchctl-dumpstate-json
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- daemon-init excerpt ---'
sed -n '1080,1135p' src/main/daemon/daemon-init.ts
printf '\n%s\n' '--- related error handling ---'
rg -n "Cannot determine the macOS GUI login-session owner|resolveMacosGuiSessionScope|runtimeScope|readMacosProcessSessionIdentity" src/main/daemon/daemon-init.ts src/main/daemon -g '!**/dist/**' -g '!**/build/**'Repository: stablyai/orca
Length of output: 16522
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- daemon-init around startup/resolver ---'
sed -n '720,790p' src/main/daemon/daemon-init.ts
printf '\n%s\n' '--- daemon-init around GUI startup caller ---'
sed -n '1118,1185p' src/main/daemon/daemon-init.ts
printf '\n%s\n' '--- macos-gui-session-scope exact lines ---'
sed -n '1,170p' src/main/daemon/macos-gui-session-scope.tsRepository: stablyai/orca
Length of output: 9708
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- resolveMacosGuiSessionScope references ---'
rg -n "resolveMacosGuiSessionScope\s*\(" src
printf '\n%s\n' '--- readMacosProcessSessionIdentity references ---'
rg -n "readMacosProcessSessionIdentity\s*\(" src
printf '\n%s\n' '--- launchctl print references ---'
rg -n "launchctl'\s*,\s*\['print'|launchctl print|pid/" src/main/daemon src -g '!**/dist/**' -g '!**/build/**'Repository: stablyai/orca
Length of output: 4146
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '630,700p' src/main/index.tsRepository: stablyai/orca
Length of output: 3917
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "function startFirstWindowStartupServices|startFirstWindowStartupServices\(" src/main/index.ts src/main -g '!**/dist/**' -g '!**/build/**'
sed -n '1,220p' src/main/index.tsRepository: stablyai/orca
Length of output: 12648
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,240p' src/main/startup/first-window-startup-services.tsRepository: stablyai/orca
Length of output: 4046
Avoid relying on launchctl print as the only session source. Its output is undocumented and can change; if that happens, GUI-session detection will fail over to the local PTY fallback instead of preserving daemon-backed sessions. Add a compatibility fallback or a more stable source for the audit-session lookup.
|
Reviewed this after dragonfire1119's live test of 1. 2. 3. Unscoped 4. Also, the symptom itself lives below this PR's layer: the #9301 preflight caches its birth-time 'accepted' verdict per daemon process, so a surviving dead-session daemon wraps every spawn in To confirm which link broke on the reporter's machine, their |
|
Data point for the pre-scope adoption guard from a live dead-session experiment (details on #9826): after a real audit-session teardown (SSH-minted session, orphan survived, reparented to launchd), |
|
Closing as superseded by #9826 after discussion with Brennan. Rationale: the live RC test (v1.4.149-rc.1.issue7936) was falsified in the field by the reporter, and the code walk above found four integration holes in the startup-time fence approach — most critically the preserve-deadlock against pre-scope protocol-25 daemons (which 1.4.150-rc.0 ships) and the unscoped-serve vs scoped-daemon replacement war. More fundamentally, a startup-time fence can only help when the app restarts and every adopt-vs-replace decision goes right; the surviving daemon itself stays broken. #9826 inverts the approach: the daemon detects its login session died from the inside (PAM probe + resolver corroboration, hang-shaped fallback trigger) and retires itself, so the next start cold-starts a fresh daemon regardless of any client-side decision — validated with live lifecycle QA and real dead-session experiments (including the launchctl stale-asid data point posted above, which confirms this PR's process-identity guard input was sound). What remains valuable from this PR's scope if we revisit later: the warm-attach fence for fast-user-switching correctness and multi-session isolation, and the login-scoped history slot. If that gets picked back up, the four findings above are the pre-merge checklist. #7936 stays open until #9826 is field-confirmed. |
Summary
Scopes macOS terminal daemons and PTYs to the login runtime that created them.
Closes #7936
Screenshots
No visual change.
Testing
pnpm lint— touched code is clean; the repository-wide command is blocked by the unchanged upstream non-exhaustive switch insrc/renderer/src/components/skills/skill-freshness-group.tsx:101pnpm typecheckpnpm test— sanitized full suite: 3,141 files, 33,353 tests passed; 7 files/56 tests skippedpnpm buildAI Review Report
Ran four review rounds and resolved every valid finding. The review checked macOS lifecycle ownership, full logout, WindowServer recovery, app quit/crash and relaunch, reboot semantics, fast user switching, concurrent clients, reconnect and duplicate-daemon races, stale discovery state, mixed protocol versions, SSH/headless behavior, mobile/web clients, supported providers, and performance.
It explicitly checked cross-platform compatibility for macOS, Linux, Windows, and WSL, including runtime platform guards, paths, shell/process behavior, and Electron IPC boundaries. A valid finding identified a missing server route for closing startup query authority; the route was restored and covered by validation.
Security Audit
Daemon discovery and attachment are fenced by UID, macOS audit-session ID, and boot UUID. Cleanup verifies the exact PID and process start time before termination, uses bounded fixed-path
launchctlandsysctlprobes, and does not log terminal contents. The review also checked IPC authentication, stale metadata, mixed-version fail-closed behavior, multi-user/session isolation, command execution, and path handling. No dependencies or secret handling changed.Notes