# Review Prs > 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. - Skill: `googleapis/review-prs` (Agent Skill, multi-file: 4 files) - Install (CLI): `npx skillmds@latest add googleapis/review-prs` - Raw SKILL.md: https://api.skillmd.com/api/skills/googleapis/review-prs/raw - Safety review: pending - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: AI & ML - Author: googleapis (https://skillmd.com/u/googleapis) - Updated: 2026-09-17 - Page: https://skillmd.com/skills/googleapis/review-prs --- # 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 ```bash gh pr view --repo googleapis/mcp-toolbox --json number,title,body,author,labels,files,additions,deletions,commits,baseRefName,headRefName,state,isDraft,reviewDecision gh pr diff --repo googleapis/mcp-toolbox gh pr checks --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 --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 #`. 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//` 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/.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//_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//head:pr`, then `git worktree add /tmp/pr pr` 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` 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 #: **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).