Tool annotations, icons, content annotations (ADR 0006) - #28
Conversation
- New metadata types: ToolAnnotations (readOnly/destructive/idempotent/ openWorld hints + title, all advisory and unset by default), Icon (src/mimeType/sizes), and content Annotations (audience/priority/ lastModified) attached via the new ContentAnnotated wrapper, whose annotations merge into the inner block's JSON object (and parse back). - Definition types gain the corresponding fields: annotations + icons on ToolDefinition, icons on Prompt/Resource/ResourceTemplate definitions — all omitted from JSON when unset, so existing fixtures are unaffected. - Smart constructors (mkToolDefinition, mkPromptDefinition, mkResourceDefinition, mkResourceTemplateDefinition) build definitions from required fields only, so future optional fields stop breaking manual constructions; in-repo manual sites migrated. - DefinitionOptions: one per-constructor customization record (description, title, icons, tool annotations, and constructor-scoped field descriptions — fixing the old global-namespace wart) behind new WithOptions variants of all five derivations. The legacy [(String, String)] description API is unchanged and now implemented as an adapter over the same generic path.
… 0.3.0.0 - DefinitionMetadata spec: annotation/icon JSON shapes (unset fields omitted), ContentAnnotated merge + FromJSON round-trip, options-based derivation carrying description/title/icons/annotations, and constructor-scoped field descriptions (same-named fields described differently per constructor — the old global-namespace wart, now fixed and pinned by a test). - Conformance corpus: the extended reference server gains annotated_probe, derived via WithOptions so the corpus pins the options wire shape (annotations/title/icons on the definition, annotated content on the result); tools-call-annotated cases in both eras, tools-list-extended regenerated deliberately. The v0.2.0-anchored fixtures are untouched (new fields are omitted when unset). - Complete example showcases WithOptions: read-only/idempotent hints on search, destructive hint on checkout, scoped argument descriptions. - VERSIONING: extending the exported definition datatypes and Content is a major change under strict PVP, so the pending 0.2.1.0 line is folded into 0.3.0.0 (CHANGELOG notes the fold; ADR_0005's landed version updated accordingly; ROADMAP updated — ADR 0006's minor-bump guess did not survive contact with the PVP). - README: annotations/icons/options section. ADR_0006 marked Landed. 172 test examples; verified on GHC 9.10.3 and 9.14.1; cabal check clean.
drshade
left a comment
There was a problem hiding this comment.
Review verdict: one small fix requested (icons dropped on parse), otherwise this is ready. Highlights before the fix:
- The 0.3.0.0 fold is the right call and I appreciate the correction — ADR 0006's "PVP minor" was my error: adding fields to exported records with exposed constructors breaks construction and exhaustive matching, which is exactly major territory. The smart constructors (
mk*Definition) are the correct structural fix so this class of change stops being breaking, and migrating all in-repo constructions (catching the latent incomplete constructions in the Unicode spec along the way) proves the pattern. - The
DefinitionOptionsdesign answers the ADR's API-shape concern well: one record for everything per-constructor,WithDescriptionbecomes a pure adapter with behavior preserved exactly (I tracedoptionsFromDescriptions+fieldDescsForagainst the old lookup semantics — identical, with scoped entries shadowing the global list), and constructor-scoped field descriptions fix a real wart.Lift-ing options values into the splice is the right mechanism. ContentAnnotatedas one added constructor instead of five optional fields minimizes churn; merge-into-object + parse-back-out matches the spec's wire shape, and the round-trip is tested.- Wire hygiene holds: every new field omitted when unset; the v0.2.0-anchored fixtures are untouched (only extended-set fixtures changed, and the
tools-list-extendedregeneration is legitimate — that server gained a tool).
172/172 locally; CI pending as I write, merge waits for green.
| <*> o .:? "description" | ||
| <*> o .:? "mimeType" | ||
| <*> o .:? "title" | ||
| <*> pure [] |
There was a problem hiding this comment.
The requested fix: this parser silently drops icons. ToJSON emits them but FromJSON reconstructs with pure [] — so any ContentResourceLink (or ResourceDefinition) that round-trips through JSON loses its icons, and the Content FromJSON instance now decodes to a value that is not Eq-equal to what was encoded. The cause is Icon lacking a FromJSON instance; the fix is small:
instance FromJSON Icon where
parseJSON = withObject "Icon" $ \o -> Icon
<$> o .: "src"
<*> o .:? "mimeType"
<*> o .:? "sizes" .!= []
-- and here:
<*> o .:? "icons" .!= []plus a round-trip test for a resource link carrying an icon (the annotated-content round-trip test is the template).
Icon gains a FromJSON instance and ResourceDefinition's parser reads the icons field instead of reconstructing with an empty list, so ContentResourceLink (and ResourceDefinition) values round-trip through JSON Eq-equal to what was encoded. Round-trip test added for a resource link carrying an icon. 173 test examples.
|
Fixed in 584c90e, exactly as suggested: |
Maintainer decision: 0.2.0.0 was published hours before this line landed and was never adopted, so it is deprecated on Hackage rather than answered with a major bump — 0.2 stays the version of the refactor, and 0.2.0.1 supersedes 0.2.0.0 in place, knowingly including changes that would ordinarily demand a major version. CHANGELOG, ADR statuses and ROADMAP updated to record the reasoning.
|
Heads-up on a versioning change since your review: the pending release is renumbered 0.3.0.0 → 0.2.0.1 (ec10207). Maintainer decision (from Tom directly): 0.2.0.0 is being deprecated on Hackage — it was published hours before this line landed and has zero adopters — so rather than burning a major version on a release nobody used, 0.2 remains the version of the refactor and 0.2.0.1 supersedes 0.2.0.0 in place, knowingly carrying changes that would ordinarily demand a major bump. The strict-PVP analysis you endorsed was correct against a live 0.2.0.0; with it deprecated and unadopted, the letter of the PVP is deliberately traded for not stranding the version number. The CHANGELOG entry, ADR statuses and ROADMAP all record this reasoning, and the deprecation note tells any hypothetical 0.2.0.0 pinner to move to 0.2.0.1. |
drshade
left a comment
There was a problem hiding this comment.
Approved. The icons round-trip fix on 584c90e is exactly as requested (FromJSON Icon + .:? "icons" .!= [] + round-trip test). 173/173 tests, CI green, v0.2.0-anchored fixtures untouched. Merging — roadmap Batch 1 (ADR 0005 + 0006) is complete. Note: the version number will be adjusted in a follow-up per Tom's decision — 0.2.0.0 has been deprecated on Hackage (zero adopters), so the pending release becomes 0.2.0.1 superseding it in place rather than the 0.3.0.0 fold.
Implements ADR 0006, completing roadmap Batch 1.
What it adds
ToolAnnotations(2025-03-26+):readOnlyHint/destructiveHint/idempotentHint/openWorldHint+ title onToolDefinition— the hints that drive client permission UX. All advisory, all unset by default.Iconlists (2025-11-25+) on tool/prompt/resource/template definitions.Annotations(audience/priority/lastModified) via a newContentAnnotatedwrapper whose annotations merge into the inner block's JSON object and parse back out — one added constructor instead of an optional field on everyContentvariant, minimizing churn for constructors of content.DefinitionOptions+WithOptionsderivations for all five families — the ADR asked for one options mechanism decided carefully so we don't accreteWithXvariants: a single per-constructor record carrying description, title, icons, tool annotations, and constructor-scoped field descriptions (two constructors can finally describe a same-named field differently; the flatWithDescriptionAPI is unchanged and now an adapter over the same generic path). Options values are lifted into the generated code viaDeriveLift. For output-typed tools, an entry keyed by the output type's name supplies its field descriptions.Versioning — the ADR's "PVP minor" guess was wrong
Extending the exported definition datatypes and
Contentis a major change under strict PVP (constructions and exhaustive matches break), so the pending 0.2.1.0 line folds into 0.3.0.0 — CHANGELOG notes the fold, ADR_0005's landed version and the ROADMAP are updated. To stop this class of change from being breaking again, new smart constructors (mkToolDefinition,mkPromptDefinition,mkResourceDefinition,mkResourceTemplateDefinition) build definitions from required fields only; all in-repo manual constructions are migrated (which also fixed latent incomplete-record constructions in the Unicode spec).Wire compatibility is unaffected: every new field is omitted from JSON when unset — the v0.2.0-anchored corpus fixtures pass untouched.
Verification
ContentAnnotatedmerge + round-trip, options-carried metadata, per-constructor description scoping (including the no-leak case), uncustomized-constructor defaults, unchanged dispatch.annotated_probe(derived viaWithOptions, so the corpus pins the options wire shape) with call cases in both eras;tools-list-extendedregenerated deliberately.cabal checkclean.