feat: Support MCP Authentication - #194
Conversation
…d), run discover_mcp_tools in a loop Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
…mprovements Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
…-fabric into david-mcp-auth Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
…mprovements Signed-off-by: David Gardner <dagardner@nvidia.com>
…-fabric into david-mcp-auth Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
|
Fern docs preview: https://nvidia-preview-pull-request-194.docs.buildwithfern.com/nemo/fabric |
Signed-off-by: David Gardner <dagardner@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@adapters/claude/src/nemo_fabric_adapters/claude/adapter.py`:
- Around line 399-406: Update _authenticated_mcp_servers and the MCP login flow
to reject stdio servers that specify authentication, preventing their raw env
credentials from reaching _login_mcp_server or add-json argv. Add a regression
test covering a stdio server with both authentication and env, asserting
credential values are absent from argv.
In `@adapters/codex/src/nemo_fabric_adapters/codex/adapter.py`:
- Around line 467-473: Update _authenticate_mcp_servers to accept the invocation
timeout, and in its _login_mcp_server call set timeout to the minimum of that
value and oauth.authorization_timeout_seconds. Pass the existing
timeout_seconds(payload) result from invoke into _authenticate_mcp_servers,
preserving the per-server OAuth timeout as the upper bound.
- Around line 211-221: Update the custom-header handling in the Codex adapter to
expand environment variables across header values before calling
mcp_auth.normalize_custom_headers. Validate the expanded mapping so CR/LF
introduced by environment variables is rejected, then build
result[name]["http_headers"] from the validated headers without performing a
second expansion.
In `@adapters/common/src/nemo_fabric_adapters/common/mcp_auth.py`:
- Around line 513-590: Track each _LoopbackOAuthCallback or provider created by
create_mcp_oauth_provider at DeepAgentsRuntime scope, and update
DeepAgentsRuntime.stop to close every tracked callback by calling
close_reserved_socket() before awaiting close(). Ensure startup-failure cleanup
performs the same callback cleanup before propagating the failure, while
preserving normal OAuth authorization behavior.
In `@adapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.py`:
- Around line 306-314: Update the custom-header handling around
mcp_auth.normalize_custom_headers to expand environment variables in the header
mapping before normalization and validation. Then pass the expanded mapping to
normalize_custom_headers and use its validated result for connection["headers"],
preserving the existing McpAuthConfigError-to-AdapterConfigError conversion.
In `@adapters/hermes/src/nemo_fabric_adapters/hermes/adapter.py`:
- Around line 536-547: Update authenticate() to avoid replacing process-global
sys.stdin while the worker thread runs interactive OAuth. Isolate
discover_mcp_tools() input in a subprocess or otherwise coordinate lifecycle
reads so the main-thread lifecycle reader continues consuming real stdin and
stop messages cannot be lost during authentication.
- Around line 508-571: Update _authenticate_mcp_servers to execute every
synchronous get_mcp_status() and refresh_agent_mcp_tools() call through
asyncio.to_thread, including both status lookups and the final tool refresh,
while preserving their existing ordering and behavior.
In `@crates/fabric-core/src/config.rs`:
- Around line 1537-1566: Update the ServiceAccount validation branch in the
McpAuthenticationConfig match to require token_url to use HTTPS before accepting
the configuration. Preserve an explicit loopback exception only if the existing
configuration policy supports local development, and return invalid_config for
non-secure endpoints using the token_url field path.
- Around line 758-814: Update the OAuth2 and ServiceAccount variants of
McpAuthenticationConfig with serde unknown-field rejection, add regression tests
confirming extra authentication fields fail deserialization, and update both
authentication schema definitions to set additionalProperties: false so Rust,
the Python SDK, and CLI validation consistently reject unknown fields.
In
`@docs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-mcpserverconfig.mdx`:
- Line 24: Update the MCP server URL documentation comment in MCPServerConfig
within crates/fabric-core/src/config.rs to format transport=stdio as inline
code, then run just docs to regenerate the reference page; do not edit the
generated MDX directly.
In `@python/src/nemo_fabric/models.py`:
- Around line 426-428: Widen the authentication type annotations to accept
mappings alongside McpAuthenticationConfig and None. Update the authentication
field near McpConfig, the McpConfig.add_server signature, and
FabricConfig.add_mcp_server to use the same dual-form pattern as relay, with
dict[str, Any] included.
In `@tests/adapters/test_adapters_common_mcp_auth.py`:
- Around line 83-95: Update
test_resolve_client_secret_uses_named_environment_variable and
test_create_mcp_oauth_provider_maps_client_configuration to accept the pytest
monkeypatch fixture and use monkeypatch.setenv for FABRIC_MCP_CLIENT_SECRET
instead of mutating os.environ. Remove the os import if no other code uses it.
In `@tests/adapters/test_claude_adapter.py`:
- Around line 364-385: Guard
test_claude_interactive_mcp_command_has_terminal_stdin with a platform
capability check and skip it when os.name is "nt" or os.openpty is unavailable.
Keep the existing pseudo-terminal assertions unchanged on supported POSIX
platforms.
In `@tests/adapters/test_codex_adapter.py`:
- Around line 275-277: Replace direct environment mutations with pytest
monkeypatching: in tests/adapters/test_codex_adapter.py lines 275-277, add the
monkeypatch fixture and use monkeypatch.setenv for all three variables; in
tests/adapters/test_hermes_adapter.py line 430, add the fixture and use
monkeypatch.setenv for FABRIC_MCP_CLIENT_SECRET; in
tests/adapters/test_claude_adapter.py lines 388-401, add the fixture and apply
monkeypatch.setenv for each browser_environment entry instead of
os.environ.update.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 03a29893-433f-404f-a2c5-67d5afdb236f
⛔ Files ignored due to path filters (6)
adapters/claude/uv.lockis excluded by!**/*.lockadapters/codex/uv.lockis excluded by!**/*.lockadapters/common/uv.lockis excluded by!**/*.lockadapters/deepagents/uv.lockis excluded by!**/*.lockadapters/hermes/uv.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (73)
adapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex/src/nemo_fabric_adapters/codex/adapter.pyadapters/common/pyproject.tomladapters/common/src/nemo_fabric_adapters/common/mcp_auth.pyadapters/deepagents/pyproject.tomladapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.pyadapters/hermes/src/nemo_fabric_adapters/hermes/adapter.pycrates/fabric-core/src/config.rscrates/fabric-core/src/lib.rsdocs/reference/api/python-library-reference/index.mddocs/reference/api/python-library-reference/nemo_fabric.models.mddocs/reference/api/rust-library-reference/nemo-fabric-core/adapter-contract/index.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/agent-config/index.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/agent-execution/index.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-mcpauthenticationconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-mcpexposure.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-mcptransport.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-oauthtokenendpointauthmethod.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-relayatifstorageconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-relayatofmode.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-relayatofsinkconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-relayatofstreamfieldnamepolicy.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-relayatofstreamtransport.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-relayotlptransport.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-relayunsupportedbehavior.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-resolutionstrategy.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-telemetryprovider.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/fn-load-adapter-descriptor.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/fn-resolve-run-plan-from-config.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/index.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-mcpconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-mcpserverconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-mcpserverplan.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-metadataconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-modelconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-resolvecontext.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-resolvedadapterdescriptor.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-runplan.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-runtimecapabilities.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-runtimeconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-skillconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-telemetryconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-telemetryplan.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-telemetryproviderconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-tooldefinitionconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-toolsconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-workflowconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-workflowentrypointconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/doctor/enum-doctorstatus.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/doctor/fn-doctor-plan.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/doctor/index.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/doctor/struct-doctorcheck.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/doctor/struct-doctorreport.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/error/enum-fabricerror.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/error/index.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/error/type-result.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/fn-version.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/index.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/index.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/schema/index.mdxpython/src/nemo_fabric/__init__.pypython/src/nemo_fabric/models.pypython/src/nemo_fabric/types.pyschemas/agent.schema.jsonschemas/run-plan.schema.jsontests/adapters/test_adapter_package_metadata.pytests/adapters/test_adapters_common_mcp_auth.pytests/adapters/test_claude_adapter.pytests/adapters/test_codex_adapter.pytests/adapters/test_deepagents.pytests/adapters/test_hermes_adapter.pytests/python/test_native_sdk.pytests/python/test_sdk_contract.py
| ### `url: String` | ||
|
|
||
| MCP server URL for network transports or executable for stdio. | ||
| MCP server URL or process command (when transport=stdio), depending on transport. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Should fix: Format the transport=stdio expression as code.
Line 24 renders the configuration expression as prose. Update the corresponding doc comment in crates/fabric-core/src/config.rs, wrap transport=stdio in backticks, and run just docs instead of editing this generated page directly.
As per coding guidelines, format expressions as inline code. As per path instructions, update the source and regenerate this reference.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@docs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-mcpserverconfig.mdx`
at line 24, Update the MCP server URL documentation comment in MCPServerConfig
within crates/fabric-core/src/config.rs to format transport=stdio as inline
code, then run just docs to regenerate the reference page; do not edit the
generated MDX directly.
Sources: Coding guidelines, Path instructions
| authentication: McpAuthenticationConfig | None = Field( | ||
| default=None, exclude_if=lambda value: value is None | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Widen the authentication annotations to accept a mapping.
The annotation is McpAuthenticationConfig | None, but the documented usage passes a plain dict. tests/python/test_sdk_contract.py Line 340-350 and Line 576-584 both pass mappings, and pydantic coerces them at runtime.
A static type checker rejects that documented form in consumer code. The relay field at Line 830 already models the dual form as RelayConfig | dict[str, Any] | None. Apply the same pattern to the field at Line 426, to McpConfig.add_server at Line 491, and to FabricConfig.add_mcp_server at Line 855.
♻️ Proposed annotation change
- authentication: McpAuthenticationConfig | None = Field(
+ authentication: McpAuthenticationConfig | Mapping[str, Any] | None = Field(
default=None, exclude_if=lambda value: value is None
)- authentication: McpAuthenticationConfig | None = None,
+ authentication: McpAuthenticationConfig | Mapping[str, Any] | None = None,Also applies to: 491-491, 855-855
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/src/nemo_fabric/models.py` around lines 426 - 428, Widen the
authentication type annotations to accept mappings alongside
McpAuthenticationConfig and None. Update the authentication field near
McpConfig, the McpConfig.add_server signature, and FabricConfig.add_mcp_server
to use the same dual-form pattern as relay, with dict[str, Any] included.
| def test_resolve_client_secret_uses_named_environment_variable(): | ||
| os.environ["FABRIC_MCP_CLIENT_SECRET"] = "oauth-secret" | ||
| config = mcp_auth.McpOAuth2Config( | ||
| client_id="fabric-client", | ||
| client_secret_env="FABRIC_MCP_CLIENT_SECRET", | ||
| scopes=(), | ||
| redirect_uri=None, | ||
| ) | ||
|
|
||
| assert ( | ||
| mcp_auth.resolve_client_secret("docs", config, require_client_id=True) | ||
| == "oauth-secret" | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use monkeypatch.setenv instead of writing to os.environ.
Line 84 and Line 164 set FABRIC_MCP_CLIENT_SECRET in the real process environment and never remove it. The value then leaks into every later test in the session and makes results order-dependent. monkeypatch already appears elsewhere in this file.
🧪 Proposed fix for both sites
-def test_resolve_client_secret_uses_named_environment_variable():
- os.environ["FABRIC_MCP_CLIENT_SECRET"] = "oauth-secret"
+def test_resolve_client_secret_uses_named_environment_variable(monkeypatch):
+ monkeypatch.setenv("FABRIC_MCP_CLIENT_SECRET", "oauth-secret")
config = mcp_auth.McpOAuth2Config(Apply the same change to test_create_mcp_oauth_provider_maps_client_configuration at Line 163-164, then drop the now-unused os import if nothing else needs it.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_resolve_client_secret_uses_named_environment_variable(): | |
| os.environ["FABRIC_MCP_CLIENT_SECRET"] = "oauth-secret" | |
| config = mcp_auth.McpOAuth2Config( | |
| client_id="fabric-client", | |
| client_secret_env="FABRIC_MCP_CLIENT_SECRET", | |
| scopes=(), | |
| redirect_uri=None, | |
| ) | |
| assert ( | |
| mcp_auth.resolve_client_secret("docs", config, require_client_id=True) | |
| == "oauth-secret" | |
| ) | |
| def test_resolve_client_secret_uses_named_environment_variable(monkeypatch): | |
| monkeypatch.setenv("FABRIC_MCP_CLIENT_SECRET", "oauth-secret") | |
| config = mcp_auth.McpOAuth2Config( | |
| client_id="fabric-client", | |
| client_secret_env="FABRIC_MCP_CLIENT_SECRET", | |
| scopes=(), | |
| redirect_uri=None, | |
| ) | |
| assert ( | |
| mcp_auth.resolve_client_secret("docs", config, require_client_id=True) | |
| == "oauth-secret" | |
| ) |
🧰 Tools
🪛 Ruff (0.16.1)
[error] 84-84: Possible hardcoded password assigned to: "FABRIC_MCP_CLIENT_SECRET"
(S105)
[error] 87-87: Possible hardcoded password assigned to argument: "client_secret_env"
(S106)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/adapters/test_adapters_common_mcp_auth.py` around lines 83 - 95, Update
test_resolve_client_secret_uses_named_environment_variable and
test_create_mcp_oauth_provider_maps_client_configuration to accept the pytest
monkeypatch fixture and use monkeypatch.setenv for FABRIC_MCP_CLIENT_SECRET
instead of mutating os.environ. Remove the os import if no other code uses it.
Source: Coding guidelines
| os.environ["DBUS_SESSION_BUS_ADDRESS"] = "unix:path=/run/user/1000/bus" | ||
| os.environ["FABRIC_UNRELATED_SECRET"] = "do-not-forward" | ||
| os.environ["XDG_RUNTIME_DIR"] = "/run/user/1000" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Three test files write os.environ directly, so values leak between tests. The shared root cause is direct process-environment mutation instead of monkeypatch.setenv. Each adapter under test reads these names from its INHERITED_ENV_NAMES set or through mcp_auth.resolve_client_secret, so a leaked value makes later tests order dependent and can mask a missing-variable failure.
tests/adapters/test_codex_adapter.py#L275-L277: replace theDBUS_SESSION_BUS_ADDRESS,FABRIC_UNRELATED_SECRET, andXDG_RUNTIME_DIRassignments withmonkeypatch.setenvand add themonkeypatchfixture to the signature.tests/adapters/test_hermes_adapter.py#L430-L430: replace theFABRIC_MCP_CLIENT_SECRETassignment withmonkeypatch.setenvand add themonkeypatchfixture to the signature.tests/adapters/test_claude_adapter.py#L388-L401: replaceos.environ.update(browser_environment)with amonkeypatch.setenvloop overbrowser_environmentand add themonkeypatchfixture to the signature.
🧰 Tools
🪛 Ruff (0.16.1)
[error] 276-276: Possible hardcoded password assigned to: "FABRIC_UNRELATED_SECRET"
(S105)
📍 Affects 3 files
tests/adapters/test_codex_adapter.py#L275-L277(this comment)tests/adapters/test_hermes_adapter.py#L430-L430tests/adapters/test_claude_adapter.py#L388-L401
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/adapters/test_codex_adapter.py` around lines 275 - 277, Replace direct
environment mutations with pytest monkeypatching: in
tests/adapters/test_codex_adapter.py lines 275-277, add the monkeypatch fixture
and use monkeypatch.setenv for all three variables; in
tests/adapters/test_hermes_adapter.py line 430, add the fixture and use
monkeypatch.setenv for FABRIC_MCP_CLIENT_SECRET; in
tests/adapters/test_claude_adapter.py lines 388-401, add the fixture and apply
monkeypatch.setenv for each browser_environment entry instead of
os.environ.update.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/adapters/test_claude_adapter.py`:
- Around line 391-411: Extend the tests around
test_claude_interactive_mcp_command_requires_local_pseudo_terminal with a
focused case that monkeypatches os.openpty to raise OSError. Invoke
_run_claude_mcp_command with interactive=True and assert ClaudeAdapterError has
code claude_mcp_authentication_failed and a message matching requires a local
pseudo-terminal.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 3e6619f4-ea4d-4671-a8e2-352a006786a0
📒 Files selected for processing (1)
tests/adapters/test_claude_adapter.py
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Test (Python 3.11, macos-arm64)
- GitHub Check: Test (Python 3.14, macos-arm64)
- GitHub Check: Test (Python 3.12, macos-arm64)
- GitHub Check: Test (Python 3.13, macos-arm64)
🧰 Additional context used
📓 Path-based instructions (13)
**/*.{rs,py,pyi,json,yaml,yml}
📄 CodeRabbit inference engine (.agents/skills/contribute-api/SKILL.md)
Determine and update every affected public surface, including the CLI, PyO3 bindings, Python SDK, type stubs, schemas, and adapter contract, so they remain in parity.
Files:
tests/adapters/test_claude_adapter.py
**/*
📄 CodeRabbit inference engine (.agents/skills/karpathy-guidelines/SKILL.md)
**/*: Before implementing, explicitly state assumptions, surface ambiguity and tradeoffs, present multiple interpretations when relevant, and ask for clarification rather than silently deciding or proceeding when requirements are unclear.
Prefer the minimum code needed to solve the requested problem: avoid speculative features, unnecessary abstractions, unrequested flexibility, and handling of impossible scenarios; simplify overcomplicated solutions.
When editing existing code, make surgical changes only: do not modify unrelated code, comments, formatting, or pre-existing dead code; match the existing style, and remove only unused imports, variables, or functions introduced by your changes.
Define verifiable success criteria for each task, such as writing regression tests for bugs and invalid-input tests for validation, then verify the implementation against those criteria. For multi-step work, state a brief plan with a verification check for each step.
**/*: Always spellNVIDIAin all caps; do not useNvidia,nvidia,nVidia,nVIDIA, orNV.
Usean NVIDIAbefore a noun, because the name begins with an “en” sound.
Do not add a registered trademark symbol afterNVIDIAwhen referring to the company; use trademark symbols with product names only when required by the document type or legal guidance.
Verify official capitalization, spacing, hyphenation, and spelling for NVIDIA and third-party product names; do not rewrite official product names for grammar or title-case rules.
Precede NVIDIA product names withNVIDIAon first mention when natural and accurate, and link the first mention when the destination helps the reader.
On first use, include the company name and full model qualifier when it helps identify the model; preserve official capitalization and punctuation, and use shorter family names only after establishing the full name.
For learning-oriented and developer content, do not force trademark symbols unless explicitly required; for press, ...
Files:
tests/adapters/test_claude_adapter.py
**/*.{rs,py}
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
For native binding changes, run
cargo check -p fabric-python --locked.Use
snake_casefor functions and variables; usePascalCasefor Rust types and Python classes.
Files:
tests/adapters/test_claude_adapter.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If Python code or a Python-facing adapter changes, run
just test-python.In Python SDK, adapters, examples, and tests, follow the existing style, use type annotations for public APIs, and keep native binding declarations synchronized with their Rust implementations.
Files:
tests/adapters/test_claude_adapter.py
**/*.{rs,py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*.{rs,py,pyi}: If public configuration types change, confirm schema snapshot tests injust test-rustpass and review generated schema diffs.
For schema or public contract changes, run both language suites and review changes underschemas/and generated API references.
Files:
tests/adapters/test_claude_adapter.py
tests/adapters/**/*.py
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
tests/adapters/**/*.py: If an adapter or integration changes, run its focused tests.
For adapter behavior changes, run focused adapter tests undertests/adapters, then runjust test-python.
Files:
tests/adapters/test_claude_adapter.py
**/*.{py,pyi,rs}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
For Python SDK or PyO3 binding changes, use
python-tests, run focused pytest tests first, thenjust test-python; rebuild withjust build-pythonwhen native code or packaging changes.Public contract changes must keep native Python binding declarations synchronized with their Rust implementations.
Files:
tests/adapters/test_claude_adapter.py
**/*.{rs,py,toml}
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
When editing version helpers, verify every
nemo-fabric-*workspace package through Cargo metadata and reject a static version inpython/pyproject.toml.
Files:
tests/adapters/test_claude_adapter.py
**/*.{toml,rs,py}
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
Avoid blind repository-wide replacement of version-like strings; distinguish package-version references from examples and unrelated dependency versions.
Files:
tests/adapters/test_claude_adapter.py
**/*.{md,mdx,yml,py,rs,sh}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/SKILL.md)
Keep documentation aligned with current NeMo Fabric behavior, repository layout, entry points, commands, package names, APIs, bindings, and support claims.
Files:
tests/adapters/test_claude_adapter.py
tests/**/*.{rs,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
When adding functionality, include tests in the corresponding Rust crate or the relevant area under
tests/.
Files:
tests/adapters/test_claude_adapter.py
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Use pytest to run Python tests.
Do not add@pytest.mark.asyncioto tests; async tests are automatically detected by the async runner.
Do not add-> Nonereturn type annotations to test functions.
When mocking a class, useunittest.mock.MagicMockorAsyncMock, using thespecargument when necessary, rather than defining a new class.
Prefix mocked class names withmock, notfake.
Prefer pytest fixtures over helper methods.
If a fixture is needed in multiple test files, define it once inconftest.pyrather than repeating it.
Define fixtures using@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and a<fixture_name>_fixturefunction; specifyscopeonly when it is notfunction.
Preferpytest.mark.parametrizeover separate tests for different input types.
Use@pytest.mark.usefixtureswhen a fixture is needed but its returned value is unused or it returns no value.
Avoid defensive programming in tests; access expected values directly so missing data raises a clear failure, such as usingresults["data"]instead ofresults.get("data").
When adapter installation metadata changes, packaging metadata tests must directly assert that the root project depends unconditionally on the exact-versionnemo-fabric-runtimedistribution.
Packaging metadata tests must verify that each root harness extra delegates to the matching version of the leaf adapter'sharnessextra.
Packaging metadata tests must verify that bare leaf dependencies remain adapter-owned and that the rootadapter-testsdependency group installs each leaf through itsharnessextra.
Packaging metadata tests must verify that every leaf providesfull; only adapters importing NeMo Relay Python APIs providerelay, while adapters using an external Relay executable havefullequal toharness.
Files:
tests/adapters/test_claude_adapter.py
{tests/**,python/tests/**}
⚙️ CodeRabbit configuration file
{tests/**,python/tests/**}: Tests should cover the behavior promised by the changed API surface, including error paths, lifecycle cleanup, and SDK/native parity where relevant.
Files:
tests/adapters/test_claude_adapter.py
🔇 Additional comments (1)
tests/adapters/test_claude_adapter.py (1)
364-366: LGTM!
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py (3)
409-424: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for CLI path resolution. Keep
_resolve_path(payload, cli_path)as the only normalization. Do not callPath.expanduser()or useharness.settings.cwd. Assert that~/bin/clauderesolves underbase_dir, while absolute paths remain unchanged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapters/claude/src/nemo_fabric_adapters/claude/adapter.py` around lines 409 - 424, The CLI path resolution regression coverage is missing. Add a test for _resolve_path(payload, cli_path) that verifies ~/bin/claude resolves relative to base_dir and absolute paths remain unchanged; keep _resolve_path as the sole normalization path without calling Path.expanduser() or using harness.settings.cwd.Source: Learnings
449-452: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake subprocess teardown bounded and exception-safe.
If
create_subprocess_execfails, close both PTY descriptors. The current cleanup leaksmaster.During timeout or cancellation, bound
process.wait(). Ifterminate()does not stop the process, callkill()and reap it. Drain the non-interactivePIPEstreams during teardown.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapters/claude/src/nemo_fabric_adapters/claude/adapter.py` around lines 449 - 452, Update the subprocess cleanup around the adapter’s exception-handling path to always close both PTY descriptors, including master, when process creation fails. During timeout or cancellation, bound process.wait(), escalate from terminate() to kill() if needed, then reap the process, and drain the non-interactive PIPE streams during teardown; keep cleanup exception-safe.
1319-1386: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRun the repository’s configured Python checks before merge.
The
python-testscommand is undefined and exits with status 127. Run the focused test withuv run --no-sync pytest tests/adapters/test_claude_adapter.py, then runjust test-python. Run the configured pre-commit checks before the final test pass.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapters/claude/src/nemo_fabric_adapters/claude/adapter.py` around lines 1319 - 1386, Run the focused Claude adapter test with uv run --no-sync pytest tests/adapters/test_claude_adapter.py, then execute the configured pre-commit checks and run just test-python. Do not use the undefined python-tests command; resolve any failures before the final test pass.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@adapters/claude/src/nemo_fabric_adapters/claude/adapter.py`:
- Around line 409-424: The CLI path resolution regression coverage is missing.
Add a test for _resolve_path(payload, cli_path) that verifies ~/bin/claude
resolves relative to base_dir and absolute paths remain unchanged; keep
_resolve_path as the sole normalization path without calling Path.expanduser()
or using harness.settings.cwd.
- Around line 449-452: Update the subprocess cleanup around the adapter’s
exception-handling path to always close both PTY descriptors, including master,
when process creation fails. During timeout or cancellation, bound
process.wait(), escalate from terminate() to kill() if needed, then reap the
process, and drain the non-interactive PIPE streams during teardown; keep
cleanup exception-safe.
- Around line 1319-1386: Run the focused Claude adapter test with uv run
--no-sync pytest tests/adapters/test_claude_adapter.py, then execute the
configured pre-commit checks and run just test-python. Do not use the undefined
python-tests command; resolve any failures before the final test pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: b59cb374-e4d1-49b3-aa33-9c8f8d2de7b3
📒 Files selected for processing (1)
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
📜 Review details
⏰ Context from checks skipped due to timeout. (17)
- GitHub Check: Preview docs
- GitHub Check: Test (Python 3.11, linux-arm64)
- GitHub Check: Test (Python 3.13, windows-amd64)
- GitHub Check: Test (Python 3.14, macos-arm64)
- GitHub Check: Test (Python 3.12, linux-arm64)
- GitHub Check: Test (Python 3.13, linux-arm64)
- GitHub Check: Test (Python 3.14, windows-amd64)
- GitHub Check: Test (Python 3.11, windows-amd64)
- GitHub Check: Test (Python 3.11, macos-arm64)
- GitHub Check: Test (Python 3.14, linux-arm64)
- GitHub Check: Test (Python 3.13, macos-arm64)
- GitHub Check: Test (Python 3.12, windows-amd64)
- GitHub Check: Test (Python 3.12, macos-arm64)
- GitHub Check: Test (Python 3.14, linux-amd64)
- GitHub Check: Test (Python 3.11, linux-amd64)
- GitHub Check: Test (Python 3.13, linux-amd64)
- GitHub Check: Pre-commit
🧰 Additional context used
📓 Path-based instructions (11)
**/*.{rs,py,pyi,json,yaml,yml}
📄 CodeRabbit inference engine (.agents/skills/contribute-api/SKILL.md)
Determine and update every affected public surface, including the CLI, PyO3 bindings, Python SDK, type stubs, schemas, and adapter contract, so they remain in parity.
Files:
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*
📄 CodeRabbit inference engine (.agents/skills/karpathy-guidelines/SKILL.md)
**/*: Before implementing, explicitly state assumptions, surface ambiguity and tradeoffs, present multiple interpretations when relevant, and ask for clarification rather than silently deciding or proceeding when requirements are unclear.
Prefer the minimum code needed to solve the requested problem: avoid speculative features, unnecessary abstractions, unrequested flexibility, and handling of impossible scenarios; simplify overcomplicated solutions.
When editing existing code, make surgical changes only: do not modify unrelated code, comments, formatting, or pre-existing dead code; match the existing style, and remove only unused imports, variables, or functions introduced by your changes.
Define verifiable success criteria for each task, such as writing regression tests for bugs and invalid-input tests for validation, then verify the implementation against those criteria. For multi-step work, state a brief plan with a verification check for each step.
**/*: Always spellNVIDIAin all caps; do not useNvidia,nvidia,nVidia,nVIDIA, orNV.
Usean NVIDIAbefore a noun, because the name begins with an “en” sound.
Do not add a registered trademark symbol afterNVIDIAwhen referring to the company; use trademark symbols with product names only when required by the document type or legal guidance.
Verify official capitalization, spacing, hyphenation, and spelling for NVIDIA and third-party product names; do not rewrite official product names for grammar or title-case rules.
Precede NVIDIA product names withNVIDIAon first mention when natural and accurate, and link the first mention when the destination helps the reader.
On first use, include the company name and full model qualifier when it helps identify the model; preserve official capitalization and punctuation, and use shorter family names only after establishing the full name.
For learning-oriented and developer content, do not force trademark symbols unless explicitly required; for press, ...
Files:
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{rs,py}
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
For native binding changes, run
cargo check -p fabric-python --locked.Use
snake_casefor functions and variables; usePascalCasefor Rust types and Python classes.
Files:
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If Python code or a Python-facing adapter changes, run
just test-python.In Python SDK, adapters, examples, and tests, follow the existing style, use type annotations for public APIs, and keep native binding declarations synchronized with their Rust implementations.
Files:
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{rs,py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*.{rs,py,pyi}: If public configuration types change, confirm schema snapshot tests injust test-rustpass and review generated schema diffs.
For schema or public contract changes, run both language suites and review changes underschemas/and generated API references.
Files:
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{py,pyi,rs}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
For Python SDK or PyO3 binding changes, use
python-tests, run focused pytest tests first, thenjust test-python; rebuild withjust build-pythonwhen native code or packaging changes.Public contract changes must keep native Python binding declarations synchronized with their Rust implementations.
Files:
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{rs,py,toml}
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
When editing version helpers, verify every
nemo-fabric-*workspace package through Cargo metadata and reject a static version inpython/pyproject.toml.
Files:
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{toml,rs,py}
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
Avoid blind repository-wide replacement of version-like strings; distinguish package-version references from examples and unrelated dependency versions.
Files:
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{md,mdx,yml,py,rs,sh}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/SKILL.md)
Keep documentation aligned with current NeMo Fabric behavior, repository layout, entry points, commands, package names, APIs, bindings, and support claims.
Files:
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
adapters/*/src/**/*.py
📄 CodeRabbit inference engine (.agents/skills/contribute-adapter/SKILL.md)
adapters/*/src/**/*.py: Implement adapters using the existing Fabricpythonorprocessrunner and normalized request/result contracts; do not add a runner or one-off abstraction for a single adapter.
Treat normalizedconfig, Fabric-resolved plans, andruntime_contextas authoritative; reserveharness.settingsfor adapter-wide behavior and apply precedence in the order: normalized config, plans/context, harness settings, descriptor/default values.
Reject conflicting duplicate declarations and unsupported behavior with actionable errors naming the field and supported alternatives; never silently drop configuration.
Validate dependency versions, hooks, and credentials before harness invocation, and never expose credential values in outputs, errors, events, logs, or fixtures.
Forward only required system, selected credential, telemetry, and documented harness-specific environment variables; never forward or log unrelated environment values.
Maintain one local adapter host per Fabric runtime across orderedstart→invoke*→stopoperations. Emit one JSON lifecycle response per request on stdout and diagnostics on stderr.
Return harness-level invocation failures as successful lifecycle responses containingresponse: null,failed: true, and structurederrorfields (code,message,retryable, and optionalmetadata).
Do not emit NeMo Relay stream records on adapter stdout; return exactly one terminal lifecycle response while streaming occurs through the SDK-owned out-of-band endpoint.
Scope workspace, generated configuration, state, sessions, and artifacts to the resolved runtime context, and isolate stateful adapter instances by Fabric runtime ID.
Usestartto initialize adapter-owned harness state, retain it for continuation across repeatedinvokecalls on the same runtime, and release it instop.
Files:
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
{adapters/**,examples/**}
⚙️ CodeRabbit configuration file
{adapters/**,examples/**}: Review adapter and example changes for command correctness, config/schema consistency, artifact handling, and compatibility with the public NeMo Fabric contracts.
Files:
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
🧠 Learnings (1)
📚 Learning: 2026-07-09T22:28:51.689Z
Learnt from: AjayThorve
Repo: NVIDIA/NeMo-Fabric PR: 43
File: adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py:164-168
Timestamp: 2026-07-09T22:28:51.689Z
Learning: In the NeMo-Fabric adapters, treat path values used in Fabric adapter configuration (including logic like `_resolve_path` in adapter.py) as config-root-relative. Do not apply `Path.expanduser()` (or otherwise apply `~`/home or shell-style expansion), because it will make the resolved paths normalize inconsistently across adapters. Also, do not rely on or add any resolution behavior that uses `harness.settings.cwd` as an override point for these adapter paths—`harness.settings.cwd` is explicitly unsupported in this adapter context.
Applied to files:
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
🔇 Additional comments (5)
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py (5)
482-485: LGTM!Also applies to: 493-496
1058-1061: 🔒 Security & PrivacyVerify per-server isolation for OAuth client secrets.
build_optionspasses one shared environment toClaudeAgentOptions.envat Line 924. These lines add each configuredclient_secret_envvalue to that process-wide environment.If Claude CLI forwards that environment to stdio MCP processes, one server can read another server’s client secret. Keep secrets only in the authentication subprocess, or confirm that Claude CLI removes them before launching each MCP server. Add a two-server isolation test.
As per path instructions, adapters must forward only required system, selected credential, telemetry, and documented harness-specific environment variables.
Source: Path instructions
1263-1267: 🩺 Stability & AvailabilityUse one invocation deadline for authentication and the query.
invokepasses the fullinvocation_timeoutto_authenticate_mcp_servers, then passes the same full value to_run_query.If authentication applies the timeout to each server or retry, multiple servers can multiply the configured deadline before the query receives another full timeout. Compute one absolute deadline and pass the remaining budget to every authentication step and the query.
Also applies to: 1285-1290
1192-1193: 🩺 Stability & AvailabilityRevalidate MCP authentication after connection changes.
The new
_mcp_authentication_checkedstate appears to make authentication one-time per runtime. The flag resets only duringstop.If an OAuth token expires or an MCP connection becomes disconnected after the first invocation, later invocations can skip login or reconnection. Confirm that Claude SDK refreshes and reconnects independently. Otherwise, recheck status per invocation or clear the flag after authentication and connection failures.
Also applies to: 1319-1386, 1452-1456
302-304: 🗄️ Data Integrity & IntegrationNo MCP contract change required. The adapter preserves the transport fields, maps OAuth fields, consumes
client_secret_envandauthorization_timeout_seconds, and rejects unsupportedclient_nameandtoken_endpoint_auth_methodfields with explicit errors.
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py (1)
1340-1356: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winApply the invocation deadline to MCP status polling.
_mcp_server_statuscan wait five seconds before login and five seconds after reconnect. It does not receiveinvocation_timeout. An invocation with a shorter timeout can exceed its deadline before_authenticate_mcp_serversreturns.Wrap the full authentication sequence in
asyncio.timeout(invocation_timeout). Add a regression test with a continuouslypendingserver and a shorter invocation timeout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapters/claude/src/nemo_fabric_adapters/claude/adapter.py` around lines 1340 - 1356, The MCP authentication flow in `_authenticate_mcp_servers` must enforce the overall invocation deadline, including both `_mcp_server_status` calls, `_login_mcp_server`, and `client.reconnect_mcp_server`. Wrap the complete per-invocation authentication sequence in `asyncio.timeout(invocation_timeout)` and add a regression test using a continuously pending server with a shorter invocation timeout.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@adapters/claude/src/nemo_fabric_adapters/claude/adapter.py`:
- Around line 1340-1356: The MCP authentication flow in
`_authenticate_mcp_servers` must enforce the overall invocation deadline,
including both `_mcp_server_status` calls, `_login_mcp_server`, and
`client.reconnect_mcp_server`. Wrap the complete per-invocation authentication
sequence in `asyncio.timeout(invocation_timeout)` and add a regression test
using a continuously pending server with a shorter invocation timeout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 6f111250-026e-4489-b31b-7b6401f87044
📒 Files selected for processing (2)
adapters/claude/src/nemo_fabric_adapters/claude/adapter.pytests/adapters/test_claude_adapter.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (15)
**/*.{rs,py,pyi,json,yaml,yml}
📄 CodeRabbit inference engine (.agents/skills/contribute-api/SKILL.md)
Determine and update every affected public surface, including the CLI, PyO3 bindings, Python SDK, type stubs, schemas, and adapter contract, so they remain in parity.
Files:
tests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*
📄 CodeRabbit inference engine (.agents/skills/karpathy-guidelines/SKILL.md)
**/*: Before implementing, explicitly state assumptions, surface ambiguity and tradeoffs, present multiple interpretations when relevant, and ask for clarification rather than silently deciding or proceeding when requirements are unclear.
Prefer the minimum code needed to solve the requested problem: avoid speculative features, unnecessary abstractions, unrequested flexibility, and handling of impossible scenarios; simplify overcomplicated solutions.
When editing existing code, make surgical changes only: do not modify unrelated code, comments, formatting, or pre-existing dead code; match the existing style, and remove only unused imports, variables, or functions introduced by your changes.
Define verifiable success criteria for each task, such as writing regression tests for bugs and invalid-input tests for validation, then verify the implementation against those criteria. For multi-step work, state a brief plan with a verification check for each step.
**/*: Always spellNVIDIAin all caps; do not useNvidia,nvidia,nVidia,nVIDIA, orNV.
Usean NVIDIAbefore a noun, because the name begins with an “en” sound.
Do not add a registered trademark symbol afterNVIDIAwhen referring to the company; use trademark symbols with product names only when required by the document type or legal guidance.
Verify official capitalization, spacing, hyphenation, and spelling for NVIDIA and third-party product names; do not rewrite official product names for grammar or title-case rules.
Precede NVIDIA product names withNVIDIAon first mention when natural and accurate, and link the first mention when the destination helps the reader.
On first use, include the company name and full model qualifier when it helps identify the model; preserve official capitalization and punctuation, and use shorter family names only after establishing the full name.
For learning-oriented and developer content, do not force trademark symbols unless explicitly required; for press, ...
Files:
tests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{rs,py}
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
For native binding changes, run
cargo check -p fabric-python --locked.Use
snake_casefor functions and variables; usePascalCasefor Rust types and Python classes.
Files:
tests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If Python code or a Python-facing adapter changes, run
just test-python.In Python SDK, adapters, examples, and tests, follow the existing style, use type annotations for public APIs, and keep native binding declarations synchronized with their Rust implementations.
Files:
tests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{rs,py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*.{rs,py,pyi}: If public configuration types change, confirm schema snapshot tests injust test-rustpass and review generated schema diffs.
For schema or public contract changes, run both language suites and review changes underschemas/and generated API references.
Files:
tests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
tests/adapters/**/*.py
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
tests/adapters/**/*.py: If an adapter or integration changes, run its focused tests.
For adapter behavior changes, run focused adapter tests undertests/adapters, then runjust test-python.
Files:
tests/adapters/test_claude_adapter.py
**/*.{py,pyi,rs}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
For Python SDK or PyO3 binding changes, use
python-tests, run focused pytest tests first, thenjust test-python; rebuild withjust build-pythonwhen native code or packaging changes.Public contract changes must keep native Python binding declarations synchronized with their Rust implementations.
Files:
tests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{rs,py,toml}
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
When editing version helpers, verify every
nemo-fabric-*workspace package through Cargo metadata and reject a static version inpython/pyproject.toml.
Files:
tests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{toml,rs,py}
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
Avoid blind repository-wide replacement of version-like strings; distinguish package-version references from examples and unrelated dependency versions.
Files:
tests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{md,mdx,yml,py,rs,sh}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/SKILL.md)
Keep documentation aligned with current NeMo Fabric behavior, repository layout, entry points, commands, package names, APIs, bindings, and support claims.
Files:
tests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
tests/**/*.{rs,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
When adding functionality, include tests in the corresponding Rust crate or the relevant area under
tests/.
Files:
tests/adapters/test_claude_adapter.py
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Use pytest to run Python tests.
Do not add@pytest.mark.asyncioto tests; async tests are automatically detected by the async runner.
Do not add-> Nonereturn type annotations to test functions.
When mocking a class, useunittest.mock.MagicMockorAsyncMock, using thespecargument when necessary, rather than defining a new class.
Prefix mocked class names withmock, notfake.
Prefer pytest fixtures over helper methods.
If a fixture is needed in multiple test files, define it once inconftest.pyrather than repeating it.
Define fixtures using@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and a<fixture_name>_fixturefunction; specifyscopeonly when it is notfunction.
Preferpytest.mark.parametrizeover separate tests for different input types.
Use@pytest.mark.usefixtureswhen a fixture is needed but its returned value is unused or it returns no value.
Avoid defensive programming in tests; access expected values directly so missing data raises a clear failure, such as usingresults["data"]instead ofresults.get("data").
When adapter installation metadata changes, packaging metadata tests must directly assert that the root project depends unconditionally on the exact-versionnemo-fabric-runtimedistribution.
Packaging metadata tests must verify that each root harness extra delegates to the matching version of the leaf adapter'sharnessextra.
Packaging metadata tests must verify that bare leaf dependencies remain adapter-owned and that the rootadapter-testsdependency group installs each leaf through itsharnessextra.
Packaging metadata tests must verify that every leaf providesfull; only adapters importing NeMo Relay Python APIs providerelay, while adapters using an external Relay executable havefullequal toharness.
Files:
tests/adapters/test_claude_adapter.py
{tests/**,python/tests/**}
⚙️ CodeRabbit configuration file
{tests/**,python/tests/**}: Tests should cover the behavior promised by the changed API surface, including error paths, lifecycle cleanup, and SDK/native parity where relevant.
Files:
tests/adapters/test_claude_adapter.py
adapters/*/src/**/*.py
📄 CodeRabbit inference engine (.agents/skills/contribute-adapter/SKILL.md)
adapters/*/src/**/*.py: Implement adapters using the existing Fabricpythonorprocessrunner and normalized request/result contracts; do not add a runner or one-off abstraction for a single adapter.
Treat normalizedconfig, Fabric-resolved plans, andruntime_contextas authoritative; reserveharness.settingsfor adapter-wide behavior and apply precedence in the order: normalized config, plans/context, harness settings, descriptor/default values.
Reject conflicting duplicate declarations and unsupported behavior with actionable errors naming the field and supported alternatives; never silently drop configuration.
Validate dependency versions, hooks, and credentials before harness invocation, and never expose credential values in outputs, errors, events, logs, or fixtures.
Forward only required system, selected credential, telemetry, and documented harness-specific environment variables; never forward or log unrelated environment values.
Maintain one local adapter host per Fabric runtime across orderedstart→invoke*→stopoperations. Emit one JSON lifecycle response per request on stdout and diagnostics on stderr.
Return harness-level invocation failures as successful lifecycle responses containingresponse: null,failed: true, and structurederrorfields (code,message,retryable, and optionalmetadata).
Do not emit NeMo Relay stream records on adapter stdout; return exactly one terminal lifecycle response while streaming occurs through the SDK-owned out-of-band endpoint.
Scope workspace, generated configuration, state, sessions, and artifacts to the resolved runtime context, and isolate stateful adapter instances by Fabric runtime ID.
Usestartto initialize adapter-owned harness state, retain it for continuation across repeatedinvokecalls on the same runtime, and release it instop.
Files:
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
{adapters/**,examples/**}
⚙️ CodeRabbit configuration file
{adapters/**,examples/**}: Review adapter and example changes for command correctness, config/schema consistency, artifact handling, and compatibility with the public NeMo Fabric contracts.
Files:
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
🧠 Learnings (1)
📚 Learning: 2026-07-09T22:28:51.689Z
Learnt from: AjayThorve
Repo: NVIDIA/NeMo-Fabric PR: 43
File: adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py:164-168
Timestamp: 2026-07-09T22:28:51.689Z
Learning: In the NeMo-Fabric adapters, treat path values used in Fabric adapter configuration (including logic like `_resolve_path` in adapter.py) as config-root-relative. Do not apply `Path.expanduser()` (or otherwise apply `~`/home or shell-style expansion), because it will make the resolved paths normalize inconsistently across adapters. Also, do not rely on or add any resolution behavior that uses `harness.settings.cwd` as an override point for these adapter paths—`harness.settings.cwd` is explicitly unsupported in this adapter context.
Applied to files:
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
🔇 Additional comments (2)
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py (1)
402-411: LGTM!Also applies to: 464-511, 1218-1219, 1269-1282
tests/adapters/test_claude_adapter.py (1)
330-355: LGTM!
…th flow Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
schemas/run-plan.schema.json (1)
1494-1500: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winFix the transport name in the
custom_headersdescription.The description says the header applies when transport is
sseorstreamable_http, but theMcpTransportenum at Line 1601-1620 definesstreamable-httpwith a hyphen. No configuration value namedstreamable_httpexists, so the description misdirects config authors.This file is generated output. Correct the Rust doc comment on
McpServerConfig::custom_headersincrates/fabric-core/src/config.rsand regenerate the schema.📝 Proposed description text
- "description": "HTTP headers passed to an MCP server when transport is `sse` or `streamable_http`.", + "description": "HTTP headers passed to an MCP server when transport is `sse` or `streamable-http`.",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@schemas/run-plan.schema.json` around lines 1494 - 1500, Update the Rust doc comment for McpServerConfig::custom_headers to reference the valid transport name streamable-http instead of streamable_http, then regenerate schemas/run-plan.schema.json so the generated description matches.docs/reference/api/python-library-reference/nemo_fabric.models.md (1)
1035-1048: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBuilder signatures still take an untyped
transport.
McpServerConfig.transportis nowLiteral['stdio', 'sse', 'streamable-http'](Line 920), butadd_serverandadd_mcp_server(Line 2327) keeptransport: str. Pydantic rejects a bad value at runtime, so this is not a correctness bug. A static type checker cannot catchtransport="websocket"at the call site, which is the main benefit of the new literal.Narrow both parameters to the same literal in
python/src/nemo_fabric/models.pyand regenerate this reference withjust docs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/reference/api/python-library-reference/nemo_fabric.models.md` around lines 1035 - 1048, Update the transport parameter types in both add_server and add_mcp_server to use the same Literal['stdio', 'sse', 'streamable-http'] type as McpServerConfig.transport, then regenerate the Python library reference documentation with just docs.python/src/nemo_fabric/models.py (1)
410-440: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAdd typed
custom_headerssupport to the Python MCP API.
McpServerConfig,McpConfig.add_server, andFabricConfig.add_mcp_serveromit the field, so direct construction and builders accept it only as an untyped extra. Add the typed field and forward it through both builders. Preserve existingextra_fields={"custom_headers": ...}usage without duplicate-key errors. Update the generated API reference and add typed validation coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/src/nemo_fabric/models.py` around lines 410 - 440, Add a typed custom_headers field to McpServerConfig, then update McpConfig.add_server and FabricConfig.add_mcp_server to accept and forward it without conflicting with existing extra_fields custom_headers entries. Preserve current extra_fields behavior, and update the generated API reference plus validation tests to cover the new typed field.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@adapters/claude/src/nemo_fabric_adapters/claude/adapter.py`:
- Around line 388-394: Remove the duration-based timeout parameter from
_self_authenticate_http_mcp_server and apply the operation timeout at its
caller, _authenticate_mcp_servers, using asyncio.timeout. Update the call and
preserve the existing timeout duration and authentication behavior while
satisfying Ruff ASYNC109.
- Around line 1219-1249: Replace the single _mcp_authentication_checked flag
with a set tracking completed MCP server names in the authentication flow. In
the loop around _self_authenticate_http_mcp_server, skip servers already in that
set and add each server only after its authentication, staged-header update,
reconnect, and connected-status validation succeed. Reset the completed-server
set in stop alongside the existing authentication state.
- Around line 463-482: Update the MCP configuration handling around config_path
and server_entry so JSON decoding and invalid mcpServers structure failures are
converted into ClaudeAdapterError, not only OSError. Ensure invoke receives the
structured failure and returns the existing harness-level response shape with
response: null, failed: true, and structured error fields.
- Around line 1229-1241: Update invoke to compute one absolute deadline at its
start, then derive each authentication timeout and the query timeout from the
remaining invocation budget rather than resetting to invocation_timeout. Pass
that remaining budget into _run_query, including accounting for per-server
status polling, so the full invoke flow remains bounded by
runtime.timeout_seconds.
- Around line 439-448: Centralize MCP OAuth token extraction by adding the
shared mcp_auth.access_token(provider) helper and replacing the local
context/current_tokens/access_token traversal in the Claude adapter’s OAuth
flow. Preserve the existing missing-token ClaudeAdapterError behavior and
metadata after using the helper.
---
Outside diff comments:
In `@docs/reference/api/python-library-reference/nemo_fabric.models.md`:
- Around line 1035-1048: Update the transport parameter types in both add_server
and add_mcp_server to use the same Literal['stdio', 'sse', 'streamable-http']
type as McpServerConfig.transport, then regenerate the Python library reference
documentation with just docs.
In `@python/src/nemo_fabric/models.py`:
- Around line 410-440: Add a typed custom_headers field to McpServerConfig, then
update McpConfig.add_server and FabricConfig.add_mcp_server to accept and
forward it without conflicting with existing extra_fields custom_headers
entries. Preserve current extra_fields behavior, and update the generated API
reference plus validation tests to cover the new typed field.
In `@schemas/run-plan.schema.json`:
- Around line 1494-1500: Update the Rust doc comment for
McpServerConfig::custom_headers to reference the valid transport name
streamable-http instead of streamable_http, then regenerate
schemas/run-plan.schema.json so the generated description matches.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 2c57d02d-2a5d-4252-9039-294013b139cc
📒 Files selected for processing (9)
adapters/claude/src/nemo_fabric_adapters/claude/adapter.pycrates/fabric-core/src/agent_config.rscrates/fabric-core/src/config.rsdocs/reference/api/python-library-reference/nemo_fabric.models.mddocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-mcpserverplan.mdxpython/src/nemo_fabric/models.pyschemas/run-plan.schema.jsontests/python/test_native_sdk.pytests/python/test_sdk_contract.py
💤 Files with no reviewable changes (1)
- tests/python/test_native_sdk.py
📜 Review details
⏰ Context from checks skipped due to timeout. (18)
- GitHub Check: Preview docs
- GitHub Check: Test (Python 3.12, windows-amd64)
- GitHub Check: Test (Python 3.13, linux-arm64)
- GitHub Check: Test (Python 3.12, linux-arm64)
- GitHub Check: Test (Python 3.11, linux-arm64)
- GitHub Check: Test (Python 3.13, linux-amd64)
- GitHub Check: Test (Python 3.12, macos-arm64)
- GitHub Check: Test (Python 3.13, windows-amd64)
- GitHub Check: Test (Python 3.11, linux-amd64)
- GitHub Check: Test (Python 3.14, windows-amd64)
- GitHub Check: Test (Python 3.11, windows-amd64)
- GitHub Check: Test (Python 3.14, linux-arm64)
- GitHub Check: Test (Python 3.13, macos-arm64)
- GitHub Check: Test (Python 3.14, macos-arm64)
- GitHub Check: Test (Python 3.12, linux-amd64)
- GitHub Check: Test (Python 3.11, macos-arm64)
- GitHub Check: Test (Python 3.14, linux-amd64)
- GitHub Check: Pre-commit
🧰 Additional context used
📓 Path-based instructions (40)
{README.md,docs/**/*.{md,mdx,yml},examples/**/*.{md,mdx,yml}}
📄 CodeRabbit inference engine (.agents/skills/contribute-docs/SKILL.md)
Keep package names, repository references, and build commands current in documentation and examples.
Files:
docs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-mcpserverplan.mdxdocs/reference/api/python-library-reference/nemo_fabric.models.md
{docs/**/*.{md,mdx,yml},examples/**/*.{md,mdx,yml}}
📄 CodeRabbit inference engine (.agents/skills/contribute-docs/SKILL.md)
Update relevant getting-started, reference, adapter, and example documentation when the corresponding examples or adapters change.
Files:
docs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-mcpserverplan.mdxdocs/reference/api/python-library-reference/nemo_fabric.models.md
**/*.mdx
📄 CodeRabbit inference engine (.agents/skills/contribute-docs/SKILL.md)
In MDX files, use JSX comment delimiters (
{/*and*/}) for top-of-file comments, including SPDX headers; do not use HTML comments.
**/*.mdx: For documentation site changes, runjust docsto regenerate Python and Rust API references and validate Fern configuration.
MDX files must use the specified JSX-comment SPDX header.
Files:
docs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-mcpserverplan.mdx
docs/**/*.{md,mdx,yml}
📄 CodeRabbit inference engine (.agents/skills/contribute-docs/SKILL.md)
Run
just docswhen the documentation site changes.
Files:
docs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-mcpserverplan.mdxdocs/reference/api/python-library-reference/nemo_fabric.models.md
**/*
📄 CodeRabbit inference engine (.agents/skills/karpathy-guidelines/SKILL.md)
**/*: Before implementing, explicitly state assumptions, surface ambiguity and tradeoffs, present multiple interpretations when relevant, and ask for clarification rather than silently deciding or proceeding when requirements are unclear.
Prefer the minimum code needed to solve the requested problem: avoid speculative features, unnecessary abstractions, unrequested flexibility, and handling of impossible scenarios; simplify overcomplicated solutions.
When editing existing code, make surgical changes only: do not modify unrelated code, comments, formatting, or pre-existing dead code; match the existing style, and remove only unused imports, variables, or functions introduced by your changes.
Define verifiable success criteria for each task, such as writing regression tests for bugs and invalid-input tests for validation, then verify the implementation against those criteria. For multi-step work, state a brief plan with a verification check for each step.
**/*: Always spellNVIDIAin all caps; do not useNvidia,nvidia,nVidia,nVIDIA, orNV.
Usean NVIDIAbefore a noun, because the name begins with an “en” sound.
Do not add a registered trademark symbol afterNVIDIAwhen referring to the company; use trademark symbols with product names only when required by the document type or legal guidance.
Verify official capitalization, spacing, hyphenation, and spelling for NVIDIA and third-party product names; do not rewrite official product names for grammar or title-case rules.
Precede NVIDIA product names withNVIDIAon first mention when natural and accurate, and link the first mention when the destination helps the reader.
On first use, include the company name and full model qualifier when it helps identify the model; preserve official capitalization and punctuation, and use shorter family names only after establishing the full name.
For learning-oriented and developer content, do not force trademark symbols unless explicitly required; for press, ...
Files:
docs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-mcpserverplan.mdxcrates/fabric-core/src/agent_config.rsdocs/reference/api/python-library-reference/nemo_fabric.models.mdtests/python/test_sdk_contract.pypython/src/nemo_fabric/models.pycrates/fabric-core/src/config.rsadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyschemas/run-plan.schema.json
**/*.{md,mdx,rst}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-guide.md)
**/*.{md,mdx,rst}: For NeMo Fabric documentation, verify technical claims against the current repository, public API, or documented command before reviewing style.
Always spellNVIDIAin all caps; do not useNvidia,nvidia, orNV.
Format commands, code elements, expressions, package names, file names, and paths as inline code.
Use descriptive link text; avoid raw URLs and weak anchors such ashereorread more.
Use title case consistently for technical documentation headings.
Introduce code blocks, lists, tables, and images with complete sentences.
Write procedures as imperative, parallel steps; split long procedures into smaller tasks.
Prefer active voice, present tense, short sentences, contractions, and plain English while preserving necessary technical precision.
Usecanfor possibility and reservemayfor permission.
Useafterfor temporal relationships instead ofonce, and preferrefer tooverseewhen directing readers to another resource.
Avoid culture-specific idioms, unnecessary Latinisms, jokes, and marketing exaggeration in technical documentation.
Spell out months in body text, avoid ordinal dates, and use clear time zones.
Spell out whole numbers from zero through nine unless they are technical values, parameters, versions, or UI values; use numerals for 10 or greater and commas in thousands.
Do not add trademark symbols to learning-oriented documentation unless the source, platform, or legal guidance explicitly requires them.
Do not replace precise technical terms with simpler words when doing so would lose precision.
Do not flag passive voice when the actor is unknown or the action is the important part.
Do not rewrite API names, package names, command flags, or code literals for style.
**/*.{md,mdx,rst}: Use consistent title case for technical-document headings and table headers; avoid quotation marks, ampersands, and exclamation marks in headings, while preserving official product, event, research, and whitepaper title ...
Files:
docs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-mcpserverplan.mdxdocs/reference/api/python-library-reference/nemo_fabric.models.md
docs/reference/api/**/*
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/SKILL.md)
Treat all files under
docs/reference/api/as generated output and do not modify them directly.
Files:
docs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-mcpserverplan.mdxdocs/reference/api/python-library-reference/nemo_fabric.models.md
docs/**/*.mdx
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/SKILL.md)
docs/**/*.mdx: Use source-relative links with the target.mdxextension for links between files underdocs/; do not use Fern site-root paths.
Use{/* ... */}delimiters for top-of-file MDX SPDX comments, not HTML comment delimiters.
Files:
docs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-mcpserverplan.mdx
**/*.{md,mdx,yml,py,rs,sh}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/SKILL.md)
Keep documentation aligned with current NeMo Fabric behavior, repository layout, entry points, commands, package names, APIs, bindings, and support claims.
Files:
docs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-mcpserverplan.mdxcrates/fabric-core/src/agent_config.rsdocs/reference/api/python-library-reference/nemo_fabric.models.mdtests/python/test_sdk_contract.pypython/src/nemo_fabric/models.pycrates/fabric-core/src/config.rsadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{md,mdx}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/SKILL.md)
**/*.{md,mdx}: Use the full product nameNVIDIA NeMo Fabricon first use, typically in the title and H1; useNeMo Fabricthereafter. Usefabricalone only for the CLI tool and surround it with backticks.
Treat incorrect or stale commands, package names, paths, APIs, support claims, procedures, examples, terminology, or public behavior documentation as blocking issues.
CapitalizeNVIDIAcorrectly and format code, commands, paths, and filenames as inline code where needed.
Use title case for technical-documentation headings.
Introduce code blocks, tables, and lists with complete lead-in sentences; ensure examples match current APIs and build commands.
Use descriptive anchor text, avoid raw URLs and generic labels such ashere, and use repository-relative.mdxpaths for links withindocs/.
Prefer active voice, present tense, short sentences, plain English, consistent terminology, and imperative, parallel, scannable procedures.
Useafterinstead ofoncewhen expressing temporal sequence, and usecanrather thanmaywhen describing possibility rather than permission.
Avoid ambiguous numeric dates and ordinal dates in body text.
For learning-oriented documentation, do not force trademark symbols unless the source document explicitly requires them.
When reporting documentation-review findings, lead withMust fix,Should fix, andNice to havecategories; include file path, line reference, current problem, rationale, and a concrete rewrite or direction.
Files:
docs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-mcpserverplan.mdxdocs/reference/api/python-library-reference/nemo_fabric.models.md
docs/**
📄 CodeRabbit inference engine (AGENTS.md)
Run just docs after changing the documentation site.
Files:
docs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-mcpserverplan.mdxdocs/reference/api/python-library-reference/nemo_fabric.models.md
{docs/**,README.md,AGENTS.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,AGENTS.md}: Review documentation for technical accuracy against the current API, command correctness, and consistency with generated schemas.
For links between files under docs/, require paths relative to the source file with the target file's .mdx extension so they work in both Fern builds and repository browsers. Flag Fern site-root links such as NeMo Fabric overview; use the repository-relative equivalent, such as NeMo Fabric overview.
Files:
docs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-mcpserverplan.mdxdocs/reference/api/python-library-reference/nemo_fabric.models.md
{*.md,**/*.md,**/*.mdx,**/*.ipynb}
⚙️ CodeRabbit configuration file
{*.md,**/*.md,**/*.mdx,**/*.ipynb}: Enforce the product name in user-facing prose: use "NVIDIA NeMo Fabric" on first use and "NeMo Fabric" thereafter. Flag standalone capitalized "Fabric" when it refers to the product. Do not flag the lowercasefabricCLI command, package/import/crate names, code identifiers, API symbols, configuration keys, file paths, or unrelated generic uses of the word.
Files:
docs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-mcpserverplan.mdxdocs/reference/api/python-library-reference/nemo_fabric.models.md
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/contribute-api/SKILL.md)
Implement new runtime or binding behavior in the shared Rust core first.
Files:
crates/fabric-core/src/agent_config.rscrates/fabric-core/src/config.rs
**/*.{rs,py,pyi,json,yaml,yml}
📄 CodeRabbit inference engine (.agents/skills/contribute-api/SKILL.md)
Determine and update every affected public surface, including the CLI, PyO3 bindings, Python SDK, type stubs, schemas, and adapter contract, so they remain in parity.
Files:
crates/fabric-core/src/agent_config.rstests/python/test_sdk_contract.pypython/src/nemo_fabric/models.pycrates/fabric-core/src/config.rsadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyschemas/run-plan.schema.json
**/*.{rs,toml}
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
For any Rust change, run
just test-rustandcargo fmt --all -- --check.For Rust core, CLI, or shared runtime semantic changes, run Rust formatting and tests, and add Python tests when behavior is exposed through the SDK.
Use Rust stable tooling; format Rust code with
cargo fmt --all, verify formatting withcargo fmt --all -- --check, and compile withcargo check --workspace --locked.
Files:
crates/fabric-core/src/agent_config.rscrates/fabric-core/src/config.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
For native binding changes, run
cargo check -p fabric-python --locked.Use
snake_casefor functions and variables; usePascalCasefor Rust types and Python classes.
Files:
crates/fabric-core/src/agent_config.rstests/python/test_sdk_contract.pypython/src/nemo_fabric/models.pycrates/fabric-core/src/config.rsadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
crates/fabric-core/**/*.{rs,py}
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Changes under
crates/fabric-coremust run both the Rust and Python test suites.
Files:
crates/fabric-core/src/agent_config.rscrates/fabric-core/src/config.rs
**/*.{rs,rmeta}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If Rust code changes, run
cargo fmt --all -- --checkandjust test-rust.
Files:
crates/fabric-core/src/agent_config.rscrates/fabric-core/src/config.rs
crates/fabric-core/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If
crates/fabric-corechanges in a way exposed through Python, run both the Rust and Python suites.For Rust API reference changes, update Rust documentation comments under
crates/fabric-core/instead of generated API reference files.
Files:
crates/fabric-core/src/agent_config.rscrates/fabric-core/src/config.rs
**/*.{rs,py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*.{rs,py,pyi}: If public configuration types change, confirm schema snapshot tests injust test-rustpass and review generated schema diffs.
For schema or public contract changes, run both language suites and review changes underschemas/and generated API references.
Files:
crates/fabric-core/src/agent_config.rstests/python/test_sdk_contract.pypython/src/nemo_fabric/models.pycrates/fabric-core/src/config.rsadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{py,pyi,rs}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
For Python SDK or PyO3 binding changes, use
python-tests, run focused pytest tests first, thenjust test-python; rebuild withjust build-pythonwhen native code or packaging changes.Public contract changes must keep native Python binding declarations synchronized with their Rust implementations.
Files:
crates/fabric-core/src/agent_config.rstests/python/test_sdk_contract.pypython/src/nemo_fabric/models.pycrates/fabric-core/src/config.rsadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{rs,py,toml}
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
When editing version helpers, verify every
nemo-fabric-*workspace package through Cargo metadata and reject a static version inpython/pyproject.toml.
Files:
crates/fabric-core/src/agent_config.rstests/python/test_sdk_contract.pypython/src/nemo_fabric/models.pycrates/fabric-core/src/config.rsadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{toml,rs,py}
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
Avoid blind repository-wide replacement of version-like strings; distinguish package-version references from examples and unrelated dependency versions.
Files:
crates/fabric-core/src/agent_config.rstests/python/test_sdk_contract.pypython/src/nemo_fabric/models.pycrates/fabric-core/src/config.rsadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
crates/fabric-core/src/**/*.rs
⚙️ CodeRabbit configuration file
crates/fabric-core/src/**/*.rs: Review the Rust core for runtime lifecycle correctness, handle validation, capability routing accuracy, schema stability, and error semantics.
Public API changes should match committed schemas, tests, and documentation.
Files:
crates/fabric-core/src/agent_config.rscrates/fabric-core/src/config.rs
**/*.{md,rst}
📄 CodeRabbit inference engine (.agents/skills/contribute-api/SKILL.md)
Update documentation and examples in the same branch as the public API change.
Files:
docs/reference/api/python-library-reference/nemo_fabric.models.md
**/*.{md,rst,txt,adoc}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-language-mechanics.md)
**/*.{md,rst,txt,adoc}: For technical documentation, use professional, active, conversational, engaging, precise, and plain-English prose. Prefer active voice, present tense, short sentences, and scannable paragraphs. Avoid casual or imprecise language, swearing, threats, insults, jokes, puns, culture-specific idioms, marketing exaggeration, and unsupported third-party comparisons.
Usecanfor possibility and reservemayfor permission; useafterfor temporal order; userefer tofor cross-references; prefer short direct sentences and specific verbs; avoid unnecessarypleasein technical documentation.
Prefer active voice when the actor matters. Passive voice is acceptable when the actor is unknown or irrelevant, when the action or result is the focus, or in programmer documentation.
Use natural contractions in conversational technical prose, but do not force them in formal legal copy, API references, or generated text.
Prefer simpler English over Latinisms: usefor exampleorsuch asinstead ofe.g.,and so oninstead ofetc.,that isinstead ofi.e.,compared toinstead ofvs., andby,through, orusinginstead ofvia. Use industry-standard terms such as in silico, in vitro, and in vivo when appropriate, and italicize them in running text.
Usethatwithout commas for essential clauses, andwhichwith commas for nonessential clauses.
Format dates and times clearly: spell out months in body text; use forms such asJune 12, 2025; avoid numeric or ordinal dates; capitalize days; use 12-hour time when appropriate; include a space beforea.m.orp.m.; useETandPTfor needed time zones; avoid24/7; and preferfrom 12:30 to 1:00 p.m.for prose ranges.
Format numbers consistently: spell out zero through nine in body text, use numerals for 10 or greater and for technical values, use commas in thousands, do not begin a sentence with a numeral, spell out ordinals, and use numerals consistently within a category wh...
Files:
docs/reference/api/python-library-reference/nemo_fabric.models.md
**/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Update relevant SDK, API reference, adapter, example, integration, and support documentation when the corresponding public surface changes.
Files:
docs/reference/api/python-library-reference/nemo_fabric.models.md
**/*.{html,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
HTML and Markdown files must use the specified SPDX HTML-comment header.
Files:
docs/reference/api/python-library-reference/nemo_fabric.models.md
**/*.{py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If Python code or a Python-facing adapter changes, run
just test-python.In Python SDK, adapters, examples, and tests, follow the existing style, use type annotations for public APIs, and keep native binding declarations synchronized with their Rust implementations.
Files:
tests/python/test_sdk_contract.pypython/src/nemo_fabric/models.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
tests/**/*.{rs,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
When adding functionality, include tests in the corresponding Rust crate or the relevant area under
tests/.
Files:
tests/python/test_sdk_contract.py
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Use pytest to run Python tests.
Do not add@pytest.mark.asyncioto tests; async tests are automatically detected by the async runner.
Do not add-> Nonereturn type annotations to test functions.
When mocking a class, useunittest.mock.MagicMockorAsyncMock, using thespecargument when necessary, rather than defining a new class.
Prefix mocked class names withmock, notfake.
Prefer pytest fixtures over helper methods.
If a fixture is needed in multiple test files, define it once inconftest.pyrather than repeating it.
Define fixtures using@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and a<fixture_name>_fixturefunction; specifyscopeonly when it is notfunction.
Preferpytest.mark.parametrizeover separate tests for different input types.
Use@pytest.mark.usefixtureswhen a fixture is needed but its returned value is unused or it returns no value.
Avoid defensive programming in tests; access expected values directly so missing data raises a clear failure, such as usingresults["data"]instead ofresults.get("data").
When adapter installation metadata changes, packaging metadata tests must directly assert that the root project depends unconditionally on the exact-versionnemo-fabric-runtimedistribution.
Packaging metadata tests must verify that each root harness extra delegates to the matching version of the leaf adapter'sharnessextra.
Packaging metadata tests must verify that bare leaf dependencies remain adapter-owned and that the rootadapter-testsdependency group installs each leaf through itsharnessextra.
Packaging metadata tests must verify that every leaf providesfull; only adapters importing NeMo Relay Python APIs providerelay, while adapters using an external Relay executable havefullequal toharness.
Files:
tests/python/test_sdk_contract.py
{tests/**,python/tests/**}
⚙️ CodeRabbit configuration file
{tests/**,python/tests/**}: Tests should cover the behavior promised by the changed API surface, including error paths, lifecycle cleanup, and SDK/native parity where relevant.
Files:
tests/python/test_sdk_contract.py
python/src/nemo_fabric/**/*.py
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/SKILL.md)
For Python API reference changes, update source docstrings under
python/src/nemo_fabric/instead of generated API reference files.
Files:
python/src/nemo_fabric/models.py
python/src/nemo_fabric/**/*.{py,rs}
📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md)
Ensure native extension naming and placement remain compatible with downstream consumers, including the editable maturin build producing
nemo_fabric._native.
Files:
python/src/nemo_fabric/models.py
python/src/nemo_fabric/**/*
⚙️ CodeRabbit configuration file
python/src/nemo_fabric/**/*: Review Python SDK changes for typed API consistency, import-time dependency neutrality, async/session behavior, and parity with the native extension.
Stubs and runtime implementations should stay aligned.
Files:
python/src/nemo_fabric/models.py
adapters/*/src/**/*.py
📄 CodeRabbit inference engine (.agents/skills/contribute-adapter/SKILL.md)
adapters/*/src/**/*.py: Implement adapters using the existing Fabricpythonorprocessrunner and normalized request/result contracts; do not add a runner or one-off abstraction for a single adapter.
Treat normalizedconfig, Fabric-resolved plans, andruntime_contextas authoritative; reserveharness.settingsfor adapter-wide behavior and apply precedence in the order: normalized config, plans/context, harness settings, descriptor/default values.
Reject conflicting duplicate declarations and unsupported behavior with actionable errors naming the field and supported alternatives; never silently drop configuration.
Validate dependency versions, hooks, and credentials before harness invocation, and never expose credential values in outputs, errors, events, logs, or fixtures.
Forward only required system, selected credential, telemetry, and documented harness-specific environment variables; never forward or log unrelated environment values.
Maintain one local adapter host per Fabric runtime across orderedstart→invoke*→stopoperations. Emit one JSON lifecycle response per request on stdout and diagnostics on stderr.
Return harness-level invocation failures as successful lifecycle responses containingresponse: null,failed: true, and structurederrorfields (code,message,retryable, and optionalmetadata).
Do not emit NeMo Relay stream records on adapter stdout; return exactly one terminal lifecycle response while streaming occurs through the SDK-owned out-of-band endpoint.
Scope workspace, generated configuration, state, sessions, and artifacts to the resolved runtime context, and isolate stateful adapter instances by Fabric runtime ID.
Usestartto initialize adapter-owned harness state, retain it for continuation across repeatedinvokecalls on the same runtime, and release it instop.
Files:
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
{adapters/**,examples/**}
⚙️ CodeRabbit configuration file
{adapters/**,examples/**}: Review adapter and example changes for command correctness, config/schema consistency, artifact handling, and compatibility with the public NeMo Fabric contracts.
Files:
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{json,jsonschema}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Public contract changes must keep checked-in JSON Schema snapshots synchronized.
Files:
schemas/run-plan.schema.json
schemas/**/*
⚙️ CodeRabbit configuration file
schemas/**/*: Schemas are generated public contract snapshots. Check that schema diffs correspond to intentional Rust type changes and are covered by core tests.
Files:
schemas/run-plan.schema.json
🧠 Learnings (3)
📚 Learning: 2026-07-24T16:07:22.255Z
Learnt from: AjayThorve
Repo: NVIDIA/NeMo-Fabric PR: 118
File: docs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-adapterdescriptor.mdx:5-5
Timestamp: 2026-07-24T16:07:22.255Z
Learning: In this repo, files generated under `docs/reference/api/**` are NVIDIA NeMo Fabric API reference output. When reviewing changes to these generated pages, do not treat sidebar `position`/ordering updates as direct manual edits—these can be regenerated by running `just docs` after adding public types. Only flag substantive content changes that are not explained by generation.
Applied to files:
docs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-mcpserverplan.mdx
📚 Learning: 2026-06-28T04:03:32.877Z
Learnt from: AjayThorve
Repo: NVIDIA/NeMo-Fabric PR: 26
File: python/tests/smoke_typed_config.py:163-177
Timestamp: 2026-06-28T04:03:32.877Z
Learning: In NVIDIA NeMo Fabric Python SDK serialization of `RuntimeCapabilities` (to satisfy the “parity contract” with Rust core and the CLI), do not emit metadata keys when the corresponding metadata is absent. Instead, omit those fields entirely so the produced JSON matches the Rust/CLI output (e.g., avoid `null`, empty objects, or placeholder metadata). During review, verify the serializer/builders follow this omission rule and that Python outputs/parity tests reflect the same shape.
Applied to files:
python/src/nemo_fabric/models.py
📚 Learning: 2026-07-09T22:28:51.689Z
Learnt from: AjayThorve
Repo: NVIDIA/NeMo-Fabric PR: 43
File: adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py:164-168
Timestamp: 2026-07-09T22:28:51.689Z
Learning: In the NeMo-Fabric adapters, treat path values used in Fabric adapter configuration (including logic like `_resolve_path` in adapter.py) as config-root-relative. Do not apply `Path.expanduser()` (or otherwise apply `~`/home or shell-style expansion), because it will make the resolved paths normalize inconsistently across adapters. Also, do not rely on or add any resolution behavior that uses `harness.settings.cwd` as an override point for these adapter paths—`harness.settings.cwd` is explicitly unsupported in this adapter context.
Applied to files:
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
🪛 Ruff (0.16.1)
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
[warning] 393-393: Async function definition with a timeout parameter
(ASYNC109)
🔇 Additional comments (12)
docs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-mcpserverplan.mdx (1)
12-18: LGTM!Also applies to: 34-41
schemas/run-plan.schema.json (2)
1300-1424: Authentication variants still accept unknown fields.Neither
oneOfbranch setsadditionalProperties: false. A misplaced field such astoken_urlundertype: "oauth2", ortoken_cache_buffer_secondsundertype: "service_account", validates successfully and is then discarded. This is the same gap flagged on the RustMcpAuthenticationConfigvariants; fix it incrates/fabric-core/src/config.rsand regenerate the snapshot.
1512-1519: LGTM!Also applies to: 1549-1599, 1601-1620, 1688-1707
python/src/nemo_fabric/models.py (2)
426-428: Theauthenticationannotation is narrower than the accepted input.
tests/python/test_sdk_contract.pyLine 346-361 passes a plain mapping, and pydantic coerces it. A static type checker rejects that form. Widen the annotation the same wayrelaymodels its dual form.
413-413: LGTM!crates/fabric-core/src/config.rs (2)
2465-2475: LGTM!Also applies to: 2929-2956
3169-3169: LGTM!Also applies to: 3183-3252, 3300-3413, 3803-3995
crates/fabric-core/src/agent_config.rs (1)
15-15: LGTM!Also applies to: 321-326
tests/python/test_sdk_contract.py (1)
334-343: LGTM!adapters/claude/src/nemo_fabric_adapters/claude/adapter.py (3)
338-344: LGTM!Also applies to: 373-385
563-563: LGTM!Also applies to: 1091-1091, 1109-1112, 1347-1347
1157-1170: 🩺 Stability & AvailabilityNo change needed: MCP configuration errors are caught.
AdapterConfigErrorsubclassesClaudeAdapterError, and_mcp_oauth_configconvertsMcpAuthConfigErrorbefore it reaches this handler.> Likely an incorrect or invalid review comment.
| async def _self_authenticate_http_mcp_server( | ||
| server_name: str, | ||
| server_url: str, | ||
| config: mcp_auth.McpOAuth2Config, | ||
| *, | ||
| timeout: float, | ||
| ) -> str: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Ruff flags the timeout parameter (ASYNC109).
The rule prefers the caller to bound the operation with asyncio.timeout instead of the coroutine accepting its own timeout argument. If the ASYNC ruleset runs in CI, this fails lint.
Either move the asyncio.timeout wrapper to _authenticate_mcp_servers at Line 1232-1237, or accept an absolute deadline instead of a duration.
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 393-393: Async function definition with a timeout parameter
(ASYNC109)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@adapters/claude/src/nemo_fabric_adapters/claude/adapter.py` around lines 388
- 394, Remove the duration-based timeout parameter from
_self_authenticate_http_mcp_server and apply the operation timeout at its
caller, _authenticate_mcp_servers, using asyncio.timeout. Update the call and
preserve the existing timeout duration and authentication behavior while
satisfying Ruff ASYNC109.
Source: Linters/SAST tools
| context = getattr(provider, "context", None) | ||
| tokens = getattr(context, "current_tokens", None) if context is not None else None | ||
| access_token: str | None = getattr(tokens, "access_token", None) if tokens is not None else None | ||
| if not access_token: | ||
| raise ClaudeAdapterError( | ||
| "claude_mcp_authentication_failed", | ||
| f"MCP server {server_name!r} OAuth flow did not return an access token", | ||
| metadata={"server": server_name}, | ||
| ) | ||
| return access_token |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the token reach-through is duplicated across adapters.
set -uo pipefail
echo '--- current_tokens / access_token usage in adapters ---'
rg -n -C4 'current_tokens|access_token' adapters
echo '--- shared provider factory ---'
rg -n -C8 'def create_mcp_oauth_provider' adapters/common/src/nemo_fabric_adapters/common/mcp_auth.pyRepository: NVIDIA/NeMo-Fabric
Length of output: 8803
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- OAuth provider consumers ---'
rg -n -C6 'create_mcp_oauth_provider|current_tokens|access_token' adapters --glob '*.py'
echo '--- shared authentication module structure ---'
sed -n '1,120p' adapters/common/src/nemo_fabric_adapters/common/mcp_auth.py
sed -n '480,570p' adapters/common/src/nemo_fabric_adapters/common/mcp_auth.py
echo '--- adapter package dependencies and tests ---'
rg -n -C3 'mcp|oauth|auth' adapters/{claude,codex,hermes,common} --glob 'pyproject.toml' --glob '*.py' --glob '*.pyi'Repository: NVIDIA/NeMo-Fabric
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import ast
root = Path("adapters")
for path in sorted(root.rglob("*.py")):
tree = ast.parse(path.read_text(encoding="utf-8"))
calls = [
node.lineno
for node in ast.walk(tree)
if isinstance(node, ast.Call)
and isinstance(node.func, ast.Attribute)
and node.func.attr == "create_mcp_oauth_provider"
]
reaches = [
node.lineno
for node in ast.walk(tree)
if isinstance(node, ast.Attribute)
and node.attr in {"current_tokens", "access_token"}
]
if calls or reaches:
print(f"{path}: create_provider={calls}, token_attributes={reaches}")
PY
echo '--- factory return and provider construction ---'
sed -n '513,610p' adapters/common/src/nemo_fabric_adapters/common/mcp_auth.py
echo '--- exact consumers in Codex and Hermes ---'
for adapter in codex hermes; do
echo "[$adapter]"
rg -n 'create_mcp_oauth_provider|current_tokens|access_token|OAuthClientProvider' "adapters/$adapter" --glob '*.py' || true
done
echo '--- MCP dependency pins ---'
rg -n -C2 'mcp([<>=]|")|mcp-client' --glob 'pyproject.toml' --glob '*lock*' --glob '*.txt' .Repository: NVIDIA/NeMo-Fabric
Length of output: 12983
🌐 Web query:
MCP Python SDK OAuthClientProvider current_tokens access_token mcp 1.26 1.28 source
💡 Result:
In the Model Context Protocol (MCP) Python SDK, the OAuthClientProvider is designed as an httpx.Auth implementation that handles OAuth2 authentication, including client registration and token management [1][2][3]. The current_tokens attribute and the access_token field are core components of the SDK's token management system [4][5][6]. Key Technical Details: 1. Token Structure: The OAuthToken class contains the access_token, refresh_token, expiry information, and scope [6]. 2. Provider Role: OAuthClientProvider is not an MCP-specific object but rather an httpx.Auth hook used to automatically manage Authorization headers [1][2]. It uses an internal OAuthContext to manage the token lifecycle [5][3]. 3. Token Lifecycle: When a request is made, the provider checks for a valid access_token [4][5]. If the token is expired or missing, it may attempt a refresh if a refresh_token is available [1]. 4. Version Context (1.26 - 1.28): - The SDK's v1.x branch (which includes versions 1.26 through 1.28) remains in maintenance mode [7]. - Version 1.28 introduced specific updates, such as adding subject and claims to the AccessToken [8][9]. - Known issues regarding token management, such as the behavior of token_expiry_time causing failures in proactive refreshes, have been identified and addressed in recent maintenance [10]. Specifically, if token_expiry_time is not correctly set (e.g., when loading from persistent storage), the SDK might treat tokens as perpetually valid or fail to trigger a refresh before a request receives a 401 response [10]. Ensuring that stored tokens are properly initialized with their expiry time is critical for stable authentication [10]. If you are encountering issues with token persistence or refreshes between these versions, ensure that your TokenStorage implementation is correctly loading and setting token expiry times upon initialization [10].
Citations:
- 1: https://py.sdk.modelcontextprotocol.io/v2/client/oauth-clients/
- 2: https://py.sdk.modelcontextprotocol.io/api/mcp/client/auth/oauth2/
- 3: https://github.com/modelcontextprotocol/python-sdk/blob/c7cbfbb3/src/mcp/client/auth/oauth2.py
- 4: https://py.sdk.modelcontextprotocol.io/v2/api/mcp/client/auth/oauth2/
- 5: https://github.com/modelcontextprotocol/python-sdk/blob/e8e64842/src/mcp/client/auth/oauth2.py
- 6: https://py.sdk.modelcontextprotocol.io/api/mcp/shared/auth/
- 7: https://pypi.org/project/mcp/1.28.1/
- 8: modelcontextprotocol/python-sdk@v1.27.0...v1.28.1
- 9: modelcontextprotocol/python-sdk@v1.14.1...v1.28.1
- 10: Critical Token Refresh Bugs - Prevents Proactive Refresh modelcontextprotocol/python-sdk#1318
Centralize MCP OAuth token extraction.
Claude is the only adapter that currently reads provider.context.current_tokens.access_token. Add mcp_auth.access_token(provider) and use it here so MCP SDK shape changes require one update in the shared module.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@adapters/claude/src/nemo_fabric_adapters/claude/adapter.py` around lines 439
- 448, Centralize MCP OAuth token extraction by adding the shared
mcp_auth.access_token(provider) helper and replacing the local
context/current_tokens/access_token traversal in the Claude adapter’s OAuth
flow. Preserve the existing missing-token ClaudeAdapterError behavior and
metadata after using the helper.
| tmp_path = config_path.parent / (config_path.name + ".tmp") | ||
| try: | ||
| config = json.loads(config_path.read_text(encoding="utf-8")) | ||
| server_entry = config.setdefault("mcpServers", {}).setdefault(server_name, {}) | ||
| server_entry.setdefault("headers", {})["Authorization"] = f"Bearer {token}" | ||
| descriptor = os.open(tmp_path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) | ||
| try: | ||
| with os.fdopen(descriptor, "w", encoding="utf-8") as stream: | ||
| json.dump(config, stream, indent=2, sort_keys=True) | ||
| stream.write("\n") | ||
| except BaseException: | ||
| tmp_path.unlink(missing_ok=True) | ||
| raise | ||
| os.replace(tmp_path, config_path) | ||
| except OSError as error: | ||
| raise ClaudeAdapterError( | ||
| "claude_mcp_configuration_failed", | ||
| f"MCP server {server_name!r} authorization header could not be staged", | ||
| metadata={"server": server_name}, | ||
| ) from error |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Only OSError is converted to a structured failure.
json.loads at Line 465 raises json.JSONDecodeError, and config.setdefault("mcpServers", {}) at Line 466 raises AttributeError if mcpServers holds a non-mapping value. Neither is an OSError, so both escape this function unchanged.
invoke at Line 1162-1170 catches only ClaudeAdapterError, TimeoutError, and ClaudeSDKError. An escaping ValueError or AttributeError therefore bypasses the structured-failure path.
As per coding guidelines: "Return harness-level invocation failures as successful lifecycle responses containing response: null, failed: true, and structured error fields."
🛡️ Proposed exception handling
- except OSError as error:
+ except (OSError, ValueError, TypeError, AttributeError) as error:
raise ClaudeAdapterError(
"claude_mcp_configuration_failed",
f"MCP server {server_name!r} authorization header could not be staged",
metadata={"server": server_name},
) from error📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tmp_path = config_path.parent / (config_path.name + ".tmp") | |
| try: | |
| config = json.loads(config_path.read_text(encoding="utf-8")) | |
| server_entry = config.setdefault("mcpServers", {}).setdefault(server_name, {}) | |
| server_entry.setdefault("headers", {})["Authorization"] = f"Bearer {token}" | |
| descriptor = os.open(tmp_path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) | |
| try: | |
| with os.fdopen(descriptor, "w", encoding="utf-8") as stream: | |
| json.dump(config, stream, indent=2, sort_keys=True) | |
| stream.write("\n") | |
| except BaseException: | |
| tmp_path.unlink(missing_ok=True) | |
| raise | |
| os.replace(tmp_path, config_path) | |
| except OSError as error: | |
| raise ClaudeAdapterError( | |
| "claude_mcp_configuration_failed", | |
| f"MCP server {server_name!r} authorization header could not be staged", | |
| metadata={"server": server_name}, | |
| ) from error | |
| tmp_path = config_path.parent / (config_path.name + ".tmp") | |
| try: | |
| config = json.loads(config_path.read_text(encoding="utf-8")) | |
| server_entry = config.setdefault("mcpServers", {}).setdefault(server_name, {}) | |
| server_entry.setdefault("headers", {})["Authorization"] = f"Bearer {token}" | |
| descriptor = os.open(tmp_path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) | |
| try: | |
| with os.fdopen(descriptor, "w", encoding="utf-8") as stream: | |
| json.dump(config, stream, indent=2, sort_keys=True) | |
| stream.write("\n") | |
| except BaseException: | |
| tmp_path.unlink(missing_ok=True) | |
| raise | |
| os.replace(tmp_path, config_path) | |
| except (OSError, ValueError, TypeError, AttributeError) as error: | |
| raise ClaudeAdapterError( | |
| "claude_mcp_configuration_failed", | |
| f"MCP server {server_name!r} authorization header could not be staged", | |
| metadata={"server": server_name}, | |
| ) from error |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@adapters/claude/src/nemo_fabric_adapters/claude/adapter.py` around lines 463
- 482, Update the MCP configuration handling around config_path and server_entry
so JSON decoding and invalid mcpServers structure failures are converted into
ClaudeAdapterError, not only OSError. Ensure invoke receives the structured
failure and returns the existing harness-level response shape with response:
null, failed: true, and structured error fields.
Source: Coding guidelines
| if self._mcp_authentication_checked: | ||
| return | ||
|
|
||
| servers = _authenticated_mcp_servers(payload) | ||
| if servers and self._mcp_config_path is None: | ||
| raise ClaudeAdapterError( | ||
| "claude_mcp_configuration_failed", | ||
| "MCP staged configuration is unavailable for OAuth authentication", | ||
| ) | ||
|
|
||
| for name, server in servers.items(): | ||
| authentication = _mcp_authentication(payload, name) | ||
| auth_timeout = min(invocation_timeout, authentication.authorization_timeout_seconds) | ||
| token = await _self_authenticate_http_mcp_server( | ||
| name, | ||
| server["url"], | ||
| authentication, | ||
| timeout=auth_timeout, | ||
| ) | ||
| assert self._mcp_config_path is not None | ||
| _update_staged_mcp_server_header(self._mcp_config_path, name, token) | ||
| await client.reconnect_mcp_server(name) | ||
| status = await self._mcp_server_status(client, name) | ||
| if status != "connected": | ||
| raise ClaudeAdapterError( | ||
| "claude_mcp_unavailable", | ||
| f"Claude MCP server {name!r} is unavailable after OAuth authentication", | ||
| metadata={"server": name, "status": status}, | ||
| ) | ||
|
|
||
| self._mcp_authentication_checked = True |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
A partial failure repeats completed interactive authorizations.
self._mcp_authentication_checked is set only after the whole loop succeeds at Line 1249. If the second server fails, the first server has already completed its OAuth flow and had its bearer header staged. The next invoke restarts the loop from the first server.
For an authorization-code flow this re-prompts the user for a server that is already authenticated. Track completed servers instead of a single boolean, and skip them on retry. Reset that set in stop alongside Line 1347.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@adapters/claude/src/nemo_fabric_adapters/claude/adapter.py` around lines 1219
- 1249, Replace the single _mcp_authentication_checked flag with a set tracking
completed MCP server names in the authentication flow. In the loop around
_self_authenticate_http_mcp_server, skip servers already in that set and add
each server only after its authentication, staged-header update, reconnect, and
connected-status validation succeed. Reset the completed-server set in stop
alongside the existing authentication state.
| for name, server in servers.items(): | ||
| authentication = _mcp_authentication(payload, name) | ||
| auth_timeout = min(invocation_timeout, authentication.authorization_timeout_seconds) | ||
| token = await _self_authenticate_http_mcp_server( | ||
| name, | ||
| server["url"], | ||
| authentication, | ||
| timeout=auth_timeout, | ||
| ) | ||
| assert self._mcp_config_path is not None | ||
| _update_staged_mcp_server_header(self._mcp_config_path, name, token) | ||
| await client.reconnect_mcp_server(name) | ||
| status = await self._mcp_server_status(client, name) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Authentication and the query each get a full invocation timeout.
Line 1231 bounds authentication by invocation_timeout. _run_query at Line 1291 then opens a fresh asyncio.timeout(invocation_timeout). Authentication runs inside invoke, so a caller that sets runtime.timeout_seconds to 60 can wait up to 120 seconds, plus the 5-second status poll per server at Line 1257. With several authenticated servers the overrun grows further.
schemas/run-plan.schema.json documents timeout_seconds as "Maximum duration of one invocation in seconds". The current behavior breaks that bound.
Compute one deadline at the start of invoke and derive both the authentication budget and the query budget from the remaining time.
⏱️ Proposed deadline handling
try:
prompt = request_prompt(payload)
invocation_timeout = timeout_seconds(payload)
+ deadline = asyncio.get_running_loop().time() + invocation_timeout
await self._authenticate_mcp_servers(
payload,
client,
- invocation_timeout,
+ deadline,
) async def _authenticate_mcp_servers(
self,
payload: dict[str, Any],
client: ClaudeSDKClient,
- invocation_timeout: float,
+ deadline: float,
) -> None: for name, server in servers.items():
authentication = _mcp_authentication(payload, name)
- auth_timeout = min(invocation_timeout, authentication.authorization_timeout_seconds)
+ remaining = deadline - asyncio.get_running_loop().time()
+ if remaining <= 0:
+ raise TimeoutError
+ auth_timeout = min(remaining, authentication.authorization_timeout_seconds)Then pass the remaining budget to _run_query instead of invocation_timeout.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@adapters/claude/src/nemo_fabric_adapters/claude/adapter.py` around lines 1229
- 1241, Update invoke to compute one absolute deadline at its start, then derive
each authentication timeout and the query timeout from the remaining invocation
budget rather than resetting to invocation_timeout. Pass that remaining budget
into _run_query, including accounting for per-server status polling, so the full
invoke flow remains bounded by runtime.timeout_seconds.
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
…g the loop Signed-off-by: David Gardner <dagardner@nvidia.com>
1 participant
Overview
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Closes FABRIC-170
I confirm this contribution is my own work, or I have the right to submit it under this project's license.
I searched existing issues and open pull requests, and this does not duplicate existing work.
Summary by CodeRabbit
New Features
Documentation
Tests