pr-review

name: pr-review description: Reviews pull requests for the Confluent MCP server. Use when reviewing PRs, doing self-review before sharing with the team, or when the user mentions "review PR", "help with PR", "review changes", "self-review", "review local changes", or "check my PR". Focuses on MCP tool wiring, OpenAPI/type coupling, ESM stubbability, transport security, and project-specific patterns. allowed-tools: - Read - Bash - Grep - Glob - Task

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 npm run generate:openapi-types)
• package-lock.json (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 npm 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

Testing

• Unit tests follow .claude/rules/unit-tests.md (assertion style, stubbing patterns, handler test structure, fake timers)
• Integration tests follow .claude/rules/integration-tests.md: colocated *.integration.test.ts, tags on outer describe, credential gating via early-return (NOT describe.skipIf, which still runs nested hooks in vitest 4)
• Class-instance mocks use createMockInstance(Class) from @tests/stubs/index.js
• No .only left in test files
• No vi.mock (the project pattern is node-deps.ts + vi.spyOn)

Use the /vitest skill to verify spy/mock APIs against current Vitest docs.

Step 6: Check PR Hygiene

• PR description follows the template: Summary of Changes, manual testing instructions (transports exercised, sample inputs/outputs, env vars), additional context
• Tests checkbox is honored: new behavior has new tests; updated behavior has updated tests
• CHANGELOG entry added when the change is user-facing (new tool, new config field, breaking change, security fix). Internal refactors do not need an entry.
• No unrelated changes bundled in (formatting passes, dependency bumps, etc., should ride their own PR unless they directly support the change)
• No secrets in the diff: .env, .env.integration, real API keys, real cluster IDs

What NOT to Flag

Style Preferences (avoid nitpicking)

• Import statements: import type { } vs import { } - both valid
• Formatting issues - Prettier and ESLint enforce these via the pre-commit hook
• Auto-removed unused imports during a refactor - handled by eslint-plugin-unused-imports
• Trailing-comma, single-quote, semi-colon preferences - Prettier owns these
• Minor naming preferences when the existing name is reasonable

Comment Preservation

• Never suggest deleting existing comments unless they are now actively misleading
• Comments explain "why" not "what"; they may look redundant but provide context the reviewer may not have
• When code is updated, preserve or update the comments rather than removing them

Output Format

For Self-Review

## Self-Review Summary

### Changes Overview

[Brief summary of what changed]

### Critical Requirements Checklist

- [ ] Tool Wiring (enum + handler + predicate + registry): [status, location of any gap]
- [ ] OpenAPI / Types Coupling: [status]
- [ ] ESM Stubbability (node-deps): [status]
- [ ] Type Safety: [status]
- [ ] Transport Security: [status, if applicable]

### Issues to Address Before PR

1. [High-priority issue with file:line]
2. [Medium-priority issue with file:line]

### Suggestions (Optional)

- [Nice-to-have improvements]

### Ready for Review?

[Yes / Not yet, with reasoning]

For Formal Review

## PR Review: #{number} - {title}

**Author:** {author}
**Branch:** {headRefName} → {baseRefName}
**Changes:** +{additions} / -{deletions} across {changedFiles} files

### Summary

[2-3 sentence summary of what the PR does and why]

### Changed Components

- [Categorized list of changed files, excluding auto-generated]

### Findings

#### Issues (Must Fix)

- [ ] **[category]**: [description] - `file:line`

#### Suggestions (Consider)

- [ ] **[category]**: [description] - `file:line`

#### Positive Observations

- [Good patterns, thorough tests, well-written code]

### Test Coverage Assessment

- **New tests added:** [Yes/No, list test files]
- **Coverage gaps:** [Untested paths or edge cases]
- **Unit vs integration balance:** [Assessment]

### Configuration & Security Notes

[Any concerns about transport security, secret handling, config surface, or CCloud auth]

### Recommendation

**[APPROVE / REQUEST CHANGES / NEEDS DISCUSSION]**

[Brief rationale]

Review Categories

Use these labels in findings:

Common Issues to Watch For

Tool Wiring Gaps

• Handler class added but never imported into tool-registry.ts
• New enum entry never instantiated → unused enum value
• enabledConnectionIds returns [] unconditionally → tool never enables

OpenAPI Drift

• openapi.json updated, openapi-schema.d.ts not regenerated → callers still see the old type
• openapi-schema.d.ts hand-edited because regeneration "broke things" - fix the spec instead

Test-Time Surprises

• vi.mock(...) introduced as a shortcut → maintenance landmine; use node-deps.ts instead
• describe.skipIf(!hasCreds) in integration tests → nested hooks still run on vitest 4
• New external I/O imported directly at use site → tests cannot stub it without restructuring

Configuration Drift

• New field added to YAML schema but not to buildConfigFromEnvAndCli - env-var users get a silently incomplete config
• .env.example / config.example.yaml not updated → onboarding breakage

Security

• API key or PEM smuggled into a fixture or .env.integration example
• Pino log line that JSON-stringifies an object containing Authorization headers
• New transport endpoint without auth or with auth bypassed under NODE_ENV=test

Performance

• await in a tight loop where Promise.all would do
• Unbounded list endpoints (no pagination, no limit) returning the full Confluent Cloud surface to a model context

Tips

• Start with the PR description and the PR template checklist
• Look at the test changes first to understand the intended behavior
• For new tools, trace ToolName enum → handler → registry → predicate; if any link is missing, the tool is dead code
• Use Task with the Explore agent for deeper codebase context (for example, "find all callers of nodeDeps.readFile to estimate the blast radius of a signature change")
• Use companion project skills to ground review in current docs:
  • /mcp-docs for MCP protocol questions (tool annotations, resource shapes, transport spec)
  • /vitest for spy/mock APIs and Vitest 4 behavior
• When the diff touches src/confluent/tools/**, **/*.test.ts, or tests/**, the matching .claude/rules/ files (tool-handlers.md, unit-tests.md, integration-tests.md) auto-load with the canonical conventions - consult them rather than re-deriving the rules from the diff.
• When suggesting a simplification, verify it with the relevant doc skill before posting - saves a back-and-forth when the "simpler" alternative does not actually exist on the current SDK version