Conversation
|
Caution Review failedThe pull request is closed. Summary by CodeRabbit
WalkthroughThis PR introduces a new MCP (Model Context Protocol) server for a Shirtify agent, updates project dependencies with range-based versioning constraints, and exposes new public APIs. The implementation includes a React-style graph-based sales agent with tool integration and an MCP HTTP transport wrapper. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant MCP as MCP Server<br/>(mcp_agent_wrapper)
participant Agent as ShirtifyAgent
participant LLM as LLM<br/>(React Graph)
participant Tools as Tools<br/>(_inventory, _send_email)
Client->>MCP: send_message(msg, context)
MCP->>MCP: Extract session_id from<br/>HTTP headers/params
MCP->>Agent: invoke(message, session_id)
Agent->>Agent: Generate/use session_id
Agent->>LLM: Execute React graph<br/>with message & thread_id
LLM->>LLM: Process via AGENT_INSTRUCTIONS
LLM->>Tools: Call tool if needed<br/>(_inventory or _send_email)
Tools-->>LLM: Return tool result
LLM-->>Agent: Return structured_response<br/>(ResponseFormat)
Agent->>Agent: Parse ResponseFormat<br/>(status: input_required/<br/>completed/error)
Agent-->>MCP: Return agent response<br/>Dict[status, content, ...]
MCP-->>Client: Return response string
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Heterogeneous changes span dependency updates, configuration, public API modifications, and new feature implementation across multiple files. New logic in Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
examples/mcp/tshirt_store_mcp/__main__.py (1)
4-4: Move load_dotenv() call inside main() to avoid side effects on import.Calling
load_dotenv()at module level executes immediately when the module is imported, which can cause unexpected side effects. It's better to call it within themain()function.Apply this diff to move the call:
-load_dotenv() - - def main(): + load_dotenv() print("Starting MCP server...")examples/mcp/tshirt_store_mcp/shirtify_agent.py (1)
117-136: Document that tools are mock implementations.Both
_inventory_tooland_send_email_toolare returning hardcoded strings instead of performing actual operations. This should be clearly documented to avoid confusion, especially since this is an example.Consider adding a note in the docstrings:
@staticmethod def _inventory_tool( color: str, size: str, ) -> str: """ Get the inventory of a specific color and size of T-shirt. + + Note: This is a mock implementation for demonstration purposes. + In production, this would query an actual inventory database. """ return f"100 {color} {size} T-shirts in stock" @staticmethod def _send_email_tool( email: str, subject: str, body: str, ) -> str: """ Send an email to a customer. + + Note: This is a mock implementation for demonstration purposes. + In production, this would send an actual email via SMTP or email service API. """ return f"Email sent to {email} with subject {subject} and body {body}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
examples/mcp/tshirt_store_mcp/__init__.py(1 hunks)examples/mcp/tshirt_store_mcp/__main__.py(1 hunks)examples/mcp/tshirt_store_mcp/mcp_agent_wrapper.py(1 hunks)examples/mcp/tshirt_store_mcp/shirtify_agent.py(1 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Format Python code with Black
Ensure code passes flake8 linting
Run mypy with the repository configuration for static typing
Run Bandit security checks using .bandit.yaml configuration
Use isort import conventions for import ordering
Add type hints to all function signatures
Follow PEP 8 naming (snake_case for variables/functions, PascalCase for classes)
Use try/except around code that may raise exceptions
Files:
examples/mcp/tshirt_store_mcp/__init__.pyexamples/mcp/tshirt_store_mcp/mcp_agent_wrapper.pyexamples/mcp/tshirt_store_mcp/shirtify_agent.pyexamples/mcp/tshirt_store_mcp/__main__.py
pyproject.toml
📄 CodeRabbit inference engine (AGENTS.md)
Manage dependencies with uv and declare them in pyproject.toml
Files:
pyproject.toml
🧬 Code graph analysis (1)
examples/mcp/tshirt_store_mcp/mcp_agent_wrapper.py (1)
examples/mcp/tshirt_store_mcp/shirtify_agent.py (2)
ShirtifyAgent(64-136)invoke(76-82)
🔇 Additional comments (2)
examples/mcp/tshirt_store_mcp/__init__.py (1)
1-1: LGTM!The package initialization correctly exposes the internal modules as public submodules.
pyproject.toml (1)
42-47: All package versions verified and no security vulnerabilities found.The specified versions exist: langchain 0.3.27, langchain-openai 0.3.35, langgraph 0.6.10, and mcp 1.13.0 are all published on PyPI. Security advisory checks show no active vulnerabilities for these specific versions.
|
happy PR #100 !!!!! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (9)
examples/mcp/tshirt_store_mcp/shirtify_agent.py (4)
68-74: Add error handling for agent initialization.The
create_react_agentcall can fail if the model is invalid, API keys are missing, or configuration is incorrect. Wrapping in try/except would provide clearer error messages on failure.As per coding guidelines.
76-82: Add error handling for graph invocation.The
graph.invoke()call can fail due to API errors, timeouts, or invalid state. Adding try/except would improve resilience.As per coding guidelines.
117-125: Add return type hint to _inventory_tool.The static method is missing a return type hint.
As per coding guidelines.
Apply this diff:
@staticmethod def _inventory_tool( color: str, size: str, -): +) -> str: """ Get the inventory of a specific color and size of T-shirt. """
127-136: Add return type hint to _send_email_tool.The static method is missing a return type hint.
As per coding guidelines.
Apply this diff:
@staticmethod def _send_email_tool( email: str, subject: str, body: str, -): +) -> str: """ Send an email to a customer. """examples/mcp/tshirt_store_mcp/mcp_agent_wrapper.py (3)
16-16: Consider deferring agent instantiation to avoid module-level failures.Instantiating
ShirtifyAgentat module level means any error during initialization (e.g., missing API keys, the invalid model name in line 65 of shirtify_agent.py) will cause the entire module import to fail. Consider lazy initialization for better error handling.
35-35: Add error handling for agent.invoke.The
agent.invoke()call can raise exceptions (network errors, API failures, etc.). Adding try/except would allow returning a proper error response instead of crashing.As per coding guidelines.
13-13: Fix the import to use relative import.The import should be relative since this module is within the same package. Absolute import will fail when the package is imported.
Apply this diff:
-from shirtify_agent import ShirtifyAgent +from .shirtify_agent import ShirtifyAgentexamples/mcp/tshirt_store_mcp/__main__.py (2)
9-9: Add error handling for MCP server startup.The
mcp.run()call can raise exceptions (e.g., port already in use, configuration errors). Adding try/except would provide clearer error messages on failure.As per coding guidelines.
2-2: Fix the import to use relative import.The import should be relative since this module is within the same package. Absolute import will fail when the package is imported.
Apply this diff:
-from mcp_agent_wrapper import mcp +from .mcp_agent_wrapper import mcp
🧹 Nitpick comments (4)
examples/mcp/tshirt_store_mcp/shirtify_agent.py (1)
1-8: Reorganize imports per isort conventions.Per coding guidelines, imports should be organized with standard library imports first, then a blank line, followed by third-party imports. Currently, they are mixed.
Apply this diff to reorganize:
from typing import Any, Dict, Literal from uuid import uuid4 + from langchain_core.runnables import RunnableConfig from langgraph.checkpoint.memory import MemorySaver from langgraph.graph.state import CompiledStateGraph from langgraph.prebuilt import create_react_agent from pydantic import BaseModelAs per coding guidelines.
examples/mcp/tshirt_store_mcp/mcp_agent_wrapper.py (2)
9-14: Reorganize imports per isort conventions.Imports should be grouped with standard library first, then third-party, then local/relative imports, with blank lines between groups.
Apply this diff:
from typing import Any, Dict + from mcp.server.fastmcp import Context, FastMCP from mcp.types import CallToolResult, TextContent -from shirtify_agent import ShirtifyAgent from starlette.requests import Request + +from .shirtify_agent import ShirtifyAgentAs per coding guidelines.
31-31: Consider using logging instead of print.Using Python's logging module instead of
print()would provide better control over log levels and output destinations.Example:
+import logging + +logger = logging.getLogger(__name__) + @mcp.tool() def send_message(message: str, context: Context) -> Dict[str, Any]: session_id: str | None = None try: request: Request = context.request_context.request # type: ignore session_id = request.query_params.get("session_id") except Exception: - print("No session ID found in request") + logger.warning("No session ID found in request") session_id = Noneexamples/mcp/tshirt_store_mcp/__main__.py (1)
1-2: Reorganize imports per isort conventions.Add a blank line between third-party and local imports for better organization.
Apply this diff:
from dotenv import load_dotenv + from .mcp_agent_wrapper import mcpAs per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.vscode/launch.json(1 hunks)examples/mcp/tshirt_store_mcp/__main__.py(1 hunks)examples/mcp/tshirt_store_mcp/mcp_agent_wrapper.py(1 hunks)examples/mcp/tshirt_store_mcp/shirtify_agent.py(1 hunks)pyproject.toml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Format Python code with Black
Ensure code passes flake8 linting
Run mypy with the repository configuration for static typing
Run Bandit security checks using .bandit.yaml configuration
Use isort import conventions for import ordering
Add type hints to all function signatures
Follow PEP 8 naming (snake_case for variables/functions, PascalCase for classes)
Use try/except around code that may raise exceptions
Files:
examples/mcp/tshirt_store_mcp/mcp_agent_wrapper.pyexamples/mcp/tshirt_store_mcp/__main__.pyexamples/mcp/tshirt_store_mcp/shirtify_agent.py
🧬 Code graph analysis (1)
examples/mcp/tshirt_store_mcp/mcp_agent_wrapper.py (1)
examples/mcp/tshirt_store_mcp/shirtify_agent.py (2)
ShirtifyAgent(64-136)invoke(76-82)
🔇 Additional comments (5)
.vscode/launch.json (1)
63-71: LGTM!The new launch configuration follows the established pattern and correctly points to the new MCP example directory and environment file.
examples/mcp/tshirt_store_mcp/shirtify_agent.py (3)
10-54: LGTM!The agent instructions are clear and comprehensive, defining the store's business rules, available products, and tool interfaces.
57-61: LGTM!The
ResponseFormatmodel is well-structured with appropriate type constraints usingLiteralfor the status field.
84-115: LGTM!The response mapping logic correctly handles all three status cases and includes a safe fallback for unexpected states.
examples/mcp/tshirt_store_mcp/__main__.py (1)
12-13: LGTM!Standard entry point pattern is correctly implemented.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
examples/mcp/tshirt_store_mcp/__main__.py (2)
2-2: Fix the import to use relative import.The import should be relative since this module is within the same package.
Apply this diff to fix the import:
-from mcp_agent_wrapper import mcp +from .mcp_agent_wrapper import mcp
7-15: Add error handling for MCP server startup.The
mcp.run()call can raise exceptions (e.g., port already in use, configuration errors). Add try/except to handle failures gracefully.As per coding guidelines.
Apply this diff to add error handling:
def main() -> None: + load_dotenv() print("Starting MCP server...") - # Can also be "sse". - # When using "sse", the url will be http://localhost:10001/sse - # When using "streamable-http", the url will be http://localhost:10001/mcp - # stdio isn't supported in this example, since rogue won't be able to connect to it. - mcp.run(transport="streamable-http") - # mcp.run(transport="sse") + try: + # Can also be "sse". + # When using "sse", the url will be http://localhost:10001/sse + # When using "streamable-http", the url will be http://localhost:10001/mcp + # stdio isn't supported in this example, since rogue won't be able to connect to it. + mcp.run(transport="streamable-http") + # mcp.run(transport="sse") + except Exception as e: + print(f"Failed to start MCP server: {e}") + raiseexamples/mcp/tshirt_store_mcp/mcp_agent_wrapper.py (3)
11-11: Fix the import to use relative import.The import should be relative since this module is within the same package.
Apply this diff to fix the import:
-from shirtify_agent import ShirtifyAgent +from .shirtify_agent import ShirtifyAgent
14-14: Defer agent instantiation to avoid module-level failures.Instantiating
ShirtifyAgentat module level means any error during initialization (e.g., missing API keys, invalid model configuration) will cause the entire module import to fail. Consider lazy initialization or adding error handling.Consider lazy initialization:
-agent = ShirtifyAgent() +_agent = None + +def _get_agent() -> ShirtifyAgent: + global _agent + if _agent is None: + _agent = ShirtifyAgent() + return _agent + mcp = FastMCP( "shirtify_agent_mcp", port=10001, host="127.0.0.1", )Then update line 42:
- response = agent.invoke(message, session_id) + response = _get_agent().invoke(message, session_id)
42-43: Add error handling for agent.invoke() call.The
agent.invoke()call can raise exceptions (network errors, API failures, etc.). Add try/except to handle failures and return appropriate error responses.As per coding guidelines.
Apply this diff to add error handling:
# Invoking our agent - response = agent.invoke(message, session_id) - return response.get("content", "") + try: + response = agent.invoke(message, session_id) + return response.get("content", "") + except Exception as e: + logger.exception("Error invoking agent") + return f"Error processing message: {str(e)}"
🧹 Nitpick comments (2)
examples/mcp/tshirt_store_mcp/mcp_agent_wrapper.py (2)
38-39: Misleading error log when session_id is None.The error log suggests a problem when
session_idis None, butShirtifyAgent.invoke()generates a new session ID when None is passed. Either remove the error log or change it to an info/debug log stating a new session will be created.Apply this diff:
if session_id is None: - logger.error("Couldn't extract session id") + logger.info("No session_id provided, a new session will be created")
42-43: Consider propagating error status from agent response.
ShirtifyAgent.invoke()returns a Dict withis_error,is_task_complete, andrequire_user_inputfields, but onlycontentis returned. Since MCP tools return strings, you may want to include error indicators in the returned text whenis_error=True.Apply this diff to include error status in the response:
# Invoking our agent - response = agent.invoke(message, session_id) - return response.get("content", "") + try: + response = agent.invoke(message, session_id) + content = response.get("content", "") + if response.get("is_error", False): + logger.warning(f"Agent returned error: {content}") + return content + except Exception as e: + logger.exception("Error invoking agent") + return f"Error processing message: {str(e)}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/mcp/tshirt_store_mcp/__main__.py(1 hunks)examples/mcp/tshirt_store_mcp/mcp_agent_wrapper.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Format Python code with Black
Ensure code passes flake8 linting
Run mypy with the repository configuration for static typing
Run Bandit security checks using .bandit.yaml configuration
Use isort import conventions for import ordering
Add type hints to all function signatures
Follow PEP 8 naming (snake_case for variables/functions, PascalCase for classes)
Use try/except around code that may raise exceptions
Files:
examples/mcp/tshirt_store_mcp/mcp_agent_wrapper.pyexamples/mcp/tshirt_store_mcp/__main__.py
🧬 Code graph analysis (1)
examples/mcp/tshirt_store_mcp/mcp_agent_wrapper.py (1)
examples/mcp/tshirt_store_mcp/shirtify_agent.py (2)
ShirtifyAgent(64-136)invoke(76-82)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: rogue_sanity
- GitHub Check: codestyle
2 participants
Description
Motivation and Context
Type of Change
Changes Made
Screenshots/Examples (if applicable)
Checklist
uv run black .to format my codeuv run flake8 .and fixed all issuesuv run mypy --config-file .mypy.ini .and addressed type checking issuesuv run bandit -c .bandit.yaml -r .for security checksuv run pytestand all tests passTesting
Test Configuration:
Test Steps:
1.
2.
3.
Additional Notes
Related Issues/PRs