ci: Add python versions to test matrix - #61
Conversation
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
WalkthroughThe CI test matrix now covers Python 3.11–3.14 on x86_64 and arm64. Hermes dependencies are excluded on Python 3.14+, while Harbor- and Hermes-dependent tests use availability gating instead of import-time failures or local Harbor stubs. ChangesVersion-aware CI and optional integration tests
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
📖 Fern docs preview: https://nvidia-preview-pull-request-61.docs.buildwithfern.com/nemo/fabric |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integrations/test_harbor_runner.py`:
- Around line 28-30: Replace the autouse fixture requires_harbor_fixture with
the same module-level pytestmark pattern used in test_harbor_integration.py.
Preserve the session-wide Harbor requirement while removing the no-teardown
yield wrapper and duplicated fixture boilerplate.
In `@tests/python/test_harbor_integration.py`:
- Around line 18-20: Replace the same-name requires_harbor_fixture override with
a module-level pytestmark that applies the existing requires_harbor fixture via
pytest.mark.usefixtures, matching the pattern in
tests/e2e/test_harbor_swebench_task.py. Remove the redundant fixture function
and retain the session-wide Harbor requirement.
🪄 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: 6a6a9692-d714-4b88-99e2-96e4c426a1b3
📒 Files selected for processing (5)
.github/workflows/ci_python.ymltests/conftest.pytests/e2e/test_harbor_swebench_task.pytests/integrations/test_harbor_runner.pytests/python/test_harbor_integration.py
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: Build and publish docs
- GitHub Check: Build wheels (x86_64)
- GitHub Check: Test (Python 3.12, x86_64)
- GitHub Check: Test (Python 3.11, x86_64)
- GitHub Check: Test (Python 3.11, arm64)
- GitHub Check: Test (Python 3.13, x86_64)
- GitHub Check: Test (Python 3.13, arm64)
- GitHub Check: Test (Python 3.12, arm64)
- GitHub Check: Build wheels (arm64)
- GitHub Check: Pre-commit
🧰 Additional context used
📓 Path-based instructions (12)
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.py: Python public APIs must use type annotations, and native Python binding declarations must remain synchronized with their Rust implementations.
Python files must begin with the specified#SPDX copyright and Apache-2.0 license header.
Files:
tests/integrations/test_harbor_runner.pytests/conftest.pytests/e2e/test_harbor_swebench_task.pytests/python/test_harbor_integration.py
**/*.{rs,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{rs,py}: Usesnake_casefor Rust and Python functions and variables; usePascalCasefor Rust types and Python classes.
Run tests for every language surface affected by a change. Changes touching the Rust core or public schemas require both Rust and Python test suites.
Public contract changes must keep native Python binding declarations synchronized with their Rust implementations.
Files:
tests/integrations/test_harbor_runner.pytests/conftest.pytests/e2e/test_harbor_swebench_task.pytests/python/test_harbor_integration.py
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: All source files must include the specified SPDX copyright and Apache-2.0 license header using the comment syntax appropriate to the file type.
Release tags must use raw Rust-compatible SemVer without a leadingv, such as0.1.0or0.1.0-rc.1.
**/*: Before implementing, explicitly state assumptions, surface ambiguity and tradeoffs, present multiple interpretations when relevant, and ask for clarification rather than silently deciding or proceeding when requirements are unclear.
Prefer the minimum code needed to solve the requested problem: avoid speculative features, unnecessary abstractions, unrequested flexibility, and handling of impossible scenarios; simplify overcomplicated solutions.
When editing existing code, make surgical changes only: do not modify unrelated code, comments, formatting, or pre-existing dead code; match the existing style, and remove only unused imports, variables, or functions introduced by your changes.
Define verifiable success criteria for each task, such as writing regression tests for bugs and invalid-input tests for validation, then verify the implementation against those criteria. For multi-step work, state a brief plan with a verification check for each step.
**/*: Keep pull request branch scope coherent and reviewable.
Run relevant tests undervalidate-changebefore opening or updating a pull request.
Format changed files with the language-native formatter.
Update documentation and examples for public behavior changes.
Update dependent maintainer or consumer guidance when code changes affect APIs, bindings, commands, paths, packaging guidance, or best practices.
Use Conventional Commit style for pull request titles:<type>: <concise imperative summary>, choosing the type from the actual change surface. Usefixonly for user-facing or runtime product-code bug fixes.
A pull request body must include#### Overview,#### Details,#### Validation,#### Where should the reviewer start?, and `#### Related ...
Files:
tests/integrations/test_harbor_runner.pytests/conftest.pytests/e2e/test_harbor_swebench_task.pytests/python/test_harbor_integration.py
**/*.{rs,py,pyi,json,yaml,yml}
📄 CodeRabbit inference engine (.agents/skills/contribute-api/SKILL.md)
Determine and update every affected public surface, including the CLI, PyO3 bindings, Python SDK, type stubs, schemas, and adapter contract, so they remain in parity.
Files:
tests/integrations/test_harbor_runner.pytests/conftest.pytests/e2e/test_harbor_swebench_task.pytests/python/test_harbor_integration.py
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Use Pytest to run Python tests.
Do not add@pytest.mark.asyncioto tests; async tests are automatically detected by the async runner.
Do not add-> Nonereturn annotations to test functions.
When mocking a class, useunittest.mock.MagicMockorAsyncMock, supplyingspecwhen necessary; do not define a new mock class.
Prefix mocked class names withmock, notfake.
Prefer pytest fixtures over helper methods.
Define shared fixtures inconftest.pyrather than repeating them across test files.
Define fixtures using@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and a<fixture_name>_fixturefunction; specifyscopeonly when it is notfunction.
Preferpytest.mark.parametrizeover separate tests for different input types.
Use@pytest.mark.usefixtureswhen a fixture is needed but its return value is unused.
Useos.environto modify environment variables in tests; do not usemonkeypatch.setenv, because the autouserestore_environ_fixtureintests/conftest.pyrestores the environment after each test.
Avoid defensive programming in tests; access expected data directly so missing data raises a clear error instead of being silently tolerated.
Run focused tests withuv run pytest -k "<pattern>"and all tests withuv run pytest.
Files:
tests/integrations/test_harbor_runner.pytests/conftest.pytests/e2e/test_harbor_swebench_task.pytests/python/test_harbor_integration.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
When Python code or a Python-facing adapter changes, run
just test-python.
Files:
tests/integrations/test_harbor_runner.pytests/conftest.pytests/e2e/test_harbor_swebench_task.pytests/python/test_harbor_integration.py
**/*.{rs,py,pyi,toml}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
When the PyO3 bridge or package metadata changes, run
just build-pythonandcargo check -p fabric-python --locked.
Files:
tests/integrations/test_harbor_runner.pytests/conftest.pytests/e2e/test_harbor_swebench_task.pytests/python/test_harbor_integration.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/integrations/test_harbor_runner.pytests/conftest.pytests/e2e/test_harbor_swebench_task.pytests/python/test_harbor_integration.py
**/.github/workflows/*.{yml,yaml}
📄 CodeRabbit inference engine (.agents/skills/maintain-ci/SKILL.md)
**/.github/workflows/*.{yml,yaml}: Definepermissions:per job that needs token access, using the minimum required scopes; avoid workflow-level permissions unless centralized inheritance is intentionally documented.
Pin every third-party GitHub Action to a full commit SHA and retain a readable version comment after the SHA.
Use action-native or ecosystem-native caching instead of genericactions/cachewhen available.
Tie cache invalidation to lockfiles, dependency manifests, or explicit tool versions; avoid caching generated outputs unless deliberate repository behavior requires it.
Useastral-sh/setup-uvcache support withcache-dependency-globanchored touv.lock.
UseSwatinem/rust-cachewith explicitshared-keyandworkspacesinstead of ad hoc target-directory caching.
Keep deploy and publish permissions isolated to only the jobs that require them; restrictpages: writeandid-token: writeto Pages deployment jobs and their reusable-workflow callers.
Usecontents: readas the minimum permission for checkout-based build, test, documentation, and packaging jobs.
Grantpull-requests: readonly to jobs that perform pull-request metadata lookups.
When usingworkflow_call, inspect both caller and callee, and ensure the caller grants every permission required by the called jobs because the callee cannot elevate permissions.
Keep local workflow commands aligned with equivalentjustfilerecipes, and keep Python, Rust, and documentation jobs aligned with their lockfiles.
Preserve intended concurrency settings, branch filters, and documentation publish guards when modifying workflows.
Before editing workflows, inspect workflow relationships and settings with searches coveringuses:,permissions:,secrets:,concurrency:,cache, andjust.
Files:
.github/workflows/ci_python.yml
{docs/**/*,.github/workflows/ci_python.yml,.github/workflows/ci_rust.yml,justfile}
📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md)
Use the current install, import, build, test, clean, and documentation commands consistently in documentation, examples, CI workflows, and just recipes.
Files:
.github/workflows/ci_python.yml
{docs/**/*,.github/workflows/ci_python.yml,.github/workflows/ci_rust.yml}
📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md)
Reflect public packaging changes in release-facing documentation and examples.
Files:
.github/workflows/ci_python.yml
{.github/workflows/ci_python.yml,.github/workflows/ci_rust.yml,justfile,pyproject.toml,python/pyproject.toml,Cargo.toml}
📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md)
CI workflows must reference the same package names and commands used by local build and installation workflows.
Files:
.github/workflows/ci_python.yml
🪛 Ruff (0.15.21)
tests/integrations/test_harbor_runner.py
[warning] 30-30: No teardown in fixture requires_harbor_fixture, use return instead of yield
Replace yield with return
(PT022)
tests/python/test_harbor_integration.py
[warning] 20-20: No teardown in fixture requires_harbor_fixture, use return instead of yield
Replace yield with return
(PT022)
🪛 zizmor (1.26.1)
.github/workflows/ci_python.yml
[warning] 86-86: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 87-87: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🔇 Additional comments (4)
.github/workflows/ci_python.yml (2)
39-44: LGTM!
86-87: 🔒 Security & PrivacyNo change needed.
harboralready requires Python >= 3.12, so the3.11exclusion matches the package floor.tests/conftest.py (1)
18-26: LGTM!tests/e2e/test_harbor_swebench_task.py (1)
31-31: LGTM!
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci_python.yml:
- Line 87: Update the test dependency installation step in the CI workflow to
pass matrix.python-version through an env variable and reference that
environment variable in the shell conditionals instead of interpolating the
matrix value directly in run. Preserve the existing conditional inclusion of the
harbor and hermes extras.
- Around line 86-87: Align the Harbor extra selection in the CI matrix with
pyproject.toml’s Python 3.12+ requirement instead of checking that the version
is not 3.11. Update the workflow matrix or its derived extras metadata to
explicitly mark Harbor availability per Python version, then use that marker in
the uv sync step; preserve the existing Hermes gating 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: abbfd1db-0618-4802-a4e5-b3a71906a9f1
⛔ Files ignored due to path filters (2)
adapters/hermes/uv.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.github/workflows/ci_python.ymladapters/hermes/pyproject.tomlpyproject.tomltests/adapters/test_hermes_adapter.pytests/conftest.pytests/e2e/test_hermes_config_mapping.pytests/e2e/test_hermes_e2e.py
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: Build and publish docs
- GitHub Check: Test (Python 3.12, x86_64)
- GitHub Check: Test (Python 3.14, arm64)
- GitHub Check: Test (Python 3.13, arm64)
- GitHub Check: Test (Python 3.14, x86_64)
- GitHub Check: Test (Python 3.13, x86_64)
- GitHub Check: Test (Python 3.12, arm64)
- GitHub Check: Test (Python 3.11, x86_64)
- GitHub Check: Test (Python 3.11, arm64)
- GitHub Check: Pre-commit
🧰 Additional context used
📓 Path-based instructions (21)
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{rs,toml}: Rust code must be formatted withcargo fmt --all; formatting can be checked withcargo fmt --all -- --check, and Rust workspaces must compile withcargo check --workspace --locked.
Rust files must begin with the specified//SPDX copyright and Apache-2.0 license header.When Rust code or Rust project configuration changes, run
cargo fmt --all -- --checkandjust test-rust.
Files:
adapters/hermes/pyproject.tomlpyproject.toml
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: All source files must include the specified SPDX copyright and Apache-2.0 license header using the comment syntax appropriate to the file type.
Release tags must use raw Rust-compatible SemVer without a leadingv, such as0.1.0or0.1.0-rc.1.
**/*: Before implementing, explicitly state assumptions, surface ambiguity and tradeoffs, present multiple interpretations when relevant, and ask for clarification rather than silently deciding or proceeding when requirements are unclear.
Prefer the minimum code needed to solve the requested problem: avoid speculative features, unnecessary abstractions, unrequested flexibility, and handling of impossible scenarios; simplify overcomplicated solutions.
When editing existing code, make surgical changes only: do not modify unrelated code, comments, formatting, or pre-existing dead code; match the existing style, and remove only unused imports, variables, or functions introduced by your changes.
Define verifiable success criteria for each task, such as writing regression tests for bugs and invalid-input tests for validation, then verify the implementation against those criteria. For multi-step work, state a brief plan with a verification check for each step.
**/*: Keep pull request branch scope coherent and reviewable.
Run relevant tests undervalidate-changebefore opening or updating a pull request.
Format changed files with the language-native formatter.
Update documentation and examples for public behavior changes.
Update dependent maintainer or consumer guidance when code changes affect APIs, bindings, commands, paths, packaging guidance, or best practices.
Use Conventional Commit style for pull request titles:<type>: <concise imperative summary>, choosing the type from the actual change surface. Usefixonly for user-facing or runtime product-code bug fixes.
A pull request body must include#### Overview,#### Details,#### Validation,#### Where should the reviewer start?, and `#### Related ...
Files:
adapters/hermes/pyproject.tomlpyproject.tomltests/adapters/test_hermes_adapter.pytests/e2e/test_hermes_config_mapping.pytests/e2e/test_hermes_e2e.pytests/conftest.py
**/*.{toml,yaml,yml,sh,bash}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
TOML, YAML, and shell files must use the specified SPDX header with
#comments.
Files:
adapters/hermes/pyproject.tomlpyproject.toml
**/pyproject.toml
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
Run
just build-pythonto verify that all Python package metadata resolves.
Files:
adapters/hermes/pyproject.tomlpyproject.toml
**/*.{rs,py,pyi,toml}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
When the PyO3 bridge or package metadata changes, run
just build-pythonandcargo check -p fabric-python --locked.
Files:
adapters/hermes/pyproject.tomlpyproject.tomltests/adapters/test_hermes_adapter.pytests/e2e/test_hermes_config_mapping.pytests/e2e/test_hermes_e2e.pytests/conftest.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/hermes/pyproject.toml
{Cargo.toml,pyproject.toml,python/pyproject.toml,python/src/nemo_fabric/**}
📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md)
{Cargo.toml,pyproject.toml,python/pyproject.toml,python/src/nemo_fabric/**}: Keep Rust package names, Python package/import paths, and native module names internally consistent across Cargo and Python packaging metadata and source paths.
Ensure generated native and Python artifacts are placed where downstream consumers expect them.
Files:
pyproject.toml
{Cargo.toml,pyproject.toml,python/pyproject.toml,Cargo.lock,uv.lock}
📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md)
Keep workspace, Rust, Python, and lockfile versions aligned where required.
Files:
pyproject.toml
{pyproject.toml,python/pyproject.toml,Cargo.toml,python/src/nemo_fabric/**}
📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md)
The editable maturin build must continue to produce the
nemo_fabric._nativeextension.
Files:
pyproject.toml
{.github/workflows/ci_python.yml,.github/workflows/ci_rust.yml,justfile,pyproject.toml,python/pyproject.toml,Cargo.toml}
📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md)
CI workflows must reference the same package names and commands used by local build and installation workflows.
Files:
pyproject.toml.github/workflows/ci_python.yml
{Cargo.toml,Cargo.lock,pyproject.toml,python/pyproject.toml,uv.lock,docs/package.json,docs/package-lock.json}
📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md)
Maintain package metadata and dependency resolution consistently across Rust, Python, documentation tooling, and their lockfiles.
Files:
pyproject.toml
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.py: Python public APIs must use type annotations, and native Python binding declarations must remain synchronized with their Rust implementations.
Python files must begin with the specified#SPDX copyright and Apache-2.0 license header.
Files:
tests/adapters/test_hermes_adapter.pytests/e2e/test_hermes_config_mapping.pytests/e2e/test_hermes_e2e.pytests/conftest.py
**/*.{rs,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{rs,py}: Usesnake_casefor Rust and Python functions and variables; usePascalCasefor Rust types and Python classes.
Run tests for every language surface affected by a change. Changes touching the Rust core or public schemas require both Rust and Python test suites.
Public contract changes must keep native Python binding declarations synchronized with their Rust implementations.
Files:
tests/adapters/test_hermes_adapter.pytests/e2e/test_hermes_config_mapping.pytests/e2e/test_hermes_e2e.pytests/conftest.py
**/*.{rs,py,pyi,json,yaml,yml}
📄 CodeRabbit inference engine (.agents/skills/contribute-api/SKILL.md)
Determine and update every affected public surface, including the CLI, PyO3 bindings, Python SDK, type stubs, schemas, and adapter contract, so they remain in parity.
Files:
tests/adapters/test_hermes_adapter.pytests/e2e/test_hermes_config_mapping.pytests/e2e/test_hermes_e2e.pytests/conftest.py
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Use Pytest to run Python tests.
Do not add@pytest.mark.asyncioto tests; async tests are automatically detected by the async runner.
Do not add-> Nonereturn annotations to test functions.
When mocking a class, useunittest.mock.MagicMockorAsyncMock, supplyingspecwhen necessary; do not define a new mock class.
Prefix mocked class names withmock, notfake.
Prefer pytest fixtures over helper methods.
Define shared fixtures inconftest.pyrather than repeating them across test files.
Define fixtures using@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and a<fixture_name>_fixturefunction; specifyscopeonly when it is notfunction.
Preferpytest.mark.parametrizeover separate tests for different input types.
Use@pytest.mark.usefixtureswhen a fixture is needed but its return value is unused.
Useos.environto modify environment variables in tests; do not usemonkeypatch.setenv, because the autouserestore_environ_fixtureintests/conftest.pyrestores the environment after each test.
Avoid defensive programming in tests; access expected data directly so missing data raises a clear error instead of being silently tolerated.
Run focused tests withuv run pytest -k "<pattern>"and all tests withuv run pytest.
Files:
tests/adapters/test_hermes_adapter.pytests/e2e/test_hermes_config_mapping.pytests/e2e/test_hermes_e2e.pytests/conftest.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
When Python code or a Python-facing adapter changes, run
just test-python.
Files:
tests/adapters/test_hermes_adapter.pytests/e2e/test_hermes_config_mapping.pytests/e2e/test_hermes_e2e.pytests/conftest.py
tests/adapters/**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
When an adapter or integration changes, run its focused tests under
tests/adapters, followed byjust test-python.
Files:
tests/adapters/test_hermes_adapter.py
{tests/**,python/tests/**}
⚙️ CodeRabbit configuration file
{tests/**,python/tests/**}: Tests should cover the behavior promised by the changed API surface, including error paths, lifecycle cleanup, and SDK/native parity where relevant.
Files:
tests/adapters/test_hermes_adapter.pytests/e2e/test_hermes_config_mapping.pytests/e2e/test_hermes_e2e.pytests/conftest.py
**/.github/workflows/*.{yml,yaml}
📄 CodeRabbit inference engine (.agents/skills/maintain-ci/SKILL.md)
**/.github/workflows/*.{yml,yaml}: Definepermissions:per job that needs token access, using the minimum required scopes; avoid workflow-level permissions unless centralized inheritance is intentionally documented.
Pin every third-party GitHub Action to a full commit SHA and retain a readable version comment after the SHA.
Use action-native or ecosystem-native caching instead of genericactions/cachewhen available.
Tie cache invalidation to lockfiles, dependency manifests, or explicit tool versions; avoid caching generated outputs unless deliberate repository behavior requires it.
Useastral-sh/setup-uvcache support withcache-dependency-globanchored touv.lock.
UseSwatinem/rust-cachewith explicitshared-keyandworkspacesinstead of ad hoc target-directory caching.
Keep deploy and publish permissions isolated to only the jobs that require them; restrictpages: writeandid-token: writeto Pages deployment jobs and their reusable-workflow callers.
Usecontents: readas the minimum permission for checkout-based build, test, documentation, and packaging jobs.
Grantpull-requests: readonly to jobs that perform pull-request metadata lookups.
When usingworkflow_call, inspect both caller and callee, and ensure the caller grants every permission required by the called jobs because the callee cannot elevate permissions.
Keep local workflow commands aligned with equivalentjustfilerecipes, and keep Python, Rust, and documentation jobs aligned with their lockfiles.
Preserve intended concurrency settings, branch filters, and documentation publish guards when modifying workflows.
Before editing workflows, inspect workflow relationships and settings with searches coveringuses:,permissions:,secrets:,concurrency:,cache, andjust.
Files:
.github/workflows/ci_python.yml
{docs/**/*,.github/workflows/ci_python.yml,.github/workflows/ci_rust.yml,justfile}
📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md)
Use the current install, import, build, test, clean, and documentation commands consistently in documentation, examples, CI workflows, and just recipes.
Files:
.github/workflows/ci_python.yml
{docs/**/*,.github/workflows/ci_python.yml,.github/workflows/ci_rust.yml}
📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md)
Reflect public packaging changes in release-facing documentation and examples.
Files:
.github/workflows/ci_python.yml
🪛 zizmor (1.26.1)
.github/workflows/ci_python.yml
[warning] 87-87: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 87-87: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🔇 Additional comments (9)
adapters/hermes/pyproject.toml (1)
26-27: LGTM!pyproject.toml (1)
53-55: LGTM! Version markers correctly gate Hermes deps for Python <3.14, consistent withadapters/hermes/pyproject.toml's tightenedrequires-python.As per coding guidelines, "Run
just build-pythonto verify that all Python package metadata resolves" should be run for this pyproject.toml change.Also applies to: 74-77, 88-95
.github/workflows/ci_python.yml (1)
43-44: LGTM!tests/conftest.py (2)
18-34:requires_harborfixture not visible in provided diff.The line-range summary states a new
requires_harborfixture is introduced in this range, immediately precedingrequires_hermes_agent, but the annotated snippet marks lines 1-26 as unchanged and only showsrequires_hermes_agent(lines 27-34) as changed. Please confirmrequires_harborfollows the samescope="session"/<name>_fixturepattern and correctly gates on Harbor's actual import failure mode (e.g.,ModuleNotFoundErrorfor theharborpackage), since this fixture underpins the Harbor test-gating cohort (tests/python/test_harbor_integration.py,tests/integrations/test_harbor_runner.py,tests/e2e/test_harbor_swebench_task.py) referenced elsewhere in the PR stack.
27-34: LGTM! Matches the required@pytest.fixture(name=..., scope=...)+<fixture_name>_fixtureconvention.As per coding guidelines, "Define fixtures using
@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and a<fixture_name>_fixturefunction".Source: Coding guidelines
tests/adapters/test_hermes_adapter.py (2)
8-8: LGTM! Skip-before-execute ordering (session-scoped fixture skip precedes test body execution) makes deferred references inside test bodies safe.As per coding guidelines, "When Python code or a Python-facing adapter changes, run
just test-python" for this Hermes adapter test change.Also applies to: 31-38, 309-422
Source: Coding guidelines
19-27: 🩺 Stability & AvailabilityNo import-time reference to the conditional imports here. The decorators and annotations in this file don’t touch
SessionDB,AIAgent,common_utils, oradapterbefore the optional import block runs.> Likely an incorrect or invalid review comment.tests/e2e/test_hermes_config_mapping.py (1)
8-18: LGTM!tests/e2e/test_hermes_e2e.py (1)
21-22: 🩺 Stability & AvailabilityNo issue here. The session-scoped
requires_hermes_agentfixture runs before the function-scopedrun_hermes_with_relayautouse fixture, so removing the inlineimportorskipdoes not change the early-skip behavior.> Likely an incorrect or invalid review comment.
|
/merge |
2 participants
Overview
Where should the reviewer start?
.github/workflows/ci_python.ymlRelated Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Closes FABRIC-52
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