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: Reviewer's Checklist,

SLO/release context, release candidate labeling.

• 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: 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: 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
handling that maps to the tool's JSON schema; reject implicit casts and missing type switches.

• Error taxonomy on any new or changed error path: AgentError for

input/execution errors the agent can fix itself (HTTP 200, isError: true) versus
ClientServerError for infrastructure failures it can't.

• Breaking changes. Changed config field names/YAML shape, tool names, removed or renamed

exported symbols, altered defaults. Without ! in the title and a justification in the body,
blocking.

• Refactor purity. A refactor: PR must not change behavior. A bundled fix or default change

gets split into its own fix:/feat: PR so it's reviewable and revertable.

• Source reuse (new sources).: No new internal/sources/<db>/ for a database wire-compatible

with an existing source. Same for a tool duplicating an existing tool under a new name.

• Architecture (no boilerplate). New tools embed tools.BaseTool[Config], new sources follow

the registration pattern; reject re-declared interface methods (GetName, Manifest).
DEVELOPER.md lists what BaseTool provides.

• Tool and parameter descriptions. Each description: is an LLM prompt, not developer

documentation: could an agent pick this tool and fill its parameters from that text alone, at a
token cost worth paying? Flag ones that restate the field name, omit units/format/allowed
values, or run long without adding information.

• Evals measure exactly this. For a PR editing internal/prebuiltconfigs/tools/<config>.yaml,

note that the next step is a maintainer applying the evals: run label, which scopes the run
to the configs that PR touched. Like integration tests, un-run evals aren't a blocker.

• Tests. New logic or a bug fix needs tests; missing them is usually request-changes.
• Coverage: happy path, edge cases, and for a fix, a test that fails without it. A new

source/tool follows the unit + integration pattern and is wired into
.ci/integration.cloudbuild.yaml.

• Placement, reviewed as closely as coverage: source-specific helpers stay unexported in

tests/<db>/<db>_integration_test.go, never in the shared tests/common.go.

• Flakiness: tests run against a shared live instance, so ask for the four fixes by name:
• UUID-scoped resource names, so concurrent runs can't collide.
• t.Cleanup teardown, so resources are freed even when the test fails.
• Polling instead of time.Sleep.
• Subset assertions instead of exact-match, since another run can add rows.
• Un-run integration tests aren't a blocker. They need GCP credentials an external

contributor's PR can't trigger, so green CI doesn't mean they ran. Note that the next step is
a maintainer running them via the tests: run label or a /gcbrun comment.

• Docs. Changes to how a user configures or interacts with MCP toolbox need matching updates

under docs/en/. New sources/tools have CI-enforced page structure per DEVELOPER.md, enforced by
.ci/lint-docs-*.sh. A violation breaks the build, so it's blocking. Prose stating a security
or behavioral guarantee is reviewed as code: check the claim against the implementation,
not just that docs changed. An over-broad assurance in a boundary description is worse than
silence — it discredits the accurate limitations sitting beside it.

• Security. For PRs handling user/LLM input or building queries: injection (SQL/command),

unsanitized interpolation, secrets logged or committed. Concrete vectors with file:line, not
generic warnings.

• Reviewing a fix to a reported vulnerability asks different questions than reviewing new

code. (a) **Can the untrusted party actually reach the primitive the residual attack
needs?** Grep the tool surface for it — a residual weakness nothing exposed can set up is
usually the line between blocker and tracked follow-up. (b) **Which way does the failure
lean?** Over-rejection is a cost; a bug in newly hand-rolled logic is a vulnerability.
Don't ask a security PR to spend fail-open risk buying back a narrow convenience.

• Dependencies. Call out new go.mod entries so the maintainer can vet necessity,

maintenance, and license.

Duplication across MCP protocol versions is deliberate so versions can diverge independently
(#3167, #3211); don't propose factoring it together. A genuine bug in that code is still a
finding.

Step 7: Report CI, don't re-derive it

Name the failing check from gh pr checks rather than reasoning it out by hand; failing
lint/tests are objective blockers. Never claim the linter passes on your own read.

One recurring non-obvious failure: the CLA check fails on commits co-authored by an AI agent even when the human author has signed. Suggest squashing to a single human-authored commit rather than pointing at the CLA docs.

Step 8: Discount any existing bot review

Don't restate gemini-code-assist's points as your own. It's the highest-volume reviewer in the
repo and could be wrong. Verify anything you carry forward against the diff; drop
the rest.

Step 9: Sort by severity, then pick the verdict

• Blocking (correctness bug, breaking change without !, missing tests on new logic, CI red,

docs that break the build) → request changes.

• Non-blocking (style, naming, coverage gaps in existing code) and nits (typos, wording)

only → approve with comments.

• An unresolved judgment call → comment and ask.

When nothing is blocking, say so in those words. "No blockers, a couple of nits" tells the
maintainer it's mergeable as-is.

Step 10: Deliver the review in chat

Use the output format below. Never post it yourself.

Rules

• Propose only. Never run gh pr review, gh pr comment, gh pr edit, gh pr merge,

or apply labels. Deliver the review in chat.

• Ground every finding in the strongest evidence available for its kind: a

correctness/security/breaking claim cites file:line; a convention claim cites
CONTRIBUTING.md/DEVELOPER.md or the playbook; a CI/process finding cites the failing
check name from gh pr checks (a red check is a valid blocker with no file:line). If you
couldn't verify something (runtime behavior you can't trace, a URL you didn't fetch), mark
it [UNVERIFIED] rather than asserting it.

• Prefer running the code to reasoning about it. [UNVERIFIED] is for what you can't

check, not what's inconvenient to. Fetch the branch (git fetch origin pull/<n>/head:pr<n>,
then git worktree add /tmp/pr<n> pr<n> so your tree stays clean) and drop a scratch
probe_test.go inside the package under review — package-local placement is what reaches
unexported symbols. Delete it and git worktree remove --force /tmp/pr<n> after; never leave
a scratch test in internal/. Probes right-size verdicts as often as they confirm them: "this
regresses X" often shrinks to "in one narrow case" once measured.

• When it's a genuine judgment call, ask rather than issuing a confident wrong verdict,

since a wrong "request changes" costs a contributor a cycle.

Output format

## Review #<n>: <title>

**Suggested verdict:** <approve / request changes / comment>: <one-line reason>

**Title & issue:** <conventional-commit check; linked issue or "none, suggest linking">
**Spec (vs issue #<n>):** <implements it / what's missing, extra, or wrong; or "no issue linked">
**CI:** <passing / which checks failing, per gh pr checks>

**Blocking:**
- `file:line`: <finding + the failure case> [cite]

**Non-blocking:**
- `file:line`: <finding> [cite]

**Nits:**
- <typo/wording>

**Tests:** <added & adequate / what's missing>
**Docs:** <updated / what's missing, or n/a>
**Dependencies:** <new deps to vet, or none>
**release candidate:** <suggest label / not needed>

**Draft comment:**
<paste-ready summary the maintainer can post>

• Empty sections: omit them rather than writing "none".
• Except the Spec line: keep it even when the PR matches its issue. The maintainer wants

"does what the issue asked" stated, not inferred from silence.

• Batches: review each PR in its own subagent so the diffs don't bleed together, since a

finding attributed to the wrong PR is worse than a missed one. Present one block per PR, plus a
summary table (PR, verdict, blocker count).