Rename diagnostics tool to doctor #92
Conversation
WalkthroughRenames and replaces the “diagnostic” tool/CLI with a new “doctor” tool/CLI across code, resources, tests, and docs. Adds a doctor resource, deletes the environment resource, implements dependency-injected doctor logic and dependencies, updates build/package configs and workflows, adjusts discovery strings, and refines logging/Sentry environment handling. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as Doctor CLI
participant Tool as Doctor Tool
participant Deps as Doctor Dependencies
User->>CLI: Run xcodebuildmcp-doctor
CLI->>Tool: doctorLogic(params, executor, showAsciiLogo=true)
Tool->>Deps: binary checks / xcode / env / plugins / runtime
Deps-->>Tool: aggregated info
Tool-->>CLI: ToolResponse (text report)
CLI-->>User: Print doctor report
sequenceDiagram
participant Client
participant Server as MCP Server
participant Resource as Doctor Resource
participant Tool as Doctor Tool
Client->>Server: Request xcodebuildmcp://doctor
Server->>Resource: Invoke handler()
Resource->>Tool: doctorLogic({}, executor)
Tool-->>Resource: ToolResponse (text)
Resource-->>Server: contents[{text}]
Server-->>Client: Doctor report
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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Claude finished @cameroncooke's task —— View job Code Review Complete ✅Reviewed PR #92: Rename diagnostics tool to doctor Todo List:
Review SummaryThis is a comprehensive and well-executed renaming effort that successfully transforms the ✅ Strengths1. Complete Implementation Coverage
2. Excellent Code Quality
3. Strong Test Coverage
4. Architecture Preservation
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (3)
src/utils/sentry.ts (1)
80-96: Honor SENTRY_DISABLED to fully disable Sentry initializationDocs say users can opt-out via SENTRY_DISABLED=true, but init currently ignores it. Use the Sentry “enabled” flag so we don’t initialize when disabled.
Apply this diff:
Sentry.init({ dsn: 'https://798607831167c7b9fe2f2912f5d3c665@o4509258288332800.ingest.de.sentry.io/4509258293837904', // Setting this option to true will send default PII data to Sentry // For example, automatic IP address collection on events sendDefaultPii: true, // Set release version to match application version release: `xcodebuildmcp@${version}`, // Set environment based on NODE_ENV environment: process.env.NODE_ENV ?? 'development', // Set tracesSampleRate to 1.0 to capture 100% of transactions for performance monitoring // We recommend adjusting this value in production tracesSampleRate: 1.0, + // Respect opt-out flag from environment + enabled: process.env.SENTRY_DISABLED !== 'true', });docs/CONTRIBUTING.md (1)
210-217: Script name mismatch: use “npm run dev” (no build:watch script exists)Package.json doesn’t define build:watch, but does have a dev script that runs tsup in watch mode.
Apply this diff:
-# Terminal 2: Start build watcher -npm run build:watch +# Terminal 2: Start build watcher +npm run devsrc/mcp/resources/__tests__/environment.test.ts (1)
3-3: Fix import path - environment resource no longer exists.The test is importing from '../environment.js' but according to the pipeline failure and PR changes, this file was removed and replaced with the doctor resource.
Apply this diff to fix the import:
-import environmentResource, { environmentResourceLogic } from '../environment.js'; +import environmentResource, { environmentResourceLogic } from '../doctor.js';The import should reference
doctorResourceLogicfrom the new doctor resource file.
🧹 Nitpick comments (9)
docs/PLUGIN_DEVELOPMENT.md (1)
303-349: Nit: Resource path should match current codebase structureDocs say resources live under
src/resources/, but elsewhere and in code they live undersrc/mcp/resources/. Consider correcting for accuracy.Apply this minimal doc tweak:
-Resources are located in `src/resources/` and follow this pattern: +Resources are located in `src/mcp/resources/` and follow this pattern:src/utils/logger.ts (1)
35-41: Add log silencing env: works as intended; consider preserving Sentry on errorsEarly-return now also silences when
XCODEBUILDMCP_SILENCE_LOGS === 'true'. Two optional refinements:
- Keep error events flowing to Sentry even when console logs are silenced.
- Support broader “truthy” values (e.g., 1/yes/TRUE) for env.
Option A: Still capture errors to Sentry when silenced (silence only console output):
export function log(level: string, message: string): void { - // Suppress logging during tests to keep test output clean - if ( - process.env.VITEST === 'true' || - process.env.NODE_ENV === 'test' || - process.env.XCODEBUILDMCP_SILENCE_LOGS === 'true' - ) { - return; - } + // Suppress console logging during tests or when explicitly silenced + const silenceLogs = process.env.XCODEBUILDMCP_SILENCE_LOGS === 'true'; + const inTests = process.env.VITEST === 'true' || process.env.NODE_ENV === 'test'; const timestamp = new Date().toISOString(); const logMessage = `[${timestamp}] [${level.toUpperCase()}] ${message}`; if (level === 'error' && SENTRY_ENABLED) { Sentry.captureMessage(logMessage); } + if (inTests || silenceLogs) { + return; + } console.error(logMessage); }Option B: Accept broader truthy env values (define helper outside and use inside):
// place near top-level const isTruthy = (v?: string) => !!v && ['true', '1', 'yes'].includes(v.toLowerCase());Then use:
-const silenceLogs = process.env.XCODEBUILDMCP_SILENCE_LOGS === 'true'; +const silenceLogs = isTruthy(process.env.XCODEBUILDMCP_SILENCE_LOGS);Also confirm docs mention
XCODEBUILDMCP_SILENCE_LOGSso users discover it. I can add the env var to the relevant docs if desired.src/utils/sentry.ts (2)
31-36: Collecting XCODEBUILDMCP_DYNAMIC_TOOLS is good; also surface it as a Sentry tagYou added the env var to the collector but it’s not exported to tags. Recommend tagging it for debugging parity with other env flags.
Apply this diff near the existing env tags:
const envVars = getEnvironmentVariables(); tags.env_XCODEBUILDMCP_DEBUG = envVars['XCODEBUILDMCP_DEBUG'] ?? 'false'; tags.env_XCODEMAKE_ENABLED = envVars['INCREMENTAL_BUILDS_ENABLED'] ?? 'false'; +tags.env_XCODEBUILDMCP_DYNAMIC_TOOLS = envVars['XCODEBUILDMCP_DYNAMIC_TOOLS'] ?? 'false';Also applies to: 115-118
13-29: Direct execSync usage violates DI guideline for external interactionsPer repo guidelines for TS files, shell commands should go through injectable executors. Consider moving these helpers behind a DI boundary so they can be controlled/disabled in tests and early init, or lazily run post-startup.
I can propose a small adapter that accepts a CommandExecutor and defaults to execSync to maintain current behavior.
Also applies to: 53-78
README.md (2)
381-383: Typo: “MseeP.a” → “MseeP.ai”Minor typo in the section header.
Apply this diff:
-### MseeP.a +### MseeP.ai
194-201: Typo: “Samplng” → “Sampling”Fix the column header spelling in the MCP Compatibility table.
Apply this diff:
-| Editor | Tools | Resources | Samplng | +| Editor | Tools | Resources | Sampling |src/mcp/tools/doctor/lib/doctor.deps.ts (2)
185-197: Harden os.userInfo() and avoid calling os.cpus() twiceos.userInfo() can throw in some environments; guard it. Also cache os.cpus() to avoid double call.
- getSystemInfo() { - return { - platform: os.platform(), - release: os.release(), - arch: os.arch(), - cpus: `${os.cpus().length} x ${os.cpus()[0]?.model ?? 'Unknown'}`, - memory: `${Math.round(os.totalmem() / (1024 * 1024 * 1024))} GB`, - hostname: os.hostname(), - username: os.userInfo().username, - homedir: os.homedir(), - tmpdir: os.tmpdir(), - }; - }, + getSystemInfo() { + const cpuInfo = os.cpus(); + let username = 'Unknown'; + try { + username = os.userInfo().username; + } catch { + username = process.env.USER ?? process.env.LOGNAME ?? 'Unknown'; + } + return { + platform: os.platform(), + release: os.release(), + arch: os.arch(), + cpus: `${cpuInfo.length} x ${cpuInfo[0]?.model ?? 'Unknown'}`, + memory: `${Math.round(os.totalmem() / (1024 * 1024 * 1024))} GB`, + hostname: os.hostname(), + username, + homedir: os.homedir(), + tmpdir: os.tmpdir(), + }; + },
112-126: Minor: avoid re-allocating versionCommands per callDefine versionCommands once at module scope for clarity and minor perf.
For example:
// At module top-level: const VERSION_COMMANDS: Record<string, string[]> = { axe: ['axe', '--version'], mise: ['mise', '--version'], }; // In checkBinaryAvailability: const cmd = VERSION_COMMANDS[binary]; if (cmd) { try { const res = await executor(cmd, 'Get Binary Version'); if (res.success && res.output) version = res.output.trim(); } catch {} }src/mcp/tools/discovery/discover_tools.ts (1)
372-372: Description should also mention the doctor workflow for consistencyThe description now lists only “(simulator-management, logging)” while the prompt includes “doctor”. Recommend adding doctor for parity.
- description: - 'Analyzes a natural language task description and enables the most relevant development workflow. Prioritizes project/workspace workflows (simulator/device/macOS) and also supports task-based workflows (simulator-management, logging) and Swift packages.', + description: + 'Analyzes a natural language task description and enables the most relevant development workflow. Prioritizes project/workspace workflows (simulator/device/macOS) and also supports task-based workflows (simulator-management, logging, doctor) and Swift packages.',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (32)
.github/ISSUE_TEMPLATE/bug_report.yml(1 hunks).github/workflows/claude.yml(1 hunks).vscode/settings.json(1 hunks).vscode/tasks.json(1 hunks)AGENTS.md(1 hunks)README.md(2 hunks)docs/ARCHITECTURE.md(5 hunks)docs/CONTRIBUTING.md(1 hunks)docs/MANUAL_TESTING.md(6 hunks)docs/PLUGIN_DEVELOPMENT.md(1 hunks)docs/RELOADEROO.md(4 hunks)docs/TESTING.md(6 hunks)docs/TOOLS.md(2 hunks)package.json(2 hunks)src/diagnostic-cli.ts(0 hunks)src/doctor-cli.ts(1 hunks)src/mcp/resources/__tests__/environment.test.ts(3 hunks)src/mcp/resources/doctor.ts(1 hunks)src/mcp/resources/environment.ts(0 hunks)src/mcp/tools/diagnostics/__tests__/diagnostic.test.ts(0 hunks)src/mcp/tools/diagnostics/diagnostic.ts(0 hunks)src/mcp/tools/diagnostics/index.ts(0 hunks)src/mcp/tools/discovery/__tests__/discover_tools.test.ts(1 hunks)src/mcp/tools/discovery/discover_tools.ts(2 hunks)src/mcp/tools/doctor/__tests__/doctor.test.ts(1 hunks)src/mcp/tools/doctor/__tests__/index.test.ts(4 hunks)src/mcp/tools/doctor/doctor.ts(1 hunks)src/mcp/tools/doctor/index.ts(1 hunks)src/mcp/tools/doctor/lib/doctor.deps.ts(1 hunks)src/utils/logger.ts(1 hunks)src/utils/sentry.ts(2 hunks)tsup.config.ts(2 hunks)
💤 Files with no reviewable changes (5)
- src/mcp/tools/diagnostics/tests/diagnostic.test.ts
- src/mcp/tools/diagnostics/index.ts
- src/diagnostic-cli.ts
- src/mcp/resources/environment.ts
- src/mcp/tools/diagnostics/diagnostic.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.test.{ts,tsx,js,jsx}: Do not usevi.mock(),vi.fn(),vi.spyOn(), etc. in tests; Vitest mocking is completely banned.
Tests should import the core...Logicfunction from a tool file and pass in a mock executor (createMockExecutororcreateMockFileSystemExecutor) to simulate different outcomes.
Files:
src/mcp/tools/discovery/__tests__/discover_tools.test.tssrc/mcp/tools/doctor/__tests__/index.test.tssrc/mcp/tools/doctor/__tests__/doctor.test.tssrc/mcp/resources/__tests__/environment.test.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
All external interactions (like running commands or accessing the file system) must be handled through injectable executors (
CommandExecutor,FileSystemExecutor).
Files:
src/mcp/tools/discovery/__tests__/discover_tools.test.tssrc/mcp/tools/doctor/__tests__/index.test.tssrc/mcp/tools/doctor/index.tssrc/utils/sentry.tssrc/mcp/tools/doctor/__tests__/doctor.test.tssrc/doctor-cli.tssrc/utils/logger.tssrc/mcp/resources/__tests__/environment.test.tstsup.config.tssrc/mcp/tools/doctor/doctor.tssrc/mcp/tools/doctor/lib/doctor.deps.tssrc/mcp/tools/discovery/discover_tools.tssrc/mcp/resources/doctor.ts
🧠 Learnings (1)
📚 Learning: 2025-08-06T11:44:09.150Z
Learnt from: CR
PR: cameroncooke/XcodeBuildMCP#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-06T11:44:09.150Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Tests should import the core `...Logic` function from a tool file and pass in a mock executor (`createMockExecutor` or `createMockFileSystemExecutor`) to simulate different outcomes.
Applied to files:
src/mcp/resources/__tests__/environment.test.ts
🧬 Code Graph Analysis (7)
src/mcp/tools/doctor/__tests__/index.test.ts (1)
src/mcp/tools/doctor/index.ts (1)
workflow(1-13)
src/mcp/tools/doctor/__tests__/doctor.test.ts (3)
src/mcp/tools/doctor/lib/doctor.deps.ts (1)
DoctorDependencies(86-93)src/mcp/tools/doctor/doctor.ts (2)
DoctorDependencies(278-278)runDoctor(29-251)build-plugins/plugin-discovery.js (1)
result(172-175)
src/doctor-cli.ts (2)
src/mcp/tools/doctor/doctor.ts (2)
runDoctor(29-251)doctorLogic(253-260)src/utils/command.ts (1)
getDefaultCommandExecutor(232-243)
src/mcp/resources/__tests__/environment.test.ts (2)
build-plugins/plugin-discovery.js (1)
result(172-175)src/utils/command.ts (1)
createMockExecutor(267-305)
src/mcp/tools/doctor/doctor.ts (4)
src/mcp/tools/doctor/lib/doctor.deps.ts (3)
DoctorDependencies(86-93)CommandExecutor(280-280)createDoctorDependencies(95-278)src/utils/logger.ts (1)
log(33-53)src/utils/typed-tool-factory.ts (1)
createTypedTool(29-60)src/utils/command.ts (1)
getDefaultCommandExecutor(232-243)
src/mcp/tools/doctor/lib/doctor.deps.ts (4)
src/utils/axe-helpers.ts (1)
areAxeToolsAvailable(44-46)src/core/plugin-registry.ts (2)
loadWorkflowGroups(24-66)loadPlugins(4-19)src/core/dynamic-tools.ts (1)
getEnabledWorkflows(64-66)src/utils/tool-registry.ts (1)
getTrackedToolNames(110-112)
src/mcp/resources/doctor.ts (4)
src/mcp/tools/doctor/lib/doctor.deps.ts (1)
CommandExecutor(280-280)src/utils/command.ts (1)
getDefaultCommandExecutor(232-243)src/utils/logger.ts (1)
log(33-53)src/mcp/tools/doctor/doctor.ts (1)
doctorLogic(253-260)
🪛 YAMLlint (1.37.1)
.github/ISSUE_TEMPLATE/bug_report.yml
[error] 35-35: trailing spaces
(trailing-spaces)
🪛 GitHub Actions: CI
src/mcp/resources/__tests__/environment.test.ts
[error] 2-2: npm test failed: Cannot find module '../environment.js' imported by environment.test.ts. Does the file exist?
⏰ 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). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: claude-review
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (38)
.vscode/tasks.json (1)
101-110: VS Code task label rename looks goodLabel updated to “build: dev doctor” aligns with repo-wide rename. No functional impact; LGTM.
docs/PLUGIN_DEVELOPMENT.md (1)
47-49: Directory rename to doctor is consistent“doctor/” correctly reflects the new system health tooling. No further action.
AGENTS.md (1)
22-23: Command rename is consistentSwitched to
npm run doctoras the Doctor CLI. Looks consistent with the broader rename..vscode/settings.json (1)
39-43: Confirm serverSampling mapping key is recognized by VS Code ChatThe key
"XcodeBuildMCP/.vscode/mcp.json: XcodeBuildMCP-Dev"looks like a composed server/profile identifier. Please verify it matches what VS Code expects so sampling actually applies. Note: settings.json is JSONC, so the trailing comma is acceptable in VS Code, but ensure no external tooling parses it as strict JSON.If needed, we can add a README snippet documenting the expected key format for future contributors.
.github/workflows/claude.yml (1)
53-54: Doctor script and references verified
- package.json defines the “doctor” script (
node build/doctor-cli.js)- No legacy
diagnostic-cli,/diagnostics/orxcodebuildmcp-diagnosticentries remain- Remaining
xcodebuildmcp://environmentURIs are intentional for the environment resource used by the doctor toolAll checks pass; no further changes needed.
docs/TESTING.md (1)
582-586: Rename to “doctor” is consistent across testing docsExamples, dependency graphs, and command snippets now uniformly use “doctor”. Looks correct and aligns with CLI/tooling changes.
Also applies to: 963-965, 1039-1040, 1111-1113, 153-157
docs/MANUAL_TESTING.md (1)
86-91: Manual testing guidance correctly updated to “doctor”All relevant examples and sequences reflect the doctor tool. Changes are consistent and accurate.
Also applies to: 153-157, 470-473, 546-548, 618-621
src/utils/sentry.ts (1)
12-12: Comment wording update looks goodRenaming the comment to “system info” is consistent with the doctor terminology.
tsup.config.ts (1)
8-9: Rename to doctor-cli and chmod adjustments look correctEntry and post-build chmod target the new doctor CLI artifact as expected.
Also applies to: 33-35
package.json (1)
9-11: Bin/script switch to doctor is consistent and correctBin maps to build/doctor-cli.js and script “doctor” runs the same. Matches tsup changes.
Also applies to: 22-23
docs/RELOADEROO.md (4)
69-71: Doctor tool example aligns with renameThe example call via reloaderoo matches the new tool name.
212-213: Info command example tweak is fineMinor doc improvement; no behavioral change.
358-360: Updated server-info guidance reads clearlyAdjusted phrasing and example are consistent.
389-391: “View system information” heading update is consistentNo issues.
README.md (3)
36-36: TOC entry rename to Doctor Tool is consistentLooks good.
102-110: Resource name/URI likely needs updating (Environment → Doctor)Per PR objectives, the environment resource was renamed to doctor and its URI changed to xcodebuildmcp://doctor. This section still references xcodebuildmcp://environment.
If the resource is now “Doctor” (or both are supported), adjust this list accordingly. I can open a doc patch to reflect the new URI or show both if you maintain a legacy alias.
295-317: Doctor Tool section and CLI example are correctThe doctor guidance and npx example match the new CLI mapping.
Also applies to: 304-306
docs/ARCHITECTURE.md (1)
23-27: Architecture docs reflect the doctor rename accuratelyEntries, distribution structure, and executable names align with the CLI rename.
Also applies to: 100-106, 372-385, 390-392
docs/CONTRIBUTING.md (1)
198-207: Doctor tool guidance is clear and consistentGood replacement for the prior diagnostic section.
src/mcp/tools/doctor/lib/doctor.deps.ts (2)
95-151: Overall dependency wiring and executor usage look solid
- External commands are routed via CommandExecutor.
- Good error handling and typed result unions.
- Runtime mode split (dynamic/static) aligns with core registries.
Also applies to: 241-267, 270-277
280-282: Remove the no-op default export from doctor.deps.tsWe confirmed there are no default imports or requires of
doctor.depsin the codebase, so it’s safe to drop the meaninglessexport default {} as const;.• File to update:
- src/mcp/tools/doctor/lib/doctor.deps.ts
Proposed change:
-export default {} as const;src/mcp/tools/discovery/__tests__/discover_tools.test.ts (1)
117-121: Update aligns with doctor renameExpectation now omits “diagnostics”. Matches prompt/description changes in discover_tools.ts.
src/mcp/tools/discovery/discover_tools.ts (1)
176-176: Prompt correctly introduces the doctor workflowIncluding “System/environment diagnostics or validation: choose "doctor"” is correct and consistent with the new workflow.
docs/TOOLS.md (2)
191-194: Rename to “System Doctor” looks goodSection header and tool entry updated appropriately.
214-214: Update Resource URI in docs/TOOLS.mdThe table still shows the old
xcodebuildmcp://environmentURI, but according to the PR notes it should now bexcodebuildmcp://doctor. Please update this entry and, if the code still recognizes the legacyenvironmentURI, retain it as a deprecated alias.• File: docs/TOOLS.md
• Section: “MCP Resources” table (around line 214)Proposed diff:
-| `xcodebuildmcp://environment` | System health checks and environment validation | `doctor` | +| `xcodebuildmcp://doctor` | System health checks and environment validation | `doctor` | +| `xcodebuildmcp://environment` (deprecated) | Back-compat alias for system health checks and environment validation | `doctor` |Please confirm whether
xcodebuildmcp://environmentremains supported in code. If not, drop the alias row.src/mcp/tools/doctor/__tests__/index.test.ts (1)
16-38: Updated workflow name/description/capabilities assertions are correctMatches src/mcp/tools/doctor/index.ts (System Doctor; capabilities include doctor and server-diagnostics).
src/doctor-cli.ts (1)
1-49: LGTM! Well-structured CLI implementation.This standalone CLI script is well-designed with proper error handling, appropriate console usage, and clean separation of concerns by delegating to
doctorLogic. The shebang, exit codes, and unhandled rejection handling are all correctly implemented.src/mcp/tools/doctor/index.ts (1)
1-13: LGTM! Clean workflow definition.The workflow export is well-structured with appropriate metadata for the System Doctor tool. The capabilities and platform specifications accurately reflect the tool's purpose.
src/mcp/resources/__tests__/environment.test.ts (1)
55-80: LGTM! Proper environment variable management in test.The test correctly manages the
XCODEBUILDMCP_DYNAMIC_TOOLSenvironment variable with proper cleanup in the finally block, ensuring no test pollution.src/mcp/resources/doctor.ts (2)
12-58: LGTM! Well-structured resource implementation with proper error handling.The
doctorResourceLogicfunction follows good practices by:
- Separating testable logic from the MCP handler
- Comprehensive error handling for both
result.isErrorand exceptions- Proper logging for debugging
- Graceful fallback messages
60-69: LGTM! Clean MCP resource definition.The resource export follows the expected MCP resource structure with appropriate URI, description, and handler delegation.
src/mcp/tools/doctor/__tests__/doctor.test.ts (3)
11-122: Excellent test setup with proper dependency injection.The
createDepshelper function provides comprehensive mock dependencies with sensible defaults and override capability. This follows the coding guidelines by using dependency injection instead ofvi.mock().
142-155: LGTM! Proper Zod schema validation testing.The schema tests correctly validate both valid and invalid inputs, ensuring the optional boolean field works as expected.
158-301: LGTM! Comprehensive behavior testing with deterministic mocks.The test cases cover various failure scenarios (plugin loading, Xcode, xcodemake, axe tools) using deterministic dependency injection. Each test properly validates the returned structure and content type.
src/mcp/tools/doctor/doctor.ts (4)
29-54: LGTM! Excellent dependency injection and system information gathering.The
runDoctorfunction uses proper dependency injection and comprehensively gathers system information from all the providers. The logging control with environment variable restoration is well implemented.
103-139: LGTM! Nice ASCII art colorization implementation.The colorization logic with ANSI color codes and character-specific coloring is well implemented and adds a nice visual touch to the CLI output.
154-234: LGTM! Comprehensive and well-formatted output generation.The output formatting creates a thorough system report with proper sectioning, emoji indicators, and handling of various data states. The conditional troubleshooting tips based on environment variables is a nice touch.
253-278: LGTM! Clean separation of concerns with MCP wrapper.The separation between
doctorLogicfor general use anddoctorMcpHandlerfor MCP-specific behavior (disabling ASCII logo) is well designed. The MCP plugin export follows the expected structure.
| ██╗ ██╗ ██████╗ ██████╗ ██████╗ ███████╗██████╗ ██╗ ██╗██╗██╗ ██████╗ ███╗ ███╗ ██████╗██████╗ | ||
| ╚██╗██╔╝██╔════╝██╔═══██╗██╔══██╗██╔════╝██╔══██╗██║ ██║██║██║ ██╔══██╗████╗ ████║██╔════╝██╔══██╗ | ||
| ╚███╔╝ ██║ ██║ ██║██║ ██║█████╗ ██████╔╝██║ ██║██║██║ ██║ ██║██╔████╔██║██║ ██████╔╝ | ||
| ██╔██╗ ██║ ██║ ██║██║ ██║██╔══╝ ██╔══██╗██║ ██║██║██║ ██║ ██║██║╚██╔╝██║██║ ██╔═══╝ |
There was a problem hiding this comment.
Fix ASCII art character encoding issue.
There appears to be a character encoding issue in the ASCII art on this line.
Apply this diff to fix the ASCII art:
- ██╔██╗ ██║ ██║ ██║██║ ██║██╔══╝ ██╔══██╗██║ ██║██║██║ ██║ ██║██║╚██╔╝██║██║ ██╔═══╝
+ ██╔██╗ ██║ ██║ ██║██║ ██║██╔══╝ ██╔══██╗██║ ██║██║██║ ██║ ██║██║╚██╔╝██║██║ ██████╔╝The ending should match the pattern of other lines in the ASCII art.
📝 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.
| ██╔██╗ ██║ ██║ ██║██║ ██║██╔══╝ ██╔══██╗██║ ██║██║██║ ██║ ██║██║╚██╔╝██║██║ ██╔═══╝ | |
| ██╔██╗ ██║ ██║ ██║██║ ██║██╔══╝ ██╔══██╗██║ ██║██║██║ ██║ ██║██║╚██╔╝██║██║ ██████╔╝ |
🤖 Prompt for AI Agents
In src/mcp/tools/doctor/doctor.ts around line 86, the ASCII art contains a
character encoding glitch; replace the malformed character(s) with standard
ASCII so the line matches the pattern and alignment of the other art lines (use
plain pipe, underscore, slash, backslash, spaces and block characters as used
elsewhere) and ensure the line ending matches the same columns and separators as
the surrounding lines so the overall banner aligns.
| const env: EnvironmentInfoProvider = { | ||
| getEnvironmentVariables() { | ||
| const relevantVars = [ | ||
| 'INCREMENTAL_BUILDS_ENABLED', | ||
| 'PATH', | ||
| 'DEVELOPER_DIR', | ||
| 'HOME', | ||
| 'USER', | ||
| 'TMPDIR', | ||
| 'NODE_ENV', | ||
| 'SENTRY_DISABLED', | ||
| ]; | ||
|
|
||
| const envVars: Record<string, string | undefined> = {}; | ||
| for (const varName of relevantVars) { | ||
| envVars[varName] = process.env[varName]; | ||
| } | ||
|
|
||
| Object.keys(process.env).forEach((key) => { | ||
| if (key.startsWith('XCODEBUILDMCP_')) { | ||
| envVars[key] = process.env[key]; | ||
| } | ||
| }); | ||
|
|
||
| return envVars; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Redact potentially sensitive env vars; current implementation may leak secrets
Collecting PATH, HOME, USER and all XCODEBUILDMCP_* envs verbatim can expose tokens/keys (e.g., API keys) in doctor output/logs. Redact values for keys that look sensitive by default, with an opt-in to show raw values.
[security]
Apply this diff to add basic redaction:
getEnvironmentVariables() {
- const relevantVars = [
+ const relevantVars = [
'INCREMENTAL_BUILDS_ENABLED',
'PATH',
'DEVELOPER_DIR',
'HOME',
'USER',
'TMPDIR',
'NODE_ENV',
'SENTRY_DISABLED',
];
- const envVars: Record<string, string | undefined> = {};
+ const envVars: Record<string, string | undefined> = {};
+ const looksSensitive = (key: string) =>
+ /TOKEN|SECRET|PASSWORD|API[-_]?KEY|DSN|AUTH|PRIVATE|CREDENTIAL/i.test(key);
+ const redact = (key: string, value?: string) =>
+ value && looksSensitive(key) ? '*** redacted ***' : value;
for (const varName of relevantVars) {
- envVars[varName] = process.env[varName];
+ envVars[varName] = redact(varName, process.env[varName]);
}
Object.keys(process.env).forEach((key) => {
if (key.startsWith('XCODEBUILDMCP_')) {
- envVars[key] = process.env[key];
+ envVars[key] = redact(key, process.env[key]);
}
});
return envVars;
},📝 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.
| const env: EnvironmentInfoProvider = { | |
| getEnvironmentVariables() { | |
| const relevantVars = [ | |
| 'INCREMENTAL_BUILDS_ENABLED', | |
| 'PATH', | |
| 'DEVELOPER_DIR', | |
| 'HOME', | |
| 'USER', | |
| 'TMPDIR', | |
| 'NODE_ENV', | |
| 'SENTRY_DISABLED', | |
| ]; | |
| const envVars: Record<string, string | undefined> = {}; | |
| for (const varName of relevantVars) { | |
| envVars[varName] = process.env[varName]; | |
| } | |
| Object.keys(process.env).forEach((key) => { | |
| if (key.startsWith('XCODEBUILDMCP_')) { | |
| envVars[key] = process.env[key]; | |
| } | |
| }); | |
| return envVars; | |
| }, | |
| const env: EnvironmentInfoProvider = { | |
| getEnvironmentVariables() { | |
| const relevantVars = [ | |
| 'INCREMENTAL_BUILDS_ENABLED', | |
| 'PATH', | |
| 'DEVELOPER_DIR', | |
| 'HOME', | |
| 'USER', | |
| 'TMPDIR', | |
| 'NODE_ENV', | |
| 'SENTRY_DISABLED', | |
| ]; | |
| const envVars: Record<string, string | undefined> = {}; | |
| const looksSensitive = (key: string) => | |
| /TOKEN|SECRET|PASSWORD|API[-_]?KEY|DSN|AUTH|PRIVATE|CREDENTIAL/i.test(key); | |
| const redact = (key: string, value?: string) => | |
| value && looksSensitive(key) ? '*** redacted ***' : value; | |
| for (const varName of relevantVars) { | |
| envVars[varName] = redact(varName, process.env[varName]); | |
| } | |
| Object.keys(process.env).forEach((key) => { | |
| if (key.startsWith('XCODEBUILDMCP_')) { | |
| envVars[key] = redact(key, process.env[key]); | |
| } | |
| }); | |
| return envVars; | |
| }, | |
| // …other properties… | |
| }; |
🤖 Prompt for AI Agents
In src/mcp/tools/doctor/lib/doctor.deps.ts around lines 158 to 183, the current
getEnvironmentVariables returns sensitive environment values (PATH, HOME, USER
and all XCODEBUILDMCP_* keys) verbatim; change it to redact likely-secret values
by default and allow opt-in to reveal raw values. Implement a redaction rule
that treats keys matching
/(?:KEY|TOKEN|SECRET|PASSWORD|PASS|AWS|GCP|PRIVATE|ACCESS)/i or keys containing
"KEY" or "TOKEN" as sensitive and replace their values with "[REDACTED]" when
building envVars; still include PATH, HOME, USER and XCODEBUILDMCP_* keys but
apply the same redaction. Add an opt-in env var (e.g., SHOW_SENSITIVE_ENV) that,
when set truthy, returns raw values instead of redacted ones. Ensure types
remain Record<string, string | undefined> and keep iteration/order behavior
unchanged.
| getNodeInfo() { | ||
| return { | ||
| version: process.version, | ||
| execPath: process.execPath, | ||
| pid: process.pid.toString(), | ||
| ppid: process.ppid.toString(), | ||
| platform: process.platform, | ||
| arch: process.arch, | ||
| cwd: process.cwd(), | ||
| argv: process.argv.join(' '), | ||
| }; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid leaking CLI args; mask potential secrets in argv
Full argv can include tokens (e.g., --token=...). Consider masking common secret-looking patterns.
[security]
getNodeInfo() {
- return {
+ const redactArg = (s: string) =>
+ s.replace(/(--?\w*(token|secret|password|api[-_]?key|dsn|auth)\w*)(=|\s+)(\S+)/gi, '$1$3***');
+ return {
version: process.version,
execPath: process.execPath,
pid: process.pid.toString(),
ppid: process.ppid.toString(),
platform: process.platform,
arch: process.arch,
- cwd: process.cwd(),
- argv: process.argv.join(' '),
+ cwd: process.cwd(),
+ argv: process.argv.map(redactArg).join(' '),
};
},📝 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.
| getNodeInfo() { | |
| return { | |
| version: process.version, | |
| execPath: process.execPath, | |
| pid: process.pid.toString(), | |
| ppid: process.ppid.toString(), | |
| platform: process.platform, | |
| arch: process.arch, | |
| cwd: process.cwd(), | |
| argv: process.argv.join(' '), | |
| }; | |
| }, | |
| getNodeInfo() { | |
| const redactArg = (s: string) => | |
| s.replace(/(--?\w*(token|secret|password|api[-_]?key|dsn|auth)\w*)(=|\s+)(\S+)/gi, '$1$3***'); | |
| return { | |
| version: process.version, | |
| execPath: process.execPath, | |
| pid: process.pid.toString(), | |
| ppid: process.ppid.toString(), | |
| platform: process.platform, | |
| arch: process.arch, | |
| cwd: process.cwd(), | |
| argv: process.argv.map(redactArg).join(' '), | |
| }; | |
| }, |
|
Claude finished @cameroncooke's task —— View job Code Review Complete ✅Reviewed PR #92: Rename diagnostics tool to doctor Todo List:
Review SummaryThis is a comprehensive and well-executed renaming effort that successfully transforms the ✅ Strengths1. Complete Implementation Coverage
2. Excellent Code Quality
3. Strong Test Coverage
4. Architecture Preservation
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
.github/ISSUE_TEMPLATE/bug_report.yml (3)
24-24: Clarify fallback when an agent isn’t availableRecommend adding a brief fallback for users not using an agent (e.g., CLI command) to reduce back-and-forth.
Proposed tweak:
- description: Ask your agent "Run the XcodeBuildMCP `doctor` tool and return the output as markdown verbatim" and then copy paste it here. + description: Ask your agent "Run the XcodeBuildMCP `doctor` tool and return the output as markdown verbatim" and then copy paste it here. If you’re not using an agent, run the `doctor` CLI (e.g., `npm run doctor`) and paste the output.
26-37: Hint the fenced block language for readabilityUsing a language hint helps readability when reporters paste the output.
- ``` + ```md XcodeBuildMCP Doctor @@ - ``` + ```
55-55: Align example server versions across the templateThe placeholder “MCP Server Version” (1.2.2) doesn’t match the sample Doctor block’s “Server Version” (1.11.2). Aligning avoids confusion.
- placeholder: 1.2.2 + placeholder: 1.11.2src/mcp/resources/__tests__/doctor.test.ts (4)
6-6: Prevent cross-test env var races: run this suite sequentiallyThis file mutates process.env. To avoid cross-file interference under Vitest’s threaded workers, run the suite sequentially.
Apply this diff:
-describe('doctor resource', () => { +describe.sequential('doctor resource', () => {Alternative (if allowed by your testing rules): use vi.stubEnv instead of manual process.env mutation to ensure cleanup even on failure.
Also applies to: 55-79
28-28: Nit: update test name to reflect doctorRename “environment” → “doctor” for consistency with the new resource.
-it('should handle successful environment data retrieval', async () => { +it('should handle successful doctor data retrieval', async () => {
55-79: Env var handling is careful; consider using vi.stubEnv for isolationNice use of try/finally to restore the original value. If permitted by your rules, vi.stubEnv('XCODEBUILDMCP_DYNAMIC_TOOLS','true') and vi.unstubAllEnvs() provide stronger isolation and readability.
103-107: Broaden error assertions to cover resource-level fallbackIf the resource returns a fallback error message (without the header), this test may miss it. Add an additional assertion to accept the fallback format too.
const result = await doctorResourceLogic(mockExecutor); expect(result.contents).toHaveLength(1); - expect(result.contents[0].text).toContain('XcodeBuildMCP Doctor'); + expect(result.contents[0].text).toContain('XcodeBuildMCP Doctor'); + // Also allow the resource-level fallback error format + expect(result.contents[0].text).toMatch(/XcodeBuildMCP Doctor|Error retrieving doctor data:/);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/ISSUE_TEMPLATE/bug_report.yml(9 hunks)README.md(3 hunks)docs/TOOLS.md(2 hunks)src/mcp/resources/__tests__/doctor.test.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- docs/TOOLS.md
🧰 Additional context used
📓 Path-based instructions (2)
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.test.{ts,tsx,js,jsx}: Do not usevi.mock(),vi.fn(),vi.spyOn(), etc. in tests; Vitest mocking is completely banned.
Tests should import the core...Logicfunction from a tool file and pass in a mock executor (createMockExecutororcreateMockFileSystemExecutor) to simulate different outcomes.
Files:
src/mcp/resources/__tests__/doctor.test.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
All external interactions (like running commands or accessing the file system) must be handled through injectable executors (
CommandExecutor,FileSystemExecutor).
Files:
src/mcp/resources/__tests__/doctor.test.ts
🧠 Learnings (1)
📚 Learning: 2025-08-06T11:44:09.150Z
Learnt from: CR
PR: cameroncooke/XcodeBuildMCP#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-06T11:44:09.150Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Tests should import the core `...Logic` function from a tool file and pass in a mock executor (`createMockExecutor` or `createMockFileSystemExecutor`) to simulate different outcomes.
Applied to files:
src/mcp/resources/__tests__/doctor.test.ts
🧬 Code Graph Analysis (1)
src/mcp/resources/__tests__/doctor.test.ts (2)
src/mcp/resources/doctor.ts (1)
doctorResourceLogic(12-58)src/utils/command.ts (1)
createMockExecutor(267-305)
⏰ 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). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (6)
.github/ISSUE_TEMPLATE/bug_report.yml (2)
24-37: Doctor block looks good; prior trailing-space lint issue appears resolvedThe switch from diagnostics → doctor in the placeholder is correct, and the trailing spaces previously flagged by yamllint in this block appear fixed. No blocking issues here.
119-119: Confirmrender: shellIs Valid (Optional: Switch tobashfor Explicit Bash Highlighting)GitHub Issue Forms accept any language identifier from the Linguist list, and “Shell” (with alias
shell) is supported just as “bash” is. If your intent is to use generic shell highlighting, you can keeprender: shell. If you prefer to match the examples in GitHub’s docs and explicitly highlight Bash syntax, change it tobash.• Location:
- .github/ISSUE_TEMPLATE/bug_report.yml line 119
• Suggested change (optional):- render: shell + render: bashsrc/mcp/resources/__tests__/doctor.test.ts (4)
3-3: Import path updated to doctor — looks correctDefault + named imports align with the new resource. No concerns.
9-10: Resource metadata assertions match the rename — LGTMURI, description, mimeType, and handler checks are correct.
Also applies to: 13-15, 19-20, 23-24
34-43: Good use of injected executor and logic assertionsUsing createMockExecutor with doctorResourceLogic follows the no-mocking guideline and keeps tests fast/deterministic.
88-94: Feature status checks read well and cover key sectionsAsserting multiple sections keeps the contract tight. Looks good.
| it('should handle spawn errors by showing doctor info', async () => { | ||
| const mockExecutor = createMockExecutor(new Error('spawn xcrun ENOENT')); | ||
|
|
||
| const result = await environmentResourceLogic(mockExecutor); | ||
| const result = await doctorResourceLogic(mockExecutor); | ||
|
|
||
| expect(result.contents).toHaveLength(1); | ||
| expect(result.contents[0].text).toContain('# XcodeBuildMCP Diagnostic Report'); | ||
| expect(result.contents[0].text).toContain('XcodeBuildMCP Doctor'); | ||
| expect(result.contents[0].text).toContain('Error: spawn xcrun ENOENT'); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify error-path expectations match implementation
This test expects the full doctor header even when the executor throws (spawn ENOENT). If doctorLogic doesn’t catch and format the error, doctorResourceLogic will return “Error retrieving doctor data: …” without the header. Please confirm the behavior to avoid flakiness.
Run to inspect doctorLogic’s error handling:
🏁 Script executed:
#!/bin/bash
# Locate and inspect doctorLogic to confirm error handling and header emission.
fd doctor.ts | rg 'src/mcp/tools/doctor/doctor.ts' && rg -n -A 120 -B 5 $'export async function doctorLogic' src/mcp/tools/doctor/doctor.tsLength of output: 1265
🏁 Script executed:
#!/bin/bash
# Inspect doctorResourceLogic to see how errors are handled and header formatting
rg -n -A 50 -B 5 "export async function doctorResourceLogic" src/mcp/resources
# Inspect runDoctor implementation to confirm where and how errors are caught/formatted
rg -n -A 50 -B 5 "async function runDoctor" src/mcp/tools/doctor/doctor.tsLength of output: 5684
Update doctor.test.ts error-path assertions to match implementation
The doctorResourceLogic catch block returns only the formatted error message (e.g.
Error retrieving doctor data: spawn xcrun ENOENT) and does not include the “XcodeBuildMCP Doctor” header on errors. Please adjust the test accordingly:
• File: src/mcp/resources/tests/doctor.test.ts (lines 45–53)
• Remove or change the header assertion and assert the full error message:
Suggested diff:
- expect(result.contents).toHaveLength(1);
- expect(result.contents[0].text).toContain('XcodeBuildMCP Doctor');
- expect(result.contents[0].text).toContain('Error: spawn xcrun ENOENT');
+ expect(result.contents).toHaveLength(1);
+ expect(result.contents[0].text).toContain(
+ 'Error retrieving doctor data: spawn xcrun ENOENT'
+ );📝 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.
| it('should handle spawn errors by showing doctor info', async () => { | |
| const mockExecutor = createMockExecutor(new Error('spawn xcrun ENOENT')); | |
| const result = await environmentResourceLogic(mockExecutor); | |
| const result = await doctorResourceLogic(mockExecutor); | |
| expect(result.contents).toHaveLength(1); | |
| expect(result.contents[0].text).toContain('# XcodeBuildMCP Diagnostic Report'); | |
| expect(result.contents[0].text).toContain('XcodeBuildMCP Doctor'); | |
| expect(result.contents[0].text).toContain('Error: spawn xcrun ENOENT'); | |
| }); | |
| it('should handle spawn errors by showing doctor info', async () => { | |
| const mockExecutor = createMockExecutor(new Error('spawn xcrun ENOENT')); | |
| const result = await doctorResourceLogic(mockExecutor); | |
| expect(result.contents).toHaveLength(1); | |
| expect(result.contents[0].text).toContain( | |
| 'Error retrieving doctor data: spawn xcrun ENOENT' | |
| ); | |
| }); |
🤖 Prompt for AI Agents
In src/mcp/resources/__tests__/doctor.test.ts around lines 45 to 53, the test
currently asserts that the error-path response includes the "XcodeBuildMCP
Doctor" header, but the implementation's catch block only returns the formatted
error message (e.g. "Error retrieving doctor data: spawn xcrun ENOENT"); update
the test to remove the header assertion and instead assert that
result.contents[0].text contains the full error message string returned by the
implementation (e.g. "Error retrieving doctor data: spawn xcrun ENOENT"),
keeping the existing creation of the mock executor and other expectations.
1 participant
Summary
This PR renames the
diagnosticstool todoctorthroughout the codebase to better align with common CLI conventions and improve user experience.Changes Made
🔧 Core Tool Changes
src/mcp/tools/diagnostics/→src/mcp/tools/doctor/diagnostic.ts→doctor.tsdiagnostic-cli.ts→doctor-cli.ts📚 Resource Changes
environment.tsresource →doctor.tsxcodebuildmcp://environment→xcodebuildmcp://doctor🧪 Test Updates
diagnosticstodoctordirectory📖 Documentation Updates
doctorcommand examples⚙️ Configuration Changes
package.jsonscripts (npm run doctor)tsup.config.tsbuild configurationImpact
doctorinstead ofdiagnosticscommandbrew doctor)Files Changed
33 files modified with comprehensive renaming across:
Testing
npm run doctorThis change improves the user experience by adopting familiar CLI conventions while maintaining all existing functionality.
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores