Enable MCP auth for NAT MCP clients #752
Conversation
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
AnuradhaKaruppiah
added
skip-ci
WalkthroughAdds OAuth2-based authentication across MCP: new auth provider with discovery and optional dynamic client registration; HTTP auth adapter and client plumbing; MCP front-end/server token verification support; Pydantic model generator moved to utils; new configs, examples, Dockerfile, docs, and comprehensive tests; MCP dependency bumped to ~1.14. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Agent as NAT Agent
participant Host as MCP Host
participant Client as MCP Client (streamable-http)
participant Provider as MCPOAuth2Provider
participant AS as Authorization Server
participant RS as MCP Server (Protected)
Agent->>Host: Run workflow
Host->>Client: Request tool
Client->>Provider: authenticate(AuthRequest: NORMAL)
alt Endpoints unknown or 401
Provider->>RS: Probe protected resource
RS-->>Provider: 401 + WWW-Authenticate (hint)
Provider->>AS: OIDC/.well-known discovery -> get auth/token[/register]
end
opt Dynamic registration
Provider->>AS: POST /register (client metadata)
AS-->>Provider: client_id[, client_secret]
end
Provider->>AS: Perform auth-code flow (user-agent redirect / code exchange)
AS-->>Provider: Access token
Provider-->>Client: Bearer token
Client->>RS: Tool call with Authorization: Bearer
RS-->>Client: Tool result
Client-->>Host: Result
Host-->>Agent: Result
sequenceDiagram
autonumber
participant Client as MCP Client
participant RS as MCP Server (Front End)
participant Verifier as TokenVerifier (Stub)
participant AS as Authorization Server
Client->>RS: HTTP request with Authorization: Bearer <token>
RS->>Verifier: Verify(<token>, resource, scopes)
Verifier->>AS: (stubbed) validate token
Verifier-->>RS: AccessToken(subject, scopes, resource)
RS-->>Client: 200 OK (tool response)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Requires jumping thru many hoops Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
AnuradhaKaruppiah
changed the title
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Prep for creating the provider Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
coderabbitai
bot
added
breaking
AnuradhaKaruppiah
removed
breaking
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/source/workflows/mcp/mcp-auth.md (1)
18-51: Replace scratchpad copy with production‑ready doc (no placeholders).Ship a stable page; avoid “Temporary/scratch pad” phrasing. Suggested minimal rewrite:
Apply:
-# 🧪 Temporary MCP Auth Planning Notes (Experimental) -This is currently a scratch pad for planning MCP auth. It will be rewritten when all the components are ready. +# Authentication with MCP +The NVIDIA NeMo Agent toolkit supports authenticating MCP clients to protected MCP servers using OAuth 2.0. @@ -## Phases of MCP auth implementation -1. [Completed] MCP client with new `mcp_oauth2` auth provider. -2. [Pending] MCP protected server with `TokenVerifier`. -3. [Pending] Changes for end-to-end MCP auth testing. +## Status overview +- Current: MCP client with `mcp_oauth2` auth provider (discovery + optional dynamic client registration + OAuth2 code flow). +- Upcoming: Protected MCP server with TokenVerifier; end‑to‑end auth tests. @@ -## Steps for testing MCP auth +## Quickstart: test MCP auth locally @@ -This starts a protected MCP server on port 9901. This MCP server has a stub token verifier that will always return success without AS introspection. +This starts a protected MCP server on port 9901. The token verifier is a stub for local testing (no AS introspection). @@ -3. Start NAT UI and enable websocket +3. Start NAT UI and enable WebSocket @@ -This starts the workflow with a MCP client that uses the `mcp_oauth2` auth provider. This provider: +This starts a workflow with an MCP client that uses the `mcp_oauth2` auth provider. The provider:
♻️ Duplicate comments (2)
src/nat/data_models/authentication.py (1)
232-250: Re‑introducing AuthRequest conflicts with prior decision; justify or revert.Earlier thread agreed to drop
AuthRequest; adding it back broadens the interface surface and couples callers to provider internals.Suggested direction:
- Keep provider.authenticate(...) simple; pass
user_id: str | Noneandwww_authenticate: str | Noneas optional params instead of a model, or- Make
AuthRequestinternal (not part of public data_models) and gate usage behind the specific provider.#!/bin/bash # Audit who consumes AuthRequest and where authenticate(...) gained the new parameter. rg -nP --type=py -C2 '\bauthenticate\s*\(' rg -nP --type=py '\bAuthRequest\b' -C2examples/MCP/simple_calculator_mcp/configs/config-mcp-auth-static.yml (1)
19-25: Name looks good; acknowledges prior feedback.Using
mcp_protected_mathaligns with the earlier rename suggestion.
🧹 Nitpick comments (37)
examples/MCP/simple_calculator_mcp/deploy_example_as/Dockerfile (7)
20-23: Avoid apt-get upgrade in build; keep image minimal and reproducible.
Dropping upgrade and pinning clone depth reduces surface and size.Apply:
- RUN apt-get update && apt-get upgrade -y \ - && apt-get install -y --no-install-recommends \ - python3 python3-pip git ca-certificates \ - && rm -rf /var/lib/apt/lists/* + RUN apt-get update \ + && apt-get install -y --no-install-recommends python3 python3-pip git ca-certificates \ + && rm -rf /var/lib/apt/lists/*
26-28: Consider avoiding update-alternatives unless required.
Most base images already provide python3/pip3; explicit names keep builds clearer.
31-31: Pin uv or use the official installer; prefer python -m pip.
Unpinned toolchain can drift; using python -m pip is safer.Apply:
- RUN pip install --no-cache-dir uv + RUN python -m pip install --no-cache-dir 'uv==<PINNED_VERSION>'Alternatively (official installer):
+ RUN curl -LsSf https://astral.sh/uv/install.sh | sh -s -- -y + ENV PATH="/root/.local/bin:${PATH}"Would you like me to check and suggest the exact uv version you’re standardizing on?
35-37: Clone with depth and branch to reduce size; no separate checkout needed.Apply:
- RUN git clone https://github.com/modelcontextprotocol/python-sdk.git mcp-sdk - WORKDIR /opt/mcp-sdk - RUN git checkout v1.14.0 + RUN git clone --depth=1 --branch v1.14.0 https://github.com/modelcontextprotocol/python-sdk.git mcp-sdk + WORKDIR /opt/mcp-sdk
40-41: Avoid editable install in a container; drop source and git afterward.
Editable installs bind runtime to source path and bloat the image.Apply:
- RUN uv pip install --system -e . + RUN uv pip install --system . + WORKDIR / + RUN rm -rf /opt/mcp-sdk && apt-get purge -y git && apt-get autoremove -y && rm -rf /var/lib/apt/lists/*
44-47: Run as non-root.
Better default security posture for examples too.Apply:
EXPOSE 9000 + # Drop privileges + RUN useradd -r -u 10001 appuser + USER 10001
46-47: Optional: add a HEALTHCHECK if the server exposes one.
Improves operability in demos.Does mcp-simple-auth-as expose a health endpoint (e.g., /health or /live)? If yes, I can add a precise HEALTHCHECK line.
examples/MCP/simple_calculator_mcp/configs/config-mcp-math.yml (1)
50-72: Provide a nat_test_llm option for CI/docs parity.Per guidelines, include a stub LLM to enable offline/testing runs while keeping NIM/OpenAI as defaults.
Apply:
llms: nim_llm: _type: nim model_name: meta/llama-3.1-70b-instruct temperature: 0.0 max_tokens: 1024 openai_llm: _type: openai model_name: gpt-3.5-turbo max_tokens: 2000 + test_llm: + _type: nat_test_llm + response_seq: ["42"] + delay_ms: 0You can switch via
--override workflow.llm_name=test_llmwhen needed.docs/source/workflows/mcp/mcp-server.md (2)
18-24: Fix official product naming.First mention must be “NVIDIA NeMo Agent toolkit”; subsequent mentions “NeMo Agent toolkit”.
Apply:
-# 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. For more information on how to use NeMo Agent toolkit as an MCP Host with one or more MCP Clients, refer to the [MCP Client](./mcp-client.md) documentation. +This guide will cover how to use the NeMo Agent toolkit as an MCP Server to publish tools using MCP. For more information on how to use the NeMo Agent toolkit as an MCP Host with one or more MCP Clients, refer to the [MCP Client](./mcp-client.md) documentation.
61-69: Optional: add test LLM note for examples.Consider a short note showing how to run with
nat_test_llmto avoid external LLM calls in CI.examples/MCP/simple_calculator_mcp/configs/config-mcp-date-stdio.yml (1)
50-76: Add nat_test_llm for testability.Include a stub LLM alongside real ones to meet the YAML guideline and ease testing.
Apply:
llms: nim_llm: _type: nim @@ openai_llm: _type: openai model_name: gpt-3.5-turbo max_tokens: 2000 + test_llm: + _type: nat_test_llm + response_seq: ["It is 10:00."] + delay_ms: 0docs/source/workflows/mcp/mcp-client.md (1)
18-25: Fix official naming (first vs. subsequent mentions).Use “NVIDIA NeMo Agent toolkit” on first mention; “NeMo Agent toolkit” thereafter.
Apply:
-# NeMo Agent Toolkit as an MCP Client +# NVIDIA NeMo Agent toolkit as an MCP Client @@ -You can use NeMo Agent toolkit as an MCP Host with one or more MCP Clients serving tools from remote MCP servers. +You can use the NeMo Agent toolkit as an MCP Host with one or more MCP Clients serving tools from remote MCP servers. @@ -This guide will cover how to use NeMo Agent toolkit as an MCP Host with one or more MCP Clients. For more information on how to use NeMo Agent toolkit as an MCP Server, please refer to the [MCP Server](./mcp-server.md) documentation. +This guide will cover how to use the NeMo Agent toolkit as an MCP Host with one or more MCP Clients. For more information on how to use the NeMo Agent toolkit as an MCP Server, please refer to the [MCP Server](./mcp-server.md) documentation.packages/nvidia_nat_mcp/src/nat/plugins/mcp/utils.py (4)
42-52: Harden generated names (classes/enums) to valid identifiers.Sanitize to avoid invalid or duplicate identifiers and Python keywords.
Apply:
- def _generate_valid_classname(class_name: str): - return class_name.replace('_', ' ').replace('-', ' ').title().replace(' ', '') + def _generate_valid_classname(class_name: str) -> str: + import keyword, re + base = re.sub(r'[^0-9a-zA-Z_]+', '_', class_name).strip('_') + if not base or base[0].isdigit(): + base = f"X{base}" + base = ''.join(part.capitalize() for part in base.split('_')) + if keyword.iskeyword(base): + base += "Model" + return base @@ - enum_name = f"{field_name.capitalize()}Enum" - field_type = Enum(enum_name, {item: item for item in enum_vals}) + import re + enum_name = f"{_generate_valid_classname(field_name)}Enum" + members = {} + for i, item in enumerate(enum_vals): + key = re.sub(r'[^0-9a-zA-Z_]+', '_', str(item)).upper().strip('_') + if not key or key[0].isdigit(): + key = f"VALUE_{i}" + while key in members: + key += "_X" + members[key] = item + field_type = Enum(enum_name, members)
62-71: Unify multi-type handling with required/nullable logic.Current early return skips required/nullable processing.
Apply:
- elif isinstance(json_type, list): - field_type = None - for t in json_type: - mapped = _type_map.get(t, Any) - field_type = mapped if field_type is None else field_type | mapped - - return field_type, Field( - default=field_properties.get("default", None if "null" in json_type else ...), - description=field_properties.get("description", "") - ) + elif isinstance(json_type, list): + field_type = Any + for t in json_type: + mapped = _type_map.get(t, Any) + field_type = field_type | mapped if field_type is not Any else mappedThen let the common required/nullable block compute defaults and optionals.
19-22: Forbid extras on generated models.Create a base with
extra="forbid"and use it increate_model.Apply:
-from pydantic import BaseModel +from pydantic import BaseModel, ConfigDict @@ - return create_model(f"{_generate_valid_classname(name)}InputSchema", **schema_dict) + class _MCPBaseModel(BaseModel): + model_config = ConfigDict(extra="forbid") + return create_model(f"{_generate_valid_classname(name)}InputSchema", __base__=_MCPBaseModel, **schema_dict)Also applies to: 95-95
45-91: Minor typing/doc polish.
- Type
mcp_input_schema: dict[str, Any].- Expand docstring with Args/Returns per Google‑style.
tests/nat/mcp/test_mcp_schema.py (1)
24-26: Name fixtures with fixture_ prefix per repo convention.Minor consistency nit; rename to aid discovery and satisfy guidelines.
Apply:
-@pytest.fixture(name="sample_schema") -def _get_sample_schema(): +@pytest.fixture(name="sample_schema") +def fixture_sample_schema():examples/MCP/simple_calculator_mcp/configs/config-mcp-auth-dynamic.yml (1)
26-31: Fix misleading comment.The list excludes calculator tools; update the comment accordingly.
Apply:
- # skip react_agent and datetime tools from the math MCP server + # exclude math tools served by the MCP server (we expose only what we need)examples/MCP/simple_calculator_mcp/configs/config-mcp-server-auth.yml (1)
37-47: Provide an offline‑friendly LLM option.Similar to the client config, consider a local variant using
nat_test_llmto avoid external dependencies during docs/CI runs.I can add
config-mcp-server-auth.local.ymlwithllms.test_llm: { _type: nat_test_llm, response_seq: ["ok"] }. Proceed?src/nat/front_ends/mcp/mcp_front_end_plugin.py (2)
84-87: Deduplicate URL construction.Use
_get_server_url()wherever the server URL is needed to avoid drift.
93-105: Validate auth config and trim exception text (TRY003).
- Keep messages concise to satisfy Ruff TRY003.
- Optionally warn when
scopesis empty.- if not self.front_end_config.auth_server_url: - raise ValueError("auth_server_url is required when require_auth is True") + if not self.front_end_config.auth_server_url: + raise ValueError("auth_server_url required when require_auth=True") + if not self.front_end_config.scopes: + logger.warning("Auth enabled but no scopes configured.") auth_settings = AuthSettings(issuer_url=AnyHttpUrl(self.front_end_config.auth_server_url), required_scopes=self.front_end_config.scopes, resource_server_url=self._get_server_url())examples/MCP/simple_calculator_mcp/configs/config-mcp-auth-static.yml (1)
47-56: Stub LLMs in configs for CI/runbooks per repo guidelines.Set
_type: nat_test_llm(withresponse_seq/delay_ms) to avoid external calls; keep real LLMs as commented examples elsewhere.-llms: - nim_llm: - _type: nim - model_name: meta/llama-3.1-70b-instruct - temperature: 0.0 - max_tokens: 1024 - openai_llm: - _type: openai - model_name: gpt-3.5-turbo - max_tokens: 2000 +llms: + nim_llm: + _type: nat_test_llm + response_seq: ["ok"] + delay_ms: 0src/nat/authentication/oauth2/oauth2_auth_code_flow_provider.py (4)
66-89: User ID resolution helper is clear; small polish.Good precedence order. Consider documenting it in the class docstring and unit‑testing each branch.
90-91: Document newauthenticate(..., auth_request=...)contract.Add a short docstring describing precedence and behavior when
user_idis unresolved (no cache save).
106-110: Don’t wrap and re‑raise; preserve the original stack per project guidance.Either let exceptions bubble or log with
logger.error(..., exc_info=True)andraise.- try: - authenticated_context = await auth_callback(self.config, AuthFlowType.OAUTH2_AUTHORIZATION_CODE) - except Exception as e: - raise RuntimeError(f"Authentication callback failed: {e}") from e + authenticated_context = await auth_callback(self.config, AuthFlowType.OAUTH2_AUTHORIZATION_CODE)Optional logging (outside hot path):
# before the call logger.debug("Starting OAuth2 auth flow via context callback")
93-101: Token cache writes are unsynchronized (minor).Concurrent requests can race updating
_authenticated_tokens. Guard writes with a lock.Additional changes outside the selected lines:
# at top-level imports import asyncio # in __init__ self._tokens_lock = asyncio.Lock() # where writing: async with self._tokens_lock: self._authenticated_tokens[user_id] = new_auth_result # and: async with self._tokens_lock: self._authenticated_tokens[user_id] = auth_resultAlso applies to: 123-125
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py (4)
58-60: Auth provider field: good placement; clarify docstring.Explicitly note it applies only to
streamable-http.- auth_provider: AuthenticationRef | None = Field(default=None, description="Reference to authentication provider") + auth_provider: AuthenticationRef | None = Field( + default=None, + description="Reference to authentication provider (streamable-http only)" + )
69-85: Validation rules look right; shorten error messages (TRY003).Keep concise messages and consistent punctuation.
- raise ValueError("Authentication is not supported for stdio transport") + raise ValueError("Auth not supported for stdio") ... - raise ValueError("command, args, and env should not be set when using sse transport") + raise ValueError("Do not set command/args/env with sse") ... - raise ValueError("Authentication is not supported for SSE transport.") + raise ValueError("Auth not supported for sse") ... - raise ValueError("command, args, and env should not be set when using streamable-http transport") + raise ValueError("Do not set command/args/env with streamable-http")
161-165: Provider resolution is correct; consider early guard.If
auth_provideris set with non‑HTTP transports, it will already be rejected by validation. Optional: assert here defensively.
166-176: Client construction branches LGTM; tighten error strings (TRY003).Minor message trim and ensure
urlis str‑casted once.- if not config.server.command: - raise ValueError("command is required for stdio transport") + if not config.server.command: + raise ValueError("command required for stdio")packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.py (2)
46-51: Constraintoken_endpoint_auth_methodto valid values.Use
Literalto avoid invalid inputs.-from pydantic import Field +from typing import Literal +from pydantic import Field ... - token_endpoint_auth_method: str = Field(default="client_secret_post", - description="The authentication method for the token endpoint.") + token_endpoint_auth_method: Literal["client_secret_post", "client_secret_basic", "private_key_jwt", "none"] = Field( + default="client_secret_post", + description="Auth method for the token endpoint.")
69-71: Shorten validation error (TRY003).Concise message satisfies linter.
- raise ValueError("Must provide either: " - "1) enable_dynamic_registration=True (dynamic), or " - "2) client_id + client_secret (hybrid)") + raise ValueError("Invalid config: enable_dynamic_registration=True or provide client_id+client_secret")tests/nat/mcp/test_mcp_auth_provider.py (2)
56-58: Silence hardcoded-secret warnings in tests.These are harmless test fixtures. Add
# noqa: S106to keep Ruff quiet.Apply this diff:
@@ def mock_config_with_credentials() -> MCPOAuth2ProviderConfig: - client_secret="test_client_secret", + client_secret="test_client_secret", # noqa: S106 (tests) @@ def mock_credentials() -> OAuth2Credentials: - client_secret="test_client_secret", + client_secret="test_client_secret", # noqa: S106 (tests) @@ - credentials=[BearerTokenCred(token="test_token")], + credentials=[BearerTokenCred(token="test_token")], # noqa: S106 (tests) @@ - credentials=[BearerTokenCred(token="test_token")], + credentials=[BearerTokenCred(token="test_token")], # noqa: S106 (tests) @@ def test_authenticate_dynamic_registration_disabled(self, mock_config, mock_endpoints, monkeypatch): - client_secret="test_client_secret", + client_secret="test_client_secret", # noqa: S106 (tests)Also applies to: 79-79, 346-346, 383-383, 409-409
336-336: Remove or underscore unused parameters to satisfy Ruff ARG00x.Clean up unused test arguments.
Apply this diff:
@@ - async def test_authenticate_with_manual_credentials(self, mock_config_with_credentials, mock_endpoints, - monkeypatch): + async def test_authenticate_with_manual_credentials(self, mock_config_with_credentials, mock_endpoints): @@ - async def test_authenticate_with_dynamic_registration(self, - mock_config, - mock_endpoints, - mock_credentials, - monkeypatch): + async def test_authenticate_with_dynamic_registration(self, mock_config, mock_endpoints, mock_credentials): @@ - async def test_authenticate_dynamic_registration_disabled(self, mock_config, mock_endpoints, monkeypatch): + async def test_authenticate_dynamic_registration_disabled(self, mock_endpoints): @@ - async def test_state_lock_prevents_concurrent_discovery(self, mock_config, mock_endpoints, mock_credentials): + async def test_state_lock_prevents_concurrent_discovery(self, mock_config, mock_credentials): @@ - async def discover_with_delay(*args, **kwargs): + async def discover_with_delay(*_args, **_kwargs):Also applies to: 369-370, 403-403, 491-491, 503-503
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py (1)
349-353: Auth scoping knob exists but is never enabled.You added
auth_for_tool_calls_onlyto support unauthenticated discovery, but it’s never set. Consider plumbing a flag throughMCPStreamableHTTPClient(...)or defaulting to tool-only auth if desired.packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.py (2)
126-140: Narrow exception handling and keep stack traces where swallowed.Catching bare
Exceptionin discovery makes debugging harder. Limit tohttpx.HTTPError | ValidationError | ValueErrorand uselogger.debug/exceptionappropriately.Apply this diff:
@@ -from pydantic import BaseModel -from pydantic import Field -from pydantic import HttpUrl +from pydantic import BaseModel +from pydantic import Field +from pydantic import HttpUrl +from pydantic import ValidationError @@ - try: - pr = ProtectedResourceMetadata.model_validate_json(body) - except Exception as e: - logger.debug("Invalid ProtectedResourceMetadata at %s: %s", url, e) + try: + pr = ProtectedResourceMetadata.model_validate_json(body) + except (ValidationError, ValueError) as e: + logger.debug("Invalid ProtectedResourceMetadata at %s: %s", url, e) return None @@ - try: - resp = await client.get(url, headers={"Accept": "application/json"}) - if resp.status_code != 200: - continue - body = await resp.aread() - try: - meta = OAuthMetadata.model_validate_json(body) - except Exception as e: - logger.debug("Invalid OAuthMetadata at %s: %s", url, e) - continue + try: + resp = await client.get(url, headers={"Accept": "application/json"}) + if resp.status_code != 200: + continue + body = await resp.aread() + try: + meta = OAuthMetadata.model_validate_json(body) + except (ValidationError, ValueError) as e: + logger.debug("Invalid OAuthMetadata at %s: %s", url, e) + continue if meta.authorization_endpoint and meta.token_endpoint: logger.info("Discovered OAuth2 endpoints from %s", url) # this is bit of a hack to get the scopes supported by the auth server self._last_oauth_scopes = meta.scopes_supported return OAuth2Endpoints( authorization_url=str(meta.authorization_endpoint), token_url=str(meta.token_endpoint), registration_url=str(meta.registration_endpoint) if meta.registration_endpoint else None, ) - except Exception as e: - logger.debug("Discovery failed at %s: %s", url, e) + except httpx.HTTPError as e: + logger.debug("Discovery failed at %s: %s", url, e)Also applies to: 141-168
221-235: Re-raising with context is fine; consider trimming message to appease TRY003 or ignore.
RuntimeError("Registration response was not valid ...")is acceptable; if you want to satisfy TRY003, either shorten the message or add# noqa: TRY003on that line.Would you like me to apply a minimal suppression comment here?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
docs/source/workflows/mcp/index.md(1 hunks)docs/source/workflows/mcp/mcp-auth.md(1 hunks)docs/source/workflows/mcp/mcp-client.md(2 hunks)docs/source/workflows/mcp/mcp-server.md(1 hunks)examples/MCP/simple_calculator_mcp/configs/config-mcp-auth-dynamic.yml(1 hunks)examples/MCP/simple_calculator_mcp/configs/config-mcp-auth-static.yml(1 hunks)examples/MCP/simple_calculator_mcp/configs/config-mcp-date-stdio.yml(1 hunks)examples/MCP/simple_calculator_mcp/configs/config-mcp-math.yml(1 hunks)examples/MCP/simple_calculator_mcp/configs/config-mcp-server-auth.yml(1 hunks)examples/MCP/simple_calculator_mcp/deploy_example_as/Dockerfile(1 hunks)packages/nvidia_nat_mcp/pyproject.toml(2 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/__init__.py(1 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.py(1 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.py(1 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/register.py(1 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py(8 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py(5 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/utils.py(1 hunks)src/nat/authentication/interfaces.py(3 hunks)src/nat/authentication/oauth2/oauth2_auth_code_flow_provider.py(2 hunks)src/nat/data_models/authentication.py(1 hunks)src/nat/front_ends/mcp/mcp_front_end_config.py(1 hunks)src/nat/front_ends/mcp/mcp_front_end_plugin.py(1 hunks)tests/nat/mcp/test_mcp_auth_provider.py(1 hunks)tests/nat/mcp/test_mcp_schema.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (17)
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
packages/*/src/**/*.py: All importable Python code in packages must live under packages//src/
All public APIs in packaged code require Python 3.11+ type hints; prefer typing/collections.abc; use typing.Annotated when useful
Provide Google-style docstrings for public APIs in packages; first line concise with a period; use backticks for code entities
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/__init__.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/utils.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/register.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.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 bareraiseand 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:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/__init__.pysrc/nat/front_ends/mcp/mcp_front_end_config.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.pytests/nat/mcp/test_mcp_schema.pytests/nat/mcp/test_mcp_auth_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.pysrc/nat/front_ends/mcp/mcp_front_end_plugin.pysrc/nat/authentication/interfaces.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/utils.pysrc/nat/data_models/authentication.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/register.pysrc/nat/authentication/oauth2/oauth2_auth_code_flow_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.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:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/__init__.pyexamples/MCP/simple_calculator_mcp/configs/config-mcp-math.ymlsrc/nat/front_ends/mcp/mcp_front_end_config.pydocs/source/workflows/mcp/mcp-server.mdexamples/MCP/simple_calculator_mcp/configs/config-mcp-auth-dynamic.ymlexamples/MCP/simple_calculator_mcp/configs/config-mcp-server-auth.ymlpackages/nvidia_nat_mcp/pyproject.tomlpackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.pydocs/source/workflows/mcp/index.mdtests/nat/mcp/test_mcp_schema.pytests/nat/mcp/test_mcp_auth_provider.pydocs/source/workflows/mcp/mcp-auth.mddocs/source/workflows/mcp/mcp-client.mdpackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.pysrc/nat/front_ends/mcp/mcp_front_end_plugin.pyexamples/MCP/simple_calculator_mcp/configs/config-mcp-auth-static.ymlsrc/nat/authentication/interfaces.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/utils.pysrc/nat/data_models/authentication.pyexamples/MCP/simple_calculator_mcp/configs/config-mcp-date-stdio.ymlpackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/register.pysrc/nat/authentication/oauth2/oauth2_auth_code_flow_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.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:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/__init__.pysrc/nat/front_ends/mcp/mcp_front_end_config.pydocs/source/workflows/mcp/mcp-server.mdpackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.pydocs/source/workflows/mcp/index.mdtests/nat/mcp/test_mcp_schema.pytests/nat/mcp/test_mcp_auth_provider.pydocs/source/workflows/mcp/mcp-auth.mddocs/source/workflows/mcp/mcp-client.mdpackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.pysrc/nat/front_ends/mcp/mcp_front_end_plugin.pysrc/nat/authentication/interfaces.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/utils.pysrc/nat/data_models/authentication.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/register.pysrc/nat/authentication/oauth2/oauth2_auth_code_flow_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.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:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/__init__.pyexamples/MCP/simple_calculator_mcp/configs/config-mcp-math.ymlsrc/nat/front_ends/mcp/mcp_front_end_config.pyexamples/MCP/simple_calculator_mcp/configs/config-mcp-auth-dynamic.ymlexamples/MCP/simple_calculator_mcp/configs/config-mcp-server-auth.ymlpackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.pytests/nat/mcp/test_mcp_schema.pytests/nat/mcp/test_mcp_auth_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.pysrc/nat/front_ends/mcp/mcp_front_end_plugin.pyexamples/MCP/simple_calculator_mcp/configs/config-mcp-auth-static.ymlsrc/nat/authentication/interfaces.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/utils.pysrc/nat/data_models/authentication.pyexamples/MCP/simple_calculator_mcp/configs/config-mcp-date-stdio.ymlpackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/register.pysrc/nat/authentication/oauth2/oauth2_auth_code_flow_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/__init__.pyexamples/MCP/simple_calculator_mcp/configs/config-mcp-math.ymlsrc/nat/front_ends/mcp/mcp_front_end_config.pydocs/source/workflows/mcp/mcp-server.mdexamples/MCP/simple_calculator_mcp/configs/config-mcp-auth-dynamic.ymlexamples/MCP/simple_calculator_mcp/configs/config-mcp-server-auth.ymlpackages/nvidia_nat_mcp/pyproject.tomlexamples/MCP/simple_calculator_mcp/deploy_example_as/Dockerfilepackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.pydocs/source/workflows/mcp/index.mdtests/nat/mcp/test_mcp_schema.pytests/nat/mcp/test_mcp_auth_provider.pydocs/source/workflows/mcp/mcp-auth.mddocs/source/workflows/mcp/mcp-client.mdpackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.pysrc/nat/front_ends/mcp/mcp_front_end_plugin.pyexamples/MCP/simple_calculator_mcp/configs/config-mcp-auth-static.ymlsrc/nat/authentication/interfaces.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/utils.pysrc/nat/data_models/authentication.pyexamples/MCP/simple_calculator_mcp/configs/config-mcp-date-stdio.ymlpackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/register.pysrc/nat/authentication/oauth2/oauth2_auth_code_flow_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/__init__.pypackages/nvidia_nat_mcp/pyproject.tomlpackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/utils.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/register.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py
**/configs/**/*.y?(a)ml
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Configuration files consumed by code must live in a neighbouring configs/ directory
Files:
examples/MCP/simple_calculator_mcp/configs/config-mcp-math.ymlexamples/MCP/simple_calculator_mcp/configs/config-mcp-auth-dynamic.ymlexamples/MCP/simple_calculator_mcp/configs/config-mcp-server-auth.ymlexamples/MCP/simple_calculator_mcp/configs/config-mcp-auth-static.ymlexamples/MCP/simple_calculator_mcp/configs/config-mcp-date-stdio.yml
**/*.{yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
In workflow/config YAML, set llms.._type: nat_test_llm to stub responses.
Files:
examples/MCP/simple_calculator_mcp/configs/config-mcp-math.ymlexamples/MCP/simple_calculator_mcp/configs/config-mcp-auth-dynamic.ymlexamples/MCP/simple_calculator_mcp/configs/config-mcp-server-auth.ymlexamples/MCP/simple_calculator_mcp/configs/config-mcp-auth-static.ymlexamples/MCP/simple_calculator_mcp/configs/config-mcp-date-stdio.yml
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 apyproject.tomlfile. Optionally, it might also contain scripts in ascripts/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 nameddata/, and should
be checked into git-lfs.
Files:
examples/MCP/simple_calculator_mcp/configs/config-mcp-math.ymlexamples/MCP/simple_calculator_mcp/configs/config-mcp-auth-dynamic.ymlexamples/MCP/simple_calculator_mcp/configs/config-mcp-server-auth.ymlexamples/MCP/simple_calculator_mcp/deploy_example_as/Dockerfileexamples/MCP/simple_calculator_mcp/configs/config-mcp-auth-static.ymlexamples/MCP/simple_calculator_mcp/configs/config-mcp-date-stdio.yml
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_config.pysrc/nat/front_ends/mcp/mcp_front_end_plugin.pysrc/nat/authentication/interfaces.pysrc/nat/data_models/authentication.pysrc/nat/authentication/oauth2/oauth2_auth_code_flow_provider.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_config.pysrc/nat/front_ends/mcp/mcp_front_end_plugin.pysrc/nat/authentication/interfaces.pysrc/nat/data_models/authentication.pysrc/nat/authentication/oauth2/oauth2_auth_code_flow_provider.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_config.pysrc/nat/front_ends/mcp/mcp_front_end_plugin.pysrc/nat/authentication/interfaces.pysrc/nat/data_models/authentication.pysrc/nat/authentication/oauth2/oauth2_auth_code_flow_provider.py
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.mddocs/source/workflows/mcp/index.mddocs/source/workflows/mcp/mcp-auth.mddocs/source/workflows/mcp/mcp-client.md
docs/source/**/*
⚙️ CodeRabbit configuration file
This directory contains the source code for the documentation. All documentation should be written in Markdown format. Any image files should be placed in the
docs/source/_staticdirectory.
Files:
docs/source/workflows/mcp/mcp-server.mddocs/source/workflows/mcp/index.mddocs/source/workflows/mcp/mcp-auth.mddocs/source/workflows/mcp/mcp-client.md
packages/*/pyproject.toml
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
packages/*/pyproject.toml: Each package under packages/ must include a pyproject.toml
Package pyproject.toml must depend on nvidia-nat or a package starting with nvidia-nat- using ~= versions (e.g., ~=1.0)
Files:
packages/nvidia_nat_mcp/pyproject.toml
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/mcp/test_mcp_schema.pytests/nat/mcp/test_mcp_auth_provider.py
⚙️ CodeRabbit configuration file
tests/**/*.py: - Ensure that tests are comprehensive, cover edge cases, and validate the functionality of the code. - Test functions should be named using thetest_prefix, using snake_case. - Any frequently repeated code should be extracted into pytest fixtures. - Pytest fixtures should define the name argument when applying the pytest.fixture decorator. The fixture
function being decorated should be named using thefixture_prefix, using snake_case. Example:
@pytest.fixture(name="my_fixture")
def fixture_my_fixture():
pass
Files:
tests/nat/mcp/test_mcp_schema.pytests/nat/mcp/test_mcp_auth_provider.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/mcp/test_mcp_schema.pytests/nat/mcp/test_mcp_auth_provider.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 packages/*/pyproject.toml : Package pyproject.toml must depend on nvidia-nat or a package starting with nvidia-nat- using ~=<two-digit> versions (e.g., ~=1.0)
Applied to files:
packages/nvidia_nat_mcp/pyproject.toml
🧬 Code graph analysis (9)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.py (2)
src/nat/data_models/authentication.py (1)
AuthProviderBaseConfig(31-37)src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (1)
redirect_uri(1047-1081)
tests/nat/mcp/test_mcp_schema.py (1)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/utils.py (1)
model_from_mcp_schema(24-95)
tests/nat/mcp/test_mcp_auth_provider.py (3)
src/nat/data_models/authentication.py (4)
AuthReason(234-239)AuthRequest(242-250)AuthResult(180-231)BearerTokenCred(158-165)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.py (15)
DiscoverOAuth2Endpoints(52-183)DynamicClientRegistration(186-234)MCPOAuth2Provider(237-349)OAuth2Credentials(46-49)OAuth2Endpoints(39-43)discover(66-105)_extract_from_www_authenticate_header(112-124)_authorization_base_url(108-110)_authorization_base_url(192-194)_build_path_aware_discovery_urls(169-180)register(196-234)authenticate(256-285)_effective_scopes(311-315)_perform_oauth2_flow(338-349)_discover_and_register(287-309)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.py (1)
MCPOAuth2ProviderConfig(23-73)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.py (4)
src/nat/authentication/interfaces.py (4)
AuthProviderBase(30-74)config(47-56)authenticate(59-74)authenticate(87-98)src/nat/data_models/authentication.py (3)
AuthReason(234-239)AuthRequest(242-250)AuthResult(180-231)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.py (1)
MCPOAuth2ProviderConfig(23-73)src/nat/authentication/oauth2/oauth2_auth_code_flow_provider.py (2)
authenticate(90-126)OAuth2AuthCodeFlowProvider(31-126)
src/nat/authentication/interfaces.py (3)
src/nat/data_models/authentication.py (2)
AuthRequest(242-250)AuthResult(180-231)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.py (1)
authenticate(256-285)src/nat/authentication/oauth2/oauth2_auth_code_flow_provider.py (1)
authenticate(90-126)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/register.py (3)
src/nat/cli/type_registry.py (1)
register_auth_provider(502-511)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.py (1)
MCPOAuth2Provider(237-349)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.py (1)
MCPOAuth2ProviderConfig(23-73)
src/nat/authentication/oauth2/oauth2_auth_code_flow_provider.py (4)
src/nat/data_models/authentication.py (2)
AuthRequest(242-250)AuthResult(180-231)src/nat/builder/context.py (1)
Context(93-277)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.py (1)
authenticate(256-285)src/nat/authentication/interfaces.py (2)
authenticate(59-74)authenticate(87-98)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py (1)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py (14)
transport(146-147)command(293-294)args(301-302)env(305-306)url(255-256)url(336-337)name(382-384)server_name(173-177)server_name(259-260)server_name(297-298)server_name(340-341)MCPStdioClient(274-319)MCPSSEClient(242-271)MCPStreamableHTTPClient(322-353)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py (8)
src/nat/authentication/interfaces.py (3)
AuthProviderBase(30-74)authenticate(59-74)authenticate(87-98)src/nat/data_models/authentication.py (3)
AuthReason(234-239)AuthRequest(242-250)BearerTokenCred(158-165)packages/nvidia_nat_mcp/src/nat/plugins/mcp/utils.py (1)
model_from_mcp_schema(24-95)src/nat/utils/type_utils.py (1)
override(56-57)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.py (1)
authenticate(256-285)src/nat/authentication/oauth2/oauth2_auth_code_flow_provider.py (1)
authenticate(90-126)tests/nat/mcp/test_mcp_client_impl.py (2)
connect_to_server(80-82)get_tools(75-77)tests/nat/mcp/test_mcp_tool.py (2)
connect_to_server(70-72)get_tools(65-67)
🪛 Ruff (0.12.2)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.py
69-71: Avoid specifying long messages outside the exception class
(TRY003)
tests/nat/mcp/test_mcp_auth_provider.py
57-57: Possible hardcoded password assigned to argument: "client_secret"
(S106)
78-78: Possible hardcoded password assigned to argument: "client_secret"
(S106)
252-252: Possible hardcoded password assigned to: "client_secret"
(S105)
336-336: Unused method argument: monkeypatch
(ARG002)
346-346: Possible hardcoded password assigned to argument: "token"
(S106)
369-369: Unused method argument: monkeypatch
(ARG002)
383-383: Possible hardcoded password assigned to argument: "token"
(S106)
403-403: Unused method argument: mock_config
(ARG002)
403-403: Unused method argument: monkeypatch
(ARG002)
409-409: Possible hardcoded password assigned to argument: "client_secret"
(S106)
434-434: Unused method argument: mock_config
(ARG002)
491-491: Unused method argument: mock_endpoints
(ARG002)
503-503: Unused function argument: args
(ARG001)
503-503: Unused function argument: kwargs
(ARG001)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.py
91-91: Do not catch blind exception: Exception
(BLE001)
97-97: Avoid specifying long messages outside the exception class
(TRY003)
134-134: Do not catch blind exception: Exception
(BLE001)
153-153: Do not catch blind exception: Exception
(BLE001)
165-165: Do not catch blind exception: Exception
(BLE001)
227-228: Avoid specifying long messages outside the exception class
(TRY003)
231-231: Avoid specifying long messages outside the exception class
(TRY003)
264-264: Avoid specifying long messages outside the exception class
(TRY003)
341-341: Avoid specifying long messages outside the exception class
(TRY003)
344-344: Avoid specifying long messages outside the exception class
(TRY003)
src/nat/front_ends/mcp/mcp_front_end_plugin.py
100-100: Avoid specifying long messages outside the exception class
(TRY003)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/register.py
23-23: Unused function argument: builder
(ARG001)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py
71-71: Avoid specifying long messages outside the exception class
(TRY003)
74-74: Avoid specifying long messages outside the exception class
(TRY003)
76-76: Avoid specifying long messages outside the exception class
(TRY003)
79-79: Avoid specifying long messages outside the exception class
(TRY003)
82-82: Avoid specifying long messages outside the exception class
(TRY003)
84-84: Avoid specifying long messages outside the exception class
(TRY003)
169-169: Avoid specifying long messages outside the exception class
(TRY003)
176-176: Avoid specifying long messages outside the exception class
(TRY003)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py
66-66: Do not catch blind exception: Exception
(BLE001)
79-79: Do not catch blind exception: Exception
(BLE001)
117-117: Do not catch blind exception: Exception
(BLE001)
135-135: Avoid specifying long messages outside the exception class
(TRY003)
165-165: Avoid specifying long messages outside the exception class
(TRY003)
416-416: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI Pipeline / Check
🔇 Additional comments (19)
examples/MCP/simple_calculator_mcp/deploy_example_as/Dockerfile (2)
1-14: License header looks correct and current.
SPDX headers and 2025 copyright year are present.
16-16: Examples directory hygiene.
Confirm this example has a README and follows the examples layout (src/, configs/, data/ as applicable).I can add a minimal README with build/run instructions for this Dockerfile if needed.
examples/MCP/simple_calculator_mcp/configs/config-mcp-math.yml (1)
68-68: Tool ordering change looks fine.Reordering
current_datetimeto the end is benign and keeps math tools grouped first.packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/__init__.py (1)
1-15: Header-only initializer is OK.SPDX header present; no exports needed here.
examples/MCP/simple_calculator_mcp/configs/config-mcp-date-stdio.yml (1)
66-71: Tool list reorder LGTM.Moving the time tool to the end is fine and keeps math tools grouped.
src/nat/front_ends/mcp/mcp_front_end_config.py (1)
26-27: Correct protocol expansion in docstring.“MCP” expands to “Model Context Protocol”, not “Modular Communication Protocol”.
[raise_ninor_issue]
Apply:- A simple MCP (Modular Communication Protocol) front end for NeMo Agent toolkit. + A simple MCP (Model Context Protocol) front end for the NeMo Agent toolkit.docs/source/workflows/mcp/mcp-client.md (1)
78-83: Grammar nit — change "it's" → "its"; duplicate paragraph not foundApply patch below; search of docs/source/workflows/mcp/mcp-client.md shows only one occurrence (lines 78–83).
- * a validated instance of it's input schema + * a validated instance of its input schemaLikely an incorrect or invalid review comment.
tests/nat/mcp/test_mcp_schema.py (1)
21-21: Import relocation LGTM.The test correctly targets the new utils location.
src/nat/authentication/interfaces.py (1)
35-35: Centralizing session cookie name is good.Using a single SESSION_COOKIE_NAME avoids drift across components.
examples/MCP/simple_calculator_mcp/configs/config-mcp-auth-dynamic.yml (1)
38-44: Consider a stub LLM for offline examples.To make the example runnable without external services, provide an alt config using
nat_test_llm(or add a commented block showing how to switch).Would you like me to add a parallel file
config-mcp-auth-dynamic.local.ymlthat swapsnimfornat_test_llmand setsresponse_seq?packages/nvidia_nat_mcp/pyproject.toml (1)
49-49: mcp_oauth2_provider entry-point presentpackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/register.py defines async def mcp_oauth2_provider; the earlier ModuleNotFoundError was environment-related (nat not on PYTHONPATH). Install the package or add packages/nvidia_nat_mcp/src (or the repo's src) to PYTHONPATH to import at runtime.
src/nat/front_ends/mcp/mcp_front_end_plugin.py (1)
107-114: LGTM: FastMCP wired with optional auth.Passing
authandtoken_verifieris correct; construction stays backward‑compatible when auth is disabled.src/nat/authentication/oauth2/oauth2_auth_code_flow_provider.py (1)
26-26: ImportingAuthRequestis appropriate.Prepares the provider for richer auth contexts.
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py (3)
27-27: Auth provider reference import is correct.Matches field usage below.
126-126: Logging improvement LGTM.Using
server_nameclarifies the transport and target.
178-178: Final log LGTM.Consistent with single‑tool log.
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.py (1)
23-31: Config class looks solid.Clear docstring and pydantic usage.
tests/nat/mcp/test_mcp_auth_provider.py (1)
106-170: Good coverage and concurrency checks.Discovery cache, header parsing, DCR success/failure, provider behavior, and lock semantics are well tested.
Also applies to: 232-305, 309-549
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.py (1)
256-286: Auth flow orchestration looks solid.Clear separation of discovery/registration and delegate flow; proper 401 normalization and locking.
Drop locks for the time being. This needs to be re-visited. Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/nat/mcp/test_mcp_auth_provider.py (2)
19-23: Add pytest-asyncio marker at module level so async tests run.Without it, async tests may be skipped or fail.
import pytest @@ from nat.plugins.mcp.auth.auth_provider_config import MCPOAuth2ProviderConfig +# Run all async tests in this module with pytest-asyncio +pytestmark = pytest.mark.asyncio +
42-47: Don’t construct HttpUrl directly in tests; pass strings and let Pydantic coerce.Pydantic v2 treats HttpUrl as a type annotation, not a constructor; calling HttpUrl(...) will raise.
- server_url=HttpUrl("https://example.com/mcp"), - redirect_uri=HttpUrl("https://example.com/callback"), + server_url="https://example.com/mcp", + redirect_uri="https://example.com/callback", @@ - authorization_url=HttpUrl("https://auth.example.com/authorize"), - token_url=HttpUrl("https://auth.example.com/token"), - registration_url=HttpUrl("https://auth.example.com/register"), + authorization_url="https://auth.example.com/authorize", + token_url="https://auth.example.com/token", + registration_url="https://auth.example.com/register",Apply similarly to other occurrences in this file.
Also applies to: 53-60, 66-71, 110-114, 136-140, 154-158, 254-258, 325-330, 406-413, 433-436, 464-468
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py (1)
54-83: Fix httpx.Auth flow: only yield requests; copy before mutation; log stack traces.Current implementation yields a response and mutates the original request; both are incorrect for httpx.Auth.
- async def async_auth_flow(self, request: httpx.Request) -> AsyncGenerator[httpx.Request, httpx.Response]: + async def async_auth_flow(self, request: httpx.Request) -> AsyncGenerator[httpx.Request, httpx.Response]: """Add authentication headers to the request using NAT auth provider.""" # Check if we should only auth tool calls, Is this needed? if self.auth_for_tool_calls_only and not self._is_tool_call_request(request): # Skip auth for non-tool calls yield request return try: # Get fresh auth headers from the NAT auth provider - auth_headers = await self._get_auth_headers(reason=AuthReason.NORMAL) - request.headers.update(auth_headers) - except Exception as e: - logger.warning("Failed to get auth headers: %s", e) + auth_headers = await self._get_auth_headers(reason=AuthReason.NORMAL) + req = request.copy() + req.headers.update(auth_headers) + except Exception: + logger.exception("Failed to get auth headers") # Continue without auth headers if auth fails - - response = yield request + req = request + response = yield req # Handle 401 responses by retrying with fresh auth if response.status_code == 401: try: # Get fresh auth headers with 401 context - auth_headers = await self._get_auth_headers(reason=AuthReason.RETRY_AFTER_401, response=response) - request.headers.update(auth_headers) - response = yield request # Retry the request - except Exception as e: - logger.warning("Failed to refresh auth after 401: %s", e) - - yield response + auth_headers = await self._get_auth_headers(reason=AuthReason.RETRY_AFTER_401, response=response) + retry_req = request.copy() + retry_req.headers.update(auth_headers) + response = yield retry_req # Retry the request + except Exception: + logger.exception("Failed to refresh auth after 401") + return
🧹 Nitpick comments (11)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.py (4)
39-44: Use SecretStr for client_secret.Avoid keeping secrets in plain strings; prefer Pydantic’s SecretStr.
-from pydantic import Field +from pydantic import Field +from pydantic import SecretStr @@ - client_secret: str | None = Field(default=None, description="OAuth2 client secret for pre-registered clients") + client_secret: SecretStr | None = Field(default=None, description="OAuth2 client secret for pre-registered clients")
16-19: Annotate validator return type (Self).Pydantic v2 model validators should return Self for clarity and type-checkers.
+from typing import Self @@ - def validate_auth_config(self): + def validate_auth_config(self) -> Self:Also applies to: 56-57
72-74: Shorten exception message to satisfy TRY003 and keep it actionable.Current multi-part message trips ruff TRY003.
- raise ValueError("Must provide either: " - "1) enable_dynamic_registration=True (dynamic), or " - "2) client_id + client_secret (hybrid)") + raise ValueError("Invalid MCP OAuth2 config: choose dynamic registration or provide client_id+client_secret.")
47-50: Constrain token_endpoint_auth_method to allowed literals.Catching typos early improves UX and validation.
-from pydantic import Field +from pydantic import Field +from typing import Literal @@ - token_endpoint_auth_method: str = Field(default="client_secret_post", + token_endpoint_auth_method: Literal["client_secret_post", "client_secret_basic", "private_key_jwt", "none"] = Field( + default="client_secret_post", description="The authentication method for the token endpoint.")tests/nat/mcp/test_mcp_auth_provider.py (1)
58-58: Silence secrets-linter warnings in tests.Mark hardcoded client_secret values in tests with noqa since they are non-sensitive fixtures.
- client_secret="test_client_secret", + client_secret="test_client_secret", # noqa: S106 (tests)Also applies to: 78-78, 248-248, 334-334, 410-410
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.py (4)
239-248: Manual-credentials path must unwrap SecretStr (from config change).If config.client_secret becomes SecretStr, unwrap before building credentials and when passing to the delegate.
@@ - self._cached_credentials = OAuth2Credentials( - client_id=self.config.client_id, - client_secret=self.config.client_secret, - ) + self._cached_credentials = OAuth2Credentials( + client_id=self.config.client_id, + client_secret=(self.config.client_secret.get_secret_value() + if getattr(self.config, "client_secret", None) else None), + ) @@ - oauth2_config = OAuth2AuthCodeFlowProviderConfig( + oauth2_config = OAuth2AuthCodeFlowProviderConfig( client_id=credentials.client_id, - client_secret=credentials.client_secret or "", + client_secret=(credentials.client_secret or ""),Also applies to: 315-322
338-354: Propagate user_id explicitly; don’t inject unknown fields into AuthRequest.AuthRequest forbids extras;
model_copy(update={"user_id": ...})is a no-op. Pass user_id to the delegate instead.@@ async def authenticate(self, user_id: str | None = None) -> AuthResult: - if auth_request.reason == AuthReason.RETRY_AFTER_401: + if auth_request.reason == AuthReason.RETRY_AFTER_401: # force fresh delegate (clears in-mem token cache) self._auth_code_provider = None - # preserve other fields, just normalize reason & inject user_id - auth_request = auth_request.model_copy(update={ - "reason": AuthReason.NORMAL, "user_id": user_id, "www_authenticate": None - }) + # normalize reason for the subsequent flow + auth_request = auth_request.model_copy(update={"reason": AuthReason.NORMAL, "www_authenticate": None}) @@ - return await self._perform_oauth2_flow(auth_request=auth_request) + return await self._perform_oauth2_flow(user_id=user_id, auth_request=auth_request) @@ - async def _perform_oauth2_flow(self, auth_request: AuthRequest | None = None) -> AuthResult: + async def _perform_oauth2_flow(self, user_id: str | None, auth_request: AuthRequest | None = None) -> AuthResult: @@ - return await self._auth_code_provider.authenticate() + return await self._auth_code_provider.authenticate(user_id=user_id)Also applies to: 355-367, 268-299
137-151: Narrow broad exceptions; log stack traces when swallowing.Replace bare
except Exceptionwith specific exceptions and use logger.exception when not re-raising, per repo guidance. Also shorten TRY003 messages.+from pydantic import ValidationError @@ def discover(self, reason: AuthReason, www_authenticate: str | None) -> tuple[OAuth2Endpoints, bool]: - if endpoints is None: - raise RuntimeError("Could not discover OAuth2 endpoints from MCP server") + if endpoints is None: + raise RuntimeError("OAuth2 discovery failed") @@ async def _fetch_pr_issuer(self, url: str) -> str | None: - try: + try: pr = ProtectedResourceMetadata.model_validate_json(body) - except Exception as e: - logger.debug("Invalid ProtectedResourceMetadata at %s: %s", url, e) + except ValidationError as e: + logger.exception("Invalid ProtectedResourceMetadata at %s", url) return None @@ async def _discover_via_issuer_or_base(self, base_or_issuer: str) -> OAuth2Endpoints | None: - async with httpx.AsyncClient(timeout=10.0) as client: + async with httpx.AsyncClient(timeout=10.0) as client: for url in urls: try: resp = await client.get(url, headers={"Accept": "application/json"}) if resp.status_code != 200: continue body = await resp.aread() - try: + try: meta = OAuthMetadata.model_validate_json(body) - except Exception as e: - logger.debug("Invalid OAuthMetadata at %s: %s", url, e) + except ValidationError: + logger.exception("Invalid OAuthMetadata at %s", url) continue if meta.authorization_endpoint and meta.token_endpoint: logger.info("Discovered OAuth2 endpoints from %s", url) # this is bit of a hack to get the scopes supported by the auth server self._last_oauth_scopes = meta.scopes_supported return OAuth2Endpoints( authorization_url=str(meta.authorization_endpoint), token_url=str(meta.token_endpoint), registration_url=str(meta.registration_endpoint) if meta.registration_endpoint else None, ) - except Exception as e: - logger.debug("Discovery failed at %s: %s", url, e) + except httpx.HTTPError: + logger.exception("Discovery failed at %s", url) return NoneAlso applies to: 152-179, 92-106
241-246: TRY003: shorten raised message.Keep the core context; avoid overly long f-string messages.
- raise RuntimeError( - f"Registration response was not valid OAuthClientInformation from {registration_url}") from e + raise RuntimeError("Invalid OAuth client registration response") from epackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py (2)
23-25: Add return type hints; sanitize logging; import Any.Align with repo standards and avoid logging potential PII in tool args.
-from typing import AsyncGenerator +from typing import AsyncGenerator, Any @@ - async def __aenter__(self): + async def __aenter__(self) -> "MCPBaseClient": @@ - async def __aexit__(self, exc_type, exc_value, traceback): + async def __aexit__(self, exc_type, exc_value, traceback) -> None: @@ - async def connect_to_server(self): + async def connect_to_server(self) -> AsyncGenerator[ClientSession, None]: @@ - async def get_tools(self): + async def get_tools(self) -> dict[str, "MCPToolClient"]: @@ - async def call_tool(self, tool_name: str, tool_args: dict | None): + async def call_tool(self, tool_name: str, tool_args: dict | None) -> Any: @@ - async def acall(self, tool_args: dict) -> str: + async def acall(self, tool_args: dict[str, Any]) -> str: @@ - logger.info("Calling tool %s with arguments %s", self._tool_name, tool_args) + logger.info("Calling tool %s", self._tool_name)Also applies to: 151-161, 165-171, 183-188, 189-201, 235-241, 410-419
191-197: Docstring inconsistency: discovery says “Uses unauthenticated session” but auth is wired at transport.Either set AuthAdapter to only auth tool calls or update the docstring.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.py(1 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.py(1 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py(8 hunks)src/nat/data_models/authentication.py(1 hunks)tests/nat/mcp/test_mcp_auth_provider.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/nat/data_models/authentication.py
🧰 Additional context used
📓 Path-based instructions (9)
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/mcp/test_mcp_auth_provider.py
⚙️ CodeRabbit configuration file
tests/**/*.py: - Ensure that tests are comprehensive, cover edge cases, and validate the functionality of the code. - Test functions should be named using thetest_prefix, using snake_case. - Any frequently repeated code should be extracted into pytest fixtures. - Pytest fixtures should define the name argument when applying the pytest.fixture decorator. The fixture
function being decorated should be named using thefixture_prefix, using snake_case. Example:
@pytest.fixture(name="my_fixture")
def fixture_my_fixture():
pass
Files:
tests/nat/mcp/test_mcp_auth_provider.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 bareraiseand 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:
tests/nat/mcp/test_mcp_auth_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.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/mcp/test_mcp_auth_provider.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:
tests/nat/mcp/test_mcp_auth_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.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:
tests/nat/mcp/test_mcp_auth_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.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:
tests/nat/mcp/test_mcp_auth_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
tests/nat/mcp/test_mcp_auth_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
packages/*/src/**/*.py: All importable Python code in packages must live under packages//src/
All public APIs in packaged code require Python 3.11+ type hints; prefer typing/collections.abc; use typing.Annotated when useful
Provide Google-style docstrings for public APIs in packages; first line concise with a period; use backticks for code entities
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.py
🧬 Code graph analysis (4)
tests/nat/mcp/test_mcp_auth_provider.py (2)
src/nat/data_models/authentication.py (4)
AuthReason(234-239)AuthRequest(242-249)AuthResult(180-231)BearerTokenCred(158-165)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.py (14)
DiscoverOAuth2Endpoints(51-196)DynamicClientRegistration(199-248)MCPOAuth2Provider(251-367)OAuth2Credentials(45-48)OAuth2Endpoints(38-42)discover(65-114)_extract_from_www_authenticate_header(122-135)_authorization_base_url(117-120)_authorization_base_url(205-208)_build_path_aware_discovery_urls(180-192)register(210-248)authenticate(268-298)_effective_scopes(327-331)_perform_oauth2_flow(355-367)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.py (4)
src/nat/authentication/interfaces.py (4)
AuthProviderBase(29-69)config(45-54)authenticate(57-69)authenticate(82-93)src/nat/data_models/authentication.py (3)
AuthReason(234-239)AuthRequest(242-249)AuthResult(180-231)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.py (1)
MCPOAuth2ProviderConfig(24-76)src/nat/authentication/oauth2/oauth2_auth_code_flow_provider.py (2)
authenticate(65-107)OAuth2AuthCodeFlowProvider(30-107)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py (5)
src/nat/authentication/interfaces.py (4)
AuthProviderBase(29-69)config(45-54)authenticate(57-69)authenticate(82-93)src/nat/data_models/authentication.py (3)
AuthReason(234-239)AuthRequest(242-249)BearerTokenCred(158-165)packages/nvidia_nat_mcp/src/nat/plugins/mcp/utils.py (1)
model_from_mcp_schema(24-95)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.py (1)
authenticate(268-298)src/nat/authentication/oauth2/oauth2_auth_code_flow_provider.py (1)
authenticate(65-107)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.py (1)
src/nat/data_models/authentication.py (2)
AuthProviderBaseConfig(31-37)AuthRequest(242-249)
🪛 Ruff (0.12.2)
tests/nat/mcp/test_mcp_auth_provider.py
57-57: Possible hardcoded password assigned to argument: "client_secret"
(S106)
78-78: Possible hardcoded password assigned to argument: "client_secret"
(S106)
248-248: Possible hardcoded password assigned to: "client_secret"
(S105)
334-334: Possible hardcoded password assigned to argument: "client_secret"
(S106)
410-410: Possible hardcoded password assigned to argument: "client_secret"
(S106)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.py
100-100: Do not catch blind exception: Exception
(BLE001)
106-106: Avoid specifying long messages outside the exception class
(TRY003)
145-145: Do not catch blind exception: Exception
(BLE001)
164-164: Do not catch blind exception: Exception
(BLE001)
176-176: Do not catch blind exception: Exception
(BLE001)
241-242: Avoid specifying long messages outside the exception class
(TRY003)
245-245: Avoid specifying long messages outside the exception class
(TRY003)
359-359: Avoid specifying long messages outside the exception class
(TRY003)
362-362: Avoid specifying long messages outside the exception class
(TRY003)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py
66-66: Do not catch blind exception: Exception
(BLE001)
79-79: Do not catch blind exception: Exception
(BLE001)
119-119: Do not catch blind exception: Exception
(BLE001)
137-137: Avoid specifying long messages outside the exception class
(TRY003)
167-167: Avoid specifying long messages outside the exception class
(TRY003)
418-418: Avoid specifying long messages outside the exception class
(TRY003)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.py
72-74: Avoid specifying long messages outside the exception class
(TRY003)
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
src/nat/front_ends/mcp/mcp_front_end_config.py (1)
47-47: Strongly type auth_server_url.Use HttpUrl for early URL validation.
Apply:
-from pydantic import Field +from pydantic import Field, HttpUrl- auth_server_url: str | None = Field(default=None, description="Authorization server URL for token validation") + auth_server_url: HttpUrl | None = Field(default=None, description="Authorization server URL for token validation")(Optional) enforce HTTPS in the same validator:
if self.auth_server_url and self.auth_server_url.scheme != "https": raise ValueError("auth_server_url must use https")packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py (4)
152-171: Add return type hints for public APIs (repo standard).Annotate
__aenter__,__aexit__, and the abstract context manager method.- async def __aenter__(self): + async def __aenter__(self) -> "MCPBaseClient": @@ - async def __aexit__(self, exc_type, exc_value, traceback): + async def __aexit__(self, exc_type, exc_value, traceback) -> None: @@ - async def connect_to_server(self): + async def connect_to_server(self) -> AsyncGenerator[ClientSession, None]:Also applies to: 183-189
98-123: Don’t mutate provider config per request; serialize or pass AuthRequest.Assigning
self.auth_provider.config.auth_request = ...is racy under concurrency and can leak the wrongAuthRequestacross in-flight requests. Quick mitigation: serialize the mutation+authenticate with a lock. Prefer follow-up refactor to acceptAuthRequestas a parameter onauthenticate(...). (Known issue in this PR thread.)@@ -from __future__ import annotations +from __future__ import annotations @@ -import logging +import logging +import asyncio @@ class AuthAdapter(httpx.Auth): - def __init__(self, auth_provider: AuthProviderBase, auth_for_tool_calls_only: bool = False): + def __init__(self, auth_provider: AuthProviderBase, auth_for_tool_calls_only: bool = False): self.auth_provider = auth_provider self.auth_for_tool_calls_only = auth_for_tool_calls_only + self._auth_lock = asyncio.Lock() @@ - try: - # Mutating the config is not thread-safe, so we need to lock here - # Is mutating the config the only way to pass the auth request to the auth provider? This needs - # to be re-visited. - self.auth_provider.config.auth_request = auth_request - auth_result = await self.auth_provider.authenticate() + try: + # Quick mitigation: serialize per-request mutation + authenticate + async with self._auth_lock: + self.auth_provider.config.auth_request = auth_request + auth_result = await self.auth_provider.authenticate() @@ - except Exception as e: - logger.warning("Failed to get auth token: %s", e) + except Exception: + logger.exception("Failed to get auth token") return {}If you want, I can open a follow-up issue to track the API refactor of
AuthProviderBase.authenticate(auth_request: AuthRequest, ...).
418-421: Don’t log tool arguments (PII/secrets risk).Drop
tool_argsfrom logs to avoid sensitive data leakage.- logger.info("Calling tool %s with arguments %s", self._tool_name, tool_args) + logger.info("Calling tool %s", self._tool_name)
62-81: Auth flow: capture stack traces; avoid swallow-then-continue.Use logger.exception() and don’t stash Exception as
ewhen you’re not using it. Keeps tracebacks, satisfies repo logging guidance, and appeases Ruff BLE001/TRY003 signal.@@ - except Exception as e: - logger.info("Failed to get auth headers: %s", e) + except Exception: + logger.exception("Failed to get auth headers") @@ - except Exception as e: - logger.info("Failed to refresh auth after 401: %s", e) + except Exception: + logger.exception("Failed to refresh auth after 401")tests/nat/mcp/test_mcp_auth_provider.py (4)
19-21: Mark async tests and quiet secret-strings in tests.Add module-level
pytestmark = pytest.mark.asyncio. Also silence S105/S106 for test-only literals via a file-level ruff directive.@@ import pytest from pydantic import SecretStr + +# ruff: noqa: S105,S106 # test-only dummy secrets/URLs +pytestmark = pytest.mark.asyncio
700-704: Remove unused noqa directives (repeat).- with patch('nat.authentication.oauth2.oauth2_auth_code_flow_provider.OAuth2AuthCodeFlowProvider' - ) as mock_oauth2_provider: # noqa: E501 + with patch('nat.authentication.oauth2.oauth2_auth_code_flow_provider.OAuth2AuthCodeFlowProvider' + ) as mock_oauth2_provider:
717-721: Remove unused noqa directives (repeat).- with patch('nat.authentication.oauth2.oauth2_auth_code_flow_provider.OAuth2AuthCodeFlowProvider' - ) as mock_oauth2_provider: # noqa: E501 + with patch('nat.authentication.oauth2.oauth2_auth_code_flow_provider.OAuth2AuthCodeFlowProvider' + ) as mock_oauth2_provider:
845-846: Remove unused noqa directives (repeat).- async def test_authenticate_retry_after_401_clears_auth_code_provider(self, - mock_config_with_credentials, + async def test_authenticate_retry_after_401_clears_auth_code_provider(self, + mock_config_with_credentials, mock_endpoints): # noqa: E501Suggestion: drop the
# noqa: E501if the line length is within limits after reflow.
🧹 Nitpick comments (5)
src/nat/front_ends/mcp/mcp_front_end_config.py (2)
46-46: Nit: wording consistency.Use “OAuth 2.1” (with a space) in descriptions.
- require_auth: bool = Field(default=False, description="Require OAuth2.1 authorization for MCP server access") + require_auth: bool = Field(default=False, description="Require OAuth 2.1 authorization for MCP server access")
19-21: Import hygiene.Drop ValidationError (unused after the validator fix). Keep imports grouped per ruff/yapf settings.
-from pydantic import ValidationErrorpackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py (2)
112-120: Use generic credential attachment; don’t assume Bearer.Leverage
AuthResult.as_requests_kwargs()to build headers so Basic, Cookie, Query, or custom creds also work.- # Check if we have BearerTokenCred - from nat.data_models.authentication import BearerTokenCred - if auth_result.credentials and isinstance(auth_result.credentials[0], BearerTokenCred): - token = auth_result.credentials[0].token.get_secret_value() - return {"Authorization": f"Bearer {token}"} - else: - logger.warning("Auth provider did not return BearerTokenCred") - return {} + headers = auth_result.as_requests_kwargs().get("headers", {}) + if not headers: + logger.warning("Auth provider returned no headers") + return headers
83-97: Minor: gate JSON parse by content-type; small robustness win.Avoid parsing non‑JSON bodies and reduce exceptions.
- try: - # Check if the request body contains a tool call - if request.content: - import json - body = json.loads(request.content.decode('utf-8')) + try: + # Parse only when JSON content is indicated + if request.headers.get("content-type", "").lower().startswith("application/json") and request.content: + import json + body = json.loads(request.content.decode("utf-8"))tests/nat/mcp/test_mcp_auth_provider.py (1)
683-689: Remove unused noqa directives.
# noqa: E501is flagged as unused by Ruff; the lines are already folded/wrapped.- with patch('nat.authentication.oauth2.oauth2_auth_code_flow_provider.OAuth2AuthCodeFlowProvider' - ) as mock_oauth2_provider: # noqa: E501 + with patch('nat.authentication.oauth2.oauth2_auth_code_flow_provider.OAuth2AuthCodeFlowProvider' + ) as mock_oauth2_provider:
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py(8 hunks)src/nat/front_ends/mcp/mcp_front_end_config.py(2 hunks)tests/nat/mcp/test_mcp_auth_provider.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
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/mcp/test_mcp_auth_provider.py
⚙️ CodeRabbit configuration file
tests/**/*.py: - Ensure that tests are comprehensive, cover edge cases, and validate the functionality of the code. - Test functions should be named using thetest_prefix, using snake_case. - Any frequently repeated code should be extracted into pytest fixtures. - Pytest fixtures should define the name argument when applying the pytest.fixture decorator. The fixture
function being decorated should be named using thefixture_prefix, using snake_case. Example:
@pytest.fixture(name="my_fixture")
def fixture_my_fixture():
pass
Files:
tests/nat/mcp/test_mcp_auth_provider.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 bareraiseand 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:
tests/nat/mcp/test_mcp_auth_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.pysrc/nat/front_ends/mcp/mcp_front_end_config.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/mcp/test_mcp_auth_provider.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:
tests/nat/mcp/test_mcp_auth_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.pysrc/nat/front_ends/mcp/mcp_front_end_config.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:
tests/nat/mcp/test_mcp_auth_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.pysrc/nat/front_ends/mcp/mcp_front_end_config.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:
tests/nat/mcp/test_mcp_auth_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.pysrc/nat/front_ends/mcp/mcp_front_end_config.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
tests/nat/mcp/test_mcp_auth_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.pysrc/nat/front_ends/mcp/mcp_front_end_config.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
packages/*/src/**/*.py: All importable Python code in packages must live under packages//src/
All public APIs in packaged code require Python 3.11+ type hints; prefer typing/collections.abc; use typing.Annotated when useful
Provide Google-style docstrings for public APIs in packages; first line concise with a period; use backticks for code entities
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py
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_config.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_config.py
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/front_ends/mcp/mcp_front_end_config.py
🧠 Learnings (3)
📚 Learning: 2025-09-17T05:20:03.228Z
Learnt from: AnuradhaKaruppiah
PR: NVIDIA/NeMo-Agent-Toolkit#752
File: packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py:99-121
Timestamp: 2025-09-17T05:20:03.228Z
Learning: The AuthAdapter class in packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py has a known concurrency race condition where `self.auth_provider.config.auth_request = auth_request` can be racy under concurrent requests. This should be addressed by either refactoring AuthProviderBase.authenticate() to accept AuthRequest as a parameter or using an asyncio.Lock to serialize access.
Applied to files:
tests/nat/mcp/test_mcp_auth_provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py
📚 Learning: 2025-09-17T05:34:04.696Z
Learnt from: AnuradhaKaruppiah
PR: NVIDIA/NeMo-Agent-Toolkit#752
File: packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py:0-0
Timestamp: 2025-09-17T05:34:04.696Z
Learning: The user AnuradhaKaruppiah confirmed that request copying is not needed in the AuthAdapter.async_auth_flow method in packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py, suggesting that httpx handles request mutation safely or there are no concurrent usage concerns for this implementation.
Applied to files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py
📚 Learning: 2025-09-17T05:19:15.641Z
Learnt from: AnuradhaKaruppiah
PR: NVIDIA/NeMo-Agent-Toolkit#752
File: src/nat/front_ends/mcp/mcp_front_end_config.py:45-48
Timestamp: 2025-09-17T05:19:15.641Z
Learning: In MCPFrontEndConfig, the auth_server_url field validation is handled by a model_validator that ensures auth_server_url is provided when require_auth=True, using direct ValidationError raising.
Applied to files:
src/nat/front_ends/mcp/mcp_front_end_config.py
🧬 Code graph analysis (2)
tests/nat/mcp/test_mcp_auth_provider.py (3)
src/nat/data_models/authentication.py (4)
AuthReason(234-239)AuthRequest(242-249)AuthResult(180-231)BearerTokenCred(158-165)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.py (19)
DiscoverOAuth2Endpoints(51-196)DynamicClientRegistration(199-248)MCPOAuth2Provider(251-367)OAuth2Credentials(45-48)OAuth2Endpoints(38-42)discover(65-114)_extract_from_www_authenticate_header(122-135)_authorization_base_url(117-120)_authorization_base_url(205-208)_build_path_aware_discovery_urls(180-192)register(210-248)authenticate(268-298)_effective_scopes(327-331)_perform_oauth2_flow(355-367)_fetch_pr_issuer(137-150)_discover_via_issuer_or_base(152-178)scopes_supported(194-196)_build_oauth2_delegate(333-353)_discover_and_register(300-325)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider_config.py (1)
MCPOAuth2ProviderConfig(24-76)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py (4)
src/nat/authentication/interfaces.py (4)
AuthProviderBase(29-69)config(45-54)authenticate(57-69)authenticate(82-93)src/nat/data_models/authentication.py (3)
AuthReason(234-239)AuthRequest(242-249)BearerTokenCred(158-165)packages/nvidia_nat_mcp/src/nat/plugins/mcp/utils.py (1)
model_from_mcp_schema(24-95)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_provider.py (1)
authenticate(268-298)
🪛 Ruff (0.12.2)
tests/nat/mcp/test_mcp_auth_provider.py
56-56: Possible hardcoded password assigned to argument: "client_secret"
(S106)
67-67: Possible hardcoded password assigned to argument: "token_url"
(S106)
77-77: Possible hardcoded password assigned to argument: "client_secret"
(S106)
112-112: Possible hardcoded password assigned to argument: "token_url"
(S106)
137-137: Possible hardcoded password assigned to argument: "token_url"
(S106)
156-156: Possible hardcoded password assigned to argument: "token_url"
(S106)
247-247: Possible hardcoded password assigned to: "client_secret"
(S105)
255-255: Possible hardcoded password assigned to argument: "token_url"
(S106)
327-327: Possible hardcoded password assigned to argument: "token_url"
(S106)
334-334: Possible hardcoded password assigned to argument: "client_secret"
(S106)
410-410: Possible hardcoded password assigned to argument: "client_secret"
(S106)
654-654: Possible hardcoded password assigned to: "client_secret"
(S105)
675-675: Possible hardcoded password assigned to: "token_endpoint_auth_method"
(S105)
684-684: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
700-700: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
717-717: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
724-724: Possible hardcoded password assigned to: "token_endpoint_auth_method"
(S105)
735-735: Possible hardcoded password assigned to argument: "token_url"
(S106)
742-742: Possible hardcoded password assigned to argument: "client_secret"
(S106)
761-761: Possible hardcoded password assigned to argument: "token_url"
(S106)
771-771: Possible hardcoded password assigned to: "client_secret"
(S105)
781-781: Possible hardcoded password assigned to argument: "token_url"
(S106)
788-788: Possible hardcoded password assigned to argument: "client_secret"
(S106)
845-845: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py
66-66: Do not catch blind exception: Exception
(BLE001)
79-79: Do not catch blind exception: Exception
(BLE001)
120-120: Do not catch blind exception: Exception
(BLE001)
138-138: Avoid specifying long messages outside the exception class
(TRY003)
168-168: Avoid specifying long messages outside the exception class
(TRY003)
419-419: Avoid specifying long messages outside the exception class
(TRY003)
src/nat/front_ends/mcp/mcp_front_end_config.py
53-53: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (1)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py (1)
352-356: LGTM: wires httpx.Auth into streamable-http correctly.Auth adapter is correctly passed to the transport; context handling is sound.
|
/merge |
This PR implements MCP client authentication changes (part 2), enabling OAuth2 authentication for protected MCP servers. First part of the auth changes were merged in #752 - Enhanced MCP client authentication with OAuth2 support and session-based authentication for WebSocket connections - Removed unused AuthReason and AuthRequest classes from authentication models - Added comprehensive CLI support for MCP authentication with default parameter handling **Documentation** I have removed the temporary doc `mcp-auth.md`. I will created a new guide for MCP Auth in a subsequent PR. auth_provider.authenticate API has been extended to include a kwargs making this a `breaking` change. Note: The authenticate API continues to remain experimental and may change in subsequent PRs. ## 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. ## Summary by CodeRabbit * **New Features** * OAuth2 support for MCP CLI/client (--auth, --auth-redirect-uri, --auth-user-id, --auth-scopes) with browser-based authorization; new simple_auth_mcp example project demonstrating protected MCP servers. * Session-aware per-session tool calls and increased default tool-call timeout (60s). * **Documentation** * Added “Using Protected MCP Servers” guide; removed prior experimental MCP auth planning doc. * **Tests** * Updated tests for response-driven discovery and OAuth2/session-aware flows. * **Breaking Changes** * Removed legacy auth models (AuthReason, AuthRequest). Authors: - Anuradha Karuppiah (https://github.com/AnuradhaKaruppiah) Approvers: - David Gardner (https://github.com/dagardner-nv) - Yuchen Zhang (https://github.com/yczhang-nv) - Zhongxuan (Daniel) Wang (https://github.com/zhongxuanwang-nv) URL: #854
3 participants
Description
MCP Auth support is a three party arch that involves a handshake between the MCP client, the MCP Server and an external AuthServer.
MCP Auth Support will be added in three PRs/phases:
mcp_oauth2auth provider.TokenVerifier.Summary:
This PR brings auth support for streamable-http MCP clients via:
This PR focuses on the MCP client side implementation with the MCP server using a stub token verifier to validate the e2e protected MCP flow. The MCP Server changes to enable TokenVerification with Auth Server Introspection are in a separate PR.
Other changes:
Doc:
MCP auth documentation is in progress and will be re-written when all relevant PRs are merged
Dependency updates
MCP packages updated to 1.14 (from 1.13)
By Submitting this PR I confirm:
Summary by CodeRabbit
New Features
Documentation
Tests
Chores