feat(project-discovery): migrate clean/list/show to session defaults #125
Conversation
WalkthroughMigrates three tools (clean, list_schemes, show_build_settings) from createTypedTool to createSessionAwareTool with public schema reductions, requirements, and mutual exclusivity. Updates tests to clear sessionStore, expect new messages, and validate empty public schema. Adjusts argument sanitization to ignore empty strings. Marks corresponding docs checklist items as done. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Tool as Tool (session-aware)
participant Wrapper as SessionAwareWrapper
participant Store as SessionStore
participant Logic as ToolLogic
participant Exec as CommandExecutor
Caller->>Tool: invoke(name, args)
Tool->>Wrapper: validate public schema (empty surface)
Wrapper->>Wrapper: sanitize args (drop null/undefined/empty strings)
Wrapper->>Store: read session defaults
Wrapper->>Wrapper: merge defaults + sanitized args
Wrapper->>Wrapper: check requirements (scheme, project/workspace)
Wrapper->>Wrapper: check exclusivity (projectPath ⟂ workspacePath)
alt validation fails
Wrapper-->>Caller: error ("Missing required session defaults" / "Mutually exclusive parameters provided")
else validation ok
Wrapper->>Logic: call with resolved params
Logic->>Exec: build and run xcodebuild command
Exec-->>Logic: result
Logic-->>Caller: result
end
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 |
commit: |
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?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 (2)
src/utils/typed-tool-factory.ts (1)
114-131: Exclusive-pairs: handle conflicting session defaults (no user input)Currently, conflicts are only pruned/validated when the user provides a concrete value. If session defaults contain multiple members of an exclusive pair, they slip through silently.
Consider rejecting conflicting session-defaults when no explicit arg is provided:
// Apply exclusive pair pruning... for (const pair of exclusivePairs) { const userProvidedConcrete = pair.some((k) => Object.prototype.hasOwnProperty.call(sanitizedArgs, k), ); if (!userProvidedConcrete) continue; for (const k of pair) { if (!Object.prototype.hasOwnProperty.call(sanitizedArgs, k) && k in merged) { delete merged[k]; } } } + + // Optional: detect conflicting session defaults when user didn't provide any side + for (const pair of exclusivePairs) { + const providedFromSession = pair.filter((k) => Object.prototype.hasOwnProperty.call(merged, k)); + const providedFromUser = pair.filter((k) => Object.prototype.hasOwnProperty.call(sanitizedArgs, k)); + if (providedFromUser.length === 0 && providedFromSession.length >= 2) { + return createErrorResponse( + 'Parameter validation failed', + `Conflicting session defaults detected: ${providedFromSession.join(', ')}. ` + + `Clear one via: ${providedFromSession + .map((k) => `session-set-defaults { "${k}": null }`) + .join(' OR ')}`, + ); + } + }src/mcp/tools/project-discovery/__tests__/list_schemes.test.ts (1)
228-231: Update outdated comment (createTypedTool → createSessionAwareTool)The wrapper here is now session‑aware. Please update the comment for accuracy.
- // Note: Direct logic function calls bypass Zod validation, so we test the actual plugin handler - // to verify Zod validation works properly. The createTypedTool wrapper handles validation. + // Note: Direct logic function calls bypass Zod validation, so we test the actual plugin handler + // to verify Zod validation works properly. The createSessionAwareTool wrapper handles validation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/session-aware-migration-todo.md(1 hunks)src/mcp/tools/project-discovery/__tests__/list_schemes.test.ts(4 hunks)src/mcp/tools/project-discovery/__tests__/show_build_settings.test.ts(5 hunks)src/mcp/tools/project-discovery/list_schemes.ts(2 hunks)src/mcp/tools/project-discovery/show_build_settings.ts(2 hunks)src/mcp/tools/utilities/__tests__/clean.test.ts(4 hunks)src/mcp/tools/utilities/clean.ts(2 hunks)src/utils/typed-tool-factory.ts(1 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/project-discovery/__tests__/list_schemes.test.tssrc/mcp/tools/utilities/__tests__/clean.test.tssrc/mcp/tools/project-discovery/__tests__/show_build_settings.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/project-discovery/__tests__/list_schemes.test.tssrc/mcp/tools/project-discovery/show_build_settings.tssrc/mcp/tools/utilities/__tests__/clean.test.tssrc/mcp/tools/project-discovery/__tests__/show_build_settings.test.tssrc/utils/typed-tool-factory.tssrc/mcp/tools/project-discovery/list_schemes.tssrc/mcp/tools/utilities/clean.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/utilities/__tests__/clean.test.ts
🧬 Code graph analysis (6)
src/mcp/tools/project-discovery/__tests__/list_schemes.test.ts (1)
src/utils/session-store.ts (1)
sessionStore(47-47)
src/mcp/tools/project-discovery/show_build_settings.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/utilities/__tests__/clean.test.ts (1)
src/utils/session-store.ts (1)
sessionStore(47-47)
src/mcp/tools/project-discovery/__tests__/show_build_settings.test.ts (1)
src/utils/session-store.ts (1)
sessionStore(47-47)
src/mcp/tools/project-discovery/list_schemes.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/utilities/clean.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
🪛 LanguageTool
docs/session-aware-migration-todo.md
[grammar] ~10-~10: There might be a mistake here.
Context: ..., configuration. ## Project Discovery - [x] `src/mcp/tools/project-discovery/lis...
(QB_NEW_EN)
[grammar] ~11-~11: There might be a mistake here.
Context: ...efaults: projectPath, workspacePath. - [x] `src/mcp/tools/project-discovery/sho...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...: projectPath, workspacePath. - [x] src/mcp/tools/project-discovery/show_build_settings.ts — session defaults: projectPath, `workspacePath...
(QB_NEW_EN)
⏰ 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 (12)
docs/session-aware-migration-todo.md (1)
8-12: Checklist updates align with the migrationThe Utilities and Project Discovery items now correctly reflect the session‑aware migration. LGTM.
src/mcp/tools/project-discovery/__tests__/list_schemes.test.ts (2)
14-17: Good test hygieneClearing sessionStore before each test avoids cross‑test leakage. LGTM.
31-36: Public schema assertion is correctStrict empty public schema check matches the new session‑aware surface. LGTM.
src/mcp/tools/utilities/__tests__/clean.test.ts (2)
8-33: Session reset + public schema checks look solidPer‑test session reset and the strict public schema assertions are correct and consistent with the new wrapper. LGTM.
68-74: Nice validation coverageThe specific message for missing scheme with workspace is asserted. Good alignment with session‑aware validation.
src/mcp/tools/project-discovery/__tests__/show_build_settings.test.ts (3)
8-10: Good: session isolationClearing sessionStore per test avoids flakiness. LGTM.
24-30: Public schema surface correctly emptyStrict empty public schema test reflects the reduced public API. LGTM.
223-243: Session requirement tests are appropriateAsserting missing scheme and project/workspace requirements matches the factory’s behavior. LGTM.
src/utils/typed-tool-factory.ts (1)
95-97: Trim whitespace and skip empty strings in sanitizedArgs
- Replace at lines 95–97:
- if (v === null || v === undefined) continue; - if (typeof v === 'string' && v.trim() === '') continue; - sanitizedArgs[k] = v; + if (v == null) continue; + const val = typeof v === 'string' ? v.trim() : v; + if (val === '') continue; + sanitizedArgs[k] = val;
- Verify no code relies on
''to clear defaults (e.g., runor review manually).rg -n --fixed-strings ": ''" rg -n --fixed-strings ": \"\""src/mcp/tools/utilities/clean.ts (1)
9-9: LGTM! Clean migration to session-aware tool.The migration correctly:
- Reduces the public schema surface by omitting
projectPath,workspacePath,scheme, andconfiguration- Configures requirements to enforce oneOf constraint for project/workspace
- Sets up mutual exclusivity between
projectPathandworkspacePath- Uses appropriate defaults for optional fields (
schemedefaults to empty string at line 119,configurationdefaults to 'Debug' at line 120)The type cast
as unknown as z.ZodType<CleanParams>is necessary becausecleanSchemaincludes refinements that prevent direct assignment toZodType.Also applies to: 149-168
src/mcp/tools/project-discovery/list_schemes.ts (1)
14-14: LGTM! Consistent session-aware migration.The migration follows the same pattern as
clean.ts:
- Public schema omits
projectPathandworkspacePath(simpler than clean since no optional fields)- Requirements enforce oneOf constraint matching the schema refinements
- Mutual exclusivity properly configured
- Type cast is necessary for the refined schema
Also applies to: 112-129
src/mcp/tools/project-discovery/show_build_settings.ts (1)
14-14: LGTM! Proper handling of required scheme parameter.The migration correctly handles the required
schemefield:
- Public schema omits
projectPath,workspacePath, andscheme- Requirements use
allOf: ['scheme']to enforce that scheme must be provided (line 120), matching the required constraint inbaseSchemaObject(line 21)- OneOf requirement for project/workspace is consistent with other tools
- Mutual exclusivity properly configured
This is the most complex of the three migrations due to the required scheme parameter, and it's handled correctly.
Also applies to: 105-124
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>
1 participant
Note
Migrates
clean,list_schemes, andshow_build_settingsto session-aware handlers, hiding path/scheme params from public schemas and tightening validation/messages; updates tests and docs accordingly.src/mcp/tools/utilities/clean.ts: switch tocreateSessionAwareTool, hideprojectPath/workspacePath/scheme/configurationfrom public schema, shorten description, enforce oneOf project/workspace and mutual exclusivity.src/mcp/tools/project-discovery/list_schemes.ts: usecreateSessionAwareTool, hide path params from public schema, shorten description, add session requirements (project/workspace) and exclusivity.src/mcp/tools/project-discovery/show_build_settings.ts: usecreateSessionAwareTool, hide path/scheme from public schema, shorten description, requireschemeand project/workspace via session, enforce exclusivity.src/utils/typed-tool-factory.ts: enhance arg sanitization (skip empty strings), add requirement checks and exclusivity handling for session-aware tools; include user-friendly hints in errors.sessionStoreinbeforeEach.docs/session-aware-migration-todo.md: check offclean,list_schemes,show_build_settings.Written by Cursor Bugbot for commit 48b32d9. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Behavior Changes
Documentation