|
| 1 | +# Bugbot Review Guide for XcodeBuildMCP |
| 2 | + |
| 3 | +## Project Snapshot |
| 4 | + |
| 5 | +XcodeBuildMCP is an MCP server exposing Xcode / Swift workflows as **tools** and **resources**. |
| 6 | +Stack: TypeScript · Node.js · plugin-based auto-discovery (`src/mcp/tools`, `src/mcp/resources`). |
| 7 | + |
| 8 | +For full details see [README.md](README.md) and [docs/ARCHITECTURE.md](docs/ARCHITECTURE.md). |
| 9 | + |
| 10 | +--- |
| 11 | + |
| 12 | +## 1. Security Checklist — Critical |
| 13 | + |
| 14 | +* No hard-coded secrets, tokens or DSNs. |
| 15 | +* All shell commands must flow through `CommandExecutor` with validated arguments (no direct `child_process` calls). |
| 16 | +* Paths must be sanitised via helpers in `src/utils/validation.ts`. |
| 17 | +* Sentry breadcrumbs / logs must **NOT** include user PII. |
| 18 | + |
| 19 | +--- |
| 20 | + |
| 21 | +## 2. Architecture Checklist — Critical |
| 22 | + |
| 23 | +| Rule | Quick diff heuristic | |
| 24 | +|------|----------------------| |
| 25 | +| Dependency injection only | New `child_process` \| `fs` import ⇒ **critical** | |
| 26 | +| Handler / Logic split | `handler` > 20 LOC or contains branching ⇒ **critical** | |
| 27 | +| Plugin auto-registration | Manual `registerTool(...)` / `registerResource(...)` ⇒ **critical** | |
| 28 | + |
| 29 | +Positive pattern skeleton: |
| 30 | + |
| 31 | +```ts |
| 32 | +// src/mcp/tools/foo-bar.ts |
| 33 | +export async function fooBarLogic( |
| 34 | + params: FooBarParams, |
| 35 | + exec: CommandExecutor = getDefaultCommandExecutor(), |
| 36 | + fs: FileSystemExecutor = getDefaultFileSystemExecutor(), |
| 37 | +) { |
| 38 | + // ... |
| 39 | +} |
| 40 | + |
| 41 | +export const handler = (p: FooBarParams) => fooBarLogic(p); |
| 42 | +``` |
| 43 | + |
| 44 | +--- |
| 45 | + |
| 46 | +## 3. Testing Checklist |
| 47 | + |
| 48 | +* **Ban on Vitest mocking** (`vi.mock`, `vi.fn`, `vi.spyOn`, `.mock*`) ⇒ critical. Use `createMockExecutor` / `createMockFileSystemExecutor`. |
| 49 | +* Each tool must have tests covering happy-path **and** at least one failure path. |
| 50 | +* Avoid the `any` type unless justified with an inline comment. |
| 51 | + |
| 52 | +--- |
| 53 | + |
| 54 | +## 4. Documentation Checklist |
| 55 | + |
| 56 | +* `docs/TOOLS.md` must exactly mirror the structure of `src/mcp/tools/**` (exclude `__tests__` and `*-shared`). |
| 57 | + *Diff heuristic*: if a PR adds/removes a tool but does **not** change `docs/TOOLS.md` ⇒ **warning**. |
| 58 | +* Update public docs when CLI parameters or tool names change. |
| 59 | + |
| 60 | +--- |
| 61 | + |
| 62 | +## 5. Common Anti-Patterns (and fixes) |
| 63 | + |
| 64 | +| Anti-pattern | Preferred approach | |
| 65 | +|--------------|--------------------| |
| 66 | +| Complex logic in `handler` | Move to `*Logic` function | |
| 67 | +| Re-implementing logging | Use `src/utils/logger.ts` | |
| 68 | +| Direct `fs` / `child_process` usage | Inject `FileSystemExecutor` / `CommandExecutor` | |
| 69 | +| Chained re-exports | Export directly from source | |
| 70 | + |
| 71 | +--- |
| 72 | + |
| 73 | +### How Bugbot Can Verify Rules |
| 74 | + |
| 75 | +1. **Mocking violations**: search `*.test.ts` for `vi.` → critical. |
| 76 | +2. **DI compliance**: search for direct `child_process` / `fs` imports outside executors. |
| 77 | +3. **Docs accuracy**: compare `docs/TOOLS.md` against `src/mcp/tools/**`. |
| 78 | +4. **Style**: ensure ESLint and Prettier pass (`npm run lint`, `npm run format:check`). |
| 79 | + |
| 80 | +--- |
| 81 | + |
| 82 | +Happy reviewing 🚀 |
0 commit comments