Add mcp/client/tool/list endpoint
#853
Conversation
Signed-off-by: Yuchen Zhang <[email protected]>
Signed-off-by: Yuchen Zhang <[email protected]>
Signed-off-by: Yuchen Zhang <[email protected]>
Signed-off-by: Yuchen Zhang <[email protected]>
Signed-off-by: Yuchen Zhang <[email protected]>
Signed-off-by: Yuchen Zhang <[email protected]>
yczhang-nv
added
feature request
WalkthroughAdds anyio-based timeouts and MCP-specific exception handling to MCP client tool listing; introduces MCPFunctionGroup state and normalizes tool wiring; awaits async function mappings in the CLI; adds a FastAPI GET endpoint to list MCP client tools with tests and documentation updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller
participant MCPClient as MCPBaseClient
participant Session as MCP Session
Caller->>MCPClient: get_tools()
rect rgb(224,239,255)
note right of MCPClient: anyio.fail_after(timeout)
MCPClient->>Session: list_tools()
alt completes before timeout
Session-->>MCPClient: tools
MCPClient-->>Caller: tools
else timeout
Session--x MCPClient: TimeoutError
MCPClient-->>Caller: MCPTimeoutError(server_name)
end
end
sequenceDiagram
autonumber
participant Client as HTTP Client
participant FastAPI as FastAPI App
participant Worker as FastApiFrontEndPluginWorker
participant Builder as WorkflowBuilder
participant Group as FunctionGroup
participant MCP as MCP Client
Client->>FastAPI: GET /mcp/client/tool/list
FastAPI->>Worker: handler(builder)
Worker->>Builder: discover MCP client groups
Builder-->>Worker: groups
loop per group
Worker->>Group: read mcp_client and server_name
Worker->>MCP: get_tools() (async, timeout-wrapped)
alt success
MCP-->>Worker: server tools
Worker->>Worker: cross-reference configured functions & overrides
else error/timeout
MCP-->>Worker: error
Worker->>Worker: mark session unhealthy, tools unavailable
end
end
Worker-->>FastAPI: MCPClientToolListResponse (JSON)
FastAPI-->>Client: 200
sequenceDiagram
autonumber
participant CLI as CLI Command
participant Fns as Awaitable[Mapping]
CLI->>Fns: await fns
Fns-->>CLI: mapping
CLI->>CLI: iterate mapping.items()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nat/cli/commands/mcp/mcp.py (1)
202-229: Await and reuse get_accessible_functions result
get_accessible_functions is async; await it once into a dict and use that mapping instead of calling .get on the coroutine or awaiting twice.- # Access functions exposed by the group - fns = group.get_accessible_functions() + # Access functions exposed by the group + fns_map = await group.get_accessible_functions() @@ - fn = fns.get(full) + fn = fns_map.get(full) @@ - for full, fn in (await fns).items(): + for full, fn in fns_map.items():
🧹 Nitpick comments (5)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py (1)
286-291: Log at error level when re-raising.Since the exception is re-raised, prefer error over warning to align with our logging guidance.
As per coding guidelines
- except Exception as e: - logger.warning("Failed to get tools: %s", e) - raise + except Exception as e: + logger.error("Failed to get tools: %s", e) + raisesrc/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (3)
1092-1107: Add return type and remove duplicate imports inside method scope.
- Add
-> Noneto the public method signature.- Avoid re-importing
AnyandBaseModelinside the function; use existing module-level imports (typing.Any,BaseModel).As per coding guidelines
- async def add_mcp_client_tool_list_route(self, app: FastAPI, builder: WorkflowBuilder): + async def add_mcp_client_tool_list_route(self, app: FastAPI, builder: WorkflowBuilder) -> None: - from typing import Any - - from pydantic import BaseModel + # Using module-level imports: typing.Any and BaseModel class MCPToolInfo(BaseModel): name: str description: str server: str available: bool class MCPClientToolListResponse(BaseModel): - mcp_clients: list[dict[str, Any]] + mcp_clients: list[dict[str, typing.Any]]
1199-1211: Log full stack traces when not re-raising.You’re handling the error and returning fallback data; use
logger.exceptionto retain stack trace context.As per coding guidelines
- except Exception as e: - logger.error(f"Error processing MCP client {group_name}: {e}") + except Exception as e: + logger.exception("Error processing MCP client %s: %s", group_name, e) mcp_clients_info.append({
1116-1124: Avoid accessing private builder state (_function_groups).This breaks encapsulation and may cause future breakage. Prefer a public accessor on
WorkflowBuilder(e.g.,get_function_groups()), or extend the builder API to supply the required data to routes.If helpful, I can propose a small
WorkflowBuilderaccessor and update this call site.packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py (1)
204-215: Tighten schema normalization; remove unused noqa and keep types clean.
- The
# noqa: E721is unnecessary since you’re usingis.- Keep the normalization but drop the comment to satisfy
ruff RUF100.- # Normalize optional typing for linter/type-checker compatibility + # Normalize optional typing for linter/type-checker compatibility single_fn = tool_fn.single_fn if single_fn is None: # Should not happen because mcp_tool_function always sets a single_fn logger.warning("Skipping tool %s because single_fn is None", function_name) continue input_schema = tool_fn.input_schema - # Convert NoneType sentinel to None for FunctionGroup.add_function signature - if input_schema is type(None): # noqa: E721 + # Convert NoneType sentinel to None for FunctionGroup.add_function signature + if input_schema is type(None): input_schema = None @@ - fn=single_fn, - input_schema=input_schema, + fn=single_fn, + input_schema=input_schema,Also applies to: 218-222
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py(3 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py(2 hunks)src/nat/cli/commands/mcp/mcp.py(1 hunks)src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{py,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.{py,yaml,yml}: Configure response_seq as a list of strings; values cycle per call, and [] yields an empty string.
Configure delay_ms to inject per-call artificial latency in milliseconds for nat_test_llm.
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.pysrc/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.pysrc/nat/cli/commands/mcp/mcp.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.py: Programmatic use: create TestLLMConfig(response_seq=[...], delay_ms=...), add with builder.add_llm("", cfg).
When retrieving the test LLM wrapper, use builder.get_llm(name, wrapper_type=LLMFrameworkEnum.) and call the framework’s method (e.g., ainvoke, achat, call).
**/*.py: In code comments/identifiers use NAT abbreviations as specified: nat for API namespace/CLI, nvidia-nat for package name, NAT for env var prefixes; do not use these abbreviations in documentation
Follow PEP 20 and PEP 8; run yapf with column_limit=120; use 4-space indentation; end files with a single trailing newline
Run ruff check --fix as linter (not formatter) using pyproject.toml config; fix warnings unless explicitly ignored
Respect naming: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise and ending with a period; surround code entities with backticks
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; profile CPU-heavy paths with cProfile or mprof before optimizing; cache expensive computations with functools.lru_cache or external cache; leverage NumPy vectorized operations when beneficial
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.pysrc/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.pysrc/nat/cli/commands/mcp/mcp.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Importable Python code inside packages must live under packages//src/
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All public APIs must have Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.pysrc/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.pysrc/nat/cli/commands/mcp/mcp.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:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.pysrc/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.pysrc/nat/cli/commands/mcp/mcp.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py
src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All importable Python code must live under src/ (or packages//src/)
Files:
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pysrc/nat/cli/commands/mcp/mcp.py
src/nat/**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Changes in src/nat should prioritize backward compatibility
Files:
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pysrc/nat/cli/commands/mcp/mcp.py
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pysrc/nat/cli/commands/mcp/mcp.py
🧬 Code graph analysis (3)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py (1)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py (7)
server_name(199-203)server_name(366-367)server_name(417-418)server_name(473-474)transport(171-172)input_schema(534-538)name(520-522)
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (4)
src/nat/builder/workflow_builder.py (2)
get_tools(541-572)get_tools(1176-1185)packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py (7)
name(520-522)get_tools(267-301)server_name(199-203)server_name(366-367)server_name(417-418)server_name(473-474)transport(171-172)packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py (1)
MCPClientConfig(89-124)src/nat/builder/function.py (1)
get_accessible_functions(485-521)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py (2)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/exception_handler.py (1)
mcp_exception_handler(199-211)packages/nvidia_nat_mcp/src/nat/plugins/mcp/exceptions.py (1)
MCPTimeoutError(60-71)
🪛 Ruff (0.13.1)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py
185-185: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
186-186: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
187-187: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
213-213: Unused noqa directive (unused: E721)
Remove unused noqa directive
(RUF100)
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py
1130-1130: Abstract raise to an inner function
(TRY301)
1130-1130: Avoid specifying long messages outside the exception class
(TRY003)
1139-1139: Do not catch blind exception: Exception
(BLE001)
1153-1153: Do not catch blind exception: Exception
(BLE001)
1164-1165: try-except-pass detected, consider logging the exception
(S110)
1164-1164: Do not catch blind exception: Exception
(BLE001)
1199-1199: Do not catch blind exception: Exception
(BLE001)
1200-1200: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1214-1214: Do not catch blind exception: Exception
(BLE001)
1215-1215: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1216-1216: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
1216-1216: Use explicit conversion flag
Replace with conversion flag
(RUF010)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py
282-282: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ 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 (2)
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (1)
244-245: New route wiring looks correct.The new MCP client tool list endpoint is registered alongside existing routes. No issues in ordering or invocation.
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py (1)
176-190: LGTM: client selection and context-managed connection.Transport selection, auth resolution, and
async with client:pattern are sound. Exposing the live client on the group is pragmatic for reuse.Please ensure
FunctionGroupallows dynamic attribute assignment across all supported runtimes. If it uses__slots__, this will fail; otherwise, it’s fine.
Signed-off-by: Yuchen Zhang <[email protected]>
Signed-off-by: Yuchen Zhang <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
docs/source/workflows/mcp/mcp-client.md (1)
257-349: Document response fields explicitly for the new endpoint.Consider adding a brief bullet list of response fields (function_group, server, transport, session_healthy, tools[].{name,description,server,available}, total_tools, available_tools) right under “Endpoint” for quick scanning, keeping the sample JSON as-is.
As per coding guidelines
tests/nat/front_ends/fastapi/test_mcp_client_endpoint.py (7)
39-41: Fix Ruff TRY003 by avoiding exception message literal.Ruff hint flags raising with a message. A bare RuntimeError is sufficient in this stub.
Apply this diff:
- if self._raise: - raise RuntimeError("Failed to get tools") + if self._raise: + raise RuntimeErrorAs per coding guidelines
75-82: Rename and type the fixture per repo conventions.Fixtures should use the fixture_ prefix and set name in the decorator; add return type.
Apply this diff:
-@pytest_asyncio.fixture -async def app_worker(): +@pytest_asyncio.fixture(name="app_worker") +async def fixture_app_worker() -> tuple[FastAPI, FastApiFrontEndPluginWorker]: cfg = Config() worker = FastApiFrontEndPluginWorker(cfg) app = FastAPI() worker.set_cors_config(app) return app, workerAs per coding guidelines
18-20: Prefer httpx.AsyncClient for async tests instead of TestClient.Using TestClient inside async tests can cause nested event loop issues. Switch to httpx.AsyncClient with ASGITransport.
Apply this diff:
from fastapi import FastAPI -from fastapi.testclient import TestClient +import httpxAs per coding guidelines
85-88: Add type hints to test signature.Annotate parameters and return type.
Apply this diff:
-async def test_mcp_client_tool_list_success_with_alias(app_worker): +async def test_mcp_client_tool_list_success_with_alias( + app_worker: tuple[FastAPI, FastApiFrontEndPluginWorker] +) -> None:As per coding guidelines
109-127: Use httpx.AsyncClient with ASGITransport in async test.Prevents flakiness with nested event loops and aligns with FastAPI testing recommendations.
Apply this diff:
- with TestClient(app) as client_http: - resp = client_http.get("/mcp/client/tool/list") - assert resp.status_code == 200 - data = resp.json() + async with httpx.AsyncClient(transport=httpx.ASGITransport(app=app), base_url="http://test") as client_http: + resp = await client_http.get("/mcp/client/tool/list") + assert resp.status_code == 200 + data = resp.json() assert "mcp_clients" in data assert len(data["mcp_clients"]) == 1 group = data["mcp_clients"][0] assert group["function_group"] == group_name assert group["server"].startswith("streamable-http:") + assert group["transport"] == "streamable-http" assert group["session_healthy"] is True assert group["total_tools"] == 1 assert group["available_tools"] == 1 assert len(group["tools"]) == 1 tool = group["tools"][0] assert tool["name"] == "alias_tool" assert tool["available"] is True assert tool["server"].startswith("streamable-http:") # Prefer workflow/override description assert tool["description"] == "Overridden desc"As per coding guidelines
131-139: Add type hints to test signature.Annotate parameters and return type.
Apply this diff:
-async def test_mcp_client_tool_list_unhealthy_marks_unavailable(app_worker): +async def test_mcp_client_tool_list_unhealthy_marks_unavailable( + app_worker: tuple[FastAPI, FastApiFrontEndPluginWorker] +) -> None:As per coding guidelines
154-165: Use httpx.AsyncClient with ASGITransport in async test (unhealthy case).Mirror the change applied to the healthy test.
Apply this diff:
- with TestClient(app) as client_http: - resp = client_http.get("/mcp/client/tool/list") - assert resp.status_code == 200 - data = resp.json() + async with httpx.AsyncClient(transport=httpx.ASGITransport(app=app), base_url="http://test") as client_http: + resp = await client_http.get("/mcp/client/tool/list") + assert resp.status_code == 200 + data = resp.json() group = data["mcp_clients"][0] assert group["function_group"] == group_name assert group["session_healthy"] is False assert group["total_tools"] == 2 assert group["available_tools"] == 0 assert len(group["tools"]) == 2 assert all(t["available"] is False for t in group["tools"])As per coding guidelines
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/source/workflows/mcp/mcp-client.md(2 hunks)tests/nat/front_ends/fastapi/test_mcp_client_endpoint.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{py,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.{py,yaml,yml}: Configure response_seq as a list of strings; values cycle per call, and [] yields an empty string.
Configure delay_ms to inject per-call artificial latency in milliseconds for nat_test_llm.
Files:
tests/nat/front_ends/fastapi/test_mcp_client_endpoint.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.py: Programmatic use: create TestLLMConfig(response_seq=[...], delay_ms=...), add with builder.add_llm("", cfg).
When retrieving the test LLM wrapper, use builder.get_llm(name, wrapper_type=LLMFrameworkEnum.) and call the framework’s method (e.g., ainvoke, achat, call).
**/*.py: In code comments/identifiers use NAT abbreviations as specified: nat for API namespace/CLI, nvidia-nat for package name, NAT for env var prefixes; do not use these abbreviations in documentation
Follow PEP 20 and PEP 8; run yapf with column_limit=120; use 4-space indentation; end files with a single trailing newline
Run ruff check --fix as linter (not formatter) using pyproject.toml config; fix warnings unless explicitly ignored
Respect naming: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise and ending with a period; surround code entities with backticks
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; profile CPU-heavy paths with cProfile or mprof before optimizing; cache expensive computations with functools.lru_cache or external cache; leverage NumPy vectorized operations when beneficial
Files:
tests/nat/front_ends/fastapi/test_mcp_client_endpoint.py
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Unit tests reside under tests/ and should use markers defined in pyproject.toml (e.g., integration)
Files:
tests/nat/front_ends/fastapi/test_mcp_client_endpoint.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/fastapi/test_mcp_client_endpoint.py
{tests/**/*.py,examples/*/tests/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{tests/**/*.py,examples/*/tests/**/*.py}: Use pytest (with pytest-asyncio for async); name test files test_*.py; test functions start with test_; extract repeated code into fixtures; fixtures must set name in decorator and be named with fixture_ prefix
Mock external services with pytest_httpserver or unittest.mock; do not hit live endpoints
Mark expensive tests with @pytest.mark.slow or @pytest.mark.integration
Files:
tests/nat/front_ends/fastapi/test_mcp_client_endpoint.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:
tests/nat/front_ends/fastapi/test_mcp_client_endpoint.pydocs/source/workflows/mcp/mcp-client.md
docs/source/**/*.md
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
docs/source/**/*.md: Use the official naming throughout documentation: first use “NVIDIA NeMo Agent toolkit”, subsequent “NeMo Agent toolkit”; never use deprecated names (Agent Intelligence toolkit, aiqtoolkit, AgentIQ, AIQ/aiq)
Documentation sources are Markdown files under docs/source; images belong in docs/source/_static
Keep docs in sync with code; documentation pipeline must pass Sphinx and link checks; avoid TODOs/FIXMEs/placeholders; avoid offensive/outdated terms; ensure spelling correctness
Do not use words listed in ci/vale/styles/config/vocabularies/nat/reject.txt; accepted terms in accept.txt are allowed
Files:
docs/source/workflows/mcp/mcp-client.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-client.md
🧬 Code graph analysis (1)
tests/nat/front_ends/fastapi/test_mcp_client_endpoint.py (3)
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (2)
set_cors_config(153-183)add_mcp_client_tool_list_route(1092-1252)packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py (2)
description(525-531)transport(171-172)packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py (3)
MCPClientConfig(89-124)MCPServerConfig(43-86)MCPToolOverrideConfig(35-40)
🪛 Ruff (0.13.1)
tests/nat/front_ends/fastapi/test_mcp_client_endpoint.py
40-40: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (1)
docs/source/workflows/mcp/mcp-client.md (1)
185-202: Section rename is clear and helpful.“via CLI” makes the scope explicit and aligns with the parallel HTTP docs below.
Signed-off-by: Yuchen Zhang <[email protected]>
Signed-off-by: Yuchen Zhang <[email protected]>
…ool-list-endpoint Signed-off-by: Yuchen Zhang <[email protected]>
Signed-off-by: Yuchen Zhang <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py (1)
288-290: Use error level when re-raising.When re-raising, prefer logger.error over warning.
As per coding guidelines
- except Exception as e: - logger.warning("Failed to get tools: %s", e) - raise + except Exception as e: + logger.error("Failed to get tools: %s", e) + raisetests/nat/front_ends/fastapi/test_mcp_client_endpoint.py (1)
75-82: Align pytest fixture naming with repo guidelines.Rename function to fixture_… and set name= to keep external reference stable.
As per coding guidelines
-@pytest_asyncio.fixture -async def app_worker(): +@pytest_asyncio.fixture(name="app_worker") +async def fixture_app_worker(): cfg = Config() worker = FastApiFrontEndPluginWorker(cfg) app = FastAPI() worker.set_cors_config(app) return app, workerpackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py (1)
256-258: Remove unused noqa (RUF100) and keep the comparison clean.The E721 noqa is unnecessary here.
- # Convert NoneType sentinel to None for FunctionGroup.add_function signature - if input_schema is type(None): # noqa: E721 - input_schema = None + # Convert NoneType sentinel to None for FunctionGroup.add_function signature + if input_schema is type(None): + input_schema = None
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py(2 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py(3 hunks)src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py(3 hunks)tests/nat/front_ends/fastapi/test_mcp_client_endpoint.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{py,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.{py,yaml,yml}: Configure response_seq as a list of strings; values cycle per call, and [] yields an empty string.
Configure delay_ms to inject per-call artificial latency in milliseconds for nat_test_llm.
Files:
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pytests/nat/front_ends/fastapi/test_mcp_client_endpoint.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.py: Programmatic use: create TestLLMConfig(response_seq=[...], delay_ms=...), add with builder.add_llm("", cfg).
When retrieving the test LLM wrapper, use builder.get_llm(name, wrapper_type=LLMFrameworkEnum.) and call the framework’s method (e.g., ainvoke, achat, call).
**/*.py: In code comments/identifiers use NAT abbreviations as specified: nat for API namespace/CLI, nvidia-nat for package name, NAT for env var prefixes; do not use these abbreviations in documentation
Follow PEP 20 and PEP 8; run yapf with column_limit=120; use 4-space indentation; end files with a single trailing newline
Run ruff check --fix as linter (not formatter) using pyproject.toml config; fix warnings unless explicitly ignored
Respect naming: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise and ending with a period; surround code entities with backticks
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; profile CPU-heavy paths with cProfile or mprof before optimizing; cache expensive computations with functools.lru_cache or external cache; leverage NumPy vectorized operations when beneficial
Files:
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pytests/nat/front_ends/fastapi/test_mcp_client_endpoint.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py
src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All importable Python code must live under src/ (or packages//src/)
Files:
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py
src/nat/**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Changes in src/nat should prioritize backward compatibility
Files:
src/nat/front_ends/fastapi/fastapi_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/fastapi/fastapi_front_end_plugin_worker.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All public APIs must have Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Files:
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.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/fastapi/fastapi_front_end_plugin_worker.pytests/nat/front_ends/fastapi/test_mcp_client_endpoint.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Unit tests reside under tests/ and should use markers defined in pyproject.toml (e.g., integration)
Files:
tests/nat/front_ends/fastapi/test_mcp_client_endpoint.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/fastapi/test_mcp_client_endpoint.py
{tests/**/*.py,examples/*/tests/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{tests/**/*.py,examples/*/tests/**/*.py}: Use pytest (with pytest-asyncio for async); name test files test_*.py; test functions start with test_; extract repeated code into fixtures; fixtures must set name in decorator and be named with fixture_ prefix
Mock external services with pytest_httpserver or unittest.mock; do not hit live endpoints
Mark expensive tests with @pytest.mark.slow or @pytest.mark.integration
Files:
tests/nat/front_ends/fastapi/test_mcp_client_endpoint.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Importable Python code inside packages must live under packages//src/
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py
🧠 Learnings (3)
📚 Learning: 2025-09-23T18:39:15.023Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-09-23T18:39:15.023Z
Learning: Applies to **/*.py : Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Applied to files:
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py
📚 Learning: 2025-08-28T20:03:37.650Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py:0-0
Timestamp: 2025-08-28T20:03:37.650Z
Learning: Function groups in NAT automatically namespace their function names with the group name (e.g., "user_report.get", "user_report.put"), making function name collisions impossible by design. No collision detection is needed when merging functions from function groups.
Applied to files:
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py
📚 Learning: 2025-08-28T14:28:34.779Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/builder/component_utils.py:110-114
Timestamp: 2025-08-28T14:28:34.779Z
Learning: FunctionGroupBaseConfig and FunctionBaseConfig are separate class hierarchies that both inherit from (TypedBaseModel, BaseModelRegistryTag). FunctionGroupBaseConfig does not inherit from FunctionBaseConfig, so isinstance checks between them won't interfere with each other.
Applied to files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py
🧬 Code graph analysis (4)
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (4)
src/nat/builder/function.py (1)
Function(46-302)src/nat/builder/workflow_builder.py (3)
WorkflowBuilder(144-1114)get_tools(541-572)get_tools(1176-1185)packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py (8)
name(520-522)description(525-531)get_tools(267-301)server_name(199-203)server_name(366-367)server_name(417-418)server_name(473-474)transport(171-172)packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py (3)
MCPClientConfig(133-168)mcp_client(49-51)mcp_client(54-56)
tests/nat/front_ends/fastapi/test_mcp_client_endpoint.py (3)
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (2)
config(120-121)add_mcp_client_tool_list_route(1093-1257)packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py (9)
description(525-531)server_name(199-203)server_name(366-367)server_name(417-418)server_name(473-474)get_tools(267-301)transport(171-172)url(362-363)url(469-470)packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py (5)
mcp_client(49-51)mcp_client(54-56)MCPClientConfig(133-168)MCPServerConfig(87-130)MCPToolOverrideConfig(79-84)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py (2)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/exception_handler.py (1)
mcp_exception_handler(199-211)packages/nvidia_nat_mcp/src/nat/plugins/mcp/exceptions.py (1)
MCPTimeoutError(60-71)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py (2)
src/nat/builder/function.py (1)
FunctionGroup(352-714)packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py (9)
args(421-422)server_name(199-203)server_name(366-367)server_name(417-418)server_name(473-474)transport(171-172)input_schema(534-538)name(520-522)description(525-531)
🪛 Ruff (0.13.1)
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py
1134-1134: Abstract raise to an inner function
(TRY301)
1134-1134: Avoid specifying long messages outside the exception class
(TRY003)
1144-1144: Redundant exception object included in logging.exception call
(TRY401)
1161-1161: Redundant exception object included in logging.exception call
(TRY401)
1170-1171: try-except-pass detected, consider logging the exception
(S110)
1170-1170: Do not catch blind exception: Exception
(BLE001)
1176-1176: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
1204-1204: Do not catch blind exception: Exception
(BLE001)
1205-1205: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1219-1219: Do not catch blind exception: Exception
(BLE001)
1220-1220: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1221-1221: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
1221-1221: Use explicit conversion flag
Replace with conversion flag
(RUF010)
tests/nat/front_ends/fastapi/test_mcp_client_endpoint.py
40-40: Avoid specifying long messages outside the exception class
(TRY003)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py
282-282: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py
256-256: Unused noqa directive (unused: E721)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (8)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py (2)
266-267: Good: public API annotated and decorated.Adding the return type and @mcp_exception_handler aligns with typing and exception handling standards.
281-283: Chain the original timeout exception (ruff B904).Preserve the cause when raising MCPTimeoutError.
As per coding guidelines
- except TimeoutError as e: - from nat.plugins.mcp.exceptions import MCPTimeoutError - raise MCPTimeoutError(self.server_name, e) + except TimeoutError as e: + from nat.plugins.mcp.exceptions import MCPTimeoutError + raise MCPTimeoutError(self.server_name, e) from epackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py (1)
228-230: LGTM: direct assignments over setattr.Switching to property-backed assignments resolves ruff B010 and improves clarity.
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (5)
1140-1146: Use parameterized logger.exception; avoid f-strings and passing the exception (TRY401).logger.exception already includes the stack; keep parameters structured.
As per coding guidelines
- except Exception as e: - logger.exception(f"Failed to connect to MCP server {client.server_name}: {e}") + except Exception as e: + logger.exception("Failed to connect to MCP server %s", client.server_name) session_healthy = False
1160-1162: Same: parameterized logger.exception for function retrieval.Avoid f-strings and redundant exception object.
As per coding guidelines
- except Exception as e: - logger.exception(f"Failed to get accessible functions for group {group_name}: {e}") + except Exception as e: + logger.exception("Failed to get accessible functions for group %s", group_name)
1170-1171: Don’t swallow exceptions.Log the exception instead of pass.
As per coding guidelines
- except Exception: - pass + except Exception: + logger.exception("Failed to process tool overrides for group %s", group_name)
1204-1215: Use logger.exception when handling and continuing.Include stack trace; avoid f-strings.
As per coding guidelines
- except Exception as e: - logger.error(f"Error processing MCP client {group_name}: {e}") + except Exception as e: + logger.exception("Error processing MCP client %s", group_name) mcp_clients_info.append({ "function_group": group_name, "server": "unknown", "transport": config.server.transport if config.server else "unknown", "session_healthy": False, - "error": str(e), + "error": str(e), "tools": [], "total_tools": 0, "workflow_tools": 0 })
1219-1221: Preserve cause and parameterize logging when re-raising (ruff B904, TRY400).Chain HTTPException and avoid f-strings in logs.
As per coding guidelines
- except Exception as e: - logger.error(f"Error in MCP client tool list endpoint: {e}") - raise HTTPException(status_code=500, detail=f"Failed to retrieve MCP client information: {str(e)}") + except Exception as e: + logger.error("Error in MCP client tool list endpoint: %s", e) + raise HTTPException( + status_code=500, + detail=f"Failed to retrieve MCP client information: {e!s}", + ) from e
Signed-off-by: Yuchen Zhang <[email protected]>
Signed-off-by: Yuchen Zhang <[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 (1)
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{py,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.{py,yaml,yml}: Configure response_seq as a list of strings; values cycle per call, and [] yields an empty string.
Configure delay_ms to inject per-call artificial latency in milliseconds for nat_test_llm.
Files:
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.py: Programmatic use: create TestLLMConfig(response_seq=[...], delay_ms=...), add with builder.add_llm("", cfg).
When retrieving the test LLM wrapper, use builder.get_llm(name, wrapper_type=LLMFrameworkEnum.) and call the framework’s method (e.g., ainvoke, achat, call).
**/*.py: In code comments/identifiers use NAT abbreviations as specified: nat for API namespace/CLI, nvidia-nat for package name, NAT for env var prefixes; do not use these abbreviations in documentation
Follow PEP 20 and PEP 8; run yapf with column_limit=120; use 4-space indentation; end files with a single trailing newline
Run ruff check --fix as linter (not formatter) using pyproject.toml config; fix warnings unless explicitly ignored
Respect naming: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise and ending with a period; surround code entities with backticks
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; profile CPU-heavy paths with cProfile or mprof before optimizing; cache expensive computations with functools.lru_cache or external cache; leverage NumPy vectorized operations when beneficial
Files:
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py
src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All importable Python code must live under src/ (or packages//src/)
Files:
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py
src/nat/**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Changes in src/nat should prioritize backward compatibility
Files:
src/nat/front_ends/fastapi/fastapi_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/fastapi/fastapi_front_end_plugin_worker.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All public APIs must have Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Files:
src/nat/front_ends/fastapi/fastapi_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/fastapi/fastapi_front_end_plugin_worker.py
🧠 Learnings (2)
📚 Learning: 2025-09-23T18:39:15.023Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-09-23T18:39:15.023Z
Learning: Applies to **/*.py : Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Applied to files:
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py
📚 Learning: 2025-08-28T20:03:37.650Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py:0-0
Timestamp: 2025-08-28T20:03:37.650Z
Learning: Function groups in NAT automatically namespace their function names with the group name (e.g., "user_report.get", "user_report.put"), making function name collisions impossible by design. No collision detection is needed when merging functions from function groups.
Applied to files:
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py
🧬 Code graph analysis (1)
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (4)
src/nat/builder/function.py (2)
Function(46-302)get_accessible_functions(486-522)src/nat/builder/workflow_builder.py (3)
WorkflowBuilder(144-1114)get_tools(541-572)get_tools(1176-1185)packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py (8)
name(520-522)description(525-531)get_tools(267-301)server_name(199-203)server_name(366-367)server_name(417-418)server_name(473-474)transport(171-172)packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py (3)
MCPClientConfig(133-168)mcp_client(49-51)mcp_client(54-56)
🪛 Ruff (0.13.1)
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py
1134-1134: Abstract raise to an inner function
(TRY301)
1134-1134: Avoid specifying long messages outside the exception class
(TRY003)
1144-1144: Redundant exception object included in logging.exception call
(TRY401)
1161-1161: Redundant exception object included in logging.exception call
(TRY401)
1174-1175: try-except-pass detected, consider logging the exception
(S110)
1174-1174: Do not catch blind exception: Exception
(BLE001)
1180-1180: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
1215-1215: Do not catch blind exception: Exception
(BLE001)
1216-1216: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1230-1230: Do not catch blind exception: Exception
(BLE001)
1231-1231: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1232-1232: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
1232-1232: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ 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 (3)
src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (3)
1174-1175: Log override parsing failures instead of swallowing them.Catching
Exceptionand doing nothing violates the logging rules and hides misconfigurations, making the endpoint harder to diagnose. Please log (at minimum) before continuing. As per coding guidelines- except Exception: - pass + except Exception: + logger.exception("Failed to process tool overrides for group %s", group_name)
1178-1213: Fix nondeterministic tool pairing between functions and short names.
zip(configured_full_to_fn.values(), configured_short_names)is nondeterministic because the short names come from a set, so descriptions, availability, and alias mapping can be assigned to the wrong tool. Iterate deterministically over the configured functions and derive the short name from each key. As per coding guidelines- configured_short_names: set[str] = set() + configured_short_names: list[str] = [] ... - configured_short_names = {name.split('.', 1)[1] for name in accessible_functions.keys()} + configured_short_names = [name.split('.', 1)[1] for name in accessible_functions.keys()] ... - for wf_fn, fn_short in zip(configured_full_to_fn.values(), configured_short_names): + for full_name, wf_fn in configured_full_to_fn.items(): + fn_short = full_name.split('.', 1)[1] orig_name = alias_to_original.get(fn_short, fn_short)
1230-1233: Log and re-raise the outer failure correctly.The top-level handler should use
logger.exception(no f-string) and chain the original error viafrom excso FastAPI keeps the traceback. As per coding guidelines- except Exception as e: - logger.error(f"Error in MCP client tool list endpoint: {e}") - raise HTTPException(status_code=500, detail=f"Failed to retrieve MCP client information: {str(e)}") + except Exception as exc: + logger.exception("Error in MCP client tool list endpoint") + raise HTTPException( + status_code=500, + detail=f"Failed to retrieve MCP client information: {exc!s}", + ) from exc
|
/merge |
2 participants
Description
Closes AIQ-1943
By Submitting this PR I confirm:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests