---
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):**

```bash
# 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:**

```bash
# 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

| Category               | File patterns                                          | What to check                                                  |
| ---------------------- | ------------------------------------------------------ | -------------------------------------------------------------- |
| Tool handlers          | `src/confluent/tools/handlers/<domain>/**`             | Enum entry, registry entry, `enabledConnectionIds`, Zod schema |
| Tool registry          | `src/confluent/tools/tool-registry.ts`                 | New handler imported and added to `handlers` map               |
| Tool name enum         | `src/confluent/tools/tool-name.ts`                     | New enum member with stable string value                       |
| Connection predicates  | `src/confluent/tools/connection-predicates.ts`         | New predicate composes existing service-block checks           |
| Domain base classes    | `src/confluent/tools/handlers/<domain>/*-tool-handler` | Domain-wide gating still correct after edits                   |
| Client managers        | `src/confluent/{base-,direct-,}client-manager.ts`      | 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 `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:

```ts
// 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

```markdown
## 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

```markdown
## 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:

| Category       | Description                                                           |
| -------------- | --------------------------------------------------------------------- |
| `tool-wiring`  | Missing enum / handler / registry / predicate hookup                  |
| `openapi`      | `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