Conversation
There was a problem hiding this comment.
Pull Request Overview
Initial implementation of an MCP (Model Context Protocol) server for the text2synth project, adding the ability to expose synthesizer control functions as MCP tools.
- Adds MCP server functionality with dynamic function generation from Pydantic models
- Implements synthesizer state management with MIDI output capabilities
- Refactors file loading to support both file paths and file objects
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/text2synth/mcp_server.py | Core MCP server implementation with function factory and synthesizer control |
| src/text2synth/state.py | Adds file object loading support and fixes linting issue |
| tests/test_mcp_server.py | Test suite for the function factory functionality |
| pyproject.toml | Adds MCP dependency and updates test coverage configuration |
| .python-version | Downgrades Python version from 3.13.5 to 3.12.11 |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/text2synth/mcp_server.py
Outdated
| Function factory from a Pydantic model. Created function can be then | ||
| exposed as MCP tools through FastMCP. | ||
|
|
||
| The created function will have a signature that is kw-only, wich one |
There was a problem hiding this comment.
Corrected spelling of 'wich' to 'which'.
| The created function will have a signature that is kw-only, wich one | |
| The created function will have a signature that is kw-only, which one |
src/text2synth/mcp_server.py
Outdated
| The created function will have a signature that is kw-only, wich one | ||
| keyword per field. Critically, the generated function will have a docstring | ||
| and __signature__ that maps the fields, type and description for the | ||
| pydantic class. This exposes the right info when exposting |
There was a problem hiding this comment.
Corrected spelling of 'exposting' to 'exposing'.
| pydantic class. This exposes the right info when exposting | |
| pydantic class. This exposes the right info when exposing |
src/text2synth/mcp_server.py
Outdated
| # Note: None is for explicit -> None, not for no annotation | ||
| sig = inspect.Signature(params, return_annotation=None) | ||
|
|
||
| # FIXME: missing __annotations__ |
There was a problem hiding this comment.
The FIXME comment indicates incomplete implementation. The function is missing annotations which should be set for proper introspection.
| # FIXME: missing __annotations__ | |
| # Set __annotations__ for proper introspection | |
| generated_func.__annotations__ = {**model_class.__annotations__, 'return': None} |
src/text2synth/mcp_server.py
Outdated
| #print("=" * 80) | ||
| #print(update_synth.__doc__) | ||
| #print("=" * 80) | ||
| #print(update_synth.__signature__) | ||
| #print("=" * 80) |
There was a problem hiding this comment.
Commented-out debug code should be removed before merging to production.
| #print("=" * 80) | |
| #print(update_synth.__doc__) | |
| #print("=" * 80) | |
| #print(update_synth.__signature__) | |
| #print("=" * 80) |
src/text2synth/mcp_server.py
Outdated
| logger = logging.getLogger(__name__) | ||
| logging.basicConfig(level=logging.DEBUG) | ||
|
|
||
| logger.debug("YOLO") |
There was a problem hiding this comment.
Debug log message 'YOLO' should be replaced with a meaningful message or removed.
| logger.debug("YOLO") | |
| logger.debug("Starting MCP server") |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/text2synth/mcp_server.py
Outdated
| # Note: None is for explicit -> None, not for no annotation | ||
| sig = inspect.Signature(params, return_annotation=None) | ||
|
|
||
| # FIXME: missing __annotations__ |
There was a problem hiding this comment.
There's a FIXME comment indicating missing annotations implementation. This should be addressed to ensure proper type introspection for the generated functions.
| # FIXME: missing __annotations__ | |
| # Set __annotations__ for proper type introspection | |
| annotations = {} | |
| for name, param in sig.parameters.items(): | |
| if param.annotation is not inspect.Parameter.empty: | |
| annotations[name] = param.annotation | |
| annotations['return'] = None | |
| generated_func.__annotations__ = annotations |
src/text2synth/mcp_server.py
Outdated
|
|
||
|
|
||
| def update_synth_state(**kw): | ||
| print(f"Update args {kw}") |
There was a problem hiding this comment.
Using print() for debugging instead of the configured logger. Replace with LOGGER.debug(f'Update args {kw}') to maintain consistent logging.
| print(f"Update args {kw}") | |
| LOGGER.debug(f"Update args {kw}") |
tests/test_mcp_server.py
Outdated
| a: int = Field(description="first arg") | ||
| b: int = Field(description="second arg") | ||
|
|
||
| def func_ref(a: Optional[int] = None, b : Optional[int] = None) -> None: |
There was a problem hiding this comment.
Inconsistent spacing around colon. Should be b: Optional[int] = None to match Python style conventions.
| def func_ref(a: Optional[int] = None, b : Optional[int] = None) -> None: | |
| def func_ref(a: Optional[int] = None, b: Optional[int] = None) -> None: |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/text2synth/mcp_server.py:1
- Inconsistent encoding between from_path (ascii) and original implementation (utf-8). This could cause decoding errors if the file contains non-ASCII characters.
import inspect
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/text2synth/mcp_server.py
Outdated
| ---------- | ||
| model_class : type[BaseModel] | ||
| The Pydantic model class to generate a function from | ||
| wrapped_func : |
There was a problem hiding this comment.
Parameter type annotation is missing for wrapped_func parameter in the docstring. Should specify 'Callable' as the type.
| wrapped_func : | |
| wrapped_func : Callable |
| # Note: None is for explicit -> None, not for no annotation | ||
| sig = inspect.Signature(params, return_annotation=None) | ||
|
|
||
| annotations = {} |
There was a problem hiding this comment.
Variable 'annotations' is redefined unnecessarily. It was already defined at line 112 and could be reused instead of creating a new dictionary.
| annotations = {} | |
| # Reuse the existing 'annotations' dictionary defined earlier |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| JU06AState | ||
| Complete synth state with all parameters set | ||
| """ | ||
| with open(path, 'r', encoding='ascii') as fp: |
There was a problem hiding this comment.
The encoding changed from 'utf-8' to 'ascii' which is more restrictive and could cause decoding errors if the file contains non-ASCII characters. Consider keeping 'utf-8' encoding unless there's a specific requirement for ASCII-only content.
| with open(path, 'r', encoding='ascii') as fp: | |
| with open(path, 'r', encoding='utf-8') as fp: |
| constraints.append(f"Possible values: {', '.join(enum_values)}") | ||
|
|
||
| # Check for numeric constraints (ge, le, gt, lt) | ||
| elif annotation in (int, float) or (get_origin(annotation) in (int, float)): |
There was a problem hiding this comment.
The condition get_origin(annotation) in (int, float) will always be False because get_origin() returns generic types like typing.Union or None, not basic types like int or float. This branch may not work as expected for generic type annotations.
| range_parts[0] = m.ge | ||
|
|
||
| if range_parts: | ||
| constraints.append(f"Range: {', '.join(str(range_parts[k]) for k in (0, 1))}") |
There was a problem hiding this comment.
The code assumes both keys 0 and 1 exist in range_parts, but only the keys that were found (ge=0, le=1) are added. This will cause a KeyError if only one constraint is present. Use range_parts.get(k) or check key existence first.
| constraints.append(f"Range: {', '.join(str(range_parts[k]) for k in (0, 1))}") | |
| range_strs = [] | |
| if 0 in range_parts: | |
| range_strs.append(f"ge={range_parts[0]}") | |
| if 1 in range_parts: | |
| range_strs.append(f"le={range_parts[1]}") | |
| constraints.append(f"Range: {', '.join(range_strs)}") |
src/text2synth/mcp_server.py
Outdated
|
|
||
|
|
||
| if __name__ == '__main__': | ||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
The variable logger is defined but never used. The code uses the global LOGGER constant instead. Either remove this unused variable or use it consistently throughout the module.
| logger = logging.getLogger(__name__) |
This is a slightly cleaned up version of what I demoed no 10th October AI tinkerers day.