Skip to content

Conversation

@AnuradhaKaruppiah
Copy link
Contributor

Description

Closes

By Submitting this PR I confirm:

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

Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
@AnuradhaKaruppiah AnuradhaKaruppiah self-assigned this Sep 30, 2025
@coderabbitai
Copy link

coderabbitai bot commented Sep 30, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@AnuradhaKaruppiah AnuradhaKaruppiah added doc Improvements or additions to documentation non-breaking Non-breaking change labels Sep 30, 2025
Signed-off-by: Anuradha Karuppiah <[email protected]>
@AnuradhaKaruppiah AnuradhaKaruppiah changed the base branch from develop to release/1.3 September 30, 2025 17:13
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
Signed-off-by: Anuradha Karuppiah <[email protected]>
@AnuradhaKaruppiah AnuradhaKaruppiah marked this pull request as ready for review September 30, 2025 18:28
@AnuradhaKaruppiah AnuradhaKaruppiah requested a review from a team as a code owner September 30, 2025 18:28
@dagardner-nv dagardner-nv deleted the branch NVIDIA:release/1.3 September 30, 2025 18:57
@AnuradhaKaruppiah
Copy link
Contributor Author

@CodeRabbit review full

@coderabbitai
Copy link

coderabbitai bot commented Sep 30, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

# Get PR details
echo "=== PR Details ==="
gh pr view 877 --json title,body,author,headRefName,baseRefName,additions,deletions,files

echo -e "\n=== Files Changed ==="
gh pr diff 877 --name-only

echo -e "\n=== PR Diff ==="
gh pr diff 877

Length of output: 15421


@AnuradhaKaruppiah, I'll provide a comprehensive review of this MCP authentication documentation PR.

Overview

This PR adds excellent comprehensive documentation for MCP OAuth2 authentication in the NeMo Agent toolkit. The structure is logical and covers both conceptual and practical aspects well.

Strengths

✅ Clear progression from concepts to configuration to examples
✅ Well-structured with appropriate headings and code blocks
✅ Includes both single-user and multi-user workflow examples
✅ Good use of Mermaid diagrams to visualize authentication flows
✅ Security considerations and troubleshooting sections included
✅ Proper cross-referencing with existing MCP documentation

Issues and Suggestions

1. Empty Placeholder File

📁 docs/examples/MCP/simple_auth_mcp/README.md is completely empty. This should either:

  • Be removed if not needed yet
  • Contain actual content
  • Have a comment explaining it's a placeholder

2. Documentation Location Inconsistency

The example README at examples/MCP/simple_auth_mcp/README.md references:

It is recommended to read the [MCP Authentication](../../../docs/source/workflows/mcp/mcp-auth.md) documentation first.

However, there's also an empty file at docs/examples/MCP/simple_auth_mcp/README.md. Should these be:

  • The same file (symlink)?
  • Different files with different purposes?
  • Only one of them?

3. Minor Typos and Grammar

Line 41 in mcp-auth.md:

- `default_user_id`: The user ID for setting discovering and adding tools

Should be:

- `default_user_id`: The user ID for discovering and adding tools

4. Configuration Example Inconsistency

In the first configuration example (line 31-37), you use:

authentication:
  auth_provider_mcp:
    _type: mcp_oauth2

But in the detailed example (line 88-94), you use:

authentication:
  mcp_oauth2_jira:
    _type: mcp_oauth2

Consider clarifying the naming convention or using consistent names throughout to avoid confusion.

5. Mermaid Diagram Enhancement

The multi-user diagram could be clearer. Consider adding styling or notes to distinguish the default user flow from UI user flows:

flowchart LR
  U0[User<br/>default-user-id<br/><i>setup only</i>] --> H2[MCP Host<br/>NAT Workflow]
  U1[User<br/>UI-User-1] --> H2
  U2[User<br/>UI-User-2] --> H2
Loading

6. Missing Information

The documentation could benefit from:

  • Token refresh behavior: How are tokens refreshed? What happens when they expire?
  • Concurrent user limits: Are there any limits on concurrent authenticated users?
  • Token storage: Where are tokens cached? What are the security implications?

7. Example Configuration Path

Line 77 references:

The MCP Authentication Example Workflow, `examples/MCP/simple_auth_mcp/README.md`

But that file doesn't contain the workflow itself. Should this reference a YAML config file instead? Or clarify what users should expect to find there?

8. Environment Variable Documentation

Lines 51-59 document environment variables, but it would be helpful to also mention:

  • What happens if NAT_USER_ID is not set (you mention it defaults to server_url, but this could be more prominent)
  • Whether these can be set in a .env file
  • Whether they can be overridden in the YAML config

9. Troubleshooting Section Enhancement

The troubleshooting section (lines 169-176) could include:

  • How to clear cached credentials
  • How to re-authenticate a user
  • Common OAuth2 error messages and their meanings

Technical Accuracy

✅ OAuth2 flow description is correct
✅ RFC references are accurate
✅ Configuration syntax matches NeMo Agent toolkit patterns
✅ Transport limitations are correctly stated

Formatting

The documentation follows proper Markdown formatting with one minor suggestion:

Line 170:

1. If setup fails, the user identified by `default_user_id` did not complete the authentication flow through the pop-up UI, or the user did not authorize the workflow to access the MCP server.

This could be split into sub-bullets for better readability:

1. **Setup fails** - This can happen if:
   - The user identified by `default_user_id` did not complete the authentication flow through the pop-up UI, or
   - The user did not authorize the workflow to access the MCP server

Summary

This is solid documentation that will be very helpful for users implementing MCP authentication. The main action items are:

  1. ⚠️ Critical: Address the empty docs/examples/MCP/simple_auth_mcp/README.md file
  2. Fix the typo on line 41 ("setting discovering" → "discovering")
  3. Consider the enhancement suggestions for clarity

Would you like me to help generate any additional content for the example README files or provide more detailed configuration examples?

Copy link
Member

@willkill07 willkill07 left a comment

Choose a reason for hiding this comment

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

Just one nit! otherwise LGTM

Signed-off-by: Anuradha Karuppiah <[email protected]>
@AnuradhaKaruppiah
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit d3e3688 into NVIDIA:release/1.3 Sep 30, 2025
17 checks passed
@AnuradhaKaruppiah AnuradhaKaruppiah deleted the ak-auth-doc branch October 1, 2025 21:39
nouraellm pushed a commit to nouraellm/NeMo-Agent-Toolkit that referenced this pull request Oct 3, 2025
Closes

## 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.

Authors:
  - Anuradha Karuppiah (https://github.com/AnuradhaKaruppiah)

Approvers:
  - Yuchen Zhang (https://github.com/yczhang-nv)
  - Eric Evans II (https://github.com/ericevans-nv)
  - Will Killian (https://github.com/willkill07)

URL: NVIDIA#877
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc Improvements or additions to documentation non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants