fix(simctl): Require names for cloned simulators

Require clone_sims callers to provide the new simulator name that simctl expects. Avoid reporting the source simulator as a clone artifact when the clone command fails.

Co-Authored-By: OpenAI Codex <noreply@openai.com>

Author: cameroncooke

src/mcp/tools/simulator-management/__tests__/clone_sims.test.ts

@@ -1,5 +1,5 @@
 import { describe, it, expect } from 'vitest';
-import { schema, clone_simsLogic } from '../clone_sims.ts';
+import { schema, clone_simsLogic, createCloneSimsExecutor } from '../clone_sims.ts';
 import { createMockExecutor } from '../../../../test-utils/mock-executors.ts';
 import { allText, runLogic } from '../../../../test-utils/test-helpers.ts';
 
@@ -14,16 +14,6 @@ describe('clone_sims tool', () => {
     it('clones a simulator and captures the new UDID', async () => {
       const newUdid = '00000000-0000-0000-0000-000000000001';
       const mock = createMockExecutor({ success: true, output: `${newUdid}\n` });
-      const res = await runLogic(() =>
-        clone_simsLogic({ sourceSimulatorId: '00000000-0000-0000-0000-000000000000' }, mock),
-      );
-      expect(res.isError).toBeFalsy();
-      const text = allText(res);
-      expect(text).toContain('Simulator cloned successfully');
-    });
-
-    it('clones with a custom name', async () => {
-      const mock = createMockExecutor({ success: true, output: 'UUID1\n' });
       const res = await runLogic(() =>
         clone_simsLogic(
           {
@@ -34,19 +24,60 @@ describe('clone_sims tool', () => {
         ),
       );
       expect(res.isError).toBeFalsy();
+      const text = allText(res);
+      expect(text).toContain('Simulator cloned successfully');
+    });
+
+    it('passes the required custom name to simctl clone', async () => {
+      const calls: string[][] = [];
+      const mock = createMockExecutor({
+        success: true,
+        output: 'UUID1\n',
+        onExecute: (command) => calls.push(command),
+      });
+
+      const executeCloneSims = createCloneSimsExecutor(mock);
+      const result = await executeCloneSims({
+        sourceSimulatorId: '00000000-0000-0000-0000-000000000000',
+        newName: 'My Clone',
+      });
+
+      expect(result.didError).toBe(false);
+      expect(calls).toEqual([
+        ['xcrun', 'simctl', 'clone', '00000000-0000-0000-0000-000000000000', 'My Clone'],
+      ]);
     });
   });
 
   describe('Failure path', () => {
     it('returns failure when clone fails', async () => {
       const mock = createMockExecutor({ success: false, error: 'No such device' });
       const res = await runLogic(() =>
-        clone_simsLogic({ sourceSimulatorId: '00000000-0000-0000-0000-000000000000' }, mock),
+        clone_simsLogic(
+          {
+            sourceSimulatorId: '00000000-0000-0000-0000-000000000000',
+            newName: 'My Clone',
+          },
+          mock,
+        ),
       );
       expect(res.isError).toBe(true);
       const text = allText(res);
       expect(text).toContain('Clone simulator failed');
       expect(text).toContain('No such device');
     });
+
+    it('omits artifacts when clone fails before producing a cloned simulator ID', async () => {
+      const mock = createMockExecutor({ success: false, error: 'No such device' });
+      const executeCloneSims = createCloneSimsExecutor(mock);
+
+      const result = await executeCloneSims({
+        sourceSimulatorId: '00000000-0000-0000-0000-000000000000',
+        newName: 'My Clone',
+      });
+
+      expect(result.didError).toBe(true);
+      expect(result.artifacts).toBeUndefined();
+    });
   });
 });

src/mcp/tools/simulator-management/clone_sims.ts

@@ -15,16 +15,13 @@ import { toErrorMessage } from '../../../utils/errors.ts';
 import { createBasicDiagnostics } from '../../../utils/diagnostics.ts';
 
 const baseSchemaObject = z.object({
-  sourceSimulatorId: z.string().uuid().describe('UDID of the simulator to clone'),
-  newName: z
-    .string()
-    .optional()
-    .describe('Name for the cloned simulator. If omitted, simctl auto-generates one.'),
+  sourceSimulatorId: z.uuid().describe('UDID of the simulator to clone'),
+  newName: z.string().min(1).describe('Name for the cloned simulator.'),
 });
 
 const internalSchemaObject = z.object({
-  sourceSimulatorId: z.string().uuid(),
-  newName: z.string().optional(),
+  sourceSimulatorId: z.uuid(),
+  newName: z.string().min(1),
 });
 
 type CloneSimsParams = z.infer<typeof internalSchemaObject>;
@@ -51,9 +48,7 @@ function createCloneSimsResult(params: {
     ...(params.diagnosticMessage
       ? { diagnostics: createBasicDiagnostics({ errors: [params.diagnosticMessage] }) }
       : {}),
-    artifacts: {
-      simulatorId: params.clonedSimulatorId ?? params.sourceSimulatorId,
-    },
+    ...(params.clonedSimulatorId ? { artifacts: { simulatorId: params.clonedSimulatorId } } : {}),
   };
 }
 
@@ -70,12 +65,11 @@ export function createCloneSimsExecutor(
 ): NonStreamingExecutor<CloneSimsParams, CloneSimsResult> {
   return async (params) => {
     try {
-      const command = ['xcrun', 'simctl', 'clone', params.sourceSimulatorId];
-      if (params.newName) {
-        command.push(params.newName);
-      }
-
-      const result = await executor(command, 'Clone Simulator', false);
+      const result = await executor(
+        ['xcrun', 'simctl', 'clone', params.sourceSimulatorId, params.newName],
+        'Clone Simulator',
+        false,
+      );
 
       if (!result.success) {
         const diagnosticMessage = result.error ?? 'Unknown error';
@@ -109,10 +103,7 @@ export async function clone_simsLogic(
   params: CloneSimsParams,
   executor: CommandExecutor,
 ): Promise<void> {
-  log(
-    'info',
-    `Cloning simulator ${params.sourceSimulatorId}${params.newName ? ` as "${params.newName}"` : ''}`,
-  );
+  log('info', `Cloning simulator ${params.sourceSimulatorId} as "${params.newName}"`);
 
   const ctx = getHandlerContext();
   const executeCloneSims = createCloneSimsExecutor(executor);