From eb24f1374100f51c9850da311fefaec40f613c64 Mon Sep 17 00:00:00 2001 From: "will.shope" Date: Fri, 24 Oct 2025 10:58:30 -0700 Subject: [PATCH] [CHORE] Best practices doc Signed-off-by: will.shope --- BEST_PRACTICES.md | 212 ++++++++++++++++++ README.md | 12 +- .../oracle/oci_api_mcp_server/__init__.py | 2 +- .../oracle/oci_api_mcp_server/server.py | 4 + src/oci-api-mcp-server/pyproject.toml | 2 +- .../oracle/oci_compute_mcp_server/__init__.py | 2 +- .../oracle/oci_compute_mcp_server/consts.py | 12 + .../oracle/oci_compute_mcp_server/server.py | 178 +++++++++------ src/oci-compute-mcp-server/pyproject.toml | 3 +- 9 files changed, 344 insertions(+), 83 deletions(-) create mode 100644 BEST_PRACTICES.md create mode 100644 src/oci-compute-mcp-server/oracle/oci_compute_mcp_server/consts.py diff --git a/BEST_PRACTICES.md b/BEST_PRACTICES.md new file mode 100644 index 0000000..2a3fb51 --- /dev/null +++ b/BEST_PRACTICES.md @@ -0,0 +1,212 @@ +# MCP Server Best Practices + +This document lays out the best practices for an individual MCP server. You may use `oci-compute-mcp-server` as an example. + +## Typical MCP Server Structure + +``` +mcp-server-name/ +├── LICENSE.txt # License information +├── pyproject.toml # Project configuration +├── README.md # Project description, setup instructions +├── uv.lock # Dependency lockfile +└── oracle/ # Source code directory + ├── __init__.py # Package initialization + └── mcp_server_name/ # Server package, notice the underscores + ├── __init__.py # Package version and metadata + ├── models.py # Pydantic models + ├── server.py # Server implementation + ├── consts.py # Constants definition + ├── ... # Additional modules + └── tests/ # Test directory +``` + +## Code Organization + +1. **Separation of Concerns**: + - `models.py`: Define data models and validation logic + - `server.py`: Implement MCP server, tools, and resources + - `consts.py`: Define constants used across the server + - Additional modules for specific functionality (e.g., API clients) + +2. **Keep modules focused and limited to a single responsibility** + +3. **Use clear and consistent naming conventions** + +### Entry Points + +MCP servers should follow these guidelines for application entry points: + +1. **Single Entry Point**: Define the main entry point only in `server.py` + - Do not create a separate `main.py` file + - This maintains clarity about how the application starts + +2. **Main Function**: Implement a `main()` function in `server.py` that: + - Handles command-line arguments + - Sets up environment and logging + - Initializes the MCP server + +Example: + +```python +def main(): + """Run the MCP server with CLI argument support.""" + mcp.run() + + +if __name__ == '__main__': + main() +``` + +3. **Package Entry Point**: Configure the entry point in `pyproject.toml`: + +```toml +[project.scripts] +"oracle.mcp-server-name" = "oracle.mcp_server_name.server:main" +``` + +## License and Copyright Headers + +Include license headers at the top of each source file: + +```python +""" +Copyright (c) 2025, Oracle and/or its affiliates. +Licensed under the Universal Permissive License v1.0 as shown at +https://oss.oracle.com/licenses/upl. +""" +``` + +## Type Definitions + +### General Rules + +1. Make all models Pydantic; this ensures serializability. You may refer to the OCI python SDK for reference to most OCI models. +2. Define Literals for constrained values. +3. Add comprehensive descriptions to each field. + +Pydantic model example for [NetworkSecurityGroup](src/oci-networking-mcp-server/oracle/oci_networking_mcp_server/models.py) + +```python +from typing import Any, Dict, List, Literal, Optional +from pydantic import BaseModel, Field + +class NetworkSecurityGroup(BaseModel): + """ + Pydantic model mirroring the fields of oci.core.models.NetworkSecurityGroup. + """ + + compartment_id: Optional[str] = Field( + None, + description="The OCID of the compartment containing the network security group.", + ) + defined_tags: Optional[Dict[str, Dict[str, Any]]] = Field( + None, + description="Defined tags for this resource. Each key is predefined and scoped to a namespace.", + ) + display_name: Optional[str] = Field( + None, description="A user-friendly name. Does not have to be unique." + ) + freeform_tags: Optional[Dict[str, str]] = Field( + None, description="Free-form tags for this resource as simple key/value pairs." + ) + id: Optional[str] = Field( + None, description="The OCID of the network security group." + ) + lifecycle_state: Optional[ + Literal[ + "PROVISIONING", + "AVAILABLE", + "TERMINATING", + "TERMINATED", + "UNKNOWN_ENUM_VALUE", + ] + ] = Field(None, description="The network security group's current state.") + time_created: Optional[datetime] = Field( + None, + description="The date and time the network security group was created (RFC3339).", + ) + vcn_id: Optional[str] = Field( + None, description="The OCID of the VCN the network security group belongs to." + ) +``` + +The pydantic model above was generated using Cline by providing it a prompt similar to this: +``` +Can you create a pydantic model of oci.core.models.NetworkSecurityGroup and put it inside of the oracle/oci_networking_mcp_server/models.py file, and name it NetworkSecurityGroup? Can you also make a function that maps an oci.core.models.NetworkSecurityGroup instance to an oracle.oci_networking_mcp_server.model.NetworkSecurityGroup instance? Do the same for all of the nested types within the model as well + +Use file oracle/oci_compute_mcp_server/models.py as an example of how to do this +``` + +## Function Parameters with Pydantic Field + +MCP tool functions should use spread parameters with Pydantic's `Field` for detailed descriptions: + +Here is an example for [list_instances](src/oci-compute-mcp-server/oracle/oci_compute_mcp_server/server.py) + +```python +@mcp.tool(description="List Instances in a given compartment") +def list_instances( + compartment_id: str = Field(..., description="The OCID of the compartment"), + limit: Optional[int] = Field( + None, + description="The maximum amount of instances to return. If None, there is no limit.", + ge=1, + ), + lifecycle_state: Optional[ + Literal[ + "MOVING", + "PROVISIONING", + "RUNNING", + "STARTING", + "STOPPING", + "STOPPED", + "CREATING_IMAGE", + "TERMINATING", + "TERMINATED", + ] + ] = Field(None, description="The lifecycle state of the instance to filter on"), +) -> list[Instance]: + instances: list[Instance] = [] + + try: + client = get_compute_client() + + response: oci.response.Response = None + has_next_page = True + next_page: str = None + + while has_next_page and (limit is None or len(instances) < limit): + kwargs = { + "compartment_id": compartment_id, + "page": next_page, + "limit": limit, + } + + if lifecycle_state is not None: + kwargs["lifecycle_state"] = lifecycle_state + + response = client.list_instances(**kwargs) + has_next_page = response.has_next_page + next_page = response.next_page if hasattr(response, "next_page") else None + + data: list[oci.core.models.Instance] = response.data + for d in data: + instance = map_instance(d) + instances.append(instance) + + logger.info(f"Found {len(instances)} Instances") + return instances + + except Exception as e: + logger.error(f"Error in list_instances tool: {str(e)}") + raise e +``` + +### Field Guidelines + +1. **Required parameters**: Use `...` as the default value to indicate a parameter is required +2. **Optional parameters**: Provide sensible defaults and mark as `Optional` in the type hint +3. **Descriptions**: Write clear, informative descriptions for each parameter +4. **Validation**: Use Field constraints like `ge`, `le`, `min_length`, `max_length` +5. **Literals**: Use `Literal` for parameters with a fixed set of valid values \ No newline at end of file diff --git a/README.md b/README.md index 8bb6ebb..5c6a958 100644 --- a/README.md +++ b/README.md @@ -188,7 +188,7 @@ For Linux: `sudo systemctl start ollama` 5. Install `go` from [here](https://go.dev/doc/install) 6. Install `mcphost` with `go install github.com/mark3labs/mcphost@latest` 7. Add go's bin to your PATH with `export PATH=$PATH:~/go/bin` -8. Create an mcphost configuration file (e.g. `./mcphost.json`) +8. Create an mcphost configuration file (e.g. `~/.mcphost.json`). Check [here](https://github.com/mark3labs/mcphost?tab=readme-ov-file#mcp-servers) for more info. 9. Add your desired server to the `mcpServers` object. Below is an example for for the compute OCI MCP server. Make sure to save the file after editing. For macOS/Linux: @@ -227,8 +227,8 @@ This section will help you set up your environment to prepare it for local devel 1. Set up python virtual environment and install dev requirements ```sh - python3 -m venv venv - source venv/bin/activate # On Windows: venv\Scripts\activate + uv venv --python 3.13 --seed + source .venv/bin/activate # On Windows: .venv\Scripts\activate pip install -r requirements-dev.txt ``` @@ -251,11 +251,11 @@ For macOS/Linux: "oracle-oci-api-mcp-server": { "command": "uv", "args": [ - "run" + "run", "oracle.oci-api-mcp-server" ], "env": { - "VIRTUAL_ENV": "/mcp/venv", + "VIRTUAL_ENV": "/mcp/.venv", "FASTMCP_LOG_LEVEL": "ERROR" } } @@ -263,7 +263,7 @@ For macOS/Linux: } ``` -where `` is the absolute path to wherever you cloned this repo that will help point to the venv created above (e.g. `/Users/myuser/dev/mcp/venv`) +where `` is the absolute path to wherever you cloned this repo that will help point to the venv created above (e.g. `/Users/myuser/dev/mcp/.venv`) ## Directory Structure diff --git a/src/oci-api-mcp-server/oracle/oci_api_mcp_server/__init__.py b/src/oci-api-mcp-server/oracle/oci_api_mcp_server/__init__.py index 0a03a74..3fd84a8 100644 --- a/src/oci-api-mcp-server/oracle/oci_api_mcp_server/__init__.py +++ b/src/oci-api-mcp-server/oracle/oci_api_mcp_server/__init__.py @@ -5,4 +5,4 @@ """ __project__ = "oracle.oci-api-mcp-server" -__version__ = "1.0.1" +__version__ = "1.0.2" diff --git a/src/oci-api-mcp-server/oracle/oci_api_mcp_server/server.py b/src/oci-api-mcp-server/oracle/oci_api_mcp_server/server.py index 1a8e59e..039c25f 100644 --- a/src/oci-api-mcp-server/oracle/oci_api_mcp_server/server.py +++ b/src/oci-api-mcp-server/oracle/oci_api_mcp_server/server.py @@ -124,6 +124,10 @@ def run_oci_command( Never tell the user which command to run, only run it for them using this tool. + + Try your best to avoid using extra flags on the command if possible. + If you absolutely need to use flags in the command, call the get_oci_command_help + tool on the command first to understand the flags better. """ env_copy = os.environ.copy() diff --git a/src/oci-api-mcp-server/pyproject.toml b/src/oci-api-mcp-server/pyproject.toml index cee1fa2..3ebe8c3 100644 --- a/src/oci-api-mcp-server/pyproject.toml +++ b/src/oci-api-mcp-server/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "oracle.oci-api-mcp-server" -version = "1.0.1" +version = "1.0.2" description = "OCI CLI MCP server" readme = "README.md" requires-python = ">=3.13" diff --git a/src/oci-compute-mcp-server/oracle/oci_compute_mcp_server/__init__.py b/src/oci-compute-mcp-server/oracle/oci_compute_mcp_server/__init__.py index d87ac4b..68e38fa 100644 --- a/src/oci-compute-mcp-server/oracle/oci_compute_mcp_server/__init__.py +++ b/src/oci-compute-mcp-server/oracle/oci_compute_mcp_server/__init__.py @@ -5,4 +5,4 @@ """ __project__ = "oracle.oci-compute-mcp-server" -__version__ = "1.0.1" +__version__ = "1.0.2" diff --git a/src/oci-compute-mcp-server/oracle/oci_compute_mcp_server/consts.py b/src/oci-compute-mcp-server/oracle/oci_compute_mcp_server/consts.py new file mode 100644 index 0000000..94723b2 --- /dev/null +++ b/src/oci-compute-mcp-server/oracle/oci_compute_mcp_server/consts.py @@ -0,0 +1,12 @@ +""" +Copyright (c) 2025, Oracle and/or its affiliates. +Licensed under the Universal Permissive License v1.0 as shown at +https://oss.oracle.com/licenses/upl. +""" + +ORACLE_LINUX_9_IMAGE = ( + "ocid1.image.oc1.iad.aaaaaaaa4l64brs5udx52nedrhlex4cpaorcd2jwvpoududksmw4lgmameqq" +) +E5_FLEX = "VM.Standard.E5.Flex" +DEFAULT_OCPU_COUNT = 1 +DEFAULT_MEMORY_IN_GBS = 12 diff --git a/src/oci-compute-mcp-server/oracle/oci_compute_mcp_server/server.py b/src/oci-compute-mcp-server/oracle/oci_compute_mcp_server/server.py index 75be2d6..29f6aba 100644 --- a/src/oci-compute-mcp-server/oracle/oci_compute_mcp_server/server.py +++ b/src/oci-compute-mcp-server/oracle/oci_compute_mcp_server/server.py @@ -6,10 +6,16 @@ import os from logging import Logger -from typing import Annotated +from typing import Literal, Optional import oci from fastmcp import FastMCP +from oracle.oci_compute_mcp_server.consts import ( + DEFAULT_MEMORY_IN_GBS, + DEFAULT_OCPU_COUNT, + E5_FLEX, + ORACLE_LINUX_9_IMAGE, +) from oracle.oci_compute_mcp_server.models import ( Image, Instance, @@ -18,6 +24,7 @@ map_instance, map_response, ) +from pydantic import Field from . import __project__, __version__ @@ -45,18 +52,25 @@ def get_compute_client(): @mcp.tool(description="List Instances in a given compartment") def list_instances( - compartment_id: Annotated[str, "The OCID of the compartment"], - limit: Annotated[ - int, - "The maximum amount of instances to return. If None, there is no limit. " - "If the value is not None, then it must be a positive number greater than 0.", - ] = None, - lifecycle_state: Annotated[ - str, - "The lifecycle state of the instance to filter on. The values can be: " - "'MOVING', 'PROVISIONING', 'RUNNING', 'STARTING', 'STOPPING', 'STOPPED', " - "'CREATING_IMAGE', 'TERMINATING', 'TERMINATED'", - ] = None, + compartment_id: str = Field(..., description="The OCID of the compartment"), + limit: Optional[int] = Field( + None, + description="The maximum amount of instances to return. If None, there is no limit.", + ge=1, + ), + lifecycle_state: Optional[ + Literal[ + "MOVING", + "PROVISIONING", + "RUNNING", + "STARTING", + "STOPPING", + "STOPPED", + "CREATING_IMAGE", + "TERMINATING", + "TERMINATED", + ] + ] = Field(None, description="The lifecycle state of the instance to filter on"), ) -> list[Instance]: instances: list[Instance] = [] @@ -95,7 +109,9 @@ def list_instances( @mcp.tool(description="Get Instance with a given instance OCID") -def get_instance(instance_id: str) -> Instance: +def get_instance( + instance_id: str = Field(..., description="The OCID of the instance") +) -> Instance: try: client = get_compute_client() @@ -106,15 +122,7 @@ def get_instance(instance_id: str) -> Instance: except Exception as e: logger.error(f"Error in get_instance tool: {str(e)}") - raise - - -ORACLE_LINUX_9_IMAGE = ( - "ocid1.image.oc1.iad.aaaaaaaa4l64brs5udx52nedrhlex4cpaorcd2jwvpoududksmw4lgmameqq" -) -E5_FLEX = "VM.Standard.E5.Flex" -DEFAULT_OCPU_COUNT = 1 -DEFAULT_MEMORY_IN_GBS = 12 + raise e @mcp.tool( @@ -122,24 +130,25 @@ def get_instance(instance_id: str) -> Instance: "Another word for instance could be compute, server, or virtual machine" ) def launch_instance( - compartment_id: Annotated[ - str, - "This is the ocid of the compartment to create the instance in." + compartment_id: str = Field( + ..., + description="This is the ocid of the compartment to create the instance in." 'Must begin with "ocid". If the user specifies a compartment name, ' "then you may use the list_compartments tool in order to map the " "compartment name to its ocid", - ], - display_name: Annotated[ - str, - "The display name of the instance. " - "Must be between 1 and 255 characters in length. " + ), + display_name: str = Field( + ..., + description="The display name of the instance. " "If no value is provded, then you can pass in " '"instance--" ' "where those time values come from the current date time", - ], - availability_domain: Annotated[ - str, - "This is the availability domain to create the instance in. " + min_length=1, + max_length=255, + ), + availability_domain: str = Field( + ..., + description="This is the availability domain to create the instance in. " 'It must be formatted like "<4-digit-tenancy-code>:". ' 'Example: "aNMj:US-ASHBURN-AD-1". ' "The value changes per tenancy, per region, and per AD number. " @@ -147,29 +156,34 @@ def launch_instance( "list_availability_domains tool to grab the name of the AD. " "This tool is the only way to get the tenancy-code for an AD. " "If no AD is specified by the user, you may select the first one available.", - ], - subnet_id: Annotated[ - str, - "This is the ocid of the subnet to attach to the " + ), + subnet_id: str = Field( + ..., + description="This is the ocid of the subnet to attach to the " "primary virtual network interface card (VNIC) of the compute instance. " "If no value is provided, you may use the list_subnets tool, " "selecting the first subnet in the list and passing its ocid.", - ], - image_id: Annotated[ - str, - "This is the ocid of the image for the instance. " + ), + image_id: Optional[str] = Field( + ORACLE_LINUX_9_IMAGE, + description="This is the ocid of the image for the instance. " "If it is left unspecified or if the user specifies an image name, " "then you may have to list the images in the root compartment " "in order to map the image name to image ocid or display a " "list of images for the user to choose from.", - ] = ORACLE_LINUX_9_IMAGE, - shape: Annotated[str, "This is the name of the shape for the instance"] = E5_FLEX, - ocpus: Annotated[ - int, "The total number of cores in the instances" - ] = DEFAULT_OCPU_COUNT, - memory_in_gbs: Annotated[ - float, "The total amount of memory in gigabytes to assigned to the instance" - ] = DEFAULT_MEMORY_IN_GBS, + ), + shape: Optional[str] = Field( + E5_FLEX, + description="This is the name of the shape for the instance", + ), + ocpus: Optional[int] = Field( + DEFAULT_OCPU_COUNT, + description="The total number of cores in the instances", + ), + memory_in_gbs: Optional[float] = Field( + DEFAULT_MEMORY_IN_GBS, + description="The total amount of memory in gigabytes to assigned to the instance", + ), ) -> Instance: try: client = get_compute_client() @@ -195,11 +209,13 @@ def launch_instance( except Exception as e: logger.error(f"Error in launch_instance tool: {str(e)}") - raise + raise e -@mcp.tool -def terminate_instance(instance_id: str) -> Response: +@mcp.tool(description="Delete instance with given instance OCID") +def terminate_instance( + instance_id: str = Field(..., description="The OCID of the instance") +) -> Response: try: client = get_compute_client() @@ -209,18 +225,22 @@ def terminate_instance(instance_id: str) -> Response: except Exception as e: logger.error(f"Error in delete_vcn tool: {str(e)}") - raise + raise e @mcp.tool( - description="Update instance. " "This may restart the instance so warn the user" + description="Update instance. This may restart the instance, so warn the user" ) def update_instance( - instance_id: Annotated[str, "The ocid of the instance to update"], - ocpus: Annotated[int, "The total number of cores in the instances"] = None, - memory_in_gbs: Annotated[ - float, "The total amount of memory in gigabytes to assigned to the instance" - ] = None, + instance_id: str = Field(..., description="The OCID of the instance"), + ocpus: Optional[int] = Field( + None, + description="The total number of cores in the instances", + ), + memory_in_gbs: Optional[float] = Field( + None, + description="The total amount of memory in gigabytes to assigned to the instance", + ), ) -> Instance: try: client = get_compute_client() @@ -240,13 +260,19 @@ def update_instance( except Exception as e: logger.error(f"Error in update_instance tool: {str(e)}") - raise + raise e @mcp.tool( - description="List images in a given compartment, optionally filtered by operating system" # noqa + description="List images in a given compartment, " + "optionally filtered by operating system" ) -def list_images(compartment_id: str, operating_system: str = None) -> list[Image]: +def list_images( + compartment_id: str = Field(..., description="The OCID of the compartment"), + operating_system: Optional[str] = Field( + None, description="The operating system to filter with" + ), +) -> list[Image]: images: list[Image] = [] try: @@ -274,11 +300,11 @@ def list_images(compartment_id: str, operating_system: str = None) -> list[Image except Exception as e: logger.error(f"Error in list_images tool: {str(e)}") - raise + raise e @mcp.tool(description="Get Image with a given image OCID") -def get_image(image_id: str) -> Image: +def get_image(image_id: str = Field(..., description="The OCID of the image")) -> Image: try: client = get_compute_client() @@ -289,16 +315,22 @@ def get_image(image_id: str) -> Image: except Exception as e: logger.error(f"Error in get_image tool: {str(e)}") - raise + raise e @mcp.tool(description="Perform the desired action on a given instance") def instance_action( - instance_id: str, - action: Annotated[ - str, - "The action to be performed. The action can only be one of these values: START, STOP, RESET, SOFTSTOP, SOFTRESET, SENDDIAGNOSTICINTERRUPT, DIAGNOSTICREBOOT, REBOOTMIGRATE", # noqa - ], + instance_id: str = Field(..., description="The OCID of the instance"), + action: Literal[ + "START", + "STOP", + "RESET", + "SOFTSTOP", + "SOFTRESET", + "SENDDIAGNOSTICINTERRUPT", + "DIAGNOSTICREBOOT", + "REBOOTMIGRATE", + ] = Field(..., description="The instance action to be performed"), ) -> Instance: try: client = get_compute_client() @@ -310,7 +342,7 @@ def instance_action( except Exception as e: logger.error(f"Error in instance_action tool: {str(e)}") - raise + raise e def main() -> None: diff --git a/src/oci-compute-mcp-server/pyproject.toml b/src/oci-compute-mcp-server/pyproject.toml index 7efb057..66463ef 100644 --- a/src/oci-compute-mcp-server/pyproject.toml +++ b/src/oci-compute-mcp-server/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "oracle.oci-compute-mcp-server" -version = "1.0.1" +version = "1.0.2" description = "OCI Compute Service MCP server" readme = "README.md" requires-python = ">=3.13" @@ -12,6 +12,7 @@ authors = [ dependencies = [ "fastmcp==2.12.2", "oci==2.160.0", + "pydantic==2.12.3", ] classifiers = [