Repository navigation
Derive mobile host port from dev tag - #8145
azooz2003-bit wants to merge 5 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughMobile host port selection is centralized in a launch-aware policy, with deterministic DEBUG ports for launch tags, explicit invalid-port handling, service delegation, environment-isolated tests, and physical-device attach endpoint validation. ChangesMobile host port and routing behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MobileHostService
participant MobileHostPortPolicy
participant UserDefaults
MobileHostService->>MobileHostPortPolicy: request configured port with environment
MobileHostPortPolicy->>UserDefaults: read stored iOS pairing port
UserDefaults-->>MobileHostPortPolicy: stored port or missing value
MobileHostPortPolicy-->>MobileHostService: valid port or launch-aware default
Suggested reviewers: 🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 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 `@cmuxTests/MobileHostServiceSettingsTests.swift`:
- Around line 168-187: The tests currently mutate global process environment
through withEnvironmentValue, causing unsafe concurrent behavior. In
cmuxTests/MobileHostServiceSettingsTests.swift:168-187, remove
withEnvironmentValue, update withLaunchTag and withoutLaunchTag to provide
environment dictionaries to their bodies, and update all call sites to pass
those dictionaries into MobileHostService.configuredPort. In
Sources/Mobile/MobileHostService.swift:480-510, add an environment parameter
defaulting to ProcessInfo.processInfo.environment to configuredPort,
resolvedDesiredPort, and defaultConfiguredPort, then propagate it to
taggedDevelopmentDefaultPort.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 80493906-9619-4654-b2be-96944fd50160
📒 Files selected for processing (3)
Sources/Mobile/MobileHostService.swiftcmuxTests/MobileHostAuthorizationTests.swiftcmuxTests/MobileHostServiceSettingsTests.swift
Greptile SummaryThis PR centralizes mobile host port selection into a new
Confidence Score: 5/5Safe to merge: the port selection refactor is self-contained, production behavior is backward-compatible for release and untagged builds, and both previous review concerns have been resolved in this iteration. The new MobileHostPortPolicy correctly threads the environment dictionary from the public API down through the private hash helper, eliminating the ProcessInfo cache problem flagged in earlier review threads. The FNV-1a hash arithmetic is correct, collision avoidance handles boundary cases, and the resolvedDesiredPort nil-on-invalid contract is preserved. Tests now inject environment dictionaries directly rather than via setenv, making them reliable. No correctness or safety issues were found in this pass. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["MobileHostService.configuredPort(defaults:environment:)"] --> B["MobileHostPortPolicy().configuredPort(defaults:environment:)"]
B --> C{Explicit port in UserDefaults?}
C -- "yes, valid (1-65535)" --> D["Return stored port"]
C -- "no / invalid" --> E["defaultConfiguredPort(environment:catalogDefaultPort:)"]
E --> F{DEBUG build?}
F -- "no" --> G["Return catalog default"]
F -- "yes" --> H{CMUX_TAG set and non-default?}
H -- "no" --> G
H -- "yes" --> I["FNV-1a hash of tag → port in 49152–65535"]
I --> J{Port == catalog default?}
J -- "yes" --> K["Shift +1 mod portCount"]
J -- "no" --> L["Return tag-derived port"]
K --> L
M["MobileHostService.resolvedDesiredPort(defaults:environment:)"] --> N["MobileHostPortPolicy().resolvedDesiredPort(defaults:environment:)"]
N --> O{Explicit port in UserDefaults?}
O -- "no" --> E
O -- "yes, valid" --> P["Return stored port"]
O -- "yes, invalid" --> Q["Return nil"]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["MobileHostService.configuredPort(defaults:environment:)"] --> B["MobileHostPortPolicy().configuredPort(defaults:environment:)"]
B --> C{Explicit port in UserDefaults?}
C -- "yes, valid (1-65535)" --> D["Return stored port"]
C -- "no / invalid" --> E["defaultConfiguredPort(environment:catalogDefaultPort:)"]
E --> F{DEBUG build?}
F -- "no" --> G["Return catalog default"]
F -- "yes" --> H{CMUX_TAG set and non-default?}
H -- "no" --> G
H -- "yes" --> I["FNV-1a hash of tag → port in 49152–65535"]
I --> J{Port == catalog default?}
J -- "yes" --> K["Shift +1 mod portCount"]
J -- "no" --> L["Return tag-derived port"]
K --> L
M["MobileHostService.resolvedDesiredPort(defaults:environment:)"] --> N["MobileHostPortPolicy().resolvedDesiredPort(defaults:environment:)"]
N --> O{Explicit port in UserDefaults?}
O -- "no" --> E
O -- "yes, valid" --> P["Return stored port"]
O -- "yes, invalid" --> Q["Return nil"]
Reviews (4): Last reviewed commit: "Use value policy for mobile host ports" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private static func withLaunchTag<T>(_ tag: String, _ body: () throws -> T) rethrows -> T { | ||
| try withEnvironmentValue(SocketControlSettings.launchTagEnvKey, value: tag, body) | ||
| } | ||
|
|
||
| private static func withoutLaunchTag<T>(_ body: () throws -> T) rethrows -> T { | ||
| try withEnvironmentValue(SocketControlSettings.launchTagEnvKey, value: nil, body) | ||
| } | ||
|
|
||
| private static func withEnvironmentValue<T>( | ||
| _ key: String, | ||
| value: String?, | ||
| _ body: () throws -> T | ||
| ) rethrows -> T { | ||
| let previous = getenv(key).map { String(cString: $0) } | ||
| if let value { | ||
| setenv(key, value, 1) | ||
| } else { | ||
| unsetenv(key) | ||
| } | ||
| defer { | ||
| if let previous { | ||
| setenv(key, previous, 1) | ||
| } else { | ||
| unsetenv(key) | ||
| } | ||
| } | ||
| return try body() | ||
| } |
There was a problem hiding this comment.
setenv not visible through ProcessInfo.processInfo.environment cache
withLaunchTag / withoutLaunchTag mutate the C-level environ via setenv/unsetenv, but taggedDevelopmentDefaultPort reads the tag through SocketControlSettings.launchTag(environment:) where the default argument is ProcessInfo.processInfo.environment. On macOS, NSProcessInfo.environment is populated from environ at first access and then cached as an immutable NSDictionary — subsequent setenv calls are not reflected. Because ProcessInfo.processInfo.environment is almost certainly first accessed during test-framework initialization (XCTest reads several env-key indicators during setup), the cache is warm before any test body runs, so withLaunchTag("nodivs") never causes the tag to appear in the dictionary the production code reads.
The established pattern for this codebase is to pass a custom [String: String] directly to the function under test (see every SocketControlSettings test that injects environment:). The environment: parameter on taggedDevelopmentDefaultPort was evidently added with this intent, but because the function is private, the test target cannot reach it, so the setenv workaround was used instead. To correctly cover tag-derived port allocation, configuredPort and resolvedDesiredPort would need to propagate an environment parameter (even if @testable-only-visible as internal) so tests can inject a known dictionary without touching the process environment.
| #if DEBUG | ||
| nonisolated private static func taggedDevelopmentDefaultPort( | ||
| environment: [String: String] = ProcessInfo.processInfo.environment | ||
| ) -> Int? { | ||
| guard let tag = SocketControlSettings.launchTag(environment: environment) else { | ||
| return nil | ||
| } | ||
| let normalizedTag = tag.trimmingCharacters(in: .whitespacesAndNewlines).lowercased() | ||
| guard !normalizedTag.isEmpty, normalizedTag != "default" else { return nil } | ||
|
|
||
| var hash: UInt32 = 2_166_136_261 | ||
| for byte in normalizedTag.utf8 { | ||
| hash ^= UInt32(byte) | ||
| hash &*= 16_777_619 | ||
| } | ||
|
|
||
| let lowerBound = 49_152 | ||
| let upperBound = 65_535 | ||
| let portCount = upperBound - lowerBound + 1 | ||
| var port = lowerBound + Int(hash % UInt32(portCount)) | ||
| if port == CmxMobileDefaults.defaultHostPort { | ||
| port = lowerBound + ((port - lowerBound + 1) % portCount) | ||
| } | ||
| return port | ||
| } | ||
| #endif |
There was a problem hiding this comment.
environment parameter on private production function
taggedDevelopmentDefaultPort accepts an environment: [String: String] parameter that was presumably added to allow dictionary-injection testing. However, no production caller passes a custom environment (both call sites inside defaultConfiguredPort use the default), and the test target cannot reach this private function at all — tests ended up using setenv instead. The parameter is dead code in the production binary and does not enable the testability it implies. It should either be promoted to an internal entry point that threads the environment from configuredPort/resolvedDesiredPort downward, or be removed if the setenv approach is confirmed to work reliably on the target platform.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmuxTests/MobileHostServiceSettingsTests.swift (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unnecessary
@Suite(.serialized).You successfully isolated the test state by using unique
UserDefaultssuites and passing theenvironmentdictionary via dependency injection. Since the tests no longer mutate the global process environment viasetenv, they are fully thread-safe and can run concurrently. Removing.serializedwill improve test execution speed.♻️ Proposed refactor
-@Suite(.serialized)🤖 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 `@cmuxTests/MobileHostServiceSettingsTests.swift` at line 13, Remove the unnecessary .serialized configuration from the test suite annotation in MobileHostServiceSettingsTests, leaving the suite otherwise unchanged so its isolated UserDefaults and injected environment continue to support concurrent execution.
🤖 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.
Outside diff comments:
In `@cmuxTests/MobileHostServiceSettingsTests.swift`:
- Line 13: Remove the unnecessary .serialized configuration from the test suite
annotation in MobileHostServiceSettingsTests, leaving the suite otherwise
unchanged so its isolated UserDefaults and injected environment continue to
support concurrent execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 68a633a6-a5eb-4a15-bfc3-afe346232085
📒 Files selected for processing (3)
Packages/macOS/CmuxSettings/Sources/CmuxSettings/SocketControl/MobileHostPortPolicy.swiftSources/Mobile/MobileHostService.swiftcmuxTests/MobileHostServiceSettingsTests.swift
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
3 participants
Closes #7691
Summary
CMUX_TAGwhen no explicit port is configuredVerification
./scripts/lint-pbxproj-test-wiring.shgit diff --checkswift test --package-path Packages/macOS/CmuxSettings --filter MobileHostPortPolicyTests./scripts/reload.sh --tag mh7691mh7691, verified/tmp/cmux-debug-mh7691.sock,identify,window display "LG HDR 4K", andrpc mobile.host.status {}mh7691and verifiedconfigured_port == port == 59315againDogfood status
http://127.0.0.1:17320/mh7691HostSettingsShortcutNotificationTests.changedSettingsFilePostsOneShortcutNotificationandCmuxCanvasUIsignal 5; failed jobs were rerun and are pending