Fix MCP auth redirect handling for remote-ssh and update docs#1023
Conversation
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
WalkthroughReplace hardcoded/local redirect URIs in docs and examples with environment-variable templates ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Handler as MCPAuthenticationFlowHandler
participant Parser as URI Parser
participant RedirectServer as Redirect Server
rect rgb(245,248,255)
note over Handler,Parser: Determine redirect binding from config (NAT_REDIRECT_URI / redirect_uri)
Client->>Handler: initiate auth flow
Handler->>Parser: parse cfg.redirect_uri
Parser-->>Handler: host, port (or defaults)
Handler->>Handler: set _redirect_host / _redirect_port
end
rect rgb(240,255,240)
note over Handler,RedirectServer: Start redirect server bound to host:port
Handler->>Handler: _start_redirect_server()
Handler->>RedirectServer: bind to host:port
RedirectServer-->>Handler: ready / error
end
rect rgb(255,250,230)
note over Handler,RedirectServer: OAuth2 redirect & token exchange
Handler->>Client: provide auth URL (includes redirect_uri)
Client->>RedirectServer: callback with auth code
RedirectServer-->>Handler: deliver auth code
Handler->>Handler: exchange code for tokens
Handler->>RedirectServer: shutdown
RedirectServer-->>Handler: stopped
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py (1)
93-98: Moveurlparseimport to module level.The import statement is placed inside the method, which is not idiomatic Python. Module-level imports improve readability and follow PEP 8 guidelines.
Apply this diff to move the import to the top of the file:
import asyncio import logging import secrets +from urllib.parse import urlparse import webbrowser import pkceAnd remove the inline import:
- # Extract host and port from redirect_uri for callback server - from urllib.parse import urlparse parsed_uri = urlparse(str(cfg.redirect_uri))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/source/workflows/mcp/mcp-auth.md(4 hunks)examples/MCP/simple_auth_mcp/configs/config-mcp-auth-jira.yml(1 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
**/*.{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_auth_mcp/configs/config-mcp-auth-jira.yml
**/*.{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:
examples/MCP/simple_auth_mcp/configs/config-mcp-auth-jira.ymlpackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py
**/configs/**
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Configuration files consumed by code must be stored next to that code in a configs/ folder
Files:
examples/MCP/simple_auth_mcp/configs/config-mcp-auth-jira.yml
**/*
⚙️ 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:
examples/MCP/simple_auth_mcp/configs/config-mcp-auth-jira.ymlpackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.pydocs/source/workflows/mcp/mcp-auth.md
examples/**/*
⚙️ CodeRabbit configuration file
examples/**/*: - This directory contains example code and usage scenarios for the toolkit, at a minimum an example should
contain a README.md or file README.ipynb.
- If an example contains Python code, it should be placed in a subdirectory named
src/and should
contain 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_auth_mcp/configs/config-mcp-auth-jira.yml
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.py: Programmatic use: create TestLLMConfig(response_seq=[...], delay_ms=...), add with builder.add_llm("", cfg).
When retrieving the test LLM wrapper, use builder.get_llm(name, wrapper_type=LLMFrameworkEnum.) and call the framework’s method (e.g., ainvoke, achat, call).
**/*.py: In code comments/identifiers use NAT abbreviations as specified: nat for API namespace/CLI, nvidia-nat for package name, NAT for env var prefixes; do not use these abbreviations in documentation
Follow PEP 20 and PEP 8; run yapf with column_limit=120; use 4-space indentation; end files with a single trailing newline
Run ruff check --fix as linter (not formatter) using pyproject.toml config; fix warnings unless explicitly ignored
Respect naming: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise and ending with a period; surround code entities with backticks
Validate and sanitize all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile or mprof before optimizing; cache expensive computations with functools.lru_cache or external cache; leverage NumPy vectorized operations when beneficial
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Importable Python code inside packages must live under packages//src/
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All public APIs must have Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.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_flow_handler.py
docs/source/**/*.md
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
docs/source/**/*.md: Use the official naming throughout documentation: first use “NVIDIA NeMo Agent toolkit”, subsequent “NeMo Agent toolkit”; never use deprecated names (Agent Intelligence toolkit, aiqtoolkit, AgentIQ, AIQ/aiq)
Documentation sources are Markdown files under docs/source; images belong in docs/source/_static
Keep docs in sync with code; documentation pipeline must pass Sphinx and link checks; avoid TODOs/FIXMEs/placeholders; avoid offensive/outdated terms; ensure spelling correctness
Do not use words listed in ci/vale/styles/config/vocabularies/nat/reject.txt; accepted terms in accept.txt are allowed
Files:
docs/source/workflows/mcp/mcp-auth.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-auth.md
🧬 Code graph analysis (1)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py (5)
tests/nat/authentication/test_oauth_exchanger.py (1)
cfg(50-57)src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (1)
redirect_uri(1021-1060)src/nat/front_ends/console/authentication_flow_handler.py (1)
_start_redirect_server(256-271)src/nat/front_ends/fastapi/fastapi_front_end_controller.py (1)
_FastApiFrontEndController(26-68)tests/nat/front_ends/auth_flow_handlers/mock_oauth2_server.py (1)
start_server(150-165)
🪛 Ruff (0.14.0)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py
167-167: Abstract raise to an inner function
(TRY301)
167-167: Avoid specifying long messages outside the exception class
(TRY003)
171-172: Store a reference to the return value of asyncio.create_task
(RUF006)
177-177: Unused noqa directive (unused: BLE001)
Remove unused noqa directive
(RUF100)
178-179: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (5)
examples/MCP/simple_auth_mcp/configs/config-mcp-auth-jira.yml (1)
44-44: LGTM! Dynamic redirect_uri enables remote server scenarios.The change from hardcoded
localhost:8000to environment variable templates with sensible defaults (localhost:8000) maintains backward compatibility while enabling remote-ssh workflows.packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py (1)
57-58: LGTM! Sensible defaults for redirect host and port.The default values preserve backward compatibility and will be overridden dynamically from the configuration.
docs/source/workflows/mcp/mcp-auth.md (3)
32-38: LGTM! Sample configuration clearly demonstrates dynamic redirect_uri.The configuration example accurately reflects the environment variable template usage with appropriate defaults, aligning with the implementation changes.
54-55: LGTM! Environment variable documentation is clear.The new environment variables are well-documented with appropriate defaults that match the implementation.
182-214: Documentation is accurate and well-verified.The claim that the
/auth/redirectendpoint is automatically registered is correct. Code verification confirms:
- The endpoint path is defined as a configurable field with default value
"/auth/redirect"infastapi_front_end_config.py- The route is registered via
app.add_api_route()infastapi_front_end_plugin_worker.py:1062-1068- Tests in
test_oauth_security_tests.pyconfirm the endpoint exists and functions as expectedThe documentation accurately reflects the implementation and provides clear, practical guidance for remote server deployments.
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py
Outdated
Show resolved
Hide resolved
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
|
/ok to test 4980b45 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py (1)
57-59: Good: explicit state for host/port and server task.Defaults, type hints, and persisting
_server_taskaddress lifecycle and RUF006. No issues.
🧹 Nitpick comments (4)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py (4)
163-169: Narrow try scope (TRY301) and pre-check outside except block.Move the
_redirect_appprecondition outside the try to avoid catching and re-wrapping your own validation error.Apply this diff:
- # If the server is already running, do nothing - if self._server_controller: - return - try: - if not self._redirect_app: - raise RuntimeError("Redirect app not built.") + # If the server is already running, do nothing + if self._server_controller: + return + if not self._redirect_app: + raise RuntimeError("Redirect app not built.") + try:As per coding guidelines.
178-180: Use logger.error and bareraiseto preserve stack; avoid long inline messages (TRY003).Log the context, then re-raise the original exception to keep its type/trace intact.
Apply this diff:
- except Exception as exc: - raise RuntimeError( - f"Failed to start MCP redirect server on {self._redirect_host}:{self._redirect_port}: {exc}") from exc + except Exception as exc: + logger.error("Failed to start MCP redirect server on %s:%d: %s", + self._redirect_host, self._redirect_port, exc) + raiseAs per coding guidelines.
172-177: Optional: await controller.start_server instead of creating a wrapper task.
_FastApiFrontEndController.start_serveralready backgrounds uvicorn internally; awaiting it returns quickly and removes the need for_server_taskplus the extra sleep.Apply this diff:
- self._server_task = asyncio.create_task( - self._server_controller.start_server(host=self._redirect_host, port=self._redirect_port)) - logger.debug("MCP redirect server starting on %s:%d", self._redirect_host, self._redirect_port) - - # Give the server a moment to bind sockets before we return - await asyncio.sleep(0.3) + await self._server_controller.start_server(host=self._redirect_host, port=self._redirect_port) + logger.debug("MCP redirect server starting on %s:%d", self._redirect_host, self._redirect_port)If you take this path, you can remove
_server_taskaltogether. Please run existing MCP auth tests to confirm no regressions.
140-143: Be explicit: catchasyncio.TimeoutError.
asyncio.wait_forraisesasyncio.TimeoutError. Being explicit improves clarity and static analysis.Apply this diff:
- except TimeoutError as exc: + except asyncio.TimeoutError as exc: logger.error("MCP authentication timed out") raise RuntimeError(f"MCP authentication timed out ({timeout} seconds). Please try again.") from exc
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{py,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.{py,yaml,yml}: Configure response_seq as a list of strings; values cycle per call, and [] yields an empty string.
Configure delay_ms to inject per-call artificial latency in milliseconds for nat_test_llm.
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.py: Programmatic use: create TestLLMConfig(response_seq=[...], delay_ms=...), add with builder.add_llm("", cfg).
When retrieving the test LLM wrapper, use builder.get_llm(name, wrapper_type=LLMFrameworkEnum.) and call the framework’s method (e.g., ainvoke, achat, call).
**/*.py: In code comments/identifiers use NAT abbreviations as specified: nat for API namespace/CLI, nvidia-nat for package name, NAT for env var prefixes; do not use these abbreviations in documentation
Follow PEP 20 and PEP 8; run yapf with column_limit=120; use 4-space indentation; end files with a single trailing newline
Run ruff check --fix as linter (not formatter) using pyproject.toml config; fix warnings unless explicitly ignored
Respect naming: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise and ending with a period; surround code entities with backticks
Validate and sanitize all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile or mprof before optimizing; cache expensive computations with functools.lru_cache or external cache; leverage NumPy vectorized operations when beneficial
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Importable Python code inside packages must live under packages//src/
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All public APIs must have Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.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/auth_flow_handler.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_flow_handler.py
🧬 Code graph analysis (1)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py (4)
tests/nat/authentication/test_oauth_exchanger.py (1)
cfg(50-57)src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (1)
redirect_uri(1021-1060)src/nat/front_ends/console/authentication_flow_handler.py (1)
_start_redirect_server(256-271)src/nat/front_ends/fastapi/fastapi_front_end_controller.py (1)
_FastApiFrontEndController(26-68)
🪛 Ruff (0.14.0)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py
168-168: Abstract raise to an inner function
(TRY301)
168-168: Avoid specifying long messages outside the exception class
(TRY003)
179-180: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (1)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py (1)
127-129: Sequence and locking look correct.Building the app once, then starting the server under
_server_lockand registering the flow is sound.
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py (1)
176-181: Catchasyncio.TimeoutErrorexplicitly.
asyncio.wait_forraisesasyncio.TimeoutError. Catching it directly is clearer and future-proof.- except TimeoutError as exc: + except asyncio.TimeoutError as exc: logger.error("MCP authentication timed out") raise RuntimeError(f"MCP authentication timed out ({timeout} seconds). Please try again.") from exc
🧹 Nitpick comments (2)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py (2)
169-175: Harden browser launch and make timeout configurable.Headless sessions may not open a browser; also avoid magic 300s. Return the URL if open fails and allow env/config override.
- logger.info("MCP authentication: Your browser has been opened for authentication.") - logger.info("This will authenticate you with the MCP server for tool discovery.") - webbrowser.open(auth_url) + opened = webbrowser.open(auth_url) + if opened: + logger.info("Opened browser for MCP authentication.") + else: + logger.warning("Could not open a browser automatically. Please navigate to: %s", auth_url) + # Consider also printing to stdout if appropriate for the front end. ... - timeout = 300 + timeout = int(getattr(cfg, "authorization_timeout_seconds", 0) or + int(os.getenv("NAT_AUTH_TIMEOUT_SECONDS", "300")))
211-216: Replace fixed sleep with readiness wait.
await asyncio.sleep(0.3)is brittle. Wait until uvicorn reportsstartedor the socket is connectable, with a short deadline.- # Give the server a moment to bind sockets before we return - await asyncio.sleep(0.3) + # Wait for the server to bind (max ~3s) + start = asyncio.get_running_loop().time() + while True: + server = getattr(self._server_controller, "_server", None) + if server and getattr(server, "started", False): + break + if asyncio.get_running_loop().time() - start > 3.0: + raise RuntimeError("Redirect server did not report ready within 3s") + await asyncio.sleep(0.05)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{py,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.{py,yaml,yml}: Configure response_seq as a list of strings; values cycle per call, and [] yields an empty string.
Configure delay_ms to inject per-call artificial latency in milliseconds for nat_test_llm.
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.py: Programmatic use: create TestLLMConfig(response_seq=[...], delay_ms=...), add with builder.add_llm("", cfg).
When retrieving the test LLM wrapper, use builder.get_llm(name, wrapper_type=LLMFrameworkEnum.) and call the framework’s method (e.g., ainvoke, achat, call).
**/*.py: In code comments/identifiers use NAT abbreviations as specified: nat for API namespace/CLI, nvidia-nat for package name, NAT for env var prefixes; do not use these abbreviations in documentation
Follow PEP 20 and PEP 8; run yapf with column_limit=120; use 4-space indentation; end files with a single trailing newline
Run ruff check --fix as linter (not formatter) using pyproject.toml config; fix warnings unless explicitly ignored
Respect naming: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise and ending with a period; surround code entities with backticks
Validate and sanitize all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile or mprof before optimizing; cache expensive computations with functools.lru_cache or external cache; leverage NumPy vectorized operations when beneficial
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Importable Python code inside packages must live under packages//src/
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All public APIs must have Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.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/auth_flow_handler.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_flow_handler.py
🧬 Code graph analysis (1)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py (5)
tests/nat/authentication/test_oauth_exchanger.py (1)
cfg(50-57)src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (1)
redirect_uri(1021-1060)src/nat/front_ends/console/authentication_flow_handler.py (1)
_start_redirect_server(256-271)src/nat/front_ends/fastapi/fastapi_front_end_controller.py (1)
_FastApiFrontEndController(26-68)tests/nat/front_ends/auth_flow_handlers/mock_oauth2_server.py (1)
start_server(150-165)
🪛 Ruff (0.14.0)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py
207-207: Abstract raise to an inner function
(TRY301)
207-207: Avoid specifying long messages outside the exception class
(TRY003)
218-219: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (1)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py (1)
57-59: Good: per-instance host/port and stored task reference.Defaults are sensible, types are annotated, and storing
self._server_taskresolves the RUF006 concern.
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py (2)
157-163: Catch asyncio.TimeoutError, not builtin TimeoutError.asyncio.wait_for raises asyncio.TimeoutError; the current except may not catch it reliably across Python versions.
Apply:
- except TimeoutError as exc: + except asyncio.TimeoutError as exc: logger.error("MCP authentication timed out") raise RuntimeError(f"MCP authentication timed out ({timeout} seconds). Please try again.") from exc
128-141: Remove code_verifier from authorization request and add code_challenge_method for PKCE RFC 7636 compliance.The current implementation incorrectly sends
code_verifierto the authorization endpoint. Per RFC 7636, the verifier must only be sent to the token endpoint; the authorization request should includecode_challengeandcode_challenge_method. The parent class correctly usesflow_state.verifierduring token exchange (line 238 ofConsoleAuthenticationFlowHandler), so the fix only requires adjusting the authorization URL generation at lines 135–140.Apply the suggested diff:
auth_url, _ = client.create_authorization_url( cfg.authorization_url, state=state, - code_verifier=flow_state.verifier if cfg.use_pkce else None, code_challenge=flow_state.challenge if cfg.use_pkce else None, + code_challenge_method="S256" if cfg.use_pkce else None, **(cfg.authorization_kwargs or {}) )
🧹 Nitpick comments (2)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py (2)
94-120: Redirect URI validation: solid; add broader https warning.Validation looks good (scheme, host, safe default port). Recommend warning on any https scheme since the local server is plain HTTP, not only when the port is missing.
Apply:
- if scheme == "https" and parsed_uri.port is None: - logger.warning( - "redirect_uri uses https without an explicit port; binding to %d (plain HTTP). " - "Terminate TLS at a reverse proxy and forward to this port.", port) + if scheme == "https": + logger.warning( + "redirect_uri uses https; the local MCP redirect server does not terminate TLS. " + "Bind will be plain HTTP on %s:%d. Terminate TLS at a reverse proxy and forward to this port.", + host, port)
176-207: Surface startup failures and log before re-raise.Poll is OK, but if uvicorn fails early (e.g., EADDRINUSE), the background task exception is lost and you only hit the 10s timeout. Also, per guidelines, log with logger.error() when re-raising.
As per coding guidelines.
try: if not self._redirect_app: raise RuntimeError("Redirect app not built.") self._server_controller = _FastApiFrontEndController(self._redirect_app) self._server_task = asyncio.create_task( self._server_controller.start_server(host=self._redirect_host, port=self._redirect_port)) logger.debug("MCP redirect server starting on %s:%d", self._redirect_host, self._redirect_port) # Wait for the server to bind (max ~10s) start = asyncio.get_running_loop().time() while True: server = getattr(self._server_controller, "_server", None) + bg_task = getattr(self._server_controller, "_server_background_task", None) + if bg_task and bg_task.done(): + exc2 = bg_task.exception() + if exc2: + raise RuntimeError("Redirect server failed to start") from exc2 if server and getattr(server, "started", False): break if asyncio.get_running_loop().time() - start > 10: raise RuntimeError("Redirect server did not report ready within 10s") await asyncio.sleep(0.1) except Exception as exc: - raise RuntimeError( - f"Failed to start MCP redirect server on {self._redirect_host}:{self._redirect_port}: {exc}") from exc + logger.error("Failed to start MCP redirect server on %s:%d: %s", + self._redirect_host, self._redirect_port, str(exc)) + raise RuntimeError( + f"Failed to start MCP redirect server on {self._redirect_host}:{self._redirect_port}: {exc}") from exc
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{py,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.{py,yaml,yml}: Configure response_seq as a list of strings; values cycle per call, and [] yields an empty string.
Configure delay_ms to inject per-call artificial latency in milliseconds for nat_test_llm.
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.py: Programmatic use: create TestLLMConfig(response_seq=[...], delay_ms=...), add with builder.add_llm("", cfg).
When retrieving the test LLM wrapper, use builder.get_llm(name, wrapper_type=LLMFrameworkEnum.) and call the framework’s method (e.g., ainvoke, achat, call).
**/*.py: In code comments/identifiers use NAT abbreviations as specified: nat for API namespace/CLI, nvidia-nat for package name, NAT for env var prefixes; do not use these abbreviations in documentation
Follow PEP 20 and PEP 8; run yapf with column_limit=120; use 4-space indentation; end files with a single trailing newline
Run ruff check --fix as linter (not formatter) using pyproject.toml config; fix warnings unless explicitly ignored
Respect naming: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise and ending with a period; surround code entities with backticks
Validate and sanitize all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile or mprof before optimizing; cache expensive computations with functools.lru_cache or external cache; leverage NumPy vectorized operations when beneficial
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Importable Python code inside packages must live under packages//src/
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All public APIs must have Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.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/auth_flow_handler.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_flow_handler.py
🧬 Code graph analysis (1)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py (4)
tests/nat/authentication/test_oauth_exchanger.py (1)
cfg(50-57)src/nat/front_ends/console/authentication_flow_handler.py (1)
_start_redirect_server(256-271)src/nat/front_ends/fastapi/fastapi_front_end_controller.py (1)
_FastApiFrontEndController(26-68)tests/nat/front_ends/auth_flow_handlers/mock_oauth2_server.py (1)
start_server(150-165)
🪛 Ruff (0.14.0)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py
101-101: Avoid specifying long messages outside the exception class
(TRY003)
105-105: Avoid specifying long messages outside the exception class
(TRY003)
110-110: Avoid specifying long messages outside the exception class
(TRY003)
188-188: Abstract raise to an inner function
(TRY301)
188-188: Avoid specifying long messages outside the exception class
(TRY003)
203-203: Abstract raise to an inner function
(TRY301)
203-203: Avoid specifying long messages outside the exception class
(TRY003)
206-207: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (2)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py (2)
57-59: Init fields and task reference — LGTM.Good defaults and storing the task reference addresses RUF006 and supports lifecycle mgmt.
94-120: Test fixtures appropriately exercise missing-port code path.The script found 25+ instances of
redirect_uriwithout explicit ports, all in test fixtures acrosstest_mcp_token_storage.py,test_mcp_auth_provider.py,test_mcp_auth_timeout.py, andtest_oauth_security_tests.py. These test cases deliberately use URLs likehttps://example.com/callbackto verify that the code correctly handles missing port specifications with the default fallback (port 8000) and warning message already implemented in lines 108–112. No regressions detected in production configuration or documentation.
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/source/workflows/mcp/mcp-auth.md (1)
41-41: Clarify redirect_uri scope.The description on line 41 now correctly states the redirect URI "must match the address where your NAT server is accessible from your browser." This is an improvement. However, consider adding a cross-reference to the new "Running the Workflow on a Remote Server" section (lines 182–231) to help users understand when and how to customize this value.
Consider updating line 41 to include a link or reference:
- `redirect_uri`: The redirect URI for the OAuth2 flow. This must match the address where your NAT server is accessible from your browser. + `redirect_uri`: The redirect URI for the OAuth2 flow. This must match the address where your NAT server is accessible from your browser. For remote servers or production deployments, see [Running the Workflow on a Remote Server](#running-the-workflow-on-a-remote-server).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/source/workflows/mcp/mcp-auth.md(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
docs/source/**/*.md
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
docs/source/**/*.md: Use the official naming throughout documentation: first use “NVIDIA NeMo Agent toolkit”, subsequent “NeMo Agent toolkit”; never use deprecated names (Agent Intelligence toolkit, aiqtoolkit, AgentIQ, AIQ/aiq)
Documentation sources are Markdown files under docs/source; images belong in docs/source/_static
Keep docs in sync with code; documentation pipeline must pass Sphinx and link checks; avoid TODOs/FIXMEs/placeholders; avoid offensive/outdated terms; ensure spelling correctness
Do not use words listed in ci/vale/styles/config/vocabularies/nat/reject.txt; accepted terms in accept.txt are allowed
Files:
docs/source/workflows/mcp/mcp-auth.md
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
docs/source/workflows/mcp/mcp-auth.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-auth.md
🔇 Additional comments (2)
docs/source/workflows/mcp/mcp-auth.md (2)
54-54: Document NAT_REDIRECT_URI default clearly.Line 54 documents the
NAT_REDIRECT_URIenvironment variable and its default behavior well. The note about port 8000 binding is helpful. Confirm that the stated behavior ("the server will bind to port 8000 by default") aligns with the implementation in the authentication handler to avoid discrepancies between documentation and runtime behavior.Verify that when
redirect_uriomits an explicit port, the NAT authentication handler indeed binds to port 8000. This should be confirmed against the implementation inpackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.pymentioned in the enriched summary.
182-231: New remote server section is well-structured and comprehensive.The "Running the Workflow on a Remote Server" section effectively addresses the PR objective by explaining why
localhostfails for remote deployments, providing concrete examples, and documenting the port binding behavior and reverse proxy guidance.Key strengths:
- Clear explanation of the OAuth2 redirect failure scenario (lines 186–188)
- Concrete examples for remote and production setups (lines 193–200)
- Important note about port 8000 default binding for HTTPS (lines 203–205)
- Reverse proxy guidance for production (lines 229–231)
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
|
/ok to test c7e80c4 |
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
|
/ok to test ec4cd99 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py (1)
95-95: Consider moving import to top of file.The
urlparseimport is inside the method. While functional, PEP 8 recommends placing imports at the module level unless there's a specific reason for lazy loading.Apply this diff to move the import to the top:
import asyncio import logging import secrets +from urllib.parse import urlparse import webbrowserThen remove it from line 95:
# Extract and validate host and port from redirect_uri for callback server - from urllib.parse import urlparse parsed_uri = urlparse(str(cfg.redirect_uri))As per coding guidelines.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{py,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.{py,yaml,yml}: Configure response_seq as a list of strings; values cycle per call, and [] yields an empty string.
Configure delay_ms to inject per-call artificial latency in milliseconds for nat_test_llm.
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.py: Programmatic use: create TestLLMConfig(response_seq=[...], delay_ms=...), add with builder.add_llm("", cfg).
When retrieving the test LLM wrapper, use builder.get_llm(name, wrapper_type=LLMFrameworkEnum.) and call the framework’s method (e.g., ainvoke, achat, call).
**/*.py: In code comments/identifiers use NAT abbreviations as specified: nat for API namespace/CLI, nvidia-nat for package name, NAT for env var prefixes; do not use these abbreviations in documentation
Follow PEP 20 and PEP 8; run yapf with column_limit=120; use 4-space indentation; end files with a single trailing newline
Run ruff check --fix as linter (not formatter) using pyproject.toml config; fix warnings unless explicitly ignored
Respect naming: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise and ending with a period; surround code entities with backticks
Validate and sanitize all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile or mprof before optimizing; cache expensive computations with functools.lru_cache or external cache; leverage NumPy vectorized operations when beneficial
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Importable Python code inside packages must live under packages//src/
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All public APIs must have Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.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/auth_flow_handler.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_flow_handler.py
🧬 Code graph analysis (1)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py (5)
tests/nat/authentication/test_oauth_exchanger.py (1)
cfg(50-57)src/nat/front_ends/fastapi/fastapi_front_end_plugin_worker.py (1)
redirect_uri(1021-1060)src/nat/front_ends/console/authentication_flow_handler.py (1)
_start_redirect_server(256-271)src/nat/front_ends/fastapi/fastapi_front_end_controller.py (1)
_FastApiFrontEndController(26-68)tests/nat/front_ends/auth_flow_handlers/mock_oauth2_server.py (1)
start_server(150-165)
🪛 Ruff (0.14.0)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py
101-101: Avoid specifying long messages outside the exception class
(TRY003)
105-105: Avoid specifying long messages outside the exception class
(TRY003)
110-110: Avoid specifying long messages outside the exception class
(TRY003)
189-189: Abstract raise to an inner function
(TRY301)
189-189: Avoid specifying long messages outside the exception class
(TRY003)
204-204: Abstract raise to an inner function
(TRY301)
204-204: Avoid specifying long messages outside the exception class
(TRY003)
207-208: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (3)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/auth_flow_handler.py (3)
57-59: LGTM! Good initialization of redirect server configuration.The instance variables are properly typed with sensible defaults. This addresses the previous concern about storing the server task reference and enables per-instance redirect server configuration based on the redirect_uri.
94-121: Excellent validation logic that addresses previous concerns.The redirect_uri parsing and validation is comprehensive:
- Enforces http/https scheme
- Requires hostname to prevent silent failures
- Defaults to port 8000 (avoiding privileged ports 80/443)
- Validates port range with clear error messages
- Provides helpful warning for https without explicit port
This properly addresses the critical concerns from previous reviews about auto-binding to privileged ports and strict validation.
177-208: LGTM! Excellent override that addresses previous concerns.The implementation correctly:
- Uses configured
_redirect_hostand_redirect_portinstead of hardcodedlocalhost:8000- Stores the task reference in
self._server_task(addressing previous review comment)- Includes a proper readiness check with reasonable 10-second timeout
- Provides contextual error messages including the host/port being bound
- Follows Google-style docstring guidelines
This properly enables MCP authentication to work with custom redirect URIs for remote-ssh scenarios.
|
/merge |
…#1023) This PR includes: 1. Doc updates to clarify redirect_uri usage for remote-ssh 2. Example config update to use env variables that match the server_host and server_port 3. Fix for starting the initial auth callback using the configured host and port instead of the hardcoded localhost/port ## 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 * **Documentation** * Added guidance for running MCP OAuth2 workflows on remote servers, clarified redirect behavior, introduced NAT_REDIRECT_URI with shell export examples. * **Examples** * Updated sample configs to use template-based redirect URIs and adjusted default user ID patterns for remote deployments. * **New Features** * Authentication flow now supports per-instance redirect binding so redirect URIs are validated and bound correctly on remote hosts. Authors: - Anuradha Karuppiah (https://github.com/AnuradhaKaruppiah) Approvers: - Eric Evans II (https://github.com/ericevans-nv) URL: NVIDIA#1023
2 participants
Description
This PR includes:
By Submitting this PR I confirm:
Summary by CodeRabbit