Add support for native Codex telemetry - #31
Conversation
Signed-off-by: David Gardner <dagardner@nvidia.com>
…based upon it being named 'plugins.toml' and being in the same directory as the config.toml. There is no way to provide an explicit path to the plugin toml Signed-off-by: David Gardner <dagardner@nvidia.com>
…trings not a path to a toml file, fix placement of the codex --config flag to not conflict with relay's flags Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
…rmes specific. Update codex adapter to use it Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
WalkthroughAdds explicit relay/native telemetry selection across schemas, core runtime, SDK normalization, and Hermes validation. Centralizes shared payload and relay-config helpers. Reworks the Codex CLI adapter for generated config files and relay gateway lifecycle. Adds new relay and native example profiles. ChangesTelemetry provider concept (schema, core, SDK)
Common payload/session/relay-config utilities
Codex CLI adapter relay gateway and profile generation
New composable telemetry example profiles
Sequence Diagram(s)sequenceDiagram
participant main
participant write_config_files
participant start_relay_gateway
participant CodexProcess
main->>write_config_files: payload
write_config_files-->>main: CodexSettings
main->>start_relay_gateway: cwd, CodexSettings
start_relay_gateway-->>main: relay process ready
main->>CodexProcess: build_command + build_env
CodexProcess-->>main: output
main->>start_relay_gateway: stop after cleanup
sequenceDiagram
participant TelemetryConfig
participant resolve_telemetry_plan
participant TelemetryPlan
participant runtime_telemetry_context
participant telemetry_ref
TelemetryConfig->>resolve_telemetry_plan: provider, enabled
resolve_telemetry_plan->>TelemetryPlan: provider, relay_enabled
TelemetryPlan->>runtime_telemetry_context: telemetry metadata
runtime_telemetry_context->>telemetry_ref: runtime telemetry
telemetry_ref->>TelemetryPlan: provider in metadata
Related issues: None found in the provided context. Related PRs: None found in the provided context. Suggested labels: telemetry, codex-cli, relay, schema-change Suggested reviewers: Reviewers familiar with fabric-core telemetry config, Codex CLI relay integration, and common adapter utilities. Poem Relay and native paths now part, Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@adapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py`:
- Around line 285-296: `write_config_files()` and `get_codex_profile_path()`
ignore `settings["codex_profile"]`, so the configured profile is never used and
docs are now misleading. Update the Codex CLI adapter to either read and honor
`harness.settings.codex_profile` when building the profile path, or remove that
setting consistently from the README and any related fixture/config references.
Use the existing symbols `write_config_files`, `get_codex_profile_path`, and
`common_utils.settings_payload` to keep the behavior and documentation aligned.
In `@adapters/common/src/nemo_fabric_adapters/common/utils.py`:
- Around line 41-47: `load_payload` currently falls back to `sys.stdin` even
when `FABRIC_INVOCATION` is set to an invalid path, which hides
misconfiguration. Update `load_payload` in the utils module so that if
`FABRIC_INVOCATION` is present but `Path(invocation_path).is_file()` is false,
it raises a clear error instead of reading from stdin; keep the stdin path only
for the case where `FABRIC_INVOCATION` is unset.
In `@tests/test_adapaters_common_hermes.py`:
- Around line 96-121: Remove the explicit `-> None` return annotations from the
test functions `test_validate_hermes_telemetry_provider_accepts_relay` and
`test_validate_hermes_telemetry_provider_rejects_native` in the Hermes test
module, and keep the rest of the test logic unchanged. Follow the test-style
rule for `tests/**/*.py` by leaving pytest test function signatures unannotated
while preserving the existing `hermes_common.validate_hermes_telemetry_provider`
assertions and parameterization.
In `@tests/test_adapaters_common_utils.py`:
- Around line 210-246: Add a test for write_relay_configs covering the (None,
None) case to lock down the expected behavior when no configs are provided. In
test_write_relay_configs, include a parameter set where both relay_config and
plugin_config are None and assert whether FABRIC_RELAY_CONFIG_PATH is still
required or whether the function should return empty/None paths. Use the
existing write_relay_configs symbol and keep the assertions aligned with the
current config-file path handling.
- Around line 232-234: The test is violating the environment փոփոխation
guideline by using monkeypatch.setenv in the test setup. Update the fixture or
test around the FABRIC_RELAY_CONFIG_PATH assignment to write directly to
os.environ instead of calling monkeypatch.setenv, and keep the change localized
in the same test helper block so restore_environ_fixture can handle cleanup.
- Around line 59-81: Update the two `test_load_payload_*` tests to modify
environment variables through `os.environ` rather than `monkeypatch.setenv` and
`monkeypatch.delenv`, since `restore_environ_fixture` already resets env state
between tests. In `test_load_payload_reads_fabric_invocation`, assign
`FABRIC_INVOCATION` directly in `os.environ` before calling
`common_utils.load_payload()`, and in `test_load_payload_falls_back_to_stdin`,
remove that key from `os.environ` before exercising the stdin fallback; keep the
existing `common_utils.load_payload` assertions unchanged.
🪄 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: f146c4ff-c0b2-4b79-bcff-2154b33b537d
📒 Files selected for processing (33)
.agents/skills/python-tests/SKILL.mdadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.pyadapters/common/src/nemo_fabric_adapters/common/hermes.pyadapters/common/src/nemo_fabric_adapters/common/utils.pyadapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.pyadapters/hermes-sdk/src/nemo_fabric_adapters/hermes_sdk/adapter.pycrates/fabric-core/src/config.rscrates/fabric-core/src/lib.rscrates/fabric-core/src/runtime.rsexamples/code-review-agent/profiles/hermes-cli-relay-otel.yamlexamples/code-review-agent/profiles/hermes-cli-relay.yamlexamples/code-review-agent/profiles/hermes-relay-cli-openinference.yamlexamples/code-review-agent/profiles/hermes-relay-openinference.yamlexamples/code-review-agent/profiles/hermes-relay-otel.yamlexamples/code-review-agent/profiles/hermes-relay.yamlexamples/code-review-agent/profiles/native-otel.yamlexamples/code-review-agent/profiles/relay-openinference.yamlexamples/code-review-agent/profiles/relay-otel.yamlexamples/code-review-agent/profiles/relay.yamlpython/src/nemo_fabric/types.pyschemas/adapter-invocation.schema.jsonschemas/agent.schema.jsonschemas/effective-config.schema.jsonschemas/run-plan.schema.jsontests/smoke_cli.pytests/smoke_relay_integration.pytests/test_adapaters_common_hermes.pytests/test_adapaters_common_utils.pytests/test_codex_cli.pytests/test_hermes_cli.pytests/test_hermes_e2e.pytests/test_hermes_sdk_adapter.pytests/test_sdk_contract.py
💤 Files with no reviewable changes (6)
- examples/code-review-agent/profiles/hermes-relay-openinference.yaml
- examples/code-review-agent/profiles/hermes-relay-otel.yaml
- examples/code-review-agent/profiles/hermes-relay-cli-openinference.yaml
- examples/code-review-agent/profiles/hermes-relay.yaml
- examples/code-review-agent/profiles/hermes-cli-relay.yaml
- examples/code-review-agent/profiles/hermes-cli-relay-otel.yaml
📜 Review details
⚠️ CI failures not shown inline (2)
GitHub Actions: Rust / 0_Test.txt: Add support for native Codex telemetry
Conclusion: failure
##[group]Run cargo test --workspace --locked
�[36;1mcargo test --workspace --locked�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
CACHE_ON_FAILURE: false
##[endgroup]
�[1m�[92m Downloading�[0m crates ...
�[1m�[92m Downloaded�[0m cfg-if v1.0.4
�[1m�[92m Downloaded�[0m strsim v0.11.1
�[1m�[92m Downloaded�[0m clap_lex v1.1.0
�[1m�[92m Downloaded�[0m bitflags v2.13.0
�[1m�[92m Downloaded�[0m anstyle-query v1.1.5
�[1m�[92m Downloaded�[0m utf8parse v0.2.2
�[1m�[92m Downloaded�[0m cfg_aliases v0.2.1
�[1m�[92m Downloaded�[0m anstyle-parse v1.0.0
�[1m�[92m Downloaded�[0m clap_derive v4.6.1
�[1m�[92m Downloaded�[0m clap v4.6.1
�[1m�[92m Downloaded�[0m anstyle v1.0.14
�[1m�[92m Downloaded�[0m anstream v1.0.0
�[1m�[92m Downloaded�[0m is_terminal_polyfill v1.70.2
�[1m�[92m Downloaded�[0m ctrlc v3.5.2
�[1m�[92m Downloaded�[0m colorchoice v1.0.5
�[1m�[92m Downloaded�[0m clap_builder v4.6.0
�[1m�[92m Downloaded�[0m nix v0.31.3
�[1m�[92m Compiling�[0m serde_core v1.0.228
�[1m�[92m Compiling�[0m itoa v1.0.18
�[1m�[92m Compiling�[0m zmij v1.0.21
�[1m�[92m Compiling�[0m equivalent v1.0.2
�[1m�[92m Compiling�[0m memchr v2.8.1
�[1m�[92m Compiling�[0m hashbrown v0.17.1
�[1m�[92m Compiling�[0m indexmap v2.14.0
�[1m�[92m Compiling�[0m ref-cast v1.0.25
�[1m�[92m Compiling�[0m unsafe-libyaml v0.2.11
�[1m�[92m Compiling�[0m serde v1.0.228
�[1m�[92m Compiling�[0m serde_json v1.0.150
�[1m�[92m Compiling�[0m utf8parse v0.2.2
�[1m�[92m Compiling�[0m cfg_aliases v0.2.1
�[1m�[92m Compiling�[0m libc v0.2.186
�[1m�[92m Compiling�[0m dyn-clone v1.0.20
�[1m�[92m Compiling�[0m ryu v1.0.23
�[1m�[92m Compiling�[0m serde_yaml v0.9.34+deprecated
�[1m�[92m Compiling�[0m sche...
GitHub Actions: Rust / Test: Add support for native Codex telemetry
Conclusion: failure
##[group]Run cargo test --workspace --locked
�[36;1mcargo test --workspace --locked�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
CACHE_ON_FAILURE: false
##[endgroup]
�[1m�[92m Downloading�[0m crates ...
�[1m�[92m Downloaded�[0m cfg-if v1.0.4
�[1m�[92m Downloaded�[0m strsim v0.11.1
�[1m�[92m Downloaded�[0m clap_lex v1.1.0
�[1m�[92m Downloaded�[0m bitflags v2.13.0
�[1m�[92m Downloaded�[0m anstyle-query v1.1.5
�[1m�[92m Downloaded�[0m utf8parse v0.2.2
�[1m�[92m Downloaded�[0m cfg_aliases v0.2.1
�[1m�[92m Downloaded�[0m anstyle-parse v1.0.0
�[1m�[92m Downloaded�[0m clap_derive v4.6.1
�[1m�[92m Downloaded�[0m clap v4.6.1
�[1m�[92m Downloaded�[0m anstyle v1.0.14
�[1m�[92m Downloaded�[0m anstream v1.0.0
�[1m�[92m Downloaded�[0m is_terminal_polyfill v1.70.2
�[1m�[92m Downloaded�[0m ctrlc v3.5.2
�[1m�[92m Downloaded�[0m colorchoice v1.0.5
�[1m�[92m Downloaded�[0m clap_builder v4.6.0
�[1m�[92m Downloaded�[0m nix v0.31.3
�[1m�[92m Compiling�[0m serde_core v1.0.228
�[1m�[92m Compiling�[0m itoa v1.0.18
�[1m�[92m Compiling�[0m zmij v1.0.21
�[1m�[92m Compiling�[0m equivalent v1.0.2
�[1m�[92m Compiling�[0m memchr v2.8.1
�[1m�[92m Compiling�[0m hashbrown v0.17.1
�[1m�[92m Compiling�[0m indexmap v2.14.0
�[1m�[92m Compiling�[0m ref-cast v1.0.25
�[1m�[92m Compiling�[0m unsafe-libyaml v0.2.11
�[1m�[92m Compiling�[0m serde v1.0.228
�[1m�[92m Compiling�[0m serde_json v1.0.150
�[1m�[92m Compiling�[0m utf8parse v0.2.2
�[1m�[92m Compiling�[0m cfg_aliases v0.2.1
�[1m�[92m Compiling�[0m libc v0.2.186
�[1m�[92m Compiling�[0m dyn-clone v1.0.20
�[1m�[92m Compiling�[0m ryu v1.0.23
�[1m�[92m Compiling�[0m serde_yaml v0.9.34+deprecated
�[1m�[92m Compiling�[0m sche...
🧰 Additional context used
📓 Path-based instructions (7)
{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:
examples/code-review-agent/profiles/relay.yamlexamples/code-review-agent/profiles/relay-otel.yamlexamples/code-review-agent/profiles/native-otel.yamlexamples/code-review-agent/profiles/relay-openinference.yamladapters/hermes-sdk/src/nemo_fabric_adapters/hermes_sdk/adapter.pyadapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.pyadapters/common/src/nemo_fabric_adapters/common/hermes.pyadapters/common/src/nemo_fabric_adapters/common/utils.pyadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py
.agents/skills/python-tests/**
⚙️ CodeRabbit configuration file
.agents/skills/python-tests/**: ---
name: python-tests
description: Python tests for Fabric; use this when writing tests
author: NVIDIA Corporation and Affiliates
license: Apache-2.0Python Test Style
- Pytest is used to run tests.
- Do not add
@pytest.mark.asyncioto any test. Async tests are automatically detected and run by the async runner; the decorator is unnecessary clutter.- Do not add a
-> Nonereturn type annotation to test functions. This is not a common convention in pytest and adds unnecessary verbosity.- When mocking a class, do not define a new class. Use
unittest.mock.MagicMockorunittest.mock.AsyncMock, with thespecconstructor argument when necessary.- The name of the mocked class should be prefixed with
mock, notfake.- Prefer pytest fixtures over helper methods.
- Do not repeat fixtures, if a fixture is needed in multiple test files, place it in a
conftest.pyfile.- When creating a fixture follow this pattern:
Only specify the scope argument when the value is something other than "function".`@pytest.fixture`(name="<fixture_name>"[, scope="<scope>"]) def <fixture_name>_fixture() -> <return_type>: ...- Prefer
pytest.mark.parametrizeover creating individual tests for
different input types.- If a fixture is needed for a test, but either does not return a value or the value is not used in the test, use the
@pytest.mark.usefixturesdecorator.Common Commands
# Focused test loop uv run pytest -k "<pattern>" # Run all tests uv run pytestReferences
pyproject.tomltests/conftest.py
Files:
.agents/skills/python-tests/SKILL.md
**/tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
**/tests/**/*.py: Use Pytest for Fabric Python tests.
Do not add@pytest.mark.asyncioto tests; async tests are auto-detected and run by the async runner.
Do not add a-> Nonereturn type annotation to test functions.
When mocking a class, useunittest.mock.MagicMockorunittest.mock.AsyncMock(withspecwhen needed) instead of defining a new mock class.
Name mocked classes with themockprefix, notfake.
Prefer pytest fixtures over helper methods.
Do not repeat fixtures across test files; if a fixture is needed in multiple test files, place it in aconftest.pyfile.
When creating a fixture, use@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])withdef <fixture_name>_fixture() -> <return_type>:; only specifyscopewhen it is notfunction.
Preferpytest.mark.parametrizeover creating individual tests for different input types.
If a fixture is needed for a test but does not return a value, or its value is unused, use@pytest.mark.usefixtures.
If a test needs to modify environment variables, useos.environ;tests/conftest.pyprovides an autouserestore_environ_fixturethat restores the environment after each test, somonkeypatch.setenvis unnecessary.
Files:
tests/test_hermes_cli.pytests/test_hermes_sdk_adapter.pytests/smoke_cli.pytests/test_sdk_contract.pytests/smoke_relay_integration.pytests/test_adapaters_common_hermes.pytests/test_adapaters_common_utils.pytests/test_hermes_e2e.pytests/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/test_hermes_cli.pytests/test_hermes_sdk_adapter.pytests/smoke_cli.pytests/test_sdk_contract.pytests/smoke_relay_integration.pytests/test_adapaters_common_hermes.pytests/test_adapaters_common_utils.pytests/test_hermes_e2e.pytests/test_codex_cli.py
python/src/nemo_fabric/**/*
⚙️ CodeRabbit configuration file
python/src/nemo_fabric/**/*: Review Python SDK changes for typed API consistency, import-time dependency neutrality, async/session behavior, and parity with the native extension.
Stubs and runtime implementations should stay aligned.
Files:
python/src/nemo_fabric/types.py
crates/fabric-core/src/**/*.rs
⚙️ CodeRabbit configuration file
crates/fabric-core/src/**/*.rs: Review the Rust core for runtime lifecycle correctness, handle validation, capability routing accuracy, schema stability, and error semantics.
Public API changes should match committed schemas, tests, and documentation.
Files:
crates/fabric-core/src/lib.rscrates/fabric-core/src/runtime.rscrates/fabric-core/src/config.rs
schemas/**/*
⚙️ CodeRabbit configuration file
schemas/**/*: Schemas are generated public contract snapshots. Check that schema diffs correspond to intentional Rust type changes and are covered by core tests.
Files:
schemas/effective-config.schema.jsonschemas/agent.schema.jsonschemas/adapter-invocation.schema.jsonschemas/run-plan.schema.json
🧠 Learnings (2)
📚 Learning: 2026-06-28T04:03:32.877Z
Learnt from: AjayThorve
Repo: NVIDIA/NeMo-Fabric PR: 26
File: python/tests/smoke_typed_config.py:163-177
Timestamp: 2026-06-28T04:03:32.877Z
Learning: In NVIDIA NeMo Fabric Python SDK serialization of `RuntimeCapabilities` (to satisfy the “parity contract” with Rust core and the CLI), do not emit metadata keys when the corresponding metadata is absent. Instead, omit those fields entirely so the produced JSON matches the Rust/CLI output (e.g., avoid `null`, empty objects, or placeholder metadata). During review, verify the serializer/builders follow this omission rule and that Python outputs/parity tests reflect the same shape.
Applied to files:
python/src/nemo_fabric/types.py
📚 Learning: 2026-06-29T22:34:57.500Z
Learnt from: AjayThorve
Repo: NVIDIA/NeMo-Fabric PR: 27
File: tests/smoke_codex_cli.py:19-31
Timestamp: 2026-06-29T22:34:57.500Z
Learning: In NVIDIA/NeMo-Fabric, treat files in `tests/` named `smoke_*.py` as directly executed (gated) smoke scripts rather than pytest-collected tests. When reviewing or adding code to these files, keep them runnable as standalone scripts (e.g., via a script `main`/entrypoint) and avoid patterns that would make pytest collect them as normal test cases (e.g., defining pytest-style `test_*` functions/classes meant for collection). For Codex CLI adapter behavioral/pytest coverage, use the separate `tests/test_codex_cli.py` rather than adding pytest tests into `tests/smoke_*.py`.
Applied to files:
tests/smoke_cli.pytests/smoke_relay_integration.py
🪛 ast-grep (0.44.0)
tests/test_adapaters_common_utils.py
[info] 65-65: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"request": {"input": "from file"}})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
tests/test_codex_cli.py
[warning] 408-408: Do not make http calls without encryption
Context: f"http://{gateway_host}"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
adapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py
[warning] 309-309: Do not make http calls without encryption
Context: f"http://{relay_gateway_host}"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[error] 546-558: Command coming from incoming request
Context: subprocess.Popen(
[
relay_command,
"--config",
str(codex_settings.relay_config_path),
"--bind",
codex_settings.relay_gateway_host,
],
cwd=cwd,
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
start_new_session=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[warning] 515-515: 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)
[error] 618-630: Command coming from incoming request
Context: subprocess.run(
command,
cwd=cwd,
env=build_env(
payload,
relay_gateway_url=codex_settings.relay_gateway_url,
),
input=request_to_prompt(payload),
text=True,
capture_output=True,
check=False,
timeout=timeout,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 618-630: Use of unsanitized data to create processes
Context: subprocess.run(
command,
cwd=cwd,
env=build_env(
payload,
relay_gateway_url=codex_settings.relay_gateway_url,
),
input=request_to_prompt(payload),
text=True,
capture_output=True,
check=False,
timeout=timeout,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(os-system-unsanitized-data)
[info] 712-712: use jsonify instead of json.dumps for JSON output
Context: json.dumps(output, sort_keys=True)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 Ruff (0.15.20)
adapters/common/src/nemo_fabric_adapters/common/hermes.py
[warning] 44-44: Avoid specifying long messages outside the exception class
(TRY003)
adapters/common/src/nemo_fabric_adapters/common/utils.py
[warning] 356-356: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 372-372: Consider moving this statement to an else block
(TRY300)
[warning] 374-374: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_hermes_e2e.py
[warning] 61-61: Async functions should not use pathlib.Path methods, use trio.Path or anyio.path
(ASYNC240)
tests/test_codex_cli.py
[warning] 166-166: Unused function argument: tmp_path
(ARG001)
[warning] 180-180: Yoda condition detected
(SIM300)
[warning] 181-181: Yoda condition detected
(SIM300)
[warning] 183-183: Yoda condition detected
Rewrite as command[-3:-1] == ["--model", "gpt-5.4"]
(SIM300)
[warning] 297-297: Unused function argument: tmp_path
(ARG001)
[error] 526-526: Possible hardcoded password assigned to: "FABRIC_UNRELATED_SECRET"
(S105)
adapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py
[warning] 213-213: Prefer TypeError exception for invalid type
(TRY004)
[warning] 213-213: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 252-254: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 273-273: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 278-280: Prefer TypeError exception for invalid type
(TRY004)
[warning] 278-280: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 319-321: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 389-391: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 489-489: Prefer TypeError exception for invalid type
(TRY004)
[warning] 489-489: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 512-514: Avoid specifying long messages outside the exception class
(TRY003)
[error] 516-516: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
[warning] 522-522: Avoid specifying long messages outside the exception class
(TRY003)
[error] 547-547: subprocess call: check for execution of untrusted input
(S603)
[warning] 564-564: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 593-593: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 602-602: Avoid specifying long messages outside the exception class
(TRY003)
[error] 619-619: subprocess call: check for execution of untrusted input
(S603)
🔇 Additional comments (39)
.agents/skills/python-tests/SKILL.md (1)
28-28: LGTM!adapters/common/src/nemo_fabric_adapters/common/utils.py (3)
80-88: LGTM!
50-64: LGTM!
346-375: 🎯 Functional CorrectnessNo change needed: this helper only runs on Relay-enabled paths. Current call sites pass relay config data when Relay is enabled, so the
FABRIC_RELAY_CONFIG_PATHcheck is not gating a no-op path.> Likely an incorrect or invalid review comment.tests/test_adapaters_common_utils.py (2)
84-97: LGTM!
7-16: LGTM!adapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py (5)
217-262: 🎯 Functional Correctness | 💤 Low valueEmpty
oteltable still emits a profile.When telemetry is
native/enabled but the observability component has neitherdeployment.environmentnorendpoint, this returns{"otel": {}}.write_config_filesthen treatsconfigas truthy and writes a profile containing an empty[otel]table. Harmless if Codex ignores it, but worth confirming it doesn't trip Codex validation.
162-205: LGTM!
496-566: LGTM!
589-651: LGTM!
269-282: 🎯 Functional CorrectnessNo issue:
toml_value(value)is validation-only
toml_value()only checks TOML-serializability and finite numbers; it returns an encoded TOML literal, not a normalized value. Dropping the return is intentional becauseconfigis serialized later from the original values.> Likely an incorrect or invalid review comment.tests/test_codex_cli.py (4)
164-167: 📐 Maintainability & Code Quality | ⚡ Quick winUnused
tmp_pathparameter.
tmp_pathisn't referenced here (thecodex_payloadfixture already consumes it); Ruff flags ARG001. Drop it. Same applies at line 297 (test_native_otel_profile_writes_codex_telemetry_config).🧹 Proposed fix
def test_oneshot_command_uses_fabric_overrides_and_codex_owned_auth( codex_payload, - tmp_path, ):Source: Linters/SAST tools
331-393: LGTM!
442-473: LGTM!
503-551: LGTM!schemas/adapter-invocation.schema.json (1)
869-869: LGTM!Also applies to: 892-964
schemas/agent.schema.json (1)
362-362: LGTM!Also applies to: 385-408
schemas/effective-config.schema.json (1)
449-449: LGTM!Also applies to: 472-495
schemas/run-plan.schema.json (1)
945-945: LGTM!Also applies to: 968-972, 987-990, 1021-1040
adapters/hermes-sdk/src/nemo_fabric_adapters/hermes_sdk/adapter.py (1)
74-74: LGTM!Also applies to: 188-188
tests/test_hermes_cli.py (1)
41-67: LGTM!tests/test_hermes_sdk_adapter.py (1)
13-14: LGTM!Also applies to: 23-33
crates/fabric-core/src/config.rs (2)
634-677: LGTM!Also applies to: 1291-1307, 1513-1533
1635-1707: LGTM!Good coverage of the default-relay, native-skips-relay, and unknown-provider-rejection paths.
crates/fabric-core/src/lib.rs (1)
19-23: LGTM!crates/fabric-core/src/runtime.rs (2)
1291-1300: LGTM!
telemetry_provideris consistently threaded into both the invocation-side and result-side metadata maps, matching the Python SDK'smetadata.get("telemetry_provider", "relay")lookup.Also applies to: 1764-1773
2006-2066: LGTM!Solid end-to-end coverage: verifies relay is skipped, no
relay_configartifact is written, and bothtelemetry_plan.providerandruntime_context.telemetry.metadata.telemetry_providerreflect"native".python/src/nemo_fabric/types.py (1)
789-800: LGTM!
metadata.get("telemetry_provider", "relay")correctly preserves backward compatibility for existing relay-only payloads while honoring the new provider metadata from Rust core.tests/test_sdk_contract.py (1)
608-621: LGTM!adapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.py (1)
30-35: LGTM!Consolidating payload loading and session-id resolution onto the shared
common_utilshelpers, and validating the telemetry provider before building settings, matches the intended layering for this cohort.Also applies to: 55-65
adapters/common/src/nemo_fabric_adapters/common/hermes.py (2)
156-159: 🗄️ Data Integrity & IntegrationTuple order is correct.
write_relay_configs(...)returns(relay_config_path, plugin_config_path), so unpacking the second element intorelay_plugins_toml_pathsetsHERMES_NEMO_RELAY_PLUGINS_TOMLfrom the plugins TOML path.> Likely an incorrect or invalid review comment.
42-45: 🎯 Functional CorrectnessTelemetry provider is intentionally fixed to relay Hermes rejects any non-
relaytelemetry provider regardless oftelemetry.enabled;provider: nativeis already covered as invalid.> Likely an incorrect or invalid review comment.examples/code-review-agent/profiles/relay-openinference.yaml (1)
16-34: 📐 Maintainability & Code QualityDuplicates relay.yaml's atif/atof block verbatim.
Same concern as flagged on
relay.yaml— see that comment for details on the composability/duplication tradeoff.examples/code-review-agent/profiles/native-otel.yaml (1)
1-24: LGTM!examples/code-review-agent/profiles/relay-otel.yaml (1)
1-31: LGTM!tests/smoke_cli.py (1)
63-98: LGTM!Multi-profile expansion and assertions correctly track the new composable relay profile model. Based on learnings, this file correctly stays a standalone script (
def main()) rather than pytest-style tests, consistent with prior guidance forsmoke_*.pyfiles.Source: Learnings
tests/smoke_relay_integration.py (1)
37-53: LGTM!Switch to stacked
hermes_sdk+relayprofiles matches the new composable profile model.tests/test_hermes_e2e.py (1)
24-24: LGTM!
profile_namesmigration andrelay_artifact_rootderivation are consistent withrelay.yaml's fixed./artifacts/relayoutput_dir across all tested profile combinations.Also applies to: 52-65, 124-124, 145-147, 192-194, 211-241
examples/code-review-agent/profiles/relay.yaml (1)
13-30: 📐 Maintainability & Code QualityThe shared Relay artifact block is intentional
relay-openinference.yamlstill needs ATOF/ATIF output, andrelay-otel.yamlis an OTel-only profile.config.componentsarrays replace earlier values, so stacked profiles would not inherit Relay observability anyway.> Likely an incorrect or invalid review comment.
| monkeypatch.setenv( | ||
| "FABRIC_RELAY_CONFIG_PATH", str(tmp_path / "nested" / "relay.json") | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Same monkeypatch.setenv guideline violation.
As per coding guidelines, environment variable changes in tests should use os.environ directly rather than monkeypatch.setenv, since restore_environ_fixture in conftest.py already restores state.
🔧 Proposed fix
- monkeypatch.setenv(
- "FABRIC_RELAY_CONFIG_PATH", str(tmp_path / "nested" / "relay.json")
- )
+ os.environ["FABRIC_RELAY_CONFIG_PATH"] = str(tmp_path / "nested" / "relay.json")📝 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.
| monkeypatch.setenv( | |
| "FABRIC_RELAY_CONFIG_PATH", str(tmp_path / "nested" / "relay.json") | |
| ) | |
| os.environ["FABRIC_RELAY_CONFIG_PATH"] = str(tmp_path / "nested" / "relay.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 `@tests/test_adapaters_common_utils.py` around lines 232 - 234, The test is
violating the environment փոփոխation guideline by using monkeypatch.setenv in
the test setup. Update the fixture or test around the FABRIC_RELAY_CONFIG_PATH
assignment to write directly to os.environ instead of calling
monkeypatch.setenv, and keep the change localized in the same test helper block
so restore_environ_fixture can handle cleanup.
Source: Coding guidelines
…d-codex Signed-off-by: David Gardner <dagardner@nvidia.com>
…file Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
…ric into david-expand-codex-otel Signed-off-by: David Gardner <dagardner@nvidia.com>
…d-codex-otel Signed-off-by: David Gardner <dagardner@nvidia.com>
|
📖 Fern docs preview: https://nvidia-preview-pull-request-31.docs.buildwithfern.com/nemo/fabric |
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
adapters/common/src/nemo_fabric_adapters/common/utils.py (1)
138-160: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
runtime_context(payload)["runtime_id"]will raiseKeyErrorwhenruntime_idis absent.This file's own
runtime_session_id()helper (added in this same PR) exists precisely becauseruntime_contextmay contain onlysession_id, onlyruntime_id, or neither — its test coverage explicitly asserts it returnsNonewhen neither is present. Yetnormalize_relay_output_dirsindexesruntime_context(payload)["runtime_id"]directly, with no.get()fallback, so any payload lackingruntime_id(a state the adjacent helper is designed to tolerate) will crashload_relay_plugin_config()with an unhandledKeyErrorinstead of degrading gracefully.🐛 Proposed fix
- runtime_id = runtime_context(payload)["runtime_id"] + runtime_id = runtime_session_id(payload) or runtime_context(payload).get("runtime_id") + if not runtime_id: + raise RuntimeError("runtime_context.runtime_id is required to scope relay output directories")🤖 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 138 - 160, The `normalize_relay_output_dirs` helper currently reads `runtime_context(payload)["runtime_id"]` directly, which can raise `KeyError` for payloads that only have `session_id` or neither. Update this function to use the same tolerant lookup behavior as `runtime_session_id()` and fall back gracefully when `runtime_id` is missing, so `load_relay_plugin_config()` does not crash. Keep the change localized in `normalize_relay_output_dirs` and preserve the existing output directory creation logic.
🤖 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/test_harbor_runner.py`:
- Around line 4-10: The test file imports tomllib directly, which breaks Python
3.10 compatibility during collection. Update the import in the test helper to
use the same Python-version fallback pattern already used elsewhere in the repo,
with tomli as the 3.10-compatible fallback and tomllib only when available. Make
the change near the existing imports in the test module so the TOML parsing
logic continues to work across supported interpreters.
---
Outside diff comments:
In `@adapters/common/src/nemo_fabric_adapters/common/utils.py`:
- Around line 138-160: The `normalize_relay_output_dirs` helper currently reads
`runtime_context(payload)["runtime_id"]` directly, which can raise `KeyError`
for payloads that only have `session_id` or neither. Update this function to use
the same tolerant lookup behavior as `runtime_session_id()` and fall back
gracefully when `runtime_id` is missing, so `load_relay_plugin_config()` does
not crash. Keep the change localized in `normalize_relay_output_dirs` and
preserve the existing output directory creation logic.
🪄 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: 062bdf53-7327-47a7-b9f2-cfb4396d36e1
📒 Files selected for processing (5)
adapters/common/src/nemo_fabric_adapters/common/utils.pypython/src/nemo_fabric/types.pytests/test_adapaters_common_hermes.pytests/test_adapaters_common_utils.pytests/test_harbor_runner.py
📜 Review details
⚠️ CI failures not shown inline (2)
GitHub Actions: Fern Docs / Build and publish docs: Merge branch 'david-expand-codex' of github.com:dagardner-nv/nemo-fab…
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mpr_number="${GITHUB_REF_NAME#pull-request/}"�[0m
�[36;1mif output="$(fern generate --docs --preview --id "pull-request-${pr_number}" 2>&1)"; then�[0m
�[36;1m fern_exit=0�[0m
�[36;1melse�[0m
�[36;1m fern_exit=$?�[0m
�[36;1mfi�[0m
�[36;1mprintf '%s\n' "$output"�[0m
�[36;1mif [ "$fern_exit" -ne 0 ]; then�[0m
�[36;1m echo "::error::Fern docs preview generation failed"�[0m
GitHub Actions: Fern Docs / 0_Build and publish docs.txt: Merge branch 'david-expand-codex' of github.com:dagardner-nv/nemo-fab…
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mpr_number="${GITHUB_REF_NAME#pull-request/}"�[0m
�[36;1mif output="$(fern generate --docs --preview --id "pull-request-${pr_number}" 2>&1)"; then�[0m
�[36;1m fern_exit=0�[0m
�[36;1melse�[0m
�[36;1m fern_exit=$?�[0m
�[36;1mfi�[0m
�[36;1mprintf '%s\n' "$output"�[0m
�[36;1mif [ "$fern_exit" -ne 0 ]; then�[0m
�[36;1m echo "::error::Fern docs preview generation failed"�[0m
🧰 Additional context used
📓 Path-based instructions (4)
python/src/nemo_fabric/**/*
⚙️ CodeRabbit configuration file
python/src/nemo_fabric/**/*: Review Python SDK changes for typed API consistency, import-time dependency neutrality, async/session behavior, and parity with the native extension.
Stubs and runtime implementations should stay aligned.
Files:
python/src/nemo_fabric/types.py
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Use Pytest to run Python tests; this style guide applies to Fabric test files.
Do not add@pytest.mark.asyncioto test functions; async tests are auto-detected by the async runner.
Do not add a-> Nonereturn type annotation to test functions.
When mocking a class, do not define a new class; useunittest.mock.MagicMockorunittest.mock.AsyncMock, addingspecwhen needed.
Name mocked classes with themockprefix rather thanfake.
Prefer pytest fixtures over helper methods.
Do not repeat fixtures; if a fixture is needed in multiple test files, move it toconftest.py.
When creating a fixture, use@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])on a function named<fixture_name>_fixture; only specifyscopewhen it is notfunction.
Preferpytest.mark.parametrizeover creating separate tests for different input types.
If a fixture is needed for a test but does not return a value or its value is unused, use@pytest.mark.usefixtures.
If you need to modify environment variables in a test, useos.environ;restore_environ_fixturewill reset them after the test, so do not usemonkeypatch.setenvfor that purpose.
Files:
tests/test_harbor_runner.pytests/test_adapaters_common_utils.pytests/test_adapaters_common_hermes.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/test_harbor_runner.pytests/test_adapaters_common_utils.pytests/test_adapaters_common_hermes.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.py
🧠 Learnings (1)
📚 Learning: 2026-06-28T04:03:32.877Z
Learnt from: AjayThorve
Repo: NVIDIA/NeMo-Fabric PR: 26
File: python/tests/smoke_typed_config.py:163-177
Timestamp: 2026-06-28T04:03:32.877Z
Learning: In NVIDIA NeMo Fabric Python SDK serialization of `RuntimeCapabilities` (to satisfy the “parity contract” with Rust core and the CLI), do not emit metadata keys when the corresponding metadata is absent. Instead, omit those fields entirely so the produced JSON matches the Rust/CLI output (e.g., avoid `null`, empty objects, or placeholder metadata). During review, verify the serializer/builders follow this omission rule and that Python outputs/parity tests reflect the same shape.
Applied to files:
python/src/nemo_fabric/types.py
🪛 ast-grep (0.44.0)
tests/test_harbor_runner.py
[warning] 245-245: Do not make http calls without encryption
Context: "http://host.docker.internal:6006/v1/traces"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[info] 99-99: use jsonify instead of json.dumps for JSON output
Context: json.dumps(config.to_mapping())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 Ruff (0.15.20)
tests/test_harbor_runner.py
[warning] 50-50: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
🔇 Additional comments (6)
tests/test_harbor_runner.py (1)
56-263: LGTM!tests/test_adapaters_common_utils.py (1)
133-235: LGTM!tests/test_adapaters_common_hermes.py (1)
197-295: LGTM!Also applies to: 396-464
adapters/common/src/nemo_fabric_adapters/common/utils.py (1)
111-135: LGTM!Also applies to: 328-346
python/src/nemo_fabric/types.py (2)
1000-1016: 🗄️ Data Integrity & IntegrationConfirm
telemetry_providermatches the config key This normalizer readsmetadata.get("telemetry_provider", "relay"), so the native/runtime layer must populatemetadata["telemetry_provider"]for therelay_enabledpath. If it usesproviderinstead,telemetry.providerwill be ignored and the code will fall back to"relay".
123-136: 🎯 Functional CorrectnessNo action needed —
_plain()already recursively copies mappings/lists and deep-copies scalar leaves, soextra_fieldsandto_mapping()both return detached JSON-compatible data.> Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 (1)
adapters/common/src/nemo_fabric_adapters/common/utils.py (1)
138-160: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
runtime_context(payload)["runtime_id"]will raiseKeyErrorwhenruntime_idis absent.This file's own
runtime_session_id()helper (added in this same PR) exists precisely becauseruntime_contextmay contain onlysession_id, onlyruntime_id, or neither — its test coverage explicitly asserts it returnsNonewhen neither is present. Yetnormalize_relay_output_dirsindexesruntime_context(payload)["runtime_id"]directly, with no.get()fallback, so any payload lackingruntime_id(a state the adjacent helper is designed to tolerate) will crashload_relay_plugin_config()with an unhandledKeyErrorinstead of degrading gracefully.🐛 Proposed fix
- runtime_id = runtime_context(payload)["runtime_id"] + runtime_id = runtime_session_id(payload) or runtime_context(payload).get("runtime_id") + if not runtime_id: + raise RuntimeError("runtime_context.runtime_id is required to scope relay output directories")🤖 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 138 - 160, The `normalize_relay_output_dirs` helper currently reads `runtime_context(payload)["runtime_id"]` directly, which can raise `KeyError` for payloads that only have `session_id` or neither. Update this function to use the same tolerant lookup behavior as `runtime_session_id()` and fall back gracefully when `runtime_id` is missing, so `load_relay_plugin_config()` does not crash. Keep the change localized in `normalize_relay_output_dirs` and preserve the existing output directory creation logic.
🤖 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/test_harbor_runner.py`:
- Around line 4-10: The test file imports tomllib directly, which breaks Python
3.10 compatibility during collection. Update the import in the test helper to
use the same Python-version fallback pattern already used elsewhere in the repo,
with tomli as the 3.10-compatible fallback and tomllib only when available. Make
the change near the existing imports in the test module so the TOML parsing
logic continues to work across supported interpreters.
---
Outside diff comments:
In `@adapters/common/src/nemo_fabric_adapters/common/utils.py`:
- Around line 138-160: The `normalize_relay_output_dirs` helper currently reads
`runtime_context(payload)["runtime_id"]` directly, which can raise `KeyError`
for payloads that only have `session_id` or neither. Update this function to use
the same tolerant lookup behavior as `runtime_session_id()` and fall back
gracefully when `runtime_id` is missing, so `load_relay_plugin_config()` does
not crash. Keep the change localized in `normalize_relay_output_dirs` and
preserve the existing output directory creation logic.
🪄 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: 062bdf53-7327-47a7-b9f2-cfb4396d36e1
📒 Files selected for processing (5)
adapters/common/src/nemo_fabric_adapters/common/utils.pypython/src/nemo_fabric/types.pytests/test_adapaters_common_hermes.pytests/test_adapaters_common_utils.pytests/test_harbor_runner.py
📜 Review details
🔇 Additional comments (6)
tests/test_harbor_runner.py (1)
56-263: LGTM!tests/test_adapaters_common_utils.py (1)
133-235: LGTM!tests/test_adapaters_common_hermes.py (1)
197-295: LGTM!Also applies to: 396-464
adapters/common/src/nemo_fabric_adapters/common/utils.py (1)
111-135: LGTM!Also applies to: 328-346
python/src/nemo_fabric/types.py (2)
1000-1016: 🗄️ Data Integrity & IntegrationConfirm
telemetry_providermatches the config key This normalizer readsmetadata.get("telemetry_provider", "relay"), so the native/runtime layer must populatemetadata["telemetry_provider"]for therelay_enabledpath. If it usesproviderinstead,telemetry.providerwill be ignored and the code will fall back to"relay".
123-136: 🎯 Functional CorrectnessNo action needed —
_plain()already recursively copies mappings/lists and deep-copies scalar leaves, soextra_fieldsandto_mapping()both return detached JSON-compatible data.> Likely an incorrect or invalid review comment.
🛑 Comments failed to post (1)
tests/test_harbor_runner.py (1)
4-10: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Check declared Python version support and existing tomllib fallback patterns fd pyproject.toml | xargs -I{} cat {} rg -n 'requires-python' rg -n 'import tomllib' -B2 -A2 rg -n 'import tomli' -B2 -A2Repository: NVIDIA/NeMo-Fabric
Length of output: 2123
Use a Python 3.10-compatible TOML import here.
tomllibis only in Python 3.11+, but this repo supports Python 3.10 and already includestomlifor older interpreters. Importingtomllibdirectly will break test collection on 3.10; use the same fallback pattern as the other test helper.🤖 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/test_harbor_runner.py` around lines 4 - 10, The test file imports tomllib directly, which breaks Python 3.10 compatibility during collection. Update the import in the test helper to use the same Python-version fallback pattern already used elsewhere in the repo, with tomli as the 3.10-compatible fallback and tomllib only when available. Make the change near the existing imports in the test module so the TOML parsing logic continues to work across supported interpreters.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_harbor_runner.py`:
- Line 167: The test mutates CODEX_HOME directly via os.environ, which can leak
state into later tests. Update the test in test_harbor_runner to use the
monkeypatch fixture and call monkeypatch.setenv("CODEX_HOME", str(tmp_path))
instead of assigning to os.environ, so the environment is automatically restored
after the test.
🪄 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: 46bcbec7-8f55-4993-a967-89822d599523
📒 Files selected for processing (1)
tests/test_harbor_runner.py
📜 Review details
⚠️ CI failures not shown inline (4)
GitHub Actions: Rust / Test: Add support for native Codex telemetry
Conclusion: failure
##[group]Run cargo test --workspace --locked
�[36;1mcargo test --workspace --locked�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
CACHE_ON_FAILURE: false
##[endgroup]
�[1m�[92m Compiling�[0m fabric-core v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-core)
�[1m�[92m Compiling�[0m fabric-cli v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-cli)
�[1m�[92m Finished�[0m `test` profile [unoptimized] target(s) in 5.75s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-c3572a4d481471cf)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
�[1m�[92m Running�[0m unittests src/lib.rs (target/debug/deps/fabric_core-9b081566c1d472c1)
running 45 tests
test config::tests::errors_for_unknown_manifest_profile ... ok
GitHub Actions: Rust / 0_Test.txt: Add support for native Codex telemetry
Conclusion: failure
##[group]Run cargo test --workspace --locked
�[36;1mcargo test --workspace --locked�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
CACHE_ON_FAILURE: false
##[endgroup]
�[1m�[92m Compiling�[0m fabric-core v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-core)
�[1m�[92m Compiling�[0m fabric-cli v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-cli)
�[1m�[92m Finished�[0m `test` profile [unoptimized] target(s) in 5.75s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-c3572a4d481471cf)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
�[1m�[92m Running�[0m unittests src/lib.rs (target/debug/deps/fabric_core-9b081566c1d472c1)
running 45 tests
test config::tests::errors_for_unknown_manifest_profile ... ok
GitHub Actions: Fern Docs / Build and publish docs: Update test
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mpr_number="${GITHUB_REF_NAME#pull-request/}"�[0m
�[36;1mif output="$(fern generate --docs --preview --id "pull-request-${pr_number}" 2>&1)"; then�[0m
�[36;1m fern_exit=0�[0m
�[36;1melse�[0m
�[36;1m fern_exit=$?�[0m
�[36;1mfi�[0m
�[36;1mprintf '%s\n' "$output"�[0m
�[36;1mif [ "$fern_exit" -ne 0 ]; then�[0m
�[36;1m echo "::error::Fern docs preview generation failed"�[0m
GitHub Actions: Fern Docs / 0_Build and publish docs.txt: Update test
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mpr_number="${GITHUB_REF_NAME#pull-request/}"�[0m
�[36;1mif output="$(fern generate --docs --preview --id "pull-request-${pr_number}" 2>&1)"; then�[0m
�[36;1m fern_exit=0�[0m
�[36;1melse�[0m
�[36;1m fern_exit=$?�[0m
�[36;1mfi�[0m
�[36;1mprintf '%s\n' "$output"�[0m
�[36;1mif [ "$fern_exit" -ne 0 ]; then�[0m
�[36;1m echo "::error::Fern docs preview generation failed"�[0m
🧰 Additional context used
📓 Path-based instructions (2)
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Usepytestto run tests.
Do not add@pytest.mark.asyncioto test functions; async tests are detected and run automatically by the async runner.
Do not add a-> Nonereturn type annotation to test functions.
When mocking a class, useunittest.mock.MagicMockorunittest.mock.AsyncMock(withspecwhen needed) instead of defining a new class.
Name mocked classes with amockprefix, notfake.
Prefer pytest fixtures over helper methods.
Do not repeat fixtures across test files; if a fixture is needed in multiple test files, place it inconftest.py.
Define fixtures with@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])anddef <fixture_name>_fixture() -> <return_type>:; only specifyscopewhen it is notPreferpytest.mark.parametrizeover creating individual tests for different input types. If a fixture is needed for a test but does not return a value or the value is unused, use@pytest.mark.usefixtures`.
Files:
tests/test_harbor_runner.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/test_harbor_runner.py
🔇 Additional comments (2)
tests/test_harbor_runner.py (2)
6-6: LGTM!Also applies to: 175-193
168-174: 🎯 Functional CorrectnessProfile name matches helper
get_codex_profile_path()already buildsfabric-{runtime_id}, sofabric-harbor-testis the expected profile name here.> Likely an incorrect or invalid review comment.
AjayThorve
left a comment
AjayThorve
left a comment
There was a problem hiding this comment.
looks good, the provider option for telemetry makes sense!
Signed-off-by: David Gardner <dagardner@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_codex_cli.py`:
- Around line 42-48: The codex_payload_fixture currently mutates CODEX_HOME
directly and leaves it set for the rest of the test session. Update the fixture
to use pytest’s monkeypatch (or an equivalent yield-based teardown) so
CODEX_HOME is automatically restored after each test, and keep the setup for the
workspace/codex-home plus team.toml creation inside codex_payload_fixture. This
will preserve the unset-environment behavior needed by the default-path tests
and prevent cross-test leakage.
🪄 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: 89103c14-d589-40c0-b4b9-d520af00db93
📒 Files selected for processing (3)
adapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.pypyproject.tomltests/test_codex_cli.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Test
- GitHub Check: Build and publish docs
⚠️ CI failures not shown inline (2)
GitHub Actions: Rust / Test: Add support for native Codex telemetry
Conclusion: failure
##[group]Run cargo test --workspace --locked
�[36;1mcargo test --workspace --locked�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
CACHE_ON_FAILURE: false
##[endgroup]
�[1m�[92m Compiling�[0m fabric-core v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-core)
�[1m�[92m Compiling�[0m fabric-cli v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-cli)
�[1m�[92m Finished�[0m `test` profile [unoptimized] target(s) in 5.86s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-c3572a4d481471cf)
running 0 tests
�[1m�[92m Running�[0m unittests src/lib.rs (target/debug/deps/fabric_core-9b081566c1d472c1)
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
running 45 tests
test config::tests::errors_for_unknown_manifest_profile ... ok
GitHub Actions: Rust / 0_Test.txt: Add support for native Codex telemetry
Conclusion: failure
##[group]Run cargo test --workspace --locked
�[36;1mcargo test --workspace --locked�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
CACHE_ON_FAILURE: false
##[endgroup]
�[1m�[92m Compiling�[0m fabric-core v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-core)
�[1m�[92m Compiling�[0m fabric-cli v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-cli)
�[1m�[92m Finished�[0m `test` profile [unoptimized] target(s) in 5.86s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-c3572a4d481471cf)
running 0 tests
�[1m�[92m Running�[0m unittests src/lib.rs (target/debug/deps/fabric_core-9b081566c1d472c1)
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
running 45 tests
test config::tests::errors_for_unknown_manifest_profile ... ok
🧰 Additional context used
📓 Path-based instructions (3)
{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/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Usepytestto run tests.
Do not add@pytest.mark.asyncioto test functions; async tests are detected and run automatically by the async runner.
Do not add a-> Nonereturn type annotation to test functions.
When mocking a class, useunittest.mock.MagicMockorunittest.mock.AsyncMock(withspecwhen needed) instead of defining a new class.
Name mocked classes with amockprefix, notfake.
Prefer pytest fixtures over helper methods.
Do not repeat fixtures across test files; if a fixture is needed in multiple test files, place it inconftest.py.
Define fixtures with@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])anddef <fixture_name>_fixture() -> <return_type>:; only specifyscopewhen it is notPreferpytest.mark.parametrizeover creating individual tests for different input types. If a fixture is needed for a test but does not return a value or the value is unused, use@pytest.mark.usefixtures`.
Files:
tests/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/test_codex_cli.py
🧬 Code graph analysis (1)
adapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py (1)
tests/test_codex_cli.py (2)
get_codex_profile_path(254-254)write_config_files(173-173)
🔇 Additional comments (10)
tests/test_codex_cli.py (1)
196-231: LGTM!pyproject.toml (1)
16-19: LGTM!adapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py (8)
290-313: LGTM!
315-408: LGTM!
452-486: LGTM!
504-518: LGTM!
527-558: LGTM!
594-605: LGTM!
25-29: 🩺 Stability & Availabilitytomli is already declared for Python < 3.11
pyproject.tomlincludestomli>=2; python_version < '3.11'in thecodexextras, so the fallback import is covered.> Likely an incorrect or invalid review comment.
411-419: 🎯 Functional CorrectnessCodex profile path matches
--profile.--profile fabric-{runtime_id}resolves to$CODEX_HOME/fabric-{runtime_id}.config.toml, so the generated file name is correct.> Likely an incorrect or invalid review comment.
2 participants
telemetry.providerconfiguration attribute, for compatibility when unset the value default's to "relay"build_commandnow only builds the command, writing out configuration files is moved towrite_config_filesCodexSettingsadded as a container for the various parameters passed aroundSummary by CodeRabbit