feat: improve Sentry logging with xcodebuild exit code classification #98
Conversation
Implements intelligent Sentry error classification to reduce noise by distinguishing between MCP server bugs and user domain errors based on xcodebuild exit codes. ## Changes Made ### Core Implementation - **CommandResponse Interface**: Added `exitCode` field to capture process exit codes for error classification - **Command Executor**: Enhanced to capture and return exit codes from child processes - **Logger Enhancement**: Added `LogContext` interface to allow selective Sentry capture control - **Build Utils Classification**: Implemented exit code-based Sentry routing logic ### Classification Logic - **Exit Code 64**: Invalid command-line arguments (MCP server error) → Logs to Sentry as ERROR level - **All Other Exit Codes**: User domain errors (missing files, compilation failures, etc.) → Logs as WARNING level, no Sentry - **Spawn Errors**: Environment issues (ENOENT, EACCES, EPERM) → No Sentry logging ### Testing - **Enhanced Mock Executor**: Added support for `exitCode` and `shouldThrow` parameters for comprehensive testing - **Complete Test Coverage**: 11 new test cases covering all classification scenarios and edge cases - **Manual Validation**: Verified real-world behavior using Reloaderoo CLI testing tool ## Benefits - **Reduced Noise**: Eliminates user compilation errors from Sentry monitoring - **Focused Alerts**: Only genuine MCP server bugs trigger Sentry notifications - **Empirical Approach**: Based on actual xcodebuild exit code testing rather than heuristics - **Backward Compatible**: No breaking changes to existing functionality ## Validation - All 1057 existing tests pass - TypeScript compilation clean - ESLint validation clean - Manual testing confirms correct classification behavior
WalkthroughAdds exitCode propagation across command execution, updates mock executors to support exitCode and forced throws, refines executeXcodeBuildCommand logging to distinguish MCP vs other failures and spawn errors, introduces a log context with optional sentry control, and adds tests covering exit-code- and spawn-error–based behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant BuildUtils as executeXcodeBuildCommand
participant Exec as Command Executor
participant Logger
Caller->>BuildUtils: run xcodebuild
BuildUtils->>Exec: execute(command)
Exec-->>BuildUtils: { success, output, error, exitCode }
alt success
BuildUtils->>Logger: log("info", successMsg, {sentry:false})
else failure with exitCode
alt exitCode == 64 (MCP)
BuildUtils->>Logger: log("error", mcpMsg, {sentry:true})
else other exitCode
BuildUtils->>Logger: log("warn", nonMcpMsg, {sentry:false})
end
else throw
BuildUtils->>Logger: log("error", errMsg, {sentry: !isSpawnError})
end
BuildUtils-->>Caller: ToolResponse
sequenceDiagram
participant Caller
participant Logger
participant Sentry
Caller->>Logger: log(level, message, context?)
Logger->>Logger: captureToSentry = SENTRY && (context.sentry ?? level=="error")
alt captureToSentry
Logger->>Sentry: capture(message)
end
Logger-->>Caller: done
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Claude finished @cameroncooke's task —— View job Code Review: Sentry Logging EnhancementReview Tasks
Overall Assessment: ✅ Excellent ImplementationThis is a well-architected solution that addresses a real operational problem with intelligent error classification. The implementation demonstrates strong engineering practices and thorough testing. 🎯 Strengths1. Empirical Approach
2. Clean Architecture
3. Robust Testing
4. Operational Impact
📋 Detailed Code ReviewCommandResponse Interface (
|
|
cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (10)
src/utils/CommandExecutor.ts (1)
22-23: Document the semantics ofexitCodefor clarity.Great addition. Consider a short JSDoc to make it explicit that this is only populated for non-detached executions and may be absent if the process exited via signal.
process: ChildProcess; - exitCode?: number; + /** + * POSIX exit code for the finished process (0 indicates success). + * Present for non-detached executions; undefined if unavailable (e.g., process exited via signal). + */ + exitCode?: number; }src/utils/command.ts (1)
122-131: Optional: also capture termination signal for richer diagnostics.If useful, extend
CommandResponseto include asignal?: NodeJS.Signalscaptured from theclosehandler’s second parameter. This helps distinguish non-zero exit vs signal termination.Example change here:
- childProcess.on('close', (code) => { + childProcess.on('close', (code, signal) => { const success = code === 0; const response: CommandResponse = { success, output: stdout, error: success ? undefined : stderr, process: childProcess, exitCode: code ?? undefined, + // signal, }; resolve(response); });And add
signal?: NodeJS.Signals;to theCommandResponseinterface (insrc/utils/CommandExecutor.ts).src/utils/logger.ts (1)
140-156: Send Sentry severity to reflect log level (improves triage in Sentry).Currently
captureMessageis called without a severity, which defaults to "info" in Sentry. Forwarding a mapped severity aligned with your logger levels makes Sentry issues easier to prioritize.Apply this diff within the selected range:
- if (captureToSentry) { - withSentry((s) => s.captureMessage(logMessage)); - } + if (captureToSentry) { + withSentry((s) => s.captureMessage(logMessage, mapToSentrySeverity(level))); + }Add the following helper (outside the selected range) and a type-only import to avoid runtime loading:
// At top of file (type-only, no runtime import) import type { SeverityLevel } from '@sentry/node'; // Below helpers function mapToSentrySeverity(level: string): SeverityLevel { const l = level.toLowerCase(); switch (l) { case 'emergency': case 'alert': case 'critical': return 'fatal'; case 'error': return 'error'; case 'warning': return 'warning'; case 'notice': return 'log'; case 'info': return 'info'; case 'debug': default: return 'debug'; } }Note:
import typeis erased at compile time and won’t load native modules at import.src/utils/build-utils.ts (2)
250-257: Good classification; include exit code in the log line for faster diagnosis.The MCP vs user error split is clear. Adding the exit code to the log message will speed up triage and correlate with tests and manual CLI checks.
- log( - isMcpError ? 'error' : 'warning', - `${platformOptions.logPrefix} ${buildAction} failed: ${result.error}`, - { sentry: isMcpError }, - ); + log( + isMcpError ? 'error' : 'warning', + `${platformOptions.logPrefix} ${buildAction} failed${ + result.exitCode !== undefined ? ` (exitCode: ${result.exitCode})` : '' + }: ${result.error}`, + { sentry: isMcpError }, + );
347-355: Spawn error gating looks right; consider extending the set if needed.Treating ENOENT/EACCES/EPERM as environment issues is sensible. If you see more spawn-related noise, consider extending to include e.g., E2BIG or ENOTDIR, but only as data justifies.
src/utils/__tests__/build-utils.test.ts (1)
1-264: Solid coverage of exit-code and spawn-error classification paths.The tests validate the main scenarios and keep assertions focused on user-visible output. Nice balance. If you adopt the “include exit code in log message” tweak, consider adding one assertion that the exit code appears in the error summary.
src/test-utils/mock-executors.ts (4)
34-36: Type shouldThrow as NodeJS.ErrnoException to carry spawn error metadataClassifying spawn errors typically relies on error.code (e.g., ENOENT, EACCES). Typing shouldThrow as NodeJS.ErrnoException preserves these fields without casting at call sites.
Apply this diff:
- shouldThrow?: Error; + shouldThrow?: NodeJS.ErrnoException;
117-118: Consider adding shouldThrow per-command for parity and richer scenariosAdding shouldThrow to per-command mocks makes it easy to simulate spawn errors for specific commands (e.g., ENOENT for a missing tool) without switching helper utilities.
Type update:
commandMap: Record< string, { success?: boolean; output?: string; error?: string; process?: unknown; exitCode?: number; + shouldThrow?: NodeJS.ErrnoException; } >,And short-circuit after lookup:
const result = commandMap[matchedKey]; + if (result.shouldThrow) { + throw result.shouldThrow; + }
146-158: Mirror success-from-exitCode defaulting here as wellSame potential contradiction exists in the multi-command executor. Aligning success with exitCode when success is omitted avoids accidental success: true with non-zero exitCode.
Apply this diff:
return { - success: result.success ?? true, // Success by default (as discussed) + success: + result.success ?? + (result.exitCode === undefined ? true : result.exitCode === 0), output: result.output ?? '', error: result.error, process: (result.process ?? mockProcess) as ChildProcess, exitCode: result.exitCode ?? (result.success === false ? 1 : 0), };
62-74: No inconsistent mocks found
Our repository-wide search forcreateMockExecutor({ … exitCode: … })andcreateCommandMatchingMockExecutor({ … exitCode: … })returned no occurrences. There are currently no mocks that set an explicit non-zeroexitCodewithout also specifyingsuccess, so no existing tests will start reporting contradictory states.You can still apply this optional refactor to derive
successfromexitCodewhen provided to prevent future inconsistencies:--- a/src/test-utils/mock-executors.ts +++ b/src/test-utils/mock-executors.ts @@ -62,7 +62,11 @@ export function createMockExecutor(result: Partial<ExecutorResult>) { return async () => ({ - success: result.success ?? true, + success: + result.success ?? + (result.exitCode === undefined ? true : result.exitCode === 0), output: result.output ?? '', error: result.error, process: (result.process ?? mockProcess) as ChildProcess,
📜 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 (6)
src/test-utils/mock-executors.ts(7 hunks)src/utils/CommandExecutor.ts(1 hunks)src/utils/__tests__/build-utils.test.ts(1 hunks)src/utils/build-utils.ts(2 hunks)src/utils/command.ts(1 hunks)src/utils/logger.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/utils/logger.ts (1)
src/utils/logging/index.ts (1)
log(5-5)
⏰ 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 (3)
src/utils/command.ts (1)
122-131: LGTM: Exit code propagation is correctly captured on 'close'.Using the 'close' event ensures stdio is fully drained, and
exitCodeis accurately surfaced. The nullish coalescing keeps the type aligned withnumber | undefined. All good.src/utils/logger.ts (1)
40-46: LGTM: Context-based Sentry gating is clean and backward compatible.The
LogContextshape is minimal and effective; optional usage avoids breaking existing call sites.src/test-utils/mock-executors.ts (1)
47-53: Early-throw path is correct and preserves real-world spawn behaviorGood precedence: shouldThrow short-circuits normal result shaping, mirroring actual spawn failures. This keeps tests faithful to runtime behavior.
1 participant
Summary
Implements intelligent Sentry error classification to reduce noise by distinguishing between MCP server bugs and user domain errors based on xcodebuild exit codes. This prevents user compilation errors from cluttering Sentry monitoring while preserving visibility into genuine server issues.
Background/Details
Root Cause Analysis
Previously, ALL xcodebuild failures were logged to Sentry as errors, creating significant noise. The challenge was differentiating between:
Empirical Research Approach
Rather than relying on heuristics, conducted comprehensive testing of xcodebuild exit codes:
Solution
Core Implementation
exitCodefield to capture process exit codesLogContextinterface for selective Sentry capture controlClassification Logic
Spawn Error Handling
Testing
Enhanced Mock Infrastructure
exitCodeandshouldThrowparameters for comprehensive testingManual Validation
Verified real-world behavior using Reloaderoo CLI testing tool:
--invalid-flag-that-does-not-exist→ ERROR level + Sentry/nonexistent/project.xcodeproj→ WARNING level, no SentryQuality Assurance
Notes
Deployment Considerations
This change will immediately reduce Sentry noise by filtering out user compilation errors while preserving visibility into genuine MCP server bugs. Monitor Sentry alert volume to confirm the improvement.
Summary by CodeRabbit