Skip to content

fix(mcp): annotate tools with readOnlyHint so reads aren't treated as edit tools - #280

Merged
dodeja merged 2 commits into
mainfrom
fix/mcp-tool-annotations
Jun 26, 2026
Merged

fix(mcp): annotate tools with readOnlyHint so reads aren't treated as edit tools#280
dodeja merged 2 commits into
mainfrom
fix/mcp-tool-annotations

Conversation

@dodeja

@dodejadodeja commented Jun 26, 2026

Copy link
Copy Markdown
Member

Root cause

The Terminal49 MCP tools were registered via server.registerTool(name, config, handler)without an annotations field. With no MCP ToolAnnotations, clients have no read-only signal and default to treating every tool as mutating/destructive. This mis-categorizes the 9 read-only lookups as edit/write operations, adding unnecessary confirmation friction and making the read tools look as risky as a write.

The fix

Add an annotations field to all 10 tool registrations in packages/mcp/src/server.ts. No tool behavior, schemas, or handlers changed — annotations only. The MCP SDK (@modelcontextprotocol/sdk ^1.29.0) supports readOnlyHint / destructiveHint / idempotentHint / openWorldHint on the registerTool config object.

Categorization

Read-only{ readOnlyHint: true, openWorldHint: true } (9 tools, but one differs on open-world, see below):

  • search_container
  • get_container
  • get_container_route
  • get_container_transport_events
  • get_shipment_details
  • list_containers
  • list_shipments
  • list_tracking_requests

Read-only, bounded catalog{ readOnlyHint: true, openWorldHint: false }:

  • get_supported_shipping_lines — the supported-carriers catalog is closed/enumerable, so openWorldHint: false.

Write (the only one){ readOnlyHint: false, destructiveHint: false, idempotentHint: true, openWorldHint: true }:

  • track_container — a create, not destructive, and idempotent: re-tracking the same number returns the existing tracking request rather than creating a duplicate.

So: 9 read-only tools + 1 non-destructive idempotent write (track_container).

Tests & docs

  • New regression test packages/mcp/src/annotations.test.ts builds the server via createTerminal49McpServer('token'), reads (server as any)._registeredTools, and asserts each tool's hints (the 9 reads are readOnlyHint===true; track_container is readOnlyHint===false, destructiveHint===false, idempotentHint===true; get_supported_shipping_lines is openWorldHint===false).
  • docs/mcp/home.mdx gets a <Note> at the top of the Tools reference section clarifying that every tool is read-only except track_container, which creates a tracking request and is idempotent.

Green gate

All gates pass:

build @terminal49/sdk ✓
build @terminal49/mcp ✓
type-check @terminal49/sdk ✓
type-check @terminal49/mcp ✓
test @terminal49/sdk 51 passed | 2 skipped
test @terminal49/mcp 81 passed (was 77; +4 from annotations.test.ts)
oxlint @terminal49/mcp clean

(Baseline was SDK 51 pass / 2 skip, MCP 77 pass.)

Notes

🤖 Generated with Claude Code

Greptile Summary

This PR adds MCP ToolAnnotations to all 10 tool registrations in packages/mcp/src/server.ts so clients can correctly distinguish read-only lookups from the single write operation (track_container). No tool handlers, schemas, or runtime behaviour are changed.

  • server.ts: One annotations: field added per registerTool call — 9 tools marked readOnlyHint: true, track_container marked readOnlyHint: false, destructiveHint: false, idempotentHint: true, and get_supported_shipping_lines additionally marked openWorldHint: false.
  • annotations.test.ts: New regression test file that introspects _registeredTools (SDK internal) to assert each tool's hint values, with separate cases for the idempotent write and the closed-world catalog.
  • docs/mcp/home.mdx: Adds a <Note> clarifying the read-only vs write split at the top of the Tools reference section.

Confidence Score: 4/5

Safe to merge — the change is additive metadata only; no handlers, schemas, or API calls are touched.

The production change in server.ts is minimal and correct: every tool gets an annotations block matching the PR categorisation. The only concerns are in the test file: it couples itself to the SDK private _registeredTools field, and the bulk read-only loop omits openWorldHint: true assertions for the eight open-world tools, leaving a gap a future regression could slip through undetected.

packages/mcp/src/annotations.test.ts — private SDK field access and incomplete openWorldHint coverage.

Important Files Changed

FilenameOverview
packages/mcp/src/server.tsAdds one annotations line per tool registration (10 tools total). No handler, schema, or behavior changes. All 9 read-only tools correctly get readOnlyHint: true; track_container correctly gets readOnlyHint: false, destructiveHint: false, idempotentHint: true; get_supported_shipping_lines correctly gets openWorldHint: false.
packages/mcp/src/annotations.test.tsNew regression test asserting annotation hints on all 10 tools. Relies on (server as any)._registeredTools — a private SDK internal — which could silently break on SDK upgrades. The bulk read-only test also omits openWorldHint assertions for the 8 open-world tools.
docs/mcp/home.mdxAdds a <Note> callout above the tools reference clarifying read-only vs write behaviour. Documentation-only change, accurate and concise.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
Client([MCP Client]) --> |calls tool| Dispatch{Tool annotation\nreadOnlyHint?}
Dispatch -->|true| ReadPath[Read-only path\nNo confirmation needed]
Dispatch -->|false| WriteCheck{destructiveHint?}
WriteCheck -->|false + idempotentHint:true| IdempotentWrite[Non-destructive\nidempotent write\ntrack_container]
WriteCheck -->|true| DestructiveWrite[Destructive write\nConfirmation required]
ReadPath --> ReadTools["9 read tools:\nsearch_container / get_container / get_container_route\nget_container_transport_events / get_shipment_details\nget_supported_shipping_lines openWorldHint:false\nlist_containers / list_shipments / list_tracking_requests"]
IdempotentWrite --> TrackAPI[Terminal49 API\nPOST /tracking_requests\nreturns existing if duplicate]
ReadTools --> ReadAPI[Terminal49 API\nGET endpoints]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
Client([MCP Client]) --> |calls tool| Dispatch{Tool annotation\nreadOnlyHint?}
Dispatch -->|true| ReadPath[Read-only path\nNo confirmation needed]
Dispatch -->|false| WriteCheck{destructiveHint?}
WriteCheck -->|false + idempotentHint:true| IdempotentWrite[Non-destructive\nidempotent write\ntrack_container]
WriteCheck -->|true| DestructiveWrite[Destructive write\nConfirmation required]
ReadPath --> ReadTools["9 read tools:\nsearch_container / get_container / get_container_route\nget_container_transport_events / get_shipment_details\nget_supported_shipping_lines openWorldHint:false\nlist_containers / list_shipments / list_tracking_requests"]
IdempotentWrite --> TrackAPI[Terminal49 API\nPOST /tracking_requests\nreturns existing if duplicate]
ReadTools --> ReadAPI[Terminal49 API\nGET endpoints]
Loading

Fix All in Codex

Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---### Issue 1 of 2
packages/mcp/src/annotations.test.ts:23-26
**Fragile private-field access**`_registeredTools` is an undocumented internal of `@modelcontextprotocol/sdk`. If a future SDK version renames or restructures this map (e.g. to `_tools` or moves hints into a nested `definition` object), every assertion in this file will silently pass against `undefined` rather than failing loudly, defeating the regression intent. Consider pinning the exact SDK minor version in the test package and adding a guard like `expect(Object.keys(tools).length).toBeGreaterThan(0)` — which already exists in the "annotates every registered tool" case — or opening a tracking issue for when `McpServer` exposes a public `getRegisteredTools()` surface.
### Issue 2 of 2
packages/mcp/src/annotations.test.ts:42-50
**`openWorldHint` not asserted for the 8 open-world read tools** — the loop checks `readOnlyHint: true` but not `openWorldHint: true`. If a future edit accidentally sets `openWorldHint: false` on, say, `list_shipments`, this test won't catch it; only `get_supported_shipping_lines` gets its `openWorldHint` explicitly verified (in a separate case). Adding `expect(annotations?.openWorldHint, name).toBe(true)` to this loop for the non-catalog tools would close the gap, or excluding `get_supported_shipping_lines` from this array and asserting `openWorldHint: true` for the remaining eight here would make the intent clearer.

Reviews (1): Last reviewed commit: "fix(mcp): annotate tools with readOnlyHi..." | Re-trigger Greptile

Greptile also left 2 inline comments on this PR.

… edit tools
Tools were registered without MCP ToolAnnotations, so clients defaulted to
treating every tool as mutating/destructive. This added friction for the 9
read-only tools and mis-categorized them as edit/write operations.
Add an `annotations` field to all 10 tool registrations:
Read-only (readOnlyHint: true, openWorldHint: true):
search_container, get_container, get_container_route,
get_container_transport_events, get_shipment_details,
list_containers, list_shipments, list_tracking_requests
Read-only, bounded catalog (readOnlyHint: true, openWorldHint: false):
get_supported_shipping_lines
Write (readOnlyHint: false, destructiveHint: false, idempotentHint: true,
openWorldHint: true):
track_container — a create, not destructive, and idempotent (re-tracking
the same number returns the existing tracking request, no duplicate).
Adds a regression test (annotations.test.ts) asserting each tool's hints,
and a note in docs/mcp/home.mdx clarifying the read/write split.
No tool behavior, schemas, or handlers changed — annotations only.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@vercel

vercelBot commented Jun 26, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

ProjectDeploymentActionsUpdated (UTC)
apiReadyReadyPreview, CommentJun 26, 2026 10:39am

Request Review

@mintlify

mintlifyBot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Preview deployment for your docs. Learn more about Mintlify Previews.

ProjectStatusPreviewUpdated (UTC)
terminal49🟢 ReadyView PreviewJun 26, 2026, 12:39 AM

@dodeja

Copy link
Copy Markdown
MemberAuthor

Triaged review feedback on this PR: no inline review comments or review summaries were found. The only PR comments are automated deployment/preview bots (Vercel, Mintlify), which aren't actionable. Nothing to address; PR remains as-is.

@dodeja
dodeja marked this pull request as ready for review June 26, 2026 10:25

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:4cabef2745

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment threadpackages/mcp/src/server.ts Outdated
Comment threadpackages/mcp/src/annotations.test.ts
Comment threadpackages/mcp/src/annotations.test.ts
track_container is not idempotent: when no container is found via search
it always creates a new tracking request, and a pending request (no linked
container yet) won't be matched by search on a subsequent call, so repeated
calls can create additional tracking requests. Set idempotentHint: false so
clients don't treat the write as safe to silently retry/replay, and correct
the docs note accordingly.
Also harden annotations.test.ts: assert openWorldHint: true for the
open-world read tools (excluding the closed-world catalog), and guard the
"annotates every tool" check against _registeredTools silently resolving to
an empty/undefined map if SDK internals change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@dodeja

Copy link
Copy Markdown
MemberAuthor

Triaged the automated review feedback and pushed 681eb04:

Addressed

  • Codex (P2): track_container idempotentHint is inaccurate. Correct — executeTrackContainer only short-circuits when an existing container is found via search; otherwise it creates a tracking request, and a pending request (no linked container yet) won't be matched by search on a retry, so repeated calls can create additional requests. Changed idempotentHint: true → false in server.ts, updated the matching assertion in annotations.test.ts, and corrected the docs note in docs/mcp/home.mdx to stop claiming full idempotency.
  • Greptile (P2): openWorldHint not asserted for the open-world read tools. Added expect(annotations?.openWorldHint).toBe(true) inside the read-only loop, excluding the closed-world catalog tool (get_supported_shipping_lines).
  • Greptile (P2): fragile _registeredTools private access could pass vacuously. Added a guard in the 'annotates every registered tool' test asserting the map is truthy and has at least readOnlyTools.length + 1 entries, so the per-tool loop fails loudly if SDK internals are renamed/restructured instead of silently passing.

Skipped

  • Greptile's suggestion to pin the exact SDK minor version in the test package — out of scope for this PR and a dependency-policy decision; the loud-failure guard above covers the stated regression risk without a version pin.

Green gate from the worktree: SDK build + type-check pass, MCP build + type-check pass, SDK tests 51 pass / 2 skip, MCP tests 81 pass. SDK public surface unchanged, so no docs/sdk/reference regeneration needed. Only the three changed files were formatted (no mass oxfmt churn).

@dodeja
dodeja merged commit a14dec6 into mainJun 26, 2026
17 checks passed
Sign up for freeto 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

@dodeja