fix(logging): detect device launch failures via json output#130
fix(logging): detect device launch failures via json output#130cameroncooke merged 2 commits intomainfrom
Conversation
WalkthroughDevice and logging tools are migrated from direct parameter validation to a session-aware architecture. Public schemas are reduced to expose only minimal required fields, while session-managed parameters (projectPath, workspacePath, scheme, configuration, deviceId) are validated internally via explicit requirements and exclusivity constraints. Tests are updated to verify strict schema exposure and new error messaging for missing session defaults. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Tool Caller
participant SAT as Session-Aware Tool
participant SessionStore as Session Store
participant Handler as Handler Logic
participant Executor as Command Executor
Caller->>SAT: invoke(params)
activate SAT
SAT->>SessionStore: fetch session defaults
activate SessionStore
SessionStore-->>SAT: defaults {scheme, deviceId, ...}
deactivate SessionStore
SAT->>SAT: validate requirements<br/>(scheme required,<br/>one of path/workspace)
alt Requirements met
SAT->>SAT: merge params + defaults
SAT->>Handler: call logic(merged, executor)
activate Handler
Handler->>Executor: executeCommand(...)
activate Executor
Executor-->>Handler: result
deactivate Executor
Handler-->>SAT: response
deactivate Handler
SAT-->>Caller: success
else Requirements unmet
SAT-->>Caller: error (Missing required<br/>session defaults)
end
deactivate SAT
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The changes span 13 files with a consistent session-aware migration pattern but require verification across heterogeneous tool types (device and logging). Logic density is moderate to high in logging tools (session lifecycle, JSON parsing, event handling), and schema/requirements validation patterns must be verified across each tool. The homogeneity of the migration pattern reduces effort slightly, but the breadth of affected tools and mixed test/implementation changes warrant careful review. Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
commit: |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| 'Device Log Capture', | ||
| true, | ||
| undefined, | ||
| true, |
There was a problem hiding this comment.
Bug: Mismatched Arguments in Command Executor
The executor function in start_device_log_cap.ts is called with five arguments, but the CommandExecutor type expects four. The extra true boolean parameter doesn't match the expected signature, which may cause runtime errors or unexpected behavior.
There was a problem hiding this comment.
This is incorrect see:
export type CommandExecutor = (
command: string[],
logPrefix?: string,
useShell?: boolean,
opts?: CommandExecOptions,
detached?: boolean,
) => Promise<CommandResponse>;![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcp/tools/logging/stop_device_log_cap.ts (1)
33-45: Tighten “not found” error message (avoid duplication).Message currently repeats the session id twice. Simplify for clarity.
- text: `Failed to stop device log capture session ${logSessionId}: Device log capture session not found: ${logSessionId}`, + text: `Device log capture session not found: ${logSessionId}`,
🧹 Nitpick comments (14)
src/mcp/tools/logging/__tests__/start_device_log_cap.test.ts (1)
152-406: Consider extracting common mock process creation pattern.The test cases at lines 152-220, 222-315, and 317-406 share a similar pattern for creating mock ChildProcess instances with EventEmitter composition. Consider extracting this into a shared helper function to reduce duplication.
Example:
function createMockChildProcess() { const emitter = new EventEmitter() as unknown as ChildProcess & { /* custom fields */ }; const stubOutput = new EventEmitter() as NodeJS.ReadableStream & { setEncoding?: (encoding: string) => void }; // ... rest of setup return { process: emitter, stubOutput, stubError }; }src/mcp/tools/logging/stop_device_log_cap.ts (4)
50-60: Add SIGKILL fallback if SIGTERM doesn’t finish.If the process ignores SIGTERM, we should escalate after the wait to prevent leaks/hangs.
await waitForSessionToFinish(session); + // Escalate if still running after grace period + if ( + !(session.hasEnded ?? false) && + session.process.killed !== true && + session.process.exitCode == null + ) { + session.process.kill?.('SIGKILL'); + await waitForSessionToFinish(session); + }
61-66: Prefer stream/promises.finished for reliable stream closure.Use the standard finished() helper for end+await semantics instead of manual event wiring.
+import { finished } from 'stream/promises'; @@ async function ensureStreamClosed(stream: fs.WriteStream): Promise<void> { const typedStream = stream as WriteStreamWithClosed; if (typedStream.destroyed || typedStream.closed) { return; } - - await new Promise<void>((resolve) => { - const onClose = (): void => resolve(); - typedStream.once('close', onClose); - typedStream.end(); - }).catch(() => { - // Ignore cleanup errors – best-effort close - }); + try { + typedStream.end(); + await finished(typedStream); + } catch { + // best‑effort close; ignore errors + } }Also applies to: 104-120
65-79: Delete session after reading logs to keep recovery path if read fails.If readFile throws after deletion, the session is gone and recovery gets harder.
- const logFilePath = session.logFilePath; - activeDeviceLogSessions.delete(logSessionId); + const logFilePath = session.logFilePath; @@ - log( + log( 'info', `Device log capture session ${logSessionId} stopped. Log file retained at: ${logFilePath}`, ); + activeDeviceLogSessions.delete(logSessionId);
131-144: Minor: remove implicit TDZ feel; declare timer before closure.Readability tweak: declare the timer before using it inside onClose.
- await new Promise<void>((resolve) => { - const onClose = (): void => { - clearTimeout(timeout); + await new Promise<void>((resolve) => { + let timer: ReturnType<typeof setTimeout>; + const onClose = (): void => { + clearTimeout(timer); session.hasEnded = true; resolve(); }; - const timeout = setTimeout(() => { + timer = setTimeout(() => { session.process.removeListener?.('close', onClose); session.hasEnded = true; resolve(); }, 1000);src/mcp/tools/device/__tests__/install_app_device.test.ts (1)
18-27: Also test success when deviceId is in session defaults.Add a case: sessionStore.setDefaults({ deviceId: 'xyz' }); await handler({ appPath: '...' }) → success.
src/mcp/tools/device/stop_app_device.ts (1)
16-19: Strengthen PID validation.Use integer and positive constraints to reject floats/negatives early.
- processId: z.number().describe('Process ID (PID) of the app to stop'), + processId: z.number().int().positive().describe('Process ID (PID) of the app to stop'),src/mcp/tools/device/__tests__/test_device.test.ts (1)
309-347: Optional: assert testRunnerEnv propagation (if implemented).Consider a command-capture test that verifies env vars are passed to the executor.
src/mcp/tools/device/build_device.ts (1)
67-72: Forward-safety: prefer pick over omit for the public schema.Omit is easy to miss when adding new internal-only fields later. Pick only the intended public keys.
- schema: baseSchemaObject.omit({ - projectPath: true, - workspacePath: true, - scheme: true, - configuration: true, - } as const).shape, + schema: baseSchemaObject.pick({ + derivedDataPath: true, + extraArgs: true, + preferXcodebuild: true, + } as const).shape,src/mcp/tools/device/install_app_device.ts (1)
38-39: Mask UDID in logs to limit PII exposure.Log a short prefix/suffix only.
- log('info', `Installing app on device ${deviceId}`); + const masked = `${deviceId.slice(0,8)}…`; + log('info', `Installing app on device ${masked}`); @@ - text: `✅ App installed successfully on device ${deviceId}\n\n${result.output}`, + text: `✅ App installed successfully on device ${masked}\n\n${result.output}`,Also applies to: 64-65
src/mcp/tools/device/test_device.ts (4)
279-286: Prefer pick over omit for public schema.Same forward-safety rationale as build_device.
- schema: baseSchemaObject.omit({ - projectPath: true, - workspacePath: true, - scheme: true, - deviceId: true, - configuration: true, - } as const).shape, + schema: baseSchemaObject.pick({ + derivedDataPath: true, + extraArgs: true, + preferXcodebuild: true, + platform: true, + testRunnerEnv: true, + } as const).shape,
286-297: Don’t eagerly bind the real FS executor in the handler.Let testDeviceLogic’s default parameter resolve the FS executor. Easier to mock in tests and avoids accidental real FS in unit tests.
handler: createSessionAwareTool<TestDeviceParams>({ internalSchema: testDeviceSchema as unknown as z.ZodType<TestDeviceParams>, - logicFunction: (params: TestDeviceParams, executor: CommandExecutor) => - testDeviceLogic( - { - ...params, - platform: params.platform ?? 'iOS', - }, - executor, - getDefaultFileSystemExecutor(), - ), + logicFunction: (params: TestDeviceParams, executor: CommandExecutor) => + testDeviceLogic( + { ...params, platform: params.platform ?? 'iOS' }, + executor + ),
222-224: Default preferXcodebuild to false for consistency.Matches build_device; avoids undefined behavior differences.
- params.preferXcodebuild, + params.preferXcodebuild ?? false,
245-273: Single cleanup path.cleanup() is called in try, catch, and finally. Keep only in finally to avoid double rm calls.
- // Clean up temporary directory - await cleanup(); @@ - await cleanup(); @@ } catch (error) { @@ } finally { await cleanup(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
docs/session-aware-migration-todo.md(1 hunks)src/mcp/tools/device/__tests__/build_device.test.ts(5 hunks)src/mcp/tools/device/__tests__/get_device_app_path.test.ts(3 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__/stop_app_device.test.ts(1 hunks)src/mcp/tools/device/__tests__/test_device.test.ts(2 hunks)src/mcp/tools/device/build_device.ts(2 hunks)src/mcp/tools/device/get_device_app_path.ts(2 hunks)src/mcp/tools/device/install_app_device.ts(2 hunks)src/mcp/tools/device/launch_app_device.ts(2 hunks)src/mcp/tools/device/stop_app_device.ts(2 hunks)src/mcp/tools/device/test_device.ts(2 hunks)src/mcp/tools/logging/__tests__/start_device_log_cap.test.ts(4 hunks)src/mcp/tools/logging/__tests__/stop_device_log_cap.test.ts(7 hunks)src/mcp/tools/logging/start_device_log_cap.ts(5 hunks)src/mcp/tools/logging/stop_device_log_cap.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.ts: Do not use Vitest mocking APIs in tests (no vi.mock, vi.fn, vi.spyOn, etc.)
In tests, import the core ...Logic function from the tool and use createMockExecutor/createMockFileSystemExecutor to simulate outcomes
Files:
src/mcp/tools/device/__tests__/launch_app_device.test.tssrc/mcp/tools/logging/__tests__/start_device_log_cap.test.tssrc/mcp/tools/device/__tests__/get_device_app_path.test.tssrc/mcp/tools/device/__tests__/test_device.test.tssrc/mcp/tools/logging/__tests__/stop_device_log_cap.test.tssrc/mcp/tools/device/__tests__/install_app_device.test.tssrc/mcp/tools/device/__tests__/stop_app_device.test.tssrc/mcp/tools/device/__tests__/build_device.test.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: Use .ts extensions for all internal relative imports (e.g., import { tool } from './tool.ts')
Use .ts extensions in re-exports for internal files (e.g., export { default } from '../shared/tool.ts')
Use .js extension only for external package entry points (e.g., import ... from '@scope/pkg/file.js')
Never import internal project files using .js extensions
Files:
src/mcp/tools/device/__tests__/launch_app_device.test.tssrc/mcp/tools/logging/__tests__/start_device_log_cap.test.tssrc/mcp/tools/device/__tests__/get_device_app_path.test.tssrc/mcp/tools/device/__tests__/test_device.test.tssrc/mcp/tools/device/get_device_app_path.tssrc/mcp/tools/logging/stop_device_log_cap.tssrc/mcp/tools/logging/__tests__/stop_device_log_cap.test.tssrc/mcp/tools/device/__tests__/install_app_device.test.tssrc/mcp/tools/device/test_device.tssrc/mcp/tools/device/stop_app_device.tssrc/mcp/tools/device/install_app_device.tssrc/mcp/tools/device/launch_app_device.tssrc/mcp/tools/device/build_device.tssrc/mcp/tools/logging/start_device_log_cap.tssrc/mcp/tools/device/__tests__/stop_app_device.test.tssrc/mcp/tools/device/__tests__/build_device.test.ts
🧠 Learnings (1)
📚 Learning: 2025-09-21T19:27:22.795Z
Learnt from: CR
PR: cameroncooke/XcodeBuildMCP#0
File: AGENTS.md:0-0
Timestamp: 2025-09-21T19:27:22.795Z
Learning: Applies to **/*.{test,spec}.ts : In tests, import the core ...Logic function from the tool and use createMockExecutor/createMockFileSystemExecutor to simulate outcomes
Applied to files:
src/mcp/tools/device/launch_app_device.tssrc/mcp/tools/device/__tests__/build_device.test.ts
🧬 Code graph analysis (16)
src/mcp/tools/device/__tests__/launch_app_device.test.ts (1)
src/utils/session-store.ts (1)
sessionStore(47-47)
src/mcp/tools/logging/__tests__/start_device_log_cap.test.ts (3)
src/utils/session-store.ts (1)
sessionStore(47-47)src/mcp/tools/logging/start_device_log_cap.ts (2)
activeDeviceLogSessions(41-41)start_device_log_capLogic(641-677)src/test-utils/mock-executors.ts (2)
createMockExecutor(27-75)createMockFileSystemExecutor(165-181)
src/mcp/tools/device/__tests__/get_device_app_path.test.ts (1)
src/utils/session-store.ts (1)
sessionStore(47-47)
src/mcp/tools/device/__tests__/test_device.test.ts (1)
src/utils/session-store.ts (1)
sessionStore(47-47)
src/mcp/tools/device/get_device_app_path.ts (2)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)src/utils/execution/index.ts (1)
getDefaultCommandExecutor(5-5)
src/mcp/tools/logging/stop_device_log_cap.ts (2)
src/mcp/tools/logging/start_device_log_cap.ts (2)
activeDeviceLogSessions(41-41)DeviceLogSession(32-39)src/utils/logger.ts (1)
log(142-162)
src/mcp/tools/logging/__tests__/stop_device_log_cap.test.ts (1)
src/mcp/tools/logging/start_device_log_cap.ts (1)
DeviceLogSession(32-39)
src/mcp/tools/device/__tests__/install_app_device.test.ts (1)
src/utils/session-store.ts (1)
sessionStore(47-47)
src/mcp/tools/device/test_device.ts (3)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)src/utils/CommandExecutor.ts (1)
CommandExecutor(11-17)src/utils/command.ts (3)
CommandExecutor(20-20)getDefaultFileSystemExecutor(216-227)getDefaultCommandExecutor(199-210)
src/mcp/tools/device/stop_app_device.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/device/install_app_device.ts (2)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)src/utils/execution/index.ts (1)
getDefaultCommandExecutor(5-5)
src/mcp/tools/device/launch_app_device.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/device/build_device.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/logging/start_device_log_cap.ts (3)
src/utils/command.ts (1)
getDefaultCommandExecutor(199-210)src/utils/logging/index.ts (1)
log(5-5)src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/device/__tests__/stop_app_device.test.ts (1)
src/utils/session-store.ts (1)
sessionStore(47-47)
src/mcp/tools/device/__tests__/build_device.test.ts (1)
src/utils/session-store.ts (1)
sessionStore(47-47)
⏰ 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: code-review
🔇 Additional comments (19)
src/mcp/tools/device/__tests__/install_app_device.test.ts (3)
14-16: Good: isolate tests by clearing session defaults.
42-49: Public schema minimalism looks right.Only appPath is exposed; deviceId is enforced via handler requirements. Matches session-aware pattern.
35-36: Description update LGTM.src/mcp/tools/device/stop_app_device.ts (1)
87-94: Session-aware wrapper + public schema shaping: LGTM.Omitting deviceId publicly while requiring it in handler aligns with the suite.
src/mcp/tools/device/__tests__/build_device.test.ts (4)
23-25: Description update LGTM.
31-41: Public schema surface is appropriately constrained.Only ['derivedDataPath','extraArgs','preferXcodebuild'] exposed; project/workspace hidden.
50-65: XOR and requirements messaging assertions look correct.Matches factory errors: “Missing required session defaults” and “Mutually exclusive...”.
74-91: Validation error expectations read well.Covers type error plus session-defaults tip per factory behavior.
src/mcp/tools/device/__tests__/test_device.test.ts (3)
18-21: Good: session defaults cleared before each test.
35-57: Public schema exposure is tight and consistent.
111-141: Requirements and XOR tests: solid coverage.Covers both missing defaults and mutual exclusivity errors.
src/mcp/tools/device/__tests__/get_device_app_path.test.ts (4)
24-26: Description update LGTM.
33-41: Public schema limited to platform: good.Prevents leaking session-managed inputs at the surface.
45-64: XOR error messaging expectations match the factory.
66-82: Requirements tests read correctly (scheme required; project/workspace needed).src/mcp/tools/device/build_device.ts (1)
73-82: Session-aware handler wiring looks solid.Requirements and exclusivePairs align with internal XOR; early factory checks prevent ambiguous input. LGTM.
If external callers depend on the old public surface (projectPath/workspacePath/scheme), confirm docs/examples were updated accordingly.
src/mcp/tools/device/test_device.ts (1)
24-24: Import change to session-aware factory: LGTM.Aligns this tool with the new pattern.
src/mcp/tools/device/install_app_device.ts (2)
85-92: Public surface + requirements: LGTM.Schema only exposes appPath; deviceId enforced via requirements. Consistent with session-defaults pattern.
Ensure tests mock utils/execution getDefaultCommandExecutor to avoid the “REAL SYSTEM EXECUTOR DETECTED IN TEST” guard.
41-46: Avoid running via shell to reduce injection risk.Device IDs and paths can contain special chars. Prefer spawning without a shell.
- const result = await executor( - ['xcrun', 'devicectl', 'device', 'install', 'app', '--device', deviceId, appPath], - 'Install app on device', - true, // useShell - undefined, // env - ); + const result = await executor( + ['xcrun', 'devicectl', 'device', 'install', 'app', '--device', deviceId, appPath], + 'Install app on device', + false, // useShell: safer + undefined, // env + );If CommandExecutor mandates useShell for this command, point me to the contract and I’ll adjust the recommendation.
1 participant
Summary
Testing
Note
Converts device tools to session-aware handlers with minimized public schemas and adds robust devicectl JSON-based launch detection for device log capture, updating tests and docs accordingly.
build_device,test_device,get_device_app_path,install_app_device,launch_app_device,stop_app_devicetocreateSessionAwareToolwith requirement rules and mutual exclusivity checks.projectPath,workspacePath,scheme,deviceId,configuration), keeping only tuning/required fields.start_device_log_capto use--json-outputand parse structured results; detect early launch failures from stderr/output; manage sessions via typedactiveDeviceLogSessionswith stream/process lifecycle and cleanup; support wait tuning viaXBMCP_LAUNCH_JSON_WAIT_MS.stop_device_log_capto work with new session shape; wait for process end, ensure stream closure, and improve error handling.sessionStore; add extensive cases for JSON success/failure, early-exit errors, and cleanup behaviors.docs/session-aware-migration-todo.mdchecklist to mark device and logging items as completed.Written by Cursor Bugbot for commit 24bdd1b. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Release Notes
New Features
Improvements