fix: read CLI version from the installed app bundle#60
Conversation
Resolve the app bundle from the launched executable path so Homebrew installs report the released version instead of falling back to dev. Add regression tests for bundle discovery and version lookup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds robust CLI version resolution (main bundle, enclosing .app, fallback "dev"); introduces administrator authentication and authenticated sudo execution paths for user isolation operations; adds helper to detect isolation username; updates setup/teardown flows to require authentication; and expands unit tests covering these behaviors. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant SetupWizard
participant UserIsolation as UserIsolationService
participant AuthProcess as sudo (auth)
participant System
Caller->>SetupWizard: runSetup / runTeardown
SetupWizard->>UserIsolation: detectedIsolationUsername(...)
alt username needed
SetupWizard->>UserIsolation: authenticateAdministratorAccess()
UserIsolation->>AuthProcess: makeAdministratorAuthenticationProcess() / run brief auth check
AuthProcess-->>UserIsolation: authentication result
alt authenticated
SetupWizard->>UserIsolation: runAuthenticatedSudoOrThrow(arguments)
UserIsolation->>System: execute privileged operations (useradd, mkdir, chmod, tee, dscl, rm, etc.)
System-->>UserIsolation: operation results
UserIsolation-->>SetupWizard: success/failure
else auth failed
UserIsolation-->>SetupWizard: throw authenticationFailed
end
else no username required
SetupWizard-->>Caller: skip privileged steps
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 (1)
Tests/MacRunnerTests/MacRunnerTests.swift (1)
9-39: Consider adding an assertion to verify the fallback path is actually exercised.The test relies on
Bundle(for: Self.self)not havingCFBundleShortVersionString. If that assumption breaks (e.g., someone adds a version to the test bundle's Info.plist), this test could pass for the wrong reason or fail unexpectedly.An optional improvement would be to explicitly verify the test bundle doesn't return a version, ensuring the fallback path is actually being tested:
💡 Optional: Add assertion for test precondition
+ // Verify the test bundle doesn't have a version, so fallback is exercised + let testBundle = Bundle(for: Self.self) + XCTAssertNil(testBundle.infoDictionary?["CFBundleShortVersionString"] as? String) + let version = CLIHandler.version( executablePath: executableURL.path, - mainBundle: Bundle(for: Self.self) + mainBundle: testBundle )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/MacRunnerTests/MacRunnerTests.swift` around lines 9 - 39, Add an explicit precondition assertion inside testCLIHandlerVersionFallsBackToEnclosingAppBundleVersion to ensure the test bundle has no CFBundleShortVersionString so the fallback is exercised: before calling CLIHandler.version(executablePath:mainBundle:) assert that Bundle(for: Self.self).object(forInfoDictionaryKey: "CFBundleShortVersionString") is nil (or that CLIHandler.version with mainBundle: Bundle(for: Self.self) returns nil) so the test fails if someone later adds a version to the test bundle; keep the assertion right before invoking CLIHandler.version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Tests/MacRunnerTests/MacRunnerTests.swift`:
- Around line 9-39: Add an explicit precondition assertion inside
testCLIHandlerVersionFallsBackToEnclosingAppBundleVersion to ensure the test
bundle has no CFBundleShortVersionString so the fallback is exercised: before
calling CLIHandler.version(executablePath:mainBundle:) assert that Bundle(for:
Self.self).object(forInfoDictionaryKey: "CFBundleShortVersionString") is nil (or
that CLIHandler.version with mainBundle: Bundle(for: Self.self) returns nil) so
the test fails if someone later adds a version to the test bundle; keep the
assertion right before invoking CLIHandler.version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 27c2caab-bbdb-4916-96a5-269ef5ffc0fe
📒 Files selected for processing (2)
Sources/Services/CLIHandler.swiftTests/MacRunnerTests/MacRunnerTests.swift
Prompt for administrator access before running setup or teardown steps, then reuse the cached sudo session for follow-up commands. Also detect orphaned isolation artifacts during teardown so cleanup still works when config is out of sync. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/Services/UserIsolationService.swift (1)
299-304: Consider wrapping inIsolationErrorfor consistency.The helper throws
ProcessExecutorError.executionFailed, which the caller (createUser) catches and re-wraps anyway. ThrowingIsolationErrordirectly would be more consistent with the service's error taxonomy and simplify the call site.♻️ Optional: throw IsolationError directly
private func runAuthenticatedSudoOrThrow(arguments: [String], errorMessage: String) throws { let result = try ProcessExecutor.runSudo(arguments: ["-n"] + arguments) guard result.succeeded else { - throw ProcessExecutorError.executionFailed("\(errorMessage): \(result.output)") + throw IsolationError.userCreationFailed("\(errorMessage): \(result.output)") } }Note: If this helper is intended for broader use (beyond user creation), you could introduce a more generic
IsolationError.commandFailed(String)case instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Services/UserIsolationService.swift` around lines 299 - 304, The helper runAuthenticatedSudoOrThrow currently throws ProcessExecutorError.executionFailed but callers like createUser expect IsolationError; change runAuthenticatedSudoOrThrow to throw an IsolationError (e.g., add or use IsolationError.commandFailed(String) or IsolationError.executionFailed(String)) and map the ProcessExecutor.runSudo failure into that IsolationError including the output/error details so callers no longer need to re-wrap ProcessExecutorError; update any throws signature and tests accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/Services/UserIsolationService.swift`:
- Around line 299-304: The helper runAuthenticatedSudoOrThrow currently throws
ProcessExecutorError.executionFailed but callers like createUser expect
IsolationError; change runAuthenticatedSudoOrThrow to throw an IsolationError
(e.g., add or use IsolationError.commandFailed(String) or
IsolationError.executionFailed(String)) and map the ProcessExecutor.runSudo
failure into that IsolationError including the output/error details so callers
no longer need to re-wrap ProcessExecutorError; update any throws signature and
tests accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9cfde924-b695-4127-9f71-b9f51a83e605
📒 Files selected for processing (3)
Sources/Services/SetupWizard.swiftSources/Services/UserIsolationService.swiftTests/MacRunnerTests/MacRunnerTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Tests/MacRunnerTests/MacRunnerTests.swift
## [1.11.1](v1.11.0...v1.11.1) (2026-03-17) ### Bug Fixes * read CLI version from the installed app bundle ([#60](#60)) ([ee3dc5d](ee3dc5d))
|
🎉 This PR is included in version 1.11.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
Summary
mac-runner versionso Homebrew-installed binaries resolve the enclosingMacRunner.appbundle and print the release version instead ofdevInfo.plistTesting
swiftis unavailable here)Summary by CodeRabbit