FIRE-831 | Rogue Server | MCP transport #103
Conversation
Summary by CodeRabbit
WalkthroughThreads Protocol and Transport through SDK, server, and evaluator-agent layers; introduces BaseEvaluatorAgent with A2A and MCP implementations plus a factory; updates orchestrator/runner to use the factory and async context managers; adds MCP example CLI and FastMCP dependency; simplifies CI/test invocation scripts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant SDK as RogueSDK
participant Orch as EvaluationOrchestrator
participant Factory as get_evaluator_agent
participant Agent as BaseEvaluatorAgent
participant A2A as A2AEvaluatorAgent
participant MCP as MCPEvaluatorAgent
participant Eval as EvaluatedAgent
SDK->>Orch: run_job(protocol, transport, ...)
Orch->>Factory: get_evaluator_agent(protocol, transport, ...)
alt protocol == A2A
Factory-->>A2A: instantiate(...)
Orch->>A2A: async with A2A
Orch->>A2A: _send_message_to_evaluated_agent(ctx,msg)
A2A->>Eval: A2A HTTP request
Eval-->>A2A: response
A2A-->>Orch: response dict
else protocol == MCP
Factory-->>MCP: instantiate(...)
Orch->>MCP: async with MCP
Orch->>MCP: _send_message_to_evaluated_agent(ctx,msg)
MCP->>Eval: MCP tool invocation (stream/request)
Eval-->>MCP: streamed/aggregated response
MCP-->>Orch: response dict
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
Comment |
…-transport-support
…-transport-support
…-transport-support
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
rogue/evaluator_agent/protocols/mcp_evaluator_agent.py (1)
26-27: Use the documented import path for fastmcp transports.The current import uses
from fastmcp.client import ..., but the official fastmcp documentation (for versions 2.12.5+) consistently shows transports should be imported fromfastmcp.client.transports. While the current path may work, using the documented path improves maintainability and aligns with fastmcp conventions.Apply this diff:
from fastmcp import Client - from fastmcp.client import SSETransport, StreamableHttpTransport + from fastmcp.client.transports import SSETransport, StreamableHttpTransportBased on learnings.
rogue/evaluator_agent/protocols/a2a_evaluator_agent.py (1)
187-187: Response format inconsistency between A2A and MCP protocols.Line 187 returns the full JSON payload (
response.model_dump_json()), while the extracted text (agent_response_text) is stored in chat history. The MCP implementation returns extracted text in the"response"key. This protocol-level inconsistency can degrade LLM tool reasoning and complicate downstream processing.Based on past review comments, consider normalizing to return extracted text:
- return {"response": response.model_dump_json()} + return { + "response": agent_response_text, + "raw": response.model_dump_json(), # optional: keep full payload + }
🧹 Nitpick comments (2)
rogue/evaluator_agent/evaluator_agent_factory.py (1)
10-23: Consider adding a docstring to document the factory function.The function signature is well-typed, but a docstring would help document the factory's purpose, parameter meanings, and return behavior for maintainers.
Apply this diff:
def get_evaluator_agent( protocol: Protocol, transport: Transport | None, evaluated_agent_address: str, judge_llm: str, scenarios: Scenarios, business_context: Optional[str], headers: Optional[dict[str, str]] = None, judge_llm_auth: Optional[str] = None, debug: bool = False, deep_test_mode: bool = False, chat_update_callback: Optional[Callable[[dict], None]] = None, **kwargs, ) -> BaseEvaluatorAgent: + """ + Factory function to create protocol-specific evaluator agents. + + :param protocol: Protocol type (A2A or MCP) + :param transport: Transport type (or None to use protocol default) + :param evaluated_agent_address: URL of the agent to evaluate + :param judge_llm: Model name for the judge LLM + :param scenarios: Collection of evaluation scenarios + :param business_context: Business context for evaluation + :param headers: Optional HTTP headers + :param judge_llm_auth: Optional API key for judge LLM + :param debug: Enable debug logging + :param deep_test_mode: Enable deep testing mode + :param chat_update_callback: Optional callback for chat updates + :return: Protocol-specific evaluator agent instance + :raises ValueError: If protocol is not supported + """ if protocol == Protocol.A2A:rogue/evaluator_agent/protocols/mcp_evaluator_agent.py (1)
131-144: Add error_type to exception logging for consistency with A2A observability.The exception handler logs structured context but omits
error_type, which the A2A implementation includes for debugging. Adding it improves observability parity across protocols.Apply this diff:
except Exception as e: logger.exception( "Error while sending message to evaluated agent using mcp", extra={ "protocol": "mcp", "agent_url": self._evaluated_agent_address, "transport": self._transport.value, "message": message[:100] + "..." if len(message) > 100 else message, + "error": str(e), + "error_type": type(e).__name__, }, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
rogue/evaluator_agent/evaluator_agent_factory.py(1 hunks)rogue/evaluator_agent/protocols/a2a_evaluator_agent.py(1 hunks)rogue/evaluator_agent/protocols/base_evaluator_agent.py(7 hunks)rogue/evaluator_agent/protocols/mcp_evaluator_agent.py(1 hunks)sdks/python/rogue_sdk/types.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sdks/python/rogue_sdk/types.py
🧰 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:
rogue/evaluator_agent/protocols/a2a_evaluator_agent.pyrogue/evaluator_agent/evaluator_agent_factory.pyrogue/evaluator_agent/protocols/mcp_evaluator_agent.pyrogue/evaluator_agent/protocols/base_evaluator_agent.py
🧬 Code graph analysis (4)
rogue/evaluator_agent/protocols/a2a_evaluator_agent.py (4)
sdks/python/rogue_sdk/types.py (3)
Protocol(68-79)Scenarios(181-199)Transport(82-93)rogue/common/remote_agent_connection.py (1)
RemoteAgentConnections(52-130)rogue/evaluator_agent/protocols/base_evaluator_agent.py (3)
BaseEvaluatorAgent(156-556)_send_message_to_evaluated_agent(491-508)_add_message_to_chat_history(519-545)rogue/evaluator_agent/protocols/mcp_evaluator_agent.py (1)
_send_message_to_evaluated_agent(75-108)
rogue/evaluator_agent/evaluator_agent_factory.py (4)
sdks/python/rogue_sdk/types.py (3)
Protocol(68-79)Scenarios(181-199)Transport(82-93)rogue/evaluator_agent/protocols/a2a_evaluator_agent.py (1)
A2AEvaluatorAgent(19-212)rogue/evaluator_agent/protocols/base_evaluator_agent.py (1)
BaseEvaluatorAgent(156-556)rogue/evaluator_agent/protocols/mcp_evaluator_agent.py (1)
MCPEvaluatorAgent(10-144)
rogue/evaluator_agent/protocols/mcp_evaluator_agent.py (3)
sdks/python/rogue_sdk/types.py (3)
Protocol(68-79)Scenarios(181-199)Transport(82-93)rogue/evaluator_agent/protocols/base_evaluator_agent.py (3)
BaseEvaluatorAgent(156-556)_send_message_to_evaluated_agent(491-508)_add_message_to_chat_history(519-545)rogue/evaluator_agent/protocols/a2a_evaluator_agent.py (1)
_send_message_to_evaluated_agent(113-199)
rogue/evaluator_agent/protocols/base_evaluator_agent.py (5)
sdks/python/rogue_sdk/types.py (11)
Protocol(68-79)Scenario(139-178)Scenarios(181-199)ScenarioType(51-55)Transport(82-93)get_default_transport(74-79)is_valid_for_protocol(92-93)EvaluationResults(237-253)ChatHistory(210-218)add_message(215-218)ChatMessage(202-207)rogue/common/agent_model_wrapper.py (1)
get_llm_from_model(11-35)rogue/evaluator_agent/policy_evaluation.py (1)
evaluate_policy(136-204)rogue/evaluator_agent/protocols/a2a_evaluator_agent.py (1)
_send_message_to_evaluated_agent(113-199)rogue/evaluator_agent/protocols/mcp_evaluator_agent.py (1)
_send_message_to_evaluated_agent(75-108)
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
rogue/evaluator_agent/a2a/a2a_evaluator_agent.py (1)
176-189: Normalize return shape: put text inresponseand includerawpayload.Aligns with MCP and avoids forcing callers to parse JSON-strings.
- return {"response": response.model_dump_json()} + return { + "response": agent_response_text, + "raw": response.model_dump(), # keep structured payload if needed + }
🧹 Nitpick comments (4)
rogue/evaluator_agent/mcp/__init__.py (1)
1-2: Drop redundant submodule import to reduce import-time side effects.Importing the module then the class is unnecessary and can contribute to cycles.
-from . import mcp_evaluator_agent from .mcp_evaluator_agent import MCPEvaluatorAgentrogue/evaluator_agent/a2a/__init__.py (1)
1-2: Remove unnecessary submodule import.Keeps the surface the same while minimizing circular-import risk.
-from . import a2a_evaluator_agent from .a2a_evaluator_agent import A2AEvaluatorAgentrogue/evaluator_agent/__init__.py (1)
1-11: Consider slimming package imports to reduce future cycle risk.Re-exporting classes is fine; importing heavy modules (e.g., evaluator_agent_factory) at package import time isn’t necessary. Consider dropping those from the top-level import list and all, keeping only subpackages and class re-exports.
Also applies to: 12-21
rogue/evaluator_agent/mcp/mcp_evaluator_agent.py (1)
132-141: Enrich MCP exception logs with error details to match A2A observability.Add error and error_type fields so logs are consistent and actionable.
- logger.exception( - "Error while sending message to evaluated agent using mcp", + logger.exception( + "❌ MCP call failed - error sending message to evaluated agent", extra={ "protocol": "mcp", "agent_url": self._evaluated_agent_address, "transport": self._transport.value, "message": message[:100] + "..." if len(message) > 100 else message, + "error": str(e), + "error_type": type(e).__name__, }, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
rogue/evaluator_agent/__init__.py(1 hunks)rogue/evaluator_agent/a2a/__init__.py(1 hunks)rogue/evaluator_agent/a2a/a2a_evaluator_agent.py(1 hunks)rogue/evaluator_agent/base_evaluator_agent.py(7 hunks)rogue/evaluator_agent/evaluator_agent_factory.py(1 hunks)rogue/evaluator_agent/mcp/__init__.py(1 hunks)rogue/evaluator_agent/mcp/mcp_evaluator_agent.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:
rogue/evaluator_agent/mcp/__init__.pyrogue/evaluator_agent/mcp/mcp_evaluator_agent.pyrogue/evaluator_agent/a2a/__init__.pyrogue/evaluator_agent/base_evaluator_agent.pyrogue/evaluator_agent/__init__.pyrogue/evaluator_agent/evaluator_agent_factory.pyrogue/evaluator_agent/a2a/a2a_evaluator_agent.py
🧬 Code graph analysis (7)
rogue/evaluator_agent/mcp/__init__.py (1)
rogue/evaluator_agent/mcp/mcp_evaluator_agent.py (1)
MCPEvaluatorAgent(10-145)
rogue/evaluator_agent/mcp/mcp_evaluator_agent.py (3)
sdks/python/rogue_sdk/types.py (3)
Protocol(68-79)Scenarios(181-199)Transport(82-93)rogue/evaluator_agent/base_evaluator_agent.py (3)
BaseEvaluatorAgent(156-556)_send_message_to_evaluated_agent(491-508)_add_message_to_chat_history(519-545)rogue/evaluator_agent/a2a/a2a_evaluator_agent.py (1)
_send_message_to_evaluated_agent(114-200)
rogue/evaluator_agent/a2a/__init__.py (1)
rogue/evaluator_agent/a2a/a2a_evaluator_agent.py (1)
A2AEvaluatorAgent(19-213)
rogue/evaluator_agent/base_evaluator_agent.py (5)
sdks/python/rogue_sdk/types.py (7)
Protocol(68-79)Scenarios(181-199)Transport(82-93)get_default_transport(74-79)is_valid_for_protocol(92-93)ChatHistory(210-218)add_message(215-218)rogue/common/agent_model_wrapper.py (1)
get_llm_from_model(11-35)rogue/evaluator_agent/policy_evaluation.py (1)
evaluate_policy(136-204)rogue/evaluator_agent/a2a/a2a_evaluator_agent.py (1)
_send_message_to_evaluated_agent(114-200)rogue/evaluator_agent/mcp/mcp_evaluator_agent.py (1)
_send_message_to_evaluated_agent(75-109)
rogue/evaluator_agent/__init__.py (3)
rogue/evaluator_agent/run_evaluator_agent.py (1)
run_evaluator_agent(220-248)rogue/evaluator_agent/a2a/a2a_evaluator_agent.py (1)
A2AEvaluatorAgent(19-213)rogue/evaluator_agent/mcp/mcp_evaluator_agent.py (1)
MCPEvaluatorAgent(10-145)
rogue/evaluator_agent/evaluator_agent_factory.py (4)
sdks/python/rogue_sdk/types.py (3)
Protocol(68-79)Scenarios(181-199)Transport(82-93)rogue/evaluator_agent/a2a/a2a_evaluator_agent.py (1)
A2AEvaluatorAgent(19-213)rogue/evaluator_agent/mcp/mcp_evaluator_agent.py (1)
MCPEvaluatorAgent(10-145)rogue/evaluator_agent/base_evaluator_agent.py (1)
BaseEvaluatorAgent(156-556)
rogue/evaluator_agent/a2a/a2a_evaluator_agent.py (4)
sdks/python/rogue_sdk/types.py (3)
Protocol(68-79)Scenarios(181-199)Transport(82-93)rogue/common/remote_agent_connection.py (1)
RemoteAgentConnections(52-130)rogue/evaluator_agent/base_evaluator_agent.py (3)
BaseEvaluatorAgent(156-556)_send_message_to_evaluated_agent(491-508)_add_message_to_chat_history(519-545)rogue/evaluator_agent/mcp/mcp_evaluator_agent.py (1)
_send_message_to_evaluated_agent(75-109)
🪛 GitHub Actions: Rogue
rogue/evaluator_agent/evaluator_agent_factory.py
[error] 1-1: ImportError: cannot import name 'A2AEvaluatorAgent' from partially initialized module 'rogue.evaluator_agent' during 'python -m rogue cli'. This is likely due to a circular import.
🪛 GitHub Actions: Test
rogue/evaluator_agent/evaluator_agent_factory.py
[error] 1-1: ImportError: cannot import name 'A2AEvaluatorAgent' from partially initialized module 'rogue.evaluator_agent' (likely due to a circular import) during pytest collection.
⏰ 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). (1)
- GitHub Check: codestyle
🔇 Additional comments (6)
rogue/evaluator_agent/mcp/mcp_evaluator_agent.py (1)
26-28: I need to verify the codebase's fastmcp version constraint and understand the actual compatibility concern.Let me search the codebase for the fastmcp version constraint:
The review comment is based on an incorrect premise. The import paths don't vary across 2.x versions.
All current FastMCP documentation consistently shows imports from
fastmcp.client.transports, notfastmcp.client. The official docs demonstratefrom fastmcp.client.transports import StreamableHttpTransportacross all 2.x versions shown (2.0.0 through current).The code's current import path (
from fastmcp.client import SSETransport, StreamableHttpTransport) is not documented anywhere. Rather than a backward compatibility issue with varying module layouts, the simple fix is to align with the documented standard import path. A try/except fallback is unnecessary and obscures the straightforward update needed.Rewritten suggestion: Update the import to use the documented path:
from fastmcp.client.transports import SSETransport, StreamableHttpTransport(no fallback needed).Likely an incorrect or invalid review comment.
rogue/evaluator_agent/base_evaluator_agent.py (5)
156-156: LGTM!The class name change to
BaseEvaluatorAgentand inheriting fromABCclearly signals this is now an abstract base class, aligning well with the protocol/transport abstraction pattern.
157-189: LGTM!The updated
__init__signature correctly introduces protocol/transport abstraction with:
- Proper default transport resolution from protocol
- Validation that transport is compatible with protocol
- Support for protocol-specific headers
- Extensibility via
*args, **kwargsfor subclasses
490-508: LGTM!Correctly made abstract with
@abstractmethoddecorator and clear documentation. The relevant code snippets confirm that bothA2AEvaluatorAgent(lines 113-199) andMCPEvaluatorAgent(lines 74-108) properly implement this method with protocol-specific logic.
519-546: LGTM!Excellent refactor! This helper centralizes chat history management and callback synchronization, eliminating duplication between protocol-specific implementations. The role mapping logic for callbacks is clear and appropriate.
547-556: Base async context manager implementation is correct; subclasses already implement proper cleanup.Both
MCPEvaluatorAgentandA2AEvaluatorAgentcorrectly override__aexit__to clean up their managed resources (MCP client and HTTP client respectively). The baseBaseEvaluatorAgentimplementation with no cleanup is appropriate since the base class doesn't manage resources. No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rogue/__main__.py (1)
131-132: Remove duplicate comment.The comment "Use subprocess to run the example agent" is duplicated from lines 120-121. Consider removing it or consolidating the logic to reduce duplication.
Apply this diff:
elif example_name == "tshirt_store_langgraph_mcp": - # Use subprocess to run the example agent cmd = [
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.vscode/launch.json(1 hunks)examples/mcp/tshirt_store_langgraph_mcp/__main__.py(1 hunks)examples/mcp/tshirt_store_langgraph_mcp/mcp_agent_wrapper.py(1 hunks)rogue/__main__.py(2 hunks)rogue/evaluator_agent/evaluator_agent_factory.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rogue/evaluator_agent/evaluator_agent_factory.py
🧰 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_langgraph_mcp/__main__.pyexamples/mcp/tshirt_store_langgraph_mcp/mcp_agent_wrapper.pyrogue/__main__.py
🧬 Code graph analysis (1)
examples/mcp/tshirt_store_langgraph_mcp/mcp_agent_wrapper.py (1)
examples/mcp/tshirt_store_langgraph_mcp/shirtify_agent.py (1)
ShirtifyAgent(64-136)
⏰ 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). (1)
- GitHub Check: codestyle
🔇 Additional comments (4)
.vscode/launch.json (1)
61-61: Module paths are valid; .env files are local configuration (gitignored by design).Both module paths (
examples.tshirt_store_agentandexamples.mcp.tshirt_store_langgraph_mcp) correctly reference importable Python modules with proper structure (init.py and main.py present). The envFile paths reference local configuration files that are explicitly excluded from version control (per .gitignore policy: "do not commit any .env files to git"). These files are expected to be created locally by developers and do not need to exist in the repository.Likely an incorrect or invalid review comment.
examples/mcp/tshirt_store_langgraph_mcp/mcp_agent_wrapper.py (1)
13-14: LGTM! Correct use of relative import.The change from absolute to relative import is appropriate for a package-internal module reference. The import ordering follows isort conventions (third-party imports, then relative imports with a blank line separator).
examples/mcp/tshirt_store_langgraph_mcp/__main__.py (1)
2-3: LGTM! Correct import structure.The relative import and blank line separator are appropriate. This follows isort conventions by placing a blank line between third-party imports (line 1) and package-relative imports (line 3).
rogue/__main__.py (1)
47-49: LGTM!The parser extension properly adds the new MCP example choice and updates the help text accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
examples/mcp/tshirt_store_langgraph_mcp/mcp_agent_wrapper.py (1)
29-41: Refine exception handling to avoid masking unexpected errors.The bare
except Exception:on line 39 catches all exceptions during session extraction, which may mask unexpected errors like attribute errors or bugs in the extraction logic.Consider catching more specific exceptions related to request context access, or at minimum, re-raise unexpected exceptions after logging.
Apply this diff to improve exception handling:
session_id: str | None = None try: request: Request = context.request_context.request # type: ignore # The session id should be in the headers for streamable-http transport session_id = request.headers.get("mcp-session-id") # The session id might also be in query param when using sse transport if session_id is None: session_id = request.query_params.get("session_id") - except Exception: + except (AttributeError, KeyError, TypeError) as e: session_id = None - logger.exception("Error while extracting session id") + logger.exception(f"Error while extracting session id: {e}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/mcp/tshirt_store_langgraph_mcp/__main__.py(1 hunks)examples/mcp/tshirt_store_langgraph_mcp/mcp_agent_wrapper.py(1 hunks)rogue/__main__.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rogue/main.py
🧰 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_langgraph_mcp/mcp_agent_wrapper.pyexamples/mcp/tshirt_store_langgraph_mcp/__main__.py
🧬 Code graph analysis (2)
examples/mcp/tshirt_store_langgraph_mcp/mcp_agent_wrapper.py (1)
examples/mcp/tshirt_store_langgraph_mcp/shirtify_agent.py (2)
ShirtifyAgent(64-136)invoke(76-82)
examples/mcp/tshirt_store_langgraph_mcp/__main__.py (1)
examples/mcp/tshirt_store_langgraph_mcp/mcp_agent_wrapper.py (1)
get_mcp_server(19-50)
⏰ 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
🔇 Additional comments (6)
examples/mcp/tshirt_store_langgraph_mcp/mcp_agent_wrapper.py (3)
9-10: LGTM!The
lru_cacheimport is appropriate for memoizing the MCP server factory function.
15-16: LGTM!The relative import is appropriate for the package structure and consistent with the CLI module.
18-25: Critical: LRU cache will ignore host/port parameters after first call.The
@lru_cache(maxsize=1)decorator will cache only the first return value. Subsequent calls with differenthostorportvalues will return the cached instance created with the first call's parameters, silently ignoring the new values.If the intent is to ensure a singleton MCP server, the parameters should be removed or validated. Otherwise, remove the cache or increase
maxsizeand ensure parameter-based caching works as expected.Apply this diff to fix the issue by removing parameters (singleton approach):
-@lru_cache(maxsize=1) -def get_mcp_server(host: str = "127.0.0.1", port: int = 10001) -> FastMCP: +def get_mcp_server() -> FastMCP: + host = "127.0.0.1" + port = 10001 agent = ShirtifyAgent() mcp = FastMCP(Or remove caching to respect parameters:
-@lru_cache(maxsize=1) def get_mcp_server(host: str = "127.0.0.1", port: int = 10001) -> FastMCP:Likely an incorrect or invalid review comment.
examples/mcp/tshirt_store_langgraph_mcp/__main__.py (3)
1-6: LGTM!The imports are well-organized and appropriate. The relative import is consistent with the package structure, and the
Literaltype hint enhances type safety for the transport parameter.
11-21: LGTM!The Click CLI setup is well-structured with appropriate defaults and type hints. The
Literaltype annotation for transport choices provides strong type safety.
23-28: Verify impact of the LRU cache issue inget_mcp_server.The implementation correctly passes the CLI-provided
hostandporttoget_mcp_server. However, due to the@lru_cache(maxsize=1)decorator inmcp_agent_wrapper.py, these parameters will be ignored after the first call, which undermines the CLI's flexibility.Ensure the caching issue identified in
mcp_agent_wrapper.py(lines 18-25) is resolved to make the CLI parameters effective.
yuval-qf
temporarily deployed
to
rogue-sanity-ci-secrets
GitHub Actions
Inactive
yuval-qf
temporarily deployed
to
rogue-sanity-ci-secrets
GitHub Actions
Inactive
yuval-qf
temporarily deployed
to
rogue-sanity-ci-secrets
GitHub Actions
Inactive
yuval-qf
temporarily deployed
to
rogue-sanity-ci-secrets
GitHub Actions
Inactive
yuval-qf
had a problem deploying
to
rogue-sanity-ci-secrets
GitHub Actions
Failure
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