refactor(architecture): separate server, test, and pure utils to fix CI errors #96
Conversation
|
Cursor review |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 83 files out of 190 files are above the max files limit of 100. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the WalkthroughConsolidates executor and filesystem types into focused modules, adds deterministic test mock executors, refactors many imports to new focused facades, streamlines TOOLS.md and generator, removes verification artifacts, and hardens/enhances the LLM-driven discover_tools control flow and sanitization. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant discover_toolsLogic
participant Sanitizer as sanitizeTaskDescription
participant LLM
participant Parser as parseLLMResponse
participant Enabler as enableWorkflows
participant Server
User->>discover_toolsLogic: request with task_description
discover_toolsLogic->>Sanitizer: sanitize input
Sanitizer-->>discover_toolsLogic: sanitized text
discover_toolsLogic->>LLM: send prompt (configurable maxTokens/temperature)
LLM-->>discover_toolsLogic: sampling result
discover_toolsLogic->>Parser: extract and validate JSON list
Parser-->>discover_toolsLogic: workflow list / error
discover_toolsLogic->>Enabler: enableWorkflows(server, selections)
Enabler-->>discover_toolsLogic: success
discover_toolsLogic-->>User: success message (or error guidance)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ 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: Architectural RefactoringThis is an excellent architectural refactoring that successfully addresses the CI module linking issues while delivering substantial code quality improvements. The changes are well-structured, thoroughly implemented, and demonstrate strong engineering principles. ✅ Strengths1. Architectural Excellence
2. DRY Principle Implementation
3. Testing Architecture Improvements
4. Code Quality Fixes
|
|
Claude finished @cameroncooke's task —— View job PR Review: Architectural RefactoringThis is an excellent architectural refactoring that successfully addresses the CI module linking issues while delivering substantial code quality improvements. The changes are well-structured, thoroughly implemented, and demonstrate strong engineering principles. Review Progress:
✅ Strengths1. Architectural Excellence
2. DRY Principle Implementation
3. Testing Architecture Improvements
4. Code Quality Fixes
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (14)
README.md (3)
194-201: Fix typo in table header: “Samplng” → “Sampling”User-facing docs should be typo-free.
Apply this diff:
-| Editor | Tools | Resources | Samplng | +| Editor | Tools | Resources | Sampling |
130-131: Fix typo: “ammending” → “amending”Minor documentation correction for professionalism.
Apply this diff:
-Configure your MCP client (Windsurf, Cursor, Claude Desktop, Claude Code etc.) to use the XcodeBuildMCP server by ammending your client application's MCP configuration. +Configure your MCP client (Windsurf, Cursor, Claude Desktop, Claude Code etc.) to use the XcodeBuildMCP server by amending your client application's MCP configuration.
419-421: Fix typo: “MseeP.a” → “MseeP.ai”Correct the link label to avoid confusion.
Apply this diff:
-### MseeP.a +### MseeP.aisrc/utils/logger.ts (1)
46-48: Use the lazy Sentry capture to avoid hard dependency and keep logging non-blockingThis change eliminates the static Sentry import and ensures error logging doesn’t block execution.
Apply this diff:
- if (level === 'error' && SENTRY_ENABLED) { - Sentry.captureMessage(logMessage); - } + if (level === 'error' && SENTRY_ENABLED) { + void captureErrorInSentry(logMessage); + }src/server/tool-registry.ts (3)
121-128: Type the discoveryTools array to satisfy no-unsafe-argument and improve safetyEmpty arrays default to any[], which violates the ESLint unsafe rules when passed to registerTools.
Apply this diff:
- const discoveryTools = []; + const discoveryTools: Parameters<McpServer['registerTools']>[0] = [];
157-176: Type the selectedTools array for the same reasonEnsures type-safe bulk registration and keeps ESLint happy.
Apply this diff:
- const selectedTools = []; + const selectedTools: Parameters<McpServer['registerTools']>[0] = [];
191-204: Type the allTools array to avoid implicit anyConsistent with the other arrays.
Apply this diff:
- const allTools = []; + const allTools: Parameters<McpServer['registerTools']>[0] = [];src/mcp/tools/discovery/discover_tools.ts (1)
24-31: Fix NaN handling for LLM env vars (can crash createMessage with invalid inputs)If env values are set but non-numeric, parseInt/parseFloat produce NaN. You’ll pass NaN to the client LLM, likely causing runtime errors.
-const getLLMConfig = (): LLMConfig => ({ - maxTokens: process.env.XCODEBUILDMCP_LLM_MAX_TOKENS - ? parseInt(process.env.XCODEBUILDMCP_LLM_TOKENS, 10) - : 200, - temperature: process.env.XCODEBUILDMCP_LLM_TEMPERATURE - ? parseFloat(process.env.XCODEBUILDMCP_LLM_TEMPERATURE) - : undefined, -}); +const getLLMConfig = (): LLMConfig => { + const maxRaw = process.env.XCODEBUILDMCP_LLM_MAX_TOKENS; + const parsedMax = maxRaw !== undefined ? Number.parseInt(maxRaw, 10) : undefined; + const safeMax = Number.isFinite(parsedMax as number) && (parsedMax as number) > 0 ? (parsedMax as number) : 200; + + const tempRaw = process.env.XCODEBUILDMCP_LLM_TEMPERATURE; + const parsedTemp = tempRaw !== undefined ? Number.parseFloat(tempRaw) : undefined; + const safeTemp = Number.isFinite(parsedTemp as number) ? (parsedTemp as number) : undefined; + + return { maxTokens: safeMax, temperature: safeTemp }; +};src/mcp/tools/simulator/test_sim.ts (1)
61-69: Missing XOR validation for simulatorId vs simulatorName (declared in docs, not enforced)This can admit invalid states (neither or both), leading to ambiguous execution.
const testSimulatorSchema = baseSchema .refine((val) => val.projectPath !== undefined || val.workspacePath !== undefined, { message: 'Either projectPath or workspacePath is required.', }) .refine((val) => !(val.projectPath !== undefined && val.workspacePath !== undefined), { message: 'projectPath and workspacePath are mutually exclusive. Provide only one.', - }); + }) + .refine((val) => val.simulatorId !== undefined || val.simulatorName !== undefined, { + message: 'Either simulatorId or simulatorName is required.', + }) + .refine((val) => !(val.simulatorId !== undefined && val.simulatorName !== undefined), { + message: 'simulatorId and simulatorName are mutually exclusive. Provide only one.', + });src/mcp/tools/ui-testing/long_press.ts (1)
127-149: Bug: describe_ui warning state is never updated, causing perpetual warningsThe map describeUITimestamps is never written to in this module (and it’s not exported), so getCoordinateWarning will always return the “describe_ui has not been called yet” warning. This is likely a regression from moving logic around.
Consider one of these fixes:
A) Minimal fix (export state and a setter from this module):
-const describeUITimestamps = new Map<string, DescribeUISession>(); +export const describeUITimestamps = new Map<string, DescribeUISession>(); const DESCRIBE_UI_WARNING_TIMEOUT = 60000; // 60 seconds -function getCoordinateWarning(simulatorUuid: string): string | null { +export function recordDescribeUiCall(simulatorUuid: string): void { + describeUITimestamps.set(simulatorUuid, { timestamp: Date.now(), simulatorUuid }); +} + +export function getCoordinateWarning(simulatorUuid: string): string | null { const session = describeUITimestamps.get(simulatorUuid); if (!session) { return 'Warning: describe_ui has not been called yet. Consider using describe_ui for precise coordinates instead of guessing from screenshots.'; }Then, in describe_ui.ts (or wherever describe_ui is implemented), record the event after a successful run:
// after successful describe_ui completion import { recordDescribeUiCall } from './long_press.js'; // or better, from a shared module recordDescribeUiCall(simulatorUuid);B) Preferred: move this session tracking to a shared module (e.g., src/utils/ui-testing-session.ts) so multiple tools can read/write state without cross-tool coupling. I can draft that module and the call sites on request.
Would you like me to propose a small shared session module and patch describe_ui.ts to call it?
src/index.ts (1)
17-17: Remove static Sentry import in src/index.tsTo preserve the lazy-loading guarantees in
src/server/instrumentation.ts, drop the top-level Sentry init in your entrypoint:• File: src/index.ts
– Line 17:import './utils/sentry.js';Proposed diff:
- // Import Sentry instrumentation - import './utils/sentry.js';Rely on
maybeWrapWithSentryinsrc/server/bootstrap.tsto initialize Sentry at runtime. If you still need global logging hooks, wire them through the lazy loader or guard them byNODE_ENV/custom flags.src/test-utils/test-execution.ts (1)
165-246: Always clean up the temporary directory (leak risk on early failures)If executeXcodeBuildCommand throws before the xcresult parse block, tempDir isn’t removed. Move cleanup to a finally block and drop the duplicate rm calls in the inner branches.
Minimal refactor:
export async function handleTestLogic( params: { @@ - try { - // Create temporary directory for xcresult bundle - const tempDir = await mkdtemp(join(tmpdir(), 'xcodebuild-test-')); - const resultBundlePath = join(tempDir, 'TestResults.xcresult'); + let tempDir: string | undefined; + try { + // Create temporary directory for xcresult bundle + tempDir = await mkdtemp(join(tmpdir(), 'xcodebuild-test-')); + const resultBundlePath = join(tempDir, 'TestResults.xcresult'); @@ - // Clean up temporary directory - await rm(tempDir, { recursive: true, force: true }); - // Return combined result - preserve isError from testResult (test failures should be marked as errors) const combinedResponse: ToolResponse = { @@ - // Clean up temporary directory even if parsing fails - try { - await rm(tempDir, { recursive: true, force: true }); - } catch (cleanupError) { - log('warn', `Failed to clean up temporary directory: ${cleanupError}`); - } - return consolidateContentForClaudeCode(testResult); } } catch (error) { @@ - } + } finally { + if (tempDir) { + try { + await rm(tempDir, { recursive: true, force: true }); + } catch (cleanupError) { + log('warn', `Failed to clean up temporary directory: ${cleanupError}`); + } + } + }src/mcp/tools/ui-testing/touch.ts (1)
121-137: Unshared describe_ui session map causes constant warnings in touch.tsThe
describeUITimestampsMap defined insrc/mcp/tools/ui-testing/touch.ts(lines 121–137) is never updated, sogetCoordinateWarningwill always return a warning even afterdescribe_uiruns. To resolve this:• In
src/mcp/tools/ui-testing/describe_ui.ts
– Export the shared session API:
ts export { recordDescribeUICall, describeUITimestamps };• In each UI tool (touch.ts, tap.ts, swipe.ts, long_press.ts)
– Remove the localdescribeUITimestampsandgetCoordinateWarningdefinitions.
– Import and use the shareddescribeUITimestampsMap and callrecordDescribeUICall(simulatorUuid)in your handlers.This ensures all coordinate‐based tools reference the same, updated timestamp map rather than their own dead code copies.
src/mcp/tools/ui-testing/type_text.ts (1)
35-39: Type mismatch: optional AxeHelpers passed to a required parameteraxeHelpers is optional here, but executeAxeCommand expects a non-optional AxeHelpers (with a default). Passing AxeHelpers | undefined can cause a TypeScript error under strict mode.
Apply this diff to provide a default AxeHelpers consistent with other tools (e.g., describe_ui):
export async function type_textLogic( params: TypeTextParams, executor: CommandExecutor, - axeHelpers?: AxeHelpers, + axeHelpers: AxeHelpers = { + getAxePath, + getBundledAxeEnvironment, + createAxeNotAvailableResponse, + }, ): Promise<ToolResponse> {
🧹 Nitpick comments (42)
README.md (1)
364-379: Env var rename looks good; consider clarifying accepted valuesThe rename to XCODEBUILDMCP_SENTRY_DISABLED and examples are consistent with the codebase. As a minor doc improvement, consider explicitly noting that only the literal string "true" disables Sentry (case-sensitive), and any other value or unset enables it. This prevents confusion for users who may try values like 1/yes/TRUE.
src/utils/logger.ts (1)
22-22: Make env parsing more robust (case-insensitive; support 1/yes)If you want to align expectations with common patterns, accept multiple truthy values for disabling. Not required, but reduces surprises.
Example:
-const SENTRY_ENABLED = process.env.XCODEBUILDMCP_SENTRY_DISABLED !== 'true'; +const disabled = String(process.env.XCODEBUILDMCP_SENTRY_DISABLED ?? '').toLowerCase(); +const SENTRY_ENABLED = !(disabled === 'true' || disabled === '1' || disabled === 'yes');src/server/tool-registry.ts (1)
59-66: Include error stack when removal failsHelps diagnosis without exposing PII beyond what’s already in the Error.
Apply this diff:
- } catch (error) { - log('error', `❌ Failed to remove tool ${name}: ${error}`); + } catch (error) { + const details = error instanceof Error ? error.stack ?? error.message : String(error); + log('error', `❌ Failed to remove tool ${name}: ${details}`);src/utils/system-info.ts (2)
20-24: Consider adding timeouts to execSync calls to avoid potential hangsxcodebuild/xcrun calls should be near-instant, but misconfigured environments can hang. A small timeout improves robustness without changing behavior.
Apply this diff:
- const xcodebuildOutput = execSync('xcodebuild -version', { encoding: 'utf8' }).trim(); + const xcodebuildOutput = execSync('xcodebuild -version', { encoding: 'utf8', timeout: 2000 }).trim(); - const path = execSync('xcode-select -p', { encoding: 'utf8' }).trim(); - const selectedXcode = execSync('xcrun --find xcodebuild', { encoding: 'utf8' }).trim(); + const path = execSync('xcode-select -p', { encoding: 'utf8', timeout: 2000 }).trim(); + const selectedXcode = execSync('xcrun --find xcodebuild', { encoding: 'utf8', timeout: 2000 }).trim();
68-73: Harden binary detection against shell injection and improve cross-platform supportUsing execSync with a string (which spawns a shell) is fragile. Prefer execFileSync with explicit args and support Windows (“where”) to reduce false negatives and avoid injection when the input is not controlled.
Apply this diff:
-import { execSync } from 'child_process'; +import { execSync, execFileSync } from 'child_process'; @@ export function checkBinaryAvailability(binary: string): { available: boolean; version?: string } { try { - execSync(`which ${binary}`, { stdio: 'ignore' }); + if (process.platform === 'win32') { + execFileSync('where', [binary], { stdio: 'ignore' }); + } else { + execFileSync('which', [binary], { stdio: 'ignore' }); + } } catch { return { available: false }; } let version: string | undefined; - const versionCommands: Record<string, string> = { - axe: 'axe --version', - mise: 'mise --version', - }; + const versionArgsMap: Record<string, string[]> = { + axe: ['--version'], + mise: ['--version'], + }; - if (binary in versionCommands) { + if (binary in versionArgsMap) { try { - version = execSync(versionCommands[binary], { - encoding: 'utf8', - stdio: ['ignore', 'pipe', 'ignore'], - }).trim(); + version = execFileSync(binary, versionArgsMap[binary], { + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'ignore'], + }).trim(); } catch { // Version command failed, but binary exists } }Also applies to: 75-87
src/utils/sentry.ts (2)
12-30: Guard Sentry initialization via the standardized env varGiven the new XCODEBUILDMCP_SENTRY_DISABLED flag and the intent to initialize Sentry only in production paths, add a local guard to prevent accidental init if this file is imported elsewhere.
Apply this diff:
-Sentry.init({ +const SENTRY_ENABLED = process.env.XCODEBUILDMCP_SENTRY_DISABLED !== 'true'; + +if (SENTRY_ENABLED) Sentry.init({ dsn: process.env.SENTRY_DSN ?? 'https://798607831167c7b9fe2f2912f5d3c665@o4509258288332800.ingest.de.sentry.io/4509258293837904', @@ - tracesSampleRate: 1.0, + tracesSampleRate: 1.0, });And wrap the tag/context setup at the bottom similarly:
-const axeAvailable = checkBinaryAvailability('axe'); +if (SENTRY_ENABLED) { + const axeAvailable = checkBinaryAvailability('axe'); // ...rest of setup... -} + Sentry.setTags(tags); +}Please verify the server/instrumentation layer doesn’t already provide an equivalent guard to avoid double checks.
37-49: Move large environment data from tags to context to avoid tag truncation and PII leaksSentry tags are truncated and intended for faceting. PATH, HOME, USER, etc., are better in contexts/extras. Keep a minimal set of tags; move the full map to a context.
Apply this diff:
-const tags: Record<string, string> = { +const tags: Record<string, string> = { nodeVersion: process.version, platform: process.platform, arch: process.arch, axeAvailable: axeAvailable.available ? 'true' : 'false', axeVersion: axeAvailable.version ?? 'Unknown', miseAvailable: miseAvailable.available ? 'true' : 'false', miseVersion: miseAvailable.version ?? 'Unknown', - ...Object.fromEntries(Object.entries(envVars).map(([k, v]) => [`env_${k}`, v ?? ''])), xcodeVersion: xcodeInfo.version ?? 'Unknown', xcodePath: xcodeInfo.path ?? 'Unknown', }; Sentry.setTags(tags); +Sentry.setContext('environment', envVars);Also applies to: 51-51
src/utils/axe-command.ts (2)
60-61: Bundled AXe detection is brittle; rely on an explicit signal instead of string comparisonUsing axeBinary !== 'axe' to infer “bundled” will misclassify absolute paths to a system-installed axe (e.g., /usr/local/bin/axe). Prefer returning an isBundled boolean from the helper.
Proposed API change:
- Update getAxePath() to return { path: string, isBundled: boolean }.
- Consume it here to select the environment only when isBundled is true.
Example diff (assuming helper returns an object):
- const axeBinary = axeHelpers.getAxePath(); + const axeInfo = axeHelpers.getAxePath(); + const axeBinary = typeof axeInfo === 'string' ? axeInfo : axeInfo?.path; if (!axeBinary) { throw new DependencyError('AXe binary not found'); } @@ - const axeEnv = axeBinary !== 'axe' ? axeHelpers.getBundledAxeEnvironment() : undefined; + const isBundled = typeof axeInfo === 'object' ? axeInfo.isBundled : axeBinary !== 'axe'; + const axeEnv = isBundled ? axeHelpers.getBundledAxeEnvironment() : undefined;I can propagate the type change and usages if you want.
43-44: Nit: unify the “AXe” label casingMinor consistency tweak: use a single casing (e.g., “[AXE]”) across logs and docs.
- logPrefix: string = '[AXe]', + logPrefix: string = '[AXE]',docs/TOOLS.md (2)
41-43: Fix grammar for stop_app_sim entrySame stray “or” pattern as above.
-- `stop_app_sim` - Stops an app running in an iOS simulator by UUID or name. or stop_app_sim({ simulatorName: "iPhone 16", bundleId: "com.example.MyApp" }) +- `stop_app_sim` - Stops an app running in an iOS simulator by UUID or name. Example: stop_app_sim({ simulatorName: 'iPhone 16', bundleId: 'com.example.MyApp' })
124-127: Bullet list formatting is off; break items onto separate linesLanguageTool flags likely stem from collapsed list items. Split them to one-per-line.
-- **Total Tools**: 59 canonical tools + 22 re-exports = 81 total -- **Workflow Groups**: 12 -- **Analysis Method**: Static AST parsing with TypeScript compiler API +- **Total Tools**: 59 canonical tools + 22 re-exports = 81 total +- **Workflow Groups**: 12 +- **Analysis Method**: Static AST parsing with TypeScript compiler APIAlso applies to: 130-130
src/mcp/tools/discovery/discover_tools.ts (2)
6-11: Prefer importing from the server barrel for stability and simpler pathingThis keeps import paths resilient to internal file moves and matches the PR’s new server barrel pattern.
-import { - enableWorkflows, - getAvailableWorkflows, - generateWorkflowDescriptions, -} from '../../../server/dynamic-tools.js'; +import { + enableWorkflows, + getAvailableWorkflows, + generateWorkflowDescriptions, +} from '../../../server/index.js';
270-283: Enforce “EXACTLY ONE” workflow selection to align with the promptCurrently multiple selections are allowed if the LLM returns >1. Either constrain to one or reject with a helpful message.
selectedWorkflows = parsedResponse; // Validate that all selected workflows are valid const validWorkflows = selectedWorkflows.filter((workflow) => workflowNames.includes(workflow), ); if (validWorkflows.length !== selectedWorkflows.length) { const invalidWorkflows = selectedWorkflows.filter( (workflow) => !workflowNames.includes(workflow), ); log('warn', `LLM selected invalid workflows: ${invalidWorkflows.join(', ')}`); selectedWorkflows = validWorkflows; } + + // Enforce the UX contract: exactly one workflow + if (selectedWorkflows.length > 1) { + log('warn', `LLM returned multiple workflows; selecting the first: ${selectedWorkflows[0]}`); + selectedWorkflows = [selectedWorkflows[0]]; + }src/mcp/tools/doctor/doctor.ts (1)
221-223: Sentry env var rename LGTM; make boolean parsing more robustSupport common truthy forms ('true'/'1'/'yes') and different casings to reduce surprises.
- `- Sentry enabled: ${doctorInfo.environmentVariables.XCODEBUILDMCP_SENTRY_DISABLED !== 'true' ? '✅ Yes' : '❌ No'}`, + `- Sentry enabled: ${ + /^(true|1|yes)$/i.test(String(doctorInfo.environmentVariables.XCODEBUILDMCP_SENTRY_DISABLED)) + ? '❌ No' + : '✅ Yes' + }`,src/mcp/tools/simulator/test_sim.ts (1)
79-83: Use consistent log level key: 'warn' instead of 'warning'Logger elsewhere uses 'warn'. Using 'warning' may drop severity handling/filtering.
- log( - 'warning', - `useLatestOS parameter is ignored when using simulatorId (UUID implies exact device/OS)`, - ); + log('warn', 'useLatestOS parameter is ignored when using simulatorId (UUID implies exact device/OS)');src/server/dynamic-tools.ts (1)
153-163: Minor typing improvement opportunity (optional)Avoid broad casts by tightening the ToolRegistration config type to accept ZodTypeAny (or the plugin meta’s exact schema type), removing the need for
as unknown.If the registration API can be typed to accept
inputSchema: ZodTypeAny | undefined, you can drop:inputSchema: tool.config.inputSchema as unknownand keep the actual schema type for downstream tooling/validation.
src/mcp/tools/ui-testing/tap.ts (1)
41-57: “Shared across UI tools” map isn’t actually shared across modulesThis local Map won’t reflect calls made from describe_ui.ts. Consider moving session state to a shared module or a well-namespaced global.
Two lightweight options:
- Shared module: create utils/ui-session.ts that exports a singleton Map and import it from all UI tools.
- Global: use a namespaced global to share across modules:
// in a shared module, then import from all UI tools export const describeUITimestamps = ((globalThis as any).__xcodebuildmcpDescribeUITimestamps as Map<string, { timestamp: number }>) ?? ((globalThis as any).__xcodebuildmcpDescribeUITimestamps = new Map<string, { timestamp: number }>());Happy to draft the shared module and update describe_ui.ts and others.
src/mcp/tools/ui-testing/key_press.ts (1)
30-34: Deduplicate AxeHelpers by importing the shared typeThis interface is re-declared here; import the shared type to keep things DRY and consistent with other files (e.g., type_text.ts).
Apply this minimal diff to remove the local interface:
-export interface AxeHelpers { - getAxePath: () => string | null; - getBundledAxeEnvironment: () => Record<string, string>; - createAxeNotAvailableResponse: () => ToolResponse; -}And add this import near the top (no diff here since it’s outside the selected range):
import type { AxeHelpers } from '../../../utils/axe-command.js';src/mcp/tools/ui-testing/long_press.ts (2)
38-42: Deduplicate AxeHelpers by importing the shared typeSame as other tools: import the AxeHelpers type from utils instead of redefining it.
Apply this diff to remove the local interface:
-export interface AxeHelpers { - getAxePath: () => string | null; - getBundledAxeEnvironment: () => Record<string, string>; - createAxeNotAvailableResponse: () => ToolResponse; -}Add this import at the top:
import type { AxeHelpers } from '../../../utils/axe-command.js';
58-59: Minor: unnecessary Number() coercionduration is already validated as a number by zod; Number(duration) is redundant.
-const delayInSeconds = Number(duration) / 1000; // Convert ms to seconds +const delayInSeconds = duration / 1000; // Convert ms to secondssrc/mcp/tools/ui-testing/button.ts (1)
36-40: Import AxeHelpers type rather than redefining itKeep AxeHelpers canonical by importing the type from utils to avoid drift across files.
Remove the local interface:
-export interface AxeHelpers { - getAxePath: () => string | null; - getBundledAxeEnvironment: () => Record<string, string>; - createAxeNotAvailableResponse: () => ToolResponse; -}Add this import at the top:
import type { AxeHelpers } from '../../../utils/axe-command.js';src/mcp/tools/ui-testing/key_sequence.ts (1)
36-41: Unify AxeHelpers type by importing it from utils (avoid local duplication).Defining AxeHelpers locally is structurally fine, but it duplicates the contract that already exists in the utils layer. Importing the type keeps the contract single-sourced and prevents drift.
Apply this diff to remove the local interface:
-export interface AxeHelpers { - getAxePath: () => string | null; - getBundledAxeEnvironment: () => Record<string, string>; - createAxeNotAvailableResponse: () => ToolResponse; -}Add this type-only import (outside the selected range):
// near other imports import type { AxeHelpers } from '../../../utils/axe-command.js';src/mcp/tools/ui-testing/swipe.ts (1)
89-93: Include all timing options in the user-facing message (nit).Only duration is reflected in optionsText; delta, preDelay, and postDelay are ignored in the message even when provided. Consider reflecting them for clarity.
- const optionsText = duration ? ` duration=${duration}s` : ''; + const parts: string[] = []; + if (duration !== undefined) parts.push(`duration=${duration}s`); + if (delta !== undefined) parts.push(`delta=${delta}`); + if (preDelay !== undefined) parts.push(`preDelay=${preDelay}ms`); + if (postDelay !== undefined) parts.push(`postDelay=${postDelay}ms`); + const optionsText = parts.length ? ` ${parts.join(' ')}` : '';src/mcp/tools/ui-testing/gesture.ts (3)
12-19: Consistent typing: import shared AxeHelpers type instead of redefining locallyTo avoid type drift across tools and centralize the contract, prefer importing the shared AxeHelpers type from the axe-command module rather than redefining it here.
Apply this addition to your imports:
+import type { AxeHelpers } from '../../../utils/axe-command.js';
85-90: Remove locally duplicated AxeHelpers interfaceThis local interface duplicates the source of truth from utils/axe-command.ts. Import the type instead (as suggested above) to keep things consistent across files.
-export interface AxeHelpers { - getAxePath: () => string | null; - getBundledAxeEnvironment: () => Record<string, string>; - createAxeNotAvailableResponse: () => ToolResponse; -}
139-143: Improve error log message for unknown/error typesInterpolating the raw error can result in “[object Object]”. This aligns with touch.ts and improves observability.
- log('error', `${LOG_PREFIX}/${toolName}: Failed - ${error}`); + log( + 'error', + `${LOG_PREFIX}/${toolName}: Failed - ${ + error instanceof Error ? error.message : String(error) + }`, + );src/test-utils/test-execution.ts (2)
57-72: Prefer execFile/spawn over shell-quoted exec for robustnessexec with a composed string risks quoting issues. Using execFile (or spawn) with arguments is safer and avoids shell interpretation.
Example replacement:
import { execFile } from 'child_process'; const execFileAsync = promisify(execFile); const { stdout } = await execFileAsync('xcrun', [ 'xcresulttool', 'get', 'test-results', 'summary', '--path', resultBundlePath, ]);
12-15: Prefer static import for stat (consistency and minor perf win)You already import from fs/promises. Pull stat in statically and drop the dynamic import.
-import { mkdtemp, rm } from 'fs/promises'; +import { mkdtemp, rm, stat } from 'fs/promises';- try { - const { stat } = await import('fs/promises'); - await stat(resultBundlePath); - log('info', `xcresult bundle exists at: ${resultBundlePath}`); - } catch { + try { + await stat(resultBundlePath); + log('info', `xcresult bundle exists at: ${resultBundlePath}`); + } catch { log('warn', `xcresult bundle does not exist at: ${resultBundlePath}`); throw new Error(`xcresult bundle not found at ${resultBundlePath}`); }Also applies to: 198-201
src/utils/index.ts (1)
26-28: Consider re-exporting AxeHelpers type for consumersMany tools/types now depend on AxeHelpers. Re-exporting it from index helps avoid deep import paths and improves consistency.
export type { ToolResponse } from '../types/common.js'; +export type { AxeHelpers } from './axe-command.js';src/mcp/tools/ui-testing/touch.ts (1)
88-90: Honor injected AxeHelpers for not-available responseUse the injected helpers when present to keep behavior overridable in tests and consistent with gesture.ts.
- return createAxeNotAvailableResponse(); + return axeHelpers?.createAxeNotAvailableResponse() ?? createAxeNotAvailableResponse();src/server/bootstrap.ts (4)
43-46: Coerce env to boolean for clarity and correctnessprocess.env.SENTRY_DSN is a string; relying on its truthiness works but is brittle. Explicitly coerce to boolean for readability and to avoid accidental non-empty string issues.
Apply this diff:
- const shouldInstrument = - options.instrumentation === 'sentry' || - (options.instrumentation !== 'none' && process.env.SENTRY_DSN); + const shouldInstrument = + options.instrumentation === 'sentry' || + (options.instrumentation !== 'none' && Boolean(process.env.SENTRY_DSN));
39-41: Log the effective version used by the server (respect deps.version)When a custom version is passed via deps.version, the log still uses the imported version, which can be misleading.
Apply this diff:
// Create base server const baseServer = createBaseServer(deps); + const effectiveVersion = deps.version ?? version;
51-51: Use the effective version in the logThis keeps the log consistent with the version actually applied to the server instance.
Apply this diff:
- log('info', `Base server created without instrumentation (version ${version})`); + log('info', `Base server created without instrumentation (version ${effectiveVersion})`);
48-50: Remove unnecessary await in returnNo try/catch around the awaited call; returning the promise directly is sufficient.
Apply this diff:
- // Apply Sentry instrumentation - return await maybeWrapWithSentry(baseServer, options.sentry); + // Apply Sentry instrumentation + return maybeWrapWithSentry(baseServer, options.sentry);src/mcp/tools/ui-testing/describe_ui.ts (2)
36-37: Type the session-tracking map for better safetyProvide explicit generics to avoid implicit any and make downstream usage safer.
Apply this diff:
-const describeUITimestamps = new Map(); +const describeUITimestamps = new Map< + string, + { timestamp: number; simulatorUuid: string } +>();
63-72: Avoid the cast by tightening executeAxeCommand’s typesCasting the result to string works, but we can eliminate it by adding overloaded signatures to executeAxeCommand so TypeScript knows that returnOutput = true yields a Promise.
If you’d like, I can open a follow-up PR to adjust the shared utility. Example overloads (in src/utils/axe-command.ts):
// Overloads export function executeAxeCommand( commandArgs: string[], simulatorUuid: string, commandName: string, executor?: CommandExecutor, axeHelpers?: AxeHelpers, logPrefix?: string, returnOutput?: false, ): Promise<void>; export function executeAxeCommand( commandArgs: string[], simulatorUuid: string, commandName: string, executor: CommandExecutor | undefined, axeHelpers: AxeHelpers | undefined, logPrefix: string | undefined, returnOutput: true, ): Promise<string>; // Implementation (existing) remains unchangedThis change would allow removing the “as string” here while keeping the same call-site shape.
src/test-utils/mock-executors.ts (3)
23-28: Expose call tracking in the return type for better DX in testsTests will likely want to access .calls and .lastCall, but the current return type (CommandExecutor) doesn’t surface those accessors. Add a dedicated MockCommandExecutor type that extends the function type with these properties.
Apply this diff:
export type CommandExecutor = ( command: string[], description?: string, allowFailure?: boolean, ) => Promise<MockCommandResult>; + +// Function type with call-tracking accessors for tests +export type MockCommandExecutor = CommandExecutor & { + readonly calls: Array<{ command: string[]; description?: string; allowFailure?: boolean }>; + readonly lastCall?: { command: string[]; description?: string; allowFailure?: boolean }; +};
55-56: Return the enriched executor type from createMockExecutorThis exposes the accessor properties with proper typing.
Apply this diff:
-): CommandExecutor { +): MockCommandExecutor {
98-99: Cast the returned function to the enriched typePreserves the function signature while surfacing the accessors to TypeScript.
Apply this diff:
- return executor; + return executor as MockCommandExecutor;src/server/generated-plugins.ts (3)
6-47: Load tool modules in parallel to reduce workflow bootstrap latencyEach loader currently awaits imports sequentially, incurring N round-trips per workflow. Switching to Promise.all parallelizes the I/O and noticeably speeds up registry hydration. Example refactor for the device loader (apply this pattern in the generator so all workflows benefit):
- device: async () => { - const { workflow } = await import('../mcp/tools/device/index.js'); - const tool_0 = await import('../mcp/tools/device/build_device.js').then((m) => m.default); - const tool_1 = await import('../mcp/tools/device/clean.js').then((m) => m.default); - const tool_2 = await import('../mcp/tools/device/discover_projs.js').then((m) => m.default); - const tool_3 = await import('../mcp/tools/device/get_app_bundle_id.js').then((m) => m.default); - const tool_4 = await import('../mcp/tools/device/get_device_app_path.js').then( - (m) => m.default, - ); - const tool_5 = await import('../mcp/tools/device/install_app_device.js').then((m) => m.default); - const tool_6 = await import('../mcp/tools/device/launch_app_device.js').then((m) => m.default); - const tool_7 = await import('../mcp/tools/device/list_devices.js').then((m) => m.default); - const tool_8 = await import('../mcp/tools/device/list_schemes.js').then((m) => m.default); - const tool_9 = await import('../mcp/tools/device/show_build_settings.js').then( - (m) => m.default, - ); - const tool_10 = await import('../mcp/tools/device/start_device_log_cap.js').then( - (m) => m.default, - ); - const tool_11 = await import('../mcp/tools/device/stop_app_device.js').then((m) => m.default); - const tool_12 = await import('../mcp/tools/device/stop_device_log_cap.js').then( - (m) => m.default, - ); - const tool_13 = await import('../mcp/tools/device/test_device.js').then((m) => m.default); - - return { - workflow, - build_device: tool_0, - clean: tool_1, - discover_projs: tool_2, - get_app_bundle_id: tool_3, - get_device_app_path: tool_4, - install_app_device: tool_5, - launch_app_device: tool_6, - list_devices: tool_7, - list_schemes: tool_8, - show_build_settings: tool_9, - start_device_log_cap: tool_10, - stop_app_device: tool_11, - stop_device_log_cap: tool_12, - test_device: tool_13, - }; - }, + device: async () => { + const [ + workflow, + tool_0, + tool_1, + tool_2, + tool_3, + tool_4, + tool_5, + tool_6, + tool_7, + tool_8, + tool_9, + tool_10, + tool_11, + tool_12, + tool_13, + ] = await Promise.all([ + import('../mcp/tools/device/index.js').then((m) => m.workflow), + import('../mcp/tools/device/build_device.js').then((m) => m.default), + import('../mcp/tools/device/clean.js').then((m) => m.default), + import('../mcp/tools/device/discover_projs.js').then((m) => m.default), + import('../mcp/tools/device/get_app_bundle_id.js').then((m) => m.default), + import('../mcp/tools/device/get_device_app_path.js').then((m) => m.default), + import('../mcp/tools/device/install_app_device.js').then((m) => m.default), + import('../mcp/tools/device/launch_app_device.js').then((m) => m.default), + import('../mcp/tools/device/list_devices.js').then((m) => m.default), + import('../mcp/tools/device/list_schemes.js').then((m) => m.default), + import('../mcp/tools/device/show_build_settings.js').then((m) => m.default), + import('../mcp/tools/device/start_device_log_cap.js').then((m) => m.default), + import('../mcp/tools/device/stop_app_device.js').then((m) => m.default), + import('../mcp/tools/device/stop_device_log_cap.js').then((m) => m.default), + import('../mcp/tools/device/test_device.js').then((m) => m.default), + ]); + + return { + workflow, + build_device: tool_0, + clean: tool_1, + discover_projs: tool_2, + get_app_bundle_id: tool_3, + get_device_app_path: tool_4, + install_app_device: tool_5, + launch_app_device: tool_6, + list_devices: tool_7, + list_schemes: tool_8, + show_build_settings: tool_9, + start_device_log_cap: tool_10, + stop_app_device: tool_11, + stop_device_log_cap: tool_12, + test_device: tool_13, + }; + },
312-312: Make WORKFLOW_LOADERS readonly to prevent accidental mutation and stabilize keyof inferenceSmall hardening that also preserves literal keys.
-export const WORKFLOW_LOADERS = { +export const WORKFLOW_LOADERS = { … -}; +} as const;
432-432: Optionally freeze WORKFLOW_METADATA with a const assertionPrevents accidental mutation and preserves literal types for downstream consumers.
export const WORKFLOW_METADATA = { … -}; +} as const;
📜 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 (36)
README.md(2 hunks)docs/TOOLS.md(5 hunks)eslint.config.js(1 hunks)src/core/plugin-registry.ts(1 hunks)src/index.ts(2 hunks)src/mcp/tools/discovery/discover_tools.ts(1 hunks)src/mcp/tools/doctor/__tests__/doctor.test.ts(2 hunks)src/mcp/tools/doctor/doctor.ts(1 hunks)src/mcp/tools/doctor/lib/doctor.deps.ts(4 hunks)src/mcp/tools/simulator/test_sim.ts(1 hunks)src/mcp/tools/ui-testing/button.ts(2 hunks)src/mcp/tools/ui-testing/describe_ui.ts(2 hunks)src/mcp/tools/ui-testing/gesture.ts(2 hunks)src/mcp/tools/ui-testing/key_press.ts(2 hunks)src/mcp/tools/ui-testing/key_sequence.ts(2 hunks)src/mcp/tools/ui-testing/long_press.ts(2 hunks)src/mcp/tools/ui-testing/swipe.ts(2 hunks)src/mcp/tools/ui-testing/tap.ts(1 hunks)src/mcp/tools/ui-testing/touch.ts(2 hunks)src/mcp/tools/ui-testing/type_text.ts(2 hunks)src/server/base-server.ts(1 hunks)src/server/bootstrap.ts(1 hunks)src/server/dynamic-tools.ts(1 hunks)src/server/generated-plugins.ts(1 hunks)src/server/index.ts(1 hunks)src/server/instrumentation.ts(1 hunks)src/server/server.ts(1 hunks)src/server/tool-registry.ts(1 hunks)src/test-utils/index.ts(1 hunks)src/test-utils/mock-executors.ts(1 hunks)src/test-utils/test-execution.ts(1 hunks)src/utils/axe-command.ts(1 hunks)src/utils/index.ts(1 hunks)src/utils/logger.ts(1 hunks)src/utils/sentry.ts(1 hunks)src/utils/system-info.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (16)
src/server/base-server.ts (1)
src/server/server.ts (1)
createBaseServer(14-14)
src/utils/system-info.ts (2)
src/mcp/tools/doctor/lib/doctor.deps.ts (3)
getXcodeInfo(113-135)getEnvironmentVariables(139-150)checkBinaryAvailability(101-109)src/mcp/tools/doctor/__tests__/doctor.test.ts (3)
getXcodeInfo(20-27)getEnvironmentVariables(30-43)checkBinaryAvailability(14-17)
src/server/bootstrap.ts (3)
src/server/instrumentation.ts (2)
SentryOptions(15-18)maybeWrapWithSentry(34-83)src/server/base-server.ts (2)
ServerDeps(15-18)createBaseServer(29-48)src/utils/logger.ts (1)
log(33-53)
src/server/instrumentation.ts (1)
src/utils/logger.ts (1)
log(33-53)
src/utils/axe-command.ts (6)
src/mcp/tools/ui-testing/button.ts (1)
AxeHelpers(36-40)src/mcp/tools/ui-testing/describe_ui.ts (1)
AxeHelpers(27-31)src/test-utils/mock-executors.ts (1)
CommandExecutor(23-27)src/utils/command.ts (1)
getDefaultCommandExecutor(232-243)src/utils/errors.ts (3)
DependencyError(127-136)AxeError(97-108)SystemError(57-66)src/utils/logger.ts (1)
log(33-53)
src/mcp/tools/ui-testing/key_press.ts (1)
src/utils/axe-command.ts (1)
executeAxeCommand(37-98)
src/mcp/tools/ui-testing/gesture.ts (1)
src/utils/axe-command.ts (1)
executeAxeCommand(37-98)
src/mcp/tools/ui-testing/key_sequence.ts (1)
src/utils/axe-command.ts (1)
executeAxeCommand(37-98)
src/mcp/tools/ui-testing/button.ts (1)
src/utils/axe-command.ts (1)
executeAxeCommand(37-98)
src/mcp/tools/ui-testing/long_press.ts (1)
src/utils/axe-command.ts (1)
executeAxeCommand(37-98)
src/mcp/tools/ui-testing/swipe.ts (1)
src/utils/axe-command.ts (1)
executeAxeCommand(37-98)
src/index.ts (1)
src/server/bootstrap.ts (1)
createServer(35-54)
src/mcp/tools/doctor/lib/doctor.deps.ts (2)
src/mcp/tools/doctor/__tests__/doctor.test.ts (3)
checkBinaryAvailability(14-17)getXcodeInfo(20-27)getEnvironmentVariables(30-43)src/utils/system-info.ts (3)
checkBinaryAvailability(68-93)getXcodeInfo(13-34)getEnvironmentVariables(39-63)
src/mcp/tools/ui-testing/describe_ui.ts (1)
src/utils/axe-command.ts (1)
executeAxeCommand(37-98)
src/mcp/tools/ui-testing/touch.ts (1)
src/utils/axe-command.ts (1)
executeAxeCommand(37-98)
src/mcp/tools/ui-testing/type_text.ts (1)
src/utils/axe-command.ts (1)
executeAxeCommand(37-98)
🪛 LanguageTool
docs/TOOLS.md
[grammar] ~38-~38: There might be a mistake here.
Context: ...pp in an iOS simulator by UUID or name. If simulator window isn't visible, use ope...
(QB_NEW_EN)
[grammar] ~124-~124: There might be a mistake here.
Context: ...nonical tools + 22 re-exports = 81 total - Workflow Groups: 12 - **Analysis Metho...
(QB_NEW_EN)
[grammar] ~125-~125: There might be a mistake here.
Context: ...rts = 81 total - Workflow Groups: 12 - Analysis Method: Static AST parsing wi...
(QB_NEW_EN)
🔇 Additional comments (44)
src/server/tool-registry.ts (2)
4-4: Import path update aligns with utils relocationGood move consolidating logging under utils; keeps server modules lean.
176-178: Intentional non-tracking?registerSelectedWorkflows uses server.registerTools without tracking, unlike discovery. If workflows may be reloaded/unloaded dynamically later, consider using registerAndTrackTools here as well. If static by design, please confirm and ignore.
src/core/plugin-registry.ts (1)
1-1: Type import relocation to server namespace makes senseCentralizing plugin types under server/plugin-types.js matches the new architecture; no runtime changes. LGTM.
eslint.config.js (1)
9-10: Broader ignore pattern for generated files is appropriateThis will cover new generated artifacts like src/server/generated-plugins.ts and future generated files across src/. Good balance between safety and convenience.
src/utils/system-info.ts (1)
39-63: LGTM: environment capture is focused and consistentGood balance between explicit keys and prefixed XCODEBUILDMCP_* enumeration. Returning empty strings avoids undefined surprises for downstream consumers.
src/utils/sentry.ts (1)
10-10: Nice: system-info helpers are centralizedImporting shared helpers reduces duplication and keeps Sentry wiring lean.
src/utils/axe-command.ts (1)
46-56: Overall: solid centralization and error mappingGood dependency injection points, standardized udid handling, and clear error taxonomy (DependencyError/AxeError/SystemError). This will reduce duplicated logic across tools.
Also applies to: 58-85, 86-97
src/mcp/tools/doctor/__tests__/doctor.test.ts (1)
40-41: Env var rename to XCODEBUILDMCP_SENTRY_DISABLED looks correctTests reflect the standardized Sentry opt-out variable. Aligns with logger/system-info changes.
Also applies to: 263-264
src/mcp/tools/simulator/test_sim.ts (1)
10-12: Import split looks right and aligns with the new architectureMoving handleTestLogic under test-utils and keeping log under utils matches the separation goals without altering behavior.
src/server/dynamic-tools.ts (1)
7-7: Import path update to server-local registry LGTMThis aligns dynamic-tools with the new server-scoped tool registry and avoids utils leakage.
src/mcp/tools/ui-testing/tap.ts (1)
6-13: Centralizing AXe command execution is a solid winConverging on the shared executeAxeCommand removes duplication and keeps behavior consistent across UI tools.
src/test-utils/index.ts (2)
1-7: Clear, test-only surface is well-definedGood documentation and separation from server code. This should help prevent the previous test/production coupling issues.
10-13: No re-export required for createMockToolResponse
Verified thatcreateMockToolResponseis defined insrc/test-utils/mock-executors.tsand is not imported from thetest-utilsbarrel anywhere in the repo. Omitting it fromsrc/test-utils/index.tswill not break existing tests.If you do want to expose it on the public test-utils surface, you can add:
-export { createMockExecutor, createMockFileSystemExecutor } from './mock-executors.js'; +export { createMockExecutor, createMockFileSystemExecutor, createMockToolResponse } from './mock-executors.js';src/mcp/tools/ui-testing/key_press.ts (1)
56-58: Centralized AXe execution usage looks goodDelegating to executeAxeCommand with a log prefix is consistent with the new utilities and reduces duplication.
src/mcp/tools/ui-testing/long_press.ts (1)
76-78: Good migration to shared AXe executorUsing executeAxeCommand with LOG_PREFIX aligns with the new centralized utility.
src/server/index.ts (2)
9-20: Barrel and boundary look goodThis cleanly scopes server-only exports and should help avoid test-time linking issues.
4-7: No server imports detected in tests – CI linking issue addressedI scanned all
*.test.*,*.spec.*, and__tests__files for imports fromsrc/server/and found none. Tests no longer reference server modules, so the “module is already linked” CI issue should be resolved.src/mcp/tools/ui-testing/button.ts (1)
62-64: Good adoption of the centralized executeAxeCommandThis removes duplicate command execution logic and standardizes logging/error handling.
src/mcp/tools/ui-testing/key_sequence.ts (2)
16-17: Good move: centralized AXe execution via shared utility.Switching to the shared executeAxeCommand removes duplicate logic and aligns this tool with the new utils layer.
65-73: Correct usage of executeAxeCommand with extended signature.The updated call passes the log prefix and aligns with the shared util’s signature. Looks good.
src/mcp/tools/ui-testing/swipe.ts (2)
16-17: Centralizing AXe execution looks good.Importing executeAxeCommand from the utils layer removes per-tool implementations and helps maintain DRYness.
95-97: Accurate adoption of the shared executor API.Passing LOG_PREFIX ensures consistent log formatting across tools. No issues found.
src/index.ts (2)
41-41: Import path update to server/tool-registry is consistent with new architecture.Matches the separation between server/test/utils modules. Looks good.
65-65: No additional createServer calls found
Only the single call in src/index.ts (the one already awaiting the Promise) and the definition in src/server/bootstrap.ts exist. No other call sites require updating.src/server/instrumentation.ts (1)
44-54: Idempotency guard looks good.The symbol-based guard prevents double wrapping and avoids accidental re-instrumentation. Nice touch.
src/server/base-server.ts (1)
29-48: Solid, side-effect-free server constructor.This clean base aligns with the new architecture and is safe for tests. Capabilities configuration looks correct.
src/mcp/tools/ui-testing/gesture.ts (1)
129-136: LGTM: Centralized AXe execution call is correctDelegating to executeAxeCommand with simulator UDID, command name, injected executor/helpers, and a log prefix matches the new shared utility signature. ReturnOutput isn’t needed here, so omitting it is appropriate.
src/server/server.ts (1)
10-14: LGTM: Backward-compatibility re-exports are clean and minimalThe wrapper clearly re-exports the new bootstrap/base APIs and documents the legacy layer. Using explicit .js extensions is consistent with ESM builds.
src/test-utils/test-execution.ts (2)
2-10: LGTM: Clear separation of concerns for test utilitiesHeader makes scope explicit (no server/MCP/Sentry). This helps avoid the prior “module already linked” contamination in CI.
17-23: LGTM: Imports now correctly source from pure utilsAll imports come from ../utils/* and types/common, fitting the new boundaries.
src/utils/index.ts (3)
1-7: LGTM: Pure utilities boundary is clearHeader communicates the contract. This helps keep server/test code out of the utils surface.
16-16: LGTM: Re-exporting axe-command consolidates the new shared runnerThis enables each UI tool to use the same execution path and error model.
21-22: LGTM: system-info exposure is useful for doctor/instrumentation without side effectsNo server import leakage here.
src/mcp/tools/ui-testing/touch.ts (2)
16-21: LGTM: Centralizing AXe execution and importing shared AxeHelpersThis aligns with the new axe-command utility and keeps types consistent.
71-73: LGTM: executeAxeCommand invocation matches the shared signaturePassing LOG_PREFIX improves log readability and parity with other tools.
src/server/bootstrap.ts (1)
35-54: Overall bootstrap flow looks solidAsync server creation, clean separation between base server and optional instrumentation, and stdio startup are well-structured. The env-driven decision to enable Sentry aligns with the PR goals.
src/mcp/tools/ui-testing/describe_ui.ts (1)
61-94: Migration to shared executor is cleanGood use of the centralized AXe executor with LOG_PREFIX and returnOutput=true. Error handling and user-facing messaging remain consistent.
src/mcp/tools/ui-testing/type_text.ts (1)
51-55: Shared executor usage looks goodThe centralized call with LOG_PREFIX is consistent with the refactor, and the error mapping remains correct.
src/mcp/tools/doctor/lib/doctor.deps.ts (2)
99-136: Solid dependency wiring and use of centralized system-infoNice consolidation: checkBinaryAvailability, getXcodeInfo, and getEnvironmentVariables now use the shared utils, and you preserve xcrun version via executor. The binary check correctly short-circuits for bundled AXe.
187-193: Safer typing when extracting tool namesExplicitly typing the tool items and filtering falsy names is a good improvement that avoids undefined entries and keeps totals accurate.
src/test-utils/mock-executors.ts (1)
112-141: In-memory FS executor is straightforward and usefulRead/write/exists semantics are intuitive; rmdir prefix matching is sufficient for tests. No concerns.
src/server/generated-plugins.ts (3)
1-4: LGTM: Clear auto-generated banner and provenance commentHeader is clear and sets correct expectations for consumers/tools.
314-315: LGTM: WorkflowName derives directly from loader keysThis keeps the public name type in sync with the generated registry.
5-312: 🔎 Verify dynamic-imported modules export namedworkflowand default toolDynamic imports in
src/server/generated-plugins.tsassume:
- Each
src/mcp/tools/**/index.tsexports a namedworkflow- Each non-index tool file under
src/mcp/tools/**has a default exportMissing or mis-named exports will resolve to
undefinedat runtime. Please run a quick repo-wide check or manually verify:
- For every path imported in
generated-plugins.ts, confirm
src/mcp/tools/.../index.tscontainsexport const workflow,export function workflow, orexport { workflow }- The corresponding
src/mcp/tools/.../…tool… .tsfile hasexport default
| - `launch_app_sim` - Launches an app in an iOS simulator by UUID or name. If simulator window isn't visible, use open_sim() first. or launch_app_sim({ simulatorName: 'iPhone 16', bundleId: 'com.example.MyApp' }) | ||
| - `list_sims` - Lists available iOS simulators with their UUIDs. |
There was a problem hiding this comment.
Fix grammar and example formatting for launch_app_sim
There’s an extra “or” and a missing “Example:”.
Apply this diff:
-- `launch_app_sim` - Launches an app in an iOS simulator by UUID or name. If simulator window isn't visible, use open_sim() first. or launch_app_sim({ simulatorName: 'iPhone 16', bundleId: 'com.example.MyApp' })
+- `launch_app_sim` - Launches an app in an iOS simulator by UUID or name. If the simulator window isn't visible, use open_sim() first. Example: launch_app_sim({ simulatorName: 'iPhone 16', bundleId: 'com.example.MyApp' })📝 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.
| - `launch_app_sim` - Launches an app in an iOS simulator by UUID or name. If simulator window isn't visible, use open_sim() first. or launch_app_sim({ simulatorName: 'iPhone 16', bundleId: 'com.example.MyApp' }) | |
| - `list_sims` - Lists available iOS simulators with their UUIDs. | |
| - `launch_app_sim` - Launches an app in an iOS simulator by UUID or name. If the simulator window isn't visible, use open_sim() first. Example: launch_app_sim({ simulatorName: 'iPhone 16', bundleId: 'com.example.MyApp' }) | |
| - `list_sims` - Lists available iOS simulators with their UUIDs. |
🧰 Tools
🪛 LanguageTool
[grammar] ~38-~38: There might be a mistake here.
Context: ...pp in an iOS simulator by UUID or name. If simulator window isn't visible, use ope...
(QB_NEW_EN)
🤖 Prompt for AI Agents
In docs/TOOLS.md around lines 38 to 39, the description for `launch_app_sim`
contains an extra stray "or" and the example is missing the "Example:" label;
remove the extra "or" from the sentence and prepend "Example:" to the example
invocation so it reads like a labeled code example (e.g., Example:
`launch_app_sim({ simulatorName: 'iPhone 16', bundleId: 'com.example.MyApp'
})`), keeping `list_sims` unchanged.
| import { createAxeNotAvailableResponse } from '../../../utils/index.js'; | ||
| import { createTypedTool } from '../../../utils/typed-tool-factory.js'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Re-introduce helper imports for the default AxeHelpers
To support the default object above, import the helpers from the shared utils barrel.
Apply this diff:
-import { createAxeNotAvailableResponse } from '../../../utils/index.js';
+import {
+ createAxeNotAvailableResponse,
+ getAxePath,
+ getBundledAxeEnvironment,
+} from '../../../utils/index.js';📝 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.
| import { createAxeNotAvailableResponse } from '../../../utils/index.js'; | |
| import { createTypedTool } from '../../../utils/typed-tool-factory.js'; | |
| import { | |
| createAxeNotAvailableResponse, | |
| getAxePath, | |
| getBundledAxeEnvironment, | |
| } from '../../../utils/index.js'; | |
| import { createTypedTool } from '../../../utils/typed-tool-factory.js'; |
🤖 Prompt for AI Agents
In src/mcp/tools/ui-testing/type_text.ts around lines 21 to 22, the default
AxeHelpers object used later relies on helper exports from the shared utils
barrel but those imports were removed; re-import the helper(s) from
'../../../utils/index.js' (for example add an import for AxeHelpers or the
specific helper names used by the default object) so the default AxeHelpers
reference resolves correctly.
src/server/instrumentation.ts
Outdated
| // Check if DSN is available | ||
| const dsn = options.dsn ?? process.env.SENTRY_DSN; | ||
| if (!dsn) { | ||
| log('info', 'No Sentry DSN configured, skipping instrumentation'); | ||
| return server; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
DSN is checked but never used to initialize Sentry.
Sentry generally requires explicit initialization (Sentry.init({ dsn })) to function. Currently, dsn is only used for gating, not initialization.
You can initialize Sentry after the dynamic import:
const dsn = options.dsn ?? process.env.SENTRY_DSN;
if (!dsn) {
log('info', 'No Sentry DSN configured, skipping instrumentation');
return server;
}
try {
- // Lazy import Sentry to avoid module linking in tests
- log('info', 'Loading Sentry instrumentation...');
- const Sentry = await import('@sentry/node');
-
- // Wrap server with Sentry instrumentation
- const instrumentedServer = Sentry.wrapMcpServerWithSentry(server);
+ // Lazy import Sentry to avoid module linking in tests
+ log('info', 'Loading Sentry instrumentation...');
+ const mod = await import('@sentry/node');
+ // Handle CJS/ESM interop
+ const Sentry: any = (mod as any)?.default ?? mod;
+ if (typeof Sentry.init === 'function') {
+ Sentry.init({ dsn });
+ } else {
+ log('info', 'Sentry SDK missing init(); skipping instrumentation');
+ return server;
+ }
+
+ // Wrap server with Sentry MCP instrumentation if available
+ if (typeof Sentry.wrapMcpServerWithSentry !== 'function') {
+ log('info', 'wrapMcpServerWithSentry not found; continuing without MCP instrumentation');
+ return server;
+ }
+ const instrumentedServer = Sentry.wrapMcpServerWithSentry(server);📝 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.
| // Check if DSN is available | |
| const dsn = options.dsn ?? process.env.SENTRY_DSN; | |
| if (!dsn) { | |
| log('info', 'No Sentry DSN configured, skipping instrumentation'); | |
| return server; | |
| } | |
| // Check if DSN is available | |
| const dsn = options.dsn ?? process.env.SENTRY_DSN; | |
| if (!dsn) { | |
| log('info', 'No Sentry DSN configured, skipping instrumentation'); | |
| return server; | |
| } | |
| try { | |
| // Lazy import Sentry to avoid module linking in tests | |
| log('info', 'Loading Sentry instrumentation...'); | |
| const mod = await import('@sentry/node'); | |
| // Handle CJS/ESM interop | |
| const Sentry: any = (mod as any)?.default ?? mod; | |
| if (typeof Sentry.init === 'function') { | |
| Sentry.init({ dsn }); | |
| } else { | |
| log('info', 'Sentry SDK missing init(); skipping instrumentation'); | |
| return server; | |
| } | |
| // Wrap server with Sentry MCP instrumentation if available | |
| if (typeof Sentry.wrapMcpServerWithSentry !== 'function') { | |
| log('info', 'wrapMcpServerWithSentry not found; continuing without MCP instrumentation'); | |
| return server; | |
| } | |
| const instrumentedServer = Sentry.wrapMcpServerWithSentry(server); | |
| return instrumentedServer; | |
| } catch (err) { | |
| log('error', 'Error loading Sentry instrumentation', { error: err }); | |
| return server; | |
| } |
🤖 Prompt for AI Agents
In src/server/instrumentation.ts around lines 56 to 61, you currently gate on
the DSN but never initialize Sentry; after the dynamic import of Sentry perform
a Sentry.init call using the resolved dsn (e.g. Sentry.init({ dsn,
...sentryOptions })) so Sentry is actually configured. Ensure you use the
imported Sentry object from the dynamic import, merge any provided options
(tracing/sample rate/integrations) rather than hardcoding, and then proceed to
set up middleware/handlers as before so instrumentation is active.
src/server/instrumentation.ts
Outdated
| try { | ||
| // Lazy import Sentry to avoid module linking in tests | ||
| log('info', 'Loading Sentry instrumentation...'); | ||
| const Sentry = await import('@sentry/node'); | ||
|
|
||
| // Wrap server with Sentry instrumentation | ||
| const instrumentedServer = Sentry.wrapMcpServerWithSentry(server); | ||
|
|
||
| // Mark as wrapped to prevent double-wrapping | ||
| (instrumentedServer as unknown as Record<string | symbol, unknown>)[SENTRY_WRAPPED_SYMBOL] = | ||
| true; | ||
|
|
||
| log('info', 'Server wrapped with Sentry MCP instrumentation'); | ||
| return instrumentedServer; | ||
| } catch (error) { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure @sentry/node interop and wrapper API actually exist.
Dynamic import of @sentry/node likely yields a CJS default export; accessing named properties directly (Sentry.wrapMcpServerWithSentry) may fail and silently skip instrumentation via the catch block. Additionally, the existence of wrapMcpServerWithSentry is not standard in @sentry/node.
-
Query the web for the presence of wrapMcpServerWithSentry and Sentry auto-initialization:
-
If the wrapper is custom or provided by another module, consider importing that module instead of @sentry/node, or implement the wrapping locally.
If you confirm there is no such function, we should either:
- Implement the wrapping in this repo, or
- Limit to Sentry.init + log/event capture without MCP-specific wrapping.
🌐 Web query:
Does @sentry/node export a function named wrapMcpServerWithSentry? Also, can @sentry/node work without calling Sentry.init if only SENTRY_DSN is set?
💡 Result:
@sentry/node does not export a function named wrapMcpServerWithSentry. This function is referenced as an export request for the Cloudflare SDK specifically[10], and does not appear in the documented exports or API of @sentry/node[1][6][9].
@sentry/node requires calling Sentry.init() in your code; it does not automatically initialize itself simply because a SENTRY_DSN environment variable is set—just setting the environment variable is not sufficient. You must explicitly call Sentry.init({ dsn: process.env.SENTRY_DSN }) or equivalent in your application for Sentry to operate[1][2][5][8]. Without this call, even if SENTRY_DSN is set, Sentry error tracking will not function.
Key details:
-
No wrapMcpServerWithSentry function in @sentry/node: The only reference to that function is in a request for the Cloudflare-related Sentry SDK, not @sentry/node[10]. Neither the docs nor typical Node.js setups mention or export it[1][6][9].
-
Initialization requirement:
-
You must call
Sentry.init()to activate Sentry features. The recommended usage is:import * as Sentry from "@sentry/node"; Sentry.init({ dsn: process.env.SENTRY_DSN });
-
The presence of
SENTRY_DSNin your environment has no effect unlessSentry.init()is explicitly called in the codebase[2][5][8].
-
-
Explicit dsn passing: The DSN (Data Source Name) can be set via environment variable, but you still need to pass it as an option to
init()or haveprocess.env.SENTRY_DSNreferenced[2][5][8]. -
Other integrations/usage: Once
Sentry.init()is called, you can use the helper methods (Sentry.captureException, etc.) throughout your application, and further calls toinit()reconfigure the instance, but are not strictly required in every file[1][6].
Conclusion:
- wrapMcpServerWithSentry is not part of @sentry/node.
- @sentry/node cannot be used without a call to
Sentry.init(), regardless of SENTRY_DSN being set in the environment.
If you need functionality similar to wrapMcpServerWithSentry, you will need to look at Sentry’s Cloudflare SDK or file an export request, as discussed in the referenced issue[10].
Citations:
- 1: https://docs.sentry.io/platforms/javascript/guides/node/
- 2: https://www.freecodecamp.org/news/how-to-add-sentry-to-your-node-js-project-with-typescript/
- 3: Replacement for Handlers.requestHandler, Handlers.errorHandler, etc. sentry-javascript#12007
- 4: https://www.jsdocs.io/package/@sentry/core
- 5: https://docs.sentry.io/platforms/javascript/configuration/options/
- 6: https://forum.sentry.io/t/do-i-need-to-re-initialize-the-sentry-instance-when-using-it-in-other-files-node-typescript/11500
- 7: https://www.youtube.com/watch?v=bsiwhthPfNA
- 8: https://trigger.dev/docs/guides/examples/sentry-error-tracking
- 9: https://community.wappler.io/t/how-to-install-sentry-on-the-back-end-nodejs/31987
- 10: Export
wrapMcpServerWithSentryfrom Cloudflare SDK sentry-javascript#16111
Missing wrapMcpServerWithSentry in @sentry/node & no explicit initialization
The function wrapMcpServerWithSentry isn’t exported by @sentry/node (it only exists in the Cloudflare SDK), and you must call Sentry.init(...)—simply having SENTRY_DSN set won’t initialize the SDK. Without these, the dynamic import will either throw (entering your catch block) or silently do nothing.
Please update src/server/instrumentation.ts around lines 63–77 to:
- Import and call the correct wrapper (or implement your own) instead of
Sentry.wrapMcpServerWithSentry. - Invoke
Sentry.init({ dsn: process.env.SENTRY_DSN })before wrapping the server.
For example:
log('info', 'Loading Sentry instrumentation...');
const Sentry = await import('@sentry/node');
Sentry.init({ dsn: process.env.SENTRY_DSN });
// TODO: replace with the real wrapper or implement equivalent
import { wrapMcpServerWithSentry } from './sentryWrapper';
const instrumentedServer = wrapMcpServerWithSentry(server);
...🤖 Prompt for AI Agents
In src/server/instrumentation.ts around lines 63–77, the code calls a
non-existent Sentry.wrapMcpServerWithSentry and never initializes the Sentry
SDK; replace the call with a real wrapper (or implement one locally) and call
Sentry.init before wrapping. Specifically: keep the lazy import of
'@sentry/node', then call Sentry.init({ dsn: process.env.SENTRY_DSN }) (and any
env/options needed), then either import/use a local wrapMcpServerWithSentry
function (or implement equivalent request/error handlers) instead of
Sentry.wrapMcpServerWithSentry, apply the wrapper to the server, set the
SENTRY_WRAPPED_SYMBOL on the instrumented server, and return it.
src/utils/axe-command.ts
Outdated
| executor: CommandExecutor = getDefaultCommandExecutor(), | ||
| axeHelpers: AxeHelpers = { getAxePath, getBundledAxeEnvironment, createAxeNotAvailableResponse }, | ||
| logPrefix: string = '[AXe]', | ||
| returnOutput: boolean = false, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm CommandExecutor accepts the optional env parameter
executeAxeCommand passes a 4th env argument to executor. Ensure all CommandExecutor implementations (including test mocks) accept this parameter to avoid type/runtime mismatches.
Run this scan to validate the CommandExecutor type signatures and usages:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching CommandExecutor type declarations and usages..."
rg -nP -C2 '\b(type|interface)\s+CommandExecutor\b'
echo
rg -nP -C2 '\bcreateMockExecutor\b'
echo
# Look for function signatures accepting 3 vs 4 params
rg -nP -C2 'CommandExecutor\s*=\s*\('
rg -nP -C2 'function\s+\w*\(\s*command:\s*string\[\],\s*description\?:\s*string,\s*allowFailure\?:\s*boolean(,|\))'Length of output: 183481
Add optional env parameter to CommandExecutor signatures
The executeAxeCommand helper now passes a fourth env argument to the executor, but our CommandExecutor types only declare up to three parameters. We need to update both definitions and all mock/real implementations to accept an optional env parameter:
• src/utils/command.ts (around line 30):
export type CommandExecutor = (
command: string[],
logPrefix?: string,
allowFailure?: boolean,
/** Optional environment variables for the spawned process */
env?: NodeJS.ProcessEnv
) => Promise<CommandResult>;• src/test-utils/mock-executors.ts (around line 23):
export type CommandExecutor = (
command: string[],
description?: string,
allowFailure?: boolean,
/** Optional env for testing overrides */
env?: Record<string, unknown>
) => Promise<MockResult>;• Ensure getDefaultCommandExecutor, createMockExecutor and any other executor implementations include env? in their parameter lists and pass it through.
This alignment will prevent type errors and ensure runtime consistency when an env object is provided.
🤖 Prompt for AI Agents
In src/utils/axe-command.ts around lines 41 to 44, the executor and related
types/implementations don't accept the optional fourth env parameter that
executeAxeCommand now passes; update the CommandExecutor type in
src/utils/command.ts (around line 30) to add an optional env?: NodeJS.ProcessEnv
parameter, update the test mock type in src/test-utils/mock-executors.ts (around
line 23) to add env?: Record<string, unknown>, and modify
getDefaultCommandExecutor, createMockExecutor and any other executor
implementations to include env? in their parameter lists and pass that env
through to the spawned process or mock result so signatures and runtime behavior
match.
src/utils/axe-command.ts
Outdated
| throw new AxeError( | ||
| `axe command '${commandName}' failed.`, | ||
| commandName, | ||
| result.error ?? result.output, | ||
| simulatorUuid, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include the actual executed command in AxeError for better diagnostics
Passing commandName as the “command” field loses detail. Include the full command string.
Apply this diff:
- throw new AxeError(
- `axe command '${commandName}' failed.`,
- commandName,
- result.error ?? result.output,
- simulatorUuid,
- );
+ throw new AxeError(
+ `axe command '${commandName}' failed.`,
+ fullCommand.join(' '),
+ result.error ?? result.output,
+ simulatorUuid,
+ );📝 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.
| throw new AxeError( | |
| `axe command '${commandName}' failed.`, | |
| commandName, | |
| result.error ?? result.output, | |
| simulatorUuid, | |
| ); | |
| } | |
| throw new AxeError( | |
| `axe command '${commandName}' failed.`, | |
| fullCommand.join(' '), | |
| result.error ?? result.output, | |
| simulatorUuid, | |
| ); |
🤖 Prompt for AI Agents
In src/utils/axe-command.ts around lines 65 to 71, the AxeError is being
constructed with commandName (a short identifier) which loses the full executed
command; replace the commandName argument with the actual executed command
string (the variable that contains the full command used to spawn the process,
e.g., fullCommand or command) so the AxeError carries the full command text,
keep the rest of the arguments the same.
src/utils/logger.ts
Outdated
| import * as Sentry from '@sentry/node'; | ||
|
|
||
| const SENTRY_ENABLED = process.env.SENTRY_DISABLED !== 'true'; | ||
| const SENTRY_ENABLED = process.env.XCODEBUILDMCP_SENTRY_DISABLED !== 'true'; | ||
|
|
||
| if (!SENTRY_ENABLED) { | ||
| log('info', 'Sentry disabled due to SENTRY_DISABLED environment variable'); | ||
| log('info', 'Sentry disabled due to XCODEBUILDMCP_SENTRY_DISABLED environment variable'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid static Sentry import and top-level side effects in a “pure utils” module; lazy-load in error path
This utils module statically imports @sentry/node and logs at import time, which conflicts with the PR’s goal of keeping utils side-effect-free and only loading Sentry in production. Recommend removing the static import and top-level log, and lazily importing Sentry only when logging errors and when enabled.
Apply this minimal diff in this hunk:
-import * as Sentry from '@sentry/node';
-
-const SENTRY_ENABLED = process.env.XCODEBUILDMCP_SENTRY_DISABLED !== 'true';
-
-if (!SENTRY_ENABLED) {
- log('info', 'Sentry disabled due to XCODEBUILDMCP_SENTRY_DISABLED environment variable');
-}
+// Sentry is loaded lazily to keep utils side‑effect‑free and avoid Node-only deps in test/runtime paths
+const SENTRY_ENABLED = process.env.XCODEBUILDMCP_SENTRY_DISABLED !== 'true';Add this helper near the top of the file (outside the function), to support lazy error reporting:
let sentryModulePromise: Promise<typeof import('@sentry/node')> | undefined;
async function captureErrorInSentry(message: string): Promise<void> {
if (!SENTRY_ENABLED) return;
// Optionally also gate by NODE_ENV === 'production' to avoid loading Sentry outside prod
try {
const Sentry = await (sentryModulePromise ??= import('@sentry/node'));
Sentry.captureMessage(message);
} catch {
// Swallow Sentry import/report failures; logging must not impact control flow
}
}
…oduction dependencies ## Root Cause Analysis CI was failing with 78 "module is already linked" errors due to import chain: test files → utils/command.js → utils/logger.js → @sentry/node → native modules This chain caused Vitest's vmThreads pool to load native modules multiple times across VM contexts, triggering Linux glibc's module linking restrictions. ## Solution: Surgical Mock Function Migration - Created isolated test-utils/mock-executors.ts with exact copies of mock functions - Removed all mock functions from utils/command.ts (production dependency chain) - Updated 54+ test files to import mocks from test-utils/mock-executors.js - Extracted CommandExecutor and FileSystemExecutor interfaces to separate files - Maintained zero behavioral changes to preserve all 1046 passing tests ## Key Changes - NEW: src/test-utils/mock-executors.ts - Isolated test mocks (no production deps) - NEW: src/utils/CommandExecutor.ts - Command executor interface - NEW: src/utils/FileSystemExecutor.ts - File system executor interface - MODIFIED: src/utils/command.ts - Removed mock functions, kept production executors - MODIFIED: 54+ test files - Updated imports to use isolated test-utils ## Import Chain Resolution BEFORE: test files → utils/command.js → utils/logger.js → @sentry/node (BROKEN) AFTER: test files → test-utils/mock-executors.js (ISOLATED ✅) ## Verification - All 1046 tests pass locally with surgical migration - Linting and TypeScript compilation clean - Production command execution unchanged - Test mock behavior preserved exactly Resolves Ubuntu CI "module is already linked" errors while maintaining test reliability and production functionality.
cameroncooke
force-pushed
the
fix/module-import-bug
branch
from
b759710 to
8c5ce80
Compare
|
Claude finished @cameroncooke's task —— View job PR Review: CI Module Linking FixThis is an excellent targeted fix that successfully addresses the CI module linking issues while maintaining clean architecture. The changes are well-structured, thoroughly implemented, and demonstrate strong engineering principles. Review Progress:
✅ Strengths1. Architectural Excellence
2. CI Issue Resolution
3. Testing Architecture Improvements
4. Code Quality Enhancements
✅ Technical Implementation Quality1. Interface Design// Clean, well-designed interfaces
export type CommandExecutor = (
command: string[],
logPrefix?: string,
useShell?: boolean,
env?: Record<string, string>,
detached?: boolean,
) => Promise<CommandResponse>;2. Test Safety Mechanisms// Excellent guardrails to prevent production calls in tests
if (process.env.VITEST === 'true' || process.env.NODE_ENV === 'test') {
throw new Error('🚨 REAL SYSTEM EXECUTOR DETECTED IN TEST! 🚨\n...');
}3. Mock Function Completeness
|
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (5)
src/mcp/tools/project-discovery/get_app_bundle_id.ts (1)
34-40: Shell injection risk: avoid '/bin/sh -c' with user-controlled appPathexecuteSyncCommand invokes '/bin/sh -c ""' with appPath interpolated. A malicious or malformed path could break quoting or inject arguments. Execute commands with argv arrays instead.
Proposed refactor:
-/** - * Sync wrapper for CommandExecutor to handle synchronous commands - */ -async function executeSyncCommand(command: string, executor: CommandExecutor): Promise<string> { - const result = await executor(['/bin/sh', '-c', command], 'Bundle ID Extraction'); - if (!result.success) { - throw new Error(result.error ?? 'Command failed'); - } - return result.output || ''; -} +/** + * Safer command executor: no shell; pass argv directly + */ +async function executeCommand( + cmd: string[], + executor: CommandExecutor, + logPrefix = 'Bundle ID Extraction', +): Promise<string> { + const result = await executor(cmd, logPrefix); + if (!result.success) { + throw new Error(result.error ?? 'Command failed'); + } + return result.output || ''; +} @@ - bundleId = await executeSyncCommand( - `defaults read "${appPath}/Info" CFBundleIdentifier`, - executor, - ); + bundleId = await executeCommand( + ['defaults', 'read', `${appPath}/Info`, 'CFBundleIdentifier'], + executor, + ); @@ - bundleId = await executeSyncCommand( - `/usr/libexec/PlistBuddy -c "Print :CFBundleIdentifier" "${appPath}/Info.plist"`, - executor, - ); + bundleId = await executeCommand( + ['/usr/libexec/PlistBuddy', '-c', 'Print :CFBundleIdentifier', `${appPath}/Info.plist`], + executor, + );Optional guardrail: if you want extra safety, reject paths containing quotes or newlines before execution.
If you prefer to keep a shell, you should at least validate/sanitize appPath to disallow quotes and control characters and use spawn with argv where possible.
Also applies to: 71-86
src/utils/template-manager.ts (1)
117-125: Bug: returning the extracted root directory instead of the 'template' subdirectoryComment says “return the template subdirectory,” but the code returns the extracted root. For local overrides you return the 'template' subdir; for downloaded templates you should mirror the same contract. This mismatch is likely to break downstream code that expects the template folder.
Apply this diff:
- // Find the extracted directory and return the template subdirectory - const extractedDir = join(tempDir, `${repo}-${version.substring(1)}`); - if (!fileSystemExecutor.existsSync(extractedDir)) { - throw new Error(`Expected template directory not found: ${extractedDir}`); - } - - log('info', `Successfully downloaded ${platform} template ${version}`); - return extractedDir; + // Find the extracted directory and return the template subdirectory + const extractedDir = join(tempDir, `${repo}-${version.substring(1)}`); + if (!fileSystemExecutor.existsSync(extractedDir)) { + throw new Error(`Expected extracted directory not found: ${extractedDir}`); + } + const templateSubdir = join(extractedDir, 'template'); + if (!fileSystemExecutor.existsSync(templateSubdir)) { + throw new Error(`Expected template subdirectory not found: ${templateSubdir}`); + } + + log('info', `Successfully downloaded ${platform} template ${version}`); + return templateSubdir;src/mcp/tools/logging/stop_device_log_cap.ts (1)
1-293: Enforce type-only, .js imports for FileSystemExecutor
To prevent runtime errors from importing source files, all occurrences ofFileSystemExecutormust be imported as a type from the compiled.jsmodule. Please update each value import from.tsto a type-only import from.js.Affected files:
- src/test-utils/mock-executors.ts
- src/utils/validation.ts
- src/utils/template-manager.ts
- src/utils/command.ts
- src/mcp/tools/project-discovery/get_mac_bundle_id.ts
- src/mcp/tools/project-discovery/get_app_bundle_id.ts
- src/mcp/tools/project-discovery/discover_projs.ts
- src/mcp/tools/macos/launch_mac_app.ts
- src/mcp/tools/macos/test_macos.ts
- src/mcp/tools/project-scaffolding/scaffold_macos_project.ts
- src/mcp/tools/device/test_device.ts
- src/mcp/tools/logging/stop_device_log_cap.ts
Example diff for each import (adjust paths as needed):
- import { FileSystemExecutor } from './FileSystemExecutor.ts'; + import type { FileSystemExecutor } from './FileSystemExecutor.js';src/utils/command.ts (2)
68-76: Align spawn options with detached semanticsWhen detached is true, set spawnOpts.detached and use stdio: 'ignore' to prevent the parent from holding references. Otherwise, keep piping.
Apply this diff:
- const spawnOpts: Parameters<typeof spawn>[2] = { - stdio: ['ignore', 'pipe', 'pipe'], // ignore stdin, pipe stdout/stderr - }; + const spawnOpts: Parameters<typeof spawn>[2] = {}; + if (detached) { + spawnOpts.detached = true; + spawnOpts.stdio = 'ignore'; + } else { + // ignore stdin, pipe stdout/stderr + spawnOpts.stdio = ['ignore', 'pipe', 'pipe']; + }
81-87: Avoid attaching stream listeners for detached processesPiping stdout/stderr for detached processes pins the child. Only attach listeners when not detached.
Apply this diff:
- let stdout = ''; - let stderr = ''; - - childProcess.stdout?.on('data', (data: Buffer) => { - stdout += data.toString(); - }); - - childProcess.stderr?.on('data', (data: Buffer) => { - stderr += data.toString(); - }); + let stdout = ''; + let stderr = ''; + if (!detached) { + childProcess.stdout?.on('data', (data: Buffer) => { + stdout += data.toString(); + }); + childProcess.stderr?.on('data', (data: Buffer) => { + stderr += data.toString(); + }); + }
♻️ Duplicate comments (1)
docs/TOOLS.md (1)
30-31: Fix grammar and add “Example:” label for launch_app_sim.This mirrors a prior suggestion and improves clarity.
-- `launch_app_sim` - Launches an app in an iOS simulator by UUID or name. If simulator window isn't visible, use open_sim() first. or launch_app_sim({ simulatorName: 'iPhone 16', bundleId: 'com.example.MyApp' }) +- `launch_app_sim` - Launches an app in an iOS simulator by UUID or name. If the simulator window isn't visible, use open_sim() first. Example: launch_app_sim({ simulatorName: 'iPhone 16', bundleId: 'com.example.MyApp' })
🧹 Nitpick comments (34)
src/mcp/tools/macos/launch_mac_app.ts (1)
12-13: Prefer type-only imports to avoid unnecessary runtime deps (and reflect new module split).
FileSystemExecutoris used only as a type here. Import it withimport typeso it’s erased at emit time. Also consider importing theCommandExecutortype from the new dedicated module while keeping the runtime import forgetDefaultCommandExecutorfromutils/command.js.Apply this diff:
-import { CommandExecutor, getDefaultCommandExecutor } from '../../../utils/command.js'; -import { FileSystemExecutor } from '../../../utils/FileSystemExecutor.ts'; +import { getDefaultCommandExecutor } from '../../../utils/command.js'; +import type { CommandExecutor } from '../../../utils/CommandExecutor.ts'; +import type { FileSystemExecutor } from '../../../utils/FileSystemExecutor.ts';scripts/update-tools-docs.ts (2)
81-83: Polish: correct singular/plural for tool count (1 tool vs 2 tools).Small readability win in generated docs.
- content += `**Purpose**: ${workflow.description} (${toolCount} tools)\n\n`; + const toolsLabel = toolCount === 1 ? 'tool' : 'tools'; + content += `**Purpose**: ${workflow.description} (${toolCount} ${toolsLabel})\n\n`;
105-106: Avoid mutating input arrays during sort.Sorting in place modifies
analysis.workflows. Defensive copy keepsanalysisimmutable to callers.- const sortedWorkflows = workflows.sort((a, b) => a.displayName.localeCompare(b.displayName)); + const sortedWorkflows = [...workflows].sort((a, b) => a.displayName.localeCompare(b.displayName));docs/TOOLS.md (1)
65-66: Minor: pluralization nit (“1 tools”).Several sections with a single tool read “(1 tools)”. This comes from the generator; see suggested fix in scripts/update-tools-docs.ts to render “1 tool”.
src/mcp/tools/device/__tests__/launch_app_device.test.ts (1)
205-246: Reduce global side effects when patching fs in tests.Overwriting fs.promises directly can introduce flaky behavior under parallel execution. Prefer scoped spies/mocks.
Example using Vitest:
import { vi } from 'vitest'; const readFileSpy = vi.spyOn(fs.promises, 'readFile').mockImplementation(async (p: any) => { if (String(p).includes('launch-')) return JSON.stringify({ result: { process: { processIdentifier: 12345 } } }); return originalReadFile(p as any); }); const unlinkSpy = vi.spyOn(fs.promises, 'unlink').mockResolvedValue(); try { // run test body } finally { readFileSpy.mockRestore(); unlinkSpy.mockRestore(); }src/utils/validation.ts (1)
25-25: Use type-only re-exports/imports to avoid runtime artifactsThese are types only; exporting/importing them as values can cause issues under isolatedModules and increases risk of runtime import churn.
Apply:
-import { ToolResponse, ValidationResult } from '../types/common.js'; +import type { ToolResponse, ValidationResult } from '../types/common.js';And at the bottom:
-export { ToolResponse, ValidationResult }; +export type { ToolResponse, ValidationResult };Also applies to: 262-262
src/mcp/tools/simulator-management/__tests__/reset_sim_location.test.ts (1)
3-3: Standardize import specifiers: prefer .js (or enable allowImportingTsExtensions)This test imports from '../reset_sim_location.ts' while other imports use .js specifiers. Unless the repo explicitly enables allowImportingTsExtensions and tooling is configured for it, prefer .js to avoid resolution issues.
Apply:
-import resetSimLocationPlugin, { reset_sim_locationLogic } from '../reset_sim_location.ts'; +import resetSimLocationPlugin, { reset_sim_locationLogic } from '../reset_sim_location.js';src/mcp/tools/project-discovery/get_app_bundle_id.ts (1)
51-65: Optional: validate .app directory shape earlierYou could proactively validate that appPath ends with .app and that Info.plist exists to produce a clearer error before running commands.
Example:
- Check: appPath.endsWith('.app')
- Check: fileSystemExecutor.existsSync(
${appPath}/Info.plist)src/mcp/tools/device/__tests__/install_app_device.test.ts (1)
41-45: Tighten types in test harness (unknown → string/typed env)Using concrete types here increases signal and catches regressions earlier without impacting the test behavior.
- let capturedCommand: unknown[] = []; + let capturedCommand: string[] = []; let capturedDescription: string = ''; let capturedUseShell: boolean = false; - let capturedEnv: unknown = undefined; + let capturedEnv: Record<string, string> | undefined = undefined; - const trackingExecutor = async ( - command: unknown[], + const trackingExecutor = async ( + command: string[], description: string, useShell: boolean, - env: unknown, + env: Record<string, string> | undefined, ) => {Also applies to: 52-57
src/mcp/tools/ui-testing/__tests__/describe_ui.test.ts (1)
11-14: Remove unused test variables
mockCallsis declared and reassigned but never used. Trimming this keeps the test concise.- let mockCalls: any[] = []; - - mockCalls = []; + // (removed unused mockCalls)src/mcp/tools/simulator/__tests__/launch_app_sim.test.ts (1)
60-61: Remove unused variable
originalExecutoris assigned but never used in this test.- const originalExecutor = mockExecutor; - + // (removed unused originalExecutor)src/mcp/tools/ui-testing/__tests__/gesture.test.ts (1)
11-11: Optional: introduce a path alias for test-utils to avoid brittle deep relativesDeep relative imports are harder to maintain across moves. Consider a short alias for test-utils:
Add to tsconfig.json:
{ "compilerOptions": { "baseUrl": "src", "paths": { "@test-utils/*": ["test-utils/*"] }, "moduleResolution": "Bundler" } }And in vitest.config.ts:
import { defineConfig } from 'vitest/config'; import path from 'node:path'; export default defineConfig({ resolve: { alias: { '@test-utils': path.resolve(__dirname, 'src/test-utils'), }, }, });Then update imports:
-} from '../../../../test-utils/mock-executors.js'; +} from '@test-utils/mock-executors.js';src/mcp/tools/macos/__tests__/test_macos.test.ts (1)
60-71: Remove unused mocks in the first two XOR validation testsBoth tests call the handler directly and don’t use the locally-declared mockExecutor or mockFileSystemExecutor. Dropping them reduces noise and avoids potential no-unused-vars lint warnings.
- const mockExecutor = createMockExecutor({ - success: true, - output: 'Test Suite All Tests passed', - }); - - const mockFileSystemExecutor = { - mkdtemp: async () => '/tmp/test-123', - rm: async () => {}, - tmpdir: () => '/tmp', - stat: async () => ({ isDirectory: () => true }), - };- const mockExecutor = createMockExecutor({ - success: true, - output: 'Test Suite All Tests passed', - }); - - const mockFileSystemExecutor = { - mkdtemp: async () => '/tmp/test-123', - rm: async () => {}, - tmpdir: () => '/tmp', - stat: async () => ({ isDirectory: () => true }), - };Also applies to: 82-93
src/mcp/tools/simulator/__tests__/boot_sim.test.ts (1)
10-13: Trim unused imports from test-utilscreateMockFileSystemExecutor and createNoopExecutor aren’t used in this file. Remove them from the import to keep things tidy and avoid lint warnings.
-import { - createMockExecutor, - createMockFileSystemExecutor, - createNoopExecutor, -} from '../../../../test-utils/mock-executors.js'; +import { createMockExecutor } from '../../../../test-utils/mock-executors.js';src/mcp/tools/ui-testing/__tests__/type_text.test.ts (1)
8-11: Narrow the test-utils import to used symbolscreateMockFileSystemExecutor isn’t referenced in this test. Removing it prevents future no-unused-vars issues and clarifies dependencies.
-import { - createMockExecutor, - createMockFileSystemExecutor, - createNoopExecutor, -} from '../../../../test-utils/mock-executors.js'; +import { createMockExecutor, createNoopExecutor } from '../../../../test-utils/mock-executors.js';src/utils/index.ts (1)
4-5: 🔧 Convert executor exports to type-onlyTo keep the utils barrel side-effect free and avoid unnecessary runtime resolution for pure interface modules, please update
src/utils/index.ts:--- a/src/utils/index.ts +++ b/src/utils/index.ts @@ lines 4-5 -export * from './CommandExecutor.js'; -export * from './FileSystemExecutor.js'; +export type { CommandExecutor, CommandResponse } from './CommandExecutor.js'; +export type { FileSystemExecutor } from './FileSystemExecutor.js';This change ensures only types are re-exported at runtime, aligning with our “pure utils” boundary. [optional_refactors_recommended]
src/mcp/tools/simulator/__tests__/stop_app_sim.test.ts (1)
7-7: Consider a path alias for test utilities to reduce fragile relative paths.This will improve maintainability across the repo when files move.
Apply this diff once aliases are configured:
-} from '../../../../test-utils/mock-executors.js'; +} from '@test-utils/mock-executors.js';Add alias support:
- tsconfig.json
{ "compilerOptions": { "baseUrl": ".", "paths": { "@test-utils/*": ["src/test-utils/*"] } } }
- vitest.config.ts (or vite.config.ts)
import { defineConfig } from 'vitest/config'; import { fileURLToPath } from 'node:url'; export default defineConfig({ resolve: { alias: { '@test-utils': fileURLToPath(new URL('./src/test-utils', import.meta.url)), }, }, });src/mcp/tools/logging/stop_device_log_cap.ts (4)
13-14: Prefer type-only import and .js extension for ESM stabilityTo prevent a runtime import of a .ts file (and ensure elision under TS), switch to a type-only import and reference the emitted .js extension. This avoids subtle bundler/Node resolution issues if the import is preserved.
-import { getDefaultFileSystemExecutor, getDefaultCommandExecutor } from '../../../utils/command.js'; -import { FileSystemExecutor } from '../../../utils/FileSystemExecutor.ts'; +import { getDefaultFileSystemExecutor, getDefaultCommandExecutor } from '../../../utils/command.js'; +import type { FileSystemExecutor } from '../../../utils/FileSystemExecutor.js';
94-96: Guard against undefined exitCode when stopping the processexitCode is typed as number | null | undefined. If it’s undefined, the current condition won’t attempt to terminate an active process. Include undefined in the check.
- if (!session.process.killed && session.process.exitCode === null) { + if (!session.process.killed && (session.process.exitCode === null || session.process.exitCode === undefined)) { session.process.kill?.('SIGTERM'); }
246-248: Use os.tmpdir() instead of hard-coded '/tmp'Hard-coding '/tmp' reduces portability. Use Node’s os.tmpdir() for cross-platform correctness.
- tmpdir(): string { - return '/tmp'; - }, + tmpdir(): string { + return os.tmpdir(); + },Add this import at the top of the file:
+import os from 'os';
152-249: Consider extracting the legacy adapter to a shared utilThe inlined adapter for backward compatibility is lengthy and duplicates fs logic. Extracting it (e.g., to utils/fs-adapter.ts) would improve reuse and readability across plugins that may need the same shim.
I can sketch a small toFileSystemExecutor(fileSystem?: unknown): FileSystemExecutor helper if helpful.
src/mcp/tools/macos/test_macos.ts (1)
19-19: Make FileSystemExecutor a type-only import (use .js extension)This keeps the import purely for typing and avoids runtime resolution of the .ts source.
-import { FileSystemExecutor } from '../../../utils/FileSystemExecutor.ts'; +import type { FileSystemExecutor } from '../../../utils/FileSystemExecutor.js';src/mcp/tools/device/test_device.ts (2)
19-19: Make FileSystemExecutor a type-only import (use .js extension)Same rationale as other files: avoid runtime import of a .ts module.
-import { FileSystemExecutor } from '../../../utils/FileSystemExecutor.ts'; +import type { FileSystemExecutor } from '../../../utils/FileSystemExecutor.js';
206-208: Consistency: default preferXcodebuild to false
test_macos.tspassesparams.preferXcodebuild ?? false. Here it forwards possibly undefined. Aligning makes behavior consistent and explicit.- params.preferXcodebuild, + params.preferXcodebuild ?? false,src/mcp/tools/logging/__tests__/stop_sim_log_cap.test.ts (1)
20-50: Optional: avoid hardcoding '/tmp' and clean up temp filesThe helper writes directly to
/tmpand doesn’t remove files, which can leak artifacts and isn’t cross-platform. Preferos.tmpdir()and add afterEach cleanup.@@ -import { describe, it, expect, beforeEach } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import os from 'os'; +import { join } from 'path'; @@ - let mockFileSystem: any; + let mockFileSystem: any; + const createdFiles: string[] = []; @@ beforeEach(() => { mockFileSystem = createMockFileSystemExecutor(); // Clear any active sessions before each test activeLogSessions.clear(); }); + + afterEach(async () => { + const fs = await import('fs/promises'); + for (const p of createdFiles.splice(0)) { + try { await fs.unlink(p); } catch {} + } + }); @@ - async function createTestLogSession(sessionId: string, logContent: string = '') { + async function createTestLogSession(sessionId: string, logContent: string = '') { @@ - const logFilePath = `/tmp/xcodemcp_sim_log_test_${sessionId}.log`; + const logFilePath = join(os.tmpdir(), `xcodemcp_sim_log_test_${sessionId}.log`); @@ - await fs.writeFile(logFilePath, logContent, 'utf-8'); + await fs.writeFile(logFilePath, logContent, 'utf-8'); + createdFiles.push(logFilePath);src/utils/CommandExecutor.ts (3)
1-1: Use type-only import to avoid unintended runtime dependency on child_processThis file defines types only; importing ChildProcess as a value will generate a runtime import. Switch to a type-only import.
Apply this diff:
-import { ChildProcess } from 'child_process'; +import type { ChildProcess } from 'child_process';
6-12: Broaden env type to align with Node’s spawn env semanticsNode’s spawn expects env values as string | undefined (NodeJS.ProcessEnv). Using Record<string, string> is stricter and can force casts downstream. Prefer NodeJS.ProcessEnv (or a compatible partial) in the API surface.
Apply this diff:
export type CommandExecutor = ( command: string[], logPrefix?: string, useShell?: boolean, - env?: Record<string, string>, + env?: NodeJS.ProcessEnv, detached?: boolean, ) => Promise<CommandResponse>;If you prefer to avoid NodeJS namespace, an alternative is:
- env?: Record<string, string>, + env?: Record<string, string | undefined>,
14-22: Consider documenting error semantics (reject vs resolve with error) in the type contractThe default executor rejects on spawn errors, while CommandResponse also has an error field. Clarify in the JSDoc whether callers should expect rejections for transport-level failures (spawn errors) vs resolution with success=false for process exit failures.
src/utils/FileSystemExecutor.ts (1)
5-16: Tighten readdir/stat typings with overloads and Node types (type-only) for better DXunknown[] for readdir loses useful type info. You can provide overloads mirroring fs.readdir and keep zero runtime cost via type-only imports. Same for stat returning a minimal shape; consider Node’s Stats.
Apply this diff:
+import type { Dirent, Stats } from 'fs'; + export interface FileSystemExecutor { mkdir(path: string, options?: { recursive?: boolean }): Promise<void>; readFile(path: string, encoding?: BufferEncoding): Promise<string>; writeFile(path: string, content: string, encoding?: BufferEncoding): Promise<void>; cp(source: string, destination: string, options?: { recursive?: boolean }): Promise<void>; - readdir(path: string, options?: { withFileTypes?: boolean }): Promise<unknown[]>; + // Overloads to mirror Node's fs.readdir behavior + readdir(path: string, options: { withFileTypes: true }): Promise<Dirent[]>; + readdir(path: string, options?: { withFileTypes?: false }): Promise<string[]>; + readdir(path: string, options?: { withFileTypes?: boolean }): Promise<string[] | Dirent[]>; rm(path: string, options?: { recursive?: boolean; force?: boolean }): Promise<void>; existsSync(path: string): boolean; - stat(path: string): Promise<{ isDirectory(): boolean }>; + stat(path: string): Promise<Stats>; mkdtemp(prefix: string): Promise<string>; tmpdir(): string; }If you want to keep the minimal surface intentionally, feel free to ignore—this is a DX improvement.
src/utils/command.ts (2)
90-121: Unref truly detached processesCall unref() so the parent can exit independently of the child. Keep your existing short readiness delay, but unref immediately after spawn.
Apply this diff:
- // For detached processes, handle differently to avoid race conditions + // For detached processes, handle differently to avoid race conditions if (detached) { + // allow parent to exit independently of the child + if (typeof childProcess.unref === 'function') { + childProcess.unref(); + } // For detached processes, only wait for spawn success/failure let resolved = false;
41-57: Shell quoting is fragile; consider spawn'sshell: trueinsteadManual quoting with sh -c is error-prone for special characters and expansions. Using spawn(commandString, { shell: true }) delegates parsing to the platform shell and avoids custom quoting logic. Not mandatory for this PR, but worth tracking.
src/test-utils/mock-executors.ts (3)
172-175: Doc nit: incorrect return type mentionedComment says “returns CommandExecutor” but this factory returns a FileSystemExecutor. Update the doc to avoid confusion.
Apply this diff:
- * @returns CommandExecutor that throws on invocation + * @returns FileSystemExecutor that throws on invocation
176-252: Intentional inconsistency: tmpdir does not throw in Noop FSAll FS methods throw except tmpdir(), which returns '/tmp'. If this is deliberate (allowing path computations without I/O), add a short note in the JSDoc to prevent confusion during test authoring.
99-148: Command pattern matching is order-dependentFirst matching key wins (Object.keys order). If overlapping patterns exist, behavior may be surprising. Consider documenting precedence or supporting priority via an array of [pattern, result] to make ordering explicit.
📜 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 (77)
TOOL_NAMING_VERIFICATION_2025-08-14_22-13.md(0 hunks)TOOL_NAMING_VERIFICATION_2025-08-14_22-13.md.bak(0 hunks)docs/TOOLS.md(4 hunks)scripts/update-tools-docs.ts(1 hunks)src/mcp/resources/__tests__/devices.test.ts(1 hunks)src/mcp/resources/__tests__/doctor.test.ts(1 hunks)src/mcp/resources/__tests__/simulators.test.ts(1 hunks)src/mcp/tools/device/__tests__/build_device.test.ts(1 hunks)src/mcp/tools/device/__tests__/get_device_app_path.test.ts(1 hunks)src/mcp/tools/device/__tests__/install_app_device.test.ts(1 hunks)src/mcp/tools/device/__tests__/launch_app_device.test.ts(1 hunks)src/mcp/tools/device/__tests__/list_devices.test.ts(1 hunks)src/mcp/tools/device/__tests__/stop_app_device.test.ts(1 hunks)src/mcp/tools/device/__tests__/test_device.test.ts(1 hunks)src/mcp/tools/device/test_device.ts(1 hunks)src/mcp/tools/logging/__tests__/start_device_log_cap.test.ts(1 hunks)src/mcp/tools/logging/__tests__/start_sim_log_cap.test.ts(1 hunks)src/mcp/tools/logging/__tests__/stop_device_log_cap.test.ts(1 hunks)src/mcp/tools/logging/__tests__/stop_sim_log_cap.test.ts(1 hunks)src/mcp/tools/logging/stop_device_log_cap.ts(1 hunks)src/mcp/tools/macos/__tests__/build_macos.test.ts(1 hunks)src/mcp/tools/macos/__tests__/build_run_macos.test.ts(1 hunks)src/mcp/tools/macos/__tests__/get_mac_app_path.test.ts(1 hunks)src/mcp/tools/macos/__tests__/launch_mac_app.test.ts(1 hunks)src/mcp/tools/macos/__tests__/test_macos.test.ts(1 hunks)src/mcp/tools/macos/launch_mac_app.ts(1 hunks)src/mcp/tools/macos/test_macos.ts(1 hunks)src/mcp/tools/project-discovery/__tests__/discover_projs.test.ts(1 hunks)src/mcp/tools/project-discovery/__tests__/get_app_bundle_id.test.ts(1 hunks)src/mcp/tools/project-discovery/__tests__/get_mac_bundle_id.test.ts(1 hunks)src/mcp/tools/project-discovery/__tests__/list_schemes.test.ts(1 hunks)src/mcp/tools/project-discovery/__tests__/show_build_settings.test.ts(1 hunks)src/mcp/tools/project-discovery/discover_projs.ts(1 hunks)src/mcp/tools/project-discovery/get_app_bundle_id.ts(1 hunks)src/mcp/tools/project-discovery/get_mac_bundle_id.ts(1 hunks)src/mcp/tools/project-scaffolding/__tests__/scaffold_ios_project.test.ts(1 hunks)src/mcp/tools/project-scaffolding/__tests__/scaffold_macos_project.test.ts(1 hunks)src/mcp/tools/project-scaffolding/scaffold_macos_project.ts(1 hunks)src/mcp/tools/simulator-management/__tests__/reset_sim_location.test.ts(1 hunks)src/mcp/tools/simulator-management/__tests__/set_sim_appearance.test.ts(1 hunks)src/mcp/tools/simulator-management/__tests__/set_sim_location.test.ts(1 hunks)src/mcp/tools/simulator-management/__tests__/sim_statusbar.test.ts(1 hunks)src/mcp/tools/simulator/__tests__/boot_sim.test.ts(1 hunks)src/mcp/tools/simulator/__tests__/build_run_sim.test.ts(1 hunks)src/mcp/tools/simulator/__tests__/build_sim.test.ts(1 hunks)src/mcp/tools/simulator/__tests__/install_app_sim.test.ts(1 hunks)src/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.ts(1 hunks)src/mcp/tools/simulator/__tests__/launch_app_sim.test.ts(1 hunks)src/mcp/tools/simulator/__tests__/list_sims.test.ts(1 hunks)src/mcp/tools/simulator/__tests__/open_sim.test.ts(1 hunks)src/mcp/tools/simulator/__tests__/screenshot.test.ts(1 hunks)src/mcp/tools/simulator/__tests__/stop_app_sim.test.ts(1 hunks)src/mcp/tools/swift-package/__tests__/swift_package_build.test.ts(1 hunks)src/mcp/tools/swift-package/__tests__/swift_package_clean.test.ts(1 hunks)src/mcp/tools/swift-package/__tests__/swift_package_run.test.ts(1 hunks)src/mcp/tools/swift-package/__tests__/swift_package_test.test.ts(1 hunks)src/mcp/tools/ui-testing/__tests__/button.test.ts(1 hunks)src/mcp/tools/ui-testing/__tests__/describe_ui.test.ts(1 hunks)src/mcp/tools/ui-testing/__tests__/gesture.test.ts(1 hunks)src/mcp/tools/ui-testing/__tests__/key_press.test.ts(1 hunks)src/mcp/tools/ui-testing/__tests__/key_sequence.test.ts(1 hunks)src/mcp/tools/ui-testing/__tests__/long_press.test.ts(1 hunks)src/mcp/tools/ui-testing/__tests__/screenshot.test.ts(1 hunks)src/mcp/tools/ui-testing/__tests__/swipe.test.ts(1 hunks)src/mcp/tools/ui-testing/__tests__/tap.test.ts(1 hunks)src/mcp/tools/ui-testing/__tests__/touch.test.ts(1 hunks)src/mcp/tools/ui-testing/__tests__/type_text.test.ts(1 hunks)src/mcp/tools/utilities/__tests__/clean.test.ts(1 hunks)src/test-utils/mock-executors.ts(1 hunks)src/utils/CommandExecutor.ts(1 hunks)src/utils/FileSystemExecutor.ts(1 hunks)src/utils/__tests__/simulator-utils.test.ts(1 hunks)src/utils/__tests__/typed-tool-factory.test.ts(1 hunks)src/utils/command.ts(1 hunks)src/utils/index.ts(1 hunks)src/utils/template-manager.ts(1 hunks)src/utils/validation.ts(1 hunks)
💤 Files with no reviewable changes (2)
- TOOL_NAMING_VERIFICATION_2025-08-14_22-13.md
- TOOL_NAMING_VERIFICATION_2025-08-14_22-13.md.bak
✅ Files skipped from review due to trivial changes (21)
- src/mcp/tools/swift-package/tests/swift_package_clean.test.ts
- src/mcp/tools/simulator-management/tests/set_sim_appearance.test.ts
- src/mcp/tools/device/tests/list_devices.test.ts
- src/mcp/tools/simulator/tests/build_sim.test.ts
- src/mcp/tools/logging/tests/start_sim_log_cap.test.ts
- src/mcp/tools/ui-testing/tests/touch.test.ts
- src/mcp/tools/ui-testing/tests/swipe.test.ts
- src/mcp/tools/simulator/tests/install_app_sim.test.ts
- src/mcp/tools/project-scaffolding/tests/scaffold_ios_project.test.ts
- src/mcp/tools/simulator-management/tests/set_sim_location.test.ts
- src/mcp/tools/logging/tests/start_device_log_cap.test.ts
- src/mcp/tools/ui-testing/tests/long_press.test.ts
- src/mcp/resources/tests/devices.test.ts
- src/mcp/tools/logging/tests/stop_device_log_cap.test.ts
- src/mcp/tools/project-discovery/tests/get_mac_bundle_id.test.ts
- src/mcp/tools/macos/tests/build_run_macos.test.ts
- src/mcp/tools/project-discovery/tests/list_schemes.test.ts
- src/mcp/tools/project-discovery/tests/get_app_bundle_id.test.ts
- src/mcp/resources/tests/doctor.test.ts
- src/mcp/tools/device/tests/get_device_app_path.test.ts
- src/mcp/tools/ui-testing/tests/screenshot.test.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/utils/CommandExecutor.ts (1)
src/utils/command.ts (2)
CommandExecutor(20-20)CommandResponse(20-20)
src/utils/FileSystemExecutor.ts (1)
src/utils/command.ts (1)
FileSystemExecutor(21-21)
src/test-utils/mock-executors.ts (2)
src/utils/CommandExecutor.ts (1)
CommandExecutor(6-12)src/utils/FileSystemExecutor.ts (1)
FileSystemExecutor(5-16)
🪛 LanguageTool
docs/TOOLS.md
[grammar] ~30-~30: There might be a mistake here.
Context: ...pp in an iOS simulator by UUID or name. If simulator window isn't visible, use ope...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...e session and returns the captured logs. ### macOS Development (macos) Purpose:...
(QB_NEW_EN)
[grammar] ~105-~105: There might be a mistake here.
Context: ...nonical tools + 22 re-exports = 81 total - Workflow Groups: 12 --- *This docume...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: claude-review
🔇 Additional comments (37)
docs/TOOLS.md (2)
21-22: Verify section separation – headings may be rendered inside the preceding list.There’s no blank line between the last bullet of the previous section and this
### iOS Simulator Developmentheading. If you regenerate after fixing the updater script to add a trailing newline per workflow, this should resolve globally.Would you like me to open a follow-up to regenerate docs after merging the script fix?
47-47: Rename acknowledgment looks good.The doc rename to get_mac_app_path aligns with the code refactor and improves consistency with iOS counterpart naming.
src/mcp/tools/device/__tests__/launch_app_device.test.ts (1)
12-12: Good migration to test-only mocks.Switching to test-utils/mock-executors removes test dependency on production executors and helps avoid “module already linked” issues in CI.
src/mcp/tools/simulator-management/__tests__/sim_statusbar.test.ts (1)
9-9: Nice: tests now use centralized mock executors.This aligns tests with the new architecture and keeps production code out of the test environment.
src/utils/validation.ts (1)
26-26: Use a type-only import with the.jsspecifier for FileSystemExecutorReplace the runtime
.tsimport with a type‐only import from the.jsmodule:- import { FileSystemExecutor } from './FileSystemExecutor.ts'; + import type { FileSystemExecutor } from './FileSystemExecutor.js';Since
FileSystemExecutoris an interface (no runtime value), this prevents importing a.tsfile at runtime and keeps our ESM bundles clean. For consistency and to avoid similar runtime‐resolution issues, consider applying the same change wherever you import executor types (e.g. insrc/utils/command.ts,src/utils/template-manager.ts, and your test‐utils). Ensure your TypeScript setup (>=4.5) supports type‐only imports from.jsspecifiers. [optional_refactors_recommended]src/utils/__tests__/simulator-utils.test.ts (1)
3-3: LGTM: tests now depend on centralized test-utils mocksSwitching to ../../test-utils/mock-executors.js decouples tests from production utils and aligns with the new architecture. Nice.
src/mcp/tools/simulator-management/__tests__/reset_sim_location.test.ts (1)
4-4: LGTM: import migrated to shared test-utilsThis aligns tests to the new mock executors module and reduces production coupling.
src/mcp/resources/__tests__/simulators.test.ts (1)
5-5: LGTM: migrate to test-utils mocksThis keeps the resource tests independent from production command infrastructure as intended in this PR.
src/mcp/tools/device/__tests__/install_app_device.test.ts (1)
8-8: Import moved to test-utils – decoupled tests from production codeVerified that
test-utilsimports don’t appear outside of test directories. This change centralizes test-only mocks and prevents accidental production imports, which should help avoid “module already linked” CI failures. LGTM.src/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.ts (1)
13-13: Centralized mock import – good boundary hygieneSwitching to test-utils keeps the utils barrel clean and avoids cross-linking test code into production modules. No change needed.
src/mcp/tools/macos/__tests__/launch_mac_app.test.ts (1)
12-12: Use of createMockFileSystemExecutor from test-utils is appropriateThis keeps filesystem mocking clearly scoped to tests and removes implicit coupling with production executors. Looks good.
src/mcp/tools/ui-testing/__tests__/describe_ui.test.ts (1)
7-7: Import migrated to test-utils – aligns with refactor goalsThe tests now depend on dedicated mock factories instead of production exports, which should reduce CI linking noise. ✅
src/mcp/tools/simulator/__tests__/launch_app_sim.test.ts (1)
3-3: Import updated to test-utils – good separationNice cleanup. This reduces the risk of re-linking production utils during test runs.
src/mcp/tools/macos/__tests__/get_mac_app_path.test.ts (1)
8-8: Good decoupling: switch to test-utils mocks avoids production coupling in testsImporting mocks from test-utils aligns with the new boundaries and eliminates accidental server/utils linkage in test runs. The type-only import also keeps runtime clean.
src/mcp/tools/swift-package/__tests__/swift_package_test.test.ts (1)
12-12: LGTM: centralized mock importConsistent with the new test-utils module; helps keep test dependencies hermetic.
src/mcp/tools/device/__tests__/build_device.test.ts (1)
8-8: LGTM: import now correctly targets test-only mocksThis aligns with the new architecture and should help prevent the CI “module already linked” issues.
src/mcp/tools/simulator/__tests__/screenshot.test.ts (1)
10-13: Mocks Centralized and Verified
All test executors are now consistently imported fromsrc/test-utils/mock-executors.ts, and production code remains unaffected:
src/test-utils/mock-executors.tsexports all four factories:
•createMockExecutor
•createNoopExecutor
•createCommandMatchingMockExecutor
•createMockFileSystemExecutor- No tests still import from
utils/command.- Every test under
__tests__referencestest-utils/mock-executors.js.- No non-test code imports from
test-utils/mock-executors.This cleanly decouples test mocks from production utilities and prevents the CI “module already linked” errors. Approving these changes.
src/mcp/tools/ui-testing/__tests__/button.test.ts (1)
7-7: LGTM: switched to centralized test mocks.Importing mocks from test-utils is consistent with the new architecture and reduces coupling with production utilities.
src/mcp/tools/swift-package/__tests__/swift_package_build.test.ts (1)
9-12: LGTM: tests now depend on test-utils/mock-executors.This keeps tests hermetic and avoids pulling in prod command modules.
src/mcp/tools/project-discovery/__tests__/show_build_settings.test.ts (1)
3-3: LGTM: import path migration to test-utils.Clean separation of test helpers from production code.
src/mcp/tools/ui-testing/__tests__/key_sequence.test.ts (1)
7-7: LGTM: mocks sourced from test-utils.Consistent with the new module boundaries and should help prevent cross-environment linking issues in CI.
src/mcp/tools/device/__tests__/stop_app_device.test.ts (1)
8-8: Import moved to test-utils looks correct and aligns with the new boundariesPath depth is correct and the switch to test-only mocks decouples the test from production utilities. API usage remains unchanged.
src/mcp/tools/macos/__tests__/test_macos.test.ts (1)
8-8: Import updated to test-utils: good moveThis keeps the test harness isolated from production modules and matches the new architecture.
src/mcp/tools/ui-testing/__tests__/tap.test.ts (1)
7-7: Import source switch to test-utils confirmedCorrect relative path and consistent use with the new test scaffolding.
src/mcp/tools/simulator/__tests__/build_run_sim.test.ts (1)
8-11: Import path migration to test-utils looks goodSwitching to centralized test mocks decouples tests from production executors and aligns with the new module boundaries.
src/mcp/tools/macos/__tests__/build_macos.test.ts (1)
10-10: LGTM on moving mocks to test-utilsThis keeps tests isolated from production modules and supports the architectural refactor.
src/mcp/tools/ui-testing/__tests__/key_press.test.ts (1)
11-11: LGTM on switching to centralized mocksImporting from test-utils/mock-executors.js keeps tests pure and avoids linking production modules during CI, which matches the PR goals.
src/mcp/tools/simulator/__tests__/stop_app_sim.test.ts (1)
7-7: LGTM: migrated mocks to test-utils to prevent prod/test coupling.Nice move—this directly targets the “module already linked” CI flake by removing test imports from prod barrels.
src/mcp/tools/utilities/__tests__/clean.test.ts (1)
3-3: LGTM: updated to centralized test mocks.Importing from test-utils/mock-executors keeps tests isolated from production code and aligns with the new boundaries.
src/mcp/tools/project-scaffolding/__tests__/scaffold_macos_project.test.ts (1)
17-17: LGTM: test mocks sourced from test-utils.This matches the architecture refactor and reduces the chance of cross-environment side effects during CI.
src/mcp/tools/project-discovery/__tests__/discover_projs.test.ts (1)
13-13: LGTM: imports switched to test-utils.Consistent with the repo-wide migration; keeps the test surface clean and deterministic.
src/mcp/tools/device/__tests__/test_device.test.ts (1)
9-12: Good move: decouple tests from production command moduleSwitching to test-utils/mock-executors keeps tests pure and prevents accidental coupling to server/runtime code. No issues spotted.
src/utils/__tests__/typed-tool-factory.test.ts (1)
8-8: LGTM: migrate to centralized test mocksImporting createMockExecutor from test-utils/mock-executors is consistent with the new boundaries. Keeps this test side-effect-free.
src/mcp/tools/simulator/__tests__/list_sims.test.ts (1)
3-6: LGTM: centralized mocks importUsing test-utils/mock-executors across tests aligns with the new architecture and avoids production module linkage during CI. Looks good.
src/mcp/tools/simulator/__tests__/open_sim.test.ts (1)
9-9: LGTM: type-only import aligns with the new test-utils boundaryUsing
type CommandExecutorfrom the centralizedtest-utils/mock-executors.jsis exactly what we want post refactor.src/mcp/tools/logging/__tests__/stop_sim_log_cap.test.ts (1)
17-17: LGTM: tests now consume mocks from test-utilsImporting
createMockFileSystemExecutorfrom the centralized test-utils module matches the new architecture and keeps tests decoupled from production code.src/utils/command.ts (1)
201-229: Great safety checks for testsGuarding default executors in test environments prevents accidental real I/O during tests. Messaging is clear and actionable.
| - `stop_app_sim` - Stops an app running in an iOS simulator by UUID or name. or stop_app_sim({ simulatorName: "iPhone 16", bundleId: "com.example.MyApp" }) | ||
| - `test_sim` - Runs tests on a simulator by UUID or name using xcodebuild test and parses xcresult output. Works with both Xcode projects (.xcodeproj) and workspaces (.xcworkspace). |
There was a problem hiding this comment.
Fix stray “or” and add “Example:” label for stop_app_sim.
Keeps descriptions consistent across tools.
-- `stop_app_sim` - Stops an app running in an iOS simulator by UUID or name. or stop_app_sim({ simulatorName: "iPhone 16", bundleId: "com.example.MyApp" })
+- `stop_app_sim` - Stops an app running in an iOS simulator by UUID or name. Example: stop_app_sim({ simulatorName: "iPhone 16", bundleId: "com.example.MyApp" })📝 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.
| - `stop_app_sim` - Stops an app running in an iOS simulator by UUID or name. or stop_app_sim({ simulatorName: "iPhone 16", bundleId: "com.example.MyApp" }) | |
| - `test_sim` - Runs tests on a simulator by UUID or name using xcodebuild test and parses xcresult output. Works with both Xcode projects (.xcodeproj) and workspaces (.xcworkspace). | |
| - `stop_app_sim` - Stops an app running in an iOS simulator by UUID or name. Example: stop_app_sim({ simulatorName: "iPhone 16", bundleId: "com.example.MyApp" }) | |
| - `test_sim` - Runs tests on a simulator by UUID or name using xcodebuild test and parses xcresult output. Works with both Xcode projects (.xcodeproj) and workspaces (.xcworkspace). |
🤖 Prompt for AI Agents
In docs/TOOLS.md around lines 33 to 34, the stop_app_sim entry contains a stray
"or" and its example lacks an "Example:" label; update the stop_app_sim bullet
to remove the extra "or" so it reads as a single sentence, and add an "Example:"
label before the usage (keeping the example format consistent with other tool
entries), ensuring punctuation and casing match the test_sim line style.
| } | ||
|
|
||
| return content + '\n'; | ||
| return content; |
There was a problem hiding this comment.
Fix Markdown section breaks: ensure a blank line between tool lists and the next header.
Returning content without a trailing newline can cause the following ### header to be swallowed by the preceding list item on some renderers (including GitHub). Restore the trailing newline to keep headings rendering correctly.
- return content;
+ return content + '\n';📝 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.
| return content; | |
| return content + '\n'; |
🤖 Prompt for AI Agents
In scripts/update-tools-docs.ts around line 95, the function returns content
without a trailing newline which can cause the next "###" header to be rendered
as part of the previous list; fix this by ensuring the content string ends with
a single newline before returning (e.g., append "\n" if missing) so there is a
blank line between the tool list and the following header.
|
|
||
| import { describe, it, expect } from 'vitest'; | ||
| import { createMockExecutor } from '../../../../utils/command.js'; | ||
| import { createMockExecutor } from '../../../../test-utils/mock-executors.js'; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Sanity-check for leftover legacy imports and TS ESM config
To ensure the refactor is complete and stable across environments, verify there are no remaining imports from the old utils/command.js and that TS ESM settings support mixed .ts/.js extension imports used in tests.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Searching for legacy imports of utils/command.js (expect: no matches)"
rg -nP --type=ts -C0 "from\s+['\"][./]+.*utils/command\.js['\"]" || true
echo
echo "2) Count of current imports of test-utils/mock-executors.(ts|js) (informational)"
rg -nP --type=ts -C0 "from\s+['\"][./]+.*test-utils/mock-executors\.(ts|js)['\"]" | wc -l
echo
echo "3) Checking tsconfig settings related to ESM and extension imports (NodeNext + allowImportingTsExtensions recommended)"
fd -HI --glob "tsconfig*.json" | while read -r f; do
echo "== $f =="
jq '{module: .compilerOptions.module, moduleResolution: .compilerOptions.moduleResolution, allowImportingTsExtensions: .compilerOptions.allowImportingTsExtensions, verbatimModuleSyntax: .compilerOptions.verbatimModuleSyntax, esModuleInterop: .compilerOptions.esModuleInterop}' "$f" || true
done
echo
echo "Expected:"
echo "- No results in step 1."
echo "- module: \"NodeNext\" and allowImportingTsExtensions: true (or an equivalent setup that supports .ts and .js extension imports in tests)."Length of output: 5173
Remove leftover legacy utils/command.js imports & align TS ESM configuration
-
Legacy imports detected in 22 files (run
rg "utils/command\\.js"for full list), for example:
• src/mcp/tools/project-discovery/get_mac_bundle_id.ts:14
• src/mcp/tools/project-discovery/get_app_bundle_id.ts:15
• src/mcp/tools/project-discovery/discover_projs.ts:12
• src/core/dynamic-tools.ts:2
• src/mcp/tools/logging/stop_device_log_cap.ts:13
…and 17 more.
Please replace all of these with the new executor import (e.g.from '../../../../test-utils/mock-executors.js'or its.tsvariant). -
TSConfig updates required to support mixed-extension imports under ESM:
In both tsconfig.json and tsconfig.test.json, undercompilerOptionsadd or update:{ "compilerOptions": { "module": "NodeNext", "moduleResolution": "NodeNext", "allowImportingTsExtensions": true // preserve other flags (e.g. esModuleInterop) as needed } }
🤖 Prompt for AI Agents
In src/mcp/tools/device/__tests__/stop_app_device.test.ts around line 8, the
test is importing the legacy utils/command.js; replace that legacy import with
the new mock executor import (e.g. from
'../../../../test-utils/mock-executors.js' or the .ts variant) to match the
other updated files, and then update tsconfig.json and tsconfig.test.json under
compilerOptions to set "module": "NodeNext", "moduleResolution": "NodeNext", and
"allowImportingTsExtensions": true so TypeScript ESM mixed-extension imports
work correctly; preserve all other existing compiler flags.
| import { getDefaultFileSystemExecutor, getDefaultCommandExecutor } from '../../../utils/command.js'; | ||
| import { FileSystemExecutor } from '../../../utils/FileSystemExecutor.ts'; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix ESM import: use .js extension and type-only import for FileSystemExecutor
Importing from a .ts path will break at runtime under Node ESM once transpiled. Also, FileSystemExecutor is a type; use import type to avoid emitting a runtime import (especially if verbatimModuleSyntax is enabled).
Apply this diff:
-import { FileSystemExecutor } from '../../../utils/FileSystemExecutor.ts';
+import type { FileSystemExecutor } from '../../../utils/FileSystemExecutor.js';Run this to find any remaining .ts path imports for FileSystemExecutor:
🏁 Script executed:
#!/bin/bash
# Find imports that reference FileSystemExecutor.ts (should be .js)
rg -nP -C2 "from ['\"][^'\"]*FileSystemExecutor\.ts['\"]"Length of output: 5588
Fix ESM import paths and use import type for FileSystemExecutor
Node ESM requires .js extensions at runtime, and since FileSystemExecutor is a type, it should be imported with import type to avoid emitting a runtime import (especially when verbatimModuleSyntax is enabled). Please update all imports of FileSystemExecutor.ts in the following files:
- src/utils/command.ts (line 16)
- src/utils/validation.ts (line 26)
- src/utils/template-manager.ts (line 7)
- src/test-utils/mock-executors.ts (line 20)
- src/mcp/tools/project-discovery/discover_projs.ts (line 13)
- src/mcp/tools/project-discovery/get_app_bundle_id.ts (line 16)
- src/mcp/tools/project-discovery/get_mac_bundle_id.ts (line 15)
- src/mcp/tools/device/test_device.ts (line 19)
- src/mcp/tools/macos/test_macos.ts (line 19)
- src/mcp/tools/macos/launch_mac_app.ts (line 13)
- src/mcp/tools/project-scaffolding/scaffold_macos_project.ts (line 18)
- src/mcp/tools/logging/stop_device_log_cap.ts (line 14)
For each, replace:
-import { FileSystemExecutor } from '…/FileSystemExecutor.ts';
+import type { FileSystemExecutor } from '…/FileSystemExecutor.js';📝 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.
| import { getDefaultFileSystemExecutor, getDefaultCommandExecutor } from '../../../utils/command.js'; | |
| import { FileSystemExecutor } from '../../../utils/FileSystemExecutor.ts'; | |
| import { getDefaultFileSystemExecutor, getDefaultCommandExecutor } from '../../../utils/command.js'; | |
| import type { FileSystemExecutor } from '../../../utils/FileSystemExecutor.js'; |
🤖 Prompt for AI Agents
In src/mcp/tools/project-discovery/discover_projs.ts around lines 12 to 13, the
import of FileSystemExecutor is using a .ts path and a runtime import; update
the imports to use Node ESM-safe .js module specifiers and import the
FileSystemExecutor only as a type: change any direct import of
'../../../utils/FileSystemExecutor.ts' to use "import type { FileSystemExecutor
} from '../../../utils/FileSystemExecutor.js';" and ensure other runtime imports
keep their .js extensions (e.g.,
getDefaultFileSystemExecutor/getDefaultCommandExecutor) so no .ts paths or
non-ESM-safe imports remain.
| getDefaultFileSystemExecutor, | ||
| getDefaultCommandExecutor, | ||
| } from '../../../utils/command.js'; | ||
| import { FileSystemExecutor } from '../../../utils/FileSystemExecutor.ts'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid importing .ts files at runtime; use .js specifier and type-only import
Align with your ESM import style and prevent accidental runtime imports of types.
-import { FileSystemExecutor } from '../../../utils/FileSystemExecutor.ts';
+import type { FileSystemExecutor } from '../../../utils/FileSystemExecutor.js';📝 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.
| import { FileSystemExecutor } from '../../../utils/FileSystemExecutor.ts'; | |
| import type { FileSystemExecutor } from '../../../utils/FileSystemExecutor.js'; |
🤖 Prompt for AI Agents
In src/mcp/tools/project-discovery/get_app_bundle_id.ts around line 16, the
current import pulls a .ts module which can cause runtime ESM resolution issues;
change the import to use the .js specifier and make it a type-only import if you
only need types (e.g., import type { FileSystemExecutor } from
'../../../utils/FileSystemExecutor.js'), or if you require runtime values import
from the .js file without the type modifier so the runtime loader resolves
correctly; update the import path and modifier accordingly and ensure any
build/tsconfig settings support preserving .js extensions.
| import { describe, it, expect, beforeEach } from 'vitest'; | ||
| import { z } from 'zod'; | ||
| import { createMockExecutor, createNoopExecutor } from '../../../../utils/command.js'; | ||
| import { createMockExecutor, createNoopExecutor } from '../../../../test-utils/mock-executors.js'; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm there are no lingering imports from utils/command.js and no production imports of test-utils
The import move looks correct. Run the following to verify the migration is complete and type re-exports are in place:
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "1) Finding any test files still importing utils/command.js..."
rg -nP -C1 '(from|require\()\s*[\'"]\.{1,4}/(?:\.{1,2}/)*utils/command\.js[\'"]' --type=ts --type=tsx --type=js --type=jsx src || true
echo
echo "2) Ensuring production code does not import test-utils..."
rg -nP 'test-utils/mock-executors' -g '!**/__tests__/**' -g '!**/*.test.*' -g '!**/test-utils/**' src || true
echo
echo "3) Checking that test-utils/mock-executors exports CommandExecutor type..."
rg -nP -C2 'export\s+(?:type\s+)?\{?\s*CommandExecutor\b' src/test-utils/mock-executors.ts || true
echo
echo "4) Showing the CommandExecutor type declaration in utils (for reference)..."
rg -nP -C2 '^\s*export\s+type\s+CommandExecutor\b|^\s*export\s+interface\s+CommandExecutor\b' src/utils --type=ts || trueLength of output: 263
The previous script failed due to quoting issues around the regex. Please rerun with the corrected patterns:
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "1) Finding any test files still importing utils/command.js..."
rg -nP -C1 "(from|require\\()\\s*['\"]\\.{1,4}/(?:\\.{1,2}/)*utils/command\\.js['\"]" --type=ts --type=tsx --type=js --type=jsx src || true
echo
echo "2) Ensuring production code does not import test-utils..."
rg -nP "test-utils/mock-executors" -g '!**/__tests__/**' -g '!**/*.test.*' -g '!**/test-utils/**' src || true
echo
echo "3) Checking that test-utils/mock-executors exports CommandExecutor type..."
rg -nP -C2 "export\s+(?:type\s+)?\{?\s*CommandExecutor\b" src/test-utils/mock-executors.ts || true
echo
echo "4) Showing the CommandExecutor type declaration in utils (for reference)..."
rg -nP -C2 "^\s*export\s+type\s+CommandExecutor\b|^\s*export\s+interface\s+CommandExecutor\b" src/utils --type=ts || trueLength of output: 1407
To verify that the migration is fully complete, let’s inspect the test-utils file and rerun the import checks without relying on unsupported file-type flags:
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "1) Searching for any tests still importing utils/command.js..."
rg -nP "utils/command\\.js" -g "__tests__/**" -g "*.test.*" src || echo "✅ No utils/command.js imports found in tests"
echo
echo "2) Ensuring no production code imports test-utils/mock-executors..."
rg -nP "test-utils/mock-executors" \
-g '!**/__tests__/**' \
-g '!**/*.test.*' \
-g '!**/test-utils/**' src \
|| echo "✅ No production imports of test-utils/mock-executors"
echo
echo "3) Listing all exports in src/test-utils/mock-executors.ts…"
rg -nP "^export " -n src/test-utils/mock-executors.ts || echo "⚠️ No exports found"
echo
echo "4) Looking for CommandExecutor references in mock-executors.ts…"
rg -nP "CommandExecutor" -n src/test-utils/mock-executors.ts || echo "⚠️ No CommandExecutor reference found"Length of output: 1878
Add CommandExecutor type re-export to mock-executors
All migration checks passed—no leftover utils/command.js imports in tests and no production code importing test-utils. The only missing piece is re-exporting the CommandExecutor type from your mock file so consumers can import it directly:
• File: src/test-utils/mock-executors.ts
Add at the top:
export type { CommandExecutor } from '../utils/CommandExecutor.ts';🤖 Prompt for AI Agents
In src/test-utils/mock-executors.ts (add at file top), re-export the
CommandExecutor type so tests can import it directly; add an export type line
that re-exports CommandExecutor from ../utils/CommandExecutor.ts (i.e. export
type { CommandExecutor } from '../utils/CommandExecutor.ts';) and save the file
so consumers can import the type from the mock-executors module.
| import { ChildProcess } from 'child_process'; | ||
| import { CommandExecutor } from '../utils/CommandExecutor.ts'; | ||
| import { FileSystemExecutor } from '../utils/FileSystemExecutor.ts'; | ||
|
|
There was a problem hiding this comment.
Fix ESM specifiers and use type-only imports for types
Like in utils/command.ts, importing from .ts paths will break at runtime. Also mark imports as type-only to avoid runtime loads.
Apply this diff:
-import { ChildProcess } from 'child_process';
-import { CommandExecutor } from '../utils/CommandExecutor.ts';
-import { FileSystemExecutor } from '../utils/FileSystemExecutor.ts';
+import type { ChildProcess } from 'child_process';
+import type { CommandExecutor } from '../utils/CommandExecutor.js';
+import type { FileSystemExecutor } from '../utils/FileSystemExecutor.js';📝 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.
| import { ChildProcess } from 'child_process'; | |
| import { CommandExecutor } from '../utils/CommandExecutor.ts'; | |
| import { FileSystemExecutor } from '../utils/FileSystemExecutor.ts'; | |
| import type { ChildProcess } from 'child_process'; | |
| import type { CommandExecutor } from '../utils/CommandExecutor.js'; | |
| import type { FileSystemExecutor } from '../utils/FileSystemExecutor.js'; |
🤖 Prompt for AI Agents
In src/test-utils/mock-executors.ts around lines 18 to 21, the imports use .ts
specifiers and are not marked type-only which can break ESM runtime loading and
cause unnecessary runtime imports; change them to ESM-safe specifiers and
type-only imports (e.g., use "import type { ChildProcess } from
'child_process';" and "import type { CommandExecutor } from
'../utils/CommandExecutor';" and "import type { FileSystemExecutor } from
'../utils/FileSystemExecutor';") so the compiler keeps them as types only and
the runtime does not attempt to load .ts modules.
| if (result instanceof Error || typeof result === 'string') { | ||
| return async () => { | ||
| throw result; | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Throw Error instances, not strings
Throwing a string loses stack and type information. Wrap string results in an Error.
Apply this diff:
- if (result instanceof Error || typeof result === 'string') {
+ if (result instanceof Error || typeof result === 'string') {
return async () => {
- throw result;
+ if (result instanceof Error) throw result;
+ throw new Error(result);
};
}📝 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 (result instanceof Error || typeof result === 'string') { | |
| return async () => { | |
| throw result; | |
| }; | |
| } | |
| if (result instanceof Error || typeof result === 'string') { | |
| return async () => { | |
| if (result instanceof Error) throw result; | |
| throw new Error(result); | |
| }; | |
| } |
🤖 Prompt for AI Agents
In src/test-utils/mock-executors.ts around lines 39 to 43, the code may throw a
string which loses stack and type information; change the returned async
function to ensure it always throws an Error instance by wrapping string results
in new Error(result) while leaving existing Error objects untouched (i.e., if
result is an Error re-throw it, otherwise construct and throw new
Error(result.toString())).
src/utils/command.ts
Outdated
| import { FileSystemExecutor } from './FileSystemExecutor.ts'; | ||
| import { CommandExecutor, CommandResponse } from './CommandExecutor.ts'; | ||
|
|
||
| /** | ||
| * Command execution response interface | ||
| */ | ||
| export interface CommandResponse { | ||
| success: boolean; | ||
| output: string; | ||
| error?: string; | ||
| process: ChildProcess; | ||
| } | ||
|
|
||
| /** | ||
| * Command executor function type for dependency injection | ||
| */ | ||
| export type CommandExecutor = ( | ||
| command: string[], | ||
| logPrefix?: string, | ||
| useShell?: boolean, | ||
| env?: Record<string, string>, | ||
| detached?: boolean, | ||
| ) => Promise<CommandResponse>; | ||
|
|
||
| /** | ||
| * File system executor interface for dependency injection | ||
| */ | ||
| export interface FileSystemExecutor { | ||
| mkdir(path: string, options?: { recursive?: boolean }): Promise<void>; | ||
| readFile(path: string, encoding?: BufferEncoding): Promise<string>; | ||
| writeFile(path: string, content: string, encoding?: BufferEncoding): Promise<void>; | ||
| cp(source: string, destination: string, options?: { recursive?: boolean }): Promise<void>; | ||
| readdir(path: string, options?: { withFileTypes?: boolean }): Promise<unknown[]>; | ||
| rm(path: string, options?: { recursive?: boolean; force?: boolean }): Promise<void>; | ||
| existsSync(path: string): boolean; | ||
| stat(path: string): Promise<{ isDirectory(): boolean }>; | ||
| mkdtemp(prefix: string): Promise<string>; | ||
| tmpdir(): string; | ||
| } | ||
| // Re-export types for backward compatibility | ||
| export { CommandExecutor, CommandResponse } from './CommandExecutor.js'; | ||
| export { FileSystemExecutor } from './FileSystemExecutor.js'; |
There was a problem hiding this comment.
Fix ESM import specifiers and use type-only imports for types
Importing from .ts paths will break at runtime in Node ESM builds. Also, use type-only imports to avoid runtime loading. The re-exports are correctly pointing at .js—mirror that for local type imports.
Apply this diff:
-import { FileSystemExecutor } from './FileSystemExecutor.ts';
-import { CommandExecutor, CommandResponse } from './CommandExecutor.ts';
+import type { FileSystemExecutor } from './FileSystemExecutor.js';
+import type { CommandExecutor, CommandResponse } from './CommandExecutor.js';📝 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.
| import { FileSystemExecutor } from './FileSystemExecutor.ts'; | |
| import { CommandExecutor, CommandResponse } from './CommandExecutor.ts'; | |
| /** | |
| * Command execution response interface | |
| */ | |
| export interface CommandResponse { | |
| success: boolean; | |
| output: string; | |
| error?: string; | |
| process: ChildProcess; | |
| } | |
| /** | |
| * Command executor function type for dependency injection | |
| */ | |
| export type CommandExecutor = ( | |
| command: string[], | |
| logPrefix?: string, | |
| useShell?: boolean, | |
| env?: Record<string, string>, | |
| detached?: boolean, | |
| ) => Promise<CommandResponse>; | |
| /** | |
| * File system executor interface for dependency injection | |
| */ | |
| export interface FileSystemExecutor { | |
| mkdir(path: string, options?: { recursive?: boolean }): Promise<void>; | |
| readFile(path: string, encoding?: BufferEncoding): Promise<string>; | |
| writeFile(path: string, content: string, encoding?: BufferEncoding): Promise<void>; | |
| cp(source: string, destination: string, options?: { recursive?: boolean }): Promise<void>; | |
| readdir(path: string, options?: { withFileTypes?: boolean }): Promise<unknown[]>; | |
| rm(path: string, options?: { recursive?: boolean; force?: boolean }): Promise<void>; | |
| existsSync(path: string): boolean; | |
| stat(path: string): Promise<{ isDirectory(): boolean }>; | |
| mkdtemp(prefix: string): Promise<string>; | |
| tmpdir(): string; | |
| } | |
| // Re-export types for backward compatibility | |
| export { CommandExecutor, CommandResponse } from './CommandExecutor.js'; | |
| export { FileSystemExecutor } from './FileSystemExecutor.js'; | |
| import type { FileSystemExecutor } from './FileSystemExecutor.js'; | |
| import type { CommandExecutor, CommandResponse } from './CommandExecutor.js'; | |
| // Re-export types for backward compatibility | |
| export { CommandExecutor, CommandResponse } from './CommandExecutor.js'; | |
| export { FileSystemExecutor } from './FileSystemExecutor.js'; |
🤖 Prompt for AI Agents
In src/utils/command.ts around lines 16 to 21, the file currently imports from
.ts specifiers which will break under Node ESM and performs normal imports for
types; change those imports to type-only imports that reference the .js
specifiers (e.g. import type { FileSystemExecutor } from
'./FileSystemExecutor.js' and import type { CommandExecutor, CommandResponse }
from './CommandExecutor.js') so no runtime modules are loaded and the import
paths match the re-exports.
src/utils/template-manager.ts
Outdated
| import { CommandExecutor } from './command.js'; | ||
| import { FileSystemExecutor } from './FileSystemExecutor.ts'; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Use .js extension (or barrel) for internal imports to avoid ESM resolution pitfalls
Similar to other imports in this repo, prefer importing with .js. Importing .ts can break under NodeNext unless you explicitly enable TS extension imports.
Apply this diff:
-import { CommandExecutor } from './command.js';
-import { FileSystemExecutor } from './FileSystemExecutor.ts';
+import { CommandExecutor } from './command.js';
+import { FileSystemExecutor } from './FileSystemExecutor.js';Verification script (optional):
🏁 Script executed:
#!/bin/bash
# Show imports using .ts extensions in utils
rg -nP "from\s+['\"][./].*\.ts['\"]" src/utils --type=ts -SLength of output: 431
Use .js extension for internal ESM imports in src/utils
To avoid NodeNext resolution issues, all imports of local helpers should reference the .js output files, not .ts.
Files needing updates:
- src/utils/template-manager.ts (line 7): change
./FileSystemExecutor.ts→./FileSystemExecutor.js - src/utils/validation.ts (line 26): change
./FileSystemExecutor.ts→./FileSystemExecutor.js - src/utils/command.ts (lines 16–17): change
./FileSystemExecutor.ts→./FileSystemExecutor.js./CommandExecutor.ts→./CommandExecutor.js
Example diff for template-manager.ts:
--- a/src/utils/template-manager.ts
+++ b/src/utils/template-manager.ts
@@ -6,7 +6,7 @@
import { CommandExecutor } from './command.js';
-import { FileSystemExecutor } from './FileSystemExecutor.ts';
+import { FileSystemExecutor } from './FileSystemExecutor.js';📝 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.
| import { CommandExecutor } from './command.js'; | |
| import { FileSystemExecutor } from './FileSystemExecutor.ts'; | |
| import { CommandExecutor } from './command.js'; | |
| import { FileSystemExecutor } from './FileSystemExecutor.js'; |
🤖 Prompt for AI Agents
In src/utils/template-manager.ts around lines 6 to 7, the local ESM import uses
a .ts extension which can break NodeNext resolution; change the import path from
./FileSystemExecutor.ts to ./FileSystemExecutor.js so internal imports reference
the emitted .js files; similarly update the other listed files
(src/utils/validation.ts line 26 and src/utils/command.ts lines 16–17) to use
.js extensions for their local imports.
This commit implements a comprehensive architectural improvement that replaces the previous barrel import pattern (importing everything from utils/index.ts) with focused facades that expose specific functionality through dedicated index.ts files in subdirectories. Key Changes: - Created 11 focused facade modules in src/utils/ (execution, logging, responses, validation, axe, plugin-registry, xcodemake, template, version, test, log-capture) - Migrated 65+ tool files from barrel imports to focused facade imports - Added ESLint rule to prevent regression to barrel imports - Updated ARCHITECTURE.md to document the new module organization pattern Performance Impact: - Eliminates loading of unused modules, reducing startup time - Improves module resolution speed for Node.js runtime - Prevents circular dependency issues that were risks with large barrel files - Enables better tree-shaking for bundlers Architecture Benefits: - Clear dependency graph - imports explicitly show what functionality is needed - Reduced coupling between unrelated utility components - Better maintainability and code organization - Type-safe imports with focused interfaces
|
Claude finished @cameroncooke's task —— View job PR Review: Architectural RefactoringThis is an excellent architectural refactoring that successfully addresses the CI module linking issues while delivering substantial code quality improvements. The changes are well-structured, thoroughly implemented, and demonstrate strong engineering principles. Review Progress
✅ Strengths1. Architectural Excellence
2. CI Issue Resolution
3. Testing Architecture Improvements
4. Code Quality Enhancements
✅ Technical Implementation Quality1. Interface Design// Clean, well-designed interfaces
export type CommandExecutor = (
command: string[],
logPrefix?: string,
useShell?: boolean,
env?: Record<string, string>,
detached?: boolean,
) => Promise<CommandResponse>;2. Test Safety Mechanisms// Excellent guardrails to prevent production calls in tests
if (process.env.VITEST === 'true' || process.env.NODE_ENV === 'test') {
throw new Error('🚨 REAL SYSTEM EXECUTOR DETECTED IN TEST! 🚨\n...');
}3. Mock Function Completeness
|
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (24)
src/mcp/tools/discovery/discover_tools.ts (1)
24-31: Harden env var parsing to prevent NaN/invalid LLM config.If the env vars are set to non-numeric values, parseInt/parseFloat will yield NaN and propagate to createMessage, likely causing runtime errors. Add guards and reasonable bounds.
Apply this diff:
-const getLLMConfig = (): LLMConfig => ({ - maxTokens: process.env.XCODEBUILDMCP_LLM_MAX_TOKENS - ? parseInt(process.env.XCODEBUILDMCP_LLM_MAX_TOKENS, 10) - : 200, - temperature: process.env.XCODEBUILDMCP_LLM_TEMPERATURE - ? parseFloat(process.env.XCODEBUILDMCP_LLM_TEMPERATURE) - : undefined, -}); +const getLLMConfig = (): LLMConfig => { + const rawMax = process.env.XCODEBUILDMCP_LLM_MAX_TOKENS; + const rawTemp = process.env.XCODEBUILDMCP_LLM_TEMPERATURE; + + let maxTokens = 200; + if (rawMax) { + const parsed = Number.parseInt(rawMax, 10); + if (Number.isFinite(parsed) && parsed > 0) { + // Clamp to a sane upper bound to avoid client errors + maxTokens = Math.min(parsed, 4000); + } else { + log('warn', `Invalid XCODEBUILDMCP_LLM_MAX_TOKENS="${rawMax}", using default ${maxTokens}`); + } + } + + let temperature: number | undefined; + if (rawTemp !== undefined) { + const parsed = Number.parseFloat(rawTemp); + if (Number.isFinite(parsed) && parsed >= 0 && parsed <= 2) { + temperature = parsed; + } else { + log('warn', `Invalid XCODEBUILDMCP_LLM_TEMPERATURE="${rawTemp}", ignoring`); + } + } + + return { maxTokens, temperature }; +};src/mcp/tools/simulator/list_sims.ts (1)
74-83: Ensure all failure returns includeisError: truein list_sims.tsThe
list_simstool currently omitsisError: truein its inner failure paths, unlike other tools. Please update the following sections to return error flags consistently:• src/mcp/tools/simulator/list_sims.ts
– Lines 74–83: on!result.success
– Lines 88–97: when!isSimulatorData(parsedData)
– Lines 135–143: in the innercatchblockApply this patch:
--- a/src/mcp/tools/simulator/list_sims.ts +++ b/src/mcp/tools/simulator/list_sims.ts @@ 74,8 @@ - if (!result.success) { - return { + if (!result.success) { + return { content: [ { type: 'text', text: `Failed to list simulators: ${result.error}`, }, + isError: true, }; @@ 88,8 @@ - if (!isSimulatorData(parsedData)) { - return { + if (!isSimulatorData(parsedData)) { + return { content: [ { type: 'text', text: 'Failed to parse simulator data: Invalid format', }, + isError: true, }; @@ 135,8 @@ - } catch { - return { + } catch { + return { content: [ { type: 'text', text: result.output, }, + isError: true, }; }src/mcp/tools/doctor/doctor.ts (2)
14-15: Fix ESM import: avoid importing a .ts file at runtimeImporting
./lib/doctor.deps.tswill emit a runtime import to a .ts path, which Node cannot resolve in ESM builds. Use the .js extension (TypeScript will type-check against the .d.ts) and keep the type-only binding.-import { type DoctorDependencies, createDoctorDependencies } from './lib/doctor.deps.ts'; +import { type DoctorDependencies, createDoctorDependencies } from './lib/doctor.deps.js';
223-224: Standardize Sentry opt-out env var across the codebaseThe PR objective specifies
XCODEBUILDMCP_SENTRY_DISABLEDas the canonical opt-out flag. We’re still referencing the oldSENTRY_DISABLEDin multiple places, which will lead to incorrect reporting and behavior. Please update all occurrences:• README.md (opt-out docs & examples)
• src/utils/sentry.ts (env var list)
• src/utils/logger.ts (SENTRY_ENABLED flag)
• src/mcp/tools/doctor/lib/doctor.deps.ts (dependency declarations)
• src/mcp/tools/doctor/tests/doctor.test.ts (test fixtures)
• src/mcp/tools/doctor/doctor.ts (doctor output)Apply this change in doctor.ts as an example:
- `\n## Sentry`, - `- Sentry enabled: ${doctorInfo.environmentVariables.SENTRY_DISABLED !== 'true' ? '✅ Yes' : '❌ No'}`, + `\n## Sentry`, + `- Sentry enabled: ${doctorInfo.environmentVariables.XCODEBUILDMCP_SENTRY_DISABLED === 'true' ? '❌ No' : '✅ Yes'}`,After updating, run a global grep for
SENTRY_DISABLEDto confirm no legacy references remain.src/mcp/tools/logging/start_device_log_cap.ts (5)
51-62: Bug: tmpdir mismatch when FileSystemExecutor is injectedYou compute the log path with os.tmpdir() but create the directory via fileSystemExecutor.tmpdir() when a fileSystemExecutor is provided. If the two differ (very likely in tests/mocks), the created path may not exist for the subsequent fs operations, causing ENOENT. Use a single tmpDir source consistently.
Apply this diff to normalize tmpDir usage:
- const logFileName = `${DEVICE_LOG_FILE_PREFIX}${logSessionId}.log`; - const logFilePath = path.join(os.tmpdir(), logFileName); + const logFileName = `${DEVICE_LOG_FILE_PREFIX}${logSessionId}.log`; + const tmpDir = fileSystemExecutor ? fileSystemExecutor.tmpdir() : os.tmpdir(); + const logFilePath = path.join(tmpDir, logFileName); @@ - if (fileSystemExecutor) { - await fileSystemExecutor.mkdir(fileSystemExecutor.tmpdir(), { recursive: true }); - await fileSystemExecutor.writeFile(logFilePath, ''); - } else { - await fs.promises.mkdir(os.tmpdir(), { recursive: true }); - await fs.promises.writeFile(logFilePath, ''); - } + if (fileSystemExecutor) { + await fileSystemExecutor.mkdir(tmpDir, { recursive: true }); + await fileSystemExecutor.writeFile(logFilePath, ''); + } else { + await fs.promises.mkdir(tmpDir, { recursive: true }); + await fs.promises.writeFile(logFilePath, ''); + }
64-69: Resource leak: write stream is never tracked or closedlogStream is created but not managed. It’s not stored for later closure, nor closed on errors. Keep a handle in the session so the stop operation can flush/close it; add an error handler to avoid silent failures.
- const logStream = fs.createWriteStream(logFilePath, { flags: 'a' }); + const logStream = fs.createWriteStream(logFilePath, { flags: 'a' }); + logStream.on('error', (e) => { + log('warn', `Device log stream error for ${logFilePath}: ${e instanceof Error ? e.message : String(e)}`); + });
31-31: Add strong typing for session registryactiveDeviceLogSessions is untyped, which weakens guarantees for downstream stop/cleanup logic. Add a typed Map including the process and stream handles.
+import type { ChildProcess } from 'node:child_process'; @@ -export const activeDeviceLogSessions = new Map(); +export const activeDeviceLogSessions: Map< + string, + { + process: ChildProcess | undefined; + logFilePath: string; + deviceUuid: string; + bundleId: string; + logStream: fs.WriteStream; + } +> = new Map();
91-97: Persist the log stream in the session for proper lifecycle managementStore the stream so your stop operation can flush/close and you can later handle cleanup safely.
- activeDeviceLogSessions.set(logSessionId, { - process: result.process, - logFilePath, - deviceUuid, - bundleId, - }); + activeDeviceLogSessions.set(logSessionId, { + process: result.process, + logFilePath, + deviceUuid, + bundleId, + logStream, + });
70-88: Adjust Device Log Capture to use detached mode
TheCommandExecutorsupports adetachedflag (its 5th argument). By default (detached = false) it waits for the process to exit—which, withxcrun … --console, never happens and blocks forever.To fix, pass
truefordetachedand stream the child process yourself:// Spawn the console session in background and get the process handle const { process: childProc } = await executor( [ 'xcrun', 'devicectl', 'device', 'process', 'launch', '--console', '--terminate-existing', '--device', deviceUuid, bundleId, ], 'Device Log Capture', true, // useShell undefined, // env true, // detached! ); // Pipe logs to your file/stream childProc.stdout?.pipe(logFileStream); childProc.stderr?.pipe(logFileStream); // Store `childProc` in your session map so you can call childProc.kill() later sessionMap.set(sessionId, childProc);This way the promise resolves immediately with the
ChildProcesshandle, you capture its output as it arrives, and you avoid hanging on a never-ending console session.src/mcp/tools/macos/build_run_macos.ts (1)
95-103: Arch parameter is ignored when resolving app path from build settingsYou accept an arch parameter in the schema but don’t pass it to xcodebuild for the -showBuildSettings call, unlike get_mac_app_path which supports macOS arch. This can produce mismatched or ambiguous build settings on dual-arch setups.
Apply this patch to include -destination for macOS arch when present:
// Add derived data path if provided if (params.derivedDataPath) { command.push('-derivedDataPath', params.derivedDataPath); } + // Handle destination for macOS when arch is specified (parity with get_mac_app_path) + if (params.arch) { + const destinationString = `platform=macOS,arch=${params.arch}`; + command.push('-destination', destinationString); + } + // Add extra args if provided if (params.extraArgs && params.extraArgs.length > 0) { command.push(...params.extraArgs); }src/mcp/tools/project-scaffolding/scaffold_ios_project.ts (4)
437-444: Avoid optional commandExecutor by using a default parameter.You’re already importing getDefaultCommandExecutor statically. Use it as a default parameter and remove the dynamic import branch below. This prevents reintroducing the utils/index.js barrel (which the PR is trying to eliminate) and simplifies the control flow.
Apply this diff:
-async function scaffoldProject( - params: Record<string, unknown>, - commandExecutor?: CommandExecutor, - fileSystemExecutor: FileSystemExecutor = getDefaultFileSystemExecutor(), -): Promise<string> { +async function scaffoldProject( + params: Record<string, unknown>, + commandExecutor: CommandExecutor = getDefaultCommandExecutor(), + fileSystemExecutor: FileSystemExecutor = getDefaultFileSystemExecutor(), +): Promise<string> {
459-463: Remove dynamic import from utils/index.js to prevent barrel reintroduction.This dynamic import directly references the old barrel and undermines the PR’s separation. With the default parameter in place, this block is no longer needed.
Apply this diff:
- // Import the default command executor if not provided - if (!commandExecutor) { - const { getDefaultCommandExecutor } = await import('../../../utils/index.js'); - commandExecutor = getDefaultCommandExecutor(); - }
72-85: Fix orientation mapping to match schema values (case/hyphen mismatch).Current orientationToIOSConstant expects "Portrait"/"LandscapeLeft" etc., but schema provides 'portrait', 'landscape-left', 'landscape-right', 'portrait-upside-down'. This produces incorrect XCConfig values.
Apply this diff:
-function orientationToIOSConstant(orientation: string): string { - switch (orientation) { - case 'Portrait': - return 'UIInterfaceOrientationPortrait'; - case 'PortraitUpsideDown': - return 'UIInterfaceOrientationPortraitUpsideDown'; - case 'LandscapeLeft': - return 'UIInterfaceOrientationLandscapeLeft'; - case 'LandscapeRight': - return 'UIInterfaceOrientationLandscapeRight'; - default: - return orientation; - } -} +function orientationToIOSConstant(orientation: string): string { + // Normalize to lower-case for matching + const key = orientation.toLowerCase(); + switch (key) { + case 'portrait': + return 'UIInterfaceOrientationPortrait'; + case 'portrait-upside-down': + return 'UIInterfaceOrientationPortraitUpsideDown'; + case 'landscape-left': + return 'UIInterfaceOrientationLandscapeLeft'; + case 'landscape-right': + return 'UIInterfaceOrientationLandscapeRight'; + default: + // Fallback: return original in case templates expect raw string + return orientation; + } +}
90-101: Correct targetedDeviceFamily typing and mapping.Schema defines targetedDeviceFamily as an array: ['iphone', 'ipad', 'universal'] yet code treats it as a string and maps to 'iPhone'/'iPad'/'iPhone+iPad'. This will silently generate wrong XCConfig values.
Apply these diffs to (1) map array values to numeric family, and (2) fix type casts and usage:
-function deviceFamilyToNumeric(family: string): string { - switch (family) { - case 'iPhone': - return '1'; - case 'iPad': - return '2'; - case 'iPhone+iPad': - return '1,2'; - default: - return '1,2'; - } -} +function deviceFamilyToNumeric( + families: ('iphone' | 'ipad' | 'universal')[], +): string { + // If 'universal' is present, it's both iPhone and iPad + if (families.includes('universal')) return '1,2'; + const codes = new Set<string>(); + if (families.includes('iphone')) codes.add('1'); + if (families.includes('ipad')) codes.add('2'); + // Default to universal if nothing recognized + return codes.size ? Array.from(codes).sort().join(',') : '1,2'; +}- const targetedDeviceFamily = params.targetedDeviceFamily as string | undefined; + const targetedDeviceFamily = params.targetedDeviceFamily as + | ('iphone' | 'ipad' | 'universal')[] + | undefined;- if (targetedDeviceFamily) { - const deviceFamilyValue = deviceFamilyToNumeric(targetedDeviceFamily); + if (targetedDeviceFamily && targetedDeviceFamily.length > 0) { + const deviceFamilyValue = deviceFamilyToNumeric(targetedDeviceFamily); result = result.replace( /TARGETED_DEVICE_FAMILY = .+/g, `TARGETED_DEVICE_FAMILY = ${deviceFamilyValue}`, ); }Also applies to: 147-187
src/mcp/tools/ui-testing/screenshot.ts (1)
86-96: Use log level 'warn' instead of 'warning'.Other tools use 'warn'. If log’s level type is a union, 'warning' will either be downgraded or fail type-checking/logging filters.
Apply this diff:
- log('warning', `${LOG_PREFIX}/screenshot: Image optimization failed, using original PNG`); + log('warn', `${LOG_PREFIX}/screenshot: Image optimization failed, using original PNG`); @@ - log('warning', `${LOG_PREFIX}/screenshot: Failed to delete temp file: ${err}`); + log('warn', `${LOG_PREFIX}/screenshot: Failed to delete temp file: ${err}`); @@ - log('warning', `${LOG_PREFIX}/screenshot: Failed to delete temporary files: ${err}`); + log('warn', `${LOG_PREFIX}/screenshot: Failed to delete temporary files: ${err}`);Also applies to: 111-116
src/mcp/tools/simulator/launch_app_sim.ts (2)
179-188: Missing isError on failure response (breaks client error detection).The error path returns content without isError, unlike other branches. This can cause the caller to treat failures as success.
if (!result.success) { return { content: [ { type: 'text', text: `Launch app in simulator operation failed: ${result.error}`, }, ], + isError: true, }; }
211-218: Missing isError on catch-all error response.Same issue here—errors should be explicit for consistent handling downstream.
return { content: [ { type: 'text', text: `Launch app in simulator operation failed: ${errorMessage}`, }, ], + isError: true, };src/mcp/tools/simulator/build_run_sim.ts (1)
203-206: Guard against missing output before regex parsing.result.success can be true while output is empty. Add a check to avoid .match on undefined.
- const buildSettingsOutput = result.output; + const buildSettingsOutput = result.output; + if (!buildSettingsOutput) { + return createTextResponse( + 'Build succeeded, but no build settings output was returned. Ensure the scheme is valid and produces build settings.', + true, + ); + }src/mcp/tools/ui-testing/swipe.ts (2)
84-89: Duration message is dropped for duration=0.The truthy check omits “0s” (valid per schema). Use an undefined check.
- const optionsText = duration ? ` duration=${duration}s` : ''; + const optionsText = duration !== undefined ? ` duration=${duration}s` : '';
199-203: Standardize log level naming: use ‘warning’ instead of ‘warn’The logger function accepts any string, but its documentation and existing usages rely on the level
warning. To avoid confusion and ensure consistency across the codebase, please replace all instances of:log('warn', …)with
log('warning', …)– In this PR, update src/mcp/tools/ui-testing/swipe.ts lines 199–203.
– Consider running a codemod or search/replace for otherlog('warn'…)calls to align with the documented level.src/mcp/tools/macos/stop_mac_app.ts (1)
49-54: Command injection risk via string interpolation in sh -c.Interpolating appName into a shell command allows injection. Avoid sh -c and pass arguments positionally.
Apply this refactor to eliminate sh -c and maintain the fallback:
- // Stop by app name - use shell command with fallback for complex logic - command = [ - 'sh', - '-c', - `pkill -f "${params.appName}" || osascript -e 'tell application "${params.appName}" to quit'`, - ]; + // Stop by app name without shell interpolation; try exact match, then AppleScript quit + try { + await executor(['pkill', '-x', String(params.appName)], 'Stop macOS App (pkill)'); + } catch { + // Fallback to AppleScript quit if pkill didn't match/failed + await executor( + ['osascript', '-e', `tell application "${params.appName}" to quit`], + 'Stop macOS App (osascript)', + ); + } + return { + content: [ + { + type: 'text', + text: `✅ macOS app stopped successfully: ${params.appName}`, + }, + ], + };Note:
- pkill -x matches the exact process name, reducing accidental matches.
- We return early after executing the stop flow; if you prefer keeping the existing structure, we can restructure the try block accordingly.
I can adjust this patch to preserve the original control flow if preferred.
src/mcp/tools/ui-testing/tap.ts (1)
36-51: Extract and Share describe_ui Session StoreThe coordinate‐warning maps in each UI tool (tap.ts, long_press.ts, swipe.ts, touch.ts) are never updated—only the internal map in
describe_ui.tsis mutated viarecordDescribeUICall. This meansgetCoordinateWarningwill always warn. We need to centralize the session store and warning logic:
In
src/mcp/tools/ui-testing/describe_ui.ts
• Export the shareddescribeUITimestampsmap (or provide agetDescribeSessionhelper).
• ExportgetCoordinateWarning(andDESCRIBE_UI_WARNING_TIMEOUT) instead of duplicating them.In each UI tool file (
tap.ts,long_press.ts,swipe.ts,touch.ts)
• Remove the localdescribeUITimestamps,DESCRIBE_UI_WARNING_TIMEOUT, andgetCoordinateWarning.
• Import the sharedgetCoordinateWarning(and any needed constants) from./describe_ui.Example diff for tap.ts:
- // Session tracking for describe_ui warnings (shared across UI tools) - const describeUITimestamps = new Map<string, { timestamp: number }>(); - const DESCRIBE_UI_WARNING_TIMEOUT = 60000; // 60 seconds - - function getCoordinateWarning(simulatorUuid: string): string | null { - const session = describeUITimestamps.get(simulatorUuid); - … - } + import { getCoordinateWarning } from './describe_ui';src/mcp/tools/doctor/lib/doctor.deps.ts (1)
170-171: Standardize Sentry env var name to align with PR (XCODEBUILDMCP_SENTRY_DISABLED).The PR standardizes the opt‑out env var to XCODEBUILDMCP_SENTRY_DISABLED. Reflect that here so doctor reports the correct flag. Consider keeping the legacy SENTRY_DISABLED only if you intentionally want to surface it.
- 'SENTRY_DISABLED', + 'XCODEBUILDMCP_SENTRY_DISABLED',docs/ARCHITECTURE.md (1)
32-36: Sentry initialization text conflicts with lazy/dynamic production-only loading in PR summaryThe “Initialization” step currently says “Sentry initialized for error tracking (optional)”, but the PR states Sentry is lazily loaded via dynamic import and only in production, with opt-out via XCODEBUILDMCP_SENTRY_DISABLED. Please update this text for consistency.
Suggested wording (outside diff range):
- Sentry initialized for error tracking (optional) + Sentry is lazily loaded via dynamic import in production environments (respects XCODEBUILDMCP_SENTRY_DISABLED for opt-out)
♻️ Duplicate comments (2)
src/utils/logger.ts (1)
60-62: Avoid top‑level logging to keep utils side‑effect‑free.Logging on import violates the “pure utils” constraint and can produce unexpected output in consumers.
-if (!SENTRY_ENABLED) { - log('info', 'Sentry disabled due to SENTRY_DISABLED environment variable'); -}src/mcp/tools/ui-testing/type_text.ts (1)
15-20: Past suggestion addressed: helper imports restored.The helper imports for getAxePath/getBundledAxeEnvironment are back; the earlier issue is resolved.
| 5. **Tool Registration** | ||
| - Discovered tools automatically registered with server using pre-generated maps | ||
| - No manual registration or configuration required | ||
| - Environment variables can still control dynamic tool discovery | ||
| - Environment variables control dynamic tool discovery behavior | ||
|
|
There was a problem hiding this comment.
Numbering clash with subsequent sections (“Request Handling” also labeled 5)
“5. Tool Registration” is followed by “5. Request Handling” (Line 58) and “6. Response Streaming” (Line 64). Update numbering to avoid confusion.
Change the following lines outside this diff range:
6. **Request Handling**
7. **Response Streaming**🧰 Tools
🪛 LanguageTool
[grammar] ~53-~53: There might be a mistake here.
Context: ... tools on demand 5. Tool Registration - Discovered tools automatically registere...
(QB_NEW_EN)
🤖 Prompt for AI Agents
docs/ARCHITECTURE.md around lines 53 to 57: the numbered section "5. Tool
Registration" conflicts with the next sections which are also numbered starting
at 5; update the subsequent section headers to correct sequence by renumbering
"5. Request Handling" to "6. Request Handling" and "6. Response Streaming" to
"7. Response Streaming" (and adjust any following top-level section numbers as
needed to keep the numbering consistent).
src/core/resources.ts
Outdated
| import { log } from '../utils/logging/index.js'; | ||
| import type { CommandExecutor } from '../utils/execution/index.js'; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Sanity check: ensure no residual imports from utils/index.js remain.
Given the refactor away from barrel imports, it’s worth verifying no stragglers remain.
🏁 Script executed:
#!/bin/bash
# Find any remaining imports from utils/index.js or utils/command.js
rg -nP --type ts -C2 $'from\\s+[\'"][^\'"]*utils/index\\.js[\'"]|from\\s+[\'"][^\'"]*utils/command\\.js[\'"]'Length of output: 4854
Remove residual barrel imports from utils modules
I ran the verification script and found multiple straggler imports of utils/command.js and utils/*/index.js across the codebase. These need to be updated to direct, specific utility file paths. For example:
- src/core/dynamic-tools.ts
– line 2:import { getDefaultCommandExecutor, CommandExecutor } from '../utils/command.js' - src/mcp/tools/project-discovery/discover_projs.ts
– line 10:import { log } from '../../../utils/logging/index.js'
– line 12:import { getDefaultFileSystemExecutor, getDefaultCommandExecutor } from '../../../utils/command.js' - src/mcp/tools/project-discovery/get_app_bundle_id.ts (lines 13–15)
- src/mcp/tools/logging/stop_sim_log_cap.ts (line 11)
- src/mcp/tools/logging/stop_device_log_cap.ts (line 13)
- src/mcp/tools/project-scaffolding/scaffold_macos_project.ts (line 17)
- src/mcp/tools/swift-package/swift_package_list.ts (line 9)
Please replace all of these barrel imports with imports from the new, specific module files (e.g. ../utils/logger.js, ../utils/execution/index.js, etc.).
🤖 Prompt for AI Agents
In src/core/resources.ts around lines 16-17, replace the barrel-style imports
from '../utils/logging/index.js' and '../utils/execution/index.js' with direct
imports to the concrete utility implementation files (e.g. import the logging
functions from the logger file such as '../utils/logger.js' and the
CommandExecutor type/implementation from its specific file under
utils/execution, not the index barrel). Update the import paths to the exact
module files that export the symbols used in this file and remove any
'/index.js' barrel references.
src/mcp/tools/device/list_devices.ts
Outdated
| import type { ToolResponse } from '../../../types/common.js'; | ||
| import { log } from '../../../utils/logging/index.js'; | ||
| import type { CommandExecutor } from '../../../utils/execution/index.js'; | ||
| import { getDefaultCommandExecutor } from '../../../utils/execution/index.js'; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Type-only imports and new execution/logging facades: LGTM
Good move to type-only imports for ToolResponse and CommandExecutor, and to consume the new logging/execution facades directly. This reduces side effects, improves treeshaking, and aligns with the PR’s modularization goals.
Run this quick check to verify the expected exports are present and there are no lingering imports from deprecated barrels:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify execution facade exports
rg -nP -C2 --type=ts '\bexport\s+(?:type|interface)\s+CommandExecutor\b|export\s*\{\s*type\s+CommandExecutor\s*\}' src/utils/execution
rg -nP -C2 --type=ts '\bexport\s+(?:const|function)\s+getDefaultCommandExecutor\b|export\s*\{\s*getDefaultCommandExecutor\s*\}' src/utils/execution
# Verify logging facade export
rg -nP -C2 --type=ts '\bexport\s+(?:function|const)\s+log\b|export\s*\{\s*log\s*\}' src/utils/logging
# Verify ToolResponse type export
rg -nP -C2 --type=ts '\bexport\s+type\s+ToolResponse\b|export\s*\{\s*type\s+ToolResponse\s*\}' src/types
# Ensure no references to deprecated barrels remain
rg -nP -C2 --type=ts "from\s+['\"][^'\"]*utils/index(\.js)?['\"]" -g '!**/node_modules/**'Length of output: 156
I didn’t see any exports or usages in the initial scan—let’s get a clearer view of the actual files and definitions:
This will list the files and show whether the expected exports actually exist and that no old barrel imports remain.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Execution utils directory =="
fd --extension ts --strip-cwd-prefix src/utils/execution
echo
echo "== Search for CommandExecutor definitions =="
rg -nC2 --type=ts '\bexport\s+(?:type|interface)\s+CommandExecutor\b|CommandExecutor' src/utils/execution
echo
echo "== Search for getDefaultCommandExecutor definitions =="
rg -nC2 --type=ts '\bexport\s+(?:const|function)\s+getDefaultCommandExecutor\b|getDefaultCommandExecutor' src/utils/execution
echo
echo "== Logging utils directory =="
fd --extension ts --strip-cwd-prefix src/utils/logging
echo
echo "== Search for log export =="
rg -nC2 --type=ts '\bexport\s+(?:function|const)\s+log\b|log' src/utils/logging
echo
echo "== Search for ToolResponse type =="
rg -nC2 --type=ts '\bexport\s+type\s+ToolResponse\b|ToolResponse' src/types
echo
echo "== Check for deprecated barrel imports =="
rg -nP -C2 --type=ts "from\s+['\"][^'\"]*utils/index(\.js)?['\"]" -g '!**/node_modules/**'Length of output: 533
Let’s correct the fd usage and explicitly list and search each directory:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== TS files under src/utils/execution =="
fd . src/utils/execution --extension ts --strip-cwd-prefix
echo
echo "== Search for CommandExecutor exports =="
rg -nP '\bexport\s+(?:type|interface)\s+CommandExecutor\b' -nC2 src/utils/execution
echo
echo "== Search for getDefaultCommandExecutor exports =="
rg -nP '\bexport\s+(?:const|function)\s+getDefaultCommandExecutor\b' -nC2 src/utils/execution
echo
echo "== TS files under src/utils/logging =="
fd . src/utils/logging --extension ts --strip-cwd-prefix
echo
echo "== Search for log exports =="
rg -nP '\bexport\s+(?:function|const)\s+log\b' -nC2 src/utils/logging
echo
echo "== TS files under src/types =="
fd . src/types --extension ts --strip-cwd-prefix
echo
echo "== Search for ToolResponse type export =="
rg -nP '\bexport\s+type\s+ToolResponse\b' -nC2 src/types
echo
echo "== Check for deprecated barrel imports =="
rg -nP "from\s+['\"][^'\"]*utils/index(\.js)?['\"]" -nC2 src -g '!**/node_modules/**'Length of output: 328
Let’s list all TS utils files and directly grep for the exports:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== TS files under src/utils ==="
find src/utils -type f -name '*.ts' | sort
echo
echo "=== CommandExecutor export ==="
rg -nP 'export\s+(?:type|interface)\s+CommandExecutor\b' -nC2 src/utils
echo
echo "=== getDefaultCommandExecutor export ==="
rg -nP 'export\s+(?:const|function)\s+getDefaultCommandExecutor\b' -nC2 src/utils
echo
echo "=== log export ==="
rg -nP 'export\s+(?:const|function)\s+log\b' -nC2 src/utils
echo
echo "=== ToolResponse type export ==="
rg -nP 'export\s+type\s+ToolResponse\b' -nC2 src/types
echo
echo "=== Deprecated barrel imports ==="
rg -nP "from\s+['\"][^'\"]*utils/index(\.js)?['\"]" -nC2 src -g '!**/node_modules/**'Length of output: 1642
Let’s explicitly verify the facade files and the ToolResponse type in their source files, and check once more for any leftover barrel imports:
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "=== Execution facade exports in src/utils/execution/index.ts ==="
rg -P -C1 'export\s+(?:type|interface)\s+CommandExecutor' src/utils/execution/index.ts || echo "⛔ CommandExecutor export not found"
rg -P -C1 'export\s+(?:const|function)\s+getDefaultCommandExecutor' src/utils/execution/index.ts || echo "⛔ getDefaultCommandExecutor export not found"
echo
echo "=== Logging facade export in src/utils/logging/index.ts ==="
rg -P -C1 'export\s+(?:function|const)\s+log' src/utils/logging/index.ts || echo "⛔ log export not found"
echo
echo "=== ToolResponse type in src/types/common.ts ==="
rg -P -C1 'export\s+type\s+ToolResponse' src/types/common.ts || echo "⛔ ToolResponse export not found"
echo
echo "=== Deprecated barrel imports anywhere under src/ ==="
rg -P "from\s+['\"][^'\"]*utils/index(\.js)?['\"]" src || echo "✅ No deprecated barrel imports found"Length of output: 1284
Missing exports in facades and incorrect type import
The new list_devices.ts is importing symbols that aren’t actually exported from their modules:
- src/utils/execution/index.ts
• Noexport type CommandExecutor
• Noexport const|getDefaultCommandExecutor - src/utils/logging/index.ts
• Noexport function|const log - src/types/common.ts
• Noexport type ToolResponse(onlyToolResponseContentis defined)
Please either add the missing re-exports to these facade index files or correct the imports in list_devices.ts to match the actual exported names.
🤖 Prompt for AI Agents
In src/mcp/tools/device/list_devices.ts (lines 9–12): the file imports names
that the facade index files do not actually export. Fix by either adding
re-exports to the facades or updating the imports to the real exported names:
update src/utils/execution/index.ts to export the CommandExecutor type and
getDefaultCommandExecutor (or change the import in list_devices.ts to the
executor type/name that file currently exposes), update
src/utils/logging/index.ts to export the log function/const used here (or import
the actual logger name the module exports), and update src/types/common.ts to
either export ToolResponse or change the import to use ToolResponseContent (or
the actual type name). Ensure import paths and types exactly match the symbols
exported from those index files so the module resolves cleanly.
| import { log } from '../../../utils/logging/index.js'; | ||
| import { createTextResponse } from '../../../utils/responses/index.js'; | ||
| import { executeXcodeBuildCommand } from '../../../utils/build/index.js'; | ||
| import { ToolResponse, XcodePlatform } from '../../../types/common.js'; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Broken import: utils/build/index.js cannot be resolved (CI failure)
CI error shows: “Could not resolve "../../../utils/build/index.js"”. The new build facade either hasn’t been added or lives at a different path. This breaks this module at import time.
Run the following script to locate the actual definition/export of executeXcodeBuildCommand and confirm the correct import path. It also shows whether a utils/build facade exists.
Once you identify the correct module, update the import here accordingly. If the intent is to provide a facade, ensure src/utils/build/index.ts exists and re-exports executeXcodeBuildCommand from its actual location to stabilize import paths across tools.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for executeXcodeBuildCommand definition..."
rg -nP -C3 '\bexport\s+(?:async\s+)?function\s+executeXcodeBuildCommand\b|\bexport\s+const\s+executeXcodeBuildCommand\b|\bmodule\.exports\s*=\s*{[^}]*executeXcodeBuildCommand' || true
echo
echo "Searching for re-exports of executeXcodeBuildCommand..."
rg -nP -C2 'export\s*{\s*executeXcodeBuildCommand\s*}' || true
echo
echo "Inspecting potential facades under utils/build..."
fd -t f -a 'index.(ts|js)' | rg -n 'utils/build' || true
echo
echo "Checking all import sites referencing utils/build/index.js (to update consistently if needed)..."
rg -nP -C2 "from ['\"][.]{1,2}/.*/utils/build/index\.js['\"]" --type=ts --type=tsx || trueLength of output: 1504
Fix import path for executeXcodeBuildCommand
The import ../../../utils/build/index.js doesn’t exist—executeXcodeBuildCommand is exported from src/utils/build-utils.ts. Update the import to point at the correct file:
• File: src/mcp/tools/macos/build_run_macos.ts (around lines 9–12)
-import { executeXcodeBuildCommand } from '../../../utils/build/index.js';
+import { executeXcodeBuildCommand } from '../../../utils/build-utils.js';📝 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.
| import { log } from '../../../utils/logging/index.js'; | |
| import { createTextResponse } from '../../../utils/responses/index.js'; | |
| import { executeXcodeBuildCommand } from '../../../utils/build/index.js'; | |
| import { ToolResponse, XcodePlatform } from '../../../types/common.js'; | |
| import { log } from '../../../utils/logging/index.js'; | |
| import { createTextResponse } from '../../../utils/responses/index.js'; | |
| import { executeXcodeBuildCommand } from '../../../utils/build-utils.js'; | |
| import { ToolResponse, XcodePlatform } from '../../../types/common.js'; |
🧰 Tools
🪛 GitHub Actions: CI
[error] 11-11: Could not resolve "../../../utils/build/index.js"
🤖 Prompt for AI Agents
In src/mcp/tools/macos/build_run_macos.ts around lines 9 to 12, the import path
for executeXcodeBuildCommand is incorrect (../../../utils/build/index.js) —
update it to import the named export from the correct file
src/utils/build-utils.ts using the relative path ../../../utils/build-utils.ts
(or ../../../utils/build-utils if your build expects no extension), and keep the
named import executeXcodeBuildCommand; adjust any export/import names if
necessary to match that file.
src/mcp/tools/ui-testing/button.ts
Outdated
| import { z } from 'zod'; | ||
| import { ToolResponse } from '../../../types/common.js'; | ||
| import { log, createTextResponse } from '../../../utils/index.js'; | ||
| import { | ||
| DependencyError, | ||
| AxeError, | ||
| SystemError, | ||
| createErrorResponse, | ||
| } from '../../../utils/index.js'; | ||
| import { CommandExecutor, getDefaultCommandExecutor } from '../../../utils/index.js'; | ||
| import type { ToolResponse } from '../../../types/common.js'; | ||
| import { log } from '../../../utils/logging/index.js'; | ||
| import { createTextResponse, createErrorResponse } from '../../../utils/responses/index.js'; | ||
| import type { CommandExecutor } from '../../../utils/execution/index.js'; | ||
| import { getDefaultCommandExecutor } from '../../../utils/execution/index.js'; | ||
| import { | ||
| createAxeNotAvailableResponse, | ||
| getAxePath, | ||
| getBundledAxeEnvironment, | ||
| } from '../../../utils/index.js'; | ||
| } from '../../../utils/axe-helpers.js'; | ||
| import { DependencyError, AxeError, SystemError } from '../../../utils/errors.js'; | ||
| import { createTypedTool } from '../../../utils/typed-tool-factory.js'; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Replace inlined executeAxeCommand with the new centralized utils/axe-command module
The PR objective mentions centralizing AXe command execution (utils/axe-command.ts). This file still inlines it and redefines AxeHelpers, which reintroduces duplication and drift risk.
Minimal refactor:
import { z } from 'zod';
import type { ToolResponse } from '../../../types/common.js';
import { log } from '../../../utils/logging/index.js';
import { createTextResponse, createErrorResponse } from '../../../utils/responses/index.js';
import type { CommandExecutor } from '../../../utils/execution/index.js';
import { getDefaultCommandExecutor } from '../../../utils/execution/index.js';
+import { executeAxeCommand } from '../../../utils/axe-command.js';
+export type { AxeHelpers } from '../../../utils/axe-command.js';
@@
-export interface AxeHelpers {
- getAxePath: () => string | null;
- getBundledAxeEnvironment: () => Record<string, string>;
- createAxeNotAvailableResponse: () => ToolResponse;
-}
@@
-// Helper function for executing axe commands (inlined from src/tools/axe/index.ts)
-async function executeAxeCommand(
- commandArgs: string[],
- simulatorUuid: string,
- commandName: string,
- executor: CommandExecutor = getDefaultCommandExecutor(),
- axeHelpers: AxeHelpers = { getAxePath, getBundledAxeEnvironment, createAxeNotAvailableResponse },
-): Promise<void> {
- // Get the appropriate axe binary path
- const axeBinary = axeHelpers.getAxePath();
- if (!axeBinary) {
- throw new DependencyError('AXe binary not found');
- }
-
- // Add --udid parameter to all commands
- const fullArgs = [...commandArgs, '--udid', simulatorUuid];
-
- // Construct the full command array with the axe binary as the first element
- const fullCommand = [axeBinary, ...fullArgs];
-
- try {
- // Determine environment variables for bundled AXe
- const axeEnv = axeBinary !== 'axe' ? axeHelpers.getBundledAxeEnvironment() : undefined;
-
- const result = await executor(fullCommand, `${LOG_PREFIX}: ${commandName}`, false, axeEnv);
-
- if (!result.success) {
- throw new AxeError(
- `axe command '${commandName}' failed.`,
- commandName,
- result.error ?? result.output,
- simulatorUuid,
- );
- }
-
- // Check for stderr output in successful commands
- if (result.error) {
- log(
- 'warn',
- `${LOG_PREFIX}: Command '${commandName}' produced stderr output but exited successfully. Output: ${result.error}`,
- );
- }
-
- // Function now returns void - the calling code creates its own response
- } catch (error) {
- if (error instanceof Error) {
- if (error instanceof AxeError) {
- throw error;
- }
-
- // Otherwise wrap it in a SystemError
- throw new SystemError(`Failed to execute axe command: ${error.message}`, error);
- }
-
- // For any other type of error
- throw new SystemError(`Failed to execute axe command: ${String(error)}`);
- }
-}If utils/axe-command.ts exposes a different signature (e.g., needs LOG_PREFIX), I can adapt the call sites accordingly.
To verify remaining duplication across tools:
Also applies to: 25-29, 94-151
🏁 Script executed:
#!/bin/bash
# Find inlined executeAxeCommand occurrences outside utils/axe-command
rg -nP --type=ts -g '!**/__tests__/**' '(?s)async\s+function\s+executeAxeCommand\s*\(' -C2Length of output: 3832
Centralize AXe command execution in all UI-testing tools
The inlined executeAxeCommand is still duplicated across multiple src/mcp/tools/ui-testing/*.ts files. To prevent drift and ensure a single source of truth, import and use the shared executeAxeCommand (and its types) from utils/axe-command.ts in every UI-testing module:
• src/mcp/tools/ui-testing/button.ts
• src/mcp/tools/ui-testing/key_press.ts
• src/mcp/tools/ui-testing/type_text.ts
• src/mcp/tools/ui-testing/long_press.ts
• src/mcp/tools/ui-testing/swipe.ts
• src/mcp/tools/ui-testing/gesture.ts
• src/mcp/tools/ui-testing/describe_ui.ts
• src/mcp/tools/ui-testing/touch.ts
• src/mcp/tools/ui-testing/tap.ts
• src/mcp/tools/ui-testing/key_sequence.ts
Example minimal refactor in button.ts:
import { getDefaultCommandExecutor } from '../../../utils/execution/index.js';
-import {
- createAxeNotAvailableResponse,
- getAxePath,
- getBundledAxeEnvironment,
-} from '../../../utils/axe-helpers.js';
+import { executeAxeCommand, AxeHelpers } from '../../../utils/axe-command.js';
-export interface AxeHelpers { /* … */ }
-// Helper function for executing axe commands (inlined)
-async function executeAxeCommand(…){ /* … */ }
+// Use the shared executeAxeCommand and AxeHelpers from utils/axe-commandPlease apply this same pattern to all listed UI-testing modules.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/mcp/tools/ui-testing/button.ts lines 1-13, the file still uses an
inlined/duplicate executeAxeCommand implementation; import the shared
executeAxeCommand and its related types from utils/axe-command.ts instead,
remove the local duplicate implementation, update the imports to reference
executeAxeCommand and any needed types, and ensure all call sites in this file
use the imported function signature; apply the same import-and-replace change to
the other listed UI-testing modules so they all use the single shared
executeAxeCommand implementation.
src/mcp/tools/ui-testing/touch.ts
Outdated
| import { | ||
| createAxeNotAvailableResponse, | ||
| getAxePath, | ||
| getBundledAxeEnvironment, | ||
| } from '../../../utils/index.js'; | ||
| } from '../../../utils/axe-helpers.js'; | ||
| import { ToolResponse } from '../../../types/common.js'; |
There was a problem hiding this comment.
Likely stale import path for AXe helpers.
Other files (long_press/key_press) import these from ../../../utils/axe/index.js. Keeping a unique path here risks module duplication or resolution errors.
Apply this diff to align with the new facade:
-import {
- createAxeNotAvailableResponse,
- getAxePath,
- getBundledAxeEnvironment,
-} from '../../../utils/axe-helpers.js';
+import {
+ createAxeNotAvailableResponse,
+ getAxePath,
+ getBundledAxeEnvironment,
+} from '../../../utils/axe/index.js';📝 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.
| import { | |
| createAxeNotAvailableResponse, | |
| getAxePath, | |
| getBundledAxeEnvironment, | |
| } from '../../../utils/index.js'; | |
| } from '../../../utils/axe-helpers.js'; | |
| import { ToolResponse } from '../../../types/common.js'; | |
| import { | |
| createAxeNotAvailableResponse, | |
| getAxePath, | |
| getBundledAxeEnvironment, | |
| } from '../../../utils/axe/index.js'; | |
| import { ToolResponse } from '../../../types/common.js'; |
🤖 Prompt for AI Agents
In src/mcp/tools/ui-testing/touch.ts around lines 14 to 19, the file imports
from '../../../utils/axe-helpers.js' which is likely stale; update the import to
use the new facade path '../../../utils/axe/index.js' (or '../../../utils/axe'
if your resolver prefers) while keeping the same named imports
(createAxeNotAvailableResponse, getAxePath, getBundledAxeEnvironment) so the
module resolution aligns with other files and avoids duplication or resolution
errors.
src/mcp/tools/utilities/clean.ts
Outdated
| import { createErrorResponse } from '../../../utils/index.js'; | ||
| import type { CommandExecutor } from '../../../utils/execution/index.js'; | ||
| import { getDefaultCommandExecutor } from '../../../utils/execution/index.js'; | ||
| import { executeXcodeBuildCommand } from '../../../utils/build/index.js'; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Build break: import path cannot be resolved (utils/build/index.js)
CI indicates: “Could not resolve "../../../utils/build/index.js"”. The import path for executeXcodeBuildCommand is currently broken.
Two resolution paths; pick one:
- Preferred (stable facade): Keep this import and add a small facade at src/utils/build/index.ts that re-exports executeXcodeBuildCommand from its actual implementation module. This preserves decoupling and avoids churn across all call sites.
- Alternative: Change this import to the actual location of executeXcodeBuildCommand (if already moved elsewhere).
If you go with the facade, add this file (adjust the relative target after running the script below to find the actual symbol):
// src/utils/build/index.ts
export { executeXcodeBuildCommand } from '../<actual-location>/index.js';Use this script to locate the symbol and verify whether a build facade already exists:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Looking for executeXcodeBuildCommand definition(s)..."
rg -n -C2 -g '!**/dist/**' --type=ts \
-e 'export\s+(?:async\s+)?function\s+executeXcodeBuildCommand\b' \
-e 'export\s+const\s+executeXcodeBuildCommand\b' \
-e '\bexecuteXcodeBuildCommand\s*:\s*\(' || true
echo
echo "Looking for existing build facade..."
fd -a -g 'src/utils/build/index.*' || trueLength of output: 767
Critical: Fix broken import for executeXcodeBuildCommand
CI is failing with “Could not resolve '../../../utils/build/index.js'”. A quick audit shows:
- The function is defined in
src/utils/build-utils.ts(line 44). - There is no existing
src/utils/build/index.*facade, so the import in
src/mcp/tools/utilities/clean.tsis invalid.
You have two options—pick one:
-
Preferred (stable façade)
Createsrc/utils/build/index.tsand re‐export from the real module:// src/utils/build/index.ts export { executeXcodeBuildCommand } from '../build-utils.js';
This will compile to
index.jsand satisfy existing imports. -
Alternative (direct import)
Change the import inclean.tsto point at the actual file:--- a/src/mcp/tools/utilities/clean.ts +++ b/src/mcp/tools/utilities/clean.ts @@ -12,1 +12,1 @@ -import { executeXcodeBuildCommand } from '../../../utils/build/index.js'; +import { executeXcodeBuildCommand } from '../../../utils/build-utils.js';
Either approach restores the broken import.
🧰 Tools
🪛 GitHub Actions: CI
[error] 12-12: Could not resolve "../../../utils/build/index.js"
🤖 Prompt for AI Agents
In src/mcp/tools/utilities/clean.ts around line 12 the import "import {
executeXcodeBuildCommand } from '../../../utils/build/index.js';" is broken
because the real function lives in src/utils/build-utils.ts; fix by either
(preferred) adding a stable façade file src/utils/build/index.ts that re-exports
the function from ../build-utils.js (so existing imports resolve to index.js
after compile) or (alternative) update the import in clean.ts to point directly
to the actual module path (e.g. '../../../utils/build-utils.js'); implement one
of these two changes and run TypeScript/CI to verify the import resolves.
src/utils/logger.ts
Outdated
| import { createRequire } from 'node:module'; | ||
| // Note: Removed "import * as Sentry from '@sentry/node'" to prevent native module loading at import time | ||
|
|
||
| const SENTRY_ENABLED = process.env.SENTRY_DISABLED !== 'true'; |
There was a problem hiding this comment.
Use the standardized env var and gate Sentry to production only.
The PR standardizes the opt‑out variable to XCODEBUILDMCP_SENTRY_DISABLED, and Sentry should only be active in production. Today this code checks SENTRY_DISABLED and doesn’t gate by NODE_ENV.
-const SENTRY_ENABLED = process.env.SENTRY_DISABLED !== 'true';
+const SENTRY_ENABLED =
+ process.env.NODE_ENV === 'production' &&
+ process.env.XCODEBUILDMCP_SENTRY_DISABLED !== 'true';🤖 Prompt for AI Agents
In src/utils/logger.ts around line 23, the code reads the wrong env var and
doesn't restrict Sentry to production; change the check to use the standardized
opt‑out variable XCODEBUILDMCP_SENTRY_DISABLED and only enable Sentry when
NODE_ENV === 'production' and XCODEBUILDMCP_SENTRY_DISABLED !== 'true'; update
the SENTRY_ENABLED assignment accordingly and ensure any related documentation
or env references in the repo are consistent with the new variable name.
## Summary - Convert all relative imports from .js to .ts extensions (564 imports across 169 files) - Update ESLint rules to enforce .ts import pattern and prevent regression - Fix remaining barrel imports to use focused facade pattern - Add comprehensive documentation for TypeScript import standards ## Background/Details The project used mixed import extensions (.js and .ts) which created inconsistency and prevented compatibility with native TypeScript runtimes like Bun and Deno. The .js imports were a legacy pattern from older TypeScript compilation setups. ## Solution 1. **Automated Conversion**: Created script to convert all relative .js imports to .ts 2. **ESLint Enforcement**: Updated rules to catch both .js imports and barrel imports 3. **External Package Handling**: Preserved .js extensions for external packages (required) 4. **Focused Facades**: Completed migration from barrel imports to focused facade pattern ## Testing - All 1046 tests pass - Build compiles successfully - ESLint passes with no violations - Verified compatibility with current build system ## Benefits - Future-proof for native TypeScript runtimes (Bun, Deno, Node.js --loader) - Better IDE experience with direct source file navigation - Consistent import pattern matching actual source files - Automated enforcement prevents regression
|
Claude finished @cameroncooke's task —— View job PR Review: Architectural RefactoringThis is an excellent architectural refactoring that successfully addresses the CI module linking issues while delivering substantial code quality improvements. The changes are well-structured, thoroughly implemented, and demonstrate strong engineering principles. Review Progress
✅ Strengths1. Architectural Excellence
2. CI Issue Resolution
3. Code Quality Improvements
4. Testing Architecture
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Change gitignore pattern from 'build/' to '/build/' to only ignore root build artifacts - Add src/utils/build/index.ts focused facade that was accidentally gitignored - This fixes CI build failures where files import from utils/build/index.ts
|
Claude finished @cameroncooke's task —— View job PR Review: Architectural RefactoringThis is an excellent architectural refactoring that successfully addresses the CI module linking issues while delivering substantial code quality improvements. The changes are well-structured, thoroughly implemented, and demonstrate strong engineering principles. Review Progress
✅ Strengths1. Architectural Excellence
2. CI Issue Resolution
3. Code Quality Improvements
4. Testing Architecture
|
## Summary - Finalize the focused facade architecture by fixing remaining inconsistent imports - Remove the deprecated utils/index.ts barrel file completely - Ensure all imports use proper focused facades throughout the codebase ## Changes Made ### Import Consistency Fixes - Fix build_device.ts to use utils/build/index.ts instead of direct build-utils.ts import - Fix test-common.ts to use utils/build/index.ts for executeXcodeBuildCommand import - Update test files to use utils/responses/index.ts for SystemError imports instead of barrel - Update scaffold project files to use proper focused facades for dynamic imports ### Architecture Completion - **REMOVED**: src/utils/index.ts - deprecated barrel file completely deleted - All imports now consistently use focused facades (build/, responses/, execution/, etc.) - No more barrel imports anywhere in the codebase ## Technical Benefits - **Improved tree-shaking**: Bundlers can better eliminate unused code - **Clearer dependencies**: Import paths explicitly show what functionality is needed - **Prevention of circular dependencies**: Focused facades reduce coupling risks - **Better performance**: Eliminates loading of unused modules during startup - **Architectural consistency**: Single pattern enforced across entire codebase ## Validation - ✅ TypeScript compilation: No errors - ✅ ESLint: Only 1 minor unrelated warning - ✅ Prettier: All files properly formatted - ✅ Tests: 1046 tests passing - ✅ Build: Successful compilation with all tools working The focused facade architecture is now fully implemented and enforced.
|
Claude finished @cameroncooke's task —— View job I'll analyze this and get back to you. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…lready linked" errors ## Root Cause Analysis CI was failing with 78 "module is already linked" errors due to: - Node.js 24.x + Vitest vmThreads pool triggering ESM linker bug - Tests still import Sentry via production dependency chain: test → tool → utils/build → utils/command → utils/logger → @sentry/node - vmThreads pool has known issues with native modules like Sentry ## Solution: Switch to threads pool - Changed pool from 'vmThreads' to 'threads' - Increased maxThreads from 1 to 4 for better performance - threads pool handles native modules better than vmThreads - Fixes the Node.js ESM linker issue while improving performance ## Evidence from dependency graph analysis Using madge revealed the exact import chain still bringing Sentry into tests: build_device.test.ts → build_device.ts → utils/build/index.ts → utils/build-utils.ts → utils/command.ts → utils/logger.ts → @sentry/node ## Benefits - Resolves CI "module is already linked" errors - Better performance (threads faster than vmThreads) - More robust handling of native modules - Maintains all existing test functionality (1046 tests pass) ## Testing - All tests pass locally with threads pool - TypeScript compilation and linting clean - No behavioral changes to test execution
|
Claude finished @cameroncooke's task —— View job PR Review: Architectural RefactoringThis is an excellent architectural refactoring that successfully addresses the CI module linking issues while delivering substantial code quality improvements. The changes are well-structured, thoroughly implemented, and demonstrate strong engineering principles. Review Checklist:
✅ Strengths1. Architectural Excellence
2. CI Issue Resolution
3. Code Quality Improvements
4. Testing Architecture
|
|
Claude finished @cameroncooke's task —— View job I'll analyze this and get back to you. |
1 participant
Summary
This PR introduces a major architectural refactoring to resolve persistent "module is already linked" errors in CI tests. It establishes clear boundaries between server infrastructure, test utilities, and pure utilities, preventing test code from importing and initializing production server components like Sentry.
Beyond fixing the CI failures, this change significantly improves code quality by eliminating over 580 lines of duplicated code, standardizing environment variables, and resolving numerous TypeScript and ESLint errors.
Background/Details
Since the Sentry v10 integration, our CI pipeline has been plagued by 74 "module is already linked" errors, occurring exclusively on the Ubuntu runner. The root cause was an architectural flaw where our central
src/utils/index.tsbarrel file exported both test utilities and production server code.This created a problematic import chain:
Tests →
src/utils/index.ts→src/server/code → Sentry InstrumentationAs a result, test suites inadvertently initialized the entire server stack, leading to module loading conflicts within Vitest's
vmThreadsenvironment on Linux.Solution
This PR implements a robust, multi-faceted solution:
Architectural Separation:
src/server/: A new dedicated directory for all server-side infrastructure, includingtool-registry.ts,dynamic-tools.ts,generated-plugins.ts, and the new server bootstrap logic.src/test-utils/: A new directory for test-only code. It contains pure mock executors and test execution logic (test-execution.ts) that have no dependencies on the production server.src/utils/: This directory has been purified to contain only side-effect-free, context-agnostic utilities. It no longer exports any server or test-specific code.Lazy Sentry Integration & Async Server Bootstrap:
createServerinsrc/server/bootstrap.ts).import()insrc/server/instrumentation.ts, ensuring it is only imported in production environments and never by tests.XCODEBUILDMCP_SENTRY_DISABLED.DRY Principle & Code Refactoring:
src/utils/axe-command.ts: A new shared utility was created to handle allaxecommand executions. This eliminated ~580 lines of duplicated logic across 10+ UI testing tools.src/utils/system-info.ts: A new shared module was created to centralize system/environment discovery logic, now used by both thedoctortool and Sentry instrumentation.Code Quality and Build Fixes:
eslint.config.jsto correctly ignore allgenerated-*.tsfiles, preventing linting errors in auto-generated code.Testing
doctortool, to align with the new dependency injection patterns and module structure.Notes
This is a foundational architectural improvement that not only fixes a critical CI issue but also makes the codebase more modular, maintainable, and resilient against future contamination between test and production code paths.
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores