refactor(7/12): migrate device and macOS tools to event-based handlers#325
refactor(7/12): migrate device and macOS tools to event-based handlers#325cameroncooke wants to merge 1 commit intorefactor/migrate-simulator-toolsfrom
Conversation
|
|
||
| return path.resolve(process.cwd(), pathValue); | ||
| } | ||
| export type { ResolveAppPathFromBuildSettingsParams } from '../../../utils/app-path-resolver.ts'; |
There was a problem hiding this comment.
Dead re-exports in refactored build-settings module
Low Severity
The re-exports of getBuildSettingsDestination, extractAppPathFromBuildSettingsOutput, resolveAppPathFromBuildSettings, and ResolveAppPathFromBuildSettingsParams from app-path-resolver.ts are unused. All consumers in this PR (build_run_device.ts and get_device_app_path.ts) were updated to import directly from app-path-resolver.ts, and a codebase-wide search confirms no other file imports these symbols from build-settings.ts. These re-exports are dead code left over from the migration.
Reviewed by Cursor Bugbot for commit ae0e2ac. Configure here.
| nextStepParams: result.nextStepParams, | ||
| attachments: result.attachments, | ||
| text, | ||
| }; |
There was a problem hiding this comment.
Duplicated runLogic test helper across four files
Low Severity
An identical ~30-line runLogic helper function is copy-pasted across install_app_device.test.ts, launch_app_device.test.ts, stop_app_device.test.ts, and get_device_app_path.test.ts. This helper creates a mock tool handler context, runs the logic, and normalizes the result. Since the project already has a test-helpers.ts utility module (imported in other test files in this PR), this shared adapter belongs there to avoid maintaining four identical copies.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit ae0e2ac. Configure here.
daf00b3 to
d9e9216
Compare
ae0e2ac to
6eabc79
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6eabc79. Configure here.
| scheme: params.scheme, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Missing withErrorHandling wrapper in buildDeviceLogic
Medium Severity
buildDeviceLogic is the only tool handler in this PR that doesn't wrap its logic in withErrorHandling. Every other migrated handler (install_app_device, launch_app_device, stop_app_device, get_device_app_path, build_run_device, list_devices) uses withErrorHandling to catch unexpected exceptions, log them, and emit structured error events. If startBuildPipeline() or any other call before finalizeInlineXcodebuild throws, the error propagates unhandled instead of being gracefully emitted as an error event to the pipeline.
Reviewed by Cursor Bugbot for commit 6eabc79. Configure here.
| import { withErrorHandling } from '../../../utils/tool-error-handling.ts'; | ||
| import { header, statusLine, section } from '../../../utils/tool-event-builders.ts'; | ||
|
|
||
| // Define schema as ZodObject (empty schema since this tool takes no parameters) |
There was a problem hiding this comment.
Dead isAvailableState check after state refactor
Low Severity
isAvailableState still checks for "Available (WiFi)", but the state assignment logic was refactored so this string is never produced. Previously, paired devices without a tunnel connection got state "Available (WiFi)". Now they get "Paired (not connected)", which isAvailableState returns false for. This means WiFi-only devices are silently downgraded from "available" to "not available," which changes the hints/sections shown to the user. If the behavioral change is intentional, the dead "Available (WiFi)" branch in isAvailableState is misleading and worth removing.
Reviewed by Cursor Bugbot for commit 6eabc79. Configure here.
| } | ||
| }, | ||
| { | ||
| header: headerEvent, | ||
| errorMessage: ({ message }: { message: string }) => `Failed to list devices: ${message}`, | ||
| logMessage: ({ message }: { message: string }) => `Error listing devices: ${message}`, | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| export const schema = listDevicesSchema.shape; |
There was a problem hiding this comment.
Bug: The list_devices tool omits setting ctx.nextStepParams when devices are found, which is inconsistent with the previous implementation and the similar list_sims tool, removing follow-up suggestions.
Severity: MEDIUM
Suggested Fix
Reinstate the logic to set ctx.nextStepParams when devices are found, similar to the implementation in list_sims.ts. This would involve adding device-related tool hints to ctx.nextStepParams and updating the corresponding test to expect these parameters instead of undefined.
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/device/list_devices.ts#L247-L351
Potential issue: The refactored `list_devices` tool no longer sets `ctx.nextStepParams`
when devices are available. This is a deviation from its previous behavior and is
inconsistent with other refactored tools in this pull request, such as
`launch_app_device.ts` and `build_device.ts`. Furthermore, the analogous `list_sims.ts`
tool correctly sets `nextStepParams` to suggest follow-up actions. This omission removes
helpful next-step suggestions for the user when interacting with physical devices,
degrading the tool's usability. While a test validates that `nextStepParams` is
`undefined`, this test appears to have been written to match the new, incorrect behavior
rather than to preserve the intended functionality.
Did we get this right? 👍 / 👎 to inform future reviews.
Summary
This is PR 7 of 12 in a stacked PR series that decouples the rendering pipeline from MCP transport. Depends on PR 6 (simulator migrations).
Migrates all device and macOS tool handlers to the new event-based handler contract. Same mechanical transformation pattern as PR 6 but for physical device and macOS desktop targets.
Tools migrated (31 files)
Device tools:
build_device,build_run_device,get_device_app_path,install_app_device,launch_app_device,list_devices,stop_app_device,test_device,build-settingsmacOS tools:
build_macos,build_run_macos,get_mac_app_path,launch_mac_app,stop_mac_app,test_macosNotable changes
test_device.tsandtest_macos.tswere the most complex handlers (~250-280 lines each). They've been significantly simplified by delegating totest-preflight.ts,device-steps.ts/macos-steps.ts, andxcodebuild-pipeline.tsfrom PR 4. The handlers are now thin orchestrators that emit events.ctx.emitthrough to the xcodebuild pipeline for real-time progress streaming.stop_app_device.tsandstop_mac_app.tsupdated to emit structured events for process termination results.Stack navigation
Test plan
npx vitest runpasses -- all device and macOS tool tests updated