// 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.
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.
Client lifecycle, single source of truth per connection
Configuration
src/config/**, src/env-schema.ts
Zod schema, YAML interpolation, both config paths covered
Transports
src/mcp/transports/**
API-key auth, DNS rebinding protection, port handling
OpenAPI surface
openapi.json
Regenerated .d.ts is committed in the same PR
Unit tests
**/*.test.ts (excluding *.integration.test.ts)
Vitest patterns, node-deps.ts indirection, no vi.mock
Integration tests
**/*.integration.test.ts, tests/harness/**
Credential gating via early-return, tags, startServer
CI / release
.semaphore/**, scripts/**, Dockerfile
No skipped checks, no smuggled secrets
Docs
README.md, docs/**, CHANGELOG.md, telemetry.md
Accuracy, MCP-tool inventory, user-facing wording
Project rules / skills
.claude/rules/**, .claude/skills/**
Frontmatter, path globs, trigger phrases
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 stubbableimport { readFile } from"node:fs/promises";
const config = awaitreadFile(path, "utf8");
// GOOD: route through node-deps for stubbabilityimport { 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 PR1. [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]
openapi.json and .d.ts out of sync, or hand-edited generated file
stubbability
New external I/O bypasses node-deps.ts; vi.mock introduced
types
Explicit any, missing Zod schema, raw fetch over openapi-fetch
config
YAML and env-var paths drift; missing example-file update
transport
Auth or DNS-rebinding regression; unauthenticated endpoint
secrets
Credentials in diff, logs, or fixtures
testing
Missing tests, wrong mocking pattern, .only left in
logging
console.* in src code, secret leaks in log lines
docs
CHANGELOG missing for user-facing change; stale README example
style
Naming, conventions where Prettier/ESLint do not already enforce
performance
Synchronous I/O in hot paths, unbounded fetches
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
