refactor(6/12): migrate simulator tools to event-based handlers#324
Open
cameroncooke wants to merge 1 commit intorefactor/runtime-handler-contractfrom
Open
refactor(6/12): migrate simulator tools to event-based handlers#324cameroncooke wants to merge 1 commit intorefactor/runtime-handler-contractfrom
cameroncooke wants to merge 1 commit intorefactor/runtime-handler-contractfrom
Conversation
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Malformed JSON mock embeds command args in device array
- Removed the incorrectly placed '-derivedDataPath' and DERIVED_DATA_DIR strings from the devices array in both test cases, restoring valid simctl JSON structure.
Or push these changes by commenting:
@cursor push 4dad9e6f53
Preview (4dad9e6f53)
diff --git a/src/mcp/tools/simulator/__tests__/build_run_sim.test.ts b/src/mcp/tools/simulator/__tests__/build_run_sim.test.ts
--- a/src/mcp/tools/simulator/__tests__/build_run_sim.test.ts
+++ b/src/mcp/tools/simulator/__tests__/build_run_sim.test.ts
@@ -194,8 +194,6 @@
state: 'Booted',
isAvailable: true,
},
- '-derivedDataPath',
- DERIVED_DATA_DIR,
],
},
}),
@@ -277,8 +275,6 @@
state: 'Booted',
isAvailable: true,
},
- '-derivedDataPath',
- DERIVED_DATA_DIR,
],
},
}),This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
Reviewed by Cursor Bugbot for commit daf00b3. Configure here.
Comment on lines
-127
to
-134
| sharedBuildParams, | ||
| { | ||
| platform: detectedPlatform, | ||
| simulatorId: params.simulatorId, | ||
| simulatorName: params.simulatorName, | ||
| useLatestOS: params.simulatorId ? false : params.useLatestOS, | ||
| logPrefix, | ||
| }, |
There was a problem hiding this comment.
Bug: The inferPlatform function is no longer passed the simulatorId, which will cause platform detection to fail for non-iOS simulators specified by UUID.
Severity: HIGH
Suggested Fix
Pass the simulatorId to the inferPlatform function call to ensure it can correctly infer the platform for simulators identified by their UUID.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/mcp/tools/simulator/build_run_sim.ts#L127-L134
Potential issue: The `inferPlatform` function is now called without the `simulatorId`.
When a simulator is identified by its UUID, this change prevents the correct platform
(like watchOS, tvOS, or visionOS) from being detected. As a result, the system will
default to iOS, leading to build or run failures because the wrong destination platform
is used.
Did we get this right? 👍 / 👎 to inform future reviews.
…ased handler contract
089f6a3 to
fa1ece2
Compare
daf00b3 to
d9e9216
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This is PR 6 of 12 in a stacked PR series that decouples the rendering pipeline from MCP transport. Depends on PR 5 (handler contract change).
Migrates all simulator and simulator-management tool handlers from the old
return toolResponse([...])pattern to the newctx.emit(event)pattern introduced in PR 5. This is the largest tool migration PR because simulators are the primary development target.Tools migrated (36 files)
Simulator tools:
boot_sim,build_sim,build_run_sim,get_sim_app_path,install_app_sim,launch_app_sim,list_sims,open_sim,record_sim_video,stop_app_sim,test_sim,screenshotSimulator management tools:
erase_sims,reset_sim_location,set_sim_appearance,set_sim_location,sim_statusbarPattern of change
Each handler follows the same mechanical transformation:
```typescript
// Before
async function handler(params) {
const result = await buildForSimulator(params);
return toolResponse([header('Build', [...]), statusLine('success', '...')]);
}
// After
async function handler(params, ctx) {
const result = await buildForSimulator(params, ctx);
ctx.emit(header('Build', [...]));
ctx.emit(statusLine('success', '...'));
}
```
Build/test tools additionally pass
ctx.emitto the xcodebuild pipeline so events stream in real-time during compilation.test_simdelegates tosimulator-test-execution.tsandtest-preflight.tsfrom PR 4.Supporting changes
simulator-utils.ts: Updated to work with the new handler contextsimulator-resolver.ts: Simplified resolver that integrates with the step modules from PR 4Stack navigation
Test plan
npx vitest runpasses -- all simulator tool tests updated