From a72960de91dd6f0d8f5bd4dcc8af5785f5819dea Mon Sep 17 00:00:00 2001 From: Cameron Cooke Date: Sun, 5 Oct 2025 10:06:31 +0100 Subject: [PATCH 01/10] feat(session-management): add session defaults store, tools, and session-aware middleware; add tests; update plan doc - Add in-memory SessionStore (set/get/clear/show) - New workflow: session-management (session-set-defaults, session-clear-defaults, session-show-defaults) - Add createSessionAwareTool with requirements preflight and args-over-defaults merge - Comprehensive tests for tools, store, and factory - Update session_management_plan.md with DI checklist, commit protocol, and pre-commit hook note Existing tools unchanged; no remote push. --- .gitignore | 1 + docs/session_management_plan.md | 413 ++++++++++++++++++ .../__tests__/index.test.ts | 56 +++ .../__tests__/session_clear_defaults.test.ts | 75 ++++ .../__tests__/session_set_defaults.test.ts | 60 +++ .../__tests__/session_show_defaults.test.ts | 45 ++ src/mcp/tools/session-management/index.ts | 8 + .../session_clear_defaults.ts | 37 ++ .../session_set_defaults.ts | 36 ++ .../session_show_defaults.ts | 12 + .../session-aware-tool-factory.test.ts | 112 +++++ src/utils/__tests__/session-store.test.ts | 38 ++ src/utils/session-store.ts | 42 ++ src/utils/typed-tool-factory.ts | 68 +++ 14 files changed, 1003 insertions(+) create mode 100644 docs/session_management_plan.md create mode 100644 src/mcp/tools/session-management/__tests__/index.test.ts create mode 100644 src/mcp/tools/session-management/__tests__/session_clear_defaults.test.ts create mode 100644 src/mcp/tools/session-management/__tests__/session_set_defaults.test.ts create mode 100644 src/mcp/tools/session-management/__tests__/session_show_defaults.test.ts create mode 100644 src/mcp/tools/session-management/index.ts create mode 100644 src/mcp/tools/session-management/session_clear_defaults.ts create mode 100644 src/mcp/tools/session-management/session_set_defaults.ts create mode 100644 src/mcp/tools/session-management/session_show_defaults.ts create mode 100644 src/utils/__tests__/session-aware-tool-factory.test.ts create mode 100644 src/utils/__tests__/session-store.test.ts create mode 100644 src/utils/session-store.ts diff --git a/.gitignore b/.gitignore index ce7a55a3..f1f63ca2 100644 --- a/.gitignore +++ b/.gitignore @@ -108,3 +108,4 @@ bundled/ /.mcpregistry_registry_token /key.pem .mcpli +.factory diff --git a/docs/session_management_plan.md b/docs/session_management_plan.md new file mode 100644 index 00000000..36904443 --- /dev/null +++ b/docs/session_management_plan.md @@ -0,0 +1,413 @@ +# Stateful Session Defaults for MCP Tools — Design, Middleware, and Plan + +Below is a concise architecture and implementation plan to introduce a session-aware defaults layer that removes repeated tool parameters from public schemas, while keeping all tool logic and tests unchanged. + +## Architecture Overview + +- **Core idea**: keep logic functions and tests untouched; move argument consolidation into a session-aware interop layer and expose minimal public schemas. +- **Data flow**: + - Client calls a tool with zero or few args → session middleware merges session defaults → validates with the internal schema → calls the existing logic function. +- **Components**: + - `SessionStore` (singleton, in-memory): set/get/clear/show defaults. + - Session-aware tool factory: merges defaults, performs preflight requirement checks (allOf/oneOf), then validates with the tool's internal zod schema. + - Public vs internal schema: plugins register a minimal "public" input schema; handlers validate with the unchanged "internal" schema. + +## Core Types + +```typescript +// src/utils/session-store.ts +export type SessionDefaults = { + projectPath?: string; + workspacePath?: string; + scheme?: string; + configuration?: string; + simulatorName?: string; + simulatorId?: string; + deviceId?: string; + useLatestOS?: boolean; + arch?: 'arm64' | 'x86_64'; +}; +``` + +## Session Store (singleton) + +```typescript +// src/utils/session-store.ts +import { log } from './logger.ts'; + +class SessionStore { + private defaults: SessionDefaults = {}; + + setDefaults(partial: Partial): void { + this.defaults = { ...this.defaults, ...partial }; + log('info', '[Session] Defaults set', { keys: Object.keys(partial) }); + } + + clear(keys?: (keyof SessionDefaults)[]): void { + if (!keys || keys.length === 0) { + this.defaults = {}; + log('info', '[Session] All defaults cleared'); + return; + } + for (const k of keys) delete this.defaults[k]; + log('info', '[Session] Defaults cleared', { keys }); + } + + get(key: K): SessionDefaults[K] { + return this.defaults[key]; + } + + getAll(): SessionDefaults { + return { ...this.defaults }; + } +} + +export const sessionStore = new SessionStore(); +``` + +## Session-Aware Tool Factory + +```typescript +// src/utils/typed-tool-factory.ts (add new helper, keep createTypedTool as-is) +import { z } from 'zod'; +import { sessionStore, type SessionDefaults } from './session-store.ts'; +import type { CommandExecutor } from './execution/index.ts'; +import { createErrorResponse } from './responses/index.ts'; +import type { ToolResponse } from '../types/common.ts'; + +export type SessionRequirement = + | { allOf: (keyof SessionDefaults)[]; message?: string } + | { oneOf: (keyof SessionDefaults)[]; message?: string }; + +function missingFromArgsAndSession( + keys: (keyof SessionDefaults)[], + args: Record, +): string[] { + return keys.filter((k) => args[k] == null && sessionStore.get(k) == null); +} + +export function createSessionAwareTool(opts: { + internalSchema: z.ZodType; + logicFunction: (params: TParams, executor: CommandExecutor) => Promise; + getExecutor: () => CommandExecutor; + // Optional extras to improve UX and ergonomics + sessionKeys?: (keyof SessionDefaults)[]; + requirements?: SessionRequirement[]; // preflight, friendlier than raw zod errors +}) { + const { internalSchema, logicFunction, getExecutor, sessionKeys = [], requirements = [] } = opts; + + return async (rawArgs: Record): Promise => { + try { + // Merge: explicit args take precedence over session defaults + const merged: Record = { ...sessionStore.getAll(), ...rawArgs }; + + // Preflight requirement checks (clear message how to fix) + for (const req of requirements) { + if ('allOf' in req) { + const missing = missingFromArgsAndSession(req.allOf, rawArgs); + if (missing.length > 0) { + return createErrorResponse( + 'Missing required session defaults', + `${req.message ?? `Required: ${req.allOf.join(', ')}`}\n` + + `Set with: session-set-defaults { ${missing.map((k) => `"${k}": "..."`).join(', ')} }`, + ); + } + } else if ('oneOf' in req) { + const missing = missingFromArgsAndSession(req.oneOf, rawArgs); + // oneOf satisfied if at least one is present in merged + const satisfied = req.oneOf.some((k) => merged[k] != null); + if (!satisfied) { + return createErrorResponse( + 'Missing required session defaults', + `${req.message ?? `Provide one of: ${req.oneOf.join(', ')}`}\n` + + `Set with: session-set-defaults { "${req.oneOf[0]}": "..." }`, + ); + } + } + } + + // Validate against unchanged internal schema (logic/api untouched) + const validated = internalSchema.parse(merged); + return await logicFunction(validated, getExecutor()); + } catch (error) { + if (error instanceof z.ZodError) { + const msgs = error.errors.map((e) => `${e.path.join('.') || 'root'}: ${e.message}`); + return createErrorResponse( + 'Parameter validation failed', + `Invalid parameters:\n${msgs.join('\n')}\n` + + `Tip: set session defaults via session-set-defaults`, + ); + } + throw error; + } + }; +} +``` + +## Plugin Migration Pattern (Example: build_sim) + +Public schema hides session fields; handler uses session-aware factory with internal schema and requirements; logic function unchanged. + +```typescript +// src/mcp/tools/simulator/build_sim.ts (key parts only) +import { z } from 'zod'; +import { createSessionAwareTool } from '../../../utils/typed-tool-factory.ts'; +import { getDefaultCommandExecutor } from '../../../utils/execution/index.ts'; + +// Existing internal schema (unchanged)… +const baseOptions = { /* as-is (scheme, simulatorId, simulatorName, configuration, …) */ }; +const baseSchemaObject = z.object({ + projectPath: z.string().optional(), + workspacePath: z.string().optional(), + ...baseOptions, +}); +const baseSchema = z.preprocess(nullifyEmptyStrings, baseSchemaObject); +const buildSimulatorSchema = baseSchema + .refine(/* as-is: projectPath XOR workspacePath */) + .refine(/* as-is: simulatorId XOR simulatorName */); + +export type BuildSimulatorParams = z.infer; + +// Public schema = internal minus session-managed fields +const sessionManaged = [ + 'projectPath', + 'workspacePath', + 'scheme', + 'configuration', + 'simulatorId', + 'simulatorName', + 'useLatestOS', +] as const; + +const publicSchemaObject = baseSchemaObject.omit( + Object.fromEntries(sessionManaged.map((k) => [k, true])) as Record, +); + +export default { + name: 'build_sim', + description: 'Builds an app for a simulator using session defaults (scheme/project/sim).', + schema: publicSchemaObject.shape, // what the MCP client sees + handler: createSessionAwareTool({ + internalSchema: buildSimulatorSchema, + logicFunction: build_simLogic, + getExecutor: getDefaultCommandExecutor, + sessionKeys: sessionManaged, + requirements: [ + { allOf: ['scheme'], message: 'scheme is required' }, + { oneOf: ['projectPath', 'workspacePath'], message: 'Provide a project or workspace' }, + { oneOf: ['simulatorId', 'simulatorName'], message: 'Provide simulatorId or simulatorName' }, + ], + }), +}; +``` + +This same pattern applies to `build_run_sim`, `test_sim`, device/macos tools, etc. Public schemas become minimal, while internal schemas and logic remain unchanged. + +## New Tool Group: session-management + +### session_set_defaults.ts + +```typescript +// src/mcp/tools/session-management/session_set_defaults.ts +import { z } from 'zod'; +import { sessionStore, type SessionDefaults } from '../../../utils/session-store.ts'; +import { createTypedTool } from '../../../utils/typed-tool-factory.ts'; +import { getDefaultCommandExecutor } from '../../../utils/execution/index.ts'; + +const schemaObj = z.object({ + projectPath: z.string().optional(), + workspacePath: z.string().optional(), + scheme: z.string().optional(), + configuration: z.string().optional(), + simulatorName: z.string().optional(), + simulatorId: z.string().optional(), + deviceId: z.string().optional(), + useLatestOS: z.boolean().optional(), + arch: z.enum(['arm64', 'x86_64']).optional(), +}); +type Params = z.infer; + +async function logic(params: Params): Promise { + sessionStore.setDefaults(params as Partial); + const current = sessionStore.getAll(); + return { content: [{ type: 'text', text: `Defaults updated:\n${JSON.stringify(current, null, 2)}` }] }; +} + +export default { + name: 'session-set-defaults', + description: 'Set session defaults used by other tools.', + schema: schemaObj.shape, + handler: createTypedTool(schemaObj, logic, getDefaultCommandExecutor), +}; +``` + +### session_clear_defaults.ts + +```typescript +// src/mcp/tools/session-management/session_clear_defaults.ts +import { z } from 'zod'; +import { sessionStore } from '../../../utils/session-store.ts'; +import { createTypedTool } from '../../../utils/typed-tool-factory.ts'; +import { getDefaultCommandExecutor } from '../../../utils/execution/index.ts'; + +const keys = [ + 'projectPath','workspacePath','scheme','configuration', + 'simulatorName','simulatorId','deviceId','useLatestOS','arch', +] as const; +const schemaObj = z.object({ + keys: z.array(z.enum(keys)).optional(), + all: z.boolean().optional(), +}); + +async function logic(params: z.infer) { + if (params.all || !params.keys) sessionStore.clear(); + else sessionStore.clear(params.keys); + return { content: [{ type: 'text', text: 'Session defaults cleared' }] }; +} + +export default { + name: 'session-clear-defaults', + description: 'Clear selected or all session defaults.', + schema: schemaObj.shape, + handler: createTypedTool(schemaObj, logic, getDefaultCommandExecutor), +}; +``` + +### session_show_defaults.ts + +```typescript +// src/mcp/tools/session-management/session_show_defaults.ts +import { sessionStore } from '../../../utils/session-store.ts'; + +export default { + name: 'session-show-defaults', + description: 'Show current session defaults.', + schema: {}, // no args + handler: async () => { + const current = sessionStore.getAll(); + return { content: [{ type: 'text', text: JSON.stringify(current, null, 2) }] }; + }, +}; +``` + +## Step-by-Step Implementation Plan (Incremental, buildable at each step) + +1. **Add SessionStore** ✅ **DONE** + - New file: `src/utils/session-store.ts`. + - No existing code changes; run: `npm run build`, `lint`, `test`. + - Commit checkpoint (after review): see Commit & Review Protocol below. + +2. **Add session-management tools** ✅ **DONE** + - New folder: `src/mcp/tools/session-management` with the three tools above. + - Register via existing plugin discovery (same pattern as others). + - Build and test. + - Commit checkpoint (after review). + +3. **Add session-aware tool factory** ✅ **DONE** + - Add `createSessionAwareTool` to `src/utils/typed-tool-factory.ts` (keep `createTypedTool` intact). + - Unit tests for requirement preflight and merge precedence. + - Commit checkpoint (after review). + +4. **Migrate 2-3 representative tools** + - Example: `simulator/build_sim`, `macos/build_macos`, `device/build_device`. + - Create `publicSchemaObject` (omit session fields), switch handler to `createSessionAwareTool` with requirements. + - Keep internal schema and logic unchanged. Build and test. + - Commit checkpoint (after review). + +5. **Migrate remaining tools in small batches** + - Apply the same pattern across simulator/device/macos/test utilities. + - After each batch: `npm run typecheck`, `lint`, `test`. + - Commit checkpoint (after review). + +6. **Final polish** + - Add tests for session tools and session-aware preflight error messages. + - Ensure public schemas no longer expose session parameters globally. + - Commit checkpoint (after review). + +## Standard Testing & DI Checklist (Mandatory) + +- Handlers must use dependency injection; tests must never call real executors. +- For validation-only tests, calling the handler is acceptable because Zod validation occurs before executor acquisition. +- For logic tests that would otherwise trigger `getDefaultCommandExecutor`, export the logic function and test it directly (no executor needed if logic doesn’t use one): + +```ts +// Example: src/mcp/tools/session-management/session_clear_defaults.ts +export async function sessionClearDefaultsLogic(params: Params): Promise { /* ... */ } +export default { + name: 'session-clear-defaults', + handler: createTypedTool(schemaObj, sessionClearDefaultsLogic, getDefaultCommandExecutor), +}; + +// Test: import logic and call directly to avoid real executor +import plugin, { sessionClearDefaultsLogic } from '../session_clear_defaults.ts'; +``` + +- Add tests for the new group and tools: + - Group metadata test: `src/mcp/tools/session-management/__tests__/index.test.ts` + - Tool tests: `session_set_defaults.test.ts`, `session_clear_defaults.test.ts`, `session_show_defaults.test.ts` + - Utils tests: `src/utils/__tests__/session-store.test.ts` + - Factory tests: `src/utils/__tests__/session-aware-tool-factory.test.ts` covering: + - Preflight requirements (allOf/oneOf) + - Merge precedence (explicit args override session defaults) + - Zod error reporting with helpful tips + +- Always run locally before requesting review: + - `npm run typecheck` + - `npm run lint` + - `npm run test` + +## Commit & Review Protocol (Enforced) + +At the end of each numbered step above: + +1. Ensure all checks pass: `typecheck`, `lint`, `test`. +2. Stage only the files for that step. +3. Prepare a concise commit message focused on the “why”. +4. Request manual review and approval before committing. Do not push. + +Example messages per step: + +- Step 1 (SessionStore) + - `chore(utils): add in-memory SessionStore for session defaults` + - Body: “Introduces singleton SessionStore with set/get/clear/show for session defaults; no behavior changes.” + +- Step 2 (session-management tools) + - `feat(session-management): add set/clear/show session defaults tools and workflow metadata` + - Body: “Adds tools to manage session defaults and exposes workflow metadata; minimal schemas via typed factory.” + +- Step 3 (middleware) + - `feat(utils): add createSessionAwareTool with preflight requirements and args>session merge` + - Body: “Session-aware interop layer performing requirements checks and Zod validation against internal schema.” + +- Step 6 (tests/final polish) + - `test(session-management): add tool, store, and middleware tests; export logic for DI` + - Body: “Covers group metadata, tools, SessionStore, and factory (requirements/merge/errors). No production behavior changes.” + +Approval flow: +- After preparing messages and confirming checks, request maintainer approval. +- On approval: commit locally (no push). +- On rejection: revise and re-run checks. + +Note on commit hooks and selective commits: +- The pre-commit hook runs format/lint/build and can auto-add or modify files, causing additional files to be included in the commit. If you must commit a minimal subset, skip hooks with: `git commit --no-verify` (use sparingly and run `npm run typecheck && npm run lint && npm run test` manually first). + +## Safety, Buildability, Testability + +- Logic functions and their types remain unchanged; existing unit tests that import logic directly continue to pass. +- Public schemas shrink; MCP clients see smaller input schemas without session fields. +- Handlers validate with internal schemas after session-defaults merge, preserving runtime guarantees. +- Preflight requirement checks return clear guidance, e.g., "Provide one of: projectPath or workspacePath" + "Set with: session-set-defaults { "projectPath": "..." }". + +## Developer Usage + +- **Set defaults once**: + - `session-set-defaults { "workspacePath": "...", "scheme": "App", "simulatorName": "iPhone 16" }` +- **Run tools without args**: + - `build_sim {}` +- **Inspect/reset**: + - `session-show-defaults {}` + - `session-clear-defaults { "all": true }` + +## Next Steps + +Would you like me to proceed with Phase 1–3 implementation (store + session tools + middleware), then migrate a first tool (build_sim) and run the test suite? \ No newline at end of file diff --git a/src/mcp/tools/session-management/__tests__/index.test.ts b/src/mcp/tools/session-management/__tests__/index.test.ts new file mode 100644 index 00000000..f263fc5e --- /dev/null +++ b/src/mcp/tools/session-management/__tests__/index.test.ts @@ -0,0 +1,56 @@ +/** + * Tests for session-management workflow metadata + */ +import { describe, it, expect } from 'vitest'; +import { workflow } from '../index.ts'; + +describe('session-management workflow metadata', () => { + describe('Workflow Structure', () => { + it('should export workflow object with required properties', () => { + expect(workflow).toHaveProperty('name'); + expect(workflow).toHaveProperty('description'); + expect(workflow).toHaveProperty('platforms'); + expect(workflow).toHaveProperty('targets'); + expect(workflow).toHaveProperty('capabilities'); + }); + + it('should have correct workflow name', () => { + expect(workflow.name).toBe('session-management'); + }); + + it('should have correct description', () => { + expect(workflow.description).toBe('Manage session defaults for XcodeBuildMCP tools.'); + }); + + it('should have correct platforms array', () => { + expect(workflow.platforms).toEqual(['iOS', 'macOS', 'tvOS', 'watchOS', 'visionOS']); + }); + + it('should have correct targets array', () => { + expect(workflow.targets).toEqual(['simulator', 'device']); + }); + + it('should have correct capabilities array', () => { + expect(workflow.capabilities).toEqual(['configuration', 'state-management']); + }); + }); + + describe('Workflow Validation', () => { + it('should have valid string properties', () => { + expect(typeof workflow.name).toBe('string'); + expect(typeof workflow.description).toBe('string'); + expect(workflow.name.length).toBeGreaterThan(0); + expect(workflow.description.length).toBeGreaterThan(0); + }); + + it('should have valid array properties', () => { + expect(Array.isArray(workflow.platforms)).toBe(true); + expect(Array.isArray(workflow.targets)).toBe(true); + expect(Array.isArray(workflow.capabilities)).toBe(true); + + expect(workflow.platforms.length).toBeGreaterThan(0); + expect(workflow.targets.length).toBeGreaterThan(0); + expect(workflow.capabilities.length).toBeGreaterThan(0); + }); + }); +}); diff --git a/src/mcp/tools/session-management/__tests__/session_clear_defaults.test.ts b/src/mcp/tools/session-management/__tests__/session_clear_defaults.test.ts new file mode 100644 index 00000000..ff83abab --- /dev/null +++ b/src/mcp/tools/session-management/__tests__/session_clear_defaults.test.ts @@ -0,0 +1,75 @@ +import { describe, it, expect, beforeEach } from 'vitest'; +import { sessionStore } from '../../../../utils/session-store.ts'; +import plugin, { sessionClearDefaultsLogic } from '../session_clear_defaults.ts'; + +describe('session-clear-defaults tool', () => { + beforeEach(() => { + sessionStore.clear(); + sessionStore.setDefaults({ + scheme: 'MyScheme', + projectPath: '/path/to/proj.xcodeproj', + simulatorName: 'iPhone 16', + deviceId: 'DEVICE-123', + useLatestOS: true, + arch: 'arm64', + }); + }); + + describe('Export Field Validation (Literal)', () => { + it('should have correct name', () => { + expect(plugin.name).toBe('session-clear-defaults'); + }); + + it('should have correct description', () => { + expect(plugin.description).toBe('Clear selected or all session defaults.'); + }); + + it('should have handler function', () => { + expect(typeof plugin.handler).toBe('function'); + }); + + it('should have schema object', () => { + expect(plugin.schema).toBeDefined(); + expect(typeof plugin.schema).toBe('object'); + }); + }); + + describe('Handler Behavior', () => { + it('should clear specific keys when provided', async () => { + const result = await sessionClearDefaultsLogic({ keys: ['scheme', 'deviceId'] }); + expect(result.isError).toBe(false); + expect(result.content[0].text).toContain('Session defaults cleared'); + + const current = sessionStore.getAll(); + expect(current.scheme).toBeUndefined(); + expect(current.deviceId).toBeUndefined(); + expect(current.projectPath).toBe('/path/to/proj.xcodeproj'); + expect(current.simulatorName).toBe('iPhone 16'); + expect(current.useLatestOS).toBe(true); + expect(current.arch).toBe('arm64'); + }); + + it('should clear all when all=true', async () => { + const result = await sessionClearDefaultsLogic({ all: true }); + expect(result.isError).toBe(false); + expect(result.content[0].text).toBe('Session defaults cleared'); + + const current = sessionStore.getAll(); + expect(Object.keys(current).length).toBe(0); + }); + + it('should clear all when no params provided', async () => { + const result = await sessionClearDefaultsLogic({}); + expect(result.isError).toBe(false); + const current = sessionStore.getAll(); + expect(Object.keys(current).length).toBe(0); + }); + + it('should validate keys enum', async () => { + const result = (await plugin.handler({ keys: ['invalid' as any] })) as any; + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain('Parameter validation failed'); + expect(result.content[0].text).toContain('keys'); + }); + }); +}); diff --git a/src/mcp/tools/session-management/__tests__/session_set_defaults.test.ts b/src/mcp/tools/session-management/__tests__/session_set_defaults.test.ts new file mode 100644 index 00000000..6315fc2a --- /dev/null +++ b/src/mcp/tools/session-management/__tests__/session_set_defaults.test.ts @@ -0,0 +1,60 @@ +import { describe, it, expect, beforeEach } from 'vitest'; +import { sessionStore } from '../../../../utils/session-store.ts'; +import plugin, { sessionSetDefaultsLogic } from '../session_set_defaults.ts'; + +describe('session-set-defaults tool', () => { + beforeEach(() => { + sessionStore.clear(); + }); + + describe('Export Field Validation (Literal)', () => { + it('should have correct name', () => { + expect(plugin.name).toBe('session-set-defaults'); + }); + + it('should have correct description', () => { + expect(plugin.description).toBe( + 'Set the session defaults needed by many tools. Most tools require one or more session defaults to be set before they can be used. Agents should set the relevent defaults at the beginning of a session.', + ); + }); + + it('should have handler function', () => { + expect(typeof plugin.handler).toBe('function'); + }); + + it('should have schema object', () => { + expect(plugin.schema).toBeDefined(); + expect(typeof plugin.schema).toBe('object'); + }); + }); + + describe('Handler Behavior', () => { + it('should set provided defaults and return updated state', async () => { + const result = await sessionSetDefaultsLogic({ + scheme: 'MyScheme', + simulatorName: 'iPhone 16', + useLatestOS: true, + arch: 'arm64', + }); + + expect(result.isError).toBe(false); + expect(result.content[0].text).toContain('Defaults updated:'); + + const current = sessionStore.getAll(); + expect(current.scheme).toBe('MyScheme'); + expect(current.simulatorName).toBe('iPhone 16'); + expect(current.useLatestOS).toBe(true); + expect(current.arch).toBe('arm64'); + }); + + it('should validate parameter types via Zod', async () => { + const result = await plugin.handler({ + useLatestOS: 'yes' as unknown as boolean, + }); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain('Parameter validation failed'); + expect(result.content[0].text).toContain('useLatestOS'); + }); + }); +}); diff --git a/src/mcp/tools/session-management/__tests__/session_show_defaults.test.ts b/src/mcp/tools/session-management/__tests__/session_show_defaults.test.ts new file mode 100644 index 00000000..f8f45802 --- /dev/null +++ b/src/mcp/tools/session-management/__tests__/session_show_defaults.test.ts @@ -0,0 +1,45 @@ +import { describe, it, expect, beforeEach } from 'vitest'; +import { sessionStore } from '../../../../utils/session-store.ts'; +import plugin from '../session_show_defaults.ts'; + +describe('session-show-defaults tool', () => { + beforeEach(() => { + sessionStore.clear(); + }); + + describe('Export Field Validation (Literal)', () => { + it('should have correct name', () => { + expect(plugin.name).toBe('session-show-defaults'); + }); + + it('should have correct description', () => { + expect(plugin.description).toBe('Show current session defaults.'); + }); + + it('should have handler function', () => { + expect(typeof plugin.handler).toBe('function'); + }); + + it('should have empty schema', () => { + expect(plugin.schema).toEqual({}); + }); + }); + + describe('Handler Behavior', () => { + it('should return empty defaults when none set', async () => { + const result = await plugin.handler({}); + expect(result.isError).toBe(false); + const parsed = JSON.parse(result.content[0].text); + expect(parsed).toEqual({}); + }); + + it('should return current defaults when set', async () => { + sessionStore.setDefaults({ scheme: 'MyScheme', simulatorId: 'SIM-123' }); + const result = await plugin.handler({}); + expect(result.isError).toBe(false); + const parsed = JSON.parse(result.content[0].text); + expect(parsed.scheme).toBe('MyScheme'); + expect(parsed.simulatorId).toBe('SIM-123'); + }); + }); +}); diff --git a/src/mcp/tools/session-management/index.ts b/src/mcp/tools/session-management/index.ts new file mode 100644 index 00000000..3614d164 --- /dev/null +++ b/src/mcp/tools/session-management/index.ts @@ -0,0 +1,8 @@ +export const workflow = { + name: 'session-management', + description: + 'Manage session defaults for projectPath/workspacePath, scheme, configuration, simulatorName/simulatorId, deviceId, useLatestOS and arch. These defaults are required by many tools and must be set before attempting to call tools that would depend on these values.', + platforms: ['iOS', 'macOS', 'tvOS', 'watchOS', 'visionOS'], + targets: ['simulator', 'device'], + capabilities: ['configuration', 'state-management'], +}; diff --git a/src/mcp/tools/session-management/session_clear_defaults.ts b/src/mcp/tools/session-management/session_clear_defaults.ts new file mode 100644 index 00000000..286a86fc --- /dev/null +++ b/src/mcp/tools/session-management/session_clear_defaults.ts @@ -0,0 +1,37 @@ +import { z } from 'zod'; +import { sessionStore } from '../../../utils/session-store.ts'; +import { createTypedTool } from '../../../utils/typed-tool-factory.ts'; +import { getDefaultCommandExecutor } from '../../../utils/execution/index.ts'; +import type { ToolResponse } from '../../../types/common.ts'; + +const keys = [ + 'projectPath', + 'workspacePath', + 'scheme', + 'configuration', + 'simulatorName', + 'simulatorId', + 'deviceId', + 'useLatestOS', + 'arch', +] as const; + +const schemaObj = z.object({ + keys: z.array(z.enum(keys)).optional(), + all: z.boolean().optional(), +}); + +type Params = z.infer; + +export async function sessionClearDefaultsLogic(params: Params): Promise { + if (params.all || !params.keys) sessionStore.clear(); + else sessionStore.clear(params.keys); + return { content: [{ type: 'text', text: 'Session defaults cleared' }], isError: false }; +} + +export default { + name: 'session-clear-defaults', + description: 'Clear selected or all session defaults.', + schema: schemaObj.shape, + handler: createTypedTool(schemaObj, sessionClearDefaultsLogic, getDefaultCommandExecutor), +}; diff --git a/src/mcp/tools/session-management/session_set_defaults.ts b/src/mcp/tools/session-management/session_set_defaults.ts new file mode 100644 index 00000000..f386ea60 --- /dev/null +++ b/src/mcp/tools/session-management/session_set_defaults.ts @@ -0,0 +1,36 @@ +import { z } from 'zod'; +import { sessionStore, type SessionDefaults } from '../../../utils/session-store.ts'; +import { createTypedTool } from '../../../utils/typed-tool-factory.ts'; +import { getDefaultCommandExecutor } from '../../../utils/execution/index.ts'; +import type { ToolResponse } from '../../../types/common.ts'; + +const schemaObj = z.object({ + projectPath: z.string().optional(), + workspacePath: z.string().optional(), + scheme: z.string().optional(), + configuration: z.string().optional(), + simulatorName: z.string().optional(), + simulatorId: z.string().optional(), + deviceId: z.string().optional(), + useLatestOS: z.boolean().optional(), + arch: z.enum(['arm64', 'x86_64']).optional(), +}); + +type Params = z.infer; + +export async function sessionSetDefaultsLogic(params: Params): Promise { + sessionStore.setDefaults(params as Partial); + const current = sessionStore.getAll(); + return { + content: [{ type: 'text', text: `Defaults updated:\n${JSON.stringify(current, null, 2)}` }], + isError: false, + }; +} + +export default { + name: 'session-set-defaults', + description: + 'Set the session defaults needed by many tools. Most tools require one or more session defaults to be set before they can be used. Agents should set the relevent defaults at the beginning of a session.', + schema: schemaObj.shape, + handler: createTypedTool(schemaObj, sessionSetDefaultsLogic, getDefaultCommandExecutor), +}; diff --git a/src/mcp/tools/session-management/session_show_defaults.ts b/src/mcp/tools/session-management/session_show_defaults.ts new file mode 100644 index 00000000..8c4e6f0b --- /dev/null +++ b/src/mcp/tools/session-management/session_show_defaults.ts @@ -0,0 +1,12 @@ +import { sessionStore } from '../../../utils/session-store.ts'; +import type { ToolResponse } from '../../../types/common.ts'; + +export default { + name: 'session-show-defaults', + description: 'Show current session defaults.', + schema: {}, + handler: async (): Promise => { + const current = sessionStore.getAll(); + return { content: [{ type: 'text', text: JSON.stringify(current, null, 2) }], isError: false }; + }, +}; diff --git a/src/utils/__tests__/session-aware-tool-factory.test.ts b/src/utils/__tests__/session-aware-tool-factory.test.ts new file mode 100644 index 00000000..66af7491 --- /dev/null +++ b/src/utils/__tests__/session-aware-tool-factory.test.ts @@ -0,0 +1,112 @@ +import { describe, it, expect, beforeEach } from 'vitest'; +import { z } from 'zod'; +import { createSessionAwareTool } from '../typed-tool-factory.ts'; +import { sessionStore } from '../session-store.ts'; +import { createMockExecutor } from '../../test-utils/mock-executors.ts'; + +describe('createSessionAwareTool', () => { + beforeEach(() => { + sessionStore.clear(); + }); + + const internalSchema = z + .object({ + scheme: z.string(), + projectPath: z.string().optional(), + workspacePath: z.string().optional(), + simulatorId: z.string().optional(), + simulatorName: z.string().optional(), + }) + .refine((v) => !!v.projectPath !== !!v.workspacePath, { + message: 'projectPath and workspacePath are mutually exclusive', + path: ['projectPath'], + }) + .refine((v) => !!v.simulatorId !== !!v.simulatorName, { + message: 'simulatorId and simulatorName are mutually exclusive', + path: ['simulatorId'], + }); + + type Params = z.infer; + + async function logic(_params: Params): Promise { + return { content: [{ type: 'text', text: 'OK' }], isError: false }; + } + + const handler = createSessionAwareTool({ + internalSchema, + logicFunction: logic, + getExecutor: () => createMockExecutor({ success: true }), + requirements: [ + { allOf: ['scheme'], message: 'scheme is required' }, + { oneOf: ['projectPath', 'workspacePath'], message: 'Provide a project or workspace' }, + { oneOf: ['simulatorId', 'simulatorName'], message: 'Provide simulatorId or simulatorName' }, + ], + }); + + it('should merge session defaults and satisfy requirements', async () => { + sessionStore.setDefaults({ + scheme: 'App', + projectPath: '/path/proj.xcodeproj', + simulatorId: 'SIM-1', + }); + + const result = await handler({}); + expect(result.isError).toBe(false); + expect(result.content[0].text).toBe('OK'); + }); + + it('should prefer explicit args over session defaults (same key wins)', async () => { + // Create a handler that echoes the chosen scheme + const echoHandler = createSessionAwareTool({ + internalSchema, + logicFunction: async (params) => ({ + content: [{ type: 'text', text: params.scheme }], + isError: false, + }), + getExecutor: () => createMockExecutor({ success: true }), + requirements: [ + { allOf: ['scheme'], message: 'scheme is required' }, + { oneOf: ['projectPath', 'workspacePath'], message: 'Provide a project or workspace' }, + { + oneOf: ['simulatorId', 'simulatorName'], + message: 'Provide simulatorId or simulatorName', + }, + ], + }); + + sessionStore.setDefaults({ + scheme: 'Default', + projectPath: '/a.xcodeproj', + simulatorId: 'SIM-A', + }); + const result = await echoHandler({ scheme: 'FromArgs' }); + expect(result.isError).toBe(false); + expect(result.content[0].text).toBe('FromArgs'); + }); + + it('should return friendly error when allOf requirement missing', async () => { + const result = await handler({ projectPath: '/p.xcodeproj', simulatorId: 'SIM-1' }); + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain('Missing required session defaults'); + expect(result.content[0].text).toContain('scheme is required'); + }); + + it('should return friendly error when oneOf requirement missing', async () => { + const result = await handler({ scheme: 'App', simulatorId: 'SIM-1' }); + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain('Missing required session defaults'); + expect(result.content[0].text).toContain('Provide a project or workspace'); + }); + + it('should surface Zod validation errors with tip when invalid', async () => { + const badHandler = createSessionAwareTool({ + internalSchema, + logicFunction: logic, + getExecutor: () => createMockExecutor({ success: true }), + }); + const result = await badHandler({ scheme: 123 }); + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain('Parameter validation failed'); + expect(result.content[0].text).toContain('Tip: set session defaults'); + }); +}); diff --git a/src/utils/__tests__/session-store.test.ts b/src/utils/__tests__/session-store.test.ts new file mode 100644 index 00000000..82a9e29e --- /dev/null +++ b/src/utils/__tests__/session-store.test.ts @@ -0,0 +1,38 @@ +import { describe, it, expect, beforeEach } from 'vitest'; +import { sessionStore } from '../session-store.ts'; + +describe('SessionStore', () => { + beforeEach(() => { + sessionStore.clear(); + }); + + it('should set and get defaults', () => { + sessionStore.setDefaults({ scheme: 'App', useLatestOS: true }); + expect(sessionStore.get('scheme')).toBe('App'); + expect(sessionStore.get('useLatestOS')).toBe(true); + }); + + it('should merge defaults on set', () => { + sessionStore.setDefaults({ scheme: 'App' }); + sessionStore.setDefaults({ simulatorName: 'iPhone 16' }); + const all = sessionStore.getAll(); + expect(all.scheme).toBe('App'); + expect(all.simulatorName).toBe('iPhone 16'); + }); + + it('should clear specific keys', () => { + sessionStore.setDefaults({ scheme: 'App', simulatorId: 'SIM-1', deviceId: 'DEV-1' }); + sessionStore.clear(['simulatorId']); + const all = sessionStore.getAll(); + expect(all.scheme).toBe('App'); + expect(all.simulatorId).toBeUndefined(); + expect(all.deviceId).toBe('DEV-1'); + }); + + it('should clear all when no keys provided', () => { + sessionStore.setDefaults({ scheme: 'App', simulatorId: 'SIM-1' }); + sessionStore.clear(); + const all = sessionStore.getAll(); + expect(Object.keys(all).length).toBe(0); + }); +}); diff --git a/src/utils/session-store.ts b/src/utils/session-store.ts new file mode 100644 index 00000000..97d76dcd --- /dev/null +++ b/src/utils/session-store.ts @@ -0,0 +1,42 @@ +import { log } from './logger.ts'; + +export type SessionDefaults = { + projectPath?: string; + workspacePath?: string; + scheme?: string; + configuration?: string; + simulatorName?: string; + simulatorId?: string; + deviceId?: string; + useLatestOS?: boolean; + arch?: 'arm64' | 'x86_64'; +}; + +class SessionStore { + private defaults: SessionDefaults = {}; + + setDefaults(partial: Partial): void { + this.defaults = { ...this.defaults, ...partial }; + log('info', `[Session] Defaults updated: ${Object.keys(partial).join(', ')}`); + } + + clear(keys?: (keyof SessionDefaults)[]): void { + if (!keys || keys.length === 0) { + this.defaults = {}; + log('info', '[Session] All defaults cleared'); + return; + } + for (const k of keys) delete this.defaults[k]; + log('info', `[Session] Defaults cleared: ${keys.join(', ')}`); + } + + get(key: K): SessionDefaults[K] { + return this.defaults[key]; + } + + getAll(): SessionDefaults { + return { ...this.defaults }; + } +} + +export const sessionStore = new SessionStore(); diff --git a/src/utils/typed-tool-factory.ts b/src/utils/typed-tool-factory.ts index 1eddf16a..7076469a 100644 --- a/src/utils/typed-tool-factory.ts +++ b/src/utils/typed-tool-factory.ts @@ -13,6 +13,7 @@ import { z } from 'zod'; import { ToolResponse } from '../types/common.ts'; import type { CommandExecutor } from './execution/index.ts'; import { createErrorResponse } from './responses/index.ts'; +import { sessionStore, type SessionDefaults } from './session-store.ts'; /** * Creates a type-safe tool handler that validates parameters at runtime @@ -58,3 +59,70 @@ export function createTypedTool( } }; } + +export type SessionRequirement = + | { allOf: (keyof SessionDefaults)[]; message?: string } + | { oneOf: (keyof SessionDefaults)[]; message?: string }; + +function missingFromArgsAndSession( + keys: (keyof SessionDefaults)[], + args: Record, +): string[] { + return keys.filter((k) => args[k] == null && sessionStore.get(k) == null); +} + +export function createSessionAwareTool(opts: { + internalSchema: z.ZodType; + logicFunction: (params: TParams, executor: CommandExecutor) => Promise; + getExecutor: () => CommandExecutor; + sessionKeys?: (keyof SessionDefaults)[]; + requirements?: SessionRequirement[]; +}) { + const { internalSchema, logicFunction, getExecutor, requirements = [] } = opts; + + return async (rawArgs: Record): Promise => { + try { + const merged: Record = { ...sessionStore.getAll(), ...rawArgs }; + + for (const req of requirements) { + if ('allOf' in req) { + const missing = missingFromArgsAndSession(req.allOf, rawArgs); + if (missing.length > 0) { + return createErrorResponse( + 'Missing required session defaults', + `${req.message ?? `Required: ${req.allOf.join(', ')}`}\n` + + `Set with: session-set-defaults { ${missing + .map((k) => `"${k}": "..."`) + .join(', ')} }`, + ); + } + } else if ('oneOf' in req) { + const satisfied = req.oneOf.some((k) => merged[k] != null); + if (!satisfied) { + return createErrorResponse( + 'Missing required session defaults', + `${req.message ?? `Provide one of: ${req.oneOf.join(', ')}`}\n` + + `Set with: session-set-defaults { "${req.oneOf[0]}": "..." }`, + ); + } + } + } + + const validated = internalSchema.parse(merged); + return await logicFunction(validated, getExecutor()); + } catch (error) { + if (error instanceof z.ZodError) { + const errorMessages = error.errors.map((e) => { + const path = e.path.length > 0 ? `${e.path.join('.')}` : 'root'; + return `${path}: ${e.message}`; + }); + + return createErrorResponse( + 'Parameter validation failed', + `Invalid parameters:\n${errorMessages.join('\n')}\nTip: set session defaults via session-set-defaults`, + ); + } + throw error; + } + }; +} From a613b9fba7de99d163c0893dc2dbd253adab582d Mon Sep 17 00:00:00 2001 From: Cameron Cooke Date: Sun, 5 Oct 2025 19:48:38 +0100 Subject: [PATCH 02/10] feat(simulator): migrate build_sim to session-aware defaults and concise description docs(session-management): add Tool Description Policy and update plan example test: update build_sim and session-management tests for session-aware preflight and description --- docs/session_management_plan.md | 70 ++++++++- .../__tests__/index.test.ts | 4 +- .../simulator/__tests__/build_sim.test.ts | 135 +++--------------- src/mcp/tools/simulator/build_sim.ts | 55 ++++--- 4 files changed, 115 insertions(+), 149 deletions(-) diff --git a/docs/session_management_plan.md b/docs/session_management_plan.md index 36904443..1ce52f0a 100644 --- a/docs/session_management_plan.md +++ b/docs/session_management_plan.md @@ -185,7 +185,7 @@ const publicSchemaObject = baseSchemaObject.omit( export default { name: 'build_sim', - description: 'Builds an app for a simulator using session defaults (scheme/project/sim).', + description: 'Builds an app for an iOS simulator.', schema: publicSchemaObject.shape, // what the MCP client sees handler: createSessionAwareTool({ internalSchema: buildSimulatorSchema, @@ -356,11 +356,26 @@ import plugin, { sessionClearDefaultsLogic } from '../session_clear_defaults.ts' - `npm run lint` - `npm run test` +### Minimal Changes Policy for Tests (Enforced) + +- Only make material, essential edits to tests required by the code change (e.g., new preflight error messages or added/removed fields). +- Do not change sample input values or defaults in tests (e.g., flipping a boolean like `preferXcodebuild`) unless strictly necessary to validate behavior. +- Preserve the original intent and coverage of logic-function tests; keep handler vs logic boundaries intact. +- When session-awareness is added, prefer setting/clearing session defaults around tests rather than altering existing assertions or sample inputs. + +### Tool Description Policy (Enforced) + +- Keep tool descriptions concise (maximum one short sentence). +- Do not mention session defaults, setup steps, examples, or parameter relationships in descriptions. +- Use clear, imperative phrasing (e.g., "Builds an app for an iOS simulator."). +- Apply consistently across all migrated tools; update any tests that assert `description` to match the concise string only. + ## Commit & Review Protocol (Enforced) At the end of each numbered step above: 1. Ensure all checks pass: `typecheck`, `lint`, `test`. + - Verify tool descriptions comply with the Tool Description Policy (concise, no session-defaults mention). 2. Stage only the files for that step. 3. Prepare a concise commit message focused on the “why”. 4. Request manual review and approval before committing. Do not push. @@ -408,6 +423,59 @@ Note on commit hooks and selective commits: - `session-show-defaults {}` - `session-clear-defaults { "all": true }` +## Manual Testing with mcpli (CLI) + +The following commands exercise the session workflow end‑to‑end using the built server. + +1) Build the server (required after code changes): + +```bash +npm run build +``` + +2) Discover a scheme (optional helper): + +```bash +mcpli --raw list-schemes --projectPath "/Volumes/Developer/XcodeBuildMCP/example_projects/iOS/MCPTest.xcodeproj" -- node build/index.js +``` + +3) Set the session defaults (project/workspace, scheme, and simulator): + +```bash +mcpli --raw session-set-defaults \ + --projectPath "/Volumes/Developer/XcodeBuildMCP/example_projects/iOS/MCPTest.xcodeproj" \ + --scheme MCPTest \ + --simulatorName "iPhone 16" \ + -- node build/index.js +``` + +4) Verify defaults are stored: + +```bash +mcpli --raw session-show-defaults -- node build/index.js +``` + +5) Run a session‑aware tool with zero or minimal args (defaults are merged automatically): + +```bash +# Optionally provide a scratch derived data path and a short timeout +mcpli --tool-timeout=60 --raw build-sim --derivedDataPath "/tmp/XBMCP_DD" -- node build/index.js +``` + +Troubleshooting: + +- If you see validation errors like “Missing required session defaults …”, (re)run step 3 with the missing keys. +- If you see connect ECONNREFUSED or the daemon appears flaky: + - Check logs: `mcpli daemon log --since=10m -- node build/index.js` + - Restart daemon: `mcpli daemon restart -- node build/index.js` + - Clean daemon state: `mcpli daemon clean -- node build/index.js` then `mcpli daemon start -- node build/index.js` + - After code changes, always: `npm run build` then `mcpli daemon restart -- node build/index.js` + +Notes: + +- Public schemas for session‑aware tools intentionally omit session fields (e.g., `scheme`, `projectPath`, `simulatorName`). Provide them once via `session-set-defaults` and then call the tool with zero/minimal flags. +- Use `--tool-timeout=` to cap long‑running builds during manual testing. + ## Next Steps Would you like me to proceed with Phase 1–3 implementation (store + session tools + middleware), then migrate a first tool (build_sim) and run the test suite? \ No newline at end of file diff --git a/src/mcp/tools/session-management/__tests__/index.test.ts b/src/mcp/tools/session-management/__tests__/index.test.ts index f263fc5e..88ba9d24 100644 --- a/src/mcp/tools/session-management/__tests__/index.test.ts +++ b/src/mcp/tools/session-management/__tests__/index.test.ts @@ -19,7 +19,9 @@ describe('session-management workflow metadata', () => { }); it('should have correct description', () => { - expect(workflow.description).toBe('Manage session defaults for XcodeBuildMCP tools.'); + expect(workflow.description).toBe( + 'Manage session defaults for projectPath/workspacePath, scheme, configuration, simulatorName/simulatorId, deviceId, useLatestOS and arch. These defaults are required by many tools and must be set before attempting to call tools that would depend on these values.', + ); }); it('should have correct platforms array', () => { diff --git a/src/mcp/tools/simulator/__tests__/build_sim.test.ts b/src/mcp/tools/simulator/__tests__/build_sim.test.ts index af69244c..969c6099 100644 --- a/src/mcp/tools/simulator/__tests__/build_sim.test.ts +++ b/src/mcp/tools/simulator/__tests__/build_sim.test.ts @@ -1,12 +1,15 @@ import { describe, it, expect, beforeEach } from 'vitest'; import { z } from 'zod'; import { createMockExecutor } from '../../../../test-utils/mock-executors.ts'; +import { sessionStore } from '../../../../utils/session-store.ts'; // Import the plugin and logic function import buildSim, { build_simLogic } from '../build_sim.ts'; describe('build_sim tool', () => { - // Only clear any remaining mocks if needed + beforeEach(() => { + sessionStore.clear(); + }); describe('Export Field Validation (Literal)', () => { it('should have correct name', () => { @@ -14,140 +17,48 @@ describe('build_sim tool', () => { }); it('should have correct description', () => { - expect(buildSim.description).toBe( - "Builds an app from a project or workspace for a specific simulator by UUID or name. Provide exactly one of projectPath or workspacePath, and exactly one of simulatorId or simulatorName. IMPORTANT: Requires either projectPath or workspacePath, plus scheme and either simulatorId or simulatorName. Example: build_sim({ projectPath: '/path/to/MyProject.xcodeproj', scheme: 'MyScheme', simulatorName: 'iPhone 16' })", - ); + expect(buildSim.description).toBe('Builds an app for an iOS simulator.'); }); it('should have handler function', () => { expect(typeof buildSim.handler).toBe('function'); }); - it('should have correct schema with required and optional fields', () => { + it('should have correct public schema (only non-session fields)', () => { const schema = z.object(buildSim.schema); - // Valid inputs - workspace - expect( - schema.safeParse({ - workspacePath: '/path/to/workspace', - scheme: 'MyScheme', - simulatorName: 'iPhone 16', - }).success, - ).toBe(true); + // Public schema should allow empty input + expect(schema.safeParse({}).success).toBe(true); - // Valid inputs - project + // Valid public inputs expect( schema.safeParse({ - projectPath: '/path/to/project.xcodeproj', - scheme: 'MyScheme', - simulatorName: 'iPhone 16', - }).success, - ).toBe(true); - - expect( - schema.safeParse({ - workspacePath: '/path/to/workspace', - scheme: 'MyScheme', - simulatorName: 'iPhone 16', - configuration: 'Release', derivedDataPath: '/path/to/derived', extraArgs: ['--verbose'], - useLatestOS: true, preferXcodebuild: false, }).success, ).toBe(true); - // Invalid inputs - missing required fields - // Note: simulatorId/simulatorName are optional at schema level, XOR validation at runtime - expect( - schema.safeParse({ - workspacePath: '/path/to/workspace', - scheme: 'MyScheme', - }).success, - ).toBe(true); // Schema validation passes, runtime XOR validation would catch missing simulator fields - - expect( - schema.safeParse({ - workspacePath: '/path/to/workspace', - simulatorName: 'iPhone 16', - }).success, - ).toBe(false); - - expect( - schema.safeParse({ - scheme: 'MyScheme', - simulatorName: 'iPhone 16', - }).success, - ).toBe(true); // Base schema allows both fields optional, XOR validation happens at handler level - - // Invalid types - expect( - schema.safeParse({ - workspacePath: 123, - scheme: 'MyScheme', - simulatorName: 'iPhone 16', - }).success, - ).toBe(false); - - expect( - schema.safeParse({ - workspacePath: '/path/to/workspace', - scheme: 123, - simulatorName: 'iPhone 16', - }).success, - ).toBe(false); - - expect( - schema.safeParse({ - workspacePath: '/path/to/workspace', - scheme: 'MyScheme', - simulatorName: 123, - }).success, - ).toBe(false); - }); - - it('should validate XOR constraint between projectPath and workspacePath', () => { - const schema = z.object(buildSim.schema); - - // Both projectPath and workspacePath provided - should be invalid - expect( - schema.safeParse({ - projectPath: '/path/to/project.xcodeproj', - workspacePath: '/path/to/workspace', - scheme: 'MyScheme', - simulatorName: 'iPhone 16', - }).success, - ).toBe(true); // Schema validation passes, but handler validation will catch this - - // Neither provided - should be invalid - expect( - schema.safeParse({ - scheme: 'MyScheme', - simulatorName: 'iPhone 16', - }).success, - ).toBe(true); // Schema validation passes, but handler validation will catch this + // Invalid types on public inputs + expect(schema.safeParse({ derivedDataPath: 123 }).success).toBe(false); + expect(schema.safeParse({ extraArgs: [123] }).success).toBe(false); + expect(schema.safeParse({ preferXcodebuild: 'yes' }).success).toBe(false); }); }); describe('Parameter Validation', () => { it('should handle missing both projectPath and workspacePath', async () => { - const mockExecutor = createMockExecutor({ success: true, output: 'Build succeeded' }); - - // Since we use XOR validation, this should fail at the handler level const result = await buildSim.handler({ scheme: 'MyScheme', simulatorName: 'iPhone 16', }); expect(result.isError).toBe(true); - expect(result.content[0].text).toContain('Parameter validation failed'); - expect(result.content[0].text).toContain('Either projectPath or workspacePath is required'); + expect(result.content[0].text).toContain('Missing required session defaults'); + expect(result.content[0].text).toContain('Provide a project or workspace'); }); it('should handle both projectPath and workspacePath provided', async () => { - const mockExecutor = createMockExecutor({ success: true, output: 'Build succeeded' }); - - // Since we use XOR validation, this should fail at the handler level const result = await buildSim.handler({ projectPath: '/path/to/project.xcodeproj', workspacePath: '/path/to/workspace', @@ -188,19 +99,14 @@ describe('build_sim tool', () => { }); it('should handle missing scheme parameter', async () => { - const mockExecutor = createMockExecutor({ success: true, output: 'Build succeeded' }); - - // Since we removed manual validation, this test now checks that Zod validation works - // by testing the typed tool handler through the default export const result = await buildSim.handler({ workspacePath: '/path/to/workspace', simulatorName: 'iPhone 16', }); expect(result.isError).toBe(true); - expect(result.content[0].text).toContain('Parameter validation failed'); - expect(result.content[0].text).toContain('scheme'); - expect(result.content[0].text).toContain('Required'); + expect(result.content[0].text).toContain('Missing required session defaults'); + expect(result.content[0].text).toContain('scheme is required'); }); it('should handle empty scheme parameter', async () => { @@ -229,17 +135,14 @@ describe('build_sim tool', () => { }); it('should handle missing both simulatorId and simulatorName', async () => { - const mockExecutor = createMockExecutor({ success: true, output: 'Build succeeded' }); - - // Should fail with XOR validation const result = await buildSim.handler({ workspacePath: '/path/to/workspace', scheme: 'MyScheme', }); expect(result.isError).toBe(true); - expect(result.content[0].text).toContain('Parameter validation failed'); - expect(result.content[0].text).toContain('Either simulatorId or simulatorName is required'); + expect(result.content[0].text).toContain('Missing required session defaults'); + expect(result.content[0].text).toContain('Provide simulatorId or simulatorName'); }); it('should handle both simulatorId and simulatorName provided', async () => { diff --git a/src/mcp/tools/simulator/build_sim.ts b/src/mcp/tools/simulator/build_sim.ts index 41145c78..66ae5054 100644 --- a/src/mcp/tools/simulator/build_sim.ts +++ b/src/mcp/tools/simulator/build_sim.ts @@ -12,6 +12,7 @@ import { executeXcodeBuildCommand } from '../../../utils/build/index.ts'; import { ToolResponse, XcodePlatform } from '../../../types/common.ts'; import type { CommandExecutor } from '../../../utils/execution/index.ts'; import { getDefaultCommandExecutor } from '../../../utils/execution/index.ts'; +import { createSessionAwareTool } from '../../../utils/typed-tool-factory.ts'; import { nullifyEmptyStrings } from '../../../utils/schema-helpers.ts'; // Unified schema: XOR between projectPath and workspacePath, and XOR between simulatorId and simulatorName @@ -135,37 +136,29 @@ export async function build_simLogic( return _handleSimulatorBuildLogic(processedParams, executor); } +// Public schema = internal minus session-managed fields +const publicSchemaObject = baseSchemaObject.omit({ + projectPath: true, + workspacePath: true, + scheme: true, + configuration: true, + simulatorId: true, + simulatorName: true, + useLatestOS: true, +} as const); + export default { name: 'build_sim', - description: - "Builds an app from a project or workspace for a specific simulator by UUID or name. Provide exactly one of projectPath or workspacePath, and exactly one of simulatorId or simulatorName. IMPORTANT: Requires either projectPath or workspacePath, plus scheme and either simulatorId or simulatorName. Example: build_sim({ projectPath: '/path/to/MyProject.xcodeproj', scheme: 'MyScheme', simulatorName: 'iPhone 16' })", - schema: baseSchemaObject.shape, // MCP SDK compatibility - handler: async (args: Record): Promise => { - try { - // Runtime validation with XOR constraints - const validatedParams = buildSimulatorSchema.parse(args); - return await build_simLogic(validatedParams, getDefaultCommandExecutor()); - } catch (error) { - if (error instanceof z.ZodError) { - // Format validation errors in a user-friendly way - const errorMessages = error.errors.map((e) => { - const path = e.path.length > 0 ? `${e.path.join('.')}` : 'root'; - return `${path}: ${e.message}`; - }); - - return { - content: [ - { - type: 'text', - text: `Parameter validation failed. Invalid parameters:\n${errorMessages.join('\n')}`, - }, - ], - isError: true, - }; - } - - // Re-throw unexpected errors - throw error; - } - }, + description: 'Builds an app for an iOS simulator.', + schema: publicSchemaObject.shape, // MCP SDK compatibility (public inputs only) + handler: createSessionAwareTool({ + internalSchema: buildSimulatorSchema as unknown as z.ZodType, + logicFunction: build_simLogic, + getExecutor: getDefaultCommandExecutor, + requirements: [ + { allOf: ['scheme'], message: 'scheme is required' }, + { oneOf: ['projectPath', 'workspacePath'], message: 'Provide a project or workspace' }, + { oneOf: ['simulatorId', 'simulatorName'], message: 'Provide simulatorId or simulatorName' }, + ], + }), }; From 654025b42216a40ec1db855add861c1411a70ef5 Mon Sep 17 00:00:00 2001 From: Cameron Cooke Date: Sun, 5 Oct 2025 20:15:09 +0100 Subject: [PATCH 03/10] fix(session-management): enforce afterEach cleanup; correct CLI example; improve oneOf error hints; add exclusivePairs pruning; clear([]) no-op; typo fix in description --- docs/session_management_plan.md | 2 +- .../__tests__/session_clear_defaults.test.ts | 6 +++- .../__tests__/session_set_defaults.test.ts | 2 +- .../__tests__/session_show_defaults.test.ts | 6 +++- .../session_set_defaults.ts | 2 +- src/mcp/tools/simulator/build_sim.ts | 4 +++ src/utils/__tests__/session-store.test.ts | 8 +++++ src/utils/session-store.ts | 7 ++++- src/utils/typed-tool-factory.ts | 30 +++++++++++++++++-- 9 files changed, 58 insertions(+), 9 deletions(-) diff --git a/docs/session_management_plan.md b/docs/session_management_plan.md index 1ce52f0a..c6b26a2f 100644 --- a/docs/session_management_plan.md +++ b/docs/session_management_plan.md @@ -459,7 +459,7 @@ mcpli --raw session-show-defaults -- node build/index.js ```bash # Optionally provide a scratch derived data path and a short timeout -mcpli --tool-timeout=60 --raw build-sim --derivedDataPath "/tmp/XBMCP_DD" -- node build/index.js +mcpli --tool-timeout=60 --raw build_sim --derivedDataPath "/tmp/XBMCP_DD" -- node build/index.js ``` Troubleshooting: diff --git a/src/mcp/tools/session-management/__tests__/session_clear_defaults.test.ts b/src/mcp/tools/session-management/__tests__/session_clear_defaults.test.ts index ff83abab..7d4a06df 100644 --- a/src/mcp/tools/session-management/__tests__/session_clear_defaults.test.ts +++ b/src/mcp/tools/session-management/__tests__/session_clear_defaults.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, beforeEach } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { sessionStore } from '../../../../utils/session-store.ts'; import plugin, { sessionClearDefaultsLogic } from '../session_clear_defaults.ts'; @@ -15,6 +15,10 @@ describe('session-clear-defaults tool', () => { }); }); + afterEach(() => { + sessionStore.clear(); + }); + describe('Export Field Validation (Literal)', () => { it('should have correct name', () => { expect(plugin.name).toBe('session-clear-defaults'); diff --git a/src/mcp/tools/session-management/__tests__/session_set_defaults.test.ts b/src/mcp/tools/session-management/__tests__/session_set_defaults.test.ts index 6315fc2a..3ad15f85 100644 --- a/src/mcp/tools/session-management/__tests__/session_set_defaults.test.ts +++ b/src/mcp/tools/session-management/__tests__/session_set_defaults.test.ts @@ -14,7 +14,7 @@ describe('session-set-defaults tool', () => { it('should have correct description', () => { expect(plugin.description).toBe( - 'Set the session defaults needed by many tools. Most tools require one or more session defaults to be set before they can be used. Agents should set the relevent defaults at the beginning of a session.', + 'Set the session defaults needed by many tools. Most tools require one or more session defaults to be set before they can be used. Agents should set the relevant defaults at the beginning of a session.', ); }); diff --git a/src/mcp/tools/session-management/__tests__/session_show_defaults.test.ts b/src/mcp/tools/session-management/__tests__/session_show_defaults.test.ts index f8f45802..e4162556 100644 --- a/src/mcp/tools/session-management/__tests__/session_show_defaults.test.ts +++ b/src/mcp/tools/session-management/__tests__/session_show_defaults.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, beforeEach } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { sessionStore } from '../../../../utils/session-store.ts'; import plugin from '../session_show_defaults.ts'; @@ -7,6 +7,10 @@ describe('session-show-defaults tool', () => { sessionStore.clear(); }); + afterEach(() => { + sessionStore.clear(); + }); + describe('Export Field Validation (Literal)', () => { it('should have correct name', () => { expect(plugin.name).toBe('session-show-defaults'); diff --git a/src/mcp/tools/session-management/session_set_defaults.ts b/src/mcp/tools/session-management/session_set_defaults.ts index f386ea60..2ea0d105 100644 --- a/src/mcp/tools/session-management/session_set_defaults.ts +++ b/src/mcp/tools/session-management/session_set_defaults.ts @@ -30,7 +30,7 @@ export async function sessionSetDefaultsLogic(params: Params): Promise { const all = sessionStore.getAll(); expect(Object.keys(all).length).toBe(0); }); + + it('should be a no-op when empty keys array provided', () => { + sessionStore.setDefaults({ scheme: 'App', simulatorId: 'SIM-1' }); + sessionStore.clear([]); + const all = sessionStore.getAll(); + expect(all.scheme).toBe('App'); + expect(all.simulatorId).toBe('SIM-1'); + }); }); diff --git a/src/utils/session-store.ts b/src/utils/session-store.ts index 97d76dcd..9df96c7c 100644 --- a/src/utils/session-store.ts +++ b/src/utils/session-store.ts @@ -21,11 +21,16 @@ class SessionStore { } clear(keys?: (keyof SessionDefaults)[]): void { - if (!keys || keys.length === 0) { + if (keys == null) { this.defaults = {}; log('info', '[Session] All defaults cleared'); return; } + if (keys.length === 0) { + // No-op when an empty array is provided (e.g., empty UI selection) + log('info', '[Session] No keys provided to clear; no changes made'); + return; + } for (const k of keys) delete this.defaults[k]; log('info', `[Session] Defaults cleared: ${keys.join(', ')}`); } diff --git a/src/utils/typed-tool-factory.ts b/src/utils/typed-tool-factory.ts index 7076469a..f053332f 100644 --- a/src/utils/typed-tool-factory.ts +++ b/src/utils/typed-tool-factory.ts @@ -77,13 +77,34 @@ export function createSessionAwareTool(opts: { getExecutor: () => CommandExecutor; sessionKeys?: (keyof SessionDefaults)[]; requirements?: SessionRequirement[]; + exclusivePairs?: (keyof SessionDefaults)[][]; // when args provide one side, drop conflicting session-default side(s) }) { - const { internalSchema, logicFunction, getExecutor, requirements = [] } = opts; + const { + internalSchema, + logicFunction, + getExecutor, + requirements = [], + exclusivePairs = [], + } = opts; return async (rawArgs: Record): Promise => { try { + // Start with session defaults merged with explicit args (args override session) const merged: Record = { ...sessionStore.getAll(), ...rawArgs }; + // Apply exclusive pair pruning: if caller provided a key in a pair, remove other keys + // from that pair which came only from session defaults (not explicitly provided) + for (const pair of exclusivePairs) { + const provided = pair.filter((k) => rawArgs[k] != null); + if (provided.length > 0) { + for (const k of pair) { + if (rawArgs[k] == null && merged[k] != null) { + delete merged[k]; + } + } + } + } + for (const req of requirements) { if ('allOf' in req) { const missing = missingFromArgsAndSession(req.allOf, rawArgs); @@ -99,10 +120,13 @@ export function createSessionAwareTool(opts: { } else if ('oneOf' in req) { const satisfied = req.oneOf.some((k) => merged[k] != null); if (!satisfied) { + const options = req.oneOf.join(', '); + const setHints = req.oneOf + .map((k) => `session-set-defaults { "${k}": "..." }`) + .join(' OR '); return createErrorResponse( 'Missing required session defaults', - `${req.message ?? `Provide one of: ${req.oneOf.join(', ')}`}\n` + - `Set with: session-set-defaults { "${req.oneOf[0]}": "..." }`, + `${req.message ?? `Provide one of: ${options}`}\nSet with: ${setHints}`, ); } } From 99c77be82ed4c6146387c554b7289f1fb5f20291 Mon Sep 17 00:00:00 2001 From: Cameron Cooke Date: Sun, 5 Oct 2025 20:19:18 +0100 Subject: [PATCH 04/10] docs(session-management): clarify required local checks (format + build) and mandate quick manual CLI test before review --- docs/session_management_plan.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/docs/session_management_plan.md b/docs/session_management_plan.md index c6b26a2f..1a323044 100644 --- a/docs/session_management_plan.md +++ b/docs/session_management_plan.md @@ -354,7 +354,10 @@ import plugin, { sessionClearDefaultsLogic } from '../session_clear_defaults.ts' - Always run locally before requesting review: - `npm run typecheck` - `npm run lint` + - `npm run format:check` + - `npm run build` - `npm run test` + - Perform a quick manual CLI check (mcpli or reloaderoo) per the Manual Testing section ### Minimal Changes Policy for Tests (Enforced) @@ -374,7 +377,7 @@ import plugin, { sessionClearDefaultsLogic } from '../session_clear_defaults.ts' At the end of each numbered step above: -1. Ensure all checks pass: `typecheck`, `lint`, `test`. +1. Ensure all checks pass: `typecheck`, `lint`, `format:check`, `build`, `test`; then perform a quick manual CLI test (mcpli or reloaderoo) per the Manual Testing section. - Verify tool descriptions comply with the Tool Description Policy (concise, no session-defaults mention). 2. Stage only the files for that step. 3. Prepare a concise commit message focused on the “why”. From 1d57c0691027576e77fbe89fe4283956f635ed58 Mon Sep 17 00:00:00 2001 From: Cameron Cooke Date: Sun, 5 Oct 2025 20:24:28 +0100 Subject: [PATCH 05/10] docs(session-management): note mcpli dash/underscore tool-name normalization; hyphenated CLI samples are valid --- docs/session_management_plan.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/session_management_plan.md b/docs/session_management_plan.md index 1a323044..06da4098 100644 --- a/docs/session_management_plan.md +++ b/docs/session_management_plan.md @@ -462,7 +462,7 @@ mcpli --raw session-show-defaults -- node build/index.js ```bash # Optionally provide a scratch derived data path and a short timeout -mcpli --tool-timeout=60 --raw build_sim --derivedDataPath "/tmp/XBMCP_DD" -- node build/index.js +mcpli --tool-timeout=60 --raw build-sim --derivedDataPath "/tmp/XBMCP_DD" -- node build/index.js ``` Troubleshooting: @@ -478,6 +478,7 @@ Notes: - Public schemas for session‑aware tools intentionally omit session fields (e.g., `scheme`, `projectPath`, `simulatorName`). Provide them once via `session-set-defaults` and then call the tool with zero/minimal flags. - Use `--tool-timeout=` to cap long‑running builds during manual testing. +- mcpli CLI normalizes tool names: tools exported with underscores (e.g., `build_sim`) can be invoked with hyphens (e.g., `build-sim`). Copy/paste samples using hyphens are valid because mcpli converts underscores to dashes. ## Next Steps From 0307df881f1c9bebfb970d053a1a271ddd30e0b4 Mon Sep 17 00:00:00 2001 From: Cameron Cooke Date: Sun, 5 Oct 2025 20:31:33 +0100 Subject: [PATCH 06/10] fix(session-aware): evaluate allOf/oneOf requirements against post-prune merged payload to align with exclusivePairs pruning --- src/utils/typed-tool-factory.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/utils/typed-tool-factory.ts b/src/utils/typed-tool-factory.ts index f053332f..fe64e020 100644 --- a/src/utils/typed-tool-factory.ts +++ b/src/utils/typed-tool-factory.ts @@ -64,11 +64,11 @@ export type SessionRequirement = | { allOf: (keyof SessionDefaults)[]; message?: string } | { oneOf: (keyof SessionDefaults)[]; message?: string }; -function missingFromArgsAndSession( +function missingFromMerged( keys: (keyof SessionDefaults)[], - args: Record, + merged: Record, ): string[] { - return keys.filter((k) => args[k] == null && sessionStore.get(k) == null); + return keys.filter((k) => merged[k] == null); } export function createSessionAwareTool(opts: { @@ -107,7 +107,7 @@ export function createSessionAwareTool(opts: { for (const req of requirements) { if ('allOf' in req) { - const missing = missingFromArgsAndSession(req.allOf, rawArgs); + const missing = missingFromMerged(req.allOf, merged); if (missing.length > 0) { return createErrorResponse( 'Missing required session defaults', From e1c4629118651e58b6ab55e4a5d6763d88acd214 Mon Sep 17 00:00:00 2001 From: Cameron Cooke Date: Sun, 5 Oct 2025 20:38:25 +0100 Subject: [PATCH 07/10] fix(session-aware): trigger exclusivePairs pruning when user explicitly provides null/undefined; add tests for null/undefined activation --- .../session-aware-tool-factory.test.ts | 44 +++++++++++++++++++ src/utils/typed-tool-factory.ts | 9 ++-- 2 files changed, 49 insertions(+), 4 deletions(-) diff --git a/src/utils/__tests__/session-aware-tool-factory.test.ts b/src/utils/__tests__/session-aware-tool-factory.test.ts index 66af7491..e44668df 100644 --- a/src/utils/__tests__/session-aware-tool-factory.test.ts +++ b/src/utils/__tests__/session-aware-tool-factory.test.ts @@ -109,4 +109,48 @@ describe('createSessionAwareTool', () => { expect(result.content[0].text).toContain('Parameter validation failed'); expect(result.content[0].text).toContain('Tip: set session defaults'); }); + + it('exclusivePairs should prune conflicting session defaults when user provides null', async () => { + const handlerWithExclusive = createSessionAwareTool({ + internalSchema, + logicFunction: logic, + getExecutor: () => createMockExecutor({ success: true }), + requirements: [ + { allOf: ['scheme'], message: 'scheme is required' }, + { oneOf: ['projectPath', 'workspacePath'], message: 'Provide a project or workspace' }, + ], + exclusivePairs: [['projectPath', 'workspacePath']], + }); + + sessionStore.setDefaults({ + scheme: 'App', + projectPath: '/path/proj.xcodeproj', + }); + + const res = await handlerWithExclusive({ workspacePath: null as unknown as string }); + expect(res.isError).toBe(true); + expect(res.content[0].text).toContain('Provide a project or workspace'); + }); + + it('exclusivePairs should prune when user provides undefined (key present)', async () => { + const handlerWithExclusive = createSessionAwareTool({ + internalSchema, + logicFunction: logic, + getExecutor: () => createMockExecutor({ success: true }), + requirements: [ + { allOf: ['scheme'], message: 'scheme is required' }, + { oneOf: ['projectPath', 'workspacePath'], message: 'Provide a project or workspace' }, + ], + exclusivePairs: [['projectPath', 'workspacePath']], + }); + + sessionStore.setDefaults({ + scheme: 'App', + projectPath: '/path/proj.xcodeproj', + }); + + const res = await handlerWithExclusive({ workspacePath: undefined as unknown as string }); + expect(res.isError).toBe(true); + expect(res.content[0].text).toContain('Provide a project or workspace'); + }); }); diff --git a/src/utils/typed-tool-factory.ts b/src/utils/typed-tool-factory.ts index fe64e020..492441b0 100644 --- a/src/utils/typed-tool-factory.ts +++ b/src/utils/typed-tool-factory.ts @@ -92,11 +92,12 @@ export function createSessionAwareTool(opts: { // Start with session defaults merged with explicit args (args override session) const merged: Record = { ...sessionStore.getAll(), ...rawArgs }; - // Apply exclusive pair pruning: if caller provided a key in a pair, remove other keys - // from that pair which came only from session defaults (not explicitly provided) + // Apply exclusive pair pruning: if caller provided/touched any key in a pair (even null/undefined), + // remove other keys from that pair which came only from session defaults (not explicitly provided). + // This ensures requirements and validation reflect the effective, post-prune payload. for (const pair of exclusivePairs) { - const provided = pair.filter((k) => rawArgs[k] != null); - if (provided.length > 0) { + const userTouched = pair.some((k) => Object.prototype.hasOwnProperty.call(rawArgs, k)); + if (userTouched) { for (const k of pair) { if (rawArgs[k] == null && merged[k] != null) { delete merged[k]; From 9046aa95b8e0d236792b20fc934f435a63e33d75 Mon Sep 17 00:00:00 2001 From: Cameron Cooke Date: Sun, 5 Oct 2025 20:41:23 +0100 Subject: [PATCH 08/10] fix(session-management): session-set-defaults clears mutually exclusive counterparts before merge to prevent invalid store state; add tests --- .../__tests__/session_set_defaults.test.ts | 32 +++++++++++++++++++ .../session_set_defaults.ts | 11 +++++++ 2 files changed, 43 insertions(+) diff --git a/src/mcp/tools/session-management/__tests__/session_set_defaults.test.ts b/src/mcp/tools/session-management/__tests__/session_set_defaults.test.ts index 3ad15f85..e7a91a72 100644 --- a/src/mcp/tools/session-management/__tests__/session_set_defaults.test.ts +++ b/src/mcp/tools/session-management/__tests__/session_set_defaults.test.ts @@ -56,5 +56,37 @@ describe('session-set-defaults tool', () => { expect(result.content[0].text).toContain('Parameter validation failed'); expect(result.content[0].text).toContain('useLatestOS'); }); + + it('should clear workspacePath when projectPath is set', async () => { + sessionStore.setDefaults({ workspacePath: '/old/App.xcworkspace' }); + await sessionSetDefaultsLogic({ projectPath: '/new/App.xcodeproj' }); + const current = sessionStore.getAll(); + expect(current.projectPath).toBe('/new/App.xcodeproj'); + expect(current.workspacePath).toBeUndefined(); + }); + + it('should clear projectPath when workspacePath is set', async () => { + sessionStore.setDefaults({ projectPath: '/old/App.xcodeproj' }); + await sessionSetDefaultsLogic({ workspacePath: '/new/App.xcworkspace' }); + const current = sessionStore.getAll(); + expect(current.workspacePath).toBe('/new/App.xcworkspace'); + expect(current.projectPath).toBeUndefined(); + }); + + it('should clear simulatorName when simulatorId is set', async () => { + sessionStore.setDefaults({ simulatorName: 'iPhone 16' }); + await sessionSetDefaultsLogic({ simulatorId: 'SIM-UUID' }); + const current = sessionStore.getAll(); + expect(current.simulatorId).toBe('SIM-UUID'); + expect(current.simulatorName).toBeUndefined(); + }); + + it('should clear simulatorId when simulatorName is set', async () => { + sessionStore.setDefaults({ simulatorId: 'SIM-UUID' }); + await sessionSetDefaultsLogic({ simulatorName: 'iPhone 16' }); + const current = sessionStore.getAll(); + expect(current.simulatorName).toBe('iPhone 16'); + expect(current.simulatorId).toBeUndefined(); + }); }); }); diff --git a/src/mcp/tools/session-management/session_set_defaults.ts b/src/mcp/tools/session-management/session_set_defaults.ts index 2ea0d105..1a931739 100644 --- a/src/mcp/tools/session-management/session_set_defaults.ts +++ b/src/mcp/tools/session-management/session_set_defaults.ts @@ -19,6 +19,17 @@ const schemaObj = z.object({ type Params = z.infer; export async function sessionSetDefaultsLogic(params: Params): Promise { + // Clear mutually exclusive counterparts before merging new defaults + const toClear = new Set(); + if (Object.prototype.hasOwnProperty.call(params, 'projectPath')) toClear.add('workspacePath'); + if (Object.prototype.hasOwnProperty.call(params, 'workspacePath')) toClear.add('projectPath'); + if (Object.prototype.hasOwnProperty.call(params, 'simulatorId')) toClear.add('simulatorName'); + if (Object.prototype.hasOwnProperty.call(params, 'simulatorName')) toClear.add('simulatorId'); + + if (toClear.size > 0) { + sessionStore.clear(Array.from(toClear)); + } + sessionStore.setDefaults(params as Partial); const current = sessionStore.getAll(); return { From cb6156ae79373726cb3667d02d362b09ab87be1c Mon Sep 17 00:00:00 2001 From: Cameron Cooke Date: Sun, 5 Oct 2025 20:55:40 +0100 Subject: [PATCH 09/10] fix(session-management): reject mutually exclusive defaults in session-set-defaults via schema refine; fix exclusivePairs pruning to ignore null/undefined and not override session defaults; update tests --- .../__tests__/session_set_defaults.test.ts | 20 ++++++++++++++ .../session_set_defaults.ts | 14 ++++++++-- .../session-aware-tool-factory.test.ts | 14 +++++----- src/utils/typed-tool-factory.ts | 27 ++++++++++++------- 4 files changed, 57 insertions(+), 18 deletions(-) diff --git a/src/mcp/tools/session-management/__tests__/session_set_defaults.test.ts b/src/mcp/tools/session-management/__tests__/session_set_defaults.test.ts index e7a91a72..215d638b 100644 --- a/src/mcp/tools/session-management/__tests__/session_set_defaults.test.ts +++ b/src/mcp/tools/session-management/__tests__/session_set_defaults.test.ts @@ -88,5 +88,25 @@ describe('session-set-defaults tool', () => { expect(current.simulatorName).toBe('iPhone 16'); expect(current.simulatorId).toBeUndefined(); }); + + it('should reject when both projectPath and workspacePath are provided', async () => { + const res = await plugin.handler({ + projectPath: '/app/App.xcodeproj', + workspacePath: '/app/App.xcworkspace', + }); + expect(res.isError).toBe(true); + expect(res.content[0].text).toContain('Parameter validation failed'); + expect(res.content[0].text).toContain('projectPath and workspacePath are mutually exclusive'); + }); + + it('should reject when both simulatorId and simulatorName are provided', async () => { + const res = await plugin.handler({ + simulatorId: 'SIM-1', + simulatorName: 'iPhone 16', + }); + expect(res.isError).toBe(true); + expect(res.content[0].text).toContain('Parameter validation failed'); + expect(res.content[0].text).toContain('simulatorId and simulatorName are mutually exclusive'); + }); }); }); diff --git a/src/mcp/tools/session-management/session_set_defaults.ts b/src/mcp/tools/session-management/session_set_defaults.ts index 1a931739..5b9d6c86 100644 --- a/src/mcp/tools/session-management/session_set_defaults.ts +++ b/src/mcp/tools/session-management/session_set_defaults.ts @@ -4,7 +4,7 @@ import { createTypedTool } from '../../../utils/typed-tool-factory.ts'; import { getDefaultCommandExecutor } from '../../../utils/execution/index.ts'; import type { ToolResponse } from '../../../types/common.ts'; -const schemaObj = z.object({ +const baseSchema = z.object({ projectPath: z.string().optional(), workspacePath: z.string().optional(), scheme: z.string().optional(), @@ -16,6 +16,16 @@ const schemaObj = z.object({ arch: z.enum(['arm64', 'x86_64']).optional(), }); +const schemaObj = baseSchema + .refine((v) => !(v.projectPath && v.workspacePath), { + message: 'projectPath and workspacePath are mutually exclusive', + path: ['projectPath'], + }) + .refine((v) => !(v.simulatorId && v.simulatorName), { + message: 'simulatorId and simulatorName are mutually exclusive', + path: ['simulatorId'], + }); + type Params = z.infer; export async function sessionSetDefaultsLogic(params: Params): Promise { @@ -42,6 +52,6 @@ export default { name: 'session-set-defaults', description: 'Set the session defaults needed by many tools. Most tools require one or more session defaults to be set before they can be used. Agents should set the relevant defaults at the beginning of a session.', - schema: schemaObj.shape, + schema: baseSchema.shape, handler: createTypedTool(schemaObj, sessionSetDefaultsLogic, getDefaultCommandExecutor), }; diff --git a/src/utils/__tests__/session-aware-tool-factory.test.ts b/src/utils/__tests__/session-aware-tool-factory.test.ts index e44668df..06dc3558 100644 --- a/src/utils/__tests__/session-aware-tool-factory.test.ts +++ b/src/utils/__tests__/session-aware-tool-factory.test.ts @@ -110,7 +110,7 @@ describe('createSessionAwareTool', () => { expect(result.content[0].text).toContain('Tip: set session defaults'); }); - it('exclusivePairs should prune conflicting session defaults when user provides null', async () => { + it('exclusivePairs should NOT prune session defaults when user provides null (treat as not provided)', async () => { const handlerWithExclusive = createSessionAwareTool({ internalSchema, logicFunction: logic, @@ -125,14 +125,15 @@ describe('createSessionAwareTool', () => { sessionStore.setDefaults({ scheme: 'App', projectPath: '/path/proj.xcodeproj', + simulatorId: 'SIM-1', }); const res = await handlerWithExclusive({ workspacePath: null as unknown as string }); - expect(res.isError).toBe(true); - expect(res.content[0].text).toContain('Provide a project or workspace'); + expect(res.isError).toBe(false); + expect(res.content[0].text).toBe('OK'); }); - it('exclusivePairs should prune when user provides undefined (key present)', async () => { + it('exclusivePairs should NOT prune when user provides undefined (key present)', async () => { const handlerWithExclusive = createSessionAwareTool({ internalSchema, logicFunction: logic, @@ -147,10 +148,11 @@ describe('createSessionAwareTool', () => { sessionStore.setDefaults({ scheme: 'App', projectPath: '/path/proj.xcodeproj', + simulatorId: 'SIM-1', }); const res = await handlerWithExclusive({ workspacePath: undefined as unknown as string }); - expect(res.isError).toBe(true); - expect(res.content[0].text).toContain('Provide a project or workspace'); + expect(res.isError).toBe(false); + expect(res.content[0].text).toBe('OK'); }); }); diff --git a/src/utils/typed-tool-factory.ts b/src/utils/typed-tool-factory.ts index 492441b0..fda73c43 100644 --- a/src/utils/typed-tool-factory.ts +++ b/src/utils/typed-tool-factory.ts @@ -89,19 +89,26 @@ export function createSessionAwareTool(opts: { return async (rawArgs: Record): Promise => { try { + // Sanitize args: treat null/undefined as "not provided" so they don't override session defaults + const sanitizedArgs: Record = {}; + for (const [k, v] of Object.entries(rawArgs)) { + if (v !== null && v !== undefined) sanitizedArgs[k] = v; + } + // Start with session defaults merged with explicit args (args override session) - const merged: Record = { ...sessionStore.getAll(), ...rawArgs }; + const merged: Record = { ...sessionStore.getAll(), ...sanitizedArgs }; - // Apply exclusive pair pruning: if caller provided/touched any key in a pair (even null/undefined), - // remove other keys from that pair which came only from session defaults (not explicitly provided). - // This ensures requirements and validation reflect the effective, post-prune payload. + // Apply exclusive pair pruning: only when caller provided a concrete (non-null/undefined) value + // for any key in the pair. When activated, drop other keys in the pair coming from session defaults. for (const pair of exclusivePairs) { - const userTouched = pair.some((k) => Object.prototype.hasOwnProperty.call(rawArgs, k)); - if (userTouched) { - for (const k of pair) { - if (rawArgs[k] == null && merged[k] != null) { - delete merged[k]; - } + 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]; } } } From de72b237be608fe064fe2df7d9e7cc64d95fdd3e Mon Sep 17 00:00:00 2001 From: Cameron Cooke Date: Sun, 5 Oct 2025 21:04:55 +0100 Subject: [PATCH 10/10] fix(session-aware): reject multiple explicit args in exclusivePairs at factory level; add test; update build_sim tests to expect factory message --- .../simulator/__tests__/build_sim.test.ts | 12 +++---- .../session-aware-tool-factory.test.ts | 32 +++++++++++++++++++ src/utils/typed-tool-factory.ts | 14 ++++++++ 3 files changed, 52 insertions(+), 6 deletions(-) diff --git a/src/mcp/tools/simulator/__tests__/build_sim.test.ts b/src/mcp/tools/simulator/__tests__/build_sim.test.ts index 969c6099..d8f1ece4 100644 --- a/src/mcp/tools/simulator/__tests__/build_sim.test.ts +++ b/src/mcp/tools/simulator/__tests__/build_sim.test.ts @@ -68,9 +68,9 @@ describe('build_sim tool', () => { expect(result.isError).toBe(true); expect(result.content[0].text).toContain('Parameter validation failed'); - expect(result.content[0].text).toContain( - 'projectPath and workspacePath are mutually exclusive', - ); + expect(result.content[0].text).toContain('Mutually exclusive parameters provided'); + expect(result.content[0].text).toContain('projectPath'); + expect(result.content[0].text).toContain('workspacePath'); }); it('should handle empty workspacePath parameter', async () => { @@ -158,9 +158,9 @@ describe('build_sim tool', () => { expect(result.isError).toBe(true); expect(result.content[0].text).toContain('Parameter validation failed'); - expect(result.content[0].text).toContain( - 'simulatorId and simulatorName are mutually exclusive', - ); + expect(result.content[0].text).toContain('Mutually exclusive parameters provided'); + expect(result.content[0].text).toContain('simulatorId'); + expect(result.content[0].text).toContain('simulatorName'); }); it('should handle empty simulatorName parameter', async () => { diff --git a/src/utils/__tests__/session-aware-tool-factory.test.ts b/src/utils/__tests__/session-aware-tool-factory.test.ts index 06dc3558..92911b74 100644 --- a/src/utils/__tests__/session-aware-tool-factory.test.ts +++ b/src/utils/__tests__/session-aware-tool-factory.test.ts @@ -155,4 +155,36 @@ describe('createSessionAwareTool', () => { expect(res.isError).toBe(false); expect(res.content[0].text).toBe('OK'); }); + + it('rejects when multiple explicit args in an exclusive pair are provided (factory-level)', async () => { + const internalSchemaNoXor = z.object({ + scheme: z.string(), + projectPath: z.string().optional(), + workspacePath: z.string().optional(), + }); + + const handlerNoXor = createSessionAwareTool>({ + internalSchema: internalSchemaNoXor, + logicFunction: (async () => ({ + content: [{ type: 'text', text: 'OK' }], + isError: false, + })) as any, + getExecutor: () => createMockExecutor({ success: true }), + requirements: [{ allOf: ['scheme'], message: 'scheme is required' }], + exclusivePairs: [['projectPath', 'workspacePath']], + }); + + const res = await handlerNoXor({ + scheme: 'App', + projectPath: '/path/a.xcodeproj', + workspacePath: '/path/b.xcworkspace', + }); + + expect(res.isError).toBe(true); + const msg = res.content[0].text; + expect(msg).toContain('Parameter validation failed'); + expect(msg).toContain('Mutually exclusive parameters provided'); + expect(msg).toContain('projectPath'); + expect(msg).toContain('workspacePath'); + }); }); diff --git a/src/utils/typed-tool-factory.ts b/src/utils/typed-tool-factory.ts index fda73c43..86a68008 100644 --- a/src/utils/typed-tool-factory.ts +++ b/src/utils/typed-tool-factory.ts @@ -95,6 +95,20 @@ export function createSessionAwareTool(opts: { if (v !== null && v !== undefined) sanitizedArgs[k] = v; } + // Factory-level mutual exclusivity check: if user provides multiple explicit values + // within an exclusive group, reject early even if tool schema doesn't enforce XOR. + for (const pair of exclusivePairs) { + const provided = pair.filter((k) => Object.prototype.hasOwnProperty.call(sanitizedArgs, k)); + if (provided.length >= 2) { + return createErrorResponse( + 'Parameter validation failed', + `Invalid parameters:\nMutually exclusive parameters provided: ${provided.join( + ', ', + )}. Provide only one.`, + ); + } + } + // Start with session defaults merged with explicit args (args override session) const merged: Record = { ...sessionStore.getAll(), ...sanitizedArgs };