feat(simulator): migrate build_run_sim to session defaults #121
Conversation
WalkthroughThis update refactors the simulator build-and-run logic and its test suite to adopt a session-aware tool wrapper, streamlining parameter validation, input schema exposure, and error handling. The command logic now leverages a public schema and enforces stricter runtime validation, with tests updated for the refined validation flow and error messaging. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Tool (build_run_sim)
participant SessionStore
participant Executor
Note over User, Tool: Request to build and run app on simulator
User->>Tool: invoke handler(params)
Tool->>SessionStore: load session defaults (if needed)
Tool->>Tool: validate input params (require scheme, XOR project/workspace, XOR id/name)
alt validation error
Tool-->>User: error (e.g., "Missing required session defaults", "Mutually exclusive parameters provided")
else valid input
Tool->>Executor: invoke build/run command
Executor-->>Tool: result
Tool-->>User: success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
commit: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/mcp/tools/simulator/build_run_sim.ts (1)
507-520: Session-aware configuration looks correct.The requirements and exclusive pairs are properly configured to enforce the tool's constraints:
schemeis required via session defaults- One of
projectPathorworkspacePathis required (XOR constraint)- One of
simulatorIdorsimulatorNameis required (XOR constraint)- Mutual exclusivity is enforced for both pairs
The type cast at line 508 is necessary because Zod's
.refine()returns aZodEffectstype rather thanZodType, but the factory's.parse()method will work correctly with refined schemas. Consider adding a comment explaining why the cast is needed to help future maintainers.Consider adding a clarifying comment:
handler: createSessionAwareTool<BuildRunSimulatorParams>({ + // Type cast needed: .refine() returns ZodEffects, but factory accepts ZodType internalSchema: buildRunSimulatorSchema as unknown as z.ZodType<BuildRunSimulatorParams>,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/mcp/tools/simulator/__tests__/build_run_sim.test.ts(3 hunks)src/mcp/tools/simulator/build_run_sim.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/build_run_sim.tssrc/mcp/tools/simulator/__tests__/build_run_sim.test.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__/build_run_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__/build_run_sim.test.ts
🧬 Code graph analysis (2)
src/mcp/tools/simulator/build_run_sim.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-174)
src/mcp/tools/simulator/__tests__/build_run_sim.test.ts (1)
src/utils/session-store.ts (1)
sessionStore(47-47)
⏰ 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). (1)
- GitHub Check: code-review
🔇 Additional comments (7)
src/mcp/tools/simulator/build_run_sim.ts (2)
13-13: LGTM!The import follows the coding guidelines by using the
.tsextension for internal module imports.
493-501: LGTM!The public schema correctly exposes only the non-session fields (
derivedDataPath,extraArgs,preferXcodebuild) by omitting all session-managed parameters. This aligns with the session-aware pattern and the test expectations.src/mcp/tools/simulator/__tests__/build_run_sim.test.ts (5)
8-9: LGTM!The import changes correctly follow the coding guidelines by using
.tsextensions. The addition ofsessionStoreis necessary for the session cleanup inbeforeEach, and the removal ofcreateMockFileSystemExecutoraligns with the testing pattern of using onlycreateMockExecutorfor this tool's tests.Based on learnings.
13-15: LGTM!The
beforeEachhook properly clears the session state before each test, preventing test pollution and ensuring each test runs with a clean slate. This is essential for testing session-aware tools.
23-23: LGTM!The test expectation correctly matches the concise description from the tool implementation.
30-52: LGTM!The schema exposure test correctly validates that only non-session fields (
derivedDataPath,extraArgs,preferXcodebuild) are exposed in the public schema, and that session-managed fields (scheme,simulatorName,projectPath, etc.) are not exposed. The test is thorough and covers both valid and invalid inputs.
537-560: LGTM!The XOR validation tests correctly verify the new error message format from the session-aware tool wrapper:
- Missing required session defaults now returns the appropriate "Missing required session defaults" error with the helpful message "Provide a project or workspace"
- Mutually exclusive parameters are properly detected and reported with "Mutually exclusive parameters provided" along with the specific parameter names
These expectations align with the factory implementation in
typed-tool-factory.ts.
1 participant
Summary
Testing
Note
Migrates
build_run_simto a session-aware tool, narrowing its public schema/description and updating tests to clear session state and expect new validation messages.src/mcp/tools/simulator/build_run_sim.ts):createSessionAwareToolwithrequirementsandexclusivePairs, merging session defaults at runtime.omit(hideprojectPath,workspacePath,scheme,configuration,simulatorId,simulatorName,useLatestOS).schemato public-only fields and shortendescription.src/mcp/tools/simulator/__tests__/build_run_sim.test.ts):beforeEachtosessionStore.clear()and update imports.derivedDataPath,extraArgs,preferXcodebuild) and adjust type checks.Written by Cursor Bugbot for commit 74eb7a4. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests