update mcp dependency; add ability to add icons #560
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. WalkthroughAdds icon support across the app: introduces a default Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant App as MCPApp
participant Decor as @app.tool / @app.async_tool
participant Server as FastMCP / MCP Server
participant Client as MCP Client
rect rgb(248,249,250)
Dev->>App: instantiate MCPApp(name, ..., icons=?)
note right of App: if icons omitted -> App._icons = [phetch]
end
rect rgb(255,250,240)
Dev->>Decor: decorate function with icons=?
note right of Decor: decorator uses supplied icons or defaults to [phetch]\nand emits tool metadata including "icons"
Decor->>App: register tool metadata (includes icons)
end
rect rgb(240,255,240)
App->>Server: create_mcp_server_for_app(..., icons=?)
note right of Server: if "icons" not in kwargs and App._icons -> kwargs["icons"]=App._icons
Server->>Client: expose tool declarations (metadata includes icons)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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 |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
src/mcp_agent/app.py
Outdated
| icon_data = base64.standard_b64encode(icon_path.read_bytes()).decode() | ||
| icon_data_uri = f"data:image/png;base64,{icon_data}" | ||
|
|
||
| self.icons = [Icon(src=icon_data_uri, mimeType="image/png", sizes=["48x48"])] |
There was a problem hiding this comment.
There appears to be an inconsistency in attribute naming. The code uses self._icons on line 139-142 but then assigns to self.icons without the underscore. For consistency with the private attribute naming convention used elsewhere in this class, this should be changed to self._icons = [Icon(...)].
| self.icons = [Icon(src=icon_data_uri, mimeType="image/png", sizes=["48x48"])] | |
| self._icons = [Icon(src=icon_data_uri, mimeType="image/png", sizes=["48x48"])] |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
|
Converted to draft. Earlier all tests were passing, but I rebased off the latest main and I see some test failures now |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
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)
1716-1721: Propagate declared icons to async tools.The async run tool ignores the icons from the decorator, diverging from the PR intent. Pass icons through:
- mcp.add_tool( + mcp.add_tool( _async_adapter, name=run_tool_name, description=full_desc, structured_output=False, + icons=icons, )
🧹 Nitpick comments (4)
pyproject.toml (1)
21-21: Verify MCP 1.18 compatibility and consider upper bound.mcp>=1.18.0 aligns with the new Icon API. Please confirm no breaking changes vs 1.13.x and consider adding an upper bound (e.g., >=1.18.0,<2.0.0) to avoid accidental major upgrades.
src/mcp_agent/app.py (2)
757-764: Update type overload to include the new icons parameter.The runtime signature has icons but the overload omits it, causing typing drift.
Add this overload (replacing the existing one):
@overload def tool( self, name: str | None = None, *, description: str | None = None, structured_output: bool | None = None, + icons: list[Icon] | None = None, ) -> Callable[[Callable[P, R]], Callable[P, R]]: ...
824-831: Add icons to async_tool overload as well.Mirror the runtime param in the overload:
@overload def async_tool( self, name: str | None = None, *, description: str | None = None, + icons: list[Icon] | None = None, ) -> Callable[[Callable[P, R]], Callable[P, R]]: ...src/mcp_agent/server/app_server.py (1)
1091-1091: Guard icons=[phetch] when phetch may be None.If default icon loading fails per earlier comment, register tools without icons:
-@mcp.tool(name="workflows-list", icons=[phetch]) +@mcp.tool(name="workflows-list", icons=([phetch] if phetch else None))Apply similarly to workflows-runs-list, workflows-run, workflows-get_status, workflows-resume, workflows-cancel, and workflows-{name}-run.
Also applies to: 1134-1134, 1191-1191, 1218-1218, 1258-1258, 1332-1332, 1759-1776
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
phetch.pngis excluded by!**/*.pnguv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
README.md(1 hunks)pyproject.toml(1 hunks)src/mcp_agent/app.py(8 hunks)src/mcp_agent/server/app_server.py(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/mcp_agent/app.py (1)
src/mcp_agent/core/context.py (1)
mcp(102-103)
src/mcp_agent/server/app_server.py (2)
src/mcp_agent/core/context.py (2)
mcp(102-103)Context(57-103)src/mcp_agent/app.py (3)
tool(746-746)tool(749-755)tool(757-811)
🪛 GitHub Actions: Pull Request Checks
src/mcp_agent/server/app_server.py
[error] 1049-1049: Test for membership should be not in. Convert to not in. Found 1 error. 1 fixable with the --fix option.
src/mcp_agent/server/app_server.py
Outdated
| icon_path = Path(__file__).parent.parent.parent.parent / "phetch.png" | ||
| icon_data = base64.standard_b64encode(icon_path.read_bytes()).decode() | ||
| icon_data_uri = f"data:image/png;base64,{icon_data}" | ||
| phetch = Icon(src=icon_data_uri, mimeType="image/png", sizes=["48x48"]) | ||
|
|
There was a problem hiding this comment.
Package-safe default icon loading.
Reading ../../../../phetch.png will fail in installed wheels. Load from a packaged resource and handle absence gracefully:
-icon_path = Path(__file__).parent.parent.parent.parent / "phetch.png"
-icon_data = base64.standard_b64encode(icon_path.read_bytes()).decode()
-icon_data_uri = f"data:image/png;base64,{icon_data}"
-phetch = Icon(src=icon_data_uri, mimeType="image/png", sizes=["48x48"])
+try:
+ from importlib import resources as ir
+ with ir.files("mcp_agent.resources").joinpath("phetch.png").open("rb") as f:
+ icon_b64 = base64.b64encode(f.read()).decode()
+ icon_data_uri = f"data:image/png;base64,{icon_b64}"
+ phetch = Icon(src=icon_data_uri, mimeType="image/png", sizes=["48x48"])
+except Exception:
+ phetch = None # Fallback: no iconWhen phetch can be None, gate decorators with icons=[phetch] to icons=([phetch] if phetch else None).
📝 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.
| icon_path = Path(__file__).parent.parent.parent.parent / "phetch.png" | |
| icon_data = base64.standard_b64encode(icon_path.read_bytes()).decode() | |
| icon_data_uri = f"data:image/png;base64,{icon_data}" | |
| phetch = Icon(src=icon_data_uri, mimeType="image/png", sizes=["48x48"]) | |
| try: | |
| from importlib import resources as ir | |
| with ir.files("mcp_agent.resources").joinpath("phetch.png").open("rb") as f: | |
| icon_b64 = base64.b64encode(f.read()).decode() | |
| icon_data_uri = f"data:image/png;base64,{icon_b64}" | |
| phetch = Icon(src=icon_data_uri, mimeType="image/png", sizes=["48x48"]) | |
| except Exception: | |
| phetch = None # Fallback: no icon |
🤖 Prompt for AI Agents
In src/mcp_agent/server/app_server.py around lines 50 to 54, loading the icon by
reading ../../../../phetch.png from the filesystem will break when installed as
a wheel; instead use importlib.resources (or pkgutil) to load the packaged
resource and wrap reading in a try/except to gracefully handle absence (set
phetch = None on failure). Convert the raw bytes to a base64 data URI only if
the resource was found. Finally update any decorator usages to pass
icons=([phetch] if phetch else None) so decorators receive None when the icon is
missing.
src/mcp_agent/server/app_server.py
Outdated
|
|
||
| icon_path = Path(__file__).parent.parent.parent.parent / "phetch.png" | ||
| icon_data = base64.standard_b64encode(icon_path.read_bytes()).decode() | ||
| icon_data_uri = f"data:image/png;base64,{icon_data}" |
There was a problem hiding this comment.
Does this have to be encoded or can it point to a CDN URL or S3 bucket?
There was a problem hiding this comment.
It an point to a URL as well. Updated the README with an example to point that out.
There was a problem hiding this comment.
Oh awesome. In that case, should the default phetch icon be hosted on S3? That would simplify needing to encode it on each MCPApp or app_server setup
There was a problem hiding this comment.
@saqadri ah, that's what you were after. Great suggestion. Any particular bucket? I see publicdata.lastmileai.com? Or should we create a new one, like mcpac-artifacts or something?
There was a problem hiding this comment.
publicdata is good. I think that's our existing public bucket
| signal_notification: SignalWaitCallback | None = None, | ||
| upstream_session: Optional["ServerSession"] = None, | ||
| model_selector: ModelSelector | None = None, | ||
| icons: list[Icon] | None = None, |
There was a problem hiding this comment.
Should this be something configurable on the config as well?
There was a problem hiding this comment.
We have to include it as a parameter to the @app.tool decorator, so including it in the MCPApp creator seemed more consistent. Also, the config would work well for external images, but not so much for embedded ones. (I suppose we could detect a file:// url and base64 encode it if we wanted to, but it still would be a bit inconsistent.) Open to changing it, though
|
@roman-van-der-krogt I think I fixed the tests too in #504 after updating the MCP dependency. Not sure if that's the same set of issues but I also had failing tests that I had to address |
roman-van-der-krogt
force-pushed
the
feat/icons
branch
from
c0b7500 to
183e6f5
Compare
src/mcp_agent/app.py
Outdated
| with (resources.files("mcp_agent.data").joinpath("phetch.png").open("rb")) as icon_file: | ||
| icon_data = base64.standard_b64encode(icon_file.read()).decode() | ||
| icon_data_uri = f"data:image/png;base64,{icon_data}" | ||
| phetch = Icon(src=icon_data_uri, mimeType="image/png", sizes=["48x48"]) |
There was a problem hiding this comment.
The code loads the default icon without error handling for missing files. Consider adding a try-except block to gracefully handle cases where phetch.png might not exist in the mcp_agent.data package. This would prevent runtime errors and allow for a fallback icon or empty list when the file is unavailable.
| with (resources.files("mcp_agent.data").joinpath("phetch.png").open("rb")) as icon_file: | |
| icon_data = base64.standard_b64encode(icon_file.read()).decode() | |
| icon_data_uri = f"data:image/png;base64,{icon_data}" | |
| phetch = Icon(src=icon_data_uri, mimeType="image/png", sizes=["48x48"]) | |
| try: | |
| with (resources.files("mcp_agent.data").joinpath("phetch.png").open("rb")) as icon_file: | |
| icon_data = base64.standard_b64encode(icon_file.read()).decode() | |
| icon_data_uri = f"data:image/png;base64,{icon_data}" | |
| phetch = Icon(src=icon_data_uri, mimeType="image/png", sizes=["48x48"]) | |
| except (FileNotFoundError, PermissionError, IOError) as e: | |
| # Fallback to empty icon if the file can't be loaded | |
| phetch = None | |
| logging.warning(f"Could not load icon 'phetch.png': {e}") | |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
| icon_path = Path("my-icon.png") | ||
| icon_data = base64.standard_b64encode(icon_path.read_bytes()).decode() | ||
| icon_data_uri = f"data:image/png;base64,{icon_data}" |
There was a problem hiding this comment.
The example is missing the required import for Path. Consider adding from pathlib import Path to the code example to ensure users can copy and run it without errors.
| icon_path = Path("my-icon.png") | |
| icon_data = base64.standard_b64encode(icon_path.read_bytes()).decode() | |
| icon_data_uri = f"data:image/png;base64,{icon_data}" | |
| from pathlib import Path | |
| import base64 | |
| icon_path = Path("my-icon.png") | |
| icon_data = base64.standard_b64encode(icon_path.read_bytes()).decode() | |
| icon_data_uri = f"data:image/png;base64,{icon_data}" | |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/mcp_agent/server/app_server.py (1)
1042-1043: Fix unsafe attribute access and use Pythonic membership test.This code directly accesses
app._iconswithout checking if the attribute exists, which could raiseAttributeError. Additionally, the membership test syntax is not Pythonic.Apply this diff to use safe attribute access and correct syntax:
- if not "icons" in kwargs and app._icons: - kwargs["icons"] = app._icons + if "icons" not in kwargs and getattr(app, "_icons", None): + kwargs["icons"] = app._icons
🧹 Nitpick comments (1)
src/mcp_agent/app.py (1)
948-959: Consider extracting icon processing logic to reduce duplication.The icon processing logic is duplicated between
@app.tooland@app.async_tooldecorators. While the current implementation is correct, extracting this to a shared helper function would improve maintainability.Consider extracting to a helper:
def _process_icons(icons: Iterable[Icon | Mapping[str, Any]] | None) -> list[Icon]: """Convert icons parameter to list of Icon objects, defaulting to phetch.""" if icons is not None: icons_list = [] for icon in icons: if isinstance(icon, Icon): icons_list.append(icon) elif isinstance(icon, Mapping): icons_list.append(Icon(**icon)) else: raise TypeError("icons entries must be Icon or mapping") return icons_list return [phetch]Then use in both decorators:
icons_list = _process_icons(icons)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/mcp_agent/data/phetch.pngis excluded by!**/*.png
📒 Files selected for processing (4)
README.md(1 hunks)src/mcp_agent/app.py(8 hunks)src/mcp_agent/server/app_server.py(10 hunks)tests/server/test_tool_decorators.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🧬 Code graph analysis (2)
tests/server/test_tool_decorators.py (2)
src/mcp_agent/server/app_server.py (4)
app(135-137)workflows(140-142)create_workflow_tools(1395-1416)create_declared_function_tools(1427-1774)src/mcp_agent/app.py (8)
tool(789-789)tool(792-802)tool(804-896)workflows(210-211)context(178-183)async_tool(899-899)async_tool(902-912)async_tool(914-1004)
src/mcp_agent/server/app_server.py (1)
src/mcp_agent/app.py (4)
MCPApp(62-1152)tool(789-789)tool(792-802)tool(804-896)
🔇 Additional comments (8)
tests/server/test_tool_decorators.py (4)
257-286: LGTM! Test correctly verifies default phetch icon behavior.The test properly validates that when no
iconsparameter is provided to@app.tool, the tool registration receives the default phetch icon.
289-324: LGTM! Test correctly verifies custom icon override behavior.The test validates that custom icons provided via the
iconsparameter properly override the default phetch icon, including both positive and negative assertions.
327-356: LGTM! Async tool test mirrors sync behavior correctly.The test properly validates that
@app.async_toolhas the same default phetch icon behavior as the sync decorator.
359-397: LGTM! Complete test coverage for icon functionality.This test completes the coverage matrix, ensuring both sync and async tool decorators support custom icons. The test suite now comprehensively validates all icon scenarios.
src/mcp_agent/server/app_server.py (2)
1084-1084: LGTM! Consistent icon application across workflow tools.All workflow management tools (
workflows-list,workflows-runs-list,workflows-run,workflows-get_status,workflows-resume,workflows-cancel) consistently use the phetch icon, providing a unified visual identity for the workflow toolset.Also applies to: 1127-1127, 1184-1184, 1211-1211, 1251-1251, 1325-1325
1576-1576: LGTM! Icon propagation correctly threaded through tool registrations.Icons are properly extracted from tool declarations and propagated to both sync and async tool registrations via
mcp.add_tool. The workflow-specific tools also correctly receive the phetch icon.Also applies to: 1660-1660, 1768-1768, 1810-1810
src/mcp_agent/app.py (2)
145-148: LGTM! Icon initialization correctly defaults to phetch.The initialization logic properly uses the
_iconsattribute consistently and provides a sensible default when no custom icons are specified.
838-849: LGTM! Icon processing handles multiple input types correctly.The decorator properly validates icon types, converts Mappings to Icon objects, and provides the phetch default when no icons are specified. Error handling with TypeError is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
examples/mcp_agent_server/temporal/main.py (1)
99-102: Add error handling for icon file loading.Reading
mag.pngwithout error handling will raise an exception if the file is missing or unreadable, preventing the module from importing.Apply this diff to add a fallback:
-icon_file = Path(__file__).parent / "mag.png" -icon_data = base64.standard_b64encode(icon_file.read_bytes()).decode() -icon_data_uri = f"data:image/png;base64,{icon_data}" -mag_icon = Icon(src=icon_data_uri, mimeType="image/png", sizes=["64x64"]) +try: + icon_file = Path(__file__).parent / "mag.png" + icon_data = base64.standard_b64encode(icon_file.read_bytes()).decode() + icon_data_uri = f"data:image/png;base64,{icon_data}" + mag_icon = Icon(src=icon_data_uri, mimeType="image/png", sizes=["64x64"]) +except (FileNotFoundError, OSError) as e: + # Fallback to emoji icon if file unavailable + mag_icon = Icon(src="emoji:🔍")src/mcp_agent/app.py (1)
57-61: Consider packaging the default icon as a resource.The
phetchicon uses a remote URL, creating an external dependency on S3 availability. Previous review comments suggested packaging the icon as a resource inmcp_agent.data(similar to how benchmarks are loaded) for better reliability and offline support.If you decide to package the icon, you could load it like this:
try: with resources.files("mcp_agent.data").joinpath("phetch.png").open("rb") as f: icon_data = base64.b64encode(f.read()).decode() icon_data_uri = f"data:image/png;base64,{icon_data}" phetch = Icon(src=icon_data_uri, mimeType="image/png", sizes=["48x48"]) except Exception: # Fallback to remote URL if packaged resource unavailable phetch = Icon( src="https://s3.us-east-1.amazonaws.com/publicdata.lastmileai.com/phetch.png", mimeType="image/png", sizes=["48x48"], )Based on previous review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
examples/mcp_agent_server/temporal/mag.pngis excluded by!**/*.png
📒 Files selected for processing (2)
examples/mcp_agent_server/temporal/main.py(2 hunks)src/mcp_agent/app.py(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
examples/mcp_agent_server/temporal/main.py (1)
src/mcp_agent/app.py (3)
tool(790-790)tool(793-803)tool(805-897)
🪛 GitHub Actions: Pull Request Checks
src/mcp_agent/app.py
[error] 2-2: F401: 'base64' imported but unused. Remove unused import or run 'ruff check --fix' to auto-fix.
[error] 22-22: F401: 'importlib.resources' imported but unused. Remove unused import or run 'ruff check --fix' to auto-fix.
examples/mcp_agent_server/temporal/main.py
[error] 15-15: F401: 'importlib.resources' imported but unused. Remove unused import or run 'ruff check --fix' to auto-fix.
🔇 Additional comments (2)
src/mcp_agent/app.py (2)
146-149: Icon initialization looks good.The icon initialization correctly uses
self._iconsconsistently and provides a sensible default with thephetchicon.
839-850: Icon parameter implementation is correct.The icon processing logic properly handles the
Iterable[Icon | Mapping]types, converts mappings to Icon instances, and defaults to[phetch]when not provided. The same logic is consistently applied inasync_tool(lines 949-960).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/mcp_agent/app.py (1)
55-59: Clean approach using a remote icon URL.Using a remote URL for the default icon is simpler than packaging and loading a file from resources. This avoids file I/O errors and packaging complexity. The URL is hosted on your own S3 bucket, which provides good control over availability.
For production deployments, consider:
- Monitoring the S3 URL availability
- Having a fallback strategy if the icon fails to load (though icons are non-critical, so this is low priority)
- Documenting this external dependency for ops teams
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/mcp_agent_server/temporal/main.py(2 hunks)src/mcp_agent/app.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/mcp_agent_server/temporal/main.py
🧰 Additional context used
🧠 Learnings (1)
📚 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/app.py
🔇 Additional comments (4)
src/mcp_agent/app.py (4)
93-93: LGTM!The
iconsparameter addition aligns with the PR objectives and follows appropriate typing conventions.
144-147: LGTM!The icon initialization logic correctly uses
self._iconsconsistently and provides a sensible default. This addresses the attribute naming concern raised in previous reviews.
787-848: LGTM!The
tooldecorator correctly handles theiconsparameter with proper type conversion for bothIconobjects and mappings. The default fallback to[phetch]ensures consistent behavior when icons aren't explicitly provided.
897-958: LGTM!The
async_tooldecorator correctly implements icon handling, mirroring the logic in thetooldecorator for consistency. Type conversion and default behavior are both correct.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/mcp_agent/server/app_server.py (1)
1525-1526: Consider defensive attribute access.While
app._iconsis always initialized inMCPApp.__init__, using a more defensive pattern would improve robustness:if "icons" not in kwargs and getattr(app, "_icons", None): kwargs["icons"] = app._iconsThis guards against edge cases where the attribute might not exist due to subclassing or other unforeseen scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/mcp_agent/app.py(6 hunks)src/mcp_agent/server/app_server.py(10 hunks)tests/server/test_tool_decorators.py(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/app.py
🧬 Code graph analysis (2)
src/mcp_agent/server/app_server.py (2)
src/mcp_agent/app.py (4)
MCPApp(64-1270)tool(907-907)tool(910-920)tool(922-1014)src/mcp_agent/core/context.py (2)
mcp(113-114)name(193-196)
tests/server/test_tool_decorators.py (3)
src/mcp_agent/server/app_server.py (4)
app(213-215)workflows(218-220)create_workflow_tools(2024-2045)create_declared_function_tools(2056-2408)src/mcp_agent/app.py (9)
MCPApp(64-1270)tool(907-907)tool(910-920)tool(922-1014)workflows(214-215)context(182-187)async_tool(1017-1017)async_tool(1020-1030)async_tool(1032-1122)tests/server/test_app_server_workflow_schema.py (2)
tool(13-20)_ToolRecorder(9-20)
🔇 Additional comments (7)
tests/server/test_tool_decorators.py (1)
6-6: Excellent test coverage for icon handling.The new tests comprehensively verify:
- Default phetch icon behavior when no icons are provided
- Custom icon override behavior
- Coverage for both
@app.tooland@app.async_tooldecoratorsThe test helper
_ToolRecorderwas properly extended to capture the icons parameter, and assertions correctly validate icon presence and equality.Also applies to: 43-58, 346-486
src/mcp_agent/app.py (4)
58-62: LGTM - Remote icon URL avoids packaging issues.Using a remote S3 URL for the default phetch icon cleanly avoids the packaging and resource loading issues raised in previous reviews. The HTTPS URL ensures secure delivery.
96-96: Good initialization logic for default icons.The icons parameter properly defaults to
[phetch]when not provided. The implementation ensuresself._iconsis always initialized and never None.Note: There's no way to specify "no icons" - both
icons=Noneand omitting the parameter result in the default phetch icon. This aligns with the PR description stating phetch is the default that developers can override.Also applies to: 148-151
929-929: Solid implementation of icon handling in tool decorator.The decorator properly:
- Accepts icons as an iterable of Icon objects or mappings
- Converts mappings to Icon instances
- Defaults to
[phetch]when not provided- Raises TypeError for invalid entries
- Stores icons in deferred tool declarations for later registration
Also applies to: 956-967, 993-993
1039-1039: Consistent icon handling in async_tool decorator.The async_tool decorator mirrors the tool decorator's icon handling logic, ensuring consistent behavior across both synchronous and asynchronous tool declarations.
Also applies to: 1066-1077, 1102-1102
src/mcp_agent/server/app_server.py (2)
1572-1572: Consistent icon application for workflow management tools.All workflow management tools (
workflows-list,workflows-run,workflows-get_status, etc.) consistently useicons=[phetch]. This matches the PR description stating that MCPApp tools use the phetch icon and cannot be changed by developers.Also applies to: 1615-1615, 1672-1672, 1699-1699, 1745-1745, 1825-1825, 2444-2444
2205-2205: Proper icon propagation through tool registration.Icons are correctly extracted from deferred tool declarations and passed to
mcp.add_tool()for both synchronous and asynchronous tools. The icons flow cleanly from decorator declarations through to final FastMCP registration.Also applies to: 2289-2289, 2402-2402
2 participants
iconsparameter toMCPAppworkflows-list,workflows-run, etc) is phetch. This cannot be changed by developer@app.[async_]tool) is phetch, but developer change throughiconsparameter on the decoratorSummary by CodeRabbit
New Features
Documentation
Tests
Examples