Skip to content

Conversation

@zhongxuanwang-nv
Copy link
Member

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

Description

Closes AIQ-1885

  • Added:

    • New CLI group and subcommands:
      • nat mcp serve wired to server start logic (src/nat/cli/commands/mcp/mcp.py)
      • nat mcp client with ping, tool list, tool call (src/nat/cli/commands/mcp/mcp.py)
        • nat mcp client ping -> ping the mcp server
        • nat mcp client tool list -> list all tools served by the server
        • nat mcp client tool call -> call a specific tool served by the server
        • nat mcp client tool -> will do nothing more than printing a help message asking for more options!
    • Entry-point registration for mcp (src/nat/cli/entrypoint.py)
    • Test suite for new MCP CLI (tests/nat/cli/commands/mcp/test_mcp_cli.py)
  • Changed:

    • Consolidated/relocated MCP CLI logic into src/nat/cli/commands/mcp/mcp.py
    • CLI reference updated to show nested structure (nat mcp serve, nat mcp client ...) in docs/source/reference/cli.md
  • Removed:

    • Legacy nat info mcp implementation (src/nat/cli/commands/info/list_mcp.py) and its tests (tests/nat/cli/commands/info/test_list_mcp.py)
  • Why it matters:

    • Clearer, future-proof CLI surface for MCP: explicit server (serve) and client operations; improves UX discoverability and keeps parity with existing start behavior.
  • Breaking changes / Migrations:

    • nat mcp (top-level) → nat mcp serve
    • nat info mcp removed; a shim now raises click.UsageError instructing users to use nat mcp client ... (src/nat/cli/commands/info/info.py)
  • Docs:

    • Updated MCP pages and examples: docs/source/reference/cli.md, docs/source/workflows/mcp/mcp-auth.md, docs/source/workflows/mcp/mcp-client.md, docs/source/workflows/mcp/mcp-server.md, examples/MCP/simple_calculator_mcp/README.md to use nat mcp serve and new client commands.

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

    • Adds a dedicated top-level MCP CLI: "nat mcp serve" to start servers and "nat mcp client" with subcommands to list, ping, and call tools.
  • Documentation

    • Updates CLI reference, auth, client, server guides, and example README to use "nat mcp serve" and the new client command names.
  • Chores

    • Adds Apache-2.0 SPDX license header.
  • Refactor

    • Deprecates old "nat info mcp" and replaces previous alias with guidance.
  • Tests

    • Adds comprehensive tests for the new MCP CLI and removes legacy MCP info tests.

Signed-off-by: Daniel Wang <[email protected]>
@zhongxuanwang-nv zhongxuanwang-nv self-assigned this Sep 17, 2025
@zhongxuanwang-nv zhongxuanwang-nv added improvement Improvement to existing functionality breaking Breaking change labels Sep 17, 2025
@coderabbitai
Copy link

coderabbitai bot commented Sep 17, 2025

Walkthrough

Adds a top-level mcp Click command group with a serve subcommand (invoked as nat mcp serve) that delegates to existing start logic, implements nat mcp client (tool list/call and ping), removes/relocates the old nat info mcp implementation behind a deprecated shim, updates docs/examples to use nat mcp serve, adds an SPDX header, and replaces related tests.

Changes

Cohort / File(s) Summary
New MCP CLI module
src/nat/cli/commands/mcp/mcp.py
Add new mcp Click group with serve subcommand (delegates to start_command), new mcp client group with tool subcommands (list, call) and ping, transport validation, MCP error handling, tool formatting/printing, async ping/list/call helpers, and JSON/text output options.
Entrypoint: register mcp
src/nat/cli/entrypoint.py
Import and register mcp_command as top-level nat mcp; remove previous alias that exposed mcp via the start command.
Remove old info/list implementation
src/nat/cli/commands/info/list_mcp.py (deleted)
Remove legacy list_mcp CLI module (tool listing, ping, transport handling); functionality moved/reimplemented in new mcp module.
Info command: deprecated shim
src/nat/cli/commands/info/info.py
Add info_mcp_deprecated command that raises a UsageError directing users to nat mcp client, registered as nat info mcp; remove previous public import of list_mcp.
Docs: update MCP CLI usage
docs/.../reference/cli.md, docs/.../workflows/mcp/mcp-auth.md, docs/.../workflows/mcp/mcp-client.md, docs/.../workflows/mcp/mcp-server.md, examples/MCP/simple_calculator_mcp/README.md
Replace nat mcp examples with nat mcp serve; update usage/help blocks to show nat mcp serve [OPTIONS]; update examples and troubleshooting references to nat mcp client subcommands (tool list/call, ping).
Packaging/header
src/nat/cli/commands/mcp/__init__.py
Add Apache-2.0 SPDX license header; no functional changes.
Tests: remove old, add new
tests/nat/cli/commands/info/test_list_mcp.py (deleted), tests/nat/cli/commands/mcp/test_mcp_cli.py (added)
Delete legacy tests for nat info mcp; add comprehensive unit tests covering nat mcp client listing, pinging, calling, transport cases, formatting, error handling, and direct/group modes.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant CLI as nat CLI
  participant MCPGroup as nat mcp
  participant Serve as "serve"
  participant Start as start_command (MCP)
  participant MCPServer as MCP Server Runtime

  User->>CLI: nat mcp serve [OPTIONS]
  CLI->>MCPGroup: dispatch "mcp"
  MCPGroup->>Serve: invoke "serve"
  Note right of Serve: serve delegates to existing start logic
  Serve->>Start: get_command(None,"mcp") / invoke
  Start->>MCPServer: initialize & run server
  MCPServer-->>User: server ready / responses
Loading
sequenceDiagram
  autonumber
  actor User
  participant CLI as nat CLI
  participant MCPClient as nat mcp client
  participant ClientLib as MCP client wrappers
  participant MCPServer as MCP Server Runtime

  User->>CLI: nat mcp client tool list / ping / call
  CLI->>MCPClient: parse options (url, transport, direct, command, args)
  MCPClient->>ClientLib: list / ping / call (stdio / sse / http)
  ClientLib->>MCPServer: transport-specific interaction
  MCPServer-->>ClientLib: response / tool list / call result
  ClientLib-->>MCPClient: formatted result
  MCPClient-->>User: text or JSON output
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.30% 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 "Add serve and client for nat mcp" concisely and accurately summarizes the main change, which introduces the mcp serve and mcp client CLI groups and related commands; it maps directly to the entrypoint, new mcp module, docs, and tests in the PR. It uses imperative mood, is descriptive, and is within the ~72-character guideline (38 characters). Therefore it satisfies the provided title requirements.
✨ 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

✅ 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/cli/entrypoint.py (1)

1-24: Conflicting dual license headers (Apache-2.0 and NvidiaProprietary). Remove the proprietary block.

Repo policy says Apache-2.0. Keep a single SPDX header.

-# SPDX-FileCopyrightText: Copyright (c) 2024-2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
-# SPDX-License-Identifier: LicenseRef-NvidiaProprietary
-#
-# NVIDIA CORPORATION, its affiliates and licensors retain all intellectual
-# property and proprietary rights in and to this material, related
-# documentation and any modifications thereto. Any use, reproduction,
-# disclosure or distribution of this material and related documentation
-# without an express license agreement from NVIDIA CORPORATION or
-# its affiliates is strictly prohibited.
+# (Removed proprietary header; file licensed under Apache-2.0 above.)
🧹 Nitpick comments (3)
docs/source/workflows/mcp/mcp-auth.md (1)

29-31: CLI switch to “nat mcp serve” looks correct. Add a clear production-safety warning.

This page is an experimental scratch pad and uses a stub TokenVerifier that accepts any token. Add an explicit warning to avoid accidental production use.

Suggested addition near the top:

+:::{warning}
+Experimental content. The sample MCP server uses a stub TokenVerifier that accepts any token. Do NOT use in production.
+:::
src/nat/cli/entrypoint.py (1)

59-64: Optional: add return type hints for functions.

Add return annotations to meet the project’s typing guideline.

-def setup_logging(log_level: str):
+def setup_logging(log_level: str) -> int:
@@
-def get_version():
+def get_version() -> str:

Also applies to: 66-74, 76-85

src/nat/cli/commands/mcp/mcp.py (1)

25-35: Minor polish: set explicit group name, add return type, and drop unused logger.

  • Use name="mcp" for clarity (registration name matches).
  • Add -> None to satisfy typing guideline.
  • Remove unused logger to avoid linter noise (or use it).
-import logging
-
 import click
@@
-from nat.cli.commands.start import start_command
-
-logger = logging.getLogger(__name__)
+from nat.cli.commands.start import start_command
@@
-@click.group(name=__name__, invoke_without_command=False, help="MCP-related commands.")
-def mcp_command():
+@click.group(name="mcp", invoke_without_command=False, help="MCP-related commands.")
+def mcp_command() -> None:
     """
     MCP-related commands.
     """
     return None
📜 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 d88606e and 991ce1c.

📒 Files selected for processing (8)
  • docs/source/reference/cli.md (2 hunks)
  • docs/source/workflows/mcp/mcp-auth.md (1 hunks)
  • docs/source/workflows/mcp/mcp-client.md (1 hunks)
  • docs/source/workflows/mcp/mcp-server.md (2 hunks)
  • examples/MCP/simple_calculator_mcp/README.md (5 hunks)
  • src/nat/cli/commands/mcp/__init__.py (1 hunks)
  • src/nat/cli/commands/mcp/mcp.py (1 hunks)
  • src/nat/cli/entrypoint.py (2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*

⚙️ 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/cli/commands/mcp/__init__.py
  • docs/source/workflows/mcp/mcp-auth.md
  • docs/source/workflows/mcp/mcp-client.md
  • docs/source/workflows/mcp/mcp-server.md
  • examples/MCP/simple_calculator_mcp/README.md
  • src/nat/cli/commands/mcp/mcp.py
  • docs/source/reference/cli.md
  • src/nat/cli/entrypoint.py
src/nat/**/*

⚙️ CodeRabbit configuration file

This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.

Files:

  • src/nat/cli/commands/mcp/__init__.py
  • src/nat/cli/commands/mcp/mcp.py
  • src/nat/cli/entrypoint.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-auth.md
  • docs/source/workflows/mcp/mcp-client.md
  • docs/source/workflows/mcp/mcp-server.md
  • docs/source/reference/cli.md
examples/**/*

⚙️ CodeRabbit configuration file

examples/**/*: - This directory contains example code and usage scenarios for the toolkit, at a minimum an example should
contain a README.md or file README.ipynb.

  • If an example contains Python code, it should be placed in a subdirectory named src/ and should
    contain a pyproject.toml file. Optionally, it might also contain scripts in a scripts/ directory.
  • If an example contains YAML files, they should be placed in a subdirectory named configs/. - If an example contains sample data files, they should be placed in a subdirectory named data/, and should
    be checked into git-lfs.

Files:

  • examples/MCP/simple_calculator_mcp/README.md
🧠 Learnings (1)
📚 Learning: 2025-08-22T20:18:28.041Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/cursor-rules.mdc:0-0
Timestamp: 2025-08-22T20:18:28.041Z
Learning: Applies to .cursor/rules/**/*.mdc : Use consistent project terminology in descriptions (e.g., NAT workflows, NAT CLI commands)

Applied to files:

  • docs/source/workflows/mcp/mcp-server.md
  • docs/source/reference/cli.md
🔇 Additional comments (8)
src/nat/cli/commands/mcp/__init__.py (1)

1-15: SPDX header: LGTM.

examples/MCP/simple_calculator_mcp/README.md (1)

38-39: Docs updated to “nat mcp serve”: looks good.

Nice consistency across examples.

Also applies to: 81-83, 101-102, 110-111, 119-120

docs/source/reference/cli.md (2)

169-170: Example updated to “nat mcp serve”: LGTM.


148-164: Sync CLI docs with implementation: reconcile --tool_names and --transport

  • No click.option matching --tool_names was found in the codebase; confirm the actual flag name and whether it accepts repeated flags (multiple=True) or a CSV, then update the docs to match.
  • @click.option('--transport', type=click.Choice(['sse','stdio','streamable-http']), default='streamable-http') appears in src/nat/cli/commands/info/list_mcp.py (around lines ~343 and ~414). Confirm whether nat mcp serve supports --transport — if yes add it to the docs, if not remove references.
src/nat/cli/entrypoint.py (2)

36-37: Registering the new top-level “mcp” group: LGTM.


115-116: Command wiring complete: LGTM.

Adds mcp without reusing the old alias. Good.

docs/source/workflows/mcp/mcp-client.md (1)

205-206: Align package-name terminology (nvidia-nat-mcp vs nvidia-nat[mcp]) across docs

Invocation nat mcp serve is correct; search returned no matches for nvidia-nat-mcp or nvidia-nat[mcp] in docs — verify repo‑wide and standardize install instructions/docs to a single canonical form (e.g., nvidia-nat[mcp] if pip extras are intended).

Location: docs/source/workflows/mcp/mcp-client.md (lines 205–206).

docs/source/workflows/mcp/mcp-server.md (1)

26-39: No changes required: --transport and repeated --tool_names behave as documented.

start_command dynamically generates CLI options from the MCP front-end config: MCPFrontEndConfig defines transport (Literal['sse','streamable-http']) and tool_names (list[str]); StartCommandGroup._build_params creates --transport and --tool_names, and tool_names is registered with multiple=True so repeated --tool_names flags (not a CSV) are correct. Note: list_mcp (info command) also exposes --transport (includes 'stdio' for client use).

Files of interest:

  • src/nat/front_ends/mcp/mcp_front_end_config.py (defines transport, tool_names)
  • src/nat/cli/commands/start.py (StartCommandGroup._build_params — dynamic CLI option creation)
  • src/nat/cli/commands/mcp/mcp.py (mcp -> serve wiring via start_command.get_command)
  • src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py (uses front_end_config.tool_names to filter)
  • docs/source/workflows/mcp/mcp-server.md (examples match current CLI)

@zhongxuanwang-nv zhongxuanwang-nv marked this pull request as ready for review September 17, 2025 22:28
@zhongxuanwang-nv zhongxuanwang-nv requested a review from a team as a code owner September 17, 2025 22:28
@zhongxuanwang-nv zhongxuanwang-nv added DO NOT MERGE PR should not be merged; see PR for details and removed DO NOT MERGE PR should not be merged; see PR for details labels Sep 17, 2025
Signed-off-by: Zhongxuan (Daniel) Wang <[email protected]>
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

🧹 Nitpick comments (4)
docs/source/workflows/mcp/mcp-server.md (4)

18-22: Fix branding on first mention and casing of “server”.

Follow the style guide: first use “NVIDIA NeMo Agent toolkit”; subsequent uses “NeMo Agent toolkit”. Also use “server” (lowercase).

-# NeMo Agent Toolkit as an MCP Server
+# NVIDIA NeMo Agent toolkit as an MCP server
@@
-This guide will cover how to use NeMo Agent toolkit as an MCP Server to publish tools using MCP.
+This guide will cover how to use NeMo Agent toolkit as an MCP server to publish tools using MCP.

79-87: Don’t expose debug routes publicly.

Add a short note that the debug endpoint is for development and should be firewalled/disabled in production to avoid leaking tool schemas.


153-157: Grammar: “an MCP”.

Use “an MCP” before vowel sound.

-In this example, we will use NeMo Agent toolkit as both a MCP client and a MCP server.
+In this example, we will use NeMo Agent toolkit as both an MCP client and an MCP server.

63-69: Unify MCP install instructions — prefer the extras form nvidia-nat[mcp]

Docs currently say nvidia-nat-mcp but the install command and most docs use nvidia-nat[mcp]. Update the doc and the CLI message to use the extras form (files to change: docs/source/workflows/mcp/mcp-server.md lines 63–69 and src/nat/cli/commands/info/list_mcp.py:162).

-**Note:** The `nat info mcp` command requires the `nvidia-nat-mcp` package. If you encounter an error about missing MCP client functionality, install it with `uv pip install nvidia-nat[mcp]`.
+**Note:** The `nat info mcp` command requires MCP client support. Install it with:
+`uv pip install 'nvidia-nat[mcp]'`
📜 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 991ce1c and 928338d.

📒 Files selected for processing (2)
  • docs/source/workflows/mcp/mcp-client.md (1 hunks)
  • docs/source/workflows/mcp/mcp-server.md (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • docs/source/workflows/mcp/mcp-client.md
🧰 Additional context used
📓 Path-based instructions (5)
docs/source/**/*.md

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

docs/source/**/*.md: Use the official naming: first use “NVIDIA NeMo Agent toolkit”; subsequent uses “NeMo Agent toolkit”; never use deprecated names in documentation
Documentation sources must be Markdown under docs/source; keep docs in sync and fix Sphinx errors/broken links
Documentation must be clear, comprehensive, free of TODO/FIXME/placeholders/offensive/outdated terms; fix spelling; adhere to Vale vocab allow/reject lists

Files:

  • docs/source/workflows/mcp/mcp-server.md
**/*.{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:

  • docs/source/workflows/mcp/mcp-server.md
**/*.{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:

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

⚙️ CodeRabbit configuration file

**/*: # Code Review Instructions

  • Ensure the code follows best practices and coding standards. - For Python code, follow
    PEP 20 and
    PEP 8 for style guidelines.
  • Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
    Example:
    def my_function(param1: int, param2: str) -> bool:
        pass
  • For Python exception handling, ensure proper stack trace preservation:
    • When re-raising exceptions: use bare 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:

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

⚙️ CodeRabbit configuration file

This directory contains the source code for the documentation. All documentation should be written in Markdown format. Any image files should be placed in the docs/source/_static directory.

Files:

  • docs/source/workflows/mcp/mcp-server.md
⏰ 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 (5)
docs/source/workflows/mcp/mcp-server.md (5)

26-34: CLI rename reflected correctly.

Docs now use “nat mcp serve …” and describe default endpoint/transport consistently. Looks good.


36-41: SSE transport example looks correct.

Backward‑compat flag and endpoint path are clear.


42-48: Options example LGTM.

Host/port/name flags match the new CLI structure.


53-59: Tool filtering example LGTM; clarify repetition vs list if supported.

If CLI accepts only repeated flags (as shown), this is fine. If comma‑separated lists are also accepted, consider noting it for parity with the debug route section.


117-121: Verify flags for “nat info mcp”.

The text mentions a --detail flag; confirm the exact flag names and defaults for the CLI and adjust if needed.

@zhongxuanwang-nv zhongxuanwang-nv marked this pull request as draft September 22, 2025 01:48
@zhongxuanwang-nv
Copy link
Member Author

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Sep 22, 2025

@zhongxuanwang-nv I'll review the changes in this PR that introduces the new nat mcp serve CLI structure.


🧠 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: 9

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/nat/cli/commands/info/info.py (1)

26-31: Add type hints to Click group.

Per guidelines, annotate parameters and return type.

-@click.group(name=__name__, invoke_without_command=False, help="Provide information about the local NAT environment.")
-def info_command(**kwargs):
+@click.group(name=__name__, invoke_without_command=False, help="Provide information about the local NAT environment.")
+def info_command(**kwargs: object) -> None:
     """
     Provide information about the local NAT environment.
     """
     pass
docs/source/workflows/mcp/mcp-client.md (1)

28-33: Replace installation command with the standalone package nvidia-nat-mcp.

File: docs/source/workflows/mcp/mcp-client.md (lines 28–33) — docs show nvidia-nat[mcp] but PyPI and runtime messages use nvidia-nat-mcp; update the command.

-uv pip install nvidia-nat[mcp]
+uv pip install nvidia-nat-mcp
🧹 Nitpick comments (9)
src/nat/cli/commands/info/info.py (1)

16-24: Remove unused logger import/variable.

logger is not used in this module.

-import logging
@@
-logger = logging.getLogger(__name__)
src/nat/cli/commands/mcp/mcp.py (4)

54-56: Silence ARG001: mark unused include_traceback.

-    def format_mcp_error(error, include_traceback=False):
-        click.echo(f"Error: {error}", err=True)
+    def format_mcp_error(error: Exception, include_traceback: bool = False) -> None:
+        _ = include_traceback
+        click.echo(f"Error: {error}", err=True)

303-307: Remove unused F821 noqa; ExceptionGroup is built‑in on 3.11+.

-        if isinstance(e, ExceptionGroup):  # noqa: F821
+        if isinstance(e, ExceptionGroup):
             primary_exception = extract_primary_exception(list(e.exceptions))
             mcp_error = convert_to_mcp_error(primary_exception, url)

614-619: Remove unused F821 noqa; ExceptionGroup is built‑in on 3.11+.

-        if isinstance(e, ExceptionGroup):  # noqa: F821
+        if isinstance(e, ExceptionGroup):
             primary_exception = extract_primary_exception(list(e.exceptions))
             mcp_error = convert_to_mcp_error(primary_exception, endpoint)
         else:
             mcp_error = convert_to_mcp_error(e, endpoint)

776-777: Log unexpected exceptions with stack traces (CLI guidance).

When not re‑raising, prefer logger.exception to capture trace.

-    except Exception as e:
-        click.echo(f"[ERROR] {e}", err=True)
+    except Exception as e:
+        logger.exception("Unhandled error in mcp_client_tool_call")
+        click.echo(f"[ERROR] {e}", err=True)
docs/source/workflows/mcp/mcp-client.md (2)

35-39: Grammar nit.

-The library handles the MCP server connection, tool discovery, and function registration. This allow the workflow to use MCP tools as regular functions.
+The library handles the MCP server connection, tool discovery, and function registration. This allows the workflow to use MCP tools as regular functions.

262-267: Troubleshooting: suggest ping explicitly.

You already mention ping; consider adding an explicit command example for clarity.

 - Verify the MCP server is running and accessible via the `nat mcp client ping` command
+ - Verify the MCP server is running and accessible, e.g.:
+   ```bash
+   nat mcp client ping --url http://localhost:9901/mcp
+   ```
tests/nat/cli/commands/mcp/test_mcp_cli.py (2)

464-470: Avoid unused noqa and dead params in test helpers.

Drop unused # noqa: ARG002 and unused args to keep Ruff quiet.

-    def _broken_list_tools(*args, **kwargs):  # noqa: ARG002
+    def _broken_list_tools(*_args, **_kwargs):
         raise RuntimeError("boom")

505-513: Prefer logging over bare try/except/pass in timeout shim.

Minor, but add a comment or log to explain intentional swallow.

-    async def _raise_timeout(coro, timeout=None, **_kwargs):  # noqa: ARG002
+    async def _raise_timeout(coro, timeout=None, **_kwargs):
         del timeout, _kwargs
         # Dispose the passed coroutine to avoid "never awaited" warnings
         try:
             coro.close()
-        except Exception:
-            pass
+        except Exception:  # pragma: no cover
+            # Best-effort cleanup; ignore errors in test shim
+            pass
📜 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 928338d and 6aa230c.

📒 Files selected for processing (9)
  • docs/source/reference/cli.md (3 hunks)
  • docs/source/workflows/mcp/mcp-client.md (4 hunks)
  • docs/source/workflows/mcp/mcp-server.md (6 hunks)
  • src/nat/cli/commands/info/info.py (1 hunks)
  • src/nat/cli/commands/info/list_mcp.py (0 hunks)
  • src/nat/cli/commands/mcp/mcp.py (1 hunks)
  • src/nat/cli/entrypoint.py (2 hunks)
  • tests/nat/cli/commands/info/test_list_mcp.py (0 hunks)
  • tests/nat/cli/commands/mcp/test_mcp_cli.py (1 hunks)
💤 Files with no reviewable changes (2)
  • tests/nat/cli/commands/info/test_list_mcp.py
  • src/nat/cli/commands/info/list_mcp.py
🚧 Files skipped from review as they are similar to previous changes (3)
  • docs/source/workflows/mcp/mcp-server.md
  • src/nat/cli/entrypoint.py
  • docs/source/reference/cli.md
🧰 Additional context used
📓 Path-based instructions (4)
**/*

⚙️ 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/cli/commands/info/info.py
  • tests/nat/cli/commands/mcp/test_mcp_cli.py
  • src/nat/cli/commands/mcp/mcp.py
  • docs/source/workflows/mcp/mcp-client.md
src/nat/**/*

⚙️ CodeRabbit configuration file

This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.

Files:

  • src/nat/cli/commands/info/info.py
  • src/nat/cli/commands/mcp/mcp.py
tests/**/*.py

⚙️ CodeRabbit configuration file

tests/**/*.py: - Ensure that tests are comprehensive, cover edge cases, and validate the functionality of the code. - Test functions should be named using 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/cli/commands/mcp/test_mcp_cli.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-client.md
🧠 Learnings (1)
📚 Learning: 2025-08-22T20:18:28.041Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/cursor-rules.mdc:0-0
Timestamp: 2025-08-22T20:18:28.041Z
Learning: Applies to .cursor/rules/**/*.mdc : Use consistent project terminology in descriptions (e.g., NAT workflows, NAT CLI commands)

Applied to files:

  • docs/source/workflows/mcp/mcp-client.md
🪛 Ruff (0.13.1)
src/nat/cli/commands/info/info.py

46-47: Avoid specifying long messages outside the exception class

(TRY003)

tests/nat/cli/commands/mcp/test_mcp_cli.py

243-243: Unused noqa directive (unused: ARG002)

Remove unused noqa directive

(RUF100)


249-249: Unused noqa directive (unused: ARG002)

Remove unused noqa directive

(RUF100)


271-271: Avoid specifying long messages outside the exception class

(TRY003)


280-280: Unused noqa directive (unused: ARG002)

Remove unused noqa directive

(RUF100)


286-286: Unused noqa directive (unused: ARG002)

Remove unused noqa directive

(RUF100)


439-439: Unused noqa directive (unused: ARG002)

Remove unused noqa directive

(RUF100)


464-464: Unused noqa directive (unused: ARG002)

Remove unused noqa directive

(RUF100)


467-467: Unused function argument: args

(ARG001)


467-467: Unused function argument: kwargs

(ARG001)


467-467: Unused noqa directive (unused: ARG002)

Remove unused noqa directive

(RUF100)


505-505: Unused noqa directive (unused: ARG002)

Remove unused noqa directive

(RUF100)


510-511: try-except-pass detected, consider logging the exception

(S110)


510-510: Do not catch blind exception: Exception

(BLE001)


594-594: Unused noqa directive (unused: BLE001)

Remove unused noqa directive

(RUF100)


618-618: Unused noqa directive (unused: BLE001)

Remove unused noqa directive

(RUF100)


672-672: Unused noqa directive (unused: ARG002)

Remove unused noqa directive

(RUF100)


675-675: Unused noqa directive (unused: ARG002)

Remove unused noqa directive

(RUF100)


684-684: Unused noqa directive (unused: ARG002)

Remove unused noqa directive

(RUF100)


723-723: Unused noqa directive (unused: ARG002)

Remove unused noqa directive

(RUF100)


726-726: Unused noqa directive (unused: ARG002)

Remove unused noqa directive

(RUF100)


759-759: Unused noqa directive (unused: BLE001)

Remove unused noqa directive

(RUF100)

src/nat/cli/commands/mcp/mcp.py

54-54: Unused function argument: include_traceback

(ARG001)


207-207: Do not catch blind exception: Exception

(BLE001)


289-289: Consider moving this statement to an else block

(TRY300)


290-290: Do not catch blind exception: Exception

(BLE001)


303-303: Unused noqa directive (unused: F821)

Remove unused noqa directive

(RUF100)


339-339: Avoid specifying long messages outside the exception class

(TRY003)


368-368: Do not catch blind exception: Exception

(BLE001)


559-559: Abstract raise to an inner function

(TRY301)


559-559: Avoid specifying long messages outside the exception class

(TRY003)


569-569: Abstract raise to an inner function

(TRY301)


569-569: Avoid specifying long messages outside the exception class

(TRY003)


577-577: Abstract raise to an inner function

(TRY301)


577-577: Avoid specifying long messages outside the exception class

(TRY003)


597-597: Abstract raise to an inner function

(TRY301)


614-614: Unused noqa directive (unused: F821)

Remove unused noqa directive

(RUF100)


683-683: Avoid specifying long messages outside the exception class

(TRY003)


776-776: Do not catch blind exception: Exception

(BLE001)

🪛 markdownlint-cli2 (0.18.1)
docs/source/workflows/mcp/mcp-client.md

221-221: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🔇 Additional comments (6)
src/nat/cli/commands/info/info.py (2)

38-51: Good deprecation shim preserving UX, message is clear.

Raising a UsageError that points users to nat mcp client is the right call for this breaking change.


50-51: No other references to nat info mcp; only the deprecated shim remains.
Ripgrep across the repo found matches only in src/nat/cli/commands/info/info.py (deprecated command + guidance at ~lines 39–51); no occurrences in docs or tests.

src/nat/cli/commands/mcp/mcp.py (2)

40-45: Wiring nat mcp serve to existing start command looks correct.

Reusing start_command.get_command(..., "mcp") keeps behavior consistent.


445-446: Potential asyncio.run() in running loop.

If these CLI handlers are ever invoked from a running loop (e.g., notebooks), asyncio.run will fail. Consider an executor that detects/bridges a running loop.

Would you like a small helper (run_async) that uses asyncio.get_running_loop() and nest_asyncio fallback to avoid runtime errors?

docs/source/workflows/mcp/mcp-client.md (1)

176-179: Nice: updated to nat mcp serve.

Example matches the new CLI shape.

tests/nat/cli/commands/mcp/test_mcp_cli.py (1)

221-229: Good: verifies MCPError formatting path.

Solid unit test coverage for the error surface.

@zhongxuanwang-nv zhongxuanwang-nv changed the title Introduce nat mcp serve (replace direct nat mcp) and align CLI structure Introduce serve and 'client' for nat mcp Sep 22, 2025
@zhongxuanwang-nv zhongxuanwang-nv changed the title Introduce serve and 'client' for nat mcp Introduce serve and client for nat mcp Sep 22, 2025
@zhongxuanwang-nv zhongxuanwang-nv changed the title Introduce serve and client for nat mcp Add serve and client for nat mcp Sep 22, 2025
Signed-off-by: Daniel Wang <[email protected]>
@zhongxuanwang-nv zhongxuanwang-nv marked this pull request as ready for review September 22, 2025 21:48
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: 3

🧹 Nitpick comments (4)
tests/nat/cli/commands/mcp/test_mcp_cli.py (2)

505-514: Replace try/except/pass with contextlib.suppress for cleaner timeout test.

Removes S110/BLE001 concerns while keeping behavior.

@@
-    async def _raise_timeout(coro, timeout=None, **_kwargs):
+    async def _raise_timeout(coro, timeout=None, **_kwargs):
         del timeout, _kwargs
-        # Dispose the passed coroutine to avoid "never awaited" warnings
-        try:
-            coro.close()
-        except Exception:
-            pass
+        # Dispose the passed coroutine to avoid "never awaited" warnings
+        from contextlib import suppress
+        with suppress(Exception):
+            coro.close()
         # Simulate asyncio.wait_for timing out
         raise asyncio.TimeoutError

467-469: Silence ARG001 without noqa by naming unused parameters with underscores.

Prevents RUF100/ARG001 and keeps intent obvious.

-def _broken_list_tools(*args, **kwargs):
+def _broken_list_tools(*_args, **_kwargs):
     raise RuntimeError("boom")
src/nat/cli/commands/mcp/mcp.py (2)

54-56: Fix unused parameter to satisfy Ruff ARG001 in fallback formatter.

-def format_mcp_error(error, include_traceback=False):
+def format_mcp_error(error, include_traceback: bool = False):
+    del include_traceback
     click.echo(f"Error: {error}", err=True)

439-441: Remove no-op subcommand check.

This is a command, not a group; the guard never triggers.

-    if ctx.invoked_subcommand is not None:
-        return
+    # No subcommands under this command.
📜 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 6aa230c and 8c2bb7c.

📒 Files selected for processing (4)
  • docs/source/workflows/mcp/mcp-client.md (5 hunks)
  • src/nat/cli/commands/info/info.py (1 hunks)
  • src/nat/cli/commands/mcp/mcp.py (1 hunks)
  • tests/nat/cli/commands/mcp/test_mcp_cli.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • docs/source/workflows/mcp/mcp-client.md
🧰 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/cli/commands/info/info.py
  • src/nat/cli/commands/mcp/mcp.py
src/nat/**/*

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

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

Files:

  • src/nat/cli/commands/info/info.py
  • src/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/cli/commands/info/info.py
  • src/nat/cli/commands/mcp/mcp.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/cli/commands/info/info.py
  • src/nat/cli/commands/mcp/mcp.py
  • tests/nat/cli/commands/mcp/test_mcp_cli.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/cli/commands/info/info.py
  • src/nat/cli/commands/mcp/mcp.py
  • tests/nat/cli/commands/mcp/test_mcp_cli.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/cli/commands/info/info.py
  • src/nat/cli/commands/mcp/mcp.py
  • tests/nat/cli/commands/mcp/test_mcp_cli.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/cli/commands/info/info.py
  • src/nat/cli/commands/mcp/mcp.py
  • tests/nat/cli/commands/mcp/test_mcp_cli.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/cli/commands/info/info.py
  • src/nat/cli/commands/mcp/mcp.py
  • tests/nat/cli/commands/mcp/test_mcp_cli.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/cli/commands/mcp/test_mcp_cli.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/cli/commands/mcp/test_mcp_cli.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/cli/commands/mcp/test_mcp_cli.py
🧠 Learnings (1)
📚 Learning: 2025-08-28T23:22:41.742Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-08-28T23:22:41.742Z
Learning: Applies to **/tests/**/*.py : Extract repeated test code into pytest fixtures; fixtures should set name=... in pytest.fixture and functions named with fixture_ prefix

Applied to files:

  • tests/nat/cli/commands/mcp/test_mcp_cli.py
🧬 Code graph analysis (2)
src/nat/cli/commands/mcp/mcp.py (5)
src/nat/cli/commands/start.py (2)
  • start_command (255-257)
  • get_command (242-244)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/exceptions.py (1)
  • MCPError (30-43)
tests/nat/cli/commands/mcp/test_mcp_cli.py (10)
  • schema_json (233-235)
  • add_function_group (675-676)
  • add_function_group (726-727)
  • get_accessible_functions (662-663)
  • get_accessible_functions (713-714)
  • initialize (252-253)
  • list_tools (255-263)
  • send_ping (269-271)
  • call_tool (265-267)
  • acall_invoke (656-658)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py (1)
  • MCPServerConfig (42-85)
src/nat/data_models/api_server.py (1)
  • TextContent (92-96)
tests/nat/cli/commands/mcp/test_mcp_cli.py (1)
src/nat/cli/commands/mcp/mcp.py (10)
  • MCPPingResult (83-95)
  • call_tool_and_print (631-696)
  • call_tool_direct (525-628)
  • format_tool (98-135)
  • list_tools_direct (234-319)
  • mcp_client_tool_call (716-786)
  • mcp_client_tool_list (416-466)
  • ping_mcp_server (322-378)
  • print_tool (138-157)
  • validate_transport_cli_args (58-80)
🪛 Ruff (0.13.1)
src/nat/cli/commands/info/info.py

42-43: Avoid specifying long messages outside the exception class

(TRY003)

src/nat/cli/commands/mcp/mcp.py

54-54: Unused function argument: include_traceback

(ARG001)


216-216: Do not catch blind exception: Exception

(BLE001)


298-298: Consider moving this statement to an else block

(TRY300)


299-299: Do not catch blind exception: Exception

(BLE001)


348-348: Avoid specifying long messages outside the exception class

(TRY003)


377-377: Do not catch blind exception: Exception

(BLE001)


568-568: Abstract raise to an inner function

(TRY301)


568-568: Avoid specifying long messages outside the exception class

(TRY003)


578-578: Abstract raise to an inner function

(TRY301)


578-578: Avoid specifying long messages outside the exception class

(TRY003)


586-586: Abstract raise to an inner function

(TRY301)


586-586: Avoid specifying long messages outside the exception class

(TRY003)


606-606: Abstract raise to an inner function

(TRY301)


692-692: Avoid specifying long messages outside the exception class

(TRY003)


785-785: Do not catch blind exception: Exception

(BLE001)

tests/nat/cli/commands/mcp/test_mcp_cli.py

243-243: Unused noqa directive (unused: ARG002)

Remove unused noqa directive

(RUF100)


249-249: Unused noqa directive (unused: ARG002)

Remove unused noqa directive

(RUF100)


271-271: Avoid specifying long messages outside the exception class

(TRY003)


280-280: Unused noqa directive (unused: ARG002)

Remove unused noqa directive

(RUF100)


286-286: Unused noqa directive (unused: ARG002)

Remove unused noqa directive

(RUF100)


439-439: Unused noqa directive (unused: ARG002)

Remove unused noqa directive

(RUF100)


464-464: Unused noqa directive (unused: ARG002)

Remove unused noqa directive

(RUF100)


467-467: Unused function argument: args

(ARG001)


467-467: Unused function argument: kwargs

(ARG001)


510-511: try-except-pass detected, consider logging the exception

(S110)


510-510: Do not catch blind exception: Exception

(BLE001)


594-594: Unused noqa directive (unused: BLE001)

Remove unused noqa directive

(RUF100)


618-618: Unused noqa directive (unused: BLE001)

Remove unused noqa directive

(RUF100)


672-672: Unused noqa directive (unused: ARG002)

Remove unused noqa directive

(RUF100)


675-675: Unused noqa directive (unused: ARG002)

Remove unused noqa directive

(RUF100)


684-684: Unused noqa directive (unused: ARG002)

Remove unused noqa directive

(RUF100)


723-723: Unused noqa directive (unused: ARG002)

Remove unused noqa directive

(RUF100)


726-726: Unused noqa directive (unused: ARG002)

Remove unused noqa directive

(RUF100)


759-759: Unused noqa directive (unused: BLE001)

Remove unused noqa directive

(RUF100)

🔇 Additional comments (6)
src/nat/cli/commands/info/info.py (1)

34-44: Deduplicate message and add return type to satisfy Ruff TRY003 and typing policy.

Move the long message to a constant and annotate the function. This also aligns with earlier feedback.

@@
 
 @click.command(
     name="mcp",
     help="Removed. Use 'nat mcp client' instead.",
 )
-def info_mcp_deprecated():
+DEPRECATED_INFO_MCP_MSG = (
+    "The 'nat info mcp' command has been removed. "
+    "Use the new 'nat mcp client' commands instead"
+)
+def info_mcp_deprecated() -> None:
@@
-    raise click.UsageError("The 'nat info mcp' command has been removed. "
-                           "Use the new 'nat mcp client' commands instead")
+    raise click.UsageError(DEPRECATED_INFO_MCP_MSG)
src/nat/cli/commands/mcp/mcp.py (5)

32-37: Add return type to Click group.

Matches project typing standards and prior feedback.

-@click.group(name=__name__, invoke_without_command=False, help="MCP-related commands.")
-def mcp_command():
+@click.group(name=__name__, invoke_without_command=False, help="MCP-related commands.")
+def mcp_command() -> None:
@@
-    return None
+    return None

381-387: Annotate nested Click groups with return types.

Aligns with public-API typing policy and prior bot suggestion.

-@mcp_command.group(name="client", invoke_without_command=False, help="MCP client commands.")
-def mcp_client_command():
+@mcp_command.group(name="client", invoke_without_command=False, help="MCP client commands.")
+def mcp_client_command() -> None:
@@
-@mcp_client_command.group(name="tool", invoke_without_command=False, help="Inspect and call MCP tools.")
-def mcp_client_tool_group():
+@mcp_client_command.group(name="tool", invoke_without_command=False, help="Inspect and call MCP tools.")
+def mcp_client_tool_group() -> None:
@@
-    return None
+    return None

Also applies to: 389-395


234-235: Type‑annotate direct listing API.

Meets src/ typing requirements and improves IDE help.

-async def list_tools_direct(command, url, tool_name=None, transport='sse', args=None, env=None):
+async def list_tools_direct(
+    command: str | None,
+    url: str | None,
+    tool_name: str | None = None,
+    transport: str = 'sse',
+    args: list[str] | None = None,
+    env: dict[str, str] | None = None,
+) -> list[dict[str, str | None]]:

397-467: Type‑annotate CLI handler signature.

Public CLI in src/ must be fully typed.

-@click.pass_context
-def mcp_client_tool_list(ctx, direct, url, transport, command, args, env, tool, detail, json_output):
+@click.pass_context
+def mcp_client_tool_list(
+    ctx: click.Context,
+    direct: bool,
+    url: str,
+    transport: str,
+    command: str | None,
+    args: str | None,
+    env: str | None,
+    tool: str | None,
+    detail: bool,
+    json_output: bool,
+) -> None:

40-41: Confirm start_command.get_command(None, "mcp") returns a click.Command (not None).

  • No registration for "mcp" found in the repo; calling add_command with None will raise at runtime. Either register the "mcp" subcommand under start_command or check for None and surface a clear error before calling add_command. Location: src/nat/cli/commands/mcp/mcp.py:41.

@zhongxuanwang-nv
Copy link
Member Author

The "Squash and merge" button is too tempting!

@zhongxuanwang-nv zhongxuanwang-nv merged commit 11d05dc into NVIDIA:develop Sep 23, 2025
17 checks passed
yczhang-nv pushed a commit to yczhang-nv/NeMo-Agent-Toolkit that referenced this pull request Sep 24, 2025
## Description
<!-- Note: The pull request title will be included in the CHANGELOG. -->
<!-- Provide a standalone description of changes in this PR. -->
<!-- Reference any issues closed by this PR with "closes NVIDIA#1234". All PRs
should have an issue they close-->
Closes AIQ-1885
- **Added**:
  - New CLI group and subcommands:
- `nat mcp serve` wired to server start logic
(`src/nat/cli/commands/mcp/mcp.py`)
- `nat mcp client` with `ping`, `tool list`, `tool call`
(`src/nat/cli/commands/mcp/mcp.py`)
        - `nat mcp client ping` -> ping the mcp server
- `nat mcp client tool list` -> list all tools served by the server
- `nat mcp client tool call` -> call a specific tool served by the
server
- `nat mcp client tool` -> will do nothing more than printing a help
message asking for more options!
  - Entry-point registration for `mcp` (`src/nat/cli/entrypoint.py`)
- Test suite for new MCP CLI
(`tests/nat/cli/commands/mcp/test_mcp_cli.py`)

- **Changed**:
- Consolidated/relocated MCP CLI logic into
`src/nat/cli/commands/mcp/mcp.py`
- CLI reference updated to show nested structure (`nat mcp serve`, `nat
mcp client ...`) in `docs/source/reference/cli.md`

- **Removed**:
- Legacy `nat info mcp` implementation
(`src/nat/cli/commands/info/list_mcp.py`) and its tests
(`tests/nat/cli/commands/info/test_list_mcp.py`)

- **Why it matters**:
- Clearer, future-proof CLI surface for MCP: explicit server (`serve`)
and client operations; improves UX discoverability and keeps parity with
existing start behavior.

- **Breaking changes / Migrations**:
  - `nat mcp` (top-level) → `nat mcp serve`
- `nat info mcp` removed; a shim now raises `click.UsageError`
instructing users to use `nat mcp client ...`
(`src/nat/cli/commands/info/info.py`)

- **Docs**:
- Updated MCP pages and examples: `docs/source/reference/cli.md`,
`docs/source/workflows/mcp/mcp-auth.md`,
`docs/source/workflows/mcp/mcp-client.md`,
`docs/source/workflows/mcp/mcp-server.md`,
`examples/MCP/simple_calculator_mcp/README.md` to use `nat mcp serve`
and new client commands.

## By Submitting this PR I confirm:
- I am familiar with the [Contributing
Guidelines](https://github.com/NVIDIA/NeMo-Agent-Toolkit/blob/develop/docs/source/resources/contributing.md).
- 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.


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* Adds a dedicated top-level MCP CLI: "nat mcp serve" to start servers
and "nat mcp client" with subcommands to list, ping, and call tools.

* **Documentation**
* Updates CLI reference, auth, client, server guides, and example README
to use "nat mcp serve" and the new client command names.

* **Chores**
  * Adds Apache-2.0 SPDX license header.

* **Refactor**
* Deprecates old "nat info mcp" and replaces previous alias with
guidance.

* **Tests**
* Adds comprehensive tests for the new MCP CLI and removes legacy MCP
info tests.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Daniel Wang <[email protected]>
Signed-off-by: Zhongxuan (Daniel) Wang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Breaking change improvement Improvement to existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants