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.
Installer avec Codex ou Claude Copiez ce prompt, collez-le dans Codex, Claude ou un autre assistant, puis laissez-le vérifier la page du skill et l'installer pour vous.
Une commande directe contourne le prompt de vérification. Examinez la source avant de l'exécuter.
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/CONTRIBUTING.md: title/scope format (Conventional
Commits, with the table), keep-PRs-small, link-an-issue. Cite for title, description,
and process findings.
type
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.
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.goinside 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).
