Skip to content

feat: declare readOnlyHint and idempotentHint on all five tools (fixes #1) - #5

Closed
Mayur021 wants to merge 1 commit into
GenAI-Security-Project:mainfrom
Mayur021:patch-1
Closed

feat: declare readOnlyHint and idempotentHint on all five tools (fixes #1)#5
Mayur021 wants to merge 1 commit into
GenAI-Security-Project:mainfrom
Mayur021:patch-1

Conversation

@Mayur021

Copy link
Copy Markdown

Closes #1.

The server registers five tools and none of them pass annotations. A client reading tools/list sees title, description and inputSchema, with nothing describing behaviour, so it cannot distinguish a read-only server from one that
simply does not say. The read-only property is real and documented, but it lives in the README where a client cannot read it.

This adds annotations to all five: readOnlyHint and idempotentHint, both true.
Accurate on every path, since each one is a GET against raw.githubusercontent.com or the GitHub trees API and returns the same result for the same arguments within the cache window.

I have deliberately left openWorldHint unset. The tools do reach an external host, but the world they reach is a single pinned repository, and which way that reads is the project's call rather than mine. Happy to add it either way.

Worth being clear about what this is not. An annotation is a declaration, not a control, and the specification is right that clients must treat annotations as untrusted unless the server is. The clause that applies here is the second one:
a first-party server published by the project itself is the case that exception exists for. This does not make the server safer. It lets a well-behaved client tell that it is read-only, which today it cannot, so a client that fails closed
on undeclared behaviour has to treat this corpus reader like an unknown third-party server.

Additive only: five object literals, no type or signature changes. Authored through the web editor, so I have not run the typecheck locally.

Note on overlap with #3: that PR adds a sixth tool, get_corpus_revision. If it merges first I will add the same two annotations to it in a follow-up, or rebase this onto it, whichever the maintainers prefer.

…GenAI-Security-Project#1)

Signed-off-by: Mayur Agnihotri <mayur.agnihotri0021@gmail.com>

Signed-off-by: Mayur Agnihotri <mayur.agnihotri0021@gmail.com>
@Mayur021

Copy link
Copy Markdown
Author

Checked the spec on the one thing that differs between this and #4, and #4 is right.

openWorldHint defaults to true (schema 2025-06-18 and 2026-07-28, same wording in both). So leaving it unset is not the neutral deferral I described in the issue, it asserts an open world, which is the opposite of what this server is. #4 setting it to false is correct and this PR is wrong on that point.

While confirming that, one thing neither of us got right. The spec annotates idempotentHint with "(This property is meaningful only when readOnlyHint == false)". Both PRs set readOnlyHint: true and idempotentHint: true together, so the second is inert by definition. It is harmless, but it is also a claim the spec says carries no meaning in this position.

That makes the correct block:

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

Happy to close this in favour of #4 with idempotentHint dropped, or to push that change here, whichever the maintainers prefer. #4 is the closer starting point.

@narko4u

narko4u commented Aug 18, 2026

Copy link
Copy Markdown

Agreed on both points — thanks for pulling the exact schema wording.

  1. openWorldHint defaulting to true (not a neutral deferral when unset) is precisely why feat: declare read-only annotations on all tools (fixes #1) #4 sets it to false. Your spec check settles it.
  2. idempotentHint meaningful only when readOnlyHint == false — correct, and feat: declare read-only annotations on all tools (fixes #1) #4 was wrong on that too.

Applied to #4 in bdece5a: all five tools now carry exactly

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

typecheck clean, wrangler deploy --dry-run clean. Happy for #5 to close in favour of #4 — it's the closer starting point and now matches the correct block.

@Mayur021

Copy link
Copy Markdown
Author

Closing in favour of #4. It now carries the block the spec check landed on, readOnlyHint with openWorldHint false. This one still has idempotentHint on read-only tools and no openWorldHint, which is the pair I argued against once we had the exact wording. #4 is the right starting point.

@Mayur021 Mayur021 closed this Aug 19, 2026
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.

Tools carry no annotations, so the read-only property lives only in prose

2 participants