Migrate simulator test tools to session defaults#122
Conversation
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
commit: |
WalkthroughSession-aware migration applied to simulator tools. get_sim_app_path.ts and test_sim.ts now use createSessionAwareTool with publicSchema omissions, explicit requirements, and exclusivePairs. New unit tests validate session/default-driven validation and executor wiring. A migration TODO document was added outlining session-default propagation across tools. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Tool as Tool (get_sim_app_path / test_sim)
participant Session as SessionStore
participant Validator as Session-Aware Wrapper
participant Logic as Logic Function
participant Exec as Command Executor
User->>Tool: Invoke with public params
Tool->>Validator: Merge public params + Session defaults
Validator->>Session: Read defaults (scheme, project/workspace, simulator...)
Session-->>Validator: Defaults
Validator->>Validator: Check requirements & exclusivePairs
alt Validation fails
Validator-->>User: Error (missing/mutually exclusive)
else Validation passes
Validator->>Logic: Call with internal params
Logic->>Exec: Run xcodebuild / test commands
Exec-->>Logic: Output / error
Logic-->>Tool: Result
Tool-->>User: Result (path / test output)
end
note over Validator,Logic: Public schema omits session-managed fields
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
f5da07f to
89cd11c
Compare
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/session-aware-migration-todo.md (1)
3-3: Fix markdown lint violation on audit linemarkdownlint (MD036) flags the italicized audit date as an improper heading. Convert it into a real heading so the doc passes lint checks.
Apply this diff:
-_Audit date: October 6, 2025_ +### Audit date: October 6, 2025Based on static analysis
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/session-aware-migration-todo.md(1 hunks)src/mcp/tools/simulator/__tests__/get_sim_app_path.test.ts(1 hunks)src/mcp/tools/simulator/__tests__/test_sim.test.ts(1 hunks)src/mcp/tools/simulator/get_sim_app_path.ts(2 hunks)src/mcp/tools/simulator/test_sim.ts(3 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__/get_sim_app_path.test.tssrc/mcp/tools/simulator/__tests__/test_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__/get_sim_app_path.test.tssrc/mcp/tools/simulator/get_sim_app_path.tssrc/mcp/tools/simulator/__tests__/test_sim.test.tssrc/mcp/tools/simulator/test_sim.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__/get_sim_app_path.test.ts
🧬 Code graph analysis (4)
src/mcp/tools/simulator/__tests__/get_sim_app_path.test.ts (3)
src/utils/session-store.ts (1)
sessionStore(47-47)src/mcp/tools/simulator/get_sim_app_path.ts (1)
get_sim_app_pathLogic(139-290)src/test-utils/mock-executors.ts (1)
createMockExecutor(27-75)
src/mcp/tools/simulator/get_sim_app_path.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-174)
src/mcp/tools/simulator/__tests__/test_sim.test.ts (1)
src/utils/session-store.ts (1)
sessionStore(47-47)
src/mcp/tools/simulator/test_sim.ts (2)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-174)src/utils/command.ts (1)
getDefaultCommandExecutor(199-210)
🪛 markdownlint-cli2 (0.18.1)
docs/session-aware-migration-todo.md
3-3: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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: code-review
- GitHub Check: Cursor Bugbot
|
@cursor review |
Merge upstream commits bringing session-aware pattern to additional tools: - getsentry#125: Migrate clean/list_schemes/show_build_settings to session defaults - getsentry#122: Migrate test_sim and get_sim_app_path to session defaults ## Conflict Resolution Resolved conflicts in 3 files by keeping our improved implementation: ### 1. src/utils/typed-tool-factory.ts - KEPT: Our simplified factory (48 lines vs upstream's 100+ lines) - KEPT: Removed requirements DSL (Zod-first validation) - KEPT: Simplified API (positional params vs config object) - KEPT: exclusivePairs as 4th parameter (critical for XOR field pruning) ### 2. src/mcp/tools/simulator/test_sim.ts - KEPT: Our comprehensive implementation with utilities - KEPT: mapPlatformStringToEnum for type safety - KEPT: logUseLatestOSWarning utility - KEPT: Simplified factory API - KEPT: macOS platform rejection validation ### 3. src/mcp/tools/simulator/__tests__/test_sim.test.ts - KEPT: Our comprehensive 500+ line test suite (23 session tests + empty string tests) - UPSTREAM HAD: Basic session tests only ## Upstream Tool Migration Updated 4 upstream tools to use our simplified factory API: - list_schemes.ts - Updated to positional params, removed .omit() bug - show_build_settings.ts - Updated to positional params, removed .omit() bug - get_sim_app_path.ts - Updated to positional params, removed .omit() bug - clean.ts - Updated to positional params, removed .omit() bug ## Test Fixes Fixed test expectations in 4 upstream test files: - list_schemes.test.ts - Updated for correct public schema (no .omit()) - show_build_settings.test.ts - Updated for correct public schema - clean.test.ts - Updated for correct public schema - get_sim_app_path.test.ts - Fixed fake path issues ## Upstream Benefits from Our Improvements Upstream tools now benefit from: - ✅ Simplified factory API (cleaner code) - ✅ Fixed .omit() bug (explicit params work correctly) - ✅ XOR field pruning (explicit overrides session for XOR fields) - ✅ Better error handling (no server crashes) - ✅ Shared utilities (platform-utils, simulator-validation, shared-schemas) ## Quality Validation ✅ TypeCheck: Zero errors ✅ Lint: Zero errors ✅ Build: Successful (91 test files) ✅ Tests: 1,233/1,233 passing (100% pass rate) ## Commits Merged from Upstream - 4bbfe5d: feat(project-discovery): migrate clean/list/show to session defaults - 79736a3: feat(simulator): migrate test_sim and get_sim_app_path to session defaults Total: 2 upstream commits + our local improvements merged successfully. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary
Testing
Note
Migrates
test_simandget_sim_app_pathto session-aware handlers with simplified public schemas and validations, adds comprehensive unit tests, and introduces a migration TODO doc.src/mcp/tools/simulator/test_sim.tsandsrc/mcp/tools/simulator/get_sim_app_path.tsto usecreateSessionAwareToolwith concise public schemas (hide session fields), requirement checks, and mutually exclusive param validation.src/mcp/tools/simulator/__tests__/test_sim.test.tsandsrc/mcp/tools/simulator/__tests__/get_sim_app_path.test.tscovering export literals, public schema exposure, session-requirement messaging, mutually exclusive params, success path, and executor failure handling.docs/session-aware-migration-todo.mdchecklist for session-aware migration progress across tools.Written by Cursor Bugbot for commit 89cd11c. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Documentation
Refactor
Tests