feat(setup): Add env var session defaults and --format mcp-json#272
Open
cameroncooke wants to merge 4 commits intomainfrom
Open
feat(setup): Add env var session defaults and --format mcp-json#272cameroncooke wants to merge 4 commits intomainfrom
cameroncooke wants to merge 4 commits intomainfrom
Conversation
- Add readEnvSessionDefaults() to parse all 15 session default env vars (XCODEBUILDMCP_WORKSPACE_PATH, XCODEBUILDMCP_SCHEME, etc.) - Wire env layer into resolveSessionDefaults() with precedence: tool overrides > config file > env vars - Add --format mcp-json flag to xcodebuildmcp setup: outputs a ready-to-paste MCP client config JSON block instead of writing config.yaml - Update CONFIGURATION.md: env vars documented as recommended method for MCP client integration, remove 'legacy' label, add layering section - Add tests for env var parsing and file-over-env precedence - Add test for --format mcp-json output Closes #267
Collect simulator and device defaults based on the workflows a user selects instead of tying setup questions to the output format. This keeps yaml and mcp-json setup aligned and lets users skip pinning a default target when they do not want one. Reframe the docs so config.yaml remains the canonical config surface and env vars are described as bootstrap values for constrained MCP clients.
commit: |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: Direct
fsimport bypasses injectedFileSystemExecutor- Updated listAvailableDevices to accept FileSystemExecutor parameter and use its methods (mkdtemp, tmpdir, readFile, rm) instead of importing fs directly from node:fs.
- ✅ Fixed: Env session defaults lost after session patch refresh
- Added env field to ConfigStoreState, stored custom env in initConfigStore, and passed it through refreshResolvedSessionFields to preserve custom environment across session defaults updates.
- ✅ Fixed: Test uses real filesystem for device list mock
- Removed real fs import from test file and updated all tests to write device list data through the mock FileSystemExecutor instead of using real filesystem operations.
Or push these changes by commenting:
@cursor push ec1a1e50e2
Preview (ec1a1e50e2)
diff --git a/src/cli/commands/__tests__/setup.test.ts b/src/cli/commands/__tests__/setup.test.ts
--- a/src/cli/commands/__tests__/setup.test.ts
+++ b/src/cli/commands/__tests__/setup.test.ts
@@ -1,5 +1,4 @@
import { afterEach, beforeEach, describe, expect, it } from 'vitest';
-import { promises as fs } from 'node:fs';
import path from 'node:path';
import { parse as parseYaml } from 'yaml';
import {
@@ -7,35 +6,31 @@
createMockFileSystemExecutor,
} from '../../../test-utils/mock-executors.ts';
import type { CommandExecutor } from '../../../utils/CommandExecutor.ts';
+import type { FileSystemExecutor } from '../../../utils/FileSystemExecutor.ts';
import type { Prompter } from '../../interactive/prompts.ts';
import { runSetupWizard } from '../setup.ts';
const cwd = '/repo';
const configPath = path.join(cwd, '.xcodebuildmcp', 'config.yaml');
-async function writeMockDeviceList(jsonPath: string): Promise<void> {
- await fs.writeFile(
- jsonPath,
- JSON.stringify({
- result: {
- devices: [
- {
- identifier: 'DEVICE-1',
- visibilityClass: 'Default',
- connectionProperties: {
- pairingState: 'paired',
- tunnelState: 'connected',
- },
- deviceProperties: {
- name: 'Cam iPhone',
- platformIdentifier: 'com.apple.platform.iphoneos',
- },
- },
- ],
+const mockDeviceListJson = JSON.stringify({
+ result: {
+ devices: [
+ {
+ identifier: 'DEVICE-1',
+ visibilityClass: 'Default',
+ connectionProperties: {
+ pairingState: 'paired',
+ tunnelState: 'connected',
+ },
+ deviceProperties: {
+ name: 'Cam iPhone',
+ platformIdentifier: 'com.apple.platform.iphoneos',
+ },
},
- }),
- );
-}
+ ],
+ },
+});
function createTestPrompter(): Prompter {
return {
@@ -72,6 +67,7 @@
it('exports a setup wizard that writes config selections', async () => {
let storedConfig = '';
+ const deviceListFiles = new Map<string, string>();
const fs = createMockFileSystemExecutor({
existsSync: (targetPath) => targetPath === configPath && storedConfig.length > 0,
@@ -90,22 +86,31 @@
return [];
},
readFile: async (targetPath) => {
- if (targetPath !== configPath) {
- throw new Error(`Unexpected read path: ${targetPath}`);
+ if (targetPath === configPath) {
+ return storedConfig;
}
- return storedConfig;
+ if (deviceListFiles.has(targetPath)) {
+ return deviceListFiles.get(targetPath)!;
+ }
+ throw new Error(`Unexpected read path: ${targetPath}`);
},
writeFile: async (targetPath, content) => {
- if (targetPath !== configPath) {
- throw new Error(`Unexpected write path: ${targetPath}`);
+ if (targetPath === configPath) {
+ storedConfig = content;
+ return;
}
- storedConfig = content;
+ deviceListFiles.set(targetPath, content);
},
+ mkdtemp: async (prefix: string) => {
+ return `${prefix}123456`;
+ },
+ tmpdir: () => '/tmp',
+ rm: async () => {},
});
const executor: CommandExecutor = async (command) => {
if (command[0] === 'xcrun' && command[1] === 'devicectl') {
- await writeMockDeviceList(command[5]);
+ await fs.writeFile(command[5], mockDeviceListJson);
return createMockCommandResponse({
success: true,
output: '',
@@ -172,6 +177,7 @@
it('shows debug-gated workflows when existing config enables debug', async () => {
let storedConfig = 'schemaVersion: 1\ndebug: true\n';
let offeredWorkflowIds: string[] = [];
+ const deviceListFiles = new Map<string, string>();
const fs = createMockFileSystemExecutor({
existsSync: (targetPath) => targetPath === configPath && storedConfig.length > 0,
@@ -190,22 +196,31 @@
return [];
},
readFile: async (targetPath) => {
- if (targetPath !== configPath) {
- throw new Error(`Unexpected read path: ${targetPath}`);
+ if (targetPath === configPath) {
+ return storedConfig;
}
- return storedConfig;
+ if (deviceListFiles.has(targetPath)) {
+ return deviceListFiles.get(targetPath)!;
+ }
+ throw new Error(`Unexpected read path: ${targetPath}`);
},
writeFile: async (targetPath, content) => {
- if (targetPath !== configPath) {
- throw new Error(`Unexpected write path: ${targetPath}`);
+ if (targetPath === configPath) {
+ storedConfig = content;
+ return;
}
- storedConfig = content;
+ deviceListFiles.set(targetPath, content);
},
+ mkdtemp: async (prefix: string) => {
+ return `${prefix}123456`;
+ },
+ tmpdir: () => '/tmp',
+ rm: async () => {},
});
const executor: CommandExecutor = async (command) => {
if (command[0] === 'xcrun' && command[1] === 'devicectl') {
- await writeMockDeviceList(command[5]);
+ await fs.writeFile(command[5], mockDeviceListJson);
return createMockCommandResponse({
success: true,
output: '',
@@ -297,6 +312,8 @@
});
it('outputs MCP config JSON when format is mcp-json', async () => {
+ const deviceListFiles = new Map<string, string>();
+
const fs = createMockFileSystemExecutor({
existsSync: () => false,
stat: async () => ({ isDirectory: () => true, mtimeMs: 0 }),
@@ -313,13 +330,25 @@
return [];
},
- readFile: async () => '',
- writeFile: async () => {},
+ readFile: async (targetPath) => {
+ if (deviceListFiles.has(targetPath)) {
+ return deviceListFiles.get(targetPath)!;
+ }
+ return '';
+ },
+ writeFile: async (targetPath, content) => {
+ deviceListFiles.set(targetPath, content);
+ },
+ mkdtemp: async (prefix: string) => {
+ return `${prefix}123456`;
+ },
+ tmpdir: () => '/tmp',
+ rm: async () => {},
});
const executor: CommandExecutor = async (command) => {
if (command[0] === 'xcrun' && command[1] === 'devicectl') {
- await writeMockDeviceList(command[5]);
+ await fs.writeFile(command[5], mockDeviceListJson);
return createMockCommandResponse({
success: true,
output: '',
@@ -474,6 +503,8 @@
});
it('collects a device default without requiring simulator selection when only device-dependent workflows are enabled', async () => {
+ const deviceListFiles = new Map<string, string>();
+
const fs = createMockFileSystemExecutor({
existsSync: () => false,
stat: async () => ({ isDirectory: () => true, mtimeMs: 0 }),
@@ -490,8 +521,20 @@
return [];
},
- readFile: async () => '',
- writeFile: async () => {},
+ readFile: async (targetPath) => {
+ if (deviceListFiles.has(targetPath)) {
+ return deviceListFiles.get(targetPath)!;
+ }
+ return '';
+ },
+ writeFile: async (targetPath, content) => {
+ deviceListFiles.set(targetPath, content);
+ },
+ mkdtemp: async (prefix: string) => {
+ return `${prefix}123456`;
+ },
+ tmpdir: () => '/tmp',
+ rm: async () => {},
});
const executor: CommandExecutor = async (command) => {
@@ -500,7 +543,7 @@
}
if (command[0] === 'xcrun' && command[1] === 'devicectl') {
- await writeMockDeviceList(command[5]);
+ await fs.writeFile(command[5], mockDeviceListJson);
return createMockCommandResponse({
success: true,
output: '',
diff --git a/src/cli/commands/setup.ts b/src/cli/commands/setup.ts
--- a/src/cli/commands/setup.ts
+++ b/src/cli/commands/setup.ts
@@ -1,6 +1,4 @@
import type { Argv } from 'yargs';
-import { promises as fs } from 'node:fs';
-import { tmpdir } from 'node:os';
import path from 'node:path';
import * as clack from '@clack/prompts';
import { getDefaultCommandExecutor, getDefaultFileSystemExecutor } from '../../utils/command.ts';
@@ -543,8 +541,12 @@
return listed;
}
-async function listAvailableDevices(executor: CommandExecutor): Promise<SetupDevice[]> {
- const jsonPath = path.join(tmpdir(), `xcodebuildmcp-setup-devices-${Date.now()}.json`);
+async function listAvailableDevices(
+ executor: CommandExecutor,
+ fs: FileSystemExecutor,
+): Promise<SetupDevice[]> {
+ const tmpDir = await fs.mkdtemp(path.join(fs.tmpdir(), 'xcodebuildmcp-setup-devices-'));
+ const jsonPath = path.join(tmpDir, 'devices.json');
try {
const result = await executor(
@@ -564,7 +566,7 @@
} catch {
// Fall back to xctrace below.
} finally {
- await fs.unlink(jsonPath).catch(() => {});
+ await fs.rm(tmpDir, { recursive: true, force: true }).catch(() => {});
}
const fallbackResult = await executor(
@@ -595,6 +597,7 @@
async function selectDevice(opts: {
existingDeviceId?: string;
executor: CommandExecutor;
+ fs: FileSystemExecutor;
prompter: Prompter;
isTTY: boolean;
quietOutput: boolean;
@@ -604,7 +607,7 @@
quietOutput: opts.quietOutput,
startMessage: 'Loading devices...',
stopMessage: 'Devices loaded.',
- task: () => listAvailableDevices(opts.executor),
+ task: () => listAvailableDevices(opts.executor, opts.fs),
});
if (devices.length === 0) {
@@ -725,6 +728,7 @@
? await selectDevice({
existingDeviceId: existing.deviceId,
executor: deps.executor,
+ fs: deps.fs,
prompter: deps.prompter,
isTTY,
quietOutput: deps.quietOutput,
diff --git a/src/utils/config-store.ts b/src/utils/config-store.ts
--- a/src/utils/config-store.ts
+++ b/src/utils/config-store.ts
@@ -65,6 +65,7 @@
fs?: FileSystemExecutor;
overrides?: RuntimeConfigOverrides;
fileConfig?: ProjectConfig;
+ env?: NodeJS.ProcessEnv;
resolved: ResolvedRuntimeConfig;
};
@@ -365,6 +366,7 @@
storeState.resolved.sessionDefaults = resolveSessionDefaults({
overrides: storeState.overrides,
fileConfig: storeState.fileConfig,
+ env: storeState.env,
});
storeState.resolved.sessionDefaultsProfiles = resolveSessionDefaultsProfiles({
overrides: storeState.overrides,
@@ -576,6 +578,7 @@
storeState.cwd = opts.cwd;
storeState.fs = opts.fs;
storeState.overrides = opts.overrides;
+ storeState.env = opts.env;
let fileConfig: ProjectConfig | undefined;
let found = false;
@@ -676,5 +679,6 @@
storeState.fs = undefined;
storeState.overrides = undefined;
storeState.fileConfig = undefined;
+ storeState.env = undefined;
storeState.resolved = { ...DEFAULT_CONFIG };
}This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Continue setup when simulator or device discovery returns no candidates instead of aborting. This keeps the explicit no-default path usable and prevents setup from failing on machines that are missing optional runtime targets. Route setup device discovery temp-file handling through FileSystemExecutor and preserve injected env-backed session defaults after config-store refreshes so tests and runtime state stay consistent.
| } | ||
|
|
||
| return 'Unknown'; | ||
| } |
Contributor
There was a problem hiding this comment.
Duplicated platform label function across two files
Low Severity
getDevicePlatformLabel in setup.ts is an exact duplicate of getPlatformLabel in src/mcp/tools/device/list_devices.ts. Both have identical logic mapping platform identifiers to human-readable labels. If the mapping ever needs updating (e.g., a new Apple platform), only one copy may get fixed, leading to inconsistencies.
Collaborator
Author
There was a problem hiding this comment.
I agree the duplication is real, but I’m treating it as refactor work rather than a correctness bug in this PR. The current mapping in setup.ts matches src/mcp/tools/device/list_devices.ts, and extracting a shared helper would be scope expansion for this review-fix pass, so I’m leaving this thread unresolved for a separate cleanup if we want it.
Return an empty device list when both devicectl and xctrace discovery fail so setup can continue with no default device instead of aborting. Add regression coverage for the double-failure discovery path.
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| return []; | ||
| } | ||
|
|
||
| return parseXctraceDevices(fallbackResult.output); |
Contributor
There was a problem hiding this comment.
Unhandled xctrace throw crashes setup wizard
Medium Severity
The xctrace fallback in listAvailableDevices is not wrapped in a try/catch, unlike the devicectl path above it. If the executor rejects (e.g., spawn error when xcrun xctrace can't start), the exception propagates up and crashes the setup wizard. This contradicts the stated design that "device discovery degrades to an empty list so the wizard can continue." The default executor in command.ts confirms it can reject(err) on spawn errors via childProcess.on('error', ...).
Additional Locations (1)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add environment variable support for all 15 session default values (e.g.
XCODEBUILDMCP_WORKSPACE_PATH,XCODEBUILDMCP_SCHEME) so MCP clients that cannot write config files can bootstrap defaults through their env config block.Wire the env layer into
resolveSessionDefaults()with precedence: tool overrides > config file > env vars.Add
--format mcp-jsontoxcodebuildmcp setupwhich outputs a ready-to-paste MCP client JSON config block instead of writingconfig.yaml. The setup wizard now collects simulator/device defaults based on selected workflows rather than output format, keeping yaml and mcp-json paths aligned.Update CONFIGURATION.md to document env vars as the recommended approach for MCP client integration and describe the layering model.
Originally contributed by ichoosetoaccept
Closes #267