feat: migrate macOS tools to session-aware defaults #132
Conversation
WalkthroughThe PR migrates macOS tools (build_macos, build_run_macos, get_mac_app_path, test_macos) from typed-tool to session-aware architecture. Changes include replacing createTypedTool with createSessionAwareTool, introducing publicSchemaObjects that omit session-managed fields, adding runtime validation requirements (scheme required, projectPath/workspacePath mutual exclusion), restructuring test validation, and marking four macOS workflows as completed in documentation. Changes
Sequence DiagramsequenceDiagram
participant Client
participant SessionAwareTool
participant Requirements
participant PublicSchema
participant internalSchema
participant LogicFunction
participant Executor
Client->>SessionAwareTool: invoke with params
SessionAwareTool->>PublicSchema: validate against public schema
PublicSchema-->>SessionAwareTool: ✓ valid params
SessionAwareTool->>Requirements: check scheme required
Requirements-->>SessionAwareTool: ✓ scheme present
SessionAwareTool->>Requirements: check XOR(projectPath, workspacePath)
Requirements-->>SessionAwareTool: ✓ exactly one provided
SessionAwareTool->>internalSchema: merge session defaults + validate
internalSchema-->>SessionAwareTool: ✓ complete params
SessionAwareTool->>LogicFunction: execute with executor
LogicFunction->>Executor: run command
Executor-->>LogicFunction: result
LogicFunction-->>SessionAwareTool: response
SessionAwareTool-->>Client: return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes follow a consistent migration pattern across 8 files (4 implementation + 4 test), reducing individual file complexity review burden. However, validation requires verification of: (1) correct schema field omissions across all four tools, (2) proper requirements/exclusivePairs configuration, (3) test setup correctly isolates sessionStore state, and (4) error messages align with new validation layer semantics. 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 |
|
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: 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/macos/__tests__/build_run_macos.test.ts (1)
440-498: Test name vs. setup mismatch.Test is titled “should use default configuration when not provided” but passes configuration: 'Debug' in args. Rename the test or drop the configuration field to exercise the default.
Apply one of these diffs:
Option A (rename):
- it('should use default configuration when not provided', async () => { + it('should pass through provided configuration', async () => {Option B (exercise default):
- const args = { - projectPath: '/path/to/project.xcodeproj', - scheme: 'MyApp', - configuration: 'Debug', - preferXcodebuild: false, - }; + const args = { + projectPath: '/path/to/project.xcodeproj', + scheme: 'MyApp', + preferXcodebuild: false, + };
🧹 Nitpick comments (7)
src/mcp/tools/macos/get_mac_app_path.ts (3)
55-65: Avoid duplicating XcodePlatform; import the shared enum instead.Define the platform once to prevent drift across files.
Apply this diff:
-import { ToolResponse } from '../../../types/common.ts'; +import { ToolResponse, XcodePlatform } from '../../../types/common.ts'; @@ -const XcodePlatform = { - iOS: 'iOS', - watchOS: 'watchOS', - tvOS: 'tvOS', - visionOS: 'visionOS', - iOSSimulator: 'iOS Simulator', - watchOSSimulator: 'watchOS Simulator', - tvOSSimulator: 'tvOS Simulator', - visionOSSimulator: 'visionOS Simulator', - macOS: 'macOS', -};
98-106: Optional: include '-destination platform=macOS' even when arch is not provided for parity.Not required for -showBuildSettings, but adding a consistent destination may help future-proof command shaping across tools.
136-155: Be robust to multiple settings lines or trailing whitespace.Your regex + trim is fine; consider normalizing via a small helper (getBuildSetting('FULL_PRODUCT_NAME')) reused across tools to avoid regex duplication.
src/mcp/tools/macos/__tests__/get_mac_app_path.test.ts (1)
355-363: Wording nit: not a Zod error.This path fails at the factory “requirements” layer, not Zod. Consider renaming the test to “should report requirement error for missing scheme” to avoid confusion.
src/mcp/tools/macos/test_macos.ts (2)
126-133: Type hygiene: declare lines as string[].Avoid implicit any array.
Apply this diff:
-function formatTestSummary(summary: Record<string, unknown>): string { - const lines = []; +function formatTestSummary(summary: Record<string, unknown>): string { + const lines: string[] = [];
329-340: Handler closure uses real FS executor; safe due to early requirements, but document this.It’s fine operationally (tests call logic directly), but a short comment noting “factory early-returns prevent getDefaultFileSystemExecutor() in tests” would help future readers.
src/mcp/tools/macos/build_run_macos.ts (1)
41-47: Public schema trimming: verify consumer behavior and consider UX hintOmitting projectPath/workspacePath/scheme/configuration/arch from the exported schema aligns with “session-managed-only” fields. Two asks:
- Verify your tool registry/UI doesn’t drop unknown args strictly based on this public schema; otherwise users can’t override via direct args even if the handler accepts them.
- Optional: add a short note in the tool description (or docs) that these fields are session-managed and set via session-set-defaults to reduce confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/session-aware-migration-todo.md(1 hunks)src/mcp/tools/macos/__tests__/build_macos.test.ts(3 hunks)src/mcp/tools/macos/__tests__/build_run_macos.test.ts(1 hunks)src/mcp/tools/macos/__tests__/get_mac_app_path.test.ts(4 hunks)src/mcp/tools/macos/__tests__/test_macos.test.ts(2 hunks)src/mcp/tools/macos/build_macos.ts(3 hunks)src/mcp/tools/macos/build_run_macos.ts(3 hunks)src/mcp/tools/macos/get_mac_app_path.ts(3 hunks)src/mcp/tools/macos/test_macos.ts(3 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/macos/test_macos.tssrc/mcp/tools/macos/__tests__/build_run_macos.test.tssrc/mcp/tools/macos/__tests__/get_mac_app_path.test.tssrc/mcp/tools/macos/build_run_macos.tssrc/mcp/tools/macos/__tests__/test_macos.test.tssrc/mcp/tools/macos/build_macos.tssrc/mcp/tools/macos/__tests__/build_macos.test.tssrc/mcp/tools/macos/get_mac_app_path.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/macos/__tests__/build_run_macos.test.tssrc/mcp/tools/macos/__tests__/get_mac_app_path.test.tssrc/mcp/tools/macos/__tests__/test_macos.test.tssrc/mcp/tools/macos/__tests__/build_macos.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/macos/__tests__/build_macos.test.ts
🧬 Code graph analysis (8)
src/mcp/tools/macos/test_macos.ts (2)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)src/utils/command.ts (2)
getDefaultFileSystemExecutor(216-227)getDefaultCommandExecutor(199-210)
src/mcp/tools/macos/__tests__/build_run_macos.test.ts (1)
src/utils/session-store.ts (1)
sessionStore(47-47)
src/mcp/tools/macos/__tests__/get_mac_app_path.test.ts (1)
src/utils/session-store.ts (1)
sessionStore(47-47)
src/mcp/tools/macos/build_run_macos.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/macos/__tests__/test_macos.test.ts (1)
src/utils/session-store.ts (1)
sessionStore(47-47)
src/mcp/tools/macos/build_macos.ts (2)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)src/utils/execution/index.ts (1)
getDefaultCommandExecutor(5-5)
src/mcp/tools/macos/__tests__/build_macos.test.ts (1)
src/utils/session-store.ts (1)
sessionStore(47-47)
src/mcp/tools/macos/get_mac_app_path.ts (2)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)src/utils/execution/index.ts (1)
getDefaultCommandExecutor(5-5)
⏰ 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 (13)
docs/session-aware-migration-todo.md (1)
26-29: macOS session-aware entries marked complete look consistent with the implementation.The checklist matches the migrated tools in this PR. No action needed.
src/mcp/tools/macos/get_mac_app_path.ts (1)
36-42: Public schema omits session-managed fields correctly.Omitting projectPath/workspacePath/scheme/configuration/arch aligns with the session-aware surface. Nice.
src/mcp/tools/macos/__tests__/build_run_macos.test.ts (1)
63-73: Good guard for mutually exclusive params via handler.Covers both explicit double-provided inputs and aligns with factory behavior.
src/mcp/tools/macos/__tests__/build_macos.test.ts (2)
32-50: Schema surface checks are aligned with session-aware design.Validating only derivedDataPath/extraArgs/preferXcodebuild keeps tests resilient to internal schema changes.
71-83: Mutual exclusivity assertion includes field names — good.Helps UX by echoing conflicting params in the message.
src/mcp/tools/macos/__tests__/get_mac_app_path.test.ts (1)
30-46: Public schema validation is minimal and correct.Only non-session fields are exposed; checks read well.
src/mcp/tools/macos/build_macos.ts (2)
50-57: Public schema omits session-managed fields — consistent.Matches the migration pattern used across macOS tools.
98-111: Session-aware handler wiring looks correct.Requirements and exclusivePairs mirror the internal XOR refine; early-return avoids touching real executors in tests. Good separation.
If you want, I can scan for any remaining createTypedTool usages under macOS to ensure full migration.
src/mcp/tools/macos/__tests__/test_macos.test.ts (2)
30-52: Public schema verification is comprehensive.Including testRunnerEnv typing strengthens the contract for env passing.
55-83: Nice: requirement and exclusivity checks via handler.This ensures tests won’t accidentally hit real executors due to factory early-returns.
src/mcp/tools/macos/test_macos.ts (1)
250-273: Good: testRunnerEnv normalization plumbed via exec opts.This keeps env handling explicit and decoupled.
src/mcp/tools/macos/build_run_macos.ts (2)
15-15: Import migration looks goodSwapping to createSessionAwareTool with a proper .ts path matches the session defaults plan and our import rules.
218-229: Rewritten review comment: Disregard original suggestions—current implementation is correct.The verification reveals the review comment contains incorrect suggestions:
sessionKeys parameter is unimplemented: Although declared in the
createSessionAwareToolsignature, it is never extracted or used by the factory. Adding it to the handler config would have no effect.z.infer is the established pattern: The entire codebase consistently uses
z.infer<typeof schema>(100+ instances). The suggestion to usez.output<typeof buildRunMacOSSchema>contradicts this pattern and is not used anywhere in the project.Current code is correct: The handler already properly uses
requirementsandexclusivePairs. The double castas unknown as z.ZodType<BuildRunMacOSParams>follows the same pattern used in other session-aware tools (e.g.,build_run_sim.ts).Registry expectations are satisfied: all tools export
.shapefor the schema property.Likely an incorrect or invalid review comment.
1 participant
Summary
Background/Details
Solution
Testing
Notes
Note
Migrates macOS build/test/path tools to session-aware handlers with session-backed defaults, exposes only non-session schema fields, updates tests and docs checklist.
createSessionAwareToolinbuild_macos.ts,build_run_macos.ts,test_macos.ts,get_mac_app_path.tswithrequirementsandexclusivePairsguards.schemato public (non-session) fields viapublicSchemaObject; simplifydescriptions.src/mcp/tools/macos/__tests__/*to validate public schemas, session requirements, mutual exclusivity, and new error messages; addsessionStore.clear()setup.docs/session-aware-migration-todo.md.Written by Cursor Bugbot for commit c6eba46. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Chores