Bug: The module-level cachedDevices in device-name-resolver.ts is never invalidated, which can lead to displaying stale device names in a long-running server process.
_{Severity: MEDIUM}

Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/utils/device-name-resolver.ts#L6

Potential issue: The `resolveDeviceName` function utilizes a module-level cache,
`cachedDevices`, which is populated once and persists for the entire process lifetime.
In a long-running server environment that handles multiple tool invocations, this cache
can become stale. If a device is disconnected, renamed, or reconnected between
invocations, the function will return the old, cached device name instead of the current
one. This results in incorrect device names being displayed in logs and user-facing
output, potentially causing confusion, although it does not affect the functional
targeting of devices by their UDID.

_{Did we get this right? 👍 / 👎 to inform future reviews.}

device-name-resolver blocks event loop with synchronous shell call

Medium Severity

device-name-resolver.ts directly imports execSync from node:child_process and readFileSync/unlinkSync from node:fs, bypassing CommandExecutor and FileSystemExecutor injection. The execSync call (with a 10-second timeout) blocks the Node.js event loop during normal test/build flows since it's called from both test-common.ts and xcodebuild-pipeline.ts. This violates the project's DI rules and makes the module untestable with mock executors.

 

^{Triggered by project rule: Bugbot Review Guide for XcodeBuildMCP}

^{Reviewed by Cursor Bugbot for commit 344aeba. Configure here.}

xcresult-test-failures uses direct execFileSync bypassing DI

Medium Severity

xcresult-test-failures.ts directly imports and uses execFileSync from node:child_process instead of accepting a CommandExecutor parameter. This violates the project's dependency injection rules requiring all shell commands to flow through CommandExecutor, and makes the module impossible to test with createMockExecutor.

 

^{Triggered by project rule: Bugbot Review Guide for XcodeBuildMCP}

^{Reviewed by Cursor Bugbot for commit 344aeba. Configure here.}

Bug: The code imports and calls getHandlerContext and handlerContextStorage, but these functions are not defined or exported in typed-tool-factory.ts, which will cause a runtime ReferenceError.
_{Severity: CRITICAL}

Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/utils/xcodebuild-pipeline.ts#L158-L166

Potential issue: The code imports `getHandlerContext` and `handlerContextStorage` from
`./typed-tool-factory.ts` in both `xcodebuild-pipeline.ts` and `test-common.ts`.
However, these functions are not defined or exported in the `typed-tool-factory.ts`
file. Consequently, any code path that calls these functions, such as `handleTestLogic`
or the fallback logic in `startBuildPipeline`, will fail at runtime with a
`ReferenceError`. This will crash the primary test execution path for any tool that
relies on this logic.

resolveTestProgressEnabled exported but never used in production code

Low Severity

resolveTestProgressEnabled is exported and tested but never called from any production code path. The progress parameter is accepted by handleTestLogic but resolveTestProgressEnabled is not invoked anywhere within the function or elsewhere outside its own test file. This is dead code that adds confusion about how progress resolution is intended to work.

 

^{Reviewed by Cursor Bugbot for commit f7616d2. Configure here.}

debugCapture.count returns zero after flush() empties lines

Low Severity

In xcodebuild-pipeline.ts, after calling debugCapture.flush(), the log message references debugCapture.count to report how many unrecognized lines were written. However, looking at createParserDebugCapture in xcodebuild-log-capture.ts, the flush() method does not clear the lines array, so count still works. But the count getter returns lines.length which is correct. Actually the real issue is that flush() writes the file and returns the path but doesn't reset state — calling flush() twice would write duplicate files. This is a minor concern given finalize is called once.

 

^{Reviewed by Cursor Bugbot for commit f7616d2. Configure here.}

Direct filesystem access bypasses FileSystemExecutor injection pattern

Medium Severity

Both xcodebuild-log-capture.ts and simulator-steps.ts import node:fs directly and use synchronous filesystem operations (mkdirSync, openSync, writeSync, closeSync, readFileSync, etc.) without accepting a FileSystemExecutor parameter. This violates the project rule requiring dependency injection for filesystem side effects and prevents testing with createMockFileSystemExecutor.

 

^{Triggered by project rule: Bugbot Review Guide for XcodeBuildMCP}

^{Reviewed by Cursor Bugbot for commit f7616d2. Configure here.}

Fallback error events emitted to wrong emit target

Medium Severity

In finalizeInlineXcodebuild, when fallback error events need to be emitted, they are sent via options.emit?.(event) (line 218). However, several call sites in test-common.ts pass emit: ctx.emit while others don't pass emit at all. When emit is not provided, fallback events are added to the returned events array but never actually emitted to the handler context, resulting in silently dropped error content when tests fail without structured diagnostics.

 

^{Reviewed by Cursor Bugbot for commit f7616d2. Configure here.}