feat: Misc MCP improvements - #177
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>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe adapters now map stdio MCP servers from separate command, argument, and environment fields. Hermes performs MCP discovery and shutdown asynchronously. Mock API streaming support and end-to-end tests cover enabled and disabled MCP servers. ChangesMCP stdio transport
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant E2ETest
participant Adapter
participant MultiServerMCPClient
participant TimeMCPServer
participant MockAPI
E2ETest->>MockAPI: Configure model response
E2ETest->>Adapter: Run MCP-enabled request
Adapter->>MultiServerMCPClient: Pass command, args, and env
MultiServerMCPClient->>TimeMCPServer: Start stdio server
Adapter->>MockAPI: Send tool-enabled request
MockAPI-->>Adapter: Stream tool-call events
Adapter->>TimeMCPServer: Invoke get_current_time
TimeMCPServer-->>Adapter: Return tool result
Adapter->>MockAPI: Submit tool result
MockAPI-->>Adapter: Stream final response
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Fern docs preview: https://nvidia-preview-pull-request-177.docs.buildwithfern.com/nemo/fabric |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/codex/src/nemo_fabric_adapters/codex/adapter.py`:
- Around line 191-192: Validate the MCP server env field as a string-to-string
mapping before forwarding it: in
adapters/codex/src/nemo_fabric_adapters/codex/adapter.py:191-192 and
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py:307-308, use _mapping
with the field-specific name “MCP server {name} env” and reject non-string keys
or values before assignment; in
adapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.py:293-294,
raise AdapterConfigError naming the server for any non-mapping or non-string
entry before assigning connection["env"]. Add rejection coverage for a
non-mapping env alongside the positive adapter tests.
In `@adapters/hermes/pyproject.toml`:
- Around line 34-40: Document the rationale for the hermes-agent dependency in
the relevant PR or packaging documentation, covering its functional need,
alternatives considered, and why the >=0.19.0 constraint is the narrowest fit.
Ensure the rationale applies consistently to both the relay and full extras.
In `@tests/e2e/test_claude.py`:
- Around line 182-191: Replace the blocking requests.post scenario setup in
tests/e2e/test_claude.py lines 182-191, tests/e2e/test_codex.py lines 29-39, and
tests/e2e/test_deepagents.py lines 116-125 with non-blocking httpx.AsyncClient
calls inside the async tests, preserving the namespace field in the Codex
request body. Remove the now-unused requests imports from all three files and
verify httpx is available as a test dependency.
- Around line 201-207: Ensure the test setup for mcp_server_time is reliable by
confirming mcp-server-time is declared in the test dependencies; if it is
optional or unavailable, add an importorskip guard near the test module setup
before config.add_mcp_server is used. Preserve the existing server configuration
and ensure missing-module cases skip cleanly rather than failing assertions.
🪄 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: f72b9558-0ede-4a6e-8423-20b53e06f25f
⛔ Files ignored due to path filters (2)
adapters/hermes/uv.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
adapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex/src/nemo_fabric_adapters/codex/adapter.pyadapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.pyadapters/hermes/pyproject.tomladapters/hermes/src/nemo_fabric_adapters/hermes/adapter.pytests/_utils/mock_api_server.pytests/adapters/test_adapter_package_metadata.pytests/adapters/test_claude_adapter.pytests/adapters/test_codex_adapter.pytests/adapters/test_deepagents.pytests/adapters/test_hermes_adapter.pytests/e2e/test_claude.pytests/e2e/test_codex.pytests/e2e/test_deepagents.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.11, linux-arm64)
- GitHub Check: Test (Python 3.13, windows-amd64)
- GitHub Check: Test (Python 3.11, linux-amd64)
- GitHub Check: Test (Python 3.13, linux-amd64)
- GitHub Check: Test (Python 3.11, windows-amd64)
- GitHub Check: Test (Python 3.14, macos-arm64)
- GitHub Check: Test (Python 3.13, linux-arm64)
- GitHub Check: Test (Python 3.12, linux-arm64)
- GitHub Check: Test (Python 3.14, windows-amd64)
- GitHub Check: Test (Python 3.14, linux-amd64)
- GitHub Check: Test (Python 3.11, macos-arm64)
- GitHub Check: Test (Python 3.13, macos-arm64)
- GitHub Check: Test (Python 3.14, linux-arm64)
- GitHub Check: Test (Python 3.12, macos-arm64)
- GitHub Check: Test (Python 3.12, linux-amd64)
- GitHub Check: Pre-commit
🧰 Additional context used
📓 Path-based instructions (27)
**/*
📄 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/hermes/pyproject.tomltests/adapters/test_adapter_package_metadata.pyadapters/hermes/src/nemo_fabric_adapters/hermes/adapter.pytests/adapters/test_deepagents.pyadapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.pytests/adapters/test_codex_adapter.pytests/e2e/test_claude.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex/src/nemo_fabric_adapters/codex/adapter.pytests/adapters/test_hermes_adapter.pytests/e2e/test_codex.pytests/adapters/test_claude_adapter.pytests/_utils/mock_api_server.pytests/e2e/test_deepagents.py
**/*.{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:
adapters/hermes/pyproject.toml
**/{Cargo.toml,Cargo.lock,pyproject.toml,package.json}
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
For new or updated dependencies, document the functional need, alternatives considered, and why the selected dependency is the narrowest fit.
Files:
adapters/hermes/pyproject.toml
**/*.{toml,lock}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a manifest or lockfile changes, run the license-diff script against
origin/main, review transitive license changes, and run theattributions-rustandattributions-pythonpre-commit hooks.
Files:
adapters/hermes/pyproject.toml
**/*.{yml,yaml,toml,lock}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
For CI or packaging changes, use
maintain-ciormaintain-packaging, then run recipes and checks whose behavior changed.
Files:
adapters/hermes/pyproject.toml
{pyproject.toml,adapters/**/pyproject.toml}
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
{pyproject.toml,adapters/**/pyproject.toml}: Update the literalproject.versionin the root setuptools project and every adapterpyproject.toml.
Keep internal exact-version requirements aligned: rootnemo-fabric-* == <version>optional dependencies and each adapter'snemo-fabric-adapters-common == <version>dependency.
Files:
adapters/hermes/pyproject.toml
**/*.{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/hermes/pyproject.tomltests/adapters/test_adapter_package_metadata.pyadapters/hermes/src/nemo_fabric_adapters/hermes/adapter.pytests/adapters/test_deepagents.pyadapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.pytests/adapters/test_codex_adapter.pytests/e2e/test_claude.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex/src/nemo_fabric_adapters/codex/adapter.pytests/adapters/test_hermes_adapter.pytests/e2e/test_codex.pytests/adapters/test_claude_adapter.pytests/_utils/mock_api_server.pytests/e2e/test_deepagents.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/hermes/pyproject.tomltests/adapters/test_adapter_package_metadata.pyadapters/hermes/src/nemo_fabric_adapters/hermes/adapter.pytests/adapters/test_deepagents.pyadapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.pytests/adapters/test_codex_adapter.pytests/e2e/test_claude.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex/src/nemo_fabric_adapters/codex/adapter.pytests/adapters/test_hermes_adapter.pytests/e2e/test_codex.pytests/adapters/test_claude_adapter.pytests/_utils/mock_api_server.pytests/e2e/test_deepagents.py
**/*.{toml,yaml,yml,sh,bash}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
TOML, YAML, and shell files must use the specified
#SPDX copyright and Apache-2.0 license headers.
Files:
adapters/hermes/pyproject.toml
**/{Cargo.toml,pyproject.toml}
📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md)
**/{Cargo.toml,pyproject.toml}: Keep package names, dependency declarations, import paths, module names, and workspace or Python package metadata internally consistent.
Prefer the standard library, an existing dependency, or a small local implementation before adding a new direct dependency.
When multiple dependencies satisfy the requirement, prefer a maintained OSS option with clear SPDX metadata, a smaller transitive graph, and permissive licensing such as Apache-2.0, MIT, BSD, or ISC.
Files:
adapters/hermes/pyproject.toml
**/{Cargo.toml,pyproject.toml,Cargo.lock,uv.lock}
📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md)
**/{Cargo.toml,pyproject.toml,Cargo.lock,uv.lock}: For new dependencies, record the functional need, viable alternatives considered, why the selected dependency is the narrowest fit, and any unresolved licensing question.
After updating manifests or lockfiles, runuv run --no-project python scripts/licensing/license_diff.py --base-ref origin/mainand review added packages and license changes.
Keep workspace, Python package, and lockfile versions aligned wherever the packaging contract requires alignment.
Files:
adapters/hermes/pyproject.toml
adapters/**/*
📄 CodeRabbit inference engine (.agents/skills/contribute-adapter/SKILL.md)
Place repository adapters under
adapters/<name>, define their install extra and packaged descriptor, select the matchingharness.resolutionstrategy, and document repository orbase_dirdescriptor discovery.
Files:
adapters/hermes/pyproject.tomladapters/hermes/src/nemo_fabric_adapters/hermes/adapter.pyadapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex/src/nemo_fabric_adapters/codex/adapter.py
adapters/*/{pyproject.toml,package.json,*.lock}
📄 CodeRabbit inference engine (.agents/skills/contribute-adapter/SKILL.md)
Keep each published leaf adapter's runtime dependencies razor-thin and adapter-owned. Do not directly declare the wrapped harness/SDK or dependencies already declared by its supported package; provide and test a dependency-free fallback for optional libraries.
Files:
adapters/hermes/pyproject.toml
adapters/*/pyproject.toml
📄 CodeRabbit inference engine (.agents/skills/contribute-adapter/SKILL.md)
Limit a leaf adapter's base installation to adapter-owned dependencies; provide a
harnessextra, afullextra for package-installable optional integrations, and arelayextra when it imports NeMo Relay Python APIs. If Relay is external, omitrelayand makefullequivalent toharness.
Files:
adapters/hermes/pyproject.toml
{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/hermes/pyproject.tomladapters/hermes/src/nemo_fabric_adapters/hermes/adapter.pyadapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex/src/nemo_fabric_adapters/codex/adapter.py
**/*.{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_adapter_package_metadata.pyadapters/hermes/src/nemo_fabric_adapters/hermes/adapter.pytests/adapters/test_deepagents.pyadapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.pytests/adapters/test_codex_adapter.pytests/e2e/test_claude.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex/src/nemo_fabric_adapters/codex/adapter.pytests/adapters/test_hermes_adapter.pytests/e2e/test_codex.pytests/adapters/test_claude_adapter.pytests/_utils/mock_api_server.pytests/e2e/test_deepagents.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_adapter_package_metadata.pyadapters/hermes/src/nemo_fabric_adapters/hermes/adapter.pytests/adapters/test_deepagents.pyadapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.pytests/adapters/test_codex_adapter.pytests/e2e/test_claude.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex/src/nemo_fabric_adapters/codex/adapter.pytests/adapters/test_hermes_adapter.pytests/e2e/test_codex.pytests/adapters/test_claude_adapter.pytests/_utils/mock_api_server.pytests/e2e/test_deepagents.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_adapter_package_metadata.pyadapters/hermes/src/nemo_fabric_adapters/hermes/adapter.pytests/adapters/test_deepagents.pyadapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.pytests/adapters/test_codex_adapter.pytests/e2e/test_claude.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex/src/nemo_fabric_adapters/codex/adapter.pytests/adapters/test_hermes_adapter.pytests/e2e/test_codex.pytests/adapters/test_claude_adapter.pytests/_utils/mock_api_server.pytests/e2e/test_deepagents.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_adapter_package_metadata.pyadapters/hermes/src/nemo_fabric_adapters/hermes/adapter.pytests/adapters/test_deepagents.pyadapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.pytests/adapters/test_codex_adapter.pytests/e2e/test_claude.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex/src/nemo_fabric_adapters/codex/adapter.pytests/adapters/test_hermes_adapter.pytests/e2e/test_codex.pytests/adapters/test_claude_adapter.pytests/_utils/mock_api_server.pytests/e2e/test_deepagents.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_adapter_package_metadata.pytests/adapters/test_deepagents.pytests/adapters/test_codex_adapter.pytests/adapters/test_hermes_adapter.pytests/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_adapter_package_metadata.pyadapters/hermes/src/nemo_fabric_adapters/hermes/adapter.pytests/adapters/test_deepagents.pyadapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.pytests/adapters/test_codex_adapter.pytests/e2e/test_claude.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex/src/nemo_fabric_adapters/codex/adapter.pytests/adapters/test_hermes_adapter.pytests/e2e/test_codex.pytests/adapters/test_claude_adapter.pytests/_utils/mock_api_server.pytests/e2e/test_deepagents.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_adapter_package_metadata.pyadapters/hermes/src/nemo_fabric_adapters/hermes/adapter.pytests/adapters/test_deepagents.pyadapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.pytests/adapters/test_codex_adapter.pytests/e2e/test_claude.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex/src/nemo_fabric_adapters/codex/adapter.pytests/adapters/test_hermes_adapter.pytests/e2e/test_codex.pytests/adapters/test_claude_adapter.pytests/_utils/mock_api_server.pytests/e2e/test_deepagents.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_adapter_package_metadata.pytests/adapters/test_deepagents.pytests/adapters/test_codex_adapter.pytests/e2e/test_claude.pytests/adapters/test_hermes_adapter.pytests/e2e/test_codex.pytests/adapters/test_claude_adapter.pytests/_utils/mock_api_server.pytests/e2e/test_deepagents.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_adapter_package_metadata.pytests/adapters/test_deepagents.pytests/adapters/test_codex_adapter.pytests/e2e/test_claude.pytests/adapters/test_hermes_adapter.pytests/e2e/test_codex.pytests/adapters/test_claude_adapter.pytests/_utils/mock_api_server.pytests/e2e/test_deepagents.py
tests/adapters/test_*.py
📄 CodeRabbit inference engine (.agents/skills/contribute-adapter/SKILL.md)
tests/adapters/test_*.py: Add focused tests for descriptor shape and exact capabilities, positive normalized-surface mappings, rejection of unsupported values and unenforceable policies, result normalization without secret leakage, one-shot execution, continuation, runtime isolation, and the packaged subprocess entry point.
Provide a credential-free fixture coveringplan,doctor, andrun; keep credential-dependent live-harness tests opt-in while retaining a deterministic CI end-to-end path.
Files:
tests/adapters/test_adapter_package_metadata.pytests/adapters/test_deepagents.pytests/adapters/test_codex_adapter.pytests/adapters/test_hermes_adapter.pytests/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_adapter_package_metadata.pytests/adapters/test_deepagents.pytests/adapters/test_codex_adapter.pytests/e2e/test_claude.pytests/adapters/test_hermes_adapter.pytests/e2e/test_codex.pytests/adapters/test_claude_adapter.pytests/_utils/mock_api_server.pytests/e2e/test_deepagents.py
adapters/**/*.{py,js,ts,java,go,rs}
📄 CodeRabbit inference engine (.agents/skills/contribute-adapter/SKILL.md)
adapters/**/*.{py,js,ts,java,go,rs}: Use the existing Fabricpythonorprocessrunner and normalized request/result contracts. Reuseadapters/common/only when its contract fits; do not add a runner or abstraction for one adapter.
For adapters consuming normalized configuration or runtime context, use the complete Fabric invocation and treatconfig,capability_plan,telemetry_plan, andruntime_contextas authoritative. Reserveharness.settingsfor harness-specific behavior.
Apply configuration precedence in this order: normalized config; Fabric-resolved plans and runtime context; harness-specific settings; descriptor and adapter defaults. Reject conflicting duplicate declarations or unsupported behavior with actionable field-specific errors; never silently drop configuration.
Run dependency and authentication preflight before invoking the harness. Declare fixed dependencies in descriptor requirements, validate versions, hooks, and credentials, and never expose credential values in output, errors, events, logs, or fixtures.
Forward only required system variables, selected credential variables, telemetry variables, and documented harness-specific environment; never forward or log unrelated environment values.
Run one local adapter host per Fabric runtime for orderedstart→invoke*→stop. Emit one JSON lifecycle response per request on stdout and diagnostics on stderr; treat early exit as a host crash.
Return harness-level invoke failures in a successful lifecycle response withresponse: null,failed: true, and structurederrorcontainingcode,message,retryable, and optionalmetadata.
Do not emit NeMo Relay stream records on adapter stdout. Relay records are sent out of band through the SDK-owned NDJSON ATOF endpoint; the adapter must return exactly one terminal lifecycle response.
Scope workspace, generated configuration, state, sessions, and artifacts to the resolved runtime context. Write artifacts only within the resolved r...
Files:
adapters/hermes/src/nemo_fabric_adapters/hermes/adapter.pyadapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex/src/nemo_fabric_adapters/codex/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/hermes/src/nemo_fabric_adapters/hermes/adapter.pyadapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex/src/nemo_fabric_adapters/codex/adapter.py
🪛 ast-grep (0.45.0)
tests/e2e/test_claude.py
[warning] 181-190: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.post(
f"{api_server}/_scenario",
json={
"tool_call": {
"name": tool_name,
"arguments": {"timezone": "America/Los_Angeles"},
}
},
timeout=5,
)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
tests/e2e/test_codex.py
[warning] 28-38: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.post(
f"{api_server}/_scenario",
json={
"tool_call": {
"name": tool_name,
"namespace": "mcp__mcp_server_time",
"arguments": {"timezone": "America/Los_Angeles"},
}
},
timeout=5,
)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
tests/_utils/mock_api_server.py
[info] 356-356: use jsonify instead of json.dumps for JSON output
Context: json.dumps(tool_call["arguments"])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 499-499: use jsonify instead of json.dumps for JSON output
Context: json.dumps(event)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 505-505: use jsonify instead of json.dumps for JSON output
Context: json.dumps(tool_call["arguments"])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 576-576: use jsonify instead of json.dumps for JSON output
Context: json.dumps(event)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
tests/e2e/test_deepagents.py
[warning] 115-124: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.post(
f"{api_server}/_scenario",
json={
"tool_call": {
"name": tool_name,
"arguments": {"timezone": "America/Los_Angeles"},
}
},
timeout=5,
)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
🪛 Ruff (0.16.1)
adapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.py
[warning] 284-286: Avoid specifying long messages outside the exception class
(TRY003)
tests/e2e/test_claude.py
[warning] 182-182: Async functions should not call blocking HTTP methods
(ASYNC210)
tests/e2e/test_codex.py
[warning] 29-29: Async functions should not call blocking HTTP methods
(ASYNC210)
tests/_utils/mock_api_server.py
[warning] 138-138: Missing return type annotation for private function responses
(ANN202)
[warning] 165-165: Missing return type annotation for private function messages
(ANN202)
tests/e2e/test_deepagents.py
[warning] 116-116: Async functions should not call blocking HTTP methods
(ASYNC210)
🔇 Additional comments (11)
adapters/codex/src/nemo_fabric_adapters/codex/adapter.py (1)
187-190: 🗄️ Data Integrity & IntegrationConfirm
common_utils.normalize_listsemantics forargs.All three adapters now depend on
normalize_listto coerceargs. Confirm the helper exists innemo-fabric-adapters-common0.2.0, and confirm how it treats a bare string and non-string elements. If it splits or stringifies a bare string, a config that setsargs: "--root ."produces one argument instead of two, and the previousshlexbehavior silently changes shape.#!/bin/bash # Locate and print normalize_list plus its tests. fd -t f 'utils.py' adapters/common | xargs -r rg -n -C 12 'def normalize_list' rg -n -C 6 'normalize_list' --glob '*.py' -g '!**/node_modules/**'tests/adapters/test_claude_adapter.py (1)
197-199: LGTM!Also applies to: 242-247
tests/adapters/test_codex_adapter.py (1)
287-296: LGTM!Also applies to: 320-328
tests/adapters/test_deepagents.py (1)
607-628: LGTM!tests/_utils/mock_api_server.py (2)
366-367: 🗄️ Data Integrity & IntegrationConfirm the Codex SDK accepts a
namespacekey on afunction_callitem.
namespaceis not a field in the OpenAI Responsesfunction_callschema. The Codex e2e test sets it and then assertstool_events[0]["server"] == "mcp_server_time", so the server attribution must come from somewhere. Confirm that the Codex SDK readsnamespace, and not amcp__<server>__<tool>prefix onname. If the SDK validates output items strictly, an unknown key can make it reject the item and the test will fail for an unrelated reason.#!/bin/bash # Find how Codex attributes an MCP tool call to a server. rg -n -C 5 'mcpToolCall' --glob '*.py' rg -n -C 4 '\bnamespace\b' adapters/codex tests/e2e/test_codex.py tests/_utils/mock_api_server.py
503-577: LGTM!tests/e2e/test_claude.py (1)
218-242: LGTM!tests/adapters/test_adapter_package_metadata.py (1)
54-54: LGTM!tests/adapters/test_hermes_adapter.py (1)
513-513: LGTM!adapters/hermes/src/nemo_fabric_adapters/hermes/adapter.py (2)
306-309: 🩺 Stability & AvailabilityConfirm that MCP discovery does not run while
redirect_stdoutis active.If
startwraps this await in the redirection scope, move the await outside it or use local output capture. Add a regression test for concurrent stdout output.
306-309: 🩺 Stability & AvailabilityConfirm the cancellation path before cleanup. If
startcancellation leavesdiscover_mcp_tools()running inasyncio.to_thread, coordinate its completion beforeshutdown_mcp_servers()and add a regression test.
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/hermes/src/nemo_fabric_adapters/hermes/adapter.py (1)
306-309: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftSerialize MCP discovery and shutdown across cancellation.
When
start()is canceled duringdiscover_mcp_tools, its cleanup callsstop()while the executor thread still runs discovery. Track and drain discovery beforeshutdown_mcp_servers()runs.🤖 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/hermes/src/nemo_fabric_adapters/hermes/adapter.py` around lines 306 - 309, Serialize MCP discovery and shutdown in adapter.py: track the awaitable or task created around discover_mcp_tools in start() so cancellation can await its completion before cleanup proceeds. Update stop() at lines 507-511 to drain any in-progress discovery before calling shutdown_mcp_servers(), preserving safe ordering across cancellation.
🤖 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/hermes/src/nemo_fabric_adapters/hermes/adapter.py`:
- Around line 306-309: Serialize MCP discovery and shutdown in adapter.py: track
the awaitable or task created around discover_mcp_tools in start() so
cancellation can await its completion before cleanup proceeds. Update stop() at
lines 507-511 to drain any in-progress discovery before calling
shutdown_mcp_servers(), preserving safe ordering across cancellation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: be64f25e-acb2-421a-b9da-d73f0c4e42b6
📒 Files selected for processing (1)
adapters/hermes/src/nemo_fabric_adapters/hermes/adapter.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (12)
**/*.{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/hermes/src/nemo_fabric_adapters/hermes/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/hermes/src/nemo_fabric_adapters/hermes/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/hermes/src/nemo_fabric_adapters/hermes/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/hermes/src/nemo_fabric_adapters/hermes/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/hermes/src/nemo_fabric_adapters/hermes/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/hermes/src/nemo_fabric_adapters/hermes/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/hermes/src/nemo_fabric_adapters/hermes/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/hermes/src/nemo_fabric_adapters/hermes/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/hermes/src/nemo_fabric_adapters/hermes/adapter.py
adapters/**/*
📄 CodeRabbit inference engine (.agents/skills/contribute-adapter/SKILL.md)
Place repository adapters under
adapters/<name>, define their install extra and packaged descriptor, select the matchingharness.resolutionstrategy, and document repository orbase_dirdescriptor discovery.
Files:
adapters/hermes/src/nemo_fabric_adapters/hermes/adapter.py
adapters/**/*.{py,js,ts,java,go,rs}
📄 CodeRabbit inference engine (.agents/skills/contribute-adapter/SKILL.md)
adapters/**/*.{py,js,ts,java,go,rs}: Use the existing Fabricpythonorprocessrunner and normalized request/result contracts. Reuseadapters/common/only when its contract fits; do not add a runner or abstraction for one adapter.
For adapters consuming normalized configuration or runtime context, use the complete Fabric invocation and treatconfig,capability_plan,telemetry_plan, andruntime_contextas authoritative. Reserveharness.settingsfor harness-specific behavior.
Apply configuration precedence in this order: normalized config; Fabric-resolved plans and runtime context; harness-specific settings; descriptor and adapter defaults. Reject conflicting duplicate declarations or unsupported behavior with actionable field-specific errors; never silently drop configuration.
Run dependency and authentication preflight before invoking the harness. Declare fixed dependencies in descriptor requirements, validate versions, hooks, and credentials, and never expose credential values in output, errors, events, logs, or fixtures.
Forward only required system variables, selected credential variables, telemetry variables, and documented harness-specific environment; never forward or log unrelated environment values.
Run one local adapter host per Fabric runtime for orderedstart→invoke*→stop. Emit one JSON lifecycle response per request on stdout and diagnostics on stderr; treat early exit as a host crash.
Return harness-level invoke failures in a successful lifecycle response withresponse: null,failed: true, and structurederrorcontainingcode,message,retryable, and optionalmetadata.
Do not emit NeMo Relay stream records on adapter stdout. Relay records are sent out of band through the SDK-owned NDJSON ATOF endpoint; the adapter must return exactly one terminal lifecycle response.
Scope workspace, generated configuration, state, sessions, and artifacts to the resolved runtime context. Write artifacts only within the resolved r...
Files:
adapters/hermes/src/nemo_fabric_adapters/hermes/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/hermes/src/nemo_fabric_adapters/hermes/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/hermes/src/nemo_fabric_adapters/hermes/adapter.py
…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>
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 (2)
external/nat/src/nemo_fabric_adapters/nat/adapter.py (2)
5-10: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDefine
NATon first use.Write
NeMo Agent Toolkit (NAT)in the module docstring. The following text usesNATwithout defining the abbreviation.As per coding guidelines, “Spell out terms on first use and put the acronym in parentheses.”
🤖 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 `@external/nat/src/nemo_fabric_adapters/nat/adapter.py` around lines 5 - 10, Update the module docstring for the NeMo Agent Toolkit adapter to spell out “NeMo Agent Toolkit” on its first occurrence and introduce the abbreviation as “NeMo Agent Toolkit (NAT)” before using “NAT” later in the text.Source: Coding guidelines
323-333: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUpdate all stdio adapter tests to use structured
urlandargs.
tests/adapters/test_external_nat_adapter.py:408-418: Assert thatcalculator.urlissys.executableand an argument containscalculator_mcp.py.tests/adapters/test_external_nat_adapter.py:998-1025: Seturlto$NAT_TEST_MCP_COMMANDand move the command arguments intoargs. Update the expected mapping.tests/adapters/test_external_nat_adapter.py:1028-1037: Remove or replace the unbalanced-quote case because structuredargsno longer parses shell quoting.Run the focused test through
python-tests, then runjust test-python.🤖 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 `@external/nat/src/nemo_fabric_adapters/nat/adapter.py` around lines 323 - 333, Update the stdio adapter tests in tests/adapters/test_external_nat_adapter.py:408-418 to assert structured url and args values, use sys.executable and verify an argument contains calculator_mcp.py; in tests/adapters/test_external_nat_adapter.py:998-1025 set url to $NAT_TEST_MCP_COMMAND, move command arguments into args, and update the expected mapping; remove or replace the unbalanced-quote case at tests/adapters/test_external_nat_adapter.py:1028-1037. The stdio handling in external/nat/src/nemo_fabric_adapters/nat/adapter.py:323-333 and the calculator example in external/nat/examples/calculator.py:61-69 require no direct changes. Run the focused test through python-tests, then run just test-python.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 `@external/nat/src/nemo_fabric_adapters/nat/adapter.py`:
- Around line 5-10: Update the module docstring for the NeMo Agent Toolkit
adapter to spell out “NeMo Agent Toolkit” on its first occurrence and introduce
the abbreviation as “NeMo Agent Toolkit (NAT)” before using “NAT” later in the
text.
- Around line 323-333: Update the stdio adapter tests in
tests/adapters/test_external_nat_adapter.py:408-418 to assert structured url and
args values, use sys.executable and verify an argument contains
calculator_mcp.py; in tests/adapters/test_external_nat_adapter.py:998-1025 set
url to $NAT_TEST_MCP_COMMAND, move command arguments into args, and update the
expected mapping; remove or replace the unbalanced-quote case at
tests/adapters/test_external_nat_adapter.py:1028-1037. The stdio handling in
external/nat/src/nemo_fabric_adapters/nat/adapter.py:323-333 and the calculator
example in external/nat/examples/calculator.py:61-69 require no direct changes.
Run the focused test through python-tests, then run just test-python.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 9455d50a-069f-4867-a68f-5088c8ca9f8a
📒 Files selected for processing (2)
external/nat/examples/calculator.pyexternal/nat/src/nemo_fabric_adapters/nat/adapter.py
📜 Review details
⏰ Context from checks skipped due to timeout. (19)
- GitHub Check: Preview docs
- GitHub Check: Test (Python 3.11, windows-amd64)
- GitHub Check: Test (Python 3.12, windows-amd64)
- GitHub Check: Test (Python 3.12, linux-arm64)
- GitHub Check: Test (Python 3.14, windows-amd64)
- GitHub Check: Test (Python 3.14, macos-arm64)
- GitHub Check: Test (Python 3.12, macos-arm64)
- GitHub Check: Test (Python 3.13, macos-arm64)
- GitHub Check: Test (Python 3.11, macos-arm64)
- GitHub Check: Test (Python 3.13, windows-amd64)
- GitHub Check: Test (Python 3.12, linux-amd64)
- GitHub Check: Test (Python 3.11, linux-amd64)
- GitHub Check: Test (Python 3.14, linux-arm64)
- GitHub Check: Test (Python 3.13, linux-arm64)
- GitHub Check: Test (Python 3.13, linux-amd64)
- GitHub Check: Test (Python 3.14, linux-amd64)
- GitHub Check: Test (Python 3.11, linux-arm64)
- GitHub Check: Test (x86_64)
- GitHub Check: Pre-commit
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{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:
external/nat/examples/calculator.pyexternal/nat/src/nemo_fabric_adapters/nat/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:
external/nat/examples/calculator.pyexternal/nat/src/nemo_fabric_adapters/nat/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:
external/nat/examples/calculator.pyexternal/nat/src/nemo_fabric_adapters/nat/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:
external/nat/examples/calculator.pyexternal/nat/src/nemo_fabric_adapters/nat/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:
external/nat/examples/calculator.pyexternal/nat/src/nemo_fabric_adapters/nat/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:
external/nat/examples/calculator.pyexternal/nat/src/nemo_fabric_adapters/nat/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:
external/nat/examples/calculator.pyexternal/nat/src/nemo_fabric_adapters/nat/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:
external/nat/examples/calculator.pyexternal/nat/src/nemo_fabric_adapters/nat/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:
external/nat/examples/calculator.pyexternal/nat/src/nemo_fabric_adapters/nat/adapter.py
🪛 ast-grep (0.45.0)
external/nat/examples/calculator.py
[info] 86-86: use jsonify instead of json.dumps for JSON output
Context: json.dumps(output.to_mapping(), indent=2)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 Ruff (0.16.1)
external/nat/src/nemo_fabric_adapters/nat/adapter.py
[warning] 58-58: Dynamically typed expressions (typing.Any) are disallowed in **metadata
(ANN401)
[warning] 72-72: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 175-175: Too many branches (13 > 12)
(PLR0912)
[warning] 287-287: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 304-304: Dynamically typed expressions (typing.Any) are disallowed in server
(ANN401)
[warning] 536-536: Too many branches (14 > 12)
(PLR0912)
[warning] 690-690: Dynamically typed expressions (typing.Any) are disallowed in build_nat_config
(ANN401)
[warning] 718-718: Prefer TypeError exception for invalid type
(TRY004)
[warning] 718-718: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 730-732: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 737-737: Dynamically typed expressions (typing.Any) are disallowed in response
(ANN401)
[warning] 770-770: Do not catch blind exception: Exception
(BLE001)
[warning] 771-774: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 850-854: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
[warning] 858-858: Do not catch blind exception: Exception
(BLE001)
[warning] 859-862: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 873-876: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
Signed-off-by: David Gardner <dagardner@nvidia.com>
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_external_nat_adapter.py`:
- Around line 1010-1018: Extend the test configuration around
nat_mcp_server_config to include the literal $NAT_TEST_MCP_COMMAND in one args
entry and one env value, then assert both values remain unexpanded while the url
continues expanding. Preserve the existing command, port, trace, and timeout
coverage.
🪄 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: 697ea2a7-2f18-4287-8c47-28c6eb982614
📒 Files selected for processing (2)
external/nat/README.mdtests/adapters/test_external_nat_adapter.py
📜 Review details
⏰ Context from checks skipped due to timeout. (18)
- GitHub Check: Preview docs
- GitHub Check: Test (Python 3.12, macos-arm64)
- GitHub Check: Test (Python 3.13, linux-amd64)
- GitHub Check: Test (Python 3.11, macos-arm64)
- GitHub Check: Test (Python 3.13, windows-amd64)
- GitHub Check: Test (Python 3.12, windows-amd64)
- GitHub Check: Test (Python 3.12, linux-amd64)
- GitHub Check: Test (Python 3.11, linux-amd64)
- GitHub Check: Test (Python 3.14, linux-arm64)
- GitHub Check: Test (Python 3.11, windows-amd64)
- GitHub Check: Test (Python 3.14, macos-arm64)
- GitHub Check: Test (Python 3.11, linux-arm64)
- GitHub Check: Test (Python 3.13, linux-arm64)
- GitHub Check: Test (Python 3.12, linux-arm64)
- GitHub Check: Test (Python 3.14, linux-amd64)
- GitHub Check: Test (Python 3.14, windows-amd64)
- GitHub Check: Test (Python 3.13, macos-arm64)
- GitHub Check: Pre-commit
🧰 Additional context used
📓 Path-based instructions (20)
**/*.{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:
external/nat/README.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:
external/nat/README.mdtests/adapters/test_external_nat_adapter.py
**/*.{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:
external/nat/README.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:
external/nat/README.md
**/*.{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:
external/nat/README.mdtests/adapters/test_external_nat_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:
external/nat/README.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:
external/nat/README.md
**/*.{html,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
HTML and Markdown files must use the specified SPDX HTML-comment header.
Files:
external/nat/README.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:
external/nat/README.md
**/*.{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_external_nat_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_external_nat_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_external_nat_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_external_nat_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_external_nat_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_external_nat_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_external_nat_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_external_nat_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_external_nat_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_external_nat_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_external_nat_adapter.py
🔇 Additional comments (3)
external/nat/README.md (1)
53-56: LGTM!tests/adapters/test_external_nat_adapter.py (2)
417-420: LGTM!
1038-1048: LGTM!
Salonijain27
left a comment
Salonijain27
left a comment
There was a problem hiding this comment.
Approved from a dependency point of view
…mprovements 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 (2)
adapters/codex/src/nemo_fabric_adapters/codex/adapter.py (1)
34-34: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse Ruff-compatible imports for all common modules.
Replace the aliased imports on lines 32–34 with
from nemo_fabric_adapters.common import ...imports.🤖 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/codex/src/nemo_fabric_adapters/codex/adapter.py` at line 34, Update the common-module imports in adapter.py to use Ruff-compatible from-imports from nemo_fabric_adapters.common, replacing the aliased import statements while preserving each imported module’s existing usage.Source: Linters/SAST tools
tests/adapters/test_codex_adapter.py (1)
653-653: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRename the unused fixture parameter. Rename
mock_codexto_mock_codexand keep the parameter so the fixture setup still runs. This clears RuffARG001.🤖 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` at line 653, In the test using the fixture parameters codex_payload, mock_codex, monkeypatch, and tmp_path, rename the unused mock_codex parameter to _mock_codex while preserving its position and fixture injection so setup still executes and Ruff ARG001 is cleared.Source: Linters/SAST tools
🤖 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/codex/src/nemo_fabric_adapters/codex/adapter.py`:
- Line 34: Update the common-module imports in adapter.py to use Ruff-compatible
from-imports from nemo_fabric_adapters.common, replacing the aliased import
statements while preserving each imported module’s existing usage.
In `@tests/adapters/test_codex_adapter.py`:
- Line 653: In the test using the fixture parameters codex_payload, mock_codex,
monkeypatch, and tmp_path, rename the unused mock_codex parameter to _mock_codex
while preserving its position and fixture injection so setup still executes and
Ruff ARG001 is cleared.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 2f98f48b-5dc5-4c97-9d81-57474ad45431
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
adapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex/src/nemo_fabric_adapters/codex/adapter.pyexternal/nat/README.mdexternal/nat/examples/calculator.pyexternal/nat/src/nemo_fabric_adapters/nat/adapter.pytests/adapters/test_adapter_package_metadata.pytests/adapters/test_claude_adapter.pytests/adapters/test_codex_adapter.pytests/adapters/test_external_nat_adapter.pytests/e2e/test_claude.py
📜 Review details
⏰ Context from checks skipped due to timeout. (18)
- GitHub Check: Preview docs
- GitHub Check: Test (Python 3.13, windows-amd64)
- GitHub Check: Test (Python 3.14, linux-amd64)
- GitHub Check: Test (Python 3.13, linux-arm64)
- GitHub Check: Test (Python 3.14, linux-arm64)
- GitHub Check: Test (Python 3.11, linux-amd64)
- GitHub Check: Test (Python 3.14, windows-amd64)
- GitHub Check: Pre-commit
- GitHub Check: Test (Python 3.12, linux-arm64)
- GitHub Check: Test (Python 3.11, windows-amd64)
- GitHub Check: Test (Python 3.12, macos-arm64)
- GitHub Check: Test (Python 3.14, macos-arm64)
- GitHub Check: Test (Python 3.12, windows-amd64)
- GitHub Check: Test (Python 3.11, macos-arm64)
- GitHub Check: Test (Python 3.12, linux-amd64)
- GitHub Check: Test (Python 3.13, macos-arm64)
- GitHub Check: Test (Python 3.13, linux-amd64)
- GitHub Check: Test (Python 3.11, linux-arm64)
🧰 Additional context used
📓 Path-based instructions (22)
**/*.{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/codex/src/nemo_fabric_adapters/codex/adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pytests/adapters/test_codex_adapter.pytests/adapters/test_adapter_package_metadata.pyexternal/nat/src/nemo_fabric_adapters/nat/adapter.pytests/adapters/test_claude_adapter.pytests/adapters/test_external_nat_adapter.pyexternal/nat/examples/calculator.pytests/e2e/test_claude.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/codex/src/nemo_fabric_adapters/codex/adapter.pyexternal/nat/README.mdadapters/claude/src/nemo_fabric_adapters/claude/adapter.pytests/adapters/test_codex_adapter.pytests/adapters/test_adapter_package_metadata.pyexternal/nat/src/nemo_fabric_adapters/nat/adapter.pytests/adapters/test_claude_adapter.pytests/adapters/test_external_nat_adapter.pyexternal/nat/examples/calculator.pytests/e2e/test_claude.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/codex/src/nemo_fabric_adapters/codex/adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pytests/adapters/test_codex_adapter.pytests/adapters/test_adapter_package_metadata.pyexternal/nat/src/nemo_fabric_adapters/nat/adapter.pytests/adapters/test_claude_adapter.pytests/adapters/test_external_nat_adapter.pyexternal/nat/examples/calculator.pytests/e2e/test_claude.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/codex/src/nemo_fabric_adapters/codex/adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pytests/adapters/test_codex_adapter.pytests/adapters/test_adapter_package_metadata.pyexternal/nat/src/nemo_fabric_adapters/nat/adapter.pytests/adapters/test_claude_adapter.pytests/adapters/test_external_nat_adapter.pyexternal/nat/examples/calculator.pytests/e2e/test_claude.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/codex/src/nemo_fabric_adapters/codex/adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pytests/adapters/test_codex_adapter.pytests/adapters/test_adapter_package_metadata.pyexternal/nat/src/nemo_fabric_adapters/nat/adapter.pytests/adapters/test_claude_adapter.pytests/adapters/test_external_nat_adapter.pyexternal/nat/examples/calculator.pytests/e2e/test_claude.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/codex/src/nemo_fabric_adapters/codex/adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pytests/adapters/test_codex_adapter.pytests/adapters/test_adapter_package_metadata.pyexternal/nat/src/nemo_fabric_adapters/nat/adapter.pytests/adapters/test_claude_adapter.pytests/adapters/test_external_nat_adapter.pyexternal/nat/examples/calculator.pytests/e2e/test_claude.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/codex/src/nemo_fabric_adapters/codex/adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pytests/adapters/test_codex_adapter.pytests/adapters/test_adapter_package_metadata.pyexternal/nat/src/nemo_fabric_adapters/nat/adapter.pytests/adapters/test_claude_adapter.pytests/adapters/test_external_nat_adapter.pyexternal/nat/examples/calculator.pytests/e2e/test_claude.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/codex/src/nemo_fabric_adapters/codex/adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pytests/adapters/test_codex_adapter.pytests/adapters/test_adapter_package_metadata.pyexternal/nat/src/nemo_fabric_adapters/nat/adapter.pytests/adapters/test_claude_adapter.pytests/adapters/test_external_nat_adapter.pyexternal/nat/examples/calculator.pytests/e2e/test_claude.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/codex/src/nemo_fabric_adapters/codex/adapter.pyexternal/nat/README.mdadapters/claude/src/nemo_fabric_adapters/claude/adapter.pytests/adapters/test_codex_adapter.pytests/adapters/test_adapter_package_metadata.pyexternal/nat/src/nemo_fabric_adapters/nat/adapter.pytests/adapters/test_claude_adapter.pytests/adapters/test_external_nat_adapter.pyexternal/nat/examples/calculator.pytests/e2e/test_claude.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/codex/src/nemo_fabric_adapters/codex/adapter.pyadapters/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/codex/src/nemo_fabric_adapters/codex/adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{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:
external/nat/README.md
**/*.{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:
external/nat/README.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:
external/nat/README.md
**/*.{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:
external/nat/README.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:
external/nat/README.md
**/*.{html,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
HTML and Markdown files must use the specified SPDX HTML-comment header.
Files:
external/nat/README.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:
external/nat/README.md
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_codex_adapter.pytests/adapters/test_adapter_package_metadata.pytests/adapters/test_claude_adapter.pytests/adapters/test_external_nat_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_codex_adapter.pytests/adapters/test_adapter_package_metadata.pytests/adapters/test_claude_adapter.pytests/adapters/test_external_nat_adapter.pytests/e2e/test_claude.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_codex_adapter.pytests/adapters/test_adapter_package_metadata.pytests/adapters/test_claude_adapter.pytests/adapters/test_external_nat_adapter.pytests/e2e/test_claude.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_codex_adapter.pytests/adapters/test_adapter_package_metadata.pytests/adapters/test_claude_adapter.pytests/adapters/test_external_nat_adapter.pytests/e2e/test_claude.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/codex/src/nemo_fabric_adapters/codex/adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
🪛 ast-grep (0.45.0)
tests/adapters/test_codex_adapter.py
[info] 628-628: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"schema_version": "ATIF-v1.7", "steps": []})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
tests/adapters/test_claude_adapter.py
[info] 832-832: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"schema_version": "ATIF-v1.7", "steps": []})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 Ruff (0.16.1)
adapters/codex/src/nemo_fabric_adapters/codex/adapter.py
[warning] 34-34: Use from nemo_fabric_adapters.common import relay_artifacts in lieu of alias
Replace with from nemo_fabric_adapters.common import relay_artifacts
(PLR0402)
tests/adapters/test_codex_adapter.py
[warning] 623-623: Missing return type annotation for private function finish_turn
(ANN202)
[warning] 626-626: Missing return type annotation for private function write_atif
Add return type annotation: None
(ANN202)
[warning] 653-653: Unused function argument: mock_codex
(ARG001)
external/nat/src/nemo_fabric_adapters/nat/adapter.py
[warning] 616-616: Dynamically typed expressions (typing.Any) are disallowed in build_nat_config
(ANN401)
tests/adapters/test_claude_adapter.py
[warning] 830-830: Missing return type annotation for private function write_atif
Add return type annotation: None
(ANN202)
tests/adapters/test_external_nat_adapter.py
[warning] 43-43: Dynamically typed expressions (typing.Any) are disallowed in **values
(ANN401)
[warning] 63-63: for loop variable component overwritten by assignment target
(PLW2901)
[warning] 70-70: for loop variable component overwritten by assignment target
(PLW2901)
🔇 Additional comments (13)
tests/adapters/test_adapter_package_metadata.py (1)
64-64: LGTM!tests/adapters/test_external_nat_adapter.py (2)
1049-1061: Cover literalargsandenvvalues.Include
$NAT_TEST_MCP_COMMANDin one argument and one environment value. Assert that both values remain unexpanded.
21-100: LGTM!Also applies to: 217-274, 312-438, 457-516, 547-749, 752-840, 938-953, 1030-1033, 1079-1119
tests/e2e/test_claude.py (2)
183-193: Use a non-blocking scenario request.
requests.postblocks the event loop inside this async test.
73-141: LGTM!Also applies to: 250-294
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py (1)
34-34: LGTM!Also applies to: 303-309, 727-745, 840-1054
adapters/codex/src/nemo_fabric_adapters/codex/adapter.py (1)
188-193: LGTM!Also applies to: 918-1118
external/nat/src/nemo_fabric_adapters/nat/adapter.py (1)
23-29: LGTM!Also applies to: 55-58, 75-244, 264-285, 314-613, 703-826
external/nat/examples/calculator.py (1)
40-41: LGTM!Also applies to: 61-69
external/nat/README.md (1)
12-124: LGTM!tests/adapters/test_claude_adapter.py (1)
77-146: LGTM!Also applies to: 197-199, 242-247, 762-812, 880-933
tests/adapters/test_codex_adapter.py (2)
139-179: LGTM!Also applies to: 328-337, 361-369, 608-622, 633-649, 652-652, 654-696
623-626: 📐 Maintainability & Code QualityLikely an incorrect or invalid review comment.
| "args": common_utils.normalize_list(server.get("args")), | ||
| } | ||
| if env := server.get("env"): | ||
| result[name]["env"] = env |
There was a problem hiding this comment.
MCP credentials are exposed in the Claude process argv.
This env mapping becomes part of the dictionary passed to ClaudeAgentOptions.mcp_servers. With the pinned claude-agent-sdk==0.2.120, dictionary MCP configs are JSON-serialized directly into the --mcp-config argument. codex reproduced this through Fabric().run(): a synthetic MCP token appeared verbatim in the spawned Claude child argv while the run succeeded.
can we avoid serializing credential-bearing MCP config inline in argv (for example, stage it in an owner-only runtime-scoped file and pass the file path), remove it during runtime cleanup.
There was a problem hiding this comment.
good point. how did we handle this in NAT (SecretStr?)
There was a problem hiding this comment.
In NAT we just did something like
subprocess.Popen(..., env=env)
We aren't passing env to Claude in an insecure way, but something Claude does.
| [project.optional-dependencies] | ||
| harness = [ | ||
| "hermes-agent>=0.17.0; python_version < '3.14'", | ||
| "hermes-agent>=0.19.0; python_version < '3.14'", |
There was a problem hiding this comment.
the supported Hermes installs do not include its MCP runtime.
Both [harness] here and [full] below install bare hermes-agent>=0.19.0, but Hermes publishes the MCP SDK only under its mcp extra. In clean isolated installs of both Fabric extras at the current PR head, mcp was not importable, tools.mcp_tool._MCP_AVAILABLE was False, and MCP discovery returned no tools even though this adapter advertises MCP support. A control install with hermes-agent[mcp] enabled MCP successfully.
i think we need hermes-agent[mcp] in both extras, update the relevant lockfiles, and add clean-install metadata coverage.
|
|
||
| discover_mcp_tools() | ||
| _clear_tool_defs_cache() | ||
| await asyncio.to_thread(discover_mcp_tools) |
There was a problem hiding this comment.
Nitty thing — await asyncio.to_thread(discover_mcp_tools) offloads the work to a worker thread, which is not a loop (like comment suggested). Should we reword the comment?
| if transport == "stdio": | ||
| command = shlex.split(url) | ||
| if not command: | ||
| raise AdapterConfigError( | ||
| "claude_invalid_configuration", "MCP command is required" | ||
| ) | ||
| result[name] = {"type": "stdio", "command": command[0], "args": command[1:]} | ||
| result[name] = { | ||
| "type": "stdio", | ||
| "command": url, | ||
| "args": common_utils.normalize_list(server.get("args")), | ||
| } | ||
| if env := server.get("env"): | ||
| result[name]["env"] = env |
There was a problem hiding this comment.
A nitty thing too, dropping shlex.split removed an implicit guard: shlex.split(" ") == [] used to trip the if not command check and raise a field-named AdapterConfigError :)
There was a problem hiding this comment.
Also so much cleaner
| "claude_invalid_configuration", "MCP server URL is required" | ||
| ) | ||
| if transport == "stdio": | ||
| command = shlex.split(url) |
There was a problem hiding this comment.
readme still has url="repo-mcp --root .",, do we need to update that?
| @@ -186,19 +185,12 @@ def _native_mcp_servers(payload: dict[str, Any]) -> dict[str, dict[str, Any]]: | |||
| ) | |||
| normalized_transport = transport.strip().lower().replace("_", "-") | |||
| if normalized_transport == "stdio": | |||
There was a problem hiding this comment.
readme still has url="repo-mcp --root .",, do we need to update that?
AjayThorve
left a comment
AjayThorve
left a comment
There was a problem hiding this comment.
just a few comments, should be good to merge post that
| "args": common_utils.normalize_list(server.get("args")), | ||
| } | ||
| if env := server.get("env"): | ||
| result[name]["env"] = env |
There was a problem hiding this comment.
good point. how did we handle this in NAT (SecretStr?)
| if transport == "stdio": | ||
| command = shlex.split(url) | ||
| if not command: | ||
| raise AdapterConfigError( | ||
| "claude_invalid_configuration", "MCP command is required" | ||
| ) | ||
| result[name] = {"type": "stdio", "command": command[0], "args": command[1:]} | ||
| result[name] = { | ||
| "type": "stdio", | ||
| "command": url, | ||
| "args": common_utils.normalize_list(server.get("args")), | ||
| } | ||
| if env := server.get("env"): | ||
| result[name]["env"] = env |
There was a problem hiding this comment.
Also so much cleaner
…pping such that the values in the file redirect to other environment variables, this prevents the issue where if the process was terminated credentials wouldn't remain on disk Signed-off-by: David Gardner <dagardner@nvidia.com>
5 participants
Overview
argsandenvfor MCP arguments to Deepagents, Claude and Codex.shlex.split(url)to split the command from the argument.Hermes Agent specific changes:
_clear_tool_defs_cachedoesn't appear to be neededdiscover_mcp_toolsandshutdown_mcp_serverswithasyncio.to_threadWhere should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Closes FABRIC-172
Closes FABRIC-168
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
Summary by CodeRabbit
New Features
Bug Fixes
Chores