/review-prs

name: review-prs
description: >-
  Review a GitHub pull request in the googleapis/mcp-toolbox repo against the
  team's reviewer checklist: PR title/description conventions, linked issue,
  logic errors and unhandled edge cases, breaking changes, test coverage, docs
  updates, security (input handling), and new dependencies. Use whenever a
  maintainer asks you to review, look over, "take a look at", or check whether
  something is ready to merge in mcp-toolbox, e.g. "review #3703", "can you look
  at this PR", "is this good to merge", or when they paste an mcp-toolbox PR link.
  PROPOSE-ONLY: delivers the review in chat for the maintainer to post; never
  approves, requests changes, comments, labels, or merges on its own.

Review PRs (mcp-toolbox)

A review here is a proposal the maintainer edits and posts, not a rubber stamp. The value is a fast, grounded read of the diff against the team's conventions.

Goal

Given a PR number or link, deliver a review the maintainer can post in seconds: a suggested verdict (approve / request changes / comment), the findings that back it grouped by severity so the important things aren't buried, and a paste-ready summary comment.

Prerequisites

• `gh` authenticated for `googleapis/mcp-toolbox`, plus the PR number(s). A GitHub MCP

server substitutes for `gh` if it isn't available: the `gh` commands below map to its read/list tools.

Workflow

Step 1: Read the source of truth

Read these live, not from memory. All three are symlinks to the repo-root files, so they track `main`; cite them by their root names.

• [references/maintainer-playbook.md](references/maintainer-playbook.md): Reviewer's Checklist,

SLO/release context, `release candidate` labeling.

• [references/CONTRIBUTING.md](references/CONTRIBUTING.md): title/scope format (Conventional

Commits, with the `type` table), keep-PRs-small, link-an-issue. Cite for title, description, and process findings.

• [references/DEVELOPER.md](references/DEVELOPER.md): tool/source naming, error taxonomy, the

patterns for adding a source/tool/integration test, CI-enforced docs structure, local test/lint commands. Cite for code, test, and docs findings. Prefer it over `GEMINI.md` (`CLAUDE.md`/`AGENTS.md` symlink to it), which only summarizes.

Step 2: Fetch the PR, its diff, and its checks

gh pr view <n> --repo googleapis/mcp-toolbox --json number,title,body,author,labels,files,additions,deletions,commits,baseRefName,headRefName,state,isDraft,reviewDecision
gh pr diff <n> --repo googleapis/mcp-toolbox
gh pr checks <n> --repo googleapis/mcp-toolbox

Step 3: Triage before reviewing

Three shapes end the review early or change its bar:

• **Auto-generated (`renovate`, `release-please`):** the only question is whether checks are

green. If so, propose merge and stop.

• **Draft (`isDraft`):** review lightly and say so; the author isn't asking for a final pass.
• **Non-code / policy** (third-party badge, backlink, promotional README line, often a drive-by

contributor): acceptance is a maintainer policy call, not a code question. Say that plainly instead of manufacturing code findings, and still check title convention and CI. Mark any URL you haven't fetched `[UNVERIFIED]`.

Step 4: Read the whole diff, including what the title doesn't mention

Skim for the shape, then dive into hunks. Three failure modes:

• **A docs-shaped title never lowers the read bar.** PR #2473, "docs: fix typo in getting started

guide", added an npm `preinstall` hook that hijacked `git` via `GITHUB_PATH` to exfiltrate an RSA-encrypted `GITHUB_TOKEN`. Read every file in any PR touching `.hugo/`, `package.json` lifecycle scripts, `.github/workflows/`, or `.ci/`. A file the title and description don't account for is itself blocking.

• **Look for what's *missing*, not just what's wrong:** a refactor applied to 4 of 5 call sites,

a fix whose mirror bug still lives elsewhere, a behavior change with no test update, an error swallowed silently.

• **A hunk is not enough context to judge a hunk.** Read the enclosing function for anything

correctness-relevant, and grep call sites when a signature, config field, or parameter changes. A finding that needs a look outside the diff is the one no other reviewer will make.

Step 5: Check the diff against the issue it claims to fix

Keep this separate from Step 6: a PR can follow every convention and still implement the wrong thing. Read the linked issue (`gh issue view <n> --repo googleapis/mcp-toolbox --comments`), then ask three questions:

• **Missing:** What the issue asked for that the diff doesn't do. A partial fix that closes the

issue is worse than none, since the remainder becomes invisible.

• **Extra:** Unrelated changes bundled in. Ask for a split (`CONTRIBUTING.md`, keep PRs small).
• **Wrong:** Implemented, but not what the issue described. Quote the issue line beside the

`file:line`.

With no linked issue the PR description is the spec: same three questions, and note that the intent is self-declared.

Step 6: Work the review dimensions

Skip a dimension when it doesn't apply: say so, don't invent a finding.

• **Title & description.** Conventional Commits with the right `type(scope)` per

`CONTRIBUTING.md`, plus `!`/`BREAKING CHANGE` for breaking changes. Body follows [`.github/PULL_REQUEST_TEMPLATE.md`](https://github.com/googleapis/mcp-toolbox/blob/main/.github/PULL_REQUEST_TEMPLATE.md): what, why, completed checklist, `Fixes #<n>`. Note a missing issue link; don't block on it alone.

• **Correctness.** Cite `file:line` and name the failure case, never

"looks risky".

• *Bugs CI won't catch:* Unhandled error returns, nil/empty input, off-by-one and boundary

conditions, concurrency, behavior contradicting stated intent.

• *Type conversion at the MCP boundary*: Drivers return native types

that don't serialize (MySQL `[]byte` for decimals, nulls as `nil`/`None`). Require explicit ha