Repository navigation
Audit and fix missing ReadOnlyHint annotations on read-intent tools #2483
Description
Problem
The Coding Agent runtime's MCP permission gate auto-approves tool calls only when the underlying tool is annotated with ReadOnlyHint: true. Anything else (including undefined / missing annotation, or ReadOnlyHint: false) prompts the user.
In production we're seeing read-intent github-mcp-server tools prompting users — for example, callers using the legacy alias get_review (now consolidated into pull_request_read).
Source flow on the runtime side:
runtime/src/mcp-client/known-mcp-servers.ts:585-596—extractMCPToolAnnotationsrequiresreadOnlyHintto be a strictboolean; missing/non-boolean →undefined.runtime/src/tools/mcp-transport.ts:509—readOnly: toolConfig.readOnly ? true : falsecollapsesundefined→false→ user prompted.runtime/src/apps/permission-gate.ts— auto-approves only whenrequest.readOnly === true.
Per agreement with the runtime team, defaulting undefined to "read-only" is not the right fix (would over-grant). Instead, ensure the github-mcp-server side is the source of truth.
What's needed
- Audit every tool currently registered by github-mcp-server for missing or incorrect
Annotations.ReadOnlyHint. Most existing read-only tools already have this set (verifiedpull_request_read,issue_triage_context,semantic_issues_search,package_search,dependabot,find_closing_pull_requests,summarize_actions). Find the gaps. - Deprecated tool aliases: when
DeprecatedToolAliases(pkg/github/deprecated_tool_aliases.go) routes a call from an old name (get_review,list_issues, etc.) to a new canonical tool (pull_request_read,issue_read, etc.), make sure the alias's tool descriptor inherits the canonical tool's annotations. Today the aliased tool may be exposed without annotations, defeating the runtime's auto-approve path. - Consider adding a CI check that fails if any tool registered via
mcp.NewToollacks an explicitReadOnlyHint(true or false). Forcing the choice at registration time prevents future regressions.
How to verify
Quick way to spot missing annotations in this repo:
grep -rln 'mcp.NewTool\|toolset.NewTool' --include='*.go' | grep -v _test | xargs -L1 -I{} sh -c 'grep -L ReadOnlyHint "{}"'
Repeat for the deprecated-alias dispatch path.
Related
- Coding Agent runtime:
runtime/src/mcp-client/{known-mcp-servers.ts,out-of-proc-mcp-host.ts,mcp-registry.ts}— current consumer of these annotations. - Coding Agent permission gate behavior: github/sweagentd
runtime/src/apps/permission-gate.ts.
cc @ritchxu
Activity
FuzzysTodd commentedon May 15, 2026
runtime/src/apps/permission-gate.ts @FuzzysTodd
policylayer-dan commentedon May 16, 2026
The root cause framing here is correct: the permission gate's source of truth should be the server-side annotation, not a runtime default. Collapsing
undefinedtofalseat the transport layer is a sensible conservative default, but it only works if the server side is actually exhaustive, which it currently isn't.One thing worth surfacing in the audit:
ReadOnlyHintalone is a fairly coarse signal. A tool can be read-only in intent but still carry meaningful risk depending on the scope of credentials it operates under (e.g., a read tool with org-wide token access). Pairing the annotation audit with a review of what OAuth scopes each tool actually requires would give you a more defensible permission model than binary read/write.For teams consuming github-mcp-server alongside other MCP servers they don't control, relying solely on server-side annotations for gate decisions is brittle, since annotation quality varies across servers. That's the gap a client-side tool allowlist and explicit approval gate before tool call addresses, regardless of what the server declares.
jluocsa commentedon May 17, 2026
Happy to take a first pass at the CI piece (item 3 in the description). I've put together a static AST-based test that walks every
.gofile inpkg/github, finds everymcp.Tool{...}composite literal, and fails the build if any one of them omits an explicitAnnotations.ReadOnlyHint:field — closing the gap that the existing runtime test explicitly calls out (Go can't distinguish "unset bool" from "explicitly false" at runtime, but the AST can).All 97 current tool registrations pass. I fault-injected a missing hint to confirm the test reports
file:line tool=<name>: <reason>.Opening a PR shortly — happy to fold in the broader audit/doc work as follow-ups if you'd like this split into multiple smaller changes.
SamMorrowDrums commentedon May 17, 2026
Tracking related work here for triage visibility: #2486 is an open implementation PR for the AST validation piece, and #2487 is a draft alternative approach against the same issue.
test(github): enforce explicit ReadOnlyHint on every mcp.Tool literal
test(github): enforce explicit ReadOnlyHint on every mcp.Tool literal