Skip to content

Conversation

KKonstantinov
Copy link
Contributor

@KKonstantinov KKonstantinov commented Sep 17, 2025

This PR adds Icons type support for modelcontextprotocol/modelcontextprotocol#973 (PR), which exposed additional metadata for Implementations, Resources, Tools and Prompts.

Motivation and Context

This PR adds the missing Icons type and respective IconsSchema zod schema.

Further, a few improvements have been made to spec.types.test.ts:

  • Moved all check* methods into a Record object
  • Removed loading of the same file from the filesystem and then searching via a text search for check* in favour of an actual runtime check on whether the check exists in the Record object
  • The previous way of checking for a string in a loaded file would make the test pass even if the check* function was to be actually commented out (or if a variable with the same name exists anywhere).
  • Added an additional test so MISSING_SDK_TYPES will not have any value that is already having a check inside the Record

How Has This Been Tested?

Tests and type checks are passing, there are no changes to actual implementation.

Breaking Changes

No breaking changes - just adding the Icons type and test improvements.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

@KKonstantinov KKonstantinov requested review from a team and felixweinberger September 17, 2025 21:08
Comment on lines 453 to 455
it.each(typesToCheck)('%s should have a compatibility test', (type) => {
expect(sdkTypeChecks[type as keyof typeof sdkTypeChecks]).toBeDefined();
});
Copy link
Member

@cliffhall cliffhall Sep 17, 2025

Choose a reason for hiding this comment

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

This should be indented one level. (why don't we have prettier on this repo??? - rhetorical)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have ran prettier manually on the file, will commit that once the .strip topic is closed above

Copy link
Member

Choose a reason for hiding this comment

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

I don't see a resolve conversation button on the strip topic, so I'll dismiss my request. If you can push your prettier changes, will review again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be indented one level. (why don't we have prettier on this repo??? - rhetorical)

@cliffhall since you mentioned this... #953 😁

const specTypes = extractExportedTypes(fs.readFileSync(SPEC_TYPES_FILE, 'utf-8'));
const sdkTypes = extractExportedTypes(fs.readFileSync(SDK_TYPES_FILE, 'utf-8'));
const testSource = fs.readFileSync(__filename, 'utf-8');
const typesToCheck = specTypes.filter(type => !MISSING_SDK_TYPES.includes(type));
Copy link
Member

Choose a reason for hiding this comment

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

nice one

@cliffhall cliffhall self-requested a review September 18, 2025 23:15
Copy link
Member

@cliffhall cliffhall left a comment

Choose a reason for hiding this comment

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

Just the prettier changes.

@KKonstantinov
Copy link
Contributor Author

Tests on the main branch are now failing because of the "sizes" property change in the spec.

This PR now sorts that too.

cc @ihrpr @cliffhall

Copy link
Member

@cliffhall cliffhall left a comment

Choose a reason for hiding this comment

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

This lines up with the spec change that was just merged.

@felixweinberger felixweinberger added the improves spec compliance When a change improves ability of SDK users to comply with spec definition label Sep 29, 2025
@felixweinberger
Copy link
Contributor

Missed this PR while reviewing this quick CI fix: #981

Introduces a small conflict as that PR already replaces

    sizes: z.optional(z.array(z.string())),

@KKonstantinov
Copy link
Contributor Author

Missed this PR while reviewing this quick CI fix: #981

Introduces a small conflict as that PR already replaces

    sizes: z.optional(z.array(z.string())),

hehe! Was just merging it. Merge pushed.

Copy link
Contributor

@ihrpr ihrpr left a comment

Choose a reason for hiding this comment

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

thanks so much for working on this. left some comments on comments but otherwise looks good!

src/types.ts Outdated
/**
* An optional list of icons for this implementation.
* This can be used by clients to display the implementation in a user interface.
* Each icon should have a `kind` property that specifies whether it is a data representation or a URL source, a `src` property that points to the icon file or data representation, and may also include a `mimeType` and `sizes` property.
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is the kind coming from? I think it's fine not explain too much what IconSchema should look like as we have a definition right above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the comment with the latest from the spec.

src/types.ts Outdated
* This can be used by clients to display the implementation in a user interface.
* Each icon should have a `kind` property that specifies whether it is a data representation or a URL source, a `src` property that points to the icon file or data representation, and may also include a `mimeType` and `sizes` property.
* The `mimeType` property should be a valid MIME type for the icon file, such as "image/png" or "image/svg+xml".
* The `sizes` property should be a string that specifies one or more sizes at which the icon file can be used, such as "48x48" or "any" for scalable formats like SVG.
Copy link
Contributor

Choose a reason for hiding this comment

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

oh, this changed to an array here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with the same comments from the main spec

@ihrpr
Copy link
Contributor

ihrpr commented Sep 30, 2025

@KKonstantinov if it's not too much to ask, please can we also add this: modelcontextprotocol/modelcontextprotocol#1565 🙏

@KKonstantinov
Copy link
Contributor Author

@KKonstantinov if it's not too much to ask, please can we also add this: modelcontextprotocol/modelcontextprotocol#1565 🙏

Done!

Copy link
Contributor

@ihrpr ihrpr left a comment

Choose a reason for hiding this comment

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

Thank you!

@ihrpr ihrpr merged commit 856d9ec into modelcontextprotocol:main Sep 30, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improves spec compliance When a change improves ability of SDK users to comply with spec definition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants