feat: add Claude Relay end-to-end support - #62
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (11)
🧰 Additional context used📓 Path-based instructions (9)**/*.py📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{rs,py}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{rs,py,pyi,json,yaml,yml}📄 CodeRabbit inference engine (.agents/skills/contribute-api/SKILL.md)
Files:
tests/**/*.py📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
Files:
**/*.{py,pyi}📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Files:
**/*.{rs,py,pyi,toml}📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Files:
tests/adapters/**/*📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Files:
{tests/**,python/tests/**}⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (1)tests/adapters/test_claude_adapter.py (2)
🪛 ast-grep (0.44.1)tests/adapters/test_claude_adapter.py[info] 658-658: use jsonify instead of json.dumps for JSON output (use-jsonify) 🪛 Ruff (0.15.21)tests/adapters/test_claude_adapter.py[warning] 633-633: Missing return type annotation for private function (ANN202) [warning] 633-633: Missing type annotation for (ANN003) 🔇 Additional comments (1)
WalkthroughThis change adds shared NeMo Relay gateway supervision, hook rendering, and observability configuration migration, then integrates Relay lifecycle, environment forwarding, plugins, artifacts, and structured errors into Claude and Codex adapters with expanded unit and end-to-end coverage. ChangesRelay observability integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Fabric
participant ClaudeAdapter
participant RelayGateway
participant ClaudeSDK
Fabric->>ClaudeAdapter: prepare_claude_relay(payload)
ClaudeAdapter->>RelayGateway: start_relay_gateway(launch, cwd)
ClaudeAdapter->>ClaudeSDK: query with Relay plugin and gateway environment
ClaudeSDK-->>ClaudeAdapter: messages and usage
ClaudeAdapter->>RelayGateway: stop_relay_gateway(process)
ClaudeAdapter-->>Fabric: normalized response with Relay artifacts
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
|
📖 Fern docs preview: https://nvidia-preview-pull-request-62.docs.buildwithfern.com/nemo/fabric |
e4c36bb to
af9f706
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/adapters/test_codex_cli.py (1)
429-445: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winStrengthen
relay_runtime/relay_artifactsassertions.The test only checks key presence (
"relay_runtime" in result,"relay_artifacts" in result) without asserting field values (e.g.,gateway_log_path,config_path, artifact contents). As per path instructions, "Tests should cover the behavior promised by the changed API surface." Add assertions on the actualrelay_runtimedict contents to catch regressions in the new relay output contract.🧪 Proposed strengthening
assert "relay_runtime" in result assert "relay_artifacts" in result + assert result["relay_runtime"]["gateway_log_path"] == str(gateway.log_path) + assert result["relay_artifacts"] == common_utils.collect_relay_artifacts( + relay_plugin_config + )🤖 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_cli.py` around lines 429 - 445, Strengthen the assertions in the test around adapter.run_codex by validating the actual relay_runtime fields, including gateway_log_path and config_path, and checking the expected relay_artifacts contents rather than only asserting key presence. Use the paths and artifact values established by codex_payload and the mocked gateway/config setup, while preserving the existing command, environment, and lifecycle assertions.Source: Path instructions
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py (1)
509-531: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRelay plugin presence incorrectly toggles skill/tool inclusion.
bool(plugins)is evaluated for bothtools=_normalized_tools(..., include_skills=...)andskills="all" if plugins else Noneafter the Relay plugin is appended toplugins. When Relay is enabled but no nativeskill_pathsare configured,_stage_skill_pluginreturns[], yet appending the relay plugin makespluginsnon-empty — so Claude getsskills="all"and a"Skill"tool added totools, even though no actual skill plugin exists. No test covers Relay-enabled-without-skills, so this regression isn't caught.🐛 Proposed fix
plugins = _stage_skill_plugin(payload) + has_skill_plugin = bool(plugins) if relay is not None: plugins.append({"type": "local", "path": str(relay.plugin_path)}) return ClaudeAgentOptions( resume=resume, cwd=resolve_cwd(payload), model=selected_model(payload), system_prompt=system_prompt, - tools=_normalized_tools(payload, include_skills=bool(plugins)), + tools=_normalized_tools(payload, include_skills=has_skill_plugin), allowed_tools=_string_list(settings.get("allowed_tools"), name="allowed_tools"), disallowed_tools=_string_list( settings.get("disallowed_tools"), name="disallowed_tools" ), permission_mode=permission_mode, max_turns=max_turns, max_budget_usd=max_budget, setting_sources=sources, cli_path=_resolve_path(payload, cli_path) if cli_path is not None else None, mcp_servers=_mcp_servers(payload), strict_mcp_config=True, - skills="all" if plugins else None, + skills="all" if has_skill_plugin else None, plugins=plugins,As per path instructions, tests should cover error paths and lifecycle behavior of the changed API surface; a regression test with
relayset but noskill_pathswould have caught this.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapters/claude/src/nemo_fabric_adapters/claude/adapter.py` around lines 509 - 531, Track whether native skill plugins were staged before appending the Relay plugin, and use that native-skill state—not bool(plugins)—for both _normalized_tools(..., include_skills=...) and the skills option in ClaudeAgentOptions. Preserve Relay in plugins while ensuring Relay-only configurations do not add the Skill tool or set skills="all"; add a regression test covering relay enabled with no skill_paths.Source: Path instructions
🤖 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/README.md`:
- Around line 188-198: Update the documented real_relay_gateway invocation to
use the configuration path consumed by the Python tests, or add the required
fixture/conftest bridge so FABRIC_NEMO_RELAY_COMMAND populates
settings["nemo_relay_command"]. Keep the command’s intended behavior of
resolving and testing the current nemo-relay CLI.
In `@adapters/claude/src/nemo_fabric_adapters/claude/adapter.py`:
- Around line 798-825: Update run_claude’s teardown handling so cleanup failures
from stop_relay_gateway or relay.plugin_path removal do not replace an already
successful output; return the valid result while surfacing cleanup diagnostics
through the established mechanism. Preserve raising cleanup_error when the
Claude operation itself did not produce a successful result, and extract relay
teardown/start logic into a focused helper or context manager to reduce
run_claude’s branching without changing behavior.
In `@adapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py`:
- Around line 569-579: Add gateway_config_path to the relay_runtime payload
constructed in the Codex adapter’s relay handling block, using the same relay
gateway configuration path source and field semantics as the Claude adapter.
Preserve the existing config_path, emitter, gateway_log_path, and
relay_artifacts fields.
In `@adapters/common/src/nemo_fabric_adapters/common/relay_gateway.py`:
- Around line 40-51: Remove the expanduser() call in resolve_relay_command and
construct the command directly from Path(value), preserving config-root-relative
resolution for non-absolute paths and executable lookup behavior for both Claude
and Codex adapters.
- Around line 161-170: Update the exception cleanup in the
wait_for_relay_gateway failure path to preserve or report any stop_relay_gateway
failure instead of silently swallowing it. Ensure the resulting
RelayGatewayError communicates both the readiness failure and cleanup failure
details while retaining the original error context.
In `@adapters/common/src/nemo_fabric_adapters/common/relay_hooks.py`:
- Line 52: Update the hook command construction in the relay hook logic around
command to safely handle executable paths containing spaces: quote
str(executable) with shlex.quote before interpolation, or switch to an exec-form
argument list that avoids shell parsing. Preserve the existing hook-forward and
agent arguments.
In `@tests/adapters/test_claude_adapter.py`:
- Around line 156-177: The relay_payload helper should follow the file’s pytest
fixture convention. Convert it to a fixture declared with
pytest.fixture(name="relay_payload") and rename the implementation to
relay_payload_fixture, preserving its configuration and telemetry setup; update
every test call site to request relay_payload as a fixture parameter instead of
invoking it with arguments.
---
Outside diff comments:
In `@adapters/claude/src/nemo_fabric_adapters/claude/adapter.py`:
- Around line 509-531: Track whether native skill plugins were staged before
appending the Relay plugin, and use that native-skill state—not
bool(plugins)—for both _normalized_tools(..., include_skills=...) and the skills
option in ClaudeAgentOptions. Preserve Relay in plugins while ensuring
Relay-only configurations do not add the Skill tool or set skills="all"; add a
regression test covering relay enabled with no skill_paths.
In `@tests/adapters/test_codex_cli.py`:
- Around line 429-445: Strengthen the assertions in the test around
adapter.run_codex by validating the actual relay_runtime fields, including
gateway_log_path and config_path, and checking the expected relay_artifacts
contents rather than only asserting key presence. Use the paths and artifact
values established by codex_payload and the mocked gateway/config setup, while
preserving the existing command, environment, and lifecycle assertions.
🪄 Autofix (Beta)
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: 6246c5f7-0236-4922-aa94-214c56f13deb
⛔ Files ignored due to path filters (2)
adapters/claude/uv.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
adapters/claude/README.mdadapters/claude/fabric-adapter.jsonadapters/claude/pyproject.tomladapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.pyadapters/common/src/nemo_fabric_adapters/common/relay_gateway.pyadapters/common/src/nemo_fabric_adapters/common/relay_hooks.pyadapters/common/src/nemo_fabric_adapters/common/utils.pytests/adapters/test_adapaters_common_utils.pytests/adapters/test_adapters_common_relay_gateway.pytests/adapters/test_adapters_common_relay_hooks.pytests/adapters/test_claude_adapter.pytests/adapters/test_codex_cli.pytests/e2e/test_claude.pytests/fixtures/claude/mock-claude-cli.py
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Pre-commit
- GitHub Check: Test (x86_64)
- GitHub Check: Test (arm64)
🧰 Additional context used
📓 Path-based instructions (21)
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.py: Python public APIs must use type annotations, and native Python binding declarations must remain synchronized with their Rust implementations.
Python files must begin with the specified#SPDX copyright and Apache-2.0 license header.
Files:
adapters/common/src/nemo_fabric_adapters/common/relay_hooks.pytests/adapters/test_adapaters_common_utils.pytests/fixtures/claude/mock-claude-cli.pytests/adapters/test_adapters_common_relay_hooks.pyadapters/common/src/nemo_fabric_adapters/common/relay_gateway.pytests/adapters/test_adapters_common_relay_gateway.pytests/e2e/test_claude.pyadapters/common/src/nemo_fabric_adapters/common/utils.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.pytests/adapters/test_codex_cli.pytests/adapters/test_claude_adapter.py
**/*.{rs,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{rs,py}: Usesnake_casefor Rust and Python functions and variables; usePascalCasefor Rust types and Python classes.
Run tests for every language surface affected by a change. Changes touching the Rust core or public schemas require both Rust and Python test suites.
Public contract changes must keep native Python binding declarations synchronized with their Rust implementations.
Files:
adapters/common/src/nemo_fabric_adapters/common/relay_hooks.pytests/adapters/test_adapaters_common_utils.pytests/fixtures/claude/mock-claude-cli.pytests/adapters/test_adapters_common_relay_hooks.pyadapters/common/src/nemo_fabric_adapters/common/relay_gateway.pytests/adapters/test_adapters_common_relay_gateway.pytests/e2e/test_claude.pyadapters/common/src/nemo_fabric_adapters/common/utils.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.pytests/adapters/test_codex_cli.pytests/adapters/test_claude_adapter.py
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: All source files must include the specified SPDX copyright and Apache-2.0 license header using the comment syntax appropriate to the file type.
Release tags must use raw Rust-compatible SemVer without a leadingv, such as0.1.0or0.1.0-rc.1.
**/*: 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.
**/*: Keep pull request branch scope coherent and reviewable.
Run relevant tests undervalidate-changebefore opening or updating a pull request.
Format changed files with the language-native formatter.
Update documentation and examples for public behavior changes.
Update dependent maintainer or consumer guidance when code changes affect APIs, bindings, commands, paths, packaging guidance, or best practices.
Use Conventional Commit style for pull request titles:<type>: <concise imperative summary>, choosing the type from the actual change surface. Usefixonly for user-facing or runtime product-code bug fixes.
A pull request body must include#### Overview,#### Details,#### Validation,#### Where should the reviewer start?, and `#### Related ...
Files:
adapters/common/src/nemo_fabric_adapters/common/relay_hooks.pytests/adapters/test_adapaters_common_utils.pytests/fixtures/claude/mock-claude-cli.pyadapters/claude/pyproject.tomltests/adapters/test_adapters_common_relay_hooks.pyadapters/claude/fabric-adapter.jsonadapters/claude/README.mdadapters/common/src/nemo_fabric_adapters/common/relay_gateway.pytests/adapters/test_adapters_common_relay_gateway.pytests/e2e/test_claude.pyadapters/common/src/nemo_fabric_adapters/common/utils.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.pytests/adapters/test_codex_cli.pytests/adapters/test_claude_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:
adapters/common/src/nemo_fabric_adapters/common/relay_hooks.pytests/adapters/test_adapaters_common_utils.pytests/fixtures/claude/mock-claude-cli.pytests/adapters/test_adapters_common_relay_hooks.pyadapters/claude/fabric-adapter.jsonadapters/common/src/nemo_fabric_adapters/common/relay_gateway.pytests/adapters/test_adapters_common_relay_gateway.pytests/e2e/test_claude.pyadapters/common/src/nemo_fabric_adapters/common/utils.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.pytests/adapters/test_codex_cli.pytests/adapters/test_claude_adapter.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
When Python code or a Python-facing adapter changes, run
just test-python.
Files:
adapters/common/src/nemo_fabric_adapters/common/relay_hooks.pytests/adapters/test_adapaters_common_utils.pytests/fixtures/claude/mock-claude-cli.pytests/adapters/test_adapters_common_relay_hooks.pyadapters/common/src/nemo_fabric_adapters/common/relay_gateway.pytests/adapters/test_adapters_common_relay_gateway.pytests/e2e/test_claude.pyadapters/common/src/nemo_fabric_adapters/common/utils.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.pytests/adapters/test_codex_cli.pytests/adapters/test_claude_adapter.py
**/*.{rs,py,pyi,toml}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
When the PyO3 bridge or package metadata changes, run
just build-pythonandcargo check -p fabric-python --locked.
Files:
adapters/common/src/nemo_fabric_adapters/common/relay_hooks.pytests/adapters/test_adapaters_common_utils.pytests/fixtures/claude/mock-claude-cli.pyadapters/claude/pyproject.tomltests/adapters/test_adapters_common_relay_hooks.pyadapters/common/src/nemo_fabric_adapters/common/relay_gateway.pytests/adapters/test_adapters_common_relay_gateway.pytests/e2e/test_claude.pyadapters/common/src/nemo_fabric_adapters/common/utils.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.pytests/adapters/test_codex_cli.pytests/adapters/test_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 Fabric contracts.
Files:
adapters/common/src/nemo_fabric_adapters/common/relay_hooks.pyadapters/claude/pyproject.tomladapters/claude/fabric-adapter.jsonadapters/claude/README.mdadapters/common/src/nemo_fabric_adapters/common/relay_gateway.pyadapters/common/src/nemo_fabric_adapters/common/utils.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/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 annotations to test functions.
When mocking a class, useunittest.mock.MagicMockorAsyncMock, supplyingspecwhen necessary; do not define a new mock class.
Prefix mocked class names withmock, notfake.
Prefer pytest fixtures over helper methods.
Define shared fixtures inconftest.pyrather than repeating them across test files.
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 return value is unused.
Useos.environto modify environment variables in tests; do not usemonkeypatch.setenv, because the autouserestore_environ_fixtureintests/conftest.pyrestores the environment after each test.
Avoid defensive programming in tests; access expected data directly so missing data raises a clear error instead of being silently tolerated.
Run focused tests withuv run pytest -k "<pattern>"and all tests withuv run pytest.
Files:
tests/adapters/test_adapaters_common_utils.pytests/fixtures/claude/mock-claude-cli.pytests/adapters/test_adapters_common_relay_hooks.pytests/adapters/test_adapters_common_relay_gateway.pytests/e2e/test_claude.pytests/adapters/test_codex_cli.pytests/adapters/test_claude_adapter.py
tests/adapters/**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
When an adapter or integration changes, run its focused tests under
tests/adapters, followed byjust test-python.
Files:
tests/adapters/test_adapaters_common_utils.pytests/adapters/test_adapters_common_relay_hooks.pytests/adapters/test_adapters_common_relay_gateway.pytests/adapters/test_codex_cli.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_adapaters_common_utils.pytests/fixtures/claude/mock-claude-cli.pytests/adapters/test_adapters_common_relay_hooks.pytests/adapters/test_adapters_common_relay_gateway.pytests/e2e/test_claude.pytests/adapters/test_codex_cli.pytests/adapters/test_claude_adapter.py
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{rs,toml}: Rust code must be formatted withcargo fmt --all; formatting can be checked withcargo fmt --all -- --check, and Rust workspaces must compile withcargo check --workspace --locked.
Rust files must begin with the specified//SPDX copyright and Apache-2.0 license header.When Rust code or Rust project configuration changes, run
cargo fmt --all -- --checkandjust test-rust.
Files:
adapters/claude/pyproject.toml
**/*.{toml,yaml,yml,sh,bash}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
TOML, YAML, and shell files must use the specified SPDX header with
#comments.
Files:
adapters/claude/pyproject.toml
**/pyproject.toml
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
Run
just build-pythonto verify that all Python package metadata resolves.
Files:
adapters/claude/pyproject.toml
**/*.{json,jsonc}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Public contract changes must keep checked-in JSON Schema snapshots synchronized.
Files:
adapters/claude/fabric-adapter.json
**/*.{md,mdx,html}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Changes affecting public behavior, adapters, examples, or workspace structure must update the corresponding documentation; public API changes require updated SDK or API reference documentation.
Files:
adapters/claude/README.md
**/README.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Update an adapter or example
README.mdwhen that adapter or example surface changes.
Files:
adapters/claude/README.md
**/*.{md,mdx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
For docs site changes, run
just docsto regenerate Python and Rust API references and validate Fern configuration.
**/*.{md,mdx}: Prioritize factual accuracy in NeMo Fabric documentation and keep commands, package names, APIs, file paths, repository layout, entry points, support claims, examples, and procedures aligned with current repository behavior.
Update relevant entry-point documentation when public behavior changes, includingREADME.md,docs/index.yml, package or crate READMEs, and adapter or integration READMEs.
Use{/* ... */}delimiters for top-of-file SPDX comments in MDX files, not HTML comment delimiters.
CapitalizeNVIDIAcorrectly and use consistent current repository terminology, product names, APIs, and feature names.
Format commands, code, expressions, file names, paths, and filenames as inline code where appropriate.
Use title case for technical-documentation headings.
Introduce code blocks, tables, and lists with complete lead-in sentences.
Use descriptive link text instead of raw URLs or generic labels such ashere.
Write procedures as short, imperative, parallel, easy-to-scan steps; prefer active voice, present tense, plain English, and concise sentences.
Useafterinstead ofoncewhen expressing temporal sequence, and usecaninstead ofmaywhen describing possibility rather than permission.
Use unambiguous date formats and avoid ordinal dates in body text.
When reviewing documentation, report findings in severity order underMust fix,Should fix, andNice to have, with file paths, line references, explanations, and concrete rewrites or directions.
Files:
adapters/claude/README.md
**/*.{html,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
HTML and Markdown files must use the specified SPDX header in an HTML comment.
Files:
adapters/claude/README.md
**/*.{md,rst}
📄 CodeRabbit inference engine (.agents/skills/contribute-api/SKILL.md)
Update documentation and examples in the same branch as the public API change.
Verify README and documentation entry points, package names, paths, examples, and public commands remain current after changes.
Files:
adapters/claude/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:
adapters/claude/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:
adapters/claude/README.md
🧠 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.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.pyadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py
🧬 Code graph analysis (4)
adapters/common/src/nemo_fabric_adapters/common/relay_gateway.py (1)
adapters/common/src/nemo_fabric_adapters/common/utils.py (1)
config_root(68-69)
adapters/common/src/nemo_fabric_adapters/common/utils.py (1)
tests/adapters/test_adapaters_common_utils.py (1)
write_relay_configs(402-402)
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py (1)
adapters/common/src/nemo_fabric_adapters/common/utils.py (6)
load_relay_plugin_config(193-217)collect_relay_artifacts(405-423)capability_plan(166-167)config_root(68-69)relay_enabled(157-158)runtime_context(85-86)
adapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py (1)
adapters/common/src/nemo_fabric_adapters/common/utils.py (5)
load_relay_plugin_config(193-217)collect_relay_artifacts(405-423)config_root(68-69)relay_enabled(157-158)runtime_context(85-86)
🪛 ast-grep (0.44.1)
tests/adapters/test_adapaters_common_utils.py
[info] 425-425: Do not hardcode temporary file or directory names
Context: "/tmp/atof"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
[info] 439-439: Do not hardcode temporary file or directory names
Context: "/tmp/atif"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
[info] 460-460: Do not hardcode temporary file or directory names
Context: "/tmp/atof"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
[info] 477-477: Do not hardcode temporary file or directory names
Context: "/tmp/atif"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
tests/fixtures/claude/mock-claude-cli.py
[warning] 17-17: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(env_log, "a", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[info] 19-24: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"ANTHROPIC_BASE_URL": os.environ.get("ANTHROPIC_BASE_URL"),
"NEMO_RELAY_GATEWAY_URL": os.environ.get("NEMO_RELAY_GATEWAY_URL"),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
adapters/common/src/nemo_fabric_adapters/common/relay_gateway.py
[error] 65-72: Command coming from incoming request
Context: subprocess.run(
[str(executable), "--version"],
stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
text=True,
timeout=RELAY_VERSION_TIMEOUT_SECONDS,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 142-154: Command coming from incoming request
Context: subprocess.Popen(
[
str(launch.executable),
"--config",
str(launch.config_path),
"--bind",
launch.bind,
],
cwd=cwd,
stdout=log_stream,
stderr=subprocess.STDOUT,
start_new_session=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[warning] 100-100: Request-controlled URL passed to urlopen; validate against an allowlist to prevent SSRF.
Context: urllib.request.urlopen(health_url, timeout=1)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(urlopen-unsanitized-data)
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
[warning] 445-445: Do not make http calls without encryption
Context: f"http://{gateway_bind}"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[info] 367-375: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"name": "nemo-fabric-relay",
"description": "NeMo Relay hooks managed by NeMo Fabric",
"version": "1.0.0",
},
indent=2,
sort_keys=True,
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 380-384: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
relay_hooks.render_relay_hooks("claude", executable),
indent=2,
sort_keys=True,
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
adapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py
[warning] 270-270: Do not make http calls without encryption
Context: f"http://{relay_gateway_bind}"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
tests/adapters/test_claude_adapter.py
[info] 158-167: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"relay": {
"config": {
"atof": {"enabled": True},
"atif": {"enabled": True},
}
}
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 608-608: use jsonify instead of json.dumps for JSON output
Context: json.dumps(output)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 Ruff (0.15.21)
adapters/common/src/nemo_fabric_adapters/common/relay_hooks.py
[warning] 50-50: Avoid specifying long messages outside the exception class
(TRY003)
tests/adapters/test_adapaters_common_utils.py
[error] 426-426: Probable insecure usage of temporary file or directory: "/tmp/atof"
(S108)
[error] 440-440: Probable insecure usage of temporary file or directory: "/tmp/atif"
(S108)
[error] 461-461: Probable insecure usage of temporary file or directory: "/tmp/atof"
(S108)
[error] 478-478: Probable insecure usage of temporary file or directory: "/tmp/atif"
(S108)
tests/adapters/test_adapters_common_relay_hooks.py
[warning] 9-9: Use from nemo_fabric_adapters.common import relay_hooks in lieu of alias
Replace with from nemo_fabric_adapters.common import relay_hooks
(PLR0402)
adapters/common/src/nemo_fabric_adapters/common/relay_gateway.py
[warning] 50-50: Avoid specifying long messages outside the exception class
(TRY003)
[error] 66-66: subprocess call: check for execution of untrusted input
(S603)
[warning] 75-77: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 80-80: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 97-99: Avoid specifying long messages outside the exception class
(TRY003)
[error] 101-101: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
[warning] 107-107: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 126-128: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 139-139: Avoid specifying long messages outside the exception class
(TRY003)
[error] 143-143: subprocess call: check for execution of untrusted input
(S603)
[warning] 157-159: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 164-167: Use contextlib.suppress(Exception) instead of try-except-pass
Replace try-except-pass with with contextlib.suppress(Exception): ...
(SIM105)
[error] 166-167: try-except-pass detected, consider logging the exception
(S110)
[warning] 166-166: Do not catch blind exception: Exception
(BLE001)
[warning] 168-170: Avoid specifying long messages outside the exception class
(TRY003)
tests/adapters/test_adapters_common_relay_gateway.py
[warning] 9-9: Use from nemo_fabric_adapters.common import relay_gateway in lieu of alias
Replace with from nemo_fabric_adapters.common import relay_gateway
(PLR0402)
tests/e2e/test_claude.py
[warning] 180-180: Async functions should not use pathlib.Path methods, use trio.Path or anyio.path
(ASYNC240)
[warning] 181-181: Async functions should not use pathlib.Path methods, use trio.Path or anyio.path
(ASYNC240)
[warning] 224-224: Async functions should not use pathlib.Path methods, use trio.Path or anyio.path
(ASYNC240)
adapters/common/src/nemo_fabric_adapters/common/utils.py
[warning] 434-436: Avoid specifying long messages outside the exception class
(TRY003)
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py
[warning] 32-32: Use from nemo_fabric_adapters.common import relay_gateway in lieu of alias
Replace with from nemo_fabric_adapters.common import relay_gateway
(PLR0402)
[warning] 33-33: Use from nemo_fabric_adapters.common import relay_hooks in lieu of alias
Replace with from nemo_fabric_adapters.common import relay_hooks
(PLR0402)
[warning] 126-126: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 136-136: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 149-149: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 704-704: Unnecessary dict comprehension for iterable; use dict.fromkeys instead
Replace with dict.fromkeys(iterable))
(C420)
[warning] 745-745: Too many branches (19 > 12)
(PLR0912)
[warning] 845-845: Do not catch blind exception: Exception
(BLE001)
adapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py
[warning] 20-20: Use from nemo_fabric_adapters.common import relay_gateway in lieu of alias
Replace with from nemo_fabric_adapters.common import relay_gateway
(PLR0402)
[warning] 21-21: Use from nemo_fabric_adapters.common import relay_hooks in lieu of alias
Replace with from nemo_fabric_adapters.common import relay_hooks
(PLR0402)
[warning] 75-75: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 116-118: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 194-196: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 220-222: Prefer TypeError exception for invalid type
(TRY004)
[warning] 220-222: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 287-289: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 341-343: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 370-372: Avoid specifying long messages outside the exception class
(TRY003)
tests/adapters/test_claude_adapter.py
[error] 440-440: Possible hardcoded password assigned to: "FABRIC_UNRELATED_SECRET"
(S105)
[error] 442-442: Possible hardcoded password assigned to: "FABRIC_UNRELATED_SECRET"
(S105)
[warning] 489-489: Missing return type annotation for private function query_result
(ANN202)
[warning] 489-489: Unused function argument: prompt
(ARG001)
[warning] 554-554: Missing return type annotation for private function query_failure
(ANN202)
[warning] 554-554: Unused function argument: prompt
(ARG001)
[warning] 554-554: Unused function argument: options
(ARG001)
🔇 Additional comments (16)
adapters/common/src/nemo_fabric_adapters/common/relay_gateway.py (1)
1-39: LGTM!Also applies to: 54-139, 171-171
tests/adapters/test_adapters_common_relay_gateway.py (1)
1-190: LGTM!adapters/claude/pyproject.toml (1)
30-30: LGTM!adapters/claude/README.md (2)
24-34: LGTM!Also applies to: 63-63
93-98: 🗄️ Data Integrity & Integration
relay_runtime.gateway_config_pathis already emitted by the Claude adapter.
The documented field name matches the adapter output, so no change is needed.> Likely an incorrect or invalid review comment.tests/fixtures/claude/mock-claude-cli.py (1)
17-28: LGTM!adapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py (1)
20-40: LGTM!Also applies to: 70-90, 106-145, 157-224, 252-346, 357-465, 475-591
tests/adapters/test_codex_cli.py (1)
1-17: LGTM!Also applies to: 18-58, 82-138, 141-240, 243-378, 472-676, 677-738
adapters/common/src/nemo_fabric_adapters/common/utils.py (1)
8-8: LGTM!Also applies to: 425-487, 508-516
tests/adapters/test_adapters_common_relay_hooks.py (1)
1-64: LGTM!tests/adapters/test_adapaters_common_utils.py (1)
414-481: LGTM!adapters/claude/src/nemo_fabric_adapters/claude/adapter.py (2)
469-537: LGTM on the rest ofbuild_options/child_environment/_relay_outputwiring (env forwarding, plugin/env masking, artifact reporting) — no issues beyond the items flagged above.Also applies to: 699-825
391-462: 🩺 Stability & AvailabilityRelay staging is already invocation-scoped.
FABRIC_RELAY_CONFIG_PATHlives under.fabric/<runtime_id>/<invocation_id>, soconfig_path.parent / "claude-plugin"is isolated per run and won’t collide across concurrent invocations.> Likely an incorrect or invalid review comment.adapters/claude/fabric-adapter.json (1)
10-20: LGTM! Adding"telemetry"toacceptsmatches the documented allowed set forconfig.accepts.tests/adapters/test_claude_adapter.py (1)
1-53: LGTM otherwise — solid coverage of the relay config/hooks/gateway/env/artifact contract.Also applies to: 121-153, 179-283, 296-347, 363-443, 445-611, 613-638
tests/e2e/test_claude.py (1)
1-269: LGTM!
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/adapters/test_codex_cli.py (2)
390-404: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winExercise a non-empty Relay artifact path.
Because
relay_plugin_confighas no components,relay_artifacts == []would still pass if artifact collection used the wrong source or failed to promote artifacts. Add a minimal artifact-bearing configuration and assert the promoted result.As per path instructions, adapter tests must cover the behavior promised by the changed API surface, including artifact handling.
Also applies to: 432-439
🤖 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_cli.py` around lines 390 - 404, Update the test setup around CodexSettings and relay_plugin_config to include one minimal relay component that produces a non-empty artifact, then assert relay_artifacts contains the expected promoted artifact after execution. Keep the existing gateway and plugin configuration flow intact while ensuring the assertion validates artifact collection from the changed API.Source: Path instructions
418-421: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCover gateway cleanup on failure paths.
This test verifies shutdown only after a successful invocation. Add a relay-enabled timeout or
OSErrorcase and assert thatstop_relay_gatewayis still called and a structured error is returned.As per path instructions, adapter tests must cover lifecycle cleanup and error paths.
Also applies to: 448-453
🤖 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_cli.py` around lines 418 - 421, Add a failure-path test alongside the existing relay gateway lifecycle test, using a relay-enabled timeout or OSError during invocation. Assert that stop_relay_gateway is called despite the failure and that the adapter returns the expected structured error response, while preserving the existing successful-invocation coverage.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/adapters/test_claude_adapter.py`:
- Around line 546-604: The test suite lacks coverage for the plugin-directory
cleanup failure path in run_claude. Add a companion test near
test_run_claude_preserves_result_when_relay_stop_fails that keeps
stop_relay_gateway successful, mocks shutil.rmtree to raise OSError, and
verifies the successful Claude result is preserved while completed is false,
failed is true, error and relay_runtime.cleanup_error contain the
claude_relay_cleanup_failed contract, and the raw cleanup exception is not
exposed.
---
Outside diff comments:
In `@tests/adapters/test_codex_cli.py`:
- Around line 390-404: Update the test setup around CodexSettings and
relay_plugin_config to include one minimal relay component that produces a
non-empty artifact, then assert relay_artifacts contains the expected promoted
artifact after execution. Keep the existing gateway and plugin configuration
flow intact while ensuring the assertion validates artifact collection from the
changed API.
- Around line 418-421: Add a failure-path test alongside the existing relay
gateway lifecycle test, using a relay-enabled timeout or OSError during
invocation. Assert that stop_relay_gateway is called despite the failure and
that the adapter returns the expected structured error response, while
preserving the existing successful-invocation coverage.
🪄 Autofix (Beta)
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: dceb5a0a-5bda-4fe1-80b7-59f9303fd218
📒 Files selected for processing (8)
adapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.pyadapters/common/src/nemo_fabric_adapters/common/relay_gateway.pyadapters/common/src/nemo_fabric_adapters/common/relay_hooks.pytests/adapters/test_adapters_common_relay_gateway.pytests/adapters/test_adapters_common_relay_hooks.pytests/adapters/test_claude_adapter.pytests/adapters/test_codex_cli.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Test (arm64)
- GitHub Check: Test (x86_64)
🧰 Additional context used
📓 Path-based instructions (10)
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.py: Python public APIs must use type annotations, and native Python binding declarations must remain synchronized with their Rust implementations.
Python files must begin with the specified#SPDX copyright and Apache-2.0 license header.
Files:
adapters/common/src/nemo_fabric_adapters/common/relay_hooks.pytests/adapters/test_adapters_common_relay_hooks.pytests/adapters/test_adapters_common_relay_gateway.pyadapters/common/src/nemo_fabric_adapters/common/relay_gateway.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.pytests/adapters/test_claude_adapter.pytests/adapters/test_codex_cli.py
**/*.{rs,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{rs,py}: Usesnake_casefor Rust and Python functions and variables; usePascalCasefor Rust types and Python classes.
Run tests for every language surface affected by a change. Changes touching the Rust core or public schemas require both Rust and Python test suites.
Public contract changes must keep native Python binding declarations synchronized with their Rust implementations.
Files:
adapters/common/src/nemo_fabric_adapters/common/relay_hooks.pytests/adapters/test_adapters_common_relay_hooks.pytests/adapters/test_adapters_common_relay_gateway.pyadapters/common/src/nemo_fabric_adapters/common/relay_gateway.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.pytests/adapters/test_claude_adapter.pytests/adapters/test_codex_cli.py
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: All source files must include the specified SPDX copyright and Apache-2.0 license header using the comment syntax appropriate to the file type.
Release tags must use raw Rust-compatible SemVer without a leadingv, such as0.1.0or0.1.0-rc.1.
**/*: 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.
**/*: Keep pull request branch scope coherent and reviewable.
Run relevant tests undervalidate-changebefore opening or updating a pull request.
Format changed files with the language-native formatter.
Update documentation and examples for public behavior changes.
Update dependent maintainer or consumer guidance when code changes affect APIs, bindings, commands, paths, packaging guidance, or best practices.
Use Conventional Commit style for pull request titles:<type>: <concise imperative summary>, choosing the type from the actual change surface. Usefixonly for user-facing or runtime product-code bug fixes.
A pull request body must include#### Overview,#### Details,#### Validation,#### Where should the reviewer start?, and `#### Related ...
Files:
adapters/common/src/nemo_fabric_adapters/common/relay_hooks.pytests/adapters/test_adapters_common_relay_hooks.pytests/adapters/test_adapters_common_relay_gateway.pyadapters/common/src/nemo_fabric_adapters/common/relay_gateway.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.pytests/adapters/test_claude_adapter.pytests/adapters/test_codex_cli.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:
adapters/common/src/nemo_fabric_adapters/common/relay_hooks.pytests/adapters/test_adapters_common_relay_hooks.pytests/adapters/test_adapters_common_relay_gateway.pyadapters/common/src/nemo_fabric_adapters/common/relay_gateway.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.pytests/adapters/test_claude_adapter.pytests/adapters/test_codex_cli.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
When Python code or a Python-facing adapter changes, run
just test-python.
Files:
adapters/common/src/nemo_fabric_adapters/common/relay_hooks.pytests/adapters/test_adapters_common_relay_hooks.pytests/adapters/test_adapters_common_relay_gateway.pyadapters/common/src/nemo_fabric_adapters/common/relay_gateway.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.pytests/adapters/test_claude_adapter.pytests/adapters/test_codex_cli.py
**/*.{rs,py,pyi,toml}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
When the PyO3 bridge or package metadata changes, run
just build-pythonandcargo check -p fabric-python --locked.
Files:
adapters/common/src/nemo_fabric_adapters/common/relay_hooks.pytests/adapters/test_adapters_common_relay_hooks.pytests/adapters/test_adapters_common_relay_gateway.pyadapters/common/src/nemo_fabric_adapters/common/relay_gateway.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.pytests/adapters/test_claude_adapter.pytests/adapters/test_codex_cli.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 Fabric contracts.
Files:
adapters/common/src/nemo_fabric_adapters/common/relay_hooks.pyadapters/common/src/nemo_fabric_adapters/common/relay_gateway.pyadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/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 annotations to test functions.
When mocking a class, useunittest.mock.MagicMockorAsyncMock, supplyingspecwhen necessary; do not define a new mock class.
Prefix mocked class names withmock, notfake.
Prefer pytest fixtures over helper methods.
Define shared fixtures inconftest.pyrather than repeating them across test files.
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 return value is unused.
Useos.environto modify environment variables in tests; do not usemonkeypatch.setenv, because the autouserestore_environ_fixtureintests/conftest.pyrestores the environment after each test.
Avoid defensive programming in tests; access expected data directly so missing data raises a clear error instead of being silently tolerated.
Run focused tests withuv run pytest -k "<pattern>"and all tests withuv run pytest.
Files:
tests/adapters/test_adapters_common_relay_hooks.pytests/adapters/test_adapters_common_relay_gateway.pytests/adapters/test_claude_adapter.pytests/adapters/test_codex_cli.py
tests/adapters/**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
When an adapter or integration changes, run its focused tests under
tests/adapters, followed byjust test-python.
Files:
tests/adapters/test_adapters_common_relay_hooks.pytests/adapters/test_adapters_common_relay_gateway.pytests/adapters/test_claude_adapter.pytests/adapters/test_codex_cli.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_adapters_common_relay_hooks.pytests/adapters/test_adapters_common_relay_gateway.pytests/adapters/test_claude_adapter.pytests/adapters/test_codex_cli.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.pyadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py
🧬 Code graph analysis (6)
adapters/common/src/nemo_fabric_adapters/common/relay_hooks.py (1)
tests/adapters/test_adapters_common_relay_hooks.py (1)
render_relay_hooks(35-35)
tests/adapters/test_adapters_common_relay_gateway.py (1)
adapters/common/src/nemo_fabric_adapters/common/relay_gateway.py (2)
RelayGatewayLaunch(31-38)RelayGatewayError(26-27)
adapters/common/src/nemo_fabric_adapters/common/relay_gateway.py (1)
tests/adapters/test_adapters_common_relay_gateway.py (2)
resolve_relay_command(21-21)start_relay_gateway(105-105)
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py (1)
tests/adapters/test_claude_adapter.py (2)
build_options(123-123)run_claude(447-447)
adapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py (1)
tests/adapters/test_codex_cli.py (1)
run_codex(430-430)
tests/adapters/test_claude_adapter.py (2)
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py (3)
ClaudeRelaySettings(87-92)prepare_claude_relay(392-463)run(843-853)adapters/common/src/nemo_fabric_adapters/common/relay_gateway.py (2)
RelayGatewayLaunch(31-38)RelayGatewayError(26-27)
🪛 ast-grep (0.44.1)
tests/adapters/test_claude_adapter.py
[info] 601-601: use jsonify instead of json.dumps for JSON output
Context: json.dumps(output)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 Ruff (0.15.21)
adapters/common/src/nemo_fabric_adapters/common/relay_gateway.py
[warning] 166-166: Do not catch blind exception: Exception
(BLE001)
[warning] 167-170: Avoid specifying long messages outside the exception class
(TRY003)
tests/adapters/test_claude_adapter.py
[warning] 575-575: Missing return type annotation for private function query_result
(ANN202)
[warning] 575-575: Unused function argument: prompt
(ARG001)
[warning] 575-575: Unused function argument: options
(ARG001)
🔇 Additional comments (13)
adapters/common/src/nemo_fabric_adapters/common/relay_gateway.py (2)
43-43: LGTM! Resolves the prior.expanduser()review comment — tilde paths are now treated as config-root-relative, matching the test's expectation.
161-176: 🎯 Functional Correctness
ExceptionGroupis supported here
adapters/commonalready requires Python>=3.11, so this runtime path is valid and doesn’t need a compatibility fallback.> Likely an incorrect or invalid review comment.adapters/common/src/nemo_fabric_adapters/common/relay_hooks.py (1)
8-8: LGTM!shlex.quote()resolves the prior review comment about unquoted executable paths breaking hook execution when the path contains spaces.Also applies to: 53-53
tests/adapters/test_adapters_common_relay_gateway.py (2)
25-37: LGTM! Good regression coverage for the fixed tilde-path handling.
160-194: 🎯 Functional CorrectnessTest correctly pins the new
ExceptionGroup-based cleanup-failure contract.Same caveat as in
relay_gateway.py: this test references the builtinExceptionGroupdirectly, so it (and the runtime code it tests) requires Python ≥3.11. Worth confirming the CI test matrix /requires-pythoncovers this.tests/adapters/test_adapters_common_relay_hooks.py (1)
32-32: LGTM! Exercises the space-in-path case that motivated theshlex.quotefix.Also applies to: 42-42
adapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py (3)
13-40: 🎯 Functional CorrectnessVerify every
CodexSettingsconsumer uses the new relay shape.The previous flat
relay_*fields were removed. Any remaining constructor or adapter-facing consumer using that shape will fail at runtime. Update all consumers and public documentation/type surfaces, or provide an intentional compatibility layer.As per coding guidelines, every affected public surface must remain in parity.
Source: Coding guidelines
116-118: LGTM!Also applies to: 127-127, 194-196, 220-222, 264-301, 317-319, 332-343, 370-372, 406-408, 455-464, 483-486, 503-507, 531-549, 569-578, 589-590
70-84: 🎯 Functional CorrectnessDrop the stricter
thread_idtype check —load_thread_id()already converts any truthy persisted value withstr(...)beforebuild_command()uses it, so a numeric JSONthread_idwon’t reach subprocess as a non-string argument.> Likely an incorrect or invalid review comment.tests/adapters/test_codex_cli.py (1)
9-9: LGTM!Also applies to: 162-167, 187-192, 224-226, 235-237, 253-269, 280-284, 299-301, 315-323, 364-366, 478-480, 501-503, 563-565, 591-593, 635-637, 659-661, 718-721
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py (2)
823-839: 🩺 Stability & AvailabilityPrior "discarded successful result on cleanup failure" bug is fixed.
This now preserves
outputand foldscleanup_errorintooutput["relay_runtime"]["cleanup_error"], only overwritingerror/completed/failedwhen the run wasn't already failed — matchingtest_run_claude_preserves_result_when_relay_stop_fails. Resolves the previously flagged critical issue.
509-512: LGTM!Also applies to: 519-519, 531-531
tests/adapters/test_claude_adapter.py (1)
156-177: LGTM!
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
|
/merge |
2 participants
Overview
Add optional NeMo Relay observability to the Claude Agent SDK adapter while preserving Claude session continuation and Relay-disabled behavior. The change also extracts the gateway lifecycle and canonical hook rendering needed by the planned Codex SDK Relay path.
Details
nvidia.fabric.claudeand add the required optional runtime dependencies.nemo-relayexecutable per invocation, translate normalized Relay intent to the installed CLI contract, and generate invocation-scoped gateway/plugin configuration.ClaudeAgentOptionsusing an invocation-scoped Claude plugin,NEMO_RELAY_GATEWAY_URL, andANTHROPIC_BASE_URL.UserPromptExpansionevent and wildcard tool matchers.Validation
uv run --no-sync pytest tests/adapters/test_adapters_common_relay_hooks.py tests/adapters/test_adapters_common_relay_gateway.py tests/adapters/test_claude_adapter.py tests/adapters/test_codex_cli.py tests/e2e/test_claude.py— 80 passed, 3 skipped.just --set no_uv true test-python— 296 passed, 10 skipped.FABRIC_NEMO_RELAY_COMMAND="$(command -v nemo-relay)" uv run --no-sync pytest tests/e2e/test_claude.py::test_fabric_claude_accepts_real_relay_gateway_with_mock_claude— passed withnemo-relay 0.4.0.uvx ruff check adapters/common/src/nemo_fabric_adapters/common/relay_gateway.py adapters/common/src/nemo_fabric_adapters/common/relay_hooks.py adapters/claude/src/nemo_fabric_adapters/claude/adapter.py adapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py tests/adapters/test_adapters_common_relay_gateway.py tests/adapters/test_adapters_common_relay_hooks.py tests/adapters/test_claude_adapter.py tests/adapters/test_codex_cli.py— passed.just --fmt --check— passed.git diff --check— passed.RUN_FABRIC_CLAUDE_RELAY_INTEGRATION=1credentialed Claude plus Relay integration. The PR remains draft until that billable acceptance gate passes.Where should the reviewer start?
Start with
adapters/common/src/nemo_fabric_adapters/common/relay_gateway.pyfor the lifecycle contract andadapters/common/src/nemo_fabric_adapters/common/relay_hooks.pyfor the Relay-owned compatibility snapshot. Then reviewprepare_claude_relay()andrun_claude()inadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyfor the Claude-specific binding and cleanup semantics.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Resolves FABRIC-58
I confirm this contribution is my own work, or I have the right to submit it under this project's license.
I searched existing issues and open pull requests, and this does not duplicate existing work.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests