Skip to content

Conversation

@zhongxuanwang-nv
Copy link
Member

@zhongxuanwang-nv zhongxuanwang-nv commented Sep 17, 2025

Description

Closes AIQ-1851

This will be a custom MCP route similar to "/health" and used for displaying tools and their schema without the need for an MCP client.

  • Added: Public HTTP route GET /debug/tools/list in src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py; supports name (repeatable or comma-separated) and detail flags; returns server_name, count, and tools[] with name, description, is_workflow, and optional schema. 
  • Changed: MCP front-end wiring to register the new custom route and include schema handling (Pydantic via model_json_schema; ChatRequest simplified to single query field). 
  • Removed: None
  • Why it matters: Enables introspecting registered MCP tools and their input schemas without an MCP client—useful for debugging, observability, and integration verification. 
  • Breaking changes / Migrations: None
  • Risks / Edge cases: Public debug endpoint may expose tool metadata/schemas; ensure it’s restricted/disabled in production or behind auth; 404 behavior for unknown tools confirmed. 
  • Docs: Added docs in docs/source/workflows/mcp/mcp-server.md with curl examples and parameter semantics. 

By Submitting this PR I confirm:

  • I am familiar with the Contributing Guidelines.
  • We require that all contributors "sign-off" on their commits. This certifies that the contribution is your original work, or you have rights to submit it under the same license, or a compatible license.
    • Any contribution which contains commits that are not Signed-Off will not be accepted.
  • When the PR is ready for review, new or existing tests cover these changes.
  • When the PR is ready for review, the documentation is up to date with these changes.

Summary by CodeRabbit

  • New Features

    • Public debug endpoint /debug/tools/list to inspect available MCP tools; supports filtering by name (repeated or comma-separated), a detail flag to include per-tool input schemas, and returns count, tools, and server name; unknown tool names return 404.
  • Documentation

    • Added docs describing the debug endpoint, usage examples for name/detail combinations, and guidance for obtaining detailed tool schemas.
  • Tests

    • Added comprehensive tests covering route structure, name/detail behaviors, schema representations, edge cases, and error handling.

Signed-off-by: Daniel Wang <daniewang@nvidia.com>
@zhongxuanwang-nv zhongxuanwang-nv self-assigned this Sep 17, 2025
@zhongxuanwang-nv zhongxuanwang-nv added feature request New feature or request non-breaking Non-breaking change labels Sep 17, 2025
@coderabbitai
Copy link

coderabbitai bot commented Sep 17, 2025

Walkthrough

Adds a new public debug HTTP endpoint (/debug/tools/list) to the MCP front-end for listing registered tools and optionally their input schemas; implements the handler in MCPFrontEndPluginWorkerBase, wires it into routing, adds tests covering many query combinations, and documents the endpoint.

Changes

Cohort / File(s) Summary
Docs: MCP server debug endpoint
docs/source/workflows/mcp/mcp-server.md
Adds a new section documenting GET /debug/tools/list: query params name (single/repeated/comma-separated) and detail, response shape, selection modes, schema inclusion rules, examples, CLI guidance, and an extraneous empty bash block.
MCP front-end: debug endpoint implementation
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
Adds _setup_debug_endpoints(self, mcp, functions) to MCPFrontEndPluginWorkerBase, registers GET /debug/tools/list, parses name (split/repeat/dedupe), applies detail logic (default depends on whether names were provided), returns 404 for missing requested tools, builds per-tool entries (name, description via get_function_description, is_workflow, optional schema), simplifies chat-style schemas to a single query field, uses model_json_schema when available, wires the setup call from add_routes, and adds imports for typing.Any and HTTPException.
Tests: debug route coverage
tests/nat/front_ends/mcp/test_mcp_debug_routes.py
Adds comprehensive tests exercising the new endpoint: route existence and structure, detail behavior with/without name, name formats (single, repeated, comma-separated), deduplication, missing-name 404, schema generation for Pydantic/chat/no-schema tools, and workflow detection. Introduces fixtures, mock Pydantic schemas, mock function classes, WorkflowMock, and uses worker._setup_debug_endpoints to install the route for tests.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Client
  participant HTTP as HTTP Server
  participant Worker as MCPFrontEndPluginWorkerBase
  participant MCP as FastMCP

  note over Worker,MCP: Endpoint: GET /debug/tools/list\nQuery: name (repeat/comma), detail=true|false

  Client->>HTTP: GET /debug/tools/list?name=toolA&detail=true
  HTTP->>Worker: invoke route handler
  Worker->>MCP: retrieve registered functions/tools
  alt name provided
    Worker->>Worker: parse `name` values (split, dedupe)
    alt any requested missing
      Worker-->>HTTP: 404 Not Found (missing names)
    else
      opt include schemas
        Worker->>Worker: build schema per tool (Pydantic model_json_schema or ChatRequest summary)
      end
      Worker-->>HTTP: 200 OK (server_name, count, tools[] or single tool)
    end
  else no name provided
    alt detail=true
      Worker->>Worker: include schemas for all tools
    else
      Worker->>Worker: return simplified list (no schemas)
    end
    Worker-->>HTTP: 200 OK (server_name, count, tools[])
  end
  HTTP-->>Client: JSON response
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly and accurately summarizes the primary change—adding a /debug HTTP route to the NAT MCP frontend to expose MCP tools—uses imperative mood and is within the ~72-character guideline, so it matches the PR objectives and the described code/docs changes.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@zhongxuanwang-nv
Copy link
Member Author

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Sep 17, 2025

@zhongxuanwang-nv I'll review the changes in this PR that adds the /debug route to the NAT MCP frontend for exposing MCP tools and their schema.


🧠 Learnings used
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#710
File: .coderabbit.yaml:33-33
Timestamp: 2025-08-25T15:24:13.143Z
Learning: User feedback indicates that CodeRabbit's chat commands, particularly `coderabbitai review` for manual incremental reviews, need better documentation and discoverability within repositories, especially when auto_incremental_review is disabled.
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

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_worker.py (1)

270-272: Gate /debug endpoints behind an explicit opt‑in flag.

Avoid exposing internals by default when bound to 0.0.0.0. Make the route opt‑in via an env flag.

Apply this diff:

-        # After registration, expose debug endpoints for tool/schema inspection
-        self._setup_debug_endpoints(mcp, functions)
+        # After registration, optionally expose debug endpoints for tool/schema inspection
+        if os.getenv("NAT_MCP_ENABLE_DEBUG_ROUTES", "").strip().lower() in {"1", "true", "yes", "on"}:
+            logger.info("Debug endpoints enabled via NAT_MCP_ENABLE_DEBUG_ROUTES.")
+            self._setup_debug_endpoints(mcp, functions)
+        else:
+            logger.info("Debug endpoints disabled. Set NAT_MCP_ENABLE_DEBUG_ROUTES=1 to enable.")

Also add the import:

 import logging
+import os
🧹 Nitpick comments (7)
docs/source/workflows/mcp/mcp-server.md (1)

101-116: Show error/404 behavior and detail toggles for completeness.

Add a small example of the 404 response and one “detail=false” example with names to match implementation.

Apply this doc tweak:

 # ...existing examples...
 
 You can control the amount of detail using the `detail` query parameter:
 
 - When requesting specific tool(s) with `name`, detailed schema is returned by default. Pass `detail=false` to suppress schemas:
 
 ```bash
 curl -s "http://localhost:9901/debug/tools/list?name=calculator_multiply&detail=false" | jq

+If any requested tool is missing, a 404 is returned:
+
+bash +curl -s "http://localhost:9901/debug/tools/list?name=not_a_tool" | jq +# { +# "error": "Tool(s) not found: not_a_tool", +# "available_tools": ["calculator_multiply", "..."] +# } +


</blockquote></details>
<details>
<summary>src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py (6)</summary><blockquote>

`106-112`: **Fix docstring and add return type.**

Docstring implies single tool; code supports multiple. Add return type for compliance.



Apply this diff:

```diff
-    def _setup_debug_endpoints(self, mcp: FastMCP, functions: dict[str, Function]):
+    def _setup_debug_endpoints(self, mcp: FastMCP, functions: dict[str, Function]) -> None:
         """Set up HTTP debug endpoints for introspecting tools and schemas.

-        Exposes:
-          - GET /debug/tools/list: List tools. Optional query param `name` returns details for a single tool.
+        Exposes:
+          - GET /debug/tools/list: List tools. Optional query param `name` (one or more) returns details for the selected tool(s).
         """

121-131: Deduplicate requested tool names while preserving order.

Avoid repeated entries when name is provided multiple times.

Apply this diff:

             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)
+            # Deduplicate while preserving the first occurrence order
+            names = list(dict.fromkeys(names))
             detail_raw = request.query_params.get("detail")

142-170: Tighten typing and avoid brittle ChatRequest detection.

  • Add return type.
  • String‑matching “ChatRequest” can false‑trigger. Prefer a type/marker check if available (e.g., issubclass(schema, ChatRequest) or hasattr(schema, "nat_chat_request")).

Apply this diff:

-            def build_schema_info(fn: Function):
+            def build_schema_info(fn: Function) -> dict[str, Any] | None:
                 schema = getattr(fn, "input_schema", None)
                 is_chat_request = False
                 if schema is not None:
                     schema_name_part = getattr(schema, "__name__", None)
                     qual_name_part = getattr(schema, "__qualname__", "")
                     schema_name = schema_name_part or qual_name_part
                     if schema_name and "ChatRequest" in str(schema_name):
                         is_chat_request = True

Follow‑up: if a concrete ChatRequest type is available in this module, switch to an issubclass check.


188-193: Use precise workflow detection.

hasattr(fn, "run") may produce false positives. Prefer isinstance(fn, Workflow) (already imported).

Apply this diff:

                     selected_entry: dict[str, Any] = {
                         "name": n,
                         "description": get_function_description(fn),
-                        "is_workflow": hasattr(fn, "run"),
+                        "is_workflow": isinstance(fn, Workflow),
                     }

209-222: Always include is_workflow for consistent shape.

Clients shouldn’t have to branch on detail to know if an entry is a workflow.

Apply this diff:

             for name, fn in functions.items():
                 list_entry: dict[str, Any] = {
                     "name": name,
                     "description": get_function_description(fn),
+                    "is_workflow": isinstance(fn, Workflow),
                 }
-                if include_schemas:
-                    list_entry["schema"] = build_schema_info(fn)
-                    list_entry["is_workflow"] = hasattr(fn, "run")
+                if include_schemas:
+                    list_entry["schema"] = build_schema_info(fn)
                 tools.append(list_entry)

113-208: Add basic route tests (Starlette TestClient).

Recommend adding coverage for: no params, names (single/multi), detail toggles, 404 on missing, schema presence for ChatRequest.

I can generate a minimal pytest using Starlette’s TestClient around FastMCP to assert response shapes. Want me to open a follow‑up PR with tests?

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 46a348d and 38985ab.

📒 Files selected for processing (2)
  • docs/source/workflows/mcp/mcp-server.md (1 hunks)
  • src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py (3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*

⚙️ 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 raise statements to maintain the original stack trace,
      and use logger.error() (not logger.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.txt file, words that might appear to be
spelling mistakes but are listed in the ci/vale/styles/config/vocabularies/nat/accept.txt file 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
  • docs/source/workflows/mcp/mcp-server.md
src/nat/**/*

⚙️ 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
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/_static directory.

Files:

  • docs/source/workflows/mcp/mcp-server.md

Signed-off-by: Daniel Wang <daniewang@nvidia.com>
Signed-off-by: Daniel Wang <daniewang@nvidia.com>
Signed-off-by: Daniel Wang <daniewang@nvidia.com>
Signed-off-by: Daniel Wang <daniewang@nvidia.com>
@zhongxuanwang-nv zhongxuanwang-nv marked this pull request as ready for review September 18, 2025 03:08
@zhongxuanwang-nv zhongxuanwang-nv requested a review from a team as a code owner September 18, 2025 03:08
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

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_worker.py (1)

243-245: Gate debug endpoints behind config/flag.

These routes enumerate internal tools/schemas; expose only when explicitly enabled (e.g., config flag or env var) or add simple auth.

Example:

+import os
@@
-        self._setup_debug_endpoints(mcp, functions)
+        if os.getenv("NAT_MCP_DEBUG_ROUTES", "0") == "1":
+            self._setup_debug_endpoints(mcp, functions)

Or add debug_routes_enabled: bool = False to MCPFrontEndConfig and check it here.

🧹 Nitpick comments (6)
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py (3)

124-131: Preserve request order and deduplicate deterministically.

Using set(...) loses order and can yield unstable output. Build an ordered, de‑duplicated list.

-            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_values = request.query_params.getlist("name")
+            names: list[str] = []
+            seen: set[str] = set()
+            for raw in raw_values:
+                for p in (s.strip() for s in raw.split(",")):
+                    if not p or p in seen:
+                        continue
+                    seen.add(p)
+                    names.append(p)

189-195: Consistent JSON error shape for 404s.

Raising HTTPException yields Starlette’s default body; consider returning a JSON shape consistent with success responses (helps clients).

-                except KeyError as e:
-                    raise HTTPException(status_code=404, detail=f"Tool \"{e.args[0]}\" not found.") from e
+                except KeyError as e:
+                    from starlette.responses import JSONResponse
+                    return JSONResponse(
+                        {"count": 0, "tools": [], "server_name": mcp.name, "error": f'Tool "{e.args[0]}" not found.'},
+                        status_code=404)

199-201: Avoid caching debug responses.

Add Cache-Control: no-store to prevent intermediaries from caching potentially sensitive metadata.

-            return JSONResponse(
-                _build_final_json(functions_to_include, _parse_detail_param(detail_raw, has_names=bool(names))))
+            return JSONResponse(
+                _build_final_json(functions_to_include, _parse_detail_param(detail_raw, has_names=bool(names))),
+                headers={"Cache-Control": "no-store"})
tests/nat/front_ends/mcp/test_mcp_debug_routes.py (3)

35-39: Avoid mutable default for lists in Pydantic models.

Use Field(default_factory=list).

-from pydantic import BaseModel
+from pydantic import BaseModel, Field
@@
-class ChatRequestSchema(BaseModel):
+class ChatRequestSchema(BaseModel):
     """Mock ChatRequest schema for testing special handling."""
-    messages: list = []
+    messages: list = Field(default_factory=list)
     model: str | None = None

126-131: Tests rely on a private method.

Prefer going through the public add_routes(...) to reduce coupling, or clearly mark this as intentional unit‑level access.

If switching is feasible, confirm whether a lightweight WorkflowBuilder stub exists to register test_functions without full workflow wiring.


137-141: Drop asyncio markers for sync TestClient tests.

These tests are sync; @pytest.mark.asyncio and async def add overhead.

Example:

-    @pytest.mark.asyncio
-    async def test_route_exists_and_structure(...):
+    def test_route_exists_and_structure(...):
         ...

Apply similarly to other tests in this module.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 38985ab and 1a2a5da.

📒 Files selected for processing (2)
  • src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py (3 hunks)
  • tests/nat/front_ends/mcp/test_mcp_debug_routes.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
src/**/*.py

📄 CodeRabbit inference engine (.cursor/rules/general.mdc)

src/**/*.py: All importable Python code must live under src/
All public APIs in src/ require Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise with a period; surround code entities with backticks

Files:

  • src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
src/nat/**/*

📄 CodeRabbit inference engine (.cursor/rules/general.mdc)

Core functionality under src/nat should prioritize backward compatibility when changed

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

📄 CodeRabbit inference engine (.cursor/rules/general.mdc)

**/*.py: Follow PEP 8/20 style; format with yapf (column_limit=120) and use 4-space indentation; end files with a single newline
Run ruff (ruff check --fix) per pyproject.toml; fix warnings unless explicitly ignored; ruff is linter-only
Use snake_case for functions/variables, PascalCase for classes, and UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: preserve stack traces and avoid duplicate logging
When re-raising exceptions, use bare raise and log with logger.error(), not logger.exception()
When catching and not re-raising, log with logger.exception() to capture stack trace
Validate and sanitize all user input; prefer httpx with SSL verification and follow OWASP Top‑10
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile/mprof; cache with functools.lru_cache or external cache; leverage NumPy vectorization when beneficial

**/*.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).

Files:

  • src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
  • tests/nat/front_ends/mcp/test_mcp_debug_routes.py
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}

📄 CodeRabbit inference engine (.cursor/rules/general.mdc)

**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}: Every file must start with the standard SPDX Apache-2.0 header; keep copyright years up‑to‑date
All source files must include the SPDX Apache‑2.0 header; do not bypass CI header checks

Files:

  • src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
  • tests/nat/front_ends/mcp/test_mcp_debug_routes.py
**/*.{py,md}

📄 CodeRabbit inference engine (.cursor/rules/general.mdc)

Never hard‑code version numbers in code or docs; versions are derived by setuptools‑scm

Files:

  • src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
  • tests/nat/front_ends/mcp/test_mcp_debug_routes.py
**/*.{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/mcp/mcp_front_end_plugin_worker.py
  • tests/nat/front_ends/mcp/test_mcp_debug_routes.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 raise statements to maintain the original stack trace,
      and use logger.error() (not logger.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.txt file, words that might appear to be
spelling mistakes but are listed in the ci/vale/styles/config/vocabularies/nat/accept.txt file 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
  • tests/nat/front_ends/mcp/test_mcp_debug_routes.py
tests/**/*.py

📄 CodeRabbit inference engine (.cursor/rules/general.mdc)

Unit tests must live under tests/ and use configured markers (e2e, integration, etc.)

Files:

  • tests/nat/front_ends/mcp/test_mcp_debug_routes.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 the test_ 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 the fixture_ prefix, using snake_case. Example:
@pytest.fixture(name="my_fixture")
def fixture_my_fixture():
pass

Files:

  • tests/nat/front_ends/mcp/test_mcp_debug_routes.py
**/tests/**/*.py

📄 CodeRabbit inference engine (.cursor/rules/general.mdc)

**/tests/**/*.py: Test functions must use the test_ prefix and snake_case
Extract repeated test code into pytest fixtures; fixtures should set name=... in @pytest.fixture and functions named with fixture_ prefix
Mark expensive tests with @pytest.mark.slow or @pytest.mark.integration
Use pytest with pytest-asyncio for async code; mock external services with pytest_httpserver or unittest.mock

Files:

  • tests/nat/front_ends/mcp/test_mcp_debug_routes.py
🧬 Code graph analysis (2)
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py (3)
src/nat/builder/function.py (1)
  • Function (46-301)
src/nat/front_ends/mcp/tool_converter.py (1)
  • get_function_description (199-236)
src/nat/data_models/common.py (1)
  • model_json_schema (135-151)
tests/nat/front_ends/mcp/test_mcp_debug_routes.py (2)
src/nat/data_models/config.py (1)
  • GeneralConfig (188-240)
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py (1)
  • _setup_debug_endpoints (107-200)

Signed-off-by: Daniel Wang <daniewang@nvidia.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py (1)

89-107: Fix return type and variable annotation: mapping contains Workflow, not just Function.

functions includes workflow (a Workflow) at Line 105, so dict[str, Function] is incorrect and will trip pyright. Use FunctionBase in both the return type and local var; update docstring accordingly.

Apply:

-def _get_all_functions(self, workflow: Workflow) -> dict[str, Function]:
-    """Get all functions from the workflow.
+def _get_all_functions(self, workflow: Workflow) -> dict[str, FunctionBase]:
+    """Get all functions and the workflow entry.
@@
-    Returns:
-        Dict mapping function names to Function objects.
+    Returns:
+        Dict mapping tool names to FunctionBase (Function or Workflow) instances.
@@
-    functions: dict[str, Function] = {}
+    functions: dict[str, FunctionBase] = {}
@@
-    functions[workflow.config.workflow.type] = workflow
+    functions[workflow.config.workflow.type] = workflow
🧹 Nitpick comments (5)
src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py (5)

109-116: Docstring nit: hyphenate “comma‑separated”.

Minor clarity tweak.

Apply:

-          - GET /debug/tools/list: List tools. Optional query param `name` (one or more, repeatable or comma separated)
+          - GET /debug/tools/list: List tools. Optional query param `name` (one or more, repeatable or comma‑separated)

127-133: Dedup names while preserving caller order (avoid set reordering).

Using set() first can reorder inputs; build an ordered, deduped list instead.

Apply:

-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_values = request.query_params.getlist("name")
+names: list[str] = []
+seen: set[str] = set()
+for raw in raw_values:
+    for p in raw.split(","):
+        name = p.strip()
+        if name and name not in seen:
+            seen.add(name)
+            names.append(name)

149-175: Make schema detection more robust; add Pydantic v1 fallback.

  • Prefer an issubclass check for ChatRequest when importable; keep name heuristic as fallback.
  • Support Pydantic v1 (schema()) in addition to v2 (model_json_schema()).

Apply:

 def _build_schema_info(fn: FunctionBase) -> dict[str, Any] | None:
     schema = getattr(fn, "input_schema", None)
     if schema is None:
         return None
 
-    # check if schema is a ChatRequest
+    # Try explicit ChatRequest type check; fall back to name heuristic
+    try:
+        from nat.front_ends.mcp.data_models import ChatRequest as _ChatRequest  # type: ignore
+    except Exception:
+        _ChatRequest = None  # type: ignore[assignment]
+
     schema_name = getattr(schema, "__name__", "")
     schema_qualname = getattr(schema, "__qualname__", "")
-    if "ChatRequest" in schema_name or "ChatRequest" in schema_qualname:
+    if (_ChatRequest and isinstance(schema, type) and issubclass(schema, _ChatRequest)) or \
+       ("ChatRequest" in schema_name or "ChatRequest" in schema_qualname):
         # Simplified interface used by MCP wrapper for ChatRequest
         return {
             "type": "object",
             "properties": {
                 "query": {
                     "type": "string", "description": "User query string"
                 }
             },
             "required": ["query"],
             "title": "ChatRequestQuery",
         }
 
-    # Pydantic models provide model_json_schema
-    if schema is not None and hasattr(schema, "model_json_schema"):
-        return schema.model_json_schema()
+    # Pydantic models: v2 (model_json_schema) and v1 (schema)
+    if schema is not None:
+        if hasattr(schema, "model_json_schema"):
+            return schema.model_json_schema()
+        if hasattr(schema, "schema"):
+            return schema.schema()
 
     return None

176-192: Use isinstance for workflow detection and wrap to stay within 120 cols.

More precise and keeps formatting within style limits.

Apply:

-                for name, fn in functions_to_include.items():
-                    list_entry: dict[str, Any] = {
-                        "name": name, "description": get_function_description(fn), "is_workflow": hasattr(fn, "run")
-                    }
+                for name, fn in functions_to_include.items():
+                    list_entry: dict[str, Any] = {
+                        "name": name,
+                        "description": get_function_description(fn),
+                        "is_workflow": isinstance(fn, Workflow),
+                    }

247-248: Gate debug endpoints behind a config flag to avoid accidental exposure.

To reduce info‑disclosure risk in prod, let users disable this route via config. Default to enabled for backward compatibility.

Apply:

-# After registration, expose debug endpoints for tool/schema inspection
-self._setup_debug_endpoints(mcp, functions)
+# After registration, optionally expose debug endpoints for tool/schema inspection
+if getattr(self.front_end_config, "enable_debug_routes", True):
+    self._setup_debug_endpoints(mcp, functions)
+else:
+    logger.info("MCP debug endpoints disabled via front_end_config.enable_debug_routes=False")
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1a2a5da and 0c6fb9a.

📒 Files selected for processing (1)
  • src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py (3 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
src/**/*.py

📄 CodeRabbit inference engine (.cursor/rules/general.mdc)

src/**/*.py: All importable Python code must live under src/
All public APIs in src/ require Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise with a period; surround code entities with backticks

Files:

  • src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
src/nat/**/*

📄 CodeRabbit inference engine (.cursor/rules/general.mdc)

Core functionality under src/nat should prioritize backward compatibility when changed

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

📄 CodeRabbit inference engine (.cursor/rules/general.mdc)

**/*.py: Follow PEP 8/20 style; format with yapf (column_limit=120) and use 4-space indentation; end files with a single newline
Run ruff (ruff check --fix) per pyproject.toml; fix warnings unless explicitly ignored; ruff is linter-only
Use snake_case for functions/variables, PascalCase for classes, and UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: preserve stack traces and avoid duplicate logging
When re-raising exceptions, use bare raise and log with logger.error(), not logger.exception()
When catching and not re-raising, log with logger.exception() to capture stack trace
Validate and sanitize all user input; prefer httpx with SSL verification and follow OWASP Top‑10
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile/mprof; cache with functools.lru_cache or external cache; leverage NumPy vectorization when beneficial

**/*.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).

Files:

  • src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}

📄 CodeRabbit inference engine (.cursor/rules/general.mdc)

**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}: Every file must start with the standard SPDX Apache-2.0 header; keep copyright years up‑to‑date
All source files must include the SPDX Apache‑2.0 header; do not bypass CI header checks

Files:

  • src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
**/*.{py,md}

📄 CodeRabbit inference engine (.cursor/rules/general.mdc)

Never hard‑code version numbers in code or docs; versions are derived by setuptools‑scm

Files:

  • src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
**/*.{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/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 raise statements to maintain the original stack trace,
      and use logger.error() (not logger.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.txt file, words that might appear to be
spelling mistakes but are listed in the ci/vale/styles/config/vocabularies/nat/accept.txt file 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 (2)
src/nat/builder/function.py (1)
  • Function (46-301)
src/nat/front_ends/mcp/tool_converter.py (1)
  • get_function_description (199-236)
⏰ 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/mcp/mcp_front_end_plugin_worker.py (3)

193-201: LGTM: 404 on first missing tool with clear message.

Behavior is sensible for a debug endpoint.


203-205: LGTM: defaulting detail based on presence of name is intuitive.

Nice UX touch; response envelope looks consistent with /health.


19-21: Imports look right for new types and exceptions.

Good use of Mapping/FunctionBase and Starlette’s HTTPException.

Also applies to: 23-23, 27-27

@zhongxuanwang-nv zhongxuanwang-nv changed the title Add a /debug route to NAT MCP fronend to expose MCP tools Add a /debug route to NAT MCP frontend to expose MCP tools Sep 18, 2025
@zhongxuanwang-nv zhongxuanwang-nv merged commit 44ee35b into NVIDIA:develop Sep 18, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature request New feature or request non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants