fix: make launch at login toggle register app - #77
Conversation
📝 WalkthroughWalkthroughRunnerManager separates login-item reconciliation from applying login-item changes. ContainerIsolationService updates concurrency isolation and container configuration closure handling. An install-source test now controls file existence explicitly. ChangesLogin item synchronization
Container service concurrency
Install-source test control
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.
Actionable comments posted: 1
🤖 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 `@Sources/Services/RunnerManager.swift`:
- Around line 390-392: Update the catch block handling register/unregister
failures in RunnerManager so currentSettings.startOnLogin is reverted to its
prior value and the configuration is saved before recording the error. Preserve
the existing error message while ensuring the UI and persisted settings reflect
the OS state after failure.
🪄 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: fe22f397-2de7-470d-9cd2-63b05968f54e
📒 Files selected for processing (2)
Sources/Services/RunnerManager.swiftTests/MacRunnerTests/MacRunnerTests.swift
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)
Sources/Services/ContainerIsolationService.swift (1)
135-135: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPrevent shell command injection.
Interpolating unescaped variables directly into a shell script makes the script vulnerable to command injection and syntax errors if the variables contain spaces or shell metacharacters.
Wrap the interpolated variables in single quotes to ensure they are treated as literal strings.🛡️ Proposed fix
- ./config.sh --unattended --url \(config.repositoryURL) --token \(config.registrationToken) + ./config.sh --unattended --url '\(config.repositoryURL)' --token '\(config.registrationToken)'🤖 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 `@Sources/Services/ContainerIsolationService.swift` at line 135, Update the command construction in ContainerIsolationService to single-quote the interpolated config.repositoryURL and config.registrationToken arguments passed to ./config.sh, ensuring values with spaces or shell metacharacters remain literal and cannot alter shell execution.
🧹 Nitpick comments (1)
Sources/Services/ContainerIsolationService.swift (1)
145-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove empty conditional block.
The
if config.enableNestedVirtualizationblock is empty. If nested virtualization is intentionally unsupported at the moment, consider replacing this with aTODOcomment or removing the block entirely to avoid linter warnings for empty statements.♻️ Proposed fix
- if config.enableNestedVirtualization { - // Reserved for framework support. - }🤖 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 `@Sources/Services/ContainerIsolationService.swift` around lines 145 - 147, Remove the empty config.enableNestedVirtualization conditional block in the surrounding isolation-service logic; if the unsupported setting must remain documented, replace it with a concise TODO comment without retaining an empty statement.
🤖 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 `@Sources/Services/ContainerIsolationService.swift`:
- Around line 151-157: Serialize concurrent createRunnerContainer operations so
reentrant calls cannot capture and later overwrite stale containerManager state.
Update the container creation flow around manager and the awaited manager.create
call, using an asynchronous queue or equivalent guard, and preserve all manager
mutations from earlier creations.
---
Outside diff comments:
In `@Sources/Services/ContainerIsolationService.swift`:
- Line 135: Update the command construction in ContainerIsolationService to
single-quote the interpolated config.repositoryURL and config.registrationToken
arguments passed to ./config.sh, ensuring values with spaces or shell
metacharacters remain literal and cannot alter shell execution.
---
Nitpick comments:
In `@Sources/Services/ContainerIsolationService.swift`:
- Around line 145-147: Remove the empty config.enableNestedVirtualization
conditional block in the surrounding isolation-service logic; if the unsupported
setting must remain documented, replace it with a concise TODO comment without
retaining an empty statement.
🪄 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: b93ae38d-d643-4d60-aeb2-d1e563a62148
📒 Files selected for processing (3)
Sources/Services/ContainerIsolationService.swiftSources/Services/RunnerManager.swiftTests/MacRunnerTests/UpdateCheckerTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/Services/RunnerManager.swift
| let container = try await manager.create( | ||
| id, | ||
| reference: imageReference, | ||
| rootfsSizeInBytes: config.diskSizeInBytes, | ||
| configuration: configuration | ||
| ) | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Reentrancy hazard causing a lost update on containerManager.
Because ContainerIsolationService is a reentrant @MainActor class, reading containerManager into a local manager variable before an await and reassigning self.containerManager = manager afterward (on line 159) creates a Time-Of-Check to Time-Of-Use (TOCTOU) race condition.
If createRunnerContainer is called concurrently (e.g., when launching multiple runners), the concurrent tasks will capture the same initial containerManager state. When they resume from suspension, the later task will blindly overwrite self.containerManager, destroying the internal state (such as tracked containers or resource allocations) mutated by the earlier task.
Consider ensuring that calls to createRunnerContainer are strictly serialized to prevent reentrancy (e.g., using an asynchronous task queue or an isCreating guard), or use a reference-type manager if the Containerization framework supports it.
🤖 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 `@Sources/Services/ContainerIsolationService.swift` around lines 151 - 157,
Serialize concurrent createRunnerContainer operations so reentrant calls cannot
capture and later overwrite stale containerManager state. Update the container
creation flow around manager and the awaited manager.create call, using an
asynchronous queue or equivalent guard, and preserve all manager mutations from
earlier creations.
|
🎉 This PR is included in version 1.17.4 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
Summary
SMAppService.register()instead of immediately reverting the toggleRoot cause
The settings handler persisted the requested value, then
syncLoginItem()immediately treated the old macOS login-item state as authoritative. Enabling the toggle was overwritten back to false before registration could run.Validation
swift test -Xswiftc -swift-version -Xswiftc 5 --filter 'MacRunnerTests.test(Enabling|Disabling)LaunchAtLogin'(2 passed)\n- full suite: 104 passed; one unrelated environment-sensitive failure intestHomebrewInstallDetectionUsesCellarPathsbecause this machine has the Homebrew cask installed\n-git diff --checkSummary by CodeRabbit