feat: add TEST_RUNNER_ environment variable support for test tools #102
Conversation
Implements explicit testRunnerEnv parameter for all test tools (test_device,
test_macos, test_sim) to support passing environment variables to xcodebuild
test commands with automatic TEST_RUNNER_ prefix handling.
## Key Features
- Added testRunnerEnv parameter to all three test tools
- Automatic TEST_RUNNER_ prefix normalization for environment variables
- Updated CommandExecutor infrastructure to support execution options
- Enhanced executeXcodeBuildCommand to accept environment variables
- Updated handleTestLogic for simulator test integration
## Security & Architecture Benefits
- Explicit parameter approach prevents environment variable injection attacks
- Follows principle of least privilege - only specified variables are passed
- Maintains clear separation between MCP server config and tool execution
- Provides audit trail of exactly which variables are used per test
## Implementation Details
- normalizeTestRunnerEnv() utility automatically adds TEST_RUNNER_ prefix
- CommandExecOptions interface supports env and cwd execution options
- All tools validate parameters with Zod schemas
- Comprehensive test coverage with 26 new tests (18 unit + 8 integration)
## Usage Example
```typescript
test_device({
projectPath: "/path/to/project.xcodeproj",
scheme: "MyScheme",
deviceId: "device-uuid",
testRunnerEnv: { "USE_DEV_MODE": "YES" } // Becomes TEST_RUNNER_USE_DEV_MODE
})
```
Addresses GitHub issue #101 with a secure, explicit approach that prevents
injection vulnerabilities while providing the requested functionality.
Tested via Reloaderoo with real xcodebuild execution validation.
WalkthroughAdds extensive docs, introduces TEST_RUNNER_ environment passthrough via a new testRunnerEnv parameter, updates execution APIs (CommandExecutor with CommandExecOptions), threads execOpts through executeXcodeBuildCommand, and adds normalization utilities and tests. Updates iOS example project by adding a UI test target, scheme test entry, and a Swift UITest verifying env passthrough. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Tool as MCP Tool (device/macos/sim)
participant TestLogic as handleTestLogic
participant Env as normalizeTestRunnerEnv
participant Build as executeXcodeBuildCommand
participant Exec as CommandExecutor
participant XCB as xcodebuild
User->>Tool: call test_* with testRunnerEnv?
Tool->>TestLogic: forward params (incl. testRunnerEnv)
alt testRunnerEnv provided
TestLogic->>Env: normalize(userVars)
Env-->>TestLogic: TEST_RUNNER_* map
TestLogic->>Build: execOpts = { env: TEST_RUNNER_* }
else no env
TestLogic->>Build: execOpts = undefined
end
Build->>Exec: run(args, opts=execOpts)
Exec->>XCB: spawn with env merged (process.env + opts.env)
XCB-->>Exec: test results / xcresult
Exec-->>Build: CommandResponse
Build-->>TestLogic: path to xcresult
TestLogic-->>Tool: parsed test outcome
Tool-->>User: result
note over Env,Exec: New/changed: env normalization + opts passthrough
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Claude finished @cameroncooke's task —— View job PR Review: TEST_RUNNER_ Environment Variable Support
|
|
Fixes #101 |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/command.ts (1)
47-50: Shell injection risk: unescaped $ and backticks allow command substitution inside double quotesWhen useShell is true, arguments containing $(...) or backticks will still be expanded by the shell even if wrapped in double quotes. Current escaping only handles " and , leaving $ and ` unescaped. A malicious arg like 'foo$(uname -a)' would execute uname.
Apply this minimal hardening to also escape $ and ` inside quoted args:
- return `"${arg.replace(/(["\\])/g, '\\$1')}"`; + // Also escape $ and backticks to prevent variable/command substitution within double quotes + return `"${arg.replace(/(["\\$`])/g, '\\$1')}"`;Optionally, prefer avoiding a shell entirely (more secure and simpler) when not needed:
- useShell: boolean = true, + useShell: boolean = false,If changing the default is too risky now, consider toggling useShell=false at call sites that don’t require shell features (pipes, redirects).
I can add unit tests demonstrating safe handling of args like 'a$(echo hacked)' and '
uname' under both modes.
🧹 Nitpick comments (28)
src/utils/CommandExecutor.ts (2)
3-6: Clarify semantics for env merging and cwd via TSDocDocument whether env replaces or merges with process.env, and whether cwd must be absolute. This helps implementers keep behavior consistent.
Apply this diff to add inline docs:
export interface CommandExecOptions { - env?: Record<string, string>; - cwd?: string; + /** + * Environment variables for the spawned process. + * Note: Values must be strings. Callers should pre-normalize (e.g., drop null/undefined). + * Recommendation: decide at call site whether to merge with process.env or pass a minimal env. + */ + env?: Record<string, string>; + /** + * Working directory for the spawned process. + * Prefer an absolute path to avoid ambiguity. + */ + cwd?: string; }
11-17: Signature change may break external implementers; consider transitional overloadsSwitching from the 4th param env?: Record<string,string> to opts?: CommandExecOptions is a breaking type change for any out-of-repo implementers. Provide an overload that accepts the legacy env position to ease migration, while steering new code to opts.
Apply this diff to introduce backward-compatible overloads:
-export type CommandExecutor = ( - command: string[], - logPrefix?: string, - useShell?: boolean, - opts?: CommandExecOptions, - detached?: boolean, -) => Promise<CommandResponse>; +export interface CommandExecutor { + /** + * Preferred signature using CommandExecOptions. + */ + ( + command: string[], + logPrefix?: string, + useShell?: boolean, + opts?: CommandExecOptions, + detached?: boolean, + ): Promise<CommandResponse>; + /** + * Deprecated legacy signature for migration: env as the 4th param. + * Implementations should map `env` to `{ env }`. + */ + ( + command: string[], + logPrefix?: string, + useShell?: boolean, + env?: Record<string, string>, + detached?: boolean, + ): Promise<CommandResponse>; +}Optionally, annotate the legacy signature with a @deprecated tag in a TSDoc block if you prefer stricter guidance.
example_projects/iOS/MCPTestUITests/MCPTestUITests.swift (2)
11-14: Doc comment is now outdated relative to this PRThe comment says “before implementation in Node layer,” but this PR implements the passthrough. Update to reflect the new, expected behavior.
Apply this diff:
-/// Current behavior (before implementation in Node layer): -/// - Running via XcodeBuildMCP test tools does not yet pass TEST_RUNNER_ -/// variables through, so this test will fail and serve as a repro. +/// Note: This repository now supports passing TEST_RUNNER_ variables. +/// This test will only pass when invoked with TEST_RUNNER_USE_DEV_MODE=YES +/// (xcodebuild strips the prefix and the runner sees USE_DEV_MODE=YES).
20-30: Make the repro test self-skipping when not configuredAs written, the test will fail when run without TEST_RUNNER_USE_DEV_MODE=YES, which is brittle for casual runs. Skip unless the runner actually receives USE_DEV_MODE.
Apply this diff:
func testEnvironmentVariablePassthrough() throws { - let env = ProcessInfo.processInfo.environment - let value = env["USE_DEV_MODE"] ?? "<nil>" + let env = ProcessInfo.processInfo.environment + guard let value = env["USE_DEV_MODE"] else { + throw XCTSkip("USE_DEV_MODE not set. Provide TEST_RUNNER_USE_DEV_MODE=YES to enable this repro.") + } XCTAssertEqual( - value, + value, "YES", - "Expected USE_DEV_MODE=YES via TEST_RUNNER_USE_DEV_MODE. Actual: \(value)" + "Expected USE_DEV_MODE=YES via TEST_RUNNER_USE_DEV_MODE. Actual: \(value)" ) }docs/RELOADEROO_FOR_XCODEBUILDMCP.md (5)
5-5: Tighten wording and fix “MCP server” phrasingMinor clarity improvement.
Apply this diff:
-You can use this guide to prompt your agent, but providing the entire document will give you no actual benefits. You will end up using more context than just using MCP server directly. So it's recommended that you curate this document by removing the example commands that you don't need and just keeping the ones that are right for your project. You'll then want to keep this file within your project workspace and then include it in the context window when you need to interact your agent to use XcodeBuildMCP tools. +You can use this guide to prompt your agent, but providing the entire document will give you no benefits. You will end up using more context than using the MCP server directly. It's recommended that you curate this document by removing unneeded examples and keeping only those relevant to your project. Keep the curated file in your workspace and include it in the context window when you need to use XcodeBuildMCP tools.
7-8: Fix typo and punctuation in the IMPORTANT note“derrived” → “derived”, and smoother phrasing.
Apply this diff:
-> [!IMPORTANT] -> Please remove this introduction before you prompt your agent with this file or any derrived version of it. +> [!IMPORTANT] +> Please remove this introduction before you prompt your agent with this file or any derived version of it.
55-57: Showcase new testRunnerEnv support in test_device exampleAdd a concrete example of passing a TEST_RUNNER_ variable via the new parameter.
Apply this diff:
- npx reloaderoo@latest inspect call-tool test_device --params '{"projectPath": "/path/to/MyProject.xcodeproj", "scheme": "MyScheme", "deviceId": "DEVICE_UDID"}' -- node build/index.js + npx reloaderoo@latest inspect call-tool test_device --params '{"projectPath": "/path/to/MyProject.xcodeproj", "scheme": "MyScheme", "deviceId": "DEVICE_UDID", "testRunnerEnv": {"USE_DEV_MODE": "YES"}}' -- node build/index.js
101-104: Document testRunnerEnv for simulator testingReflect the new feature in the simulator example as well.
Apply this diff:
- npx reloaderoo@latest inspect call-tool test_sim --params '{"projectPath": "/path/to/MyProject.xcodeproj", "scheme": "MyScheme", "simulatorName": "iPhone 16"}' -- node build/index.js + npx reloaderoo@latest inspect call-tool test_sim --params '{"projectPath": "/path/to/MyProject.xcodeproj", "scheme": "MyScheme", "simulatorName": "iPhone 16", "testRunnerEnv": {"USE_DEV_MODE": "YES"}}' -- node build/index.js
149-150: Include testRunnerEnv in macOS test exampleHelps users understand the macOS flow as well.
Apply this diff:
- npx reloaderoo@latest inspect call-tool test_macos --params '{"projectPath": "/path/to/MyProject.xcodeproj", "scheme": "MyScheme"}' -- node build/index.js + npx reloaderoo@latest inspect call-tool test_macos --params '{"projectPath": "/path/to/MyProject.xcodeproj", "scheme": "MyScheme", "testRunnerEnv": {"USE_DEV_MODE": "YES"}}' -- node build/index.jsexample_projects/iOS/MCPTest.xcodeproj/project.pbxproj (1)
128-129: Suspicious Xcode metadata: LastSwiftUpdateCheck/CreatedOnToolsVersion set to 2600/26.0These values suggest Xcode “26.0”, which is inconsistent with LastUpgradeCheck = 1620 and typical Xcode versions. Might be a tooling glitch.
Consider setting:
- LastSwiftUpdateCheck to 1620 (or your actual Xcode version),
- CreatedOnToolsVersion for UITests to 16.x,
to avoid future migration prompts.Also applies to: 135-137
src/utils/environment.ts (2)
70-86: Broaden types to match documented behavior; keep null/undefined-safe iterationDocstring and PR notes say null/undefined values are skipped, but the signature only allows string. Align types to avoid unnecessary casting at call sites and to make intent explicit.
Apply:
-export function normalizeTestRunnerEnv(vars: Record<string, string>): Record<string, string> { +export function normalizeTestRunnerEnv( + vars: Record<string, string | null | undefined> | null | undefined, +): Record<string, string> { const normalized: Record<string, string> = {}; - for (const [key, value] of Object.entries(vars ?? {})) { + for (const [key, value] of Object.entries(vars ?? {})) { if (value == null) continue; const prefixedKey = key.startsWith('TEST_RUNNER_') ? key : `TEST_RUNNER_${key}`; normalized[prefixedKey] = value; } return normalized; }
70-86: Optional hardening: validate/sanitize keys to typical env var formatWhile env injection risk is low (passed via spawn env), odd keys (spaces, “=”) can cause downstream issues. Consider normalizing or rejecting invalid names.
For example, warn and skip if the unprefixed name doesn’t match /^[A-Za-z_][A-Za-z0-9_]*$/.
src/utils/build-utils.ts (3)
44-51: Update JSDoc to include execOpts parameterThe function now accepts execOpts; document it for consumers (env, cwd) to encourage consistent usage.
98-110: Default cwd to project directory when not providedMany workflows benefit from running xcodebuild in the project dir (especially with relative paths). If execOpts.cwd is not set, default it to projectDir.
Apply:
@@ - let result; + let result; + const finalExecOpts = execOpts?.cwd ? execOpts : { ...execOpts, cwd: projectDir || execOpts?.cwd }; @@ - result = await executor(command, platformOptions.logPrefix, true, execOpts); + result = await executor(command, platformOptions.logPrefix, true, finalExecOpts);Also applies to: 229-230
221-226: Consider threading execOpts into xcodemake path as well (future-proofing)If callers later use execOpts for things like DEVELOPER_DIR or custom PATH, builds via xcodemake would miss them.
Propagate execOpts (or at least its env/cwd) into executeXcodemakeCommand/executeMakeCommand if their APIs allow it.
src/mcp/tools/device/test_device.ts (2)
41-47: Validate testRunnerEnv keys to a safe patternTo reduce surprises (e.g., spaces or “=”), constrain keys to typical env var names, while still allowing optional TEST_RUNNER_ prefix pre-normalization.
- testRunnerEnv: z - .record(z.string(), z.string()) + testRunnerEnv: z + .record(z.string().regex(/^(TEST_RUNNER_)?[A-Za-z_][A-Za-z0-9_]*$/), z.string()) .optional() .describe( 'Environment variables to pass to the test runner (TEST_RUNNER_ prefix added automatically)', ),
279-295: Expose full validation schema for better UX/DocsYou pass baseSchemaObject.shape to the tool schema, but the actual validation (mutual exclusivity) is in testDeviceSchema. Exposing testDeviceSchema would surface those constraints in generated UIs.
Consider:
- schema: testDeviceSchema (instead of baseSchemaObject.shape)
- Keep handler validator as-is.
src/utils/__tests__/test-runner-env-integration.test.ts (1)
90-106: Add collision and dollar-sign value scenariosTwo gaps worth testing:
- Collision precedence (explicitly prefixed should override normalized).
- Preservation of values containing $ (important to assert normalization is value-transparent).
Apply this diff within the “Core normalization functionality” describe block:
+ it('should prefer explicitly prefixed keys on collision', () => { + const vars = { FOO: 'one', TEST_RUNNER_FOO: 'two' }; + const normalized = normalizeTestRunnerEnv(vars as any); + expect(normalized).toEqual({ TEST_RUNNER_FOO: 'two' }); + }); + + it('should preserve values with dollar signs verbatim', () => { + const vars = { PATH_HINT: '$HOME/bin:$PATH' }; + const normalized = normalizeTestRunnerEnv(vars); + expect(normalized).toEqual({ TEST_RUNNER_PATH_HINT: '$HOME/bin:$PATH' }); + });src/mcp/tools/macos/test_macos.ts (1)
321-324: Consider mentioning testRunnerEnv in the tool descriptionA short note helps discoverability for users scanning the MCP tool help.
Apply this diff to append a sentence:
- description: - 'Runs tests for a macOS project or workspace using xcodebuild test and parses xcresult output. Provide exactly one of projectPath or workspacePath. IMPORTANT: Requires scheme. Example: test_macos({ projectPath: "/path/to/MyProject.xcodeproj", scheme: "MyScheme" })', + description: + 'Runs tests for a macOS project or workspace using xcodebuild test and parses xcresult output. Provide exactly one of projectPath or workspacePath. IMPORTANT: Requires scheme. Accepts optional testRunnerEnv (Record<string,string>) to pass TEST_RUNNER_* variables to the test runner. Example: test_macos({ projectPath: "/path/to/MyProject.xcodeproj", scheme: "MyScheme", testRunnerEnv: { USE_DEV_MODE: "YES" } })',src/utils/command.ts (1)
86-118: “detached” behavior is misnamed or incompletedetached changes waiting semantics but does not set spawn’s detached option. Either:
- Pass the flag through to spawnOpts.detached, or
- Rename the parameter to nonBlocking/asyncStart to reflect behavior.
Minimal change to pass the flag through:
- const spawnOpts: Parameters<typeof spawn>[2] = { + const spawnOpts: Parameters<typeof spawn>[2] = { stdio: ['ignore', 'pipe', 'pipe'], // ignore stdin, pipe stdout/stderr env: { ...process.env, ...(opts?.env ?? {}) }, cwd: opts?.cwd, + detached, };If this semantic change is undesirable now, consider a follow-up rename instead.
docs/TEST_RUNNER_ENV_IMPLEMENTATION_PLAN.md (8)
257-261: Harden Zod schema: accept nulls, constrain keys, and document precedenceThe current schema allows any string key (including empty/invalid env names) and only
string | undefinedvalues. If the intent is to “handle null/undefined,” acceptnulland filter it. Also constrain keys to env‑safe names.-testRunnerEnv: z - .record(z.string(), z.string().optional()) - .optional() - .describe('Test runner environment variables (TEST_RUNNER_ prefix added automatically)') +const envKey = z.string() + .min(1) + .regex(/^[A-Za-z_][A-Za-z0-9_]*$/, 'ENV keys must match ^[A-Za-z_][A-Za-z0-9_]*$'); +const envVal = z.string().or(z.literal('')).nullable().optional(); + +testRunnerEnv: z + .record(envKey, envVal) + .optional() + .describe('Test runner environment variables (keys auto-prefixed with TEST_RUNNER_; null/undefined dropped)')Optional: Add docs/tests clarifying that explicit
TEST_RUNNER_FOOtakes precedence overFOO.
120-125: Shell example: quote values, mention multi-var export alternativeMinor UX nit: quote values to survive spaces/specials, and consider
envorexportpatterns for readability.-TEST_RUNNER_USE_DEV_MODE=YES \ -TEST_RUNNER_runsForEachTargetApplicationUIConfiguration=NO \ +TEST_RUNNER_USE_DEV_MODE="YES" \ +TEST_RUNNER_runsForEachTargetApplicationUIConfiguration="NO" \ xcodebuild test ...Alternative:
env TEST_RUNNER_USE_DEV_MODE="YES" TEST_RUNNER_runsForEachTargetApplicationUIConfiguration="NO" xcodebuild test …- Or set via
exportin the shell session.
101-107: Clarify messaging: explicit unprefixed input vs “only TEST_RUNNER_ can be set”Two statements read inconsistently:
- Lines 101–107: “User Experience: users provide unprefixed keys; prefix added automatically.”
- Lines 350–357: “Prefix Enforcement: Only TEST_RUNNER_ prefixed variables can be set.”
Suggest rewording the latter to: “Only variables sent to xcodebuild are TEST_RUNNER_-prefixed; users may provide unprefixed or already-prefixed keys; the system normalizes and enforces the prefix.” This avoids confusion and matches the proposed utility behavior.
Also applies to: 350-357
139-148: Swift repro: prefer XCTSkip over failing when precondition isn’t metIf the test’s purpose is to validate passthrough only when configured, skipping is less noisy than failing.
- guard useDevMode else { - XCTFail("Test requires USE_DEV_MODE=YES via TEST_RUNNER_USE_DEV_MODE") - return - } + guard useDevMode else { + throw XCTSkip("Requires USE_DEV_MODE=YES via TEST_RUNNER_USE_DEV_MODE") + }
265-274: Log only keys (not values) when debugging exec envReinforce “never log values” by demonstrating a safe debug pattern in examples.
-const execEnv = normalizeTestRunnerEnv(params.testRunnerEnv); +const execEnv = normalizeTestRunnerEnv(params.testRunnerEnv); +if (params.verbose) { + const keys = Object.keys(execEnv).sort(); + logger.debug(`Passing TEST_RUNNER_ keys: ${keys.join(', ')}`); +}Also applies to: 295-299
373-376: Add timeout to CommandExecOptions for resilienceSince you’re generalizing exec options, adding an optional
timeoutMs?: numbernow would help avoid hungxcodebuildprocesses in CI.Example:
export type CommandExecOptions = { cwd?: string; env?: Record<string, string | undefined>; timeoutMs?: number; };Ensure the default executor wires this into
spawn/manual timers.
133-137: Fix markdownlint issues: remove trailing colons from headingsMD026 flags trailing punctuation in headings. Remove the trailing “:” from “Tasks:” headings.
-#### Tasks: +#### TasksAlso applies to: 337-341, 344-349
314-323: Integration tests: add collision test for prefixed + unprefixed keysTo lock in behavior, add a test where both
TEST_RUNNER_FOOandFOOare passed. Assert that only one final key exists and document precedence.I can contribute the test case if you’d like.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (16)
docs/RELOADEROO_FOR_XCODEBUILDMCP.md(1 hunks)docs/TEST_RUNNER_ENV_IMPLEMENTATION_PLAN.md(1 hunks)example_projects/iOS/MCPTest.xcodeproj/project.pbxproj(10 hunks)example_projects/iOS/MCPTest.xcodeproj/xcshareddata/xcschemes/MCPTest.xcscheme(1 hunks)example_projects/iOS/MCPTestUITests/MCPTestUITests.swift(1 hunks)src/mcp/tools/device/test_device.ts(4 hunks)src/mcp/tools/macos/test_macos.ts(4 hunks)src/mcp/tools/simulator/test_sim.ts(2 hunks)src/utils/CommandExecutor.ts(1 hunks)src/utils/__tests__/environment.test.ts(1 hunks)src/utils/__tests__/test-runner-env-integration.test.ts(1 hunks)src/utils/build-utils.ts(3 hunks)src/utils/command.ts(3 hunks)src/utils/environment.ts(1 hunks)src/utils/execution/index.ts(1 hunks)src/utils/test-common.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/utils/command.ts (2)
src/utils/CommandExecutor.ts (1)
CommandExecOptions(3-6)src/utils/execution/index.ts (1)
CommandExecOptions(8-8)
src/utils/__tests__/environment.test.ts (1)
src/utils/environment.ts (1)
normalizeTestRunnerEnv(78-86)
src/utils/test-common.ts (2)
src/utils/CommandExecutor.ts (1)
CommandExecOptions(3-6)src/utils/environment.ts (1)
normalizeTestRunnerEnv(78-86)
src/utils/build-utils.ts (1)
src/utils/CommandExecutor.ts (1)
CommandExecOptions(3-6)
src/utils/__tests__/test-runner-env-integration.test.ts (1)
src/utils/environment.ts (1)
normalizeTestRunnerEnv(78-86)
🪛 LanguageTool
docs/RELOADEROO_FOR_XCODEBUILDMCP.md
[style] ~5-~5: Consider making your message more concise.
Context: ...t will give you no actual benefits. You will end up using more context than just using MCP server...
(DO_END_UP_VBG)
[grammar] ~5-~5: There might be a mistake here.
Context: ...ill end up using more context than just using MCP server directly. So it's recommende...
(QB_NEW_EN)
[grammar] ~7-~7: There might be a mistake here.
Context: ...use XcodeBuildMCP tools. > [!IMPORTANT] > Please remove this introduction before...
(QB_NEW_EN)
[grammar] ~8-~8: Ensure spelling is correct
Context: ...prompt your agent with this file or any derrived version of it. ## Installation Reload...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
docs/TEST_RUNNER_ENV_IMPLEMENTATION_PLAN.md
[grammar] ~58-~58: There might be a mistake here.
Context: ...alysis ### XcodeBuildMCP Execution Flow 1. All Xcode commands flow through `execute...
(QB_NEW_EN)
[grammar] ~76-~76: There might be a mistake here.
Context: ... 1. Automatic Detection (❌ Rejected) - Scan process.env for TEST_RUNNER_ vari...
(QB_NEW_EN)
[grammar] ~81-~81: There might be a mistake here.
Context: ...nt 2. Explicit Parameter (✅ Chosen) - Add testRunnerEnv parameter to test to...
(QB_NEW_EN)
[grammar] ~87-~87: There might be a mistake here.
Context: ...Approach** (🤔 Future Enhancement) - Both automatic + explicit with explicit o...
(QB_NEW_EN)
[grammar] ~88-~88: There might be a mistake here.
Context: ...xplicit with explicit overriding - Issue: Adds complexity, deferred for fu...
(QB_NEW_EN)
[grammar] ~88-~88: There might be a mistake here.
Context: ...exity, deferred for future consideration ### Expert Analysis Summary **RepoPrompt Ana...
(QB_NEW_EN)
[grammar] ~94-~94: There might be a mistake here.
Context: ...licit approach as optimal, highlighting: - Security benefits of explicit allow-list...
(QB_NEW_EN)
[grammar] ~95-~95: There might be a mistake here.
Context: ...benefits of explicit allow-list approach - Architectural soundness of extending Com...
(QB_NEW_EN)
[grammar] ~96-~96: There might be a mistake here.
Context: ...l soundness of extending CommandExecutor - Recommendation for automatic prefix hand...
(QB_NEW_EN)
[grammar] ~133-~133: There might be a mistake here.
Context: ...e and later prove fix works #### Tasks: - [ ] Create test in `example_projects/iOS...
(QB_NEW_EN)
[grammar] ~158-~158: There might be a mistake here.
Context: .../utils/CommandExecutor.ts **Changes**: - AddCommandExecOptions` type for execut...
(QB_NEW_EN)
[grammar] ~180-~180: There might be a mistake here.
Context: .../utils/execution/index.ts **Changes**: - Re-exportCommandExecOptions` type ```...
(QB_NEW_EN)
[grammar] ~191-~191: There might be a mistake here.
Context: ...**: src/utils/command.ts Changes: - Modify getDefaultCommandExecutor to me...
(QB_NEW_EN)
[grammar] ~204-~204: There might be a mistake here.
Context: ...src/utils/environment.ts Changes: - Add normalizeTestRunnerEnv function `...
(QB_NEW_EN)
[grammar] ~227-~227: There might be a mistake here.
Context: ...src/utils/build-utils.ts Changes: - Add optional `execOpts?: CommandExecOpti...
(QB_NEW_EN)
[grammar] ~250-~250: There might be a mistake here.
Context: ...ols/device/test_device.ts **Changes**: - AddtestRunnerEnv` to Zod schema with v...
(QB_NEW_EN)
[grammar] ~281-~281: There might be a mistake here.
Context: ... device test tool - Schema addition for testRunnerEnv - Import normalizeTestRunnerEnv - Pass ...
(QB_NEW_EN)
[grammar] ~282-~282: There might be a mistake here.
Context: ...a addition for testRunnerEnv - Import normalizeTestRunnerEnv - Pass execution options to `executeXcodeB...
(QB_NEW_EN)
[grammar] ~289-~289: There might be a mistake here.
Context: ...ols/simulator/test_sim.ts **Changes**: - AddtestRunnerEnv` to schema - Pass thr...
(QB_NEW_EN)
[grammar] ~295-~295: There might be a mistake here.
Context: ... src/utils/test/index.ts Changes: - Update handleTestLogic signature to ac...
(QB_NEW_EN)
[grammar] ~296-~296: There might be a mistake here.
Context: ...e handleTestLogic signature to accept testRunnerEnv?: Record<string, string | undefined> - Import and use normalizeTestRunnerEnv ...
(QB_NEW_EN)
[grammar] ~297-~297: There might be a mistake here.
Context: ..., string | undefined>- Import and usenormalizeTestRunnerEnv- Pass execution options toexecuteXcodeB...
(QB_NEW_EN)
[grammar] ~308-~308: There might be a mistake here.
Context: ...tests__/environment.test.ts **Tests**: - TestnormalizeTestRunnerEnv` with vario...
(QB_NEW_EN)
[grammar] ~327-~327: There might be a mistake here.
Context: ...files in each tool directory Tests: - Verify schema exports include new `testR...
(QB_NEW_EN)
[grammar] ~337-~337: There might be a mistake here.
Context: ...Reproduction Test Validation Tasks: - Run reproduction test from Phase 0 with ...
(QB_NEW_EN)
[grammar] ~344-~344: There might be a mistake here.
Context: ... Real-World Scenario Testing Tasks: - Test with actual iOS project using `runs...
(QB_NEW_EN)
[grammar] ~345-~345: There might be a mistake here.
Context: ...rld Scenario Testing Tasks: - Test with actual iOS project using `runsForEachTa...
(QB_NEW_EN)
[grammar] ~345-~345: There might be a mistake here.
Context: ...*: - Test with actual iOS project using runsForEachTargetApplicationUIConfiguration - Verify performance difference when varia...
(QB_NEW_EN)
[grammar] ~352-~352: There might be a mistake here.
Context: ...ty Considerations ### Security Benefits - No Environment Leakage: Only explicit ...
(QB_NEW_EN)
[grammar] ~358-~358: There might be a mistake here.
Context: ... can be set ### Security Best Practices - Never log environment variable values (k...
(QB_NEW_EN)
[grammar] ~366-~366: There might be a mistake here.
Context: ...tectural Benefits ### Clean Integration - Extends existing CommandExecutor patte...
(QB_NEW_EN)
[grammar] ~396-~396: There might be a mistake here.
Context: ...to test tools and have variables appear in test runner environment 2. Security...
(QB_NEW_EN)
[grammar] ~397-~397: There might be a mistake here.
Context: ...unintended environment variable leakage from server process 3. Usability: Users ...
(QB_NEW_EN)
[grammar] ~411-~411: There might be a mistake here.
Context: ...0**: 1-2 hours (reproduction test setup) - Phase 1: 4-6 hours (infrastructure cha...
(QB_NEW_EN)
[grammar] ~412-~412: There might be a mistake here.
Context: ... 1**: 4-6 hours (infrastructure changes) - Phase 2: 3-4 hours (tool integration) ...
(QB_NEW_EN)
[grammar] ~413-~413: There might be a mistake here.
Context: ...Phase 2*: 3-4 hours (tool integration) - Phase 3: 4-5 hours (testing) - **Pha...
(QB_NEW_EN)
[grammar] ~414-~414: There might be a mistake here.
Context: ...tion) - Phase 3: 4-5 hours (testing) - Phase 4: 2-3 hours (validation) **Tot...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
docs/TEST_RUNNER_ENV_IMPLEMENTATION_PLAN.md
14-14: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
133-133: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: claude-review
🔇 Additional comments (20)
src/utils/execution/index.ts (1)
8-8: Re-exporting CommandExecOptions from the execution facade — LGTMCentralizing the types here keeps imports consistent for consumers. No concerns.
example_projects/iOS/MCPTest.xcodeproj/xcshareddata/xcschemes/MCPTest.xcscheme (1)
32-44: UI test target wiring into the scheme — LGTMThe Testables entry correctly references MCPTestUITests.xctest and enables parallelization. No blocking issues.
example_projects/iOS/MCPTest.xcodeproj/project.pbxproj (1)
10-17: Target dependency wiring looks correctThe PBXContainerItemProxy and PBXTargetDependency correctly point MCPTestUITests at MCPTest. This is necessary for UI tests to run.
Also applies to: 194-200
src/utils/environment.ts (1)
78-86: LGTM: clear, side-effect-free normalizationCreates a new map, preserves TEST_RUNNER_ keys, and skips nullish values. Implementation is straightforward.
src/utils/test-common.ts (2)
183-201: Good propagation of exec options through the build command pathexecOpts threading into executeXcodeBuildCommand for the ‘test’ action cleanly enables TEST_RUNNER_ env passthrough.
178-182: No env clobbering: merge semantics already handled in CommandExecutorI checked
src/utils/command.tsand confirmed that the default spawn options merge the providedopts.envwithprocess.env:// src/utils/command.ts:67–71 const spawnOpts: Parameters<typeof spawn>[2] = { stdio: ['ignore', 'pipe', 'pipe'], env: { ...process.env, ...(opts?.env ?? {}) }, cwd: opts?.cwd, };Since
opts.envis overlaid ontoprocess.env, passing throughexecOptsas{ env: normalizeTestRunnerEnv(...) }will not clobberPATHor other variables. No changes are needed here.src/utils/build-utils.ts (1)
229-230: LGTM: exec options are forwarded to the executor for xcodebuild pathThis enables environment passthrough for test runs, while leaving xcodemake unaffected.
src/mcp/tools/device/test_device.ts (1)
199-203: Correctly builds exec options with normalized TEST_RUNNER_ envThe normalization and conditional execOpts creation look solid and match the intended security posture.
src/utils/__tests__/environment.test.ts (1)
9-44: Solid coverage of core normalization pathsBasic functionality tests precisely capture the contract (prefixing, preservation, mixed keys). Nice job keeping assertions explicit.
src/mcp/tools/simulator/test_sim.ts (2)
57-62: Schema addition looks right and constrains values to stringsUsing z.record(z.string(), z.string()) is appropriate; optional is correct. The description communicates behavior clearly.
105-106: Normalization of testRunnerEnv confirmedhandleTestLogic in
src/utils/test-common.ts(lines 179–180) wrapsparams.testRunnerEnvwithnormalizeTestRunnerEnvbefore assigning toexecOpts, preventing any raw pass-through. No further changes needed.src/utils/__tests__/test-runner-env-integration.test.ts (1)
12-166: Good end-to-end normalization scenariosRealistic permutations, null filtering, special chars, and immutability are well covered. This complements the unit suite nicely.
src/mcp/tools/macos/test_macos.ts (2)
42-47: Schema: clear, minimally permissive, and aligned with intended usageOptional testRunnerEnv with string-only values is a sensible boundary. Description is explicit about automatic prefixing.
243-247: Correct construction of execOpts with normalized envCreating execOpts only when provided and normalizing via normalizeTestRunnerEnv is exactly what we want to avoid leaking arbitrary env names.
src/utils/command.ts (4)
17-21: Type re-exports are correct and help keep import sites stableRe-exporting CommandExecOptions alongside existing types is clean and backward compatible.
29-31: Docstring updates correctly reflect the new opts parameterClear guidance on opts.env and opts.cwd. No issues.
37-38: Signature change to accept opts is appropriateConsolidating env and cwd under CommandExecOptions is a good API surface improvement.
69-71: env and cwd merge behavior is correctMerging process.env with opts?.env and threading cwd is the right approach. No leakage of testRunnerEnv into logs since it’s not interpolated in the command string.
docs/TEST_RUNNER_ENV_IMPLEMENTATION_PLAN.md (2)
1-7: Strong, security‑aware plan; aligns with PR objectivesThis implementation plan is thoughtful and consistent with the PR goals: explicit env allow‑listing, Zod validation, threading execution options, and end‑to‑end tests. Nice job keeping backward compatibility and calling out non‑logging of values.
232-240: All executeXcodeBuildCommand invocations support the new optional execOpts parameterI’ve verified that every direct call to
executeXcodeBuildCommandeither passes the sixthexecOptsargument when needed (e.g. in test runners) or omits it—relying on its optional signature—so there are no broken call sites. Additionally, theCommandExecOptionstype is correctly re-exported:• src/utils/test-common.ts and both macOS/device test modules pass
execOptsas the sixth argument.
• All other usages (clean, simulator, macOS/device build flows) supply only five arguments, which remains compatible.
• TheCommandExecOptionstype is re-exported in src/utils/command.ts and src/utils/execution/index.ts.No further changes required.
| ``` | ||
| TEST_RUNNER_<VAR> Set an environment variable whose name is prefixed | ||
| with TEST_RUNNER_ to have that variable passed, with | ||
| its prefix stripped, to all test runner processes | ||
| launched during a test action. For example, | ||
| TEST_RUNNER_Foo=Bar xcodebuild test ... sets the | ||
| environment variable Foo=Bar in the test runner's | ||
| environment. | ||
| ``` |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add fenced language and include authoritative citation for the man page snippet
For markdownlint and clarity, specify a fenced language (use text) for the man-page excerpt. Also, please add a citation to the current xcodebuild(1) manual to future‑proof the claim.
🌐 Web query:
Point me to the official Apple developer documentation or man page reference that describes TEST_RUNNER_ environment variable behavior for xcodebuild test.
💡 Result:
The TEST_RUNNER_ environment variable behavior for xcodebuild test is officially documented in the xcodebuild(1) man page[3][4]. When you set an environment variable prefixed with TEST_RUNNER_ (e.g., TEST_RUNNER_Foo=Bar), this variable is passed to all test runner processes during a test action, with the prefix stripped (so Foo=Bar is visible within the test runner's environment)[3][4].
The exact reference states:
Set an environment variable whose name is prefixed with TEST_RUNNER_ to have that variable passed, with its prefix stripped, to all test runner processes launched during a test action. For example, TEST_RUNNER_Foo=Bar xcodebuild test ... sets the environment variable Foo=Bar in the test runner's environment[3][4].
Key points:
- The prefix TEST_RUNNER_ is stripped, so code in your test runner accesses
ProcessInfo.processInfo.environment["Foo"], notTEST_RUNNER_Foo[3][4][1]. - This mechanism allows you to reliably inject environment variables into your tests via the CLI, CI/CD systems, or custom scripts[1][3][4].
- Existing variables can be modified using the token
__CURRENT_VALUE__to represent their current value[3].
For further practical details and Apple's guidance on this topic, review the official Apple developer man page[3], the GitHub pages mirror[4], and relevant WWDC documentation[1]. The full reference for Xcode build environment settings is available at the Apple Developer Documentation[4].
Citations:
- 1: https://developer.apple.com/videos/play/wwdc2022/110361/
- 2: https://docs.appcircle.io/workflows/ios-specific-workflow-steps/xcodebuild-for-unit-and-ui-test
- 3: https://leancrew.com/all-this/man/man1/xcodebuild.html
- 4: https://keith.github.io/xcode-man-pages/xcodebuild.1.html
- 5: https://www.nathankrishnan.com/passing-arguments-and-environment-variables-to-xctests/
- 6: https://www.nsnam.org/docs/release/3.40/tutorial/singlehtml/index.html
- 7: https://developer.apple.com/documentation/xcode-release-notes/xcode-16_3-release-notes
- 8: https://docs.aws.amazon.com/devicefarm/latest/developerguide/custom-test-environment-test-types-xcode-ui-env.html
- 9: https://developer.apple.com/documentation/xctest
- 10: https://developer.apple.com/documentation/Xcode/adding-tests-to-your-xcode-project
docs/TEST_RUNNER_ENV_IMPLEMENTATION_PLAN.md – add fenced code language and official citation
Please update the snippet at lines 14–22 to use a fenced code block with text as the language and include an authoritative citation to the xcodebuild(1) man page. For example:
Set an environment variable whose name is prefixed with TEST_RUNNER_ to have that variable passed, with its prefix stripped, to all test runner processes launched during a test action. For example, TEST_RUNNER_Foo=Bar xcodebuild test … sets the environment variable Foo=Bar in the test runner’s environment.
# Reference: xcodebuild(1) man page (https://developer.apple.com/man-pages/xcodebuild.1.html)
- ensures markdownlint passes the snippet as plain text
- future-proofs the documentation with an official Apple citation
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
14-14: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In docs/TEST_RUNNER_ENV_IMPLEMENTATION_PLAN.md around lines 14 to 22, the fenced
code block should be marked as plain text and include an authoritative citation
to the xcodebuild(1) man page; replace the existing block fence language with
```text and append a reference line such as "Reference: xcodebuild(1) man page
(https://developer.apple.com/man-pages/xcodebuild.1.html)" at the end of the
block so the snippet is treated as plain text by markdownlint and includes the
official Apple citation.
| // In the returned function: | ||
| const env = { ...process.env, ...(opts?.env ?? {}) }; | ||
| // Pass env and opts?.cwd to spawn/exec call | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Filter undefined before merging env to avoid child_process spawn errors
Merging { ...process.env, ...(opts?.env ?? {}) } will include keys with undefined values if any slip through, which can crash spawn/exec. Filter them proactively.
-const env = { ...process.env, ...(opts?.env ?? {}) };
+const merged = { ...process.env, ...(opts?.env ?? {}) } as Record<string, unknown>;
+const env = Object.fromEntries(
+ Object.entries(merged).filter(([, v]) => v !== undefined && v !== null).map(([k, v]) => [k, String(v)])
+);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // In the returned function: | |
| const env = { ...process.env, ...(opts?.env ?? {}) }; | |
| // Pass env and opts?.cwd to spawn/exec call | |
| ``` | |
| // In the returned function: | |
| const merged = { ...process.env, ...(opts?.env ?? {}) } as Record<string, unknown>; | |
| const env = Object.fromEntries( | |
| Object.entries(merged) | |
| .filter(([, v]) => v !== undefined && v !== null) | |
| .map(([k, v]) => [k, String(v)]) | |
| ); | |
| // Pass env and opts?.cwd to spawn/exec call |
🤖 Prompt for AI Agents
In docs/TEST_RUNNER_ENV_IMPLEMENTATION_PLAN.md around lines 195 to 198, filter
out undefined values from opts?.env before merging with process.env to prevent
child_process spawn/exec errors; create a new object from opts?.env that removes
entries with undefined values (e.g., via Object.entries(...).filter(...) then
Object.fromEntries(...)) and merge that with process.env when building env, and
ensure you pass that filtered env and opts?.cwd into the spawn/exec call.
| export function normalizeTestRunnerEnv( | ||
| userVars?: Record<string, string | undefined> | ||
| ): Record<string, string> { | ||
| const result: Record<string, string> = {}; | ||
| if (userVars) { | ||
| for (const [key, value] of Object.entries(userVars)) { | ||
| if (value !== undefined) { | ||
| result[`TEST_RUNNER_${key}`] = value; | ||
| } | ||
| } | ||
| } | ||
| return result; | ||
| } | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
normalizeTestRunnerEnv will double‑prefix keys and doesn’t handle nulls; tighten sanitization
The sample implementation unconditionally prefixes keys, causing TEST_RUNNER_TEST_RUNNER_FOO if callers already supply a prefixed key. It also doesn’t accept or filter null values (contradicting the plan) and risks passing non‑strings to the process env if callers later broaden types. Let’s preserve existing prefixes, drop null/undefined, trim keys, and enforce an env‑safe key pattern.
Apply this diff to the snippet:
-export function normalizeTestRunnerEnv(
- userVars?: Record<string, string | undefined>
-): Record<string, string> {
- const result: Record<string, string> = {};
- if (userVars) {
- for (const [key, value] of Object.entries(userVars)) {
- if (value !== undefined) {
- result[`TEST_RUNNER_${key}`] = value;
- }
- }
- }
- return result;
-}
+export function normalizeTestRunnerEnv(
+ userVars?: Record<string, string | null | undefined>
+): Record<string, string> {
+ const result: Record<string, string> = {};
+ if (!userVars) return result;
+ for (const [rawKey, rawVal] of Object.entries(userVars)) {
+ if (rawVal == null) continue; // drop null/undefined
+ const key = String(rawKey).trim();
+ // Accept only ENV-safe keys: letters, digits, underscore, starting with letter/underscore
+ if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(key)) continue;
+ const finalKey = key.startsWith('TEST_RUNNER_') ? key : `TEST_RUNNER_${key}`;
+ result[finalKey] = String(rawVal);
+ }
+ return result;
+}Follow‑ups:
- Define precedence if both
FOOandTEST_RUNNER_FOOappear; commonly, let explicitTEST_RUNNER_FOOwin. I can add a test for this if helpful.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In docs/TEST_RUNNER_ENV_IMPLEMENTATION_PLAN.md around lines 208 to 221, the
normalizeTestRunnerEnv implementation unconditionally prefixes keys (causing
TEST_RUNNER_TEST_RUNNER_FOO), does not treat null values as absent, doesn’t trim
keys, and may allow non-string values; update the function to: skip null or
undefined values, trim key names, detect and preserve an existing TEST_RUNNER_
prefix instead of re‑prefixing, enforce that keys match a safe env var pattern
(e.g., /^[A-Z0-9_]+$/) after trimming and uppercasing, and resolve precedence so
an explicit TEST_RUNNER_FOO provided wins over FOO; ensure returned values are
strings only.
| 8BC6F14D2E58FBAD008DD7EC /* Sources */ = { | ||
| isa = PBXSourcesBuildPhase; | ||
| buildActionMask = 2147483647; | ||
| files = ( | ||
| ); | ||
| runOnlyForDeploymentPostprocessing = 0; | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
UITest Sources build phase is empty — tests may not compile or run
PBXSourcesBuildPhase.files is empty for MCPTestUITests. Unless you're relying on new Xcode auto-synchronization that also wires build files (unlikely), no test sources will be compiled.
Run this to confirm the UITest file is referenced as a PBXBuildFile and included in the Sources phase:
If empty, add MCPTestUITests.swift to the target’s Sources.
I can generate a pbxproj patch to add the build file entry if you share the exact relative path of MCPTestUITests.swift.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect at least one build file for MCPTestUITests.swift and its inclusion in the Sources phase
rg -n "MCPTestUITests\\.swift" -C3 example_projects/iOS | cat
rg -nP '(?s)8BC6F14D2E58FBAD008DD7EC \/\* Sources \*\/.*?files = \((.*?)\);' example_projects/iOS/MCPTest.xcodeproj/project.pbxproj -C0 | catLength of output: 82
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate any UITest Swift files under the iOS project to determine their relative paths
find example_projects/iOS -type f -name "*UITests.swift"Length of output: 135
Add MCPTestUITests.swift to the MCPTestUITests target’s Sources build phase
The PBXSourcesBuildPhase for the MCPTestUITests target (ID 8BC6F14D2E58FBAD008DD7EC) is empty, and project.pbxproj contains no PBXFileReference or PBXBuildFile entries for the UI test file at
example_projects/iOS/MCPTestUITests/MCPTestUITests.swift. Without these entries, Xcode won’t compile or run your UI tests.
Please update example_projects/iOS/MCPTest.xcodeproj/project.pbxproj as follows:
- In the PBXFileReference section, add an entry for
MCPTestUITests/MCPTestUITests.swift, e.g./* Begin PBXFileReference section */ + <FILE_REF_GUID> /* MCPTestUITests.swift */ = { + isa = PBXFileReference; + lastKnownFileType = sourcecode.swift; + name = MCPTestUITests.swift; + path = MCPTestUITests/MCPTestUITests.swift; + sourceTree = "<group>"; + }; /* End PBXFileReference section */
- In the PBXBuildFile section, add a build-file entry pointing to that
fileRef, e.g./* Begin PBXBuildFile section */ + <BUILD_FILE_GUID> /* MCPTestUITests.swift in Sources */ = { + isa = PBXBuildFile; + fileRef = <FILE_REF_GUID> /* MCPTestUITests.swift */; + }; /* End PBXBuildFile section */
- In the PBXSourcesBuildPhase (ID 8BC6F14D2E58FBAD008DD7EC), include the new build-file GUID in the
filesarray, e.g.8BC6F14D2E58FBAD008DD7EC /* Sources */ = { isa = PBXSourcesBuildPhase; buildActionMask = 2147483647; - files = ( - ); + files = ( + <BUILD_FILE_GUID> /* MCPTestUITests.swift in Sources */, + ); runOnlyForDeploymentPostprocessing = 0; };
Let me know if you’d like a fully populated patch with the actual GUIDs.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In example_projects/iOS/MCPTest.xcodeproj/project.pbxproj around lines 185 to
191, the PBXSourcesBuildPhase for the MCPTestUITests target (ID
8BC6F14D2E58FBAD008DD7EC) is empty and the MCPTestUITests.swift file is not
referenced; add a PBXFileReference entry for
example_projects/iOS/MCPTestUITests/MCPTestUITests.swift in the PBXFileReference
section, add a corresponding PBXBuildFile entry in the PBXBuildFile section that
points to that fileRef, and then insert the new PBXBuildFile GUID into the files
array of the PBXSourcesBuildPhase with ID 8BC6F14D2E58FBAD008DD7EC so Xcode
includes and compiles the UI test file.
| isa = XCBuildConfiguration; | ||
| buildSettings = { | ||
| CODE_SIGN_STYLE = Automatic; | ||
| CURRENT_PROJECT_VERSION = 1; | ||
| DEVELOPMENT_TEAM = BR6WD3M6ZD; | ||
| GENERATE_INFOPLIST_FILE = YES; | ||
| IPHONEOS_DEPLOYMENT_TARGET = 26.0; | ||
| MARKETING_VERSION = 1.0; | ||
| PRODUCT_BUNDLE_IDENTIFIER = com.cameroncooke.test.MCPTestUITests; | ||
| PRODUCT_NAME = "$(TARGET_NAME)"; | ||
| STRING_CATALOG_GENERATE_SYMBOLS = NO; | ||
| SWIFT_APPROACHABLE_CONCURRENCY = YES; | ||
| SWIFT_EMIT_LOC_STRINGS = NO; | ||
| SWIFT_UPCOMING_FEATURE_MEMBER_IMPORT_VISIBILITY = YES; | ||
| SWIFT_VERSION = 5.0; | ||
| TARGETED_DEVICE_FAMILY = "1,2"; | ||
| TEST_TARGET_NAME = MCPTest; | ||
| }; | ||
| name = Debug; |
There was a problem hiding this comment.
Fix invalid iOS deployment target (26.0) on UITest target
26.0 is not a valid iOS deployment version and will break builds. Align the UITest target with the project-level IPHONEOS_DEPLOYMENT_TARGET (18.2) or inherit it.
Apply this diff in both Debug and Release configurations:
- IPHONEOS_DEPLOYMENT_TARGET = 26.0;
+ IPHONEOS_DEPLOYMENT_TARGET = 18.2; // align with project-level settingAlso applies to: 402-421
🤖 Prompt for AI Agents
In example_projects/iOS/MCPTest.xcodeproj/project.pbxproj around lines 381-399
(and similarly 402-421 for Release), the UITest build configurations set
IPHONEOS_DEPLOYMENT_TARGET = 26.0 which is invalid; change
IPHONEOS_DEPLOYMENT_TARGET to 18.2 to match the project-level deployment target
or remove the IPHONEOS_DEPLOYMENT_TARGET entry so the target inherits the
project value, and apply the same fix in both Debug and Release configuration
blocks.
| if isDevMode { | ||
| XCTSkip("Dev mode: skipping heavy or duplicated UI configuration runs") | ||
| } |
There was a problem hiding this comment.
Fix: XCTSkip must be thrown (or tried) to compile
XCTSkip is throwing; calling it without try/throw will fail to compile.
Apply this diff:
- if isDevMode {
- XCTSkip("Dev mode: skipping heavy or duplicated UI configuration runs")
- }
+ if isDevMode {
+ throw XCTSkip("Dev mode: skipping heavy or duplicated UI configuration runs")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if isDevMode { | |
| XCTSkip("Dev mode: skipping heavy or duplicated UI configuration runs") | |
| } | |
| if isDevMode { | |
| throw XCTSkip("Dev mode: skipping heavy or duplicated UI configuration runs") | |
| } |
🤖 Prompt for AI Agents
In example_projects/iOS/MCPTestUITests/MCPTestUITests.swift around lines 36–38,
the call to XCTSkip is a throwing function and must be invoked with try (or
handled); change the line to call try XCTSkip("Dev mode: skipping heavy or
duplicated UI configuration runs") and ensure the enclosing test method
signature includes throws (e.g., func testSomething() throws) or otherwise
handle the thrown error with do/try/catch so the file compiles.
| describe('Prefix Handling Edge Cases', () => { | ||
| it('should not double-prefix already prefixed keys', () => { | ||
| const input = { TEST_RUNNER_FOO: 'value1' }; | ||
| const result = normalizeTestRunnerEnv(input); | ||
|
|
||
| expect(result).toEqual({ | ||
| TEST_RUNNER_FOO: 'value1', | ||
| }); | ||
|
|
||
| // Ensure no double prefixing occurred | ||
| expect(result).not.toHaveProperty('TEST_RUNNER_TEST_RUNNER_FOO'); | ||
| }); | ||
|
|
||
| it('should handle partial prefix matches correctly', () => { | ||
| const input = { | ||
| TEST_RUN: 'value1', // Should get prefixed (not TEST_RUNNER_) | ||
| TEST_RUNNER: 'value2', // Should get prefixed (no underscore) | ||
| TEST_RUNNER_FOO: 'value3', // Should not get prefixed | ||
| }; | ||
| const result = normalizeTestRunnerEnv(input); | ||
|
|
||
| expect(result).toEqual({ | ||
| TEST_RUNNER_TEST_RUN: 'value1', | ||
| TEST_RUNNER_TEST_RUNNER: 'value2', | ||
| TEST_RUNNER_FOO: 'value3', | ||
| }); | ||
| }); | ||
|
|
||
| it('should handle case-sensitive prefix detection', () => { | ||
| const input = { | ||
| test_runner_foo: 'value1', // lowercase - should get prefixed | ||
| Test_Runner_Bar: 'value2', // mixed case - should get prefixed | ||
| TEST_RUNNER_BAZ: 'value3', // correct case - should not get prefixed | ||
| }; | ||
| const result = normalizeTestRunnerEnv(input); | ||
|
|
||
| expect(result).toEqual({ | ||
| TEST_RUNNER_test_runner_foo: 'value1', | ||
| TEST_RUNNER_Test_Runner_Bar: 'value2', | ||
| TEST_RUNNER_BAZ: 'value3', | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add collision-precedence test (explicitly prefixed key should win)
Edge-case coverage is strong, but we’re missing the “dual-provided key” case. Today, when both FOO and TEST_RUNNER_FOO are present, the implementation will iterate in insertion order and let the explicitly prefixed key overwrite the normalized one. Let’s lock that behavior in tests to avoid regressions.
Apply this diff inside the “Prefix Handling Edge Cases” describe block:
+ it('should prefer explicitly prefixed keys when collisions occur', () => {
+ const input = {
+ FOO: 'normalized-value',
+ TEST_RUNNER_FOO: 'explicit-value',
+ };
+ const result = normalizeTestRunnerEnv(input as any);
+ expect(result).toEqual({
+ TEST_RUNNER_FOO: 'explicit-value',
+ });
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe('Prefix Handling Edge Cases', () => { | |
| it('should not double-prefix already prefixed keys', () => { | |
| const input = { TEST_RUNNER_FOO: 'value1' }; | |
| const result = normalizeTestRunnerEnv(input); | |
| expect(result).toEqual({ | |
| TEST_RUNNER_FOO: 'value1', | |
| }); | |
| // Ensure no double prefixing occurred | |
| expect(result).not.toHaveProperty('TEST_RUNNER_TEST_RUNNER_FOO'); | |
| }); | |
| it('should handle partial prefix matches correctly', () => { | |
| const input = { | |
| TEST_RUN: 'value1', // Should get prefixed (not TEST_RUNNER_) | |
| TEST_RUNNER: 'value2', // Should get prefixed (no underscore) | |
| TEST_RUNNER_FOO: 'value3', // Should not get prefixed | |
| }; | |
| const result = normalizeTestRunnerEnv(input); | |
| expect(result).toEqual({ | |
| TEST_RUNNER_TEST_RUN: 'value1', | |
| TEST_RUNNER_TEST_RUNNER: 'value2', | |
| TEST_RUNNER_FOO: 'value3', | |
| }); | |
| }); | |
| it('should handle case-sensitive prefix detection', () => { | |
| const input = { | |
| test_runner_foo: 'value1', // lowercase - should get prefixed | |
| Test_Runner_Bar: 'value2', // mixed case - should get prefixed | |
| TEST_RUNNER_BAZ: 'value3', // correct case - should not get prefixed | |
| }; | |
| const result = normalizeTestRunnerEnv(input); | |
| expect(result).toEqual({ | |
| TEST_RUNNER_test_runner_foo: 'value1', | |
| TEST_RUNNER_Test_Runner_Bar: 'value2', | |
| TEST_RUNNER_BAZ: 'value3', | |
| }); | |
| }); | |
| describe('Prefix Handling Edge Cases', () => { | |
| it('should not double-prefix already prefixed keys', () => { | |
| const input = { TEST_RUNNER_FOO: 'value1' }; | |
| const result = normalizeTestRunnerEnv(input); | |
| expect(result).toEqual({ | |
| TEST_RUNNER_FOO: 'value1', | |
| }); | |
| // Ensure no double prefixing occurred | |
| expect(result).not.toHaveProperty('TEST_RUNNER_TEST_RUNNER_FOO'); | |
| }); | |
| it('should handle partial prefix matches correctly', () => { | |
| const input = { | |
| TEST_RUN: 'value1', // Should get prefixed (not TEST_RUNNER_) | |
| TEST_RUNNER: 'value2', // Should get prefixed (no underscore) | |
| TEST_RUNNER_FOO: 'value3', // Should not get prefixed | |
| }; | |
| const result = normalizeTestRunnerEnv(input); | |
| expect(result).toEqual({ | |
| TEST_RUNNER_TEST_RUN: 'value1', | |
| TEST_RUNNER_TEST_RUNNER: 'value2', | |
| TEST_RUNNER_FOO: 'value3', | |
| }); | |
| }); | |
| it('should handle case-sensitive prefix detection', () => { | |
| const input = { | |
| test_runner_foo: 'value1', // lowercase - should get prefixed | |
| Test_Runner_Bar: 'value2', // mixed case - should get prefixed | |
| TEST_RUNNER_BAZ: 'value3', // correct case - should not get prefixed | |
| }; | |
| const result = normalizeTestRunnerEnv(input); | |
| expect(result).toEqual({ | |
| TEST_RUNNER_test_runner_foo: 'value1', | |
| TEST_RUNNER_Test_Runner_Bar: 'value2', | |
| TEST_RUNNER_BAZ: 'value3', | |
| }); | |
| }); | |
| it('should prefer explicitly prefixed keys when collisions occur', () => { | |
| const input = { | |
| FOO: 'normalized-value', | |
| TEST_RUNNER_FOO: 'explicit-value', | |
| }; | |
| const result = normalizeTestRunnerEnv(input as any); | |
| expect(result).toEqual({ | |
| TEST_RUNNER_FOO: 'explicit-value', | |
| }); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In src/utils/__tests__/environment.test.ts around lines 166 to 207, add a new
test inside the "Prefix Handling Edge Cases" describe block that verifies
collision precedence: create an input object containing both a plain key (e.g.
FOO) and its explicitly prefixed counterpart (TEST_RUNNER_FOO) with different
values, call normalizeTestRunnerEnv, and assert that the resulting map contains
only TEST_RUNNER_FOO with the explicitly prefixed value (i.e., the explicit
prefixed key wins), and that no normalized duplicate (e.g. TEST_RUNNER_FOO from
FOO) overwrites or coexists; keep the test name clear like "should prefer
explicitly prefixed key when both plain and prefixed are provided".
|
|
||
| // Types | ||
| export type { CommandExecutor, CommandResponse } from '../CommandExecutor.ts'; | ||
| export type { CommandExecutor, CommandResponse, CommandExecOptions } from '../CommandExecutor.ts'; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify downstream imports now consume CommandExecOptions from this facade
To avoid mixed import paths, ensure all modules import CommandExecOptions from utils/execution/index.ts rather than the underlying file.
Run this script to find any direct imports from CommandExecutor.ts that should be switched to the execution facade:
🏁 Script executed:
#!/bin/bash
# Find direct imports of CommandExecutor.ts to standardize on utils/execution
rg -nP -C1 --type=ts "from ['\"][^'\"]*CommandExecutor\.ts['\"]"Length of output: 1134
Standardize imports to use the utils/execution facade
We still have direct imports of CommandExecutor, CommandResponse, and CommandExecOptions from the underlying CommandExecutor.ts file. To avoid mixed import paths, all downstream modules should import these types from the new facade at src/utils/execution/index.ts (i.e. utils/execution).
Files requiring updates:
-
src/utils/command.ts
• Line 17:- import { CommandExecutor, CommandResponse, CommandExecOptions } from './CommandExecutor.ts'; + import type { CommandExecutor, CommandResponse, CommandExecOptions } from './execution';
• Line 20:
- export { CommandExecutor, CommandResponse, CommandExecOptions } from './CommandExecutor.ts'; + export type { CommandExecutor, CommandResponse, CommandExecOptions } from './execution';
-
src/test-utils/mock-executors.ts
• Line 19:- import { CommandExecutor } from '../utils/CommandExecutor.ts'; + import type { CommandExecutor } from '../utils/execution';
Please apply these changes so that every module consistently consumes types via the utils/execution facade.
🤖 Prompt for AI Agents
In src/utils/execution/index.ts around line 8, the facade should re-export
CommandExecutor, CommandResponse, and CommandExecOptions so downstream modules
import from "utils/execution"; update this file to export those types (already
showing export line) and then update imports in the callers: change
src/utils/command.ts at lines 17 and 20 to import the types from
"utils/execution" instead of the underlying CommandExecutor.ts, and change
src/test-utils/mock-executors.ts at line 19 to import from "utils/execution" as
well; ensure all import paths use the facade path (utils/execution) and run
type-check to confirm no remaining direct imports of CommandExecutor.ts remain.
|
the PR build is working for me! |
2 participants
Summary
Implements support for TEST_RUNNER_ environment variable passthrough to xcodebuild test commands, addressing issue #101. This enables test frameworks to receive environment variables from MCP clients through xcodebuild's built-in TEST_RUNNER_ prefix mechanism.
Background/Details
The issue requested the ability to pass environment variables to test runners when using xcodebuild test commands. This is particularly useful for:
xcodebuild has built-in support for TEST_RUNNER_ prefixed environment variables - it automatically strips the prefix and passes the remaining variables to the test runner process.
Solution
Implemented an explicit parameter approach that:
Infrastructure Updates:
CommandExecutorwithCommandExecOptionsinterface for environment and working directory supportexecuteXcodeBuildCommandto accept execution options for environment variable passinghandleTestLogicto support test runner environment variablesNew Utility Function:
normalizeTestRunnerEnv()insrc/utils/environment.tsTEST_RUNNER_if not already prefixedTool Integration:
testRunnerEnvparameter to all three test tools:test_device,test_macos,test_simSecurity Design:
Testing
normalizeTestRunnerEnvfunction covering edge casestestRunnerEnv: { USE_DEV_MODE: "YES" }- passesArchitecture Benefits
This explicit approach provides several advantages over automatic environment detection:
Notes
Summary by CodeRabbit
New Features
Documentation
Tests