feat: move Claude adapter to v1alpha2 - #197
Conversation
Signed-off-by: Zhongxuan Wang <daniewang@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 Claude adapter now consumes validated ChangesClaude typed configuration migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Lifecycle serving
participant ClaudeRuntime
participant RuntimeContext
participant Relay gateway
participant Claude SDK
Lifecycle serving->>ClaudeRuntime: Load AgentConfig.from_mapping
ClaudeRuntime->>RuntimeContext: Validate runtime context
ClaudeRuntime->>Relay gateway: Start Relay with typed configuration
ClaudeRuntime->>Claude SDK: Build options and invoke query
Claude SDK-->>ClaudeRuntime: Return result messages
ClaudeRuntime-->>Lifecycle serving: Return normalized result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/adapters/test_claude_adapter.py`:
- Line 543: Update the pytest.raises assertion using the match argument in the
relevant test to escape the period in “SKILL.md”, so the regex requires a
literal dot while preserving the existing expected error.
🪄 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: 6f913ae5-9e45-47fe-aab4-9056d2166098
⛔ Files ignored due to path filters (2)
adapters/claude/uv.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
adapters/claude/fabric-adapter.jsonadapters/claude/pyproject.tomladapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/common/src/nemo_fabric_adapters/common/utils.pycrates/fabric-cli/assets/adapters/claude/fabric-adapter.jsontests/adapters/test_adapter_package_metadata.pytests/adapters/test_claude_adapter.py
📜 Review details
⏰ Context from checks skipped due to timeout. (15)
- GitHub Check: Preview docs
- GitHub Check: Test (Python 3.14, macos-arm64)
- GitHub Check: Test (Python 3.12, windows-amd64)
- GitHub Check: Test (Python 3.13, linux-arm64)
- GitHub Check: Test (Python 3.14, windows-amd64)
- GitHub Check: Test (Python 3.11, windows-amd64)
- GitHub Check: Test (Python 3.11, linux-arm64)
- GitHub Check: Test (Python 3.11, linux-amd64)
- GitHub Check: Test (Python 3.11, macos-arm64)
- GitHub Check: Test (Python 3.12, macos-arm64)
- GitHub Check: Test (Python 3.14, linux-amd64)
- GitHub Check: Test (Python 3.14, linux-arm64)
- GitHub Check: Test (Python 3.12, linux-arm64)
- GitHub Check: Test (Python 3.12, linux-amd64)
- GitHub Check: Pre-commit
🧰 Additional context used
📓 Path-based instructions (28)
**/*.{rs,py,pyi,json,yaml,yml}
📄 CodeRabbit inference engine (.agents/skills/contribute-api/SKILL.md)
Determine and update every affected public surface, including the CLI, PyO3 bindings, Python SDK, type stubs, schemas, and adapter contract, so they remain in parity.
Files:
adapters/claude/fabric-adapter.jsoncrates/fabric-cli/assets/adapters/claude/fabric-adapter.jsontests/adapters/test_adapter_package_metadata.pyadapters/common/src/nemo_fabric_adapters/common/utils.pytests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*
📄 CodeRabbit inference engine (.agents/skills/karpathy-guidelines/SKILL.md)
**/*: Before implementing, explicitly state assumptions, surface ambiguity and tradeoffs, present multiple interpretations when relevant, and ask for clarification rather than silently deciding or proceeding when requirements are unclear.
Prefer the minimum code needed to solve the requested problem: avoid speculative features, unnecessary abstractions, unrequested flexibility, and handling of impossible scenarios; simplify overcomplicated solutions.
When editing existing code, make surgical changes only: do not modify unrelated code, comments, formatting, or pre-existing dead code; match the existing style, and remove only unused imports, variables, or functions introduced by your changes.
Define verifiable success criteria for each task, such as writing regression tests for bugs and invalid-input tests for validation, then verify the implementation against those criteria. For multi-step work, state a brief plan with a verification check for each step.
**/*: Always spellNVIDIAin all caps; do not useNvidia,nvidia,nVidia,nVIDIA, orNV.
Usean NVIDIAbefore a noun, because the name begins with an “en” sound.
Do not add a registered trademark symbol afterNVIDIAwhen referring to the company; use trademark symbols with product names only when required by the document type or legal guidance.
Verify official capitalization, spacing, hyphenation, and spelling for NVIDIA and third-party product names; do not rewrite official product names for grammar or title-case rules.
Precede NVIDIA product names withNVIDIAon first mention when natural and accurate, and link the first mention when the destination helps the reader.
On first use, include the company name and full model qualifier when it helps identify the model; preserve official capitalization and punctuation, and use shorter family names only after establishing the full name.
For learning-oriented and developer content, do not force trademark symbols unless explicitly required; for press, ...
Files:
adapters/claude/fabric-adapter.jsonadapters/claude/pyproject.tomlcrates/fabric-cli/assets/adapters/claude/fabric-adapter.jsontests/adapters/test_adapter_package_metadata.pyadapters/common/src/nemo_fabric_adapters/common/utils.pytests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{json,jsonschema}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Public contract changes must keep checked-in JSON Schema snapshots synchronized.
Files:
adapters/claude/fabric-adapter.jsoncrates/fabric-cli/assets/adapters/claude/fabric-adapter.json
adapters/*/fabric-adapter.json
📄 CodeRabbit inference engine (.agents/skills/contribute-adapter/SKILL.md)
Define a narrow, truthful adapter descriptor; keep
config.accepts,config.generates, requirements, telemetry declarations, lifecycle capabilities, and advertised capabilities synchronized with implementation and tests.
Files:
adapters/claude/fabric-adapter.json
adapters/*/{README.md,fabric-adapter.json}
📄 CodeRabbit inference engine (.agents/skills/contribute-adapter/SKILL.md)
Document installation, supported configuration, harness-only settings, credentials, lifecycle, telemetry, artifacts, limitations, and focused test commands; keep documentation consistent with descriptor claims.
Files:
adapters/claude/fabric-adapter.json
{adapters/**,examples/**}
⚙️ CodeRabbit configuration file
{adapters/**,examples/**}: Review adapter and example changes for command correctness, config/schema consistency, artifact handling, and compatibility with the public NeMo Fabric contracts.
Files:
adapters/claude/fabric-adapter.jsonadapters/claude/pyproject.tomladapters/common/src/nemo_fabric_adapters/common/utils.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.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/claude/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/claude/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/claude/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/claude/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/claude/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/claude/pyproject.tomltests/adapters/test_adapter_package_metadata.pyadapters/common/src/nemo_fabric_adapters/common/utils.pytests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{toml,rs,py}
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
Avoid blind repository-wide replacement of version-like strings; distinguish package-version references from examples and unrelated dependency versions.
Files:
adapters/claude/pyproject.tomltests/adapters/test_adapter_package_metadata.pyadapters/common/src/nemo_fabric_adapters/common/utils.pytests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.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/claude/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/claude/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/claude/pyproject.toml
adapters/*/pyproject.toml
📄 CodeRabbit inference engine (.agents/skills/contribute-adapter/SKILL.md)
adapters/*/pyproject.toml: Keep leaf adapter runtime dependencies razor-thin and adapter-owned. Do not declare the wrapped harness/SDK or dependencies already declared by its supported package; provide dependency-free fallbacks for optional libraries.
Provide every installable Python leaf adapter with aharnessextra and afullextra for package-installable integrations; provide arelayextra when importing NeMo Relay Python APIs, but not when Relay is an external executable.
For packaged harnesses, declare and document aharnessversion constraint supported by upstream contracts and test evidence; do not claim a broader range than the evidence supports.
Files:
adapters/claude/pyproject.toml
adapters/*/{pyproject.toml,uv.lock}
📄 CodeRabbit inference engine (.agents/skills/contribute-adapter/SKILL.md)
Keep each adapter package independent and small, using Python package metadata and lock files as applicable.
Files:
adapters/claude/pyproject.toml
**/*.{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/common/src/nemo_fabric_adapters/common/utils.pytests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If Python code or a Python-facing adapter changes, run
just test-python.In Python SDK, adapters, examples, and tests, follow the existing style, use type annotations for public APIs, and keep native binding declarations synchronized with their Rust implementations.
Files:
tests/adapters/test_adapter_package_metadata.pyadapters/common/src/nemo_fabric_adapters/common/utils.pytests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{rs,py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*.{rs,py,pyi}: If public configuration types change, confirm schema snapshot tests injust test-rustpass and review generated schema diffs.
For schema or public contract changes, run both language suites and review changes underschemas/and generated API references.
Files:
tests/adapters/test_adapter_package_metadata.pyadapters/common/src/nemo_fabric_adapters/common/utils.pytests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
tests/adapters/**/*.py
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
tests/adapters/**/*.py: If an adapter or integration changes, run its focused tests.
For adapter behavior changes, run focused adapter tests undertests/adapters, then runjust test-python.
Files:
tests/adapters/test_adapter_package_metadata.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/common/src/nemo_fabric_adapters/common/utils.pytests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{md,mdx,yml,py,rs,sh}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/SKILL.md)
Keep documentation aligned with current NeMo Fabric behavior, repository layout, entry points, commands, package names, APIs, bindings, and support claims.
Files:
tests/adapters/test_adapter_package_metadata.pyadapters/common/src/nemo_fabric_adapters/common/utils.pytests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
tests/**/*.{rs,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
When adding functionality, include tests in the corresponding Rust crate or the relevant area under
tests/.
Files:
tests/adapters/test_adapter_package_metadata.pytests/adapters/test_claude_adapter.py
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Use pytest to run Python tests.
Do not add@pytest.mark.asyncioto tests; async tests are automatically detected by the async runner.
Do not add-> Nonereturn type annotations to test functions.
When mocking a class, useunittest.mock.MagicMockorAsyncMock, using thespecargument when necessary, rather than defining a new class.
Prefix mocked class names withmock, notfake.
Prefer pytest fixtures over helper methods.
If a fixture is needed in multiple test files, define it once inconftest.pyrather than repeating it.
Define fixtures using@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and a<fixture_name>_fixturefunction; specifyscopeonly when it is notfunction.
Preferpytest.mark.parametrizeover separate tests for different input types.
Use@pytest.mark.usefixtureswhen a fixture is needed but its returned value is unused or it returns no value.
Avoid defensive programming in tests; access expected values directly so missing data raises a clear failure, such as usingresults["data"]instead ofresults.get("data").
When adapter installation metadata changes, packaging metadata tests must directly assert that the root project depends unconditionally on the exact-versionnemo-fabric-runtimedistribution.
Packaging metadata tests must verify that each root harness extra delegates to the matching version of the leaf adapter'sharnessextra.
Packaging metadata tests must verify that bare leaf dependencies remain adapter-owned and that the rootadapter-testsdependency group installs each leaf through itsharnessextra.
Packaging metadata tests must verify that every leaf providesfull; only adapters importing NeMo Relay Python APIs providerelay, while adapters using an external Relay executable havefullequal toharness.
Files:
tests/adapters/test_adapter_package_metadata.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_claude_adapter.py
adapters/*/src/**/*.py
📄 CodeRabbit inference engine (.agents/skills/contribute-adapter/SKILL.md)
adapters/*/src/**/*.py: Implement adapters using the existing Fabricpythonorprocessrunner and normalized request/result contracts; do not add a runner or one-off abstraction for a single adapter.
Treat normalizedconfig, Fabric-resolved plans, andruntime_contextas authoritative; reserveharness.settingsfor adapter-wide behavior and apply precedence in the order: normalized config, plans/context, harness settings, descriptor/default values.
Reject conflicting duplicate declarations and unsupported behavior with actionable errors naming the field and supported alternatives; never silently drop configuration.
Validate dependency versions, hooks, and credentials before harness invocation, and never expose credential values in outputs, errors, events, logs, or fixtures.
Forward only required system, selected credential, telemetry, and documented harness-specific environment variables; never forward or log unrelated environment values.
Maintain one local adapter host per Fabric runtime across orderedstart→invoke*→stopoperations. Emit one JSON lifecycle response per request on stdout and diagnostics on stderr.
Return harness-level invocation failures as successful lifecycle responses containingresponse: null,failed: true, and structurederrorfields (code,message,retryable, and optionalmetadata).
Do not emit NeMo Relay stream records on adapter stdout; return exactly one terminal lifecycle response while streaming occurs through the SDK-owned out-of-band endpoint.
Scope workspace, generated configuration, state, sessions, and artifacts to the resolved runtime context, and isolate stateful adapter instances by Fabric runtime ID.
Usestartto initialize adapter-owned harness state, retain it for continuation across repeatedinvokecalls on the same runtime, and release it instop.
Files:
adapters/common/src/nemo_fabric_adapters/common/utils.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
🧠 Learnings (2)
📚 Learning: 2026-06-29T22:34:52.407Z
Learnt from: AjayThorve
Repo: NVIDIA/NeMo-Fabric PR: 27
File: adapters/codex-cli/fabric-adapter.json:13-15
Timestamp: 2026-06-29T22:34:52.407Z
Learning: In NeMo-Fabric adapter manifest files (e.g., `*/fabric-adapter.json`), keep `config.accepts` limited to the top-level Fabric capability sections that `resolve_capability_plan` consumes (such as `models`, `tools`, `mcp`, `skills`, `telemetry`). Do not add adapter-owned `harness.settings` keys to `config.accepts`; `harness.settings` should remain adapter-owned and be passed through unchanged.
Applied to files:
adapters/claude/fabric-adapter.jsoncrates/fabric-cli/assets/adapters/claude/fabric-adapter.json
📚 Learning: 2026-07-09T22:28:51.689Z
Learnt from: AjayThorve
Repo: NVIDIA/NeMo-Fabric PR: 43
File: adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py:164-168
Timestamp: 2026-07-09T22:28:51.689Z
Learning: In the NeMo-Fabric adapters, treat path values used in Fabric adapter configuration (including logic like `_resolve_path` in adapter.py) as config-root-relative. Do not apply `Path.expanduser()` (or otherwise apply `~`/home or shell-style expansion), because it will make the resolved paths normalize inconsistently across adapters. Also, do not rely on or add any resolution behavior that uses `harness.settings.cwd` as an override point for these adapter paths—`harness.settings.cwd` is explicitly unsupported in this adapter context.
Applied to files:
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
🪛 Ruff (0.16.1)
tests/adapters/test_claude_adapter.py
[warning] 543-543: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
[warning] 751-751: Unnecessary dict comprehension for iterable; use dict.fromkeys instead
Replace with dict.fromkeys(iterable))
(C420)
[warning] 905-907: Abstract raise to an inner function
(TRY301)
🔇 Additional comments (8)
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py (2)
33-35: LGTM!Also applies to: 166-253, 257-326, 363-398, 451-477, 529-645, 675-675, 745-778, 803-813, 887-915, 930-957, 1030-1064, 1123-1123
33-35: 📐 Maintainability & Code QualityRun the required Python suite.
The stated validation includes focused tests only. Run
just test-pythonbefore merge for this Python-facing adapter change.As per coding guidelines, “If Python code or a Python-facing adapter changes, run
just test-python.”Source: Coding guidelines
adapters/claude/fabric-adapter.json (1)
44-44: LGTM!adapters/claude/pyproject.toml (1)
28-28: 📐 Maintainability & Code QualityValidate and document the new direct dependency.
Record the functional need, alternatives considered, and why
nemo-fabric-adapter-contractis the narrowest fit. Run the required license-diff script and theattributions-rustandattributions-pythonpre-commit hooks. Confirm the lockfile and package metadata remain synchronized.As per coding guidelines, “For new dependencies, record the functional need, viable alternatives considered, why the selected dependency is the narrowest fit,” and run the license-diff and attribution checks.
Also applies to: 55-55
Source: Coding guidelines
crates/fabric-cli/assets/adapters/claude/fabric-adapter.json (1)
44-44: LGTM!adapters/common/src/nemo_fabric_adapters/common/utils.py (1)
234-264: LGTM!Also applies to: 306-306
tests/adapters/test_adapter_package_metadata.py (1)
69-69: LGTM!tests/adapters/test_claude_adapter.py (1)
30-31: LGTM!Also applies to: 51-96, 170-170, 203-246, 277-277, 335-356, 380-382, 419-419, 494-496, 510-510, 523-536, 539-541, 562-657, 684-684, 743-786, 849-849, 961-1018, 1089-1198, 1242-1290, 1343-1359, 1401-1422, 1435-1435
Signed-off-by: Zhongxuan Wang <daniewang@nvidia.com>
|
Fern docs preview: https://nvidia-preview-pull-request-197.docs.buildwithfern.com/nemo/fabric |
Signed-off-by: Zhongxuan Wang <daniewang@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 `@adapters/claude/src/nemo_fabric_adapters/claude/adapter.py`:
- Around line 954-955: Update invoke() to propagate the per-invocation
request_id into the Relay correlation context before processing each request,
while continuing to use runtime_id for runtime-scoped artifact handling. Add a
two-turn test that invokes the adapter with distinct request IDs and verifies
each request is correlated with its own ID.
🪄 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: bd03b7c1-ca85-4885-8e80-837e5ebd07d2
📒 Files selected for processing (2)
adapters/claude/src/nemo_fabric_adapters/claude/adapter.pytests/adapters/test_claude_adapter.py
📜 Review details
⏰ Context from checks skipped due to timeout. (16)
- GitHub Check: Preview docs
- GitHub Check: Test (Python 3.14, linux-arm64)
- GitHub Check: Test (Python 3.12, windows-amd64)
- GitHub Check: Test (Python 3.14, windows-amd64)
- GitHub Check: Test (Python 3.13, windows-amd64)
- GitHub Check: Test (Python 3.14, macos-arm64)
- GitHub Check: Test (Python 3.12, macos-arm64)
- GitHub Check: Test (Python 3.11, linux-arm64)
- GitHub Check: Test (Python 3.12, linux-arm64)
- GitHub Check: Test (Python 3.12, linux-amd64)
- GitHub Check: Test (Python 3.11, macos-arm64)
- GitHub Check: Test (Python 3.14, linux-amd64)
- GitHub Check: Test (Python 3.11, windows-amd64)
- GitHub Check: Test (Python 3.13, macos-arm64)
- GitHub Check: Test (Python 3.13, linux-amd64)
- GitHub Check: Pre-commit
🧰 Additional context used
📓 Path-based instructions (15)
**/*.{rs,py,pyi,json,yaml,yml}
📄 CodeRabbit inference engine (.agents/skills/contribute-api/SKILL.md)
Determine and update every affected public surface, including the CLI, PyO3 bindings, Python SDK, type stubs, schemas, and adapter contract, so they remain in parity.
Files:
tests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*
📄 CodeRabbit inference engine (.agents/skills/karpathy-guidelines/SKILL.md)
**/*: Before implementing, explicitly state assumptions, surface ambiguity and tradeoffs, present multiple interpretations when relevant, and ask for clarification rather than silently deciding or proceeding when requirements are unclear.
Prefer the minimum code needed to solve the requested problem: avoid speculative features, unnecessary abstractions, unrequested flexibility, and handling of impossible scenarios; simplify overcomplicated solutions.
When editing existing code, make surgical changes only: do not modify unrelated code, comments, formatting, or pre-existing dead code; match the existing style, and remove only unused imports, variables, or functions introduced by your changes.
Define verifiable success criteria for each task, such as writing regression tests for bugs and invalid-input tests for validation, then verify the implementation against those criteria. For multi-step work, state a brief plan with a verification check for each step.
**/*: Always spellNVIDIAin all caps; do not useNvidia,nvidia,nVidia,nVIDIA, orNV.
Usean NVIDIAbefore a noun, because the name begins with an “en” sound.
Do not add a registered trademark symbol afterNVIDIAwhen referring to the company; use trademark symbols with product names only when required by the document type or legal guidance.
Verify official capitalization, spacing, hyphenation, and spelling for NVIDIA and third-party product names; do not rewrite official product names for grammar or title-case rules.
Precede NVIDIA product names withNVIDIAon first mention when natural and accurate, and link the first mention when the destination helps the reader.
On first use, include the company name and full model qualifier when it helps identify the model; preserve official capitalization and punctuation, and use shorter family names only after establishing the full name.
For learning-oriented and developer content, do not force trademark symbols unless explicitly required; for press, ...
Files:
tests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{rs,py}
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
For native binding changes, run
cargo check -p fabric-python --locked.Use
snake_casefor functions and variables; usePascalCasefor Rust types and Python classes.
Files:
tests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If Python code or a Python-facing adapter changes, run
just test-python.In Python SDK, adapters, examples, and tests, follow the existing style, use type annotations for public APIs, and keep native binding declarations synchronized with their Rust implementations.
Files:
tests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{rs,py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*.{rs,py,pyi}: If public configuration types change, confirm schema snapshot tests injust test-rustpass and review generated schema diffs.
For schema or public contract changes, run both language suites and review changes underschemas/and generated API references.
Files:
tests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
tests/adapters/**/*.py
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
tests/adapters/**/*.py: If an adapter or integration changes, run its focused tests.
For adapter behavior changes, run focused adapter tests undertests/adapters, then runjust test-python.
Files:
tests/adapters/test_claude_adapter.py
**/*.{py,pyi,rs}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
For Python SDK or PyO3 binding changes, use
python-tests, run focused pytest tests first, thenjust test-python; rebuild withjust build-pythonwhen native code or packaging changes.Public contract changes must keep native Python binding declarations synchronized with their Rust implementations.
Files:
tests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{rs,py,toml}
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
When editing version helpers, verify every
nemo-fabric-*workspace package through Cargo metadata and reject a static version inpython/pyproject.toml.
Files:
tests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{toml,rs,py}
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
Avoid blind repository-wide replacement of version-like strings; distinguish package-version references from examples and unrelated dependency versions.
Files:
tests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
**/*.{md,mdx,yml,py,rs,sh}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/SKILL.md)
Keep documentation aligned with current NeMo Fabric behavior, repository layout, entry points, commands, package names, APIs, bindings, and support claims.
Files:
tests/adapters/test_claude_adapter.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.py
tests/**/*.{rs,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
When adding functionality, include tests in the corresponding Rust crate or the relevant area under
tests/.
Files:
tests/adapters/test_claude_adapter.py
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Use pytest to run Python tests.
Do not add@pytest.mark.asyncioto tests; async tests are automatically detected by the async runner.
Do not add-> Nonereturn type annotations to test functions.
When mocking a class, useunittest.mock.MagicMockorAsyncMock, using thespecargument when necessary, rather than defining a new class.
Prefix mocked class names withmock, notfake.
Prefer pytest fixtures over helper methods.
If a fixture is needed in multiple test files, define it once inconftest.pyrather than repeating it.
Define fixtures using@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and a<fixture_name>_fixturefunction; specifyscopeonly when it is notfunction.
Preferpytest.mark.parametrizeover separate tests for different input types.
Use@pytest.mark.usefixtureswhen a fixture is needed but its returned value is unused or it returns no value.
Avoid defensive programming in tests; access expected values directly so missing data raises a clear failure, such as usingresults["data"]instead ofresults.get("data").
When adapter installation metadata changes, packaging metadata tests must directly assert that the root project depends unconditionally on the exact-versionnemo-fabric-runtimedistribution.
Packaging metadata tests must verify that each root harness extra delegates to the matching version of the leaf adapter'sharnessextra.
Packaging metadata tests must verify that bare leaf dependencies remain adapter-owned and that the rootadapter-testsdependency group installs each leaf through itsharnessextra.
Packaging metadata tests must verify that every leaf providesfull; only adapters importing NeMo Relay Python APIs providerelay, while adapters using an external Relay executable havefullequal toharness.
Files:
tests/adapters/test_claude_adapter.py
{tests/**,python/tests/**}
⚙️ CodeRabbit configuration file
{tests/**,python/tests/**}: Tests should cover the behavior promised by the changed API surface, including error paths, lifecycle cleanup, and SDK/native parity where relevant.
Files:
tests/adapters/test_claude_adapter.py
adapters/*/src/**/*.py
📄 CodeRabbit inference engine (.agents/skills/contribute-adapter/SKILL.md)
adapters/*/src/**/*.py: Implement adapters using the existing Fabricpythonorprocessrunner and normalized request/result contracts; do not add a runner or one-off abstraction for a single adapter.
Treat normalizedconfig, Fabric-resolved plans, andruntime_contextas authoritative; reserveharness.settingsfor adapter-wide behavior and apply precedence in the order: normalized config, plans/context, harness settings, descriptor/default values.
Reject conflicting duplicate declarations and unsupported behavior with actionable errors naming the field and supported alternatives; never silently drop configuration.
Validate dependency versions, hooks, and credentials before harness invocation, and never expose credential values in outputs, errors, events, logs, or fixtures.
Forward only required system, selected credential, telemetry, and documented harness-specific environment variables; never forward or log unrelated environment values.
Maintain one local adapter host per Fabric runtime across orderedstart→invoke*→stopoperations. Emit one JSON lifecycle response per request on stdout and diagnostics on stderr.
Return harness-level invocation failures as successful lifecycle responses containingresponse: null,failed: true, and structurederrorfields (code,message,retryable, and optionalmetadata).
Do not emit NeMo Relay stream records on adapter stdout; return exactly one terminal lifecycle response while streaming occurs through the SDK-owned out-of-band endpoint.
Scope workspace, generated configuration, state, sessions, and artifacts to the resolved runtime context, and isolate stateful adapter instances by Fabric runtime ID.
Usestartto initialize adapter-owned harness state, retain it for continuation across repeatedinvokecalls on the same runtime, and release it instop.
Files:
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
{adapters/**,examples/**}
⚙️ CodeRabbit configuration file
{adapters/**,examples/**}: Review adapter and example changes for command correctness, config/schema consistency, artifact handling, and compatibility with the public NeMo Fabric contracts.
Files:
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
🧠 Learnings (1)
📚 Learning: 2026-07-09T22:28:51.689Z
Learnt from: AjayThorve
Repo: NVIDIA/NeMo-Fabric PR: 43
File: adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py:164-168
Timestamp: 2026-07-09T22:28:51.689Z
Learning: In the NeMo-Fabric adapters, treat path values used in Fabric adapter configuration (including logic like `_resolve_path` in adapter.py) as config-root-relative. Do not apply `Path.expanduser()` (or otherwise apply `~`/home or shell-style expansion), because it will make the resolved paths normalize inconsistently across adapters. Also, do not rely on or add any resolution behavior that uses `harness.settings.cwd` as an override point for these adapter paths—`harness.settings.cwd` is explicitly unsupported in this adapter context.
Applied to files:
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
🔇 Additional comments (2)
tests/adapters/test_claude_adapter.py (1)
543-544: LGTM!Also applies to: 547-551
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py (1)
33-33: LGTM!Also applies to: 171-178, 898-898, 919-919
Signed-off-by: Zhongxuan Wang <daniewang@nvidia.com>
Signed-off-by: Zhongxuan Wang <daniewang@nvidia.com>
Signed-off-by: Zhongxuan Wang <daniewang@nvidia.com>
Signed-off-by: Zhongxuan Wang <daniewang@nvidia.com>
AnuradhaKaruppiah
left a comment
AnuradhaKaruppiah
left a comment
There was a problem hiding this comment.
Functional review against FABRIC-184 typed-contract acceptance criteria.
Signed-off-by: Zhongxuan Wang <daniewang@nvidia.com>
Signed-off-by: Zhongxuan Wang <daniewang@nvidia.com>
Signed-off-by: Zhongxuan Wang <daniewang@nvidia.com>
Signed-off-by: Zhongxuan Wang <daniewang@nvidia.com>
Signed-off-by: Zhongxuan Wang <daniewang@nvidia.com>
Signed-off-by: Zhongxuan Wang <daniewang@nvidia.com>
Signed-off-by: Zhongxuan Wang <daniewang@nvidia.com>
Signed-off-by: Zhongxuan Wang <daniewang@nvidia.com>
Signed-off-by: Zhongxuan Wang <daniewang@nvidia.com>
|
/merge |
#### Overview Upgrade NeMo Fabric to NeMo Relay `>=0.7.2,<0.8` and move the public Relay observability configuration to schema v3. This is a hard migration: explicit schema-v2 configurations and removed flat OTLP/OpenInference fields are rejected with actionable errors. Configurations without an explicit observability version follow Relay 0.7's implicit-v3 behavior. Relay 0.7.2 is the narrowest supported dependency range for this contract. Retaining Relay 0.6 would preserve two configuration models across adapters and retain the Hermes compatibility constraint introduced in #206. The reviewed license delta is limited to `nemo-relay` changing from 0.6.0 to 0.7.2 and the addition of `nemo-relay-cli-bin` 0.7.2; both packages are Apache-2.0, with no new copyleft or unresolved licenses. Claude and Codex harness installations now depend directly on the supported `nemo-relay-cli-bin` distribution, so Platform Evaluator environments receive the version-matched gateway executable without the unused Relay Python package or separate Cargo/shell provisioning. Bare adapter installations remain unchanged. The Python SDK now requires Pydantic 2.12 or newer because the Relay v3 models use `Field(exclude_if=...)`; older Pydantic releases accept that metadata but do not apply it during serialization. This branch now incorporates merged #197's Claude v1alpha2 adapter contract. The conflict resolution preserves typed `AgentConfig` input while retaining Relay 0.7 validation and schema-v3 configuration. This PR supersedes #215. The branch moved from a fork to `NVIDIA/NeMo-Fabric` so the required NVSkills validation can run. #### Details - Replace the Relay observability v2 model with Relay 0.7's native v3 endpoint model across the Rust schema, Python SDK, and generated references. - Remove v2 rendering and version-negotiation code from the shared Relay adapter path. - Apply the v3 contract consistently across Claude, Codex, Deep Agents, and Hermes. - Reject explicit v2, malformed OpenTelemetry containers/endpoints, and legacy flat exporter fields before adapter setup or TOML materialization. - Update dependency metadata, lockfiles, attributions, examples, and integration guidance. - Package the standalone Relay PyPI CLI binary with Claude and Codex harness/full installs and remove separate Cargo CLI provisioning from CI. - Preserve the top-level Relay plugin configuration at version 1; only the observability component uses schema version 3. #### Validation - `just test-python` with the packaged NeMo Relay 0.7.2 CLI - 1,067 passed, 16 skipped - Focused cross-layer regression matrix before the packaging-only follow-up - 298 passed - `cargo fmt --all -- --check` - `just test-rust` - 117 passed - `cargo check -p fabric-python --locked` - `just test-typescript` - `just docs` - Full pre-commit - all 9 hooks passed, including workflow lint, lockfiles, attributions, and dependency-license checks - Clean, exact Claude and Codex leaf `harness` installs both exposed `nemo-relay 0.7.2` while excluding the `nemo-relay` Python distribution - Final-head `Runtime.invoke_stream()` checks with mocked inference: Claude and Codex streamed through the packaged CLI - 2 passed - Final-head `Runtime.invoke_stream()` checks with `nemo-relay` absent from `PATH`: Deep Agents and Hermes streamed through in-process Relay - 2 passed - Built both leaf wheels and verified `harness` and `full` metadata require `nemo-relay-cli-bin>=0.7.2,<0.8` - SDK-level AgentEvaluator matrix before the packaging-only follow-up with mocked inference and the actual NeMo Relay 0.7.2 CLI - 30/30: Codex 10/10, Claude 10/10, Deep Agents 5/5, and Hermes 5/5 - Verified 30 unique ATIF directories, paths, and session IDs; all trajectories use `ATIF-v1.7` - Verified schema v3 in all authored Relay JSON configurations and materialized `plugins.toml` files - Reviewed the lockfile license delta: `nemo-relay` 0.6.0 to 0.7.2 plus `nemo-relay-cli-bin` 0.7.2; all Apache-2.0 #### Where should the reviewer start? Start with `crates/fabric-core/src/config.rs` for the public v3 configuration contract and `adapters/common/src/nemo_fabric_adapters/common/relay_gateway.py` for the shared adapter validation boundary. The corresponding compatibility coverage is in `tests/adapters/test_adapters_common_relay_gateway.py` and `tests/python/test_sdk_contract.py`. For the merged Claude contract, review `adapters/claude/src/nemo_fabric_adapters/claude/adapter.py` and `tests/adapters/test_claude_adapter.py`. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Relates to [FABRIC-189: Update NeMo Relay dependency to 0.7](https://linear.app/nvidia/issue/FABRIC-189/update-nemo-relay-dependency-to-07) - Relates to #197 - Relates to #206 - Relates to #215 - [x] I confirm this contribution is my own work, or I have the right to submit it under this project's license. - [x] I searched existing issues and open pull requests, and this does not duplicate existing work. ## Summary by CodeRabbit - **New Features** - Added Relay observability schema v3 with typed OpenTelemetry endpoints, projections, mappings, headers, timeouts, and optional full-payload retention. - Added support for multiple telemetry endpoints and clearer exporter validation. - Relay gateways now use isolated user-scoped configuration. - **Bug Fixes** - Prevented unintended ambient or inherited Relay configuration from affecting runs. - Rejected unsupported legacy observability configurations with clearer errors. - **Documentation** - Updated installation, integration, API, and example guidance for Relay CLI versions 0.7.2–0.7.x and schema v3. - Clarified which adapter installation options include the Relay CLI. Authors: - Ajay Thorve (https://github.com/AjayThorve) Approvers: - Zhongxuan (Daniel) Wang (https://github.com/zhongxuanwang-nv) - David Gardner (https://github.com/dagardner-nv) URL: #216
2 participants
Overview
Move the Claude adapter to the
fabric.adapter/v1alpha2typed contract. Lifecycle startup now validatesAgentConfig; model, MCP, skills, tool policy, instructions, and runtime context use contract dataclasses while the preview invocation request stays opaque.Preserve Relay behavior with a backward-compatible typed model-name override in the shared Relay helper. No shared legacy helper is removed because Claude, Codex, Hermes, and Deep Agents still have callers.
Per-invocation Relay request-ID correlation requires SDK/Relay support and is deferred.
Validated with
pytest -q tests/integrations/test_harbor_runner.py::test_swebench_matrix_translates_harbor_inputs_to_typed_config tests/adapters/test_claude_adapter.py tests/adapters/test_adapter_package_metadata.py(67 passed).Where should the reviewer start?
Start with
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py, then the descriptor and focused lifecycle tests.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Relates to FABRIC-184
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.