Commit 5b7bacc

- `maintainer/` — skills for maintaining the toolbox itself (e.g. `triage-issues`,

`review-prs`).

name: review-prs

description: >-

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.

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.

- `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.

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.

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

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]`.

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.

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.

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.

- **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.

- **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.

- **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.

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.

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.

- **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.

Use the output format below. Never post it yourself.

- **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.

- **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.

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

../../../../CONTRIBUTING.md

../../../../DEVELOPER.md

../../../../maintainer-playbook.md