PR Review

PR Review Skill

Reviews pull requests for the Confluent MCP (Model Context Protocol) server, focusing on project-specific patterns and the failure modes most likely to slip past TypeScript and lint.

Two Review Modes

The mode is selected by the invocation context, not by the user. If the user supplies a PR number/URL or asks about someone else's PR, run Formal Review Mode. Otherwise (no PR number, working from a local branch, phrases like "self-review" or "check my PR") run Self-Review Mode. When ambiguous, ask which mode to use.

Self-Review Mode (for PR authors)

Use when: the author wants to check their own changes before sharing with the team. Typically used on a draft PR or against local changes before pushing.

Goals:

• Catch issues early, before formal review
• Verify MCP tool wiring is complete (enum + handler + registry + predicate)
• Ensure new external I/O is stubbable
• Confirm OpenAPI spec and generated types stay in sync

Formal Review Mode (for reviewers)

Use when: a reviewer needs to evaluate a PR from another team member.

Goals:

• Quickly understand the scope and purpose of changes
• Identify potential issues or concerns
• Provide constructive feedback grounded in MCP/Confluent conventions
• Verify the PR template checklist is honored

Review Process

Step 1: Gather Information

For local changes (self-review):

# files changed since divergence from main
git diff main --name-only

# overview and full diff
git diff main --stat
git diff main

For GitHub PRs:

# PR metadata
gh pr view <PR_NUMBER> --json number,title,body,author,baseRefName,headRefName,additions,deletions,changedFiles,state,reviewDecision

# if no PR number is given, try the current branch
gh pr view --json number,title,body,author,baseRefName,headRefName,additions,deletions,changedFiles,state,reviewDecision

# diff
gh pr diff <PR_NUMBER>

# existing reviews and inline comments
gh pr view <PR_NUMBER> --json reviews,comments

# referenced issues
gh issue view <ISSUE_NUMBER> --json body,comments

Step 2: Filter Files for Review

SKIP these paths entirely (auto-generated or vendored):

• dist/** (compiled output)
• node_modules/**
• coverage/**
• src/confluent/openapi-schema.d.ts (generated by pnpm run generate:openapi-types)
• pnpm-lock.yaml (review only top-level dependency changes via package.json)
• TEST-result.xml

DO review carefully (small file, big blast radius):

• openapi.json (paired with the regenerated .d.ts)
• package.json (new deps, scripts, engine bumps)
• config.example.yaml, .env.example, .env.integration.example (user-facing configuration)
• vitest.config.ts, tsconfig.json, tsconfig.build.json, eslint.config.mjs
• .semaphore/** (CI configuration)
• Dockerfile, docker-compose.yml

Step 3: Categorize the Changes

Step 4: Check Critical Requirements

IMPORTANT: only review lines that were actually changed in the PR diff. Context lines from the diff are for understanding, not for review. Do not flag pre-existing issues in unchanged code.

1. Tool Wiring Completeness (MANDATORY for new tools)

A new MCP tool needs all four of:

• Entry in ToolName enum (src/confluent/tools/tool-name.ts)
• Handler class extending BaseToolHandler (or a domain subclass like FlinkToolHandler)
• enabledConnectionIds(runtime) using a predicate from connection-predicates.ts
• Registration in ToolHandlerRegistry.handlers (src/confluent/tools/tool-registry.ts)

Red flags:

• New handler class but no tool-registry.ts change → tool will not be loaded
• New enum entry but no handler → runtime error when iterating enabled tools
• Wrong predicate (e.g., hasKafka for a Schema Registry tool) → tool enables on the wrong connections and silently no-ops or errors

The canonical wiring lives in .claude/rules/tool-handlers.md, which auto-loads when files under src/confluent/tools/**/*.ts are touched.

2. OpenAPI / Generated Types Coupling

If openapi.json changed:

• src/confluent/openapi-schema.d.ts is regenerated and committed in the same PR
• The diff in the .d.ts reflects the spec change (no stale paths or schemas)

If openapi-schema.d.ts changed without openapi.json changing, the file was hand-edited - flag it. The generator is pnpm run generate:openapi-types; the file should never be edited manually.

3. ESM Stubbability via node-deps.ts

ESM named imports are read-only from outside the defining module, so vi.spyOn cannot intercept direct named imports. This project's workaround is src/confluent/node-deps.ts: external I/O (filesystem, env, network not via openapi-fetch/Kafka clients, third-party constructors) routes through that namespace, and tests spy on the wrapper.

Red flags in non-test code:

// BAD: direct named import at use site → not stubbable
import { readFile } from "node:fs/promises";
const config = await readFile(path, "utf8");

// GOOD: route through node-deps for stubbability
import { nodeDeps } from "@src/confluent/node-deps.js";
const config = await nodeDeps.readFile(path, "utf8");

Red flag in tests: vi.mock(...) calls. The project does not use vi.mock; wrap the dependency in node-deps.ts and use vi.spyOn(nodeDeps, "readFile") instead. The /vitest skill confirms the patterns.

4. Type Safety

• No new explicit any (the project disables noImplicitAny for OpenAPI types, but explicit any at use sites is still a code smell)
• Tool input schemas are Zod schemas, not loose object types
• REST calls use openapi-fetch with typed paths from the generated schema, not raw fetch
• Imports use .js extensions (ESM requirement) and @src/* for internal modules
• No @ts-ignore / @ts-expect-error without a comment explaining why

5. Single Responsibility

• Handlers do one tool's worth of work; cross-domain logic moves to a shared helper
• No "god clients" - BaseClientManager owns REST and Schema Registry; DirectClientManager adds Kafka admin/producer/consumer. New client kinds extend, not inflate, these.

Step 5: Check Project-Specific Patterns

Predicate-Based Tool Gating

• enabledConnectionIds(runtime) uses an existing predicate where one fits
• If a brand-new predicate is introduced, it composes existing service-block checks rather than re-walking runtime.connections ad hoc
• Domain base classes (e.g., FlinkToolHandler) implement enabledConnectionIds once for the whole domain; per-handler overrides should be rare and well-justified

Configuration Surface (two paths must stay in sync)

MCPServerConfiguration is produced by either loadConfigFromYaml() (-c <path>) or buildConfigFromEnvAndCli() (legacy env+CLI). Both produce the same Zod-validated shape.

• New configuration fields are added to the Zod schema in src/config/models.ts AND honored by buildConfigFromEnvAndCli (or explicitly excluded from the legacy path with a note)
• config.example.yaml and .env.example are updated when user-facing config changes
• ${VAR} interpolation continues to work for any new YAML fields

Transport Security

• HTTP/SSE handlers preserve API-key auth and DNS rebinding protection
• No new endpoint added without authentication unless explicitly intended (and called out in the PR description)
• Secrets never go through Pino at info level or above; sensitive fields are redacted (Pino's redact option, configured in src/logger.ts)

Logger Usage

• Uses the project logger from src/logger.ts (Pino), not console.log / console.error
• Errors use logger.error({ err }) so Pino's serializer captures stack traces
• No log lines that include credentials, tokens, or full request bodies that may contain secrets