feat(hermes): support native and CLI-wrapped Relay execution - #75
feat(hermes): support native and CLI-wrapped Relay execution#75bbednarski9 wants to merge 4 commits into
Conversation
Architecture and lifecycle diagramsOne adapter, two Relay ownership boundariesflowchart TD
F[NeMo Fabric runtime] --> H[Existing nvidia.fabric.hermes adapter]
H --> M{relay_launch_mode}
M -->|native_plugin default| N[Hermes Python execution]
N --> HP[Hermes observability/nemo_relay plugin]
HP --> RP[Relay Python managed LLM and tool APIs]
RP --> FP[First-party components]
RP --> DP[Dynamic plugins]
RP --> U1[Provider endpoint]
M -->|cli_wrapper opt-in| C[nemo-relay run --agent hermes]
C --> G[Transient Relay gateway and hooks]
G --> HC[Hermes CLI child]
HC --> G
G --> FP2[First-party components]
G --> DP2[Dynamic plugins]
G --> U2[Provider endpoint]
RP --> A[ATOF / ATIF / OpenInference artifacts]
G --> A
The provider request stays under the Hermes adapter in both cases. The difference is where Relay owns interception and lifecycle: inside the Python process for native mode, or at the transient gateway/process boundary for CLI-wrapper mode. Native-plugin invocationsequenceDiagram
participant Fabric as Fabric Hermes adapter
participant RelayPy as Relay Python host
participant Hermes as Hermes native runtime
participant Plugin as Relay components/plugins
participant Provider as Model provider
Fabric->>Fabric: Resolve Relay v2 config and dynamic specs
Fabric->>RelayPy: Initialize owned plugin activation
RelayPy->>Plugin: Validate and register in declared order
Fabric->>Hermes: Run one Hermes session
Hermes->>RelayPy: Managed LLM/tool request
RelayPy->>Plugin: Run interceptors and guardrails
RelayPy->>Provider: Dispatch effective request
Provider-->>RelayPy: Provider response
RelayPy->>Plugin: Run response/lifecycle hooks
RelayPy-->>Hermes: Effective response
Hermes-->>Fabric: Run result and session artifacts
Fabric->>RelayPy: Close activation after session teardown
CLI-wrapper invocationsequenceDiagram
participant Fabric as Fabric Hermes adapter
participant Lifecycle as Relay plugin lifecycle
participant Relay as nemo-relay run
participant Hermes as Hermes CLI child
participant Provider as Model provider
Fabric->>Fabric: Write config.toml and plugins.toml
Fabric->>Lifecycle: plugins add / enable / validate
Lifecycle-->>Fabric: Invocation-scoped activation receipt
Fabric->>Relay: run --agent hermes --plugin-config-path ...
Relay->>Relay: Start transient gateway and hooks
Relay->>Hermes: Start Hermes with preserved args/session
Hermes->>Relay: Provider request through gateway
Relay->>Provider: Dispatch intercepted request
Provider-->>Relay: Provider response
Relay-->>Hermes: Intercepted response
Hermes-->>Relay: Exit status
Relay->>Relay: Flush telemetry and stop gateway
Relay-->>Fabric: Exit status and artifacts
Configuration and artifact flowflowchart LR
FC[Fabric YAML / typed config] --> PC[Canonical Relay plugin document]
FC --> HM[Hermes launch settings]
PC --> V2[Relay observability v2]
PC --> DY[Ordered dynamic plugin specs]
V2 --> FS[ATOF file sinks]
V2 --> OI[OpenInference / OTLP mappings]
DY --> NA[Native owned activation]
DY --> CL[CLI lifecycle activation]
FS --> COL[Fabric artifact discovery]
OI --> COL
HM --> NA
HM --> CL
Review note: the draft intentionally records the Relay 0.6 package publication, Hermes compatibility PR, deterministic interception smoke, and CLI cancellation coverage as gates before moving to ready-for-review. |
WalkthroughRelay integration now requires observability v2, models ATOF output as typed file or stream sinks, supports canonical ChangesRelay v2 contracts and SDK models
Relay translation and plugin lifecycle
Hermes Relay launch modes
Validation and fixtures
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
a9296c6 to
76a0ed0
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: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/src/nemo_fabric/types.py (1)
754-766: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject mixed external and inline Relay plugin configuration.
Calling
enable_relay(plugin_config_path=...)after configuringobservability,components,dynamic_plugins, orpolicyproduces a mapping that the Rust resolver rejects. The reverse call order has the same problem.Validate the prospective Relay mapping before assignment, mirror the guard in
models.py, and test both call orders.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/src/nemo_fabric/types.py` around lines 754 - 766, Update the Relay configuration logic in enable_relay to reject mappings that mix plugin_config_path with inline observability, components, dynamic_plugins, or policy settings. Validate the prospective relay mapping before assigning it, using the same guard established in models.py, and add coverage for both call orders: external plugin configuration after inline settings and inline settings after plugin configuration.
🤖 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/common/src/nemo_fabric_adapters/common/utils.py`:
- Around line 293-347: Update the relay.dynamic_plugins loading path to validate
each spec’s plugin_id and kind before appending it to resolved, matching the
manifest identity rules used by the external plugins.dynamic branch: require a
string plugin_id and restrict kind to rust_dynamic or worker. Raise a clear
indexed ValueError for invalid identity data so relay_api_dynamic_plugins
receives only validated specs.
- Around line 363-386: Update the ATOF file-sink handling in the configuration
loop so multiple file sinks without explicit filenames receive distinct default
filenames and cannot overwrite each other. Preserve explicitly configured
sink["filename"] values, while deriving a deterministic unique filename from
each sink’s position or configuration for missing values; ensure the resulting
path remains under the resolved output directory.
- Around line 758-780: Add a bounded timeout to the subprocess.run call in
_run_relay_lifecycle_command and catch subprocess.TimeoutExpired, raising a
clear RuntimeError that identifies the Relay lifecycle action and timeout.
Preserve the existing nonzero-return handling for commands that complete
normally.
- Around line 350-361: Update normalize_relay_output_dirs to obtain the
identifier through the existing runtime_id(payload) helper instead of indexing
runtime_context(payload), preserving its ValueError handling. Rename the local
identifier variable to current_runtime_id and update all subsequent references
in the function to avoid shadowing the helper.
- Around line 388-402: Align the ATIF filename default used by
_relay_api_atif_config with normalize_relay_output_dirs by changing it to
"trajectory-{session_id}.atif.json". Keep the existing normalized-config
behavior and other ATIF settings unchanged so both paths produce consistent
artifact names.
- Around line 782-817: Update _relay_plugin_manifest_registered and
_attach_relay_dynamic_plugin_config to resolve relative manifest references
against plugin_config_path.parent before comparing them with the configured
target. Preserve absolute-path handling and use the same base-directory
resolution in both helpers so registration checks and config attachment remain
consistent regardless of the process working directory.
In `@adapters/hermes/README.md`:
- Around line 8-14: Update the native_plugin description in the adapter README
to state that Relay is activated through the Python API only when Relay
telemetry is enabled, while preserving the existing Python SDK and
dynamic-plugin host details.
In `@adapters/hermes/src/nemo_fabric_adapters/hermes/adapter.py`:
- Around line 416-449: The invoke_relay_wrapped_hermes function must replace
blocking subprocess.run with async subprocess execution, enforce a timeout, and
handle task cancellation by terminating the child process group. Preserve the
existing result shape and stdout/stderr handling, while ensuring hung
Relay/Hermes processes are bounded and cleaned up before returning or
propagating cancellation.
- Around line 367-380: Restrict the scope of native_relay_plugin_environment to
the synchronous _invoke_hermes call only; do not enter it across plugin
initialization, context entry, or awaits. Restructure the native execution flow
around _invoke_hermes so each invocation sets and restores
HERMES_NEMO_RELAY_PLUGINS_TOML immediately around that call, or serialize native
executions to prevent concurrent environment overrides.
In `@crates/fabric-core/src/config.rs`:
- Around line 685-699: The RelayDynamicPluginConfig contract currently accepts
empty plugin_id values; reject them consistently across Rust validation,
generated schema, and native Python declarations. Update validate_relay_config
and the RelayDynamicPluginConfig plugin_id schema/type metadata to require at
least one character, synchronize the corresponding Python declaration, and add a
regression test confirming invalid identifiers are rejected before provisioning.
In `@docs/sdk/python.mdx`:
- Around line 193-199: Regenerate the Python API reference for
FabricConfig.enable_relay so its documented signature includes
plugin_config_path, matching the current definition in models.py. Update the
generated nemo_fabric.models reference without changing the source API or
surrounding documentation.
In `@pyproject.toml`:
- Line 80: Update the nemo-relay dependency constraint in the project dependency
configuration to use an available published release instead of the nonexistent
0.6.0 version. Preserve the existing Relay extra dependency declaration and
compatible version constraint style.
In `@python/src/nemo_fabric/models.py`:
- Around line 269-275: Update RelayConfig to reject plugin_config_path when
inline observability, components, dynamic_plugins, or policy are also provided,
matching validate_relay_config. Remove dict fallbacks from sinks and
dynamic_plugins, replace arbitrary attribute_mappings dictionaries with their
concrete typed model, and update enable_relay to use model_validate for
dictionary inputs instead of dict conversion.
In `@tests/adapters/test_adapaters_common_utils.py`:
- Around line 547-550: Update the affected test to set RELAY_TOKEN through
os.environ["RELAY_TOKEN"] instead of monkeypatch.setenv, and remove the
now-unused monkeypatch fixture parameter from the test signature.
- Around line 546-565: The test
test_relay_api_plugin_config_uses_observability_v2_and_first_party_pii uses
hard-coded /tmp paths that trigger Ruff S108. Add the pytest tmp_path fixture,
derive both relay output-directory paths from it, and update the expected
serialized configuration values accordingly; apply the same change to the
related test section around the first-party PII assertions.
In `@tests/adapters/test_hermes_dual_mode.py`:
- Around line 267-279: Replace the hand-written _Activation mock with a
MagicMock fixture whose __aenter__ and __aexit__ attributes are AsyncMock
instances, preserving the report data and context-manager behavior used by the
tests. Update the relevant assertions to verify both async methods were awaited,
and remove the custom class and its annotation warnings.
- Around line 66-73: Extend
test_explicit_max_tokens_is_shared_by_native_and_cli_modes to exercise both CLI
configuration and native _invoke_hermes execution. Ensure _invoke_hermes uses
the shared model-level max_tokens value with the same precedence as
build_hermes_config, then assert the native AIAgent arguments receive 4096
instead of the hardcoded default.
---
Outside diff comments:
In `@python/src/nemo_fabric/types.py`:
- Around line 754-766: Update the Relay configuration logic in enable_relay to
reject mappings that mix plugin_config_path with inline observability,
components, dynamic_plugins, or policy settings. Validate the prospective relay
mapping before assigning it, using the same guard established in models.py, and
add coverage for both call orders: external plugin configuration after inline
settings and inline settings after plugin configuration.
🪄 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: 9ad83b3b-55c2-4e91-a51a-fc3ee750479d
📒 Files selected for processing (31)
.gitignoreadapters/common/src/nemo_fabric_adapters/common/relay_gateway.pyadapters/common/src/nemo_fabric_adapters/common/utils.pyadapters/deepagents/pyproject.tomladapters/hermes/README.mdadapters/hermes/src/nemo_fabric_adapters/hermes/adapter.pycrates/fabric-core/src/config.rscrates/fabric-core/src/error.rscrates/fabric-core/src/runtime.rsdocs/reference/api/python-library-reference/index.mddocs/reference/api/python-library-reference/nemo_fabric.models.mddocs/sdk/python.mdxexamples/code_review_agent/config.pyexamples/harbor/demo/task/environment/fabric/configs/hermes-relay.yamlpyproject.tomlpython/src/nemo_fabric/__init__.pypython/src/nemo_fabric/models.pypython/src/nemo_fabric/types.pyschemas/adapter-invocation.schema.jsonschemas/agent.schema.jsonschemas/effective-config.schema.jsonschemas/run-plan.schema.jsontests/adapters/test_adapaters_common_utils.pytests/adapters/test_adapters_common_relay_gateway.pytests/adapters/test_hermes_adapter.pytests/adapters/test_hermes_dual_mode.pytests/e2e/test_hermes_e2e.pytests/fixtures/file-config-agent/profiles/mcp-github.yamltests/fixtures/file-config-agent/profiles/relay-openinference.yamltests/fixtures/file-config-agent/profiles/relay.yamltests/python/test_sdk_contract.py
| entries = ((document.get("plugins") or {}).get("dynamic") or []) | ||
| if not isinstance(entries, list): | ||
| raise ValueError("plugins.dynamic must be an array of tables") | ||
| specs: list[dict[str, Any]] = [] | ||
| for index, entry in enumerate(entries): | ||
| if not isinstance(entry, dict): | ||
| raise ValueError(f"plugins.dynamic[{index}] must be a table") | ||
| manifest_value = entry.get("manifest") | ||
| if not isinstance(manifest_value, str) or not manifest_value: | ||
| raise ValueError(f"plugins.dynamic[{index}].manifest must be a string") | ||
| manifest_path = Path(manifest_value) | ||
| if not manifest_path.is_absolute(): | ||
| manifest_path = plugin_config_path.parent / manifest_path | ||
| manifest_path = manifest_path.resolve() | ||
| if not manifest_path.is_file(): | ||
| raise RuntimeError(f"Relay plugin manifest was not found: {manifest_path}") | ||
| with manifest_path.open("rb") as stream: | ||
| manifest = tomllib.load(stream) | ||
| identity = manifest.get("plugin") or {} | ||
| plugin_id = identity.get("id") | ||
| kind = identity.get("kind") | ||
| if not isinstance(plugin_id, str) or kind not in {"rust_dynamic", "worker"}: | ||
| raise ValueError( | ||
| f"plugins.dynamic[{index}] manifest has an invalid plugin identity" | ||
| ) | ||
| config = entry.get("config") or {} | ||
| if not isinstance(config, dict): | ||
| raise ValueError(f"plugins.dynamic[{index}].config must be a table") | ||
| specs.append( | ||
| { | ||
| "plugin_id": plugin_id, | ||
| "kind": kind, | ||
| "manifest_ref": str(manifest_path), | ||
| "config": copy.deepcopy(config), | ||
| } | ||
| ) | ||
| return specs | ||
|
|
||
| specs = relay.get("dynamic_plugins") or [] | ||
| if not isinstance(specs, list): | ||
| raise ValueError("relay.dynamic_plugins must be a list") | ||
| root = Path(config_root(payload)).resolve() | ||
| resolved: list[dict[str, Any]] = [] | ||
| for index, spec in enumerate(specs): | ||
| if not isinstance(spec, dict): | ||
| raise ValueError(f"relay.dynamic_plugins[{index}] must be an object") | ||
| item = copy.deepcopy(spec) | ||
| for field in ("manifest_ref", "environment_ref"): | ||
| value = item.get(field) | ||
| if value is None: | ||
| continue | ||
| path = Path(str(value)) | ||
| item[field] = str(path if path.is_absolute() else (root / path).resolve()) | ||
| resolved.append(item) | ||
| return resolved |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validation gap between the two dynamic-plugin loading paths.
The external plugins.dynamic TOML branch (lines 293-329) strictly validates manifest identity (plugin_id, kind in {"rust_dynamic", "worker"}) by reading the manifest file. The relay.dynamic_plugins branch (lines 331-347) performs no equivalent validation — it only checks that each entry is a dict and resolves path fields. If plugin_id/kind are missing or malformed in this second path, the failure surfaces later as an unhelpful KeyError in relay_api_dynamic_plugins (line 484-485) rather than a clear validation error here.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 295-295: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 299-299: Prefer TypeError exception for invalid type
(TRY004)
[warning] 299-299: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 302-302: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 308-308: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 315-317: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 320-320: Prefer TypeError exception for invalid type
(TRY004)
[warning] 320-320: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 333-333: Prefer TypeError exception for invalid type
(TRY004)
[warning] 333-333: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 338-338: Prefer TypeError exception for invalid type
(TRY004)
[warning] 338-338: Avoid specifying long messages outside the exception class
(TRY003)
🤖 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/common/src/nemo_fabric_adapters/common/utils.py` around lines 293 -
347, Update the relay.dynamic_plugins loading path to validate each spec’s
plugin_id and kind before appending it to resolved, matching the manifest
identity rules used by the external plugins.dynamic branch: require a string
plugin_id and restrict kind to rust_dynamic or worker. Raise a clear indexed
ValueError for invalid identity data so relay_api_dynamic_plugins receives only
validated specs.
| def normalize_relay_output_dirs( | ||
| plugin_config: dict[str, Any], payload: dict[str, Any] | ||
| ) -> None: | ||
| base = Path(config_root(payload)).resolve() | ||
| runtime_id = runtime_context(payload)["runtime_id"] | ||
| for component in plugin_config.get("components", []): | ||
| if component.get("kind") != "observability": | ||
| continue | ||
| config = component.setdefault("config", {}) | ||
| config.setdefault("version", 1) | ||
| for section_name in ("atof", "atif"): | ||
| section = config.get(section_name) | ||
| if not isinstance(section, dict) or not section.get("enabled"): | ||
| continue | ||
| version = int(config.setdefault("version", 2)) | ||
| if version != 2: | ||
| raise ValueError("NeMo Relay observability config version 2 is required") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Bypasses existing runtime_id() helper — inconsistent error handling.
Line 354 directly indexes runtime_context(payload)["runtime_id"], which raises a raw KeyError if missing. The module already defines runtime_id(payload) (lines 98-104) that validates this and raises a clear ValueError. Reuse it here for consistent error semantics.
🔧 Proposed fix
- base = Path(config_root(payload)).resolve()
- runtime_id = runtime_context(payload)["runtime_id"]
+ base = Path(config_root(payload)).resolve()
+ current_runtime_id = runtime_id(payload)(and update subsequent usages of the local runtime_id variable to current_runtime_id to avoid shadowing the module-level function name)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def normalize_relay_output_dirs( | |
| plugin_config: dict[str, Any], payload: dict[str, Any] | |
| ) -> None: | |
| base = Path(config_root(payload)).resolve() | |
| runtime_id = runtime_context(payload)["runtime_id"] | |
| for component in plugin_config.get("components", []): | |
| if component.get("kind") != "observability": | |
| continue | |
| config = component.setdefault("config", {}) | |
| config.setdefault("version", 1) | |
| for section_name in ("atof", "atif"): | |
| section = config.get(section_name) | |
| if not isinstance(section, dict) or not section.get("enabled"): | |
| continue | |
| version = int(config.setdefault("version", 2)) | |
| if version != 2: | |
| raise ValueError("NeMo Relay observability config version 2 is required") | |
| def normalize_relay_output_dirs( | |
| plugin_config: dict[str, Any], payload: dict[str, Any] | |
| ) -> None: | |
| base = Path(config_root(payload)).resolve() | |
| current_runtime_id = runtime_id(payload) | |
| for component in plugin_config.get("components", []): | |
| if component.get("kind") != "observability": | |
| continue | |
| config = component.setdefault("config", {}) | |
| version = int(config.setdefault("version", 2)) | |
| if version != 2: | |
| raise ValueError("NeMo Relay observability config version 2 is required") |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 361-361: Avoid specifying long messages outside the exception class
(TRY003)
🤖 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/common/src/nemo_fabric_adapters/common/utils.py` around lines 350 -
361, Update normalize_relay_output_dirs to obtain the identifier through the
existing runtime_id(payload) helper instead of indexing
runtime_context(payload), preserving its ValueError handling. Rename the local
identifier variable to current_runtime_id and update all subsequent references
in the function to avoid shadowing the helper.
| atof = config.get("atof") | ||
| if isinstance(atof, dict) and atof.get("enabled"): | ||
| sinks = atof.setdefault("sinks", []) | ||
| if not isinstance(sinks, list): | ||
| raise ValueError("Relay ATOF sinks must be a list") | ||
| if not any( | ||
| isinstance(sink, dict) and sink.get("type") == "file" for sink in sinks | ||
| ): | ||
| sinks.append({"type": "file"}) | ||
| for sink in sinks: | ||
| if not isinstance(sink, dict) or sink.get("type") != "file": | ||
| continue | ||
| output_directory = sink.get("output_directory") | ||
| path = ( | ||
| Path(output_directory) | ||
| if output_directory | ||
| else base / "artifacts" / "relay" | ||
| ) | ||
| if not path.is_absolute(): | ||
| path = base / path | ||
| else: | ||
| path = base / "artifacts" / "relay" | ||
|
|
||
| section["output_directory"] = str(path / str(runtime_id)) | ||
| Path(section["output_directory"]).mkdir(parents=True, exist_ok=True) | ||
| if section_name == "atof": | ||
| section.setdefault("filename", "events.atof.jsonl") | ||
| section.setdefault("mode", "overwrite") | ||
| if section_name == "atif": | ||
| section.setdefault("filename_template", "trajectory-{session_id}.atif.json") | ||
| section.setdefault("agent_name", agent_name(payload)) | ||
| section.setdefault("model_name", relay_model_name(payload)) | ||
| sink["output_directory"] = str(path / str(runtime_id)) | ||
| Path(sink["output_directory"]).mkdir(parents=True, exist_ok=True) | ||
| sink.setdefault("filename", "events.atof.jsonl") | ||
| sink.setdefault("mode", "overwrite") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Multiple ATOF file sinks share the same default output filename, risking collisions.
The loop at lines 372-386 assigns every type == "file" sink to base / "artifacts" / "relay" / runtime_id (or the sink's own output_directory joined the same way) and defaults filename to "events.atof.jsonl" for all of them. If a user configures two file sinks without explicit filenames (for example, to split by category), they will silently write to the same path and clobber each other's output.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 367-367: Avoid specifying long messages outside the exception class
(TRY003)
🤖 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/common/src/nemo_fabric_adapters/common/utils.py` around lines 363 -
386, Update the ATOF file-sink handling in the configuration loop so multiple
file sinks without explicit filenames receive distinct default filenames and
cannot overwrite each other. Preserve explicitly configured sink["filename"]
values, while deriving a deterministic unique filename from each sink’s position
or configuration for missing values; ensure the resulting path remains under the
resolved output directory.
| atif = config.get("atif") | ||
| if isinstance(atif, dict) and atif.get("enabled"): | ||
| output_directory = atif.get("output_directory") | ||
| path = ( | ||
| Path(output_directory) | ||
| if output_directory | ||
| else base / "artifacts" / "relay" | ||
| ) | ||
| if not path.is_absolute(): | ||
| path = base / path | ||
| atif["output_directory"] = str(path / str(runtime_id)) | ||
| Path(atif["output_directory"]).mkdir(parents=True, exist_ok=True) | ||
| atif.setdefault("filename_template", "trajectory-{session_id}.atif.json") | ||
| atif.setdefault("agent_name", agent_name(payload)) | ||
| atif.setdefault("model_name", relay_model_name(payload)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
ATIF filename_template default diverges from _relay_api_atif_config's default.
normalize_relay_output_dirs defaults filename_template to "trajectory-{session_id}.atif.json" (line 400), but _relay_api_atif_config (lines 556-558) defaults the same field to "nemo-relay-atif-{session_id}.json". If _relay_api_atif_config is ever invoked on a config that skipped normalization (or before normalization runs), the effective default filename silently differs from what normalize_relay_output_dirs/collect_relay_artifacts expect, producing inconsistent artifact naming.
🔧 Align defaults
- filename_template=value.get(
- "filename_template", "nemo-relay-atif-{session_id}.json"
- ),
+ filename_template=value.get(
+ "filename_template", "trajectory-{session_id}.atif.json"
+ ),🤖 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/common/src/nemo_fabric_adapters/common/utils.py` around lines 388 -
402, Align the ATIF filename default used by _relay_api_atif_config with
normalize_relay_output_dirs by changing it to
"trajectory-{session_id}.atif.json". Keep the existing normalized-config
behavior and other ATIF settings unchanged so both paths produce consistent
artifact names.
| def _run_relay_lifecycle_command( | ||
| executable: Path, | ||
| relay_config_path: Path, | ||
| args: list[str], | ||
| *, | ||
| env: dict[str, str], | ||
| cwd: Path, | ||
| ) -> None: | ||
| completed = subprocess.run( | ||
| [str(executable), "--config", str(relay_config_path), *args], | ||
| cwd=cwd, | ||
| env=env, | ||
| text=True, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.STDOUT, | ||
| check=False, | ||
| ) | ||
| if completed.returncode != 0: | ||
| action = " ".join(args[:2]) | ||
| raise RuntimeError( | ||
| f"NeMo Relay {action} failed with status {completed.returncode}" | ||
| ) | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Missing timeout on Relay CLI subprocess call.
subprocess.run at line 766 has no timeout, so if the Relay CLI hangs (for example during plugins add/enable/validate), this call blocks indefinitely with no recovery path. This is a critical external-call hazard for a lifecycle operation invoked during agent provisioning.
🔧 Add a timeout and handle expiry
- completed = subprocess.run(
+ completed = subprocess.run(
[str(executable), "--config", str(relay_config_path), *args],
cwd=cwd,
env=env,
text=True,
stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
check=False,
+ timeout=60,
)Wrap the call in a try/except subprocess.TimeoutExpired to raise a clear RuntimeError on hang.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 765-773: Command coming from incoming request
Context: subprocess.run(
[str(executable), "--config", str(relay_config_path), *args],
cwd=cwd,
env=env,
text=True,
stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.15.21)
[error] 766-766: subprocess call: check for execution of untrusted input
(S603)
[warning] 777-779: Avoid specifying long messages outside the exception class
(TRY003)
🤖 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/common/src/nemo_fabric_adapters/common/utils.py` around lines 758 -
780, Add a bounded timeout to the subprocess.run call in
_run_relay_lifecycle_command and catch subprocess.TimeoutExpired, raising a
clear RuntimeError that identifies the Relay lifecycle action and timeout.
Preserve the existing nonzero-return handling for commands that complete
normally.
| sinks: list[ | ||
| Annotated[ | ||
| RelayAtofFileSinkConfig | RelayAtofStreamSinkConfig, | ||
| Field(discriminator="type"), | ||
| ] | ||
| | dict[str, Any] | ||
| ] = Field(default_factory=list) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
models_file="$(fd -a '^models\.py$' python/src/nemo_fabric | head -n1)"
tests_file="$(fd -a '^test_sdk_contract\.py$' tests/python python/tests | head -n1)"
ast-grep outline "$models_file" \
--match 'RelayAtofConfig|RelayOtlpConfig|RelayDynamicPluginConfig|RelayConfig|FabricConfig' \
--view expanded
sed -n '243,290p;340,400p;544,594p' "$models_file"
rg -n -C3 'RelayAtofConfig|attribute_mappings|dynamic_plugins|plugin_config_path' "$tests_file"Repository: NVIDIA/NeMo-Fabric
Length of output: 1900
🏁 Script executed:
# Get the actual file content at the specified line ranges
models_file="python/src/nemo_fabric/models.py"
echo "=== Lines 269-275 (sinks union) ==="
sed -n '269,275p' "$models_file"
echo -e "\n=== Lines 355 (attribute_mappings) ==="
sed -n '355p' "$models_file"
echo -e "\n=== Lines 377-399 (dynamic_plugins) ==="
sed -n '377,399p' "$models_file"
echo -e "\n=== Lines 547-592 (RelayConfig full definition) ==="
sed -n '547,592p' "$models_file"
echo -e "\n=== Around plugin_config_path ==="
rg -n 'plugin_config_path' "$models_file" -B2 -A2Repository: NVIDIA/NeMo-Fabric
Length of output: 4120
🏁 Script executed:
# Check if there are model validators that enforce constraints
rg -n 'field_validator|model_validator' "python/src/nemo_fabric/models.py" -A5 | grep -A5 "RelayConfig\|dynamic_plugins\|attribute_mappings\|sinks"
# Check if RelayConfig has any validation logic for plugin_config_path conflicts
rg -n "class RelayConfig" "python/src/nemo_fabric/models.py" -A100 | head -n 120
# Look for Rust equivalents to understand parity requirements
fd -a 'models.rs' | head -n3 | xargs rg -l 'RelayConfig\|plugin_config_path' 2>/dev/null || echo "No Rust models.rs found in obvious locations"
# Check test coverage for these edge cases
rg -n 'plugin_config_path' "tests/" -t py -B2 -A2 | head -40Repository: NVIDIA/NeMo-Fabric
Length of output: 7889
🏁 Script executed:
# Search for Rust models or documentation about the plugin_config_path conflict
find . -name "*.rs" -type f | xargs rg -l 'RelayConfig\|plugin_config_path' 2>/dev/null | head -5
# Check if there's any validation logic in enable_relay or RelayConfig
rg -n 'class RelayConfig|enable_relay' "python/src/nemo_fabric/models.py" -A50 | grep -E 'validator|plugin_config_path' -B2 -A2
# Look for any test that validates rejecting plugin_config_path + inline config
rg -n 'plugin_config_path' "tests/" -t py --multiline-dotall -B5 -A5 | grep -E 'assert|raises|plugin_config_path.*components|plugin_config_path.*observability' -B3 -A3Repository: NVIDIA/NeMo-Fabric
Length of output: 2170
🏁 Script executed:
# Look for Rust source code more broadly, including fabric-core or relay
find . -name "*.rs" -type f -path "*/src/*" | head -20
# Check if there's documentation about plugin_config_path being mutually exclusive
rg -i 'plugin.config.path.*mutually|mutually.*plugin.config.path|external.*inline|inline.*external' . -t py -t md -t txt
# Verify whether RelayConfig actually allows both plugin_config_path and inline config simultaneously
# by checking the test that sets plugin_config_path alongside observability, components, dynamic_plugins
sed -n '770,825p' "tests/adapters/test_adapaters_common_utils.py"Repository: NVIDIA/NeMo-Fabric
Length of output: 2502
🏁 Script executed:
# Check Rust config.rs for RelayConfig and whether it rejects plugin_config_path + inline
rg -n 'plugin_config_path\|struct RelayConfig' "./crates/fabric-core/src/config.rs" -A30 | head -80
# Look for validation logic in Rust that would reject the coexistence
rg -n 'validate|check' "./crates/fabric-core/src/config.rs" | grep -i 'relay\|plugin' -B2 -A2
# Check if Python tests actually test the conflict scenario
rg -n 'enable_relay' "tests/sdk" -t py -A20 | grep -E 'plugin_config_path.*components|plugin_config_path.*observability|plugin_config_path.*dynamic_plugins' -B10 -A5Repository: NVIDIA/NeMo-Fabric
Length of output: 455
🏁 Script executed:
# Examine the Rust validate_relay_config function
sed -n '1862,1920p' "./crates/fabric-core/src/config.rs"
# Look for the RelayConfig struct definition in Rust
rg -n 'struct RelayConfig' "./crates/fabric-core/src/config.rs" -A40 | head -50
# Check test files for SDK contract tests
find . -name "test_sdk*.py" -o -name "*test*contract*.py" | head -10
# See how enable_relay is used in practice
rg -n 'enable_relay' "tests/" -t py -B2 -A15 | grep -E 'plugin_config_path|components|dynamic_plugins|observability' -B5 -A5 | head -50Repository: NVIDIA/NeMo-Fabric
Length of output: 8220
🏁 Script executed:
# Check if RelayConfig has any validators that enforce the plugin_config_path constraint
rg -n '`@model_validator`|`@field_validator`' "python/src/nemo_fabric/models.py" -A10 | grep -B5 -A5 'RelayConfig\|plugin_config_path'
# Look at what the current enable_relay test actually passes
sed -n '220,330p' "tests/python/test_sdk_contract.py"Repository: NVIDIA/NeMo-Fabric
Length of output: 3808
Add validators to RelayConfig to enforce Rust's plugin_config_path and inline-config mutual exclusivity, and use typed models for sinks and dynamic_plugins.
The Rust validator (validate_relay_config in fabric-core/src/config.rs) rejects any attempt to combine plugin_config_path with inline observability, components, dynamic_plugins, or policy. Python currently allows this invalid combination. Additionally, sinks and dynamic_plugins accept dict[str, Any] fallbacks that bypass discriminator validation, and attribute_mappings accepts arbitrary dict[str, str] without schema enforcement.
Remove the dict fallbacks from sinks and dynamic_plugins, use concrete models for attribute_mappings, and add a model validator to RelayConfig that rejects the plugin_config_path + inline-config combination:
Proposed changes
- sinks: list[
- Annotated[
- RelayAtofFileSinkConfig | RelayAtofStreamSinkConfig,
- Field(discriminator="type"),
- ]
- | dict[str, Any]
- ] = Field(default_factory=list)
+ sinks: list[
+ Annotated[
+ RelayAtofFileSinkConfig | RelayAtofStreamSinkConfig,
+ Field(discriminator="type"),
+ ]
+ ] = Field(default_factory=list)
+class RelayOtlpAttributeMapping(FabricBaseModel):
+ key: str
+ alias: str
+
- attribute_mappings: list[dict[str, str]] = Field(default_factory=list)
+ attribute_mappings: list[RelayOtlpAttributeMapping] = Field(default_factory=list)
- dynamic_plugins: list[RelayDynamicPluginConfig | dict[str, Any]] = Field(
+ dynamic_plugins: list[RelayDynamicPluginConfig] = Field(
default_factory=list
)
+class RelayConfig(FabricBaseModel):
+ ...
+ `@model_validator`(mode="before")
+ `@classmethod`
+ def reject_plugin_config_path_with_inline(cls, value: Any) -> Any:
+ if isinstance(value, Mapping):
+ plugin_config_path = value.get("plugin_config_path")
+ if plugin_config_path is not None and any(
+ value.get(k) for k in ["observability", "components", "dynamic_plugins", "policy"]
+ ):
+ raise ValueError(
+ "plugin_config_path cannot be combined with inline Relay observability, "
+ "components, dynamic_plugins, or policy"
+ )
+ return valueUpdate enable_relay to use model_validate for dict inputs instead of dict() conversion.
As per path instructions, Python SDK changes must preserve typed API consistency and parity with the native extension. This ensures the builder cannot emit configurations that Rust will reject at validation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sinks: list[ | |
| Annotated[ | |
| RelayAtofFileSinkConfig | RelayAtofStreamSinkConfig, | |
| Field(discriminator="type"), | |
| ] | |
| | dict[str, Any] | |
| ] = Field(default_factory=list) | |
| sinks: list[ | |
| Annotated[ | |
| RelayAtofFileSinkConfig | RelayAtofStreamSinkConfig, | |
| Field(discriminator="type"), | |
| ] | |
| ] = Field(default_factory=list) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/src/nemo_fabric/models.py` around lines 269 - 275, Update RelayConfig
to reject plugin_config_path when inline observability, components,
dynamic_plugins, or policy are also provided, matching validate_relay_config.
Remove dict fallbacks from sinks and dynamic_plugins, replace arbitrary
attribute_mappings dictionaries with their concrete typed model, and update
enable_relay to use model_validate for dictionary inputs instead of dict
conversion.
Sources: Coding guidelines, Path instructions
| monkeypatch: pytest.MonkeyPatch, | ||
| ): | ||
| pytest.importorskip("nemo_relay") | ||
| monkeypatch.setenv("RELAY_TOKEN", "test-only-value") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use os.environ instead of monkeypatch.setenv.
Set os.environ["RELAY_TOKEN"] and remove the unused fixture parameter.
As per coding guidelines, “Use os.environ to modify environment variables in tests; do not use monkeypatch.setenv.”
🤖 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_adapaters_common_utils.py` around lines 547 - 550, Update
the affected test to set RELAY_TOKEN through os.environ["RELAY_TOKEN"] instead
of monkeypatch.setenv, and remove the now-unused monkeypatch fixture parameter
from the test signature.
Source: Coding guidelines
| def test_explicit_max_tokens_is_shared_by_native_and_cli_modes(tmp_path: Path): | ||
| payload = _payload(tmp_path) | ||
| settings = common_utils.settings_payload(payload) | ||
| settings["max_tokens"] = 4096 | ||
|
|
||
| config = adapter.build_hermes_config(payload, relay_enabled=True) | ||
|
|
||
| assert config["model"]["max_tokens"] == 4096 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Exercise both execution paths in this parity test.
This only tests build_hermes_config(). Native _invoke_hermes still uses settings.get("max_tokens", 512), so model-level max_tokens is ignored while CLI mode honors it. Align the precedence and assert the actual native AIAgent arguments.
🤖 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_hermes_dual_mode.py` around lines 66 - 73, Extend
test_explicit_max_tokens_is_shared_by_native_and_cli_modes to exercise both CLI
configuration and native _invoke_hermes execution. Ensure _invoke_hermes uses
the shared model-level max_tokens value with the same precedence as
build_hermes_config, then assert the native AIAgent arguments receive 4096
instead of the hardcoded default.
| class _Activation: | ||
| def __init__(self) -> None: | ||
| self.report = {"diagnostics": []} | ||
| self.entered = False | ||
| self.closed = False | ||
|
|
||
| async def __aenter__(self): | ||
| self.entered = True | ||
| return self | ||
|
|
||
| async def __aexit__(self, *_args): | ||
| self.closed = True | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Replace the hand-written activation mock with MagicMock and AsyncMock.
Use a fixture with mocked __aenter__ and __aexit__ methods, then assert they were awaited. This also resolves the reported annotation warnings.
As per coding guidelines, “When mocking a class, use unittest.mock.MagicMock or AsyncMock; do not define a new mock class.”
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 273-273: Missing return type annotation for special method __aenter__
(ANN204)
[warning] 277-277: Missing return type annotation for special method __aexit__
(ANN204)
[warning] 277-277: Missing type annotation for *_args
(ANN002)
🤖 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_hermes_dual_mode.py` around lines 267 - 279, Replace the
hand-written _Activation mock with a MagicMock fixture whose __aenter__ and
__aexit__ attributes are AsyncMock instances, preserving the report data and
context-manager behavior used by the tests. Update the relevant assertions to
verify both async methods were awaited, and remove the custom class and its
annotation warnings.
Sources: Coding guidelines, Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/hermes/src/nemo_fabric_adapters/hermes/adapter.py`:
- Around line 299-304: Constrain the invocation_id derived in the adapter flow
before constructing isolation_root, rejecting absolute paths and traversal
components or validating the resolved path remains beneath hermes_home /
relay-cli. Preserve valid invocation IDs and ensure unsafe values cannot create
directories or write state outside the sandbox; add a regression test covering
absolute and parent-directory traversal IDs.
In `@tests/adapters/test_adapaters_common_utils.py`:
- Around line 756-763: Extend the test around common_utils.write_relay_configs
to verify that both returned paths, relay_path and plugin_path, correspond to
files written in config_directory. Add existence or content assertions for
config.toml and plugins.toml while preserving the existing path-value
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: a74d835e-c353-4df6-b3c6-85aa200157fe
📒 Files selected for processing (4)
adapters/common/src/nemo_fabric_adapters/common/utils.pyadapters/hermes/src/nemo_fabric_adapters/hermes/adapter.pytests/adapters/test_adapaters_common_utils.pytests/adapters/test_hermes_dual_mode.py
📜 Review details
🧰 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:
tests/adapters/test_hermes_dual_mode.pytests/adapters/test_adapaters_common_utils.pyadapters/common/src/nemo_fabric_adapters/common/utils.pyadapters/hermes/src/nemo_fabric_adapters/hermes/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:
tests/adapters/test_hermes_dual_mode.pytests/adapters/test_adapaters_common_utils.pyadapters/common/src/nemo_fabric_adapters/common/utils.pyadapters/hermes/src/nemo_fabric_adapters/hermes/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:
tests/adapters/test_hermes_dual_mode.pytests/adapters/test_adapaters_common_utils.pyadapters/common/src/nemo_fabric_adapters/common/utils.pyadapters/hermes/src/nemo_fabric_adapters/hermes/adapter.py
**/*.{rs,py,pyi,json,yaml,yml}
📄 CodeRabbit inference engine (.agents/skills/contribute-api/SKILL.md)
Determine and update every affected public surface, including the CLI, PyO3 bindings, Python SDK, type stubs, schemas, and adapter contract, so they remain in parity.
Files:
tests/adapters/test_hermes_dual_mode.pytests/adapters/test_adapaters_common_utils.pyadapters/common/src/nemo_fabric_adapters/common/utils.pyadapters/hermes/src/nemo_fabric_adapters/hermes/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_hermes_dual_mode.pytests/adapters/test_adapaters_common_utils.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:
tests/adapters/test_hermes_dual_mode.pytests/adapters/test_adapaters_common_utils.pyadapters/common/src/nemo_fabric_adapters/common/utils.pyadapters/hermes/src/nemo_fabric_adapters/hermes/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:
tests/adapters/test_hermes_dual_mode.pytests/adapters/test_adapaters_common_utils.pyadapters/common/src/nemo_fabric_adapters/common/utils.pyadapters/hermes/src/nemo_fabric_adapters/hermes/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_hermes_dual_mode.pytests/adapters/test_adapaters_common_utils.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_hermes_dual_mode.pytests/adapters/test_adapaters_common_utils.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/utils.pyadapters/hermes/src/nemo_fabric_adapters/hermes/adapter.py
🧠 Learnings (1)
📚 Learning: 2026-07-09T22:28:51.689Z
Learnt from: AjayThorve
Repo: NVIDIA/NeMo-Fabric PR: 43
File: adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py:164-168
Timestamp: 2026-07-09T22:28:51.689Z
Learning: In the NeMo-Fabric adapters, treat path values used in Fabric adapter configuration (including logic like `_resolve_path` in adapter.py) as config-root-relative. Do not apply `Path.expanduser()` (or otherwise apply `~`/home or shell-style expansion), because it will make the resolved paths normalize inconsistently across adapters. Also, do not rely on or add any resolution behavior that uses `harness.settings.cwd` as an override point for these adapter paths—`harness.settings.cwd` is explicitly unsupported in this adapter context.
Applied to files:
adapters/hermes/src/nemo_fabric_adapters/hermes/adapter.py
🧬 Code graph analysis (2)
adapters/common/src/nemo_fabric_adapters/common/utils.py (1)
tests/adapters/test_adapaters_common_utils.py (1)
write_relay_configs(666-666)
adapters/hermes/src/nemo_fabric_adapters/hermes/adapter.py (1)
adapters/common/src/nemo_fabric_adapters/common/utils.py (2)
runtime_id(99-105)runtime_context(95-96)
🪛 Ruff (0.15.21)
adapters/common/src/nemo_fabric_adapters/common/utils.py
[warning] 675-677: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (3)
tests/adapters/test_adapaters_common_utils.py (1)
750-753: Useos.environfor environment changes.This repeats the existing environment-mutation issue: use
os.environ.pop("FABRIC_RELAY_CONFIG_PATH", None)instead ofmonkeypatch.delenv(...), then remove the fixture parameter.Source: Coding guidelines
adapters/common/src/nemo_fabric_adapters/common/utils.py (1)
667-680: 🎯 Functional CorrectnessNormalize returned configuration paths before subprocess use.
If
config_directoryis relative, the files are written relative to the parent process, whileprepare_relay_cli_launchlater invokes Relay withcwd=config_root. This can make--configpoint to a different location. Normalize the directory here or enforce absolute paths at every caller.Suggested fix
if config_directory is None: config_path = os.environ.get("FABRIC_RELAY_CONFIG_PATH") if not config_path: raise RuntimeError( "FABRIC_RELAY_CONFIG_PATH is required when Relay is enabled" ) - config_dir = Path(config_path).parent / "relay-config" + config_dir = (Path(config_path).parent / "relay-config").resolve() else: - config_dir = Path(config_directory) + config_dir = Path(config_directory).resolve()tests/adapters/test_hermes_dual_mode.py (1)
210-212: LGTM!
| invocation_id = str( | ||
| common_utils.runtime_context(payload).get("invocation_id") | ||
| or common_utils.runtime_id(payload) | ||
| ) | ||
| isolation_root = hermes_home / "relay-cli" / invocation_id | ||
| isolation_root.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Constrain invocation_id before using it as a path.
An absolute or traversal-containing value such as /tmp/shared or ../../shared escapes hermes_home / relay-cli, allowing Relay configuration and XDG state to be written outside the intended invocation sandbox. Reject unsafe IDs or verify the resolved path remains beneath the isolation root, and add a traversal regression test.
Suggested containment check
invocation_id = str(
common_utils.runtime_context(payload).get("invocation_id")
or common_utils.runtime_id(payload)
)
- isolation_root = hermes_home / "relay-cli" / invocation_id
+ relay_root = (hermes_home / "relay-cli").resolve()
+ isolation_root = (relay_root / invocation_id).resolve()
+ try:
+ isolation_root.relative_to(relay_root)
+ except ValueError as exc:
+ raise ValueError(
+ "runtime_context.invocation_id escapes the Relay isolation root"
+ ) from exc
isolation_root.mkdir(parents=True, exist_ok=True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| invocation_id = str( | |
| common_utils.runtime_context(payload).get("invocation_id") | |
| or common_utils.runtime_id(payload) | |
| ) | |
| isolation_root = hermes_home / "relay-cli" / invocation_id | |
| isolation_root.mkdir(parents=True, exist_ok=True) | |
| invocation_id = str( | |
| common_utils.runtime_context(payload).get("invocation_id") | |
| or common_utils.runtime_id(payload) | |
| ) | |
| relay_root = (hermes_home / "relay-cli").resolve() | |
| isolation_root = (relay_root / invocation_id).resolve() | |
| try: | |
| isolation_root.relative_to(relay_root) | |
| except ValueError as exc: | |
| raise ValueError( | |
| "runtime_context.invocation_id escapes the Relay isolation root" | |
| ) from exc | |
| isolation_root.mkdir(parents=True, exist_ok=True) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@adapters/hermes/src/nemo_fabric_adapters/hermes/adapter.py` around lines 299
- 304, Constrain the invocation_id derived in the adapter flow before
constructing isolation_root, rejecting absolute paths and traversal components
or validating the resolved path remains beneath hermes_home / relay-cli.
Preserve valid invocation IDs and ensure unsafe values cannot create directories
or write state outside the sandbox; add a regression test covering absolute and
parent-directory traversal IDs.
| relay_path, plugin_path = common_utils.write_relay_configs( | ||
| relay_config={"agents": {"hermes": {"command": "hermes"}}}, | ||
| plugin_config={"version": 1, "components": []}, | ||
| config_directory=config_directory, | ||
| ) | ||
|
|
||
| assert relay_path == config_directory / "config.toml" | ||
| assert plugin_path == config_directory / "plugins.toml" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert that the isolated files are actually written.
The test checks only returned path values; it would pass even if either file were not created. Add file-existence or content assertions to cover the new output-directory behavior.
Suggested assertions
assert relay_path == config_directory / "config.toml"
assert plugin_path == config_directory / "plugins.toml"
+ assert relay_path is not None and relay_path.is_file()
+ assert plugin_path is not None and plugin_path.is_file()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| relay_path, plugin_path = common_utils.write_relay_configs( | |
| relay_config={"agents": {"hermes": {"command": "hermes"}}}, | |
| plugin_config={"version": 1, "components": []}, | |
| config_directory=config_directory, | |
| ) | |
| assert relay_path == config_directory / "config.toml" | |
| assert plugin_path == config_directory / "plugins.toml" | |
| relay_path, plugin_path = common_utils.write_relay_configs( | |
| relay_config={"agents": {"hermes": {"command": "hermes"}}}, | |
| plugin_config={"version": 1, "components": []}, | |
| config_directory=config_directory, | |
| ) | |
| assert relay_path == config_directory / "config.toml" | |
| assert plugin_path == config_directory / "plugins.toml" | |
| assert relay_path is not None and relay_path.is_file() | |
| assert plugin_path is not None and plugin_path.is_file() |
🤖 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_adapaters_common_utils.py` around lines 756 - 763, Extend
the test around common_utils.write_relay_configs to verify that both returned
paths, relay_path and plugin_path, correspond to files written in
config_directory. Add existence or content assertions for config.toml and
plugins.toml while preserving the existing path-value assertions.
Source: Path instructions
1 participant
Overview
Adds two explicit NeMo Relay execution strategies to the existing Hermes adapter while preserving the current native behavior as the default:
native_plugin: Hermes runs in-process and uses Relay's Python managed-execution/plugin APIs.cli_wrapper: Fabric launchesnemo-relay run --agent hermes, allowing Relay to own the transient gateway, hooks, child process, and cleanup lifecycle.This also aligns Fabric's typed Relay configuration with the Relay 0.6 contract: observability version 2, ATOF sinks, OpenInference/OTLP mark projection and attribute mappings, ordered dynamic-plugin activation specs, canonical
plugins.tomlinput, and sink-aware artifact discovery.The goal is to let the same Fabric Hermes evaluation run through either integration boundary without creating a second Hermes adapter or changing the provider request selected by the evaluation.
What changed
One Hermes adapter, two launch modes
harness.settings.relay_launch_modewithnative_pluginas the backward-compatible default andcli_wrapperas an opt-in mode.harness.settings.relay_cli_commandso evaluations can select an installed or source-built Relay CLI.observability/nemo_relayplugin enabled only in native mode; CLI mode delegates lifecycle ownership tonemo-relay run.RunOutputmetadata.Relay 0.6 typed configuration
plugins.tomlas the shared plugin configuration source.Invocation-scoped dynamic plugins
add/enable/validatelifecycle in invocation-isolated XDG directories before starting Hermes.Artifact and compatibility behavior
Where should the reviewer start?
adapters/hermes/src/nemo_fabric_adapters/hermes/adapter.pyfor launch-mode selection and lifecycle ownership.adapters/common/src/nemo_fabric_adapters/common/utils.pyfor canonical config loading, Relay 0.6 normalization, dynamic-plugin provisioning, and artifact discovery.crates/fabric-core/src/config.rsandpython/src/nemo_fabric/models.pyfor the typed public contract.tests/adapters/test_hermes_dual_mode.pyfor mode-specific regression coverage.docs/sdk/python.mdxfor the external configuration surface.Validation
Local regression checks after rebasing onto current
main(b7530e9):cargo fmt --all -- --checkcargo test --workspace --locked— passeduv run --no-sync pytest -q— 329 passed, 10 skippedgit diff --check— passedReal Terminal-Bench 2.0 smoke validation used the same
regex-logtask and qualified Anthropic model in both modes:cli_wrapper1.00native_plugin1.00Both runs preserved the selected provider/model and produced coherent Hermes session artifacts. Event counts are intentionally not expected to match because CLI mode observes the Relay gateway boundary while native mode observes Hermes's managed Python boundary.
Draft blockers and follow-up gates
This PR is intentionally draft:
nemo-relay~=0.6.0, but 0.6 is not yet available from the configured package index. The lockfiles therefore still resolve 0.5 anduv lock --checkcannot pass. Refresh the root and adapter lockfiles once 0.6 is published.Compatibility
native_pluginwhenrelay_launch_modeis absent.hermes_cliadapter is introduced.Related Issues and Work
Depends on NousResearch/hermes-agent#65104.
Builds on NeMo Relay's 0.6 Python dynamic-plugin host and transparent Hermes runner architecture.
This contribution is focused on one Hermes/Relay integration objective.
I searched for duplicate open NeMo Fabric PRs before opening this draft.
Summary by CodeRabbit
New Features
sinks(file + streaming) and enhanced file/ATIF handling.plugins.tomlsupport plus invocation-scoped dynamic plugin activation and provisioning.native_pluginandcli_wrapperlaunch modes with improved command/config isolation.Bug Fixes
Documentation