Add session-aware simulator runtime tooling#133
Conversation
Use session-aware factory across simulator runtime tools, consolidate simulatorId defaults, and update docs/tests to match.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughMigrates simulator tools to a session-aware pattern: replaces Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as Session-Aware Handler
participant SessionStore as Session Store
participant Logic as Tool Logic
participant Executor as Command Executor
Client->>Handler: Invoke tool with public params (may omit simulatorId)
Handler->>SessionStore: Fetch session defaults
SessionStore-->>Handler: Return default `simulatorId` (if present)
Handler->>Handler: Merge defaults into internal params
Handler->>Logic: Call logicFunction with internal params (includes simulatorId)
Logic->>Executor: Run simctl / xcrun commands using simulatorId
Executor-->>Logic: Command result
Logic-->>Handler: ToolResponse (omitting session-managed fields)
Handler-->>Client: Response (conforms to public schema)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
|
@cursor review |
commit: |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/mcp/tools/simulator/launch_app_sim.ts (1)
168-185: Next-steps hint may be wrong today for start_sim_log_cap.Docs/TODO show start_sim_log_cap not yet migrated (still using simulatorUuid). The hint here recommends simulatorId/simulatorName, which could mislead users. Either:
- Migrate start_sim_log_cap to accept simulatorId (and add alias hydration), or
- Temporarily adjust the hint to mention simulatorUuid until that migration lands.
This is a cross-tool UX mismatch.
src/mcp/tools/simulator/install_app_sim.ts (1)
36-45: Return isError: true on failures for consistency.Other tools surface isError on failure; align install_app_sim to simplify callers and tests.
if (!result.success) { return { content: [ { type: 'text', text: `Install app in simulator operation failed: ${result.error}`, }, ], + isError: true, }; } ... return { content: [ { type: 'text', text: `Install app in simulator operation failed: ${errorMessage}`, }, ], + isError: true, };Also applies to: 79-88
src/mcp/tools/simulator/boot_sim.ts (1)
29-38: Surface isError on failure paths.Standardize error shape with other tools.
if (!result.success) { return { content: [ { type: 'text', text: `Boot simulator operation failed: ${result.error}`, }, ], + isError: true, }; } ... return { content: [ { type: 'text', text: `Boot simulator operation failed: ${errorMessage}`, }, ], + isError: true, };Also applies to: 57-63
🧹 Nitpick comments (22)
src/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.ts (2)
66-105: Type the captured params for stronger guarantees.Use the actual parameter type instead of unknown to catch regressions at compile time.
Apply:
- let capturedParams: unknown = null; + let capturedParams: Parameters<LogCaptureFunction>[0] | null = null; - const logCaptureStub: LogCaptureFunction = async (params) => { + const logCaptureStub: LogCaptureFunction = async (params, _executor) => { capturedParams = params; return { sessionId: 'test-session-123', logFilePath: '/tmp/xcodemcp_sim_log_test-session-123.log', processes: [], error: undefined, }; };
107-136: Mirror the typing improvement in the “ignore args” test.Same rationale as above; also explicitly accept the unused executor arg.
Apply:
- let capturedParams: unknown = null; + let capturedParams: Parameters<LogCaptureFunction>[0] | null = null; - const logCaptureStub: LogCaptureFunction = async (params) => { + const logCaptureStub: LogCaptureFunction = async (params, _executor) => { capturedParams = params; return { sessionId: 'test-session-456', logFilePath: '/tmp/xcodemcp_sim_log_test-session-456.log', processes: [], error: undefined, }; };src/mcp/tools/simulator/launch_app_logs_sim.ts (2)
32-46: Param-name bridge is fine; consider clarifying the type boundary.You map simulatorId → simulatorUuid for the capture layer. To reduce confusion:
- Either rename the field in LogCaptureFunction to simulatorId (if upstream allows), or
- Mark LogCaptureFunction as internal (not exported) and type it via a local Params alias.
Apply (if keeping it internal):
-export type LogCaptureFunction = ( +type LogCaptureFunction = ( params: { simulatorUuid: string; bundleId: string; captureConsole?: boolean; }, executor: CommandExecutor, ) => Promise<{ sessionId: string; logFilePath: string; processes: unknown[]; error?: string }>;And stop exporting it from the module’s public surface (tests can still import the type via typeof startLogCapture or a dedicated exported type alias if needed).
54-61: Optional: include log file hint in success message.If available from startLogCapture, consider showing the log file path to aid discovery.
Example:
- `App launched successfully in simulator ${params.simulatorId} with log capture enabled.\n\nLog capture session ID: ${sessionId}\n\nNext Steps:\n1. Interact with your app in the simulator.\n2. Use 'stop_and_get_simulator_log({ logSessionId: "${sessionId}" })' to stop capture and retrieve logs.`, + `App launched successfully in simulator ${params.simulatorId} with log capture enabled.\n\nLog capture session ID: ${sessionId}\nNext Steps:\n1. Interact with your app in the simulator.\n2. Use 'stop_and_get_simulator_log({ logSessionId: "${sessionId}" })' to stop capture and retrieve logs.`,(and optionally append the log file path if surfaced by the capture layer)
src/mcp/tools/simulator/__tests__/record_sim_video.test.ts (1)
5-7: Use createMockExecutor instead of ad‑hoc stub for the command executor.Align tests with the project’s testing pattern.
Apply:
-import { createMockFileSystemExecutor } from '../../../../test-utils/mock-executors.ts'; +import { createMockExecutor, createMockFileSystemExecutor } from '../../../../test-utils/mock-executors.ts'; -const DUMMY_EXECUTOR: any = (async () => ({ success: true })) as any; // CommandExecutor stub +const EXECUTOR = createMockExecutor(); - DUMMY_EXECUTOR, + EXECUTOR, - DUMMY_EXECUTOR, + EXECUTOR, - DUMMY_EXECUTOR, + EXECUTOR, - DUMMY_EXECUTOR, + EXECUTOR,As per coding guidelines • Based on learnings
Also applies to: 63-74, 114-123, 128-138, 173-182
src/mcp/tools/simulator/record_sim_video.ts (3)
29-34: Coerce fps to number to be CLI‑friendly.Accept string inputs like "30" from CLI/env without failing schema.
Apply:
- fps: z.number().int().min(1).max(120).optional().describe('Frames per second (default 30)'), + fps: z.coerce.number().int().min(1).max(120).optional().describe('Frames per second (default 30)'),
23-27: Tighten simulatorId validation message wording.Match param name to error text.
Apply:
- simulatorId: z - .string() - .uuid('Invalid Simulator UUID format') - .describe('UUID of the simulator to record'), + simulatorId: z + .string() + .uuid('Invalid simulatorId (UUID) format') + .describe('UUID of the simulator to record'),
229-234: Avoid double type cast on internalSchema.Replace
as unknown aswith a typed constant for clarity and safer typing.Apply:
+// Keep schema strongly typed without double casting +const internalRecordSimVideoSchema: z.ZodType<RecordSimVideoParams> = recordSimVideoSchema; ... handler: createSessionAwareTool<RecordSimVideoParams>({ - internalSchema: recordSimVideoSchema as unknown as z.ZodType<RecordSimVideoParams>, + internalSchema: internalRecordSimVideoSchema, logicFunction: record_sim_videoLogic, getExecutor: getDefaultCommandExecutor, requirements: [{ allOf: ['simulatorId'], message: 'simulatorId is required' }], }),src/mcp/tools/simulator/launch_app_sim.ts (3)
30-36: Redundant XOR checks; keep one source of truth.You enforce simulatorId/simulatorName XOR both in the factory (exclusivePairs) and here via Zod refinements. Consider removing one to avoid drift and double maintenance. The factory-level check already blocks explicit double-provide and prunes session defaults.
47-95: Name lookup: handle duplicate names and improve matching robustness.If multiple devices share the same name across runtimes, first-match may be surprising. Optionally prefer a booted device, most recent runtime, or return a disambiguation list. You could also case-normalize for friendlier matching.
157-166: Failure return lacks isError flag; align with other error branches.Earlier failures set isError: true; this branch does not. Standardize so clients can reliably detect errors.
if (!result.success) { return { content: [ { type: 'text', text: `Launch app in simulator operation failed: ${result.error}`, }, ], + isError: true, }; }docs/session-aware-migration-todo.md (1)
36-41: Checklist updates look consistent.Runtime actions marked complete and note hydration of legacy simulatorUuid. Consider adding a short legend explaining “hydrate simulatorUuid” to reduce ambiguity.
src/mcp/tools/simulator/__tests__/launch_app_sim.test.ts (2)
193-229: Add assertion for isError once handler standardizes failure returns.If you adopt the isError standardization in the tool, mirror it here to assert isError: true on launch failure.
231-292: Add a test for empty-string sanitization.Since nullifyEmptyStrings is used, cover a case like simulatorName: '' with a valid session default simulatorId to ensure the handler resolves correctly.
src/mcp/tools/simulator/__tests__/boot_sim.test.ts (1)
120-151: Nice command-capture verification.Captures all executor args including allowStderr/timeout; keeps tests hermetic. Consider adding one test where simulatorId comes solely from session defaults to exercise the session-aware path.
src/mcp/tools/simulator/__tests__/stop_app_sim.test.ts (2)
28-36: Add a test for conflicting session defaults only (no explicit args).Covers the path where both simulatorId and simulatorName exist in session defaults and schema refinement triggers.
+ it('should reject when session defaults contain both simulatorId and simulatorName', async () => { + sessionStore.setDefaults({ simulatorId: 'SIM-UUID', simulatorName: 'iPhone 16' } as any); + const result = await plugin.handler({ bundleId: 'com.example.app' }); + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain('Parameter validation failed'); + expect(result.content[0].text).toMatch(/mutually exclusive/i); + });
87-112: Consider a shared sequenced/tracking executor helper for reuse.Inline executors are fine, but a small test util (e.g., createSequencedExecutor) would DRY patterns across suites.
Also applies to: 233-271
src/mcp/tools/simulator/install_app_sim.ts (2)
49-57: Prefer Info.plist; keep a legacy fallback.defaults works more reliably against Info.plist. Add a fallback to current path for compatibility.
- const bundleIdResult = await executor( - ['defaults', 'read', `${params.appPath}/Info`, 'CFBundleIdentifier'], - 'Extract Bundle ID', - false, - undefined, - ); - if (bundleIdResult.success) { - bundleId = bundleIdResult.output.trim(); - } + const plistPath = `${params.appPath}/Info.plist`; + const bundleIdResult = await executor( + ['defaults', 'read', plistPath, 'CFBundleIdentifier'], + 'Extract Bundle ID', + false, + undefined, + ); + if (bundleIdResult.success) { + bundleId = bundleIdResult.output.trim(); + } else { + const legacyResult = await executor( + ['defaults', 'read', `${params.appPath}/Info`, 'CFBundleIdentifier'], + 'Extract Bundle ID (legacy path)', + false, + undefined, + ); + if (legacyResult.success) bundleId = legacyResult.output.trim(); + }
96-101: Avoid potential function‑type mismatch under stricter TS.Wrap logicFunction to match the two‑arg signature expected by the factory. Runtime stays identical.
- handler: createSessionAwareTool<InstallAppSimParams>({ - internalSchema: installAppSimSchemaObject, - logicFunction: install_app_simLogic, + handler: createSessionAwareTool<InstallAppSimParams>({ + internalSchema: installAppSimSchemaObject, + logicFunction: (p, ex) => install_app_simLogic(p, ex), getExecutor: getDefaultCommandExecutor, requirements: [{ allOf: ['simulatorId'], message: 'simulatorId is required' }], }),src/mcp/tools/simulator/boot_sim.ts (1)
43-50: Optional: unify open_sim invocation format.Elsewhere messages use open_sim({}). Consider matching for consistency.
- text: `✅ Simulator booted successfully. To make it visible, use: open_sim() + text: `✅ Simulator booted successfully. To make it visible, use: open_sim({}) ... -1. Open the Simulator app (makes it visible): open_sim() +1. Open the Simulator app (makes it visible): open_sim({})src/mcp/tools/simulator/stop_app_sim.ts (2)
27-33: Consolidate XOR validation to one place to avoid double messaging.You enforce exclusivity both in schema refinement and factory.exclusivePairs. Consider dropping the schema-level refine and rely on factory + requirements for a single, consistent error surface.
Also applies to: 159-166
47-52: Add a modest timeout to external calls.Prevents hangs if simctl stalls. 30s is a reasonable default.
- const simulatorListResult = await executor( + const simulatorListResult = await executor( ['xcrun', 'simctl', 'list', 'devices', 'available', '--json'], 'List Simulators', - true, + true, + 30_000, ); ... - const result = await executor(command, 'Stop App in Simulator', true, undefined); + const result = await executor(command, 'Stop App in Simulator', true, 30_000);Also applies to: 109-111
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
docs/RELOADEROO_FOR_XCODEBUILDMCP.md(2 hunks)docs/session-aware-migration-todo.md(1 hunks)src/mcp/tools/simulator/__tests__/boot_sim.test.ts(7 hunks)src/mcp/tools/simulator/__tests__/install_app_sim.test.ts(10 hunks)src/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.ts(6 hunks)src/mcp/tools/simulator/__tests__/launch_app_sim.test.ts(8 hunks)src/mcp/tools/simulator/__tests__/record_sim_video.test.ts(7 hunks)src/mcp/tools/simulator/__tests__/stop_app_sim.test.ts(4 hunks)src/mcp/tools/simulator/boot_sim.ts(3 hunks)src/mcp/tools/simulator/install_app_sim.ts(4 hunks)src/mcp/tools/simulator/launch_app_logs_sim.ts(4 hunks)src/mcp/tools/simulator/launch_app_sim.ts(7 hunks)src/mcp/tools/simulator/record_sim_video.ts(6 hunks)src/mcp/tools/simulator/stop_app_sim.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.ts: Do not use Vitest mocking APIs in tests (no vi.mock, vi.fn, vi.spyOn, etc.)
In tests, import the core ...Logic function from the tool and use createMockExecutor/createMockFileSystemExecutor to simulate outcomes
Files:
src/mcp/tools/simulator/__tests__/record_sim_video.test.tssrc/mcp/tools/simulator/__tests__/launch_app_sim.test.tssrc/mcp/tools/simulator/__tests__/install_app_sim.test.tssrc/mcp/tools/simulator/__tests__/stop_app_sim.test.tssrc/mcp/tools/simulator/__tests__/boot_sim.test.tssrc/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: Use .ts extensions for all internal relative imports (e.g., import { tool } from './tool.ts')
Use .ts extensions in re-exports for internal files (e.g., export { default } from '../shared/tool.ts')
Use .js extension only for external package entry points (e.g., import ... from '@scope/pkg/file.js')
Never import internal project files using .js extensions
Files:
src/mcp/tools/simulator/__tests__/record_sim_video.test.tssrc/mcp/tools/simulator/__tests__/launch_app_sim.test.tssrc/mcp/tools/simulator/stop_app_sim.tssrc/mcp/tools/simulator/launch_app_logs_sim.tssrc/mcp/tools/simulator/launch_app_sim.tssrc/mcp/tools/simulator/__tests__/install_app_sim.test.tssrc/mcp/tools/simulator/__tests__/stop_app_sim.test.tssrc/mcp/tools/simulator/boot_sim.tssrc/mcp/tools/simulator/__tests__/boot_sim.test.tssrc/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.tssrc/mcp/tools/simulator/install_app_sim.tssrc/mcp/tools/simulator/record_sim_video.ts
🧠 Learnings (1)
📚 Learning: 2025-09-21T19:27:22.795Z
Learnt from: CR
PR: cameroncooke/XcodeBuildMCP#0
File: AGENTS.md:0-0
Timestamp: 2025-09-21T19:27:22.795Z
Learning: Applies to **/*.{test,spec}.ts : In tests, import the core ...Logic function from the tool and use createMockExecutor/createMockFileSystemExecutor to simulate outcomes
Applied to files:
src/mcp/tools/simulator/__tests__/record_sim_video.test.tssrc/mcp/tools/simulator/__tests__/install_app_sim.test.tssrc/mcp/tools/simulator/__tests__/boot_sim.test.tssrc/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.ts
🧬 Code graph analysis (11)
src/mcp/tools/simulator/__tests__/launch_app_sim.test.ts (3)
src/utils/session-store.ts (1)
sessionStore(47-47)src/mcp/tools/simulator/launch_app_sim.ts (1)
launch_app_simLogic(40-201)src/test-utils/mock-executors.ts (1)
createMockExecutor(27-75)
src/mcp/tools/simulator/stop_app_sim.ts (2)
src/utils/schema-helpers.ts (1)
nullifyEmptyStrings(14-24)src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/simulator/launch_app_logs_sim.ts (4)
src/utils/execution/index.ts (2)
CommandExecutor(8-8)getDefaultCommandExecutor(5-5)src/utils/log-capture/index.ts (1)
startLogCapture(1-1)src/utils/responses/index.ts (1)
ToolResponse(15-15)src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/simulator/launch_app_sim.ts (2)
src/utils/schema-helpers.ts (1)
nullifyEmptyStrings(14-24)src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/simulator/__tests__/install_app_sim.test.ts (3)
src/utils/session-store.ts (1)
sessionStore(47-47)src/test-utils/mock-executors.ts (1)
createMockFileSystemExecutor(165-181)src/mcp/tools/simulator/install_app_sim.ts (1)
install_app_simLogic(20-90)
src/mcp/tools/simulator/__tests__/stop_app_sim.test.ts (3)
src/utils/session-store.ts (1)
sessionStore(47-47)src/test-utils/mock-executors.ts (1)
createMockExecutor(27-75)src/mcp/tools/simulator/stop_app_sim.ts (1)
stop_app_simLogic(37-147)
src/mcp/tools/simulator/boot_sim.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/simulator/__tests__/boot_sim.test.ts (3)
src/utils/session-store.ts (1)
sessionStore(47-47)src/test-utils/mock-executors.ts (1)
createMockExecutor(27-75)src/mcp/tools/simulator/boot_sim.ts (1)
boot_simLogic(19-65)
src/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.ts (3)
src/utils/session-store.ts (1)
sessionStore(47-47)src/mcp/tools/simulator/launch_app_logs_sim.ts (2)
LogCaptureFunction(9-16)launch_app_logs_simLogic(32-62)src/test-utils/mock-executors.ts (1)
createMockExecutor(27-75)
src/mcp/tools/simulator/install_app_sim.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/simulator/record_sim_video.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: code-review
🔇 Additional comments (18)
src/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.ts (5)
13-19: Good session hygiene in tests.Clearing sessionStore in beforeEach avoids cross-test leakage. LGTM.
28-39: Public schema shape assertion is on point.Asserting only ['args','bundleId'] are exposed matches the session-aware design that hides simulatorId. LGTM.
42-50: Error-path expectations align with session requirements.Validates “Missing required session defaults” and guidance to use session-set-defaults. Solid.
52-63: Validation messaging checks are precise.Confirms Zod error formatting and tips from createSessionAwareTool when bundleId is absent. LGTM.
138-166: Failure-path assertion looks correct.Covers the branch where log capture returns an error and surfaces it. LGTM.
src/mcp/tools/simulator/launch_app_logs_sim.ts (1)
67-73: Session-aware handler wiring looks correct.
- Uses publicSchemaObject.shape for exposure.
- createSessionAwareTool with internalSchema and simulatorId requirement.
LGTM.src/mcp/tools/simulator/__tests__/record_sim_video.test.ts (1)
15-36: Validation and logic coverage looks solid.Good mutual‑exclusivity, stop+rename flow, and version gate assertions. No issues found.
Also applies to: 63-85, 88-147, 151-187
src/mcp/tools/simulator/record_sim_video.ts (1)
221-235: Public/private schema split and session‑aware handler wiring look correct.Omitting
simulatorIdfrom public schema while enforcing via session requirements is consistent with the new session defaults model.docs/RELOADEROO_FOR_XCODEBUILDMCP.md (2)
63-65: Param rename looks good.Example now correctly uses simulatorId.
112-115: No action required on the documented parameter; clarify the migration transition state instead.The documented parameter
simulatorUuidis correct—start_sim_log_caphas not yet been migrated to usesimulatorId(marked as[ ]indocs/session-aware-migration-todo.md). However, the review comment's concern about user confusion is valid: other simulator tools in the docs usesimulatorId(migrated tools likelaunch_app_logs_sim), while logging and UI-testing tools still usesimulatorUuid(not yet migrated).Rather than changing the parameter name, add a brief note to clarify that logging and UI-testing tools are in transition:
- **`start_sim_log_cap`**: Starts log capture for a simulator. ```bash npx reloaderoo@latest inspect call-tool start_sim_log_cap --params '{"simulatorUuid": "SIMULATOR_UUID", "bundleId": "com.example.MyApp"}' -- node build/index.jsNote: Logging tools currently use
simulatorUuid; other simulator tools usesimulatorIdafter recent migrations.This prevents user confusion without creating incorrect documentation. </blockquote></details> <details> <summary>src/mcp/tools/simulator/__tests__/launch_app_sim.test.ts (1)</summary><blockquote> `18-39`: **Public schema tests: nice coverage.** Validates only non-session fields are exposed and required bundleId. Good alignment with session-aware design. </blockquote></details> <details> <summary>src/mcp/tools/simulator/__tests__/boot_sim.test.ts (1)</summary><blockquote> `33-41`: **Good handler requirement assertions.** Clear expectations for session-default errors and guidance text. </blockquote></details> <details> <summary>src/mcp/tools/simulator/__tests__/stop_app_sim.test.ts (2)</summary><blockquote> `8-10`: **Good session hygiene in tests.** Resetting session defaults in beforeEach avoids cross-test coupling. --- `51-62`: **Great mutual‑exclusivity coverage.** Assertions align with factory’s early XOR check and messaging. </blockquote></details> <details> <summary>src/mcp/tools/simulator/stop_app_sim.ts (1)</summary><blockquote> `124-133`: **LGTM: clear success surface and name/UUID echo.** Message formatting and simulator resolution UX read well. </blockquote></details> <details> <summary>src/mcp/tools/simulator/__tests__/install_app_sim.test.ts (3)</summary><blockquote> `21-33`: **Public shape assertions look right.** Only appPath exposed publicly; matches session‑aware design. --- `37-45`: **Handler requirements test aligns with session defaults flow.** Good expectation on ‘Missing required session defaults’. --- `219-249`: **Update expectations to include isError: true (after logic fix).** Once install_app_sim returns isError on failures, adjust tests accordingly. ```diff expect(result).toEqual({ content: [ { type: 'text', text: 'Install app in simulator operation failed: Install failed', }, ], -}); + isError: true, +});expect(result).toEqual({ content: [ { type: 'text', text: 'Install app in simulator operation failed: Command execution failed', }, ], -}); + isError: true, +});expect(result).toEqual({ content: [ { type: 'text', text: 'Install app in simulator operation failed: String error', }, ], -}); + isError: true, +});Also applies to: 251-275, 277-301
| @@ -1,12 +1,11 @@ | |||
| import { describe, it, expect, vi, afterEach } from 'vitest'; | |||
There was a problem hiding this comment.
Remove Vitest mocking API usage (no vi.* in tests).
Guideline breach: tests must not use Vitest mocking APIs. Drop vi import and afterEach(vi.restoreAllMocks).
Apply:
-import { describe, it, expect, vi, afterEach } from 'vitest';
+import { describe, it, expect } from 'vitest';
-afterEach(() => {
- vi.restoreAllMocks();
-});As per coding guidelines • Based on learnings
Also applies to: 10-12
🤖 Prompt for AI Agents
In src/mcp/tools/simulator/__tests__/record_sim_video.test.ts around line 1 (and
also lines ~10-12), the test imports and uses Vitest's mocking API (vi) and
calls afterEach(vi.restoreAllMocks); remove the vi import from the import list
and delete the afterEach(vi.restoreAllMocks) call. If cleanup of side effects is
still required, replace with explicit manual cleanup code or framework-agnostic
helpers, but do not reintroduce any vi.* usage.
| const launchAppLogsSimSchemaObject = z.object({ | ||
| simulatorId: z.string().describe('UUID of the simulator to target'), | ||
| bundleId: z | ||
| .string() | ||
| .describe("Bundle identifier of the app to launch (e.g., 'com.example.MyApp')"), | ||
| args: z.array(z.string()).optional().describe('Additional arguments to pass to the app'), | ||
| }); |
There was a problem hiding this comment.
Strengthen input validation for empty strings.
Currently z.string() accepts ''. An empty simulatorId from session defaults won’t be sanitized and will pass validation.
Apply:
-const launchAppLogsSimSchemaObject = z.object({
- simulatorId: z.string().describe('UUID of the simulator to target'),
+const launchAppLogsSimSchemaObject = z.object({
+ simulatorId: z.string().min(1, 'Required').describe('UUID of the simulator to target'),
bundleId: z
- .string()
+ .string()
+ .min(1, 'Required')
.describe("Bundle identifier of the app to launch (e.g., 'com.example.MyApp')"),
args: z.array(z.string()).optional().describe('Additional arguments to pass to the app'),
});Also applies to: 26-31
🤖 Prompt for AI Agents
In src/mcp/tools/simulator/launch_app_logs_sim.ts around lines 18 to 24 (and
also apply the same change to lines 26 to 31), the z.string() schema allows
empty strings so an empty simulatorId or bundleId can pass validation; update
the schema to reject empty strings by replacing z.string() with
z.string().nonempty() (or z.string().min(1)) and add a clear error message via
.nonempty({ message: '...'}), applying the same nonempty constraint to bundleId
and any other string fields in the indicated lines.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const userParamName = params.simulatorUuid ? 'simulatorUuid' : 'simulatorName'; | ||
| const userParamValue = params.simulatorUuid ?? params.simulatorName; | ||
| const userParamName = params.simulatorId | ||
| ? 'simulatorId' |
There was a problem hiding this comment.
Switching this helper to emit simulatorId means the success message now tells callers to run start_sim_log_cap({ simulatorId: … }). However, start_sim_log_cap (see src/mcp/tools/logging/start_sim_log_cap.ts) still validates a simulatorUuid parameter. Following the suggested Next Steps now fails with Parameter validation failed: simulatorUuid: Required. Please keep emitting simulatorUuid here (or update the logging tool in the same change) so the follow-up workflow continues to work.
Revert launch hints to emit simulatorUuid until logging tools migrate to session defaults, and align test expectations.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcp/tools/simulator/launch_app_sim.ts (1)
168-181: Fix parameter name mismatch in success message.Lines 168–169 conditionally emit either
simulatorNameorsimulatorUuidin the Next Steps, butstart_sim_log_caponly acceptssimulatorUuidas a parameter. Whenparams.simulatorNameis provided, the suggested command fails validation.Change line 168 to always use
'simulatorUuid':const userParamName = 'simulatorUuid';Line 169 should remain as
userParamValue = params.simulatorName ?? simulatorId(which resolves tosimulatorIdin both cases).
🧹 Nitpick comments (1)
src/mcp/tools/simulator/launch_app_sim.ts (1)
208-216: Session-aware migration looks correct, but verify the type cast.The handler correctly uses
createSessionAwareToolwith properrequirementsandexclusivePairsenforcement. However, line 209's double cast (as unknown as z.ZodType<LaunchAppSimParams>) bypasses type safety. This is needed becauselaunchAppSimSchema(with.refine()) is aZodEffects, not a plainZodType.Consider updating
createSessionAwareTool's type signature to acceptZodEffectsor use a more precise type to avoid the cast.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/mcp/tools/simulator/__tests__/launch_app_sim.test.ts(8 hunks)src/mcp/tools/simulator/launch_app_sim.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mcp/tools/simulator/tests/launch_app_sim.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: Use .ts extensions for all internal relative imports (e.g., import { tool } from './tool.ts')
Use .ts extensions in re-exports for internal files (e.g., export { default } from '../shared/tool.ts')
Use .js extension only for external package entry points (e.g., import ... from '@scope/pkg/file.js')
Never import internal project files using .js extensions
Files:
src/mcp/tools/simulator/launch_app_sim.ts
🧬 Code graph analysis (1)
src/mcp/tools/simulator/launch_app_sim.ts (2)
src/utils/schema-helpers.ts (1)
nullifyEmptyStrings(14-24)src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: code-review
🔇 Additional comments (2)
src/mcp/tools/simulator/launch_app_sim.ts (2)
9-38: LGTM! Schema migration to simulatorId is well-structured.The schema properly enforces mutual exclusivity between
simulatorIdandsimulatorNamethrough both descriptions and runtime refinements.
199-202: Public schema now requires session defaults for simulator identification.The public schema omits both
simulatorIdandsimulatorName, meaning callers cannot provide these parameters directly. They must be set via session defaults first. This aligns with the PR's session-aware architecture, but ensure this design is intentional and documented.
| let capturedParams: any = null; | ||
| const logCaptureStub: LogCaptureFunction = async (params: any, executor: any) => { | ||
| it('should ignore args for log capture setup', async () => { | ||
| let capturedParams: unknown = null; |
There was a problem hiding this comment.
This test now asserts that we ignore the args parameter, but launch_app_logs_sim still exposes args and callers rely on it to pass flags to their app. Because the new logic only calls startLogCapture and never forwards params.args, user-specified arguments are dropped and the app always launches without them. That regresses the previous behavior where we appended the args to the simctl launch command. Please keep propagating args (for example by extending startLogCapture to accept an args array and include it in the launch invocation) instead of discarding them.
Allow launch_app_logs_sim to pass args through startLogCapture and ensure the log capture helper includes them in the underlying simctl launch.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/utils/log_capture.ts (3)
55-74: Avoid shell and sanitize user args to prevent injection and quoting bugs.You’re appending user-supplied args then invoking the executor with useShell=true. That exposes shell‑injection/quoting pitfalls. Prefer spawning without a shell for argv arrays.
Apply:
- const stdoutLogResult = await executor( - launchCommand, - 'Console Log Capture', - true, // useShell + const stdoutLogResult = await executor( + launchCommand, + 'Console Log Capture', + false, // useShell (avoid shell; safer argv handling) undefined, // env true, // detached - don't wait for this streaming process to complete );If you must keep a shell, validate/escape args rigorously (reject control chars, newlines, and NUL; enforce expected flag patterns).
76-84: Resource leak on console capture failure.On failure you return without closing logStream (and without removing the empty file). Close the stream (and optionally unlink) before returning.
if (!stdoutLogResult.success) { + try { logStream.end(); } catch {} + try { await fs.promises.unlink(logFilePath); } catch {} return { sessionId: '', logFilePath: '', processes: [], error: stdoutLogResult.error ?? 'Failed to start console log capture', }; }
108-115: Cleanup running processes and stream on OS log failure.If OS log spawn fails after console capture started, you return leaving a detached process and open stream.
if (!osLogResult.success) { + for (const p of processes) { + try { + if (!p.killed && p.exitCode === null) p.kill('SIGTERM'); + } catch {} + } + try { logStream.end(); } catch {} return { sessionId: '', logFilePath: '', processes: [], error: osLogResult.error ?? 'Failed to start OS log capture', }; }
♻️ Duplicate comments (1)
src/mcp/tools/simulator/launch_app_logs_sim.ts (1)
19-25: Reject empty strings for simulatorId and bundleId.z.string() accepts ''. An empty value from session defaults will pass and break the simctl call. Enforce non-empty.
-const launchAppLogsSimSchemaObject = z.object({ - simulatorId: z.string().describe('UUID of the simulator to target'), - bundleId: z - .string() - .describe("Bundle identifier of the app to launch (e.g., 'com.example.MyApp')"), +const launchAppLogsSimSchemaObject = z.object({ + simulatorId: z.string().min(1, 'Required').describe('UUID of the simulator to target'), + bundleId: z + .string() + .min(1, 'Required') + .describe("Bundle identifier of the app to launch (e.g., 'com.example.MyApp')"), args: z.array(z.string()).optional().describe('Additional arguments to pass to the app'), });
🧹 Nitpick comments (2)
src/utils/log_capture.ts (1)
6-7: Minor import path nit (consistency).From src/utils/log_capture.ts,
../utils/logger.tsresolves back to the same folder. Prefer./logger.tsfor clarity.-import { log } from '../utils/logger.ts'; +import { log } from './logger.ts';src/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.ts (1)
107-137: Rename test to reflect behavior (args are forwarded, not ignored).The title says “ignore args” but the expectation verifies args are propagated.
-it('should ignore args for log capture setup', async () => { +it('should forward args to log capture setup', async () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.ts(6 hunks)src/mcp/tools/simulator/launch_app_logs_sim.ts(3 hunks)src/utils/log_capture.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: Use .ts extensions for all internal relative imports (e.g., import { tool } from './tool.ts')
Use .ts extensions in re-exports for internal files (e.g., export { default } from '../shared/tool.ts')
Use .js extension only for external package entry points (e.g., import ... from '@scope/pkg/file.js')
Never import internal project files using .js extensions
Files:
src/mcp/tools/simulator/launch_app_logs_sim.tssrc/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.tssrc/utils/log_capture.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.ts: Do not use Vitest mocking APIs in tests (no vi.mock, vi.fn, vi.spyOn, etc.)
In tests, import the core ...Logic function from the tool and use createMockExecutor/createMockFileSystemExecutor to simulate outcomes
Files:
src/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.ts
🧠 Learnings (1)
📚 Learning: 2025-09-21T19:27:22.795Z
Learnt from: CR
PR: cameroncooke/XcodeBuildMCP#0
File: AGENTS.md:0-0
Timestamp: 2025-09-21T19:27:22.795Z
Learning: Applies to **/*.{test,spec}.ts : In tests, import the core ...Logic function from the tool and use createMockExecutor/createMockFileSystemExecutor to simulate outcomes
Applied to files:
src/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.ts
🧬 Code graph analysis (3)
src/mcp/tools/simulator/launch_app_logs_sim.ts (2)
src/utils/log_capture.ts (1)
startLogCapture(30-141)src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.ts (3)
src/utils/session-store.ts (1)
sessionStore(47-47)src/mcp/tools/simulator/launch_app_logs_sim.ts (2)
LogCaptureFunction(9-17)launch_app_logs_simLogic(33-63)src/test-utils/mock-executors.ts (1)
createMockExecutor(27-75)
src/utils/log_capture.ts (1)
src/utils/command.ts (2)
CommandExecutor(20-20)getDefaultCommandExecutor(199-210)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: code-review
🔇 Additional comments (8)
src/utils/log_capture.ts (1)
42-45: LGTM: args plumbed with safe default.Adding args?: string[] with default [] is sound and non-breaking.
src/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.ts (5)
16-19: LGTM: test isolation via sessionStore.clear().Good hygiene to reset session defaults before each test.
28-39: LGTM: public schema exposes only non‑session fields.Assertion on keys ['args','bundleId'] matches the session‑aware design.
42-63: LGTM: handler requirement messages validated.Nice coverage for missing simulatorId defaults and bundleId validation path.
66-105: LGTM: success path asserts exact literal response and captured params.The DI pattern with a stubbed LogCaptureFunction adheres to testing guidelines.
139-167: LGTM: failure surfaced with actionable message.Clear error propagation when log capture fails.
src/mcp/tools/simulator/launch_app_logs_sim.ts (2)
40-45: LGTM: args propagated to log capture.Forwarding args into captureParams fixes the earlier regression.
65-74: Verification complete: all concerns confirmed.
- ✓
startLogCapturesignature includesargs?: string[](src/utils/log_capture.ts:35)- ✓
launch_app_logs_sim.tsimports from correct path:../../../utils/log-capture/index.ts(hyphen index)- ✓ No accidental stale underscore imports in tool files
- ✓ Args properly threaded through
LogCaptureFunctiontype and logic- ✓ Backwards compatible—existing callers omit optional
argsparameter safely
| const userParamName = params.simulatorUuid ? 'simulatorUuid' : 'simulatorName'; | ||
| const userParamValue = params.simulatorUuid ?? params.simulatorName; | ||
| const userParamName = params.simulatorName ? 'simulatorName' : 'simulatorUuid'; | ||
| const userParamValue = params.simulatorName ?? simulatorId; |
There was a problem hiding this comment.
launch_app_sim now expects simulatorId/simulatorName, but this fallback still emits 'simulatorUuid'. As a result the success message tells clients to call launch_app_sim({ simulatorUuid: ... }), which the new session-aware handler rejects, leading to "Missing required session defaults" errors. Please update the fallback to simulatorId so the next-steps snippet stays valid.
| const userParamValue = params.simulatorName ?? simulatorId; | |
| const userParamName = params.simulatorName ? 'simulatorName' : 'simulatorId'; |
1 participant
Summary
Testing
Note
Migrates simulator runtime tools to session-aware handlers using simulatorId, trims public schemas, updates validations, tests, docs, and extends log capture to pass launch args.
boot_sim,install_app_sim,launch_app_sim,launch_app_logs_sim,stop_app_sim,record_sim_videotocreateSessionAwareToolwith session defaults.simulatorUuid→simulatorIdacross logic, messages, and next-step hints; enforce requirements and mutual exclusivity (simulatorIdvssimulatorName).simulatorId; mark simulator runtime items complete in migration TODO.startLogCaptureacceptsargsand forwards them tosimctl launchwith--console-pty.Written by Cursor Bugbot for commit 1c71e45. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Changes