Support custom MCP server implementations #1087
Conversation
Signed-off-by: Anuradha Karuppiah <[email protected]>
AnuradhaKaruppiah
added
improvement
WalkthroughDelegates MCP server creation and route registration from the plugin to worker classes; adds configurable and validated Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as User/CLI
participant Plugin as MCPFrontEndPlugin
participant Worker as MCPFrontEndPluginWorker
participant FastMCP as FastMCP
participant UV as uvicorn
CLI->>Plugin: run(...)
Plugin->>Plugin: _get_worker_instance()
Plugin->>Worker: await create_mcp_server()
Worker->>Worker: configure auth (optional)
Worker-->>Plugin: FastMCP instance
Plugin->>Worker: await add_routes(mcp, builder)
Worker->>Worker: _default_add_routes() (health, funcs, debug)
alt base_path configured (streamable-http)
Plugin->>Plugin: _run_with_mount(mcp) (FastAPI wrapper)
Plugin->>UV: run uvicorn with mounted app
else base_path absent or SSE
Plugin->>FastMCP: start inline (SSE or streamable-http)
end
Plugin-->>CLI: server ready / URL logged
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
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 |
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
AnuradhaKaruppiah
changed the title
Signed-off-by: Anuradha Karuppiah <[email protected]>
AnuradhaKaruppiah
force-pushed
the
ak-nat-mcp-custom-frontend
branch
from
8f15f7b to
2a5be28
Compare
|
@CodeRabbit review full |
|
@AnuradhaKaruppiah: I'll perform a full review of all the changes in this PR. ✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyproject.toml (1)
1-1: Missing SPDX header (compliance).All files must start with the standard SPDX Apache-2.0 header. Add it at the top.
As per coding guidelines.
Apply this diff:
+## SPDX-FileCopyrightText: Copyright (c) 2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved. +## SPDX-License-Identifier: Apache-2.0 + [build-system] build-backend = "setuptools.build_meta" requires = ["setuptools >= 64", "setuptools-scm>=8"]src/nat/front_ends/mcp/mcp_front_end_plugin.py (1)
45-57: Validate custom worker type.Ensure the dynamically loaded class is a subclass of McpServerWorker to avoid runtime errors.
if self.front_end_config.runner_class: module_name, class_name = self.front_end_config.runner_class.rsplit(".", 1) import importlib module = importlib.import_module(module_name) worker_class = getattr(module, class_name) else: worker_class = self.get_worker_class() + if not issubclass(worker_class, McpServerWorker): + raise TypeError( + f"Configured runner_class {worker_class!r} must subclass McpServerWorker." + ) + return worker_class(self.full_config)
🧹 Nitpick comments (12)
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py (4)
16-21: Add a module-level docstring.Public modules must include a concise Google‑style module docstring.
As per coding guidelines.
import logging from abc import ABC from abc import abstractmethod from collections.abc import Mapping from typing import Any +"""MCP server worker base classes and default implementation."""
239-246: Preserve order when parsing repeated/comma-separatednameparams.Current set() drops order. Deduplicate while preserving client-specified order.
- names_param_list = set(request.query_params.getlist("name")) - names: list[str] = [] - for raw in names_param_list: - # if p.strip() is empty, it won't be included in the list! - parts = [p.strip() for p in raw.split(",") if p.strip()] - names.extend(parts) + raw_names = request.query_params.getlist("name") + parts: list[str] = [] + for raw in raw_names: + parts.extend([p.strip() for p in raw.split(",") if p.strip()]) + # Deduplicate while preserving order + names = list(dict.fromkeys(parts))
71-101: Health check relies on private MCP internals.Using mcp._mcp_server.request_handlers[...] is fragile across MCP versions. Prefer a public API or add a safe fallback.
Consider:
- If MCP exposes a ping method, call it.
- Otherwise, catch KeyError/AttributeError explicitly and return healthy if app starts, logging at debug.
Would you like me to propose a compatibility shim that tries multiple strategies?
230-318: 404 detail echoes user input; constrain exposure.Minor hardening: limit length and sanitize the tool name in error messages.
- except KeyError as e: - raise HTTPException(status_code=404, detail=f"Tool \"{e.args[0]}\" not found.") from e + except KeyError as e: + missing = str(e.args[0])[:100] + raise HTTPException(status_code=404, detail=f'Tool "{missing}" not found.') from etests/nat/front_ends/mcp/test_mcp_custom_server.py (4)
140-150: Align fixture naming with project conventions.Use fixture_ prefix and set name= to keep the public fixture name.
As per coding guidelines.
-@pytest.fixture -def base_config(): +@pytest.fixture(name="base_config") +def fixture_base_config(): """Create base config for testing.""" echo_function_config = EchoFunctionConfig() mcp_front_end_config = MCPFrontEndConfig(name="Test Server", host="localhost", port=9999) return Config(general=GeneralConfig(front_end=mcp_front_end_config), workflow=echo_function_config, functions={"echo": echo_function_config})
327-332: Fix Ruff ARG001 by ignoring or prefixing unused arg.Rename the unused parameter to _request.
- async def mock_call_next(request): + async def mock_call_next(_request): return mock_response
292-297: Avoid asserting private attributes.builder._workflow is private and may change. Prefer asserting via public behavior (e.g., call list-tools endpoint or check MCP registration if a public API exists).
If there's no public accessor, consider exposing a minimal getter in WorkflowBuilder for tests. Do you want me to draft that?
282-293: Middleware assertion: make it robust.Instead of len(user_middleware) > 0, assert that our logging middleware is present.
assert any(getattr(m.cls, "__name__", "") == "log_requests" for m in mcp.app.user_middleware)docs/source/extend/custom-mcp-worker.md (4)
156-163: Add language to fenced code block.Label the “Expected output” block as text.
-``` +```text INFO: Started server process [12345] INFO: Waiting for application startup. INFO: Application startup complete. INFO: Uvicorn running on http://localhost:9000 (Press CTRL+C to quit)--- `176-181`: **Add language to fenced code block.** Label the “Observe the logs” block as text. ```diff -``` +```text INFO:my_package.logging_worker:Request: POST /tools/call INFO:my_package.logging_worker:Response: 200 (0.45s)--- `205-216`: **Hyphenate “front-end” in prose.** Minor style fix. ```diff -- **`self.front_end_config`**: MCP server configuration +- **`self.front_end_config`**: MCP server configuration - `name`: Server name - - `host`: Server host address - - `port`: Server port number - - `debug`: Debug mode flag + - `host`: Server host address + - `port`: Server port number + - `debug`: Debug-mode flag
20-22: Update cross-reference wording.Avoid “NAT” and clarify the reference.
-We recommend reading the [MCP Server Guide](../workflows/mcp/mcp-server.md) before proceeding with this documentation, to understand how MCP servers work in NAT. +We recommend reading the [MCP Server Guide](../workflows/mcp/mcp-server.md) before proceeding, to understand how MCP servers work in the NeMo Agent toolkit.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
docs/source/extend/custom-mcp-worker.md(1 hunks)pyproject.toml(1 hunks)src/nat/front_ends/mcp/mcp_front_end_plugin.py(3 hunks)src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py(3 hunks)tests/nat/front_ends/mcp/test_mcp_custom_server.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
{**/*.py,**/*.sh,**/*.md,**/*.toml,**/*.y?(a)ml,**/*.json,**/*.txt,**/*.ini,**/*.cfg,**/*.ipynb}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{**/*.py,**/*.sh,**/*.md,**/*.toml,**/*.y?(a)ml,**/*.json,**/*.txt,**/*.ini,**/*.cfg,**/*.ipynb}: Every file must start with the standard SPDX Apache-2.0 header
Confirm copyright years are up to date when a file is changed
All source files must include the SPDX Apache-2.0 header template (copy from an existing file)
Files:
pyproject.tomlsrc/nat/front_ends/mcp/mcp_front_end_plugin_worker.pydocs/source/extend/custom-mcp-worker.mdtests/nat/front_ends/mcp/test_mcp_custom_server.pysrc/nat/front_ends/mcp/mcp_front_end_plugin.py
{**/pyproject.toml,uv.lock}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
New dependencies must be added to both pyproject.toml (alphabetically) and uv.lock via ‘uv pip install --sync’
Files:
pyproject.toml
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
pyproject.tomlsrc/nat/front_ends/mcp/mcp_front_end_plugin_worker.pydocs/source/extend/custom-mcp-worker.mdtests/nat/front_ends/mcp/test_mcp_custom_server.pysrc/nat/front_ends/mcp/mcp_front_end_plugin.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: In code comments use the abbreviations: nat (API namespace/CLI), nvidia-nat (package), NAT (env var prefixes); never use these abbreviations in documentation
Follow PEP 20 and PEP 8 for Python style
Run yapf with column_limit=120; yapf is used for formatting (run second)
Indent with 4 spaces (no tabs) and end each file with a single trailing newline
Use ruff (ruff check --fix) as a linter (not formatter) per pyproject.toml; fix warnings unless explicitly ignored
Respect Python naming schemes: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
When re-raising exceptions, use bare raise to preserve stack trace; log with logger.error(), not logger.exception()
When catching and logging without re-raising, use logger.exception() to capture full stack trace
Provide Google-style docstrings for every public module, class, function, and CLI command
Docstring first line must be a concise description ending with a period
Validate and sanitize all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work (HTTP, DB, file I/O)
Cache expensive computations with functools.lru_cache or an external cache when appropriate
Leverage NumPy vectorized operations when beneficial and feasible
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.pytests/nat/front_ends/mcp/test_mcp_custom_server.pysrc/nat/front_ends/mcp/mcp_front_end_plugin.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{src/**/*.py,packages/*/src/**/*.py}: All importable Python code must live under src/ or packages//src/
All public APIs must have Python 3.11+ type hints on parameters and return values
Prefer typing/collections.abc abstractions (e.g., Sequence over list)
Use typing.Annotated for units or metadata when useful
Treat pyright warnings as errors during development
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.pysrc/nat/front_ends/mcp/mcp_front_end_plugin.py
src/nat/**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
src/nat/**/* contains core functionality; changes should prioritize backward compatibility
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.pysrc/nat/front_ends/mcp/mcp_front_end_plugin.py
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.pysrc/nat/front_ends/mcp/mcp_front_end_plugin.py
{docs/source/**/*.md,**/README.@(md|ipynb)}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{docs/source/**/*.md,**/README.@(md|ipynb)}: Use the full name “NVIDIA NeMo Agent toolkit” on first use in documentation, then “NeMo Agent toolkit”; in headings use “NeMo Agent Toolkit” (capital T)
Do not use deprecated names (Agent Intelligence toolkit, aiqtoolkit, AgentIQ, AIQ/aiq) in documentation unless explicitly referring to deprecated names
Never use “NAT”/“nat” abbreviations in documentation
Documentation must be clear/comprehensive; avoid TODOs/FIXMEs/placeholders; avoid offensive/outdated terms; ensure spelling is correct
Files:
docs/source/extend/custom-mcp-worker.md
docs/source/**/*.md
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
docs/source/**/*.md: Documentation sources are Markdown files under docs/source
Surround code entities with backticks in docs to avoid Vale false positives
Do not use words listed in ci/vale/styles/config/vocabularies/nat/reject.txt; accepted words in accept.txt are allowed
Files:
docs/source/extend/custom-mcp-worker.md
docs/source/**/*
⚙️ CodeRabbit configuration file
This directory contains the source code for the documentation. All documentation should be written in Markdown format. Any image files should be placed in the
docs/source/_staticdirectory.
Files:
docs/source/extend/custom-mcp-worker.md
{tests/**/*.py,examples/*/tests/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Unit tests live in tests/ or examples/*/tests and use markers defined in pyproject.toml (e.g., integration)
Files:
tests/nat/front_ends/mcp/test_mcp_custom_server.py
{tests/**/*.py,examples/*/tests/**/*.py,packages/*/tests/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{tests/**/*.py,examples/*/tests/**/*.py,packages/*/tests/**/*.py}: Use pytest with pytest-asyncio for async code
Test functions should be named with test_ prefix using snake_case
Extract frequently repeated test code into pytest fixtures
Pytest fixtures should define the name argument in @pytest.fixture and the function should be prefixed fixture_ using snake_case
Mock external services with pytest_httpserver or unittest.mock; avoid live endpoints
Mark slow tests with @pytest.mark.slow to skip in default test suite
Mark integration tests requiring external services with @pytest.mark.integration and follow nat-tests guidelines
Files:
tests/nat/front_ends/mcp/test_mcp_custom_server.py
tests/**/*.py
⚙️ CodeRabbit configuration file
tests/**/*.py: - Ensure that tests are comprehensive, cover edge cases, and validate the functionality of the code. - Test functions should be named using thetest_prefix, using snake_case. - Any frequently repeated code should be extracted into pytest fixtures. - Pytest fixtures should define the name argument when applying the pytest.fixture decorator. The fixture
function being decorated should be named using thefixture_prefix, using snake_case. Example:
@pytest.fixture(name="my_fixture")
def fixture_my_fixture():
pass
Files:
tests/nat/front_ends/mcp/test_mcp_custom_server.py
🪛 LanguageTool
docs/source/extend/custom-mcp-worker.md
[style] ~45-~45: Consider using “who” when you are referring to a person instead of an object.
Context: ...ollowing example shows a minimal worker that adds request logging middleware. Each ...
(THAT_WHO)
[grammar] ~216-~216: Use a hyphen to join words.
Context: ...neral: General settings including front end config - llms`: LLM configuration...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
docs/source/extend/custom-mcp-worker.md
157-157: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
177-177: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.1)
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
193-193: Avoid specifying long messages outside the exception class
(TRY003)
tests/nat/front_ends/mcp/test_mcp_custom_server.py
327-327: Unused function argument: request
(ARG001)
🔇 Additional comments (2)
docs/source/extend/custom-mcp-worker.md (1)
1-16: Header looks good.SPDX block present and correct.
pyproject.toml (1)
27-27: Based on my verification of the lock file and web research, the FastAPI 0.120.0 update in the codebase is properly reflected:
- FastAPI 0.120.0 contains a transition to the annotated_doc.Doc package, with no new dependency constraints introduced
- Earlier versions bumped Starlette to allow up to 0.46.0 with constraint >=0.40.0,<0.47.0, which means Starlette 0.46.2 is compatible
- The uv.lock file confirms FastAPI 0.120.0 is pinned, Uvicorn 0.35.0 satisfies the <0.36 constraint, and Starlette 0.46.2 falls within the expected version range
The lock file is consistent with pyproject.toml specifications and no compatibility conflicts are present.
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
|
@CodeRabbit review full |
|
@AnuradhaKaruppiah: I'll perform a full review of all the changes in this PR again. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
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)
ci/vale/styles/config/vocabularies/nat/accept.txt (1)
1-4: Add SPDX header to comply with repo policy.This .txt file lacks the required SPDX Apache-2.0 header. Please prepend it as comments.
As per coding guidelines.
+# SPDX-FileCopyrightText: Copyright (c) 2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + # List of case-sensitive regular expressions matching words that should be accepted by Vale. For product names like # "cuDF" or "cuML", we want to ensure that they are capitalized the same way they're written by the product owners. # Regular expressions are parsed according to the Go syntax: https://golang.org/pkg/regexp/syntax/
♻️ Duplicate comments (1)
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py (1)
184-186: Gate debug endpoints behind a config flag.Only register debug endpoints when debug is enabled to avoid exposing internal metadata.
- # After registration, expose debug endpoints for tool/schema inspection - self._setup_debug_endpoints(mcp, functions) + # Expose debug endpoints only in debug mode + if self.front_end_config.debug: + self._setup_debug_endpoints(mcp, functions)As per coding guidelines.
🧹 Nitpick comments (10)
docs/source/extend/mcp-server.md (1)
136-141: Specify a language for the fenced block.markdownlint MD040: add a language (e.g., text) to the “Expected output” block.
-``` +```text INFO: Started server process [12345] INFO: Waiting for application startup. INFO: Application startup complete. INFO: Uvicorn running on http://localhost:9000 (Press CTRL+C to quit)As per coding guidelines. </blockquote></details> <details> <summary>src/nat/front_ends/mcp/mcp_front_end_plugin.py (1)</summary><blockquote> `45-57`: **Add return type and validate configured worker class.** Annotate return type and guard against misconfiguration with issubclass + clearer import errors. ```diff - def _get_worker_instance(self): + def _get_worker_instance(self) -> MCPFrontEndPluginWorkerBase: """Get an instance of the worker class.""" # Import the worker class dynamically if specified in config - if self.front_end_config.runner_class: - module_name, class_name = self.front_end_config.runner_class.rsplit(".", 1) - import importlib - module = importlib.import_module(module_name) - worker_class = getattr(module, class_name) + if self.front_end_config.runner_class: + module_name, class_name = self.front_end_config.runner_class.rsplit(".", 1) + import importlib + try: + module = importlib.import_module(module_name) + worker_class = getattr(module, class_name) + except (ModuleNotFoundError, AttributeError) as e: + raise ImportError(f"Cannot import worker '{self.front_end_config.runner_class}': {e}") from e else: worker_class = self.get_worker_class() + if not issubclass(worker_class, MCPFrontEndPluginWorkerBase): + raise TypeError(f"Configured worker '{worker_class.__module__}.{worker_class.__qualname__}' " + f"must subclass MCPFrontEndPluginWorkerBase") return worker_class(self.full_config)As per coding guidelines.
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py (8)
45-59: Annotate init return type.Prefer explicit -> None on init for consistency with project typing rules.
- def __init__(self, config: Config): + def __init__(self, config: Config) -> None:As per coding guidelines.
60-66: Add explicit return type.Annotate helper with -> None.
- def _setup_health_endpoint(self, mcp: FastMCP): + def _setup_health_endpoint(self, mcp: FastMCP) -> None:As per coding guidelines.
71-76: Avoid relying on private attributes in health check.Accessing mcp._mcp_server.request_handlers is fragile. Guard with getattr and fall back to a public ping if available.
- # Call the ping handler directly (same one that responds to MCP pings) - await mcp._mcp_server.request_handlers[PingRequest](ping_request) + # Prefer public API; fall back carefully if needed + handler = getattr(getattr(mcp, "_mcp_server", None), "request_handlers", {}).get(PingRequest) # type: ignore[attr-defined] + if callable(handler): + await handler(ping_request) + elif hasattr(mcp, "ping"): + # type: ignore[attr-defined] + await mcp.ping() # public ping if exposed by FastMCP + else: + raise RuntimeError("Ping handler not available via public or known internal API")If FastMCP exposes a canonical ping API, consider switching to it directly.
110-129: Add return type annotation for add_routes.The abstract and overrides should declare -> None.
- async def add_routes(self, mcp: FastMCP, builder: WorkflowBuilder): + async def add_routes(self, mcp: FastMCP, builder: WorkflowBuilder) -> None:Apply the same to concrete overrides below.
As per coding guidelines.
132-149: Add return type annotation.mark helper as returning None for consistency.
- async def _default_add_routes(self, mcp: FastMCP, builder: WorkflowBuilder): + async def _default_add_routes(self, mcp: FastMCP, builder: WorkflowBuilder) -> None:As per coding guidelines.
180-183: Comment/code mismatch.Comment says “Add a simple fallback function if no functions were found” but code raises RuntimeError. Align the comment or implement a real fallback.
- # Add a simple fallback function if no functions were found + # If no functions were found, raise an error to surface misconfiguration
348-367: Pydantic URL construction: avoid calling AnyHttpUrl(...) directly.Let Pydantic validate from strings (or use TypeAdapter) to reduce runtime coupling to pydantic internals.
- from pydantic import AnyHttpUrl - server_url = f"http://{self.front_end_config.host}:{self.front_end_config.port}" - auth_settings = AuthSettings(issuer_url=AnyHttpUrl(self.front_end_config.server_auth.issuer_url), - required_scopes=self.front_end_config.server_auth.scopes, - resource_server_url=AnyHttpUrl(server_url)) + auth_settings = AuthSettings( + issuer_url=self.front_end_config.server_auth.issuer_url, + required_scopes=self.front_end_config.server_auth.scopes, + resource_server_url=server_url, + )If AuthSettings requires AnyHttpUrl types, consider:
from pydantic import AnyHttpUrl, TypeAdapter issuer = TypeAdapter(AnyHttpUrl).validate_python(self.front_end_config.server_auth.issuer_url) resource = TypeAdapter(AnyHttpUrl).validate_python(server_url)As per coding guidelines.
369-377: Return type annotation for override.Add -> None to the concrete add_routes for consistency.
- async def add_routes(self, mcp: FastMCP, builder: WorkflowBuilder): + async def add_routes(self, mcp: FastMCP, builder: WorkflowBuilder) -> None:As per coding guidelines.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ci/vale/styles/config/vocabularies/nat/accept.txt(1 hunks)docs/source/extend/mcp-server.md(1 hunks)docs/source/index.md(1 hunks)pyproject.toml(1 hunks)src/nat/front_ends/mcp/mcp_front_end_plugin.py(2 hunks)src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/source/index.md
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
📓 Path-based instructions (8)
{docs/source/**/*.md,**/README.@(md|ipynb)}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{docs/source/**/*.md,**/README.@(md|ipynb)}: Use the full name “NVIDIA NeMo Agent toolkit” on first use in documentation, then “NeMo Agent toolkit”; in headings use “NeMo Agent Toolkit” (capital T)
Do not use deprecated names (Agent Intelligence toolkit, aiqtoolkit, AgentIQ, AIQ/aiq) in documentation unless explicitly referring to deprecated names
Never use “NAT”/“nat” abbreviations in documentation
Documentation must be clear/comprehensive; avoid TODOs/FIXMEs/placeholders; avoid offensive/outdated terms; ensure spelling is correct
Files:
docs/source/extend/mcp-server.md
docs/source/**/*.md
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
docs/source/**/*.md: Documentation sources are Markdown files under docs/source
Surround code entities with backticks in docs to avoid Vale false positives
Do not use words listed in ci/vale/styles/config/vocabularies/nat/reject.txt; accepted words in accept.txt are allowed
Files:
docs/source/extend/mcp-server.md
{**/*.py,**/*.sh,**/*.md,**/*.toml,**/*.y?(a)ml,**/*.json,**/*.txt,**/*.ini,**/*.cfg,**/*.ipynb}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{**/*.py,**/*.sh,**/*.md,**/*.toml,**/*.y?(a)ml,**/*.json,**/*.txt,**/*.ini,**/*.cfg,**/*.ipynb}: Every file must start with the standard SPDX Apache-2.0 header
Confirm copyright years are up to date when a file is changed
All source files must include the SPDX Apache-2.0 header template (copy from an existing file)
Files:
docs/source/extend/mcp-server.mdsrc/nat/front_ends/mcp/mcp_front_end_plugin.pyci/vale/styles/config/vocabularies/nat/accept.txtsrc/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
docs/source/extend/mcp-server.mdsrc/nat/front_ends/mcp/mcp_front_end_plugin.pyci/vale/styles/config/vocabularies/nat/accept.txtsrc/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
docs/source/**/*
⚙️ CodeRabbit configuration file
This directory contains the source code for the documentation. All documentation should be written in Markdown format. Any image files should be placed in the
docs/source/_staticdirectory.
Files:
docs/source/extend/mcp-server.md
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: In code comments use the abbreviations: nat (API namespace/CLI), nvidia-nat (package), NAT (env var prefixes); never use these abbreviations in documentation
Follow PEP 20 and PEP 8 for Python style
Run yapf with column_limit=120; yapf is used for formatting (run second)
Indent with 4 spaces (no tabs) and end each file with a single trailing newline
Use ruff (ruff check --fix) as a linter (not formatter) per pyproject.toml; fix warnings unless explicitly ignored
Respect Python naming schemes: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
When re-raising exceptions, use bare raise to preserve stack trace; log with logger.error(), not logger.exception()
When catching and logging without re-raising, use logger.exception() to capture full stack trace
Provide Google-style docstrings for every public module, class, function, and CLI command
Docstring first line must be a concise description ending with a period
Validate and sanitize all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work (HTTP, DB, file I/O)
Cache expensive computations with functools.lru_cache or an external cache when appropriate
Leverage NumPy vectorized operations when beneficial and feasible
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin.pysrc/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{src/**/*.py,packages/*/src/**/*.py}: All importable Python code must live under src/ or packages//src/
All public APIs must have Python 3.11+ type hints on parameters and return values
Prefer typing/collections.abc abstractions (e.g., Sequence over list)
Use typing.Annotated for units or metadata when useful
Treat pyright warnings as errors during development
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin.pysrc/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
src/nat/**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
src/nat/**/* contains core functionality; changes should prioritize backward compatibility
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin.pysrc/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin.pysrc/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
🧠 Learnings (4)
📚 Learning: 2025-10-22T22:02:12.903Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-10-22T22:02:12.903Z
Learning: Applies to {docs/source/**/*.md,**/README.@(md|ipynb)} : Use the full name “NVIDIA NeMo Agent toolkit” on first use in documentation, then “NeMo Agent toolkit”; in headings use “NeMo Agent Toolkit” (capital T)
Applied to files:
docs/source/extend/mcp-server.md
📚 Learning: 2025-10-22T22:02:12.903Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-10-22T22:02:12.903Z
Learning: Applies to {docs/source/**/*.md,**/README.@(md|ipynb)} : Never use “NAT”/“nat” abbreviations in documentation
Applied to files:
docs/source/extend/mcp-server.md
📚 Learning: 2025-08-22T20:18:28.041Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/cursor-rules.mdc:0-0
Timestamp: 2025-08-22T20:18:28.041Z
Learning: Applies to .cursor/rules/**/*.mdc : Use consistent project terminology in descriptions (e.g., NAT workflows, NAT CLI commands)
Applied to files:
docs/source/extend/mcp-server.md
📚 Learning: 2025-10-22T22:02:12.903Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-10-22T22:02:12.903Z
Learning: Applies to **/*.py : In code comments use the abbreviations: nat (API namespace/CLI), nvidia-nat (package), NAT (env var prefixes); never use these abbreviations in documentation
Applied to files:
docs/source/extend/mcp-server.md
🧬 Code graph analysis (2)
src/nat/front_ends/mcp/mcp_front_end_plugin.py (1)
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py (2)
create_mcp_server(92-108)create_mcp_server(338-367)
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py (5)
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (2)
add_routes(241-253)front_end_config(126-127)tests/nat/front_ends/mcp/test_mcp_custom_routes.py (1)
add_routes(35-51)src/nat/builder/workflow_builder.py (2)
WorkflowBuilder(144-1157)build(263-378)src/nat/front_ends/mcp/tool_converter.py (1)
register_function_with_mcp(254-290)src/nat/front_ends/mcp/introspection_token_verifier.py (1)
IntrospectionTokenVerifier(28-73)
🪛 LanguageTool
docs/source/extend/mcp-server.md
[style] ~43-~43: Consider using “who” when you are referring to a person instead of an object.
Context: ...ollowing example shows a minimal worker that adds a custom status endpoint to the MC...
(THAT_WHO)
[grammar] ~221-~221: Use a hyphen to join words.
Context: ...neral: General settings including front end config - llms`: LLM configuration...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
docs/source/extend/mcp-server.md
136-136: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.1)
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
182-182: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (4)
ci/vale/styles/config/vocabularies/nat/accept.txt (1)
88-88: LGTM: vocabulary addition.Adding “[Mm]iddleware” is appropriate and alphabetically placed between Mem0 and Milvus.
docs/source/extend/mcp-server.md (2)
1-16: License header: good.SPDX header present and correct.
20-25: Product naming is correct.Uses “NVIDIA NeMo Agent toolkit” on first use and “NeMo Agent toolkit” thereafter; CLI shown in code blocks.
Based on learnings.
src/nat/front_ends/mcp/mcp_front_end_plugin.py (1)
63-71: Delegation to worker: good.run() cleanly delegates server creation and route registration to the worker.
There was a problem hiding this comment.
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/nat/front_ends/mcp/mcp_front_end_plugin.py (1)
45-56: Add return type annotation.The method
_get_worker_instanceis missing a return type annotation. Per coding guidelines, all Python methods should use type hints for parameters and return values.Apply this diff:
- def _get_worker_instance(self): + def _get_worker_instance(self) -> MCPFrontEndPluginWorkerBase: """Get an instance of the worker class."""
🧹 Nitpick comments (1)
docs/source/extend/mcp-server.md (1)
210-243: Add hyphen to compound adjective.On line 221, "front end config" should be "front-end config" when used as a compound adjective before a noun.
Apply this diff:
-- **`self.front_end_config`**: MCP server configuration +- **`self.front_end_config`**: MCP server configurationNote: The actual field name
front_end_config(with underscore) remains unchanged - this is only about the prose description.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
ci/vale/styles/config/vocabularies/nat/accept.txt(1 hunks)docs/source/extend/mcp-server.md(1 hunks)docs/source/index.md(1 hunks)pyproject.toml(1 hunks)src/nat/front_ends/mcp/mcp_front_end_plugin.py(2 hunks)src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
{**/*.py,**/*.sh,**/*.md,**/*.toml,**/*.y?(a)ml,**/*.json,**/*.txt,**/*.ini,**/*.cfg,**/*.ipynb}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{**/*.py,**/*.sh,**/*.md,**/*.toml,**/*.y?(a)ml,**/*.json,**/*.txt,**/*.ini,**/*.cfg,**/*.ipynb}: Every file must start with the standard SPDX Apache-2.0 header
Confirm copyright years are up to date when a file is changed
All source files must include the SPDX Apache-2.0 header template (copy from an existing file)
Files:
ci/vale/styles/config/vocabularies/nat/accept.txtdocs/source/index.mddocs/source/extend/mcp-server.mdsrc/nat/front_ends/mcp/mcp_front_end_plugin.pysrc/nat/front_ends/mcp/mcp_front_end_plugin_worker.pypyproject.toml
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
ci/vale/styles/config/vocabularies/nat/accept.txtdocs/source/index.mddocs/source/extend/mcp-server.mdsrc/nat/front_ends/mcp/mcp_front_end_plugin.pysrc/nat/front_ends/mcp/mcp_front_end_plugin_worker.pypyproject.toml
{docs/source/**/*.md,**/README.@(md|ipynb)}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{docs/source/**/*.md,**/README.@(md|ipynb)}: Use the full name “NVIDIA NeMo Agent toolkit” on first use in documentation, then “NeMo Agent toolkit”; in headings use “NeMo Agent Toolkit” (capital T)
Do not use deprecated names (Agent Intelligence toolkit, aiqtoolkit, AgentIQ, AIQ/aiq) in documentation unless explicitly referring to deprecated names
Never use “NAT”/“nat” abbreviations in documentation
Documentation must be clear/comprehensive; avoid TODOs/FIXMEs/placeholders; avoid offensive/outdated terms; ensure spelling is correct
Files:
docs/source/index.mddocs/source/extend/mcp-server.md
docs/source/**/*.md
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
docs/source/**/*.md: Documentation sources are Markdown files under docs/source
Surround code entities with backticks in docs to avoid Vale false positives
Do not use words listed in ci/vale/styles/config/vocabularies/nat/reject.txt; accepted words in accept.txt are allowed
Files:
docs/source/index.mddocs/source/extend/mcp-server.md
docs/source/**/*
⚙️ CodeRabbit configuration file
This directory contains the source code for the documentation. All documentation should be written in Markdown format. Any image files should be placed in the
docs/source/_staticdirectory.
Files:
docs/source/index.mddocs/source/extend/mcp-server.md
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: In code comments use the abbreviations: nat (API namespace/CLI), nvidia-nat (package), NAT (env var prefixes); never use these abbreviations in documentation
Follow PEP 20 and PEP 8 for Python style
Run yapf with column_limit=120; yapf is used for formatting (run second)
Indent with 4 spaces (no tabs) and end each file with a single trailing newline
Use ruff (ruff check --fix) as a linter (not formatter) per pyproject.toml; fix warnings unless explicitly ignored
Respect Python naming schemes: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
When re-raising exceptions, use bare raise to preserve stack trace; log with logger.error(), not logger.exception()
When catching and logging without re-raising, use logger.exception() to capture full stack trace
Provide Google-style docstrings for every public module, class, function, and CLI command
Docstring first line must be a concise description ending with a period
Validate and sanitize all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work (HTTP, DB, file I/O)
Cache expensive computations with functools.lru_cache or an external cache when appropriate
Leverage NumPy vectorized operations when beneficial and feasible
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin.pysrc/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{src/**/*.py,packages/*/src/**/*.py}: All importable Python code must live under src/ or packages//src/
All public APIs must have Python 3.11+ type hints on parameters and return values
Prefer typing/collections.abc abstractions (e.g., Sequence over list)
Use typing.Annotated for units or metadata when useful
Treat pyright warnings as errors during development
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin.pysrc/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
src/nat/**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
src/nat/**/* contains core functionality; changes should prioritize backward compatibility
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin.pysrc/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin.pysrc/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
{**/pyproject.toml,uv.lock}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
New dependencies must be added to both pyproject.toml (alphabetically) and uv.lock via ‘uv pip install --sync’
Files:
pyproject.toml
🧠 Learnings (4)
📚 Learning: 2025-10-22T22:02:12.903Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-10-22T22:02:12.903Z
Learning: Applies to {docs/source/**/*.md,**/README.@(md|ipynb)} : Use the full name “NVIDIA NeMo Agent toolkit” on first use in documentation, then “NeMo Agent toolkit”; in headings use “NeMo Agent Toolkit” (capital T)
Applied to files:
docs/source/extend/mcp-server.md
📚 Learning: 2025-10-22T22:02:12.903Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-10-22T22:02:12.903Z
Learning: Applies to {docs/source/**/*.md,**/README.@(md|ipynb)} : Never use “NAT”/“nat” abbreviations in documentation
Applied to files:
docs/source/extend/mcp-server.md
📚 Learning: 2025-08-22T20:18:28.041Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/cursor-rules.mdc:0-0
Timestamp: 2025-08-22T20:18:28.041Z
Learning: Applies to .cursor/rules/**/*.mdc : Use consistent project terminology in descriptions (e.g., NAT workflows, NAT CLI commands)
Applied to files:
docs/source/extend/mcp-server.md
📚 Learning: 2025-10-22T22:02:12.903Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-10-22T22:02:12.903Z
Learning: Applies to **/*.py : In code comments use the abbreviations: nat (API namespace/CLI), nvidia-nat (package), NAT (env var prefixes); never use these abbreviations in documentation
Applied to files:
docs/source/extend/mcp-server.md
🪛 LanguageTool
docs/source/extend/mcp-server.md
[style] ~43-~43: Consider using “who” when you are referring to a person instead of an object.
Context: ...ollowing example shows a minimal worker that adds a custom status endpoint to the MC...
(THAT_WHO)
[grammar] ~221-~221: Use a hyphen to join words.
Context: ...neral: General settings including front end config - llms`: LLM configuration...
(QB_NEW_EN_HYPHEN)
🪛 Ruff (0.14.1)
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
182-182: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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: CI Pipeline / Check
🔇 Additional comments (24)
ci/vale/styles/config/vocabularies/nat/accept.txt (1)
88-88: LGTM!The addition of [Mm]iddleware to the accepted vocabulary aligns with the new MCP server customization features documented in this PR.
docs/source/index.md (1)
124-124: LGTM!The new documentation entry for custom MCP server workers is properly placed in the Extend section and follows the correct toctree format.
src/nat/front_ends/mcp/mcp_front_end_plugin.py (2)
36-43: LGTM!The
@typing.finaldecorator correctly indicates thatget_worker_class_nameshould not be overridden by subclasses. The method implementation is clean and straightforward.
58-82: LGTM!The refactored flow cleanly delegates MCP server creation and route registration to the worker, providing a clear extension point for custom implementations. The separation of concerns is well-designed.
docs/source/extend/mcp-server.md (10)
1-16: LGTM!The Apache-2.0 SPDX header is correctly formatted and includes the proper copyright year 2025.
18-34: LGTM!The introduction clearly explains when and why developers should create custom MCP server workers. The use cases are well-documented and relevant to enterprise requirements.
35-48: LGTM!The section provides clear context and appropriate framing for the tutorial. The note to read the MCP Server Guide first is helpful for readers.
49-88: LGTM!The code example is clear, well-commented, and demonstrates proper inheritance and method overriding patterns. The use of
super().add_routes()to extend default behavior is the correct approach.
90-95: LGTM!The key components breakdown clearly explains the important patterns used in the example.
96-125: LGTM!The YAML configuration example is clear and correctly shows how to register a custom worker using the
runner_classfield.
127-165: LGTM!The testing section provides clear commands and expected outputs, making it easy for developers to verify their custom worker implementation.
167-189: LGTM!The explanation of
super().add_routes()is clear and accurately describes what default functionality it provides.
191-209: LGTM!The section clearly explains when and how to override
create_mcp_server(), and importantly notes that custom workers take ownership of authentication configuration when overriding this method.
245-253: LGTM!The summary effectively recaps the key points and emphasizes the benefits of custom workers for enterprise use cases.
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py (9)
37-59: LGTM!The base class docstring clearly explains the purpose and recommends using MCPFrontEndPluginWorker instead of direct inheritance. The constructor is well-documented and properly initializes the memory profiler.
60-90: LGTM!The health endpoint implementation correctly uses the MCP server's internal ping handler to verify server health. The error handling is appropriate, returning a 503 status code for unhealthy states.
91-108: LGTM!The abstract method
create_mcp_server()provides a clear extension point for custom server implementations. The docstring includes a helpful example showing the expected return pattern.
110-130: LGTM!The abstract method
add_routes()is well-documented with clear guidance on calling_default_add_routes()for standard behavior before adding custom features. The example demonstrates the recommended pattern.
132-185: LGTM!The
_default_add_routes()method provides a comprehensive default implementation that handles health endpoints, workflow building, function registration, and debug endpoints. The filtering logic for tool_names is well-implemented, supporting both individual function names and function group prefixes.
187-208: LGTM!The
_get_all_functions()method correctly extracts functions from the workflow, function groups, and includes the workflow itself using the configured alias. The implementation is straightforward and comprehensive.
210-316: LGTM!The debug endpoints provide valuable introspection capabilities:
/debug/tools/listsupports filtering and detail control with sensible defaults/debug/memory/statsexposes memory profiling statistics- The implementation handles query parameters flexibly (comma-separated and repeated)
- Error handling for missing tools is appropriate
318-367: LGTM!The concrete
MCPFrontEndPluginWorkerimplementation provides excellent default behavior:
create_mcp_server()properly handles optional authentication configuration- Authentication settings are correctly constructed from front-end config
- The IntrospectionTokenVerifier is instantiated when auth is enabled
- All configuration parameters are passed through to FastMCP
369-377: LGTM!The
add_routes()implementation correctly delegates to the base class_default_add_routes()method, providing a clean extension pattern for subclasses.pyproject.toml (1)
27-27: FastAPI 0.120.1 verified as available and secure.The version exists on PyPI and is not affected by known security vulnerabilities (CSRF vulnerability fixed in 0.65.2 and ReDoS vulnerability fixed in 0.109.1). The semver constraint ~=0.120.1 appropriately allows patch-level updates.
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/nat/front_ends/mcp/mcp_front_end_config.py(1 hunks)src/nat/front_ends/mcp/mcp_front_end_plugin.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: In code comments use the abbreviations: nat (API namespace/CLI), nvidia-nat (package), NAT (env var prefixes); never use these abbreviations in documentation
Follow PEP 20 and PEP 8 for Python style
Run yapf with column_limit=120; yapf is used for formatting (run second)
Indent with 4 spaces (no tabs) and end each file with a single trailing newline
Use ruff (ruff check --fix) as a linter (not formatter) per pyproject.toml; fix warnings unless explicitly ignored
Respect Python naming schemes: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
When re-raising exceptions, use bare raise to preserve stack trace; log with logger.error(), not logger.exception()
When catching and logging without re-raising, use logger.exception() to capture full stack trace
Provide Google-style docstrings for every public module, class, function, and CLI command
Docstring first line must be a concise description ending with a period
Validate and sanitize all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work (HTTP, DB, file I/O)
Cache expensive computations with functools.lru_cache or an external cache when appropriate
Leverage NumPy vectorized operations when beneficial and feasible
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin.pysrc/nat/front_ends/mcp/mcp_front_end_config.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{src/**/*.py,packages/*/src/**/*.py}: All importable Python code must live under src/ or packages//src/
All public APIs must have Python 3.11+ type hints on parameters and return values
Prefer typing/collections.abc abstractions (e.g., Sequence over list)
Use typing.Annotated for units or metadata when useful
Treat pyright warnings as errors during development
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin.pysrc/nat/front_ends/mcp/mcp_front_end_config.py
src/nat/**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
src/nat/**/* contains core functionality; changes should prioritize backward compatibility
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin.pysrc/nat/front_ends/mcp/mcp_front_end_config.py
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin.pysrc/nat/front_ends/mcp/mcp_front_end_config.py
{**/*.py,**/*.sh,**/*.md,**/*.toml,**/*.y?(a)ml,**/*.json,**/*.txt,**/*.ini,**/*.cfg,**/*.ipynb}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{**/*.py,**/*.sh,**/*.md,**/*.toml,**/*.y?(a)ml,**/*.json,**/*.txt,**/*.ini,**/*.cfg,**/*.ipynb}: Every file must start with the standard SPDX Apache-2.0 header
Confirm copyright years are up to date when a file is changed
All source files must include the SPDX Apache-2.0 header template (copy from an existing file)
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin.pysrc/nat/front_ends/mcp/mcp_front_end_config.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin.pysrc/nat/front_ends/mcp/mcp_front_end_config.py
🧬 Code graph analysis (1)
src/nat/front_ends/mcp/mcp_front_end_plugin.py (2)
tests/nat/front_ends/mcp/test_mcp_debug_routes.py (1)
worker(104-106)src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py (4)
create_mcp_server(92-108)create_mcp_server(338-367)add_routes(111-130)add_routes(369-377)
🪛 Ruff (0.14.2)
src/nat/front_ends/mcp/mcp_front_end_plugin.py
126-126: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (4)
src/nat/front_ends/mcp/mcp_front_end_plugin.py (4)
45-56: LGTM: Clean worker instantiation pattern.The dynamic import logic correctly handles both custom and default worker classes, with appropriate fallback behavior.
63-70: LGTM: Proper delegation to worker.The worker-driven lifecycle correctly delegates MCP server creation and route registration to the worker instance, enabling the extensibility described in the PR objectives.
75-100: Good conditional logic with appropriate warnings.The base_path mounting logic correctly handles transport compatibility and provides clear logging. The URL construction at line 84 depends on proper base_path validation (see comment on config file).
126-126: Static analysis false positive - current code is correct.The static analysis tool suggests using
logger.exception()instead oflogger.error(), but this is incorrect. The coding guidelines explicitly state: "When re-raising exceptions, use bareraiseto preserve stack trace; log withlogger.error(), notlogger.exception()." The current implementation correctly useslogger.error()before re-raising.As per coding guidelines.
There was a problem hiding this comment.
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)
docs/source/workflows/mcp/mcp-server.md (1)
20-24: Use full product name on first mention.Per doc style, first use should be “NVIDIA NeMo Agent toolkit”; subsequent uses can be “NeMo Agent toolkit”. Update the first textual mention accordingly.
-This guide will cover how to use NeMo Agent toolkit as an MCP Server to publish tools using MCP. +This guide will cover how to use NVIDIA NeMo Agent toolkit as an MCP Server to publish tools using MCP.As per coding guidelines.
🧹 Nitpick comments (2)
docs/source/workflows/mcp/mcp-server.md (2)
75-77: Specifybase_pathconstraints (leading slash, trailing slash, normalization).Document required/accepted formats to prevent misconfigurations (e.g.,
"/api/v1"vs"api/v1"vs"/api/v1/") and whether normalization strips trailing slashes.Proposed note text:
:::note -The `base_path` feature requires the `streamable-http` transport. SSE transport does not support custom base paths. +The `base_path` feature requires the `streamable-http` transport. SSE transport does not support custom base paths. + +Constraints: +- Must start with `/` (e.g., `/api/v1`). +- Trailing slash is [accepted|rejected]; if accepted it is normalized to `/api/v1`. +- Empty string or `/` disables custom mounting (defaults to root). +- Whitespace and URL-encoded characters are not allowed. :::As per coding guidelines.
62-74: Clarify thatbase_pathaffects ALL routes (including/healthand/debug/*) and update documentation examples.After verifying the implementation, I confirmed that when
base_pathis configured,app.mount(base_path, mcp_server_app)mounts the entire MCP server application at that path. This means all routes get the prefix, not just/mcp:
- With
base_path: "/api/v1":
- MCP endpoint:
/api/v1/mcp- Health check:
/api/v1/health- Debug routes:
/api/v1/debug/tools/list,/api/v1/debug/memory/statsThe current documentation (lines 62-77) only mentions the MCP endpoint. However, later sections contain numerous examples that will break with
base_path:
- Lines 126-329: Multiple
curlexamples for/debug/tools/listuse root paths- Lines 355-372: Health check examples use
/healthandnat mcp client ping --url http://localhost:9901/mcpRecommended additions:
- After line 73, add explicit clarification:
When using `base_path`, all server routes are mounted under that path: - MCP endpoint: `http://localhost:9901/api/v1/mcp` - Health endpoint: `http://localhost:9901/api/v1/health` - Debug endpoints: `http://localhost:9901/api/v1/debug/tools/list` - CLI commands must include the base path: `nat mcp client ping --url http://localhost:9901/api/v1/mcp`
- Add notes to the
/debug/tools/listsection (around line 122) and health check section (around line 352) reminding users to prependbase_pathif configured.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/source/workflows/mcp/mcp-server.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
{docs/source/**/*.md,**/README.@(md|ipynb)}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{docs/source/**/*.md,**/README.@(md|ipynb)}: Use the full name “NVIDIA NeMo Agent toolkit” on first use in documentation, then “NeMo Agent toolkit”; in headings use “NeMo Agent Toolkit” (capital T)
Do not use deprecated names (Agent Intelligence toolkit, aiqtoolkit, AgentIQ, AIQ/aiq) in documentation unless explicitly referring to deprecated names
Never use “NAT”/“nat” abbreviations in documentation
Documentation must be clear/comprehensive; avoid TODOs/FIXMEs/placeholders; avoid offensive/outdated terms; ensure spelling is correct
Files:
docs/source/workflows/mcp/mcp-server.md
docs/source/**/*.md
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
docs/source/**/*.md: Documentation sources are Markdown files under docs/source
Surround code entities with backticks in docs to avoid Vale false positives
Do not use words listed in ci/vale/styles/config/vocabularies/nat/reject.txt; accepted words in accept.txt are allowed
Files:
docs/source/workflows/mcp/mcp-server.md
{**/*.py,**/*.sh,**/*.md,**/*.toml,**/*.y?(a)ml,**/*.json,**/*.txt,**/*.ini,**/*.cfg,**/*.ipynb}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{**/*.py,**/*.sh,**/*.md,**/*.toml,**/*.y?(a)ml,**/*.json,**/*.txt,**/*.ini,**/*.cfg,**/*.ipynb}: Every file must start with the standard SPDX Apache-2.0 header
Confirm copyright years are up to date when a file is changed
All source files must include the SPDX Apache-2.0 header template (copy from an existing file)
Files:
docs/source/workflows/mcp/mcp-server.md
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
docs/source/workflows/mcp/mcp-server.md
docs/source/**/*
⚙️ CodeRabbit configuration file
This directory contains the source code for the documentation. All documentation should be written in Markdown format. Any image files should be placed in the
docs/source/_staticdirectory.
Files:
docs/source/workflows/mcp/mcp-server.md
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
AnuradhaKaruppiah
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/nat/front_ends/mcp/mcp_front_end_plugin.py (1)
48-48: Add return type hint for consistency.The
_get_worker_instancemethod is missing a return type hint. For consistency with the rest of the codebase and the fix applied to_run_with_mount(line 107), add the return type.Apply this diff to add the return type hint:
- def _get_worker_instance(self): + def _get_worker_instance(self) -> MCPFrontEndPluginWorkerBase: """Get an instance of the worker class."""
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/nat/front_ends/mcp/mcp_front_end_plugin.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: In code comments use the abbreviations: nat (API namespace/CLI), nvidia-nat (package), NAT (env var prefixes); never use these abbreviations in documentation
Follow PEP 20 and PEP 8 for Python style
Run yapf with column_limit=120; yapf is used for formatting (run second)
Indent with 4 spaces (no tabs) and end each file with a single trailing newline
Use ruff (ruff check --fix) as a linter (not formatter) per pyproject.toml; fix warnings unless explicitly ignored
Respect Python naming schemes: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
When re-raising exceptions, use bare raise to preserve stack trace; log with logger.error(), not logger.exception()
When catching and logging without re-raising, use logger.exception() to capture full stack trace
Provide Google-style docstrings for every public module, class, function, and CLI command
Docstring first line must be a concise description ending with a period
Validate and sanitize all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work (HTTP, DB, file I/O)
Cache expensive computations with functools.lru_cache or an external cache when appropriate
Leverage NumPy vectorized operations when beneficial and feasible
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{src/**/*.py,packages/*/src/**/*.py}: All importable Python code must live under src/ or packages//src/
All public APIs must have Python 3.11+ type hints on parameters and return values
Prefer typing/collections.abc abstractions (e.g., Sequence over list)
Use typing.Annotated for units or metadata when useful
Treat pyright warnings as errors during development
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin.py
src/nat/**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
src/nat/**/* contains core functionality; changes should prioritize backward compatibility
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin.py
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin.py
{**/*.py,**/*.sh,**/*.md,**/*.toml,**/*.y?(a)ml,**/*.json,**/*.txt,**/*.ini,**/*.cfg,**/*.ipynb}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{**/*.py,**/*.sh,**/*.md,**/*.toml,**/*.y?(a)ml,**/*.json,**/*.txt,**/*.ini,**/*.cfg,**/*.ipynb}: Every file must start with the standard SPDX Apache-2.0 header
Confirm copyright years are up to date when a file is changed
All source files must include the SPDX Apache-2.0 header template (copy from an existing file)
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin.py
🧬 Code graph analysis (1)
src/nat/front_ends/mcp/mcp_front_end_plugin.py (1)
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py (4)
create_mcp_server(92-108)create_mcp_server(338-367)add_routes(111-130)add_routes(369-377)
🪛 Ruff (0.14.2)
src/nat/front_ends/mcp/mcp_front_end_plugin.py
129-129: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (5)
src/nat/front_ends/mcp/mcp_front_end_plugin.py (5)
24-25: LGTM! Good use of TYPE_CHECKING for forward references.The TYPE_CHECKING import enables the FastMCP type hint on line 107 without incurring runtime import overhead, which is best practice for type hints.
66-73: LGTM! Clean delegation to worker abstraction.The refactored flow properly delegates MCP server creation and route registration to the worker, enabling the extensibility feature described in the PR objectives.
78-95: LGTM! Proper handling of base_path mounting with transport-specific logic.The implementation correctly handles the SSE transport limitation and provides clear user feedback via logging. The mounting logic for other transports is well-structured.
107-151: LGTM! Well-structured mounting implementation with proper lifecycle management.The
_run_with_mountmethod correctly implements mounting at a custom base_path using a FastAPI wrapper. The type hint for themcpparameter (noted in the past review) has been properly addressed with the forward reference.The lifespan context manager properly handles the MCP server session lifecycle, and the FastAPI mounting logic is clean and correct.
129-130: Static analysis false positive: logger.error() is correct here.The Ruff hint suggesting
logger.exception()is incorrect in this context. The coding guidelines explicitly state: "When re-raising exceptions, use bareraiseto preserve stack trace; log withlogger.error(), notlogger.exception()." Since line 130 re-raises the exception with bareraise, usinglogger.error()on line 129 is the correct approach to avoid duplicate stack traces.Based on coding guidelines.
Signed-off-by: Anuradha Karuppiah <[email protected]>
|
Thanks for the review @yczhang-nv! |
Signed-off-by: Anuradha Karuppiah <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py (1)
167-167: Consider extracting the exception message to a constant.The error message could be defined as a module-level constant to follow PEP 8 style guidelines.
+# Error message constants +_NO_FUNCTIONS_ERROR = "No functions found in workflow. Please check your configuration." + # ... (in _default_add_routes) - raise RuntimeError("No functions found in workflow. Please check your configuration.") + raise RuntimeError(_NO_FUNCTIONS_ERROR)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: In code comments use the abbreviations: nat (API namespace/CLI), nvidia-nat (package), NAT (env var prefixes); never use these abbreviations in documentation
Follow PEP 20 and PEP 8 for Python style
Run yapf with column_limit=120; yapf is used for formatting (run second)
Indent with 4 spaces (no tabs) and end each file with a single trailing newline
Use ruff (ruff check --fix) as a linter (not formatter) per pyproject.toml; fix warnings unless explicitly ignored
Respect Python naming schemes: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
When re-raising exceptions, use bare raise to preserve stack trace; log with logger.error(), not logger.exception()
When catching and logging without re-raising, use logger.exception() to capture full stack trace
Provide Google-style docstrings for every public module, class, function, and CLI command
Docstring first line must be a concise description ending with a period
Validate and sanitize all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work (HTTP, DB, file I/O)
Cache expensive computations with functools.lru_cache or an external cache when appropriate
Leverage NumPy vectorized operations when beneficial and feasible
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{src/**/*.py,packages/*/src/**/*.py}: All importable Python code must live under src/ or packages//src/
All public APIs must have Python 3.11+ type hints on parameters and return values
Prefer typing/collections.abc abstractions (e.g., Sequence over list)
Use typing.Annotated for units or metadata when useful
Treat pyright warnings as errors during development
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
src/nat/**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
src/nat/**/* contains core functionality; changes should prioritize backward compatibility
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
{**/*.py,**/*.sh,**/*.md,**/*.toml,**/*.y?(a)ml,**/*.json,**/*.txt,**/*.ini,**/*.cfg,**/*.ipynb}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{**/*.py,**/*.sh,**/*.md,**/*.toml,**/*.y?(a)ml,**/*.json,**/*.txt,**/*.ini,**/*.cfg,**/*.ipynb}: Every file must start with the standard SPDX Apache-2.0 header
Confirm copyright years are up to date when a file is changed
All source files must include the SPDX Apache-2.0 header template (copy from an existing file)
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
🧬 Code graph analysis (1)
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py (5)
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (2)
add_routes(241-253)front_end_config(126-127)tests/nat/front_ends/mcp/test_mcp_custom_routes.py (1)
add_routes(35-51)src/nat/builder/workflow_builder.py (2)
WorkflowBuilder(144-1157)build(263-378)src/nat/front_ends/mcp/tool_converter.py (1)
register_function_with_mcp(254-290)src/nat/front_ends/mcp/introspection_token_verifier.py (1)
IntrospectionTokenVerifier(28-73)
🪛 Ruff (0.14.2)
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
167-167: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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: CI Pipeline / Check
🔇 Additional comments (7)
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py (7)
37-43: LGTM!The class documentation clearly explains the purpose of the base class and provides appropriate guidance on which class to inherit from.
91-101: LGTM!The abstract method provides a clear extensibility point for customizing MCP server creation. The docstring appropriately documents that subclasses of FastMCP can be returned.
103-116: LGTM!The updated documentation clearly explains how plugins should implement this method and how they can leverage the _default_add_routes() helper.
117-170: Well-structured default implementation.The method provides a comprehensive default route registration flow that subclasses can leverage. The logic for filtering functions by tool_names correctly handles both exact function names and group-name prefixes.
303-321: LGTM!The class documentation provides an excellent example demonstrating how to extend the worker class and customize server behavior while still leveraging the default route registration.
323-352: Authentication setup looks correct, but verify protocol usage.The authentication wiring correctly creates AuthSettings and IntrospectionTokenVerifier when server_auth is configured. However, please verify the protocol used in the server_url.
354-362: LGTM!The implementation correctly delegates to the base class
_default_add_routes()method, providing the standard route registration behavior while allowing subclasses to override and customize as needed.
|
/merge |
2 participants
Description
This PR updates and documents an extensibility feature in the
NeMo Agent Toolkitthat allows developers to create custom MCP server workers by subclassing MCPFrontEndPluginWorker.Developers can now extend MCPFrontEndPluginWorker to implement custom authentication, middleware, telemetry, or transport logic.
Note:
The fast api change in #1117 will be merged before this PR
By Submitting this PR I confirm:
Summary by CodeRabbit
New Features
Documentation