Skip to content

Conversation

@leomonan
Copy link
Contributor

@leomonan leomonan commented May 17, 2025

Please see discusion in:
#31

Summary

This PR addresses two main issues in the chroma-mcp implementation:

Removes Optional type annotations that were causing parameter recognition problems in Cursor MCP
Enhances chroma_add_documents function to properly validate document IDs

Changes
Type Annotation Fixes

Removed all Optional type annotations from parameters across the codebase
Parameters now use direct type hints with default values (e.g., Dict = None instead of Optional[Dict] = None)
This resolves issues where Cursor MCP would incorrectly treat parameters as undefined, particularly with metadata parameters

Document ID Validation Improvements

Added strict validation for the ids parameter in chroma_add_documents
Ensures that ids cannot be null or empty strings
This prevents the limitation where a collection could only store a single document when IDs weren't properly provided

Auto-generated IDs Removal

Removed code that automatically generated IDs
Forces explicit ID provision to ensure better document management and avoid unexpected behavior
Improves consistency with Chroma's expected usage patterns

Motivation

When using chroma-mcp with Cursor, I encountered parameter recognition issues where the MCP framework would fail to properly process Optional-typed parameters. Additionally, the automatic ID generation was causing confusion and potential data loss when users weren't explicitly providing document identifiers.

These changes improve reliability and consistency across different environments while enforcing better practices for document management.
Testing

Tested in multiple Cursor environments to verify parameter recognition works correctly, and that the ID validation properly enforces the required constraints.

@leomonan
Copy link
Contributor Author

#32

Fix: Empty Collection List Handling

This PR modifies the chroma_list_collections function to return a special marker value of ["NO_COLLECTIONS_FOUND"] instead of an empty list when no collections exist in the database.
Problem

When the Chroma database is first initialized and empty, the previous implementation returned an empty list ([]). In practice, we discovered that AI clients do not correctly interpret this empty result, leading to confusion and repetitive API calls as they try to determine if there was an error or if the result truly means "no collections exist."
Solution

By returning a special marker value (["NO_COLLECTIONS_FOUND"]) instead of an empty list, we provide a clear and explicit indication that:

The API call was successful
The database was queried correctly
No collections exist at this time

This improves the interaction between AI systems and the Chroma database, reducing unnecessary API calls and making the response more semantically meaningful to AI clients.
Additional Changes

Improved docstring documentation to specify the new return value behavior
Added comments explaining the empty check logic
Simplified parameter handling by removing Optional type hints that were causing MCP parameter identification issues

Copy link
Collaborator

@jairad26 jairad26 left a comment

Choose a reason for hiding this comment

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

i did a little more digging since it seems strange that all mcp servers wouldn’t be able to have optional parameters. i can understand that maybe Optional is not directly supported. It turns out you have to specify optional parameters verbosely, ie limit: int | None
see here
https://github.com/modelcontextprotocol/servers/blob/dc7e4286d6145640fb88c2dceaa109a6ecaeee1e/src/fetch/src/mcp_server_fetch/server.py#L66
then, you do not need to make edits to the limit and offset tests.

Thank you for your continued work on this! will definitely merge and release this when it’s done, just want to make sure it is in a good state

@jairad26
Copy link
Collaborator

hi @leomonan just wanted to follow up on this, as i do want to give you the proper credit as a contributor for these changes. Instead of removing Optional entirely, if you could replace it with with | None as I described above we can merge this in.

@leomonan
Copy link
Contributor Author

leomonan commented Jun 5, 2025

hi @leomonan just wanted to follow up on this, as i do want to give you the proper credit as a contributor for these changes. Instead of removing Optional entirely, if you could replace it with with | None as I described above we can merge this in.

Sure, That sounds like a good idea.

@leomonan leomonan requested a review from jairad26 June 5, 2025 06:41
@jairad26 jairad26 merged commit 0aaaa13 into chroma-core:main Jun 11, 2025
2 of 4 checks passed
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.

2 participants