Skip to content

feat: add read-only MCP tool annotations - #209

Open
conorbronsdon wants to merge 1 commit into
googleanalytics:mainfrom
conorbronsdon:feat/read-only-tool-annotations
Open

feat: add read-only MCP tool annotations#209
conorbronsdon wants to merge 1 commit into
googleanalytics:mainfrom
conorbronsdon:feat/read-only-tool-annotations

Conversation

@conorbronsdon

@conorbronsdon conorbronsdon commented Aug 7, 2026

Copy link
Copy Markdown

Sets annotations on every tool this server returns from tools/list:

"annotations": { "readOnlyHint": true, "openWorldHint": true }

Today the key is absent from the wire — adk_to_mcp_tool_type builds each mcp.types.Tool from name, description, and inputSchema only, and nothing downstream sets the field. Annotations are how a client learns a tool only reads, which it can use when describing the tool and when deciding whether a call needs a confirmation prompt.

readOnlyHint: true rests on what the code calls: across all nine tools, the only methods invoked on the Data and Admin API clients are reads (get_property, get_metadata, the list_* methods, and the run_report/run_realtime_report/run_funnel_report methods). destructiveHint and idempotentHint are omitted because the mcp package documents both as meaningful only when readOnlyHint == false.

Verified over stdio: tools/list shows the key on 0/9 tools before this change and 9/9 after. Tests pin the exact annotations object per tool and the tool-name set — the name pin is a deliberate tripwire so a future tool can't inherit readOnlyHint silently; say the word if you'd rather not carry it.

A follow-up comment below carries the full mechanism and verification detail for in-depth review.

Every tool returned from tools/list now carries readOnlyHint: true and
openWorldHint: true. Previously the annotations key was absent entirely,
since adk_to_mcp_tool_type sets only name, description, and inputSchema.

readOnlyHint reflects what the code calls. Across the nine tools the only
methods invoked on the Data and Admin API clients are get_property,
get_metadata, the list_* methods, and the run_*_report query methods; none
of them writes. The analytics.readonly scope in tools/client.py is defense
in depth rather than the guarantee, because google.auth.default applies
scopes through with_scopes_if_required, which leaves the user credentials
from 'gcloud auth application-default login' untouched.

destructiveHint and idempotentHint are omitted because ToolAnnotations
documents both as meaningful only when readOnlyHint is false.
@conorbronsdon

conorbronsdon commented Aug 7, 2026

Copy link
Copy Markdown
Author

In-depth implementation and verification notes — written so both human reviewers and coding agents evaluating this PR have the full mechanism in one place. The description above is the short version; this comment carries the verification detail and the judgment calls a reviewer may want to interrogate.

Why readOnlyHint rests on the code, not the OAuth scope. analytics_mcp/tools/client.py requests analytics.readonly, but that supports the claim without establishing it: google.auth.default applies scopes through with_scopes_if_required, which returns credentials untouched when requires_scopes is False — and that is the case for the user credentials produced by gcloud auth application-default login, the setup path the README documents. The scope therefore binds for service accounts and is a no-op on the documented ADC path (the README's gcloud commands do request analytics.readonly when minting the credential, so that path is scoped — just not by this code). It is defense in depth; the read-only guarantee is that the nine tools invoke only read methods: get_property, get_metadata, list_account_summaries, list_google_ads_links, list_reporting_data_annotations, run_report (twice), run_realtime_report, run_funnel_report.

Why openWorldHint is set explicitly. The tools send requests to the Google Analytics APIs, and true matches the spec default, so it carries no information a strict client couldn't infer. It is set anyway so a reader of tools/list doesn't need to know the defaults to read a security-adjacent field. Happy to drop it if you'd rather keep the payload minimal.

Why the other two hints are omitted rather than set. mcp.types.ToolAnnotations documents destructiveHint and idempotentHint as "meaningful only when readOnlyHint == false" — checked at both 1.24.0, the dependency floor, and 1.29.0 — matching https://modelcontextprotocol.io/specification/2025-11-25/schema#toolannotations

Wire verification. Ran the server over stdio (initializenotifications/initializedtools/list, newline-delimited JSON-RPC), checking key presence rather than truthiness. Before: annotations present on 0/9 tools, raw substring count 0 on the wire. After: present on 9/9, raw count 9, and each tool holds its own ToolAnnotations instance (the type is mutable; a shared instance would let a change to one tool's annotations reach all nine, so the test asserts distinct objects).

Suite. nox -s tests-3.12 and tests-3.13: 2 tests before, 5 after, OK on both. nox -s lint passes. 3.10 and 3.11 were not run locally; the change uses no version-specific syntax.

The tool-name pin, and PR #178. tests/coordinator_test.py pins the exact set of nine tool names. Without it, tools added later inherit readOnlyHint: true silently — the one failure mode worth a build break instead of a default. The cost is a one-line update whenever the tool list changes: #178 would add ten tools (its title says nine; the diff adds ten), the merge with this branch is textually clean, and the conflict is semantic only. If #178 lands first I will rebase and extend the pin. Maintainers who consider the pin churn can ask for it to be dropped; the annotation assertions stand on their own without it.

Two things you will notice in CI output. The tool count here is nine (from tools/list) while the README documents seven — list_property_annotations and run_conversions_report are registered but undocumented, left alone to keep this PR narrow. And importing the coordinator surfaces two pre-existing ADK [EXPERIMENTAL] UserWarnings in test output, new only because the suite did not import ADK before; they are left visible rather than filtered.

@conorbronsdon
conorbronsdon marked this pull request as ready for review August 7, 2026 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant