Add support for an ADAPTER_PYTHON environment variable as a shortcut for setting python_env in the config - #45
Conversation
Signed-off-by: David Gardner <dagardner@nvidia.com>
…_PYTHON Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRenames the adapter Python interpreter env var to ChangesADAPTER_PYTHON runtime and preflight
Estimated code review effort: 3 (Moderate) | ~25 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
📖 Fern docs preview: https://nvidia-preview-pull-request-45.docs.buildwithfern.com/nemo/fabric |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@README.md`:
- Line 103: The README has inconsistent Markdown formatting for the
ADAPTER_PYTHON identifier, using double backticks in the relevant sentence while
nearby references use single backticks. Update the documentation text so the
ADAPTER_PYTHON reference matches the surrounding style, and keep the same
identifier formatting consistent wherever it appears in this section.
- Line 108: The README sentence in the adapter guidance contains a comma splice
before “however”; update the text in the ADAPTER_PYTHON guidance so the two
independent clauses are separated correctly, preferably by splitting into two
sentences rather than using a semicolon. Keep the meaning the same while fixing
the punctuation around the clause describing the adapter package and the note
that adapters are small and self-contained.
In `@tests/python/test_code_review_example.py`:
- Around line 121-145: Add a test for the `main_module.main` fallback path where
the Fabric result has no `"response"` in `output.output`, so the `else:
print(output.error.message)` branch is exercised. Use the existing
`test_example_entrypoint_shows_response_after_normalized_output` as the
reference point and create a parametrized or separate case that mocks
`Fabric.run` to return a result whose normalized output lacks `"response"` and
provides an `error` object with a `message`, then assert the printed fallback
message and JSON output behavior.
🪄 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: 661f7789-b595-4006-a10a-8387e72cc7b9
📒 Files selected for processing (12)
README.mdcrates/fabric-core/src/error.rscrates/fabric-core/src/runtime.rsdocs/reference/api/rust-library-reference/fabric-core/error/enum-fabricerror.mdxexamples/code_review_agent/README.mdexamples/code_review_agent/__main__.pyexamples/code_review_agent/config.pytests/e2e/test_hermes_e2e.pytests/e2e/test_hermes_runtime.pytests/fixtures/file-config-agent/profiles/hermes-sdk.yamltests/python/test_code_review_example.pytests/python/test_native_sdk.py
💤 Files with no reviewable changes (2)
- examples/code_review_agent/config.py
- tests/fixtures/file-config-agent/profiles/hermes-sdk.yaml
📜 Review details
🧰 Additional context used
📓 Path-based instructions (18)
{README.md,docs/index.yml,docs/**/*.{md,mdx}}
📄 CodeRabbit inference engine (.agents/skills/contribute-docs/SKILL.md)
Update entry-point docs when examples or reading paths change
Files:
README.mddocs/reference/api/rust-library-reference/fabric-core/error/enum-fabricerror.mdx
{README.md,docs/index.yml}
📄 CodeRabbit inference engine (.agents/skills/contribute-docs/SKILL.md)
Update
README.mdordocs/index.ymlwhen entry points changed
Files:
README.md
**/README.md
📄 CodeRabbit inference engine (.agents/skills/contribute-docs/SKILL.md)
Update relevant adapter or example
README.mdfiles when examples or adapters have changed
Files:
README.mdexamples/code_review_agent/README.md
**/*.{md,mdx}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-brand-terminology.md)
**/*.{md,mdx}: SpellNVIDIAin all caps; do not useNvidia,nvidia,nVidia,nVIDIA, orNV.
Usean NVIDIAbefore a noun becauseNVIDIAstarts with an "en" sound.
Do not add a registered trademark symbol afterNVIDIAwhen referring to the company.
Use trademark symbols with product names only when the document type or legal guidance requires them.
Verify official capitalization, spacing, and hyphenation for NVIDIA product names.
Precede NVIDIA product names withNVIDIAon first mention when it is natural and accurate.
Link the first mention of a product name when the destination helps the reader.
Do not rewrite product names for grammar or title-case rules.
Preserve third-party product names according to the owner's spelling.
Include the company name and full model qualifier on first use when it helps identify the model.
Preserve the official capitalization and punctuation of model names.
Use shorter family names only after the full model name is established.
For learning-oriented docs, technical blog posts, GTC sessions, tutorials, and developer guides: do not force trademark symbols unless the source, platform, or legal guidance explicitly requires them.
For learning-oriented docs, technical blog posts, GTC sessions, tutorials, and developer guides: keep the product name accurate and consistent.
For press releases, product landing pages, packaging, sales content, or legal copy: follow the current NVIDIA trademark and copyright guidance.
For press releases, product landing pages, packaging, sales content, or legal copy: attribute trademarks on first use when required.
For press releases, product landing pages, packaging, sales content, or legal copy: use the required trademark symbol for the specific product or service.
For press releases, product landing pages, packaging, sales content, or legal copy: do not invent trademark attributions; verify current legal copy.
If the platform requires legal copy, confirm the current source of truth instead...
Files:
README.mddocs/reference/api/rust-library-reference/fabric-core/error/enum-fabricerror.mdxexamples/code_review_agent/README.md
**/*.{md,mdx,rst}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-technical-docs.md)
**/*.{md,mdx,rst}: When reviewing technical documentation, verify that commands, examples, paths, APIs, and support claims match the current repository.
Make technical documents easy to scan by fixing headings, lead-in sentences, lists, tables, and procedure shape.
Preserve exact code, command, API, package, and UI strings unless they are factually wrong.
Prefer focused findings over broad rewrites.
Use title case consistently in technical documentation headings.
Avoid quotation marks, ampersands, and exclamation marks in technical documentation headings.
Keep product, event, research, and whitepaper names in their official title case.
Use title case for table headers.
Do not force social-media sentence case into technical documentation.
Format code elements, commands, parameters, package names, and expressions in monospace.
Format directories, file names, and paths in monospace.
Use angle brackets inside monospace for variables inside paths.
Format error messages and strings with quotation marks, using code formatting when that is clearer.
Format UI buttons, menus, fields, and labels in bold.
Use angle brackets between UI labels for menu paths.
Use italics on first use for new terms, and only when the term is introduced.
Italicize publication titles.
Write keyboard shortcuts in plain text.
Use Owner/repo link text for GitHub repositories.
Introduce every code block with a complete sentence.
Do not make a code block complete the grammar of the previous sentence.
Do not continue a sentence after a code block.
Use syntax highlighting when the format supports it.
Avoid the word "snippet" unless the surrounding documentation already uses it as a term of art.
Keep inline method, function, and class references consistent with nearby docs, and omit empty parentheses in prose when no call is shown.
Use descriptive anchor text that matches the destination title when possible.
Avoid raw URLs in running text.
Avoid generic anchors such as "here," "this page," and "read more....
Files:
README.mddocs/reference/api/rust-library-reference/fabric-core/error/enum-fabricerror.mdxexamples/code_review_agent/README.md
**/*.{md,mdx,rst,txt}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*.{md,mdx,rst,txt}: If documentation or examples changed, runjust docswhen practical and verify documented commands against the current repository.
For documentation-only changes, usecontribute-docsandreview-doc-style; runjust docsfor docs-site or generated-reference changes.
Files:
README.mddocs/reference/api/rust-library-reference/fabric-core/error/enum-fabricerror.mdxexamples/code_review_agent/README.md
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
For CI or packaging changes, use
maintain-ciormaintain-packaging, then run the recipes and checks whose behavior changed.
Files:
README.mdexamples/code_review_agent/__main__.pytests/e2e/test_hermes_runtime.pytests/e2e/test_hermes_e2e.pycrates/fabric-core/src/error.rsdocs/reference/api/rust-library-reference/fabric-core/error/enum-fabricerror.mdxexamples/code_review_agent/README.mdtests/python/test_code_review_example.pytests/python/test_native_sdk.pycrates/fabric-core/src/runtime.rs
{docs/**,README.md,AGENTS.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,AGENTS.md}: Review documentation for technical accuracy against the current API, command correctness, and consistency with generated schemas.
Files:
README.mddocs/reference/api/rust-library-reference/fabric-core/error/enum-fabricerror.mdx
**/*.{py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*.{py,pyi}: If Python code or a Python-facing adapter changed, runjust test-python.
For Python SDK or PyO3 binding changes, usepython-tests, run focused pytest tests first, then runjust test-python; rebuild withjust build-pythonwhen native code or packaging changed.
Files:
examples/code_review_agent/__main__.pytests/e2e/test_hermes_runtime.pytests/e2e/test_hermes_e2e.pytests/python/test_code_review_example.pytests/python/test_native_sdk.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:
examples/code_review_agent/__main__.pyexamples/code_review_agent/README.md
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Usepytestto run Python tests.
Do not add@pytest.mark.asyncioto test functions; async tests should be auto-detected and run 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, usingspecwhen necessary.
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.
When creating a fixture, use the pattern@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])followed bydef <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 you need to modify environment variables in a test, useos.environ;tests/conftest.pyprovides an autouserestore_environ_fixturethat restores the environment after each test, somonkeypatch.setenvis unnecessary.
Avoid defensive programming in tests; access expected dictionary keys directly (for example,results["data"]) instead of using.get(), so failures raise loudly and clearly.
Files:
tests/e2e/test_hermes_runtime.pytests/e2e/test_hermes_e2e.pytests/python/test_code_review_example.pytests/python/test_native_sdk.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/e2e/test_hermes_runtime.pytests/e2e/test_hermes_e2e.pytests/python/test_code_review_example.pytests/python/test_native_sdk.py
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*.rs: If Rust code changed, runcargo fmt --all -- --checkandjust test-rust.
If public configuration types changed, confirm the schema snapshot tests injust test-rustpass and review generated schema diffs.
For Rust core, CLI, or shared runtime semantics changes, run Rust formatting and tests; add Python tests when the behavior is exposed through the SDK, and run relevant tests for CLI behavior.
Files:
crates/fabric-core/src/error.rscrates/fabric-core/src/runtime.rs
crates/fabric-core/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If
crates/fabric-corechanged in a way exposed through Python, run both the Rust and Python suites.
Files:
crates/fabric-core/src/error.rscrates/fabric-core/src/runtime.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If the PyO3 bridge or package metadata changed, run
just build-pythonandcargo check -p fabric-python --locked.
Files:
crates/fabric-core/src/error.rscrates/fabric-core/src/runtime.rs
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/error.rscrates/fabric-core/src/runtime.rs
docs/**/*.{md,mdx}
📄 CodeRabbit inference engine (.agents/skills/contribute-docs/SKILL.md)
docs/**/*.{md,mdx}: Prefer the documented public API, not internal shortcuts
Keep package names, repository references, and build commands current
Update relevant getting-started or reference docs
Ensure example commands still match current package names and paths
Files:
docs/reference/api/rust-library-reference/fabric-core/error/enum-fabricerror.mdx
**/*.mdx
📄 CodeRabbit inference engine (.agents/skills/contribute-docs/SKILL.md)
In MDX files, top-of-file comments must use JSX comment delimiters (
{/* ... */}); do not use HTML comments for MDX SPDX headersUse
{/* ... */}delimiters for top-of-file SPDX comments in MDX files; do not use HTML comment delimiters.
Files:
docs/reference/api/rust-library-reference/fabric-core/error/enum-fabricerror.mdx
🧬 Code graph analysis (2)
tests/python/test_native_sdk.py (1)
crates/fabric-core/src/runtime.rs (1)
start_runtime(412-422)
crates/fabric-core/src/runtime.rs (1)
crates/fabric-core/src/config.rs (1)
RunPlan(1321-1353)
🪛 Clippy (1.96.0)
crates/fabric-core/src/runtime.rs
[warning] 1339-1339: this if statement can be collapsed
(warning)
🔇 Additional comments (14)
examples/code_review_agent/__main__.py (2)
39-43: LGTM!
57-61: 🩺 Stability & AvailabilityNo change needed here. The supported code-review variants always populate
output.responseon success, andRunResult.erroris reserved for failures, so the fallback won’t dereferenceNonein normal use.> Likely an incorrect or invalid review comment.examples/code_review_agent/README.md (1)
25-25: LGTM!Also applies to: 61-63, 78-79
tests/python/test_code_review_example.py (1)
9-11: LGTM!README.md (1)
97-109: LGTM!tests/e2e/test_hermes_e2e.py (1)
47-47: LGTM!tests/e2e/test_hermes_runtime.py (1)
38-40: LGTM!tests/python/test_native_sdk.py (2)
8-15: LGTM!
24-51: Solid coverage for ADAPTER_PYTHON selection and preflight rejection.Both success and failure paths are exercised; env mutation relies on the repo's autouse
restore_environ_fixturefor cleanup, consistent with the tests path instructions.crates/fabric-core/src/error.rs (1)
150-159: LGTM!docs/reference/api/rust-library-reference/fabric-core/error/enum-fabricerror.mdx (1)
12-12: LGTM!Also applies to: 274-289
crates/fabric-core/src/runtime.rs (3)
7-7: LGTM!Also applies to: 28-28, 590-595, 651-651
939-939: 🩺 Stability & AvailabilityPreflight validation isn't reused by
invoke().
start()validates the resolvedADAPTER_PYTHONinterpreter once viapreflight_python_adapter, butrun_python_adapter(called on everyinvoke) independently re-resolves the interpreter viaresolve_python_command(...).pathand never callsvalidate_python_commandagain. If the runtime handle is invoked multiple times and the environment/interpreter changes between calls (e.g., a venv is rebuilt or removed), the second invocation bypasses preflight entirely and only fails with a genericProcessRunnerspawn error instead ofInvalidAdapterPython.Is a single preflight check at
start()intended to be sufficient for the lifetime of the runtime handle, or should the validatedPythonCommandbe cached and reused byinvoke()to avoid silent revalidation gaps?Also applies to: 1332-1386
2566-2652: LGTM!
Signed-off-by: David Gardner <dagardner@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 491-510: The env construction in the codex CLI adapter is
bypassing the allowlist when a virtualenv is active because
virtualenv_subprocess_env() returns a full environment copy. Update the env
merge in the adapter logic that builds the subprocess environment so it only
applies the virtualenv-specific overrides needed for compatibility, while still
preserving the INHERITED_ENV_NAMES restriction and the configured payload env
handling.
In `@adapters/common/src/nemo_fabric_adapters/common/utils.py`:
- Around line 33-49: virtualenv_subprocess_env() currently mismatches its
expected contract: it returns {} when no virtualenv is active, and it returns a
full os.environ copy when one is active, which breaks the test and leaks
non-allowlisted variables downstream. Update
adapters/common/src/nemo_fabric_adapters/common/utils.py so
virtualenv_subprocess_env layers only the virtualenv-specific changes onto a
caller-provided base environment instead of always copying os.environ, and make
the no-virtualenv case preserve the incoming environment. Then adjust
build_env() to pass its allowlisted env into
virtualenv_subprocess_env(base_env=env) so the INHERITED_ENV_NAMES boundary is
preserved.
In `@tests/adapters/test_adapaters_common_utils.py`:
- Around line 61-70: The test is asserting the old behavior of
virtualenv_subprocess_env when current_virtualenv() is None, but the API now
expects an optional base_env to be preserved unchanged in that case. Update
test_virtualenv_subprocess_env_preserves_environment_outside_virtualenv in
test_adapaters_common_utils.py to pass a base environment into
common_utils.virtualenv_subprocess_env and assert that the returned env matches
that base_env and is a distinct object, rather than comparing against
os.environ.
🪄 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: bb2c385f-9ac5-4669-9680-fa63cffd3555
📒 Files selected for processing (4)
adapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.pyadapters/common/src/nemo_fabric_adapters/common/utils.pyadapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.pytests/adapters/test_adapaters_common_utils.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Build and publish docs
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*.{py,pyi}: If Python code or a Python-facing adapter changed, runjust test-python.
For Python SDK or PyO3 binding changes, usepython-tests, run focused pytest tests first, then runjust test-python; rebuild withjust build-pythonwhen native code or packaging changed.
Files:
adapters/common/src/nemo_fabric_adapters/common/utils.pyadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.pyadapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.pytests/adapters/test_adapaters_common_utils.py
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
For CI or packaging changes, use
maintain-ciormaintain-packaging, then run the recipes and checks whose behavior changed.
Files:
adapters/common/src/nemo_fabric_adapters/common/utils.pyadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.pyadapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.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/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.pyadapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.py
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Usepytestto run Python tests.
Do not add@pytest.mark.asyncioto test functions; async tests should be auto-detected and run 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, usingspecwhen necessary.
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.
When creating a fixture, use the pattern@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])followed bydef <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 you need to modify environment variables in a test, useos.environ;tests/conftest.pyprovides an autouserestore_environ_fixturethat restores the environment after each test, somonkeypatch.setenvis unnecessary.
Avoid defensive programming in tests; access expected dictionary keys directly (for example,results["data"]) instead of using.get(), so failures raise loudly and clearly.
Files:
tests/adapters/test_adapaters_common_utils.py
tests/adapters/**/*.py
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
tests/adapters/**/*.py: If an adapter or integration changed, run its focused tests.
For adapter behavior changes, run the focused adapter tests undertests/adapters, thenjust test-python.
Files:
tests/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_adapaters_common_utils.py
🧬 Code graph analysis (1)
adapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py (1)
adapters/common/src/nemo_fabric_adapters/common/utils.py (1)
settings_payload(101-103)
🪛 Ruff (0.15.20)
adapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py
[warning] 504-504: Prefer TypeError exception for invalid type
(TRY004)
[warning] 504-504: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (4)
adapters/common/src/nemo_fabric_adapters/common/utils.py (1)
25-30: LGTM!adapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.py (1)
182-200: LGTM!
build_envalready starts from a fullos.environ.copy()(no allowlist), so layeringvirtualenv_subprocess_env()on top here is safe — no leak risk like in codex-cli's allowlisted build_env.tests/adapters/test_adapaters_common_utils.py (2)
7-7: LGTM!
16-58: LGTM!
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
…ython Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/fabric-core/src/error.rs (1)
150-157: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winError message omits the resolved
pathfield.
pathis captured on the variant but never surfaced in the#[error(...)]message — onlyvalueis shown. IfADAPTER_PYTHONis set to a relative path, users won't see what absolute path Fabric actually tried to resolve, making the error harder to debug.♻️ Suggested message tweak
- #[error("environment variable `ADAPTER_PYTHON` (`{value}`) must point to a valid file")] + #[error("environment variable `ADAPTER_PYTHON` (`{value}`) resolved to `{path}`, which does not exist")]🤖 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 `@crates/fabric-core/src/error.rs` around lines 150 - 157, The InvalidAdapterPython error message currently only reports the raw ADAPTER_PYTHON value and omits the resolved path, so update the #[error(...)] on the InvalidAdapterPython variant in error.rs to include both value and path. Keep the existing variant fields unchanged, but surface the path field in the formatted message so callers can see the absolute path Fabric tried to validate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/fabric-core/src/error.rs`:
- Around line 150-157: The InvalidAdapterPython error message currently only
reports the raw ADAPTER_PYTHON value and omits the resolved path, so update the
#[error(...)] on the InvalidAdapterPython variant in error.rs to include both
value and path. Keep the existing variant fields unchanged, but surface the path
field in the formatted message so callers can see the absolute path Fabric tried
to validate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 65e39392-a122-4060-82f9-8cef18b8275c
📒 Files selected for processing (6)
README.mdadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.pyadapters/common/src/nemo_fabric_adapters/common/utils.pyadapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.pycrates/fabric-core/src/error.rstests/adapters/test_adapaters_common_utils.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (17)
{README.md,docs/index.yml,docs/**/*.{md,mdx}}
📄 CodeRabbit inference engine (.agents/skills/contribute-docs/SKILL.md)
Update entry-point docs when examples or reading paths change
Files:
README.md
{README.md,docs/index.yml}
📄 CodeRabbit inference engine (.agents/skills/contribute-docs/SKILL.md)
Update
README.mdordocs/index.ymlwhen entry points changed
Files:
README.md
**/README.md
📄 CodeRabbit inference engine (.agents/skills/contribute-docs/SKILL.md)
Update relevant adapter or example
README.mdfiles when examples or adapters have changed
Files:
README.md
**/*.{md,mdx}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-brand-terminology.md)
**/*.{md,mdx}: SpellNVIDIAin all caps; do not useNvidia,nvidia,nVidia,nVIDIA, orNV.
Usean NVIDIAbefore a noun becauseNVIDIAstarts with an "en" sound.
Do not add a registered trademark symbol afterNVIDIAwhen referring to the company.
Use trademark symbols with product names only when the document type or legal guidance requires them.
Verify official capitalization, spacing, and hyphenation for NVIDIA product names.
Precede NVIDIA product names withNVIDIAon first mention when it is natural and accurate.
Link the first mention of a product name when the destination helps the reader.
Do not rewrite product names for grammar or title-case rules.
Preserve third-party product names according to the owner's spelling.
Include the company name and full model qualifier on first use when it helps identify the model.
Preserve the official capitalization and punctuation of model names.
Use shorter family names only after the full model name is established.
For learning-oriented docs, technical blog posts, GTC sessions, tutorials, and developer guides: do not force trademark symbols unless the source, platform, or legal guidance explicitly requires them.
For learning-oriented docs, technical blog posts, GTC sessions, tutorials, and developer guides: keep the product name accurate and consistent.
For press releases, product landing pages, packaging, sales content, or legal copy: follow the current NVIDIA trademark and copyright guidance.
For press releases, product landing pages, packaging, sales content, or legal copy: attribute trademarks on first use when required.
For press releases, product landing pages, packaging, sales content, or legal copy: use the required trademark symbol for the specific product or service.
For press releases, product landing pages, packaging, sales content, or legal copy: do not invent trademark attributions; verify current legal copy.
If the platform requires legal copy, confirm the current source of truth instead...
Files:
README.md
**/*.{md,mdx,rst}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-technical-docs.md)
**/*.{md,mdx,rst}: When reviewing technical documentation, verify that commands, examples, paths, APIs, and support claims match the current repository.
Make technical documents easy to scan by fixing headings, lead-in sentences, lists, tables, and procedure shape.
Preserve exact code, command, API, package, and UI strings unless they are factually wrong.
Prefer focused findings over broad rewrites.
Use title case consistently in technical documentation headings.
Avoid quotation marks, ampersands, and exclamation marks in technical documentation headings.
Keep product, event, research, and whitepaper names in their official title case.
Use title case for table headers.
Do not force social-media sentence case into technical documentation.
Format code elements, commands, parameters, package names, and expressions in monospace.
Format directories, file names, and paths in monospace.
Use angle brackets inside monospace for variables inside paths.
Format error messages and strings with quotation marks, using code formatting when that is clearer.
Format UI buttons, menus, fields, and labels in bold.
Use angle brackets between UI labels for menu paths.
Use italics on first use for new terms, and only when the term is introduced.
Italicize publication titles.
Write keyboard shortcuts in plain text.
Use Owner/repo link text for GitHub repositories.
Introduce every code block with a complete sentence.
Do not make a code block complete the grammar of the previous sentence.
Do not continue a sentence after a code block.
Use syntax highlighting when the format supports it.
Avoid the word "snippet" unless the surrounding documentation already uses it as a term of art.
Keep inline method, function, and class references consistent with nearby docs, and omit empty parentheses in prose when no call is shown.
Use descriptive anchor text that matches the destination title when possible.
Avoid raw URLs in running text.
Avoid generic anchors such as "here," "this page," and "read more....
Files:
README.md
**/*.{md,mdx,rst,txt}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*.{md,mdx,rst,txt}: If documentation or examples changed, runjust docswhen practical and verify documented commands against the current repository.
For documentation-only changes, usecontribute-docsandreview-doc-style; runjust docsfor docs-site or generated-reference changes.
Files:
README.md
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
For CI or packaging changes, use
maintain-ciormaintain-packaging, then run the recipes and checks whose behavior changed.
Files:
README.mdtests/adapters/test_adapaters_common_utils.pyadapters/common/src/nemo_fabric_adapters/common/utils.pycrates/fabric-core/src/error.rsadapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.pyadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py
{docs/**,README.md,AGENTS.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,AGENTS.md}: Review documentation for technical accuracy against the current API, command correctness, and consistency with generated schemas.
Files:
README.md
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Usepytestto run Python tests.
Do not add@pytest.mark.asyncioto test functions; async tests should be auto-detected and run 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, usingspecwhen necessary.
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.
When creating a fixture, use the pattern@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])followed bydef <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 you need to modify environment variables in a test, useos.environ;tests/conftest.pyprovides an autouserestore_environ_fixturethat restores the environment after each test, somonkeypatch.setenvis unnecessary.
Avoid defensive programming in tests; access expected dictionary keys directly (for example,results["data"]) instead of using.get(), so failures raise loudly and clearly.
Files:
tests/adapters/test_adapaters_common_utils.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*.{py,pyi}: If Python code or a Python-facing adapter changed, runjust test-python.
For Python SDK or PyO3 binding changes, usepython-tests, run focused pytest tests first, then runjust test-python; rebuild withjust build-pythonwhen native code or packaging changed.
Files:
tests/adapters/test_adapaters_common_utils.pyadapters/common/src/nemo_fabric_adapters/common/utils.pyadapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.pyadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py
tests/adapters/**/*.py
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
tests/adapters/**/*.py: If an adapter or integration changed, run its focused tests.
For adapter behavior changes, run the focused adapter tests undertests/adapters, thenjust test-python.
Files:
tests/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_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-cli/src/nemo_fabric_adapters/hermes_cli/adapter.pyadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*.rs: If Rust code changed, runcargo fmt --all -- --checkandjust test-rust.
If public configuration types changed, confirm the schema snapshot tests injust test-rustpass and review generated schema diffs.
For Rust core, CLI, or shared runtime semantics changes, run Rust formatting and tests; add Python tests when the behavior is exposed through the SDK, and run relevant tests for CLI behavior.
Files:
crates/fabric-core/src/error.rs
crates/fabric-core/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If
crates/fabric-corechanged in a way exposed through Python, run both the Rust and Python suites.
Files:
crates/fabric-core/src/error.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If the PyO3 bridge or package metadata changed, run
just build-pythonandcargo check -p fabric-python --locked.
Files:
crates/fabric-core/src/error.rs
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/error.rs
🧬 Code graph analysis (1)
adapters/common/src/nemo_fabric_adapters/common/utils.py (1)
tests/adapters/test_adapaters_common_utils.py (1)
virtualenv_subprocess_env(52-52)
🔇 Additional comments (5)
README.md (1)
103-108: LGTM!adapters/common/src/nemo_fabric_adapters/common/utils.py (1)
33-54: Fullos.environcopy still returned unconditionally; downstream allowlist bypass persists.
virtualenv_subprocess_env()now always starts fromos.environ.copy(), fixing the earlier test-mismatch, but still gives callers the entire process environment rather than just the virtualenv delta. This was previously flagged as a critical leak vector when merged into allowlisted adapter environments (seeadapters/codex-cli/.../adapter.py), and that risk is unresolved here — the suggestedbase_envparameter fix from the prior review was not adopted.adapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py (1)
456-475: Allowlist removed entirely — codex CLI subprocess now inherits the full process environment unconditionally.
INHERITED_ENV_NAMESfiltering was dropped, andbuild_envnow seedsenvfromvirtualenv_subprocess_env(), which returns a fullos.environcopy regardless of venv state. Previously the leak was conditional on running inside a virtualenv; now every fabric process env var (potential secrets/tokens) is passed to the codex CLI subprocess unconditionally. Same root-cause concern raised previously, now broader in scope since the allowlist itself is gone rather than merely bypassed.adapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.py (1)
181-192: LGTM!tests/adapters/test_adapaters_common_utils.py (1)
61-70: LGTM!
ericevans-nv
left a comment
There was a problem hiding this comment.
Approving since the core ADAPTER_PYTHON behavior looks good and the Codex env inheritance is intentional. I left one suggested guard for --show-output so it doesn’t raise on a successful result that lacks output.response; that can be applied here or handled as a follow-up if you want to formalize response as part of the output contract separately.
Co-authored-by: Eric Evans II <194135482+ericevans-nv@users.noreply.github.com> Signed-off-by: David Gardner <dagardner@nvidia.com>
c458b77 to
86f2b39
Compare
Signed-off-by: David Gardner <dagardner@nvidia.com>
Overview
HERMES_PYTHONidea used in the Hermes Agent exampleADAPTER_PYTHONfunctions as the default value for thepython_envsetting, when unset follows existing fallback rules.PATHand other environment variables to ensure sub-processes are run within the virtualenv.Unrelated changes:
--show-outputflag toexamples/code_review_agent/__main__.py, to make reading the output easierWhere should the reviewer start?
crates/fabric-core/src/runtime.rsRelated Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Closes FABRIC-56
I confirm this contribution is my own work, or I have the right to submit it under this project's license.
I searched existing issues and open pull requests, and this does not duplicate existing work.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
--show-outputoption to display the adapter response after the normalized result.ADAPTER_PYTHONsupport for selecting the adapter Python interpreter.Bug Fixes
ADAPTER_PYTHON.Documentation
ADAPTER_PYTHONand clarified precedence.Chores / Tests