fix(simulator): Normalize selector refresh and platform inference

[cameroncooke]

Improve simulator platform inference and session-default selector handling so simulator tools stay correct, deterministic, and non-blocking.

The earlier approach in #196 solved a key part of the problem by introducing build-settings-based detection, but we still had architectural gaps: stale selector/platform state in session defaults, blocking name/id lookups on startup/update paths, and inconsistent normalization between config/session/tool boundaries.

This PR keeps the useful detection direction while broadening the fix to include a runtime-first inference path (simctl metadata first, build settings fallback), cached simulator platform reuse with staleness invalidation, MCP-only hydration behavior, and background refresh with revision guards to avoid stale async writes.

Alternative considered: keep platform detection only in build-settings lookups and leave selector resolution synchronous. That was simpler but still left stale state risks and avoidable latency in the critical path.

Refs #196

---

Note

Medium Risk
Changes simulator destination/platform inference and session-default hydration/refresh behavior, which can affect build/test targeting across iOS/watchOS/tvOS/visionOS. Background refresh with revision-guards reduces stale writes, but introduces new async paths and caching that need coverage (added tests help).

Overview
Simulator tools now infer the correct simulator platform (iOS/watchOS/tvOS/visionOS) using simctl runtime metadata first, then fallback to build-settings detection, with an optional cached simulatorPlatform in session defaults (inferPlatform, platform-detection). build_sim, build_run_sim, and test_sim were updated to use this inference for xcodebuild -destination and logging, enabling non-iOS simulator targeting.

Session defaults handling is reworked to be deterministic for CLI/daemon: bootstrapRuntime only hydrates sessionDefaults into sessionStore for the mcp runtime, and schedules an async simulator metadata refresh instead of blocking on selector resolution. session_set_defaults similarly stops synchronous name/id resolution, clears stale selector/platform fields on change, and schedules a revision-guarded background refresh that can also persist updated simulatorId/simulatorName/simulatorPlatform.

Build execution now resolves relative projectPath/workspacePath/derivedDataPath to absolute paths before invoking xcodebuild, and xcodemake execution no longer mutates process.cwd() (uses child-process cwd instead). Docs timestamps and several fixtures/tests were updated/added to cover the new inference, hydration boundaries, and path/cwd behavior.

^{Written by Cursor Bugbot for commit a0c10d0. This will update automatically on new commits. Configure here.}

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/cameroncooke/XcodeBuildMCP/xcodebuildmcp@206

commit: a0c10d0

[coderabbitai]

Walkthrough

This pull request introduces platform inference logic for Xcode simulators and refactors simulator selector handling. New utility modules enable dynamic platform detection from simulator metadata, build settings, and simulator names. Simulator selector handling is reworked to support both simulatorId and simulatorName with background resolution via a new refresh scheduler. Session bootstrap logic is updated to hydrate session defaults for MCP runtime only. Simulator build, run, and test tools are refactored to use inferred platforms instead of hard-coded values. Session store and schema definitions are extended with simulatorPlatform caching and revision tracking. Corresponding test coverage is added for platform inference, detection, and bootstrap behaviour.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: normalizing simulator selector refresh and platform inference logic.
Description check	✅ Passed	The pull request description clearly explains the changes: improving simulator platform inference and session-default selector handling, with runtime-first inference, cached platform reuse, MCP-only hydration, and background refresh with revision guards.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

🤖 Fix all issues with AI agents

In `@src/mcp/tools/simulator/build_run_sim.ts`:
- Line 471: Remove the emoji from user-facing log/output strings: replace
occurrences like log('info', `✅ ${platformName} simulator build & run
succeeded.`) and the similar string at the other occurrence with plain-text
messages (e.g., "Simulator build & run succeeded" or "Successfully built and ran
the simulator for ${platformName}") so that platformName remains interpolated
but no emoji characters are present; update the strings in build_run_sim.ts
where log(...) and the other user-facing output are created.

In `@src/runtime/bootstrap-runtime.ts`:
- Around line 75-78: hydrateSessionDefaultsForMcp currently may return early
when config.sessionDefaults is empty, but the calling code always logs
"simulator metadata refresh scheduled"; change hydrateSessionDefaultsForMcp to
return a boolean indicating whether it actually scheduled a refresh (true when
defaults present and refresh scheduled, false otherwise) and update the caller
in bootstrap-runtime (the if (opts.runtime === 'mcp') block) to only call
log('info', '[Session] Hydrated MCP session defaults; simulator metadata refresh
scheduled.') when hydrateSessionDefaultsForMcp(config.sessionDefaults) returns
true; ensure the boolean return is documented in the function signature and
handled accordingly by the caller.

In `@src/utils/infer-platform.ts`:
- Around line 152-168: The current resolveProjectFromSession can return both
projectPath and workspacePath when params don't supply either but session
defaults contain both; update resolveProjectFromSession so that when
params.projectPath and params.workspacePath are both undefined and defaults
contain both paths, it prefers workspacePath (sets projectPath to undefined and
workspacePath to defaults.workspacePath) to avoid passing mutually-exclusive
both values into detectPlatformFromScheme; keep existing behavior when one param
is provided or only one default exists, and ensure scheme still falls back to
defaults.scheme.

🧹 Nitpick comments (12)

src/utils/platform-detection.ts (1)

41-51: settingName is internally controlled, but consider a minor hardening.

Static analysis flags a potential ReDoS via new RegExp(...) constructed from a variable. In practice, extractBuildSettingValues is only called with literal strings ('SDKROOT', 'SUPPORTED_PLATFORMS'), so this is safe today. If this helper were ever exported or called with external input, the unescaped settingName would become exploitable. A light safeguard would be to escape the input or mark the function's contract explicitly.

This is not blocking — just a defensive note for future maintainability.

src/utils/__tests__/platform-detection.test.ts (1)

6-115: Good coverage — consider adding a test for the "neither path provided" error path.

The tests thoroughly cover SDKROOT detection, fallback to SUPPORTED_PLATFORMS, non-simulator SDKROOTs, multi-block output, mutual exclusivity, and command failure. One untested branch is when both projectPath and workspacePath are undefined, which returns an error at lines 74–81 of platform-detection.ts.

src/utils/simulator-resolver.ts (1)

75-121: Consider extracting shared simctl device-lookup logic to reduce duplication.

resolveSimulatorIdToName and resolveSimulatorNameToId (lines 20–66) are nearly identical — the only differences are the find predicate and the log messages. A shared internal helper (e.g. findDevice(executor, predicate)) would eliminate the duplicated executor call, JSON parsing, and iteration logic.

♻️ Sketch of a shared helper

+type DevicePredicate = (device: { udid: string; name: string }) => boolean;
+
+async function findSimulatorDevice(
+  executor: CommandExecutor,
+  predicate: DevicePredicate,
+): Promise<{ udid: string; name: string } | { error: string }> {
+  const result = await executor(
+    ['xcrun', 'simctl', 'list', 'devices', 'available', '--json'],
+    'List Simulators',
+    false,
+  );
+  if (!result.success) {
+    return { error: `Failed to list simulators: ${result.error}` };
+  }
+  let simulatorsData: { devices: Record<string, Array<{ udid: string; name: string }>> };
+  try {
+    simulatorsData = JSON.parse(result.output) as typeof simulatorsData;
+  } catch (parseError) {
+    return { error: `Failed to parse simulator list: ${parseError}` };
+  }
+  for (const runtime in simulatorsData.devices) {
+    const match = simulatorsData.devices[runtime].find(predicate);
+    if (match) return match;
+  }
+  return { error: 'not found' };
+}

Both public functions would then delegate to this helper and only handle logging and result-shaping.

src/mcp/tools/simulator/build_sim.ts (1)

104-106: String manipulation for logPrefix is slightly fragile.

detectedPlatform.replace(' Simulator', '') works for all current simulator platform enum values, but silently produces an odd prefix (e.g. "macOS Simulator Build") if inferPlatform ever returns a non-simulator platform. Since this is a simulator-only tool the risk is low, but a minor guard or mapping would make it more robust.

src/utils/__tests__/infer-platform.test.ts (1)

8-224: Missing test coverage for the simulator-name inference source.

The inferPlatform function has a simulator-name path (using inferPlatformFromSimulatorName) that triggers when simctl lookup fails but a recognisable simulator name is provided. None of the current tests exercise this source. Consider adding a test where simctl returns empty devices and a descriptive simulatorName (e.g. "Apple Watch Ultra 2") is provided — the expected result should be { platform: watchOSSimulator, source: 'simulator-name' }.

📝 Suggested test case

+  it('infers platform from simulator name when runtime lookup returns no match', async () => {
+    const mockExecutor: CommandExecutor = async () =>
+      createMockCommandResponse({
+        success: true,
+        output: JSON.stringify({ devices: {} }),
+      });
+
+    const result = await inferPlatform({ simulatorName: 'Apple Watch Ultra 2' }, mockExecutor);
+
+    expect(result.platform).toBe(XcodePlatform.watchOSSimulator);
+    expect(result.source).toBe('simulator-name');
+  });

src/mcp/tools/simulator/__tests__/build_sim.test.ts (1)

187-196: createTrackingExecutor return type is implicit — consider annotating it.

The returned async function is structurally compatible with CommandExecutor but not explicitly typed, which means a future signature change could silently break these tests without a compile-time error.

Minor type annotation

-    function createTrackingExecutor(callHistory: Array<{ command: string[]; logPrefix?: string }>) {
-      return async (command: string[], logPrefix?: string) => {
+    function createTrackingExecutor(callHistory: Array<{ command: string[]; logPrefix?: string }>): CommandExecutor {
+      return async (command, logPrefix) => {

docs/investigations/platform-inference-staleness-and-xor-normalization.md (1)

31-37: Duplicate heading names trigger markdownlint MD024 warnings.

Each domain section reuses the same sub-heading names (Current State (verified), Decision, Required Invariants, etc.). While the structure is clear, this causes MD024 violations and makes anchor-link navigation ambiguous. Consider prefixing sub-headings with the domain name, e.g. ### Domain 1: Current State or using ### level consistently to differentiate.

src/runtime/bootstrap-runtime.ts (1)

36-52: Hydration function does not pass an executor to the background refresh.

scheduleSimulatorDefaultsRefresh is called without an executor property. In this context, refreshSimulatorDefaults will fall back to getDefaultCommandExecutor() (line 47 of simulator-defaults-refresh.ts), which is likely fine for MCP runtime. However, bootstrapRuntime already has a FileSystemExecutor (fs) available. If the intent is to allow test injection of the executor for bootstrap flows, this omission may make the hydration path harder to test in isolation.

src/mcp/tools/session-management/session_set_defaults.ts (1)

131-143: Background refresh is scheduled even when the selector hasn't changed.

When selectorProvided is true but selectorChanged is false (i.e., the user re-sets the same simulator ID/name), a background refresh is still scheduled. This is harmless — it may resolve previously missing counterparts or a missing platform — but it's worth noting as intentional if the goal is self-healing.

src/mcp/tools/simulator/build_run_sim.ts (1)

168-169: Unnecessary intermediate variable platformDestination.

platformDestination on line 168 is simply assigned detectedPlatform with no transformation. You could use detectedPlatform directly in lines 189, 191, and 194, which would reduce cognitive overhead.

src/utils/simulator-defaults-refresh.ts (1)

20-22: Test-environment skip is a pragmatic choice, but consider making it injectable.

shouldSkipBackgroundRefresh checks NODE_ENV and VITEST environment variables to skip scheduling in tests. This works but couples the module to environment variables. If testability becomes a concern later, an injectable skip predicate could be cleaner.

src/utils/infer-platform.ts (1)

170-228: inferPlatformFromSimctl — solid defensive handling, one small robustness nit.

The error handling (simctl failure, JSON parse errors, missing devices key, non-array device lists, non-object entries) is thorough. The availability belt-and-suspenders check on line 221 is a nice touch even though the available filter is already in the command.

One minor point: result.output may be typed as string | undefined. JSON.parse(undefined) throws a SyntaxError that is caught by the catch block, so correctness is fine, but guarding with a fallback would make intent clearer and avoid a misleading stack trace:

-    parsed = JSON.parse(result.output);
+    parsed = JSON.parse(result.output ?? '');

[cameroncooke]

Addressed the open review issues in the current branch:

1. Removed emoji characters from build_run_sim user-facing/log strings.
2. Updated MCP bootstrap hydration flow so hydrateSessionDefaultsForMcp returns a boolean and the "refresh scheduled" log only emits when a refresh is actually scheduled.
3. Updated resolveProjectFromSession to prefer workspacePath when both project/workspace defaults exist and neither is explicitly provided.
4. Fixed session_set_defaults notices so selector clear messages only appear when a counterpart value actually existed; wording no longer claims a selector "changed" unless it did.
5. Removed unnecessary export surface for inferSimulatorSelectorForTool (it is internal-only).

Also added/updated tests for the changed behavior:

• session-defaults notice behavior
• workspace preference in platform inference

Validation run locally:

• npm run lint
• npm run format:check
• npm run typecheck
• npm run test
• npm run build

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.}

[cursor]

New test file uses banned Vitest mocking APIs

Medium Severity

This newly added test file uses vi.hoisted, vi.fn, vi.mock, executorMock.mockReset, executorMock.mockResolvedValue, and executorMock.mockRejectedValue, all of which are banned by the project's testing rules. The rule states: "Ban on Vitest mocking (vi.mock, vi.fn, vi.spyOn, .mock*) ⇒ critical. Use createMockExecutor / createMockFileSystemExecutor." The root cause is that executeXcodemakeCommand calls getDefaultCommandExecutor() internally without accepting an injected executor, forcing the test to use module mocking instead of dependency injection.