-
Notifications
You must be signed in to change notification settings - Fork 198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ToolAnnotations #185
ToolAnnotations #185
Conversation
73587e8
to
6179891
Compare
This comment was marked as outdated.
This comment was marked as outdated.
a8c8a5d
to
2c348a2
Compare
… clarification to `Tool.description`
4c30b60
to
c17ee30
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I have some worries here, particularly around the trust model. I do think adding more expressivity here will be a good thing, but we need to be careful not to promise something that will actually be broken in practice.
schema/draft/schema.ts
Outdated
icon?: { | ||
data: string; // base64-encoded image data | ||
contentType: string; // MIME type of the image | ||
}; | ||
|
||
/** | ||
* A URL from which a client can fetch an icon to represent this object. | ||
*/ | ||
icon_url?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should support per-tool icons—I think this will be very visually noisy in any application that implements them.
I'd rather lean on the idea of servers having icons (although this is probably just achievable with a favicon), or data having icons (e.g., a resource loaded from the web has the favicon of that webpage).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense - I don't really have a strong opinion, though I could imagine genuinely different tools wanting their own icons and tool bundles that work together on the same task just shipping the same icon.
Other related things:
- @jerome3o-anthropic mentioned tool icons as potentially useful, not sure if he has a strong opinion though.
- Add several annotation fields #201 proposes adding icons to
Annotated
. I guess the use case there is for more ephemeral objects, so not directly analogous w.r.t. the "too noisy" problem, but fyi in case you wanted to chime in there.
I'll default to pulling them out of here absent followup discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I do lean toward custom icons being acceptable for data types, but less so for tools (which IMO should bear the branding of their server).
schema/draft/schema.ts
Outdated
/** | ||
* If true, this tool does not perform writes or updates, | ||
* or otherwise change server-side state in ways that would | ||
* be visible in subsequent tool calls. | ||
* | ||
* If not set, this is assumed to be false. | ||
*/ | ||
readOnly?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are a few dimensions we care about:
- Is this read-only?
- If not read-only, is it destructive? (e.g., "delete" is different from "append")
- If not read-only, is it idempotent? (What is the effect of calling it multiple times?)
I wonder how we can best capture these nuances. IMO we should try to land this expressivity all at once, so server authors can update their implementations to capture the most useful information in one fell swoop.
Additionally, in all cases, we must remember that clients can't necessarily trust servers when they provide this information (a malicious server could claim that nothing bad will happen, then do very bad things), so it must be taken as a hint only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First, totally agreed on emphasizing trust - I'll add language about this to every property. (Also just realized I haven't documented this at all, will add.)
Re contingent properties like "destructive" and "idempotent" (and I agree we should add the obvious ones eagerly) - it's that classic data modeling situation where we could either use a variant type (pro: no nonsense combinations, i.e. "correct by construction") or a product of bools (pro: only primitive values).
I guess the variant-type version in TS would be something like
type ReadOnly = {
readonly: true;
};
type Writable = {
readonly: false;
destructive: boolean;
idempotent: boolean;
};
type OperationMode = ReadOnly | Writable;
Personally I'd be inclined to do this kind of thing rather than just a plank of bools, but interested in your thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these annotations should be explicitly mentioned in:
https://spec.modelcontextprotocol.io/specification/draft/#security-and-trust--safety
with guidance on how they interact with user consent from Client and Server author perspectives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PederHP thanks for calling this out - added
schema/draft/schema.ts
Outdated
/** | ||
* If true, this tool may interact with an open set of "external" | ||
* entities, e.g. by making web queries. | ||
* | ||
* If not set, this is assumed to be true. | ||
*/ | ||
openWorld?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit skeptical about this one, for the trust reason I mention above. Even if this is false
, a malicious server could still send the data elsewhere.
Can you share more about how you envision the client behaving differently based on this flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mental model for this is that it describes whether or not a tool operates in a closed environment - so e.g. a database tool would be false
(as would the current filesystem tool), but an open web query tool would be true
. A client might use this to select only tools that operate in closed environments.
There's definitely an elephant in the room here, that clients will often want to know much more fine-grained and varietal operational information about the behavior of specific tools than we can provide in a universal schema. The protocol's stance on how that kind of info is best delivered is one question, but more immediately it means that the properties we do include need to pay their way - I think of the metric as being some kind of 80/20 rule where a property is informative enough, for enough tools, to warrant inclusion. openWorld
came from some conversations about whether a tool "reads externally", which passed the smell test for me, but I could definitely see a counterargument that it's not sufficiently general to be included here.
Re trust - yes, definitely a concern here, but not in a qualitatively different way as the other operational properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the information itself, if it could be trusted, would be very useful, but I wonder how a client makes use of this flag knowing that it's not trustable.
I guess that maybe degrades to, "Clients should apply a high scrutiny bar to all untrusted servers and ignore most of these annotations, until the user tells them not to."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100%, actually I think "Clients should ignore annotations from untrusted servers" applies to all annotations, even title
- but especially the ones that describe operational properties. Worth strengthening the language.
Separately, do we discuss the question of server trust elsewhere in the spec? (Not as a matter of definition, but operationally.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some general mentions—e.g., https://spec.modelcontextprotocol.io/specification/2024-11-05/#security-and-trust--safety
- Replace readOnly property with more descriptive effectHints object - Rename openWorld to openWorldHint to clarify it's a hint - Add additional tool effect properties (destructive, idempotent) - Clarify these properties are hints only in documentation - Remove unused DisplayAnnotations interface 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
e47b39d
to
3a96a59
Compare
@jspahrsummers went ahead and did a variant-type version of Re docs: given that the wording is "a tool definition includes" I'm not sure if the aim is to be concise here and defer to the schema for the full definition, or we want to be completist, so I held off on adding |
Drive by comment, since I've been musing something similar recently and was pointed this direction. The model that might be worth considering is to have a place for servers to put arbitrary annotations on primitives (in addition to standard annotations). This allows for the broader ecosystem to experiment as much as they please, while also creating a space to watch trends emerge. Over time, if cultivated properly, the more "standard" annotations should emerge, but with the added benefit of the community already having psuedo-standardized on expected behavior. |
schema/draft/schema.ts
Outdated
/** | ||
* If true, this tool does not perform writes or updates, | ||
* or otherwise change server-side state in ways that would | ||
* be visible in subsequent tool calls. | ||
* | ||
* If not set, this is assumed to be false. | ||
*/ | ||
readOnly?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these annotations should be explicitly mentioned in:
https://spec.modelcontextprotocol.io/specification/draft/#security-and-trust--safety
with guidance on how they interact with user consent from Client and Server author perspectives.
@@ -1980,6 +2013,43 @@ | |||
], | |||
"type": "object" | |||
}, | |||
"ToolAnnotations": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having some kind of additional properties mechanism to have free-form annotations would be very useful. The use case is having internal MCP servers that we use with internal MCP clients (that are back-end services). The annotations we'd use (which could include rate-limiting information, information sensitivity classifiers, etc.) would likely not be general enough to ever find their way into the spec, but they'd of great value to us.
The alternative is that we need maintain this information elsewhere - or do a branch of the SDK(s) and add the fields as what we do is all internal, but that's not optimal either. Free-form annotations are very useful for MCP-based infrastructures that aren't directly user-facing. We don't need to have our annotations be known to clients at large, for servers we'll never distribute externally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All objects in the MCP spec are intentionally open-ended, so it's always possible to add custom fields that only your server and client implementations understand. (No forking required!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another drive by comment, I just stumbled on this PR whilst considering a parallel internal implementation. Some thoughts:
- Information sensitivity classifiers is one of the examples we had. So this is always going to need to be extensible for different businesses with different definitions / risks / policies
- Looks like effectHints is not extensible as currently implemented? We had other property ideas e.g. "appendOnly" or "irrevocable". If we want this to be extensible for now then maybe effectHints should be too
- In general it might be good to give examples of how this should be extended for custom metadata / hints and guidance on when to share back standards
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All objects in the MCP spec are intentionally open-ended, so it's always possible to add custom fields that only your server and client implementations understand. (No forking required!)
I hope you don't mind a comment on this on a merged PR, but it seemed like the obvious place. Very glad to hear it - I think this makes the protocol stronger and helps facilitate bottom-up growth.
This is true for weakly-typed languages, like Python or TypeScript, but for C# it makes a difference if the schemas explicitly has metadata and/or kwargs-style additional properties (as some types do, but not all).
So I think either adding a basetype that makes this explicit, or adding it explicitly to types where it is likely to be used (like Tools) would be very helpful. Especially for cases where a server is Python/TypeScript and the Client is C#.
I assume similar concerns are valid for Rust and Java. Perhaps I should open an issue about consistency in terms of how open-ended types are handled in the scehma?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM!
schema/draft/schema.ts
Outdated
effectHints?: | ||
| { | ||
/* Tool is read-only. */ | ||
readOnly: true; | ||
} | ||
| { | ||
/* Tool is read-write. */ | ||
readOnly: false; | ||
/* If true, tool may perform destructive updates to its environment, as opposed to only additive updates. Default: true */ | ||
destructive?: boolean; | ||
/* If true, repeated calls with the same arguments will have no additional effects. Default: false */ | ||
idempotent?: boolean; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I think naming these types is helpful, for documentation but also to create more distinct entities in the JSON Schema, and possibly assist codegen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question, are subobjects also open-ended in the spec? [Edit: looks like the answer is yes, for spec-defined objects... another reason to break these out into their own types]
If not, the extensibility discussion above is actually shifting my preference back towards simple bool properties (with appropriate documentation of when variant properties should be ignored) for the obvious reason...
Edit 2: I'm going to go ahead and flatten this, even though we could add multilevel extensibility by making effectHints
a variant type across subobjects. In the presence of extensibility it feels like the ROI of the extra precision isn't high enough to offset the extra fussiness, since we'd be defining a taxonomy that some tools won't fit into w.r.t. custom properties, as well as complicating parsing.
- simplify data model for operational properties, to facilitate extensibility (at the cost of correctness by construction) - added documentation for `annotations` property in tool definitions - added language in `Security and Trust & Safety` section re trusting annotations
@jspahrsummers looks like my post-approve changes caused your review to be dismissed, sorry about that, can I get you to TAL? Here's what's changed:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
{{< callout type="warning" >}} For trust & safety and security, clients **MUST** consider | ||
tool annotations to be untrusted unless they come from trusted servers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this needs a closing tag. Can you confirm by building the docs locally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
argh yes confirmed and fixed
29d16f3
to
8cf6f27
Compare
Motivation and Context
ToolAnnotations
provides additional optional properties for describing a tool's behavior beyond itsinputSchema
anddescription
(the latter being intended for model hinting, rather than user presentation).ToolAnnotations
contains both operational and display properties. Display properties are defined in theDisplayAnnotations
interface, whichToolAnnotations
inherits from. (This anticipates the use ofDisplayAnnotations
elsewhere, as proposed in #201 .)How Has This Been Tested?
N/A
Breaking Changes
None
Types of changes
Checklist
Additional context