-
Notifications
You must be signed in to change notification settings - Fork 772
Make mcp-agent Context a derivative of FastMCP context #504
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughContext now subclasses MCPContext and gains FastMCP-aware properties, request binding, session/logger resolution, async log/report_progress/read_resource methods, and typing updates. Server/app/tool layers detect and inject FastMCP Context parameters and bind per-request app Contexts for workflows and tools. Tests updated to validate per-request binding and richer tool metadata; mcp dependency bumped. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App Context
participant Ctx as Context (MCPContext)
participant Req as Request Context
participant FMCP as FastMCP
participant Tool as Tool/Workflow
participant Log as Logger
Note over App,Ctx: App-level Context exists (global)
App->>Ctx: hold app Context
Note over Req,Ctx: Per-request binding
Req->>Ctx: bind_request(request_ctx, fastmcp?)
Ctx->>Ctx: set _request_context, _fastmcp, bound_app_context
Note over Tool,Ctx: Invocation receives Context
Tool->>Ctx: injected bound context (param or kw-only ctx)
Note over Ctx,FMCP: Session/resource resolution
Ctx->>FMCP: resolve session / read_resource(uri)
alt FMCP provides
FMCP-->>Tool: data / ServerSession
else no FMCP
Ctx-->>Tool: raise ValueError (read_resource)
end
Note over Tool,Log: Logging and progress
Tool->>Ctx: ctx.log(...) / ctx.report_progress(...)
Ctx->>Log: forward to request logger or app logger
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
src/mcp_agent/core/context.py
Outdated
def logger(self) -> "Logger": | ||
return self.app.logger if self.app else None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logger
property's return type annotation is Logger
, but it returns None
when self.app
is None
. This type mismatch could lead to runtime errors when calling methods on what's expected to be a Logger
object. Consider either:
- Updating the return type annotation to
Logger | None
to accurately reflect the possible return values, or - Providing a fallback logger implementation when
self.app
isNone
This would help prevent potential NoneType
errors during execution.
def logger(self) -> "Logger": | |
return self.app.logger if self.app else None | |
def logger(self) -> "Logger | None": | |
return self.app.logger if self.app else None |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/mcp_agent/core/context.py (5)
10-10
: Fix Ruff F401: remove unused BaseModel import
BaseModel
is no longer used after switching toMCPContext
.-from pydantic import BaseModel, ConfigDict +from pydantic import ConfigDict
126-163
: Narrow exception handling in session resolutionCatching bare
Exception
can hide real issues. Limit to the expected failures (e.g.,AttributeError
,RuntimeError
) for clearer diagnostics.- try: - return super().session # type: ignore[misc] - except Exception: + try: + return super().session # type: ignore[misc] + except (AttributeError, RuntimeError): pass @@ - try: + try: mcp = self.mcp if mcp is not None: ctx = mcp.get_context() # FastMCP.get_context returns a Context that raises outside a request; # guard accordingly. - try: + try: return getattr(ctx, "session", None) - except Exception: + except (AttributeError, RuntimeError): return None - except Exception: + except (AttributeError, RuntimeError): passConsider applying the same pattern to other helpers in this file that currently use
except Exception
.
165-167
: Fix return type for logger propertyThis can return
None
whenapp
is absent; reflect that in the annotation.- def logger(self) -> "Logger": + def logger(self) -> "Logger | None": return self.app.logger if self.app else None
209-244
: Honor logger_name in local fallbackWhen falling back to local logging, use
logger_name
if provided to keep parity with FastMCP’s namespacing.- _logger = self.logger + _logger = self.logger + if logger_name: + _logger = get_logger(logger_name, session_id=self.session_id, context=self)
71-71
: Verified — upstream_session typing OK; unify client_session_factory annotations (optional)Runtime: OK — start_server tolerates factories with/without context (tries with context then falls back), MCPAgentClientSession accepts an optional context, and SessionProxy subclasses ServerSession so Context.upstream_session: Optional[ServerSession] is correct.
Recommended (optional) refactor — align type hints for client_session_factory to avoid static-type confusion:
- src/mcp_agent/core/context.py — upstream_session remains at line ~71 (Optional[ServerSession]).
- src/mcp_agent/mcp/mcp_server_registry.py — start_server signature (≈ lines 108–116).
- src/mcp_agent/mcp/gen_client.py — gen_client/connect signatures (≈ lines 17–31).
- src/mcp_agent/mcp/mcp_connection_manager.py — launch_server/get_server/get_server_capabilities signatures (≈ lines 64–70, 410–416, 519–526).
No critical fixes required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mcp_agent/core/context.py
(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: saqadri
PR: lastmile-ai/mcp-agent#386
File: src/mcp_agent/mcp/mcp_server_registry.py:110-116
Timestamp: 2025-08-28T15:07:10.015Z
Learning: In MCP server registry methods, when client_session_factory parameters are updated to accept additional context parameters, ensure the type hints match what is actually passed (Context instance vs ServerSession) and that the default factory (MCPAgentClientSession) can handle the number of arguments being passed to avoid TypeError at runtime.
Learnt from: rholinshead
PR: lastmile-ai/mcp-agent#241
File: src/mcp_agent/mcp/mcp_agent_client_session.py:174-187
Timestamp: 2025-05-27T19:10:22.911Z
Learning: In the MCP Agent codebase, all request parameter types inherit from RequestParams or NotificationParams with consistent Meta class structures, so trace context injection using the base Meta classes doesn't cause type-mismatch issues.
📚 Learning: 2025-08-28T15:07:10.015Z
Learnt from: saqadri
PR: lastmile-ai/mcp-agent#386
File: src/mcp_agent/mcp/mcp_server_registry.py:110-116
Timestamp: 2025-08-28T15:07:10.015Z
Learning: In MCP server registry methods, when client_session_factory parameters are updated to accept additional context parameters, ensure the type hints match what is actually passed (Context instance vs ServerSession) and that the default factory (MCPAgentClientSession) can handle the number of arguments being passed to avoid TypeError at runtime.
Applied to files:
src/mcp_agent/core/context.py
📚 Learning: 2025-07-22T18:59:49.368Z
Learnt from: CR
PR: lastmile-ai/mcp-agent#0
File: examples/usecases/reliable_conversation/CLAUDE.md:0-0
Timestamp: 2025-07-22T18:59:49.368Z
Learning: Applies to examples/usecases/reliable_conversation/examples/reliable_conversation/src/**/*.py : Use mcp-agent's Agent abstraction for ALL LLM interactions, including quality evaluation, to ensure consistent tool access, logging, and error handling.
Applied to files:
src/mcp_agent/core/context.py
🧬 Code graph analysis (1)
src/mcp_agent/core/context.py (3)
src/mcp_agent/server/app_server.py (2)
app
(135-137)workflow_registry
(145-147)src/mcp_agent/app.py (5)
MCPApp
(42-967)logger
(193-210)upstream_session
(173-174)upstream_session
(177-178)session_id
(189-190)src/mcp_agent/logging/logger.py (2)
Logger
(34-311)get_logger
(513-541)
🪛 GitHub Actions: Pull Request Checks
src/mcp_agent/core/context.py
[error] 10-10: Ruff: F401 'pydantic.BaseModel' imported but unused. Remove unused import. (Command: 'ruff check')
🔇 Additional comments (3)
src/mcp_agent/core/context.py (3)
108-125
: Nice FastMCP fallback behaviorGraceful fallback to app-managed FastMCP when no request-bound context exists looks good.
245-259
: Good no-op behavior for report_progress outside requestsMatches API while avoiding spurious errors. LGTM.
260-284
: Resource read fallback is sensibleDelegating to app-level FastMCP when not request-bound is a solid UX improvement, with a clear error otherwise.
src/mcp_agent/core/context.py
Outdated
def bind_request(self, request_context: Any, fastmcp: FastMCP | None = None) -> "Context": | ||
"""Return a shallow-copied Context bound to a specific FastMCP request. | ||
- Shares app-wide state (config, registries, token counter, etc.) with the original Context | ||
- Attaches `_request_context` and `_fastmcp` so FastMCP Context APIs work during the request | ||
- Does not mutate the original Context (safe for concurrent requests) | ||
""" | ||
# Shallow copy to preserve references to registries/loggers while keeping isolation | ||
bound: Context = self.model_copy(deep=False) | ||
try: | ||
setattr(bound, "_request_context", request_context) | ||
except Exception: | ||
pass | ||
try: | ||
if fastmcp is None: | ||
fastmcp = getattr(self, "_fastmcp", None) or self.mcp | ||
setattr(bound, "_fastmcp", fastmcp) | ||
except Exception: | ||
pass | ||
return bound | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid Pydantic-only dependency in bind_request
self.model_copy
assumes a Pydantic base. If MCPContext
isn’t a Pydantic model, this will raise. Use model_copy
when available, else fall back to copy.copy
.
- # Shallow copy to preserve references to registries/loggers while keeping isolation
- bound: Context = self.model_copy(deep=False)
+ # Shallow copy to preserve references to registries/loggers while keeping isolation
+ # Prefer Pydantic's model_copy when available; otherwise use copy.copy
+ bound: Context = (
+ self.model_copy(deep=False) # type: ignore[attr-defined]
+ if hasattr(self, "model_copy")
+ else copy.copy(self)
+ )
Add the missing import (outside this hunk):
# at top-level imports
import copy
🤖 Prompt for AI Agents
In src/mcp_agent/core/context.py around lines 170 to 190, bind_request currently
calls self.model_copy(deep=False) which assumes the Context is a Pydantic model
and will raise if not; change this to use model_copy only when present (e.g., if
hasattr(self, "model_copy")), otherwise fall back to using copy.copy(self) to
perform a shallow copy; also add the missing top-level import "import copy" to
the module imports so the fallback works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcp_agent/core/context.py (1)
76-78
: Fix mutable class-level default: loaded_subagents is shared across all Context instancesThis list will be shared (and can bleed across requests/apps). Use a per-instance default.
Apply this diff:
- loaded_subagents: List["AgentSpec"] = [] + loaded_subagents: List["AgentSpec"] | None = NoneAnd initialize it during context setup (see below).
@@ - context = Context() + context = Context() + if context.loaded_subagents is None: + context.loaded_subagents = []
♻️ Duplicate comments (2)
src/mcp_agent/core/context.py (2)
179-181
: Don’t assume Pydantic: model_copy may not exist on MCPContextMCPContext may not expose Pydantic’s model_copy; this will raise at runtime in bind_request.
Apply this diff to use Pydantic when available, else fall back to copy.copy:
@@ - bound: Context = self.model_copy(deep=False) + bound: Context = ( + self.model_copy(deep=False) # type: ignore[attr-defined] + if hasattr(self, "model_copy") + else copy.copy(self) + )Add import:
@@ -import asyncio +import asyncio +import copyAlso applies to: 5-6
164-167
: Type mismatch: logger can be NoneReturn type is Logger but None is returned when app is None. Align the annotation (or provide a fallback logger).
- def logger(self) -> "Logger": + def logger(self) -> "Logger | None": return self.app.logger if self.app else None
🧹 Nitpick comments (2)
src/mcp_agent/core/context.py (2)
211-246
: Honor logger_name in fallback logging pathWhen not in a FastMCP request, the logger_name parameter is ignored. Respect it to keep parity with FastMCP behavior and improve observability.
- # Fall back to local logger if available + # Fall back to local logger if available try: - _logger = self.logger + _logger = ( + get_logger(logger_name, session_id=self.session_id, context=self) + if logger_name + else self.logger + ) if _logger is not None: if level == "debug": _logger.debug(message) elif level == "warning": _logger.warning(message) elif level == "error": _logger.error(message) else: _logger.info(message)
99-103
: Pydantic config likely inert after switching to MCPContextIf MCPContext isn’t a Pydantic model, model_config/ConfigDict won’t have effect. Consider removing the pydantic dependency here or add a brief comment clarifying why it remains.
Also applies to: 10-10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mcp_agent/core/context.py
(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: saqadri
PR: lastmile-ai/mcp-agent#386
File: src/mcp_agent/mcp/mcp_server_registry.py:110-116
Timestamp: 2025-08-28T15:07:10.015Z
Learning: In MCP server registry methods, when client_session_factory parameters are updated to accept additional context parameters, ensure the type hints match what is actually passed (Context instance vs ServerSession) and that the default factory (MCPAgentClientSession) can handle the number of arguments being passed to avoid TypeError at runtime.
Learnt from: rholinshead
PR: lastmile-ai/mcp-agent#241
File: src/mcp_agent/mcp/mcp_agent_client_session.py:174-187
Timestamp: 2025-05-27T19:10:22.911Z
Learning: In the MCP Agent codebase, all request parameter types inherit from RequestParams or NotificationParams with consistent Meta class structures, so trace context injection using the base Meta classes doesn't cause type-mismatch issues.
📚 Learning: 2025-08-28T15:07:10.015Z
Learnt from: saqadri
PR: lastmile-ai/mcp-agent#386
File: src/mcp_agent/mcp/mcp_server_registry.py:110-116
Timestamp: 2025-08-28T15:07:10.015Z
Learning: In MCP server registry methods, when client_session_factory parameters are updated to accept additional context parameters, ensure the type hints match what is actually passed (Context instance vs ServerSession) and that the default factory (MCPAgentClientSession) can handle the number of arguments being passed to avoid TypeError at runtime.
Applied to files:
src/mcp_agent/core/context.py
📚 Learning: 2025-07-22T18:59:49.368Z
Learnt from: CR
PR: lastmile-ai/mcp-agent#0
File: examples/usecases/reliable_conversation/CLAUDE.md:0-0
Timestamp: 2025-07-22T18:59:49.368Z
Learning: Applies to examples/usecases/reliable_conversation/examples/reliable_conversation/src/**/*.py : Use mcp-agent's Agent abstraction for ALL LLM interactions, including quality evaluation, to ensure consistent tool access, logging, and error handling.
Applied to files:
src/mcp_agent/core/context.py
🧬 Code graph analysis (1)
src/mcp_agent/core/context.py (3)
src/mcp_agent/server/app_server.py (2)
app
(135-137)workflow_registry
(145-147)src/mcp_agent/app.py (5)
MCPApp
(53-1001)logger
(204-221)upstream_session
(184-185)upstream_session
(188-189)session_id
(200-201)src/mcp_agent/logging/logger.py (7)
Logger
(34-311)get_logger
(513-541)debug
(261-269)warning
(281-289)error
(291-299)info
(271-279)progress
(301-311)
🔇 Additional comments (5)
src/mcp_agent/core/context.py (5)
108-125
: Behavior change: fastmcp now returns None outside requests—confirm downstream expectationsThe override relaxes the base contract (likely non-Optional) and may surface None at call sites that previously assumed a value or an exception. Verify callers handle Optional[FastMCP].
126-163
: Session resolution fallback looks soundPrioritization (explicit upstream_session → active FastMCP request → app.get_context()) is reasonable and defensive.
262-286
: read_resource fallback and error messaging look goodGraceful delegation to FastMCP when available; clear error otherwise.
193-210
: client_id/request_id overrides are safe and defensiveGood fallbacks that avoid raising outside of requests.
7-15
: Imports, TYPE_CHECKING aliases, base-class switch, and mcp property: LGTMAlso applies to: 39-56, 60-75, 104-107
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/mcp_agent/core/context.py (1)
198-219
: Guardbind_request
against missingmodel_copy
.After switching
Context
to inherit fromMCPContext
, we can no longer assumeself
exposes Pydantic’smodel_copy
. WhenMCPContext
(or any future subclass) omits it, this line raisesAttributeError
, so every_workflow_run
that callsbind_request
blows up before tools execute. Please keep the Pydantic fast path but fall back to a plain shallow copy whenmodel_copy
is absent.+import copy @@ - bound: Context = self.model_copy(deep=False) + bound: Context = ( + self.model_copy(deep=False) # type: ignore[attr-defined] + if hasattr(self, "model_copy") + else copy.copy(self) + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/mcp_agent/app.py
(2 hunks)src/mcp_agent/cli/cloud/commands/deploy/main.py
(3 hunks)src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py
(1 hunks)src/mcp_agent/core/context.py
(5 hunks)src/mcp_agent/server/app_server.py
(6 hunks)src/mcp_agent/server/tool_adapter.py
(3 hunks)tests/core/test_context.py
(1 hunks)tests/server/test_app_server.py
(1 hunks)tests/server/test_tool_decorators.py
(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-28T15:07:10.015Z
Learnt from: saqadri
PR: lastmile-ai/mcp-agent#386
File: src/mcp_agent/mcp/mcp_server_registry.py:110-116
Timestamp: 2025-08-28T15:07:10.015Z
Learning: In MCP server registry methods, when client_session_factory parameters are updated to accept additional context parameters, ensure the type hints match what is actually passed (Context instance vs ServerSession) and that the default factory (MCPAgentClientSession) can handle the number of arguments being passed to avoid TypeError at runtime.
Applied to files:
src/mcp_agent/server/app_server.py
📚 Learning: 2025-07-22T18:59:49.368Z
Learnt from: CR
PR: lastmile-ai/mcp-agent#0
File: examples/usecases/reliable_conversation/CLAUDE.md:0-0
Timestamp: 2025-07-22T18:59:49.368Z
Learning: Applies to examples/usecases/reliable_conversation/examples/reliable_conversation/src/**/*.py : Use mcp-agent's Agent abstraction for ALL LLM interactions, including quality evaluation, to ensure consistent tool access, logging, and error handling.
Applied to files:
src/mcp_agent/core/context.py
🧬 Code graph analysis (9)
src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (1)
src/mcp_agent/cli/utils/ux.py (1)
print_info
(32-49)
tests/server/test_app_server.py (2)
src/mcp_agent/executor/workflow.py (1)
create
(148-167)src/mcp_agent/app.py (1)
context
(165-170)
src/mcp_agent/app.py (1)
src/mcp_agent/core/context.py (3)
mcp
(105-106)fastmcp
(109-124)Context
(60-313)
tests/server/test_tool_decorators.py (5)
src/mcp_agent/server/app_server.py (4)
app
(135-137)workflows
(140-142)_workflow_run
(1858-2061)workflow_registry
(145-147)src/mcp_agent/app.py (7)
MCPApp
(54-1035)context
(165-170)initialize
(228-331)upstream_session
(189-190)upstream_session
(193-194)workflows
(197-198)workflow
(438-472)src/mcp_agent/core/context.py (5)
Context
(60-313)mcp
(105-106)fastmcp
(109-124)name
(185-188)session
(127-162)src/mcp_agent/executor/temporal/workflow_registry.py (1)
get_workflow
(66-77)src/mcp_agent/executor/workflow_registry.py (2)
get_workflow
(69-82)get_workflow
(242-253)
src/mcp_agent/server/tool_adapter.py (1)
src/mcp_agent/server/app_server.py (1)
_annotation_is_fast_ctx
(1459-1475)
src/mcp_agent/server/app_server.py (4)
src/mcp_agent/core/context.py (5)
mcp
(105-106)fastmcp
(109-124)Context
(60-313)name
(185-188)bind_request
(198-219)src/mcp_agent/server/tool_adapter.py (1)
_annotation_is_fast_ctx
(41-57)src/mcp_agent/app.py (4)
upstream_session
(189-190)upstream_session
(193-194)context
(165-170)workflow
(438-472)src/mcp_agent/executor/workflow.py (1)
create
(148-167)
src/mcp_agent/cli/cloud/commands/deploy/main.py (1)
src/mcp_agent/cli/utils/ux.py (2)
print_info
(32-49)print_success
(52-63)
tests/core/test_context.py (2)
src/mcp_agent/core/context.py (10)
Context
(60-313)logger
(165-182)read_resource
(290-313)session
(127-162)mcp
(105-106)fastmcp
(109-124)bind_request
(198-219)log
(239-273)name
(185-188)description
(191-194)src/mcp_agent/logging/logger.py (1)
Logger
(34-311)
src/mcp_agent/core/context.py (3)
src/mcp_agent/server/app_server.py (2)
app
(135-137)workflow_registry
(145-147)src/mcp_agent/app.py (6)
MCPApp
(54-1035)logger
(209-226)upstream_session
(189-190)upstream_session
(193-194)session_id
(205-206)context
(165-170)src/mcp_agent/logging/logger.py (2)
Logger
(34-311)get_logger
(513-541)
def _annotation_is_fast_ctx(annotation) -> bool: | ||
if _Ctx is None or annotation is inspect._empty: | ||
return False | ||
if annotation is _Ctx: | ||
return True | ||
try: | ||
origin = _typing.get_origin(annotation) | ||
if origin is not None: | ||
return any( | ||
_annotation_is_fast_ctx(arg) for arg in _typing.get_args(annotation) | ||
) | ||
except Exception: | ||
pass | ||
try: | ||
return "fastmcp" in str(annotation) | ||
except Exception: | ||
return False | ||
|
||
existing_ctx_param = None | ||
for param in sig.parameters.values(): | ||
if param.name == "app_ctx": | ||
continue | ||
annotation = param.annotation | ||
if annotation is inspect._empty and param.name in ("ctx", "context"): | ||
existing_ctx_param = param.name | ||
break | ||
if _annotation_is_fast_ctx(annotation): | ||
existing_ctx_param = param.name | ||
break | ||
return_ann = sig.return_annotation | ||
|
||
# Copy annotations and remove app_ctx | ||
ann = dict(getattr(fn, "__annotations__", {})) | ||
ann.pop("app_ctx", None) | ||
|
||
# Add ctx parameter annotation | ||
ctx_param_name = "ctx" | ||
ann[ctx_param_name] = _Ctx | ||
# Determine context parameter name | ||
ctx_param_name = existing_ctx_param or "ctx" | ||
if _Ctx is not None: | ||
ann[ctx_param_name] = _Ctx | ||
ann["return"] = getattr(fn, "__annotations__", {}).get("return", return_ann) | ||
|
||
# Filter parameters to remove app_ctx | ||
params = [p for p in sig.parameters.values() if p.name != "app_ctx"] | ||
|
||
# Create ctx parameter | ||
ctx_param = inspect.Parameter( | ||
ctx_param_name, | ||
kind=inspect.Parameter.KEYWORD_ONLY, | ||
annotation=_Ctx, | ||
) | ||
# Filter parameters to remove app_ctx and, when needed, ctx/context placeholders | ||
params = [] | ||
for p in sig.parameters.values(): | ||
if p.name == "app_ctx": | ||
continue | ||
if existing_ctx_param is None and ( | ||
(p.annotation is inspect._empty and p.name in ("ctx", "context")) | ||
or _annotation_is_fast_ctx(p.annotation) | ||
): | ||
continue | ||
params.append(p) | ||
|
||
# Create ctx parameter when not already present | ||
if existing_ctx_param is None: | ||
ctx_param = inspect.Parameter( | ||
ctx_param_name, | ||
kind=inspect.Parameter.KEYWORD_ONLY, | ||
annotation=_Ctx, | ||
) | ||
signature_params = params + [ctx_param] | ||
else: | ||
signature_params = params |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle Context subclasses when detecting existing FastMCP ctx parameters
If a tool already declares ctx: Context
(our Context now subclasses FastMCP’s Context), _annotation_is_fast_ctx
returns False
. We keep that parameter in params
and then append a new keyword-only ctx
, so inspect.Signature
raises ValueError: duplicate parameter name: 'ctx'
. That breaks tool registration for perfectly valid signatures. Please treat subclasses of _Ctx
as fast-context annotations so we reuse the existing parameter instead of adding another.
Apply this diff:
@@
- if annotation is _Ctx:
+ if inspect.isclass(annotation):
+ try:
+ if issubclass(annotation, _Ctx):
+ return True
+ except TypeError:
+ pass
+ if annotation is _Ctx:
return True
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
def _annotation_is_fast_ctx(annotation) -> bool: | |
if _Ctx is None or annotation is inspect._empty: | |
return False | |
if annotation is _Ctx: | |
return True | |
try: | |
origin = _typing.get_origin(annotation) | |
if origin is not None: | |
return any( | |
_annotation_is_fast_ctx(arg) for arg in _typing.get_args(annotation) | |
) | |
except Exception: | |
pass | |
try: | |
return "fastmcp" in str(annotation) | |
except Exception: | |
return False | |
existing_ctx_param = None | |
for param in sig.parameters.values(): | |
if param.name == "app_ctx": | |
continue | |
annotation = param.annotation | |
if annotation is inspect._empty and param.name in ("ctx", "context"): | |
existing_ctx_param = param.name | |
break | |
if _annotation_is_fast_ctx(annotation): | |
existing_ctx_param = param.name | |
break | |
return_ann = sig.return_annotation | |
# Copy annotations and remove app_ctx | |
ann = dict(getattr(fn, "__annotations__", {})) | |
ann.pop("app_ctx", None) | |
# Add ctx parameter annotation | |
ctx_param_name = "ctx" | |
ann[ctx_param_name] = _Ctx | |
# Determine context parameter name | |
ctx_param_name = existing_ctx_param or "ctx" | |
if _Ctx is not None: | |
ann[ctx_param_name] = _Ctx | |
ann["return"] = getattr(fn, "__annotations__", {}).get("return", return_ann) | |
# Filter parameters to remove app_ctx | |
params = [p for p in sig.parameters.values() if p.name != "app_ctx"] | |
# Create ctx parameter | |
ctx_param = inspect.Parameter( | |
ctx_param_name, | |
kind=inspect.Parameter.KEYWORD_ONLY, | |
annotation=_Ctx, | |
) | |
# Filter parameters to remove app_ctx and, when needed, ctx/context placeholders | |
params = [] | |
for p in sig.parameters.values(): | |
if p.name == "app_ctx": | |
continue | |
if existing_ctx_param is None and ( | |
(p.annotation is inspect._empty and p.name in ("ctx", "context")) | |
or _annotation_is_fast_ctx(p.annotation) | |
): | |
continue | |
params.append(p) | |
# Create ctx parameter when not already present | |
if existing_ctx_param is None: | |
ctx_param = inspect.Parameter( | |
ctx_param_name, | |
kind=inspect.Parameter.KEYWORD_ONLY, | |
annotation=_Ctx, | |
) | |
signature_params = params + [ctx_param] | |
else: | |
signature_params = params | |
def _annotation_is_fast_ctx(annotation) -> bool: | |
if _Ctx is None or annotation is inspect._empty: | |
return False | |
if inspect.isclass(annotation): | |
try: | |
if issubclass(annotation, _Ctx): | |
return True | |
except TypeError: | |
pass | |
if annotation is _Ctx: | |
return True | |
try: | |
origin = _typing.get_origin(annotation) | |
if origin is not None: | |
return any( | |
_annotation_is_fast_ctx(arg) for arg in _typing.get_args(annotation) | |
) | |
except Exception: | |
pass | |
try: | |
return "fastmcp" in str(annotation) | |
except Exception: | |
return False |
…rs, such as title, icons and meta
def _detect_context_param(signature: inspect.Signature) -> str | None: | ||
for param in signature.parameters.values(): | ||
if param.name == "app_ctx": | ||
continue | ||
if _annotation_is_fast_ctx(param.annotation): | ||
return param.name | ||
if param.annotation is inspect._empty and param.name in {"ctx", "context"}: | ||
return param.name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _detect_context_param
function checks for FastMCP context parameters but doesn't handle parameters annotated with Context
from mcp_agent.core.context
. Since this PR makes the agent's Context
a derivative of FastMCP's context, it would be more robust to also detect parameters annotated with the agent's Context
class. This would ensure consistent behavior when developers use either context type in their function signatures.
def _detect_context_param(signature: inspect.Signature) -> str | None: | |
for param in signature.parameters.values(): | |
if param.name == "app_ctx": | |
continue | |
if _annotation_is_fast_ctx(param.annotation): | |
return param.name | |
if param.annotation is inspect._empty and param.name in {"ctx", "context"}: | |
return param.name | |
def _detect_context_param(signature: inspect.Signature) -> str | None: | |
for param in signature.parameters.values(): | |
if param.name == "app_ctx": | |
continue | |
if _annotation_is_fast_ctx(param.annotation): | |
return param.name | |
if _annotation_is_agent_ctx(param.annotation): | |
return param.name | |
if param.annotation is inspect._empty and param.name in {"ctx", "context"}: | |
return param.name | |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcp_agent/server/app_server.py (1)
1671-1733
: Async wrapper: same annotation issue for existing context param.Mirror the sync-path fix so the signature carries the _Ctx annotation when a custom-named FastMCP Context param exists.
params = [] if sig_async is not None: for p in sig_async.parameters.values(): if p.name == "app_ctx": continue - if existing_ctx_param is None and ( + if existing_ctx_param is None and ( _annotation_is_fast_ctx(p.annotation) or p.name in ("ctx", "context") ): continue params.append(p) # Append kw-only context param - if existing_ctx_param is None: + if existing_ctx_param is None: if _Ctx is not None: ctx_param = inspect.Parameter( ctx_param_name, kind=inspect.Parameter.KEYWORD_ONLY, annotation=_Ctx, ) else: ctx_param = inspect.Parameter( ctx_param_name, kind=inspect.Parameter.KEYWORD_ONLY, ) signature_params = params + [ctx_param] else: - signature_params = params + # Ensure the existing context param is annotated on the signature + if _Ctx is not None: + params = [ + (p.replace(annotation=_Ctx) if p.name == existing_ctx_param else p) + for p in params + ] + signature_params = params
🧹 Nitpick comments (4)
tests/server/test_tool_decorators.py (1)
112-129
: Good end-to-end sync-tool wrap and bound context assertions.Validates unwrapping to plain return, and checks bound_app_context presence/distinctness and metadata types.
Minor: consider asserting that no workflows-* aliases were decorated for sync tools (you already compute decorated_names).
src/mcp_agent/server/app_server.py (1)
1452-1486
: Duplicate FastMCP-context detection logic — centralize to avoid drift._local
_annotation_is_fast_ctx
mirrors tool_adapter.py. Prefer importing and reusing a single implementation to keep behavior consistent across adapters.Apply by moving detection helpers to mcp_agent.server.tool_adapter and importing here.
src/mcp_agent/app.py (2)
609-627
: Annotation check for app_ctx is string-based; prefer direct type-aware check.You import Context at module scope. Replace string search with a typing-aware check to support Optional/Union annotations cleanly.
- if param.annotation != _inspect.Parameter.empty: - ann_str = str(param.annotation) - if "mcp_agent.core.context.Context" in ann_str: + if param.annotation != _inspect._empty: + from typing import get_origin, get_args, Union + ann = param.annotation + if ann is Context or Context in (get_args(ann) or ()): app_context_param_name = param_name break
912-915
: Duplicate import of validate_tool_schema.Already imported at module top. Keep a single import unless there’s a proven cycle.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lock
is excluded by!**/*.lock
📒 Files selected for processing (4)
pyproject.toml
(1 hunks)src/mcp_agent/app.py
(10 hunks)src/mcp_agent/server/app_server.py
(9 hunks)tests/server/test_tool_decorators.py
(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: saqadri
PR: lastmile-ai/mcp-agent#386
File: src/mcp_agent/mcp/mcp_server_registry.py:110-116
Timestamp: 2025-08-28T15:07:10.015Z
Learning: In MCP server registry methods, when client_session_factory parameters are updated to accept additional context parameters, ensure the type hints match what is actually passed (Context instance vs ServerSession) and that the default factory (MCPAgentClientSession) can handle the number of arguments being passed to avoid TypeError at runtime.
📚 Learning: 2025-08-28T15:07:10.015Z
Learnt from: saqadri
PR: lastmile-ai/mcp-agent#386
File: src/mcp_agent/mcp/mcp_server_registry.py:110-116
Timestamp: 2025-08-28T15:07:10.015Z
Learning: In MCP server registry methods, when client_session_factory parameters are updated to accept additional context parameters, ensure the type hints match what is actually passed (Context instance vs ServerSession) and that the default factory (MCPAgentClientSession) can handle the number of arguments being passed to avoid TypeError at runtime.
Applied to files:
src/mcp_agent/server/app_server.py
🧬 Code graph analysis (3)
tests/server/test_tool_decorators.py (3)
src/mcp_agent/server/app_server.py (3)
app
(135-137)workflows
(140-142)_workflow_run
(1869-2072)src/mcp_agent/app.py (13)
MCPApp
(57-1124)context
(168-173)tool
(766-766)tool
(769-779)tool
(781-871)async_tool
(874-874)async_tool
(877-887)async_tool
(889-976)initialize
(231-334)upstream_session
(192-193)upstream_session
(196-197)workflows
(200-201)workflow
(441-475)src/mcp_agent/core/context.py (6)
Context
(60-313)mcp
(105-106)fastmcp
(109-124)name
(185-188)description
(191-194)session
(127-162)
src/mcp_agent/server/app_server.py (3)
src/mcp_agent/core/context.py (5)
mcp
(105-106)fastmcp
(109-124)Context
(60-313)name
(185-188)bind_request
(198-219)src/mcp_agent/server/tool_adapter.py (1)
_annotation_is_fast_ctx
(41-57)src/mcp_agent/executor/workflow.py (1)
create
(148-167)
src/mcp_agent/app.py (1)
src/mcp_agent/core/context.py (3)
mcp
(105-106)fastmcp
(109-124)Context
(60-313)
⏰ 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: checks / test
🔇 Additional comments (6)
tests/server/test_tool_decorators.py (3)
81-89
: Metadata plumbing for @app.tool looks good.Title/annotations/icons/meta/structured_output are verified on registration; this aligns with the new declared-tool shape.
154-161
: Async tool registration and metadata checks are correct.Aliases suppressed and metadata normalized; looks consistent with server registration.
Also applies to: 174-184
229-291
: Per-request binding test is robust.Covers upstream_session preservation, FastMCP bridge, request_context propagation, and workflow _mcp_request_context. Solid coverage.
src/mcp_agent/server/app_server.py (1)
1884-1910
: Per-request bound context wiring looks correct.Binding via Context.bind_request, exposing ctx.bound_app_context, and creating workflows with the bound context align with tests.
Please confirm this also runs under Temporal engine by executing the Temporal-backed tests if present.
Also applies to: 1924-1937
src/mcp_agent/app.py (1)
858-866
: Bare-usage guard is correct and avoids ambiguity.The extra structured_output check is sensible.
Also applies to: 963-971
pyproject.toml (1)
21-21
: Add upper bound constraint to mcp dependency for version stability.The upgrade from 1.16.0 to 1.18.0 spans two consecutive releases (1.16.0 Oct 2, 1.17.0 Oct 10, 1.18.0 Oct 16, 2025). Based on the codebase analysis and documentation review:
- Context injection via type hints is a stable, automatically-injected API across all versions
- Tool schema generation maintains backward compatibility; unstructured results are provided for compatibility, and classification can be suppressed with the
structured_output=False
parameter- No breaking API changes detected between these versions
Recommendation: Modify the constraint to
"mcp>=1.18.0,<2.0"
to prevent accidental major version upgrades that could introduce protocol-level breaking changes, while allowing safe minor/patch updates."mcp>=1.18.0,<2.0",
def _is_fast_ctx_annotation(annotation) -> bool: | ||
if _Ctx is None or annotation is _inspect._empty: | ||
return False | ||
if annotation is _Ctx: | ||
return True | ||
try: | ||
origin = _typing.get_origin(annotation) | ||
if origin is not None: | ||
return any( | ||
_is_fast_ctx_annotation(arg) | ||
for arg in _typing.get_args(annotation) | ||
) | ||
except Exception: | ||
pass | ||
try: | ||
return "fastmcp" in str(annotation) | ||
except Exception: | ||
return False | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: injecting FastMCP server where a FastMCP Context is expected.
In the fallback, you set fast_ctx = app_ctx.fastmcp (a FastMCP server), but the function param expects mcp.server.fastmcp.Context. This is the wrong type and will break consumers outside the usual request path.
Use FastMCP.get_context() when available; otherwise leave None rather than injecting the server.
- elif p.annotation is _inspect._empty and p.name in (
+ elif p.annotation is _inspect._empty and p.name in (
"ctx",
"context",
):
needs_fast_ctx = True
if needs_fast_ctx and p.name not in call_kwargs:
- fast_ctx = getattr(workflow_self, "_mcp_request_context", None)
- if fast_ctx is None and app_context_param_name:
- fast_ctx = getattr(
- call_kwargs.get(app_context_param_name, None),
- "fastmcp",
- None,
- )
- call_kwargs[p.name] = fast_ctx
+ fast_ctx = getattr(workflow_self, "_mcp_request_context", None)
+ if fast_ctx is None and app_context_param_name:
+ _app_ctx = call_kwargs.get(app_context_param_name, None)
+ _fastmcp = getattr(_app_ctx, "fastmcp", None)
+ if _fastmcp and hasattr(_fastmcp, "get_context"):
+ try:
+ fast_ctx = _fastmcp.get_context()
+ except Exception:
+ fast_ctx = None
+ if fast_ctx is not None:
+ call_kwargs[p.name] = fast_ctx
Also applies to: 656-674
🤖 Prompt for AI Agents
In src/mcp_agent/app.py around lines 635-653 (and similarly 656-674), the
fallback logic wrongly injects app_ctx.fastmcp (the FastMCP server) into
parameters that expect a FastMCP Context; replace that behavior so you call
FastMCP.get_context() when available and use its result, and if get_context() is
not present or returns None then leave the parameter as None instead of
assigning the server instance; update any conditional branches to check for a
get_context attribute on app_ctx.fastmcp and prefer its return value, removing
the direct assignment of the server to avoid supplying the wrong type to
consumers.
existing_ctx_param = _detect_context_param(sig) | ||
ctx_param_name = existing_ctx_param or "ctx" | ||
|
||
ann[ctx_param_name] = _Ctx | ||
if _Ctx is not None: | ||
ann[ctx_param_name] = _Ctx | ||
ann["return"] = getattr(fn, "__annotations__", {}).get("return", return_ann) | ||
_wrapper.__annotations__ = ann | ||
_wrapper.__name__ = name_local | ||
_wrapper.__doc__ = description or (fn.__doc__ or "") | ||
|
||
params = [p for p in sig.parameters.values() if p.name != "app_ctx"] | ||
ctx_param = inspect.Parameter( | ||
ctx_param_name, | ||
kind=inspect.Parameter.KEYWORD_ONLY, | ||
annotation=_Ctx, | ||
) | ||
if existing_ctx_param is None: | ||
ctx_param = inspect.Parameter( | ||
ctx_param_name, | ||
kind=inspect.Parameter.KEYWORD_ONLY, | ||
annotation=_Ctx, | ||
) | ||
signature_params = params + [ctx_param] | ||
else: | ||
signature_params = params | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Existing context param loses type annotation in signature; may break FastMCP injection for custom param names.
When a function already has a FastMCP Context parameter under a non-standard name (not “ctx/context”), you add annotations but reuse original Parameter objects in signature, leaving their annotation unchanged. Frameworks often consult Parameter.annotation on the signature, not function.annotations, so context injection may not trigger.
Proposed fix: replace that parameter with one annotated as _Ctx when building the wrapper signature.
params = [p for p in sig.parameters.values() if p.name != "app_ctx"]
- if existing_ctx_param is None:
+ # Ensure the existing context param is annotated on the signature
+ if existing_ctx_param is not None and _Ctx is not None:
+ params = [
+ (p.replace(annotation=_Ctx) if p.name == existing_ctx_param else p)
+ for p in params
+ ]
+ if existing_ctx_param is None:
ctx_param = inspect.Parameter(
ctx_param_name,
kind=inspect.Parameter.KEYWORD_ONLY,
annotation=_Ctx,
)
signature_params = params + [ctx_param]
else:
signature_params = params
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
existing_ctx_param = _detect_context_param(sig) | |
ctx_param_name = existing_ctx_param or "ctx" | |
ann[ctx_param_name] = _Ctx | |
if _Ctx is not None: | |
ann[ctx_param_name] = _Ctx | |
ann["return"] = getattr(fn, "__annotations__", {}).get("return", return_ann) | |
_wrapper.__annotations__ = ann | |
_wrapper.__name__ = name_local | |
_wrapper.__doc__ = description or (fn.__doc__ or "") | |
params = [p for p in sig.parameters.values() if p.name != "app_ctx"] | |
ctx_param = inspect.Parameter( | |
ctx_param_name, | |
kind=inspect.Parameter.KEYWORD_ONLY, | |
annotation=_Ctx, | |
) | |
if existing_ctx_param is None: | |
ctx_param = inspect.Parameter( | |
ctx_param_name, | |
kind=inspect.Parameter.KEYWORD_ONLY, | |
annotation=_Ctx, | |
) | |
signature_params = params + [ctx_param] | |
else: | |
signature_params = params | |
existing_ctx_param = _detect_context_param(sig) | |
ctx_param_name = existing_ctx_param or "ctx" | |
if _Ctx is not None: | |
ann[ctx_param_name] = _Ctx | |
ann["return"] = getattr(fn, "__annotations__", {}).get("return", return_ann) | |
_wrapper.__annotations__ = ann | |
_wrapper.__name__ = name_local | |
_wrapper.__doc__ = description or (fn.__doc__ or "") | |
params = [p for p in sig.parameters.values() if p.name != "app_ctx"] | |
# Ensure the existing context param is annotated on the signature | |
if existing_ctx_param is not None and _Ctx is not None: | |
params = [ | |
(p.replace(annotation=_Ctx) if p.name == existing_ctx_param else p) | |
for p in params | |
] | |
if existing_ctx_param is None: | |
ctx_param = inspect.Parameter( | |
ctx_param_name, | |
kind=inspect.Parameter.KEYWORD_ONLY, | |
annotation=_Ctx, | |
) | |
signature_params = params + [ctx_param] | |
else: | |
signature_params = params |
🤖 Prompt for AI Agents
In src/mcp_agent/server/app_server.py around lines 1604 to 1624, the wrapper
builds __signature__ using the original Parameter objects when an existing
context parameter has a non-standard name, so its Parameter.annotation remains
unchanged; replace that parameter with a new inspect.Parameter that has the same
name and kind but annotation set to _Ctx (preserving
POSITIONAL_ONLY/POSITIONAL_OR_KEYWORD/KEYWORD_ONLY and default value) before
assigning signature_params so the wrapper.signature reflects the context type
and allows FastMCP injection.
"title": title, | ||
"annotations": annotations_obj, | ||
"icons": icons_list, | ||
"meta": meta_payload, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be _meta
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm I looked at the MCP SDK and it looks like the decorator has meta, not _meta. Where did you see _meta?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mcp_agent/cli/cloud/commands/deploy/main.py
(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: saqadri
PR: lastmile-ai/mcp-agent#386
File: src/mcp_agent/mcp/mcp_server_registry.py:110-116
Timestamp: 2025-08-28T15:07:10.015Z
Learning: In MCP server registry methods, when client_session_factory parameters are updated to accept additional context parameters, ensure the type hints match what is actually passed (Context instance vs ServerSession) and that the default factory (MCPAgentClientSession) can handle the number of arguments being passed to avoid TypeError at runtime.
🧬 Code graph analysis (1)
src/mcp_agent/cli/cloud/commands/deploy/main.py (2)
src/mcp_agent/cli/utils/ux.py (2)
print_info
(32-49)print_success
(52-63)src/mcp_agent/cli/secrets/processor.py (1)
process_config_secrets
(41-153)
🔇 Additional comments (4)
src/mcp_agent/cli/cloud/commands/deploy/main.py (4)
172-178
: Approved: UX improvement, but unrelated to PR objectives.The addition of the print statement at line 178 improves user experience by informing users when the default app name is used, maintaining consistency with line 174. However, this change appears unrelated to the PR's stated goal of making Context a derivative of FastMCP context.
204-204
: Cosmetic change unrelated to PR objectives.This whitespace addition has no functional impact but is inconsistent with the PR's stated objectives about Context.
230-230
: Cosmetic formatting change unrelated to PR objectives.This formatting change has no functional impact and appears unrelated to the PR's stated goal of making Context a derivative of FastMCP context.
289-289
: Function call correctly updated to match import refactor.The call site correctly uses the new import alias. This change is functionally equivalent to the previous direct function call, assuming the import at line 27 is valid.
from mcp_agent.cli.secrets.processor import ( | ||
process_config_secrets, | ||
) | ||
from mcp_agent.cli.secrets import processor as secrets_processor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Inconsistent with PR objectives: verify import path.
This import refactor appears unrelated to the PR's stated goal of making Context a derivative of FastMCP context. The change from direct function import to module alias assumes that mcp_agent.cli.secrets
exports the processor
module.
Run the following script to verify the import path is correct:
🏁 Script executed:
#!/bin/bash
# Verify that mcp_agent.cli.secrets exports processor module
# Check if __init__.py exists and what it exports
fd -t f "__init__.py" src/mcp_agent/cli/secrets/ --exec cat {}
# Verify processor.py exists
fd -t f "processor.py" src/mcp_agent/cli/secrets/
Length of output: 339
Critical issue: Import bypasses package's public API.
The import at line 27 attempts to import processor
directly from mcp_agent.cli.secrets
, but the package's __init__.py
does not export it in __all__
. While the processor.py
file exists, this import violates the package's intended public API.
Update the import to use the explicit module path: from mcp_agent.cli.secrets.processor import process_config_secrets
or add processor
to the package's __all__
list in __init__.py
.
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/commands/deploy/main.py around line 27, the code
imports the submodule via the package public API (importing processor from
mcp_agent.cli.secrets) which is not exported in the package __all__; change the
import to explicitly import the symbol from the concrete module (import
process_config_secrets from mcp_agent.cli.secrets.processor) and update any
local references to call that symbol, or alternatively add "processor" to
mcp_agent/cli/secrets/__init__.py __all__ so the original import is valid.
This makes it much easier to interface with the MCP server session everywhere that
Context
is available.It's especially useful for @app.tool decorated functions:
@app.tool
def my_agent_fn(params: Any, app_ctx: Context) -> str:
app_ctx.elicit(...)
app_ctx.log(...)
return "Mission accomplished"
Summary by CodeRabbit
New Features
Improvements
Refactor
Tests